* [PATCH RFC v4 0/2] leds-lp50xx: Support reg to set multi_index.
@ 2025-05-26 14:53 Johan Adolfsson
2025-05-26 14:54 ` [PATCH RFC v4 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index Johan Adolfsson
2025-05-26 14:54 ` [PATCH RFC v4 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example Johan Adolfsson
0 siblings, 2 replies; 10+ messages in thread
From: Johan Adolfsson @ 2025-05-26 14:53 UTC (permalink / raw)
To: Lee Jones, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andrew Davis, Jacek Anaszewski
Cc: linux-leds, linux-kernel, devicetree, Johan Adolfsson, kernel
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 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 | 19 ++++++++++++-------
drivers/leds/leds-lp50xx.c | 8 +++++++-
2 files changed, 19 insertions(+), 8 deletions(-)
---
base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
change-id: 20250225-led-fix-444fb544584a
Best regards,
--
Johan Adolfsson <johan.adolfsson@axis.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH RFC v4 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index
2025-05-26 14:53 [PATCH RFC v4 0/2] leds-lp50xx: Support reg to set multi_index Johan Adolfsson
@ 2025-05-26 14:54 ` Johan Adolfsson
2025-05-26 21:54 ` Jacek Anaszewski
2025-05-26 14:54 ` [PATCH RFC v4 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example Johan Adolfsson
1 sibling, 1 reply; 10+ messages in thread
From: Johan Adolfsson @ 2025-05-26 14:54 UTC (permalink / raw)
To: Lee Jones, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andrew Davis, Jacek Anaszewski
Cc: linux-leds, linux-kernel, devicetree, Johan Adolfsson, kernel
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.
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 | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
index 02cb1565a9fb..8067aaa916bf 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,13 @@ 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 && 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH RFC v4 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example
2025-05-26 14:53 [PATCH RFC v4 0/2] leds-lp50xx: Support reg to set multi_index Johan Adolfsson
2025-05-26 14:54 ` [PATCH RFC v4 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index Johan Adolfsson
@ 2025-05-26 14:54 ` Johan Adolfsson
2025-05-26 15:34 ` Krzysztof Kozlowski
1 sibling, 1 reply; 10+ messages in thread
From: Johan Adolfsson @ 2025-05-26 14:54 UTC (permalink / raw)
To: Lee Jones, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andrew Davis, Jacek Anaszewski
Cc: linux-leds, linux-kernel, devicetree, Johan Adolfsson, kernel
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 | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
index 402c25424525..a7b2d87cc39d 100644
--- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
@@ -81,7 +81,12 @@ patternProperties:
properties:
reg:
- maxItems: 1
+ 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 +143,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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH RFC v4 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example
2025-05-26 14:54 ` [PATCH RFC v4 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example Johan Adolfsson
@ 2025-05-26 15:34 ` Krzysztof Kozlowski
2025-05-26 16:42 ` Johan Adolfsson
0 siblings, 1 reply; 10+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-26 15:34 UTC (permalink / raw)
To: Johan Adolfsson, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Andrew Davis, Jacek Anaszewski
Cc: linux-leds, linux-kernel, devicetree, kernel
On 26/05/2025 16:54, 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 | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> index 402c25424525..a7b2d87cc39d 100644
> --- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> @@ -81,7 +81,12 @@ patternProperties:
>
> properties:
> reg:
> - maxItems: 1
> + minimum: 0
> + maximum: 2
"not compatible with minimum
and maximum."
No, it is compatible. Just do:
items:
- minimum: 0
maximum: 2
You call this patchset still an RFC, which usually means - do not
review, not ready. Usually when I review RFC I received negative
response that why do I review it... Therefore I tend to don't care about
RFC. Some maintainers completely ignore RFC.
Please EXPLICITLY document in cover letter why this is RFC and what you
expect from us (IOW, why this is not ready for review).
If dropping RFC, keep versioning (people also tend to do it wrong
completely messing up the tools), although I see you use b4, so this
should be without problem.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC v4 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example
2025-05-26 15:34 ` Krzysztof Kozlowski
@ 2025-05-26 16:42 ` Johan Adolfsson
2025-05-27 5:48 ` Krzysztof Kozlowski
0 siblings, 1 reply; 10+ messages in thread
From: Johan Adolfsson @ 2025-05-26 16:42 UTC (permalink / raw)
To: Krzysztof Kozlowski, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Andrew Davis, Jacek Anaszewski,
Johan Adolfsson
Cc: linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, Kernel
>From: Krzysztof Kozlowski <krzk@kernel.org>
>Sent: Monday, May 26, 2025 17:34
>To: Johan Adolfsson; Lee Jones; Pavel Machek; Rob Herring; Krzysztof Kozlowski; Conor Dooley; Andrew Davis; Jacek Anaszewski
>Cc: linux-leds@vger.kernel.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Kernel
>Subject: Re: [PATCH RFC v4 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example
>
>On 26/05/2025 16:54, 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 | 19 ++++++++++++-------
>> 1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
>> index 402c25424525..a7b2d87cc39d 100644
>> --- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
>> +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
>> @@ -81,7 +81,12 @@ patternProperties:
>>
>> properties:
>> reg:
>> - maxItems: 1
>> + minimum: 0
>> + maximum: 2
>"not compatible with minimum
> and maximum."
>
>No, it is compatible. Just do:
>
>items:
> - minimum: 0
> maximum: 2
I have tried every variant of that I can think of and can't make it pass the check:
DT_SCHEMA_FILES="Documentation/devicetree/bindings/leds/leds-lp50xx.yaml" make dt_binding_check
Exactly how should it look like?
>You call this patchset still an RFC, which usually means - do not
>review, not ready. Usually when I review RFC I received negative
>response that why do I review it... Therefore I tend to don't care about
>RFC. Some maintainers completely ignore RFC.
>
>Please EXPLICITLY document in cover letter why this is RFC and what you
>expect from us (IOW, why this is not ready for review).
>
>If dropping RFC, keep versioning (people also tend to do it wrong
>completely messing up the tools), although I see you use b4, so this
>should be without problem.
>
>Best regards,
>Krzysztof
Thank you for your patience!
I think its' ready for review now, If I can figure out the correct syntax for maxItems together with minimum and maximum
(if we need maxItems?) or would v4 be acceptable?
Best regards
/Johan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC v4 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index
2025-05-26 14:54 ` [PATCH RFC v4 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index Johan Adolfsson
@ 2025-05-26 21:54 ` Jacek Anaszewski
2025-06-12 10:53 ` Lee Jones
0 siblings, 1 reply; 10+ messages in thread
From: Jacek Anaszewski @ 2025-05-26 21:54 UTC (permalink / raw)
To: Johan Adolfsson, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Andrew Davis
Cc: linux-leds, linux-kernel, devicetree, kernel
Hi Johan,
On 5/26/25 16:54, 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.
This is a bug and I don't see any value in keeping buggy code.
Just expect reg to be present and make sure that all in-tree
dts files using these bindings use them in a proper way.
To not break existing users of stable releases, if any of them
implement DT subnodes without 'reg' property, we can just not mark this
commit with "Fixed" tag, so that it wasn't applied to stable releases.
Although I am not sure if we should not fix it there as well.
I'm leaving it to Lee.
> 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 | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> index 02cb1565a9fb..8067aaa916bf 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,13 @@ 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 && 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++;
> }
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC v4 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example
2025-05-26 16:42 ` Johan Adolfsson
@ 2025-05-27 5:48 ` Krzysztof Kozlowski
0 siblings, 0 replies; 10+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-27 5:48 UTC (permalink / raw)
To: Johan Adolfsson, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Andrew Davis, Jacek Anaszewski
Cc: linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, Kernel
On 26/05/2025 18:42, Johan Adolfsson wrote:
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
>>> index 402c25424525..a7b2d87cc39d 100644
>>> --- a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
>>> +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
>>> @@ -81,7 +81,12 @@ patternProperties:
>>>
>>> properties:
>>> reg:
>>> - maxItems: 1
>>> + minimum: 0
>>> + maximum: 2
>> "not compatible with minimum
>> and maximum."
>>
>> No, it is compatible. Just do:
>>
>> items:
> > - minimum: 0
> > maximum: 2
>
> I have tried every variant of that I can think of and can't make it pass the check:
> DT_SCHEMA_FILES="Documentation/devicetree/bindings/leds/leds-lp50xx.yaml" make dt_binding_check
>
> Exactly how should it look like?
I pasted the exact syntax.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC v4 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index
2025-05-26 21:54 ` Jacek Anaszewski
@ 2025-06-12 10:53 ` Lee Jones
2025-06-14 13:38 ` Jacek Anaszewski
0 siblings, 1 reply; 10+ messages in thread
From: Lee Jones @ 2025-06-12 10:53 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Johan Adolfsson, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andrew Davis, linux-leds, linux-kernel, devicetree,
kernel
On Mon, 26 May 2025, Jacek Anaszewski wrote:
> Hi Johan,
>
> On 5/26/25 16:54, 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.
>
> This is a bug and I don't see any value in keeping buggy code.
> Just expect reg to be present and make sure that all in-tree
> dts files using these bindings use them in a proper way.
>
> To not break existing users of stable releases, if any of them
> implement DT subnodes without 'reg' property, we can just not mark this
> commit with "Fixed" tag, so that it wasn't applied to stable releases.
> Although I am not sure if we should not fix it there as well.
> I'm leaving it to Lee.
We cannot assume that a patch won't end up in LTS just by omitting the
Fixes: tag. Sasha's AUTOSEL tooling it still likely to pick it up if we
describe the commit as a fix, which we do and is correct.
I see no reason not to apply it. If users are relying on broken
semantics, then those should be fixed also.
Is everyone happy with this patch as-is?
> > 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 | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> > index 02cb1565a9fb..8067aaa916bf 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,13 @@ 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 && 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++;
> > }
> >
>
> --
> Best regards,
> Jacek Anaszewski
>
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC v4 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index
2025-06-12 10:53 ` Lee Jones
@ 2025-06-14 13:38 ` Jacek Anaszewski
2025-06-19 10:02 ` Lee Jones
0 siblings, 1 reply; 10+ messages in thread
From: Jacek Anaszewski @ 2025-06-14 13:38 UTC (permalink / raw)
To: Lee Jones
Cc: Johan Adolfsson, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andrew Davis, linux-leds, linux-kernel, devicetree,
kernel
On 6/12/25 12:53, Lee Jones wrote:
> On Mon, 26 May 2025, Jacek Anaszewski wrote:
>
>> Hi Johan,
>>
>> On 5/26/25 16:54, 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.
>>
>> This is a bug and I don't see any value in keeping buggy code.
>> Just expect reg to be present and make sure that all in-tree
>> dts files using these bindings use them in a proper way.
>>
>> To not break existing users of stable releases, if any of them
>> implement DT subnodes without 'reg' property, we can just not mark this
>> commit with "Fixed" tag, so that it wasn't applied to stable releases.
>> Although I am not sure if we should not fix it there as well.
>> I'm leaving it to Lee.
>
> We cannot assume that a patch won't end up in LTS just by omitting the
> Fixes: tag. Sasha's AUTOSEL tooling it still likely to pick it up if we
> describe the commit as a fix, which we do and is correct.
>
> I see no reason not to apply it. If users are relying on broken
> semantics, then those should be fixed also.
>
> Is everyone happy with this patch as-is?
Nope, we should require presence of proper 'reg' value then.
>>> 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 | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
>>> index 02cb1565a9fb..8067aaa916bf 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,13 @@ 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 && multi_index >= LP50XX_LEDS_PER_MODULE) {
Here we should fail if 'reg' is not present too.
>>> + 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++;
>>> }
>>>
>>
>> --
>> Best regards,
>> Jacek Anaszewski
>>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC v4 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index
2025-06-14 13:38 ` Jacek Anaszewski
@ 2025-06-19 10:02 ` Lee Jones
0 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2025-06-19 10:02 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Johan Adolfsson, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andrew Davis, linux-leds, linux-kernel, devicetree,
kernel
On Sat, 14 Jun 2025, Jacek Anaszewski wrote:
> On 6/12/25 12:53, Lee Jones wrote:
> > On Mon, 26 May 2025, Jacek Anaszewski wrote:
> >
> > > Hi Johan,
> > >
> > > On 5/26/25 16:54, 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.
> > >
> > > This is a bug and I don't see any value in keeping buggy code.
> > > Just expect reg to be present and make sure that all in-tree
> > > dts files using these bindings use them in a proper way.
> > >
> > > To not break existing users of stable releases, if any of them
> > > implement DT subnodes without 'reg' property, we can just not mark this
> > > commit with "Fixed" tag, so that it wasn't applied to stable releases.
> > > Although I am not sure if we should not fix it there as well.
> > > I'm leaving it to Lee.
> >
> > We cannot assume that a patch won't end up in LTS just by omitting the
> > Fixes: tag. Sasha's AUTOSEL tooling it still likely to pick it up if we
> > describe the commit as a fix, which we do and is correct.
> >
> > I see no reason not to apply it. If users are relying on broken
> > semantics, then those should be fixed also.
> >
> > Is everyone happy with this patch as-is?
>
> Nope, we should require presence of proper 'reg' value then.
>
> > > > 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 | 8 +++++++-
> > > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> > > > index 02cb1565a9fb..8067aaa916bf 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,13 @@ 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 && multi_index >= LP50XX_LEDS_PER_MODULE) {
>
> Here we should fail if 'reg' is not present too.
Works for me.
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-06-19 10:02 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-26 14:53 [PATCH RFC v4 0/2] leds-lp50xx: Support reg to set multi_index Johan Adolfsson
2025-05-26 14:54 ` [PATCH RFC v4 1/2] leds: leds-lp50xx: Handle reg to get correct multi_index Johan Adolfsson
2025-05-26 21:54 ` Jacek Anaszewski
2025-06-12 10:53 ` Lee Jones
2025-06-14 13:38 ` Jacek Anaszewski
2025-06-19 10:02 ` Lee Jones
2025-05-26 14:54 ` [PATCH RFC v4 2/2] dt-bindings: leds: lp50xx: Document child reg, fix example Johan Adolfsson
2025-05-26 15:34 ` Krzysztof Kozlowski
2025-05-26 16:42 ` Johan Adolfsson
2025-05-27 5:48 ` Krzysztof Kozlowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).