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

[Carlos Song]

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 at kernel.org>
> Cc: Frank Li <frank.li at nxp.com>; Aisheng Dong <aisheng.dong at nxp.com>;
> shawnguo at kernel.org; s.hauer at pengutronix.de; kernel at pengutronix.de;
> festevam at gmail.com; vz at mleia.com; wsa at kernel.org;
> linux-i2c at vger.kernel.org; imx at lists.linux.dev;
> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org
> Subject: Re: [PATCH v2] i2c: imx-lpi2c: support smbus block read feature
> 
> 
> 
> > -----Original Message-----
> > From: Andi Shyti <andi.shyti at kernel.org>
> > Sent: Tuesday, January 20, 2026 7:31 PM
> > To: Carlos Song <carlos.song at nxp.com>
> > Cc: Frank Li <frank.li at nxp.com>; Aisheng Dong <aisheng.dong at nxp.com>;
> > shawnguo at kernel.org; s.hauer at pengutronix.de; kernel at pengutronix.de;
> > festevam at gmail.com; vz at mleia.com; wsa at kernel.org;
> > linux-i2c at vger.kernel.org; imx at lists.linux.dev;
> > linux-arm-kernel at lists.infradead.org; linux-kernel at 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'
> > 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 at 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)?
> >
> 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