* [PATCH v2] i2c: at91: Fix driver for SAMA5D2 SoC @ 2025-01-13 7:02 Alexander Shiyan 2025-01-13 13:17 ` Ahmad Fatoum 2025-01-14 9:52 ` Sascha Hauer 0 siblings, 2 replies; 5+ messages in thread From: Alexander Shiyan @ 2025-01-13 7:02 UTC (permalink / raw) To: barebox; +Cc: 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 <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)); at91_disable_twi_interrupts(dev); -- 2.39.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] i2c: at91: Fix driver for SAMA5D2 SoC 2025-01-13 7:02 [PATCH v2] i2c: at91: Fix driver for SAMA5D2 SoC Alexander Shiyan @ 2025-01-13 13:17 ` Ahmad Fatoum 2025-01-13 13:37 ` Alexander Shiyan 2025-01-14 9:52 ` Sascha Hauer 1 sibling, 1 reply; 5+ messages in thread From: Ahmad Fatoum @ 2025-01-13 13:17 UTC (permalink / raw) To: Alexander Shiyan, barebox 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 | ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] i2c: at91: Fix driver for SAMA5D2 SoC 2025-01-13 13:17 ` Ahmad Fatoum @ 2025-01-13 13:37 ` Alexander Shiyan 2025-01-14 9:27 ` Ahmad Fatoum 0 siblings, 1 reply; 5+ messages in thread From: Alexander Shiyan @ 2025-01-13 13:37 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox 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 <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 | ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] i2c: at91: Fix driver for SAMA5D2 SoC 2025-01-13 13:37 ` Alexander Shiyan @ 2025-01-14 9:27 ` Ahmad Fatoum 0 siblings, 0 replies; 5+ messages in thread From: Ahmad Fatoum @ 2025-01-14 9:27 UTC (permalink / raw) To: Alexander Shiyan; +Cc: barebox 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 | ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] i2c: at91: Fix driver for SAMA5D2 SoC 2025-01-13 7:02 [PATCH v2] i2c: at91: Fix driver for SAMA5D2 SoC Alexander Shiyan 2025-01-13 13:17 ` Ahmad Fatoum @ 2025-01-14 9:52 ` Sascha Hauer 1 sibling, 0 replies; 5+ messages in thread From: Sascha Hauer @ 2025-01-14 9:52 UTC (permalink / raw) To: barebox, Alexander Shiyan On Mon, 13 Jan 2025 10:02:16 +0300, 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. > > 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, -- Sascha Hauer <s.hauer@pengutronix.de> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-01-14 9:54 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-01-13 7:02 [PATCH v2] i2c: at91: Fix driver for SAMA5D2 SoC Alexander Shiyan 2025-01-13 13:17 ` Ahmad Fatoum 2025-01-13 13:37 ` Alexander Shiyan 2025-01-14 9:27 ` Ahmad Fatoum 2025-01-14 9:52 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox