[V2,1/8] USB: EHCI: make ehci-spear a separate driver

Message ID 1360923853-7875-2-git-send-email-manjunath.goudar@linaro.org
State New
Headers show

Commit Message

manjunath.goudar@linaro.org Feb. 15, 2013, 10:24 a.m.
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.

Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
Signed-off-by: Deepak Saxena <dsaxena@linaro.org>
Cc: Greg KH <greg@kroah.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Acked-by: Viresh Kumar <viresh.linux@gmail.com>
Cc: Shiraz Hashim <shiraz.hashim@st.com>
Cc: linux-usb@vger.kernel.org
Cc: spear-devel@list.st.com
---
 drivers/usb/host/Kconfig      |    8 +++++
 drivers/usb/host/Makefile     |    2 +-
 drivers/usb/host/ehci-hcd.c   |    6 +---
 drivers/usb/host/ehci-spear.c |   77 +++++++++++++++++++++--------------------
 4 files changed, 50 insertions(+), 43 deletions(-)

Comments

Alan Stern Feb. 20, 2013, 4:13 p.m. | #1
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
manjunath.goudar@linaro.org Feb. 21, 2013, 8:18 a.m. | #2
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.
Alan Stern Feb. 21, 2013, 3:54 p.m. | #3
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

Patch

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");