On Thu, 31 Oct 2013, Peter Chen wrote:
> Individual controller driver has different requirement for wakeup
> setting, so move it from core to itself. In order to align with
> current etting the default wakeup setting is enabled (except for
> chipidea host).
>
> Pass compile test with below commands:
> make O=outout/all allmodconfig
> make -j$CPU_NUM O=outout/all drivers/usb
>
> Signed-off-by: Peter Chen <[email protected]>
This is basically right. I have only a couple of minor comments...
> diff --git a/drivers/staging/usbip/vhci_hcd.c
> b/drivers/staging/usbip/vhci_hcd.c
> index d7974cb..d618a19 100644
> --- a/drivers/staging/usbip/vhci_hcd.c
> +++ b/drivers/staging/usbip/vhci_hcd.c
> @@ -1030,6 +1030,7 @@ static int vhci_hcd_probe(struct platform_device *pdev)
> the_controller = NULL;
> return ret;
> }
> + device_wakeup_enable(hcd->self.controller);
>
> usbip_dbg_vhci_hc("bye\n");
> return 0;
The host controller in usbip, like the one in dummy_hcd, is a virtual
device. It can't generate interrupts or wakeup events. So this change
isn't needed.
> 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().)
> diff --git a/drivers/usb/host/ohci-sm501.c b/drivers/usb/host/ohci-sm501.c
> index 2a5de5f..2a01c24 100644
> --- a/drivers/usb/host/ohci-sm501.c
> +++ b/drivers/usb/host/ohci-sm501.c
> @@ -169,6 +169,7 @@ static int ohci_hcd_sm501_drv_probe(struct
> platform_device *pdev)
> if (retval)
> goto err5;
>
> + device_wakeup_enable(hcd->self.controller);
> /* enable power and unmask interrupts */
>
> sm501_unit_power(dev->parent, SM501_GATE_USB_HOST, 1);
This is another example of bad style.
> 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.
> 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.
> diff --git a/drivers/usb/renesas_usbhs/mod_host.c
> b/drivers/usb/renesas_usbhs/mod_host.c
> index e40f565..e564ec5 100644
> --- a/drivers/usb/renesas_usbhs/mod_host.c
> +++ b/drivers/usb/renesas_usbhs/mod_host.c
> @@ -1470,6 +1470,7 @@ static int usbhsh_start(struct usbhs_priv *priv)
> if (ret < 0)
> return 0;
>
> + device_wakeup_enable(hcd->self.controller);
> /*
> * pipe initialize and enable DCP
> */
Here is another example of bad style.
After you fix up these things, you can add my Acked-by: to the patch.
Alan Stern
--
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
