[PATCH v6 0/2] leds-lp50xx: Support reg to set multi_index.

[PATCH v6 0/2] leds-lp50xx: Support reg to set multi_index.

[Johan Adolfsson]

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?
Update bindings to match implementation, and add description for the
reg property.

Signed-off-by: Johan Adolfsson <johan.adolfsson@axis.com>
---
Changes in v6:
- Remove maxItems från devicetree binding
- Link to v5: https://lore.kernel.org/r/20250616-led-fix-v5-0-f59c740831ab@axis.com

Changes in v5:
- Fail if reg is not set.
- Adjust devicetree schema, use items.
- Link to v4: https://lore.kernel.org/r/20250526-led-fix-v4-0-33345f6c4a78@axis.com

Changes in v4:
- Remove maxItems from devicetree schema, not compatible with minimum
  and maximum.
- Link to v3: https://lore.kernel.org/r/20250523-led-fix-v3-0-86d2690d2698@axis.com

Changes in v3:
- Update To and Cc.
- Rephrase bindings descriptions, add constraints.
- Link to v2: https://lore.kernel.org/r/20250522-led-fix-v2-0-652046323ec3@axis.com

Changes in v2:
- Avoid duplicate assignment. dev_err and return -EINVAL on error.
- Update bindings doc.
- Link to v1: https://lore.kernel.org/r/20250506-led-fix-v1-1-56a39b55a7fc@axis.com

---
Johan Adolfsson (2):
      leds: leds-lp50xx: Handle reg to get correct multi_index
      dt-bindings: leds: lp50xx: Document child reg, fix example

 .../devicetree/bindings/leds/leds-lp50xx.yaml       | 21 ++++++++++++++-------
 drivers/leds/leds-lp50xx.c                          | 11 ++++++++++-
 2 files changed, 24 insertions(+), 8 deletions(-)
---
base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
change-id: 20250225-led-fix-444fb544584a

Best regards,
-- 
Johan Adolfsson <johan.adolfsson@axis.com>

[PATCH v6 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index

[Johan Adolfsson]

mc_subled used for multi_index needs well defined array indexes,
to guarantee the desired result, 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, an error is returned,
If reg is out of range, an error is returned.
reg within led child nodes starts with 0, to map to the iout in each bank.

Signed-off-by: Johan Adolfsson <johan.adolfsson@axis.com>
---
 drivers/leds/leds-lp50xx.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
index 02cb1565a9fb..344791b6c575 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,16 @@ 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 != 0) {
+				dev_err(priv->dev, "reg must be set\n");
+				return -EINVAL;
+			} else if (multi_index >= LP50XX_LEDS_PER_MODULE) {
+				dev_err(priv->dev, "reg %i out of range\n", multi_index);
+				return -EINVAL;
+			}
 
-			mc_led_info[num_colors].color_index = color_id;
+			mc_led_info[multi_index].color_index = color_id;
 			num_colors++;
 		}
 

-- 
2.30.2

Re: [PATCH v6 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index

[Jacek Anaszewski]

Hi Johan,

On 6/16/25 13:25, Johan Adolfsson wrote:
> mc_subled used for multi_index needs well defined array indexes,
> to guarantee the desired result, 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, an error is returned,
> If reg is out of range, an error is returned.
> reg within led child nodes starts with 0, to map to the iout in each bank.
> 
> Signed-off-by: Johan Adolfsson <johan.adolfsson@axis.com>
> ---
>   drivers/leds/leds-lp50xx.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> index 02cb1565a9fb..344791b6c575 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;

Any specific reason for initializing this to num_colors?

>   			ret = fwnode_property_read_u32(led_node, "color",
>   						       &color_id);
>   			if (ret) {
> @@ -483,8 +484,16 @@ 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 != 0) {
> +				dev_err(priv->dev, "reg must be set\n");
> +				return -EINVAL;
> +			} else if (multi_index >= LP50XX_LEDS_PER_MODULE) {
> +				dev_err(priv->dev, "reg %i out of range\n", multi_index);
> +				return -EINVAL;
> +			}
>   
> -			mc_led_info[num_colors].color_index = color_id;
> +			mc_led_info[multi_index].color_index = color_id;
>   			num_colors++;
>   		}
>   
> 


Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v6 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index

