[V2,07/12] board: LaCie: Move common headers to board-common directory

Message ID 1447393422-4169-8-git-send-email-nm@ti.com
State New
Headers show

Commit Message

Nishanth Menon Nov. 13, 2015, 5:43 a.m.
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@sequanux.org>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>

Signed-off-by: Nishanth Menon <nm@ti.com>

---
 board/LaCie/common/cpld-gpio-bus.c                            | 2 +-
 board/LaCie/common/{ => include/board-common}/common.h        | 0
 board/LaCie/common/{ => include/board-common}/cpld-gpio-bus.h | 0
 board/LaCie/edminiv2/edminiv2.c                               | 2 +-
 board/LaCie/net2big_v2/net2big_v2.c                           | 4 ++--
 board/LaCie/netspace_v2/netspace_v2.c                         | 2 +-
 6 files changed, 5 insertions(+), 5 deletions(-)
 rename board/LaCie/common/{ => include/board-common}/common.h (100%)
 rename board/LaCie/common/{ => include/board-common}/cpld-gpio-bus.h (100%)

-- 
2.6.2.402.g2635c2b
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Comments

Nishanth Menon Nov. 13, 2015, 11:57 p.m. | #1
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@sequanux.org>

>>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>

>>>>

>>>> Signed-off-by: Nishanth Menon <nm@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Nishanth Menon Nov. 15, 2015, 5:38 a.m. | #2
On 11/14/2015 05:56 PM, Masahiro Yamada wrote:
> 2015-11-13 14:43 GMT+09:00 Nishanth Menon <nm@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@sequanux.org>

>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>

>>

>> Signed-off-by: Nishanth Menon <nm@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?


-- 
Regards,
Nishanth Menon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Nishanth Menon Nov. 16, 2015, 3:34 p.m. | #3
On 11/15/2015 09:32 PM, Masahiro Yamada wrote:
> 2015-11-15 14:38 GMT+09:00 Nishanth Menon <nm@ti.com>:

>> On 11/14/2015 05:56 PM, Masahiro Yamada wrote:

>>> 2015-11-13 14:43 GMT+09:00 Nishanth Menon <nm@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@sequanux.org>

>>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>

>>>>

>>>> Signed-off-by: Nishanth Menon <nm@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.

-- 
Regards,
Nishanth Menon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Patch

diff --git a/board/LaCie/common/cpld-gpio-bus.c b/board/LaCie/common/cpld-gpio-bus.c
index 9b24dc535c04..92a80243c5e0 100644
--- a/board/LaCie/common/cpld-gpio-bus.c
+++ b/board/LaCie/common/cpld-gpio-bus.c
@@ -13,7 +13,7 @@ 
  */
 
 #include <asm/arch/gpio.h>
-#include "cpld-gpio-bus.h"
+#include <board-common/cpld-gpio-bus.h>
 
 static void cpld_gpio_bus_set_addr(struct cpld_gpio_bus *bus, unsigned addr)
 {
diff --git a/board/LaCie/common/common.h b/board/LaCie/common/include/board-common/common.h
similarity index 100%
rename from board/LaCie/common/common.h
rename to board/LaCie/common/include/board-common/common.h
diff --git a/board/LaCie/common/cpld-gpio-bus.h b/board/LaCie/common/include/board-common/cpld-gpio-bus.h
similarity index 100%
rename from board/LaCie/common/cpld-gpio-bus.h
rename to board/LaCie/common/include/board-common/cpld-gpio-bus.h
diff --git a/board/LaCie/edminiv2/edminiv2.c b/board/LaCie/edminiv2/edminiv2.c
index edf6281797bf..66d0e8502256 100644
--- a/board/LaCie/edminiv2/edminiv2.c
+++ b/board/LaCie/edminiv2/edminiv2.c
@@ -11,7 +11,7 @@ 
 #include <common.h>
 #include <miiphy.h>
 #include <asm/arch/orion5x.h>
-#include "../common/common.h"
+#include <board-common/common.h>
 #include <spl.h>
 #include <ns16550.h>
 
diff --git a/board/LaCie/net2big_v2/net2big_v2.c b/board/LaCie/net2big_v2/net2big_v2.c
index 263bb5426c0d..0bfe76fde334 100644
--- a/board/LaCie/net2big_v2/net2big_v2.c
+++ b/board/LaCie/net2big_v2/net2big_v2.c
@@ -18,8 +18,8 @@ 
 #include <asm/arch/gpio.h>
 
 #include "net2big_v2.h"
-#include "../common/common.h"
-#include "../common/cpld-gpio-bus.h"
+#include <board-common/common.h>
+#include <board-common/cpld-gpio-bus.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
diff --git a/board/LaCie/netspace_v2/netspace_v2.c b/board/LaCie/netspace_v2/netspace_v2.c
index 17e629622ff7..4ea76d152e6b 100644
--- a/board/LaCie/netspace_v2/netspace_v2.c
+++ b/board/LaCie/netspace_v2/netspace_v2.c
@@ -17,7 +17,7 @@ 
 #include <asm/arch/gpio.h>
 
 #include "netspace_v2.h"
-#include "../common/common.h"
+#include <board-common/common.h>
 
 DECLARE_GLOBAL_DATA_PTR;