On 10/30/20 6:25 PM, Alper Nebi Yasak wrote:
> The cros_ec_keyb driver currently uses EC_CMD_MKBP_STATE to scan the
> keyboard, but this host command was superseded by EC_CMD_GET_NEXT_EVENT
> and unavailable on more recent devices (including gru-kevin), as it was
> removed in cros-ec commit 87a071941b89 ("mkbp: Add support for buttons
> and switches.") dated 2016-07-06.
>
> The EC_CMD_GET_NEXT_EVENT has been available since cros-ec commit
> d1ed75815efe ("MKBP event signalling implementation") dated 2014-10-20,
> but it looks like it isn't included in firmware-* branches for at least
> link, nyan-big, samus, snow, spring, panther and peach-pit which have
> defconfigs in U-Boot. So this patch falls back to the old method if the
> EC doesn't recognize the newer command.
>
> The implementation is mostly adapted from Depthcharge commit
> f88af26b44fc ("cros_ec: Change keyboard scanning method.").
>
> On a gru-kevin, the current driver before this patch fails to read the
> pressed keys with:
>
> out: cmd=0x60: 03 9d 60 00 00 00 00 00
> in-header: 03 fc 01 00 00 00 00 00
> in-data:
> ec_command_inptr: len=-1, din=0000000000000000
> check_for_keys: keyboard scan failed
>
> However the keyboard works fine with the newer command:
>
> out: cmd=0x67: 03 96 67 00 00 00 00 00
> in-header: 03 ef 00 00 0e 00 00 00
> in-data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ec_command_inptr: len=14, din=00000000f412df30
> key_matrix_decode: num_keys = 0
> 0 valid keycodes found
> out: cmd=0x67: 03 96 67 00 00 00 00 00
> in-header: 03 df 00 00 0e 00 00 00
> in-data: 00 00 00 00 00 00 00 00 00 00 00 00 10 00
> ec_command_inptr: len=14, din=00000000f412df30
> key_matrix_decode: num_keys = 1
> valid=1, row=4, col=11
> keycode=28
> 1 valid keycodes found
> {0d}
>
> Signed-off-by: Alper Nebi Yasak <alpernebiya...@gmail.com>
> Reviewed-by: Simon Glass <s...@chromium.org>
This patch has been applied to origin/master.
Now when I compile sandbox_defconfig and run './u-boot -D' it spits out
zillions of
** Unknown EC command 0x67
** Unknown EC command 0x67
** Unknown EC command 0x67
** Unknown EC command 0x67
** Unknown EC command 0x67
** Unknown EC command 0x67
** Unknown EC command 0x67
** Unknown EC command 0x67
When I revert the patch the messages are gone.
So something is really missing in this patch.
Best regards
Heinrich
> ---
>
> drivers/input/cros_ec_keyb.c | 32 ++++++++++++++++++++++++++------
> drivers/misc/cros_ec.c | 15 +++++++++++++++
> include/cros_ec.h | 11 +++++++++++
> 3 files changed, 52 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/cros_ec_keyb.c b/drivers/input/cros_ec_keyb.c
> index 00bf58f2b5d2..0c0f52205b28 100644
> --- a/drivers/input/cros_ec_keyb.c
> +++ b/drivers/input/cros_ec_keyb.c
> @@ -47,15 +47,35 @@ static int check_for_keys(struct udevice *dev, struct
> key_matrix_key *keys,
> struct key_matrix_key *key;
> static struct mbkp_keyscan last_scan;
> static bool last_scan_valid;
> - struct mbkp_keyscan scan;
> + struct ec_response_get_next_event event;
> + struct mbkp_keyscan *scan = (struct mbkp_keyscan *)
> + &event.data.key_matrix;
> unsigned int row, col, bit, data;
> int num_keys;
> + int ret;
>
> - if (cros_ec_scan_keyboard(dev->parent, &scan)) {
> - debug("%s: keyboard scan failed\n", __func__);
> + /* Get pending MKBP event. It may not be a key matrix event. */
> + do {
> + ret = cros_ec_get_next_event(dev->parent, &event);
> + /* The EC has no events for us at this time. */
> + if (ret == -EC_RES_UNAVAILABLE)
> + return -EIO;
> + else if (ret)
> + break;
> + } while (event.event_type != EC_MKBP_EVENT_KEY_MATRIX);
> +
> + /* Try the old command if the EC doesn't support the above. */
> + if (ret == -EC_RES_INVALID_COMMAND) {
> + if (cros_ec_scan_keyboard(dev->parent, scan)) {
> + debug("%s: keyboard scan failed\n", __func__);
> + return -EIO;
> + }
> + } else if (ret) {
> + debug("%s: Error getting next MKBP event. (%d)\n",
> + __func__, ret);
> return -EIO;
> }
> - *samep = last_scan_valid && !memcmp(&last_scan, &scan, sizeof(scan));
> + *samep = last_scan_valid && !memcmp(&last_scan, scan, sizeof(*scan));
>
> /*
> * This is a bit odd. The EC has no way to tell us that it has run
> @@ -64,14 +84,14 @@ static int check_for_keys(struct udevice *dev, struct
> key_matrix_key *keys,
> * that this scan is the same as the last.
> */
> last_scan_valid = true;
> - memcpy(&last_scan, &scan, sizeof(last_scan));
> + memcpy(&last_scan, scan, sizeof(last_scan));
>
> for (col = num_keys = bit = 0; col < priv->matrix.num_cols;
> col++) {
> for (row = 0; row < priv->matrix.num_rows; row++) {
> unsigned int mask = 1 << (bit & 7);
>
> - data = scan.data[bit / 8];
> + data = scan->data[bit / 8];
> if ((data & mask) && num_keys < max_count) {
> key = keys + num_keys++;
> key->row = row;
> diff --git a/drivers/misc/cros_ec.c b/drivers/misc/cros_ec.c
> index a5534b16673b..c3674908ee82 100644
> --- a/drivers/misc/cros_ec.c
> +++ b/drivers/misc/cros_ec.c
> @@ -415,6 +415,21 @@ int cros_ec_scan_keyboard(struct udevice *dev, struct
> mbkp_keyscan *scan)
> return 0;
> }
>
> +int cros_ec_get_next_event(struct udevice *dev,
> + struct ec_response_get_next_event *event)
> +{
> + int ret;
> +
> + ret = ec_command(dev, EC_CMD_GET_NEXT_EVENT, 0, NULL, 0,
> + event, sizeof(*event));
> + if (ret < 0)
> + return ret;
> + else if (ret != sizeof(*event))
> + return -EC_RES_INVALID_RESPONSE;
> +
> + return 0;
> +}
> +
> int cros_ec_read_id(struct udevice *dev, char *id, int maxlen)
> {
> struct ec_response_get_version *r;
> diff --git a/include/cros_ec.h b/include/cros_ec.h
> index f4b9b7a5c26e..f187bd0d4b5b 100644
> --- a/include/cros_ec.h
> +++ b/include/cros_ec.h
> @@ -82,6 +82,17 @@ int cros_ec_read_id(struct udevice *dev, char *id, int
> maxlen);
> */
> int cros_ec_scan_keyboard(struct udevice *dev, struct mbkp_keyscan *scan);
>
> +/**
> + * Get the next pending MKBP event from the ChromeOS EC device.
> + *
> + * Send a message requesting the next event and return the result.
> + *
> + * @param event Place to put the event.
> + * @return 0 if ok, <0 on error.
> + */
> +int cros_ec_get_next_event(struct udevice *dev,
> + struct ec_response_get_next_event *event);
> +
> /**
> * Read which image is currently running on the CROS-EC device.
> *
>
