lp5523/lp5xx-common : Keyboard and RGB LEDs Not Working on Nokia N900

Hi

For some time now, the keyboard LEDs on Nokia N900 have not been
working. In dmesg is the error:
  
    lp5523x probe of 2-0032 failed with error -22

and consequently they do not get populated under /sys/class/leds.

I have finally had time to look into this, and found that the N900 dts
falls short of the current lp55xx binding specs. I have updated it and
will be submitting a separate patch. However, this was not enough to
make them work, and a new error appeared:

    [11363.247375] lp5523x 2-0032: GPIO lookup for consumer enable
    [11363.254394] lp5523x 2-0032: using device tree for GPIO lookup
    [11363.254455] of_get_named_gpiod_flags: can't parse 'enable-gpios' property of node '/ocp@68000000/i2c@48072000/lp5523@32[0]'
    [11363.254547] of_get_named_gpiod_flags: parsed 'enable-gpio' property of node '/ocp@68000000/i2c@48072000/lp5523@32[0]' - status (0)
    [11363.254638] gpio gpiochip1: Persistence not supported for GPIO 9
    [11363.254669] gpio-41 (enable): no flags found for enable
    [11363.295959] lp5523x 2-0032: device detection err: -121
    [11363.303710] lp5523x: probe of 2-0032 failed with error -121


This is because the chip does not get enabled even though dts contains:

    enable-gpios = <&gpio2 9 GPIO_ACTIVE_HIGH>; /* 41 */

which is correct for this board.


I came up with this patch (against 5.12-rc6), which makes it work again.



However, I am not sure if this is a proper way to fix the issue, or if
it is a dirty hack. It looks like all the chips handled by this driver
(lp5521, lp5523, lp5562, and lp8501) get enabled by pulling their ENable
pin high, so I suppose it is fine, but would like to get some feedback
before submitting a cleaned up patch. It is also possible that this
should be defined via dts elsewhere, although at this point I am not
sure how.

Looking forward to your help.

Sincerely
Sicelo

Hi

On Tue, Apr 13, 2021 at 07:02:26PM +0200, Sicelo wrote:
> Hi

> 

> For some time now, the keyboard LEDs on Nokia N900 have not been

> working. In dmesg is the error:

>   

>     lp5523x probe of 2-0032 failed with error -22

> 

> and consequently they do not get populated under /sys/class/leds.

> 

> I have finally had time to look into this, and found that the N900 dts

> falls short of the current lp55xx binding specs. I have updated it and

> will be submitting a separate patch. However, this was not enough to


I have since realized that I should have included the adjusted dts in
the initial email. Please find it attached in this follow-up email and
sincere apologies for this ommission.

> make them work, and a new error appeared:

> 

>     [11363.247375] lp5523x 2-0032: GPIO lookup for consumer enable

>     [11363.254394] lp5523x 2-0032: using device tree for GPIO lookup

>     [11363.254455] of_get_named_gpiod_flags: can't parse 'enable-gpios' property of node '/ocp@68000000/i2c@48072000/lp5523@32[0]'

>     [11363.254547] of_get_named_gpiod_flags: parsed 'enable-gpio' property of node '/ocp@68000000/i2c@48072000/lp5523@32[0]' - status (0)

>     [11363.254638] gpio gpiochip1: Persistence not supported for GPIO 9

>     [11363.254669] gpio-41 (enable): no flags found for enable

>     [11363.295959] lp5523x 2-0032: device detection err: -121

>     [11363.303710] lp5523x: probe of 2-0032 failed with error -121

> 

> 

> This is because the chip does not get enabled even though dts contains:

> 

>     enable-gpios = <&gpio2 9 GPIO_ACTIVE_HIGH>; /* 41 */

> 

> which is correct for this board.

> 

> 

> I came up with this patch (against 5.12-rc6), which makes it work again.

> 

> 

> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c

> index 81de1346bf5d..9af84fc335b3 100644

> --- a/drivers/leds/leds-lp55xx-common.c

> +++ b/drivers/leds/leds-lp55xx-common.c

> @@ -694,7 +694,7 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,

