diff mbox series

[08/13] i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout

Message ID 20240215-mbly-i2c-v1-8-19a336e91dca@bootlin.com
State New
Headers
Series Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts |

Commit Message

Théo Lebrun Feb. 15, 2024, 4:52 p.m. UTC 
  The FIFO flush function uses a jiffies amount to detect timeouts as the
flushing is async. Replace with ktime to get more accurate precision
and support short timeouts.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Linus Walleij Feb. 19, 2024, 2:21 p.m. UTC | #1 
On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> The FIFO flush function uses a jiffies amount to detect timeouts as the
> flushing is async. Replace with ktime to get more accurate precision
> and support short timeouts.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Excellent patch. Thanks.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Théo Lebrun Feb. 19, 2024, 2:38 p.m. UTC | #2 
Hello,

On Mon Feb 19, 2024 at 3:21 PM CET, Linus Walleij wrote:
> On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
>
> > The FIFO flush function uses a jiffies amount to detect timeouts as the
> > flushing is async. Replace with ktime to get more accurate precision
> > and support short timeouts.
> >
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>
> Excellent patch. Thanks.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Somewhat related to this patch: while writing it, I noticed the total
timeout of flush_i2c_fifo() is 10 times the timeout. Without this
series, this means 10*200ms of busywaiting!

If you have an opinion on a more sensible option for this I could add a
patch to my V2. I don't know enough to pick a sensible value.

I'm unsure if it makes sense that the timeout of flush_i2c_fifo() is a
multiple of the transfer timeout. Does it make sense that those two
timeouts are correlated?

Big thanks for your review,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Linus Walleij Feb. 19, 2024, 2:53 p.m. UTC | #3 
On Mon, Feb 19, 2024 at 3:38 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Somewhat related to this patch: while writing it, I noticed the total
> timeout of flush_i2c_fifo() is 10 times the timeout. Without this
> series, this means 10*200ms of busywaiting!
>
> If you have an opinion on a more sensible option for this I could add a
> patch to my V2. I don't know enough to pick a sensible value.
>
> I'm unsure if it makes sense that the timeout of flush_i2c_fifo() is a
> multiple of the transfer timeout. Does it make sense that those two
> timeouts are correlated?

I have a *vague* memory of the timeouts for flushing needing to be longer
but I might be mistaken. This is probably a Srinidhi or even Sachin question...
Sadly I don't have their current mail addresses.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index e68b8e0d7919..afd54999bbbb 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -219,8 +219,8 @@  static inline void i2c_clr_bit(void __iomem *reg, u32 mask)
 static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
 {
 #define LOOP_ATTEMPTS 10
+	ktime_t timeout;
 	int i;
-	unsigned long timeout;
 
 	/*
 	 * flush the transmit and receive FIFO. The flushing
@@ -232,9 +232,9 @@  static int flush_i2c_fifo(struct nmk_i2c_dev *priv)
 	writel((I2C_CR_FTX | I2C_CR_FRX), priv->virtbase + I2C_CR);
 
 	for (i = 0; i < LOOP_ATTEMPTS; i++) {
-		timeout = jiffies + priv->adap.timeout;
+		timeout = ktime_add_us(ktime_get(), priv->timeout_usecs);
 
-		while (!time_after(jiffies, timeout)) {
+		while (ktime_after(timeout, ktime_get())) {
 			if ((readl(priv->virtbase + I2C_CR) &
 				(I2C_CR_FTX | I2C_CR_FRX)) == 0)
 				return 0;