diff mbox series

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

Message ID 20211112133956.655179-3-minyard@acm.org
State Superseded
Delegated to: Wolfram Sang
Headers show
Series i2c:imx: Deliver a timely stop on slave side, fix recv | expand

Commit Message

Corey Minyard Nov. 12, 2021, 1:39 p.m. UTC 
From: Corey Minyard <minyard@acm.org>

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(+)

Comments

Wolfram Sang Nov. 23, 2021, 10:28 a.m. UTC | #1 
On Fri, Nov 12, 2021 at 07:39:56AM -0600, minyard@acm.org wrote:
> From: Corey Minyard <minyard@acm.org>
> 
> 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.

From what I understand, the patch is correct but the description may be
wrong?

AFAIU the patch adds the slave event I2C_SLAVE_READ_PROCESSED to the
case when the last byte was transferred. We as the client got a NAK from
the controller. However, the byte WAS processed, so the event is ok and
not a dummy?
Corey Minyard Nov. 23, 2021, 12:44 p.m. UTC | #2 
On Tue, Nov 23, 2021 at 11:28:41AM +0100, Wolfram Sang wrote:
> On Fri, Nov 12, 2021 at 07:39:56AM -0600, minyard@acm.org wrote:
> > From: Corey Minyard <minyard@acm.org>
> > 
> > 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.
> 
> From what I understand, the patch is correct but the description may be
> wrong?
> 
> AFAIU the patch adds the slave event I2C_SLAVE_READ_PROCESSED to the
> case when the last byte was transferred. We as the client got a NAK from
> the controller. However, the byte WAS processed, so the event is ok and
> not a dummy?
> 

I think the description is correct.  Devices that are read from (which
is just eeprom at the moment) expect that there is a dummy read at the
end of a read transaction.  Apparently this is what at least some slave
hardware does.  The I2C device being read doesn't know when the master
device will finish the operation.  So to be ready for the next read it
always reads ahead one.  When the read is terminated by the master,
there is an extra byte left lying around that is discarded.

The IMX driver doesn't work this way.  So when I was testing, I noticed
that if I did two reads in a row it was one byte off on the second read.

-corey
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 27f969b3dc07..41355fc8bff4 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -771,6 +771,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;
 	}