[PATCH] i2c: i801: Improve status polling

[PATCH] i2c: i801: Improve status polling

[Heiner Kallweit]

Polling uses the same timeout as irq mode: 400 * 500us = 200ms = HZ / 5.
So let's use the adapter->timeout value also for polling. This has the
advantage that userspace can control the timeout value for polling as
well. In addition change the code to make it better readable.
Last but not least remove the timeout debug messages. Calls to both
functions are followed by a call to i801_check_post() that will print
an error message in case of timeout.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 36 +++++++++++++----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index f6d7866f1..2c6e84108 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -156,9 +156,6 @@
 #define SMBAUXCTL_CRC		BIT(0)
 #define SMBAUXCTL_E32B		BIT(1)
 
-/* Other settings */
-#define MAX_RETRIES		400
-
 /* I801 command constants */
 #define I801_QUICK		0x00
 #define I801_BYTE		0x04
@@ -447,42 +444,35 @@ static int i801_check_post(struct i801_priv *priv, int status)
 /* 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;
+	unsigned long timeout = jiffies + priv->adapter.timeout;
+	int status, busy;
 
-	/* 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));
+		busy = status & SMBHSTSTS_HOST_BUSY;
+		status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
+		if (!busy && status)
+			return status;
+	} while (time_is_after_eq_jiffies(timeout));
 
-	if (timeout > MAX_RETRIES) {
-		dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
-		return -ETIMEDOUT;
-	}
-	return status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR);
+	return -ETIMEDOUT;
 }
 
 /* Wait for either BYTE_DONE or an error flag being set */
 static int i801_wait_byte_done(struct i801_priv *priv)
 {
-	int timeout = 0;
+	unsigned long timeout = jiffies + priv->adapter.timeout;
 	int status;
 
-	/* We will always wait for a fraction of a second! */
 	do {
 		usleep_range(250, 500);
 		status = inb_p(SMBHSTSTS(priv));
-	} while (!(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE)) &&
-		 (timeout++ < MAX_RETRIES));
+		if (status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE))
+			return status & STATUS_ERROR_FLAGS;
+	} while (time_is_after_eq_jiffies(timeout));
 
-	if (timeout > MAX_RETRIES) {
-		dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
-		return -ETIMEDOUT;
-	}
-	return status & STATUS_ERROR_FLAGS;
+	return -ETIMEDOUT;
 }
 
 static int i801_transaction(struct i801_priv *priv, int xact)
-- 
2.31.1

Re: [PATCH] i2c: i801: Improve status polling

[Jean Delvare]

Hi Heiner,

On Tue, 25 May 2021 21:59:05 +0200, Heiner Kallweit wrote:
> Polling uses the same timeout as irq mode: 400 * 500us = 200ms = HZ / 5.
> So let's use the adapter->timeout value also for polling. This has the
> advantage that userspace can control the timeout value for polling as
> well. In addition change the code to make it better readable.
> Last but not least remove the timeout debug messages. Calls to both
> functions are followed by a call to i801_check_post() that will print
> an error message in case of timeout.

I think that the intent was to differentiate between regular polling
and BYTE_DONE polling. But I agree it's not really important, as the
caller probably already knows which type of transaction failed (simple
or block).

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 36 +++++++++++++----------------------
>  1 file changed, 13 insertions(+), 23 deletions(-)
> (...)

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Tested-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Improve status polling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

On Tue, May 25, 2021 at 09:59:05PM +0200, Heiner Kallweit wrote:
> Polling uses the same timeout as irq mode: 400 * 500us = 200ms = HZ / 5.
> So let's use the adapter->timeout value also for polling. This has the
> advantage that userspace can control the timeout value for polling as
> well. In addition change the code to make it better readable.
> Last but not least remove the timeout debug messages. Calls to both
> functions are followed by a call to i801_check_post() that will print
> an error message in case of timeout.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]