Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1419384 > unrolled thread

[PATCH v10 13/14] usb: gadget: udc: adapt to OTG core

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v10 13/14] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-06-10 15:10 +0200
    Re: [PATCH v10 13/14] usb: gadget: udc: adapt to OTG core Peter Chen <hzpeterchen@gmail.com> - 2016-06-12 13:50 +0200
      Re: [PATCH v10 13/14] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-06-13 09:20 +0200
        Re: [PATCH v10 13/14] usb: gadget: udc: adapt to OTG core Peter Chen <hzpeterchen@gmail.com> - 2016-06-13 09:30 +0200
          Re: [PATCH v10 13/14] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-06-13 09:40 +0200
            Re: [PATCH v10 13/14] usb: gadget: udc: adapt to OTG core Peter Chen <hzpeterchen@gmail.com> - 2016-06-13 09:50 +0200
    [PATCH v11 13/14] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-06-13 10:00 +0200
      Re: [PATCH v11 13/14] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-06-13 10:10 +0200
      Re: [PATCH v11 13/14] usb: gadget: udc: adapt to OTG core Peter Chen <hzpeterchen@gmail.com> - 2016-06-13 10:10 +0200

#1419384 — [PATCH v10 13/14] usb: gadget: udc: adapt to OTG core

The OTG state machine needs a mechanism to start and
stop the gadget controller as well as connect/disconnect
from the bus. Add usb_gadget_start(), usb_gadget_stop()
and usb_gadget_connect_control().

Introduce usb_otg_add_gadget_udc() to allow controller drivers
to register a gadget controller that is part of an OTG instance.

Register with OTG core when UDC is added in usb_add_gadget_udc_release()
and unregister on usb_del_gadget_udc().

Notify the OTG core when gadget function driver is available on
udc_bind_to_driver() and when it is removed in usb_gadget_remove_driver().

We need to unlock the usb_lock mutex before calling
usb_otg_register_gadget() else it will cause a circular
locking dependency.

Ignore softconnect sysfs control when we're in OTG
mode as OTG FSM should care of gadget softconnect using
the b_bus_req mechanism.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/gadget/udc/core.c | 202 +++++++++++++++++++++++++++++++++++++++---
 include/linux/usb/gadget.h    |   4 +
 2 files changed, 196 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 42756d7..9d8a1d0 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -28,6 +28,11 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb.h>
+#include <linux/usb/otg.h>
+#include <linux/usb/of.h>
+
+#include <linux/of.h>
+#include <linux/of_platform.h>
 
 #include "trace.h"
 
