| Message ID | 1365746856-7772-3-git-send-email-manjunath.goudar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Friday 12 April 2013, Manjunath Goudar wrote: > This patch splits the PCI portion of ohci-hcd out into its > own separate driver module, called ohci-pci. Consistently with the > current practice, the decision whether to build this module is not > user-configurable. If OHCI and PCI are enabled then the module will > be built, always. Have you tested this on a PC? That's something you should mention in the changelog above. > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > index 7a1a248..97a8f16 100644 > --- a/drivers/usb/host/Kconfig > +++ b/drivers/usb/host/Kconfig > @@ -333,6 +333,11 @@ config USB_ISP1362_HCD > To compile this driver as a module, choose M here: the > module will be called isp1362-hcd. > > +config USB_OHCI_PCI > + tristate "OHCI PCI support" > + depends on USB_OHCI_HCD && PCI > + default y > + > config USB_OHCI_HCD > tristate "OHCI HCD support" > depends on USB && USB_ARCH_HAS_OHCI While you say that the module will always be built when OHCI and PCI are enabled, that is not actually enforce here, it's just the default behavior that can be disabled. > diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c > index 0c1646b..b00edea 100644 > --- a/drivers/usb/host/ohci-hcd.c > +++ b/drivers/usb/host/ohci-hcd.c > @@ -81,7 +81,7 @@ static const char hcd_name [] = "ohci_hcd"; > static void ohci_dump (struct ohci_hcd *ohci, int verbose); > > #ifdef CONFIG_PCI > -static void sb800_prefetch(struct ohci_hcd *ohci, int on); > +void sb800_prefetch(struct ohci_hcd *ohci, int on); > #else > static inline void sb800_prefetch(struct ohci_hcd *ohci, int on) > { > @@ -584,7 +584,7 @@ int ohci_init(struct ohci_hcd *ohci) > > return ret; > I think it would be better to move that function from ohci-hcd.c into ohci-pci.c and keep it static. It does not seem to have any dependency on other symbols in ohci-hcd.c. > @@ -584,7 +584,7 @@ int ohci_init(struct ohci_hcd *ohci) > > return ret; > } > - > +EXPORT_SYMBOL_GPL(ohci_init); > /*-------------------------------------------------------------------------*/ > > /* Start an OHCI controller, set the BUS operational > ... > diff --git a/drivers/usb/host/ohci-mem.c b/drivers/usb/host/ohci-mem.c > index d44867b..f98727f 100644 > --- a/drivers/usb/host/ohci-mem.c > +++ b/drivers/usb/host/ohci-mem.c > @@ -29,7 +29,7 @@ void ohci_hcd_init (struct ohci_hcd *ohci) > spin_lock_init (&ohci->lock); > INIT_LIST_HEAD (&ohci->pending); > } > - > +EXPORT_SYMBOL_GPL(ohci_hcd_init); > /*-------------------------------------------------------------------------*/ > > static int ohci_mem_init (struct ohci_hcd *ohci) Ah, these hunks should have been in the first patch. > #ifdef CONFIG_PM > - .pci_suspend = ohci_suspend, > - .pci_resume = ohci_resume, > +#define ohci_suspend NULL > +#define ohci_resume NULL > #endif > What are you adding these macros for? They seem out of place. Arnd
On 12 April 2013 16:48, Arnd Bergmann <arnd@linaro.org> wrote: > On Friday 12 April 2013, Manjunath Goudar wrote: > > This patch splits the PCI portion of ohci-hcd out into its > > own separate driver module, called ohci-pci. Consistently with the > > current practice, the decision whether to build this module is not > > user-configurable. If OHCI and PCI are enabled then the module will > > be built, always. > > Have you tested this on a PC? That's something you should mention > in the changelog above. > > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > > index 7a1a248..97a8f16 100644 > > --- a/drivers/usb/host/Kconfig > > +++ b/drivers/usb/host/Kconfig > > @@ -333,6 +333,11 @@ config USB_ISP1362_HCD > > To compile this driver as a module, choose M here: the > > module will be called isp1362-hcd. > > > > +config USB_OHCI_PCI > > + tristate "OHCI PCI support" > > + depends on USB_OHCI_HCD && PCI > > + default y > > + > > config USB_OHCI_HCD > > tristate "OHCI HCD support" > > depends on USB && USB_ARCH_HAS_OHCI > > > While you say that the module will always be built when OHCI and PCI > are enabled, that is not actually enforce here, it's just the default > behavior that can be disabled. > Then you suggesting me to remove "depends on USB_OHCI_HCD && PCI" statements > diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c > > index 0c1646b..b00edea 100644 > > --- a/drivers/usb/host/ohci-hcd.c > > +++ b/drivers/usb/host/ohci-hcd.c > > @@ -81,7 +81,7 @@ static const char hcd_name [] = "ohci_hcd"; > > static void ohci_dump (struct ohci_hcd *ohci, int verbose); > > > > #ifdef CONFIG_PCI > > -static void sb800_prefetch(struct ohci_hcd *ohci, int on); > > +void sb800_prefetch(struct ohci_hcd *ohci, int on); > > #else > > static inline void sb800_prefetch(struct ohci_hcd *ohci, int on) > > { > > @@ -584,7 +584,7 @@ int ohci_init(struct ohci_hcd *ohci) > > > > return ret; > > > > I think it would be better to move that function from ohci-hcd.c > into ohci-pci.c and keep it static. It does not seem to have any > dependency on other symbols in ohci-hcd.c. > ya sure. > > > @@ -584,7 +584,7 @@ int ohci_init(struct ohci_hcd *ohci) > > > > return ret; > > } > > - > > +EXPORT_SYMBOL_GPL(ohci_init); > > > /*-------------------------------------------------------------------------*/ > > > > /* Start an OHCI controller, set the BUS operational > > > ... > > diff --git a/drivers/usb/host/ohci-mem.c b/drivers/usb/host/ohci-mem.c > > index d44867b..f98727f 100644 > > --- a/drivers/usb/host/ohci-mem.c > > +++ b/drivers/usb/host/ohci-mem.c > > @@ -29,7 +29,7 @@ void ohci_hcd_init (struct ohci_hcd *ohci) > > spin_lock_init (&ohci->lock); > > INIT_LIST_HEAD (&ohci->pending); > > } > > - > > +EXPORT_SYMBOL_GPL(ohci_hcd_init); > > > /*-------------------------------------------------------------------------*/ > > > > static int ohci_mem_init (struct ohci_hcd *ohci) > > Ah, these hunks should have been in the first patch. > > > #ifdef CONFIG_PM > > - .pci_suspend = ohci_suspend, > > - .pci_resume = ohci_resume, > > +#define ohci_suspend NULL > > +#define ohci_resume NULL > > #endif > > > > What are you adding these macros for? They seem out of place. > > For here now I am feeling not required,what you are saying? > Arnd > Manjunath Goudar
On Monday 15 April 2013, Manjunath Goudar wrote: > On 12 April 2013 16:48, Arnd Bergmann <arnd@linaro.org> wrote: > > > On Friday 12 April 2013, Manjunath Goudar wrote: > > > This patch splits the PCI portion of ohci-hcd out into its > > > own separate driver module, called ohci-pci. Consistently with the > > > current practice, the decision whether to build this module is not > > > user-configurable. If OHCI and PCI are enabled then the module will > > > be built, always. > > > > Have you tested this on a PC? That's something you should mention > > in the changelog above. > > > > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > > > index 7a1a248..97a8f16 100644 > > > --- a/drivers/usb/host/Kconfig > > > +++ b/drivers/usb/host/Kconfig > > > @@ -333,6 +333,11 @@ config USB_ISP1362_HCD > > > To compile this driver as a module, choose M here: the > > > module will be called isp1362-hcd. > > > > > > +config USB_OHCI_PCI > > > + tristate "OHCI PCI support" > > > + depends on USB_OHCI_HCD && PCI > > > + default y > > > + > > > config USB_OHCI_HCD > > > tristate "OHCI HCD support" > > > depends on USB && USB_ARCH_HAS_OHCI > > > > > > While you say that the module will always be built when OHCI and PCI > > are enabled, that is not actually enforce here, it's just the default > > behavior that can be disabled. > > > > > Then you suggesting me to remove "depends on USB_OHCI_HCD && PCI" > statements No, I think it's better to fix the changeset comment. > > > #ifdef CONFIG_PM > > > - .pci_suspend = ohci_suspend, > > > - .pci_resume = ohci_resume, > > > +#define ohci_suspend NULL > > > +#define ohci_resume NULL > > > #endif > > > > > > > What are you adding these macros for? They seem out of place. > > > > For here now I am feeling not required,what you are saying? I was just asking you for the purpose of adding the macros because it looks like they are not needed. Arnd
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index 7a1a248..97a8f16 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -333,6 +333,11 @@ config USB_ISP1362_HCD To compile this driver as a module, choose M here: the module will be called isp1362-hcd. +config USB_OHCI_PCI + tristate "OHCI PCI support" + depends on USB_OHCI_HCD && PCI + default y + config USB_OHCI_HCD tristate "OHCI HCD support" depends on USB && USB_ARCH_HAS_OHCI diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile index 58c14c1..39ddcca 100644 --- a/drivers/usb/host/Makefile +++ b/drivers/usb/host/Makefile @@ -41,6 +41,7 @@ obj-$(CONFIG_USB_OXU210HP_HCD) += oxu210hp-hcd.o obj-$(CONFIG_USB_ISP116X_HCD) += isp116x-hcd.o obj-$(CONFIG_USB_ISP1362_HCD) += isp1362-hcd.o obj-$(CONFIG_USB_OHCI_HCD) += ohci-hcd.o +obj-$(CONFIG_USB_OHCI_PCI) += ohci-pci.o obj-$(CONFIG_USB_UHCI_HCD) += uhci-hcd.o obj-$(CONFIG_USB_FHCI_HCD) += fhci.o obj-$(CONFIG_USB_XHCI_HCD) += xhci-hcd.o diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c index 0c1646b..b00edea 100644 --- a/drivers/usb/host/ohci-hcd.c +++ b/drivers/usb/host/ohci-hcd.c @@ -81,7 +81,7 @@ static const char hcd_name [] = "ohci_hcd"; static void ohci_dump (struct ohci_hcd *ohci, int verbose); #ifdef CONFIG_PCI -static void sb800_prefetch(struct ohci_hcd *ohci, int on); +void sb800_prefetch(struct ohci_hcd *ohci, int on); #else static inline void sb800_prefetch(struct ohci_hcd *ohci, int on) { @@ -584,7 +584,7 @@ int ohci_init(struct ohci_hcd *ohci) return ret; } - +EXPORT_SYMBOL_GPL(ohci_init); /*-------------------------------------------------------------------------*/ /* Start an OHCI controller, set the BUS operational @@ -1146,11 +1146,6 @@ MODULE_AUTHOR (DRIVER_AUTHOR); MODULE_DESCRIPTION(DRIVER_DESC); MODULE_LICENSE ("GPL"); -#ifdef CONFIG_PCI -#include "ohci-pci.c" -#define PCI_DRIVER ohci_pci_driver -#endif - #if defined(CONFIG_ARCH_SA1100) && defined(CONFIG_SA1111) #include "ohci-sa1111.c" #define SA1111_DRIVER ohci_hcd_sa1111_driver @@ -1246,7 +1241,7 @@ MODULE_LICENSE ("GPL"); #define PLATFORM_DRIVER ohci_platform_driver #endif -#if !defined(PCI_DRIVER) && \ +#if !IS_ENABLED(CONFIG_USB_OHCI_PCI) && \ !defined(PLATFORM_DRIVER) && \ !defined(OMAP1_PLATFORM_DRIVER) && \ !defined(OMAP3_PLATFORM_DRIVER) && \ @@ -1321,12 +1316,6 @@ static int __init ohci_hcd_mod_init(void) goto error_sa1111; #endif -#ifdef PCI_DRIVER - retval = pci_register_driver(&PCI_DRIVER); - if (retval < 0) - goto error_pci; -#endif - #ifdef SM501_OHCI_DRIVER retval = platform_driver_register(&SM501_OHCI_DRIVER); if (retval < 0) @@ -1420,10 +1409,6 @@ static int __init ohci_hcd_mod_init(void) platform_driver_unregister(&SM501_OHCI_DRIVER); error_sm501: #endif -#ifdef PCI_DRIVER - pci_unregister_driver(&PCI_DRIVER); - error_pci: -#endif #ifdef SA1111_DRIVER sa1111_driver_unregister(&SA1111_DRIVER); error_sa1111: diff --git a/drivers/usb/host/ohci-mem.c b/drivers/usb/host/ohci-mem.c index d44867b..f98727f 100644 --- a/drivers/usb/host/ohci-mem.c +++ b/drivers/usb/host/ohci-mem.c @@ -29,7 +29,7 @@ void ohci_hcd_init (struct ohci_hcd *ohci) spin_lock_init (&ohci->lock); INIT_LIST_HEAD (&ohci->pending); } - +EXPORT_SYMBOL_GPL(ohci_hcd_init); /*-------------------------------------------------------------------------*/ static int ohci_mem_init (struct ohci_hcd *ohci) diff --git a/drivers/usb/host/ohci-pci.c b/drivers/usb/host/ohci-pci.c index 951514e..76d668a 100644 --- a/drivers/usb/host/ohci-pci.c +++ b/drivers/usb/host/ohci-pci.c @@ -14,12 +14,19 @@ * This file is licenced under the GPL. */ -#ifndef CONFIG_PCI -#error "This file is PCI bus glue. CONFIG_PCI must be defined." -#endif - -#include <linux/pci.h> #include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/usb.h> +#include <linux/usb/hcd.h> + +#include "ohci.h" +#include "pci-quirks.h" + +#define DRIVER_DESC "OHCI PCI platform driver" + +static const char hcd_name[] = "ohci-pci"; /*-------------------------------------------------------------------------*/ @@ -175,7 +182,7 @@ static int ohci_quirk_amd700(struct usb_hcd *hcd) return 0; } -static void sb800_prefetch(struct ohci_hcd *ohci, int on) +void sb800_prefetch(struct ohci_hcd *ohci, int on) { struct pci_dev *pdev; u16 misc; @@ -187,7 +194,7 @@ static void sb800_prefetch(struct ohci_hcd *ohci, int on) else pci_write_config_word(pdev, 0x50, misc | 0x0300); } - +EXPORT_SYMBOL_GPL(sb800_prefetch); /* List of quirks for OHCI */ static const struct pci_device_id ohci_pci_quirks[] = { { @@ -299,57 +306,21 @@ static int ohci_pci_start (struct usb_hcd *hcd) /*-------------------------------------------------------------------------*/ -static const struct hc_driver ohci_pci_hc_driver = { - .description = hcd_name, - .product_desc = "OHCI Host Controller", - .hcd_priv_size = sizeof(struct ohci_hcd), - - /* - * generic hardware linkage - */ - .irq = ohci_irq, - .flags = HCD_MEMORY | HCD_USB11, - - /* - * basic lifecycle operations - */ - .reset = ohci_pci_reset, - .start = ohci_pci_start, - .stop = ohci_stop, - .shutdown = ohci_shutdown, - #ifdef CONFIG_PM - .pci_suspend = ohci_suspend, - .pci_resume = ohci_resume, +#define ohci_suspend NULL +#define ohci_resume NULL #endif - /* - * 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, +static struct hc_driver __read_mostly ohci_pci_hc_driver; - /* - * root hub support - */ - .hub_status_data = ohci_hub_status_data, - .hub_control = ohci_hub_control, -#ifdef CONFIG_PM - .bus_suspend = ohci_bus_suspend, - .bus_resume = ohci_bus_resume, -#endif - .start_port_reset = ohci_start_port_reset, +static const struct ohci_driver_overrides overrides = { + .product_desc = "OHCI PCI host controller", + .reset = ohci_pci_reset, + .start = ohci_pci_start, }; -/*-------------------------------------------------------------------------*/ - - static const struct pci_device_id pci_ids [] = { { /* handle any USB OHCI controller */ PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_USB_OHCI, ~0), @@ -377,3 +348,24 @@ static struct pci_driver ohci_pci_driver = { }, #endif }; + +static int __init ohci_pci_init(void) +{ + if (usb_disabled()) + return -ENODEV; + + pr_info("%s: " DRIVER_DESC "\n", hcd_name); + + ohci_init_driver(&ohci_pci_hc_driver, &overrides); + return pci_register_driver(&ohci_pci_driver); +} +module_init(ohci_pci_init); + +static void __exit ohci_pci_cleanup(void) +{ + pci_unregister_driver(&ohci_pci_driver); +} +module_exit(ohci_pci_cleanup); + +MODULE_DESCRIPTION(DRIVER_DESC); +MODULE_LICENSE("GPL"); diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h index d62ba8c..2f07705 100644 --- a/drivers/usb/host/ohci.h +++ b/drivers/usb/host/ohci.h @@ -734,6 +734,7 @@ extern int ohci_init (struct ohci_hcd *ohci); extern void ohci_stop (struct usb_hcd *hcd); extern void ohci_hcd_init (struct ohci_hcd *ohci); extern int ohci_run (struct ohci_hcd *ohci); +extern void sb800_prefetch(struct ohci_hcd *ohci, int on); #if defined(CONFIG_PM) || defined(CONFIG_PCI) extern int ohci_restart (struct ohci_hcd *ohci); #endif
This patch splits the PCI portion of ohci-hcd out into its own separate driver module, called ohci-pci. Consistently with the current practice, the decision whether to build this module is not user-configurable. If OHCI and PCI are enabled then the module will be built, always. Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Greg KH <greg@kroah.com> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: linux-usb@vger.kernel.org --- drivers/usb/host/Kconfig | 5 +++ drivers/usb/host/Makefile | 1 + drivers/usb/host/ohci-hcd.c | 21 ++-------- drivers/usb/host/ohci-mem.c | 2 +- drivers/usb/host/ohci-pci.c | 94 ++++++++++++++++++++----------------------- drivers/usb/host/ohci.h | 1 + 6 files changed, 54 insertions(+), 70 deletions(-)