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

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

[Masahiro Yamada]

This should be declared for xhci_ctrl_tx() to avoid build error.

Signed-off-by: Masahiro Yamada <yamada.masahiro@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;
 int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe,
 		 struct devrequest *req, int length, void *buffer);
 int xhci_check_maxpacket(struct usb_device *udev);
-- 
1.9.1

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

[Marek Vasut]

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

--->8---
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 2afa386..b5a2ea5 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -22,6 +22,7 @@
 #include <asm/io.h>
 #include <linux/list.h>
 #include <linux/compat.h>
+#include <usb.h>

 #define MAX_EP_CTX_NUM         31
 #define XHCI_ALIGNMENT         64
---8<---

>  int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe,
>  		 struct devrequest *req, int length, void *buffer);
>  int xhci_check_maxpacket(struct usb_device *udev);
> 


-- 
Best regards,
Marek Vasut

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

[Masahiro Yamada]

Hi Marek,


2016-05-06 19:50 GMT+09:00 Marek Vasut <marex@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@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?


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.




> --->8---
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 2afa386..b5a2ea5 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -22,6 +22,7 @@
>  #include <asm/io.h>
>  #include <linux/list.h>
>  #include <linux/compat.h>
> +#include <usb.h>
>
>  #define MAX_EP_CTX_NUM         31
>  #define XHCI_ALIGNMENT         64
> ---8<---
>
>>  int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe,
>>                struct devrequest *req, int length, void *buffer);
>>  int xhci_check_maxpacket(struct usb_device *udev);
>>
>
>
> --
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Best Regards
Masahiro Yamada

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

[Marek Vasut]

On 05/06/2016 01:31 PM, Masahiro Yamada wrote:
> Hi Marek,

Hi!

> 2016-05-06 19:50 GMT+09:00 Marek Vasut <marex@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@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.

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

CCing Tom and Simon, so I can get some more input on this. I cannot
decide either way myself.

>> --->8---
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 2afa386..b5a2ea5 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -22,6 +22,7 @@
>>  #include <asm/io.h>
>>  #include <linux/list.h>
>>  #include <linux/compat.h>
>> +#include <usb.h>
>>
>>  #define MAX_EP_CTX_NUM         31
>>  #define XHCI_ALIGNMENT         64
>> ---8<---
>>
>>>  int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe,
>>>                struct devrequest *req, int length, void *buffer);
>>>  int xhci_check_maxpacket(struct usb_device *udev);
>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
> 
> 
> 


-- 
Best regards,
Marek Vasut

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

[Masahiro Yamada]

Hi Marek,


2016-05-07 1:40 GMT+09:00 Marek Vasut <marex@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@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@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

[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@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@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@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

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

[Masahiro Yamada]

Hi Marek,


2016-05-11 20:22 GMT+09:00 Marek Vasut <marex@denx.de>:

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

Ah, right.

That will make APIs clearer, but I cannot touch it
because I do not have either time or skill to refactor it.


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

Right.

But, what I can tell at least is that
it would make the compiler to parse additional headers for nothing.



-- 
Best Regards
Masahiro Yamada

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

[Marek Vasut]

On 05/12/2016 05:36 AM, Masahiro Yamada wrote:
> Hi Marek,
> 
> 
> 2016-05-11 20:22 GMT+09:00 Marek Vasut <marex@denx.de>:
> 
>>>> 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 ?
> 
> Ah, right.
> 
> That will make APIs clearer, but I cannot touch it
> because I do not have either time or skill to refactor it.

Ha, ok.

>>> 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.
> 
> Right.
> 
> But, what I can tell at least is that
> it would make the compiler to parse additional headers for nothing.

Right.

-- 
Best regards,
Marek Vasut