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

[Andi Shyti <andi.shyti@kernel.org>]

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