Re: Query: Regulator framework in EHCI driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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.