* [PATCH 2/5] m5mols: Replace IRQ workqueue to waitqueue only
2011-10-21 7:35 [PATCH 1/5] m5mols: Add more functions to check busy status HeungJun, Kim
@ 2011-10-21 7:35 ` HeungJun, Kim
2011-11-14 23:04 ` Sylwester Nawrocki
2011-10-21 7:35 ` [PATCH 3/5] m5mols: Support for interrupt in the sensor's booting time HeungJun, Kim
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: HeungJun, Kim @ 2011-10-21 7:35 UTC (permalink / raw)
To: linux-media; +Cc: HeungJun, Kim, Kyungmin Park
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, ®, 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, ®);
- 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/5] m5mols: Replace IRQ workqueue to waitqueue only
2011-10-21 7:35 ` [PATCH 2/5] m5mols: Replace IRQ workqueue to waitqueue only HeungJun, Kim
@ 2011-11-14 23:04 ` Sylwester Nawrocki
2011-11-15 7:31 ` HeungJun, Kim
0 siblings, 1 reply; 9+ messages in thread
From: Sylwester Nawrocki @ 2011-11-14 23:04 UTC (permalink / raw)
To: HeungJun, Kim; +Cc: linux-media, Kyungmin Park
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,®, 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,®);
> - 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
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH 2/5] m5mols: Replace IRQ workqueue to waitqueue only
2011-11-14 23:04 ` Sylwester Nawrocki
@ 2011-11-15 7:31 ` HeungJun, Kim
0 siblings, 0 replies; 9+ messages in thread
From: HeungJun, Kim @ 2011-11-15 7:31 UTC (permalink / raw)
To: 'Sylwester Nawrocki'; +Cc: linux-media, 'Kyungmin Park'
> -----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,®, 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,®);
> > - 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/5] m5mols: Support for interrupt in the sensor's booting time
2011-10-21 7:35 [PATCH 1/5] m5mols: Add more functions to check busy status HeungJun, Kim
2011-10-21 7:35 ` [PATCH 2/5] m5mols: Replace IRQ workqueue to waitqueue only HeungJun, Kim
@ 2011-10-21 7:35 ` HeungJun, Kim
2011-10-21 7:35 ` [PATCH 4/5] m5mols: Add boot flag for not showing the msg of I2C error HeungJun, Kim
2011-10-21 7:35 ` [PATCH 5/5] m5mols: Relocation the order and count for CAPTURE interrupt HeungJun, Kim
3 siblings, 0 replies; 9+ messages in thread
From: HeungJun, Kim @ 2011-10-21 7:35 UTC (permalink / raw)
To: linux-media; +Cc: HeungJun, Kim, Kyungmin Park
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/5] m5mols: Add boot flag for not showing the msg of I2C error
2011-10-21 7:35 [PATCH 1/5] m5mols: Add more functions to check busy status HeungJun, Kim
2011-10-21 7:35 ` [PATCH 2/5] m5mols: Replace IRQ workqueue to waitqueue only HeungJun, Kim
2011-10-21 7:35 ` [PATCH 3/5] m5mols: Support for interrupt in the sensor's booting time HeungJun, Kim
@ 2011-10-21 7:35 ` HeungJun, Kim
2011-11-14 22:35 ` Sylwester Nawrocki
2011-10-21 7:35 ` [PATCH 5/5] m5mols: Relocation the order and count for CAPTURE interrupt HeungJun, Kim
3 siblings, 1 reply; 9+ messages in thread
From: HeungJun, Kim @ 2011-10-21 7:35 UTC (permalink / raw)
To: linux-media; +Cc: HeungJun, Kim, Kyungmin Park
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 4/5] m5mols: Add boot flag for not showing the msg of I2C error
2011-10-21 7:35 ` [PATCH 4/5] m5mols: Add boot flag for not showing the msg of I2C error HeungJun, Kim
@ 2011-11-14 22:35 ` Sylwester Nawrocki
2011-11-15 7:05 ` HeungJun, Kim
0 siblings, 1 reply; 9+ messages in thread
From: Sylwester Nawrocki @ 2011-11-14 22:35 UTC (permalink / raw)
To: Heungjun Kim; +Cc: linux-media, Kyungmin Park
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
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH 4/5] m5mols: Add boot flag for not showing the msg of I2C error
2011-11-14 22:35 ` Sylwester Nawrocki
@ 2011-11-15 7:05 ` HeungJun, Kim
0 siblings, 0 replies; 9+ messages in thread
From: HeungJun, Kim @ 2011-11-15 7:05 UTC (permalink / raw)
To: 'Sylwester Nawrocki'
Cc: 'linux-media', 'Kyungmin Park'
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 5/5] m5mols: Relocation the order and count for CAPTURE interrupt
2011-10-21 7:35 [PATCH 1/5] m5mols: Add more functions to check busy status HeungJun, Kim
` (2 preceding siblings ...)
2011-10-21 7:35 ` [PATCH 4/5] m5mols: Add boot flag for not showing the msg of I2C error HeungJun, Kim
@ 2011-10-21 7:35 ` HeungJun, Kim
3 siblings, 0 replies; 9+ messages in thread
From: HeungJun, Kim @ 2011-10-21 7:35 UTC (permalink / raw)
To: linux-media; +Cc: HeungJun, Kim, Kyungmin Park
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
^ permalink raw reply related [flat|nested] 9+ messages in thread