Linux I2C development
 help / color / mirror / Atom feed
* [PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART
@ 2026-08-06 22:37 Vincent Jardin via B4 Relay
  2026-08-11 10:18 ` Carlos Song (OSS)
  0 siblings, 1 reply; 4+ messages in thread
From: Vincent Jardin via B4 Relay @ 2026-08-06 22:37 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, 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.

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.
---
 drivers/i2c/busses/i2c-imx.c | 143 +++++++++++++++++++++++++++++++------------
 1 file changed, 103 insertions(+), 40 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 9477d814fde9..81c1bd461606 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,
@@ -1583,6 +1622,27 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
 	struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
 	int use_dma = 0;
 
+	for (i = 0; i < num; i++) {
+		/*
+		 * I2C_FUNC_PROTOCOL_MANGLING is advertised for
+		 * I2C_M_REV_DIR_ADDR only; reject the flags it also covers but
+		 * that this driver does not implement, rather than silently
+		 * transferring something the caller did not ask for.
+		 */
+		if (msgs[i].flags & (I2C_M_IGNORE_NAK | I2C_M_NO_RD_ACK |
+				     I2C_M_STOP))
+			return -EOPNOTSUPP;
+
+		if (!(msgs[i].flags & I2C_M_NOSTART))
+			continue;
+
+		/* Nothing to continue from, and a zero-length continuation
+		 * would leave the controller with no byte to clock.
+		 */
+		if (i == 0 || msgs[i].len == 0)
+			return -EOPNOTSUPP;
+	}
+
 	/* Start I2C transfer */
 	result = i2c_imx_start(i2c_imx, atomic);
 	if (result) {
@@ -1604,7 +1664,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 +1696,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 +1791,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: 6c68fa601b49683ecb04eded993a71dfa8b2ba0b
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] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART
  2026-08-06 22:37 [PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART Vincent Jardin via B4 Relay
@ 2026-08-11 10:18 ` Carlos Song (OSS)
  2026-08-12  5:47   ` Vincent Jardin
  0 siblings, 1 reply; 4+ messages in thread
From: Carlos Song (OSS) @ 2026-08-11 10:18 UTC (permalink / raw)
  To: vjardin@free.fr, Oleksij Rempel, Pengutronix Kernel Team,
	Andi Shyti, Frank Li, Sascha Hauer, Fabio Estevam
  Cc: linux-i2c@vger.kernel.org, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org



> -----Original Message-----
> From: Vincent Jardin via B4 Relay <devnull+vjardin.free.fr@kernel.org>
> Sent: Friday, August 7, 2026 6:38 AM
> To: Oleksij Rempel <o.rempel@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Andi Shyti <andi.shyti@kernel.org>; Frank Li
> <frank.li@nxp.com>; Sascha Hauer <s.hauer@pengutronix.de>; Fabio
> Estevam <festevam@gmail.com>
> Cc: linux-i2c@vger.kernel.org; imx@lists.linux.dev;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Vincent
> Jardin <vjardin@free.fr>
> Subject: [PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART
> 
> 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.
>


Hi, Vincent,

Thank you very much for your fix. Sorry for late ack.

This is a such rare i2c frame design in the Realtek RTL8366SE SMI read frame, right?

So you add I2C_FUNC_NOSTART | I2C_FUNC_PROTOCOL_MANGLING in i2c imx functionality.
Then support I2C_M_REV_DIR_ADDR flag check to flip read/write bit and support I2C_M_NOSTART to skip the
next start head, right? Have you test this i2c-imx feature in your LS board with RTL8366SE?

Is Realtek RTL8366SE SMI driver upstream? Can I found the driver?
I want to see this driver how to prepare and handle the i2c msg in this special frame.

Carlos Song
> 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.
> ---
>  drivers/i2c/busses/i2c-imx.c | 143
> +++++++++++++++++++++++++++++++------------
>  1 file changed, 103 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index
> 9477d814fde9..81c1bd461606 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, @@ -1583,6
> +1622,27 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
>  	struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
>  	int use_dma = 0;
> 
> +	for (i = 0; i < num; i++) {
> +		/*
> +		 * I2C_FUNC_PROTOCOL_MANGLING is advertised for
> +		 * I2C_M_REV_DIR_ADDR only; reject the flags it also covers but
> +		 * that this driver does not implement, rather than silently
> +		 * transferring something the caller did not ask for.
> +		 */
> +		if (msgs[i].flags & (I2C_M_IGNORE_NAK | I2C_M_NO_RD_ACK |
> +				     I2C_M_STOP))
> +			return -EOPNOTSUPP;
> +
> +		if (!(msgs[i].flags & I2C_M_NOSTART))
> +			continue;
> +
> +		/* Nothing to continue from, and a zero-length continuation
> +		 * would leave the controller with no byte to clock.
> +		 */
> +		if (i == 0 || msgs[i].len == 0)
> +			return -EOPNOTSUPP;
> +	}
> +
>  	/* Start I2C transfer */
>  	result = i2c_imx_start(i2c_imx, atomic);
>  	if (result) {
> @@ -1604,7 +1664,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 +1696,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 +1791,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: 6c68fa601b49683ecb04eded993a71dfa8b2ba0b
> change-id: 20260807-for-upstream-i2c-imx-lx2160-reverse-7a94b9561188
> 
> Best regards,
> --
> Vincent Jardin <vjardin@free.fr>
> 
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART
  2026-08-11 10:18 ` Carlos Song (OSS)
@ 2026-08-12  5:47   ` Vincent Jardin
  2026-08-12  9:20     ` Carlos Song (OSS)
  0 siblings, 1 reply; 4+ messages in thread
From: Vincent Jardin @ 2026-08-12  5:47 UTC (permalink / raw)
  To: Carlos Song (OSS)
  Cc: Oleksij Rempel, Pengutronix Kernel Team, Andi Shyti, Frank Li,
	Sascha Hauer, Fabio Estevam, linux-i2c@vger.kernel.org,
	imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org

Hi Carlos,

Thanks a lot for the review, and no problem for the delay.

> This is a such rare i2c frame design in the Realtek RTL8366SE SMI read
> frame, right?

It is rare, but I have been inspired for I2C_M_REV_DIR_ADDR by the source
of two client drivers that use it.

Note that I2C_M_NOSTART is not rare at all. It is what regmap uses for a real
gather write. regmap_i2c_gather_write() opens with

        if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_NOSTART))
                return -ENOTSUPP;

