On Tue, Oct 16, 2012 at 10:14:43AM -0400, Alan Stern wrote:
> On Tue, 16 Oct 2012, Peter Chen wrote:
>
> > Without this condition, all controllers will do this delay,
> > and increase the resume time.
> >
> > Only enabled and unsuspended port needs this delay, but
> > Some buggy hardware(like Synopsys usb controller) will
> > clear suspend bit once they receive/send resume signal,
> > so it takes resume bit as consideration.
> >
> > Tested it at Freescale i.mx6q Sabrelite board.
>
> This can be improved a little bit.
>
> > Signed-off-by: Peter Chen <peter.chen@xxxxxxxxxxxxx>
> > ---
> > drivers/usb/host/ehci-hub.c | 22 +++++++++++++++++-----
> > 1 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
> > index c788022..6505063 100644
> > --- a/drivers/usb/host/ehci-hub.c
> > +++ b/drivers/usb/host/ehci-hub.c
> > @@ -384,11 +384,23 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
> > ehci_writel(ehci, ehci->command, &ehci->regs->command);
> > ehci->rh_state = EHCI_RH_RUNNING;
> >
> > - /* Some controller/firmware combinations need a delay during which
> > - * they set up the port statuses. See Bugzilla #8190. */
> > - spin_unlock_irq(&ehci->lock);
> > - msleep(8);
> > - spin_lock_irq(&ehci->lock);
> > + /*
> > + * According to Bugzilla #8190, the port status for some controllers
> > + * will be wrong without a delay. At their wrong status, the port
> > + * is enabled, but not suspended neither resumed.
> > + */
> > + i = HCS_N_PORTS (ehci->hcs_params);
> > + while (i--) {
> > + temp = ehci_readl(ehci, &ehci->regs->port_status [i]);
>
> No space before the '['.
A lot of place has this problem at this file, besides, there is a space
before ')', like "struct ehci_hcd *ehci = hcd_to_ehci (hcd);",
Do you need I submit a patch to fix this coding style problem?
>
> > + if ((temp & PORT_PE) &&
> > + !(temp & (PORT_SUSPEND | PORT_RESUME))) {
> > + ehci_warn(ehci, "Port status(0x%x) is error\n", temp);
>
> s/is error/is wrong/
>
> Also, use ehci_dbg(), not ehci_warn(). This isn't an indication of a
> failure so it doesn't need to be a warning.
>
> > + spin_unlock_irq(&ehci->lock);
> > + msleep(8);
> > + spin_lock_irq(&ehci->lock);
>
> Add "break;" here. You don't want to have multiple delays. :-)
>
> > + }
> > + }
> > +
>
> Alan Stern
>
>
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
