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@xxxxxxxxx>
> ---
> 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.
--
Jean Delvare
SUSE L3 Support
