Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()

[Hans de Goede <hansg@kernel.org>]

Hi,

On 4-Sep-25 1:01 AM, Aleksandrs Vinarskis wrote:
>> Hi Aleksandrs,
>>
>> Thank you for working on this.
>>
>> On 2-Sep-25 1:10 PM, Aleksandrs Vinarskis wrote:
>>> From: Hans de Goede <hansg@kernel.org>
>>>
>>> Turn of_led_get() into a more generic __of_led_get() helper function,
>>> which can lookup LEDs in devicetree by either name or index.
>>>
>>> And use this new helper to add devicetree support to the generic
>>> (non devicetree specific) [devm_]led_get() function.
>>>
>>> This uses the standard devicetree pattern of adding a -names string array
>>> to map names to the indexes for an array of resources.
>>>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> Reviewed-by: Lee Jones <lee@kernel.org>
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Please update this to:
>>
>> Reviewed-by: Hans de Goede <hansg@kernel.org>
>>
>> to match the update of the author which you already did.
>>
>> Also note that checkpatch should complain about the mismatch,
>> please ensure to run checkpatch before posting v2.
> 
> Hi,
> 
> ahh, I actually did not even see that email got changed, apologies. Seems
> 'b4' auto-corrected it when sending,

I already wondered if it might be something like that. b4 probably did
this because of the .mailmap entry mapping my Red Hat address (which
I've stopped using since I'm leaving Red Hat) to hansg@kernel.org .

> which would explain why checkpatch
> did not catch it, as I run it before importing and sending via 'b4'. Sure,
> will fix - did you mean to change your signoff to R-by, or is it a mistake?

It is a mistake please keep it as S-o-b.

> 
>>
>>> Tested-by: Aleksandrs Vinarskis <alex@vinarskis.com>
>>> ---
>>>  drivers/leds/led-class.c | 38 +++++++++++++++++++++++++++++---------
>>>  1 file changed, 29 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index 15633fbf3c166aa4f521774d245f6399a642bced..6f2ef4fa556b44ed3bf69dff556ae16fd2b7652b 100644
>>> --- a/drivers/leds/led-class.c
>>> +++ b/drivers/leds/led-class.c
>>> @@ -248,19 +248,18 @@ static const struct class leds_class = {
>>>  	.pm = &leds_class_dev_pm_ops,
>>>  };
>>>  
>>> -/**
>>> - * of_led_get() - request a LED device via the LED framework
>>> - * @np: device node to get the LED device from
>>> - * @index: the index of the LED
>>> - *
>>> - * 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)
>>> +static struct led_classdev *__of_led_get(struct device_node *np, int index,
>>> +					 const char *name)
>>>  {
>>>  	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 (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);
>>> @@ -271,6 +270,20 @@ static struct led_classdev *of_led_get(struct device_node *np, int index)
>>>  	return led_module_get(led_dev);
>>>  }
>>>  
>>> +/**
>>> + * of_led_get() - request a LED device via the LED framework
>>> + * @np: device node to get the LED device from
>>> + * @index: the index of the LED
>>> + *
>>> + * 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.
>>> + */
>>> +struct led_classdev *of_led_get(struct device_node *np, int index)
>>> +{
>>> +	return __of_led_get(np, index, NULL);
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_led_get);
>>
>> I probably did this myself, but since of_led_get() is private now
>> (I guess it was not private before?) and since we are moving away from
>> "of" specific functions to using generic dev_xxxx functions in the kernel
>> in general, I think this just should be a static helper.
>>
>> Or probably even better: just add the name argument to of_led_get()
>> before without renaming it, update the existing callers to pass
>> an extra NULL arg and completely drop this wrapper.
>>
> 
> That indeed sounds like a better and cleaner option, will change it.
> This way also incorporates the rest of the feedback on this series.

Sounds good.

Regards,

Hans



>>> +
>>>  /**
>>>   * led_put() - release a LED device
>>>   * @led_cdev: LED device
>>> @@ -342,9 +355,16 @@ EXPORT_SYMBOL_GPL(devm_of_led_get);
>>>  struct led_classdev *led_get(struct device *dev, char *con_id)
>>>  {
>>>  	struct led_lookup_data *lookup;
>>> +	struct led_classdev *led_cdev;
>>>  	const char *provider = NULL;
>>>  	struct device *led_dev;
>>>  
>>> +	if (dev->of_node) {
>>> +		led_cdev = __of_led_get(dev->of_node, -1, con_id);
>>> +		if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT)
>>> +			return led_cdev;
>>> +	}
>>> +
>>>  	mutex_lock(&leds_lookup_lock);
>>>  	list_for_each_entry(lookup, &leds_lookup_list, list) {
>>>  		if (!strcmp(lookup->dev_id, dev_name(dev)) &&
>>>