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

["Benoît Thébaudeau" <benoit.thebaudeau@advansee.com>]

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