Re: [PATCH v2] leds: class: Use firmware nodes for device lookup

[Tommaso Merciai]

Hi Lee,
Thanks for your patch.

On Wed, May 20, 2026 at 04:22:25PM +0100, Lee Jones wrote:
> On Wed, 13 May 2026, Alban Bedel wrote:
> 
> > Replace the OF based lookup with the fwnode equivalent to get support
> > for ACPI and software nodes.
> > 
> > Signed-off-by: Alban Bedel <alban.bedel@xxxxxxxxxx>
> > ---
> > v2: * Keep the doc string
> >     * Update comment to reference the function now used
> > ---
> >  drivers/leds/led-class.c | 30 ++++++++++++++++--------------
> >  1 file changed, 16 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index 9e14ae588f78..a17db3d6644f 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -249,32 +249,34 @@ static const struct class leds_class = {
> >  };
> >  
> >  /**
> > - * of_led_get() - request a LED device via the LED framework
> > - * @np: device node to get the LED device from
> > + * fwnode_led_get() - request a LED device via the LED framework
> > + * @fwnode: firmware node to get the LED device from
> >   * @index: the index of the LED
> >   * @name: the name of the LED used to map it to its function, if present
> >   *
> >   * Returns the LED device parsed from the phandle specified in the "leds"
> >   * property of a device tree node or a negative error-code on failure.
> >   */
> > -static struct led_classdev *of_led_get(struct device_node *np, int index,
> > -				       const char *name)
> > +static struct led_classdev *fwnode_led_get(struct fwnode_handle *fwnode,
> > +					   int index, const char *name)
> >  {
> > +	struct fwnode_handle *led_node;
> >  	struct device *led_dev;
> > -	struct device_node *led_node;
> >  
> >  	/*
> >  	 * For named LEDs, first look up the name in the "led-names" property.
> > -	 * If it cannot be found, then of_parse_phandle() will propagate the error.
> > +	 * If it cannot be found, then fwnode_find_reference() will propagate
> > +	 * the error.
> >  	 */
> >  	if (name)
> > -		index = of_property_match_string(np, "led-names", name);
> > -	led_node = of_parse_phandle(np, "leds", index);
> > -	if (!led_node)
> > -		return ERR_PTR(-ENOENT);
> > +		index = fwnode_property_match_string(fwnode, "led-names",
> > +						     name);
> > +	led_node = fwnode_find_reference(fwnode, "leds", index);
> 
> What happens if fwnode_property_match_string() returns an error?

Agree.

I think we need to check index:
		if (index < 0)
			return ERR_PTR(-ENOENT);

If not I'm getting:

	[   19.008923] ov5645 0-003c: OV5645 detected at address 0x3c
	[   19.014903] ov5645 0-003c: error -EINVAL: getting privacy LED
	[   19.020725] ov5645 0-003c: error -EINVAL: could not register v4l2 device
	[   19.028611] ov5645 0-003c: probe with driver ov5645 failed with error -22

While testing:

OV5645 image sensor with Renesas RZ/G3E:

	- arch/arm64/boot/dts/renesas/r9a09g047e57-smarc-cru-csi-ov5645.dtso

Kind Regards,
Tommaso

> 
> > +	if (IS_ERR(led_node))
> > +		return ERR_CAST(led_node);
> >  
> > -	led_dev = class_find_device_by_fwnode(&leds_class, of_fwnode_handle(led_node));
> > -	of_node_put(led_node);
> > +	led_dev = class_find_device_by_fwnode(&leds_class, led_node);
> > +	fwnode_handle_put(led_node);
> >  
> >  	return led_module_get(led_dev);
> >  }
> > @@ -332,7 +334,7 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
> >  	if (!dev)
> >  		return ERR_PTR(-EINVAL);
> >  
> > -	led = of_led_get(dev->of_node, index, NULL);
> > +	led = fwnode_led_get(dev_fwnode(dev), index, NULL);
> >  	if (IS_ERR(led))
> >  		return led;
> >  
> > @@ -354,7 +356,7 @@ struct led_classdev *led_get(struct device *dev, char *con_id)
> >  	const char *provider = NULL;
> >  	struct device *led_dev;
> >  
> > -	led_cdev = of_led_get(dev->of_node, -1, con_id);
> > +	led_cdev = fwnode_led_get(dev_fwnode(dev), -1, con_id);
> >  	if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT)
> >  		return led_cdev;
> >  
> > -- 
> > 2.39.5
> > 
> 
> -- 
> Lee Jones