[PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status()

[PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status()

[Enric Balletbo i Serra]

Dear all,

The purpose of this series is get rid of the remaining places where the
cros_ec_cmd_xfer() function is used in favour of the
cros_ec_cmd_xfer_status() helper. This allows us to make the
cros_ec_cmd_xfer() function private and only expose to the users a
single way to send commands to the Embedded Controller.

With these changes we also want to help future improvements in the
interface, like the Prashant's series (i.e [1]) to introduce a
cros_ec_cmd() that will allow us to remove more duplicated code in
different places.

Best regards,
 Enric

Note: Prashant, looks like you should fix your sendmail as the patches
      are not threaded.

[1] https://lkml.org/lkml/2020/2/5/614

Enric Balletbo i Serra (8):
  platform/chrome: cros_ec_proto: Report command not supported
  Input: cros_ec_keyb: Use cros_ec_cmd_xfer_status helper
  platform/chrome: cros_ec_vbc: Use cros_ec_cmd_xfer_status helper
  platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper
  platform/chrome: cros_ec_sysfs: Use cros_ec_cmd_xfer_status helper
  platform/chrome: cros_ec_lightbar: Use cros_ec_cmd_xfer_status helper
  platform/chrome: cros_ec: Use cros_ec_cmd_xfer_status helper
  platform/chrome: cros_ec_proto: Do not export cros_ec_cmd_xfer()

 drivers/input/keyboard/cros_ec_keyb.c       | 14 +++---
 drivers/platform/chrome/cros_ec.c           |  2 +-
 drivers/platform/chrome/cros_ec_chardev.c   |  2 +-
 drivers/platform/chrome/cros_ec_lightbar.c  | 50 ++++++---------------
 drivers/platform/chrome/cros_ec_proto.c     | 14 ++++--
 drivers/platform/chrome/cros_ec_sysfs.c     | 36 +++++++--------
 drivers/platform/chrome/cros_ec_vbc.c       |  4 +-
 include/linux/platform_data/cros_ec_proto.h |  3 --
 8 files changed, 50 insertions(+), 75 deletions(-)

-- 
2.25.0

[PATCH 1/8] platform/chrome: cros_ec_proto: Report command not supported

[Enric Balletbo i Serra]

In practice most drivers that use the EC protocol what really care is if
the result was successful or not, hence, we introduced a
cros_ec_cmd_xfer_status() function that converts EC errors to standard
Linux error codes. On some few cases, though, we are interested on know
if the command is supported or not, and in such cases, just ignore the
error. To achieve this, return a -ENOTSUPP error when the command is not
supported.

This will allow us to finish the conversion of all users to use the
cros_ec_cmd_xfer_status() function instead of cros_ec_cmd_xfer() and
make the latest private to the protocol driver, so users of the protocol
are not confused in which function they should use.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/platform/chrome/cros_ec_proto.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 3cfa643f1d07..3e745e0fe092 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -553,7 +553,10 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer);
  * replied with success status. It's not necessary to check msg->result when
  * using this function.
  *
- * Return: The number of bytes transferred on success or negative error code.
+ * Return:
+ * >=0 - The number of bytes transferred
+ * -ENOTSUPP - Operation not supported
+ * -EPROTO - Protocol error
  */
 int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
 			    struct cros_ec_command *msg)
