| Message ID | 1447393422-4169-8-git-send-email-nm@ti.com |
|---|---|
| State | New |
| Headers | show |
On 11/13/2015 09:32 AM, Simon Guinot wrote: > On Fri, Nov 13, 2015 at 09:06:45AM -0500, Tom Rini wrote: >> On Fri, Nov 13, 2015 at 11:30:43AM +0100, Simon Guinot wrote: >>> Hi Nishanth, >>> >>> On Thu, Nov 12, 2015 at 11:43:37PM -0600, Nishanth Menon wrote: >>>> Header files can be located in a generic location without >>>> needing to reference them with ../common/ >>>> >>>> Generated with the following script >>>> >>>> #!/bin/bash >>>> vendor=board/LaCie >>>> common=$vendor/common >>>> >>>> cfiles=`git grep "../common" $vendor|grep "#include"|cut -d '"' -f2|sort -u|grep c$` >>>> headers=`git grep "../common" $vendor|grep "#include"|cut -d '"' -f2|sort -u|grep h$` >>>> >>>> mkdir -p $common/include/board-common >>>> set -x >>>> for header in $headers >>>> do >>>> echo "processing $header in $common" >>>> hbase=`basename $header` >>>> git mv $common/$hbase $common/include/board-common >>>> sed -i -e "s/\"..\/common\/$hbase\"/<board-common\/$hbase>/g" $vendor/*/*.[chS] >>>> sed -i -e "s/\"$hbase\"/<board-common\/$hbase>/g" $vendor/common/*.[chS] >>>> done >>>> >>>> Cc: Simon Guinot <simon.guinot@sequanux.org> >>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> >>>> >>>> Signed-off-by: Nishanth Menon <nm@ti.com> >>>> --- >>>> board/LaCie/common/cpld-gpio-bus.c | 2 +- >>>> board/LaCie/common/{ => include/board-common}/common.h | 0 >>> >>> Is that really a good idea to move a LaCie-specific file named common.h >>> to a place shared with other boards ? >>> >>>> board/LaCie/common/{ => include/board-common}/cpld-gpio-bus.h | 0 >>> >>> IMO, this headers are specific to LaCie boards and it don't make much >>> sense to move them to a shared place. Moreover it is quite convenient to >>> have them close from the board setup files. >>> >>> Please don't move them. >> >> Please read and then suggest changes in the "Makefile: Include vendor >> common library in include search path" thread. Thanks! > > Hi Tom, > > Do you mean I answered without even really looking at the suggested > change ? Well, it is true :) > > Sorry about that. I have been confused by the "git move file" notation > which I am not familiar with. So, I withdraw my previous objections. > Thanks. > Now, I have to say that I am still not convinced by the change. > > After applying this patch, I can see that: > > #include "../common/cpld-gpio-bus.h" > > have been replaced with: > > #include <board-common/cpld-gpio-bus.h> > > While this change is fine, I can also see that the header file has been > moved from: > > board/LaCie/common/cpld-gpio-bus.h > > to: > > board/LaCie/common/include/board-common/cpld-gpio-bus.h > > With both the strings "board" and "common" duplicated, I am definitively > not a big fan of this new path. Moreover I think that moving the headers > away from the board setup files will harm a little bit the code reading. > > Maybe it would be better to have a shorter path like: > > board/LaCie/include/board-common/cpld-gpio-bus.h That can easily be done as well. for me, it is just regenerating the scripts.. I strongly suggest to comment on the original patch https://patchwork.ozlabs.org/patch/544030/ and suggest this so that other platform folks have the discussion context as well. > > Anyway, IMHO things are fine as they are. But if anyone thinks this > change is needed or valuable, then I am OK with it. IMHO, I started on this story, because we have a need to have a board/ti/common directory and adding more cruft on top of what is already a bunch of crufty includes is just painful. If you are interested in the complete history: https://patchwork.ozlabs.org/patch/540280/ https://patchwork.ozlabs.org/patch/541068/ https://patchwork.ozlabs.org/patch/542424/ Tom, Simon, Are you guys ok with board/$(VENDOR)/include? -- Regards, Nishanth Menon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On 11/14/2015 05:56 PM, Masahiro Yamada wrote: > 2015-11-13 14:43 GMT+09:00 Nishanth Menon <nm@ti.com>: >> Header files can be located in a generic location without >> needing to reference them with ../common/ >> >> Generated with the following script >> >> #!/bin/bash >> vendor=board/LaCie >> common=$vendor/common >> >> cfiles=`git grep "../common" $vendor|grep "#include"|cut -d '"' -f2|sort -u|grep c$` >> headers=`git grep "../common" $vendor|grep "#include"|cut -d '"' -f2|sort -u|grep h$` >> >> mkdir -p $common/include/board-common >> set -x >> for header in $headers >> do >> echo "processing $header in $common" >> hbase=`basename $header` >> git mv $common/$hbase $common/include/board-common >> sed -i -e "s/\"..\/common\/$hbase\"/<board-common\/$hbase>/g" $vendor/*/*.[chS] >> sed -i -e "s/\"$hbase\"/<board-common\/$hbase>/g" $vendor/common/*.[chS] >> done >> >> Cc: Simon Guinot <simon.guinot@sequanux.org> >> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> >> >> Signed-off-by: Nishanth Menon <nm@ti.com> >> --- > > > As far as I understood from 02 to 12, > the effect of this series is: > > either > replace "../common/foo.h" with <board-common/foo.h> for board/specific board files. > or > replace "bar.h" with <board-common/bar.h> yes - for board/common headers which are exposed. > > Vendor common headers are referenced within their own directory. > #include "..." is better than #include <...> in such cases. Not after this series, which is what is the 3rd change done by this series: The headers are moved to a common location away from the board/common directory. This is more inline with what you did with mach. > I still do not understand what problem this series wants to solve. standardize board common header inclusion strategy across boards in a consistent manner similar to what mach/ changes have been doing. Overall, you did mention in https://patchwork.ozlabs.org/patch/541068/ [step 1] move SoC-specific headers to arch/<arch>/mach-<soc>/include/mach [step 2] change #include <asm/arch/foo.h> to #include <mach/foo.h> Why did we not let folks user relative includes such as #include "../../mach/xyz.h" ? because it constraints us from changing the directory architecture in the future. This is exactly the same problem that board/<vendor>/ folders have. Why is it that you dont see that as a problem? -- Regards, Nishanth Menon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On 11/15/2015 09:32 PM, Masahiro Yamada wrote: > 2015-11-15 14:38 GMT+09:00 Nishanth Menon <nm@ti.com>: >> On 11/14/2015 05:56 PM, Masahiro Yamada wrote: >>> 2015-11-13 14:43 GMT+09:00 Nishanth Menon <nm@ti.com>: >>>> Header files can be located in a generic location without >>>> needing to reference them with ../common/ >>>> >>>> Generated with the following script >>>> >>>> #!/bin/bash >>>> vendor=board/LaCie >>>> common=$vendor/common >>>> >>>> cfiles=`git grep "../common" $vendor|grep "#include"|cut -d '"' -f2|sort -u|grep c$` >>>> headers=`git grep "../common" $vendor|grep "#include"|cut -d '"' -f2|sort -u|grep h$` >>>> >>>> mkdir -p $common/include/board-common >>>> set -x >>>> for header in $headers >>>> do >>>> echo "processing $header in $common" >>>> hbase=`basename $header` >>>> git mv $common/$hbase $common/include/board-common >>>> sed -i -e "s/\"..\/common\/$hbase\"/<board-common\/$hbase>/g" $vendor/*/*.[chS] >>>> sed -i -e "s/\"$hbase\"/<board-common\/$hbase>/g" $vendor/common/*.[chS] >>>> done >>>> >>>> Cc: Simon Guinot <simon.guinot@sequanux.org> >>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> >>>> >>>> Signed-off-by: Nishanth Menon <nm@ti.com> >>>> --- >>> >>> >>> As far as I understood from 02 to 12, >>> the effect of this series is: >>> >>> either >>> replace "../common/foo.h" with <board-common/foo.h> >> for board/specific board files. >> >>> or >>> replace "bar.h" with <board-common/bar.h> >> yes - for board/common headers which are exposed. >> >>> >>> Vendor common headers are referenced within their own directory. >>> #include "..." is better than #include <...> in such cases. >> >> Not after this series, which is what is the 3rd change done by this >> series: The headers are moved to a common location away from the >> board/common directory. >> >> This is more inline with what you did with mach. >> >>> I still do not understand what problem this series wants to solve. >> >> standardize board common header inclusion strategy across boards in a >> consistent manner similar to what mach/ changes have been doing. >> >> Overall, you did mention in https://patchwork.ozlabs.org/patch/541068/ >> >> >> [step 1] move SoC-specific headers to arch/<arch>/mach-<soc>/include/mach >> >> [step 2] change #include <asm/arch/foo.h> to #include <mach/foo.h> >> >> >> >> Why did we not let folks user relative includes such as #include >> "../../mach/xyz.h" ? because it constraints us from changing the >> directory architecture in the future. >> >> This is exactly the same problem that board/<vendor>/ folders have. >> >> >> Why is it that you dont see that as a problem? >> > > > You are misunderstanding. > > SoC headers (either <asm/arch/*.h> in old style, or <mach/*.h> in new > style) are exported. > > > For example, arch/asm/include/asm/gpio.h includes <asm/arch/gpio.h>. > Also, many files under drivers/ include <asm/arch/*.h> > > I do not think any drivers should depend on SoC specific headers, > but it is the place where we stand now. > > > OTOH, vendor headers should be only referenced locally. > We should not expand the scope. No need to standardize the include path. > ^^^ > > Relative path is a correct way to include a header file with local scope. > > Even Linux does so. > > For example > > drivers/pinctrl/sunxi/pinctrl-sunxi.c > Hmm... so, lets review our status currently: a) board/$(VENDOR)/board_X, board/$(VENDOR)/board_Y, board/$(VENDOR)/board_Z all need a common function, this is currently in board/$(VENDOR)/common/xyz.c. b) the function prototype for the function needs to be in xyz.h so that board/$(VENDOR)/board_[XYZ] can use the function. c) your suggestion is to stay in local scope for board/$(VENDOR)/board_[XYZ] by "../common/xyz.h" So much I have understood. I dont understand *why* you feel that vendor headers should only be referenced locally. Could you elaborate a little more on that? Is it because of the risk that drivers will now be able to do <board-common/xyz.h> ? If that is the case, and Tom, Simon, you folks agree as well, I can drop the entire series. -- Regards, Nishanth Menon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
diff --git a/board/LaCie/common/cpld-gpio-bus.c b/board/LaCie/common/cpld-gpio-bus.c index 9b24dc535c04..92a80243c5e0 100644 --- a/board/LaCie/common/cpld-gpio-bus.c +++ b/board/LaCie/common/cpld-gpio-bus.c @@ -13,7 +13,7 @@ */ #include <asm/arch/gpio.h> -#include "cpld-gpio-bus.h" +#include <board-common/cpld-gpio-bus.h> static void cpld_gpio_bus_set_addr(struct cpld_gpio_bus *bus, unsigned addr) { diff --git a/board/LaCie/common/common.h b/board/LaCie/common/include/board-common/common.h similarity index 100% rename from board/LaCie/common/common.h rename to board/LaCie/common/include/board-common/common.h diff --git a/board/LaCie/common/cpld-gpio-bus.h b/board/LaCie/common/include/board-common/cpld-gpio-bus.h similarity index 100% rename from board/LaCie/common/cpld-gpio-bus.h rename to board/LaCie/common/include/board-common/cpld-gpio-bus.h diff --git a/board/LaCie/edminiv2/edminiv2.c b/board/LaCie/edminiv2/edminiv2.c index edf6281797bf..66d0e8502256 100644 --- a/board/LaCie/edminiv2/edminiv2.c +++ b/board/LaCie/edminiv2/edminiv2.c @@ -11,7 +11,7 @@ #include <common.h> #include <miiphy.h> #include <asm/arch/orion5x.h> -#include "../common/common.h" +#include <board-common/common.h> #include <spl.h> #include <ns16550.h> diff --git a/board/LaCie/net2big_v2/net2big_v2.c b/board/LaCie/net2big_v2/net2big_v2.c index 263bb5426c0d..0bfe76fde334 100644 --- a/board/LaCie/net2big_v2/net2big_v2.c +++ b/board/LaCie/net2big_v2/net2big_v2.c @@ -18,8 +18,8 @@ #include <asm/arch/gpio.h> #include "net2big_v2.h" -#include "../common/common.h" -#include "../common/cpld-gpio-bus.h" +#include <board-common/common.h> +#include <board-common/cpld-gpio-bus.h> DECLARE_GLOBAL_DATA_PTR; diff --git a/board/LaCie/netspace_v2/netspace_v2.c b/board/LaCie/netspace_v2/netspace_v2.c index 17e629622ff7..4ea76d152e6b 100644 --- a/board/LaCie/netspace_v2/netspace_v2.c +++ b/board/LaCie/netspace_v2/netspace_v2.c @@ -17,7 +17,7 @@ #include <asm/arch/gpio.h> #include "netspace_v2.h" -#include "../common/common.h" +#include <board-common/common.h> DECLARE_GLOBAL_DATA_PTR;
Header files can be located in a generic location without needing to reference them with ../common/ Generated with the following script #!/bin/bash vendor=board/LaCie common=$vendor/common cfiles=`git grep "../common" $vendor|grep "#include"|cut -d '"' -f2|sort -u|grep c$` headers=`git grep "../common" $vendor|grep "#include"|cut -d '"' -f2|sort -u|grep h$` mkdir -p $common/include/board-common set -x for header in $headers do echo "processing $header in $common" hbase=`basename $header` git mv $common/$hbase $common/include/board-common sed -i -e "s/\"..\/common\/$hbase\"/<board-common\/$hbase>/g" $vendor/*/*.[chS] sed -i -e "s/\"$hbase\"/<board-common\/$hbase>/g" $vendor/common/*.[chS] done Cc: Simon Guinot <simon.guinot@sequanux.org> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> Signed-off-by: Nishanth Menon <nm@ti.com> --- board/LaCie/common/cpld-gpio-bus.c | 2 +- board/LaCie/common/{ => include/board-common}/common.h | 0 board/LaCie/common/{ => include/board-common}/cpld-gpio-bus.h | 0 board/LaCie/edminiv2/edminiv2.c | 2 +- board/LaCie/net2big_v2/net2big_v2.c | 4 ++-- board/LaCie/netspace_v2/netspace_v2.c | 2 +- 6 files changed, 5 insertions(+), 5 deletions(-) rename board/LaCie/common/{ => include/board-common}/common.h (100%) rename board/LaCie/common/{ => include/board-common}/cpld-gpio-bus.h (100%) -- 2.6.2.402.g2635c2b _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot