Query: Regulator framework in EHCI driver

Query: Regulator framework in EHCI driver

[Gupta, Ajay Kumar]

Hi Anand/Felipe,

This is regarding regulator framework for 1V8 supply to EHCI PHY from twl4030 device.

[EHCI port on OMAP3EVM uses SMSC USB3320 PHY and uses 1V8 supply from twl4030 chip.]

I found twl4030_usb_ldo_init () function in drivers/usb/otg/twl4030-usb.c, which uses regulator framework but it can not be used by all the board which are not using twl4030 PHY. Don't you think this function need to be moved to some other common location? 

Currently we are not enabling 1V8 supply explicitly but still EHCI works on EVM as most of the twl4030 supplies are enabled by default.

I was thinking of adding regulator framework in EHCI driver which will enable the required supplies based on one of the board_data passed (same as reset_gpio) from board files.

Regards,
Ajay Gupta

Re: Query: Regulator framework in EHCI driver

[Felipe Balbi]

On Tue, Nov 03, 2009 at 12:55:53PM +0100, ext Gupta, Ajay Kumar wrote:
> Hi Anand/Felipe,
> 
> This is regarding regulator framework for 1V8 supply to EHCI PHY from twl4030 device.
> 
> [EHCI port on OMAP3EVM uses SMSC USB3320 PHY and uses 1V8 supply from twl4030 chip.]
> 
> I found twl4030_usb_ldo_init () function in drivers/usb/otg/twl4030-usb.c, which uses regulator framework but it can not be used by all the board which are not using twl4030 PHY. Don't you think this function need to be moved to some other common location?
> 
> Currently we are not enabling 1V8 supply explicitly but still EHCI works on EVM as most of the twl4030 supplies are enabled by default.
> 
> I was thinking of adding regulator framework in EHCI driver which will enable the required supplies based on one of the board_data passed (same as reset_gpio) from board files.

You should be able to regulator_get() the 1v8 supply and
regulator_enable() it. The regulator was already setup in twl4030-core.c
so that should work just fine. It's not a good idea to export
twl4030_ldo_init() since that's only for twl4030-usb itself. If you
regulator_get()/regulator_enable() then the regulator will be
refcounted.

You might want to add more consumers to the same supply based on board
information though. Today it's hardcoded to one for every board in
twl4030-core.c

-- 
balbi

RE: Query: Regulator framework in EHCI driver

[Gupta, Ajay Kumar]

> >
> > This is regarding regulator framework for 1V8 supply to EHCI PHY from
> twl4030 device.
> >
> > [EHCI port on OMAP3EVM uses SMSC USB3320 PHY and uses 1V8 supply from
> twl4030 chip.]
> >
> > I found twl4030_usb_ldo_init () function in drivers/usb/otg/twl4030-
> usb.c, which uses regulator framework but it can not be used by all the
> board which are not using twl4030 PHY. Don't you think this function need
> to be moved to some other common location?
> >
> > Currently we are not enabling 1V8 supply explicitly but still EHCI works
> on EVM as most of the twl4030 supplies are enabled by default.
> >
> > I was thinking of adding regulator framework in EHCI driver which will
> enable the required supplies based on one of the board_data passed (same
> as reset_gpio) from board files.
> 
> You should be able to regulator_get() the 1v8 supply and
> regulator_enable() it. The regulator was already setup in twl4030-core.c
> so that should work just fine. 

I can do regulator_get() and regulator_enable() but where ? Are you referring to board file?

I was thinking of doing it in a generic form within EHCI driver (ehci-omap.c).

> It's not a good idea to export
> twl4030_ldo_init() since that's only for twl4030-usb itself.

I am not asking for exporting twl4030_ldo_init() but to remove EHCI specific
regulator part from this file to ehci-omap.c in a generic form.

> If you regulator_get()/regulator_enable() then the regulator will be
> refcounted.
> 
> You might want to add more consumers to the same supply based on board
> information though.

It can be done but the issue it to make regulator_get()/regulator_enable() calls at a common location.

-Ajay

> Today it's hardcoded to one for every board in
> twl4030-core.c
> 
> --
> balbi

Re: Query: Regulator framework in EHCI driver

[Mark Brown]

On Wed, Nov 04, 2009 at 06:02:48PM +0530, Gupta, Ajay Kumar wrote:

