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 <redacted>

...

 #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