Re: [PATCH RFC v3 00/11] leds: Add support for hardware-initiated hardware control trigger transition

[Lee Jones]

Why is this still an RFC?

> Some laptops can tune their keyboard backlight according to ambient
> light sensors (auto mode). This capability is essentially a hardware
> control trigger. Meanwhile, such laptops also offer a shrotcut for
> cycling through brightness levels and auto mode. For example, on
> ThinkBook, pressing Fn+Space ("shortcut") cycles keyboard backlight
> levels in the following sequence:
> 
>   1 => 2 => 0 => auto => 1 ...
> 
> Recent ThinkPad models should have similar sequence too.
> 
> However, there are some issues preventing us from using a private
> hardware control trigger:
> 
> 1. We want a mechanism to tell userspace which trigger is the hardware
>    control one, so that userspace can determine if auto mode is on/off,
>    as well as turing it on/off programmatically without obtaining the
>    trigger's name via other channels
> 2. Writing brightness has the side effect of disabling hardware control,
>    but the hardware control trigger remains active, resulting in the
>    software and hardware being out of sync. Most LED drivers that
>    supports hardware control also suffer from the same issue
> 3. Turing on/off auto mode via the shortcut cannot activate/deactivate
>    the corresponding hardware control trigger, making the software state
>    out of sync
> 4. Even with #3 solved, deactivating the hardware control trigger has
>    the side effect of emitting LED_OFF, breaking the shortcut cycle,
>    especially "auto => 1"
> 
> This RFC series tries to demonstrate a path on solving these issues:
> 
> - Introduce an attribute "trigger_may_offload", so that userspace can
>   determine:
>   - if the LED device supports hardware control (supported => visible)
>   - which trigger is the hardware control trigger selected by the LED
>     device
>   - if the trigger is selected ("<foo_trigger>")
>   - if the trigger is offloaded ("[foo_trigger]")
>     - A callback offloaded() is added so that LED triggers can report
>       their hardware control state
> - Remove hardware control trigger when writing brightness
> - Add led_trigger_notify_hw_control_changed() interface, so that LED
>   drivers can notify the LED core about hardware-initiated hardware
>   control transitions. The LED core will then determine if the
>   transition is allowed and switching between "none" (i.e., no trigger)
>   and the device's private trigger accordingly
>   - This capability is restricted to the device's private trigger. If
>     the current trigger is neither the private trigger nor "none", no
>     transition will be made
>   - This interface is gated behind Kconfig LEDS_TRIGGERS_HW_CHANGED and
>     LED device flag LED_TRIG_HW_CHANGED
> - Tune the logic of trigger deactivation so that it won't emit LED_OFF
>   when the deactivation is triggered by hardware
> 
> The last three patches are included in the RFC series to demonstrate how
> to these interfaces are supposed to be utilized, so that ideapad-laptop
> can expose the auto mode of ThinkBook's keyboard backlight. They can be
> submitted separately once the dust settles, if preferred.
> 
> [ Summary of other approaches ]
> 
> < custom attribute >
> 
> Pros:
> - simplicity, KISS
> - no need to touch the LED core
> - extensible as long as it has a sensor-neutral name
>   - a sensor-related name could potentially lead to a mess if a future
>     device implements auto mode based on multiple different sensors
> 
> Cons:
> - must have zero influence on brightness_set[_blocking] callbacks
>   in order not to break triggers
>   - potential interference with triggers and the brightness attribute,
>     can't solve #2
> - weird semantic (an attribute other than "brightness" and "trigger"
>   changes the brightness)
> 
> < private hardware control trigger (this series) >
> 
> Pros:
> - mutually exclusive with other triggers and the brightness attribute
>   (hence less chaos)
> - semantic correctness
> - acts as an aggregate switch to turn on/off auto mode even a future
>   device implements auto mode based on multiple different sensors
>   - extensibility (through trigger attributes)
> 
> Cons:
> - complexity
> 
> [ Previous discussion threads ]
> 
> https://lore.kernel.org/r/08580ec5-1d7b-4612-8a3f-75bc2f40aad2@xxxxxxxxxxxxxxxx
> https://lore.kernel.org/r/1dbfcf656cdb4af0299f90d7426d2ec7e2b8ac9e.camel@xxxxxxxx
> 
> Signed-off-by: Rong Zhang <i@xxxxxxxx>
> ---
> Changes in v3:
> - Integrate https://lore.kernel.org/all/20260712-leds-hw-control-brightness-set-v1-1-1de593b09d26@xxxxxxxx/
>   into the series
>   - Adopt __led_trigger_is_hw_controlled() in the rest of the series
> - Rearrange the series so that the code using the offloaded() callback is
>   introduced before the driver implementation (thanks Thomas Weißschuh)
> - Reword documentations and commit messages (ditto)
> - Adopt guard() and lockdep (ditto)
> - Address concerns from Sashiko
>   - Fix a race condition in ideapad_kbd_bl_led_cdev_brightness_set()
>   - Fix trigger re-registration of ideapad_kbd_bl_auto_trigger
>   - https://sashiko.dev/#/patchset/20260618-leds-trigger-hw-changed-v2-0-c28c44053cf3%40rong.moe
> - Make registration failures of ideapad_kbd_bl_auto_trigger non-fatal
> - Link to v2: https://patch.msgid.link/20260618-leds-trigger-hw-changed-v2-0-c28c44053cf3@xxxxxxxx
> 
> Changes in v2:
> - Restrict the led_trigger_notify_hw_control_changed() interface to
>   private triggers only
>   - Drop PATCH v1 1/9 ("leds: Load trigger modules on-demand if used as
>     hw control trigger"), not relavant any more
> - Gate the led_trigger_notify_hw_control_changed() interface behind
>   Kconfig LEDS_TRIGGERS_HW_CHANGED and LED device flag
>   LED_TRIG_HW_CHANGED
> - Fix lock ordering inversion
> - ideapad-laptop:
>   - Only call led_trigger_notify_hw_control_changed() when needed
>   - Serialize keyboard backlight notifications
> - Reword commit messages and documentations
> - Link to v1: https://patch.msgid.link/20260227190617.271388-1-i@xxxxxxxx
> 
> ---
> Rong Zhang (11):
>       leds: Move led_trigger_is_hw_controlled() to the right place
>       leds: class: Remove hardware control trigger when writing brightness
>       leds: trigger: Add offloaded() callback and provide trigger_may_offload attribute
>       leds: cros_ec: trigger: Implement offloaded() callback
>       leds: turris-omnia: trigger: Implement offloaded() and declare hw_control_trigger
>       leds: trigger: netdev: Implement offloaded() callback
>       leds: trigger: Enforce strict checks in led_trigger_is_hw_controlled()
>       leds: trigger: Add led_trigger_notify_hw_control_changed() interface
>       platform/x86: ideapad-laptop: Decouple hardware & classdev brightness for keyboard backlight
>       platform/x86: ideapad-laptop: Serialize keyboard backlight notifications
>       platform/x86: ideapad-laptop: Fully support auto keyboard backlight
> 
>  Documentation/ABI/testing/sysfs-class-led          |  25 ++
>  .../ABI/testing/sysfs-class-led-trigger-netdev     |   3 +
>  Documentation/leds/leds-class.rst                  |  72 ++++++
>  drivers/leds/led-class.c                           |  35 ++-
>  drivers/leds/led-triggers.c                        | 146 +++++++++++-
>  drivers/leds/leds-cros_ec.c                        |   6 +
>  drivers/leds/leds-turris-omnia.c                   |   7 +
>  drivers/leds/leds.h                                |   2 +
>  drivers/leds/trigger/Kconfig                       |   9 +
>  drivers/leds/trigger/ledtrig-netdev.c              |   8 +
>  drivers/platform/x86/lenovo/Kconfig                |   1 +
>  drivers/platform/x86/lenovo/ideapad-laptop.c       | 264 ++++++++++++++++-----
>  include/linux/leds.h                               |  19 ++
>  13 files changed, 532 insertions(+), 65 deletions(-)
> ---
> base-commit: 1229e2e57a5c2980ccd457b9b53ea0eed5a22ab3
> change-id: 20260506-leds-trigger-hw-changed-96a62188cbdf
> 
> Thanks,
> Rong
> 

-- 
Lee Jones