diff mbox

[RFC,V5,1/2] USB: OHCI: prepare to make ohci-hcd a library module

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

Commit Message

manjunath.goudar@linaro.org May 20, 2013, 12:14 p.m. UTC
This patch prepares ohci-hcd for being split up into a core
library and separate platform driver modules.  A generic
ohci_hc_driver structure is created, containing all the "standard"
values, and a new mechanism is added whereby a driver module can
specify a set of overrides to those values.  In addition the
ohci_restart(),ohci_suspend() and ohci_resume() routines need
to be EXPORTed for use by the drivers.

Added ohci_setip(() and ohci_start() routine for to start the generic
controller rather than each having its own idiosyncratic approach.
This allow to clean duplicated code in most of SOC driver

In V2:
 -ohci_hcd_init() ohci_run() and ohci_stop() are not made non-static.
 -Adds the ohci_setup() and ohci_start() routine.

In V3:
 -purpose of ohci_setup() and ohci_start() function description written in the patch
  description.
 -ohci_init() are not made non-static but now called beginning of the ohci_restart().
 -ohci_run() signature change reverted back.
 -unrelated changes removed.
 -duplicate comment line removed.
 -inline ohci_suspend() and ohci_resume() is not needed so removed from ohci.h file.

In V4:
 -ohci-init() EXPORTed because it is called by all bus glue modules.
 -ohci-setup() removed from 1/2 added into 2/2 patch.

In V5:
 -Again ohci_setup() is added and EXPORTed because to replace the ohci_init() from
  all bus glues.
 -ohci_init() is not made non-static function.

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/ohci-hcd.c |  104 ++++++++++++++++++++++++++++++++++++++-----
 drivers/usb/host/ohci-hub.c |    1 -
 drivers/usb/host/ohci.h     |   17 +++++++
 3 files changed, 111 insertions(+), 11 deletions(-)

Comments

Alan Stern May 20, 2013, 5:53 p.m. UTC | #1
On Mon, 20 May 2013, Manjunath Goudar wrote:

> This patch prepares ohci-hcd for being split up into a core
> library and separate platform driver modules.  A generic
> ohci_hc_driver structure is created, containing all the "standard"
> values, and a new mechanism is added whereby a driver module can
> specify a set of overrides to those values.  In addition the
> ohci_restart(),ohci_suspend() and ohci_resume() routines need
> to be EXPORTed for use by the drivers.
> 
> Added ohci_setip(() and ohci_start() routine for to start the generic
> controller rather than each having its own idiosyncratic approach.
> This allow to clean duplicated code in most of SOC driver
> 
> In V2:
>  -ohci_hcd_init() ohci_run() and ohci_stop() are not made non-static.
>  -Adds the ohci_setup() and ohci_start() routine.
> 
> In V3:
>  -purpose of ohci_setup() and ohci_start() function description written in the patch
>   description.
>  -ohci_init() are not made non-static but now called beginning of the ohci_restart().
>  -ohci_run() signature change reverted back.
>  -unrelated changes removed.
>  -duplicate comment line removed.
>  -inline ohci_suspend() and ohci_resume() is not needed so removed from ohci.h file.
> 
> In V4:
>  -ohci-init() EXPORTed because it is called by all bus glue modules.
>  -ohci-setup() removed from 1/2 added into 2/2 patch.
> 
> In V5:
>  -Again ohci_setup() is added and EXPORTed because to replace the ohci_init() from
>   all bus glues.
>  -ohci_init() is not made non-static function.

This version of the patch is okay except for one problem...

> @@ -949,12 +967,13 @@ static void ohci_stop (struct usb_hcd *hcd)
>  #if defined(CONFIG_PM) || defined(CONFIG_PCI)
>  
>  /* must not be called from interrupt context */
> -static int ohci_restart (struct ohci_hcd *ohci)
> +int ohci_restart(struct ohci_hcd *ohci)
>  {
>  	int temp;
>  	int i;
>  	struct urb_priv *priv;
>  
> +	ohci_init(ohci);
>  	spin_lock_irq(&ohci->lock);
>  	ohci->rh_state = OHCI_RH_HALTED;
>  

Having made this change, it is important that the call to ohci_init() 
in ohci-pci.c:ohci_quirk_nec_worker() be removed.  Otherwise that 
routine will end up calling ohci_init() twice: once directly and once 
inside ohci_restart().

And it's important that this change be done in _this_ patch, not in a 
later patch.  We want everything to work correctly even when people 
have applied the 1/2 patch without the 2/2 patch.

Alan Stern
manjunath.goudar@linaro.org May 21, 2013, 6:40 a.m. UTC | #2
On 20 May 2013 23:23, Alan Stern <stern@rowland.harvard.edu> wrote:

> On Mon, 20 May 2013, Manjunath Goudar wrote:
>
> > This patch prepares ohci-hcd for being split up into a core
> > library and separate platform driver modules.  A generic
> > ohci_hc_driver structure is created, containing all the "standard"
> > values, and a new mechanism is added whereby a driver module can
> > specify a set of overrides to those values.  In addition the
> > ohci_restart(),ohci_suspend() and ohci_resume() routines need
> > to be EXPORTed for use by the drivers.
> >
> > Added ohci_setip(() and ohci_start() routine for to start the generic
> > controller rather than each having its own idiosyncratic approach.
> > This allow to clean duplicated code in most of SOC driver
> >
> > In V2:
> >  -ohci_hcd_init() ohci_run() and ohci_stop() are not made non-static.
> >  -Adds the ohci_setup() and ohci_start() routine.
> >
> > In V3:
> >  -purpose of ohci_setup() and ohci_start() function description written
> in the patch
> >   description.
> >  -ohci_init() are not made non-static but now called beginning of the
> ohci_restart().
> >  -ohci_run() signature change reverted back.
> >  -unrelated changes removed.
> >  -duplicate comment line removed.
> >  -inline ohci_suspend() and ohci_resume() is not needed so removed from
> ohci.h file.
> >
> > In V4:
> >  -ohci-init() EXPORTed because it is called by all bus glue modules.
> >  -ohci-setup() removed from 1/2 added into 2/2 patch.
> >
> > In V5:
> >  -Again ohci_setup() is added and EXPORTed because to replace the
> ohci_init() from
> >   all bus glues.
> >  -ohci_init() is not made non-static function.
>
> This version of the patch is okay except for one problem...
>
> > @@ -949,12 +967,13 @@ static void ohci_stop (struct usb_hcd *hcd)
> >  #if defined(CONFIG_PM) || defined(CONFIG_PCI)
> >
> >  /* must not be called from interrupt context */
> > -static int ohci_restart (struct ohci_hcd *ohci)
> > +int ohci_restart(struct ohci_hcd *ohci)
> >  {
> >       int temp;
> >       int i;
> >       struct urb_priv *priv;
> >
> > +     ohci_init(ohci);
> >       spin_lock_irq(&ohci->lock);
> >       ohci->rh_state = OHCI_RH_HALTED;
> >
>
> Having made this change, it is important that the call to ohci_init()
> in ohci-pci.c:ohci_quirk_nec_worker() be removed.  Otherwise that
> routine will end up calling ohci_init() twice: once directly and once
> inside ohci_restart().
>
>
yes I accepted your suggestion.I am planing to remove ohci_init() from
ohci_quirk_nec_worker() function,then routine end up with only once not
twice.

Is my approach is correct?



> And it's important that this change be done in _this_ patch, not in a
> later patch.  We want everything to work correctly even when people
> have applied the 1/2 patch without the 2/2 patch.
>
> Alan Stern
>
>
Yes I agree.

Thanks
Manjunath Goudar
diff mbox

Patch

diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 9e6de95..f55c077 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -79,13 +79,7 @@  static const char	hcd_name [] = "ohci_hcd";
 #include "pci-quirks.h"
 
 static void ohci_dump (struct ohci_hcd *ohci, int verbose);
-static int ohci_init (struct ohci_hcd *ohci);
 static void ohci_stop (struct usb_hcd *hcd);
-
-#if defined(CONFIG_PM) || defined(CONFIG_PCI)
-static int ohci_restart (struct ohci_hcd *ohci);
-#endif
-
 #ifdef CONFIG_PCI
 static void sb800_prefetch(struct ohci_hcd *ohci, int on);
 #else
@@ -768,6 +762,30 @@  retry:
 	return 0;
 }
 
+/* ohci_setup routine for generic controller initialization */
+
+int ohci_setup(struct usb_hcd *hcd)
+{
+	struct ohci_hcd		*ohci = hcd_to_ohci(hcd);
+
+	return ohci_init(ohci);
+}
+EXPORT_SYMBOL_GPL(ohci_setup);
+
+/* ohci_start routine for generic controller start of all OHCI bus glue */
+static int ohci_start(struct usb_hcd *hcd)
+{
+	struct ohci_hcd		*ohci = hcd_to_ohci(hcd);
+	int	ret;
+
+	ret = ohci_run(ohci);
+	if (ret < 0) {
+		ohci_err(ohci, "can't start\n");
+		ohci_stop(hcd);
+	}
+	return ret;
+}
+
 /*-------------------------------------------------------------------------*/
 
 /* an interrupt happens */
@@ -949,12 +967,13 @@  static void ohci_stop (struct usb_hcd *hcd)
 #if defined(CONFIG_PM) || defined(CONFIG_PCI)
 
 /* must not be called from interrupt context */
-static int ohci_restart (struct ohci_hcd *ohci)
+int ohci_restart(struct ohci_hcd *ohci)
 {
 	int temp;
 	int i;
 	struct urb_priv *priv;
 
+	ohci_init(ohci);
 	spin_lock_irq(&ohci->lock);
 	ohci->rh_state = OHCI_RH_HALTED;
 
@@ -1008,12 +1027,13 @@  static int ohci_restart (struct ohci_hcd *ohci)
 	ohci_dbg(ohci, "restart complete\n");
 	return 0;
 }
+EXPORT_SYMBOL_GPL(ohci_restart);
 
 #endif
 
 #ifdef CONFIG_PM
 
-static int __maybe_unused ohci_suspend(struct usb_hcd *hcd, bool do_wakeup)
+int ohci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 {
 	struct ohci_hcd	*ohci = hcd_to_ohci (hcd);
 	unsigned long	flags;
@@ -1031,9 +1051,9 @@  static int __maybe_unused ohci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(ohci_suspend);
 
-
-static int __maybe_unused ohci_resume(struct usb_hcd *hcd, bool hibernated)
+int ohci_resume(struct usb_hcd *hcd, bool hibernated)
 {
 	struct ohci_hcd		*ohci = hcd_to_ohci(hcd);
 	int			port;
@@ -1081,11 +1101,75 @@  static int __maybe_unused ohci_resume(struct usb_hcd *hcd, bool hibernated)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(ohci_resume);
 
 #endif
 
 /*-------------------------------------------------------------------------*/
 
+/*
+ * Generic structure: This gets copied for platform drivers so that
+ * individual entries can be overridden as needed.
+ */
+
+static const struct hc_driver ohci_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_setup,
+	.start =                ohci_start,
+	.stop =                 ohci_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_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,
+};
+
+void ohci_init_driver(struct hc_driver *drv,
+		const struct ohci_driver_overrides *over)
+{
+	/* Copy the generic table to drv and then apply the overrides */
+	*drv = ohci_hc_driver;
+
+	drv->product_desc = over->product_desc;
+	drv->hcd_priv_size += over->extra_priv_size;
+	if (over->reset)
+		drv->reset = over->reset;
+}
+EXPORT_SYMBOL_GPL(ohci_init_driver);
+
+/*-------------------------------------------------------------------------*/
+
 MODULE_AUTHOR (DRIVER_AUTHOR);
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE ("GPL");
diff --git a/drivers/usb/host/ohci-hub.c b/drivers/usb/host/ohci-hub.c
index db09dae..2ff8e6c 100644
--- a/drivers/usb/host/ohci-hub.c
+++ b/drivers/usb/host/ohci-hub.c
@@ -176,7 +176,6 @@  __acquires(ohci->lock)
 	if (status == -EBUSY) {
 		if (!autostopped) {
 			spin_unlock_irq (&ohci->lock);
-			(void) ohci_init (ohci);
 			status = ohci_restart (ohci);
 
 			usb_root_hub_lost_power(hcd->self.root_hub);
diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h
index d329914..3b58482 100644
--- a/drivers/usb/host/ohci.h
+++ b/drivers/usb/host/ohci.h
@@ -718,3 +718,20 @@  static inline u32 roothub_status (struct ohci_hcd *hc)
 	{ return ohci_readl (hc, &hc->regs->roothub.status); }
 static inline u32 roothub_portstatus (struct ohci_hcd *hc, int i)
 	{ return read_roothub (hc, portstatus [i], 0xffe0fce0); }
+
+/* Declarations of things exported for use by ohci platform drivers */
+
+struct ohci_driver_overrides {
+	const char	*product_desc;
+	size_t		extra_priv_size;
+	int		(*reset)(struct usb_hcd *hcd);
+};
+
+extern void	ohci_init_driver(struct hc_driver *drv,
+				const struct ohci_driver_overrides *over);
+extern int	ohci_restart(struct ohci_hcd *ohci);
+extern int	ohci_setup(struct usb_hcd *hcd);
+#ifdef CONFIG_PM
+extern int	ohci_suspend(struct usb_hcd *hcd, bool do_wakeup);
+extern int	ohci_resume(struct usb_hcd *hcd, bool hibernated);
+#endif