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:0.084 / U:0.344 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site