Re: [PATCH 1/3] leds: triggers: add support for RGB triggers

[Heiner Kallweit]

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@xxxxxxxxx>
>>>>>> ---
>>>>>>     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.

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