[U-Boot] [PATCH 0/2] GPIO LED improvements (resend)

[U-Boot] [PATCH 0/2] GPIO LED improvements (resend)

[Bernhard Nortmann]

This is a resubmission of http://patchwork.ozlabs.org/patch/455748/
and http://patchwork.ozlabs.org/patch/455747/, but this time with
proper formatting and through patman.

Regards, B. Nortmann


Bernhard Nortmann (2):
  add generic stubs for GPIO LEDs
  allow LED initialization without STATUS_LED_BOOT

 common/board_r.c          |  9 +++++---
 drivers/misc/gpio_led.c   | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/misc/status_led.c |  2 +-
 include/status_led.h      |  1 +
 4 files changed, 62 insertions(+), 4 deletions(-)

-- 
2.4.6

[U-Boot] [PATCH 1/2] add generic stubs for GPIO LEDs

[Bernhard Nortmann]

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 <bernhard.nortmann@web.de>
---

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

diff --git a/drivers/misc/gpio_led.c b/drivers/misc/gpio_led.c
index 3e95727..164c30d 100644
--- a/drivers/misc/gpio_led.c
+++ b/drivers/misc/gpio_led.c
@@ -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] [PATCH 1/2] add generic stubs for GPIO LEDs

[Simon Glass]

Hi Bernard,

On 21 August 2015 at 07:13, Bernhard Nortmann <bernhard.nortmann@web.de> wrote:
> 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 <bernhard.nortmann@web.de>
> ---
>
>  drivers/misc/gpio_led.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)

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

Reviewed-by: Simon Glass <sjg@chromium.org>

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.

>
> diff --git a/drivers/misc/gpio_led.c b/drivers/misc/gpio_led.c
> index 3e95727..164c30d 100644
> --- a/drivers/misc/gpio_led.c
> +++ b/drivers/misc/gpio_led.c
> @@ -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

Regards,
Simon

[U-Boot] [PATCH 1/2] add generic stubs for GPIO LEDs

[Bernhard Nortmann]

Hello Simon!

Am 23.08.2015 23:21, schrieb Simon Glass:
> Hi Bernard,
>
> [...]
> If this is a new option it should be added to Kconfig. Otherwise:
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
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.

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

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

Regards, B. Nortmann

[U-Boot] [PATCH 1/2] add generic stubs for GPIO LEDs

[Tom Rini]

On Mon, Aug 24, 2015 at 11:51:46AM +0200, Bernhard Nortmann wrote:
> Hello Simon!
> 
> Am 23.08.2015 23:21, schrieb Simon Glass:
> >Hi Bernard,
> >
> >[...]
> >If this is a new option it should be added to Kconfig. Otherwise:
> >
> >Reviewed-by: Simon Glass <sjg@chromium.org>
> 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.
> 
> >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".
> 
> >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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150828/ca807ae8/attachment.sig>

[U-Boot] [PATCH 1/2] add generic stubs for GPIO LEDs

[Bernhard Nortmann]

Hello Tom!

To be honest, I have no convincing idea on how to move forward with this.
My original intent was to get some GPIO LED support for the sunxi family of
devices, namely my Banana Pi (sun7i/A20).

Following Simon's suggestion, I had a closer look at the new DM-based
drivers/led/led_gpio.c. I agree that this is basically where I'd want to
move my stuff to, instead of adding onto the drivers/misc/gpio_led.c code.

Unfortunately the driver model code currently has no connection at all to
the actual "led" command handler in common/cmd_led.c. To make things worse,
cmd_led.c declares the available LEDs 'statically' at compile time (via 
array
initializers). Also CMD_LED is not available in Kconfig yet, and still uses
the old .h includes.

Taking a closer look at the existing LED code, I get the impression that a
lot of it has 'grown' historically - and probably should go through a major
refactor / rewrite cycle. I have identified no less than three other
"subsystems" dealing with LEDs (in addition to drivers/led/ and cmd_led.c):

* The "status led" code in drivers/misc/ deals with LED functionality using
a "bit mask" approach, which in turns calls board-specific 
implementations of
__led_init, __led_set and/or __led_toggle. It also offers limited 
support for
'software blinking' of LEDs with the status_led_tick() callback handler, but
that only seems supported (i.e. actually used) on NIOS2 and PowerPC.

* drivers/misc/ also offers 'legacy' support for GPIO LEDs. For this, the
__led_init, __led_set and __led_toggle functions are rerouted to appropriate
gpio_*() calls.

* U-Boot also supports "colored" LEDs via 
STATUS_LED_(RED|GREEN|YELLOW|BLUE).
For this a set of weak functions is prefined at board level
(e.g. common/board_f.c), and supposed to be overridden by (device-)specific
implementations. These "colored" LED functions (or stubs) are made available
through cmd_led.c again.

