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

[Jacek Anaszewski <j.anaszewski@samsung.com>]

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

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

>>>            }
>>>        }
>>>        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