[PATCH 0/3] i2c:imx: Deliver a timely stop on slave side, fix recv

[PATCH 0/3] i2c:imx: Deliver a timely stop on slave side, fix recv

[minyard]

I was working on an application that needs the stop condition
immediately.  So this adds a timer after each byte is received/sent and
if the bus is idle at the timeout, send the stop.

Also, I noticed when you use the i2c-slave-eeprom, if you read some data
and then read some data again, the last byte of the first read will be
the first byte of the second read.  This is because i2c-slave-eeprom
expects a read-ahead.  That's what the documentation says, at least.

-corey

[PATCH 1/3] i2c:imx: Add timer for handling the stop condition

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Most IMX I2C interfaces don't generate an interrupt on a stop condition,
so it won't generate a timely stop event on a slave mode transfer.
Some users, like IPMB, need a timely stop event to work properly.

So, add a timer and add the proper handling to generate a stop event in
slave mode if the interface goes idle.

Signed-off-by: Corey Minyard <minyard@acm.org>
Tested-by: Andrew Manley <andrew.manley@sealingtech.com>
Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com>
---
 drivers/i2c/busses/i2c-imx.c | 78 +++++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 3576b63a6c03..97369fe48b30 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -37,6 +37,8 @@
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
+#include <linux/spinlock.h>
+#include <linux/timer.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -210,6 +212,10 @@ struct imx_i2c_struct {
 	struct imx_i2c_dma	*dma;
 	struct i2c_client	*slave;
 	enum i2c_slave_event last_slave_event;
+
+	/* For checking slave events. */
+	spinlock_t	  slave_lock;
+	struct timer_list slave_timer;
 };
 
 static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
@@ -680,7 +686,7 @@ static void i2c_imx_slave_event(struct imx_i2c_struct *i2c_imx,
 
 static void i2c_imx_slave_finish_op(struct imx_i2c_struct *i2c_imx)
 {
-	u8 val;
+	u8 val = 0;
 
 	while (i2c_imx->last_slave_event != I2C_SLAVE_STOP) {
 		switch (i2c_imx->last_slave_event) {
@@ -701,10 +707,11 @@ static void i2c_imx_slave_finish_op(struct imx_i2c_struct *i2c_imx)
 	}
 }
 
-static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
-				     unsigned int status, unsigned int ctl)
+/* Returns true if the timer should be restarted, false if not. */
+static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx,
+					unsigned int status, unsigned int ctl)
 {
-	u8 value;
+	u8 value = 0;
 
 	if (status & I2SR_IAL) { /* Arbitration lost */
 		i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
@@ -712,6 +719,16 @@ static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
 			return IRQ_HANDLED;
 	}
 
+	if (!(status & I2SR_IBB)) {
+		/* No master on the bus, that could mean a stop condition. */
+		i2c_imx_slave_finish_op(i2c_imx);
+		return IRQ_HANDLED;
+	}
+
+	if (!(status & I2SR_ICF))
+		/* Data transfer still in progress, ignore this. */
+		goto out;
+
 	if (status & I2SR_IAAS) { /* Addressed as a slave */
 		i2c_imx_slave_finish_op(i2c_imx);
 		if (status & I2SR_SRW) { /* Master wants to read from us*/
@@ -737,16 +754,9 @@ static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
 			imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
 		}
 	} else if (!(ctl & I2CR_MTX)) { /* Receive mode */
-		if (status & I2SR_IBB) { /* No STOP signal detected */
-			value = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
-			i2c_imx_slave_event(i2c_imx,
-					    I2C_SLAVE_WRITE_RECEIVED, &value);
-		} else { /* STOP signal is detected */
-			dev_dbg(&i2c_imx->adapter.dev,
-				"STOP signal detected");
-			i2c_imx_slave_event(i2c_imx,
-					    I2C_SLAVE_STOP, &value);
-		}
+		value = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
+		i2c_imx_slave_event(i2c_imx,
+				    I2C_SLAVE_WRITE_RECEIVED, &value);
 	} else if (!(status & I2SR_RXAK)) { /* Transmit mode received ACK */
 		ctl |= I2CR_MTX;
 		imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
@@ -755,15 +765,32 @@ static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
 				    I2C_SLAVE_READ_PROCESSED, &value);
 
 		imx_i2c_write_reg(value, i2c_imx, IMX_I2C_I2DR);
-	} else { /* Transmit mode received NAK */
+	} else { /* Transmit mode received NAK, operation is done */
 		ctl &= ~I2CR_MTX;
 		imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
 		imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
+		i2c_imx_slave_finish_op(i2c_imx);
+		return IRQ_HANDLED;
 	}
 
+out:
+	mod_timer(&i2c_imx->slave_timer, jiffies + 1);
 	return IRQ_HANDLED;
 }
 
+static void i2c_imx_slave_timeout(struct timer_list *t)
+{
+	struct imx_i2c_struct *i2c_imx = from_timer(i2c_imx, t, slave_timer);
+	unsigned int ctl, status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&i2c_imx->slave_lock, flags);
+	status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+	i2c_imx_slave_handle(i2c_imx, status, ctl);
+	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
+}
+
 static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx)
 {
 	int temp;
@@ -843,7 +870,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
 {
 	struct imx_i2c_struct *i2c_imx = dev_id;
 	unsigned int ctl, status;
+	unsigned long flags;
 
+	spin_lock_irqsave(&i2c_imx->slave_lock, flags);
 	status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
 	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 
@@ -851,14 +880,20 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
 		i2c_imx_clear_irq(i2c_imx, I2SR_IIF);
 		if (i2c_imx->slave) {
 			if (!(ctl & I2CR_MSTA)) {
-				return i2c_imx_slave_isr(i2c_imx, status, ctl);
-			} else if (i2c_imx->last_slave_event !=
-				   I2C_SLAVE_STOP) {
-				i2c_imx_slave_finish_op(i2c_imx);
+				irqreturn_t ret;
+
+				ret = i2c_imx_slave_handle(i2c_imx,
+							   status, ctl);
+				spin_unlock_irqrestore(&i2c_imx->slave_lock,
+						       flags);
+				return ret;
 			}
+			i2c_imx_slave_finish_op(i2c_imx);
 		}
+		spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
 		return i2c_imx_master_isr(i2c_imx, status);
 	}
+	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
 
 	return IRQ_NONE;
 }
@@ -1378,6 +1413,9 @@ static int i2c_imx_probe(struct platform_device *pdev)
 	if (!i2c_imx)
 		return -ENOMEM;
 
+	spin_lock_init(&i2c_imx->slave_lock);
+	timer_setup(&i2c_imx->slave_timer, i2c_imx_slave_timeout, 0);
+
 	match = device_get_match_data(&pdev->dev);
 	if (match)
 		i2c_imx->hwdata = match;
@@ -1491,6 +1529,8 @@ static int i2c_imx_remove(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	del_timer_sync(&i2c_imx->slave_timer);
+
 	/* remove adapter */
 	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
 	i2c_del_adapter(&i2c_imx->adapter);
-- 
2.25.1

Re: [PATCH 1/3] i2c:imx: Add timer for handling the stop condition

[Corey Minyard]

On Mon, Oct 04, 2021 at 07:32:14PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>

Any comments for this patch series?

-corey

> 
> Most IMX I2C interfaces don't generate an interrupt on a stop condition,
> so it won't generate a timely stop event on a slave mode transfer.
> Some users, like IPMB, need a timely stop event to work properly.
> 
> So, add a timer and add the proper handling to generate a stop event in
> slave mode if the interface goes idle.
> 
> Signed-off-by: Corey Minyard <minyard@acm.org>
> Tested-by: Andrew Manley <andrew.manley@sealingtech.com>
> Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com>
> ---
>  drivers/i2c/busses/i2c-imx.c | 78 +++++++++++++++++++++++++++---------
>  1 file changed, 59 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 3576b63a6c03..97369fe48b30 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -37,6 +37,8 @@
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
> +#include <linux/spinlock.h>
> +#include <linux/timer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -210,6 +212,10 @@ struct imx_i2c_struct {
>  	struct imx_i2c_dma	*dma;
>  	struct i2c_client	*slave;
>  	enum i2c_slave_event last_slave_event;
> +
> +	/* For checking slave events. */
> +	spinlock_t	  slave_lock;
> +	struct timer_list slave_timer;
>  };
>  
>  static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> @@ -680,7 +686,7 @@ static void i2c_imx_slave_event(struct imx_i2c_struct *i2c_imx,
>  
>  static void i2c_imx_slave_finish_op(struct imx_i2c_struct *i2c_imx)
>  {
> -	u8 val;
> +	u8 val = 0;
>  
>  	while (i2c_imx->last_slave_event != I2C_SLAVE_STOP) {
>  		switch (i2c_imx->last_slave_event) {
> @@ -701,10 +707,11 @@ static void i2c_imx_slave_finish_op(struct imx_i2c_struct *i2c_imx)
>  	}
>  }
>  
> -static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
> -				     unsigned int status, unsigned int ctl)
> +/* Returns true if the timer should be restarted, false if not. */
> +static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx,
> +					unsigned int status, unsigned int ctl)
>  {
> -	u8 value;
> +	u8 value = 0;
>  
>  	if (status & I2SR_IAL) { /* Arbitration lost */
>  		i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
> @@ -712,6 +719,16 @@ static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
>  			return IRQ_HANDLED;
>  	}
>  
> +	if (!(status & I2SR_IBB)) {
> +		/* No master on the bus, that could mean a stop condition. */
> +		i2c_imx_slave_finish_op(i2c_imx);
> +		return IRQ_HANDLED;
> +	}
> +
> +	if (!(status & I2SR_ICF))
> +		/* Data transfer still in progress, ignore this. */
> +		goto out;
> +
>  	if (status & I2SR_IAAS) { /* Addressed as a slave */
>  		i2c_imx_slave_finish_op(i2c_imx);
>  		if (status & I2SR_SRW) { /* Master wants to read from us*/
> @@ -737,16 +754,9 @@ static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
>  			imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
>  		}
>  	} else if (!(ctl & I2CR_MTX)) { /* Receive mode */
> -		if (status & I2SR_IBB) { /* No STOP signal detected */
> -			value = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> -			i2c_imx_slave_event(i2c_imx,
> -					    I2C_SLAVE_WRITE_RECEIVED, &value);
> -		} else { /* STOP signal is detected */
> -			dev_dbg(&i2c_imx->adapter.dev,
> -				"STOP signal detected");
> -			i2c_imx_slave_event(i2c_imx,
> -					    I2C_SLAVE_STOP, &value);
> -		}
> +		value = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> +		i2c_imx_slave_event(i2c_imx,
> +				    I2C_SLAVE_WRITE_RECEIVED, &value);
>  	} else if (!(status & I2SR_RXAK)) { /* Transmit mode received ACK */
>  		ctl |= I2CR_MTX;
>  		imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> @@ -755,15 +765,32 @@ static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
>  				    I2C_SLAVE_READ_PROCESSED, &value);
>  
>  		imx_i2c_write_reg(value, i2c_imx, IMX_I2C_I2DR);
> -	} else { /* Transmit mode received NAK */
> +	} else { /* Transmit mode received NAK, operation is done */
>  		ctl &= ~I2CR_MTX;
>  		imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
>  		imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> +		i2c_imx_slave_finish_op(i2c_imx);
> +		return IRQ_HANDLED;
>  	}
>  
> +out:
> +	mod_timer(&i2c_imx->slave_timer, jiffies + 1);
>  	return IRQ_HANDLED;
>  }
>  
> +static void i2c_imx_slave_timeout(struct timer_list *t)
> +{
> +	struct imx_i2c_struct *i2c_imx = from_timer(i2c_imx, t, slave_timer);
> +	unsigned int ctl, status;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&i2c_imx->slave_lock, flags);
> +	status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> +	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	i2c_imx_slave_handle(i2c_imx, status, ctl);
> +	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
> +}
> +
>  static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx)
>  {
>  	int temp;
> @@ -843,7 +870,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  {
>  	struct imx_i2c_struct *i2c_imx = dev_id;
>  	unsigned int ctl, status;
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&i2c_imx->slave_lock, flags);
>  	status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
>  	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>  
> @@ -851,14 +880,20 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  		i2c_imx_clear_irq(i2c_imx, I2SR_IIF);
>  		if (i2c_imx->slave) {
>  			if (!(ctl & I2CR_MSTA)) {
> -				return i2c_imx_slave_isr(i2c_imx, status, ctl);
> -			} else if (i2c_imx->last_slave_event !=
> -				   I2C_SLAVE_STOP) {
> -				i2c_imx_slave_finish_op(i2c_imx);
> +				irqreturn_t ret;
> +
> +				ret = i2c_imx_slave_handle(i2c_imx,
> +							   status, ctl);
> +				spin_unlock_irqrestore(&i2c_imx->slave_lock,
> +						       flags);
> +				return ret;
>  			}
> +			i2c_imx_slave_finish_op(i2c_imx);
>  		}
> +		spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
>  		return i2c_imx_master_isr(i2c_imx, status);
>  	}
> +	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
>  
>  	return IRQ_NONE;
>  }
> @@ -1378,6 +1413,9 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	if (!i2c_imx)
>  		return -ENOMEM;
>  
> +	spin_lock_init(&i2c_imx->slave_lock);
> +	timer_setup(&i2c_imx->slave_timer, i2c_imx_slave_timeout, 0);
> +
>  	match = device_get_match_data(&pdev->dev);
>  	if (match)
>  		i2c_imx->hwdata = match;
> @@ -1491,6 +1529,8 @@ static int i2c_imx_remove(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> +	del_timer_sync(&i2c_imx->slave_timer);
> +
>  	/* remove adapter */
>  	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
>  	i2c_del_adapter(&i2c_imx->adapter);
> -- 
> 2.25.1
> 

Re: [PATCH 1/3] i2c:imx: Add timer for handling the stop condition

[Oleksij Rempel]

On Mon, Oct 04, 2021 at 07:32:14PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Most IMX I2C interfaces don't generate an interrupt on a stop condition,
> so it won't generate a timely stop event on a slave mode transfer.
> Some users, like IPMB, need a timely stop event to work properly.
> 
> So, add a timer and add the proper handling to generate a stop event in
> slave mode if the interface goes idle.
> 
> Signed-off-by: Corey Minyard <minyard@acm.org>
> Tested-by: Andrew Manley <andrew.manley@sealingtech.com>
> Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com>

Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!

> ---
>  drivers/i2c/busses/i2c-imx.c | 78 +++++++++++++++++++++++++++---------
>  1 file changed, 59 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 3576b63a6c03..97369fe48b30 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -37,6 +37,8 @@
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
> +#include <linux/spinlock.h>
> +#include <linux/timer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -210,6 +212,10 @@ struct imx_i2c_struct {
>  	struct imx_i2c_dma	*dma;
>  	struct i2c_client	*slave;
>  	enum i2c_slave_event last_slave_event;
> +
> +	/* For checking slave events. */
> +	spinlock_t	  slave_lock;
> +	struct timer_list slave_timer;
>  };
>  
>  static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> @@ -680,7 +686,7 @@ static void i2c_imx_slave_event(struct imx_i2c_struct *i2c_imx,
>  
>  static void i2c_imx_slave_finish_op(struct imx_i2c_struct *i2c_imx)
>  {
> -	u8 val;
> +	u8 val = 0;
>  
>  	while (i2c_imx->last_slave_event != I2C_SLAVE_STOP) {
>  		switch (i2c_imx->last_slave_event) {
> @@ -701,10 +707,11 @@ static void i2c_imx_slave_finish_op(struct imx_i2c_struct *i2c_imx)
>  	}
>  }
>  
> -static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
> -				     unsigned int status, unsigned int ctl)
> +/* Returns true if the timer should be restarted, false if not. */
> +static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx,
> +					unsigned int status, unsigned int ctl)
>  {
> -	u8 value;
> +	u8 value = 0;
>  
>  	if (status & I2SR_IAL) { /* Arbitration lost */
>  		i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
> @@ -712,6 +719,16 @@ static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
>  			return IRQ_HANDLED;
>  	}
>  
> +	if (!(status & I2SR_IBB)) {
> +		/* No master on the bus, that could mean a stop condition. */
> +		i2c_imx_slave_finish_op(i2c_imx);
> +		return IRQ_HANDLED;
> +	}
> +
> +	if (!(status & I2SR_ICF))
> +		/* Data transfer still in progress, ignore this. */
> +		goto out;
> +
>  	if (status & I2SR_IAAS) { /* Addressed as a slave */
>  		i2c_imx_slave_finish_op(i2c_imx);
>  		if (status & I2SR_SRW) { /* Master wants to read from us*/
> @@ -737,16 +754,9 @@ static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
>  			imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
>  		}
>  	} else if (!(ctl & I2CR_MTX)) { /* Receive mode */
> -		if (status & I2SR_IBB) { /* No STOP signal detected */
> -			value = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> -			i2c_imx_slave_event(i2c_imx,
> -					    I2C_SLAVE_WRITE_RECEIVED, &value);
> -		} else { /* STOP signal is detected */
> -			dev_dbg(&i2c_imx->adapter.dev,
> -				"STOP signal detected");
> -			i2c_imx_slave_event(i2c_imx,
> -					    I2C_SLAVE_STOP, &value);
> -		}
> +		value = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> +		i2c_imx_slave_event(i2c_imx,
> +				    I2C_SLAVE_WRITE_RECEIVED, &value);
>  	} else if (!(status & I2SR_RXAK)) { /* Transmit mode received ACK */
>  		ctl |= I2CR_MTX;
>  		imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> @@ -755,15 +765,32 @@ static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx,
>  				    I2C_SLAVE_READ_PROCESSED, &value);
>  
>  		imx_i2c_write_reg(value, i2c_imx, IMX_I2C_I2DR);
> -	} else { /* Transmit mode received NAK */
> +	} else { /* Transmit mode received NAK, operation is done */
>  		ctl &= ~I2CR_MTX;
>  		imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
>  		imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> +		i2c_imx_slave_finish_op(i2c_imx);
> +		return IRQ_HANDLED;
>  	}
>  
> +out:
> +	mod_timer(&i2c_imx->slave_timer, jiffies + 1);
>  	return IRQ_HANDLED;
>  }
>  
> +static void i2c_imx_slave_timeout(struct timer_list *t)
> +{
> +	struct imx_i2c_struct *i2c_imx = from_timer(i2c_imx, t, slave_timer);
> +	unsigned int ctl, status;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&i2c_imx->slave_lock, flags);
> +	status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> +	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +	i2c_imx_slave_handle(i2c_imx, status, ctl);
> +	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
> +}
> +
>  static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx)
>  {
>  	int temp;
> @@ -843,7 +870,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  {
>  	struct imx_i2c_struct *i2c_imx = dev_id;
>  	unsigned int ctl, status;
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&i2c_imx->slave_lock, flags);
>  	status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
>  	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>  
> @@ -851,14 +880,20 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  		i2c_imx_clear_irq(i2c_imx, I2SR_IIF);
>  		if (i2c_imx->slave) {
>  			if (!(ctl & I2CR_MSTA)) {
> -				return i2c_imx_slave_isr(i2c_imx, status, ctl);
> -			} else if (i2c_imx->last_slave_event !=
> -				   I2C_SLAVE_STOP) {
> -				i2c_imx_slave_finish_op(i2c_imx);
> +				irqreturn_t ret;
> +
> +				ret = i2c_imx_slave_handle(i2c_imx,
> +							   status, ctl);
> +				spin_unlock_irqrestore(&i2c_imx->slave_lock,
> +						       flags);
> +				return ret;
>  			}
> +			i2c_imx_slave_finish_op(i2c_imx);
>  		}
> +		spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
>  		return i2c_imx_master_isr(i2c_imx, status);
>  	}
> +	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
>  
>  	return IRQ_NONE;
>  }
> @@ -1378,6 +1413,9 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	if (!i2c_imx)
>  		return -ENOMEM;
>  
> +	spin_lock_init(&i2c_imx->slave_lock);
> +	timer_setup(&i2c_imx->slave_timer, i2c_imx_slave_timeout, 0);
> +
>  	match = device_get_match_data(&pdev->dev);
>  	if (match)
>  		i2c_imx->hwdata = match;
> @@ -1491,6 +1529,8 @@ static int i2c_imx_remove(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> +	del_timer_sync(&i2c_imx->slave_timer);
> +
>  	/* remove adapter */
>  	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
>  	i2c_del_adapter(&i2c_imx->adapter);
> -- 
> 2.25.1
> 
> 

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

[PATCH 2/3] i2c:imx: Add an extra read at the end of an I2C slave read

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The I2C slave interface expects that the driver will read ahead one
byte.  The IMX driver/device doesn't do this, but simulate it so that
read operations get their index set correctly.

Signed-off-by: Corey Minyard <minyard@acm.org>
Tested-by: Andrew Manley <andrew.manley@sealingtech.com>
Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com>
---
 drivers/i2c/busses/i2c-imx.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 97369fe48b30..26a04dc0590b 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -769,6 +769,15 @@ static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx,
 		ctl &= ~I2CR_MTX;
 		imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
 		imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
+
+		/*
+		 * The i2c slave interface requires one extra dummy
+		 * read at the end to keep things in line.  See the
+		 * I2C slave docs for details.
+		 */
+		i2c_imx_slave_event(i2c_imx,
+				    I2C_SLAVE_READ_PROCESSED, &value);
+
 		i2c_imx_slave_finish_op(i2c_imx);
 		return IRQ_HANDLED;
 	}
