[PATCH] Add 'led' command

GoogleGroups
1

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.

Updated based on feedback:
http://www.mail-archive.com/u-boot@lists.denx.de/msg41847.html
https://groups.google.com/d/topic/beagleboard/8Wf1HiK_QBo/discussion
* Fixed a handful of style issues.
* Significantly reduced the number of #ifdefs and redundant code
* Converted redundant code into loops test against a structure
* Made use of cmd_usage()
* Introduced a str_onoff() function, but haven't yet put it in common
* Eliminated trailing newline

Signed-off-by: Jason Kridner <jkridner@beagleboard.org>

2

Dear Jason Kridner,

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 ]

I still wonder if such a patch will help to get rid of the ton of LEd
drivers we already have in U-Boot, or if it just adds another such
interface?

Which drivers ("led.c" files) will be obsoleted by this patch?

If it is intended to be a generic interface - how can this then be
combined with the status_led.c driver?

The configuration "green", "yellow", "red" seems to be very specific
to me - I guess this applies just to very few boards?

...

+struct led_tbl_s {
+ char *string; /* String for use in the command */
+ led_id_t mask; /* Mask used for calling __led_set() */
+ void (*on)(void); /* Optional fucntion for turning LED on */
+ void (*off)(void); /* Optional fucntion for turning LED on */
+};
+
+typedef struct led_tbl_s led_tbl_t;
+
+static const led_tbl_t led_commands = {
+#ifdef CONFIG_BOARD_SPECIFIC_LED
+#ifdef STATUS_LED_BIT
+ { "0", STATUS_LED_BIT, NULL, NULL },
+#endif
+#ifdef STATUS_LED_BIT1
+ { "1", STATUS_LED_BIT1, NULL, NULL },
+#endif
+#ifdef STATUS_LED_BIT2
+ { "2", STATUS_LED_BIT2, NULL, NULL },
+#endif
+#ifdef STATUS_LED_BIT3
+ { "3", STATUS_LED_BIT3, NULL, NULL },
+#endif

What are these "status bits" good for when they have no actual
handlers attached? Where are they actually used?

+#ifdef STATUS_LED_GREEN
+ { "green", STATUS_LED_GREEN, green_LED_off, green_LED_on },
+#endif
+#ifdef STATUS_LED_YELLOW
+ { "yellow", STATUS_LED_YELLOW, yellow_LED_off, yellow_LED_on },
+#endif
+#ifdef STATUS_LED_RED
+ { "red", STATUS_LED_RED, red_LED_off, red_LED_on },
+#endif
+#ifdef STATUS_LED_BLUE
+ { "blue", STATUS_LED_BLUE, blue_LED_off, blue_LED_on },
+#endif

We do not allow CamelCase identifiers (like "green_LED_off").

Best regards,

Wolfgang Denk

3

Dear Jason Kridner,

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 ]

I still wonder if such a patch will help to get rid of the ton of LEd
drivers we already have in U-Boot, or if it just adds another such
interface?

It neither adds nor subtracts.

Which drivers (“led.c” files) will be obsoleted by this patch?

None. The objective is simply to expose led.c functionality to a command-line function.

If it is intended to be a generic interface - how can this then be
combined with the status_led.c driver?

It is intended to utilize status_led.h and therefore to be complementary to status_led.c. Looking at it right now, it looks like I can better reuse some functions in that driver, so I will modify the code to do that. My apologies for missing it.

The configuration “green”, “yellow”, “red” seems to be very specific
to me - I guess this applies just to very few boards?

Yes, but it is in status_led.h, so I wanted to include the support for it.

…

+struct led_tbl_s {

  • char string; / String for use in the command */
  • led_id_t mask; /* Mask used for calling __led_set() */
  • void (on)(void); / Optional fucntion for turning LED on */
  • void (off)(void); / Optional fucntion for turning LED on */
    +};

