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

["Mark Pearson" <mpearson-lenovo@squebb.ca>]

Hi Sebastian,

On Mon, Sep 1, 2025, at 12: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 :)
>
Looks like you're right to make this platform specific. At least for now it looks like the definitions are tied to the platform.
Strange as we have a common spec on the x86 Thinkpads, but at least for now this is t14s Qualcomm specific.

>> 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 did get back some details - not quite as much as I wanted, but enough to confirm that all your definitions look correct.
Main point of interest is they didn't send me the BL2 details - so not sure if that is needed?

> 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 still have some more digging to do here I'm afraid.

> 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.
>
Ack. Don't know the details on that yet.

Mark