[PATCH v2] i2c: imx-lpi2c: support smbus block read feature

[PATCH v2] i2c: imx-lpi2c: support smbus block read feature

[Carlos Song]

The LPI2C controller automatically sends a NACK after the last byte of a
receive command unless the next command in MTDR is also a receive command.
If MTDR is empty when a receive completes, NACK is transmitted by default.

To comply with SMBus block read, start with a 2-byte read:
- The first byte is the block length. Once received, update MTDR
  immediately to keep MTDR non-empty.
- Issue a new receive command for the remaining data before the second
  byte arrives ensuring continuous ACK instead of NACK.

Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
Changes since v1:
* According to discussion with Frank, improve comment and commit log by AI.
  This comment and commit log looks more clear.
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 94 +++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index d882126c1778..dfacb0aec3c0 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -90,6 +90,7 @@
 #define MRDR_RXEMPTY	BIT(14)
 #define MDER_TDDE	BIT(0)
 #define MDER_RDDE	BIT(1)
+#define MSR_RDF_ASSERT(x) FIELD_GET(MSR_RDF, (x))
 
 #define SCR_SEN		BIT(0)
 #define SCR_RST		BIT(1)
@@ -461,7 +462,7 @@ static bool lpi2c_imx_write_txfifo(struct lpi2c_imx_struct *lpi2c_imx, bool atom
 
 static bool lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx, bool atomic)
 {
-	unsigned int blocklen, remaining;
+	unsigned int remaining;
 	unsigned int temp, data;
 
 	do {
@@ -472,15 +473,6 @@ static bool lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx, bool atomi
 		lpi2c_imx->rx_buf[lpi2c_imx->delivered++] = data & 0xff;
 	} while (1);
 
-	/*
-	 * First byte is the length of remaining packet in the SMBus block
-	 * data read. Add it to msgs->len.
-	 */
-	if (lpi2c_imx->block_data) {
-		blocklen = lpi2c_imx->rx_buf[0];
-		lpi2c_imx->msglen += blocklen;
-	}
-
 	remaining = lpi2c_imx->msglen - lpi2c_imx->delivered;
 
 	if (!remaining) {
@@ -493,12 +485,7 @@ static bool lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx, bool atomi
 	lpi2c_imx_set_rx_watermark(lpi2c_imx);
 
 	/* multiple receive commands */
-	if (lpi2c_imx->block_data) {
-		lpi2c_imx->block_data = 0;
-		temp = remaining;
-		temp |= (RECV_DATA << 8);
-		writel(temp, lpi2c_imx->base + LPI2C_MTDR);
-	} else if (!(lpi2c_imx->delivered & 0xff)) {
+	if (!(lpi2c_imx->delivered & 0xff)) {
 		temp = (remaining > CHUNK_DATA ? CHUNK_DATA : remaining) - 1;
 		temp |= (RECV_DATA << 8);
 		writel(temp, lpi2c_imx->base + LPI2C_MTDR);
@@ -536,18 +523,81 @@ static int lpi2c_imx_write_atomic(struct lpi2c_imx_struct *lpi2c_imx,
 	return err;
 }
 
+static unsigned int lpi2c_SMBus_block_read_single_byte(struct lpi2c_imx_struct *lpi2c_imx)
+{
+	unsigned int val, data;
+	int ret;
+
+	ret = readl_poll_timeout(lpi2c_imx->base + LPI2C_MSR, val,
+				 MSR_RDF_ASSERT(val), 1, 1000);
+	if (ret) {
+		dev_err(&lpi2c_imx->adapter.dev, "SMBus read count timeout %d\n", ret);
+		return ret;
+	}
+
+	data = readl(lpi2c_imx->base + LPI2C_MRDR);
+	lpi2c_imx->rx_buf[lpi2c_imx->delivered++] = data & 0xff;
+
+	return data;
+}
+
 static void lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
 				struct i2c_msg *msgs)
 {
-	unsigned int temp;
+	unsigned int temp, ret, block_len;
 
 	lpi2c_imx->rx_buf = msgs->buf;
 	lpi2c_imx->block_data = msgs->flags & I2C_M_RECV_LEN;
 
 	lpi2c_imx_set_rx_watermark(lpi2c_imx);
-	temp = msgs->len > CHUNK_DATA ? CHUNK_DATA - 1 : msgs->len - 1;
-	temp |= (RECV_DATA << 8);
-	writel(temp, lpi2c_imx->base + LPI2C_MTDR);
+
+	if (!lpi2c_imx->block_data) {
+		temp = msgs->len > CHUNK_DATA ? CHUNK_DATA - 1 : msgs->len - 1;
+		temp |= (RECV_DATA << 8);
+		writel(temp, lpi2c_imx->base + LPI2C_MTDR);
+	} else {
+		/*
+		 * The LPI2C controller automatically sends a NACK after the last byte of a
+		 * receive command, unless the next command in MTDR is also a receive command.
+		 * If MTDR is empty when a receive completes, a NACK is sent by default.
+		 *
+		 * To comply with the SMBus block read spec, we start with a 2-byte read:
+		 * The first byte in RXFIFO is the block length. Once this byte arrives, the
+		 * controller immediately updates MTDR with the next read command, ensuring
+		 * continuous ACK instead of NACK.
+		 *
+		 * The second byte is the first block data byte. Therefore, the subsequent
+		 * read command should request (block_len - 1) bytes, since one data byte
+		 * has already been read.
+		 */
+
+		writel((RECV_DATA << 8) | 0x01, lpi2c_imx->base + LPI2C_MTDR);
+
+		/* Read the first byte as block len */
+		block_len = lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
+		if (block_len < 0) {
+			dev_err(&lpi2c_imx->adapter.dev, "SMBus read data length timeout\n");
+			return;
+		}
+
+		/* Confirm SMBus transfer meets protocol */
+		if (block_len == 0 || block_len > I2C_SMBUS_BLOCK_MAX) {
+			dev_err(&lpi2c_imx->adapter.dev, "Invalid SMBus block read length\n");
+			return;
+		}
+
+		/* If just read 1 byte then read out from fifo. No need new command update */
+		if (block_len == 1) {
+			ret = lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
+			if (ret < 0)
+				dev_err(&lpi2c_imx->adapter.dev, "SMBus read data timeout\n");
+			return;
+		}
+
+		/* Block read other length data need to update command again*/
+		writel((RECV_DATA << 8) | (block_len - 2), lpi2c_imx->base + LPI2C_MTDR);
+		lpi2c_imx->msglen += block_len;
+	}
 }
 
 static bool lpi2c_imx_read_chunk_atomic(struct lpi2c_imx_struct *lpi2c_imx)
@@ -599,6 +649,10 @@ static bool is_use_dma(struct lpi2c_imx_struct *lpi2c_imx, struct i2c_msg *msg)
 	if (pm_suspend_in_progress())
 		return false;
 
+	/* DMA is not suitable for SMBus block read */
+	if (msg->flags & I2C_M_RECV_LEN)
+		return false;
+
 	/*
 	 * When the length of data is less than I2C_DMA_THRESHOLD,
 	 * cpu mode is used directly to avoid low performance.
-- 
2.34.1

Re: [PATCH v2] i2c: imx-lpi2c: support smbus block read feature

[Frank Li]

On Fri, Nov 21, 2025 at 07:01:00PM +0800, Carlos Song wrote:
> The LPI2C controller automatically sends a NACK after the last byte of a
> receive command unless the next command in MTDR is also a receive command.
> If MTDR is empty when a receive completes, NACK is transmitted by default.
>
> To comply with SMBus block read, start with a 2-byte read:
> - The first byte is the block length. Once received, update MTDR
>   immediately to keep MTDR non-empty.
> - Issue a new receive command for the remaining data before the second
>   byte arrives ensuring continuous ACK instead of NACK.
>
> Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> ---
> Changes since v1:
> * According to discussion with Frank, improve comment and commit log by AI.
>   This comment and commit log looks more clear.

Needn't mention "by AI" every time.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 94 +++++++++++++++++++++++-------
>  1 file changed, 74 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index d882126c1778..dfacb0aec3c0 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -90,6 +90,7 @@
>  #define MRDR_RXEMPTY	BIT(14)
>  #define MDER_TDDE	BIT(0)
>  #define MDER_RDDE	BIT(1)
> +#define MSR_RDF_ASSERT(x) FIELD_GET(MSR_RDF, (x))
>
>  #define SCR_SEN		BIT(0)
>  #define SCR_RST		BIT(1)
> @@ -461,7 +462,7 @@ static bool lpi2c_imx_write_txfifo(struct lpi2c_imx_struct *lpi2c_imx, bool atom
>
>  static bool lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx, bool atomic)
>  {
> -	unsigned int blocklen, remaining;
> +	unsigned int remaining;
>  	unsigned int temp, data;
>
>  	do {
> @@ -472,15 +473,6 @@ static bool lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx, bool atomi
>  		lpi2c_imx->rx_buf[lpi2c_imx->delivered++] = data & 0xff;
>  	} while (1);
>
> -	/*
> -	 * First byte is the length of remaining packet in the SMBus block
> -	 * data read. Add it to msgs->len.
> -	 */
> -	if (lpi2c_imx->block_data) {
> -		blocklen = lpi2c_imx->rx_buf[0];
> -		lpi2c_imx->msglen += blocklen;
> -	}
> -
>  	remaining = lpi2c_imx->msglen - lpi2c_imx->delivered;
>
>  	if (!remaining) {
> @@ -493,12 +485,7 @@ static bool lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx, bool atomi
>  	lpi2c_imx_set_rx_watermark(lpi2c_imx);
>
>  	/* multiple receive commands */
> -	if (lpi2c_imx->block_data) {
> -		lpi2c_imx->block_data = 0;
> -		temp = remaining;
> -		temp |= (RECV_DATA << 8);
> -		writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> -	} else if (!(lpi2c_imx->delivered & 0xff)) {
> +	if (!(lpi2c_imx->delivered & 0xff)) {
>  		temp = (remaining > CHUNK_DATA ? CHUNK_DATA : remaining) - 1;
>  		temp |= (RECV_DATA << 8);
>  		writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> @@ -536,18 +523,81 @@ static int lpi2c_imx_write_atomic(struct lpi2c_imx_struct *lpi2c_imx,
>  	return err;
>  }
>
> +static unsigned int lpi2c_SMBus_block_read_single_byte(struct lpi2c_imx_struct *lpi2c_imx)
> +{
> +	unsigned int val, data;
> +	int ret;
> +
> +	ret = readl_poll_timeout(lpi2c_imx->base + LPI2C_MSR, val,
> +				 MSR_RDF_ASSERT(val), 1, 1000);
> +	if (ret) {
> +		dev_err(&lpi2c_imx->adapter.dev, "SMBus read count timeout %d\n", ret);
> +		return ret;
> +	}
> +
> +	data = readl(lpi2c_imx->base + LPI2C_MRDR);
> +	lpi2c_imx->rx_buf[lpi2c_imx->delivered++] = data & 0xff;
> +
> +	return data;
> +}
> +
>  static void lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
>  				struct i2c_msg *msgs)
>  {
> -	unsigned int temp;
> +	unsigned int temp, ret, block_len;
>
>  	lpi2c_imx->rx_buf = msgs->buf;
>  	lpi2c_imx->block_data = msgs->flags & I2C_M_RECV_LEN;
>
>  	lpi2c_imx_set_rx_watermark(lpi2c_imx);
> -	temp = msgs->len > CHUNK_DATA ? CHUNK_DATA - 1 : msgs->len - 1;
> -	temp |= (RECV_DATA << 8);
> -	writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> +
> +	if (!lpi2c_imx->block_data) {
> +		temp = msgs->len > CHUNK_DATA ? CHUNK_DATA - 1 : msgs->len - 1;
> +		temp |= (RECV_DATA << 8);
> +		writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> +	} else {
> +		/*
> +		 * The LPI2C controller automatically sends a NACK after the last byte of a
> +		 * receive command, unless the next command in MTDR is also a receive command.
> +		 * If MTDR is empty when a receive completes, a NACK is sent by default.
> +		 *
> +		 * To comply with the SMBus block read spec, we start with a 2-byte read:
> +		 * The first byte in RXFIFO is the block length. Once this byte arrives, the
> +		 * controller immediately updates MTDR with the next read command, ensuring
> +		 * continuous ACK instead of NACK.
> +		 *
> +		 * The second byte is the first block data byte. Therefore, the subsequent
> +		 * read command should request (block_len - 1) bytes, since one data byte
> +		 * has already been read.
> +		 */
> +
> +		writel((RECV_DATA << 8) | 0x01, lpi2c_imx->base + LPI2C_MTDR);
> +
> +		/* Read the first byte as block len */
> +		block_len = lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
> +		if (block_len < 0) {
> +			dev_err(&lpi2c_imx->adapter.dev, "SMBus read data length timeout\n");
> +			return;
> +		}
> +
> +		/* Confirm SMBus transfer meets protocol */
> +		if (block_len == 0 || block_len > I2C_SMBUS_BLOCK_MAX) {
> +			dev_err(&lpi2c_imx->adapter.dev, "Invalid SMBus block read length\n");
> +			return;
> +		}
> +
> +		/* If just read 1 byte then read out from fifo. No need new command update */
> +		if (block_len == 1) {
> +			ret = lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
> +			if (ret < 0)
> +				dev_err(&lpi2c_imx->adapter.dev, "SMBus read data timeout\n");
> +			return;
> +		}
> +
> +		/* Block read other length data need to update command again*/
> +		writel((RECV_DATA << 8) | (block_len - 2), lpi2c_imx->base + LPI2C_MTDR);
> +		lpi2c_imx->msglen += block_len;
> +	}
>  }
>
>  static bool lpi2c_imx_read_chunk_atomic(struct lpi2c_imx_struct *lpi2c_imx)
> @@ -599,6 +649,10 @@ static bool is_use_dma(struct lpi2c_imx_struct *lpi2c_imx, struct i2c_msg *msg)
>  	if (pm_suspend_in_progress())
>  		return false;
>
> +	/* DMA is not suitable for SMBus block read */
> +	if (msg->flags & I2C_M_RECV_LEN)
> +		return false;
> +
>  	/*
>  	 * When the length of data is less than I2C_DMA_THRESHOLD,
>  	 * cpu mode is used directly to avoid low performance.
> --
> 2.34.1
>

Re: [PATCH v2] i2c: imx-lpi2c: support smbus block read feature

[Andi Shyti]

Hi Carlos,

On Fri, Nov 21, 2025 at 07:01:00PM +0800, Carlos Song wrote:
> The LPI2C controller automatically sends a NACK after the last byte of a
> receive command unless the next command in MTDR is also a receive command.
> If MTDR is empty when a receive completes, NACK is transmitted by default.
> 
> To comply with SMBus block read, start with a 2-byte read:
> - The first byte is the block length. Once received, update MTDR
>   immediately to keep MTDR non-empty.
> - Issue a new receive command for the remaining data before the second
>   byte arrives ensuring continuous ACK instead of NACK.

What is the real fix you are addressing here? Can you be a bit
more descriptive?

> Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")

Is this tag really needed?

> Signed-off-by: Carlos Song <carlos.song@nxp.com>

...

>  #define MRDR_RXEMPTY	BIT(14)
>  #define MDER_TDDE	BIT(0)
>  #define MDER_RDDE	BIT(1)
> +#define MSR_RDF_ASSERT(x) FIELD_GET(MSR_RDF, (x))

This name sounds more as an imperative action, do you mean
something like MSR_RDF_ASSERTED(x)?

>  #define SCR_SEN		BIT(0)
>  #define SCR_RST		BIT(1)

...

> +static unsigned int lpi2c_SMBus_block_read_single_byte(struct lpi2c_imx_struct *lpi2c_imx)
> +{
> +	unsigned int val, data;
> +	int ret;
> +
> +	ret = readl_poll_timeout(lpi2c_imx->base + LPI2C_MSR, val,
> +				 MSR_RDF_ASSERT(val), 1, 1000);

What is the value of val here?

> +	if (ret) {
> +		dev_err(&lpi2c_imx->adapter.dev, "SMBus read count timeout %d\n", ret);
> +		return ret;

This is an unsigned function and you are trying to return a
negative number.

Can we also add a comment saying that readl_poll_timeout() fails
only if it times out?

> +	}
> +

...

> +		/* Read the first byte as block len */
> +		block_len = lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
> +		if (block_len < 0) {

both block_len and lpi2c_SMBus_block_read_single_byte() are
unsigned, but you are comparing for negative value.

> +			dev_err(&lpi2c_imx->adapter.dev, "SMBus read data length timeout\n");
> +			return;
> +		}
> +

...

> +		if (block_len == 1) {
> +			ret = lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
> +			if (ret < 0)
> +				dev_err(&lpi2c_imx->adapter.dev, "SMBus read data timeout\n");
> +			return;

do we need to increment msglen here?

> +		}
> +
> +		/* Block read other length data need to update command again*/

Please fix the commenting style here.

> +		writel((RECV_DATA << 8) | (block_len - 2), lpi2c_imx->base + LPI2C_MTDR);
> +		lpi2c_imx->msglen += block_len;
> +	}

Thanks,
Andi

Re: [PATCH v2] i2c: imx-lpi2c: support smbus block read feature

[Carlos Song]

> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Tuesday, January 20, 2026 7:31 PM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: Frank Li <frank.li@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; vz@mleia.com; wsa@kernel.org;
> linux-i2c@vger.kernel.org; imx@lists.linux.dev;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH v2] i2c: imx-lpi2c: support smbus block read feature
> 
> Caution: This is an external email. Please take care when clicking links or opening
> attachments. When in doubt, report the message using the 'Report this email'
> button
> 
> 
> Hi Carlos,
> 
> On Fri, Nov 21, 2025 at 07:01:00PM +0800, Carlos Song wrote:
> > The LPI2C controller automatically sends a NACK after the last byte of
> > a receive command unless the next command in MTDR is also a receive
> command.
> > If MTDR is empty when a receive completes, NACK is transmitted by default.
> >
> > To comply with SMBus block read, start with a 2-byte read:
> > - The first byte is the block length. Once received, update MTDR
> >   immediately to keep MTDR non-empty.
> > - Issue a new receive command for the remaining data before the second
> >   byte arrives ensuring continuous ACK instead of NACK.
> 
> What is the real fix you are addressing here? Can you be a bit more descriptive?
> 
> > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> 
> Is this tag really needed?
> 
Hi, Andi

From previous code logic, looks like driver want to support this feature but when I use i2ctool to test, it totally doesn't work. So I make patch to support this feature really. Sorry for my misleading commit log. As you suggested, I should add more explanation about "what I'm fixing". I can fix this in V2.

> > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> 
> ...
> 
> >  #define MRDR_RXEMPTY BIT(14)
> >  #define MDER_TDDE    BIT(0)
> >  #define MDER_RDDE    BIT(1)
> > +#define MSR_RDF_ASSERT(x) FIELD_GET(MSR_RDF, (x))
> 
> This name sounds more as an imperative action, do you mean something like
> MSR_RDF_ASSERTED(x)?
> 
Yes I will fix this in V2.

> >  #define SCR_SEN              BIT(0)
> >  #define SCR_RST              BIT(1)
> 
> ...
> 
> > +static unsigned int lpi2c_SMBus_block_read_single_byte(struct
> > +lpi2c_imx_struct *lpi2c_imx) {
> > +     unsigned int val, data;
> > +     int ret;
> > +
> > +     ret = readl_poll_timeout(lpi2c_imx->base + LPI2C_MSR, val,
> > +                              MSR_RDF_ASSERT(val), 1, 1000);
> 
> What is the value of val here?
> 
It is the value of LPI2C_MSR which CPU read out. We only care about RDF bit, in order to ensure real-time, I have to poll this register and once RDF asserted, move on immediately.

> > +     if (ret) {
> > +             dev_err(&lpi2c_imx->adapter.dev, "SMBus read count
> timeout %d\n", ret);
> > +             return ret;
> 
> This is an unsigned function and you are trying to return a negative number.
> 

I will fix this in V2.
> Can we also add a comment saying that readl_poll_timeout() fails only if it times
> out?
> 
Yes, you are right, I will add this comment before the readl_poll_timeout line.

> > +     }
> > +
> 
> ...
> 
> > +             /* Read the first byte as block len */
> > +             block_len = lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
> > +             if (block_len < 0) {
> 
> both block_len and lpi2c_SMBus_block_read_single_byte() are unsigned, but
> you are comparing for negative value.
> 
I will fix this in V2. Looks I need to split this function to make the return value type aligned.

> > +                     dev_err(&lpi2c_imx->adapter.dev, "SMBus read
> data length timeout\n");
> > +                     return;
> > +             }
> > +
> 
> ...
> 
> > +             if (block_len == 1) {
> > +                     ret =
> lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
> > +                     if (ret < 0)
> > +                             dev_err(&lpi2c_imx->adapter.dev, "SMBus
> read data timeout\n");
> > +                     return;
> 
> do we need to increment msglen here?
> 

lpi2c_imx->msglen is not needed to update here. It is only for RX bytes count in IRQ.
But, msgs-> len really need to be updated.

> > +             }
> > +
> > +             /* Block read other length data need to update command
> > + again*/
> 
> Please fix the commenting style here.
> 
Will fix at V2.

Thank you very much for all your suggestions!
> > +             writel((RECV_DATA << 8) | (block_len - 2), lpi2c_imx->base +
> LPI2C_MTDR);
> > +             lpi2c_imx->msglen += block_len;
> > +     }
> 
> Thanks,
> Andi

RE: [PATCH v2] i2c: imx-lpi2c: support smbus block read feature

[Carlos Song]

Sorry.. Will fix at V3. This is V2 version..

> -----Original Message-----
> From: Carlos Song
> Sent: Wednesday, January 21, 2026 2:41 PM
> To: Andi Shyti <andi.shyti@kernel.org>
> Cc: Frank Li <frank.li@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; vz@mleia.com; wsa@kernel.org;
> linux-i2c@vger.kernel.org; imx@lists.linux.dev;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] i2c: imx-lpi2c: support smbus block read feature
> 
> 
> 
> > -----Original Message-----
> > From: Andi Shyti <andi.shyti@kernel.org>
> > Sent: Tuesday, January 20, 2026 7:31 PM
> > To: Carlos Song <carlos.song@nxp.com>
> > Cc: Frank Li <frank.li@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> > festevam@gmail.com; vz@mleia.com; wsa@kernel.org;
> > linux-i2c@vger.kernel.org; imx@lists.linux.dev;
> > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: [EXT] Re: [PATCH v2] i2c: imx-lpi2c: support smbus block read
> > feature
> >
> > Caution: This is an external email. Please take care when clicking
> > links or opening attachments. When in doubt, report the message using the
> 'Report this email'
> > button
> >
> >
> > Hi Carlos,
> >
> > On Fri, Nov 21, 2025 at 07:01:00PM +0800, Carlos Song wrote:
> > > The LPI2C controller automatically sends a NACK after the last byte
> > > of a receive command unless the next command in MTDR is also a
> > > receive
> > command.
> > > If MTDR is empty when a receive completes, NACK is transmitted by default.
> > >
> > > To comply with SMBus block read, start with a 2-byte read:
> > > - The first byte is the block length. Once received, update MTDR
> > >   immediately to keep MTDR non-empty.
> > > - Issue a new receive command for the remaining data before the second
> > >   byte arrives ensuring continuous ACK instead of NACK.
> >
> > What is the real fix you are addressing here? Can you be a bit more
> descriptive?
> >
> > > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> >
> > Is this tag really needed?
> >
> Hi, Andi
> 
> From previous code logic, looks like driver want to support this feature but when
> I use i2ctool to test, it totally doesn't work. So I make patch to support this
> feature really. Sorry for my misleading commit log. As you suggested, I should
> add more explanation about "what I'm fixing". I can fix this in V2.
> 
> > > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> >
> > ...
> >
> > >  #define MRDR_RXEMPTY BIT(14)
> > >  #define MDER_TDDE    BIT(0)
> > >  #define MDER_RDDE    BIT(1)
> > > +#define MSR_RDF_ASSERT(x) FIELD_GET(MSR_RDF, (x))
> >
> > This name sounds more as an imperative action, do you mean something
> > like MSR_RDF_ASSERTED(x)?
> >
> Yes I will fix this in V2.
> 
> > >  #define SCR_SEN              BIT(0)
> > >  #define SCR_RST              BIT(1)
> >
> > ...
> >
> > > +static unsigned int lpi2c_SMBus_block_read_single_byte(struct
> > > +lpi2c_imx_struct *lpi2c_imx) {
> > > +     unsigned int val, data;
> > > +     int ret;
> > > +
> > > +     ret = readl_poll_timeout(lpi2c_imx->base + LPI2C_MSR, val,
> > > +                              MSR_RDF_ASSERT(val), 1, 1000);
> >
> > What is the value of val here?
> >
> It is the value of LPI2C_MSR which CPU read out. We only care about RDF bit, in
> order to ensure real-time, I have to poll this register and once RDF asserted,
> move on immediately.
> 
> > > +     if (ret) {
> > > +             dev_err(&lpi2c_imx->adapter.dev, "SMBus read count
> > timeout %d\n", ret);
> > > +             return ret;
> >
> > This is an unsigned function and you are trying to return a negative number.
> >
> 
> I will fix this in V2.
> > Can we also add a comment saying that readl_poll_timeout() fails only
> > if it times out?
> >
> Yes, you are right, I will add this comment before the readl_poll_timeout line.
> 
> > > +     }
> > > +
> >
> > ...
> >
> > > +             /* Read the first byte as block len */
> > > +             block_len =
> lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
> > > +             if (block_len < 0) {
> >
> > both block_len and lpi2c_SMBus_block_read_single_byte() are unsigned,
> > but you are comparing for negative value.
> >
> I will fix this in V2. Looks I need to split this function to make the return value
> type aligned.
> 
> > > +                     dev_err(&lpi2c_imx->adapter.dev, "SMBus read
> > data length timeout\n");
> > > +                     return;
> > > +             }
> > > +
> >
> > ...
> >
> > > +             if (block_len == 1) {
> > > +                     ret =
> > lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
> > > +                     if (ret < 0)
> > > +                             dev_err(&lpi2c_imx->adapter.dev,
> > > + "SMBus
> > read data timeout\n");
> > > +                     return;
> >
> > do we need to increment msglen here?
> >
> 
> lpi2c_imx->msglen is not needed to update here. It is only for RX bytes count in
> IRQ.
> But, msgs-> len really need to be updated.
> 
> > > +             }
> > > +
> > > +             /* Block read other length data need to update command
> > > + again*/
> >
> > Please fix the commenting style here.
> >
> Will fix at V2.
> 
> Thank you very much for all your suggestions!
> > > +             writel((RECV_DATA << 8) | (block_len - 2),
> > > + lpi2c_imx->base +
> > LPI2C_MTDR);
> > > +             lpi2c_imx->msglen += block_len;
> > > +     }
> >
> > Thanks,
> > Andi