Re: [PATCH] v4l: vsp1: Move subdev operations from HGO to common histogram code

[Niklas Söderlund <niklas.soderlund@xxxxxxxxxxxx>]

Hi Laurent,

Thanks for your patch.

On 2016-09-05 18:13:39 +0300, Laurent Pinchart wrote:
> The code will be shared with the HGT entity, move it to the generic
> histogram implementation.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> ---
>  drivers/media/platform/vsp1/vsp1_drv.c   |   7 +-
>  drivers/media/platform/vsp1/vsp1_hgo.c   | 308 ++--------------------------
>  drivers/media/platform/vsp1/vsp1_hgo.h   |   7 +-
>  drivers/media/platform/vsp1/vsp1_histo.c | 334 +++++++++++++++++++++++++++++--
>  drivers/media/platform/vsp1/vsp1_histo.h |  25 ++-
>  5 files changed, 355 insertions(+), 326 deletions(-)
> 

<snip>

>  int vsp1_histogram_init(struct vsp1_device *vsp1, struct vsp1_histogram *histo,
> -			const char *name, size_t data_size, u32 format)
> +			enum vsp1_entity_type type, const char *name,
> +			const struct vsp1_entity_operations *ops,
> +			const unsigned int *formats, unsigned int num_formats,
> +			size_t data_size, u32 meta_format)
>  {
>  	int ret;
>  
> -	histo->vsp1 = vsp1;
> +	histo->formats = formats;
> +	histo->num_formats = num_formats;
>  	histo->data_size = data_size;
> -	histo->format = format;
> +	histo->meta_format = meta_format;
>  
>  	histo->pad.flags = MEDIA_PAD_FL_SINK;
>  	histo->video.vfl_dir = VFL_DIR_RX;
> @@ -268,7 +559,16 @@ int vsp1_histogram_init(struct vsp1_device *vsp1, struct vsp1_histogram *histo,
>  	INIT_LIST_HEAD(&histo->irqqueue);
>  	init_waitqueue_head(&histo->wait_queue);
>  
> -	/* Initialize the media entity... */
> +	/* Initialize the VSP entity... */
> +	histo->entity.ops = ops;
> +	histo->entity.type = type;
> +
> +	ret = vsp1_entity_init(vsp1, &histo->entity, name, 2, &histo_ops,
> +			       MEDIA_ENT_F_PROC_VIDEO_STATISTICS);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* ... and the media entity... */
>  	ret = media_entity_pads_init(&histo->video.entity, 1, &histo->pad);
>  	if (ret < 0)
>  		return ret;


You forgot to update the histo video device name to match the subdevice 
name here. Something like this makes vsp-tests work for me again.

@@ -577,7 +577,7 @@ int vsp1_histogram_init(struct vsp1_device *vsp1, struct vsp1_histogram *histo,
        histo->video.v4l2_dev = &vsp1->v4l2_dev;
        histo->video.fops = &histo_v4l2_fops;
        snprintf(histo->video.name, sizeof(histo->video.name),
-                "%s histo", name);
+                "%s histo", histo->entity.subdev.name);
        histo->video.vfl_type = VFL_TYPE_GRABBER;
        histo->video.release = video_device_release_empty;
        histo->video.ioctl_ops = &histo_v4l2_ioctl_ops;

Without this fix the names listed using media-ctl -p show:

- entity 1: hgo histo (1 pad, 1 link)

Instead as it did before:

- entity 1: fe928000.vsp1 hgo histo (1 pad, 1 link)

Other then that

Tested-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>

> @@ -293,10 +593,10 @@ int vsp1_histogram_init(struct vsp1_device *vsp1, struct vsp1_histogram *histo,
>  	histo->queue.ops = &histo_video_queue_qops;
>  	histo->queue.mem_ops = &vb2_vmalloc_memops;
>  	histo->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> -	histo->queue.dev = histo->vsp1->dev;
> +	histo->queue.dev = vsp1->dev;
>  	ret = vb2_queue_init(&histo->queue);
>  	if (ret < 0) {
> -		dev_err(histo->vsp1->dev, "failed to initialize vb2 queue\n");
> +		dev_err(vsp1->dev, "failed to initialize vb2 queue\n");
>  		goto error;
>  	}
>  
> @@ -304,7 +604,7 @@ int vsp1_histogram_init(struct vsp1_device *vsp1, struct vsp1_histogram *histo,
>  	histo->video.queue = &histo->queue;
>  	ret = video_register_device(&histo->video, VFL_TYPE_GRABBER, -1);
>  	if (ret < 0) {
> -		dev_err(histo->vsp1->dev, "failed to register video device\n");
> +		dev_err(vsp1->dev, "failed to register video device\n");
>  		goto error;
>  	}
>  
> @@ -314,11 +614,3 @@ error:
>  	vsp1_histogram_cleanup(histo);
>  	return ret;
>  }
> -
> -void vsp1_histogram_cleanup(struct vsp1_histogram *histo)
> -{
> -	if (video_is_registered(&histo->video))
> -		video_unregister_device(&histo->video);
> -
> -	media_entity_cleanup(&histo->video.entity);
> -}

<snip>

-- 
Regards,
Niklas Söderlund