> -----Original Message-----
> From: James Hogan
> Sent: 08 December 2014 17:18
> To: Sifan Naeem; [email protected]
> Cc: [email protected]; [email protected]; 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 <[email protected]>
> > ---
> > 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, 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.
>
Sure.
Thanks,
Sifan
> The rest looks reasonable to me, even if unfortunate that it is necessary in
> the first place.
>
> Thanks for the hard work!
>
> Cheers
> James
>
> > + img_ir_write(priv, IMG_IR_IRQ_ENABLE,
> > + hw->suspend_irqen & IMG_IR_IRQ_EDGE);
> > +
> > + /* Timer activated to re-enable the protocol. */
> > + mod_timer(&hw->suspend_timer,
> > + jiffies + msecs_to_jiffies(5));
> > return;
> > + }
> > ir_status &= ~(IMG_IR_RXDVAL | IMG_IR_RXDVALD2);
> > img_ir_write(priv, IMG_IR_STATUS, ir_status);
> >
> > len = (ir_status & IMG_IR_RXDLEN) >> IMG_IR_RXDLEN_SHIFT;
> > /* some versions report wrong length for certain code types */
> > - ct = hw->decoder->control.code_type;
> > if (hw->ct_quirks[ct] & IMG_IR_QUIRK_CODE_LEN_INCR)
> > ++len;
> >
> > @@ -958,7 +1006,7 @@ static void img_ir_probe_hw_caps(struct
> img_ir_priv *priv)
> > hw->ct_quirks[IMG_IR_CODETYPE_PULSELEN]
> > |= IMG_IR_QUIRK_CODE_LEN_INCR;
> > hw->ct_quirks[IMG_IR_CODETYPE_BIPHASE]
> > - |= IMG_IR_QUIRK_CODE_BROKEN;
> > + |= IMG_IR_QUIRK_CODE_IRQ;
> > hw->ct_quirks[IMG_IR_CODETYPE_2BITPULSEPOS]
> > |= IMG_IR_QUIRK_CODE_BROKEN;
> > }
> > @@ -977,6 +1025,8 @@ int img_ir_probe_hw(struct img_ir_priv *priv)
> >
> > /* Set up the end timer */
> > setup_timer(&hw->end_timer, img_ir_end_timer, (unsigned
> long)priv);
> > + setup_timer(&hw->suspend_timer, img_ir_suspend_timer,
> > + (unsigned long)priv);
> >
> > /* Register a clock notifier */
> > if (!IS_ERR(priv->clk)) {
> > diff --git a/drivers/media/rc/img-ir/img-ir-hw.h
> > b/drivers/media/rc/img-ir/img-ir-hw.h
> > index 5e59e8e..8578aa7 100644
> > --- a/drivers/media/rc/img-ir/img-ir-hw.h
> > +++ b/drivers/media/rc/img-ir/img-ir-hw.h
> > @@ -221,6 +221,7 @@ enum img_ir_mode {
> > * @rdev: Remote control device
> > * @clk_nb: Notifier block for clock notify events.
> > * @end_timer: Timer until repeat timeout.
> > + * @suspend_timer: Timer to re-enable protocol.
> > * @decoder: Current decoder settings.
> > * @enabled_protocols: Currently enabled protocols.
> > * @clk_hz: Current core clock rate in Hz.
> > @@ -235,6 +236,7 @@ struct img_ir_priv_hw {
> > struct rc_dev *rdev;
> > struct notifier_block clk_nb;
> > struct timer_list end_timer;
> > + struct timer_list suspend_timer;
> > const struct img_ir_decoder *decoder;
> > u64 enabled_protocols;
> > unsigned long clk_hz;
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
