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

[Nishanth Menon]

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?

-- 
Regards,
Nishanth Menon