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

Dmitry Torokhov
2024-06-11T18:11:04Z

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