lkml.org 
[lkml]   [2016]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH/RFT v2 11/17] USB: OHCI: make ohci-da8xx a separate driver
    From
    Date
    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.

    >
    > 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.

    > 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
    >

    \
     
     \ /
      Last update: 2016-10-25 02:39    [W:4.600 / U:0.328 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site