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);
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
