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@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park<kyungmin.park@xxxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
