Hi Hans,
Hans Verkuil <hverk...@xs4all.nl> writes:
> On 04/02/2016 04:26 PM, Robert Jarzmik wrote:
>> diff --git a/drivers/media/platform/soc_camera/pxa_camera.c
>> b/drivers/media/platform/soc_camera/pxa_camera.c
>> index b8dd878e98d6..30d266bbab55 100644
>> --- a/drivers/media/platform/soc_camera/pxa_camera.c
>> +++ b/drivers/media/platform/soc_camera/pxa_camera.c
>
> When you prepare the final patch series, please put the driver in
> drivers/media/platform/pxa-camera and not in the soc-camera directory.
Sure.
Would you accept the final patch to make the move, so that I keep the
bisectability, ie. that all patches are applied while still in ../soc_camera,
and then the final one making just the move to .../platform ?
>> + if (format->name)
>> + strlcpy(f->description, format->name, sizeof(f->description));
>
> You can drop this since the core fills in the description. That means the
> 'name' field of struct soc_mbus_pixelfmt is not needed either.
Ok, let's try this, for v3.
>> +static int pxac_vidioc_querycap(struct file *file, void *priv,
>> + struct v4l2_capability *cap)
>> +{
>> + strlcpy(cap->bus_info, "platform:pxa-camera", sizeof(cap->bus_info));
>> + strlcpy(cap->driver, PXA_CAM_DRV_NAME, sizeof(cap->driver));
>> + strlcpy(cap->card, pxa_cam_driver_description, sizeof(cap->card));
>> + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
>> + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
>
> Tiny fix: you can drop the capabilities assignment: the v4l2 core does that
> for you these days.
Well, above kernel v4.7, if I drop the assignement, v4l2-compliance -s finds 2
new errors:
Required ioctls:
fail: v4l2-compliance.cpp(534): dcaps & ~caps
test VIDIOC_QUERYCAP: FAIL
Allow for multiple opens:
test second video open: OK
fail: v4l2-compliance.cpp(534): dcaps & ~caps
test VIDIOC_QUERYCAP: FAIL
test VIDIOC_G/S_PRIORITY: OK
So there is something fishy here if the core provides this data ...
>> +static int pxac_vidioc_enum_input(struct file *file, void *priv,
>> + struct v4l2_input *i)
>> +{
>> + if (i->index > 0)
>> + return -EINVAL;
>> +
>> + memset(i, 0, sizeof(*i));
>
> The memset can be dropped, it's cleared for you.
OK, for v3.
>> +static void pxac_vb2_queue(struct vb2_buffer *vb)
>> +{
>> + struct pxa_buffer *buf = vb2_to_pxa_buffer(vb);
>> + struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vb->vb2_queue);
>> +
>> + dev_dbg(pcdev_to_dev(pcdev),
>> + "%s(vb=%p) nb_channels=%d size=%lu active=%p\n",
>> + __func__, vb, pcdev->channels, vb2_get_plane_payload(vb, 0),
>> + pcdev->active);
>> +
>> + list_add_tail(&buf->queue, &pcdev->capture);
>> +
>> + pxa_dma_add_tail_buf(pcdev, buf);
>> +
>> + if (!pcdev->active)
>> + pxa_camera_start_capture(pcdev);
>
> This is normally done from start_streaming. Are you really sure this is the
> right
> place? I strongly recommend moving this start_capture call.
Well, it was at least with the previous framework.
Previously this was done here to "hot-queue" a buffer :
- if a previous capture was still running, the buffer was queued by
pxa_dma_add_tail_buf(), and no restart of the DMA pump was necessary
- if the previous capture was finished, a new one was initiated
Now if the new framework takes care of that, I can move the
pxa_camera_start_capture() into start_streaming(), no problem, let me try in the
next patchset. That might take a bit of time because testing both the
"hot-queue" and the "queue but hot-queuing missed" is a bit tricky.
> You may also want to use the struct vb2queue min_buffers_needed field to
> specify
> the minimum number of buffers that should be queued up before start_streaming
> can
> be called. Whether that's needed depends on your DMA engine.
I have no minimum required by the pxa dmaengine driver, that's good.
>> +
>> + v4l2_disable_ioctl(vdev, VIDIOC_G_STD);
>> + v4l2_disable_ioctl(vdev, VIDIOC_S_STD);
>> + v4l2_disable_ioctl(vdev, VIDIOC_ENUMSTD);
>
> I don't think this is needed since the tvnorms field of struct video_device
> == 0,
> signalling that there is no STD support.
OK, for v3.
Cheers.
--
Robert
