| Message ID | 1380338659-7896-6-git-send-email-otavio@ossystems.com.br |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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.
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
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
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(-)