RE: [PATCH v2 15/16] iio: adc: max1027: Add support for external triggers

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, September 5, 2021 6:11 PM
> To: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Thomas Petazzoni
> <thomas.petazzoni@xxxxxxxxxxx>; Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> >  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á