[PATCH 00/13] media: atmel-isi: extract the hw releated functions into structure

[PATCH 00/13] media: atmel-isi: extract the hw releated functions into structure

[Josh Wu]

This series refactor the atmel-isi drvier. In the meantime, extract all
the hardware related functions, and made it as function table. Also add
some hardware data.

All those hardware functions, datas are defined with the compatible
string.

In this way, it is easy to add another compatible string for new
hardware support.


Josh Wu (13):
  atmel-isi: use try_or_set_fmt() for both set_fmt() and try_fmt()
  atmel-isi: move the is_support() close to try/set format function
  atmel-isi: add isi_hw_initialize() function to handle hw setup
  atmel-isi: move the cfg1 initialize to isi_hw_initialize()
  atmel-isi: add a function: isi_hw_wait_status() to check ISI_SR status
  atmel-isi: check ISI_SR's flags by polling instead of interrupt
  atmel-isi: move hw code into isi_hw_initialize()
  atmel-isi: remove the function set_dma_ctrl() as it just use once
  atmel-isi: add a function start_isi()
  atmel-isi: reuse start_dma() function in isi interrupt handler
  atmel-isi: add hw_uninitialize() in stop_streaming()
  atmel-isi: use union for the fbd (frame buffer descriptor)
  atmel-isi: use an hw_data structure according compatible string

 drivers/media/platform/soc_camera/atmel-isi.c | 529 ++++++++++++++------------
 1 file changed, 277 insertions(+), 252 deletions(-)

-- 
1.9.1

[PATCH 01/13] atmel-isi: use try_or_set_fmt() for both set_fmt() and try_fmt()

[Josh Wu]

From: Josh Wu <josh.wu@atmel.com>

Since atmel-isi has similar set_fmt() and try_fmt() functions. So this
patch will add a new function which can be called by set_fmt() and
try_fmt().

That can increase the reusability.

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 105 ++++++++++----------------
 1 file changed, 41 insertions(+), 64 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index c398b28..dc81df3 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -571,16 +571,16 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
 	return vb2_queue_init(q);
 }
 
-static int isi_camera_set_fmt(struct soc_camera_device *icd,
-			      struct v4l2_format *f)
+static int try_or_set_fmt(struct soc_camera_device *icd,
+		   struct v4l2_format *f,
+		   struct v4l2_subdev_format *format)
 {
-	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_subdev_format format = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-	};
-	struct v4l2_mbus_framefmt *mf = &format.format;
+	const struct soc_camera_format_xlate *xlate;
+	struct v4l2_subdev_pad_config pad_cfg;
+
+	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
+	struct v4l2_mbus_framefmt *mf = &format->format;
 	int ret;
 
 	/* check with atmel-isi support format, if not support use YUYV */
@@ -594,8 +594,11 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
 		return -EINVAL;
 	}
 
