> -----Original Message----- > From: linux-media-owner@xxxxxxxxxxxxxxx [mailto:linux-media- > owner@xxxxxxxxxxxxxxx] On Behalf Of Sylwester Nawrocki > Sent: Tuesday, November 15, 2011 8:04 AM > To: HeungJun, Kim > Cc: linux-media@xxxxxxxxxxxxxxx; 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@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 ? 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html Thanks for the comments. I'll re-works this patches. Regards, Heungjun Kim -- 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
