Hello.
On 17-02-2011 14:30, Felipe Balbi wrote:
Just a few cosmetic fixes to usb_gadget_probe_driver()
and usb_gadget_unregister_driver().
Decreased a few indentation levels with goto statements.
Signed-off-by: Felipe Balbi<[email protected]>
[...]
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 2fe3046..86decba 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -1801,90 +1801,95 @@ void musb_gadget_cleanup(struct musb *musb)
int usb_gadget_probe_driver(struct usb_gadget_driver *driver,
int (*bind)(struct usb_gadget *))
{
- int retval;
- unsigned long flags;
- struct musb *musb = the_gadget;
+ struct musb *musb = the_gadget;
+ unsigned long flags;
+ int retval = -EINVAL;
if (!driver
|| driver->speed != USB_SPEED_HIGH
|| !bind || !driver->setup)
- return -EINVAL;
+ goto err0;
Not sure what this gains...
/* driver must be initialized to support peripheral mode */
if (!musb) {
DBG(1, "%s, no dev??\n", __func__);
- return -ENODEV;
+ retval = -ENODEV;
+ goto err0;
This too...
}
DBG(3, "registering driver %s\n", driver->function);
- spin_lock_irqsave(&musb->lock, flags);
Is it really correct to remove spinlocm protection here?
if (musb->gadget_driver) {
DBG(1, "%s is already bound to %s\n",
musb_driver_name,
musb->gadget_driver->driver.name);
retval = -EBUSY;
- } else {
- musb->gadget_driver = driver;
- musb->g.dev.driver =&driver->driver;
- driver->driver.bus = NULL;
- musb->softconnect = 1;
- retval = 0;
+ goto err0;
}
+ spin_lock_irqsave(&musb->lock, flags);
+ musb->gadget_driver = driver;
+ musb->g.dev.driver =&driver->driver;
+ driver->driver.bus = NULL;
+ musb->softconnect = 1;
spin_unlock_irqrestore(&musb->lock, flags);
- if (retval == 0) {
Yeah, I have noticed this too and wanted to change it -- but forgot... :-)
[...]
- if (is_otg_enabled(musb)) {
- struct usb_hcd *hcd = musb_to_hcd(musb);
+ if (is_otg_enabled(musb)) {
+ struct usb_hcd *hcd = musb_to_hcd(musb);
- DBG(3, "OTG startup...\n");
+ DBG(3, "OTG startup...\n");
- /* REVISIT: funcall to other code, which also
- * handles power budgeting ... this way also
- * ensures HdrcStart is indirectly called.
- */
- retval = usb_add_hcd(musb_to_hcd(musb), -1, 0);
- if (retval< 0) {
- DBG(1, "add_hcd failed, %d\n", retval);
- spin_lock_irqsave(&musb->lock, flags);
- otg_set_peripheral(musb->xceiv, NULL);
You're removing this call -- is it intentional?
- musb->gadget_driver = NULL;
- musb->g.dev.driver = NULL;
- spin_unlock_irqrestore(&musb->lock, flags);
- } else {
- hcd->self.uses_pio_for_control = 1;
- }
+ /* REVISIT: funcall to other code, which also
+ * handles power budgeting ... this way also
+ * ensures HdrcStart is indirectly called.
+ */
+ retval = usb_add_hcd(musb_to_hcd(musb), -1, 0);
+ if (retval < 0) {
+ DBG(1, "add_hcd failed, %d\n", retval);
+ goto err2;
}
+
+ hcd->self.uses_pio_for_control = 1;
}
+ return 0;
+
+err2:
+ if (!is_otg_enabled(musb))
+ musb_stop(musb);
Hm, this wasn't called before -- you don't describe why it's needed.
+
+err1:
+ musb->gadget_driver = NULL;
+ musb->g.dev.driver = NULL;
+
+err0:
return retval;
}
EXPORT_SYMBOL(usb_gadget_probe_driver);
@@ -1939,14 +1944,17 @@ static void stop_activity(struct musb *musb, struct
usb_gadget_driver *driver)
*/
int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
{
- unsigned long flags;
- int retval = 0;
struct musb *musb = the_gadget;
+ unsigned long flags;
if (!driver || !driver->unbind || !musb)
return -EINVAL;
- /* REVISIT always use otg_set_peripheral() here too;
+ if (!musb->gadget_driver)
+ return -EINVAL;
+
+ /*
+ * REVISIT always use otg_set_peripheral() here too;
* this needs to shut down the OTG engine.
*/
@@ -1956,29 +1964,26 @@ int usb_gadget_unregister_driver(struct
usb_gadget_driver *driver)
musb_hnp_stop(musb);
#endif
- if (musb->gadget_driver == driver) {
+ (void) musb_gadget_vbus_draw(&musb->g, 0);
- (void) musb_gadget_vbus_draw(&musb->g, 0);
+ musb->xceiv->state = OTG_STATE_UNDEFINED;
+ stop_activity(musb, driver);
+ otg_set_peripheral(musb->xceiv, NULL);
- musb->xceiv->state = OTG_STATE_UNDEFINED;
- stop_activity(musb, driver);
- otg_set_peripheral(musb->xceiv, NULL);
+ DBG(3, "unregistering driver %s\n", driver->function);
- DBG(3, "unregistering driver %s\n", driver->function);
- spin_unlock_irqrestore(&musb->lock, flags);
- driver->unbind(&musb->g);
- spin_lock_irqsave(&musb->lock, flags);
+ spin_unlock_irqrestore(&musb->lock, flags);
+ driver->unbind(&musb->g);
+ spin_lock_irqsave(&musb->lock, flags);
- musb->gadget_driver = NULL;
- musb->g.dev.driver = NULL;
+ musb->gadget_driver = NULL;
+ musb->g.dev.driver = NULL;
- musb->is_active = 0;
- musb_platform_try_idle(musb, 0);
- } else
- retval = -EINVAL;
+ musb->is_active = 0;
+ musb_platform_try_idle(musb, 0);
spin_unlock_irqrestore(&musb->lock, flags);
- if (is_otg_enabled(musb)&& retval == 0) {
+ if (is_otg_enabled(musb)) {
usb_remove_hcd(musb_to_hcd(musb));
/* FIXME we need to be able to register another
* gadget driver here and have everything work;
@@ -1986,7 +1991,10 @@ int usb_gadget_unregister_driver(struct
usb_gadget_driver *driver)
*/
}
- return retval;
+ if (!is_otg_enabled(musb))
+ musb_stop(musb);
Wasn't called before either -- you don't describe why it's needed...
+
+ return 0;
}
EXPORT_SYMBOL(usb_gadget_unregister_driver);
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
