| Message ID | 1360923853-7875-2-git-send-email-manjunath.goudar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 15 Feb 2013, Manjunath Goudar wrote: > Separate the SPEAr host controller driver from ehci-hcd host code > into its own driver module. > > In V2: > Replaced spear as SPEAr everywhere, leaving functions/variables/config options. > --- a/drivers/usb/host/Kconfig > +++ b/drivers/usb/host/Kconfig > @@ -162,6 +162,14 @@ config USB_EHCI_HCD_OMAP > Enables support for the on-chip EHCI controller on > OMAP3 and later chips. > > +config USB_EHCI_HCD_SPEAR > + tristate "Support for ST SPEAr on-chip EHCI USB controller" > + depends on USB_EHCI_HCD && PLAT_SPEAR > + default y > + ---help--- > + Enables support for the on-chip EHCI controller on > + ST SPEAr chips. Is it a good idea to make this option interactive? That might cause people to disable it by mistake. > --- a/drivers/usb/host/Makefile > +++ b/drivers/usb/host/Makefile > @@ -27,7 +27,7 @@ obj-$(CONFIG_USB_EHCI_HCD) += ehci-hcd.o > obj-$(CONFIG_USB_EHCI_PCI) += ehci-pci.o > obj-$(CONFIG_USB_EHCI_HCD_PLATFORM) += ehci-platform.o > obj-$(CONFIG_USB_EHCI_MXC) += ehci-mxc.o > - > +obj-$(CONFIG_USB_EHCI_HCD_SPEAR)+= ehci-spear.o Please don't eliminate the blank line that separates the EHCI drivers from the following non-EHCI drivers. > obj-$(CONFIG_USB_OXU210HP_HCD) += oxu210hp-hcd.o > obj-$(CONFIG_USB_ISP116X_HCD) += isp116x-hcd.o > obj-$(CONFIG_USB_ISP1362_HCD) += isp1362-hcd.o > --- a/drivers/usb/host/ehci-spear.c > +++ b/drivers/usb/host/ehci-spear.c > +static const char hcd_name[] = "ehci-SPEAr"; > @@ -209,11 +188,35 @@ static struct platform_driver spear_ehci_hcd_driver = { > .remove = spear_ehci_hcd_drv_remove, > .shutdown = usb_hcd_platform_shutdown, > .driver = { > - .name = "spear-ehci", > + .name = hcd_name, You must not change the driver's name. It won't work on non-DT systems; the platform bus relies on matching drivers to devices by comparing their names. > .bus = &platform_bus_type, > .pm = &ehci_spear_pm_ops, > .of_match_table = of_match_ptr(spear_ehci_id_table), > } > }; > > -MODULE_ALIAS("platform:spear-ehci"); You must not remove the MODULE_ALIAS. > +static const struct ehci_driver_overrides spear_overrides __initdata = { > + .reset = ehci_spear_setup, > +}; You forgot to use the .extra_priv_size field. It will allow the driver to be simplified by storing the "clk" field of struct spear_ehci in the private part of the ehci_hcd structure. Also, you can completely eliminate the ehci_spear_setup routine if you move the lines /* registers start at offset 0x0 */ ehci->caps = hcd->regs; into spear_ehci_hcd_drv_probe. > +MODULE_DESCRIPTION(DRIVER_DESC); > +MODULE_LICENSE("GPL"); There probably should be a MODULE_AUTHOR field, yes? Alan Stern
On 20 February 2013 21:43, Alan Stern <stern@rowland.harvard.edu> wrote: > On Fri, 15 Feb 2013, Manjunath Goudar wrote: > > > Separate the SPEAr host controller driver from ehci-hcd host code > > into its own driver module. > > > > In V2: > > Replaced spear as SPEAr everywhere, leaving functions/variables/config > options. > > > --- a/drivers/usb/host/Kconfig > > +++ b/drivers/usb/host/Kconfig > > @@ -162,6 +162,14 @@ config USB_EHCI_HCD_OMAP > > Enables support for the on-chip EHCI controller on > > OMAP3 and later chips. > > > > +config USB_EHCI_HCD_SPEAR > > + tristate "Support for ST SPEAr on-chip EHCI USB controller" > > + depends on USB_EHCI_HCD && PLAT_SPEAR > > + default y > > + ---help--- > > + Enables support for the on-chip EHCI controller on > > + ST SPEAr chips. > > Is it a good idea to make this option interactive? That might cause > people to disable it by mistake. > > Thank you very much your approach. This is my approach, who are by mistake disabling. config USB_EHCI_HCD_SPEAR tristate "Support for ST SPEAr on-chip EHCI USB controller" depends on PLAT_SPEAR select USB_EHCI_HCD default y ---help--- Enables support for the on-chip EHCI controller on ST SPEAr chips. Is it ok. > > --- a/drivers/usb/host/Makefile > > +++ b/drivers/usb/host/Makefile > > @@ -27,7 +27,7 @@ obj-$(CONFIG_USB_EHCI_HCD) += ehci-hcd.o > > obj-$(CONFIG_USB_EHCI_PCI) += ehci-pci.o > > obj-$(CONFIG_USB_EHCI_HCD_PLATFORM) += ehci-platform.o > > obj-$(CONFIG_USB_EHCI_MXC) += ehci-mxc.o > > - > > +obj-$(CONFIG_USB_EHCI_HCD_SPEAR)+= ehci-spear.o > > Please don't eliminate the blank line that separates the EHCI drivers > from the following non-EHCI drivers. > Thank you. Sure I will making these change in v3 version patch list > > > obj-$(CONFIG_USB_OXU210HP_HCD) += oxu210hp-hcd.o > > obj-$(CONFIG_USB_ISP116X_HCD) += isp116x-hcd.o > > obj-$(CONFIG_USB_ISP1362_HCD) += isp1362-hcd.o > > > --- a/drivers/usb/host/ehci-spear.c > > +++ b/drivers/usb/host/ehci-spear.c > > > +static const char hcd_name[] = "ehci-SPEAr"; > > > @@ -209,11 +188,35 @@ static struct platform_driver > spear_ehci_hcd_driver = { > > .remove = spear_ehci_hcd_drv_remove, > > .shutdown = usb_hcd_platform_shutdown, > > .driver = { > > - .name = "spear-ehci", > > + .name = hcd_name, > > You must not change the driver's name. It won't work on non-DT > systems; the platform bus relies on matching drivers to devices by > comparing their names. > > Here I am planing to avoid two different string for single driver "spear-ehci" is used in above initialization "ehci-spear" one is used for printing driver name in module_init, instead of two, why cant we go for single string "spear-ehci" only. > > .bus = &platform_bus_type, > > .pm = &ehci_spear_pm_ops, > > .of_match_table = of_match_ptr(spear_ehci_id_table), > > } > > }; > > > > -MODULE_ALIAS("platform:spear-ehci"); > > You must not remove the MODULE_ALIAS. > thank you.Already fixed this. > > +static const struct ehci_driver_overrides spear_overrides __initdata = { > > + .reset = ehci_spear_setup, > > +}; > > You forgot to use the .extra_priv_size field. It will allow the driver > to be simplified by storing the "clk" field of struct spear_ehci in the > private part of the ehci_hcd structure. > > Also, you can completely eliminate the ehci_spear_setup routine if you > move the lines > > /* registers start at offset 0x0 */ > ehci->caps = hcd->regs; > > into spear_ehci_hcd_drv_probe. > Thank you, sure I will modify as your suggestion. > > +MODULE_DESCRIPTION(DRIVER_DESC); > > +MODULE_LICENSE("GPL"); > > There probably should be a MODULE_AUTHOR field, yes? > > Alan Stern > > Yes I will add MODULE_AUTHOR field here.
On Thu, 21 Feb 2013, Manjunath Goudar wrote: > On 20 February 2013 21:43, Alan Stern <stern@rowland.harvard.edu> wrote: > > > On Fri, 15 Feb 2013, Manjunath Goudar wrote: > > > > > Separate the SPEAr host controller driver from ehci-hcd host code > > > into its own driver module. > > > > > > In V2: > > > Replaced spear as SPEAr everywhere, leaving functions/variables/config > > options. > > > > > --- a/drivers/usb/host/Kconfig > > > +++ b/drivers/usb/host/Kconfig > > > @@ -162,6 +162,14 @@ config USB_EHCI_HCD_OMAP > > > Enables support for the on-chip EHCI controller on > > > OMAP3 and later chips. > > > > > > +config USB_EHCI_HCD_SPEAR > > > + tristate "Support for ST SPEAr on-chip EHCI USB controller" > > > + depends on USB_EHCI_HCD && PLAT_SPEAR > > > + default y > > > + ---help--- > > > + Enables support for the on-chip EHCI controller on > > > + ST SPEAr chips. > > > > Is it a good idea to make this option interactive? That might cause > > people to disable it by mistake. > > > > Thank you very much your approach. > > This is my approach, who are by mistake disabling. > > config USB_EHCI_HCD_SPEAR > tristate "Support for ST SPEAr on-chip EHCI USB controller" > depends on PLAT_SPEAR > select USB_EHCI_HCD > default y > ---help--- > Enables support for the on-chip EHCI controller on > ST SPEAr chips. > > Is it ok. I'm not sure what you mean. But other people have said that it's better for these options to be interactive, so this is okay. > > > @@ -209,11 +188,35 @@ static struct platform_driver > > spear_ehci_hcd_driver = { > > > .remove = spear_ehci_hcd_drv_remove, > > > .shutdown = usb_hcd_platform_shutdown, > > > .driver = { > > > - .name = "spear-ehci", > > > + .name = hcd_name, > > > > You must not change the driver's name. It won't work on non-DT > > systems; the platform bus relies on matching drivers to devices by > > comparing their names. > > > > Here I am planing to avoid two different string for single driver > "spear-ehci" is used in above initialization "ehci-spear" one is used for > printing driver name in module_init, instead of two, why cant we go for > single string "spear-ehci" only. You need to have two different strings because there are two different entities with different names: The driver file is ehci-spear (or ehci-spear.ko for the module). The device is spear-ehci (this is determined by the platform code, not the USB stack). Alan Stern
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index 3a21c5d..15e8032 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -162,6 +162,14 @@ config USB_EHCI_HCD_OMAP Enables support for the on-chip EHCI controller on OMAP3 and later chips. +config USB_EHCI_HCD_SPEAR + tristate "Support for ST SPEAr on-chip EHCI USB controller" + depends on USB_EHCI_HCD && PLAT_SPEAR + default y + ---help--- + Enables support for the on-chip EHCI controller on + ST SPEAr chips. + config USB_EHCI_MSM bool "Support for MSM on-chip EHCI USB controller" depends on USB_EHCI_HCD && ARCH_MSM diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile index 001fbff..c8fcde9 100644 --- a/drivers/usb/host/Makefile +++ b/drivers/usb/host/Makefile @@ -27,7 +27,7 @@ obj-$(CONFIG_USB_EHCI_HCD) += ehci-hcd.o obj-$(CONFIG_USB_EHCI_PCI) += ehci-pci.o obj-$(CONFIG_USB_EHCI_HCD_PLATFORM) += ehci-platform.o obj-$(CONFIG_USB_EHCI_MXC) += ehci-mxc.o - +obj-$(CONFIG_USB_EHCI_HCD_SPEAR)+= ehci-spear.o obj-$(CONFIG_USB_OXU210HP_HCD) += oxu210hp-hcd.o obj-$(CONFIG_USB_ISP116X_HCD) += isp116x-hcd.o obj-$(CONFIG_USB_ISP1362_HCD) += isp1362-hcd.o diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index b416a3f..c4afd86 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -1297,11 +1297,6 @@ MODULE_LICENSE ("GPL"); #define PLATFORM_DRIVER vt8500_ehci_driver #endif -#ifdef CONFIG_PLAT_SPEAR -#include "ehci-spear.c" -#define PLATFORM_DRIVER spear_ehci_hcd_driver -#endif - #ifdef CONFIG_USB_EHCI_MSM #include "ehci-msm.c" #define PLATFORM_DRIVER ehci_msm_driver @@ -1346,6 +1341,7 @@ MODULE_LICENSE ("GPL"); !IS_ENABLED(CONFIG_USB_EHCI_HCD_PLATFORM) && \ !IS_ENABLED(CONFIG_USB_CHIPIDEA_HOST) && \ !IS_ENABLED(CONFIG_USB_EHCI_MXC) && \ + !IS_ENABLED(CONFIG_PLAT_SPEAR) && \ !defined(PLATFORM_DRIVER) && \ !defined(PS3_SYSTEM_BUS_DRIVER) && \ !defined(OF_PLATFORM_DRIVER) && \ diff --git a/drivers/usb/host/ehci-spear.c b/drivers/usb/host/ehci-spear.c index 466c1bb..87775a6 100644 --- a/drivers/usb/host/ehci-spear.c +++ b/drivers/usb/host/ehci-spear.c @@ -1,5 +1,5 @@ /* -* Driver for EHCI HCD on SPEAR SOC +* Driver for EHCI HCD on SPEAr SOC * * Copyright (C) 2010 ST Micro Electronics, * Deepak Sikri <deepak.sikri@st.com> @@ -12,10 +12,22 @@ */ #include <linux/clk.h> +#include <linux/dma-mapping.h> +#include <linux/io.h> #include <linux/jiffies.h> +#include <linux/kernel.h> +#include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> #include <linux/pm.h> +#include <linux/usb.h> +#include <linux/usb/hcd.h> + +#include "ehci.h" + +#define DRIVER_DESC "EHCI SPEAr driver" + +static const char hcd_name[] = "ehci-SPEAr"; struct spear_ehci { struct ehci_hcd ehci; @@ -43,40 +55,7 @@ static int ehci_spear_setup(struct usb_hcd *hcd) return ehci_setup(hcd); } - -static const struct hc_driver ehci_spear_hc_driver = { - .description = hcd_name, - .product_desc = "SPEAr EHCI", - .hcd_priv_size = sizeof(struct spear_ehci), - - /* generic hardware linkage */ - .irq = ehci_irq, - .flags = HCD_MEMORY | HCD_USB2, - - /* basic lifecycle operations */ - .reset = ehci_spear_setup, - .start = ehci_run, - .stop = ehci_stop, - .shutdown = ehci_shutdown, - - /* managing i/o requests and associated device resources */ - .urb_enqueue = ehci_urb_enqueue, - .urb_dequeue = ehci_urb_dequeue, - .endpoint_disable = ehci_endpoint_disable, - .endpoint_reset = ehci_endpoint_reset, - - /* scheduling support */ - .get_frame_number = ehci_get_frame, - - /* root hub support */ - .hub_status_data = ehci_hub_status_data, - .hub_control = ehci_hub_control, - .bus_suspend = ehci_bus_suspend, - .bus_resume = ehci_bus_resume, - .relinquish_port = ehci_relinquish_port, - .port_handed_over = ehci_port_handed_over, - .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete, -}; +static struct hc_driver __read_mostly ehci_spear_hc_driver; #ifdef CONFIG_PM static int ehci_spear_drv_suspend(struct device *dev) @@ -209,11 +188,35 @@ static struct platform_driver spear_ehci_hcd_driver = { .remove = spear_ehci_hcd_drv_remove, .shutdown = usb_hcd_platform_shutdown, .driver = { - .name = "spear-ehci", + .name = hcd_name, .bus = &platform_bus_type, .pm = &ehci_spear_pm_ops, .of_match_table = of_match_ptr(spear_ehci_id_table), } }; -MODULE_ALIAS("platform:spear-ehci"); + +static const struct ehci_driver_overrides spear_overrides __initdata = { + .reset = ehci_spear_setup, +}; + +static int __init ehci_spear_init(void) +{ + if (usb_disabled()) + return -ENODEV; + + pr_info("%s: " DRIVER_DESC "\n", hcd_name); + + ehci_init_driver(&ehci_spear_hc_driver, &spear_overrides); + return platform_driver_register(&spear_ehci_hcd_driver); +} +module_init(ehci_spear_init); + +static void __exit ehci_spear_cleanup(void) +{ + platform_driver_unregister(&spear_ehci_hcd_driver); +} +module_exit(ehci_spear_cleanup); + +MODULE_DESCRIPTION(DRIVER_DESC); +MODULE_LICENSE("GPL");