lkml.org 
[lkml]   [2016]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] leds: triggers: add support for RGB triggers
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@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.
>>
> 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.

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

\
 
 \ /
  Last update: 2016-03-21 17:21    [W:0.075 / U:0.984 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site