Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 02/23/2017 10:25 PM, Heiner Kallweit wrote:
> Am 23.02.2017 um 22:04 schrieb Jacek Anaszewski:
>> Hi Heiner,
>>
>> Thanks for the patch. I have one comment below.
>>
>> On 02/22/2017 09:35 PM, Heiner Kallweit wrote:
>>> When registering a LED device we have the option to set a default trigger.
>>> Depending on load order of drivers this trigger may not be available yet.
>>> (affected LED device in my case: a DT-configured GPIO LED)
>>> So far if the default trigger can't be found this error is silently
>>> ignored.
>>>
>>> Let's change this to return EPROBE_DEFER if the default trigger can't be
>>> found. This gives the system the chance to probe the LED device later
>>> once the trigger is available.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> ---
>>>  drivers/leds/led-class.c    |  6 +++++-
>>>  drivers/leds/led-triggers.c | 11 ++++++++---
>>>  include/linux/leds.h        |  5 +++--
>>>  3 files changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index f2b0a80a..efe4f5a3 100644
>>> --- a/drivers/leds/led-class.c
>>> +++ b/drivers/leds/led-class.c
>>> @@ -295,7 +295,11 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>>>  	led_init_core(led_cdev);
>>>  
>>>  #ifdef CONFIG_LEDS_TRIGGERS
>>> -	led_trigger_set_default(led_cdev);
>>> +	ret = led_trigger_set_default(led_cdev);
>>> +	if (ret) {
>>> +		led_classdev_unregister(led_cdev);
>>> +		return ret;
>>> +	}
>>>  #endif
>>>  
>>>  	dev_dbg(parent, "Registered led device: %s\n",
>>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>>> index 431123b0..bad9e986 100644
>>> --- a/drivers/leds/led-triggers.c
>>> +++ b/drivers/leds/led-triggers.c
>>> @@ -157,21 +157,26 @@ void led_trigger_remove(struct led_classdev *led_cdev)
>>>  }
>>>  EXPORT_SYMBOL_GPL(led_trigger_remove);
>>>  
>>> -void led_trigger_set_default(struct led_classdev *led_cdev)
>>> +int led_trigger_set_default(struct led_classdev *led_cdev)
>>>  {
>>>  	struct led_trigger *trig;
>>> +	int ret = -EPROBE_DEFER;
>>>  
>>>  	if (!led_cdev->default_trigger)
>>> -		return;
>>> +		return 0;
>>>  
>>>  	down_read(&triggers_list_lock);
>>>  	down_write(&led_cdev->trigger_lock);
>>>  	list_for_each_entry(trig, &trigger_list, next_trig) {
>>> -		if (!strcmp(led_cdev->default_trigger, trig->name))
>>> +		if (!strcmp(led_cdev->default_trigger, trig->name)) {
>>>  			led_trigger_set(led_cdev, trig);
>>> +			ret = 0;
>>
>> I wonder why we don't break the loop after matching the trigger?
>>
>> I think that we can add break here while we are at it since LED Trigger
>> core doesn't allow for registering two triggers with the same name.
>>
> Indeed.
> 
>> Would you mind sending an update and mention it also in the commit
>> message?
>>
> Fine with me if we can silently improve the existing code ..

We shouldn't enclose unrelated changes in a patch, but in this
case you're modifying the condition body anyway, so adding suitable
comment in the commit message should be enough.

-- 
Best regards,
Jacek Anaszewski