[PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[Vishnu Sankar]

Dynamically detect keyboard backlight capabilities and set
max_brightness correctly (2 for old models, 3 for new models
with Auto mode).

Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
---
 drivers/platform/x86/lenovo/thinkpad_acpi.c | 33 ++++++++++++++++++---
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
index cc19fe520ea9..f670cdd1791e 100644
--- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
+++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
@@ -5043,6 +5043,9 @@ static struct ibm_struct video_driver_data = {
 static enum led_brightness kbdlight_brightness;
 static DEFINE_MUTEX(kbdlight_mutex);
 
+/* Maximum level supported by hardware, will be updated in init */
+static int kbdlight_max_level = 2;
+
 static int kbdlight_set_level(int level)
 {
 	int ret = 0;
@@ -5050,6 +5053,10 @@ static int kbdlight_set_level(int level)
 	if (!hkey_handle)
 		return -ENXIO;
 
+	/* Validate against detected max level */
+	if (level < 0 || level > kbdlight_max_level)
+		return -EINVAL;
+
 	mutex_lock(&kbdlight_mutex);
 
 	if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level))
@@ -5075,6 +5082,7 @@ static int kbdlight_get_level(void)
 	if (status < 0)
 		return status;
 
+	/* Status can be 0, 1, 2, or 3 (Auto) */
 	return status & 0x3;
 }
 
@@ -5143,7 +5151,7 @@ static enum led_brightness kbdlight_sysfs_get(struct led_classdev *led_cdev)
 static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
 	.led_classdev = {
 		.name		= "tpacpi::kbd_backlight",
-		.max_brightness	= 2,
+		.max_brightness	= 2,	/*Initial value, will be updated in init*/
 		.flags		= LED_BRIGHT_HW_CHANGED,
 		.brightness_set_blocking = &kbdlight_sysfs_set,
 		.brightness_get	= &kbdlight_sysfs_get,
@@ -5167,6 +5175,17 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
 	kbdlight_brightness = kbdlight_sysfs_get(NULL);
 	tp_features.kbdlight = 1;
 
+	/* Detect hardware capabilities and set max_brightness */
+	if (acpi_evalf(hkey_handle, NULL, "MLCS", "dd", 3)) {
+		/* MLCS accepts level 3 - new ThinkPad with Auto mode */
+		kbdlight_max_level = 3;
+		tpacpi_led_kbdlight.led_classdev.max_brightness = 3;
+	} else {
+		/* MLCS rejects level 3 - old ThinkPad */
+		kbdlight_max_level = 2;
+		tpacpi_led_kbdlight.led_classdev.max_brightness = 2;
+	}
+
 	rc = led_classdev_register(&tpacpi_pdev->dev,
 				   &tpacpi_led_kbdlight.led_classdev);
 	if (rc < 0) {
@@ -5201,6 +5220,7 @@ static int kbdlight_set_level_and_update(int level)
 static int kbdlight_read(struct seq_file *m)
 {
 	int level;
+	int i;
 
 	if (!tp_features.kbdlight) {
 		seq_printf(m, "status:\t\tnot supported\n");
@@ -5210,9 +5230,13 @@ static int kbdlight_read(struct seq_file *m)
 			seq_printf(m, "status:\t\terror %d\n", level);
 		else
 			seq_printf(m, "status:\t\t%d\n", level);
-		seq_printf(m, "commands:\t0, 1, 2\n");
-	}
 
+		/* Show available commands based on hardware */
+		seq_puts(m, "commands:\t0");
+		for (i = 1; i <= tpacpi_led_kbdlight.led_classdev.max_brightness; i++)
+			seq_printf(m, ", %d", i);
+		seq_puts(m, "\n");
+	}
 	return 0;
 }
 
@@ -5230,7 +5254,8 @@ static int kbdlight_write(char *buf)
 			return res;
 	}
 
-	if (level >= 3 || level < 0)
+	/* Validate against max level */
+	if (level < 0 || level > tpacpi_led_kbdlight.led_classdev.max_brightness)
 		return -EINVAL;
 
 	return kbdlight_set_level_and_update(level);
-- 
2.51.0

Re: [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[Hans de Goede]

Hi Vishnu,

On 4-Feb-26 00:22, Vishnu Sankar wrote:
> Dynamically detect keyboard backlight capabilities and set
> max_brightness correctly (2 for old models, 3 for new models
> with Auto mode).

Thank you for your patch.

If I understand this correctly, writing 3 as level does not
make the backlight more bright then writing 2, but instead
it puts the backlight in some auto mode ?

If I've that correct then  userspace should keep seeing
a range of 0 - 2 and the special auto mode value should
be reported / be made settable through a separate als_enabled
sysfs attribute under the LED class device. See:

Documentation/ABI/testing/sysfs-platform-dell-laptop

You can add extra attributes there by setting the groups
member of the struct led_classdev, see kbd_led_groups[]
in drivers/platform/x86/dell/dell-laptop.c, except that
you should use a .is_visible callback to only show this
on hw which supports it and you only need 1 group with
1 attribute.

Regards,

Hans






> 
> Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
> ---
>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 33 ++++++++++++++++++---
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index cc19fe520ea9..f670cdd1791e 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -5043,6 +5043,9 @@ static struct ibm_struct video_driver_data = {
>  static enum led_brightness kbdlight_brightness;
>  static DEFINE_MUTEX(kbdlight_mutex);
>  
> +/* Maximum level supported by hardware, will be updated in init */
> +static int kbdlight_max_level = 2;
> +
>  static int kbdlight_set_level(int level)
>  {
>  	int ret = 0;
> @@ -5050,6 +5053,10 @@ static int kbdlight_set_level(int level)
>  	if (!hkey_handle)
>  		return -ENXIO;
>  
> +	/* Validate against detected max level */
> +	if (level < 0 || level > kbdlight_max_level)
> +		return -EINVAL;
> +
>  	mutex_lock(&kbdlight_mutex);
>  
>  	if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level))
> @@ -5075,6 +5082,7 @@ static int kbdlight_get_level(void)
>  	if (status < 0)
>  		return status;
>  
> +	/* Status can be 0, 1, 2, or 3 (Auto) */
>  	return status & 0x3;
>  }
>  
> @@ -5143,7 +5151,7 @@ static enum led_brightness kbdlight_sysfs_get(struct led_classdev *led_cdev)
>  static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
>  	.led_classdev = {
>  		.name		= "tpacpi::kbd_backlight",
> -		.max_brightness	= 2,
> +		.max_brightness	= 2,	/*Initial value, will be updated in init*/
>  		.flags		= LED_BRIGHT_HW_CHANGED,
>  		.brightness_set_blocking = &kbdlight_sysfs_set,
>  		.brightness_get	= &kbdlight_sysfs_get,
> @@ -5167,6 +5175,17 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
>  	kbdlight_brightness = kbdlight_sysfs_get(NULL);
>  	tp_features.kbdlight = 1;
>  
> +	/* Detect hardware capabilities and set max_brightness */
> +	if (acpi_evalf(hkey_handle, NULL, "MLCS", "dd", 3)) {
> +		/* MLCS accepts level 3 - new ThinkPad with Auto mode */
> +		kbdlight_max_level = 3;
> +		tpacpi_led_kbdlight.led_classdev.max_brightness = 3;
> +	} else {
> +		/* MLCS rejects level 3 - old ThinkPad */
> +		kbdlight_max_level = 2;
> +		tpacpi_led_kbdlight.led_classdev.max_brightness = 2;
> +	}
> +
>  	rc = led_classdev_register(&tpacpi_pdev->dev,
>  				   &tpacpi_led_kbdlight.led_classdev);
>  	if (rc < 0) {
> @@ -5201,6 +5220,7 @@ static int kbdlight_set_level_and_update(int level)
>  static int kbdlight_read(struct seq_file *m)
>  {
>  	int level;
> +	int i;
>  
>  	if (!tp_features.kbdlight) {
>  		seq_printf(m, "status:\t\tnot supported\n");
> @@ -5210,9 +5230,13 @@ static int kbdlight_read(struct seq_file *m)
>  			seq_printf(m, "status:\t\terror %d\n", level);
>  		else
>  			seq_printf(m, "status:\t\t%d\n", level);
> -		seq_printf(m, "commands:\t0, 1, 2\n");
> -	}
>  
> +		/* Show available commands based on hardware */
> +		seq_puts(m, "commands:\t0");
> +		for (i = 1; i <= tpacpi_led_kbdlight.led_classdev.max_brightness; i++)
> +			seq_printf(m, ", %d", i);
> +		seq_puts(m, "\n");
> +	}
>  	return 0;
>  }
>  
> @@ -5230,7 +5254,8 @@ static int kbdlight_write(char *buf)
>  			return res;
>  	}
>  
> -	if (level >= 3 || level < 0)
> +	/* Validate against max level */
> +	if (level < 0 || level > tpacpi_led_kbdlight.led_classdev.max_brightness)
>  		return -EINVAL;
>  
>  	return kbdlight_set_level_and_update(level);

