> 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. 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 > -- > balbi > Ido -- Consultant for Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum -- 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
