[RFC PATCH 0/9] leds: Add support for hw initiated hw control trigger transition

[RFC PATCH 0/9] leds: Add support for hw initiated hw control trigger transition

[Rong Zhang]

Hi all,

Some laptops can tune their keyboard backlight according to ambient
light sensors (auto mode). This capability is essentially a hw control
trigger. Meanwhile, such laptops also offer a shrotcut for cycling
through brightness levels and auto mode. For example, on ThinkBook,
pressing Fn+Space cycles keyboard backlight levels in the following
sequence:

  1 => 2 => 0 => auto => 1 ...

Recent ThinkPad models should have similar sequence too.

However, there are some issues preventing us from using hw control
trigger:

1. We want a mechanism to tell userspace which trigger is the hw control
   trigger, so that userspace can determine if auto mode is on/off or
   turing it on/off programmatically without obtaining the hw control
   trigger's name via other channels
2. Turing on/off auto mode via the shortcut cannot activate/deactivate
   the hw control trigger, making the software state out-of-sync
3. Even with #1 resolved, deactivating the hw control trigger after
   receiving the event indicating "auto => 1" has a side effect of
   emitting LED_OFF, breaking the shortcut cycle

This RFC series tries to demonstrate a path on solving these issues:

- Introduce an attribute called trigger_may_offload, so that userspace
   can determine:
   - if the LED device supports hw control (supported => visible)
   - which trigger is the hw control trigger
   - if the hw control trigger is selected
   - if the hw control trigger is in hw control (i.e., offloaded)
     - A callback offloaded() is added so that LED triggers can report
       their hw control state
- Add led_trigger_notify_hw_control_changed() interface, so that LED
  drivers can notify the LED core about hardware initiated hw control
  state transitions. The LED core will then determine if the transition
  is allowed and turning on/off the hw control trigger accordingly
- Tune the logic of trigger deactivation so that it won't emit LED_OFF
  when the deactivation is triggered by hardware

The last two patches are included into the RFC series to demonstrate how
to utilize these interfaces to add support for auto keyboard backlight
to ThinkBook. They will be submitted separately once the dust settles.

Currently no Kconfig entry is provided to disable either interface. If
needed, I will add one later.

[ Summary of other approaches ]

< custom attribute >

Pros:
- simplicity, KISS
- no need to touch the LED core
- extensible as long as it has a sensor-neutral name
  - a sensor-related name could potentially lead to a mess if a future
    device implements auto mode based on multiple different sensors

Cons:
- must have zero influence on brightness_set[_blocking] callbacks
  in order not to break triggers
  - potential interference with triggers and the brightness attribute
- weird semantic (an attribute other than "brightness" and "trigger"
  changes the brightness)

< hw control trigger (this series) >

Pros:
- mutually exclusive with other triggers (hence less chaos)
- semantic correctness
- acts as an aggregate switch to turn on/off auto mode even a future
  device implements auto mode based on multiple different sensors
  - extensibility (through trigger attributes)

Cons:
- complexity

[ Previous discussion threads ]

https://lore.kernel.org/r/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@app.fastmail.com

https://lore.kernel.org/r/1dbfcf656cdb4af0299f90d7426d2ec7e2b8ac9e.camel@rong.moe

Thanks,
Rong

Rong Zhang (9):
  leds: Load trigger modules on-demand if used as hw control trigger
  leds: Add callback offloaded() to query the state of hw control
    trigger
  leds: cros_ec: Implement offloaded() callback for trigger
  leds: turris-omnia: Implement offloaded() callback for trigger
  leds: trigger: netdev: Implement offloaded() callback
  leds: Add trigger_may_offload attribute
  leds: trigger: Add led_trigger_notify_hw_control_changed() interface
  platform/x86: ideapad-laptop: Decouple HW & cdev brightness for kbd
    backlight
  platform/x86: ideapad-laptop: Fully support auto kbd backlight

 .../obsolete/sysfs-class-led-trigger-netdev   |  15 ++
 Documentation/ABI/testing/sysfs-class-led     |  22 ++
 .../testing/sysfs-class-led-trigger-netdev    |  13 --
 Documentation/leds/leds-class.rst             |  72 ++++++-
 drivers/leds/led-class.c                      |  23 +++
 drivers/leds/led-triggers.c                   | 176 +++++++++++++++-
 drivers/leds/leds-cros_ec.c                   |   6 +
 drivers/leds/leds-turris-omnia.c              |   7 +
 drivers/leds/leds.h                           |   3 +
 drivers/leds/trigger/ledtrig-netdev.c         |  10 +
 drivers/platform/x86/lenovo/Kconfig           |   1 +
 drivers/platform/x86/lenovo/ideapad-laptop.c  | 194 ++++++++++++++----
 include/linux/leds.h                          |   6 +
 13 files changed, 492 insertions(+), 56 deletions(-)
 create mode 100644 Documentation/ABI/obsolete/sysfs-class-led-trigger-netdev


base-commit: a75cb869a8ccc88b0bc7a44e1597d9c7995c56e5
-- 
2.51.0

[RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Rong Zhang]

In the following patches, we are about to support hardware initiated
trigger transitions to/from the device's hw control trigger. In case
the LED hardware switches itself to hw control mode, hw control trigger
must be loaded before so that the transition can be processed.

Load the trigger module specified by hw_control_trigger, so that
hardware initiated trigger transitions can be processed when the hw
control trigger is compiled as a module.

Signed-off-by: Rong Zhang <i@rong.moe>
---
 drivers/leds/led-class.c    |  1 +
 drivers/leds/led-triggers.c | 33 +++++++++++++++++++++++++++++++++
 drivers/leds/leds.h         |  1 +
 3 files changed, 35 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index d34a194535604..0fa45f22246e3 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -576,6 +576,7 @@ int led_classdev_register_ext(struct device *parent,
 
 #ifdef CONFIG_LEDS_TRIGGERS
 	led_trigger_set_default(led_cdev);
+	led_load_hw_control_trigger(led_cdev);
 #endif
 
 	mutex_unlock(&led_cdev->led_access);
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index b1223218bda11..3066bc91a5f94 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -313,6 +313,39 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_trigger_set_default);
 
+static inline bool led_match_hw_control_trigger(struct led_classdev *led_cdev,
+						struct led_trigger *trig)
+{
+	return (!strcmp(led_cdev->hw_control_trigger, trig->name) &&
+		trigger_relevant(led_cdev, trig));
+}
+
+void led_load_hw_control_trigger(struct led_classdev *led_cdev)
+{
+	struct led_trigger *trig;
+	bool found = false;
+
+	if (!led_cdev->hw_control_trigger)
+		return;
+
+	/* default_trigger is handled by led_trigger_set_default(). */
+	if (led_cdev->default_trigger &&
+	    !strcmp(led_cdev->default_trigger, led_cdev->hw_control_trigger))
+		return;
+
+	down_read(&triggers_list_lock);
+	list_for_each_entry(trig, &trigger_list, next_trig) {
+		found = led_match_hw_control_trigger(led_cdev, trig);
+		if (found)
+			break;
+	}
+	up_read(&triggers_list_lock);
+
+	if (!found)
+		request_module_nowait("ledtrig:%s", led_cdev->hw_control_trigger);
+}
+EXPORT_SYMBOL_GPL(led_load_hw_control_trigger);
+
 /* LED Trigger Interface */
 
 int led_trigger_register(struct led_trigger *trig)
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index bee46651e068f..e85afd4d04fd0 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -21,6 +21,7 @@ void led_init_core(struct led_classdev *led_cdev);
 void led_stop_software_blink(struct led_classdev *led_cdev);
 void led_set_brightness_nopm(struct led_classdev *led_cdev, unsigned int value);
 void led_set_brightness_nosleep(struct led_classdev *led_cdev, unsigned int value);
+void led_load_hw_control_trigger(struct led_classdev *led_cdev);
 ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
 			const struct bin_attribute *attr, char *buf,
 			loff_t pos, size_t count);
-- 
2.51.0

[RFC PATCH 2/9] leds: Add callback offloaded() to query the state of hw control trigger

[Rong Zhang]

Currently, we have multiple triggers implementing hw control. However,
the LED core doesn't really know the hw control state since the
coordination is done directly between the hw control trigger and the LED
device.

In the following patches, we are about to support hardware initiated
trigger transitions to/from hw control trigger. The hw control state is
a fundamental factor of determining if a transition should be permitted.

Add a callback offloaded() so that the LED core can query the hw control
state and use it to make decisions in hw initiated trigger transitions.

Signed-off-by: Rong Zhang <i@rong.moe>
---
 Documentation/leds/leds-class.rst | 5 +++++
 include/linux/leds.h              | 1 +
 2 files changed, 6 insertions(+)

diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index 5db620ed27aa2..84665200a88dc 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -235,6 +235,11 @@ LED driver must implement the following API to support hw control:
                 Returns a pointer to a struct device or NULL if nothing
                 is currently attached.
 
+LED trigger must implement the following API to support hw control:
+    - offloaded:
+                return a boolean indicating if the trigger is offloaded to
+                hardware.
+
 LED driver can activate additional modes by default to workaround the
 impossibility of supporting each different mode on the supported trigger.
 Examples are hardcoding the blink speed to a set interval, enable special
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b16b803cc1ac5..7332034a43c85 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -485,6 +485,7 @@ struct led_trigger {
 	const char	 *name;
 	int		(*activate)(struct led_classdev *led_cdev);
 	void		(*deactivate)(struct led_classdev *led_cdev);
+	bool		(*offloaded)(struct led_classdev *led_cdev);
 
 	/* Brightness set by led_trigger_event */
 	enum led_brightness brightness;
-- 
2.51.0

[RFC PATCH 3/9] leds: cros_ec: Implement offloaded() callback for trigger

[Rong Zhang]

"chromeos-auto" is a private hw control trigger which always stays in hw
control mode. Implement offloaded() callback with its return value to be
always true to reflect this.

Signed-off-by: Rong Zhang <i@rong.moe>
---
 drivers/leds/leds-cros_ec.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/leds/leds-cros_ec.c b/drivers/leds/leds-cros_ec.c
index bea3cc3fbfd2d..f48e3cf6ccf68 100644
--- a/drivers/leds/leds-cros_ec.c
+++ b/drivers/leds/leds-cros_ec.c
@@ -86,12 +86,18 @@ static int cros_ec_led_trigger_activate(struct led_classdev *led_cdev)
 	return cros_ec_led_send_cmd(priv->cros_ec, &arg);
 }
 
+static bool cros_ec_led_trigger_offloaded(struct led_classdev *led_cdev)
+{
+	return true;
+}
+
 static struct led_hw_trigger_type cros_ec_led_trigger_type;
 
 static struct led_trigger cros_ec_led_trigger = {
 	.name = "chromeos-auto",
 	.trigger_type = &cros_ec_led_trigger_type,
 	.activate = cros_ec_led_trigger_activate,
+	.offloaded = cros_ec_led_trigger_offloaded,
 };
 
 static int cros_ec_led_brightness_set_blocking(struct led_classdev *led_cdev,
-- 
2.51.0

[RFC PATCH 4/9] leds: turris-omnia: Implement offloaded() callback for trigger

[Rong Zhang]

"omnia-mcu" is a private hw control trigger which always stays in hw
control mode. Implement offloaded() callback with its return value to be
always true to reflect this.

Meanwhile, declare it as a hw control trigger as it's forgotten before.

Signed-off-by: Rong Zhang <i@rong.moe>
---
 drivers/leds/leds-turris-omnia.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index 25ee5c1eb820b..8e016ca864030 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -195,10 +195,16 @@ static void omnia_hwtrig_deactivate(struct led_classdev *cdev)
 			err);
 }
 
+static bool omnia_hwtrig_offloaded(struct led_classdev *cdev)
+{
+	return true;
+}
+
 static struct led_trigger omnia_hw_trigger = {
 	.name		= "omnia-mcu",
 	.activate	= omnia_hwtrig_activate,
 	.deactivate	= omnia_hwtrig_deactivate,
+	.offloaded	= omnia_hwtrig_offloaded,
 	.trigger_type	= &omnia_hw_trigger_type,
 };
 
@@ -251,6 +257,7 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	 * by LED class from the linux,default-trigger property.
 	 */
 	cdev->default_trigger = omnia_hw_trigger.name;
+	cdev->hw_control_trigger = omnia_hw_trigger.name;
 
 	/* Put the LED into software mode */
 	ret = omnia_cmd_write_u8(client, OMNIA_CMD_LED_MODE, OMNIA_CMD_LED_MODE_LED(led->reg) |
-- 
2.51.0

[RFC PATCH 5/9] leds: trigger: netdev: Implement offloaded() callback

[Rong Zhang]

"netdev" can run in hw control mode according to hw capabilities and
trigger options. Implement offloaded() callback to provide its hw
control state.

Signed-off-by: Rong Zhang <i@rong.moe>
---
 drivers/leds/trigger/ledtrig-netdev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 12cb3311ea220..edde167b60a54 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -749,10 +749,18 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
 	kfree(trigger_data);
 }
 
+static bool netdev_trig_offloaded(struct led_classdev *led_cdev)
+{
+	struct led_netdev_data *trigger_data = led_get_trigger_data(led_cdev);
+
+	return trigger_data->hw_control;
+}
+
 static struct led_trigger netdev_led_trigger = {
 	.name = "netdev",
 	.activate = netdev_trig_activate,
 	.deactivate = netdev_trig_deactivate,
+	.offloaded = netdev_trig_offloaded,
 	.groups = netdev_trig_groups,
 };
 
-- 
2.51.0

[RFC PATCH 6/9] leds: Add trigger_may_offload attribute

[Rong Zhang]

Currently, we have multiple triggers implementing hw control. Only
"netdev" provides a custom attribute to determine if it's offloaded to
hardware (i.e., in hw control mode). For other triggers, there is no
obvious way userspace can determine the hw control state
programmatically. Moreover, userspace can't even query if an LED device
supports hw control and the name of hw control trigger.

Add a new attribute "trigger_may_offload" to the LED core, so that
userspace can determine:

- if the LED device supports hw control (supported => visible)
- which trigger is the hw control trigger
- if the hw control trigger is selected
- if the hw control trigger is in hw control

Signed-off-by: Rong Zhang <i@rong.moe>
---
 .../obsolete/sysfs-class-led-trigger-netdev   | 15 +++++++
 Documentation/ABI/testing/sysfs-class-led     | 22 ++++++++++
 .../testing/sysfs-class-led-trigger-netdev    | 13 ------
 Documentation/leds/leds-class.rst             |  4 +-
 drivers/leds/led-class.c                      | 22 ++++++++++
 drivers/leds/led-triggers.c                   | 42 +++++++++++++++++++
 drivers/leds/leds.h                           |  2 +
 drivers/leds/trigger/ledtrig-netdev.c         |  2 +
 8 files changed, 108 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/ABI/obsolete/sysfs-class-led-trigger-netdev

diff --git a/Documentation/ABI/obsolete/sysfs-class-led-trigger-netdev b/Documentation/ABI/obsolete/sysfs-class-led-trigger-netdev
new file mode 100644
index 0000000000000..d1a7713d45700
--- /dev/null
+++ b/Documentation/ABI/obsolete/sysfs-class-led-trigger-netdev
@@ -0,0 +1,15 @@
+What:		/sys/class/leds/<led>/offloaded
+Date:		March 2026
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Communicate whether the LED trigger modes are offloaded to
+		hardware or whether software fallback is used.
+
+		If 0, the LED is using software fallback to blink.
+
+		If 1, the LED blinking in requested mode is offloaded to
+		hardware.
+
+		Since 7.1, /sys/class/leds/<led>/trigger_may_offload provides
+		a generic method to query the offloaded state of supported
+		trigger, so this has been deprecated.
diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
index 0313b82644f24..403f411b09036 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -78,6 +78,28 @@ Description:
 		(which would often be configured in the device tree for the
 		hardware).
 
+What:		/sys/class/leds/<led>/trigger_may_offload
+Date:		March 2026
+KernelVersion:	7.1
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Names and states of triggers that may be offloaded to hardware.
+		Such triggers are also called "hw control trigger" in some
+		context.
+
+		Only exists when the LED supports trigger offload.
+
+		Returns a list of hw control triggers on read. The optional
+		brackets around the trigger name indicate the state of the
+		current trigger:
+
+		- `foo_trigger`: the trigger is not selected.
+		- `<foo_trigger>`: the trigger is selected, but falls back to
+		  software blink for some reason (e.g., incompatible trigger
+		  parameters)
+		- `[foo_trigger]`: the trigger is selected and offloaded to
+		  hardware.
+
 What:		/sys/class/leds/<led>/inverted
 Date:		January 2011
 KernelVersion:	2.6.38
diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
index ed46b37ab8a28..396d37a4b820d 100644
--- a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev
@@ -62,19 +62,6 @@ Description:
 		When offloaded is true, the blink interval is controlled by
 		hardware and won't reflect the value set in interval.
 
-What:		/sys/class/leds/<led>/offloaded
-Date:		Jun 2023
-KernelVersion:	6.5
-Contact:	linux-leds@vger.kernel.org
-Description:
-		Communicate whether the LED trigger modes are offloaded to
-		hardware or whether software fallback is used.
-
-		If 0, the LED is using software fallback to blink.
-
-		If 1, the LED blinking in requested mode is offloaded to
-		hardware.
-
 What:		/sys/class/leds/<led>/link_10
 Date:		Jun 2023
 KernelVersion:	6.5
diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index 84665200a88dc..cf7733e30bace 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -177,7 +177,9 @@ limited to blink but also to turn off or on autonomously.
 To support this feature, a LED needs to implement various additional
 ops and needs to declare specific support for the supported triggers.
 
-With hw control we refer to the LED driven by hardware.
+With hw control we refer to the LED driven by hardware. In hw control mode,
+the current trigger is offloaded to hardware. The `trigger_may_offload`
+attribute can be used to query supported triggers and their states.
 
 LED driver must define the following value to support hw control:
 
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 0fa45f22246e3..8a661e2616f06 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -90,8 +90,30 @@ static const struct bin_attribute *const led_trigger_bin_attrs[] = {
 	&bin_attr_trigger,
 	NULL,
 };