-	dev_dbg(icd->parent, "Plan to set format %dx%d\n",
-			pix->width, pix->height);
+	/* 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;
 
 	mf->width	= pix->width;
 	mf->height	= pix->height;
@@ -603,7 +606,11 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
 	mf->colorspace	= pix->colorspace;
 	mf->code	= xlate->code;
 
-	ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, &format);
+	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, format);
+	else
+		ret = v4l2_subdev_call(sd, pad, set_fmt, &pad_cfg, format);
+
 	if (ret < 0)
 		return ret;
 
@@ -614,64 +621,14 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
 	pix->height		= mf->height;
 	pix->field		= mf->field;
 	pix->colorspace		= mf->colorspace;
-	icd->current_fmt	= xlate;
-
-	dev_dbg(icd->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_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_format format = {
-		.which = V4L2_SUBDEV_FORMAT_TRY,
-	};
-	struct v4l2_mbus_framefmt *mf = &format.format;
-	u32 pixfmt = pix->pixelformat;
-	int ret;
-
-	/* check with atmel-isi support format, if not support use YUYV */
-	if (!is_supported(icd, pix->pixelformat))
-		pix->pixelformat = V4L2_PIX_FMT_YUYV;
-
-	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
-	if (pixfmt && !xlate) {
-		dev_warn(icd->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, pad, set_fmt, &pad_cfg, &format);
-	if (ret < 0)
-		return ret;
-
-	pix->width	= mf->width;
-	pix->height	= mf->height;
-	pix->colorspace	= mf->colorspace;
+	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		icd->current_fmt = xlate;
 
 	switch (mf->field) {
 	case V4L2_FIELD_ANY:
-		pix->field = V4L2_FIELD_NONE;
-		break;
 	case V4L2_FIELD_NONE:
+		pix->field = V4L2_FIELD_NONE;
 		break;
 	default:
 		dev_err(icd->parent, "Field type %d unsupported.\n",
@@ -682,6 +639,26 @@ static int isi_camera_try_fmt(struct soc_camera_device *icd,
 	return ret;
 }
 
+static int isi_camera_set_fmt(struct soc_camera_device *icd,
+			      struct v4l2_format *f)
+{
+	struct v4l2_subdev_format format = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+
+	return try_or_set_fmt(icd, f, &format);
+}
+
+static int isi_camera_try_fmt(struct soc_camera_device *icd,
+			      struct v4l2_format *f)
+{
+	struct v4l2_subdev_format format = {
+		.which = V4L2_SUBDEV_FORMAT_TRY,
+	};
+
+	return try_or_set_fmt(icd, f, &format);
+}
+
 static const struct soc_mbus_pixelfmt isi_camera_formats[] = {
 	{
 		.fourcc			= V4L2_PIX_FMT_YUYV,
-- 
1.9.1

[PATCH 02/13] atmel-isi: move the is_support() close to try/set format function

[Josh Wu]

As is_support() only used by try/set format function so put them
together.

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 36 +++++++++++++--------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index dc81df3..3793b68 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -189,24 +189,6 @@ static void configure_geometry(struct atmel_isi *isi, u32 width,
 	return;
 }
 
-static bool is_supported(struct soc_camera_device *icd,
-		const u32 pixformat)
-{
-	switch (pixformat) {
-	/* YUV, including grey */
-	case V4L2_PIX_FMT_GREY:
-	case V4L2_PIX_FMT_YUYV:
-	case V4L2_PIX_FMT_UYVY:
-	case V4L2_PIX_FMT_YVYU:
-	case V4L2_PIX_FMT_VYUY:
-	/* RGB */
-	case V4L2_PIX_FMT_RGB565:
-		return true;
-	default:
-		return false;
-	}
-}
-
 static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
 {
 	if (isi->active) {
@@ -571,6 +553,24 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
 	return vb2_queue_init(q);
 }
 
+static bool is_supported(struct soc_camera_device *icd,
+		const u32 pixformat)
+{
+	switch (pixformat) {
+	/* YUV, including grey */
+	case V4L2_PIX_FMT_GREY:
+	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_YVYU:
+	case V4L2_PIX_FMT_VYUY:
+	/* RGB */
+	case V4L2_PIX_FMT_RGB565:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int try_or_set_fmt(struct soc_camera_device *icd,
 		   struct v4l2_format *f,
 		   struct v4l2_subdev_format *format)
-- 
1.9.1

[PATCH 03/13] atmel-isi: add isi_hw_initialize() function to handle hw setup

[Josh Wu]

Move the hardware operation to one isi_hw_initialize(). Then we will
call it in start_streaming().

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 51 +++++++++++++++------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 3793b68..0c3cb74 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -88,6 +88,7 @@ struct atmel_isi {
 
 	struct isi_platform_data	pdata;
 	u16				width_flags;	/* max 12 bits */
+	u32				bus_param;
 
 	struct list_head		video_buffer_list;
 	struct frame_buffer		*active;
@@ -189,6 +190,30 @@ static void configure_geometry(struct atmel_isi *isi, u32 width,
 	return;
 }
 
+static void isi_hw_initialize(struct atmel_isi *isi)
+{
+	u32 common_flags = isi->bus_param;
+	u32 cfg1 = 0;
+
+	/* set bus param for ISI */
+	if (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
+		cfg1 |= ISI_CFG1_HSYNC_POL_ACTIVE_LOW;
+	if (common_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+		cfg1 |= ISI_CFG1_VSYNC_POL_ACTIVE_LOW;
+	if (common_flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
+		cfg1 |= ISI_CFG1_PIXCLK_POL_ACTIVE_FALLING;
+
+	if (isi->pdata.has_emb_sync)
+		cfg1 |= ISI_CFG1_EMB_SYNC;
+	if (isi->pdata.full_mode)
+		cfg1 |= ISI_CFG1_FULL_MODE;
+
+	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
+
+	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
+	isi_writel(isi, ISI_CFG1, cfg1);
+}
+
 static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
 {
 	if (isi->active) {
@@ -464,6 +489,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	/* Disable all interrupts */
 	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
 
+	isi_hw_initialize(isi);
+
 	configure_geometry(isi, icd->user_width, icd->user_height,
 				icd->current_fmt);
 
@@ -835,7 +862,6 @@ static int isi_camera_set_bus_param(struct soc_camera_device *icd)
 	struct v4l2_mbus_config cfg = {.type = V4L2_MBUS_PARALLEL,};
 	unsigned long common_flags;
 	int ret;
-	u32 cfg1 = 0;
 
 	ret = v4l2_subdev_call(sd, video, g_mbus_config, &cfg);
 	if (!ret) {
@@ -888,33 +914,12 @@ static int isi_camera_set_bus_param(struct soc_camera_device *icd)
 		return ret;
 	}
 
-	/* set bus param for ISI */
-	if (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
-		cfg1 |= ISI_CFG1_HSYNC_POL_ACTIVE_LOW;
-	if (common_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
-		cfg1 |= ISI_CFG1_VSYNC_POL_ACTIVE_LOW;
-	if (common_flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
-		cfg1 |= ISI_CFG1_PIXCLK_POL_ACTIVE_FALLING;
-
 	dev_dbg(icd->parent, "vsync active %s, hsync active %s, sampling on pix clock %s edge\n",
 		common_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW ? "low" : "high",
 		common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW ? "low" : "high",
 		common_flags & V4L2_MBUS_PCLK_SAMPLE_FALLING ? "falling" : "rising");
 
-	if (isi->pdata.has_emb_sync)
-		cfg1 |= ISI_CFG1_EMB_SYNC;
-	if (isi->pdata.full_mode)
-		cfg1 |= ISI_CFG1_FULL_MODE;
-
-	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
-
-	/* Enable PM and peripheral clock before operate isi registers */
-	pm_runtime_get_sync(ici->v4l2_dev.dev);
-
-	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-	isi_writel(isi, ISI_CFG1, cfg1);
-
-	pm_runtime_put(ici->v4l2_dev.dev);
+	isi->bus_param = common_flags;
 
 	return 0;
 }
-- 
1.9.1

[PATCH 04/13] atmel-isi: move the cfg1 initialize to isi_hw_initialize()

[Josh Wu]

Since cfg1 initialization just about the frame rate, and it hardware
related, just move it to isi_hw_initialize().

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 0c3cb74..4bd8258 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -210,6 +210,10 @@ static void isi_hw_initialize(struct atmel_isi *isi)
 
 	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
 
+	cfg1 |= isi->pdata.frate & ISI_CFG1_FRATE_DIV_MASK;
+
+	cfg1 |= ISI_CFG1_DISCR;
+
 	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
 	isi_writel(isi, ISI_CFG1, cfg1);
 }
@@ -409,9 +413,8 @@ static void buffer_cleanup(struct vb2_buffer *vb)
 
 static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
 {
-	u32 ctrl, cfg1;
+	u32 ctrl;
 
-	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);
@@ -436,10 +439,6 @@ static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
 		isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_P_CH);
 	}
 
-	cfg1 &= ~ISI_CFG1_FRATE_DIV_MASK;
-	/* Enable linked list */
-	cfg1 |= isi->pdata.frate | ISI_CFG1_DISCR;
-
 	/* Enable ISI */
 	ctrl = ISI_CTRL_EN;
 
@@ -447,7 +446,6 @@ static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
 		ctrl |= ISI_CTRL_CDC;
 
 	isi_writel(isi, ISI_CTRL, ctrl);
-	isi_writel(isi, ISI_CFG1, cfg1);
 }
 
 static void buffer_queue(struct vb2_buffer *vb)
-- 
1.9.1

[PATCH 05/13] atmel-isi: add a function: isi_hw_wait_status() to check ISI_SR status

[Josh Wu]

From: Josh Wu <josh.wu@atmel.com>

Extract the code that check ISI_SR flag into a function:
isi_hw_wait_status().

In this patch, we use isi_hw_wait_status() to check CDC pending status.

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 28 ++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 4bd8258..f0508ea 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -190,6 +190,21 @@ static void configure_geometry(struct atmel_isi *isi, u32 width,
 	return;
 }
 
+static int isi_hw_wait_status(struct atmel_isi *isi, int status_flag,
+			       int wait_ms)
+{
+	unsigned long timeout = jiffies + wait_ms * HZ;
+
+	while ((isi_readl(isi, ISI_STATUS) & status_flag) &&
+			time_before(jiffies, timeout))
+		msleep(1);
+
+	if (time_after(jiffies, timeout))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
 static void isi_hw_initialize(struct atmel_isi *isi)
 {
 	u32 common_flags = isi->bus_param;
@@ -511,7 +526,6 @@ static void stop_streaming(struct vb2_queue *vq)
 	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;
@@ -523,15 +537,11 @@ static void stop_streaming(struct vb2_queue *vq)
 	spin_unlock_irq(&isi->lock);
 
 	if (!isi->enable_preview_path) {
-		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->parent,
-				"Timeout waiting for finishing codec request\n");
+		ret = isi_hw_wait_status(isi, ISI_CTRL_CDC,
+					 FRAME_INTERVAL_MILLI_SEC);
+		if (ret)
+			dev_err(icd->parent, "Timeout waiting for finishing codec request\n");
 	}
 
 	/* Disable interrupts */
-- 
1.9.1

[PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt

[Josh Wu]

In current code, we use a interrupt to check whether ISI reset/disable
action is done. Actually, we also can check ISI SR to check the
reset/disable action by polling, and it is simpler and straight forward.

So this patch use isi_hw_wait_status() to check the action status. As
the interrupt checking the status is useless, so just remove the interrupt
& completion code.

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 59 ++++++---------------------
 1 file changed, 13 insertions(+), 46 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index f0508ea..4ddc309 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -12,7 +12,6 @@
  */
 
 #include <linux/clk.h>
-#include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/fs.h>
 #include <linux/init.h>
@@ -81,7 +80,6 @@ struct atmel_isi {
 	struct isi_dma_desc		dma_desc[MAX_BUFFER_NUM];
 	bool				enable_preview_path;
 
-	struct completion		complete;
 	/* ISI peripherial clock */
 	struct clk			*pclk;
 	unsigned int			irq;
@@ -281,51 +279,14 @@ static irqreturn_t isi_interrupt(int irq, void *dev_id)
 	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 (likely(pending & ISI_SR_CXFR_DONE) ||
-				likely(pending & ISI_SR_PXFR_DONE))
-			ret = atmel_isi_handle_streaming(isi);
-	}
+	if (likely(pending & ISI_SR_CXFR_DONE) ||
+	    likely(pending & ISI_SR_PXFR_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(500));
-	if (timeout == 0)
-		return -ETIMEDOUT;
-
-	return 0;
-}
-
 /* ------------------------------------------------------------------
 	Videobuf operations
    ------------------------------------------------------------------*/
@@ -493,8 +454,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	pm_runtime_get_sync(ici->v4l2_dev.dev);
 
 	/* Reset ISI */
-	ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
-	if (ret < 0) {
+	isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
+
+	/* Check Reset status */
+	ret  = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
+	if (ret) {
 		dev_err(icd->parent, "Reset ISI timed out\n");
 		pm_runtime_put(ici->v4l2_dev.dev);
 		return ret;
@@ -549,8 +513,11 @@ static void stop_streaming(struct vb2_queue *vq)
 			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)
+	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
+
+	/* Check Reset status */
+	ret  = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
+	if (ret)
 		dev_err(icd->parent, "Disable ISI timed out\n");
 
 	pm_runtime_put(ici->v4l2_dev.dev);
-- 
1.9.1

[PATCH 07/13] atmel-isi: move hw code into isi_hw_initialize()

[Josh Wu]

That make hw operation code separate with general code.

Also since reset action can be failed, so add a return value for
isi_hw_initialze().

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 34 +++++++++++++++++----------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 4ddc309..ed4d04b 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -203,10 +203,27 @@ static int isi_hw_wait_status(struct atmel_isi *isi, int status_flag,
 	return 0;
 }
 
-static void isi_hw_initialize(struct atmel_isi *isi)
+static int isi_hw_initialize(struct atmel_isi *isi)
 {
 	u32 common_flags = isi->bus_param;
 	u32 cfg1 = 0;
+	int ret;
+
+	/* Reset ISI */
+	isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
+
+	/* Check Reset status */
+	ret  = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
+	if (ret) {
+		dev_err(isi->soc_host.icd->parent, "Reset ISI timed out\n");
+		return ret;
+	}
+
+	/* Disable all interrupts */
+	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
+
+	/* Clear any pending interrupt */
+	isi_readl(isi, ISI_STATUS);
 
 	/* set bus param for ISI */
 	if (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
@@ -229,6 +246,8 @@ static void isi_hw_initialize(struct atmel_isi *isi)
 
 	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
 	isi_writel(isi, ISI_CFG1, cfg1);
+
+	return 0;
 }
 
 static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
@@ -453,27 +472,16 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	pm_runtime_get_sync(ici->v4l2_dev.dev);
 
-	/* Reset ISI */
-	isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
-
-	/* Check Reset status */
-	ret  = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
+	ret = isi_hw_initialize(isi);
 	if (ret) {
-		dev_err(icd->parent, "Reset ISI timed out\n");
 		pm_runtime_put(ici->v4l2_dev.dev);
 		return ret;
 	}
-	/* Disable all interrupts */
-	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
-
-	isi_hw_initialize(isi);
 
 	configure_geometry(isi, icd->user_width, icd->user_height,
 				icd->current_fmt);
 
 	spin_lock_irq(&isi->lock);
-	/* Clear any pending interrupt */
-	isi_readl(isi, ISI_STATUS);
 
 	if (count)
 		start_dma(isi, isi->active);
-- 
1.9.1

[PATCH 08/13] atmel-isi: remove the function set_dma_ctrl() as it just use once

[Josh Wu]

From: Josh Wu <josh.wu@atmel.com>

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index ed4d04b..e1ad18f 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -46,11 +46,6 @@ struct 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;
@@ -385,7 +380,7 @@ static int buffer_prepare(struct vb2_buffer *vb)
 			desc->p_fbd->fb_address =
 					vb2_dma_contig_plane_dma_addr(vb, 0);
 			desc->p_fbd->next_fbd_address = 0;
-			set_dma_ctrl(desc->p_fbd, ISI_DMA_CTRL_WB);
+			desc->p_fbd->dma_ctrl = ISI_DMA_CTRL_WB;
 
 			buf->p_dma_desc = desc;
 		}
-- 
1.9.1

[PATCH 09/13] atmel-isi: add a function start_isi()

[Josh Wu]

Since start_dma() has code which is to start isi, so move such code into
a new function: start_isi().

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 32 +++++++++++++++------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index e1ad18f..0e42171 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -245,6 +245,20 @@ static int isi_hw_initialize(struct atmel_isi *isi)
 	return 0;
 }
 
+static void start_isi(struct atmel_isi *isi)
+{
+	u32 ctrl;
+
+	/* cxfr for the codec path, pxfr for the preview path */
+	isi_writel(isi, ISI_INTEN,
+		   ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
+
+	/* Enable ISI */
+	ctrl = ISI_CTRL_EN |
+	       (isi->enable_preview_path ? 0 : ISI_CTRL_CDC);
+	isi_writel(isi, ISI_CTRL, ctrl);
+}
+
 static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
 {
 	if (isi->active) {
@@ -403,12 +417,6 @@ static void buffer_cleanup(struct vb2_buffer *vb)
 
 static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
 {
-	u32 ctrl;
-
-	/* 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->enable_preview_path) {
 		if (isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) {
@@ -429,13 +437,6 @@ static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
 		isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_P_CH);
 	}
 
-	/* Enable ISI */
-	ctrl = ISI_CTRL_EN;
-
-	if (!isi->enable_preview_path)
-		ctrl |= ISI_CTRL_CDC;
-
-	isi_writel(isi, ISI_CTRL, ctrl);
 }
 
 static void buffer_queue(struct vb2_buffer *vb)
@@ -478,8 +479,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	spin_lock_irq(&isi->lock);
 
-	if (count)
+	if (count) {
 		start_dma(isi, isi->active);
+		start_isi(isi);
+	}
+
 	spin_unlock_irq(&isi->lock);
 
 	return 0;
-- 
1.9.1

[PATCH 10/13] atmel-isi: reuse start_dma() function in isi interrupt handler

[Josh Wu]

Reuse start_dma() in interrupt handler function to avoid duplication.
Also we need to move start_dma() function up in the file.

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 62 +++++++++++----------------
 1 file changed, 25 insertions(+), 37 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 0e42171..c1a8698 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -259,6 +259,30 @@ static void start_isi(struct atmel_isi *isi)
 	isi_writel(isi, ISI_CTRL, ctrl);
 }
 
+static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
+{
+	/* Check if already in a frame */
+	if (!isi->enable_preview_path) {
+		if (isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) {
+			dev_err(isi->soc_host.icd->parent, "Already in frame handling.\n");
+			return;
+		}
+
+		isi_writel(isi, ISI_DMA_C_DSCR,
+				(u32)buffer->p_dma_desc->fbd_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);
+	} else {
+		isi_writel(isi, ISI_DMA_P_DSCR,
+				(u32)buffer->p_dma_desc->fbd_phys);
+		isi_writel(isi, ISI_DMA_P_CTRL,
+				ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
+		isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_P_CH);
+	}
+
+}
+
 static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
 {
 	if (isi->active) {
@@ -277,19 +301,7 @@ static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
 		/* start next dma frame. */
 		isi->active = list_entry(isi->video_buffer_list.next,
 					struct frame_buffer, list);
-		if (!isi->enable_preview_path) {
-			isi_writel(isi, ISI_DMA_C_DSCR,
-				(u32)isi->active->p_dma_desc->fbd_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);
-		} else {
-			isi_writel(isi, ISI_DMA_P_DSCR,
-				(u32)isi->active->p_dma_desc->fbd_phys);
-			isi_writel(isi, ISI_DMA_P_CTRL,
-				ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
-			isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_P_CH);
-		}
+		start_dma(isi, isi->active);
 	}
 	return IRQ_HANDLED;
 }
@@ -415,30 +427,6 @@ static void buffer_cleanup(struct vb2_buffer *vb)
 		list_add(&buf->p_dma_desc->list, &isi->dma_desc_head);
 }
 
-static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
-{
-	/* Check if already in a frame */
-	if (!isi->enable_preview_path) {
-		if (isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) {
-			dev_err(isi->soc_host.icd->parent, "Already in frame handling.\n");
-			return;
-		}
-
-		isi_writel(isi, ISI_DMA_C_DSCR,
-				(u32)buffer->p_dma_desc->fbd_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);
-	} else {
-		isi_writel(isi, ISI_DMA_P_DSCR,
-				(u32)buffer->p_dma_desc->fbd_phys);
-		isi_writel(isi, ISI_DMA_P_CTRL,
-				ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
-		isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_P_CH);
-	}
-
-}
-
 static void buffer_queue(struct vb2_buffer *vb)
 {
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
-- 
1.9.1

[PATCH 11/13] atmel-isi: add hw_uninitialize() in stop_streaming()

[Josh Wu]

add new function: hw_uninitialize() and call it when stop_streaming().

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 46 +++++++++++++++------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index c1a8698..7d2e952 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -245,6 +245,31 @@ static int isi_hw_initialize(struct atmel_isi *isi)
 	return 0;
 }
 
+static void isi_hw_uninitialize(struct atmel_isi *isi)
+{
+	int ret;
+
+	if (!isi->enable_preview_path) {
+		/* Wait until the end of the current frame. */
+		ret = isi_hw_wait_status(isi, ISI_CTRL_CDC,
+					 FRAME_INTERVAL_MILLI_SEC);
+		if (ret)
+			dev_err(isi->soc_host.icd->parent, "Timeout waiting for finishing codec request\n");
+	}
+
+	/* Disable interrupts */
+	isi_writel(isi, ISI_INTDIS,
+			ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
+
+	/* Disable ISI and wait for it is done */
+	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
+
+	/* Check Reset status */
+	ret  = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
+	if (ret)
+		dev_err(isi->soc_host.icd->parent, "Disable ISI timed out\n");
+}
+
 static void start_isi(struct atmel_isi *isi)
 {
 	u32 ctrl;
@@ -484,7 +509,6 @@ static void stop_streaming(struct vb2_queue *vq)
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 	struct atmel_isi *isi = ici->priv;
 	struct frame_buffer *buf, *node;
-	int ret = 0;
 
 	spin_lock_irq(&isi->lock);
 	isi->active = NULL;
@@ -495,25 +519,7 @@ static void stop_streaming(struct vb2_queue *vq)
 	}
 	spin_unlock_irq(&isi->lock);
 
-	if (!isi->enable_preview_path) {
-		/* Wait until the end of the current frame. */
-		ret = isi_hw_wait_status(isi, ISI_CTRL_CDC,
-					 FRAME_INTERVAL_MILLI_SEC);
-		if (ret)
-			dev_err(icd->parent, "Timeout waiting for finishing codec request\n");
-	}
-
-	/* Disable interrupts */
-	isi_writel(isi, ISI_INTDIS,
-			ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
-
-	/* Disable ISI and wait for it is done */
-	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
-	/* Check Reset status */
-	ret  = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
-	if (ret)
-		dev_err(icd->parent, "Disable ISI timed out\n");
+	isi_hw_uninitialize(isi);
 
 	pm_runtime_put(ici->v4l2_dev.dev);
 }
-- 
1.9.1

[PATCH 11/13] atmel-isi: add hw_uninitialize()

[Josh Wu]

add new function: hw_uninitialize() and call it when stop_streaming().

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 46 +++++++++++++++------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index b789523..843102f 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -245,6 +245,31 @@ static int isi_hw_initialize(struct atmel_isi *isi)
 	return 0;
 }
 
+static void isi_hw_uninitialize(struct atmel_isi *isi)
+{
+	int ret;
+
+	if (!isi->enable_preview_path) {
+		/* Wait until the end of the current frame. */
+		ret = isi_hw_wait_status(isi, ISI_CTRL_CDC,
+				         FRAME_INTERVAL_MILLI_SEC);
+		if (ret)
+			dev_err(isi->soc_host.icd->parent, "Timeout waiting for finishing codec request\n");
+	}
+
+	/* Disable interrupts */
+	isi_writel(isi, ISI_INTDIS,
+			ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
+
+	/* Disable ISI and wait for it is done */
+	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
+
+	/* Check Reset status */
+	ret  = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
+	if (ret)
+		dev_err(isi->soc_host.icd->parent, "Disable ISI timed out\n");
+}
+
 static void start_isi(struct atmel_isi *isi)
 {
 	u32 ctrl;
@@ -484,7 +509,6 @@ static void stop_streaming(struct vb2_queue *vq)
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 	struct atmel_isi *isi = ici->priv;
 	struct frame_buffer *buf, *node;
-	int ret = 0;
 
 	spin_lock_irq(&isi->lock);
 	isi->active = NULL;
@@ -495,25 +519,7 @@ static void stop_streaming(struct vb2_queue *vq)
 	}
 	spin_unlock_irq(&isi->lock);
 
-	if (!isi->enable_preview_path) {
-		/* Wait until the end of the current frame. */
-		ret = isi_hw_wait_status(isi, ISI_CTRL_CDC,
-				         FRAME_INTERVAL_MILLI_SEC);
-		if (ret)
-			dev_err(icd->parent, "Timeout waiting for finishing codec request\n");
-	}
-
-	/* Disable interrupts */
-	isi_writel(isi, ISI_INTDIS,
-			ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
-
-	/* Disable ISI and wait for it is done */
-	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
-	/* Check Reset status */
-	ret  = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
-	if (ret)
-		dev_err(icd->parent, "Disable ISI timed out\n");
+	isi_hw_uninitialize(isi);
 
 	pm_runtime_put(ici->v4l2_dev.dev);
 }
-- 
1.9.1

[PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor)

[Josh Wu]

From: Josh Wu <josh.wu@atmel.com>

This way, we can easy to add other type of fbd for new hardware.

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 33 ++++++++++++++++++---------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 7d2e952..b4c1f38 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -37,7 +37,7 @@
 #define FRAME_INTERVAL_MILLI_SEC	(1000 / MIN_FRAME_RATE)
 
 /* Frame buffer descriptor */
-struct fbd {
+struct fbd_isi_v2 {
 	/* Physical address of the frame buffer */
 	u32 fb_address;
 	/* DMA Control Register(only in HISI2) */
@@ -46,9 +46,13 @@ struct fbd {
 	u32 next_fbd_address;
 };
 
+union fbd {
+	struct fbd_isi_v2 fbd_isi;
+};
+
 struct isi_dma_desc {
 	struct list_head list;
-	struct fbd *p_fbd;
+	union fbd *p_fbd;
 	dma_addr_t fbd_phys;
 };
 
@@ -69,7 +73,7 @@ struct atmel_isi {
 	struct vb2_alloc_ctx		*alloc_ctx;
 
 	/* Allocate descriptors for dma buffer use */
-	struct fbd			*p_fb_descriptors;
+	union fbd			*p_fb_descriptors;
 	dma_addr_t			fb_descriptors_phys;
 	struct				list_head dma_desc_head;
 	struct isi_dma_desc		dma_desc[MAX_BUFFER_NUM];
@@ -396,6 +400,16 @@ static int buffer_init(struct vb2_buffer *vb)
 	return 0;
 }
 
+static void isi_hw_init_dma_desc(union fbd *p_fdb, u32 fb_addr,
+				 u32 next_fbd_addr)
+{
+	struct fbd_isi_v2 *p = &(p_fdb->fbd_isi);
+
+	p->fb_address = fb_addr;
+	p->next_fbd_address = next_fbd_addr;
+	p->dma_ctrl = ISI_DMA_CTRL_WB;
+}
+
 static int buffer_prepare(struct vb2_buffer *vb)
 {
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
@@ -428,10 +442,7 @@ static int buffer_prepare(struct vb2_buffer *vb)
 			list_del_init(&desc->list);
 
 			/* Initialize the dma descriptor */
-			desc->p_fbd->fb_address =
-					vb2_dma_contig_plane_dma_addr(vb, 0);
-			desc->p_fbd->next_fbd_address = 0;
-			desc->p_fbd->dma_ctrl = ISI_DMA_CTRL_WB;
+			isi_hw_init_dma_desc(desc->p_fbd, vb2_dma_contig_plane_dma_addr(vb, 0), 0);
 
 			buf->p_dma_desc = desc;
 		}
@@ -923,7 +934,7 @@ static int atmel_isi_remove(struct platform_device *pdev)
 	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,
+			sizeof(union fbd) * MAX_BUFFER_NUM,
 			isi->p_fb_descriptors,
 			isi->fb_descriptors_phys);
 	pm_runtime_disable(&pdev->dev);
@@ -1010,7 +1021,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&isi->dma_desc_head);
 
 	isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
-				sizeof(struct fbd) * MAX_BUFFER_NUM,
+				sizeof(union fbd) * MAX_BUFFER_NUM,
 				&isi->fb_descriptors_phys,
 				GFP_KERNEL);
 	if (!isi->p_fb_descriptors) {
@@ -1021,7 +1032,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
 	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);
+					i * sizeof(union fbd);
 		list_add(&isi->dma_desc[i].list, &isi->dma_desc_head);
 	}
 
@@ -1080,7 +1091,7 @@ err_ioremap:
 	vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
 err_alloc_ctx:
 	dma_free_coherent(&pdev->dev,
-			sizeof(struct fbd) * MAX_BUFFER_NUM,
+			sizeof(union fbd) * MAX_BUFFER_NUM,
 			isi->p_fb_descriptors,
 			isi->fb_descriptors_phys);
 
-- 
1.9.1

[PATCH 13/13] atmel-isi: use an hw_data structure according compatible string

[Josh Wu]

From: Josh Wu <josh.wu@atmel.com>

The hw_data can define new hw_ops, hw_regs, which has all hardware related
data. That can make us easy to add another hardware support.

Also rename the hw related function with 'isi_' prefix.

Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 88 ++++++++++++++++++++++-----
 1 file changed, 72 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index b4c1f38..7f7be7d 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -18,6 +18,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
@@ -91,6 +92,8 @@ struct atmel_isi {
 	struct frame_buffer		*active;
 
 	struct soc_camera_host		soc_host;
+	struct at91_camera_hw_ops	*hw_ops;
+	struct at91_camera_hw_regs	*hw_regs;
 };
 
 static void isi_writel(struct atmel_isi *isi, u32 reg, u32 val)
@@ -102,6 +105,29 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
 	return readl(isi->regs + reg);
 }
 
+struct at91_camera_hw_ops {
+	/* start dma transfer */
+	void (*hw_start_dma)(struct atmel_isi *isi, struct frame_buffer *buf);
+	/* start ISI hardware capture */
+	void (*hw_start)(struct atmel_isi *isi);
+	int  (*hw_initialize)(struct atmel_isi *isi);
+	void (*hw_uninitialize)(struct atmel_isi *isi);
+	void (*hw_configure)(struct atmel_isi *isi, u32 width, u32 height,
+			     const struct soc_camera_format_xlate *xlate);
+	irqreturn_t (*hw_interrupt)(int irq, void *dev_id);
+	void (*hw_init_dma_desc)(union fbd *p_fdb, u32 fb_addr,
+			      u32 next_fbd_addr);
+};
+
+struct at91_camera_hw_regs {
+	u32 status_reg_offset;
+};
+
+struct at91_camera_hw_data {
+	struct at91_camera_hw_ops *hw_ops;
+	struct at91_camera_hw_regs *hw_regs;
+};
+
 static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
 		const struct soc_camera_format_xlate *xlate)
 {
@@ -140,7 +166,7 @@ static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
 	return ISI_CFG2_YCC_SWAP_DEFAULT;
 }
 
-static void configure_geometry(struct atmel_isi *isi, u32 width,
+static void isi_configure_geometry(struct atmel_isi *isi, u32 width,
 		u32 height, const struct soc_camera_format_xlate *xlate)
 {
 	u32 cfg2, psize;
@@ -192,8 +218,8 @@ static int isi_hw_wait_status(struct atmel_isi *isi, int status_flag,
 {
 	unsigned long timeout = jiffies + wait_ms * HZ;
 
-	while ((isi_readl(isi, ISI_STATUS) & status_flag) &&
-			time_before(jiffies, timeout))
+	while ((isi_readl(isi, isi->hw_regs->status_reg_offset) & status_flag)
+			&& time_before(jiffies, timeout))
 		msleep(1);
 
 	if (time_after(jiffies, timeout))
@@ -274,7 +300,7 @@ static void isi_hw_uninitialize(struct atmel_isi *isi)
 		dev_err(isi->soc_host.icd->parent, "Disable ISI timed out\n");
 }
 
-static void start_isi(struct atmel_isi *isi)
+static void isi_start(struct atmel_isi *isi)
 {
 	u32 ctrl;
 
@@ -288,7 +314,7 @@ static void start_isi(struct atmel_isi *isi)
 	isi_writel(isi, ISI_CTRL, ctrl);
 }
 
-static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
+static void isi_start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
 {
 	/* Check if already in a frame */
 	if (!isi->enable_preview_path) {
@@ -330,7 +356,8 @@ static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
 		/* start next dma frame. */
 		isi->active = list_entry(isi->video_buffer_list.next,
 					struct frame_buffer, list);
-		start_dma(isi, isi->active);
+
+		(*isi->hw_ops->hw_start_dma)(isi, isi->active);
 	}
 	return IRQ_HANDLED;
 }
@@ -419,6 +446,7 @@ static int buffer_prepare(struct vb2_buffer *vb)
 	struct atmel_isi *isi = ici->priv;
 	unsigned long size;
 	struct isi_dma_desc *desc;
+	u32 vb_addr;
 
 	size = icd->sizeimage;
 
@@ -442,7 +470,8 @@ static int buffer_prepare(struct vb2_buffer *vb)
 			list_del_init(&desc->list);
 
 			/* Initialize the dma descriptor */
-			isi_hw_init_dma_desc(desc->p_fbd, vb2_dma_contig_plane_dma_addr(vb, 0), 0);
+			vb_addr = vb2_dma_contig_plane_dma_addr(vb, 0);
+			(*isi->hw_ops->hw_init_dma_desc)(desc->p_fbd, vb_addr, 0);
 
 			buf->p_dma_desc = desc;
 		}
@@ -478,7 +507,7 @@ static void buffer_queue(struct vb2_buffer *vb)
 	if (isi->active == NULL) {
 		isi->active = buf;
 		if (vb2_is_streaming(vb->vb2_queue))
-			start_dma(isi, buf);
+			(*isi->hw_ops->hw_start_dma)(isi, buf);
 	}
 	spin_unlock_irqrestore(&isi->lock, flags);
 }
@@ -492,20 +521,20 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	pm_runtime_get_sync(ici->v4l2_dev.dev);
 
-	ret = isi_hw_initialize(isi);
+	ret = (*isi->hw_ops->hw_initialize)(isi);
 	if (ret) {
 		pm_runtime_put(ici->v4l2_dev.dev);
 		return ret;
 	}
 
-	configure_geometry(isi, icd->user_width, icd->user_height,
-				icd->current_fmt);
+	(*isi->hw_ops->hw_configure)(isi, icd->user_width, icd->user_height,
+				     icd->current_fmt);
 
 	spin_lock_irq(&isi->lock);
 
 	if (count) {
-		start_dma(isi, isi->active);
-		start_isi(isi);
+		(*isi->hw_ops->hw_start_dma)(isi, isi->active);
+		(*isi->hw_ops->hw_start)(isi);
 	}
 
 	spin_unlock_irq(&isi->lock);
@@ -530,7 +559,7 @@ static void stop_streaming(struct vb2_queue *vq)
 	}
 	spin_unlock_irq(&isi->lock);
 
-	isi_hw_uninitialize(isi);
+	(*isi->hw_ops->hw_uninitialize)(isi);
 
 	pm_runtime_put(ici->v4l2_dev.dev);
 }
@@ -993,6 +1022,7 @@ static int atmel_isi_parse_dt(struct atmel_isi *isi,
 	return 0;
 }
 
+static const struct of_device_id atmel_isi_of_match[];
 static int atmel_isi_probe(struct platform_device *pdev)
 {
 	unsigned int irq;
@@ -1000,6 +1030,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
 	struct resource *regs;
 	int ret, i;
 	struct soc_camera_host *soc_host;
+	struct at91_camera_hw_data *hw_data;
 
 	isi = devm_kzalloc(&pdev->dev, sizeof(struct atmel_isi), GFP_KERNEL);
 	if (!isi) {
@@ -1015,6 +1046,11 @@ static int atmel_isi_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	hw_data = (struct at91_camera_hw_data *)
+		of_match_device(atmel_isi_of_match, &pdev->dev)->data;
+	isi->hw_ops = hw_data->hw_ops;
+	isi->hw_regs = hw_data->hw_regs;
+
 	isi->active = NULL;
 	spin_lock_init(&isi->lock);
 	INIT_LIST_HEAD(&isi->video_buffer_list);
@@ -1060,7 +1096,8 @@ static int atmel_isi_probe(struct platform_device *pdev)
 		goto err_req_irq;
 	}
 
-	ret = devm_request_irq(&pdev->dev, irq, isi_interrupt, 0, "isi", isi);
+	ret = devm_request_irq(&pdev->dev, irq, isi->hw_ops->hw_interrupt, 0,
+			       "isi", isi);
 	if (ret) {
 		dev_err(&pdev->dev, "Unable to request irq %d\n", irq);
 		goto err_req_irq;
@@ -1119,13 +1156,32 @@ static int atmel_isi_runtime_resume(struct device *dev)
 }
 #endif /* CONFIG_PM */
 
+static struct at91_camera_hw_ops at91sam9g45_ops = {
+	.hw_initialize = isi_hw_initialize,
+	.hw_uninitialize = isi_hw_uninitialize,
+	.hw_configure = isi_configure_geometry,
+	.hw_start = isi_start,
+	.hw_start_dma = isi_start_dma,
+	.hw_interrupt = isi_interrupt,
+	.hw_init_dma_desc = isi_hw_init_dma_desc,
+};
+
+static struct at91_camera_hw_regs at91sam9g45_regs = {
+	.status_reg_offset = ISI_STATUS,
+};
+
+static struct at91_camera_hw_data at91sam9g45_data = {
+	.hw_ops = &at91sam9g45_ops,
+	.hw_regs = &at91sam9g45_regs,
+};
+
 static const struct dev_pm_ops atmel_isi_dev_pm_ops = {
 	SET_RUNTIME_PM_OPS(atmel_isi_runtime_suspend,
 				atmel_isi_runtime_resume, NULL)
 };
 
 static const struct of_device_id atmel_isi_of_match[] = {
-	{ .compatible = "atmel,at91sam9g45-isi" },
+	{ .compatible = "atmel,at91sam9g45-isi", .data = &at91sam9g45_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, atmel_isi_of_match);
-- 
1.9.1

Re: [PATCH 00/13] media: atmel-isi: extract the hw releated functions into structure

[Ludovic Desroches]

Hi Josh,

On Mon, Jan 18, 2016 at 08:21:36PM +0800, Josh Wu wrote:
> This series refactor the atmel-isi drvier. In the meantime, extract all
> the hardware related functions, and made it as function table. Also add
> some hardware data.
> 
> All those hardware functions, datas are defined with the compatible
> string.
> 
> In this way, it is easy to add another compatible string for new
> hardware support.

What is the goal of these patches? I mean is it to ease the support of
ISC?

Discussing with Songjun, I understand that he wanted to have one driver
for ISI and one for ISC but I have the feeling that your patches go in
the opposite direction. What is your mind about this?

Thanks

Regards

Ludovic

> 
> 
> Josh Wu (13):
>   atmel-isi: use try_or_set_fmt() for both set_fmt() and try_fmt()
>   atmel-isi: move the is_support() close to try/set format function
>   atmel-isi: add isi_hw_initialize() function to handle hw setup
>   atmel-isi: move the cfg1 initialize to isi_hw_initialize()
>   atmel-isi: add a function: isi_hw_wait_status() to check ISI_SR status
>   atmel-isi: check ISI_SR's flags by polling instead of interrupt
>   atmel-isi: move hw code into isi_hw_initialize()
>   atmel-isi: remove the function set_dma_ctrl() as it just use once
>   atmel-isi: add a function start_isi()
>   atmel-isi: reuse start_dma() function in isi interrupt handler
>   atmel-isi: add hw_uninitialize() in stop_streaming()
>   atmel-isi: use union for the fbd (frame buffer descriptor)
>   atmel-isi: use an hw_data structure according compatible string
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 529 ++++++++++++++------------
>  1 file changed, 277 insertions(+), 252 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [PATCH 00/13] media: atmel-isi: extract the hw releated functions into structure

[Josh Wu]

Hi, Ludovic

2016-01-19 22:52 GMT+08:00 Ludovic Desroches <ludovic.desroches@atmel.com>:
>
> Hi Josh,
>
> On Mon, Jan 18, 2016 at 08:21:36PM +0800, Josh Wu wrote:
> > This series refactor the atmel-isi drvier. In the meantime, extract all
> > the hardware related functions, and made it as function table. Also add
> > some hardware data.
> >
> > All those hardware functions, datas are defined with the compatible
> > string.
> >
> > In this way, it is easy to add another compatible string for new
> > hardware support.
>
> What is the goal of these patches? I mean is it to ease the support of
> ISC?


yes, I think these patches can make it easy to support ISC. As after
those patches, we can reuse the main framework, and just replace the
hardware operations to support ISC. (the patches are queued yet, I
would like to get some feedback before do such changes.)


>
>
> Discussing with Songjun, I understand that he wanted to have one driver
> for ISI and one for ISC but I have the feeling that your patches go in
> the opposite direction. What is your mind about this?


In my point of view, I prefer to use one driver to support both ISI
and ISC hardware. That would reduce many duplicated code. In the
meantime, using the different compatible string with different
hardware operation functions table can make ISC work well and also can
support further hardware IP.

What's your thought about this?

Best Regards,
Josh Wu

>
> Thanks
>
> Regards
>
> Ludovic
>
> >
> >
> > Josh Wu (13):
> >   atmel-isi: use try_or_set_fmt() for both set_fmt() and try_fmt()
> >   atmel-isi: move the is_support() close to try/set format function
> >   atmel-isi: add isi_hw_initialize() function to handle hw setup
> >   atmel-isi: move the cfg1 initialize to isi_hw_initialize()
> >   atmel-isi: add a function: isi_hw_wait_status() to check ISI_SR status
> >   atmel-isi: check ISI_SR's flags by polling instead of interrupt
> >   atmel-isi: move hw code into isi_hw_initialize()
> >   atmel-isi: remove the function set_dma_ctrl() as it just use once
> >   atmel-isi: add a function start_isi()
> >   atmel-isi: reuse start_dma() function in isi interrupt handler
> >   atmel-isi: add hw_uninitialize() in stop_streaming()
> >   atmel-isi: use union for the fbd (frame buffer descriptor)
> >   atmel-isi: use an hw_data structure according compatible string
> >
> >  drivers/media/platform/soc_camera/atmel-isi.c | 529 ++++++++++++++------------
> >  1 file changed, 277 insertions(+), 252 deletions(-)
> >
> > --
> > 1.9.1
> >

Re: [PATCH 01/13] atmel-isi: use try_or_set_fmt() for both set_fmt() and try_fmt()

[Guennadi Liakhovetski]

On Mon, 18 Jan 2016, Josh Wu wrote:

> From: Josh Wu <josh.wu@atmel.com>
> 
> Since atmel-isi has similar set_fmt() and try_fmt() functions. So this
> patch will add a new function which can be called by set_fmt() and
> try_fmt().
> 
> That can increase the reusability.
> 
> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 105 ++++++++++----------------
>  1 file changed, 41 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index c398b28..dc81df3 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -571,16 +571,16 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
>  	return vb2_queue_init(q);
>  }
>  
> -static int isi_camera_set_fmt(struct soc_camera_device *icd,
> -			      struct v4l2_format *f)
> +static int try_or_set_fmt(struct soc_camera_device *icd,
> +		   struct v4l2_format *f,
> +		   struct v4l2_subdev_format *format)
>  {
> -	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_subdev_format format = {
> -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> -	};
> -	struct v4l2_mbus_framefmt *mf = &format.format;
> +	const struct soc_camera_format_xlate *xlate;
> +	struct v4l2_subdev_pad_config pad_cfg;
> +
> +	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> +	struct v4l2_mbus_framefmt *mf = &format->format;
>  	int ret;
>  
>  	/* check with atmel-isi support format, if not support use YUYV */
> @@ -594,8 +594,11 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
>  		return -EINVAL;

Since you're already touching this, please, also fix the "if (!xlate)" 
check in the .try_fmt() case. Basically .try_fmt() shouldn't fail because 
of unsupported parameters. If input parameters are unsupported, the driver 
should replace them with "default" or "closest" ones, at least with 
something, that is supported. For an example, please, have a look at 
sh_mobile_ceu.c or rcar_vin.c. So, once you unite .set_fmt() and 
.try_fmt() in your new try_or_set_fmt() function, you can do:

	xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
	if (!xlate) {
		if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
			dev_warn(icd->parent, "Format %x not found\n",
				 pix->pixelformat);
			return -EINVAL;
		}

		/* Pick up a supported format */
		xlate = icd->current_fmt;
		pixfmt = xlate->host_fmt->fourcc;
		pix->pixelformat = pixfmt;
		pix->colorspace = icd->colorspace;
	}

if you decide to keep the current format, or pick up another 
known-supported one.

>  	}
>  
> -	dev_dbg(icd->parent, "Plan to set format %dx%d\n",
> -			pix->width, pix->height);
> +	/* 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;

This isn't needed in the .set_fmt() case. soc-camera only calls .set_fmt() 
after having (successfully) called .try_fmt, so, sizes have already been 
adjusted. You can put this into isi_camera_try_fmt() before calling 
try_or_set_fmt().

>  
>  	mf->width	= pix->width;
>  	mf->height	= pix->height;
> @@ -603,7 +606,11 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
>  	mf->colorspace	= pix->colorspace;
>  	mf->code	= xlate->code;
>  
> -	ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, &format);
> +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +		ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, format);

I don't think you have to special case this just to use NULL, using 
&pad_cfg for both should work.

> +	else
> +		ret = v4l2_subdev_call(sd, pad, set_fmt, &pad_cfg, format);
> +
>  	if (ret < 0)
>  		return ret;
>  
> @@ -614,64 +621,14 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
>  	pix->height		= mf->height;
>  	pix->field		= mf->field;
>  	pix->colorspace		= mf->colorspace;
> -	icd->current_fmt	= xlate;
> -
> -	dev_dbg(icd->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_subdev_pad_config pad_cfg;
> -	struct v4l2_subdev_format format = {
> -		.which = V4L2_SUBDEV_FORMAT_TRY,
> -	};
> -	struct v4l2_mbus_framefmt *mf = &format.format;
> -	u32 pixfmt = pix->pixelformat;
> -	int ret;
> -
> -	/* check with atmel-isi support format, if not support use YUYV */
> -	if (!is_supported(icd, pix->pixelformat))
> -		pix->pixelformat = V4L2_PIX_FMT_YUYV;
> -
> -	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
> -	if (pixfmt && !xlate) {
> -		dev_warn(icd->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, pad, set_fmt, &pad_cfg, &format);
> -	if (ret < 0)
> -		return ret;
> -
> -	pix->width	= mf->width;
> -	pix->height	= mf->height;
> -	pix->colorspace	= mf->colorspace;
> +	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +		icd->current_fmt = xlate;
>  
>  	switch (mf->field) {
>  	case V4L2_FIELD_ANY:
> -		pix->field = V4L2_FIELD_NONE;
> -		break;
>  	case V4L2_FIELD_NONE:
> +		pix->field = V4L2_FIELD_NONE;
>  		break;
>  	default:
>  		dev_err(icd->parent, "Field type %d unsupported.\n",

The driver only supports progressive field order. So, I would just 
directly set .field = V4L2_FIELD_NONE before calling subdev's .set_fmt().
Then in the .set_fmt() case check, whether the subdev has changed it to 
anything else, and fail if so.

Thanks
Guennadi

> @@ -682,6 +639,26 @@ static int isi_camera_try_fmt(struct soc_camera_device *icd,
>  	return ret;
>  }
>  
> +static int isi_camera_set_fmt(struct soc_camera_device *icd,
> +			      struct v4l2_format *f)
> +{
> +	struct v4l2_subdev_format format = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +	};
> +
> +	return try_or_set_fmt(icd, f, &format);
> +}
> +
> +static int isi_camera_try_fmt(struct soc_camera_device *icd,
> +			      struct v4l2_format *f)
> +{
> +	struct v4l2_subdev_format format = {
> +		.which = V4L2_SUBDEV_FORMAT_TRY,
> +	};
> +
> +	return try_or_set_fmt(icd, f, &format);
> +}
> +
>  static const struct soc_mbus_pixelfmt isi_camera_formats[] = {
>  	{
>  		.fourcc			= V4L2_PIX_FMT_YUYV,
> -- 
> 1.9.1
> 

Re: [PATCH 02/13] atmel-isi: move the is_support() close to try/set format function

[Guennadi Liakhovetski]

On Mon, 18 Jan 2016, Josh Wu wrote:

> As is_support() only used by try/set format function so put them

Typo: "is_supported()"

Thanks
Guennadi

> together.
> 
> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 36 +++++++++++++--------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index dc81df3..3793b68 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -189,24 +189,6 @@ static void configure_geometry(struct atmel_isi *isi, u32 width,
>  	return;
>  }
>  
> -static bool is_supported(struct soc_camera_device *icd,
> -		const u32 pixformat)
> -{
> -	switch (pixformat) {
> -	/* YUV, including grey */
> -	case V4L2_PIX_FMT_GREY:
> -	case V4L2_PIX_FMT_YUYV:
> -	case V4L2_PIX_FMT_UYVY:
> -	case V4L2_PIX_FMT_YVYU:
> -	case V4L2_PIX_FMT_VYUY:
> -	/* RGB */
> -	case V4L2_PIX_FMT_RGB565:
> -		return true;
> -	default:
> -		return false;
> -	}
> -}
> -
>  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>  {
>  	if (isi->active) {
> @@ -571,6 +553,24 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
>  	return vb2_queue_init(q);
>  }
>  
> +static bool is_supported(struct soc_camera_device *icd,
> +		const u32 pixformat)
> +{
> +	switch (pixformat) {
> +	/* YUV, including grey */
> +	case V4L2_PIX_FMT_GREY:
> +	case V4L2_PIX_FMT_YUYV:
> +	case V4L2_PIX_FMT_UYVY:
> +	case V4L2_PIX_FMT_YVYU:
> +	case V4L2_PIX_FMT_VYUY:
> +	/* RGB */
> +	case V4L2_PIX_FMT_RGB565:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  static int try_or_set_fmt(struct soc_camera_device *icd,
>  		   struct v4l2_format *f,
>  		   struct v4l2_subdev_format *format)
> -- 
> 1.9.1
> 

Re: [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt

[Guennadi Liakhovetski]

On Mon, 18 Jan 2016, Josh Wu wrote:

> In current code, we use a interrupt to check whether ISI reset/disable
> action is done. Actually, we also can check ISI SR to check the
> reset/disable action by polling, and it is simpler and straight forward.
> 
> So this patch use isi_hw_wait_status() to check the action status. As
> the interrupt checking the status is useless, so just remove the interrupt
> & completion code.

Sorry, I'm not convinced. Switching from interrupt-driven to polling seems 
counter-productive to me. Why do you want this?

Thanks
Guennadi

> 
> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 59 ++++++---------------------
>  1 file changed, 13 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index f0508ea..4ddc309 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -12,7 +12,6 @@
>   */
>  
>  #include <linux/clk.h>
> -#include <linux/completion.h>
>  #include <linux/delay.h>
>  #include <linux/fs.h>
>  #include <linux/init.h>
> @@ -81,7 +80,6 @@ struct atmel_isi {
>  	struct isi_dma_desc		dma_desc[MAX_BUFFER_NUM];
>  	bool				enable_preview_path;
>  
> -	struct completion		complete;
>  	/* ISI peripherial clock */
>  	struct clk			*pclk;
>  	unsigned int			irq;
> @@ -281,51 +279,14 @@ static irqreturn_t isi_interrupt(int irq, void *dev_id)
>  	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 (likely(pending & ISI_SR_CXFR_DONE) ||
> -				likely(pending & ISI_SR_PXFR_DONE))
> -			ret = atmel_isi_handle_streaming(isi);
> -	}
> +	if (likely(pending & ISI_SR_CXFR_DONE) ||
> +	    likely(pending & ISI_SR_PXFR_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(500));
> -	if (timeout == 0)
> -		return -ETIMEDOUT;
> -
> -	return 0;
> -}
> -
>  /* ------------------------------------------------------------------
>  	Videobuf operations
>     ------------------------------------------------------------------*/
> @@ -493,8 +454,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  	pm_runtime_get_sync(ici->v4l2_dev.dev);
>  
>  	/* Reset ISI */
> -	ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
> -	if (ret < 0) {
> +	isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
> +
> +	/* Check Reset status */
> +	ret  = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
> +	if (ret) {
>  		dev_err(icd->parent, "Reset ISI timed out\n");
>  		pm_runtime_put(ici->v4l2_dev.dev);
>  		return ret;
> @@ -549,8 +513,11 @@ static void stop_streaming(struct vb2_queue *vq)
>  			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)
> +	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> +
> +	/* Check Reset status */
> +	ret  = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
> +	if (ret)
>  		dev_err(icd->parent, "Disable ISI timed out\n");
>  
>  	pm_runtime_put(ici->v4l2_dev.dev);
> -- 
> 1.9.1
> 

Re: [PATCH 07/13] atmel-isi: move hw code into isi_hw_initialize()

[Guennadi Liakhovetski]

On Mon, 18 Jan 2016, Josh Wu wrote:

> That make hw operation code separate with general code.
> 
> Also since reset action can be failed, so add a return value for
> isi_hw_initialze().
> 
> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 34 +++++++++++++++++----------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index 4ddc309..ed4d04b 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -203,10 +203,27 @@ static int isi_hw_wait_status(struct atmel_isi *isi, int status_flag,
>  	return 0;
>  }
>  
> -static void isi_hw_initialize(struct atmel_isi *isi)
> +static int isi_hw_initialize(struct atmel_isi *isi)
>  {
>  	u32 common_flags = isi->bus_param;
>  	u32 cfg1 = 0;
> +	int ret;
> +
> +	/* Reset ISI */
> +	isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
> +
> +	/* Check Reset status */
> +	ret  = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);

You could also remove the superfluous space while at it.

Thanks
Guennadi

> +	if (ret) {
> +		dev_err(isi->soc_host.icd->parent, "Reset ISI timed out\n");
> +		return ret;
> +	}
> +
> +	/* Disable all interrupts */
> +	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
> +
> +	/* Clear any pending interrupt */
> +	isi_readl(isi, ISI_STATUS);
>  
>  	/* set bus param for ISI */
>  	if (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> @@ -229,6 +246,8 @@ static void isi_hw_initialize(struct atmel_isi *isi)
>  
>  	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>  	isi_writel(isi, ISI_CFG1, cfg1);
> +
> +	return 0;
>  }
>  
>  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> @@ -453,27 +472,16 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	pm_runtime_get_sync(ici->v4l2_dev.dev);
>  
> -	/* Reset ISI */
> -	isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
> -
> -	/* Check Reset status */
> -	ret  = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
> +	ret = isi_hw_initialize(isi);
>  	if (ret) {
> -		dev_err(icd->parent, "Reset ISI timed out\n");
>  		pm_runtime_put(ici->v4l2_dev.dev);
>  		return ret;
>  	}
> -	/* Disable all interrupts */
> -	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
> -
> -	isi_hw_initialize(isi);
>  
>  	configure_geometry(isi, icd->user_width, icd->user_height,
>  				icd->current_fmt);
>  
>  	spin_lock_irq(&isi->lock);
> -	/* Clear any pending interrupt */
> -	isi_readl(isi, ISI_STATUS);
>  
>  	if (count)
>  		start_dma(isi, isi->active);
> -- 
> 1.9.1
> 

Re: [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor)

[Guennadi Liakhovetski]

On Mon, 18 Jan 2016, Josh Wu wrote:

> From: Josh Wu <josh.wu@atmel.com>
> 
> This way, we can easy to add other type of fbd for new hardware.

Ok, I've applied all your 13 patches to check, what the resulting driver 
would look like. To me it looks like you really abstract away _everything_ 
remotely hardware-specific. What is left is yet another abstraction layer, 
into which you can pack a wide range of hardware types, which are very 
different from the original ISI. I mean, you could probably pack - to some 
extent, maybe sacrificing some features - other existing soc-camera 
drivers, like MX3, MX2, CEU,... - essentially those, using VB2. And I 
don't think that's a good idea. We have a class of V4L2 camera bridge 
drivers, that's fine. They use all the standard APIs to connect to the 
user-space and to other V4L2 drivers in video pipelines - V4L2 ioctl()s, 
subdev, Media Controller, VB2, V4L2 control API etc. Under that we have 
soc-camera - mainly for a few existing bridge drivers, because it takes a 
part of bridge driver's implementation freedom away and many or most 
modern camera bridge interfaces are more complex, than what soc-camera 
currently supports, and extending it makes little sense, it is just more 
logical to create a full-features V4L2 bridge driver with a full access to 
all relevant APIs. With your patches #12 and #13 you seem to be creating 
an even tighter, narrower API for very thin drivers. That just provide a 
couple of hardware-related functions and create a V4L2 bridge driver from 
that. What kind of hardware is that new controller, that you'd like to 
support by the same driver? Wouldn't it be better to create a new driver 
for it? Is it really similar to the ISI controller?

Thanks
Guennadi

> 
> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 33 ++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index 7d2e952..b4c1f38 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -37,7 +37,7 @@
>  #define FRAME_INTERVAL_MILLI_SEC	(1000 / MIN_FRAME_RATE)
>  
>  /* Frame buffer descriptor */
> -struct fbd {
> +struct fbd_isi_v2 {
>  	/* Physical address of the frame buffer */
>  	u32 fb_address;
>  	/* DMA Control Register(only in HISI2) */
> @@ -46,9 +46,13 @@ struct fbd {
>  	u32 next_fbd_address;
>  };
>  
> +union fbd {
> +	struct fbd_isi_v2 fbd_isi;
> +};
> +
>  struct isi_dma_desc {
>  	struct list_head list;
> -	struct fbd *p_fbd;
> +	union fbd *p_fbd;
>  	dma_addr_t fbd_phys;
>  };
>  
> @@ -69,7 +73,7 @@ struct atmel_isi {
>  	struct vb2_alloc_ctx		*alloc_ctx;
>  
>  	/* Allocate descriptors for dma buffer use */
> -	struct fbd			*p_fb_descriptors;
> +	union fbd			*p_fb_descriptors;
>  	dma_addr_t			fb_descriptors_phys;
>  	struct				list_head dma_desc_head;
>  	struct isi_dma_desc		dma_desc[MAX_BUFFER_NUM];
> @@ -396,6 +400,16 @@ static int buffer_init(struct vb2_buffer *vb)
>  	return 0;
>  }
>  
> +static void isi_hw_init_dma_desc(union fbd *p_fdb, u32 fb_addr,
> +				 u32 next_fbd_addr)
> +{
> +	struct fbd_isi_v2 *p = &(p_fdb->fbd_isi);

Superfluous parentheses

> +
> +	p->fb_address = fb_addr;
> +	p->next_fbd_address = next_fbd_addr;
> +	p->dma_ctrl = ISI_DMA_CTRL_WB;
> +}
> +
>  static int buffer_prepare(struct vb2_buffer *vb)
>  {
>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> @@ -428,10 +442,7 @@ static int buffer_prepare(struct vb2_buffer *vb)
>  			list_del_init(&desc->list);
>  
>  			/* Initialize the dma descriptor */
> -			desc->p_fbd->fb_address =
> -					vb2_dma_contig_plane_dma_addr(vb, 0);
> -			desc->p_fbd->next_fbd_address = 0;
> -			desc->p_fbd->dma_ctrl = ISI_DMA_CTRL_WB;
> +			isi_hw_init_dma_desc(desc->p_fbd, vb2_dma_contig_plane_dma_addr(vb, 0), 0);
>  
>  			buf->p_dma_desc = desc;
>  		}
> @@ -923,7 +934,7 @@ static int atmel_isi_remove(struct platform_device *pdev)
>  	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,
> +			sizeof(union fbd) * MAX_BUFFER_NUM,
>  			isi->p_fb_descriptors,
>  			isi->fb_descriptors_phys);
>  	pm_runtime_disable(&pdev->dev);
> @@ -1010,7 +1021,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
>  	INIT_LIST_HEAD(&isi->dma_desc_head);
>  
>  	isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
> -				sizeof(struct fbd) * MAX_BUFFER_NUM,
> +				sizeof(union fbd) * MAX_BUFFER_NUM,
>  				&isi->fb_descriptors_phys,
>  				GFP_KERNEL);
>  	if (!isi->p_fb_descriptors) {
> @@ -1021,7 +1032,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
>  	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);
> +					i * sizeof(union fbd);
>  		list_add(&isi->dma_desc[i].list, &isi->dma_desc_head);
>  	}
>  
> @@ -1080,7 +1091,7 @@ err_ioremap:
>  	vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
>  err_alloc_ctx:
>  	dma_free_coherent(&pdev->dev,
> -			sizeof(struct fbd) * MAX_BUFFER_NUM,
> +			sizeof(union fbd) * MAX_BUFFER_NUM,
>  			isi->p_fb_descriptors,
>  			isi->fb_descriptors_phys);
>  
> -- 
> 1.9.1
> 

Re: [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor)

[Josh Wu]

add all people in reply,

Hi, Guennadi

Thanks for the review.

2016-01-25 3:31 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> On Mon, 18 Jan 2016, Josh Wu wrote:
>
>> From: Josh Wu <josh.wu@atmel.com>
>>
>> This way, we can easy to add other type of fbd for new hardware.
>
> Ok, I've applied all your 13 patches to check, what the resulting driver
> would look like. To me it looks like you really abstract away _everything_
> remotely hardware-specific. What is left is yet another abstraction layer,
> into which you can pack a wide range of hardware types, which are very
> different from the original ISI. I mean, you could probably pack - to some
> extent, maybe sacrificing some features - other existing soc-camera
> drivers, like MX3, MX2, CEU,... - essentially those, using VB2. And I
> don't think that's a good idea. We have a class of V4L2 camera bridge
> drivers, that's fine. They use all the standard APIs to connect to the
> user-space and to other V4L2 drivers in video pipelines - V4L2 ioctl()s,
> subdev, Media Controller, VB2, V4L2 control API etc. Under that we have
> soc-camera - mainly for a few existing bridge drivers, because it takes a
> part of bridge driver's implementation freedom away and many or most
> modern camera bridge interfaces are more complex, than what soc-camera
> currently supports, and extending it makes little sense, it is just more
> logical to create a full-features V4L2 bridge driver with a full access to
> all relevant APIs.

It sounds the general v4l2 driver framework is more suitable than
soc-camera framework for the new hardware.
So is it easy for v4l2 platform driver to use the soc-camera sensors?

> With your patches #12 and #13 you seem to be creating
> an even tighter, narrower API for very thin drivers. That just provide a
> couple of hardware-related functions and create a V4L2 bridge driver from
> that. What kind of hardware is that new controller, that you'd like to
> support by the same driver? Wouldn't it be better to create a new driver
> for it? Is it really similar to the ISI controller?

The new hardware is SAMA5D2 Image Sensor Controller. You can find the
datasheet here:
http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf

Actually, The ISC hardware is very different from ISI hardware. ISC
has no Preivew/Codec path, it just has many data blocks to process
sensor data.
With the abstraction of my patches, ISC can rewrite the interrupt
handler, initialization, configure and etc to work in same ISI driver,
though. But like you mentioned, it's very tight, maybe it's not easy
to add extend functions.

So I was convinced to write a new v4l2 camera driver for ISC if it is
easy to support soc-camera sensors.

>
> Thanks
> Guennadi

Best Regards,
Josh Wu

>
>>
>> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
>> ---
>>
>>  drivers/media/platform/soc_camera/atmel-isi.c | 33 ++++++++++++++++++---------
>>  1 file changed, 22 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index 7d2e952..b4c1f38 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -37,7 +37,7 @@
>>  #define FRAME_INTERVAL_MILLI_SEC     (1000 / MIN_FRAME_RATE)
>>
>>  /* Frame buffer descriptor */
>> -struct fbd {
>> +struct fbd_isi_v2 {
>>       /* Physical address of the frame buffer */
>>       u32 fb_address;
>>       /* DMA Control Register(only in HISI2) */
>> @@ -46,9 +46,13 @@ struct fbd {
>>       u32 next_fbd_address;
>>  };
>>
>> +union fbd {
>> +     struct fbd_isi_v2 fbd_isi;
>> +};
>> +
>>  struct isi_dma_desc {
>>       struct list_head list;
>> -     struct fbd *p_fbd;
>> +     union fbd *p_fbd;
>>       dma_addr_t fbd_phys;
>>  };
>>
>> @@ -69,7 +73,7 @@ struct atmel_isi {
>>       struct vb2_alloc_ctx            *alloc_ctx;
>>
>>       /* Allocate descriptors for dma buffer use */
>> -     struct fbd                      *p_fb_descriptors;
>> +     union fbd                       *p_fb_descriptors;
>>       dma_addr_t                      fb_descriptors_phys;
>>       struct                          list_head dma_desc_head;
>>       struct isi_dma_desc             dma_desc[MAX_BUFFER_NUM];
>> @@ -396,6 +400,16 @@ static int buffer_init(struct vb2_buffer *vb)
>>       return 0;
>>  }
>>
>> +static void isi_hw_init_dma_desc(union fbd *p_fdb, u32 fb_addr,
>> +                              u32 next_fbd_addr)
>> +{
>> +     struct fbd_isi_v2 *p = &(p_fdb->fbd_isi);
>
> Superfluous parentheses
>
>> +
>> +     p->fb_address = fb_addr;
>> +     p->next_fbd_address = next_fbd_addr;
>> +     p->dma_ctrl = ISI_DMA_CTRL_WB;
>> +}
>> +
>>  static int buffer_prepare(struct vb2_buffer *vb)
>>  {
>>       struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> @@ -428,10 +442,7 @@ static int buffer_prepare(struct vb2_buffer *vb)
>>                       list_del_init(&desc->list);
>>
>>                       /* Initialize the dma descriptor */
>> -                     desc->p_fbd->fb_address =
>> -                                     vb2_dma_contig_plane_dma_addr(vb, 0);
>> -                     desc->p_fbd->next_fbd_address = 0;
>> -                     desc->p_fbd->dma_ctrl = ISI_DMA_CTRL_WB;
>> +                     isi_hw_init_dma_desc(desc->p_fbd, vb2_dma_contig_plane_dma_addr(vb, 0), 0);
>>
>>                       buf->p_dma_desc = desc;
>>               }
>> @@ -923,7 +934,7 @@ static int atmel_isi_remove(struct platform_device *pdev)
>>       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,
>> +                     sizeof(union fbd) * MAX_BUFFER_NUM,
>>                       isi->p_fb_descriptors,
>>                       isi->fb_descriptors_phys);
>>       pm_runtime_disable(&pdev->dev);
>> @@ -1010,7 +1021,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
>>       INIT_LIST_HEAD(&isi->dma_desc_head);
>>
>>       isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
>> -                             sizeof(struct fbd) * MAX_BUFFER_NUM,
>> +                             sizeof(union fbd) * MAX_BUFFER_NUM,
>>                               &isi->fb_descriptors_phys,
>>                               GFP_KERNEL);
>>       if (!isi->p_fb_descriptors) {
>> @@ -1021,7 +1032,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
>>       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);
>> +                                     i * sizeof(union fbd);
>>               list_add(&isi->dma_desc[i].list, &isi->dma_desc_head);
>>       }
>>
>> @@ -1080,7 +1091,7 @@ err_ioremap:
>>       vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
>>  err_alloc_ctx:
>>       dma_free_coherent(&pdev->dev,
>> -                     sizeof(struct fbd) * MAX_BUFFER_NUM,
>> +                     sizeof(union fbd) * MAX_BUFFER_NUM,
>>                       isi->p_fb_descriptors,
>>                       isi->fb_descriptors_phys);
>>
>> --
>> 1.9.1
>>

Re: [PATCH 07/13] atmel-isi: move hw code into isi_hw_initialize()

[Josh Wu]

Hi, Guennadi

Thanks for the reivew.

2016-01-25 2:09 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> On Mon, 18 Jan 2016, Josh Wu wrote:
>
>> That make hw operation code separate with general code.
>>
>> Also since reset action can be failed, so add a return value for
>> isi_hw_initialze().
>>
>> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
>> ---
>>
>>  drivers/media/platform/soc_camera/atmel-isi.c | 34 +++++++++++++++++----------
>>  1 file changed, 21 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index 4ddc309..ed4d04b 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -203,10 +203,27 @@ static int isi_hw_wait_status(struct atmel_isi *isi, int status_flag,
>>       return 0;
>>  }
>>
>> -static void isi_hw_initialize(struct atmel_isi *isi)
>> +static int isi_hw_initialize(struct atmel_isi *isi)
>>  {
>>       u32 common_flags = isi->bus_param;
>>       u32 cfg1 = 0;
>> +     int ret;
>> +
>> +     /* Reset ISI */
>> +     isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
>> +
>> +     /* Check Reset status */
>> +     ret  = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
>
> You could also remove the superfluous space while at it.

sure, I'll fix the duplicated space here.

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
>> +     if (ret) {
>> +             dev_err(isi->soc_host.icd->parent, "Reset ISI timed out\n");
>> +             return ret;
>> +     }
>> +
>> +     /* Disable all interrupts */
>> +     isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>> +
>> +     /* Clear any pending interrupt */
>> +     isi_readl(isi, ISI_STATUS);
>>
>>       /* set bus param for ISI */
>>       if (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
>> @@ -229,6 +246,8 @@ static void isi_hw_initialize(struct atmel_isi *isi)
>>
>>       isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>>       isi_writel(isi, ISI_CFG1, cfg1);
>> +
>> +     return 0;
>>  }
>>
>>  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>> @@ -453,27 +472,16 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>
>>       pm_runtime_get_sync(ici->v4l2_dev.dev);
>>
>> -     /* Reset ISI */
>> -     isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
>> -
>> -     /* Check Reset status */
>> -     ret  = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
>> +     ret = isi_hw_initialize(isi);
>>       if (ret) {
>> -             dev_err(icd->parent, "Reset ISI timed out\n");
>>               pm_runtime_put(ici->v4l2_dev.dev);
>>               return ret;
>>       }
>> -     /* Disable all interrupts */
>> -     isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>> -
>> -     isi_hw_initialize(isi);
>>
>>       configure_geometry(isi, icd->user_width, icd->user_height,
>>                               icd->current_fmt);
>>
>>       spin_lock_irq(&isi->lock);
>> -     /* Clear any pending interrupt */
>> -     isi_readl(isi, ISI_STATUS);
>>
>>       if (count)
>>               start_dma(isi, isi->active);
>> --
>> 1.9.1
>>

Re: [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor)

[Guennadi Liakhovetski]

Hi Josh,

(resending with all CC)

On Tue, 26 Jan 2016, Josh Wu wrote:

> Hi, Guennadi
> 
> Thanks for the review.
> 
> 2016-01-25 3:31 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > On Mon, 18 Jan 2016, Josh Wu wrote:
> >
> >> From: Josh Wu <josh.wu@atmel.com>
> >>
> >> This way, we can easy to add other type of fbd for new hardware.
> >
> > Ok, I've applied all your 13 patches to check, what the resulting driver
> > would look like. To me it looks like you really abstract away _everything_
> > remotely hardware-specific. What is left is yet another abstraction layer,
> > into which you can pack a wide range of hardware types, which are very
> > different from the original ISI. I mean, you could probably pack - to some
> > extent, maybe sacrificing some features - other existing soc-camera
> > drivers, like MX3, MX2, CEU,... - essentially those, using VB2. And I
> > don't think that's a good idea. We have a class of V4L2 camera bridge
> > drivers, that's fine. They use all the standard APIs to connect to the
> > user-space and to other V4L2 drivers in video pipelines - V4L2 ioctl()s,
> > subdev, Media Controller, VB2, V4L2 control API etc. Under that we have
> > soc-camera - mainly for a few existing bridge drivers, because it takes a
> > part of bridge driver's implementation freedom away and many or most
> > modern camera bridge interfaces are more complex, than what soc-camera
> > currently supports, and extending it makes little sense, it is just more
> > logical to create a full-features V4L2 bridge driver with a full access to
> > all relevant APIs.
> 
> It sounds the general v4l2 driver framework is more suitable than
> soc-camera framework for the new hardware.

Then, please, go for one!

> So is it easy for v4l2 platform driver to use the soc-camera sensors?

Not sure, haven't tried in a while. It used to be difficult, but it must 
have become more simple, I think there are examples of that in the 
mainline. I think em28xx does that, but probably in the meantime the 
integration possibilities have become even better.

> > With your patches #12 and #13 you seem to be creating
> > an even tighter, narrower API for very thin drivers. That just provide a
> > couple of hardware-related functions and create a V4L2 bridge driver from
> > that. What kind of hardware is that new controller, that you'd like to
> > support by the same driver? Wouldn't it be better to create a new driver
> > for it? Is it really similar to the ISI controller?
> 
> The new hardware is SAMA5D2 Image Sensor Controller. You can find the
> datasheet here:
> http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf
> 
> Actually, The ISC hardware is very different from ISI hardware. ISC
> has no Preivew/Codec path, it just has many data blocks to process
> sensor data.
> With the abstraction of my patches, ISC can rewrite the interrupt
> handler, initialization, configure and etc to work in same ISI driver,
> though. But like you mentioned, it's very tight, maybe it's not easy
> to add extend functions.
> 
> So I was convinced to write a new v4l2 camera driver for ISC if it is
> easy to support soc-camera sensors.

Please, write a new driver :)

Thanks
Guennadi

> Best Regards,
> Josh Wu

Re: [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt

[Josh Wu]

Hi, Guennadi

2016-01-25 0:58 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> On Mon, 18 Jan 2016, Josh Wu wrote:
>
>> In current code, we use a interrupt to check whether ISI reset/disable
>> action is done. Actually, we also can check ISI SR to check the
>> reset/disable action by polling, and it is simpler and straight forward.
>>
>> So this patch use isi_hw_wait_status() to check the action status. As
>> the interrupt checking the status is useless, so just remove the interrupt
>> & completion code.
>
> Sorry, I'm not convinced. Switching from interrupt-driven to polling seems
> counter-productive to me. Why do you want this?

Yes, it is counter-productive If such code is called very frequently.
But here we only used for reset and disable the hardware.

I think it is more straightforward and simple when we just check
status register after write the reset or disable the ISI control
register. Using interrupt here is over designed.
for the sake of performance I should use cpu_releax() instead of
msleep() when waiting for the status bit.

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
>>
>> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
>> ---
>>
>>  drivers/media/platform/soc_camera/atmel-isi.c | 59 ++++++---------------------
>>  1 file changed, 13 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index f0508ea..4ddc309 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -12,7 +12,6 @@
>>   */
>>
>>  #include <linux/clk.h>
>> -#include <linux/completion.h>
>>  #include <linux/delay.h>
>>  #include <linux/fs.h>
>>  #include <linux/init.h>
>> @@ -81,7 +80,6 @@ struct atmel_isi {
>>       struct isi_dma_desc             dma_desc[MAX_BUFFER_NUM];
>>       bool                            enable_preview_path;
>>
>> -     struct completion               complete;
>>       /* ISI peripherial clock */
>>       struct clk                      *pclk;
>>       unsigned int                    irq;
>> @@ -281,51 +279,14 @@ static irqreturn_t isi_interrupt(int irq, void *dev_id)
>>       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 (likely(pending & ISI_SR_CXFR_DONE) ||
>> -                             likely(pending & ISI_SR_PXFR_DONE))
>> -                     ret = atmel_isi_handle_streaming(isi);
>> -     }
>> +     if (likely(pending & ISI_SR_CXFR_DONE) ||
>> +         likely(pending & ISI_SR_PXFR_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(500));
>> -     if (timeout == 0)
>> -             return -ETIMEDOUT;
>> -
>> -     return 0;
>> -}
>> -
>>  /* ------------------------------------------------------------------
>>       Videobuf operations
>>     ------------------------------------------------------------------*/
>> @@ -493,8 +454,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>       pm_runtime_get_sync(ici->v4l2_dev.dev);
>>
>>       /* Reset ISI */
>> -     ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
>> -     if (ret < 0) {
>> +     isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
>> +
>> +     /* Check Reset status */
>> +     ret  = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
>> +     if (ret) {
>>               dev_err(icd->parent, "Reset ISI timed out\n");
>>               pm_runtime_put(ici->v4l2_dev.dev);
>>               return ret;
>> @@ -549,8 +513,11 @@ static void stop_streaming(struct vb2_queue *vq)
>>                       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)
>> +     isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> +
>> +     /* Check Reset status */
>> +     ret  = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
>> +     if (ret)
>>               dev_err(icd->parent, "Disable ISI timed out\n");
>>
>>       pm_runtime_put(ici->v4l2_dev.dev);
>> --
>> 1.9.1
>>

Re: [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor)

[Josh Wu]

Hi, Guennadi

2016-01-26 22:10 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> Hi Josh,
>
> (resending with all CC)
>
> On Tue, 26 Jan 2016, Josh Wu wrote:
>
>> Hi, Guennadi
>>
>> Thanks for the review.
>>
>> 2016-01-25 3:31 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
>> > On Mon, 18 Jan 2016, Josh Wu wrote:
>> >
>> >> From: Josh Wu <josh.wu@atmel.com>
>> >>
>> >> This way, we can easy to add other type of fbd for new hardware.
>> >
>> > Ok, I've applied all your 13 patches to check, what the resulting driver
>> > would look like. To me it looks like you really abstract away _everything_
>> > remotely hardware-specific. What is left is yet another abstraction layer,
>> > into which you can pack a wide range of hardware types, which are very
>> > different from the original ISI. I mean, you could probably pack - to some
>> > extent, maybe sacrificing some features - other existing soc-camera
>> > drivers, like MX3, MX2, CEU,... - essentially those, using VB2. And I
>> > don't think that's a good idea. We have a class of V4L2 camera bridge
>> > drivers, that's fine. They use all the standard APIs to connect to the
>> > user-space and to other V4L2 drivers in video pipelines - V4L2 ioctl()s,
>> > subdev, Media Controller, VB2, V4L2 control API etc. Under that we have
>> > soc-camera - mainly for a few existing bridge drivers, because it takes a
>> > part of bridge driver's implementation freedom away and many or most
>> > modern camera bridge interfaces are more complex, than what soc-camera
>> > currently supports, and extending it makes little sense, it is just more
>> > logical to create a full-features V4L2 bridge driver with a full access to
>> > all relevant APIs.
>>
>> It sounds the general v4l2 driver framework is more suitable than
>> soc-camera framework for the new hardware.
>
> Then, please, go for one!
>
>> So is it easy for v4l2 platform driver to use the soc-camera sensors?
>
> Not sure, haven't tried in a while. It used to be difficult, but it must
> have become more simple, I think there are examples of that in the
> mainline. I think em28xx does that, but probably in the meantime the
> integration possibilities have become even better.

I'll try to see what I'll get.

>
>> > With your patches #12 and #13 you seem to be creating
>> > an even tighter, narrower API for very thin drivers. That just provide a
>> > couple of hardware-related functions and create a V4L2 bridge driver from
>> > that. What kind of hardware is that new controller, that you'd like to
>> > support by the same driver? Wouldn't it be better to create a new driver
>> > for it? Is it really similar to the ISI controller?
>>
>> The new hardware is SAMA5D2 Image Sensor Controller. You can find the
>> datasheet here:
>> http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf
>>
>> Actually, The ISC hardware is very different from ISI hardware. ISC
>> has no Preivew/Codec path, it just has many data blocks to process
>> sensor data.
>> With the abstraction of my patches, ISC can rewrite the interrupt
>> handler, initialization, configure and etc to work in same ISI driver,
>> though. But like you mentioned, it's very tight, maybe it's not easy
>> to add extend functions.
>>
>> So I was convinced to write a new v4l2 camera driver for ISC if it is
>> easy to support soc-camera sensors.
>
> Please, write a new driver :)

Ok, so for this serial patch, I will drop the abstract interface
relevant patches, and keep the refactor patch in v2. Thanks.

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
>> Best Regards,
>> Josh Wu

Re: [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt

[Guennadi Liakhovetski]

On Tue, 26 Jan 2016, Josh Wu wrote:

> Hi, Guennadi
> 
> 2016-01-25 0:58 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > On Mon, 18 Jan 2016, Josh Wu wrote:
> >
> >> In current code, we use a interrupt to check whether ISI reset/disable
> >> action is done. Actually, we also can check ISI SR to check the
> >> reset/disable action by polling, and it is simpler and straight forward.
> >>
> >> So this patch use isi_hw_wait_status() to check the action status. As
> >> the interrupt checking the status is useless, so just remove the interrupt
> >> & completion code.
> >
> > Sorry, I'm not convinced. Switching from interrupt-driven to polling seems
> > counter-productive to me. Why do you want this?
> 
> Yes, it is counter-productive If such code is called very frequently.
> But here we only used for reset and disable the hardware.
> 
> I think it is more straightforward and simple when we just check
> status register after write the reset or disable the ISI control
> register. Using interrupt here is over designed.

I still don't see how you're gaining from this. If you were writing new 
code and you would say: "you know, this path will only be hit rarely, we 
don't know whether interrupt would work, but we have here this simple 
polling solution, that works" I would understand. But you already have a 
technically-superior solution in place! And it works, right? Why change 
it? On the one hand it's your hardware and it's up to you which direction 
to develop it, and my task is just to help you integrate your work into 
the mainline. On the other hand I really don't see any benefits, sorry. 
I'd prefer not to do this unless you can explain the benefits.

> for the sake of performance I should use cpu_releax() instead of
> msleep() when waiting for the status bit.

Don't think that's a good idea. cpu_relax() allows the scheduler to switch 
over to a different task, but if none is active, it'll come back instead 
of going to sleep. Unless of course your event really kicks in very 
quickly - much faster than 1ms. But depending on your architecture 
implementation, msleep(1) might also be implemented as a busy look, then 
cpu_relax() would be better.

Thanks
Guennadi

> 
> Best Regards,
> Josh Wu
> 
> >
> > Thanks
> > Guennadi
> >
> >>
> >> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
> >> ---
> >>
> >>  drivers/media/platform/soc_camera/atmel-isi.c | 59 ++++++---------------------
> >>  1 file changed, 13 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> >> index f0508ea..4ddc309 100644
> >> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> >> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> >> @@ -12,7 +12,6 @@
> >>   */
> >>
> >>  #include <linux/clk.h>
> >> -#include <linux/completion.h>
> >>  #include <linux/delay.h>
> >>  #include <linux/fs.h>
> >>  #include <linux/init.h>
> >> @@ -81,7 +80,6 @@ struct atmel_isi {
> >>       struct isi_dma_desc             dma_desc[MAX_BUFFER_NUM];
> >>       bool                            enable_preview_path;
> >>
> >> -     struct completion               complete;
> >>       /* ISI peripherial clock */
> >>       struct clk                      *pclk;
> >>       unsigned int                    irq;
> >> @@ -281,51 +279,14 @@ static irqreturn_t isi_interrupt(int irq, void *dev_id)
> >>       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 (likely(pending & ISI_SR_CXFR_DONE) ||
> >> -                             likely(pending & ISI_SR_PXFR_DONE))
> >> -                     ret = atmel_isi_handle_streaming(isi);
> >> -     }
> >> +     if (likely(pending & ISI_SR_CXFR_DONE) ||
> >> +         likely(pending & ISI_SR_PXFR_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(500));
> >> -     if (timeout == 0)
> >> -             return -ETIMEDOUT;
> >> -
> >> -     return 0;
> >> -}
> >> -
> >>  /* ------------------------------------------------------------------
> >>       Videobuf operations
> >>     ------------------------------------------------------------------*/
> >> @@ -493,8 +454,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
> >>       pm_runtime_get_sync(ici->v4l2_dev.dev);
> >>
> >>       /* Reset ISI */
> >> -     ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
> >> -     if (ret < 0) {
> >> +     isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
> >> +
> >> +     /* Check Reset status */
> >> +     ret  = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
> >> +     if (ret) {
> >>               dev_err(icd->parent, "Reset ISI timed out\n");
> >>               pm_runtime_put(ici->v4l2_dev.dev);
> >>               return ret;
> >> @@ -549,8 +513,11 @@ static void stop_streaming(struct vb2_queue *vq)
> >>                       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)
> >> +     isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> >> +
> >> +     /* Check Reset status */
> >> +     ret  = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
> >> +     if (ret)
> >>               dev_err(icd->parent, "Disable ISI timed out\n");
> >>
> >>       pm_runtime_put(ici->v4l2_dev.dev);
> >> --
> >> 1.9.1
> >>
> 

Re: [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor)

[Guennadi Liakhovetski]

On Tue, 26 Jan 2016, Josh Wu wrote:

> Ok, so for this serial patch, I will drop the abstract interface
> relevant patches, and keep the refactor patch in v2. Thanks.

Yes, please, drop the last 2 patches, and, preferably, the "polling 
instead of IRQ" one too.

Thanks
Guennadi