Re: iio: ad7192: Pending IRQ on SDO/RDY causing retrigger of interrupt handler and missing every sample

[Nuno Sá]

Hi Fabrizio,

Thanks for reporting this...

On Wed, 2023-04-05 at 11:53 +0200, Fabrizio Lamarque wrote:
> Link:
> https://lore.kernel.org/all/20210906065630.16325-3-alexandru.tachici@xxxxxxxxxx/
> 
> 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...

> 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):


https://lore.kernel.org/linux-iio/20230306044737.862-1-honda@xxxxxxxxxxxxx/


- Nuno Sá