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

[Simon Glass]

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.

- Simon