Re: [PATCH v1 5/9] media: vimc: Centralize subdev internal_ops initialization

[Shuah Khan <skhan@linuxfoundation.org>]

On 4/24/24 17:57, Laurent Pinchart wrote:
> Initialize the subdev internal_ops field in the vimc_ent_sd_register()
> function. This handles the internal ops the same way as the subdev ops,
> and prepares for moving to the V4L2 subdev active state API.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/media/test-drivers/vimc/vimc-common.c  | 2 ++
>   drivers/media/test-drivers/vimc/vimc-common.h  | 2 ++
>   drivers/media/test-drivers/vimc/vimc-debayer.c | 5 ++---
>   drivers/media/test-drivers/vimc/vimc-lens.c    | 2 +-
>   drivers/media/test-drivers/vimc/vimc-scaler.c  | 5 ++---
>   drivers/media/test-drivers/vimc/vimc-sensor.c  | 4 +---
>   6 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vimc/vimc-common.c b/drivers/media/test-drivers/vimc/vimc-common.c
> index 2e72974e35b4..3da2271215c6 100644
> --- a/drivers/media/test-drivers/vimc/vimc-common.c
> +++ b/drivers/media/test-drivers/vimc/vimc-common.c
> @@ -358,6 +358,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>   			 u32 function,
>   			 u16 num_pads,
>   			 struct media_pad *pads,
> +			 const struct v4l2_subdev_internal_ops *int_ops,
>   			 const struct v4l2_subdev_ops *sd_ops)
>   {
>   	int ret;
> @@ -367,6 +368,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>   
>   	/* Initialize the subdev */
>   	v4l2_subdev_init(sd, sd_ops);
> +	sd->internal_ops = int_ops;
>   	sd->entity.function = function;
>   	sd->entity.ops = &vimc_ent_sd_mops;
>   	sd->owner = THIS_MODULE;
> diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
> index 6a76717e0384..7a45a2117748 100644
> --- a/drivers/media/test-drivers/vimc/vimc-common.h
> +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> @@ -215,6 +215,7 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat);
>    * @num_pads:	number of pads to initialize
>    * @pads:	the array of pads of the entity, the caller should set the
>    *		flags of the pads
> + * @int_ops:	pointer to &struct v4l2_subdev_internal_ops.
>    * @sd_ops:	pointer to &struct v4l2_subdev_ops.
>    *
>    * Helper function initialize and register the struct vimc_ent_device and struct
> @@ -227,6 +228,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>   			 u32 function,
>   			 u16 num_pads,
>   			 struct media_pad *pads,
> +			 const struct v4l2_subdev_internal_ops *int_ops,
>   			 const struct v4l2_subdev_ops *sd_ops);
>   
>   /**
> diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
> index e2f12a7fb58f..d4ca57b3672d 100644
> --- a/drivers/media/test-drivers/vimc/vimc-debayer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
> @@ -591,12 +591,11 @@ static struct vimc_ent_device *vimc_debayer_add(struct vimc_device *vimc,
>   	ret = vimc_ent_sd_register(&vdebayer->ved, &vdebayer->sd, v4l2_dev,
>   				   vcfg_name,
>   				   MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2,
> -				   vdebayer->pads, &vimc_debayer_ops);
> +				   vdebayer->pads, &vimc_debayer_internal_ops,
> +				   &vimc_debayer_ops);
>   	if (ret)
>   		goto err_free_hdl;
>   
> -	vdebayer->sd.internal_ops = &vimc_debayer_internal_ops;
> -
>   	vdebayer->ved.process_frame = vimc_debayer_process_frame;
>   	vdebayer->ved.dev = vimc->mdev.dev;
>   	vdebayer->mean_win_size = vimc_debayer_ctrl_mean_win_size.def;
> diff --git a/drivers/media/test-drivers/vimc/vimc-lens.c b/drivers/media/test-drivers/vimc/vimc-lens.c
> index e7d78fa8ccdb..42c58a3e3f1b 100644
> --- a/drivers/media/test-drivers/vimc/vimc-lens.c
> +++ b/drivers/media/test-drivers/vimc/vimc-lens.c
> @@ -72,7 +72,7 @@ static struct vimc_ent_device *vimc_lens_add(struct vimc_device *vimc,
>   
>   	ret = vimc_ent_sd_register(&vlens->ved, &vlens->sd, v4l2_dev,
>   				   vcfg_name, MEDIA_ENT_F_LENS, 0,
> -				   NULL, &vimc_lens_ops);
> +				   NULL, NULL, &vimc_lens_ops);
>   	if (ret)
>   		goto err_free_hdl;
>   
> diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
> index 013cd84f82be..4f9c44a663e1 100644
> --- a/drivers/media/test-drivers/vimc/vimc-scaler.c
> +++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
> @@ -421,14 +421,13 @@ static struct vimc_ent_device *vimc_scaler_add(struct vimc_device *vimc,
>   	ret = vimc_ent_sd_register(&vscaler->ved, &vscaler->sd, v4l2_dev,
>   				   vcfg_name,
>   				   MEDIA_ENT_F_PROC_VIDEO_SCALER, 2,
> -				   vscaler->pads, &vimc_scaler_ops);
> +				   vscaler->pads, &vimc_scaler_internal_ops,
> +				   &vimc_scaler_ops);
>   	if (ret) {
>   		kfree(vscaler);
>   		return ERR_PTR(ret);
>   	}
>   
> -	vscaler->sd.internal_ops = &vimc_scaler_internal_ops;
> -
>   	vscaler->ved.process_frame = vimc_scaler_process_frame;
>   	vscaler->ved.dev = vimc->mdev.dev;
>   
> diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
> index 11df18332865..5c31d9435cdd 100644
> --- a/drivers/media/test-drivers/vimc/vimc-sensor.c
> +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
> @@ -424,12 +424,10 @@ static struct vimc_ent_device *vimc_sensor_add(struct vimc_device *vimc,
>   	ret = vimc_ent_sd_register(&vsensor->ved, &vsensor->sd, v4l2_dev,
>   				   vcfg_name,
>   				   MEDIA_ENT_F_CAM_SENSOR, 1, &vsensor->pad,
> -				   &vimc_sensor_ops);
> +				   &vimc_sensor_internal_ops, &vimc_sensor_ops);
>   	if (ret)
>   		goto err_free_tpg;
>   
> -	vsensor->sd.internal_ops = &vimc_sensor_internal_ops;
> -
>   	vsensor->ved.process_frame = vimc_sensor_process_frame;
>   	vsensor->ved.dev = vimc->mdev.dev;
>   

Looks good to me.

thanks,
-- Shuah