| Message ID | 20250506-led-fix-v1-1-56a39b55a7fc@axis.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] leds: leds-lp50xx: Handle reg to get correct multi_index | expand |
On Tue, 06 May 2025, Johan Adolfsson wrote: > mc_subled used for multi_index needs well defined array indexes, > to guarantee the desired result, optionally use reg for that. > > If devicetree child nodes is processed in random or reverse order > you may end up with multi_index "blue green red" instead of the expected > "red green blue". > If user space apps uses multi_index to deduce how to control the leds > they would most likely be broken without this patch if devicetree > processing is reversed (which it appears to be). > > arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dts has reg set > but I don't see how it can have worked without this change. > > If reg is not set, the previous behavior is kept, index will be in > the order nodes are processed. > > Signed-off-by: Johan Adolfsson <johan.adolfsson@axis.com> > --- > Since devicetree nodes are (sometimes?) processed in reverse order, > support reg as the actual multi_index index so yo get well defined > color order presented in the multi_index file. > Not sure if reusing reg for this is the correct way or if another > property such as "multi_index" or similar should be used instead. > Looks like reg is used for similar things at least. > Or should the whole "reverse the devicetree" problem be fixed instead? > --- > drivers/leds/leds-lp50xx.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c > index 02cb1565a9fb..48db024081f5 100644 > --- a/drivers/leds/leds-lp50xx.c > +++ b/drivers/leds/leds-lp50xx.c > @@ -476,6 +476,7 @@ static int lp50xx_probe_dt(struct lp50xx *priv) > return -ENOMEM; > > fwnode_for_each_child_node(child, led_node) { > + int multi_index = num_colors; > ret = fwnode_property_read_u32(led_node, "color", > &color_id); > if (ret) { > @@ -483,8 +484,15 @@ static int lp50xx_probe_dt(struct lp50xx *priv) > dev_err(priv->dev, "Cannot read color\n"); > return ret; > } > + ret = fwnode_property_read_u32(led_node, "reg", &multi_index); > + if (ret) { > + multi_index = num_colors; Didn't we already initialise this? > + } else if (multi_index >= LP50XX_LEDS_PER_MODULE) { > + dev_warn(priv->dev, "reg %i out of range\n", multi_index); This should probably fail outright. > + multi_index = num_colors; > + } > > - mc_led_info[num_colors].color_index = color_id; > + mc_led_info[multi_index].color_index = color_id; > num_colors++; > } > > > --- > base-commit: 38fec10eb60d687e30c8c6b5420d86e8149f7557 > change-id: 20250225-led-fix-444fb544584a > > Best regards, > -- > Johan Adolfsson <johan.adolfsson@axis.com> >
Hi Johan, On 5/6/25 12:39, Johan Adolfsson wrote: > mc_subled used for multi_index needs well defined array indexes, > to guarantee the desired result, optionally use reg for that. > > If devicetree child nodes is processed in random or reverse order > you may end up with multi_index "blue green red" instead of the expected > "red green blue". > If user space apps uses multi_index to deduce how to control the leds > they would most likely be broken without this patch if devicetree > processing is reversed (which it appears to be). Are you trying to solve some real problem that occurred to you? The order of DT nodes parsing is not a problem here - we save color index in subled_info to be able to figure out which color is on which position. This information can be retrieved in sysfs by reading multi_index file.
From: Jacek Anaszewski <jacek.anaszewski@gmail.com> Sent: Saturday, May 10, 2025 17:32 To: Johan Adolfsson; Lee Jones; Pavel Machek Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index >Hi Johan, > >On 5/6/25 12:39, Johan Adolfsson wrote: >> mc_subled used for multi_index needs well defined array indexes, >> to guarantee the desired result, optionally use reg for that. >> >> If devicetree child nodes is processed in random or reverse order >> you may end up with multi_index "blue green red" instead of the expected >> "red green blue". >> If user space apps uses multi_index to deduce how to control the leds >> they would most likely be broken without this patch if devicetree >> processing is reversed (which it appears to be). > >Are you trying to solve some real problem that occurred to you? Yes! Since the subled is indexed by the processing order, it gets reversed if devicetree processing is reversed (which I understand is a "feature"), so instead of "red green blue" I would get "blue green red" in the multi_index file without this patch. The mapping to the hardware does not match that, so writing "255 0 0" to multi_intensity will give me red and not blue. >The order of DT nodes parsing is not a problem here - we save >color index in subled_info to be able to figure out which color >is on which position. This information can be retrieved in sysfs >by reading multi_index file. Maybe the bug is somewhere else in the leds-lp50xx if that is supposed to work, but I still think it's a good thing to be able to get the expected order in the multi_index file. > >-- >Best regards, >Jacek Anaszewski Best regards /Johan
On 5/12/25 12:59, Johan Adolfsson wrote: > > From: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Sent: Saturday, May 10, 2025 17:32 > To: Johan Adolfsson; Lee Jones; Pavel Machek > Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel > Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index > >> Hi Johan, >> >> On 5/6/25 12:39, Johan Adolfsson wrote: >>> mc_subled used for multi_index needs well defined array indexes, >>> to guarantee the desired result, optionally use reg for that. >>> >>> If devicetree child nodes is processed in random or reverse order >>> you may end up with multi_index "blue green red" instead of the expected >>> "red green blue". >>> If user space apps uses multi_index to deduce how to control the leds >>> they would most likely be broken without this patch if devicetree >>> processing is reversed (which it appears to be). >> >> Are you trying to solve some real problem that occurred to you? > > Yes! Since the subled is indexed by the processing order, it gets reversed if devicetree processing is reversed > (which I understand is a "feature"), so instead of "red green blue" I would get "blue green red" in the multi_index file without this patch. > The mapping to the hardware does not match that, so writing "255 0 0" to multi_intensity will give me red and not blue. You are expected to write intensities to the multi_intensity file according to the order of colors listed in multi_index file. >> The order of DT nodes parsing is not a problem here - we save >> color index in subled_info to be able to figure out which color >> is on which position. This information can be retrieved in sysfs >> by reading multi_index file. > > Maybe the bug is somewhere else in the leds-lp50xx if that is supposed to work, > but I still think it's a good thing to be able to get the expected order in the multi_index file. Please make sure you read Documentation/leds/leds-class-multicolor.rst. There is no such term as "expected order in the multi_index file". The framework is called multicolor, not rgb. The order of colors does not need to be RGB.
Hi Jacek, >From: Jacek Anaszewski <jacek.anaszewski@gmail.com> >Sent: Monday, May 12, 2025 20:10 >To: Johan Adolfsson; Lee Jones; Pavel Machek >Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel >Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index > >On 5/12/25 12:59, Johan Adolfsson wrote: >> >> From: Jacek Anaszewski <jacek.anaszewski@gmail.com> >> Sent: Saturday, May 10, 2025 17:32 >> To: Johan Adolfsson; Lee Jones; Pavel Machek >> Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel >> Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index >> >>> Hi Johan, >>> >>> On 5/6/25 12:39, Johan Adolfsson wrote: >>>> mc_subled used for multi_index needs well defined array indexes, >>>> to guarantee the desired result, optionally use reg for that. >>>> >>>> If devicetree child nodes is processed in random or reverse order >>>> you may end up with multi_index "blue green red" instead of the expected >>>> "red green blue". >>>> If user space apps uses multi_index to deduce how to control the leds >>>> they would most likely be broken without this patch if devicetree >>>> processing is reversed (which it appears to be). >>> >>> Are you trying to solve some real problem that occurred to you? >> >> Yes! Since the subled is indexed by the processing order, it gets reversed if devicetree processing is reversed >> (which I understand is a "feature"), so instead of "red green blue" I would get "blue green red" in the multi_index file without this patch. >> The mapping to the hardware does not match that, so writing "255 0 0" to multi_intensity will give me red and not blue. >You are expected to write intensities to the multi_intensity file >according to the order of colors listed in multi_index file. That is what I did above, but LED turned red and not blue. >>> The order of DT nodes parsing is not a problem here - we save >>> color index in subled_info to be able to figure out which color >>> is on which position. This information can be retrieved in sysfs >>> by reading multi_index file. >> >> Maybe the bug is somewhere else in the leds-lp50xx if that is supposed to work, >> but I still think it's a good thing to be able to get the expected order in the multi_index file. > >Please make sure you read Documentation/leds/leds-class-multicolor.rst. >There is no such term as "expected order in the multi_index file". >The framework is called multicolor, not rgb. The order of colors does >not need to be RGB. Maybe I'm missing something here - but how do i specify that a certain pin on the driver IC is connected to a certain color of the LED. The devicetree looks like this: It seems the first number in multi_intensity seem to go to pin regardless of what multi_index says. multi-led@0 { reg = <0x0>; color = <LED_COLOR_ID_RGB>; label = "led:rgb"; function = "led"; linux,default-trigger = "default-on"; max-brightness = <255>; #address-cells = <1>; #size-cells = <0>; /* Need BLUE GREEN RED here or reg to give red green blue in multi_index! */ led-0@0 { color = <LED_COLOR_ID_RED>; reg = <0>; }; led-1@1 { color = <LED_COLOR_ID_GREEN>; reg = <1>; }; led-2@2 { color = <LED_COLOR_ID_BLUE>; reg = <4>; }; }; >-- >Best regards, >Jacek Anaszewski Best regards /Johan
Hi Johan, On 5/13/25 15:04, Johan Adolfsson wrote: > > Hi Jacek, > >> From: Jacek Anaszewski <jacek.anaszewski@gmail.com> >> Sent: Monday, May 12, 2025 20:10 >> To: Johan Adolfsson; Lee Jones; Pavel Machek >> Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel >> Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index >> >> On 5/12/25 12:59, Johan Adolfsson wrote: >>> >>> From: Jacek Anaszewski <jacek.anaszewski@gmail.com> >>> Sent: Saturday, May 10, 2025 17:32 >>> To: Johan Adolfsson; Lee Jones; Pavel Machek >>> Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel >>> Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index >>> >>>> Hi Johan, >>>> >>>> On 5/6/25 12:39, Johan Adolfsson wrote: >>>>> mc_subled used for multi_index needs well defined array indexes, >>>>> to guarantee the desired result, optionally use reg for that. >>>>> >>>>> If devicetree child nodes is processed in random or reverse order >>>>> you may end up with multi_index "blue green red" instead of the expected >>>>> "red green blue". >>>>> If user space apps uses multi_index to deduce how to control the leds >>>>> they would most likely be broken without this patch if devicetree >>>>> processing is reversed (which it appears to be). >>>> >>>> Are you trying to solve some real problem that occurred to you? >>> >>> Yes! Since the subled is indexed by the processing order, it gets reversed if devicetree processing is reversed >>> (which I understand is a "feature"), so instead of "red green blue" I would get "blue green red" in the multi_index file without this patch. >>> The mapping to the hardware does not match that, so writing "255 0 0" to multi_intensity will give me red and not blue. > >> You are expected to write intensities to the multi_intensity file >> according to the order of colors listed in multi_index file. > > That is what I did above, but LED turned red and not blue. > >>>> The order of DT nodes parsing is not a problem here - we save >>>> color index in subled_info to be able to figure out which color >>>> is on which position. This information can be retrieved in sysfs >>>> by reading multi_index file. >>> >>> Maybe the bug is somewhere else in the leds-lp50xx if that is supposed to work, >>> but I still think it's a good thing to be able to get the expected order in the multi_index file. >> >> Please make sure you read Documentation/leds/leds-class-multicolor.rst. >> There is no such term as "expected order in the multi_index file". >> The framework is called multicolor, not rgb. The order of colors does >> not need to be RGB. > > Maybe I'm missing something here - but how do i specify that a certain pin on the driver IC is connected to a certain color of the LED. > The devicetree looks like this: > It seems the first number in multi_intensity seem to go to pin regardless of what multi_index says. OK, indeed there is a problem. Let's continue in your patch thread. > > multi-led@0 { > reg = <0x0>; > color = <LED_COLOR_ID_RGB>; > label = "led:rgb"; > function = "led"; > linux,default-trigger = "default-on"; > max-brightness = <255>; > #address-cells = <1>; > #size-cells = <0>; > > /* Need BLUE GREEN RED here or reg to give red green blue in multi_index! */ > led-0@0 { > color = <LED_COLOR_ID_RED>; > reg = <0>; > }; > > led-1@1 { > color = <LED_COLOR_ID_GREEN>; > reg = <1>; > }; > > led-2@2 { > color = <LED_COLOR_ID_BLUE>; > reg = <4>; This needs to match node-name[@unit-address], i.e. here 2. > }; > }; >
Hi Johan, On 5/6/25 12:39, Johan Adolfsson wrote: > mc_subled used for multi_index needs well defined array indexes, > to guarantee the desired result, optionally use reg for that. > > If devicetree child nodes is processed in random or reverse order > you may end up with multi_index "blue green red" instead of the expected > "red green blue". > If user space apps uses multi_index to deduce how to control the leds > they would most likely be broken without this patch if devicetree > processing is reversed (which it appears to be). > > arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dts has reg set > but I don't see how it can have worked without this change. > > If reg is not set, the previous behavior is kept, index will be in > the order nodes are processed. > > Signed-off-by: Johan Adolfsson <johan.adolfsson@axis.com> > --- > Since devicetree nodes are (sometimes?) processed in reverse order, > support reg as the actual multi_index index so yo get well defined > color order presented in the multi_index file. > Not sure if reusing reg for this is the correct way or if another > property such as "multi_index" or similar should be used instead. > Looks like reg is used for similar things at least. > Or should the whole "reverse the devicetree" problem be fixed instead? > --- > drivers/leds/leds-lp50xx.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c > index 02cb1565a9fb..48db024081f5 100644 > --- a/drivers/leds/leds-lp50xx.c > +++ b/drivers/leds/leds-lp50xx.c > @@ -476,6 +476,7 @@ static int lp50xx_probe_dt(struct lp50xx *priv) > return -ENOMEM; > > fwnode_for_each_child_node(child, led_node) { > + int multi_index = num_colors; > ret = fwnode_property_read_u32(led_node, "color", > &color_id); > if (ret) { > @@ -483,8 +484,15 @@ static int lp50xx_probe_dt(struct lp50xx *priv) > dev_err(priv->dev, "Cannot read color\n"); > return ret; > } > + ret = fwnode_property_read_u32(led_node, "reg", &multi_index); > + if (ret) { > + multi_index = num_colors; Why not to fail if 'reg' parsing fails? It is marked required in DT bindings [0]. > + } else if (multi_index >= LP50XX_LEDS_PER_MODULE) { > + dev_warn(priv->dev, "reg %i out of range\n", multi_index); > + multi_index = num_colors; > + } > > - mc_led_info[num_colors].color_index = color_id; > + mc_led_info[multi_index].color_index = color_id; > num_colors++; > } > > > --- > base-commit: 38fec10eb60d687e30c8c6b5420d86e8149f7557 > change-id: 20250225-led-fix-444fb544584a > > Best regards, [0] Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
From: Jacek Anaszewski <jacek.anaszewski@gmail.com> Sent: Tuesday, May 13, 2025 21:52 To: Johan Adolfsson; Lee Jones; Pavel Machek Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index .. >> } >> + ret = fwnode_property_read_u32(led_node, "reg", &multi_index); >> + if (ret) { >> + multi_index = num_colors; > >Why not to fail if 'reg' parsing fails? >It is marked required in DT bindings [0]. I didn't want to start failing if reg is missing since it has never been handled until now, despite what the doc says since 2022... ... >[0] Documentation/devicetree/bindings/leds/leds-lp50xx.yaml > >-- >Best regards, >Jacek Anaszewski Best regards /Johan
From: Jacek Anaszewski <jacek.anaszewski@gmail.com> Sent: Tuesday, May 13, 2025 21:50 To: Johan Adolfsson; Lee Jones; Pavel Machek Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index ... >> Maybe I'm missing something here - but how do i specify that a certain pin on the driver IC is connected to a certain color of the LED. >> The devicetree looks like this: >> It seems the first number in multi_intensity seem to go to pin regardless of what multi_index says. >OK, indeed there is a problem. Let's continue in your patch thread. >> >> multi-led@0 { >> reg = <0x0>; >> color = <LED_COLOR_ID_RGB>; >> label = "led:rgb"; >> function = "led"; >> linux,default-trigger = "default-on"; >> max-brightness = <255>; >> #address-cells = <1>; >> #size-cells = <0>; >> >> /* Need BLUE GREEN RED here or reg to give red green blue in multi_index! */ >> led-0@0 { >> color = <LED_COLOR_ID_RED>; >> reg = <0>; >> }; >> >> led-1@1 { >> color = <LED_COLOR_ID_GREEN>; >> reg = <1>; >> }; >> >> led-2@2 { >> color = <LED_COLOR_ID_BLUE>; >> reg = <4>; > >This needs to match node-name[@unit-address], i.e. here 2. Sorry, my bad. I copied the version where I wanted to test my changes to address the comment from Lee Jones and do error out instead of warn if the reg is out of range . >-- >Best regards, >Jacek Anaszewski Best regards /Johan
Hi Johan, On 5/14/25 16:34, Johan Adolfsson wrote: > From: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Sent: Tuesday, May 13, 2025 21:52 > To: Johan Adolfsson; Lee Jones; Pavel Machek > Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel > Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index > .. >>> } >>> + ret = fwnode_property_read_u32(led_node, "reg", &multi_index); >>> + if (ret) { >>> + multi_index = num_colors; >> >> Why not to fail if 'reg' parsing fails? >> It is marked required in DT bindings [0]. > > I didn't want to start failing if reg is missing since it has never been handled until now, despite what the doc says since 2022... There is one in-tree user [0], and we will need to patch it as well, because it uses reg 0,1,2 for each RGB LED module, instead of iout numbers as it will be after your change. We will need to also state clearly in the bindings that 'reg' property maps to iouts for the non-banked RGB LED modules. For banked RGB LED modules it is more tricky, because there is one LED multicolor class device created for them. Probably to be correct we would need make the 'reg' properties in the subnodes also arrays reflecting iouts that will be governed by BANK_A_Color, BANK_B_Color, and BANK_C_Color registers respectively. And DT parser in the driver would need to enforce proper iout definition for the banked modules E.g. the multi-led@3 node from the example should look like below: multi-led@3 { #address-cells = <1>; #size-cells = <0>; reg = <0x3>, <0x4>, <0x5>; color = <LED_COLOR_ID_RGB>; function = LED_FUNCTION_STANDBY; led@9 { reg = <0x9>, <0xc>, <0xf>; color = <LED_COLOR_ID_RED>; }; led@a { reg = <0xa>, <0xd>, <0x10>; color = <LED_COLOR_ID_GREEN>; }; led@b { reg = <0xb>, <0xe>, <0x11>; color = <LED_COLOR_ID_BLUE>; }; [0] arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dts
Hi Jacek, >From: Jacek Anaszewski <jacek.anaszewski@gmail.com> >Sent: Wednesday, May 14, 2025 21:55 >To: Johan Adolfsson; Lee Jones; Pavel Machek >Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel >Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index > >Hi Johan, > >On 5/14/25 16:34, Johan Adolfsson wrote: > From: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Sent: Tuesday, May 13, 2025 21:52 > To: Johan Adolfsson; Lee Jones; Pavel Machek > Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel > Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index > .. >>> } >>> + ret = fwnode_property_read_u32(led_node, "reg", &multi_index); >>> + if (ret) { >>> + multi_index = num_colors; >> >> Why not to fail if 'reg' parsing fails? >> It is marked required in DT bindings [0]. > >> I didn't want to start failing if reg is missing since it has never been handled until now, despite what the doc says since 2022... > >There is one in-tree user [0], and we will need to patch it as well, >because it uses reg 0,1,2 for each RGB LED module, instead of iout >numbers as it will be after your change. Not sure i follow you here. What works for me is starting with reg = <0> for the childs of each bank (multi-led). The register (maps to iout I guess) is calculated based on led_number * 3 + i where i is the offset as well as the color_index in subled_info (reg). So I don't think additional changes to that dts is needed. > >We will need to also state clearly in the bindings that 'reg' property >maps to iouts for the non-banked RGB LED modules. Not sure it actually does, haven't really tested that, or at least not gotten it to work as I expected. For the non-rgb led in the product I requested those to be on the first pin on each bank (wasting 2 pins per bank), since I couldn't figure out how to configure it in another way when prototyping. Since the 3 outputs per bank share a single brightness setting, it could complicate things as well. (And we didn't need all the pins anyway) >For banked RGB LED modules it is more tricky, because there is one >LED multicolor class device created for them. Probably to be correct >we would need make the 'reg' properties in the subnodes also arrays >reflecting iouts that will be governed by BANK_A_Color, BANK_B_Color, >and BANK_C_Color registers respectively. And DT parser in the driver >would need to enforce proper iout definition for the banked modules >E.g. the multi-led@3 node from the example should look like below: > > multi-led@3 { > #address-cells = <1>; > #size-cells = <0>; > reg = <0x3>, <0x4>, <0x5>; > color = <LED_COLOR_ID_RGB>; > function = LED_FUNCTION_STANDBY; > > led@9 { > reg = <0x9>, <0xc>, <0xf>; > color = <LED_COLOR_ID_RED>; > }; > > led@a { > reg = <0xa>, <0xd>, <0x10>; > color = <LED_COLOR_ID_GREEN>; > }; > > led@b { > reg = <0xb>, <0xe>, <0x11>; > color = <LED_COLOR_ID_BLUE>; > }; > > >[0] arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dts Not sure about that, I dont' think we handle an array of reg values within the led node. >-- >Best regards, >Jacek Anaszewski Best regards /Johan
Hi Johan, On 5/15/25 09:36, Johan Adolfsson wrote: > Hi Jacek, > >> From: Jacek Anaszewski <jacek.anaszewski@gmail.com> >> Sent: Wednesday, May 14, 2025 21:55 >> To: Johan Adolfsson; Lee Jones; Pavel Machek >> Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel >> Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index >> >> Hi Johan, >> >> On 5/14/25 16:34, Johan Adolfsson wrote: >> From: Jacek Anaszewski <jacek.anaszewski@gmail.com> >> Sent: Tuesday, May 13, 2025 21:52 >> To: Johan Adolfsson; Lee Jones; Pavel Machek >> Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; Kernel >> Subject: Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index >> .. >>>> } >>>> + ret = fwnode_property_read_u32(led_node, "reg", &multi_index); >>>> + if (ret) { >>>> + multi_index = num_colors; >>> >>> Why not to fail if 'reg' parsing fails? >>> It is marked required in DT bindings [0]. >> >>> I didn't want to start failing if reg is missing since it has never been handled until now, despite what the doc says since 2022... >> >> There is one in-tree user [0], and we will need to patch it as well, >> because it uses reg 0,1,2 for each RGB LED module, instead of iout >> numbers as it will be after your change. > > Not sure i follow you here. What works for me is starting with reg = <0> for the childs of each bank (multi-led). > The register (maps to iout I guess) is calculated based on led_number * 3 + i where i is the offset as well as the color_index in subled_info (reg). > So I don't think additional changes to that dts is needed. OK, if we document that reg properly as an index of the LED within given RGB LED module. Initially I thought that it would be better to map it directly to the IOUT, but the way you propose will allow to keep existing DTS unchanged. >> We will need to also state clearly in the bindings that 'reg' property >> maps to iouts for the non-banked RGB LED modules. > > Not sure it actually does, haven't really tested that, or at least not gotten it to work as I expected. > For the non-rgb led in the product I requested those to be on the first pin on each bank (wasting 2 pins per bank), > since I couldn't figure out how to configure it in another way when prototyping. > > Since the 3 outputs per bank share a single brightness setting, it could complicate things as well. > (And we didn't need all the pins anyway) Here I was talking about non-banked LEDs. But regarding banked LEDs, setting LEDn_Bank_EN bits assigns all three LEDs from given RGB LED module to the bank. Then all IOUTs from the module are controlled via BANK_A/B/C_Color registers, and by common Bank_Brightness register. >> For banked RGB LED modules it is more tricky, because there is one >> LED multicolor class device created for them. Probably to be correct >> we would need make the 'reg' properties in the subnodes also arrays >> reflecting iouts that will be governed by BANK_A_Color, BANK_B_Color, >> and BANK_C_Color registers respectively. And DT parser in the driver >> would need to enforce proper iout definition for the banked modules > >> E.g. the multi-led@3 node from the example should look like below: >> >> multi-led@3 { >> #address-cells = <1>; >> #size-cells = <0>; >> reg = <0x3>, <0x4>, <0x5>; >> color = <LED_COLOR_ID_RGB>; >> function = LED_FUNCTION_STANDBY; >> >> led@9 { >> reg = <0x9>, <0xc>, <0xf>; >> color = <LED_COLOR_ID_RED>; >> }; >> >> led@a { >> reg = <0xa>, <0xd>, <0x10>; >> color = <LED_COLOR_ID_GREEN>; >> }; >> >> led@b { >> reg = <0xb>, <0xe>, <0x11>; >> color = <LED_COLOR_ID_BLUE>; >> }; >> >> >> [0] arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dts > > Not sure about that, I dont' think we handle an array of reg values within the led node. Above I proposed to implement handling of it. But, if we are not going to treat subLED's reg properties as IOUTs, but as an index of the LED within RGB LED module, then we should change the bindings example for the banked case to the following, i.e. have subLED's reg also 0,1,2: multi-led@3 { #address-cells = <1>; #size-cells = <0>; reg = <0x3>, <0x4>, <0x5>; color = <LED_COLOR_ID_RGB>; function = LED_FUNCTION_STANDBY; led@0 { reg = <0x0>; color = <LED_COLOR_ID_RED>; }; led@1 { reg = <0x1>; color = <LED_COLOR_ID_GREEN>; }; led@2 { reg = <0x2>; color = <LED_COLOR_ID_BLUE>; }; };
diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c index 02cb1565a9fb..48db024081f5 100644 --- a/drivers/leds/leds-lp50xx.c +++ b/drivers/leds/leds-lp50xx.c @@ -476,6 +476,7 @@ static int lp50xx_probe_dt(struct lp50xx *priv) return -ENOMEM; fwnode_for_each_child_node(child, led_node) { + int multi_index = num_colors; ret = fwnode_property_read_u32(led_node, "color", &color_id); if (ret) { @@ -483,8 +484,15 @@ static int lp50xx_probe_dt(struct lp50xx *priv) dev_err(priv->dev, "Cannot read color\n"); return ret; } + ret = fwnode_property_read_u32(led_node, "reg", &multi_index); + if (ret) { + multi_index = num_colors; + } else if (multi_index >= LP50XX_LEDS_PER_MODULE) { + dev_warn(priv->dev, "reg %i out of range\n", multi_index); + multi_index = num_colors; + } - mc_led_info[num_colors].color_index = color_id; + mc_led_info[multi_index].color_index = color_id; num_colors++; }
mc_subled used for multi_index needs well defined array indexes, to guarantee the desired result, optionally use reg for that. If devicetree child nodes is processed in random or reverse order you may end up with multi_index "blue green red" instead of the expected "red green blue". If user space apps uses multi_index to deduce how to control the leds they would most likely be broken without this patch if devicetree processing is reversed (which it appears to be). arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dts has reg set but I don't see how it can have worked without this change. If reg is not set, the previous behavior is kept, index will be in the order nodes are processed. Signed-off-by: Johan Adolfsson <johan.adolfsson@axis.com> --- Since devicetree nodes are (sometimes?) processed in reverse order, support reg as the actual multi_index index so yo get well defined color order presented in the multi_index file. Not sure if reusing reg for this is the correct way or if another property such as "multi_index" or similar should be used instead. Looks like reg is used for similar things at least. Or should the whole "reverse the devicetree" problem be fixed instead? --- drivers/leds/leds-lp50xx.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) --- base-commit: 38fec10eb60d687e30c8c6b5420d86e8149f7557 change-id: 20250225-led-fix-444fb544584a Best regards,