Message-ID:
 <VI2PR04MB111476EC2286A529FE09F774AE896A@VI2PR04MB11147.eurprd04.prod.outlook.com>
Date: Wed, 21 Jan 2026 06:41:28 +0000
From: Carlos Song <carlos.song@....com>
To: Andi Shyti <andi.shyti@...nel.org>
CC: Frank Li <frank.li@....com>, Aisheng Dong <aisheng.dong@....com>,
	"shawnguo@...nel.org" <shawnguo@...nel.org>, "s.hauer@...gutronix.de"
	<s.hauer@...gutronix.de>, "kernel@...gutronix.de" <kernel@...gutronix.de>,
	"festevam@...il.com" <festevam@...il.com>, "vz@...ia.com" <vz@...ia.com>,
	"wsa@...nel.org" <wsa@...nel.org>, "linux-i2c@...r.kernel.org"
	<linux-i2c@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] i2c: imx-lpi2c: support smbus block read feature



> -----Original Message-----
> From: Andi Shyti <andi.shyti@...nel.org>
> Sent: Tuesday, January 20, 2026 7:31 PM
> To: Carlos Song <carlos.song@....com>
> Cc: Frank Li <frank.li@....com>; Aisheng Dong <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: [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'
> button
> 
> 
> 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?
> 
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.

> > 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)?
> 
Yes I will fix this in V2.

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

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

I will fix this in V2.
> 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.

> > +     }
> > +
> 
> ...
> 
> > +             /* 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.
> 
I will fix this in V2. Looks I need to split this function to make the return value type aligned.

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

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.

> > +             }
> > +
> > +             /* 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!
> > +             writel((RECV_DATA << 8) | (block_len - 2), lpi2c_imx->base +
> LPI2C_MTDR);
> > +             lpi2c_imx->msglen += block_len;
> > +     }
> 
> Thanks,
> Andi

Powered by blists - more mailing lists