Message-ID: <56F40B4D.2050800@gmail.com>
Date:	Thu, 24 Mar 2016 16:44:13 +0100
From:	Heiner Kallweit <hkallweit1@...il.com>
To:	Jacek Anaszewski <j.anaszewski@...sung.com>
Cc:	"linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] leds: triggers: add support for RGB triggers

Am 24.03.2016 um 14:23 schrieb Jacek Anaszewski:
> On 03/23/2016 05:36 PM, Heiner Kallweit wrote:
>> Am 23.03.2016 um 17:02 schrieb Jacek Anaszewski:
>>> On 03/23/2016 12:57 PM, Heiner Kallweit wrote:
>>>> Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
>>>>> On 03/22/2016 11:06 PM, Heiner Kallweit wrote:
>>>>>> Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
>>>>>>> On 03/22/2016 12:47 PM, Heiner Kallweit wrote:
>>>>>>>> Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
>>>>>>>>> On 03/21/2016 06:34 PM, Heiner Kallweit wrote:
>>>>>>>>>> Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
>>>>>>>>>>> On 03/19/2016 08:11 PM, Heiner Kallweit wrote:
>>>>>>>>>>>> Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
>>>>>>>>>>>>> On 03/17/2016 08:53 PM, Heiner Kallweit wrote:
>>>>>>>>>>>>>> Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
>>>>>>>>>>>>>>> Hi Heiner,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 03/13/2016 06:14 PM, Heiner Kallweit wrote:
>>>>>>>>>>>>>>>> Add basic support for RGB triggers. Triggers with flag LED_TRIG_CAP_RGB
>>>>>>>>>>>>>>>> set are available to RGB LED devices only.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>          drivers/leds/led-triggers.c | 15 ++++++++++++---
>>>>>>>>>>>>>>>>          include/linux/leds.h        |  3 +++
>>>>>>>>>>>>>>>>          2 files changed, 15 insertions(+), 3 deletions(-)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>>>>>>>>>>>>>>>> index 2181581..3ccf88b 100644
>>>>>>>>>>>>>>>> --- a/drivers/leds/led-triggers.c
>>>>>>>>>>>>>>>> +++ b/drivers/leds/led-triggers.c
>>>>>>>>>>>>>>>> @@ -30,6 +30,13 @@ static LIST_HEAD(trigger_list);
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>           /* Used by LED Class */
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> +static inline bool led_trig_check_rgb(struct led_trigger *trig,
>>>>>>>>>>>>>>>> +                      struct led_classdev *led_cdev)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> +    return !(trig->flags & LED_TRIG_CAP_RGB) ||
>>>>>>>>>>>>>>>> +           led_cdev->flags & LED_DEV_CAP_RGB;
>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Could you explain what is the purpose of this function?
>>>>>>>>>>>>>>> What actually do we want to check here?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Triggers using RGB functionality can't be used with non-RGB LED's.
>>>>>>>>>>>>>> This check checks for such unsupported combinations:
>>>>>>>>>>>>>> It returns false if the trigger uses RGB functionality but LED doesn't
>>>>>>>>>>>>>> support the RGB extension.
>>>>>>>>>>>>>
>>>>>>>>>>>>> We need more meaningful name for it. Maybe led_trigger_is_supported() ?
>>>>>>>>>>>>> And let's make it no-op for !CONFIG_LEDS_CLASS_RGB case.
>>>>>>>>>>>>>
>>>>>>>>>>>> OK, led_trigger_is_supported() is better.
>>>>>>>>>>>>
>>>>>>>>>>>> Making the function a no-op in the non-RGB case would have some impact:
>>>>>>>>>>>> We'd have to make sure that all public trigger functions are a de-facto no-op
>>>>>>>>>>>> for RGB triggers (at least register / unregister). Means we would need
>>>>>>>>>>>> something like this in each public trigger function:
>>>>>>>>>>>>
>>>>>>>>>>>> #if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
>>>>>>>>>>>>          if (trig->flags & LED_TRIG_CAP_RGB))
>>>>>>>>>>>>              return;
>>>>>>>>>>>> #endif
>>>>>>>>>>>>
>>>>>>>>>>>> I think this would add a lot of overhead and therefore IMHO it's better to
>>>>>>>>>>>> not make the check function a no-op.
>>>>>>>>>>>
>>>>>>>>>>> Wouldn't it suffice to make the no-op returning true?
>>>>>>>>>>> Preventing RGB trigger registration for non-RGB LED class configuration
>>>>>>>>>>> seems to be different thing, also to be considered.
>>>>>>>>>>>
>>>>>>>>>> No, it's not sufficient. Let's say the RGB extension is disabled and we have a RGB trigger.
>>>>>>>>>> The check is a no-op now (returns always true), therefore the RGB trigger would be displayed
>>>>>>>>>> in the list of available triggers also for all non-RGB LED's.
>>>>>>>>>
>>>>>>>>> If RGB trigger was made dependent on LED RGB class, then the related
>>>>>>>>> Kconfig symbol would remain undefined in !CONFIG_LEDS_CLASS_RGB case.
>>>>>>>>>
>>>>>>>> Making a RGB trigger dependent on LED RGB class would mean to enclose all calls to trigger
>>>>>>>> functions in the RGB trigger like this:
>>>>>>>> #if IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
>>>>>>>>        trigger_function()
>>>>>>>> #endif
>>>>>>>
>>>>>>> You probably think about the case when we have two triggers in
>>>>>>> single module, like in the planned {rgb-}heartbeat case?
>>>>>>>
>>>>>>> If so this is an argument for having RGB triggers in separate files.
>>>>>>>
>>>>>> I mean the case of triggers implemented outside drivers/leds. There the trigger code
>>>>>> often is not separated from other functionality (e.g. drivers/tty/vt/keyboard.c)
>>>>>> and it's not directly under our (LED core) control.
>>>>>>
>>>>>>>> This would apply to led_trigger_(un)register, led_trigger_event, led_trigger_blink, etc.
>>>>>>>> And I think it wouldn't be too nice to force other kernel modules wanting to implement
>>>>>>>> a RGB trigger to add these conditional compile statements.
>>>>>>>
>>>>>>> What other modules do you have on mind? LED triggers are implemented in
>>>>>>> their own files.
>>>>>>>
>>>>>> That's true for the triggers under drivers/leds/trigger, but not necessarily for triggers
>>>>>> implemented in other parts of the kernel.
>>>>>
>>>>> In this case surrounding all the trigger implementation  with
>>>>> IS_ENABLED(CONFIG_LEDS_CLASS_RGB) guard would do.
>>>>
>>>> Yes, that's what would need to be done. But IMHO it's not nice to force trigger implementations
>>>> in other parts of the kernel to guard each trigger-related call this way.
>>>
>>> My main objection is that led_trigger_is_supported() would be redundant
>>> in led_trigger_store() and led_trigger_show() for non-RGB LED subsystem
>>> configuration.
>>>
>> Yes, it's redundant for non-RGB configurations. But it affects sysfs access only
>> and overhead / impact should be minimal to negligible
> 
> I agree.
> 
>>>> Also it might happen
>>>> that a trigger is implemented w/o this guarding and w/o informing you.
>>>> Then this (RGB) trigger would show up also for all non-RGB LED's.
>>>
>>> It is likely that it wouldn't compile without led-rgb-core.o.
>>>
>> It would compile because the only relevant difference between a RGB and a non-RGB trigger is a flag
>> being set in struct led_trigger.
> 
> RGB trigger would probably need to use some led-rgb-core API, e.g. as
> in case of led_trigger_range_event() from your patch - we've already
> agreed about moving most of its internals to led-rgb-core.c
> 
>>>> I still think that not making led_trigger_is_supported() a no-op in the non-RGB case is a small
>>>> price for preventing such potential issues.
>>>
>>> We could avoid the issues by adding a guard in led_trigger_register(),
>>> that would prevent RGB trigger registration in !CONFIG_LEDS_CLASS_RGB
>>> case.
>>>
>> With "preventing registration" most likely you mean registering being a no-op.
> 
> Actually I mean checking if trigger is supported by current LED
> subsystem configuration, i.e. we will need to use
> led_is_trigger_supported() in led_trigger_register(). This is another
> argument for this API to be no-op only if !CONFIG_LEDS_TRIGGERS.
> To conclude - I agree that it shouldn't be no-op in
> !CONFIG_LEDS_CLASS_RGB case.
> 
Yes, we could extend led_is_trigger_supported() to check also for the case
RGB trigger and non-RGB LED core config.
However, as mentioned in the following, adding this check to led_trigger_register()
only is not sufficient. We'd have to add it to all exported trigger functions.
Returning an error from led_trigger_register() most likely is no option as this
would for sure cause calling driver probe() functions to fail.

