Re: [PATCH v5 2/2] media: i2c: mira016: Add driver for Mira016

[Jacopo Mondi]

On Thu, Oct 01, 2026 at 10:52:35AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Thu, Oct 01, 2026 at 09:19:02AM +0200, Jacopo Mondi wrote:
> > Hi Sakari
> >
> > On Thu, Oct 01, 2026 at 09:46:08AM +0300, Sakari Ailus wrote:
> > > Hi Jacopo,
> > >
> > > On Wed, Sep 30, 2026 at 12:51:20PM +0200, Jacopo Mondi wrote:
> > > > Add driver for the ams OSRAM Mira016 sensor.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > > > +static int mira016_parse_endpoint(struct device *dev, struct mira016 *mira016)
> > > > +{
> > > > +	struct fwnode_handle *endpoint __free(fwnode_handle) = NULL;
> > > > +	struct v4l2_fwnode_endpoint ep_cfg = {
> > > > +		.bus_type = V4L2_MBUS_CSI2_DPHY
> > > > +	};
> > > > +	int ret;
> > > > +
> > > > +	endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);
> > > > +	if (!endpoint)
> > > > +		return -ENODEV;
> > > > +
> > > > +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/*
> > > > +	 * Link frequencies: the driver supports a single link frequency,
> > > > +	 * no need to check bitmap after this call.
> > > > +	 */
> > > > +	ret = v4l2_link_freq_to_bitmap(dev, ep_cfg.link_frequencies,
> > > > +				       ep_cfg.nr_of_link_frequencies,
> > > > +				       mira016_link_freqs,
> > > > +				       ARRAY_SIZE(mira016_link_freqs),
> > > > +				       &mira016->link_freq_bitmap);
> > > > +	if (ret) {
> > > > +		v4l2_fwnode_endpoint_free(&ep_cfg);
> > >
> > > I recall commenting about this at in least three occasions earlier.
> >
> > And, again, I have replied twice to your comment without receiving a
> > response: https://lore.kernel.org/linux-media/arPJ_-yKVMXE-Gav@zed/
> >
> > I'll repeat here anyway: do not mix cleanups and gotos. In this case
> > it's harmless, but why contradict the usage notes to avoid typing out
> > v4l2_fwnode_endpoint_free() 2 times ?
>
> It's not about typing but correct error handling. It's much easier to miss
> unwinding whatever needs to be unwound in multiple places when you're not
> using goto's.
>
> In other words, the pattern you're following is bad, please stop using it.
>

You seem to be missing my main point, and if you think it's not
correct please tell me why instead of keep repeating the same thing
over and over.

This routine uses cleanups because of

	struct fwnode_handle *endpoint __free(fwnode_handle) = NULL;

functions using cleanups shall not use gotos

from cleanup.h

 * Lastly, given that the benefit of cleanup helpers is removal of
 * "goto", and that the "goto" statement can jump between scopes, the
 * expectation is that usage of "goto" and cleanup helpers is never
 * mixed in the same function. I.e. for a given routine, convert all
 * resources that need a "goto" cleanup to scope-based cleanup, or
 * convert none of them.

As said, in this case is harmless, but given that this function might
be extended I wouldn't introduce a goto now to later having to care if
it gets in the way of the cleanup path or not.



> >
> > >
> > > Also applies to the other driver.
> > >
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	/* TODO: Implement D-PHY configuration to support continuous clock. */
> > > > +	if (!(ep_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)) {
> > > > +		dev_err(dev, "Continuous clock is not supported\n");
> > > > +		v4l2_fwnode_endpoint_free(&ep_cfg);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	mira016->bus_config = ep_cfg.bus.mipi_csi2.flags;
> > > > +
> > > > +	v4l2_fwnode_endpoint_free(&ep_cfg);
> > > > +
> > > > +	return 0;
> > > > +}
> > >
>
> --
> Kind regards,
>
> Sakari Ailus