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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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@ideasonboard.com>
> > > +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.

> 
> >
> > 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