From mboxrd@z Thu Jan  1 00:00:00 1970
From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Date: Sun, 23 Dec 2012 13:15:08 +0100
Subject: [U-Boot] [PATCH 1/1] OMAP3: add boot status GPIO LED for IGEP
 boards
In-Reply-To: <50D6B9E4.3060309@compulab.co.il>
References: <1356140950-14209-1-git-send-email-javier.martinez@collabora.co.uk>
	<50D6B9E4.3060309@compulab.co.il>
Message-ID: <50D6F5CC.2020002@collabora.co.uk>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 12/23/2012 08:59 AM, Igor Grinberg wrote:
> On 12/22/12 03:49, Javier Martinez Canillas wrote:
>> This patch adds an GPIO LED boot status for IGEP boards.
>> 

Hi Igor, thanks a lot for your comments

>> @@ -52,9 +55,15 @@ static const u32 gpmc_lan_config[] = {
>>  int board_init(void)
>>  {
>>  	gpmc_init(); /* in SRAM or SDRAM, finish GPMC */
>> +	/* machine type for kernel */
>> +	gd->bd->bi_arch_number = MACH_TYPE_IGEP0020;
> 
> Please, use CONFIG_MACH_TYPE instead.
> 
> [...]
> 

Yes, I was just setting this to have some state that could be used latter on
board/isee/led.c but now I realized that bi_arch_number is set in
arch/arm/lib/board.c

gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */

So, I'm just going to remove this assignment

>> diff --git a/board/isee/igep0030/igep0030.c b/board/isee/igep0030/igep0030.c
>> index 107cb7f..394a6cf 100644
>> --- a/board/isee/igep0030/igep0030.c
>> +++ b/board/isee/igep0030/igep0030.c
> 
> [...]
> 
>> @@ -39,9 +42,15 @@ DECLARE_GLOBAL_DATA_PTR;
>>  int board_init(void)
>>  {
>>  	gpmc_init(); /* in SRAM or SDRAM, finish GPMC */
>> +	/* machine type for kernel */
>> +	gd->bd->bi_arch_number = MACH_TYPE_IGEP0030;
> 
> same here
> 

ditto

> [...]
> 
>> diff --git a/board/isee/led.c b/board/isee/led.c
>> new file mode 100644
>> index 0000000..b7a9a02
>> --- /dev/null
>> +++ b/board/isee/led.c
> 
> [...]
> 
>> +
>> +/* GPIO pins for the LED */
>> +#define IGEP0020_GPIO_LED    27
>> +#define IGEP0030_GPIO_LED    16
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +static int __get_gpio()
>> +{
>> +	if (gd->bd->bi_arch_number == MACH_TYPE_IGEP0020)
>> +		return IGEP0020_GPIO_LED;
>> +	else
>> +		return IGEP0030_GPIO_LED;
>> +}
> 
> Once you define the CONFIG_MACH_TYPE in your board config file,
> you can then just do something like:
> 
> #if (CONFIG_MACH_TYPE == MACH_TYPE_IGEP0020)
> #define IGEP_GPIO_LED	27
> #else /* MACH_TYPE_IGEP0030 */
> #define IGEP_GPIO_LED	16
> #endif
> 
> remove the above function completely and just use IGEP_GPIO_LED define.
> This will also probably simplify the below code.
> What do you think?
> 

Agreed, your approach is much better and simpler.

Will send a v2 with your suggestions,

Thanks a lot and best regards,
Javier