Change in coreboot[master]: ec/google/chromeec: New host command EC_CMD_GET_KEYBD_TOP_ROW_LAYOUT
Rajat Jain has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40030 )
Change subject: ec/google/chromeec: New host command EC_CMD_GET_KEYBD_TOP_ROW_LAYOUT ......................................................................
ec/google/chromeec: New host command EC_CMD_GET_KEYBD_TOP_ROW_LAYOUT
Add commmand to query the EC for the keyboard's top row layout. Also add supporting data structures for the exchange.
Signed-off-by: Rajat Jain rajatja@google.com Change-Id: I26aff6dd0e701e0cecb3b66bc54c5a23688f0109 --- M src/ec/google/chromeec/ec_commands.h 1 file changed, 72 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40030/1
diff --git a/src/ec/google/chromeec/ec_commands.h b/src/ec/google/chromeec/ec_commands.h index 7b5a067..7c0d6bc 100644 --- a/src/ec/google/chromeec/ec_commands.h +++ b/src/ec/google/chromeec/ec_commands.h @@ -2972,6 +2972,78 @@ } __ec_align1;
/*****************************************************************************/ +/* Get the Keyboard Top Row Layout */ +#define EC_CMD_GET_KEYBD_TOP_ROW_LAYOUT 0x002E + +/* Possible values for the top row keys */ +enum top_key { + TK_ABSENT = 0, + TK_F1, + TK_F2, + TK_F3, + TK_F4, + TK_F5, + TK_F6, + TK_F7, + TK_F8, + TK_F9, + TK_F10, + TK_F11, + TK_F12, + TK_F13, + TK_F14, + TK_F15, + TK_BACK, + TK_FORWARD, + TK_REFRESH, + TK_FULLSCREEN, + TK_OVERVIEW, + TK_BRIGHTNESS_DOWN, + TK_BRIGHTNESS_UP, + TK_VOL_MUTE, + TK_VOL_DOWN, + TK_VOL_UP, + TK_SNIP, + TK_PRIVACY_SCRN_TOGGLE, + TK_KBD_BKLIGHT_DOWN, + TK_KBD_BKLIGHT_UP, + TK_PLAY_PAUSE, + TK_NEXT_TRACK, + TK_PREV_TRACK, +}; + +/* Maximum number of top row keys, excluding Esc and Screenlock keys */ +#define MAX_TOP_ROW_KEYS 15 + +struct top_row_layout { + + /* + * Number of top row keys, excluding Esc and Screenlock. + * Set this to 0 to disable all Vivaldi keyboard code + * (i.e. not expose any tables to the kernel). + */ + uint8_t num_top_row_keys; + + /* + * The action keys in the top row, in order from left to right. + * The values are filled from enum top_key. Esc and Screenlock + * keys are not considered part of top row keys. + */ + uint8_t action_keys[MAX_TOP_ROW_KEYS]; + + /* + * Is the keyboard capable of sending function keys *in addition to * + * action keys. This is possible for e.g. if the keyboard has a + * dedicated Fn key. + */ + uint8_t can_send_function_keys; +}; + +struct ec_response_top_row_layout { + struct top_row_layout top_row; +}; + +/*****************************************************************************/ /* USB charging control commands */
/* Set USB port charging mode */
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:
(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: This file is typically updated by copying the one that gets checked into EC codebase. Example: https://review.coreboot.org/c/coreboot/+/38409
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 Just curious: Under what conditions would you want to do this?
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?
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3035: nit: No space?
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3039: can_send_function_keys What if the keyboard can only send function keys?
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?
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3044: ; All ec cmd/response structures use __ec_align
Tim Wawrzynczak 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:
(4 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
Just curious: Under what conditions would you want to do this?
What would be the behavior of the keys if the new table isn't exposed?
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.
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?
+1
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3044: ;
All ec cmd/response structures use __ec_align
You'll need __ec_align1 (align to 1-byte boundary)
Jett Rink 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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... File src/ec/google/chromeec/ec_commands.h:
PS1:
This file is typically updated by copying the one that gets checked into EC codebase. […]
Yes, this should be done in platform/ec repo first, then pulled over to coreboot
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3039: can_send_function_keys
What if the keyboard can only send function keys?
Let make it an enum instead
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.
Paul Menzel 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:
(1 comment)
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).
Jett Rink 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:
PS1:
There are quite a lot of differences in EC repository's [cros/master:include/ec_commands. […]
src/platform/ec master branch should be the place where changes to ec_commands.h to take place
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
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 […]
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.
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.
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.
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:
(2 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 3028: action 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.
Will do
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3043: struct top_row_layout top_row;
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.
Please see in the new review I have out there. That version does not include this header in acpigen_* file. I have still kept the top_row structure because that way atleast I do not have to duplicate another enum in the EC code.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40030
to look at the new patch set (#2).
Change subject: ec/google/chromeec: Add host command EC_CMD_GET_KEYBD_TOP_ROW_LAYOUT ......................................................................
ec/google/chromeec: Add host command EC_CMD_GET_KEYBD_TOP_ROW_LAYOUT
Add command to query the EC for the keyboard's top row layout. Also add supporting data structures for the exchange.
Signed-off-by: Rajat Jain rajatja@google.com Change-Id: I26aff6dd0e701e0cecb3b66bc54c5a23688f0109 --- M src/ec/google/chromeec/ec_commands.h 1 file changed, 69 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40030/2
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40030
to look at the new patch set (#3).
Change subject: ec/google/chromeec: Add host command EC_CMD_GET_KEYBD_CONFIG ......................................................................
ec/google/chromeec: Add host command EC_CMD_GET_KEYBD_CONFIG
Add command to query the EC for the keyboard layout. Also add supporting data structures for the exchange.
Signed-off-by: Rajat Jain rajatja@google.com Change-Id: I26aff6dd0e701e0cecb3b66bc54c5a23688f0109 --- M src/ec/google/chromeec/ec_commands.h 1 file changed, 77 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/40030/3
Rajat Jain has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40030 )
Change subject: ec/google/chromeec: Add host command EC_CMD_GET_KEYBD_CONFIG ......................................................................
Patch Set 3:
(7 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
Will do.
Done
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 […]
Done
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
I realized that actually 0x129A is the next available host command, so used that.
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3028: action keys
As discussed, we should only be adding action key information to ec_commands.h. […]
Done
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3035:
nit: No space? […]
Done
https://review.coreboot.org/c/coreboot/+/40030/1/src/ec/google/chromeec/ec_c... PS1, Line 3043: struct top_row_layout top_row;
Like discussed, we cannot share the structure in ec_commands.h and ssdt generator. […]
Done
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) […]
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40030 )
Change subject: ec/google/chromeec: Add host command EC_CMD_GET_KEYBD_CONFIG ......................................................................
Patch Set 3:
Sorry not touching this one here until it's merged in chromium 😊
Rajat Jain has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40030 )
Change subject: ec/google/chromeec: Add host command EC_CMD_GET_KEYBD_CONFIG ......................................................................
Patch Set 3:
Patch Set 3:
Sorry not touching this one here until it's merged in chromium 😊
Here's the link in case you want to review this on the chromium tree: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2133824
:-)
Rajat Jain has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40030 )
Change subject: ec/google/chromeec: Add host command EC_CMD_GET_KEYBD_CONFIG ......................................................................
Patch Set 3:
Patch Set 3:
Sorry not touching this one here until it's merged in chromium 😊
Now that it has landed in chromium, can we merge this patchset here?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40030 )
Change subject: ec/google/chromeec: Add host command EC_CMD_GET_KEYBD_CONFIG ......................................................................
Patch Set 3: Code-Review+2
-
Furquan Shaikh (Code Review) -
Jett Rink (Code Review)
-
Paul Menzel (Code Review)
-
Rajat Jain (Code Review)
-
Tim Wawrzynczak (Code Review)