Date:   Fri, 7 Feb 2020 10:47:15 -0800
From:   Gwendal Grignou <gwendal@...omium.org>
To:     Prashant Malani <pmalani@...omium.org>
Cc:     Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        Lee Jones <lee.jones@...aro.org>,
        Fabien Lahoudere <fabien.lahoudere@...labora.com>,
        "open list:IIO SUBSYSTEM AND DRIVERS" <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v2 10/17] iio: cros_ec: Use cros_ec_cmd()

On Thu, Feb 6, 2020 at 10:50 AM Prashant Malani <pmalani@...omium.org> wrote:
>
> 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@...labora.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@...omium.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@...omium.org>
> > >
> > > Acked-by: Jonathan Cameron <Jonathan.Cameron@...wei.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?
When the EC does not support sensor fifo,
cros_ec_motion_send_host_cmd() is on the data path. For instance, it
is called 2 times every 10ms by chrome to calculate the lid angle. I
would be reluctant to call malloc. Given it is well encapsulated into
the sensor stack. Does it make sense to call cros_ec_cmd_xfer
directly?

Gwendal.
>
> 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)
> > >

Powered by blists - more mailing lists