Message-ID: <CAPJMGm5WxnmtyW2DnZe4rSUFJ-KtKGmNsf7pStcaK=4suBHWuw@mail.gmail.com>
Date:   Wed, 5 Apr 2023 15:20:49 +0200
From:   Fabrizio Lamarque <fl.scratchpad@...il.com>
To:     Nuno Sá <noname.nuno@...il.com>
Cc:     alexandru.tachici@...log.com, linux-kernel@...r.kernel.org,
        Jonathan Cameron <jic23@...nel.org>
Subject: Re: iio: ad7192: Pending IRQ on SDO/RDY causing retrigger of
 interrupt handler and missing every sample

On Wed, Apr 5, 2023 at 12:30 PM Nuno Sá <noname.nuno@...il.com> wrote:
> On Wed, 2023-04-05 at 11:53 +0200, Fabrizio Lamarque wrote:
> > Link:
> > https://lore.kernel.org/all/20210906065630.16325-3-alexandru.tachici@analog.com/
> >
> > This commit broke the driver functionality, i.e. cat in_voltage1_raw
> > triggers a correct sampling sequence only the first time, then it
> > always returns the first sampled value.
> >
> > Following the sequence of ad_sigma_delta_single_conversion() within
> > ad_sigma_delta.c, this is due to:
> >
> > - IRQ resend mechanism is always enabled for ARM cores
> > (CONFIG_HARDIRQS_SW_RESEND)
> > - Edge IRQs are always made pending when a corresponding event
> > happens, even after disable_irq(). This is intentional and designed to
> > prevent missing signal edges.
> > - Level IRQs are not impacted by IRQ resend (i.e. IRQ_PENDING is
> > always cleared).
> > - SPI communication causes the IRQ to be set pending (even if
> > corresponding interrupt is disabled)
> > - The second time ad_sigma_delta_single_conversion() is called,
> > enable_irq() will trigger the interrupt immediately, even if RDY line
> > is high.
> > - In turn, this causes the call
> > wait_for_completion_interruptible_timeout() to exit immediately, with
> > RDY line still high.
> > - Right after the SPI register read, the MODE register is written with
> > AD_SD_MODE_IDLE, and pending conversion is stopped. Hence DATA
> > register is never updated.
> >
> > I would suggest to revert this commit or set the flag with
> > IRQF_TRIGGER_LOW instead of IRQF_TRIGGER_FALLING, but I am not sure
> > about the problem solved by this commit and how to replicate it.
> > Another option would be to keep IRQ flags within bindings only.
> >
>
> I'm starting to think that what's stated in that commit
>
> "Leaving IRQ on level instead of falling might trigger a sample read
> when the IRQ is enabled, as the SDO line is already low. "
>
> might actually be related with something else...

Hi Nuno Sá,
 thank you again for your replies and the time you are spending in checking
the reported issues.
I see what you mean...
I was asking for details on the actual issue that was solved to have a better
understanding of the change.

>
> > As a side note, AD7192 datasheet says that the falling edge on SDO
> > line _can_ be used as an interrupt to processor, but it also states
> > that the _level_ on SDO/RDY line indicates the sample is ready. What
> > happens on SDO/RDY interrupt line before the ADC conversion is
> > triggered should be ignored.
> >
>
> Well, from the datasheet (as you already know):
>
> "
> In addition, DOUT/RDY operates as a data ready pin, going low to indicate
> the completion of a conversion. If the data is not read after the conversion,
> the pin goes high before the next
> update occurs. The DOUT/RDY falling edge can be used as an interrupt to a
> processor, indicating that valid
> data is available.
> "
>
> So I really read IRQF_TRIGGER_FALLING in the above and all the other sigma
> delta devices have the same setting (I think). So the fact that it works with
> level IRQs might just be lucky instead of fixing the real problem. Can you try
> the below patch (I'm starting to think it might be related):
>

We have been using those ADC series for about a decade, the shared SDO/RDY
signal has its own quirks.
We also discussed several years ago in a support ticket with ADI, both
level and edge
sensing should be acceptable, provided that limitations are understood.

>
> https://lore.kernel.org/linux-iio/20230306044737.862-1-honda@mechatrax.com/

I just tested the patch, but, at least on the platform I'm working on
(I.MX6), it does not
solve the issue.
Whereas the thread describes the very same issue I am experiencing, I
am not sure
IRQ_DISABLE_UNLAZY would have any impact. By reading CPU registers I see
the IRQ line disabled at the hardware level, but when the IRQ flag of
the processor
is set (and this happens even if the interrupt is masked in HW), the
interrupt is immediately
triggered anyway.
(I see GPIOx_IMR cleared, so interrupt is masked, but GPIOx_ISR set. As soon as
enable_irq() is called the interrupt is triggered. Just by clearing
GPIOx_ISR before
enable_irq() solves the issue. I might share a few debug printk).

The "real issue" here is that we have a pending IRQ on the RDY line set
even if IRQ is disabled (masked) at the hardware level.
It does not happen for level sensing interrupts.

This link might further clarify why this should be intentional:
https://www.kernel.org/doc/html/latest/core-api/genericirq.html#delayed-interrupt-disable
(note that I see the IRQ masked at the hardware level anyway, but it
does not prevent
the described behavior to happen)

In case the interrupt flag has to be IRQF_TRIGGER_FALLING, it might be viable to
enable the IRQ before the measurement is requested and eventually
ignore any spurious
interrupt until the measurement is started (SPI write completed).

Fabrizio Lamarque

Powered by blists - more mailing lists