Gupta, Ajay Kumar wrote:
> 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.
Ajay,
It's better to check for EHCI_HCD_OMAP_MODE_PHY instead
of EHCI_HCD_OMAP_MODE_UNKNOWN. The other option is
EHCI_HCD_OMAP_MODE_TLL, and it will never use a regulator.
Regards,
Anand
>
> > 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
