Hi,
On Wed, Dec 14, 2011 at 11:38 AM, Ulf Samuelsson ulf_samuelsson@telia.com wrote:
On 2011-12-14 20:11, Simon Glass wrote:
Hi,
On Tue, Dec 13, 2011 at 3:55 PM, Ulf Samuelsson ulf_samuelsson@telia.com wrote:
On 2010-11-05 13:21, Wolfgang Denk wrote:
Dear Jason Kridner,
In message1288936236-30603-1-git-send-email-jkridner@beagleboard.org you wrote:
It is desired to have the led command on the BeagleBoard to allow for some interaction in the scripts.
This patch allows any board implementing the coloured LED API to control the LEDs from the console.
led [green | yellow | red | all ] [ on | off ]
or
led [ 1 | 2 | 3 | all ] [ on | off ]
Adds configuration item CONFIG_CMD_LED enabling the command.
Partially based on patch from Ulf Samuelsson: http://www.mail-archive.com/u-boot@lists.denx.de/msg09593.html.
Signed-off-by: Jason Kridnerjkridner@beagleboard.org
common/Makefile | 1 + common/cmd_led.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 0 deletions(-) create mode 100644 common/cmd_led.c
I understand the requirement, but I think it is more than time to come up with a common solution here instead of adding more and more copies of very similar code.
We already have:
arch/arm/cpu/arm920t/ep93xx/led.c
The file below is mapping the led commands I.E: red_led_on to at91 I/O calls like at91_set_gpio_value. A merge, means that a common way of setting GPIO must be available
arch/arm/cpu/arm926ejs/at91/led.c
The only thing the files below do are to initialize the pins for the LED. While the actual pins are hidden in the configs, you have a variation of which LEDs are available. Also you need to enable different clocks. It is really an extension of the board init file. Maybe they should be merged with the board file instead. I personally don't see a problem with keeping the separate file.
There is no commonality between these files and the led.c in "arch/arm/cpu/arm926ejs/at91/led.c"
board/atmel/at91cap9adk/led.c board/atmel/at91rm9200dk/led.c board/atmel/at91rm9200ek/led.c board/atmel/at91sam9260ek/led.c board/atmel/at91sam9261ek/led.c board/atmel/at91sam9263ek/led.c board/atmel/at91sam9m10g45ek/led.c board/atmel/at91sam9rlek/led.c board/ronetix/pm9261/led.c board/ronetix/pm9263/led.c board/eukrea/cpu9260/led.c
board/logicpd/zoom2/led.c board/ns9750dev/led.c board/psyent/pk1c20/led.c
Please, let's unify these.
Best regards,
Wolfgang Denk
The proposed patch is adding a command, and which uses the coloured LED interface and there is no commonality between this code and the code in the board and cpu directories.
IMO this LED interface is not very nice. Is there a reason there is not just one function? Perhaps like:
enum led_colour { LED_GREEN, LED_YELLOW, LED_RED, LED_BLUE, };
void led_init(void);
/** * Set the state of a coloured LED * * @param led LED to adjust * @param on 1 to turn it on, 0 to turn it off */ void led_set_state(enum led_colour led, int on);
instead of:
extern void coloured_LED_init (void); extern void red_LED_on(void); extern void red_LED_off(void); extern void green_LED_on(void); extern void green_LED_off(void); extern void yellow_LED_on(void); extern void yellow_LED_off(void); extern void blue_LED_on(void); extern void blue_LED_off(void);
and associated weak symbols.
It may even be possible to tidy up the existing status_led.h file at the same time.
The reason is simple. The API was developed to figure out why u-boot never reached the prompt. The API should be so simple to use in assembler that it did not introduce further bugs.
In assembler the call maps to something like (IIRC): bl green_LED_on
Much easier than passing parameters in registers.
Yes I see what you mean
mov r0, #LED_GREEN mov r1, #1 bl led_set_state
presumably you have r0 and r1 used for other things. Still it seems painful to subject board.c to this very assembler-specific API. Perhaps the current API could be relegated to a driver somewhere.
There are other advantages of doing it your way of course.
If you want to change, make sure you don't break any boards.
I am cleaning this up as part of the new board.c effort, waiting on some responses on relocation still.
Regards, Simon
BR Ulf Samuelsson
I agree that the command is sort-of sideways, but really I think this should be cleaned up before adding more code that uses the API. It just makes the job harder.
Regards, Simon
-- Best Regards Ulf Samuelsson
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
-- Best Regards Ulf Samuelsson +46 722 427437