| Message ID | 20200703062653.29159-2-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: some PM changes for cdns3 and xhci-plat | expand |
On Fri, Jul 03, 2020 at 02:26:45PM +0800, Peter Chen wrote: > Since we have both USB2 and USB3 PHYs for cdns3 controller, it is > better we have a unity API to handle both USB2 and USB3's power, it > could simplify code for error handling and further power management > implementation. > > Reviewed-by: Jun Li <jun.li@nxp.com> > Signed-off-by: Peter Chen <peter.chen@nxp.com> > --- > drivers/usb/cdns3/core.c | 44 ++++++++++++++++++++++++++-------------- > 1 file changed, 29 insertions(+), 15 deletions(-) > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c > index 19bbb5b7e6b6..bfd09aa98c12 100644 > --- a/drivers/usb/cdns3/core.c > +++ b/drivers/usb/cdns3/core.c > @@ -384,6 +384,28 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role) > return ret; > } > > +static int cdns3_set_phy_power(struct cdns3 *cdns, bool on) Please just make function calls self-describing, instead of having to try to remember what a specific flag means. This isn't as bad as some functions, but the general idea remains, this should be 2 functions: set_phy_power_off() set_phy_power_on() no need for the cdns3_ prefix, it's a static function. Then have those two functions call a helper if you really want to, but that means that reading where those functions are called is obvious and no need to backtrack to where this was declared to determine that the second parameter meant on/off and not is_disabled/not_disabled or something crazy like that. > +{ > + int ret = 0; > + > + if (on) { > + ret = phy_power_on(cdns->usb2_phy); See, phy gets it right. > + if (ret) > + return ret; > + > + ret = phy_power_on(cdns->usb3_phy); > + if (ret) { > + phy_power_off(cdns->usb2_phy); > + return ret; > + } > + } else { > + phy_power_off(cdns->usb3_phy); > + phy_power_off(cdns->usb2_phy); Ick, even worse, this needs to be 2 separate functions as there is NO common code path here at all. thanks, greg k-h
On 20-07-03 09:00:44, Greg KH wrote: > On Fri, Jul 03, 2020 at 02:26:45PM +0800, Peter Chen wrote: > > Since we have both USB2 and USB3 PHYs for cdns3 controller, it is > > better we have a unity API to handle both USB2 and USB3's power, it > > could simplify code for error handling and further power management > > implementation. > > > > Reviewed-by: Jun Li <jun.li@nxp.com> > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > > --- > > drivers/usb/cdns3/core.c | 44 ++++++++++++++++++++++++++-------------- > > 1 file changed, 29 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c > > index 19bbb5b7e6b6..bfd09aa98c12 100644 > > --- a/drivers/usb/cdns3/core.c > > +++ b/drivers/usb/cdns3/core.c > > @@ -384,6 +384,28 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role) > > return ret; > > } > > > > +static int cdns3_set_phy_power(struct cdns3 *cdns, bool on) > > Please just make function calls self-describing, instead of having to > try to remember what a specific flag means. This isn't as bad as some > functions, but the general idea remains, this should be 2 functions: > set_phy_power_off() > set_phy_power_on() > > no need for the cdns3_ prefix, it's a static function. > > Then have those two functions call a helper if you really want to, but > that means that reading where those functions are called is obvious and > no need to backtrack to where this was declared to determine that the > second parameter meant on/off and not is_disabled/not_disabled or > something crazy like that. > > > > > +{ > > + int ret = 0; > > + > > + if (on) { > > + ret = phy_power_on(cdns->usb2_phy); > > See, phy gets it right. > > > + if (ret) > > + return ret; > > + > > + ret = phy_power_on(cdns->usb3_phy); > > + if (ret) { > > + phy_power_off(cdns->usb2_phy); > > + return ret; > > + } > > + } else { > > + phy_power_off(cdns->usb3_phy); > > + phy_power_off(cdns->usb2_phy); > > Ick, even worse, this needs to be 2 separate functions as there is NO > common code path here at all. > The purpose of this API is adding two PHY (USB2 and USB3) APIs into one private APIs since two PHYs power operation are always called together and we will have more than one places to call them. If you think it is not necessary, and make code not easy to read, I could skip this patch.
On Fri, Jul 03, 2020 at 07:16:19AM +0000, Peter Chen wrote: > On 20-07-03 09:00:44, Greg KH wrote: > > On Fri, Jul 03, 2020 at 02:26:45PM +0800, Peter Chen wrote: > > > Since we have both USB2 and USB3 PHYs for cdns3 controller, it is > > > better we have a unity API to handle both USB2 and USB3's power, it > > > could simplify code for error handling and further power management > > > implementation. > > > > > > Reviewed-by: Jun Li <jun.li@nxp.com> > > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > > > --- > > > drivers/usb/cdns3/core.c | 44 ++++++++++++++++++++++++++-------------- > > > 1 file changed, 29 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c > > > index 19bbb5b7e6b6..bfd09aa98c12 100644 > > > --- a/drivers/usb/cdns3/core.c > > > +++ b/drivers/usb/cdns3/core.c > > > @@ -384,6 +384,28 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role) > > > return ret; > > > } > > > > > > +static int cdns3_set_phy_power(struct cdns3 *cdns, bool on) > > > > Please just make function calls self-describing, instead of having to > > try to remember what a specific flag means. This isn't as bad as some > > functions, but the general idea remains, this should be 2 functions: > > set_phy_power_off() > > set_phy_power_on() > > > > no need for the cdns3_ prefix, it's a static function. > > > > Then have those two functions call a helper if you really want to, but > > that means that reading where those functions are called is obvious and > > no need to backtrack to where this was declared to determine that the > > second parameter meant on/off and not is_disabled/not_disabled or > > something crazy like that. > > > > > > > > > +{ > > > + int ret = 0; > > > + > > > + if (on) { > > > + ret = phy_power_on(cdns->usb2_phy); > > > > See, phy gets it right. > > > > > + if (ret) > > > + return ret; > > > + > > > + ret = phy_power_on(cdns->usb3_phy); > > > + if (ret) { > > > + phy_power_off(cdns->usb2_phy); > > > + return ret; > > > + } > > > + } else { > > > + phy_power_off(cdns->usb3_phy); > > > + phy_power_off(cdns->usb2_phy); > > > > Ick, even worse, this needs to be 2 separate functions as there is NO > > common code path here at all. > > > > The purpose of this API is adding two PHY (USB2 and USB3) APIs into one > private APIs since two PHYs power operation are always called together > and we will have more than one places to call them. That's fine, just make 2 separate functions that do this, don't combine it into one that does two totally separate code paths based on a flag that is passed into it. thanks, greg k-h
On 20-07-03 09:23:36, Greg KH wrote: > On Fri, Jul 03, 2020 at 07:16:19AM +0000, Peter Chen wrote: > > On 20-07-03 09:00:44, Greg KH wrote: > > > On Fri, Jul 03, 2020 at 02:26:45PM +0800, Peter Chen wrote: > > > > Since we have both USB2 and USB3 PHYs for cdns3 controller, it is > > > > better we have a unity API to handle both USB2 and USB3's power, it > > > > could simplify code for error handling and further power management > > > > implementation. > > > > > > > > Reviewed-by: Jun Li <jun.li@nxp.com> > > > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > > > > --- > > > > drivers/usb/cdns3/core.c | 44 ++++++++++++++++++++++++++-------------- > > > > 1 file changed, 29 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c > > > > index 19bbb5b7e6b6..bfd09aa98c12 100644 > > > > --- a/drivers/usb/cdns3/core.c > > > > +++ b/drivers/usb/cdns3/core.c > > > > @@ -384,6 +384,28 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role) > > > > return ret; > > > > } > > > > > > > > +static int cdns3_set_phy_power(struct cdns3 *cdns, bool on) > > > > > > Please just make function calls self-describing, instead of having to > > > try to remember what a specific flag means. This isn't as bad as some > > > functions, but the general idea remains, this should be 2 functions: > > > set_phy_power_off() > > > set_phy_power_on() > > > > > > no need for the cdns3_ prefix, it's a static function. > > > > > > Then have those two functions call a helper if you really want to, but > > > that means that reading where those functions are called is obvious and > > > no need to backtrack to where this was declared to determine that the > > > second parameter meant on/off and not is_disabled/not_disabled or > > > something crazy like that. > > > > > > > > > > > > > +{ > > > > + int ret = 0; > > > > + > > > > + if (on) { > > > > + ret = phy_power_on(cdns->usb2_phy); > > > > > > See, phy gets it right. > > > > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = phy_power_on(cdns->usb3_phy); > > > > + if (ret) { > > > > + phy_power_off(cdns->usb2_phy); > > > > + return ret; > > > > + } > > > > + } else { > > > > + phy_power_off(cdns->usb3_phy); > > > > + phy_power_off(cdns->usb2_phy); > > > > > > Ick, even worse, this needs to be 2 separate functions as there is NO > > > common code path here at all. > > > > > > > The purpose of this API is adding two PHY (USB2 and USB3) APIs into one > > private APIs since two PHYs power operation are always called together > > and we will have more than one places to call them. > > That's fine, just make 2 separate functions that do this, don't combine > it into one that does two totally separate code paths based on a flag > that is passed into it. > Thanks, will change.
Greg KH <gregkh@linuxfoundation.org> writes: > On Fri, Jul 03, 2020 at 02:26:45PM +0800, Peter Chen wrote: >> Since we have both USB2 and USB3 PHYs for cdns3 controller, it is >> better we have a unity API to handle both USB2 and USB3's power, it >> could simplify code for error handling and further power management >> implementation. >> >> Reviewed-by: Jun Li <jun.li@nxp.com> >> Signed-off-by: Peter Chen <peter.chen@nxp.com> >> --- >> drivers/usb/cdns3/core.c | 44 ++++++++++++++++++++++++++-------------- >> 1 file changed, 29 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c >> index 19bbb5b7e6b6..bfd09aa98c12 100644 >> --- a/drivers/usb/cdns3/core.c >> +++ b/drivers/usb/cdns3/core.c >> @@ -384,6 +384,28 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role) >> return ret; >> } >> >> +static int cdns3_set_phy_power(struct cdns3 *cdns, bool on) > > Please just make function calls self-describing, instead of having to > try to remember what a specific flag means. This isn't as bad as some > functions, but the general idea remains, this should be 2 functions: > set_phy_power_off() > set_phy_power_on() > > no need for the cdns3_ prefix, it's a static function. the cdns3_ prefix helps when using function trace or function graph trace. It's a lot easier to say "trace anything matching cdns3_*" than it is to remember each function ;-)
diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c index 19bbb5b7e6b6..bfd09aa98c12 100644 --- a/drivers/usb/cdns3/core.c +++ b/drivers/usb/cdns3/core.c @@ -384,6 +384,28 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role) return ret; } +static int cdns3_set_phy_power(struct cdns3 *cdns, bool on) +{ + int ret = 0; + + if (on) { + ret = phy_power_on(cdns->usb2_phy); + if (ret) + return ret; + + ret = phy_power_on(cdns->usb3_phy); + if (ret) { + phy_power_off(cdns->usb2_phy); + return ret; + } + } else { + phy_power_off(cdns->usb3_phy); + phy_power_off(cdns->usb2_phy); + } + + return 0; +} + /** * cdns3_probe - probe for cdns3 core device * @pdev: Pointer to cdns3 core platform device @@ -477,14 +499,10 @@ static int cdns3_probe(struct platform_device *pdev) if (ret) goto err1; - ret = phy_power_on(cdns->usb2_phy); + ret = cdns3_set_phy_power(cdns, true); if (ret) goto err2; - ret = phy_power_on(cdns->usb3_phy); - if (ret) - goto err3; - sw_desc.set = cdns3_role_set; sw_desc.get = cdns3_role_get; sw_desc.allow_userspace_control = true; @@ -496,16 +514,16 @@ static int cdns3_probe(struct platform_device *pdev) if (IS_ERR(cdns->role_sw)) { ret = PTR_ERR(cdns->role_sw); dev_warn(dev, "Unable to register Role Switch\n"); - goto err4; + goto err3; } ret = cdns3_drd_init(cdns); if (ret) - goto err5; + goto err4; ret = cdns3_core_init_role(cdns); if (ret) - goto err5; + goto err4; device_set_wakeup_capable(dev, true); pm_runtime_set_active(dev); @@ -522,14 +540,11 @@ static int cdns3_probe(struct platform_device *pdev) dev_dbg(dev, "Cadence USB3 core: probe succeed\n"); return 0; -err5: +err4: cdns3_drd_exit(cdns); usb_role_switch_unregister(cdns->role_sw); -err4: - phy_power_off(cdns->usb3_phy); - err3: - phy_power_off(cdns->usb2_phy); + cdns3_set_phy_power(cdns, false); err2: phy_exit(cdns->usb3_phy); err1: @@ -553,8 +568,7 @@ static int cdns3_remove(struct platform_device *pdev) pm_runtime_put_noidle(&pdev->dev); cdns3_exit_roles(cdns); usb_role_switch_unregister(cdns->role_sw); - phy_power_off(cdns->usb2_phy); - phy_power_off(cdns->usb3_phy); + cdns3_set_phy_power(cdns, false); phy_exit(cdns->usb2_phy); phy_exit(cdns->usb3_phy); return 0;