Re: [PATCH v3 6/7] platform/x86: thinkpad_acpi: Add support for auto keyboard backlight

[Rong Zhang]

Hi Qiling,

On Sun, 2026-11-15 at 01:48 +0800, Qiling Zhu wrote:
> Wire the EC keyboard backlight auto mode (level 3) up as a private
> hardware control trigger, named "tpacpi-kbd-auto", on top of the LED
> hw control trigger framework.  On machines that support auto mode
> (MLCG bit 20) the trigger is registered and set as the LED's trigger
> type.
> 
> Selecting the trigger switches the backlight to auto mode; the
> offloaded state is derived from the cached EC level.  When Fn+Space
> cycles into or out of auto mode, the driver notifies the trigger core,
> so the trigger state shown in the trigger sysfs attribute follows
> hardware-initiated transitions.  If the EC is already in auto mode at
> driver initialization, the trigger is set as the default one.  Writing
> a regular level through either sysfs or proc leaves auto mode, and the
> trigger is dropped by the LED core.
> 
> Registering the trigger is allowed to fail; basic backlight control
> keeps working in that case.
> 
> Signed-off-by: Qiling Zhu <kmatzkaczor244@xxxxxxxxx>
> ---
>  drivers/platform/x86/lenovo/Kconfig         |  1 +
>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 95 +++++++++++++++++++--
>  2 files changed, 89 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/Kconfig b/drivers/platform/x86/lenovo/Kconfig
> index 516594993..77d159a3a 100644
> --- a/drivers/platform/x86/lenovo/Kconfig
> +++ b/drivers/platform/x86/lenovo/Kconfig
> @@ -83,6 +83,7 @@ config THINKPAD_ACPI
>  	select NVRAM
>  	select NEW_LEDS
>  	select LEDS_CLASS
> +	select LEDS_TRIGGERS
>  	select INPUT_SPARSEKMAP
>  	help
>  	  This is a driver for the IBM and Lenovo ThinkPad laptops. It adds
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index f9e2d324d..f11e61417 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -5106,7 +5106,12 @@ static struct ibm_struct video_driver_data = {
>  /* EC level that selects the auto mode for the keyboard backlight */
>  #define TPACPI_KBD_LIGHT_AUTO_LEVEL 3
>  
> -static enum led_brightness kbdlight_brightness;
> +/*
> + * Cached EC level, including TPACPI_KBD_LIGHT_AUTO_LEVEL.  All state
> + * tracking compares EC levels; the auto mode is translated to a
> + * brightness value only when reporting to the LED core.
> + */
> +static int kbdlight_brightness;
>  static DEFINE_MUTEX(kbdlight_mutex);
>  
>  /*
> @@ -5191,6 +5196,38 @@ static bool kbdlight_is_supported(void)
>  	return status & BIT(9);
>  }
>  
> +static struct led_hw_trigger_type kbdlight_auto_trigger_type;
> +
> +static enum led_brightness kbdlight_level_to_brightness(int level)
> +{
> +	/* Auto mode is not a brightness level; report it as off. */
> +	if (level == TPACPI_KBD_LIGHT_AUTO_LEVEL)
> +		return 0;
> +
> +	return level;
> +}

Please move the helper to PATCH 1 and use it there.

Meanwhile, the return type can be int.

