Re: [PATCH 3/5] rc: img-ir: biphase enabled with workaround
Hi Sifan,
On 11/12/14 18:54, Sifan Naeem wrote:
+/*
+ * Timer function to re-enable the current protocol after it had been
+ * cleared when invalid interrupts were generated due to a quirk in
+the
+ * img-ir decoder.
+ */
+static void img_ir_suspend_timer(unsigned long arg) {
+ struct img_ir_priv *priv = (struct img_ir_priv *)arg;
+
+ img_ir_write(priv, IMG_IR_IRQ_CLEAR,
+ IMG_IR_IRQ_ALL ~IMG_IR_IRQ_EDGE);
+
+ /* Don't set IRQ if it has changed in a different context. */
Should you even be clearing IRQs in that case? Maybe safer to just treat that
case as a return immediately without touching anything sort of situation.
don't have to clear it for this work around to work, so will remove.
+ if ((priv-hw.suspend_irqen IMG_IR_IRQ_EDGE) ==
+ img_ir_read(priv, IMG_IR_IRQ_ENABLE))
+ img_ir_write(priv, IMG_IR_IRQ_ENABLE, priv-
hw.suspend_irqen);
+ /* enable */
+ img_ir_write(priv, IMG_IR_CONTROL, priv-hw.reg_timings.ctrl); }
To clarify, I was only referring to the case where the irq mask has
changed unexpectedly. If it hasn't changed then it would seem to make
sense to clear pending interrupts (i.e. the ones we've been
intentionally ignoring) before re-enabling them.
When you say it works without, do you mean there never are pending
interrupts (if you don't press any other buttons on the remote)?
Cheers
James
signature.asc
Description: OpenPGP digital signature
RE: [PATCH 3/5] rc: img-ir: biphase enabled with workaround
-Original Message-
From: James Hogan
Sent: 12 December 2014 10:56
To: Sifan Naeem; mche...@osg.samsung.com
Cc: linux-ker...@vger.kernel.org; linux-media@vger.kernel.org; James
Hartley; Ezequiel Garcia
Subject: Re: [PATCH 3/5] rc: img-ir: biphase enabled with workaround
Hi Sifan,
On 11/12/14 18:54, Sifan Naeem wrote:
+/*
+ * Timer function to re-enable the current protocol after it had
+been
+ * cleared when invalid interrupts were generated due to a quirk in
+the
+ * img-ir decoder.
+ */
+static void img_ir_suspend_timer(unsigned long arg) {
+ struct img_ir_priv *priv = (struct img_ir_priv *)arg;
+
+ img_ir_write(priv, IMG_IR_IRQ_CLEAR,
+ IMG_IR_IRQ_ALL ~IMG_IR_IRQ_EDGE);
+
+ /* Don't set IRQ if it has changed in a different context. */
Should you even be clearing IRQs in that case? Maybe safer to just
treat that case as a return immediately without touching anything sort
of situation.
don't have to clear it for this work around to work, so will remove.
+ if ((priv-hw.suspend_irqen IMG_IR_IRQ_EDGE) ==
+ img_ir_read(priv, IMG_IR_IRQ_ENABLE))
+ img_ir_write(priv, IMG_IR_IRQ_ENABLE, priv-
hw.suspend_irqen);
+ /* enable */
+ img_ir_write(priv, IMG_IR_CONTROL, priv-hw.reg_timings.ctrl); }
To clarify, I was only referring to the case where the irq mask has changed
unexpectedly. If it hasn't changed then it would seem to make sense to clear
pending interrupts (i.e. the ones we've been intentionally ignoring) before
re-enabling them.
When you say it works without, do you mean there never are pending
interrupts (if you don't press any other buttons on the remote)?
Nope, with the change I submitted in v2 (removed the clearing IRQ) there are no
pending interrupts at the end.
But as before it goes through the workaround couple of times for each interrupt
before settling down.
Thanks,
Sifan
Cheers
James
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/5] rc: img-ir: biphase enabled with workaround
-Original Message-
From: James Hogan
Sent: 08 December 2014 17:18
To: Sifan Naeem; mche...@osg.samsung.com
Cc: linux-ker...@vger.kernel.org; linux-media@vger.kernel.org; James
Hartley; Ezequiel Garcia
Subject: Re: [PATCH 3/5] rc: img-ir: biphase enabled with workaround
On 04/12/14 15:38, Sifan Naeem wrote:
Biphase decoding in the current img-ir has got a quirk, where multiple
Interrupts are generated when an incomplete IR code is received by the
decoder.
Patch adds a work around for the quirk and enables biphase decoding.
Signed-off-by: Sifan Naeem sifan.na...@imgtec.com
---
drivers/media/rc/img-ir/img-ir-hw.c | 56
+--
drivers/media/rc/img-ir/img-ir-hw.h |2 ++
2 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/drivers/media/rc/img-ir/img-ir-hw.c
b/drivers/media/rc/img-ir/img-ir-hw.c
index 4a1407b..a977467 100644
--- a/drivers/media/rc/img-ir/img-ir-hw.c
+++ b/drivers/media/rc/img-ir/img-ir-hw.c
@@ -52,6 +52,11 @@ static struct img_ir_decoder *img_ir_decoders[] = {
#define IMG_IR_QUIRK_CODE_BROKEN 0x1 /* Decode is broken
*/
#define IMG_IR_QUIRK_CODE_LEN_INCR 0x2 /* Bit length needs
increment */
+/*
+ * The decoder generates rapid interrupts without actually having
+ * received any new data after an incomplete IR code is decoded.
+ */
+#define IMG_IR_QUIRK_CODE_IRQ 0x4
/* functions for preprocessing timings, ensuring max is set */
@@ -547,6 +552,7 @@ static void img_ir_set_decoder(struct img_ir_priv
*priv,
/* stop the end timer and switch back to normal mode */
del_timer_sync(hw-end_timer);
+ del_timer_sync(hw-suspend_timer);
FYI, this'll need rebasing due to conflicting with img-ir/hw: Fix potential
deadlock stopping timer. The new del_timer_sync will need to be when spin
lock isn't held, i.e. still next to the other one, and don't forget to ensure
that
suspend_timer doesn't get started if
hw-stopping.
Yes, I'll rebase and resend the patch.
hw-mode = IMG_IR_M_NORMAL;
/* clear the wakeup scancode filter */ @@ -843,6 +849,26 @@ static
void img_ir_end_timer(unsigned long arg)
spin_unlock_irq(priv-lock);
}
+/*
+ * Timer function to re-enable the current protocol after it had been
+ * cleared when invalid interrupts were generated due to a quirk in
+the
+ * img-ir decoder.
+ */
+static void img_ir_suspend_timer(unsigned long arg) {
+ struct img_ir_priv *priv = (struct img_ir_priv *)arg;
+
You should take the spin lock for most of this function now that
img-ir/hw: Fix potential deadlock stopping timer is applied and it is safe
to
do so.
done
+ img_ir_write(priv, IMG_IR_IRQ_CLEAR,
+ IMG_IR_IRQ_ALL ~IMG_IR_IRQ_EDGE);
+
+ /* Don't set IRQ if it has changed in a different context. */
Wouldn't hurt to clarify this while you're at it (it confused me for a moment
thinking it was concerned about the enabled raw event IRQs
(IMG_IR_IRQ_EDGE) changing).
Ok
Maybe Don't overwrite enabled valid/match IRQs if they have already been
changed by e.g. a filter change.
Should you even be clearing IRQs in that case? Maybe safer to just treat that
case as a return immediately without touching anything sort of situation.
don't have to clear it for this work around to work, so will remove.
+ if ((priv-hw.suspend_irqen IMG_IR_IRQ_EDGE) ==
+ img_ir_read(priv, IMG_IR_IRQ_ENABLE))
+ img_ir_write(priv, IMG_IR_IRQ_ENABLE, priv-
hw.suspend_irqen);
+ /* enable */
+ img_ir_write(priv, IMG_IR_CONTROL, priv-hw.reg_timings.ctrl); }
+
#ifdef CONFIG_COMMON_CLK
static void img_ir_change_frequency(struct img_ir_priv *priv,
struct clk_notifier_data *change) @@ -
908,15 +934,37 @@ void
img_ir_isr_hw(struct img_ir_priv *priv, u32 irq_status)
if (!hw-decoder)
return;
+ ct = hw-decoder-control.code_type;
+
ir_status = img_ir_read(priv, IMG_IR_STATUS);
- if (!(ir_status (IMG_IR_RXDVAL | IMG_IR_RXDVALD2)))
+ if (!(ir_status (IMG_IR_RXDVAL | IMG_IR_RXDVALD2))) {
+ if (!(priv-hw.ct_quirks[ct] IMG_IR_QUIRK_CODE_IRQ))
(I suggest adding || hw-stopping to this case)
+ return;
+ /*
+* The below functionality is added as a work around to stop
+* multiple Interrupts generated when an incomplete IR code
is
+* received by the decoder.
+* The decoder generates rapid interrupts without actually
+* having received any new data. After a single interrupt it's
+* expected to clear up, but instead multiple interrupts are
+* rapidly generated. only way to get out of this loop is to
+* reset the control register after a short delay.
+*/
+ img_ir_write(priv
Re: [PATCH 3/5] rc: img-ir: biphase enabled with workaround
On 04/12/14 15:38, Sifan Naeem wrote:
Biphase decoding in the current img-ir has got a quirk, where multiple
Interrupts are generated when an incomplete IR code is received by the
decoder.
Patch adds a work around for the quirk and enables biphase decoding.
Signed-off-by: Sifan Naeem sifan.na...@imgtec.com
---
drivers/media/rc/img-ir/img-ir-hw.c | 56
+--
drivers/media/rc/img-ir/img-ir-hw.h |2 ++
2 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/drivers/media/rc/img-ir/img-ir-hw.c
b/drivers/media/rc/img-ir/img-ir-hw.c
index 4a1407b..a977467 100644
--- a/drivers/media/rc/img-ir/img-ir-hw.c
+++ b/drivers/media/rc/img-ir/img-ir-hw.c
@@ -52,6 +52,11 @@ static struct img_ir_decoder *img_ir_decoders[] = {
#define IMG_IR_QUIRK_CODE_BROKEN 0x1 /* Decode is broken */
#define IMG_IR_QUIRK_CODE_LEN_INCR 0x2 /* Bit length needs increment */
+/*
+ * The decoder generates rapid interrupts without actually having
+ * received any new data after an incomplete IR code is decoded.
+ */
+#define IMG_IR_QUIRK_CODE_IRQ0x4
/* functions for preprocessing timings, ensuring max is set */
@@ -547,6 +552,7 @@ static void img_ir_set_decoder(struct img_ir_priv *priv,
/* stop the end timer and switch back to normal mode */
del_timer_sync(hw-end_timer);
+ del_timer_sync(hw-suspend_timer);
FYI, this'll need rebasing due to conflicting with img-ir/hw: Fix
potential deadlock stopping timer. The new del_timer_sync will need to
be when spin lock isn't held, i.e. still next to the other one, and
don't forget to ensure that suspend_timer doesn't get started if
hw-stopping.
hw-mode = IMG_IR_M_NORMAL;
/* clear the wakeup scancode filter */
@@ -843,6 +849,26 @@ static void img_ir_end_timer(unsigned long arg)
spin_unlock_irq(priv-lock);
}
+/*
+ * Timer function to re-enable the current protocol after it had been
+ * cleared when invalid interrupts were generated due to a quirk in the
+ * img-ir decoder.
+ */
+static void img_ir_suspend_timer(unsigned long arg)
+{
+ struct img_ir_priv *priv = (struct img_ir_priv *)arg;
+
You should take the spin lock for most of this function now that
img-ir/hw: Fix potential deadlock stopping timer is applied and it is
safe to do so.
+ img_ir_write(priv, IMG_IR_IRQ_CLEAR,
+ IMG_IR_IRQ_ALL ~IMG_IR_IRQ_EDGE);
+
+ /* Don't set IRQ if it has changed in a different context. */
Wouldn't hurt to clarify this while you're at it (it confused me for a
moment thinking it was concerned about the enabled raw event IRQs
(IMG_IR_IRQ_EDGE) changing).
Maybe Don't overwrite enabled valid/match IRQs if they have already
been changed by e.g. a filter change.
Should you even be clearing IRQs in that case? Maybe safer to just treat
that case as a return immediately without touching anything sort of
situation.
+ if ((priv-hw.suspend_irqen IMG_IR_IRQ_EDGE) ==
+ img_ir_read(priv, IMG_IR_IRQ_ENABLE))
+ img_ir_write(priv, IMG_IR_IRQ_ENABLE, priv-hw.suspend_irqen);
+ /* enable */
+ img_ir_write(priv, IMG_IR_CONTROL, priv-hw.reg_timings.ctrl);
+}
+
#ifdef CONFIG_COMMON_CLK
static void img_ir_change_frequency(struct img_ir_priv *priv,
struct clk_notifier_data *change)
@@ -908,15 +934,37 @@ void img_ir_isr_hw(struct img_ir_priv *priv, u32
irq_status)
if (!hw-decoder)
return;
+ ct = hw-decoder-control.code_type;
+
ir_status = img_ir_read(priv, IMG_IR_STATUS);
- if (!(ir_status (IMG_IR_RXDVAL | IMG_IR_RXDVALD2)))
+ if (!(ir_status (IMG_IR_RXDVAL | IMG_IR_RXDVALD2))) {
+ if (!(priv-hw.ct_quirks[ct] IMG_IR_QUIRK_CODE_IRQ))
(I suggest adding || hw-stopping to this case)
+ return;
+ /*
+ * The below functionality is added as a work around to stop
+ * multiple Interrupts generated when an incomplete IR code is
+ * received by the decoder.
+ * The decoder generates rapid interrupts without actually
+ * having received any new data. After a single interrupt it's
+ * expected to clear up, but instead multiple interrupts are
+ * rapidly generated. only way to get out of this loop is to
+ * reset the control register after a short delay.
+ */
+ img_ir_write(priv, IMG_IR_CONTROL, 0);
+ hw-suspend_irqen = img_ir_read(priv, IMG_IR_IRQ_ENABLE);
You're reusing hw-suspend_irqen. What if you get this workaround being
activated between img_ir_enable_wake() and img_ir_disable_wake()? I
suggest just using a new img_ir_priv_hw member.
The rest looks reasonable to me, even if unfortunate that it is
necessary in the first place.
Thanks for the hard work!
Cheers
James
+
