Message-ID: <5qtornbvoc7soqud5o3e3uwvtifuy5vocmjsdwggydl6jrvmmm@5nc5ob4cj6ec>
Date: Wed, 3 Sep 2025 00:58:29 +0200
From: Sebastian Reichel <sre@...nel.org>
To: Konrad Dybcio <konradybcio@...nel.org>
Cc: Mark Pearson <mpearson-lenovo@...ebb.ca>, 
	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>, "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" <platform-driver-x86@...r.kernel.org>, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH 2/3] platform: arm64: thinkpad-t14s-ec: new driver

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

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

Powered by blists - more mailing lists