> > You should be able to regulator_get() the 1v8 supply and
> > regulator_enable() it. The regulator was already setup in twl4030-core.c
> > so that should work just fine. 

> I can do regulator_get() and regulator_enable() but where ? Are you referring to board file?

> I was thinking of doing it in a generic form within EHCI driver (ehci-omap.c).

Do it in the EHCI driver.  The EHCI driver should just unconditionally
use the regulators, the regulator API will be compiled out if support is
disabled in Kconfig.

RE: Query: Regulator framework in EHCI driver

[Gadiyar, Anand]

Mark Brown wrote:
> On Wed, Nov 04, 2009 at 06:02:48PM +0530, Gupta, Ajay Kumar wrote:
> 
> > > You should be able to regulator_get() the 1v8 supply and
> > > regulator_enable() it. The regulator was already setup in twl4030-core.c
> > > so that should work just fine. 
> 
> > I can do regulator_get() and regulator_enable() but where ? Are you referring to board file?
> 
> > I was thinking of doing it in a generic form within EHCI driver (ehci-omap.c).
> 
> Do it in the EHCI driver.  The EHCI driver should just unconditionally
> use the regulators, the regulator API will be compiled out if support is
> disabled in Kconfig.
> 

NAK to do it unconditionally in the driver. This is board specific.
It just so happens that this is the regulator used on the beagleboard
and EVM, but it could very easily be another one (like on the SDP).

How about a hook from the platform code in mach-omap2/usb-ehci.c instead?

Re: Query: Regulator framework in EHCI driver

[Mark Brown]

On Wed, Nov 04, 2009 at 08:00:52PM +0530, Gadiyar, Anand wrote:
> Mark Brown wrote:
> > On Wed, Nov 04, 2009 at 06:02:48PM +0530, Gupta, Ajay Kumar wrote:

> > > I can do regulator_get() and regulator_enable() but where ? Are you referring to board file?

> > > I was thinking of doing it in a generic form within EHCI driver (ehci-omap.c).

> > Do it in the EHCI driver.  The EHCI driver should just unconditionally
> > use the regulators, the regulator API will be compiled out if support is
> > disabled in Kconfig.

> NAK to do it unconditionally in the driver. This is board specific.
> It just so happens that this is the regulator used on the beagleboard
> and EVM, but it could very easily be another one (like on the SDP).

Obviously the physical regulator hookup is board specific - please take
a look at how the regulator API operates here, none of this needs to be
handled by drivers using the API which was what the discussion above
(regarding regulator_get() and regulator_enable()) was about.

regulator_get() takes the struct device for the consumer (the EHCI
controller in this case) and a name for the supply both of which depend
only on the consumer.  The board drivers set up the mapping from these
to actual physical regulators during initialisation and the whole
process is completely transparent to the consumer drivers, they can just
ask for a fixed thing.

> How about a hook from the platform code in mach-omap2/usb-ehci.c instead?

If this is needed something is broken.

RE: Query: Regulator framework in EHCI driver

[Gadiyar, Anand]

Mark Brown wrote:
> On Wed, Nov 04, 2009 at 08:00:52PM +0530, Gadiyar, Anand wrote:
> > Mark Brown wrote:
> > > On Wed, Nov 04, 2009 at 06:02:48PM +0530, Gupta, Ajay Kumar wrote:
> 
> > > > I can do regulator_get() and regulator_enable() but where ? Are you referring to board file?
> 
> > > > I was thinking of doing it in a generic form within EHCI driver (ehci-omap.c).
> 
> > > Do it in the EHCI driver.  The EHCI driver should just unconditionally
> > > use the regulators, the regulator API will be compiled out if support is
> > > disabled in Kconfig.
> 
> > NAK to do it unconditionally in the driver. This is board specific.
> > It just so happens that this is the regulator used on the beagleboard
> > and EVM, but it could very easily be another one (like on the SDP).
> 
> Obviously the physical regulator hookup is board specific - please take
> a look at how the regulator API operates here, none of this needs to be
> handled by drivers using the API which was what the discussion above
> (regarding regulator_get() and regulator_enable()) was about.
> 
> regulator_get() takes the struct device for the consumer (the EHCI
> controller in this case) and a name for the supply both of which depend
> only on the consumer.  The board drivers set up the mapping from these
> to actual physical regulators during initialisation and the whole
> process is completely transparent to the consumer drivers, they can just
> ask for a fixed thing.
> 

