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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aW9jUs2-kiVIxTfn@zenone.zhora.eu>
Date: Tue, 20 Jan 2026 12:30:38 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Carlos Song <carlos.song@....com>
Cc: frank.li@....com, aisheng.dong@....com, shawnguo@...nel.org, 
	s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com, vz@...ia.com, 
	wsa@...nel.org, linux-i2c@...r.kernel.org, imx@...ts.linux.dev, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] i2c: imx-lpi2c: support smbus block read feature

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