lkml.org 
[lkml]   [2016]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code
    From
    Date


    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@microchip.com>
    >> ---
    >>
    >> 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
    >

    \
     
     \ /
      Last update: 2016-08-08 12:41    [W:5.468 / U:0.236 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site