2021-10-05 00:33:32

by Corey Minyard

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

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




2021-10-05 00:34:31

by Corey Minyard

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

From: Corey Minyard <[email protected]>

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 <[email protected]>
Tested-by: Andrew Manley <[email protected]>
Reviewed-by: Andrew Manley <[email protected]>
---
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

2021-10-05 00:35:21

by Corey Minyard

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

From: Corey Minyard <[email protected]>

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

Signed-off-by: Corey Minyard <[email protected]>
Tested-by: Andrew Manley <[email protected]>
Reviewed-by: Andrew Manley <[email protected]>
---
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

2021-10-05 00:36:14

by Corey Minyard

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

From: Corey Minyard <[email protected]>

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 <[email protected]>
Tested-by: Andrew Manley <[email protected]>
Reviewed-by: Andrew Manley <[email protected]>
---
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

2021-11-01 17:29:26

by Corey Minyard

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

On Mon, Oct 04, 2021 at 07:32:14PM -0500, [email protected] wrote:
> From: Corey Minyard <[email protected]>

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 <[email protected]>
> Tested-by: Andrew Manley <[email protected]>
> Reviewed-by: Andrew Manley <[email protected]>
> ---
> 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
>

2021-11-02 09:01:13

by Uwe Kleine-König

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

On Mon, Oct 04, 2021 at 07:32:16PM -0500, [email protected] wrote:
> From: Corey Minyard <[email protected]>
>
> The timer is too slow and significantly reduces performance. Use an
> hrtimer to get things working faster.
>
> Signed-off-by: Corey Minyard <[email protected]>
> Tested-by: Andrew Manley <[email protected]>
> Reviewed-by: Andrew Manley <[email protected]>
> ---
> 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/ |


Attachments:
(No filename) (2.30 kB)
signature.asc (499.00 B)
Download all attachments

2021-11-02 11:53:15

by Corey Minyard

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

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, [email protected] wrote:
> > From: Corey Minyard <[email protected]>
> >
> > The timer is too slow and significantly reduces performance. Use an
> > hrtimer to get things working faster.
> >
> > Signed-off-by: Corey Minyard <[email protected]>
> > Tested-by: Andrew Manley <[email protected]>
> > Reviewed-by: Andrew Manley <[email protected]>
> > ---
> > 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/ |


2021-11-10 09:03:25

by Oleksij Rempel

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

On Mon, Oct 04, 2021 at 07:32:15PM -0500, [email protected] wrote:
> From: Corey Minyard <[email protected]>
>
> 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 <[email protected]>
> Tested-by: Andrew Manley <[email protected]>
> Reviewed-by: Andrew Manley <[email protected]>

Reviewed-by: Oleksij Rempel <[email protected]>

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

2021-11-10 09:03:47

by Oleksij Rempel

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

On Mon, Oct 04, 2021 at 07:32:14PM -0500, [email protected] wrote:
> From: Corey Minyard <[email protected]>
>
> 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 <[email protected]>
> Tested-by: Andrew Manley <[email protected]>
> Reviewed-by: Andrew Manley <[email protected]>

Reviewed-by: Oleksij Rempel <[email protected]>

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 |

2021-11-10 09:05:44

by Oleksij Rempel

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

On Mon, Oct 04, 2021 at 07:32:16PM -0500, [email protected] wrote:
> From: Corey Minyard <[email protected]>
>
> The timer is too slow and significantly reduces performance. Use an
> hrtimer to get things working faster.
>
> Signed-off-by: Corey Minyard <[email protected]>
> Tested-by: Andrew Manley <[email protected]>
> Reviewed-by: Andrew Manley <[email protected]>

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 |

2021-11-10 14:49:00

by Corey Minyard

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

On Wed, Nov 10, 2021 at 10:03:38AM +0100, Oleksij Rempel wrote:
> On Mon, Oct 04, 2021 at 07:32:16PM -0500, [email protected] wrote:
> > From: Corey Minyard <[email protected]>
> >
> > The timer is too slow and significantly reduces performance. Use an
> > hrtimer to get things working faster.
> >
> > Signed-off-by: Corey Minyard <[email protected]>
> > Tested-by: Andrew Manley <[email protected]>
> > Reviewed-by: Andrew Manley <[email protected]>
>
> 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 |