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

[Lee Jones <lee@kernel.org>]

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 [李琼斯]