Re: [PATCH] omap3: ehci: Add regulator framework

Re: [PATCH] omap3: ehci: Add regulator framework

[Felipe Balbi]

Hi,

On Mon, Dec 21, 2009 at 12:35:38PM +0100, ext Gupta, Ajay Kumar wrote:
>> This looks OK from a regulator API point of view.
>
>Felipe,
>
>Any update on this patch? It’s already reviewed by Mark Brown.
>
>Please add this one to your queue if there are no comments on this.
>
>[1] http://marc.info/?l=linux-omap&m=125827476331516&w=2

thanks for reminding me about this one. I had lost it.

Applied now on [1]

[1] http://gitorious.org/usb/usb/commit/5ffe22e280c3dde0c844e30ccef45abfcf1c3ccb

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

[PATCH] omap3: ehci: Add regulator framework

[Ajay Kumar Gupta]

Adding regulator framework in EHCI driver.

OMAP3 has three HS USB ports so it can have three different regulator
for each PHY connected to each port.

Currently these regulators are assumed to be optional and driver doesn't
fail but continue with the initialization if it doesn't get any regulators.

Regulator supply names has to be mapped in board files as 'hsusbN' where
'N' is port number and can be {0, 1 ,2}.

Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com>
---
 drivers/usb/host/ehci-omap.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 97ce7d5..91ab9da 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -37,6 +37,7 @@
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 #include <linux/gpio.h>
+#include <linux/regulator/consumer.h>
 #include <plat/usb.h>
 
 struct usb_hcd *ghcd;
@@ -191,6 +192,11 @@ struct ehci_hcd_omap {
 	void __iomem		*uhh_base;
 	void __iomem		*tll_base;
 	void __iomem		*ehci_base;
+
+	/* Regulators for USB PHYs.
+	 * Each PHY can have a seperate regulator.
+	 */
+	struct regulator        *regulator[OMAP3_HS_USB_PORTS];
 };
 
 /*-------------------------------------------------------------------------*/
@@ -839,6 +845,8 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 
 	int irq = platform_get_irq(pdev, 0);
 	int ret = -ENODEV;
+	int i;
+	char supply[7];
 
 	if (!pdata) {
 		dev_dbg(&pdev->dev, "missing platform_data\n");
@@ -918,6 +926,21 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 		goto err_tll_ioremap;
 	}
 
+	/* get ehci regulator and enable */
+	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
+		if (omap->port_mode[i] != EHCI_HCD_OMAP_MODE_PHY) {
+			omap->regulator[i] = NULL;
+			continue;
+		}
+		snprintf(supply, sizeof(supply), "hsusb%d", i);
+		omap->regulator[i] = regulator_get(omap->dev, supply);
+		if (IS_ERR(omap->regulator[i]))
+			dev_dbg(&pdev->dev,
+			"failed to get ehci port%d regulator\n", i);
+		else
+			regulator_enable(omap->regulator[i]);
+	}
+
 	ret = omap_start_ehc(omap, hcd);
 	if (ret) {
 		dev_dbg(&pdev->dev, "failed to start ehci\n");
@@ -949,6 +972,12 @@ err_add_hcd:
 	omap_stop_ehc(omap, hcd);
 
 err_start:
+	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
+		if (omap->regulator[i]) {
+			regulator_disable(omap->regulator[i]);
+			regulator_put(omap->regulator[i]);
+		}
+	}
 	iounmap(omap->tll_base);
 
 err_tll_ioremap:
@@ -981,6 +1010,7 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev)
 {
 	struct ehci_hcd_omap *omap = platform_get_drvdata(pdev);
 	struct usb_hcd *hcd = ehci_to_hcd(omap->ehci);
+	int i;
 
 	if (omap->port_mode[0] != EHCI_HCD_OMAP_MODE_UNKNOWN)
 		device_remove_file(&pdev->dev, &dev_attr_port1);
@@ -992,6 +1022,12 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev)
 	usb_remove_hcd(hcd);
 	omap_stop_ehc(omap, hcd);
 	iounmap(hcd->regs);
