Re: [PATCH v5 1/9] usb: cdns3: introduce set_phy_power_on{off} APIs

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

On 20-08-13 12:09:58, Pawel Laszczak wrote:
> 
> >
> >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: 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 19bbb5b7e6b6..8818935d157b 100644
> >--- a/drivers/usb/cdns3/core.c
> >+++ b/drivers/usb/cdns3/core.c
> >@@ -384,6 +384,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)
> >+{
> >+	int ret;
> >+
> >+	ret = phy_power_on(cdns->usb2_phy);
> >+	if (ret)
> >+		return ret;
> >+
> >+	ret = phy_power_on(cdns->usb3_phy);
> >+	if (ret)
> >+		phy_power_off(cdns->usb2_phy);
> >+
> >+	return ret;
> >+}
> >+
> >+static void set_phy_power_off(struct cdns3 *cdns)
> >+{
> >+	phy_power_off(cdns->usb3_phy);
> >+	phy_power_off(cdns->usb2_phy);
> >+}
> >+
> > /**
> >  * cdns3_probe - probe for cdns3 core device
> >  * @pdev: Pointer to cdns3 core platform device
> >@@ -477,14 +498,10 @@ static int cdns3_probe(struct platform_device *pdev)
> > 	if (ret)
> > 		goto err1;
> >
> >-	ret = phy_power_on(cdns->usb2_phy);
> >+	ret = set_phy_power_on(cdns);
> > 	if (ret)
> > 		goto err2;
> >
> >-	ret = phy_power_on(cdns->usb3_phy);
> >-	if (ret)
> >-		goto err3;
> >-
> > 	sw_desc.set = cdns3_role_set;
> > 	sw_desc.get = cdns3_role_get;
> > 	sw_desc.allow_userspace_control = true;
> >@@ -496,16 +513,16 @@ static int cdns3_probe(struct platform_device *pdev)
> > 	if (IS_ERR(cdns->role_sw)) {
> > 		ret = PTR_ERR(cdns->role_sw);
> > 		dev_warn(dev, "Unable to register Role Switch\n");
> >-		goto err4;
> >+		goto err3;
> > 	}
> >
> > 	ret = cdns3_drd_init(cdns);
> > 	if (ret)
> >-		goto err5;
> >+		goto err4;
> >
> > 	ret = cdns3_core_init_role(cdns);
> > 	if (ret)
> >-		goto err5;
> >+		goto err4;
> >
> > 	device_set_wakeup_capable(dev, true);
> > 	pm_runtime_set_active(dev);
> >@@ -522,14 +539,11 @@ static int cdns3_probe(struct platform_device *pdev)
> > 	dev_dbg(dev, "Cadence USB3 core: probe succeed\n");
> >
> > 	return 0;
> >-err5:
> >+err4:
> > 	cdns3_drd_exit(cdns);
> > 	usb_role_switch_unregister(cdns->role_sw);
> >-err4:
> >-	phy_power_off(cdns->usb3_phy);
> >-
> > err3:
> >-	phy_power_off(cdns->usb2_phy);
> >+	set_phy_power_off(cdns);
> > err2:
> > 	phy_exit(cdns->usb3_phy);
> > err1:
> 
> Dan Carpenter suggested me to use more meaningful labels instead err1 .. err5.

Yes, it is reasonable, we could follow this rule when design the new
function.

Peter
>  
> Reviewed-by: Pawel Laszczak<pawell@cadence.com>
> 
> Pawel
> 
> >@@ -553,8 +567,7 @@ static int cdns3_remove(struct platform_device *pdev)
> > 	pm_runtime_put_noidle(&pdev->dev);
> > 	cdns3_exit_roles(cdns);
> > 	usb_role_switch_unregister(cdns->role_sw);
> >-	phy_power_off(cdns->usb2_phy);
> >-	phy_power_off(cdns->usb3_phy);
> >+	set_phy_power_off(cdns);
> > 	phy_exit(cdns->usb2_phy);
> > 	phy_exit(cdns->usb3_phy);
> > 	return 0;
> >--
> >2.17.1
> 

-- 

Thanks,
Peter Chen