linux-kernel - Re: [PATCH 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 for Android: free password hash cracker in your pocket

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

Message-ID: <wyv3ounark6jccvhj4vp5qxgmn4bleq6hsqinr4s6r32kld4xp@lhbmetuhydns>
Date: Mon, 1 Sep 2025 17:20:45 +0200
From: Sebastian Reichel <sre@...nel.org>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
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>, 
	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>, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	platform-driver-x86@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver

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@...aro.org>

Thanks for the review.

Greetings,

-- Sebastian