Re: [PATCH v7 1/3] usb: cdns3: introduce set_phy_power_on{off} APIs

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

On 20-08-27 16:09:36, Felipe Balbi wrote:
> Peter Chen <peter.chen@nxp.com> writes:
> 
> > Since we have both USB2 and USB3 PHYs for cdns3 controller, it is
> > better we have unity APIs to handle both USB2 and USB3's power, it
> > could simplify code for error handling and further power management
> > implementation.
> >
> > Reviewed-by: Pawel Laszczak <pawell@cadence.com>
> > Reviewed-by: Jun Li <jun.li@nxp.com>
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/cdns3/core.c | 43 ++++++++++++++++++++++++++--------------
> >  1 file changed, 28 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > index 5c1586ec7824..e56dbb6a898c 100644
> > --- a/drivers/usb/cdns3/core.c
> > +++ b/drivers/usb/cdns3/core.c
> > @@ -371,6 +371,27 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
> >  	return ret;
> >  }
> >  
> > +static int set_phy_power_on(struct cdns3 *cdns)
> 
> care to keep the cdns3_ prefix? It's useful when you want to use
> e.g. ftrace and function_graph trace

I original added it, but Greg suggested it is not needed.

https://www.spinics.net/lists/linux-usb/msg197076.html

> 
> > +{
> > +	int ret;
> > +
> > +	ret = phy_power_on(cdns->usb2_phy);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = phy_power_on(cdns->usb3_phy);
> 
> let's say we fail here
> 
> > +	if (ret)
> > +		phy_power_off(cdns->usb2_phy);
> 
> usb2_phy will be powered off..

If usb3_phy's power_on has failed, it powers off usb2 phy's power which
is powered on before, and return fail.

> 
> > +	return ret;
> > +}
> > +
> > +static void set_phy_power_off(struct cdns3 *cdns)
> > +{
> > +	phy_power_off(cdns->usb3_phy);
> > +	phy_power_off(cdns->usb2_phy);
> 
> Won't this cause a problem?
> 

What's the problem you have seen?

-- 

Thanks,
Peter Chen