[PATCH 1/5] m5mols: Add more functions to check busy status

[PATCH 1/5] m5mols: Add more functions to check busy status

[HeungJun, Kim]

This patch add 3 type of checking busy status functions. 1) Keep busy-loop
ignoring the error of I2C negative return, 2) Provide the read status value
from I2C to the caller, 3) Compare masked value with desire value.

Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/m5mols/m5mols.h      |    1 -
 drivers/media/video/m5mols/m5mols_core.c |   43 ++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/media/video/m5mols/m5mols.h b/drivers/media/video/m5mols/m5mols.h
index 89d09a8..c8e1572 100644
--- a/drivers/media/video/m5mols/m5mols.h
+++ b/drivers/media/video/m5mols/m5mols.h
@@ -259,7 +259,6 @@ int m5mols_read_u8(struct v4l2_subdev *sd, u32 reg_comb, u8 *val);
 int m5mols_read_u16(struct v4l2_subdev *sd, u32 reg_comb, u16 *val);
 int m5mols_read_u32(struct v4l2_subdev *sd, u32 reg_comb, u32 *val);
 int m5mols_write(struct v4l2_subdev *sd, u32 reg_comb, u32 val);
-int m5mols_busy(struct v4l2_subdev *sd, u8 category, u8 cmd, u8 value);
 
 /*
  * Mode operation of the M-5MOLS
diff --git a/drivers/media/video/m5mols/m5mols_core.c b/drivers/media/video/m5mols/m5mols_core.c
index 5d21d05..73db96e 100644
--- a/drivers/media/video/m5mols/m5mols_core.c
+++ b/drivers/media/video/m5mols/m5mols_core.c
@@ -271,22 +271,49 @@ int m5mols_write(struct v4l2_subdev *sd, u32 reg, u32 val)
 	return 0;
 }
 
-int m5mols_busy(struct v4l2_subdev *sd, u8 category, u8 cmd, u8 mask)
+/**
+ * __m5mols_busy - Base function of checking busy status.
+ * @use: use return value for checking I2C err, or keep the loop until RETRY.
+ * @mask: mask the I2C value for comparing desire value, or do not mask.
+ * @desire: this is compared with the I2C value.
+ *
+ * The M-5MOLS use frequently polling method for checking registers.
+ */
+int __m5mols_busy(struct v4l2_subdev *sd, bool use, u8 mask, u8 desire,
+		  u8 *value, u8 category, u8 cmd)
 {
-	u8 busy;
 	int i;
 	int ret;
 
 	for (i = 0; i < M5MOLS_I2C_CHECK_RETRY; i++) {
-		ret = m5mols_read_u8(sd, I2C_REG(category, cmd, 1), &busy);
-		if (ret < 0)
+		ret = m5mols_read_u8(sd, I2C_REG(category, cmd, 1), value);
+
+		if ((use ? ret : 0) < 0)
 			return ret;
-		if ((busy & mask) == mask)
+		if ((mask ? (*value & mask) : *value) == desire)
 			return 0;
 	}
 	return -EBUSY;
 }
 
+int m5mols_busy(struct v4l2_subdev *sd, u8 desire, u8 category, u8 cmd)
+{
+	u8 dummy;
+	return __m5mols_busy(sd, false, 0x0, desire, &dummy, category, cmd);
+}
+
+int m5mols_busy_val(struct v4l2_subdev *sd, u8 desire, u8 *value,
+		    u8 category, u8 cmd)
+{
+	return __m5mols_busy(sd, false, 0x0, desire, value, category, cmd);
+}
+
+int m5mols_busy_mask(struct v4l2_subdev *sd, u8 desire, u8 category, u8 cmd)
+{
+	u8 dummy;
+	return __m5mols_busy(sd, true, desire, desire, &dummy, category, cmd);
+}
+
 /**
  * m5mols_enable_interrupt - Clear interrupt pending bits and unmask interrupts
  *
@@ -316,7 +343,7 @@ static int m5mols_reg_mode(struct v4l2_subdev *sd, u8 mode)
 {
 	int ret = m5mols_write(sd, SYSTEM_SYSMODE, mode);
 
-	return ret ? ret : m5mols_busy(sd, CAT_SYSTEM, CAT0_SYSMODE, mode);
+	return ret ? ret : m5mols_busy_mask(sd, mode, CAT_SYSTEM, CAT0_SYSMODE);
 }
 
 /**
@@ -832,8 +859,8 @@ static int m5mols_s_power(struct v4l2_subdev *sd, int on)
 		if (!ret)
 			ret = m5mols_write(sd, AF_MODE, REG_AF_POWEROFF);
 		if (!ret)
-			ret = m5mols_busy(sd, CAT_SYSTEM, CAT0_STATUS,
-					REG_AF_IDLE);
+			ret = m5mols_busy_mask(sd, REG_AF_IDLE,
+						CAT_SYSTEM, CAT0_STATUS);
 		if (!ret)
 			v4l2_info(sd, "Success soft-landing lens\n");
 	}
-- 
1.7.4.1

[PATCH 2/5] m5mols: Replace IRQ workqueue to waitqueue only

[HeungJun, Kim]

In M-5MOLS driver, the workqueue code for IRQ is hard to re-use. So, remove
the IRQ workqueue, and use only waitqueue for waiting IRQ with timeout.
The info->issue has the status that interrupt is issued or not, then
the info->interrupt has the IRQ status register at that time.

Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/m5mols/m5mols.h         |    7 +--
 drivers/media/video/m5mols/m5mols_capture.c |   34 ++-------------
 drivers/media/video/m5mols/m5mols_core.c    |   60 +++++++++++----------------
 3 files changed, 32 insertions(+), 69 deletions(-)

diff --git a/drivers/media/video/m5mols/m5mols.h b/drivers/media/video/m5mols/m5mols.h
index c8e1572..75f7984 100644
--- a/drivers/media/video/m5mols/m5mols.h
+++ b/drivers/media/video/m5mols/m5mols.h
@@ -164,7 +164,6 @@ struct m5mols_version {
  * @res_type: current resolution type
  * @code: current code
  * @irq_waitq: waitqueue for the capture
- * @work_irq: workqueue for the IRQ
  * @flags: state variable for the interrupt handler
  * @handle: control handler
  * @autoexposure: Auto Exposure control
@@ -181,6 +180,7 @@ struct m5mols_version {
  * @lock_ae: true means the Auto Exposure is locked
  * @lock_awb: true means the Aut WhiteBalance is locked
  * @resolution:	register value for current resolution
+ * @issue: "true" means the M-5MOLS sensor's interrupt issued
  * @interrupt: register value for current interrupt status
  * @mode: register value for current operation mode
  * @mode_save: register value for current operation mode for saving
@@ -194,7 +194,6 @@ struct m5mols_info {
 	int res_type;
 	enum v4l2_mbus_pixelcode code;
 	wait_queue_head_t irq_waitq;
-	struct work_struct work_irq;
 	unsigned long flags;
 
 	struct v4l2_ctrl_handler handle;
@@ -211,6 +210,7 @@ struct m5mols_info {
 	struct m5mols_version ver;
 	struct m5mols_capture cap;
 	bool power;
+	bool issue;
 	bool ctrl_sync;
 	bool lock_ae;
 	bool lock_awb;
@@ -221,8 +221,6 @@ struct m5mols_info {
 	int (*set_power)(struct device *dev, int on);
 };
 
-#define ST_CAPT_IRQ 0
-
 #define is_powered(__info) (__info->power)
 #define is_ctrl_synced(__info) (__info->ctrl_sync)
 #define is_available_af(__info)	(__info->ver.af)
@@ -283,6 +281,7 @@ int m5mols_write(struct v4l2_subdev *sd, u32 reg_comb, u32 val);
 int m5mols_mode(struct m5mols_info *info, u8 mode);
 
 int m5mols_enable_interrupt(struct v4l2_subdev *sd, u8 reg);
+int m5mols_timeout_interrupt(struct v4l2_subdev *sd, u8 condition, u32 timeout);
 int m5mols_sync_controls(struct m5mols_info *info);
 int m5mols_start_capture(struct m5mols_info *info);
 int m5mols_do_scenemode(struct m5mols_info *info, u8 mode);
diff --git a/drivers/media/video/m5mols/m5mols_capture.c b/drivers/media/video/m5mols/m5mols_capture.c
index 3248ac8..18a56bf 100644
--- a/drivers/media/video/m5mols/m5mols_capture.c
+++ b/drivers/media/video/m5mols/m5mols_capture.c
@@ -29,22 +29,6 @@
 #include "m5mols.h"
 #include "m5mols_reg.h"
 
-static int m5mols_capture_error_handler(struct m5mols_info *info,
-					int timeout)
-{
-	int ret;
-
-	/* Disable all interrupts and clear relevant interrupt staus bits */
-	ret = m5mols_write(&info->sd, SYSTEM_INT_ENABLE,
-			   info->interrupt & ~(REG_INT_CAPTURE));
-	if (ret)
-		return ret;
-
-	if (timeout == 0)
-		return -ETIMEDOUT;
-
-	return 0;
-}
 /**
  * m5mols_read_rational - I2C read of a rational number
  *
@@ -121,7 +105,6 @@ int m5mols_start_capture(struct m5mols_info *info)
 {
 	struct v4l2_subdev *sd = &info->sd;
 	u8 resolution = info->resolution;
-	int timeout;
 	int ret;
 
 	/*
@@ -142,14 +125,9 @@ int m5mols_start_capture(struct m5mols_info *info)
 		ret = m5mols_enable_interrupt(sd, REG_INT_CAPTURE);
 	if (!ret)
 		ret = m5mols_mode(info, REG_CAPTURE);
-	if (!ret) {
+	if (!ret)
 		/* Wait for capture interrupt, after changing capture mode */
