* [PATCH 00/17] platform/chrome: Replace cros_ec_cmd_xfer_status @ 2020-01-30 20:30 Prashant Malani 2020-01-30 20:30 ` [PATCH 10/17] iio: cros_ec: Use cros_ec_send_cmd_msg() Prashant Malani 0 siblings, 1 reply; 4+ messages in thread From: Prashant Malani @ 2020-01-30 20:30 UTC (permalink / raw) To: linux-kernel Cc: Prashant Malani, Akshu Agrawal, Alexandre Belloni, moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..., Benjamin Tissoires, Dmitry Torokhov, Enric Balletbo i Serra, Evan Green, Fabien Lahoudere, Guenter Roeck, Gwendal Grignou, Hans Verkuil, Hartmut Knaack, Jonathan Cameron, Lars-Peter Clausen, Lee Jones, open list:I2C SUBSYSTEM HOST DRIVERS, open list:IIO SUBSYSTEM AND DRIVERS, open list:HID CORE LAYER, open list:MEDIA INPUT INFRASTRUCTURE (V4L/DVB), open list:POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS, open list:PWM SUBSYSTEM, open list:REAL TIME CLOCK (RTC) SUBSYSTEM, Mauro Carvalho Chehab, Neil Armstrong, Nick Vaccaro, Peter Meerwald-Stadler, Raul E Rangel, Uwe Kleine-König, Wolfram Sang 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 10/17] iio: cros_ec: Use cros_ec_send_cmd_msg() 2020-01-30 20:30 [PATCH 00/17] platform/chrome: Replace cros_ec_cmd_xfer_status Prashant Malani @ 2020-01-30 20:30 ` Prashant Malani 2020-02-02 9:43 ` Jonathan Cameron 0 siblings, 1 reply; 4+ messages in thread From: Prashant Malani @ 2020-01-30 20:30 UTC (permalink / raw) To: linux-kernel Cc: Prashant Malani, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Benson Leung, Enric Balletbo i Serra, Guenter Roeck, Gwendal Grignou, Fabien Lahoudere, Nick Vaccaro, open list:IIO SUBSYSTEM AND DRIVERS 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, + ¶ms, 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 10/17] iio: cros_ec: Use cros_ec_send_cmd_msg() 2020-01-30 20:30 ` [PATCH 10/17] iio: cros_ec: Use cros_ec_send_cmd_msg() Prashant Malani @ 2020-02-02 9:43 ` Jonathan Cameron 2020-02-03 18:31 ` Prashant Malani 0 siblings, 1 reply; 4+ messages in thread From: Jonathan Cameron @ 2020-02-02 9:43 UTC (permalink / raw) To: Prashant Malani Cc: linux-kernel, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Benson Leung, Enric Balletbo i Serra, Guenter Roeck, Gwendal Grignou, Fabien Lahoudere, Nick Vaccaro, open list:IIO SUBSYSTEM AND DRIVERS 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, > + ¶ms, 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; > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 10/17] iio: cros_ec: Use cros_ec_send_cmd_msg() 2020-02-02 9:43 ` Jonathan Cameron @ 2020-02-03 18:31 ` Prashant Malani 0 siblings, 0 replies; 4+ messages in thread From: Prashant Malani @ 2020-02-03 18:31 UTC (permalink / raw) To: Jonathan Cameron Cc: Linux Kernel Mailing List, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Benson Leung, Enric Balletbo i Serra, Guenter Roeck, Gwendal Grignou, Fabien Lahoudere, Nick Vaccaro, open list:IIO SUBSYSTEM AND DRIVERS 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, > > + ¶ms, 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; > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-02-03 18:31 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-30 20:30 [PATCH 00/17] platform/chrome: Replace cros_ec_cmd_xfer_status Prashant Malani 2020-01-30 20:30 ` [PATCH 10/17] iio: cros_ec: Use cros_ec_send_cmd_msg() Prashant Malani 2020-02-02 9:43 ` Jonathan Cameron 2020-02-03 18:31 ` Prashant Malani
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).