>         of_property_read_u8(np, "clock-mode", &pdata->clock_mode);

> 

>         pdata->enable_gpiod = devm_gpiod_get_optional(dev, "enable",

> -                                                     GPIOD_ASIS);

> +                                                     GPIOD_OUT_HIGH);

>         if (IS_ERR(pdata->enable_gpiod))

>                 return ERR_CAST(pdata->enable_gpiod);

> 

> 

> However, I am not sure if this is a proper way to fix the issue, or if

> it is a dirty hack. It looks like all the chips handled by this driver

> (lp5521, lp5523, lp5562, and lp8501) get enabled by pulling their ENable

> pin high, so I suppose it is fine, but would like to get some feedback

> before submitting a cleaned up patch. It is also possible that this

> should be defined via dts elsewhere, although at this point I am not

> sure how.

> 

> Looking forward to your help.


---

diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts
index 32335d4ce478..d40c3d2c4914 100644
--- a/arch/arm/boot/dts/omap3-n900.dts
+++ b/arch/arm/boot/dts/omap3-n900.dts
@@ -8,6 +8,7 @@

 #include "omap34xx.dtsi"
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/leds/common.h>

 /*
  * Default secure signed bootloader (Nokia X-Loader) does not enable L3 firewall
@@ -630,63 +631,92 @@ indicator {
        };

        lp5523: lp5523@32 {
+               #address-cells = <1>;
+               #size-cells = <0>;
                compatible = "national,lp5523";
                reg = <0x32>;
                clock-mode = /bits/ 8 <0>; /* LP55XX_CLOCK_AUTO */
-               enable-gpio = <&gpio2 9 GPIO_ACTIVE_HIGH>; /* 41 */
+               enable-gpios = <&gpio2 9 GPIO_ACTIVE_HIGH>; /* 41 */

