Hi, On Thu, Jul 26, 2012 at 07:00:03PM +0800, Bhupesh SHARMA wrote: > > > Yes. Felipe, plz go through the problem description as mentioned in > > > the patch log message which can be seen here: > > > http://permalink.gmane.org/gmane.linux.usb.general/66585 > > > > sure, please see above patch. The difference is that this has no impact > > on the controller drivers. > > Yes. I had sent a modified patch (on top of my original one) that did not affect the > controller driver and instead modified the way 'pullup' is called from the udc-core, > based on the simple 'is_pulleddown' flag. Plz see it here: > > http://www.spinics.net/lists/linux-usb/msg66338.html > > This patch has an added advantage of not even modifying the webcam_driver like composite drivers > and has been tested to work well at-least with the webcam gadget. That patch doesn't make a lot of sense. I'll paste it here for reference: | diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c index a6ebeec..4851d99 100644 | --- a/drivers/usb/gadget/udc-core.c | +++ b/drivers/usb/gadget/udc-core.c | @@ -355,7 +355,8 @@ found: | driver->unbind(udc->gadget); | goto err1; | } | - usb_gadget_connect(udc->gadget); | + if (!udc->gadget->is_pulleddown) | + usb_gadget_connect(udc->gadget); | } else { | | ret = usb_gadget_start(udc->gadget, driver, bind); diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 665b1d1..b1d997a 100644 | --- a/include/linux/usb/gadget.h | +++ b/include/linux/usb/gadget.h | @@ -583,6 +583,7 @@ struct usb_gadget { | unsigned b_hnp_enable:1; | unsigned a_hnp_support:1; | unsigned a_alt_hnp_support:1; | + unsigned is_pulleddown:1; | const char *name; | struct device dev; | }; | @@ -808,9 +809,16 @@ static inline int usb_gadget_connect(struct usb_gadget *gadget) | */ | static inline int usb_gadget_disconnect(struct usb_gadget *gadget) { | + int ret; | + | if (!gadget->ops->pullup) | return -EOPNOTSUPP; | - return gadget->ops->pullup(gadget, 0); | + ret = gadget->ops->pullup(gadget, 0); | + | + if (!ret) | + gadget->is_pulleddown = true; | + | + return ret; | } My problem is with this last hunk, for a number of reasons. a. you never clear that flag which means that if you load uvc (which controls pullup), then unload it and load g_mass_storage (which doesn't control pullup) g_mass_storage will never enumerate. b. gadget driver and udc are not talking to each other, so there's no explicit agreement on what to do with pullup and who will control it. c. I want all gadget drivers to control their own pullups in the near future, so that exposing that to userland becomes simpler, this patch doesn't get close to that. d. you tested only with the gadget you care about. You need a much more extensive test to be able to show me that this will cause no regressions. e. This changes the behavior for all gadgets after the first gadget is loaded (because you never clear is_pulleddown). In summary, this patch is wrong and can't be applied. -- balbi
Attachment:
signature.asc
Description: Digital signature
