Patchwork [1/3] i2c: mxs: always end a transfer with a proper STOP

login
register
mail settings
Submitter Lucas Stach
Date March 14, 2013, 11:49 a.m.
Message ID <1363261750-26645-1-git-send-email-l.stach@pengutronix.de>
Download mbox | patch
Permalink /patch/227631/
State Superseded
Headers show

Comments

Lucas Stach - March 14, 2013, 11:49 a.m.
Our transfers always start with the device address, so there is never a
situation where we just do a restart transfer. Full blown transfers should
always end with a STOP as per i2c spec.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/i2c/busses/i2c-mxs.c |   32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)
Wolfram Sang - April 8, 2013, 5:21 p.m.
On Thu, Mar 14, 2013 at 12:49:08PM +0100, Lucas Stach wrote:
> Our transfers always start with the device address, so there is never a
> situation where we just do a restart transfer. Full blown transfers should
> always end with a STOP as per i2c spec.

? I don't get the description. Does "restart transfer" mean repeated
start? What has the device address to do with it?

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lucas Stach - April 9, 2013, 7:26 a.m.
Am Montag, den 08.04.2013, 19:21 +0200 schrieb Wolfram Sang:
> On Thu, Mar 14, 2013 at 12:49:08PM +0100, Lucas Stach wrote:
> > Our transfers always start with the device address, so there is never a
> > situation where we just do a restart transfer. Full blown transfers should
> > always end with a STOP as per i2c spec.
> 
> ? I don't get the description. Does "restart transfer" mean repeated
> start? What has the device address to do with it?
> 

A restart transfer is when you just repeat the START condition, without
putting the device address on the bus again.

In the MXS driver we put the device address on the bus for every
transaction we get handed in from the i2c core, so there is never a
situation where we just repeat the start condition without sending out
the device address. Before this patch we would not match every
transaction, but only the last in the list of pending ones, with a STOP
condition, which is a violation of the spec.

If there are no other comments, I'll send out a V2 today, to take in
Marek's remarks.
Wolfram Sang - April 9, 2013, 8:32 a.m.
Hi,

> A restart transfer is when you just repeat the START condition, without
> putting the device address on the bus again.

Well, never heard this term before. Where did you get it from?

> In the MXS driver we put the device address on the bus for every
> transaction we get handed in from the i2c core, so there is never a
> situation where we just repeat the start condition without sending out
> the device address. Before this patch we would not match every
> transaction, but only the last in the list of pending ones, with a STOP
> condition, which is a violation of the spec.

I still don't get it. You can drop a STOP if you replace it with
a repeated start. In fact, this is crucial in multi-master setups,
otherwise another master could break into your transfer containing
multilple messages. So, if MXS does the right thing on sending START
(doing a correct start sequence), we should not send STOP. If it needs
the STOP to create a correct START, then be it. But then, I'd wonder why
it worked so far...

Regards,

   Wolfram

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lucas Stach - April 15, 2013, 7:50 a.m.
Hi Wolfram,

Am Dienstag, den 09.04.2013, 10:32 +0200 schrieb Wolfram Sang:
> Hi,
> 
> > A restart transfer is when you just repeat the START condition, without
> > putting the device address on the bus again.
> 
> Well, never heard this term before. Where did you get it from?
> 
> > In the MXS driver we put the device address on the bus for every
> > transaction we get handed in from the i2c core, so there is never a
> > situation where we just repeat the start condition without sending out
> > the device address. Before this patch we would not match every
> > transaction, but only the last in the list of pending ones, with a STOP
> > condition, which is a violation of the spec.
> 
> I still don't get it. You can drop a STOP if you replace it with
> a repeated start. In fact, this is crucial in multi-master setups,
> otherwise another master could break into your transfer containing
> multilple messages. So, if MXS does the right thing on sending START
> (doing a correct start sequence), we should not send STOP. If it needs
> the STOP to create a correct START, then be it. But then, I'd wonder why
> it worked so far...
> 

Ok, I looked this up again and got a nice explanation by Uwe and it
seems I based this patch on a wrong interpretation of the spec on my
side. I'll resend without this one.

Regards,
Lucas

Patch

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 120f246..f9704b2 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -87,10 +87,12 @@ 
 				 MXS_I2C_CTRL0_XFER_COUNT(1))
 
 #define MXS_CMD_I2C_WRITE	(MXS_I2C_CTRL0_PRE_SEND_START |	\
