Re: [PATCH 8/8] i2c: i801: call i801_check_post() from i801_access()

[Heiner Kallweit <hkallweit1@gmail.com>]

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.