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