linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] platform: arm64: thinkpad-t14s-ec: new driver
@ 2025-08-31 21:28 Sebastian Reichel
  2025-08-31 21:28 ` [PATCH 1/3] dt-bindings: platform: Add Lenovo Thinkpad T14s EC Sebastian Reichel
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Sebastian Reichel @ 2025-08-31 21:28 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
	Bjorn Andersson, Konrad Dybcio, Mark Pearson
  Cc: Derek J. Clark, Henrique de Moraes Holschuh, devicetree,
	linux-kernel, platform-driver-x86, linux-arm-msm

Introduce driver for the ThinkPad T14s Gen6 Snapdragon EC. In theory
it seems to be compatible with the ThinkPad ACPI driver, but these
devices are booted with device tree. As the name implies, the existing
ThinkPad ACPI driver only supports the ACPI interface. Looking at
the implementation, the ACPI DSDT contains many mapping functions
to translate the low level I2C messages into the interface used by
the ThinkPad ACPI driver. Adding DT support to the ThinkPad ACPI driver
would require adding all those translation functions, which would add
more or less the same amount of code as writing a separate driver using
the low level interface directly. I don't think it's sensible to make
the existing ACPI driver even more complicated, so I went for a separate
driver.

I managed to get system LEDs, audio LEDs, extra keys and the keyboard
backlight control working. The EC also seems to be used for some thermal
bits, which I haven't looked into deeply. As far as I understand most
thermal and fan control is handled by a different controller
(0x36@i2c5) anyways.

Apart from that the EC is involved in proper system suspend, which
is something I do not yet understand (I don't have any documentation
apart from the dis-assembled DSDT and existing ACPI driver). Right
now I disabled wake capabilities for the IRQ, since it would wake
up the system when closing the LID. Hopefully a way to mask specific
events will be found in the future.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
Sebastian Reichel (3):
      dt-bindings: platform: Add Lenovo Thinkpad T14s EC
      platform: arm64: thinkpad-t14s-ec: new driver
      arm64: dts: qcom: x1e80100-t14s: add EC

 .../bindings/platform/lenovo,thinkpad-t14s-ec.yaml |  49 ++
 MAINTAINERS                                        |   6 +
 .../dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi    |  23 +
 drivers/platform/arm64/Kconfig                     |  20 +
 drivers/platform/arm64/Makefile                    |   1 +
 drivers/platform/arm64/lenovo-thinkpad-t14s.c      | 597 +++++++++++++++++++++
 6 files changed, 696 insertions(+)
---
base-commit: c8bc81a52d5a2ac2e4b257ae123677cf94112755
change-id: 20250831-thinkpad-t14s-ec-ddeb23dbdafb

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/3] dt-bindings: platform: Add Lenovo Thinkpad T14s EC
  2025-08-31 21:28 [PATCH 0/3] platform: arm64: thinkpad-t14s-ec: new driver Sebastian Reichel
@ 2025-08-31 21:28 ` Sebastian Reichel
  2025-08-31 23:53   ` Bryan O'Donoghue
  2025-09-01  9:28   ` Krzysztof Kozlowski
  2025-08-31 21:28 ` [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver Sebastian Reichel
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 18+ messages in thread
From: Sebastian Reichel @ 2025-08-31 21:28 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
	Bjorn Andersson, Konrad Dybcio, Mark Pearson
  Cc: Derek J. Clark, Henrique de Moraes Holschuh, devicetree,
	linux-kernel, platform-driver-x86, linux-arm-msm

Add binding for the EC found in the Thinkpad T14s Gen6 Snapdragon,
which is based on the Qualcomm X1 Elite. Some of the system LEDs
and extra keys are only accessible via the EC.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 .../bindings/platform/lenovo,thinkpad-t14s-ec.yaml | 49 ++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml b/Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..bab20df2d9ede9a3cb0359944b26b3d18ff7d9b8
--- /dev/null
+++ b/Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/platform/lenovo,thinkpad-t14s-ec.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Lenovo Thinkpad T14s Embedded Controller.
+
+maintainers:
+  - Sebastian Reichel <sre@kernel.org>
+
+description:
+  The Qualcomm Snapdragon-based Lenovo Thinkpad T14s has an Embedded Controller
+  (EC) which handles things such as keyboard backlight, LEDs or non-standard keys.
+  This binding describes the interface, on an I2C bus, to this EC.
+
+properties:
+  compatible:
+    const: lenovo,thinkpad-t14s-ec
+
+  reg:
+    const: 0x28
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |+
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c1 {
+        clock-frequency = <400000>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        embedded-controller@28 {
+            compatible = "lenovo,thinkpad-t14s-ec";
+            reg = <0x28>;
+            interrupts-extended = <&tlmm 66 IRQ_TYPE_LEVEL_LOW>;
+        };
+    };
+...

-- 
2.50.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver
  2025-08-31 21:28 [PATCH 0/3] platform: arm64: thinkpad-t14s-ec: new driver Sebastian Reichel
  2025-08-31 21:28 ` [PATCH 1/3] dt-bindings: platform: Add Lenovo Thinkpad T14s EC Sebastian Reichel
