* [PATCH] leds: core: use deferred probing if default trigger isn't available yet
@ 2017-02-22 20:35 Heiner Kallweit
2017-02-23 21:04 ` Jacek Anaszewski
2017-02-23 21:08 ` Pavel Machek
0 siblings, 2 replies; 9+ messages in thread
From: Heiner Kallweit @ 2017-02-22 20:35 UTC (permalink / raw)
To: Jacek Anaszewski, Richard Purdie, Pavel Machek; +Cc: linux-leds@vger.kernel.org
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;
+ }
}
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) {}
--
2.11.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet
2017-02-22 20:35 [PATCH] leds: core: use deferred probing if default trigger isn't available yet Heiner Kallweit
@ 2017-02-23 21:04 ` Jacek Anaszewski
2017-02-23 21:25 ` Heiner Kallweit
2017-02-23 21:08 ` Pavel Machek
1 sibling, 1 reply; 9+ messages in thread
From: Jacek Anaszewski @ 2017-02-23 21:04 UTC (permalink / raw)
To: Heiner Kallweit, Richard Purdie, Pavel Machek; +Cc: linux-leds@vger.kernel.org
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.
Would you mind sending an update and mention it also in the commit
message?
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) {}
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet
2017-02-22 20:35 [PATCH] leds: core: use deferred probing if default trigger isn't available yet Heiner Kallweit
2017-02-23 21:04 ` Jacek Anaszewski
@ 2017-02-23 21:08 ` Pavel Machek
2017-02-23 21:23 ` Heiner Kallweit
1 sibling, 1 reply; 9+ messages in thread
From: Pavel Machek @ 2017-02-23 21:08 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Jacek Anaszewski, Richard Purdie, linux-leds@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 1734 bytes --]
On Wed 2017-02-22 21:35:52, 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.
I see a lot of EPROBE_DEFERs on N900, and it is quite nasty, as it
spams a log a lot.
Rather then re-trying LED registration few times, could we make sure
leds are always registered after triggers or something like that?
Pavel
>
> 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;
> + }
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet
2017-02-23 21:08 ` Pavel Machek
@ 2017-02-23 21:23 ` Heiner Kallweit
2017-02-26 17:10 ` Jacek Anaszewski
0 siblings, 1 reply; 9+ messages in thread
From: Heiner Kallweit @ 2017-02-23 21:23 UTC (permalink / raw)
To: Pavel Machek; +Cc: Jacek Anaszewski, Richard Purdie, linux-leds@vger.kernel.org
Am 23.02.2017 um 22:08 schrieb Pavel Machek:
> On Wed 2017-02-22 21:35:52, 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.
>
> I see a lot of EPROBE_DEFERs on N900, and it is quite nasty, as it
> spams a log a lot.
>
Usually error messages are printed only if there is an error and it is not
EPROBE_DEFER. However indeed there still may be several drivers not
taking into account that a subsystem they depend on may return
EPROBE_DEFER and this should not be treated as "hard error".
> Rather then re-trying LED registration few times, could we make sure
> leds are always registered after triggers or something like that?
>
I'm afraid if guaranteeing a particular order would be possible w/o
significant effort then the whole deferred probing concept wouldn't exist.
I could imagine that we can try reordering definitions in the DTS to
ensure a certain load order. But this might be somewhat fragile.
So using the existing concept of deferred probing seems to me to be
the cleaner solution.
Heiner
> Pavel
>
>
>>
>> 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;
>> + }
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet
2017-02-23 21:04 ` Jacek Anaszewski
@ 2017-02-23 21:25 ` Heiner Kallweit
2017-02-26 17:09 ` Jacek Anaszewski
0 siblings, 1 reply; 9+ messages in thread
From: Heiner Kallweit @ 2017-02-23 21:25 UTC (permalink / raw)
To: Jacek Anaszewski, Richard Purdie, Pavel Machek; +Cc: linux-leds@vger.kernel.org
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) {}
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet
2017-02-23 21:25 ` Heiner Kallweit
@ 2017-02-26 17:09 ` Jacek Anaszewski
0 siblings, 0 replies; 9+ messages in thread
From: Jacek Anaszewski @ 2017-02-26 17:09 UTC (permalink / raw)
To: Heiner Kallweit, Richard Purdie, Pavel Machek; +Cc: linux-leds@vger.kernel.org
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet
2017-02-23 21:23 ` Heiner Kallweit
@ 2017-02-26 17:10 ` Jacek Anaszewski
2017-02-26 21:00 ` Pavel Machek
0 siblings, 1 reply; 9+ messages in thread
From: Jacek Anaszewski @ 2017-02-26 17:10 UTC (permalink / raw)
To: Heiner Kallweit, Pavel Machek; +Cc: Richard Purdie, linux-leds@vger.kernel.org
On 02/23/2017 10:23 PM, Heiner Kallweit wrote:
> Am 23.02.2017 um 22:08 schrieb Pavel Machek:
>> On Wed 2017-02-22 21:35:52, 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.
>>
>> I see a lot of EPROBE_DEFERs on N900, and it is quite nasty, as it
>> spams a log a lot.
>>
> Usually error messages are printed only if there is an error and it is not
> EPROBE_DEFER. However indeed there still may be several drivers not
> taking into account that a subsystem they depend on may return
> EPROBE_DEFER and this should not be treated as "hard error".
>
>> Rather then re-trying LED registration few times, could we make sure
>> leds are always registered after triggers or something like that?
>>
> I'm afraid if guaranteeing a particular order would be possible w/o
> significant effort then the whole deferred probing concept wouldn't exist.
>
> I could imagine that we can try reordering definitions in the DTS to
> ensure a certain load order. But this might be somewhat fragile.
> So using the existing concept of deferred probing seems to me to be
> the cleaner solution.
We could go for a solution similar to v4l2-async.c mechanism which
allows for deferring video pipeline linking until all video pipeline
entities are probed, but in the LED subsystem case I think it would be
an overengineering. V4L2 media device has to wait for completion of
probing of several v4l2 drivers, whereas here we've got to wait for
a single driver. EPROBE_DEFER seems to fit for that ideally.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet
2017-02-26 17:10 ` Jacek Anaszewski
@ 2017-02-26 21:00 ` Pavel Machek
2017-02-26 22:07 ` Jacek Anaszewski
0 siblings, 1 reply; 9+ messages in thread
From: Pavel Machek @ 2017-02-26 21:00 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Heiner Kallweit, Richard Purdie, linux-leds@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 2403 bytes --]
On Sun 2017-02-26 18:10:56, Jacek Anaszewski wrote:
> On 02/23/2017 10:23 PM, Heiner Kallweit wrote:
> > Am 23.02.2017 um 22:08 schrieb Pavel Machek:
> >> On Wed 2017-02-22 21:35:52, 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.
> >>
> >> I see a lot of EPROBE_DEFERs on N900, and it is quite nasty, as it
> >> spams a log a lot.
> >>
> > Usually error messages are printed only if there is an error and it is not
> > EPROBE_DEFER. However indeed there still may be several drivers not
> > taking into account that a subsystem they depend on may return
> > EPROBE_DEFER and this should not be treated as "hard error".
> >
> >> Rather then re-trying LED registration few times, could we make sure
> >> leds are always registered after triggers or something like that?
> >>
> > I'm afraid if guaranteeing a particular order would be possible w/o
> > significant effort then the whole deferred probing concept wouldn't exist.
> >
> > I could imagine that we can try reordering definitions in the DTS to
> > ensure a certain load order. But this might be somewhat fragile.
> > So using the existing concept of deferred probing seems to me to be
> > the cleaner solution.
>
> We could go for a solution similar to v4l2-async.c mechanism which
> allows for deferring video pipeline linking until all video pipeline
> entities are probed, but in the LED subsystem case I think it would be
> an overengineering. V4L2 media device has to wait for completion of
> probing of several v4l2 drivers, whereas here we've got to wait for
> a single driver. EPROBE_DEFER seems to fit for that ideally.
I was more thinking of ... just initialize all triggers earlier then
all LEDs?
Initcalls do have few levels we could use..
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] leds: core: use deferred probing if default trigger isn't available yet
2017-02-26 21:00 ` Pavel Machek
@ 2017-02-26 22:07 ` Jacek Anaszewski
0 siblings, 0 replies; 9+ messages in thread
From: Jacek Anaszewski @ 2017-02-26 22:07 UTC (permalink / raw)
To: Pavel Machek, Heiner Kallweit; +Cc: Richard Purdie, linux-leds@vger.kernel.org
On 02/26/2017 10:00 PM, Pavel Machek wrote:
> On Sun 2017-02-26 18:10:56, Jacek Anaszewski wrote:
>> On 02/23/2017 10:23 PM, Heiner Kallweit wrote:
>>> Am 23.02.2017 um 22:08 schrieb Pavel Machek:
>>>> On Wed 2017-02-22 21:35:52, 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.
>>>>
>>>> I see a lot of EPROBE_DEFERs on N900, and it is quite nasty, as it
>>>> spams a log a lot.
>>>>
>>> Usually error messages are printed only if there is an error and it is not
>>> EPROBE_DEFER. However indeed there still may be several drivers not
>>> taking into account that a subsystem they depend on may return
>>> EPROBE_DEFER and this should not be treated as "hard error".
>>>
>>>> Rather then re-trying LED registration few times, could we make sure
>>>> leds are always registered after triggers or something like that?
>>>>
>>> I'm afraid if guaranteeing a particular order would be possible w/o
>>> significant effort then the whole deferred probing concept wouldn't exist.
>>>
>>> I could imagine that we can try reordering definitions in the DTS to
>>> ensure a certain load order. But this might be somewhat fragile.
>>> So using the existing concept of deferred probing seems to me to be
>>> the cleaner solution.
>>
>> We could go for a solution similar to v4l2-async.c mechanism which
>> allows for deferring video pipeline linking until all video pipeline
>> entities are probed, but in the LED subsystem case I think it would be
>> an overengineering. V4L2 media device has to wait for completion of
>> probing of several v4l2 drivers, whereas here we've got to wait for
>> a single driver. EPROBE_DEFER seems to fit for that ideally.
>
> I was more thinking of ... just initialize all triggers earlier then
> all LEDs?
>
> Initcalls do have few levels we could use..
All LED triggers are registered from initialization functions
(marked with __init), and there are some LED class drivers that
are also probed from similar functions. We could e.g. try to move
triggers to subsys_initcall, but it would require thorough testing
to assure that we're not breaking anything.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-02-26 22:08 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-22 20:35 [PATCH] leds: core: use deferred probing if default trigger isn't available yet Heiner Kallweit
2017-02-23 21:04 ` Jacek Anaszewski
2017-02-23 21:25 ` Heiner Kallweit
2017-02-26 17:09 ` Jacek Anaszewski
2017-02-23 21:08 ` Pavel Machek
2017-02-23 21:23 ` Heiner Kallweit
2017-02-26 17:10 ` Jacek Anaszewski
2017-02-26 21:00 ` Pavel Machek
2017-02-26 22:07 ` Jacek Anaszewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox