Re: [PATCH 1/1] cros_ec: Handling EC_CMD_GET_NEXT_EVENT
Hi,
On Mon, 9 Nov 2020 at 14:25, Heinrich Schuchardt 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
> >> ---
> >> 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!
Re: [PATCH 1/1] cros_ec: Handling EC_CMD_GET_NEXT_EVENT
Hi,
On Mon, 9 Nov 2020 at 14:25, Heinrich Schuchardt 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
> >> ---
> >> 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
Re: [PATCH 1/1] cros_ec: Handling EC_CMD_GET_NEXT_EVENT
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
>> ---
>> 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
>>
Re: [PATCH 1/1] cros_ec: Handling EC_CMD_GET_NEXT_EVENT
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
> ---
> 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
>
