GPIO LED improvements (resend)

For boards that support LEDs driven via GPIO (CONFIG_GPIO_LED), it may be useful to have some generic stubs (wrapper functions) for the "colored" LEDs.

This allows defining STATUS_LED_* values directly to GPIO numbers, e.g.: #define STATUS_LED_GREEN 248 /* = PH24 */

To keep those optional, it's probably best to introduce an additional configuration setting. I've chosen CONFIG_GPIO_LED_STUBS for that. Placing the code in drivers/misc/gpio_led.c also ensures that it automatically depends on CONFIG_GPIO_LED too.

Signed-off-by Bernhard Nortmann <[email protected]>

---

 drivers/misc/gpio_led.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

drivers/misc/gpio_led.c +54 -1

index 3e95727..164c30d 100644
@@ -51,3 +51,57 @@ void __led_toggle(led_id_t mask)
 {
        gpio_set_value(mask, !gpio_get_value(mask));
 }
+
+#ifdef CONFIG_GPIO_LED_STUBS
+
+/* 'generic' override of colored LED stubs, to use GPIO functions instead */
+
+#ifdef STATUS_LED_RED
+void red_led_on(void)
+{
+       __led_set(STATUS_LED_RED, STATUS_LED_ON);
+}
+
+void red_led_off(void)
+{
+       __led_set(STATUS_LED_RED, STATUS_LED_OFF);
+}
+#endif
+
+#ifdef STATUS_LED_GREEN
+void green_led_on(void)
+{
+       __led_set(STATUS_LED_GREEN, STATUS_LED_ON);
+}
+
+void green_led_off(void)
+{
+       __led_set(STATUS_LED_GREEN, STATUS_LED_OFF);
+}
+#endif
+
+#ifdef STATUS_LED_YELLOW
+void yellow_led_on(void)
+{
+       __led_set(STATUS_LED_YELLOW, STATUS_LED_ON);
+}
+
+void yellow_led_off(void)
+{
+       __led_set(STATUS_LED_YELLOW, STATUS_LED_OFF);
+}
+#endif
+
+#ifdef STATUS_LED_BLUE
+void blue_led_on(void)
+{
+       __led_set(STATUS_LED_BLUE, STATUS_LED_ON);
+}
+
+void blue_led_off(void)
+{
+       __led_set(STATUS_LED_BLUE, STATUS_LED_OFF);
+}
+#endif
+
+#endif /* CONFIG_GPIO_LED_STUBS */
--

2.4.6

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Hi Bernard,

[...]
If this is a new option it should be added to Kconfig. Otherwise:

Reviewed-by Simon Glass <[email protected]>

I am not a huge fan of the existing API. I would suggest that if you have the energy you could replace it with something like:

enum led_colour_t {
    led_red,
    red_green,
    ...
};

int led_set_on(enum led_colour_t colour, bool on)

BTW there is a device-tree-enabled LED driver node (see drivers/led). It might be worth considering using this on some sunxi boards.

Hello Simon!

Am 23.08.2015 23:21, schrieb Simon Glass:

Quoted 6 lines

Hi Bernard,

[...]
If this is a new option it should be added to Kconfig. Otherwise:

Reviewed-by Simon Glass <[email protected]>

Right. Kconfig wasn't on my agenda, but I agree that it should go in there. Unfortunately this points out further problems. The existing CONFIG_GPIO_LED is not part of Kconfig, but gets set via board-specific (.h) includes. Kconfig
has/supports CONFIG_LED_GPIO, but that is related to the driver model variant, and doesn't affect inclusion of drivers/misc/gpio_led.c ... admittedly it's a bit of a mess.

Quoted 10 lines

I am not a huge fan of the existing API. I would suggest that if you have the energy you could replace it with something like:

enum led_colour_t {
   led_red,
   red_green,
   ...
};

int led_set_on(enum led_colour_t colour, bool on)