FYI, some clients that need REV_DIR_ADDR and NOSTART,

  drivers/input/joystick/as5011.c
  drivers/video/fbdev/matrox/matroxfb_maven.c

both build the identical 2-message pattern,

  i2c_check_functionality(adapter,
                                I2C_FUNC_NOSTART |
                                I2C_FUNC_PROTOCOL_MANGLING)

I do not have those devices, so I did not check it beside code readings.

Some clients that need NOSTART alone,

  drivers/base/regmap/regmap-i2c.c
  drivers/infiniband/hw/hfi1/qsfp.c
  drivers/gpu/drm/i915/display/dvo_ivch.c

About the i2c masters, that use I2C_M_REV_DIR_ADDR in code,

  drivers/i2c/algos/i2c-algo-bit.c
  drivers/i2c/algos/i2c-algo-pcf.c
  drivers/i2c/busses/i2c-s3c2410.c <- the model I did investigate
  drivers/i2c/busses/i2c-tegra-bpmp.c
  drivers/media/pci/cobalt/cobalt-i2c.c

So a taxnonomy can be,

  adapter            REV_DIR impl   MANGLING adv   NOSTART adv   usable
  i2c-algo-bit           yes            yes            yes         yes
  i2c-algo-pcf           yes            yes            no          no
  i2c-s3c2410            yes            yes            yes         yes
  i2c-tegra-bpmp         yes            yes            yes         yes
  cobalt-i2c             yes         (private adapter, not exposed)
  i2c-brcmstb            no             yes            yes         no
  i2c-pxa                no             yes            yes         no
  i2c-tegra              no             yes         yes (cond)     no
  i2c-imx (this patch)   yes            yes            yes         yes

> Is Realtek RTL8366SE SMI driver upstream? Can I found the driver?

Not yet, and I cannot point you at a tree today. It is Realtek's
"Unmanaged Switch" DSA driver, which is not public yet.

