Patchwork [U-Boot,6/7] status_led: Add support for inverted LEDs

login
register
mail settings
Submitter Otavio Salvador
Date Sept. 28, 2013, 3:24 a.m.
Message ID <1380338659-7896-6-git-send-email-otavio@ossystems.com.br>
Download mbox | patch
Permalink /patch/278706/
State Superseded
Headers show

Comments

Otavio Salvador - Sept. 28, 2013, 3:24 a.m.
There're cases we want to use active-low LEDs and the 'inverted' logic
needs to be added. This includes it using the STATUS_LED_INVERT macro.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---
 doc/README.LED            |  2 ++
 drivers/misc/status_led.c | 21 ++++++++++++++++++---
 include/status_led.h      | 14 ++++++++++++++
 3 files changed, 34 insertions(+), 3 deletions(-)
Benoît Thébaudeau - Sept. 28, 2013, 2:17 p.m.
Dear Otavio Salvador,

On Saturday, September 28, 2013 5:24:17 AM, Otavio Salvador wrote:
> There're cases we want to use active-low LEDs and the 'inverted' logic
> needs to be added. This includes it using the STATUS_LED_INVERT macro.

There is already a STATUS_LED_ACTIVE definition (though not one per LED) in
include/status_led.h for some platforms. Wouldn't it be worth keeping the same
naming here for consistency (i.e. STATUS_LED_ACTIVEn, which would also imply
exchanging 0/1 values)?

[...]

Best regards,
Benoît
Fabio Estevam - Sept. 28, 2013, 4:49 p.m.
On Sat, Sep 28, 2013 at 11:17 AM, Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:
> Dear Otavio Salvador,
>
> On Saturday, September 28, 2013 5:24:17 AM, Otavio Salvador wrote:
>> There're cases we want to use active-low LEDs and the 'inverted' logic
>> needs to be added. This includes it using the STATUS_LED_INVERT macro.
>
> There is already a STATUS_LED_ACTIVE definition (though not one per LED) in
> include/status_led.h for some platforms. Wouldn't it be worth keeping the same
> naming here for consistency (i.e. STATUS_LED_ACTIVEn, which would also imply
> exchanging 0/1 values)?

I agree. "INVERT" is confusing, because we don't know what is the normal state.

Doing like Benoît suggests would be clearer: STATUS_LED_ACTIVE0 or
STATUS_LED_ACTIVE1.
Otavio Salvador - Sept. 28, 2013, 7:08 p.m.
On Sat, Sep 28, 2013 at 1:49 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Sat, Sep 28, 2013 at 11:17 AM, Benoît Thébaudeau
> <benoit.thebaudeau@advansee.com> wrote:
>> Dear Otavio Salvador,
>>
>> On Saturday, September 28, 2013 5:24:17 AM, Otavio Salvador wrote:
>>> There're cases we want to use active-low LEDs and the 'inverted' logic
>>> needs to be added. This includes it using the STATUS_LED_INVERT macro.
>>
>> There is already a STATUS_LED_ACTIVE definition (though not one per LED) in
>> include/status_led.h for some platforms. Wouldn't it be worth keeping the same
>> naming here for consistency (i.e. STATUS_LED_ACTIVEn, which would also imply
>> exchanging 0/1 values)?
>
> I agree. "INVERT" is confusing, because we don't know what is the normal state.
>
> Doing like Benoît suggests would be clearer: STATUS_LED_ACTIVE0 or
> STATUS_LED_ACTIVE1.

The problem here is that the BIT LEDs are used in the cmd_led and it
does not have the 'active' knowledge but a ON OFF concept. So what we
do there is to change the intended status. The STATUS_LED_ACTIVE is
for the STATUS_LED_BOOT and not for a 'specific' bit.
Benoît Thébaudeau - Sept. 29, 2013, 1:40 p.m.
Hi Otavio,

On Saturday, September 28, 2013 9:08:48 PM, Otavio Salvador wrote:
> On Sat, Sep 28, 2013 at 1:49 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > On Sat, Sep 28, 2013 at 11:17 AM, Benoît Thébaudeau
> > <benoit.thebaudeau@advansee.com> wrote:
> >> Dear Otavio Salvador,
> >>
> >> On Saturday, September 28, 2013 5:24:17 AM, Otavio Salvador wrote:
> >>> There're cases we want to use active-low LEDs and the 'inverted' logic
> >>> needs to be added. This includes it using the STATUS_LED_INVERT macro.
> >>
> >> There is already a STATUS_LED_ACTIVE definition (though not one per LED)
> >> in
> >> include/status_led.h for some platforms. Wouldn't it be worth keeping the
> >> same
> >> naming here for consistency (i.e. STATUS_LED_ACTIVEn, which would also
> >> imply
> >> exchanging 0/1 values)?
> >
> > I agree. "INVERT" is confusing, because we don't know what is the normal
> > state.
> >
> > Doing like Benoît suggests would be clearer: STATUS_LED_ACTIVE0 or
> > STATUS_LED_ACTIVE1.
> 
> The problem here is that the BIT LEDs are used in the cmd_led and it
> does not have the 'active' knowledge but a ON OFF concept. So what we
> do there is to change the intended status. The STATUS_LED_ACTIVE is
> for the STATUS_LED_BOOT and not for a 'specific' bit.

