[PATCH v2 02/17] platform/chrome: chardev: Use cros_ec_cmd()

[PATCH v2 02/17] platform/chrome: chardev: Use cros_ec_cmd()

[Prashant Malani]

Update the Chrome OS character device driver to use the newly introduced
cros_ec_cmd() function instead of cros_ec_cmd_xfer_status(), thus
avoiding message buffer set up work which is already done by the new
function.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2:
- Updated to use newer function name and parameter list.

 drivers/platform/chrome/cros_ec_chardev.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index c65e70bc168d8c..f92fe4139c4367 100644
--- a/drivers/platform/chrome/cros_ec_chardev.c
+++ b/drivers/platform/chrome/cros_ec_chardev.c
@@ -57,25 +57,20 @@ static int ec_get_version(struct cros_ec_dev *ec, char *str, int maxlen)
 		"unknown", "read-only", "read-write", "invalid",
 	};
 	struct ec_response_get_version *resp;
-	struct cros_ec_command *msg;
 	int ret;
 
-	msg = kzalloc(sizeof(*msg) + sizeof(*resp), GFP_KERNEL);
-	if (!msg)
+	resp = kzalloc(sizeof(*resp), GFP_KERNEL);
+	if (!resp)
 		return -ENOMEM;
 
-	msg->command = EC_CMD_GET_VERSION + ec->cmd_offset;
-	msg->insize = sizeof(*resp);
-
-	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	ret = cros_ec_cmd(ec->ec_dev, 0, ec->cmd_offset + EC_CMD_GET_VERSION,
+			  NULL, 0, resp, sizeof(*resp), NULL);
 	if (ret < 0) {
 		snprintf(str, maxlen,
-			 "Unknown EC version, returned error: %d\n",
-			 msg->result);
+			 "Unknown EC version, returned error: %d\n", ret);
 		goto exit;
 	}
 
-	resp = (struct ec_response_get_version *)msg->data;
 	if (resp->current_image >= ARRAY_SIZE(current_image_name))
 		resp->current_image = 3; /* invalid */
 
@@ -85,7 +80,7 @@ static int ec_get_version(struct cros_ec_dev *ec, char *str, int maxlen)
 
 	ret = 0;
 exit:
-	kfree(msg);
+	kfree(resp);
 	return ret;
 }
 
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 03/17] platform/chrome: proto: Use cros_ec_cmd()

[Prashant Malani]

Replace the use of cros_ec_cmd_xfer_status() with the new function
cros_ec_cmd().

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2:
- Updated to use new function name and parameter list.

 drivers/platform/chrome/cros_ec_proto.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index b3d5368f596813..aa7ae1f394cc91 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -811,31 +811,20 @@ EXPORT_SYMBOL(cros_ec_get_host_event);
  */
 int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
 {
-	struct cros_ec_command *msg;
 	int ret;
 
 	if (ec->features[0] == -1U && ec->features[1] == -1U) {
 		/* features bitmap not read yet */
-		msg = kzalloc(sizeof(*msg) + sizeof(ec->features), GFP_KERNEL);
-		if (!msg)
-			return -ENOMEM;
-
-		msg->command = EC_CMD_GET_FEATURES + ec->cmd_offset;
-		msg->insize = sizeof(ec->features);
-
-		ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+		ret = cros_ec_cmd(ec->ec_dev, 0,
+				  ec->cmd_offset + EC_CMD_GET_FEATURES, NULL, 0,
+				  ec->features, sizeof(ec->features), NULL);
 		if (ret < 0) {
-			dev_warn(ec->dev, "cannot get EC features: %d/%d\n",
-				 ret, msg->result);
+			dev_warn(ec->dev, "cannot get EC features: %d\n", ret);
 			memset(ec->features, 0, sizeof(ec->features));
-		} else {
-			memcpy(ec->features, msg->data, sizeof(ec->features));
 		}
 
 		dev_dbg(ec->dev, "EC features %08x %08x\n",
 			ec->features[0], ec->features[1]);
-
-		kfree(msg);
 	}
 
 	return ec->features[feature / 32] & EC_FEATURE_MASK_0(feature);
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 04/17] platform/chrome: usbpd_logger: Use cros_ec_cmd()

[Prashant Malani]

Convert the earlier call of cros_ec_cmd_xfer_status() to
cros_ec_cmd() which does the buffer setup and message allocation.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2:
- Updated to use new function name and parameter list.

 drivers/platform/chrome/cros_usbpd_logger.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/chrome/cros_usbpd_logger.c b/drivers/platform/chrome/cros_usbpd_logger.c
index 7de3ea75ef46eb..084b6d4b692128 100644
--- a/drivers/platform/chrome/cros_usbpd_logger.c
+++ b/drivers/platform/chrome/cros_usbpd_logger.c
@@ -61,19 +61,15 @@ static int append_str(char *buf, int pos, const char *fmt, ...)
 static struct ec_response_pd_log *ec_get_log_entry(struct logger_data *logger)
 {
 	struct cros_ec_dev *ec_dev = logger->ec_dev;
-	struct cros_ec_command *msg;
 	int ret;
 
-	msg = (struct cros_ec_command *)logger->ec_buffer;
-
-	msg->command = ec_dev->cmd_offset + EC_CMD_PD_GET_LOG_ENTRY;
-	msg->insize = CROS_USBPD_LOG_RESP_SIZE;
-
-	ret = cros_ec_cmd_xfer_status(ec_dev->ec_dev, msg);
+	ret = cros_ec_cmd(ec_dev->ec_dev, 0,
+			  ec_dev->cmd_offset + EC_CMD_PD_GET_LOG_ENTRY, NULL, 0,
+			  logger->ec_buffer, CROS_USBPD_LOG_RESP_SIZE, NULL);
 	if (ret < 0)
 		return ERR_PTR(ret);
 
-	return (struct ec_response_pd_log *)msg->data;
+	return (struct ec_response_pd_log *)logger->ec_buffer;
 }
 
 static void cros_usbpd_print_log_entry(struct ec_response_pd_log *r,
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 05/17] platform/chrome: sensorhub: Use cros_ec_cmd()

[Prashant Malani]

Update the register() function to use the new cros_ec_cmd() function
instead of cros_ec_cmd_xfer_status().

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2:
- Updated to use new function name and parameter list.

 drivers/platform/chrome/cros_ec_sensorhub.c | 30 +++++++++------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_sensorhub.c b/drivers/platform/chrome/cros_ec_sensorhub.c
index 79fefd3bb0fa61..3ccf98af89453c 100644
--- a/drivers/platform/chrome/cros_ec_sensorhub.c
+++ b/drivers/platform/chrome/cros_ec_sensorhub.c
@@ -53,7 +53,7 @@ static int cros_ec_sensorhub_register(struct device *dev,
 	struct cros_ec_dev *ec = sensorhub->ec;
 	struct ec_params_motion_sense *params;
 	struct ec_response_motion_sense *resp;
-	struct cros_ec_command *msg;
+	void *ec_buf;
 	int ret, i, sensor_num;
 	char *name;
 
@@ -70,27 +70,24 @@ static int cros_ec_sensorhub_register(struct device *dev,
 		return -EINVAL;
 	}
 
-	/* Prepare a message to send INFO command to each sensor. */
-	msg = kzalloc(sizeof(*msg) + max(sizeof(*params), sizeof(*resp)),
-		      GFP_KERNEL);
-	if (!msg)
+	ec_buf = kzalloc(max(sizeof(*params), sizeof(*resp)), GFP_KERNEL);
+	if (!ec_buf)
 		return -ENOMEM;
 
-	msg->version = 1;
-	msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
-	msg->outsize = sizeof(*params);
-	msg->insize = sizeof(*resp);
-	params = (struct ec_params_motion_sense *)msg->data;
-	resp = (struct ec_response_motion_sense *)msg->data;
+	params = ec_buf;
+	resp = ec_buf;
 
 	for (i = 0; i < sensor_num; i++) {
 		params->cmd = MOTIONSENSE_CMD_INFO;
 		params->info.sensor_num = i;
 
-		ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+		ret = cros_ec_cmd(ec->ec_dev, 1,
+				  EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset,
+				  params, sizeof(*params), resp, sizeof(*resp),
+				  NULL);
 		if (ret < 0) {
-			dev_warn(dev, "no info for EC sensor %d : %d/%d\n",
-				 i, ret, msg->result);
+			dev_warn(dev, "no info for EC sensor %d : %d\n",
+				 i, ret);
 			continue;
 		}
 
@@ -140,11 +137,10 @@ static int cros_ec_sensorhub_register(struct device *dev,
 			goto error;
 	}
 
-	kfree(msg);
-	return 0;
+	ret =  0;
 
 error:
-	kfree(msg);
+	kfree(ec_buf);
 	return ret;
 }
 
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 06/17] platform/chrome: debugfs: Use cros_ec_cmd()

[Prashant Malani]

Replace cros_ec_cmd_xfer_status() calls to the new function cros_ec_cmd(),
which is more readable and does the message setup code.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2:
- Updated to use new function name and parameter list.

 drivers/platform/chrome/cros_ec_debugfs.c | 131 +++++++---------------
 1 file changed, 43 insertions(+), 88 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
index ecfada00e6c513..5eb3a341ede343 100644
--- a/drivers/platform/chrome/cros_ec_debugfs.c
+++ b/drivers/platform/chrome/cros_ec_debugfs.c
@@ -31,7 +31,7 @@
  * @ec: EC device this debugfs information belongs to
  * @dir: dentry for debugfs files
  * @log_buffer: circular buffer for console log information
- * @read_msg: preallocated EC command and buffer to read console log
+ * @ec_buf: preallocated EC buffer to send and receive log commands and output.
  * @log_mutex: mutex to protect circular buffer
  * @log_wq: waitqueue for log readers
  * @log_poll_work: recurring task to poll EC for new console log data
@@ -42,7 +42,7 @@ struct cros_ec_debugfs {
 	struct dentry *dir;
 	/* EC log */
 	struct circ_buf log_buffer;
-	struct cros_ec_command *read_msg;
+	void *ec_buf;
 	struct mutex log_mutex;
 	wait_queue_head_t log_wq;
 	struct delayed_work log_poll_work;
@@ -62,18 +62,17 @@ static void cros_ec_console_log_work(struct work_struct *__work)
 			     log_poll_work);
 	struct cros_ec_dev *ec = debug_info->ec;
 	struct circ_buf *cb = &debug_info->log_buffer;
-	struct cros_ec_command snapshot_msg = {
-		.command = EC_CMD_CONSOLE_SNAPSHOT + ec->cmd_offset,
-	};
 
 	struct ec_params_console_read_v1 *read_params =
-		(struct ec_params_console_read_v1 *)debug_info->read_msg->data;
-	uint8_t *ec_buffer = (uint8_t *)debug_info->read_msg->data;
+		(struct ec_params_console_read_v1 *)debug_info->ec_buf;
+	uint8_t *ec_buffer = (uint8_t *)debug_info->ec_buf;
 	int idx;
 	int buf_space;
 	int ret;
 
-	ret = cros_ec_cmd_xfer_status(ec->ec_dev, &snapshot_msg);
+	ret = cros_ec_cmd(ec->ec_dev, 0,
+			  EC_CMD_CONSOLE_SNAPSHOT + ec->cmd_offset, NULL, 0,
+			  NULL, 0, NULL);
 	if (ret < 0)
 		goto resched;
 
@@ -90,8 +89,11 @@ static void cros_ec_console_log_work(struct work_struct *__work)
 
 		memset(read_params, '\0', sizeof(*read_params));
 		read_params->subcmd = CONSOLE_READ_RECENT;
-		ret = cros_ec_cmd_xfer_status(ec->ec_dev,
-					      debug_info->read_msg);
+
+		ret = cros_ec_cmd(ec->ec_dev, 1,
+				  EC_CMD_CONSOLE_READ + ec->cmd_offset,
+				  read_params, sizeof(*read_params), ec_buffer,
+				  ec->ec_dev->max_response, NULL);
 		if (ret < 0)
 			break;
 
