[RFC PATCH 0/1] Add generic inverted led triggers

[RFC PATCH 0/1] Add generic inverted led triggers

[Tobias Schramm]

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

[RFC PATCH 1/1] leds: add generic inverted led trigger support

[Tobias Schramm]

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 <t.schramm@manjaro.org>
---
 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

Re: [RFC PATCH 1/1] leds: add generic inverted led trigger support

[Jacek Anaszewski]

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 <t.schramm@manjaro.org>

-- 
Best regards,
Jacek Anaszewski

Re: [RFC PATCH 0/1] Add generic inverted led triggers

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1412 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RFC PATCH 1/1] leds: add generic inverted led trigger support

[Tobias Schramm]

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

Re: [RFC PATCH 0/1] Add generic inverted led triggers

[Tobias Schramm]

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