2011-06-03 10:31:28

by Josh Wu

[permalink] [raw]
Subject: [PATCH v3] [media] at91: add Atmel Image Sensor Interface (ISI) support

This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
- Using soc-camera framework with videobuf2 dma-contig allocator
- Supporting video streaming of YUV packed format
- Tested on AT91SAM9M10G45-EK with OV2640

Signed-off-by: Josh Wu <[email protected]>
---
base on branch staging/for_v3.0
Modified in V3:
refined the interrupt handling code.
added a list to manage the allocated descriptors.
removed redundant code in isi_camera_set_bus_param().
return error when platform data is not provided.

drivers/media/video/Kconfig | 8 +
drivers/media/video/Makefile | 1 +
drivers/media/video/atmel-isi.c | 1074 +++++++++++++++++++++++++++++++++++++++
include/media/atmel-isi.h | 119 +++++
4 files changed, 1202 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/atmel-isi.c
create mode 100644 include/media/atmel-isi.h

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index bb53de7..93d1098 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -952,6 +952,14 @@ config VIDEO_SAMSUNG_S5P_FIMC
To compile this driver as a module, choose M here: the
module will be called s5p-fimc.

+config VIDEO_ATMEL_ISI
+ tristate "ATMEL Image Sensor Interface (ISI) support"
+ depends on VIDEO_DEV && SOC_CAMERA && ARCH_AT91
+ select VIDEOBUF2_DMA_CONTIG
+ ---help---
+ This module makes the ATMEL Image Sensor Interface available
+ as a v4l2 device.
+
config VIDEO_S5P_MIPI_CSIS
tristate "Samsung S5P and EXYNOS4 MIPI CSI receiver driver"
depends on VIDEO_V4L2 && PM_RUNTIME && PLAT_S5P && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index f0fecd6..d9833f4 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_PXA27x) += pxa_camera.o
obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += sh_mobile_csi2.o
obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o
obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o
+obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o

obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/

diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
new file mode 100644
index 0000000..a020bd3
--- /dev/null
+++ b/drivers/media/video/atmel-isi.c
@@ -0,0 +1,1074 @@
+/*
+ * Copyright (c) 2011 Atmel Corporation
+ * Josh Wu, <[email protected]>
+ *
+ * Based on previous work by Lars Haring, <[email protected]>
+ * and Sedji Gaouaou
+ * Based on the bttv driver for Bt848 with respective copyright holders
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <media/atmel-isi.h>
+#include <media/soc_camera.h>
+#include <media/soc_mediabus.h>
+#include <media/videobuf2-dma-contig.h>
+
+#define MAX_BUFFER_NUM 32
+#define MAX_SUPPORT_WIDTH 2048
+#define MAX_SUPPORT_HEIGHT 2048
+#define VID_LIMIT_BYTES (16 * 1024 * 1024)
+#define MIN_FRAME_RATE 15
+#define FRAME_INTERVAL_MILLI_SEC (1000 / MIN_FRAME_RATE)
+
+/* ISI states */
+enum {
+ ISI_STATE_IDLE = 0,
+ ISI_STATE_READY,
+ ISI_STATE_WAIT_SOF,
+};
+
+/* Frame buffer descriptor */
+struct fbd {
+ /* Physical address of the frame buffer */
+ u32 fb_address;
+ /* DMA Control Register(only in HISI2) */
+ u32 dma_ctrl;
+ /* Physical address of the next fbd */
+ u32 next_fbd_address;
+};
+
+static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl)
+{
+ fb_desc->dma_ctrl = ctrl;
+}
+
+struct isi_dma_desc {
+ struct list_head list;
+ struct fbd *p_fbd;
+ u32 fbd_phys;
+};
+
+/* Frame buffer data */
+struct frame_buffer {
+ struct vb2_buffer vb;
+ struct fbd *p_fb_desc;
+ u32 fb_desc_phys;
+ struct list_head list;
+};
+
+struct atmel_isi {
+ /* Protects the access of variables shared with the ISR */
+ spinlock_t lock;
+ void __iomem *regs;
+
+ int sequence;
+ /* State of the ISI module in capturing mode */
+ int state;
+
+ /* Wait queue for waiting for SOF */
+ wait_queue_head_t vsync_wq;
+
+ struct vb2_alloc_ctx *alloc_ctx;
+
+ /* Allocate descriptors for dma buffer use */
+ struct fbd *p_fb_descriptors;
+ u32 fb_descriptors_phys;
+ struct list_head dma_desc_head;
+ struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
+
+ struct completion complete;
+ struct clk *pclk;
+ unsigned int irq;
+
+ struct isi_platform_data *pdata;
+
+ struct list_head video_buffer_list;
+ struct frame_buffer *active;
+
+ struct soc_camera_device *icd;
+ struct soc_camera_host soc_host;
+};
+
+static void isi_writel(struct atmel_isi *isi, u32 reg, u32 val)
+{
+ writel(val, isi->regs + reg);
+}
+static u32 isi_readl(struct atmel_isi *isi, u32 reg)
+{
+ return readl(isi->regs + reg);
+}
+
+static int configure_geometry(struct atmel_isi *isi, u32 width,
+ u32 height, enum v4l2_mbus_pixelcode code)
+{
+ u32 cfg2, cr;
+
+ switch (code) {
+ /* YUV, including grey */
+ case V4L2_MBUS_FMT_Y8_1X8:
+ cr = ISI_CFG2_GRAYSCALE;
+ break;
+ case V4L2_MBUS_FMT_UYVY8_2X8:
+ cr = ISI_CFG2_YCC_SWAP_MODE_3;
+ break;
+ case V4L2_MBUS_FMT_VYUY8_2X8:
+ cr = ISI_CFG2_YCC_SWAP_MODE_2;
+ break;
+ case V4L2_MBUS_FMT_YUYV8_2X8:
+ cr = ISI_CFG2_YCC_SWAP_MODE_1;
+ break;
+ case V4L2_MBUS_FMT_YVYU8_2X8:
+ cr = ISI_CFG2_YCC_SWAP_DEFAULT;
+ break;
+ /* RGB, TODO */
+ default:
+ return -EINVAL;
+ }
+
+ isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
+
+ cfg2 = isi_readl(isi, ISI_CFG2);
+ cfg2 |= cr;
+ /* Set width */
+ cfg2 &= ~(ISI_CFG2_IM_HSIZE_MASK);
+ cfg2 |= ((width - 1) << ISI_CFG2_IM_HSIZE_OFFSET) &
+ ISI_CFG2_IM_HSIZE_MASK;
+ /* Set height */
+ cfg2 &= ~(ISI_CFG2_IM_VSIZE_MASK);
+ cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
+ & ISI_CFG2_IM_VSIZE_MASK;
+ isi_writel(isi, ISI_CFG2, cfg2);
+
+ return 0;
+}
+
+static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
+{
+ if (isi->active) {
+ struct vb2_buffer *vb = &isi->active->vb;
+ struct frame_buffer *buf = isi->active;
+
+ list_del_init(&buf->list);
+ do_gettimeofday(&vb->v4l2_buf.timestamp);
+ vb->v4l2_buf.sequence = isi->sequence++;
+ vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
+ }
+
+ if (list_empty(&isi->video_buffer_list)) {
+ isi->active = NULL;
+ } else {
+ /* start next dma frame. */
+ isi->active = list_entry(isi->video_buffer_list.next,
+ struct frame_buffer, list);
+ isi_writel(isi, ISI_DMA_C_DSCR, isi->active->fb_desc_phys);
+ isi_writel(isi, ISI_DMA_C_CTRL,
+ ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
+ isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
+ }
+ return IRQ_HANDLED;
+}
+
+/* ISI interrupt service routine */
+static irqreturn_t isi_interrupt(int irq, void *dev_id)
+{
+ struct atmel_isi *isi = dev_id;
+ u32 status, mask, pending;
+ irqreturn_t ret = IRQ_NONE;
+
+ spin_lock(&isi->lock);
+
+ status = isi_readl(isi, ISI_STATUS);
+ mask = isi_readl(isi, ISI_INTMASK);
+ pending = status & mask;
+
+ if (pending & ISI_CTRL_SRST) {
+ complete(&isi->complete);
+ isi_writel(isi, ISI_INTDIS, ISI_CTRL_SRST);
+ ret = IRQ_HANDLED;
+ } else if (pending & ISI_CTRL_DIS) {
+ complete(&isi->complete);
+ isi_writel(isi, ISI_INTDIS, ISI_CTRL_DIS);
+ ret = IRQ_HANDLED;
+ } else {
+ if ((pending & ISI_SR_VSYNC) &&
+ (isi->state == ISI_STATE_IDLE)) {
+ isi->state = ISI_STATE_READY;
+ wake_up_interruptible(&isi->vsync_wq);
+ ret = IRQ_HANDLED;
+ }
+ if (likely(pending & ISI_SR_CXFR_DONE))
+ ret = atmel_isi_handle_streaming(isi);
+ }
+
+ spin_unlock(&isi->lock);
+ return ret;
+}
+
+#define WAIT_ISI_RESET 1
+#define WAIT_ISI_DISABLE 0
+static int atmel_isi_wait_status(struct atmel_isi *isi, int wait_reset)
+{
+ unsigned long timeout;
+ /*
+ * The reset or disable will only succeed if we have a
+ * pixel clock from the camera.
+ */
+ init_completion(&isi->complete);
+
+ if (wait_reset) {
+ isi_writel(isi, ISI_INTEN, ISI_CTRL_SRST);
+ isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
+ } else {
+ isi_writel(isi, ISI_INTEN, ISI_CTRL_DIS);
+ isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
+ }
+
+ timeout = wait_for_completion_timeout(&isi->complete,
+ msecs_to_jiffies(100));
+ if (timeout == 0)
+ return -ETIMEDOUT;
+
+ return 0;
+}
+
+/* ------------------------------------------------------------------
+ Videobuf operations
+ ------------------------------------------------------------------*/
+static int queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
+ unsigned int *nplanes, unsigned long sizes[],
+ void *alloc_ctxs[])
+{
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ unsigned long size;
+ int ret, bytes_per_line;
+
+ /* Reset ISI */
+ ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
+ if (ret < 0) {
+ dev_err(icd->dev.parent, "Reset ISI timed out\n");
+ return ret;
+ }
+ /* Disable all interrupts */
+ isi_writel(isi, ISI_INTDIS, ~0UL);
+
+ bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
+ icd->current_fmt->host_fmt);
+
+ if (bytes_per_line < 0)
+ return bytes_per_line;
+
+ size = bytes_per_line * icd->user_height;
+
+ if (*nbuffers == 0)
+ *nbuffers = MAX_BUFFER_NUM;
+ else if (*nbuffers > MAX_BUFFER_NUM)
+ *nbuffers = MAX_BUFFER_NUM;
+
+ if (size * *nbuffers > VID_LIMIT_BYTES)
+ *nbuffers = VID_LIMIT_BYTES / size;
+
+ *nplanes = 1;
+ sizes[0] = size;
+ alloc_ctxs[0] = isi->alloc_ctx;
+
+ isi->sequence = 0;
+ isi->active = NULL;
+
+ dev_dbg(icd->dev.parent, "%s, count=%d, size=%ld\n", __func__,
+ *nbuffers, size);
+
+ return 0;
+}
+
+static int buffer_init(struct vb2_buffer *vb)
+{
+ struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
+
+ buf->p_fb_desc = NULL;
+ memset(&buf->fb_desc_phys, 0, sizeof(buf->fb_desc_phys));
+ INIT_LIST_HEAD(&buf->list);
+
+ return 0;
+}
+
+static int buffer_prepare(struct vb2_buffer *vb)
+{
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
+ struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ unsigned long size;
+ struct isi_dma_desc *desc;
+ int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
+ icd->current_fmt->host_fmt);
+
+ if (bytes_per_line < 0)
+ return bytes_per_line;
+
+ size = bytes_per_line * icd->user_height;
+
+ if (vb2_plane_size(vb, 0) < size) {
+ dev_err(icd->dev.parent, "%s data will not fit into plane (%lu < %lu)\n",
+ __func__, vb2_plane_size(vb, 0), size);
+ return -EINVAL;
+ }
+
+ vb2_set_plane_payload(&buf->vb, 0, size);
+
+ if (!buf->p_fb_desc) {
+ if (list_empty(&isi->dma_desc_head)) {
+ dev_err(icd->dev.parent, "Not enough dma descriptors.\n");
+ return -EINVAL;
+ } else {
+ /* Get an available descriptor */
+ desc = list_entry(isi->dma_desc_head.next,
+ struct isi_dma_desc, list);
+ buf->p_fb_desc = desc->p_fbd;
+ buf->fb_desc_phys = desc->fbd_phys;
+
+ /* Initialize the dma descriptor */
+ buf->p_fb_desc->fb_address =
+ vb2_dma_contig_plane_paddr(vb, 0);
+ buf->p_fb_desc->next_fbd_address = 0;
+ set_dma_ctrl(buf->p_fb_desc, ISI_DMA_CTRL_WB);
+
+ /* Delete the descriptor since now it is used */
+ list_del_init(&desc->list);
+ }
+ }
+ return 0;
+}
+
+static void buffer_cleanup(struct vb2_buffer *vb)
+{
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
+ unsigned long flags;
+ int i;
+
+ spin_lock_irqsave(&isi->lock, flags);
+ if (buf->p_fb_desc) {
+ /* This descriptor is available now and we add to head list */
+ for (i = 0; i < MAX_BUFFER_NUM; i++)
+ if (isi->dma_desc[i].p_fbd == buf->p_fb_desc) {
+ list_add(&isi->dma_desc[i].list,
+ &isi->dma_desc_head);
+ break;
+ }
+
+ buf->p_fb_desc = NULL;
+ memset(&buf->fb_desc_phys, 0, sizeof(buf->fb_desc_phys));
+ }
+ spin_unlock_irqrestore(&isi->lock, flags);
+}
+
+static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
+{
+ u32 ctrl, cfg1;
+
+ cfg1 = isi_readl(isi, ISI_CFG1);
+ /* Enable irq: cxfr for the codec path, pxfr for the preview path */
+ isi_writel(isi, ISI_INTEN,
+ ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
+
+ /* Check if already in a frame */
+ if (isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) {
+ dev_err(isi->icd->dev.parent, "Already in frame handling.\n");
+ return;
+ }
+
+ isi_writel(isi, ISI_DMA_C_DSCR, buffer->fb_desc_phys);
+ isi_writel(isi, ISI_DMA_C_CTRL, ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
+ isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
+
+ /* Enable linked list */
+ cfg1 |= isi->pdata->frate | ISI_CFG1_DISCR;
+
+ /* Enable codec path and ISI */
+ ctrl = ISI_CTRL_CDC | ISI_CTRL_EN;
+ isi_writel(isi, ISI_CTRL, ctrl);
+ isi_writel(isi, ISI_CFG1, cfg1);
+}
+
+static void buffer_queue(struct vb2_buffer *vb)
+{
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
+ unsigned long flags = 0;
+
+ spin_lock_irqsave(&isi->lock, flags);
+ list_add_tail(&buf->list, &isi->video_buffer_list);
+
+ if (isi->active == NULL) {
+ isi->active = buf;
+ start_dma(isi, buf);
+ }
+ spin_unlock_irqrestore(&isi->lock, flags);
+}
+
+static int start_streaming(struct vb2_queue *vq)
+{
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+
+ u32 sr = 0;
+ int ret;
+
+ spin_lock_irq(&isi->lock);
+ isi->state = ISI_STATE_IDLE;
+
+ /* Clear any pending SOF interrupt */
+ sr = isi_readl(isi, ISI_STATUS);
+ /* Enable VSYNC interrupt for SOF */
+ isi_writel(isi, ISI_INTEN, ISI_SR_VSYNC);
+ isi_writel(isi, ISI_CTRL, ISI_CTRL_EN);
+
+ spin_unlock_irq(&isi->lock);
+ dev_dbg(icd->dev.parent, "Waiting for SOF\n");
+ ret = wait_event_interruptible(isi->vsync_wq,
+ isi->state != ISI_STATE_IDLE);
+ if (ret)
+ return ret;
+
+ if (isi->state != ISI_STATE_READY)
+ return -EIO;
+
+ spin_lock_irq(&isi->lock);
+ isi->state = ISI_STATE_WAIT_SOF;
+ isi_writel(isi, ISI_INTDIS, ISI_SR_VSYNC);
+ spin_unlock_irq(&isi->lock);
+
+ return 0;
+}
+
+/* abort streaming and wait for last buffer */
+static int stop_streaming(struct vb2_queue *vq)
+{
+ struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ struct frame_buffer *buf, *node;
+ int ret = 0;
+ unsigned long timeout;
+
+ spin_lock_irq(&isi->lock);
+ isi->active = NULL;
+ /* Release all active buffers */
+ list_for_each_entry_safe(buf, node, &isi->video_buffer_list, list) {
+ list_del_init(&buf->list);
+ vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
+ }
+ spin_unlock_irq(&isi->lock);
+
+ timeout = jiffies + FRAME_INTERVAL_MILLI_SEC * HZ;
+ /* Wait until the end of the current frame. */
+ while ((isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) &&
+ time_before(jiffies, timeout))
+ msleep(1);
+
+ if (time_after(jiffies, timeout)) {
+ dev_err(icd->dev.parent,
+ "Timeout waiting for finishing codec request\n");
+ return -ETIMEDOUT;
+ }
+
+ /* Disable interrupts */
+ isi_writel(isi, ISI_INTDIS,
+ ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
+
+ /* Disable ISI and wait for it is done */
+ ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
+ if (ret < 0)
+ dev_err(icd->dev.parent, "Disable ISI timed out\n");
+
+ return ret;
+}
+
+static struct vb2_ops isi_video_qops = {
+ .queue_setup = queue_setup,
+ .buf_init = buffer_init,
+ .buf_prepare = buffer_prepare,
+ .buf_cleanup = buffer_cleanup,
+ .buf_queue = buffer_queue,
+ .start_streaming = start_streaming,
+ .stop_streaming = stop_streaming,
+ .wait_prepare = soc_camera_unlock,
+ .wait_finish = soc_camera_lock,
+};
+
+/* ------------------------------------------------------------------
+ SOC camera operations for the device
+ ------------------------------------------------------------------*/
+static int isi_camera_init_videobuf(struct vb2_queue *q,
+ struct soc_camera_device *icd)
+{
+ q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ q->io_modes = VB2_MMAP;
+ q->drv_priv = icd;
+ q->buf_struct_size = sizeof(struct frame_buffer);
+ q->ops = &isi_video_qops;
+ q->mem_ops = &vb2_dma_contig_memops;
+
+ return vb2_queue_init(q);
+}
+
+static int isi_camera_set_fmt(struct soc_camera_device *icd,
+ struct v4l2_format *f)
+{
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+ const struct soc_camera_format_xlate *xlate;
+ struct v4l2_pix_format *pix = &f->fmt.pix;
+ struct v4l2_mbus_framefmt mf;
+ int ret;
+
+ xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
+ if (!xlate) {
+ dev_warn(icd->dev.parent, "Format %x not found\n",
+ pix->pixelformat);
+ return -EINVAL;
+ }
+
+ dev_dbg(icd->dev.parent, "Plan to set format %dx%d\n",
+ pix->width, pix->height);
+
+ mf.width = pix->width;
+ mf.height = pix->height;
+ mf.field = pix->field;
+ mf.colorspace = pix->colorspace;
+ mf.code = xlate->code;
+
+ ret = v4l2_subdev_call(sd, video, s_mbus_fmt, &mf);
+ if (ret < 0)
+ return ret;
+
+ if (mf.code != xlate->code)
+ return -EINVAL;
+
+ ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
+ if (ret < 0)
+ return ret;
+
+ pix->width = mf.width;
+ pix->height = mf.height;
+ pix->field = mf.field;
+ pix->colorspace = mf.colorspace;
+ icd->current_fmt = xlate;
+
+ dev_dbg(icd->dev.parent, "Finally set format %dx%d\n",
+ pix->width, pix->height);
+
+ return ret;
+}
+
+static int isi_camera_try_fmt(struct soc_camera_device *icd,
+ struct v4l2_format *f)
+{
+ struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+ const struct soc_camera_format_xlate *xlate;
+ struct v4l2_pix_format *pix = &f->fmt.pix;
+ struct v4l2_mbus_framefmt mf;
+ u32 pixfmt = pix->pixelformat;
+ int ret;
+
+ xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
+ if (pixfmt && !xlate) {
+ dev_warn(icd->dev.parent, "Format %x not found\n", pixfmt);
+ return -EINVAL;
+ }
+
+ /* limit to Atmel ISI hardware capabilities */
+ if (pix->height > MAX_SUPPORT_HEIGHT)
+ pix->height = MAX_SUPPORT_HEIGHT;
+ if (pix->width > MAX_SUPPORT_WIDTH)
+ pix->width = MAX_SUPPORT_WIDTH;
+
+ /* limit to sensor capabilities */
+ mf.width = pix->width;
+ mf.height = pix->height;
+ mf.field = pix->field;
+ mf.colorspace = pix->colorspace;
+ mf.code = xlate->code;
+
+ ret = v4l2_subdev_call(sd, video, try_mbus_fmt, &mf);
+ if (ret < 0)
+ return ret;
+
+ pix->width = mf.width;
+ pix->height = mf.height;
+ pix->colorspace = mf.colorspace;
+
+ switch (mf.field) {
+ case V4L2_FIELD_ANY:
+ pix->field = V4L2_FIELD_NONE;
+ break;
+ case V4L2_FIELD_NONE:
+ break;
+ default:
+ dev_err(icd->dev.parent, "Field type %d unsupported.\n",
+ mf.field);
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static const struct soc_mbus_pixelfmt isi_camera_formats[] = {
+ {
+ .fourcc = V4L2_PIX_FMT_YUYV,
+ .name = "Packed YUV422 16 bit",
+ .bits_per_sample = 8,
+ .packing = SOC_MBUS_PACKING_2X8_PADHI,
+ .order = SOC_MBUS_ORDER_LE,
+ },
+};
+
+/* This will be corrected as we get more formats */
+static bool isi_camera_packing_supported(const struct soc_mbus_pixelfmt *fmt)
+{
+ return fmt->packing == SOC_MBUS_PACKING_NONE ||
+ (fmt->bits_per_sample == 8 &&
+ fmt->packing == SOC_MBUS_PACKING_2X8_PADHI) ||
+ (fmt->bits_per_sample > 8 &&
+ fmt->packing == SOC_MBUS_PACKING_EXTEND16);
+}
+
+static unsigned long make_bus_param(struct atmel_isi *isi)
+{
+ unsigned long flags;
+ /*
+ * Platform specified synchronization and pixel clock polarities are
+ * only a recommendation and are only used during probing. Atmel ISI
+ * camera interface only works in master mode, i.e., uses HSYNC and
+ * VSYNC signals from the sensor
+ */
+ flags = SOCAM_MASTER |
+ SOCAM_HSYNC_ACTIVE_HIGH |
+ SOCAM_HSYNC_ACTIVE_LOW |
+ SOCAM_VSYNC_ACTIVE_HIGH |
+ SOCAM_VSYNC_ACTIVE_LOW |
+ SOCAM_PCLK_SAMPLE_RISING |
+ SOCAM_PCLK_SAMPLE_FALLING |
+ SOCAM_DATA_ACTIVE_HIGH;
+
+ if (isi->pdata->data_width_flags & ISI_DATAWIDTH_10)
+ flags |= SOCAM_DATAWIDTH_10;
+
+ if (isi->pdata->data_width_flags & ISI_DATAWIDTH_8)
+ flags |= SOCAM_DATAWIDTH_8;
+
+ if (flags & SOCAM_DATAWIDTH_MASK)
+ return flags;
+
+ return 0;
+}
+
+static int isi_camera_try_bus_param(struct soc_camera_device *icd,
+ unsigned char buswidth)
+{
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ unsigned long camera_flags;
+ int ret;
+
+ camera_flags = icd->ops->query_bus_param(icd);
+ ret = soc_camera_bus_param_compatible(camera_flags,
+ make_bus_param(isi));
+ if (!ret)
+ return -EINVAL;
+ return 0;
+}
+
+
+static int isi_camera_get_formats(struct soc_camera_device *icd,
+ unsigned int idx,
+ struct soc_camera_format_xlate *xlate)
+{
+ struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+ int formats = 0, ret;
+ /* sensor format */
+ enum v4l2_mbus_pixelcode code;
+ /* soc camera host format */
+ const struct soc_mbus_pixelfmt *fmt;
+
+ ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, idx, &code);
+ if (ret < 0)
+ /* No more formats */
+ return 0;
+
+ fmt = soc_mbus_get_fmtdesc(code);
+ if (!fmt) {
+ dev_err(icd->dev.parent,
+ "Invalid format code #%u: %d\n", idx, code);
+ return 0;
+ }
+
+ /* This also checks support for the requested bits-per-sample */
+ ret = isi_camera_try_bus_param(icd, fmt->bits_per_sample);
+ if (ret < 0) {
+ dev_err(icd->dev.parent,
+ "Fail to try the bus parameters.\n");
+ return 0;
+ }
+
+ switch (code) {
+ case V4L2_MBUS_FMT_UYVY8_2X8:
+ case V4L2_MBUS_FMT_VYUY8_2X8:
+ case V4L2_MBUS_FMT_YUYV8_2X8:
+ case V4L2_MBUS_FMT_YVYU8_2X8:
+ formats++;
+ if (xlate) {
+ xlate->host_fmt = &isi_camera_formats[0];
+ xlate->code = code;
+ xlate++;
+ dev_dbg(icd->dev.parent, "Providing format %s using code %d\n",
+ isi_camera_formats[0].name, code);
+ }
+ break;
+ default:
+ if (!isi_camera_packing_supported(fmt))
+ return 0;
+ if (xlate)
+ dev_dbg(icd->dev.parent,
+ "Providing format %s in pass-through mode\n",
+ fmt->name);
+ }
+
+ /* Generic pass-through */
+ formats++;
+ if (xlate) {
+ xlate->host_fmt = fmt;
+ xlate->code = code;
+ xlate++;
+ }
+
+ return formats;
+}
+
+/* Called with .video_lock held */
+static int isi_camera_add_device(struct soc_camera_device *icd)
+{
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ int ret;
+
+ if (isi->icd)
+ return -EBUSY;
+
+ ret = clk_enable(isi->pclk);
+ if (ret)
+ return ret;
+
+ isi->icd = icd;
+ dev_dbg(icd->dev.parent, "Atmel ISI Camera driver attached to camera %d\n",
+ icd->devnum);
+ return 0;
+}
+/* Called with .video_lock held */
+static void isi_camera_remove_device(struct soc_camera_device *icd)
+{
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+
+ BUG_ON(icd != isi->icd);
+
+ clk_disable(isi->pclk);
+ isi->icd = NULL;
+
+ dev_dbg(icd->dev.parent, "Atmel ISI Camera driver detached from camera %d\n",
+ icd->devnum);
+}
+
+static unsigned int isi_camera_poll(struct file *file, poll_table *pt)
+{
+ struct soc_camera_device *icd = file->private_data;
+
+ return vb2_poll(&icd->vb2_vidq, file, pt);
+}
+
+static int isi_camera_querycap(struct soc_camera_host *ici,
+ struct v4l2_capability *cap)
+{
+ strcpy(cap->driver, "atmel-isi");
+ strcpy(cap->card, "Atmel Image Sensor Interface");
+ cap->capabilities = (V4L2_CAP_VIDEO_CAPTURE |
+ V4L2_CAP_STREAMING);
+ return 0;
+}
+
+static int isi_camera_set_bus_param(struct soc_camera_device *icd, u32 pixfmt)
+{
+ struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+ struct atmel_isi *isi = ici->priv;
+ unsigned long bus_flags, camera_flags, common_flags;
+ int ret;
+ u32 cfg1 = 0;
+
+ camera_flags = icd->ops->query_bus_param(icd);
+
+ bus_flags = make_bus_param(isi);
+ common_flags = soc_camera_bus_param_compatible(camera_flags, bus_flags);
+ dev_dbg(icd->dev.parent, "Flags cam: 0x%lx host: 0x%lx common: 0x%lx\n",
+ camera_flags, bus_flags, common_flags);
+ if (!common_flags)
+ return -EINVAL;
+
+ /* Make choises, based on platform preferences */
+ if ((common_flags & SOCAM_HSYNC_ACTIVE_HIGH) &&
+ (common_flags & SOCAM_HSYNC_ACTIVE_LOW)) {
+ if (isi->pdata->hsync_act_low)
+ common_flags &= ~SOCAM_HSYNC_ACTIVE_HIGH;
+ else
+ common_flags &= ~SOCAM_HSYNC_ACTIVE_LOW;
+ }
+
+ if ((common_flags & SOCAM_VSYNC_ACTIVE_HIGH) &&
+ (common_flags & SOCAM_VSYNC_ACTIVE_LOW)) {
+ if (isi->pdata->vsync_act_low)
+ common_flags &= ~SOCAM_VSYNC_ACTIVE_HIGH;
+ else
+ common_flags &= ~SOCAM_VSYNC_ACTIVE_LOW;
+ }
+
+ if ((common_flags & SOCAM_PCLK_SAMPLE_RISING) &&
+ (common_flags & SOCAM_PCLK_SAMPLE_FALLING)) {
+ if (isi->pdata->pclk_act_falling)
+ common_flags &= ~SOCAM_PCLK_SAMPLE_RISING;
+ else
+ common_flags &= ~SOCAM_PCLK_SAMPLE_FALLING;
+ }
+
+ ret = icd->ops->set_bus_param(icd, common_flags);
+ if (ret < 0) {
+ dev_dbg(icd->dev.parent, "Camera set_bus_param(%lx) returned %d\n",
+ common_flags, ret);
+ return ret;
+ }
+
+ /* set bus param for ISI */
+ if (common_flags & SOCAM_HSYNC_ACTIVE_LOW)
+ cfg1 |= ISI_CFG1_HSYNC_POL_ACTIVE_LOW;
+ if (common_flags & SOCAM_VSYNC_ACTIVE_LOW)
+ cfg1 |= ISI_CFG1_VSYNC_POL_ACTIVE_LOW;
+ if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
+ cfg1 |= ISI_CFG1_PIXCLK_POL_ACTIVE_FALLING;
+
+ if (isi->pdata->has_emb_sync)
+ cfg1 |= ISI_CFG1_EMB_SYNC;
+ if (isi->pdata->isi_full_mode)
+ cfg1 |= ISI_CFG1_FULL_MODE;
+
+ isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
+ isi_writel(isi, ISI_CFG1, cfg1);
+
+ return 0;
+}
+
+static struct soc_camera_host_ops isi_soc_camera_host_ops = {
+ .owner = THIS_MODULE,
+ .add = isi_camera_add_device,
+ .remove = isi_camera_remove_device,
+ .set_fmt = isi_camera_set_fmt,
+ .try_fmt = isi_camera_try_fmt,
+ .get_formats = isi_camera_get_formats,
+ .init_videobuf2 = isi_camera_init_videobuf,
+ .poll = isi_camera_poll,
+ .querycap = isi_camera_querycap,
+ .set_bus_param = isi_camera_set_bus_param,
+};
+
+/* -----------------------------------------------------------------------*/
+static int __devexit atmel_isi_remove(struct platform_device *pdev)
+{
+ struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
+ struct atmel_isi *isi = container_of(soc_host,
+ struct atmel_isi, soc_host);
+
+ soc_camera_host_unregister(soc_host);
+ vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
+ dma_free_coherent(&pdev->dev,
+ sizeof(struct fbd) * MAX_BUFFER_NUM,
+ isi->p_fb_descriptors,
+ isi->fb_descriptors_phys);
+
+ free_irq(isi->irq, isi);
+ iounmap(isi->regs);
+ clk_put(isi->pclk);
+ kfree(isi);
+
+ return 0;
+}
+
+static int __devinit atmel_isi_probe(struct platform_device *pdev)
+{
+ unsigned int irq;
+ struct atmel_isi *isi;
+ struct clk *pclk;
+ struct resource *regs;
+ int ret, i;
+ struct device *dev = &pdev->dev;
+ struct soc_camera_host *soc_host;
+
+ regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!regs)
+ return -ENXIO;
+
+ pclk = clk_get(&pdev->dev, "isi_clk");
+ if (IS_ERR(pclk))
+ return PTR_ERR(pclk);
+
+ isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
+ if (!isi) {
+ ret = -ENOMEM;
+ dev_err(&pdev->dev, "Can't allocate interface!\n");
+ goto err_alloc_isi;
+ }
+
+ isi->pclk = pclk;
+
+ spin_lock_init(&isi->lock);
+ init_waitqueue_head(&isi->vsync_wq);
+
+ isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
+ sizeof(struct fbd) * MAX_BUFFER_NUM,
+ &isi->fb_descriptors_phys,
+ GFP_KERNEL);
+ if (!isi->p_fb_descriptors) {
+ ret = -ENOMEM;
+ dev_err(&pdev->dev, "Can't allocate descriptors!\n");
+ goto err_alloc_descriptors;
+ }
+
+ INIT_LIST_HEAD(&isi->dma_desc_head);
+ for (i = 0; i < MAX_BUFFER_NUM; i++) {
+ isi->dma_desc[i].p_fbd = isi->p_fb_descriptors + i;
+ isi->dma_desc[i].fbd_phys = isi->fb_descriptors_phys +
+ i * sizeof(struct fbd);
+ list_add(&isi->dma_desc[i].list, &isi->dma_desc_head);
+ }
+
+ isi->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
+ if (IS_ERR(isi->alloc_ctx)) {
+ ret = PTR_ERR(isi->alloc_ctx);
+ goto err_alloc_ctx;
+ }
+
+ isi->regs = ioremap(regs->start, resource_size(regs));
+ if (!isi->regs) {
+ ret = -ENOMEM;
+ goto err_ioremap;
+ }
+
+ if (!dev->platform_data) {
+ dev_err(&pdev->dev,
+ "No config available for Atmel ISI\n");
+ ret = -EINVAL;
+ goto err_req_irq;
+ }
+
+ isi->pdata = dev->platform_data;
+ if (isi->pdata->data_width_flags == 0) {
+ dev_err(&pdev->dev,
+ "Invalid data width for Atmel ISI.\n");
+ ret = -EINVAL;
+ goto err_req_irq;
+ }
+
+ isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ ret = irq;
+ goto err_req_irq;
+ }
+
+ ret = request_irq(irq, isi_interrupt, 0, "isi", isi);
+ if (ret) {
+ dev_err(&pdev->dev, "Unable to request irq %d\n", irq);
+ goto err_req_irq;
+ }
+ isi->irq = irq;
+
+ INIT_LIST_HEAD(&isi->video_buffer_list);
+ isi->active = NULL;
+
+ soc_host = &isi->soc_host;
+ soc_host->drv_name = "isi-camera";
+ soc_host->ops = &isi_soc_camera_host_ops;
+ soc_host->priv = isi;
+ soc_host->v4l2_dev.dev = &pdev->dev;
+ soc_host->nr = pdev->id;
+
+ ret = soc_camera_host_register(soc_host);
+ if (ret) {
+ dev_err(&pdev->dev, "Unable to register soc camera host\n");
+ goto err_register_soc_camera_host;
+ }
+ return 0;
+
+err_register_soc_camera_host:
+ free_irq(isi->irq, isi);
+err_req_irq:
+ iounmap(isi->regs);
+err_ioremap:
+ vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
+err_alloc_ctx:
+ dma_free_coherent(&pdev->dev,
+ sizeof(struct fbd) * MAX_BUFFER_NUM,
+ isi->p_fb_descriptors,
+ isi->fb_descriptors_phys);
+err_alloc_descriptors:
+ kfree(isi);
+err_alloc_isi:
+ clk_put(isi->pclk);
+
+ return ret;
+}
+
+static struct platform_driver atmel_isi_driver = {
+ .probe = atmel_isi_probe,
+ .remove = __devexit_p(atmel_isi_remove),
+ .driver = {
+ .name = "atmel_isi",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init atmel_isi_init_module(void)
+{
+ return platform_driver_probe(&atmel_isi_driver, &atmel_isi_probe);
+}
+
+static void __exit atmel_isi_exit(void)
+{
+ platform_driver_unregister(&atmel_isi_driver);
+}
+module_init(atmel_isi_init_module);
+module_exit(atmel_isi_exit);
+
+MODULE_AUTHOR("Josh Wu <[email protected]>");
+MODULE_DESCRIPTION("The V4L2 driver for Atmel Linux");
+MODULE_LICENSE("GPL");
+MODULE_SUPPORTED_DEVICE("video");
diff --git a/include/media/atmel-isi.h b/include/media/atmel-isi.h
new file mode 100644
index 0000000..26cece5
--- /dev/null
+++ b/include/media/atmel-isi.h
@@ -0,0 +1,119 @@
+/*
+ * Register definitions for the Atmel Image Sensor Interface.
+ *
+ * Copyright (C) 2011 Atmel Corporation
+ * Josh Wu, <[email protected]>
+ *
+ * Based on previous work by Lars Haring, <[email protected]>
+ * and Sedji Gaouaou
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef __ATMEL_ISI_H__
+#define __ATMEL_ISI_H__
+
+#include <linux/types.h>
+
+/* ISI_V2 register offsets */
+#define ISI_CFG1 0x0000
+#define ISI_CFG2 0x0004
+#define ISI_PSIZE 0x0008
+#define ISI_PDECF 0x000c
+#define ISI_Y2R_SET0 0x0010
+#define ISI_Y2R_SET1 0x0014
+#define ISI_R2Y_SET0 0x0018
+#define ISI_R2Y_SET1 0x001C
+#define ISI_R2Y_SET2 0x0020
+#define ISI_CTRL 0x0024
+#define ISI_STATUS 0x0028
+#define ISI_INTEN 0x002C
+#define ISI_INTDIS 0x0030
+#define ISI_INTMASK 0x0034
+#define ISI_DMA_CHER 0x0038
+#define ISI_DMA_CHDR 0x003C
+#define ISI_DMA_CHSR 0x0040
+#define ISI_DMA_P_ADDR 0x0044
+#define ISI_DMA_P_CTRL 0x0048
+#define ISI_DMA_P_DSCR 0x004C
+#define ISI_DMA_C_ADDR 0x0050
+#define ISI_DMA_C_CTRL 0x0054
+#define ISI_DMA_C_DSCR 0x0058
+
+/* Bitfields in CFG1 */
+#define ISI_CFG1_HSYNC_POL_ACTIVE_LOW (1 << 2)
+#define ISI_CFG1_VSYNC_POL_ACTIVE_LOW (1 << 3)
+#define ISI_CFG1_PIXCLK_POL_ACTIVE_FALLING (1 << 4)
+#define ISI_CFG1_EMB_SYNC (1 << 6)
+#define ISI_CFG1_CRC_SYNC (1 << 7)
+/* Constants for FRATE(ISI_V2) */
+#define ISI_CFG1_FRATE_CAPTURE_ALL (0 << 8)
+#define ISI_CFG1_FRATE_DIV_2 (1 << 8)
+#define ISI_CFG1_FRATE_DIV_3 (2 << 8)
+#define ISI_CFG1_FRATE_DIV_4 (3 << 8)
+#define ISI_CFG1_FRATE_DIV_5 (4 << 8)
+#define ISI_CFG1_FRATE_DIV_6 (5 << 8)
+#define ISI_CFG1_FRATE_DIV_7 (6 << 8)
+#define ISI_CFG1_FRATE_DIV_8 (7 << 8)
+#define ISI_CFG1_DISCR (1 << 11)
+#define ISI_CFG1_FULL_MODE (1 << 12)
+
+/* Bitfields in CFG2 */
+#define ISI_CFG2_GRAYSCALE (1 << 13)
+/* Constants for YCC_SWAP(ISI_V2) */
+#define ISI_CFG2_YCC_SWAP_DEFAULT (0 << 28)
+#define ISI_CFG2_YCC_SWAP_MODE_1 (1 << 28)
+#define ISI_CFG2_YCC_SWAP_MODE_2 (2 << 28)
+#define ISI_CFG2_YCC_SWAP_MODE_3 (3 << 28)
+#define ISI_CFG2_IM_VSIZE_OFFSET 0
+#define ISI_CFG2_IM_HSIZE_OFFSET 16
+#define ISI_CFG2_IM_VSIZE_MASK (0x7FF << ISI_CFG2_IM_VSIZE_OFFSET)
+#define ISI_CFG2_IM_HSIZE_MASK (0x7FF << ISI_CFG2_IM_HSIZE_OFFSET)
+
+/* Bitfields in CTRL */
+/* Also using in SR(ISI_V2) */
+#define ISI_CTRL_EN (1 << 0)
+#define ISI_CTRL_CDC (1 << 8)
+/* Also using in SR/IER/IDR/IMR(ISI_V2) */
+#define ISI_CTRL_DIS (1 << 1)
+#define ISI_CTRL_SRST (1 << 2)
+
+/* Bitfields in SR */
+#define ISI_SR_SIP (1 << 19)
+/* Also using in SR/IER/IDR/IMR */
+#define ISI_SR_VSYNC (1 << 10)
+#define ISI_SR_PXFR_DONE (1 << 16)
+#define ISI_SR_CXFR_DONE (1 << 17)
+#define ISI_SR_P_OVR (1 << 24)
+#define ISI_SR_C_OVR (1 << 25)
+#define ISI_SR_CRC_ERR (1 << 26)
+#define ISI_SR_FR_OVR (1 << 27)
+
+/* Bitfields in DMA_C_CTRL & in DMA_P_CTRL */
+#define ISI_DMA_CTRL_FETCH (1 << 0)
+#define ISI_DMA_CTRL_WB (1 << 1)
+#define ISI_DMA_CTRL_IEN (1 << 2)
+#define ISI_DMA_CTRL_DONE (1 << 3)
+
+/* Bitfields in DMA_CHSR/CHER/CHDR */
+#define ISI_DMA_CHSR_P_CH (1 << 0)
+#define ISI_DMA_CHSR_C_CH (1 << 1)
+
+/* Definition for isi_platform_data */
+#define ISI_DATAWIDTH_8 0x01
+#define ISI_DATAWIDTH_10 0x02
+
+struct isi_platform_data {
+ u8 has_emb_sync;
+ u8 emb_crc_sync;
+ u8 hsync_act_low;
+ u8 vsync_act_low;
+ u8 pclk_act_falling;
+ u8 isi_full_mode;
+ u32 data_width_flags;
+ /* Using for ISI_CFG1 */
+ u32 frate;
+};
+
+#endif /* __ATMEL_ISI_H__ */
--
1.6.3.3


2011-06-03 14:51:40

by Guennadi Liakhovetski

[permalink] [raw]
Subject: Re: [PATCH v3] [media] at91: add Atmel Image Sensor Interface (ISI) support

On Fri, 3 Jun 2011, Josh Wu wrote:

> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
> - Using soc-camera framework with videobuf2 dma-contig allocator
> - Supporting video streaming of YUV packed format
> - Tested on AT91SAM9M10G45-EK with OV2640
>
> Signed-off-by: Josh Wu <[email protected]>
> ---
> base on branch staging/for_v3.0
> Modified in V3:
> refined the interrupt handling code.
> added a list to manage the allocated descriptors.
> removed redundant code in isi_camera_set_bus_param().
> return error when platform data is not provided.
>
> drivers/media/video/Kconfig | 8 +
> drivers/media/video/Makefile | 1 +
> drivers/media/video/atmel-isi.c | 1074 +++++++++++++++++++++++++++++++++++++++
> include/media/atmel-isi.h | 119 +++++
> 4 files changed, 1202 insertions(+), 0 deletions(-)
> create mode 100644 drivers/media/video/atmel-isi.c
> create mode 100644 include/media/atmel-isi.h
>
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index bb53de7..93d1098 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -952,6 +952,14 @@ config VIDEO_SAMSUNG_S5P_FIMC
> To compile this driver as a module, choose M here: the
> module will be called s5p-fimc.
>
> +config VIDEO_ATMEL_ISI
> + tristate "ATMEL Image Sensor Interface (ISI) support"
> + depends on VIDEO_DEV && SOC_CAMERA && ARCH_AT91
> + select VIDEOBUF2_DMA_CONTIG
> + ---help---
> + This module makes the ATMEL Image Sensor Interface available
> + as a v4l2 device.
> +
> config VIDEO_S5P_MIPI_CSIS
> tristate "Samsung S5P and EXYNOS4 MIPI CSI receiver driver"
> depends on VIDEO_V4L2 && PM_RUNTIME && PLAT_S5P && VIDEO_V4L2_SUBDEV_API
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index f0fecd6..d9833f4 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_PXA27x) += pxa_camera.o
> obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += sh_mobile_csi2.o
> obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o
> obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o
> +obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>
> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/
>
> diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
> new file mode 100644
> index 0000000..a020bd3
> --- /dev/null
> +++ b/drivers/media/video/atmel-isi.c
> @@ -0,0 +1,1074 @@
> +/*
> + * Copyright (c) 2011 Atmel Corporation
> + * Josh Wu, <[email protected]>
> + *
> + * Based on previous work by Lars Haring, <[email protected]>
> + * and Sedji Gaouaou
> + * Based on the bttv driver for Bt848 with respective copyright holders
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <media/atmel-isi.h>
> +#include <media/soc_camera.h>
> +#include <media/soc_mediabus.h>
> +#include <media/videobuf2-dma-contig.h>
> +
> +#define MAX_BUFFER_NUM 32
> +#define MAX_SUPPORT_WIDTH 2048
> +#define MAX_SUPPORT_HEIGHT 2048
> +#define VID_LIMIT_BYTES (16 * 1024 * 1024)
> +#define MIN_FRAME_RATE 15
> +#define FRAME_INTERVAL_MILLI_SEC (1000 / MIN_FRAME_RATE)
> +
> +/* ISI states */
> +enum {
> + ISI_STATE_IDLE = 0,
> + ISI_STATE_READY,
> + ISI_STATE_WAIT_SOF,
> +};
> +
> +/* Frame buffer descriptor */
> +struct fbd {
> + /* Physical address of the frame buffer */
> + u32 fb_address;
> + /* DMA Control Register(only in HISI2) */
> + u32 dma_ctrl;
> + /* Physical address of the next fbd */
> + u32 next_fbd_address;
> +};
> +
> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl)
> +{
> + fb_desc->dma_ctrl = ctrl;
> +}
> +
> +struct isi_dma_desc {
> + struct list_head list;
> + struct fbd *p_fbd;
> + u32 fbd_phys;
> +};
> +
> +/* Frame buffer data */
> +struct frame_buffer {
> + struct vb2_buffer vb;
> + struct fbd *p_fb_desc;
> + u32 fb_desc_phys;

Why don't you replace the above two members with "struct isi_dma_desc *"?
Then you also don't need to scan the DMA descriptor array in
.buf_cleanup().

> + struct list_head list;
> +};
> +
> +struct atmel_isi {
> + /* Protects the access of variables shared with the ISR */
> + spinlock_t lock;
> + void __iomem *regs;
> +
> + int sequence;
> + /* State of the ISI module in capturing mode */
> + int state;
> +
> + /* Wait queue for waiting for SOF */
> + wait_queue_head_t vsync_wq;
> +
> + struct vb2_alloc_ctx *alloc_ctx;
> +
> + /* Allocate descriptors for dma buffer use */
> + struct fbd *p_fb_descriptors;
> + u32 fb_descriptors_phys;
> + struct list_head dma_desc_head;
> + struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
> +
> + struct completion complete;
> + struct clk *pclk;
> + unsigned int irq;
> +
> + struct isi_platform_data *pdata;
> +
> + struct list_head video_buffer_list;
> + struct frame_buffer *active;
> +
> + struct soc_camera_device *icd;
> + struct soc_camera_host soc_host;
> +};
> +
> +static void isi_writel(struct atmel_isi *isi, u32 reg, u32 val)
> +{
> + writel(val, isi->regs + reg);
> +}
> +static u32 isi_readl(struct atmel_isi *isi, u32 reg)
> +{
> + return readl(isi->regs + reg);
> +}
> +
> +static int configure_geometry(struct atmel_isi *isi, u32 width,
> + u32 height, enum v4l2_mbus_pixelcode code)
> +{
> + u32 cfg2, cr;
> +
> + switch (code) {
> + /* YUV, including grey */
> + case V4L2_MBUS_FMT_Y8_1X8:
> + cr = ISI_CFG2_GRAYSCALE;
> + break;
> + case V4L2_MBUS_FMT_UYVY8_2X8:
> + cr = ISI_CFG2_YCC_SWAP_MODE_3;
> + break;
> + case V4L2_MBUS_FMT_VYUY8_2X8:
> + cr = ISI_CFG2_YCC_SWAP_MODE_2;
> + break;
> + case V4L2_MBUS_FMT_YUYV8_2X8:
> + cr = ISI_CFG2_YCC_SWAP_MODE_1;
> + break;
> + case V4L2_MBUS_FMT_YVYU8_2X8:
> + cr = ISI_CFG2_YCC_SWAP_DEFAULT;
> + break;
> + /* RGB, TODO */
> + default:
> + return -EINVAL;
> + }
> +
> + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> +
> + cfg2 = isi_readl(isi, ISI_CFG2);
> + cfg2 |= cr;
> + /* Set width */
> + cfg2 &= ~(ISI_CFG2_IM_HSIZE_MASK);
> + cfg2 |= ((width - 1) << ISI_CFG2_IM_HSIZE_OFFSET) &
> + ISI_CFG2_IM_HSIZE_MASK;
> + /* Set height */
> + cfg2 &= ~(ISI_CFG2_IM_VSIZE_MASK);
> + cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
> + & ISI_CFG2_IM_VSIZE_MASK;
> + isi_writel(isi, ISI_CFG2, cfg2);
> +
> + return 0;
> +}
> +
> +static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> +{
> + if (isi->active) {
> + struct vb2_buffer *vb = &isi->active->vb;
> + struct frame_buffer *buf = isi->active;
> +
> + list_del_init(&buf->list);
> + do_gettimeofday(&vb->v4l2_buf.timestamp);
> + vb->v4l2_buf.sequence = isi->sequence++;
> + vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> + }
> +
> + if (list_empty(&isi->video_buffer_list)) {
> + isi->active = NULL;
> + } else {
> + /* start next dma frame. */
> + isi->active = list_entry(isi->video_buffer_list.next,
> + struct frame_buffer, list);
> + isi_writel(isi, ISI_DMA_C_DSCR, isi->active->fb_desc_phys);
> + isi_writel(isi, ISI_DMA_C_CTRL,
> + ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
> + isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
> + }
> + return IRQ_HANDLED;
> +}
> +
> +/* ISI interrupt service routine */
> +static irqreturn_t isi_interrupt(int irq, void *dev_id)
> +{
> + struct atmel_isi *isi = dev_id;
> + u32 status, mask, pending;
> + irqreturn_t ret = IRQ_NONE;
> +
> + spin_lock(&isi->lock);
> +
> + status = isi_readl(isi, ISI_STATUS);
> + mask = isi_readl(isi, ISI_INTMASK);
> + pending = status & mask;
> +
> + if (pending & ISI_CTRL_SRST) {
> + complete(&isi->complete);
> + isi_writel(isi, ISI_INTDIS, ISI_CTRL_SRST);
> + ret = IRQ_HANDLED;
> + } else if (pending & ISI_CTRL_DIS) {
> + complete(&isi->complete);
> + isi_writel(isi, ISI_INTDIS, ISI_CTRL_DIS);
> + ret = IRQ_HANDLED;
> + } else {
> + if ((pending & ISI_SR_VSYNC) &&
> + (isi->state == ISI_STATE_IDLE)) {

Superfluous second pair of parenthesis

> + isi->state = ISI_STATE_READY;
> + wake_up_interruptible(&isi->vsync_wq);
> + ret = IRQ_HANDLED;
> + }
> + if (likely(pending & ISI_SR_CXFR_DONE))
> + ret = atmel_isi_handle_streaming(isi);
> + }
> +
> + spin_unlock(&isi->lock);
> + return ret;
> +}
> +
> +#define WAIT_ISI_RESET 1
> +#define WAIT_ISI_DISABLE 0
> +static int atmel_isi_wait_status(struct atmel_isi *isi, int wait_reset)
> +{
> + unsigned long timeout;
> + /*
> + * The reset or disable will only succeed if we have a
> + * pixel clock from the camera.
> + */
> + init_completion(&isi->complete);
> +
> + if (wait_reset) {
> + isi_writel(isi, ISI_INTEN, ISI_CTRL_SRST);
> + isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
> + } else {
> + isi_writel(isi, ISI_INTEN, ISI_CTRL_DIS);
> + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> + }
> +
> + timeout = wait_for_completion_timeout(&isi->complete,
> + msecs_to_jiffies(100));
> + if (timeout == 0)
> + return -ETIMEDOUT;
> +
> + return 0;
> +}
> +
> +/* ------------------------------------------------------------------
> + Videobuf operations
> + ------------------------------------------------------------------*/
> +static int queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
> + unsigned int *nplanes, unsigned long sizes[],
> + void *alloc_ctxs[])
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + unsigned long size;
> + int ret, bytes_per_line;
> +
> + /* Reset ISI */
> + ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
> + if (ret < 0) {
> + dev_err(icd->dev.parent, "Reset ISI timed out\n");
> + return ret;
> + }
> + /* Disable all interrupts */
> + isi_writel(isi, ISI_INTDIS, ~0UL);
> +
> + bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> + icd->current_fmt->host_fmt);
> +
> + if (bytes_per_line < 0)
> + return bytes_per_line;
> +
> + size = bytes_per_line * icd->user_height;
> +
> + if (*nbuffers == 0)
> + *nbuffers = MAX_BUFFER_NUM;
> + else if (*nbuffers > MAX_BUFFER_NUM)
> + *nbuffers = MAX_BUFFER_NUM;

