* [PATCH] i2c: imx: add support for I2C_M_STOP flag
@ 2023-07-06 12:57 Mans Rullgard
2023-07-14 13:22 ` Oleksij Rempel
2023-07-27 20:30 ` Andi Shyti
0 siblings, 2 replies; 4+ messages in thread
From: Mans Rullgard @ 2023-07-06 12:57 UTC (permalink / raw)
To: Oleksij Rempel, Pengutronix Kernel Team
Cc: Andi Shyti, Shawn Guo, Sascha Hauer, Fabio Estevam,
NXP Linux Team, linux-i2c, linux-arm-kernel, linux-kernel
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(-)
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;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] i2c: imx: add support for I2C_M_STOP flag
2023-07-06 12:57 [PATCH] i2c: imx: add support for I2C_M_STOP flag Mans Rullgard
@ 2023-07-14 13:22 ` Oleksij Rempel
2023-07-27 20:30 ` Andi Shyti
1 sibling, 0 replies; 4+ messages in thread
From: Oleksij Rempel @ 2023-07-14 13:22 UTC (permalink / raw)
To: Mans Rullgard
Cc: Oleksij Rempel, Pengutronix Kernel Team, Andi Shyti, Shawn Guo,
Sascha Hauer, linux-kernel, linux-i2c, Fabio Estevam,
linux-arm-kernel, NXP Linux Team
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
--
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 |
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] i2c: imx: add support for I2C_M_STOP flag
2023-07-06 12:57 [PATCH] i2c: imx: add support for I2C_M_STOP flag Mans Rullgard
2023-07-14 13:22 ` Oleksij Rempel
@ 2023-07-27 20:30 ` Andi Shyti
1 sibling, 0 replies; 4+ messages in thread
From: Andi Shyti @ 2023-07-27 20:30 UTC (permalink / raw)
To: Mans Rullgard
Cc: Oleksij Rempel, Pengutronix Kernel Team, Shawn Guo, Sascha Hauer,
Fabio Estevam, NXP Linux Team, linux-i2c, linux-arm-kernel,
linux-kernel
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 */
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] i2c: imx: add support for I2C_M_STOP flag
@ 2019-02-27 18:21 Mans Rullgard
0 siblings, 0 replies; 4+ messages in thread
From: Mans Rullgard @ 2019-02-27 18:21 UTC (permalink / raw)
To: linux-i2c; +Cc: linux-kernel
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(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index c406700789e1..43b4d5bc2196 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -908,10 +908,17 @@ static int i2c_imx_xfer(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);
+ 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);
@@ -921,6 +928,10 @@ static int i2c_imx_xfer(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 */
@@ -948,6 +959,13 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
result = i2c_imx_dma_write(i2c_imx, &msgs[i]);
else
result = i2c_imx_write(i2c_imx, &msgs[i]);
+
+ 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);
+ }
}
if (result)
goto fail0;
@@ -1034,7 +1052,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.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-07-27 20:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-06 12:57 [PATCH] i2c: imx: add support for I2C_M_STOP flag Mans Rullgard
2023-07-14 13:22 ` Oleksij Rempel
2023-07-27 20:30 ` Andi Shyti
-- strict thread matches above, loose matches on Subject: below --
2019-02-27 18:21 Mans Rullgard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).