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

Hello Lee,

This is a resend of a patch series originally sent [0] almost a
month ago (July, 28). The series add a batch of cleanups patches
for the mfd cros_ec driver and its subdevices drivers. The first
batch of cleanups was posted by Doug Anderson [1] and have already
been merged. The patches were taken from the ChromeOS 3.8 kernel
and after this series, no cleanups patches for cros_ec are left.
The remaining commits add support not yet available in mainline.

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   | 89 +++++++++++++++++----------------
 drivers/mfd/cros_ec.c                   | 88 ++++++++++++++++++++++++++++----
 drivers/mfd/cros_ec_spi.c               | 20 ++++----
 include/linux/mfd/cros_ec.h             | 24 ++++++---
 5 files changed, 154 insertions(+), 72 deletions(-)

There were no changes on this resend, just picked Acked-by and
Tested-by tags and also added my own Signed-off-by tag to all
the patches as suggested by Andreas Färber even when I just
picked them from downstream and rebased on top of linux-next.

The patches should be merged together which means that they
should go through your mfd tree once the relevant acks are
obtained.

Best regards,
Javier

[0]: https://www.mail-archive.com/linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg11385.html
[1]: https://lkml.org/lkml/2014/6/16/681

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 <redacted>
Reviewed-by: Vadim Bendebury <redacted>
Signed-off-by: Javier Martinez Canillas <redacted>
---
 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

From: Andrew Bresticker <redacted>

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

Signed-off-by: Andrew Bresticker <redacted>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Javier Martinez Canillas <redacted>
---
 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

From: Andrew Bresticker <redacted>

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 <redacted>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Javier Martinez Canillas <redacted>
---
 drivers/mfd/cros_ec.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index c53804a..634c434 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,39 @@ 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;
 
 	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.
+		 */
+		for (i = 0; i < EC_COMMAND_RETRIES; i++) {
+			struct cros_ec_command status_msg;
+			struct ec_response_get_comms_status status;
+
+			msleep(EC_RETRY_DELAY_MS);
+
+			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);
+
+			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

Javier,

On Wed, Aug 20, 2014 at 5:13 AM, Javier Martinez Canillas
[off-list ref] wrote:

From: Andrew Bresticker <redacted>

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

Slight nit that the LPC driver doesn't exist upstream.  This is in
prep for adding the LPC driver, though.

Signed-off-by: Andrew Bresticker <redacted>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Javier Martinez Canillas <redacted>
---
 drivers/mfd/cros_ec.c     | 10 +++++++++-
 drivers/mfd/cros_ec_spi.c | 11 -----------
 2 files changed, 9 insertions(+), 12 deletions(-)

After comment nitfix:

Reviewed-by: Doug Anderson <dianders@chromium.org>

Hello Doug,

On 08/21/2014 12:36 AM, Doug Anderson wrote:

Javier,

On Wed, Aug 20, 2014 at 5:13 AM, Javier Martinez Canillas
[off-list ref] wrote:

quoted

From: Andrew Bresticker <redacted>

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

Slight nit that the LPC driver doesn't exist upstream.  This is in
prep for adding the LPC driver, though.

Right, the downstream commit was also touching the LPC driver and I
stripped that part but forget to update the commit message, sorry about
that. I'll fix it and do a re-spin.

quoted

Signed-off-by: Andrew Bresticker <redacted>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Javier Martinez Canillas <redacted>
---
 drivers/mfd/cros_ec.c     | 10 +++++++++-
 drivers/mfd/cros_ec_spi.c | 11 -----------
 2 files changed, 9 insertions(+), 12 deletions(-)

After comment nitfix:

Reviewed-by: Doug Anderson <dianders@chromium.org>

Thanks and best regards,
Javier

On Wed, 20 Aug 2014, Javier Martinez Canillas wrote:

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.

Are you planning on doing that?

* This same logic probably needs to be applied to the i2c driver.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Randall Spangler <redacted>
Reviewed-by: Vadim Bendebury <redacted>
Signed-off-by: Javier Martinez Canillas <redacted>
---
 drivers/mfd/cros_ec_spi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

I'm willing to accept this as a stand-in.

Acked-by: Lee Jones <redacted>

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

Hello Lee,

On 08/21/2014 03:37 PM, Lee Jones wrote:

On Wed, 20 Aug 2014, Javier Martinez Canillas wrote:

quoted

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.

Are you planning on doing that?

Yes, I'll add to my TO-DO list to look how better solve this after the
remaining functionality that is present in downstream but is still not in
mainline gets merged.

quoted

* This same logic probably needs to be applied to the i2c driver.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Randall Spangler <redacted>
Reviewed-by: Vadim Bendebury <redacted>
Signed-off-by: Javier Martinez Canillas <redacted>
---
 drivers/mfd/cros_ec_spi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

I'm willing to accept this as a stand-in.

Thanks.

Acked-by: Lee Jones <redacted>

Best regards,
Javier

On Wed, 20 Aug 2014, Javier Martinez Canillas wrote:

From: Andrew Bresticker <redacted>

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

Signed-off-by: Andrew Bresticker <redacted>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Javier Martinez Canillas <redacted>
---
 drivers/mfd/cros_ec.c     | 10 +++++++++-
 drivers/mfd/cros_ec_spi.c | 11 -----------
 2 files changed, 9 insertions(+), 12 deletions(-)

Acked-by: Lee Jones <redacted>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html