[U-Boot] [PATCH] usb: xhci: add struct devrequest declaration

[Marek Vasut]

On 05/11/2016 01:03 PM, Masahiro Yamada wrote:
> Hi Marek,

Hi!

> 2016-05-07 1:40 GMT+09:00 Marek Vasut <marex at denx.de>:
>> On 05/06/2016 01:31 PM, Masahiro Yamada wrote:
>>> Hi Marek,
>>
>> Hi!
>>
>>> 2016-05-06 19:50 GMT+09:00 Marek Vasut <marex at denx.de>:
>>>> On 05/06/2016 12:36 PM, Masahiro Yamada wrote:
>>>>> This should be declared for xhci_ctrl_tx() to avoid build error.
>>>>
>>>> Can you please include the build error in the commit message ?
>>>> That is extremely useful.
>>>>
>>>>> Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com>
>>>>> ---
>>>>>
>>>>>  drivers/usb/host/xhci.h | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>>>>> index 2afa386..16dd61a 100644
>>>>> --- a/drivers/usb/host/xhci.h
>>>>> +++ b/drivers/usb/host/xhci.h
>>>>> @@ -1252,6 +1252,8 @@ void xhci_acknowledge_event(struct xhci_ctrl *ctrl);
>>>>>  union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected);
>>>>>  int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe,
>>>>>                int length, void *buffer);
>>>>> +
>>>>> +struct devrequest;
>>>>
>>>> I don't think that's the right thing to do, since the structure
>>>> devrequest is not defined anywhere in this file or the headers
>>>> which are included in this file.
>>>>
>>>> Will this patch work for you instead? It includes usb.h , which
>>>> defines the struct devrequest.
>>>
>>> Of course, works.
>>> But why?
>>
>> Because you want to have definition of every symbol you use in your
>> headers when you include that header. I am not a big fan of huge stack
>> of #include statements in a driver.
> 
> Agree.  That's why this patch is here.
> 
> See this patch.
> http://patchwork.ozlabs.org/patch/620989/
> 
> 
> xhci-dwc3.c needs to include xhci.h, but not usb.h
> because this driver just wants to register the xHCI controller.
> It need not know complicated USB protocol things in the first place.

In that case, shouldn't the xhci_register() be in xhci.h instead ?

> Your way adds #include <include/usb.h> to xhci.h,
> so xhci-dwc3.c is compelled to include unnecessary usb.h
> 
> It means huge stack of includes you do not like, isn't it?

It does add possibly unused symbols to the namespace, yes.
I don't think I really mind this though, since the symbol
definition is available when I include xhci.h somewhere.

>>> xhci_ctrl_tx() only takes a pointer to struct devrequest.
>>>
>>> This header does not need to know
>>> the members of struct devrequest, or sizeof(struct devrequest).
>>>
>>> We need to teach it that devrequest is a structure.
>>> That's enough.
>>>
>>>
>>> This is a very common way to fix
>>> "warning: 'struct devrequest' declared inside parameter list" error.
>>>
>>> For example,
>>>
>>> head -25 include/linux/clk.h
>>> head -30 include/linux/pinctrl/pinctrl.h
>>>
>>> in Linux Kernel.
>>
>> Ha, I didn't know about that. Is that some new recommended practice or
>> is this a matter of taste ?
> 
> I guess neither of them.
> 
> I recommend this practice for any projects written in C
> for two reasons:
> 
> 
> [1] Avoid unneeded includes
>   This is explained above.
> 
> 
> [2] Scope control
> 
> We have a good example in Linux Kernel.
> 
> struct clk is defined in drivers/clk/clk.c
> 
> It means that no clock consumers/providers are allowed
> to have access to any members of struct clk directly.
> 
> (If you want to get access to such private members,
> it is a good charm you are doing wrong.)
> 
> So, include/linux/clk.h has just a declaration:
> 
> struct clk;
> 
> It can claim clk is a structure,
> but not expose its internal details.
> 
> When we implement APIs, it is a good practice to think about
> private/public members.

This is a good point about the clk API. OK, let's go with this approach.

>> CCing Tom and Simon, so I can get some more input on this. I cannot
>> decide either way myself.
>>
> 
> You can.   Just consult yourself as a software engineer.

No comment.

-- 
Best regards,
Marek Vasut