Message-ID: <56F45D00.70501@gmail.com>
Date:	Thu, 24 Mar 2016 22:32:48 +0100
From:	Jacek Anaszewski <jacek.anaszewski@...il.com>
To:	Heiner Kallweit <hkallweit1@...il.com>,
	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

On 03/24/2016 04:44 PM, Heiner Kallweit wrote:
> 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.

I confused here checking whether a trigger is supported by given
LED class device with preventing LED RGB trigger registration
by !CONFIG_LEDS_CLASS_RGB. I think that the latter can be avoided
by making RGB triggers dependent on CONFIG_LEDS_CLASS_RGB and in
case of RGB triggers implemented outside LED subsystem the trigger
implementation and function calls have to be guarded or if the
existence of the trigger is crucial for the driver then it can
always select CONFIG_LEDS_CLASS_RGB in the Kconfig. We have to make
some decision here or this discussion will never end.

> 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);
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
>

-- 
Best Regards,
Jacek Anaszewski

Powered by blists - more mailing lists