Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46724 )
Change subject: libpayload/keyboard: Revise scancode set and translation config ......................................................................
Patch Set 2:
(2 comments)
I'm not clear on the motivation for the change, e.g., to get rid of PC_KEYBOARD_AT_TRANSLATED or fix some other issue. I've worked on enough other OSes that "because Linux does it" isn't usually a very compelling argument, though this does seem like one of the few cases I've seen where Linux goes by what the firmware set (usually Linux goes off and does its own thing regardless). I'm not arguing against the change, but given the potential for breakage the phrase "if it ain't broke don't fix it" comes to mind. I'm sure something's being fixed, but it's not obvious to me and I'd generally expect the translation mode to be based on what code set(s) the keyboard can support, rather than setting the keyboard mode based on the current translation mode.
https://review.coreboot.org/c/coreboot/+/46724/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/46724/2/payloads/libpayload/drivers... PS2, Line 334: set < 1 || set > 3 Might be better as "set != I8042_SCANCODE_SET_1 && set != I8042_SCANCODE_SET_2", depending on whether these #defines still exist after this change.
https://review.coreboot.org/c/coreboot/+/46724/2/payloads/libpayload/drivers... PS2, Line 374: 2 : 1 This should probably use I8042_SCANCODE_SET_2 : I8042_SCANCODE_SET_1 , or delete the seemingly now unused #defines from i8042.h.