From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nishanth Menon Date: Fri, 13 Nov 2015 17:57:17 -0600 Subject: [U-Boot] [PATCH V2 07/12] board: LaCie: Move common headers to board-common directory In-Reply-To: <20151113153209.GU4665@kw.sim.vm.gnt> References: <1447393422-4169-1-git-send-email-nm@ti.com> <1447393422-4169-8-git-send-email-nm@ti.com> <20151113103043.GT4665@kw.sim.vm.gnt> <20151113140645.GK8060@bill-the-cat> <20151113153209.GU4665@kw.sim.vm.gnt> Message-ID: <564678DD.6070100@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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\"//g" $vendor/*/*.[chS] >>>> sed -i -e "s/\"$hbase\"//g" $vendor/common/*.[chS] >>>> done >>>> >>>> Cc: Simon Guinot >>>> Cc: Albert ARIBAUD >>>> >>>> Signed-off-by: Nishanth Menon >>>> --- >>>> 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 > > 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