Yes, I've been code-dancing a bit back and forth to get this stuff to fit in with the existing API. One reason I have selected these "stub" functions is that they are meant to match (and replace) the existing weak definitions in arch/arm/lib/board.c. Also, my impression is that they exist as separate functions to allow common/cmd_led.c to use them as function pointers, namely for the construction of the led_commands[] array.

The API may not be pretty, but I have in fact tried to keep it as close to the original as possible, with the goal of allowing to simply substitute GPIO numbers for the "LED bits".

Quoted 2 lines

BTW there is a device-tree-enabled LED driver node (see drivers/led). It might be worth considering using this on some sunxi boards.

Thanks! I'll definitely have a look into it.
(That one is related to the CONFIG_LED_GPIO mentioned above.)

Quoted 10 lines

Hello Simon!

Am 23.08.2015 23:21, schrieb Simon Glass:

Quoted 6 lines

Hi Bernard,

[...]
If this is a new option it should be added to Kconfig. Otherwise:

Reviewed-by Simon Glass <[email protected]>

Right. Kconfig wasn't on my agenda, but I agree that it should go in

Quoted 1 line

Unfortunately this points out further problems. The existing

Quoted 9 lines

is not part of Kconfig, but gets set via board-specific (.h) includes. Kconfig
has/supports CONFIG_LED_GPIO, but that is related to the driver model variant, and doesn't affect inclusion of drivers/misc/gpio_led.c ... admittedly it's a bit of a mess.

Quoted 3 lines

I am not a huge fan of the existing API. I would suggest that if you have the energy you could replace it with something like:

Quoted 8 lines

Quoted 7 lines

enum led_colour_t {
   led_red,
   red_green,
   ...
};

int led_set_on(enum led_colour_t colour, bool on)

Yes, I've been code-dancing a bit back and forth to get this stuff

Quoted 2 lines

with the existing API. One reason I have selected these "stub" functions is that they are meant to match (and replace) the existing weak

Quoted 1 line

in arch/arm/lib/board.c. Also, my impression is that they exist as

Quoted 4 lines

functions to allow common/cmd_led.c to use them as function pointers, namely for the construction of the led_commands[] array.

The API may not be pretty, but I have in fact tried to keep it as

Quoted 7 lines

the original as possible, with the goal of allowing to simply substitute GPIO numbers for the "LED bits".

Quoted 2 lines

BTW there is a device-tree-enabled LED driver node (see drivers/led). It might be worth considering using this on some sunxi boards.

Thanks! I'll definitely have a look into it.
(That one is related to the CONFIG_LED_GPIO mentioned above.)

So do you want to do these further clean-ups on top of this series or in addition to? Thanks!

For boards that support LEDs driven via GPIO (CONFIG_GPIO_LED), it may be useful to have some generic stubs (wrapper functions) for the "colored" LEDs.

This allows defining STATUS_LED_* values directly to GPIO numbers,
e.g.: #define STATUS_LED_GREEN 248 /* = PH24 */

To keep those optional, it's probably best to introduce an additional configuration setting. I've chosen CONFIG_GPIO_LED_STUBS for that. Placing the code in drivers/misc/gpio_led.c also ensures that it automatically depends on CONFIG_GPIO_LED too.

Signed-off-by Bernhard Nortmann <[email protected]>

Reviewed-by Simon Glass <[email protected]>

For current U-Boot to initialize status LEDs via status_led_init(), it is required to have both CONFIG_STATUS_LED and STATUS_LED_BOOT defined. This may be a particular concern with GPIO LEDs, where __led_init() is required to correctly set up the GPIO (gpio_request and gpio_direction_output). Without STATUS_LED_BOOT the initialization isn't called, which could leave the user with a non-functional "led" command - due to the fact that the LED routines in gpio_led.c use gpio_set_value() just fine, but the GPIO never got set up properly in the first place.

I think having CONFIG_STATUS_LED is sufficient to justify a corresponding call to status_led_init(), even with no STATUS_LED_BOOT defined. To do so, common/board_r.c needs call that routine, so it now is exposed via status_led.h.

Signed-off-by Bernhard Nortmann <[email protected]>