Okay. I see now. Thanks.

So, if a board does not need a regulator at all, then the APIs do nothing?

> > How about a hook from the platform code in mach-omap2/usb-ehci.c instead?
> 
> If this is needed something is broken.
> 
> 

Re: Query: Regulator framework in EHCI driver

[Mark Brown]

On Wed, Nov 04, 2009 at 08:15:02PM +0530, Gadiyar, Anand wrote:

> So, if a board does not need a regulator at all, then the APIs do nothing?

If the kernel is built without regulator API support then the get,
release, enable and disable functions are stubbed out to always report
success.  If the kernel has been built with regulator support then
generally a fixed voltage regulator should be defined by the board.  The
expectation is that if the regulator API is of no use on a board since
there are no regulators set up then it'll be disabled in Kconfig.

RE: Query: Regulator framework in EHCI driver

[Gadiyar, Anand]

Mark Brown:
> On Wed, Nov 04, 2009 at 08:15:02PM +0530, Gadiyar, Anand wrote:
> 
> > So, if a board does not need a regulator at all, then the APIs do nothing?
> 
> If the kernel is built without regulator API support then the get,
> release, enable and disable functions are stubbed out to always report
> success.  If the kernel has been built with regulator support then
> generally a fixed voltage regulator should be defined by the board.  The
> expectation is that if the regulator API is of no use on a board since
> there are no regulators set up then it'll be disabled in Kconfig.
> 

I meant: for the same driver (say EHCI in this case) on two different
boards, if one of them needs a particular regulator (say the 1v8 from TWL4030)
and the other needs none at all (but needs the regulator API in general for
other devices on the board), then what would happen?

Re: Query: Regulator framework in EHCI driver

[Mark Brown]

On Wed, Nov 04, 2009 at 08:20:46PM +0530, Gadiyar, Anand wrote:

> I meant: for the same driver (say EHCI in this case) on two different
> boards, if one of them needs a particular regulator (say the 1v8 from TWL4030)
> and the other needs none at all (but needs the regulator API in general for
> other devices on the board), then what would happen?

Like I say, if the kernel is built with regulator support then a fixed
voltage regulator should be defined representing the supply if it is
provided by a fixed voltage regulator - if the supply is essential to
the correct operation of the device this will reflect the actual
hardware.

If the supply is genuinely optional and need not be connected for
correct operation of the device then the consumer driver should
gracefully handle failure to acquire the regulator, for example by
disabling functionality that depends on the additional supply, but this
is relatively unusual.

RE: Query: Regulator framework in EHCI driver

[Gupta, Ajay Kumar]

> Mark Brown:
> > On Wed, Nov 04, 2009 at 08:15:02PM +0530, Gadiyar, Anand wrote:
> >
> > > So, if a board does not need a regulator at all, then the APIs do
> nothing?
> >
> > If the kernel is built without regulator API support then the get,
> > release, enable and disable functions are stubbed out to always report
> > success.  If the kernel has been built with regulator support then
> > generally a fixed voltage regulator should be defined by the board.  The
> > expectation is that if the regulator API is of no use on a board since
> > there are no regulators set up then it'll be disabled in Kconfig.
> >
> 
> I meant: for the same driver (say EHCI in this case) on two different
> boards, if one of them needs a particular regulator (say the 1v8 from
> TWL4030)
> and the other needs none at all (but needs the regulator API in general
> for
> other devices on the board), then what would happen?

I think we can add a check for supply name in EHCI driver, if its valid then we will call regulator_get()/enable() but if it's NULL then don't do anything.

So the boards which don't use any regulator they can pass a NULL to supply name.

-Ajay 

Re: Query: Regulator framework in EHCI driver

[Mark Brown]

On Thu, Nov 05, 2009 at 08:54:02AM +0530, Gupta, Ajay Kumar wrote:

> I think we can add a check for supply name in EHCI driver, if its valid then we will call regulator_get()/enable() but if it's NULL then don't do anything.
> 
> So the boards which don't use any regulator they can pass a NULL to supply name.

No, this would be a substantial misuse of the regulator API.  The supply
name should not be being passed through as platform data, the driver
should request a fixed name (usually the name of the relevant physical
supply to the chip) and let the API map this onto the actual supply for
the system.