And last but not least, cmd_led.c sneaks in a 'hardware blink' function
__led_blink() as yet another stub that can be overwritten...

I don't feel like I would actually be in a position (or know a major part of
board implementations well enough) to tackle all of this. I'd expect 
that all
of these "subsystems" would need to be cleaned up and unified in some 
sort of
common (and 'generic') LED framework, which in turn would work closely 
together
with the actual "led" command. cmd_led.c should be able to manage LED
"descriptors" dynamically, allowing them to be registered by the various
implementations (possibly together with a set of function pointers that
provides cmd_led.c with a uniform interface to actual LED 'functionality').

With regard to the original proposed patch: I'm not even sure I'd want to go
forward with this given the current state. My checking did not seem to 
reveal
any boards that make use of the "colored" LED (weak) functions and legacy
gpio_led.c at the same time, so it might be safe to introduce the proposed
GPIO LED stubs even without having a new CONFIG_GPIO_LED_STUBS option.

OTOH, the whole patch seems a bit pointless - given that the "future" 
direction
we want to take for GPIO LEDs will be based on driver model anyway...

Regards, B. Nortmann


Am 28.08.2015 um 17:00 schrieb Tom Rini:
 > On Mon, Aug 24, 2015 at 11:51:46AM +0200, Bernhard Nortmann wrote:
 >> Hello Simon!
 >>
 >> Am 23.08.2015 23:21, schrieb Simon Glass:
 >>> Hi Bernard,
 >>>
 >>> [...]
 >>> If this is a new option it should be added to Kconfig. Otherwise:
 >>>
 >>> Reviewed-by: Simon Glass <sjg@chromium.org>
 >> 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.
 >>
 >>> 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".
 >>
 >>> 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!

[U-Boot] [U-Boot,1/2] add generic stubs for GPIO LEDs

[Tom Rini]

On Fri, Aug 21, 2015 at 03:13:20PM +0200, Bernhard Nortmann wrote:

> 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 <bernhard.nortmann@web.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151024/2bac70ab/attachment.sig>

[U-Boot] [PATCH 2/2] allow LED initialization without STATUS_LED_BOOT

[Bernhard Nortmann]

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 <bernhard.nortmann@web.de>
---

 common/board_r.c          | 9 ++++++---
 drivers/misc/status_led.c | 2 +-
 include/status_led.h      | 1 +
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/common/board_r.c b/common/board_r.c
index bf6c725..d1e3bf2 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -539,11 +539,14 @@ static int initr_kgdb(void)
 }
 #endif
 
-#if defined(CONFIG_STATUS_LED) && defined(STATUS_LED_BOOT)
+#if defined(CONFIG_STATUS_LED)
 static int initr_status_led(void)
 {
+#if defined(STATUS_LED_BOOT)
 	status_led_set(STATUS_LED_BOOT, STATUS_LED_BLINKING);
-
+#else
+	status_led_init();
+#endif
 	return 0;
 }
 #endif
@@ -830,7 +833,7 @@ init_fnc_t init_sequence_r[] = {
 	|| defined(CONFIG_M68K)
 	timer_init,		/* initialize timer */
 #endif
-#if defined(CONFIG_STATUS_LED) && defined(STATUS_LED_BOOT)
+#if defined(CONFIG_STATUS_LED)
 	initr_status_led,
 #endif
 	/* PPC has a udelay(20) here dating from 2002. Why? */
diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c
index 9869d98..31e8831 100644
--- a/drivers/misc/status_led.c
+++ b/drivers/misc/status_led.c
@@ -73,7 +73,7 @@ led_dev_t led_dev[] = {
 
 static int status_led_init_done = 0;
 
-static void status_led_init (void)
+void status_led_init(void)
 {
 	led_dev_t *ld;
 	int i;
diff --git a/include/status_led.h b/include/status_led.h
index a5e35df..b0ff274 100644
--- a/include/status_led.h
+++ b/include/status_led.h
@@ -23,6 +23,7 @@
 #define STATUS_LED_BLINKING	1
 #define STATUS_LED_ON		2
 
+void status_led_init(void);
 void status_led_tick (unsigned long timestamp);
 void status_led_set  (int led, int state);
 
-- 
2.4.6

[U-Boot] [U-Boot, 2/2] allow LED initialization without STATUS_LED_BOOT

[Tom Rini]

On Fri, Aug 21, 2015 at 03:13:21PM +0200, Bernhard Nortmann wrote:

> 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 <bernhard.nortmann@web.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151024/b11f715f/attachment.sig>