Re: [PATCH v2 2/2] i3c: master: Add basic driver for the Renesas I3C controller

From: Wolfram Sang <wsa+renesas@sang-engineering.com>
To: Frank Li <Frank.li@nxp.com>
Cc: linux-renesas-soc@vger.kernel.org,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>,
	Kees Cook <kees@kernel.org>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Magnus Damm <magnus.damm@gmail.com>,
	linux-i3c@lists.infradead.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2 2/2] i3c: master: Add basic driver for the Renesas I3C controller
Date: Fri, 18 Jul 2025 12:48:31 +0200	[thread overview]
Message-ID: <aHomfzV_dJMwFyCN@shikoro> (raw)
In-Reply-To: <aHlJ8KQzcamyaZA1@lizhi-Precision-Tower-5810>

Hi Frank,

> > +#define NDBSTLV0		0x398
> > +#define  NDBSTLV0_RDBLV(x)	(((x) >> 8) & 0xff)
> 
> Can you use FILE_GET()?

You mean FIELD_GET? Probably yes.

> > +#define RENESAS_I3C_MAX_DEVS	8
> > +#define I2C_INIT_MSG		-1
> > +
> > +/* Bus condition timing */
> > +#define I3C_BUS_THIGH_MIXED_NS	40		/* 40ns */
> > +#define I3C_BUS_FREE_TIME_NS	1300		/* 1.3us for Mixed Bus with I2C FM Device*/
> > +#define I3C_BUS_AVAL_TIME_NS	1000		/* 1us */
> > +#define I3C_BUS_IDLE_TIME_NS	200000		/* 200us */
> 
> Do you have document reference to such timeout value?  If it is spec defined
> timeout, please move to master.h and add ref to spec sections number.

They are all in the specs. Will move them.

> > +#define XFER_TIMEOUT		(msecs_to_jiffies(1000))
> 
> Is it engineer choosen timeout or spec defined? add comments to show why
> choose this timeout value.

Consistency. All current I3C controller drivers use this value. If we
want to improve it, we should do it in a seperate series for all drivers
IMO.

> > +	/* Wait for reset completion  */
> > +	return readl_relaxed_poll_timeout(i3c->regs + RSTCTL, val,
> > +					  !(val & RSTCTL_RI3CRST), 0, 1000);
> 
> All you use customer's readl at other place. here, you should use
> read_poll_timeout(renesas_i3c_reg_read, ...) to keep consistent. check other
> place.

I will use regmap_read_poll_timeout().

> > +			pp_high_ticks = DIV_ROUND_UP(I3C_BUS_THIGH_MIXED_NS,
> > +						     1000000000 / rate);
> 
> 100000000 use NSEC_PER_SEC, check other place.

Ack.

> > +	/* Extended Bit Rate setting */
> > +	renesas_i3c_reg_write(i3c->regs, EXTBR, EXTBR_EBRLO(od_low_ticks) |
> > +					   EXTBR_EBRHO(od_high_ticks) |
> > +					   EXTBR_EBRLP(pp_low_ticks) |
> > +					   EXTBR_EBRHP(pp_high_ticks));
> 
> I feel renesas_i3c_reg_write is too long, renesas_write() should be enough.

It is long, but precise. renesas_write() could mean anything. It would
also be confusing if some functions start with renesas_* only and some
with renesas_i3c_*. But this is already too much bike-shedding for my
taste. I will do the extra work and switch to regmap and hope that the
overhead is not noticable. I hope I can squeeze this in today.

> > +static struct renesas_i3c_irq_desc renesas_i3c_irqs[] = {
> 
> const?

Yes.

Kind regards,

   Wolfram