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

[Jean Delvare <jdelvare@suse.de>]

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.

-- 
Jean Delvare
SUSE L3 Support