-               chan0 {
+               led@0 {
+                       reg = <0>;
                        chan-name = "lp5523:kb1";
                        led-cur = /bits/ 8 <50>;
                        max-cur = /bits/ 8 <100>;
+                       color = <LED_COLOR_ID_WHITE>;
+                       function = LED_FUNCTION_KBD_BACKLIGHT;
                };

-               chan1 {
+               led@1 {
+                       reg = <1>;
                        chan-name = "lp5523:kb2";
                        led-cur = /bits/ 8 <50>;
                        max-cur = /bits/ 8 <100>;
+                       color = <LED_COLOR_ID_WHITE>;
+                       function = LED_FUNCTION_KBD_BACKLIGHT;
                };

-               chan2 {
+               led@2 {
+                       reg = <2>;
                        chan-name = "lp5523:kb3";
                        led-cur = /bits/ 8 <50>;
                        max-cur = /bits/ 8 <100>;
+                       color = <LED_COLOR_ID_WHITE>;
+                       function = LED_FUNCTION_KBD_BACKLIGHT;
                };

-               chan3 {
+               led@3 {
+                       reg = <3>;
                        chan-name = "lp5523:kb4";
                        led-cur = /bits/ 8 <50>;
                        max-cur = /bits/ 8 <100>;
+                       color = <LED_COLOR_ID_WHITE>;
+                       function = LED_FUNCTION_KBD_BACKLIGHT;
                };

-               chan4 {
+               led@4 {
+                       reg = <4>;
                        chan-name = "lp5523:b";
                        led-cur = /bits/ 8 <50>;
                        max-cur = /bits/ 8 <100>;
+                       color = <LED_COLOR_ID_BLUE>;
+                       function = LED_FUNCTION_STATUS;
                };

-               chan5 {
+               led@5 {
+                       reg = <5>;
                        chan-name = "lp5523:g";
                        led-cur = /bits/ 8 <50>;
                        max-cur = /bits/ 8 <100>;
+                       color = <LED_COLOR_ID_GREEN>;
+                       function = LED_FUNCTION_STATUS;
                };

-               chan6 {
+               led@6 {
+                       reg = <6>;
                        chan-name = "lp5523:r";
                        led-cur = /bits/ 8 <50>;
                        max-cur = /bits/ 8 <100>;
+                       color = <LED_COLOR_ID_RED>;
+                       function = LED_FUNCTION_STATUS;
                };

-               chan7 {
+               led@7 {
+                       reg = <7>;
                        chan-name = "lp5523:kb5";
                        led-cur = /bits/ 8 <50>;
                        max-cur = /bits/ 8 <100>;
+                       color = <LED_COLOR_ID_WHITE>;
+                       function = LED_FUNCTION_KBD_BACKLIGHT;
                };

-               chan8 {
+               led@8 {
+                       reg = <8>;
                        chan-name = "lp5523:kb6";
                        led-cur = /bits/ 8 <50>;
                        max-cur = /bits/ 8 <100>;
+                       color = <LED_COLOR_ID_WHITE>;
+                       function = LED_FUNCTION_KBD_BACKLIGHT;
                };
        };

Hi!

> > For some time now, the keyboard LEDs on Nokia N900 have not been

> > working. In dmesg is the error:

> >   

> >     lp5523x probe of 2-0032 failed with error -22

> > 

> > and consequently they do not get populated under /sys/class/leds.

> > 

> > I have finally had time to look into this, and found that the N900 dts

> > falls short of the current lp55xx binding specs. I have updated it and

> > will be submitting a separate patch. However, this was not enough to

> 

> I have since realized that I should have included the adjusted dts in

> the initial email. Please find it attached in this follow-up email and

> sincere apologies for this ommission.

> 

> > make them work, and a new error appeared:

> > 

> >     [11363.247375] lp5523x 2-0032: GPIO lookup for consumer enable

> >     [11363.254394] lp5523x 2-0032: using device tree for GPIO lookup

> >     [11363.254455] of_get_named_gpiod_flags: can't parse 'enable-gpios' property of node '/ocp@68000000/i2c@48072000/lp5523@32[0]'

> >     [11363.254547] of_get_named_gpiod_flags: parsed 'enable-gpio' property of node '/ocp@68000000/i2c@48072000/lp5523@32[0]' - status (0)

> >     [11363.254638] gpio gpiochip1: Persistence not supported for GPIO 9

> >     [11363.254669] gpio-41 (enable): no flags found for enable

> >     [11363.295959] lp5523x 2-0032: device detection err: -121

> >     [11363.303710] lp5523x: probe of 2-0032 failed with error -121

> > 

> > 

> > This is because the chip does not get enabled even though dts contains:

> > 

> >     enable-gpios = <&gpio2 9 GPIO_ACTIVE_HIGH>; /* 41 */

> > 

> > which is correct for this board.

> > 

> > 

> > I came up with this patch (against 5.12-rc6), which makes it work again.

> > 

> > 

> > diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c

> > index 81de1346bf5d..9af84fc335b3 100644

> > --- a/drivers/leds/leds-lp55xx-common.c

> > +++ b/drivers/leds/leds-lp55xx-common.c

> > @@ -694,7 +694,7 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,

> >         of_property_read_u8(np, "clock-mode", &pdata->clock_mode);

> > 

> >         pdata->enable_gpiod = devm_gpiod_get_optional(dev, "enable",

> > -                                                     GPIOD_ASIS);

> > +                                                     GPIOD_OUT_HIGH);

> >         if (IS_ERR(pdata->enable_gpiod))

> >                 return ERR_CAST(pdata->enable_gpiod);


This does not look obviously bad. If this is sent as patch I can
simply apply, I'll likely do it.

> > However, I am not sure if this is a proper way to fix the issue, or if

> > it is a dirty hack. It looks like all the chips handled by this driver

> > (lp5521, lp5523, lp5562, and lp8501) get enabled by pulling their ENable

> > pin high, so I suppose it is fine, but would like to get some feedback

> > before submitting a cleaned up patch. It is also possible that this

> > should be defined via dts elsewhere, although at this point I am not

> > sure how.

> > 

> > Looking forward to your help.


But I can't take dts changes; these should likely be in same series
and cc-ed to relevant maintainers.

Best regards,
							Pavel
-- 
http://www.livejournal.com/~pavelmachek