/me wondering, why not just

if (!*nbuffers || *nbuffers > MAX_BUFFER_NUM)
*nbuffers = MAX_BUFFER_NUM;

> +
> + if (size * *nbuffers > VID_LIMIT_BYTES)
> + *nbuffers = VID_LIMIT_BYTES / size;
> +
> + *nplanes = 1;
> + sizes[0] = size;
> + alloc_ctxs[0] = isi->alloc_ctx;
> +
> + isi->sequence = 0;
> + isi->active = NULL;
> +
> + dev_dbg(icd->dev.parent, "%s, count=%d, size=%ld\n", __func__,
> + *nbuffers, size);
> +
> + return 0;
> +}
> +
> +static int buffer_init(struct vb2_buffer *vb)
> +{
> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
> +
> + buf->p_fb_desc = NULL;
> + memset(&buf->fb_desc_phys, 0, sizeof(buf->fb_desc_phys));

I don't think this is what the reviewer meant - here you're using memset
to nullify a u32 field. He probably was asking you to clean the whole
struct frame_buffer, which I actually don't necessarily agree with. So,
unless there's an evidence, why that memest is needed, I'd just keep your
original "->fb_desc_phys = 0". Notice, none of the soc-camera drivers
nullifies anything in .buf_init(), it cannot also be mandatory, because
the actual method is optional.

> + INIT_LIST_HEAD(&buf->list);
> +
> + return 0;
> +}
> +
> +static int buffer_prepare(struct vb2_buffer *vb)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + unsigned long size;
> + struct isi_dma_desc *desc;
> + int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> + icd->current_fmt->host_fmt);
> +
> + if (bytes_per_line < 0)
> + return bytes_per_line;
> +
> + size = bytes_per_line * icd->user_height;
> +
> + if (vb2_plane_size(vb, 0) < size) {
> + dev_err(icd->dev.parent, "%s data will not fit into plane (%lu < %lu)\n",
> + __func__, vb2_plane_size(vb, 0), size);
> + return -EINVAL;
> + }
> +
> + vb2_set_plane_payload(&buf->vb, 0, size);
> +
> + if (!buf->p_fb_desc) {
> + if (list_empty(&isi->dma_desc_head)) {
> + dev_err(icd->dev.parent, "Not enough dma descriptors.\n");
> + return -EINVAL;
> + } else {
> + /* Get an available descriptor */
> + desc = list_entry(isi->dma_desc_head.next,
> + struct isi_dma_desc, list);
> + buf->p_fb_desc = desc->p_fbd;
> + buf->fb_desc_phys = desc->fbd_phys;
> +
> + /* Initialize the dma descriptor */
> + buf->p_fb_desc->fb_address =
> + vb2_dma_contig_plane_paddr(vb, 0);
> + buf->p_fb_desc->next_fbd_address = 0;
> + set_dma_ctrl(buf->p_fb_desc, ISI_DMA_CTRL_WB);
> +
> + /* Delete the descriptor since now it is used */
> + list_del_init(&desc->list);

It probably doesn't matter here, if there were a race, you'd need to
protect anyway, but stylistically you usually first grab ownership of an
object, and only then you modify it, so, it would be more logical to first
list_del(init) the buffer. As for locking as such - we just rely on v4l2
file-operation mutex locking for now.

> + }
> + }
> + return 0;
> +}
> +
> +static void buffer_cleanup(struct vb2_buffer *vb)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
> + unsigned long flags;
> + int i;
> +
> + spin_lock_irqsave(&isi->lock, flags);

You say, this lock protects data, shared with the ISR, these are the
buffer list and the ->state, none of them is accessed below.

> + if (buf->p_fb_desc) {
> + /* This descriptor is available now and we add to head list */
> + for (i = 0; i < MAX_BUFFER_NUM; i++)
> + if (isi->dma_desc[i].p_fbd == buf->p_fb_desc) {
> + list_add(&isi->dma_desc[i].list,
> + &isi->dma_desc_head);
> + break;
> + }

With modified struct frame_buffer the above loop will go.

> +
> + buf->p_fb_desc = NULL;
> + memset(&buf->fb_desc_phys, 0, sizeof(buf->fb_desc_phys));

You really need to do clean up in .buf_cleanup() and in .buf_init()? I
think, one of them should be enough. And memset is bogus here too.

> + }
> + spin_unlock_irqrestore(&isi->lock, flags);
> +}
> +
> +static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
> +{
> + u32 ctrl, cfg1;
> +
> + cfg1 = isi_readl(isi, ISI_CFG1);
> + /* Enable irq: cxfr for the codec path, pxfr for the preview path */
> + isi_writel(isi, ISI_INTEN,
> + ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
> +
> + /* Check if already in a frame */
> + if (isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) {
> + dev_err(isi->icd->dev.parent, "Already in frame handling.\n");
> + return;
> + }
> +
> + isi_writel(isi, ISI_DMA_C_DSCR, buffer->fb_desc_phys);
> + isi_writel(isi, ISI_DMA_C_CTRL, ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
> + isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
> +
> + /* Enable linked list */
> + cfg1 |= isi->pdata->frate | ISI_CFG1_DISCR;
> +
> + /* Enable codec path and ISI */
> + ctrl = ISI_CTRL_CDC | ISI_CTRL_EN;
> + isi_writel(isi, ISI_CTRL, ctrl);
> + isi_writel(isi, ISI_CFG1, cfg1);
> +}
> +
> +static void buffer_queue(struct vb2_buffer *vb)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
> + unsigned long flags = 0;
> +
> + spin_lock_irqsave(&isi->lock, flags);
> + list_add_tail(&buf->list, &isi->video_buffer_list);
> +
> + if (isi->active == NULL) {
> + isi->active = buf;
> + start_dma(isi, buf);
> + }
> + spin_unlock_irqrestore(&isi->lock, flags);
> +}
> +
> +static int start_streaming(struct vb2_queue *vq)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> +
> + u32 sr = 0;
> + int ret;
> +
> + spin_lock_irq(&isi->lock);
> + isi->state = ISI_STATE_IDLE;
> +
> + /* Clear any pending SOF interrupt */
> + sr = isi_readl(isi, ISI_STATUS);
> + /* Enable VSYNC interrupt for SOF */
> + isi_writel(isi, ISI_INTEN, ISI_SR_VSYNC);
> + isi_writel(isi, ISI_CTRL, ISI_CTRL_EN);
> +
> + spin_unlock_irq(&isi->lock);
> + dev_dbg(icd->dev.parent, "Waiting for SOF\n");
> + ret = wait_event_interruptible(isi->vsync_wq,
> + isi->state != ISI_STATE_IDLE);
> + if (ret)
> + return ret;
> +
> + if (isi->state != ISI_STATE_READY)
> + return -EIO;
> +
> + spin_lock_irq(&isi->lock);
> + isi->state = ISI_STATE_WAIT_SOF;
> + isi_writel(isi, ISI_INTDIS, ISI_SR_VSYNC);
> + spin_unlock_irq(&isi->lock);
> +
> + return 0;
> +}
> +
> +/* abort streaming and wait for last buffer */
> +static int stop_streaming(struct vb2_queue *vq)
> +{
> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + struct frame_buffer *buf, *node;
> + int ret = 0;
> + unsigned long timeout;
> +
> + spin_lock_irq(&isi->lock);
> + isi->active = NULL;
> + /* Release all active buffers */
> + list_for_each_entry_safe(buf, node, &isi->video_buffer_list, list) {
> + list_del_init(&buf->list);
> + vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
> + }
> + spin_unlock_irq(&isi->lock);
> +
> + timeout = jiffies + FRAME_INTERVAL_MILLI_SEC * HZ;
> + /* Wait until the end of the current frame. */
> + while ((isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) &&
> + time_before(jiffies, timeout))
> + msleep(1);
> +
> + if (time_after(jiffies, timeout)) {
> + dev_err(icd->dev.parent,
> + "Timeout waiting for finishing codec request\n");
> + return -ETIMEDOUT;
> + }
> +
> + /* Disable interrupts */
> + isi_writel(isi, ISI_INTDIS,
> + ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
> +
> + /* Disable ISI and wait for it is done */
> + ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
> + if (ret < 0)
> + dev_err(icd->dev.parent, "Disable ISI timed out\n");
> +
> + return ret;
> +}
> +
> +static struct vb2_ops isi_video_qops = {
> + .queue_setup = queue_setup,
> + .buf_init = buffer_init,
> + .buf_prepare = buffer_prepare,
> + .buf_cleanup = buffer_cleanup,
> + .buf_queue = buffer_queue,
> + .start_streaming = start_streaming,
> + .stop_streaming = stop_streaming,
> + .wait_prepare = soc_camera_unlock,
> + .wait_finish = soc_camera_lock,
> +};
> +
> +/* ------------------------------------------------------------------
> + SOC camera operations for the device
> + ------------------------------------------------------------------*/
> +static int isi_camera_init_videobuf(struct vb2_queue *q,
> + struct soc_camera_device *icd)
> +{
> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + q->io_modes = VB2_MMAP;
> + q->drv_priv = icd;
> + q->buf_struct_size = sizeof(struct frame_buffer);
> + q->ops = &isi_video_qops;
> + q->mem_ops = &vb2_dma_contig_memops;
> +
> + return vb2_queue_init(q);
> +}
> +
> +static int isi_camera_set_fmt(struct soc_camera_device *icd,
> + struct v4l2_format *f)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> + const struct soc_camera_format_xlate *xlate;
> + struct v4l2_pix_format *pix = &f->fmt.pix;
> + struct v4l2_mbus_framefmt mf;
> + int ret;
> +
> + xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
> + if (!xlate) {
> + dev_warn(icd->dev.parent, "Format %x not found\n",
> + pix->pixelformat);
> + return -EINVAL;
> + }
> +
> + dev_dbg(icd->dev.parent, "Plan to set format %dx%d\n",
> + pix->width, pix->height);
> +
> + mf.width = pix->width;
> + mf.height = pix->height;
> + mf.field = pix->field;
> + mf.colorspace = pix->colorspace;
> + mf.code = xlate->code;
> +
> + ret = v4l2_subdev_call(sd, video, s_mbus_fmt, &mf);
> + if (ret < 0)
> + return ret;
> +
> + if (mf.code != xlate->code)
> + return -EINVAL;
> +
> + ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
> + if (ret < 0)
> + return ret;
> +
> + pix->width = mf.width;
> + pix->height = mf.height;
> + pix->field = mf.field;
> + pix->colorspace = mf.colorspace;
> + icd->current_fmt = xlate;
> +
> + dev_dbg(icd->dev.parent, "Finally set format %dx%d\n",
> + pix->width, pix->height);
> +
> + return ret;
> +}
> +
> +static int isi_camera_try_fmt(struct soc_camera_device *icd,
> + struct v4l2_format *f)
> +{
> + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> + const struct soc_camera_format_xlate *xlate;
> + struct v4l2_pix_format *pix = &f->fmt.pix;
> + struct v4l2_mbus_framefmt mf;
> + u32 pixfmt = pix->pixelformat;
> + int ret;
> +
> + xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
> + if (pixfmt && !xlate) {
> + dev_warn(icd->dev.parent, "Format %x not found\n", pixfmt);
> + return -EINVAL;
> + }
> +
> + /* limit to Atmel ISI hardware capabilities */
> + if (pix->height > MAX_SUPPORT_HEIGHT)
> + pix->height = MAX_SUPPORT_HEIGHT;
> + if (pix->width > MAX_SUPPORT_WIDTH)
> + pix->width = MAX_SUPPORT_WIDTH;
> +
> + /* limit to sensor capabilities */
> + mf.width = pix->width;
> + mf.height = pix->height;
> + mf.field = pix->field;
> + mf.colorspace = pix->colorspace;
> + mf.code = xlate->code;
> +
> + ret = v4l2_subdev_call(sd, video, try_mbus_fmt, &mf);
> + if (ret < 0)
> + return ret;
> +
> + pix->width = mf.width;
> + pix->height = mf.height;
> + pix->colorspace = mf.colorspace;
> +
> + switch (mf.field) {
> + case V4L2_FIELD_ANY:
> + pix->field = V4L2_FIELD_NONE;
> + break;
> + case V4L2_FIELD_NONE:
> + break;
> + default:
> + dev_err(icd->dev.parent, "Field type %d unsupported.\n",
> + mf.field);
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +
> +static const struct soc_mbus_pixelfmt isi_camera_formats[] = {
> + {
> + .fourcc = V4L2_PIX_FMT_YUYV,
> + .name = "Packed YUV422 16 bit",
> + .bits_per_sample = 8,
> + .packing = SOC_MBUS_PACKING_2X8_PADHI,
> + .order = SOC_MBUS_ORDER_LE,
> + },
> +};
> +
> +/* This will be corrected as we get more formats */
> +static bool isi_camera_packing_supported(const struct soc_mbus_pixelfmt *fmt)
> +{
> + return fmt->packing == SOC_MBUS_PACKING_NONE ||
> + (fmt->bits_per_sample == 8 &&
> + fmt->packing == SOC_MBUS_PACKING_2X8_PADHI) ||
> + (fmt->bits_per_sample > 8 &&
> + fmt->packing == SOC_MBUS_PACKING_EXTEND16);
> +}
> +
> +static unsigned long make_bus_param(struct atmel_isi *isi)
> +{
> + unsigned long flags;
> + /*
> + * Platform specified synchronization and pixel clock polarities are
> + * only a recommendation and are only used during probing. Atmel ISI
> + * camera interface only works in master mode, i.e., uses HSYNC and
> + * VSYNC signals from the sensor
> + */
> + flags = SOCAM_MASTER |
> + SOCAM_HSYNC_ACTIVE_HIGH |
> + SOCAM_HSYNC_ACTIVE_LOW |
> + SOCAM_VSYNC_ACTIVE_HIGH |
> + SOCAM_VSYNC_ACTIVE_LOW |
> + SOCAM_PCLK_SAMPLE_RISING |
> + SOCAM_PCLK_SAMPLE_FALLING |
> + SOCAM_DATA_ACTIVE_HIGH;
> +
> + if (isi->pdata->data_width_flags & ISI_DATAWIDTH_10)
> + flags |= SOCAM_DATAWIDTH_10;
> +
> + if (isi->pdata->data_width_flags & ISI_DATAWIDTH_8)
> + flags |= SOCAM_DATAWIDTH_8;
> +
> + if (flags & SOCAM_DATAWIDTH_MASK)
> + return flags;
> +
> + return 0;
> +}
> +
> +static int isi_camera_try_bus_param(struct soc_camera_device *icd,
> + unsigned char buswidth)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + unsigned long camera_flags;
> + int ret;
> +
> + camera_flags = icd->ops->query_bus_param(icd);
> + ret = soc_camera_bus_param_compatible(camera_flags,
> + make_bus_param(isi));
> + if (!ret)
> + return -EINVAL;
> + return 0;
> +}
> +
> +
> +static int isi_camera_get_formats(struct soc_camera_device *icd,
> + unsigned int idx,
> + struct soc_camera_format_xlate *xlate)
> +{
> + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> + int formats = 0, ret;
> + /* sensor format */
> + enum v4l2_mbus_pixelcode code;
> + /* soc camera host format */
> + const struct soc_mbus_pixelfmt *fmt;
> +
> + ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, idx, &code);
> + if (ret < 0)
> + /* No more formats */
> + return 0;
> +
> + fmt = soc_mbus_get_fmtdesc(code);
> + if (!fmt) {
> + dev_err(icd->dev.parent,
> + "Invalid format code #%u: %d\n", idx, code);
> + return 0;
> + }
> +
> + /* This also checks support for the requested bits-per-sample */
> + ret = isi_camera_try_bus_param(icd, fmt->bits_per_sample);
> + if (ret < 0) {
> + dev_err(icd->dev.parent,
> + "Fail to try the bus parameters.\n");
> + return 0;
> + }
> +
> + switch (code) {
> + case V4L2_MBUS_FMT_UYVY8_2X8:
> + case V4L2_MBUS_FMT_VYUY8_2X8:
> + case V4L2_MBUS_FMT_YUYV8_2X8:
> + case V4L2_MBUS_FMT_YVYU8_2X8:
> + formats++;
> + if (xlate) {
> + xlate->host_fmt = &isi_camera_formats[0];
> + xlate->code = code;
> + xlate++;
> + dev_dbg(icd->dev.parent, "Providing format %s using code %d\n",
> + isi_camera_formats[0].name, code);
> + }
> + break;
> + default:
> + if (!isi_camera_packing_supported(fmt))
> + return 0;
> + if (xlate)
> + dev_dbg(icd->dev.parent,
> + "Providing format %s in pass-through mode\n",
> + fmt->name);
> + }
> +
> + /* Generic pass-through */
> + formats++;
> + if (xlate) {
> + xlate->host_fmt = fmt;
> + xlate->code = code;
> + xlate++;
> + }
> +
> + return formats;
> +}
> +
> +/* Called with .video_lock held */
> +static int isi_camera_add_device(struct soc_camera_device *icd)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + int ret;
> +
> + if (isi->icd)
> + return -EBUSY;
> +
> + ret = clk_enable(isi->pclk);
> + if (ret)
> + return ret;
> +
> + isi->icd = icd;
> + dev_dbg(icd->dev.parent, "Atmel ISI Camera driver attached to camera %d\n",
> + icd->devnum);
> + return 0;
> +}
> +/* Called with .video_lock held */
> +static void isi_camera_remove_device(struct soc_camera_device *icd)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> +
> + BUG_ON(icd != isi->icd);
> +
> + clk_disable(isi->pclk);
> + isi->icd = NULL;
> +
> + dev_dbg(icd->dev.parent, "Atmel ISI Camera driver detached from camera %d\n",
> + icd->devnum);
> +}
> +
> +static unsigned int isi_camera_poll(struct file *file, poll_table *pt)
> +{
> + struct soc_camera_device *icd = file->private_data;
> +
> + return vb2_poll(&icd->vb2_vidq, file, pt);
> +}
> +
> +static int isi_camera_querycap(struct soc_camera_host *ici,
> + struct v4l2_capability *cap)
> +{
> + strcpy(cap->driver, "atmel-isi");
> + strcpy(cap->card, "Atmel Image Sensor Interface");
> + cap->capabilities = (V4L2_CAP_VIDEO_CAPTURE |
> + V4L2_CAP_STREAMING);
> + return 0;
> +}
> +
> +static int isi_camera_set_bus_param(struct soc_camera_device *icd, u32 pixfmt)
> +{
> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> + struct atmel_isi *isi = ici->priv;
> + unsigned long bus_flags, camera_flags, common_flags;
> + int ret;
> + u32 cfg1 = 0;
> +
> + camera_flags = icd->ops->query_bus_param(icd);
> +
> + bus_flags = make_bus_param(isi);
> + common_flags = soc_camera_bus_param_compatible(camera_flags, bus_flags);
> + dev_dbg(icd->dev.parent, "Flags cam: 0x%lx host: 0x%lx common: 0x%lx\n",
> + camera_flags, bus_flags, common_flags);
> + if (!common_flags)
> + return -EINVAL;
> +
> + /* Make choises, based on platform preferences */
> + if ((common_flags & SOCAM_HSYNC_ACTIVE_HIGH) &&
> + (common_flags & SOCAM_HSYNC_ACTIVE_LOW)) {
> + if (isi->pdata->hsync_act_low)
> + common_flags &= ~SOCAM_HSYNC_ACTIVE_HIGH;
> + else
> + common_flags &= ~SOCAM_HSYNC_ACTIVE_LOW;
> + }
> +
> + if ((common_flags & SOCAM_VSYNC_ACTIVE_HIGH) &&
> + (common_flags & SOCAM_VSYNC_ACTIVE_LOW)) {
> + if (isi->pdata->vsync_act_low)
> + common_flags &= ~SOCAM_VSYNC_ACTIVE_HIGH;
> + else
> + common_flags &= ~SOCAM_VSYNC_ACTIVE_LOW;
> + }
> +
> + if ((common_flags & SOCAM_PCLK_SAMPLE_RISING) &&
> + (common_flags & SOCAM_PCLK_SAMPLE_FALLING)) {
> + if (isi->pdata->pclk_act_falling)
> + common_flags &= ~SOCAM_PCLK_SAMPLE_RISING;
> + else
> + common_flags &= ~SOCAM_PCLK_SAMPLE_FALLING;
> + }
> +
> + ret = icd->ops->set_bus_param(icd, common_flags);
> + if (ret < 0) {
> + dev_dbg(icd->dev.parent, "Camera set_bus_param(%lx) returned %d\n",
> + common_flags, ret);
> + return ret;
> + }
> +
> + /* set bus param for ISI */
> + if (common_flags & SOCAM_HSYNC_ACTIVE_LOW)
> + cfg1 |= ISI_CFG1_HSYNC_POL_ACTIVE_LOW;
> + if (common_flags & SOCAM_VSYNC_ACTIVE_LOW)
> + cfg1 |= ISI_CFG1_VSYNC_POL_ACTIVE_LOW;
> + if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
> + cfg1 |= ISI_CFG1_PIXCLK_POL_ACTIVE_FALLING;
> +
> + if (isi->pdata->has_emb_sync)
> + cfg1 |= ISI_CFG1_EMB_SYNC;
> + if (isi->pdata->isi_full_mode)
> + cfg1 |= ISI_CFG1_FULL_MODE;
> +
> + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> + isi_writel(isi, ISI_CFG1, cfg1);
> +
> + return 0;
> +}
> +
> +static struct soc_camera_host_ops isi_soc_camera_host_ops = {
> + .owner = THIS_MODULE,
> + .add = isi_camera_add_device,
> + .remove = isi_camera_remove_device,
> + .set_fmt = isi_camera_set_fmt,
> + .try_fmt = isi_camera_try_fmt,
> + .get_formats = isi_camera_get_formats,
> + .init_videobuf2 = isi_camera_init_videobuf,
> + .poll = isi_camera_poll,
> + .querycap = isi_camera_querycap,
> + .set_bus_param = isi_camera_set_bus_param,
> +};
> +
> +/* -----------------------------------------------------------------------*/
> +static int __devexit atmel_isi_remove(struct platform_device *pdev)
> +{
> + struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
> + struct atmel_isi *isi = container_of(soc_host,
> + struct atmel_isi, soc_host);
> +

Wouldn't it be better to first free IRQs to prevent any possible
interrupts?

> + soc_camera_host_unregister(soc_host);
> + vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
> + dma_free_coherent(&pdev->dev,
> + sizeof(struct fbd) * MAX_BUFFER_NUM,
> + isi->p_fb_descriptors,
> + isi->fb_descriptors_phys);
> +
> + free_irq(isi->irq, isi);
> + iounmap(isi->regs);
> + clk_put(isi->pclk);
> + kfree(isi);
> +
> + return 0;
> +}
> +
> +static int __devinit atmel_isi_probe(struct platform_device *pdev)
> +{
> + unsigned int irq;
> + struct atmel_isi *isi;
> + struct clk *pclk;
> + struct resource *regs;
> + int ret, i;
> + struct device *dev = &pdev->dev;
> + struct soc_camera_host *soc_host;
> +
> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!regs)
> + return -ENXIO;
> +
> + pclk = clk_get(&pdev->dev, "isi_clk");
> + if (IS_ERR(pclk))
> + return PTR_ERR(pclk);
> +
> + isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
> + if (!isi) {
> + ret = -ENOMEM;
> + dev_err(&pdev->dev, "Can't allocate interface!\n");
> + goto err_alloc_isi;
> + }
> +
> + isi->pclk = pclk;
> +
> + spin_lock_init(&isi->lock);
> + init_waitqueue_head(&isi->vsync_wq);
> +
> + isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
> + sizeof(struct fbd) * MAX_BUFFER_NUM,
> + &isi->fb_descriptors_phys,
> + GFP_KERNEL);
> + if (!isi->p_fb_descriptors) {
> + ret = -ENOMEM;
> + dev_err(&pdev->dev, "Can't allocate descriptors!\n");
> + goto err_alloc_descriptors;
> + }
> +
> + INIT_LIST_HEAD(&isi->dma_desc_head);
> + for (i = 0; i < MAX_BUFFER_NUM; i++) {
> + isi->dma_desc[i].p_fbd = isi->p_fb_descriptors + i;
> + isi->dma_desc[i].fbd_phys = isi->fb_descriptors_phys +
> + i * sizeof(struct fbd);
> + list_add(&isi->dma_desc[i].list, &isi->dma_desc_head);
> + }
> +
> + isi->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
> + if (IS_ERR(isi->alloc_ctx)) {
> + ret = PTR_ERR(isi->alloc_ctx);
> + goto err_alloc_ctx;
> + }
> +
> + isi->regs = ioremap(regs->start, resource_size(regs));
> + if (!isi->regs) {
> + ret = -ENOMEM;
> + goto err_ioremap;
> + }
> +
> + if (!dev->platform_data) {

I would do this check right in the beginning of this function to simplify
error handling.

> + dev_err(&pdev->dev,
> + "No config available for Atmel ISI\n");
> + ret = -EINVAL;
> + goto err_req_irq;
> + }
> +
> + isi->pdata = dev->platform_data;
> + if (isi->pdata->data_width_flags == 0) {

And this check too. You can even unite them into one like

+ if (!dev->platform_data || !dev->platform_data->data_width_flags) {
+ ...
+ }

Thanks
Guennadi

> + dev_err(&pdev->dev,
> + "Invalid data width for Atmel ISI.\n");
> + ret = -EINVAL;
> + goto err_req_irq;
> + }
> +
> + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + ret = irq;
> + goto err_req_irq;
> + }
> +
> + ret = request_irq(irq, isi_interrupt, 0, "isi", isi);
> + if (ret) {
> + dev_err(&pdev->dev, "Unable to request irq %d\n", irq);
> + goto err_req_irq;
> + }
> + isi->irq = irq;
> +
> + INIT_LIST_HEAD(&isi->video_buffer_list);
> + isi->active = NULL;
> +
> + soc_host = &isi->soc_host;
> + soc_host->drv_name = "isi-camera";
> + soc_host->ops = &isi_soc_camera_host_ops;
> + soc_host->priv = isi;
> + soc_host->v4l2_dev.dev = &pdev->dev;
> + soc_host->nr = pdev->id;
> +
> + ret = soc_camera_host_register(soc_host);
> + if (ret) {
> + dev_err(&pdev->dev, "Unable to register soc camera host\n");
> + goto err_register_soc_camera_host;
> + }
> + return 0;
> +
> +err_register_soc_camera_host:
> + free_irq(isi->irq, isi);
> +err_req_irq:
> + iounmap(isi->regs);
> +err_ioremap:
> + vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
> +err_alloc_ctx:
> + dma_free_coherent(&pdev->dev,
> + sizeof(struct fbd) * MAX_BUFFER_NUM,
> + isi->p_fb_descriptors,
> + isi->fb_descriptors_phys);
> +err_alloc_descriptors:
> + kfree(isi);
> +err_alloc_isi:
> + clk_put(isi->pclk);
> +
> + return ret;
> +}
> +
> +static struct platform_driver atmel_isi_driver = {
> + .probe = atmel_isi_probe,
> + .remove = __devexit_p(atmel_isi_remove),
> + .driver = {
> + .name = "atmel_isi",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init atmel_isi_init_module(void)
> +{
> + return platform_driver_probe(&atmel_isi_driver, &atmel_isi_probe);
> +}
> +
> +static void __exit atmel_isi_exit(void)
> +{
> + platform_driver_unregister(&atmel_isi_driver);
> +}
> +module_init(atmel_isi_init_module);
> +module_exit(atmel_isi_exit);
> +
> +MODULE_AUTHOR("Josh Wu <[email protected]>");
> +MODULE_DESCRIPTION("The V4L2 driver for Atmel Linux");
> +MODULE_LICENSE("GPL");
> +MODULE_SUPPORTED_DEVICE("video");
> diff --git a/include/media/atmel-isi.h b/include/media/atmel-isi.h
> new file mode 100644
> index 0000000..26cece5
> --- /dev/null
> +++ b/include/media/atmel-isi.h
> @@ -0,0 +1,119 @@
> +/*
> + * Register definitions for the Atmel Image Sensor Interface.
> + *
> + * Copyright (C) 2011 Atmel Corporation
> + * Josh Wu, <[email protected]>
> + *
> + * Based on previous work by Lars Haring, <[email protected]>
> + * and Sedji Gaouaou
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef __ATMEL_ISI_H__
> +#define __ATMEL_ISI_H__
> +
> +#include <linux/types.h>
> +
> +/* ISI_V2 register offsets */
> +#define ISI_CFG1 0x0000
> +#define ISI_CFG2 0x0004
> +#define ISI_PSIZE 0x0008
> +#define ISI_PDECF 0x000c
> +#define ISI_Y2R_SET0 0x0010
> +#define ISI_Y2R_SET1 0x0014
> +#define ISI_R2Y_SET0 0x0018
> +#define ISI_R2Y_SET1 0x001C
> +#define ISI_R2Y_SET2 0x0020
> +#define ISI_CTRL 0x0024
> +#define ISI_STATUS 0x0028
> +#define ISI_INTEN 0x002C
> +#define ISI_INTDIS 0x0030
> +#define ISI_INTMASK 0x0034
> +#define ISI_DMA_CHER 0x0038
> +#define ISI_DMA_CHDR 0x003C
> +#define ISI_DMA_CHSR 0x0040
> +#define ISI_DMA_P_ADDR 0x0044
> +#define ISI_DMA_P_CTRL 0x0048
> +#define ISI_DMA_P_DSCR 0x004C
> +#define ISI_DMA_C_ADDR 0x0050
> +#define ISI_DMA_C_CTRL 0x0054
> +#define ISI_DMA_C_DSCR 0x0058
> +
> +/* Bitfields in CFG1 */
> +#define ISI_CFG1_HSYNC_POL_ACTIVE_LOW (1 << 2)
> +#define ISI_CFG1_VSYNC_POL_ACTIVE_LOW (1 << 3)
> +#define ISI_CFG1_PIXCLK_POL_ACTIVE_FALLING (1 << 4)
> +#define ISI_CFG1_EMB_SYNC (1 << 6)
> +#define ISI_CFG1_CRC_SYNC (1 << 7)
> +/* Constants for FRATE(ISI_V2) */
> +#define ISI_CFG1_FRATE_CAPTURE_ALL (0 << 8)
> +#define ISI_CFG1_FRATE_DIV_2 (1 << 8)
> +#define ISI_CFG1_FRATE_DIV_3 (2 << 8)
> +#define ISI_CFG1_FRATE_DIV_4 (3 << 8)
> +#define ISI_CFG1_FRATE_DIV_5 (4 << 8)
> +#define ISI_CFG1_FRATE_DIV_6 (5 << 8)
> +#define ISI_CFG1_FRATE_DIV_7 (6 << 8)
> +#define ISI_CFG1_FRATE_DIV_8 (7 << 8)
> +#define ISI_CFG1_DISCR (1 << 11)
> +#define ISI_CFG1_FULL_MODE (1 << 12)
> +
> +/* Bitfields in CFG2 */
> +#define ISI_CFG2_GRAYSCALE (1 << 13)
> +/* Constants for YCC_SWAP(ISI_V2) */
> +#define ISI_CFG2_YCC_SWAP_DEFAULT (0 << 28)
> +#define ISI_CFG2_YCC_SWAP_MODE_1 (1 << 28)
> +#define ISI_CFG2_YCC_SWAP_MODE_2 (2 << 28)
> +#define ISI_CFG2_YCC_SWAP_MODE_3 (3 << 28)
> +#define ISI_CFG2_IM_VSIZE_OFFSET 0
> +#define ISI_CFG2_IM_HSIZE_OFFSET 16
> +#define ISI_CFG2_IM_VSIZE_MASK (0x7FF << ISI_CFG2_IM_VSIZE_OFFSET)
> +#define ISI_CFG2_IM_HSIZE_MASK (0x7FF << ISI_CFG2_IM_HSIZE_OFFSET)
> +
> +/* Bitfields in CTRL */
> +/* Also using in SR(ISI_V2) */
> +#define ISI_CTRL_EN (1 << 0)
> +#define ISI_CTRL_CDC (1 << 8)
> +/* Also using in SR/IER/IDR/IMR(ISI_V2) */
> +#define ISI_CTRL_DIS (1 << 1)
> +#define ISI_CTRL_SRST (1 << 2)
> +
> +/* Bitfields in SR */
> +#define ISI_SR_SIP (1 << 19)
> +/* Also using in SR/IER/IDR/IMR */
> +#define ISI_SR_VSYNC (1 << 10)
> +#define ISI_SR_PXFR_DONE (1 << 16)
> +#define ISI_SR_CXFR_DONE (1 << 17)
> +#define ISI_SR_P_OVR (1 << 24)
> +#define ISI_SR_C_OVR (1 << 25)
> +#define ISI_SR_CRC_ERR (1 << 26)
> +#define ISI_SR_FR_OVR (1 << 27)
> +
> +/* Bitfields in DMA_C_CTRL & in DMA_P_CTRL */
> +#define ISI_DMA_CTRL_FETCH (1 << 0)
> +#define ISI_DMA_CTRL_WB (1 << 1)
> +#define ISI_DMA_CTRL_IEN (1 << 2)
> +#define ISI_DMA_CTRL_DONE (1 << 3)
> +
> +/* Bitfields in DMA_CHSR/CHER/CHDR */
> +#define ISI_DMA_CHSR_P_CH (1 << 0)
> +#define ISI_DMA_CHSR_C_CH (1 << 1)
> +
> +/* Definition for isi_platform_data */
> +#define ISI_DATAWIDTH_8 0x01
> +#define ISI_DATAWIDTH_10 0x02
> +
> +struct isi_platform_data {
> + u8 has_emb_sync;
> + u8 emb_crc_sync;
> + u8 hsync_act_low;
> + u8 vsync_act_low;
> + u8 pclk_act_falling;
> + u8 isi_full_mode;
> + u32 data_width_flags;
> + /* Using for ISI_CFG1 */
> + u32 frate;
> +};
> +
> +#endif /* __ATMEL_ISI_H__ */
> --
> 1.6.3.3
>

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

2011-06-07 07:05:46

by Josh Wu

[permalink] [raw]
Subject: RE: [PATCH v3] [media] at91: add Atmel Image Sensor Interface (ISI) support

Hi, Guennadi

On Friday, June 03, 2011, Guennadi Liakhovetski wrote

> On Fri, 3 Jun 2011, Josh Wu wrote:

>> This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
>> - Using soc-camera framework with videobuf2 dma-contig allocator
>> - Supporting video streaming of YUV packed format
>> - Tested on AT91SAM9M10G45-EK with OV2640
>>
>> Signed-off-by: Josh Wu <[email protected]>
>> ---
>> base on branch staging/for_v3.0
>> Modified in V3:
>> refined the interrupt handling code.
>> added a list to manage the allocated descriptors.
>> removed redundant code in isi_camera_set_bus_param().
>> return error when platform data is not provided.
>>
>> drivers/media/video/Kconfig | 8 +
>> drivers/media/video/Makefile | 1 +
>> drivers/media/video/atmel-isi.c | 1074 +++++++++++++++++++++++++++++++++++++++
>> include/media/atmel-isi.h | 119 +++++
>> 4 files changed, 1202 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/media/video/atmel-isi.c
>> create mode 100644 include/media/atmel-isi.h
>>
>> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
>> index bb53de7..93d1098 100644
>> --- a/drivers/media/video/Kconfig
>> +++ b/drivers/media/video/Kconfig
>> @@ -952,6 +952,14 @@ config VIDEO_SAMSUNG_S5P_FIMC
>> To compile this driver as a module, choose M here: the
>> module will be called s5p-fimc.
>>
>> +config VIDEO_ATMEL_ISI
>> + tristate "ATMEL Image Sensor Interface (ISI) support"
>> + depends on VIDEO_DEV && SOC_CAMERA && ARCH_AT91
>> + select VIDEOBUF2_DMA_CONTIG
>> + ---help---
>> + This module makes the ATMEL Image Sensor Interface available
>> + as a v4l2 device.
>> +
>> config VIDEO_S5P_MIPI_CSIS
>> tristate "Samsung S5P and EXYNOS4 MIPI CSI receiver driver"
>> depends on VIDEO_V4L2 && PM_RUNTIME && PLAT_S5P && VIDEO_V4L2_SUBDEV_API
>> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
>> index f0fecd6..d9833f4 100644
>> --- a/drivers/media/video/Makefile
>> +++ b/drivers/media/video/Makefile
>> @@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_PXA27x) += pxa_camera.o
>> obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += sh_mobile_csi2.o
>> obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o
>> obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o
>> +obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>>
>> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc/
>>
>> diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
>> new file mode 100644
>> index 0000000..a020bd3
>> --- /dev/null
>> +++ b/drivers/media/video/atmel-isi.c
>> @@ -0,0 +1,1074 @@
>> +/*
>> + * Copyright (c) 2011 Atmel Corporation
>> + * Josh Wu, <[email protected]>
>> + *
>> + * Based on previous work by Lars Haring, <[email protected]>
>> + * and Sedji Gaouaou
>> + * Based on the bttv driver for Bt848 with respective copyright holders
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/fs.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +
>> +#include <media/atmel-isi.h>
>> +#include <media/soc_camera.h>
>> +#include <media/soc_mediabus.h>
>> +#include <media/videobuf2-dma-contig.h>
>> +
>> +#define MAX_BUFFER_NUM 32
>> +#define MAX_SUPPORT_WIDTH 2048
>> +#define MAX_SUPPORT_HEIGHT 2048
>> +#define VID_LIMIT_BYTES (16 * 1024 * 1024)
>> +#define MIN_FRAME_RATE 15
>> +#define FRAME_INTERVAL_MILLI_SEC (1000 / MIN_FRAME_RATE)
>> +
>> +/* ISI states */
>> +enum {
>> + ISI_STATE_IDLE = 0,
>> + ISI_STATE_READY,
>> + ISI_STATE_WAIT_SOF,
>> +};
>> +
>> +/* Frame buffer descriptor */
>> +struct fbd {
>> + /* Physical address of the frame buffer */
>> + u32 fb_address;
>> + /* DMA Control Register(only in HISI2) */
>> + u32 dma_ctrl;
>> + /* Physical address of the next fbd */
>> + u32 next_fbd_address;
>> +};
>> +
>> +static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl)
>> +{
>> + fb_desc->dma_ctrl = ctrl;
>> +}
>> +
>> +struct isi_dma_desc {
>> + struct list_head list;
>> + struct fbd *p_fbd;
>> + u32 fbd_phys;
>> +};
>> +
>> +/* Frame buffer data */
>> +struct frame_buffer {
>> + struct vb2_buffer vb;
>> + struct fbd *p_fb_desc;
>> + u32 fb_desc_phys;

> Why don't you replace the above two members with "struct isi_dma_desc *"?
> Then you also don't need to scan the DMA descriptor array in
> .buf_cleanup().

Will fix in v4. This change makes code simpler.

>> + struct list_head list;
>> +};
>> +
>> +struct atmel_isi {
>> + /* Protects the access of variables shared with the ISR */
>> + spinlock_t lock;
>> + void __iomem *regs;
>> +
>> + int sequence;
>> + /* State of the ISI module in capturing mode */
>> + int state;
>> +
>> + /* Wait queue for waiting for SOF */
>> + wait_queue_head_t vsync_wq;
>> +
>> + struct vb2_alloc_ctx *alloc_ctx;
>> +
>> + /* Allocate descriptors for dma buffer use */
>> + struct fbd *p_fb_descriptors;
>> + u32 fb_descriptors_phys;
>> + struct list_head dma_desc_head;
>> + struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
>> +
>> + struct completion complete;
>> + struct clk *pclk;
>> + unsigned int irq;
>> +
>> + struct isi_platform_data *pdata;
>> +
>> + struct list_head video_buffer_list;
>> + struct frame_buffer *active;
>> +
>> + struct soc_camera_device *icd;
>> + struct soc_camera_host soc_host;
>> +};
>> +
>> +static void isi_writel(struct atmel_isi *isi, u32 reg, u32 val)
>> +{
>> + writel(val, isi->regs + reg);
>> +}
>> +static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>> +{
>> + return readl(isi->regs + reg);
>> +}
>> +
>> +static int configure_geometry(struct atmel_isi *isi, u32 width,
>> + u32 height, enum v4l2_mbus_pixelcode code)
>> +{
>> + u32 cfg2, cr;
>> +
>> + switch (code) {
>> + /* YUV, including grey */
>> + case V4L2_MBUS_FMT_Y8_1X8:
>> + cr = ISI_CFG2_GRAYSCALE;
>> + break;
>> + case V4L2_MBUS_FMT_UYVY8_2X8:
>> + cr = ISI_CFG2_YCC_SWAP_MODE_3;
>> + break;
>> + case V4L2_MBUS_FMT_VYUY8_2X8:
>> + cr = ISI_CFG2_YCC_SWAP_MODE_2;
>> + break;
>> + case V4L2_MBUS_FMT_YUYV8_2X8:
>> + cr = ISI_CFG2_YCC_SWAP_MODE_1;
>> + break;
>> + case V4L2_MBUS_FMT_YVYU8_2X8:
>> + cr = ISI_CFG2_YCC_SWAP_DEFAULT;
>> + break;
>> + /* RGB, TODO */
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> +
>> + cfg2 = isi_readl(isi, ISI_CFG2);
>> + cfg2 |= cr;
>> + /* Set width */
>> + cfg2 &= ~(ISI_CFG2_IM_HSIZE_MASK);
>> + cfg2 |= ((width - 1) << ISI_CFG2_IM_HSIZE_OFFSET) &
>> + ISI_CFG2_IM_HSIZE_MASK;
>> + /* Set height */
>> + cfg2 &= ~(ISI_CFG2_IM_VSIZE_MASK);
>> + cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
>> + & ISI_CFG2_IM_VSIZE_MASK;
>> + isi_writel(isi, ISI_CFG2, cfg2);
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>> +{
>> + if (isi->active) {
>> + struct vb2_buffer *vb = &isi->active->vb;
>> + struct frame_buffer *buf = isi->active;
>> +
>> + list_del_init(&buf->list);
>> + do_gettimeofday(&vb->v4l2_buf.timestamp);
>> + vb->v4l2_buf.sequence = isi->sequence++;
>> + vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
>> + }
>> +
>> + if (list_empty(&isi->video_buffer_list)) {
>> + isi->active = NULL;
>> + } else {
>> + /* start next dma frame. */
>> + isi->active = list_entry(isi->video_buffer_list.next,
>> + struct frame_buffer, list);
>> + isi_writel(isi, ISI_DMA_C_DSCR, isi->active->fb_desc_phys);
>> + isi_writel(isi, ISI_DMA_C_CTRL,
>> + ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
>> + isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
>> + }
>> + return IRQ_HANDLED;
>> +}
>> +
>> +/* ISI interrupt service routine */
>> +static irqreturn_t isi_interrupt(int irq, void *dev_id)
>> +{
>> + struct atmel_isi *isi = dev_id;
>> + u32 status, mask, pending;
>> + irqreturn_t ret = IRQ_NONE;
>> +
>> + spin_lock(&isi->lock);
>> +
>> + status = isi_readl(isi, ISI_STATUS);
>> + mask = isi_readl(isi, ISI_INTMASK);
>> + pending = status & mask;
>> +
>> + if (pending & ISI_CTRL_SRST) {
>> + complete(&isi->complete);
>> + isi_writel(isi, ISI_INTDIS, ISI_CTRL_SRST);
>> + ret = IRQ_HANDLED;
>> + } else if (pending & ISI_CTRL_DIS) {
>> + complete(&isi->complete);
>> + isi_writel(isi, ISI_INTDIS, ISI_CTRL_DIS);
>> + ret = IRQ_HANDLED;
>> + } else {
>> + if ((pending & ISI_SR_VSYNC) &&
>> + (isi->state == ISI_STATE_IDLE)) {

> Superfluous second pair of parenthesis

I think using both pair of parenthesis will make code clearer. Can you accept this?

>> + isi->state = ISI_STATE_READY;
>> + wake_up_interruptible(&isi->vsync_wq);
>> + ret = IRQ_HANDLED;
>> + }
>> + if (likely(pending & ISI_SR_CXFR_DONE))
>> + ret = atmel_isi_handle_streaming(isi);
>> + }
>> +
>> + spin_unlock(&isi->lock);
>> + return ret;
>> +}
>> +
>> +#define WAIT_ISI_RESET 1
>> +#define WAIT_ISI_DISABLE 0
>> +static int atmel_isi_wait_status(struct atmel_isi *isi, int wait_reset)
>> +{
>> + unsigned long timeout;
>> + /*
>> + * The reset or disable will only succeed if we have a
>> + * pixel clock from the camera.
>> + */
>> + init_completion(&isi->complete);
>> +
>> + if (wait_reset) {
>> + isi_writel(isi, ISI_INTEN, ISI_CTRL_SRST);
>> + isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
>> + } else {
>> + isi_writel(isi, ISI_INTEN, ISI_CTRL_DIS);
>> + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> + }
>> +
>> + timeout = wait_for_completion_timeout(&isi->complete,
>> + msecs_to_jiffies(100));
>> + if (timeout == 0)
>> + return -ETIMEDOUT;
>> +
>> + return 0;
>> +}
>> +
>> +/* ------------------------------------------------------------------
>> + Videobuf operations
>> + ------------------------------------------------------------------*/
>> +static int queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>> + unsigned int *nplanes, unsigned long sizes[],
>> + void *alloc_ctxs[])
>> +{
>> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> + unsigned long size;
>> + int ret, bytes_per_line;
>> +
>> + /* Reset ISI */
>> + ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
>> + if (ret < 0) {
>> + dev_err(icd->dev.parent, "Reset ISI timed out\n");
>> + return ret;
>> + }
>> + /* Disable all interrupts */
>> + isi_writel(isi, ISI_INTDIS, ~0UL);
>> +
>> + bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
>> + icd->current_fmt->host_fmt);
>> +
>> + if (bytes_per_line < 0)
>> + return bytes_per_line;
>> +
>> + size = bytes_per_line * icd->user_height;
>> +
>> + if (*nbuffers == 0)
>> + *nbuffers = MAX_BUFFER_NUM;
>> + else if (*nbuffers > MAX_BUFFER_NUM)
>> + *nbuffers = MAX_BUFFER_NUM;

> /me wondering, why not just
>
> if (!*nbuffers || *nbuffers > MAX_BUFFER_NUM)
> *nbuffers = MAX_BUFFER_NUM;

Will fixed in v4.

>> +
>> + if (size * *nbuffers > VID_LIMIT_BYTES)
>> + *nbuffers = VID_LIMIT_BYTES / size;
>> +
>> + *nplanes = 1;
>> + sizes[0] = size;
>> + alloc_ctxs[0] = isi->alloc_ctx;
>> +
>> + isi->sequence = 0;
>> + isi->active = NULL;
>> +
>> + dev_dbg(icd->dev.parent, "%s, count=%d, size=%ld\n", __func__,
>> + *nbuffers, size);
>> +
>> + return 0;
>> +}
>> +
>> +static int buffer_init(struct vb2_buffer *vb)
>> +{
>> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
>> +
>> + buf->p_fb_desc = NULL;
>> + memset(&buf->fb_desc_phys, 0, sizeof(buf->fb_desc_phys));

> I don't think this is what the reviewer meant - here you're using memset
> to nullify a u32 field. He probably was asking you to clean the whole
> struct frame_buffer, which I actually don't necessarily agree with. So,
> unless there's an evidence, why that memest is needed, I'd just keep your
> original "->fb_desc_phys = 0". Notice, none of the soc-camera drivers
> nullifies anything in .buf_init(), it cannot also be mandatory, because
> the actual method is optional.

Well, in the v4 version, only following code are needed in this function.
buf->p_dma_desc = NULL;
INIT_LIST_HEAD(&buf->list);

>> + INIT_LIST_HEAD(&buf->list);
>> +
>> + return 0;
>> +}
>> +
>> +static int buffer_prepare(struct vb2_buffer *vb)
>> +{
>> + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
>> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> + unsigned long size;
>> + struct isi_dma_desc *desc;
>> + int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
>> + icd->current_fmt->host_fmt);
>> +
>> + if (bytes_per_line < 0)
>> + return bytes_per_line;
>> +
>> + size = bytes_per_line * icd->user_height;
>> +
>> + if (vb2_plane_size(vb, 0) < size) {
>> + dev_err(icd->dev.parent, "%s data will not fit into plane (%lu < %lu)\n",
>> + __func__, vb2_plane_size(vb, 0), size);
>> + return -EINVAL;
>> + }
>> +
>> + vb2_set_plane_payload(&buf->vb, 0, size);
>> +
>> + if (!buf->p_fb_desc) {
>> + if (list_empty(&isi->dma_desc_head)) {
>> + dev_err(icd->dev.parent, "Not enough dma descriptors.\n");
>> + return -EINVAL;
>> + } else {
>> + /* Get an available descriptor */
>> + desc = list_entry(isi->dma_desc_head.next,
>> + struct isi_dma_desc, list);
>> + buf->p_fb_desc = desc->p_fbd;
>> + buf->fb_desc_phys = desc->fbd_phys;
>> +
>> + /* Initialize the dma descriptor */
>> + buf->p_fb_desc->fb_address =
>> + vb2_dma_contig_plane_paddr(vb, 0);
>> + buf->p_fb_desc->next_fbd_address = 0;
>> + set_dma_ctrl(buf->p_fb_desc, ISI_DMA_CTRL_WB);
>> +
>> + /* Delete the descriptor since now it is used */
>> + list_del_init(&desc->list);

> It probably doesn't matter here, if there were a race, you'd need to
> protect anyway, but stylistically you usually first grab ownership of an
> object, and only then you modify it, so, it would be more logical to first
> list_del(init) the buffer. As for locking as such - we just rely on v4l2
> file-operation mutex locking for now.

I will fix it in v4.

>> + }
>> + }
>> + return 0;
>> +}
>> +
>> +static void buffer_cleanup(struct vb2_buffer *vb)
>> +{
>> + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
>> + unsigned long flags;
>> + int i;
>> +
>> + spin_lock_irqsave(&isi->lock, flags);

> You say, this lock protects data, shared with the ISR, these are the
> buffer list and the ->state, none of them is accessed below.

I think I can remove the spin_lock here.

>> + if (buf->p_fb_desc) {
>> + /* This descriptor is available now and we add to head list */
>> + for (i = 0; i < MAX_BUFFER_NUM; i++)
>> + if (isi->dma_desc[i].p_fbd == buf->p_fb_desc) {
>> + list_add(&isi->dma_desc[i].list,
>> + &isi->dma_desc_head);
>> + break;
>> + }

> With modified struct frame_buffer the above loop will go.

sure.

>> +
>> + buf->p_fb_desc = NULL;
>> + memset(&buf->fb_desc_phys, 0, sizeof(buf->fb_desc_phys));

> You really need to do clean up in .buf_cleanup() and in .buf_init()? I
> think, one of them should be enough. And memset is bogus here too.

This initialization code is only needed in buffer_init(). I will remove such code in buffer_cleanup().

>> + }
>> + spin_unlock_irqrestore(&isi->lock, flags);
>> +}
>> +
>> +static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
>> +{
>> + u32 ctrl, cfg1;
>> +
>> + cfg1 = isi_readl(isi, ISI_CFG1);
>> + /* Enable irq: cxfr for the codec path, pxfr for the preview path */
>> + isi_writel(isi, ISI_INTEN,
>> + ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
>> +
>> + /* Check if already in a frame */
>> + if (isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) {
>> + dev_err(isi->icd->dev.parent, "Already in frame handling.\n");
>> + return;
>> + }
>> +
>> + isi_writel(isi, ISI_DMA_C_DSCR, buffer->fb_desc_phys);
>> + isi_writel(isi, ISI_DMA_C_CTRL, ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
>> + isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
>> +
>> + /* Enable linked list */
>> + cfg1 |= isi->pdata->frate | ISI_CFG1_DISCR;
>> +
>> + /* Enable codec path and ISI */
>> + ctrl = ISI_CTRL_CDC | ISI_CTRL_EN;
>> + isi_writel(isi, ISI_CTRL, ctrl);
>> + isi_writel(isi, ISI_CFG1, cfg1);
>> +}
>> +
>> +static void buffer_queue(struct vb2_buffer *vb)
>> +{
>> + struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> + struct frame_buffer *buf = container_of(vb, struct frame_buffer, vb);
>> + unsigned long flags = 0;
>> +
>> + spin_lock_irqsave(&isi->lock, flags);
>> + list_add_tail(&buf->list, &isi->video_buffer_list);
>> +
>> + if (isi->active == NULL) {
>> + isi->active = buf;
>> + start_dma(isi, buf);
>> + }
>> + spin_unlock_irqrestore(&isi->lock, flags);
>> +}
>> +
>> +static int start_streaming(struct vb2_queue *vq)
>> +{
>> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> +
>> + u32 sr = 0;
>> + int ret;
>> +
>> + spin_lock_irq(&isi->lock);
>> + isi->state = ISI_STATE_IDLE;
>> +
>> + /* Clear any pending SOF interrupt */
>> + sr = isi_readl(isi, ISI_STATUS);
>> + /* Enable VSYNC interrupt for SOF */
>> + isi_writel(isi, ISI_INTEN, ISI_SR_VSYNC);
>> + isi_writel(isi, ISI_CTRL, ISI_CTRL_EN);
>> +
>> + spin_unlock_irq(&isi->lock);
>> + dev_dbg(icd->dev.parent, "Waiting for SOF\n");
>> + ret = wait_event_interruptible(isi->vsync_wq,
>> + isi->state != ISI_STATE_IDLE);
>> + if (ret)
>> + return ret;
>> +
>> + if (isi->state != ISI_STATE_READY)
>> + return -EIO;
>> +
>> + spin_lock_irq(&isi->lock);
>> + isi->state = ISI_STATE_WAIT_SOF;
>> + isi_writel(isi, ISI_INTDIS, ISI_SR_VSYNC);
>> + spin_unlock_irq(&isi->lock);
>> +
>> + return 0;
>> +}
>> +
>> +/* abort streaming and wait for last buffer */
>> +static int stop_streaming(struct vb2_queue *vq)
>> +{
>> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> + struct frame_buffer *buf, *node;
>> + int ret = 0;
>> + unsigned long timeout;
>> +
>> + spin_lock_irq(&isi->lock);
>> + isi->active = NULL;
>> + /* Release all active buffers */
>> + list_for_each_entry_safe(buf, node, &isi->video_buffer_list, list) {
>> + list_del_init(&buf->list);
>> + vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
>> + }
>> + spin_unlock_irq(&isi->lock);
>> +
>> + timeout = jiffies + FRAME_INTERVAL_MILLI_SEC * HZ;
>> + /* Wait until the end of the current frame. */
>> + while ((isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) &&
>> + time_before(jiffies, timeout))
>> + msleep(1);
>> +
>> + if (time_after(jiffies, timeout)) {
>> + dev_err(icd->dev.parent,
>> + "Timeout waiting for finishing codec request\n");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + /* Disable interrupts */
>> + isi_writel(isi, ISI_INTDIS,
>> + ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
>> +
>> + /* Disable ISI and wait for it is done */
>> + ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
>> + if (ret < 0)
>> + dev_err(icd->dev.parent, "Disable ISI timed out\n");
>> +
>> + return ret;
>> +}
>> +
>> +static struct vb2_ops isi_video_qops = {
>> + .queue_setup = queue_setup,
>> + .buf_init = buffer_init,
>> + .buf_prepare = buffer_prepare,
>> + .buf_cleanup = buffer_cleanup,
>> + .buf_queue = buffer_queue,
>> + .start_streaming = start_streaming,
>> + .stop_streaming = stop_streaming,
>> + .wait_prepare = soc_camera_unlock,
>> + .wait_finish = soc_camera_lock,
>> +};
>> +
>> +/* ------------------------------------------------------------------
>> + SOC camera operations for the device
>> + ------------------------------------------------------------------*/
>> +static int isi_camera_init_videobuf(struct vb2_queue *q,
>> + struct soc_camera_device *icd)
>> +{
>> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> + q->io_modes = VB2_MMAP;
>> + q->drv_priv = icd;
>> + q->buf_struct_size = sizeof(struct frame_buffer);
>> + q->ops = &isi_video_qops;
>> + q->mem_ops = &vb2_dma_contig_memops;
>> +
>> + return vb2_queue_init(q);
>> +}
>> +
>> +static int isi_camera_set_fmt(struct soc_camera_device *icd,
>> + struct v4l2_format *f)
>> +{
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> + const struct soc_camera_format_xlate *xlate;
>> + struct v4l2_pix_format *pix = &f->fmt.pix;
>> + struct v4l2_mbus_framefmt mf;
>> + int ret;
>> +
>> + xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
>> + if (!xlate) {
>> + dev_warn(icd->dev.parent, "Format %x not found\n",
>> + pix->pixelformat);
>> + return -EINVAL;
>> + }
>> +
>> + dev_dbg(icd->dev.parent, "Plan to set format %dx%d\n",
>> + pix->width, pix->height);
>> +
>> + mf.width = pix->width;
>> + mf.height = pix->height;
>> + mf.field = pix->field;
>> + mf.colorspace = pix->colorspace;
>> + mf.code = xlate->code;
>> +
>> + ret = v4l2_subdev_call(sd, video, s_mbus_fmt, &mf);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (mf.code != xlate->code)
>> + return -EINVAL;
>> +
>> + ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
>> + if (ret < 0)
>> + return ret;
>> +
>> + pix->width = mf.width;
>> + pix->height = mf.height;
>> + pix->field = mf.field;
>> + pix->colorspace = mf.colorspace;
>> + icd->current_fmt = xlate;
>> +
>> + dev_dbg(icd->dev.parent, "Finally set format %dx%d\n",
>> + pix->width, pix->height);
>> +
>> + return ret;
>> +}
>> +
>> +static int isi_camera_try_fmt(struct soc_camera_device *icd,
>> + struct v4l2_format *f)
>> +{
>> + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> + const struct soc_camera_format_xlate *xlate;
>> + struct v4l2_pix_format *pix = &f->fmt.pix;
>> + struct v4l2_mbus_framefmt mf;
>> + u32 pixfmt = pix->pixelformat;
>> + int ret;
>> +
>> + xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
>> + if (pixfmt && !xlate) {
>> + dev_warn(icd->dev.parent, "Format %x not found\n", pixfmt);
>> + return -EINVAL;
>> + }
>> +
>> + /* limit to Atmel ISI hardware capabilities */
>> + if (pix->height > MAX_SUPPORT_HEIGHT)
>> + pix->height = MAX_SUPPORT_HEIGHT;
>> + if (pix->width > MAX_SUPPORT_WIDTH)
>> + pix->width = MAX_SUPPORT_WIDTH;
>> +
>> + /* limit to sensor capabilities */
>> + mf.width = pix->width;
>> + mf.height = pix->height;
>> + mf.field = pix->field;
>> + mf.colorspace = pix->colorspace;
>> + mf.code = xlate->code;
>> +
>> + ret = v4l2_subdev_call(sd, video, try_mbus_fmt, &mf);
>> + if (ret < 0)
>> + return ret;
>> +
>> + pix->width = mf.width;
>> + pix->height = mf.height;
>> + pix->colorspace = mf.colorspace;
>> +
>> + switch (mf.field) {
>> + case V4L2_FIELD_ANY:
>> + pix->field = V4L2_FIELD_NONE;
>> + break;
>> + case V4L2_FIELD_NONE:
>> + break;
>> + default:
>> + dev_err(icd->dev.parent, "Field type %d unsupported.\n",
>> + mf.field);
>> + ret = -EINVAL;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static const struct soc_mbus_pixelfmt isi_camera_formats[] = {
>> + {
>> + .fourcc = V4L2_PIX_FMT_YUYV,
>> + .name = "Packed YUV422 16 bit",
>> + .bits_per_sample = 8,
>> + .packing = SOC_MBUS_PACKING_2X8_PADHI,
>> + .order = SOC_MBUS_ORDER_LE,
>> + },
>> +};
>> +
>> +/* This will be corrected as we get more formats */
>> +static bool isi_camera_packing_supported(const struct soc_mbus_pixelfmt *fmt)
>> +{
>> + return fmt->packing == SOC_MBUS_PACKING_NONE ||
>> + (fmt->bits_per_sample == 8 &&
>> + fmt->packing == SOC_MBUS_PACKING_2X8_PADHI) ||
>> + (fmt->bits_per_sample > 8 &&
>> + fmt->packing == SOC_MBUS_PACKING_EXTEND16);
>> +}
>> +
>> +static unsigned long make_bus_param(struct atmel_isi *isi)
>> +{
>> + unsigned long flags;
>> + /*
>> + * Platform specified synchronization and pixel clock polarities are
>> + * only a recommendation and are only used during probing. Atmel ISI
>> + * camera interface only works in master mode, i.e., uses HSYNC and
>> + * VSYNC signals from the sensor
>> + */
>> + flags = SOCAM_MASTER |
>> + SOCAM_HSYNC_ACTIVE_HIGH |
>> + SOCAM_HSYNC_ACTIVE_LOW |
>> + SOCAM_VSYNC_ACTIVE_HIGH |
>> + SOCAM_VSYNC_ACTIVE_LOW |
>> + SOCAM_PCLK_SAMPLE_RISING |
>> + SOCAM_PCLK_SAMPLE_FALLING |
>> + SOCAM_DATA_ACTIVE_HIGH;
>> +
>> + if (isi->pdata->data_width_flags & ISI_DATAWIDTH_10)
>> + flags |= SOCAM_DATAWIDTH_10;
>> +
>> + if (isi->pdata->data_width_flags & ISI_DATAWIDTH_8)
>> + flags |= SOCAM_DATAWIDTH_8;
>> +
>> + if (flags & SOCAM_DATAWIDTH_MASK)
>> + return flags;
>> +
>> + return 0;
>> +}
>> +
>> +static int isi_camera_try_bus_param(struct soc_camera_device *icd,
>> + unsigned char buswidth)
>> +{
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> + unsigned long camera_flags;
>> + int ret;
>> +
>> + camera_flags = icd->ops->query_bus_param(icd);
>> + ret = soc_camera_bus_param_compatible(camera_flags,
>> + make_bus_param(isi));
>> + if (!ret)
>> + return -EINVAL;
>> + return 0;
>> +}
>> +
>> +
>> +static int isi_camera_get_formats(struct soc_camera_device *icd,
>> + unsigned int idx,
>> + struct soc_camera_format_xlate *xlate)
>> +{
>> + struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> + int formats = 0, ret;
>> + /* sensor format */
>> + enum v4l2_mbus_pixelcode code;
>> + /* soc camera host format */
>> + const struct soc_mbus_pixelfmt *fmt;
>> +
>> + ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, idx, &code);
>> + if (ret < 0)
>> + /* No more formats */
>> + return 0;
>> +
>> + fmt = soc_mbus_get_fmtdesc(code);
>> + if (!fmt) {
>> + dev_err(icd->dev.parent,
>> + "Invalid format code #%u: %d\n", idx, code);
>> + return 0;
>> + }
>> +
>> + /* This also checks support for the requested bits-per-sample */
>> + ret = isi_camera_try_bus_param(icd, fmt->bits_per_sample);
>> + if (ret < 0) {
>> + dev_err(icd->dev.parent,
>> + "Fail to try the bus parameters.\n");
>> + return 0;
>> + }
>> +
>> + switch (code) {
>> + case V4L2_MBUS_FMT_UYVY8_2X8:
>> + case V4L2_MBUS_FMT_VYUY8_2X8:
>> + case V4L2_MBUS_FMT_YUYV8_2X8:
>> + case V4L2_MBUS_FMT_YVYU8_2X8:
>> + formats++;
>> + if (xlate) {
>> + xlate->host_fmt = &isi_camera_formats[0];
>> + xlate->code = code;
>> + xlate++;
>> + dev_dbg(icd->dev.parent, "Providing format %s using code %d\n",
>> + isi_camera_formats[0].name, code);
>> + }
>> + break;
>> + default:
>> + if (!isi_camera_packing_supported(fmt))
>> + return 0;
>> + if (xlate)
>> + dev_dbg(icd->dev.parent,
>> + "Providing format %s in pass-through mode\n",
>> + fmt->name);
>> + }
>> +
>> + /* Generic pass-through */
>> + formats++;
>> + if (xlate) {
>> + xlate->host_fmt = fmt;
>> + xlate->code = code;
>> + xlate++;
>> + }
>> +
>> + return formats;
>> +}
>> +
>> +/* Called with .video_lock held */
>> +static int isi_camera_add_device(struct soc_camera_device *icd)
>> +{
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> + int ret;
>> +
>> + if (isi->icd)
>> + return -EBUSY;
>> +
>> + ret = clk_enable(isi->pclk);
>> + if (ret)
>> + return ret;
>> +
>> + isi->icd = icd;
>> + dev_dbg(icd->dev.parent, "Atmel ISI Camera driver attached to camera %d\n",
>> + icd->devnum);
>> + return 0;
>> +}
>> +/* Called with .video_lock held */
>> +static void isi_camera_remove_device(struct soc_camera_device *icd)
>> +{
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> +
>> + BUG_ON(icd != isi->icd);
>> +
>> + clk_disable(isi->pclk);
>> + isi->icd = NULL;
>> +
>> + dev_dbg(icd->dev.parent, "Atmel ISI Camera driver detached from camera %d\n",
>> + icd->devnum);
>> +}
>> +
>> +static unsigned int isi_camera_poll(struct file *file, poll_table *pt)
>> +{
>> + struct soc_camera_device *icd = file->private_data;
>> +
>> + return vb2_poll(&icd->vb2_vidq, file, pt);
>> +}
>> +
>> +static int isi_camera_querycap(struct soc_camera_host *ici,
>> + struct v4l2_capability *cap)
>> +{
>> + strcpy(cap->driver, "atmel-isi");
>> + strcpy(cap->card, "Atmel Image Sensor Interface");
>> + cap->capabilities = (V4L2_CAP_VIDEO_CAPTURE |
>> + V4L2_CAP_STREAMING);
>> + return 0;
>> +}
>> +
>> +static int isi_camera_set_bus_param(struct soc_camera_device *icd, u32 pixfmt)
>> +{
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> + unsigned long bus_flags, camera_flags, common_flags;
>> + int ret;
>> + u32 cfg1 = 0;
>> +
>> + camera_flags = icd->ops->query_bus_param(icd);
>> +
>> + bus_flags = make_bus_param(isi);
>> + common_flags = soc_camera_bus_param_compatible(camera_flags, bus_flags);
>> + dev_dbg(icd->dev.parent, "Flags cam: 0x%lx host: 0x%lx common: 0x%lx\n",
>> + camera_flags, bus_flags, common_flags);
>> + if (!common_flags)
>> + return -EINVAL;
>> +
>> + /* Make choises, based on platform preferences */
>> + if ((common_flags & SOCAM_HSYNC_ACTIVE_HIGH) &&
>> + (common_flags & SOCAM_HSYNC_ACTIVE_LOW)) {
>> + if (isi->pdata->hsync_act_low)
>> + common_flags &= ~SOCAM_HSYNC_ACTIVE_HIGH;
>> + else
>> + common_flags &= ~SOCAM_HSYNC_ACTIVE_LOW;
>> + }
>> +
>> + if ((common_flags & SOCAM_VSYNC_ACTIVE_HIGH) &&
>> + (common_flags & SOCAM_VSYNC_ACTIVE_LOW)) {
>> + if (isi->pdata->vsync_act_low)
>> + common_flags &= ~SOCAM_VSYNC_ACTIVE_HIGH;
>> + else
>> + common_flags &= ~SOCAM_VSYNC_ACTIVE_LOW;
>> + }
>> +
>> + if ((common_flags & SOCAM_PCLK_SAMPLE_RISING) &&
>> + (common_flags & SOCAM_PCLK_SAMPLE_FALLING)) {
>> + if (isi->pdata->pclk_act_falling)
>> + common_flags &= ~SOCAM_PCLK_SAMPLE_RISING;
>> + else
>> + common_flags &= ~SOCAM_PCLK_SAMPLE_FALLING;
>> + }
>> +
>> + ret = icd->ops->set_bus_param(icd, common_flags);
>> + if (ret < 0) {
>> + dev_dbg(icd->dev.parent, "Camera set_bus_param(%lx) returned %d\n",
>> + common_flags, ret);
>> + return ret;
>> + }
>> +
>> + /* set bus param for ISI */
>> + if (common_flags & SOCAM_HSYNC_ACTIVE_LOW)
>> + cfg1 |= ISI_CFG1_HSYNC_POL_ACTIVE_LOW;
>> + if (common_flags & SOCAM_VSYNC_ACTIVE_LOW)
>> + cfg1 |= ISI_CFG1_VSYNC_POL_ACTIVE_LOW;
>> + if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
>> + cfg1 |= ISI_CFG1_PIXCLK_POL_ACTIVE_FALLING;
>> +
>> + if (isi->pdata->has_emb_sync)
>> + cfg1 |= ISI_CFG1_EMB_SYNC;
>> + if (isi->pdata->isi_full_mode)
>> + cfg1 |= ISI_CFG1_FULL_MODE;
>> +
>> + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> + isi_writel(isi, ISI_CFG1, cfg1);
>> +
>> + return 0;
>> +}
>> +
>> +static struct soc_camera_host_ops isi_soc_camera_host_ops = {
>> + .owner = THIS_MODULE,
>> + .add = isi_camera_add_device,
>> + .remove = isi_camera_remove_device,
>> + .set_fmt = isi_camera_set_fmt,
>> + .try_fmt = isi_camera_try_fmt,
>> + .get_formats = isi_camera_get_formats,
>> + .init_videobuf2 = isi_camera_init_videobuf,
>> + .poll = isi_camera_poll,
>> + .querycap = isi_camera_querycap,
>> + .set_bus_param = isi_camera_set_bus_param,
>> +};
>> +
>> +/* -----------------------------------------------------------------------*/
>> +static int __devexit atmel_isi_remove(struct platform_device *pdev)
>> +{
>> + struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
>> + struct atmel_isi *isi = container_of(soc_host,
>> + struct atmel_isi, soc_host);
>> +

> Wouldn't it be better to first free IRQs to prevent any possible
> interrupts?

I'll move it ahead.

>> + soc_camera_host_unregister(soc_host);
>> + vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
>> + dma_free_coherent(&pdev->dev,
>> + sizeof(struct fbd) * MAX_BUFFER_NUM,
>> + isi->p_fb_descriptors,
>> + isi->fb_descriptors_phys);
>> +
>> + free_irq(isi->irq, isi);
>> + iounmap(isi->regs);
>> + clk_put(isi->pclk);
>> + kfree(isi);
>> +
>> + return 0;
>> +}
>> +
>> +static int __devinit atmel_isi_probe(struct platform_device *pdev)
>> +{
>> + unsigned int irq;
>> + struct atmel_isi *isi;
>> + struct clk *pclk;
>> + struct resource *regs;
>> + int ret, i;
>> + struct device *dev = &pdev->dev;
>> + struct soc_camera_host *soc_host;
>> +
>> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!regs)
>> + return -ENXIO;
>> +
>> + pclk = clk_get(&pdev->dev, "isi_clk");
>> + if (IS_ERR(pclk))
>> + return PTR_ERR(pclk);
>> +
>> + isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
>> + if (!isi) {
>> + ret = -ENOMEM;
>> + dev_err(&pdev->dev, "Can't allocate interface!\n");
>> + goto err_alloc_isi;
>> + }
>> +
>> + isi->pclk = pclk;
>> +
>> + spin_lock_init(&isi->lock);
>> + init_waitqueue_head(&isi->vsync_wq);
>> +
>> + isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
>> + sizeof(struct fbd) * MAX_BUFFER_NUM,
>> + &isi->fb_descriptors_phys,
>> + GFP_KERNEL);
>> + if (!isi->p_fb_descriptors) {
>> + ret = -ENOMEM;
>> + dev_err(&pdev->dev, "Can't allocate descriptors!\n");
>> + goto err_alloc_descriptors;
>> + }
>> +
>> + INIT_LIST_HEAD(&isi->dma_desc_head);
>> + for (i = 0; i < MAX_BUFFER_NUM; i++) {
>> + isi->dma_desc[i].p_fbd = isi->p_fb_descriptors + i;
>> + isi->dma_desc[i].fbd_phys = isi->fb_descriptors_phys +
>> + i * sizeof(struct fbd);
>> + list_add(&isi->dma_desc[i].list, &isi->dma_desc_head);
>> + }
>> +
>> + isi->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
>> + if (IS_ERR(isi->alloc_ctx)) {
>> + ret = PTR_ERR(isi->alloc_ctx);
>> + goto err_alloc_ctx;
>> + }
>> +
>> + isi->regs = ioremap(regs->start, resource_size(regs));
>> + if (!isi->regs) {
>> + ret = -ENOMEM;
>> + goto err_ioremap;
>> + }
>> +
>> + if (!dev->platform_data) {

> I would do this check right in the beginning of this function to simplify
> error handling.

yes, indeed simplify the error handling code a lot. I'll change it.

>> + dev_err(&pdev->dev,
>> + "No config available for Atmel ISI\n");
>> + ret = -EINVAL;
>> + goto err_req_irq;
>> + }
>> +
>> + isi->pdata = dev->platform_data;
>> + if (isi->pdata->data_width_flags == 0) {

> And this check too. You can even unite them into one like

>+ if (!dev->platform_data || !dev->platform_data->data_width_flags) {
>+ ...
>+ }

I will change this.

> Thanks
> Guennadi

Thank you.

>> [snip]

Best Regards,
Josh Wu