[PATCH v2 0/7] Second batch of cleanups for cros_ec

[PATCH v2 0/7] Second batch of cleanups for cros_ec

[Javier Martinez Canillas]

This is a second batch of cleanups patches for the mfd cros_ec
driver and its subdevices drivers. The first batch of cleanups
was posted by Doug Anderson [0] and have already been merged.
The patches were picked from the ChromeOS 3.8 kernel and after
these no cleanups patches for cros_ec are left, only commits
that add cros ec support not yet available in mainline.

This is a second version of the patch series that addresses
issues pointed out by Doug Anderson and Lee Jones on v1 [1].

There is almost no functionality added on this series but the
idea is to reduce the delta between the mainline drivers and
the ones in the downstream Chrome OS 3.8 kernel so the missing
functionality can be added on top once these cleanups patches
are merged. The missing functionlity currently in mainline is:

- Chrome OS Embedded Controller userspace device interface
- Chrome OS Embedded Controller Low Pin Count (LPC) inteface
- Access to vboot context stored on a block device
- Access to vboot context stored on EC's nvram

The patches in this series are authored by different people
(all on cc) and consist of the following:

Andrew Bresticker (3):
  mfd: cros_ec: stop calling ->cmd_xfer() directly
  mfd: cros_ec: move locking into cros_ec_cmd_xfer
  mfd: cros_ec: wait for completion of commands that return IN_PROGRESS

Derek Basehore (1):
  i2c: i2c-cros-ec-tunnel: Set retries to 3

Doug Anderson (1):
  mfd: cros_ec: Delay for 50ms when we see EC_CMD_REBOOT_EC

Todd Broch (2):
  mfd: cros_ec: Instantiate sub-devices from device tree
  Input: cros_ec_keyb: Optimize ghosting algorithm.

 drivers/i2c/busses/i2c-cros-ec-tunnel.c |  5 +-
 drivers/input/keyboard/cros_ec_keyb.c   | 94 ++++++++++++++++++---------------
 drivers/mfd/cros_ec.c                   | 78 ++++++++++++++++++++-------
 drivers/mfd/cros_ec_spi.c               | 20 ++++---
 include/linux/mfd/cros_ec.h             | 24 ++++++---
 5 files changed, 141 insertions(+), 80 deletions(-)

Patches #1, #2, #6 and #7 do not depend of others so they can be
merged independently but patches #3, #4 and #5 have to be merged
in that specific order since they depend on the previous one. 

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/6/16/681
[1]: http://comments.gmane.org/gmane.linux.drivers.i2c/19256

[PATCH v2 1/7] mfd: cros_ec: Delay for 50ms when we see EC_CMD_REBOOT_EC

[Javier Martinez Canillas]

From: Doug Anderson <dianders@chromium.org>

If someone sends a EC_CMD_REBOOT_EC to the EC, the EC will likely be
unresponsive for quite a while.  Add a delay to the end of the command
to prevent random failures of future commands.

NOTES:
* This could be optimized a bit by simply delaying the next command
  sent, but EC_CMD_REBOOT_EC is such a rare command that the extra
  complexity doesn't seem worth it.
* This is a bit of an "ugly hack" since the SPI driver is effectively
  snooping on the communication and making a lot of assumptions.  It
  would be nice to architect in some better solution long term.
* This same logic probably needs to be applied to the i2c driver.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/cros_ec_spi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index 588c700..b396705 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -65,6 +65,12 @@
   */
 #define EC_SPI_RECOVERY_TIME_NS	(200 * 1000)
 
