Until now the max1027.c driver, which handles 10-bit devices (max10xx)
and 12-bit devices (max12xx), only supported hardware triggers. When a
hardware trigger is not wired it is very convenient to trigger periodic
conversions with timers or on userspace demand with a sysfs
trigger. Overall, when several values are needed at the same time, using
triggers and buffers improves quite a lot the performances.
This series does a bit of cleaning/code reorganization before actually
bringing more flexibility to the driver, up to the point where it is
possible to use an external trigger (hardware or software) even without
the IRQ line wired.
This series is currently based on a v4.14-rc1 kernel and the external
triggering mechanism has been tested on a custom board where the IRQ and
the EOC lines have not been populated.
How to test sysfs triggers:
echo 0 > /sys/bus/iio/devices/iio_sysfs_trigger/add_trigger
cat /sys/bus/iio/devices/iio_sysfs_trigger/trigger0/name > \
/sys/bus/iio/devices/iio:device0/trigger/current_trigger
echo 1 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltageX_en
echo 1 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltageY_en
echo 1 > /sys/bus/iio/devices/iio:device0/buffer/enable
cat /dev/iio\:device0 > /tmp/data &
echo 1 > /sys/bus/iio/devices/trigger0/trigger_now
od -t x1 /tmp/data
Cheers,
Miquèl
Changes in v2:
[All]
* Overall quite a few changes, I'll try to list them here but I made
significant changes on the last few commits so it's hard to have an
exhaustive and detailed list.
* Simplified the return statements as advised by Nuno.
* Dropped useless debug messages.
* Used iio_trigger_validate_own_device() instead of an internal
variable when possible.
* Added Nuno's Reviewed-by's when relevant.
[Created a new patch to fix the style]
[Created a new patch to ensure st->buffer is DMA-safe]
[Push only the requested samples]
* Dropped a useless check over active_scan_mask mask in
->set_trigger_state().
* Dropped the st->buffer indirection with a missing __be16 type.
* Do not push only the requested samples in the IIO buffers, rely on the
core to handle this by providing additional 'available_scan_masks'
instead of dropping this entry from the initial setup.
[Create a helper to configure the trigger]
* Avoided messing with new lines.
* Dropped cnvst_trigger, used a function parameter instead.
[Prevent single channel accesses during buffer reads]
* Used iio_device_claim_direct_mode() when relevant.
* Dropped the extra iio_buffer_enabled() call.
* Prevented returning with a mutex held.
[Introduce an end of conversion helper]
* Moved the check against active scan mask to the very end of the series
where we actually make use of it.
* Moved the Queue declaration to another patch.
[Dropped the patch: Prepare re-using the EOC interrupt]
[Consolidate the end of conversion helper]
* Used a dynamic completion object instead of a static queue.
* Reworded the commit message to actually describe what this commit
does.
[Support software triggers]
* Dropped the patch and replaced it with something hopefully close to
what Jonathan and Nuno described in their reviews.
[Enable software triggers to be used without IRQ]
* Wrote a more generic commit message, not focusing on software
triggers.
Miquel Raynal (16):
iio: adc: max1027: Fix style
iio: adc: max1027: Drop extra warning message
iio: adc: max1027: Drop useless debug messages
iio: adc: max1027: Avoid device managed allocators for DMA purposes
iio: adc: max1027: Minimize the number of converted channels
iio: adc: max1027: Rename a helper
iio: adc: max1027: Create a helper to enable/disable the cnvst trigger
iio: adc: max1027: Simplify the _set_trigger_state() helper
iio: adc: max1027: Ensure a default cnvst trigger configuration
iio: adc: max1027: Create a helper to configure the channels to scan
iio: adc: max1027: Prevent single channel accesses during buffer reads
iio: adc: max1027: Separate the IRQ handler from the read logic
iio: adc: max1027: Introduce an end of conversion helper
iio: adc: max1027: Don't just sleep when the EOC interrupt is
available
iio: adc: max1027: Add support for external triggers
iio: adc: max1027: Don't reject external triggers when there is no IRQ
drivers/iio/adc/max1027.c | 300 ++++++++++++++++++++++++++++----------
1 file changed, 219 insertions(+), 81 deletions(-)
--
2.27.0
We don't expect the (hardware) cnvst trigger to be enabled at boot time,
this is a user choice made in sysfs and there is a dedicated callback to
enable/disable this trigger. Hence, we can just ensure it is disabled in
the probe at initialization time and then assume that whenever a
->read_raw() call happens, the trigger has been disabled and conversions
will start on register write.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/iio/adc/max1027.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 9269931ca09d..476b90f12330 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -288,10 +288,6 @@ static int max1027_read_single_value(struct iio_dev *indio_dev,
return -EBUSY;
}
- ret = max1027_enable_trigger(indio_dev, false);
- if (ret)
- return ret;
-
/* Configure conversion register with the requested chan */
st->reg = MAX1027_CONV_REG | MAX1027_CHAN(chan->channel) |
MAX1027_NOSCAN;
@@ -541,6 +537,11 @@ static int max1027_probe(struct spi_device *spi)
goto free_buffer;
}
+ /* Assume conversion on register write for now */
+ ret = max1027_enable_trigger(indio_dev, false);
+ if (ret)
+ return ret;
+
return devm_iio_device_register(&spi->dev, indio_dev);
free_buffer:
--
2.27.0
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
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.
In order to authorize external triggers, we need to drop the
->validate_trigger() verification.
Signed-off-by: Miquel Raynal <[email protected]>
---
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);
+ 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,
};
--
2.27.0
On Thu, 2 Sep 2021 23:14:36 +0200
Miquel Raynal <[email protected]> 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 <[email protected]>
> ---
> 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,
> };
>
> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Sunday, September 5, 2021 6:11 PM
> To: Miquel Raynal <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>; [email protected];
> [email protected]; Thomas Petazzoni
> <[email protected]>; Sa, Nuno <[email protected]>
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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?
Hi Jonathan, Nuno,
[email protected] wrote on Sun, 5 Sep 2021 17:10:46 +0100:
> On Thu, 2 Sep 2021 23:14:36 +0200
> Miquel Raynal <[email protected]> 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
Hi Nuno, Jonathan,
[email protected] wrote on Mon, 6 Sep 2021 09:53:15 +0000:
> > -----Original Message-----
> > From: Jonathan Cameron <[email protected]>
> > Sent: Sunday, September 5, 2021 6:11 PM
> > To: Miquel Raynal <[email protected]>
> > Cc: Lars-Peter Clausen <[email protected]>; [email protected];
> > [email protected]; Thomas Petazzoni
> > <[email protected]>; Sa, Nuno <[email protected]>
> > 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 <[email protected]> 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 forgot to do this, I think you misunderstood the following patch:
"iio: adc: max1027: Don't just sleep when the EOC interrupt is
available"
As I am having deep troubles reworking this once again, I will try to
explain how this is build below and look for your feedback on it.
> >
> > 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.
> >
[...]
> 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?
I renamed this handler with a generic name, because it actually serves
several different purposes, see below.
> 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á
So here is how I understand the device:
* During a raw read:
The IRQ indicates an EOC.
* During a triggered read (internal trigger):
The driver has no knowledge of the trigger being fired, it only
sees the IRQ firing at EOC. This means the internal trigger cannot be
used without the IRQ.
Now here is what I understand from the IIO subsystem and what I tried
to implement:
* Perform a raw read:
The driver needs to setup the channels, start the operation, wait for
the end of the conversion, return the value. This is all done in the
->raw_read() hook in process context. Raw reads can either use the
EOC IRQ if wired or just wait for a sufficiently large amount of
time.
* Perform a triggered read (internal trigger):
The device will get triggered by a hardware event that is out of
control from the driver. The driver is only aware of the IRQ firing
when the conversion is done. It then must push the samples to a set
of buffers and notify the IIO core.
* Perform a triggered read (external trigger):
This looks very much like a raw read from the driver point of view,
the difference being that, when the external trigger fires, the IIO
core will first call iio_pollfunc_store_time() as hard IRQ handler
and then calls a threaded handler that is supposed to configure the
channels, start the conversions, wait for it and again push the
samples when their are ready. These two handlers are registered by
devm_iio_triggered_buffer_setup().
There is an additional level of complexity as, my hardware does not
feature this EOC IRQ (it is not wired and thus cannot be used). I want
to be able to also support the absence of IRQ which is not necessary
for any operation but the internal trigger use.
How I ended-up implementing things in v2:
* Raw read:
Start the conversion.
Wait for the conversion to be done:
- With IRQ: use wait_for_completion(), the IRQ will fire, calling
iio_trigger_generic_data_rdy_poll() [1], calling complete() in this
situation.
- Without IRQ: busy wait loop.
[1] Maybe this is the thing that is bothering you, using the internal
IIO trigger interrupt tree logic to handle a regular EOC situation. In
all drivers that I had a chance to look at, it's always done like that
but perhaps it was bad luck and the more I look at it, the less I
understand its use. I will propose an alternative where the hard IRQ
handler really is the EOC handler and eventually calls a threaded
handler in the case of an internal triggering to push the samples.
I don't see the point of using iio_trigger_generic_data_rdy_poll()
anymore (maybe I'm wrong, tell me after reading v3?).
I ended-up writing this driver this way because I thought that I had to
provide a single IRQ handler and use
iio_trigger_generic_data_rdy_poll() but now I think I either
misinterpreted the comments or it was bad advise. I fell that the
driver is much simpler to understand in v3 where there is:
- one hard IRQ handler registered as the primary interrupt handler:
its purpose is to handle EOC conditions
- one threaded handler registered as the secondary interrupt handler:
it will only be triggered when using the internal trigger, its purpose
is to read and push the samples
- one threaded handler registered as the external trigger handler:
it will start the conversion, wait for EOC (either by busy waiting if
there is no IRQ or by waiting for completion otherwise - complete()
being called in the primary IRQ handler), read the data and push it
to the buffers.
If this implementation does not fit your requirements, I will really
need more advanced advises about what you require, what I should avoid
and perhaps what is wrong in the above explanation :)
Thanks,
Miquèl
On Wed, 15 Sep 2021 12:18:32 +0200
Miquel Raynal <[email protected]> wrote:
> Hi Jonathan, Nuno,
>
> [email protected] wrote on Sun, 5 Sep 2021 17:10:46 +0100:
>
> > On Thu, 2 Sep 2021 23:14:36 +0200
> > Miquel Raynal <[email protected]> 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.
That is the right thing to do. The split should be done a little differently
than you have it in v3. I've added comments to that patch.
Data ready triggers are always a little messy because we end up with a split that
is:
Trigger side - Interrupt comes in here...
--------- GENERIC IIO HANDLING ----- Take the trigger and routes it to the device code ---
Device side - We do the data reading here.
The reason for this is that we may well have other devices using the same trigger
and we want to keep the model looking the same for all devices.
A push into an iio buffer should always be on the device side of that boundary.
>
> > 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
>
On Wed, 15 Sep 2021 17:45:07 +0200
Miquel Raynal <[email protected]> wrote:
> Hi Nuno, Jonathan,
>
> [email protected] wrote on Mon, 6 Sep 2021 09:53:15 +0000:
>
> > > -----Original Message-----
> > > From: Jonathan Cameron <[email protected]>
> > > Sent: Sunday, September 5, 2021 6:11 PM
> > > To: Miquel Raynal <[email protected]>
> > > Cc: Lars-Peter Clausen <[email protected]>; [email protected];
> > > [email protected]; Thomas Petazzoni
> > > <[email protected]>; Sa, Nuno <[email protected]>
> > > 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 <[email protected]> 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 forgot to do this, I think you misunderstood the following patch:
> "iio: adc: max1027: Don't just sleep when the EOC interrupt is
> available"
>
> As I am having deep troubles reworking this once again, I will try to
> explain how this is build below and look for your feedback on it.
Unfortunately I'm not successfully managing to convey what I am trying
to get you to change this to...
>
> > >
> > > 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.
> > >
>
> [...]
>
> > 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?
>
> I renamed this handler with a generic name, because it actually serves
> several different purposes, see below.
>
> > 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á
>
I'm going to take a multi pronged approach to trying to get us on the same
page design wise. I've replied to v3 with code snippets that will hopefully
convey the explanation I'm giving here.
Data ready triggers always provide some blurring of the nice clean
trigger -> IIO CORE HANDLING -> device handling to grab data and push it out..
but we have a fairly standard model for this and whilst limited in functionality
I think the max1027 was keeping to this model.
> So here is how I understand the device:
> * During a raw read:
> The IRQ indicates an EOC.
> * During a triggered read (internal trigger):
> The driver has no knowledge of the trigger being fired, it only
> sees the IRQ firing at EOC. This means the internal trigger cannot be
> used without the IRQ.
>
> Now here is what I understand from the IIO subsystem and what I tried
> to implement:
> * Perform a raw read:
> The driver needs to setup the channels, start the operation, wait for
> the end of the conversion, return the value. This is all done in the
> ->raw_read() hook in process context. Raw reads can either use the
> EOC IRQ if wired or just wait for a sufficiently large amount of
> time.
> * Perform a triggered read (internal trigger):
> The device will get triggered by a hardware event that is out of
> control from the driver. The driver is only aware of the IRQ firing
> when the conversion is done. It then must push the samples to a set
> of buffers and notify the IIO core.
True, but there are two steps to this.
1) The trigger IRQ fires. That then calls a tree of other interrupts
(iio_trigger_poll()).
One of that tree of interrupts is attached by the driver.
That interrupt handler is then called to actually take the data indicated
by the EOC interrupt and push it to the buffer. Note this second interrupt
is registered as part of iio_triggered_buffer_setup() which is why that
function takes interrupt handlers.
> * Perform a triggered read (external trigger):
> This looks very much like a raw read from the driver point of view,
> the difference being that, when the external trigger fires, the IIO
> core will first call iio_pollfunc_store_time() as hard IRQ handler
> and then calls a threaded handler that is supposed to configure the
> channels, start the conversions, wait for it and again push the
> samples when their are ready. These two handlers are registered by
> devm_iio_triggered_buffer_setup().
That is true in your code, but it is not the correct way to do this because
in the event of a trigger other than the EOC one, we should not have the
interrupt to indicate the end of the read we are manually starting calling
into the handler for the trigger (as it wasn't the trigger!) - it should
be handled at the level above that.
If you have a different trigger than the EOC one provided by this driver then
the top level interrupt handler (the one that actually handles the EOC interrupt
should not call iio_trigger_generic_data_ready_poll() because we need to do
something different in that interrupt handler. It is this handler that
you should modify so that it does different things depending on whether or
not the EOC trigger is in use. (you can check this using iio_trigger_using_own(iio_dev))
I've tried to find a simple example of this, but they are all rather convoluted for
various device specific reasons (getting the best possible timestamp for example)
https://elixir.bootlin.com/linux/latest/source/drivers/iio/pressure/zpa2326.c#L786
more or less follows the model I'm describing with some rather fiddly timestamp
handling..
>
> There is an additional level of complexity as, my hardware does not
> feature this EOC IRQ (it is not wired and thus cannot be used). I want
> to be able to also support the absence of IRQ which is not necessary
> for any operation but the internal trigger use.
>
> How I ended-up implementing things in v2:
> * Raw read:
> Start the conversion.
> Wait for the conversion to be done:
> - With IRQ: use wait_for_completion(), the IRQ will fire, calling
> iio_trigger_generic_data_rdy_poll() [1], calling complete() in this
> situation.
> - Without IRQ: busy wait loop.
>
> [1] Maybe this is the thing that is bothering you, using the internal
> IIO trigger interrupt tree logic to handle a regular EOC situation.
Yes.
> In
> all drivers that I had a chance to look at, it's always done like that
> but perhaps it was bad luck and the more I look at it, the less I
> understand its use. I will propose an alternative where the hard IRQ
> handler really is the EOC handler and eventually calls a threaded
> handler in the case of an internal triggering to push the samples.
> I don't see the point of using iio_trigger_generic_data_rdy_poll()
> anymore (maybe I'm wrong, tell me after reading v3?).
>
> I ended-up writing this driver this way because I thought that I had to
> provide a single IRQ handler and use
> iio_trigger_generic_data_rdy_poll() but now I think I either
> misinterpreted the comments or it was bad advise. I fell that the
> driver is much simpler to understand in v3 where there is:
> - one hard IRQ handler registered as the primary interrupt handler:
> its purpose is to handle EOC conditions
> - one threaded handler registered as the secondary interrupt handler:
> it will only be triggered when using the internal trigger, its purpose
> is to read and push the samples
> - one threaded handler registered as the external trigger handler:
> it will start the conversion, wait for EOC (either by busy waiting if
> there is no IRQ or by waiting for completion otherwise - complete()
> being called in the primary IRQ handler), read the data and push it
> to the buffers.
This nearly works, but I think the trigger reference counting will be broken
because you call iio_trigger_notify_done() without calling iio_trigger_poll().
You could cheat and drop the notify_done() - which is the solution used when
we have fifos involved and hence have to do a still more complex dance but
that isn't the right solution here.
This is more complex than should be needed and doesn't fit the model IIO
uses to separate triggers and data capture.
A few 'rules' which get bent occasionally, particularly when a driver only
supports it's own trigger.
* iio_trigger_poll() only called in an interrupt handler that is handling
the interrupt which is the trigger in use. So in this case, only when
the driver is configured to use the EOC based trigger.
* iio_push_to_buffers_*(), iio_trigger_notify_done() only called from a
trigger handler which is registered with iio_triggered_buffer_setup))
To keep to this set of rules we need two paths in the trigger irq handler
and in the trigger handler (the one on the 'device' side of the internal
irq chip that IIO uses to 'broadcast' triggers).
I may well have messed up details in the following but hopefully I manage
to convey the basic approach I'm suggesting.
So replace that iio_trigger_generic_data_rdy_poll() with one that does
something along the lines of
irqreturn_t irq_handler(int irq, void *private)
{
struct iio_dev *indio_dev = private;
struct max1027_state *st = iio_priv(indio_dev);
if (!iio_buffer_enabled(indio_dev) ||
!iio_trigger_using_own(trigger) {
complete(st->completion);
return IRQ_HANDLED;
}
/* So we only use this as a trigger - if it is the trigger! */
return iio_trigger_generic_data_rdy_poll(st->trig, irq);
}
registered in the
dev_request_threaded_irq() call with the private parameter changed to indio_dev
as you have done in v3.
and
irqreturn_t trigger_handler(void *private, int irq)
{
struct iio_poll_func *pf = private;
struct io_dev *indio_dev = pf->indio_dev;
if (!iio_trigger_using_own(indio_dev)) {
ret = max1027_configure_chans_and_start(indio_dev);
if (ret)
return IRQ_HANDLED;
ret = max1027_wait_eoc(indio_dev);
if (ret)
return IRQ_HANDLED;
ret = max1027_read_scan(indio_dev);
if (ret)
return IRQ_HANDLED;
iio_trigger_notify_done(indio_dev);
return IRQ_HANDLED;
} else {
ret = max1027_read_scan(indio_dev);
if (ret)
dev_err(&indio_dev->dev,
"Cannot read scanned values (%d)\n", ret);
iio_trigger_notify_done(indio_dev->trig);
return IRQ_HANDLED;
}
/* Feel free to rearrange as you like! */
}
>
> If this implementation does not fit your requirements, I will really
> need more advanced advises about what you require, what I should avoid
> and perhaps what is wrong in the above explanation :)
>
> Thanks,
> Miquèl
Hi Jonathan,
[email protected] wrote on Sat, 18 Sep 2021 18:13:08 +0100:
> On Wed, 15 Sep 2021 12:18:32 +0200
> Miquel Raynal <[email protected]> wrote:
>
> > Hi Jonathan, Nuno,
> >
> > [email protected] wrote on Sun, 5 Sep 2021 17:10:46 +0100:
> >
> > > On Thu, 2 Sep 2021 23:14:36 +0200
> > > Miquel Raynal <[email protected]> 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.
> That is the right thing to do. The split should be done a little differently
> than you have it in v3. I've added comments to that patch.
>
> Data ready triggers are always a little messy because we end up with a split that
> is:
>
> Trigger side - Interrupt comes in here...
>
> --------- GENERIC IIO HANDLING ----- Take the trigger and routes it to the device code ---
>
> Device side - We do the data reading here.
>
> The reason for this is that we may well have other devices using the same trigger
> and we want to keep the model looking the same for all devices.
>
> A push into an iio buffer should always be on the device side of that boundary.
This is much clearer, I think I have got the main idea.
However I have a question that is specific to the current situation. In
the case of this particular device, I don't really understand how
another device could use the same trigger than the hardware one,
because we have no indication of the trigger being latched. When we get
the information the data is already in the FIFO, this means we get the
information much later than when the hardware transitioned to indicate
a conversion request. Is it that in your model, we should be able to
use the EOC IRQ handler to trigger another IIO device, even though
this implies an additional delay?
Thanks,
Miquèl
> > > 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
> >
Hi Jonathan,
[email protected] wrote on Sat, 18 Sep 2021 18:39:20 +0100:
> On Wed, 15 Sep 2021 17:45:07 +0200
> Miquel Raynal <[email protected]> wrote:
>
> > Hi Nuno, Jonathan,
> >
> > [email protected] wrote on Mon, 6 Sep 2021 09:53:15 +0000:
> >
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <[email protected]>
> > > > Sent: Sunday, September 5, 2021 6:11 PM
> > > > To: Miquel Raynal <[email protected]>
> > > > Cc: Lars-Peter Clausen <[email protected]>; [email protected];
> > > > [email protected]; Thomas Petazzoni
> > > > <[email protected]>; Sa, Nuno <[email protected]>
> > > > 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 <[email protected]> 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 forgot to do this, I think you misunderstood the following patch:
> > "iio: adc: max1027: Don't just sleep when the EOC interrupt is
> > available"
> >
> > As I am having deep troubles reworking this once again, I will try to
> > explain how this is build below and look for your feedback on it.
>
> Unfortunately I'm not successfully managing to convey what I am trying
> to get you to change this to...
This is certainly more related to my lack of 'IIO culture', I think all
this makes much more sense now that (I think) I understand the
theoretical model and I need to thank you a lot for your patience and
all the clarifications you brought until now.
> > > > 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.
> > > >
> >
> > [...]
> >
> > > 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?
> >
> > I renamed this handler with a generic name, because it actually serves
> > several different purposes, see below.
> >
> > > 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á
> >
> I'm going to take a multi pronged approach to trying to get us on the same
> page design wise. I've replied to v3 with code snippets that will hopefully
> convey the explanation I'm giving here.
>
> Data ready triggers always provide some blurring of the nice clean
> trigger -> IIO CORE HANDLING -> device handling to grab data and push it out..
> but we have a fairly standard model for this and whilst limited in functionality
> I think the max1027 was keeping to this model.
>
> > So here is how I understand the device:
> > * During a raw read:
> > The IRQ indicates an EOC.
> > * During a triggered read (internal trigger):
> > The driver has no knowledge of the trigger being fired, it only
> > sees the IRQ firing at EOC. This means the internal trigger cannot be
> > used without the IRQ.
> >
> > Now here is what I understand from the IIO subsystem and what I tried
> > to implement:
> > * Perform a raw read:
> > The driver needs to setup the channels, start the operation, wait for
> > the end of the conversion, return the value. This is all done in the
> > ->raw_read() hook in process context. Raw reads can either use the
> > EOC IRQ if wired or just wait for a sufficiently large amount of
> > time.
> > * Perform a triggered read (internal trigger):
> > The device will get triggered by a hardware event that is out of
> > control from the driver. The driver is only aware of the IRQ firing
> > when the conversion is done. It then must push the samples to a set
> > of buffers and notify the IIO core.
>
> True, but there are two steps to this.
> 1) The trigger IRQ fires. That then calls a tree of other interrupts
> (iio_trigger_poll()).
> One of that tree of interrupts is attached by the driver.
> That interrupt handler is then called to actually take the data indicated
> by the EOC interrupt and push it to the buffer. Note this second interrupt
> is registered as part of iio_triggered_buffer_setup() which is why that
> function takes interrupt handlers.
>
> > * Perform a triggered read (external trigger):
> > This looks very much like a raw read from the driver point of view,
> > the difference being that, when the external trigger fires, the IIO
> > core will first call iio_pollfunc_store_time() as hard IRQ handler
> > and then calls a threaded handler that is supposed to configure the
> > channels, start the conversions, wait for it and again push the
> > samples when their are ready. These two handlers are registered by
> > devm_iio_triggered_buffer_setup().
>
>
> That is true in your code, but it is not the correct way to do this because
> in the event of a trigger other than the EOC one, we should not have the
> interrupt to indicate the end of the read we are manually starting calling
> into the handler for the trigger (as it wasn't the trigger!) - it should
> be handled at the level above that.
>
> If you have a different trigger than the EOC one provided by this driver then
> the top level interrupt handler (the one that actually handles the EOC interrupt
> should not call iio_trigger_generic_data_ready_poll() because we need to do
> something different in that interrupt handler. It is this handler that
> you should modify so that it does different things depending on whether or
> not the EOC trigger is in use. (you can check this using iio_trigger_using_own(iio_dev))
>
> I've tried to find a simple example of this, but they are all rather convoluted for
> various device specific reasons (getting the best possible timestamp for example)
>
> https://elixir.bootlin.com/linux/latest/source/drivers/iio/pressure/zpa2326.c#L786
> more or less follows the model I'm describing with some rather fiddly timestamp
> handling..
>
> >
> > There is an additional level of complexity as, my hardware does not
> > feature this EOC IRQ (it is not wired and thus cannot be used). I want
> > to be able to also support the absence of IRQ which is not necessary
> > for any operation but the internal trigger use.
> >
> > How I ended-up implementing things in v2:
> > * Raw read:
> > Start the conversion.
> > Wait for the conversion to be done:
> > - With IRQ: use wait_for_completion(), the IRQ will fire, calling
> > iio_trigger_generic_data_rdy_poll() [1], calling complete() in this
> > situation.
> > - Without IRQ: busy wait loop.
> >
> > [1] Maybe this is the thing that is bothering you, using the internal
> > IIO trigger interrupt tree logic to handle a regular EOC situation.
>
> Yes.
>
> > In
> > all drivers that I had a chance to look at, it's always done like that
> > but perhaps it was bad luck and the more I look at it, the less I
> > understand its use. I will propose an alternative where the hard IRQ
> > handler really is the EOC handler and eventually calls a threaded
> > handler in the case of an internal triggering to push the samples.
> > I don't see the point of using iio_trigger_generic_data_rdy_poll()
> > anymore (maybe I'm wrong, tell me after reading v3?).
>
>
>
> >
> > I ended-up writing this driver this way because I thought that I had to
> > provide a single IRQ handler and use
> > iio_trigger_generic_data_rdy_poll() but now I think I either
> > misinterpreted the comments or it was bad advise. I fell that the
> > driver is much simpler to understand in v3 where there is:
> > - one hard IRQ handler registered as the primary interrupt handler:
> > its purpose is to handle EOC conditions
> > - one threaded handler registered as the secondary interrupt handler:
> > it will only be triggered when using the internal trigger, its purpose
> > is to read and push the samples
> > - one threaded handler registered as the external trigger handler:
> > it will start the conversion, wait for EOC (either by busy waiting if
> > there is no IRQ or by waiting for completion otherwise - complete()
> > being called in the primary IRQ handler), read the data and push it
> > to the buffers.
> This nearly works, but I think the trigger reference counting will be broken
> because you call iio_trigger_notify_done() without calling iio_trigger_poll().
> You could cheat and drop the notify_done() - which is the solution used when
> we have fifos involved and hence have to do a still more complex dance but
> that isn't the right solution here.
>
> This is more complex than should be needed and doesn't fit the model IIO
> uses to separate triggers and data capture.
>
> A few 'rules' which get bent occasionally, particularly when a driver only
> supports it's own trigger.
> * iio_trigger_poll() only called in an interrupt handler that is handling
> the interrupt which is the trigger in use. So in this case, only when
> the driver is configured to use the EOC based trigger.
> * iio_push_to_buffers_*(), iio_trigger_notify_done() only called from a
> trigger handler which is registered with iio_triggered_buffer_setup))
>
> To keep to this set of rules we need two paths in the trigger irq handler
> and in the trigger handler (the one on the 'device' side of the internal
> irq chip that IIO uses to 'broadcast' triggers).
>
> I may well have messed up details in the following but hopefully I manage
> to convey the basic approach I'm suggesting.
With this and your previous answer, I think I well understand now what
you meant in your first reviews. Thank you so much for all the
guidance, I'll try to make v4 fit.
> So replace that iio_trigger_generic_data_rdy_poll() with one that does
> something along the lines of
>
> irqreturn_t irq_handler(int irq, void *private)
> {
> struct iio_dev *indio_dev = private;
> struct max1027_state *st = iio_priv(indio_dev);
>
> if (!iio_buffer_enabled(indio_dev) ||
> !iio_trigger_using_own(trigger) {
> complete(st->completion);
> return IRQ_HANDLED;
> }
> /* So we only use this as a trigger - if it is the trigger! */
> return iio_trigger_generic_data_rdy_poll(st->trig, irq);
Understood.
> }
>
> registered in the
> dev_request_threaded_irq() call with the private parameter changed to indio_dev
> as you have done in v3.
>
> and
>
> irqreturn_t trigger_handler(void *private, int irq)
> {
> struct iio_poll_func *pf = private;
> struct io_dev *indio_dev = pf->indio_dev;
>
> if (!iio_trigger_using_own(indio_dev)) {
> ret = max1027_configure_chans_and_start(indio_dev);
> if (ret)
> return IRQ_HANDLED;
>
> ret = max1027_wait_eoc(indio_dev);
> if (ret)
> return IRQ_HANDLED;
>
> ret = max1027_read_scan(indio_dev);
> if (ret)
> return IRQ_HANDLED;
> iio_trigger_notify_done(indio_dev);
> return IRQ_HANDLED;
> } else {
> ret = max1027_read_scan(indio_dev);
> if (ret)
> dev_err(&indio_dev->dev,
> "Cannot read scanned values (%d)\n", ret);
>
> iio_trigger_notify_done(indio_dev->trig);
>
> return IRQ_HANDLED;
> }
> /* Feel free to rearrange as you like! */
>
> }
>
> >
> > If this implementation does not fit your requirements, I will really
> > need more advanced advises about what you require, what I should avoid
> > and perhaps what is wrong in the above explanation :)
> >
> > Thanks,
> > Miquèl
>
Thanks,
Miquèl
On Mon, 20 Sep 2021 10:37:39 +0200
Miquel Raynal <[email protected]> wrote:
> Hi Jonathan,
>
> [email protected] wrote on Sat, 18 Sep 2021 18:13:08 +0100:
>
> > On Wed, 15 Sep 2021 12:18:32 +0200
> > Miquel Raynal <[email protected]> wrote:
> >
> > > Hi Jonathan, Nuno,
> > >
> > > [email protected] wrote on Sun, 5 Sep 2021 17:10:46 +0100:
> > >
> > > > On Thu, 2 Sep 2021 23:14:36 +0200
> > > > Miquel Raynal <[email protected]> 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.
> > That is the right thing to do. The split should be done a little differently
> > than you have it in v3. I've added comments to that patch.
> >
> > Data ready triggers are always a little messy because we end up with a split that
> > is:
> >
> > Trigger side - Interrupt comes in here...
> >
> > --------- GENERIC IIO HANDLING ----- Take the trigger and routes it to the device code ---
> >
> > Device side - We do the data reading here.
> >
> > The reason for this is that we may well have other devices using the same trigger
> > and we want to keep the model looking the same for all devices.
> >
> > A push into an iio buffer should always be on the device side of that boundary.
>
> This is much clearer, I think I have got the main idea.
>
> However I have a question that is specific to the current situation. In
> the case of this particular device, I don't really understand how
> another device could use the same trigger than the hardware one,
> because we have no indication of the trigger being latched. When we get
> the information the data is already in the FIFO, this means we get the
> information much later than when the hardware transitioned to indicate
> a conversion request. Is it that in your model, we should be able to
> use the EOC IRQ handler to trigger another IIO device, even though
> this implies an additional delay?
It's not ideal, but sometimes it is better to have somewhat consistent
'synchronization' between multiple devices. You are correct that anything
else using a data ready trigger will be a bit late - but the frequencies
will at least be matched. Not great but the best possible under these
circumstances.
If it's possible to use a truely shared hardware trigger that is obviously
better than you can do here.
Jonathan
>
> Thanks,
> Miquèl
>
> > > > 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
> > >
Hi Jonathan,
[email protected] wrote on Mon, 20 Sep 2021 18:43:38 +0100:
> On Mon, 20 Sep 2021 10:37:39 +0200
> Miquel Raynal <[email protected]> wrote:
>
> > Hi Jonathan,
> >
> > [email protected] wrote on Sat, 18 Sep 2021 18:13:08 +0100:
> >
> > > On Wed, 15 Sep 2021 12:18:32 +0200
> > > Miquel Raynal <[email protected]> wrote:
> > >
> > > > Hi Jonathan, Nuno,
> > > >
> > > > [email protected] wrote on Sun, 5 Sep 2021 17:10:46 +0100:
> > > >
> > > > > On Thu, 2 Sep 2021 23:14:36 +0200
> > > > > Miquel Raynal <[email protected]> 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.
> > > That is the right thing to do. The split should be done a little differently
> > > than you have it in v3. I've added comments to that patch.
> > >
> > > Data ready triggers are always a little messy because we end up with a split that
> > > is:
> > >
> > > Trigger side - Interrupt comes in here...
> > >
> > > --------- GENERIC IIO HANDLING ----- Take the trigger and routes it to the device code ---
> > >
> > > Device side - We do the data reading here.
> > >
> > > The reason for this is that we may well have other devices using the same trigger
> > > and we want to keep the model looking the same for all devices.
> > >
> > > A push into an iio buffer should always be on the device side of that boundary.
> >
> > This is much clearer, I think I have got the main idea.
> >
> > However I have a question that is specific to the current situation. In
> > the case of this particular device, I don't really understand how
> > another device could use the same trigger than the hardware one,
> > because we have no indication of the trigger being latched. When we get
> > the information the data is already in the FIFO, this means we get the
> > information much later than when the hardware transitioned to indicate
> > a conversion request. Is it that in your model, we should be able to
> > use the EOC IRQ handler to trigger another IIO device, even though
> > this implies an additional delay?
>
> It's not ideal, but sometimes it is better to have somewhat consistent
> 'synchronization' between multiple devices. You are correct that anything
> else using a data ready trigger will be a bit late - but the frequencies
> will at least be matched. Not great but the best possible under these
> circumstances.
>
> If it's possible to use a truely shared hardware trigger that is obviously
> better than you can do here.
Ok. This was definitely a part of the puzzle that I missed in the first
place, making harder the understanding (and the interest) of the driver
vs. core split.
Cheers,
Miquèl