Re: [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs
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 wrote: > > On Sat, Sep 28, 2013 at 11:17 AM, Benoît Thébaudeau > > 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs
On Sat, Sep 28, 2013 at 1:49 PM, Fabio Estevam wrote: > On Sat, Sep 28, 2013 at 11:17 AM, Benoît Thébaudeau > 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. -- Otavio Salvador O.S. Systems http://www.ossystems.com.brhttp://code.ossystems.com.br Mobile: +55 (53) 9981-7854Mobile: +1 (347) 903-9750 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs
On Sat, Sep 28, 2013 at 11:17 AM, Benoît Thébaudeau 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs
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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
