Linux IIO development
 help / color / mirror / Atom feed
From: "Nuno Sá" <noname.nuno@gmail.com>
To: "Uwe Kleine-König" <u.kleine-koenig@baylibre.com>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"David Lechner" <dlechner@baylibre.com>,
	"Dumitru Ceclan" <dumitru.ceclan@analog.com>,
	"Jonathan Cameron" <jic23@kernel.org>,
	"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
	"Lars-Peter Clausen" <lars@metafoo.de>,
	"Michael Hennerich" <Michael.Hennerich@analog.com>,
	"Nuno Sa" <nuno.sa@analog.com>, "Rob Herring" <robh@kernel.org>
Cc: devicetree@vger.kernel.org, linux-iio@vger.kernel.org
Subject: Re: [PATCH v2 3/4] iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO
Date: Wed, 30 Oct 2024 14:04:58 +0100	[thread overview]
Message-ID: <a575430a74a7825a2df9fad1a8e073ad0507b0e7.camel@gmail.com> (raw)
In-Reply-To: <20241028160748.489596-9-u.kleine-koenig@baylibre.com>

On Mon, 2024-10-28 at 17:07 +0100, Uwe Kleine-König wrote:
> Some of the ADCs by Analog signal their irq condition on the MISO line.
> So typically that line is connected to an SPI controller and a GPIO. The
> GPIO is used as input and the respective interrupt is enabled when the
> last SPI transfer is completed.
> 
> Depending on the GPIO controller the toggling MISO line might make the
> interrupt pending even while it's masked. In that case the irq handler
> is called immediately after irq_enable() and so before the device
> actually pulls that line low which results in non-sense values being
> reported to the upper layers.
> 
> The only way to find out if the line was actually pulled low is to read
> the GPIO. (There is a flag in AD7124's status register that also signals
> if an interrupt was asserted, but reading that register toggles the MISO
> line and so might trigger another spurious interrupt.)
> 
> Add the possibility to specify an interrupt GPIO in the machine
> description instead of a plain interrupt. This GPIO is used as interrupt
> source and to check if the irq line is actually active in the irq
> handler.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

Hi all,

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. Also looking at drivers as the xilinx gpio
controller, it seems some are careful about this [2] and make sure to clear all
pending IRQs when unmasking.

Jonathan also said this:

"True enough - that race is a possibility, but not all interrupt inputs
are capable of gpio usage whilst setup to received interrupts."

To my understanding this also means this is doomed to fail for some devices or am I
not following it?

All that said, my naive feeling would be for a masked line to not get any pending IRQ
and if it does, the driver should make sure to clean all outstanding interrupts when
unmasking. But I'm far from being an expert of the IRQ subsystem. Maybe it would be
interesting to get some inputs about someone who actually knows better?

[1]: https://elixir.bootlin.com/linux/v6.11.5/source/kernel/irq/chip.c#L366
[2]: https://elixir.bootlin.com/linux/v6.11.5/source/kernel/irq/chip.c#L366

- Nuno Sá



  reply	other threads:[~2024-10-30 13:05 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-28 16:07 [PATCH v2 0/4] iio: adc: ad7124: Make it work on de10-nano Uwe Kleine-König
2024-10-28 16:07 ` [PATCH v2 1/4] dt-bindings: iio: adc: adi,ad7124: Use symbolic name for interrupt flag Uwe Kleine-König
2024-10-29  7:36   ` Krzysztof Kozlowski
2024-10-28 16:07 ` [PATCH v2 2/4] dt-bindings: iio: adc: adi,ad7124: Allow specifications of a gpio for irq line Uwe Kleine-König
2024-11-01 19:20   ` Rob Herring
2024-10-28 16:07 ` [PATCH v2 3/4] iio: adc: ad_sigma_delta: Add support for reading irq status using a GPIO Uwe Kleine-König
2024-10-30 13:04   ` Nuno Sá [this message]
2024-10-30 20:44     ` Jonathan Cameron
2024-10-31 10:40       ` Uwe Kleine-König
2024-10-31 12:05         ` Nuno Sá
2024-10-31 12:28           ` Nuno Sá
2024-11-04 12:49           ` Uwe Kleine-König
2024-11-04 13:15             ` Nuno Sá
2024-11-05  9:20               ` Uwe Kleine-König
2024-11-05 10:30                 ` Nuno Sá
2024-10-28 16:07 ` [PATCH v2 4/4] iio: adc: ad7124: Disable all channels at probe time Uwe Kleine-König
2024-10-30  7:17   ` Nuno Sá
2024-10-28 16:38 ` [PATCH v2 0/4] iio: adc: ad7124: Make it work on de10-nano David Lechner
2024-11-18 18:12 ` Uwe Kleine-König
2024-11-23 14:24   ` Jonathan Cameron

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a575430a74a7825a2df9fad1a8e073ad0507b0e7.camel@gmail.com \
    --to=noname.nuno@gmail.com \
    --cc=Michael.Hennerich@analog.com \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dlechner@baylibre.com \
    --cc=dumitru.ceclan@analog.com \
    --cc=jic23@kernel.org \
    --cc=krzk+dt@kernel.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=nuno.sa@analog.com \
    --cc=robh@kernel.org \
    --cc=u.kleine-koenig@baylibre.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox