On Mon, 25 Aug 2014, Javier Martinez Canillas wrote:
> From: Andrew Bresticker <abrestic@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Reviewed-by: Simon Glass <sjg@xxxxxxxxxxxx>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@xxxxxxxxxxxxxxx>
> ---
>
> 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?
> 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().
> 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?
> + /*
> + * 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.
> + ret = ec_dev->cmd_xfer(ec_dev, &status_msg);
> + if (ret < 0)
> + break;
What does a ret of >0 mean?
> + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
