| 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.
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,