mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Qiling Zhu <kmatzkaczor244@gmail.com>
To: "Mark Pearson" <mpearson-lenovo@squebb.ca>,
	"Derek J. Clark" <derekjohn.clark@gmail.com>,
	"Rong Zhang" <i@rong.moe>, "Hans de Goede" <hansg@kernel.org>,
	"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
	"Henrique de Moraes Holschuh" <hmh@hmh.eng.br>
Cc: Griffin Kroah-Hartman <griffin@kroah.com>,
	ibm-acpi-devel@lists.sourceforge.net, Lee Jones <lee@kernel.org>,
	linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org,
	Pavel Machek <pavel@kernel.org>,
	platform-driver-x86@vger.kernel.org, vishnuocv@gmail.com,
	vsankar@lenovo.com, Qiling Zhu <kmatzkaczor244@gmail.com>
Subject: [RFC PATCH v2 0/2] Add support for auto keyboard backlight
Date: Mon,  2 Nov 2026 23:10:17 +0800	[thread overview]
Message-ID: <20261102151211.40701-1-kmatzkaczor244@gmail.com> (raw)
In-Reply-To: <20261101100123.28749-1-kmatzkaczor244@gmail.com>

Hi Mark, Rong, Ilpo,

Thanks for the review comments on v1. This v2 addresses them and is
now split into a pure refactor (1/2) and the feature addition (2/2),
as requested.

Summary of the changes:

* Kconfig: the "depends on LEDS_TRIGGERS_HW_CHANGED" is gone. While
  at it I also added a "select LEDS_TRIGGERS": without it the trigger
  helpers become no-op stubs and the whole feature would silently
  disappear in configurations that don't opt into LEDS_TRIGGERS,
  which seemed worse than a small select. HW_CHANGED support stays
  optional; without it the trigger still works, only the
  hardware-initiated transitions are not reported. The select goes
  beyond what was suggested, so please let me know if you'd rather
  drop it.

* The auto mode is never reported to the LED core as level 3: a
  single helper maps it to a brightness of 0, used by both
  brightness_get and the brightness_hw_changed notification, so
  level 3 has no path into the LED core at all. Internal state
  tracking compares raw EC levels.

* Writing a regular level through /proc/acpi/ibm/kbdlight now drops
  the trigger when leaving auto mode. Instead of raising the
  hardware-control notification and letting the worker handle it, I
  went through the same path brightness_store() uses: drop the
  trigger and hand the new level to the LED core, which applies it
  after the LED_OFF queued by dropping the trigger. That way proc
  and sysfs behave alike.

* suspend/resume no longer touch the LED while auto mode is active
  (v1 would have restored brightness 2 there and left the trigger
  state inconsistent, as Rong pointed out — thank you). The LED core
  is now kept out of suspend entirely in auto mode; resume just
  re-reads the EC level and reconciles. My reasoning is that in auto
  mode the EC owns the backlight, so the driver should not make
  decisions on its behalf — the firmware already turns the backlight
  off for suspend and restores it on resume by itself. This is based
  on observing my own machine (s2idle), so I may be wrong about
  other models. The non-auto paths are unchanged: suspend still
  turns the backlight off and resume still restores the previous
  level.

One thing Sashiko (the Linux Foundation's AI review system)
brought up in its review of v1, and which I then verified in the
code, seems worth raising here: led_trigger_hw_control_changed_worker()
reads the transition flag before acquiring the LED's trigger_lock,
and the read and the transition are not atomic. If, while a worker
carrying "deactivate" is still on its way, userspace re-selects the
private trigger, the worker then drops the trigger that userspace
just set, and no further event arrives to reconcile the state.
I haven't reliably reproduced this, but the window looks real, and
the ideapad wiring seems equally exposed. My feeling is that this
would be better addressed in the trigger core than worked around in
each driver, but I may be missing something — Rong, I'd appreciate
your thoughts.

A few observations that may be useful for the discussion:

* I disassembled the DSDT and instrumented the driver to see what
  MLCS returns: it is the EC status word, and bit 31 looks like an
  error flag — the DSDT skips the hotkey event when it is set. On my
  machine it reads 0 for all levels, with no level echo, and the
  error path is not reachable. That is why activation verifies with a
  read-back rather than the return value.

* The 0x1012 event is raised by the DSDT itself: MLCS issues it when
  the write succeeded, the level actually changed, and the hotkey is
  not masked. This explains why driver-initiated writes also see the
  event and why rewriting the same level doesn't.

* The Fn+Space cycle order on this machine is 0 -> 3 -> 1 -> 2 -> 0,
  confirmed by testing; sharing in case it is useful for comparison
  across models. Nothing in the code or tests relies on it.

An open question from v1 that I'm still unsure about: on models
without an ALS the auto mode presumably doesn't exist — how should
the driver handle that? Mark, do you know what the EC does when
asked for level 3 on such machines, and whether there is a reliable
way to tell at probe time whether auto mode is supported? For now
the trigger is registered unconditionally on keyboard-backlight-
capable machines.

Vishnu, Rong mentioned in the v1 discussion that you are also working
on the same support using the hw control trigger framework — how is
that going? If you have answers to the questions above, or if there
is a chance to combine the two efforts, I'd be happy to coordinate.

Tested on the same ThinkPad X1 Carbon Gen 14 (linux-next
next-20261030) with targeted tests and on-hardware verification of
the affected paths, including suspend/resume; checkpatch --strict
clean.

Regards,
Qiling Zhu

This patch was developed with LLM assistance; I reviewed every design
decision against the framework sources and validated the behavior on
real hardware myself.

Qiling Zhu (2):
  platform/x86: thinkpad_acpi: Extract kbdlight_notify_change() helper
  platform/x86: thinkpad_acpi: Add support for auto keyboard backlight

 drivers/platform/x86/lenovo/Kconfig         |   1 +
 drivers/platform/x86/lenovo/thinkpad_acpi.c | 179 +++++++++++++++++---
 2 files changed, 159 insertions(+), 21 deletions(-)

-- 
2.56.0


  parent reply	other threads:[~2026-11-02 15:13 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-11-01 10:01 [RFC PATCH 0/1] platform/x86: thinkpad_acpi: " Qiling Zhu
2026-11-01 10:01 ` [RFC PATCH 1/1] " Qiling Zhu
2026-11-01 18:24   ` Rong Zhang
2026-11-01 17:57 ` [RFC PATCH 0/1] " Rong Zhang
2026-11-02 15:10 ` Qiling Zhu [this message]
2026-11-02 15:10   ` [RFC PATCH v2 1/2] platform/x86: thinkpad_acpi: Extract kbdlight_notify_change() helper Qiling Zhu
2026-11-02 15:10   ` [RFC PATCH v2 2/2] platform/x86: thinkpad_acpi: Add support for auto keyboard backlight Qiling Zhu
2026-11-02 18:32     ` Rong Zhang
2026-11-02 18:13   ` [RFC PATCH v2 0/2] " Rong Zhang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20261102151211.40701-1-kmatzkaczor244@gmail.com \
    --to=kmatzkaczor244@gmail.com \
    --cc=derekjohn.clark@gmail.com \
    --cc=griffin@kroah.com \
    --cc=hansg@kernel.org \
    --cc=hmh@hmh.eng.br \
    --cc=i@rong.moe \
    --cc=ibm-acpi-devel@lists.sourceforge.net \
    --cc=ilpo.jarvinen@linux.intel.com \
    --cc=lee@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=mpearson-lenovo@squebb.ca \
    --cc=pavel@kernel.org \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=vishnuocv@gmail.com \
    --cc=vsankar@lenovo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®