* [PATCH 1/3] leds: triggers: add support for RGB triggers
@ 2016-03-13 17:14 Heiner Kallweit
2016-03-17 13:41 ` Jacek Anaszewski
0 siblings, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2016-03-13 17:14 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds
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;
+}
+
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;
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;
}
}
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;
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);
--
2.7.2
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-13 17:14 [PATCH 1/3] leds: triggers: add support for RGB triggers Heiner Kallweit
@ 2016-03-17 13:41 ` Jacek Anaszewski
2016-03-17 19:53 ` Heiner Kallweit
0 siblings, 1 reply; 18+ messages in thread
From: Jacek Anaszewski @ 2016-03-17 13:41 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-leds
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?
> 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;
> 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;
> }
> }
> 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;
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-17 13:41 ` Jacek Anaszewski
@ 2016-03-17 19:53 ` Heiner Kallweit
2016-03-18 13:10 ` Jacek Anaszewski
0 siblings, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2016-03-17 19:53 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, Linux Kernel Mailing List
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.
>> 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;
>> }
>> }
>> 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);
>>
>>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-17 19:53 ` Heiner Kallweit
@ 2016-03-18 13:10 ` Jacek Anaszewski
2016-03-19 19:11 ` Heiner Kallweit
0 siblings, 1 reply; 18+ messages in thread
From: Jacek Anaszewski @ 2016-03-18 13:10 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-leds, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-18 13:10 ` Jacek Anaszewski
@ 2016-03-19 19:11 ` Heiner Kallweit
2016-03-21 15:35 ` Jacek Anaszewski
0 siblings, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2016-03-19 19:11 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, Linux Kernel Mailing List
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.
>>>> 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);
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-19 19:11 ` Heiner Kallweit
@ 2016-03-21 15:35 ` Jacek Anaszewski
2016-03-21 17:34 ` Heiner Kallweit
0 siblings, 1 reply; 18+ messages in thread
From: Jacek Anaszewski @ 2016-03-21 15:35 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-leds, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-21 15:35 ` Jacek Anaszewski
@ 2016-03-21 17:34 ` Heiner Kallweit
2016-03-22 8:05 ` Jacek Anaszewski
0 siblings, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2016-03-21 17:34 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, Linux Kernel Mailing List
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@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.
>
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.
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);
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-21 17:34 ` Heiner Kallweit
@ 2016-03-22 8:05 ` Jacek Anaszewski
2016-03-22 11:47 ` Heiner Kallweit
0 siblings, 1 reply; 18+ messages in thread
From: Jacek Anaszewski @ 2016-03-22 8:05 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-leds, Linux Kernel Mailing List
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@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.
>>
> 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.
> 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);
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-22 8:05 ` Jacek Anaszewski
@ 2016-03-22 11:47 ` Heiner Kallweit
2016-03-22 16:00 ` Jacek Anaszewski
0 siblings, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2016-03-22 11:47 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, Linux Kernel Mailing List
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@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.
>>>
>> 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);
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-22 11:47 ` Heiner Kallweit
@ 2016-03-22 16:00 ` Jacek Anaszewski
2016-03-22 22:06 ` Heiner Kallweit
0 siblings, 1 reply; 18+ messages in thread
From: Jacek Anaszewski @ 2016-03-22 16:00 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-leds, Linux Kernel Mailing List
On 03/22/2016 12:47 PM, Heiner Kallweit wrote:
> 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@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.
>>>>
>>> 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
You probably think about the case when we have two triggers in
single module, like in the planned {rgb-}heartbeat case?
If so this is an argument for having RGB triggers in separate files.
> 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.
What other modules do you have on mind? LED triggers are implemented in
their own files.
> 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);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-22 16:00 ` Jacek Anaszewski
@ 2016-03-22 22:06 ` Heiner Kallweit
2016-03-23 8:32 ` Jacek Anaszewski
0 siblings, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2016-03-22 22:06 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, Linux Kernel Mailing List
Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
> On 03/22/2016 12:47 PM, Heiner Kallweit wrote:
>> 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@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.
>>>>>
>>>> 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
>
> You probably think about the case when we have two triggers in
> single module, like in the planned {rgb-}heartbeat case?
>
> If so this is an argument for having RGB triggers in separate files.
>
I mean the case of triggers implemented outside drivers/leds. There the trigger code
often is not separated from other functionality (e.g. drivers/tty/vt/keyboard.c)
and it's not directly under our (LED core) control.
>> 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.
>
> What other modules do you have on mind? LED triggers are implemented in
> their own files.
>
That's true for the triggers under drivers/leds/trigger, but not necessarily for triggers
implemented in other parts of the kernel.
>> 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);
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-22 22:06 ` Heiner Kallweit
@ 2016-03-23 8:32 ` Jacek Anaszewski
2016-03-23 11:57 ` Heiner Kallweit
0 siblings, 1 reply; 18+ messages in thread
From: Jacek Anaszewski @ 2016-03-23 8:32 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-leds, Linux Kernel Mailing List
On 03/22/2016 11:06 PM, Heiner Kallweit wrote:
> Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
>> On 03/22/2016 12:47 PM, Heiner Kallweit wrote:
>>> 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@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.
>>>>>>
>>>>> 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
>>
>> You probably think about the case when we have two triggers in
>> single module, like in the planned {rgb-}heartbeat case?
>>
>> If so this is an argument for having RGB triggers in separate files.
>>
> I mean the case of triggers implemented outside drivers/leds. There the trigger code
> often is not separated from other functionality (e.g. drivers/tty/vt/keyboard.c)
> and it's not directly under our (LED core) control.
>
>>> 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.
>>
>> What other modules do you have on mind? LED triggers are implemented in
>> their own files.
>>
> That's true for the triggers under drivers/leds/trigger, but not necessarily for triggers
> implemented in other parts of the kernel.
In this case surrounding all the trigger implementation with
IS_ENABLED(CONFIG_LEDS_CLASS_RGB) guard would do.
In the aformentioned drivers/tty/vt/keyboard.c we have even more generic
IS_ENABLED(CONFIG_LEDS_TRIGGERS) guard anyway.
>>> 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);
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-23 8:32 ` Jacek Anaszewski
@ 2016-03-23 11:57 ` Heiner Kallweit
2016-03-23 16:02 ` Jacek Anaszewski
0 siblings, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2016-03-23 11:57 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, Linux Kernel Mailing List
Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
> On 03/22/2016 11:06 PM, Heiner Kallweit wrote:
>> Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
>>> On 03/22/2016 12:47 PM, Heiner Kallweit wrote:
>>>> 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@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.
>>>>>>>
>>>>>> 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
>>>
>>> You probably think about the case when we have two triggers in
>>> single module, like in the planned {rgb-}heartbeat case?
>>>
>>> If so this is an argument for having RGB triggers in separate files.
>>>
>> I mean the case of triggers implemented outside drivers/leds. There the trigger code
>> often is not separated from other functionality (e.g. drivers/tty/vt/keyboard.c)
>> and it's not directly under our (LED core) control.
>>
>>>> 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.
>>>
>>> What other modules do you have on mind? LED triggers are implemented in
>>> their own files.
>>>
>> That's true for the triggers under drivers/leds/trigger, but not necessarily for triggers
>> implemented in other parts of the kernel.
>
> In this case surrounding all the trigger implementation with
> IS_ENABLED(CONFIG_LEDS_CLASS_RGB) guard would do.
Yes, that's what would need to be done. But IMHO it's not nice to force trigger implementations
in other parts of the kernel to guard each trigger-related call this way. Also it might happen
that a trigger is implemented w/o this guarding and w/o informing you.
Then this (RGB) trigger would show up also for all non-RGB LED's.
I still think that not making led_trigger_is_supported() a no-op in the non-RGB case is a small
price for preventing such potential issues.
> In the aformentioned drivers/tty/vt/keyboard.c we have even more generic
> IS_ENABLED(CONFIG_LEDS_TRIGGERS) guard anyway.
>
>>>> 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);
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-23 11:57 ` Heiner Kallweit
@ 2016-03-23 16:02 ` Jacek Anaszewski
2016-03-23 16:36 ` Heiner Kallweit
0 siblings, 1 reply; 18+ messages in thread
From: Jacek Anaszewski @ 2016-03-23 16:02 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-leds, Linux Kernel Mailing List
On 03/23/2016 12:57 PM, Heiner Kallweit wrote:
> Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
>> On 03/22/2016 11:06 PM, Heiner Kallweit wrote:
>>> Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
>>>> On 03/22/2016 12:47 PM, Heiner Kallweit wrote:
>>>>> 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@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.
>>>>>>>>
>>>>>>> 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
>>>>
>>>> You probably think about the case when we have two triggers in
>>>> single module, like in the planned {rgb-}heartbeat case?
>>>>
>>>> If so this is an argument for having RGB triggers in separate files.
>>>>
>>> I mean the case of triggers implemented outside drivers/leds. There the trigger code
>>> often is not separated from other functionality (e.g. drivers/tty/vt/keyboard.c)
>>> and it's not directly under our (LED core) control.
>>>
>>>>> 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.
>>>>
>>>> What other modules do you have on mind? LED triggers are implemented in
>>>> their own files.
>>>>
>>> That's true for the triggers under drivers/leds/trigger, but not necessarily for triggers
>>> implemented in other parts of the kernel.
>>
>> In this case surrounding all the trigger implementation with
>> IS_ENABLED(CONFIG_LEDS_CLASS_RGB) guard would do.
>
> Yes, that's what would need to be done. But IMHO it's not nice to force trigger implementations
> in other parts of the kernel to guard each trigger-related call this way.
My main objection is that led_trigger_is_supported() would be redundant
in led_trigger_store() and led_trigger_show() for non-RGB LED subsystem
configuration.
> Also it might happen
> that a trigger is implemented w/o this guarding and w/o informing you.
> Then this (RGB) trigger would show up also for all non-RGB LED's.
It is likely that it wouldn't compile without led-rgb-core.o.
> I still think that not making led_trigger_is_supported() a no-op in the non-RGB case is a small
> price for preventing such potential issues.
We could avoid the issues by adding a guard in led_trigger_register(),
that would prevent RGB trigger registration in !CONFIG_LEDS_CLASS_RGB
case.
>> In the aformentioned drivers/tty/vt/keyboard.c we have even more generic
>> IS_ENABLED(CONFIG_LEDS_TRIGGERS) guard anyway.
>>
>>>>> 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);
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-23 16:02 ` Jacek Anaszewski
@ 2016-03-23 16:36 ` Heiner Kallweit
2016-03-24 13:23 ` Jacek Anaszewski
0 siblings, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2016-03-23 16:36 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, Linux Kernel Mailing List
Am 23.03.2016 um 17:02 schrieb Jacek Anaszewski:
> On 03/23/2016 12:57 PM, Heiner Kallweit wrote:
>> Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
>>> On 03/22/2016 11:06 PM, Heiner Kallweit wrote:
>>>> Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
>>>>> On 03/22/2016 12:47 PM, Heiner Kallweit wrote:
>>>>>> 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@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.
>>>>>>>>>
>>>>>>>> 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
>>>>>
>>>>> You probably think about the case when we have two triggers in
>>>>> single module, like in the planned {rgb-}heartbeat case?
>>>>>
>>>>> If so this is an argument for having RGB triggers in separate files.
>>>>>
>>>> I mean the case of triggers implemented outside drivers/leds. There the trigger code
>>>> often is not separated from other functionality (e.g. drivers/tty/vt/keyboard.c)
>>>> and it's not directly under our (LED core) control.
>>>>
>>>>>> 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.
>>>>>
>>>>> What other modules do you have on mind? LED triggers are implemented in
>>>>> their own files.
>>>>>
>>>> That's true for the triggers under drivers/leds/trigger, but not necessarily for triggers
>>>> implemented in other parts of the kernel.
>>>
>>> In this case surrounding all the trigger implementation with
>>> IS_ENABLED(CONFIG_LEDS_CLASS_RGB) guard would do.
>>
>> Yes, that's what would need to be done. But IMHO it's not nice to force trigger implementations
>> in other parts of the kernel to guard each trigger-related call this way.
>
> My main objection is that led_trigger_is_supported() would be redundant
> in led_trigger_store() and led_trigger_show() for non-RGB LED subsystem
> configuration.
>
Yes, it's redundant for non-RGB configurations. But it affects sysfs access only
and overhead / impact should be minimal to negligible.
>> Also it might happen
>> that a trigger is implemented w/o this guarding and w/o informing you.
>> Then this (RGB) trigger would show up also for all non-RGB LED's.
>
> It is likely that it wouldn't compile without led-rgb-core.o.
>
It would compile because the only relevant difference between a RGB and a non-RGB trigger is a flag
being set in struct led_trigger.
>> I still think that not making led_trigger_is_supported() a no-op in the non-RGB case is a small
>> price for preventing such potential issues.
>
> We could avoid the issues by adding a guard in led_trigger_register(),
> that would prevent RGB trigger registration in !CONFIG_LEDS_CLASS_RGB
> case.
>
With "preventing registration" most likely you mean registering being a no-op.
I'm afraid we'd need the same also in all other public trigger functions, because it may cause
problems if registering is a no-op and we call e.g. led_trigger_event then (not being a no-op).
That's what I meant when I wrote earlier in this thread that we might need something like this
in all exported trigger functions:
#if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
if (trig->flags & LED_TRIG_CAP_RGB))
return;
#endif
And this seems to be much more overhead than the one check in sysfs access not being a no-op
in the non-RGB case.
>>> In the aformentioned drivers/tty/vt/keyboard.c we have even more generic
>>> IS_ENABLED(CONFIG_LEDS_TRIGGERS) guard anyway.
>>>
>>>>>> 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);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-23 16:36 ` Heiner Kallweit
@ 2016-03-24 13:23 ` Jacek Anaszewski
2016-03-24 15:44 ` Heiner Kallweit
0 siblings, 1 reply; 18+ messages in thread
From: Jacek Anaszewski @ 2016-03-24 13:23 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: linux-leds, Linux Kernel Mailing List
On 03/23/2016 05:36 PM, Heiner Kallweit wrote:
> Am 23.03.2016 um 17:02 schrieb Jacek Anaszewski:
>> On 03/23/2016 12:57 PM, Heiner Kallweit wrote:
>>> Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
>>>> On 03/22/2016 11:06 PM, Heiner Kallweit wrote:
>>>>> Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
>>>>>> On 03/22/2016 12:47 PM, Heiner Kallweit wrote:
>>>>>>> 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@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.
>>>>>>>>>>
>>>>>>>>> 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
>>>>>>
>>>>>> You probably think about the case when we have two triggers in
>>>>>> single module, like in the planned {rgb-}heartbeat case?
>>>>>>
>>>>>> If so this is an argument for having RGB triggers in separate files.
>>>>>>
>>>>> I mean the case of triggers implemented outside drivers/leds. There the trigger code
>>>>> often is not separated from other functionality (e.g. drivers/tty/vt/keyboard.c)
>>>>> and it's not directly under our (LED core) control.
>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> What other modules do you have on mind? LED triggers are implemented in
>>>>>> their own files.
>>>>>>
>>>>> That's true for the triggers under drivers/leds/trigger, but not necessarily for triggers
>>>>> implemented in other parts of the kernel.
>>>>
>>>> In this case surrounding all the trigger implementation with
>>>> IS_ENABLED(CONFIG_LEDS_CLASS_RGB) guard would do.
>>>
>>> Yes, that's what would need to be done. But IMHO it's not nice to force trigger implementations
>>> in other parts of the kernel to guard each trigger-related call this way.
>>
>> My main objection is that led_trigger_is_supported() would be redundant
>> in led_trigger_store() and led_trigger_show() for non-RGB LED subsystem
>> configuration.
>>
> Yes, it's redundant for non-RGB configurations. But it affects sysfs access only
> and overhead / impact should be minimal to negligible
I agree.
>>> Also it might happen
>>> that a trigger is implemented w/o this guarding and w/o informing you.
>>> Then this (RGB) trigger would show up also for all non-RGB LED's.
>>
>> It is likely that it wouldn't compile without led-rgb-core.o.
>>
> It would compile because the only relevant difference between a RGB and a non-RGB trigger is a flag
> being set in struct led_trigger.
RGB trigger would probably need to use some led-rgb-core API, e.g. as
in case of led_trigger_range_event() from your patch - we've already
agreed about moving most of its internals to led-rgb-core.c
>>> I still think that not making led_trigger_is_supported() a no-op in the non-RGB case is a small
>>> price for preventing such potential issues.
>>
>> We could avoid the issues by adding a guard in led_trigger_register(),
>> that would prevent RGB trigger registration in !CONFIG_LEDS_CLASS_RGB
>> case.
>>
> With "preventing registration" most likely you mean registering being a no-op.
Actually I mean checking if trigger is supported by current LED
subsystem configuration, i.e. we will need to use
led_is_trigger_supported() in led_trigger_register(). This is another
argument for this API to be no-op only if !CONFIG_LEDS_TRIGGERS.
To conclude - I agree that it shouldn't be no-op in
!CONFIG_LEDS_CLASS_RGB case.
> I'm afraid we'd need the same also in all other public trigger functions, because it may cause
> problems if registering is a no-op and we call e.g. led_trigger_event then (not being a no-op).
> That's what I meant when I wrote earlier in this thread that we might need something like this
> in all exported trigger functions:
>
> #if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
> if (trig->flags & LED_TRIG_CAP_RGB))
> return;
> #endif
>
> And this seems to be much more overhead than the one check in sysfs access not being a no-op
> in the non-RGB case.
>
>>>> In the aformentioned drivers/tty/vt/keyboard.c we have even more generic
>>>> IS_ENABLED(CONFIG_LEDS_TRIGGERS) guard anyway.
>>>>
>>>>>>> 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);
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-24 13:23 ` Jacek Anaszewski
@ 2016-03-24 15:44 ` Heiner Kallweit
2016-03-24 21:32 ` Jacek Anaszewski
0 siblings, 1 reply; 18+ messages in thread
From: Heiner Kallweit @ 2016-03-24 15:44 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds, Linux Kernel Mailing List
Am 24.03.2016 um 14:23 schrieb Jacek Anaszewski:
> On 03/23/2016 05:36 PM, Heiner Kallweit wrote:
>> Am 23.03.2016 um 17:02 schrieb Jacek Anaszewski:
>>> On 03/23/2016 12:57 PM, Heiner Kallweit wrote:
>>>> Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
>>>>> On 03/22/2016 11:06 PM, Heiner Kallweit wrote:
>>>>>> Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
>>>>>>> On 03/22/2016 12:47 PM, Heiner Kallweit wrote:
>>>>>>>> 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@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.
>>>>>>>>>>>
>>>>>>>>>> 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
>>>>>>>
>>>>>>> You probably think about the case when we have two triggers in
>>>>>>> single module, like in the planned {rgb-}heartbeat case?
>>>>>>>
>>>>>>> If so this is an argument for having RGB triggers in separate files.
>>>>>>>
>>>>>> I mean the case of triggers implemented outside drivers/leds. There the trigger code
>>>>>> often is not separated from other functionality (e.g. drivers/tty/vt/keyboard.c)
>>>>>> and it's not directly under our (LED core) control.
>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> What other modules do you have on mind? LED triggers are implemented in
>>>>>>> their own files.
>>>>>>>
>>>>>> That's true for the triggers under drivers/leds/trigger, but not necessarily for triggers
>>>>>> implemented in other parts of the kernel.
>>>>>
>>>>> In this case surrounding all the trigger implementation with
>>>>> IS_ENABLED(CONFIG_LEDS_CLASS_RGB) guard would do.
>>>>
>>>> Yes, that's what would need to be done. But IMHO it's not nice to force trigger implementations
>>>> in other parts of the kernel to guard each trigger-related call this way.
>>>
>>> My main objection is that led_trigger_is_supported() would be redundant
>>> in led_trigger_store() and led_trigger_show() for non-RGB LED subsystem
>>> configuration.
>>>
>> Yes, it's redundant for non-RGB configurations. But it affects sysfs access only
>> and overhead / impact should be minimal to negligible
>
> I agree.
>
>>>> Also it might happen
>>>> that a trigger is implemented w/o this guarding and w/o informing you.
>>>> Then this (RGB) trigger would show up also for all non-RGB LED's.
>>>
>>> It is likely that it wouldn't compile without led-rgb-core.o.
>>>
>> It would compile because the only relevant difference between a RGB and a non-RGB trigger is a flag
>> being set in struct led_trigger.
>
> RGB trigger would probably need to use some led-rgb-core API, e.g. as
> in case of led_trigger_range_event() from your patch - we've already
> agreed about moving most of its internals to led-rgb-core.c
>
>>>> I still think that not making led_trigger_is_supported() a no-op in the non-RGB case is a small
>>>> price for preventing such potential issues.
>>>
>>> We could avoid the issues by adding a guard in led_trigger_register(),
>>> that would prevent RGB trigger registration in !CONFIG_LEDS_CLASS_RGB
>>> case.
>>>
>> With "preventing registration" most likely you mean registering being a no-op.
>
> Actually I mean checking if trigger is supported by current LED
> subsystem configuration, i.e. we will need to use
> led_is_trigger_supported() in led_trigger_register(). This is another
> argument for this API to be no-op only if !CONFIG_LEDS_TRIGGERS.
> To conclude - I agree that it shouldn't be no-op in
> !CONFIG_LEDS_CLASS_RGB case.
>
Yes, we could extend led_is_trigger_supported() to check also for the case
RGB trigger and non-RGB LED core config.
However, as mentioned in the following, adding this check to led_trigger_register()
only is not sufficient. We'd have to add it to all exported trigger functions.
Returning an error from led_trigger_register() most likely is no option as this
would for sure cause calling driver probe() functions to fail.
Adding such a check to the trigger functions isn't strictly needed because RGB triggers
can't be activated anyway via sysfs in the non-RGB LED core case.
>> I'm afraid we'd need the same also in all other public trigger functions, because it may cause
>> problems if registering is a no-op and we call e.g. led_trigger_event then (not being a no-op).
>> That's what I meant when I wrote earlier in this thread that we might need something like this
>> in all exported trigger functions:
>>
>> #if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
>> if (trig->flags & LED_TRIG_CAP_RGB))
>> return;
>> #endif
>>
>> And this seems to be much more overhead than the one check in sysfs access not being a no-op
>> in the non-RGB case.
>>
>>>>> In the aformentioned drivers/tty/vt/keyboard.c we have even more generic
>>>>> IS_ENABLED(CONFIG_LEDS_TRIGGERS) guard anyway.
>>>>>
>>>>>>>> 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);
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] leds: triggers: add support for RGB triggers
2016-03-24 15:44 ` Heiner Kallweit
@ 2016-03-24 21:32 ` Jacek Anaszewski
0 siblings, 0 replies; 18+ messages in thread
From: Jacek Anaszewski @ 2016-03-24 21:32 UTC (permalink / raw)
To: Heiner Kallweit, Jacek Anaszewski; +Cc: linux-leds, Linux Kernel Mailing List
On 03/24/2016 04:44 PM, Heiner Kallweit wrote:
> Am 24.03.2016 um 14:23 schrieb Jacek Anaszewski:
>> On 03/23/2016 05:36 PM, Heiner Kallweit wrote:
>>> Am 23.03.2016 um 17:02 schrieb Jacek Anaszewski:
>>>> On 03/23/2016 12:57 PM, Heiner Kallweit wrote:
>>>>> Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
>>>>>> On 03/22/2016 11:06 PM, Heiner Kallweit wrote:
>>>>>>> Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
>>>>>>>> On 03/22/2016 12:47 PM, Heiner Kallweit wrote:
>>>>>>>>> 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@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.
>>>>>>>>>>>>
>>>>>>>>>>> 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
>>>>>>>>
>>>>>>>> You probably think about the case when we have two triggers in
>>>>>>>> single module, like in the planned {rgb-}heartbeat case?
>>>>>>>>
>>>>>>>> If so this is an argument for having RGB triggers in separate files.
>>>>>>>>
>>>>>>> I mean the case of triggers implemented outside drivers/leds. There the trigger code
>>>>>>> often is not separated from other functionality (e.g. drivers/tty/vt/keyboard.c)
>>>>>>> and it's not directly under our (LED core) control.
>>>>>>>
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> What other modules do you have on mind? LED triggers are implemented in
>>>>>>>> their own files.
>>>>>>>>
>>>>>>> That's true for the triggers under drivers/leds/trigger, but not necessarily for triggers
>>>>>>> implemented in other parts of the kernel.
>>>>>>
>>>>>> In this case surrounding all the trigger implementation with
>>>>>> IS_ENABLED(CONFIG_LEDS_CLASS_RGB) guard would do.
>>>>>
>>>>> Yes, that's what would need to be done. But IMHO it's not nice to force trigger implementations
>>>>> in other parts of the kernel to guard each trigger-related call this way.
>>>>
>>>> My main objection is that led_trigger_is_supported() would be redundant
>>>> in led_trigger_store() and led_trigger_show() for non-RGB LED subsystem
>>>> configuration.
>>>>
>>> Yes, it's redundant for non-RGB configurations. But it affects sysfs access only
>>> and overhead / impact should be minimal to negligible
>>
>> I agree.
>>
>>>>> Also it might happen
>>>>> that a trigger is implemented w/o this guarding and w/o informing you.
>>>>> Then this (RGB) trigger would show up also for all non-RGB LED's.
>>>>
>>>> It is likely that it wouldn't compile without led-rgb-core.o.
>>>>
>>> It would compile because the only relevant difference between a RGB and a non-RGB trigger is a flag
>>> being set in struct led_trigger.
>>
>> RGB trigger would probably need to use some led-rgb-core API, e.g. as
>> in case of led_trigger_range_event() from your patch - we've already
>> agreed about moving most of its internals to led-rgb-core.c
>>
>>>>> I still think that not making led_trigger_is_supported() a no-op in the non-RGB case is a small
>>>>> price for preventing such potential issues.
>>>>
>>>> We could avoid the issues by adding a guard in led_trigger_register(),
>>>> that would prevent RGB trigger registration in !CONFIG_LEDS_CLASS_RGB
>>>> case.
>>>>
>>> With "preventing registration" most likely you mean registering being a no-op.
>>
>> Actually I mean checking if trigger is supported by current LED
>> subsystem configuration, i.e. we will need to use
>> led_is_trigger_supported() in led_trigger_register(). This is another
>> argument for this API to be no-op only if !CONFIG_LEDS_TRIGGERS.
>> To conclude - I agree that it shouldn't be no-op in
>> !CONFIG_LEDS_CLASS_RGB case.
>>
> Yes, we could extend led_is_trigger_supported() to check also for the case
> RGB trigger and non-RGB LED core config.
I confused here checking whether a trigger is supported by given
LED class device with preventing LED RGB trigger registration
by !CONFIG_LEDS_CLASS_RGB. I think that the latter can be avoided
by making RGB triggers dependent on CONFIG_LEDS_CLASS_RGB and in
case of RGB triggers implemented outside LED subsystem the trigger
implementation and function calls have to be guarded or if the
existence of the trigger is crucial for the driver then it can
always select CONFIG_LEDS_CLASS_RGB in the Kconfig. We have to make
some decision here or this discussion will never end.
> However, as mentioned in the following, adding this check to led_trigger_register()
> only is not sufficient. We'd have to add it to all exported trigger functions.
> Returning an error from led_trigger_register() most likely is no option as this
> would for sure cause calling driver probe() functions to fail.
>
> Adding such a check to the trigger functions isn't strictly needed because RGB triggers
> can't be activated anyway via sysfs in the non-RGB LED core case.
>
>>> I'm afraid we'd need the same also in all other public trigger functions, because it may cause
>>> problems if registering is a no-op and we call e.g. led_trigger_event then (not being a no-op).
>>> That's what I meant when I wrote earlier in this thread that we might need something like this
>>> in all exported trigger functions:
>>>
>>> #if !IS_ENABLED(CONFIG_LEDS_CLASS_RGB)
>>> if (trig->flags & LED_TRIG_CAP_RGB))
>>> return;
>>> #endif
>>>
>>> And this seems to be much more overhead than the one check in sysfs access not being a no-op
>>> in the non-RGB case.
>>>
>>>>>> In the aformentioned drivers/tty/vt/keyboard.c we have even more generic
>>>>>> IS_ENABLED(CONFIG_LEDS_TRIGGERS) guard anyway.
>>>>>>
>>>>>>>>> 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);
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>
>
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-03-24 21:33 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-13 17:14 [PATCH 1/3] leds: triggers: add support for RGB triggers Heiner Kallweit
2016-03-17 13:41 ` Jacek Anaszewski
2016-03-17 19:53 ` Heiner Kallweit
2016-03-18 13:10 ` Jacek Anaszewski
2016-03-19 19:11 ` Heiner Kallweit
2016-03-21 15:35 ` Jacek Anaszewski
2016-03-21 17:34 ` Heiner Kallweit
2016-03-22 8:05 ` Jacek Anaszewski
2016-03-22 11:47 ` Heiner Kallweit
2016-03-22 16:00 ` Jacek Anaszewski
2016-03-22 22:06 ` Heiner Kallweit
2016-03-23 8:32 ` Jacek Anaszewski
2016-03-23 11:57 ` Heiner Kallweit
2016-03-23 16:02 ` Jacek Anaszewski
2016-03-23 16:36 ` Heiner Kallweit
2016-03-24 13:23 ` Jacek Anaszewski
2016-03-24 15:44 ` Heiner Kallweit
2016-03-24 21:32 ` Jacek Anaszewski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.