Patchwork [6/7] usb: phy: tegra: Add error handling & clean up.

login
register
mail settings
Submitter Venu Byravarasu
Date March 18, 2013, 12:29 p.m.
Message ID <1363609781-4045-7-git-send-email-vbyravarasu@nvidia.com>
Download mbox | patch
Permalink /patch/228483/
State Superseded, archived
Headers show

Comments

Venu Byravarasu - March 18, 2013, 12:29 p.m.
Check return values from all GPIO APIs and handle errors accordingly.
Remove clk_disable_unprepare which is no more needed.

Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>
---
 drivers/usb/phy/tegra_usb_phy.c |   50 ++++++++++++++++++++++++++++++--------
 1 files changed, 39 insertions(+), 11 deletions(-)
Stephen Warren - March 19, 2013, 7:25 p.m.
On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> Check return values from all GPIO APIs and handle errors accordingly.
> Remove clk_disable_unprepare which is no more needed.

Patches 6 and 7 each fail checkpatch with "WARNING: line over 80
characters".
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - March 19, 2013, 8:10 p.m.
On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> Check return values from all GPIO APIs and handle errors accordingly.

> Remove clk_disable_unprepare which is no more needed.

The call to clk_disable_unprepare is incorrect in the current code. The
way you worded that, it sounds like it's no longer needed because of the
changes made in this patch. I would re-write that last sentence as:

Remove the call to clk_disable_unprepare(); this function does not
prepare or enable the clock, so the error path should not disable or
unprepare it.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - April 3, 2013, 7:34 p.m.
On 03/19/2013 02:10 PM, Stephen Warren wrote:
> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
>> Check return values from all GPIO APIs and handle errors accordingly.
> 
>> Remove clk_disable_unprepare which is no more needed.
> 
> The call to clk_disable_unprepare is incorrect in the current code. The
> way you worded that, it sounds like it's no longer needed because of the
> changes made in this patch. I would re-write that last sentence as:
> 
> Remove the call to clk_disable_unprepare(); this function does not
> prepare or enable the clock, so the error path should not disable or
> unprepare it.

V2 didn't address this review feedback:-(

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
index 29c5ac4..7ead114 100644
--- a/drivers/usb/phy/tegra_usb_phy.c
+++ b/drivers/usb/phy/tegra_usb_phy.c
@@ -542,9 +542,17 @@  static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
 	unsigned long val;
 	void __iomem *base = phy->regs;
 
-	gpio_direction_output(phy->reset_gpio, 0);
+	ret = gpio_direction_output(phy->reset_gpio, 0);
+	if (ret < 0) {
+		dev_err(phy->dev, "gpio %d direction not set\n", phy->reset_gpio);
+		return ret;
+	}
 	msleep(5);
-	gpio_direction_output(phy->reset_gpio, 1);
+	ret = gpio_direction_output(phy->reset_gpio, 1);
+	if (ret < 0) {
+		dev_err(phy->dev, "gpio %d direction not set\n", phy->reset_gpio);
+		return ret;
+	}
 
 	clk_prepare_enable(phy->clk);
 	msleep(1);
@@ -624,24 +632,44 @@  static int	tegra_phy_init(struct usb_phy *x)
 			of_get_named_gpio(phy->dev->of_node,
 					  "nvidia,phy-reset-gpio", 0);
 		if (!gpio_is_valid(phy->reset_gpio)) {
-			pr_err("%s: invalid reset gpio: %d\n", __func__,
+			dev_err(phy->dev, "invalid gpio: %d\n",
+				phy->reset_gpio);
+			err = phy->reset_gpio;
+			goto cleanup_clk_get;
+		}
+
+		err = gpio_request(phy->reset_gpio, "ulpi_phy_reset_b");
+		if (err < 0) {
+			dev_err(phy->dev, "request failed for gpio: %d\n",
 			       phy->reset_gpio);
-			err = -EINVAL;
-			goto err1;
+			goto cleanup_clk_get;
+		}
+
+		err = gpio_direction_output(phy->reset_gpio, 0);
+		if (err < 0) {
+			dev_err(phy->dev, "gpio %d direction not set to output\n",
+			       phy->reset_gpio);
+			goto cleanup_gpio_req;
 		}
-		gpio_request(phy->reset_gpio, "ulpi_phy_reset_b");
-		gpio_direction_output(phy->reset_gpio, 0);
+
 		phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
+		if (!phy->ulpi) {
+			dev_err(phy->dev, "otg_ulpi_create returned err\n");
+			err = -ENOMEM;
+			goto cleanup_gpio_req;
+		}
+
 		phy->ulpi->io_priv = phy->regs + ULPI_VIEWPORT;
 	} else {
 		err = utmip_pad_open(phy);
 		if (err < 0)
-			goto err1;
+			return err;
 	}
 	return 0;
-err1:
-	clk_disable_unprepare(phy->pll_u);
-	clk_put(phy->pll_u);
+cleanup_gpio_req:
+	gpio_free(phy->reset_gpio);
+cleanup_clk_get:
+	clk_put(phy->clk);
 	return err;
 }