Re: [PATCH] media: sun6i-mipi-csi2: Use V4L2 subdev active state

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Sun, Feb 08, 2026 at 02:23:39PM +0330, arash golgol wrote:
> Hi Laurent,
> 
> Thank you for the careful review and the detailed comments.
> 
> On Sun, Feb 8, 2026 at 1:19 AM Laurent Pinchart wrote:
> > On Fri, Feb 06, 2026 at 04:04:55PM +0330, Arash Golgol wrote:
> ...
> >
> > You can drop the else and write
> >
> >         ret = v4l2_subdev_call(source_subdev, video, s_stream, 1);
> >         if (ret && ret != -ENOIOCTLCMD)
> >                 goto disable;
> >
> >         ret = 0;
> >         goto unlock;
> >
> 
> Sure, I'll do this in v2.
> 
> >
> > Error paths below should call v4l2_subdev_cleanup(), which should be ...
> > ... introduced here:
> >
> > error_v4l2_subdev_cleanup:
> >         v4l2_subdev_cleanup(subdev);
> 
> Yes, you're right about error handling.
> I was checking the code for the proper place to jump to the
> error_v4l2_subdev_cleanup label.
> Consider this section of code, trying to set up the bridge upstream
> source (in middle of
>  sun6i_mipi_csi2_bridge_setup()).
> 
>         ret = sun6i_mipi_csi2_bridge_source_setup(csi2_dev);
>         if (ret && ret != -ENODEV)
>                  goto error_v4l2_notifier_cleanup;
> 
>         (rest of the code)
> 
>         error_v4l2_notifier_cleanup:
>                 v4l2_async_nf_cleanup(notifier);
> 
> I was wondering whether jumping to error_v4l2_notifier_cleanup on failure of
> sun6i_mipi_csi2_bridge_source_setup() is meaningful, as in that case no
> async connection has been added to the notifier yet.

It doesn't seem needed indeed, but it also doesn't hurt.

> So I guess failure of sun6i_mipi_csi2_bridge_source_setup() could be a
> proper place to jump to
> error_v4l2_subdev_cleanup label. In this model, failure to set up the
> upstream source means
> the bridge cannot function at all, so I unwind back to subdev_cleanup().
> 
> Something like this:
> 
>         ret = v4l2_subdev_init_finalize(subdev);
>         if (ret < 0)
>                 goto error_media_entity_cleanup;
> 
>         /* V4L2 Async */
> 
>         v4l2_async_subdev_nf_init(notifier, subdev);
>         notifier->ops = &sun6i_mipi_csi2_notifier_ops;
> 
>         ret = sun6i_mipi_csi2_bridge_source_setup(csi2_dev);
>         if (ret && ret != -ENODEV)
>                 goto error_v4l2_subdev_cleanup;
> 
>        (rest of the code ... )
> 
>         error_v4l2_subdev_cleanup:
>                 v4l2_subdev_cleanup(subdev);
> 
>         error_media_entity_cleanup:
>                 media_entity_cleanup(&subdev->entity);
> 
> Is my understanding correct?
> If yes, should I add this change to v2 too?

That looks good to me.

> > Those are only small issues, overall the patch is very nice. Thank you
> > for taking the extra step to convert the driver to the V4L2 subdev
> > active state. I expect we'll merge the next version once the comments
> > are addressed.
> 
> Thank you for your kind words.
> I've prepared v2 and addressed your comments, but I wanted to clarify
> the correct error handling approach here before sending it.

-- 
Regards,

Laurent Pinchart