Received-SPF: pass (google.com: domain of linux-kernel+bounces-210405-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 11 Jun 2024 11:11:00 -0700
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Daisuke Nojiri <dnojiri@chromium.org>
Cc: Benson Leung <bleung@chromium.org>, Tzung-Bi Shih <tzungbi@kernel.org>,
	Guenter Roeck <groeck@chromium.org>, Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Reka Norman <rekanorman@chromium.org>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Pavan Holla <pholla@chromium.org>,
	Gwendal Grignou <gwendal@chromium.org>,
	Lukasz Majczak <lma@chromium.org>,
	Ching-Kang Yen <chingkang@chromium.org>,
	Stephen Boyd <swboyd@chromium.org>,
	Prashant Malani <pmalani@chromium.org>,
	chrome-platform@lists.linux.dev, linux-kernel@vger.kernel.org,
	linux-input@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v4 1/3] platform/chrome: Add struct
 ec_response_get_next_event_v3
Message-ID: <ZmiTNEjxE-ZCotNu@google.com>
References: <cover.1717779167.git.dnojiri@chromium.org>
 <979b1fdaa5b3d3165e53f5429470c42581394d14.1717779167.git.dnojiri@chromium.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <979b1fdaa5b3d3165e53f5429470c42581394d14.1717779167.git.dnojiri@chromium.org>

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