[PATCH/RFT v2 11/17] USB: OHCI: make ohci-da8xx a separate driver
David Lechner
david at lechnology.com
Mon Oct 24 17:38:11 PDT 2016
On 10/24/2016 11:46 AM, ahaslam at baylibre.com wrote:
> From: Manjunath Goudar <manjunath.goudar at 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.
>
> Signed-off-by: Manjunath Goudar <manjunath.goudar at 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.
> 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.
> +
> +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?
>
> 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.
> @@ -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
> +{
> + 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
>
More information about the linux-arm-kernel
mailing list