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

[Masahiro Yamada]

Hi Marek,


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.

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?



>> 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.





> 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.



-- 
Best Regards
Masahiro Yamada