I stared working on a depthcharge CL, because currently depthcharge just uses EC_CMD_MKBP_STATE to get the current keypresses. Formerly, the keyboard FIFO was just that, a FIFO that contain key matrices. However, this will not be the case since the FIFO is now a general events FIFO which can contain button or switch presses.

I hadn't looked at depthcharge code before, but I started on a draft CL. Unfortunately, I was having issues with the AP booting which I'll investigate next week.

https://chromium-review.googlesource.com/#/c/358989/

I remember that Vincent and I once decided to keep the firmware on the old EC_CMD_MKBP_STATE API for a reason, although I don't recall all the details and intricacies that went into that decision (see http://crosreview.com/274070 and http://crosbug.com/p/40771 for some context). I guess one of the issues might be loosing events... your patch looks like it will just drop anything that's not a keypress on the floor. Are we okay with that, or shouldn't we try to leave them available until the kernel is up to read them?

FWIW, are extra event types really the best way to handle this? Wouldn't it also be possible (and maybe cleaner?) to add a new "virtual" row or column to the key matrix and report all of these special keys and switches as if they were on there? Why should the interface make such an architectural difference between the volume button on the C-panel and the one on the side?

Oh, and also, maybe this is all moot because I'm not sure if we need or want to read these keys through depthcharge's havekey()/getchar() anyway. We don't (currently) need the volume buttons, and the power button has always been special (handled like a GPIO) anyway. So maybe all we need is a depthcharge GPIO driver that wraps reading the EC. (In fact, the power button was already implemented with chrome-os-partner:53208, so what else are we actually trying to achieve here?)

Yeah, I wasn't sure about handling the other events in the FIFO. This is the big issue. Would the kernel care about those events? The other items in the FIFO are (for now) button presses and switch transitions. I believe the kernel will also query the state of the switches upon init.

The issue with adding another row or column was a lot of hard-coded assumptions about the key matrix size and ghosting. See chrome-os-partner:54988 c#6. Additionally, I had a discussion with Doug that some upstream folks may prefer the buttons on the side to behave different than the volume button on the C-panel. Maybe they actually want it to be F9/F10 and have the side buttons be different.

A change needs to be made here because as it stands, no keyboard presses will be recognized during the firmware screens since EC_CMD_MKBP_STATE is dropped.

> Maybe they actually want it to be F9/F10 and have the side buttons be different.

Sure... I mean, I'm not saying they should count as the same button. They should be different just like left and right Alt are different. But I think it might be more natural if they were reported through the same mechanism.

> A change needs to be made here because as it stands, no keyboard presses will be recognized during the firmware screens since EC_CMD_MKBP_STATE is dropped.

What do you mean, being dropped? Are you saying you plan to deprecate the whole command in the EC code? Why? 

@2: we thought about putting them on the matrix, but it was ugly due because of both ghosting reasons (kernel has ghost detection so it would need a special case to exempt a row or column from ghost detection) and because of the hardcoding of the number 13, as Aseda mentioned.

Also: I just realized that we may need (in the kernel) to move handling of these keys to a separate driver since the main keyboard gets "inhibited" in the kernel, but we don't want to inhibit switches like the lid switch.  There might be a better way to solve this--need to check.

>Sure... I mean, I'm not saying they should count as the same button. They should be different just like left and right Alt are different. But I think it might be more natural if they were reported through the same mechanism.

Ah, sorry for misunderstanding. I think since they are non-matrixed keys, they shouldn't be placed in the key matrix. The virtual battery key seemed weird enough...

>What do you mean, being dropped? Are you saying you plan to deprecate the whole command in the EC code? Why?

Yes, because that command assumed that the FIFO was just a keyboard FIFO and the EC was simply popping off an element from the FIFO. But now, that won't be the case. Additionally, we have EC_CMD_GET_NEXT_EVENT which should be used instead.

> Yes, because that command assumed that the FIFO was just a keyboard FIFO and the EC was simply popping off an element from the FIFO. But now, that won't be the case. Additionally, we have EC_CMD_GET_NEXT_EVENT which should be used instead.

Sorry, are you saying "now" as in "now that we need to read other FIFOs"? My question was in relation to what I mentioned in #2, that I don't think we need to read any of these keys through MKBP anyway (at least for Kevin). We're not interested in volume keys in firmware, and the power button is handled differently anyway.

We can still do this just to prepare for the next Ryu or whatever, but it doesn't need to be rushed to go into Kevin then. We should probably take our time to really think the issue with dropped events through (I don't really know what kinds of events we're sending right now, and what additional ones we might want to start sending in the future).

