This patch adds generic inverted LED triggers. With this patch applied
any trigger can be used with inverted brightness levels by appending
"-inverted" to the name of a trigger.
This is can be useful for devices that do not have dedicated LEDs for e.g.
disk activity indication. With this patch applied the power led can be set
to default-state = on and trigger = disk-activity-inverted. Then the led
will be on by default, indicating the power state of the device but it
will turn off briefly whenever there is disk activity.
I think dual-use of LEDs might come in handy for quite a few devices since
a lot of embedded boards and upcoming ARM based notebooks do only have one
or two LEDs.
Best regards,
Tobias
Tobias Schramm (1):
leds: add generic inverted led trigger support
drivers/leds/led-triggers.c | 148 +++++++++++++++++++++++++++---------
include/linux/leds.h | 1 +
2 files changed, 111 insertions(+), 38 deletions(-)
--
2.24.1
This commit allows inversion of triggers by appending "-inverted" to the
name of a trigger.
This can be useful when dealing with device that lack separate LEDs for
indicating e.g. disk activity. With this commit applied the power LED
can be used as a disk activity indicator by setting its default state to
on and using the "disk-activity-inverted" trigger. The LED will then turn
off briefly when there is disk activity.
Signed-off-by: Tobias Schramm <[email protected]>
---
drivers/leds/led-triggers.c | 148 +++++++++++++++++++++++++++---------
include/linux/leds.h | 1 +
2 files changed, 111 insertions(+), 38 deletions(-)
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 79e30d2cb7a5..c40c58c6e345 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -27,14 +27,80 @@ LIST_HEAD(trigger_list);
/* Used by LED Class */
+#define TRIGGER_INVERT_SUFFIX "-inverted"
+
+/*
+ * Check suffix of trigger name agains TRIGGER_INVERT_SUFFIX
+ */
+static bool led_trigger_is_inverted(const char *trigname)
+{
+ if (strlen(trigname) >= strlen(TRIGGER_INVERT_SUFFIX)) {
+ return !strcmp(trigname + strlen(trigname) -
+ strlen(TRIGGER_INVERT_SUFFIX),
+ TRIGGER_INVERT_SUFFIX);
+ }
+
+ return false;
+}
+
+/*
+ * Get length of trigger name name without TRIGGER_INVERT_SUFFIX
+ */
+static size_t led_trigger_get_name_len(const char *trigname)
+{
+ // Subtract length of TRIGGER_INVERT_SUFFIX if trigger is inverted
+ if (led_trigger_is_inverted(trigname))
+ return strlen(trigname) - strlen(TRIGGER_INVERT_SUFFIX);
+ return strlen(trigname);
+}
+
+/*
+ * Find and set led trigger by name
+ */
+static int led_trigger_set_str_(struct led_classdev *led_cdev,
+ const char *trigname, bool lock)
+{
+ struct led_trigger *trig;
+ bool inverted = led_trigger_is_inverted(trigname);
+ size_t len = led_trigger_get_name_len(trigname);
+
+ down_read(&triggers_list_lock);
+ list_for_each_entry(trig, &trigger_list, next_trig) {
+ /* Compare trigger name without inversion suffix */
+ if (strlen(trig->name) == len &&
+ !strncmp(trigname, trig->name, len)) {
+ if (lock)
+ down_write(&led_cdev->trigger_lock);
+ led_trigger_set(led_cdev, trig);
+ if (inverted)
+ led_cdev->flags |= LED_INVERT_TRIGGER;
+ else
+ led_cdev->flags &= ~LED_INVERT_TRIGGER;
+ if (lock)
+ up_write(&led_cdev->trigger_lock);
+
+ up_read(&triggers_list_lock);
+ return 0;
+ }
+ }
+ /* we come here only if trigname matches no trigger */
+ up_read(&triggers_list_lock);
+ return -EINVAL;
+}
+
+#define led_trigger_set_str(cdev, name) led_trigger_set_str_(cdev, name, true)
+#define led_trigger_set_str_unlocked(cdev, name) \
+ led_trigger_set_str_(cdev, name, false)
+
+
ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
struct bin_attribute *bin_attr, char *buf,
loff_t pos, size_t count)
{
struct device *dev = kobj_to_dev(kobj);
struct led_classdev *led_cdev = dev_get_drvdata(dev);
- struct led_trigger *trig;
int ret = count;
+ char *name;
mutex_lock(&led_cdev->led_access);
@@ -48,20 +114,10 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
goto unlock;
}
- down_read(&triggers_list_lock);
- list_for_each_entry(trig, &trigger_list, next_trig) {
- if (sysfs_streq(buf, trig->name)) {
- 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;
- }
- }
- /* we come here only if buf matches no trigger */
- ret = -EINVAL;
- up_read(&triggers_list_lock);
+ name = strim(buf);
+ ret = led_trigger_set_str(led_cdev, name);
+ if (!ret)
+ ret = count;
unlock:
mutex_unlock(&led_cdev->led_access);
@@ -93,12 +149,22 @@ static int led_trigger_format(char *buf, size_t size,
led_cdev->trigger ? "none" : "[none]");
list_for_each_entry(trig, &trigger_list, next_trig) {
- bool hit = led_cdev->trigger &&
- !strcmp(led_cdev->trigger->name, trig->name);
+ bool hit = led_cdev->trigger == trig;
+ bool inverted = led_cdev->flags & LED_INVERT_TRIGGER;
+
+ /* print non-inverted trigger */
+ len += led_trigger_snprintf(buf + len, size - len,
+ " %s%s%s",
+ hit && !inverted ? "[" : "",
+ trig->name,
+ hit && !inverted ? "]" : "");
+ /* print inverted trigger */
len += led_trigger_snprintf(buf + len, size - len,
- " %s%s%s", hit ? "[" : "",
- trig->name, hit ? "]" : "");
+ " %s%s"TRIGGER_INVERT_SUFFIX"%s",
+ hit && inverted ? "[" : "",
+ trig->name,
+ hit && inverted ? "]" : "");
}
len += led_trigger_snprintf(buf + len, size - len, "\n");
@@ -235,21 +301,15 @@ EXPORT_SYMBOL_GPL(led_trigger_remove);
void led_trigger_set_default(struct led_classdev *led_cdev)
{
- struct led_trigger *trig;
+ bool found;
if (!led_cdev->default_trigger)
return;
down_read(&triggers_list_lock);
- down_write(&led_cdev->trigger_lock);
- list_for_each_entry(trig, &trigger_list, next_trig) {
- if (!strcmp(led_cdev->default_trigger, trig->name)) {
- led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
- led_trigger_set(led_cdev, trig);
- break;
- }
- }
- up_write(&led_cdev->trigger_lock);
+ found = !led_trigger_set_str(led_cdev, led_cdev->default_trigger);
+ if (found)
+ led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
up_read(&triggers_list_lock);
}
EXPORT_SYMBOL_GPL(led_trigger_set_default);
@@ -292,11 +352,14 @@ int led_trigger_register(struct led_trigger *trig)
/* Register with any LEDs that have this as a default trigger */
down_read(&leds_list_lock);
list_for_each_entry(led_cdev, &leds_list, node) {
+ bool found;
+
down_write(&led_cdev->trigger_lock);
- if (!led_cdev->trigger && led_cdev->default_trigger &&
- !strcmp(led_cdev->default_trigger, trig->name)) {
- led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
- led_trigger_set(led_cdev, trig);
+ if (!led_cdev->trigger && led_cdev->default_trigger) {
+ found = !led_trigger_set_str_unlocked(led_cdev,
+ led_cdev->default_trigger);
+ if (found)
+ led_cdev->flags |= LED_INIT_DEFAULT_TRIGGER;
}
up_write(&led_cdev->trigger_lock);
}
@@ -369,8 +432,14 @@ void led_trigger_event(struct led_trigger *trig,
return;
read_lock(&trig->leddev_list_lock);
- list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
- led_set_brightness(led_cdev, brightness);
+ list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) {
+ /* Reverse brightness if LED is inverted */
+ if (led_cdev->flags & LED_INVERT_TRIGGER)
+ led_set_brightness(led_cdev,
+ led_cdev->max_brightness - brightness);
+ else
+ led_set_brightness(led_cdev, brightness);
+ }
read_unlock(&trig->leddev_list_lock);
}
EXPORT_SYMBOL_GPL(led_trigger_event);
@@ -388,10 +457,13 @@ static void led_trigger_blink_setup(struct led_trigger *trig,
read_lock(&trig->leddev_list_lock);
list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) {
- if (oneshot)
+ bool trigger_inverted =
+ !!(led_cdev->flags & LED_INVERT_TRIGGER);
+ if (oneshot) {
+ /* use logical xnor to determine inversion parameter */
led_blink_set_oneshot(led_cdev, delay_on, delay_off,
- invert);
- else
+ (!!invert) == trigger_inverted);
+ } else
led_blink_set(led_cdev, delay_on, delay_off);
}
read_unlock(&trig->leddev_list_lock);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 75353e5f9d13..fa707170b69e 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -74,6 +74,7 @@ struct led_classdev {
#define LED_BRIGHT_HW_CHANGED BIT(21)
#define LED_RETAIN_AT_SHUTDOWN BIT(22)
#define LED_INIT_DEFAULT_TRIGGER BIT(23)
+#define LED_INVERT_TRIGGER BIT(24)
/* set_brightness_work / blink_timer flags, atomic, private. */
unsigned long work_flags;
--
2.24.1
Hi Tobias,
We have already some triggers that accomplish that by introducing
a custom sysfs file. It seems less intrusive to do the same for
ledtrig-disk.c. You could also try to make this a generic feature
for all LED triggers.
On 3/8/20 3:27 PM, Tobias Schramm wrote:
> This commit allows inversion of triggers by appending "-inverted" to the
> name of a trigger.
>
> This can be useful when dealing with device that lack separate LEDs for
> indicating e.g. disk activity. With this commit applied the power LED
> can be used as a disk activity indicator by setting its default state to
> on and using the "disk-activity-inverted" trigger. The LED will then turn
> off briefly when there is disk activity.
>
> Signed-off-by: Tobias Schramm <[email protected]>
--
Best regards,
Jacek Anaszewski
Hi!
> This patch adds generic inverted LED triggers. With this patch applied
> any trigger can be used with inverted brightness levels by appending
> "-inverted" to the name of a trigger.
Not a big fan (sorry).
We have already _way_ too many triggers, we don't want to have twice
that much.
> This is can be useful for devices that do not have dedicated LEDs for e.g.
> disk activity indication. With this patch applied the power led can be set
> to default-state = on and trigger = disk-activity-inverted. Then the led
> will be on by default, indicating the power state of the device but it
> will turn off briefly whenever there is disk activity.
Better implementation might be to have a trigger attribute doing the
inverting.
> I think dual-use of LEDs might come in handy for quite a few devices since
> a lot of embedded boards and upcoming ARM based notebooks do only have one
> or two LEDs.
Inverting really does not work with all the triggers; numlock-inverted
will not get too many
users. always-on-inverted... blink-inverted.... I guess it does make
sense for disk activity (but be warned disk can be continuously active
for quite a while).
What triggers do you think make sense inverted?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Jacek,
thanks dor the feedback.
> We have already some triggers that accomplish that by introducing
> a custom sysfs file.
[...]
> You could also try to make this a generic feature
> for all LED triggers.
Based on the other feedback I got I will do just that. Pavel is right,
we do already have a lot of triggers, doubling the number of triggers is
probably not the best idea.
Tobias
Hi Pavel,
thanks for the feedback.
> Not a big fan (sorry).
>
> We have already _way_ too many triggers, we don't want to have twice
> that much.
True. Doubling the amount of triggers is probably not a good idea.
>
> Better implementation might be to have a trigger attribute doing the
> inverting.
I agree. Especially since Jacek pointed out that some triggers do that
already.
>
> Inverting really does not work with all the triggers; numlock-inverted
> will not get too many
> users. always-on-inverted... blink-inverted.... I guess it does make
> sense for disk activity (but be warned disk can be continuously active
> for quite a while).
>
> What triggers do you think make sense inverted?
I think all kinds of activity indicators (disk-activity, mmc, usb, ide,
nand, cpu, network, etc.) make sense. Guess I'll add a flags field to
the led_trigger struct and have an invertible flag that specifies
whether a trigger should be invertible or not.
Thanks again,
Tobias