| Message ID | 1462530989-28434-1-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Superseded |
| Headers | show |
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@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot -- Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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);
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(+) -- 1.9.1 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot