Hi Paul, On Mon, July 30, 2012 12:00 pm, Paul Zimmerman wrote: >> From: Ido Shayevitz [mailto:idos@xxxxxxxxxxxxxx] >> Sent: Monday, July 30, 2012 3:15 AM >> >> On Thu, July 26, 2012 4:38 pm, Paul Zimmerman wrote: >> >> From: linux-usb-owner@xxxxxxxxxxxxxxx >> >> [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Ido Shayevitz >> >> Sent: Wednesday, July 25, 2012 5:46 AM >> >> --- >> >> drivers/usb/dwc3/Kconfig | 6 +- >> >> drivers/usb/dwc3/Makefile | 2 + >> >> drivers/usb/dwc3/core.c | 15 +- >> >> drivers/usb/dwc3/core.h | 54 +++++- >> >> drivers/usb/dwc3/dwc3_otg.c | 512 >> >> ++++++++++++++++++++++++++++++++++++++++++ >> >> drivers/usb/dwc3/dwc3_otg.h | 38 +++ >> >> drivers/usb/dwc3/gadget.c | 63 +++++ >> >> drivers/usb/host/xhci-plat.c | 21 ++ >> >> drivers/usb/host/xhci.c | 13 +- >> >> 9 files changed, 711 insertions(+), 13 deletions(-) >> >> create mode 100644 drivers/usb/dwc3/dwc3_otg.c >> >> create mode 100644 drivers/usb/dwc3/dwc3_otg.h >> >> >> >> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >> >> index d13c60f..0cc108d 100644 >> >> --- a/drivers/usb/dwc3/Kconfig >> >> +++ b/drivers/usb/dwc3/Kconfig >> >> @@ -1,9 +1,9 @@ >> >> config USB_DWC3 >> >> tristate "DesignWare USB3 DRD Core Support" >> >> - depends on (USB && USB_GADGET) >> >> + depends on (USB || USB_GADGET) >> > >> > Are you sure this is correct? How can a dual-role device work unless >> both >> > USB and USB_GADGET are set? >> >> If the USB_DWC3 is not selected then even dwc3/core.c is not being built >> and core.c supports also non DRD cores (depends on DWC3_MODE), so we >> want >> to build the DWC3 if there is at least host support or a gadget support. >> In case that the DWC_MODE is DRD but USB is not on, then dwc3/host.c >> will >> add the xHCI device, but will be no xHCI driver (xhci-plat) that will >> probe on this device. > > I just tried compiling this without USB_GADGET defined, and I get > these errors: > > HOSTCC arch/x86/boot/compressed/mkpiggy > ERROR: "usb_gadget_map_request" [drivers/usb/dwc3/dwc3.ko] undefined! > ERROR: "usb_del_gadget_udc" [drivers/usb/dwc3/dwc3.ko] undefined! > ERROR: "usb_gadget_unmap_request" [drivers/usb/dwc3/dwc3.ko] undefined! > ERROR: "usb_add_gadget_udc" [drivers/usb/dwc3/dwc3.ko] undefined! > Yes, you are totally right. So we have two options: The first is, if you agree with my reasoning: "and core.c supports also non DRD cores (depends on DWC3_MODE), so we want to build the DWC3 if there is at least host support or a gadget support." Then we may want to build gadget.c and ep0.c only if USB_GADGET is selected and this requires also changes in core.c or core.h (providing empty prototype for dwc3_gadget_init() ?) The second option is indeed to keep it as it is, means USB && USB_GADGET. It means that kernel image size will have to include xHCI driver and dwc3 gadget driver, nevertheless if the controller is DRD or not. Basically I am ok with that too. Please let me know what you and Felipe decide... >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> >> index c34452a..5343e39 100644 >> >> --- a/drivers/usb/dwc3/core.c >> >> +++ b/drivers/usb/dwc3/core.c >> >> @@ -517,15 +517,24 @@ static int __devinit dwc3_probe(struct >> >> platform_device *pdev) >> >> break; >> >> case DWC3_MODE_DRD: >> >> dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG); >> >> + ret = dwc3_otg_init(dwc); >> >> + if (ret) { >> >> + dev_err(dev, "failed to initialize otg\n"); >> >> + goto err1; >> >> + } >> >> + >> > >> > You should follow the existing pattern for error handling in this >> > function. If there is an error, jump to the error exit, and put your >> > exit function there, instead of here. >> >> I think I followed the pattern for error handling in this function. >> Please see that it is the same as "case DWC3_MODE_HOST" or "case >> DWC3_MODE_DEVICE" >> >> >> ret = dwc3_host_init(dwc); >> >> if (ret) { >> >> dev_err(dev, "failed to initialize host\n"); >> >> + dwc3_otg_exit(dwc); >> > >> > Same here. >> >> Here I was thinking of moving "dwc3_otg_exit()" to the error handling >> section, but I think it will make the error handling section too clumsy >> since eventhough the mode is DWC3_MODE_DRD, in this point I want to exit >> only the otg and not the host or gadget. So this should have a new error >> label that will be located between err1 and err2. On the other hand if >> the >> error will happen later, as the "goto err2" statement below in this >> function, then if the mode is DRD I will want to activate the exit >> function of also the host and gadget but not doing the otg exit again >> (the >> new label) and then do err1... So it becomes clumsy. > > Yes, I see now that the error handling for DRD in that function was > broken before your patch, and your patch fixes it. So it's up to > Felipe to decide if the way you did it is acceptable. > > -- > Paul > > -- > 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 > 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
