Re: [patch v2.6.39 2/3] usb: musb: gadget: beautify usb_gadget_probe_driver()/usb_gadget_unregister_driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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<balbi@xxxxxx>
[...]

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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux