Re: [PATCH v5 2/3] platform: arm64: Add Lenovo ThinkPad X13s EC driver

[Stephan Gerhold]

On Fri, Oct 02, 2026 at 03:43:07PM +0000, Alex Robinson wrote:
> Add support for the ThinkPad X13s embedded controller to expose keyboard
> backlight control and firmware-driven brightness changes when booting
> with Device Tree. Its backlight, event and power-management interfaces
> differ from the T14s EC and require separate handling.
> 
> Preserve brightness across lid and system-sleep transitions using the
> sequences described by the X13s ACPI firmware. Keep temporary suspend
> blanking from overwriting the EC's saved lid-restore brightness.
> 
> Report firmware-driven brightness changes such as Fn+Space, but do not
> report lid-open restoration as a new brightness change, avoiding a
> spurious keyboard-backlight on-screen display.
> 
> Leave EC wakeup disabled by default because the shared interrupt can
> wake the system on lid closure and no selective event mask is known.
> 
> This follows earlier X13s EC work by Konrad Dybcio and Steev Klimaszewski.
> 
> Assisted-by: LLM
> Signed-off-by: Alex Robinson <alex@xxxxxxxxxxxxx>
> 

I'm mostly just curious, is the input/hotkey functionality in the T14s
EC driver not relevant for the X13s or did you omit it because you don't
need it and/or to keep the initial driver more simple? It seems to be
present in the X13s ACPI similar to the T14s.

> [...]
> diff --git a/drivers/platform/arm64/lenovo-thinkpad-x13s.c b/drivers/platform/arm64/lenovo-thinkpad-x13s.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..f1ae4c8e67159ebe04ec7aa6521b0ac9ad07a97c
> --- /dev/null
> +++ b/drivers/platform/arm64/lenovo-thinkpad-x13s.c
> @@ -0,0 +1,408 @@
> [...]
> +static int x13s_power_gpio(struct x13s_ec *ec, int value)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&ec->lock);
> +	ret = gpiod_set_value_cansleep(ec->power_state, value);
> +	if (ret) {
> +		dev_err(&ec->client->dev, "Failed to set power-state GPIO to %d: %d\n",
> +			value, ret);
> +	}
> +	return ret;
> +}
> +
> +static void x13s_exit_low_power(struct x13s_ec *ec)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&ec->lock);
> +	x13s_power_gpio(ec, 1);
> +	/* DSDT: GPIO176 high, Sleep(10), then register 0x80 <- 0xaa. */
> +	fsleep(10000);
> +	ret = x13s_write(ec, X13S_EC_REG_POWER_STATE, X13S_EC_POWER_STATE_EXIT);
> +	if (ret)
> +		dev_err(&ec->client->dev, "Failed to exit low power: %d\n", ret);

AFAICT the ACPI implementation has a retry loop here with 3 retries,
similar to t14s_ec_write_sequence() in the T14s EC driver
(\_SB.I2C9.ECWS). Did you omit that on purpose?

> +}
> +
> +static int x13s_suspend(struct device *dev)
> +{
> +	struct x13s_ec *ec = dev_get_drvdata(dev);
> +	int ret;
> +
> +	/* The IRQ handler takes lock, so disable it before taking the mutex. */
> +	disable_irq(ec->client->irq);
> +
> +	/* Capture hardware brightness after completing queued LED updates. */
> +	flush_work(&ec->led.set_brightness_work);
> +	ret = led_update_brightness(&ec->led);
> +	if (ret)
> +		dev_err(dev, "Failed to save backlight brightness: %d\n", ret);
> +
> +	mutex_lock(&ec->lock);
> +	ec->preserve_saved_brightness = true;
> +	mutex_unlock(&ec->lock);
> +
> +	/* This flushes brightness work, which also takes lock. */
> +	led_classdev_suspend(&ec->led);
> +
> +	guard(mutex)(&ec->lock);
> +	/* DSDT: register 0x80 <- 0x55, then GPIO176 low. */
> +	ret = x13s_write(ec, X13S_EC_REG_POWER_STATE, X13S_EC_POWER_STATE_ENTER);

Same here.

Thanks,
Stephan