Hi Robert, On 09/17/2014 03:52 PM, Stephen Warren wrote: > On 09/17/2014 02:47 PM, Dinh Nguyen wrote: >> Hi Robert, >> >> On 9/12/14, 7:13 AM, Robert Baldyga wrote: >>> Hi Dinh, >>> >>> On 08/26/2014 06:19 PM, dinguyen@xxxxxxxxxxxxxxxxxxxxx wrote: >>>> From: Dinh Nguyen <dinguyen@xxxxxxxxxxxxxxxxxxxxx> >>>> >>>> Add the proper init calls for either host, gadget or both in platform.c >>>> >>>> Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxxxxxxxxxxxxx> >>>> Acked-by: Paul Zimmerman <paulz@xxxxxxxxxxxx> >>>> --- >>>> drivers/usb/dwc2/core.h | 13 +++++++++++++ >>>> drivers/usb/dwc2/gadget.c | 2 +- >>>> drivers/usb/dwc2/platform.c | 28 ++++++++++++++++++++++++---- >>>> 3 files changed, 38 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >>>> index f55e62d..3a49a00 100644 >>>> --- a/drivers/usb/dwc2/core.h >>>> +++ b/drivers/usb/dwc2/core.h >>>> @@ -960,6 +960,19 @@ extern void dwc2_dump_global_registers(struct >>>> dwc2_hsotg *hsotg); >>>> */ >>>> extern u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg); >>>> >>>> +/* Gadget defines */ >>>> +#if defined(CONFIG_USB_DWC2_PERIPHERAL) || >>>> defined(CONFIG_USB_DWC2_DUAL_ROLE) >>>> +extern int s3c_hsotg_remove(struct dwc2_hsotg *hsotg); >>>> +extern void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2); >>> >>> Function s3c_hsotg_core_init() is used only inside file gadget.c so >>> exporting it makes no sense. By the way it should be static. >> >> Yes, I agree here. Fixed up in v5. I went back to look at the code and realized that I had to use s3c_hsotg_core_init() in patch "5/12 -> usb: dwc2: Initialize the USB core for peripheral mode". I need to add the call to s3c_hsotg_core_init() for a B-cable insert event, and this is in hcd.c. So I need to export it. >> >>> >>>> +extern int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq); >>>> +#else >>>> +static inline void s3c_hsotg_core_init(struct dwc2_hsotg *dwc2) {} >>>> +static inline int s3c_hsotg_remove(struct dwc2_hsotg *dwc2) >>>> +{ return 0; } >>>> +static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) >>>> +{ return 0; } >>> >>> It also makes no sense to have this functions declared if you don't have >>> to use them. They are called in one place in code, inside >>> dwc2_driver_probe() function, so you can rather use if defined() there. >> >> I'm not sure I agree here. This is necessary for the current runtime >> implementation of the role initialization. This is probably relevant >> with your next 2 comments. >> >>> >>>> +#endif >>>> + >>>> #if defined(CONFIG_USB_DWC2_HOST) || >>>> defined(CONFIG_USB_DWC2_DUAL_ROLE) >>>> /** >>>> * dwc2_hcd_get_frame_number() - Returns current frame number >>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >>>> index 96f868f..efa68a0 100644 >>>> --- a/drivers/usb/dwc2/gadget.c >>>> +++ b/drivers/usb/dwc2/gadget.c >>>> @@ -3572,7 +3572,7 @@ err_clk: >>>> * s3c_hsotg_remove - remove function for hsotg driver >>>> * @pdev: The platform information for the driver >>>> */ >>>> -static int s3c_hsotg_remove(struct dwc2_hsotg *hsotg) >>>> +int s3c_hsotg_remove(struct dwc2_hsotg *hsotg) >>>> { >>>> usb_del_gadget_udc(&hsotg->gadget); >>>> >>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c >>>> index dd2f8f5..2871f351 100644 >>>> --- a/drivers/usb/dwc2/platform.c >>>> +++ b/drivers/usb/dwc2/platform.c >>>> @@ -92,7 +92,14 @@ static int dwc2_driver_remove(struct >>>> platform_device *dev) >>>> { >>>> struct dwc2_hsotg *hsotg = platform_get_drvdata(dev); >>>> >>>> - dwc2_hcd_remove(hsotg); >>>> + if (IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL)) >>>> + s3c_hsotg_remove(hsotg); >>>> + else if (IS_ENABLED(CONFIG_USB_DWC2_HOST)) >>>> + dwc2_hcd_remove(hsotg); >>>> + else { /* dual role */ >>>> + dwc2_hcd_remove(hsotg); >>>> + s3c_hsotg_remove(hsotg); >>>> + } >>> >>> Why don't make this checks compile-time? >>> >> >> Do you have a reason for a compile-time versus runtime here? It just >> seems that from a few discussion threads on lkml that there is a general >> biased towards using IS_ENABLED() as it looks a bit cleaner than >> littering the code with a bunch of #ifdefs. > > With typical compiler optimization, if (IS_ENABLED(...)) *is* a > compile-time check. Yet, it allows the code within the if block body to > be parsed, so that even if the code doesn't make it into the binary, it > still gets syntax checking etc. Thanks Stephen for this information! Robert, are your comments properly addressed for this patch? Thanks, Dinh -- 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