| Message ID | 1621003477-11250-3-git-send-email-michal.vokac@ysoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | Regression in lp55xx driver since multicolor framework was added? | expand |
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
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
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>; }; };
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(-)