Message-id: <56F254BA.8030107@samsung.com>
Date:	Wed, 23 Mar 2016 09:32:58 +0100
From:	Jacek Anaszewski <j.anaszewski@...sung.com>
To:	Heiner Kallweit <hkallweit1@...il.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/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.
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