Sorry, I meant "now" as in with my outstanding EC changes to have button and switch events go through the same keyboard FIFO for MKBP. If/when those EC patches land, key scanning won't work as-is because the EC_CMD_MKBP_GET_STATE host command will be unsupported. I think that's problematic for Kevin since they'll probably want to test input at the firmware screens (Tab, Ctrl+D, Ctrl+U, etc.)

I agree that firmware doesn't care about volume buttons (for now), but I'm concerned about fixing key scanning.

The MKBP events that can currently be set are:
- Keyboard matrix changed
- new host event
- new sensor fifo data

I proposed adding:
- non-matrix buttons changed
- switches have changed

Ah, okay, got it. So your changes to the EC fifo stuff are so intrusive that you can't leave the old command with its behavior alive, essentially.

I'll let you EC guys decide how you want to solve this, then. If dropping all non-keyboard events on the floor is fine I'm happy to take that on the AP firmware side. If not you'll have to come up with something clever.

>Ah, okay, got it. So your changes to the EC fifo stuff are so intrusive that you can't leave the old command with its behavior alive, essentially.

Yep, precisely.

I think it's okay to drop non-keyboard events for now as the only other events in the FIFO would be buttons and switches. And as stated earlier, for those that you care about, you handle them in other ways.

But if I'm failing to see some reason why we we shouldn't drop non-keyboard events, don't hesitate to let me know.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/824f9fadc257ecbbaab2e2296f56dfc72325dd7f

commit 824f9fadc257ecbbaab2e2296f56dfc72325dd7f
Author: Aseda Aboagye <aaboagye@google.com>
Date: Fri Jul 08 16:24:20 2016

mkbp: Extend EC_CMD_MKBP_GET_INFO.

- Added ability to query the buttons and switches.
- Added ability to report the available buttons or switches.

BUG= chromium:626863 
BRANCH=None
TEST=make -j buildall

CQ-DEPEND=CL:358633
CQ-DEPEND=CL:358634
CQ-DEPEND=CL:358989

Change-Id: Ie821491269e8d09578eba92127895c0b6b8e91a9
Signed-off-by: Aseda Aboagye <aaboagye@google.com>
Reviewed-on: https://chromium-review.googlesource.com/358926
Commit-Ready: Aseda Aboagye <aaboagye@chromium.org>
Tested-by: Aseda Aboagye <aaboagye@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>

[modify] https://crrev.com/824f9fadc257ecbbaab2e2296f56dfc72325dd7f/include/ec_commands.h
[modify] https://crrev.com/824f9fadc257ecbbaab2e2296f56dfc72325dd7f/common/keyboard_mkbp.c

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/0325284e17c04dbd03552f2b152cf3e079d7148f

commit 0325284e17c04dbd03552f2b152cf3e079d7148f
Author: Aseda Aboagye <aaboagye@google.com>
Date: Thu Jul 07 04:11:01 2016

mkbp: Add keyboard_update_button().

MKBP can now support buttons, so this commit adds the
keyboard_update_button() function which will be used to handle the
non-matrixed buttons.

BUG=chrome-os-partner:54976
BUG= chromium:626863 
BRANCH=None
TEST=Flash kevin, press volume and power buttons and verify that
keyboard is still functional.
TEST=make -j buildall

CQ-DEPEND=CL:358633

Change-Id: I1c2d36d2113715cf6bd8c6fa7b26fe9253f6ac9f
Signed-off-by: Aseda Aboagye <aaboagye@google.com>
Reviewed-on: https://chromium-review.googlesource.com/358634
Commit-Ready: Aseda Aboagye <aaboagye@chromium.org>
Tested-by: Aseda Aboagye <aaboagye@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>

[modify] https://crrev.com/0325284e17c04dbd03552f2b152cf3e079d7148f/common/button.c
[modify] https://crrev.com/0325284e17c04dbd03552f2b152cf3e079d7148f/common/keyboard_mkbp.c

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/87a071941b89e3f7fd3eb329b682e60b3fbd6c73

commit 87a071941b89e3f7fd3eb329b682e60b3fbd6c73
Author: Aseda Aboagye <aaboagye@google.com>
Date: Thu Jul 07 02:37:00 2016

mkbp: Add support for buttons and switches.

Currently, the matrix keyboard protocol does not have support for
handling non-matrixed keys.  This commit adds support for buttons which
do not appear in the keyboard matrix as well as switches.

