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

[Sakari Ailus]

Hi Jacopo,

On Tue, Sep 08, 2026 at 11:21:51AM +0200, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Tue, Sep 08, 2026 at 11:12:45AM +0300, Sakari Ailus wrote:
> > 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.)
> 
> include/linux/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.
> 
> I take it as "do not mix gotos and cleanups"
> 
> As the 2 cleanup paths are trivial, I would rather do not mix the two.

I don't recall asking for that.

-- 
Sakari Ailus