+/*
+ * The EC is unresponsive for a time after a reboot command.  Add a
+ * simple delay to make sure that the bus stays locked.
+ */
+#define EC_REBOOT_DELAY_MS	50
+
 /**
  * struct cros_ec_spi - information about a SPI-connected EC
  *
@@ -318,6 +324,9 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
 
 	ret = len;
 exit:
+	if (ec_msg->command == EC_CMD_REBOOT_EC)
+		msleep(EC_REBOOT_DELAY_MS);
+
 	mutex_unlock(&ec_spi->lock);
 	return ret;
 }
-- 
2.0.1

[PATCH v2 2/7] i2c: i2c-cros-ec-tunnel: Set retries to 3

[Javier Martinez Canillas]

From: Derek Basehore <dbasehore@chromium.org>

Since the i2c bus can get wedged on the EC sometimes, set the number of retries
to 3. Since we un-wedge the bus immediately after the wedge happens, this is the
correct fix since only one transfer will fail.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Acked-by: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/i2c/busses/i2c-cros-ec-tunnel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
index 3c15dcc..97e6369 100644
--- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -16,6 +16,8 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
+#define I2C_MAX_RETRIES 3
+
 /**
  * struct ec_i2c_device - Driver data for I2C tunnel
  *
@@ -290,6 +292,7 @@ static int ec_i2c_probe(struct platform_device *pdev)
 	bus->adap.algo_data = bus;
 	bus->adap.dev.parent = &pdev->dev;
 	bus->adap.dev.of_node = np;
+	bus->adap.retries = I2C_MAX_RETRIES;
 
 	err = i2c_add_adapter(&bus->adap);
 	if (err) {
-- 
2.0.1

[PATCH v2 3/7] mfd: cros_ec: stop calling ->cmd_xfer() directly

[Javier Martinez Canillas]

From: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Instead of having users of the ChromeOS EC call the interface-specific
cmd_xfer() callback directly, introduce a central cros_ec_cmd_xfer()
to use instead.  This will allow us to put all the locking and retry
logic in one place instead of duplicating it across the different
drivers.

Signed-off-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
Reviewed-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/busses/i2c-cros-ec-tunnel.c |  2 +-
 drivers/input/keyboard/cros_ec_keyb.c   |  2 +-
 drivers/mfd/cros_ec.c                   |  7 +++++++
 include/linux/mfd/cros_ec.h             | 24 ++++++++++++++++++------
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
index 97e6369..ec5c38d 100644
--- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -229,7 +229,7 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
 	msg.indata = response;
 	msg.insize = response_len;
 
-	result = bus->ec->cmd_xfer(bus->ec, &msg);
+	result = cros_ec_cmd_xfer(bus->ec, &msg);
 	if (result < 0)
 		goto exit;
 
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 0bdbf2d..f8d4a8b 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -182,7 +182,7 @@ static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
 		.insize = ckdev->cols,
 	};
 
-	return ckdev->ec->cmd_xfer(ckdev->ec, &msg);
+	return cros_ec_cmd_xfer(ckdev->ec, &msg);
 }
 
 static irqreturn_t cros_ec_keyb_irq(int irq, void *data)
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index 4873f9c..a9faebd 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -62,6 +62,13 @@ int cros_ec_check_result(struct cros_ec_device *ec_dev,
 }
 EXPORT_SYMBOL(cros_ec_check_result);
 
+int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
+		     struct cros_ec_command *msg)
+{
+	return ec_dev->cmd_xfer(ec_dev, msg);
+}
+EXPORT_SYMBOL(cros_ec_cmd_xfer);
+
 static const struct mfd_cell cros_devs[] = {
 	{
 		.name = "cros-ec-keyb",
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index fcbe9d1..0e166b9 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -62,10 +62,6 @@ struct cros_ec_command {
  * @dev: Device pointer
  * @was_wake_device: true if this device was set to wake the system from
  * sleep at the last suspend
- * @cmd_xfer: send command to EC and get response
- *     Returns the number of bytes received if the communication succeeded, but
- *     that doesn't mean the EC was happy with the command. The caller
- *     should check msg.result for the EC's result code.
  *
  * @priv: Private data
  * @irq: Interrupt to use
@@ -82,6 +78,10 @@ struct cros_ec_command {
  * @dout_size: size of dout buffer to allocate (zero to use static dout)
  * @parent: pointer to parent device (e.g. i2c or spi device)
  * @wake_enabled: true if this device can wake the system from sleep
+ * @cmd_xfer: send command to EC and get response
+ *     Returns the number of bytes received if the communication succeeded, but
+ *     that doesn't mean the EC was happy with the command. The caller
+ *     should check msg.result for the EC's result code.
  * @lock: one transaction at a time
  */
 struct cros_ec_device {
@@ -92,8 +92,6 @@ struct cros_ec_device {
 	struct device *dev;
 	bool was_wake_device;
 	struct class *cros_class;
-	int (*cmd_xfer)(struct cros_ec_device *ec,
-			struct cros_ec_command *msg);
 
 	/* These are used to implement the platform-specific interface */
 	void *priv;
@@ -104,6 +102,8 @@ struct cros_ec_device {
 	int dout_size;
 	struct device *parent;
 	bool wake_enabled;
+	int (*cmd_xfer)(struct cros_ec_device *ec,
+			struct cros_ec_command *msg);
 	struct mutex lock;
 };
 
@@ -153,6 +153,18 @@ int cros_ec_check_result(struct cros_ec_device *ec_dev,
 			 struct cros_ec_command *msg);
 
 /**
+ * cros_ec_cmd_xfer - Send a command to the ChromeOS EC
+ *
+ * Call this to send a command to the ChromeOS EC.  This should be used
+ * instead of calling the EC's cmd_xfer() callback directly.
+ *
+ * @ec_dev: EC device
+ * @msg: Message to write
+ */
+int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
+		     struct cros_ec_command *msg);
+
+/**
  * cros_ec_remove - Remove a ChromeOS EC
  *
  * Call this to deregister a ChromeOS EC, then clean up any private data.
-- 
2.0.1

[PATCH v2 4/7] mfd: cros_ec: move locking into cros_ec_cmd_xfer

[Javier Martinez Canillas]

From: Andrew Bresticker <abrestic@chromium.org>

Now that there's a central cros_ec_cmd_xfer(), move the locking
out of the SPI driver.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---

Changes since v1:
 - Remove mention of LPC driver in the commit message since it does not
   exist in mainline yet. Suggested by Doug Anderson.
---
 drivers/mfd/cros_ec.c     | 10 +++++++++-
 drivers/mfd/cros_ec_spi.c | 11 -----------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index a9faebd..c53804a 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -65,7 +65,13 @@ EXPORT_SYMBOL(cros_ec_check_result);
 int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 		     struct cros_ec_command *msg)
 {
-	return ec_dev->cmd_xfer(ec_dev, msg);
+	int ret;
+
+	mutex_lock(&ec_dev->lock);
+	ret = ec_dev->cmd_xfer(ec_dev, msg);
+	mutex_unlock(&ec_dev->lock);
+
+	return ret;
 }
 EXPORT_SYMBOL(cros_ec_cmd_xfer);
 
@@ -98,6 +104,8 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 			return -ENOMEM;
 	}
 
+	mutex_init(&ec_dev->lock);
+
 	err = mfd_add_devices(dev, 0, cros_devs,
 			      ARRAY_SIZE(cros_devs),
 			      NULL, ec_dev->irq, NULL);
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index b396705..bf6e08e 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -79,13 +79,11 @@
  *	if no record
  * @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that
  *      is sent when we want to turn off CS at the end of a transaction.
- * @lock: mutex to ensure only one user of cros_ec_cmd_xfer_spi at a time
  */
 struct cros_ec_spi {
 	struct spi_device *spi;
 	s64 last_transfer_ns;
 	unsigned int end_of_msg_delay;
-	struct mutex lock;
 };
 
 static void debug_packet(struct device *dev, const char *name, u8 *ptr,
@@ -232,13 +230,6 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
 	int sum;
 	int ret = 0, final_ret;
 
-	/*
-	 * We have the shared ec_dev buffer plus we do lots of separate spi_sync
-	 * calls, so we need to make sure only one person is using this at a
-	 * time.
-	 */
-	mutex_lock(&ec_spi->lock);
-
 	len = cros_ec_prepare_tx(ec_dev, ec_msg);
 	dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);
 
@@ -327,7 +318,6 @@ exit:
 	if (ec_msg->command == EC_CMD_REBOOT_EC)
 		msleep(EC_REBOOT_DELAY_MS);
 
-	mutex_unlock(&ec_spi->lock);
 	return ret;
 }
 
