| Message ID | 1304417333-30745-1-git-send-email-grinberg@compulab.co.il |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Igor Grinberg, In message <1304417333-30745-1-git-send-email-grinberg@compulab.co.il> you wrote: > This board used machine_is_* macros for identifying the arch number. > Fix this by introducing a board specific configuration variable. > > Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> > --- > v2: remove the ifdeferry by introducing config variable, > Alessandro, what about this one? ... > + gd->bd->bi_arch_number = CONFIG_BOARD_MACH_TYPE; In principle this is OK, but why do you invent yet another new CONFIG_ variable (and without documenting it) ? We have a number of boards that already use a similar construct with CONFIG_MACH_TYPE, so I suggest you do the same. And while being there, could you please also add a description for CONFIG_MACH_TYPE to the README? Thanks! Best regards, Wolfgang Denk
Hi Wolfgang, On 05/03/11 15:29, Wolfgang Denk wrote: > Dear Igor Grinberg, > > In message <1304417333-30745-1-git-send-email-grinberg@compulab.co.il> you wrote: >> This board used machine_is_* macros for identifying the arch number. >> Fix this by introducing a board specific configuration variable. >> >> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> >> --- >> v2: remove the ifdeferry by introducing config variable, >> Alessandro, what about this one? > ... >> + gd->bd->bi_arch_number = CONFIG_BOARD_MACH_TYPE; > In principle this is OK, but why do you invent yet another new CONFIG_ > variable (and without documenting it) ? Well, it was meant to be board specific, so local documentation in config file should be enough. > We have a number of boards that already use a similar construct with > CONFIG_MACH_TYPE, so I suggest you do the same. Didn't know that, I though it is new... silly me... ;) Thanks for pointing. > And while being there, could you please also add a description for > CONFIG_MACH_TYPE to the README? Thanks! I'll try my best, but this will take a while... Other patches in this series are not affected by this one, so can be easily applied.
diff --git a/board/ti/omap1610inn/omap1610innovator.c b/board/ti/omap1610inn/omap1610innovator.c index 44818bb..a071f63 100644 --- a/board/ti/omap1610inn/omap1610innovator.c +++ b/board/ti/omap1610inn/omap1610innovator.c @@ -63,12 +63,7 @@ static inline void delay (unsigned long loops) int board_init (void) { - if (machine_is_omap_h2()) - gd->bd->bi_arch_number = MACH_TYPE_OMAP_H2; - else if (machine_is_omap_innovator()) - gd->bd->bi_arch_number = MACH_TYPE_OMAP_INNOVATOR; - else - gd->bd->bi_arch_number = MACH_TYPE_OMAP_GENERIC; + gd->bd->bi_arch_number = CONFIG_BOARD_MACH_TYPE; /* adress of boot parameters */ gd->bd->bi_boot_params = 0x10000100; diff --git a/include/configs/omap1610h2.h b/include/configs/omap1610h2.h index 7e53ae6..0770d9d 100644 --- a/include/configs/omap1610h2.h +++ b/include/configs/omap1610h2.h @@ -34,7 +34,9 @@ #define CONFIG_OMAP 1 /* in a TI OMAP core */ #define CONFIG_OMAP1610 1 /* which is in a 1610 */ #define CONFIG_H2_OMAP1610 1 /* on an H2 Board */ -#define CONFIG_MACH_OMAP_H2 /* Select board mach-type */ + +/* Select board mach-type */ +#define CONFIG_BOARD_MACH_TYPE MACH_TYPE_OMAP_H2 /* input clock of PLL */ /* the OMAP1610 H2 has 12MHz input clock */ diff --git a/include/configs/omap1610inn.h b/include/configs/omap1610inn.h index be569a3..1859780 100644 --- a/include/configs/omap1610inn.h +++ b/include/configs/omap1610inn.h @@ -34,7 +34,9 @@ #define CONFIG_OMAP 1 /* in a TI OMAP core */ #define CONFIG_OMAP1610 1 /* which is in a 1610 */ #define CONFIG_INNOVATOROMAP1610 1 /* a Innovator Board */ -#define CONFIG_MACH_OMAP_INNOVATOR /* Select board mach-type */ + +/* Select board mach-type */ +#define CONFIG_BOARD_MACH_TYPE MACH_TYPE_OMAP_INNOVATOR /* input clock of PLL */ /* the OMAP1610 Innovator has 12MHz input clock */
This board used machine_is_* macros for identifying the arch number. Fix this by introducing a board specific configuration variable. Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> --- v2: remove the ifdeferry by introducing config variable, Alessandro, what about this one? board/ti/omap1610inn/omap1610innovator.c | 7 +------ include/configs/omap1610h2.h | 4 +++- include/configs/omap1610inn.h | 4 +++- 3 files changed, 7 insertions(+), 8 deletions(-)