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: >> Hi Ramiro, >> >> See my review comments below: >> >> On 12/12/16 16:00, Ramiro Oliveira wrote: >>> Add support for the DesignWare CSI-2 Host IP Prototyping Kit >>> >>> Signed-off-by: Ramiro Oliveira <roliveir@xxxxxxxxxxxx> > > [snip] >>> + >>> +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. > >>> + >>> +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. > >>> + 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html