Re: [PATCH v2] i2c: at91: Fix driver for SAMA5D2 SoC

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 13.01.25 08:02, Alexander Shiyan wrote:
> Most bits of the AT91 SR status register are cleared after reading,
> so we may lose the status at the end of the wait loop.
> Let's use the value already readed to check for "completed" status.
> 
> Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>
> ---
>  drivers/i2c/busses/i2c-at91.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index dbc5aa4ddf..8929dbaede 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -276,7 +276,7 @@ static int at91_twi_wait_completion(struct at91_twi_dev *dev)
>  
>  		dev->transfer_status |= status;
>  
> -	} while (!(at91_twi_read(dev, AT91_TWI_SR) & AT91_TWI_TXCOMP));
> +	} while (!(status & AT91_TWI_TXCOMP));

This makes the function harder to reason about, because the status read
is done at the start, but only acted upon at the very end.

Would it make sense to change this to a while loop or move it into the
loop directly after the other status read and early exit if it's true?

>  
>  	at91_disable_twi_interrupts(dev);
>  


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |