leds: triggers: add support for RGB triggers
PATCH 1/3 leds: triggers: add support for RGB triggers
Add basic support for RGB triggers. Triggers with flag LED_TRIG_CAP_RGB set are available to RGB LED devices only.
--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)
drivers/leds/led-triggers.c +12 -3
@@ -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);
include/linux/leds.h +3 -0
@@ -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);
Signature · 1 line
2.7.2
Hi Heiner,
Quoted 14 lines · Heiner Kallweit
Add basic support for RGB triggers. Triggers with flag
LED_TRIG_CAP_RGBset are available to RGB LED devices only.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)drivers/leds/led-triggers.c +0
-0
drivers/leds/led-triggers.c static LIST_HEAD(trigger_list); · line 38
Quoted 9 lines
drivers/leds/led-triggers.c +6
-0@@ -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?
Quoted 43 lines
drivers/leds/led-triggers.c +5
-3ssize_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);include/linux/leds.h +3
-0@@ -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);
Signature · 2 lines
Best regards, Jacek Anaszewski
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 20 lines
Hi Heiner,
Quoted 17 lines · Heiner Kallweit
Add basic support for RGB triggers. Triggers with flag
LED_TRIG_CAP_RGBset are available to RGB LED devices only.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)drivers/leds/led-triggers.c +0
-0@@ -30,6 +30,13 @@ static LIST_HEAD(trigger_list);/* Used by LED Class */
Quoted 10 lines
drivers/leds/led-triggers.c static LIST_HEAD(trigger_list); · line 38
Quoted 6 lines
drivers/leds/led-triggers.c +6
-0+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.
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, · line 63
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0ssize_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.
Quoted 13 lines
Quoted 13 lines
drivers/leds/led-triggers.c +1
-3down_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 " );
Quoted 3 lines
drivers/leds/led-triggers.c ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr, · line 95
Quoted 3 lines
drivers/leds/led-triggers.c +2
-0list_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.
Quoted 20 lines
Quoted 18 lines
drivers/leds/led-triggers.c +0
-0if (led_cdev->trigger && !strcmp (led_cdev->trigger->name, trig->name)) len +=sprintf (buf+len,"[%s] " , trig->name);include/linux/leds.h +3
-0@@ -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);
Quoted 31 lines · Heiner Kallweit
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 20 lines
Hi Heiner,
Quoted 17 lines · Heiner Kallweit
Add basic support for RGB triggers. Triggers with flag
LED_TRIG_CAP_RGBset are available to RGB LED devices only.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)drivers/leds/led-triggers.c +0
-0@@ -30,6 +30,13 @@ static LIST_HEAD(trigger_list);/* Used by LED Class */ Quoted 10 lines
drivers/leds/led-triggers.c static LIST_HEAD(trigger_list); · line 38
Quoted 6 lines
drivers/leds/led-triggers.c +6
-0+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?
Quoted 4 lines
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.
Quoted 19 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, · line 63
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0ssize_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.
Quoted 7 lines
Quoted 7 lines
drivers/leds/led-triggers.c +1
-3down_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.
Quoted 35 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr, · line 95
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0} }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.
Quoted 20 lines
Quoted 18 lines
drivers/leds/led-triggers.c +0
-0if (led_cdev->trigger && !strcmp (led_cdev->trigger->name, trig->name)) len +=sprintf (buf+len,"[%s] " , trig->name);include/linux/leds.h +3
-0@@ -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);
Signature · 2 lines
Best regards, Jacek Anaszewski
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 32 lines · Heiner Kallweit
Quoted 31 lines
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 30 lines
Hi Heiner,
Quoted 14 lines · Heiner Kallweit
Add basic support for RGB triggers. Triggers with flag
LED_TRIG_CAP_RGBset are available to RGB LED devices only.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)drivers/leds/led-triggers.c +0
-0drivers/leds/led-triggers.c static LIST_HEAD(trigger_list); · line 38
Quoted 9 lines
drivers/leds/led-triggers.c +6
-0@@ -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?
Quoted 8 lines
Quoted 4 lines
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.
Quoted 20 lines
Quoted 19 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, · line 63
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0ssize_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.
Quoted 7 lines
Quoted 7 lines
drivers/leds/led-triggers.c +1
-3down_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 ;
Quoted 2 lines
This seems to be an unrelated cleanup. Please submit it separately.
OK
Quoted 37 lines
Quoted 35 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr, · line 95
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0} }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.
Quoted 20 lines
Quoted 18 lines
drivers/leds/led-triggers.c +0
-0if (led_cdev->trigger && !strcmp (led_cdev->trigger->name, trig->name)) len +=sprintf (buf+len,"[%s] " , trig->name);include/linux/leds.h +3
-0@@ -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);
Quoted 48 lines · Heiner Kallweit
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 32 lines · Heiner Kallweit
Quoted 31 lines
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 30 lines
Hi Heiner,
Quoted 14 lines · Heiner Kallweit
Add basic support for RGB triggers. Triggers with flag
LED_TRIG_CAP_RGBset are available to RGB LED devices only.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)drivers/leds/led-triggers.c +0
-0drivers/leds/led-triggers.c static LIST_HEAD(trigger_list); · line 38
Quoted 9 lines
drivers/leds/led-triggers.c +6
-0@@ -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?
Quoted 8 lines
Quoted 4 lines
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:
Quoted 7 lines
#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.
Quoted 64 lines
Quoted 20 lines
Quoted 19 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, · line 63
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0ssize_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.
Quoted 7 lines
Quoted 7 lines
drivers/leds/led-triggers.c +1
-3down_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 ;Quoted 2 lines
This seems to be an unrelated cleanup. Please submit it separately.
OK
Quoted 37 lines
Quoted 35 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr, · line 95
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0} }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.
Quoted 20 lines
Quoted 18 lines
drivers/leds/led-triggers.c +0
-0if (led_cdev->trigger && !strcmp (led_cdev->trigger->name, trig->name)) len +=sprintf (buf+len,"[%s] " , trig->name);include/linux/leds.h +3
-0@@ -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);
Signature · 2 lines
Best regards, Jacek Anaszewski
Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
Quoted 54 lines · Heiner Kallweit
Quoted 53 lines
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 32 lines · Heiner Kallweit
Quoted 31 lines
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 30 lines
Hi Heiner,
Quoted 14 lines · Heiner Kallweit
Add basic support for RGB triggers. Triggers with flag
LED_TRIG_CAP_RGBset are available to RGB LED devices only.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)drivers/leds/led-triggers.c +0
-0drivers/leds/led-triggers.c static LIST_HEAD(trigger_list); · line 38
Quoted 9 lines
drivers/leds/led-triggers.c +6
-0@@ -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?
Quoted 8 lines
Quoted 4 lines
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
Quoted 7 lines
Quoted 2 lines
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.
Quoted 66 lines
Quoted 64 lines
Quoted 20 lines
Quoted 19 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, · line 63
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0ssize_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.
Quoted 7 lines
Quoted 7 lines
drivers/leds/led-triggers.c +1
-3down_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 ;Quoted 2 lines
This seems to be an unrelated cleanup. Please submit it separately.
OK
Quoted 37 lines
Quoted 35 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr, · line 95
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0} }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.
Quoted 20 lines
Quoted 18 lines
drivers/leds/led-triggers.c +0
-0if (led_cdev->trigger && !strcmp (led_cdev->trigger->name, trig->name)) len +=sprintf (buf+len,"[%s] " , trig->name);include/linux/leds.h +3
-0@@ -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);
Quoted 58 lines · Heiner Kallweit
Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
Quoted 57 lines · Heiner Kallweit
Quoted 55 lines
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 32 lines · Heiner Kallweit
Quoted 31 lines
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 30 lines
Hi Heiner,
Quoted 14 lines · Heiner Kallweit
Add basic support for RGB triggers. Triggers with flag
LED_TRIG_CAP_RGBset are available to RGB LED devices only.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)drivers/leds/led-triggers.c +0
-0drivers/leds/led-triggers.c static LIST_HEAD(trigger_list); · line 38
Quoted 9 lines
drivers/leds/led-triggers.c +6
-0@@ -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?
Quoted 8 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 73 lines
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.Quoted 66 lines
Quoted 64 lines
Quoted 20 lines
Quoted 19 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, · line 63
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0ssize_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.
Quoted 7 lines
Quoted 7 lines
drivers/leds/led-triggers.c +1
-3down_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 ;Quoted 2 lines
This seems to be an unrelated cleanup. Please submit it separately.
OK
Quoted 37 lines
Quoted 35 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr, · line 95
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0} }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.
Quoted 20 lines
Quoted 18 lines
drivers/leds/led-triggers.c +0
-0if (led_cdev->trigger && !strcmp (led_cdev->trigger->name, trig->name)) len +=sprintf (buf+len,"[%s] " , trig->name);include/linux/leds.h +3
-0@@ -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);
Signature · 2 lines
Best regards, Jacek Anaszewski
Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
Quoted 63 lines · Heiner Kallweit
Quoted 62 lines
Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
Quoted 61 lines · Heiner Kallweit
Quoted 48 lines
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 32 lines · Heiner Kallweit
Quoted 31 lines
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 30 lines
Hi Heiner,
Quoted 14 lines · Heiner Kallweit
Add basic support for RGB triggers. Triggers with flag
LED_TRIG_CAP_RGBset are available to RGB LED devices only.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)drivers/leds/led-triggers.c +0
-0drivers/leds/led-triggers.c static LIST_HEAD(trigger_list); · line 38
Quoted 9 lines
drivers/leds/led-triggers.c +6
-0@@ -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?
Quoted 8 lines
Quoted 4 lines
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:Quoted 7 lines
#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.
Quoted 7 lines
Quoted 3 lines
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
Kconfigsymbol 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.
Quoted 75 lines
Quoted 73 lines
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.Quoted 66 lines
Quoted 64 lines
Quoted 20 lines
Quoted 19 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, · line 63
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0ssize_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.
Quoted 7 lines
Quoted 7 lines
drivers/leds/led-triggers.c +1
-3down_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 ;Quoted 2 lines
This seems to be an unrelated cleanup. Please submit it separately.
OK
Quoted 37 lines
Quoted 35 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr, · line 95
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0} }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.
Quoted 20 lines
Quoted 18 lines
drivers/leds/led-triggers.c +0
-0if (led_cdev->trigger && !strcmp (led_cdev->trigger->name, trig->name)) len +=sprintf (buf+len,"[%s] " , trig->name);include/linux/leds.h +3
-0@@ -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);
Quoted 68 lines · Heiner Kallweit
Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
Quoted 67 lines · Heiner Kallweit
Quoted 65 lines
Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
Quoted 54 lines · Heiner Kallweit
Quoted 53 lines
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 32 lines · Heiner Kallweit
Quoted 31 lines
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 30 lines
Hi Heiner,
Quoted 14 lines · Heiner Kallweit
Add basic support for RGB triggers. Triggers with flag
LED_TRIG_CAP_RGBset are available to RGB LED devices only.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)drivers/leds/led-triggers.c +0
-0drivers/leds/led-triggers.c static LIST_HEAD(trigger_list); · line 38
Quoted 9 lines
drivers/leds/led-triggers.c +6
-0@@ -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?
Quoted 8 lines
Quoted 4 lines
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
Quoted 7 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
If RGB trigger was made dependent on LED RGB class, then the related
Kconfigsymbol 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:
#ifIS_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.
Quoted 3 lines
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.
Quoted 86 lines
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.Quoted 75 lines
Quoted 73 lines
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.Quoted 66 lines
Quoted 64 lines
Quoted 20 lines
Quoted 19 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, · line 63
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0ssize_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.
Quoted 7 lines
Quoted 7 lines
drivers/leds/led-triggers.c +1
-3down_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 ;Quoted 2 lines
This seems to be an unrelated cleanup. Please submit it separately.
OK
Quoted 37 lines
Quoted 35 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr, · line 95
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0} }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.
Quoted 20 lines
Quoted 18 lines
drivers/leds/led-triggers.c +0
-0if (led_cdev->trigger && !strcmp (led_cdev->trigger->name, trig->name)) len +=sprintf (buf+len,"[%s] " , trig->name);include/linux/leds.h +3
-0@@ -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);
Signature · 2 lines
Best regards, Jacek Anaszewski
Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
Quoted 78 lines · Heiner Kallweit
Quoted 68 lines
Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
Quoted 67 lines · Heiner Kallweit
Quoted 65 lines
Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
Quoted 54 lines · Heiner Kallweit
Quoted 53 lines
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 32 lines · Heiner Kallweit
Quoted 31 lines
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 30 lines
Hi Heiner,
Quoted 14 lines · Heiner Kallweit
Add basic support for RGB triggers. Triggers with flag
LED_TRIG_CAP_RGBset are available to RGB LED devices only.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)drivers/leds/led-triggers.c +0
-0drivers/leds/led-triggers.c static LIST_HEAD(trigger_list); · line 38
Quoted 9 lines
drivers/leds/led-triggers.c +6
-0@@ -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?
Quoted 8 lines
Quoted 4 lines
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
Quoted 7 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
If RGB trigger was made dependent on LED RGB class, then the related
Kconfigsymbol 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:
#ifIS_ENABLED(CONFIG_LEDS_CLASS_RGB)
trigger_function()
#endif
Quoted 5 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 88 lines
Quoted 86 lines
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.Quoted 75 lines
Quoted 73 lines
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.Quoted 66 lines
Quoted 64 lines
Quoted 20 lines
Quoted 19 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, · line 63
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0ssize_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.
Quoted 7 lines
Quoted 7 lines
drivers/leds/led-triggers.c +1
-3down_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 ;Quoted 2 lines
This seems to be an unrelated cleanup. Please submit it separately.
OK
Quoted 37 lines
Quoted 35 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr, · line 95
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0} }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.
Quoted 20 lines
Quoted 18 lines
drivers/leds/led-triggers.c +0
-0if (led_cdev->trigger && !strcmp (led_cdev->trigger->name, trig->name)) len +=sprintf (buf+len,"[%s] " , trig->name);include/linux/leds.h +3
-0@@ -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);
Quoted 92 lines · Heiner Kallweit
Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
Quoted 78 lines · Heiner Kallweit
Quoted 68 lines
Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
Quoted 67 lines · Heiner Kallweit
Quoted 65 lines
Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
Quoted 54 lines · Heiner Kallweit
Quoted 53 lines
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 32 lines · Heiner Kallweit
Quoted 31 lines
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 30 lines
Hi Heiner,
Quoted 14 lines · Heiner Kallweit
Add basic support for RGB triggers. Triggers with flag
LED_TRIG_CAP_RGBset are available to RGB LED devices only.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)drivers/leds/led-triggers.c +0
-0drivers/leds/led-triggers.c static LIST_HEAD(trigger_list); · line 38
Quoted 9 lines
drivers/leds/led-triggers.c +6
-0@@ -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?
Quoted 8 lines
Quoted 4 lines
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
Quoted 7 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
If RGB trigger was made dependent on LED RGB class, then the related
Kconfigsymbol 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:
#ifIS_ENABLED(CONFIG_LEDS_CLASS_RGB)
trigger_function()
#endifQuoted 5 lines
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.Quoted 4 lines
Quoted 3 lines
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.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 91 lines
Quoted 88 lines
Quoted 86 lines
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.Quoted 75 lines
Quoted 73 lines
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.Quoted 66 lines
Quoted 64 lines
Quoted 20 lines
Quoted 19 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, · line 63
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0ssize_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.
Quoted 7 lines
Quoted 7 lines
drivers/leds/led-triggers.c +1
-3down_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 ;Quoted 2 lines
This seems to be an unrelated cleanup. Please submit it separately.
OK
Quoted 37 lines
Quoted 35 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr, · line 95
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0} }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.
Quoted 20 lines
Quoted 18 lines
drivers/leds/led-triggers.c +0
-0if (led_cdev->trigger && !strcmp (led_cdev->trigger->name, trig->name)) len +=sprintf (buf+len,"[%s] " , trig->name);include/linux/leds.h +3
-0@@ -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);
Signature · 2 lines
Best regards, Jacek Anaszewski
Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
Quoted 93 lines · Heiner Kallweit
Quoted 92 lines
Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
Quoted 78 lines · Heiner Kallweit
Quoted 68 lines
Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
Quoted 67 lines · Heiner Kallweit
Quoted 65 lines
Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
Quoted 54 lines · Heiner Kallweit
Quoted 53 lines
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 32 lines · Heiner Kallweit
Quoted 31 lines
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 30 lines
Hi Heiner,
Quoted 14 lines · Heiner Kallweit
Add basic support for RGB triggers. Triggers with flag
LED_TRIG_CAP_RGBset are available to RGB LED devices only.Signed-off-by Heiner Kallweit <[email protected]>--- drivers/leds/led-triggers.c | 15 ++++++++++++--- include/linux/leds.h | 3 +++ 2 files changed, 15 insertions(+), 3 deletions(-)drivers/leds/led-triggers.c +0
-0drivers/leds/led-triggers.c static LIST_HEAD(trigger_list); · line 38
Quoted 9 lines
drivers/leds/led-triggers.c +6
-0@@ -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?
Quoted 8 lines
Quoted 4 lines
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
Quoted 7 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
If RGB trigger was made dependent on LED RGB class, then the related
Kconfigsymbol 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:
#ifIS_ENABLED(CONFIG_LEDS_CLASS_RGB)
trigger_function()
#endifQuoted 5 lines
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.Quoted 4 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 96 lines
In the aformentioned
drivers/tty/vt/keyboard.cwe have even more genericIS_ENABLED(CONFIG_LEDS_TRIGGERS)guard anyway.Quoted 91 lines
Quoted 88 lines
Quoted 86 lines
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.Quoted 75 lines
Quoted 73 lines
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.Quoted 66 lines
Quoted 64 lines
Quoted 20 lines
Quoted 19 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, · line 63
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0ssize_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.
Quoted 7 lines
Quoted 7 lines
drivers/leds/led-triggers.c +1
-3down_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 ;Quoted 2 lines
This seems to be an unrelated cleanup. Please submit it separately.
OK
Quoted 37 lines
Quoted 35 lines
Quoted 9 lines
drivers/leds/led-triggers.c ssize_t led_trigger_show(struct device *dev, struct device_attribute *attr, · line 95
Quoted 9 lines
drivers/leds/led-triggers.c +2
-0} }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.
Quoted 20 lines
Quoted 18 lines
drivers/leds/led-triggers.c +0
-0if (led_cdev->trigger && !strcmp (led_cdev->trigger->name, trig->name)) len +=sprintf (buf+len,"[%s] " , trig->name);include/linux/leds.h +3
-0@@ -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);
Quoted 97 lines · Heiner Kallweit
Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
Quoted 96 lines · Heiner Kallweit
Quoted 95 lines
Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
Quoted 78 lines · Heiner Kallweit
Quoted 68 lines
Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
Quoted 67 lines · Heiner Kallweit
Quoted 65 lines
Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
Quoted 54 lines · Heiner Kallweit
Quoted 53 lines
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 32 lines · Heiner Kallweit
Quoted 31 lines
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 30 lines
Hi Heiner,
Quoted 23 lines · Heiner Kallweit
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 <[email protected]>
---
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?
Quoted 8 lines
Quoted 4 lines
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
Quoted 7 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
If RGB trigger was made dependent on LED RGB class, then the related
Kconfigsymbol 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:
#ifIS_ENABLED(CONFIG_LEDS_CLASS_RGB)
trigger_function()
#endifQuoted 5 lines
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.Quoted 7 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 99 lines
Quoted 96 lines
In the aformentioned
drivers/tty/vt/keyboard.cwe have even more genericIS_ENABLED(CONFIG_LEDS_TRIGGERS)guard anyway.Quoted 91 lines
Quoted 88 lines
Quoted 86 lines
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.Quoted 75 lines
Quoted 73 lines
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.Quoted 66 lines
Quoted 64 lines
Quoted 20 lines
Quoted 19 lines
Quoted 9 lines
Quoted 9 lines
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.
Quoted 7 lines
Quoted 7 lines
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;Quoted 2 lines
This seems to be an unrelated cleanup. Please submit it separately.
OK
Quoted 37 lines
Quoted 35 lines
Quoted 9 lines
Quoted 9 lines
}
}
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.
Quoted 20 lines
Quoted 18 lines
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);
Signature · 2 lines
Best regards, Jacek Anaszewski
Am 23.03.2016 um 17:02 schrieb Jacek Anaszewski:
Quoted 104 lines · Heiner Kallweit
Quoted 103 lines
Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
Quoted 101 lines · Heiner Kallweit
Quoted 92 lines
Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
Quoted 78 lines · Heiner Kallweit
Quoted 68 lines
Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
Quoted 67 lines · Heiner Kallweit
Quoted 65 lines
Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
Quoted 54 lines · Heiner Kallweit
Quoted 53 lines
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 32 lines · Heiner Kallweit
Quoted 31 lines
Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
Quoted 30 lines
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 <[email protected]>
---
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?Quoted 8 lines
Quoted 4 lines
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
Quoted 7 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
If RGB trigger was made dependent on LED RGB class, then the related
Kconfigsymbol 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:
#ifIS_ENABLED(CONFIG_LEDS_CLASS_RGB)
trigger_function()
#endifQuoted 5 lines
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.Quoted 4 lines
Quoted 3 lines
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.Quoted 5 lines
Quoted 3 lines
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.
Quoted 7 lines
Quoted 2 lines
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 inled_trigger_store()andled_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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 7 lines
Quoted 2 lines
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.
Quoted 101 lines
Quoted 99 lines
Quoted 96 lines
In the aformentioned
drivers/tty/vt/keyboard.cwe have even more genericIS_ENABLED(CONFIG_LEDS_TRIGGERS)guard anyway.Quoted 91 lines
Quoted 88 lines
Quoted 86 lines
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.Quoted 75 lines
Quoted 73 lines
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.Quoted 66 lines
Quoted 64 lines
Quoted 20 lines
Quoted 19 lines
Quoted 9 lines
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.
Quoted 7 lines
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;Quoted 2 lines
This seems to be an unrelated cleanup. Please submit it separately.
OK
Quoted 37 lines
Quoted 35 lines
Quoted 9 lines
}
}
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.
Quoted 20 lines
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);
Quoted 108 lines · Heiner Kallweit
Am 23.03.2016 um 17:02 schrieb Jacek Anaszewski:
Quoted 107 lines · Heiner Kallweit
Quoted 105 lines
Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
Quoted 93 lines · Heiner Kallweit
Quoted 92 lines
Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
Quoted 78 lines · Heiner Kallweit
Quoted 68 lines
Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
Quoted 67 lines · Heiner Kallweit
Quoted 65 lines
Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
Quoted 54 lines · Heiner Kallweit
Quoted 53 lines
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 32 lines · Heiner Kallweit
Quoted 31 lines
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 <[email protected]>
---
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?Quoted 8 lines
Quoted 4 lines
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
Quoted 7 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
If RGB trigger was made dependent on LED RGB class, then the related
Kconfigsymbol 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:
#ifIS_ENABLED(CONFIG_LEDS_CLASS_RGB)
trigger_function()
#endifQuoted 5 lines
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.Quoted 4 lines
Quoted 3 lines
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.Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 4 lines
My main objection is that
led_trigger_is_supported()would be redundant inled_trigger_store()andled_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.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
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.
Quoted 117 lines
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_eventthen (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.
Quoted 101 lines
Quoted 99 lines
Quoted 96 lines
In the aformentioned
drivers/tty/vt/keyboard.cwe have even more genericIS_ENABLED(CONFIG_LEDS_TRIGGERS)guard anyway.Quoted 91 lines
Quoted 88 lines
Quoted 86 lines
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.Quoted 75 lines
Quoted 73 lines
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.Quoted 66 lines
Quoted 64 lines
Quoted 20 lines
Quoted 19 lines
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;Quoted 2 lines
This seems to be an unrelated cleanup. Please submit it separately.
OK
Quoted 37 lines
Quoted 35 lines
}
}
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);
Signature · 2 lines
Best regards, Jacek Anaszewski
Am 24.03.2016 um 14:23 schrieb Jacek Anaszewski:
Quoted 138 lines · Heiner Kallweit
Quoted 108 lines
Am 23.03.2016 um 17:02 schrieb Jacek Anaszewski:
Quoted 107 lines · Heiner Kallweit
Quoted 105 lines
Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
Quoted 93 lines · Heiner Kallweit
Quoted 92 lines
Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
Quoted 78 lines · Heiner Kallweit
Quoted 68 lines
Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
Quoted 67 lines · Heiner Kallweit
Quoted 65 lines
Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
Quoted 54 lines · Heiner Kallweit
Quoted 53 lines
Am 18.03.2016 um 14:10 schrieb Jacek Anaszewski:
Quoted 40 lines · Heiner Kallweit
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 <[email protected]>
---
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
Quoted 7 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
If RGB trigger was made dependent on LED RGB class, then the related
Kconfigsymbol 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:
#ifIS_ENABLED(CONFIG_LEDS_CLASS_RGB)
trigger_function()
#endifQuoted 5 lines
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.Quoted 4 lines
Quoted 3 lines
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.Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 4 lines
My main objection is that
led_trigger_is_supported()would be redundant inled_trigger_store()andled_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.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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 toled-rgb-core.cQuoted 7 lines
Quoted 7 lines
Quoted 2 lines
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.
Quoted 9 lines
Quoted 1 line
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()inled_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.
Quoted 119 lines
Quoted 117 lines
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_eventthen (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.
Quoted 101 lines
Quoted 99 lines
Quoted 96 lines
In the aformentioned
drivers/tty/vt/keyboard.cwe have even more genericIS_ENABLED(CONFIG_LEDS_TRIGGERS)guard anyway.Quoted 91 lines
Quoted 88 lines
Quoted 86 lines
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.Quoted 75 lines
Quoted 73 lines
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.Quoted 66 lines
Quoted 64 lines
Quoted 22 lines
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
Quoted 37 lines
}
}
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);
Quoted 148 lines · Heiner Kallweit
Am 24.03.2016 um 14:23 schrieb Jacek Anaszewski:
Quoted 138 lines · Heiner Kallweit
Quoted 108 lines
Am 23.03.2016 um 17:02 schrieb Jacek Anaszewski:
Quoted 107 lines · Heiner Kallweit
Quoted 105 lines
Am 23.03.2016 um 09:32 schrieb Jacek Anaszewski:
Quoted 93 lines · Heiner Kallweit
Quoted 92 lines
Am 22.03.2016 um 17:00 schrieb Jacek Anaszewski:
Quoted 78 lines · Heiner Kallweit
Quoted 68 lines
Am 22.03.2016 um 09:05 schrieb Jacek Anaszewski:
Quoted 67 lines · Heiner Kallweit
Quoted 65 lines
Am 21.03.2016 um 16:35 schrieb Jacek Anaszewski:
Quoted 54 lines · Heiner Kallweit
Quoted 53 lines
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 <[email protected]>
---
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;
#endifQuoted 7 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 3 lines
If RGB trigger was made dependent on LED RGB class, then the related
Kconfigsymbol 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:
#ifIS_ENABLED(CONFIG_LEDS_CLASS_RGB)
trigger_function()
#endifQuoted 5 lines
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.Quoted 4 lines
Quoted 3 lines
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.Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 4 lines
My main objection is that
led_trigger_is_supported()would be redundant inled_trigger_store()andled_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.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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 toled-rgb-core.cQuoted 7 lines
Quoted 7 lines
Quoted 2 lines
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.Quoted 9 lines
Quoted 1 line
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()inled_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.
Quoted 2 lines
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.
Quoted 129 lines
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 fromled_trigger_register()most likely is no option as this would for sure cause calling driverprobe()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.
Quoted 119 lines
Quoted 117 lines
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_eventthen (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.
Quoted 101 lines
Quoted 99 lines
Quoted 96 lines
In the aformentioned
drivers/tty/vt/keyboard.cwe have even more genericIS_ENABLED(CONFIG_LEDS_TRIGGERS)guard anyway.Quoted 91 lines
Quoted 88 lines
Quoted 86 lines
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.Quoted 75 lines
Quoted 73 lines
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.Quoted 66 lines
Quoted 64 lines
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);
Signature · 2 lines
Best Regards, Jacek Anaszewski