What I can share is the message construction, which is the part you
asked about and is not Realtek-specific.

        static int as5011_i2c_read(struct i2c_client *client,
                                   uint8_t aregaddr, signed char *value)
        {
                uint8_t data[2] = { aregaddr };
                struct i2c_msg msg_set[2] = {
                        {
                                .addr = client->addr,
                                .flags = I2C_M_REV_DIR_ADDR,
                                .len = 1,
                                .buf = (uint8_t *)data
                        },
                        {
                                .addr = client->addr,
                                .flags = I2C_M_RD | I2C_M_NOSTART,
                                .len = 1,
                                .buf = (uint8_t *)data
                        }
                };
                int error;

                error = i2c_transfer(client->adapter, msg_set, 2);
                if (error < 0)
                        return error;

                *value = data[0] & 0x80 ? -1 * (1 + ~data[0]) : data[0];
                return 0;
        }

And the Realtek accessor, which is the same two messages with a 2-byte
register and 2-byte data instead of 1 and 1:

        u8 ra[2]   = { reg & 0xff, (reg >> 8) & 0xff };
        u8 data[2] = { 0xff, 0xff };
        struct i2c_msg msgs[2] = {
                {
                        .addr  = client->addr,
                        .flags = I2C_M_REV_DIR_ADDR,
                        .len   = sizeof(ra),
                        .buf   = ra,
                }, {
                        .addr  = client->addr,
                        .flags = I2C_M_RD | I2C_M_NOSTART,
                        .len   = sizeof(data),
                        .buf   = data,
                },
        };

        ret = i2c_transfer(client->adapter, msgs, 2);
        if (ret != 2)
                return ret < 0 ? ret : -EIO;

        *val = data[0] | (data[1] << 8);

msgs[0] is a write message carrying I2C_M_REV_DIR_ADDR, so the address
byte goes out with the read bit set while the master keeps transmitting
the two register-address bytes. msgs[1] is the read half with
I2C_M_NOSTART, so no repeated start is emitted and the controller simply
turns the bus around. Writes are an ordinary unflagged 4-byte write and
need nothing from this patch.

> ... Have you test this i2c-imx feature in your LS board with RTL8366SE?

Yes, on an LX2160A board carrying four RTL8366SE-CG. Two are strapped to
the chip's 2-wire "EEPROM SMI" mode and hang off hardware i2c-imx
controllers; the other two are on MDIO, for both the same switch registers
are reachable both ways and should answer the same values.

First, the failure on an unpatched kernel, which is reproducible with
nothing but i2ctools and is I think the clearest way to see the problem.
These two commands are byte-identical:

        # i2ctransfer -y -f -a 1 w2@0x7c 0x00 0x13 r4@0x7c
        0xff 0xff 0x00 0x00
        # i2ctransfer -y -f -a 1 r6@0x7c        # no preceding write at all
        0xff 0xff 0x00 0x00 0x00 0x00

Then, with this patch both chips answer correctly. For testing this patch,
I did use the following that I have just pushed to help, for the record:

        https://github.com/vjardin/smi-probe

        # smi-probe -t i2c -b /dev/i2c-1 -a 0x7c id
        chip_num    0x6980  CHIP_RTL8367E (inside RTL8366SE-CG)
        chip_ver    0x0030
        svlan_tpid  0x88a8  reset default, as expected

        # smi-probe -t i2c -b /dev/i2c-1 -a 0x7c rd 0x1300
        reg 0x1300 = 0x6980
        # smi-probe -t i2c -b /dev/i2c-1 -a 0x7c rd 0x1202
        reg 0x1202 = 0x88a8

        # smi-probe -t i2c -b /dev/i2c-2 -a 0x5c rd 0x1202 # U19, IIC3
        reg 0x1202 = 0x88a8
        # smi-probe -t mdio -b 0x8b97000 -a 0x1a rd 0x1202 # MDIO control
        reg 0x1202 = 0x88a8

Then it binds as a DSA switch and enumerates its four user ports:

        realtek-US-switch-dsa-i2c 2-005c: RTK DSA unit 0 (EEPROM SMI, addr 0x5c)

I have many other i2c devices on this board, no regression.

Thanks again for your review,
  Vincent

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART
  2026-08-12  5:47   ` Vincent Jardin
@ 2026-08-12  9:20     ` Carlos Song (OSS)
  0 siblings, 0 replies; 4+ messages in thread
