On Wed, Jul 4, 2012 at 3:49 PM, Jean Delvare <khali@xxxxxxxxxxxx> wrote:
> Hi Daniel,
>
> Thanks again for the continued feedback.
>
> On Wed, 4 Jul 2012 14:12:26 +0800, Daniel Kurtz wrote:
>> On Tue, Jul 3, 2012 at 9:39 PM, Jean Delvare <khali@xxxxxxxxxxxx> wrote:
>> > On Tue, 3 Jul 2012 17:50:13 +0800, Daniel Kurtz wrote:
>> > > On Tue, Jul 3, 2012 at 4:19 PM, Jean Delvare <khali@xxxxxxxxxxxx> wrote:
>> > > > -/* wait for INTR bit as advised by Intel */
>> > > > -static void i801_wait_intr(struct i801_priv *priv)
>> > > > +/* Wait for BUSY being cleared and either INTR or an error flag being set */
>> > > > +static int i801_wait_intr(struct i801_priv *priv)
>> > > > {
>> > > > int timeout = 0;
>> > > > int status;
>> > > >
>> > > > + /* We will always wait for a fraction of a second! */
>> > > > do {
>> > > > usleep_range(250, 500);
>> > > > status = inb_p(SMBHSTSTS(priv));
>> > > > - } while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES));
>> > > > + } while (((status & SMBHSTSTS_HOST_BUSY) ||
>> > > > + !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
>> > > > + (timeout++ < MAX_RETRIES));
>> > >
>> > > From your experiments, I think you saw that the final status (ERROR |
>> > > INTR) will be set before or at the same time as HOST_BUSY -> 0, so the
>> > > loop need only check HOST_BUSY, and then just return the final status
>> > > bits of the transfer (ie, ERROR_FLAGS | INTR). So, the function name
>> > > could be something like:
>> > >
>> > > i801_wait_xfer_status()
>> >
>> > You're right. But I did these tests out of curiosity, maybe I shouldn't
>> > have published the results, because now I feel reluctant to use the
>> > results. The reason is that this is not specified in the datasheet, so
>> > I have no guarantee that the same holds for all chips or all
>> > transaction types.
>> >
>> > Furthermore, I see no reason to not play it safe. What costs time and
>> > CPU here is the sleep and the inb_p. The extra test is pretty cheap
>> > IMHO.
>>
>> Sorry, I forgot to make my actual point last time. This loop actually
>> terminates *early* now, since it ends when an error occur, without
>> waiting for the transaction to finish.
>
> This wasn't my intent, and also not what my code is doing, unless I am
> reading it wrong.
>
>> I think it would be safer to
>> wait for the transaction to finish (HOST_BUSY -> 0) first, and then
>> return the status.
>
> Which is what I'm doing, as far as I can see. Leaving the timeout case
> aside for a moment, the loop keeps running if the following condition
> is met:
>
> (status & SMBHSTSTS_HOST_BUSY) || !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))
>
> Which, per De Morgan's laws [1], means the loop _exit_ condition is:
>
> !(status & SMBHSTSTS_HOST_BUSY) && (status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))
>
> That is, we only exit the loop if the BUSY bit is cleared _and_ either
> INTR or an error flag is set. In other words this is a late exit. Are
> you OK with it now, or am I really missing something?
Nope. You are right, I was reading the code wrong.
We are in complete agreement about what it should be doing, and you
have convinced me that it is in fact doing it.
>
> [1] http://en.wikipedia.org/wiki/De_Morgan%27s_laws
>
>> > (...)
>> > @@ -272,7 +272,7 @@ static int i801_wait_intr(struct i801_pr
>>
>> "i801_wait_intr()" isn't such a good name anymore. I recommend
>> i801_wait_xfer_status(), or i801_wait_host_idle().
>
> i801_wait_intr() isn't such a bad name IMHO, we are still ideally hoping
> that INTR will be set, as this means the transaction will be
> successful. The name has the advantage of being symmetric with
> i801_wait_byte_done(), so it's clear which one of the status bits we
> are expecting.
>
> "i801_wait_host_idle" would be a good name too, but the actual name
> doesn't really matter anyway, as the next patch merges both functions
> into a single one. I had written this patch before you posted your
> proposed changes, but it can still be applied on top of these (with the
> usual context adjustment.) Unless you really don't like it? If so,
> please tell me why.
>
>> > dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
>>
>> "Timed out waiting for transaction to complete!\n"
>
> My next patch already changes this to:
>
> dev_err(&priv->pci_dev->dev,
> "Timeout waiting for %02x! (%02x)\n", wait_set, status);
>
> My message is more generic because the function is now generic, but the
> information provided is more or less the same. If you think wording can
> be improved, just let me know.
I'm really not sure this function is that necessary. The code seems
more confusing now with the two waits combined into a single function.
Keeping them separate also means you don't have to do extra work like
passing "wait_cleared = 0" and making sure not to clear BYTE_DONE in
the BYTE_DONE case.
>
> ---
> drivers/i2c/busses/i2c-i801.c | 50 ++++++++++++++++-------------------------
> 1 file changed, 20 insertions(+), 30 deletions(-)
>
> --- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-i801.c 2012-07-03 16:49:42.000000000 +0200
> +++ linux-3.5-rc5/drivers/i2c/busses/i2c-i801.c 2012-07-03 17:10:37.062356531 +0200
> @@ -254,29 +254,16 @@ static int i801_check_post(struct i801_p
> return result;
> }
>
> -/* Wait for BUSY being cleared and either INTR or an error flag being set */
> -static int i801_wait_intr(struct i801_priv *priv)
> -{
> - int timeout = 0;
> - int status;
> -
> - /* We will always wait for a fraction of a second! */
> - do {
> - usleep_range(250, 500);
> - status = inb_p(SMBHSTSTS(priv));
> - } while (((status & SMBHSTSTS_HOST_BUSY) ||
> - !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
> - (timeout++ < MAX_RETRIES));
> -
> - if (timeout > MAX_RETRIES) {
> - dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
> - return -ETIMEDOUT;
> - }
> - return status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR);
> -}
> -
> -/* Wait for either BYTE_DONE or an error flag being set */
> -static int i801_wait_byte_done(struct i801_priv *priv)
> +/*
> + * Wait for some status register bits being cleared and others being set.
> + * Typical usage:
> + * - Wait for BUSY being cleared and either INTR or an error flag being set
> + * (end of transaction)
> + * - Wait for either BYTE_DONE or an error flag being set (step of
> + * byte-by-byte block transaction)
> + * Return the status flags which should be cleared automatically.
> + */
> +static int i801_wait(struct i801_priv *priv, int wait_cleared, int wait_set)
> {
> int timeout = 0;
> int status;
> @@ -285,14 +272,17 @@ static int i801_wait_byte_done(struct i8
> do {
> usleep_range(250, 500);
> status = inb_p(SMBHSTSTS(priv));
> - } while (!(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE)) &&
> + } while (((status & wait_cleared) ||
> + !(status & (STATUS_ERROR_FLAGS | wait_set))) &&
> (timeout++ < MAX_RETRIES));
>
> - if (timeout > MAX_RETRIES) {
> - dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
> + if (unlikely(timeout > MAX_RETRIES)) {
> + dev_err(&priv->pci_dev->dev,
> + "Timeout waiting for %02x! (%02x)\n", wait_set, status);
> return -ETIMEDOUT;
> }
> - return status & STATUS_ERROR_FLAGS;
> + status &= ~SMBHSTSTS_BYTE_DONE; /* Must be cleared explicitly */
> + return status & (STATUS_ERROR_FLAGS | wait_set);
> }
>
> static int i801_transaction(struct i801_priv *priv, int xact)
> @@ -308,7 +298,7 @@ static int i801_transaction(struct i801_
> * SMBSCMD are passed in xact */
> outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
>
> - status = i801_wait_intr(priv);
> + status = i801_wait(priv, SMBHSTSTS_HOST_BUSY, SMBHSTSTS_INTR);
> return i801_check_post(priv, status);
> }
>
> @@ -387,7 +377,7 @@ static int i801_block_transaction_byte_b
> outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
> SMBHSTCNT(priv));
>
> - status = i801_wait_byte_done(priv);
> + status = i801_wait(priv, 0, SMBHSTSTS_BYTE_DONE);
> if (status)
> goto exit;
>
> @@ -419,7 +409,7 @@ static int i801_block_transaction_byte_b
> outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
> }
>
> - status = i801_wait_intr(priv);
> + status = i801_wait(priv, SMBHSTSTS_HOST_BUSY, SMBHSTSTS_INTR);
> exit:
> return i801_check_post(priv, status);
> }
>
> --
> Jean Delvare
--
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
