Robert Jarzmik wrote, On 01/17/2009 05:34 AM:
> Vernon Sauder <vernoninhand@xxxxxxxxx> writes:
>
>> From: Vernon Sauder <vsauder@xxxxxxxxxx>
>>
>> If a configuration change results in an endpoint being de-commissioned,
>> clean it up before deconfiguring it so the gadgets don't hang when
>> removed.
>> diff --git a/drivers/usb/gadget/pxa27x_udc.c b/drivers/usb/gadget/pxa27x_udc.c
>> index 4b5ac47..a896431 100644
>> --- a/drivers/usb/gadget/pxa27x_udc.c
>> +++ b/drivers/usb/gadget/pxa27x_udc.c
>> @@ -19,6 +19,9 @@
>> * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> *
>> */
>> +
>> +/* #define VERBOSE_DEBUG */
>> +
> A leftover maybe ?
"Purposely" leftover? There is no other hint that this file contains
verbose debug messages. Is there some other way to make that note? Or
should I have known that already?
>
> <snip>
>
>> @@ -405,8 +409,16 @@ static void update_pxa_ep_matches(struct pxa_udc *udc)
>>
>> for (i = 1; i < NR_USB_ENDPOINTS; i++) {
>> udc_usb_ep = &udc->udc_usb_ep[i];
>> - if (udc_usb_ep->pxa_ep)
>> - udc_usb_ep->pxa_ep = find_pxa_ep(udc, udc_usb_ep);
>> + if (udc_usb_ep->pxa_ep) {
>> + struct pxa_ep *ep = find_pxa_ep(udc, udc_usb_ep);
> Please, no declaration in code flow. Take that "struct pxa_ep *ep" to the
> beginning of the function.
I will correct this. I thought it was OK at the beginning of a new scope.
>> + if (ep) {
>> + udc_usb_ep->pxa_ep = ep;
>> + }
>> + else {
>> + ep_warn(udc_usb_ep->pxa_ep, "new config drops ep; disabling now\n");
>> + pxa_ep_disable(&udc_usb_ep->usb_ep);
>> + }
>> + }
>> }
>> }
> I think coding style requires something like :
> if (ep) {
> udc_usb_ep->pxa_ep = ep;
> } else {
> ep_warn(udc_usb_ep->pxa_ep, "new config drops ep; disabling now\n");
> pxa_ep_disable(&udc_usb_ep->usb_ep);
> }
>
True, my mistake. I will correct.
> And then, you'll have to explain to me why this is necessary.
>
> The normal path, as I understand it, is :
> - reconf irq arrives
> - irq_udc_reconfig() is called
> => pxa27x_change_interface() is called
> => gadget.setup() fonction is called
> => and then, update_pxa_ep_matches() is called after
>
> My understanding of the gadget API is that it is the "setup()" duty to disable
> unecessary endpoints by calling pxa_ep_disable() when needed.
>
> Now, if you've thought of a case where the gadget API does not behave that way,
> perhaps due to lack of knowledge of pxa27x_udc internal "mess" of pxa_ep and
> usb_ep matching, please explain it to me and we'll talk about that chunk again.
>
Continued in another thread.
> Cheers.
>
> --
> Robert
--
Regards,
Vernon Sauder :)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
