[U-Boot] [PATCH V2 07/12] board: LaCie: Move common headers to board-common directory

[Tom Rini]

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 at ti.com>:
> >> On 11/14/2015 05:56 PM, Masahiro Yamada wrote:
> >>> 2015-11-13 14:43 GMT+09:00 Nishanth Menon <nm at 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 at sequanux.org>
> >>>> Cc: Albert ARIBAUD <albert.u.boot at aribaud.net>
> >>>>
> >>>> Signed-off-by: Nishanth Menon <nm at 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151116/5bfb2637/attachment.sig>