Hello Thejaswani Putta,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to review the following change.
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys.
These changes are enabled with a config flag in libpayload and by default it is disabled, whichever board needs it will enable it in its Kconfig.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/Kconfig M payloads/libpayload/drivers/i8042/keyboard.c 2 files changed, 44 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/1
diff --git a/payloads/libpayload/Kconfig b/payloads/libpayload/Kconfig index 97b970b..16e4c3a 100644 --- a/payloads/libpayload/Kconfig +++ b/payloads/libpayload/Kconfig @@ -353,6 +353,10 @@ bool "Ignore keyboard failures during init and always add input device" default n
+config PC_KEYBOARD_TREAT_MEDIA_KEYS_AS_FN + bool "Donot ignore prefix scan code OXE0 for some special keys" + default n + config PC_KEYBOARD_LAYOUT_US bool "English (US) keyboard layout" depends on PC_KEYBOARD diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index 48d35a0..e079a96 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -230,6 +230,40 @@ return modifier; }
+/* Ignore almost all special function keys & get the next key */ +/* F12 key generates 0xe0 0xb on press when fn is engaged. */ +static int keyboard_media_keys(void) +{ + unsigned char ch; + int ret = 0; + + ch = keyboard_get_scancode(); + switch (ch) { + case 0xb: + ret = KEY_F(12); + break; + case 0x5e: + ret = POWER_BUTTON; + break; + case 0x4b: + ret = KEY_LEFT; + break; + case 0x4d: + ret = KEY_RIGHT; + break; + case 0x48: + ret = KEY_UP; + break; + case 0x50: + ret = KEY_DOWN; + break; + default: + ret = 0; + } + printf("Teja: Returning %x \n", ret); + return ret; +} + int keyboard_getchar(void) { unsigned char ch; @@ -240,6 +274,12 @@
ch = keyboard_get_scancode();
+ /* process media keys, only return the keys that are needed */ + if (IS_ENABLED(CONFIG_LP_PC_KEYBOARD_TREAT_MEDIA_KEYS_AS_FN)) { + if (ch == 0xe0) { + return keyboard_media_keys(); + } + } if (!(ch & 0x80) && ch < 0x59) { shift = (modifier & KB_MOD_SHIFT) ^ (modifier & KB_MOD_CAPSLOCK) ? 1 : 0;
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36654/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/1/payloads/libpayload/drivers... PS1, Line 263: printf("Teja: Returning %x \n", ret); unnecessary whitespace before a quoted newline
https://review.coreboot.org/c/coreboot/+/36654/1/payloads/libpayload/drivers... PS1, Line 281: } trailing whitespace
Hello build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#2).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys.
These changes are enabled with a config flag in libpayload and by default it is disabled, whichever board needs it will enable it in its Kconfig.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/Kconfig M payloads/libpayload/drivers/i8042/keyboard.c 2 files changed, 43 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/2
Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36654/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/1/payloads/libpayload/drivers... PS1, Line 263: printf("Teja: Returning %x \n", ret);
unnecessary whitespace before a quoted newline
Done
https://review.coreboot.org/c/coreboot/+/36654/1/payloads/libpayload/drivers... PS1, Line 281: }
trailing whitespace
Done
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/2/payloads/libpayload/drivers... PS2, Line 241: switch (ch) { : case 0xb: : ret = KEY_F(12); : break; : case 0x5e: : ret = POWER_BUTTON; : break; : case 0x4b: : ret = KEY_LEFT; : break; : case 0x4d: : ret = KEY_RIGHT; : break; : case 0x48: : ret = KEY_UP; : break; : case 0x50: : ret = KEY_DOWN; : break; : default: : ret = 0; : } We don't want to hard code this list for everyone it will be different on each platform.
My thought is to have a mapping that can be passed into the keyboard handler on a per board basis in depthcharge.
something like
struct keyboard_media_key_mapping { const int map[256]; }
const struct keyboard_media_key_mapping *media_key_mapping;
void keyboard_set_media_key_map(const struct keyboard_media_key_mapping *map) { media_key_mapping = map; }
Then in depthcharge in board.c
const struct keyboard_media_key_mapping media_key_mapping = { .map = { [0x0b] = KEY_F(12), [0x5e] = POWER_BUTTON, ... } };
....
keyboard_set_media_key_map(&media_key_mapping);
I think I have my consts and struct initialization correct.
Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/2/payloads/libpayload/drivers... PS2, Line 241: switch (ch) { : case 0xb: : ret = KEY_F(12); : break; : case 0x5e: : ret = POWER_BUTTON; : break; : case 0x4b: : ret = KEY_LEFT; : break; : case 0x4d: : ret = KEY_RIGHT; : break; : case 0x48: : ret = KEY_UP; : break; : case 0x50: : ret = KEY_DOWN; : break; : default: : ret = 0; : }
We don't want to hard code this list for everyone it will be different on each platform. […]
Ack
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#3).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever mode require this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/Kconfig M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/drivers/usb/usbhid.c M payloads/libpayload/include/libpayload.h 4 files changed, 22 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/3
Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/2/payloads/libpayload/drivers... PS2, Line 241: switch (ch) { : case 0xb: : ret = KEY_F(12); : break; : case 0x5e: : ret = POWER_BUTTON; : break; : case 0x4b: : ret = KEY_LEFT; : break; : case 0x4d: : ret = KEY_RIGHT; : break; : case 0x48: : ret = KEY_UP; : break; : case 0x50: : ret = KEY_DOWN; : break; : default: : ret = 0; : }
Ack
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/3/payloads/libpayload/drivers... PS3, Line 233: void keyboard_set_media_key_map(const struct keyboard_media_key_mapping *mapping) { open brace '{' following function definitions go on the next line
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#4).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever mode require this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/Kconfig M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/drivers/usb/usbhid.c M payloads/libpayload/include/libpayload.h 4 files changed, 23 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/4
Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/3/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/3/payloads/libpayload/drivers... PS3, Line 233: void keyboard_set_media_key_map(const struct keyboard_media_key_mapping *mapping) {
open brace '{' following function definitions go on the next line
Done
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#5).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever mode require this change will update its own media key mappingi and also enable the Kconfig flag.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/Kconfig M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/drivers/usb/usbhid.c M payloads/libpayload/include/libpayload.h 4 files changed, 24 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/5
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36654/5/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/5/payloads/libpayload/drivers... PS5, Line 247: CONFIG_LP_PC_KEYBOARD_TREAT_MEDIA_KEYS_AS_FN I don't think we need a new config for this, just check that media_key_mapping is not null.
https://review.coreboot.org/c/coreboot/+/36654/5/payloads/libpayload/include... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/36654/5/payloads/libpayload/include... PS5, Line 185: int I know I had this as an int but I now think it should be an unsigned short
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#6).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever board require this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/drivers/usb/usbhid.c M payloads/libpayload/include/libpayload.h 3 files changed, 20 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/6
Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36654/5/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/5/payloads/libpayload/drivers... PS5, Line 247: CONFIG_LP_PC_KEYBOARD_TREAT_MEDIA_KEYS_AS_FN
I don't think we need a new config for this, just check that media_key_mapping is not null.
Done
https://review.coreboot.org/c/coreboot/+/36654/5/payloads/libpayload/include... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/36654/5/payloads/libpayload/include... PS5, Line 185: int
I know I had this as an int but I now think it should be an unsigned short
Agree, Done!
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/6/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/6/payloads/libpayload/drivers... PS6, Line 248: if (ch == 0xe0) { change 0xe0 to MEDIA_KEY_PREFIX or something like this?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/6/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/6/payloads/libpayload/drivers... PS6, Line 235: media_key_mapping = mapping; how about use the call back function? If we use array, this will contain most "0". Because we just need deal with six keys right now.
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#7).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever board require this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/drivers/usb/usbhid.c M payloads/libpayload/include/libpayload.h 3 files changed, 21 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/7
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#8).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever board require this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/drivers/usb/usbhid.c M payloads/libpayload/include/libpayload.h 3 files changed, 18 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/8/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/8/payloads/libpayload/drivers... PS8, Line 46: int (*media_key_mapping_callback)(char); function definition argument 'char' should also have an identifier name
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#9).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever board require this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/drivers/usb/usbhid.c M payloads/libpayload/include/libpayload.h 3 files changed, 17 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/9/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/9/payloads/libpayload/drivers... PS9, Line 46: int (*media_key_mapping_callback)(char); function definition argument 'char' should also have an identifier name
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#10).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever board require this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/drivers/usb/usbhid.c M payloads/libpayload/include/libpayload.h 3 files changed, 17 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/10
Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36654/6/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/6/payloads/libpayload/drivers... PS6, Line 235: media_key_mapping = mapping;
how about use the call back function? If we use array, this will contain most "0". […]
Done
https://review.coreboot.org/c/coreboot/+/36654/6/payloads/libpayload/drivers... PS6, Line 248: if (ch == 0xe0) {
change 0xe0 to MEDIA_KEY_PREFIX or something like this?
Done
https://review.coreboot.org/c/coreboot/+/36654/8/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/8/payloads/libpayload/drivers... PS8, Line 46: int (*media_key_mapping_callback)(char);
function definition argument 'char' should also have an identifier name
Done
https://review.coreboot.org/c/coreboot/+/36654/9/payloads/libpayload/drivers... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/9/payloads/libpayload/drivers... PS9, Line 46: int (*media_key_mapping_callback)(char);
function definition argument 'char' should also have an identifier name
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/driver... File payloads/libpayload/drivers/usb/usbhid.c:
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/driver... PS10, Line 32: #include <libpayload.h> why change this?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/includ... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/includ... PS10, Line 68: #include <keycodes.h> is this needed?
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#11).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever board require this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/include/libpayload.h 2 files changed, 20 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/11
Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/driver... File payloads/libpayload/drivers/usb/usbhid.c:
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/driver... PS10, Line 32: #include <libpayload.h>
why change this?
Reverted as it is not required anymore.
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/includ... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/includ... PS10, Line 68: #include <keycodes.h>
is this needed?
Yes, all the keyboard macros are declared in keycodes.h, this header is included here so that libpayload will have all that macro definitions required in depthcharge for the fn(keyboard_media_key_mapping).
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#12).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever board require this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/curses/local.h M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/include/libpayload.h 3 files changed, 20 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/12
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/includ... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/includ... PS10, Line 68: #include <keycodes.h>
Yes, all the keyboard macros are declared in keycodes. […]
I can include keycodes.h in depthcharge. Could you try add this in board.c?
Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/includ... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/includ... PS10, Line 68: #include <keycodes.h>
I can include keycodes.h in depthcharge. Could you try add this in board. […]
Sure Eric, we can do that.
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#13).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever board require this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/include/libpayload.h 2 files changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/13
Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/includ... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/includ... PS10, Line 68: #include <keycodes.h>
Sure Eric, we can do that.
sorry for the confusion, keycodes.h is under libpayload/include. So, I included it directly in depthcharge(board.c) as it has access to LIBPAYLOAD_DIR(/build/drallion/firmware/libpayload)
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 13: Code-Review+1
Seems better, wait for the Mat's comment.
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36654/13/payloads/libpayload/driver... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/13/payloads/libpayload/driver... PS13, Line 249: if (media_key_mapping_callback != NULL) { : if (ch == MEDIA_KEY_PREFIX) { nit: this could be a single statement
https://review.coreboot.org/c/coreboot/+/36654/13/payloads/libpayload/driver... PS13, Line 252: ret = media_key_mapping_callback(ch); : return ret; nit: return media_key_mapping_callback(ch);
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#14).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever board require this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/include/libpayload.h 2 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/14
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/14/payloads/libpayload/driver... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/14/payloads/libpayload/driver... PS14, Line 251: return (media_key_mapping_callback(ch)); return is not a function, parentheses are not required
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#15).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0, we will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever board require this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/include/libpayload.h 2 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/15
Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36654/13/payloads/libpayload/driver... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/13/payloads/libpayload/driver... PS13, Line 249: if (media_key_mapping_callback != NULL) { : if (ch == MEDIA_KEY_PREFIX) {
nit: this could be a single statement
Done
https://review.coreboot.org/c/coreboot/+/36654/13/payloads/libpayload/driver... PS13, Line 252: ret = media_key_mapping_callback(ch); : return ret;
nit: return media_key_mapping_callback(ch);
Done
https://review.coreboot.org/c/coreboot/+/36654/14/payloads/libpayload/driver... File payloads/libpayload/drivers/i8042/keyboard.c:
https://review.coreboot.org/c/coreboot/+/36654/14/payloads/libpayload/driver... PS14, Line 251: return (media_key_mapping_callback(ch));
return is not a function, parentheses are not required
Done
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 15: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 15:
(3 comments)
The motivation for this change is unclear to me.
https://review.coreboot.org/c/coreboot/+/36654/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36654/13//COMMIT_MSG@9 PS13, Line 9: , we will These are two sentences: … 0xE0. We will …
https://review.coreboot.org/c/coreboot/+/36654/13//COMMIT_MSG@10 PS13, Line 10: these except for the power button, F12 and cursor keys on drallion. Why should these be ignored? What effect do these presses currently have to be ignored?
https://review.coreboot.org/c/coreboot/+/36654/13//COMMIT_MSG@13 PS13, Line 13: require requires
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36654/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36654/13//COMMIT_MSG@10 PS13, Line 10: these except for the power button, F12 and cursor keys on drallion.
Why should these be ignored? What effect do these presses currently have to be ignored?
those are we want handled and omit others.
Hello EricR Lai, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Thejaswani Putta,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36654
to look at the new patch set (#16).
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0. We will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever board requires this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/include/libpayload.h 2 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36654/16
Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 16:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36654/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36654/13//COMMIT_MSG@9 PS13, Line 9: , we will
These are two sentences: … 0xE0. […]
Done
https://review.coreboot.org/c/coreboot/+/36654/13//COMMIT_MSG@10 PS13, Line 10: these except for the power button, F12 and cursor keys on drallion.
those are we want handled and omit others.
Ack
https://review.coreboot.org/c/coreboot/+/36654/13//COMMIT_MSG@13 PS13, Line 13: require
requires
Done
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 16:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/driver... File payloads/libpayload/drivers/usb/usbhid.c:
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/driver... PS10, Line 32: #include <libpayload.h>
Reverted as it is not required anymore.
Done
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/includ... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/36654/10/payloads/libpayload/includ... PS10, Line 68: #include <keycodes.h>
sorry for the confusion, keycodes.h is under libpayload/include. […]
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
Patch Set 16: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36654 )
Change subject: libpayload: keyboard: Ignore special keys ......................................................................
libpayload: keyboard: Ignore special keys
Some special keys emit a prefix scan code 0xE0. We will ignore all these except for the power button, F12 and cursor keys on drallion.
Media key mapping is set in depthcharge and will be sent to libpayload keyboard driver. Whichever board requires this change will update its own media key mapping.
BUG:b:139511038 TEST=boot in recovery mode, press F12 to go to diagnostic mode and power button to confirm. Also in recovery mode left arrow, right arrow, up arrow, down arrow changes the language on the firmware screen.
Change-Id: I1c11939d18391bebe53ca21cf33a096ba369cd56 Signed-off-by: Thejaswani Putta thejaswani.putta@intel.corp-partner.google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36654 Reviewed-by: EricR Lai ericr_lai@compal.corp-partner.google.com Reviewed-by: Mathew King mathewk@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M payloads/libpayload/drivers/i8042/keyboard.c M payloads/libpayload/include/libpayload.h 2 files changed, 12 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified EricR Lai: Looks good to me, approved Mathew King: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/i8042/keyboard.c b/payloads/libpayload/drivers/i8042/keyboard.c index 48d35a0..f9932ed 100644 --- a/payloads/libpayload/drivers/i8042/keyboard.c +++ b/payloads/libpayload/drivers/i8042/keyboard.c @@ -35,6 +35,7 @@ #include "i8042.h"
#define POWER_BUTTON 0x90 +#define MEDIA_KEY_PREFIX 0xE0
struct layout_maps { const char *country; @@ -43,6 +44,7 @@
static struct layout_maps *map; static int modifier = 0; +int (*media_key_mapping_callback)(char ch);
static struct layout_maps keyboard_layouts[] = { #if CONFIG(LP_PC_KEYBOARD_LAYOUT_US) @@ -230,6 +232,11 @@ return modifier; }
+void initialize_keyboard_media_key_mapping_callback(int (*media_key_mapper)(char)) +{ + media_key_mapping_callback = media_key_mapper; +} + int keyboard_getchar(void) { unsigned char ch; @@ -239,6 +246,10 @@ while (!keyboard_havechar()) ;
ch = keyboard_get_scancode(); + if ((media_key_mapping_callback != NULL) && (ch == MEDIA_KEY_PREFIX)) { + ch = keyboard_get_scancode(); + return media_key_mapping_callback(ch); + }
if (!(ch & 0x80) && ch < 0x59) { shift = diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index 80bfaae..bfe9da5 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -187,6 +187,7 @@ int keyboard_getchar(void); int keyboard_set_layout(char *country); int keyboard_getmodifier(void); +void initialize_keyboard_media_key_mapping_callback(int (*media_key_mapper)(char));
enum KEYBOARD_MODIFIERS { KB_MOD_SHIFT = (1 << 0),
-
build bot (Jenkins) (Code Review) -
EricR Lai (Code Review)
-
Mathew King (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Thejaswani Putta (Code Review)