@@ -563,6 +566,10 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
 	ret = cros_ec_cmd_xfer(ec_dev, msg);
 	if (ret < 0) {
 		dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
+	} else if (msg->result == EC_RES_INVALID_VERSION) {
+		dev_dbg(ec_dev->dev, "Command invalid version (err:%d)\n",
+			msg->result);
+		return -ENOTSUPP;
 	} else if (msg->result != EC_RES_SUCCESS) {
 		dev_dbg(ec_dev->dev, "Command result (err: %d)\n", msg->result);
 		return -EPROTO;
-- 
2.25.0

[PATCH 2/8] Input: cros_ec_keyb: Use cros_ec_cmd_xfer_status helper

[Enric Balletbo i Serra]

This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case there is no advantage of doing this
apart from that we want to make cros_ec_cmd_xfer() a private function
for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/input/keyboard/cros_ec_keyb.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 2b71c5a51f90..fc1793ca2f17 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -347,18 +347,14 @@ static int cros_ec_keyb_info(struct cros_ec_device *ec_dev,
 	params->info_type = info_type;
 	params->event_type = event_type;
 
-	ret = cros_ec_cmd_xfer(ec_dev, msg);
-	if (ret < 0) {
-		dev_warn(ec_dev->dev, "Transfer error %d/%d: %d\n",
-			 (int)info_type, (int)event_type, ret);
-	} else if (msg->result == EC_RES_INVALID_VERSION) {
+	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
+	if (ret == -ENOTSUPP) {
 		/* With older ECs we just return 0 for everything */
 		memset(result, 0, result_size);
 		ret = 0;
-	} else if (msg->result != EC_RES_SUCCESS) {
-		dev_warn(ec_dev->dev, "Error getting info %d/%d: %d\n",
-			 (int)info_type, (int)event_type, msg->result);
-		ret = -EPROTO;
+	} else if (ret < 0) {
+		dev_warn(ec_dev->dev, "Transfer error %d/%d: %d\n",
+			 (int)info_type, (int)event_type, ret);
 	} else if (ret != result_size) {
 		dev_warn(ec_dev->dev, "Wrong size %d/%d: %d != %zu\n",
 			 (int)info_type, (int)event_type,
-- 
2.25.0

[PATCH 3/8] platform/chrome: cros_ec_vbc: Use cros_ec_cmd_xfer_status helper

[Enric Balletbo i Serra]

This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case the change is trivial and the only
reason to do it is because we want to make cros_ec_cmd_xfer() a private
function for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/platform/chrome/cros_ec_vbc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_vbc.c b/drivers/platform/chrome/cros_ec_vbc.c
index 8edae465105c..46482d12cffe 100644
--- a/drivers/platform/chrome/cros_ec_vbc.c
+++ b/drivers/platform/chrome/cros_ec_vbc.c
@@ -40,7 +40,7 @@ static ssize_t vboot_context_read(struct file *filp, struct kobject *kobj,
 	msg->outsize = para_sz;
 	msg->insize = resp_sz;
 
-	err = cros_ec_cmd_xfer(ecdev, msg);
+	err = cros_ec_cmd_xfer_status(ecdev, msg);
 	if (err < 0) {
 		dev_err(dev, "Error sending read request: %d\n", err);
 		kfree(msg);
@@ -83,7 +83,7 @@ static ssize_t vboot_context_write(struct file *filp, struct kobject *kobj,
 	msg->outsize = para_sz;
 	msg->insize = 0;
 
-	err = cros_ec_cmd_xfer(ecdev, msg);
+	err = cros_ec_cmd_xfer_status(ecdev, msg);
 	if (err < 0) {
 		dev_err(dev, "Error sending write request: %d\n", err);
 		kfree(msg);
-- 
2.25.0

[PATCH 4/8] platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper

[Enric Balletbo i Serra]

This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case the change is trivial and the only
reason to do it is because we want to make cros_ec_cmd_xfer() a private
function for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/platform/chrome/cros_ec_chardev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index c65e70bc168d..b51ab24055f3 100644
--- a/drivers/platform/chrome/cros_ec_chardev.c
+++ b/drivers/platform/chrome/cros_ec_chardev.c
@@ -301,7 +301,7 @@ static long cros_ec_chardev_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
 	}
 
 	s_cmd->command += ec->cmd_offset;
-	ret = cros_ec_cmd_xfer(ec->ec_dev, s_cmd);
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, s_cmd);
 	/* Only copy data to userland if data was received. */
 	if (ret < 0)
 		goto exit;
-- 
2.25.0

[PATCH 5/8] platform/chrome: cros_ec_sysfs: Use cros_ec_cmd_xfer_status helper

[Enric Balletbo i Serra]

This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case the change is trivial and the only
reason to do it is because we want to make cros_ec_cmd_xfer() a private
function for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.

Looking at the code I am even unsure that makes sense differentiate
these two errors but let's not change the behaviour for now.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/platform/chrome/cros_ec_sysfs.c | 36 ++++++++++++-------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index 07dac97ad57c..d45ea5d5bfa4 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -149,14 +149,14 @@ static ssize_t version_show(struct device *dev,
 	/* Get build info. */
 	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);
-	if (ret < 0)
-		count += scnprintf(buf + count, PAGE_SIZE - count,
-				   "Build info:    XFER ERROR %d\n", ret);
-	else if (msg->result != EC_RES_SUCCESS)
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	if (ret == -EPROTO) {
 		count += scnprintf(buf + count, PAGE_SIZE - count,
 				   "Build info:    EC error %d\n", msg->result);
-	else {
+	} else if (ret < 0) {
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Build info:    XFER ERROR %d\n", ret);
+	} else {
 		msg->data[EC_HOST_PARAM_SIZE - 1] = '\0';
 		count += scnprintf(buf + count, PAGE_SIZE - count,
 				   "Build info:    %s\n", msg->data);
@@ -165,14 +165,14 @@ static ssize_t version_show(struct device *dev,
 	/* Get chip info. */
 	msg->command = EC_CMD_GET_CHIP_INFO + ec->cmd_offset;
 	msg->insize = sizeof(*r_chip);
-	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
-	if (ret < 0)
-		count += scnprintf(buf + count, PAGE_SIZE - count,
-				   "Chip info:     XFER ERROR %d\n", ret);
-	else if (msg->result != EC_RES_SUCCESS)
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	if (ret == -EPROTO) {
 		count += scnprintf(buf + count, PAGE_SIZE - count,
 				   "Chip info:     EC error %d\n", msg->result);
-	else {
+	} else if (ret < 0) {
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Chip info:     XFER ERROR %d\n", ret);
+	} else {
 		r_chip = (struct ec_response_get_chip_info *)msg->data;
 
 		r_chip->vendor[sizeof(r_chip->vendor) - 1] = '\0';
@@ -189,14 +189,14 @@ static ssize_t version_show(struct device *dev,
 	/* Get board version */
 	msg->command = EC_CMD_GET_BOARD_VERSION + ec->cmd_offset;
 	msg->insize = sizeof(*r_board);
-	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
-	if (ret < 0)
-		count += scnprintf(buf + count, PAGE_SIZE - count,
-				   "Board version: XFER ERROR %d\n", ret);
-	else if (msg->result != EC_RES_SUCCESS)
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	if (ret == -EPROTO) {
 		count += scnprintf(buf + count, PAGE_SIZE - count,
 				   "Board version: EC error %d\n", msg->result);
-	else {
+	} else if (ret < 0) {
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Board version: XFER ERROR %d\n", ret);
+	} else {
 		r_board = (struct ec_response_board_version *)msg->data;
 
 		count += scnprintf(buf + count, PAGE_SIZE - count,
-- 
2.25.0

[PATCH 6/8] platform/chrome: cros_ec_lightbar: Use cros_ec_cmd_xfer_status helper

[Enric Balletbo i Serra]

This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). It allows us to remove some redundand code. In this
case, though, we are changing a bit the behaviour because of returning
-EINVAL on protocol error we propagate the error return for
cros_ec_cmd_xfer_status() function, but I think it will be fine, even
more clear as we don't mask the Linux error code.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/platform/chrome/cros_ec_lightbar.c | 50 ++++++----------------
 1 file changed, 13 insertions(+), 37 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
index b4c110c5fee0..b59180bff5a3 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -116,7 +116,7 @@ static int get_lightbar_version(struct cros_ec_dev *ec,
 
 	param = (struct ec_params_lightbar *)msg->data;
 	param->cmd = LIGHTBAR_CMD_VERSION;
-	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
 	if (ret < 0) {
 		ret = 0;
 		goto exit;
@@ -193,15 +193,10 @@ static ssize_t brightness_store(struct device *dev,
 	if (ret)
 		goto exit;
 
-	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
 	if (ret < 0)
 		goto exit;
 
-	if (msg->result != EC_RES_SUCCESS) {
-		ret = -EINVAL;
-		goto exit;
-	}
-
 	ret = count;
 exit:
 	kfree(msg);
@@ -258,13 +253,10 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
 					goto exit;
 			}
 
-			ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+			ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
 			if (ret < 0)
 				goto exit;
 
-			if (msg->result != EC_RES_SUCCESS)
-				goto exit;
-
 			i = 0;
 			ok = 1;
 		}
@@ -305,14 +297,13 @@ static ssize_t sequence_show(struct device *dev,
 	if (ret)
 		goto exit;
 
-	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
-	if (ret < 0)
-		goto exit;
-
-	if (msg->result != EC_RES_SUCCESS) {
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	if (ret == -EPROTO) {
 		ret = scnprintf(buf, PAGE_SIZE,
 				"ERROR: EC returned %d\n", msg->result);
 		goto exit;
+	} else if (ret < 0) {
+		goto exit;
 	}
 
 	resp = (struct ec_response_lightbar *)msg->data;
@@ -344,13 +335,10 @@ static int lb_send_empty_cmd(struct cros_ec_dev *ec, uint8_t cmd)
 	if (ret)
 		goto error;
 
-	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
 	if (ret < 0)
 		goto error;
-	if (msg->result != EC_RES_SUCCESS) {
-		ret = -EINVAL;
-		goto error;
-	}
+
 	ret = 0;
 error:
 	kfree(msg);
@@ -377,13 +365,10 @@ static int lb_manual_suspend_ctrl(struct cros_ec_dev *ec, uint8_t enable)
 	if (ret)
 		goto error;
 
-	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
 	if (ret < 0)
 		goto error;
-	if (msg->result != EC_RES_SUCCESS) {
-		ret = -EINVAL;
-		goto error;
-	}
+
 	ret = 0;
 error:
 	kfree(msg);
@@ -425,15 +410,10 @@ static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
 	if (ret)
 		goto exit;
 
-	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
 	if (ret < 0)
 		goto exit;
 
-	if (msg->result != EC_RES_SUCCESS) {
-		ret = -EINVAL;
-		goto exit;
-	}
-
 	ret = count;
 exit:
 	kfree(msg);
@@ -487,13 +467,9 @@ static ssize_t program_store(struct device *dev, struct device_attribute *attr,
 	 */
 	msg->outsize = count + extra_bytes;
 
-	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
 	if (ret < 0)
 		goto exit;
-	if (msg->result != EC_RES_SUCCESS) {
-		ret = -EINVAL;
-		goto exit;
-	}
 
 	ret = count;
 exit:
-- 
2.25.0

[PATCH 7/8] platform/chrome: cros_ec: Use cros_ec_cmd_xfer_status helper

[Enric Balletbo i Serra]

This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case the change is trivial and the only
reason to do it is because we want to make cros_ec_cmd_xfer() a private
function for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/platform/chrome/cros_ec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 6fc8f2c3ac51..e179411bdfbe 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -120,7 +120,7 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
 
 	buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
 
-	ret = cros_ec_cmd_xfer(ec_dev, &buf.msg);
+	ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
 
 	/* For now, report failure to transition to S0ix with a warning. */
 	if (ret >= 0 && ec_dev->host_sleep_v1 &&
-- 
2.25.0

[PATCH 8/8] platform/chrome: cros_ec_proto: Do not export cros_ec_cmd_xfer()

[Enric Balletbo i Serra]

Now that all the remaining users of cros_ec_cmd_xfer() has been removed,
make this function private to the cros_ec_proto module.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/platform/chrome/cros_ec_proto.c     | 5 ++---
 include/linux/platform_data/cros_ec_proto.h | 3 ---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 3e745e0fe092..11a2db7cd0f7 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -496,8 +496,8 @@ EXPORT_SYMBOL(cros_ec_query_all);
  *
  * Return: 0 on success or negative error code.
  */
-int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
-		     struct cros_ec_command *msg)
+static int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
+			    struct cros_ec_command *msg)
 {
 	int ret;
 
@@ -541,7 +541,6 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 
 	return ret;
 }
-EXPORT_SYMBOL(cros_ec_cmd_xfer);
 
 /**
  * cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC.
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index ba5914770191..1334fedc07cb 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -212,9 +212,6 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
 int cros_ec_check_result(struct cros_ec_device *ec_dev,
 			 struct cros_ec_command *msg);
 
-int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
-		     struct cros_ec_command *msg);
-
 int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
 			    struct cros_ec_command *msg);
 
-- 
2.25.0

Re: [PATCH 4/8] platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper

[Prashant Malani]

Hi Enric,

On Thu, Feb 20, 2020 at 04:58:55PM +0100, Enric Balletbo i Serra wrote:
> This patch makes use of cros_ec_cmd_xfer_status() instead of
> cros_ec_cmd_xfer(). In this case the change is trivial and the only
> reason to do it is because we want to make cros_ec_cmd_xfer() a private
> function for the EC protocol and let people only use the
> cros_ec_cmd_xfer_status() to return Linux standard error codes.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  drivers/platform/chrome/cros_ec_chardev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
> index c65e70bc168d..b51ab24055f3 100644
> --- a/drivers/platform/chrome/cros_ec_chardev.c
> +++ b/drivers/platform/chrome/cros_ec_chardev.c
> @@ -301,7 +301,7 @@ static long cros_ec_chardev_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
>  	}
>  
>  	s_cmd->command += ec->cmd_offset;
> -	ret = cros_ec_cmd_xfer(ec->ec_dev, s_cmd);
> +	ret = cros_ec_cmd_xfer_status(ec->ec_dev, s_cmd);

One issue I see here is that if we were to later convert
cros_ec_cmd_xfer_status() to cros_ec_cmd(), we would lose the
s_cmd->result value, since I was hoping to avoid returning msg->result
via a pointer parameter. I don't know if userspace actually uses that, but I
am assuming it does.

So, should cros_ec_cmd() keep the *result pointer like so ?:

int cros_ec_cmd(struct cros_ec_device *ec, u32 version, u32 command,
               void *outdata, u32 outsize, void *indata, u32 insize, u32 *result);

This way, we can manually re-populate s_cmd->result with |*result|.

Or, should we drop msg->result while returning s_cmd to userspace? I am
guessing the answer is no, but thought I'd check with you first.

Best regards,


>  	/* Only copy data to userland if data was received. */
>  	if (ret < 0)
>  		goto exit;
> -- 
> 2.25.0
> 

Re: [PATCH 0/8] Migrate all cros_ec_cmd_xfer() calls to cros_ec_cmd_xfer_status()

[Prashant Malani]

On Thu, Feb 20, 2020 at 04:58:51PM +0100, Enric Balletbo i Serra wrote:
> Dear all,
> 
> The purpose of this series is get rid of the remaining places where the
> cros_ec_cmd_xfer() function is used in favour of the
> cros_ec_cmd_xfer_status() helper. This allows us to make the
> cros_ec_cmd_xfer() function private and only expose to the users a
> single way to send commands to the Embedded Controller.
> 
> With these changes we also want to help future improvements in the
> interface, like the Prashant's series (i.e [1]) to introduce a
> cros_ec_cmd() that will allow us to remove more duplicated code in
> different places.
> 
> Best regards,
>  Enric
> 
> Note: Prashant, looks like you should fix your sendmail as the patches
>       are not threaded.
> 
> [1] https://lkml.org/lkml/2020/2/5/614
> 
> Enric Balletbo i Serra (8):
>   platform/chrome: cros_ec_proto: Report command not supported
>   Input: cros_ec_keyb: Use cros_ec_cmd_xfer_status helper
>   platform/chrome: cros_ec_vbc: Use cros_ec_cmd_xfer_status helper
>   platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper
>   platform/chrome: cros_ec_sysfs: Use cros_ec_cmd_xfer_status helper
>   platform/chrome: cros_ec_lightbar: Use cros_ec_cmd_xfer_status helper
>   platform/chrome: cros_ec: Use cros_ec_cmd_xfer_status helper
>   platform/chrome: cros_ec_proto: Do not export cros_ec_cmd_xfer()

I picked this series on a device running 4.19 and didn't see any
unusual behaviour or dmesg logs, so for the entire series:

Tested-by: Prashant Malani <pmalani@chromium.org>
> 
>  drivers/input/keyboard/cros_ec_keyb.c       | 14 +++---
>  drivers/platform/chrome/cros_ec.c           |  2 +-
>  drivers/platform/chrome/cros_ec_chardev.c   |  2 +-
>  drivers/platform/chrome/cros_ec_lightbar.c  | 50 ++++++---------------
>  drivers/platform/chrome/cros_ec_proto.c     | 14 ++++--
>  drivers/platform/chrome/cros_ec_sysfs.c     | 36 +++++++--------
>  drivers/platform/chrome/cros_ec_vbc.c       |  4 +-
>  include/linux/platform_data/cros_ec_proto.h |  3 --
>  8 files changed, 50 insertions(+), 75 deletions(-)
> 
> -- 
> 2.25.0
> 

Re: [PATCH 4/8] platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper

[Enric Balletbo i Serra]

Hi Prashant,

On 25/2/20 20:55, Prashant Malani wrote:
> Hi Enric,
> 
> On Thu, Feb 20, 2020 at 04:58:55PM +0100, Enric Balletbo i Serra wrote:
>> This patch makes use of cros_ec_cmd_xfer_status() instead of
>> cros_ec_cmd_xfer(). In this case the change is trivial and the only
>> reason to do it is because we want to make cros_ec_cmd_xfer() a private
>> function for the EC protocol and let people only use the
>> cros_ec_cmd_xfer_status() to return Linux standard error codes.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>>  drivers/platform/chrome/cros_ec_chardev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
>> index c65e70bc168d..b51ab24055f3 100644
>> --- a/drivers/platform/chrome/cros_ec_chardev.c
>> +++ b/drivers/platform/chrome/cros_ec_chardev.c
>> @@ -301,7 +301,7 @@ static long cros_ec_chardev_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
>>  	}
>>  
>>  	s_cmd->command += ec->cmd_offset;
>> -	ret = cros_ec_cmd_xfer(ec->ec_dev, s_cmd);
>> +	ret = cros_ec_cmd_xfer_status(ec->ec_dev, s_cmd);
> 
> One issue I see here is that if we were to later convert
> cros_ec_cmd_xfer_status() to cros_ec_cmd(), we would lose the
> s_cmd->result value, since I was hoping to avoid returning msg->result
> via a pointer parameter. I don't know if userspace actually uses that, but I
> am assuming it does.
> 

I'd like to avoid returning msg->result via a pointer parameter too. I didn't
analyze all the cases but I suspect that in most cases it is not really needed,
so let's start by converting to the cros_ec_cmd the cases that are clear and
let's go one by one for the ones that are not clear.

IMO cros_ec_cmd should return the same as cros_ec_cmd_xfer_status. So you should
use cros_ec_cmd_xfer_status inside cros_ec_cmd.

Regards,
 Enric

> So, should cros_ec_cmd() keep the *result pointer like so ?:
> 
> int cros_ec_cmd(struct cros_ec_device *ec, u32 version, u32 command,
>                void *outdata, u32 outsize, void *indata, u32 insize, u32 *result);
> 
> This way, we can manually re-populate s_cmd->result with |*result|.
> 
> Or, should we drop msg->result while returning s_cmd to userspace? I am
> guessing the answer is no, but thought I'd check with you first.
> 
> Best regards,
> 
> 
>>  	/* Only copy data to userland if data was received. */
>>  	if (ret < 0)
>>  		goto exit;
>> -- 
>> 2.25.0
>>

Re: [PATCH 4/8] platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper

[Prashant Malani]

Hi Enric,

On Wed, Feb 26, 2020 at 7:00 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Prashant,
>
> On 25/2/20 20:55, Prashant Malani wrote:
> > Hi Enric,
> >
> > On Thu, Feb 20, 2020 at 04:58:55PM +0100, Enric Balletbo i Serra wrote:
> >> This patch makes use of cros_ec_cmd_xfer_status() instead of
> >> cros_ec_cmd_xfer(). In this case the change is trivial and the only
> >> reason to do it is because we want to make cros_ec_cmd_xfer() a private
> >> function for the EC protocol and let people only use the
> >> cros_ec_cmd_xfer_status() to return Linux standard error codes.
> >>
> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> ---
> >>
> >>  drivers/platform/chrome/cros_ec_chardev.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
> >> index c65e70bc168d..b51ab24055f3 100644
> >> --- a/drivers/platform/chrome/cros_ec_chardev.c
> >> +++ b/drivers/platform/chrome/cros_ec_chardev.c
> >> @@ -301,7 +301,7 @@ static long cros_ec_chardev_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
> >>      }
> >>
> >>      s_cmd->command += ec->cmd_offset;
> >> -    ret = cros_ec_cmd_xfer(ec->ec_dev, s_cmd);
> >> +    ret = cros_ec_cmd_xfer_status(ec->ec_dev, s_cmd);
> >
> > One issue I see here is that if we were to later convert
> > cros_ec_cmd_xfer_status() to cros_ec_cmd(), we would lose the
> > s_cmd->result value, since I was hoping to avoid returning msg->result
> > via a pointer parameter. I don't know if userspace actually uses that, but I
> > am assuming it does.
> >
>
> I'd like to avoid returning msg->result via a pointer parameter too. I didn't
> analyze all the cases but I suspect that in most cases it is not really needed,
> so let's start by converting to the cros_ec_cmd the cases that are clear and
> let's go one by one for the ones that are not clear.
>
I think the major one I was concerned about was in user-space:
https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/master/util/comm-dev.c#149

The above seems to return the result (after applying an offset).
FWIU the attempt would be to not change the user-space API, so would
we need to change this user-space too?

Best regards,

> IMO cros_ec_cmd should return the same as cros_ec_cmd_xfer_status. So you should
> use cros_ec_cmd_xfer_status inside cros_ec_cmd.
>
> Regards,
>  Enric
>
> > So, should cros_ec_cmd() keep the *result pointer like so ?:
> >
> > int cros_ec_cmd(struct cros_ec_device *ec, u32 version, u32 command,
> >                void *outdata, u32 outsize, void *indata, u32 insize, u32 *result);
> >
> > This way, we can manually re-populate s_cmd->result with |*result|.
> >
> > Or, should we drop msg->result while returning s_cmd to userspace? I am
> > guessing the answer is no, but thought I'd check with you first.
> >
> > Best regards,
> >
> >
> >>      /* Only copy data to userland if data was received. */
> >>      if (ret < 0)
> >>              goto exit;
> >> --
> >> 2.25.0
> >>

Re: [PATCH 2/8] Input: cros_ec_keyb: Use cros_ec_cmd_xfer_status helper

[Enric Balletbo i Serra]

Hi Dmitry,

Gentle ping, I'd like feedback from you on this series, and are you fine with
this change?

Thanks,
 Enric

On 20/2/20 16:58, Enric Balletbo i Serra wrote:
> This patch makes use of cros_ec_cmd_xfer_status() instead of
> cros_ec_cmd_xfer(). In this case there is no advantage of doing this
> apart from that we want to make cros_ec_cmd_xfer() a private function
> for the EC protocol and let people only use the
> cros_ec_cmd_xfer_status() to return Linux standard error codes.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  drivers/input/keyboard/cros_ec_keyb.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 2b71c5a51f90..fc1793ca2f17 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -347,18 +347,14 @@ static int cros_ec_keyb_info(struct cros_ec_device *ec_dev,
>  	params->info_type = info_type;
>  	params->event_type = event_type;
>  
> -	ret = cros_ec_cmd_xfer(ec_dev, msg);
> -	if (ret < 0) {
> -		dev_warn(ec_dev->dev, "Transfer error %d/%d: %d\n",
> -			 (int)info_type, (int)event_type, ret);
> -	} else if (msg->result == EC_RES_INVALID_VERSION) {
> +	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> +	if (ret == -ENOTSUPP) {
>  		/* With older ECs we just return 0 for everything */
>  		memset(result, 0, result_size);
>  		ret = 0;
> -	} else if (msg->result != EC_RES_SUCCESS) {
> -		dev_warn(ec_dev->dev, "Error getting info %d/%d: %d\n",
> -			 (int)info_type, (int)event_type, msg->result);
> -		ret = -EPROTO;
> +	} else if (ret < 0) {
> +		dev_warn(ec_dev->dev, "Transfer error %d/%d: %d\n",
> +			 (int)info_type, (int)event_type, ret);
>  	} else if (ret != result_size) {
>  		dev_warn(ec_dev->dev, "Wrong size %d/%d: %d != %zu\n",
>  			 (int)info_type, (int)event_type,
>