[RFC 0/2] Regression in lp55xx driver since multicolor framework was added?

[RFC 0/2] Regression in lp55xx driver since multicolor framework was added?

[Michal Vokáč]

Hi,

Linux v5.9 and newer fails on our platform that uses the lp5562 LED
controller with the following error:

lp5562: probe of 1-0030 failed with error -22

The problem exists since introduction of the multicolor framework to
the lp55xx driver. It is caused by the lp55xx_parse_logical_led function
here [1]. Even if the LED multicolor framework is not enabled the color
property is required. But it did not used to be.

There are at least two other in-tree platforms that use the same
controller and could suffer from this problem.

I wonder what is a proper fix here. Either fix the driver to not
require the color property (do not know ho to do so) or update the binding
and DTs to reflect the real state of the driver (in this series).

Thank you in advance for any comments,
Michal

[1] https://elixir.bootlin.com/linux/v5.13-rc1/source/drivers/leds/leds-lp55xx-common.c#L639

Michal Vokáč (2):
  dt-bindings: leds: Add color as a required property for lp55xx
    controller
  ARM: dts: imx6dl-yapp4: Fix lp5562 driver probe

 Documentation/devicetree/bindings/leds/leds-lp55xx.yaml | 10 ++++++++++
 arch/arm/boot/dts/imx6dl-yapp4-common.dtsi              | 11 ++++-------
 2 files changed, 14 insertions(+), 7 deletions(-)

-- 
2.7.4

[RFC 1/2] dt-bindings: leds: Add color as a required property for lp55xx controller

[Michal Vokáč]

Since addition of the multicolor LED framework in commit 92a81562e695
("leds: lp55xx: Add multicolor framework support to lp55xx") the color
property becomes required even if the multicolor framework is not enabled
and used.

Fix the binding documentation to reflect the real state.

Fixes: 92a81562e695 ("leds: lp55xx: Add multicolor framework support to lp55xx")
Cc: <stable@vger.kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: linux-leds@vger.kernel.org
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 Documentation/devicetree/bindings/leds/leds-lp55xx.yaml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
index f552cd143d5b..e6bdd1cb615a 100644
--- a/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
@@ -101,6 +101,7 @@ patternProperties:
         description: name of channel
 
 required:
+  - color
   - compatible
   - reg
 
@@ -127,6 +128,7 @@ examples:
                chan-name = "d1";
                led-cur = /bits/ 8 <0x14>;
                max-cur = /bits/ 8 <0x20>;
+               color = <LED_COLOR_ID_RED>;
            };
 
            led@1 {
@@ -134,6 +136,7 @@ examples:
                chan-name = "d2";
                led-cur = /bits/ 8 <0x14>;
                max-cur = /bits/ 8 <0x20>;
+               color = <LED_COLOR_ID_BLUE>;
            };
 
            led@2 {
@@ -141,6 +144,7 @@ examples:
                chan-name = "d3";
                led-cur = /bits/ 8 <0x14>;
                max-cur = /bits/ 8 <0x20>;
+               color = <LED_COLOR_ID_GREEN>;
            };
 
            led@3 {
@@ -148,6 +152,7 @@ examples:
                chan-name = "d4";
                led-cur = /bits/ 8 <0x14>;
                max-cur = /bits/ 8 <0x20>;
+               color = <LED_COLOR_ID_RED>;
            };
 
            led@4 {
@@ -155,6 +160,7 @@ examples:
                chan-name = "d5";
                led-cur = /bits/ 8 <0x14>;
                max-cur = /bits/ 8 <0x20>;
+               color = <LED_COLOR_ID_BLUE>;
            };
 
            led@5 {
@@ -162,6 +168,7 @@ examples:
                chan-name = "d6";
                led-cur = /bits/ 8 <0x14>;
                max-cur = /bits/ 8 <0x20>;
+               color = <LED_COLOR_ID_GREEN>;
            };
 
            led@6 {
@@ -169,6 +176,7 @@ examples:
                chan-name = "d7";
                led-cur = /bits/ 8 <0x14>;
                max-cur = /bits/ 8 <0x20>;
+               color = <LED_COLOR_ID_RED>;
            };
 
            led@7 {
@@ -176,6 +184,7 @@ examples:
                chan-name = "d8";
                led-cur = /bits/ 8 <0x14>;
                max-cur = /bits/ 8 <0x20>;
+               color = <LED_COLOR_ID_BLUE>;
            };
 
            led@8 {
@@ -183,6 +192,7 @@ examples:
                chan-name = "d9";
                led-cur = /bits/ 8 <0x14>;
                max-cur = /bits/ 8 <0x20>;
+               color = <LED_COLOR_ID_GREEN>;
            };
         };
 
-- 
2.7.4

Re: [RFC 1/2] dt-bindings: leds: Add color as a required property for lp55xx controller

