Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code

From: Wu, Songjun
Date: Mon Aug 08 2016 - 06:03:04 EST




On 8/8/2016 17:37, Hans Verkuil wrote:
On 08/03/2016 10:08 AM, Songjun Wu wrote:
Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

Signed-off-by: Songjun Wu <songjun.wu@xxxxxxxxxxxxx>
---

Changes in v8:
- Power on the sensor on the first open in function
'isc_open'.
- Power off the sensor on the last release in function
'isc_release'.
- Remove the switch of the pipeline.

Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

Changes in v2:
- Add "depends on COMMON_CLK" and "VIDEO_V4L2_SUBDEV_API"
in Kconfig file.
- Correct typos and coding style according to Laurent's remarks
- Delete the loop while in 'isc_clk_enable' function.
- Replace 'hsync_active', 'vsync_active' and 'pclk_sample'
with 'pfe_cfg0' in struct isc_subdev_entity.
- Add the code to support VIDIOC_CREATE_BUFS in
'isc_queue_setup' function.
- Invoke isc_config to configure register in
'isc_start_streaming' function.
- Add the struct completion 'comp' to synchronize with
the frame end interrupt in 'isc_stop_streaming' function.
- Check the return value of the clk_prepare_enable
in 'isc_open' function.
- Set the default format in 'isc_open' function.
- Add an exit condition in the loop while in 'isc_config'.
- Delete the hardware setup operation in 'isc_set_format'.
- Refuse format modification during streaming
in 'isc_s_fmt_vid_cap' function.
- Invoke v4l2_subdev_alloc_pad_config to allocate and
initialize the pad config in 'isc_async_complete' function.
- Remove the '.owner = THIS_MODULE,' in atmel_isc_driver.
- Replace the module_platform_driver_probe() with
module_platform_driver().

drivers/media/platform/Kconfig | 1 +
drivers/media/platform/Makefile | 2 +
drivers/media/platform/atmel/Kconfig | 9 +
drivers/media/platform/atmel/Makefile | 1 +
drivers/media/platform/atmel/atmel-isc-regs.h | 165 +++
drivers/media/platform/atmel/atmel-isc.c | 1503 +++++++++++++++++++++++++
6 files changed, 1681 insertions(+)
create mode 100644 drivers/media/platform/atmel/Kconfig
create mode 100644 drivers/media/platform/atmel/Makefile
create mode 100644 drivers/media/platform/atmel/atmel-isc-regs.h
create mode 100644 drivers/media/platform/atmel/atmel-isc.c


<snip>

diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 0000000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c

<snip>

+static int isc_set_default_fmt(struct isc_device *isc)
+{
+ u32 index = isc->num_user_formats - 1;

Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.

Accept, thank you.
The raw format is listed on the beginning, in order to select non-raw format, the last format is picked. But it's not very important, it's better to use the beginning of the format list.

+ struct v4l2_format f = {
+ .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+ .fmt.pix = {
+ .width = VGA_WIDTH,
+ .height = VGA_HEIGHT,
+ .field = V4L2_FIELD_NONE,
+ .pixelformat = isc->user_formats[index]->fourcc,
+ },
+ };
+
+ return isc_set_fmt(isc, &f);
+}
+
+static int isc_open(struct file *file)
+{
+ struct isc_device *isc = video_drvdata(file);
+ struct v4l2_subdev *sd = isc->current_subdev->sd;
+ int ret;
+
+ if (mutex_lock_interruptible(&isc->lock))
+ return -ERESTARTSYS;
+
+ ret = v4l2_fh_open(file);
+ if (ret < 0)
+ goto unlock;
+
+ if (!v4l2_fh_is_singular_file(file))
+ goto unlock;
+
+ ret = v4l2_subdev_call(sd, core, s_power, 1);
+ if (ret < 0 && ret != -ENOIOCTLCMD)
+ goto unlock;
+
+ ret = isc_set_default_fmt(isc);

This doesn't belong here, this needs to be done in isc_async_complete().

Having the code here means that every time you open the device, the format
changes back to the default. That's not what you want.

Only on the first open, the default format will be set. If it's done in isc_async_complete(), there is a problem, the sensor is powered off, the default format maybe not be set successfully, because in some sensor drivers, calling set_fmt will set the register directly.

+ if (ret)
+ goto unlock;
+
+unlock:
+ mutex_unlock(&isc->lock);
+ return ret;
+}
+

<snip>

+static int isc_async_complete(struct v4l2_async_notifier *notifier)
+{
+ struct isc_device *isc = container_of(notifier->v4l2_dev,
+ struct isc_device, v4l2_dev);
+ struct isc_subdev_entity *sd_entity;
+ struct video_device *vdev = &isc->video_dev;
+ struct vb2_queue *q = &isc->vb2_vidq;
+ int ret;
+
+ isc->current_subdev = container_of(notifier,
+ struct isc_subdev_entity, notifier);
+ sd_entity = isc->current_subdev;
+
+ mutex_init(&isc->lock);
+ init_completion(&isc->comp);
+
+ /* Initialize videobuf2 queue */
+ q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_READ;
+ q->drv_priv = isc;
+ q->buf_struct_size = sizeof(struct isc_buffer);
+ q->ops = &isc_vb2_ops;
+ q->mem_ops = &vb2_dma_contig_memops;
+ q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+ q->lock = &isc->lock;
+ q->min_buffers_needed = 1;
+ q->dev = isc->dev;
+
+ ret = vb2_queue_init(q);
+ if (ret < 0) {
+ v4l2_err(&isc->v4l2_dev,
+ "vb2_queue_init() failed: %d\n", ret);
+ return ret;
+ }
+
+ /* Init video dma queues */
+ INIT_LIST_HEAD(&isc->dma_queue);
+ spin_lock_init(&isc->dma_queue_lock);
+
+ /* Register video device */
+ strlcpy(vdev->name, ATMEL_ISC_NAME, sizeof(vdev->name));
+ vdev->release = video_device_release_empty;
+ vdev->fops = &isc_fops;
+ vdev->ioctl_ops = &isc_ioctl_ops;
+ vdev->v4l2_dev = &isc->v4l2_dev;
+ vdev->vfl_dir = VFL_DIR_RX;
+ vdev->queue = q;
+ vdev->lock = &isc->lock;
+ vdev->ctrl_handler = isc->current_subdev->sd->ctrl_handler;
+ vdev->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_CAPTURE;
+ video_set_drvdata(vdev, isc);
+
+ ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
+ if (ret < 0) {
+ v4l2_err(&isc->v4l2_dev,
+ "video_register_device failed: %d\n", ret);
+ return ret;
+ }

This should be done last. Once the device is registered apps can use it, so
everything should be configured.

Accept, thank you.

+
+ sd_entity->config = v4l2_subdev_alloc_pad_config(sd_entity->sd);
+ if (sd_entity->config == NULL)
+ return -ENOMEM;
+
+ ret = isc_formats_init(isc);
+ if (ret < 0) {
+ v4l2_err(&isc->v4l2_dev,
+ "Init format failed: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}

Regards,

Hans