LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Sa, Nuno" <Nuno.Sa@analog.com>
To: Jonathan Cameron <jic23@kernel.org>,
	Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
	"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: RE: [PATCH v2 15/16] iio: adc: max1027: Add support for external triggers
Date: Mon, 6 Sep 2021 09:53:15 +0000	[thread overview]
Message-ID: <SA1PR03MB6355E88079A1B425FC236F3399D29@SA1PR03MB6355.namprd03.prod.outlook.com> (raw)
In-Reply-To: <20210905171046.1681482d@jic23-huawei>



> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, September 5, 2021 6:11 PM
> To: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org;
> linux-kernel@vger.kernel.org; Thomas Petazzoni
> <thomas.petazzoni@bootlin.com>; Sa, Nuno <Nuno.Sa@analog.com>
> Subject: Re: [PATCH v2 15/16] iio: adc: max1027: Add support for
> external triggers
> 
> [External]
> 
> 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 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.
> 
> Jonathan
> 
> 
> 
> >
> > In order to authorize external triggers, we need to drop the
> > ->validate_trigger() verification.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/iio/adc/max1027.c | 59
> +++++++++++++++++++++++++++++++--------
> >  1 file changed, 47 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > index e734d32a5507..b9b7b9245509 100644
> > --- a/drivers/iio/adc/max1027.c
> > +++ b/drivers/iio/adc/max1027.c
> > @@ -414,17 +414,6 @@ static int
> max1027_debugfs_reg_access(struct iio_dev *indio_dev,
> >  	return spi_write(st->spi, val, 1);
> >  }
> >
> > -static int max1027_validate_trigger(struct iio_dev *indio_dev,
> > -				    struct iio_trigger *trig)
> > -{
> > -	struct max1027_state *st = iio_priv(indio_dev);
> > -
> > -	if (st->trig != trig)
> > -		return -EINVAL;
> > -
> > -	return 0;
> > -}
> > -
> >  static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig,
> bool state)
> >  {
> >  	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > @@ -469,6 +458,13 @@ static int max1027_read_scan(struct iio_dev
> *indio_dev)
> >  	return 0;
> >  }
> >
> > +static bool max1027_own_trigger_enabled(struct iio_dev
> *indio_dev)
> > +{
> > +	int ret = iio_trigger_validate_own_device(indio_dev->trig,
> indio_dev);
> > +
> > +	return ret ? false : true;
> > +}
> > +
> >  static irqreturn_t max1027_threaded_handler(int irq, void *private)
> >  {
> >  	struct iio_poll_func *pf = private;
> > @@ -487,7 +483,47 @@ static irqreturn_t
> max1027_threaded_handler(int irq, void *private)
> >  		return IRQ_HANDLED;
> >  	}
> >
> > +	/* From that point on, buffers are enabled */
> > +
> > +	/*
> > +	 * The cnvst HW trigger is not in use:
> > +	 * we need to handle an external trigger request.
> > +	 */
> > +	if (!max1027_own_trigger_enabled(indio_dev)) {
> > +		if (irq != st->spi->irq) {
> > +			/*
> > +			 * First, the IRQ number will be the one
> allocated for
> > +			 * this poll function by the IIO core, it means
> that
> > +			 * this is an external trigger request, we need to
> start
> > +			 * a conversion.
> > +			 */
> > +			ret =
> max1027_configure_chans_and_start(indio_dev);
> > +			if (ret)
> > +				goto out;
> > +
> > +			ret = max1027_wait_eoc(indio_dev);
> > +			if (ret)
> > +				goto out;
> > +		} else {
> > +			/*
> > +			 * The pollfunc that has been called "manually"
> by the
> > +			 * IIO core now expects the EOC signaling (this
> is the
> > +			 * device IRQ firing), we need to call
> complete().
> > +			 */
> > +			complete(&st->complete);
> 
> Completion shouldn't be down here in the trigger handler, it should be
> in the top
> level interrupt handler.  So you need to replace the
> iio_trigger_generic_data_poll with a specific handler for this device.
> 
> > +			return IRQ_HANDLED;
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * We end here under two situations:
> > +	 * - an external trigger is in use and the *_wait_eoc() call
> succeeded,
> > +	 *   the data is ready and may be retrieved.
> > +	 * - the cnvst HW trigger is in use (the handler actually starts
> here),
> > +	 *   the data is also ready.
> > +	 */
> >  	ret = max1027_read_scan(indio_dev);
> > +out:
> >  	if (ret)
> >  		dev_err(&indio_dev->dev,
> >  			"Cannot read scanned values (%d)\n", ret);
> > @@ -504,7 +540,6 @@ static const struct iio_trigger_ops
> max1027_trigger_ops = {
> >
> >  static const struct iio_info max1027_info = {
> >  	.read_raw = &max1027_read_raw,
> > -	.validate_trigger = &max1027_validate_trigger,
> >  	.debugfs_reg_access = &max1027_debugfs_reg_access,
> >  };
> >

I'm also confused by this. Going through the series, I was actually
thinking that raw_reads were in fact using the EOC IRQ until I realized
that 'complete()' was being called from the trigger handler... So,
I'm also not sure how is this supposed to work? But I'm probably
missing something as I guess you tested this and how I'm understanding
things, you should have gotten timeouts for raw_reads.

Anyways, as Jonathan said, I was also expecting to see the 'complete()' call
from the device IRQ handler. Other thing than that is just asking for trouble
:). 

- Nuno Sá

  reply	other threads:[~2021-09-06  9:53 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 [this message]
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
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=SA1PR03MB6355E88079A1B425FC236F3399D29@SA1PR03MB6355.namprd03.prod.outlook.com \
    --to=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=miquel.raynal@bootlin.com \
    --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).