[Rob Herring]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

On Fri, 14 May 2021 16:44:36 +0200, Michal Vokáč wrote:
> Since addition of the multicolor LED framework in commit 92a81562e695
> ("leds: lp55xx: Add multicolor framework support to lp55xx") the color
> property becomes required even if the multicolor framework is not enabled
> and used.
> 
> Fix the binding documentation to reflect the real state.
> 
> Fixes: 92a81562e695 ("leds: lp55xx: Add multicolor framework support to lp55xx")
> Cc: <stable@vger.kernel.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: linux-leds@vger.kernel.org
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
>  Documentation/devicetree/bindings/leds/leds-lp55xx.yaml | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-lp55xx.example.dt.yaml: led-controller@32: 'color' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-lp55xx.example.dt.yaml: led-controller@33: 'color' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml

See https://patchwork.ozlabs.org/patch/1478502

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[RFC 2/2] ARM: dts: imx6dl-yapp4: Fix lp5562 driver probe

[Michal Vokáč]

Since the LED multicolor framework support was added in commit
92a81562e695 ("leds: lp55xx: Add multicolor framework support to lp55xx")
LEDs on this platform stopped working.

Author of the framework attempted to accommodate this DT to the
framework in commit b86d3d21cd4c ("ARM: dts: imx6dl-yapp4: Add reg property
to the lp5562 channel node") but that is not sufficient. A color property
is now required even if the multicolor framework is not used, otherwise
the driver probe fails:

  lp5562: probe of 1-0030 failed with error -22

Add the color property to fix this and remove the actually unused white
channel.

Fixes: b86d3d21cd4c ("ARM: dts: imx6dl-yapp4: Add reg property to the lp5562 channel node")
Cc: <stable@vger.kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: linux-leds@vger.kernel.org
Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
 arch/arm/boot/dts/imx6dl-yapp4-common.dtsi | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi b/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
index 7d2c72562c73..3107bf7fbce5 100644
--- a/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
+++ b/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
@@ -5,6 +5,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/leds/common.h>
 #include <dt-bindings/pwm/pwm.h>
 
 / {
@@ -271,6 +272,7 @@
 			led-cur = /bits/ 8 <0x20>;
 			max-cur = /bits/ 8 <0x60>;
 			reg = <0>;
+			color = <LED_COLOR_ID_RED>;
 		};
 
 		chan@1 {
@@ -278,6 +280,7 @@
 			led-cur = /bits/ 8 <0x20>;
 			max-cur = /bits/ 8 <0x60>;
 			reg = <1>;
+			color = <LED_COLOR_ID_GREEN>;
 		};
 
 		chan@2 {
@@ -285,13 +288,7 @@
 			led-cur = /bits/ 8 <0x20>;
 			max-cur = /bits/ 8 <0x60>;
 			reg = <2>;
-		};
-
-		chan@3 {
-			chan-name = "W";
-			led-cur = /bits/ 8 <0x0>;
-			max-cur = /bits/ 8 <0x0>;
-			reg = <3>;
+			color = <LED_COLOR_ID_BLUE>;
 		};
 	};
 
-- 
2.7.4

Re: [RFC 2/2] ARM: dts: imx6dl-yapp4: Fix lp5562 driver probe

[Pavel Machek]

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

On Fri 2021-05-14 16:44:37, Michal Vokáč wrote:
> Since the LED multicolor framework support was added in commit
> 92a81562e695 ("leds: lp55xx: Add multicolor framework support to lp55xx")
> LEDs on this platform stopped working.
> 
> Author of the framework attempted to accommodate this DT to the
> framework in commit b86d3d21cd4c ("ARM: dts: imx6dl-yapp4: Add reg property
> to the lp5562 channel node") but that is not sufficient. A color property
> is now required even if the multicolor framework is not used, otherwise
> the driver probe fails:
> 
>   lp5562: probe of 1-0030 failed with error -22
> 
> Add the color property to fix this and remove the actually unused white
> channel.
> 
> Fixes: b86d3d21cd4c ("ARM: dts: imx6dl-yapp4: Add reg property to the lp5562 channel node")

I believe this is for arm maintainers to take...

> diff --git a/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi b/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
> index 7d2c72562c73..3107bf7fbce5 100644
> --- a/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
> +++ b/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
> @@ -5,6 +5,7 @@
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/interrupt-controller/irq.h>
>  #include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
>  #include <dt-bindings/pwm/pwm.h>
>  
>  / {
> @@ -271,6 +272,7 @@
>  			led-cur = /bits/ 8 <0x20>;
>  			max-cur = /bits/ 8 <0x60>;
>  			reg = <0>;
> +			color = <LED_COLOR_ID_RED>;
>  		};
>  
>  		chan@1 {
> @@ -278,6 +280,7 @@
>  			led-cur = /bits/ 8 <0x20>;
>  			max-cur = /bits/ 8 <0x60>;
>  			reg = <1>;
> +			color = <LED_COLOR_ID_GREEN>;
>  		};
>  
>  		chan@2 {
> @@ -285,13 +288,7 @@
>  			led-cur = /bits/ 8 <0x20>;
>  			max-cur = /bits/ 8 <0x60>;
>  			reg = <2>;
> -		};
> -
> -		chan@3 {
> -			chan-name = "W";
> -			led-cur = /bits/ 8 <0x0>;
> -			max-cur = /bits/ 8 <0x0>;
> -			reg = <3>;
> +			color = <LED_COLOR_ID_BLUE>;
>  		};
>  	};
>  