@@ -1060,6 +1065,113 @@ static inline void usb_gadget_udc_stop(struct usb_udc *udc)
 }
 
 /**
+ * usb_gadget_to_udc - get the UDC owning the gadget
+ *
+ * udc_lock must be held.
+ * Returs NULL if UDC is not found.
+ */
+static struct usb_udc *usb_gadget_to_udc(struct usb_gadget *gadget)
+{
+	struct usb_udc *udc;
+
+	list_for_each_entry(udc, &udc_list, list)
+		if (udc->gadget == gadget)
+			return udc;
+
+	return NULL;
+}
+
+/**
+ * usb_gadget_start - start the usb gadget controller
+ * @gadget: the gadget device to start
+ *
+ * This is external API for use by OTG core.
+ *
+ * Start the usb device controller. Does not connect to the bus.
+ */
+static int usb_gadget_start(struct usb_gadget *gadget)
+{
+	int ret;
+	struct usb_udc *udc;
+
+	mutex_lock(&udc_lock);
+	udc = usb_gadget_to_udc(gadget);
+	if (!udc) {
+		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+			__func__);
+		mutex_unlock(&udc_lock);
+		return -EINVAL;
+	}
+
+	ret = usb_gadget_udc_start(udc);
+	if (ret)
+		dev_err(&udc->dev, "USB Device Controller didn't start: %d\n",
+			ret);
+
+	mutex_unlock(&udc_lock);
+
+	return ret;
+}
+
+/**
+ * usb_gadget_stop - stop the usb gadget controller
+ * @gadget: the gadget device we want to stop
+ *
+ * This is external API for use by OTG core.
+ *
+ * Stop the gadget controller. Does not disconnect from the bus.
+ * Caller must ensure that gadget has disconnected from the bus
+ * before calling usb_gadget_stop().
+ */
+static int usb_gadget_stop(struct usb_gadget *gadget)
+{
+	struct usb_udc *udc;
+
+	mutex_lock(&udc_lock);
+	udc = usb_gadget_to_udc(gadget);
+	if (!udc) {
+		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+			__func__);
+		mutex_unlock(&udc_lock);
+		return -EINVAL;
+	}
+
+	if (gadget->connected)
+		dev_dbg(gadget->dev.parent,
+			"%s: called while still connected\n", __func__);
+
+	usb_gadget_udc_stop(udc);
+	mutex_unlock(&udc_lock);
+
+	return 0;
+}
+
+static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect)
+{
+	struct usb_udc *udc;
+
+	mutex_lock(&udc_lock);
+	udc = usb_gadget_to_udc(gadget);
+	if (!udc) {
+		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+			__func__);
+		mutex_unlock(&udc_lock);
+		return -EINVAL;
+	}
+
+	if (connect) {
+		usb_gadget_connect(udc->gadget);
+	} else {
+		usb_gadget_disconnect(udc->gadget);
+		udc->driver->disconnect(udc->gadget);
+	}
+
+	mutex_unlock(&udc_lock);
+
+	return 0;
+}
+
+/**
  * usb_udc_release - release the usb_udc struct
  * @dev: the dev member within usb_udc
  *
@@ -1082,6 +1194,12 @@ static void usb_udc_nop_release(struct device *dev)
 	dev_vdbg(dev, "%s\n", __func__);
 }
 
+struct otg_gadget_ops otg_gadget_intf = {
+	.start = usb_gadget_start,
+	.stop = usb_gadget_stop,
+	.connect_control = usb_gadget_connect_control,
+};
+
 /**
  * usb_add_gadget_udc_release - adds a new gadget to the udc class driver list
  * @parent: the parent device to this udc. Usually the controller driver's
@@ -1137,6 +1255,14 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 	usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
 	udc->vbus = true;
 
+	if (gadget->otg_dev) {
+		mutex_unlock(&udc_lock);
+		ret = usb_otg_register_gadget(gadget, &otg_gadget_intf);
+		mutex_lock(&udc_lock);
+		if (ret)
+			goto err5;
+	}
+
 	/* pick up one of pending gadget drivers */
 	list_for_each_entry(driver, &gadget_driver_pending_list, pending) {
 		if (!driver->udc_name || strcmp(driver->udc_name,
@@ -1145,7 +1271,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 			if (ret != -EPROBE_DEFER)
 				list_del(&driver->pending);
 			if (ret)
-				goto err4;
+				goto err5;
 			break;
 		}
 	}
@@ -1154,6 +1280,8 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 
 	return 0;
 
+err5:
+	device_del(&udc->dev);
 err4:
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
@@ -1215,6 +1343,33 @@ int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget)
 }
 EXPORT_SYMBOL_GPL(usb_add_gadget_udc);
 