@ 2025-08-31 21:28 ` Sebastian Reichel
  2025-09-01  8:43   ` Bryan O'Donoghue
                     ` (2 more replies)
  2025-08-31 21:28 ` [PATCH 3/3] arm64: dts: qcom: x1e80100-t14s: add EC Sebastian Reichel
                   ` (2 subsequent siblings)
  4 siblings, 3 replies; 18+ messages in thread
From: Sebastian Reichel @ 2025-08-31 21:28 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
	Bjorn Andersson, Konrad Dybcio, Mark Pearson
  Cc: Derek J. Clark, Henrique de Moraes Holschuh, devicetree,
	linux-kernel, platform-driver-x86, linux-arm-msm

Introduce EC driver for the ThinkPad T14s Gen6 Snapdragon, which
is in theory compatible with ThinkPad ACPI. On Linux the system
is booted with device tree, which is not supported by the ThinkPad
ACPI driver. Also most of the hardware compatibility is handled
via ACPI tables, which are obviously not used when booting via
device tree. Thus adding DT compatibility to the existing driver
is not worth it (almost no code sharing).

The driver currently exposes features, which are not available
via other means:

 * Extra Keys
 * System LEDs
 * Keyboard Backlight Control

The driver has been developed by reading the ACPI DSDT. There
are some more features around thermal control, which are not
yet supported by the driver.

The speaker mute and mic mute LEDs need some additional changes
in the ALSA UCM to be set automatically.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 MAINTAINERS                                   |   6 +
 drivers/platform/arm64/Kconfig                |  20 +
 drivers/platform/arm64/Makefile               |   1 +
 drivers/platform/arm64/lenovo-thinkpad-t14s.c | 597 ++++++++++++++++++++++++++
 4 files changed, 624 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e94d68c980c5f8bef2e1caf26b1a775df6aa1d84..589466169c222b2e088c6112a1e724c95e948f72 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25092,6 +25092,12 @@ W:	http://thinkwiki.org/wiki/Ibm-acpi
 T:	git git://repo.or.cz/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git
 F:	drivers/platform/x86/lenovo/thinkpad_acpi.c
 
+THINKPAD T14S EMBEDDED CONTROLLER DRIVER
+M:	Sebastian Reichel <sre@kernel.org>
+S:	Maintained
+F:	Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml
+F:	drivers/platform/arm64/lenovo-thinkpad-t14s.c
+
 THINKPAD LMI DRIVER
 M:	Mark Pearson <mpearson-lenovo@squebb.ca>
 L:	platform-driver-x86@vger.kernel.org
diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
index 06288aebc5599435065a37f8dacd046b5def80bd..10f905d7d6bfa5fad30a0689d3a20481268c781e 100644
--- a/drivers/platform/arm64/Kconfig
+++ b/drivers/platform/arm64/Kconfig
@@ -70,4 +70,24 @@ config EC_LENOVO_YOGA_C630
 
 	  Say M or Y here to include this support.
 
+config EC_LENOVO_THINKPAD_T14S
+	tristate "Lenovo Thinkpad T14s Embedded Controller driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on I2C
+	depends on INPUT
+	select INPUT_SPARSEKMAP
+	select LEDS_CLASS
+	select NEW_LEDS
+	select SND_CTL_LED if SND
+	help
+	  Driver for the Embedded Controller in the Qualcomm Snapdragon-based
+	  Lenovo Thinkpad T14s, which provides access to keyboard backlight
+	  and status LEDs.
+
+	  This driver provides support for the mentioned laptop where this
+	  information is not properly exposed via the standard Qualcomm
+	  devices.
+
+	  Say M or Y here to include this support.
+
 endif # ARM64_PLATFORM_DEVICES
diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
index 46a99eba3264cc40e812567d1533bb86031a6af3..60c131cff6a15bb51a49c9edab95badf513ee0f6 100644
--- a/drivers/platform/arm64/Makefile
+++ b/drivers/platform/arm64/Makefile
@@ -8,3 +8,4 @@
 obj-$(CONFIG_EC_ACER_ASPIRE1)	+= acer-aspire1-ec.o
 obj-$(CONFIG_EC_HUAWEI_GAOKUN)	+= huawei-gaokun-ec.o
 obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o
+obj-$(CONFIG_EC_LENOVO_THINKPAD_T14S) += lenovo-thinkpad-t14s.o
diff --git a/drivers/platform/arm64/lenovo-thinkpad-t14s.c b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
new file mode 100644
index 0000000000000000000000000000000000000000..ab783870e8eadfe13d83500c7f39440291e42cc9
--- /dev/null
+++ b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
@@ -0,0 +1,597 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2025, Sebastian Reichel
+ */
+
+#define DEBUG
+
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
+#include <linux/leds.h>
+#include <linux/lockdep.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define THINKPAD_T14S_EC_CMD_ECRD 0x02
+#define THINKPAD_T14S_EC_CMD_ECWR 0x03
+#define THINKPAD_T14S_EC_CMD_EVT 0xf0
+
+#define THINKPAD_T14S_EC_REG_LED	0x0c
+#define THINKPAD_T14S_EC_REG_KBD_BL1	0x0d
+#define THINKPAD_T14S_EC_REG_KBD_BL2	0xe1
+#define THINKPAD_T14S_EC_KBD_BL1_MASK	GENMASK_U8(7, 6)
+#define THINKPAD_T14S_EC_KBD_BL1_SHIFT	6
+#define THINKPAD_T14S_EC_KBD_BL2_MASK	GENMASK_U8(3, 2)
+#define THINKPAD_T14S_EC_KBD_BL2_SHIFT	2
+#define THINKPAD_T14S_EC_REG_AUD	0x30
+#define THINKPAD_T14S_EC_MIC_MUTE_LED	BIT(5)
+#define THINKPAD_T14S_EC_SPK_MUTE_LED	BIT(6)
+
+#define THINKPAD_T14S_EC_EVT_NONE			0x00
+#define THINKPAD_T14S_EC_EVT_KEY_FN_4			0x13
+#define THINKPAD_T14S_EC_EVT_KEY_FN_F7			0x16
+#define THINKPAD_T14S_EC_EVT_KEY_FN_SPACE		0x1F
+#define THINKPAD_T14S_EC_EVT_KEY_TP_DOUBLE_TAP		0x20
+#define THINKPAD_T14S_EC_EVT_AC_CONNECTED		0x26
+#define THINKPAD_T14S_EC_EVT_AC_DISCONNECTED		0x27
+#define THINKPAD_T14S_EC_EVT_KEY_POWER			0x28
+#define THINKPAD_T14S_EC_EVT_LID_OPEN			0x2A
+#define THINKPAD_T14S_EC_EVT_LID_CLOSED			0x2B
+#define THINKPAD_T14S_EC_EVT_KEY_FN_F12			0x62
+#define THINKPAD_T14S_EC_EVT_KEY_FN_TAB			0x63
+#define THINKPAD_T14S_EC_EVT_KEY_FN_F8			0x64
+#define THINKPAD_T14S_EC_EVT_KEY_FN_F10			0x65
+#define THINKPAD_T14S_EC_EVT_KEY_FN_F4			0x6A
+#define THINKPAD_T14S_EC_EVT_KEY_FN_D			0x6B
+#define THINKPAD_T14S_EC_EVT_KEY_FN_T			0x6C
+#define THINKPAD_T14S_EC_EVT_KEY_FN_H			0x6D
+#define THINKPAD_T14S_EC_EVT_KEY_FN_M			0x6E
+#define THINKPAD_T14S_EC_EVT_KEY_FN_L			0x6F
+#define THINKPAD_T14S_EC_EVT_KEY_FN_RIGHT_SHIFT		0x71
+#define THINKPAD_T14S_EC_EVT_KEY_FN_ESC			0x74
+#define THINKPAD_T14S_EC_EVT_KEY_FN_N			0x79
+#define THINKPAD_T14S_EC_EVT_KEY_FN_F11			0x7A
+#define THINKPAD_T14S_EC_EVT_KEY_FN_G			0x7E
+
+enum thinkpad_t14s_ec_led_status_t {
+	THINKPAD_EC_LED_OFF = 0x00,
+	THINKPAD_EC_LED_ON = 0x80,
+	THINKPAD_EC_LED_BLINK = 0xc0,
+};
+
+struct thinkpad_t14s_ec_led_classdev {
+	struct led_classdev led_classdev;
+	int led;
+	enum thinkpad_t14s_ec_led_status_t cache;
+	struct thinkpad_t14s_ec *ec;
+};
+
+struct thinkpad_t14s_ec {
+	struct regmap *regmap;
+	struct device *dev;
+	struct thinkpad_t14s_ec_led_classdev led_pwr_btn;
+	struct thinkpad_t14s_ec_led_classdev led_chrg_orange;
+	struct thinkpad_t14s_ec_led_classdev led_chrg_white;
+	struct thinkpad_t14s_ec_led_classdev led_lid_logo_dot;
+	struct led_classdev kbd_backlight;
+	struct led_classdev led_mic_mute;
+	struct led_classdev led_spk_mute;
+	struct input_dev *inputdev;
+};
+
+static const struct regmap_config thinkpad_t14s_ec_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0xff,
+};
+
+static int thinkpad_t14s_ec_write(void *context, unsigned int reg,
+				  unsigned int val)
+{
+	char buf[5] = {THINKPAD_T14S_EC_CMD_ECWR, reg, 0x00, 0x01, val};
+	struct thinkpad_t14s_ec *ec = context;
+	struct i2c_client *client = to_i2c_client(ec->dev);
+	int ret;
+
+	ret = i2c_master_send(client, buf, sizeof(buf));
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int thinkpad_t14s_ec_read(void *context, unsigned int reg,
+				 unsigned int *val)
+{
+	char buf[4] = {THINKPAD_T14S_EC_CMD_ECRD, reg, 0x00, 0x01};
+	struct thinkpad_t14s_ec *ec = context;
+	struct i2c_client *client = to_i2c_client(ec->dev);
+	struct i2c_msg request, response;
+	u8 result;
+	int ret;
+
+	request.addr = client->addr;
+	request.flags = I2C_M_STOP;
+	request.len = sizeof(buf);
+	request.buf = buf;
+	response.addr = client->addr;
+	response.flags = I2C_M_RD;
+	response.len = 1;
+	response.buf = &result;
+
+	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+	ret = __i2c_transfer(client->adapter, &request, 1);
+	if (ret < 0)
+		goto out;
+	ret = __i2c_transfer(client->adapter, &response, 1);
+	if (ret < 0)
+		goto out;
+
+	*val = result;
+	ret = 0;
+
+out:
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	return ret;
+}
+
+static const struct regmap_bus thinkpad_t14s_ec_regmap_bus = {
+	.reg_write = thinkpad_t14s_ec_write,
+	.reg_read = thinkpad_t14s_ec_read,
+};
+
+static int thinkpad_t14s_ec_read_evt(struct thinkpad_t14s_ec *ec, u8 *val)
+{
+	char buf[4] = {THINKPAD_T14S_EC_CMD_EVT, 0x00, 0x00, 0x01};
+	struct i2c_client *client = to_i2c_client(ec->dev);
+	struct i2c_msg request, response;
+	int ret;
+
+	request.addr = client->addr;
+	request.flags = I2C_M_STOP;
+	request.len = sizeof(buf);
+	request.buf = buf;
+	response.addr = client->addr;
+	response.flags = I2C_M_RD;
+	response.len = 1;
+	response.buf = val;
+
+	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+	ret = __i2c_transfer(client->adapter, &request, 1);
+	if (ret < 0)
+		goto out;
+	ret = __i2c_transfer(client->adapter, &response, 1);
+	if (ret < 0)
+		goto out;
+
+	ret = 0;
+
+out:
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+	return ret;
+}
+
+static int thinkpad_t14s_led_set_status(struct thinkpad_t14s_ec *ec,
+			struct thinkpad_t14s_ec_led_classdev *led,
+			const enum thinkpad_t14s_ec_led_status_t ledstatus)
+{
+	int ret;
+
+	ret = regmap_write(ec->regmap, THINKPAD_T14S_EC_REG_LED,
+			   led->led | ledstatus);
+	if (ret < 0)
+		return ret;
+
+	led->cache = ledstatus;
+	return 0;
+}
+
+static int thinkpad_t14s_led_set(struct led_classdev *led_cdev,
+				 enum led_brightness brightness)
+{
+	struct thinkpad_t14s_ec_led_classdev *led = container_of(led_cdev,
+			struct thinkpad_t14s_ec_led_classdev, led_classdev);
+	enum thinkpad_t14s_ec_led_status_t new_state;
+
+	if (brightness == LED_OFF)
+		new_state = THINKPAD_EC_LED_OFF;
+	else if (led->cache != THINKPAD_EC_LED_BLINK)
+		new_state = THINKPAD_EC_LED_ON;
+	else
+		new_state = THINKPAD_EC_LED_BLINK;
+
+	return thinkpad_t14s_led_set_status(led->ec, led, new_state);
+}
+
+static int thinkpad_t14s_led_blink_set(struct led_classdev *led_cdev,
+				       unsigned long *delay_on,
+				       unsigned long *delay_off)
+{
+	struct thinkpad_t14s_ec_led_classdev *led = container_of(led_cdev,
+			struct thinkpad_t14s_ec_led_classdev, led_classdev);
+
+	/* Can we choose the flash rate? */
+	if (*delay_on == 0 && *delay_off == 0) {
+		/* yes. set them to the hardware blink rate (1 Hz) */
+		*delay_on = 500; /* ms */
+		*delay_off = 500; /* ms */
+	} else if ((*delay_on != 500) || (*delay_off != 500))
+		return -EINVAL;
+
+	return thinkpad_t14s_led_set_status(led->ec, led, THINKPAD_EC_LED_BLINK);
+}
+
+static int thinkpad_t14s_init_led(struct thinkpad_t14s_ec *ec,
+				  struct thinkpad_t14s_ec_led_classdev *led,
+				  u8 id, const char *name)
+{
+	led->led_classdev.name = name;
+	led->led_classdev.flags = LED_RETAIN_AT_SHUTDOWN;
+	led->led_classdev.max_brightness = 1;
+	led->led_classdev.brightness_set_blocking = thinkpad_t14s_led_set;
+	led->led_classdev.blink_set = thinkpad_t14s_led_blink_set;
+	led->ec = ec;
+	led->led = id;
+
+	return devm_led_classdev_register(ec->dev, &led->led_classdev);
+}
+
+static int thinkpad_t14s_leds_probe(struct thinkpad_t14s_ec *ec)
+{
+	int ret;
+
+	ret = thinkpad_t14s_init_led(ec, &ec->led_pwr_btn, 0,
+				     "platform::power");
+	if (ret)
+		return ret;
+	ret = thinkpad_t14s_init_led(ec, &ec->led_chrg_orange, 1,
+				     "platform:amber:battery-charging");
+	if (ret)
+		return ret;
+	ret = thinkpad_t14s_init_led(ec, &ec->led_chrg_white, 2,
+				     "platform:white:battery-full");
+	if (ret)
+		return ret;
+	ret = thinkpad_t14s_init_led(ec, &ec->led_lid_logo_dot, 10,
+				     "platform::lid_logo_dot");
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int thinkpad_t14s_kbd_bl_set(struct led_classdev *led_cdev,
+				    enum led_brightness brightness)
+{
+	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
+					struct thinkpad_t14s_ec, kbd_backlight);
+	int ret;
+
+	ret = regmap_update_bits(ec->regmap, THINKPAD_T14S_EC_REG_KBD_BL1,
+				 THINKPAD_T14S_EC_KBD_BL1_MASK,
+				 brightness << THINKPAD_T14S_EC_KBD_BL1_SHIFT);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_update_bits(ec->regmap, THINKPAD_T14S_EC_REG_KBD_BL2,
+				 THINKPAD_T14S_EC_KBD_BL2_MASK,
+				 brightness << THINKPAD_T14S_EC_KBD_BL2_SHIFT);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static enum led_brightness thinkpad_t14s_kbd_bl_get(struct led_classdev *led_cdev)
+{
+	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
+					struct thinkpad_t14s_ec, kbd_backlight);
+	int ret, val;
+
+	ret = regmap_read(ec->regmap, THINKPAD_T14S_EC_REG_KBD_BL1, &val);
+	if (ret < 0)
+		return ret;
+
+	val &= THINKPAD_T14S_EC_KBD_BL1_MASK;
+
+	return val >> THINKPAD_T14S_EC_KBD_BL1_SHIFT;
+}
+
+static void thinkpad_t14s_kbd_bl_update(struct thinkpad_t14s_ec *ec)
+{
+	enum led_brightness brightness = thinkpad_t14s_kbd_bl_get(&ec->kbd_backlight);
+
+	led_classdev_notify_brightness_hw_changed(&ec->kbd_backlight, brightness);
+}
+
+static int thinkpad_t14s_kbd_backlight_probe(struct thinkpad_t14s_ec *ec)
+{
+	ec->kbd_backlight.name = "platform::kbd_backlight";
+	ec->kbd_backlight.flags = LED_BRIGHT_HW_CHANGED;
+	ec->kbd_backlight.max_brightness = 2;
+	ec->kbd_backlight.brightness_set_blocking = thinkpad_t14s_kbd_bl_set;
+	ec->kbd_backlight.brightness_get = thinkpad_t14s_kbd_bl_get;
+
+	return devm_led_classdev_register(ec->dev, &ec->kbd_backlight);
+}
+
+static enum led_brightness thinkpad_t14s_audio_led_get(struct thinkpad_t14s_ec *ec,
+						       u8 led_bit)
+{
+	int ret, val;
+
+	ret = regmap_read(ec->regmap, THINKPAD_T14S_EC_REG_AUD, &val);
+	if (ret < 0)
+		return ret;
+
+	return !!(val && led_bit);
+}
+
+static enum led_brightness thinkpad_t14s_audio_led_set(struct thinkpad_t14s_ec *ec,
+						       u8 led_bit,
+						       enum led_brightness brightness)
+{
+	u8 val = brightness ? led_bit : 0;
+
+	return regmap_update_bits(ec->regmap, THINKPAD_T14S_EC_REG_AUD, led_bit, val);
+}
+
+static enum led_brightness thinkpad_t14s_mic_mute_led_get(struct led_classdev *led_cdev)
+{
+	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
+					struct thinkpad_t14s_ec, led_mic_mute);
+
+	return thinkpad_t14s_audio_led_get(ec, THINKPAD_T14S_EC_MIC_MUTE_LED);
+}
+
+static int thinkpad_t14s_mic_mute_led_set(struct led_classdev *led_cdev,
+					  enum led_brightness brightness)
+{
+	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
+					struct thinkpad_t14s_ec, led_mic_mute);
+
+	return thinkpad_t14s_audio_led_set(ec, THINKPAD_T14S_EC_MIC_MUTE_LED, brightness);
+}
+
+static enum led_brightness thinkpad_t14s_spk_mute_led_get(struct led_classdev *led_cdev)
+{
+	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
+					struct thinkpad_t14s_ec, led_spk_mute);
+
+	return thinkpad_t14s_audio_led_get(ec, THINKPAD_T14S_EC_SPK_MUTE_LED);
+}
+
+static int thinkpad_t14s_spk_mute_led_set(struct led_classdev *led_cdev,
+					  enum led_brightness brightness)
+{
+	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
+					struct thinkpad_t14s_ec, led_spk_mute);
+
+	return thinkpad_t14s_audio_led_set(ec, THINKPAD_T14S_EC_SPK_MUTE_LED, brightness);
+}
+
+static int thinkpad_t14s_kbd_audio_led_probe(struct thinkpad_t14s_ec *ec)
+{
+	int ret;
+
+	ec->led_mic_mute.name = "platform::micmute";
+	ec->led_mic_mute.max_brightness = 1,
+	ec->led_mic_mute.default_trigger = "audio-micmute",
+	ec->led_mic_mute.brightness_set_blocking = thinkpad_t14s_mic_mute_led_set;
+	ec->led_mic_mute.brightness_get = thinkpad_t14s_mic_mute_led_get;
+
+	ec->led_spk_mute.name = "platform::mute";
+	ec->led_spk_mute.max_brightness = 1,
+	ec->led_spk_mute.default_trigger = "audio-mute",
+	ec->led_spk_mute.brightness_set_blocking = thinkpad_t14s_spk_mute_led_set;
+	ec->led_spk_mute.brightness_get = thinkpad_t14s_spk_mute_led_get;
+
+	ret = devm_led_classdev_register(ec->dev, &ec->led_mic_mute);
+	if (ret)
+		return ret;
+
+	return devm_led_classdev_register(ec->dev, &ec->led_spk_mute);
+}
+
+/*
+ * using code 0x16 ignores the provided KEY code and use KEY_MUTE,
+ * so all codes have a 0x1000 offset
+ */
+static const struct key_entry thinkpad_t14s_keymap[] = {
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_4, { KEY_SLEEP } },
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_N, { KEY_VENDOR } },
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F4, { KEY_MICMUTE } },
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F7, { KEY_SWITCHVIDEOMODE } },
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F8, { KEY_MODE } },
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F10, { KEY_SELECTIVE_SCREENSHOT } },
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F11, { KEY_LINK_PHONE } },
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F12, { KEY_BOOKMARKS } },
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_SPACE, { KEY_KBDILLUMTOGGLE } },
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_ESC, { KEY_FN_ESC } },
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_TAB, { KEY_ZOOM } },
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_RIGHT_SHIFT, { KEY_FN_RIGHT_SHIFT } },
+	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_TP_DOUBLE_TAP, { KEY_PROG4 } },
+	{ KE_END }
+};
+
+static int thinkpad_t14s_input_probe(struct thinkpad_t14s_ec *ec)
+{
+	int ret;
+
+	ec->inputdev = devm_input_allocate_device(ec->dev);
+	if (!ec->inputdev)
+		return -ENOMEM;
+
+	ec->inputdev->name = "ThinkPad Extra Buttons";
+	ec->inputdev->phys = "thinkpad/input0";
+	ec->inputdev->id.bustype = BUS_HOST;
+	ec->inputdev->dev.parent = ec->dev;
+
+	ret = sparse_keymap_setup(ec->inputdev, thinkpad_t14s_keymap, NULL);
+	if (ret)
+		return ret;
+
+	ret = input_register_device(ec->inputdev);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static irqreturn_t thinkpad_t14s_ec_irq_handler(int irq, void *data)
+{
+	struct thinkpad_t14s_ec *ec = data;
+	int ret;
+	u8 val;
+
+	ret = thinkpad_t14s_ec_read_evt(ec, &val);
+	if (ret < 0) {
+		dev_err(ec->dev, "Failed to read event\n");
+		return IRQ_HANDLED;
+	}
+
+	switch (val) {
+	case THINKPAD_T14S_EC_EVT_NONE:
+		break;
+	case THINKPAD_T14S_EC_EVT_KEY_FN_SPACE:
+		thinkpad_t14s_kbd_bl_update(ec);
+		fallthrough;
+	case THINKPAD_T14S_EC_EVT_KEY_FN_F4:
+	case THINKPAD_T14S_EC_EVT_KEY_FN_F7:
+	case THINKPAD_T14S_EC_EVT_KEY_FN_4:
+	case THINKPAD_T14S_EC_EVT_KEY_FN_F8:
+	case THINKPAD_T14S_EC_EVT_KEY_FN_F12:
+	case THINKPAD_T14S_EC_EVT_KEY_FN_TAB:
+	case THINKPAD_T14S_EC_EVT_KEY_FN_F10:
+	case THINKPAD_T14S_EC_EVT_KEY_FN_N:
+	case THINKPAD_T14S_EC_EVT_KEY_FN_F11:
+	case THINKPAD_T14S_EC_EVT_KEY_FN_ESC:
+	case THINKPAD_T14S_EC_EVT_KEY_FN_RIGHT_SHIFT:
+	case THINKPAD_T14S_EC_EVT_KEY_TP_DOUBLE_TAP:
+		sparse_keymap_report_event(ec->inputdev, 0x1000 + val, 1, true);
+		break;
+	case THINKPAD_T14S_EC_EVT_AC_CONNECTED:
+		dev_dbg(ec->dev, "AC connected\n");
+		break;
+	case THINKPAD_T14S_EC_EVT_AC_DISCONNECTED:
+		dev_dbg(ec->dev, "AC disconnected\n");
+		break;
+	case THINKPAD_T14S_EC_EVT_KEY_POWER:
+		dev_dbg(ec->dev, "power button\n");
+		break;
+	case THINKPAD_T14S_EC_EVT_LID_OPEN:
+		dev_dbg(ec->dev, "LID open\n");
+		break;
+	case THINKPAD_T14S_EC_EVT_LID_CLOSED:
+		dev_dbg(ec->dev, "LID closed\n");
+		break;
+	case THINKPAD_T14S_EC_EVT_KEY_FN_G:
+		dev_dbg(ec->dev, "FN + G - toggle double-tapping\n");
+		break;
+	case THINKPAD_T14S_EC_EVT_KEY_FN_L:
+		dev_dbg(ec->dev, "FN + L - low performance mode\n");
+		break;
+	case THINKPAD_T14S_EC_EVT_KEY_FN_M:
+		dev_dbg(ec->dev, "FN + M - medium performance mode\n");
+		break;
+	case THINKPAD_T14S_EC_EVT_KEY_FN_H:
+		dev_dbg(ec->dev, "FN + H - high performance mode\n");
+		break;
+	case THINKPAD_T14S_EC_EVT_KEY_FN_T:
+		dev_dbg(ec->dev, "FN + T - toggle intelligent cooling mode\n");
+		break;
+	case THINKPAD_T14S_EC_EVT_KEY_FN_D:
+		dev_dbg(ec->dev, "FN + D - toggle privacy guard mode\n");
+		break;
+	default:
+		dev_info(ec->dev, "Unknown EC event: 0x%02x\n", val);
+		break;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int thinkpad_t14s_ec_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct thinkpad_t14s_ec *ec;
+	int ret;
+
+	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
+	if (!ec)
+		return -ENOMEM;
+
+	ec->dev = dev;
+
+	ec->regmap = devm_regmap_init(dev, &thinkpad_t14s_ec_regmap_bus,
+				      ec, &thinkpad_t14s_ec_regmap_config);
+	if (IS_ERR(ec->regmap))
+		return dev_err_probe(dev, PTR_ERR(ec->regmap),
+				     "Failed to init regmap\n");
+
+	ret = devm_request_threaded_irq(dev, client->irq, NULL,
+					thinkpad_t14s_ec_irq_handler,
+					IRQF_ONESHOT, dev_name(dev), ec);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to get IRQ\n");
+
+	ret = thinkpad_t14s_leds_probe(ec);
+	if (ret < 0)
+		return ret;
+
+	ret = thinkpad_t14s_kbd_backlight_probe(ec);
+	if (ret < 0)
+		return ret;
+
+	ret = thinkpad_t14s_kbd_audio_led_probe(ec);
+	if (ret < 0)
+		return ret;
+
+	ret = thinkpad_t14s_input_probe(ec);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Disable wakeup support by default, because the driver currently does
+	 * not support masking any events and the laptop should not wake up when
+	 * the LID is closed.
+	 */
+	device_wakeup_disable(dev);
+
+	return 0;
+}
+
+static const struct of_device_id thinkpad_t14s_ec_of_match[] = {
+	{ .compatible = "lenovo,thinkpad-t14s-ec" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, thinkpad_t14s_ec_of_match);
+
+static const struct i2c_device_id thinkpad_t14s_ec_i2c_id_table[] = {
+	{ "thinkpad-t14s-ec", },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, thinkpad_t14s_ec_i2c_id_table);
+
+static struct i2c_driver thinkpad_t14s_ec_i2c_driver = {
+	.driver = {
+		.name = "thinkpad-t14s-ec",
+		.of_match_table = thinkpad_t14s_ec_of_match
+	},
+	.probe = thinkpad_t14s_ec_probe,
+	.id_table = thinkpad_t14s_ec_i2c_id_table,
+};
+module_i2c_driver(thinkpad_t14s_ec_i2c_driver);
+
+MODULE_AUTHOR("Sebastian Reichel <sre@kernel.org>");
+MODULE_DESCRIPTION("Lenovo Thinkpad T14s Embedded Controller");
+MODULE_LICENSE("GPL");

-- 
2.50.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 3/3] arm64: dts: qcom: x1e80100-t14s: add EC
  2025-08-31 21:28 [PATCH 0/3] platform: arm64: thinkpad-t14s-ec: new driver Sebastian Reichel
  2025-08-31 21:28 ` [PATCH 1/3] dt-bindings: platform: Add Lenovo Thinkpad T14s EC Sebastian Reichel
  2025-08-31 21:28 ` [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver Sebastian Reichel
@ 2025-08-31 21:28 ` Sebastian Reichel
  2025-09-02 10:28   ` Konrad Dybcio
  2025-09-01  9:03 ` [PATCH 0/3] platform: arm64: thinkpad-t14s-ec: new driver Neil Armstrong
  2025-09-02 13:17 ` Rob Herring (Arm)
  4 siblings, 1 reply; 18+ messages in thread
From: Sebastian Reichel @ 2025-08-31 21:28 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
	Bjorn Andersson, Konrad Dybcio, Mark Pearson
  Cc: Derek J. Clark, Henrique de Moraes Holschuh, devicetree,
	linux-kernel, platform-driver-x86, linux-arm-msm

Describe ThinkPad Embedded Controller in the T14s device tree,
which adds LED and special key support.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 .../dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi    | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
index ac1dddf27da30e6a9f7e1d1ecbd5192bf2d0671e..7a9ec0c33b3ca847c5496e3ec145c70ccb7a3fe3 100644
--- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
@@ -887,6 +887,23 @@ eusb6_repeater: redriver@4f {
 	};
 };
 
+&i2c6 {
+	clock-frequency = <400000>;
+	status = "okay";
+
+	ec@28 {
+		compatible = "lenovo,thinkpad-t14s-ec";
+		reg = <0x28>;
+
+		interrupts-extended = <&tlmm 66 IRQ_TYPE_LEVEL_LOW>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&ec_int_n_default>;
+
+		wakeup-source;
+	};
+};
+
 &i2c7 {
 	clock-frequency = <400000>;
 
@@ -1267,6 +1284,12 @@ &tlmm {
 			       <72 2>, /* Secure EC I2C connection (?) */
 			       <238 1>; /* UFS Reset */
 
+	ec_int_n_default: ec-int-n-state {
+		pins = "gpio66";
+		function = "gpio";
+		bias-disable;
+	};
+
 	eusb3_reset_n: eusb3-reset-n-state {
 		pins = "gpio6";
 		function = "gpio";

-- 
2.50.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] dt-bindings: platform: Add Lenovo Thinkpad T14s EC
  2025-08-31 21:28 ` [PATCH 1/3] dt-bindings: platform: Add Lenovo Thinkpad T14s EC Sebastian Reichel
