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@xxxxxxxxx>
>> ---
>> ---
>> 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) {}
>>
>
