Message-Id: <1acdc04a-e692-4ea6-8580-13f0b6d24f44@app.fastmail.com>
Date: Mon, 09 Feb 2026 13:44:19 -0500
From: "Mark Pearson" <mpearson-lenovo@...ebb.ca>
To: "Rong Zhang" <i@...g.moe>, "Hans de Goede" <hansg@...nel.org>,
 "Vishnu Sankar" <vishnuocv@...il.com>
Cc: "Henrique de Moraes Holschuh" <hmh@....eng.br>,
 "Derek J . Clark" <derekjohn.clark@...il.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 ibm-acpi-devel@...ts.sourceforge.net,
 "platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>,
 linux-kernel@...r.kernel.org, "Vishnu Sankar" <vsankar@...ovo.com>
Subject: Re: [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

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

Powered by blists - more mailing lists