@ 2025-08-31 23:53   ` Bryan O'Donoghue
  2025-09-01  9:28   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 18+ messages in thread
From: Bryan O'Donoghue @ 2025-08-31 23:53 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Hans de Goede, Ilpo Järvinen, Bjorn Andersson, Konrad Dybcio,
	Mark Pearson
  Cc: Derek J. Clark, Henrique de Moraes Holschuh, devicetree,
	linux-kernel, platform-driver-x86, linux-arm-msm

On 31/08/2025 22:28, Sebastian Reichel wrote:
> Add binding for the EC found in the Thinkpad T14s Gen6 Snapdragon,
> which is based on the Qualcomm X1 Elite. Some of the system LEDs
> and extra keys are only accessible via the EC.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>   .../bindings/platform/lenovo,thinkpad-t14s-ec.yaml | 49 ++++++++++++++++++++++
>   1 file changed, 49 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml b/Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..bab20df2d9ede9a3cb0359944b26b3d18ff7d9b8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/platform/lenovo,thinkpad-t14s-ec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Lenovo Thinkpad T14s Embedded Controller.
> +
> +maintainers:
> +  - Sebastian Reichel <sre@kernel.org>
> +
> +description:
> +  The Qualcomm Snapdragon-based Lenovo Thinkpad T14s has an Embedded Controller
> +  (EC) which handles things such as keyboard backlight, LEDs or non-standard keys.
> +  This binding describes the interface, on an I2C bus, to this EC.
> +
> +properties:
> +  compatible:
> +    const: lenovo,thinkpad-t14s-ec
> +
> +  reg:
> +    const: 0x28
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |+
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c1 {
> +        clock-frequency = <400000>;
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        embedded-controller@28 {
> +            compatible = "lenovo,thinkpad-t14s-ec";
> +            reg = <0x28>;
> +            interrupts-extended = <&tlmm 66 IRQ_TYPE_LEVEL_LOW>;
> +        };
> +    };
> +...
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver
  2025-08-31 21:28 ` [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver Sebastian Reichel
@ 2025-09-01  8:43   ` Bryan O'Donoghue
  2025-09-01 15:20     ` Sebastian Reichel
  2025-09-01  9:51   ` Ilpo Järvinen
  2025-09-01 13:48   ` Mark Pearson
  2 siblings, 1 reply; 18+ messages in thread
From: Bryan O'Donoghue @ 2025-09-01  8:43 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Hans de Goede, Ilpo Järvinen, Bjorn Andersson, Konrad Dybcio,
	Mark Pearson
  Cc: Derek J. Clark, Henrique de Moraes Holschuh, devicetree,
	linux-kernel, platform-driver-x86, linux-arm-msm

On 31/08/2025 22:28, Sebastian Reichel wrote:
> Introduce EC driver for the ThinkPad T14s Gen6 Snapdragon, which
> is in theory compatible with ThinkPad ACPI. On Linux the system
> is booted with device tree, which is not supported by the ThinkPad
> ACPI driver. Also most of the hardware compatibility is handled
> via ACPI tables, which are obviously not used when booting via
> device tree. Thus adding DT compatibility to the existing driver
> is not worth it (almost no code sharing).

What is the name of that driver, you should name it in your commit log.

Lenovo WMI ?
> 
> The driver currently exposes features, which are not available
> via other means:
> 
>   * Extra Keys
>   * System LEDs
>   * Keyboard Backlight Control
> 
> The driver has been developed by reading the ACPI DSDT. There
> are some more features around thermal control, which are not
> yet supported by the driver.
> 
> The speaker mute and mic mute LEDs need some additional changes
> in the ALSA UCM to be set automatically.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>   MAINTAINERS                                   |   6 +
>   drivers/platform/arm64/Kconfig                |  20 +
>   drivers/platform/arm64/Makefile               |   1 +
>   drivers/platform/arm64/lenovo-thinkpad-t14s.c | 597 ++++++++++++++++++++++++++
>   4 files changed, 624 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e94d68c980c5f8bef2e1caf26b1a775df6aa1d84..589466169c222b2e088c6112a1e724c95e948f72 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25092,6 +25092,12 @@ W:	http://thinkwiki.org/wiki/Ibm-acpi
>   T:	git git://repo.or.cz/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git
>   F:	drivers/platform/x86/lenovo/thinkpad_acpi.c
>   
> +THINKPAD T14S EMBEDDED CONTROLLER DRIVER
> +M:	Sebastian Reichel <sre@kernel.org>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml
> +F:	drivers/platform/arm64/lenovo-thinkpad-t14s.c
> +
>   THINKPAD LMI DRIVER
>   M:	Mark Pearson <mpearson-lenovo@squebb.ca>
>   L:	platform-driver-x86@vger.kernel.org
> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> index 06288aebc5599435065a37f8dacd046b5def80bd..10f905d7d6bfa5fad30a0689d3a20481268c781e 100644
> --- a/drivers/platform/arm64/Kconfig
> +++ b/drivers/platform/arm64/Kconfig
> @@ -70,4 +70,24 @@ config EC_LENOVO_YOGA_C630
>   
>   	  Say M or Y here to include this support.
>   
> +config EC_LENOVO_THINKPAD_T14S
> +	tristate "Lenovo Thinkpad T14s Embedded Controller driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on I2C
> +	depends on INPUT
> +	select INPUT_SPARSEKMAP
> +	select LEDS_CLASS
> +	select NEW_LEDS
> +	select SND_CTL_LED if SND
> +	help
> +	  Driver for the Embedded Controller in the Qualcomm Snapdragon-based
> +	  Lenovo Thinkpad T14s, which provides access to keyboard backlight
> +	  and status LEDs.
> +
> +	  This driver provides support for the mentioned laptop where this
> +	  information is not properly exposed via the standard Qualcomm
> +	  devices.
> +
> +	  Say M or Y here to include this support.
> +
>   endif # ARM64_PLATFORM_DEVICES
> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
> index 46a99eba3264cc40e812567d1533bb86031a6af3..60c131cff6a15bb51a49c9edab95badf513ee0f6 100644
> --- a/drivers/platform/arm64/Makefile
> +++ b/drivers/platform/arm64/Makefile
> @@ -8,3 +8,4 @@
>   obj-$(CONFIG_EC_ACER_ASPIRE1)	+= acer-aspire1-ec.o
>   obj-$(CONFIG_EC_HUAWEI_GAOKUN)	+= huawei-gaokun-ec.o
>   obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o
> +obj-$(CONFIG_EC_LENOVO_THINKPAD_T14S) += lenovo-thinkpad-t14s.o
> diff --git a/drivers/platform/arm64/lenovo-thinkpad-t14s.c b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ab783870e8eadfe13d83500c7f39440291e42cc9
> --- /dev/null
> +++ b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> @@ -0,0 +1,597 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025, Sebastian Reichel
> + */
> +
> +#define DEBUG
> +
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/leds.h>
> +#include <linux/lockdep.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define THINKPAD_T14S_EC_CMD_ECRD 0x02
> +#define THINKPAD_T14S_EC_CMD_ECWR 0x03
> +#define THINKPAD_T14S_EC_CMD_EVT 0xf0
> +
> +#define THINKPAD_T14S_EC_REG_LED	0x0c
> +#define THINKPAD_T14S_EC_REG_KBD_BL1	0x0d
> +#define THINKPAD_T14S_EC_REG_KBD_BL2	0xe1
> +#define THINKPAD_T14S_EC_KBD_BL1_MASK	GENMASK_U8(7, 6)
> +#define THINKPAD_T14S_EC_KBD_BL1_SHIFT	6
> +#define THINKPAD_T14S_EC_KBD_BL2_MASK	GENMASK_U8(3, 2)
> +#define THINKPAD_T14S_EC_KBD_BL2_SHIFT	2
> +#define THINKPAD_T14S_EC_REG_AUD	0x30
> +#define THINKPAD_T14S_EC_MIC_MUTE_LED	BIT(5)
> +#define THINKPAD_T14S_EC_SPK_MUTE_LED	BIT(6)
> +
> +#define THINKPAD_T14S_EC_EVT_NONE			0x00
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_4			0x13
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F7			0x16
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_SPACE		0x1F
> +#define THINKPAD_T14S_EC_EVT_KEY_TP_DOUBLE_TAP		0x20
> +#define THINKPAD_T14S_EC_EVT_AC_CONNECTED		0x26
> +#define THINKPAD_T14S_EC_EVT_AC_DISCONNECTED		0x27
> +#define THINKPAD_T14S_EC_EVT_KEY_POWER			0x28
> +#define THINKPAD_T14S_EC_EVT_LID_OPEN			0x2A
> +#define THINKPAD_T14S_EC_EVT_LID_CLOSED			0x2B
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F12			0x62
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_TAB			0x63
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F8			0x64
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F10			0x65
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F4			0x6A
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_D			0x6B
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_T			0x6C
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_H			0x6D
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_M			0x6E
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_L			0x6F
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_RIGHT_SHIFT		0x71
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_ESC			0x74
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_N			0x79
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F11			0x7A
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_G			0x7E
> +
> +enum thinkpad_t14s_ec_led_status_t {
> +	THINKPAD_EC_LED_OFF = 0x00,
> +	THINKPAD_EC_LED_ON = 0x80,
> +	THINKPAD_EC_LED_BLINK = 0xc0,
> +};
> +
> +struct thinkpad_t14s_ec_led_classdev {
> +	struct led_classdev led_classdev;
> +	int led;
> +	enum thinkpad_t14s_ec_led_status_t cache;
> +	struct thinkpad_t14s_ec *ec;
> +};
> +
> +struct thinkpad_t14s_ec {
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct thinkpad_t14s_ec_led_classdev led_pwr_btn;
> +	struct thinkpad_t14s_ec_led_classdev led_chrg_orange;
> +	struct thinkpad_t14s_ec_led_classdev led_chrg_white;
> +	struct thinkpad_t14s_ec_led_classdev led_lid_logo_dot;
> +	struct led_classdev kbd_backlight;
> +	struct led_classdev led_mic_mute;
> +	struct led_classdev led_spk_mute;
> +	struct input_dev *inputdev;
> +};
> +
> +static const struct regmap_config thinkpad_t14s_ec_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xff,
> +};
> +
> +static int thinkpad_t14s_ec_write(void *context, unsigned int reg,
> +				  unsigned int val)
> +{
> +	char buf[5] = {THINKPAD_T14S_EC_CMD_ECWR, reg, 0x00, 0x01, val};
> +	struct thinkpad_t14s_ec *ec = context;
> +	struct i2c_client *client = to_i2c_client(ec->dev);
> +	int ret;
> +
> +	ret = i2c_master_send(client, buf, sizeof(buf));
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int thinkpad_t14s_ec_read(void *context, unsigned int reg,
> +				 unsigned int *val)
> +{
> +	char buf[4] = {THINKPAD_T14S_EC_CMD_ECRD, reg, 0x00, 0x01};
> +	struct thinkpad_t14s_ec *ec = context;
> +	struct i2c_client *client = to_i2c_client(ec->dev);
> +	struct i2c_msg request, response;
> +	u8 result;
> +	int ret;
> +
> +	request.addr = client->addr;
> +	request.flags = I2C_M_STOP;
> +	request.len = sizeof(buf);
> +	request.buf = buf;
> +	response.addr = client->addr;
> +	response.flags = I2C_M_RD;
> +	response.len = 1;
> +	response.buf = &result;
> +
> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> +	ret = __i2c_transfer(client->adapter, &request, 1);
> +	if (ret < 0)
> +		goto out;
> +	ret = __i2c_transfer(client->adapter, &response, 1);
> +	if (ret < 0)
> +		goto out;
> +
> +	*val = result;
> +	ret = 0;
> +
> +out:
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	return ret;
> +}
> +
> +static const struct regmap_bus thinkpad_t14s_ec_regmap_bus = {
> +	.reg_write = thinkpad_t14s_ec_write,
> +	.reg_read = thinkpad_t14s_ec_read,
> +};
> +
> +static int thinkpad_t14s_ec_read_evt(struct thinkpad_t14s_ec *ec, u8 *val)
> +{
> +	char buf[4] = {THINKPAD_T14S_EC_CMD_EVT, 0x00, 0x00, 0x01};
> +	struct i2c_client *client = to_i2c_client(ec->dev);
> +	struct i2c_msg request, response;
> +	int ret;
> +
> +	request.addr = client->addr;
> +	request.flags = I2C_M_STOP;
> +	request.len = sizeof(buf);
> +	request.buf = buf;
> +	response.addr = client->addr;
> +	response.flags = I2C_M_RD;
> +	response.len = 1;
> +	response.buf = val;
> +
> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> +	ret = __i2c_transfer(client->adapter, &request, 1);
> +	if (ret < 0)
> +		goto out;

I realise this is your coding style but suggest newline after these gotos.

> +	ret = __i2c_transfer(client->adapter, &response, 1);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = 0;
> +
> +out:
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	return ret;
> +}
> +
> +static int thinkpad_t14s_led_set_status(struct thinkpad_t14s_ec *ec,
> +			struct thinkpad_t14s_ec_led_classdev *led,
> +			const enum thinkpad_t14s_ec_led_status_t ledstatus)
> +{
> +	int ret;
> +
> +	ret = regmap_write(ec->regmap, THINKPAD_T14S_EC_REG_LED,
> +			   led->led | ledstatus);
> +	if (ret < 0)
> +		return ret;
> +
> +	led->cache = ledstatus;
> +	return 0;
> +}
> +
> +static int thinkpad_t14s_led_set(struct led_classdev *led_cdev,
> +				 enum led_brightness brightness)
> +{
> +	struct thinkpad_t14s_ec_led_classdev *led = container_of(led_cdev,
> +			struct thinkpad_t14s_ec_led_classdev, led_classdev);
> +	enum thinkpad_t14s_ec_led_status_t new_state;
> +
> +	if (brightness == LED_OFF)
> +		new_state = THINKPAD_EC_LED_OFF;
> +	else if (led->cache != THINKPAD_EC_LED_BLINK)
> +		new_state = THINKPAD_EC_LED_ON;
> +	else
> +		new_state = THINKPAD_EC_LED_BLINK;
> +
> +	return thinkpad_t14s_led_set_status(led->ec, led, new_state);
> +}
> +
> +static int thinkpad_t14s_led_blink_set(struct led_classdev *led_cdev,
> +				       unsigned long *delay_on,
> +				       unsigned long *delay_off)
> +{
> +	struct thinkpad_t14s_ec_led_classdev *led = container_of(led_cdev,
> +			struct thinkpad_t14s_ec_led_classdev, led_classdev);
> +
> +	/* Can we choose the flash rate? */
> +	if (*delay_on == 0 && *delay_off == 0) {
> +		/* yes. set them to the hardware blink rate (1 Hz) */
> +		*delay_on = 500; /* ms */
> +		*delay_off = 500; /* ms */
> +	} else if ((*delay_on != 500) || (*delay_off != 500))
> +		return -EINVAL;

Those 500s should probably be defines
> +
> +	return thinkpad_t14s_led_set_status(led->ec, led, THINKPAD_EC_LED_BLINK);
> +}
> +
> +static int thinkpad_t14s_init_led(struct thinkpad_t14s_ec *ec,
> +				  struct thinkpad_t14s_ec_led_classdev *led,
> +				  u8 id, const char *name)
> +{
> +	led->led_classdev.name = name;
> +	led->led_classdev.flags = LED_RETAIN_AT_SHUTDOWN;
> +	led->led_classdev.max_brightness = 1;
> +	led->led_classdev.brightness_set_blocking = thinkpad_t14s_led_set;
> +	led->led_classdev.blink_set = thinkpad_t14s_led_blink_set;
> +	led->ec = ec;
> +	led->led = id;
> +
> +	return devm_led_classdev_register(ec->dev, &led->led_classdev);
> +}
> +
> +static int thinkpad_t14s_leds_probe(struct thinkpad_t14s_ec *ec)
> +{
> +	int ret;
> +
> +	ret = thinkpad_t14s_init_led(ec, &ec->led_pwr_btn, 0,
> +				     "platform::power");
> +	if (ret)
> +		return ret;
> +	ret = thinkpad_t14s_init_led(ec, &ec->led_chrg_orange, 1,
> +				     "platform:amber:battery-charging");
> +	if (ret)
> +		return ret;

Suggest newlines after these rets per how you do it in 
thinkpad_t14s_input_probe() later on.


> +	ret = thinkpad_t14s_init_led(ec, &ec->led_chrg_white, 2,
> +				     "platform:white:battery-full");
> +	if (ret)
> +		return ret;
> +	ret = thinkpad_t14s_init_led(ec, &ec->led_lid_logo_dot, 10,
> +				     "platform::lid_logo_dot");
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int thinkpad_t14s_kbd_bl_set(struct led_classdev *led_cdev,
> +				    enum led_brightness brightness)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, kbd_backlight);
> +	int ret;
> +
> +	ret = regmap_update_bits(ec->regmap, THINKPAD_T14S_EC_REG_KBD_BL1,
> +				 THINKPAD_T14S_EC_KBD_BL1_MASK,
> +				 brightness << THINKPAD_T14S_EC_KBD_BL1_SHIFT);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_update_bits(ec->regmap, THINKPAD_T14S_EC_REG_KBD_BL2,
> +				 THINKPAD_T14S_EC_KBD_BL2_MASK,
> +				 brightness << THINKPAD_T14S_EC_KBD_BL2_SHIFT);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static enum led_brightness thinkpad_t14s_kbd_bl_get(struct led_classdev *led_cdev)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, kbd_backlight);
> +	int ret, val;
> +
> +	ret = regmap_read(ec->regmap, THINKPAD_T14S_EC_REG_KBD_BL1, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	val &= THINKPAD_T14S_EC_KBD_BL1_MASK;
> +
> +	return val >> THINKPAD_T14S_EC_KBD_BL1_SHIFT;
> +}
> +
> +static void thinkpad_t14s_kbd_bl_update(struct thinkpad_t14s_ec *ec)
> +{
> +	enum led_brightness brightness = thinkpad_t14s_kbd_bl_get(&ec->kbd_backlight);
> +
> +	led_classdev_notify_brightness_hw_changed(&ec->kbd_backlight, brightness);
> +}
> +
> +static int thinkpad_t14s_kbd_backlight_probe(struct thinkpad_t14s_ec *ec)
> +{
> +	ec->kbd_backlight.name = "platform::kbd_backlight";
> +	ec->kbd_backlight.flags = LED_BRIGHT_HW_CHANGED;
> +	ec->kbd_backlight.max_brightness = 2;
> +	ec->kbd_backlight.brightness_set_blocking = thinkpad_t14s_kbd_bl_set;
> +	ec->kbd_backlight.brightness_get = thinkpad_t14s_kbd_bl_get;
> +
> +	return devm_led_classdev_register(ec->dev, &ec->kbd_backlight);
> +}
> +
> +static enum led_brightness thinkpad_t14s_audio_led_get(struct thinkpad_t14s_ec *ec,
> +						       u8 led_bit)
> +{
> +	int ret, val;
> +
> +	ret = regmap_read(ec->regmap, THINKPAD_T14S_EC_REG_AUD, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return !!(val && led_bit);
> +}
> +
> +static enum led_brightness thinkpad_t14s_audio_led_set(struct thinkpad_t14s_ec *ec,
> +						       u8 led_bit,
> +						       enum led_brightness brightness)
> +{
> +	u8 val = brightness ? led_bit : 0;
> +
> +	return regmap_update_bits(ec->regmap, THINKPAD_T14S_EC_REG_AUD, led_bit, val);
> +}
> +
> +static enum led_brightness thinkpad_t14s_mic_mute_led_get(struct led_classdev *led_cdev)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, led_mic_mute);
> +
> +	return thinkpad_t14s_audio_led_get(ec, THINKPAD_T14S_EC_MIC_MUTE_LED);
> +}
> +
> +static int thinkpad_t14s_mic_mute_led_set(struct led_classdev *led_cdev,
> +					  enum led_brightness brightness)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, led_mic_mute);
> +
> +	return thinkpad_t14s_audio_led_set(ec, THINKPAD_T14S_EC_MIC_MUTE_LED, brightness);
> +}
> +
> +static enum led_brightness thinkpad_t14s_spk_mute_led_get(struct led_classdev *led_cdev)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, led_spk_mute);
> +
> +	return thinkpad_t14s_audio_led_get(ec, THINKPAD_T14S_EC_SPK_MUTE_LED);
> +}
> +
> +static int thinkpad_t14s_spk_mute_led_set(struct led_classdev *led_cdev,
> +					  enum led_brightness brightness)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, led_spk_mute);
> +
> +	return thinkpad_t14s_audio_led_set(ec, THINKPAD_T14S_EC_SPK_MUTE_LED, brightness);
> +}
> +
> +static int thinkpad_t14s_kbd_audio_led_probe(struct thinkpad_t14s_ec *ec)
> +{
> +	int ret;
> +
> +	ec->led_mic_mute.name = "platform::micmute";
> +	ec->led_mic_mute.max_brightness = 1,
> +	ec->led_mic_mute.default_trigger = "audio-micmute",
> +	ec->led_mic_mute.brightness_set_blocking = thinkpad_t14s_mic_mute_led_set;
> +	ec->led_mic_mute.brightness_get = thinkpad_t14s_mic_mute_led_get;
> +
> +	ec->led_spk_mute.name = "platform::mute";
> +	ec->led_spk_mute.max_brightness = 1,
> +	ec->led_spk_mute.default_trigger = "audio-mute",
> +	ec->led_spk_mute.brightness_set_blocking = thinkpad_t14s_spk_mute_led_set;
> +	ec->led_spk_mute.brightness_get = thinkpad_t14s_spk_mute_led_get;
> +
> +	ret = devm_led_classdev_register(ec->dev, &ec->led_mic_mute);
> +	if (ret)
> +		return ret;
> +
> +	return devm_led_classdev_register(ec->dev, &ec->led_spk_mute);
> +}
> +
> +/*
> + * using code 0x16 ignores the provided KEY code and use KEY_MUTE,
> + * so all codes have a 0x1000 offset
> + */
> +static const struct key_entry thinkpad_t14s_keymap[] = {
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_4, { KEY_SLEEP } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_N, { KEY_VENDOR } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F4, { KEY_MICMUTE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F7, { KEY_SWITCHVIDEOMODE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F8, { KEY_MODE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F10, { KEY_SELECTIVE_SCREENSHOT } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F11, { KEY_LINK_PHONE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F12, { KEY_BOOKMARKS } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_SPACE, { KEY_KBDILLUMTOGGLE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_ESC, { KEY_FN_ESC } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_TAB, { KEY_ZOOM } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_RIGHT_SHIFT, { KEY_FN_RIGHT_SHIFT } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_TP_DOUBLE_TAP, { KEY_PROG4 } },
> +	{ KE_END }
> +};
> +
> +static int thinkpad_t14s_input_probe(struct thinkpad_t14s_ec *ec)
> +{
> +	int ret;
> +
> +	ec->inputdev = devm_input_allocate_device(ec->dev);
> +	if (!ec->inputdev)
> +		return -ENOMEM;
> +
> +	ec->inputdev->name = "ThinkPad Extra Buttons";
> +	ec->inputdev->phys = "thinkpad/input0";
> +	ec->inputdev->id.bustype = BUS_HOST;
> +	ec->inputdev->dev.parent = ec->dev;
> +
> +	ret = sparse_keymap_setup(ec->inputdev, thinkpad_t14s_keymap, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = input_register_device(ec->inputdev);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static irqreturn_t thinkpad_t14s_ec_irq_handler(int irq, void *data)
> +{
> +	struct thinkpad_t14s_ec *ec = data;
> +	int ret;
> +	u8 val;
> +
> +	ret = thinkpad_t14s_ec_read_evt(ec, &val);
> +	if (ret < 0) {
> +		dev_err(ec->dev, "Failed to read event\n");
> +		return IRQ_HANDLED;
> +	}
> +
> +	switch (val) {
> +	case THINKPAD_T14S_EC_EVT_NONE:
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_SPACE:
> +		thinkpad_t14s_kbd_bl_update(ec);
> +		fallthrough;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F4:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F7:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_4:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F8:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F12:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_TAB:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F10:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_N:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F11:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_ESC:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_RIGHT_SHIFT:
> +	case THINKPAD_T14S_EC_EVT_KEY_TP_DOUBLE_TAP:
> +		sparse_keymap_report_event(ec->inputdev, 0x1000 + val, 1, true);
> +		break;
> +	case THINKPAD_T14S_EC_EVT_AC_CONNECTED:
> +		dev_dbg(ec->dev, "AC connected\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_AC_DISCONNECTED:
> +		dev_dbg(ec->dev, "AC disconnected\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_POWER:
> +		dev_dbg(ec->dev, "power button\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_LID_OPEN:
> +		dev_dbg(ec->dev, "LID open\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_LID_CLOSED:
> +		dev_dbg(ec->dev, "LID closed\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_G:
> +		dev_dbg(ec->dev, "FN + G - toggle double-tapping\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_L:
> +		dev_dbg(ec->dev, "FN + L - low performance mode\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_M:
> +		dev_dbg(ec->dev, "FN + M - medium performance mode\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_H:
> +		dev_dbg(ec->dev, "FN + H - high performance mode\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_T:
> +		dev_dbg(ec->dev, "FN + T - toggle intelligent cooling mode\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_D:
> +		dev_dbg(ec->dev, "FN + D - toggle privacy guard mode\n");
> +		break;
> +	default:
> +		dev_info(ec->dev, "Unknown EC event: 0x%02x\n", val);
> +		break;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int thinkpad_t14s_ec_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct thinkpad_t14s_ec *ec;
> +	int ret;
> +
> +	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> +	if (!ec)
> +		return -ENOMEM;
> +
> +	ec->dev = dev;
> +
> +	ec->regmap = devm_regmap_init(dev, &thinkpad_t14s_ec_regmap_bus,
> +				      ec, &thinkpad_t14s_ec_regmap_config);
> +	if (IS_ERR(ec->regmap))
> +		return dev_err_probe(dev, PTR_ERR(ec->regmap),
> +				     "Failed to init regmap\n");
> +
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +					thinkpad_t14s_ec_irq_handler,
> +					IRQF_ONESHOT, dev_name(dev), ec);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to get IRQ\n");
> +
> +	ret = thinkpad_t14s_leds_probe(ec);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = thinkpad_t14s_kbd_backlight_probe(ec);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = thinkpad_t14s_kbd_audio_led_probe(ec);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = thinkpad_t14s_input_probe(ec);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Disable wakeup support by default, because the driver currently does
> +	 * not support masking any events and the laptop should not wake up when
> +	 * the LID is closed.
> +	 */
> +	device_wakeup_disable(dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id thinkpad_t14s_ec_of_match[] = {
> +	{ .compatible = "lenovo,thinkpad-t14s-ec" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, thinkpad_t14s_ec_of_match);
> +
> +static const struct i2c_device_id thinkpad_t14s_ec_i2c_id_table[] = {
> +	{ "thinkpad-t14s-ec", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, thinkpad_t14s_ec_i2c_id_table);
> +
> +static struct i2c_driver thinkpad_t14s_ec_i2c_driver = {
> +	.driver = {
> +		.name = "thinkpad-t14s-ec",
> +		.of_match_table = thinkpad_t14s_ec_of_match
> +	},
> +	.probe = thinkpad_t14s_ec_probe,
> +	.id_table = thinkpad_t14s_ec_i2c_id_table,
> +};
> +module_i2c_driver(thinkpad_t14s_ec_i2c_driver);
> +
> +MODULE_AUTHOR("Sebastian Reichel <sre@kernel.org>");
> +MODULE_DESCRIPTION("Lenovo Thinkpad T14s Embedded Controller");
> +MODULE_LICENSE("GPL");
> 

Aside from those few nits, great to see someone take this on, glorious 
in fact.

I don't have this particular hardware myself so I can't test but:

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

---
bod


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/3] platform: arm64: thinkpad-t14s-ec: new driver
  2025-08-31 21:28 [PATCH 0/3] platform: arm64: thinkpad-t14s-ec: new driver Sebastian Reichel
                   ` (2 preceding siblings ...)
  2025-08-31 21:28 ` [PATCH 3/3] arm64: dts: qcom: x1e80100-t14s: add EC Sebastian Reichel
@ 2025-09-01  9:03 ` Neil Armstrong
  2025-09-02 13:17 ` Rob Herring (Arm)
  4 siblings, 0 replies; 18+ messages in thread
From: Neil Armstrong @ 2025-09-01  9:03 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
	Bjorn Andersson, Konrad Dybcio, Mark Pearson
  Cc: Derek J. Clark, Henrique de Moraes Holschuh, devicetree,
	linux-kernel, platform-driver-x86, linux-arm-msm

On 31/08/2025 23:28, Sebastian Reichel wrote:
> Introduce driver for the ThinkPad T14s Gen6 Snapdragon EC. In theory
> it seems to be compatible with the ThinkPad ACPI driver, but these
> devices are booted with device tree. As the name implies, the existing
> ThinkPad ACPI driver only supports the ACPI interface. Looking at
> the implementation, the ACPI DSDT contains many mapping functions
> to translate the low level I2C messages into the interface used by
> the ThinkPad ACPI driver. Adding DT support to the ThinkPad ACPI driver
> would require adding all those translation functions, which would add
> more or less the same amount of code as writing a separate driver using
> the low level interface directly. I don't think it's sensible to make
> the existing ACPI driver even more complicated, so I went for a separate
> driver.
> 
> I managed to get system LEDs, audio LEDs, extra keys and the keyboard
> backlight control working. The EC also seems to be used for some thermal
> bits, which I haven't looked into deeply. As far as I understand most
> thermal and fan control is handled by a different controller
> (0x36@i2c5) anyways.
> 
> Apart from that the EC is involved in proper system suspend, which
> is something I do not yet understand (I don't have any documentation
> apart from the dis-assembled DSDT and existing ACPI driver). Right
> now I disabled wake capabilities for the IRQ, since it would wake
> up the system when closing the LID. Hopefully a way to mask specific
> events will be found in the future.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
> Sebastian Reichel (3):
>        dt-bindings: platform: Add Lenovo Thinkpad T14s EC
>        platform: arm64: thinkpad-t14s-ec: new driver
>        arm64: dts: qcom: x1e80100-t14s: add EC
> 
>   .../bindings/platform/lenovo,thinkpad-t14s-ec.yaml |  49 ++
>   MAINTAINERS                                        |   6 +
>   .../dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi    |  23 +
>   drivers/platform/arm64/Kconfig                     |  20 +
>   drivers/platform/arm64/Makefile                    |   1 +
>   drivers/platform/arm64/lenovo-thinkpad-t14s.c      | 597 +++++++++++++++++++++
>   6 files changed, 696 insertions(+)
> ---
> base-commit: c8bc81a52d5a2ac2e4b257ae123677cf94112755
> change-id: 20250831-thinkpad-t14s-ec-ddeb23dbdafb
> 
> Best regards,

Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on Thinkpad T14S OLED

All worked :-)

Thanks !

Neil

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] dt-bindings: platform: Add Lenovo Thinkpad T14s EC
  2025-08-31 21:28 ` [PATCH 1/3] dt-bindings: platform: Add Lenovo Thinkpad T14s EC Sebastian Reichel
  2025-08-31 23:53   ` Bryan O'Donoghue
@ 2025-09-01  9:28   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 18+ messages in thread
From: Krzysztof Kozlowski @ 2025-09-01  9:28 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
	Bjorn Andersson, Konrad Dybcio, Mark Pearson
  Cc: Derek J. Clark, Henrique de Moraes Holschuh, devicetree,
	linux-kernel, platform-driver-x86, linux-arm-msm

On 31/08/2025 23:28, Sebastian Reichel wrote:
> Add binding for the EC found in the Thinkpad T14s Gen6 Snapdragon,
> which is based on the Qualcomm X1 Elite. Some of the system LEDs
> and extra keys are only accessible via the EC.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>  .../bindings/platform/lenovo,thinkpad-t14s-ec.yaml | 49 ++++++++++++++++++++++

Please place it in embedded-controller. I moved there all ECs.

>  1 file changed, 49 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml b/Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..bab20df2d9ede9a3cb0359944b26b3d18ff7d9b8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/platform/lenovo,thinkpad-t14s-ec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Lenovo Thinkpad T14s Embedded Controller.

Drop full stop, titles never have them.

> +
> +maintainers:
> +  - Sebastian Reichel <sre@kernel.org>
> +
> +description:
> +  The Qualcomm Snapdragon-based Lenovo Thinkpad T14s has an Embedded Controller
> +  (EC) which handles things such as keyboard backlight, LEDs or non-standard keys.

Please wrap at 80.

> +  This binding describes the interface, on an I2C bus, to this EC.

Drop, or just describe the hardware, not binding.

> +
> +properties:
> +  compatible:
> +    const: lenovo,thinkpad-t14s-ec
> +
> +  reg:
> +    const: 0x28
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |+
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c1 {

i2c

> +        clock-frequency = <400000>;

Drop

> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        embedded-controller@28 {
> +            compatible = "lenovo,thinkpad-t14s-ec";
> +            reg = <0x28>;
> +            interrupts-extended = <&tlmm 66 IRQ_TYPE_LEVEL_LOW>;
> +        };
> +    };
> +...
> 


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver
  2025-08-31 21:28 ` [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver Sebastian Reichel
  2025-09-01  8:43   ` Bryan O'Donoghue
@ 2025-09-01  9:51   ` Ilpo Järvinen
  2025-09-01 13:48   ` Mark Pearson
  2 siblings, 0 replies; 18+ messages in thread
From: Ilpo Järvinen @ 2025-09-01  9:51 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Hans de Goede,
	Bryan O'Donoghue, Bjorn Andersson, Konrad Dybcio,
	Mark Pearson, Derek J. Clark, Henrique de Moraes Holschuh,
	devicetree, LKML, platform-driver-x86, linux-arm-msm

On Sun, 31 Aug 2025, Sebastian Reichel wrote:

> Introduce EC driver for the ThinkPad T14s Gen6 Snapdragon, which
> is in theory compatible with ThinkPad ACPI. On Linux the system
> is booted with device tree, which is not supported by the ThinkPad
> ACPI driver. Also most of the hardware compatibility is handled
> via ACPI tables, which are obviously not used when booting via
> device tree. Thus adding DT compatibility to the existing driver
> is not worth it (almost no code sharing).
> 
> The driver currently exposes features, which are not available
> via other means:
> 
>  * Extra Keys
>  * System LEDs
>  * Keyboard Backlight Control
> 
> The driver has been developed by reading the ACPI DSDT. There
> are some more features around thermal control, which are not
> yet supported by the driver.
> 
> The speaker mute and mic mute LEDs need some additional changes
> in the ALSA UCM to be set automatically.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>  MAINTAINERS                                   |   6 +
>  drivers/platform/arm64/Kconfig                |  20 +
>  drivers/platform/arm64/Makefile               |   1 +
>  drivers/platform/arm64/lenovo-thinkpad-t14s.c | 597 ++++++++++++++++++++++++++
>  4 files changed, 624 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e94d68c980c5f8bef2e1caf26b1a775df6aa1d84..589466169c222b2e088c6112a1e724c95e948f72 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25092,6 +25092,12 @@ W:	http://thinkwiki.org/wiki/Ibm-acpi
>  T:	git git://repo.or.cz/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git
>  F:	drivers/platform/x86/lenovo/thinkpad_acpi.c
>  
> +THINKPAD T14S EMBEDDED CONTROLLER DRIVER
> +M:	Sebastian Reichel <sre@kernel.org>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml
> +F:	drivers/platform/arm64/lenovo-thinkpad-t14s.c
> +
>  THINKPAD LMI DRIVER
>  M:	Mark Pearson <mpearson-lenovo@squebb.ca>
>  L:	platform-driver-x86@vger.kernel.org
> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> index 06288aebc5599435065a37f8dacd046b5def80bd..10f905d7d6bfa5fad30a0689d3a20481268c781e 100644
> --- a/drivers/platform/arm64/Kconfig
> +++ b/drivers/platform/arm64/Kconfig
> @@ -70,4 +70,24 @@ config EC_LENOVO_YOGA_C630
>  
>  	  Say M or Y here to include this support.
>  
> +config EC_LENOVO_THINKPAD_T14S
> +	tristate "Lenovo Thinkpad T14s Embedded Controller driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on I2C
> +	depends on INPUT
> +	select INPUT_SPARSEKMAP
> +	select LEDS_CLASS
> +	select NEW_LEDS
> +	select SND_CTL_LED if SND
> +	help
> +	  Driver for the Embedded Controller in the Qualcomm Snapdragon-based
> +	  Lenovo Thinkpad T14s, which provides access to keyboard backlight
> +	  and status LEDs.
> +
> +	  This driver provides support for the mentioned laptop where this
> +	  information is not properly exposed via the standard Qualcomm
> +	  devices.
> +
> +	  Say M or Y here to include this support.
> +
>  endif # ARM64_PLATFORM_DEVICES
> diff --git a/drivers/platform/arm64/Makefile b/drivers/platform/arm64/Makefile
> index 46a99eba3264cc40e812567d1533bb86031a6af3..60c131cff6a15bb51a49c9edab95badf513ee0f6 100644
> --- a/drivers/platform/arm64/Makefile
> +++ b/drivers/platform/arm64/Makefile
> @@ -8,3 +8,4 @@
>  obj-$(CONFIG_EC_ACER_ASPIRE1)	+= acer-aspire1-ec.o
>  obj-$(CONFIG_EC_HUAWEI_GAOKUN)	+= huawei-gaokun-ec.o
>  obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o
> +obj-$(CONFIG_EC_LENOVO_THINKPAD_T14S) += lenovo-thinkpad-t14s.o
> diff --git a/drivers/platform/arm64/lenovo-thinkpad-t14s.c b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ab783870e8eadfe13d83500c7f39440291e42cc9
> --- /dev/null
> +++ b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> @@ -0,0 +1,597 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025, Sebastian Reichel
> + */
> +
> +#define DEBUG

?

> +
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/leds.h>
> +#include <linux/lockdep.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define THINKPAD_T14S_EC_CMD_ECRD 0x02
> +#define THINKPAD_T14S_EC_CMD_ECWR 0x03
> +#define THINKPAD_T14S_EC_CMD_EVT 0xf0
> +
> +#define THINKPAD_T14S_EC_REG_LED	0x0c
> +#define THINKPAD_T14S_EC_REG_KBD_BL1	0x0d
> +#define THINKPAD_T14S_EC_REG_KBD_BL2	0xe1
> +#define THINKPAD_T14S_EC_KBD_BL1_MASK	GENMASK_U8(7, 6)

+ linux/bits.h

> +#define THINKPAD_T14S_EC_KBD_BL1_SHIFT	6
> +#define THINKPAD_T14S_EC_KBD_BL2_MASK	GENMASK_U8(3, 2)
> +#define THINKPAD_T14S_EC_KBD_BL2_SHIFT	2

Use FIELD_GET/PREP() and drop any _SHIFT defines. Don't forget to add 
linux/bitfield.h when using FIELD_*().

> +#define THINKPAD_T14S_EC_REG_AUD	0x30
> +#define THINKPAD_T14S_EC_MIC_MUTE_LED	BIT(5)
> +#define THINKPAD_T14S_EC_SPK_MUTE_LED	BIT(6)
> +
> +#define THINKPAD_T14S_EC_EVT_NONE			0x00
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_4			0x13
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F7			0x16
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_SPACE		0x1F
> +#define THINKPAD_T14S_EC_EVT_KEY_TP_DOUBLE_TAP		0x20
> +#define THINKPAD_T14S_EC_EVT_AC_CONNECTED		0x26
> +#define THINKPAD_T14S_EC_EVT_AC_DISCONNECTED		0x27
> +#define THINKPAD_T14S_EC_EVT_KEY_POWER			0x28
> +#define THINKPAD_T14S_EC_EVT_LID_OPEN			0x2A
> +#define THINKPAD_T14S_EC_EVT_LID_CLOSED			0x2B
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F12			0x62
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_TAB			0x63
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F8			0x64
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F10			0x65
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F4			0x6A
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_D			0x6B
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_T			0x6C
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_H			0x6D
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_M			0x6E
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_L			0x6F
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_RIGHT_SHIFT		0x71
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_ESC			0x74
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_N			0x79
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F11			0x7A
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_G			0x7E
> +
> +enum thinkpad_t14s_ec_led_status_t {
> +	THINKPAD_EC_LED_OFF = 0x00,
> +	THINKPAD_EC_LED_ON = 0x80,
> +	THINKPAD_EC_LED_BLINK = 0xc0,

Align values.

> +};
> +
> +struct thinkpad_t14s_ec_led_classdev {
> +	struct led_classdev led_classdev;
> +	int led;
> +	enum thinkpad_t14s_ec_led_status_t cache;
> +	struct thinkpad_t14s_ec *ec;
> +};
> +
> +struct thinkpad_t14s_ec {
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct thinkpad_t14s_ec_led_classdev led_pwr_btn;
> +	struct thinkpad_t14s_ec_led_classdev led_chrg_orange;
> +	struct thinkpad_t14s_ec_led_classdev led_chrg_white;
> +	struct thinkpad_t14s_ec_led_classdev led_lid_logo_dot;
> +	struct led_classdev kbd_backlight;
> +	struct led_classdev led_mic_mute;
> +	struct led_classdev led_spk_mute;
> +	struct input_dev *inputdev;
> +};
> +
> +static const struct regmap_config thinkpad_t14s_ec_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xff,
> +};
> +
> +static int thinkpad_t14s_ec_write(void *context, unsigned int reg,
> +				  unsigned int val)
> +{
> +	char buf[5] = {THINKPAD_T14S_EC_CMD_ECWR, reg, 0x00, 0x01, val};

Please use u8 instead of char for anything that is binary data.

> +	struct thinkpad_t14s_ec *ec = context;
> +	struct i2c_client *client = to_i2c_client(ec->dev);
> +	int ret;
> +
> +	ret = i2c_master_send(client, buf, sizeof(buf));
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int thinkpad_t14s_ec_read(void *context, unsigned int reg,
> +				 unsigned int *val)
> +{
> +	char buf[4] = {THINKPAD_T14S_EC_CMD_ECRD, reg, 0x00, 0x01};

Ditto.

> +	struct thinkpad_t14s_ec *ec = context;
> +	struct i2c_client *client = to_i2c_client(ec->dev);
> +	struct i2c_msg request, response;
> +	u8 result;
> +	int ret;
> +
> +	request.addr = client->addr;
> +	request.flags = I2C_M_STOP;
> +	request.len = sizeof(buf);
> +	request.buf = buf;
> +	response.addr = client->addr;
> +	response.flags = I2C_M_RD;
> +	response.len = 1;
> +	response.buf = &result;
> +
> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> +	ret = __i2c_transfer(client->adapter, &request, 1);
> +	if (ret < 0)
> +		goto out;
> +	ret = __i2c_transfer(client->adapter, &response, 1);
> +	if (ret < 0)
> +		goto out;
> +
> +	*val = result;
> +	ret = 0;
> +
> +out:
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	return ret;
> +}
> +
> +static const struct regmap_bus thinkpad_t14s_ec_regmap_bus = {
> +	.reg_write = thinkpad_t14s_ec_write,
> +	.reg_read = thinkpad_t14s_ec_read,
> +};
> +
> +static int thinkpad_t14s_ec_read_evt(struct thinkpad_t14s_ec *ec, u8 *val)
> +{
> +	char buf[4] = {THINKPAD_T14S_EC_CMD_EVT, 0x00, 0x00, 0x01};
> +	struct i2c_client *client = to_i2c_client(ec->dev);
> +	struct i2c_msg request, response;
> +	int ret;
> +
> +	request.addr = client->addr;
> +	request.flags = I2C_M_STOP;
> +	request.len = sizeof(buf);
> +	request.buf = buf;
> +	response.addr = client->addr;
> +	response.flags = I2C_M_RD;
> +	response.len = 1;
> +	response.buf = val;
> +
> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> +	ret = __i2c_transfer(client->adapter, &request, 1);
> +	if (ret < 0)
> +		goto out;
> +	ret = __i2c_transfer(client->adapter, &response, 1);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = 0;
> +
> +out:
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	return ret;
> +}
> +
> +static int thinkpad_t14s_led_set_status(struct thinkpad_t14s_ec *ec,
> +			struct thinkpad_t14s_ec_led_classdev *led,
> +			const enum thinkpad_t14s_ec_led_status_t ledstatus)
> +{
> +	int ret;
> +
> +	ret = regmap_write(ec->regmap, THINKPAD_T14S_EC_REG_LED,
> +			   led->led | ledstatus);
> +	if (ret < 0)
> +		return ret;
> +
> +	led->cache = ledstatus;
> +	return 0;
> +}
> +
> +static int thinkpad_t14s_led_set(struct led_classdev *led_cdev,
> +				 enum led_brightness brightness)
> +{
> +	struct thinkpad_t14s_ec_led_classdev *led = container_of(led_cdev,
> +			struct thinkpad_t14s_ec_led_classdev, led_classdev);
> +	enum thinkpad_t14s_ec_led_status_t new_state;
> +
> +	if (brightness == LED_OFF)
> +		new_state = THINKPAD_EC_LED_OFF;
> +	else if (led->cache != THINKPAD_EC_LED_BLINK)
> +		new_state = THINKPAD_EC_LED_ON;
> +	else
> +		new_state = THINKPAD_EC_LED_BLINK;
> +
> +	return thinkpad_t14s_led_set_status(led->ec, led, new_state);
> +}
> +
> +static int thinkpad_t14s_led_blink_set(struct led_classdev *led_cdev,
> +				       unsigned long *delay_on,
> +				       unsigned long *delay_off)
> +{
> +	struct thinkpad_t14s_ec_led_classdev *led = container_of(led_cdev,
> +			struct thinkpad_t14s_ec_led_classdev, led_classdev);
> +
> +	/* Can we choose the flash rate? */
> +	if (*delay_on == 0 && *delay_off == 0) {
> +		/* yes. set them to the hardware blink rate (1 Hz) */
> +		*delay_on = 500; /* ms */
> +		*delay_off = 500; /* ms */
> +	} else if ((*delay_on != 500) || (*delay_off != 500))
> +		return -EINVAL;

Please add a define for this delay with unit in the define's name so you 
can use that instead of literals and unit comments. Since the times are 
the same, I don't think off/on needs own defines.

> +
> +	return thinkpad_t14s_led_set_status(led->ec, led, THINKPAD_EC_LED_BLINK);
> +}
> +
> +static int thinkpad_t14s_init_led(struct thinkpad_t14s_ec *ec,
> +				  struct thinkpad_t14s_ec_led_classdev *led,
> +				  u8 id, const char *name)
> +{
> +	led->led_classdev.name = name;
> +	led->led_classdev.flags = LED_RETAIN_AT_SHUTDOWN;
> +	led->led_classdev.max_brightness = 1;
> +	led->led_classdev.brightness_set_blocking = thinkpad_t14s_led_set;
> +	led->led_classdev.blink_set = thinkpad_t14s_led_blink_set;
> +	led->ec = ec;
> +	led->led = id;
> +
> +	return devm_led_classdev_register(ec->dev, &led->led_classdev);
> +}
> +
> +static int thinkpad_t14s_leds_probe(struct thinkpad_t14s_ec *ec)
> +{
> +	int ret;
> +
> +	ret = thinkpad_t14s_init_led(ec, &ec->led_pwr_btn, 0,
> +				     "platform::power");
> +	if (ret)
> +		return ret;
> +	ret = thinkpad_t14s_init_led(ec, &ec->led_chrg_orange, 1,
> +				     "platform:amber:battery-charging");
> +	if (ret)
> +		return ret;
> +	ret = thinkpad_t14s_init_led(ec, &ec->led_chrg_white, 2,
> +				     "platform:white:battery-full");
> +	if (ret)
> +		return ret;
> +	ret = thinkpad_t14s_init_led(ec, &ec->led_lid_logo_dot, 10,
> +				     "platform::lid_logo_dot");
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int thinkpad_t14s_kbd_bl_set(struct led_classdev *led_cdev,
> +				    enum led_brightness brightness)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, kbd_backlight);
> +	int ret;
> +
> +	ret = regmap_update_bits(ec->regmap, THINKPAD_T14S_EC_REG_KBD_BL1,
> +				 THINKPAD_T14S_EC_KBD_BL1_MASK,
> +				 brightness << THINKPAD_T14S_EC_KBD_BL1_SHIFT);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_update_bits(ec->regmap, THINKPAD_T14S_EC_REG_KBD_BL2,
> +				 THINKPAD_T14S_EC_KBD_BL2_MASK,
> +				 brightness << THINKPAD_T14S_EC_KBD_BL2_SHIFT);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static enum led_brightness thinkpad_t14s_kbd_bl_get(struct led_classdev *led_cdev)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, kbd_backlight);
> +	int ret, val;

Why is val signed?

> +
> +	ret = regmap_read(ec->regmap, THINKPAD_T14S_EC_REG_KBD_BL1, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	val &= THINKPAD_T14S_EC_KBD_BL1_MASK;
> +
> +	return val >> THINKPAD_T14S_EC_KBD_BL1_SHIFT;
> +}
> +
> +static void thinkpad_t14s_kbd_bl_update(struct thinkpad_t14s_ec *ec)
> +{
> +	enum led_brightness brightness = thinkpad_t14s_kbd_bl_get(&ec->kbd_backlight);
> +
> +	led_classdev_notify_brightness_hw_changed(&ec->kbd_backlight, brightness);
> +}
> +
> +static int thinkpad_t14s_kbd_backlight_probe(struct thinkpad_t14s_ec *ec)
> +{
> +	ec->kbd_backlight.name = "platform::kbd_backlight";
> +	ec->kbd_backlight.flags = LED_BRIGHT_HW_CHANGED;
> +	ec->kbd_backlight.max_brightness = 2;
> +	ec->kbd_backlight.brightness_set_blocking = thinkpad_t14s_kbd_bl_set;
> +	ec->kbd_backlight.brightness_get = thinkpad_t14s_kbd_bl_get;
> +
> +	return devm_led_classdev_register(ec->dev, &ec->kbd_backlight);
> +}
> +
> +static enum led_brightness thinkpad_t14s_audio_led_get(struct thinkpad_t14s_ec *ec,
> +						       u8 led_bit)
> +{
> +	int ret, val;
> +
> +	ret = regmap_read(ec->regmap, THINKPAD_T14S_EC_REG_AUD, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return !!(val && led_bit);

Please don't rely on implicit conversion for bool -> enum led_brightness!

> +}
> +
> +static enum led_brightness thinkpad_t14s_audio_led_set(struct thinkpad_t14s_ec *ec,
> +						       u8 led_bit,
> +						       enum led_brightness brightness)
> +{
> +	u8 val = brightness ? led_bit : 0;
> +
> +	return regmap_update_bits(ec->regmap, THINKPAD_T14S_EC_REG_AUD, led_bit, val);
> +}
> +
> +static enum led_brightness thinkpad_t14s_mic_mute_led_get(struct led_classdev *led_cdev)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, led_mic_mute);
> +
> +	return thinkpad_t14s_audio_led_get(ec, THINKPAD_T14S_EC_MIC_MUTE_LED);
> +}
> +
> +static int thinkpad_t14s_mic_mute_led_set(struct led_classdev *led_cdev,
> +					  enum led_brightness brightness)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, led_mic_mute);
> +
> +	return thinkpad_t14s_audio_led_set(ec, THINKPAD_T14S_EC_MIC_MUTE_LED, brightness);
> +}
> +
> +static enum led_brightness thinkpad_t14s_spk_mute_led_get(struct led_classdev *led_cdev)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, led_spk_mute);
> +
> +	return thinkpad_t14s_audio_led_get(ec, THINKPAD_T14S_EC_SPK_MUTE_LED);
> +}
> +
> +static int thinkpad_t14s_spk_mute_led_set(struct led_classdev *led_cdev,
> +					  enum led_brightness brightness)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, led_spk_mute);
> +
> +	return thinkpad_t14s_audio_led_set(ec, THINKPAD_T14S_EC_SPK_MUTE_LED, brightness);
> +}
> +
> +static int thinkpad_t14s_kbd_audio_led_probe(struct thinkpad_t14s_ec *ec)
> +{
> +	int ret;
> +
> +	ec->led_mic_mute.name = "platform::micmute";
> +	ec->led_mic_mute.max_brightness = 1,
> +	ec->led_mic_mute.default_trigger = "audio-micmute",
> +	ec->led_mic_mute.brightness_set_blocking = thinkpad_t14s_mic_mute_led_set;
> +	ec->led_mic_mute.brightness_get = thinkpad_t14s_mic_mute_led_get;
> +
> +	ec->led_spk_mute.name = "platform::mute";
> +	ec->led_spk_mute.max_brightness = 1,
> +	ec->led_spk_mute.default_trigger = "audio-mute",
> +	ec->led_spk_mute.brightness_set_blocking = thinkpad_t14s_spk_mute_led_set;
> +	ec->led_spk_mute.brightness_get = thinkpad_t14s_spk_mute_led_get;
> +
> +	ret = devm_led_classdev_register(ec->dev, &ec->led_mic_mute);
> +	if (ret)
> +		return ret;
> +
> +	return devm_led_classdev_register(ec->dev, &ec->led_spk_mute);
> +}
> +
> +/*
> + * using code 0x16 ignores the provided KEY code and use KEY_MUTE,
> + * so all codes have a 0x1000 offset
> + */
> +static const struct key_entry thinkpad_t14s_keymap[] = {
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_4, { KEY_SLEEP } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_N, { KEY_VENDOR } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F4, { KEY_MICMUTE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F7, { KEY_SWITCHVIDEOMODE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F8, { KEY_MODE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F10, { KEY_SELECTIVE_SCREENSHOT } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F11, { KEY_LINK_PHONE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F12, { KEY_BOOKMARKS } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_SPACE, { KEY_KBDILLUMTOGGLE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_ESC, { KEY_FN_ESC } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_TAB, { KEY_ZOOM } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_RIGHT_SHIFT, { KEY_FN_RIGHT_SHIFT } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_TP_DOUBLE_TAP, { KEY_PROG4 } },
> +	{ KE_END }
> +};
> +
> +static int thinkpad_t14s_input_probe(struct thinkpad_t14s_ec *ec)
> +{
> +	int ret;
> +
> +	ec->inputdev = devm_input_allocate_device(ec->dev);
> +	if (!ec->inputdev)
> +		return -ENOMEM;
> +
> +	ec->inputdev->name = "ThinkPad Extra Buttons";
> +	ec->inputdev->phys = "thinkpad/input0";
> +	ec->inputdev->id.bustype = BUS_HOST;
> +	ec->inputdev->dev.parent = ec->dev;
> +
> +	ret = sparse_keymap_setup(ec->inputdev, thinkpad_t14s_keymap, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = input_register_device(ec->inputdev);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static irqreturn_t thinkpad_t14s_ec_irq_handler(int irq, void *data)
> +{
> +	struct thinkpad_t14s_ec *ec = data;
> +	int ret;
> +	u8 val;
> +
> +	ret = thinkpad_t14s_ec_read_evt(ec, &val);
> +	if (ret < 0) {
> +		dev_err(ec->dev, "Failed to read event\n");
> +		return IRQ_HANDLED;
> +	}
> +
> +	switch (val) {
> +	case THINKPAD_T14S_EC_EVT_NONE:
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_SPACE:
> +		thinkpad_t14s_kbd_bl_update(ec);
> +		fallthrough;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F4:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F7:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_4:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F8:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F12:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_TAB:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F10:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_N:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F11:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_ESC:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_RIGHT_SHIFT:
> +	case THINKPAD_T14S_EC_EVT_KEY_TP_DOUBLE_TAP:
> +		sparse_keymap_report_event(ec->inputdev, 0x1000 + val, 1, true);

This 0x1000 should be defined.

> +		break;
> +	case THINKPAD_T14S_EC_EVT_AC_CONNECTED:
> +		dev_dbg(ec->dev, "AC connected\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_AC_DISCONNECTED:
> +		dev_dbg(ec->dev, "AC disconnected\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_POWER:
> +		dev_dbg(ec->dev, "power button\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_LID_OPEN:
> +		dev_dbg(ec->dev, "LID open\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_LID_CLOSED:
> +		dev_dbg(ec->dev, "LID closed\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_G:
> +		dev_dbg(ec->dev, "FN + G - toggle double-tapping\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_L:
> +		dev_dbg(ec->dev, "FN + L - low performance mode\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_M:
> +		dev_dbg(ec->dev, "FN + M - medium performance mode\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_H:
> +		dev_dbg(ec->dev, "FN + H - high performance mode\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_T:
> +		dev_dbg(ec->dev, "FN + T - toggle intelligent cooling mode\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_D:
> +		dev_dbg(ec->dev, "FN + D - toggle privacy guard mode\n");
> +		break;
> +	default:
> +		dev_info(ec->dev, "Unknown EC event: 0x%02x\n", val);
> +		break;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int thinkpad_t14s_ec_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct thinkpad_t14s_ec *ec;
> +	int ret;
> +
> +	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> +	if (!ec)
> +		return -ENOMEM;
> +
> +	ec->dev = dev;
> +
> +	ec->regmap = devm_regmap_init(dev, &thinkpad_t14s_ec_regmap_bus,
> +				      ec, &thinkpad_t14s_ec_regmap_config);
> +	if (IS_ERR(ec->regmap))
> +		return dev_err_probe(dev, PTR_ERR(ec->regmap),
> +				     "Failed to init regmap\n");
> +
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +					thinkpad_t14s_ec_irq_handler,
> +					IRQF_ONESHOT, dev_name(dev), ec);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to get IRQ\n");
> +
> +	ret = thinkpad_t14s_leds_probe(ec);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = thinkpad_t14s_kbd_backlight_probe(ec);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = thinkpad_t14s_kbd_audio_led_probe(ec);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = thinkpad_t14s_input_probe(ec);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Disable wakeup support by default, because the driver currently does
> +	 * not support masking any events and the laptop should not wake up when
> +	 * the LID is closed.
> +	 */
> +	device_wakeup_disable(dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id thinkpad_t14s_ec_of_match[] = {
> +	{ .compatible = "lenovo,thinkpad-t14s-ec" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, thinkpad_t14s_ec_of_match);
> +
> +static const struct i2c_device_id thinkpad_t14s_ec_i2c_id_table[] = {
> +	{ "thinkpad-t14s-ec", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, thinkpad_t14s_ec_i2c_id_table);
> +
> +static struct i2c_driver thinkpad_t14s_ec_i2c_driver = {
> +	.driver = {
> +		.name = "thinkpad-t14s-ec",
> +		.of_match_table = thinkpad_t14s_ec_of_match

Please add a comma to any non-terminator entry.

> +	},
> +	.probe = thinkpad_t14s_ec_probe,
> +	.id_table = thinkpad_t14s_ec_i2c_id_table,
> +};
> +module_i2c_driver(thinkpad_t14s_ec_i2c_driver);
> +
> +MODULE_AUTHOR("Sebastian Reichel <sre@kernel.org>");
> +MODULE_DESCRIPTION("Lenovo Thinkpad T14s Embedded Controller");
> +MODULE_LICENSE("GPL");
> 
> 

-- 
 i.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver
  2025-08-31 21:28 ` [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver Sebastian Reichel
  2025-09-01  8:43   ` Bryan O'Donoghue
  2025-09-01  9:51   ` Ilpo Järvinen
@ 2025-09-01 13:48   ` Mark Pearson
  2025-09-01 16:10     ` Sebastian Reichel
  2 siblings, 1 reply; 18+ messages in thread
From: Mark Pearson @ 2025-09-01 13:48 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
	Bjorn Andersson, Konrad Dybcio
  Cc: Derek J . Clark, Henrique de Moraes Holschuh, devicetree,
	linux-kernel, platform-driver-x86@vger.kernel.org, linux-arm-msm

Hi Sebastian,

On Sun, Aug 31, 2025, at 5:28 PM, Sebastian Reichel wrote:
> Introduce EC driver for the ThinkPad T14s Gen6 Snapdragon, which
> is in theory compatible with ThinkPad ACPI. On Linux the system
> is booted with device tree, which is not supported by the ThinkPad
> ACPI driver. Also most of the hardware compatibility is handled
> via ACPI tables, which are obviously not used when booting via
> device tree. Thus adding DT compatibility to the existing driver
> is not worth it (almost no code sharing).
>
> The driver currently exposes features, which are not available
> via other means:
>
>  * Extra Keys
>  * System LEDs
>  * Keyboard Backlight Control
>
> The driver has been developed by reading the ACPI DSDT. There
> are some more features around thermal control, which are not
> yet supported by the driver.
>

Thanks for working on this - it's great.

I'll see if I can get the EC spec so I can do some checking on the values (I thought I had it already, but I can't find it). If this file can be used for other platforms then it might be good to rename the file to not be specific to the t14s? I'm curious if it can be used on the X13s or the Yoga platform.

Couple of notes
 - I do agree it doesn't make sense to add this to thinkpad_acpi. That file is too big anyway.
 - If there are other pieces like this where some detail of the platform is needed, please do let me know. I never got enough time to work on this platform directly, and it wasn't in our Linux program, but I do have access and support from the platform team for getting details on it. If I can help, so not too much reverse engineering is needed, I'm happy to.

Mark

> The speaker mute and mic mute LEDs need some additional changes
> in the ALSA UCM to be set automatically.
>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>  MAINTAINERS                                   |   6 +
>  drivers/platform/arm64/Kconfig                |  20 +
>  drivers/platform/arm64/Makefile               |   1 +
>  drivers/platform/arm64/lenovo-thinkpad-t14s.c | 597 ++++++++++++++++++++++++++
>  4 files changed, 624 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 
> e94d68c980c5f8bef2e1caf26b1a775df6aa1d84..589466169c222b2e088c6112a1e724c95e948f72 
> 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25092,6 +25092,12 @@ W:	http://thinkwiki.org/wiki/Ibm-acpi
>  T:	git git://repo.or.cz/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git
>  F:	drivers/platform/x86/lenovo/thinkpad_acpi.c
> 
> +THINKPAD T14S EMBEDDED CONTROLLER DRIVER
> +M:	Sebastian Reichel <sre@kernel.org>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/platform/lenovo,thinkpad-t14s-ec.yaml
> +F:	drivers/platform/arm64/lenovo-thinkpad-t14s.c
> +
>  THINKPAD LMI DRIVER
>  M:	Mark Pearson <mpearson-lenovo@squebb.ca>
>  L:	platform-driver-x86@vger.kernel.org
> diff --git a/drivers/platform/arm64/Kconfig 
> b/drivers/platform/arm64/Kconfig
> index 
> 06288aebc5599435065a37f8dacd046b5def80bd..10f905d7d6bfa5fad30a0689d3a20481268c781e 
> 100644
> --- a/drivers/platform/arm64/Kconfig
> +++ b/drivers/platform/arm64/Kconfig
> @@ -70,4 +70,24 @@ config EC_LENOVO_YOGA_C630
> 
>  	  Say M or Y here to include this support.
> 
> +config EC_LENOVO_THINKPAD_T14S
> +	tristate "Lenovo Thinkpad T14s Embedded Controller driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on I2C
> +	depends on INPUT
> +	select INPUT_SPARSEKMAP
> +	select LEDS_CLASS
> +	select NEW_LEDS
> +	select SND_CTL_LED if SND
> +	help
> +	  Driver for the Embedded Controller in the Qualcomm Snapdragon-based
> +	  Lenovo Thinkpad T14s, which provides access to keyboard backlight
> +	  and status LEDs.
> +
> +	  This driver provides support for the mentioned laptop where this
> +	  information is not properly exposed via the standard Qualcomm
> +	  devices.
> +
> +	  Say M or Y here to include this support.
> +
>  endif # ARM64_PLATFORM_DEVICES
> diff --git a/drivers/platform/arm64/Makefile 
> b/drivers/platform/arm64/Makefile
> index 
> 46a99eba3264cc40e812567d1533bb86031a6af3..60c131cff6a15bb51a49c9edab95badf513ee0f6 
> 100644
> --- a/drivers/platform/arm64/Makefile
> +++ b/drivers/platform/arm64/Makefile
> @@ -8,3 +8,4 @@
>  obj-$(CONFIG_EC_ACER_ASPIRE1)	+= acer-aspire1-ec.o
>  obj-$(CONFIG_EC_HUAWEI_GAOKUN)	+= huawei-gaokun-ec.o
>  obj-$(CONFIG_EC_LENOVO_YOGA_C630) += lenovo-yoga-c630.o
> +obj-$(CONFIG_EC_LENOVO_THINKPAD_T14S) += lenovo-thinkpad-t14s.o
> diff --git a/drivers/platform/arm64/lenovo-thinkpad-t14s.c 
> b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> new file mode 100644
> index 
> 0000000000000000000000000000000000000000..ab783870e8eadfe13d83500c7f39440291e42cc9
> --- /dev/null
> +++ b/drivers/platform/arm64/lenovo-thinkpad-t14s.c
> @@ -0,0 +1,597 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025, Sebastian Reichel
> + */
> +
> +#define DEBUG
> +
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/leds.h>
> +#include <linux/lockdep.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define THINKPAD_T14S_EC_CMD_ECRD 0x02
> +#define THINKPAD_T14S_EC_CMD_ECWR 0x03
> +#define THINKPAD_T14S_EC_CMD_EVT 0xf0
> +
> +#define THINKPAD_T14S_EC_REG_LED	0x0c
> +#define THINKPAD_T14S_EC_REG_KBD_BL1	0x0d
> +#define THINKPAD_T14S_EC_REG_KBD_BL2	0xe1
> +#define THINKPAD_T14S_EC_KBD_BL1_MASK	GENMASK_U8(7, 6)
> +#define THINKPAD_T14S_EC_KBD_BL1_SHIFT	6
> +#define THINKPAD_T14S_EC_KBD_BL2_MASK	GENMASK_U8(3, 2)
> +#define THINKPAD_T14S_EC_KBD_BL2_SHIFT	2
> +#define THINKPAD_T14S_EC_REG_AUD	0x30
> +#define THINKPAD_T14S_EC_MIC_MUTE_LED	BIT(5)
> +#define THINKPAD_T14S_EC_SPK_MUTE_LED	BIT(6)
> +
> +#define THINKPAD_T14S_EC_EVT_NONE			0x00
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_4			0x13
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F7			0x16
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_SPACE		0x1F
> +#define THINKPAD_T14S_EC_EVT_KEY_TP_DOUBLE_TAP		0x20
> +#define THINKPAD_T14S_EC_EVT_AC_CONNECTED		0x26
> +#define THINKPAD_T14S_EC_EVT_AC_DISCONNECTED		0x27
> +#define THINKPAD_T14S_EC_EVT_KEY_POWER			0x28
> +#define THINKPAD_T14S_EC_EVT_LID_OPEN			0x2A
> +#define THINKPAD_T14S_EC_EVT_LID_CLOSED			0x2B
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F12			0x62
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_TAB			0x63
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F8			0x64
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F10			0x65
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F4			0x6A
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_D			0x6B
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_T			0x6C
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_H			0x6D
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_M			0x6E
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_L			0x6F
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_RIGHT_SHIFT		0x71
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_ESC			0x74
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_N			0x79
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_F11			0x7A
> +#define THINKPAD_T14S_EC_EVT_KEY_FN_G			0x7E
> +
> +enum thinkpad_t14s_ec_led_status_t {
> +	THINKPAD_EC_LED_OFF = 0x00,
> +	THINKPAD_EC_LED_ON = 0x80,
> +	THINKPAD_EC_LED_BLINK = 0xc0,
> +};
> +
> +struct thinkpad_t14s_ec_led_classdev {
> +	struct led_classdev led_classdev;
> +	int led;
> +	enum thinkpad_t14s_ec_led_status_t cache;
> +	struct thinkpad_t14s_ec *ec;
> +};
> +
> +struct thinkpad_t14s_ec {
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct thinkpad_t14s_ec_led_classdev led_pwr_btn;
> +	struct thinkpad_t14s_ec_led_classdev led_chrg_orange;
> +	struct thinkpad_t14s_ec_led_classdev led_chrg_white;
> +	struct thinkpad_t14s_ec_led_classdev led_lid_logo_dot;
> +	struct led_classdev kbd_backlight;
> +	struct led_classdev led_mic_mute;
> +	struct led_classdev led_spk_mute;
> +	struct input_dev *inputdev;
> +};
> +
> +static const struct regmap_config thinkpad_t14s_ec_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xff,
> +};
> +
> +static int thinkpad_t14s_ec_write(void *context, unsigned int reg,
> +				  unsigned int val)
> +{
> +	char buf[5] = {THINKPAD_T14S_EC_CMD_ECWR, reg, 0x00, 0x01, val};
> +	struct thinkpad_t14s_ec *ec = context;
> +	struct i2c_client *client = to_i2c_client(ec->dev);
> +	int ret;
> +
> +	ret = i2c_master_send(client, buf, sizeof(buf));
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int thinkpad_t14s_ec_read(void *context, unsigned int reg,
> +				 unsigned int *val)
> +{
> +	char buf[4] = {THINKPAD_T14S_EC_CMD_ECRD, reg, 0x00, 0x01};
> +	struct thinkpad_t14s_ec *ec = context;
> +	struct i2c_client *client = to_i2c_client(ec->dev);
> +	struct i2c_msg request, response;
> +	u8 result;
> +	int ret;
> +
> +	request.addr = client->addr;
> +	request.flags = I2C_M_STOP;
> +	request.len = sizeof(buf);
> +	request.buf = buf;
> +	response.addr = client->addr;
> +	response.flags = I2C_M_RD;
> +	response.len = 1;
> +	response.buf = &result;
> +
> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> +	ret = __i2c_transfer(client->adapter, &request, 1);
> +	if (ret < 0)
> +		goto out;
> +	ret = __i2c_transfer(client->adapter, &response, 1);
> +	if (ret < 0)
> +		goto out;
> +
> +	*val = result;
> +	ret = 0;
> +
> +out:
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	return ret;
> +}
> +
> +static const struct regmap_bus thinkpad_t14s_ec_regmap_bus = {
> +	.reg_write = thinkpad_t14s_ec_write,
> +	.reg_read = thinkpad_t14s_ec_read,
> +};
> +
> +static int thinkpad_t14s_ec_read_evt(struct thinkpad_t14s_ec *ec, u8 
> *val)
> +{
> +	char buf[4] = {THINKPAD_T14S_EC_CMD_EVT, 0x00, 0x00, 0x01};
> +	struct i2c_client *client = to_i2c_client(ec->dev);
> +	struct i2c_msg request, response;
> +	int ret;
> +
> +	request.addr = client->addr;
> +	request.flags = I2C_M_STOP;
> +	request.len = sizeof(buf);
> +	request.buf = buf;
> +	response.addr = client->addr;
> +	response.flags = I2C_M_RD;
> +	response.len = 1;
> +	response.buf = val;
> +
> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> +	ret = __i2c_transfer(client->adapter, &request, 1);
> +	if (ret < 0)
> +		goto out;
> +	ret = __i2c_transfer(client->adapter, &response, 1);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = 0;
> +
> +out:
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +	return ret;
> +}
> +
> +static int thinkpad_t14s_led_set_status(struct thinkpad_t14s_ec *ec,
> +			struct thinkpad_t14s_ec_led_classdev *led,
> +			const enum thinkpad_t14s_ec_led_status_t ledstatus)
> +{
> +	int ret;
> +
> +	ret = regmap_write(ec->regmap, THINKPAD_T14S_EC_REG_LED,
> +			   led->led | ledstatus);
> +	if (ret < 0)
> +		return ret;
> +
> +	led->cache = ledstatus;
> +	return 0;
> +}
> +
> +static int thinkpad_t14s_led_set(struct led_classdev *led_cdev,
> +				 enum led_brightness brightness)
> +{
> +	struct thinkpad_t14s_ec_led_classdev *led = container_of(led_cdev,
> +			struct thinkpad_t14s_ec_led_classdev, led_classdev);
> +	enum thinkpad_t14s_ec_led_status_t new_state;
> +
> +	if (brightness == LED_OFF)
> +		new_state = THINKPAD_EC_LED_OFF;
> +	else if (led->cache != THINKPAD_EC_LED_BLINK)
> +		new_state = THINKPAD_EC_LED_ON;
> +	else
> +		new_state = THINKPAD_EC_LED_BLINK;
> +
> +	return thinkpad_t14s_led_set_status(led->ec, led, new_state);
> +}
> +
> +static int thinkpad_t14s_led_blink_set(struct led_classdev *led_cdev,
> +				       unsigned long *delay_on,
> +				       unsigned long *delay_off)
> +{
> +	struct thinkpad_t14s_ec_led_classdev *led = container_of(led_cdev,
> +			struct thinkpad_t14s_ec_led_classdev, led_classdev);
> +
> +	/* Can we choose the flash rate? */
> +	if (*delay_on == 0 && *delay_off == 0) {
> +		/* yes. set them to the hardware blink rate (1 Hz) */
> +		*delay_on = 500; /* ms */
> +		*delay_off = 500; /* ms */
> +	} else if ((*delay_on != 500) || (*delay_off != 500))
> +		return -EINVAL;
> +
> +	return thinkpad_t14s_led_set_status(led->ec, led, 
> THINKPAD_EC_LED_BLINK);
> +}
> +
> +static int thinkpad_t14s_init_led(struct thinkpad_t14s_ec *ec,
> +				  struct thinkpad_t14s_ec_led_classdev *led,
> +				  u8 id, const char *name)
> +{
> +	led->led_classdev.name = name;
> +	led->led_classdev.flags = LED_RETAIN_AT_SHUTDOWN;
> +	led->led_classdev.max_brightness = 1;
> +	led->led_classdev.brightness_set_blocking = thinkpad_t14s_led_set;
> +	led->led_classdev.blink_set = thinkpad_t14s_led_blink_set;
> +	led->ec = ec;
> +	led->led = id;
> +
> +	return devm_led_classdev_register(ec->dev, &led->led_classdev);
> +}
> +
> +static int thinkpad_t14s_leds_probe(struct thinkpad_t14s_ec *ec)
> +{
> +	int ret;
> +
> +	ret = thinkpad_t14s_init_led(ec, &ec->led_pwr_btn, 0,
> +				     "platform::power");
> +	if (ret)
> +		return ret;
> +	ret = thinkpad_t14s_init_led(ec, &ec->led_chrg_orange, 1,
> +				     "platform:amber:battery-charging");
> +	if (ret)
> +		return ret;
> +	ret = thinkpad_t14s_init_led(ec, &ec->led_chrg_white, 2,
> +				     "platform:white:battery-full");
> +	if (ret)
> +		return ret;
> +	ret = thinkpad_t14s_init_led(ec, &ec->led_lid_logo_dot, 10,
> +				     "platform::lid_logo_dot");
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int thinkpad_t14s_kbd_bl_set(struct led_classdev *led_cdev,
> +				    enum led_brightness brightness)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, kbd_backlight);
> +	int ret;
> +
> +	ret = regmap_update_bits(ec->regmap, THINKPAD_T14S_EC_REG_KBD_BL1,
> +				 THINKPAD_T14S_EC_KBD_BL1_MASK,
> +				 brightness << THINKPAD_T14S_EC_KBD_BL1_SHIFT);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_update_bits(ec->regmap, THINKPAD_T14S_EC_REG_KBD_BL2,
> +				 THINKPAD_T14S_EC_KBD_BL2_MASK,
> +				 brightness << THINKPAD_T14S_EC_KBD_BL2_SHIFT);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static enum led_brightness thinkpad_t14s_kbd_bl_get(struct 
> led_classdev *led_cdev)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, kbd_backlight);
> +	int ret, val;
> +
> +	ret = regmap_read(ec->regmap, THINKPAD_T14S_EC_REG_KBD_BL1, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	val &= THINKPAD_T14S_EC_KBD_BL1_MASK;
> +
> +	return val >> THINKPAD_T14S_EC_KBD_BL1_SHIFT;
> +}
> +
> +static void thinkpad_t14s_kbd_bl_update(struct thinkpad_t14s_ec *ec)
> +{
> +	enum led_brightness brightness = 
> thinkpad_t14s_kbd_bl_get(&ec->kbd_backlight);
> +
> +	led_classdev_notify_brightness_hw_changed(&ec->kbd_backlight, 
> brightness);
> +}
> +
> +static int thinkpad_t14s_kbd_backlight_probe(struct thinkpad_t14s_ec 
> *ec)
> +{
> +	ec->kbd_backlight.name = "platform::kbd_backlight";
> +	ec->kbd_backlight.flags = LED_BRIGHT_HW_CHANGED;
> +	ec->kbd_backlight.max_brightness = 2;
> +	ec->kbd_backlight.brightness_set_blocking = thinkpad_t14s_kbd_bl_set;
> +	ec->kbd_backlight.brightness_get = thinkpad_t14s_kbd_bl_get;
> +
> +	return devm_led_classdev_register(ec->dev, &ec->kbd_backlight);
> +}
> +
> +static enum led_brightness thinkpad_t14s_audio_led_get(struct 
> thinkpad_t14s_ec *ec,
> +						       u8 led_bit)
> +{
> +	int ret, val;
> +
> +	ret = regmap_read(ec->regmap, THINKPAD_T14S_EC_REG_AUD, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return !!(val && led_bit);
> +}
> +
> +static enum led_brightness thinkpad_t14s_audio_led_set(struct 
> thinkpad_t14s_ec *ec,
> +						       u8 led_bit,
> +						       enum led_brightness brightness)
> +{
> +	u8 val = brightness ? led_bit : 0;
> +
> +	return regmap_update_bits(ec->regmap, THINKPAD_T14S_EC_REG_AUD, 
> led_bit, val);
> +}
> +
> +static enum led_brightness thinkpad_t14s_mic_mute_led_get(struct 
> led_classdev *led_cdev)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, led_mic_mute);
> +
> +	return thinkpad_t14s_audio_led_get(ec, THINKPAD_T14S_EC_MIC_MUTE_LED);
> +}
> +
> +static int thinkpad_t14s_mic_mute_led_set(struct led_classdev 
> *led_cdev,
> +					  enum led_brightness brightness)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, led_mic_mute);
> +
> +	return thinkpad_t14s_audio_led_set(ec, THINKPAD_T14S_EC_MIC_MUTE_LED, 
> brightness);
> +}
> +
> +static enum led_brightness thinkpad_t14s_spk_mute_led_get(struct 
> led_classdev *led_cdev)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, led_spk_mute);
> +
> +	return thinkpad_t14s_audio_led_get(ec, THINKPAD_T14S_EC_SPK_MUTE_LED);
> +}
> +
> +static int thinkpad_t14s_spk_mute_led_set(struct led_classdev 
> *led_cdev,
> +					  enum led_brightness brightness)
> +{
> +	struct thinkpad_t14s_ec *ec = container_of(led_cdev,
> +					struct thinkpad_t14s_ec, led_spk_mute);
> +
> +	return thinkpad_t14s_audio_led_set(ec, THINKPAD_T14S_EC_SPK_MUTE_LED, 
> brightness);
> +}
> +
> +static int thinkpad_t14s_kbd_audio_led_probe(struct thinkpad_t14s_ec 
> *ec)
> +{
> +	int ret;
> +
> +	ec->led_mic_mute.name = "platform::micmute";
> +	ec->led_mic_mute.max_brightness = 1,
> +	ec->led_mic_mute.default_trigger = "audio-micmute",
> +	ec->led_mic_mute.brightness_set_blocking = 
> thinkpad_t14s_mic_mute_led_set;
> +	ec->led_mic_mute.brightness_get = thinkpad_t14s_mic_mute_led_get;
> +
> +	ec->led_spk_mute.name = "platform::mute";
> +	ec->led_spk_mute.max_brightness = 1,
> +	ec->led_spk_mute.default_trigger = "audio-mute",
> +	ec->led_spk_mute.brightness_set_blocking = 
> thinkpad_t14s_spk_mute_led_set;
> +	ec->led_spk_mute.brightness_get = thinkpad_t14s_spk_mute_led_get;
> +
> +	ret = devm_led_classdev_register(ec->dev, &ec->led_mic_mute);
> +	if (ret)
> +		return ret;
> +
> +	return devm_led_classdev_register(ec->dev, &ec->led_spk_mute);
> +}
> +
> +/*
> + * using code 0x16 ignores the provided KEY code and use KEY_MUTE,
> + * so all codes have a 0x1000 offset
> + */
> +static const struct key_entry thinkpad_t14s_keymap[] = {
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_4, { KEY_SLEEP } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_N, { KEY_VENDOR } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F4, { KEY_MICMUTE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F7, { 
> KEY_SWITCHVIDEOMODE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F8, { KEY_MODE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F10, { 
> KEY_SELECTIVE_SCREENSHOT } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F11, { KEY_LINK_PHONE 
> } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_F12, { KEY_BOOKMARKS } 
> },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_SPACE, { 
> KEY_KBDILLUMTOGGLE } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_ESC, { KEY_FN_ESC } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_TAB, { KEY_ZOOM } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_FN_RIGHT_SHIFT, { 
> KEY_FN_RIGHT_SHIFT } },
> +	{ KE_KEY, 0x1000 + THINKPAD_T14S_EC_EVT_KEY_TP_DOUBLE_TAP, { 
> KEY_PROG4 } },
> +	{ KE_END }
> +};
> +
> +static int thinkpad_t14s_input_probe(struct thinkpad_t14s_ec *ec)
> +{
> +	int ret;
> +
> +	ec->inputdev = devm_input_allocate_device(ec->dev);
> +	if (!ec->inputdev)
> +		return -ENOMEM;
> +
> +	ec->inputdev->name = "ThinkPad Extra Buttons";
> +	ec->inputdev->phys = "thinkpad/input0";
> +	ec->inputdev->id.bustype = BUS_HOST;
> +	ec->inputdev->dev.parent = ec->dev;
> +
> +	ret = sparse_keymap_setup(ec->inputdev, thinkpad_t14s_keymap, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = input_register_device(ec->inputdev);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static irqreturn_t thinkpad_t14s_ec_irq_handler(int irq, void *data)
> +{
> +	struct thinkpad_t14s_ec *ec = data;
> +	int ret;
> +	u8 val;
> +
> +	ret = thinkpad_t14s_ec_read_evt(ec, &val);
> +	if (ret < 0) {
> +		dev_err(ec->dev, "Failed to read event\n");
> +		return IRQ_HANDLED;
> +	}
> +
> +	switch (val) {
> +	case THINKPAD_T14S_EC_EVT_NONE:
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_SPACE:
> +		thinkpad_t14s_kbd_bl_update(ec);
> +		fallthrough;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F4:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F7:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_4:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F8:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F12:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_TAB:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F10:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_N:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_F11:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_ESC:
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_RIGHT_SHIFT:
> +	case THINKPAD_T14S_EC_EVT_KEY_TP_DOUBLE_TAP:
> +		sparse_keymap_report_event(ec->inputdev, 0x1000 + val, 1, true);
> +		break;
> +	case THINKPAD_T14S_EC_EVT_AC_CONNECTED:
> +		dev_dbg(ec->dev, "AC connected\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_AC_DISCONNECTED:
> +		dev_dbg(ec->dev, "AC disconnected\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_POWER:
> +		dev_dbg(ec->dev, "power button\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_LID_OPEN:
> +		dev_dbg(ec->dev, "LID open\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_LID_CLOSED:
> +		dev_dbg(ec->dev, "LID closed\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_G:
> +		dev_dbg(ec->dev, "FN + G - toggle double-tapping\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_L:
> +		dev_dbg(ec->dev, "FN + L - low performance mode\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_M:
> +		dev_dbg(ec->dev, "FN + M - medium performance mode\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_H:
> +		dev_dbg(ec->dev, "FN + H - high performance mode\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_T:
> +		dev_dbg(ec->dev, "FN + T - toggle intelligent cooling mode\n");
> +		break;
> +	case THINKPAD_T14S_EC_EVT_KEY_FN_D:
> +		dev_dbg(ec->dev, "FN + D - toggle privacy guard mode\n");
> +		break;
> +	default:
> +		dev_info(ec->dev, "Unknown EC event: 0x%02x\n", val);
> +		break;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int thinkpad_t14s_ec_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct thinkpad_t14s_ec *ec;
> +	int ret;
> +
> +	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> +	if (!ec)
> +		return -ENOMEM;
> +
> +	ec->dev = dev;
> +
> +	ec->regmap = devm_regmap_init(dev, &thinkpad_t14s_ec_regmap_bus,
> +				      ec, &thinkpad_t14s_ec_regmap_config);
> +	if (IS_ERR(ec->regmap))
> +		return dev_err_probe(dev, PTR_ERR(ec->regmap),
> +				     "Failed to init regmap\n");
> +
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +					thinkpad_t14s_ec_irq_handler,
> +					IRQF_ONESHOT, dev_name(dev), ec);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to get IRQ\n");
> +
> +	ret = thinkpad_t14s_leds_probe(ec);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = thinkpad_t14s_kbd_backlight_probe(ec);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = thinkpad_t14s_kbd_audio_led_probe(ec);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = thinkpad_t14s_input_probe(ec);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Disable wakeup support by default, because the driver currently 
> does
> +	 * not support masking any events and the laptop should not wake up 
> when
> +	 * the LID is closed.
> +	 */
> +	device_wakeup_disable(dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id thinkpad_t14s_ec_of_match[] = {
> +	{ .compatible = "lenovo,thinkpad-t14s-ec" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, thinkpad_t14s_ec_of_match);
> +
> +static const struct i2c_device_id thinkpad_t14s_ec_i2c_id_table[] = {
> +	{ "thinkpad-t14s-ec", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, thinkpad_t14s_ec_i2c_id_table);
> +
> +static struct i2c_driver thinkpad_t14s_ec_i2c_driver = {
> +	.driver = {
> +		.name = "thinkpad-t14s-ec",
> +		.of_match_table = thinkpad_t14s_ec_of_match
> +	},
> +	.probe = thinkpad_t14s_ec_probe,
> +	.id_table = thinkpad_t14s_ec_i2c_id_table,
> +};
> +module_i2c_driver(thinkpad_t14s_ec_i2c_driver);
> +
> +MODULE_AUTHOR("Sebastian Reichel <sre@kernel.org>");
> +MODULE_DESCRIPTION("Lenovo Thinkpad T14s Embedded Controller");
> +MODULE_LICENSE("GPL");
>
> -- 
> 2.50.1

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver
  2025-09-01  8:43   ` Bryan O'Donoghue
@ 2025-09-01 15:20     ` Sebastian Reichel
  2025-09-01 15:52       ` Bryan O'Donoghue
  0 siblings, 1 reply; 18+ messages in thread
From: Sebastian Reichel @ 2025-09-01 15:20 UTC (permalink / raw)
  To: Bryan O'Donoghue
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Hans de Goede,
	Ilpo Järvinen, Bjorn Andersson, Konrad Dybcio, Mark Pearson,
	Derek J. Clark, Henrique de Moraes Holschuh, devicetree,
	linux-kernel, platform-driver-x86, linux-arm-msm

Hello Bryan,

On Mon, Sep 01, 2025 at 09:43:11AM +0100, Bryan O'Donoghue wrote:
> On 31/08/2025 22:28, Sebastian Reichel wrote:
> > Introduce EC driver for the ThinkPad T14s Gen6 Snapdragon, which
> > is in theory compatible with ThinkPad ACPI. On Linux the system
> > is booted with device tree, which is not supported by the ThinkPad
> > ACPI driver. Also most of the hardware compatibility is handled
> > via ACPI tables, which are obviously not used when booting via
> > device tree. Thus adding DT compatibility to the existing driver
> > is not worth it (almost no code sharing).
> 
> What is the name of that driver, you should name it in your commit
> log. Lenovo WMI ?

The existing driver is known as "ThinkPad ACPI" and thus already
referenced in the commit message. You can find it here:

drivers/platform/x86/lenovo/thinkpad_acpi.c

Feel free to suggest a specific wording that I can take over, which
would have helped you to figure that out :)

> [...]
> > +	ret = __i2c_transfer(client->adapter, &request, 1);
> > +	if (ret < 0)
> > +		goto out;
> 
> I realise this is your coding style but suggest newline after these gotos.

I will look into using that style in v2 throughout the file.

> [...]
> > +static int thinkpad_t14s_led_blink_set(struct led_classdev *led_cdev,
> > +				       unsigned long *delay_on,
> > +				       unsigned long *delay_off)
> > +{
> > +	struct thinkpad_t14s_ec_led_classdev *led = container_of(led_cdev,
> > +			struct thinkpad_t14s_ec_led_classdev, led_classdev);
> > +
> > +	/* Can we choose the flash rate? */
> > +	if (*delay_on == 0 && *delay_off == 0) {
> > +		/* yes. set them to the hardware blink rate (1 Hz) */
> > +		*delay_on = 500; /* ms */
> > +		*delay_off = 500; /* ms */
> > +	} else if ((*delay_on != 500) || (*delay_off != 500))
> > +		return -EINVAL;
> 
> Those 500s should probably be defines

Ack.

> Aside from those few nits, great to see someone take this on, glorious in
> fact.
> 
> I don't have this particular hardware myself so I can't test but:
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Thanks for the review.

Greetings,

-- Sebastian

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver
  2025-09-01 15:20     ` Sebastian Reichel
@ 2025-09-01 15:52       ` Bryan O'Donoghue
  0 siblings, 0 replies; 18+ messages in thread
From: Bryan O'Donoghue @ 2025-09-01 15:52 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Hans de Goede,
	Ilpo Järvinen, Bjorn Andersson, Konrad Dybcio, Mark Pearson,
	Derek J. Clark, Henrique de Moraes Holschuh, devicetree,
	linux-kernel, platform-driver-x86, linux-arm-msm

On 01/09/2025 16:20, Sebastian Reichel wrote:
> You can find it here:
> 
> drivers/platform/x86/lenovo/thinkpad_acpi.c

That'd do ;)

---
bod

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver
  2025-09-01 13:48   ` Mark Pearson
@ 2025-09-01 16:10     ` Sebastian Reichel
  2025-09-01 16:23       ` Neil Armstrong
  2025-09-02 10:27       ` Konrad Dybcio
  0 siblings, 2 replies; 18+ messages in thread
From: Sebastian Reichel @ 2025-09-01 16:10 UTC (permalink / raw)
  To: Mark Pearson
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Hans de Goede,
	Ilpo Järvinen, Bryan O'Donoghue, Bjorn Andersson,
	Konrad Dybcio, Derek J . Clark, Henrique de Moraes Holschuh,
	devicetree, linux-kernel, platform-driver-x86@vger.kernel.org,
	linux-arm-msm

Hello Mark,

On Mon, Sep 01, 2025 at 09:48:39AM -0400, Mark Pearson wrote:
> On Sun, Aug 31, 2025, at 5:28 PM, Sebastian Reichel wrote:
> > Introduce EC driver for the ThinkPad T14s Gen6 Snapdragon, which
> > is in theory compatible with ThinkPad ACPI. On Linux the system
> > is booted with device tree, which is not supported by the ThinkPad
> > ACPI driver. Also most of the hardware compatibility is handled
> > via ACPI tables, which are obviously not used when booting via
> > device tree. Thus adding DT compatibility to the existing driver
> > is not worth it (almost no code sharing).
> >
> > The driver currently exposes features, which are not available
> > via other means:
> >
> >  * Extra Keys
> >  * System LEDs
> >  * Keyboard Backlight Control
> >
> > The driver has been developed by reading the ACPI DSDT. There
> > are some more features around thermal control, which are not
> > yet supported by the driver.
> >
> 
> Thanks for working on this - it's great.

It's a personal scratch your own itch project, as I daily drive the
machine.

> I'll see if I can get the EC spec so I can do some checking on the
> values (I thought I had it already, but I can't find it). If this
> file can be used for other platforms then it might be good to
> rename the file to not be specific to the t14s? I'm curious if it
> can be used on the X13s or the Yoga platform.

Maybe. I only have the T14s (apart of my older Intel/AMD ThinkPads,
which use the ACPI driver). The ACPI DSDT functions completley
abstract the lowlevel I2C interface, so in theory every ThinkPad
could have a completley different EC and still use the same ACPI
driver. So this needs to be checked per-device. Hopefully the low
level interface is similar in those, so that we don't need to spam
the kernel tree with multiple different EC drivers :)

> Couple of notes
>  - I do agree it doesn't make sense to add this to thinkpad_acpi.
>    That file is too big anyway.
>  - If there are other pieces like this where some detail of the
>    platform is needed, please do let me know. I never got enough
>    time to work on this platform directly, and it wasn't in our
>    Linux program, but I do have access and support from the
>    platform team for getting details on it. If I can help, so not
>    too much reverse engineering is needed, I'm happy to.

Thanks for the offer.

I would be interested in bits around system suspend. Right now
support on X1E is limited to sending the CPU into suspend. Much of
the machine seems to be still powered. Right now the keyboard
backlight and all the status LEDs stay on and the LID + power led
does not go into the typical breathing pattern. Additionally I had
to disable wakeup capabilities for the EC interrupt, as closing the
LID generates an event and thus an interrupt, which wakes the
system. Obviousy that is undesired from user's perspective. My guess
is, that there might be some register to mask events, but I haven't
found it so far. Alternatively the EC might mask them automatically
when the system is send into suspend, which I also have not yet
figured out :) The only bit I know is, that EC register 0xE0 is
involved in modern standby.

Apart from that and (probably) unrelated to the EC: I noticed that
accessing the built-in webcam (with the X1E camera patches from
Bryan O'Donoghue) does not enable the status LED. It would be
nice if you can check how that is wired, so that it can be enabled
when a camera stream is started.

Greetings,

-- Sebastian

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver
  2025-09-01 16:10     ` Sebastian Reichel
@ 2025-09-01 16:23       ` Neil Armstrong
  2025-09-02 10:27       ` Konrad Dybcio
  1 sibling, 0 replies; 18+ messages in thread
From: Neil Armstrong @ 2025-09-01 16:23 UTC (permalink / raw)
  To: Sebastian Reichel, Mark Pearson
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Hans de Goede,
	Ilpo Järvinen, Bryan O'Donoghue, Bjorn Andersson,
	Konrad Dybcio, Derek J . Clark, Henrique de Moraes Holschuh,
	devicetree, linux-kernel, platform-driver-x86@vger.kernel.org,
	linux-arm-msm

On 01/09/2025 18:10, Sebastian Reichel wrote:
> Hello Mark,
> 
> On Mon, Sep 01, 2025 at 09:48:39AM -0400, Mark Pearson wrote:
>> On Sun, Aug 31, 2025, at 5:28 PM, Sebastian Reichel wrote:
>>> Introduce EC driver for the ThinkPad T14s Gen6 Snapdragon, which
>>> is in theory compatible with ThinkPad ACPI. On Linux the system
>>> is booted with device tree, which is not supported by the ThinkPad
>>> ACPI driver. Also most of the hardware compatibility is handled
>>> via ACPI tables, which are obviously not used when booting via
>>> device tree. Thus adding DT compatibility to the existing driver
>>> is not worth it (almost no code sharing).
>>>
>>> The driver currently exposes features, which are not available
>>> via other means:
>>>
>>>   * Extra Keys
>>>   * System LEDs
>>>   * Keyboard Backlight Control
>>>
>>> The driver has been developed by reading the ACPI DSDT. There
>>> are some more features around thermal control, which are not
>>> yet supported by the driver.
>>>
>>
>> Thanks for working on this - it's great.
> 
> It's a personal scratch your own itch project, as I daily drive the
> machine.
> 
>> I'll see if I can get the EC spec so I can do some checking on the
>> values (I thought I had it already, but I can't find it). If this
>> file can be used for other platforms then it might be good to
>> rename the file to not be specific to the t14s? I'm curious if it
>> can be used on the X13s or the Yoga platform.
> 
> Maybe. I only have the T14s (apart of my older Intel/AMD ThinkPads,
> which use the ACPI driver). The ACPI DSDT functions completley
> abstract the lowlevel I2C interface, so in theory every ThinkPad
> could have a completley different EC and still use the same ACPI
> driver. So this needs to be checked per-device. Hopefully the low
> level interface is similar in those, so that we don't need to spam
> the kernel tree with multiple different EC drivers :)
> 
>> Couple of notes
>>   - I do agree it doesn't make sense to add this to thinkpad_acpi.
>>     That file is too big anyway.
>>   - If there are other pieces like this where some detail of the
>>     platform is needed, please do let me know. I never got enough
>>     time to work on this platform directly, and it wasn't in our
>>     Linux program, but I do have access and support from the
>>     platform team for getting details on it. If I can help, so not
>>     too much reverse engineering is needed, I'm happy to.
> 
> Thanks for the offer.
> 
> I would be interested in bits around system suspend. Right now
> support on X1E is limited to sending the CPU into suspend. Much of
> the machine seems to be still powered. Right now the keyboard
> backlight and all the status LEDs stay on and the LID + power led
> does not go into the typical breathing pattern. Additionally I had
> to disable wakeup capabilities for the EC interrupt, as closing the
> LID generates an event and thus an interrupt, which wakes the
> system. Obviousy that is undesired from user's perspective. My guess
> is, that there might be some register to mask events, but I haven't
> found it so far. Alternatively the EC might mask them automatically
> when the system is send into suspend, which I also have not yet
> figured out :) The only bit I know is, that EC register 0xE0 is
> involved in modern standby.

I was wondering if there's a command to poweroff the system when still plugged in ?
The actual behavior does a PSCI poweroff using the PMICs but since the EC
keeps the power on and is not aware we want to poweroff, if just reboots.

Neil

> 
> Apart from that and (probably) unrelated to the EC: I noticed that
> accessing the built-in webcam (with the X1E camera patches from
> Bryan O'Donoghue) does not enable the status LED. It would be
> nice if you can check how that is wired, so that it can be enabled
> when a camera stream is started.
> 
> Greetings,
> 
> -- Sebastian


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver
  2025-09-01 16:10     ` Sebastian Reichel
  2025-09-01 16:23       ` Neil Armstrong
@ 2025-09-02 10:27       ` Konrad Dybcio
  2025-09-02 22:58         ` Sebastian Reichel
  1 sibling, 1 reply; 18+ messages in thread
From: Konrad Dybcio @ 2025-09-02 10:27 UTC (permalink / raw)
  To: Sebastian Reichel, Mark Pearson
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Hans de Goede,
	Ilpo Järvinen, Bryan O'Donoghue, Bjorn Andersson,
	Konrad Dybcio, Derek J . Clark, Henrique de Moraes Holschuh,
	devicetree, linux-kernel, platform-driver-x86@vger.kernel.org,
	linux-arm-msm

On 9/1/25 6:10 PM, Sebastian Reichel wrote:
> Hello Mark,
> 
> On Mon, Sep 01, 2025 at 09:48:39AM -0400, Mark Pearson wrote:
>> On Sun, Aug 31, 2025, at 5:28 PM, Sebastian Reichel wrote:
>>> Introduce EC driver for the ThinkPad T14s Gen6 Snapdragon, which
>>> is in theory compatible with ThinkPad ACPI. On Linux the system
>>> is booted with device tree, which is not supported by the ThinkPad
>>> ACPI driver. Also most of the hardware compatibility is handled
>>> via ACPI tables, which are obviously not used when booting via
>>> device tree. Thus adding DT compatibility to the existing driver
>>> is not worth it (almost no code sharing).
>>>
>>> The driver currently exposes features, which are not available
>>> via other means:
>>>
>>>  * Extra Keys
>>>  * System LEDs
>>>  * Keyboard Backlight Control
>>>
>>> The driver has been developed by reading the ACPI DSDT. There
>>> are some more features around thermal control, which are not
>>> yet supported by the driver.
>>>
>>
>> Thanks for working on this - it's great.
> 
> It's a personal scratch your own itch project, as I daily drive the
> machine.
> 
>> I'll see if I can get the EC spec so I can do some checking on the
>> values (I thought I had it already, but I can't find it). If this
>> file can be used for other platforms then it might be good to
>> rename the file to not be specific to the t14s? I'm curious if it
>> can be used on the X13s or the Yoga platform.
> 
> Maybe. I only have the T14s (apart of my older Intel/AMD ThinkPads,
> which use the ACPI driver). The ACPI DSDT functions completley
> abstract the lowlevel I2C interface, so in theory every ThinkPad
> could have a completley different EC and still use the same ACPI
> driver. So this needs to be checked per-device. Hopefully the low
> level interface is similar in those, so that we don't need to spam
> the kernel tree with multiple different EC drivers :)
> 
>> Couple of notes
>>  - I do agree it doesn't make sense to add this to thinkpad_acpi.
>>    That file is too big anyway.
>>  - If there are other pieces like this where some detail of the
>>    platform is needed, please do let me know. I never got enough
>>    time to work on this platform directly, and it wasn't in our
>>    Linux program, but I do have access and support from the
>>    platform team for getting details on it. If I can help, so not
>>    too much reverse engineering is needed, I'm happy to.
> 
> Thanks for the offer.
> 
> I would be interested in bits around system suspend. Right now
> support on X1E is limited to sending the CPU into suspend. Much of
> the machine seems to be still powered. Right now the keyboard
> backlight and all the status LEDs stay on and the LID + power led
> does not go into the typical breathing pattern. Additionally I had
> to disable wakeup capabilities for the EC interrupt, as closing the
> LID generates an event and thus an interrupt, which wakes the
> system. Obviousy that is undesired from user's perspective. My guess
> is, that there might be some register to mask events, but I haven't
> found it so far. Alternatively the EC might mask them automatically
> when the system is send into suspend, which I also have not yet
> figured out :) The only bit I know is, that EC register 0xE0 is
> involved in modern standby.
> 
> Apart from that and (probably) unrelated to the EC: I noticed that
> accessing the built-in webcam (with the X1E camera patches from
> Bryan O'Donoghue) does not enable the status LED. It would be
> nice if you can check how that is wired, so that it can be enabled
> when a camera stream is started.

FWIW a couple years ago I tried to do something similar for the X13s
EC, and the software interface looks somewhat familiar..

This never ended up becoming anything big, but just in case this is
useful for anyone:

https://github.com/SoMainline/linux/commit/c0cc2c60177a33597c33586bfe27b5f440e36745

Konrad

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] arm64: dts: qcom: x1e80100-t14s: add EC
  2025-08-31 21:28 ` [PATCH 3/3] arm64: dts: qcom: x1e80100-t14s: add EC Sebastian Reichel
@ 2025-09-02 10:28   ` Konrad Dybcio
  0 siblings, 0 replies; 18+ messages in thread
From: Konrad Dybcio @ 2025-09-02 10:28 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
	Bjorn Andersson, Konrad Dybcio, Mark Pearson
  Cc: Derek J. Clark, Henrique de Moraes Holschuh, devicetree,
	linux-kernel, platform-driver-x86, linux-arm-msm

On 8/31/25 11:28 PM, Sebastian Reichel wrote:
> Describe ThinkPad Embedded Controller in the T14s device tree,
> which adds LED and special key support.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>  .../dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi    | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
> index ac1dddf27da30e6a9f7e1d1ecbd5192bf2d0671e..7a9ec0c33b3ca847c5496e3ec145c70ccb7a3fe3 100644
> --- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi
> @@ -887,6 +887,23 @@ eusb6_repeater: redriver@4f {
>  	};
>  };
>  
> +&i2c6 {
> +	clock-frequency = <400000>;
> +	status = "okay";

a newline before 'status' is common practice> +
> +	ec@28 {

embedded-controller@

> +		compatible = "lenovo,thinkpad-t14s-ec";
> +		reg = <0x28>;
> +
> +		interrupts-extended = <&tlmm 66 IRQ_TYPE_LEVEL_LOW>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&ec_int_n_default>;

property-n
property-names

in this order, please

Konrad


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/3] platform: arm64: thinkpad-t14s-ec: new driver
  2025-08-31 21:28 [PATCH 0/3] platform: arm64: thinkpad-t14s-ec: new driver Sebastian Reichel
                   ` (3 preceding siblings ...)
  2025-09-01  9:03 ` [PATCH 0/3] platform: arm64: thinkpad-t14s-ec: new driver Neil Armstrong
@ 2025-09-02 13:17 ` Rob Herring (Arm)
  4 siblings, 0 replies; 18+ messages in thread
From: Rob Herring (Arm) @ 2025-09-02 13:17 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Konrad Dybcio, platform-driver-x86, linux-kernel,
	Bryan O'Donoghue, Ilpo Järvinen, Hans de Goede,
	Bjorn Andersson, Mark Pearson, Henrique de Moraes Holschuh,
	Conor Dooley, Krzysztof Kozlowski, Derek J. Clark, linux-arm-msm,
	devicetree


On Sun, 31 Aug 2025 23:28:30 +0200, Sebastian Reichel wrote:
> Introduce driver for the ThinkPad T14s Gen6 Snapdragon EC. In theory
> it seems to be compatible with the ThinkPad ACPI driver, but these
> devices are booted with device tree. As the name implies, the existing
> ThinkPad ACPI driver only supports the ACPI interface. Looking at
> the implementation, the ACPI DSDT contains many mapping functions
> to translate the low level I2C messages into the interface used by
> the ThinkPad ACPI driver. Adding DT support to the ThinkPad ACPI driver
> would require adding all those translation functions, which would add
> more or less the same amount of code as writing a separate driver using
> the low level interface directly. I don't think it's sensible to make
> the existing ACPI driver even more complicated, so I went for a separate
> driver.
> 
> I managed to get system LEDs, audio LEDs, extra keys and the keyboard
> backlight control working. The EC also seems to be used for some thermal
> bits, which I haven't looked into deeply. As far as I understand most
> thermal and fan control is handled by a different controller
> (0x36@i2c5) anyways.
> 
> Apart from that the EC is involved in proper system suspend, which
> is something I do not yet understand (I don't have any documentation
> apart from the dis-assembled DSDT and existing ACPI driver). Right
> now I disabled wake capabilities for the IRQ, since it would wake
> up the system when closing the LID. Hopefully a way to mask specific
> events will be found in the future.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
> Sebastian Reichel (3):
>       dt-bindings: platform: Add Lenovo Thinkpad T14s EC
>       platform: arm64: thinkpad-t14s-ec: new driver
>       arm64: dts: qcom: x1e80100-t14s: add EC
> 
>  .../bindings/platform/lenovo,thinkpad-t14s-ec.yaml |  49 ++
>  MAINTAINERS                                        |   6 +
>  .../dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtsi    |  23 +
>  drivers/platform/arm64/Kconfig                     |  20 +
>  drivers/platform/arm64/Makefile                    |   1 +
>  drivers/platform/arm64/lenovo-thinkpad-t14s.c      | 597 +++++++++++++++++++++
>  6 files changed, 696 insertions(+)
> ---
> base-commit: c8bc81a52d5a2ac2e4b257ae123677cf94112755
> change-id: 20250831-thinkpad-t14s-ec-ddeb23dbdafb
> 
> Best regards,
> --
> Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


This patch series was applied (using b4) to base:
 Base: using specified base-commit c8bc81a52d5a2ac2e4b257ae123677cf94112755

If this is not the correct base, please add 'base-commit' tag
(or use b4 which does this automatically)

New warnings running 'make CHECK_DTBS=y for arch/arm64/boot/dts/qcom/' for 20250831-thinkpad-t14s-ec-v1-0-6e06a07afe0f@collabora.com:

arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s-oled.dtb: ec@28 (lenovo,thinkpad-t14s-ec): 'wakeup-source' does not match any of the regexes: '^pinctrl-[0-9]+$'
	from schema $id: http://devicetree.org/schemas/platform/lenovo,thinkpad-t14s-ec.yaml#
arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dtb: ec@28 (lenovo,thinkpad-t14s-ec): 'wakeup-source' does not match any of the regexes: '^pinctrl-[0-9]+$'
	from schema $id: http://devicetree.org/schemas/platform/lenovo,thinkpad-t14s-ec.yaml#






^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver
  2025-09-02 10:27       ` Konrad Dybcio
@ 2025-09-02 22:58         ` Sebastian Reichel
  0 siblings, 0 replies; 18+ messages in thread
From: Sebastian Reichel @ 2025-09-02 22:58 UTC (permalink / raw)
  To: Konrad Dybcio
  Cc: Mark Pearson, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Hans de Goede, Ilpo Järvinen, Bryan O'Donoghue,
	Bjorn Andersson, Derek J . Clark, Henrique de Moraes Holschuh,
	devicetree, linux-kernel, platform-driver-x86@vger.kernel.org,
	linux-arm-msm

[-- Attachment #1: Type: text/plain, Size: 4718 bytes --]

Hi,

On Tue, Sep 02, 2025 at 12:27:24PM +0200, Konrad Dybcio wrote:
> On 9/1/25 6:10 PM, Sebastian Reichel wrote:
> > Hello Mark,
> > 
> > On Mon, Sep 01, 2025 at 09:48:39AM -0400, Mark Pearson wrote:
> >> On Sun, Aug 31, 2025, at 5:28 PM, Sebastian Reichel wrote:
> >>> Introduce EC driver for the ThinkPad T14s Gen6 Snapdragon, which
> >>> is in theory compatible with ThinkPad ACPI. On Linux the system
> >>> is booted with device tree, which is not supported by the ThinkPad
> >>> ACPI driver. Also most of the hardware compatibility is handled
> >>> via ACPI tables, which are obviously not used when booting via
> >>> device tree. Thus adding DT compatibility to the existing driver
> >>> is not worth it (almost no code sharing).
> >>>
> >>> The driver currently exposes features, which are not available
> >>> via other means:
> >>>
> >>>  * Extra Keys
> >>>  * System LEDs
> >>>  * Keyboard Backlight Control
> >>>
> >>> The driver has been developed by reading the ACPI DSDT. There
> >>> are some more features around thermal control, which are not
> >>> yet supported by the driver.
> >>>
> >>
> >> Thanks for working on this - it's great.
> > 
> > It's a personal scratch your own itch project, as I daily drive the
> > machine.
> > 
> >> I'll see if I can get the EC spec so I can do some checking on the
> >> values (I thought I had it already, but I can't find it). If this
> >> file can be used for other platforms then it might be good to
> >> rename the file to not be specific to the t14s? I'm curious if it
> >> can be used on the X13s or the Yoga platform.
> > 
> > Maybe. I only have the T14s (apart of my older Intel/AMD ThinkPads,
> > which use the ACPI driver). The ACPI DSDT functions completley
> > abstract the lowlevel I2C interface, so in theory every ThinkPad
> > could have a completley different EC and still use the same ACPI
> > driver. So this needs to be checked per-device. Hopefully the low
> > level interface is similar in those, so that we don't need to spam
> > the kernel tree with multiple different EC drivers :)
> > 
> >> Couple of notes
> >>  - I do agree it doesn't make sense to add this to thinkpad_acpi.
> >>    That file is too big anyway.
> >>  - If there are other pieces like this where some detail of the
> >>    platform is needed, please do let me know. I never got enough
> >>    time to work on this platform directly, and it wasn't in our
> >>    Linux program, but I do have access and support from the
> >>    platform team for getting details on it. If I can help, so not
> >>    too much reverse engineering is needed, I'm happy to.
> > 
> > Thanks for the offer.
> > 
> > I would be interested in bits around system suspend. Right now
> > support on X1E is limited to sending the CPU into suspend. Much of
> > the machine seems to be still powered. Right now the keyboard
> > backlight and all the status LEDs stay on and the LID + power led
> > does not go into the typical breathing pattern. Additionally I had
> > to disable wakeup capabilities for the EC interrupt, as closing the
> > LID generates an event and thus an interrupt, which wakes the
> > system. Obviousy that is undesired from user's perspective. My guess
> > is, that there might be some register to mask events, but I haven't
> > found it so far. Alternatively the EC might mask them automatically
> > when the system is send into suspend, which I also have not yet
> > figured out :) The only bit I know is, that EC register 0xE0 is
> > involved in modern standby.
> > 
> > Apart from that and (probably) unrelated to the EC: I noticed that
> > accessing the built-in webcam (with the X1E camera patches from
> > Bryan O'Donoghue) does not enable the status LED. It would be
> > nice if you can check how that is wired, so that it can be enabled
> > when a camera stream is started.
> 
> FWIW a couple years ago I tried to do something similar for the X13s
> EC, and the software interface looks somewhat familiar..
> 
> This never ended up becoming anything big, but just in case this is
> useful for anyone:
> 
> https://github.com/SoMainline/linux/commit/c0cc2c60177a33597c33586bfe27b5f440e36745

I had a quick look and I would say that looks quite different:

 * The EC read/write/event function uses the same command bytes, but does
   not have 0x00 0x01 between the register and the value, so
   different functions would be needed for T14s and X13s
 * Almost all event codes are different
 * The register numbers are also different

So my fears, that the ACPI functions allow more or less completely
changing the low level interface were true :(

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2025-09-02 22:58 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-31 21:28 [PATCH 0/3] platform: arm64: thinkpad-t14s-ec: new driver Sebastian Reichel
2025-08-31 21:28 ` [PATCH 1/3] dt-bindings: platform: Add Lenovo Thinkpad T14s EC Sebastian Reichel
2025-08-31 23:53   ` Bryan O'Donoghue
2025-09-01  9:28   ` Krzysztof Kozlowski
2025-08-31 21:28 ` [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver Sebastian Reichel
2025-09-01  8:43   ` Bryan O'Donoghue
2025-09-01 15:20     ` Sebastian Reichel
2025-09-01 15:52       ` Bryan O'Donoghue
2025-09-01  9:51   ` Ilpo Järvinen
2025-09-01 13:48   ` Mark Pearson
2025-09-01 16:10     ` Sebastian Reichel
2025-09-01 16:23       ` Neil Armstrong
2025-09-02 10:27       ` Konrad Dybcio
2025-09-02 22:58         ` Sebastian Reichel
2025-08-31 21:28 ` [PATCH 3/3] arm64: dts: qcom: x1e80100-t14s: add EC Sebastian Reichel
2025-09-02 10:28   ` Konrad Dybcio
2025-09-01  9:03 ` [PATCH 0/3] platform: arm64: thinkpad-t14s-ec: new driver Neil Armstrong
2025-09-02 13:17 ` Rob Herring (Arm)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).