-- 
2.25.1

Re: [PATCH 2/3] i2c:imx: Add an extra read at the end of an I2C slave read

[Oleksij Rempel]

On Mon, Oct 04, 2021 at 07:32:15PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> The I2C slave interface expects that the driver will read ahead one
> byte.  The IMX driver/device doesn't do this, but simulate it so that
> read operations get their index set correctly.
> 
> Signed-off-by: Corey Minyard <minyard@acm.org>
> Tested-by: Andrew Manley <andrew.manley@sealingtech.com>
> Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com>

Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>

> ---
>  drivers/i2c/busses/i2c-imx.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 97369fe48b30..26a04dc0590b 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -769,6 +769,15 @@ static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx,
>  		ctl &= ~I2CR_MTX;
>  		imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
>  		imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> +
> +		/*
> +		 * The i2c slave interface requires one extra dummy
> +		 * read at the end to keep things in line.  See the
> +		 * I2C slave docs for details.
> +		 */
> +		i2c_imx_slave_event(i2c_imx,
> +				    I2C_SLAVE_READ_PROCESSED, &value);
> +
>  		i2c_imx_slave_finish_op(i2c_imx);
>  		return IRQ_HANDLED;
>  	}
> -- 
> 2.25.1
> 
> 

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

[PATCH 3/3] i2c:imx: Use an hrtimer, not a timer, for checking for bus idle

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The timer is too slow and significantly reduces performance.  Use an
hrtimer to get things working faster.

