From mboxrd@z Thu Jan  1 00:00:00 1970
From: Daniel Gorsulowski <Daniel.Gorsulowski@esd.eu>
Date: Fri, 08 May 2009 07:28:09 +0200
Subject: [U-Boot] [PATCH 1/1] at91: Add command to control up to 3 GPIO
 LEDs from the console
In-Reply-To: <200905070832.01278.sr@denx.de>
References: <1241619669338-git-send-email-Daniel.Gorsulowski@esd.eu>
	<200905070832.01278.sr@denx.de>
Message-ID: <4A03C2E9.1010501@esd.eu>
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

Hi Stefan,

Stefan Roese wrote:
> Hi Daniel,
> 
> On Wednesday 06 May 2009, Daniel Gorsulowski wrote:
>> This patch allows any at91 board, implementing the GPIO LED API,
>> to control the LEDs from the console.
>>
>> led [ 1 | 2 | 3 | all ]  [ on | off ]
> 
> Why limit this to a max of 3 LED's? If this is a generic command (which I like 
> btw) then we should support a user/board defined number of LED's. In your case 
> it's 3, but the infrastructure should support any number.
> 
> More comments below.
> 
> <snip>
> 
...
> 
> I suggest to use something like this here:
> 
> 	led_nr = simple_strtoul(argv[1], NULL, 10);
> 	if (led_nr > CONFIG_LED_MAX) {
> 		printf ("Usage:\n%s\n", cmdtp->usage);
> 		return 1;
> 	}
> 
> 	if (strcmp(argv[2], "off") == 0) {
> 		on = 1;
> 	} else if (strcmp(argv[2], "on") == 0) {
> 		on = 0;
> 	} else {
> 		printf ("Usage:\n%s\n", cmdtp->usage);
> 		return 1;
> 	}
> 
> 	user_led(led_nr, on);
> 
> No ugly #ifdef's in this case. What do you think?
> 
> Best regards,
> Stefan
> 
I agree with you.
Please give me some days, to implement your basic approaches.
I've many other things to do and it's not that easy (for me)
to create a tidy patch.

Best regards,
Daniel