Re: [U-Boot] [RFC] Add 'led' command
I think that before we go ahead and do changes, we should ask ourselves:
Why does a bootloader need LED support?
Just because its on the board?
Do we need to support all LEDs because they are there?
i implemented the LED command because it seemed to have some application,
even though I could not think of any specific use at the time.
Would be curious to find out what people would use the LED function for, except
for debugging.
Before that is known, it is hard to Judge if a change is warranted.
Admit that using green_LED_on for my second red LED on the bord
has some drawbacks, but how do you make a generalized command.
led 1 on sucks as a user interface.
led red on is much much better.
Implementing a change which results in broken boards support is a no no.
IMHO it also has very little incremental value to do the modifications.
Best Regards
Ulf Samuelsson
ulf_samuels...@telia.com
+46 (722) 427 437
15 dec 2011 kl. 19:17 skrev Jason Kridner jkrid...@beagleboard.org:
I hope I'm not too far off from understanding the comments made at the
end of this thread...
On Wed, Dec 14, 2011 at 4:31 PM, Simon Glass s...@chromium.org wrote:
Hi,
On Wed, Dec 14, 2011 at 11:38 AM, Ulf Samuelsson
ulf_samuels...@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_samuels...@telia.com wrote:
On 2010-11-05 13:21, Wolfgang Denk wrote:
Dear Jason Kridner,
In message1288936236-30603-1-git-send-email-jkrid...@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 Kridnerjkrid...@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 on1 to turn it on, 0 to turn it off
*/
void led_set_state(enum led_colour led, int on);
The concept of selecting an LED by color seems broken to me. Boards
may have many LEDs of the same color.
There is value in having additional meta-data regarding an LED, such
as its color, location, silk-screen text or anticipated use. It seems
to me all of that should be ancillary to the index of the LED. The
information to control the LED, such as the GPIO pin or associated
register, could further be stored in the data structure associated
with an LED index.
The only reason I implemented a function with the existing API was
that the API was established across
Re: [U-Boot] [RFC] Add 'led' command
I hope I'm not too far off from understanding the comments made at the
end of this thread...
On Wed, Dec 14, 2011 at 4:31 PM, Simon Glass s...@chromium.org wrote:
Hi,
On Wed, Dec 14, 2011 at 11:38 AM, Ulf Samuelsson
ulf_samuels...@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_samuels...@telia.com wrote:
On 2010-11-05 13:21, Wolfgang Denk wrote:
Dear Jason Kridner,
In message1288936236-30603-1-git-send-email-jkrid...@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 Kridnerjkrid...@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);
The concept of selecting an LED by color seems broken to me. Boards
may have many LEDs of the same color.
There is value in having additional meta-data regarding an LED, such
as its color, location, silk-screen text or anticipated use. It seems
to me all of that should be ancillary to the index of the LED. The
information to control the LED, such as the GPIO pin or associated
register, could further be stored in the data structure associated
with an LED index.
The only reason I implemented a function with the existing API was
that the API was established across several platforms. Trying to find
time to dedicate to cleaning up those several implementations without
having any of the hardware (meaning lots of communications to other
folks rather than just making the code work) is difficult, but I'm
happy to contribute code fragments/patches if there is interest.
My approach would be to provide an array of structures to the command
that is initialized by the board file that contains the meta data for
the LED and pointers to the functions that perform the required
manipulation. Default functions would be the platform GPIO functions.
The array would also have the required arguments for those
manipulation functions, ie. the pin numbers to pass to the GPIO
functions.
The base definition is the easy part. The hard part is following up
with all of the platforms and still making sure that the super-simple
assembly functions can work efficiently.
instead of:
extern void coloured_LED_init (void);
extern void red_LED_on(void);
extern void red_LED_off(void);
extern void
Re: [U-Boot] [RFC] Add 'led' command
Hi,
On Tue, Dec 13, 2011 at 3:55 PM, Ulf Samuelsson
ulf_samuels...@telia.com wrote:
On 2010-11-05 13:21, Wolfgang Denk wrote:
Dear Jason Kridner,
In message1288936236-30603-1-git-send-email-jkrid...@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 Kridnerjkrid...@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 on1 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.
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] Add 'led' command
Hi,
On Wed, Dec 14, 2011 at 11:38 AM, Ulf Samuelsson
ulf_samuels...@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_samuels...@telia.com wrote:
On 2010-11-05 13:21, Wolfgang Denk wrote:
Dear Jason Kridner,
In message1288936236-30603-1-git-send-email-jkrid...@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 Kridnerjkrid...@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
Re: [U-Boot] [RFC] Add 'led' command
On 2010-11-05 13:21, Wolfgang Denk wrote: Dear Jason Kridner, In message1288936236-30603-1-git-send-email-jkrid...@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 Kridnerjkrid...@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. -- Best Regards Ulf Samuelsson ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] Add 'led' command
On Saturday, November 13, 2010 18:31:39 Mike Frysinger wrote:
On Friday, November 12, 2010 09:42:52 Jason Kridner wrote:
On Tue, Nov 9, 2010 at 8:52 AM, Mike Frysinger wrote:
On Friday, November 05, 2010 01:50:36 Jason Kridner wrote:
+ if (strcmp(argv[2], off) == 0) {
+ state = 0;
+ } else if (strcmp(argv[2], on) == 0) {
+ state = 1;
i could have sworn we had a helper somewhere to handle boolean
strings ...
common/cmd_cache.c has an internal on_off function. All other places
seem to do individual strcmp. Let me know if you find such a helper.
Is there value to putting this in a function like the one in
cmd_cache.c?
i think there's value in moving this to generalizing and moving to common
code. there are other places where we handle env vars which could have the
value on or off.
ah, i found what i was thinking of. there is a getenv_yesno helper.
obviously not the same as on/off, but should be a good model for a new
str_onoff helper.
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] Add 'led' command
On Friday, November 12, 2010 09:42:52 Jason Kridner wrote:
On Tue, Nov 9, 2010 at 8:52 AM, Mike Frysinger vap...@gentoo.org wrote:
On Friday, November 05, 2010 01:50:36 Jason Kridner wrote:
+ if (strcmp(argv[2], off) == 0) {
+ state = 0;
+ } else if (strcmp(argv[2], on) == 0) {
+ state = 1;
i could have sworn we had a helper somewhere to handle boolean strings
...
common/cmd_cache.c has an internal on_off function. All other places
seem to do individual strcmp. Let me know if you find such a helper.
Is there value to putting this in a function like the one in
cmd_cache.c?
i think there's value in moving this to generalizing and moving to common
code. there are other places where we handle env vars which could have the
value on or off.
+#if defined(STATUS_LED_BIT) defined(CONFIG_BOARD_SPECIFIC_LED)
+ if (strcmp(argv[1], 0) == 0) {
+ mask = STATUS_LED_BIT;
+ __led_set(mask, state);
+ }
+ else
+#endif
+#if defined(STATUS_LED_BIT1) defined(CONFIG_BOARD_SPECIFIC_LED)
+ if (strcmp(argv[1], 1) == 0) {
+ mask = STATUS_LED_BIT1;
+ __led_set(mask, state);
+ }
+ else
+#endif
+#if defined(STATUS_LED_BIT2) defined(CONFIG_BOARD_SPECIFIC_LED)
+ if (strcmp(argv[1], 2) == 0) {
+ mask = STATUS_LED_BIT2;
+ __led_set(mask, state);
+ }
+ else
+#endif
+#if defined(STATUS_LED_BIT3) defined(CONFIG_BOARD_SPECIFIC_LED)
+ if (strcmp(argv[1], 3) == 0) {
+ mask = STATUS_LED_BIT3;
+ __led_set(mask, state);
+ }
+ else
+#endif
i dont know why you need the mask variable here at all
It is an ugly hack at avoiding definition of the bit pattern to be
passed into __led_set(), to keep that function lean as defined by the
platform.
i dont follow. why are these two things different ?
...
mask = STATUS_LED_BIT3;
__led_set(mask, state);
...
__led_set(STATUS_LED_BIT3, state);
...
also, these #ifdef trees scream for some sort of unification
It impacts performance, but what do you think if I just put them into
a data structure and loop, like what I'm suggesting above with my
functions?
i mean at least create a single define that expands into the duplicated code
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] Add 'led' command
Mike,
Thanks for the feedback (ack to all of it). I didn't fix-up the style
from the original patch given that I figured there'd be enough
comments to deal with regarding the overall architecture of it, but it
seems there is a need for such a generic command and this really is
the easiest way to move along the process of getting something. It
isn't like this is the calling of my career to fix the LED command,
but I need it and maybe I can provide some better ideas along the way
without this taking an eternity...
On Tue, Nov 9, 2010 at 8:52 AM, Mike Frysinger vap...@gentoo.org wrote:
On Friday, November 05, 2010 01:50:36 Jason Kridner wrote:
+int do_led ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[] )
much of the style of this code is broken. and i cant imagine this code
compiling warning free with current master.
no spaces around the paren, and the argv has been constified.
also, this should be marked static
+ if ((argc != 3)){
space before the brace and useless set of paren here
+ printf(Usage:\n%s\n, cmdtp-usage);
+ return 1;
return cmd_usage(cmdtp);
+ if (strcmp(argv[2], off) == 0) {
+ state = 0;
+ } else if (strcmp(argv[2], on) == 0) {
+ state = 1;
i could have sworn we had a helper somewhere to handle boolean strings ...
common/cmd_cache.c has an internal on_off function. All other places
seem to do individual strcmp. Let me know if you find such a helper.
Is there value to putting this in a function like the one in
cmd_cache.c?
static int on_off (const char *s)
{
if (strcmp(s, on) == 0) {
return (1);
} else if (strcmp(s, off) == 0) {
return (0);
}
return (-1);
}
+ printf (Usage:\n%s\n, cmdtp-usage);
+ return 1;
return cmd_usage(cmdtp);
+#if defined(STATUS_LED_BIT) defined(CONFIG_BOARD_SPECIFIC_LED)
+ if (strcmp(argv[1], 0) == 0) {
+ mask = STATUS_LED_BIT;
+ __led_set(mask, state);
+ }
+ else
+#endif
+#if defined(STATUS_LED_BIT1) defined(CONFIG_BOARD_SPECIFIC_LED)
+ if (strcmp(argv[1], 1) == 0) {
+ mask = STATUS_LED_BIT1;
+ __led_set(mask, state);
+ }
+ else
+#endif
+#if defined(STATUS_LED_BIT2) defined(CONFIG_BOARD_SPECIFIC_LED)
+ if (strcmp(argv[1], 2) == 0) {
+ mask = STATUS_LED_BIT2;
+ __led_set(mask, state);
+ }
+ else
+#endif
+#if defined(STATUS_LED_BIT3) defined(CONFIG_BOARD_SPECIFIC_LED)
+ if (strcmp(argv[1], 3) == 0) {
+ mask = STATUS_LED_BIT3;
+ __led_set(mask, state);
+ }
+ else
+#endif
i dont know why you need the mask variable here at all
It is an ugly hack at avoiding definition of the bit pattern to be
passed into __led_set(), to keep that function lean as defined by the
platform. I think a more practical approach would be to define an
led_set(led_no, state) function that only ever set the state of a
single LED at a time (with LEDs having an integral state, in case they
want to encode color or brightness in the integer) and then to create
query functions:
int led_get_state(led_no) - to return the current LED state
const char * led_get_description(led_no) - to return the pointer to
a constant character string that could be used in prompts and a looped
strcmp --- or maybe just have a single constant string with a known
separator, |, that can be reused in the usage output.
int led_last_no() - to return the index number of the last LED on
the board (I could stand to have it be a LED_LAST_NO definition)
In the led command, on would always be the value 1 and 0 would
always be off, but passing an integer would be fine. I'm sure most
implementations will simply be a GPIO, but I can imagine someone using
the command to adjust the state of a back-light.
I'm sure a better scheme could be dreamed, but that is my simple
attempt with a minimal set of functions. I think such a change would
require some good cooperation with many maintainers to make sure I'm
not breaking their systems.
also, these #ifdef trees scream for some sort of unification
It impacts performance, but what do you think if I just put them into
a data structure and loop, like what I'm suggesting above with my
functions?
It would be possible to simply create something like
const char * char my_leds = red|yellow|green|top|bottom|backlight;
For legacy purposes, in status_led.h I could have something like I've
done for the usage command today and create a generic driver using
the existing function definitions (blue_LED_*, red_LED_*, etc.). It
would mean that a structure of ifdefs would still be there, but that
most implementors wouldn't need to use them and I'd replace this
particular set of ifdefs with a for loop that incremented per
character and used a strncmp when it found a | or \0, exiting after
finding the \0.
Sound like an improvement?
Re: [U-Boot] [RFC] Add 'led' command
On Friday, November 05, 2010 01:50:36 Jason Kridner wrote:
+int do_led ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[] )
much of the style of this code is broken. and i cant imagine this code
compiling warning free with current master.
no spaces around the paren, and the argv has been constified.
also, this should be marked static
+ if ((argc != 3)){
space before the brace and useless set of paren here
+ printf(Usage:\n%s\n, cmdtp-usage);
+ return 1;
return cmd_usage(cmdtp);
+ if (strcmp(argv[2], off) == 0) {
+ state = 0;
+ } else if (strcmp(argv[2], on) == 0) {
+ state = 1;
i could have sworn we had a helper somewhere to handle boolean strings ...
+ printf (Usage:\n%s\n, cmdtp-usage);
+ return 1;
return cmd_usage(cmdtp);
+#if defined(STATUS_LED_BIT) defined(CONFIG_BOARD_SPECIFIC_LED)
+ if (strcmp(argv[1], 0) == 0) {
+ mask = STATUS_LED_BIT;
+ __led_set(mask, state);
+ }
+ else
+#endif
+#if defined(STATUS_LED_BIT1) defined(CONFIG_BOARD_SPECIFIC_LED)
+ if (strcmp(argv[1], 1) == 0) {
+ mask = STATUS_LED_BIT1;
+ __led_set(mask, state);
+ }
+ else
+#endif
+#if defined(STATUS_LED_BIT2) defined(CONFIG_BOARD_SPECIFIC_LED)
+ if (strcmp(argv[1], 2) == 0) {
+ mask = STATUS_LED_BIT2;
+ __led_set(mask, state);
+ }
+ else
+#endif
+#if defined(STATUS_LED_BIT3) defined(CONFIG_BOARD_SPECIFIC_LED)
+ if (strcmp(argv[1], 3) == 0) {
+ mask = STATUS_LED_BIT3;
+ __led_set(mask, state);
+ }
+ else
+#endif
i dont know why you need the mask variable here at all
also, these #ifdef trees scream for some sort of unification
+ } else {
+ printf (Usage:\n%s\n, cmdtp-usage);
+ return 1;
return cmd_usage(cmptp);
+
files should not have trailing new lines
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] Add 'led' command
Dear Jason Kridner, In message 1288936236-30603-1-git-send-email-jkrid...@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 Kridner jkrid...@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 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/eukrea/cpu9260/led.c board/logicpd/zoom2/led.c board/ns9750dev/led.c board/psyent/pk1c20/led.c board/ronetix/pm9261/led.c board/ronetix/pm9263/led.c Please, let's unify these. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Dear Lord: I just want *one* one-armed manager so I never have to hear On the other hand, again. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] Add 'led' command
Dear Wolfgang Denk, 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 Kridner jkrid...@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/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 At least the atmel stuff are functions to implement the control of the LEDs (via gpio, i2c, spi etc.) which inherently is board specific; but not a command interface to control them from u-boot prompt/scripts. His patch tries to add a command, not a LED implementation. Such a command was on my mind for a while. Best Regards, Reinhard ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [RFC] Add 'led' command
On Fri, Nov 5, 2010 at 9:13 AM, Reinhard Meyer u-b...@emk-elektronik.de wrote: Dear Wolfgang Denk, 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 Kridner jkrid...@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/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 At least the atmel stuff are functions to implement the control of the LEDs (via gpio, i2c, spi etc.) which inherently is board specific; but not a command interface to control them from u-boot prompt/scripts. His patch tries to add a command, not a LED implementation. Such a command was on my mind for a while. I tried to make it such that this command is enabled by the implementations on the other architectures by following the existing design. I don't know how they are making use of the LED functions, so it seems this command is required to make their implementations useful. I hope that is reason enough to at least get different maintainers to try this command out and give some additional feedback. It would be great if we had a summary of how these LED functions are used. For the BeagleBoard, we are simply enabling scripts to use this command. I think others are using the LED functions to indicate boot status and other u-boot native operations. Does such a summary exist so that I can make any command implementation suitable? Regards, Jason ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