From: Carlos Song (OSS) @ 2026-08-12  9:20 UTC (permalink / raw)
  To: Vincent Jardin, Carlos Song (OSS)
  Cc: Oleksij Rempel, Pengutronix Kernel Team, Andi Shyti, Frank Li,
	Sascha Hauer, Fabio Estevam, linux-i2c@vger.kernel.org,
	imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org



> -----Original Message-----
> From: Vincent Jardin <vjardin@free.fr>
> Sent: Wednesday, August 12, 2026 1:47 PM
> To: Carlos Song (OSS) <carlos.song@oss.nxp.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Andi Shyti <andi.shyti@kernel.org>; Frank Li
> <frank.li@nxp.com>; Sascha Hauer <s.hauer@pengutronix.de>; Fabio
> Estevam <festevam@gmail.com>; linux-i2c@vger.kernel.org;
> imx@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and
> I2C_M_NOSTART
> 
> Hi Carlos,
> 
> Thanks a lot for the review, and no problem for the delay.
> 
> > This is a such rare i2c frame design in the Realtek RTL8366SE SMI read
> > frame, right?
> 
> It is rare, but I have been inspired for I2C_M_REV_DIR_ADDR by the source of
> two client drivers that use it.
> 
> Note that I2C_M_NOSTART is not rare at all. It is what regmap uses for a real
> gather write. regmap_i2c_gather_write() opens with
> 
>         if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_NOSTART))
>                 return -ENOTSUPP;
> 
> FYI, some clients that need REV_DIR_ADDR and NOSTART,
> 
>   drivers/input/joystick/as5011.c
>   drivers/video/fbdev/matrox/matroxfb_maven.c
> 
> both build the identical 2-message pattern,
> 
>   i2c_check_functionality(adapter,
>                                 I2C_FUNC_NOSTART |
>                                 I2C_FUNC_PROTOCOL_MANGLING)
> 
> I do not have those devices, so I did not check it beside code readings.
> 
> Some clients that need NOSTART alone,
> 
>   drivers/base/regmap/regmap-i2c.c
>   drivers/infiniband/hw/hfi1/qsfp.c
>   drivers/gpu/drm/i915/display/dvo_ivch.c
> 
> About the i2c masters, that use I2C_M_REV_DIR_ADDR in code,
> 
>   drivers/i2c/algos/i2c-algo-bit.c
>   drivers/i2c/algos/i2c-algo-pcf.c
>   drivers/i2c/busses/i2c-s3c2410.c <- the model I did investigate
>   drivers/i2c/busses/i2c-tegra-bpmp.c
>   drivers/media/pci/cobalt/cobalt-i2c.c
> 
> So a taxnonomy can be,
> 
>   adapter            REV_DIR impl   MANGLING adv   NOSTART adv
> usable
>   i2c-algo-bit           yes            yes            yes
> yes
>   i2c-algo-pcf           yes            yes            no
> no
>   i2c-s3c2410            yes            yes            yes
> yes
>   i2c-tegra-bpmp         yes            yes            yes
> yes
>   cobalt-i2c             yes         (private adapter, not exposed)
>   i2c-brcmstb            no             yes            yes
> no
>   i2c-pxa                no             yes            yes
> no
>   i2c-tegra              no             yes         yes (cond)
> no
>   i2c-imx (this patch)   yes            yes            yes
> yes
> 
> > Is Realtek RTL8366SE SMI driver upstream? Can I found the driver?
> 
> Not yet, and I cannot point you at a tree today. It is Realtek's "Unmanaged
> Switch" DSA driver, which is not public yet.
> 
> What I can share is the message construction, which is the part you asked
> about and is not Realtek-specific.
> 
>         static int as5011_i2c_read(struct i2c_client *client,
>                                    uint8_t aregaddr, signed char
> *value)
>         {
>                 uint8_t data[2] = { aregaddr };
>                 struct i2c_msg msg_set[2] = {
>                         {
>                                 .addr = client->addr,
>                                 .flags = I2C_M_REV_DIR_ADDR,
>                                 .len = 1,
>                                 .buf = (uint8_t *)data
>                         },
>                         {
>                                 .addr = client->addr,
>                                 .flags = I2C_M_RD |
> I2C_M_NOSTART,
>                                 .len = 1,
>                                 .buf = (uint8_t *)data
>                         }
>                 };
>                 int error;
> 
>                 error = i2c_transfer(client->adapter, msg_set, 2);
>                 if (error < 0)
>                         return error;
> 
>                 *value = data[0] & 0x80 ? -1 * (1 + ~data[0]) : data[0];
>                 return 0;
>         }
> 
> And the Realtek accessor, which is the same two messages with a 2-byte
> register and 2-byte data instead of 1 and 1:
> 
>         u8 ra[2]   = { reg & 0xff, (reg >> 8) & 0xff };
>         u8 data[2] = { 0xff, 0xff };
>         struct i2c_msg msgs[2] = {
>                 {
>                         .addr  = client->addr,
>                         .flags = I2C_M_REV_DIR_ADDR,
>                         .len   = sizeof(ra),
>                         .buf   = ra,
>                 }, {
>                         .addr  = client->addr,
>                         .flags = I2C_M_RD | I2C_M_NOSTART,
>                         .len   = sizeof(data),
>                         .buf   = data,
>                 },
>         };
> 
>         ret = i2c_transfer(client->adapter, msgs, 2);
>         if (ret != 2)
>                 return ret < 0 ? ret : -EIO;
> 
>         *val = data[0] | (data[1] << 8);
> 
> msgs[0] is a write message carrying I2C_M_REV_DIR_ADDR, so the address
> byte goes out with the read bit set while the master keeps transmitting the
> two register-address bytes. msgs[1] is the read half with I2C_M_NOSTART, so
> no repeated start is emitted and the controller simply turns the bus around.
> Writes are an ordinary unflagged 4-byte write and need nothing from this
> patch.
> 
> > ... Have you test this i2c-imx feature in your LS board with RTL8366SE?
> 
> Yes, on an LX2160A board carrying four RTL8366SE-CG. Two are strapped to
> the chip's 2-wire "EEPROM SMI" mode and hang off hardware i2c-imx
> controllers; the other two are on MDIO, for both the same switch registers are
> reachable both ways and should answer the same values.
> 
> First, the failure on an unpatched kernel, which is reproducible with nothing
> but i2ctools and is I think the clearest way to see the problem.
> These two commands are byte-identical:
> 
>         # i2ctransfer -y -f -a 1 w2@0x7c 0x00 0x13 r4@0x7c
>         0xff 0xff 0x00 0x00
>         # i2ctransfer -y -f -a 1 r6@0x7c        # no preceding write at all
>         0xff 0xff 0x00 0x00 0x00 0x00
> 
> Then, with this patch both chips answer correctly. For testing this patch, I did
> use the following that I have just pushed to help, for the record:
> 
>         https://github.com/vjardin/smi-probe
> 
>         # smi-probe -t i2c -b /dev/i2c-1 -a 0x7c id
>         chip_num    0x6980  CHIP_RTL8367E (inside RTL8366SE-CG)
>         chip_ver    0x0030
>         svlan_tpid  0x88a8  reset default, as expected
> 
>         # smi-probe -t i2c -b /dev/i2c-1 -a 0x7c rd 0x1300
>         reg 0x1300 = 0x6980
>         # smi-probe -t i2c -b /dev/i2c-1 -a 0x7c rd 0x1202
>         reg 0x1202 = 0x88a8
> 
>         # smi-probe -t i2c -b /dev/i2c-2 -a 0x5c rd 0x1202 # U19, IIC3
>         reg 0x1202 = 0x88a8
>         # smi-probe -t mdio -b 0x8b97000 -a 0x1a rd 0x1202 # MDIO
> control
>         reg 0x1202 = 0x88a8
> 
> Then it binds as a DSA switch and enumerates its four user ports:
> 
>         realtek-US-switch-dsa-i2c 2-005c: RTK DSA unit 0 (EEPROM SMI,
> addr 0x5c)
> 
> I have many other i2c devices on this board, no regression.
> 
> Thanks again for your review,
>   Vincent


