Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
> On 03/21/2016 06:34 PM, Heiner Kallweit wrote:
>> 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.
>
> If RGB trigger was made dependent on LED RGB class, then the related
> Kconfig symbol would remain undefined in !CONFIG_LEDS_CLASS_RGB case.
>
Making a RGB trigger dependent on LED RGB class would mean to enclose all calls to trigger
functions in the RGB trigger like this:
#if IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
trigger_function()
#endif
This would apply to led_trigger_(un)register, led_trigger_event, led_trigger_blink, etc.
And I think it wouldn't be too nice to force other kernel modules wanting to implement
a RGB trigger to add these conditional compile statements.
Alternatively, as mentioned before, we would have to add this to all public trigger functions:
#if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
if (trig->flags & LED_TRIG_CAP_RGB))
return;
#endif
I think this would add significant overhead w/o gaining really something.
>> 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);
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>
--
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