Signed-off-by: Corey Minyard <minyard@acm.org>
Tested-by: Andrew Manley <andrew.manley@sealingtech.com>
Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com>
---
 drivers/i2c/busses/i2c-imx.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 26a04dc0590b..4b0e9d1784dd 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -38,7 +38,7 @@
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/spinlock.h>
-#include <linux/timer.h>
+#include <linux/hrtimer.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -53,6 +53,8 @@
 /* This will be the driver name the kernel reports */
 #define DRIVER_NAME "imx-i2c"
 
+#define I2C_IMX_CHECK_DELAY 30000 /* Time to check for bus idle, in NS */
+
 /*
  * Enable DMA if transfer byte size is bigger than this threshold.
  * As the hardware request, it must bigger than 4 bytes.\
@@ -214,8 +216,8 @@ struct imx_i2c_struct {
 	enum i2c_slave_event last_slave_event;
 
 	/* For checking slave events. */
-	spinlock_t	  slave_lock;
-	struct timer_list slave_timer;
+	spinlock_t     slave_lock;
+	struct hrtimer slave_timer;
 };
 
 static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
@@ -783,13 +785,16 @@ static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx,
 	}
 
 out:
-	mod_timer(&i2c_imx->slave_timer, jiffies + 1);
+	hrtimer_try_to_cancel(&i2c_imx->slave_timer);
+	hrtimer_forward_now(&i2c_imx->slave_timer, I2C_IMX_CHECK_DELAY);
+	hrtimer_restart(&i2c_imx->slave_timer);
 	return IRQ_HANDLED;
 }
 