Re: Query: Regulator framework in EHCI driver

[Felipe Balbi]

Hi,

On Thu, Nov 05, 2009 at 10:36:41AM +0100, ext Mark Brown wrote:
> On Thu, Nov 05, 2009 at 08:54:02AM +0530, Gupta, Ajay Kumar wrote:
> 
> > I think we can add a check for supply name in EHCI driver, if its
> valid then we will call regulator_get()/enable() but if it's NULL then
> don't do anything.
> > 
> > So the boards which don't use any regulator they can pass a NULL to
> supply name.

I don't know if I'm the only one, but Anand's mails are coming in over
80 chars lines.

> No, this would be a substantial misuse of the regulator API.  The supply
> name should not be being passed through as platform data, the driver
> should request a fixed name (usually the name of the relevant physical
> supply to the chip) and let the API map this onto the actual supply for
> the system.

I agree with Mark here. The best solution would be to provide a fixed
voltage regulator for the other boards.

-- 
balbi

RE: Query: Regulator framework in EHCI driver

[Gadiyar, Anand]

Felipe Balbi wrote:
> On Thu, Nov 05, 2009 at 10:36:41AM +0100, ext Mark Brown wrote:
> > On Thu, Nov 05, 2009 at 08:54:02AM +0530, Gupta, Ajay Kumar wrote:
> > 
> > > I think we can add a check for supply name in EHCI driver, if its
> > > valid then we will call regulator_get()/enable() but if it's NULL then don't do anything.
> > > 
> > > So the boards which don't use any regulator they can pass a NULL to supply name.
> 
> I don't know if I'm the only one, but Anand's mails are coming in over
> 80 chars lines.

Sorry. Broken mailer I suppose :(

> 
> > No, this would be a substantial misuse of the regulator API.  The supply
> > name should not be being passed through as platform data, the driver
> > should request a fixed name (usually the name of the relevant physical
> > supply to the chip) and let the API map this onto the actual supply for
> > the system.
> 
> I agree with Mark here. The best solution would be to provide a fixed
> voltage regulator for the other boards.
> 

How do you add a fixed voltage regulator for a board which doesn't really
have a controllable regulator for that voltage? (the PHY supply is wired
directly from the main board power-supply, no GPIO for on-off control)

Maybe a dummy regulator would be useful?

Re: Query: Regulator framework in EHCI driver

[Mark Brown]

On Thu, Nov 05, 2009 at 04:01:24PM +0530, Gadiyar, Anand wrote:

> How do you add a fixed voltage regulator for a board which doesn't really
> have a controllable regulator for that voltage? (the PHY supply is wired
> directly from the main board power-supply, no GPIO for on-off control)

> Maybe a dummy regulator would be useful?

Like drivers/regulator/fixed.c?

RE: Query: Regulator framework in EHCI driver

[Gadiyar, Anand]

> On Thu, Nov 05, 2009 at 04:01:24PM +0530, Gadiyar, Anand wrote:
> 
> > How do you add a fixed voltage regulator for a board which 
> doesn't really
> > have a controllable regulator for that voltage? (the PHY 
> supply is wired
> > directly from the main board power-supply, no GPIO for 
> on-off control)
> 
> > Maybe a dummy regulator would be useful?
> 
> Like drivers/regulator/fixed.c?
> 
> 

Exactly. Thanks!

(I'm stupid. I should read the code first)

RE: Query: Regulator framework in EHCI driver

[Gupta, Ajay Kumar]

Hi,
> -----Original Message-----
> From: Gadiyar, Anand
> Sent: Thursday, November 05, 2009 4:56 PM
> To: Mark Brown
> Cc: felipe.balbi@nokia.com; Gupta, Ajay Kumar; linux-omap@vger.kernel.org;
> Aggarwal, Anuj
> Subject: RE: Query: Regulator framework in EHCI driver
> 
> > On Thu, Nov 05, 2009 at 04:01:24PM +0530, Gadiyar, Anand wrote:
> >
> > > How do you add a fixed voltage regulator for a board which
> > doesn't really
> > > have a controllable regulator for that voltage? (the PHY
> > supply is wired
> > > directly from the main board power-supply, no GPIO for
> > on-off control)
> >

Hi,

Please review this RFC version. It works with OMAP3EVM after adding 'ehci1' supply mapping in board file.

-Ajay

======================= RFC ============================

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 'ehciX' where
'X' can be {0, 1 ,2} depending on the ports.

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

diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 97ce7d5..c4391fb 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[5];
 
 	if (!pdata) {
 		dev_dbg(&pdev->dev, "missing platform_data\n");
@@ -918,6 +926,19 @@ 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_UNKNOWN)
+			continue;
+		sprintf(supply, "ehci%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");
@@ -981,6 +1002,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 +1014,15 @@ 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->port_mode[i] == EHCI_HCD_OMAP_MODE_UNKNOWN)
+			continue;
+		if (omap->regulator[i]) {
+			if (regulator_is_enabled(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: Query: Regulator framework in EHCI driver

[Mark Brown]

On Wed, Nov 11, 2009 at 08:26:07PM +0530, Gupta, Ajay Kumar wrote:

> +	/* get ehci regulator and enable */
> +	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
> +		if (omap->port_mode[i] == EHCI_HCD_OMAP_MODE_UNKNOWN)
> +			continue;
> +		sprintf(supply, "ehci%d", i);

The use of sprintf() here looks suspicious - these things would normally
be completely fixed.  I appreciate that that's the result, it just looks
suspicous.  Picking out of an array of fixed names would be more
idiomatic.  It'd also be idiomatic to use whatever the supply on the
chip is called in the datasheet - ehci might be accurate but it'd be a
bit of a surprising choice, it'd be good to check.

If you do stick with this approach you probably want to use snprintf()
and make supply be 6 bytes rather than 5 bytes long.

> +	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
> +		if (omap->port_mode[i] == EHCI_HCD_OMAP_MODE_UNKNOWN)
> +			continue;
> +		if (omap->regulator[i]) {
> +			if (regulator_is_enabled(omap->regulator[i]))
> +				regulator_disable(omap->regulator[i]);
> +			regulator_put(omap->regulator[i]);
> +		}

For robustness I'd drop the first check for MODE_UNKNOWN here - it
doesn't add anything.  You also want to call regulator_disable() before
you free the regulator.

RE: Query: Regulator framework in EHCI driver

[Gupta, Ajay Kumar]

Hi,

> > +	/* get ehci regulator and enable */
> > +	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
> > +		if (omap->port_mode[i] == EHCI_HCD_OMAP_MODE_UNKNOWN)
> > +			continue;
> > +		sprintf(supply, "ehci%d", i);
> 
> The use of sprintf() here looks suspicious - these things would normally
> be completely fixed.  I appreciate that that's the result, it just looks
> suspicous.  
> Picking out of an array of fixed names would be more idiomatic.

I think this one is good to take.


>  It'd also be idiomatic to use whatever the supply on the
> chip is called in the datasheet - ehci might be accurate but it'd be a
> bit of a surprising choice, it'd be good to check.

Supply names on EHCI PHY SMSC-USB3320 (used on EVM) chip datasheet is 'vdd18' but this also may change as other boards use different PHY.

> 
> If you do stick with this approach you probably want to use snprintf()
> and make supply be 6 bytes rather than 5 bytes long.
> 
> > +	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
> > +		if (omap->port_mode[i] == EHCI_HCD_OMAP_MODE_UNKNOWN)
> > +			continue;
> > +		if (omap->regulator[i]) {
> > +			if (regulator_is_enabled(omap->regulator[i]))
> > +				regulator_disable(omap->regulator[i]);
> > +			regulator_put(omap->regulator[i]);
> > +		}
> 
> For robustness I'd drop the first check for MODE_UNKNOWN here - it
> doesn't add anything.

MODE_UNKNOWN means that the port is not connected and so no need to check
the regulator availability.

>  You also want to call regulator_disable() before
> you free the regulator.

It is being done under 'if (regulator_is_enabled)' check.

-Ajay

RE: Query: Regulator framework in EHCI driver

[Gupta, Ajay Kumar]

Hi,
> 
> > > +	/* get ehci regulator and enable */
> > > +	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
> > > +		if (omap->port_mode[i] == EHCI_HCD_OMAP_MODE_UNKNOWN)
> > > +			continue;
> > > +		sprintf(supply, "ehci%d", i);
> >
> > The use of sprintf() here looks suspicious - these things would normally
> > be completely fixed.  I appreciate that that's the result, it just looks
> > suspicous.
> > Picking out of an array of fixed names would be more idiomatic.
> 
> I think this one is good to take.
> 
> 
> >  It'd also be idiomatic to use whatever the supply on the
> > chip is called in the datasheet - ehci might be accurate but it'd be a
> > bit of a surprising choice, it'd be good to check.
> 
> Supply names on EHCI PHY SMSC-USB3320 (used on EVM) chip datasheet is
> 'vdd18' but this also may change as other boards use different PHY.

Another point,

OMAP ports can be used as OHCI over serial interface so supply name
as 'ehciN' would be confusing.

I would change it to 'hsusb-portN' in next revision.

Regards,
Ajay
> 
> >
> > If you do stick with this approach you probably want to use snprintf()
> > and make supply be 6 bytes rather than 5 bytes long.
> >
> > > +	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
> > > +		if (omap->port_mode[i] == EHCI_HCD_OMAP_MODE_UNKNOWN)
> > > +			continue;
> > > +		if (omap->regulator[i]) {
> > > +			if (regulator_is_enabled(omap->regulator[i]))
> > > +				regulator_disable(omap->regulator[i]);
> > > +			regulator_put(omap->regulator[i]);
> > > +		}
> >
> > For robustness I'd drop the first check for MODE_UNKNOWN here - it
> > doesn't add anything.
> 
> MODE_UNKNOWN means that the port is not connected and so no need to check
> the regulator availability.
> 
> >  You also want to call regulator_disable() before
> > you free the regulator.
> 
> It is being done under 'if (regulator_is_enabled)' check.
> 
> -Ajay
> --
> 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: Query: Regulator framework in EHCI driver

[Gadiyar, Anand]

Gupta, Ajay Kumar wrote:
> Hi,
> 
> > > +	/* get ehci regulator and enable */
> > > +	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
> > > +		if (omap->port_mode[i] == EHCI_HCD_OMAP_MODE_UNKNOWN)
> > > +			continue;
> > > +		sprintf(supply, "ehci%d", i);
> > 
> > The use of sprintf() here looks suspicious - these things would normally
> > be completely fixed.  I appreciate that that's the result, it just looks
> > suspicous.  
> > Picking out of an array of fixed names would be more idiomatic.
> 
> I think this one is good to take.
> 
> 
> >  It'd also be idiomatic to use whatever the supply on the
> > chip is called in the datasheet - ehci might be accurate but it'd be a
> > bit of a surprising choice, it'd be good to check.
> 
> Supply names on EHCI PHY SMSC-USB3320 (used on EVM) chip 
> datasheet is 'vdd18' but this also may change as other boards 
> use different PHY.
> 
> > 
> > If you do stick with this approach you probably want to use snprintf()
> > and make supply be 6 bytes rather than 5 bytes long.
> > 
> > > +	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
> > > +		if (omap->port_mode[i] == EHCI_HCD_OMAP_MODE_UNKNOWN)
> > > +			continue;
> > > +		if (omap->regulator[i]) {
> > > +			if (regulator_is_enabled(omap->regulator[i]))
> > > +				regulator_disable(omap->regulator[i]);
> > > +			regulator_put(omap->regulator[i]);
> > > +		}
> > 
> > For robustness I'd drop the first check for MODE_UNKNOWN here - it
> > doesn't add anything.
> 
> MODE_UNKNOWN means that the port is not connected and so no 
> need to check
> the regulator availability.

Ajay,

It's better to check for EHCI_HCD_OMAP_MODE_PHY instead
of EHCI_HCD_OMAP_MODE_UNKNOWN. The other option is
EHCI_HCD_OMAP_MODE_TLL, and it will never use a regulator.


Regards,
Anand

> 
> >  You also want to call regulator_disable() before
> > you free the regulator.
> 
> It is being done under 'if (regulator_is_enabled)' check.
> 
> -Ajay
> 

RE: Query: Regulator framework in EHCI driver

[Gupta, Ajay Kumar]

Hi,
> > > If you do stick with this approach you probably want to use snprintf()
> > > and make supply be 6 bytes rather than 5 bytes long.
> > >
> > > > +	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
> > > > +		if (omap->port_mode[i] == EHCI_HCD_OMAP_MODE_UNKNOWN)
> > > > +			continue;
> > > > +		if (omap->regulator[i]) {
> > > > +			if (regulator_is_enabled(omap->regulator[i]))
> > > > +				regulator_disable(omap->regulator[i]);
> > > > +			regulator_put(omap->regulator[i]);
> > > > +		}
> > >
> > > For robustness I'd drop the first check for MODE_UNKNOWN here - it
> > > doesn't add anything.
> >
> > MODE_UNKNOWN means that the port is not connected and so no
> > need to check
> > the regulator availability.
> 
> Ajay,
> 
> It's better to check for EHCI_HCD_OMAP_MODE_PHY instead
> of EHCI_HCD_OMAP_MODE_UNKNOWN. The other option is
> EHCI_HCD_OMAP_MODE_TLL, and it will never use a regulator.

Ok fine, I will take this one also in next revision.

-Ajay
> 
> 
> Regards,
> Anand
> 
> >
> > >  You also want to call regulator_disable() before
> > > you free the regulator.
> >
> > It is being done under 'if (regulator_is_enabled)' check.
> >
> > -Ajay
> >

Re: Query: Regulator framework in EHCI driver

[Mark Brown]

On Thu, Nov 12, 2009 at 09:11:58AM +0530, Gupta, Ajay Kumar wrote:

> > > +		if (omap->port_mode[i] == EHCI_HCD_OMAP_MODE_UNKNOWN)
> > > +			continue;
> > > +		if (omap->regulator[i]) {
> > > +			if (regulator_is_enabled(omap->regulator[i]))
> > > +				regulator_disable(omap->regulator[i]);
> > > +			regulator_put(omap->regulator[i]);
> > > +		}

> > For robustness I'd drop the first check for MODE_UNKNOWN here - it
> > doesn't add anything.

> MODE_UNKNOWN means that the port is not connected and so no need to check
> the regulator availability.

Sure, currently - the point is that if you're going to check for the
individual allocations anyway then the check doesn't buy you anything.

RE: Query: Regulator framework in EHCI driver

[Gupta, Ajay Kumar]

Hi,

> > MODE_UNKNOWN means that the port is not connected and so no need to
> check
> > the regulator availability.
> 
> Sure, currently - the point is that if you're going to check for the
> individual allocations anyway then the check doesn't buy you anything.

Now I have taken this into consideration during driver exit function.

Please review the v2 of this patch below.

Regards,
Ajay
================ cut here =======================

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 |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 97ce7d5..394c0c6 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, 7, "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");
@@ -981,6 +1004,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 +1016,13 @@ 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]) {
+			if (regulator_is_enabled(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: Query: Regulator framework in EHCI driver

[Mark Brown]

On Fri, Nov 13, 2009 at 09:52:14AM +0530, Gupta, Ajay Kumar wrote:

> +		snprintf(supply, 7, "hsusb%d", i);

It'd be better to use sizeof() rather than a hard coded 7 here.

> +             omap->regulator[i] = regulator_get(omap->dev, supply);

Looking at your code this needs to be regulator_get_exclusive() - you
conditinoally disable the regulator when unregistering depending on if
it reports as enabled but if the supply is shared then it can be enabled
without this driver having done so.

Alternatively, make the regulator_disable() in the shutdown path
unconditional.

RE: Query: Regulator framework in EHCI driver

[Gupta, Ajay Kumar]

Hi,

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Friday, November 13, 2009 6:25 PM
> To: Gupta, Ajay Kumar
> Cc: Gadiyar, Anand; felipe.balbi@nokia.com; linux-omap@vger.kernel.org;
> Aggarwal, Anuj
> Subject: Re: Query: Regulator framework in EHCI driver
> 
> On Fri, Nov 13, 2009 at 09:52:14AM +0530, Gupta, Ajay Kumar wrote:
> 
> > +		snprintf(supply, 7, "hsusb%d", i);
> 
> It'd be better to use sizeof() rather than a hard coded 7 here.

Ok fine.
> 
> > +             omap->regulator[i] = regulator_get(omap->dev, supply);
> 
> Looking at your code this needs to be regulator_get_exclusive() - you
> conditinoally disable the regulator when unregistering depending on if
> it reports as enabled but if the supply is shared then it can be enabled
> without this driver having done so.
> 
> Alternatively, make the regulator_disable() in the shutdown path
> unconditional.

Ok, I will take this change and submit this patch.

Thanks,
Ajay