I meant that the current use of STATUS_LED_ACTIVE could be extended to what you
are trying to do here:
+#ifndef STATUS_LED_ACTIVE
+#define STATUS_LED_ACTIVE 1
+#endif
+#ifndef STATUS_LED_ACTIVE1
+#define STATUS_LED_ACTIVE1 1
+#endif
+#ifndef STATUS_LED_ACTIVE2
+#define STATUS_LED_ACTIVE2 1
+#endif
+#ifndef STATUS_LED_ACTIVE3
+#define STATUS_LED_ACTIVE3 1
+#endif

But then, maybe it's not the call to __led_set() that should be changed, but
rather how the passed value is used in the underlying layer, e.g. in
drivers/misc/gpio_led.c. However, since the status LED API takes binary states,
it better fits in drivers/misc/status_led.c as you did.

That means that __led_set() would no longer take STATUS_LED_ON/OFF, but rather
something like STATUS_LED_HI/LO, and led_state_value() would convert
STATUS_LED_ON/OFF to STATUS_LED_HI/LO according to STATUS_LED_ACTIVEn.

Best regards,
Benoît

Patch

diff --git a/doc/README.LED b/doc/README.LED
index c3bcb3a..c14555a 100644
--- a/doc/README.LED
+++ b/doc/README.LED
@@ -43,6 +43,8 @@  STATUS_LED_RED is the red LED.  It is used signal errors. This must be a valid
 STATUS_LED_BIT value. Other similar color LED's are STATUS_LED_YELLOW and
 STATUS_LED_BLUE.
 
+STATUS_LED_INVERT and STATUS_LED_INVERT<n> to use active-low LEDs.
+
 These board must define these functions
 
 __led_init is called once to initialize the LED to STATUS_LED_STATE. One time
diff --git a/drivers/misc/status_led.c b/drivers/misc/status_led.c
index 6b71ad4..5878d15 100644
--- a/drivers/misc/status_led.c
+++ b/drivers/misc/status_led.c
@@ -23,19 +23,22 @@  typedef struct {
 	led_id_t mask;
 	int state;
 	int period;
+	int invert;
 	int cnt;
 } led_dev_t;
 
-led_dev_t led_dev[] = {
+static led_dev_t led_dev[] = {
     {	STATUS_LED_BIT,
 	STATUS_LED_STATE,
 	STATUS_LED_PERIOD,
+	STATUS_LED_INVERT,
 	0,
     },
 #if defined(STATUS_LED_BIT1)
     {	STATUS_LED_BIT1,
 	STATUS_LED_STATE1,
 	STATUS_LED_PERIOD1,
+	STATUS_LED_INVERT1,
 	0,
     },
 #endif
@@ -43,6 +46,7 @@  led_dev_t led_dev[] = {
     {	STATUS_LED_BIT2,
 	STATUS_LED_STATE2,
 	STATUS_LED_PERIOD2,
+	STATUS_LED_INVERT2,
 	0,
     },
 #endif
@@ -50,6 +54,7 @@  led_dev_t led_dev[] = {
     {	STATUS_LED_BIT3,
 	STATUS_LED_STATE3,
 	STATUS_LED_PERIOD3,
+	STATUS_LED_INVERT3,
 	0,
     },
 #endif
@@ -64,9 +69,11 @@  static void status_led_init (void)
 	led_dev_t *ld;
 	int i;
 
-	for (i = 0, ld = led_dev; i < MAX_LED_DEV; i++, ld++)
-		__led_init (ld->mask, ld->state);
+	/* Avoid join in a call loop */
 	status_led_init_done = 1;
+
+	for (i = 0, ld = led_dev; i < MAX_LED_DEV; i++, ld++)
+		status_led_set (i, ld->state);
 }
 
 void status_led_tick (ulong timestamp)
@@ -107,5 +114,13 @@  void status_led_set (int led, int state)
 		ld->cnt = 0;		/* always start with full period    */
 		state = STATUS_LED_ON;	/* always start with LED _ON_       */
 	}
+
+	if (ld->invert) {
+		if (state == STATUS_LED_ON)
+			state = STATUS_LED_OFF;
+		else
+			state = STATUS_LED_ON;
+	}
+
 	__led_set (ld->mask, state);
 }
diff --git a/include/status_led.h b/include/status_led.h
index ecff60d..0da3fda 100644
--- a/include/status_led.h
+++ b/include/status_led.h
@@ -288,6 +288,20 @@  extern void __led_set (led_id_t mask, int state);
 #else
 # error Status LED configuration missing
 #endif
+
+#ifndef STATUS_LED_INVERT
+#define STATUS_LED_INVERT 0
+#endif
+#ifndef STATUS_LED_INVERT1
+#define STATUS_LED_INVERT1 0
+#endif
+#ifndef STATUS_LED_INVERT2
+#define STATUS_LED_INVERT2 0
+#endif
+#ifndef STATUS_LED_INVERT3
+#define STATUS_LED_INVERT3 0
+#endif
+
 /************************************************************************/
 
 #ifndef CONFIG_BOARD_SPECIFIC_LED