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

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

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.

-- 

Thanks,
Peter Chen