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

[Tom Rini]

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 at 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 at sequanux.org>
> >>>>> Cc: Albert ARIBAUD <albert.u.boot at aribaud.net>
> >>>>>
> >>>>> Signed-off-by: Nishanth Menon <nm at 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.

-- 
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/20151115/80a42c48/attachment.sig>