usb: xhci: add struct devrequest declaration
Subsystems (2)
PATCH 1/1 usb: xhci: add struct devrequest declaration
This should be declared for xhci_ctrl_tx() to avoid build error.
---
drivers/usb/host/xhci.h | 2 ++ 1 file changed, 2 insertions(+)
drivers/usb/host/xhci.h +2 -0
@@ -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);
Signature · 1 line
1.9.1
PATCH 1/1 usb: xhci: add struct devrequest declaration
Quoted 1 line · Masahiro Yamada
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.
Quoted 11 lines
Signed-off-by Masahiro Yamada <[email protected]>---
drivers/usb/host/xhci.h | 2 ++ 1 file changed, 2 insertions(+)drivers/usb/host/xhci.h +0
-0
drivers/usb/host/xhci.h void xhci_acknowledge_event(struct xhci_ctrl *ctrl); · line 1256
Quoted 5 lines
drivers/usb/host/xhci.h +2
-0@@ -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---
drivers/usb/host/xhci.h +1 -0
@@ -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<---
Quoted 4 lines
drivers/usb/host/xhci.h +0
-0int 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);
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 usb: xhci: add struct devrequest declaration
Hi Marek,
2016-05-06 19:50 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 27 lines · Masahiro Yamada
Quoted 1 line
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.
Quoted 11 lines
Signed-off-by Masahiro Yamada <[email protected]>---
drivers/usb/host/xhci.h | 2 ++ 1 file changed, 2 insertions(+)drivers/usb/host/xhci.h +0
-0drivers/usb/host/xhci.h void xhci_acknowledge_event(struct xhci_ctrl *ctrl); · line 1256
Quoted 5 lines
drivers/usb/host/xhci.h +2
-0@@ -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.
Quoted 2 lines
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.
Quoted 28 lines
--->8---
drivers/usb/host/xhci.h +1
-1@@ -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<---drivers/usb/host/xhci.h void xhci_acknowledge_event(struct xhci_ctrl *ctrl); · line 1259
Quoted 5 lines
drivers/usb/host/xhci.h +0
-0int 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
Signature · 2 lines
Best Regards Masahiro Yamada
PATCH 1/1 usb: xhci: add struct devrequest declaration
Quoted 1 line · Masahiro Yamada
Hi Marek,
Hi!
Quoted 28 lines
2016-05-06 19:50 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 27 lines · Masahiro Yamada
Quoted 1 line
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.
Quoted 11 lines
Signed-off-by Masahiro Yamada <[email protected]>---
drivers/usb/host/xhci.h | 2 ++ 1 file changed, 2 insertions(+)drivers/usb/host/xhci.h +0
-0drivers/usb/host/xhci.h void xhci_acknowledge_event(struct xhci_ctrl *ctrl); · line 1256
Quoted 5 lines
drivers/usb/host/xhci.h +2
-0@@ -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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 12 lines
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.
Quoted 6 lines
For example,
head -25
include/linux/clk.h
head -30include/linux/pinctrl/pinctrl.hin 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.
Quoted 31 lines
Quoted 28 lines
--->8---
drivers/usb/host/xhci.h +1
-1@@ -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<---drivers/usb/host/xhci.h void xhci_acknowledge_event(struct xhci_ctrl *ctrl); · line 1259
Quoted 5 lines
drivers/usb/host/xhci.h +0
-0int 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
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 usb: xhci: add struct devrequest declaration
Hi Marek,
2016-05-07 1:40 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 31 lines
2016-05-06 19:50 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 29 lines · Masahiro Yamada
Quoted 1 line
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.
Quoted 11 lines
Signed-off-by Masahiro Yamada <[email protected]>---
drivers/usb/host/xhci.h | 2 ++ 1 file changed, 2 insertions(+)drivers/usb/host/xhci.h +0
-0drivers/usb/host/xhci.h void xhci_acknowledge_event(struct xhci_ctrl *ctrl); · line 1256
Quoted 5 lines
drivers/usb/host/xhci.h +2
-0@@ -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.
Quoted 6 lines
Quoted 2 lines
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?
Quoted 14 lines
Quoted 14 lines
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,
Quoted 7 lines
Quoted 4 lines
head -25
include/linux/clk.h
head -30include/linux/pinctrl/pinctrl.hin 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.
Quoted 3 lines
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.
Signature · 2 lines
Best Regards Masahiro Yamada
PATCH 1/1 usb: xhci: add struct devrequest declaration
Quoted 1 line · Masahiro Yamada
Hi Marek,
Hi!
Quoted 46 lines
2016-05-07 1:40 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 31 lines
2016-05-06 19:50 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 29 lines · Masahiro Yamada
Quoted 1 line
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.
Quoted 11 lines
Signed-off-by Masahiro Yamada <[email protected]>---
drivers/usb/host/xhci.h | 2 ++ 1 file changed, 2 insertions(+)drivers/usb/host/xhci.h +0
-0drivers/usb/host/xhci.h void xhci_acknowledge_event(struct xhci_ctrl *ctrl); · line 1256
Quoted 5 lines
drivers/usb/host/xhci.h +2
-0@@ -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.Quoted 6 lines
Quoted 2 lines
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.
Quoted 7 lines
See this patch.
http://patchwork.ozlabs.org/patch/620989/
xhci-dwc3.cneeds to includexhci.h, but notusb.hbecause 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 ?
Quoted 4 lines
Your way adds #include <
include/usb.h> toxhci.h, soxhci-dwc3.cis compelled to include unnecessaryusb.hIt 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.
Quoted 46 lines
Quoted 14 lines
Quoted 14 lines
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,
Quoted 7 lines
Quoted 4 lines
head -25
include/linux/clk.h
head -30include/linux/pinctrl/pinctrl.hin 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.cIt 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.hhas just a declaration:
Quoted 7 lines
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.
Quoted 5 lines
Quoted 3 lines
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.
Signature · 2 lines
Best regards, Marek Vasut
PATCH 1/1 usb: xhci: add struct devrequest declaration
Hi Marek,
2016-05-11 20:22 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 10 lines
Quoted 10 lines
Quoted 3 lines
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/
Quoted 5 lines
Quoted 3 lines
xhci-dwc3.cneeds to includexhci.h, but notusb.hbecause 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 inxhci.hinstead ?
Ah, right.
That will make APIs clearer, but I cannot touch it because I do not have either time or skill to refactor it.
Quoted 8 lines
Quoted 4 lines
Your way adds #include <
include/usb.h> toxhci.h, soxhci-dwc3.cis compelled to include unnecessaryusb.hIt 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.hsomewhere.
Right.
But, what I can tell at least is that
it would make the compiler to parse additional headers for nothing.
Signature · 2 lines
Best Regards Masahiro Yamada
PATCH 1/1 usb: xhci: add struct devrequest declaration
Quoted 19 lines · Masahiro Yamada
Hi Marek,
2016-05-11 20:22 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 14 lines
Quoted 13 lines
Quoted 3 lines
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.cneeds to includexhci.h, but notusb.hbecause this driver just wants to register the xHCI controller. It need not know complicated USB protocol things in the first place.
Quoted 6 lines
Quoted 1 line
In that case, shouldn't the
xhci_register()be inxhci.hinstead ?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.
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Your way adds #include <
include/usb.h> toxhci.h, soxhci-dwc3.cis compelled to include unnecessaryusb.hIt means huge stack of includes you do not like, isn't it?
Quoted 8 lines
Quoted 3 lines
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.hsomewhere.Right.
But, what I can tell at least is that
it would make the compiler to parse additional headers for nothing.
Right.
Signature · 2 lines
Best regards, Marek Vasut