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

Groups > linux.kernel > #1420246 > unrolled thread

Re: [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

  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

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

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] | [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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web