On 05/11/2016 01:03 PM, Masahiro Yamada wrote: > Hi Marek, Hi!
> 2016-05-07 1:40 GMT+09:00 Marek Vasut <ma...@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 <ma...@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.masah...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
