lkml.org 
[lkml]   [2020]   [Feb]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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@kernel.org> wrote:
>
> On Thu, 30 Jan 2020 12:30:54 -0800
> Prashant Malani <pmalani@chromium.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@chromium.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;
> >
>

\
 
 \ /
  Last update: 2020-02-03 19:33    [W:0.085 / U:1.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site