Hi, Vincent

Thank you very much for such clear instructions. That helps lot to understand what happened.
Follow this, I also spend some time to learn and understand this case.

1. Why need I2C_M_REV_DIR_ADDR but not directly read?
The msg with I2C_M_REV_DIR_ADDR is a write msg in fact, it is a write message, master write data and target ack,
only w bit is toggled to r bit.
Because the device don't follow the standard I2C spec, it need this special frame to write:
[1] S + addr + r + [ACK] + data +[ACK] + data...
Yes, not wrong, a read bit on bus but it is a write msg for this device.

But the standard spec write msg is
[2] S + addr + w +[ACK] +data + [ACK] +data
So add this I2C_M_REV_DIR_ADDR flag to make this write msg w bit is changed to r bit, msg [2] become msg [1] so the device worked.
Only change the r/w bit, other data flow keep write logic.

2. What I2C_M_NOSTART is doing?
It help skip the msg repeat start header, next msg start from data. If the first msg setting this flag, it should be unsupported(so you add a loop check to make sure the first msg without I2C_M_NOSTART flag).

3. The msg with I2C_M_NOSTART should keep the same direction with pre msg?
No, not need. For example, RTL8366SE frame:

(1){S Addr Rd [A] reg[7:0] [A] reg[15:8] [A]} (2){ [data[7:0]] A [data[15:8]] NA} P

