| Message ID | 1447393422-4169-8-git-send-email-nm@ti.com |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
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. Thanks, Simon > 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%) > > 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; > > -- > 2.6.2.402.g2635c2b
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!
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. 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 Anyway, IMHO things are fine as they are. But if anyone thinks this change is needed or valuable, then I am OK with it. Thanks, Simon
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?
Hi Nishanth, On 13 November 2015 at 16:57, Nishanth Menon <nm@ti.com> wrote: > 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? Yes. - Simon
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> or replace "bar.h" with <board-common/bar.h> Vendor common headers are referenced within their own directory. #include "..." is better than #include <...> in such cases. I still do not understand what problem this series wants to solve.
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?
On Fri, Nov 13, 2015 at 07:05:40PM -0700, Simon Glass wrote: > Hi Nishanth, > > On 13 November 2015 at 16:57, Nishanth Menon <nm@ti.com> wrote: > > 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? > > Yes. Also Yes.
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
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.
On Mon, Nov 16, 2015 at 09:34:07AM -0600, Nishanth Menon wrote: > 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. So I think I see the point that Masahiro (and others) are making and it makes sense, sorry for not seeing it earlier myself. Since we have many matches on <board-common/foo.h> but no functions in common (or even common function names but with different prototypes) we really aren't gaining anything by doing this kind of change. So yes, we should just reference this locally since it is local to board/$(VENDOR). Thanks for taking the time to do this Nishanth but NAK.
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%)