[PATCH v3 0/6] Second batch of cleanups for cros_ec

[PATCH v3 0/6] Second batch of cleanups for cros_ec

[Javier Martinez Canillas]

Hello,

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 third version of the patch series that addresses
issues pointed out by Lee Jones on v2 [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 (1):
  mfd: cros_ec: Instantiate sub-devices from device tree

 drivers/i2c/busses/i2c-cros-ec-tunnel.c |  5 ++-
 drivers/input/keyboard/cros_ec_keyb.c   |  2 +-
 drivers/mfd/cros_ec.c                   | 74 +++++++++++++++++++++++++--------
 drivers/mfd/cros_ec_spi.c               | 20 ++++-----
 include/linux/mfd/cros_ec.h             | 24 ++++++++---
 5 files changed, 88 insertions(+), 37 deletions(-)

Patches #1, #2 and #6 can be applied independently but patches #3, #4 and #5
rely on the previous one so they should be applied together and in that order.

Best regards,
Javier

[0]: https://www.mail-archive.com/linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg11385.html
[1]: https://www.mail-archive.com/linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg35858.html

[PATCH v3 1/6] 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.1.0

[PATCH v3 2/6] i2c: i2c-cros-ec-tunnel: Set retries to 3

[Javier Martinez Canillas]

From: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Acked-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
---
 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.1.0

[PATCH v3 3/6] 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 8c09b3e..462bfcb 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -157,7 +157,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.1.0

[PATCH v3 4/6] 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 v2: None

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

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

[Javier Martinez Canillas]

From: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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.

The delay in milliseconds and the number of retries are the values
that were used by the flashrom tool when retrying commands.

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

Changes since v2:
 - Explain in the commit message from where the delay and retry values come from.
   Commented by Andrew Bresticker.
 - Move the needed definitions inside the if block. Suggested by Lee Jones.
 - Only check if result is EC_RES_IN_PROGRESS instead of checking also if ret is
   -EAGAIN since the former implies the later. Suggested by Lee Jones.
 - Use usleep_range() instead of msleep() since doesn't handle values between 1~20.
   Suggested by Lee Jones.

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, 34 insertions(+)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index c53804a..af2fadf 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)
@@ -69,6 +73,36 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 
 	mutex_lock(&ec_dev->lock);
 	ret = ec_dev->cmd_xfer(ec_dev, msg);
