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

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

[Sicelo]

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.


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.

Sincerely
Sicelo

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

[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;
                };
        };

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3271 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]