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

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

On 13.01.25 14:37, Alexander Shiyan wrote:
> Hello.
> 
> As far as I understand, AT91_TWI_TXCOMP is set at the same time as
> one of the AT91_TWI_RXRDY or AT91_TWI_TXRDY signals, so we need
> to check these bits first to perform the read or write operation of
> the next byte.
> If it was the last byte (when the STOP bit is transmitted) we exit.

If the case AT91_TWI_TXCOMP on its own can't happen, then this is fine.

Acked-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Thanks,
Ahmad

> 
> Thanks!
> 
> пн, 13 янв. 2025 г. в 16:17, 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 |
> 


-- 
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 |