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

[Heiner Kallweit]

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.

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