-static void i2c_imx_slave_timeout(struct timer_list *t)
+static enum hrtimer_restart i2c_imx_slave_timeout(struct hrtimer *t)
 {
-	struct imx_i2c_struct *i2c_imx = from_timer(i2c_imx, t, slave_timer);
+	struct imx_i2c_struct *i2c_imx = container_of(t, struct imx_i2c_struct,
+						      slave_timer);
 	unsigned int ctl, status;
 	unsigned long flags;
 
@@ -798,6 +803,7 @@ static void i2c_imx_slave_timeout(struct timer_list *t)
 	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 	i2c_imx_slave_handle(i2c_imx, status, ctl);
 	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
+	return HRTIMER_NORESTART;
 }
 
 static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx)
@@ -1423,7 +1429,8 @@ static int i2c_imx_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	spin_lock_init(&i2c_imx->slave_lock);
-	timer_setup(&i2c_imx->slave_timer, i2c_imx_slave_timeout, 0);
+	hrtimer_init(&i2c_imx->slave_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
+	i2c_imx->slave_timer.function = i2c_imx_slave_timeout;
 
 	match = device_get_match_data(&pdev->dev);
 	if (match)
@@ -1538,7 +1545,7 @@ static int i2c_imx_remove(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	del_timer_sync(&i2c_imx->slave_timer);
+	hrtimer_cancel(&i2c_imx->slave_timer);
 
 	/* remove adapter */
 	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
-- 
2.25.1

Re: [PATCH 3/3] i2c:imx: Use an hrtimer, not a timer, for checking for bus idle

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]

On Mon, Oct 04, 2021 at 07:32:16PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> The timer is too slow and significantly reduces performance.  Use an
> hrtimer to get things working faster.
> 
> Signed-off-by: Corey Minyard <minyard@acm.org>
> Tested-by: Andrew Manley <andrew.manley@sealingtech.com>
> Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com>
> ---
>  drivers/i2c/busses/i2c-imx.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 26a04dc0590b..4b0e9d1784dd 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -38,7 +38,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
>  #include <linux/spinlock.h>
> -#include <linux/timer.h>
> +#include <linux/hrtimer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -53,6 +53,8 @@
>  /* This will be the driver name the kernel reports */
>  #define DRIVER_NAME "imx-i2c"
>  
> +#define I2C_IMX_CHECK_DELAY 30000 /* Time to check for bus idle, in NS */
> +
>  /*
>   * Enable DMA if transfer byte size is bigger than this threshold.
>   * As the hardware request, it must bigger than 4 bytes.\
> @@ -214,8 +216,8 @@ struct imx_i2c_struct {
>  	enum i2c_slave_event last_slave_event;
>  
>  	/* For checking slave events. */
> -	spinlock_t	  slave_lock;
> -	struct timer_list slave_timer;
> +	spinlock_t     slave_lock;
> +	struct hrtimer slave_timer;

This is unrelated to this patch, moreover it was introduced only in
patch 1.

>  };
>  
>  static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> @@ -783,13 +785,16 @@ static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx,
>  	}
>  
>  out:
> -	mod_timer(&i2c_imx->slave_timer, jiffies + 1);
> +	hrtimer_try_to_cancel(&i2c_imx->slave_timer);

Don't you need to check the return value here?

> +	hrtimer_forward_now(&i2c_imx->slave_timer, I2C_IMX_CHECK_DELAY);
> +	hrtimer_restart(&i2c_imx->slave_timer);
>  	return IRQ_HANDLED;
>  }
>  

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] i2c:imx: Use an hrtimer, not a timer, for checking for bus idle

[Corey Minyard]

