| Message ID | 20230706125729.17335-1-mans@mansr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: imx: add support for I2C_M_STOP flag | expand |
Hi Mans, Thank you for you patch. On Thu, Jul 06, 2023 at 01:57:29PM +0100, Mans Rullgard wrote: > Add support for the I2C_M_STOP flag to the i2c-imx driver. This allows > devices requiring a stop between messages to work with this controller. Can you please add more description to the commit message. For example which i2c client drivers need this functionality and what HW (imx SoC variant) was tested with this patch. > Signed-off-by: Mans Rullgard <mans@mansr.com> > --- > drivers/i2c/busses/i2c-imx.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index 42189a5f2905..f59d086688ff 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -1262,10 +1262,17 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter, > > /* read/write data */ > for (i = 0; i < num; i++) { > - if (i == num - 1) > - is_lastmsg = true; > + if (is_lastmsg) { > + /* previous message had I2C_M_STOP flag set */ > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > + temp |= I2CR_MSTA; > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > + result = i2c_imx_bus_busy(i2c_imx, 1, atomic); > + if (result) > + goto fail0; > + } > > - if (i) { > + if (i && !is_lastmsg) { > dev_dbg(&i2c_imx->adapter.dev, > "<%s> repeated start\n", __func__); > temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > @@ -1275,6 +1282,10 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter, > if (result) > goto fail0; > } Here some issues should be fixed: - Reusing is_lastmsg to signal STOP signal for not last msg is confusing. May be is_stopped will be OK. - Please combine if statements for I2CR_MSTA and I2CR_RSTA case. One of both should be used on not first message. I2CR_MSTA if I2CR_MSTA was cleared on previous round, or I2CR_RSTA if I2CR_MSTA was not cleared before. In current implementation this patch will fail to set Repeated Start condition if first message was with the I2C_M_STOP flag, but other messages do not have this flag. For example: When Message 1 finishes with the I2C_M_STOP flag, it will clear the I2CR_MSTA flag, which changes the mode to Slave mode. Message 2, not having an I2C_M_STOP flag, will simply set the I2CR_MSTA flag again, starting a new transaction with a new Start condition, not requiring a Repeated Start (I2CR_RSTA). However, by Message 3, the issue arises. Since Message 2 does not clear I2CR_MSTA (as it lacks the I2C_M_STOP flag), setting I2CR_MSTA again in Message 3 will have no effect because it's already set. But the problem here is that the "is_lastmsg" flag is still true from Message 1, which causes the driver to skip setting the I2CR_RSTA flag. Therefore, a Repeated Start condition is not generated between Message 2 and 3, which is a problematic behavior if the device expects a Repeated Start condition between the two messages. > + if (i == num - 1 || (msgs[i].flags & I2C_M_STOP)) > + is_lastmsg = true; > + > dev_dbg(&i2c_imx->adapter.dev, > "<%s> transfer message: %d\n", __func__, i); > /* write/read data */ > @@ -1304,6 +1315,13 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter, > result = i2c_imx_dma_write(i2c_imx, &msgs[i]); > else > result = i2c_imx_write(i2c_imx, &msgs[i], atomic); > + > + if (msgs[i].flags & I2C_M_STOP) { > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > + temp &= ~I2CR_MSTA; > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > + i2c_imx_bus_busy(i2c_imx, 0, atomic); > + } > } > if (result) > goto fail0; > @@ -1425,7 +1443,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx, > > static u32 i2c_imx_func(struct i2c_adapter *adapter) > { > - return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL > + return I2C_FUNC_I2C | I2C_FUNC_PROTOCOL_MANGLING | I2C_FUNC_SMBUS_EMUL > | I2C_FUNC_SMBUS_READ_BLOCK_DATA; > } > > -- > 2.41.0 Regards, Oleksij
Hi Mans, on top of Oleksij's comments... [...] > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -1262,10 +1262,17 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter, > > /* read/write data */ > for (i = 0; i < num; i++) { > - if (i == num - 1) > - is_lastmsg = true; > + if (is_lastmsg) { > + /* previous message had I2C_M_STOP flag set */ > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > + temp |= I2CR_MSTA; > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > + result = i2c_imx_bus_busy(i2c_imx, 1, atomic); > + if (result) > + goto fail0; > + } > > - if (i) { > + if (i && !is_lastmsg) { } else if (i) { looks a bit simplier to me. > dev_dbg(&i2c_imx->adapter.dev, > "<%s> repeated start\n", __func__); > temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > @@ -1275,6 +1282,10 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter, > if (result) > goto fail0; > } > + > + if (i == num - 1 || (msgs[i].flags & I2C_M_STOP)) > + is_lastmsg = true; you don't need this "i == num - 1" here. Andi > dev_dbg(&i2c_imx->adapter.dev, > "<%s> transfer message: %d\n", __func__, i); > /* write/read data */
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 42189a5f2905..f59d086688ff 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -1262,10 +1262,17 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter, /* read/write data */ for (i = 0; i < num; i++) { - if (i == num - 1) - is_lastmsg = true; + if (is_lastmsg) { + /* previous message had I2C_M_STOP flag set */ + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); + temp |= I2CR_MSTA; + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); + result = i2c_imx_bus_busy(i2c_imx, 1, atomic); + if (result) + goto fail0; + } - if (i) { + if (i && !is_lastmsg) { dev_dbg(&i2c_imx->adapter.dev, "<%s> repeated start\n", __func__); temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); @@ -1275,6 +1282,10 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter, if (result) goto fail0; } + + if (i == num - 1 || (msgs[i].flags & I2C_M_STOP)) + is_lastmsg = true; + dev_dbg(&i2c_imx->adapter.dev, "<%s> transfer message: %d\n", __func__, i); /* write/read data */ @@ -1304,6 +1315,13 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter, result = i2c_imx_dma_write(i2c_imx, &msgs[i]); else result = i2c_imx_write(i2c_imx, &msgs[i], atomic); + + if (msgs[i].flags & I2C_M_STOP) { + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); + temp &= ~I2CR_MSTA; + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); + i2c_imx_bus_busy(i2c_imx, 0, atomic); + } } if (result) goto fail0; @@ -1425,7 +1443,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx, static u32 i2c_imx_func(struct i2c_adapter *adapter) { - return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL + return I2C_FUNC_I2C | I2C_FUNC_PROTOCOL_MANGLING | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_READ_BLOCK_DATA; }
Add support for the I2C_M_STOP flag to the i2c-imx driver. This allows devices requiring a stop between messages to work with this controller. Signed-off-by: Mans Rullgard <mans@mansr.com> --- drivers/i2c/busses/i2c-imx.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)