Date:   Mon, 3 Feb 2020 10:31:40 -0800
From:   Prashant Malani <pmalani@...omium.org>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     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>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Guenter Roeck <groeck@...omium.org>,
        Gwendal Grignou <gwendal@...omium.org>,
        Fabien Lahoudere <fabien.lahoudere@...labora.com>,
        Nick Vaccaro <nvaccaro@...omium.org>,
        "open list:IIO SUBSYSTEM AND DRIVERS" <linux-iio@...r.kernel.org>
Subject: Re: [PATCH 10/17] iio: cros_ec: Use cros_ec_send_cmd_msg()

Hi Jonathan, Thanks for the comments. Please see responses inline.

On Sun, Feb 2, 2020 at 1:43 AM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Thu, 30 Jan 2020 12:30:54 -0800
> Prashant Malani <pmalani@...omium.org> 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@...omium.org>
>
> 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.

Sorry about that. I'll be sure to add maintainers to Patch 1 in the
next versions.
I tried to follow : https://lwn.net/Articles/585782/ , but I think the
script might not be suited to this use case.
>
> 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;
> };
Noted.
>
> > +     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.
Noted. Perhaps we can use cros_ec_cmd_xfer() instead of
cros_ec_cmd_xfer_status(). That I think will
keep readability the same and remove the need for comments.
>
> >
> >       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;
> >
>

Powered by blists - more mailing lists