Re: [PATCH 3/3] arm64: dts: qcom: sc8280xp-x13s: Add embedded controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dts: missing pinctrl configuration for i2c8 bus
- [Low] dts: i2c8 node is inserted out of numerical order
- [Low] dts: pinctrl nodes in tlmm are out of alphabetical order
--

commit e4a47547cc8af70376ce1a7b8590d0f5c4ae2ab8
Author: Clover <alex@ironrobin.net>

arm64: dts: qcom: sc8280xp-x13s: Add embedded controller

This commit enables the embedded controller at address 0x28 on the i2c8 bus
for the Lenovo ThinkPad X13s. It configures the falling-edge event interrupt
on GPIO103 and the active-high power-state output on GPIO176.

> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> index 218573a97785b..e0eb415ef9b42 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
[ ... ]
> @@ -885,6 +885,21 @@ vreg_l7q: ldo7 {
>  	};
>  };
>  
> +&i2c8 {

[Severity: Low]
Does this i2c8 node violate the standard convention of sorting referenced
bus nodes numerically? It appears to be inserted after &i2c11 and before
&i2c21.

> +	clock-frequency = <400000>;
> +	status = "okay";

[Severity: Medium]
Will enabling the &i2c8 bus without specifying pinctrl-names and pinctrl-0
cause silent I2C communication failures? It looks like omitting explicit
multiplexing for the SDA and SCL pins means the bus will rely on bootloader
leftover state, which is fragile if the pins are reset or misconfigured by
power management.

> +
> +	embedded-controller@28 {
[ ... ]
> @@ -1540,6 +1555,21 @@ hastings_reg_en: hastings-reg-en-state {
>  &tlmm {
>  	gpio-reserved-ranges = <70 2>, <74 6>, <125 2>, <128 2>, <154 4>;
>  
> +	ec_power_state: ec-power-state {

[Severity: Low]
Are the ec_power_state and ec_int_n child nodes inserted out of alphabetical
order within the &tlmm block? They are placed at the top before bt_default,
and ec_power_state is placed before ec_int_n.

> +		pins = "gpio176";
> +		function = "gpio";
> +		drive-strength = <2>;


I also noticed a few issues in the corresponding platform driver code in
drivers/platform/arm64/lenovo-thinkpad-x13s.c:

drivers/platform/arm64/lenovo-thinkpad-x13s.c:x13s_probe() {
	...
	ec->power_state = devm_gpiod_get(dev, "power-state", GPIOD_OUT_HIGH);
	if (IS_ERR(ec->power_state))
		return dev_err_probe(dev, PTR_ERR(ec->power_state),
				     "Failed to acquire power-state GPIO high\n");

	ec->led.name = "platform::kbd_backlight";
	ec->led.max_brightness = 2;
	ec->led.flags = LED_BRIGHT_HW_CHANGED | LED_RETAIN_AT_SHUTDOWN;
	ec->led.brightness_get = x13s_brightness_get;
	ec->led.brightness_set_blocking = x13s_brightness_set;
	ret = devm_led_classdev_register(dev, &ec->led);
	...
}

[Severity: Medium]
This is a pre-existing issue, but does x13s_probe() skip the mandatory 10ms
settling delay and the 0xaa I2C exit command after driving the power-state
GPIO high? It looks like devm_led_classdev_register() synchronously queries
the backlight state over I2C immediately after, which could fail or timeout
if the EC is still in a low-power state.

drivers/platform/arm64/lenovo-thinkpad-x13s.c:x13s_suspend() {
	...
	/* Capture hardware state before firmware can reset the backlight. */
	ret = x13s_brightness(ec);
	if (ret < 0) {
		dev_err(dev, "Failed to save backlight brightness: %d\n", ret);
		goto out;
	}
	...
}

[Severity: Medium]
This isn't a bug introduced by this patch, but does returning this
non-critical I2C read failure unconditionally abort the entire system suspend?
If a transient bus error occurs while reading the keyboard backlight
brightness, the power management core will abort the transition and the
laptop will fail to sleep.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925215358.33417-1-alex@ironrobin.net?part=3