From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?= Date: Sun, 29 Sep 2013 15:40:10 +0200 (CEST) Subject: [U-Boot] [PATCH 6/7] status_led: Add support for inverted LEDs In-Reply-To: References: <1380338659-7896-1-git-send-email-otavio@ossystems.com.br> <1380338659-7896-6-git-send-email-otavio@ossystems.com.br> <1059532413.2231026.1380377822470.JavaMail.zimbra@advansee.com> Message-ID: <1799293256.2243041.1380462010404.JavaMail.zimbra@advansee.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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