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.
What happens to ext_regulator[0] if you fail to enable
ext_regulator[1]? Does it remain permanently enabled?
> + }
> +
> 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?
How come you have an ohci_regulator_put_all() routine but not an
ohci_regulator_disable_all() routine?
And likewise for _get_ and _enable_?
> +
> 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?
Alan Stern
--
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