On Tue, Nov 02, 2021 at 09:58:06AM +0100, Uwe Kleine-König wrote:
> On Mon, Oct 04, 2021 at 07:32:16PM -0500, minyard@acm.org wrote:
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > The timer is too slow and significantly reduces performance.  Use an
> > hrtimer to get things working faster.
> > 
> > Signed-off-by: Corey Minyard <minyard@acm.org>
> > Tested-by: Andrew Manley <andrew.manley@sealingtech.com>
> > Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com>
> > ---
> >  drivers/i2c/busses/i2c-imx.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > index 26a04dc0590b..4b0e9d1784dd 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -38,7 +38,7 @@
> >  #include <linux/iopoll.h>
> >  #include <linux/kernel.h>
> >  #include <linux/spinlock.h>
> > -#include <linux/timer.h>
> > +#include <linux/hrtimer.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > @@ -53,6 +53,8 @@
> >  /* This will be the driver name the kernel reports */
> >  #define DRIVER_NAME "imx-i2c"
> >  
> > +#define I2C_IMX_CHECK_DELAY 30000 /* Time to check for bus idle, in NS */
> > +
> >  /*
> >   * Enable DMA if transfer byte size is bigger than this threshold.
> >   * As the hardware request, it must bigger than 4 bytes.\
> > @@ -214,8 +216,8 @@ struct imx_i2c_struct {
> >  	enum i2c_slave_event last_slave_event;
> >  
> >  	/* For checking slave events. */
> > -	spinlock_t	  slave_lock;
> > -	struct timer_list slave_timer;
> > +	spinlock_t     slave_lock;
> > +	struct hrtimer slave_timer;
> 
> This is unrelated to this patch, moreover it was introduced only in
> patch 1.

The second line is important for this patch, of course.  I assume you
mean the indention of the first line, which is just keeping things lined
up.

> 
> >  };
> >  
> >  static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> > @@ -783,13 +785,16 @@ static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx,
> >  	}
> >  
> >  out:
> > -	mod_timer(&i2c_imx->slave_timer, jiffies + 1);
> > +	hrtimer_try_to_cancel(&i2c_imx->slave_timer);
> 
> Don't you need to check the return value here?

Not really.  The possible return values are:

 *  *  0 when the timer was not active
 *  *  1 when the timer was active
 *  * -1 when the timer is currently executing the callback function and
 *    cannot be stopped

and if it returns 0 or 1, then everything is fine.  If it returns -1,
then the code will still work, though it may be redone (or already have
been done) by the timer function.  So it doesn't matter.

Maybe I should add a comment about this?

Thanks for reviewing.

-corey

> 
> > +	hrtimer_forward_now(&i2c_imx->slave_timer, I2C_IMX_CHECK_DELAY);
> > +	hrtimer_restart(&i2c_imx->slave_timer);
> >  	return IRQ_HANDLED;
> >  }
> >  
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 3/3] i2c:imx: Use an hrtimer, not a timer, for checking for bus idle

[Oleksij Rempel]

On Mon, Oct 04, 2021 at 07:32:16PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> The timer is too slow and significantly reduces performance.  Use an
> hrtimer to get things working faster.
> 
> Signed-off-by: Corey Minyard <minyard@acm.org>
> Tested-by: Andrew Manley <andrew.manley@sealingtech.com>
> Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com>

Do we need to keep this change history? If no, please merge it to the
first patch.