+				 MXS_I2C_CTRL0_POST_SEND_STOP | \
 				 MXS_I2C_CTRL0_MASTER_MODE |	\
 				 MXS_I2C_CTRL0_DIRECTION)
 
 #define MXS_CMD_I2C_READ	(MXS_I2C_CTRL0_SEND_NAK_ON_LAST | \
+				 MXS_I2C_CTRL0_POST_SEND_STOP | \
 				 MXS_I2C_CTRL0_MASTER_MODE)
 
 /**
@@ -158,8 +160,7 @@  static void mxs_i2c_dma_irq_callback(void *param)
 	mxs_i2c_dma_finish(i2c);
 }
 
-static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
-			struct i2c_msg *msg, uint32_t flags)
+static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap, struct i2c_msg *msg)
 {
 	struct dma_async_tx_descriptor *desc;
 	struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
@@ -200,7 +201,7 @@  static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
 		 */
 
 		/* Queue the PIO register write transfer. */
-		i2c->pio_data[1] = flags | MXS_CMD_I2C_READ |
+		i2c->pio_data[1] = MXS_CMD_I2C_READ |
 				MXS_I2C_CTRL0_XFER_COUNT(msg->len);
 		desc = dmaengine_prep_slave_sg(i2c->dmach,
 					(struct scatterlist *)&i2c->pio_data[1],
@@ -231,7 +232,7 @@  static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap,
 		 */
 
 		/* Queue the PIO register write transfer. */
-		i2c->pio_data[0] = flags | MXS_CMD_I2C_WRITE |
+		i2c->pio_data[0] = MXS_CMD_I2C_WRITE |
 				MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1);
 		desc = dmaengine_prep_slave_sg(i2c->dmach,
 					(struct scatterlist *)&i2c->pio_data[0],
@@ -326,8 +327,7 @@  static int mxs_i2c_pio_wait_cplt(struct mxs_i2c_dev *i2c)
 	return 0;
 }
 
-static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
-			struct i2c_msg *msg, uint32_t flags)
+static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap, struct i2c_msg *msg)
 {
 	struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
 	uint32_t addr_data = msg->addr << 1;
@@ -355,7 +355,7 @@  static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 			return ret;
 
 		/* READ command. */
-		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_READ | flags |
+		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_READ |
 			MXS_I2C_CTRL0_XFER_COUNT(msg->len),
 			i2c->regs + MXS_I2C_CTRL0);
 
@@ -373,7 +373,7 @@  static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 		addr_data |= I2C_SMBUS_WRITE;
 
 		/* WRITE command. */
-		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_WRITE | flags |
+		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_WRITE |
 			MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1),
 			i2c->regs + MXS_I2C_CTRL0);
 
@@ -418,17 +418,13 @@  static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 /*
  * Low level master read/write transaction.
  */
-static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
-				int stop)
+static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg)
 {
 	struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
 	int ret;
-	int flags;
 
-	flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0;
-
-	dev_dbg(i2c->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
-		msg->addr, msg->len, msg->flags, stop);
+	dev_dbg(i2c->dev, "addr: 0x%04x, len: %d, flags: 0x%x\n",
+		msg->addr, msg->len, msg->flags);
 
 	if (msg->len == 0)
 		return -EINVAL;
@@ -440,13 +436,13 @@  static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 	 * based on this empirical measurement and a lot of previous frobbing.
 	 */
 	if (msg->len < 8) {
-		ret = mxs_i2c_pio_setup_xfer(adap, msg, flags);
+		ret = mxs_i2c_pio_setup_xfer(adap, msg);
 		if (ret)
 			mxs_i2c_reset(i2c);
 	} else {
 		i2c->cmd_err = 0;
 		INIT_COMPLETION(i2c->cmd_complete);
-		ret = mxs_i2c_dma_setup_xfer(adap, msg, flags);
+		ret = mxs_i2c_dma_setup_xfer(adap, msg);
 		if (ret)
 			return ret;
 
@@ -479,7 +475,7 @@  static int mxs_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 	int err;
 
 	for (i = 0; i < num; i++) {
-		err = mxs_i2c_xfer_msg(adap, &msgs[i], i == (num - 1));
+		err = mxs_i2c_xfer_msg(adap, &msgs[i]);
 		if (err)
 			return err;
 	}