-		timeout = wait_event_interruptible_timeout(info->irq_waitq,
-					   test_bit(ST_CAPT_IRQ, &info->flags),
-					   msecs_to_jiffies(2000));
-		if (test_and_clear_bit(ST_CAPT_IRQ, &info->flags))
-			ret = m5mols_capture_error_handler(info, timeout);
-	}
+		ret = m5mols_timeout_interrupt(sd, REG_INT_CAPTURE, 2000);
 	if (!ret)
 		ret = m5mols_lock_3a(info, false);
 	if (ret)
@@ -175,15 +153,13 @@ int m5mols_start_capture(struct m5mols_info *info)
 		ret = m5mols_write(sd, CAPC_START, REG_CAP_START_MAIN);
 	if (!ret) {
 		/* Wait for the capture completion interrupt */
-		timeout = wait_event_interruptible_timeout(info->irq_waitq,
-					   test_bit(ST_CAPT_IRQ, &info->flags),
-					   msecs_to_jiffies(2000));
-		if (test_and_clear_bit(ST_CAPT_IRQ, &info->flags)) {
+		ret = m5mols_timeout_interrupt(sd, REG_INT_CAPTURE, 2000);
+		if (!ret) {
 			ret = m5mols_capture_info(info);
 			if (!ret)
 				v4l2_subdev_notify(sd, 0, &info->cap.total);
 		}
 	}
 
-	return m5mols_capture_error_handler(info, timeout);
+	return ret;
 }
diff --git a/drivers/media/video/m5mols/m5mols_core.c b/drivers/media/video/m5mols/m5mols_core.c
index 73db96e..f3b9415 100644
--- a/drivers/media/video/m5mols/m5mols_core.c
+++ b/drivers/media/video/m5mols/m5mols_core.c
@@ -333,6 +333,28 @@ int m5mols_enable_interrupt(struct v4l2_subdev *sd, u8 reg)
 	return ret;
 }
 
