Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40030 )
Change subject: ec/google/chromeec: New host command EC_CMD_GET_KEYBD_TOP_ROW_LAYOUT ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... File src/ec/google/chromeec/ec_commands.h:
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3022: Set this to 0 to disable all Vivaldi keyboard code
This is just an escape hatch in case some one wants to bypass the vivaldi code. […]
Done
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3028: action keys
Isn't it both action and function keys? […]
As discussed, we should only be adding action key information to ec_commands.h. This entry is supposed to provide information *only* about action keys. Adding IDs for function keys is not required.
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3043: struct top_row_layout top_row;
Can't we just put the members in the above structure directly here? […]
Like discussed, we cannot share the structure in ec_commands.h and ssdt generator. This is because ssdt generator will be added to either arch/x86 or vendorcode/google/chromeos (since this still has some assumptions about Chrome OS). That file cannot directly include a chromeec header. Instead chromeec header will contain the command/response that are handled by EC driver in coreboot.
Thus, you can directly include the members in the above structure here.