Re: [PATCH 2/2] platform/x86: ideapad-laptop: Fully support auto kbd backlight

[Mark Pearson]

Thanks Rong,

On Thu, Aug 7, 2025, at 10:50 AM, Rong Zhang wrote:
> Hi Mark,
>
> On Wed, 2025-08-06 at 15:02 -0400, Mark Pearson wrote:
>> Hi Rong,
>> 
>> On Tue, Aug 5, 2025, at 10:01 AM, Rong Zhang wrote:
>> > 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 sysfs node is exposed to the userspace:
>> > /sys/class/leds/platform::kbd_backlight/als_enabled. The name is chosen
>> > to align with dell-laptop, which provides a similar feature.
>> > 
>> > Signed-off-by: Rong Zhang <i@xxxxxxxx>
>> > ---
>> >  .../ABI/testing/sysfs-platform-ideapad-laptop | 12 ++++
>> >  drivers/platform/x86/lenovo/ideapad-laptop.c  | 65 ++++++++++++++++++-
>> >  2 files changed, 75 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop 
>> > b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
>> > index 5ec0dee9e707..a2b78aa60aaa 100644
>> > --- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
>> > +++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
>> > @@ -50,3 +50,15 @@ Description:
>> >  		Controls whether the "always on USB charging" feature is
>> >  		enabled or not. This feature enables charging USB devices
>> >  		even if the computer is not turned on.
>> > +
>> > +What:		/sys/class/leds/platform::kbd_backlight/als_enabled
>> > +Date:		July 2025
>> > +KernelVersion:	6.17
>> > +Contact:	platform-driver-x86@xxxxxxxxxxxxxxx
>> > +Description:
>> > +		This file allows to control the automatic keyboard
>> > +		illumination mode on some systems that have an ambient
>> > +		light sensor. Write 1 to this file to enable the auto
>> > +		mode, 0 to disable it. In this mode, the actual
>> > +		brightness level is not available and reading the
>> > +		"brightness" file always returns 0.
>> > diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c 
>> > b/drivers/platform/x86/lenovo/ideapad-laptop.c
>> > index 5014c1d0b633..49f2fc68add4 100644
>> > --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
>> > +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
>> > @@ -1712,6 +1712,57 @@ static void ideapad_kbd_bl_notify(struct 
>> > ideapad_private *priv)
>> >  	ideapad_kbd_bl_notify_known(priv, brightness);
>> >  }
>> > 
>> > +static ssize_t als_enabled_show(struct device *dev,
>> > +				struct device_attribute *attr,
>> > +				char *buf)
>> > +{
>> > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> > +	struct ideapad_private *priv = container_of(led_cdev, struct 
>> > ideapad_private, kbd_bl.led);
>> > +	int hw_brightness;
>> > +
>> > +	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
>> > +	if (hw_brightness < 0)
>> > +		return hw_brightness;
>> > +
>> > +	return sysfs_emit(buf, "%d\n", hw_brightness == 
>> > KBD_BL_AUTO_MODE_HW_BRIGHTNESS);
>> > +}
>> > +
>> > +static ssize_t als_enabled_store(struct device *dev,
>> > +				 struct device_attribute *attr,
>> > +				 const char *buf, size_t count)
>> > +{
>> > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> > +	struct ideapad_private *priv = container_of(led_cdev, struct 
>> > ideapad_private, kbd_bl.led);
>> > +	bool state;
>> > +	int err;
>> > +
>> > +	err = kstrtobool(buf, &state);
>> > +	if (err)
>> > +		return err;
>> > +
>> > +	/*
>> > +	 * Auto (ALS) mode uses a predefined HW brightness value. It is
>> > +	 * impossible to disable it without setting another brightness value.
>> > +	 * Set the brightness to 0 when disabling is requested.
>> > +	 */
>> > +	err = ideapad_kbd_bl_hw_brightness_set(priv, state ? 
>> > KBD_BL_AUTO_MODE_HW_BRIGHTNESS : 0);
>> > +	if (err)
>> > +		return err;
>> > +
>> > +	/* Both HW brightness values map to 0 in the LED classdev. */
>> > +	ideapad_kbd_bl_notify_known(priv, 0);
>> > +
>> > +	return count;
>> > +}
>> > +
>> > +static DEVICE_ATTR_RW(als_enabled);
>> > +
>> > +static struct attribute *ideapad_kbd_bl_als_attrs[] = {
>> > +	&dev_attr_als_enabled.attr,
>> > +	NULL,
>> > +};
>> > +ATTRIBUTE_GROUPS(ideapad_kbd_bl_als);
>> > +
>> >  static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>> >  {
>> >  	int brightness, err;
>> > @@ -1722,10 +1773,20 @@ 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)) {
>> > +	switch (priv->kbd_bl.type) {
>> > +	case KBD_BL_TRISTATE_AUTO:
>> > +		/* The sysfs node will be 
>> > /sys/class/leds/platform::kbd_backlight/als_enabled */
>> > +		priv->kbd_bl.led.groups = ideapad_kbd_bl_als_groups;
>> > +		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);
>> > -- 
>> > 2.50.1
>> 
>> We're looking to implement this feature on the Thinkpads, so this change is timely :)
>
> Whoo, it's good to hear that.
>
>> I did wonder if we should be making changes at the LED class level? Something similar to LED_BRIGHT_HW_CHANGED maybe as a way to advertise that auto mode is supported and some hooks to support that in sysfs?
>
> To the best of my knowledge, there is already an ideal model to fit the
> auto brightness mode, which is private LED trigger.
>
> To utilize it, these are four pieces of the puzzle:
>
> (1) implement a private LED trigger (see leds-cros_ec and
>     leds-turris-omnia, for example)
> (2) turn on/off the auto brightness mode when the activate/deactivate
>     hooks are called
> (3) switch to the private LED trigger/the "none" trigger when such mode
>     is turned on/off by the HW (i.e., when Fn+Space is pressed)
> (4) notifying the userspace of the HW-triggered LED trigger change
>
> I'd finished (1) and (2) in my early experiments and verified their
> functionality. However, I eventually realized the dilemma that pressing
> Fn+Space would bring everything into an inconsistent state because of
> the lack of (3).
>
> For (3), when the HW turns on the auto brightness mode, we need:
>
>    mutex_lock(&led_cdev->led_access);
>   
>    down_write(&led_cdev->trigger_lock);
>    led_trigger_set(led_cdev, <THE PRIVATE LED TRIGGER>);
>    up_write(&led_cdev->trigger_lock);
>   
>    mutex_unlock(&led_cdev->led_access);
>
> When off, we need:
>
>    mutex_lock(&led_cdev->led_access);
>   
>    led_trigger_remove(led_cdev);
>   
>    mutex_unlock(&led_cdev->led_access);
>
> I never thought of (4) at that moment. Therefore, I eventually doubted
> whether it was worth so much overhead and turned to the method in the
> current patch.
>
> Think twice now, I think it is worth implementing (1)-(3) as long as
> (4) can be addressed. I just found that both led_trigger_set() and
> led_trigger_remove() send a uevent once the trigger is changed [1], and
> verified this using `udevadm monitor'. We have collected all four
> pieces of the puzzle, hooray!
>
> If you are OK with the private LED trigger approach, I will adopt it in
> [PATCH v2].
>
> [1]: commit 52c47742f79d ("leds: triggers: send uevent when changing
> triggers")
>
I'm not a LED expert, but your proposal (including details below) looks sensible to me.

>> I know it would be more work, but I'm guessing this is going to be a common feature across multiple vendors it might need doing at a common layer.
>
> CC'ing LED class maintainers.
>
excellent idea :)

> Private LED triggers currently have two users: leds-cros_ec and leds-
> turris-omnia. Their private triggers are respectively named "chromeos-
> auto" and "omnia-mcu".
>
> I agree that this is going to be a common feature. A generic name for
> such a feature helps userspace [2] identify it. What about introducing
> a namespace for private LED triggers, so that we can name these
> triggers like "hw-driven:driver-specific-name"?
>
> [2]: AFAIK, KDE Plasma already includes kbd_backlight in its battery
> panel (Plasma 5) or brightness panel (Plasma 6).
>
>> As a note - on the Thinkpads we've had to look at getting the correct Intel ISH firmware loaded (and we're working on getting that upstream to linux-firmware). Is that needed on the Ideapads for the feature to work well or not?
>
> My device (ThinkBook 14 G7+ ASP) has an AMD Ryzen CPU, so the answer
> about Intel ISH firmware is apparent :P
>
Ah...yeah - that won't apply.

> It has two sensor hubs [3]. The ALS sensor is under the AMD Sensor
> Fusion Hub (SFH). The auto brightness mode requires the amd_sfh driver
> to be loaded to work properly, but does *not* need the kernel to load
> the firmware. More details below.
>
> * AMD Sensor Fusion Hub 1.1 (1022:164a, driver: amd_sfh -> hid-sensor-
> hub):
> `` amd_sfh registers a standard HID sensor hub virtual device, which is
> then used by hid-sensor-hub.
> `` Checking the source code of amd_sfh, it doesn't use the firmware
> subsystem, so SFH1.1 seems to have the firmware built into the
> platform.
> `` Firmware version: 0xb000026.
>
> -- Ambient Light Sensor (ALS, driver: hid-sensor-als):
> ``` hid-sensor-als registers an IIO device. It can be monitored via
> iio-sensor-proxy [4].
> ``` The EC can't collect data from it until amd_sfh is loaded. Manually
> unloading (rmmod) amd_sfh also breaks the data availability.
>
> * Ideapad HID sensor hub (IDEA5003/048D:5003, driver: i2c-hid-acpi
> -> hid-sensor-hub):
> `` No IIO sensor is registered because all HID Usages used to pass
> sensor values are vendor-specific.
> `` The only way to monitor it is HIDRAW.
>
> -- Human Presence Detection sensor (HPD, driver: hid-sensor-custom):
> ``` sensor-model=BIOMETRIC_HUMAN_DETECTION
> ``` friendly-name=AMS_TMF882X HOD V2010 Sensor
> ``` sensor-description=2.4 HOR0.0.19
> ``` The EC uses it to wake the device from S0ix (s2idle) on human
> approach.
> ``` I've managed to figure out how to parse its reports to get the
> distance between the human body and the device, as well as its
> confidence.
>
> -- Unknown sensor (driver: hid-sensor-custom):
> ``` sensor-model=LENOVO
> ``` friendly-name=Lenovo AMS_HPD V0302 Sensor
> ``` sensor-manufacturer=LENOVO
> ``` It reports an increasing number periodically.
>
> -- Unknown sensor (driver: hid-sensor-custom):
> ``` sensor-model=Lenovo Customized Gest
> ``` friendly-name=Lenovo AMS_GESTRUE V0209 Sensor
> ``` sensor-manufacturer=LENOVO
> ``` It never sends any HID report.
>
I'll double check if/when we have any of this on the Thinkpads...I don't think we do, but I'm sure we will at some point.

> [3]: Maybe this is a workaround so that the EC can collect data from
> the HPD sensor in S0ix, otherwise this is so weird to have two separate
> sensor hubs since AMD SFH also supports HPD sensors. But the wake-on-
> human-presence feature is already weird anyway - my device wakes itself
> when I am napping at the desk :-/ Zzz...
> [4]: https://gitlab.freedesktop.org/hadess/iio-sensor-proxy
>
> I will just stop here as this somehow becomes off-topic. If you need
> more information about my device (or if you can provide some
> information for me, big thanks \o/), feel free to email me in private.
>
As side notes
 - we are looking at the HPD stuff too...but that's a topic for another thread ;)
 - If your system is waking itself - try the AMD debug tool (https://git.kernel.org/pub/scm/linux/kernel/git/superm1/amd-debug-tools.git) and you might be able to figure out which device is waking you up.
I'll discuss the sensorhubs with the AMD folk too to get their input - I'm a bit behind on that. I'll ping you off thread if we can do something there (and feel free to directly nag me if I don't send anything in the next couple of weeks...it's a bit hectic right now)

Thanks for all the details
Mark