Hi Laurent,
Thanks for your feedback.
On 2025-06-13 02:28:20 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Fri, Jun 06, 2025 at 08:26:04PM +0200, Niklas Söderlund wrote:
> > Before moving Gen2 to media controller simplify the creation of controls
> > by not exposing the sub-device controls on the video device. This could
> > be done while enabling media controller but doing it separately reduces
> > the changes needed to do so.
> >
> > The rework also allows the cleanup and remove paths to be simplified by
> > folding all special cases into the only remaining call site.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > * Changes since v4
> > - Broken out from a larger patch.
> > ---
> > .../platform/renesas/rcar-vin/rcar-core.c | 84 ++++---------------
> > 1 file changed, 18 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > index 8cb3d0a3520f..597e868a6bc5 100644
> > --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> > @@ -365,14 +365,6 @@ static int rvin_group_parse_of(struct rvin_dev *vin, unsigned int port,
> > return ret;
> > }
> >
> > -static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
> > -{
> > - if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
> > - v4l2_async_nf_unregister(&vin->group->notifier);
> > - v4l2_async_nf_cleanup(&vin->group->notifier);
> > - }
> > -}
> > -
> > static int rvin_parallel_parse_of(struct rvin_dev *vin)
> > {
> > struct fwnode_handle *fwnode __free(fwnode_handle) = NULL;
> > @@ -510,7 +502,7 @@ static void rvin_free_controls(struct rvin_dev *vin)
> > vin->vdev.ctrl_handler = NULL;
> > }
> >
> > -static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev)
> > +static int rvin_create_controls(struct rvin_dev *vin)
> > {
> > int ret;
> >
> > @@ -528,16 +520,6 @@ static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev
> > return ret;
> > }
> >
>
> Now that the control handler for the video device only has a single
> control, you can reduce the number of buckets:
>
> - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1);
Agreed.
>
> > - /* For the non-MC mode add controls from the subdevice. */
> > - if (subdev) {
> > - ret = v4l2_ctrl_add_handler(&vin->ctrl_handler,
> > - subdev->ctrl_handler, NULL, true);
> > - if (ret < 0) {
> > - rvin_free_controls(vin);
> > - return ret;
> > - }
> > - }
> > -
> > vin->vdev.ctrl_handler = &vin->ctrl_handler;
> >
> > return 0;
> > @@ -627,11 +609,6 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> > if (ret < 0 && ret != -ENOIOCTLCMD)
> > return ret;
> >
> > - /* Add the controls */
> > - ret = rvin_create_controls(vin, subdev);
> > - if (ret < 0)
> > - return ret;
> > -
> > vin->parallel.subdev = subdev;
> >
> > return 0;
> > @@ -885,34 +862,17 @@ static int rvin_csi2_setup_links(struct rvin_group *group)
> > return ret;
> > }
> >
> > -static void rvin_csi2_cleanup(struct rvin_dev *vin)
> > -{
> > - rvin_group_notifier_cleanup(vin);
> > - rvin_free_controls(vin);
> > -}
> > -
> > static int rvin_csi2_init(struct rvin_dev *vin)
> > {
> > int ret;
> >
> > -
> > - ret = rvin_create_controls(vin, NULL);
> > - if (ret < 0)
> > - return ret;
> > -
> > ret = rvin_group_get(vin, rvin_csi2_setup_links, &rvin_csi2_media_ops);
> > if (ret)
> > - goto err_controls;
> > + return ret;
> >
> > ret = rvin_group_notifier_init(vin, 1, RVIN_CSI_MAX);
> > if (ret)
> > - goto err_group;
> > -
> > - return 0;
> > -err_group:
> > - rvin_group_put(vin);
> > -err_controls:
> > - rvin_free_controls(vin);
> > + rvin_group_put(vin);
> >
> > return ret;
> > }
> > @@ -966,34 +926,17 @@ static int rvin_isp_setup_links(struct rvin_group *group)
> > return ret;
> > }
> >
> > -static void rvin_isp_cleanup(struct rvin_dev *vin)
> > -{
> > - rvin_group_notifier_cleanup(vin);
> > - rvin_free_controls(vin);
> > -}
> > -
> > static int rvin_isp_init(struct rvin_dev *vin)
> > {
> > int ret;
> >
> > -
> > - ret = rvin_create_controls(vin, NULL);
> > - if (ret < 0)
> > - return ret;
> > -
> > ret = rvin_group_get(vin, rvin_isp_setup_links, NULL);
> > if (ret)
> > - goto err_controls;
> > + return ret;
> >
> > ret = rvin_group_notifier_init(vin, 2, RVIN_ISP_MAX);
> > if (ret)
> > - goto err_group;
> > -
> > - return 0;
> > -err_group:
> > - rvin_group_put(vin);
> > -err_controls:
> > - rvin_free_controls(vin);
> > + rvin_group_put(vin);
> >
> > return ret;
> > }
> > @@ -1372,6 +1315,10 @@ static int rcar_vin_probe(struct platform_device *pdev)
> > if (ret)
> > return ret;
> >
> > + ret = rvin_create_controls(vin);
> > + if (ret < 0)
> > + return ret;
>
> Don't you need to call rvin_dma_unregister() here ?
Indeed, there is a similar issue where rvin_id_put() is not called where
it should introduced by this series.
>
> > +
> > if (vin->info->use_isp) {
> > ret = rvin_isp_init(vin);
> > } else if (vin->info->use_mc) {
> > @@ -1390,6 +1337,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
> > }
> >
> > if (ret) {
> > + rvin_free_controls(vin);
> > rvin_dma_unregister(vin);
> > rvin_id_put(vin);
> > return ret;
>
> Error labels at the end of the function will make this more manageable.
I agree, I have added a small patch early in this series that converts
this error path to use labels and fixed the missing call to
rvin_id_put() and rvin_dma_unregister() in the error paths using labels
as you suggest it turns out nicer.
>
> > @@ -1409,13 +1357,17 @@ static void rcar_vin_remove(struct platform_device *pdev)
> >
> > rvin_v4l2_unregister(vin);
> >
> > - if (vin->info->use_isp)
> > - rvin_isp_cleanup(vin);
> > - else if (vin->info->use_mc)
> > - rvin_csi2_cleanup(vin);
> > + if (vin->info->use_isp || vin->info->use_mc) {
> > + if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
> > + v4l2_async_nf_unregister(&vin->group->notifier);
> > + v4l2_async_nf_cleanup(&vin->group->notifier);
> > + }
> > + }
> >
> > rvin_group_put(vin);
> >
> > + rvin_free_controls(vin);
> > +
> > rvin_id_put(vin);
> >
> > rvin_dma_unregister(vin);
>
> --
> Regards,
>
> Laurent Pinchart
--
Kind Regards,
Niklas Söderlund
