Rajat Jain 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:
(7 comments)
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... File src/ec/google/chromeec/ec_commands.h:
PS1:
Yes, this should be done in platform/ec repo first, then pulled over to coreboot
There are quite a lot of differences in EC repository's [cros/master:include/ec_commands.h] and coreboot's [src/ec/google/chromeec/ec_commands.h] today. I don't want to bring in anything else with my patch and complicate things. I plan to submit this same patch to EC repo (or can do so that first if needed).
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
What would be the behavior of the keys if the new table isn't exposed?
This is just an escape hatch in case some one wants to bypass the vivaldi code. I couldn't think of any reason why some one would want to use it in chromeos, but still decided to document it in case some one wants to for some reason I couldn't think of.
If the table isn't exposed to the kernel, then everything (EC, coreboot, kernel) would continue to process keys just like it does today. The top row will continue to send function keys, and the chrome will keep on converting them to action keys.
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?
No, these are action keys. Essentially for chromeos keyboards, the "action keys" have always been (and will always continue to be) one of the roles of the top row keys. In fact, it is the primary role (but primary/secondary doesn't matter here). With some recent keyboards (Wilco etc), the keyboards can now *additionally* send function keys becaus eof presence of an Fn key, but even on those products, function key role is secondary. So what I am saying here is that I cannot think of a device that would want to have *pure* function keys (only). (Sidenote, even if there was such a device, we don't have to specify function keys in order, they will always be from F1-Fx where x = number of top keys).
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3032: action_keys
I would just say keys, b/c as Furquan noted they are action/function.
See above
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3039: can_send_function_keys
Let make it an enum instead
I can make it an enum, but I just don't think a keyboard that wants to just send F1-F10 keys makes any sense for chromeos. Do you? There has never been such a keyboard and why would we want such an (internal) keyboard in chromeos?
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?
No, I re-use this same structure for everywhere we need to pass top row layout. Eg. to ACPI code, and even on the EC side. I'll be sending out EC review today so you'll see there.
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3044: ;
You'll need __ec_align1 (align to 1-byte boundary)
Will do.