Re: [cip-dev] [PATCH 6.1.y-cip 04/22] i3c: master: Add basic driver for the Renesas I3C controller

[Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>]

Hi Pavel,
Thanks for your review!


On Fri, Jan 09, 2026 at 11:22:42AM +0100, Pavel Machek wrote:
> Hi!
> 
> Some comments and questions are below.
> 
> > Add a basic driver for the I3C controller found in Renesas RZ/G3S and
> > G3E SoCs. Support I3C pure busses (tested with two targets) and mixed
> > busses (two I3C devices plus various I2C targets). DAA and communication
> > with temperature sensors worked reliably at various speeds.
> > 
> > Missing features such as IBI, HotJoin, and target mode will be added
> > incrementally.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > Link: https://lore.kernel.org/r/20250724094146.6443-5-wsa+renesas@sang-engineering.com
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > [tommaso.merciai:
> >  - Removed  __counted_by(ncmds) from struct renesas_i3c_cmd cmds[]
> >  - Added struct reset_control *tresetn, *presetn into struct
> >    renesas_i3c
> >  - Use devm_reset_control_get_optional_exclusive() into probe function +
> >    reset_control_deassert() and devm_add_action_or_reset() for tresetn,
> >    presetn error path.
> >  - Fixed renesas_i3c_remove() from static void to static int]
> > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> 
> > diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> > new file mode 100644
> > index 000000000000..432be328a4e4
> > --- /dev/null
> > +++ b/drivers/i3c/master/renesas-i3c.c
> ...
> > +	/* Initialize Queue/Buffer threshold */
> > +	renesas_writel(i3c->regs, NQTHCTL, NQTHCTL_IBIDSSZ(6) |
> > +					     NQTHCTL_CMDQTH(1));
> > +
> > +	/* The only supported configuration is two entries*/
> 
> Add a space before */.

Ack, thanks.

> 
> ...
> > +	/* Hot-Join Acknowlege setting. */
> > +	renesas_set_bit(i3c->regs, BCTL, BCTL_HJACKCTL);
> 
> I would not mind consistency in the dots at the end of comment. Normal
> convention is not to have them unless comment is a sentence.

Oks, thanks.

> 
> 
> > +static int renesas_i3c_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *i3c_xfers,
> > +					 int i3c_nxfers)
> > +{
> > +	struct i3c_master_controller *m = i3c_dev_get_master(dev);
> > +	struct renesas_i3c *i3c = to_renesas_i3c(m);
> > +	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
> > +	struct renesas_i3c_xfer *xfer;
> > +	int i;
> > +
> > +	/* Enable I3C bus. */
> > +	renesas_i3c_bus_enable(m, true);
> > +
> > +	xfer = renesas_i3c_alloc_xfer(i3c, 1);
> > +	if (!xfer)
> > +		return -ENOMEM;
> 
> ...where is xfer freed?

Please correct me if I'm wrong but xfer is freed at the end of
renesas_i3c_send_ccc_cmd()

> 
> > +		xfer = i3c->xferqueue.cur;
> > +		cmd = xfer->cmds;
> > +
> > +		/* Clear the Respone Queue Full status flag*/
> > +		renesas_clear_bit(i3c->regs, NTST, NTST_RSPQFF);
> 
> Space before */, please.
> 
> > +	ret = reset_control_deassert(i3c->tresetn);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = reset_control_deassert(i3c->presetn);
> > +	if (ret)
> ## BUG:		return ret;
> 
> > +	ret = devm_add_action_or_reset(&pdev->dev, renesas_i3c_reset_control_assert,
> > +				       i3c->tresetn);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_add_action_or_reset(&pdev->dev, renesas_i3c_reset_control_assert,
> > +				       i3c->presetn);
> > +	if (ret)
> > +		return ret;
> 
> This is wrong, AFAICT. If return happens at marked line, we'll leave
> tresetn asserted.

Ouch, you are right!
This should be:

	ret = reset_control_deassert(i3c->tresetn);
	if (ret)
		return ret;

	ret = reset_control_deassert(i3c->presetn);
	if (ret) {
		reset_control_assert(i3c->tresetn);
		return ret;
	}

As I've introduce these calls because of missing

devm_reset_control_get_optional_exclusive_deasserted()



Should I fix in v2? Or you can take of this while applying?
Thanks.


> 
> Best regards,
> 								Pavel
> -- 
> In cooperation with Nabla.


Kind Regards,
Tommaso