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, dingu...@opensource.altera.com wrote:
>>>> From: Dinh Nguyen <dingu...@opensource.altera.com>
>>>>
>>>> Add the proper init calls for either host, gadget or both in platform.c
>>>>
>>>> Signed-off-by: Dinh Nguyen <dingu...@opensource.altera.com>
>>>> Acked-by: Paul Zimmerman <pa...@synopsys.com>
>>>> ---
>>>> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