> ---
>  drivers/i2c/busses/i2c-imx.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 26a04dc0590b..4b0e9d1784dd 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -38,7 +38,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
>  #include <linux/spinlock.h>
> -#include <linux/timer.h>
> +#include <linux/hrtimer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -53,6 +53,8 @@
>  /* This will be the driver name the kernel reports */
>  #define DRIVER_NAME "imx-i2c"
>  
> +#define I2C_IMX_CHECK_DELAY 30000 /* Time to check for bus idle, in NS */
> +
>  /*
>   * Enable DMA if transfer byte size is bigger than this threshold.
>   * As the hardware request, it must bigger than 4 bytes.\
> @@ -214,8 +216,8 @@ struct imx_i2c_struct {
>  	enum i2c_slave_event last_slave_event;
>  
>  	/* For checking slave events. */
> -	spinlock_t	  slave_lock;
> -	struct timer_list slave_timer;
> +	spinlock_t     slave_lock;
> +	struct hrtimer slave_timer;
>  };
>  
>  static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> @@ -783,13 +785,16 @@ static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx,
>  	}
>  
>  out:
> -	mod_timer(&i2c_imx->slave_timer, jiffies + 1);
> +	hrtimer_try_to_cancel(&i2c_imx->slave_timer);
> +	hrtimer_forward_now(&i2c_imx->slave_timer, I2C_IMX_CHECK_DELAY);
> +	hrtimer_restart(&i2c_imx->slave_timer);
>  	return IRQ_HANDLED;
>  }
>  
> -static void i2c_imx_slave_timeout(struct timer_list *t)
> +static enum hrtimer_restart i2c_imx_slave_timeout(struct hrtimer *t)
>  {
> -	struct imx_i2c_struct *i2c_imx = from_timer(i2c_imx, t, slave_timer);
> +	struct imx_i2c_struct *i2c_imx = container_of(t, struct imx_i2c_struct,
> +						      slave_timer);
>  	unsigned int ctl, status;
>  	unsigned long flags;
>  
> @@ -798,6 +803,7 @@ static void i2c_imx_slave_timeout(struct timer_list *t)
>  	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>  	i2c_imx_slave_handle(i2c_imx, status, ctl);
>  	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
> +	return HRTIMER_NORESTART;
>  }
>  
>  static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx)
> @@ -1423,7 +1429,8 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	spin_lock_init(&i2c_imx->slave_lock);
> -	timer_setup(&i2c_imx->slave_timer, i2c_imx_slave_timeout, 0);
> +	hrtimer_init(&i2c_imx->slave_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> +	i2c_imx->slave_timer.function = i2c_imx_slave_timeout;
>  
>  	match = device_get_match_data(&pdev->dev);
>  	if (match)
> @@ -1538,7 +1545,7 @@ static int i2c_imx_remove(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> -	del_timer_sync(&i2c_imx->slave_timer);
> +	hrtimer_cancel(&i2c_imx->slave_timer);
>  
>  	/* remove adapter */
>  	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
> -- 
> 2.25.1
> 
> 

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

Re: [PATCH 3/3] i2c:imx: Use an hrtimer, not a timer, for checking for bus idle

[Corey Minyard]

On Wed, Nov 10, 2021 at 10:03:38AM +0100, Oleksij Rempel wrote:
> On Mon, Oct 04, 2021 at 07:32:16PM -0500, minyard@acm.org wrote:
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > The timer is too slow and significantly reduces performance.  Use an
> > hrtimer to get things working faster.
> > 
> > Signed-off-by: Corey Minyard <minyard@acm.org>
> > Tested-by: Andrew Manley <andrew.manley@sealingtech.com>
> > Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com>
> 
> Do we need to keep this change history? If no, please merge it to the
> first patch.

Yeah, I can do that.  It make sense.

Thanks,

-corey

> 
> > ---
> >  drivers/i2c/busses/i2c-imx.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > index 26a04dc0590b..4b0e9d1784dd 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -38,7 +38,7 @@
> >  #include <linux/iopoll.h>
> >  #include <linux/kernel.h>
> >  #include <linux/spinlock.h>
> > -#include <linux/timer.h>
> > +#include <linux/hrtimer.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > @@ -53,6 +53,8 @@
> >  /* This will be the driver name the kernel reports */
> >  #define DRIVER_NAME "imx-i2c"
> >  
> > +#define I2C_IMX_CHECK_DELAY 30000 /* Time to check for bus idle, in NS */
> > +
> >  /*
> >   * Enable DMA if transfer byte size is bigger than this threshold.
> >   * As the hardware request, it must bigger than 4 bytes.\
> > @@ -214,8 +216,8 @@ struct imx_i2c_struct {
> >  	enum i2c_slave_event last_slave_event;
> >  
> >  	/* For checking slave events. */
> > -	spinlock_t	  slave_lock;
> > -	struct timer_list slave_timer;
> > +	spinlock_t     slave_lock;
> > +	struct hrtimer slave_timer;
> >  };
> >  
> >  static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> > @@ -783,13 +785,16 @@ static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx,
> >  	}
> >  
> >  out:
> > -	mod_timer(&i2c_imx->slave_timer, jiffies + 1);
> > +	hrtimer_try_to_cancel(&i2c_imx->slave_timer);
> > +	hrtimer_forward_now(&i2c_imx->slave_timer, I2C_IMX_CHECK_DELAY);
> > +	hrtimer_restart(&i2c_imx->slave_timer);
> >  	return IRQ_HANDLED;
> >  }
> >  
> > -static void i2c_imx_slave_timeout(struct timer_list *t)
> > +static enum hrtimer_restart i2c_imx_slave_timeout(struct hrtimer *t)
> >  {
> > -	struct imx_i2c_struct *i2c_imx = from_timer(i2c_imx, t, slave_timer);
> > +	struct imx_i2c_struct *i2c_imx = container_of(t, struct imx_i2c_struct,
> > +						      slave_timer);
> >  	unsigned int ctl, status;
> >  	unsigned long flags;
> >  
> > @@ -798,6 +803,7 @@ static void i2c_imx_slave_timeout(struct timer_list *t)
> >  	ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> >  	i2c_imx_slave_handle(i2c_imx, status, ctl);
> >  	spin_unlock_irqrestore(&i2c_imx->slave_lock, flags);
> > +	return HRTIMER_NORESTART;
> >  }
> >  
> >  static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx)
> > @@ -1423,7 +1429,8 @@ static int i2c_imx_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  
> >  	spin_lock_init(&i2c_imx->slave_lock);
> > -	timer_setup(&i2c_imx->slave_timer, i2c_imx_slave_timeout, 0);
> > +	hrtimer_init(&i2c_imx->slave_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> > +	i2c_imx->slave_timer.function = i2c_imx_slave_timeout;
> >  
> >  	match = device_get_match_data(&pdev->dev);
> >  	if (match)
> > @@ -1538,7 +1545,7 @@ static int i2c_imx_remove(struct platform_device *pdev)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	del_timer_sync(&i2c_imx->slave_timer);
> > +	hrtimer_cancel(&i2c_imx->slave_timer);
> >  
> >  	/* remove adapter */
> >  	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
> > -- 
> > 2.25.1
> > 
> > 
> 
> -- 
> 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 |