On Tue, Feb 28, 2012 at 09:36:07AM -0800, Ido Shayevitz wrote: > > > On Thu, Feb 09, 2012 at 04:58:15AM -0800, Ido Shayevitz wrote: > >> > >> > Hi, > >> > > >> > On Thu, Feb 09, 2012 at 02:04:30PM +0200, Ido Shayevitz wrote: > >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > >> >> index 582df0b..c2fb969 100644 > >> >> --- a/drivers/usb/dwc3/gadget.c > >> >> +++ b/drivers/usb/dwc3/gadget.c > >> >> @@ -518,7 +518,7 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep > >> >> *dep, > >> >> if (ret) > >> >> return ret; > >> >> > >> >> - dep->desc = desc; > >> >> + dep->endpoint.desc = desc; > >> > > >> > isn't this already set by choose_ep_by_config() ?? > >> > >> Yes, I agree, but I think that as long as the api is > >> ep->enable(ep*,desc*) > >> then we should keep this line, since it means that we need to enable the > >> endpoint with the given descriptor and not rely on the desc inside the > >> ep. > >> In my opinion, when changing the api to ep->enable(ep*) as Sebastian > >> suggested, then we can remove this line. > > > > no no, makes no sense, actually. We will be adding something which we > > *know* will be removed. Please rework this part to remove that line you > > added on all UDCs. > > Sorry for the late response, I missed your answer :-) > > Specifically for the dwc3 we cannot remove this line since > __dwc3_gadget_ep_enable() is also called by dwc3_gadget_start() which > doesn't set the desc before calling, so removing the line is causing a > bug. If you think that it is fine to just change also dwc3_gadget_start() > it is fine by me, I will do it. > > Still, I am suggesting to remove the line only when we will do the rework > to chnage the ep->enable API, this way no such bugs will occur. Yes we > *know* that it will be removed, but as matter of API design, since the > function gets a pointer to desc, it cannot rely on the caller to set the > desc, as happen with the dwc3_gadget_start() case. > > I patched only ci13xxx and dwc3 since they are the only 2 I can test. > I can do also for all the other UDCs (each in it's own commit) and let the > community to test? let me know if you want me to do it. Yup, makes sense. Fix them all and wait for a Tested-by or Acked-by someone else ;-) > In the next phase I can rework to change the ep->enable API and remove the > lines we agreed that are need to be removed. For now, for this change: > "remove the redundant desc pointer", I think that it is logically ok to > just replace any location of dep->desc with dep->endpoint.desc ok, sounds like a plan to me. thanks -- balbi
Attachment:
signature.asc
Description: Digital signature
