Rajat Jain has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39888 )
Change subject: ec/google/chromeec: Emit SSDT for PS2 keyboard if needed ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... File src/ec/google/chromeec/ec_ps2_keyboard.c:
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 21: static
static const?
Done
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 224: : #define KEYMAP(scancode, keycode) (((uint32_t)(scancode) << 16) | (keycode & 0xFFFF))
nit: Preferably all #defines go at the top of the file, unless they are in a #define/#undef combo.
Done
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 261:
there is an extra blank line here
Done
https://review.coreboot.org/c/coreboot/+/39888/1/src/ec/google/chromeec/ec_p... PS1, Line 283: _SB.PCI0.PS2K
Is this the required path? Shouldn't it be under the parent (EC) ? If it doesn't belong under the EC, then why it in the EC driver? I don't actually see any google_chromeec_() calls in here.
Long story. I had explained and discussed this with Furquan earlier. Essentially, the PS2 device is provided by the Serio which is part of EC (Look for SIO_EC_ENABLE_PS2K) and was only recently moved out of the EC scope (see commit "ec/google/chromeec/acpi: move PS2K under PCI0") for some reason. But the device ACPI files are still provided by chromeec (src/ec/google/chromeec/acpi/superio.asl).