Re: [PATCH v4 1/3] platform/chrome: Add struct ec_response_get_next_event_v3

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Daisuke,

On Fri, Jun 07, 2024 at 10:02:56AM -0700, Daisuke Nojiri wrote:
> Add struct ec_response_get_next_event_v3 to upgrade
> EC_CMD_GET_NEXT_EVENT to version 3.
> 
> Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org>
> ---
>  .../linux/platform_data/cros_ec_commands.h    | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> index 070e49c5381e..fff191a8d413 100644
> --- a/include/linux/platform_data/cros_ec_commands.h
> +++ b/include/linux/platform_data/cros_ec_commands.h
> @@ -3527,6 +3527,34 @@ union __ec_align_offset1 ec_response_get_next_data_v1 {
>  };
>  BUILD_ASSERT(sizeof(union ec_response_get_next_data_v1) == 16);
>  
> +union __ec_align_offset1 ec_response_get_next_data_v3 {
> +	uint8_t key_matrix[18];
> +
> +	/* Unaligned */
> +	uint32_t host_event;
> +	uint64_t host_event64;
> +
> +	struct __ec_todo_unpacked {
> +		/* For aligning the fifo_info */
> +		uint8_t reserved[3];
> +		struct ec_response_motion_sense_fifo_info info;
> +	} sensor_fifo;
> +
> +	uint32_t buttons;
> +
> +	uint32_t switches;
> +
> +	uint32_t fp_events;
> +
> +	uint32_t sysrq;
> +
> +	/* CEC events from enum mkbp_cec_event */
> +	uint32_t cec_events;
> +
> +	uint8_t cec_message[16];
> +};
> +BUILD_ASSERT(sizeof(union ec_response_get_next_data_v3) == 18);
> +
>  struct ec_response_get_next_event {
>  	uint8_t event_type;
>  	/* Followed by event data if any */
> @@ -3539,6 +3567,12 @@ struct ec_response_get_next_event_v1 {
>  	union ec_response_get_next_data_v1 data;
>  } __ec_align1;
>  
> +struct ec_response_get_next_event_v3 {
> +	uint8_t event_type;
> +	/* Followed by event data if any */
> +	union ec_response_get_next_data_v3 data;
> +} __ec_align1;
> +

It is not really obvious that ec_response_get_next_event and
ec_response_get_next_event_v3 are layout compatible. I would simply
extend the union and add key_matrix_v3 field instead of defining
a brand new union.

And I would drop ec_response_get_next_event_v1 and added missing fields
to the original union as well...

Thanks.

-- 
Dmitry