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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 24 Jun 2021 09:54:32 +0200
From:   Michal Vokáč <michal.vokac@...ft.com>
To:     Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>
Cc:     Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Shawn Guo <shawnguo@...nel.org>,
        Fabio Estevam <festevam@...il.com>, devicetree@...r.kernel.org,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        NXP Linux Team <linux-imx@....com>,
        linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [RFC 2/2] ARM: dts: imx6dl-yapp4: Fix lp5562 driver probe

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