Skip to contentSkip to conversation

i2c: at91: Fix driver for SAMA5D2 SoC

PATCH Applied Alexander Shiyan 3 participants 5 messages Jan 13, 2025 – Jan 14, 2025

Subsystems (2)
I2C subsystem host drivers Arm/Microchip (AT91) SoC support
lore
Share & tools
lore

Alexander Shiyan

author
#0 lore raw

PATCH v2 1/1 i2c: at91: Fix driver for SAMA5D2 SoC

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 <[email protected]>
---
 drivers/i2c/busses/i2c-at91.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-at91.c +1 -1
index dbc5aa4ddf..8929dbaede 100644
@@ -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));
 
 	at91_disable_twi_interrupts(dev);
Signature · 1 line
2.39.1
#1 lore raw
Quoted 14 lines · Alexander Shiyan

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 <[email protected]>
---
 drivers/i2c/busses/i2c-at91.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-at91.c +0 -0
index dbc5aa4ddf..8929dbaede 100644

drivers/i2c/busses/i2c-at91.c static int at91_twi_wait_completion(struct at91_twi_dev *dev) · line 279

Quoted 5 lines
drivers/i2c/busses/i2c-at91.c +1 -1
@@ -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?

Quoted 3 lines
drivers/i2c/busses/i2c-at91.c +0 -0
 	at91_disable_twi_interrupts(dev);
Signature · 4 lines
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 |
author
#2 lore raw

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.

Thanks!

пн, 13 янв. 2025 г. в 16:17, Ahmad Fatoum <[email protected]>:

Quoted 38 lines · Alexander Shiyan
Quoted 14 lines

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 <[email protected]>
---
 drivers/i2c/busses/i2c-at91.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-at91.c +0 -0
index dbc5aa4ddf..8929dbaede 100644

drivers/i2c/busses/i2c-at91.c static int at91_twi_wait_completion(struct at91_twi_dev *dev) · line 279

Quoted 5 lines
drivers/i2c/busses/i2c-at91.c +1 -1
@@ -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?

drivers/i2c/busses/i2c-at91.c static int at91_twi_wait_completion(struct at91_twi_dev *dev) · line 282

Quoted 3 lines
drivers/i2c/busses/i2c-at91.c +0 -0
      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 |
Acked-by
#3 lore raw
Quoted 7 lines · Alexander Shiyan

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 <[email protected]>

Thanks,
Ahmad

Quoted 43 lines

Thanks!

пн, 13 янв. 2025 г. в 16:17, Ahmad Fatoum <[email protected]>:

Quoted 38 lines · Alexander Shiyan
Quoted 14 lines

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 <[email protected]>
---
 drivers/i2c/busses/i2c-at91.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-at91.c +0 -0
index dbc5aa4ddf..8929dbaede 100644

drivers/i2c/busses/i2c-at91.c static int at91_twi_wait_completion(struct at91_twi_dev *dev) · line 279

Quoted 5 lines
drivers/i2c/busses/i2c-at91.c +1 -1
@@ -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?

drivers/i2c/busses/i2c-at91.c static int at91_twi_wait_completion(struct at91_twi_dev *dev) · line 282

Quoted 3 lines
drivers/i2c/busses/i2c-at91.c +0 -0
      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 |
Signature · 4 lines
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 |
Applied
#4 lore raw
In reply to Alexander Shiyan
Quoted 5 lines · Alexander Shiyan

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.

Applied, thanks!

[1/1] i2c: at91: Fix driver for SAMA5D2 SoC
https://git.pengutronix.de/cgit/barebox/commit/?id=da9d4089f0d8 (link may not be stable)

Best regards,

Signature · 1 line
Sascha Hauer <[email protected]>