Re: [PATCH v2 3/4] iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO

["Nuno Sá" <noname.nuno@gmail.com>]

On Mon, 2024-11-04 at 13:49 +0100, Uwe Kleine-König wrote:
> Hello,
> 
> [adding rmk to Cc as the docs state that he invented lazy disabling]
> 
> On Thu, Oct 31, 2024 at 01:05:21PM +0100, Nuno Sá wrote:
> > On Thu, 2024-10-31 at 11:40 +0100, Uwe Kleine-König wrote:
> > > On Wed, Oct 30, 2024 at 08:44:29PM +0000, Jonathan Cameron wrote:
> > > > On Wed, 30 Oct 2024 14:04:58 +0100
> > > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > > Regarding this, I do share some of the concerns already raised by Jonathan.
> > > > > I fear
> > > > > that we're papering around an issue with the IRQ controller rather than
> > > > > being an
> > > > > issue with the device. When I look at irq_disable() docs [1], it feels that
> > > > > we're
> > > > > already doing what we're supposed to do. IOW, we disable the lazy approach
> > > > > so we
> > > > > *should* not get any pending IRQ.
> > > 
> > > I think this is wrong and you always have to be prepared to see an irq
> > > triggering that became pending while masked.
> 
> I did some research, here are my findings:
> 
> https://www.kernel.org/doc/html/v6.12-rc6/core-api/genericirq.html#delayed-interrupt-disable
> reads:
> 
> 	The interrupt is kept enabled and is masked in the flow handler
> 	when an interrupt event happens. This prevents losing edge
> 	interrupts on hardware which does not store an edge interrupt
> 	event while the interrupt is disabled at the hardware level.
> 
> This suggests that lazy disabling is needed for some controllers that
> stop their event detection when disabled. I read that as: *Normally* an
> irq event gets pending in hardware while the irq is disabled.

I might be wrong, but I think that the lazy approach is the one for optimization. I
think the reasoning is that __normally__ no more IRQs will come so no need to access
the HW. But also thinking more on the subject and looking at what the lazy approach
is doing, I take back what I said in previous emails. I *think* the expectation for a
received IRQ while the line is masked (or disabled?!), is to keep it as pending (both
on HW - when possible - and in SW).

> 
> The lazy disable approach is expected to work fine always, the reason to
> implement non-lazy disabling is "only" a performance optimisation. See
> commit e9849777d0e27cdd2902805be51da73e7c79578c.

Not sure If I understood you correctly, but I think is the other way around? 
Also, as said in the commit, I think it also prevents the same interrupt from
happening twice (in some cases).

> 
> With the DOUT/̅R̅D̅Y pin the ad7124 (and others) is in this "Unfortunately
> there are devices which do not allow the interrupt to be disabled easily
> at the device level." class.
> 
> However that makes me wonder what is the difference between the
> irq_mask() and irq_disable() callbacks defined in struct irq_chip.

Wondering the same...

Thanks for digging into this. This has been a long standing thing with sigma delta
ADCs (I'm fairly sure this discussion about being an issue on the IRQ controller or
not already happened before).

- Nuno Sá