On Thu, Sep 18, 2014 at 10:17 AM, Dinh Nguyen
<dingu...@opensource.altera.com> wrote:
> 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.
I can see your reason for the comment. So I'll move the export of the
function into the next patch that needs it, to make it clearer.
Dinh
>
>>>
>>>>
>>>>> +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
