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.
Another point,
OMAP ports can be used as OHCI over serial interface so supply name
as 'ehciN' would be confusing.
I would change it to 'hsusb-portN' in next revision.
Regards,
Ajay
>
> >
> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