+	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
+		if (omap->regulator[i]) {
+			regulator_disable(omap->regulator[i]);
+			regulator_put(omap->regulator[i]);
+		}
+	}
 	iounmap(omap->tll_base);
 	iounmap(omap->uhh_base);
 	usb_put_hcd(hcd);
-- 
1.6.2.4

RE: [PATCH] omap3: ehci: Add regulator framework

[Pandita, Vikram]

Ajay

>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Gupta,
>Ajay Kumar
>Sent: Sunday, November 15, 2009 2:46 AM
>To: linux-omap@vger.kernel.org
>Cc: felipe.balbi@nokia.com; broonie@opensource.wolfsonmicro.com; Gupta, Ajay Kumar
>Subject: [PATCH] omap3: ehci: Add regulator framework
<snip>
>+	/* get ehci regulator and enable */
>+	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
>+		if (omap->port_mode[i] != EHCI_HCD_OMAP_MODE_PHY) {
>+			omap->regulator[i] = NULL;
>+			continue;
>+		}
>+		snprintf(supply, sizeof(supply), "hsusb%d", i);

supply[] being auto variable, is it guaranteed to be null terminated? 
Suggestion:
		n = snprintf(supply, sizeof(supply), "hsusb%d", i);
		supply[n] = '\0';

>+		omap->regulator[i] = regulator_get(omap->dev, supply);
>+		if (IS_ERR(omap->regulator[i]))
>+			dev_dbg(&pdev->dev,
>+			"failed to get ehci port%d regulator\n", i);
>+		else
>+			regulator_enable(omap->regulator[i]);
>+	}
>+
<snip>

RE: [PATCH] omap3: ehci: Add regulator framework

[Pandita, Vikram]

>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of
>Pandita, Vikram
>>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Gupta,
>>Ajay Kumar
>>Sent: Sunday, November 15, 2009 2:46 AM
>>To: linux-omap@vger.kernel.org
>>Cc: felipe.balbi@nokia.com; broonie@opensource.wolfsonmicro.com; Gupta, Ajay Kumar
>>Subject: [PATCH] omap3: ehci: Add regulator framework
><snip>
>>+	/* get ehci regulator and enable */
>>+	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
>>+		if (omap->port_mode[i] != EHCI_HCD_OMAP_MODE_PHY) {
>>+			omap->regulator[i] = NULL;
>>+			continue;
>>+		}
>>+		snprintf(supply, sizeof(supply), "hsusb%d", i);
>
>supply[] being auto variable, is it guaranteed to be null terminated?
>Suggestion:
>		n = snprintf(supply, sizeof(supply), "hsusb%d", i);
>		supply[n] = '\0';

Setting null is redundant as snprintf() does that internally too.

Can see snprinf calls vsnprintf and:
vsnprintf( ... ) 
...
       if (size > 0) {
                if (str < end)
>>>>                        *str = '\0';
                else
                        end[-1] = '\0';
        }
...
So I guess this is a noisy review comment :) and please ignore.
Patch looks good.

>
>>+		omap->regulator[i] = regulator_get(omap->dev, supply);
>>+		if (IS_ERR(omap->regulator[i]))
>>+			dev_dbg(&pdev->dev,
>>+			"failed to get ehci port%d regulator\n", i);
>>+		else
>>+			regulator_enable(omap->regulator[i]);
>>+	}
>>+
><snip>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] omap3: ehci: Add regulator framework

[Mark Brown]

On Sun, Nov 15, 2009 at 02:15:48PM +0530, Ajay Kumar Gupta wrote:
> Adding regulator framework in EHCI driver.
> 
> OMAP3 has three HS USB ports so it can have three different regulator
> for each PHY connected to each port.
> 
> Currently these regulators are assumed to be optional and driver doesn't
> fail but continue with the initialization if it doesn't get any regulators.
> 
> Regulator supply names has to be mapped in board files as 'hsusbN' where
> 'N' is port number and can be {0, 1 ,2}.
> 
> Signed-off-by: Ajay Kumar Gupta <ajay.gupta@ti.com>

This looks OK from a regulator API point of view.