+	if (msg->result == EC_RES_IN_PROGRESS) {
+		int i;
+		struct cros_ec_command status_msg;
+		struct ec_response_get_comms_status status;
+
+		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);
+
+		/*
+		 * Query the EC's status until it's no longer busy or
+		 * we encounter an error.
+		 */
+		for (i = 0; i < EC_COMMAND_RETRIES; i++) {
+			usleep_range(EC_RETRY_DELAY_MS, EC_RETRY_DELAY_MS + 1);
+
+			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.1.0

[PATCH v3 6/6] 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 v2:
 - Drop if guards since of_node is unconditionally built. Suggested by Lee Jones
 - Drop unneeded comment about of_platform_populate(). Suggested by Lee Jones.
 - Fix error message if of_platform_populate() fails. Suggested by Lee Jones.

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 | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index 751af0b..7c533d2 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>
@@ -109,22 +110,10 @@ 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;
+	struct device_node *node = dev->of_node;
 	int err = 0;
 
 	if (ec_dev->din_size) {
@@ -140,12 +129,12 @@ 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;
+	if (node) {
+		err = of_platform_populate(node, NULL, NULL, dev);
+		if (err) {
+			dev_err(dev, "Failed to register subordinate devices");
+			return err;
+		}
 	}
 
 	dev_info(dev, "Chrome EC device registered\n");
-- 
2.1.0

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

[Lee Jones]

On Thu, 11 Sep 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.
> 
> The delay in milliseconds and the number of retries are the values
> that were used by the flashrom tool when retrying commands.
> 
> 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 v2:
>  - Explain in the commit message from where the delay and retry values come from.
>    Commented by Andrew Bresticker.
>  - Move the needed definitions inside the if block. Suggested by Lee Jones.
>  - Only check if result is EC_RES_IN_PROGRESS instead of checking also if ret is
>    -EAGAIN since the former implies the later. Suggested by Lee Jones.
>  - Use usleep_range() instead of msleep() since doesn't handle values between 1~20.
>    Suggested by Lee Jones.
> 
> 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, 34 insertions(+)
> 
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index c53804a..af2fadf 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)
> @@ -69,6 +73,36 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
>  
>  	mutex_lock(&ec_dev->lock);
>  	ret = ec_dev->cmd_xfer(ec_dev, msg);
> +	if (msg->result == EC_RES_IN_PROGRESS) {
> +		int i;
> +		struct cros_ec_command status_msg;
> +		struct ec_response_get_comms_status status;
> +
> +		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);
> +
> +		/*
> +		 * Query the EC's status until it's no longer busy or
> +		 * we encounter an error.
> +		 */
> +		for (i = 0; i < EC_COMMAND_RETRIES; i++) {
> +			usleep_range(EC_RETRY_DELAY_MS, EC_RETRY_DELAY_MS + 1);

Remove the EC_RETRY_DELAY_MS define and place the values in raw.

You're now sleeping for 10us.  Did you test the changes?

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

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

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

[Lee Jones]

On Thu, 11 Sep 2014, Javier Martinez Canillas wrote:

> From: Todd Broch <tbroch-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8rSCDK34cm6iQ@public.gmane.org.uk>
> ---
> 
> Changes since v2:
>  - Drop if guards since of_node is unconditionally built. Suggested by Lee Jones
>  - Drop unneeded comment about of_platform_populate(). Suggested by Lee Jones.
>  - Fix error message if of_platform_populate() fails. Suggested by Lee Jones.
> 
> 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 | 27 ++++++++-------------------
>  1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index 751af0b..7c533d2 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>
> @@ -109,22 +110,10 @@ 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;
> +	struct device_node *node = dev->of_node;
>  	int err = 0;
>  
>  	if (ec_dev->din_size) {
> @@ -140,12 +129,12 @@ 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;

So these devices will only ever probe with DT now ...

> +	if (node) {

So it would be wrong for dev->of_node not to be populated.

> +		err = of_platform_populate(node, NULL, NULL, dev);
> +		if (err) {
> +			dev_err(dev, "Failed to register subordinate devices");
> +			return err;
> +		}
>  	}
>  
>  	dev_info(dev, "Chrome EC device registered\n");

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

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

[Javier Martinez Canillas]

Hello Lee,

Thanks a lot for your feedback.

On 09/17/2014 06:23 PM, Lee Jones wrote:
>>  
>>  	mutex_lock(&ec_dev->lock);
>>  	ret = ec_dev->cmd_xfer(ec_dev, msg);
>> +	if (msg->result == EC_RES_IN_PROGRESS) {
>> +		int i;
>> +		struct cros_ec_command status_msg;
>> +		struct ec_response_get_comms_status status;
>> +
>> +		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);
>> +
>> +		/*
>> +		 * Query the EC's status until it's no longer busy or
>> +		 * we encounter an error.
>> +		 */
>> +		for (i = 0; i < EC_COMMAND_RETRIES; i++) {
>> +			usleep_range(EC_RETRY_DELAY_MS, EC_RETRY_DELAY_MS + 1);
> 
> Remove the EC_RETRY_DELAY_MS define and place the values in raw.
>

Ok, will do.

> You're now sleeping for 10us.  Did you test the changes?
> 

Duh, I must had been sleepy since I thought about changing the define but I
completely missed... which proves your point that raw values are more explicit
than using a define here.

Yes, I'm testing the changes and making sure that it does not add any
regression but I was not able to reproduce the case when an EC command result
is IN_PROGRESS. I'll investigate further on how to properly test that branch
before posting v4.

Best regards,
Javier

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

[Javier Martinez Canillas]

Hello Lee,

On 09/17/2014 06:31 PM, 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;
>> +	struct device_node *node = dev->of_node;
>>  	int err = 0;
>>  
>>  	if (ec_dev->din_size) {
>> @@ -140,12 +129,12 @@ 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;
> 
> So these devices will only ever probe with DT now ...
>

Well, these are preparatory patches to reduce the delta between upstream and
the downstream so the missing functionality could be added. One of the missing
drivers is the cros_ec_dev.c [0] which allows user-space to access the
ChromeOS Embedded Controller using a virtual character device (/dev/cros_ec).

Since that is a virtual device, it does not fit on the DT which only describes
hw and also is used on x86 machines so that subdevice is still probed using
mfd_add_devices() and the mfd_cells array is not empty in the downstream
cros_ec driver [1].

That's why I didn't just made the cros_ec MFD to depend on OF, since I didn't
want to diverge too much from the downstream driver because the idea of the
series was to reduce the difference in order to add the missing bits on top.

>> +	if (node) {
> 
> So it would be wrong for dev->of_node not to be populated.
> 

As explained above, DT, non-DT and x86 platforms instantiate the cros-ec-dev
cells but DT platforms can define other child nodes. But I can remove the
conditional if you want and reintroduce it once cros-ec-dev support is added.

>> +		err = of_platform_populate(node, NULL, NULL, dev);
>> +		if (err) {
>> +			dev_err(dev, "Failed to register subordinate devices");
>> +			return err;
>> +		}
>>  	}
>>  
>>  	dev_info(dev, "Chrome EC device registered\n");
> 

Best regards,
Javier

[0]:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/mfd/cros_ec_dev.c
[1]:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/mfd/cros_ec.c#93

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

[Javier Martinez Canillas]

Hello Lee,

On 09/18/2014 09:49 AM, Javier Martinez Canillas wrote:
>>>  int cros_ec_register(struct cros_ec_device *ec_dev)
>>>  {
>>>  	struct device *dev = ec_dev->dev;
>>> +	struct device_node *node = dev->of_node;
>>>  	int err = 0;
>>>  
>>>  	if (ec_dev->din_size) {
>>> @@ -140,12 +129,12 @@ 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;
>> 
>> So these devices will only ever probe with DT now ...
>>
> 
> Well, these are preparatory patches to reduce the delta between upstream and
> the downstream so the missing functionality could be added. One of the missing
> drivers is the cros_ec_dev.c [0] which allows user-space to access the
> ChromeOS Embedded Controller using a virtual character device (/dev/cros_ec).
> 
> Since that is a virtual device, it does not fit on the DT which only describes
> hw and also is used on x86 machines so that subdevice is still probed using
> mfd_add_devices() and the mfd_cells array is not empty in the downstream
> cros_ec driver [1].
> 

I posted a new revision (v4) of the series but dropped this patch since this
change probably makes more sense once the cros_ec_dev driver lands. Since then
the mfd cells array will not be empty and not all subdevices will be probed
with of_platform_populate().

Best regards,
Javier

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

[Lee Jones]

On Thu, 18 Sep 2014, Javier Martinez Canillas wrote:

> Hello Lee,
> 
> On 09/17/2014 06:31 PM, 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;
> >> +	struct device_node *node = dev->of_node;
> >>  	int err = 0;
> >>  
> >>  	if (ec_dev->din_size) {
> >> @@ -140,12 +129,12 @@ 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;
> > 
> > So these devices will only ever probe with DT now ...
> >
> 
> Well, these are preparatory patches to reduce the delta between upstream and
> the downstream so the missing functionality could be added. One of the missing
> drivers is the cros_ec_dev.c [0] which allows user-space to access the
> ChromeOS Embedded Controller using a virtual character device (/dev/cros_ec).
> 
> Since that is a virtual device, it does not fit on the DT which only describes
> hw and also is used on x86 machines so that subdevice is still probed using
> mfd_add_devices() and the mfd_cells array is not empty in the downstream
> cros_ec driver [1].
> 
> That's why I didn't just made the cros_ec MFD to depend on OF, since I didn't
> want to diverge too much from the downstream driver because the idea of the
> series was to reduce the difference in order to add the missing bits on top.
> 
> >> +	if (node) {
> > 
> > So it would be wrong for dev->of_node not to be populated.
> > 
> 
> As explained above, DT, non-DT and x86 platforms instantiate the cros-ec-dev
> cells but DT platforms can define other child nodes. But I can remove the
> conditional if you want and reintroduce it once cros-ec-dev support is added.

Yes, that makes sense.  I only care about doing what's right for
Mainline, so if it doesn't make sense here, then we shouldn't be doing
it.

> >> +		err = of_platform_populate(node, NULL, NULL, dev);
> >> +		if (err) {
> >> +			dev_err(dev, "Failed to register subordinate devices");
> >> +			return err;
> >> +		}
> >>  	}
> >>  
> >>  	dev_info(dev, "Chrome EC device registered\n");
> > 
> 
> Best regards,
> Javier
> 
> [0]:
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/mfd/cros_ec_dev.c
> [1]:
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/mfd/cros_ec.c#93

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

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

[Javier Martinez Canillas]

Hello Lee,

On 09/19/2014 02:15 AM, Lee Jones wrote:
>> 
>> As explained above, DT, non-DT and x86 platforms instantiate the cros-ec-dev
>> cells but DT platforms can define other child nodes. But I can remove the
>> conditional if you want and reintroduce it once cros-ec-dev support is added.
> 
> Yes, that makes sense.  I only care about doing what's right for
> Mainline, so if it doesn't make sense here, then we shouldn't be doing
> it.
> 

Understood, that's why I dropped this since as explained earlier, it makes more
sense to include this change once the cros_ec_dev driver is pushed to upstream.

The last version of the series (v4) I posted a week ago [0], does not include
this patch so I hope the series can be picked for 3.18 since most of the patches
were already acked by you.

Thanks a lot and best regards,
Javier

[0]: https://www.mail-archive.com/linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg36770.html

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

[Lee Jones]

On Thu, 25 Sep 2014, Javier Martinez Canillas wrote:

> Hello Lee,
> 
> On 09/19/2014 02:15 AM, Lee Jones wrote:
> >> 
> >> As explained above, DT, non-DT and x86 platforms instantiate the cros-ec-dev
> >> cells but DT platforms can define other child nodes. But I can remove the
> >> conditional if you want and reintroduce it once cros-ec-dev support is added.
> > 
> > Yes, that makes sense.  I only care about doing what's right for
> > Mainline, so if it doesn't make sense here, then we shouldn't be doing
> > it.
> > 
> 
> Understood, that's why I dropped this since as explained earlier, it makes more
> sense to include this change once the cros_ec_dev driver is pushed to upstream.
> 
> The last version of the series (v4) I posted a week ago [0], does not include
> this patch so I hope the series can be picked for 3.18 since most of the patches
> were already acked by you.

Unfortunately this is going to have to wait until v3.19 now, as Linus
is closing the -rcs early.  I have subsequently applied my last
patch to the MFD tree for v3.18.  It will be good for these patches to
get some soak testing in -next anyway, rather than rush them in.

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

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

[Javier Martinez Canillas]

On 09/25/2014 04:48 PM, Lee Jones wrote:
> On Thu, 25 Sep 2014, Javier Martinez Canillas wrote:
>> 
>> The last version of the series (v4) I posted a week ago [0], does not include
>> this patch so I hope the series can be picked for 3.18 since most of the patches
>> were already acked by you.
> 
> Unfortunately this is going to have to wait until v3.19 now, as Linus
> is closing the -rcs early.  I have subsequently applied my last
> patch to the MFD tree for v3.18.  It will be good for these patches to
> get some soak testing in -next anyway, rather than rush them in.
> 

No worries. Does that mean that you are going to pick this series after
the 3.18 merge window or should I resend this once 3.18-rc1 is released?

Best regards,
Javier

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

[Lee Jones]

On Thu, 25 Sep 2014, Javier Martinez Canillas wrote:

> On 09/25/2014 04:48 PM, Lee Jones wrote:
> > On Thu, 25 Sep 2014, Javier Martinez Canillas wrote:
> >> 
> >> The last version of the series (v4) I posted a week ago [0], does not include
> >> this patch so I hope the series can be picked for 3.18 since most of the patches
> >> were already acked by you.
> > 
> > Unfortunately this is going to have to wait until v3.19 now, as Linus
> > is closing the -rcs early.  I have subsequently applied my last
> > patch to the MFD tree for v3.18.  It will be good for these patches to
> > get some soak testing in -next anyway, rather than rush them in.
> > 
> 
> No worries. Does that mean that you are going to pick this series after
> the 3.18 merge window or should I resend this once 3.18-rc1 is released?

If you can re-send with any Acks you've been given (no new ones) after
-rc1, that would be helpful.

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

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

[Javier Martinez Canillas]

On 09/26/2014 09:21 AM, Lee Jones wrote:
> On Thu, 25 Sep 2014, Javier Martinez Canillas wrote:
>> > Unfortunately this is going to have to wait until v3.19 now, as Linus
>> > is closing the -rcs early.  I have subsequently applied my last
>> > patch to the MFD tree for v3.18.  It will be good for these patches to
>> > get some soak testing in -next anyway, rather than rush them in.
>> > 
>> 
>> No worries. Does that mean that you are going to pick this series after
>> the 3.18 merge window or should I resend this once 3.18-rc1 is released?
> 
> If you can re-send with any Acks you've been given (no new ones) after
> -rc1, that would be helpful.
> 

Will do it, thanks a lot for your help.

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

[Javier Martinez Canillas]

Hello Lee,

On 09/25/2014 04:48 PM, Lee Jones wrote:
> On Thu, 25 Sep 2014, Javier Martinez Canillas wrote:
>> 
>> Understood, that's why I dropped this since as explained earlier, it makes more
>> sense to include this change once the cros_ec_dev driver is pushed to upstream.
>> 
>> The last version of the series (v4) I posted a week ago [0], does not include
>> this patch so I hope the series can be picked for 3.18 since most of the patches
>> were already acked by you.
> 
> Unfortunately this is going to have to wait until v3.19 now, as Linus
> is closing the -rcs early.  I have subsequently applied my last
> patch to the MFD tree for v3.18.  It will be good for these patches to
> get some soak testing in -next anyway, rather than rush them in.
> 

I don't want to be annoying but I was just wondering if now that Linus has
delayed the 3.18 merge window for two weeks [1], you will consider picking
this series. Most of the patches have been acked by you a couple of weeks
ago anyways and I dropped $subject from the last version of the series [2],
which was the patch you had issues with.

It would be awesome if this can make it to 3.18 so we can start adding the
missing functionality and target it to 3.19.

Thanks a lot and best regards,
Javier

[1]: http://thread.gmane.org/gmane.linux.kernel/1797057
[2]: https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg36770.html