Re: [PATCH 04/13] i3c: mipi-i3c-hci-pci: Use readl_poll_timeout()

From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Frank Li <Frank.li@nxp.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>, linux-i3c@lists.infradead.org
Subject: Re: [PATCH 04/13] i3c: mipi-i3c-hci-pci: Use readl_poll_timeout()
Date: Fri, 14 Nov 2025 18:20:58 +0100	[thread overview]
Message-ID: <202511141720583d31a5e7@mail.local> (raw)
In-Reply-To: <aRdcOH3vXPkxWScy@lizhi-Precision-Tower-5810>

On 14/11/2025 11:43:36-0500, Frank Li wrote:
> On Fri, Nov 14, 2025 at 05:02:05PM +0100, Alexandre Belloni wrote:
> > On 12/11/2025 10:59:57-0500, Frank Li wrote:
> > > On Wed, Nov 12, 2025 at 12:03:30PM +0200, Adrian Hunter wrote:
> > > > Use readl_poll_timeout() instead of open-coding the polling loop.
> > >
> > > nice
> > > >
> > > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > > > ---
> > > >  drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c | 14 ++++++--------
> > > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c b/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c
> > > > index c74cc511a344..c15e0556a691 100644
> > > > --- a/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c
> > > > +++ b/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c
> > > > @@ -8,6 +8,7 @@
> > > >   */
> > > >  #include <linux/acpi.h>
> > > >  #include <linux/idr.h>
> > > > +#include <linux/iopoll.h>
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/pci.h>
> > > > @@ -24,11 +25,12 @@ static DEFINE_IDA(mipi_i3c_hci_pci_ida);
> > > >  #define INTEL_RESETS			0x04
> > > >  #define INTEL_RESETS_RESET		BIT(0)
> > > >  #define INTEL_RESETS_RESET_DONE		BIT(1)
> > > > +#define INTEL_RESETS_TIMEOUT_US		10000
> > >
> > > 10 * USEC_PER_MSEC
> >
> > This is super hard nitpicking, I guess everyone knows 10000µs is 10ms
> 
> It is from
> https://lore.kernel.org/all/aPnrc-Z5WVJorGr7@smile.fi.intel.com/
> 

Well, Andy is super nitpicky. I guess 10000 has US just before so there
won't be any confusion on the actual unit

In your patch, I wouldn't change it for MMC5633_WAIT_SET_RESET_US but I
agree the call to regmap_read_poll_timeout needs to be clearer.

> Generally, I only provide such nit at early version.
> 
> I think benefit is avoid miss count 0. Maybe C23, 10'000, especially when
> number is big.
> 
> Frank
> >
> > >
> > > Frank
> > > >
> > > >  static int intel_init(struct pci_dev *pci)
> > > >  {
> > > > -	unsigned long timeout;
> > > >  	void __iomem *priv;
> > > > +	u32 reg;
> > > >  	int ret;
> > > >
> > > >  	priv = devm_ioremap(&pci->dev,
> > > > @@ -43,13 +45,9 @@ static int intel_init(struct pci_dev *pci)
> > > >
> > > >  	/* Assert reset, wait for completion and release reset */
> > > >  	writel(0, priv + INTEL_RESETS);
> > > > -	timeout = jiffies + msecs_to_jiffies(10);
> > > > -	while (!(readl(priv + INTEL_RESETS) &
> > > > -		 INTEL_RESETS_RESET_DONE)) {
> > > > -		if (time_after(jiffies, timeout))
> > > > -			break;
> > > > -		cpu_relax();
> > > > -	}
> > > > +	readl_poll_timeout(priv + INTEL_RESETS, reg,
> > > > +			   reg & INTEL_RESETS_RESET_DONE, 0,
> > > > +			   INTEL_RESETS_TIMEOUT_US);
> > > >  	writel(INTEL_RESETS_RESET, priv + INTEL_RESETS);
> > > >
> > > >  	return 0;
> > > > --
> > > > 2.51.0
> > > >
> > > >
> > > > --
> > > > linux-i3c mailing list
> > > > linux-i3c@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-i3c
> >
> > --
> > Alexandre Belloni, co-owner and COO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> >
> > --
> > linux-i3c mailing list
> > linux-i3c@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-i3c

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c