Re: [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[Rong Zhang]

Hi Hans, Vishnu and Mark,

On Sun, 2026-02-08 at 11:54 +0100, Hans de Goede wrote:
> Hi Vishnu,
> 
> On 4-Feb-26 00:22, Vishnu Sankar wrote:
> > Dynamically detect keyboard backlight capabilities and set
> > max_brightness correctly (2 for old models, 3 for new models
> > with Auto mode).
> 
> Thank you for your patch.
> 
> If I understand this correctly, writing 3 as level does not
> make the backlight more bright then writing 2, but instead
> it puts the backlight in some auto mode ?
> 
> If I've that correct then  userspace should keep seeing
> a range of 0 - 2 and the special auto mode value should
> be reported / be made settable through a separate als_enabled
> sysfs attribute under the LED class device. See:
> 
> Documentation/ABI/testing/sysfs-platform-dell-laptop
> 
> You can add extra attributes there by setting the groups
> member of the struct led_classdev, see kbd_led_groups[]
> in drivers/platform/x86/dell/dell-laptop.c, except that
> you should use a .is_visible callback to only show this
> on hw which supports it and you only need 1 group with
> 1 attribute.

When I implemented "als_enabled" for ideapad-laptop, Mark Pearson
suggested it'd better to introduce "something similar to
LED_BRIGHT_HW_CHANGED" rather than using custom attributes, as "this is
going to be a common feature across multiple vendors it might need
doing at a common layer". Also, auto mode can be activated by HW as a
result of user input, so we need an approach to notify userspace just
like what LED_BRIGHT_HW_CHANGED does. More importantly, the read value
of the brightness attribute becomes nonsense when auto mode is on. This
matches the semantic of hw control trigger.

I agreed with Mark and had a proposal of allowing HW to initiate a
transition from "none" to hw control trigger and vice versa. See the
thread in
https://lore.kernel.org/all/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@app.fastmail.com/

I hadn't push it further due to other things taking the priority,
though I already had a PoC back to then. I quickly rebased the PoC with
some cleanups and put it here for preview:

https://github.com/Rongronggg9/linux/tree/leds-trigger-hw-changed

I will find some time to refine it and send an RFC series.

Thanks,
Rong

> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> > 
> > Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> > Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
> > ---
> >  drivers/platform/x86/lenovo/thinkpad_acpi.c | 33 ++++++++++++++++++---
> >  1 file changed, 29 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > index cc19fe520ea9..f670cdd1791e 100644
> > --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > @@ -5043,6 +5043,9 @@ static struct ibm_struct video_driver_data = {
> >  static enum led_brightness kbdlight_brightness;
> >  static DEFINE_MUTEX(kbdlight_mutex);
> >  
> > +/* Maximum level supported by hardware, will be updated in init */
> > +static int kbdlight_max_level = 2;
> > +
> >  static int kbdlight_set_level(int level)
> >  {
> >  	int ret = 0;
> > @@ -5050,6 +5053,10 @@ static int kbdlight_set_level(int level)
> >  	if (!hkey_handle)
> >  		return -ENXIO;
> >  
> > +	/* Validate against detected max level */
> > +	if (level < 0 || level > kbdlight_max_level)
> > +		return -EINVAL;
> > +
> >  	mutex_lock(&kbdlight_mutex);
> >  
> >  	if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level))
> > @@ -5075,6 +5082,7 @@ static int kbdlight_get_level(void)
> >  	if (status < 0)
> >  		return status;
> >  
> > +	/* Status can be 0, 1, 2, or 3 (Auto) */
> >  	return status & 0x3;
> >  }
> >  
> > @@ -5143,7 +5151,7 @@ static enum led_brightness kbdlight_sysfs_get(struct led_classdev *led_cdev)
> >  static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
> >  	.led_classdev = {
> >  		.name		= "tpacpi::kbd_backlight",
> > -		.max_brightness	= 2,
> > +		.max_brightness	= 2,	/*Initial value, will be updated in init*/
> >  		.flags		= LED_BRIGHT_HW_CHANGED,
> >  		.brightness_set_blocking = &kbdlight_sysfs_set,
> >  		.brightness_get	= &kbdlight_sysfs_get,
> > @@ -5167,6 +5175,17 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
> >  	kbdlight_brightness = kbdlight_sysfs_get(NULL);
> >  	tp_features.kbdlight = 1;
> >  
> > +	/* Detect hardware capabilities and set max_brightness */
> > +	if (acpi_evalf(hkey_handle, NULL, "MLCS", "dd", 3)) {
> > +		/* MLCS accepts level 3 - new ThinkPad with Auto mode */
> > +		kbdlight_max_level = 3;
> > +		tpacpi_led_kbdlight.led_classdev.max_brightness = 3;
> > +	} else {
> > +		/* MLCS rejects level 3 - old ThinkPad */
> > +		kbdlight_max_level = 2;
> > +		tpacpi_led_kbdlight.led_classdev.max_brightness = 2;
> > +	}
> > +
> >  	rc = led_classdev_register(&tpacpi_pdev->dev,
> >  				   &tpacpi_led_kbdlight.led_classdev);
> >  	if (rc < 0) {
> > @@ -5201,6 +5220,7 @@ static int kbdlight_set_level_and_update(int level)
> >  static int kbdlight_read(struct seq_file *m)
> >  {
> >  	int level;
> > +	int i;
> >  
> >  	if (!tp_features.kbdlight) {
> >  		seq_printf(m, "status:\t\tnot supported\n");
> > @@ -5210,9 +5230,13 @@ static int kbdlight_read(struct seq_file *m)
> >  			seq_printf(m, "status:\t\terror %d\n", level);
> >  		else
> >  			seq_printf(m, "status:\t\t%d\n", level);
> > -		seq_printf(m, "commands:\t0, 1, 2\n");
> > -	}
> >  
> > +		/* Show available commands based on hardware */
> > +		seq_puts(m, "commands:\t0");
> > +		for (i = 1; i <= tpacpi_led_kbdlight.led_classdev.max_brightness; i++)
> > +			seq_printf(m, ", %d", i);
> > +		seq_puts(m, "\n");
> > +	}
> >  	return 0;
> >  }
> >  
> > @@ -5230,7 +5254,8 @@ static int kbdlight_write(char *buf)
> >  			return res;
> >  	}
> >  
> > -	if (level >= 3 || level < 0)
> > +	/* Validate against max level */
> > +	if (level < 0 || level > tpacpi_led_kbdlight.led_classdev.max_brightness)
> >  		return -EINVAL;
> >  
> >  	return kbdlight_set_level_and_update(level);

