Re: [PATCH/RFT v2 11/17] USB: OHCI: make ohci-da8xx a separate driver

[Axel Haslam <ahaslam@baylibre.com>]

On Tue, Oct 25, 2016 at 2:38 AM, David Lechner <david@lechnology.com> wrote:
> On 10/24/2016 11:46 AM, ahaslam@baylibre.com wrote:
>>
>> From: Manjunath Goudar <manjunath.goudar@linaro.org>
>>
>> Separate the Davinci OHCI host controller driver from ohci-hcd
>> host code so that it can be built as a separate driver module.
>> This work is part of enabling multi-platform kernels on ARM;
>> it would be nice to have in 3.11.
>
>
> No need for comment about kernel 3.11.

yes, ok.

>
>>
>> Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
>> ---
>>  drivers/usb/host/Kconfig      |   2 +-
>>  drivers/usb/host/Makefile     |   1 +
>>  drivers/usb/host/ohci-da8xx.c | 185
>> +++++++++++++++++-------------------------
>>  drivers/usb/host/ohci-hcd.c   |  18 ----
>>  4 files changed, 76 insertions(+), 130 deletions(-)
>>
>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>> index 83b6cec..642c6fe8 100644
>> --- a/drivers/usb/host/Kconfig
>> +++ b/drivers/usb/host/Kconfig
>> @@ -479,7 +479,7 @@ config USB_OHCI_HCD_OMAP3
>>           OMAP3 and later chips.
>>
>>  config USB_OHCI_HCD_DAVINCI
>> -       bool "OHCI support for TI DaVinci DA8xx"
>> +       tristate "OHCI support for TI DaVinci DA8xx"
>>         depends on ARCH_DAVINCI_DA8XX
>>         depends on USB_OHCI_HCD=y
>
>
> Need to drop the "=y" here, otherwise you still can't compile this as a
> module.

Im able to complile it as a module, but ok ill remove it.

>
>>         select PHY_DA8XX_USB
>> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
>> index 6ef785b..2644537 100644
>> --- a/drivers/usb/host/Makefile
>> +++ b/drivers/usb/host/Makefile
>> @@ -61,6 +61,7 @@ obj-$(CONFIG_USB_OHCI_HCD_AT91)       += ohci-at91.o
>>  obj-$(CONFIG_USB_OHCI_HCD_S3C2410)     += ohci-s3c2410.o
>>  obj-$(CONFIG_USB_OHCI_HCD_LPC32XX)     += ohci-nxp.o
>>  obj-$(CONFIG_USB_OHCI_HCD_PXA27X)      += ohci-pxa27x.o
>> +obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)     += ohci-da8xx.o
>>
>>  obj-$(CONFIG_USB_UHCI_HCD)     += uhci-hcd.o
>>  obj-$(CONFIG_USB_FHCI_HCD)     += fhci.o
>> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
>> index e98066d..5585d9e 100644
>> --- a/drivers/usb/host/ohci-da8xx.c
>> +++ b/drivers/usb/host/ohci-da8xx.c
>> @@ -11,16 +11,31 @@
>>   * kind, whether express or implied.
>>   */
>>
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/jiffies.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>>  #include <linux/platform_device.h>
>> -#include <linux/clk.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/platform_data/usb-davinci.h>
>> +#include <linux/platform_device.h>
>
>
> linux/platform_device.h is listed twice
>
>> +#include <linux/usb.h>
>> +#include <linux/usb/hcd.h>
>> +#include <asm/unaligned.h>
>>
>> -#ifndef CONFIG_ARCH_DAVINCI_DA8XX
>> -#error "This file is DA8xx bus glue.  Define CONFIG_ARCH_DAVINCI_DA8XX."
>> -#endif
>> +#include "ohci.h"
>> +
>> +#define DRIVER_DESC "OHCI DA8XX driver"
>> +
>> +static const char hcd_name[] = "ohci-da8xx";
>
>
> why static const char instead of #define? This is only used one time in a
> pr_info, so it seems kind of pointless anyway.

Other drivers are using static const for the same variable.
i think static const is preferred over #define because #define doet give a type.
If you dont mind ill keep it static const.


