Re: [PATCH] thinkpad_acpi: Add Auto mode support with dynamic max_brightness

[Hans de Goede <hansg@kernel.org>]

Hi all,

On 9-Feb-26 19:44, Mark Pearson wrote:

...

> 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

There is no need for this, the mere presence of
the "auto_brightness_enabled" sysfs attribute (which can
be in a sysfs-attr-group with an is_visible callback)
is enough to indicate that the backlight is auto
brightness capable.

>  - auto_brightness_enabled - indicates if the LED is in the auto_brightness controlled state or not.

This is for auto-brightness based on an ambient light sensor
(ALS), right ?

My vote would go to use "als_enabled", just like is already done in:

Documentation/ABI/testing/sysfs-platform-dell-laptop

adding new sysfs attributes to a LED class device although
possible is a bit frowned upon though.

In that sense using a trigger is better because it more
closely matches how the LED class API is supposed to be
used would maybe be better.

So I've gone and re-read Rong's trigger proposal:

https://lore.kernel.org/all/a90584179f4c90cd58c03051280a6dda63f6cc1d.camel@rong.moe/

Rong, previously you also went a bit further with implementing this
already which you described here:

https://lore.kernel.org/all/8a132e7473655ca0119af10339c63beb4df7c201.camel@rong.moe/

One of the problems you encountered there is what to do if
the user actually set a trigger themselves and the EC moves
between fixed-brightness-value <-> ALS .

My first idea was to just always override the trigger with
the special ALS trigger or none.

But thinking more about this this is wrong. E.g. there
are triggers which turn the backlight on when user input
is detected and then off after a while, which would be
a perfect reasonable thing to use together with a kbd-backlight.

Thinking more about this triggers are typically for deciding
when to turn the LED on/off not for controlling brightness
many of them actually allow still writing the brightness
sysfs attr and then when the LED should be on according to
that trigger, the trigger use the last written brightness.

Looking at things this way ALS is not really a trigger, it
is more of a brightness control mechanism.

So I think the best and also KISS solution here would be
to go with adding a "als_enabled" sysfs attr to the
LED class device, which is only visible when support,
just like is already done in:

Documentation/ABI/testing/sysfs-platform-dell-laptop

I would also call led_classdev_notify_brightness_hw_changed() 
when the EC moves between fixed-brightness-value <-> ALS.

Userspace will likely already have a poll() going on on
the brightness_hw_changed sysfs attr, so this way userspace
which is aware of the als_enabled sysfs attr can also check
that.

You can then report brightness_max as the new value when calling
led_classdev_notify_brightness_hw_changed() since the ALS can go
up to brightness_max, likewise you could also always return
brightness_max when reading the brightness value while in ALS mode.

The only real question left then is what to do on brightness
writes. I would do the same as what triggers do here, ignore
writing non 0 values and turn off the backlight (and thus also
ALS) when 0 is written.

Note as for actually allowing "auto" for the brightness value
(read/write) that would break userspace assumptions that that
file always contains an integer, so that is not an option IMHO.

Regards,

Hans