On 01/12/2017 06:43 PM, Ramiro Oliveira wrote:

Hi Hans,

Thank you for your feedback.

On 1/11/2017 11:54 AM, Hans Verkuil wrote:

quoted

Hi Ramiro,

See my review comments below:

On 12/12/16 16:00, Ramiro Oliveira wrote:

quoted

Add support for the DesignWare CSI-2 Host IP Prototyping Kit

Signed-off-by: Ramiro Oliveira <redacted>

[snip]

quoted

quoted

+
+static int vid_dev_subdev_s_power(struct v4l2_subdev *sd, int on)
+{
+    return 0;
+}

Just drop this empty function, shouldn't be needed.

When I start my system I'm hoping all the subdevs have s_power registered. If it
doesn't exist should I change the way I handle it, or will the core handle it
for me?

If it isn't provided, then it is just skipped. The general rule is that
you only provide these ops if they do something useful.

quoted

quoted

+
+static int vid_dev_subdev_registered(struct v4l2_subdev *sd)
+{
+    struct video_device_dev *vid_dev = v4l2_get_subdevdata(sd);
+    struct vb2_queue *q = &vid_dev->vb_queue;
+    struct video_device *vfd = &vid_dev->ve.vdev;
+    int ret;
+
+    memset(vfd, 0, sizeof(*vfd));
+
+    strlcpy(vfd->name, VIDEO_DEVICE_NAME, sizeof(vfd->name));
+
+    vfd->fops = &vid_dev_fops;
+    vfd->ioctl_ops = &vid_dev_ioctl_ops;
+    vfd->v4l2_dev = sd->v4l2_dev;
+    vfd->minor = -1;
+    vfd->release = video_device_release_empty;
+    vfd->queue = q;
+
+    INIT_LIST_HEAD(&vid_dev->vidq.active);
+    init_waitqueue_head(&vid_dev->vidq.wq);
+    memset(q, 0, sizeof(*q));
+    q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+    q->io_modes = VB2_MMAP | VB2_USERPTR;

Add VB2_DMABUF and VB2_READ.

I'll add them, but I'm not using them, is it standard procedure to add them all
even if they aren't used?

You may not use them, but others might. And it doesn't cost anything to add them.

quoted

quoted

+    q->ops = &vb2_video_qops;
+    q->mem_ops = &vb2_vmalloc_memops;

Why is vmalloc used? Can't you use dma_contig or dma_sg and avoid having to copy
the image data? That's a really bad design given the amount of video data that
you have to copy.

When I started development, the arch I was using (ARC) didn't support
dma_contig, so I was forced to use vmalloc.

Since then things have changed and I'm already using dma_contig, however it
wasn't included in this patch. I'll add it to the next patch.

Ah, good. If you are switching to dma_contig, then remove VB2_USERPTR.
VB2_DMABUF should be used instead.

Regards,

	Hans