>
>> +
>> +static struct hc_driver __read_mostly ohci_da8xx_hc_driver;
>> +
>> +static int (*orig_ohci_hub_control)(struct usb_hcd  *hcd, u16 typeReq,
>> +                       u16 wValue, u16 wIndex, char *buf, u16 wLength);
>> +static int (*orig_ohci_hub_status_data)(struct usb_hcd *hcd, char *buf);
>>
>>  static struct clk *usb11_clk;
>>  static struct phy *usb11_phy;
>> @@ -73,7 +88,7 @@ static void ohci_da8xx_ocic_handler(struct
>> da8xx_ohci_root_hub *hub)
>>                 hub->set_power(0);
>>  }
>>
>> -static int ohci_da8xx_init(struct usb_hcd *hcd)
>> +static int ohci_da8xx_reset(struct usb_hcd *hcd)
>>  {
>>         struct device *dev              = hcd->self.controller;
>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(dev);
>> @@ -93,7 +108,7 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
>>          */
>>         ohci->num_ports = 1;
>>
>> -       result = ohci_init(ohci);
>> +       result = ohci_setup(hcd);
>>         if (result < 0) {
>>                 ohci_da8xx_disable();
>>                 return result;
>> @@ -121,30 +136,12 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
>>         return result;
>>  }
>>
>> -static void ohci_da8xx_stop(struct usb_hcd *hcd)
>> -{
>> -       ohci_stop(hcd);
>> -       ohci_da8xx_disable();
>> -}
>> -
>> -static int ohci_da8xx_start(struct usb_hcd *hcd)
>> -{
>> -       struct ohci_hcd *ohci           = hcd_to_ohci(hcd);
>> -       int result;
>> -
>> -       result = ohci_run(ohci);
>> -       if (result < 0)
>> -               ohci_da8xx_stop(hcd);
>> -
>> -       return result;
>> -}
>> -
>>  /*
>>   * Update the status data from the hub with the over-current indicator
>> change.
>>   */
>>  static int ohci_da8xx_hub_status_data(struct usb_hcd *hcd, char *buf)
>>  {
>> -       int length              = ohci_hub_status_data(hcd, buf);
>> +       int length              = orig_ohci_hub_status_data(hcd, buf);
>>
>>         /* See if we have OCIC flag set */
>>         if (ocic_flag) {
>> @@ -226,66 +223,13 @@ static int ohci_da8xx_hub_control(struct usb_hcd
>> *hcd, u16 typeReq, u16 wValue,
>>                 }
>>         }
>>
>> -       return ohci_hub_control(hcd, typeReq, wValue, wIndex, buf,
>> wLength);
>> +       return orig_ohci_hub_control(hcd, typeReq, wValue,
>> +                       wIndex, buf, wLength);
>>  }
>>
>> -static const struct hc_driver ohci_da8xx_hc_driver = {
>> -       .description            = hcd_name,
>> -       .product_desc           = "DA8xx OHCI",
>> -       .hcd_priv_size          = sizeof(struct ohci_hcd),
>> -
>> -       /*
>> -        * generic hardware linkage
>> -        */
>> -       .irq                    = ohci_irq,
>> -       .flags                  = HCD_USB11 | HCD_MEMORY,
>> -
>> -       /*
>> -        * basic lifecycle operations
>> -        */
>> -       .reset                  = ohci_da8xx_init,
>> -       .start                  = ohci_da8xx_start,
>> -       .stop                   = ohci_da8xx_stop,
>> -       .shutdown               = ohci_shutdown,
>> -
>> -       /*
>> -        * managing i/o requests and associated device resources
>> -        */
>> -       .urb_enqueue            = ohci_urb_enqueue,
>> -       .urb_dequeue            = ohci_urb_dequeue,
>> -       .endpoint_disable       = ohci_endpoint_disable,
>> -
>> -       /*
>> -        * scheduling support
>> -        */
>> -       .get_frame_number       = ohci_get_frame,
>> -
>> -       /*
>> -        * root hub support
>> -        */
>> -       .hub_status_data        = ohci_da8xx_hub_status_data,
>> -       .hub_control            = ohci_da8xx_hub_control,
>> -
>> -#ifdef CONFIG_PM
>> -       .bus_suspend            = ohci_bus_suspend,
>> -       .bus_resume             = ohci_bus_resume,
>> -#endif
>> -       .start_port_reset       = ohci_start_port_reset,
>> -};
>> -
>>
>> /*-------------------------------------------------------------------------*/
>>
>> -
>> -/**
>> - * usb_hcd_da8xx_probe - initialize DA8xx-based HCDs
>> - * Context: !in_interrupt()
>> - *
>> - * Allocates basic resources for this USB host controller, and
>> - * then invokes the start() method for the HCD associated with it
>> - * through the hotplug entry's driver_data.
>> - */
>> -static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
>> -                              struct platform_device *pdev)
>> +static int ohci_da8xx_probe(struct platform_device *pdev)
>>  {
>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(&pdev->dev);
>>         struct usb_hcd  *hcd;
>> @@ -295,6 +239,11 @@ static int usb_hcd_da8xx_probe(const struct hc_driver
>> *driver,
>>         if (hub == NULL)
>>                 return -ENODEV;
>>
>> +       hcd = usb_create_hcd(&ohci_da8xx_hc_driver, &pdev->dev,
>> +                               dev_name(&pdev->dev));
>> +       if (!hcd)
>> +               return -ENOMEM;
>> +
>
>
> Won't this leak hdc if there is an error later?
>
>>         usb11_clk = devm_clk_get(&pdev->dev, "usb11");
>>         if (IS_ERR(usb11_clk)) {
>>                 if (PTR_ERR(usb11_clk) != -EPROBE_DEFER)
>> @@ -309,9 +258,6 @@ static int usb_hcd_da8xx_probe(const struct hc_driver
>> *driver,
>>                 return PTR_ERR(usb11_phy);
>>         }
>>
>> -       hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
>> -       if (!hcd)
>> -               return -ENOMEM;
>
>
> Why does this need to be moved?
>

it should not have moved this, will fix.


>>
>>         mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>         hcd->regs = devm_ioremap_resource(&pdev->dev, mem);
>> @@ -323,13 +269,12 @@ static int usb_hcd_da8xx_probe(const struct
>> hc_driver *driver,
>>         hcd->rsrc_start = mem->start;
>>         hcd->rsrc_len = resource_size(mem);
>>
>> -       ohci_hcd_init(hcd_to_ohci(hcd));
>> -
>>         irq = platform_get_irq(pdev, 0);
>>         if (irq < 0) {
>>                 error = -ENODEV;
>>                 goto err;
>>         }
>> +
>>         error = usb_add_hcd(hcd, irq, 0);
>>         if (error)
>>                 goto err;
>> @@ -348,35 +293,14 @@ static int usb_hcd_da8xx_probe(const struct
>> hc_driver *driver,
>>         return error;
>>  }
>>
>> -/**
>> - * usb_hcd_da8xx_remove - shutdown processing for DA8xx-based HCDs
>> - * @dev: USB Host Controller being removed
>> - * Context: !in_interrupt()
>> - *
>> - * Reverses the effect of usb_hcd_da8xx_probe(), first invoking
>> - * the HCD's stop() method.  It is always called from a thread
>> - * context, normally "rmmod", "apmd", or something similar.
>> - */
>> -static inline void
>> -usb_hcd_da8xx_remove(struct usb_hcd *hcd, struct platform_device *pdev)
>> +static int ohci_da8xx_remove(struct platform_device *pdev)
>>  {
>> +       struct usb_hcd  *hcd = platform_get_drvdata(pdev);
>>         struct da8xx_ohci_root_hub *hub = dev_get_platdata(&pdev->dev);
>>
>>         hub->ocic_notify(NULL);
>>         usb_remove_hcd(hcd);
>>         usb_put_hcd(hcd);
>> -}
>> -
>> -static int ohci_hcd_da8xx_drv_probe(struct platform_device *dev)
>> -{
>> -       return usb_hcd_da8xx_probe(&ohci_da8xx_hc_driver, dev);
>> -}
>> -
>> -static int ohci_hcd_da8xx_drv_remove(struct platform_device *dev)
>> -{
>> -       struct usb_hcd  *hcd = platform_get_drvdata(dev);
>> -
>> -       usb_hcd_da8xx_remove(hcd, dev);
>>
>>         return 0;
>>  }
>> @@ -426,12 +350,16 @@ static int ohci_da8xx_resume(struct platform_device
>> *dev)
>>  }
>>  #endif
>>
>> +static const struct ohci_driver_overrides da8xx_overrides __initconst = {
>> +       .reset          = ohci_da8xx_reset
>> +};
>> +
>>  /*
>>   * Driver definition to register with platform structure.
>>   */
>>  static struct platform_driver ohci_hcd_da8xx_driver = {
>> -       .probe          = ohci_hcd_da8xx_drv_probe,
>> -       .remove         = ohci_hcd_da8xx_drv_remove,
>> +       .probe          = ohci_da8xx_probe,
>> +       .remove         = ohci_da8xx_remove,
>>         .shutdown       = usb_hcd_platform_shutdown,
>>  #ifdef CONFIG_PM
>>         .suspend        = ohci_da8xx_suspend,
>
>
> It would probably be a good idea to change the driver name here. Currently
> it is "ohci". Although this would be better in a separate patch if the name
> has to be changed to match in other files as well.
>

ok, ill do a separate patch for that.

>> @@ -442,4 +370,39 @@ static int ohci_da8xx_resume(struct platform_device
>> *dev)
>>         },
>>  };
>>
>> +static int __init ohci_da8xx_init(void)
>> +{
>> +
>> +       if (usb_disabled())
>> +               return -ENODEV;
>> +
>> +       pr_info("%s: " DRIVER_DESC "\n", hcd_name);
>> +       ohci_init_driver(&ohci_da8xx_hc_driver, &da8xx_overrides);
>> +
>> +       /*
>> +        * The Davinci da8xx HW has some unusual quirks, which require
>> +        * da8xx-specific workarounds. We override certain hc_driver
>> +        * functions here to achieve that. We explicitly do not enhance
>> +        * ohci_driver_overrides to allow this more easily, since this
>> +        * is an unusual case, and we don't want to encourage others to
>> +        * override these functions by making it too easy.
>> +        */
>> +
>> +       orig_ohci_hub_control = ohci_da8xx_hc_driver.hub_control;
>> +       orig_ohci_hub_status_data = ohci_da8xx_hc_driver.hub_status_data;
>> +
>> +       ohci_da8xx_hc_driver.hub_status_data     =
>> ohci_da8xx_hub_status_data;
>> +       ohci_da8xx_hc_driver.hub_control         = ohci_da8xx_hub_control;
>> +
>> +       return platform_driver_register(&ohci_hcd_da8xx_driver);
>> +}
>> +module_init(ohci_da8xx_init);
>> +
>> +static void __exit ohci_da8xx_cleanup(void)
>
>
> ohci_da8xx_exit would be a better name
>

ok.

>> +{
>> +       platform_driver_unregister(&ohci_hcd_da8xx_driver);
>> +}
>> +module_exit(ohci_da8xx_cleanup);
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> +MODULE_LICENSE("GPL");
>>  MODULE_ALIAS("platform:ohci");
>
>
> this will need to be changed too if you change the driver name
>
>> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
>> index 1700908..8de174a 100644
>> --- a/drivers/usb/host/ohci-hcd.c
>> +++ b/drivers/usb/host/ohci-hcd.c
>> @@ -1219,11 +1219,6 @@ void ohci_init_driver(struct hc_driver *drv,
>>  #define SA1111_DRIVER          ohci_hcd_sa1111_driver
>>  #endif
>>
>> -#ifdef CONFIG_USB_OHCI_HCD_DAVINCI
>> -#include "ohci-da8xx.c"
>> -#define DAVINCI_PLATFORM_DRIVER        ohci_hcd_da8xx_driver
>> -#endif
>> -
>>  #ifdef CONFIG_USB_OHCI_HCD_PPC_OF
>>  #include "ohci-ppc-of.c"
>>  #define OF_PLATFORM_DRIVER     ohci_hcd_ppc_of_driver
>> @@ -1303,19 +1298,9 @@ static int __init ohci_hcd_mod_init(void)
>>                 goto error_tmio;
>>  #endif
>>
>> -#ifdef DAVINCI_PLATFORM_DRIVER
>> -       retval = platform_driver_register(&DAVINCI_PLATFORM_DRIVER);
>> -       if (retval < 0)
>> -               goto error_davinci;
>> -#endif
>> -
>>         return retval;
>>
>>         /* Error path */
>> -#ifdef DAVINCI_PLATFORM_DRIVER
>> -       platform_driver_unregister(&DAVINCI_PLATFORM_DRIVER);
>> - error_davinci:
>> -#endif
>>  #ifdef TMIO_OHCI_DRIVER
>>         platform_driver_unregister(&TMIO_OHCI_DRIVER);
>>   error_tmio:
>> @@ -1351,9 +1336,6 @@ static int __init ohci_hcd_mod_init(void)
>>
>>  static void __exit ohci_hcd_mod_exit(void)
>>  {
>> -#ifdef DAVINCI_PLATFORM_DRIVER
>> -       platform_driver_unregister(&DAVINCI_PLATFORM_DRIVER);
>> -#endif
>>  #ifdef TMIO_OHCI_DRIVER
>>         platform_driver_unregister(&TMIO_OHCI_DRIVER);
>>  #endif
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html