Hi Songjun,
Some more comments below. Except for one in the open/release functions
it's all small things.
On 07/29/2016 09:54 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 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 | 1611 +++++++++++++++++++++++++
> 6 files changed, 1789 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..f2ef664
> --- /dev/null
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -0,0 +1,1611 @@
<snip>
> +static unsigned int sensor_preferred = 1;
> +module_param(sensor_preferred, uint, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(sensor_preferred,
> + "Sensor is preferred to output the specified format (1-on 0-off) default 1");
I have no idea what this means. Can you elaborate? Why would you want to set this to 0?
<snip>
> +static inline bool sensor_is_preferred(const struct isc_format *isc_fmt)
> +{
> + if ((sensor_preferred && isc_fmt->sd_support) ||
> + !isc_fmt->isc_support)
I'd just do:
return (sensor_preferred && isc_fmt->sd_support) ||
!isc_fmt->isc_support;
> + return true;
> + else
> + return false;
> +}
> +
<snip>
> +static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f,
> + struct isc_format **current_fmt, u32 *code)
> +{
> + struct isc_format *isc_fmt;
> + struct v4l2_pix_format *pixfmt = &f->fmt.pix;
> + struct v4l2_subdev_format format = {
> + .which = V4L2_SUBDEV_FORMAT_TRY,
> + };
> + u32 mbus_code;
> + int ret;
> +
> + if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> + return -EINVAL;
> +
> + isc_fmt = find_format_by_fourcc(isc, pixfmt->pixelformat);
> + if (!isc_fmt) {
> + v4l2_warn(&isc->v4l2_dev, "Format 0x%x not found\n",
> + pixfmt->pixelformat);
> + isc_fmt = isc->user_formats[isc->num_user_formats - 1];
> + pixfmt->pixelformat = isc_fmt->fourcc;
> + }
> +
> + /* Limit to Atmel ISC hardware capabilities */
> + if (pixfmt->width > ISC_MAX_SUPPORT_WIDTH)
> + pixfmt->width = ISC_MAX_SUPPORT_WIDTH;
> + if (pixfmt->height > ISC_MAX_SUPPORT_HEIGHT)
> + pixfmt->height = ISC_MAX_SUPPORT_HEIGHT;
> +
> + if (sensor_is_preferred(isc_fmt))
> + mbus_code = isc_fmt->sd_mbus_code;
> + else
> + mbus_code = isc_fmt->isc_mbus_code;
> +
> + v4l2_fill_mbus_format(&format.format, pixfmt, mbus_code);
> + ret = v4l2_subdev_call(isc->current_subdev->sd, pad, set_fmt,
> + isc->current_subdev->config, &format);
> + if (ret < 0)
> + return ret;
> +
> + v4l2_fill_pix_format(pixfmt, &format.format);
> +
> + switch (pixfmt->field) {
> + case V4L2_FIELD_ANY:
> + case V4L2_FIELD_NONE:
> + break;
> + default:
> + pixfmt->field = V4L2_FIELD_NONE;
> + }
Drop the switch and just always set field to FIELD_NONE.
> +
> + pixfmt->bytesperline = pixfmt->width * isc_fmt->bpp;
> + pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
> +
> + if (current_fmt)
> + *current_fmt = isc_fmt;
> +
> + if (code)
> + *code = mbus_code;
> +
> + return 0;
> +}
<snip>
> +static int isc_set_default_fmt(struct isc_device *isc)
> +{
> + u32 index = isc->num_user_formats - 1;
> + struct v4l2_format f = {
> + .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
> + .fmt.pix = {
> + .width = VGA_WIDTH,
> + .height = VGA_HEIGHT,
> + .field = V4L2_FIELD_ANY,
Use FIELD_NONE instead of ANY.
> + .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;
> +
> + ret = v4l2_subdev_call(sd, core, s_power, 1);
Only do this if this is the first open...
> + if (ret < 0 && ret != -ENOIOCTLCMD)
> + goto unlock;
> +
> + ret = isc_set_default_fmt(isc);
> + if (ret)
> + goto unlock;
> +
> +unlock:
> + mutex_unlock(&isc->lock);
> + return ret;
> +}
> +
> +static int isc_release(struct file *file)
> +{
> + struct isc_device *isc = video_drvdata(file);
> + struct v4l2_subdev *sd = isc->current_subdev->sd;
> + int ret;
> +
> + mutex_lock(&isc->lock);
> +
> + ret = _vb2_fop_release(file, NULL);
> +
> + v4l2_subdev_call(sd, core, s_power, 0);
...and only do this on the last release.
Otherwise a simple 'v4l2-ctl -D' call would turn off the power when it closes its
filehandle, even if another process is streaming.
See drivers/media/platform/am437x/am437x-vpfe.c for a good example on how to handle
this (search for v4l2_fh_is_singular).
> +
> + mutex_unlock(&isc->lock);
> +
> + return ret;
> +}
> +
> +static const struct v4l2_file_operations isc_fops = {
> + .owner = THIS_MODULE,
> + .open = isc_open,
> + .release = isc_release,
> + .unlocked_ioctl = video_ioctl2,
> + .read = vb2_fop_read,
> + .mmap = vb2_fop_mmap,
> + .poll = vb2_fop_poll,
> +};
> +
> +static irqreturn_t isc_interrupt(int irq, void *dev_id)
> +{
> + struct isc_device *isc = (struct isc_device *)dev_id;
> + struct regmap *regmap = isc->regmap;
> + u32 isc_intsr, isc_intmask, pending;
> + irqreturn_t ret = IRQ_NONE;
> +
> + spin_lock(&isc->dma_queue_lock);
> +
> + regmap_read(regmap, ISC_INTSR, &isc_intsr);
> + regmap_read(regmap, ISC_INTMASK, &isc_intmask);
> +
> + pending = isc_intsr & isc_intmask;
> +
> + if (likely(pending & ISC_INT_DDONE)) {
> + if (isc->cur_frm) {
> + struct vb2_v4l2_buffer *vbuf = &isc->cur_frm->vb;
> + struct vb2_buffer *vb = &vbuf->vb2_buf;
> +
> + vb->timestamp = ktime_get_ns();
> + vbuf->sequence = isc->sequence++;
> + vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> + isc->cur_frm = NULL;
> + }
> +
> + if (!list_empty(&isc->dma_queue) && !isc->stop) {
> + isc->cur_frm = list_first_entry(&isc->dma_queue,
> + struct isc_buffer, list);
> + list_del(&isc->cur_frm->list);
> +
> + isc_start_dma(regmap, isc->cur_frm,
> + isc->current_fmt->reg_dctrl_dview);
> + }
> +
> + if (isc->stop)
> + complete(&isc->comp);
> +
> + ret = IRQ_HANDLED;
> + }
> +
> + spin_unlock(&isc->dma_queue_lock);
> +
> + return ret;
> +}
> +
> +static int isc_async_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct isc_device *isc = container_of(notifier->v4l2_dev,
> + struct isc_device, v4l2_dev);
> + struct isc_subdev_entity *subdev_entity =
> + container_of(notifier, struct isc_subdev_entity, notifier);
> +
> + if (video_is_registered(&isc->video_dev)) {
> + v4l2_err(&isc->v4l2_dev, "only supports one sub-device.\n");
> + return -EBUSY;
> + }
> +
> + subdev_entity->sd = subdev;
> +
> + return 0;
> +}
> +
> +static void isc_async_unbind(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct isc_device *isc = container_of(notifier->v4l2_dev,
> + struct isc_device, v4l2_dev);
> +
> + video_unregister_device(&isc->video_dev);
> + if (isc->current_subdev->config)
> + v4l2_subdev_free_pad_config(isc->current_subdev->config);
> +}
> +
> +static struct isc_format *find_format_by_code(unsigned int code, int *index)
> +{
> + struct isc_format *fmt = &isc_formats[0];
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(isc_formats); i++) {
> + if (fmt->sd_mbus_code == code) {
> + *index = i;
> + return fmt;
> + }
> +
> + fmt++;
> + }
> +
> + return NULL;
> +}
> +
> +static int isc_formats_init(struct isc_device *isc)
> +{
> + struct isc_format *fmt;
> + struct v4l2_subdev *subdev = isc->current_subdev->sd;
> + int num_fmts, i, j;
> + struct v4l2_subdev_mbus_code_enum mbus_code = {
> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + };
> +
> + fmt = &isc_formats[0];
> + for (i = 0; i < ARRAY_SIZE(isc_formats); i++) {
> + fmt->isc_support = false;
> + fmt->sd_support = false;
> +
> + fmt++;
> + }
> +
> + while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
> + NULL, &mbus_code)) {
> + mbus_code.index++;
> + fmt = find_format_by_code(mbus_code.code, &i);
> + if (!fmt)
> + continue;
> +
> + fmt->sd_support = true;
> +
> + if (i <= RAW_FMT_INDEX_END) {
> + for (j = ISC_FMT_INDEX_START;
> + j <= ISC_FMT_INDEX_END; j++) {
> + isc_formats[j].isc_support = true;
> + isc_formats[j].isc_mbus_code = mbus_code.code;
> + isc_formats[j].reg_isc_bps = fmt->reg_sd_bps;
> + isc_formats[j].reg_bay_cfg = fmt->reg_bay_cfg;
> + }
> + }
> + }
> +
> + fmt = &isc_formats[0];
> + for (i = 0, num_fmts = 0; i < ARRAY_SIZE(isc_formats); i++) {
> + if (fmt->isc_support || fmt->sd_support)
> + num_fmts++;
> +
> + fmt++;
> + }
> +
> + if (!num_fmts)
> + return -ENXIO;
> +
> + isc->num_user_formats = num_fmts;
> + isc->user_formats = devm_kcalloc(isc->dev,
> + num_fmts, sizeof(struct isc_format *),
> + GFP_KERNEL);
> + if (!isc->user_formats) {
> + v4l2_err(&isc->v4l2_dev, "could not allocate memory\n");
> + return -ENOMEM;
> + }
> +
> + fmt = &isc_formats[0];
> + for (i = 0, j = 0; i < ARRAY_SIZE(isc_formats); i++) {
> + if (fmt->isc_support || fmt->sd_support)
> + isc->user_formats[j++] = fmt;
> +
> + fmt++;
> + }
> +
> + return 0;
> +}
> +
> +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;
Is there any reason VB2_DMABUF and VB2_READ are missing here?
> + 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;
> + }
> +
> + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
