On Mon, Jul 28, 2014 at 10:45:57PM +0200, Sebastian Hesselbarth wrote:
> On 07/28/2014 10:23 PM, Markus Pargmann wrote:
> > Add some code to parse and enable vbus supply when probing.
> > 
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/usb/imx/chipidea-imx.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
> > index 9b6829b8f59f..a4cf4a1a5f3d 100644
> > --- a/drivers/usb/imx/chipidea-imx.c
> > +++ b/drivers/usb/imx/chipidea-imx.c
> > @@ -15,6 +15,7 @@
> >  #include <common.h>
> >  #include <init.h>
> >  #include <io.h>
> > +#include <linux/err.h>
> >  #include <of.h>
> >  #include <errno.h>
> >  #include <driver.h>
> > @@ -23,6 +24,7 @@
> >  #include <usb/chipidea-imx.h>
> >  #include <usb/ulpi.h>
> >  #include <usb/fsl_usb2.h>
> > +#include <regulator.h>
> >  
> >  #define MXC_EHCI_PORTSC_MASK ((0xf << 28) | (1 << 25))
> >  
> > @@ -187,6 +189,16 @@ static int imx_chipidea_probe(struct device_d *dev)
> >  		ret = -ENODEV;
> >  	}
> >  
> > +	if (ci->mode == IMX_USB_MODE_HOST) {
> > +		struct regulator *vbus = regulator_get(dev, "vbus");
> > +		if (!IS_ERR(vbus)) {
> > +			regulator_enable(vbus);
> > +		} else {
> > +			dev_err(dev, "Failed to get vbus regulator %ld\n", PTR_ERR(vbus));
> > +			ret = PTR_ERR(vbus);
> > +		}
> 
> regulator_get should return a NULL pointer if there is no regulator for
> "vbus". The NULL regulator is a valid (dummy) regulator that can be used
> as if there was a real one.
> 
> That basically means, you should check for an error and always use
> regulator_enable otherwise:
> 
> struct regulator *vbus = regulator_get(dev, "vbus");
> if (IS_ERR(vbus)) {
> 	dev_err(dev, "Failed to get vbus regulator %ld\n", PTR_ERR(vbus));
> 	return PTR_ERR(vbus);
> }
> 
> regulator_enable(vbus);

Thanks, will change that.

> 
> But even more important: You should also check for a regulator on
> peripheral/otg mode. That's the cases where you can actually break
> things when you don't disable the regulator.

Oh right, I didn't thought about that. I will change the code so that it
is either enabled or disabled, depending on the mode.

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |