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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