+typedef struct led_tbl_s led_tbl_t;
+
+static const led_tbl_t led_commands = {
+#ifdef CONFIG_BOARD_SPECIFIC_LED
+#ifdef STATUS_LED_BIT

  • { “0”, STATUS_LED_BIT, NULL, NULL },
    +#endif
    +#ifdef STATUS_LED_BIT1
  • { “1”, STATUS_LED_BIT1, NULL, NULL },
    +#endif
    +#ifdef STATUS_LED_BIT2
  • { “2”, STATUS_LED_BIT2, NULL, NULL },
    +#endif
    +#ifdef STATUS_LED_BIT3
  • { “3”, STATUS_LED_BIT3, NULL, NULL },
    +#endif

What are these “status bits” good for when they have no actual
handlers attached? Where are they actually used?

If no LED specific handler is provided, __led_set is used. I’m going to switch this to status_led_set() based on your feedback.

+#ifdef STATUS_LED_GREEN

  • { “green”, STATUS_LED_GREEN, green_LED_off, green_LED_on },
    +#endif
    +#ifdef STATUS_LED_YELLOW
  • { “yellow”, STATUS_LED_YELLOW, yellow_LED_off, yellow_LED_on },
    +#endif
    +#ifdef STATUS_LED_RED
  • { “red”, STATUS_LED_RED, red_LED_off, red_LED_on },
    +#endif
    +#ifdef STATUS_LED_BLUE
  • { “blue”, STATUS_LED_BLUE, blue_LED_off, blue_LED_on },
    +#endif

We do not allow CamelCase identifiers (like “green_LED_off”).

Easy enough to fix.

4

Adding u-boot list… seem to have missed it in my reply.

5

Adding u-boot list… seem to have missed it in my reply.

Dear Jason Kridner,

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 ]

I still wonder if such a patch will help to get rid of the ton of LEd
drivers we already have in U-Boot, or if it just adds another such
interface?

It neither adds nor subtracts.

Which drivers (“led.c” files) will be obsoleted by this patch?

None. The objective is simply to expose led.c functionality to a command-line function.

If it is intended to be a generic interface - how can this then be
combined with the status_led.c driver?

It is intended to utilize status_led.h and therefore to be complementary to status_led.c. Looking at it right now, it looks like I can better reuse some functions in that driver, so I will modify the code to do that. My apologies for missing it.

The configuration “green”, “yellow”, “red” seems to be very specific
to me - I guess this applies just to very few boards?

Yes, but it is in status_led.h, so I wanted to include the support for it.

…

+struct led_tbl_s {

  • char string; / String for use in the command */
  • led_id_t mask; /* Mask used for calling __led_set() */
  • void (on)(void); / Optional fucntion for turning LED on */
  • void (off)(void); / Optional fucntion for turning LED on */
    +};

+typedef struct led_tbl_s led_tbl_t;
+
+static const led_tbl_t led_commands = {
+#ifdef CONFIG_BOARD_SPECIFIC_LED
+#ifdef STATUS_LED_BIT

  • { “0”, STATUS_LED_BIT, NULL, NULL },
    +#endif
    +#ifdef STATUS_LED_BIT1
  • { “1”, STATUS_LED_BIT1, NULL, NULL },
    +#endif
    +#ifdef STATUS_LED_BIT2
  • { “2”, STATUS_LED_BIT2, NULL, NULL },
    +#endif
    +#ifdef STATUS_LED_BIT3
  • { “3”, STATUS_LED_BIT3, NULL, NULL },
    +#endif

What are these “status bits” good for when they have no actual
handlers attached? Where are they actually used?

If no LED specific handler is provided, __led_set is used. I’m going to switch this to status_led_set() based on your feedback.

+#ifdef STATUS_LED_GREEN

  • { “green”, STATUS_LED_GREEN, green_LED_off, green_LED_on },
    +#endif
    +#ifdef STATUS_LED_YELLOW
  • { “yellow”, STATUS_LED_YELLOW, yellow_LED_off, yellow_LED_on },
    +#endif
    +#ifdef STATUS_LED_RED
  • { “red”, STATUS_LED_RED, red_LED_off, red_LED_on },
    +#endif
    +#ifdef STATUS_LED_BLUE
  • { “blue”, STATUS_LED_BLUE, blue_LED_off, blue_LED_on },
    +#endif

We do not allow CamelCase identifiers (like “green_LED_off”).

Easy enough to fix.

I might have spoken too soon. Those identifiers come straight out of status_led.h. Would you like me to run a script across the entire codebase to switch them?

I am doing so with:
for file in find . | grep '\.[ch]$'; do perl -i -pe ‘s/(green|yellow|red|blue)LED(on|off)/$1_led_$2/g’ $file; done

Eventually, maybe I’ll have my head wrapped around how all of this led.c stuff works well enough to really give you a global clean-up. I still feel like I’m being suckered into something when all I want is a command to access the functions that are already there. I guess that is the price of open source. :slight_smile: