Messages in this thread | | | Subject | Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code | From | "Wu, Songjun" <> | Date | Mon, 8 Aug 2016 18:02:10 +0800 |
| |
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 >
| |