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

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1485 bytes --]

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

> +{
> +	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..

> +	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?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]