1. Patchew
  2. linux
  3. View series

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

Carlos Song posted 1 patch 1 week, 3 days ago 
drivers/i2c/busses/i2c-imx-lpi2c.c | 94 +++++++++++++++++++++++-------
1 file changed, 74 insertions(+), 20 deletions(-)
[PATCH v2] i2c: imx-lpi2c: support smbus block read feature
Posted by Carlos Song 1 week, 3 days ago 
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
Posted by Frank Li 1 week, 3 days ago 
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
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.