[coreboot-gerrit] [M] Change in coreboot[main]: ec/starlabs/merlin: Add Intel Virtual Button driver

[Paul Menzel (Code Review)]

Attention is currently required from: Sean Rhodes.

Paul Menzel has posted comments on this change by Sean Rhodes. ( https://review.coreboot.org/c/coreboot/+/83879?usp=email )

Change subject: ec/starlabs/merlin: Add Intel Virtual Button driver
......................................................................

Patch Set 3:

(8 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/83879/comment/984f8baa_d02a7cfd?usp... :
PS3, Line 15: Ubuntu
Which version?

https://review.coreboot.org/c/coreboot/+/83879/comment/7c552914_0bdbf459?usp... :
PS3, Line 16: show
show*n*

https://review.coreboot.org/c/coreboot/+/83879/comment/bfa0c2e1_3339c64a?usp... :
PS3, Line 15: Tested on `starlite_adl` with Ubuntu, by checking the
            : virtual keyboard is show when the tablet is undocked
            : and hidden when docked.
Can it be tested by looking at some logs?

https://review.coreboot.org/c/coreboot/+/83879/comment/ab2f2b4b_fc6c5aff?usp... :
PS3, Line 9: Add Intel Virtual Button driver which is used to report
           : to the OS whether a tablet is docked or undocked.
           : 
           : This is currently only used on `mb/starlite_adl` so the
           : GPIO is hardcoded to GPP_F15 for now.
           : 
           : Tested on `starlite_adl` with Ubuntu, by checking the
           : virtual keyboard is show when the tablet is undocked
           : and hidden when docked.
Please try to use 72 characters per line.

File src/ec/starlabs/merlin/acpi/battery.asl:

PS3: 
This is from a different commit, isn’t it?

File src/ec/starlabs/merlin/acpi/dock.asl:

PS3: 
There is already:

1.  `src/mainboard/purism/librem_jsl/acpi/vbtn.asl`
2.  `src/ec/google/wilco/acpi/vbtn.asl`

Should it be at least named like this? Any possibilities to generalize the driver?

File src/ec/starlabs/merlin/acpi/ec.asl:

https://review.coreboot.org/c/coreboot/+/83879/comment/2ea24e94_aef2f756?usp... :
PS3, Line 53: #if CONFIG(SOC_INTEL_COMMON)
            : 			\LIDS = 0x03
            : #endif
Please comment in the commit message why this is done, or add a comment?

File src/ec/starlabs/merlin/acpi/hid.asl:

https://review.coreboot.org/c/coreboot/+/83879/comment/d14cea88_9f8bc1b8?usp... :
PS3, Line 352: Case (0x08)
             : 					{
             : 						Return (_SB.PCI0.LPCB.EC.VBTN.VGBS())
             : 					}
Please mention this in the commit message?