+
+static DEVICE_ATTR(trigger_may_offload, 0644, led_trigger_may_offload_show, NULL);
+static struct attribute *led_trigger_attrs[] = {
+	&dev_attr_trigger_may_offload.attr,
+	NULL,
+};
+static umode_t led_trigger_is_visible(struct kobject *kobj,
+				      struct attribute *attr,
+				      int idx)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	if (attr == &dev_attr_trigger_may_offload.attr &&
+	    !led_cdev->hw_control_trigger)
+		return 0;
+
+	return attr->mode;
+}
+
 static const struct attribute_group led_trigger_group = {
 	.bin_attrs = led_trigger_bin_attrs,
+	.attrs = led_trigger_attrs,
+	.is_visible = led_trigger_is_visible,
 };
 #endif
 
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 3066bc91a5f94..f8100381fc684 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -346,6 +346,48 @@ void led_load_hw_control_trigger(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_load_hw_control_trigger);
 
+/*
+ * Caller must ensure led_cdev->trigger_lock held,
+ * and led_cdev->trigger->name must match led_cdev->hw_control_trigger.
+ */
+static bool led_trigger_get_offloaded(struct led_classdev *led_cdev)
+{
+	if (likely(led_cdev->trigger->offloaded))
+		return led_cdev->trigger->offloaded(led_cdev);
+
+	dev_err_once(led_cdev->dev,
+		     "hw control trigger %s doesn't implement offloaded(), this is a bug\n",
+		     led_cdev->trigger->name);
+	return false;
+}
+
+ssize_t led_trigger_may_offload_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	bool hit, offloaded = false;
+	int len;
+
+	mutex_lock(&led_cdev->led_access);
+	down_read(&led_cdev->trigger_lock);
+
+	hit = led_cdev->trigger && led_match_hw_control_trigger(led_cdev, led_cdev->trigger);
+	if (hit)
+		offloaded = led_trigger_get_offloaded(led_cdev);
+
+	/* [offloaded] <active_but_not_offloaded> inactive */
+	len = led_trigger_snprintf(buf, PAGE_SIZE,
+				   hit ? (offloaded ? "[%s]\n" : "<%s>\n")
+				       : "%s\n",
+				   led_cdev->hw_control_trigger);
+
+	up_read(&led_cdev->trigger_lock);
+	mutex_unlock(&led_cdev->led_access);
+
+	return len;
+}
+EXPORT_SYMBOL_GPL(led_trigger_may_offload_show);
+
 /* LED Trigger Interface */
 
 int led_trigger_register(struct led_trigger *trig)
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index e85afd4d04fd0..ee0dbddd411ec 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -28,6 +28,8 @@ ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
 ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 			const struct bin_attribute *bin_attr, char *buf,
 			loff_t pos, size_t count);
+ssize_t led_trigger_may_offload_show(struct device *dev,
+				     struct device_attribute *attr, char *buf);
 
 extern struct rw_semaphore leds_list_lock;
 extern struct list_head leds_list;
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index edde167b60a54..0a4f4469ff9b0 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -482,6 +482,8 @@ static ssize_t offloaded_show(struct device *dev,
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 
+	dev_warn_once(dev, "offloaded attribute has been deprecated, see trigger_may_offload.\n");
+
 	return sprintf(buf, "%d\n", trigger_data->hw_control);
 }
 
-- 
2.51.0

[RFC PATCH 7/9] leds: trigger: Add led_trigger_notify_hw_control_changed() interface

[Rong Zhang]

Some LED devices can autonomously activate/deactivate hw control mode.
Currently, we have no mechanism for LED drivers to notify the LED core
about such events and initiate a trigger transition to reflect the
hardware state.

Add a new interface called led_trigger_notify_hw_control_changed(), so
that LED drivers can call it to notify the LED core about the
transition.

The interface only allows two transitions:

1. "none" => hw control trigger (offloaded)
2. hw control trigger (offloaded) => "none"

If the current trigger is neither hw control trigger nor "none", or if
hw control trigger is not offloaded, no trigger transition will be made.
This protects selected sw triggers.

Note that LED_OFF won't be emitted during the #2 transition, as some
hardware may have selected a new brightness level during its hardware
state transition (e.g., laptop keyboards with a shortcut cycling through
different backlight brightnesses and auto mode).

The interface is designed as a void function as any failure should be
non-fatal and the result of transition should not have any impact on the
LED drivers' event handling procedures.

Signed-off-by: Rong Zhang <i@rong.moe>
---
 Documentation/leds/leds-class.rst |  63 +++++++++++++++++++
 drivers/leds/led-triggers.c       | 101 +++++++++++++++++++++++++++++-
 include/linux/leds.h              |   5 ++
 3 files changed, 166 insertions(+), 3 deletions(-)

diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index cf7733e30bace..4d84db1067b43 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -255,9 +255,72 @@ the end use hw_control_set to activate hw control.
 A trigger can use hw_control_get to check if a LED is already in hw control
 and init their flags.
 
+Alternatively, a private trigger can be implemented along with the LED driver
+if the hw control mode of the LED doesn't fit any generic trigger. To associate
+the private trigger with the LED classdev, their `trigger_type` must be the same.
+The name of the private trigger must be the same as `hw_control_trigger`. Since
+both the LED classdev and the private trigger are in the same LED driver, it's not
+necessary for them to coordinate via `hw_control_*` callbacks.
+
 When the LED is in hw control, no software blink is possible and doing so
 will effectively disable hw control.
 
+Hardware initiated hw control mode transition
+==========================================
+
+Some hardware can autonomously activate/deactivate hw control mode. After the
+mode transition, the LED hardware notifies the LED driver. To update the current
+trigger accordingly, call `led_trigger_notify_hw_control_changed` on the
+classdev. The driver must set `hw_control_trigger` before registering, or else
+calling this is a bug and will trigger a WARN_ON. An LED driver that implements
+a private trigger can pass a pointer to the private trigger as the last
+parameter, otherwise NULL should be passed. The private trigger must have been
+properly registered (see above) and named after `hw_control_trigger`, or else a
+WARN_ON will be triggered.
+
+For convenience, `hw_control_trigger` refers to a trigger name defined in LED
+classdev, while "hw control trigger" refers to a unique trigger with the
+same name as the former.
+
+Only two transitions are defined:
+
+- "none" => hw control trigger (offloaded):
+        This happens when the hardware autonomously activates hw control mode
+        and when "none" (i.e., no trigger) is currently active. If the hw
+        control trigger is already active and offloaded during the hw control
+        mode transition, this is essentially a no-op.
+
+        The activation sequence for the hw control trigger will be executed as
+        normal. After switching to the hw control trigger, its offloaded status
+        is checked and must be true. Failing to set the offloaded status
+        appropriately will trigger a WARN_ON.
+
+        The LED driver must be able to handle the activation sequence even if
+        the hardware is currently under hw control mode. If the hardware can
+        handle hw control mode transition idempotently, the LED driver probably
+        already has this capability. Otherwise, the LED driver should take extra
+        care to handle the transition.
+
+        If error occurs in the activation sequence, the LED Trigger core reverts
+        the effective trigger to "none".
+
+- hw control trigger (offloaded) => "none"
+        This happens when the hardware autonomously deactivates hw control mode
+        and when the hw control trigger is currently active and offloaded. If
+        "none" (i.e., no trigger) is active during the hw control mode
+        transition, this is essentially a no-op.
+
+        The deactivation sequence for the hw control trigger will be executed as
+        normal, except that the current LED brightness is retained. The reason
+        for keeping the brightness unchanged is that some hardware may choose a
+        specific brightness instead of simply turning off the LED during its hw
+        control mode transition.
+
+        The idempotence rule also applies.
+
+If the current trigger is neither hw control trigger nor "none", or if hw
+control trigger is not offloaded, no transition will be made.
+
 Known Issues
 ============
 
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index f8100381fc684..0d0279ac8291b 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -7,6 +7,7 @@
  * Author: Richard Purdie <rpurdie@openedhand.com>
  */
 
+#include <linux/bug.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -162,8 +163,8 @@ ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
 }
 EXPORT_SYMBOL_GPL(led_trigger_read);
 
-/* Caller must ensure led_cdev->trigger_lock held */
-int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
+static int __led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig,
+			     bool hw_triggered)
 {
 	char *event = NULL;
 	char *envp[2];
@@ -194,7 +195,15 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 		led_cdev->trigger_data = NULL;
 		led_cdev->activated = false;
 		led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;
-		led_set_brightness(led_cdev, LED_OFF);
+
+		/*
+		 * Hardware may have select a new brightness level during its
+		 * hw control mode transition, so only reset brightness if we
+		 * are switching to another trigger or if the switching is not
+		 * hardware triggered.
+		 */
+		if (trig || !hw_triggered)
+			led_set_brightness(led_cdev, LED_OFF);
 	}
 	if (trig) {
 		spin_lock(&trig->leddev_list_lock);
@@ -258,6 +267,12 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 
 	return ret;
 }
+
+/* Caller must ensure led_cdev->trigger_lock held */
+int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
+{
+	return __led_trigger_set(led_cdev, trig, false);
+}
 EXPORT_SYMBOL_GPL(led_trigger_set);
 
 void led_trigger_remove(struct led_classdev *led_cdev)
@@ -478,6 +493,86 @@ int devm_led_trigger_register(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_led_trigger_register);
 
+static void led_trigger_do_hw_control_transition(struct led_classdev *led_cdev, bool activate,
+						 struct led_trigger *hc_trig)
+{
+	int err = 0;
+
+	if (!led_cdev->trigger) {
+		/* "none" => hw control trigger (offloaded). */
+		if (activate) {
+			err = __led_trigger_set(led_cdev, hc_trig, true);
+
+			/*
+			 * hw control trigger must recognize the current hw state during
+			 * its activation and mark itself as offloaded.
+			 */
+			WARN_ON(!err && !led_trigger_get_offloaded(led_cdev));
+		}
+	} else if (led_cdev->trigger == hc_trig && led_trigger_get_offloaded(led_cdev)) {
+		/* hw control trigger (offloaded) => "none". */
+		if (!activate)
+			err = __led_trigger_set(led_cdev, NULL, true);
+	} else {
+		/* Other trigger is active, or hw control trigger is not offloaded. */
+		dev_dbg(led_cdev->dev,
+			"Do not %s hw control trigger %s while %s is active",
+			activate ? "activate" : "deactivate", hc_trig->name,
+			led_cdev->trigger->name);
+
+		return;
+	}
+
+	if (err)
+		dev_warn(led_cdev->dev, "Failed to %s hw control trigger %s: %d",
+			 activate ? "activate" : "deactivate", hc_trig->name, err);
+}
+
+/* Caller must ensure led_cdev->led_access held */
+void led_trigger_notify_hw_control_changed(struct led_classdev *led_cdev, bool activate,
+					   struct led_trigger *priv_trig)
+{
+	struct led_trigger *trig;
+
+	down_write(&led_cdev->trigger_lock);
+
+	if (WARN_ON(!led_cdev->hw_control_trigger))
+		goto out;
+
+	/* Fast path: hw control trigger is a private trigger. */
+	if (priv_trig) {
+		if (WARN_ON(!led_match_hw_control_trigger(led_cdev, priv_trig)))
+			goto out;
+
+		led_trigger_do_hw_control_transition(led_cdev, activate, priv_trig);
+		goto out;
+	}
+
+	/* Fast path: hw control trigger is the current trigger. */
+	if (led_cdev->trigger && led_match_hw_control_trigger(led_cdev, led_cdev->trigger)) {
+		led_trigger_do_hw_control_transition(led_cdev, activate, led_cdev->trigger);
+		goto out;
+	}
+
+	down_read(&triggers_list_lock);
+	list_for_each_entry(trig, &trigger_list, next_trig) {
+		if (led_match_hw_control_trigger(led_cdev, trig)) {
+			led_trigger_do_hw_control_transition(led_cdev, activate, trig);
+
+			up_read(&triggers_list_lock);
+			goto out;
+		}
+	}
+	up_read(&triggers_list_lock);
+
+	dev_dbg(led_cdev->dev, "hw control trigger not found: %s",
+		led_cdev->hw_control_trigger);
+
+out:
+	up_write(&led_cdev->trigger_lock);
+}
+EXPORT_SYMBOL_GPL(led_trigger_notify_hw_control_changed);
+
 /* Simple LED Trigger Interface */
 
 void led_trigger_event(struct led_trigger *trig,
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 7332034a43c85..82578724fd60c 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -533,6 +533,8 @@ void led_trigger_blink_oneshot(struct led_trigger *trigger,
 			       int invert);
 void led_trigger_set_default(struct led_classdev *led_cdev);
 int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger);
+void led_trigger_notify_hw_control_changed(struct led_classdev *led_cdev, bool activate,
+					   struct led_trigger *priv_trig);
 void led_trigger_remove(struct led_classdev *led_cdev);
 
 static inline void led_set_trigger_data(struct led_classdev *led_cdev,
@@ -583,6 +585,9 @@ static inline int led_trigger_set(struct led_classdev *led_cdev,
 {
 	return 0;
 }
+static inline void led_trigger_notify_hw_control_changed(struct led_classdev *led_cdev,
+							 bool activate,
+							 struct led_trigger *priv_trig) {}
 
 static inline void led_trigger_remove(struct led_classdev *led_cdev) {}
 static inline void led_set_trigger_data(struct led_classdev *led_cdev) {}
-- 
2.51.0

[RFC PATCH 8/9] platform/x86: ideapad-laptop: Decouple HW & cdev brightness for kbd backlight

[Rong Zhang]

Some recent models come with an ambient light sensor (ALS). On these
models, their EC will automatically set the keyboard backlight to an
appropriate brightness when the effective "HW brightness" is 3. "HW
brightness" can't be perfectly mapped to an LED classdev brightness, but
the EC does use this predefined brightness value to represent auto mode.

Currently, the code processing keyboard backlight is coupled with LED
classdev, making it hard to expose the auto brightness (ALS) mode to the
userspace.

As the first step toward the goal, decouple HW brightness from LED
classdev brightness, and update comments about corresponding backlight
modes.

Signed-off-by: Rong Zhang <i@rong.moe>
---
 drivers/platform/x86/lenovo/ideapad-laptop.c | 114 +++++++++++++------
 1 file changed, 80 insertions(+), 34 deletions(-)

diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
index ae1ebb071fab0..b9af0294fc933 100644
--- a/drivers/platform/x86/lenovo/ideapad-laptop.c
+++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
@@ -134,10 +134,31 @@ enum {
 };
 
 /*
- * These correspond to the number of supported states - 1
- * Future keyboard types may need a new system, if there's a collision
- * KBD_BL_TRISTATE_AUTO has no way to report or set the auto state
- * so it effectively has 3 states, but needs to handle 4
+ * The enumeration has two purposes:
+ *   - as an internal identifier for all known types of keyboard backlight
+ *   - as a mandatory parameter of the KBLC command
+ *
+ * For each type, the HW brightness values are defined as follows:
+ * +--------------------------+----------+-----+------+------+
+ * |            HW brightness |        0 |   1 |    2 |    3 |
+ * | Type                     |          |     |      |      |
+ * +--------------------------+----------+-----+------+------+
+ * | KBD_BL_STANDARD          |      off |  on |  N/A |  N/A |
+ * +--------------------------+----------+-----+------+------+
+ * | KBD_BL_TRISTATE          |      off | low | high |  N/A |
+ * +--------------------------+----------+-----+------+------+
+ * | KBD_BL_TRISTATE_AUTO     |      off | low | high | auto |
+ * +--------------------------+----------+-----+------+------+
+ *
+ * We map LED classdev brightness for KBD_BL_TRISTATE_AUTO as follows:
+ * +--------------------------+----------+-----+------+
+ * |  LED classdev brightness |        0 |   1 |    2 |
+ * | Operation                |          |     |      |
+ * +--------------------------+----------+-----+------+
+ * | Read                     | off/auto | low | high |
+ * +--------------------------+----------+-----+------+
+ * | Write                    |      off | low | high |
+ * +--------------------------+----------+-----+------+
  */
 enum {
 	KBD_BL_STANDARD      = 1,
@@ -1592,7 +1613,24 @@ static int ideapad_kbd_bl_check_tristate(int type)
 	return (type == KBD_BL_TRISTATE) || (type == KBD_BL_TRISTATE_AUTO);
 }
 
-static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
+static int ideapad_kbd_bl_brightness_parse(struct ideapad_private *priv,
+					   unsigned int hw_brightness)
+{
+	/* Off, low or high */
+	if (hw_brightness <= priv->kbd_bl.led.max_brightness)
+		return hw_brightness;
+
+	/* Auto, report as off */
+	if (hw_brightness == priv->kbd_bl.led.max_brightness + 1)
+		return 0;
+
+	/* Unknown value */
+	dev_warn(&priv->platform_device->dev,
+		 "Unknown keyboard backlight value: %u", hw_brightness);
+	return -EINVAL;
+}
+
+static int ideapad_kbd_bl_hw_brightness_get(struct ideapad_private *priv)
 {
 	unsigned long value;
 	int err;
@@ -1606,21 +1644,7 @@ static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
 		if (err)
 			return err;
 
-		/* Convert returned value to brightness level */
-		value = FIELD_GET(KBD_BL_GET_BRIGHTNESS, value);
-
-		/* Off, low or high */
-		if (value <= priv->kbd_bl.led.max_brightness)
-			return value;
-
-		/* Auto, report as off */
-		if (value == priv->kbd_bl.led.max_brightness + 1)
-			return 0;
-
-		/* Unknown value */
-		dev_warn(&priv->platform_device->dev,
-			 "Unknown keyboard backlight value: %lu", value);
-		return -EINVAL;
+		return FIELD_GET(KBD_BL_GET_BRIGHTNESS, value);
 	}
 
 	err = eval_hals(priv->adev->handle, &value);
@@ -1630,6 +1654,16 @@ static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
 	return !!test_bit(HALS_KBD_BL_STATE_BIT, &value);
 }
 
+static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
+{
+	int hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
+
+	if (hw_brightness < 0)
+		return hw_brightness;
+
+	return ideapad_kbd_bl_brightness_parse(priv, hw_brightness);
+}
+
 static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_classdev *led_cdev)
 {
 	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
@@ -1637,24 +1671,31 @@ static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_cla
 	return ideapad_kbd_bl_brightness_get(priv);
 }
 