What is going on here? "White" channel seems to have disappeared?

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

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

Re: [RFC 2/2] ARM: dts: imx6dl-yapp4: Fix lp5562 driver probe

[Michal Vokáč]

On 23. 06. 21 22:13, Pavel Machek wrote:
> On Fri 2021-05-14 16:44:37, Michal Vokáč wrote:
>> Since the LED multicolor framework support was added in commit
>> 92a81562e695 ("leds: lp55xx: Add multicolor framework support to lp55xx")
>> LEDs on this platform stopped working.
>>
>> Author of the framework attempted to accommodate this DT to the
>> framework in commit b86d3d21cd4c ("ARM: dts: imx6dl-yapp4: Add reg property
>> to the lp5562 channel node") but that is not sufficient. A color property
>> is now required even if the multicolor framework is not used, otherwise
>> the driver probe fails:
>>
>>    lp5562: probe of 1-0030 failed with error -22
>>
>> Add the color property to fix this and remove the actually unused white
>> channel.
>>
>> Fixes: b86d3d21cd4c ("ARM: dts: imx6dl-yapp4: Add reg property to the lp5562 channel node")
> 
> I believe this is for arm maintainers to take...

Hi Pavel,

Thank you for your reply.
As described in the cover letter, my primary intention was to bring
attention to the problem. Addition of the multicolor framework broke
devicetree forward compatibility. The old devicetrees does not work
with newer kernels. Addition of the multicolor framework caused the
color property to become a required one even if the framework is
not enabled in kernel config nor used in the dts. So the reality and
the dt-bindings documentation do not match.

IMO this could be fixed in two ways. First is adapt the dt-binding
documentation to match the reality. State that the color property is
always required. With this we need to fix all the examples and dts
files by adding the color property. This is quite tricky because we
do not always know the color and it also becomes required for the
led-controller node. See the error reported by Rob's bot for patch 1/2:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-lp55xx.example.dt.yaml: led-controller@32: 'color' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/leds/leds-lp55xx.yaml

Second option is to fix this in the LED driver. The driver should not
require the color property if the multicolor framework is not enabled.

I would really like to know Rob's opinion here.
  
>> diff --git a/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi b/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
>> index 7d2c72562c73..3107bf7fbce5 100644
>> --- a/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
>> +++ b/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
>> @@ -5,6 +5,7 @@
>>   #include <dt-bindings/gpio/gpio.h>
>>   #include <dt-bindings/interrupt-controller/irq.h>
>>   #include <dt-bindings/input/input.h>
>> +#include <dt-bindings/leds/common.h>
>>   #include <dt-bindings/pwm/pwm.h>
>>   
>>   / {
>> @@ -271,6 +272,7 @@
>>   			led-cur = /bits/ 8 <0x20>;
>>   			max-cur = /bits/ 8 <0x60>;
>>   			reg = <0>;
>> +			color = <LED_COLOR_ID_RED>;
>>   		};
>>   
>>   		chan@1 {
>> @@ -278,6 +280,7 @@
>>   			led-cur = /bits/ 8 <0x20>;
>>   			max-cur = /bits/ 8 <0x60>;
>>   			reg = <1>;
>> +			color = <LED_COLOR_ID_GREEN>;
>>   		};
>>   
>>   		chan@2 {
>> @@ -285,13 +288,7 @@
>>   			led-cur = /bits/ 8 <0x20>;
>>   			max-cur = /bits/ 8 <0x60>;
>>   			reg = <2>;
>> -		};
>> -
>> -		chan@3 {
>> -			chan-name = "W";
>> -			led-cur = /bits/ 8 <0x0>;
>> -			max-cur = /bits/ 8 <0x0>;
>> -			reg = <3>;
>> +			color = <LED_COLOR_ID_BLUE>;
>>   		};
>>   	};
>>   
> 
> What is going on here? "White" channel seems to have disappeared?

Yes, it is described in the commit message. I know this is not optimal.
The white channel is actually not used on this platform. So the right
approach would be to add the white color property in this fix commit
and remove the whole chan@3 node in next commit. I can do it that way.

Michal