Re: [PATCH RFC] leds: leds-lp50xx: Handle reg to get correct multi_index

[Lee Jones <lee@kernel.org>]

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

-- 
Lee Jones [李琼斯]