Hi Enric,
Thanks for taking a look at the patch. Please see my response inline:
On Thu, Feb 6, 2020 at 5:45 AM Enric Balletbo i Serra
<enric.balletbo@xxxxxxxxxxxxx> wrote:
>
> Hi Prashant,
>
> On 6/2/20 13:17, Jonathan Cameron wrote:
> > On Wed, 5 Feb 2020 11:00:13 -0800
> > Prashant Malani <pmalani@xxxxxxxxxxxx> wrote:
> >
> >> Replace cros_ec_cmd_xfer_status() with cros_ec_cmd()
> >> which does the message buffer setup and cleanup.
> >>
> >> For one other usage, replace the cros_ec_cmd_xfer_status() call with a
> >> call to cros_ec_cmd_xfer(), in preparation for the removal of the former
> >> function.
> >>
> >> Signed-off-by: Prashant Malani <pmalani@xxxxxxxxxxxx>
> >
> > Acked-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >
> >> ---
> >>
> >> Changes in v2:
> >> - Updated to use new function name and parameter list.
> >> - Used C99 element setting to initialize param struct.
> >> - For second usage, replaced cros_ec_cmd_xfer_status() with
> >> cros_ec_cmd_xfer() which is functionally similar.
> >>
> >> .../cros_ec_sensors/cros_ec_sensors_core.c | 25 +++++++------------
> >> 1 file changed, 9 insertions(+), 16 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 d3a3626c7cd834..94e22e7d927631 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
> >> @@ -30,24 +30,15 @@ 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}
> >> + struct ec_params_get_cmd_versions params = {
> >> + .cmd = cmd,
> >> };
> >> + struct ec_response_get_cmd_versions resp = {0};
> >>
> >> - ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> >> + ret = cros_ec_cmd(ec_dev, 0, EC_CMD_GET_CMD_VERSIONS + cmd_offset,
> >> + ¶ms, sizeof(params), &resp, sizeof(resp), NULL);
> >> if (ret >= 0)
> >> - *mask = buf.resp.version_mask;
> >> + *mask = resp.version_mask;
> >> return ret;
> >> }
> >>
> >> @@ -171,9 +162,11 @@ int cros_ec_motion_send_host_cmd(struct cros_ec_sensors_core_state *state,
> >>
> >> memcpy(state->msg->data, &state->param, sizeof(state->param));
> >>
> >> - ret = cros_ec_cmd_xfer_status(state->ec, state->msg);
> >> + ret = cros_ec_cmd_xfer(state->ec, state->msg);
> >> if (ret < 0)
> >> return ret;
> >> + else if (state->msg->result != EC_RES_SUCCESS)
> >> + return -EPROTO;
> >>
>
> There is no way to use the new cros_ec_cmd here?
I think it is doable. From looking at the code I felt the factors we
need to be careful about are:
- The function cros_ec_motion_send_host_cmd() is called from a few
other files, each of which set up the struct cros_ec_command
differently (reference:
https://elixir.bootlin.com/linux/latest/ident/cros_ec_motion_send_host_cmd)
- It is not clear to me how readability will be affected by making the
change to cros_ec_cmd().
Due to the above two factors, but primarily because I wanted to avoid
making such an involved large change in this 17 patch series, I
reasoned it would be better to make the transition to cros_ec_cmd()
for these files in a separate patch/series.
My plan after this patch series is to work on this driver(perhaps we
can eliminate cros_ec_motion_send_host_cmd() itself?), and then remove
cros_ec_cmd_xfer() usage.
WDYT?
Best regards,
>
>
> >> if (ret &&
> >> state->resp != (struct ec_response_motion_sense *)state->msg->data)
> >