Additionally, the keyboard FIFO is now just a general MKBP events FIFO
which MKBP events are free to use. Now, buttons and switches wil join
the key matrix event.

BUG=chrome-os-partner:54988
BUG=chrome-os-partner:54976
BUG= chromium:626863 
BRANCH=None
TEST=Flash kevin, and verify that keyboard is still functional.
TEST=make -j buildall

CQ-DEPEND=CL:358926

Change-Id: If4ada904cbd5d77823a0710d4671484b198c9d91
Signed-off-by: Aseda Aboagye <aaboagye@google.com>
Reviewed-on: https://chromium-review.googlesource.com/358633
Commit-Ready: Aseda Aboagye <aaboagye@chromium.org>
Tested-by: Aseda Aboagye <aaboagye@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>

[modify] https://crrev.com/87a071941b89e3f7fd3eb329b682e60b3fbd6c73/common/keyboard_mkbp.c
[modify] https://crrev.com/87a071941b89e3f7fd3eb329b682e60b3fbd6c73/common/build.mk
[modify] https://crrev.com/87a071941b89e3f7fd3eb329b682e60b3fbd6c73/include/keyboard_mkbp.h
[modify] https://crrev.com/87a071941b89e3f7fd3eb329b682e60b3fbd6c73/common/mkbp_event.c
[modify] https://crrev.com/87a071941b89e3f7fd3eb329b682e60b3fbd6c73/test/test_config.h
[modify] https://crrev.com/87a071941b89e3f7fd3eb329b682e60b3fbd6c73/include/config.h
[modify] https://crrev.com/87a071941b89e3f7fd3eb329b682e60b3fbd6c73/core/host/host_exe.lds
[modify] https://crrev.com/87a071941b89e3f7fd3eb329b682e60b3fbd6c73/test/kb_mkbp.c
[modify] https://crrev.com/87a071941b89e3f7fd3eb329b682e60b3fbd6c73/include/ec_commands.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/depthcharge/+/f88af26b44fc8941a59b45dfb36f8fe4225e07d0

commit f88af26b44fc8941a59b45dfb36f8fe4225e07d0
Author: Aseda Aboagye <aaboagye@google.com>
Date: Sat Jul 09 01:20:32 2016

cros_ec: Change keyboard scanning method.

The Matrix KeyBoard Protocol (MKBP) was recently extended to support
reporting button and switch events. Consequently, support for the
EC_CMD_MKBP_GET_STATE host command was dropped since the keyboard FIFO
became a general MKBP events FIFO and that host command assumed that the
FIFO was strictly for keyboard matrices.  This has been superseded by the
EC_CMD_GET_NEXT_EVENT host command.

Currently, button and switch events may be present in the FIFO, but we
will just ignore those and drop them on the floor.  For those buttons
and switches that we care about, they are handled in different ways.

This commit introduces a new Kconfig:

    CONFIG_DRIVER_INPUT_MKBP_OLD_COMMAND

This config option should be used to enable retrieving of key matrix
changes via EC_CMD_MKBP_GET_STATE. This is for legacy Chrome ECs where
the MKBP FIFO was solely just a key matrix FIFO.

BUG= chromium:626863 
BRANCH=None
TEST=Verify that keypresses are recognized during FW screens on kevin.
TEST=Verify that button presses and other events are ignored during FW
screens on kevin.

CQ-DEPEND=CL:358926

Change-Id: Ia9cf29b063178b9eca20e9d2602dc91308e56d4a
Signed-off-by: Aseda Aboagye <aaboagye@google.com>
Reviewed-on: https://chromium-review.googlesource.com/358989
Commit-Ready: Aseda Aboagye <aaboagye@chromium.org>
Tested-by: Aseda Aboagye <aaboagye@chromium.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>

[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/nyan_blaze/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/veyron_jerry/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/src/drivers/ec/cros/commands.h
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/rush/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/elm/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/src/drivers/ec/cros/ec.h
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/daisy/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/src/drivers/ec/cros/ec.c
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/veyron_shark/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/oak/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/veyron_thea/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/veyron_mighty/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/nyan/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/veyron_speedy/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/peach_pit/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/veyron_gus/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/src/drivers/input/mkbp/keyboard.c
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/veyron_jaq/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/nyan_big/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/veyron_pinky/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/veyron_minnie/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/board/veyron_nicky/defconfig
[modify] https://crrev.com/f88af26b44fc8941a59b45dfb36f8fe4225e07d0/src/drivers/input/mkbp/Kconfig

EC side is done, I believe.

This works.