Message-ID: <a90584179f4c90cd58c03051280a6dda63f6cc1d.camel@rong.moe>
Date: Thu, 07 Aug 2025 22:50:37 +0800
From: Rong Zhang <i@...g.moe>
To: 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"	
 <platform-driver-x86@...r.kernel.org>, linux-kernel@...r.kernel.org, Lee
 Jones	 <lee@...nel.org>, Pavel Machek <pavel@...nel.org>,
 linux-leds@...r.kernel.org
Subject: Re: [PATCH 2/2] platform/x86: ideapad-laptop: Fully support auto
 kbd backlight

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

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

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

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.

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

> Mark

Thanks,
Rong

Powered by blists - more mailing lists