[PATCH 1/1] cros_ec: Handling EC_CMD_GET_NEXT_EVENT
With commit 690079767803 ("cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT") check_for_keys() tries to read keyboard strokes using EC_CMD_GET_NEXT_EVENT. But the sandbox driver does not understand this command. We need to reply with -EC_RES_INVALID_COMMAND to force check_for_keys() to fall back to use EC_CMD_MKBP_STATE. Currently the driver prints
** Unknown EC command 0x67
in this case. With the patch the message is suppressed.
In a future patch we should upgrade the sandbox driver to provide EC_CMD_GET_NEXT_EVENT support.
Fixes: 690079767803 ("cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT") Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- process_cmd() should always return an appropriate negative enum ec_status in case of an error and not simply -1. But fixing the return values is beyond the scope of this patch. --- drivers/misc/cros_ec_sandbox.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/misc/cros_ec_sandbox.c b/drivers/misc/cros_ec_sandbox.c index a191f061b8..ff7f782742 100644 --- a/drivers/misc/cros_ec_sandbox.c +++ b/drivers/misc/cros_ec_sandbox.c @@ -460,6 +460,16 @@ static int process_cmd(struct ec_state *ec, case EC_CMD_ENTERING_MODE: len = 0; break; + case EC_CMD_GET_NEXT_EVENT: + /* + * TODO: + * This driver emulates an old keyboard device supporting + * EC_CMD_MKBP_STATE. Current Chrome OS keyboards use + * EC_CMD_GET_NEXT_EVENT. Cf. + * "mkbp: Add support for buttons and switches" + * https://chromium.googlesource.com/chromiumos/platform/ec/+/87a071941b89e3f7f... + */ + return -EC_RES_INVALID_COMMAND; default: printf(" ** Unknown EC command %#02x\n", req_hdr->command); return -1; -- 2.28.0
On 09/11/2020 23:34, Heinrich Schuchardt wrote:
With commit 690079767803 ("cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT") check_for_keys() tries to read keyboard strokes using EC_CMD_GET_NEXT_EVENT. But the sandbox driver does not understand this command. We need to reply with -EC_RES_INVALID_COMMAND to force check_for_keys() to fall back to use EC_CMD_MKBP_STATE. Currently the driver prints
** Unknown EC command 0x67in this case. With the patch the message is suppressed.
In a future patch we should upgrade the sandbox driver to provide EC_CMD_GET_NEXT_EVENT support.
Fixes: 690079767803 ("cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT") Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
process_cmd() should always return an appropriate negative enum ec_status in case of an error and not simply -1. But fixing the return values is beyond the scope of this patch.
(Looks to me like -1 is already == -EC_RES_INVALID_COMMAND from include/ec_commands.h definitions, but I'd agree the latter form should be preferred.)
drivers/misc/cros_ec_sandbox.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/misc/cros_ec_sandbox.c b/drivers/misc/cros_ec_sandbox.c index a191f061b8..ff7f782742 100644 --- a/drivers/misc/cros_ec_sandbox.c +++ b/drivers/misc/cros_ec_sandbox.c @@ -460,6 +460,16 @@ static int process_cmd(struct ec_state *ec, case EC_CMD_ENTERING_MODE: len = 0; break;
- case EC_CMD_GET_NEXT_EVENT:
/** TODO:* This driver emulates an old keyboard device supporting* EC_CMD_MKBP_STATE. Current Chrome OS keyboards use* EC_CMD_GET_NEXT_EVENT. Cf.* "mkbp: Add support for buttons and switches"* https://chromium.googlesource.com/chromiumos/platform/ec/+/87a071941b89e3f7fd3eb329b682e60b3fbd6c73*/return -EC_RES_INVALID_COMMAND;
I'll try implementing the TODO, sorry for the fallout.
default: printf(" ** Unknown EC command %#02x\n", req_hdr->command); return -1; -- 2.28.0
On 11/9/20 10:13 PM, Alper Nebi Yasak wrote:
On 09/11/2020 23:34, Heinrich Schuchardt wrote:
With commit 690079767803 ("cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT") check_for_keys() tries to read keyboard strokes using EC_CMD_GET_NEXT_EVENT. But the sandbox driver does not understand this command. We need to reply with -EC_RES_INVALID_COMMAND to force check_for_keys() to fall back to use EC_CMD_MKBP_STATE. Currently the driver prints
** Unknown EC command 0x67in this case. With the patch the message is suppressed.
In a future patch we should upgrade the sandbox driver to provide EC_CMD_GET_NEXT_EVENT support.
Fixes: 690079767803 ("cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT") Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
process_cmd() should always return an appropriate negative enum ec_status in case of an error and not simply -1. But fixing the return values is beyond the scope of this patch.
(Looks to me like -1 is already == -EC_RES_INVALID_COMMAND from include/ec_commands.h definitions, but I'd agree the latter form should be preferred.)
If you look at the complete function, you will find other "return -1;" statements where return codes other than -EC_RES_INVALID_COMMAND make more sense. E.g. after
printf("** Unknown flash region %d\n", req->region);
it would be reasonable to return EC_RES_INVALID_PARAM.
Best regards
Heinrich
drivers/misc/cros_ec_sandbox.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/misc/cros_ec_sandbox.c b/drivers/misc/cros_ec_sandbox.c index a191f061b8..ff7f782742 100644 --- a/drivers/misc/cros_ec_sandbox.c +++ b/drivers/misc/cros_ec_sandbox.c @@ -460,6 +460,16 @@ static int process_cmd(struct ec_state *ec, case EC_CMD_ENTERING_MODE: len = 0; break;
- case EC_CMD_GET_NEXT_EVENT:
/** TODO:* This driver emulates an old keyboard device supporting* EC_CMD_MKBP_STATE. Current Chrome OS keyboards use* EC_CMD_GET_NEXT_EVENT. Cf.* "mkbp: Add support for buttons and switches"* https://chromium.googlesource.com/chromiumos/platform/ec/+/87a071941b89e3f7fd3eb329b682e60b3fbd6c73*/return -EC_RES_INVALID_COMMAND;I'll try implementing the TODO, sorry for the fallout.
default: printf(" ** Unknown EC command %#02x\n", req_hdr->command); return -1; -- 2.28.0
Hi,
On Mon, 9 Nov 2020 at 14:25, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 11/9/20 10:13 PM, Alper Nebi Yasak wrote:
On 09/11/2020 23:34, Heinrich Schuchardt wrote:
With commit 690079767803 ("cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT") check_for_keys() tries to read keyboard strokes using EC_CMD_GET_NEXT_EVENT. But the sandbox driver does not understand this command. We need to reply with -EC_RES_INVALID_COMMAND to force check_for_keys() to fall back to use EC_CMD_MKBP_STATE. Currently the driver prints
** Unknown EC command 0x67in this case. With the patch the message is suppressed.
In a future patch we should upgrade the sandbox driver to provide EC_CMD_GET_NEXT_EVENT support.
Fixes: 690079767803 ("cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT") Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
process_cmd() should always return an appropriate negative enum ec_status in case of an error and not simply -1. But fixing the return values is beyond the scope of this patch.
(Looks to me like -1 is already == -EC_RES_INVALID_COMMAND from include/ec_commands.h definitions, but I'd agree the latter form should be preferred.)
If you look at the complete function, you will find other "return -1;" statements where return codes other than -EC_RES_INVALID_COMMAND make more sense. E.g. after
printf("** Unknown flash region %d\n", req->region);it would be reasonable to return EC_RES_INVALID_PARAM.
Best regards
Heinrich
drivers/misc/cros_ec_sandbox.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
Shall we take Alper's patch to implement this command?
Regards, Simon
Hi,
On Mon, 9 Nov 2020 at 14:25, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 11/9/20 10:13 PM, Alper Nebi Yasak wrote:
On 09/11/2020 23:34, Heinrich Schuchardt wrote:
With commit 690079767803 ("cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT") check_for_keys() tries to read keyboard strokes using EC_CMD_GET_NEXT_EVENT. But the sandbox driver does not understand this command. We need to reply with -EC_RES_INVALID_COMMAND to force check_for_keys() to fall back to use EC_CMD_MKBP_STATE. Currently the driver prints
** Unknown EC command 0x67in this case. With the patch the message is suppressed.
In a future patch we should upgrade the sandbox driver to provide EC_CMD_GET_NEXT_EVENT support.
Fixes: 690079767803 ("cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT") Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
process_cmd() should always return an appropriate negative enum ec_status in case of an error and not simply -1. But fixing the return values is beyond the scope of this patch.
(Looks to me like -1 is already == -EC_RES_INVALID_COMMAND from include/ec_commands.h definitions, but I'd agree the latter form should be preferred.)
If you look at the complete function, you will find other "return -1;" statements where return codes other than -EC_RES_INVALID_COMMAND make more sense. E.g. after
printf("** Unknown flash region %d\n", req->region);it would be reasonable to return EC_RES_INVALID_PARAM.
Best regards
Heinrich
drivers/misc/cros_ec_sandbox.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
Shall we take Alper's patch to implement this command?
Regards, Simon
Applied to u-boot-dm, thanks!
participants (3)
-
Alper Nebi Yasak -
Heinrich Schuchardt -
Simon Glass