Re: [PATCH 2/3] platform: arm64: lenovo-thinkpad-t14s: Add Gen 7 EC support

[Hans de Goede <hansg@kernel.org>]

Hi,

On 9-Nov-26 22:59, Abel Vesa wrote:
> On 26-11-09 15:52:16, Hans de Goede wrote:
>> Hi Abel,
>>
>> On 6-Nov-26 23:01, Abel Vesa wrote:
>>> The embedded controller of the ThinkPad T14s Gen 7 uses the same
>>> register layout and event codes as the Gen 6 one, but a different set
>>> of I2C commands. Going by the ACPI EC accessors in the Gen 7 firmware:
>>>
>>>   - register read:  write {0x80, reg}, then write {0x80} + read 1 byte
>>>   - register write: write {0x81, reg, val}
>>>   - event read:     write {0x84, 0x00}, then write {0x84} + read 1 byte
>>>> With the Gen 6 commands the EC never reports any event and the LED
>>> and keyboard backlight accesses have no effect.
>>
>> Thank you for working in this. I've been testing this and my own T14s gen7
>> and also I've taken a closer look at the DSDT leading to a bunch of
>> improvements. Instead of making these as suggested I've just gone ahead and
>> implemented these as fixup-s to be squashed into a v2 patch.
>>
>> For easier review by you I've split the improvements into 4 patches (attached):
>>
>> 1: Fix Fn-F12 not emitting a key-press + filter out some EC codes being reported
>> when pressing Fn + Fxx key combos which already send an I2C-HID kbd event.
>>
>> 2: Add Gen 7 suspend/resume handling
>>
>> 3: Cleanup: has_kbd_bl2_and_standby and mic_mute_led_generic checks are
>> really just checks for the original (gen6) support vs gen7 support
>> drop the bools and directly check which variant ec->variant points
>> at instead.
>> 4: Cleanup: has_kbd_bl2_and_standby and mic_mute_led_generic checks are
>> really just checks for the original (gen6) support vs gen7 support
>> drop the bools and directly check which variant ec->variant points
>> at instead.
>>
>>> Describe the bus accessors and event read per variant and add one for
>>> the Gen 7. The Gen 7 firmware does not use the second keyboard backlight
>>> register or the modern standby register, it drives the mic mute LED as a
>>> generic LED (id 0x0e) instead of a bit in the audio register and, when
>>> the keyboard backlight is cycled with Fn+Space, it also reports 3 for
>>> the high level, so handle those differences too. Also handle the Gen 7
>>> only events as known ones.
>>
>> So about these differences:
>>
>> 1: For the spk / mic mute LEDs, on a hunch I tried using std LED controls
>> with a LED idx/id of 15 for the spk mute LED and that also works.
>>
>> I did not find either way (REG_AUD vs REG_LED) for the spk mute LED in
>> the DSDT, instead the DSDT ends up sending an ACPI notify event, relying
>> on some windows driver to handle this.
>>
>> How did you find that REG_AUD still works, did you just try the gen 6
>> method ?
> 
> Yep, just tried the Gen 6 method and it worked. Nothing in the DSDT
> for it either.
> 
> Re-checked just now: the speaker mute led lights up both via REG_AUD bit 6
> and as generic led 15 via REG_LED. Since the mic mute led is already a generic
> led (0x0e) on Gen 7, I'm fine with moving the speaker mute one to 15 too,
> if you prefer. Otherwise I'll keep REG_AUD, since that path is shared with Gen 6.

I don't have a preference either way.

I tried both methods (REG_LED and REG_AUD) for the spkr mute LED and both
work. Which ever one is written last wins and gets to determine the actual
LED state.

This means that the spk-mute bit in REG_AUD may not actually reflect the LED
state, since it stays set when disabling the LED through REG_LED.

>> 2: For the kbd backlight, again the DSDT ends up sending an ACPI notify event
>> and does not access the EC itself at all.
>>
>> How did you find that T14S_EC_REG_KBD_BL1 also works on gen 7?
> 
> Same, tried the Gen 6 register and it worked. Writes change the level
> and reads follow the Fn+Space changes, which is how I noticed the extra
> level.
> 
>>
>> 3: For suspend/resume the DSDT does have modern-standby handling both
>> making the LEDS glow and setting some magic bit (which presumably makes
>> the EC use less power or maybe makes it send less wakeup events if we
>> were to make the EC IRQ a wakeup source) ?  This is implemented in
>> my second fixup patch.
> 
> Squashed it in, thanks. Haven't been able to verify this path on my
> unit though, suspend has some unrelated other issues. Will try tomorrow.
> 
>>
>> Regards,
>>
>> Hans
> 
> All four fixups are squashed into patch 2 for v2, with your
> Co-developed-by and Signed-off-by.

Thank you.

Regards,

Hans