[PATCH 00/17] platform/chrome: Replace cros_ec_cmd_xfer_status

[PATCH 00/17] platform/chrome: Replace cros_ec_cmd_xfer_status

[Prashant Malani]

Many callers of cros_ec_cmd_xfer_status() use similar setup and cleanup
code, including setting up the cros_ec_command message struct and
copying the received buffer.

This series introduces a replacement function cros_ec_send_cmd_msg() that
performs this setup and teardown, and then updates all call sites that
used xfer_status() to use the new function instead.

The final patch in the series drops cros_ec_cmd_xfer_status() altogether.

Prashant Malani (17):
  platform/chrome: Add EC command msg wrapper
  platform/chrome: chardev: Use send_cmd_msg()
  platform/chrome: proto: Use send_cmd_msg
  platform/chrome: usbpd_logger: Use cmd_send_msg()
  platform/chrome: sensorhub: Use send_cmd_msg()
  platform/chrome: debugfs: Use send_cmd_msg()
  platform/chrome: sysfs: Use send_cmd_msg()
  extcon: cros_ec: Use cros_ec_send_cmd_msg()
  hid: google-hammer: Use cros_ec_send_cmd_msg()
  iio: cros_ec: Use cros_ec_send_cmd_msg()
  ASoC: cros_ec_codec: Use cros_ec_send_cmd_msg()
  power: supply: cros: Use cros_ec_send_cmd_msg()
  pwm: cros-ec: Remove cros_ec_cmd_xfer_status()
  rtc: cros-ec: Use cros_ec_send_cmd_msg()
  media: cros-ec-cec: Use cros_ec_send_cmd_msg()
  i2c: cros-ec-tunnel: Use cros_ec_send_cmd_msg()
  platform/chrome: Drop cros_ec_cmd_xfer_status()

 drivers/extcon/extcon-usbc-cros-ec.c          |  62 ++------
 drivers/hid/hid-google-hammer.c               |  23 +--
 drivers/i2c/busses/i2c-cros-ec-tunnel.c       |  23 ++-
 .../cros_ec_sensors/cros_ec_sensors_core.c    |  43 +++---
 .../media/platform/cros-ec-cec/cros-ec-cec.c  |  39 ++---
 drivers/platform/chrome/cros_ec_chardev.c     |  18 +--
 drivers/platform/chrome/cros_ec_debugfs.c     | 135 ++++++------------
 drivers/platform/chrome/cros_ec_proto.c       |  75 ++++++----
 drivers/platform/chrome/cros_ec_sensorhub.c   |  29 ++--
 drivers/platform/chrome/cros_ec_sysfs.c       | 106 ++++++--------
 drivers/platform/chrome/cros_usbpd_logger.c   |  13 +-
 drivers/power/supply/cros_usbpd-charger.c     |  63 ++------
 drivers/pwm/pwm-cros-ec.c                     |  27 ++--
 drivers/rtc/rtc-cros-ec.c                     |  27 ++--
 include/linux/platform_data/cros_ec_proto.h   |   6 +-
 sound/soc/codecs/cros_ec_codec.c              |  71 +++------
 16 files changed, 276 insertions(+), 484 deletions(-)

-- 
2.25.0.341.g760bfbb309-goog

[PATCH 10/17] iio: cros_ec: Use cros_ec_send_cmd_msg()

[Prashant Malani]

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>
---
 .../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;
+	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;
 
 	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;
 
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH 10/17] iio: cros_ec: Use cros_ec_send_cmd_msg()

[Jonathan Cameron]

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.

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;
};

> +	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.

>  
>  	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;
>  

Re: [PATCH 10/17] iio: cros_ec: Use cros_ec_send_cmd_msg()

[Prashant Malani]

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;
> >
>