[Johan Adolfsson]

Hi Jacek.

>From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>Sent: Monday, June 16, 2025 23:29
>To: Johan Adolfsson; Lee Jones; Pavel Machek; Rob Herring; Krzysztof Kozlowski; Conor Dooley; Andrew Davis
>Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Kernel
>Subject: Re: [PATCH v6 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index
>
>Hi Johan,
>
>On 6/16/25 13:25, Johan Adolfsson wrote:
..
>>   drivers/leds/leds-lp50xx.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
>> index 02cb1565a9fb..344791b6c575 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;

>Any specific reason for initializing this to num_colors?

Sorry - leftover from initial patch where I kept the original behavior if reg was not set.
Another patch version coming up soon (assuming failing on reg not set is the agreed upon solution).

>>
>--
>Best regards,
>Jacek Anaszewski

Best regards
/Johan

[PATCH v6 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example

[Johan Adolfsson]

The led child reg node is the index within the bank, document that
and update the example accordingly.

Signed-off-by: Johan Adolfsson <johan.adolfsson@axis.com>
---
 .../devicetree/bindings/leds/leds-lp50xx.yaml       | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
index 402c25424525..cb450aed718c 100644
--- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
@@ -81,7 +81,14 @@ patternProperties:
 
         properties:
           reg:
-            maxItems: 1
+            items:
+              - minimum: 0
+                maximum: 2
+
+            description:
+              This property denotes the index within the LED bank.
+              The value will act as the index in the multi_index file to give
+              consistent result independent of devicetree processing order.
 
         required:
           - reg
@@ -138,18 +145,18 @@ examples:
                 color = <LED_COLOR_ID_RGB>;
                 function = LED_FUNCTION_STANDBY;
 
-                led@3 {
-                    reg = <0x3>;
+                led@0 {
+                    reg = <0x0>;
                     color = <LED_COLOR_ID_RED>;
                 };
 
-                led@4 {
-                    reg = <0x4>;
+                led@1 {
+                    reg = <0x1>;
                     color = <LED_COLOR_ID_GREEN>;
                 };
 
-                led@5 {
-                    reg = <0x5>;
+                led@2 {
+                    reg = <0x2>;
                     color = <LED_COLOR_ID_BLUE>;
                 };
             };

-- 
2.30.2

Re: [PATCH v6 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example

[Conor Dooley]

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

On Mon, Jun 16, 2025 at 01:25:35PM +0200, Johan Adolfsson wrote:
> The led child reg node is the index within the bank, document that
> and update the example accordingly.
> 
> Signed-off-by: Johan Adolfsson <johan.adolfsson@axis.com>
> ---
>  .../devicetree/bindings/leds/leds-lp50xx.yaml       | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> index 402c25424525..cb450aed718c 100644
> --- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> @@ -81,7 +81,14 @@ patternProperties:
>  
>          properties:
>            reg:
> -            maxItems: 1
> +            items:
> +              - minimum: 0
> +                maximum: 2
> +
> +            description:
> +              This property denotes the index within the LED bank.

> +              The value will act as the index in the multi_index file to give
> +              consistent result independent of devicetree processing order.

This looks like commentary on the particulars of the driver
implementation in linux, which shouldn't be in a binding.

>  
>          required:
>            - reg
> @@ -138,18 +145,18 @@ examples:
>                  color = <LED_COLOR_ID_RGB>;
>                  function = LED_FUNCTION_STANDBY;
>  
> -                led@3 {
> -                    reg = <0x3>;
> +                led@0 {
> +                    reg = <0x0>;

Do you have any explanation for why these numbers, outside the range you
said is valid, were in the binding's example?
Additionally, can you mention in the commit message what the source was
for the 0-2 range?

Cheers,
Conor.

>                      color = <LED_COLOR_ID_RED>;
>                  };
>  
> -                led@4 {
> -                    reg = <0x4>;
> +                led@1 {
> +                    reg = <0x1>;
>                      color = <LED_COLOR_ID_GREEN>;
>                  };
>  
> -                led@5 {
> -                    reg = <0x5>;
> +                led@2 {
> +                    reg = <0x2>;
>                      color = <LED_COLOR_ID_BLUE>;
>                  };
>              };
> 
> -- 
> 2.30.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example

[Conor Dooley]

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

On Mon, Jun 16, 2025 at 03:45:29PM +0000, Johan Adolfsson wrote:
> 
> ________________________________
> From: Conor Dooley
> Sent: Monday, June 16, 2025 17:03
> To: Johan Adolfsson
> Cc: Lee Jones; Pavel Machek; Rob Herring; Krzysztof Kozlowski; Conor Dooley; Andrew Davis; Jacek Anaszewski; linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Kernel
> Subject: Re: [PATCH v6 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example
> 
> On Mon, Jun 16, 2025 at 01:25:35PM +0200, Johan Adolfsson wrote:
> > The led child reg node is the index within the bank, document that
> > and update the example accordingly.
> >
> > Signed-off-by: Johan Adolfsson <johan.adolfsson@axis.com>
> > ---
> >  .../devicetree/bindings/leds/leds-lp50xx.yaml       | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> > index 402c25424525..cb450aed718c 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> > +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> > @@ -81,7 +81,14 @@ patternProperties:
> >
> >>          properties:
> >>            reg:
> >> -            maxItems: 1
> >> +            items:
> >> +              - minimum: 0
> >> +                maximum: 2
> >> +
> >> +            description:
> >> +              This property denotes the index within the LED bank.
> 
> >> +              The value will act as the index in the multi_index file to give
> >> +              consistent result independent of devicetree processing order.
> >
> >This looks like commentary on the particulars of the driver
> >implementation in linux, which shouldn't be in a binding.
> 
> Just trying to explain what the reg value actually does (and why).
> Before my patch the bindings were there but no code that handled it.
> If the weird reverse processing order wasn't a thing there would not have been a problem.

That's all driver implementation detail that another OS might not care
about if implemented differently, and is therefore not permitted in the
binding. The text about "index within the LED bank" should be sufficient
to describe how the hardware is configured, no?

> >>          required:
> >>            - reg
> >> @@ -138,18 +145,18 @@ examples:
> >>                  color = <LED_COLOR_ID_RGB>;
> >>                  function = LED_FUNCTION_STANDBY;
> >>
> >> -                led@3 {
> >> -                    reg = <0x3>;
> >> +                led@0 {
> >> +                    reg = <0x0>;
> >
> >Do you have any explanation for why these numbers, outside the range you
> >said is valid, were in the binding's example?
> 
> No idea, the driver hasn't handled the reg property on the led child until my patch, but
> the property was introduced by this commit:
> commit 3eb229f203c2bc42efbfbafba7f83c8deeca80c9
> Author: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Date:   Tue Jun 7 09:52:46 2022 +0200
> 
>     dt-bindings: leds: lp50xx: correct reg/unit addresses in example
> 
>     The multi-led node defined address/size cells, so it is intended to have
>     children with unit addresses.
> 
>     The second multi-led's reg property defined three LED indexes within one
>     reg item, which is not correct - these are three separate items.
> 
>     Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>     Reviewed-by: Rob Herring <robh@kernel.org>
>     Signed-off-by: Rob Herring <robh@kernel.org>
>     Link: https://lore.kernel.org/r/20220607075247.58048-1-krzysztof.kozlowski@linaro.org

Right, so random shite that Krzysztof put in, based on the reg property
in the multi-led parent, to satisfy the tooling. It's worth mentioning
in your commit message that these values you're replacing were
speculative.

> >Additionally, can you mention in the commit message what the source was
> >for the 0-2 range?
> The LED driver chip has banks with 3 outputs each, this is how I have interpreted what the code does.

Please put that in the commit message.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]