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

[Tom Rini]

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.

[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!

-- 
Tom