[PATCH v3] i2c: i801: Don't clear status flags twice in interrupt mode

[PATCH v3] i2c: i801: Don't clear status flags twice in interrupt mode

[Heiner Kallweit]

In interrupt mode we clear the status flags twice, in the interrupt
handler and in i801_check_post(). Remove clearing the status flags
from i801_check_post() and handle polling mode by using the
SMBus unlocking write to also clear the status flags if still set.
To be precise: One could still argue that the status flags are
cleared twice in interrupt mode, but it comes for free.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- clear status flags also in i801_wait_byte_done()
- remove outdated comment at i801_check_post()
v3:
- merge unlocking SMBus and clearing status flags
- avoid the complexity added with v2
---
 drivers/i2c/busses/i2c-i801.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 930c6edbe..128a25de7 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -372,11 +372,6 @@ static int i801_check_pre(struct i801_priv *priv)
 	return 0;
 }
 
-/*
- * Convert the status register to an error code, and clear it.
- * Note that status only contains the bits we want to clear, not the
- * actual register value.
- */
 static int i801_check_post(struct i801_priv *priv, int status)
 {
 	int result = 0;
@@ -401,7 +396,6 @@ static int i801_check_post(struct i801_priv *priv, int status)
 		    !(status & SMBHSTSTS_FAILED))
 			dev_err(&priv->pci_dev->dev,
 				"Failed terminating the transaction\n");
-		outb_p(STATUS_FLAGS, SMBHSTSTS(priv));
 		return -ETIMEDOUT;
 	}
 
@@ -440,9 +434,6 @@ static int i801_check_post(struct i801_priv *priv, int status)
 		dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n");
 	}
 
-	/* Clear status flags except BYTE_DONE, to be cleared by caller */
-	outb_p(status, SMBHSTSTS(priv));
-
 	return result;
 }
 
@@ -939,8 +930,11 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	}
 
 out:
-	/* Unlock the SMBus device for use by BIOS/ACPI */
-	outb_p(SMBHSTSTS_INUSE_STS, SMBHSTSTS(priv));
+	/*
+	 * Unlock the SMBus device for use by BIOS/ACPI,
+	 * and clear status flags if not done already.
+	 */
+	outb_p(SMBHSTSTS_INUSE_STS | STATUS_FLAGS, SMBHSTSTS(priv));
 
 	pm_runtime_mark_last_busy(&priv->pci_dev->dev);
 	pm_runtime_put_autosuspend(&priv->pci_dev->dev);
-- 
2.34.1

Re: [PATCH v3] i2c: i801: Don't clear status flags twice in interrupt mode

[Jean Delvare]

On Sat, 04 Dec 2021 21:04:40 +0100, Heiner Kallweit wrote:
> In interrupt mode we clear the status flags twice, in the interrupt
> handler and in i801_check_post(). Remove clearing the status flags
> from i801_check_post() and handle polling mode by using the
> SMBus unlocking write to also clear the status flags if still set.
> To be precise: One could still argue that the status flags are
> cleared twice in interrupt mode, but it comes for free.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - clear status flags also in i801_wait_byte_done()
> - remove outdated comment at i801_check_post()
> v3:
> - merge unlocking SMBus and clearing status flags
> - avoid the complexity added with v2
> ---
>  drivers/i2c/busses/i2c-i801.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 930c6edbe..128a25de7 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -372,11 +372,6 @@ static int i801_check_pre(struct i801_priv *priv)
>  	return 0;
>  }
>  
> -/*
> - * Convert the status register to an error code, and clear it.
> - * Note that status only contains the bits we want to clear, not the
> - * actual register value.
> - */
>  static int i801_check_post(struct i801_priv *priv, int status)
>  {
>  	int result = 0;
> @@ -401,7 +396,6 @@ static int i801_check_post(struct i801_priv *priv, int status)
>  		    !(status & SMBHSTSTS_FAILED))
>  			dev_err(&priv->pci_dev->dev,
>  				"Failed terminating the transaction\n");
> -		outb_p(STATUS_FLAGS, SMBHSTSTS(priv));
>  		return -ETIMEDOUT;
>  	}
>  
> @@ -440,9 +434,6 @@ static int i801_check_post(struct i801_priv *priv, int status)
>  		dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n");
>  	}
>  
> -	/* Clear status flags except BYTE_DONE, to be cleared by caller */
> -	outb_p(status, SMBHSTSTS(priv));
> -
>  	return result;
>  }
>  
> @@ -939,8 +930,11 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	}
>  
>  out:
> -	/* Unlock the SMBus device for use by BIOS/ACPI */
> -	outb_p(SMBHSTSTS_INUSE_STS, SMBHSTSTS(priv));
> +	/*
> +	 * Unlock the SMBus device for use by BIOS/ACPI,
> +	 * and clear status flags if not done already.
> +	 */
> +	outb_p(SMBHSTSTS_INUSE_STS | STATUS_FLAGS, SMBHSTSTS(priv));
>  
>  	pm_runtime_mark_last_busy(&priv->pci_dev->dev);
>  	pm_runtime_put_autosuspend(&priv->pci_dev->dev);

Looks good to me.

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


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v3] i2c: i801: Don't clear status flags twice in interrupt mode

[Wolfram Sang]

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

On Sat, Dec 04, 2021 at 09:04:40PM +0100, Heiner Kallweit wrote:
> In interrupt mode we clear the status flags twice, in the interrupt
> handler and in i801_check_post(). Remove clearing the status flags
> from i801_check_post() and handle polling mode by using the
> SMBus unlocking write to also clear the status flags if still set.
> To be precise: One could still argue that the status flags are
> cleared twice in interrupt mode, but it comes for free.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


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