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

[Pavel Machek <pavel@nabladev.com>]

[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]

Hi!

> > > +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()

Can you check the code again? It looked to me like ccc_cmd() does its
own allocation, and frees it. priv_xfers() does allocation, but does
not free it, otherwise being similar to ccc_cmd().

> > > +	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()

Yeah, and then we would need to do two more asserts when the devm_
setup fails.

Perhaps backporting
devm_reset_control_get_optional_exclusive_deasserted() would be best?

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

I won't do fixing this. I'll likely apply patch with known problems,
and will expect you to fix it up in follow up patches.

Best regards,
								Pavel
-- 
In cooperation with Nabla.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]