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

quoted

-----Original Message-----
From: Andi Shyti <andi.shyti@kernel.org>
Sent: Tuesday, January 20, 2026 7:31 PM
To: Carlos Song <redacted>
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'

quoted

button


Hi Carlos,

On Fri, Nov 21, 2025 at 07:01:00PM +0800, Carlos Song wrote:

quoted

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.

quoted

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?

quoted

quoted

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.

quoted

quoted

Signed-off-by: Carlos Song <redacted>

...

quoted

 #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.

quoted

quoted

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

...

quoted

+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.

quoted

quoted

+     if (ret) {
+             dev_err(&lpi2c_imx->adapter.dev, "SMBus read count

timeout %d\n", ret);

quoted

+             return ret;

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

I will fix this in V2.

quoted

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.

quoted

quoted

+     }
+

...

quoted

+             /* Read the first byte as block len */
+             block_len =

lpi2c_SMBus_block_read_single_byte(lpi2c_imx);

quoted

quoted

+             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.

quoted

quoted

+                     dev_err(&lpi2c_imx->adapter.dev, "SMBus read

data length timeout\n");

quoted

+                     return;
+             }
+

...

quoted

+             if (block_len == 1) {
+                     ret =

lpi2c_SMBus_block_read_single_byte(lpi2c_imx);

quoted

+                     if (ret < 0)
+                             dev_err(&lpi2c_imx->adapter.dev,
+ "SMBus

read data timeout\n");

quoted

+                     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.

quoted

quoted

+             }
+
+             /* 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!

quoted

quoted

+             writel((RECV_DATA << 8) | (block_len - 2),
+ lpi2c_imx->base +

LPI2C_MTDR);

quoted

+             lpi2c_imx->msglen += block_len;
+     }

Thanks,
Andi