diff mbox series

[1/8] i2c: i801: improve interrupt handler

Message ID 05bec021-6958-0157-b825-619ac21ddd41@gmail.com
State Superseded
Headers show
Series i2c: i801: Series with minor improvements | expand

Commit Message

Heiner Kallweit April 15, 2022, 4:54 p.m. UTC
Not sure if it can happen, but better play safe: If SMBHSTSTS_BYTE_DONE
and an error flag is set, then don't trust the result and skip calling
i801_isr_byte_done(). In addition clear status bit SMBHSTSTS_BYTE_DONE
in the main interrupt handler, this allows to simplify the code a
little.

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

Comments

Jean Delvare June 7, 2022, 12:34 p.m. UTC | #1
Hi Heiner, 

On Fri, 15 Apr 2022 18:54:32 +0200, Heiner Kallweit wrote:
> Not sure if it can happen, but better play safe: If SMBHSTSTS_BYTE_DONE
> and an error flag is set, then don't trust the result and skip calling
> i801_isr_byte_done(). In addition clear status bit SMBHSTSTS_BYTE_DONE
> in the main interrupt handler, this allows to simplify the code a
> little.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ff706349b..c481f121d 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -556,9 +556,6 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>  		/* Write next byte, except for IRQ after last byte */
>  		outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
>  	}
> -
> -	/* Clear BYTE_DONE to continue with next byte */
> -	outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
>  }
>  
>  static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
> @@ -588,7 +585,6 @@ static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
>   *      BUS_ERR - SMI# transaction collision
>   *      FAILED - transaction was canceled due to a KILL request
>   *    When any of these occur, update ->status and signal completion.
> - *    ->status must be cleared before kicking off the next transaction.
>   *
>   * 2) For byte-by-byte (I2C read/write) transactions, one BYTE_DONE interrupt
>   *    occurs for each byte of a byte-by-byte to prepare the next byte.
> @@ -613,23 +609,18 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>  	}
>  
>  	status = inb_p(SMBHSTSTS(priv));
> -	if (status & SMBHSTSTS_BYTE_DONE)
> +	if ((status & SMBHSTSTS_BYTE_DONE) && !(status & STATUS_ERROR_FLAGS))

Isn't this better written

	if ((status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS)) == SMBHSTSTS_BYTE_DONE)

? At least my compiler generates smaller binary code.

>  		i801_isr_byte_done(priv);
>  
>  	/*
> -	 * Clear remaining IRQ sources: Completion of last command, errors
> -	 * and the SMB_ALERT signal. SMB_ALERT status is set after signal
> -	 * assertion independently of the interrupt generation being blocked
> -	 * or not so clear it always when the status is set.
> -	 */
> -	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS | SMBHSTSTS_SMBALERT_STS;
> -	if (status)
> -		outb_p(status, SMBHSTSTS(priv));
> -	status &= ~SMBHSTSTS_SMBALERT_STS; /* SMB_ALERT not reported */
> -	/*
> -	 * Report transaction result.
> -	 * ->status must be cleared before the next transaction is started.
> +	 * Clear IRQ sources: SMB_ALERT status is set after signal assertion
> +	 * independently of the interrupt generation being blocked or not
> +	 * so clear it always when the status is set.
>  	 */
> +	status &= STATUS_FLAGS | SMBHSTSTS_SMBALERT_STS;
> +	outb_p(status, SMBHSTSTS(priv));

You are making the call to outb_p() unconditional. Is this under the
assumption that at least one of the bits must be set, so the condition
was always met?

> +
> +	status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
>  	if (status) {
>  		priv->status = status;
>  		complete(&priv->done);

Tested OK on my system, looks good overall, nice simplification.
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ff706349b..c481f121d 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -556,9 +556,6 @@  static void i801_isr_byte_done(struct i801_priv *priv)
 		/* Write next byte, except for IRQ after last byte */
 		outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
 	}
-
-	/* Clear BYTE_DONE to continue with next byte */
-	outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
 }
 
 static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
@@ -588,7 +585,6 @@  static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
  *      BUS_ERR - SMI# transaction collision
  *      FAILED - transaction was canceled due to a KILL request
  *    When any of these occur, update ->status and signal completion.
- *    ->status must be cleared before kicking off the next transaction.
  *
  * 2) For byte-by-byte (I2C read/write) transactions, one BYTE_DONE interrupt
  *    occurs for each byte of a byte-by-byte to prepare the next byte.
@@ -613,23 +609,18 @@  static irqreturn_t i801_isr(int irq, void *dev_id)
 	}
 
 	status = inb_p(SMBHSTSTS(priv));
-	if (status & SMBHSTSTS_BYTE_DONE)
+	if ((status & SMBHSTSTS_BYTE_DONE) && !(status & STATUS_ERROR_FLAGS))
 		i801_isr_byte_done(priv);
 
 	/*
-	 * Clear remaining IRQ sources: Completion of last command, errors
-	 * and the SMB_ALERT signal. SMB_ALERT status is set after signal
-	 * assertion independently of the interrupt generation being blocked
-	 * or not so clear it always when the status is set.
-	 */
-	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS | SMBHSTSTS_SMBALERT_STS;
-	if (status)
-		outb_p(status, SMBHSTSTS(priv));
-	status &= ~SMBHSTSTS_SMBALERT_STS; /* SMB_ALERT not reported */
-	/*
-	 * Report transaction result.
-	 * ->status must be cleared before the next transaction is started.
+	 * Clear IRQ sources: SMB_ALERT status is set after signal assertion
+	 * independently of the interrupt generation being blocked or not
+	 * so clear it always when the status is set.
 	 */
+	status &= STATUS_FLAGS | SMBHSTSTS_SMBALERT_STS;
+	outb_p(status, SMBHSTSTS(priv));
+
+	status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
 	if (status) {
 		priv->status = status;
 		complete(&priv->done);