| Message ID | 1363261750-26645-1-git-send-email-l.stach@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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
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
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; }
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(-)