* [PATCH RESEND v2] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART
@ 2026-10-23 8:05 ` Vincent Jardin
0 siblings, 0 replies; 4+ messages in thread
From: Vincent Jardin via B4 Relay @ 2026-10-23 8:05 UTC (permalink / raw)
To: Oleksij Rempel, Pengutronix Kernel Team, Andi Shyti, Frank Li,
Sascha Hauer, Fabio Estevam
Cc: linux-i2c, imx, linux-arm-kernel, linux-kernel, Carlos Song,
Haobin Jiang, Vincent Jardin
From: Vincent Jardin <vjardin@free.fr>
The Realtek RTL8366SE SMI read frame per:
S Addr Rd [A] reg[7:0] [A] reg[15:8] [A] [data[7:0]] A [data[15:8]] NA P
Linux support it using I2C_M_REV_DIR_ADDR on a write message
that inverts the transmitted R/W bit, and I2C_M_NOSTART on the following
read message that continues the frame without re-addressing.
This NXP i2c-imx is missing such support, so on an LX2160A the Realtek switch
could not be used.
Acked-by: Carlos Song <carlos.song@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Vincent Jardin <vjardin@free.fr>
---
Follow the logics from i2c-algo-bit and i2c-s3c2410 (addr ^= 1).
Documentation/i2c/i2c-protocol.rst says I2C_M_NOSTART "may also be used
between direction changes by some rare devices"; the RTL8366SE is one of
such device !
- The NOSTART read turns the bus around without waiting for a completion,
because the controller stretches SCL after the previous byte and the
frame is therefore still open. That is what lets i2c_imx_read() enter
the state the ISR would otherwise have moved to.
- I2C_FUNC_PROTOCOL_MANGLING also covers I2C_M_IGNORE_NAK,
I2C_M_NO_RD_ACK and I2C_M_STOP, which this driver does not implement.
They are rejected with -EOPNOTSUPP rather than advertised and silently
ignored. I2C_M_NOSTART on the first message is rejected as well.
---
RESEND:
- No change since v2 of 2026-08-12, only added:
Acked-by from Carlos Song and Reviewed-by from Frank Li
- Resent as a new thread
- Haobin Jiang's "i2c: imx: honour I2C_M_IGNORE_NAK" v3 is built on top of it:
https://lore.kernel.org/r/tencent_342A87BFCECA70C9982E68A79AAD878B6909@qq.com
- Link to v2: https://lore.kernel.org/r/20260812-for-upstream-i2c-imx-lx2160-reverse-v2-1-f1343714c5a9@free.fr
Changes in v2:
- message validation with i2c_imx_check_msgs() (Carlos Song)
No functional change
- Link to v1: https://lore.kernel.org/r/20260807-for-upstream-i2c-imx-lx2160-reverse-v1-1-f276ce8cb660@free.fr
---
drivers/i2c/busses/i2c-imx.c | 153 ++++++++++++++++++++++++++++++++-----------
1 file changed, 113 insertions(+), 40 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 9477d814fde9..7dc9c43673ee 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1000,16 +1000,10 @@ static inline int i2c_imx_isr_write(struct imx_i2c_struct *i2c_imx)
return 1;
}
-static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
+static inline void i2c_imx_setup_read(struct imx_i2c_struct *i2c_imx)
{
- int result;
unsigned int temp;
- result = i2c_imx_isr_acked(i2c_imx);
- if (result)
- return result;
-
- /* setup bus to read data */
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
temp &= ~I2CR_MTX;
if ((i2c_imx->msg->len - 1) || (i2c_imx->msg->flags & I2C_M_RECV_LEN))
@@ -1017,6 +1011,18 @@ static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
+}
+
+static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
+{
+ int result;
+
+ result = i2c_imx_isr_acked(i2c_imx);
+ if (result)
+ return result;
+
+ /* setup bus to read data */
+ i2c_imx_setup_read(i2c_imx);
return 0;
}
@@ -1172,6 +1178,16 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
return i2c_imx_master_isr(i2c_imx, status);
}
+static u8 i2c_imx_addr_byte(struct i2c_msg *msg)
+{
+ u8 addr = i2c_8bit_addr_from_msg(msg);
+
+ if (msg->flags & I2C_M_REV_DIR_ADDR)
+ addr ^= 1;
+
+ return addr;
+}
+
static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
struct i2c_msg *msgs)
{
@@ -1200,7 +1216,7 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
* Write slave address.
* The first byte must be transmitted by the CPU.
*/
- imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
+ imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
time_left = wait_for_completion_timeout(
&i2c_imx->dma->cmd_complete,
msecs_to_jiffies(DMA_TIMEOUT));
@@ -1242,14 +1258,20 @@ static int i2c_imx_prepare_read(struct imx_i2c_struct *i2c_imx,
int result;
unsigned int temp = 0;
- /* write slave address */
- imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
- result = i2c_imx_trx_complete(i2c_imx, !use_dma);
- if (result)
- return result;
- result = i2c_imx_acked(i2c_imx);
- if (result)
- return result;
+ /*
+ * I2C_M_NOSTART continues a frame that is already open, so there is
+ * no address phase: go straight to turning the bus around.
+ */
+ if (!(msgs->flags & I2C_M_NOSTART)) {
+ /* write slave address */
+ imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
+ result = i2c_imx_trx_complete(i2c_imx, !use_dma);
+ if (result)
+ return result;
+ result = i2c_imx_acked(i2c_imx);
+ if (result)
+ return result;
+ }
dev_dbg(&i2c_imx->adapter.dev, "<%s> setup bus\n", __func__);
@@ -1371,16 +1393,18 @@ static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
int i, result;
dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n",
- __func__, i2c_8bit_addr_from_msg(msgs));
+ __func__, i2c_imx_addr_byte(msgs));
- /* write slave address */
- imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
- result = i2c_imx_trx_complete(i2c_imx, true);
- if (result)
- return result;
- result = i2c_imx_acked(i2c_imx);
- if (result)
- return result;
+ if (!(msgs->flags & I2C_M_NOSTART)) {
+ /* write slave address */
+ imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
+ result = i2c_imx_trx_complete(i2c_imx, true);
+ if (result)
+ return result;
+ result = i2c_imx_acked(i2c_imx);
+ if (result)
+ return result;
+ }
dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__);
/* write data */
@@ -1402,7 +1426,7 @@ static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
{
dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n",
- __func__, i2c_8bit_addr_from_msg(msgs));
+ __func__, i2c_imx_addr_byte(msgs));
i2c_imx->state = IMX_I2C_STATE_WRITE;
i2c_imx->msg = msgs;
@@ -1411,8 +1435,16 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
/*
* By writing the device address we start the state machine in the ISR.
* The ISR will report when it is done or when it fails.
+ *
+ * I2C_M_NOSTART continues a frame that is already open and so has no
+ * address byte: push the first data byte instead. That raises the same
+ * interrupt and the ISR carries on from the second byte.
*/
- imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
+ if (msgs->flags & I2C_M_NOSTART)
+ imx_i2c_write_reg(msgs->buf[i2c_imx->msg_buf_idx++], i2c_imx,
+ IMX_I2C_I2DR);
+ else
+ imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
wait_event_timeout(i2c_imx->queue,
i2c_imx->state == IMX_I2C_STATE_DONE ||
i2c_imx->state == IMX_I2C_STATE_FAILED,
@@ -1529,22 +1561,29 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
dev_dbg(&i2c_imx->adapter.dev,
"<%s> write slave address: addr=0x%x\n",
- __func__, i2c_8bit_addr_from_msg(msgs));
+ __func__, i2c_imx_addr_byte(msgs));
i2c_imx->is_lastmsg = is_lastmsg;
- if (block_data)
- i2c_imx->state = IMX_I2C_STATE_READ_BLOCK_DATA;
- else
- i2c_imx->state = IMX_I2C_STATE_READ;
i2c_imx->msg = msgs;
i2c_imx->msg_buf_idx = 0;
- /*
- * By writing the device address we start the state machine in the ISR.
- * The ISR will report when it is done or when it fails.
- */
- imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
+ if (msgs->flags & I2C_M_NOSTART) {
+ i2c_imx->state = block_data ? IMX_I2C_STATE_READ_BLOCK_DATA_LEN
+ : IMX_I2C_STATE_READ_CONTINUE;
+ i2c_imx_setup_read(i2c_imx);
+ } else {
+ if (block_data)
+ i2c_imx->state = IMX_I2C_STATE_READ_BLOCK_DATA;
+ else
+ i2c_imx->state = IMX_I2C_STATE_READ;
+
+ /*
+ * By writing the device address we start the state machine in the ISR.
+ * The ISR will report when it is done or when it fails.
+ */
+ imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
+ }
wait_event_timeout(i2c_imx->queue,
i2c_imx->state == IMX_I2C_STATE_DONE ||
i2c_imx->state == IMX_I2C_STATE_FAILED,
@@ -1574,6 +1613,33 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
return ret;
}
+#define I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS \
+ (I2C_M_IGNORE_NAK | I2C_M_NO_RD_ACK | I2C_M_STOP)
+
+static int i2c_imx_check_msgs(struct i2c_msg *msgs, int num)
+{
+ int i;
+
+ for (i = 0; i < num; i++) {
+ /* Reject rather than silently transfer */
+ if (msgs[i].flags & I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS)
+ return -EOPNOTSUPP;
+
+ if (msgs[i].flags & I2C_M_NOSTART) {
+ /*
+ * NOSTART continues an already open frame. The first
+ * message has nothing to continue from,
+ * a 0 length continuation leaves the controller with
+ * no byte to clock.
+ */
+ if (i == 0 || msgs[i].len == 0)
+ return -EOPNOTSUPP;
+ }
+ }
+
+ return 0;
+}
+
static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
struct i2c_msg *msgs, int num, bool atomic)
{
@@ -1583,6 +1649,10 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
int use_dma = 0;
+ result = i2c_imx_check_msgs(msgs, num);
+ if (result)
+ return result;
+
/* Start I2C transfer */
result = i2c_imx_start(i2c_imx, atomic);
if (result) {
@@ -1604,7 +1674,7 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
if (i == num - 1)
is_lastmsg = true;
- if (i) {
+ if (i && !(msgs[i].flags & I2C_M_NOSTART)) {
dev_dbg(&i2c_imx->adapter.dev,
"<%s> repeated start\n", __func__);
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
@@ -1636,7 +1706,8 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
#endif
use_dma = i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD &&
- msgs[i].flags & I2C_M_DMA_SAFE;
+ msgs[i].flags & I2C_M_DMA_SAFE &&
+ !(msgs[i].flags & I2C_M_NOSTART);
if (msgs[i].flags & I2C_M_RD) {
int block_data = msgs->flags & I2C_M_RECV_LEN;
@@ -1730,7 +1801,9 @@ 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
- | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
+ | I2C_FUNC_SMBUS_READ_BLOCK_DATA
+ | I2C_FUNC_NOSTART
+ | I2C_FUNC_PROTOCOL_MANGLING;
}
static const struct i2c_algorithm i2c_imx_algo = {
---
base-commit: 3d6d817622b0a9721e3cc404df3469171582be13
change-id: 20260807-for-upstream-i2c-imx-lx2160-reverse-7a94b9561188
Best regards,
--
Vincent Jardin <vjardin@free.fr>
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH RESEND v2] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART
@ 2026-10-23 8:05 ` Vincent Jardin
0 siblings, 0 replies; 4+ messages in thread
From: Vincent Jardin @ 2026-10-23 8:05 UTC (permalink / raw)
To: Oleksij Rempel, Pengutronix Kernel Team, Andi Shyti, Frank Li,
Sascha Hauer, Fabio Estevam
Cc: linux-i2c, imx, linux-arm-kernel, linux-kernel, Carlos Song,
Haobin Jiang, Vincent Jardin
The Realtek RTL8366SE SMI read frame per:
S Addr Rd [A] reg[7:0] [A] reg[15:8] [A] [data[7:0]] A [data[15:8]] NA P
Linux support it using I2C_M_REV_DIR_ADDR on a write message
that inverts the transmitted R/W bit, and I2C_M_NOSTART on the following
read message that continues the frame without re-addressing.
This NXP i2c-imx is missing such support, so on an LX2160A the Realtek switch
could not be used.
Acked-by: Carlos Song <carlos.song@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Vincent Jardin <vjardin@free.fr>
---
Follow the logics from i2c-algo-bit and i2c-s3c2410 (addr ^= 1).
Documentation/i2c/i2c-protocol.rst says I2C_M_NOSTART "may also be used
between direction changes by some rare devices"; the RTL8366SE is one of
such device !
- The NOSTART read turns the bus around without waiting for a completion,
because the controller stretches SCL after the previous byte and the
frame is therefore still open. That is what lets i2c_imx_read() enter
the state the ISR would otherwise have moved to.
- I2C_FUNC_PROTOCOL_MANGLING also covers I2C_M_IGNORE_NAK,
I2C_M_NO_RD_ACK and I2C_M_STOP, which this driver does not implement.
They are rejected with -EOPNOTSUPP rather than advertised and silently
ignored. I2C_M_NOSTART on the first message is rejected as well.
---
RESEND:
- No change since v2 of 2026-08-12, only added:
Acked-by from Carlos Song and Reviewed-by from Frank Li
- Resent as a new thread
- Haobin Jiang's "i2c: imx: honour I2C_M_IGNORE_NAK" v3 is built on top of it:
https://lore.kernel.org/r/tencent_342A87BFCECA70C9982E68A79AAD878B6909@qq.com
- Link to v2: https://lore.kernel.org/r/20260812-for-upstream-i2c-imx-lx2160-reverse-v2-1-f1343714c5a9@free.fr
Changes in v2:
- message validation with i2c_imx_check_msgs() (Carlos Song)
No functional change
- Link to v1: https://lore.kernel.org/r/20260807-for-upstream-i2c-imx-lx2160-reverse-v1-1-f276ce8cb660@free.fr
---
drivers/i2c/busses/i2c-imx.c | 153 ++++++++++++++++++++++++++++++++-----------
1 file changed, 113 insertions(+), 40 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 9477d814fde9..7dc9c43673ee 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1000,16 +1000,10 @@ static inline int i2c_imx_isr_write(struct imx_i2c_struct *i2c_imx)
return 1;
}
-static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
+static inline void i2c_imx_setup_read(struct imx_i2c_struct *i2c_imx)
{
- int result;
unsigned int temp;
- result = i2c_imx_isr_acked(i2c_imx);
- if (result)
- return result;
-
- /* setup bus to read data */
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
temp &= ~I2CR_MTX;
if ((i2c_imx->msg->len - 1) || (i2c_imx->msg->flags & I2C_M_RECV_LEN))
@@ -1017,6 +1011,18 @@ static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
+}
+
+static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
+{
+ int result;
+
+ result = i2c_imx_isr_acked(i2c_imx);
+ if (result)
+ return result;
+
+ /* setup bus to read data */
+ i2c_imx_setup_read(i2c_imx);
return 0;
}
@@ -1172,6 +1178,16 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
return i2c_imx_master_isr(i2c_imx, status);
}
+static u8 i2c_imx_addr_byte(struct i2c_msg *msg)
+{
+ u8 addr = i2c_8bit_addr_from_msg(msg);
+
+ if (msg->flags & I2C_M_REV_DIR_ADDR)
+ addr ^= 1;
+
+ return addr;
+}
+
static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
struct i2c_msg *msgs)
{
@@ -1200,7 +1216,7 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
* Write slave address.
* The first byte must be transmitted by the CPU.
*/
- imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
+ imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
time_left = wait_for_completion_timeout(
&i2c_imx->dma->cmd_complete,
msecs_to_jiffies(DMA_TIMEOUT));
@@ -1242,14 +1258,20 @@ static int i2c_imx_prepare_read(struct imx_i2c_struct *i2c_imx,
int result;
unsigned int temp = 0;
- /* write slave address */
- imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
- result = i2c_imx_trx_complete(i2c_imx, !use_dma);
- if (result)
- return result;
- result = i2c_imx_acked(i2c_imx);
- if (result)
- return result;
+ /*
+ * I2C_M_NOSTART continues a frame that is already open, so there is
+ * no address phase: go straight to turning the bus around.
+ */
+ if (!(msgs->flags & I2C_M_NOSTART)) {
+ /* write slave address */
+ imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
+ result = i2c_imx_trx_complete(i2c_imx, !use_dma);
+ if (result)
+ return result;
+ result = i2c_imx_acked(i2c_imx);
+ if (result)
+ return result;
+ }
dev_dbg(&i2c_imx->adapter.dev, "<%s> setup bus\n", __func__);
@@ -1371,16 +1393,18 @@ static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
int i, result;
dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n",
- __func__, i2c_8bit_addr_from_msg(msgs));
+ __func__, i2c_imx_addr_byte(msgs));
- /* write slave address */
- imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
- result = i2c_imx_trx_complete(i2c_imx, true);
- if (result)
- return result;
- result = i2c_imx_acked(i2c_imx);
- if (result)
- return result;
+ if (!(msgs->flags & I2C_M_NOSTART)) {
+ /* write slave address */
+ imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
+ result = i2c_imx_trx_complete(i2c_imx, true);
+ if (result)
+ return result;
+ result = i2c_imx_acked(i2c_imx);
+ if (result)
+ return result;
+ }
dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__);
/* write data */
@@ -1402,7 +1426,7 @@ static int i2c_imx_atomic_write(struct imx_i2c_struct *i2c_imx,
static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
{
dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n",
- __func__, i2c_8bit_addr_from_msg(msgs));
+ __func__, i2c_imx_addr_byte(msgs));
i2c_imx->state = IMX_I2C_STATE_WRITE;
i2c_imx->msg = msgs;
@@ -1411,8 +1435,16 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
/*
* By writing the device address we start the state machine in the ISR.
* The ISR will report when it is done or when it fails.
+ *
+ * I2C_M_NOSTART continues a frame that is already open and so has no
+ * address byte: push the first data byte instead. That raises the same
+ * interrupt and the ISR carries on from the second byte.
*/
- imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
+ if (msgs->flags & I2C_M_NOSTART)
+ imx_i2c_write_reg(msgs->buf[i2c_imx->msg_buf_idx++], i2c_imx,
+ IMX_I2C_I2DR);
+ else
+ imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
wait_event_timeout(i2c_imx->queue,
i2c_imx->state == IMX_I2C_STATE_DONE ||
i2c_imx->state == IMX_I2C_STATE_FAILED,
@@ -1529,22 +1561,29 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
dev_dbg(&i2c_imx->adapter.dev,
"<%s> write slave address: addr=0x%x\n",
- __func__, i2c_8bit_addr_from_msg(msgs));
+ __func__, i2c_imx_addr_byte(msgs));
i2c_imx->is_lastmsg = is_lastmsg;
- if (block_data)
- i2c_imx->state = IMX_I2C_STATE_READ_BLOCK_DATA;
- else
- i2c_imx->state = IMX_I2C_STATE_READ;
i2c_imx->msg = msgs;
i2c_imx->msg_buf_idx = 0;
- /*
- * By writing the device address we start the state machine in the ISR.
- * The ISR will report when it is done or when it fails.
- */
- imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
+ if (msgs->flags & I2C_M_NOSTART) {
+ i2c_imx->state = block_data ? IMX_I2C_STATE_READ_BLOCK_DATA_LEN
+ : IMX_I2C_STATE_READ_CONTINUE;
+ i2c_imx_setup_read(i2c_imx);
+ } else {
+ if (block_data)
+ i2c_imx->state = IMX_I2C_STATE_READ_BLOCK_DATA;
+ else
+ i2c_imx->state = IMX_I2C_STATE_READ;
+
+ /*
+ * By writing the device address we start the state machine in the ISR.
+ * The ISR will report when it is done or when it fails.
+ */
+ imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
+ }
wait_event_timeout(i2c_imx->queue,
i2c_imx->state == IMX_I2C_STATE_DONE ||
i2c_imx->state == IMX_I2C_STATE_FAILED,
@@ -1574,6 +1613,33 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
return ret;
}
+#define I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS \
+ (I2C_M_IGNORE_NAK | I2C_M_NO_RD_ACK | I2C_M_STOP)
+
+static int i2c_imx_check_msgs(struct i2c_msg *msgs, int num)
+{
+ int i;
+
+ for (i = 0; i < num; i++) {
+ /* Reject rather than silently transfer */
+ if (msgs[i].flags & I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS)
+ return -EOPNOTSUPP;
+
+ if (msgs[i].flags & I2C_M_NOSTART) {
+ /*
+ * NOSTART continues an already open frame. The first
+ * message has nothing to continue from,
+ * a 0 length continuation leaves the controller with
+ * no byte to clock.
+ */
+ if (i == 0 || msgs[i].len == 0)
+ return -EOPNOTSUPP;
+ }
+ }
+
+ return 0;
+}
+
static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
struct i2c_msg *msgs, int num, bool atomic)
{
@@ -1583,6 +1649,10 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
int use_dma = 0;
+ result = i2c_imx_check_msgs(msgs, num);
+ if (result)
+ return result;
+
/* Start I2C transfer */
result = i2c_imx_start(i2c_imx, atomic);
if (result) {
@@ -1604,7 +1674,7 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
if (i == num - 1)
is_lastmsg = true;
- if (i) {
+ if (i && !(msgs[i].flags & I2C_M_NOSTART)) {
dev_dbg(&i2c_imx->adapter.dev,
"<%s> repeated start\n", __func__);
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
@@ -1636,7 +1706,8 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
#endif
use_dma = i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD &&
- msgs[i].flags & I2C_M_DMA_SAFE;
+ msgs[i].flags & I2C_M_DMA_SAFE &&
+ !(msgs[i].flags & I2C_M_NOSTART);
if (msgs[i].flags & I2C_M_RD) {
int block_data = msgs->flags & I2C_M_RECV_LEN;
@@ -1730,7 +1801,9 @@ 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
- | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
+ | I2C_FUNC_SMBUS_READ_BLOCK_DATA
+ | I2C_FUNC_NOSTART
+ | I2C_FUNC_PROTOCOL_MANGLING;
}
static const struct i2c_algorithm i2c_imx_algo = {
---
base-commit: 3d6d817622b0a9721e3cc404df3469171582be13
change-id: 20260807-for-upstream-i2c-imx-lx2160-reverse-7a94b9561188
Best regards,
--
Vincent Jardin <vjardin@free.fr>
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RESEND v2] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART
2026-10-23 8:05 ` Vincent Jardin
(?)
@ 2026-10-23 8:34 ` sashiko-bot
2026-10-23 14:25 ` Vincent Jardin
-1 siblings, 1 reply; 4+ messages in thread
From: sashiko-bot @ 2026-10-23 8:34 UTC (permalink / raw)
To: Vincent Jardin; +Cc: imx, Frank.Li
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] i2c: imx: premature NACK on read messages followed by I2C_M_RD | I2C_M_NOSTART
--
commit cc53d2cc1adbe1a23f92c25512b4a73a38c5f255
Author: Vincent Jardin <vjardin@free.fr>
i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART
This commit adds support for I2C_M_REV_DIR_ADDR and I2C_M_NOSTART to the
i2c-imx driver so that devices such as the Realtek RTL8366SE switch can
perform SMI read frames. It also adds message validation in
i2c_imx_check_msgs() and advertises I2C_FUNC_NOSTART and
I2C_FUNC_PROTOCOL_MANGLING.
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 9477d814fde94..7dc9c43673eee 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
[ ... ]
> @@ -1529,22 +1561,29 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
>
> dev_dbg(&i2c_imx->adapter.dev,
> "<%s> write slave address: addr=0x%x\n",
> - __func__, i2c_8bit_addr_from_msg(msgs));
> + __func__, i2c_imx_addr_byte(msgs));
>
> i2c_imx->is_lastmsg = is_lastmsg;
>
> - if (block_data)
> - i2c_imx->state = IMX_I2C_STATE_READ_BLOCK_DATA;
> - else
> - i2c_imx->state = IMX_I2C_STATE_READ;
> i2c_imx->msg = msgs;
> i2c_imx->msg_buf_idx = 0;
>
> - /*
> - * By writing the device address we start the state machine in the ISR.
> - * The ISR will report when it is done or when it fails.
> - */
> - imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
> + if (msgs->flags & I2C_M_NOSTART) {
> + i2c_imx->state = block_data ? IMX_I2C_STATE_READ_BLOCK_DATA_LEN
> + : IMX_I2C_STATE_READ_CONTINUE;
> + i2c_imx_setup_read(i2c_imx);
> + } else {
> + if (block_data)
> + i2c_imx->state = IMX_I2C_STATE_READ_BLOCK_DATA;
> + else
> + i2c_imx->state = IMX_I2C_STATE_READ;
> +
> + /*
> + * By writing the device address we start the state machine in the ISR.
> + * The ISR will report when it is done or when it fails.
> + */
> + imx_i2c_write_reg(i2c_imx_addr_byte(msgs), i2c_imx, IMX_I2C_I2DR);
> + }
[ ... ]
> @@ -1574,6 +1613,33 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs,
> return ret;
> }
>
> +#define I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS \
> + (I2C_M_IGNORE_NAK | I2C_M_NO_RD_ACK | I2C_M_STOP)
> +
> +static int i2c_imx_check_msgs(struct i2c_msg *msgs, int num)
> +{
> + int i;
> +
> + for (i = 0; i < num; i++) {
> + /* Reject rather than silently transfer */
> + if (msgs[i].flags & I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS)
> + return -EOPNOTSUPP;
> +
> + if (msgs[i].flags & I2C_M_NOSTART) {
> + /*
> + * NOSTART continues an already open frame. The first
> + * message has nothing to continue from,
> + * a 0 length continuation leaves the controller with
> + * no byte to clock.
> + */
> + if (i == 0 || msgs[i].len == 0)
> + return -EOPNOTSUPP;
> + }
> + }
> +
> + return 0;
> +}
[Severity: Medium]
What happens if a caller splits a read across two messages, passing
msgs[i - 1] with I2C_M_RD followed by msgs[i] with I2C_M_RD | I2C_M_NOSTART?
In i2c_imx_check_msgs(), that sequence is accepted as long as i > 0 and
msgs[i].len > 0. However, the read paths for msgs[i - 1]
(i2c_imx_setup_read(), i2c_imx_isr_read_continue(), i2c_imx_prepare_read(),
i2c_imx_atomic_read(), and i2c_imx_dma_read()) unconditionally set I2CR_TXAK
on the last byte of msgs[i - 1] and set I2CR_MTX before reading the final byte
from IMX_I2C_I2DR:
drivers/i2c/busses/i2c-imx.c:i2c_imx_isr_read_continue() {
if ((i2c_imx->msg->len - 1) == i2c_imx->msg_buf_idx) {
...
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
temp |= I2CR_MTX;
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
next_state = IMX_I2C_STATE_DONE;
} else if (i2c_imx->msg_buf_idx == (i2c_imx->msg->len - 2)) {
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
temp |= I2CR_TXAK;
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
}
}
Because the controller sends a NACK at the end of msgs[i - 1], the target
device aborts transmission and releases SDA. When msgs[i] then runs
i2c_imx_setup_read() (or i2c_imx_prepare_read() in atomic mode), it clears
I2CR_MTX, performs another dummy read of IMX_I2C_I2DR, and clocks in 0xff for
the remainder of the transfer.
Should i2c_imx_check_msgs() reject I2C_M_RD | I2C_M_NOSTART when preceded by
an I2C_M_RD message (or conversely, when a non-final read message is followed
by I2C_M_NOSTART), or should the read state machine suppress I2CR_TXAK,
I2CR_MTX, and the second dummy read across the message boundary?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-for-upstream-i2c-imx-lx2160-reverse-v2-1-03afae754886@free.fr?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RESEND v2] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART
2026-10-23 8:34 ` sashiko-bot
@ 2026-10-23 14:25 ` Vincent Jardin
0 siblings, 0 replies; 4+ messages in thread
From: Vincent Jardin @ 2026-10-23 14:25 UTC (permalink / raw)
To: sashiko-reviews; +Cc: imx, Frank.Li
Hi M. Bot,
> > +}
>
> [Severity: Medium]
> What happens if a caller splits a read across two messages, passing
> msgs[i - 1] with I2C_M_RD followed by msgs[i] with I2C_M_RD | I2C_M_NOSTART?
>
> In i2c_imx_check_msgs(), that sequence is accepted as long as i > 0 and
> msgs[i].len > 0. However, the read paths for msgs[i - 1]
> (i2c_imx_setup_read(), i2c_imx_isr_read_continue(), i2c_imx_prepare_read(),
> i2c_imx_atomic_read(), and i2c_imx_dma_read()) unconditionally set I2CR_TXAK
> on the last byte of msgs[i - 1] and set I2CR_MTX before reading the final byte
> from IMX_I2C_I2DR:
>
> drivers/i2c/busses/i2c-imx.c:i2c_imx_isr_read_continue() {
> if ((i2c_imx->msg->len - 1) == i2c_imx->msg_buf_idx) {
> ...
> temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> temp |= I2CR_MTX;
> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> next_state = IMX_I2C_STATE_DONE;
> } else if (i2c_imx->msg_buf_idx == (i2c_imx->msg->len - 2)) {
> temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> temp |= I2CR_TXAK;
> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> }
> }
>
> Because the controller sends a NACK at the end of msgs[i - 1], the target
> device aborts transmission and releases SDA. When msgs[i] then runs
> i2c_imx_setup_read() (or i2c_imx_prepare_read() in atomic mode), it clears
> I2CR_MTX, performs another dummy read of IMX_I2C_I2DR, and clocks in 0xff for
> the remainder of the transfer.
>
> Should i2c_imx_check_msgs() reject I2C_M_RD | I2C_M_NOSTART when preceded by
> an I2C_M_RD message (or conversely, when a non-final read message is followed
> by I2C_M_NOSTART), or should the read state machine suppress I2CR_TXAK,
> I2CR_MTX, and the second dummy read across the message boundary?
hmm, ok, updated into the v3
thank you,
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-10-23 14:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-23 8:05 [PATCH RESEND v2] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART Vincent Jardin via B4 Relay
2026-10-23 8:05 ` Vincent Jardin
2026-10-23 8:34 ` sashiko-bot
2026-10-23 14:25 ` Vincent Jardin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.