+/* m5mols_timeout_interrupt - Wait interrupt and figure out which interrupt. */
+int m5mols_timeout_interrupt(struct v4l2_subdev *sd, u8 condition, u32 timeout)
+{
+	struct m5mols_info *info = to_m5mols(sd);
+	u8 reg;
+	int timed;
+	int ret;
+
+	timed = wait_event_interruptible_timeout(info->irq_waitq,
+			info->issue, msecs_to_jiffies(timeout));
+	if (!timed)
+		return -ETIMEDOUT;
+
+	ret = m5mols_busy_val(sd, condition, &reg, CAT_SYSTEM, CAT0_INT_FACTOR);
+	if (ret || (!ret && reg != condition))
+		return -EINVAL;
+
+	info->interrupt = reg;
+	info->issue = false;
+	return 0;
+}
+
 /**
  * m5mols_reg_mode - Write the mode and check busy status
  *
@@ -901,46 +923,13 @@ static const struct v4l2_subdev_ops m5mols_ops = {
 	.video		= &m5mols_video_ops,
 };
 
-static void m5mols_irq_work(struct work_struct *work)
-{
-	struct m5mols_info *info =
-		container_of(work, struct m5mols_info, work_irq);
-	struct v4l2_subdev *sd = &info->sd;
-	u8 reg;
-	int ret;
-
-	if (!is_powered(info) ||
-			m5mols_read_u8(sd, SYSTEM_INT_FACTOR, &info->interrupt))
-		return;
-
-	switch (info->interrupt & REG_INT_MASK) {
-	case REG_INT_AF:
-		if (!is_available_af(info))
-			break;
-		ret = m5mols_read_u8(sd, AF_STATUS, &reg);
-		v4l2_dbg(2, m5mols_debug, sd, "AF %s\n",
-			 reg == REG_AF_FAIL ? "Failed" :
-			 reg == REG_AF_SUCCESS ? "Success" :
-			 reg == REG_AF_IDLE ? "Idle" : "Busy");
-		break;
-	case REG_INT_CAPTURE:
-		if (!test_and_set_bit(ST_CAPT_IRQ, &info->flags))
-			wake_up_interruptible(&info->irq_waitq);
-
-		v4l2_dbg(2, m5mols_debug, sd, "CAPTURE\n");
-		break;
-	default:
-		v4l2_dbg(2, m5mols_debug, sd, "Undefined: %02x\n", reg);
-		break;
-	};
-}
-
 static irqreturn_t m5mols_irq_handler(int irq, void *data)
 {
 	struct v4l2_subdev *sd = data;
 	struct m5mols_info *info = to_m5mols(sd);
 
-	schedule_work(&info->work_irq);
+	info->issue = true;
+	wake_up_interruptible(&info->irq_waitq);
 
 	return IRQ_HANDLED;
 }
@@ -999,7 +988,6 @@ static int __devinit m5mols_probe(struct i2c_client *client,
 	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
 
 	init_waitqueue_head(&info->irq_waitq);
-	INIT_WORK(&info->work_irq, m5mols_irq_work);
 	ret = request_irq(client->irq, m5mols_irq_handler,
 			  IRQF_TRIGGER_RISING, MODULE_NAME, sd);
 	if (ret) {
-- 
1.7.4.1

[PATCH 3/5] m5mols: Support for interrupt in the sensor's booting time

[HeungJun, Kim]

The M-5MOLS suports 2 booting ways. 1) waiting specific delay(over 520ms),
or 2) waiting with interrupt. The way of interrupt type supports optimum delay
for booting by waiting interrupt. Also, in case of using this way, it doesn't
need the extra delay in the m5mols_sensor_power() for stabilization.

Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/m5mols/m5mols_core.c |   16 ++++++++++------
 drivers/media/video/m5mols/m5mols_reg.h  |    3 ++-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/media/video/m5mols/m5mols_core.c b/drivers/media/video/m5mols/m5mols_core.c
index f3b9415..24e66ad 100644
--- a/drivers/media/video/m5mols/m5mols_core.c
+++ b/drivers/media/video/m5mols/m5mols_core.c
@@ -738,7 +738,6 @@ static int m5mols_sensor_power(struct m5mols_info *info, bool enable)
 		}
 
 		gpio_set_value(pdata->gpio_reset, !pdata->reset_polarity);
-		usleep_range(1000, 1000);
 		info->power = true;
 
 		return ret;
@@ -755,7 +754,6 @@ static int m5mols_sensor_power(struct m5mols_info *info, bool enable)
 		info->set_power(&client->dev, 0);
 
 	gpio_set_value(pdata->gpio_reset, pdata->reset_polarity);
-	usleep_range(1000, 1000);
 	info->power = false;
 
 	return ret;
@@ -773,18 +771,24 @@ int __attribute__ ((weak)) m5mols_update_fw(struct v4l2_subdev *sd,
  *
  * Booting internal ARM core makes the M-5MOLS is ready for getting commands
  * with I2C. It's the first thing to be done after it powered up. It must wait
- * at least 520ms recommended by M-5MOLS datasheet, after executing arm booting.
+ * at least 520ms recommended by M-5MOLS datasheet. Otherwise we also can check
+ * the register CATF_CAM_START is still in FLASH_MODE or not, after sensor's
+ * I2C transfer status is stabled and we write REG_START_ARM_BOOT command
+ * on the CAT_FLASH category.
  */
 static int m5mols_sensor_armboot(struct v4l2_subdev *sd)
 {
 	int ret;
 
-	ret = m5mols_write(sd, FLASH_CAM_START, REG_START_ARM_BOOT);
+	/* Execute ARM boot sequence */
+	ret = m5mols_busy(sd, REG_IN_FLASH_MODE, CAT_FLASH, CATF_CAM_START);
+	if (!ret)
+		ret = m5mols_write(sd, FLASH_CAM_START, REG_START_ARM_BOOT);
+	if (!ret)
+		ret = m5mols_timeout_interrupt(sd, REG_INT_MODE, 2000);
 	if (ret < 0)
 		return ret;
 
-	msleep(520);
-
 	ret = m5mols_get_version(sd);
 	if (!ret)
 		ret = m5mols_update_fw(sd, m5mols_sensor_power);
diff --git a/drivers/media/video/m5mols/m5mols_reg.h b/drivers/media/video/m5mols/m5mols_reg.h
index c755bd6..533aa27 100644
--- a/drivers/media/video/m5mols/m5mols_reg.h
+++ b/drivers/media/video/m5mols/m5mols_reg.h
@@ -405,6 +405,7 @@
 					 * after power-up */
 
 #define FLASH_CAM_START		I2C_REG(CAT_FLASH, CATF_CAM_START, 1)
-#define REG_START_ARM_BOOT	0x01
+#define REG_START_ARM_BOOT	0x01	/* value in case of writing */
+#define REG_IN_FLASH_MODE	0x00	/* value in case of reading */
 
 #endif	/* M5MOLS_REG_H */
-- 
1.7.4.1

[PATCH 4/5] m5mols: Add boot flag for not showing the msg of I2C error

[HeungJun, Kim]

In M-5MOLS sensor, the I2C error can be occured before sensor booting done,
becase I2C interface is not stabilized although the sensor have done already
for booting, so the right value is deliver through I2C interface. In case,
it needs to make the I2C error msg not to be printed.

Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/m5mols/m5mols.h      |    2 ++
 drivers/media/video/m5mols/m5mols_core.c |   17 +++++++++++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/m5mols/m5mols.h b/drivers/media/video/m5mols/m5mols.h
index 75f7984..0d7e202 100644
--- a/drivers/media/video/m5mols/m5mols.h
+++ b/drivers/media/video/m5mols/m5mols.h
@@ -175,6 +175,7 @@ struct m5mols_version {
  * @ver: information of the version
  * @cap: the capture mode attributes
  * @power: current sensor's power status
+ * @boot: "true" means the M-5MOLS sensor done ARM Booting
  * @ctrl_sync: true means all controls of the sensor are initialized
  * @int_capture: true means the capture interrupt is issued once
  * @lock_ae: true means the Auto Exposure is locked
@@ -210,6 +211,7 @@ struct m5mols_info {
 	struct m5mols_version ver;
 	struct m5mols_capture cap;
 	bool power;
+	bool boot;
 	bool issue;
 	bool ctrl_sync;
 	bool lock_ae;
diff --git a/drivers/media/video/m5mols/m5mols_core.c b/drivers/media/video/m5mols/m5mols_core.c
index 24e66ad..0aae868 100644
--- a/drivers/media/video/m5mols/m5mols_core.c
+++ b/drivers/media/video/m5mols/m5mols_core.c
@@ -138,6 +138,7 @@ static u32 m5mols_swap_byte(u8 *data, u8 length)
 static int m5mols_read(struct v4l2_subdev *sd, u32 size, u32 reg, u32 *val)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct m5mols_info *info = to_m5mols(sd);
 	u8 rbuf[M5MOLS_I2C_MAX_SIZE + 1];
 	u8 category = I2C_CATEGORY(reg);
 	u8 cmd = I2C_COMMAND(reg);
@@ -168,8 +169,10 @@ static int m5mols_read(struct v4l2_subdev *sd, u32 size, u32 reg, u32 *val)
 
 	ret = i2c_transfer(client->adapter, msg, 2);
 	if (ret < 0) {
-		v4l2_err(sd, "read failed: size:%d cat:%02x cmd:%02x. %d\n",
-			 size, category, cmd, ret);
+		if (info->boot)
+			v4l2_err(sd,
+				"read failed: cat:%02x cmd:%02x ret:%d\n",
+				category, cmd, ret);
 		return ret;
 	}
 
@@ -232,6 +235,7 @@ int m5mols_read_u32(struct v4l2_subdev *sd, u32 reg, u32 *val)
 int m5mols_write(struct v4l2_subdev *sd, u32 reg, u32 val)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct m5mols_info *info = to_m5mols(sd);
 	u8 wbuf[M5MOLS_I2C_MAX_SIZE + 4];
 	u8 category = I2C_CATEGORY(reg);
 	u8 cmd = I2C_COMMAND(reg);
@@ -263,8 +267,10 @@ int m5mols_write(struct v4l2_subdev *sd, u32 reg, u32 val)
 
 	ret = i2c_transfer(client->adapter, msg, 1);
 	if (ret < 0) {
-		v4l2_err(sd, "write failed: size:%d cat:%02x cmd:%02x. %d\n",
-			size, category, cmd, ret);
+		if (info->boot)
+			v4l2_err(sd,
+				"write failed: cat:%02x cmd:%02x ret:%d\n",
+				category, cmd, ret);
 		return ret;
 	}
 
@@ -778,6 +784,7 @@ int __attribute__ ((weak)) m5mols_update_fw(struct v4l2_subdev *sd,
  */
 static int m5mols_sensor_armboot(struct v4l2_subdev *sd)
 {
+	struct m5mols_info *info = to_m5mols(sd);
 	int ret;
 
 	/* Execute ARM boot sequence */
@@ -786,6 +793,8 @@ static int m5mols_sensor_armboot(struct v4l2_subdev *sd)
 		ret = m5mols_write(sd, FLASH_CAM_START, REG_START_ARM_BOOT);
 	if (!ret)
 		ret = m5mols_timeout_interrupt(sd, REG_INT_MODE, 2000);
+	if (!ret)
+		info->boot = true;
 	if (ret < 0)
 		return ret;
 
-- 
1.7.4.1

[PATCH 5/5] m5mols: Relocation the order and count for CAPTURE interrupt

[HeungJun, Kim]

The double enabling CAPTURE interrupt is not needed in m5mols_start_capture(),
so remove these, and add one at the only booting time once. Also, fix the order
of CAPTURE sequence to the right way.

Signed-off-by: HeungJun, Kim <riverful.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/m5mols/m5mols_capture.c |   37 ++++++++------------------
 drivers/media/video/m5mols/m5mols_core.c    |    4 ++-
 2 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/media/video/m5mols/m5mols_capture.c b/drivers/media/video/m5mols/m5mols_capture.c
index 18a56bf..5bb0f96 100644
--- a/drivers/media/video/m5mols/m5mols_capture.c
+++ b/drivers/media/video/m5mols/m5mols_capture.c
@@ -108,51 +108,38 @@ int m5mols_start_capture(struct m5mols_info *info)
 	int ret;
 
 	/*
-	 * Preparing capture. Setting control & interrupt before entering
-	 * capture mode
-	 *
-	 * 1) change to MONITOR mode for operating control & interrupt
-	 * 2) set controls (considering v4l2_control value & lock 3A)
-	 * 3) set interrupt
-	 * 4) change to CAPTURE mode
+	 * CAPTURE - capture using ISP in the various sized and formats
+	 * (JPEG/RAW-BAYER/YUV422 for recording). As soon as changing to
+	 * CAPTURE mode, the CAPTURE is started. And until desired jiffies,
+	 * wait interrupt.
 	 */
 	ret = m5mols_mode(info, REG_MONITOR);
 	if (!ret)
 		ret = m5mols_sync_controls(info);
 	if (!ret)
-		ret = m5mols_lock_3a(info, true);
+		ret = m5mols_write(sd, CAPP_YUVOUT_MAIN, REG_JPEG);
+	if (!ret)
+		ret = m5mols_write(sd, CAPP_MAIN_IMAGE_SIZE, resolution);
 	if (!ret)
-		ret = m5mols_enable_interrupt(sd, REG_INT_CAPTURE);
+		ret = m5mols_lock_3a(info, true);
 	if (!ret)
 		ret = m5mols_mode(info, REG_CAPTURE);
 	if (!ret)
-		/* Wait for capture interrupt, after changing capture mode */
 		ret = m5mols_timeout_interrupt(sd, REG_INT_CAPTURE, 2000);
 	if (!ret)
 		ret = m5mols_lock_3a(info, false);
 	if (ret)
 		return ret;
+
 	/*
-	 * Starting capture. Setting capture frame count and resolution and
-	 * the format(available format: JPEG, Bayer RAW, YUV).
-	 *
-	 * 1) select single or multi(enable to 25), format, size
-	 * 2) set interrupt
-	 * 3) start capture(for main image, now)
-	 * 4) get information
-	 * 5) notify file size to v4l2 device(e.g, to s5p-fimc v4l2 device)
+	 * TRANSFER - transfer captured image and information. As soon as
+	 * sending CAPC_START commands, the TRANSFER is started. And until
+	 * desired jiffies, wait interrupt.
 	 */
 	ret = m5mols_write(sd, CAPC_SEL_FRAME, 1);
 	if (!ret)
-		ret = m5mols_write(sd, CAPP_YUVOUT_MAIN, REG_JPEG);
-	if (!ret)
-		ret = m5mols_write(sd, CAPP_MAIN_IMAGE_SIZE, resolution);
-	if (!ret)
-		ret = m5mols_enable_interrupt(sd, REG_INT_CAPTURE);
-	if (!ret)
 		ret = m5mols_write(sd, CAPC_START, REG_CAP_START_MAIN);
 	if (!ret) {
-		/* Wait for the capture completion interrupt */
 		ret = m5mols_timeout_interrupt(sd, REG_INT_CAPTURE, 2000);
 		if (!ret) {
 			ret = m5mols_capture_info(info);
diff --git a/drivers/media/video/m5mols/m5mols_core.c b/drivers/media/video/m5mols/m5mols_core.c
index 0aae868..09bb258 100644
--- a/drivers/media/video/m5mols/m5mols_core.c
+++ b/drivers/media/video/m5mols/m5mols_core.c
@@ -806,9 +806,11 @@ static int m5mols_sensor_armboot(struct v4l2_subdev *sd)
 
 	v4l2_dbg(1, m5mols_debug, sd, "Success ARM Booting\n");
 
+	/* Initialize general setting for M-5MOLS */
 	ret = m5mols_write(sd, PARM_INTERFACE, REG_INTERFACE_MIPI);
 	if (!ret)
-		ret = m5mols_enable_interrupt(sd, REG_INT_AF);
+		ret = m5mols_enable_interrupt(sd,
+				REG_INT_AF | REG_INT_CAPTURE);
 
 	return ret;
 }
-- 
1.7.4.1

Re: [PATCH 4/5] m5mols: Add boot flag for not showing the msg of I2C error

[Sylwester Nawrocki]

Hi HeungJun,

Sorry for late review. Please see my comments below..

On 10/21/2011 09:35 AM, HeungJun, Kim wrote:
> In M-5MOLS sensor, the I2C error can be occured before sensor booting done,
> becase I2C interface is not stabilized although the sensor have done already
> for booting, so the right value is deliver through I2C interface. In case,
> it needs to make the I2C error msg not to be printed.
> 
> Signed-off-by: HeungJun, Kim<riverful.kim@samsung.com>
> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> ---
>   drivers/media/video/m5mols/m5mols.h      |    2 ++
>   drivers/media/video/m5mols/m5mols_core.c |   17 +++++++++++++----
>   2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/video/m5mols/m5mols.h b/drivers/media/video/m5mols/m5mols.h
> index 75f7984..0d7e202 100644
> --- a/drivers/media/video/m5mols/m5mols.h
> +++ b/drivers/media/video/m5mols/m5mols.h
> @@ -175,6 +175,7 @@ struct m5mols_version {
>    * @ver: information of the version
>    * @cap: the capture mode attributes
>    * @power: current sensor's power status
> + * @boot: "true" means the M-5MOLS sensor done ARM Booting

How about making this "booting" instead (the opposite meaning) ? Also there is
no need for quotation marks.

>    * @ctrl_sync: true means all controls of the sensor are initialized
>    * @int_capture: true means the capture interrupt is issued once
>    * @lock_ae: true means the Auto Exposure is locked
> @@ -210,6 +211,7 @@ struct m5mols_info {
>   	struct m5mols_version ver;
>   	struct m5mols_capture cap;
>   	bool power;
> +	bool boot;
>   	bool issue;
>   	bool ctrl_sync;
>   	bool lock_ae;
> diff --git a/drivers/media/video/m5mols/m5mols_core.c b/drivers/media/video/m5mols/m5mols_core.c
> index 24e66ad..0aae868 100644
> --- a/drivers/media/video/m5mols/m5mols_core.c
> +++ b/drivers/media/video/m5mols/m5mols_core.c
> @@ -138,6 +138,7 @@ static u32 m5mols_swap_byte(u8 *data, u8 length)
>   static int m5mols_read(struct v4l2_subdev *sd, u32 size, u32 reg, u32 *val)
>   {
>   	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct m5mols_info *info = to_m5mols(sd);
>   	u8 rbuf[M5MOLS_I2C_MAX_SIZE + 1];
>   	u8 category = I2C_CATEGORY(reg);
>   	u8 cmd = I2C_COMMAND(reg);
> @@ -168,8 +169,10 @@ static int m5mols_read(struct v4l2_subdev *sd, u32 size, u32 reg, u32 *val)
> 
>   	ret = i2c_transfer(client->adapter, msg, 2);
>   	if (ret<  0) {
> -		v4l2_err(sd, "read failed: size:%d cat:%02x cmd:%02x. %d\n",
> -			 size, category, cmd, ret);
> +		if (info->boot)
> +			v4l2_err(sd,
> +				"read failed: cat:%02x cmd:%02x ret:%d\n",
> +				category, cmd, ret);
>   		return ret;

To avoid dodgy indentation, this could be for instance rewritten as:

   	ret = i2c_transfer(client->adapter, msg, 2);
   	if (ret == 2)
		return 0;

	if (!info->booting)
		v4l2_err(sd, "read failed: size:%d cat:%02x cmd:%02x. %d\n",
			 size, category, cmd, ret);

  	return ret < 0 ? ret : -EIO;

>   	}
> 
> @@ -232,6 +235,7 @@ int m5mols_read_u32(struct v4l2_subdev *sd, u32 reg, u32 *val)
>   int m5mols_write(struct v4l2_subdev *sd, u32 reg, u32 val)
>   {
>   	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct m5mols_info *info = to_m5mols(sd);
>   	u8 wbuf[M5MOLS_I2C_MAX_SIZE + 4];
>   	u8 category = I2C_CATEGORY(reg);
>   	u8 cmd = I2C_COMMAND(reg);
> @@ -263,8 +267,10 @@ int m5mols_write(struct v4l2_subdev *sd, u32 reg, u32 val)
> 
>   	ret = i2c_transfer(client->adapter, msg, 1);
>   	if (ret<  0) {
> -		v4l2_err(sd, "write failed: size:%d cat:%02x cmd:%02x. %d\n",
> -			size, category, cmd, ret);
> +		if (info->boot)
> +			v4l2_err(sd,
> +				"write failed: cat:%02x cmd:%02x ret:%d\n",
> +				category, cmd, ret);

Ditto.

>   		return ret;
>   	}
> 
> @@ -778,6 +784,7 @@ int __attribute__ ((weak)) m5mols_update_fw(struct v4l2_subdev *sd,
>    */
>   static int m5mols_sensor_armboot(struct v4l2_subdev *sd)
>   {
> +	struct m5mols_info *info = to_m5mols(sd);
>   	int ret;
> 
>   	/* Execute ARM boot sequence */
> @@ -786,6 +793,8 @@ static int m5mols_sensor_armboot(struct v4l2_subdev *sd)
>   		ret = m5mols_write(sd, FLASH_CAM_START, REG_START_ARM_BOOT);
>   	if (!ret)
>   		ret = m5mols_timeout_interrupt(sd, REG_INT_MODE, 2000);
> +	if (!ret)
> +		info->boot = true;

If you move this line after the check below, there is no need for "if (!ret)".

>   	if (ret<  0)
>   		return ret;
> 

--
Regards,
Sylwester

Re: [PATCH 2/5] m5mols: Replace IRQ workqueue to waitqueue only

[Sylwester Nawrocki]

On 10/21/2011 09:35 AM, HeungJun, Kim wrote:
> In M-5MOLS driver, the workqueue code for IRQ is hard to re-use. So, remove
> the IRQ workqueue, and use only waitqueue for waiting IRQ with timeout.
> The info->issue has the status that interrupt is issued or not, then
> the info->interrupt has the IRQ status register at that time.
> 
> Signed-off-by: HeungJun, Kim<riverful.kim@samsung.com>
> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> ---
>   drivers/media/video/m5mols/m5mols.h         |    7 +--
>   drivers/media/video/m5mols/m5mols_capture.c |   34 ++-------------
>   drivers/media/video/m5mols/m5mols_core.c    |   60 +++++++++++----------------
>   3 files changed, 32 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/media/video/m5mols/m5mols.h b/drivers/media/video/m5mols/m5mols.h
> index c8e1572..75f7984 100644
> --- a/drivers/media/video/m5mols/m5mols.h
> +++ b/drivers/media/video/m5mols/m5mols.h
> @@ -164,7 +164,6 @@ struct m5mols_version {
>    * @res_type: current resolution type
>    * @code: current code
>    * @irq_waitq: waitqueue for the capture
> - * @work_irq: workqueue for the IRQ
>    * @flags: state variable for the interrupt handler
>    * @handle: control handler
>    * @autoexposure: Auto Exposure control
> @@ -181,6 +180,7 @@ struct m5mols_version {
>    * @lock_ae: true means the Auto Exposure is locked
>    * @lock_awb: true means the Aut WhiteBalance is locked
>    * @resolution:	register value for current resolution
> + * @issue: "true" means the M-5MOLS sensor's interrupt issued
>    * @interrupt: register value for current interrupt status
>    * @mode: register value for current operation mode
>    * @mode_save: register value for current operation mode for saving
> @@ -194,7 +194,6 @@ struct m5mols_info {
>   	int res_type;
>   	enum v4l2_mbus_pixelcode code;
>   	wait_queue_head_t irq_waitq;
> -	struct work_struct work_irq;
>   	unsigned long flags;
> 
>   	struct v4l2_ctrl_handler handle;
> @@ -211,6 +210,7 @@ struct m5mols_info {
>   	struct m5mols_version ver;
>   	struct m5mols_capture cap;
>   	bool power;
> +	bool issue;
>   	bool ctrl_sync;
>   	bool lock_ae;
>   	bool lock_awb;
> @@ -221,8 +221,6 @@ struct m5mols_info {
>   	int (*set_power)(struct device *dev, int on);
>   };
> 
> -#define ST_CAPT_IRQ 0
> -
>   #define is_powered(__info) (__info->power)
>   #define is_ctrl_synced(__info) (__info->ctrl_sync)
>   #define is_available_af(__info)	(__info->ver.af)
> @@ -283,6 +281,7 @@ int m5mols_write(struct v4l2_subdev *sd, u32 reg_comb, u32 val);
>   int m5mols_mode(struct m5mols_info *info, u8 mode);
> 
>   int m5mols_enable_interrupt(struct v4l2_subdev *sd, u8 reg);
> +int m5mols_timeout_interrupt(struct v4l2_subdev *sd, u8 condition, u32 timeout);
>   int m5mols_sync_controls(struct m5mols_info *info);
>   int m5mols_start_capture(struct m5mols_info *info);
>   int m5mols_do_scenemode(struct m5mols_info *info, u8 mode);
> diff --git a/drivers/media/video/m5mols/m5mols_capture.c b/drivers/media/video/m5mols/m5mols_capture.c
> index 3248ac8..18a56bf 100644
> --- a/drivers/media/video/m5mols/m5mols_capture.c
> +++ b/drivers/media/video/m5mols/m5mols_capture.c
> @@ -29,22 +29,6 @@
>   #include "m5mols.h"
>   #include "m5mols_reg.h"
> 
> -static int m5mols_capture_error_handler(struct m5mols_info *info,
> -					int timeout)
> -{
> -	int ret;
> -
> -	/* Disable all interrupts and clear relevant interrupt staus bits */
> -	ret = m5mols_write(&info->sd, SYSTEM_INT_ENABLE,
> -			   info->interrupt&  ~(REG_INT_CAPTURE));
> -	if (ret)
> -		return ret;
> -
> -	if (timeout == 0)
> -		return -ETIMEDOUT;
> -
> -	return 0;
> -}
>   /**
>    * m5mols_read_rational - I2C read of a rational number
>    *
> @@ -121,7 +105,6 @@ int m5mols_start_capture(struct m5mols_info *info)
>   {
>   	struct v4l2_subdev *sd =&info->sd;
>   	u8 resolution = info->resolution;
> -	int timeout;
>   	int ret;
> 
>   	/*
> @@ -142,14 +125,9 @@ int m5mols_start_capture(struct m5mols_info *info)
>   		ret = m5mols_enable_interrupt(sd, REG_INT_CAPTURE);
>   	if (!ret)
>   		ret = m5mols_mode(info, REG_CAPTURE);
> -	if (!ret) {
> +	if (!ret)
>   		/* Wait for capture interrupt, after changing capture mode */
> -		timeout = wait_event_interruptible_timeout(info->irq_waitq,
> -					   test_bit(ST_CAPT_IRQ,&info->flags),
> -					   msecs_to_jiffies(2000));
> -		if (test_and_clear_bit(ST_CAPT_IRQ,&info->flags))
> -			ret = m5mols_capture_error_handler(info, timeout);
> -	}
> +		ret = m5mols_timeout_interrupt(sd, REG_INT_CAPTURE, 2000);
>   	if (!ret)
>   		ret = m5mols_lock_3a(info, false);
>   	if (ret)
> @@ -175,15 +153,13 @@ int m5mols_start_capture(struct m5mols_info *info)
>   		ret = m5mols_write(sd, CAPC_START, REG_CAP_START_MAIN);
>   	if (!ret) {
>   		/* Wait for the capture completion interrupt */
> -		timeout = wait_event_interruptible_timeout(info->irq_waitq,
> -					   test_bit(ST_CAPT_IRQ,&info->flags),
> -					   msecs_to_jiffies(2000));
> -		if (test_and_clear_bit(ST_CAPT_IRQ,&info->flags)) {
> +		ret = m5mols_timeout_interrupt(sd, REG_INT_CAPTURE, 2000);
> +		if (!ret) {
>   			ret = m5mols_capture_info(info);
>   			if (!ret)
>   				v4l2_subdev_notify(sd, 0,&info->cap.total);
>   		}
>   	}
> 
> -	return m5mols_capture_error_handler(info, timeout);
> +	return ret;
>   }
> diff --git a/drivers/media/video/m5mols/m5mols_core.c b/drivers/media/video/m5mols/m5mols_core.c
> index 73db96e..f3b9415 100644
> --- a/drivers/media/video/m5mols/m5mols_core.c
> +++ b/drivers/media/video/m5mols/m5mols_core.c
> @@ -333,6 +333,28 @@ int m5mols_enable_interrupt(struct v4l2_subdev *sd, u8 reg)
>   	return ret;
>   }
> 
> +/* m5mols_timeout_interrupt - Wait interrupt and figure out which interrupt. */
> +int m5mols_timeout_interrupt(struct v4l2_subdev *sd, u8 condition, u32 timeout)

Could this be m5mols_wait_interrupt() instead ?

> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	u8 reg;
> +	int timed;
> +	int ret;
> +
> +	timed = wait_event_interruptible_timeout(info->irq_waitq,
> +			info->issue, msecs_to_jiffies(timeout));

I'm a bit sceptic about replacing current atomic test with a non atomic one.
Using bit operations (test_and_clear_bit() ?) could probably save us from loosing 
any interrupt. Still, there might be no problem if you're careful enough.

> +	if (!timed)
> +		return -ETIMEDOUT;
> +
> +	ret = m5mols_busy_val(sd, condition,&reg, CAT_SYSTEM, CAT0_INT_FACTOR);
> +	if (ret || (!ret&&  reg != condition))

It could be simplified to:

	if (ret || reg != condition)

> +		return -EINVAL;
> +
> +	info->interrupt = reg;
> +	info->issue = false;

I think this might be racy, as this variable is also being changed in the interrupt
handler. 

> +	return 0;
> +}
> +
>   /**
>    * m5mols_reg_mode - Write the mode and check busy status
>    *
> @@ -901,46 +923,13 @@ static const struct v4l2_subdev_ops m5mols_ops = {
>   	.video		=&m5mols_video_ops,
>   };
> 
> -static void m5mols_irq_work(struct work_struct *work)
> -{
> -	struct m5mols_info *info =
> -		container_of(work, struct m5mols_info, work_irq);
> -	struct v4l2_subdev *sd =&info->sd;
> -	u8 reg;
> -	int ret;
> -
> -	if (!is_powered(info) ||
> -			m5mols_read_u8(sd, SYSTEM_INT_FACTOR,&info->interrupt))
> -		return;
> -
> -	switch (info->interrupt&  REG_INT_MASK) {
> -	case REG_INT_AF:
> -		if (!is_available_af(info))
> -			break;
> -		ret = m5mols_read_u8(sd, AF_STATUS,&reg);
> -		v4l2_dbg(2, m5mols_debug, sd, "AF %s\n",
> -			 reg == REG_AF_FAIL ? "Failed" :
> -			 reg == REG_AF_SUCCESS ? "Success" :
> -			 reg == REG_AF_IDLE ? "Idle" : "Busy");
> -		break;
> -	case REG_INT_CAPTURE:
> -		if (!test_and_set_bit(ST_CAPT_IRQ,&info->flags))
> -			wake_up_interruptible(&info->irq_waitq);
> -
> -		v4l2_dbg(2, m5mols_debug, sd, "CAPTURE\n");
> -		break;
> -	default:
> -		v4l2_dbg(2, m5mols_debug, sd, "Undefined: %02x\n", reg);
> -		break;
> -	};
> -}
> -
>   static irqreturn_t m5mols_irq_handler(int irq, void *data)
>   {
>   	struct v4l2_subdev *sd = data;
>   	struct m5mols_info *info = to_m5mols(sd);
> 
> -	schedule_work(&info->work_irq);
> +	info->issue = true;
> +	wake_up_interruptible(&info->irq_waitq);
> 
>   	return IRQ_HANDLED;
>   }
> @@ -999,7 +988,6 @@ static int __devinit m5mols_probe(struct i2c_client *client,
>   	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
> 
>   	init_waitqueue_head(&info->irq_waitq);
> -	INIT_WORK(&info->work_irq, m5mols_irq_work);
>   	ret = request_irq(client->irq, m5mols_irq_handler,
>   			  IRQF_TRIGGER_RISING, MODULE_NAME, sd);
>   	if (ret) {

--
Regards,
Sylwester

RE: [PATCH 4/5] m5mols: Add boot flag for not showing the msg of I2C error

[HeungJun, Kim]

Hi Sylwester,

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Sylwester Nawrocki
> Sent: Tuesday, November 15, 2011 7:35 AM
> To: Heungjun Kim
> Cc: linux-media; Kyungmin Park
> Subject: Re: [PATCH 4/5] m5mols: Add boot flag for not showing the msg of
> I2C error
> 
> Hi HeungJun,
> 
> Sorry for late review. Please see my comments below..
Nevermind. It's fine.

> 
> On 10/21/2011 09:35 AM, HeungJun, Kim wrote:
> > In M-5MOLS sensor, the I2C error can be occured before sensor booting
> done,
> > becase I2C interface is not stabilized although the sensor have done
> already
> > for booting, so the right value is deliver through I2C interface. In case,
> > it needs to make the I2C error msg not to be printed.
> >
> > Signed-off-by: HeungJun, Kim<riverful.kim@samsung.com>
> > Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > ---
> >   drivers/media/video/m5mols/m5mols.h      |    2 ++
> >   drivers/media/video/m5mols/m5mols_core.c |   17 +++++++++++++----
> >   2 files changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/video/m5mols/m5mols.h
> b/drivers/media/video/m5mols/m5mols.h
> > index 75f7984..0d7e202 100644
> > --- a/drivers/media/video/m5mols/m5mols.h
> > +++ b/drivers/media/video/m5mols/m5mols.h
> > @@ -175,6 +175,7 @@ struct m5mols_version {
> >    * @ver: information of the version
> >    * @cap: the capture mode attributes
> >    * @power: current sensor's power status
> > + * @boot: "true" means the M-5MOLS sensor done ARM Booting
> 
> How about making this "booting" instead (the opposite meaning) ? Also there
> is
> no need for quotation marks.
Ok. It sounds close to the meaning.

> 
> >    * @ctrl_sync: true means all controls of the sensor are initialized
> >    * @int_capture: true means the capture interrupt is issued once
> >    * @lock_ae: true means the Auto Exposure is locked
> > @@ -210,6 +211,7 @@ struct m5mols_info {
> >   	struct m5mols_version ver;
> >   	struct m5mols_capture cap;
> >   	bool power;
> > +	bool boot;
> >   	bool issue;
> >   	bool ctrl_sync;
> >   	bool lock_ae;
> > diff --git a/drivers/media/video/m5mols/m5mols_core.c
> b/drivers/media/video/m5mols/m5mols_core.c
> > index 24e66ad..0aae868 100644
> > --- a/drivers/media/video/m5mols/m5mols_core.c
> > +++ b/drivers/media/video/m5mols/m5mols_core.c
> > @@ -138,6 +138,7 @@ static u32 m5mols_swap_byte(u8 *data, u8 length)
> >   static int m5mols_read(struct v4l2_subdev *sd, u32 size, u32 reg, u32
> *val)
> >   {
> >   	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +	struct m5mols_info *info = to_m5mols(sd);
> >   	u8 rbuf[M5MOLS_I2C_MAX_SIZE + 1];
> >   	u8 category = I2C_CATEGORY(reg);
> >   	u8 cmd = I2C_COMMAND(reg);
> > @@ -168,8 +169,10 @@ static int m5mols_read(struct v4l2_subdev *sd, u32
> size, u32 reg, u32 *val)
> >
> >   	ret = i2c_transfer(client->adapter, msg, 2);
> >   	if (ret<  0) {
> > -		v4l2_err(sd, "read failed: size:%d cat:%02x cmd:%02x. %d\n",
> > -			 size, category, cmd, ret);
> > +		if (info->boot)
> > +			v4l2_err(sd,
> > +				"read failed: cat:%02x cmd:%02x ret:%d\n",
> > +				category, cmd, ret);
> >   		return ret;
> 
> To avoid dodgy indentation, this could be for instance rewritten as:
> 
>    	ret = i2c_transfer(client->adapter, msg, 2);
>    	if (ret == 2)
> 		return 0;
> 
> 	if (!info->booting)
> 		v4l2_err(sd, "read failed: size:%d cat:%02x cmd:%02x. %d\n",
> 			 size, category, cmd, ret);
> 
>   	return ret < 0 ? ret : -EIO;
>
Ok. If the reason is dodgy indentation, it would be better to change the message
string to the more shorter others. I'll consider at the next version patch. 

> >   	}
> >
> > @@ -232,6 +235,7 @@ int m5mols_read_u32(struct v4l2_subdev *sd, u32 reg,
> u32 *val)
> >   int m5mols_write(struct v4l2_subdev *sd, u32 reg, u32 val)
> >   {
> >   	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +	struct m5mols_info *info = to_m5mols(sd);
> >   	u8 wbuf[M5MOLS_I2C_MAX_SIZE + 4];
> >   	u8 category = I2C_CATEGORY(reg);
> >   	u8 cmd = I2C_COMMAND(reg);
> > @@ -263,8 +267,10 @@ int m5mols_write(struct v4l2_subdev *sd, u32 reg,
> u32 val)
> >
> >   	ret = i2c_transfer(client->adapter, msg, 1);
> >   	if (ret<  0) {
> > -		v4l2_err(sd, "write failed: size:%d cat:%02x cmd:%02x. %d\n",
> > -			size, category, cmd, ret);
> > +		if (info->boot)
> > +			v4l2_err(sd,
> > +				"write failed: cat:%02x cmd:%02x ret:%d\n",
> > +				category, cmd, ret);
> 
> Ditto.
> 
> >   		return ret;
> >   	}
> >
> > @@ -778,6 +784,7 @@ int __attribute__ ((weak)) m5mols_update_fw(struct
> v4l2_subdev *sd,
> >    */
> >   static int m5mols_sensor_armboot(struct v4l2_subdev *sd)
> >   {
> > +	struct m5mols_info *info = to_m5mols(sd);
> >   	int ret;
> >
> >   	/* Execute ARM boot sequence */
> > @@ -786,6 +793,8 @@ static int m5mols_sensor_armboot(struct v4l2_subdev
> *sd)
> >   		ret = m5mols_write(sd, FLASH_CAM_START, REG_START_ARM_BOOT);
> >   	if (!ret)
> >   		ret = m5mols_timeout_interrupt(sd, REG_INT_MODE, 2000);
> > +	if (!ret)
> > +		info->boot = true;
> 
> If you move this line after the check below, there is no need for "if
> (!ret)".
It looks better. I'll adapt it.

> 
> >   	if (ret<  0)
> >   		return ret;
> >
> 
> --
> Regards,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/5] m5mols: Replace IRQ workqueue to waitqueue only

[HeungJun, Kim]

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Sylwester Nawrocki
> Sent: Tuesday, November 15, 2011 8:04 AM
> To: HeungJun, Kim
> Cc: linux-media@vger.kernel.org; Kyungmin Park
> Subject: Re: [PATCH 2/5] m5mols: Replace IRQ workqueue to waitqueue only
> 
> On 10/21/2011 09:35 AM, HeungJun, Kim wrote:
> > In M-5MOLS driver, the workqueue code for IRQ is hard to re-use. So,
> remove
> > the IRQ workqueue, and use only waitqueue for waiting IRQ with timeout.
> > The info->issue has the status that interrupt is issued or not, then
> > the info->interrupt has the IRQ status register at that time.
> >
> > Signed-off-by: HeungJun, Kim<riverful.kim@samsung.com>
> > Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > ---
> >   drivers/media/video/m5mols/m5mols.h         |    7 +--
> >   drivers/media/video/m5mols/m5mols_capture.c |   34 ++-------------
> >   drivers/media/video/m5mols/m5mols_core.c    |   60 +++++++++++-----------
> -----
> >   3 files changed, 32 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/media/video/m5mols/m5mols.h
> b/drivers/media/video/m5mols/m5mols.h
> > index c8e1572..75f7984 100644
> > --- a/drivers/media/video/m5mols/m5mols.h
> > +++ b/drivers/media/video/m5mols/m5mols.h
> > @@ -164,7 +164,6 @@ struct m5mols_version {
> >    * @res_type: current resolution type
> >    * @code: current code
> >    * @irq_waitq: waitqueue for the capture
> > - * @work_irq: workqueue for the IRQ
> >    * @flags: state variable for the interrupt handler
> >    * @handle: control handler
> >    * @autoexposure: Auto Exposure control
> > @@ -181,6 +180,7 @@ struct m5mols_version {
> >    * @lock_ae: true means the Auto Exposure is locked
> >    * @lock_awb: true means the Aut WhiteBalance is locked
> >    * @resolution:	register value for current resolution
> > + * @issue: "true" means the M-5MOLS sensor's interrupt issued
> >    * @interrupt: register value for current interrupt status
> >    * @mode: register value for current operation mode
> >    * @mode_save: register value for current operation mode for saving
> > @@ -194,7 +194,6 @@ struct m5mols_info {
> >   	int res_type;
> >   	enum v4l2_mbus_pixelcode code;
> >   	wait_queue_head_t irq_waitq;
> > -	struct work_struct work_irq;
> >   	unsigned long flags;
> >
> >   	struct v4l2_ctrl_handler handle;
> > @@ -211,6 +210,7 @@ struct m5mols_info {
> >   	struct m5mols_version ver;
> >   	struct m5mols_capture cap;
> >   	bool power;
> > +	bool issue;
> >   	bool ctrl_sync;
> >   	bool lock_ae;
> >   	bool lock_awb;
> > @@ -221,8 +221,6 @@ struct m5mols_info {
> >   	int (*set_power)(struct device *dev, int on);
> >   };
> >
> > -#define ST_CAPT_IRQ 0
> > -
> >   #define is_powered(__info) (__info->power)
> >   #define is_ctrl_synced(__info) (__info->ctrl_sync)
> >   #define is_available_af(__info)	(__info->ver.af)
> > @@ -283,6 +281,7 @@ int m5mols_write(struct v4l2_subdev *sd, u32 reg_comb,
> u32 val);
> >   int m5mols_mode(struct m5mols_info *info, u8 mode);
> >
> >   int m5mols_enable_interrupt(struct v4l2_subdev *sd, u8 reg);
> > +int m5mols_timeout_interrupt(struct v4l2_subdev *sd, u8 condition, u32
> timeout);
> >   int m5mols_sync_controls(struct m5mols_info *info);
> >   int m5mols_start_capture(struct m5mols_info *info);
> >   int m5mols_do_scenemode(struct m5mols_info *info, u8 mode);
> > diff --git a/drivers/media/video/m5mols/m5mols_capture.c
> b/drivers/media/video/m5mols/m5mols_capture.c
> > index 3248ac8..18a56bf 100644
> > --- a/drivers/media/video/m5mols/m5mols_capture.c
> > +++ b/drivers/media/video/m5mols/m5mols_capture.c
> > @@ -29,22 +29,6 @@
> >   #include "m5mols.h"
> >   #include "m5mols_reg.h"
> >
> > -static int m5mols_capture_error_handler(struct m5mols_info *info,
> > -					int timeout)
> > -{
> > -	int ret;
> > -
> > -	/* Disable all interrupts and clear relevant interrupt staus bits */
> > -	ret = m5mols_write(&info->sd, SYSTEM_INT_ENABLE,
> > -			   info->interrupt&  ~(REG_INT_CAPTURE));
> > -	if (ret)
> > -		return ret;
> > -
> > -	if (timeout == 0)
> > -		return -ETIMEDOUT;
> > -
> > -	return 0;
> > -}
> >   /**
> >    * m5mols_read_rational - I2C read of a rational number
> >    *
> > @@ -121,7 +105,6 @@ int m5mols_start_capture(struct m5mols_info *info)
> >   {
> >   	struct v4l2_subdev *sd =&info->sd;
> >   	u8 resolution = info->resolution;
> > -	int timeout;
> >   	int ret;
> >
> >   	/*
> > @@ -142,14 +125,9 @@ int m5mols_start_capture(struct m5mols_info *info)
> >   		ret = m5mols_enable_interrupt(sd, REG_INT_CAPTURE);
> >   	if (!ret)
> >   		ret = m5mols_mode(info, REG_CAPTURE);
> > -	if (!ret) {
> > +	if (!ret)
> >   		/* Wait for capture interrupt, after changing capture mode */
> > -		timeout = wait_event_interruptible_timeout(info->irq_waitq,
> > -					   test_bit(ST_CAPT_IRQ,&info->flags),
> > -					   msecs_to_jiffies(2000));
> > -		if (test_and_clear_bit(ST_CAPT_IRQ,&info->flags))
> > -			ret = m5mols_capture_error_handler(info, timeout);
> > -	}
> > +		ret = m5mols_timeout_interrupt(sd, REG_INT_CAPTURE, 2000);
> >   	if (!ret)
> >   		ret = m5mols_lock_3a(info, false);
> >   	if (ret)
> > @@ -175,15 +153,13 @@ int m5mols_start_capture(struct m5mols_info *info)
> >   		ret = m5mols_write(sd, CAPC_START, REG_CAP_START_MAIN);
> >   	if (!ret) {
> >   		/* Wait for the capture completion interrupt */
> > -		timeout = wait_event_interruptible_timeout(info->irq_waitq,
> > -					   test_bit(ST_CAPT_IRQ,&info->flags),
> > -					   msecs_to_jiffies(2000));
> > -		if (test_and_clear_bit(ST_CAPT_IRQ,&info->flags)) {
> > +		ret = m5mols_timeout_interrupt(sd, REG_INT_CAPTURE, 2000);
> > +		if (!ret) {
> >   			ret = m5mols_capture_info(info);
> >   			if (!ret)
> >   				v4l2_subdev_notify(sd, 0,&info->cap.total);
> >   		}
> >   	}
> >
> > -	return m5mols_capture_error_handler(info, timeout);
> > +	return ret;
> >   }
> > diff --git a/drivers/media/video/m5mols/m5mols_core.c
> b/drivers/media/video/m5mols/m5mols_core.c
> > index 73db96e..f3b9415 100644
> > --- a/drivers/media/video/m5mols/m5mols_core.c
> > +++ b/drivers/media/video/m5mols/m5mols_core.c
> > @@ -333,6 +333,28 @@ int m5mols_enable_interrupt(struct v4l2_subdev *sd,
> u8 reg)
> >   	return ret;
> >   }
> >
> > +/* m5mols_timeout_interrupt - Wait interrupt and figure out which
> interrupt. */
> > +int m5mols_timeout_interrupt(struct v4l2_subdev *sd, u8 condition, u32
> timeout)
> 
> Could this be m5mols_wait_interrupt() instead ?
I wanna avoid the name of m5mols_wait_interrupt() :), and try to choose
the more similar other name. Nevermind. It's just private.

Ok. I'll use this.

> 
> > +{
> > +	struct m5mols_info *info = to_m5mols(sd);
> > +	u8 reg;
> > +	int timed;
> > +	int ret;
> > +
> > +	timed = wait_event_interruptible_timeout(info->irq_waitq,
> > +			info->issue, msecs_to_jiffies(timeout));
> 
> I'm a bit sceptic about replacing current atomic test with a non atomic one.
> Using bit operations (test_and_clear_bit() ?) could probably save us from
> loosing
> any interrupt. Still, there might be no problem if you're careful enough.
You're right. The case occurred the problem is only that the sensor issues interrupt
signal one more while processing this, but, the sensor is slow as the timeout is needed.

But, conceptually it's right to use bit operation. I'll change this for the next.

> 
> > +	if (!timed)
> > +		return -ETIMEDOUT;
> > +
> > +	ret = m5mols_busy_val(sd, condition,&reg, CAT_SYSTEM,
> CAT0_INT_FACTOR);
> > +	if (ret || (!ret&&  reg != condition))
> 
> It could be simplified to:
> 
> 	if (ret || reg != condition)
Ok. Also will use this.

> 
> > +		return -EINVAL;
> > +
> > +	info->interrupt = reg;
> > +	info->issue = false;
> 
> I think this might be racy, as this variable is also being changed in the
> interrupt
> handler.
It looks racy, I agree. But, as I said the m5mols is slow so and it's not
happened the same interrupt at one more time serially.

But, I also change this for the next version.


> 
> > +	return 0;
> > +}
> > +
> >   /**
> >    * m5mols_reg_mode - Write the mode and check busy status
> >    *
> > @@ -901,46 +923,13 @@ static const struct v4l2_subdev_ops m5mols_ops = {
> >   	.video		=&m5mols_video_ops,
> >   };
> >
> > -static void m5mols_irq_work(struct work_struct *work)
> > -{
> > -	struct m5mols_info *info =
> > -		container_of(work, struct m5mols_info, work_irq);
> > -	struct v4l2_subdev *sd =&info->sd;
> > -	u8 reg;
> > -	int ret;
> > -
> > -	if (!is_powered(info) ||
> > -			m5mols_read_u8(sd, SYSTEM_INT_FACTOR,&info->interrupt))
> > -		return;
> > -
> > -	switch (info->interrupt&  REG_INT_MASK) {
> > -	case REG_INT_AF:
> > -		if (!is_available_af(info))
> > -			break;
> > -		ret = m5mols_read_u8(sd, AF_STATUS,&reg);
> > -		v4l2_dbg(2, m5mols_debug, sd, "AF %s\n",
> > -			 reg == REG_AF_FAIL ? "Failed" :
> > -			 reg == REG_AF_SUCCESS ? "Success" :
> > -			 reg == REG_AF_IDLE ? "Idle" : "Busy");
> > -		break;
> > -	case REG_INT_CAPTURE:
> > -		if (!test_and_set_bit(ST_CAPT_IRQ,&info->flags))
> > -			wake_up_interruptible(&info->irq_waitq);
> > -
> > -		v4l2_dbg(2, m5mols_debug, sd, "CAPTURE\n");
> > -		break;
> > -	default:
> > -		v4l2_dbg(2, m5mols_debug, sd, "Undefined: %02x\n", reg);
> > -		break;
> > -	};
> > -}
> > -
> >   static irqreturn_t m5mols_irq_handler(int irq, void *data)
> >   {
> >   	struct v4l2_subdev *sd = data;
> >   	struct m5mols_info *info = to_m5mols(sd);
> >
> > -	schedule_work(&info->work_irq);
> > +	info->issue = true;
> > +	wake_up_interruptible(&info->irq_waitq);
> >
> >   	return IRQ_HANDLED;
> >   }
> > @@ -999,7 +988,6 @@ static int __devinit m5mols_probe(struct i2c_client
> *client,
> >   	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
> >
> >   	init_waitqueue_head(&info->irq_waitq);
> > -	INIT_WORK(&info->work_irq, m5mols_irq_work);
> >   	ret = request_irq(client->irq, m5mols_irq_handler,
> >   			  IRQF_TRIGGER_RISING, MODULE_NAME, sd);
> >   	if (ret) {
> 
> --
> Regards,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Thanks for the comments.

I'll re-works this patches.

Regards,
Heungjun Kim