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

From: Axel Haslam
Date: Tue Oct 25 2016 - 03:40:08 EST

On Tue, Oct 25, 2016 at 2:38 AM, David Lechner <david@xxxxxxxxxxxxxx> wrote:
> On 10/24/2016 11:46 AM, ahaslam@xxxxxxxxxxxx wrote:
>>
>> From: Manjunath Goudar <manjunath.goudar@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html