| Message ID | 1368163578-16827-2-git-send-email-manjunath.goudar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 10 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. > > ohci_setup() and ohci_start() routine for generic controller initialization > rather than each having its own idiosyncratic approach. Please add a verb to this sentence. > The major point of difficulty lies in ohci-pci's many vendor- and > device-specific workarounds. Some of them have to be applied before > calling ohci_setup() and ohci_start() some after, which necessitates > a fair amount of code motion. The other platform drivers require > much smaller changes. I get the impression that this paragraph describes patch 2/2, not this one. > In V2: > -ohci_hcd_init() ohci_run() and ohci_stop() are not made non-static. > -ohci_setup() and ohci_start() routine for generic controller initialization > rather than each having its own idiosyncratic approach. You shouldn't just copy the sentence above. This part of the description is supposed to tell how V2 differs from V1. In this case, the difference is that V2 adds the ohci_setup() and ohci_start() routines, so that's what you should say. > @@ -768,6 +763,33 @@ retry: > return 0; > } > > +/* ohci_setup routine for generic controller initialization */ > + > +static int ohci_setup(struct usb_hcd *hcd) > +{ > + struct ohci_hcd *ohci = hcd_to_ohci(hcd); > + int retval; > + > + /* data structure init */ > + retval = ohci_init(ohci); > + if (retval) > + return retval; > + return 0; How about simply doing: return ohci_init(ohci); That allows you to remove one declaration and three lines of executable code. > +} > + > +/* 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; You need to add a blank line here. Didn't I mention this before? > + ret = ohci_run(ohci); > + if (ret < 0) { > + ohci_err(ohci, "can't start\n"); > + ohci_stop(hcd); > + } > + return ret; > +} > + > /*-------------------------------------------------------------------------*/ > > /* an interrupt happens */ > @@ -949,13 +971,14 @@ 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; > > spin_lock_irq(&ohci->lock); > + ohci_init(ohci); I think this will cause problems. Better call ohci_init() _before_ doing the spin_lock_irq(). > ohci->rh_state = OHCI_RH_HALTED; > > /* Recycle any "live" eds/tds (and urbs). */ > @@ -1001,6 +1024,7 @@ static int ohci_restart (struct ohci_hcd *ohci) > ohci->ed_controltail = NULL; > ohci->ed_bulktail = NULL; > > + temp = ohci_run(ohci); > if ((temp = ohci_run (ohci)) < 0) { This change looks very wrong. You should be able to figure out why. Honestly, Manjunath, don't you even proofread your patches before mailing them out to other people? Alan Stern
On 10 May 2013 20:13, Alan Stern <stern@rowland.harvard.edu> wrote: > On Fri, 10 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. > > > > ohci_setup() and ohci_start() routine for generic controller > initialization > > rather than each having its own idiosyncratic approach. > > Please add a verb to this sentence. > > Added ohci_start() routine for generic controller initialization rather than each having its own idiosyncratic approach.This allow to clean duplicated code in most of SOC driver I have been removed ohci_setup() function,now directly calling ohci_init() function because all drivers modules are calling ohci_init() if I am keeping static otherwise other drivers are not building thats what ohci_init() routine need to be EXPORT. Is this Ok. > > The major point of difficulty lies in ohci-pci's many vendor- and > > device-specific workarounds. Some of them have to be applied before > > calling ohci_setup() and ohci_start() some after, which necessitates > > a fair amount of code motion. The other platform drivers require > > much smaller changes. > > I get the impression that this paragraph describes patch 2/2, not this > one. > > Above paragraph has been moved to the patch 2/2. > > In V2: > > -ohci_hcd_init() ohci_run() and ohci_stop() are not made non-static. > > -ohci_setup() and ohci_start() routine for generic controller > initialization > > rather than each having its own idiosyncratic approach. > > You shouldn't just copy the sentence above. This part of the > description is supposed to tell how V2 differs from V1. In this case, > the difference is that V2 adds the ohci_setup() and ohci_start() > routines, so that's what you should say. > > sure I will write clear description. > @@ -768,6 +763,33 @@ retry: > > return 0; > > } > > > > +/* ohci_setup routine for generic controller initialization */ > > + > > +static int ohci_setup(struct usb_hcd *hcd) > > +{ > > + struct ohci_hcd *ohci = hcd_to_ohci(hcd); > > + int retval; > > + > > + /* data structure init */ > > + retval = ohci_init(ohci); > > + if (retval) > > + return retval; > > + return 0; > > How about simply doing: > > return ohci_init(ohci); > > That allows you to remove one declaration and three lines of executable > code. > > ohci_setup() removed. > > +} > > + > > +/* 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; > > You need to add a blank line here. Didn't I mention this before? > > Sure I will do. > + ret = ohci_run(ohci); > > + if (ret < 0) { > > + ohci_err(ohci, "can't start\n"); > > + ohci_stop(hcd); > > + } > > + return ret; > > +} > > + > > > /*-------------------------------------------------------------------------*/ > > > > /* an interrupt happens */ > > @@ -949,13 +971,14 @@ 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; > > > > spin_lock_irq(&ohci->lock); > > + ohci_init(ohci); > > I think this will cause problems. Better call ohci_init() _before_ > doing the spin_lock_irq(). > > not requred to call ohci_init(ohci) in ohci_restart(). > > ohci->rh_state = OHCI_RH_HALTED; > > > > /* Recycle any "live" eds/tds (and urbs). */ > > @@ -1001,6 +1024,7 @@ static int ohci_restart (struct ohci_hcd *ohci) > > ohci->ed_controltail = NULL; > > ohci->ed_bulktail = NULL; > > > > + temp = ohci_run(ohci); > > if ((temp = ohci_run (ohci)) < 0) { > > This change looks very wrong. You should be able to figure out why. > > its wrong not required to call "temp = ohci_run(ohci)" here. > Honestly, Manjunath, don't you even proofread your patches before > mailing them out to other people? > > ya sure before sending I will verify once. Thank you for guiding me. > Alan Stern > > Manjunath Goudar
On Tue, 14 May 2013, Manjunath Goudar wrote: > Added ohci_start() routine for generic controller initialization > rather than each having its own idiosyncratic approach.This allow to clean > duplicated code in most of SOC driver > > I have been removed ohci_setup() function,now directly calling ohci_init() > function because all drivers > modules are calling ohci_init() if I am keeping static otherwise other > drivers are not building thats what ohci_init() > routine need to be EXPORT. > > Is this Ok. I can't tell without seeing the new patch. > > > ohci->rh_state = OHCI_RH_HALTED; > > > > > > /* Recycle any "live" eds/tds (and urbs). */ > > > @@ -1001,6 +1024,7 @@ static int ohci_restart (struct ohci_hcd *ohci) > > > ohci->ed_controltail = NULL; > > > ohci->ed_bulktail = NULL; > > > > > > + temp = ohci_run(ohci); > > > if ((temp = ohci_run (ohci)) < 0) { > > > > This change looks very wrong. You should be able to figure out why. > > > > > its wrong not required to call "temp = ohci_run(ohci)" here. The mistake is that the existing code _already_ does "temp = ohci_run(ohci)", but you added another line doing exactly the same thing. Alan Stern
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c index 9e6de95..7190920 100644 --- a/drivers/usb/host/ohci-hcd.c +++ b/drivers/usb/host/ohci-hcd.c @@ -81,11 +81,6 @@ static const char hcd_name [] = "ohci_hcd"; 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 +763,33 @@ retry: return 0; } +/* ohci_setup routine for generic controller initialization */ + +static int ohci_setup(struct usb_hcd *hcd) +{ + struct ohci_hcd *ohci = hcd_to_ohci(hcd); + int retval; + + /* data structure init */ + retval = ohci_init(ohci); + if (retval) + return retval; + return 0; +} + +/* 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,13 +971,14 @@ 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; spin_lock_irq(&ohci->lock); + ohci_init(ohci); ohci->rh_state = OHCI_RH_HALTED; /* Recycle any "live" eds/tds (and urbs). */ @@ -1001,6 +1024,7 @@ static int ohci_restart (struct ohci_hcd *ohci) ohci->ed_controltail = NULL; ohci->ed_bulktail = NULL; + temp = ohci_run(ohci); if ((temp = ohci_run (ohci)) < 0) { ohci_err (ohci, "can't restart, %d\n", temp); return temp; @@ -1008,12 +1032,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 +1056,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 +1106,73 @@ 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, + .bus_suspend = ohci_bus_suspend, + .bus_resume = ohci_bus_resume, + .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..892c5bc 100644 --- a/drivers/usb/host/ohci.h +++ b/drivers/usb/host/ohci.h @@ -718,3 +718,19 @@ 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); +#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
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. ohci_setup() and ohci_start() routine for generic controller initialization rather than each having its own idiosyncratic approach. The major point of difficulty lies in ohci-pci's many vendor- and device-specific workarounds. Some of them have to be applied before calling ohci_setup() and ohci_start() some after, which necessitates a fair amount of code motion. The other platform drivers require much smaller changes. In V2: -ohci_hcd_init() ohci_run() and ohci_stop() are not made non-static. -ohci_setup() and ohci_start() routine for generic controller initialization rather than each having its own idiosyncratic approach. 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. 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 | 105 +++++++++++++++++++++++++++++++++++++++---- drivers/usb/host/ohci-hub.c | 1 - drivers/usb/host/ohci.h | 16 +++++++ 3 files changed, 112 insertions(+), 10 deletions(-)