+/**
+ * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list
+ * @parent: the parent device to this udc. Usually the controller
+ * driver's device.
+ * @gadget: the gadget to be added to the list
+ * @otg_dev: the OTG controller device
+ *
+ * If otg_dev is NULL then device tree node is checked
+ * for OTG controller via the otg-controller property.
+ * Returns zero on success, negative errno otherwise.
+ */
+int usb_otg_add_gadget_udc(struct device *parent, struct usb_gadget *gadget,
+			   struct device *otg_dev)
+{
+	if (!otg_dev) {
+		gadget->otg_dev = of_usb_get_otg(parent->of_node);
+		if (!gadget->otg_dev)
+			return -ENODEV;
+	} else {
+		gadget->otg_dev = otg_dev;
+	}
+
+	return usb_add_gadget_udc_release(parent, gadget, NULL);
+}
+EXPORT_SYMBOL_GPL(usb_otg_add_gadget_udc);
+
+/* udc_lock must be held */
 static void usb_gadget_remove_driver(struct usb_udc *udc)
 {
 	dev_dbg(&udc->dev, "unregistering UDC driver [%s]\n",
@@ -1222,10 +1377,18 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 
-	usb_gadget_disconnect(udc->gadget);
-	udc->driver->disconnect(udc->gadget);
+	/* If OTG/dual-role, the otg core manages UDC start/stop */
+	if (udc->gadget->otg_dev) {
+		mutex_unlock(&udc_lock);
+		usb_otg_gadget_ready(udc->gadget, false);
+		mutex_lock(&udc_lock);
+	} else {
+		usb_gadget_disconnect(udc->gadget);
+		udc->driver->disconnect(udc->gadget);
+		usb_gadget_udc_stop(udc);
+	}
+
 	udc->driver->unbind(udc->gadget);
-	usb_gadget_udc_stop(udc);
 
 	udc->driver = NULL;
 	udc->dev.driver = NULL;
@@ -1259,6 +1422,9 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
 	}
 	mutex_unlock(&udc_lock);
 
+	if (gadget->otg_dev)
+		usb_otg_unregister_gadget(gadget);
+
 	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
 	flush_work(&gadget->work);
 	device_unregister(&udc->dev);
@@ -1268,6 +1434,7 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
 
 /* ------------------------------------------------------------------------- */
 
+/* udc_lock must be held */
 static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
 {
 	int ret;
@@ -1282,17 +1449,26 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
 	ret = driver->bind(udc->gadget, driver);
 	if (ret)
 		goto err1;
-	ret = usb_gadget_udc_start(udc);
-	if (ret) {
-		driver->unbind(udc->gadget);
-		goto err1;
+
+	/* If OTG/dual-role, the otg core manages UDC start/stop */
+	if (udc->gadget->otg_dev) {
+		mutex_unlock(&udc_lock);
+		usb_otg_gadget_ready(udc->gadget, true);
+		mutex_lock(&udc_lock);
+	} else {
+		ret = usb_gadget_udc_start(udc);
+		if (ret) {
+			mutex_unlock(&udc_lock);
+			driver->unbind(udc->gadget);
+			goto err1;
+		}
+		usb_udc_connect_control(udc);
 	}
-	usb_udc_connect_control(udc);
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 	return 0;
 err1:
-	if (ret != -EISNAM)
+	if ((ret != -EISNAM) && (ret != -EPROBE_DEFER))
 		dev_err(&udc->dev, "failed to start %s: %d\n",
 			udc->driver->function, ret);
 	udc->driver = NULL;
@@ -1389,6 +1565,12 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
 		return -EOPNOTSUPP;
 	}
 
+	/* In OTG/dual-role mode, soft-connect should be handled by OTG core */
+	if (udc->gadget->otg_dev) {
+		dev_err(dev, "soft-connect not supported in OTG mode\n");
+		return -EOPNOTSUPP;
+	}
+
 	if (sysfs_streq(buf, "connect")) {
 		usb_gadget_udc_start(udc);
 		usb_gadget_connect(udc->gadget);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 1d74fb8..8c6880d 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -701,6 +701,10 @@ extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
 extern void usb_del_gadget_udc(struct usb_gadget *gadget);
 extern char *usb_get_gadget_udc_name(void);
 
+extern int usb_otg_add_gadget_udc(struct device *parent,
+				  struct usb_gadget *gadget,
+				  struct device *otg_dev);
+
 /*-------------------------------------------------------------------------*/
 
 /* utility to simplify dealing with string descriptors */
-- 
2.7.4

[toc] | [next] | [standalone]

#1420246

On Fri, Jun 10, 2016 at 04:07:22PM +0300, Roger Quadros wrote:
>  
> +/**
> + * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list
> + * @parent: the parent device to this udc. Usually the controller
> + * driver's device.

It seems it should be udc device

> +/* udc_lock must be held */
>  static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
>  {
>  	int ret;
> @@ -1282,17 +1449,26 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
>  	ret = driver->bind(udc->gadget, driver);
>  	if (ret)
>  		goto err1;
> -	ret = usb_gadget_udc_start(udc);
> -	if (ret) {
> -		driver->unbind(udc->gadget);
> -		goto err1;
> +
> +	/* If OTG/dual-role, the otg core manages UDC start/stop */
> +	if (udc->gadget->otg_dev) {
> +		mutex_unlock(&udc_lock);
> +		usb_otg_gadget_ready(udc->gadget, true);
> +		mutex_lock(&udc_lock);
> +	} else {
> +		ret = usb_gadget_udc_start(udc);
> +		if (ret) {
> +			mutex_unlock(&udc_lock);
> +			driver->unbind(udc->gadget);
> +			goto err1;
> +		}
> +		usb_udc_connect_control(udc);
>  	}
> -	usb_udc_connect_control(udc);
>  
>  	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>  	return 0;
>  err1:
> -	if (ret != -EISNAM)
> +	if ((ret != -EISNAM) && (ret != -EPROBE_DEFER))

It seems it will not introduce -EPROBE_DEFER with your changes
in udc_bind_to_driver

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1420504

On 12/06/16 14:36, Peter Chen wrote:
> On Fri, Jun 10, 2016 at 04:07:22PM +0300, Roger Quadros wrote:
>>  
>> +/**
>> + * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list
>> + * @parent: the parent device to this udc. Usually the controller
>> + * driver's device.
> 
> It seems it should be udc device

Parent and udc->dev are not the same right?

I guess i'll omit the second statement to avoid confusion. So.

@parent: the parent device to this udc.

> 
>> +/* udc_lock must be held */
>>  static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
>>  {
>>  	int ret;
>> @@ -1282,17 +1449,26 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
>>  	ret = driver->bind(udc->gadget, driver);
>>  	if (ret)
>>  		goto err1;
>> -	ret = usb_gadget_udc_start(udc);
>> -	if (ret) {
>> -		driver->unbind(udc->gadget);
>> -		goto err1;
>> +
>> +	/* If OTG/dual-role, the otg core manages UDC start/stop */
>> +	if (udc->gadget->otg_dev) {
>> +		mutex_unlock(&udc_lock);
>> +		usb_otg_gadget_ready(udc->gadget, true);
>> +		mutex_lock(&udc_lock);
>> +	} else {
>> +		ret = usb_gadget_udc_start(udc);
>> +		if (ret) {
>> +			mutex_unlock(&udc_lock);
>> +			driver->unbind(udc->gadget);
>> +			goto err1;
>> +		}
>> +		usb_udc_connect_control(udc);
>>  	}
>> -	usb_udc_connect_control(udc);
>>  
>>  	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>>  	return 0;
>>  err1:
>> -	if (ret != -EISNAM)
>> +	if ((ret != -EISNAM) && (ret != -EPROBE_DEFER))
> 
> It seems it will not introduce -EPROBE_DEFER with your changes
> in udc_bind_to_driver
> 

Good catch. Left over bit when I moved defer probing out of here.

--
cheers,
-roger

[toc] | [prev] | [next] | [standalone]

#1420517

On Mon, Jun 13, 2016 at 10:14:31AM +0300, Roger Quadros wrote:
> On 12/06/16 14:36, Peter Chen wrote:
> > On Fri, Jun 10, 2016 at 04:07:22PM +0300, Roger Quadros wrote:
> >>  
> >> +/**
> >> + * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list
> >> + * @parent: the parent device to this udc. Usually the controller
> >> + * driver's device.
> > 
> > It seems it should be udc device
> 
> Parent and udc->dev are not the same right?

Sure, udc's parent is otg device.

> 
> I guess i'll omit the second statement to avoid confusion. So.
> 
> @parent: the parent device to this udc.

Where you call below APIs? It seems to be a udc driver, right?
So, when you try to get "otg-controller" from the node, this node
should be udc.

/**
 * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list
 * @parent: the parent device to this udc. Usually the controller
 * driver's device.
 * @gadget: the gadget to be added to the list
 * @otg_dev: the OTG controller device
 *
 * If otg_dev is NULL then device tree node is checked
 * for OTG controller via the otg-controller property.
 * Returns zero on success, negative errno otherwise.
 */
int usb_otg_add_gadget_udc(struct device *parent, struct usb_gadget *gadget,
			   struct device *otg_dev)
{
	if (!otg_dev) {
		gadget->otg_dev = of_usb_get_otg(parent->of_node);
		if (!gadget->otg_dev)
			return -ENODEV;
	} else {
		gadget->otg_dev = otg_dev;
	}

	return usb_add_gadget_udc_release(parent, gadget, NULL);
}
EXPORT_SYMBOL_GPL(usb_otg_add_gadget_udc);
-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1420523

On 13/06/16 10:20, Peter Chen wrote:
> On Mon, Jun 13, 2016 at 10:14:31AM +0300, Roger Quadros wrote:
>> On 12/06/16 14:36, Peter Chen wrote:
>>> On Fri, Jun 10, 2016 at 04:07:22PM +0300, Roger Quadros wrote:
>>>>  
>>>> +/**
>>>> + * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list
>>>> + * @parent: the parent device to this udc. Usually the controller
>>>> + * driver's device.
>>>
>>> It seems it should be udc device
>>
>> Parent and udc->dev are not the same right?
> 
> Sure, udc's parent is otg device.
> 
>>
>> I guess i'll omit the second statement to avoid confusion. So.
>>
>> @parent: the parent device to this udc.
> 
> Where you call below APIs? It seems to be a udc driver, right?
> So, when you try to get "otg-controller" from the node, this node
> should be udc.

@parent is actually the device that represents the USB Device controller
in the device tree. When you call usb_add_gadget_udc_release() a new
udc->dev device is created as it's child.

See explanation for the @parent argument in usb_add_gadget_udc_release().
As we want to keep the parent argument identical to that I will not make
any changes then.

> 
> /**
>  * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list
>  * @parent: the parent device to this udc. Usually the controller
>  * driver's device.
>  * @gadget: the gadget to be added to the list
>  * @otg_dev: the OTG controller device
>  *
>  * If otg_dev is NULL then device tree node is checked
>  * for OTG controller via the otg-controller property.
>  * Returns zero on success, negative errno otherwise.
>  */
> int usb_otg_add_gadget_udc(struct device *parent, struct usb_gadget *gadget,
> 			   struct device *otg_dev)
> {
> 	if (!otg_dev) {
> 		gadget->otg_dev = of_usb_get_otg(parent->of_node);
> 		if (!gadget->otg_dev)
> 			return -ENODEV;
> 	} else {
> 		gadget->otg_dev = otg_dev;
> 	}
> 
> 	return usb_add_gadget_udc_release(parent, gadget, NULL);
> }
> EXPORT_SYMBOL_GPL(usb_otg_add_gadget_udc);
> 

--
cheers,
-roger

[toc] | [prev] | [next] | [standalone]

#1420539

On Mon, Jun 13, 2016 at 10:37:59AM +0300, Roger Quadros wrote:
> On 13/06/16 10:20, Peter Chen wrote:
> > On Mon, Jun 13, 2016 at 10:14:31AM +0300, Roger Quadros wrote:
> >> On 12/06/16 14:36, Peter Chen wrote:
> >>> On Fri, Jun 10, 2016 at 04:07:22PM +0300, Roger Quadros wrote:
> >>>>  
> >>>> +/**
> >>>> + * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list
> >>>> + * @parent: the parent device to this udc. Usually the controller
> >>>> + * driver's device.
> >>>
> >>> It seems it should be udc device
> >>
> >> Parent and udc->dev are not the same right?
> > 
> > Sure, udc's parent is otg device.
> > 
> >>
> >> I guess i'll omit the second statement to avoid confusion. So.
> >>
> >> @parent: the parent device to this udc.
> > 
> > Where you call below APIs? It seems to be a udc driver, right?
> > So, when you try to get "otg-controller" from the node, this node
> > should be udc.
> 
> @parent is actually the device that represents the USB Device controller
> in the device tree. When you call usb_add_gadget_udc_release() a new
> udc->dev device is created as it's child.
> 
> See explanation for the @parent argument in usb_add_gadget_udc_release().
> As we want to keep the parent argument identical to that I will not make
> any changes then.

Oh, yes. The parent should be glue layer udc device or the dual-role controller
device.

Peter
> 
> > 
> > /**
> >  * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list
> >  * @parent: the parent device to this udc. Usually the controller
> >  * driver's device.
> >  * @gadget: the gadget to be added to the list
> >  * @otg_dev: the OTG controller device
> >  *
> >  * If otg_dev is NULL then device tree node is checked
> >  * for OTG controller via the otg-controller property.
> >  * Returns zero on success, negative errno otherwise.
> >  */
> > int usb_otg_add_gadget_udc(struct device *parent, struct usb_gadget *gadget,
> > 			   struct device *otg_dev)
> > {
> > 	if (!otg_dev) {
> > 		gadget->otg_dev = of_usb_get_otg(parent->of_node);
> > 		if (!gadget->otg_dev)
> > 			return -ENODEV;
> > 	} else {
> > 		gadget->otg_dev = otg_dev;
> > 	}
> > 
> > 	return usb_add_gadget_udc_release(parent, gadget, NULL);
> > }
> > EXPORT_SYMBOL_GPL(usb_otg_add_gadget_udc);
> > 
> 
> --
> cheers,
> -roger

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1420551 — [PATCH v11 13/14] usb: gadget: udc: adapt to OTG core

The OTG state machine needs a mechanism to start and
stop the gadget controller as well as connect/disconnect
from the bus. Add usb_gadget_start(), usb_gadget_stop()
and usb_gadget_connect_control().

Introduce usb_otg_add_gadget_udc() to allow controller drivers
to register a gadget controller that is part of an OTG instance.

Register with OTG core when UDC is added in usb_add_gadget_udc_release()
and unregister on usb_del_gadget_udc().

Notify the OTG core when gadget function driver is available on
udc_bind_to_driver() and when it is removed in usb_gadget_remove_driver().

We need to unlock the usb_lock mutex before calling
usb_otg_register_gadget() else it will cause a circular
locking dependency.

Ignore softconnect sysfs control when we're in OTG
mode as OTG FSM should care of gadget softconnect using
the b_bus_req mechanism.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
v11:
- removed left over -EPROBE_DEFER in udc_bind_to_driver().

 drivers/usb/gadget/udc/core.c | 202 +++++++++++++++++++++++++++++++++++++++---
 include/linux/usb/gadget.h    |   4 +
 2 files changed, 196 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 42756d7..1290f03 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -28,6 +28,11 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb.h>
+#include <linux/usb/otg.h>
+#include <linux/usb/of.h>
+
+#include <linux/of.h>
+#include <linux/of_platform.h>
 
 #include "trace.h"
 
@@ -1060,6 +1065,113 @@ static inline void usb_gadget_udc_stop(struct usb_udc *udc)
 }
 
 /**
+ * usb_gadget_to_udc - get the UDC owning the gadget
+ *
+ * udc_lock must be held.
+ * Returs NULL if UDC is not found.
+ */
+static struct usb_udc *usb_gadget_to_udc(struct usb_gadget *gadget)
+{
+	struct usb_udc *udc;
+
+	list_for_each_entry(udc, &udc_list, list)
+		if (udc->gadget == gadget)
+			return udc;
+
+	return NULL;
+}
+
+/**
+ * usb_gadget_start - start the usb gadget controller
+ * @gadget: the gadget device to start
+ *
+ * This is external API for use by OTG core.
+ *
+ * Start the usb device controller. Does not connect to the bus.
+ */
+static int usb_gadget_start(struct usb_gadget *gadget)
+{
+	int ret;
+	struct usb_udc *udc;
+
+	mutex_lock(&udc_lock);
+	udc = usb_gadget_to_udc(gadget);
+	if (!udc) {
+		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+			__func__);
+		mutex_unlock(&udc_lock);
+		return -EINVAL;
+	}
+
+	ret = usb_gadget_udc_start(udc);
+	if (ret)
+		dev_err(&udc->dev, "USB Device Controller didn't start: %d\n",
+			ret);
+
+	mutex_unlock(&udc_lock);
+
+	return ret;
+}
+
+/**
+ * usb_gadget_stop - stop the usb gadget controller
+ * @gadget: the gadget device we want to stop
+ *
+ * This is external API for use by OTG core.
+ *
+ * Stop the gadget controller. Does not disconnect from the bus.
+ * Caller must ensure that gadget has disconnected from the bus
+ * before calling usb_gadget_stop().
+ */
+static int usb_gadget_stop(struct usb_gadget *gadget)
+{
+	struct usb_udc *udc;
+
+	mutex_lock(&udc_lock);
+	udc = usb_gadget_to_udc(gadget);
+	if (!udc) {
+		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+			__func__);
+		mutex_unlock(&udc_lock);
+		return -EINVAL;
+	}
+
+	if (gadget->connected)
+		dev_dbg(gadget->dev.parent,
+			"%s: called while still connected\n", __func__);
+
+	usb_gadget_udc_stop(udc);
+	mutex_unlock(&udc_lock);
+
+	return 0;
+}
+
+static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect)
+{
+	struct usb_udc *udc;
+
+	mutex_lock(&udc_lock);
+	udc = usb_gadget_to_udc(gadget);
+	if (!udc) {
+		dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
+			__func__);
+		mutex_unlock(&udc_lock);
+		return -EINVAL;
+	}
+
+	if (connect) {
+		usb_gadget_connect(udc->gadget);
+	} else {
+		usb_gadget_disconnect(udc->gadget);
+		udc->driver->disconnect(udc->gadget);
+	}
+
+	mutex_unlock(&udc_lock);
+
+	return 0;
+}
+
+/**
  * usb_udc_release - release the usb_udc struct
  * @dev: the dev member within usb_udc
  *
@@ -1082,6 +1194,12 @@ static void usb_udc_nop_release(struct device *dev)
 	dev_vdbg(dev, "%s\n", __func__);
 }
 
+struct otg_gadget_ops otg_gadget_intf = {
+	.start = usb_gadget_start,
+	.stop = usb_gadget_stop,
+	.connect_control = usb_gadget_connect_control,
+};
+
 /**
  * usb_add_gadget_udc_release - adds a new gadget to the udc class driver list
  * @parent: the parent device to this udc. Usually the controller driver's
@@ -1137,6 +1255,14 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 	usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
 	udc->vbus = true;
 
+	if (gadget->otg_dev) {
+		mutex_unlock(&udc_lock);
+		ret = usb_otg_register_gadget(gadget, &otg_gadget_intf);
+		mutex_lock(&udc_lock);
+		if (ret)
+			goto err5;
+	}
+
 	/* pick up one of pending gadget drivers */
 	list_for_each_entry(driver, &gadget_driver_pending_list, pending) {
 		if (!driver->udc_name || strcmp(driver->udc_name,
@@ -1145,7 +1271,7 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 			if (ret != -EPROBE_DEFER)
 				list_del(&driver->pending);
 			if (ret)
-				goto err4;
+				goto err5;
 			break;
 		}
 	}
@@ -1154,6 +1280,8 @@ int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget,
 
 	return 0;
 
+err5:
+	device_del(&udc->dev);
 err4:
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
@@ -1215,6 +1343,33 @@ int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget)
 }
 EXPORT_SYMBOL_GPL(usb_add_gadget_udc);
 
+/**
+ * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list
+ * @parent: the parent device to the UDC. Usually the Gadget controller
+ * driver's device.
+ * @gadget: the gadget to be added to the list
+ * @otg_dev: the OTG controller device
+ *
+ * If otg_dev is NULL then device tree node is checked
+ * for OTG controller via the otg-controller property.
+ * Returns zero on success, negative errno otherwise.
+ */
+int usb_otg_add_gadget_udc(struct device *parent, struct usb_gadget *gadget,
+			   struct device *otg_dev)
+{
+	if (!otg_dev) {
+		gadget->otg_dev = of_usb_get_otg(parent->of_node);
+		if (!gadget->otg_dev)
+			return -ENODEV;
+	} else {
+		gadget->otg_dev = otg_dev;
+	}
+
+	return usb_add_gadget_udc_release(parent, gadget, NULL);
+}
+EXPORT_SYMBOL_GPL(usb_otg_add_gadget_udc);
+
+/* udc_lock must be held */
 static void usb_gadget_remove_driver(struct usb_udc *udc)
 {
 	dev_dbg(&udc->dev, "unregistering UDC driver [%s]\n",
@@ -1222,10 +1377,18 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 
-	usb_gadget_disconnect(udc->gadget);
-	udc->driver->disconnect(udc->gadget);
+	/* If OTG/dual-role, the otg core manages UDC start/stop */
+	if (udc->gadget->otg_dev) {
+		mutex_unlock(&udc_lock);
+		usb_otg_gadget_ready(udc->gadget, false);
+		mutex_lock(&udc_lock);
+	} else {
+		usb_gadget_disconnect(udc->gadget);
+		udc->driver->disconnect(udc->gadget);
+		usb_gadget_udc_stop(udc);
+	}
+
 	udc->driver->unbind(udc->gadget);
-	usb_gadget_udc_stop(udc);
 
 	udc->driver = NULL;
 	udc->dev.driver = NULL;
@@ -1259,6 +1422,9 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
 	}
 	mutex_unlock(&udc_lock);
 
+	if (gadget->otg_dev)
+		usb_otg_unregister_gadget(gadget);
+
 	kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE);
 	flush_work(&gadget->work);
 	device_unregister(&udc->dev);
@@ -1268,6 +1434,7 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
 
 /* ------------------------------------------------------------------------- */
 
+/* udc_lock must be held */
 static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
 {
 	int ret;
@@ -1282,17 +1449,26 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
 	ret = driver->bind(udc->gadget, driver);
 	if (ret)
 		goto err1;
-	ret = usb_gadget_udc_start(udc);
-	if (ret) {
-		driver->unbind(udc->gadget);
-		goto err1;
+
+	/* If OTG/dual-role, the otg core manages UDC start/stop */
+	if (udc->gadget->otg_dev) {
+		mutex_unlock(&udc_lock);
+		usb_otg_gadget_ready(udc->gadget, true);
+		mutex_lock(&udc_lock);
+	} else {
+		ret = usb_gadget_udc_start(udc);
+		if (ret) {
+			mutex_unlock(&udc_lock);
+			driver->unbind(udc->gadget);
+			goto err1;
+		}
+		usb_udc_connect_control(udc);
 	}
-	usb_udc_connect_control(udc);
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 	return 0;
 err1:
-	if (ret != -EISNAM)
+	if ((ret != -EISNAM))
 		dev_err(&udc->dev, "failed to start %s: %d\n",
 			udc->driver->function, ret);
 	udc->driver = NULL;
@@ -1389,6 +1565,12 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
 		return -EOPNOTSUPP;
 	}
 
+	/* In OTG/dual-role mode, soft-connect should be handled by OTG core */
+	if (udc->gadget->otg_dev) {
+		dev_err(dev, "soft-connect not supported in OTG mode\n");
+		return -EOPNOTSUPP;
+	}
+
 	if (sysfs_streq(buf, "connect")) {
 		usb_gadget_udc_start(udc);
 		usb_gadget_connect(udc->gadget);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 1d74fb8..8c6880d 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -701,6 +701,10 @@ extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
 extern void usb_del_gadget_udc(struct usb_gadget *gadget);
 extern char *usb_get_gadget_udc_name(void);
 
+extern int usb_otg_add_gadget_udc(struct device *parent,
+				  struct usb_gadget *gadget,
+				  struct device *otg_dev);
+
 /*-------------------------------------------------------------------------*/
 
 /* utility to simplify dealing with string descriptors */
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1420565 — Re: [PATCH v11 13/14] usb: gadget: udc: adapt to OTG core

On 13/06/16 10:56, Peter Chen wrote:
> On Mon, Jun 13, 2016 at 10:55:12AM +0300, Roger Quadros wrote:
>>  err1:
>> -	if (ret != -EISNAM)
>> +	if ((ret != -EISNAM))
> 
> Since you do not need above change.

Ah :P, will fix it.
> 
> Expect above, I am ok with this patch.
> 
> Acked-by: Peter Chen <peter.chen@nxp.com>

Thanks for the patient review of this entire series :).

cheers,
-roger

[toc] | [prev] | [next] | [standalone]

#1420572 — Re: [PATCH v11 13/14] usb: gadget: udc: adapt to OTG core

On Mon, Jun 13, 2016 at 10:55:12AM +0300, Roger Quadros wrote:
>  err1:
> -	if (ret != -EISNAM)
> +	if ((ret != -EISNAM))

Since you do not need above change.

Expect above, I am ok with this patch.

Acked-by: Peter Chen <peter.chen@nxp.com>

>  		dev_err(&udc->dev, "failed to start %s: %d\n",
>  			udc->driver->function, ret);
>  	udc->driver = NULL;
> @@ -1389,6 +1565,12 @@ static ssize_t usb_udc_softconn_store(struct device *dev,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	/* In OTG/dual-role mode, soft-connect should be handled by OTG core */
> +	if (udc->gadget->otg_dev) {
> +		dev_err(dev, "soft-connect not supported in OTG mode\n");
> +		return -EOPNOTSUPP;
> +	}
> +
>  	if (sysfs_streq(buf, "connect")) {
>  		usb_gadget_udc_start(udc);
>  		usb_gadget_connect(udc->gadget);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 1d74fb8..8c6880d 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -701,6 +701,10 @@ extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
>  extern void usb_del_gadget_udc(struct usb_gadget *gadget);
>  extern char *usb_get_gadget_udc_name(void);
>  
> +extern int usb_otg_add_gadget_udc(struct device *parent,
> +				  struct usb_gadget *gadget,
> +				  struct device *otg_dev);
> +
>  /*-------------------------------------------------------------------------*/
>  
>  /* utility to simplify dealing with string descriptors */
> -- 
> 2.7.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web