For (1):
{S Addr Wr [A] reg[7:0] [A] reg[15:8] [A] is a write msg and "change the Wr bit to Rd bit" by I2C_M_REV_DIR_ADDR.
So i2c bus signal is
{S Addr Rd [A] reg[7:0] [A] reg[15:8] [A]}

For (2):
{Rs Addr Rd [A] [data[7:0]] A [data[15:8]] NA} is a read msg and "skip the {Rs Addr Rd [A]}" by I2C_M_NOSTART.
So i2c bus signal is [data[7:0]] A [data[15:8]] NA.

This is a write msg then a read msg. So you add the I2C_M_NOSTART flag handle in these 4 path: atomic read/irq read/atomic write/irq write.
Do I understand right?


About code, Could this part of the code be improved? Of course, that's a matter of personal preference.

@@ -1583,6 +1622,27 @@ static int i2c_imx_xfer_common(struct i2c_adapter *adapter,
        struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
        int use_dma = 0;
 
+       for (i = 0; i < num; i++) {
+               /*
+                * I2C_FUNC_PROTOCOL_MANGLING is advertised for
+                * I2C_M_REV_DIR_ADDR only; reject the flags it also covers but
+                * that this driver does not implement, rather than silently
+                * transferring something the caller did not ask for.
+                */
+               if (msgs[i].flags & (I2C_M_IGNORE_NAK | I2C_M_NO_RD_ACK |
+                                    I2C_M_STOP))
+                       return -EOPNOTSUPP;
+
+               if (!(msgs[i].flags & I2C_M_NOSTART))
+                       continue;
+
+               /* Nothing to continue from, and a zero-length continuation
+                * would leave the controller with no byte to clock.
+                */
+               if (i == 0 || msgs[i].len == 0)
+                       return -EOPNOTSUPP;
+       }
+
        /* Start I2C transfer */
        result = i2c_imx_start(i2c_imx, atomic);
        if (result) {


I prefer this:
1. Do a single function for this check.
static int i2c_imx_check_msgs()
2. Add macro definition.
#define I2C_IMX_UNSUPPORTED_PROTOCOL_MANGLING_FLAGS \
	(I2C_M_IGNORE_NAK | I2C_M_NO_RD_ACK | I2C_M_STOP)
3. drop continue and use:
		/* NOSTART-specific sanity checks. */
		if (msgs[i].flags & I2C_M_NOSTART) {
			/*
			 * NOSTART continues an already open frame. The first
			 * message has nothing to continue from, and a
			 * zero-length continuation leaves the controller with
			 * no byte to clock.
			 */
			if (i == 0 || msgs[i].len == 0)
				return -EOPNOTSUPP;
		}

It is up to you. All in all, I think this patch is nice for me and thank you again for the improvements.

Carlos Song


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-08-12  9:20 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-06 22:37 [PATCH] i2c: imx: add I2C_M_REV_DIR_ADDR and I2C_M_NOSTART Vincent Jardin via B4 Relay
2026-08-11 10:18 ` Carlos Song (OSS)
2026-08-12  5:47   ` Vincent Jardin
2026-08-12  9:20     ` Carlos Song (OSS)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox