Message-ID: <8a132e7473655ca0119af10339c63beb4df7c201.camel@rong.moe>
Date: Sat, 16 Aug 2025 04:36:48 +0800
From: Rong Zhang <i@...g.moe>
To: Lee Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>, Mark
 Pearson	 <mpearson-lenovo@...ebb.ca>
Cc: Ike Panhc <ikepanhc@...il.com>, "Derek J . Clark"
	 <derekjohn.clark@...il.com>, Ilpo Järvinen
	 <ilpo.jarvinen@...ux.intel.com>, Hans de Goede <hansg@...nel.org>, 
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-leds@...r.kernel.org
Subject: Re: [PATCH 2/2] platform/x86: ideapad-laptop: Fully support auto
 kbd backlight

Hi All,

On Fri, 2025-08-08 at 13:57 -0400, Mark Pearson wrote:
> 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@...g.moe>
> > > > ---
> > > >  .../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@...r.kernel.org
> > > > +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);

After some careful consideration, I propose a new API for this:

static int __led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig,
			     bool hw_triggered)
{
	[...]
	if (led_cdev->trigger) {
	[...]
		if (trig || !hw_triggered)
			led_set_brightness(led_cdev, LED_OFF);
	}
	[...]
}

int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
{
	return __led_trigger_set(led_cdev, trig, false);
}

void led_trigger_notify_hw_control_changed(struct led_classdev *led_cdev, bool activate)
{
	/* Callers are expected to acquire led_cdev->led_access first. */
	/* Hold led_cdev->trigger_lock, release when finish. */

	hc_trig = /* Search for the trigger named after led_cdev->hw_control_trigger. */

	/* We have 6 possible situations to handle: */

	/* Hardware just activated hw control mode, while "none" is active */
	if (activate && !led_cdev->trigger)
		__led_trigger_set(led_cdev, hc_trig, true);

	/* Hardware just deactivated hw control mode, while hc_trig is active */
	else if (!activate && led_cdev->trigger == hc_trig)
		__led_trigger_set(led_cdev, NULL, true);

	/* Hardware just activated hw control mode, while hc_trig is active */
	/* Hardware just activated hw control mode, while another trigger is active */
	/* Hardware just deactivated hw control mode, while "none" is active */
	/* Hardware just deactivated hw control mode, while another trigger is active */
}

void led_trigger_notify_hw_control_changed_fast(struct led_classdev *led_cdev,
						struct led_trigger *hc_trig, bool activate)
{
	/*
	 * LED drivers with private trigger may call this, eliminating the need
	 * to search for hc_trig. Everything else is the same as
	 * led_trigger_notify_hw_control_changed.
	 */
}

I will send an RFC patchset for this when ready (probably in the next
week). For now, let me explain the new API:

1. If a keyboard is capable of adjusting its backlight automatically
according to ALS, a private trigger can excellently represent this
capability. However, most keyboards with auto backlight can also switch
it on and off upon user input, making the activation state of such
private triggers meaningless noise. This is a major blocker. If we
provide an API for HW-triggered LED trigger transition, I expect
various (future or existing) drivers for keyboards/laptops can adopt
it.

2. When HW deactivates hw control mode on its own, it may choose a
specific brightness instead of simply turning off the LED. We must keep
it unchanged, or else brightness cycling will be broken. In the case of
my ThinkBook, pressing Fn+Space cycles the keyboard backlight in the
following order: auto => low => high => off => auto. @Mark, how is the
case for IdeaPad/ThinkPad?

3. Only "hc_trig <=> none" transition is possible. Should we also
reactivate the current trigger if it is neither hw control trigger nor
"none"?

4. The API are void functions, since LED drivers are always responsible
for participating in the (de)activation sequence of hw control mode and
should collect enough information about the current status during their
participation.

5. The LED driver needs to be able to handle the activation sequence
even if the hardware is currently under hw control mode, and vice
versa. This shouldn't be a big problem since I suppose most LED
hardware is capable of entering/leaving hw control mode idempotently.
In case any driver needs it, the new API doesn't touch
led_cdev->led_access so that LED drivers can protect their housekeeping
work, preparing for the idempotence.

6. I am not very familiar with ledtrig-netdev. In my glance, it always
checks the HW state and initializes its options accordingly on
activation, so it is apparently non-idempotent. I think this is not a
big deal since there shouldn't be any NIC that would switch hw control
state for its LED on its own.

> > 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 will first incorporate this patch into the RFC patch mentioned above
to provide a reference usage and demonstrate the simplicity of this API
in use. When the discussion is settled, I will submit them separately.

> > > 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).

When designing the above API, I realized that such a simple naming
trick has inherent flaws: it doesn't make sense for dual-role triggers.

The "netdev" trigger is either a software trigger or a hardware-driven
trigger, depending on the underlying LED hardware and trigger options.
It has an attribute "offloaded" for its dual-role capability.

Thus, I propose a read-only attribute "trigger_may_offload". It is
exposed when led_cdev->hw_control_trigger is defined. Its value depends
on the state of the hw control trigger:

- Offloaded: "[hw_control_trigger]"
- Active but not offloaded: "<hw_control_trigger>"
- Inactive: "hw_control_trigger"

In this way, it perfectly answers two questions (Which trigger? What is
its state?) from userspace at once. It can also be easily extended if
we (unfortunately) meet some HW with more than one possible hw control
trigger.

A new method led_trigger->offloaded() also needs to be introduced to
make the attribute useful. This should be easy.

I will send another RFC patchset for this when ready. This should be
independent of the one for led_trigger_notify_hw_control_changed since
their functionalities are orthogonal.

> > > 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.
> > 

Hi Mark,

> As side notes
>  - we are looking at the HPD stuff too...but that's a topic for another thread ;)

So glad to hear that. I definitely want to write a driver for IDEA5003
sensor hub, but due to the lack of documentation and time, I put it
aside for the time being.

>  - 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 your information. I had done some tests with amd-debug-tools
before, so I knew the wakeup interrupt was from EC due to the wake-on-
human-presence feature. I've disabled it somehow with Lenovo BaiYing[1]
and it's persistent. I didn't mention these in the previous reply
because I thought these were just trivial details. But again, thanks
for your information.

[1]: I suppose writing 0 to
/sys/bus/acpi/devices/IDEA5003:00/physical_node/0018:048D:5003.0002/HID
-SENSOR-*/enable_sensor can achieve the same effect.

> Thanks for all the details
> Mark

Thanks,
Rong

Powered by blists - more mailing lists