Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200205190028.183069-1-pmalani@chromium.org> <20200205190028.183069-11-pmalani@chromium.org>
 <20200206121753.7b809631@archlinux> <671a55aa-1e5e-4e21-4a62-55db4dee368a@collabora.com>
In-Reply-To: <671a55aa-1e5e-4e21-4a62-55db4dee368a@collabora.com>
From:   Prashant Malani <pmalani@chromium.org>
Date:   Thu, 6 Feb 2020 10:49:53 -0800
Message-ID: <CACeCKad4zp9O7WAPu5S1rmUDwkzWLjk_1i7YtPvXUG=nDvkYAA@mail.gmail.com>
Subject: Re: [PATCH v2 10/17] iio: cros_ec: Use cros_ec_cmd()
To:     Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Hartmut Knaack <knaack.h@gmx.de>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        Gwendal Grignou <gwendal@chromium.org>,
        Lee Jones <lee.jones@linaro.org>,
        Fabien Lahoudere <fabien.lahoudere@collabora.com>,
        "open list:IIO SUBSYSTEM AND DRIVERS" <linux-iio@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@collabora.com> wrote:
>
> Hi Prashant,
>
> On 6/2/20 13:17, Jonathan Cameron wrote:
> > On Wed,  5 Feb 2020 11:00:13 -0800
> > Prashant Malani <pmalani@chromium.org> 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@chromium.org>
> >
> > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> >> ---
> >>
> >> 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,
> >> +                      &params, 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)
> >