LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Nuno Sa <Nuno.Sa@analog.com>
Subject: Re: [PATCH v2 15/16] iio: adc: max1027: Add support for external triggers
Date: Wed, 15 Sep 2021 12:18:32 +0200	[thread overview]
Message-ID: <20210915121832.7766fdd7@xps13> (raw)
In-Reply-To: <20210905171046.1681482d@jic23-huawei>

Hi Jonathan, Nuno,

jic23@kernel.org wrote on Sun, 5 Sep 2021 17:10:46 +0100:

> On Thu,  2 Sep 2021 23:14:36 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > So far the driver only supported to use the hardware cnvst trigger. This
> > was purely a software limitation.
> > 
> > The IRQ handler is already registered as being a poll function and thus
> > can be called upon external triggering. In this case, a new conversion
> > must be started, and one must wait for the data to be ready before
> > reading the samples.
> > 
> > As the same handler can be called from different places, we check the
> > value of the current IRQ with the value of the registered device
> > IRQ. Indeed, the first step is to get called with a different IRQ number
> > than ours, this is the "pullfunc" version which requests a new  
> 
> pullfunc?
> 
> > conversion. During the execution of the handler, we will wait for the
> > EOC interrupt to happen. This interrupt is handled by the same
> > helper. This time the IRQ number is the one we registered, we can in
> > this case call complete() to unlock the primary handler and return. The
> > primary handler continues executing by retrieving the data normally and
> > finally returns.  
> 
> Interesting to use the irq number..
> 
> I'm a little nervous about how this has ended up structured.
> I'm not 100% sure my understanding of how you've done it is correct.
> 
> We should have the following situation:
> 
> IRQ IN
>   |
>   v
> Trigger IRQ / EOC IRQ  (this is the spi->irq)  (currently iio_trigger_generic_data_poll_ready)
>   |              |
>   ---------      v
>   |        |   complete
>   v        v
> TrigH1    (TrigH2)   (these are the IRQs below the irq_chip IIO uses to demux triggers)
> 
> 
> So when using it's own trigger we are using an internal interrupt
> tree burried inside the IIO core.  When using it only as an EOC interrupt we shouldn't
> be anywhere near that internal interrupt chip.
> 
> So I'm surprised the IRQ matches with the spi->irq as 
> those trigH1 and trigH2 will have their own IRQ numbers.
> 
> For reference I think your architecture is currently
> 
> IRQ IN
>   |
>   v
>   Trigger IRQ
>   |
>   v
>  TRIG H1
>  Either fills the buffer or does the completion.
> 
> I am a little confused how this works with an external trigger because the Trig H1 interrupt
> should be disabled unless we are using the trigger.  That control isn't exposed to the
> driver at all.
> 
> Is my understanding right or have I gotten confused somewhere?

I think the confusion comes from the fact that in the
current implementation, Trigger IRQ and EOC IRQ handlers are the same.
This comes from a possible misunderstanding in the previous review,
where I understood that you and Nuno wanted to keep using
iio_trigger_generic_data_rdy_poll() hand have a single handler in the
driver (which I think is far from optimal). I can try to split that
handler again to have two distinct paths.

> I also can't see a path in which the eoc interrupt will get fired for raw_reads.
> 
> Could you talk me through how that works currently?
> 
> I suspect part of the confusion here is that this driver happens to be using the
> standard core handler iio_trigger_generic_data_rdy_poll which hides away that
> there are two interrupt handlers in a normal IIO driver for a device with a
> trigger and buffered mode.
> 1 for the trigger and 1 for the buffer.  Whether the buffer one is a result
> of the trigger one (via iio_poll_trigger) is down to whether the device is
> using it's own trigger or not.

Also, to answer Nuno about the question: is this actually working: IIRC
I mentioned it in the cover letter but my hardware does not have the
EOC line wired so I am unable to actually test that I am not breaking
this. My main goal is to be able to use external triggers (such as a
timer) and I am a bit struggling with the constraints of my hardware +
the design of this chip.

I will provide a third implementation in v3 and if this still does not
fit your mental model please guide me with maybe an untested code
snippet just to show me how you think this should be implemented.

Thank you both for the numerous reviews and precious feedback anyway!
Miquèl


  parent reply	other threads:[~2021-09-15 10:18 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-02 21:14 [PATCH v2 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 01/16] iio: adc: max1027: Fix style Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 02/16] iio: adc: max1027: Drop extra warning message Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 03/16] iio: adc: max1027: Drop useless debug messages Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 04/16] iio: adc: max1027: Avoid device managed allocators for DMA purposes Miquel Raynal
2021-09-05 15:26   ` Jonathan Cameron
2021-09-02 21:14 ` [PATCH v2 05/16] iio: adc: max1027: Minimize the number of converted channels Miquel Raynal
2021-09-05 15:34   ` Jonathan Cameron
2021-09-15  9:46     ` Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 06/16] iio: adc: max1027: Rename a helper Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 07/16] iio: adc: max1027: Create a helper to enable/disable the cnvst trigger Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 08/16] iio: adc: max1027: Simplify the _set_trigger_state() helper Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 09/16] iio: adc: max1027: Ensure a default cnvst trigger configuration Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 10/16] iio: adc: max1027: Create a helper to configure the channels to scan Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 11/16] iio: adc: max1027: Prevent single channel accesses during buffer reads Miquel Raynal
2021-09-05 15:38   ` Jonathan Cameron
2021-09-02 21:14 ` [PATCH v2 12/16] iio: adc: max1027: Separate the IRQ handler from the read logic Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 13/16] iio: adc: max1027: Introduce an end of conversion helper Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 14/16] iio: adc: max1027: Don't just sleep when the EOC interrupt is available Miquel Raynal
2021-09-06  9:38   ` Sa, Nuno
2021-09-15  9:55     ` Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 15/16] iio: adc: max1027: Add support for external triggers Miquel Raynal
2021-09-05 16:10   ` Jonathan Cameron
2021-09-06  9:53     ` Sa, Nuno
2021-09-15 15:45       ` Miquel Raynal
2021-09-18 17:39         ` Jonathan Cameron
2021-09-20 10:47           ` Miquel Raynal
2021-09-15 10:18     ` Miquel Raynal [this message]
2021-09-18 17:13       ` Jonathan Cameron
2021-09-20  8:37         ` Miquel Raynal
2021-09-20 17:43           ` Jonathan Cameron
2021-09-21  8:11             ` Miquel Raynal
2021-09-02 21:14 ` [PATCH v2 16/16] iio: adc: max1027: Don't reject external triggers when there is no IRQ Miquel Raynal

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=20210915121832.7766fdd7@xps13 \
    --to=miquel.raynal@bootlin.com \
    --cc=Nuno.Sa@analog.com \
    --cc=jic23@kernel.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=thomas.petazzoni@bootlin.com \
    --subject='Re: [PATCH v2 15/16] iio: adc: max1027: Add support for external triggers' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).