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:
(8 comments)
https://review.coreboot.org/c/coreboot/+/40030/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40030/1//COMMIT_MSG@7 PS1, Line 7: ec/google/chromeec: New host command EC_CMD_GET_KEYBD_TOP_ROW_LAYOUT
Please make it a statement by adding a verb (in imperative mood).
Will do.
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... File src/ec/google/chromeec/ec_commands.h:
PS1:
src/platform/ec master branch should be the place where changes to ec_commands.h to take place
Will do.
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 2976: 0x002E
I think we are actually on on 0x0130 for the next available host command
Will do
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. 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.
I believe this is clear now, resolving the comment.
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).
As discussed, I believe this is clear now. Resolving the conflict.
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
Discussed this in detail with Furquan. These are actually just action keys because chromeos keyboard would always send action keys. The function keys are optional. Resolving the comment. Pleasse re-comment if needed.
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3035:
nit: No space?
will do
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3039: can_send_function_keys
It could be like a function lock, the default press is function and then the FN + key is the normal chromos key (like back). We should account for that use case too.
Right, that use case is accounted for. This code generates a keymap that should cover all scancodes that a keyboard can generate. It does not matter whether the action or function key is primary role of a key. In this case, we need to ensure that we generate keymaps for both function and action keys, which we are generating with this code today.
I'm resolving this comment, please feel free to reopen if you think it is needed.