Adding such a check to the trigger functions isn't strictly needed because RGB triggers
can't be activated anyway via sysfs in the non-RGB LED core case.

>> I'm afraid we'd need the same also in all other public trigger functions, because it may cause
>> problems if registering is a no-op and we call e.g. led_trigger_event then (not being a no-op).
>> That's what I meant when I wrote earlier in this thread that we might need something like this
>> in all exported trigger functions:
>>
>> #if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
>>     if (trig->flags & LED_TRIG_CAP_RGB))
>>         return;
>> #endif
>>
>> And this seems to be much more overhead than the one check in sysfs access not being a no-op
>> in the non-RGB case.
>>
>>>>> In the aformentioned drivers/tty/vt/keyboard.c we have even more generic
>>>>> IS_ENABLED(CONFIG_LEDS_TRIGGERS) guard anyway.
>>>>>
>>>>>>>> Alternatively, as mentioned before, we would have to add this to all public trigger functions:
>>>>>>>>
>>>>>>>> #if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
>>>>>>>>        if (trig->flags & LED_TRIG_CAP_RGB))
>>>>>>>>            return;
>>>>>>>> #endif
>>>>>>>> I think this would add significant overhead w/o gaining really something.
>>>>>>>>
>>>>>>>>>> We could maximum remove the "|| led_cdev->flags & LED_DEV_CAP_RGB" from the check if
>>>>>>>>>> the RGB extension is disabled. But it's open whether this minimal gain in a non-critical
>>>>>>>>>> code path justifies this.
>>>>>>>>>>
>>>>>>>>>>>>>>>>          ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>>>>>>>>>                  const char *buf, size_t count)
>>>>>>>>>>>>>>>>          {
>>>>>>>>>>>>>>>> @@ -52,12 +59,12 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
>>>>>>>>>>>>>>>>              down_read(&triggers_list_lock);
>>>>>>>>>>>>>>>>              list_for_each_entry(trig, &trigger_list, next_trig) {
>>>>>>>>>>>>>>>>                  if (sysfs_streq(buf, trig->name)) {
>>>>>>>>>>>>>>>> +            if (!led_trig_check_rgb(trig, led_cdev))
>>>>>>>>>>>>>>>> +                break;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Check for the case that userspace wants to set a RGB trigger for a non-RGB LED via sysfs.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>                      down_write(&led_cdev->trigger_lock);
>>>>>>>>>>>>>>>>                      led_trigger_set(led_cdev, trig);
>>>>>>>>>>>>>>>>                      up_write(&led_cdev->trigger_lock);
>>>>>>>>>>>>>>>> -
>>>>>>>>>>>>>>>> -            up_read(&triggers_list_lock);
>>>>>>>>>>>>>>>> -            goto unlock;
>>>>>>>>>>>>>>>> +            break;
>>>>>>>>>>>>>
>>>>>>>>>>>>> This seems to be an unrelated cleanup. Please submit it separately.
>>>>>>>>>>>>>
>>>>>>>>>>>> OK
>>>>>>>>>>>>
>>>>>>>>>>>>>>>>                  }
>>>>>>>>>>>>>>>>              }
>>>>>>>>>>>>>>>>              up_read(&triggers_list_lock);
>>>>>>>>>>>>>>>> @@ -84,6 +91,8 @@ ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr,
>>>>>>>>>>>>>>>>                  len += sprintf(buf+len, "none ");
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>              list_for_each_entry(trig, &trigger_list, next_trig) {
>>>>>>>>>>>>>>>> +        if (!led_trig_check_rgb(trig, led_cdev))
>>>>>>>>>>>>>>>> +            continue;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Omit RGB triggers when listing the available triggers for a non-RGB LED via sysfs.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>                  if (led_cdev->trigger && !strcmp(led_cdev->trigger->name,
>>>>>>>>>>>>>>>>                                      trig->name))
>>>>>>>>>>>>>>>>                      len += sprintf(buf+len, "[%s] ", trig->name);
>>>>>>>>>>>>>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>>>>>>>>>>>>>>> index 58e22e6..07eb074 100644
>>>>>>>>>>>>>>>> --- a/include/linux/leds.h
>>>>>>>>>>>>>>>> +++ b/include/linux/leds.h
>>>>>>>>>>>>>>>> @@ -248,6 +248,9 @@ enum led_brightness led_hsv_to_rgb(enum led_brightness hsv);
>>>>>>>>>>>>>>>>          struct led_trigger {
>>>>>>>>>>>>>>>>              /* Trigger Properties */
>>>>>>>>>>>>>>>>              const char     *name;
>>>>>>>>>>>>>>>> +    u8        flags;
>>>>>>>>>>>>>>>> +#define LED_TRIG_CAP_RGB    BIT(0)
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>              void        (*activate)(struct led_classdev *led_cdev);
>>>>>>>>>>>>>>>>              void        (*deactivate)(struct led_classdev *led_cdev);
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
> 
> 

Powered by blists - more mailing lists