On 10/16/2013 05:58 PM, Alan Stern wrote:
On Wed, 16 Oct 2013, Nikita Kiryanov wrote:Add regulator support for devices that sit on USB ports, thus allowing usb devices power management for suspend/resume. Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> Signed-off-by: Nikita Kiryanov <nikita@xxxxxxxxxxxxxx> Signed-off-by: Igor Grinberg <grinberg@xxxxxxxxxxxxxx> --- drivers/usb/host/ohci-pxa27x.c | 47 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c index 35e739e..ae4c64f 100644 --- a/drivers/usb/host/ohci-pxa27x.c +++ b/drivers/usb/host/ohci-pxa27x.c @@ -109,6 +109,7 @@ struct pxa27x_ohci { struct device *dev; struct clk *clk; struct regulator *usb_regulator; + struct regulator *ext_regulator[PXA_UHC_MAX_PORTNUM]; void __iomem *mmio_base; }; @@ -217,7 +218,7 @@ extern void pxa27x_clear_otgph(void); static int pxa27x_start_hc(struct pxa27x_ohci *ohci, struct device *dev) { - int retval = 0; + int i, retval = 0; struct pxaohci_platform_data *inf; uint32_t uhchr; @@ -227,6 +228,16 @@ static int pxa27x_start_hc(struct pxa27x_ohci *ohci, struct device *dev) if (retval) return retval; + for (i = 0; i < PXA_UHC_MAX_PORTNUM; i++) { + retval = 0; + if (ohci->ext_regulator[i]) + retval = regulator_enable(ohci->ext_regulator[i]); + + if (retval) + pr_err("%s: regulator enable failed: port%d, err=%d\n", + __func__, i, retval);You could get rid of the "retval = 0;" line if you wrote this as: if (ohci->ext_regulator[i]) { retval = regulator_enable(...); if (retval) dev_err(dev, ...) } Note: dev_err, not pr_err.
Good point.
What happens to ext_regulator[0] if you fail to enable ext_regulator[1]? Does it remain permanently enabled?
It will be turned off when pxa27x_stop_hc is called.
+ } + clk_prepare_enable(ohci->clk); pxa27x_reset_hc(ohci); @@ -257,8 +268,22 @@ static int pxa27x_start_hc(struct pxa27x_ohci *ohci, struct device *dev) return 0; } +static void ohci_regulator_put_all(struct pxa27x_ohci *ohci) +{ + int i; + + for (i = 0; i < PXA_UHC_MAX_PORTNUM; i++) { + if (ohci->ext_regulator[i]) + regulator_put(ohci->ext_regulator[i]); + } + + /* usb regulator should be present if we get here */ + regulator_put(ohci->usb_regulator); +}What happens if you call regulator_put() for an enabled regulator?
It'll remain enabled, but there's not much that can be done about this: The problem with regulators is that there's no way to tell *why* they're enabled. The same regulator can supply more than one device, and thus be turned on by multiple drivers. Because of this there's no way to tell if corrective measures should be taken before a regulator_put.
How come you have an ohci_regulator_put_all() routine but not an ohci_regulator_disable_all() routine? And likewise for _get_ and _enable_?
The code that would've been ohci_regulator_[get|enable|disable]_all is used only once in each case, so I didn't feel the need to encapsulate it into a function. I can make the change if you want me to.
+ static void pxa27x_stop_hc(struct pxa27x_ohci *ohci, struct device *dev) { + int i; struct pxaohci_platform_data *inf; uint32_t uhccoms; @@ -278,6 +303,12 @@ static void pxa27x_stop_hc(struct pxa27x_ohci *ohci, struct device *dev) udelay(10); clk_disable_unprepare(ohci->clk); + + for (i = 0; i < PXA_UHC_MAX_PORTNUM; i++) { + if (ohci->ext_regulator[i]) + regulator_disable(ohci->ext_regulator[i]); + } + regulator_disable(ohci->usb_regulator); } @@ -360,12 +391,13 @@ static int ohci_pxa_of_init(struct platform_device *pdev) */ int usb_hcd_pxa27x_probe (const struct hc_driver *driver, struct platform_device *pdev) { - int retval, irq; + int retval, irq, i; struct usb_hcd *hcd; struct pxaohci_platform_data *inf; struct pxa27x_ohci *ohci; struct resource *r; struct clk *usb_clk; + char supply[7]; retval = ohci_pxa_of_init(pdev); if (retval) @@ -427,6 +459,13 @@ int usb_hcd_pxa27x_probe (const struct hc_driver *driver, struct platform_device goto err3; } + for (i = 0; i < PXA_UHC_MAX_PORTNUM; i++) { + snprintf(supply, sizeof(supply), "fsusb%d", i); + ohci->ext_regulator[i] = regulator_get(&pdev->dev, supply); + if (IS_ERR(ohci->ext_regulator[i])) + ohci->ext_regulator[i] = NULL;Shouldn't this be a fatal error?
No, and it's not even necessarily an error. This can happen for valid reasons, for example if there is no regulator powered device on that port. In this case the appropriate fsusb%d regulator wouldn't have been registered, and regulator_get would fail. If it happened due to some other error, it's still not fatal because the subsystem and the rest of the regulator powered usb devices can continue to work normally.
Alan Stern
-- Regards, Nikita. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
