[U-Boot] [PATCH 1/6] omap4: usb: Add omap-ehci support

[Govindraj]

Hi Tom,

On Wed, Dec 14, 2011 at 9:30 PM, Tom Rini <tom.rini at gmail.com> wrote:
> On Wed, Dec 14, 2011 at 5:09 AM, Govindraj.R <govindraj.raja at ti.com> wrote:
>> From: "Govindraj.R" <govindraj.raja at ti.com>
> [snip]
>> +/* TLL Register Set */
>> +#define        OMAP_USBTLL_SYSCONFIG_SIDLEMODE                 (1 << 3)
>
> Globally, please fix all #define<tab> to #define<space>.
>
>> +#define        OMAP_USBTLL_SYSCONFIG_AUTOIDLE                  (1 << 0)
>> +#define        OMAP_USBTLL_SYSSTATUS_RESETDONE                 (1 << 0)
>
> Just use '1' not '(1 << 0)', globally.
>
> [snip]
>> +++ b/drivers/usb/host/ehci-omap.c
> [snip]
>> +       reg = ULPI_FUNC_CTRL_RESET
>> +               /* FUNCTION_CTRL_SET register */
>> +               | (ULPI_SET(ULPI_FUNC_CTRL) << EHCI_INSNREG05_ULPI_REGADD_SHIFT)
>> +               /* Write */
>> +               | (2 << EHCI_INSNREG05_ULPI_OPSEL_SHIFT)
>> +               /* PORTn */
>> +               | ((port + 1) << EHCI_INSNREG05_ULPI_PORTSEL_SHIFT)
>> +               /* start ULPI access*/
>> +               | (1 << EHCI_INSNREG05_ULPI_CONTROL_SHIFT);
>
> Can you please re-write this as:
> /*
>  * What these values are doing...
>  */
> reg = A | (B << B_SHIFT) | ...
>
> [snip]
>> +       /* Wait for ULPI access completion */
>> +       while ((readl(&ehci->insreg05_utmi_ulpi)
>> +               & (1 << EHCI_INSNREG05_ULPI_CONTROL_SHIFT)))
>> +               if (get_timer(init) > CONFIG_SYS_HZ) {
>
> Perhaps gmail is getting the indentation wrong here but it seems not
> right and the norm is to not start the newline with '&' or '|' or
> similar.
>

Thanks for the comments will fix and re-post a new version.

> [snip]
>> +       /*
>> +        * An undocumented "feature" in the OMAP3 EHCI controller,
>> +        * causes suspended ports to be taken out of suspend when
>> +        * the USBCMD.Run/Stop bit is cleared (for example when
>> +        * we do ehci_bus_suspend).
>> +        * This breaks suspend-resume if the root-hub is allowed
>> +        * to suspend. Writing 1 to this undocumented register bit
>> +        * disables this feature and restores normal behavior.
>> +        */
>
> Thank you for clearly commenting on an undocumented "feature"!
>
> Even 'tho there's changes requested already I've moved this to Remy in
> patchwork because I want his comments as the USB maintainer as well.
> Thanks!

Yes fine, I will CC him the next version,
in case if  there are no comments until I post v2.

--
Thanks,
Govindraj.R