@@ -359,7 +349,6 @@ static int cros_ec_spi_probe(struct spi_device *spi)
 	if (ec_spi == NULL)
 		return -ENOMEM;
 	ec_spi->spi = spi;
-	mutex_init(&ec_spi->lock);
 	ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
 	if (!ec_dev)
 		return -ENOMEM;
-- 
2.0.1

[PATCH v2 5/7] mfd: cros_ec: wait for completion of commands that return IN_PROGRESS

[Javier Martinez Canillas]

From: Andrew Bresticker <abrestic@chromium.org>

When an EC command returns EC_RES_IN_PROGRESS, we need to query
the state of the EC until it indicates that it is no longer busy.
Do this in cros_ec_cmd_xfer() under the EC's mutex so that other
commands (e.g. keyboard, I2C passtru) aren't issued to the EC while
it is working on the in-progress command.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

Changes since v1:
 - The *xfer() calls don't modify the passed cros_ec_command so there is
   no need to populate it inside the for loop. Suggested by Lee Jones.
---
 drivers/mfd/cros_ec.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index c53804a..cd0c93c 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -23,6 +23,10 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/cros_ec.h>
 #include <linux/mfd/cros_ec_commands.h>
+#include <linux/delay.h>
+
+#define EC_COMMAND_RETRIES	50
+#define EC_RETRY_DELAY_MS	10
 
 int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
 		       struct cros_ec_command *msg)
@@ -65,10 +69,38 @@ EXPORT_SYMBOL(cros_ec_check_result);
 int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 		     struct cros_ec_command *msg)
 {
-	int ret;
+	int ret, i;
+	struct cros_ec_command status_msg;
+	struct ec_response_get_comms_status status;
 
 	mutex_lock(&ec_dev->lock);
 	ret = ec_dev->cmd_xfer(ec_dev, msg);
+	if (ret == -EAGAIN && msg->result == EC_RES_IN_PROGRESS) {
+		/*
+		 * Query the EC's status until it's no longer busy or
+		 * we encounter an error.
+		 */
+		status_msg.version = 0;
+		status_msg.command = EC_CMD_GET_COMMS_STATUS;
+		status_msg.outdata = NULL;
+		status_msg.outsize = 0;
+		status_msg.indata = (uint8_t *)&status;
+		status_msg.insize = sizeof(status);
+
+		for (i = 0; i < EC_COMMAND_RETRIES; i++) {
+			msleep(EC_RETRY_DELAY_MS);
+
+			ret = ec_dev->cmd_xfer(ec_dev, &status_msg);
+			if (ret < 0)
+				break;
+
+			msg->result = status_msg.result;
+			if (status_msg.result != EC_RES_SUCCESS)
+				break;
+			if (!(status.flags & EC_COMMS_STATUS_PROCESSING))
+				break;
+		}
+	}
 	mutex_unlock(&ec_dev->lock);
 
 	return ret;
-- 
2.0.1

[PATCH v2 6/7] mfd: cros_ec: Instantiate sub-devices from device tree

[Javier Martinez Canillas]

From: Todd Broch <tbroch@chromium.org>

If the EC device tree node has sub-nodes, try to instantiate them as
MFD sub-devices.  We can configure the EC features provided by the board.

Signed-off-by: Todd Broch <tbroch@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---

Changes since v1:
 - Don't leave an empty struct mfd_cell array. Suggested by Lee Jones.
 - Just use of_platform_populate() instead of manually iterating through
   sub-devices and calling mfd_add_devices. Suggested by Lee Jones.
---
 drivers/mfd/cros_ec.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index cd0c93c..ab70791 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -21,6 +21,7 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/mfd/core.h>
+#include <linux/of_platform.h>
 #include <linux/mfd/cros_ec.h>
 #include <linux/mfd/cros_ec_commands.h>
 #include <linux/delay.h>
@@ -107,22 +108,12 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 }
 EXPORT_SYMBOL(cros_ec_cmd_xfer);
 
