| Message ID | 05bec021-6958-0157-b825-619ac21ddd41@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | i2c: i801: Series with minor improvements | expand |
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.
On 07.06.2022 14:34, Jean Delvare wrote: > 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. > OK. Meanwhile I prefer readability over saving few bytes, but here IMO the readability doesn't suffer. >> 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? > So far we exclude here the case where SMBHSTSTS is written in i801_isr_byte_done(). The patch allows to write SMBHSTSTS here only, so we don't need the condition any longer. >> + >> + 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 --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);
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(-)