Re: [PATCH 10/17] iio: cros_ec: Use cros_ec_send_cmd_msg()

[Jonathan Cameron]

On Thu, 30 Jan 2020 12:30:54 -0800
Prashant Malani <pmalani@xxxxxxxxxxxx> wrote:

> Replace cros_ec_cmd_xfer_status() with cros_ec_send_cmd_msg()
> which does the message buffer setup and cleanup.
> 
> Signed-off-by: Prashant Malani <pmalani@xxxxxxxxxxxx>

In a series like this, make sure that patch 1 with the actual code being
pulled out is sent to everyone.  Looking at what we have here, this
doesn't seem to fit well at all for one case, and I'm can't say the
other case shows much advantage either.

Jonathan

> ---
>  .../cros_ec_sensors/cros_ec_sensors_core.c    | 43 +++++++++----------
>  1 file changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index 81a7f692de2f37..f92032e97a84d7 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -31,24 +31,16 @@ static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev,
>  					     u16 cmd_offset, u16 cmd, u32 *mask)
>  {
>  	int ret;
> -	struct {
> -		struct cros_ec_command msg;
> -		union {
> -			struct ec_params_get_cmd_versions params;
> -			struct ec_response_get_cmd_versions resp;
> -		};
> -	} __packed buf = {
> -		.msg = {
> -			.command = EC_CMD_GET_CMD_VERSIONS + cmd_offset,
> -			.insize = sizeof(struct ec_response_get_cmd_versions),
> -			.outsize = sizeof(struct ec_params_get_cmd_versions)
> -			},
> -		.params = {.cmd = cmd}
> -	};
> -
> -	ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> +	struct ec_params_get_cmd_versions params = {0};
> +	struct ec_response_get_cmd_versions resp = {0};
> +
> +	params.cmd = cmd;
Use c99 element setting to set this directly rather than zeroing
explicitly then setting the element.

Something like.

struct ec_params_get_cmde_versions params = {
	.cmd = cmd;
};

> +	ret = cros_ec_send_cmd_msg(ec_dev, 0,
> +				   EC_CMD_GET_CMD_VERSIONS + cmd_offset,
> +				   &params, sizeof(params),
> +				   &resp, sizeof(resp));
>  	if (ret >= 0)
> -		*mask = buf.resp.version_mask;
> +		*mask = resp.version_mask;
>  	return ret;
>  }
>  
> @@ -164,15 +156,22 @@ int cros_ec_motion_send_host_cmd(struct cros_ec_sensors_core_state *state,
>  				 u16 opt_length)
>  {
>  	int ret;
> +	struct cros_ec_command *msg = state->msg;

With this change the code becomes less readable and needs a comment to explain
why it is doing something odd.   Either you need to figure out how to
make this fit properly such that the comment is not needed, or leave
the code alone.

>  
>  	if (opt_length)
> -		state->msg->insize = min(opt_length, state->ec->max_response);
> +		msg->insize = min(opt_length, state->ec->max_response);
>  	else
> -		state->msg->insize = state->ec->max_response;
> +		msg->insize = state->ec->max_response;
>  
> -	memcpy(state->msg->data, &state->param, sizeof(state->param));
> -
> -	ret = cros_ec_cmd_xfer_status(state->ec, state->msg);
> +	/*
> +	 * In order to not disrupt the usage of struct cros_ec_command *msg,
> +	 * which is defined higher up in the call stack, we pass in its
> +	 * members to cros_ec_send_cmd_msg, instead of removing it at all
> +	 * calling locations.
> +	 */
> +	ret = cros_ec_send_cmd_msg(state->ec, msg->version, msg->command,
> +				   &state->param, sizeof(state->param),
> +				   msg->data, msg->insize);
>  	if (ret < 0)
>  		return ret;
>