| Message ID | 20201109203425.25546-1-xypron.glpk@gmx.de |
|---|---|
| State | Accepted |
| Commit | a3e458524c15710e4ac9ce1556a5f0898084d09a |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [1/1] cros_ec: Handling EC_CMD_GET_NEXT_EVENT | expand |
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 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. (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 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. > > (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 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. > > > > (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 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. > > > > (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!
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; default: printf(" ** Unknown EC command %#02x\n", req_hdr->command); return -1;
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(+) -- 2.28.0