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

[Abel Vesa]

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.

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

Thanks,
Abel