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

[Heiner Kallweit <hkallweit1@gmail.com>]

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

> Best regards,
> Jacek Anaszewski
> 
>> +		}
>>  	}
>>  	up_write(&led_cdev->trigger_lock);
>>  	up_read(&triggers_list_lock);
>> +
>> +	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(led_trigger_set_default);
>>  
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index 38c0bd7c..3e36ce31 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -280,7 +280,7 @@ extern void led_trigger_blink_oneshot(struct led_trigger *trigger,
>>  				      unsigned long *delay_on,
>>  				      unsigned long *delay_off,
>>  				      int invert);
>> -extern void led_trigger_set_default(struct led_classdev *led_cdev);
>> +extern int led_trigger_set_default(struct led_classdev *led_cdev);
>>  extern void led_trigger_set(struct led_classdev *led_cdev,
>>  			struct led_trigger *trigger);
>>  extern void led_trigger_remove(struct led_classdev *led_cdev);
>> @@ -326,7 +326,8 @@ static inline void led_trigger_blink_oneshot(struct led_trigger *trigger,
>>  				      unsigned long *delay_on,
>>  				      unsigned long *delay_off,
>>  				      int invert) {}
>> -static inline void led_trigger_set_default(struct led_classdev *led_cdev) {}
>> +static inline int led_trigger_set_default(struct led_classdev *led_cdev)
>> +							{ return 0; }
>>  static inline void led_trigger_set(struct led_classdev *led_cdev,
>>  				struct led_trigger *trigger) {}
>>  static inline void led_trigger_remove(struct led_classdev *led_cdev) {}
>>
>