-static const struct mfd_cell cros_devs[] = {
-	{
-		.name = "cros-ec-keyb",
-		.id = 1,
-		.of_compatible = "google,cros-ec-keyb",
-	},
-	{
-		.name = "cros-ec-i2c-tunnel",
-		.id = 2,
-		.of_compatible = "google,cros-ec-i2c-tunnel",
-	},
-};
-
 int cros_ec_register(struct cros_ec_device *ec_dev)
 {
 	struct device *dev = ec_dev->dev;
+#ifdef CONFIG_OF
+	struct device_node *node = dev->of_node;
+#endif
 	int err = 0;
 
 	if (ec_dev->din_size) {
@@ -138,13 +129,19 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 
 	mutex_init(&ec_dev->lock);
 
-	err = mfd_add_devices(dev, 0, cros_devs,
-			      ARRAY_SIZE(cros_devs),
-			      NULL, ec_dev->irq, NULL);
-	if (err) {
-		dev_err(dev, "failed to add mfd devices\n");
-		return err;
+#ifdef CONFIG_OF
+	/*
+	 * Add sub-devices declared in the device tree.  NOTE they should NOT be
+	 * declared in cros_devs
+	 */
+	if (node) {
+		err = of_platform_populate(node, NULL, NULL, dev);
+		if (err) {
+			dev_err(dev, "fail to add %s\n", node->full_name);
+			return err;
+		}
 	}
+#endif
 
 	dev_info(dev, "Chrome EC device registered\n");
 
-- 
2.0.1

[PATCH v2 7/7] Input: cros_ec_keyb: Optimize ghosting algorithm.

[Javier Martinez Canillas]

From: Todd Broch <tbroch-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Previous algorithm was a bit conservative and complicating with
respect to identifying key ghosting.  This CL uses the bitops hamming
weight function (hweight8) to count the number of matching rows for
colM & colN.  If that number is > 1 ghosting is present.

Additionally it removes NULL keys and our one virtual keypress
KEY_BATTERY from consideration as these inputs are never physical
keypresses.

Signed-off-by: Todd Broch <tbroch-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Vincent Palatin <vpalatin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Luigi Semenzato <semenzato-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Tested-by: Andreas Färber <afaerber-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8oXa1LcwEujcA@public.gmane.orgk>
---
 drivers/input/keyboard/cros_ec_keyb.c | 92 +++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 43 deletions(-)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index f8d4a8b..462bfcb 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -22,6 +22,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/bitops.h>
 #include <linux/i2c.h>
 #include <linux/input.h>
 #include <linux/interrupt.h>
@@ -38,6 +39,7 @@
  * @row_shift: log2 or number of rows, rounded up
  * @keymap_data: Matrix keymap data used to convert to keyscan values
  * @ghost_filter: true to enable the matrix key-ghosting filter
+ * @valid_keys: bitmap of existing keys for each matrix column
  * @old_kb_state: bitmap of keys pressed last scan
  * @dev: Device pointer
  * @idev: Input device
@@ -49,6 +51,7 @@ struct cros_ec_keyb {
 	int row_shift;
 	const struct matrix_keymap_data *keymap_data;
 	bool ghost_filter;
+	uint8_t *valid_keys;
 	uint8_t *old_kb_state;
 
 	struct device *dev;
@@ -57,39 +60,15 @@ struct cros_ec_keyb {
 };
 
 
-static bool cros_ec_keyb_row_has_ghosting(struct cros_ec_keyb *ckdev,
-					  uint8_t *buf, int row)
-{
-	int pressed_in_row = 0;
-	int row_has_teeth = 0;
-	int col, mask;
-
-	mask = 1 << row;
-	for (col = 0; col < ckdev->cols; col++) {
-		if (buf[col] & mask) {
-			pressed_in_row++;
-			row_has_teeth |= buf[col] & ~mask;
-			if (pressed_in_row > 1 && row_has_teeth) {
-				/* ghosting */
-				dev_dbg(ckdev->dev,
-					"ghost found at: r%d c%d, pressed %d, teeth 0x%x\n",
-					row, col, pressed_in_row,
-					row_has_teeth);
-				return true;
-			}
-		}
-	}
-
-	return false;
-}
-
 /*
  * Returns true when there is at least one combination of pressed keys that
  * results in ghosting.
  */
 static bool cros_ec_keyb_has_ghosting(struct cros_ec_keyb *ckdev, uint8_t *buf)
 {
-	int row;
+	int col1, col2, buf1, buf2;
+	struct device *dev = ckdev->dev;
+	uint8_t *valid_keys = ckdev->valid_keys;
 
 	/*
 	 * Ghosting happens if for any pressed key X there are other keys
@@ -103,27 +82,23 @@ static bool cros_ec_keyb_has_ghosting(struct cros_ec_keyb *ckdev, uint8_t *buf)
 	 *
 	 * In this case only X, Y, and Z are pressed, but g appears to be
 	 * pressed too (see Wikipedia).
-	 *
-	 * We can detect ghosting in a single pass (*) over the keyboard state
-	 * by maintaining two arrays.  pressed_in_row counts how many pressed
-	 * keys we have found in a row.  row_has_teeth is true if any of the
-	 * pressed keys for this row has other pressed keys in its column.  If
-	 * at any point of the scan we find that a row has multiple pressed
-	 * keys, and at least one of them is at the intersection with a column
-	 * with multiple pressed keys, we're sure there is ghosting.
-	 * Conversely, if there is ghosting, we will detect such situation for
-	 * at least one key during the pass.
-	 *
-	 * (*) This looks linear in the number of keys, but it's not.  We can
-	 * cheat because the number of rows is small.
 	 */
-	for (row = 0; row < ckdev->rows; row++)
-		if (cros_ec_keyb_row_has_ghosting(ckdev, buf, row))
-			return true;
+	for (col1 = 0; col1 < ckdev->cols; col1++) {
+		buf1 = buf[col1] & valid_keys[col1];
+		for (col2 = col1 + 1; col2 < ckdev->cols; col2++) {
+			buf2 = buf[col2] & valid_keys[col2];
+			if (hweight8(buf1 & buf2) > 1) {
+				dev_dbg(dev, "ghost found at: B[%02d]:0x%02x & B[%02d]:0x%02x",
+					col1, buf1, col2, buf2);
+				return true;
+			}
+		}
+	}
 
 	return false;
 }
 
+
 /*
  * Compares the new keyboard state to the old one and produces key
  * press/release events accordingly.  The keyboard state is 13 bytes (one byte
@@ -222,6 +197,30 @@ static void cros_ec_keyb_close(struct input_dev *dev)
 	free_irq(ec->irq, ckdev);
 }
 
+/*
+ * Walks keycodes flipping bit in buffer COLUMNS deep where bit is ROW.  Used by
+ * ghosting logic to ignore NULL or virtual keys.
+ */
+static void cros_ec_keyb_compute_valid_keys(struct cros_ec_keyb *ckdev)
+{
+	int row, col;
+	int row_shift = ckdev->row_shift;
+	unsigned short *keymap = ckdev->idev->keycode;
+	unsigned short code;
+
+	BUG_ON(ckdev->idev->keycodesize != sizeof(*keymap));
+
+	for (col = 0; col < ckdev->cols; col++) {
+		for (row = 0; row < ckdev->rows; row++) {
+			code = keymap[MATRIX_SCAN_CODE(row, col, row_shift)];
+			if (code && (code != KEY_BATTERY))
+				ckdev->valid_keys[col] |= 1 << row;
+		}
+		dev_dbg(ckdev->dev, "valid_keys[%02d] = 0x%02x\n",
+			col, ckdev->valid_keys[col]);
+	}
+}
+
 static int cros_ec_keyb_probe(struct platform_device *pdev)
 {
 	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
@@ -242,6 +241,11 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
 					    &ckdev->cols);
 	if (err)
 		return err;
+
+	ckdev->valid_keys = devm_kzalloc(&pdev->dev, ckdev->cols, GFP_KERNEL);
+	if (!ckdev->valid_keys)
+		return -ENOMEM;
+
 	ckdev->old_kb_state = devm_kzalloc(&pdev->dev, ckdev->cols, GFP_KERNEL);
 	if (!ckdev->old_kb_state)
 		return -ENOMEM;
@@ -285,6 +289,8 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
 	input_set_capability(idev, EV_MSC, MSC_SCAN);
 	input_set_drvdata(idev, ckdev);
 	ckdev->idev = idev;
+	cros_ec_keyb_compute_valid_keys(ckdev);
+
 	err = input_register_device(ckdev->idev);
 	if (err) {
 		dev_err(dev, "cannot register input device\n");
-- 
2.0.1

Re: [PATCH v2 0/7] Second batch of cleanups for cros_ec

[Dmitry Torokhov]

On Mon, Aug 25, 2014 at 03:40:01PM +0200, Javier Martinez Canillas wrote:
> This is a second batch of cleanups patches for the mfd cros_ec
> driver and its subdevices drivers. The first batch of cleanups
> was posted by Doug Anderson [0] and have already been merged.
> The patches were picked from the ChromeOS 3.8 kernel and after
> these no cleanups patches for cros_ec are left, only commits
> that add cros ec support not yet available in mainline.
> 
> This is a second version of the patch series that addresses
> issues pointed out by Doug Anderson and Lee Jones on v1 [1].
> 
> There is almost no functionality added on this series but the
> idea is to reduce the delta between the mainline drivers and
> the ones in the downstream Chrome OS 3.8 kernel so the missing
> functionality can be added on top once these cleanups patches
> are merged. The missing functionlity currently in mainline is:
> 
> - Chrome OS Embedded Controller userspace device interface
> - Chrome OS Embedded Controller Low Pin Count (LPC) inteface
> - Access to vboot context stored on a block device
> - Access to vboot context stored on EC's nvram
> 
> The patches in this series are authored by different people
> (all on cc) and consist of the following:
> 
> Andrew Bresticker (3):
>   mfd: cros_ec: stop calling ->cmd_xfer() directly
>   mfd: cros_ec: move locking into cros_ec_cmd_xfer
>   mfd: cros_ec: wait for completion of commands that return IN_PROGRESS
> 
> Derek Basehore (1):
>   i2c: i2c-cros-ec-tunnel: Set retries to 3
> 
> Doug Anderson (1):
>   mfd: cros_ec: Delay for 50ms when we see EC_CMD_REBOOT_EC
> 
> Todd Broch (2):
>   mfd: cros_ec: Instantiate sub-devices from device tree
>   Input: cros_ec_keyb: Optimize ghosting algorithm.
> 
>  drivers/i2c/busses/i2c-cros-ec-tunnel.c |  5 +-
>  drivers/input/keyboard/cros_ec_keyb.c   | 94 ++++++++++++++++++---------------
>  drivers/mfd/cros_ec.c                   | 78 ++++++++++++++++++++-------
>  drivers/mfd/cros_ec_spi.c               | 20 ++++---
>  include/linux/mfd/cros_ec.h             | 24 ++++++---
>  5 files changed, 141 insertions(+), 80 deletions(-)
> 
> Patches #1, #2, #6 and #7 do not depend of others so they can be
> merged independently but patches #3, #4 and #5 have to be merged
> in that specific order since they depend on the previous one. 

#7 does not apply to my tree (I guess it depends on the 1st batch which I
expect will go through MFD tree?). Maybe you could rebase it on top of my next
so it can be applied sooner? Or it really needs parts of patchset #1?

Thanks.

-- 
Dmitry

Re: [PATCH v2 0/7] Second batch of cleanups for cros_ec

[Javier Martinez Canillas]

Hello Dmitry,

On 08/25/2014 07:05 PM, Dmitry Torokhov wrote:
>> 
>> Patches #1, #2, #6 and #7 do not depend of others so they can be
>> merged independently but patches #3, #4 and #5 have to be merged
>> in that specific order since they depend on the previous one. 
> 
> #7 does not apply to my tree (I guess it depends on the 1st batch which I
> expect will go through MFD tree?). Maybe you could rebase it on top of my next
> so it can be applied sooner? Or it really needs parts of patchset #1?
> 

The first batch sent by Doug did indeed touch this driver and the patches
were merged through the MFD tree for 3.17 as you said. I see that your
next branch is based on 3.16-rc6 and that is why it does not apply cleanly.

I guess you will rebase your next branch for 3.18 on top of 3.17-rc1
anyways which will fix this issue? If not please let me know and I can of
course re-spin the patch so it applies cleanly on top of 3.16-rc6.

> Thanks.
> 

Best regards,
Javier

Re: [PATCH v2 0/7] Second batch of cleanups for cros_ec

[Dmitry Torokhov]

On Mon, Aug 25, 2014 at 07:28:01PM +0200, Javier Martinez Canillas wrote:
> Hello Dmitry,
> 
> On 08/25/2014 07:05 PM, Dmitry Torokhov wrote:
> >> 
> >> Patches #1, #2, #6 and #7 do not depend of others so they can be
> >> merged independently but patches #3, #4 and #5 have to be merged
> >> in that specific order since they depend on the previous one. 
> > 
> > #7 does not apply to my tree (I guess it depends on the 1st batch which I
> > expect will go through MFD tree?). Maybe you could rebase it on top of my next
> > so it can be applied sooner? Or it really needs parts of patchset #1?
> > 
> 
> The first batch sent by Doug did indeed touch this driver and the patches
> were merged through the MFD tree for 3.17 as you said. I see that your
> next branch is based on 3.16-rc6 and that is why it does not apply cleanly.
> 
> I guess you will rebase your next branch for 3.18 on top of 3.17-rc1
> anyways which will fix this issue? If not please let me know and I can of
> course re-spin the patch so it applies cleanly on top of 3.16-rc6.

I normally merge with mainline around -rc3 so please ping me around that time
if you do not see the patch in my tree.

Thanks.

-- 
Dmitry

Re: [PATCH v2 0/7] Second batch of cleanups for cros_ec

[Javier Martinez Canillas]

Hello Dmitry,

On 08/25/2014 08:01 PM, Dmitry Torokhov wrote:
> On Mon, Aug 25, 2014 at 07:28:01PM +0200, Javier Martinez Canillas wrote:
>> > 
>> > #7 does not apply to my tree (I guess it depends on the 1st batch which I
>> > expect will go through MFD tree?). Maybe you could rebase it on top of my next
>> > so it can be applied sooner? Or it really needs parts of patchset #1?
>> > 
>> 
>> The first batch sent by Doug did indeed touch this driver and the patches
>> were merged through the MFD tree for 3.17 as you said. I see that your
>> next branch is based on 3.16-rc6 and that is why it does not apply cleanly.
>> 
>> I guess you will rebase your next branch for 3.18 on top of 3.17-rc1
>> anyways which will fix this issue? If not please let me know and I can of
>> course re-spin the patch so it applies cleanly on top of 3.16-rc6.
> 
> I normally merge with mainline around -rc3 so please ping me around that time
> if you do not see the patch in my tree.
>

Will do, thanks a lot for your help!

> Thanks.
> 

Best regards,
Javier

Re: [PATCH v2 7/7] Input: cros_ec_keyb: Optimize ghosting algorithm.

[Dmitry Torokhov]

On Mon, Aug 25, 2014 at 03:40:08PM +0200, Javier Martinez Canillas wrote:
> From: Todd Broch <tbroch@chromium.org>
> 
> Previous algorithm was a bit conservative and complicating with
> respect to identifying key ghosting.  This CL uses the bitops hamming
> weight function (hweight8) to count the number of matching rows for
> colM & colN.  If that number is > 1 ghosting is present.
> 
> Additionally it removes NULL keys and our one virtual keypress
> KEY_BATTERY from consideration as these inputs are never physical
> keypresses.
> 
> Signed-off-by: Todd Broch <tbroch@chromium.org>
> Reviewed-by: Vincent Palatin <vpalatin@chromium.org>
> Reviewed-by: Luigi Semenzato <semenzato@chromium.org>
> Tested-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Applied, thank you.

-- 
Dmitry

Re: [PATCH v2 6/7] mfd: cros_ec: Instantiate sub-devices from device tree

[Javier Martinez Canillas]

Hello Lee,

Sorry for the delay but had been on holidays last week.

On 09/04/2014 10:25 AM, Lee Jones wrote:
>>  
>> -static const struct mfd_cell cros_devs[] = {
>> -	{
>> -		.name = "cros-ec-keyb",
>> -		.id = 1,
>> -		.of_compatible = "google,cros-ec-keyb",
>> -	},
>> -	{
>> -		.name = "cros-ec-i2c-tunnel",
>> -		.id = 2,
>> -		.of_compatible = "google,cros-ec-i2c-tunnel",
>> -	},
>> -};
>> -
>>  int cros_ec_register(struct cros_ec_device *ec_dev)
>>  {
>>  	struct device *dev = ec_dev->dev;
>> +#ifdef CONFIG_OF
>> +	struct device_node *node = dev->of_node;
>> +#endif
> 
> Drop the #ifdiffery.
>

I used the #ifdef guards because I remembered of_node being conditionally
build but I see that this is not longer true after commmit:

c9e358d driver-core: remove conditionals around devicetree pointers

So I'll remove it, thanks for the pointer.

>>  	int err = 0;
>>  
>>  	if (ec_dev->din_size) {
>> @@ -138,13 +129,19 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>>  
>>  	mutex_init(&ec_dev->lock);
>>  
>> -	err = mfd_add_devices(dev, 0, cros_devs,
>> -			      ARRAY_SIZE(cros_devs),
>> -			      NULL, ec_dev->irq, NULL);
>> -	if (err) {
>> -		dev_err(dev, "failed to add mfd devices\n");
>> -		return err;
>> +#ifdef CONFIG_OF
> 
> And here.
> 

Nod.

>> +	/*
>> +	 * Add sub-devices declared in the device tree.  NOTE they should NOT be
>> +	 * declared in cros_devs
>> +	 */
> 
> Drop this comment.  cros_devs no longer exists and we know what
> of_platform_populate() does.
> 

Ok.

>> +	if (node) {
>> +		err = of_platform_populate(node, NULL, NULL, dev);
>> +		if (err) {
>> +			dev_err(dev, "fail to add %s\n", node->full_name);
> 
> That's not true.  Just put "Failed to register subordinate devices".
> 

Ok.

>> +			return err;
>> +		}
>>  	}
>> +#endif
>>  
>>  	dev_info(dev, "Chrome EC device registered\n");
>>  
> 

Thanks a lot for the feedback and best regards,
Javier

Re: [PATCH v2 5/7] mfd: cros_ec: wait for completion of commands that return IN_PROGRESS

[Javier Martinez Canillas]

Hello Lee,

On 09/04/2014 10:34 AM, Lee Jones wrote:
> On Mon, 25 Aug 2014, Javier Martinez Canillas wrote:
>> From: Andrew Bresticker <abrestic@chromium.org>
>> 
>> When an EC command returns EC_RES_IN_PROGRESS, we need to query
>> the state of the EC until it indicates that it is no longer busy.
>> Do this in cros_ec_cmd_xfer() under the EC's mutex so that other
>> commands (e.g. keyboard, I2C passtru) aren't issued to the EC while
>> it is working on the in-progress command.
>> 
>> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>> 
>> Changes since v1:
>>  - The *xfer() calls don't modify the passed cros_ec_command so there is
>>    no need to populate it inside the for loop. Suggested by Lee Jones.
>> ---
>>  drivers/mfd/cros_ec.c | 34 +++++++++++++++++++++++++++++++++-
>>  1 file changed, 33 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>> index c53804a..cd0c93c 100644
>> --- a/drivers/mfd/cros_ec.c
>> +++ b/drivers/mfd/cros_ec.c
>> @@ -23,6 +23,10 @@
>>  #include <linux/mfd/core.h>
>>  #include <linux/mfd/cros_ec.h>
>>  #include <linux/mfd/cros_ec_commands.h>
>> +#include <linux/delay.h>
>> +
>> +#define EC_COMMAND_RETRIES	50
>> +#define EC_RETRY_DELAY_MS	10
> 
> Where did these values come from?
> 

These patches were taken from the ChromeOS 3.8 kernel so I don't really know
why these values were chosen. I'll let Andrew or one of the ChromiumOS folks
to answer this question.

>>  int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
>>  		       struct cros_ec_command *msg)
>> @@ -65,10 +69,38 @@ EXPORT_SYMBOL(cros_ec_check_result);
>>  int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
>>  		     struct cros_ec_command *msg)
>>  {
>> -	int ret;
>> +	int ret, i;
>> +	struct cros_ec_command status_msg;
>> +	struct ec_response_get_comms_status status;
> 
> Please put these inside the if().
> 

Ok.

>>  	mutex_lock(&ec_dev->lock);
>>  	ret = ec_dev->cmd_xfer(ec_dev, msg);
>> +	if (ret == -EAGAIN && msg->result == EC_RES_IN_PROGRESS) {
> 
> Is there ever a time where (ret == -EAGAIN) but (msg->result !=
> EC_RES_IN_PROGRESS) [note the !=].  And/or is there ever a time where
> (msg->result == EC_RES_IN_PROGRESS) but (ret != -EAGAIN) [again, not
> the !=].
> 
> Another way of explaining it.  Can ret be anything other than -EAGAIN
> when the result is EC_RES_IN_PROGRESS.  And can the result be anything
> other than EC_RES_IN_PROGRESS when ret is -EAGAIN?
>

For the first question, no. ret is always -EAGAIN when result is
EC_RES_IN_PROGRESS.

All the cros_ec transport drivers (cros_ec_{i2c,spi,lpc}) have the following
code block:

	switch (msg->result) {
	...
	case EC_RES_IN_PROGRESS:
		ret = -EAGAIN;
	...
	};

For the second question, yes AFAICT. Some transports transfer function return
-EAGAIN and that error is propagated. As an example i2c_transfer() returns
-EAGAIN if the struct i2c_adapter bus_lock mutex is tried to be acquired.

But after looking at all the cros_ec transport drivers it seems to be safe to
just check if result is EC_RES_IN_PROGRESS instead of checking also if ret is
-EAGAIN since (at least on all the current transport drivers) the former
implies the later.

>> +		/*
>> +		 * Query the EC's status until it's no longer busy or
>> +		 * we encounter an error.
>> +		 */
>> +		status_msg.version = 0;
>> +		status_msg.command = EC_CMD_GET_COMMS_STATUS;
>> +		status_msg.outdata = NULL;
>> +		status_msg.outsize = 0;
>> +		status_msg.indata = (uint8_t *)&status;
>> +		status_msg.insize = sizeof(status);
>> +
>> +		for (i = 0; i < EC_COMMAND_RETRIES; i++) {
>> +			msleep(EC_RETRY_DELAY_MS);
> 
> msleep() doesn't handle any time below 20ms well, use usleep() or even
> better usleep_range() instead.
> 

Ok.

>> +			ret = ec_dev->cmd_xfer(ec_dev, &status_msg);
>> +			if (ret < 0)
>> +				break;
> 
> What does a ret of >0 mean?
> 

When ret > 0, it means the actual amount of data received in the transfer.

>> +			msg->result = status_msg.result;
>> +			if (status_msg.result != EC_RES_SUCCESS)
>> +				break;
>> +			if (!(status.flags & EC_COMMS_STATUS_PROCESSING))
>> +				break;
>> +		}
>> +	}
>>  	mutex_unlock(&ec_dev->lock);
>>  
>>  	return ret;
> 

Best regards,
Javier

Re: [PATCH v2 5/7] mfd: cros_ec: wait for completion of commands that return IN_PROGRESS

[Lee Jones]

On Mon, 08 Sep 2014, Javier Martinez Canillas wrote:
> On 09/04/2014 10:34 AM, Lee Jones wrote:
> > On Mon, 25 Aug 2014, Javier Martinez Canillas wrote:
> >> From: Andrew Bresticker <abrestic@chromium.org>
> >> 
> >> When an EC command returns EC_RES_IN_PROGRESS, we need to query
> >> the state of the EC until it indicates that it is no longer busy.
> >> Do this in cros_ec_cmd_xfer() under the EC's mutex so that other
> >> commands (e.g. keyboard, I2C passtru) aren't issued to the EC while
> >> it is working on the in-progress command.
> >> 
> >> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> >> Reviewed-by: Simon Glass <sjg@chromium.org>
> >> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> >> ---
> >> 
> >> Changes since v1:
> >>  - The *xfer() calls don't modify the passed cros_ec_command so there is
> >>    no need to populate it inside the for loop. Suggested by Lee Jones.
> >> ---
> >>  drivers/mfd/cros_ec.c | 34 +++++++++++++++++++++++++++++++++-
> >>  1 file changed, 33 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> >> index c53804a..cd0c93c 100644
> >> --- a/drivers/mfd/cros_ec.c
> >> +++ b/drivers/mfd/cros_ec.c

[...]

> >>  	mutex_lock(&ec_dev->lock);
> >>  	ret = ec_dev->cmd_xfer(ec_dev, msg);
> >> +	if (ret == -EAGAIN && msg->result == EC_RES_IN_PROGRESS) {
> > 
> > Is there ever a time where (ret == -EAGAIN) but (msg->result !=
> > EC_RES_IN_PROGRESS) [note the !=].  And/or is there ever a time where
> > (msg->result == EC_RES_IN_PROGRESS) but (ret != -EAGAIN) [again, not
> > the !=].
> > 
> > Another way of explaining it.  Can ret be anything other than -EAGAIN
> > when the result is EC_RES_IN_PROGRESS.  And can the result be anything
> > other than EC_RES_IN_PROGRESS when ret is -EAGAIN?

[...]

> But after looking at all the cros_ec transport drivers it seems to be safe to
> just check if result is EC_RES_IN_PROGRESS instead of checking also if ret is
> -EAGAIN since (at least on all the current transport drivers) the former
> implies the later.

That's exactly what I was getting at.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 5/7] mfd: cros_ec: wait for completion of commands that return IN_PROGRESS

[Andrew Bresticker]

On Mon, Sep 8, 2014 at 4:39 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> Hello Lee,
>
> On 09/04/2014 10:34 AM, Lee Jones wrote:
>> On Mon, 25 Aug 2014, Javier Martinez Canillas wrote:
>>> From: Andrew Bresticker <abrestic@chromium.org>
>>>
>>> When an EC command returns EC_RES_IN_PROGRESS, we need to query
>>> the state of the EC until it indicates that it is no longer busy.
>>> Do this in cros_ec_cmd_xfer() under the EC's mutex so that other
>>> commands (e.g. keyboard, I2C passtru) aren't issued to the EC while
>>> it is working on the in-progress command.
>>>
>>> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>>> ---
>>>
>>> Changes since v1:
>>>  - The *xfer() calls don't modify the passed cros_ec_command so there is
>>>    no need to populate it inside the for loop. Suggested by Lee Jones.
>>> ---
>>>  drivers/mfd/cros_ec.c | 34 +++++++++++++++++++++++++++++++++-
>>>  1 file changed, 33 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>>> index c53804a..cd0c93c 100644
>>> --- a/drivers/mfd/cros_ec.c
>>> +++ b/drivers/mfd/cros_ec.c
>>> @@ -23,6 +23,10 @@
>>>  #include <linux/mfd/core.h>
>>>  #include <linux/mfd/cros_ec.h>
>>>  #include <linux/mfd/cros_ec_commands.h>
>>> +#include <linux/delay.h>
>>> +
>>> +#define EC_COMMAND_RETRIES  50
>>> +#define EC_RETRY_DELAY_MS   10
>>
>> Where did these values come from?
>>
>
> These patches were taken from the ChromeOS 3.8 kernel so I don't really know
> why these values were chosen. I'll let Andrew or one of the ChromiumOS folks
> to answer this question.

These are the values flashrom used when retrying commands.