linux-kernel - Re: [PATCH v3 2/3] platform: arm64: thinkpad-t14s-ec: new driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f16a858d-fb41-4cc9-a138-7bfe5ee2800f@oss.qualcomm.com>
Date: Mon, 8 Sep 2025 09:28:12 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Sebastian Reichel <sre@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
 <conor+dt@...nel.org>, Hans de Goede <hansg@...nel.org>,
        Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        Mark Pearson <mpearson-lenovo@...ebb.ca>
Cc: "Derek J. Clark" <derekjohn.clark@...il.com>,
        Henrique de Moraes Holschuh <hmh@....eng.br>,
        Neil Armstrong <neil.armstrong@...aro.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v3 2/3] platform: arm64: thinkpad-t14s-ec: new driver

On 9/6/25 3:12 AM, 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
> (drivers/platform/x86/lenovo/thinkpad_acpi.c). 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 as there is almost no code sharing.

[...]


couple nits, feel free to ignore

> +#define T14S_EC_EVT_NONE			0x00
> +#define T14S_EC_EVT_KEY_FN_4			0x13
> +#define T14S_EC_EVT_KEY_FN_F7			0x16
> +#define T14S_EC_EVT_KEY_FN_SPACE		0x1F
> +#define T14S_EC_EVT_KEY_TP_DOUBLE_TAP		0x20
> +#define T14S_EC_EVT_AC_CONNECTED		0x26
> +#define T14S_EC_EVT_AC_DISCONNECTED		0x27
> +#define T14S_EC_EVT_KEY_POWER			0x28
> +#define T14S_EC_EVT_LID_OPEN			0x2A
> +#define T14S_EC_EVT_LID_CLOSED			0x2B
> +#define T14S_EC_EVT_KEY_FN_F12			0x62
> +#define T14S_EC_EVT_KEY_FN_TAB			0x63
> +#define T14S_EC_EVT_KEY_FN_F8			0x64
> +#define T14S_EC_EVT_KEY_FN_F10			0x65
> +#define T14S_EC_EVT_KEY_FN_F4			0x6A
> +#define T14S_EC_EVT_KEY_FN_D			0x6B
> +#define T14S_EC_EVT_KEY_FN_T			0x6C
> +#define T14S_EC_EVT_KEY_FN_H			0x6D
> +#define T14S_EC_EVT_KEY_FN_M			0x6E
> +#define T14S_EC_EVT_KEY_FN_L			0x6F
> +#define T14S_EC_EVT_KEY_FN_RIGHT_SHIFT		0x71
> +#define T14S_EC_EVT_KEY_FN_ESC			0x74
> +#define T14S_EC_EVT_KEY_FN_N			0x79
> +#define T14S_EC_EVT_KEY_FN_F11			0x7A
> +#define T14S_EC_EVT_KEY_FN_G			0x7E

Please use lowercase hex consistently across the file

[...]

> +enum thinkpad_t14s_ec_led_status_t {
> +	T14S_EC_LED_OFF =	0x00,
> +	T14S_EC_LED_ON =	0x80,
> +	T14S_EC_LED_BLINK =	0xc0,

These conveniently translate to: BIT(7) and BIT(6)|BIT(7), meaning
BIT(7) could mean "ON" and BIT(6) could mean "pulse" (can you pulse
a disabled LED? arcane secrets..)

[...]

> +	if (brightness == LED_OFF)
> +		new_state = T14S_EC_LED_OFF;
> +	else if (led->cache != T14S_EC_LED_BLINK)

==s are easier to logically follow than !=, but this is totally
potayto/potahto

[...]

> +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);
> +
> +	if (*delay_on == 0 && *delay_off == 0) {
> +		/* We can choose the blink rate */

"can't"?

Needless to say, amazing work on piecing all this together, Sebastian!

Konrad