[coreboot-gerrit] [M] Change in coreboot[master]: mb/google/link: rework TP/TS ACPI for new Windows I2C driver

[Nico Huber (Code Review)]

Attention is currently required from: CoolStar, Felix Singer, Martin Roth, Matt DeVillier, Stefan Reinauer.

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/76159?usp=email )

Change subject: mb/google/link: rework TP/TS ACPI for new Windows I2C driver
......................................................................

Patch Set 8:

(6 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/76159/comment/fc478fdb_dc16e065 :
PS8, Line 30: The original Sleep Button devices are preserved for Linux due to the
Why can't we do what Samus does in this case?

File src/mainboard/google/link/acpi/mainboard.asl:

https://review.coreboot.org/c/coreboot/+/76159/comment/0238f036_eeee5ce1 :
PS1, Line 14: // Trackpad Wake is GPIO12
            : 		Name(_PRW, Package() { BOARD_TRACKPAD_WAKE_GPIO, 0x03 } )
            :
> Okay, well then I didn't say anything 😊

I was about to say: it's moved, not removed. But then realized we end up
with two device nodes for the same device. So why not move everything?

File src/mainboard/google/link/acpi/mainboard.asl:

https://review.coreboot.org/c/coreboot/+/76159/comment/f9f0fb70_004646e5 :
PS7, Line 50: LINK
> I2C bus driver code: https://github.com/coolstar/gmbusi2c […]

Thanks for the link! looks quite generic.

I just remembered, we have `BOOTxxxx` registered. I guess we should use
that? cf. `enum coreboot_acpi_ids` in `src/include/acpi/acpi.h`.

Only for my curiosity: I guess the binding is done in the `.inf` with
```
%GmBusI2C.DeviceDescArb%=GmBusI2C_Device, ACPI\LINK0000
```
correct? Do you know where this is documented?

https://review.coreboot.org/c/coreboot/+/76159/comment/f153cbe5_3ae00a91 :
PS7, Line 51: 		Method (_STA, 0, NotSerialized)  // _STA: Status
            : 		{
            : 			Return (0x0F)
            : 		}
> _STA added here for clarity, since the peripheral devices have it too (for consistency with google/s […]

Well, for Samus it's actual methods with code. Here it's just dead code
constantly returning the default value and (at least for myself) distracting
the reader.

https://review.coreboot.org/c/coreboot/+/76159/comment/0208653a_07e710ea :
PS7, Line 56: MMIO
> creating a _DSM with a UUID / functions seems overkill, since MMIO here is a replacement for what wo […]

It would be the canonical way, though. I would at least add a comment,
that it's a custom OS interface (for the casual reader it would look
like dead code otherwise).

https://review.coreboot.org/c/coreboot/+/76159/comment/c96c1461_37426011 :
PS7, Line 82: ISTP
> Done

A downstream Linux driver maybe? I couldn't find any reference upstream.