> +
> +static int kbdlight_auto_trigger_activate(struct led_classdev *led_cdev)
> +{
> +	guard(mutex)(&kbdlight_mutex);
> +
> +	return kbdlight_set_level(TPACPI_KBD_LIGHT_AUTO_LEVEL);

This does not update kbdlight_brightness. See also my reply to PATCH 3.

> +}
> +
> +static bool kbdlight_auto_trigger_hw_offloaded(struct led_classdev *led_cdev)
> +{
> +	guard(mutex)(&kbdlight_mutex);
> +
> +	return kbdlight_brightness == TPACPI_KBD_LIGHT_AUTO_LEVEL;
> +}
> +
> +static struct led_trigger kbdlight_auto_trigger = {
> +	.name		= "tpacpi-kbd-auto",
> +	.trigger_type	= &kbdlight_auto_trigger_type,
> +	.activate	= kbdlight_auto_trigger_activate,
> +	.hw_offloaded	= kbdlight_auto_trigger_hw_offloaded,
> +};
> +
>  static int kbdlight_set_level_and_update(int level);
>  
>  static int kbdlight_sysfs_set(struct led_classdev *led_cdev,
> @@ -5204,11 +5241,11 @@ static enum led_brightness kbdlight_sysfs_get(struct led_classdev *led_cdev)
>  	int level;
>  
>  	level = kbdlight_get_level();
> -	/* Auto mode is not a brightness level; report it as off. */
> -	if (level < 0 || level == TPACPI_KBD_LIGHT_AUTO_LEVEL)
> +
> +	if (level < 0)
>  		return 0;
>  
> -	return level;
> +	return kbdlight_level_to_brightness(level);

What a back-and-forth change! That's why you should really move
kbdlight_level_to_brightness() to PATCH 1 and use it there.

>  }
>  
>  static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
> @@ -5235,9 +5272,37 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
>  		return -ENODEV;
>  	}
>  
> -	kbdlight_brightness = kbdlight_sysfs_get(NULL);
> +	kbdlight_brightness = kbdlight_get_level();
> +	if (kbdlight_brightness < 0)
> +		kbdlight_brightness = 0;
> +
>  	tp_features.kbdlight = 1;
>  
> +	if (tp_features.kbdlight_auto) {
> +		rc = devm_led_trigger_register(&tpacpi_pdev->dev,
> +					       &kbdlight_auto_trigger);
> +		if (rc) {
> +			/*
> +			 * Auto-mode trigger unavailable; basic backlight
> +			 * control keeps working.
> +			 */
> +			dev_warn(&tpacpi_pdev->dev,
> +				 "auto-mode trigger unavailable\n");
> +		} else {
> +			struct led_classdev *led_cdev =
> +				&tpacpi_led_kbdlight.led_classdev;
> +
> +			led_cdev->flags |= LED_TRIG_HW_CHANGED;
> +			led_cdev->trigger_type = &kbdlight_auto_trigger_type;
> +			led_cdev->hw_control_trigger =
> +				kbdlight_auto_trigger.name;
> +
> +			if (kbdlight_brightness == TPACPI_KBD_LIGHT_AUTO_LEVEL)

If it can't preserve across hibernation, how can it preserve across
boots?

If it does preserve, you'll need to take extra care to prevent the LED
core from resetting it on exit, or else this is always a no-op. Note that
setting LED_RETAIN_AT_SHUTDOWN is not sufficient as the teardown routine
of hw control trigger emits LED_OFF.

I am also thinking about enhancing the LED core to properly handle
LED_RETAIN_AT_SHUTDOWN for hw control trigger. That being said, please go
ahead without my enhancement in this cycle by implementing a similar
mechanism like ideapad-laptop, as cross-subsystem coordination takes much
more time.

> +				led_cdev->default_trigger =
> +					kbdlight_auto_trigger.name;

Lines can go up to 100 characters.

> +		}
> +	}

Add a helper function to reduce indentation and improve readability.

> +
>  	rc = led_classdev_register(&tpacpi_pdev->dev,
>  				   &tpacpi_led_kbdlight.led_classdev);
>  	if (rc < 0) {
> @@ -5347,6 +5412,8 @@ static void kbdlight_resume(void)
>  static void kbdlight_notify_level(int level)
>  {
>  	struct led_classdev *led_cdev = &tpacpi_led_kbdlight.led_classdev;
> +	bool is_auto, was_auto;
> +	int brightness;
>  
>  	/*
>  	 * Check the brightness actually changed, setting the brightness
> @@ -5355,15 +5422,29 @@ static void kbdlight_notify_level(int level)
>  	if (level == kbdlight_brightness)
>  		return;
>  
> +	is_auto = level == TPACPI_KBD_LIGHT_AUTO_LEVEL;
> +	was_auto = kbdlight_brightness == TPACPI_KBD_LIGHT_AUTO_LEVEL;
> +
> +	if (is_auto != was_auto)
> +		led_trigger_notify_hw_control_changed(led_cdev, is_auto);

Skip when trigger registration has failed.

> +
>  	kbdlight_brightness = level;
> -	led_classdev_notify_brightness_hw_changed(led_cdev, level);
> +	brightness = kbdlight_level_to_brightness(level);

Let's inline it into the next line.

Thanks,
Rong

> +
> +	led_classdev_notify_brightness_hw_changed(led_cdev, brightness);
>  }
>  
>  static void kbdlight_notify_change(void)
>  {
> +	int level;
> +
>  	guard(mutex)(&kbdlight_mutex);
>  
> -	kbdlight_notify_level(kbdlight_sysfs_get(NULL));
> +	level = kbdlight_get_level();
> +	if (level < 0)
> +		return;
> +
> +	kbdlight_notify_level(level);
>  }
>  
>  static struct ibm_struct kbdlight_driver_data = {