On Fri, Nov 01, 2013 at 11:41:05AM -0400, Alan Stern wrote:
> > diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> > index dfe9d0f..3ad2205 100644
> > --- a/drivers/usb/core/hcd-pci.c
> > +++ b/drivers/usb/core/hcd-pci.c
> > @@ -267,6 +267,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct
> > pci_device_id *id)
> > retval = usb_add_hcd(hcd, hcd_irq, IRQF_SHARED);
> > if (retval != 0)
> > dev_set_drvdata(&dev->dev, NULL);
> > + device_wakeup_enable(hcd->self.controller);
> > for_each_companion(dev, hcd, ehci_post_add);
> > up_write(&companions_rwsem);
> > } else {
> > @@ -277,6 +278,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct
> > pci_device_id *id)
> > dev_set_drvdata(&dev->dev, NULL);
> > else
> > for_each_companion(dev, hcd, non_ehci_add);
> > + device_wakeup_enable(hcd->self.controller);
> > up_read(&companions_rwsem);
> > }
>
> It would be better to put a single call to device_wakeup_enable() a few
> lines farther down, after the "goto unmap_registers" line.
>
> > diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> > index a06d501..bf405fd 100644
> > --- a/drivers/usb/host/ehci-fsl.c
> > +++ b/drivers/usb/host/ehci-fsl.c
> > @@ -139,6 +139,7 @@ static int usb_hcd_fsl_probe(const struct hc_driver
> > *driver,
> > if (retval != 0)
> > goto err4;
> >
> > + device_wakeup_enable(hcd->self.controller);
> > #ifdef CONFIG_USB_OTG
> > if (pdata->operating_mode == FSL_USB2_DR_OTG) {
> > struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>
> Here and in some other places, the patch displays a bad sense of style.
> The device_wakeup_enable() call belongs along with the other
> hcd-related statements; it has nothing to do with CONFIG_USB_OTG.
> Therefore you should put a blank line before the "#ifdef".
>
> (You could also consider removing the blank line that precedes
> device_wakeup_enable().)
So the basic code stype is: before comment and MACRO, there is
a blank line.
But after if (), keep a blank line will let code look like clean, do
you think so?
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index b8dffd5..bd4213b 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -210,6 +210,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const
> > struct pci_device_id *id)
> > IRQF_SHARED);
> > if (retval)
> > goto put_usb3_hcd;
> > + device_wakeup_enable(xhci->shared_hcd->self.controller);
> > /* Roothub already marked as USB 3.0 speed */
> >
> > /* We know the LPM timeout algorithms for this host, let the USB core
>
> xhci-hcd is tricky. It registers two hcd structures, but they have the
> same parent controller. In xhci-pci.c, the first hcd is registered by
> the call to usb_hcd_pci_probe(). That call enables wakeup, so you
> don't need to enable it again here.
>
For pci device, do you want hcd-pci to control wakeup setting together?
or let each pci(xhci/ehci/ohci/uhci-pci) driver controller it?
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index d9c169f..a24e406 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -140,6 +140,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > if (ret)
> > goto unmap_registers;
> >
> > + device_wakeup_enable(hcd->self.controller);
> > /* USB 2.0 roothub is stored in the platform_device now. */
> > hcd = platform_get_drvdata(pdev);
> > xhci = hcd_to_xhci(hcd);
> > @@ -160,6 +161,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > if (ret)
> > goto put_usb3_hcd;
> >
> > + device_wakeup_enable(xhci->shared_hcd->self.controller);
> > return 0;
> >
> > put_usb3_hcd:
>
> A similar comment applies to xhci-plat.c. The first
> device_wakeup_enable() call is sufficient; you don't need to add the
> second one.
Yes, will change.
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
