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:
 <VI2PR04MB11147F5B660417134E6F9F9CAE896A@VI2PR04MB11147.eurprd04.prod.outlook.com>
Date: Wed, 21 Jan 2026 06:43:05 +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

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@...nel.org>
> 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: 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