Re: [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[Vishnu Sankar]

Hi Hans and Rong,

Thank you so much for the comments.

On Mon, Feb 9, 2026 at 6:03 AM Rong Zhang <i@rong.moe> wrote:
>
> Hi Hans, Vishnu and Mark,
>
> On Sun, 2026-02-08 at 11:54 +0100, Hans de Goede wrote:
> > Hi Vishnu,
> >
> > On 4-Feb-26 00:22, Vishnu Sankar wrote:
> > > Dynamically detect keyboard backlight capabilities and set
> > > max_brightness correctly (2 for old models, 3 for new models
> > > with Auto mode).
> >
> > Thank you for your patch.
> >
> > If I understand this correctly, writing 3 as level does not
> > make the backlight more bright then writing 2, but instead
> > it puts the backlight in some auto mode ?
> >
This is correct.
> > If I've that correct then  userspace should keep seeing
> > a range of 0 - 2 and the special auto mode value should
> > be reported / be made settable through a separate als_enabled
> > sysfs attribute under the LED class device. See:
> >
> > Documentation/ABI/testing/sysfs-platform-dell-laptop
> >
> > You can add extra attributes there by setting the groups
> > member of the struct led_classdev, see kbd_led_groups[]
> > in drivers/platform/x86/dell/dell-laptop.c, except that
> > you should use a .is_visible callback to only show this
> > on hw which supports it and you only need 1 group with
> > 1 attribute.
Got it.
I believe, once Rong's changes come up, We may need to handle it a bit
different.
>
> When I implemented "als_enabled" for ideapad-laptop, Mark Pearson
> suggested it'd better to introduce "something similar to
> LED_BRIGHT_HW_CHANGED" rather than using custom attributes, as "this is
> going to be a common feature across multiple vendors it might need
> doing at a common layer". Also, auto mode can be activated by HW as a
> result of user input, so we need an approach to notify userspace just
> like what LED_BRIGHT_HW_CHANGED does. More importantly, the read value
> of the brightness attribute becomes nonsense when auto mode is on. This
> matches the semantic of hw control trigger.
>
> I agreed with Mark and had a proposal of allowing HW to initiate a
> transition from "none" to hw control trigger and vice versa. See the
> thread in
> https://lore.kernel.org/all/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@app.fastmail.com/
Thank you Rong.
We had a discussion a few months back about this internally.
I will discuss with Mark on this approach again.
>
> I hadn't push it further due to other things taking the priority,
> though I already had a PoC back to then. I quickly rebased the PoC with
> some cleanups and put it here for preview:
>
> https://github.com/Rongronggg9/linux/tree/leds-trigger-hw-changed
>
> I will find some time to refine it and send an RFC series.
Thank you.
>
> Thanks,
> Rong
>
> > Regards,
> >
> > Hans
> >
> >
> >
> >
> >
> >
> > >
> > > Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> > > Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
> > > ---
> > >  drivers/platform/x86/lenovo/thinkpad_acpi.c | 33 ++++++++++++++++++---
> > >  1 file changed, 29 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > index cc19fe520ea9..f670cdd1791e 100644
> > > --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > > @@ -5043,6 +5043,9 @@ static struct ibm_struct video_driver_data = {
> > >  static enum led_brightness kbdlight_brightness;
> > >  static DEFINE_MUTEX(kbdlight_mutex);
> > >
> > > +/* Maximum level supported by hardware, will be updated in init */
> > > +static int kbdlight_max_level = 2;
> > > +
> > >  static int kbdlight_set_level(int level)
> > >  {
> > >     int ret = 0;
> > > @@ -5050,6 +5053,10 @@ static int kbdlight_set_level(int level)
> > >     if (!hkey_handle)
> > >             return -ENXIO;
> > >
> > > +   /* Validate against detected max level */
> > > +   if (level < 0 || level > kbdlight_max_level)
> > > +           return -EINVAL;
> > > +
> > >     mutex_lock(&kbdlight_mutex);
> > >
> > >     if (!acpi_evalf(hkey_handle, NULL, "MLCS", "dd", level))
> > > @@ -5075,6 +5082,7 @@ static int kbdlight_get_level(void)
> > >     if (status < 0)
> > >             return status;
> > >
> > > +   /* Status can be 0, 1, 2, or 3 (Auto) */
> > >     return status & 0x3;
> > >  }
> > >
> > > @@ -5143,7 +5151,7 @@ static enum led_brightness kbdlight_sysfs_get(struct led_classdev *led_cdev)
> > >  static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
> > >     .led_classdev = {
> > >             .name           = "tpacpi::kbd_backlight",
> > > -           .max_brightness = 2,
> > > +           .max_brightness = 2,    /*Initial value, will be updated in init*/
> > >             .flags          = LED_BRIGHT_HW_CHANGED,
> > >             .brightness_set_blocking = &kbdlight_sysfs_set,
> > >             .brightness_get = &kbdlight_sysfs_get,
> > > @@ -5167,6 +5175,17 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
> > >     kbdlight_brightness = kbdlight_sysfs_get(NULL);
> > >     tp_features.kbdlight = 1;
> > >
> > > +   /* Detect hardware capabilities and set max_brightness */
> > > +   if (acpi_evalf(hkey_handle, NULL, "MLCS", "dd", 3)) {
> > > +           /* MLCS accepts level 3 - new ThinkPad with Auto mode */
> > > +           kbdlight_max_level = 3;
> > > +           tpacpi_led_kbdlight.led_classdev.max_brightness = 3;
> > > +   } else {
> > > +           /* MLCS rejects level 3 - old ThinkPad */
> > > +           kbdlight_max_level = 2;
> > > +           tpacpi_led_kbdlight.led_classdev.max_brightness = 2;
> > > +   }
> > > +
> > >     rc = led_classdev_register(&tpacpi_pdev->dev,
> > >                                &tpacpi_led_kbdlight.led_classdev);
> > >     if (rc < 0) {
> > > @@ -5201,6 +5220,7 @@ static int kbdlight_set_level_and_update(int level)
> > >  static int kbdlight_read(struct seq_file *m)
> > >  {
> > >     int level;
> > > +   int i;
> > >
> > >     if (!tp_features.kbdlight) {
> > >             seq_printf(m, "status:\t\tnot supported\n");
> > > @@ -5210,9 +5230,13 @@ static int kbdlight_read(struct seq_file *m)
> > >                     seq_printf(m, "status:\t\terror %d\n", level);
> > >             else
> > >                     seq_printf(m, "status:\t\t%d\n", level);
> > > -           seq_printf(m, "commands:\t0, 1, 2\n");
> > > -   }
> > >
> > > +           /* Show available commands based on hardware */
> > > +           seq_puts(m, "commands:\t0");
> > > +           for (i = 1; i <= tpacpi_led_kbdlight.led_classdev.max_brightness; i++)
> > > +                   seq_printf(m, ", %d", i);
> > > +           seq_puts(m, "\n");
> > > +   }
> > >     return 0;
> > >  }
> > >
> > > @@ -5230,7 +5254,8 @@ static int kbdlight_write(char *buf)
> > >                     return res;
> > >     }
> > >
> > > -   if (level >= 3 || level < 0)
> > > +   /* Validate against max level */
> > > +   if (level < 0 || level > tpacpi_led_kbdlight.led_classdev.max_brightness)
> > >             return -EINVAL;
> > >
> > >     return kbdlight_set_level_and_update(level);



-- 

Regards,

      Vishnu Sankar
     +817015150407 (Japan)

Re: [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[Mark Pearson]

On Sun, Feb 8, 2026, at 3:58 PM, Rong Zhang wrote:
> Hi Hans, Vishnu and Mark,
>
> On Sun, 2026-02-08 at 11:54 +0100, Hans de Goede wrote:
>> Hi Vishnu,
>> 
>> On 4-Feb-26 00:22, Vishnu Sankar wrote:
>> > Dynamically detect keyboard backlight capabilities and set
>> > max_brightness correctly (2 for old models, 3 for new models
>> > with Auto mode).
>> 
>> Thank you for your patch.
>> 
>> If I understand this correctly, writing 3 as level does not
>> make the backlight more bright then writing 2, but instead
>> it puts the backlight in some auto mode ?
>> 
>> If I've that correct then  userspace should keep seeing
>> a range of 0 - 2 and the special auto mode value should
>> be reported / be made settable through a separate als_enabled
>> sysfs attribute under the LED class device. See:
>> 
>> Documentation/ABI/testing/sysfs-platform-dell-laptop
>> 
>> You can add extra attributes there by setting the groups
>> member of the struct led_classdev, see kbd_led_groups[]
>> in drivers/platform/x86/dell/dell-laptop.c, except that
>> you should use a .is_visible callback to only show this
>> on hw which supports it and you only need 1 group with
>> 1 attribute.
>
> When I implemented "als_enabled" for ideapad-laptop, Mark Pearson
> suggested it'd better to introduce "something similar to
> LED_BRIGHT_HW_CHANGED" rather than using custom attributes, as "this is
> going to be a common feature across multiple vendors it might need
> doing at a common layer". Also, auto mode can be activated by HW as a
> result of user input, so we need an approach to notify userspace just
> like what LED_BRIGHT_HW_CHANGED does. More importantly, the read value
> of the brightness attribute becomes nonsense when auto mode is on. This
> matches the semantic of hw control trigger.
>
> I agreed with Mark and had a proposal of allowing HW to initiate a
> transition from "none" to hw control trigger and vice versa. See the
> thread in
> https://lore.kernel.org/all/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@app.fastmail.com/
>
> I hadn't push it further due to other things taking the priority,
> though I already had a PoC back to then. I quickly rebased the PoC with
> some cleanups and put it here for preview:
>
> https://github.com/Rongronggg9/linux/tree/leds-trigger-hw-changed
>
> I will find some time to refine it and send an RFC series.
>
Hi Rong,

Thanks for highlighting this (have to be honest - I'd forgotten we'd discussed it).
I think my suggestion may have been understood and I wonder your approach is more complicated than needed.

I was thinking we add a new flag to the led_classdev. e.g
#define LED_AUTO_BRIGHTNESS         BIT(26)

Then the platform driver can set this flag and in led_classdev_register_ext we'd handle it appropriately to create a sysfs (e.g. auto_brightness_capable) node so user space knows auto is supported.
Other than that:
 - When the brightness is read and auton is being used - return "auto" instead of a value. Hopefully that doesn't break anything for user space?
 - When the brightness is set, you can use a value or 'auto" as you desire (Documentation would need updating to allow this)

Really I was just looking for a way to advertise to user space that a auto option would be supported :)

Mark

Re: [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[Rong Zhang]

Hi Mark,

Thanks for your reply.

On Mon, 2026-02-09 at 10:46 -0500, Mark Pearson wrote:
> 
> On Sun, Feb 8, 2026, at 3:58 PM, Rong Zhang wrote:
> > Hi Hans, Vishnu and Mark,
> > 
> > On Sun, 2026-02-08 at 11:54 +0100, Hans de Goede wrote:
> > > Hi Vishnu,
> > > 
> > > On 4-Feb-26 00:22, Vishnu Sankar wrote:
> > > > Dynamically detect keyboard backlight capabilities and set
> > > > max_brightness correctly (2 for old models, 3 for new models
> > > > with Auto mode).
> > > 
> > > Thank you for your patch.
> > > 
> > > If I understand this correctly, writing 3 as level does not
> > > make the backlight more bright then writing 2, but instead
> > > it puts the backlight in some auto mode ?
> > > 
> > > If I've that correct then  userspace should keep seeing
> > > a range of 0 - 2 and the special auto mode value should
> > > be reported / be made settable through a separate als_enabled
> > > sysfs attribute under the LED class device. See:
> > > 
> > > Documentation/ABI/testing/sysfs-platform-dell-laptop
> > > 
> > > You can add extra attributes there by setting the groups
> > > member of the struct led_classdev, see kbd_led_groups[]
> > > in drivers/platform/x86/dell/dell-laptop.c, except that
> > > you should use a .is_visible callback to only show this
> > > on hw which supports it and you only need 1 group with
> > > 1 attribute.
> > 
> > When I implemented "als_enabled" for ideapad-laptop, Mark Pearson
> > suggested it'd better to introduce "something similar to
> > LED_BRIGHT_HW_CHANGED" rather than using custom attributes, as "this is
> > going to be a common feature across multiple vendors it might need
> > doing at a common layer". Also, auto mode can be activated by HW as a
> > result of user input, so we need an approach to notify userspace just
> > like what LED_BRIGHT_HW_CHANGED does. More importantly, the read value
> > of the brightness attribute becomes nonsense when auto mode is on. This
> > matches the semantic of hw control trigger.
> > 
> > I agreed with Mark and had a proposal of allowing HW to initiate a
> > transition from "none" to hw control trigger and vice versa. See the
> > thread in
> > https://lore.kernel.org/all/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@app.fastmail.com/
> > 
> > I hadn't push it further due to other things taking the priority,
> > though I already had a PoC back to then. I quickly rebased the PoC with
> > some cleanups and put it here for preview:
> > 
> > https://github.com/Rongronggg9/linux/tree/leds-trigger-hw-changed
> > 
> > I will find some time to refine it and send an RFC series.
> > 
> Hi Rong,
> 
> Thanks for highlighting this (have to be honest - I'd forgotten we'd discussed it).
> I think my suggestion may have been understood and I wonder your approach is more complicated than needed.

If there is a mechanism to set the brightness on specific events or
conditions, it is a trigger. If the trigger is controlled by hardware,
it's a hw control trigger. That's why I propose using a private hw
control trigger to represent this to make it semantically correct.

> I was thinking we add a new flag to the led_classdev. e.g
> #define LED_AUTO_BRIGHTNESS         BIT(26)

Implementing it this way is still complicated as far as I can imagine:

- A new attribute to expose the capability as you've said.
- We need to extend brightness_get/brightness_set[_blocking] interfaces
to accept/emit a special brightness value to represent auto mode.
- We should handle brightness setting requests from usersapce and from
led triggers separately: the former can put the LED into auto mode
while the latter cannot.
- Deprecate brightness and brightness_hw_changed while introducing new
attributes. We can't extend existing attributes as I will explain
later. That's the most frustrating part :-/

And this approach becomes a bit weird if a future SKU comes with its
auto mode tunable: you will have some device attributes which are only
meaningful when auto mode is active. This is all because they are
fundamentally trigger attributes in the first place...

> Then the platform driver can set this flag and in led_classdev_register_ext we'd handle it appropriately to create a sysfs (e.g. auto_brightness_capable) node so user space knows auto is supported.
> Other than that:
>  - When the brightness is read and auton is being used - return "auto" instead of a value. Hopefully that doesn't break anything for user space?

It will likely break something. We can't extend an interface with new
data types.

For example, existing userspace programs may have being using these for
long: 

- POSIX shell: [ -eq, -ne, -gt, -ge, -lt, -le ]
- Bash: let, (( ))
- C: atoi(), atol(), atoll(), fscanf(), vfscanf()
- Python: int()
- Regex: [0-9], \d (PCRE), [[:digit:]] (POSIX)
- ... And more similar things dealing with integers

I think it's not worth deprecating the existing interface just to
introduce something that is not fundamentally "brightness".

>  - When the brightness is set, you can use a value or 'auto" as you desire (Documentation would need updating to allow this)
> Really I was just looking for a way to advertise to user space that a auto option would be supported :)

That's my goal too.

I admit that my proposal is complicated and may need a lot of time to
make it into its right path. It may even be rejected by LED folks. But
it's the best approach I can think of considering our requirements on
the interface:

1. It shouldn't break any existing interfaces.
2. It's exposed to userspace for getting or setting its status.
3. HW status transition should reach userspace (similar to
LED_BRIGHT_HW_CHANGED).

I will see if I can finish my RFC patch this week or next. If it's
rejected probably we will have to continue on "als_enabled"...

Thanks,
Rong

> Mark
> 
> 

Re: [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[Mark Pearson]

Thanks Rong

On Mon, Feb 9, 2026, at 1:14 PM, Rong Zhang wrote:
> Hi Mark,
>
> Thanks for your reply.
>
> On Mon, 2026-02-09 at 10:46 -0500, Mark Pearson wrote:
>> 
>> On Sun, Feb 8, 2026, at 3:58 PM, Rong Zhang wrote:
>> > Hi Hans, Vishnu and Mark,
>> > 
>> > On Sun, 2026-02-08 at 11:54 +0100, Hans de Goede wrote:
>> > > Hi Vishnu,
>> > > 
>> > > On 4-Feb-26 00:22, Vishnu Sankar wrote:
>> > > > Dynamically detect keyboard backlight capabilities and set
>> > > > max_brightness correctly (2 for old models, 3 for new models
>> > > > with Auto mode).
>> > > 
>> > > Thank you for your patch.
>> > > 
>> > > If I understand this correctly, writing 3 as level does not
>> > > make the backlight more bright then writing 2, but instead
>> > > it puts the backlight in some auto mode ?
>> > > 
>> > > If I've that correct then  userspace should keep seeing
>> > > a range of 0 - 2 and the special auto mode value should
>> > > be reported / be made settable through a separate als_enabled
>> > > sysfs attribute under the LED class device. See:
>> > > 
>> > > Documentation/ABI/testing/sysfs-platform-dell-laptop
>> > > 
>> > > You can add extra attributes there by setting the groups
>> > > member of the struct led_classdev, see kbd_led_groups[]
>> > > in drivers/platform/x86/dell/dell-laptop.c, except that
>> > > you should use a .is_visible callback to only show this
>> > > on hw which supports it and you only need 1 group with
>> > > 1 attribute.
>> > 
>> > When I implemented "als_enabled" for ideapad-laptop, Mark Pearson
>> > suggested it'd better to introduce "something similar to
>> > LED_BRIGHT_HW_CHANGED" rather than using custom attributes, as "this is
>> > going to be a common feature across multiple vendors it might need
>> > doing at a common layer". Also, auto mode can be activated by HW as a
>> > result of user input, so we need an approach to notify userspace just
>> > like what LED_BRIGHT_HW_CHANGED does. More importantly, the read value
>> > of the brightness attribute becomes nonsense when auto mode is on. This
>> > matches the semantic of hw control trigger.
>> > 
>> > I agreed with Mark and had a proposal of allowing HW to initiate a
>> > transition from "none" to hw control trigger and vice versa. See the
>> > thread in
>> > https://lore.kernel.org/all/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@app.fastmail.com/
>> > 
>> > I hadn't push it further due to other things taking the priority,
>> > though I already had a PoC back to then. I quickly rebased the PoC with
>> > some cleanups and put it here for preview:
>> > 
>> > https://github.com/Rongronggg9/linux/tree/leds-trigger-hw-changed
>> > 
>> > I will find some time to refine it and send an RFC series.
>> > 
>> Hi Rong,
>> 
>> Thanks for highlighting this (have to be honest - I'd forgotten we'd discussed it).
>> I think my suggestion may have been understood and I wonder your approach is more complicated than needed.
>
> If there is a mechanism to set the brightness on specific events or
> conditions, it is a trigger. If the trigger is controlled by hardware,
> it's a hw control trigger. That's why I propose using a private hw
> control trigger to represent this to make it semantically correct.
>

Ah. I think it will be confusing for most users. They're not going to think of it as a trigger (that's my guess anyway)

>> I was thinking we add a new flag to the led_classdev. e.g
>> #define LED_AUTO_BRIGHTNESS         BIT(26)
>
> Implementing it this way is still complicated as far as I can imagine:
>
> - A new attribute to expose the capability as you've said.
> - We need to extend brightness_get/brightness_set[_blocking] interfaces
> to accept/emit a special brightness value to represent auto mode.
> - We should handle brightness setting requests from usersapce and from
> led triggers separately: the former can put the LED into auto mode
> while the latter cannot.
> - Deprecate brightness and brightness_hw_changed while introducing new
> attributes. We can't extend existing attributes as I will explain
> later. That's the most frustrating part :-/
>
> And this approach becomes a bit weird if a future SKU comes with its
> auto mode tunable: you will have some device attributes which are only
> meaningful when auto mode is active. This is all because they are
> fundamentally trigger attributes in the first place...
>
>> Then the platform driver can set this flag and in led_classdev_register_ext we'd handle it appropriately to create a sysfs (e.g. auto_brightness_capable) node so user space knows auto is supported.
>> Other than that:
>>  - When the brightness is read and auton is being used - return "auto" instead of a value. Hopefully that doesn't break anything for user space?
>
> It will likely break something. We can't extend an interface with new
> data types.
>
> For example, existing userspace programs may have being using these for
> long: 
>
> - POSIX shell: [ -eq, -ne, -gt, -ge, -lt, -le ]
> - Bash: let, (( ))
> - C: atoi(), atol(), atoll(), fscanf(), vfscanf()
> - Python: int()
> - Regex: [0-9], \d (PCRE), [[:digit:]] (POSIX)
> - ... And more similar things dealing with integers
>
> I think it's not worth deprecating the existing interface just to
> introduce something that is not fundamentally "brightness".
>

Yeah - that's fair. You're right - we shouldn't change the brightness field.

So, how about adding two sysfs nodes to the LED class?
 - auto_brightness_capable - indicates the LED brightness can go into an auto control mode
 - auto_brightness_enabled - indicates if the LED is in the auto_brightness controlled state or not.

Then it's up to the individual drivers (thinkpad/ideapad/whatever) to set the fields appropriately as they change modes.
User space will need changing to handle these, but such is life.

>>  - When the brightness is set, you can use a value or 'auto" as you desire (Documentation would need updating to allow this)
>> Really I was just looking for a way to advertise to user space that a auto option would be supported :)
>
> That's my goal too.
>
> I admit that my proposal is complicated and may need a lot of time to
> make it into its right path. It may even be rejected by LED folks. But
> it's the best approach I can think of considering our requirements on
> the interface:
>
> 1. It shouldn't break any existing interfaces.
> 2. It's exposed to userspace for getting or setting its status.
> 3. HW status transition should reach userspace (similar to
> LED_BRIGHT_HW_CHANGED).

Just to check - for #3 do you mean it should report the brightness changes when it's in auto mode (i.e. if it got brighter or dimmer); or if it should just report it switched in/out of auto mode. 
I don't think we need to report every brightness status change - and switching modes should be user directed so is no different to currently. Am I missing something?

Thanks
Mark

Re: [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[Hans de Goede]

Hi all,

On 9-Feb-26 19:44, Mark Pearson wrote:

...

> Yeah - that's fair. You're right - we shouldn't change the brightness field.
> 
> So, how about adding two sysfs nodes to the LED class?
>  - auto_brightness_capable - indicates the LED brightness can go into an auto control mode

There is no need for this, the mere presence of
the "auto_brightness_enabled" sysfs attribute (which can
be in a sysfs-attr-group with an is_visible callback)
is enough to indicate that the backlight is auto
brightness capable.

>  - auto_brightness_enabled - indicates if the LED is in the auto_brightness controlled state or not.

This is for auto-brightness based on an ambient light sensor
(ALS), right ?

My vote would go to use "als_enabled", just like is already done in:

Documentation/ABI/testing/sysfs-platform-dell-laptop

adding new sysfs attributes to a LED class device although
possible is a bit frowned upon though.

In that sense using a trigger is better because it more
closely matches how the LED class API is supposed to be
used would maybe be better.

So I've gone and re-read Rong's trigger proposal:

https://lore.kernel.org/all/a90584179f4c90cd58c03051280a6dda63f6cc1d.camel@rong.moe/

Rong, previously you also went a bit further with implementing this
already which you described here:

https://lore.kernel.org/all/8a132e7473655ca0119af10339c63beb4df7c201.camel@rong.moe/

One of the problems you encountered there is what to do if
the user actually set a trigger themselves and the EC moves
between fixed-brightness-value <-> ALS .

My first idea was to just always override the trigger with
the special ALS trigger or none.

But thinking more about this this is wrong. E.g. there
are triggers which turn the backlight on when user input
is detected and then off after a while, which would be
a perfect reasonable thing to use together with a kbd-backlight.

Thinking more about this triggers are typically for deciding
when to turn the LED on/off not for controlling brightness
many of them actually allow still writing the brightness
sysfs attr and then when the LED should be on according to
that trigger, the trigger use the last written brightness.

Looking at things this way ALS is not really a trigger, it
is more of a brightness control mechanism.

So I think the best and also KISS solution here would be
to go with adding a "als_enabled" sysfs attr to the
LED class device, which is only visible when support,
just like is already done in:

Documentation/ABI/testing/sysfs-platform-dell-laptop

I would also call led_classdev_notify_brightness_hw_changed() 
when the EC moves between fixed-brightness-value <-> ALS.

Userspace will likely already have a poll() going on on
the brightness_hw_changed sysfs attr, so this way userspace
which is aware of the als_enabled sysfs attr can also check
that.

You can then report brightness_max as the new value when calling
led_classdev_notify_brightness_hw_changed() since the ALS can go
up to brightness_max, likewise you could also always return
brightness_max when reading the brightness value while in ALS mode.

The only real question left then is what to do on brightness
writes. I would do the same as what triggers do here, ignore
writing non 0 values and turn off the backlight (and thus also
ALS) when 0 is written.

Note as for actually allowing "auto" for the brightness value
(read/write) that would break userspace assumptions that that
file always contains an integer, so that is not an option IMHO.

Regards,

Hans

Re: [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[Rong Zhang]

Hi all,

Thanks for your insight.

On Tue, 2026-02-10 at 11:31 +0100, Hans de Goede wrote:
> Hi all,
> 
> On 9-Feb-26 19:44, Mark Pearson wrote:
> 
> ...
> 
> > Yeah - that's fair. You're right - we shouldn't change the brightness field.
> > 
> > So, how about adding two sysfs nodes to the LED class?
> >  - auto_brightness_capable - indicates the LED brightness can go into an auto control mode
> 
> There is no need for this, the mere presence of
> the "auto_brightness_enabled" sysfs attribute (which can
> be in a sysfs-attr-group with an is_visible callback)
> is enough to indicate that the backlight is auto
> brightness capable.
> 
> >  - auto_brightness_enabled - indicates if the LED is in the auto_brightness controlled state or not.
> 
> This is for auto-brightness based on an ambient light sensor
> (ALS), right ?
> 
> My vote would go to use "als_enabled", just like is already done in:
> 
> Documentation/ABI/testing/sysfs-platform-dell-laptop

In my previous reply I said 'if my proposal is rejected we will have to
continue on "als_enabled"'. But after some consideration, I have a
major concern about its naming: extensibility.

More and more devices already come with a human presence detection
sensor (HPD). Suppose that a future device implements auto-brightness
based on its HPD, the name will be irrelevant and we must introduce
"hpd_enabled" then. Userspace programs must be rewritten to handle
both.

Mark, do you think such devices may appear in the foreseeable future?

Another concern of mine on the approach is that it may lead to chaos if
a future device implements auto-brightness based on multiple sensors.
In this case a well-defined interface should have an aggregated
attribute^ simply representing whether auto-brightness is on or off.

For this reason, a sensor-neutral name will be better if we should
stick with the device attribute approach.

^: A private hw control trigger is like an aggregated attribute while
it can provide trigger attributes for fine-grained control.

> adding new sysfs attributes to a LED class device although
> possible is a bit frowned upon though.

Yeah, That's one of the reasons why I made that proposal.

> In that sense using a trigger is better because it more
> closely matches how the LED class API is supposed to be
> used would maybe be better.
> 
> So I've gone and re-read Rong's trigger proposal:
> 
> https://lore.kernel.org/all/a90584179f4c90cd58c03051280a6dda63f6cc1d.camel@rong.moe/
> 
> Rong, previously you also went a bit further with implementing this
> already which you described here:
> 
> https://lore.kernel.org/all/8a132e7473655ca0119af10339c63beb4df7c201.camel@rong.moe/
> 
> One of the problems you encountered there is what to do if
> the user actually set a trigger themselves and the EC moves
> between fixed-brightness-value <-> ALS .
> 
> My first idea was to just always override the trigger with
> the special ALS trigger or none.

My PoC does the opposite, see my explanation below.

> But thinking more about this this is wrong. E.g. there
> are triggers which turn the backlight on when user input
> is detected and then off after a while, which would be
> a perfect reasonable thing to use together with a kbd-backlight.

Yes, that's why my PoC intentionally does nothing when an other trigger
is active, effectively allowing the active trigger to override the ALS
trigger. See led_trigger_do_hw_control_transition().

> Thinking more about this triggers are typically for deciding
> when to turn the LED on/off not for controlling brightness

The trigger "pattern" can control LED's brightness.

> many of them actually allow still writing the brightness
> sysfs attr and then when the LED should be on according to
> that trigger, the trigger use the last written brightness.

Thanks for the information! I didn't know there are triggers behaving
like this before.

> Looking at things this way ALS is not really a trigger, it
> is more of a brightness control mechanism.

IIUC, being able to control something that is not capable for a general
purpose LED trigger is the reason why the private trigger interface
exists.

> So I think the best and also KISS solution here would be
> to go with adding a "als_enabled" sysfs attr to the
> LED class device, which is only visible when support,
> just like is already done in:
> 
> Documentation/ABI/testing/sysfs-platform-dell-laptop
> 
> I would also call led_classdev_notify_brightness_hw_changed() 
> when the EC moves between fixed-brightness-value <-> ALS.
> 
> Userspace will likely already have a poll() going on on
> the brightness_hw_changed sysfs attr, so this way userspace
> which is aware of the als_enabled sysfs attr can also check
> that.
> 
> You can then report brightness_max as the new value when calling
> led_classdev_notify_brightness_hw_changed() since the ALS can go
> up to brightness_max, likewise you could also always return
> brightness_max when reading the brightness value while in ALS mode.

Makes sense. If we decide we should stick with the device attribute
approach, I will adopt this in v2 of my ideapad-laptop series.

> The only real question left then is what to do on brightness
> writes. I would do the same as what triggers do here, ignore
> writing non 0 values and turn off the backlight (and thus also
> ALS) when 0 is written.

I have two concerns on this behavior:

1.

IIUC, there is no way for a LED device to determine if it is attached
to a trigger. The LED core does know this, but it won't tell the LED
device driver.

In other words, we can't distinguish trigger requests from userspace
ones in our brightness_set[_blocking] callback, so we have to either
ignore both or none. As a result, we can't ignore anything and must
blindly accept any incoming requests in order not to break triggers.

That's another reason why I made my proposal -- it must become a
trigger in order to be aware of other triggers.

Am I missing something? Or did you mean we should add the attribute to
the LED core?

2.

I quickly rechecked the LED core's code, and it doesn't behave as you
expected. It doesn't ignore non-zero written values when a trigger is
attached to the LED, and it will set the LED's brightness to the
written value despite the trigger (it will be discarded by next trigger
event, though).

When the effective trigger is a hw control trigger, LED core's behavior
effectively disables hw control. Hmm... it seems that this side-effect
has been documented:

   When the LED is in hw control, no software blink is possible and
   doing so will effectively disable hw control.

So in my perspective the hw control trigger approach is semantically
correct here (in an unexpected way).

> Note as for actually allowing "auto" for the brightness value
> (read/write) that would break userspace assumptions that that
> file always contains an integer, so that is not an option IMHO.
> 
> Regards,
> 
> Hans
> 

On Mon, 2026-02-09 at 13:44 -0500, Mark Pearson wrote:
> Thanks Rong
> 
> On Mon, Feb 9, 2026, at 1:14 PM, Rong Zhang wrote:
> > [...]
> > 
> > If there is a mechanism to set the brightness on specific events or
> > conditions, it is a trigger. If the trigger is controlled by hardware,
> > it's a hw control trigger. That's why I propose using a private hw
> > control trigger to represent this to make it semantically correct.
> 
> Ah. I think it will be confusing for most users. They're not going to think of it as a trigger (that's my guess anyway)

I guess most users simply tune the keyboard backlight via desktop
environments, so it won't directly make them confused. When it comes to
desktop environments, this is precisely why we want the interface
capable to notify userspace about HW status transition.

And both `cros_ec' and `turris-omnia' have been using private hw
control triggers to represent auto mode.

> > [...]
> > 
> > I admit that my proposal is complicated and may need a lot of time to
> > make it into its right path. It may even be rejected by LED folks. But
> > it's the best approach I can think of considering our requirements on
> > the interface:
> > 
> > 1. It shouldn't break any existing interfaces.
> > 2. It's exposed to userspace for getting or setting its status.
> > 3. HW status transition should reach userspace (similar to
> > LED_BRIGHT_HW_CHANGED).
> 
> Just to check - for #3 do you mean it should report the brightness changes when it's in auto mode (i.e. if it got brighter or dimmer); or if it should just report it switched in/out of auto mode. 
> I don't think we need to report every brightness status change - and switching modes should be user directed so is no different to currently. Am I missing something?

I meant auto-brightness on <-> off, or fixed-brightness-value <-> ALS
in Hans' words.

> Thanks
> Mark

To conclude:

1. My proposal:

Upsides:
- mutually exclusive with other triggers (hence less chaos)
- semantic correctness
- extensibility (through trigger attributes)
- acts as an aggregate switch to turn on/off hw control

Downsides:
- complexity
- needs approval from LED folks
- (I admit both are major blockers...)

2. Device attribute

Upsides:
- simplicity, KISS
- no need to touch LED core
- extensible as long as it has a sensor-neutral name

Downsides:
- must have zero influence on the brightness_set[_blocking] callback in
order not to break triggers
- potential interference with triggers and the brightness attribute
- weird semantic

I am actually OK with both approaches. If you still consider the device
attribute approach is better after reading my concerns, I will no
longer insist on my proposal.

Thanks,
Rong

Re: [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[Mark Pearson]

On Tue, Feb 10, 2026, at 11:11 AM, Rong Zhang wrote:
> Hi all,
>
> Thanks for your insight.
>
> On Tue, 2026-02-10 at 11:31 +0100, Hans de Goede wrote:
>> Hi all,
>> 
>> On 9-Feb-26 19:44, Mark Pearson wrote:
>> 
>> ...
>> 
>> > Yeah - that's fair. You're right - we shouldn't change the brightness field.
>> > 
>> > So, how about adding two sysfs nodes to the LED class?
>> >  - auto_brightness_capable - indicates the LED brightness can go into an auto control mode
>> 
>> There is no need for this, the mere presence of
>> the "auto_brightness_enabled" sysfs attribute (which can
>> be in a sysfs-attr-group with an is_visible callback)
>> is enough to indicate that the backlight is auto
>> brightness capable.

Good point - agreed

>> 
>> >  - auto_brightness_enabled - indicates if the LED is in the auto_brightness controlled state or not.
>> 
>> This is for auto-brightness based on an ambient light sensor
>> (ALS), right ?
>> 
>> My vote would go to use "als_enabled", just like is already done in:
>> 
>> Documentation/ABI/testing/sysfs-platform-dell-laptop
>

I didn't realise it had been done before (2014 too!)

> In my previous reply I said 'if my proposal is rejected we will have to
> continue on "als_enabled"'. But after some consideration, I have a
> major concern about its naming: extensibility.
>
> More and more devices already come with a human presence detection
> sensor (HPD). Suppose that a future device implements auto-brightness
> based on its HPD, the name will be irrelevant and we must introduce
> "hpd_enabled" then. Userspace programs must be rewritten to handle
> both.
>
> Mark, do you think such devices may appear in the foreseeable future?
>

Not on my radar....but I also don't have a crystal ball.

> Another concern of mine on the approach is that it may lead to chaos if
> a future device implements auto-brightness based on multiple sensors.
> In this case a well-defined interface should have an aggregated
> attribute^ simply representing whether auto-brightness is on or off.
>
> For this reason, a sensor-neutral name will be better if we should
> stick with the device attribute approach.
>

I'm with Hans that if it's been done before then consistency would be nice.
But then again the Dell implementation is Dell specific and we're trying to do something a bit more generic so I don't feel strongly about it

What about "auto_enabled" and the value can be "off" or "als" for now, and if some other mechanism is added in the future it is extensible to other control mechanisms (and they could be multiple - e.g. "als,hpd"). I'm worried we're over-thinking it though.

> ^: A private hw control trigger is like an aggregated attribute while
> it can provide trigger attributes for fine-grained control.
>
>> adding new sysfs attributes to a LED class device although
>> possible is a bit frowned upon though.
>
> Yeah, That's one of the reasons why I made that proposal.
>
>> In that sense using a trigger is better because it more
>> closely matches how the LED class API is supposed to be
>> used would maybe be better.
>> 
>> So I've gone and re-read Rong's trigger proposal:
>> 
>> https://lore.kernel.org/all/a90584179f4c90cd58c03051280a6dda63f6cc1d.camel@rong.moe/
>> 
>> Rong, previously you also went a bit further with implementing this
>> already which you described here:
>> 
>> https://lore.kernel.org/all/8a132e7473655ca0119af10339c63beb4df7c201.camel@rong.moe/
>> 
>> One of the problems you encountered there is what to do if
>> the user actually set a trigger themselves and the EC moves
>> between fixed-brightness-value <-> ALS .
>> 
>> My first idea was to just always override the trigger with
>> the special ALS trigger or none.
>
> My PoC does the opposite, see my explanation below.
>
>> But thinking more about this this is wrong. E.g. there
>> are triggers which turn the backlight on when user input
>> is detected and then off after a while, which would be
>> a perfect reasonable thing to use together with a kbd-backlight.
>
> Yes, that's why my PoC intentionally does nothing when an other trigger
> is active, effectively allowing the active trigger to override the ALS
> trigger. See led_trigger_do_hw_control_transition().
>
>> Thinking more about this triggers are typically for deciding
>> when to turn the LED on/off not for controlling brightness
>
> The trigger "pattern" can control LED's brightness.
>
>> many of them actually allow still writing the brightness
>> sysfs attr and then when the LED should be on according to
>> that trigger, the trigger use the last written brightness.
>
> Thanks for the information! I didn't know there are triggers behaving
> like this before.
>
>> Looking at things this way ALS is not really a trigger, it
>> is more of a brightness control mechanism.
>
> IIUC, being able to control something that is not capable for a general
> purpose LED trigger is the reason why the private trigger interface
> exists.
>
>> So I think the best and also KISS solution here would be
>> to go with adding a "als_enabled" sysfs attr to the
>> LED class device, which is only visible when support,
>> just like is already done in:
>> 
>> Documentation/ABI/testing/sysfs-platform-dell-laptop
>> 
>> I would also call led_classdev_notify_brightness_hw_changed() 
>> when the EC moves between fixed-brightness-value <-> ALS.
>> 
>> Userspace will likely already have a poll() going on on
>> the brightness_hw_changed sysfs attr, so this way userspace
>> which is aware of the als_enabled sysfs attr can also check
>> that.
>> 
>> You can then report brightness_max as the new value when calling
>> led_classdev_notify_brightness_hw_changed() since the ALS can go
>> up to brightness_max, likewise you could also always return
>> brightness_max when reading the brightness value while in ALS mode.
>
> Makes sense. If we decide we should stick with the device attribute
> approach, I will adopt this in v2 of my ideapad-laptop series.
>
>> The only real question left then is what to do on brightness
>> writes. I would do the same as what triggers do here, ignore
>> writing non 0 values and turn off the backlight (and thus also
>> ALS) when 0 is written.
>
> I have two concerns on this behavior:
>
> 1.
>
> IIUC, there is no way for a LED device to determine if it is attached
> to a trigger. The LED core does know this, but it won't tell the LED
> device driver.
>
> In other words, we can't distinguish trigger requests from userspace
> ones in our brightness_set[_blocking] callback, so we have to either
> ignore both or none. As a result, we can't ignore anything and must
> blindly accept any incoming requests in order not to break triggers.
>
> That's another reason why I made my proposal -- it must become a
> trigger in order to be aware of other triggers.
>
> Am I missing something? Or did you mean we should add the attribute to
> the LED core?
>
> 2.
>
> I quickly rechecked the LED core's code, and it doesn't behave as you
> expected. It doesn't ignore non-zero written values when a trigger is
> attached to the LED, and it will set the LED's brightness to the
> written value despite the trigger (it will be discarded by next trigger
> event, though).
>

I'm a bit confused here to be honest.
If a user sets a specific level - then it should disable auto mode shouldn't it?

> When the effective trigger is a hw control trigger, LED core's behavior
> effectively disables hw control. Hmm... it seems that this side-effect
> has been documented:
>
>    When the LED is in hw control, no software blink is possible and
>    doing so will effectively disable hw control.
>
> So in my perspective the hw control trigger approach is semantically
> correct here (in an unexpected way).
>
>> Note as for actually allowing "auto" for the brightness value
>> (read/write) that would break userspace assumptions that that
>> file always contains an integer, so that is not an option IMHO.
>> 

Yeah - I think we're all in agreement that it was a bad idea. I have swept it back under the rock it came from

>> Regards,
>> 
>> Hans
>> 
>
> On Mon, 2026-02-09 at 13:44 -0500, Mark Pearson wrote:
>> Thanks Rong
>> 
>> On Mon, Feb 9, 2026, at 1:14 PM, Rong Zhang wrote:
>> > [...]
>> > 
>> > If there is a mechanism to set the brightness on specific events or
>> > conditions, it is a trigger. If the trigger is controlled by hardware,
>> > it's a hw control trigger. That's why I propose using a private hw
>> > control trigger to represent this to make it semantically correct.
>> 
>> Ah. I think it will be confusing for most users. They're not going to think of it as a trigger (that's my guess anyway)
>
> I guess most users simply tune the keyboard backlight via desktop
> environments, so it won't directly make them confused. When it comes to
> desktop environments, this is precisely why we want the interface
> capable to notify userspace about HW status transition.
>
> And both `cros_ec' and `turris-omnia' have been using private hw
> control triggers to represent auto mode.
>
>> > [...]
>> > 
>> > I admit that my proposal is complicated and may need a lot of time to
>> > make it into its right path. It may even be rejected by LED folks. But
>> > it's the best approach I can think of considering our requirements on
>> > the interface:
>> > 
>> > 1. It shouldn't break any existing interfaces.
>> > 2. It's exposed to userspace for getting or setting its status.
>> > 3. HW status transition should reach userspace (similar to
>> > LED_BRIGHT_HW_CHANGED).
>> 
>> Just to check - for #3 do you mean it should report the brightness changes when it's in auto mode (i.e. if it got brighter or dimmer); or if it should just report it switched in/out of auto mode. 
>> I don't think we need to report every brightness status change - and switching modes should be user directed so is no different to currently. Am I missing something?
>
> I meant auto-brightness on <-> off, or fixed-brightness-value <-> ALS
> in Hans' words.
>

great - makes sense

>> Thanks
>> Mark
>
> To conclude:
>
> 1. My proposal:
>
> Upsides:
> - mutually exclusive with other triggers (hence less chaos)
> - semantic correctness
> - extensibility (through trigger attributes)
> - acts as an aggregate switch to turn on/off hw control
>
> Downsides:
> - complexity
> - needs approval from LED folks
> - (I admit both are major blockers...)
>
> 2. Device attribute
>
> Upsides:
> - simplicity, KISS
> - no need to touch LED core
> - extensible as long as it has a sensor-neutral name
>
> Downsides:
> - must have zero influence on the brightness_set[_blocking] callback in
> order not to break triggers
> - potential interference with triggers and the brightness attribute
> - weird semantic
>
> I am actually OK with both approaches. If you still consider the device
> attribute approach is better after reading my concerns, I will no
> longer insist on my proposal.
>

My (limited) vote is for the simplest solution - because it's just a keyboard light. But I'll defer to Hans here - I don't think I know enough to have a strong opinion.

Mark

Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[Mark Pearson]

On Tue, Feb 10, 2026, at 4:17 PM, Mark Pearson wrote:
> On Tue, Feb 10, 2026, at 11:11 AM, Rong Zhang wrote:
>> Hi all,
>>
>> Thanks for your insight.
>>
>> On Tue, 2026-02-10 at 11:31 +0100, Hans de Goede wrote:
>>> Hi all,
>>> 
>>> On 9-Feb-26 19:44, Mark Pearson wrote:
>>> 
>>> ...
>>> 
>>> > Yeah - that's fair. You're right - we shouldn't change the brightness field.
>>> > 
>>> > So, how about adding two sysfs nodes to the LED class?
>>> >  - auto_brightness_capable - indicates the LED brightness can go into an auto control mode
>>> 
>>> There is no need for this, the mere presence of
>>> the "auto_brightness_enabled" sysfs attribute (which can
>>> be in a sysfs-attr-group with an is_visible callback)
>>> is enough to indicate that the backlight is auto
>>> brightness capable.
>
> Good point - agreed
>
>>> 
>>> >  - auto_brightness_enabled - indicates if the LED is in the auto_brightness controlled state or not.
>>> 
>>> This is for auto-brightness based on an ambient light sensor
>>> (ALS), right ?
>>> 
>>> My vote would go to use "als_enabled", just like is already done in:
>>> 
>>> Documentation/ABI/testing/sysfs-platform-dell-laptop
>>
>
> I didn't realise it had been done before (2014 too!)
>
>> In my previous reply I said 'if my proposal is rejected we will have to
>> continue on "als_enabled"'. But after some consideration, I have a
>> major concern about its naming: extensibility.
>>
>> More and more devices already come with a human presence detection
>> sensor (HPD). Suppose that a future device implements auto-brightness
>> based on its HPD, the name will be irrelevant and we must introduce
>> "hpd_enabled" then. Userspace programs must be rewritten to handle
>> both.
>>
>> Mark, do you think such devices may appear in the foreseeable future?
>>
>
> Not on my radar....but I also don't have a crystal ball.
>
>> Another concern of mine on the approach is that it may lead to chaos if
>> a future device implements auto-brightness based on multiple sensors.
>> In this case a well-defined interface should have an aggregated
>> attribute^ simply representing whether auto-brightness is on or off.
>>
>> For this reason, a sensor-neutral name will be better if we should
>> stick with the device attribute approach.
>>
>
> I'm with Hans that if it's been done before then consistency would be 
> nice.
> But then again the Dell implementation is Dell specific and we're 
> trying to do something a bit more generic so I don't feel strongly 
> about it
>
> What about "auto_enabled" and the value can be "off" or "als" for now, 
> and if some other mechanism is added in the future it is extensible to 
> other control mechanisms (and they could be multiple - e.g. "als,hpd"). 
> I'm worried we're over-thinking it though.
>
>> ^: A private hw control trigger is like an aggregated attribute while
>> it can provide trigger attributes for fine-grained control.
>>
>>> adding new sysfs attributes to a LED class device although
>>> possible is a bit frowned upon though.
>>
>> Yeah, That's one of the reasons why I made that proposal.
>>
>>> In that sense using a trigger is better because it more
>>> closely matches how the LED class API is supposed to be
>>> used would maybe be better.
>>> 
>>> So I've gone and re-read Rong's trigger proposal:
>>> 
>>> https://lore.kernel.org/all/a90584179f4c90cd58c03051280a6dda63f6cc1d.camel@rong.moe/
>>> 
>>> Rong, previously you also went a bit further with implementing this
>>> already which you described here:
>>> 
>>> https://lore.kernel.org/all/8a132e7473655ca0119af10339c63beb4df7c201.camel@rong.moe/
>>> 
>>> One of the problems you encountered there is what to do if
>>> the user actually set a trigger themselves and the EC moves
>>> between fixed-brightness-value <-> ALS .
>>> 
>>> My first idea was to just always override the trigger with
>>> the special ALS trigger or none.
>>
>> My PoC does the opposite, see my explanation below.
>>
>>> But thinking more about this this is wrong. E.g. there
>>> are triggers which turn the backlight on when user input
>>> is detected and then off after a while, which would be
>>> a perfect reasonable thing to use together with a kbd-backlight.
>>
>> Yes, that's why my PoC intentionally does nothing when an other trigger
>> is active, effectively allowing the active trigger to override the ALS
>> trigger. See led_trigger_do_hw_control_transition().
>>
>>> Thinking more about this triggers are typically for deciding
>>> when to turn the LED on/off not for controlling brightness
>>
>> The trigger "pattern" can control LED's brightness.
>>
>>> many of them actually allow still writing the brightness
>>> sysfs attr and then when the LED should be on according to
>>> that trigger, the trigger use the last written brightness.
>>
>> Thanks for the information! I didn't know there are triggers behaving
>> like this before.
>>
>>> Looking at things this way ALS is not really a trigger, it
>>> is more of a brightness control mechanism.
>>
>> IIUC, being able to control something that is not capable for a general
>> purpose LED trigger is the reason why the private trigger interface
>> exists.
>>
>>> So I think the best and also KISS solution here would be
>>> to go with adding a "als_enabled" sysfs attr to the
>>> LED class device, which is only visible when support,
>>> just like is already done in:
>>> 
>>> Documentation/ABI/testing/sysfs-platform-dell-laptop
>>> 
>>> I would also call led_classdev_notify_brightness_hw_changed() 
>>> when the EC moves between fixed-brightness-value <-> ALS.
>>> 
>>> Userspace will likely already have a poll() going on on
>>> the brightness_hw_changed sysfs attr, so this way userspace
>>> which is aware of the als_enabled sysfs attr can also check
>>> that.
>>> 
>>> You can then report brightness_max as the new value when calling
>>> led_classdev_notify_brightness_hw_changed() since the ALS can go
>>> up to brightness_max, likewise you could also always return
>>> brightness_max when reading the brightness value while in ALS mode.
>>
>> Makes sense. If we decide we should stick with the device attribute
>> approach, I will adopt this in v2 of my ideapad-laptop series.
>>
>>> The only real question left then is what to do on brightness
>>> writes. I would do the same as what triggers do here, ignore
>>> writing non 0 values and turn off the backlight (and thus also
>>> ALS) when 0 is written.
>>
>> I have two concerns on this behavior:
>>
>> 1.
>>
>> IIUC, there is no way for a LED device to determine if it is attached
>> to a trigger. The LED core does know this, but it won't tell the LED
>> device driver.
>>
>> In other words, we can't distinguish trigger requests from userspace
>> ones in our brightness_set[_blocking] callback, so we have to either
>> ignore both or none. As a result, we can't ignore anything and must
>> blindly accept any incoming requests in order not to break triggers.
>>
>> That's another reason why I made my proposal -- it must become a
>> trigger in order to be aware of other triggers.
>>
>> Am I missing something? Or did you mean we should add the attribute to
>> the LED core?
>>
>> 2.
>>
>> I quickly rechecked the LED core's code, and it doesn't behave as you
>> expected. It doesn't ignore non-zero written values when a trigger is
>> attached to the LED, and it will set the LED's brightness to the
>> written value despite the trigger (it will be discarded by next trigger
>> event, though).
>>
>
> I'm a bit confused here to be honest.
> If a user sets a specific level - then it should disable auto mode shouldn't it?
>
>> When the effective trigger is a hw control trigger, LED core's behavior
>> effectively disables hw control. Hmm... it seems that this side-effect
>> has been documented:
>>
>>    When the LED is in hw control, no software blink is possible and
>>    doing so will effectively disable hw control.
>>
>> So in my perspective the hw control trigger approach is semantically
>> correct here (in an unexpected way).
>>
>>> Note as for actually allowing "auto" for the brightness value
>>> (read/write) that would break userspace assumptions that that
>>> file always contains an integer, so that is not an option IMHO.
>>> 
>
> Yeah - I think we're all in agreement that it was a bad idea. I have 
> swept it back under the rock it came from
>
>>> Regards,
>>> 
>>> Hans
>>> 
>>
>> On Mon, 2026-02-09 at 13:44 -0500, Mark Pearson wrote:
>>> Thanks Rong
>>> 
>>> On Mon, Feb 9, 2026, at 1:14 PM, Rong Zhang wrote:
>>> > [...]
>>> > 
>>> > If there is a mechanism to set the brightness on specific events or
>>> > conditions, it is a trigger. If the trigger is controlled by hardware,
>>> > it's a hw control trigger. That's why I propose using a private hw
>>> > control trigger to represent this to make it semantically correct.
>>> 
>>> Ah. I think it will be confusing for most users. They're not going to think of it as a trigger (that's my guess anyway)
>>
>> I guess most users simply tune the keyboard backlight via desktop
>> environments, so it won't directly make them confused. When it comes to
>> desktop environments, this is precisely why we want the interface
>> capable to notify userspace about HW status transition.
>>
>> And both `cros_ec' and `turris-omnia' have been using private hw
>> control triggers to represent auto mode.
>>
>>> > [...]
>>> > 
>>> > I admit that my proposal is complicated and may need a lot of time to
>>> > make it into its right path. It may even be rejected by LED folks. But
>>> > it's the best approach I can think of considering our requirements on
>>> > the interface:
>>> > 
>>> > 1. It shouldn't break any existing interfaces.
>>> > 2. It's exposed to userspace for getting or setting its status.
>>> > 3. HW status transition should reach userspace (similar to
>>> > LED_BRIGHT_HW_CHANGED).
>>> 
>>> Just to check - for #3 do you mean it should report the brightness changes when it's in auto mode (i.e. if it got brighter or dimmer); or if it should just report it switched in/out of auto mode. 
>>> I don't think we need to report every brightness status change - and switching modes should be user directed so is no different to currently. Am I missing something?
>>
>> I meant auto-brightness on <-> off, or fixed-brightness-value <-> ALS
>> in Hans' words.
>>
>
> great - makes sense
>
>>> Thanks
>>> Mark
>>
>> To conclude:
>>
>> 1. My proposal:
>>
>> Upsides:
>> - mutually exclusive with other triggers (hence less chaos)
>> - semantic correctness
>> - extensibility (through trigger attributes)
>> - acts as an aggregate switch to turn on/off hw control
>>
>> Downsides:
>> - complexity
>> - needs approval from LED folks
>> - (I admit both are major blockers...)
>>
>> 2. Device attribute
>>
>> Upsides:
>> - simplicity, KISS
>> - no need to touch LED core
>> - extensible as long as it has a sensor-neutral name
>>
>> Downsides:
>> - must have zero influence on the brightness_set[_blocking] callback in
>> order not to break triggers
>> - potential interference with triggers and the brightness attribute
>> - weird semantic
>>
>> I am actually OK with both approaches. If you still consider the device
>> attribute approach is better after reading my concerns, I will no
>> longer insist on my proposal.
>>
>
> My (limited) vote is for the simplest solution - because it's just a 
> keyboard light. But I'll defer to Hans here - I don't think I know 
> enough to have a strong opinion.
>

Hi Hans - do you have an opinion on the path forwards? Would appreciate your expert guidance here :)

Mark