-static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness)
+static int ideapad_kbd_bl_hw_brightness_set(struct ideapad_private *priv,
+					    unsigned int hw_brightness)
 {
-	int err;
 	unsigned long value;
 	int type = priv->kbd_bl.type;
 
 	if (ideapad_kbd_bl_check_tristate(type)) {
-		if (brightness > priv->kbd_bl.led.max_brightness)
-			return -EINVAL;
-
-		value = FIELD_PREP(KBD_BL_SET_BRIGHTNESS, brightness) |
+		value = FIELD_PREP(KBD_BL_SET_BRIGHTNESS, hw_brightness) |
 			FIELD_PREP(KBD_BL_COMMAND_TYPE, type) |
 			KBD_BL_COMMAND_SET;
-		err = exec_kblc(priv->adev->handle, value);
+		return exec_kblc(priv->adev->handle, value);
 	} else {
-		err = exec_sals(priv->adev->handle, brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF);
+		value = hw_brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF;
+		return exec_sals(priv->adev->handle, value);
 	}
+}
+
+static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness)
+{
+	int err;
 
+	if (brightness > priv->kbd_bl.led.max_brightness)
+		return -EINVAL;
+
+	err = ideapad_kbd_bl_hw_brightness_set(priv, brightness);
 	if (err)
 		return err;
 
@@ -1671,6 +1712,16 @@ static int ideapad_kbd_bl_led_cdev_brightness_set(struct led_classdev *led_cdev,
 	return ideapad_kbd_bl_brightness_set(priv, brightness);
 }
 
+static void ideapad_kbd_bl_notify_known(struct ideapad_private *priv, unsigned int brightness)
+{
+	if (brightness == priv->kbd_bl.last_brightness)
+		return;
+
+	priv->kbd_bl.last_brightness = brightness;
+
+	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
+}
+
 static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
 {
 	int brightness;
@@ -1682,12 +1733,7 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
 	if (brightness < 0)
 		return;
 
-	if (brightness == priv->kbd_bl.last_brightness)
-		return;
-
-	priv->kbd_bl.last_brightness = brightness;
-
-	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
+	ideapad_kbd_bl_notify_known(priv, brightness);
 }
 
 static int ideapad_kbd_bl_init(struct ideapad_private *priv)
-- 
2.51.0

[RFC PATCH 9/9] platform/x86: ideapad-laptop: Fully support auto kbd backlight

[Rong Zhang]

Currently, the auto brightness mode of keyboard backlight maps to
brightness=0 in LED classdev. The only method to switch to such a mode
is by pressing the manufacturer-defined shortcut (Fn+Space). However, 0
is a multiplexed brightness value; writing 0 simply results in the
backlight being turned off.

With brightness processing code decoupled from LED classdev, we can now
fully support the auto brightness mode. In this mode, the keyboard
backlight is controlled by the EC according to the ambient light sensor
(ALS).

To utilize this, a private hw control trigger "ideapad-auto" is added,
with the event handling procedure calling the
led_trigger_notify_hw_control_changed() interface to activate/deactivate
the private trigger according to the current LED trigger state.

Meanwhile, block brightness changes on exit to prevent the side effect
of LED device unregistration when the private trigger is active from
resetting the brightness to zero, so that we can retain the state of
auto mode among boots.

Signed-off-by: Rong Zhang <i@rong.moe>
---
 drivers/platform/x86/lenovo/Kconfig          |  1 +
 drivers/platform/x86/lenovo/ideapad-laptop.c | 86 ++++++++++++++++++--
 2 files changed, 79 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/lenovo/Kconfig b/drivers/platform/x86/lenovo/Kconfig
index f885127b007f1..626180370add4 100644
--- a/drivers/platform/x86/lenovo/Kconfig
+++ b/drivers/platform/x86/lenovo/Kconfig
@@ -16,6 +16,7 @@ config IDEAPAD_LAPTOP
 	select INPUT_SPARSEKMAP
 	select NEW_LEDS
 	select LEDS_CLASS
+	select LEDS_TRIGGERS
 	help
 	  This is a driver for Lenovo IdeaPad netbooks contains drivers for
 	  rfkill switch, hotkey, fan control and backlight control.
diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
index b9af0294fc933..99cdd18cc1a5d 100644
--- a/drivers/platform/x86/lenovo/ideapad-laptop.c
+++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
@@ -26,6 +26,7 @@
 #include <linux/kernel.h>
 #include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/platform_profile.h>
 #include <linux/power_supply.h>
@@ -166,6 +167,8 @@ enum {
 	KBD_BL_TRISTATE_AUTO = 3,
 };
 
+#define KBD_BL_AUTO_MODE_HW_BRIGHTNESS	3
+
 #define KBD_BL_QUERY_TYPE		0x1
 #define KBD_BL_TRISTATE_TYPE		0x5
 #define KBD_BL_TRISTATE_AUTO_TYPE	0x7
@@ -1620,8 +1623,9 @@ static int ideapad_kbd_bl_brightness_parse(struct ideapad_private *priv,
 	if (hw_brightness <= priv->kbd_bl.led.max_brightness)
 		return hw_brightness;
 
-	/* Auto, report as off */
-	if (hw_brightness == priv->kbd_bl.led.max_brightness + 1)
+	/* Auto (controlled by EC according to ALS), report as off */
+	if (priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO &&
+	    hw_brightness == KBD_BL_AUTO_MODE_HW_BRIGHTNESS)
 		return 0;
 
 	/* Unknown value */
@@ -1709,9 +1713,39 @@ static int ideapad_kbd_bl_led_cdev_brightness_set(struct led_classdev *led_cdev,
 {
 	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
 
+	/*
+	 * When deinitializing: It must be the side effect of led_cdev
+	 * unregistration when our private trigger is active. We've set
+	 * LED_RETAIN_AT_SHUTDOWN to retain led_cdev brightness level. To do the
+	 * same for auto mode, gate changes and return early.
+	 */
+	if (unlikely(!priv->kbd_bl.initialized))
+		return 0;
+
 	return ideapad_kbd_bl_brightness_set(priv, brightness);
 }
 
+static int ideapad_kbd_bl_auto_trigger_activate(struct led_classdev *led_cdev)
+{
+	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
+
+	return ideapad_kbd_bl_hw_brightness_set(priv, KBD_BL_AUTO_MODE_HW_BRIGHTNESS);
+}
+
+static bool ideapad_kbd_bl_auto_trigger_offloaded(struct led_classdev *led_cdev)
+{
+	return true;
+}
+
+static struct led_hw_trigger_type ideapad_kbd_bl_auto_trigger_type;
+
+static struct led_trigger ideapad_kbd_bl_auto_trigger = {
+	.name = "ideapad-auto",
+	.trigger_type = &ideapad_kbd_bl_auto_trigger_type,
+	.activate = ideapad_kbd_bl_auto_trigger_activate,
+	.offloaded = ideapad_kbd_bl_auto_trigger_offloaded,
+};
+
 static void ideapad_kbd_bl_notify_known(struct ideapad_private *priv, unsigned int brightness)
 {
 	if (brightness == priv->kbd_bl.last_brightness)
@@ -1724,12 +1758,23 @@ static void ideapad_kbd_bl_notify_known(struct ideapad_private *priv, unsigned i
 
 static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
 {
-	int brightness;
+	int hw_brightness, brightness;
 
 	if (!priv->kbd_bl.initialized)
 		return;
 
-	brightness = ideapad_kbd_bl_brightness_get(priv);
+	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
+	if (hw_brightness < 0)
+		return;
+
+	if (priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
+		bool activate = hw_brightness == KBD_BL_AUTO_MODE_HW_BRIGHTNESS;
+
+		led_trigger_notify_hw_control_changed(&priv->kbd_bl.led, activate,
+						      &ideapad_kbd_bl_auto_trigger);
+	}
+
+	brightness = ideapad_kbd_bl_brightness_parse(priv, hw_brightness);
 	if (brightness < 0)
 		return;
 
@@ -1738,7 +1783,7 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
 
 static int ideapad_kbd_bl_init(struct ideapad_private *priv)
 {
-	int brightness, err;
+	int hw_brightness, brightness, err;
 
 	if (!priv->features.kbd_bl)
 		return -ENODEV;
@@ -1746,12 +1791,37 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
 	if (WARN_ON(priv->kbd_bl.initialized))
 		return -EEXIST;
 
-	if (ideapad_kbd_bl_check_tristate(priv->kbd_bl.type))
+	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
+	if (hw_brightness < 0)
+		return hw_brightness;
+
+	switch (priv->kbd_bl.type) {
+	case KBD_BL_TRISTATE_AUTO:
+		err = devm_led_trigger_register(&priv->platform_device->dev,
+						&ideapad_kbd_bl_auto_trigger);
+		if (err)
+			return err;
+
+		priv->kbd_bl.led.trigger_type       = &ideapad_kbd_bl_auto_trigger_type;
+		priv->kbd_bl.led.hw_control_trigger = ideapad_kbd_bl_auto_trigger.name;
+
+		/* HW remembers the last brightness level, including auto mode. */
+		if (hw_brightness == KBD_BL_AUTO_MODE_HW_BRIGHTNESS)
+			priv->kbd_bl.led.default_trigger = ideapad_kbd_bl_auto_trigger.name;
+
+		fallthrough;
+	case KBD_BL_TRISTATE:
 		priv->kbd_bl.led.max_brightness = 2;
-	else
+		break;
+	case KBD_BL_STANDARD:
 		priv->kbd_bl.led.max_brightness = 1;
+		break;
+	default:
+		/* This has already been validated by ideapad_check_features(). */
+		unreachable();
+	}
 
-	brightness = ideapad_kbd_bl_brightness_get(priv);
+	brightness = ideapad_kbd_bl_brightness_parse(priv, hw_brightness);
 	if (brightness < 0)
 		return brightness;
 
-- 
2.51.0

Re: [RFC PATCH 7/9] leds: trigger: Add led_trigger_notify_hw_control_changed() interface

[Randy Dunlap]

On 2/27/26 11:06 AM, Rong Zhang wrote:
> diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
> index cf7733e30bace..4d84db1067b43 100644
> --- a/Documentation/leds/leds-class.rst
> +++ b/Documentation/leds/leds-class.rst
> @@ -255,9 +255,72 @@ the end use hw_control_set to activate hw control.
>  A trigger can use hw_control_get to check if a LED is already in hw control
>  and init their flags.
>  
> +Alternatively, a private trigger can be implemented along with the LED driver
> +if the hw control mode of the LED doesn't fit any generic trigger. To associate
> +the private trigger with the LED classdev, their `trigger_type` must be the same.
> +The name of the private trigger must be the same as `hw_control_trigger`. Since
> +both the LED classdev and the private trigger are in the same LED driver, it's not
> +necessary for them to coordinate via `hw_control_*` callbacks.
> +
>  When the LED is in hw control, no software blink is possible and doing so
>  will effectively disable hw control.
>  
> +Hardware initiated hw control mode transition
> +==========================================

The heading underline line must be at least as long as the heading line,
so please add more ================ here.

thanks.
-- 
~Randy

Re: [RFC PATCH 0/9] leds: Add support for hw initiated hw control trigger transition

[Mark Pearson]

Hi Rong,

On Fri, Feb 27, 2026, at 2:05 PM, Rong Zhang wrote:
> Hi all,
>
> Some laptops can tune their keyboard backlight according to ambient
> light sensors (auto mode). This capability is essentially a hw control
> trigger. Meanwhile, such laptops also offer a shrotcut for cycling
> through brightness levels and auto mode. For example, on ThinkBook,
> pressing Fn+Space cycles keyboard backlight levels in the following
> sequence:
>
>   1 => 2 => 0 => auto => 1 ...
>
> Recent ThinkPad models should have similar sequence too.
>
> However, there are some issues preventing us from using hw control
> trigger:
>
> 1. We want a mechanism to tell userspace which trigger is the hw control
>    trigger, so that userspace can determine if auto mode is on/off or
>    turing it on/off programmatically without obtaining the hw control
>    trigger's name via other channels
> 2. Turing on/off auto mode via the shortcut cannot activate/deactivate
>    the hw control trigger, making the software state out-of-sync
> 3. Even with #1 resolved, deactivating the hw control trigger after
>    receiving the event indicating "auto => 1" has a side effect of
>    emitting LED_OFF, breaking the shortcut cycle
>
> This RFC series tries to demonstrate a path on solving these issues:
>
> - Introduce an attribute called trigger_may_offload, so that userspace
>    can determine:
>    - if the LED device supports hw control (supported => visible)
>    - which trigger is the hw control trigger
>    - if the hw control trigger is selected
>    - if the hw control trigger is in hw control (i.e., offloaded)
>      - A callback offloaded() is added so that LED triggers can report
>        their hw control state
> - Add led_trigger_notify_hw_control_changed() interface, so that LED
>   drivers can notify the LED core about hardware initiated hw control
>   state transitions. The LED core will then determine if the transition
>   is allowed and turning on/off the hw control trigger accordingly
> - Tune the logic of trigger deactivation so that it won't emit LED_OFF
>   when the deactivation is triggered by hardware
>
> The last two patches are included into the RFC series to demonstrate how
> to utilize these interfaces to add support for auto keyboard backlight
> to ThinkBook. They will be submitted separately once the dust settles.
>
> Currently no Kconfig entry is provided to disable either interface. If
> needed, I will add one later.
>
> [ Summary of other approaches ]
>
> < custom attribute >
>
> Pros:
> - simplicity, KISS
> - no need to touch the LED core
> - extensible as long as it has a sensor-neutral name
>   - a sensor-related name could potentially lead to a mess if a future
>     device implements auto mode based on multiple different sensors
>
> Cons:
> - must have zero influence on brightness_set[_blocking] callbacks
>   in order not to break triggers
>   - potential interference with triggers and the brightness attribute
> - weird semantic (an attribute other than "brightness" and "trigger"
>   changes the brightness)
>
> < hw control trigger (this series) >
>
> Pros:
> - mutually exclusive with other triggers (hence less chaos)
> - semantic correctness
> - acts as an aggregate switch to turn on/off auto mode even a future
>   device implements auto mode based on multiple different sensors
>   - extensibility (through trigger attributes)
>
> Cons:
> - complexity
>
> [ Previous discussion threads ]
>
> https://lore.kernel.org/r/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@app.fastmail.com
>
> https://lore.kernel.org/r/1dbfcf656cdb4af0299f90d7426d2ec7e2b8ac9e.camel@rong.moe
>
> Thanks,
> Rong
>
> Rong Zhang (9):
>   leds: Load trigger modules on-demand if used as hw control trigger
>   leds: Add callback offloaded() to query the state of hw control
>     trigger
>   leds: cros_ec: Implement offloaded() callback for trigger
>   leds: turris-omnia: Implement offloaded() callback for trigger
>   leds: trigger: netdev: Implement offloaded() callback
>   leds: Add trigger_may_offload attribute
>   leds: trigger: Add led_trigger_notify_hw_control_changed() interface
>   platform/x86: ideapad-laptop: Decouple HW & cdev brightness for kbd
>     backlight
>   platform/x86: ideapad-laptop: Fully support auto kbd backlight
>
>  .../obsolete/sysfs-class-led-trigger-netdev   |  15 ++
>  Documentation/ABI/testing/sysfs-class-led     |  22 ++
>  .../testing/sysfs-class-led-trigger-netdev    |  13 --
>  Documentation/leds/leds-class.rst             |  72 ++++++-
>  drivers/leds/led-class.c                      |  23 +++
>  drivers/leds/led-triggers.c                   | 176 +++++++++++++++-
>  drivers/leds/leds-cros_ec.c                   |   6 +
>  drivers/leds/leds-turris-omnia.c              |   7 +
>  drivers/leds/leds.h                           |   3 +
>  drivers/leds/trigger/ledtrig-netdev.c         |  10 +
>  drivers/platform/x86/lenovo/Kconfig           |   1 +
>  drivers/platform/x86/lenovo/ideapad-laptop.c  | 194 ++++++++++++++----
>  include/linux/leds.h                          |   6 +
>  13 files changed, 492 insertions(+), 56 deletions(-)
>  create mode 100644 Documentation/ABI/obsolete/sysfs-class-led-trigger-netdev
>
>
> base-commit: a75cb869a8ccc88b0bc7a44e1597d9c7995c56e5
> -- 
> 2.51.0

Thanks for your work on this.

For the series: As it's a RFC, I'm not bothering with notes on any typo's or grammer stuff.

Overall I think the implementation works and I understand it better from our initial discussions. Thank you for putting this together.

I'm not a huge fan of the term offloaded - I would lean towards just calling it hw_control (or similar). But I see it was used in the ledtrig-netdev driver so I don't feel strongly about this.

Vishnu - can you check out how this would work with the Thinkpad implementation that you've been working on, please? I think that will be helpful to highlight any design issues.

Mark

Re: [RFC PATCH 0/9] leds: Add support for hw initiated hw control trigger transition

[Vishnu Sankar]

Hi Rong and Mark,

On Thu, Mar 5, 2026 at 5:05 AM Mark Pearson <mpearson-lenovo@squebb.ca> wrote:
>
> Hi Rong,
>
> On Fri, Feb 27, 2026, at 2:05 PM, Rong Zhang wrote:
> > Hi all,
> >
> > Some laptops can tune their keyboard backlight according to ambient
> > light sensors (auto mode). This capability is essentially a hw control
> > trigger. Meanwhile, such laptops also offer a shrotcut for cycling
> > through brightness levels and auto mode. For example, on ThinkBook,
> > pressing Fn+Space cycles keyboard backlight levels in the following
> > sequence:
> >
> >   1 => 2 => 0 => auto => 1 ...
> >
> > Recent ThinkPad models should have similar sequence too.
> >
> > However, there are some issues preventing us from using hw control
> > trigger:
> >
> > 1. We want a mechanism to tell userspace which trigger is the hw control
> >    trigger, so that userspace can determine if auto mode is on/off or
> >    turing it on/off programmatically without obtaining the hw control
> >    trigger's name via other channels
> > 2. Turing on/off auto mode via the shortcut cannot activate/deactivate
> >    the hw control trigger, making the software state out-of-sync
> > 3. Even with #1 resolved, deactivating the hw control trigger after
> >    receiving the event indicating "auto => 1" has a side effect of
> >    emitting LED_OFF, breaking the shortcut cycle
> >
> > This RFC series tries to demonstrate a path on solving these issues:
> >
> > - Introduce an attribute called trigger_may_offload, so that userspace
> >    can determine:
> >    - if the LED device supports hw control (supported => visible)
> >    - which trigger is the hw control trigger
> >    - if the hw control trigger is selected
> >    - if the hw control trigger is in hw control (i.e., offloaded)
> >      - A callback offloaded() is added so that LED triggers can report
> >        their hw control state
> > - Add led_trigger_notify_hw_control_changed() interface, so that LED
> >   drivers can notify the LED core about hardware initiated hw control
> >   state transitions. The LED core will then determine if the transition
> >   is allowed and turning on/off the hw control trigger accordingly
> > - Tune the logic of trigger deactivation so that it won't emit LED_OFF
> >   when the deactivation is triggered by hardware
> >
> > The last two patches are included into the RFC series to demonstrate how
> > to utilize these interfaces to add support for auto keyboard backlight
> > to ThinkBook. They will be submitted separately once the dust settles.
> >
> > Currently no Kconfig entry is provided to disable either interface. If
> > needed, I will add one later.
> >
> > [ Summary of other approaches ]
> >
> > < custom attribute >
> >
> > Pros:
> > - simplicity, KISS
> > - no need to touch the LED core
> > - extensible as long as it has a sensor-neutral name
> >   - a sensor-related name could potentially lead to a mess if a future
> >     device implements auto mode based on multiple different sensors
> >
> > Cons:
> > - must have zero influence on brightness_set[_blocking] callbacks
> >   in order not to break triggers
> >   - potential interference with triggers and the brightness attribute
> > - weird semantic (an attribute other than "brightness" and "trigger"
> >   changes the brightness)
> >
> > < hw control trigger (this series) >
> >
> > Pros:
> > - mutually exclusive with other triggers (hence less chaos)
> > - semantic correctness
> > - acts as an aggregate switch to turn on/off auto mode even a future
> >   device implements auto mode based on multiple different sensors
> >   - extensibility (through trigger attributes)
> >
> > Cons:
> > - complexity
> >
> > [ Previous discussion threads ]
> >
> > https://lore.kernel.org/r/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@app.fastmail.com
> >
> > https://lore.kernel.org/r/1dbfcf656cdb4af0299f90d7426d2ec7e2b8ac9e.camel@rong.moe
> >
> > Thanks,
> > Rong
> >
> > Rong Zhang (9):
> >   leds: Load trigger modules on-demand if used as hw control trigger
> >   leds: Add callback offloaded() to query the state of hw control
> >     trigger
> >   leds: cros_ec: Implement offloaded() callback for trigger
> >   leds: turris-omnia: Implement offloaded() callback for trigger
> >   leds: trigger: netdev: Implement offloaded() callback
> >   leds: Add trigger_may_offload attribute
> >   leds: trigger: Add led_trigger_notify_hw_control_changed() interface
> >   platform/x86: ideapad-laptop: Decouple HW & cdev brightness for kbd
> >     backlight
> >   platform/x86: ideapad-laptop: Fully support auto kbd backlight
> >
> >  .../obsolete/sysfs-class-led-trigger-netdev   |  15 ++
> >  Documentation/ABI/testing/sysfs-class-led     |  22 ++
> >  .../testing/sysfs-class-led-trigger-netdev    |  13 --
> >  Documentation/leds/leds-class.rst             |  72 ++++++-
> >  drivers/leds/led-class.c                      |  23 +++
> >  drivers/leds/led-triggers.c                   | 176 +++++++++++++++-
> >  drivers/leds/leds-cros_ec.c                   |   6 +
> >  drivers/leds/leds-turris-omnia.c              |   7 +
> >  drivers/leds/leds.h                           |   3 +
> >  drivers/leds/trigger/ledtrig-netdev.c         |  10 +
> >  drivers/platform/x86/lenovo/Kconfig           |   1 +
> >  drivers/platform/x86/lenovo/ideapad-laptop.c  | 194 ++++++++++++++----
> >  include/linux/leds.h                          |   6 +
> >  13 files changed, 492 insertions(+), 56 deletions(-)
> >  create mode 100644 Documentation/ABI/obsolete/sysfs-class-led-trigger-netdev
> >
> >
> > base-commit: a75cb869a8ccc88b0bc7a44e1597d9c7995c56e5
> > --
> > 2.51.0
>
> Thanks for your work on this.
>
> For the series: As it's a RFC, I'm not bothering with notes on any typo's or grammer stuff.
>
> Overall I think the implementation works and I understand it better from our initial discussions. Thank you for putting this together.
>
> I'm not a huge fan of the term offloaded - I would lean towards just calling it hw_control (or similar). But I see it was used in the ledtrig-netdev driver so I don't feel strongly about this.
>
> Vishnu - can you check out how this would work with the Thinkpad implementation that you've been working on, please? I think that will be helpful to highlight any design issues.

I will use this patch and do the changes needed for the Thinkpad and
let you know soon.


>
> Mark



-- 

Regards,

      Vishnu Sankar

Re: [RFC PATCH 0/9] leds: Add support for hw initiated hw control trigger transition

[Rong Zhang]

Hi Mark,

On Wed, 2026-03-04 at 15:05 -0500, Mark Pearson wrote:
> Hi Rong,
> 
> On Fri, Feb 27, 2026, at 2:05 PM, Rong Zhang wrote:
> > Hi all,
> > 
> > Some laptops can tune their keyboard backlight according to ambient
> > light sensors (auto mode). This capability is essentially a hw control
> > trigger. Meanwhile, such laptops also offer a shrotcut for cycling
> > through brightness levels and auto mode. For example, on ThinkBook,
> > pressing Fn+Space cycles keyboard backlight levels in the following
> > sequence:
> > 
> >   1 => 2 => 0 => auto => 1 ...
> > 
> > Recent ThinkPad models should have similar sequence too.
> > 
> > However, there are some issues preventing us from using hw control
> > trigger:
> > 
> > 1. We want a mechanism to tell userspace which trigger is the hw control
> >    trigger, so that userspace can determine if auto mode is on/off or
> >    turing it on/off programmatically without obtaining the hw control
> >    trigger's name via other channels
> > 2. Turing on/off auto mode via the shortcut cannot activate/deactivate
> >    the hw control trigger, making the software state out-of-sync
> > 3. Even with #1 resolved, deactivating the hw control trigger after
> >    receiving the event indicating "auto => 1" has a side effect of
> >    emitting LED_OFF, breaking the shortcut cycle
> > 
> > This RFC series tries to demonstrate a path on solving these issues:
> > 
> > - Introduce an attribute called trigger_may_offload, so that userspace
> >    can determine:
> >    - if the LED device supports hw control (supported => visible)
> >    - which trigger is the hw control trigger
> >    - if the hw control trigger is selected
> >    - if the hw control trigger is in hw control (i.e., offloaded)
> >      - A callback offloaded() is added so that LED triggers can report
> >        their hw control state
> > - Add led_trigger_notify_hw_control_changed() interface, so that LED
> >   drivers can notify the LED core about hardware initiated hw control
> >   state transitions. The LED core will then determine if the transition
> >   is allowed and turning on/off the hw control trigger accordingly
> > - Tune the logic of trigger deactivation so that it won't emit LED_OFF
> >   when the deactivation is triggered by hardware
> > 
> > The last two patches are included into the RFC series to demonstrate how
> > to utilize these interfaces to add support for auto keyboard backlight
> > to ThinkBook. They will be submitted separately once the dust settles.
> > 
> > Currently no Kconfig entry is provided to disable either interface. If
> > needed, I will add one later.
> > 
> > [ Summary of other approaches ]
> > 
> > < custom attribute >
> > 
> > Pros:
> > - simplicity, KISS
> > - no need to touch the LED core
> > - extensible as long as it has a sensor-neutral name
> >   - a sensor-related name could potentially lead to a mess if a future
> >     device implements auto mode based on multiple different sensors
> > 
> > Cons:
> > - must have zero influence on brightness_set[_blocking] callbacks
> >   in order not to break triggers
> >   - potential interference with triggers and the brightness attribute
> > - weird semantic (an attribute other than "brightness" and "trigger"
> >   changes the brightness)
> > 
> > < hw control trigger (this series) >
> > 
> > Pros:
> > - mutually exclusive with other triggers (hence less chaos)
> > - semantic correctness
> > - acts as an aggregate switch to turn on/off auto mode even a future
> >   device implements auto mode based on multiple different sensors
> >   - extensibility (through trigger attributes)
> > 
> > Cons:
> > - complexity
> > 
> > [ Previous discussion threads ]
> > 
> > https://lore.kernel.org/r/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@app.fastmail.com
> > 
> > https://lore.kernel.org/r/1dbfcf656cdb4af0299f90d7426d2ec7e2b8ac9e.camel@rong.moe
> > 
> > Thanks,
> > Rong
> > 
> > Rong Zhang (9):
> >   leds: Load trigger modules on-demand if used as hw control trigger
> >   leds: Add callback offloaded() to query the state of hw control
> >     trigger
> >   leds: cros_ec: Implement offloaded() callback for trigger
> >   leds: turris-omnia: Implement offloaded() callback for trigger
> >   leds: trigger: netdev: Implement offloaded() callback
> >   leds: Add trigger_may_offload attribute
> >   leds: trigger: Add led_trigger_notify_hw_control_changed() interface
> >   platform/x86: ideapad-laptop: Decouple HW & cdev brightness for kbd
> >     backlight
> >   platform/x86: ideapad-laptop: Fully support auto kbd backlight
> > 
> >  .../obsolete/sysfs-class-led-trigger-netdev   |  15 ++
> >  Documentation/ABI/testing/sysfs-class-led     |  22 ++
> >  .../testing/sysfs-class-led-trigger-netdev    |  13 --
> >  Documentation/leds/leds-class.rst             |  72 ++++++-
> >  drivers/leds/led-class.c                      |  23 +++
> >  drivers/leds/led-triggers.c                   | 176 +++++++++++++++-
> >  drivers/leds/leds-cros_ec.c                   |   6 +
> >  drivers/leds/leds-turris-omnia.c              |   7 +
> >  drivers/leds/leds.h                           |   3 +
> >  drivers/leds/trigger/ledtrig-netdev.c         |  10 +
> >  drivers/platform/x86/lenovo/Kconfig           |   1 +
> >  drivers/platform/x86/lenovo/ideapad-laptop.c  | 194 ++++++++++++++----
> >  include/linux/leds.h                          |   6 +
> >  13 files changed, 492 insertions(+), 56 deletions(-)
> >  create mode 100644 Documentation/ABI/obsolete/sysfs-class-led-trigger-netdev
> > 
> > 
> > base-commit: a75cb869a8ccc88b0bc7a44e1597d9c7995c56e5
> > -- 
> > 2.51.0
> 
> Thanks for your work on this.
> 
> For the series: As it's a RFC, I'm not bothering with notes on any typo's or grammer stuff.
> 
> Overall I think the implementation works and I understand it better from our initial discussions. Thank you for putting this together.
> 
> I'm not a huge fan of the term offloaded - I would lean towards just calling it hw_control (or similar). But I see it was used in the ledtrig-netdev driver so I don't feel strongly about this.
> 

FYI, "hw control" is a historical and internal term. I used it because
it's used a lot in the documentation to the doc consistent. Otherwise
"offload(ed)" should be used. See commit 44f0fb8dfe26 ("leds: trigger:
netdev: rename 'hw_control' sysfs entry to 'offloaded'").

I admit that I was somewhat lost in the terminological soup and there
may be some room for rephrasing in my documentation and commit
messages.

Thanks,
Rong

> Vishnu - can you check out how this would work with the Thinkpad implementation that you've been working on, please? I think that will be helpful to highlight any design issues.
> 
> Mark

Re: [RFC PATCH 0/9] leds: Add support for hw initiated hw control trigger transition

[Mark Pearson]

On Thu, Mar 5, 2026, at 7:37 AM, Rong Zhang wrote:
> Hi Mark,
>
> On Wed, 2026-03-04 at 15:05 -0500, Mark Pearson wrote:
>> Hi Rong,
>> 
>> On Fri, Feb 27, 2026, at 2:05 PM, Rong Zhang wrote:
>> > Hi all,
>> > 
>> > Some laptops can tune their keyboard backlight according to ambient
>> > light sensors (auto mode). This capability is essentially a hw control
>> > trigger. Meanwhile, such laptops also offer a shrotcut for cycling
>> > through brightness levels and auto mode. For example, on ThinkBook,
>> > pressing Fn+Space cycles keyboard backlight levels in the following
>> > sequence:
>> > 
>> >   1 => 2 => 0 => auto => 1 ...
>> > 
>> > Recent ThinkPad models should have similar sequence too.
>> > 
>> > However, there are some issues preventing us from using hw control
>> > trigger:
>> > 
>> > 1. We want a mechanism to tell userspace which trigger is the hw control
>> >    trigger, so that userspace can determine if auto mode is on/off or
>> >    turing it on/off programmatically without obtaining the hw control
>> >    trigger's name via other channels
>> > 2. Turing on/off auto mode via the shortcut cannot activate/deactivate
>> >    the hw control trigger, making the software state out-of-sync
>> > 3. Even with #1 resolved, deactivating the hw control trigger after
>> >    receiving the event indicating "auto => 1" has a side effect of
>> >    emitting LED_OFF, breaking the shortcut cycle
>> > 
>> > This RFC series tries to demonstrate a path on solving these issues:
>> > 
>> > - Introduce an attribute called trigger_may_offload, so that userspace
>> >    can determine:
>> >    - if the LED device supports hw control (supported => visible)
>> >    - which trigger is the hw control trigger
>> >    - if the hw control trigger is selected
>> >    - if the hw control trigger is in hw control (i.e., offloaded)
>> >      - A callback offloaded() is added so that LED triggers can report
>> >        their hw control state
>> > - Add led_trigger_notify_hw_control_changed() interface, so that LED
>> >   drivers can notify the LED core about hardware initiated hw control
>> >   state transitions. The LED core will then determine if the transition
>> >   is allowed and turning on/off the hw control trigger accordingly
>> > - Tune the logic of trigger deactivation so that it won't emit LED_OFF
>> >   when the deactivation is triggered by hardware
>> > 
>> > The last two patches are included into the RFC series to demonstrate how
>> > to utilize these interfaces to add support for auto keyboard backlight
>> > to ThinkBook. They will be submitted separately once the dust settles.
>> > 
>> > Currently no Kconfig entry is provided to disable either interface. If
>> > needed, I will add one later.
>> > 
>> > [ Summary of other approaches ]
>> > 
>> > < custom attribute >
>> > 
>> > Pros:
>> > - simplicity, KISS
>> > - no need to touch the LED core
>> > - extensible as long as it has a sensor-neutral name
>> >   - a sensor-related name could potentially lead to a mess if a future
>> >     device implements auto mode based on multiple different sensors
>> > 
>> > Cons:
>> > - must have zero influence on brightness_set[_blocking] callbacks
>> >   in order not to break triggers
>> >   - potential interference with triggers and the brightness attribute
>> > - weird semantic (an attribute other than "brightness" and "trigger"
>> >   changes the brightness)
>> > 
>> > < hw control trigger (this series) >
>> > 
>> > Pros:
>> > - mutually exclusive with other triggers (hence less chaos)
>> > - semantic correctness
>> > - acts as an aggregate switch to turn on/off auto mode even a future
>> >   device implements auto mode based on multiple different sensors
>> >   - extensibility (through trigger attributes)
>> > 
>> > Cons:
>> > - complexity
>> > 
>> > [ Previous discussion threads ]
>> > 
>> > https://lore.kernel.org/r/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@app.fastmail.com
>> > 
>> > https://lore.kernel.org/r/1dbfcf656cdb4af0299f90d7426d2ec7e2b8ac9e.camel@rong.moe
>> > 
>> > Thanks,
>> > Rong
>> > 
>> > Rong Zhang (9):
>> >   leds: Load trigger modules on-demand if used as hw control trigger
>> >   leds: Add callback offloaded() to query the state of hw control
>> >     trigger
>> >   leds: cros_ec: Implement offloaded() callback for trigger
>> >   leds: turris-omnia: Implement offloaded() callback for trigger
>> >   leds: trigger: netdev: Implement offloaded() callback
>> >   leds: Add trigger_may_offload attribute
>> >   leds: trigger: Add led_trigger_notify_hw_control_changed() interface
>> >   platform/x86: ideapad-laptop: Decouple HW & cdev brightness for kbd
>> >     backlight
>> >   platform/x86: ideapad-laptop: Fully support auto kbd backlight
>> > 
>> >  .../obsolete/sysfs-class-led-trigger-netdev   |  15 ++
>> >  Documentation/ABI/testing/sysfs-class-led     |  22 ++
>> >  .../testing/sysfs-class-led-trigger-netdev    |  13 --
>> >  Documentation/leds/leds-class.rst             |  72 ++++++-
>> >  drivers/leds/led-class.c                      |  23 +++
>> >  drivers/leds/led-triggers.c                   | 176 +++++++++++++++-
>> >  drivers/leds/leds-cros_ec.c                   |   6 +
>> >  drivers/leds/leds-turris-omnia.c              |   7 +
>> >  drivers/leds/leds.h                           |   3 +
>> >  drivers/leds/trigger/ledtrig-netdev.c         |  10 +
>> >  drivers/platform/x86/lenovo/Kconfig           |   1 +
>> >  drivers/platform/x86/lenovo/ideapad-laptop.c  | 194 ++++++++++++++----
>> >  include/linux/leds.h                          |   6 +
>> >  13 files changed, 492 insertions(+), 56 deletions(-)
>> >  create mode 100644 Documentation/ABI/obsolete/sysfs-class-led-trigger-netdev
>> > 
>> > 
>> > base-commit: a75cb869a8ccc88b0bc7a44e1597d9c7995c56e5
>> > -- 
>> > 2.51.0
>> 
>> Thanks for your work on this.
>> 
>> For the series: As it's a RFC, I'm not bothering with notes on any typo's or grammer stuff.
>> 
>> Overall I think the implementation works and I understand it better from our initial discussions. Thank you for putting this together.
>> 
>> I'm not a huge fan of the term offloaded - I would lean towards just calling it hw_control (or similar). But I see it was used in the ledtrig-netdev driver so I don't feel strongly about this.
>> 
>
> FYI, "hw control" is a historical and internal term. I used it because
> it's used a lot in the documentation to the doc consistent. Otherwise
> "offload(ed)" should be used. See commit 44f0fb8dfe26 ("leds: trigger:
> netdev: rename 'hw_control' sysfs entry to 'offloaded'").
>
> I admit that I was somewhat lost in the terminological soup and there
> may be some room for rephrasing in my documentation and commit
> messages.
>
Fair enough :) I think that overrides my mild preferences.

Mark

Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Lee Jones]

On Sat, 28 Feb 2026, Rong Zhang wrote:

> In the following patches, we are about to support hardware initiated

Let's not talk about other commits.  Only tell us what's happening here.

> trigger transitions to/from the device's hw control trigger. In case
> the LED hardware switches itself to hw control mode, hw control trigger
> must be loaded before so that the transition can be processed.
> 
> Load the trigger module specified by hw_control_trigger, so that
> hardware initiated trigger transitions can be processed when the hw

"hardware"

> control trigger is compiled as a module.
> 
> Signed-off-by: Rong Zhang <i@rong.moe>
> ---
>  drivers/leds/led-class.c    |  1 +
>  drivers/leds/led-triggers.c | 33 +++++++++++++++++++++++++++++++++
>  drivers/leds/leds.h         |  1 +
>  3 files changed, 35 insertions(+)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index d34a194535604..0fa45f22246e3 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -576,6 +576,7 @@ int led_classdev_register_ext(struct device *parent,
>  
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	led_trigger_set_default(led_cdev);
> +	led_load_hw_control_trigger(led_cdev);

led_trigger_load_hw_control

>  #endif
>  
>  	mutex_unlock(&led_cdev->led_access);
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index b1223218bda11..3066bc91a5f94 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -313,6 +313,39 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
>  }
>  EXPORT_SYMBOL_GPL(led_trigger_set_default);
>  
> +static inline bool led_match_hw_control_trigger(struct led_classdev *led_cdev,
> +						struct led_trigger *trig)
> +{
> +	return (!strcmp(led_cdev->hw_control_trigger, trig->name) &&
> +		trigger_relevant(led_cdev, trig));


This is ugly.  Break it out and provide some commentary.

> +}
> +
> +void led_load_hw_control_trigger(struct led_classdev *led_cdev)
> +{
> +	struct led_trigger *trig;
> +	bool found = false;
> +
> +	if (!led_cdev->hw_control_trigger)
> +		return;
> +
> +	/* default_trigger is handled by led_trigger_set_default(). */

Sentences start with uppercase chars.

> +	if (led_cdev->default_trigger &&
> +	    !strcmp(led_cdev->default_trigger, led_cdev->hw_control_trigger))
> +		return;

Do you need to check default_trigger?

strcmp() should be able to handle empty strings.

> +
> +	down_read(&triggers_list_lock);
> +	list_for_each_entry(trig, &trigger_list, next_trig) {
> +		found = led_match_hw_control_trigger(led_cdev, trig);
> +		if (found)
> +			break;
> +	}
> +	up_read(&triggers_list_lock);
> +
> +	if (!found)
> +		request_module_nowait("ledtrig:%s", led_cdev->hw_control_trigger);
> +}
> +EXPORT_SYMBOL_GPL(led_load_hw_control_trigger);
> +
>  /* LED Trigger Interface */
>  
>  int led_trigger_register(struct led_trigger *trig)
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index bee46651e068f..e85afd4d04fd0 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -21,6 +21,7 @@ void led_init_core(struct led_classdev *led_cdev);
>  void led_stop_software_blink(struct led_classdev *led_cdev);
>  void led_set_brightness_nopm(struct led_classdev *led_cdev, unsigned int value);
>  void led_set_brightness_nosleep(struct led_classdev *led_cdev, unsigned int value);
> +void led_load_hw_control_trigger(struct led_classdev *led_cdev);
>  ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
>  			const struct bin_attribute *attr, char *buf,
>  			loff_t pos, size_t count);
> -- 
> 2.51.0
> 

-- 
Lee Jones [李琼斯]

Re: [RFC PATCH 0/9] leds: Add support for hw initiated hw control trigger transition

[Lee Jones]

I'd like for the netdev folks to take a look at this please.

> Some laptops can tune their keyboard backlight according to ambient
> light sensors (auto mode). This capability is essentially a hw control
> trigger. Meanwhile, such laptops also offer a shrotcut for cycling
> through brightness levels and auto mode. For example, on ThinkBook,
> pressing Fn+Space cycles keyboard backlight levels in the following
> sequence:
> 
>   1 => 2 => 0 => auto => 1 ...
> 
> Recent ThinkPad models should have similar sequence too.
> 
> However, there are some issues preventing us from using hw control
> trigger:
> 
> 1. We want a mechanism to tell userspace which trigger is the hw control
>    trigger, so that userspace can determine if auto mode is on/off or
>    turing it on/off programmatically without obtaining the hw control
>    trigger's name via other channels
> 2. Turing on/off auto mode via the shortcut cannot activate/deactivate
>    the hw control trigger, making the software state out-of-sync
> 3. Even with #1 resolved, deactivating the hw control trigger after
>    receiving the event indicating "auto => 1" has a side effect of
>    emitting LED_OFF, breaking the shortcut cycle
> 
> This RFC series tries to demonstrate a path on solving these issues:
> 
> - Introduce an attribute called trigger_may_offload, so that userspace
>    can determine:
>    - if the LED device supports hw control (supported => visible)
>    - which trigger is the hw control trigger
>    - if the hw control trigger is selected
>    - if the hw control trigger is in hw control (i.e., offloaded)
>      - A callback offloaded() is added so that LED triggers can report
>        their hw control state
> - Add led_trigger_notify_hw_control_changed() interface, so that LED
>   drivers can notify the LED core about hardware initiated hw control
>   state transitions. The LED core will then determine if the transition
>   is allowed and turning on/off the hw control trigger accordingly
> - Tune the logic of trigger deactivation so that it won't emit LED_OFF
>   when the deactivation is triggered by hardware
> 
> The last two patches are included into the RFC series to demonstrate how
> to utilize these interfaces to add support for auto keyboard backlight
> to ThinkBook. They will be submitted separately once the dust settles.
> 
> Currently no Kconfig entry is provided to disable either interface. If
> needed, I will add one later.
> 
> [ Summary of other approaches ]
> 
> < custom attribute >
> 
> Pros:
> - simplicity, KISS
> - no need to touch the LED core
> - extensible as long as it has a sensor-neutral name
>   - a sensor-related name could potentially lead to a mess if a future
>     device implements auto mode based on multiple different sensors
> 
> Cons:
> - must have zero influence on brightness_set[_blocking] callbacks
>   in order not to break triggers
>   - potential interference with triggers and the brightness attribute
> - weird semantic (an attribute other than "brightness" and "trigger"
>   changes the brightness)
> 
> < hw control trigger (this series) >
> 
> Pros:
> - mutually exclusive with other triggers (hence less chaos)
> - semantic correctness
> - acts as an aggregate switch to turn on/off auto mode even a future
>   device implements auto mode based on multiple different sensors
>   - extensibility (through trigger attributes)
> 
> Cons:
> - complexity
> 
> [ Previous discussion threads ]
> 
> https://lore.kernel.org/r/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@app.fastmail.com
> 
> https://lore.kernel.org/r/1dbfcf656cdb4af0299f90d7426d2ec7e2b8ac9e.camel@rong.moe
> 
> Thanks,
> Rong
> 
> Rong Zhang (9):
>   leds: Load trigger modules on-demand if used as hw control trigger
>   leds: Add callback offloaded() to query the state of hw control
>     trigger
>   leds: cros_ec: Implement offloaded() callback for trigger
>   leds: turris-omnia: Implement offloaded() callback for trigger
>   leds: trigger: netdev: Implement offloaded() callback
>   leds: Add trigger_may_offload attribute
>   leds: trigger: Add led_trigger_notify_hw_control_changed() interface
>   platform/x86: ideapad-laptop: Decouple HW & cdev brightness for kbd
>     backlight
>   platform/x86: ideapad-laptop: Fully support auto kbd backlight
> 
>  .../obsolete/sysfs-class-led-trigger-netdev   |  15 ++
>  Documentation/ABI/testing/sysfs-class-led     |  22 ++
>  .../testing/sysfs-class-led-trigger-netdev    |  13 --
>  Documentation/leds/leds-class.rst             |  72 ++++++-
>  drivers/leds/led-class.c                      |  23 +++
>  drivers/leds/led-triggers.c                   | 176 +++++++++++++++-
>  drivers/leds/leds-cros_ec.c                   |   6 +
>  drivers/leds/leds-turris-omnia.c              |   7 +
>  drivers/leds/leds.h                           |   3 +
>  drivers/leds/trigger/ledtrig-netdev.c         |  10 +
>  drivers/platform/x86/lenovo/Kconfig           |   1 +
>  drivers/platform/x86/lenovo/ideapad-laptop.c  | 194 ++++++++++++++----
>  include/linux/leds.h                          |   6 +
>  13 files changed, 492 insertions(+), 56 deletions(-)
>  create mode 100644 Documentation/ABI/obsolete/sysfs-class-led-trigger-netdev
> 
> 
> base-commit: a75cb869a8ccc88b0bc7a44e1597d9c7995c56e5
> -- 
> 2.51.0
> 

-- 
Lee Jones [李琼斯]

Re: [RFC PATCH 0/9] leds: Add support for hw initiated hw control trigger transition

[Andrew Lunn]

On Tue, Mar 10, 2026 at 12:10:24PM +0000, Lee Jones wrote:
> I'd like for the netdev folks to take a look at this please.

Before i get the rational of these patches....

Have they been tested with CONFIG_PROVE_LOCKING enabled? My experience
with networking is that it is very easy to get tied up in AB-BA
deadlocks. You need the LED to be as dumb as possible, it is always
logically 'below' the trigger. Having the LED calling up into the
trigger generally gets you into trouble.

	Andrew

Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Andrew Lunn]

On Sat, Feb 28, 2026 at 03:05:58AM +0800, Rong Zhang wrote:
> In the following patches, we are about to support hardware initiated
> trigger transitions to/from the device's hw control trigger. In case
> the LED hardware switches itself to hw control mode, hw control trigger
> must be loaded before so that the transition can be processed.

This sounds backwards around.

A Linux LED starts out life as a dumb LED. You can set its brightness
using /sys/class/leds/<foo>/brightness.

Userspace policy can then give additional meaning to the LED. It could
blink a heartbeat, disk activity, show rf-kill status, activity on a
serial port, what link speed eth42 has etc. The general design in
Linux is that any LED can be used for any of these functions. You
decide what an LED should indicate by selecting the trigger for it. To
be able to select the trigger, the trigger needs to be already loaded.

Only once you have the trigger load, and the LED using the trigger,
then you can think about can the trigger be offloaded to hardware.

In linux, policy is in user space. If policy says led X is to be used
with trigger Y, user space can get the needed trigger module loaded.

     Andrew

Re: [RFC PATCH 0/9] leds: Add support for hw initiated hw control trigger transition

[Andrew Lunn]

On Sat, Feb 28, 2026 at 03:05:57AM +0800, Rong Zhang wrote:
> Hi all,
> 
> Some laptops can tune their keyboard backlight according to ambient
> light sensors (auto mode). This capability is essentially a hw control
> trigger. Meanwhile, such laptops also offer a shrotcut for cycling
> through brightness levels and auto mode. For example, on ThinkBook,
> pressing Fn+Space cycles keyboard backlight levels in the following
> sequence:
> 
>   1 => 2 => 0 => auto => 1 ...
> 
> Recent ThinkPad models should have similar sequence too.

With networking, we consider the hardware as an accelerator for what
the Linux network stack can already do in software. We let the user
configure the network stack however they want it, and then we ask the
hardware, can you accelerate this, so we don't need to do it in
software? It might say -EOPNOTSUPP, so it is left in software. It
might say 0, and we never see the packets, because the hardware is
doing the work. The user is not really aware acceleration is
happening, because they just configure the network stack how they
want, independent of acceleration or not.

For PHY and MAC LEDs it is exactly the same. Generally, the LEDs in
RJ45 connectors, or on the front panel can be turned on/off. The
netdev LED trigger knows if the link is 10M, 100M, 1G etc. It knows if
packets are being transmitted or received. It can make the LEDs show
the link speed, or blink for packet activity, in software, using
simple set_brightness calls. It will also ask the LED, can you
accelerate this? Can you get the state directly from the PHY/MAC? The
LED in my keyboard shift lock will say -EOPNOTSUPP, and the netdev
trigger will keep blinking it in software. The LED driver by the PHY
might say 0, and blink the LED itself. Or it might say -EOPNOTSUPP,
because the vendor decided to only support RX+TX, not only RX.

My preference is this model of accelerating what Linux can already do
should be used everywhere.

You know how many levels the LED supports, so the trigger can easily
implement the steps, 0, 1, 2, 0, 1, 2, based on receiving some event
from somewhere.  You can also accelerate this, you can ask the LED,
can you directly receive the event? -EOPNOTSUPP, keep controlling it
from software. 0, stop driving it from software, the hardware will
accelerate it.

If the system has access to a light sensor, the trigger can also
decide on 0, 1, or 2, based on polling the light sensor every
second. It can then ask the LED, do you have direct access to the
light sensor, can you accelerate this?

The experience from networking is, once you get into the mindset of
the hardware is there to accelerate what Linux can already do, a lot
of problems just disappear. It might you first need to implement the
software version, but once you have that, acceleration is easy.

Does Linux already have a software version of what you want? Can you
accelerate it?

	Andrew

Re: [RFC PATCH 0/9] leds: Add support for hw initiated hw control trigger transition

[Andrew Lunn]

> I'm not a huge fan of the term offloaded

In networking, 'offload' is key to the whole concept. We take what the
network stack is doing in software and try to offload it to the
hardware.

See my reply to patch 0/42 for more details.

	Andrew

Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Rong Zhang]

Hi Lee,

On Tue, 2026-03-10 at 12:01 +0000, Lee Jones wrote:
> On Sat, 28 Feb 2026, Rong Zhang wrote:
> 
> > In the following patches, we are about to support hardware initiated
> 
> Let's not talk about other commits.  Only tell us what's happening here.
> 
> > trigger transitions to/from the device's hw control trigger. In case
> > the LED hardware switches itself to hw control mode, hw control trigger
> > must be loaded before so that the transition can be processed.
> > 
> > Load the trigger module specified by hw_control_trigger, so that
> > hardware initiated trigger transitions can be processed when the hw
> 
> "hardware"
> 
> > control trigger is compiled as a module.
> > 
> > Signed-off-by: Rong Zhang <i@rong.moe>
> > ---
> >  drivers/leds/led-class.c    |  1 +
> >  drivers/leds/led-triggers.c | 33 +++++++++++++++++++++++++++++++++
> >  drivers/leds/leds.h         |  1 +
> >  3 files changed, 35 insertions(+)
> > 
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index d34a194535604..0fa45f22246e3 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -576,6 +576,7 @@ int led_classdev_register_ext(struct device *parent,
> >  
> >  #ifdef CONFIG_LEDS_TRIGGERS
> >  	led_trigger_set_default(led_cdev);
> > +	led_load_hw_control_trigger(led_cdev);
> 
> led_trigger_load_hw_control
> 
> >  #endif
> >  
> >  	mutex_unlock(&led_cdev->led_access);
> > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > index b1223218bda11..3066bc91a5f94 100644
> > --- a/drivers/leds/led-triggers.c
> > +++ b/drivers/leds/led-triggers.c
> > @@ -313,6 +313,39 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
> >  }
> >  EXPORT_SYMBOL_GPL(led_trigger_set_default);
> >  
> > +static inline bool led_match_hw_control_trigger(struct led_classdev *led_cdev,
> > +						struct led_trigger *trig)
> > +{
> > +	return (!strcmp(led_cdev->hw_control_trigger, trig->name) &&
> > +		trigger_relevant(led_cdev, trig));
> 
> 
> This is ugly.  Break it out and provide some commentary.
> 
> > +}
> > +
> > +void led_load_hw_control_trigger(struct led_classdev *led_cdev)
> > +{
> > +	struct led_trigger *trig;
> > +	bool found = false;
> > +
> > +	if (!led_cdev->hw_control_trigger)
> > +		return;
> > +
> > +	/* default_trigger is handled by led_trigger_set_default(). */
> 
> Sentences start with uppercase chars.
> 
> > +	if (led_cdev->default_trigger &&
> > +	    !strcmp(led_cdev->default_trigger, led_cdev->hw_control_trigger))
> > +		return;
> 
> Do you need to check default_trigger?
> 
> strcmp() should be able to handle empty strings.

It checks the NULLity of led_cdev->default_trigger instead of its
emptiness. While passing an empty string to strcmp is fine, passing a
NULL pointer to strcmp() leads to a NULL dereference.

Thanks for your review. ACK to other comments :)

Thanks,
Rong

> 
> > +
> > +	down_read(&triggers_list_lock);
> > +	list_for_each_entry(trig, &trigger_list, next_trig) {
> > +		found = led_match_hw_control_trigger(led_cdev, trig);
> > +		if (found)
> > +			break;
> > +	}
> > +	up_read(&triggers_list_lock);
> > +
> > +	if (!found)
> > +		request_module_nowait("ledtrig:%s", led_cdev->hw_control_trigger);
> > +}
> > +EXPORT_SYMBOL_GPL(led_load_hw_control_trigger);
> > +
> >  /* LED Trigger Interface */
> >  
> >  int led_trigger_register(struct led_trigger *trig)
> > diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> > index bee46651e068f..e85afd4d04fd0 100644
> > --- a/drivers/leds/leds.h
> > +++ b/drivers/leds/leds.h
> > @@ -21,6 +21,7 @@ void led_init_core(struct led_classdev *led_cdev);
> >  void led_stop_software_blink(struct led_classdev *led_cdev);
> >  void led_set_brightness_nopm(struct led_classdev *led_cdev, unsigned int value);
> >  void led_set_brightness_nosleep(struct led_classdev *led_cdev, unsigned int value);
> > +void led_load_hw_control_trigger(struct led_classdev *led_cdev);
> >  ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
> >  			const struct bin_attribute *attr, char *buf,
> >  			loff_t pos, size_t count);
> > -- 
> > 2.51.0
> > 

Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Rong Zhang]

Hi Andrew,

Thanks for your review.

On Tue, 2026-03-10 at 20:22 +0100, Andrew Lunn wrote:
> On Sat, Feb 28, 2026 at 03:05:58AM +0800, Rong Zhang wrote:
> > In the following patches, we are about to support hardware initiated
> > trigger transitions to/from the device's hw control trigger. In case
> > the LED hardware switches itself to hw control mode, hw control trigger
> > must be loaded before so that the transition can be processed.
> 
> This sounds backwards around.
> 
> A Linux LED starts out life as a dumb LED. You can set its brightness
> using /sys/class/leds/<foo>/brightness.
> 
> Userspace policy can then give additional meaning to the LED. It could
> blink a heartbeat, disk activity, show rf-kill status, activity on a
> serial port, what link speed eth42 has etc. The general design in
> Linux is that any LED can be used for any of these functions. You
> decide what an LED should indicate by selecting the trigger for it. To
> be able to select the trigger, the trigger needs to be already loaded.
> 
> Only once you have the trigger load, and the LED using the trigger,
> then you can think about can the trigger be offloaded to hardware.

While I agree that the current policy makes its own sense and works
well on most devices, it leads to out-of-sync software states on recent
laptops.

When the LED's hardware autonomously put itself into hw control mode,
it is offloaded to the hardware per se -- we *can't* prevent this from
happening.

The series is about how to update software state to reflect the
hardware state change. Blindly keeping the software state despite the
hardware state is hardly meaningful and makes software out-of-sync.


It's worth mentioning that the series doesn't really cause any
behavioral change compared to the current policy when we consider the
LED device's hardware state. Let's consider what will happen after LED
X has autonomously activated hw control:

[ Software state #a: trigger=none ]

Current: LED X stays in hw control mode. Trigger unchanged
(trigger=none, hence software is out-of-sync with hardware).

This series: LED X stays in hw control mode. Trigger updated to reflect
the hardware state (trigger=hw_control_trigger, offloaded=1).

[ Software state #b: trigger=other ]
[ Software state #c: trigger=hw_control_trigger, offloaded=0 ]

Current & this series: LED X stays in hw control mode until the next
trigger event. Software state unchanged and up-to-date.


And what will happen after LED X has autonomously deactivated hw
control:

[ Software state #d: trigger=hw_control_trigger, offloaded=1 ]

Current: LED X stays in software control mode. Trigger unchanged
(trigger=hw_control_trigger, offloaded=1, hence software is out-of-sync
with hardware).

This series: LED X stays in software control mode. Trigger updated to
reflect the hardware state (trigger=none).

[ Software state #e: trigger=other ]
[ Software state #f: trigger=hw_control_trigger, offloaded=0 ]

Current & this series: LED X stays in software control mode. Software
state unchanged and up-to-date.


As shown above, this series doesn't change the LED's hardware state and
it just updates the software state to notify user about that. If you'd
like to enforce software state's priority, we would have to explicitly
undo the hardware state change immediately after the LED's hardware has
autonomously activated/deactivated hw control mode.

Moreover, if an LED device doesn't autonomously change its hw control
state at all, this series should have little software/hardware impact
on them. IIUC, that's the case of all LEDs with
hw_control_trigger=netdev.

> 
> In linux, policy is in user space. If policy says led X is to be used
> with trigger Y, user space can get the needed trigger module loaded.

Just to provide some background of this patch: I decided to include it
in the series when I thought about these three scenarios:

[ Scenario #1 ]

LED X: no default trigger; hw control trigger is Y; capable for
hardware initiated trigger transition

LED W: default trigger is Y

In this case, trigger Y has been be loaded automatically due to LED W.
Hardware initiated trigger transitions from LED X can be processed
properly.

[ Scenario #2 ]

LED X: same as above

CONFIG_LEDS_TRIGGER_Y=y

In this case, trigger Y is builtin. Hardware initiated trigger
transitions from LED X can be processed properly.

Most distros have their own set of builtin triggers, so that's not
necessarily user's choice.

[ Scenario #3 ]

LED X: same as above

Other LEDs: default trigger is not Y

In this case, missing trigger Y leads to hardware initiated trigger
transitions from LED X being ignored.


Hence, not loading hw control trigger automatically leads to behavioral
divergences caused by other LED devices or distro configurations. LED X
is a victim of such divergences, but it was never intended to cause
them.

The main purpose of introducing this commit is to prevent such
divergences. We should only allow behavioral divergences when
CONFIG_LEDS_TRIGGER_Y=n or when trigger Y is blacklisted.


That being said, I am OK to drop this patch from the series. ideapad-
laptop (see [RFC PATCH 9/9]) registers a private trigger, so it doesn't
depend on this patch. AFAIK this also applies to thinkpad_acpi as long
as it implements auto brightness in a similar way.

Thanks,
Rong

> 
>      Andrew

Re: [RFC PATCH 0/9] leds: Add support for hw initiated hw control trigger transition

[Rong Zhang]

Hi Andrew,

On Tue, 2026-03-10 at 20:04 +0100, Andrew Lunn wrote:
> On Tue, Mar 10, 2026 at 12:10:24PM +0000, Lee Jones wrote:
> > I'd like for the netdev folks to take a look at this please.
> 
> Before i get the rational of these patches....
> 
> Have they been tested with CONFIG_PROVE_LOCKING enabled? My experience
> with networking is that it is very easy to get tied up in AB-BA
> deadlocks. You need the LED to be as dumb as possible, it is always
> logically 'below' the trigger. Having the LED calling up into the
> trigger generally gets you into trouble.

I will test it. Thanks for your reminder.

Thanks,
Rong

> 
> 	Andrew

Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Andrew Lunn]

> While I agree that the current policy makes its own sense and works
> well on most devices, it leads to out-of-sync software states on recent
> laptops.
> 
> When the LED's hardware autonomously put itself into hw control mode,
> it is offloaded to the hardware per se -- we *can't* prevent this from
> happening.

If you cannot control the hardware, why are you trying to control the
hardware?

> The series is about how to update software state to reflect the
> hardware state change. Blindly keeping the software state despite the
> hardware state is hardly meaningful and makes software out-of-sync.

Since you cannot control the hardware, just don't register the
LED. That gives a truer picture. Something else than Linux is
controlling it.

Do you get a notification when that something else takes control? ACPI
event or something? If you do, can just re-impose the software state
back on the hardware.

> As shown above, this series doesn't change the LED's hardware state and
> it just updates the software state to notify user about that. If you'd
> like to enforce software state's priority, we would have to explicitly
> undo the hardware state change immediately after the LED's hardware has
> autonomously activated/deactivated hw control mode.

Yes, if you decide Linux is driving the hardware, the Linux state
should always be imposed back on the hardware. Just consider it flaky
hardware which needs hitting over the head every so often to make
work.

But maybe we should take a step back here. What are your real use
cases here? Why do you want Linux to control this hardware, when
something else already is controlling it? Is /sys/class/led even the
right API? That will depend on what your use cases are.

	Andrew

Re: [RFC PATCH 0/9] leds: Add support for hw initiated hw control trigger transition

[Rong Zhang]

Hi Andrew,

Thanks for your review.

On Tue, 2026-03-10 at 20:57 +0100, Andrew Lunn wrote:
> On Sat, Feb 28, 2026 at 03:05:57AM +0800, Rong Zhang wrote:
> > Hi all,
> > 
> > Some laptops can tune their keyboard backlight according to ambient
> > light sensors (auto mode). This capability is essentially a hw control
> > trigger. Meanwhile, such laptops also offer a shrotcut for cycling
> > through brightness levels and auto mode. For example, on ThinkBook,
> > pressing Fn+Space cycles keyboard backlight levels in the following
> > sequence:
> > 
> >   1 => 2 => 0 => auto => 1 ...
> > 
> > Recent ThinkPad models should have similar sequence too.
> 
> With networking, we consider the hardware as an accelerator for what
> the Linux network stack can already do in software. We let the user
> configure the network stack however they want it, and then we ask the
> hardware, can you accelerate this, so we don't need to do it in
> software? It might say -EOPNOTSUPP, so it is left in software. It
> might say 0, and we never see the packets, because the hardware is
> doing the work. The user is not really aware acceleration is
> happening, because they just configure the network stack how they
> want, independent of acceleration or not.
> 
> For PHY and MAC LEDs it is exactly the same. Generally, the LEDs in
> RJ45 connectors, or on the front panel can be turned on/off. The
> netdev LED trigger knows if the link is 10M, 100M, 1G etc. It knows if
> packets are being transmitted or received. It can make the LEDs show
> the link speed, or blink for packet activity, in software, using
> simple set_brightness calls. It will also ask the LED, can you
> accelerate this? Can you get the state directly from the PHY/MAC? The
> LED in my keyboard shift lock will say -EOPNOTSUPP, and the netdev
> trigger will keep blinking it in software. The LED driver by the PHY
> might say 0, and blink the LED itself. Or it might say -EOPNOTSUPP,
> because the vendor decided to only support RX+TX, not only RX.
> 
> My preference is this model of accelerating what Linux can already do
> should be used everywhere.

Well, there's a thing Linux can't do -- it can't prevent the keyboard
to autonomously activate/deactivate auto brightness after the shortcut
is pressed. The shortcut is processed by EC with zero OS involvement.
That's why brightness_hw_changed exists -- we can't prevent the
hardware to change the brightness on its own, so the best thing we can
do is to notify userspace about the event.

Now the same situation happens on a special brightness control mode
(auto brightness), which is essentially a hw control trigger. Hence I
figured out this series. 

> 
> You know how many levels the LED supports, so the trigger can easily
> implement the steps, 0, 1, 2, 0, 1, 2, based on receiving some event
> from somewhere.  You can also accelerate this, you can ask the LED,
> can you directly receive the event? -EOPNOTSUPP, keep controlling it
> from software. 0, stop driving it from software, the hardware will
> accelerate it.
> 
> If the system has access to a light sensor, 

At least on my device (ThinkBook) I can access the als sensor through
iio bus (driver: hid-sensor-als). But I am unsure if ThinkPad also
exposes it to the OS.

> the trigger can also
> decide on 0, 1, or 2, based on polling the light sensor every
> second. 

I hardly see the advantages of a kernel mode trigger with 1s polling
interval compared to a trigger implemented in userspace, especially
when considering the inconvenience of configuring brightness curves.

AFAIK, KDE already implement a userspace trigger for screen backlight.
So that's also another issue: even if we implement a als-based software
trigger, it can't be used by video backlight and vice versa.

> It can then ask the LED, do you have direct access to the
> light sensor, can you accelerate this?

No, ThinkBook/ThinkPad doesn't report the curve or accept custom curves
so we can't really determine whether a specific curve can be offloaded.

> 
> The experience from networking is, once you get into the mindset of
> the hardware is there to accelerate what Linux can already do, a lot
> of problems just disappear. It might you first need to implement the
> software version, but once you have that, acceleration is easy.
> 
> Does Linux already have a software version of what you want? Can you
> accelerate it?

Such a trigger doesn't exist yet.

If some day in the future we have a als-based software trigger, this
series also provides a migration path thanks to the trigger_may_offload
attribute. Userspace are supposed to query it for the hw control
trigger's name.

Thanks,
Rong

> 
> 	Andrew

Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Rong Zhang]

Hi Andrew,

On Wed, 2026-03-11 at 19:44 +0100, Andrew Lunn wrote:
> > While I agree that the current policy makes its own sense and works
> > well on most devices, it leads to out-of-sync software states on recent
> > laptops.
> > 
> > When the LED's hardware autonomously put itself into hw control mode,
> > it is offloaded to the hardware per se -- we *can't* prevent this from
> > happening.
> 
> If you cannot control the hardware, why are you trying to control the
> hardware?

We can control the hardware. We can set brightness, enable or disable
auto mode freely.

We just can't prevent the EC from responding to the Fn+Space shortcut.
So it's essentially user's choice to switch to the hw control trigger
and make it offloaded to hardware (sorry if my cover letter and replies
didn't express this well).

> 
> > The series is about how to update software state to reflect the
> > hardware state change. Blindly keeping the software state despite the
> > hardware state is hardly meaningful and makes software out-of-sync.
> 
> Since you cannot control the hardware, just don't register the
> LED. That gives a truer picture. Something else than Linux is
> controlling it.

As my previous reply said, it's common that an LED driver can't prevent
hardware from changing its state autonomously. Prior to the
introduction of auto brightness mode, brightness_hw_changed is enough
to handle this. The issue only emerges when recent models start to
provide an auto brightness mode based on the ALS sensor.

> 
> Do you get a notification when that something else takes control? ACPI
> event or something?

Yes, and the event is used in the series to change software state.

> If you do, can just re-impose the software state
> back on the hardware.
> 
> > As shown above, this series doesn't change the LED's hardware state and
> > it just updates the software state to notify user about that. If you'd
> > like to enforce software state's priority, we would have to explicitly
> > undo the hardware state change immediately after the LED's hardware has
> > autonomously activated/deactivated hw control mode.
> 
> Yes, if you decide Linux is driving the hardware, the Linux state
> should always be imposed back on the hardware. Just consider it flaky
> hardware which needs hitting over the head every so often to make
> work.

I don't think we should re-impose the software state back on the
hardware. We never do this for brightness and decided to introduce
brightness_hw_changed. That's should also applies to hw control
trigger.

Moreover, re-imposing the software state breaks the shortcut by
changing the brightness cycle from

  1 => 2 => 0 => auto => 1 ...

to

  1 => 2 => 0 => (auto => 0) => (auto => 0) => ...

> 
> But maybe we should take a step back here. What are your real use
> cases here? Why do you want Linux to control this hardware, when
> something else already is controlling it? Is /sys/class/led even the
> right API? That will depend on what your use cases are.

FYI, desktop environments (e.g., GNOME, KDE) can control the backlight
brightness of keyboards through sliders and heavily depend on
brightness_hw_changed to update the sliders and display OSD once the
shortcut is pressed. This matches the experience on Windows, where
utilities from manufacturers provide brightness sliders and display
OSD.

The shortcut itself doesn't send any keyboard events to the OS. Ths OS
only sees an event notifying that keyboard backlight has changed. We
want the event of turning on/off auto brightness reaches userspace like
what brightness_hw_changed does for brightness changes. Also we don't
want the auto brightness mode to have interference to other triggers or
the brightness attribute and vice versa, so making it a hw control
trigger is an approach to make them mutually exclusive.

Thanks,
Rong

> 
> 	Andrew

Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Andrew Lunn]

> We just can't prevent the EC from responding to the Fn+Space shortcut.
> So it's essentially user's choice to switch to the hw control trigger
> and make it offloaded to hardware (sorry if my cover letter and replies
> didn't express this well).

Do you have any control over the EC?

You have a two bosses dilemma. You need to eliminate one of the
bosses. Either the EC controls the LED, or Linux does. Having both
controlling it is just going to work out badly.

> As my previous reply said, it's common that an LED driver can't prevent
> hardware from changing its state autonomously. Prior to the
> introduction of auto brightness mode, brightness_hw_changed is enough
> to handle this. The issue only emerges when recent models start to
> provide an auto brightness mode based on the ALS sensor.

Do you have a software driven brightness mode based on an ALS? What
API do you use to control this? Can you use that API, and accelerate
it?

> FYI, desktop environments (e.g., GNOME, KDE) can control the backlight
> brightness of keyboards through sliders and heavily depend on
> brightness_hw_changed to update the sliders and display OSD once the
> shortcut is pressed.

Hold up. Terminology problem. I'm a networking guy, i know networking
terms. By slider, do you mean a software scroll bar sort of thing? I'm
an XFCE users. I can control the display backlight with a slider on
the battery charge applet. And i can use Fn F4/F5. I've not seen a
software scroll bar for the keyboard backlight, but i think
<CTRL><SPC> allows me to change the keyboard backlight.

So we have a slider, which is purely software, Linux. And we have key
presses, which you are calling shortcut, which the EC acts on, and
might tell Linux, maybe, but not about the key press, but the action
the EC took because of the key press.

You have some API to the EC to ask it nicely to act on the software
slide, but it is the EC which really controls the LED, not Linux.

To me a Linux LED is a poor fit for what you want, and i think a
trigger is even worse. The problems you have are because the
LED+trigger model, plus using the hardware for acceleration, does not
fit with the EC actually controlling the hardware.

I would suggest you look at the API the EC exports and find a better
model for it.

	Andrew

Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Rong Zhang]

Hi Andrew,

On Wed, 2026-03-11 at 22:29 +0100, Andrew Lunn wrote:
> > We just can't prevent the EC from responding to the Fn+Space shortcut.
> > So it's essentially user's choice to switch to the hw control trigger
> > and make it offloaded to hardware (sorry if my cover letter and replies
> > didn't express this well).
> 
> Do you have any control over the EC?
> 
> You have a two bosses dilemma. You need to eliminate one of the
> bosses. Either the EC controls the LED, or Linux does. Having both
> controlling it is just going to work out badly.

I agree that the manufacturers designed the interface poorly :-/

I am not affiliated with any laptop manufacturers so I am just speaking
for myself:

IMO the real boss is the user. Both the shortcut (Fn+Space) and the
ACPI interface are just "message channels" for the EC to know about the
user's choice.

Being able to press such a shortcut always implies that the user is
physically in front of the device. In this case it no longer about
whether Linux or the EC controls the LED, but both should respect
user's choice. That was why brightness_hw_changed was introduced to
respect user's choice and pass it to the userspace. So far there has
been ~10 drivers utilizing the brightness_hw_changed interface.

> 
> > As my previous reply said, it's common that an LED driver can't prevent
> > hardware from changing its state autonomously. Prior to the
> > introduction of auto brightness mode, brightness_hw_changed is enough
> > to handle this. The issue only emerges when recent models start to
> > provide an auto brightness mode based on the ALS sensor.
> 
> Do you have a software driven brightness mode based on an ALS? What
> API do you use to control this? Can you use that API, and accelerate
> it?

All devices I've seen implement an EC driven auto brightness mode based
on an ALS.

@Mark, do you know any device implementing a software driven auto
brightness mode?

> 
> > FYI, desktop environments (e.g., GNOME, KDE) can control the backlight
> > brightness of keyboards through sliders and heavily depend on
> > brightness_hw_changed to update the sliders and display OSD once the
> > shortcut is pressed.
> 
> Hold up. Terminology problem. I'm a networking guy, i know networking
> terms. By slider, do you mean a software scroll bar sort of thing? 

Yes. See
https://blogs.kde.org/2024/09/04/brightness-controls-for-all-your-displays/

(it was about display backlight but it also showed the keyboard one in
the same image)

> I'm
> an XFCE users. I can control the display backlight with a slider on
> the battery charge applet. And i can use Fn F4/F5. I've not seen a
> software scroll bar for the keyboard backlight, but i think
> <CTRL><SPC> allows me to change the keyboard backlight.
> 
> So we have a slider, which is purely software, Linux. And we have key
> presses, which you are calling shortcut, which the EC acts on, and
> might tell Linux, maybe, but not about the key press, but the action
> the EC took because of the key press.

"might tell", "maybe"

It always tells the OS that the state of keyboard backlight has
changed.

> 
> You have some API to the EC to ask it nicely to act on the software
> slide, but it is the EC which really controls the LED, not Linux.
> 
> To me a Linux LED is a poor fit for what you want, and i think a
> trigger is even worse. The problems you have are because the
> LED+trigger model, plus using the hardware for acceleration, does not
> fit with the EC actually controlling the hardware.
> 
> I would suggest you look at the API the EC exports and find a better
> model for it.

An LED classdev may be unable to perfectly fit this, but nothing is
perfect and so far it's the best thing we have. It's a fortunate to
have the LED subsystem. Windows, without a similar interface, ends up
being filled with disgusting software pre-installed by the
manufacturer.

IMO the presence of brightness_hw_changed proves that an LED classdev,
as long as appropriate interfaces are provided, can work well with such
hardware. And I don't think there is too much difference between EC
setting a static brightness value due to a shortcut being pressed and
EC turning on/off the auto brightness mode due to the same shortcut --
if we can handle the former well, we can also implement a similar
mechanism for the latter.


Do you have any recommendations for a "better model"?

Did you mean do not register LED classdevs at all? This isn't really
viable and will break userspace. Some drivers has been using LED
classdevs for keyboard backlight for over a decade. And many
`*::kbd_backlight' LEDs rely on brightness_hw_changed, so it's very
common that we can't take 100% control over EC. LED classdevs and EC-
controlled keyboard backlight have lived in harmony for a long time.

If we still register the keybaord backlight as an LED classdev but use
a custom attribute to report/set the auto brightness mode. IMO this is
even uglier than LED+trigger, as writing to such a non-brightness
attribute will interfere with the brightness attribute and the active
trigger and vice versa. Even if we rule out the EC's action, such
interference still exists as long as users use the attribute.


As for the software-vs-hardware priority issue, how about adding an
attribute "hw_change_policy" so that users can select if the software
state should be always reimposed to hardware?

- If the policy is reimpose, calling
led_trigger_notify_hw_control_changed() or
led_classdev_notify_brightness_hw_changed() results in the software
state to be reimposed to the hardware and no event should reach
userspace.

- Otherwise calling led_trigger_notify_hw_control_changed() or
led_classdev_notify_brightness_hw_changed() updates software state and
notifies userspace.

Thanks,
Rong

> 
> 	Andrew

Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Mark Pearson]

On Thu, Mar 12, 2026, at 2:01 PM, Rong Zhang wrote:
> Hi Andrew,
>
> On Wed, 2026-03-11 at 22:29 +0100, Andrew Lunn wrote:
>> > We just can't prevent the EC from responding to the Fn+Space shortcut.
>> > So it's essentially user's choice to switch to the hw control trigger
>> > and make it offloaded to hardware (sorry if my cover letter and replies
>> > didn't express this well).
>> 
>> Do you have any control over the EC?
>> 
>> You have a two bosses dilemma. You need to eliminate one of the
>> bosses. Either the EC controls the LED, or Linux does. Having both
>> controlling it is just going to work out badly.
>
> I agree that the manufacturers designed the interface poorly :-/
>
> I am not affiliated with any laptop manufacturers so I am just speaking
> for myself:
>
> IMO the real boss is the user. Both the shortcut (Fn+Space) and the
> ACPI interface are just "message channels" for the EC to know about the
> user's choice.
>
> Being able to press such a shortcut always implies that the user is
> physically in front of the device. In this case it no longer about
> whether Linux or the EC controls the LED, but both should respect
> user's choice. That was why brightness_hw_changed was introduced to
> respect user's choice and pass it to the userspace. So far there has
> been ~10 drivers utilizing the brightness_hw_changed interface.
>
I am affiliated with a laptop manufacturer :) Happy to take suggestions on what should be improved or is missing (can't promise anything but happy to consider it and take it for review).

We can set the brightness, get the status, and the FW sends events when it changes - all supported on Linux (for Lenovo devices). This looks like a pretty decent API to me. What is it missing?

I don't understand the two bosses issue I'm afraid. The user ('Linux' in your description?) tells the EC what it wants the LED to be, and the EC does it. The EC is not a boss.

>> 
>> > As my previous reply said, it's common that an LED driver can't prevent
>> > hardware from changing its state autonomously. Prior to the
>> > introduction of auto brightness mode, brightness_hw_changed is enough
>> > to handle this. The issue only emerges when recent models start to
>> > provide an auto brightness mode based on the ALS sensor.
>> 
>> Do you have a software driven brightness mode based on an ALS? What
>> API do you use to control this? Can you use that API, and accelerate
>> it?
>
> All devices I've seen implement an EC driven auto brightness mode based
> on an ALS.
>
> @Mark, do you know any device implementing a software driven auto
> brightness mode?
>

I don't - to my knowledge in auto mode it's always driven by the HW/FW.

If there was a SW approach it would read the sensor and set the brightness to low/medium/high (and not to auto) so I'm struggling to understand the issue here. What am I missing?

>> 
>> > FYI, desktop environments (e.g., GNOME, KDE) can control the backlight
>> > brightness of keyboards through sliders and heavily depend on
>> > brightness_hw_changed to update the sliders and display OSD once the
>> > shortcut is pressed.
>> 
>> Hold up. Terminology problem. I'm a networking guy, i know networking
>> terms. By slider, do you mean a software scroll bar sort of thing? 
>
> Yes. See
> https://blogs.kde.org/2024/09/04/brightness-controls-for-all-your-displays/
>
> (it was about display backlight but it also showed the keyboard one in
> the same image)
>
>> I'm
>> an XFCE users. I can control the display backlight with a slider on
>> the battery charge applet. And i can use Fn F4/F5. I've not seen a
>> software scroll bar for the keyboard backlight, but i think
>> <CTRL><SPC> allows me to change the keyboard backlight.
>> 
>> So we have a slider, which is purely software, Linux. And we have key
>> presses, which you are calling shortcut, which the EC acts on, and
>> might tell Linux, maybe, but not about the key press, but the action
>> the EC took because of the key press.
>
> "might tell", "maybe"
>
> It always tells the OS that the state of keyboard backlight has
> changed.
>
>> 
>> You have some API to the EC to ask it nicely to act on the software
>> slide, but it is the EC which really controls the LED, not Linux.
>> 
>> To me a Linux LED is a poor fit for what you want, and i think a
>> trigger is even worse. The problems you have are because the
>> LED+trigger model, plus using the hardware for acceleration, does not
>> fit with the EC actually controlling the hardware.
>> 
>> I would suggest you look at the API the EC exports and find a better
>> model for it.
>
> An LED classdev may be unable to perfectly fit this, but nothing is
> perfect and so far it's the best thing we have. It's a fortunate to
> have the LED subsystem. Windows, without a similar interface, ends up
> being filled with disgusting software pre-installed by the
> manufacturer.
>

Afraid I don't understand what we are debating here.

Isn't the whole goal of this patch to make it so LED classdev is a better fit to address missing functionality? Why would switching to something else (I have no idea what) be better? Especially given the the keyboard backlight is currently a LED device, and changing that would potentially break things for users.

From my perspective if I could just tear this out and have a Lenovo only keyboard_backlight implementation under (for example) /sys/devices/thinkpad_acpi it would be so much easier. But I don't think it is the right thing to do. My experience is if we define a common approach then all vendors will use it going forward - which is better for the Linux experience overall.
Or we don't have fully implemented features for Linux users? That's kinda sucky.

I don't think the two bosses argument is valid (or at least I don't understand it). Are there any other critical implementation details that make this a poor choice and will bite us in the long run?

I personally find the implementation more complicated than I originally expected, but having looked at it and understood better what Rong was proposing I understand the benefits and I think it works. We're still checking it out on Thinkpad to confirm that, but this patch is a RFC so I think that's part of the process.

> IMO the presence of brightness_hw_changed proves that an LED classdev,
> as long as appropriate interfaces are provided, can work well with such
> hardware. And I don't think there is too much difference between EC
> setting a static brightness value due to a shortcut being pressed and
> EC turning on/off the auto brightness mode due to the same shortcut --
> if we can handle the former well, we can also implement a similar
> mechanism for the latter.
>
>
> Do you have any recommendations for a "better model"?
>
> Did you mean do not register LED classdevs at all? This isn't really
> viable and will break userspace. Some drivers has been using LED
> classdevs for keyboard backlight for over a decade. And many
> `*::kbd_backlight' LEDs rely on brightness_hw_changed, so it's very
> common that we can't take 100% control over EC. LED classdevs and EC-
> controlled keyboard backlight have lived in harmony for a long time.
>
> If we still register the keybaord backlight as an LED classdev but use
> a custom attribute to report/set the auto brightness mode. IMO this is
> even uglier than LED+trigger, as writing to such a non-brightness
> attribute will interfere with the brightness attribute and the active
> trigger and vice versa. Even if we rule out the EC's action, such
> interference still exists as long as users use the attribute.
>
>
> As for the software-vs-hardware priority issue, how about adding an
> attribute "hw_change_policy" so that users can select if the software
> state should be always reimposed to hardware?

Is this needed? When wouldn't this be the case?

If the SW sets a specific brightness that should become the setting. It would override any previous choices and tell the HW that is what is wanted now - don't change it (until the user says otherwise).
If we're in auto mode and the HW changes the brightness - it doesn't change the setting from auto mode, just the reported brightness level if queried.

>
> - If the policy is reimpose, calling
> led_trigger_notify_hw_control_changed() or
> led_classdev_notify_brightness_hw_changed() results in the software
> state to be reimposed to the hardware and no event should reach
> userspace.
>
> - Otherwise calling led_trigger_notify_hw_control_changed() or
> led_classdev_notify_brightness_hw_changed() updates software state and
> notifies userspace.
>
I feel like I'm missing something here - but it's been a long day :(

Mark

Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Rong Zhang]

Hi Mark,

On Thu, 2026-03-12 at 17:46 -0400, Mark Pearson wrote:
> 
> On Thu, Mar 12, 2026, at 2:01 PM, Rong Zhang wrote:
> > Hi Andrew,
> > 
> > On Wed, 2026-03-11 at 22:29 +0100, Andrew Lunn wrote:
> > > > We just can't prevent the EC from responding to the Fn+Space shortcut.
> > > > So it's essentially user's choice to switch to the hw control trigger
> > > > and make it offloaded to hardware (sorry if my cover letter and replies
> > > > didn't express this well).
> > > 
> > > Do you have any control over the EC?
> > > 
> > > You have a two bosses dilemma. You need to eliminate one of the
> > > bosses. Either the EC controls the LED, or Linux does. Having both
> > > controlling it is just going to work out badly.
> > 
> > I agree that the manufacturers designed the interface poorly :-/
> > 
> > I am not affiliated with any laptop manufacturers so I am just speaking
> > for myself:
> > 
> > IMO the real boss is the user. Both the shortcut (Fn+Space) and the
> > ACPI interface are just "message channels" for the EC to know about the
> > user's choice.
> > 
> > Being able to press such a shortcut always implies that the user is
> > physically in front of the device. In this case it no longer about
> > whether Linux or the EC controls the LED, but both should respect
> > user's choice. That was why brightness_hw_changed was introduced to
> > respect user's choice and pass it to the userspace. So far there has
> > been ~10 drivers utilizing the brightness_hw_changed interface.
> > 
> I am affiliated with a laptop manufacturer :) Happy to take suggestions on what should be improved or is missing (can't promise anything but happy to consider it and take it for review).
> 
> We can set the brightness, get the status, and the FW sends events when it changes - all supported on Linux (for Lenovo devices). This looks like a pretty decent API to me. What is it missing?

IMO, one missing thing is that there's no approach for the OS to
prevent the EC from responding to Fn+Space. Hence no 100% pure software
control is possible. We end up have a mixed SW + EC control.

Another missing thing is that there's no approach for the OS to
query/set the ALS-to-brightness curve (or trip points, whatever you
call it) of the EC driven auto brightness mode. Therefore, should we
have a kernel mode ALS-based software trigger, we would never know if
our curve could be offloaded.

That being said, I don't think either of these two missing things is a
big deal, since most laptops (if not all) are just designed to work
like this and I don't think we would have a kernel mode ALS-based
software trigger any soon. No 100% pure software control wasn't, isn't
and shouldn't be a blocker of using an LED classdev. As I've said,
that's exactly why brightness_hw_changed was introduced.

> 
> I don't understand the two bosses issue I'm afraid. The user ('Linux' in your description?) tells the EC what it wants the LED to be, and the EC does it. The EC is not a boss.

The "user" means the one (i.e., a human) who is using the device. And
"message channels" mean:

User -> pressing Fn+Space -> EC -> update keyboard backlight

User -> LED classdev / manufacturer utilities -> ACPI -> EC -> update
keyboard backlight

They are all about the user, as a human, tells their intention to the
device. Of course there may be some userspace software or kernel
trigger blinking the LED, but again, that's still the user's choice and
intention. Hence I don't think EC is a boss either, and the user is the
real boss.

> 
> > > 
> > > > As my previous reply said, it's common that an LED driver can't prevent
> > > > hardware from changing its state autonomously. Prior to the
> > > > introduction of auto brightness mode, brightness_hw_changed is enough
> > > > to handle this. The issue only emerges when recent models start to
> > > > provide an auto brightness mode based on the ALS sensor.
> > > 
> > > Do you have a software driven brightness mode based on an ALS? What
> > > API do you use to control this? Can you use that API, and accelerate
> > > it?
> > 
> > All devices I've seen implement an EC driven auto brightness mode based
> > on an ALS.
> > 
> > @Mark, do you know any device implementing a software driven auto
> > brightness mode?
> > 
> 
> I don't - to my knowledge in auto mode it's always driven by the HW/FW.

Thanks.

> 
> If there was a SW approach it would read the sensor and set the brightness to low/medium/high (and not to auto) so I'm struggling to understand the issue here. What am I missing?

My understanding of Andrew's words (see also his previous replies) is:

   hw control trigger is fundamentally an accelerated (offloaded)
   software trigger. Only if there is a software-driven ALS-based
   trigger and the curve matches the FW one can we treat the auto
   brightness mode as a hw control trigger.

But those laptops with an ALS and keyboard backlight are not designed
to work like this, and the curve may be very specific to the ALS and
the luminance of the keyboard backlight. So I asked you to confirm if
there is any device designed to use an software driven auto brightness
mode (even under Windows).

Hmm, forgot to ask about that... Is there any device comes with ALS-
based auto brightness mode but Linux cannot read the the ALS? If such
devices exist, the "accelerated software trigger" model is no longer
relevant.

Also that's why we have private LED triggers -- it's useful when the
HW/FW has its own triggering functionality. For example, "cros-ec-led"
has a private trigger and declares it as its hw control trigger.

> 
> > > 
> > > > FYI, desktop environments (e.g., GNOME, KDE) can control the backlight
> > > > brightness of keyboards through sliders and heavily depend on
> > > > brightness_hw_changed to update the sliders and display OSD once the
> > > > shortcut is pressed.
> > > 
> > > Hold up. Terminology problem. I'm a networking guy, i know networking
> > > terms. By slider, do you mean a software scroll bar sort of thing? 
> > 
> > Yes. See
> > https://blogs.kde.org/2024/09/04/brightness-controls-for-all-your-displays/
> > 
> > (it was about display backlight but it also showed the keyboard one in
> > the same image)
> > 
> > > I'm
> > > an XFCE users. I can control the display backlight with a slider on
> > > the battery charge applet. And i can use Fn F4/F5. I've not seen a
> > > software scroll bar for the keyboard backlight, but i think
> > > <CTRL><SPC> allows me to change the keyboard backlight.
> > > 
> > > So we have a slider, which is purely software, Linux. And we have key
> > > presses, which you are calling shortcut, which the EC acts on, and
> > > might tell Linux, maybe, but not about the key press, but the action
> > > the EC took because of the key press.
> > 
> > "might tell", "maybe"
> > 
> > It always tells the OS that the state of keyboard backlight has
> > changed.
> > 
> > > 
> > > You have some API to the EC to ask it nicely to act on the software
> > > slide, but it is the EC which really controls the LED, not Linux.
> > > 
> > > To me a Linux LED is a poor fit for what you want, and i think a
> > > trigger is even worse. The problems you have are because the
> > > LED+trigger model, plus using the hardware for acceleration, does not
> > > fit with the EC actually controlling the hardware.
> > > 
> > > I would suggest you look at the API the EC exports and find a better
> > > model for it.
> > 
> > An LED classdev may be unable to perfectly fit this, but nothing is
> > perfect and so far it's the best thing we have. It's a fortunate to
> > have the LED subsystem. Windows, without a similar interface, ends up
> > being filled with disgusting software pre-installed by the
> > manufacturer.
> > 
> 
> Afraid I don't understand what we are debating here.
> 
> Isn't the whole goal of this patch to make it so LED classdev is a better fit to address missing functionality? Why would switching to something else (I have no idea what) be better? Especially given the the keyboard backlight is currently a LED device, and changing that would potentially break things for users.
> 
> From my perspective if I could just tear this out and have a Lenovo only keyboard_backlight implementation under (for example) /sys/devices/thinkpad_acpi it would be so much easier. But I don't think it is the right thing to do. My experience is if we define a common approach then all vendors will use it going forward - which is better for the Linux experience overall.
> Or we don't have fully implemented features for Linux users? That's kinda sucky.

I agreed with you. Just some supplemental information:

ideapad-laptop has an custom attribute "fn_lock". This used to be
the only way for userspace to turn on/off FnLock. The attribute does
not support any notification mechanism.

Since devices with FnLock also come with an LED indicating the status
of FnLock, an LED classdev has been added with a new
LED_FUNCTION_FNLOCK macro for dt-bindings and UAPI. See commit
7ab6c64663a0 ("dt-bindings: leds: Add LED_FUNCTION_FNLOCK") and commit
07f48f668fac ("platform/x86: ideapad-laptop: add FnLock LED class
device"). Since then, userspace receives notifications through
brightness_hw_changed when the user presses Fn+Esc.

I think this shows that an LED classdev, as a common interface, has its
vitality even when being used in a very specific use case.

> 
> I don't think the two bosses argument is valid (or at least I don't understand it). Are there any other critical implementation details that make this a poor choice and will bite us in the long run?
> 
> I personally find the implementation more complicated than I originally expected, but having looked at it and understood better what Rong was proposing I understand the benefits and I think it works. We're still checking it out on Thinkpad to confirm that, but this patch is a RFC so I think that's part of the process.
> 
> > IMO the presence of brightness_hw_changed proves that an LED classdev,
> > as long as appropriate interfaces are provided, can work well with such
> > hardware. And I don't think there is too much difference between EC
> > setting a static brightness value due to a shortcut being pressed and
> > EC turning on/off the auto brightness mode due to the same shortcut --
> > if we can handle the former well, we can also implement a similar
> > mechanism for the latter.
> > 
> > 
> > Do you have any recommendations for a "better model"?
> > 
> > Did you mean do not register LED classdevs at all? This isn't really
> > viable and will break userspace. Some drivers has been using LED
> > classdevs for keyboard backlight for over a decade. And many
> > `*::kbd_backlight' LEDs rely on brightness_hw_changed, so it's very
> > common that we can't take 100% control over EC. LED classdevs and EC-
> > controlled keyboard backlight have lived in harmony for a long time.
> > 
> > If we still register the keybaord backlight as an LED classdev but use
> > a custom attribute to report/set the auto brightness mode. IMO this is
> > even uglier than LED+trigger, as writing to such a non-brightness
> > attribute will interfere with the brightness attribute and the active
> > trigger and vice versa. Even if we rule out the EC's action, such
> > interference still exists as long as users use the attribute.
> > 
> > 
> > As for the software-vs-hardware priority issue, how about adding an
> > attribute "hw_change_policy" so that users can select if the software
> > state should be always reimposed to hardware?
> 
> Is this needed? When wouldn't this be the case?
> 
> If the SW sets a specific brightness that should become the setting. It would override any previous choices and tell the HW that is what is wanted now - don't change it (until the user says otherwise).
> If we're in auto mode and the HW changes the brightness - it doesn't change the setting from auto mode, just the reported brightness level if queried.

My understanding of Andrew's words is:

   Linux LED should always be the boss. Tell the HW: don't change it *even
   if the user presses Fn+Space*. Failing to accomplish this means that we
   are in "a two bosses dilemma", hence "a Linux LED is a poor fit for
   what you want" and "a trigger is even worse".

Since Andrew cares about the software's precedence, the best thing we
can do is adding an attribute for users to select their preference. The
attribute's default value must not be reimpose, otherwise it breaks
existing userspace programs relying on brightness_hw_changed and
confuses most users.

But yeah, personally I don't think it's needed either. It's been 9
years since the introduction of brightness_hw_changed, and there's no
complaint about HW/FW overriding the software configured brightness.
After all, it's the user who decides to press the shortcut and asks the
EC to change the brightness or turn on/off the auto brightness mode.

Thanks,
Rong

> 
> > 
> > - If the policy is reimpose, calling
> > led_trigger_notify_hw_control_changed() or
> > led_classdev_notify_brightness_hw_changed() results in the software
> > state to be reimposed to the hardware and no event should reach
> > userspace.
> > 
> > - Otherwise calling led_trigger_notify_hw_control_changed() or
> > led_classdev_notify_brightness_hw_changed() updates software state and
> > notifies userspace.
> > 
> I feel like I'm missing something here - but it's been a long day :(
> 
> Mark

Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Mark Pearson]

On Fri, Mar 13, 2026, at 10:01 AM, Rong Zhang wrote:
> Hi Mark,
>
> On Thu, 2026-03-12 at 17:46 -0400, Mark Pearson wrote:
>> 
>> On Thu, Mar 12, 2026, at 2:01 PM, Rong Zhang wrote:
>> > Hi Andrew,
>> > 
>> > On Wed, 2026-03-11 at 22:29 +0100, Andrew Lunn wrote:
>> > > > We just can't prevent the EC from responding to the Fn+Space shortcut.
>> > > > So it's essentially user's choice to switch to the hw control trigger
>> > > > and make it offloaded to hardware (sorry if my cover letter and replies
>> > > > didn't express this well).
>> > > 
>> > > Do you have any control over the EC?
>> > > 
>> > > You have a two bosses dilemma. You need to eliminate one of the
>> > > bosses. Either the EC controls the LED, or Linux does. Having both
>> > > controlling it is just going to work out badly.
>> > 
>> > I agree that the manufacturers designed the interface poorly :-/
>> > 
>> > I am not affiliated with any laptop manufacturers so I am just speaking
>> > for myself:
>> > 
>> > IMO the real boss is the user. Both the shortcut (Fn+Space) and the
>> > ACPI interface are just "message channels" for the EC to know about the
>> > user's choice.
>> > 
>> > Being able to press such a shortcut always implies that the user is
>> > physically in front of the device. In this case it no longer about
>> > whether Linux or the EC controls the LED, but both should respect
>> > user's choice. That was why brightness_hw_changed was introduced to
>> > respect user's choice and pass it to the userspace. So far there has
>> > been ~10 drivers utilizing the brightness_hw_changed interface.
>> > 
>> I am affiliated with a laptop manufacturer :) Happy to take suggestions on what should be improved or is missing (can't promise anything but happy to consider it and take it for review).
>> 
>> We can set the brightness, get the status, and the FW sends events when it changes - all supported on Linux (for Lenovo devices). This looks like a pretty decent API to me. What is it missing?
>
> IMO, one missing thing is that there's no approach for the OS to
> prevent the EC from responding to Fn+Space. Hence no 100% pure software
> control is possible. We end up have a mixed SW + EC control.
>
> Another missing thing is that there's no approach for the OS to
> query/set the ALS-to-brightness curve (or trip points, whatever you
> call it) of the EC driven auto brightness mode. Therefore, should we
> have a kernel mode ALS-based software trigger, we would never know if
> our curve could be offloaded.
>
> That being said, I don't think either of these two missing things is a
> big deal, since most laptops (if not all) are just designed to work
> like this and I don't think we would have a kernel mode ALS-based
> software trigger any soon. No 100% pure software control wasn't, isn't
> and shouldn't be a blocker of using an LED classdev. As I've said,
> that's exactly why brightness_hw_changed was introduced.
>
Got it - thanks for the input.

My personal opinion (not feedback from the FW team)
 - disabling the FN+spacebar is not a useful feature. It's not exactly something you would trigger accidentally. I can't think when people would need to disable it rather than just choosing to not use it.
 - Programming the curve would be more interesting - but becomes a bit of a pro-user use case. Happy to suggest it, but it's a nice-to-have feature IMO.

Agreed that neither of these seem a valid reason to block the implementation.

>> 
>> I don't understand the two bosses issue I'm afraid. The user ('Linux' in your description?) tells the EC what it wants the LED to be, and the EC does it. The EC is not a boss.
>
> The "user" means the one (i.e., a human) who is using the device. And
> "message channels" mean:
>
> User -> pressing Fn+Space -> EC -> update keyboard backlight
>
> User -> LED classdev / manufacturer utilities -> ACPI -> EC -> update
> keyboard backlight
>
> They are all about the user, as a human, tells their intention to the
> device. Of course there may be some userspace software or kernel
> trigger blinking the LED, but again, that's still the user's choice and
> intention. Hence I don't think EC is a boss either, and the user is the
> real boss.
>
I think we're in agreement :)

>> 
>> > > 
>> > > > As my previous reply said, it's common that an LED driver can't prevent
>> > > > hardware from changing its state autonomously. Prior to the
>> > > > introduction of auto brightness mode, brightness_hw_changed is enough
>> > > > to handle this. The issue only emerges when recent models start to
>> > > > provide an auto brightness mode based on the ALS sensor.
>> > > 
>> > > Do you have a software driven brightness mode based on an ALS? What
>> > > API do you use to control this? Can you use that API, and accelerate
>> > > it?
>> > 
>> > All devices I've seen implement an EC driven auto brightness mode based
>> > on an ALS.
>> > 
>> > @Mark, do you know any device implementing a software driven auto
>> > brightness mode?
>> > 
>> 
>> I don't - to my knowledge in auto mode it's always driven by the HW/FW.
>
> Thanks.
>
>> 
>> If there was a SW approach it would read the sensor and set the brightness to low/medium/high (and not to auto) so I'm struggling to understand the issue here. What am I missing?
>
> My understanding of Andrew's words (see also his previous replies) is:
>
>    hw control trigger is fundamentally an accelerated (offloaded)
>    software trigger. Only if there is a software-driven ALS-based
>    trigger and the curve matches the FW one can we treat the auto
>    brightness mode as a hw control trigger.
>
> But those laptops with an ALS and keyboard backlight are not designed
> to work like this, and the curve may be very specific to the ALS and
> the luminance of the keyboard backlight. So I asked you to confirm if
> there is any device designed to use an software driven auto brightness
> mode (even under Windows).
>
> Hmm, forgot to ask about that... Is there any device comes with ALS-
> based auto brightness mode but Linux cannot read the the ALS? If such
> devices exist, the "accelerated software trigger" model is no longer
> relevant.
>
I don't know of any SW driven auto brightness mode.

Afraid I also don't know about reading the ALS. I'll see if I can find out - but I don't think it's directly important to this series which is about supporting the new HW automated feature.
We're not trying to implement an automated SW based control feature with this series :)

> Also that's why we have private LED triggers -- it's useful when the
> HW/FW has its own triggering functionality. For example, "cros-ec-led"
> has a private trigger and declares it as its hw control trigger.
>

ack

>> 
>> > > 
>> > > > FYI, desktop environments (e.g., GNOME, KDE) can control the backlight
>> > > > brightness of keyboards through sliders and heavily depend on
>> > > > brightness_hw_changed to update the sliders and display OSD once the
>> > > > shortcut is pressed.
>> > > 
>> > > Hold up. Terminology problem. I'm a networking guy, i know networking
>> > > terms. By slider, do you mean a software scroll bar sort of thing? 
>> > 
>> > Yes. See
>> > https://blogs.kde.org/2024/09/04/brightness-controls-for-all-your-displays/
>> > 
>> > (it was about display backlight but it also showed the keyboard one in
>> > the same image)
>> > 
>> > > I'm
>> > > an XFCE users. I can control the display backlight with a slider on
>> > > the battery charge applet. And i can use Fn F4/F5. I've not seen a
>> > > software scroll bar for the keyboard backlight, but i think
>> > > <CTRL><SPC> allows me to change the keyboard backlight.
>> > > 
>> > > So we have a slider, which is purely software, Linux. And we have key
>> > > presses, which you are calling shortcut, which the EC acts on, and
>> > > might tell Linux, maybe, but not about the key press, but the action
>> > > the EC took because of the key press.
>> > 
>> > "might tell", "maybe"
>> > 
>> > It always tells the OS that the state of keyboard backlight has
>> > changed.
>> > 
>> > > 
>> > > You have some API to the EC to ask it nicely to act on the software
>> > > slide, but it is the EC which really controls the LED, not Linux.
>> > > 
>> > > To me a Linux LED is a poor fit for what you want, and i think a
>> > > trigger is even worse. The problems you have are because the
>> > > LED+trigger model, plus using the hardware for acceleration, does not
>> > > fit with the EC actually controlling the hardware.
>> > > 
>> > > I would suggest you look at the API the EC exports and find a better
>> > > model for it.
>> > 
>> > An LED classdev may be unable to perfectly fit this, but nothing is
>> > perfect and so far it's the best thing we have. It's a fortunate to
>> > have the LED subsystem. Windows, without a similar interface, ends up
>> > being filled with disgusting software pre-installed by the
>> > manufacturer.
>> > 
>> 
>> Afraid I don't understand what we are debating here.
>> 
>> Isn't the whole goal of this patch to make it so LED classdev is a better fit to address missing functionality? Why would switching to something else (I have no idea what) be better? Especially given the the keyboard backlight is currently a LED device, and changing that would potentially break things for users.
>> 
>> From my perspective if I could just tear this out and have a Lenovo only keyboard_backlight implementation under (for example) /sys/devices/thinkpad_acpi it would be so much easier. But I don't think it is the right thing to do. My experience is if we define a common approach then all vendors will use it going forward - which is better for the Linux experience overall.
>> Or we don't have fully implemented features for Linux users? That's kinda sucky.
>
> I agreed with you. Just some supplemental information:
>
> ideapad-laptop has an custom attribute "fn_lock". This used to be
> the only way for userspace to turn on/off FnLock. The attribute does
> not support any notification mechanism.
>
> Since devices with FnLock also come with an LED indicating the status
> of FnLock, an LED classdev has been added with a new
> LED_FUNCTION_FNLOCK macro for dt-bindings and UAPI. See commit
> 7ab6c64663a0 ("dt-bindings: leds: Add LED_FUNCTION_FNLOCK") and commit
> 07f48f668fac ("platform/x86: ideapad-laptop: add FnLock LED class
> device"). Since then, userspace receives notifications through
> brightness_hw_changed when the user presses Fn+Esc.
>
> I think this shows that an LED classdev, as a common interface, has its
> vitality even when being used in a very specific use case.
>
>> 
>> I don't think the two bosses argument is valid (or at least I don't understand it). Are there any other critical implementation details that make this a poor choice and will bite us in the long run?
>> 
>> I personally find the implementation more complicated than I originally expected, but having looked at it and understood better what Rong was proposing I understand the benefits and I think it works. We're still checking it out on Thinkpad to confirm that, but this patch is a RFC so I think that's part of the process.
>> 
>> > IMO the presence of brightness_hw_changed proves that an LED classdev,
>> > as long as appropriate interfaces are provided, can work well with such
>> > hardware. And I don't think there is too much difference between EC
>> > setting a static brightness value due to a shortcut being pressed and
>> > EC turning on/off the auto brightness mode due to the same shortcut --
>> > if we can handle the former well, we can also implement a similar
>> > mechanism for the latter.
>> > 
>> > 
>> > Do you have any recommendations for a "better model"?
>> > 
>> > Did you mean do not register LED classdevs at all? This isn't really
>> > viable and will break userspace. Some drivers has been using LED
>> > classdevs for keyboard backlight for over a decade. And many
>> > `*::kbd_backlight' LEDs rely on brightness_hw_changed, so it's very
>> > common that we can't take 100% control over EC. LED classdevs and EC-
>> > controlled keyboard backlight have lived in harmony for a long time.
>> > 
>> > If we still register the keybaord backlight as an LED classdev but use
>> > a custom attribute to report/set the auto brightness mode. IMO this is
>> > even uglier than LED+trigger, as writing to such a non-brightness
>> > attribute will interfere with the brightness attribute and the active
>> > trigger and vice versa. Even if we rule out the EC's action, such
>> > interference still exists as long as users use the attribute.
>> > 
>> > 
>> > As for the software-vs-hardware priority issue, how about adding an
>> > attribute "hw_change_policy" so that users can select if the software
>> > state should be always reimposed to hardware?
>> 
>> Is this needed? When wouldn't this be the case?
>> 
>> If the SW sets a specific brightness that should become the setting. It would override any previous choices and tell the HW that is what is wanted now - don't change it (until the user says otherwise).
>> If we're in auto mode and the HW changes the brightness - it doesn't change the setting from auto mode, just the reported brightness level if queried.
>
> My understanding of Andrew's words is:
>
>    Linux LED should always be the boss. Tell the HW: don't change it *even
>    if the user presses Fn+Space*. Failing to accomplish this means that we
>    are in "a two bosses dilemma", hence "a Linux LED is a poor fit for
>    what you want" and "a trigger is even worse".
>
> Since Andrew cares about the software's precedence, the best thing we
> can do is adding an attribute for users to select their preference. The
> attribute's default value must not be reimpose, otherwise it breaks
> existing userspace programs relying on brightness_hw_changed and
> confuses most users.
>

For the current HW, the attribute is not going to be supported. There's no way to disable FN+space as it goes directly to the BIOS (which then notifies the OS).

I guess the OS can get the notification and then say 'nope - despite the fact they just pressed the FN + spacebar, they actually don't want anything to change', and revert the setting to the previous setting. 
If the user had to specifically set the attribute to do this it's fine - but it feels like something that nobody would ever use and a whole bunch of extra complexity, to me

> But yeah, personally I don't think it's needed either. It's been 9
> years since the introduction of brightness_hw_changed, and there's no
> complaint about HW/FW overriding the software configured brightness.
> After all, it's the user who decides to press the shortcut and asks the
> EC to change the brightness or turn on/off the auto brightness mode.
>

I think you and I are both in agreement. 

Andrew - are you still against the current proposal or have your concerns been addressed?

How do we proceed on finding something that we can move forward with?

Mark