| Message ID | 9103e680-6436-42a3-d4be-39edf851aaf9@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | i2c: i801: Series with minor improvements | expand |
Hi Heiner, On Fri, 15 Apr 2022 18:59:46 +0200, Heiner Kallweit wrote: > Avoid code duplication by calling i801_check_post() from i801_access(). > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/i2c/busses/i2c-i801.c | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) Overall I like the idea. I only have one question to make sure I'm not missing something. > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 9061333f2..ecec7a3a8 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -432,7 +432,7 @@ static int i801_wait_intr(struct i801_priv *priv) > busy = status & SMBHSTSTS_HOST_BUSY; > status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR; > if (!busy && status) > - return status; > + return status & STATUS_ERROR_FLAGS; > } while (time_is_after_eq_jiffies(timeout)); Do I understand correctly that this change isn't really related to the rest of the patch, and could have been done independently? You are filtering out SMBHSTSTS_INTR simply because i801_check_post() will never check it anyway, right? If so, I wonder if that's really something we want to do, as ultimately this adds code with no functional benefit just to be "cleaner". But please correct me if I'm wrong.
On 10.06.2022 16:31, Jean Delvare wrote: > Hi Heiner, > > On Fri, 15 Apr 2022 18:59:46 +0200, Heiner Kallweit wrote: >> Avoid code duplication by calling i801_check_post() from i801_access(). >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/i2c/busses/i2c-i801.c | 20 +++++++++----------- >> 1 file changed, 9 insertions(+), 11 deletions(-) > > Overall I like the idea. I only have one question to make sure I'm not > missing something. > >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c >> index 9061333f2..ecec7a3a8 100644 >> --- a/drivers/i2c/busses/i2c-i801.c >> +++ b/drivers/i2c/busses/i2c-i801.c >> @@ -432,7 +432,7 @@ static int i801_wait_intr(struct i801_priv *priv) >> busy = status & SMBHSTSTS_HOST_BUSY; >> status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR; >> if (!busy && status) >> - return status; >> + return status & STATUS_ERROR_FLAGS; >> } while (time_is_after_eq_jiffies(timeout)); > > Do I understand correctly that this change isn't really related to the > rest of the patch, and could have been done independently? > > You are filtering out SMBHSTSTS_INTR simply because i801_check_post() > will never check it anyway, right? If so, I wonder if that's really > something we want to do, as ultimately this adds code with no > functional benefit just to be "cleaner". But please correct me if I'm > wrong. > Reason is that in few places we check whether return value of i801_wait_intr() is zero, this would fail if not filtering out SMBHSTSTS_INTR. Example: i801_transaction() returns the return value of i801_wait_intr() now. And in i801_block_transaction_by_block() we check whether return value of i801_transaction() is zero.
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 9061333f2..ecec7a3a8 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -432,7 +432,7 @@ static int i801_wait_intr(struct i801_priv *priv) busy = status & SMBHSTSTS_HOST_BUSY; status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR; if (!busy && status) - return status; + return status & STATUS_ERROR_FLAGS; } while (time_is_after_eq_jiffies(timeout)); return -ETIMEDOUT; @@ -456,7 +456,6 @@ static int i801_wait_byte_done(struct i801_priv *priv) static int i801_transaction(struct i801_priv *priv, int xact) { - int status; unsigned long result; const struct i2c_adapter *adap = &priv->adapter; @@ -465,13 +464,12 @@ static int i801_transaction(struct i801_priv *priv, int xact) outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START, SMBHSTCNT(priv)); result = wait_for_completion_timeout(&priv->done, adap->timeout); - return i801_check_post(priv, result ? priv->status : -ETIMEDOUT); + return result ? priv->status : -ETIMEDOUT; } outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv)); - status = i801_wait_intr(priv); - return i801_check_post(priv, status); + return i801_wait_intr(priv); } static int i801_block_transaction_by_block(struct i801_priv *priv, @@ -618,7 +616,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id) status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR; if (status) { - priv->status = status; + priv->status = status & STATUS_ERROR_FLAGS; complete(&priv->done); } @@ -668,7 +666,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, reinit_completion(&priv->done); outb_p(priv->cmd | SMBHSTCNT_START, SMBHSTCNT(priv)); result = wait_for_completion_timeout(&priv->done, adap->timeout); - return i801_check_post(priv, result ? priv->status : -ETIMEDOUT); + return result ? priv->status : -ETIMEDOUT; } for (i = 1; i <= len; i++) { @@ -682,7 +680,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, status = i801_wait_byte_done(priv); if (status) - goto exit; + return status; if (i == 1 && read_write == I2C_SMBUS_READ && command != I2C_SMBUS_I2C_BLOCK_DATA) { @@ -712,9 +710,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv)); } - status = i801_wait_intr(priv); -exit: - return i801_check_post(priv, status); + return i801_wait_intr(priv); } /* Block transaction function */ @@ -922,6 +918,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, else ret = i801_single_transaction(priv, data, read_write, size); + ret = i801_check_post(priv, ret); + /* Some BIOSes don't like it when PEC is enabled at reboot or resume time, so we forcibly disable it after every transaction. Turn off E32B for the same reason. */
Avoid code duplication by calling i801_check_post() from i801_access(). Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)