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] [day] [month] [year] [list]

Message-ID: <kshw3zf3rbpc4ytb6ofucannuv2uax6lz74q53qs73xcbxpl5z@qr6mgnj6olkm>
Date: Mon, 8 Sep 2025 23:59:51 +0200
From: Sebastian Reichel <sre@...nel.org>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: 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>, 
	"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

Hi,

On Mon, Sep 08, 2025 at 09:28:12AM +0200, Konrad Dybcio wrote:
> 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

I will do a v4 adding events for thermal zone status update (just
debug prints for now), as I noticed the unknown event id messages
in my dmesg after doing a full kernel compile.

> > +#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
> 
> [...]

Changed in v4.

> > +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..)

FIELD_PREP would also be an option, but I don't think this becomes
more readable by using either and kept this as-is.

> [...]
> 
> > +	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
> 
> [...]

Changed in v4.

> > +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"?

I will change this to:

"Userspace does not provide a blink rate; we can choose it"

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

Thanks. Btw. I tried the suspend logic from your X13s EC driver as
the ACPI DSDT ECNT functions are exactly the same for X13s and T14s
and you obviously got the logic from there. But I haven't noticed
any difference and the PM handler is commented out in your driver.
Thinkpads typically have a breathing animation on their power and
LID leds when the machine is suspended. My expectation is, that the
EC enables at least this animation when being programmed correctly.

Greetings,

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)