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);
> - /* 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 ?
> +
> 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.
> @@ -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
