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

From: Pawel Laszczak <pawell@cadence.com>
To: Peter Chen <peter.chen@nxp.com>,
	"balbi@kernel.org" <balbi@kernel.org>,
	"mathias.nyman@intel.com" <mathias.nyman@intel.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Cc: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-imx@nxp.com" <linux-imx@nxp.com>,
	"rogerq@ti.com" <rogerq@ti.com>,
	"jun.li@nxp.com" <jun.li@nxp.com>
Subject: RE: [PATCH v5 1/9] usb: cdns3: introduce set_phy_power_on{off} APIs
Date: Thu, 13 Aug 2020 12:09:58 +0000	[thread overview]
Message-ID: <DM6PR07MB55298C1258149664502DCDCFDD430@DM6PR07MB5529.namprd07.prod.outlook.com> (raw)
In-Reply-To: <20200707074941.28078-2-peter.chen@nxp.com>

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

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