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@xxxxxxx>
> > > Signed-off-by: Peter Chen <peter.chen@xxxxxxx>
> > > ---
> > > 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
