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@xxxxxxxxx> >> --- >> 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.
