Re: [PATCH v5 10/12] media: rcar-vin: Only expose VIN controls

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

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@ragnatech.se>
> > ---
> > * 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