Re: [PATCH v4 1/9] usb: cdns3: introduce cdns3_set_phy_power API

[Peter Chen <peter.chen@nxp.com>]

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.

-- 

Thanks,
Peter Chen