[RFC] Add 'led' command

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 <jkridner@beagleboard.org>

Dear Jason Kridner,

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 <jkridner@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

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 <jkridner@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

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

+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

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...

+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? Shall I give the suggestion in code?

>> + 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

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