* Re: [PATCH] Input: applespi - turn the keyboard backlight off across suspend
2026-10-01 8:41 [PATCH] Input: applespi - turn the keyboard backlight off across suspend Michal Szpakowski
@ 2026-10-01 8:56 ` sashiko-bot
2026-10-01 9:56 ` [PATCH v2] " Michal Szpakowski
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-10-01 8:56 UTC (permalink / raw)
To: Michal Szpakowski; +Cc: dmitry.torokhov, linux-input
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] input: applespi: backlight level saved as 0 during hibernation
- [Medium] input: applespi: caps-lock LED fails to turn off during suspend
--
commit 37ebd40953a1312d5d67af7477063eae746c320a
Author: Michal Szpakowski <michi.szpakowski@gmail.com>
Input: applespi - turn the keyboard backlight off across suspend
The patch sets the LED_CORE_SUSPENDRESUME flag for the keyboard backlight,
allowing the LED core to blank it during suspend and restore it on resume.
This resolves an issue where the backlight stayed lit through s2idle.
Fixes: 038b1a05eae6 ("Input: add Apple SPI keyboard and trackpad driver")
Cc: stable@vger.kernel.org
> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index b5ff71cd5a701..838b380bde6e1 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -1772,6 +1772,7 @@ static int applespi_probe(struct spi_device *spi)
> applespi->backlight_info.name = "spi::kbd_backlight";
> applespi->backlight_info.default_trigger = "kbd-backlight";
> applespi->backlight_info.brightness_set = applespi_set_bl_level;
> + applespi->backlight_info.flags = LED_CORE_SUSPENDRESUME;
[Severity: Medium]
During hibernation, the PM core calls led_suspend(), which updates
applespi->have_bl_level to 0. Does this cause the user's preferred backlight
level to be permanently overwritten with 0 when applespi_poweroff_late()
later saves the state?
For example, in drivers/input/keyboard/applespi.c:
applespi_poweroff_late() {
...
applespi_save_bl_level(applespi, applespi->have_bl_level);
}
[Severity: Medium]
Could adding this flag also block applespi_suspend() from successfully turning
off the caps-lock LED?
If the LED core initiates an asynchronous backlight SPI write to dim the
keyboard before suspend, applespi->cmd_msg_queued will be set.
When applespi_suspend() then calls applespi_set_capsl_led(), the call to
applespi_send_cmd_msg() returns early without queueing the caps-lock command
because the backlight command is still in flight.
applespi_suspend() then calls applespi_drain_writes(), which sets
applespi->drain = true. When the backlight write eventually completes, the
pending caps-lock off command is permanently aborted because the drain flag
is set, potentially leaving the caps-lock LED in an incorrect state.
>
> sts = devm_led_classdev_register(&spi->dev, &applespi->backlight_info);
> if (sts)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261001084154.4703-1-michi.szpakowski@gmail.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH v2] Input: applespi - turn the keyboard backlight off across suspend
2026-10-01 8:41 [PATCH] Input: applespi - turn the keyboard backlight off across suspend Michal Szpakowski
2026-10-01 8:56 ` sashiko-bot
@ 2026-10-01 9:56 ` Michal Szpakowski
1 sibling, 0 replies; 3+ messages in thread
From: Michal Szpakowski @ 2026-10-01 9:56 UTC (permalink / raw)
To: linux-input, Dmitry Torokhov
Cc: Ronald Tschalär, Aditya Garg, Lukas Wunner, stable
The keyboard backlight of the SPI topcase stays lit through s2idle: the
driver's suspend handler only switches the caps-lock LED off, and the
backlight LED is registered without LED_CORE_SUSPENDRESUME, so the LED
core never gets to blank it. Under deep (S3) the rails go down anyway,
which is why it only shows on machines that default to s2idle.
Set the flag, and deal with the two things that follow from the LED
core now sending a command of its own right before applespi_suspend():
- The LED is a child of the SPI device, so its "off" command is queued
just before applespi_suspend() runs. The driver keeps one command in
flight, so the caps-lock command was skipped while that one was still
out, and then dropped by the drain. Wait for the write to finish first;
the completion now wakes the drain queue unconditionally so that this
wait has something to wake it.
- The level the LED core blanks to would be what poweroff_late and
shutdown save to EFI for the next boot. Keep the last level the user
asked for in its own field, updated only while the LED is not
suspended, and save that instead.
Tested on a MacBookPro13,1 (2016): 30 s of s2idle with the backlight at
200/255 leaves it lit on the stock kernel; with this patch it goes dark
at suspend and is back at 200 on wake, and a caps-lock LED that was on
at suspend is off during the sleep and on again afterwards. A
hibernation with the backlight at 200 leaves the level for 200 in the
EFI variable instead of the blanked minimum.
Fixes: 038b1a05eae6 ("Input: add Apple SPI keyboard and trackpad driver")
Cc: stable@vger.kernel.org
Signed-off-by: Michal Szpakowski <michi.szpakowski@gmail.com>
---
v2: address the two points from the automated review on v1 - the
caps-lock command racing the LED core's backlight command at
suspend, and the blanked level being saved to EFI at power-off.
drivers/input/keyboard/applespi.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -407,6 +407,7 @@
bool have_cl_led_on;
unsigned int want_bl_level;
unsigned int have_bl_level;
+ unsigned int bl_level_persist;
unsigned int cmd_msg_cntr;
/* lock to protect the above parameters and flags below */
spinlock_t cmd_msg_lock;
@@ -724,7 +725,7 @@
if (is_write_msg)
applespi->write_active = false;
- if (applespi->drain && !applespi->write_active)
+ if (!applespi->write_active)
wake_up_all(&applespi->drain_complete);
if (is_write_msg) {
@@ -923,6 +924,14 @@
KBD_BL_LEVEL_MIN);
}
+ /*
+ * The LED core blanks the backlight around suspend; that level is
+ * not what the user wants restored after a power-off, so keep the
+ * last one they asked for separately.
+ */
+ if (!(led_cdev->flags & LED_SUSPENDED))
+ applespi->bl_level_persist = applespi->want_bl_level;
+
applespi_send_cmd_msg(applespi);
}
@@ -1772,6 +1781,7 @@
applespi->backlight_info.name = "spi::kbd_backlight";
applespi->backlight_info.default_trigger = "kbd-backlight";
applespi->backlight_info.brightness_set = applespi_set_bl_level;
+ applespi->backlight_info.flags = LED_CORE_SUSPENDRESUME;
sts = devm_led_classdev_register(&spi->dev, &applespi->backlight_info);
if (sts)
@@ -1791,6 +1801,14 @@
return 0;
}
+static void applespi_wait_for_write(struct applespi_data *applespi)
+{
+ guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
+
+ wait_event_lock_irq(applespi->drain_complete, !applespi->write_active,
+ applespi->cmd_msg_lock);
+}
+
static void applespi_drain_writes(struct applespi_data *applespi)
{
guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
@@ -1829,7 +1847,7 @@
{
struct applespi_data *applespi = spi_get_drvdata(spi);
- applespi_save_bl_level(applespi, applespi->have_bl_level);
+ applespi_save_bl_level(applespi, applespi->bl_level_persist);
}
static int applespi_poweroff_late(struct device *dev)
@@ -1837,7 +1855,7 @@
struct spi_device *spi = to_spi_device(dev);
struct applespi_data *applespi = spi_get_drvdata(spi);
- applespi_save_bl_level(applespi, applespi->have_bl_level);
+ applespi_save_bl_level(applespi, applespi->bl_level_persist);
return 0;
}
@@ -1849,6 +1867,14 @@
acpi_status acpi_sts;
int sts;
+ /*
+ * The backlight LED is a child of this device, so the LED core has
+ * just sent its "off" command; wait for it, or the caps-lock command
+ * is skipped (one command in flight at a time) and then dropped by the
+ * drain below.
+ */
+ applespi_wait_for_write(applespi);
+
/* turn off caps-lock - it'll stay on otherwise */
sts = applespi_set_capsl_led(applespi, false);
if (sts)
^ permalink raw reply [flat|nested] 3+ messages in thread