@@ -198,44 +200,28 @@ static ssize_t cros_ec_pdinfo_read(struct file *file,
 	char read_buf[EC_USB_PD_MAX_PORTS * 40], *p = read_buf;
 	struct cros_ec_debugfs *debug_info = file->private_data;
 	struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
-	struct {
-		struct cros_ec_command msg;
-		union {
-			struct ec_response_usb_pd_control_v1 resp;
-			struct ec_params_usb_pd_control params;
-		};
-	} __packed ec_buf;
-	struct cros_ec_command *msg;
-	struct ec_response_usb_pd_control_v1 *resp;
-	struct ec_params_usb_pd_control *params;
+	struct ec_response_usb_pd_control_v1 resp = {0};
+	struct ec_params_usb_pd_control params = {0};
 	int i;
 
-	msg = &ec_buf.msg;
-	params = (struct ec_params_usb_pd_control *)msg->data;
-	resp = (struct ec_response_usb_pd_control_v1 *)msg->data;
-
-	msg->command = EC_CMD_USB_PD_CONTROL;
-	msg->version = 1;
-	msg->insize = sizeof(*resp);
-	msg->outsize = sizeof(*params);
-
 	/*
 	 * Read status from all PD ports until failure, typically caused
 	 * by attempting to read status on a port that doesn't exist.
 	 */
 	for (i = 0; i < EC_USB_PD_MAX_PORTS; ++i) {
-		params->port = i;
-		params->role = 0;
-		params->mux = 0;
-		params->swap = 0;
+		params.port = i;
+		params.role = 0;
+		params.mux = 0;
+		params.swap = 0;
 
-		if (cros_ec_cmd_xfer_status(ec_dev, msg) < 0)
+		if (cros_ec_cmd(ec_dev, 1, EC_CMD_USB_PD_CONTROL, &params,
+				sizeof(params), &resp, sizeof(resp), NULL) < 0)
 			break;
 
 		p += scnprintf(p, sizeof(read_buf) + read_buf - p,
 			       "p%d: %s en:%.2x role:%.2x pol:%.2x\n", i,
-			       resp->state, resp->enabled, resp->role,
-			       resp->polarity);
+			       resp.state, resp.enabled, resp.role,
+			       resp.polarity);
 	}
 
 	return simple_read_from_buffer(user_buf, count, ppos,
@@ -247,25 +233,17 @@ static ssize_t cros_ec_uptime_read(struct file *file, char __user *user_buf,
 {
 	struct cros_ec_debugfs *debug_info = file->private_data;
 	struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
-	struct {
-		struct cros_ec_command cmd;
-		struct ec_response_uptime_info resp;
-	} __packed msg = {};
-	struct ec_response_uptime_info *resp;
+	struct ec_response_uptime_info resp = {};
 	char read_buf[32];
 	int ret;
 
-	resp = (struct ec_response_uptime_info *)&msg.resp;
-
-	msg.cmd.command = EC_CMD_GET_UPTIME_INFO;
-	msg.cmd.insize = sizeof(*resp);
-
-	ret = cros_ec_cmd_xfer_status(ec_dev, &msg.cmd);
+	ret = cros_ec_cmd(ec_dev, 0, EC_CMD_GET_UPTIME_INFO, NULL, 0, &resp,
+			  sizeof(resp), NULL);
 	if (ret < 0)
 		return ret;
 
 	ret = scnprintf(read_buf, sizeof(read_buf), "%u\n",
-			resp->time_since_ec_boot_ms);
+			resp.time_since_ec_boot_ms);
 
 	return simple_read_from_buffer(user_buf, count, ppos, read_buf, ret);
 }
@@ -295,29 +273,15 @@ static const struct file_operations cros_ec_uptime_fops = {
 
 static int ec_read_version_supported(struct cros_ec_dev *ec)
 {
-	struct ec_params_get_cmd_versions_v1 *params;
-	struct ec_response_get_cmd_versions *response;
+	struct ec_params_get_cmd_versions_v1 params = {0};
+	struct ec_response_get_cmd_versions resp = {0};
 	int ret;
 
-	struct cros_ec_command *msg;
-
-	msg = kzalloc(sizeof(*msg) + max(sizeof(*params), sizeof(*response)),
-		GFP_KERNEL);
-	if (!msg)
-		return 0;
-
-	msg->command = EC_CMD_GET_CMD_VERSIONS + ec->cmd_offset;
-	msg->outsize = sizeof(*params);
-	msg->insize = sizeof(*response);
-
-	params = (struct ec_params_get_cmd_versions_v1 *)msg->data;
-	params->cmd = EC_CMD_CONSOLE_READ;
-	response = (struct ec_response_get_cmd_versions *)msg->data;
-
-	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg) >= 0 &&
-	      response->version_mask & EC_VER_MASK(1);
-
-	kfree(msg);
+	params.cmd = EC_CMD_CONSOLE_READ;
+	ret = cros_ec_cmd(ec->ec_dev, 0,
+			  EC_CMD_GET_CMD_VERSIONS + ec->cmd_offset, &params,
+			  sizeof(params), &resp, sizeof(resp), NULL);
+	ret = ret >= 0 && resp.version_mask & EC_VER_MASK(1);
 
 	return ret;
 }
@@ -342,17 +306,11 @@ static int cros_ec_create_console_log(struct cros_ec_debugfs *debug_info)
 
 	read_params_size = sizeof(struct ec_params_console_read_v1);
 	read_response_size = ec->ec_dev->max_response;
-	debug_info->read_msg = devm_kzalloc(ec->dev,
-		sizeof(*debug_info->read_msg) +
-			max(read_params_size, read_response_size), GFP_KERNEL);
-	if (!debug_info->read_msg)
+	debug_info->ec_buf = devm_kzalloc(ec->dev,
+		max(read_params_size, read_response_size), GFP_KERNEL);
+	if (!debug_info->ec_buf)
 		return -ENOMEM;
 
-	debug_info->read_msg->version = 1;
-	debug_info->read_msg->command = EC_CMD_CONSOLE_READ + ec->cmd_offset;
-	debug_info->read_msg->outsize = read_params_size;
-	debug_info->read_msg->insize = read_response_size;
-
 	debug_info->log_buffer.buf = buf;
 	debug_info->log_buffer.head = 0;
 	debug_info->log_buffer.tail = 0;
@@ -382,20 +340,17 @@ static int cros_ec_create_panicinfo(struct cros_ec_debugfs *debug_info)
 {
 	struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
 	int ret;
-	struct cros_ec_command *msg;
+	void *buf;
 	int insize;
 
 	insize = ec_dev->max_response;
 
-	msg = devm_kzalloc(debug_info->ec->dev,
-			sizeof(*msg) + insize, GFP_KERNEL);
-	if (!msg)
+	buf = devm_kzalloc(debug_info->ec->dev, insize, GFP_KERNEL);
+	if (!buf)
 		return -ENOMEM;
 
-	msg->command = EC_CMD_GET_PANIC_INFO;
-	msg->insize = insize;
-
-	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
+	ret = cros_ec_cmd(ec_dev, 0, EC_CMD_GET_PANIC_INFO, NULL, 0, buf,
+			  insize, NULL);
 	if (ret < 0) {
 		ret = 0;
 		goto free;
@@ -405,7 +360,7 @@ static int cros_ec_create_panicinfo(struct cros_ec_debugfs *debug_info)
 	if (ret == 0)
 		goto free;
 
-	debug_info->panicinfo_blob.data = msg->data;
+	debug_info->panicinfo_blob.data = buf;
 	debug_info->panicinfo_blob.size = ret;
 
 	debugfs_create_blob("panicinfo", S_IFREG | 0444, debug_info->dir,
@@ -414,7 +369,7 @@ static int cros_ec_create_panicinfo(struct cros_ec_debugfs *debug_info)
 	return 0;
 
 free:
-	devm_kfree(debug_info->ec->dev, msg);
+	devm_kfree(debug_info->ec->dev, buf);
 	return ret;
 }
 
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 07/17] platform/chrome: sysfs: Use cros_ec_cmd()

[Prashant Malani]

Replace cros_ec_cmd_xfer_status() calls to the new function
cros_ec_cmd() which is more readable and does the message setup code.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2:
- Updated to use new function name and parameter list.

 drivers/platform/chrome/cros_ec_sysfs.c | 103 +++++++++---------------
 1 file changed, 39 insertions(+), 64 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index 07dac97ad57c67..107eb81dff1305 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -51,20 +51,13 @@ static ssize_t reboot_store(struct device *dev,
 		{"hibernate",    EC_REBOOT_HIBERNATE, 0},
 		{"at-shutdown",  -1, EC_REBOOT_FLAG_ON_AP_SHUTDOWN},
 	};
-	struct cros_ec_command *msg;
-	struct ec_params_reboot_ec *param;
+	struct ec_params_reboot_ec param = {0};
 	int got_cmd = 0, offset = 0;
 	int i;
 	int ret;
 	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
 
-	msg = kmalloc(sizeof(*msg) + sizeof(*param), GFP_KERNEL);
-	if (!msg)
-		return -ENOMEM;
-
-	param = (struct ec_params_reboot_ec *)msg->data;
-
-	param->flags = 0;
+	param.flags = 0;
 	while (1) {
 		/* Find word to start scanning */
 		while (buf[offset] && isspace(buf[offset]))
@@ -76,9 +69,9 @@ static ssize_t reboot_store(struct device *dev,
 			if (!strncasecmp(words[i].str, buf+offset,
 					 strlen(words[i].str))) {
 				if (words[i].flags) {
-					param->flags |= words[i].flags;
+					param.flags |= words[i].flags;
 				} else {
-					param->cmd = words[i].cmd;
+					param.cmd = words[i].cmd;
 					got_cmd = 1;
 				}
 				break;
@@ -95,15 +88,11 @@ static ssize_t reboot_store(struct device *dev,
 		goto exit;
 	}
 
-	msg->version = 0;
-	msg->command = EC_CMD_REBOOT_EC + ec->cmd_offset;
-	msg->outsize = sizeof(*param);
-	msg->insize = 0;
-	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	ret = cros_ec_cmd(ec->ec_dev, 0, EC_CMD_REBOOT_EC + ec->cmd_offset,
+			  &param, sizeof(param), NULL, 0, NULL);
 	if (ret < 0)
 		count = ret;
 exit:
-	kfree(msg);
 	return count;
 }
 
@@ -115,25 +104,23 @@ static ssize_t version_show(struct device *dev,
 	struct ec_response_get_chip_info *r_chip;
 	struct ec_response_board_version *r_board;
 	struct cros_ec_command *msg;
+	void *ec_buf;
 	int ret;
 	int count = 0;
 	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
 
-	msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
-	if (!msg)
+	ec_buf = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
+	if (!ec_buf)
 		return -ENOMEM;
 
 	/* Get versions. RW may change. */
-	msg->version = 0;
-	msg->command = EC_CMD_GET_VERSION + ec->cmd_offset;
-	msg->insize = sizeof(*r_ver);
-	msg->outsize = 0;
-	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	ret = cros_ec_cmd(ec->ec_dev, 0, EC_CMD_GET_VERSION  + ec->cmd_offset,
+			  NULL, 0, ec_buf, sizeof(*r_ver), NULL);
 	if (ret < 0) {
 		count = ret;
 		goto exit;
 	}
-	r_ver = (struct ec_response_get_version *)msg->data;
+	r_ver = (struct ec_response_get_version *)ec_buf;
 	/* Strings should be null-terminated, but let's be sure. */
 	r_ver->version_string_ro[sizeof(r_ver->version_string_ro) - 1] = '\0';
 	r_ver->version_string_rw[sizeof(r_ver->version_string_rw) - 1] = '\0';
@@ -145,8 +132,10 @@ static ssize_t version_show(struct device *dev,
 			   "Firmware copy: %s\n",
 			   (r_ver->current_image < ARRAY_SIZE(image_names) ?
 			    image_names[r_ver->current_image] : "?"));
+	memset(ec_buf, 0, sizeof(*msg) + EC_HOST_PARAM_SIZE);
 
 	/* Get build info. */
+	msg = (struct cros_ec_command *)ec_buf;
 	msg->command = EC_CMD_GET_BUILD_INFO + ec->cmd_offset;
 	msg->insize = EC_HOST_PARAM_SIZE;
 	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
@@ -205,40 +194,28 @@ static ssize_t version_show(struct device *dev,
 	}
 
 exit:
-	kfree(msg);
+	kfree(ec_buf);
 	return count;
 }
 
 static ssize_t flashinfo_show(struct device *dev,
 			      struct device_attribute *attr, char *buf)
 {
-	struct ec_response_flash_info *resp;
-	struct cros_ec_command *msg;
+	struct ec_response_flash_info resp = {0};
 	int ret;
 	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
 
-	msg = kmalloc(sizeof(*msg) + sizeof(*resp), GFP_KERNEL);
-	if (!msg)
-		return -ENOMEM;
-
-	/* The flash info shouldn't ever change, but ask each time anyway. */
-	msg->version = 0;
-	msg->command = EC_CMD_FLASH_INFO + ec->cmd_offset;
-	msg->insize = sizeof(*resp);
-	msg->outsize = 0;
-	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	ret = cros_ec_cmd(ec->ec_dev, 0, EC_CMD_FLASH_INFO + ec->cmd_offset,
+			  NULL, 0, &resp, sizeof(resp), NULL);
 	if (ret < 0)
 		goto exit;
 
-	resp = (struct ec_response_flash_info *)msg->data;
-
 	ret = scnprintf(buf, PAGE_SIZE,
 			"FlashSize %d\nWriteSize %d\n"
 			"EraseSize %d\nProtectSize %d\n",
-			resp->flash_size, resp->write_block_size,
-			resp->erase_block_size, resp->protect_block_size);
+			resp.flash_size, resp.write_block_size,
+			resp.erase_block_size, resp.protect_block_size);
 exit:
-	kfree(msg);
 	return ret;
 }
 
@@ -249,29 +226,27 @@ static ssize_t kb_wake_angle_show(struct device *dev,
 	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
 	struct ec_response_motion_sense *resp;
 	struct ec_params_motion_sense *param;
-	struct cros_ec_command *msg;
+	void *ec_buf;
 	int ret;
 
-	msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
-	if (!msg)
+	ec_buf = kmalloc(EC_HOST_PARAM_SIZE, GFP_KERNEL);
+	if (!ec_buf)
 		return -ENOMEM;
 
-	param = (struct ec_params_motion_sense *)msg->data;
-	msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
-	msg->version = 2;
+	param = (struct ec_params_motion_sense *)ec_buf;
+	resp = (struct ec_response_motion_sense *)ec_buf;
 	param->cmd = MOTIONSENSE_CMD_KB_WAKE_ANGLE;
 	param->kb_wake_angle.data = EC_MOTION_SENSE_NO_VALUE;
-	msg->outsize = sizeof(*param);
-	msg->insize = sizeof(*resp);
 
-	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	ret = cros_ec_cmd(ec->ec_dev, 2,
+			  EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset, param,
+			  sizeof(*param), resp, sizeof(*resp), NULL);
 	if (ret < 0)
 		goto exit;
 
-	resp = (struct ec_response_motion_sense *)msg->data;
 	ret = scnprintf(buf, PAGE_SIZE, "%d\n", resp->kb_wake_angle.ret);
 exit:
-	kfree(msg);
+	kfree(ec_buf);
 	return ret;
 }
 
@@ -281,7 +256,8 @@ static ssize_t kb_wake_angle_store(struct device *dev,
 {
 	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
 	struct ec_params_motion_sense *param;
-	struct cros_ec_command *msg;
+	struct ec_response_motion_sense *resp;
+	void *ec_buf;
 	u16 angle;
 	int ret;
 
@@ -289,20 +265,19 @@ static ssize_t kb_wake_angle_store(struct device *dev,
 	if (ret)
 		return ret;
 
-	msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
-	if (!msg)
+	ec_buf = kmalloc(EC_HOST_PARAM_SIZE, GFP_KERNEL);
+	if (!ec_buf)
 		return -ENOMEM;
 
-	param = (struct ec_params_motion_sense *)msg->data;
-	msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
-	msg->version = 2;
+	param = (struct ec_params_motion_sense *)ec_buf;
+	resp = (struct ec_response_motion_sense *)ec_buf;
 	param->cmd = MOTIONSENSE_CMD_KB_WAKE_ANGLE;
 	param->kb_wake_angle.data = angle;
-	msg->outsize = sizeof(*param);
-	msg->insize = sizeof(struct ec_response_motion_sense);
 
-	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
-	kfree(msg);
+	ret = cros_ec_cmd(ec->ec_dev, 2,
+			  EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset, param,
+			  sizeof(*param), resp, sizeof(*resp), NULL);
+	kfree(ec_buf);
 	if (ret < 0)
 		return ret;
 	return count;
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 08/17] extcon: cros_ec: Use cros_ec_cmd()

[Prashant Malani]

Replace cros_ec_pd_command() with cros_ec_cmd() which does the same
thing, but is defined in a common location in platform/chrome and
exposed for other modules to use.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2:
- Updated to use new function name and parameter list.

 drivers/extcon/extcon-usbc-cros-ec.c | 61 ++++------------------------
 1 file changed, 8 insertions(+), 53 deletions(-)

diff --git a/drivers/extcon/extcon-usbc-cros-ec.c b/drivers/extcon/extcon-usbc-cros-ec.c
index 5290cc2d19d953..2939cedca04798 100644
--- a/drivers/extcon/extcon-usbc-cros-ec.c
+++ b/drivers/extcon/extcon-usbc-cros-ec.c
@@ -45,49 +45,6 @@ enum usb_data_roles {
 	DR_DEVICE,
 };
 
-/**
- * cros_ec_pd_command() - Send a command to the EC.
- * @info: pointer to struct cros_ec_extcon_info
- * @command: EC command
- * @version: EC command version
- * @outdata: EC command output data
- * @outsize: Size of outdata
- * @indata: EC command input data
- * @insize: Size of indata
- *
- * Return: 0 on success, <0 on failure.
- */
-static int cros_ec_pd_command(struct cros_ec_extcon_info *info,
-			      unsigned int command,
-			      unsigned int version,
-			      void *outdata,
-			      unsigned int outsize,
-			      void *indata,
-			      unsigned int insize)
-{
-	struct cros_ec_command *msg;
-	int ret;
-
-	msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
-	if (!msg)
-		return -ENOMEM;
-
-	msg->version = version;
-	msg->command = command;
-	msg->outsize = outsize;
-	msg->insize = insize;
-
-	if (outsize)
-		memcpy(msg->data, outdata, outsize);
-
-	ret = cros_ec_cmd_xfer_status(info->ec, msg);
-	if (ret >= 0 && insize)
-		memcpy(indata, msg->data, insize);
-
-	kfree(msg);
-	return ret;
-}
-
 /**
  * cros_ec_usb_get_power_type() - Get power type info about PD device attached
  * to given port.
@@ -102,8 +59,8 @@ static int cros_ec_usb_get_power_type(struct cros_ec_extcon_info *info)
 	int ret;
 
 	req.port = info->port_id;
-	ret = cros_ec_pd_command(info, EC_CMD_USB_PD_POWER_INFO, 0,
-				 &req, sizeof(req), &resp, sizeof(resp));
+	ret = cros_ec_cmd(info->ec, 0, EC_CMD_USB_PD_POWER_INFO, &req,
+			  sizeof(req), &resp, sizeof(resp), NULL);
 	if (ret < 0)
 		return ret;
 
@@ -123,9 +80,8 @@ static int cros_ec_usb_get_pd_mux_state(struct cros_ec_extcon_info *info)
 	int ret;
 
 	req.port = info->port_id;
-	ret = cros_ec_pd_command(info, EC_CMD_USB_PD_MUX_INFO, 0,
-				 &req, sizeof(req),
-				 &resp, sizeof(resp));
+	ret = cros_ec_cmd(info->ec, 0, EC_CMD_USB_PD_MUX_INFO, &req,
+			  sizeof(req), &resp, sizeof(resp), NULL);
 	if (ret < 0)
 		return ret;
 
@@ -152,9 +108,8 @@ static int cros_ec_usb_get_role(struct cros_ec_extcon_info *info,
 	pd_control.role = USB_PD_CTRL_ROLE_NO_CHANGE;
 	pd_control.mux = USB_PD_CTRL_MUX_NO_CHANGE;
 	pd_control.swap = USB_PD_CTRL_SWAP_NONE;
-	ret = cros_ec_pd_command(info, EC_CMD_USB_PD_CONTROL, 1,
-				 &pd_control, sizeof(pd_control),
-				 &resp, sizeof(resp));
+	ret = cros_ec_cmd(info->ec, 1, EC_CMD_USB_PD_CONTROL, &pd_control,
+			  sizeof(pd_control), &resp, sizeof(resp), NULL);
 	if (ret < 0)
 		return ret;
 
@@ -177,8 +132,8 @@ static int cros_ec_pd_get_num_ports(struct cros_ec_extcon_info *info)
 	struct ec_response_usb_pd_ports resp;
 	int ret;
 
-	ret = cros_ec_pd_command(info, EC_CMD_USB_PD_PORTS,
-				 0, NULL, 0, &resp, sizeof(resp));
+	ret = cros_ec_cmd(info->ec, 0, EC_CMD_USB_PD_PORTS, NULL, 0, &resp,
+			  sizeof(resp), NULL);
 	if (ret < 0)
 		return ret;
 
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 09/17] hid: google-hammer: Use cros_ec_cmd()

[Prashant Malani]

Replace cros_ec_cmd_xfer_status() with cros_ec_cmd() which does the
message buffer setup and cleanup.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2:
- Updated to use new function name and parameter list.

 drivers/hid/hid-google-hammer.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
index 2aa4ed157aec87..fb0d2a01e2736e 100644
--- a/drivers/hid/hid-google-hammer.c
+++ b/drivers/hid/hid-google-hammer.c
@@ -53,38 +53,25 @@ static bool cbas_parse_base_state(const void *data)
 static int cbas_ec_query_base(struct cros_ec_device *ec_dev, bool get_state,
 				  bool *state)
 {
-	struct ec_params_mkbp_info *params;
-	struct cros_ec_command *msg;
+	struct ec_params_mkbp_info params = {0};
 	int ret;
 
-	msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)),
-		      GFP_KERNEL);
-	if (!msg)
-		return -ENOMEM;
-
-	msg->command = EC_CMD_MKBP_INFO;
-	msg->version = 1;
-	msg->outsize = sizeof(*params);
-	msg->insize = sizeof(u32);
-	params = (struct ec_params_mkbp_info *)msg->data;
-	params->info_type = get_state ?
+	params.info_type = get_state ?
 		EC_MKBP_INFO_CURRENT : EC_MKBP_INFO_SUPPORTED;
-	params->event_type = EC_MKBP_EVENT_SWITCH;
+	params.event_type = EC_MKBP_EVENT_SWITCH;
 
-	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
+	ret = cros_ec_cmd(ec_dev, 1, EC_CMD_MKBP_INFO, &params, sizeof(params),
+			  state, sizeof(u32), NULL);
 	if (ret >= 0) {
 		if (ret != sizeof(u32)) {
 			dev_warn(ec_dev->dev, "wrong result size: %d != %zu\n",
 				 ret, sizeof(u32));
 			ret = -EPROTO;
 		} else {
-			*state = cbas_parse_base_state(msg->data);
 			ret = 0;
 		}
 	}
 
-	kfree(msg);
-
 	return ret;
 }
 
-- 
2.25.0.341.g760bfbb309-goog

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

[Prashant Malani]

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@chromium.org>
---

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;
 
 	if (ret &&
 	    state->resp != (struct ec_response_motion_sense *)state->msg->data)
-- 
2.25.0.341.g760bfbb309-goog

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

[Gwendal Grignou]

On Wed, Feb 5, 2020 at 11:13 AM Prashant Malani <pmalani@chromium.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@chromium.org>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>
> 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;
>
>         if (ret &&
>             state->resp != (struct ec_response_motion_sense *)state->msg->data)
> --
> 2.25.0.341.g760bfbb309-goog
>

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

[Jonathan Cameron]

On Wed,  5 Feb 2020 11:00:13 -0800
Prashant Malani <pmalani@chromium.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@chromium.org>

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.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;
>  
>  	if (ret &&
>  	    state->resp != (struct ec_response_motion_sense *)state->msg->data)

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

[Enric Balletbo i Serra]

Hi Prashant,

On 6/2/20 13:17, Jonathan Cameron wrote:
> On Wed,  5 Feb 2020 11:00:13 -0800
> Prashant Malani <pmalani@chromium.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@chromium.org>
> 
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.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?


>>  	if (ret &&
>>  	    state->resp != (struct ec_response_motion_sense *)state->msg->data)
> 

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

[Prashant Malani]

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@collabora.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@chromium.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@chromium.org>
> >
> > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.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?

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

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

[Gwendal Grignou]

On Thu, Feb 6, 2020 at 10:50 AM Prashant Malani <pmalani@chromium.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@collabora.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@chromium.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@chromium.org>
> > >
> > > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.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)
> > >

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

[Enric Balletbo i Serra]

Hi Gwendal, Prashant et all

On 7/2/20 19:47, Gwendal Grignou wrote:
> On Thu, Feb 6, 2020 at 10:50 AM Prashant Malani <pmalani@chromium.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@collabora.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@chromium.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@chromium.org>
>>>>
>>>> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.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?
> 

Thanks Gwendal for pointing this, it makes totally sense, and I suspect this can
happen on other cases.

Just to make clear, my concern is not about not using the new 'cros_ec_cmd'
here, is about changing 'cros_ec_cmd_xfer_status' for 'cros_ec_cmd_xfer'. Also,
my other concern is how useful is the new 'cros_ec_cmd' replacing what we have
now if cannot replace all current uses.

My points of view are this:

* Actually we have cros_ec_cmd_xfer and cros_ec_cmd_xfer_status, use the second
one is better, in fact, we tried to move all the cros_ec_cmd_xfer to the _status
version in the past because makes the code and error handling cleaner. So I'm
reticent to get back to use cros_ec_cmd_xfer instead of cros_ec_cmd_xfer_status.

* The users of the cros-ec protocol sometimes they mallocing/freeing at runtime,
and sometimes they don't. IMHO *non* mallocing/freeing is usually better, more
efficient and faster. Would be nice to standardize this.

* If we want to introduce a new 'cros_ec_cmd', this should make the code cleaner
and ideally should be the way we tell the users they should use to communicate
with the cros-ec and not open coding constantly. Ideally, should be a
replacement of all current 'cros_ec_cmd_xfer*' versions.

* If 'cros_ec_cmd' *cannot* replace all the cases, it should be clear to the
user in which cases he should use this function and in which cases shouldn't use
this function.

* Finally, what pointed Gwendal, what's the best approach to send commands to
the EC by default, is better use dynamic memory? or is better use the stack? is
it always safe use the stack? is always efficient use allocated memory?

As you can see I have a lot of questions still around, but taking in
consideration that this will be an important change I think that makes sense
spend some time discussing it.

What do you think?

Enric


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

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

[Prashant Malani]

Hi All (trimming most code parts of the thread for the sake of brevity),

Thanks for listing the points Enric, Please see my notes inline:

On Mon, Feb 10, 2020 at 3:03 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Gwendal, Prashant et all
>
> On 7/2/20 19:47, Gwendal Grignou wrote:
> > On Thu, Feb 6, 2020 at 10:50 AM Prashant Malani <pmalani@chromium.org> wrote:
> >>
> >> Hi Enric,
> >>
> >> Thanks for taking a look at the patch. Please see my response inline:
....
> >>>>> @@ -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?
> >
>
> Thanks Gwendal for pointing this, it makes totally sense, and I suspect this can
> happen on other cases.
>
> Just to make clear, my concern is not about not using the new 'cros_ec_cmd'
> here, is about changing 'cros_ec_cmd_xfer_status' for 'cros_ec_cmd_xfer'. Also,
> my other concern is how useful is the new 'cros_ec_cmd' replacing what we have
> now if cannot replace all current uses.
>
> My points of view are this:
>
> * Actually we have cros_ec_cmd_xfer and cros_ec_cmd_xfer_status, use the second
> one is better, in fact, we tried to move all the cros_ec_cmd_xfer to the _status
> version in the past because makes the code and error handling cleaner. So I'm
> reticent to get back to use cros_ec_cmd_xfer instead of cros_ec_cmd_xfer_status.
>
> * The users of the cros-ec protocol sometimes they mallocing/freeing at runtime,
> and sometimes they don't. IMHO *non* mallocing/freeing is usually better, more
> efficient and faster. Would be nice to standardize this.

I think we should look at latency (I am assuming that is one of the
concerns Gwendal was referring to).
We should certainly do more rigorous measurements, but I did a crude
measurement across a devm_kzalloc() used on one of the EC commands
inside platform/chrome for struct EC command:
- Used ktime_get_ns() to record time before and after the devm_kzalloc()
- Used ktime_sub to subtract the "after" and "before" values:

        struct cros_ec_command *msg;
        int ret;
+       ktime_t start, end, diff;

+       start = ktime_get_ns();
        msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
+       end = ktime_get_ns();
        if (!msg)
                return -ENOMEM;

+       diff = ktime_sub(end, start);
+       printk("%s(): TEST: kzalloc took: %lld\n", __func__, ktime_to_ns(diff));

On an i5 1.6 GHz system, across 16 call measurements I got the
following latency values (in ns):
- Count, N:16
- Average: 72.375
- Std. Dev : 28.768
- Max: 143
- Min:  51

Are these values significant for the various call-sites? I think the
driver authors might be able to comment better there (unfortunately I
don't have enough context for each case).
Of course there will be other overhead (memcpy) but I think this is a
good starting point for the discussion.
(My apologies if this measurement method is incorrect/inaccurate.)

>
> * If we want to introduce a new 'cros_ec_cmd', this should make the code cleaner
> and ideally should be the way we tell the users they should use to communicate
> with the cros-ec and not open coding constantly. Ideally, should be a
> replacement of all current 'cros_ec_cmd_xfer*' versions.

As I mentioned previously, I think all calls of cros_ec_cmd_xfer() can
be converted to use cros_ec_cmd() (especially since the new API has a
*result pointer),
but I think it should be staged out a bit more (since cases like iio:
cros_ec driver require non-trivial refactoring which I think is better
in a patch/series).

>
> * If 'cros_ec_cmd' *cannot* replace all the cases, it should be clear to the
> user in which cases he should use this function and in which cases shouldn't use
> this function.

This seems like a good compromise, but my expectation is that if there
is a "fast" and "slow" version of the same functionality, developers
would be inclined to use the "fast" version always?


> * Finally, what pointed Gwendal, what's the best approach to send commands to
> the EC by default, is better use dynamic memory? or is better use the stack? is
> it always safe use the stack? is always efficient use allocated memory?
>
> As you can see I have a lot of questions still around, but taking in
> consideration that this will be an important change I think that makes sense
> spend some time discussing it.
>
> What do you think?
>
> Enric
>
>
> > 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)
> >>>>

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

[Prashant Malani]

Hi All,

Just thought I'd ping this thread since it's been a week since the last
email.

On Mon, Feb 10, 2020 at 12:14:01PM -0800, Prashant Malani wrote:
> Hi All (trimming most code parts of the thread for the sake of brevity),
> 
> Thanks for listing the points Enric, Please see my notes inline:
> 
> On Mon, Feb 10, 2020 at 3:03 AM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
> >
> > Hi Gwendal, Prashant et all
> >
> > On 7/2/20 19:47, Gwendal Grignou wrote:
> > > On Thu, Feb 6, 2020 at 10:50 AM Prashant Malani <pmalani@chromium.org> wrote:
> > >>
> > >> Hi Enric,
> > >>
> > >> Thanks for taking a look at the patch. Please see my response inline:
> ....
> > >>>>> @@ -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?
> > >
> >
> > Thanks Gwendal for pointing this, it makes totally sense, and I suspect this can
> > happen on other cases.
> >
> > Just to make clear, my concern is not about not using the new 'cros_ec_cmd'
> > here, is about changing 'cros_ec_cmd_xfer_status' for 'cros_ec_cmd_xfer'. Also,
> > my other concern is how useful is the new 'cros_ec_cmd' replacing what we have
> > now if cannot replace all current uses.
> >
> > My points of view are this:
> >
> > * Actually we have cros_ec_cmd_xfer and cros_ec_cmd_xfer_status, use the second
> > one is better, in fact, we tried to move all the cros_ec_cmd_xfer to the _status
> > version in the past because makes the code and error handling cleaner. So I'm
> > reticent to get back to use cros_ec_cmd_xfer instead of cros_ec_cmd_xfer_status.
> >
> > * The users of the cros-ec protocol sometimes they mallocing/freeing at runtime,
> > and sometimes they don't. IMHO *non* mallocing/freeing is usually better, more
> > efficient and faster. Would be nice to standardize this.
> 
> I think we should look at latency (I am assuming that is one of the
> concerns Gwendal was referring to).
> We should certainly do more rigorous measurements, but I did a crude
> measurement across a devm_kzalloc() used on one of the EC commands
> inside platform/chrome for struct EC command:
> - Used ktime_get_ns() to record time before and after the devm_kzalloc()
> - Used ktime_sub to subtract the "after" and "before" values:
> 
>         struct cros_ec_command *msg;
>         int ret;
> +       ktime_t start, end, diff;
> 
> +       start = ktime_get_ns();
>         msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
> +       end = ktime_get_ns();
>         if (!msg)
>                 return -ENOMEM;
> 
> +       diff = ktime_sub(end, start);
> +       printk("%s(): TEST: kzalloc took: %lld\n", __func__, ktime_to_ns(diff));
> 
> On an i5 1.6 GHz system, across 16 call measurements I got the
> following latency values (in ns):
> - Count, N:16
> - Average: 72.375
> - Std. Dev : 28.768
> - Max: 143
> - Min:  51
> 
> Are these values significant for the various call-sites? I think the
> driver authors might be able to comment better there (unfortunately I
> don't have enough context for each case).
> Of course there will be other overhead (memcpy) but I think this is a
> good starting point for the discussion.
> (My apologies if this measurement method is incorrect/inaccurate.)

Any thoughts / comments here?

On an overall note, I think keeping cros_ec_cmd_xfer() and cros_ec_cmd()
might be a good starting point.

In this way, we are not introducing any extra function. Also, we can
begin converting the cros_ec_cmd_xfer() use cases (a few call-sites may
need to be investigated from a latency perspective). The
cros_ec_cmd_xfer() conversions are better handled in separate patch
series.

Best regards,

-Prashant
> 
> >
> > * If we want to introduce a new 'cros_ec_cmd', this should make the code cleaner
> > and ideally should be the way we tell the users they should use to communicate
> > with the cros-ec and not open coding constantly. Ideally, should be a
> > replacement of all current 'cros_ec_cmd_xfer*' versions.
> 
> As I mentioned previously, I think all calls of cros_ec_cmd_xfer() can
> be converted to use cros_ec_cmd() (especially since the new API has a
> *result pointer),
> but I think it should be staged out a bit more (since cases like iio:
> cros_ec driver require non-trivial refactoring which I think is better
> in a patch/series).
> 
> >
> > * If 'cros_ec_cmd' *cannot* replace all the cases, it should be clear to the
> > user in which cases he should use this function and in which cases shouldn't use
> > this function.
> 
> This seems like a good compromise, but my expectation is that if there
> is a "fast" and "slow" version of the same functionality, developers
> would be inclined to use the "fast" version always?
> 
> 
> > * Finally, what pointed Gwendal, what's the best approach to send commands to
> > the EC by default, is better use dynamic memory? or is better use the stack? is
> > it always safe use the stack? is always efficient use allocated memory?
> >
> > As you can see I have a lot of questions still around, but taking in
> > consideration that this will be an important change I think that makes sense
> > spend some time discussing it.
> >
> > What do you think?
> >
> > Enric
> >
> >
> > > 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)
> > >>>>

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

[Enric Balletbo i Serra]

Hi Prashant,

Could you base your next series on top of [1]. Also, if you can give your
feedback and test those, would be much appreciated ;-)

BTW, I think you need to fix your sendmail as the series are not threaded and
appear as independent patches in patchwork, which is a bit hard to follow.

Thanks,
 Enric

[1] https://lore.kernel.org/patchwork/cover/1197210/


On 18/2/20 19:30, Prashant Malani wrote:
> Hi All,
> 
> Just thought I'd ping this thread since it's been a week since the last
> email.
> 
> On Mon, Feb 10, 2020 at 12:14:01PM -0800, Prashant Malani wrote:
>> Hi All (trimming most code parts of the thread for the sake of brevity),
>>
>> Thanks for listing the points Enric, Please see my notes inline:
>>
>> On Mon, Feb 10, 2020 at 3:03 AM Enric Balletbo i Serra
>> <enric.balletbo@collabora.com> wrote:
>>>
>>> Hi Gwendal, Prashant et all
>>>
>>> On 7/2/20 19:47, Gwendal Grignou wrote:
>>>> On Thu, Feb 6, 2020 at 10:50 AM Prashant Malani <pmalani@chromium.org> wrote:
>>>>>
>>>>> Hi Enric,
>>>>>
>>>>> Thanks for taking a look at the patch. Please see my response inline:
>> ....
>>>>>>>> @@ -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?
>>>>
>>>
>>> Thanks Gwendal for pointing this, it makes totally sense, and I suspect this can
>>> happen on other cases.
>>>
>>> Just to make clear, my concern is not about not using the new 'cros_ec_cmd'
>>> here, is about changing 'cros_ec_cmd_xfer_status' for 'cros_ec_cmd_xfer'. Also,
>>> my other concern is how useful is the new 'cros_ec_cmd' replacing what we have
>>> now if cannot replace all current uses.
>>>
>>> My points of view are this:
>>>
>>> * Actually we have cros_ec_cmd_xfer and cros_ec_cmd_xfer_status, use the second
>>> one is better, in fact, we tried to move all the cros_ec_cmd_xfer to the _status
>>> version in the past because makes the code and error handling cleaner. So I'm
>>> reticent to get back to use cros_ec_cmd_xfer instead of cros_ec_cmd_xfer_status.
>>>
>>> * The users of the cros-ec protocol sometimes they mallocing/freeing at runtime,
>>> and sometimes they don't. IMHO *non* mallocing/freeing is usually better, more
>>> efficient and faster. Would be nice to standardize this.
>>
>> I think we should look at latency (I am assuming that is one of the
>> concerns Gwendal was referring to).
>> We should certainly do more rigorous measurements, but I did a crude
>> measurement across a devm_kzalloc() used on one of the EC commands
>> inside platform/chrome for struct EC command:
>> - Used ktime_get_ns() to record time before and after the devm_kzalloc()
>> - Used ktime_sub to subtract the "after" and "before" values:
>>
>>         struct cros_ec_command *msg;
>>         int ret;
>> +       ktime_t start, end, diff;
>>
>> +       start = ktime_get_ns();
>>         msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
>> +       end = ktime_get_ns();
>>         if (!msg)
>>                 return -ENOMEM;
>>
>> +       diff = ktime_sub(end, start);
>> +       printk("%s(): TEST: kzalloc took: %lld\n", __func__, ktime_to_ns(diff));
>>
>> On an i5 1.6 GHz system, across 16 call measurements I got the
>> following latency values (in ns):
>> - Count, N:16
>> - Average: 72.375
>> - Std. Dev : 28.768
>> - Max: 143
>> - Min:  51
>>
>> Are these values significant for the various call-sites? I think the
>> driver authors might be able to comment better there (unfortunately I
>> don't have enough context for each case).
>> Of course there will be other overhead (memcpy) but I think this is a
>> good starting point for the discussion.
>> (My apologies if this measurement method is incorrect/inaccurate.)
> 
> Any thoughts / comments here?
> 
> On an overall note, I think keeping cros_ec_cmd_xfer() and cros_ec_cmd()
> might be a good starting point.
> 
> In this way, we are not introducing any extra function. Also, we can
> begin converting the cros_ec_cmd_xfer() use cases (a few call-sites may
> need to be investigated from a latency perspective). The
> cros_ec_cmd_xfer() conversions are better handled in separate patch
> series.
> 
> Best regards,
> 
> -Prashant
>>
>>>
>>> * If we want to introduce a new 'cros_ec_cmd', this should make the code cleaner
>>> and ideally should be the way we tell the users they should use to communicate
>>> with the cros-ec and not open coding constantly. Ideally, should be a
>>> replacement of all current 'cros_ec_cmd_xfer*' versions.
>>
>> As I mentioned previously, I think all calls of cros_ec_cmd_xfer() can
>> be converted to use cros_ec_cmd() (especially since the new API has a
>> *result pointer),
>> but I think it should be staged out a bit more (since cases like iio:
>> cros_ec driver require non-trivial refactoring which I think is better
>> in a patch/series).
>>
>>>
>>> * If 'cros_ec_cmd' *cannot* replace all the cases, it should be clear to the
>>> user in which cases he should use this function and in which cases shouldn't use
>>> this function.
>>
>> This seems like a good compromise, but my expectation is that if there
>> is a "fast" and "slow" version of the same functionality, developers
>> would be inclined to use the "fast" version always?
>>
>>
>>> * Finally, what pointed Gwendal, what's the best approach to send commands to
>>> the EC by default, is better use dynamic memory? or is better use the stack? is
>>> it always safe use the stack? is always efficient use allocated memory?
>>>
>>> As you can see I have a lot of questions still around, but taking in
>>> consideration that this will be an important change I think that makes sense
>>> spend some time discussing it.
>>>
>>> What do you think?
>>>
>>> Enric
>>>
>>>
>>>> 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)
>>>>>>>

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

[Prashant Malani]

On Thu, Feb 20, 2020 at 05:07:00PM +0100, Enric Balletbo i Serra wrote:
> Hi Prashant,
> 
> Could you base your next series on top of [1]. Also, if you can give your
> feedback and test those, would be much appreciated ;-)

Sure Enric, I will attempt to rebase on top of [1] this week. I'll
update you once this is done. Thanks!

-Prashant
> 
> BTW, I think you need to fix your sendmail as the series are not threaded and
> appear as independent patches in patchwork, which is a bit hard to follow.
> 
> Thanks,
>  Enric
> 
> [1] https://lore.kernel.org/patchwork/cover/1197210/
> 
> 
> On 18/2/20 19:30, Prashant Malani wrote:
> > Hi All,
> > 
> > Just thought I'd ping this thread since it's been a week since the last
> > email.
> > 
> > On Mon, Feb 10, 2020 at 12:14:01PM -0800, Prashant Malani wrote:
> >> Hi All (trimming most code parts of the thread for the sake of brevity),
> >>
> >> Thanks for listing the points Enric, Please see my notes inline:
> >>
> >> On Mon, Feb 10, 2020 at 3:03 AM Enric Balletbo i Serra
> >> <enric.balletbo@collabora.com> wrote:
> >>>
> >>> Hi Gwendal, Prashant et all
> >>>
> >>> On 7/2/20 19:47, Gwendal Grignou wrote:
> >>>> On Thu, Feb 6, 2020 at 10:50 AM Prashant Malani <pmalani@chromium.org> wrote:
> >>>>>
> >>>>> Hi Enric,
> >>>>>
> >>>>> Thanks for taking a look at the patch. Please see my response inline:
> >> ....
> >>>>>>>> @@ -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?
> >>>>
> >>>
> >>> Thanks Gwendal for pointing this, it makes totally sense, and I suspect this can
> >>> happen on other cases.
> >>>
> >>> Just to make clear, my concern is not about not using the new 'cros_ec_cmd'
> >>> here, is about changing 'cros_ec_cmd_xfer_status' for 'cros_ec_cmd_xfer'. Also,
> >>> my other concern is how useful is the new 'cros_ec_cmd' replacing what we have
> >>> now if cannot replace all current uses.
> >>>
> >>> My points of view are this:
> >>>
> >>> * Actually we have cros_ec_cmd_xfer and cros_ec_cmd_xfer_status, use the second
> >>> one is better, in fact, we tried to move all the cros_ec_cmd_xfer to the _status
> >>> version in the past because makes the code and error handling cleaner. So I'm
> >>> reticent to get back to use cros_ec_cmd_xfer instead of cros_ec_cmd_xfer_status.
> >>>
> >>> * The users of the cros-ec protocol sometimes they mallocing/freeing at runtime,
> >>> and sometimes they don't. IMHO *non* mallocing/freeing is usually better, more
> >>> efficient and faster. Would be nice to standardize this.
> >>
> >> I think we should look at latency (I am assuming that is one of the
> >> concerns Gwendal was referring to).
> >> We should certainly do more rigorous measurements, but I did a crude
> >> measurement across a devm_kzalloc() used on one of the EC commands
> >> inside platform/chrome for struct EC command:
> >> - Used ktime_get_ns() to record time before and after the devm_kzalloc()
> >> - Used ktime_sub to subtract the "after" and "before" values:
> >>
> >>         struct cros_ec_command *msg;
> >>         int ret;
> >> +       ktime_t start, end, diff;
> >>
> >> +       start = ktime_get_ns();
> >>         msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
> >> +       end = ktime_get_ns();
> >>         if (!msg)
> >>                 return -ENOMEM;
> >>
> >> +       diff = ktime_sub(end, start);
> >> +       printk("%s(): TEST: kzalloc took: %lld\n", __func__, ktime_to_ns(diff));
> >>
> >> On an i5 1.6 GHz system, across 16 call measurements I got the
> >> following latency values (in ns):
> >> - Count, N:16
> >> - Average: 72.375
> >> - Std. Dev : 28.768
> >> - Max: 143
> >> - Min:  51
> >>
> >> Are these values significant for the various call-sites? I think the
> >> driver authors might be able to comment better there (unfortunately I
> >> don't have enough context for each case).
> >> Of course there will be other overhead (memcpy) but I think this is a
> >> good starting point for the discussion.
> >> (My apologies if this measurement method is incorrect/inaccurate.)
> > 
> > Any thoughts / comments here?
> > 
> > On an overall note, I think keeping cros_ec_cmd_xfer() and cros_ec_cmd()
> > might be a good starting point.
> > 
> > In this way, we are not introducing any extra function. Also, we can
> > begin converting the cros_ec_cmd_xfer() use cases (a few call-sites may
> > need to be investigated from a latency perspective). The
> > cros_ec_cmd_xfer() conversions are better handled in separate patch
> > series.
> > 
> > Best regards,
> > 
> > -Prashant
> >>
> >>>
> >>> * If we want to introduce a new 'cros_ec_cmd', this should make the code cleaner
> >>> and ideally should be the way we tell the users they should use to communicate
> >>> with the cros-ec and not open coding constantly. Ideally, should be a
> >>> replacement of all current 'cros_ec_cmd_xfer*' versions.
> >>
> >> As I mentioned previously, I think all calls of cros_ec_cmd_xfer() can
> >> be converted to use cros_ec_cmd() (especially since the new API has a
> >> *result pointer),
> >> but I think it should be staged out a bit more (since cases like iio:
> >> cros_ec driver require non-trivial refactoring which I think is better
> >> in a patch/series).
> >>
> >>>
> >>> * If 'cros_ec_cmd' *cannot* replace all the cases, it should be clear to the
> >>> user in which cases he should use this function and in which cases shouldn't use
> >>> this function.
> >>
> >> This seems like a good compromise, but my expectation is that if there
> >> is a "fast" and "slow" version of the same functionality, developers
> >> would be inclined to use the "fast" version always?
> >>
> >>
> >>> * Finally, what pointed Gwendal, what's the best approach to send commands to
> >>> the EC by default, is better use dynamic memory? or is better use the stack? is
> >>> it always safe use the stack? is always efficient use allocated memory?
> >>>
> >>> As you can see I have a lot of questions still around, but taking in
> >>> consideration that this will be an important change I think that makes sense
> >>> spend some time discussing it.
> >>>
> >>> What do you think?
> >>>
> >>> Enric
> >>>
> >>>
> >>>> 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)
> >>>>>>>

[PATCH v2 11/17] ASoC: cros_ec_codec: Use cros_ec_cmd()

[Prashant Malani]

Replace send_ec_host_command() with cros_ec_cmd() which does the same
thing, but is defined in a common location in platform/chrome and
exposed for other modules to use. This allows us to remove
send_ec_host_command() entirely.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2:
- Updated to use new function name and parameter list.

 sound/soc/codecs/cros_ec_codec.c | 119 +++++++++++--------------------
 1 file changed, 42 insertions(+), 77 deletions(-)

diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
index 6a24f570c5e86f..8516ba5f7624f8 100644
--- a/sound/soc/codecs/cros_ec_codec.c
+++ b/sound/soc/codecs/cros_ec_codec.c
@@ -69,38 +69,6 @@ static int ec_codec_capable(struct cros_ec_codec_priv *priv, uint8_t cap)
 	return priv->ec_capabilities & BIT(cap);
 }
 
-static int send_ec_host_command(struct cros_ec_device *ec_dev, uint32_t cmd,
-				uint8_t *out, size_t outsize,
-				uint8_t *in, size_t insize)
-{
-	int ret;
-	struct cros_ec_command *msg;
-
-	msg = kmalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
-	if (!msg)
-		return -ENOMEM;
-
-	msg->version = 0;
-	msg->command = cmd;
-	msg->outsize = outsize;
-	msg->insize = insize;
-
-	if (outsize)
-		memcpy(msg->data, out, outsize);
-
-	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
-	if (ret < 0)
-		goto error;
-
-	if (insize)
-		memcpy(in, msg->data, insize);
-
-	ret = 0;
-error:
-	kfree(msg);
-	return ret;
-}
-
 static int calculate_sha256(struct cros_ec_codec_priv *priv,
 			    uint8_t *buf, uint32_t size, uint8_t *digest)
 {
@@ -149,18 +117,18 @@ static int dmic_get_gain(struct snd_kcontrol *kcontrol,
 
 	p.cmd = EC_CODEC_DMIC_GET_GAIN_IDX;
 	p.get_gain_idx_param.channel = EC_CODEC_DMIC_CHANNEL_0;
-	ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_DMIC,
-				   (uint8_t *)&p, sizeof(p),
-				   (uint8_t *)&r, sizeof(r));
+	ret = cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_DMIC,
+			  (uint8_t *)&p, sizeof(p), (uint8_t *)&r, sizeof(r),
+			  NULL);
 	if (ret < 0)
 		return ret;
 	ucontrol->value.integer.value[0] = r.gain;
 
 	p.cmd = EC_CODEC_DMIC_GET_GAIN_IDX;
 	p.get_gain_idx_param.channel = EC_CODEC_DMIC_CHANNEL_1;
-	ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_DMIC,
-				   (uint8_t *)&p, sizeof(p),
-				   (uint8_t *)&r, sizeof(r));
+	ret = cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_DMIC,
+			  (uint8_t *)&p, sizeof(p), (uint8_t *)&r, sizeof(r),
+			  NULL);
 	if (ret < 0)
 		return ret;
 	ucontrol->value.integer.value[1] = r.gain;
@@ -191,16 +159,16 @@ static int dmic_put_gain(struct snd_kcontrol *kcontrol,
 	p.cmd = EC_CODEC_DMIC_SET_GAIN_IDX;
 	p.set_gain_idx_param.channel = EC_CODEC_DMIC_CHANNEL_0;
 	p.set_gain_idx_param.gain = left;
-	ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_DMIC,
-				   (uint8_t *)&p, sizeof(p), NULL, 0);
+	ret = cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_DMIC,
+			  (uint8_t *)&p, sizeof(p), NULL, 0, NULL);
 	if (ret < 0)
 		return ret;
 
 	p.cmd = EC_CODEC_DMIC_SET_GAIN_IDX;
 	p.set_gain_idx_param.channel = EC_CODEC_DMIC_CHANNEL_1;
 	p.set_gain_idx_param.gain = right;
-	return send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_DMIC,
-				    (uint8_t *)&p, sizeof(p), NULL, 0);
+	return cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_DMIC,
+			   (uint8_t *)&p, sizeof(p), NULL, 0, NULL);
 }
 
 static const DECLARE_TLV_DB_SCALE(dmic_gain_tlv, 0, 100, 0);
@@ -231,9 +199,9 @@ static int dmic_probe(struct snd_soc_component *component)
 
 	p.cmd = EC_CODEC_DMIC_GET_MAX_GAIN;
 
-	ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_DMIC,
-				   (uint8_t *)&p, sizeof(p),
-				   (uint8_t *)&r, sizeof(r));
+	ret = cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_DMIC,
+			  (uint8_t *)&p, sizeof(p), (uint8_t *)&r, sizeof(r),
+			  NULL);
 	if (ret < 0) {
 		dev_warn(dev, "get_max_gain() unsupported\n");
 		return 0;
@@ -279,8 +247,8 @@ static int i2s_rx_hw_params(struct snd_pcm_substream *substream,
 
 	p.cmd = EC_CODEC_I2S_RX_SET_SAMPLE_DEPTH;
 	p.set_sample_depth_param.depth = depth;
-	ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX,
-				   (uint8_t *)&p, sizeof(p), NULL, 0);
+	ret = cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_I2S_RX,
+			  (uint8_t *)&p, sizeof(p), NULL, 0, NULL);
 	if (ret < 0)
 		return ret;
 
@@ -289,8 +257,8 @@ static int i2s_rx_hw_params(struct snd_pcm_substream *substream,
 
 	p.cmd = EC_CODEC_I2S_RX_SET_BCLK;
 	p.set_bclk_param.bclk = snd_soc_params_to_bclk(params);
-	return send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX,
-				    (uint8_t *)&p, sizeof(p), NULL, 0);
+	return cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_I2S_RX,
+			   (uint8_t *)&p, sizeof(p), NULL, 0, NULL);
 }
 
 static int i2s_rx_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
@@ -333,8 +301,8 @@ static int i2s_rx_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 
 	p.cmd = EC_CODEC_I2S_RX_SET_DAIFMT;
 	p.set_daifmt_param.daifmt = daifmt;
-	return send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX,
-				    (uint8_t *)&p, sizeof(p), NULL, 0);
+	return cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_I2S_RX,
+			   (uint8_t *)&p, sizeof(p), NULL, 0, NULL);
 }
 
 static const struct snd_soc_dai_ops i2s_rx_dai_ops = {
@@ -364,8 +332,8 @@ static int i2s_rx_event(struct snd_soc_dapm_widget *w,
 		return 0;
 	}
 
-	return send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX,
-				    (uint8_t *)&p, sizeof(p), NULL, 0);
+	return cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_I2S_RX,
+			   (uint8_t *)&p, sizeof(p), NULL, 0, NULL);
 }
 
 static struct snd_soc_dapm_widget i2s_rx_dapm_widgets[] = {
@@ -415,9 +383,8 @@ static void *wov_map_shm(struct cros_ec_codec_priv *priv,
 
 	p.cmd = EC_CODEC_GET_SHM_ADDR;
 	p.get_shm_addr_param.shm_id = shm_id;
-	if (send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC,
-				 (uint8_t *)&p, sizeof(p),
-				 (uint8_t *)&r, sizeof(r)) < 0) {
+	if (cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC, (uint8_t *)&p,
+			sizeof(p), (uint8_t *)&r, sizeof(r), NULL) < 0) {
 		dev_err(priv->dev, "failed to EC_CODEC_GET_SHM_ADDR\n");
 		return NULL;
 	}
@@ -453,9 +420,8 @@ static void *wov_map_shm(struct cros_ec_codec_priv *priv,
 		p.set_shm_addr_param.phys_addr = priv->ap_shm_last_alloc;
 		p.set_shm_addr_param.len = req;
 		p.set_shm_addr_param.shm_id = shm_id;
-		if (send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC,
-					 (uint8_t *)&p, sizeof(p),
-					 NULL, 0) < 0) {
+		if (cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC,
+				(uint8_t *)&p, sizeof(p), NULL, 0, NULL) < 0) {
 			dev_err(priv->dev, "failed to EC_CODEC_SET_SHM_ADDR\n");
 			return NULL;
 		}
@@ -571,9 +537,9 @@ static int wov_read_audio_shm(struct cros_ec_codec_priv *priv)
 	int ret;
 
 	p.cmd = EC_CODEC_WOV_READ_AUDIO_SHM;
-	ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_WOV,
-				   (uint8_t *)&p, sizeof(p),
-				   (uint8_t *)&r, sizeof(r));
+	ret = cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_WOV,
+			  (uint8_t *)&p, sizeof(p), (uint8_t *)&r, sizeof(r),
+			  NULL);
 	if (ret) {
 		dev_err(priv->dev, "failed to EC_CODEC_WOV_READ_AUDIO_SHM\n");
 		return ret;
@@ -596,9 +562,9 @@ static int wov_read_audio(struct cros_ec_codec_priv *priv)
 
 	while (remain >= 0) {
 		p.cmd = EC_CODEC_WOV_READ_AUDIO;
-		ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_WOV,
-					   (uint8_t *)&p, sizeof(p),
-					   (uint8_t *)&r, sizeof(r));
+		ret = cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_WOV,
+				  (uint8_t *)&p, sizeof(p), (uint8_t *)&r,
+				  sizeof(r), NULL);
 		if (ret) {
 			dev_err(priv->dev,
 				"failed to EC_CODEC_WOV_READ_AUDIO\n");
@@ -669,8 +635,8 @@ static int wov_enable_put(struct snd_kcontrol *kcontrol,
 		else
 			p.cmd = EC_CODEC_WOV_DISABLE;
 
-		ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_WOV,
-					   (uint8_t *)&p, sizeof(p), NULL, 0);
+		ret = cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_WOV,
+				  (uint8_t *)&p, sizeof(p), NULL, 0, NULL);
 		if (ret) {
 			dev_err(priv->dev, "failed to %s wov\n",
 				enabled ? "enable" : "disable");
@@ -716,8 +682,8 @@ static int wov_set_lang_shm(struct cros_ec_codec_priv *priv,
 	p.cmd = EC_CODEC_WOV_SET_LANG_SHM;
 	memcpy(pp->hash, digest, SHA256_DIGEST_SIZE);
 	pp->total_len = size;
-	ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_WOV,
-				   (uint8_t *)&p, sizeof(p), NULL, 0);
+	ret = cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_WOV,
+			  (uint8_t *)&p, sizeof(p), NULL, 0, NULL);
 	if (ret) {
 		dev_err(priv->dev, "failed to EC_CODEC_WOV_SET_LANG_SHM\n");
 		return ret;
@@ -743,8 +709,8 @@ static int wov_set_lang(struct cros_ec_codec_priv *priv,
 		pp->offset = i;
 		memcpy(pp->buf, buf + i, req);
 		pp->len = req;
-		ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_WOV,
-					   (uint8_t *)&p, sizeof(p), NULL, 0);
+		ret = cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_WOV,
+				  (uint8_t *)&p, sizeof(p), NULL, 0, NULL);
 		if (ret) {
 			dev_err(priv->dev, "failed to EC_CODEC_WOV_SET_LANG\n");
 			return ret;
@@ -782,9 +748,9 @@ static int wov_hotword_model_put(struct snd_kcontrol *kcontrol,
 		goto leave;
 
 	p.cmd = EC_CODEC_WOV_GET_LANG;
-	ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_WOV,
-				   (uint8_t *)&p, sizeof(p),
-				   (uint8_t *)&r, sizeof(r));
+	ret = cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC_WOV,
+			  (uint8_t *)&p, sizeof(p), (uint8_t *)&r, sizeof(r),
+			  NULL);
 	if (ret)
 		goto leave;
 
@@ -1020,9 +986,8 @@ static int cros_ec_codec_platform_probe(struct platform_device *pdev)
 	atomic_set(&priv->dmic_probed, 0);
 
 	p.cmd = EC_CODEC_GET_CAPABILITIES;
-	ret = send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC,
-				   (uint8_t *)&p, sizeof(p),
-				   (uint8_t *)&r, sizeof(r));
+	ret = cros_ec_cmd(priv->ec_device, 0, EC_CMD_EC_CODEC, (uint8_t *)&p,
+			  sizeof(p), (uint8_t *)&r, sizeof(r), NULL);
 	if (ret) {
 		dev_err(dev, "failed to EC_CODEC_GET_CAPABILITIES\n");
 		return ret;
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH v2 11/17] ASoC: cros_ec_codec: Use cros_ec_cmd()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]

On Wed, Feb 05, 2020 at 11:00:16AM -0800, Prashant Malani wrote:
> Replace send_ec_host_command() with cros_ec_cmd() which does the same
> thing, but is defined in a common location in platform/chrome and
> exposed for other modules to use. This allows us to remove
> send_ec_host_command() entirely.

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 12/17] power: supply: cros: Use cros_ec_cmd()

[Prashant Malani]

Replace cros_usbpd_charger_ec_command() with cros_ec_cmd() which does
the same thing, but is defined in a common location in platform/chrome
and exposed for other modules to use. This allows us to remove
cros_usbpd_charger_ec_command() entirely.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2:
- Updated to use new function name and parameter list.

 drivers/power/supply/cros_usbpd-charger.c | 58 ++++-------------------
 1 file changed, 10 insertions(+), 48 deletions(-)

diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
index 30c3d37511c9e1..4631d96fda2ca1 100644
--- a/drivers/power/supply/cros_usbpd-charger.c
+++ b/drivers/power/supply/cros_usbpd-charger.c
@@ -91,46 +91,13 @@ static bool cros_usbpd_charger_port_is_dedicated(struct port_data *port)
 	return port->port_number >= port->charger->num_usbpd_ports;
 }
 
-static int cros_usbpd_charger_ec_command(struct charger_data *charger,
-					 unsigned int version,
-					 unsigned int command,
-					 void *outdata,
-					 unsigned int outsize,
-					 void *indata,
-					 unsigned int insize)
-{
-	struct cros_ec_dev *ec_dev = charger->ec_dev;
-	struct cros_ec_command *msg;
-	int ret;
-
-	msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
-	if (!msg)
-		return -ENOMEM;
-
-	msg->version = version;
-	msg->command = ec_dev->cmd_offset + command;
-	msg->outsize = outsize;
-	msg->insize = insize;
-
-	if (outsize)
-		memcpy(msg->data, outdata, outsize);
-
-	ret = cros_ec_cmd_xfer_status(charger->ec_device, msg);
-	if (ret >= 0 && insize)
-		memcpy(indata, msg->data, insize);
-
-	kfree(msg);
-	return ret;
-}
-
 static int cros_usbpd_charger_get_num_ports(struct charger_data *charger)
 {
 	struct ec_response_charge_port_count resp;
 	int ret;
 
-	ret = cros_usbpd_charger_ec_command(charger, 0,
-					    EC_CMD_CHARGE_PORT_COUNT,
-					    NULL, 0, &resp, sizeof(resp));
+	ret = cros_ec_cmd(charger->ec_device, 0, EC_CMD_CHARGE_PORT_COUNT, NULL,
+			  0, &resp, sizeof(resp), NULL);
 	if (ret < 0)
 		return ret;
 
@@ -142,8 +109,8 @@ static int cros_usbpd_charger_get_usbpd_num_ports(struct charger_data *charger)
 	struct ec_response_usb_pd_ports resp;
 	int ret;
 
-	ret = cros_usbpd_charger_ec_command(charger, 0, EC_CMD_USB_PD_PORTS,
-					    NULL, 0, &resp, sizeof(resp));
+	ret = cros_ec_cmd(charger->ec_device, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
+			  &resp, sizeof(resp), NULL);
 	if (ret < 0)
 		return ret;
 
@@ -159,10 +126,8 @@ static int cros_usbpd_charger_get_discovery_info(struct port_data *port)
 
 	req.port = port->port_number;
 
-	ret = cros_usbpd_charger_ec_command(charger, 0,
-					    EC_CMD_USB_PD_DISCOVERY,
-					    &req, sizeof(req),
-					    &resp, sizeof(resp));
+	ret = cros_ec_cmd(charger->ec_device, 0, EC_CMD_USB_PD_DISCOVERY, &req,
+			  sizeof(req), &resp, sizeof(resp), NULL);
 	if (ret < 0) {
 		dev_err(charger->dev,
 			"Unable to query discovery info (err:0x%x)\n", ret);
@@ -189,10 +154,8 @@ static int cros_usbpd_charger_get_power_info(struct port_data *port)
 	int ret;
 
 	req.port = port->port_number;
-	ret = cros_usbpd_charger_ec_command(charger, 0,
-					    EC_CMD_USB_PD_POWER_INFO,
-					    &req, sizeof(req),
-					    &resp, sizeof(resp));
+	ret = cros_ec_cmd(charger->ec_device, 0, EC_CMD_USB_PD_POWER_INFO, &req,
+			  sizeof(req), &resp, sizeof(resp), NULL);
 	if (ret < 0) {
 		dev_err(dev, "Unable to query PD power info (err:0x%x)\n", ret);
 		return ret;
@@ -334,9 +297,8 @@ static int cros_usbpd_charger_set_ext_power_limit(struct charger_data *charger,
 	req.current_lim = current_lim;
 	req.voltage_lim = voltage_lim;
 
-	ret = cros_usbpd_charger_ec_command(charger, 0,
-					    EC_CMD_EXTERNAL_POWER_LIMIT,
-					    &req, sizeof(req), NULL, 0);
+	ret = cros_ec_cmd(charger->ec_device, 0, EC_CMD_EXTERNAL_POWER_LIMIT,
+			  &req, sizeof(req), NULL, 0, NULL);
 	if (ret < 0)
 		dev_err(charger->dev,
 			"Unable to set the 'External Power Limit': %d\n", ret);
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH v2 12/17] power: supply: cros: Use cros_ec_cmd()

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4508 bytes --]

Hi,

On Wed, Feb 05, 2020 at 11:00:18AM -0800, Prashant Malani wrote:
> Replace cros_usbpd_charger_ec_command() with cros_ec_cmd() which does
> the same thing, but is defined in a common location in platform/chrome
> and exposed for other modules to use. This allows us to remove
> cros_usbpd_charger_ec_command() entirely.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---

Acked-by: Sebastian Reichel <sre@kernel.org>

-- Sebastian

> Changes in v2:
> - Updated to use new function name and parameter list.
> 
>  drivers/power/supply/cros_usbpd-charger.c | 58 ++++-------------------
>  1 file changed, 10 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
> index 30c3d37511c9e1..4631d96fda2ca1 100644
> --- a/drivers/power/supply/cros_usbpd-charger.c
> +++ b/drivers/power/supply/cros_usbpd-charger.c
> @@ -91,46 +91,13 @@ static bool cros_usbpd_charger_port_is_dedicated(struct port_data *port)
>  	return port->port_number >= port->charger->num_usbpd_ports;
>  }
>  
> -static int cros_usbpd_charger_ec_command(struct charger_data *charger,
> -					 unsigned int version,
> -					 unsigned int command,
> -					 void *outdata,
> -					 unsigned int outsize,
> -					 void *indata,
> -					 unsigned int insize)
> -{
> -	struct cros_ec_dev *ec_dev = charger->ec_dev;
> -	struct cros_ec_command *msg;
> -	int ret;
> -
> -	msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
> -	if (!msg)
> -		return -ENOMEM;
> -
> -	msg->version = version;
> -	msg->command = ec_dev->cmd_offset + command;
> -	msg->outsize = outsize;
> -	msg->insize = insize;
> -
> -	if (outsize)
> -		memcpy(msg->data, outdata, outsize);
> -
> -	ret = cros_ec_cmd_xfer_status(charger->ec_device, msg);
> -	if (ret >= 0 && insize)
> -		memcpy(indata, msg->data, insize);
> -
> -	kfree(msg);
> -	return ret;
> -}
> -
>  static int cros_usbpd_charger_get_num_ports(struct charger_data *charger)
>  {
>  	struct ec_response_charge_port_count resp;
>  	int ret;
>  
> -	ret = cros_usbpd_charger_ec_command(charger, 0,
> -					    EC_CMD_CHARGE_PORT_COUNT,
> -					    NULL, 0, &resp, sizeof(resp));
> +	ret = cros_ec_cmd(charger->ec_device, 0, EC_CMD_CHARGE_PORT_COUNT, NULL,
> +			  0, &resp, sizeof(resp), NULL);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -142,8 +109,8 @@ static int cros_usbpd_charger_get_usbpd_num_ports(struct charger_data *charger)
>  	struct ec_response_usb_pd_ports resp;
>  	int ret;
>  
> -	ret = cros_usbpd_charger_ec_command(charger, 0, EC_CMD_USB_PD_PORTS,
> -					    NULL, 0, &resp, sizeof(resp));
> +	ret = cros_ec_cmd(charger->ec_device, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
> +			  &resp, sizeof(resp), NULL);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -159,10 +126,8 @@ static int cros_usbpd_charger_get_discovery_info(struct port_data *port)
>  
>  	req.port = port->port_number;
>  
> -	ret = cros_usbpd_charger_ec_command(charger, 0,
> -					    EC_CMD_USB_PD_DISCOVERY,
> -					    &req, sizeof(req),
> -					    &resp, sizeof(resp));
> +	ret = cros_ec_cmd(charger->ec_device, 0, EC_CMD_USB_PD_DISCOVERY, &req,
> +			  sizeof(req), &resp, sizeof(resp), NULL);
>  	if (ret < 0) {
>  		dev_err(charger->dev,
>  			"Unable to query discovery info (err:0x%x)\n", ret);
> @@ -189,10 +154,8 @@ static int cros_usbpd_charger_get_power_info(struct port_data *port)
>  	int ret;
>  
>  	req.port = port->port_number;
> -	ret = cros_usbpd_charger_ec_command(charger, 0,
> -					    EC_CMD_USB_PD_POWER_INFO,
> -					    &req, sizeof(req),
> -					    &resp, sizeof(resp));
> +	ret = cros_ec_cmd(charger->ec_device, 0, EC_CMD_USB_PD_POWER_INFO, &req,
> +			  sizeof(req), &resp, sizeof(resp), NULL);
>  	if (ret < 0) {
>  		dev_err(dev, "Unable to query PD power info (err:0x%x)\n", ret);
>  		return ret;
> @@ -334,9 +297,8 @@ static int cros_usbpd_charger_set_ext_power_limit(struct charger_data *charger,
>  	req.current_lim = current_lim;
>  	req.voltage_lim = voltage_lim;
>  
> -	ret = cros_usbpd_charger_ec_command(charger, 0,
> -					    EC_CMD_EXTERNAL_POWER_LIMIT,
> -					    &req, sizeof(req), NULL, 0);
> +	ret = cros_ec_cmd(charger->ec_device, 0, EC_CMD_EXTERNAL_POWER_LIMIT,
> +			  &req, sizeof(req), NULL, 0, NULL);
>  	if (ret < 0)
>  		dev_err(charger->dev,
>  			"Unable to set the 'External Power Limit': %d\n", ret);
> -- 
> 2.25.0.341.g760bfbb309-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 13/17] pwm: cros-ec: Remove cros_ec_cmd_xfer_status()

[Prashant Malani]

Convert existing usages of cros_ec_cmd_xfer_status() to cros_ec_cmd(),
which accomplishes the same thing but also does the EC message struct
setup, is defined in platform/chrome and is accessible by other
modules.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2:
- Updated to use new function name and parameter list.
- Use C99 element setting for struct initialization.

 drivers/pwm/pwm-cros-ec.c | 59 +++++++++++----------------------------
 1 file changed, 16 insertions(+), 43 deletions(-)

diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index 89497448d21775..57a1cab4cfacad 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -32,59 +32,32 @@ static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
 
 static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty)
 {
-	struct {
-		struct cros_ec_command msg;
-		struct ec_params_pwm_set_duty params;
-	} __packed buf;
-	struct ec_params_pwm_set_duty *params = &buf.params;
-	struct cros_ec_command *msg = &buf.msg;
-
-	memset(&buf, 0, sizeof(buf));
-
-	msg->version = 0;
-	msg->command = EC_CMD_PWM_SET_DUTY;
-	msg->insize = 0;
-	msg->outsize = sizeof(*params);
-
-	params->duty = duty;
-	params->pwm_type = EC_PWM_TYPE_GENERIC;
-	params->index = index;
-
-	return cros_ec_cmd_xfer_status(ec, msg);
+	struct ec_params_pwm_set_duty params = {
+		.duty = duty,
+		.pwm_type = EC_PWM_TYPE_GENERIC,
+		.index = index,
+	};
+
+	return cros_ec_cmd(ec, 0, EC_CMD_PWM_SET_DUTY, &params,
+			   sizeof(params), NULL, 0, NULL);
 }
 
 static int __cros_ec_pwm_get_duty(struct cros_ec_device *ec, u8 index,
 				  u32 *result)
 {
-	struct {
-		struct cros_ec_command msg;
-		union {
-			struct ec_params_pwm_get_duty params;
-			struct ec_response_pwm_get_duty resp;
-		};
-	} __packed buf;
-	struct ec_params_pwm_get_duty *params = &buf.params;
-	struct ec_response_pwm_get_duty *resp = &buf.resp;
-	struct cros_ec_command *msg = &buf.msg;
+	struct ec_params_pwm_get_duty params = {
+		.pwm_type = EC_PWM_TYPE_GENERIC,
+		.index = index,
+	};
+	struct ec_response_pwm_get_duty resp = {0};
 	int ret;
 
-	memset(&buf, 0, sizeof(buf));
-
-	msg->version = 0;
-	msg->command = EC_CMD_PWM_GET_DUTY;
-	msg->insize = sizeof(*resp);
-	msg->outsize = sizeof(*params);
-
-	params->pwm_type = EC_PWM_TYPE_GENERIC;
-	params->index = index;
-
-	ret = cros_ec_cmd_xfer_status(ec, msg);
-	if (result)
-		*result = msg->result;
+	ret = cros_ec_cmd(ec, 0, EC_CMD_PWM_GET_DUTY, &params, sizeof(params),
+			  &resp, sizeof(resp), result);
 	if (ret < 0)
 		return ret;
 
-	return resp->duty;
+	return resp.duty;
 }
 
 static int cros_ec_pwm_get_duty(struct cros_ec_device *ec, u8 index)
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 14/17] rtc: cros-ec: Use cros_ec_cmd()

[Prashant Malani]

Replace cros_ec_cmd_xfer_status() with cros_ec_cmd() which does the
message buffer setup and cleanup.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2:
- Updated to use new function name and parameter list.

 drivers/rtc/rtc-cros-ec.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/rtc/rtc-cros-ec.c b/drivers/rtc/rtc-cros-ec.c
index f7343c289cab73..6886100ad0b8b7 100644
--- a/drivers/rtc/rtc-cros-ec.c
+++ b/drivers/rtc/rtc-cros-ec.c
@@ -33,16 +33,11 @@ static int cros_ec_rtc_get(struct cros_ec_device *cros_ec, u32 command,
 			   u32 *response)
 {
 	int ret;
-	struct {
-		struct cros_ec_command msg;
-		struct ec_response_rtc data;
-	} __packed msg;
 
-	memset(&msg, 0, sizeof(msg));
-	msg.msg.command = command;
-	msg.msg.insize = sizeof(msg.data);
+	struct ec_response_rtc data = {0};
 
-	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+	ret = cros_ec_cmd(cros_ec, 0, command, NULL, 0, &data, sizeof(data),
+			  NULL);
 	if (ret < 0) {
 		dev_err(cros_ec->dev,
 			"error getting %s from EC: %d\n",
@@ -51,7 +46,7 @@ static int cros_ec_rtc_get(struct cros_ec_device *cros_ec, u32 command,
 		return ret;
 	}
 
-	*response = msg.data.time;
+	*response = data.time;
 
 	return 0;
 }
@@ -60,17 +55,11 @@ static int cros_ec_rtc_set(struct cros_ec_device *cros_ec, u32 command,
 			   u32 param)
 {
 	int ret = 0;
-	struct {
-		struct cros_ec_command msg;
-		struct ec_response_rtc data;
-	} __packed msg;
+	struct ec_response_rtc  data;
 
-	memset(&msg, 0, sizeof(msg));
-	msg.msg.command = command;
-	msg.msg.outsize = sizeof(msg.data);
-	msg.data.time = param;
-
-	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+	data.time = param;
+	ret = cros_ec_cmd(cros_ec, 0, command, &data, sizeof(data), NULL, 0,
+			  NULL);
 	if (ret < 0) {
 		dev_err(cros_ec->dev, "error setting %s on EC: %d\n",
 			command == EC_CMD_RTC_SET_VALUE ? "time" : "alarm",
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH v2 14/17] rtc: cros-ec: Use cros_ec_cmd()

[Alexandre Belloni]

On 05/02/2020 11:00:22-0800, Prashant Malani wrote:
> Replace cros_ec_cmd_xfer_status() with cros_ec_cmd() which does the
> message buffer setup and cleanup.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
> 
> Changes in v2:
> - Updated to use new function name and parameter list.
> 
>  drivers/rtc/rtc-cros-ec.c | 27 ++++++++-------------------
>  1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-cros-ec.c b/drivers/rtc/rtc-cros-ec.c
> index f7343c289cab73..6886100ad0b8b7 100644
> --- a/drivers/rtc/rtc-cros-ec.c
> +++ b/drivers/rtc/rtc-cros-ec.c
> @@ -33,16 +33,11 @@ static int cros_ec_rtc_get(struct cros_ec_device *cros_ec, u32 command,
>  			   u32 *response)
>  {
>  	int ret;
> -	struct {
> -		struct cros_ec_command msg;
> -		struct ec_response_rtc data;
> -	} __packed msg;
>  
> -	memset(&msg, 0, sizeof(msg));
> -	msg.msg.command = command;
> -	msg.msg.insize = sizeof(msg.data);
> +	struct ec_response_rtc data = {0};
>  
> -	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> +	ret = cros_ec_cmd(cros_ec, 0, command, NULL, 0, &data, sizeof(data),
> +			  NULL);
>  	if (ret < 0) {
>  		dev_err(cros_ec->dev,
>  			"error getting %s from EC: %d\n",
> @@ -51,7 +46,7 @@ static int cros_ec_rtc_get(struct cros_ec_device *cros_ec, u32 command,
>  		return ret;
>  	}
>  
> -	*response = msg.data.time;
> +	*response = data.time;
>  
>  	return 0;
>  }
> @@ -60,17 +55,11 @@ static int cros_ec_rtc_set(struct cros_ec_device *cros_ec, u32 command,
>  			   u32 param)
>  {
>  	int ret = 0;
> -	struct {
> -		struct cros_ec_command msg;
> -		struct ec_response_rtc data;
> -	} __packed msg;
> +	struct ec_response_rtc  data;
>  
> -	memset(&msg, 0, sizeof(msg));
> -	msg.msg.command = command;
> -	msg.msg.outsize = sizeof(msg.data);
> -	msg.data.time = param;
> -
> -	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> +	data.time = param;
> +	ret = cros_ec_cmd(cros_ec, 0, command, &data, sizeof(data), NULL, 0,
> +			  NULL);
>  	if (ret < 0) {
>  		dev_err(cros_ec->dev, "error setting %s on EC: %d\n",
>  			command == EC_CMD_RTC_SET_VALUE ? "time" : "alarm",
> -- 
> 2.25.0.341.g760bfbb309-goog
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 15/17] media: cros-ec-cec: Use cros_ec_cmd()

[Prashant Malani]

Replace cros_ec_cmd_xfer_status() with cros_ec_cmd() which does the
message buffer setup and cleanup, but is located in platform/chrome
and used by other drivers.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
Reported-by: kbuild test robot <lkp@intel.com>
---

Changes in v2:
- Updated to use new function name and parameter list.
- Used C99 element setting to initialize struct.

 .../media/platform/cros-ec-cec/cros-ec-cec.c  | 45 +++++++------------
 1 file changed, 16 insertions(+), 29 deletions(-)

diff --git a/drivers/media/platform/cros-ec-cec/cros-ec-cec.c b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
index 0e7e2772f08f96..a462af1c9ae04b 100644
--- a/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
+++ b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
@@ -93,18 +93,14 @@ static int cros_ec_cec_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
 {
 	struct cros_ec_cec *cros_ec_cec = adap->priv;
 	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
-	struct {
-		struct cros_ec_command msg;
-		struct ec_params_cec_set data;
-	} __packed msg = {};
+	struct ec_params_cec_set data = {
+		.cmd = CEC_CMD_LOGICAL_ADDRESS,
+		.val = logical_addr,
+	};
 	int ret;
 
-	msg.msg.command = EC_CMD_CEC_SET;
-	msg.msg.outsize = sizeof(msg.data);
-	msg.data.cmd = CEC_CMD_LOGICAL_ADDRESS;
-	msg.data.val = logical_addr;
-
-	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+	ret = cros_ec_cmd(cros_ec, 0, EC_CMD_CEC_SET, &data, sizeof(data),
+			  NULL, 0, NULL);
 	if (ret < 0) {
 		dev_err(cros_ec->dev,
 			"error setting CEC logical address on EC: %d\n", ret);
@@ -119,17 +115,12 @@ static int cros_ec_cec_transmit(struct cec_adapter *adap, u8 attempts,
 {
 	struct cros_ec_cec *cros_ec_cec = adap->priv;
 	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
-	struct {
-		struct cros_ec_command msg;
-		struct ec_params_cec_write data;
-	} __packed msg = {};
+	struct ec_params_cec_write data = {};
 	int ret;
 
-	msg.msg.command = EC_CMD_CEC_WRITE_MSG;
-	msg.msg.outsize = cec_msg->len;
-	memcpy(msg.data.msg, cec_msg->msg, cec_msg->len);
-
-	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+	memcpy(data.msg, cec_msg->msg, cec_msg->len);
+	ret = cros_ec_cmd(cros_ec, 0, EC_CMD_CEC_WRITE_MSG, &data,
+			  sizeof(cec_msg->len), NULL, 0, NULL);
 	if (ret < 0) {
 		dev_err(cros_ec->dev,
 			"error writing CEC msg on EC: %d\n", ret);
@@ -143,18 +134,14 @@ static int cros_ec_cec_adap_enable(struct cec_adapter *adap, bool enable)
 {
 	struct cros_ec_cec *cros_ec_cec = adap->priv;
 	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
-	struct {
-		struct cros_ec_command msg;
-		struct ec_params_cec_set data;
-	} __packed msg = {};
+	struct ec_params_cec_set data = {
+		.cmd = CEC_CMD_ENABLE,
+		.val = enable,
+	};
 	int ret;
 
-	msg.msg.command = EC_CMD_CEC_SET;
-	msg.msg.outsize = sizeof(msg.data);
-	msg.data.cmd = CEC_CMD_ENABLE;
-	msg.data.val = enable;
-
-	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+	ret = cros_ec_cmd(cros_ec, 0, EC_CMD_CEC_SET, &data, sizeof(data),
+			  NULL, 0, NULL);
 	if (ret < 0) {
 		dev_err(cros_ec->dev,
 			"error %sabling CEC on EC: %d\n",
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH v2 15/17] media: cros-ec-cec: Use cros_ec_cmd()

[Hans Verkuil]

Hi Prashant,

On 05/02/2020 20:00, Prashant Malani wrote:
> Replace cros_ec_cmd_xfer_status() with cros_ec_cmd() which does the
> message buffer setup and cleanup, but is located in platform/chrome
> and used by other drivers.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> Reported-by: kbuild test robot <lkp@intel.com>

This source has moved to drivers/media/cec/platform/cros-ec/cros-ec-cec.c.

Can you rebase/repost?

Regards,

	Hans

> ---
> 
> Changes in v2:
> - Updated to use new function name and parameter list.
> - Used C99 element setting to initialize struct.
> 
>  .../media/platform/cros-ec-cec/cros-ec-cec.c  | 45 +++++++------------
>  1 file changed, 16 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/platform/cros-ec-cec/cros-ec-cec.c b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
> index 0e7e2772f08f96..a462af1c9ae04b 100644
> --- a/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
> +++ b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
> @@ -93,18 +93,14 @@ static int cros_ec_cec_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
>  {
>  	struct cros_ec_cec *cros_ec_cec = adap->priv;
>  	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> -	struct {
> -		struct cros_ec_command msg;
> -		struct ec_params_cec_set data;
> -	} __packed msg = {};
> +	struct ec_params_cec_set data = {
> +		.cmd = CEC_CMD_LOGICAL_ADDRESS,
> +		.val = logical_addr,
> +	};
>  	int ret;
>  
> -	msg.msg.command = EC_CMD_CEC_SET;
> -	msg.msg.outsize = sizeof(msg.data);
> -	msg.data.cmd = CEC_CMD_LOGICAL_ADDRESS;
> -	msg.data.val = logical_addr;
> -
> -	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> +	ret = cros_ec_cmd(cros_ec, 0, EC_CMD_CEC_SET, &data, sizeof(data),
> +			  NULL, 0, NULL);
>  	if (ret < 0) {
>  		dev_err(cros_ec->dev,
>  			"error setting CEC logical address on EC: %d\n", ret);
> @@ -119,17 +115,12 @@ static int cros_ec_cec_transmit(struct cec_adapter *adap, u8 attempts,
>  {
>  	struct cros_ec_cec *cros_ec_cec = adap->priv;
>  	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> -	struct {
> -		struct cros_ec_command msg;
> -		struct ec_params_cec_write data;
> -	} __packed msg = {};
> +	struct ec_params_cec_write data = {};
>  	int ret;
>  
> -	msg.msg.command = EC_CMD_CEC_WRITE_MSG;
> -	msg.msg.outsize = cec_msg->len;
> -	memcpy(msg.data.msg, cec_msg->msg, cec_msg->len);
> -
> -	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> +	memcpy(data.msg, cec_msg->msg, cec_msg->len);
> +	ret = cros_ec_cmd(cros_ec, 0, EC_CMD_CEC_WRITE_MSG, &data,
> +			  sizeof(cec_msg->len), NULL, 0, NULL);
>  	if (ret < 0) {
>  		dev_err(cros_ec->dev,
>  			"error writing CEC msg on EC: %d\n", ret);
> @@ -143,18 +134,14 @@ static int cros_ec_cec_adap_enable(struct cec_adapter *adap, bool enable)
>  {
>  	struct cros_ec_cec *cros_ec_cec = adap->priv;
>  	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> -	struct {
> -		struct cros_ec_command msg;
> -		struct ec_params_cec_set data;
> -	} __packed msg = {};
> +	struct ec_params_cec_set data = {
> +		.cmd = CEC_CMD_ENABLE,
> +		.val = enable,
> +	};
>  	int ret;
>  
> -	msg.msg.command = EC_CMD_CEC_SET;
> -	msg.msg.outsize = sizeof(msg.data);
> -	msg.data.cmd = CEC_CMD_ENABLE;
> -	msg.data.val = enable;
> -
> -	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> +	ret = cros_ec_cmd(cros_ec, 0, EC_CMD_CEC_SET, &data, sizeof(data),
> +			  NULL, 0, NULL);
>  	if (ret < 0) {
>  		dev_err(cros_ec->dev,
>  			"error %sabling CEC on EC: %d\n",
>