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

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

On Tue, Sep 08, 2026 at 09:17:15AM +0200, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Mon, Sep 07, 2026 at 09:45:16AM +0200, Jacopo Mondi wrote:
> > Hi Sakari, thanks for the review
> >
> 
> [snip]
> 
> > > > +
> > > > +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
> > > > +	};
> > > > +
> > > > +	endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);
> > > > +	if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg))
> > > > +		return dev_err_probe(dev, -EINVAL, "Failed to parse endpoint\n");
> > >
> > > Don't mask error codes! Just return the error code returned by
> > > v4l2_fwnode_endpoint_alloc_parse().
> > >
> >
> > With PTR_ERR() I presume
> >
> > > > +
> > > > +	/*
> > > > +	 * Link frequencies: the driver supports a single link frequency,
> > > > +	 * no need to check bitmap after this call.
> > > > +	 */
> > > > +	if (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)) {
> > >
> > > Ditto.
> > >
> > > > +		v4l2_fwnode_endpoint_free(&ep_cfg);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	/* TODO: Implement D-PHY configuration to support continuous clock. */
> > > > +	if (!(ep_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)) {
> > > > +		v4l2_fwnode_endpoint_free(&ep_cfg);
> > >
> > > Instead of callind v4l2_fwnode_endpoint_free() here and above, I'd add a
> > > label for error handling.
> > >
> >
> > ack
> 
> I'll actually backtrack on this.
> 
> Sashiko pointed out that mixing gotos and cleanups is probably not a good idea
> and this time, the bot is right.

I'm not quite sure what you mean. The general practice is that if error
handling is trivial and there's only a single location to unwind something,
you should do it on the site. In more complex cases use labels and gotos.
That's what we have here. (There are of course more complicated cases where
it's not that simple, but this isn't what we're discussing here.)

-- 
Sakari Ailus