[PATCH v2 00/16] Bring external triggers support to MAX1027-like ADCs

[PATCH v2 00/16] Bring external triggers support to MAX1027-like ADCs

[Miquel Raynal]

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

[PATCH v2 01/16] iio: adc: max1027: Fix style

[Miquel Raynal]

Follow checkpatch.pl's main advices before hacking into the driver, mainly:
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
WARNING: Prefer 'unsigned int *' to bare use of 'unsigned *'
CHECK: Comparison to NULL could be written "!foo"
CHECK: Alignment should match open parenthesis

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index b753658bb41e..5c4633a54b30 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -327,8 +327,8 @@ static int max1027_read_raw(struct iio_dev *indio_dev,
 }
 
 static int max1027_debugfs_reg_access(struct iio_dev *indio_dev,
-				      unsigned reg, unsigned writeval,
-				      unsigned *readval)
+				      unsigned int reg, unsigned int writeval,
+				      unsigned int *readval)
 {
 	struct max1027_state *st = iio_priv(indio_dev);
 	u8 *val = (u8 *)st->buffer;
@@ -424,7 +424,7 @@ static int max1027_probe(struct spi_device *spi)
 	pr_debug("%s: probe(spi = 0x%p)\n", __func__, spi);
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
-	if (indio_dev == NULL) {
+	if (!indio_dev) {
 		pr_err("Can't allocate iio device\n");
 		return -ENOMEM;
 	}
@@ -443,9 +443,9 @@ static int max1027_probe(struct spi_device *spi)
 	indio_dev->available_scan_masks = st->info->available_scan_masks;
 
 	st->buffer = devm_kmalloc_array(&indio_dev->dev,
-				  indio_dev->num_channels, 2,
-				  GFP_KERNEL);
-	if (st->buffer == NULL) {
+					indio_dev->num_channels, 2,
+					GFP_KERNEL);
+	if (!st->buffer) {
 		dev_err(&indio_dev->dev, "Can't allocate buffer\n");
 		return -ENOMEM;
 	}
@@ -462,7 +462,7 @@ static int max1027_probe(struct spi_device *spi)
 
 		st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-trigger",
 						  indio_dev->name);
-		if (st->trig == NULL) {
+		if (!st->trig) {
 			ret = -ENOMEM;
 			dev_err(&indio_dev->dev,
 				"Failed to allocate iio trigger\n");
-- 
2.27.0

[PATCH v2 02/16] iio: adc: max1027: Drop extra warning message

[Miquel Raynal]

Memory allocation errors automatically trigger the right logs, no need
to have our own.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/max1027.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 5c4633a54b30..3994d3566f05 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -445,10 +445,8 @@ static int max1027_probe(struct spi_device *spi)
 	st->buffer = devm_kmalloc_array(&indio_dev->dev,
 					indio_dev->num_channels, 2,
 					GFP_KERNEL);
-	if (!st->buffer) {
-		dev_err(&indio_dev->dev, "Can't allocate buffer\n");
+	if (!st->buffer)
 		return -ENOMEM;
-	}
 
 	if (spi->irq) {
 		ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
-- 
2.27.0

[PATCH v2 03/16] iio: adc: max1027: Drop useless debug messages

[Miquel Raynal]

These two debug messages bring absolutely no value, let's drop them.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 3994d3566f05..f27044324141 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -392,8 +392,6 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct max1027_state *st = iio_priv(indio_dev);
 
-	pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private);
-
 	/* fill buffer with all channel */
 	spi_read(st->spi, st->buffer, indio_dev->masklength * 2);
 
@@ -421,8 +419,6 @@ static int max1027_probe(struct spi_device *spi)
 	struct iio_dev *indio_dev;
 	struct max1027_state *st;
 
-	pr_debug("%s: probe(spi = 0x%p)\n", __func__, spi);
-
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev) {
 		pr_err("Can't allocate iio device\n");
-- 
2.27.0

[PATCH v2 04/16] iio: adc: max1027: Avoid device managed allocators for DMA purposes

[Miquel Raynal]

My overall understanding [1] is that devm_ allocations will require an
extra 16 bytes at the beginning of the allocated buffer to store
information about the managing device, this shifts a little bit the
buffer which may then not be fully aligned on cache lines, disqualifying
them for being suitable for DMA.

Let's switch to a regular kmalloc based allocator to ensure st->buffer
is DMA-able, as required by the IIO subsystem.

[1] https://linux-arm-kernel.infradead.narkive.com/vyJqy0RQ/question-devm-kmalloc-for-dma

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index f27044324141..1167d50c1d67 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -438,9 +438,7 @@ static int max1027_probe(struct spi_device *spi)
 	indio_dev->num_channels = st->info->num_channels;
 	indio_dev->available_scan_masks = st->info->available_scan_masks;
 
-	st->buffer = devm_kmalloc_array(&indio_dev->dev,
-					indio_dev->num_channels, 2,
-					GFP_KERNEL);
+	st->buffer = kmalloc_array(indio_dev->num_channels, 2, GFP_KERNEL);
 	if (!st->buffer)
 		return -ENOMEM;
 
@@ -451,7 +449,7 @@ static int max1027_probe(struct spi_device *spi)
 						      NULL);
 		if (ret < 0) {
 			dev_err(&indio_dev->dev, "Failed to setup buffer\n");
-			return ret;
+			goto free_buffer;
 		}
 
 		st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-trigger",
@@ -460,7 +458,7 @@ static int max1027_probe(struct spi_device *spi)
 			ret = -ENOMEM;
 			dev_err(&indio_dev->dev,
 				"Failed to allocate iio trigger\n");
-			return ret;
+			goto free_buffer;
 		}
 
 		st->trig->ops = &max1027_trigger_ops;
@@ -470,7 +468,7 @@ static int max1027_probe(struct spi_device *spi)
 		if (ret < 0) {
 			dev_err(&indio_dev->dev,
 				"Failed to register iio trigger\n");
-			return ret;
+			goto free_buffer;
 		}
 
 		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
@@ -481,7 +479,7 @@ static int max1027_probe(struct spi_device *spi)
 						st->trig);
 		if (ret < 0) {
 			dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n");
-			return ret;
+			goto free_buffer;
 		}
 	}
 
@@ -490,7 +488,7 @@ static int max1027_probe(struct spi_device *spi)
 	ret = spi_write(st->spi, &st->reg, 1);
 	if (ret < 0) {
 		dev_err(&indio_dev->dev, "Failed to reset the ADC\n");
-		return ret;
+		goto free_buffer;
 	}
 
 	/* Disable averaging */
@@ -498,10 +496,15 @@ static int max1027_probe(struct spi_device *spi)
 	ret = spi_write(st->spi, &st->reg, 1);
 	if (ret < 0) {
 		dev_err(&indio_dev->dev, "Failed to configure averaging register\n");
-		return ret;
+		goto free_buffer;
 	}
 
 	return devm_iio_device_register(&spi->dev, indio_dev);
+
+free_buffer:
+	kfree(st->buffer);
+
+	return ret;
 }
 
 static struct spi_driver max1027_driver = {
-- 
2.27.0

Re: [PATCH v2 04/16] iio: adc: max1027: Avoid device managed allocators for DMA purposes

[Jonathan Cameron]

On Thu,  2 Sep 2021 23:14:25 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> My overall understanding [1] is that devm_ allocations will require an
> extra 16 bytes at the beginning of the allocated buffer to store
> information about the managing device, this shifts a little bit the
> buffer which may then not be fully aligned on cache lines, disqualifying
> them for being suitable for DMA.
> 
> Let's switch to a regular kmalloc based allocator to ensure st->buffer
> is DMA-able, as required by the IIO subsystem.
> 
> [1] https://linux-arm-kernel.infradead.narkive.com/vyJqy0RQ/question-devm-kmalloc-for-dma
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
As per the other thread (I was late to the discussion) I don't believe this is
necessary because there is no chance of the data in the cacheline being touched
by the CPU whilst DMA is going on.

https://linux-arm-kernel.infradead.narkive.com/vyJqy0RQ/question-devm-kmalloc-for-dma#post5

Writes before dma transfer are safe.  It's writes during it that can cause problem so as
long as a driver isn't doing devm_ calls during such writes (and I really hope this isn't)
then we are safe.  The device will only overwrite the data with what was already there.

Jonathan

> ---
>  drivers/iio/adc/max1027.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index f27044324141..1167d50c1d67 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -438,9 +438,7 @@ static int max1027_probe(struct spi_device *spi)
>  	indio_dev->num_channels = st->info->num_channels;
>  	indio_dev->available_scan_masks = st->info->available_scan_masks;
>  
> -	st->buffer = devm_kmalloc_array(&indio_dev->dev,
> -					indio_dev->num_channels, 2,
> -					GFP_KERNEL);
> +	st->buffer = kmalloc_array(indio_dev->num_channels, 2, GFP_KERNEL);
>  	if (!st->buffer)
>  		return -ENOMEM;
>  
> @@ -451,7 +449,7 @@ static int max1027_probe(struct spi_device *spi)
>  						      NULL);
>  		if (ret < 0) {
>  			dev_err(&indio_dev->dev, "Failed to setup buffer\n");
> -			return ret;
> +			goto free_buffer;
>  		}
>  
>  		st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-trigger",
> @@ -460,7 +458,7 @@ static int max1027_probe(struct spi_device *spi)
>  			ret = -ENOMEM;
>  			dev_err(&indio_dev->dev,
>  				"Failed to allocate iio trigger\n");
> -			return ret;
> +			goto free_buffer;
>  		}
>  
>  		st->trig->ops = &max1027_trigger_ops;
> @@ -470,7 +468,7 @@ static int max1027_probe(struct spi_device *spi)
>  		if (ret < 0) {
>  			dev_err(&indio_dev->dev,
>  				"Failed to register iio trigger\n");
> -			return ret;
> +			goto free_buffer;
>  		}
>  
>  		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
> @@ -481,7 +479,7 @@ static int max1027_probe(struct spi_device *spi)
>  						st->trig);
>  		if (ret < 0) {
>  			dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n");
> -			return ret;
> +			goto free_buffer;
>  		}
>  	}
>  
> @@ -490,7 +488,7 @@ static int max1027_probe(struct spi_device *spi)
>  	ret = spi_write(st->spi, &st->reg, 1);
>  	if (ret < 0) {
>  		dev_err(&indio_dev->dev, "Failed to reset the ADC\n");
> -		return ret;
> +		goto free_buffer;
>  	}
>  
>  	/* Disable averaging */
> @@ -498,10 +496,15 @@ static int max1027_probe(struct spi_device *spi)
>  	ret = spi_write(st->spi, &st->reg, 1);
>  	if (ret < 0) {
>  		dev_err(&indio_dev->dev, "Failed to configure averaging register\n");
> -		return ret;
> +		goto free_buffer;
>  	}
>  
>  	return devm_iio_device_register(&spi->dev, indio_dev);
> +
> +free_buffer:
> +	kfree(st->buffer);
> +
> +	return ret;
>  }
>  
>  static struct spi_driver max1027_driver = {

[PATCH v2 05/16] iio: adc: max1027: Minimize the number of converted channels

[Miquel Raynal]

Provide a list of ->available_scan_masks which match the device's
capabilities. Basically, the devices is able to scan from 0 to N, N
being the highest voltage channel requested by the user. The temperature
can be included or not, but cannot be retrieved alone.

The consequence is, instead of reading and pushing to the IIO buffers
all channels each time, the "minimum" number of channels will be scanned
and pushed based on the ->active_scan_mask.

For example, if the user wants channels 1, 4 and 5, all channels from
0 to 5 will be scanned and pushed to the IIO buffers. The core will then
filter out the unneeded samples based on the ->active_scan_mask that has
been selected and only channels 1, 4 and 5 will be available to the user
in the shared buffer.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 46 +++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 1167d50c1d67..3c41d2c0ed2a 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -172,18 +172,39 @@ static const struct iio_chan_spec max1231_channels[] = {
 	MAX1X31_CHANNELS(12),
 };
 
+#define MAX1X27_SCAN_MASK_TEMP BIT(0)
+
+#define MAX1X27_SCAN_MASKS(temp)					\
+	GENMASK(1, 1 - (temp)), GENMASK(2, 1 - (temp)),			\
+	GENMASK(3, 1 - (temp)), GENMASK(4, 1 - (temp)),			\
+	GENMASK(5, 1 - (temp)), GENMASK(6, 1 - (temp)),			\
+	GENMASK(7, 1 - (temp)), GENMASK(8, 1 - (temp))
+
+#define MAX1X29_SCAN_MASKS(temp)					\
+	MAX1X27_SCAN_MASKS(temp),					\
+	GENMASK(9, 1 - (temp)), GENMASK(10, 1 - (temp)),		\
+	GENMASK(11, 1 - (temp)), GENMASK(12, 1 - (temp))
+
+#define MAX1X31_SCAN_MASKS(temp)					\
+	MAX1X29_SCAN_MASKS(temp),					\
+	GENMASK(13, 1 - (temp)), GENMASK(14, 1 - (temp)),		\
+	GENMASK(15, 1 - (temp)), GENMASK(16, 1 - (temp))
+
 static const unsigned long max1027_available_scan_masks[] = {
-	0x000001ff,
+	MAX1X27_SCAN_MASKS(0),
+	MAX1X27_SCAN_MASKS(1),
 	0x00000000,
 };
 
 static const unsigned long max1029_available_scan_masks[] = {
-	0x00001fff,
+	MAX1X29_SCAN_MASKS(0),
+	MAX1X29_SCAN_MASKS(1),
 	0x00000000,
 };
 
 static const unsigned long max1031_available_scan_masks[] = {
-	0x0001ffff,
+	MAX1X31_SCAN_MASKS(0),
+	MAX1X31_SCAN_MASKS(1),
 	0x00000000,
 };
 
@@ -368,9 +389,15 @@ static int max1027_set_trigger_state(struct iio_trigger *trig, bool state)
 		if (ret < 0)
 			return ret;
 
-		/* Scan from 0 to max */
-		st->reg = MAX1027_CONV_REG | MAX1027_CHAN(0) |
-			  MAX1027_SCAN_N_M | MAX1027_TEMP;
+		/*
+		 * Scan from chan 0 to the highest requested channel.
+		 * Include temperature on demand.
+		 */
+		st->reg = MAX1027_CONV_REG | MAX1027_SCAN_0_N;
+		st->reg |= MAX1027_CHAN(fls(*indio_dev->active_scan_mask) - 2);
+		if (*indio_dev->active_scan_mask & MAX1X27_SCAN_MASK_TEMP)
+			st->reg |= MAX1027_TEMP;
+
 		ret = spi_write(st->spi, &st->reg, 1);
 		if (ret < 0)
 			return ret;
@@ -391,9 +418,14 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private)
 	struct iio_poll_func *pf = private;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct max1027_state *st = iio_priv(indio_dev);
+	unsigned int scanned_chans;
+
+	scanned_chans = fls(*indio_dev->active_scan_mask) - 1;
+	if (*indio_dev->active_scan_mask & MAX1X27_SCAN_MASK_TEMP)
+		scanned_chans++;
 
 	/* fill buffer with all channel */
-	spi_read(st->spi, st->buffer, indio_dev->masklength * 2);
+	spi_read(st->spi, st->buffer, scanned_chans * 2);
 
 	iio_push_to_buffers(indio_dev, st->buffer);
 
-- 
2.27.0

Re: [PATCH v2 05/16] iio: adc: max1027: Minimize the number of converted channels

[Jonathan Cameron]

On Thu,  2 Sep 2021 23:14:26 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Provide a list of ->available_scan_masks which match the device's
> capabilities. Basically, the devices is able to scan from 0 to N, N
> being the highest voltage channel requested by the user. The temperature
> can be included or not, but cannot be retrieved alone.
> 
> The consequence is, instead of reading and pushing to the IIO buffers
> all channels each time, the "minimum" number of channels will be scanned
> and pushed based on the ->active_scan_mask.
> 
> For example, if the user wants channels 1, 4 and 5, all channels from
> 0 to 5 will be scanned and pushed to the IIO buffers. The core will then
> filter out the unneeded samples based on the ->active_scan_mask that has
> been selected and only channels 1, 4 and 5 will be available to the user
> in the shared buffer.

This explanation is excellent.  If you are respinning it would be great
to have the essence of it as a comment alongside the code.
The bit about temp in particular was something I'd missed.

Nice optimization in general.

Jonathan
 
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/iio/adc/max1027.c | 46 +++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 1167d50c1d67..3c41d2c0ed2a 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -172,18 +172,39 @@ static const struct iio_chan_spec max1231_channels[] = {
>  	MAX1X31_CHANNELS(12),
>  };
>  
> +#define MAX1X27_SCAN_MASK_TEMP BIT(0)
> +
> +#define MAX1X27_SCAN_MASKS(temp)					\
> +	GENMASK(1, 1 - (temp)), GENMASK(2, 1 - (temp)),			\
> +	GENMASK(3, 1 - (temp)), GENMASK(4, 1 - (temp)),			\
> +	GENMASK(5, 1 - (temp)), GENMASK(6, 1 - (temp)),			\
> +	GENMASK(7, 1 - (temp)), GENMASK(8, 1 - (temp))
> +
> +#define MAX1X29_SCAN_MASKS(temp)					\
> +	MAX1X27_SCAN_MASKS(temp),					\
> +	GENMASK(9, 1 - (temp)), GENMASK(10, 1 - (temp)),		\
> +	GENMASK(11, 1 - (temp)), GENMASK(12, 1 - (temp))
> +
> +#define MAX1X31_SCAN_MASKS(temp)					\
> +	MAX1X29_SCAN_MASKS(temp),					\
> +	GENMASK(13, 1 - (temp)), GENMASK(14, 1 - (temp)),		\
> +	GENMASK(15, 1 - (temp)), GENMASK(16, 1 - (temp))
> +
>  static const unsigned long max1027_available_scan_masks[] = {
> -	0x000001ff,
> +	MAX1X27_SCAN_MASKS(0),
> +	MAX1X27_SCAN_MASKS(1),
>  	0x00000000,
>  };
>  
>  static const unsigned long max1029_available_scan_masks[] = {
> -	0x00001fff,
> +	MAX1X29_SCAN_MASKS(0),
> +	MAX1X29_SCAN_MASKS(1),
>  	0x00000000,
>  };
>  
>  static const unsigned long max1031_available_scan_masks[] = {
> -	0x0001ffff,
> +	MAX1X31_SCAN_MASKS(0),
> +	MAX1X31_SCAN_MASKS(1),
>  	0x00000000,
>  };
>  
> @@ -368,9 +389,15 @@ static int max1027_set_trigger_state(struct iio_trigger *trig, bool state)
>  		if (ret < 0)
>  			return ret;
>  
> -		/* Scan from 0 to max */
> -		st->reg = MAX1027_CONV_REG | MAX1027_CHAN(0) |
> -			  MAX1027_SCAN_N_M | MAX1027_TEMP;
> +		/*
> +		 * Scan from chan 0 to the highest requested channel.
> +		 * Include temperature on demand.
> +		 */
> +		st->reg = MAX1027_CONV_REG | MAX1027_SCAN_0_N;
> +		st->reg |= MAX1027_CHAN(fls(*indio_dev->active_scan_mask) - 2);
> +		if (*indio_dev->active_scan_mask & MAX1X27_SCAN_MASK_TEMP)
> +			st->reg |= MAX1027_TEMP;
> +
>  		ret = spi_write(st->spi, &st->reg, 1);
>  		if (ret < 0)
>  			return ret;
> @@ -391,9 +418,14 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private)
>  	struct iio_poll_func *pf = private;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct max1027_state *st = iio_priv(indio_dev);
> +	unsigned int scanned_chans;
> +
> +	scanned_chans = fls(*indio_dev->active_scan_mask) - 1;
> +	if (*indio_dev->active_scan_mask & MAX1X27_SCAN_MASK_TEMP)
> +		scanned_chans++;
>  
>  	/* fill buffer with all channel */
> -	spi_read(st->spi, st->buffer, indio_dev->masklength * 2);
> +	spi_read(st->spi, st->buffer, scanned_chans * 2);
>  
>  	iio_push_to_buffers(indio_dev, st->buffer);
>  

Re: [PATCH v2 05/16] iio: adc: max1027: Minimize the number of converted channels

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sun, 5 Sep 2021 16:34:06 +0100:

> On Thu,  2 Sep 2021 23:14:26 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Provide a list of ->available_scan_masks which match the device's
> > capabilities. Basically, the devices is able to scan from 0 to N, N
> > being the highest voltage channel requested by the user. The temperature
> > can be included or not, but cannot be retrieved alone.
> > 
> > The consequence is, instead of reading and pushing to the IIO buffers
> > all channels each time, the "minimum" number of channels will be scanned
> > and pushed based on the ->active_scan_mask.
> > 
> > For example, if the user wants channels 1, 4 and 5, all channels from
> > 0 to 5 will be scanned and pushed to the IIO buffers. The core will then
> > filter out the unneeded samples based on the ->active_scan_mask that has
> > been selected and only channels 1, 4 and 5 will be available to the user
> > in the shared buffer.  
> 
> This explanation is excellent.  If you are respinning it would be great
> to have the essence of it as a comment alongside the code.
> The bit about temp in particular was something I'd missed.

No problem, I'll add a comment inline as well.

> Nice optimization in general.

Thanks!

> Jonathan
>  

Best regards,
Miquèl

[PATCH v2 06/16] iio: adc: max1027: Rename a helper

[Miquel Raynal]

Make it clear that the *_set_trigger_state() hook is responsible for
cnvst based conversions by renaming the helper. This may avoid
confusions with software trigger support that is going to be
introduced.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/max1027.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 3c41d2c0ed2a..efea5ea14615 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -375,7 +375,7 @@ static int max1027_validate_trigger(struct iio_dev *indio_dev,
 	return 0;
 }
 
-static int max1027_set_trigger_state(struct iio_trigger *trig, bool state)
+static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
 {
 	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
 	struct max1027_state *st = iio_priv(indio_dev);
@@ -436,7 +436,7 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private)
 
 static const struct iio_trigger_ops max1027_trigger_ops = {
 	.validate_device = &iio_trigger_validate_own_device,
-	.set_trigger_state = &max1027_set_trigger_state,
+	.set_trigger_state = &max1027_set_cnvst_trigger_state,
 };
 
 static const struct iio_info max1027_info = {
-- 
2.27.0

[PATCH v2 07/16] iio: adc: max1027: Create a helper to enable/disable the cnvst trigger

[Miquel Raynal]

There are two ways to physically trigger a conversion:
- A falling edge on the cnvst pin
- A write operation on the conversion register

Let's create a helper for this.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/max1027.c | 41 ++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index efea5ea14615..7611d96fc901 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -257,6 +257,25 @@ struct max1027_state {
 	u8				reg ____cacheline_aligned;
 };
 
+static int max1027_enable_trigger(struct iio_dev *indio_dev, bool enable)
+{
+	struct max1027_state *st = iio_priv(indio_dev);
+
+	st->reg = MAX1027_SETUP_REG | MAX1027_REF_MODE2;
+
+	/*
+	 * Start acquisition on:
+	 * MODE0: external hardware trigger wired to the cnvst input pin
+	 * MODE2: conversion register write
+	 */
+	if (enable)
+		st->reg |= MAX1027_CKS_MODE0;
+	else
+		st->reg |= MAX1027_CKS_MODE2;
+
+	return spi_write(st->spi, &st->reg, 1);
+}
+
 static int max1027_read_single_value(struct iio_dev *indio_dev,
 				     struct iio_chan_spec const *chan,
 				     int *val)
@@ -269,14 +288,9 @@ static int max1027_read_single_value(struct iio_dev *indio_dev,
 		return -EBUSY;
 	}
 
-	/* Start acquisition on conversion register write */
-	st->reg = MAX1027_SETUP_REG | MAX1027_REF_MODE2 | MAX1027_CKS_MODE2;
-	ret = spi_write(st->spi, &st->reg, 1);
-	if (ret < 0) {
-		dev_err(&indio_dev->dev,
-			"Failed to configure setup register\n");
+	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) |
@@ -382,11 +396,8 @@ static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
 	int ret;
 
 	if (state) {
-		/* Start acquisition on cnvst */
-		st->reg = MAX1027_SETUP_REG | MAX1027_CKS_MODE0 |
-			  MAX1027_REF_MODE2;
-		ret = spi_write(st->spi, &st->reg, 1);
-		if (ret < 0)
+		ret = max1027_enable_trigger(indio_dev, state);
+		if (ret)
 			return ret;
 
 		/*
@@ -403,10 +414,8 @@ static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
 			return ret;
 	} else {
 		/* Start acquisition on conversion register write */
-		st->reg = MAX1027_SETUP_REG | MAX1027_CKS_MODE2	|
-			  MAX1027_REF_MODE2;
-		ret = spi_write(st->spi, &st->reg, 1);
-		if (ret < 0)
+		ret = max1027_enable_trigger(indio_dev, state);
+		if (ret)
 			return ret;
 	}
 
-- 
2.27.0

[PATCH v2 08/16] iio: adc: max1027: Simplify the _set_trigger_state() helper

[Miquel Raynal]

The call to max1027_enable_trigger() is the same in both cases thanks to
the 'state' variable, so factorize a little bit to simplify the code and
explain why we call this helper.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 7611d96fc901..9269931ca09d 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -395,11 +395,17 @@ static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
 	struct max1027_state *st = iio_priv(indio_dev);
 	int ret;
 
+	/*
+	 * In order to disable the convst trigger, start acquisition on
+	 * conversion register write, which basically disables triggering
+	 * conversions upon cnvst changes and thus has the effect of disabling
+	 * the external hardware trigger.
+	 */
+	ret = max1027_enable_trigger(indio_dev, state);
+	if (ret)
+		return ret;
+
 	if (state) {
-		ret = max1027_enable_trigger(indio_dev, state);
-		if (ret)
-			return ret;
-
 		/*
 		 * Scan from chan 0 to the highest requested channel.
 		 * Include temperature on demand.
@@ -412,11 +418,6 @@ static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
 		ret = spi_write(st->spi, &st->reg, 1);
 		if (ret < 0)
 			return ret;
-	} else {
-		/* Start acquisition on conversion register write */
-		ret = max1027_enable_trigger(indio_dev, state);
-		if (ret)
-			return ret;
 	}
 
 	return 0;
-- 
2.27.0

[PATCH v2 09/16] iio: adc: max1027: Ensure a default cnvst trigger configuration

[Miquel Raynal]

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 <miquel.raynal@bootlin.com>
---
 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

[PATCH v2 10/16] iio: adc: max1027: Create a helper to configure the channels to scan

[Miquel Raynal]

These bits are meant to be reused for triggered buffers setup.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 476b90f12330..f4cb5c75604b 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -257,6 +257,19 @@ struct max1027_state {
 	u8				reg ____cacheline_aligned;
 };
 
+/* Scan from chan 0 to the highest requested channel. Include temperature on demand. */
+static int max1027_configure_chans_and_start(struct iio_dev *indio_dev)
+{
+	struct max1027_state *st = iio_priv(indio_dev);
+
+	st->reg = MAX1027_CONV_REG | MAX1027_SCAN_0_N;
+	st->reg |= MAX1027_CHAN(fls(*indio_dev->active_scan_mask) - 2);
+	if (*indio_dev->active_scan_mask & MAX1X27_SCAN_MASK_TEMP)
+		st->reg |= MAX1027_TEMP;
+
+	return spi_write(st->spi, &st->reg, 1);
+}
+
 static int max1027_enable_trigger(struct iio_dev *indio_dev, bool enable)
 {
 	struct max1027_state *st = iio_priv(indio_dev);
@@ -388,7 +401,6 @@ static int max1027_validate_trigger(struct iio_dev *indio_dev,
 static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
 {
 	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
-	struct max1027_state *st = iio_priv(indio_dev);
 	int ret;
 
 	/*
@@ -402,17 +414,8 @@ static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
 		return ret;
 
 	if (state) {
-		/*
-		 * Scan from chan 0 to the highest requested channel.
-		 * Include temperature on demand.
-		 */
-		st->reg = MAX1027_CONV_REG | MAX1027_SCAN_0_N;
-		st->reg |= MAX1027_CHAN(fls(*indio_dev->active_scan_mask) - 2);
-		if (*indio_dev->active_scan_mask & MAX1X27_SCAN_MASK_TEMP)
-			st->reg |= MAX1027_TEMP;
-
-		ret = spi_write(st->spi, &st->reg, 1);
-		if (ret < 0)
+		ret = max1027_configure_chans_and_start(indio_dev);
+		if (ret)
 			return ret;
 	}
 
-- 
2.27.0

[PATCH v2 11/16] iio: adc: max1027: Prevent single channel accesses during buffer reads

[Miquel Raynal]

When hardware buffers are enabled (the cnvst pin being the trigger), one
should not mess with the device state by requesting a single channel
read.

There is already a iio_buffer_enabled() check in *_read_single_value()
to merely prevent this situation but the check is inconsistent since
buffers can be enabled after the if clause anyway. Instead, use the core
mutex by calling iio_device_claim/release_direct_mode().

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 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 f4cb5c75604b..57f62ea2d7aa 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -296,10 +296,9 @@ static int max1027_read_single_value(struct iio_dev *indio_dev,
 	int ret;
 	struct max1027_state *st = iio_priv(indio_dev);
 
-	if (iio_buffer_enabled(indio_dev)) {
-		dev_warn(&indio_dev->dev, "trigger mode already enabled");
-		return -EBUSY;
-	}
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
 
 	/* Configure conversion register with the requested chan */
 	st->reg = MAX1027_CONV_REG | MAX1027_CHAN(chan->channel) |
@@ -325,6 +324,8 @@ static int max1027_read_single_value(struct iio_dev *indio_dev,
 	if (ret < 0)
 		return ret;
 
+	iio_device_release_direct_mode(indio_dev);
+
 	*val = be16_to_cpu(st->buffer[0]);
 
 	return IIO_VAL_INT;
-- 
2.27.0

Re: [PATCH v2 11/16] iio: adc: max1027: Prevent single channel accesses during buffer reads

[Jonathan Cameron]

On Thu,  2 Sep 2021 23:14:32 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> When hardware buffers are enabled (the cnvst pin being the trigger), one
> should not mess with the device state by requesting a single channel
> read.
> 
> There is already a iio_buffer_enabled() check in *_read_single_value()
> to merely prevent this situation but the check is inconsistent since
> buffers can be enabled after the if clause anyway. Instead, use the core
> mutex by calling iio_device_claim/release_direct_mode().
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  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 f4cb5c75604b..57f62ea2d7aa 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -296,10 +296,9 @@ static int max1027_read_single_value(struct iio_dev *indio_dev,
>  	int ret;
>  	struct max1027_state *st = iio_priv(indio_dev);
>  
> -	if (iio_buffer_enabled(indio_dev)) {
> -		dev_warn(&indio_dev->dev, "trigger mode already enabled");
> -		return -EBUSY;
> -	}
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;

>  
>  	/* Configure conversion register with the requested chan */
>  	st->reg = MAX1027_CONV_REG | MAX1027_CHAN(chan->channel) |
> @@ -325,6 +324,8 @@ static int max1027_read_single_value(struct iio_dev *indio_dev,
>  	if (ret < 0)
>  		return ret;

Must also be released in error paths.  Treat it like a lock (there is one
internally) so to avoid permanent deadlock we must release it even if things
are going wrong

>  
> +	iio_device_release_direct_mode(indio_dev);
> +
>  	*val = be16_to_cpu(st->buffer[0]);
>  
>  	return IIO_VAL_INT;

[PATCH v2 12/16] iio: adc: max1027: Separate the IRQ handler from the read logic

[Miquel Raynal]

Create a max1027_read_scan() helper which will make clearer the future IRQ
handler updates (no functional change).

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 57f62ea2d7aa..5456432b9404 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -423,22 +423,37 @@ static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
 	return 0;
 }
 
-static irqreturn_t max1027_trigger_handler(int irq, void *private)
+static int max1027_read_scan(struct iio_dev *indio_dev)
 {
-	struct iio_poll_func *pf = private;
-	struct iio_dev *indio_dev = pf->indio_dev;
 	struct max1027_state *st = iio_priv(indio_dev);
 	unsigned int scanned_chans;
+	int ret;
 
 	scanned_chans = fls(*indio_dev->active_scan_mask) - 1;
 	if (*indio_dev->active_scan_mask & MAX1X27_SCAN_MASK_TEMP)
 		scanned_chans++;
 
 	/* fill buffer with all channel */
-	spi_read(st->spi, st->buffer, scanned_chans * 2);
+	ret = spi_read(st->spi, st->buffer, scanned_chans * 2);
+	if (ret < 0)
+		return ret;
 
 	iio_push_to_buffers(indio_dev, st->buffer);
 
+	return 0;
+}
+
+static irqreturn_t max1027_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	int ret;
+
+	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;
-- 
2.27.0

[PATCH v2 13/16] iio: adc: max1027: Introduce an end of conversion helper

[Miquel Raynal]

For now this helper only waits for the maximum duration of a single
conversion (even though a temperature measurement will take twice this
time, we only care about the temperature which happens first, so we will
still only wait for a single sample).

This helper will soon be improved to properly handle the end of
conversion interrupt as well as a higher number of samples.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 5456432b9404..b85fe0a48ff9 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -60,6 +60,9 @@
 #define MAX1027_NAVG_32   (0x03 << 2)
 #define MAX1027_AVG_EN    BIT(4)
 
+/* Device can achieve 300ksps so we assume a 3.33us conversion delay */
+#define MAX1027_CONVERSION_UDELAY 4
+
 enum max1027_id {
 	max1027,
 	max1029,
@@ -257,6 +260,15 @@ struct max1027_state {
 	u8				reg ____cacheline_aligned;
 };
 
+static int max1027_wait_eoc(struct iio_dev *indio_dev)
+{
+	unsigned int conversion_time = MAX1027_CONVERSION_UDELAY;
+
+	usleep_range(conversion_time, conversion_time * 2);
+
+	return 0;
+}
+
 /* Scan from chan 0 to the highest requested channel. Include temperature on demand. */
 static int max1027_configure_chans_and_start(struct iio_dev *indio_dev)
 {
@@ -315,9 +327,11 @@ static int max1027_read_single_value(struct iio_dev *indio_dev,
 	/*
 	 * For an unknown reason, when we use the mode "10" (write
 	 * conversion register), the interrupt doesn't occur every time.
-	 * So we just wait 1 ms.
+	 * So we just wait the maximum conversion time and deliver the value.
 	 */
-	mdelay(1);
+	ret = max1027_wait_eoc(indio_dev);
+	if (ret)
+		return ret;
 
 	/* Read result */
 	ret = spi_read(st->spi, st->buffer, (chan->type == IIO_TEMP) ? 4 : 2);
-- 
2.27.0

[PATCH v2 14/16] iio: adc: max1027: Don't just sleep when the EOC interrupt is available

[Miquel Raynal]

The interrupt will fire upon end of conversion. This currently can
happen in two situations: either the cnvst trigger was enabled and
toggled, or a single read was requested and the data is ready. The first
situation is already covered while the second is not. Instead, a waiting
delay is applied. Let's handle these interrupts more properly by adding
second path in our EOC helper.

Rename the interrupt handler to a more generic name as it won't only
handle triggered situations.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index b85fe0a48ff9..e734d32a5507 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -256,15 +256,27 @@ struct max1027_state {
 	struct iio_trigger		*trig;
 	__be16				*buffer;
 	struct mutex			lock;
+	struct completion		complete;
 
 	u8				reg ____cacheline_aligned;
 };
 
 static int max1027_wait_eoc(struct iio_dev *indio_dev)
 {
+	struct max1027_state *st = iio_priv(indio_dev);
 	unsigned int conversion_time = MAX1027_CONVERSION_UDELAY;
+	int ret;
 
-	usleep_range(conversion_time, conversion_time * 2);
+	if (st->spi->irq) {
+		ret = wait_for_completion_timeout(&st->complete,
+						  msecs_to_jiffies(1000));
+		if (!ret)
+			return ret;
+
+		reinit_completion(&st->complete);
+	} else {
+		usleep_range(conversion_time, conversion_time * 2);
+	}
 
 	return 0;
 }
@@ -457,12 +469,24 @@ static int max1027_read_scan(struct iio_dev *indio_dev)
 	return 0;
 }
 
-static irqreturn_t max1027_trigger_handler(int irq, void *private)
+static irqreturn_t max1027_threaded_handler(int irq, void *private)
 {
 	struct iio_poll_func *pf = private;
 	struct iio_dev *indio_dev = pf->indio_dev;
+	struct max1027_state *st = iio_priv(indio_dev);
 	int ret;
 
+	/*
+	 * When buffers are disabled, no trigger is in use but if we are here it
+	 * means that the device IRQ is enabled:
+	 * this is a single read EOC interrupt, we only need to call complete()
+	 * and return.
+	 */
+	if (!iio_buffer_enabled(indio_dev)) {
+		complete(&st->complete);
+		return IRQ_HANDLED;
+	}
+
 	ret = max1027_read_scan(indio_dev);
 	if (ret)
 		dev_err(&indio_dev->dev,
@@ -501,6 +525,7 @@ static int max1027_probe(struct spi_device *spi)
 	st->info = &max1027_chip_info_tbl[spi_get_device_id(spi)->driver_data];
 
 	mutex_init(&st->lock);
+	init_completion(&st->complete);
 
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->info = &max1027_info;
@@ -516,7 +541,7 @@ static int max1027_probe(struct spi_device *spi)
 	if (spi->irq) {
 		ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
 						      &iio_pollfunc_store_time,
-						      &max1027_trigger_handler,
+						      &max1027_threaded_handler,
 						      NULL);
 		if (ret < 0) {
 			dev_err(&indio_dev->dev, "Failed to setup buffer\n");
-- 
2.27.0

RE: [PATCH v2 14/16] iio: adc: max1027: Don't just sleep when the EOC interrupt is available

[Sa, Nuno]

> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> Sent: Thursday, September 2, 2021 11:15 PM
> To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; Sa, Nuno
> <Nuno.Sa@analog.com>; Miquel Raynal <miquel.raynal@bootlin.com>
> Subject: [PATCH v2 14/16] iio: adc: max1027: Don't just sleep when the
> EOC interrupt is available
> 
> [External]
> 
> The interrupt will fire upon end of conversion. This currently can
> happen in two situations: either the cnvst trigger was enabled and
> toggled, or a single read was requested and the data is ready. The first
> situation is already covered while the second is not. Instead, a waiting
> delay is applied. Let's handle these interrupts more properly by adding
> second path in our EOC helper.
> 
> Rename the interrupt handler to a more generic name as it won't only
> handle triggered situations.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/iio/adc/max1027.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index b85fe0a48ff9..e734d32a5507 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -256,15 +256,27 @@ struct max1027_state {
>  	struct iio_trigger		*trig;
>  	__be16				*buffer;
>  	struct mutex			lock;
> +	struct completion		complete;
> 
>  	u8				reg ____cacheline_aligned;
>  };
> 
>  static int max1027_wait_eoc(struct iio_dev *indio_dev)
>  {
> +	struct max1027_state *st = iio_priv(indio_dev);
>  	unsigned int conversion_time =
> MAX1027_CONVERSION_UDELAY;
> +	int ret;
> 
> -	usleep_range(conversion_time, conversion_time * 2);
> +	if (st->spi->irq) {
> +		ret = wait_for_completion_timeout(&st->complete,
> +
> msecs_to_jiffies(1000));
> +		if (!ret)
> +			return ret;
> +
> +		reinit_completion(&st->complete);

I would call this before the waiting...

- Nuno Sá

Re: [PATCH v2 14/16] iio: adc: max1027: Don't just sleep when the EOC interrupt is available

[Miquel Raynal]

Hi Nuno,

Nuno.Sa@analog.com wrote on Mon, 6 Sep 2021 09:38:02 +0000:

> > -----Original Message-----
> > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > Sent: Thursday, September 2, 2021 11:15 PM
> > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> > <lars@metafoo.de>; linux-iio@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; Sa, Nuno
> > <Nuno.Sa@analog.com>; Miquel Raynal <miquel.raynal@bootlin.com>
> > Subject: [PATCH v2 14/16] iio: adc: max1027: Don't just sleep when the
> > EOC interrupt is available
> > 
> > [External]
> > 
> > The interrupt will fire upon end of conversion. This currently can
> > happen in two situations: either the cnvst trigger was enabled and
> > toggled, or a single read was requested and the data is ready. The first
> > situation is already covered while the second is not. Instead, a waiting
> > delay is applied. Let's handle these interrupts more properly by adding
> > second path in our EOC helper.
> > 
> > Rename the interrupt handler to a more generic name as it won't only
> > handle triggered situations.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/iio/adc/max1027.c | 31 ++++++++++++++++++++++++++++---
> >  1 file changed, 28 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > index b85fe0a48ff9..e734d32a5507 100644
> > --- a/drivers/iio/adc/max1027.c
> > +++ b/drivers/iio/adc/max1027.c
> > @@ -256,15 +256,27 @@ struct max1027_state {
> >  	struct iio_trigger		*trig;
> >  	__be16				*buffer;
> >  	struct mutex			lock;
> > +	struct completion		complete;
> > 
> >  	u8				reg ____cacheline_aligned;
> >  };
> > 
> >  static int max1027_wait_eoc(struct iio_dev *indio_dev)
> >  {
> > +	struct max1027_state *st = iio_priv(indio_dev);
> >  	unsigned int conversion_time =
> > MAX1027_CONVERSION_UDELAY;
> > +	int ret;
> > 
> > -	usleep_range(conversion_time, conversion_time * 2);
> > +	if (st->spi->irq) {
> > +		ret = wait_for_completion_timeout(&st->complete,
> > +
> > msecs_to_jiffies(1000));
> > +		if (!ret)
> > +			return ret;
> > +
> > +		reinit_completion(&st->complete);  
> 
> I would call this before the waiting...

Sure, this is probably better.

Thanks,
Miquèl

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

[Miquel Raynal]

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 <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 59 +++++++++++++++++++++++++++++++--------
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index e734d32a5507..b9b7b9245509 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -414,17 +414,6 @@ static int max1027_debugfs_reg_access(struct iio_dev *indio_dev,
 	return spi_write(st->spi, val, 1);
 }
 
-static int max1027_validate_trigger(struct iio_dev *indio_dev,
-				    struct iio_trigger *trig)
-{
-	struct max1027_state *st = iio_priv(indio_dev);
-
-	if (st->trig != trig)
-		return -EINVAL;
-
-	return 0;
-}
-
 static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
 {
 	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
@@ -469,6 +458,13 @@ static int max1027_read_scan(struct iio_dev *indio_dev)
 	return 0;
 }
 
+static bool max1027_own_trigger_enabled(struct iio_dev *indio_dev)
+{
+	int ret = iio_trigger_validate_own_device(indio_dev->trig, indio_dev);
+
+	return ret ? false : true;
+}
+
 static irqreturn_t max1027_threaded_handler(int irq, void *private)
 {
 	struct iio_poll_func *pf = private;
@@ -487,7 +483,47 @@ static irqreturn_t max1027_threaded_handler(int irq, void *private)
 		return IRQ_HANDLED;
 	}
 
+	/* From that point on, buffers are enabled */
+
+	/*
+	 * The cnvst HW trigger is not in use:
+	 * we need to handle an external trigger request.
+	 */
+	if (!max1027_own_trigger_enabled(indio_dev)) {
+		if (irq != st->spi->irq) {
+			/*
+			 * First, the IRQ number will be the one allocated for
+			 * this poll function by the IIO core, it means that
+			 * this is an external trigger request, we need to start
+			 * a conversion.
+			 */
+			ret = max1027_configure_chans_and_start(indio_dev);
+			if (ret)
+				goto out;
+
+			ret = max1027_wait_eoc(indio_dev);
+			if (ret)
+				goto out;
+		} else {
+			/*
+			 * The pollfunc that has been called "manually" by the
+			 * IIO core now expects the EOC signaling (this is the
+			 * device IRQ firing), we need to call complete().
+			 */
+			complete(&st->complete);
+			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

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

[Jonathan Cameron]

On Thu,  2 Sep 2021 23:14:36 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> So far the driver only supported to use the hardware cnvst trigger. This
> was purely a software limitation.
> 
> The IRQ handler is already registered as being a poll function and thus
> can be called upon external triggering. In this case, a new conversion
> must be started, and one must wait for the data to be ready before
> reading the samples.
> 
> As the same handler can be called from different places, we check the
> value of the current IRQ with the value of the registered device
> IRQ. Indeed, the first step is to get called with a different IRQ number
> than ours, this is the "pullfunc" version which requests a new

pullfunc?

> conversion. During the execution of the handler, we will wait for the
> EOC interrupt to happen. This interrupt is handled by the same
> helper. This time the IRQ number is the one we registered, we can in
> this case call complete() to unlock the primary handler and return. The
> primary handler continues executing by retrieving the data normally and
> finally returns.

Interesting to use the irq number..

I'm a little nervous about how this has ended up structured.
I'm not 100% sure my understanding of how you've done it is correct.

We should have the following situation:

IRQ IN
  |
  v
Trigger IRQ / EOC IRQ  (this is the spi->irq)  (currently iio_trigger_generic_data_poll_ready)
  |              |
  ---------      v
  |        |   complete
  v        v
TrigH1    (TrigH2)   (these are the IRQs below the irq_chip IIO uses to demux triggers)


So when using it's own trigger we are using an internal interrupt
tree burried inside the IIO core.  When using it only as an EOC interrupt we shouldn't
be anywhere near that internal interrupt chip.

So I'm surprised the IRQ matches with the spi->irq as 
those trigH1 and trigH2 will have their own IRQ numbers.

For reference I think your architecture is currently

IRQ IN
  |
  v
  Trigger IRQ
  |
  v
 TRIG H1
 Either fills the buffer or does the completion.

I am a little confused how this works with an external trigger because the Trig H1 interrupt
should be disabled unless we are using the trigger.  That control isn't exposed to the
driver at all.

Is my understanding right or have I gotten confused somewhere?
I also can't see a path in which the eoc interrupt will get fired for raw_reads.

Could you talk me through how that works currently?

I suspect part of the confusion here is that this driver happens to be using the
standard core handler iio_trigger_generic_data_rdy_poll which hides away that
there are two interrupt handlers in a normal IIO driver for a device with a
trigger and buffered mode.
1 for the trigger and 1 for the buffer.  Whether the buffer one is a result
of the trigger one (via iio_poll_trigger) is down to whether the device is
using it's own trigger or not.

Jonathan



> 
> In order to authorize external triggers, we need to drop the
> ->validate_trigger() verification.  
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/iio/adc/max1027.c | 59 +++++++++++++++++++++++++++++++--------
>  1 file changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index e734d32a5507..b9b7b9245509 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -414,17 +414,6 @@ static int max1027_debugfs_reg_access(struct iio_dev *indio_dev,
>  	return spi_write(st->spi, val, 1);
>  }
>  
> -static int max1027_validate_trigger(struct iio_dev *indio_dev,
> -				    struct iio_trigger *trig)
> -{
> -	struct max1027_state *st = iio_priv(indio_dev);
> -
> -	if (st->trig != trig)
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>  static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
>  {
>  	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> @@ -469,6 +458,13 @@ static int max1027_read_scan(struct iio_dev *indio_dev)
>  	return 0;
>  }
>  
> +static bool max1027_own_trigger_enabled(struct iio_dev *indio_dev)
> +{
> +	int ret = iio_trigger_validate_own_device(indio_dev->trig, indio_dev);
> +
> +	return ret ? false : true;
> +}
> +
>  static irqreturn_t max1027_threaded_handler(int irq, void *private)
>  {
>  	struct iio_poll_func *pf = private;
> @@ -487,7 +483,47 @@ static irqreturn_t max1027_threaded_handler(int irq, void *private)
>  		return IRQ_HANDLED;
>  	}
>  
> +	/* From that point on, buffers are enabled */
> +
> +	/*
> +	 * The cnvst HW trigger is not in use:
> +	 * we need to handle an external trigger request.
> +	 */
> +	if (!max1027_own_trigger_enabled(indio_dev)) {
> +		if (irq != st->spi->irq) {
> +			/*
> +			 * First, the IRQ number will be the one allocated for
> +			 * this poll function by the IIO core, it means that
> +			 * this is an external trigger request, we need to start
> +			 * a conversion.
> +			 */
> +			ret = max1027_configure_chans_and_start(indio_dev);
> +			if (ret)
> +				goto out;
> +
> +			ret = max1027_wait_eoc(indio_dev);
> +			if (ret)
> +				goto out;
> +		} else {
> +			/*
> +			 * The pollfunc that has been called "manually" by the
> +			 * IIO core now expects the EOC signaling (this is the
> +			 * device IRQ firing), we need to call complete().
> +			 */
> +			complete(&st->complete);

Completion shouldn't be down here in the trigger handler, it should be in the top
level interrupt handler.  So you need to replace the
iio_trigger_generic_data_poll with a specific handler for this device.

> +			return IRQ_HANDLED;
> +		}
> +	}
> +
> +	/*
> +	 * We end here under two situations:
> +	 * - an external trigger is in use and the *_wait_eoc() call succeeded,
> +	 *   the data is ready and may be retrieved.
> +	 * - the cnvst HW trigger is in use (the handler actually starts here),
> +	 *   the data is also ready.
> +	 */
>  	ret = max1027_read_scan(indio_dev);
> +out:
>  	if (ret)
>  		dev_err(&indio_dev->dev,
>  			"Cannot read scanned values (%d)\n", ret);
> @@ -504,7 +540,6 @@ static const struct iio_trigger_ops max1027_trigger_ops = {
>  
>  static const struct iio_info max1027_info = {
>  	.read_raw = &max1027_read_raw,
> -	.validate_trigger = &max1027_validate_trigger,
>  	.debugfs_reg_access = &max1027_debugfs_reg_access,
>  };
>  

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

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, September 5, 2021 6:11 PM
> To: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org;
> linux-kernel@vger.kernel.org; Thomas Petazzoni
> <thomas.petazzoni@bootlin.com>; Sa, Nuno <Nuno.Sa@analog.com>
> Subject: Re: [PATCH v2 15/16] iio: adc: max1027: Add support for
> external triggers
> 
> [External]
> 
> On Thu,  2 Sep 2021 23:14:36 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > So far the driver only supported to use the hardware cnvst trigger.
> This
> > was purely a software limitation.
> >
> > The IRQ handler is already registered as being a poll function and
> thus
> > can be called upon external triggering. In this case, a new conversion
> > must be started, and one must wait for the data to be ready before
> > reading the samples.
> >
> > As the same handler can be called from different places, we check
> the
> > value of the current IRQ with the value of the registered device
> > IRQ. Indeed, the first step is to get called with a different IRQ
> number
> > than ours, this is the "pullfunc" version which requests a new
> 
> pullfunc?
> 
> > conversion. During the execution of the handler, we will wait for the
> > EOC interrupt to happen. This interrupt is handled by the same
> > helper. This time the IRQ number is the one we registered, we can in
> > this case call complete() to unlock the primary handler and return.
> The
> > primary handler continues executing by retrieving the data normally
> and
> > finally returns.
> 
> Interesting to use the irq number..
> 
> I'm a little nervous about how this has ended up structured.
> I'm not 100% sure my understanding of how you've done it is correct.
> 
> We should have the following situation:
> 
> IRQ IN
>   |
>   v
> Trigger IRQ / EOC IRQ  (this is the spi->irq)  (currently
> iio_trigger_generic_data_poll_ready)
>   |              |
>   ---------      v
>   |        |   complete
>   v        v
> TrigH1    (TrigH2)   (these are the IRQs below the irq_chip IIO uses to
> demux triggers)
> 
> 
> So when using it's own trigger we are using an internal interrupt
> tree burried inside the IIO core.  When using it only as an EOC interrupt
> we shouldn't
> be anywhere near that internal interrupt chip.
> 
> So I'm surprised the IRQ matches with the spi->irq as
> those trigH1 and trigH2 will have their own IRQ numbers.
> 
> For reference I think your architecture is currently
> 
> IRQ IN
>   |
>   v
>   Trigger IRQ
>   |
>   v
>  TRIG H1
>  Either fills the buffer or does the completion.
> 
> I am a little confused how this works with an external trigger because
> the Trig H1 interrupt
> should be disabled unless we are using the trigger.  That control isn't
> exposed to the
> driver at all.
> 
> Is my understanding right or have I gotten confused somewhere?
> I also can't see a path in which the eoc interrupt will get fired for
> raw_reads.
> 
> Could you talk me through how that works currently?
> 
> I suspect part of the confusion here is that this driver happens to be
> using the
> standard core handler iio_trigger_generic_data_rdy_poll which hides
> away that
> there are two interrupt handlers in a normal IIO driver for a device with
> a
> trigger and buffered mode.
> 1 for the trigger and 1 for the buffer.  Whether the buffer one is a
> result
> of the trigger one (via iio_poll_trigger) is down to whether the device
> is
> using it's own trigger or not.
> 
> Jonathan
> 
> 
> 
> >
> > In order to authorize external triggers, we need to drop the
> > ->validate_trigger() verification.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/iio/adc/max1027.c | 59
> +++++++++++++++++++++++++++++++--------
> >  1 file changed, 47 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > index e734d32a5507..b9b7b9245509 100644
> > --- a/drivers/iio/adc/max1027.c
> > +++ b/drivers/iio/adc/max1027.c
> > @@ -414,17 +414,6 @@ static int
> max1027_debugfs_reg_access(struct iio_dev *indio_dev,
> >  	return spi_write(st->spi, val, 1);
> >  }
> >
> > -static int max1027_validate_trigger(struct iio_dev *indio_dev,
> > -				    struct iio_trigger *trig)
> > -{
> > -	struct max1027_state *st = iio_priv(indio_dev);
> > -
> > -	if (st->trig != trig)
> > -		return -EINVAL;
> > -
> > -	return 0;
> > -}
> > -
> >  static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig,
> bool state)
> >  {
> >  	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > @@ -469,6 +458,13 @@ static int max1027_read_scan(struct iio_dev
> *indio_dev)
> >  	return 0;
> >  }
> >
> > +static bool max1027_own_trigger_enabled(struct iio_dev
> *indio_dev)
> > +{
> > +	int ret = iio_trigger_validate_own_device(indio_dev->trig,
> indio_dev);
> > +
> > +	return ret ? false : true;
> > +}
> > +
> >  static irqreturn_t max1027_threaded_handler(int irq, void *private)
> >  {
> >  	struct iio_poll_func *pf = private;
> > @@ -487,7 +483,47 @@ static irqreturn_t
> max1027_threaded_handler(int irq, void *private)
> >  		return IRQ_HANDLED;
> >  	}
> >
> > +	/* From that point on, buffers are enabled */
> > +
> > +	/*
> > +	 * The cnvst HW trigger is not in use:
> > +	 * we need to handle an external trigger request.
> > +	 */
> > +	if (!max1027_own_trigger_enabled(indio_dev)) {
> > +		if (irq != st->spi->irq) {
> > +			/*
> > +			 * First, the IRQ number will be the one
> allocated for
> > +			 * this poll function by the IIO core, it means
> that
> > +			 * this is an external trigger request, we need to
> start
> > +			 * a conversion.
> > +			 */
> > +			ret =
> max1027_configure_chans_and_start(indio_dev);
> > +			if (ret)
> > +				goto out;
> > +
> > +			ret = max1027_wait_eoc(indio_dev);
> > +			if (ret)
> > +				goto out;
> > +		} else {
> > +			/*
> > +			 * The pollfunc that has been called "manually"
> by the
> > +			 * IIO core now expects the EOC signaling (this
> is the
> > +			 * device IRQ firing), we need to call
> complete().
> > +			 */
> > +			complete(&st->complete);
> 
> Completion shouldn't be down here in the trigger handler, it should be
> in the top
> level interrupt handler.  So you need to replace the
> iio_trigger_generic_data_poll with a specific handler for this device.
> 
> > +			return IRQ_HANDLED;
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * We end here under two situations:
> > +	 * - an external trigger is in use and the *_wait_eoc() call
> succeeded,
> > +	 *   the data is ready and may be retrieved.
> > +	 * - the cnvst HW trigger is in use (the handler actually starts
> here),
> > +	 *   the data is also ready.
> > +	 */
> >  	ret = max1027_read_scan(indio_dev);
> > +out:
> >  	if (ret)
> >  		dev_err(&indio_dev->dev,
> >  			"Cannot read scanned values (%d)\n", ret);
> > @@ -504,7 +540,6 @@ static const struct iio_trigger_ops
> max1027_trigger_ops = {
> >
> >  static const struct iio_info max1027_info = {
> >  	.read_raw = &max1027_read_raw,
> > -	.validate_trigger = &max1027_validate_trigger,
> >  	.debugfs_reg_access = &max1027_debugfs_reg_access,
> >  };
> >

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

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

- Nuno Sá

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

[Miquel Raynal]

Hi Nuno, Jonathan,

Nuno.Sa@analog.com wrote on Mon, 6 Sep 2021 09:53:15 +0000:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, September 5, 2021 6:11 PM
> > To: Miquel Raynal <miquel.raynal@bootlin.com>
> > Cc: Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Thomas Petazzoni
> > <thomas.petazzoni@bootlin.com>; Sa, Nuno <Nuno.Sa@analog.com>
> > Subject: Re: [PATCH v2 15/16] iio: adc: max1027: Add support for
> > external triggers
> > 
> > [External]
> > 
> > On Thu,  2 Sep 2021 23:14:36 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > So far the driver only supported to use the hardware cnvst trigger.  
> > This  
> > > was purely a software limitation.
> > >
> > > The IRQ handler is already registered as being a poll function and  
> > thus  
> > > can be called upon external triggering. In this case, a new conversion
> > > must be started, and one must wait for the data to be ready before
> > > reading the samples.
> > >
> > > As the same handler can be called from different places, we check  
> > the  
> > > value of the current IRQ with the value of the registered device
> > > IRQ. Indeed, the first step is to get called with a different IRQ  
> > number  
> > > than ours, this is the "pullfunc" version which requests a new  
> > 
> > pullfunc?
> >   
> > > conversion. During the execution of the handler, we will wait for the
> > > EOC interrupt to happen. This interrupt is handled by the same
> > > helper. This time the IRQ number is the one we registered, we can in
> > > this case call complete() to unlock the primary handler and return.  
> > The  
> > > primary handler continues executing by retrieving the data normally  
> > and  
> > > finally returns.  
> > 
> > Interesting to use the irq number..
> > 
> > I'm a little nervous about how this has ended up structured.
> > I'm not 100% sure my understanding of how you've done it is correct.
> > 
> > We should have the following situation:
> > 
> > IRQ IN
> >   |
> >   v
> > Trigger IRQ / EOC IRQ  (this is the spi->irq)  (currently
> > iio_trigger_generic_data_poll_ready)
> >   |              |
> >   ---------      v
> >   |        |   complete
> >   v        v
> > TrigH1    (TrigH2)   (these are the IRQs below the irq_chip IIO uses to
> > demux triggers)
> > 
> > 
> > So when using it's own trigger we are using an internal interrupt
> > tree burried inside the IIO core.  When using it only as an EOC interrupt
> > we shouldn't
> > be anywhere near that internal interrupt chip.
> > 
> > So I'm surprised the IRQ matches with the spi->irq as
> > those trigH1 and trigH2 will have their own IRQ numbers.
> > 
> > For reference I think your architecture is currently
> > 
> > IRQ IN
> >   |
> >   v
> >   Trigger IRQ
> >   |
> >   v
> >  TRIG H1
> >  Either fills the buffer or does the completion.
> > 
> > I am a little confused how this works with an external trigger because
> > the Trig H1 interrupt
> > should be disabled unless we are using the trigger.  That control isn't
> > exposed to the
> > driver at all.
> > 
> > Is my understanding right or have I gotten confused somewhere?
> > I also can't see a path in which the eoc interrupt will get fired for
> > raw_reads.
> > 
> > Could you talk me through how that works currently?

I 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

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

[Jonathan Cameron]

On Wed, 15 Sep 2021 17:45:07 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Nuno, Jonathan,
> 
> Nuno.Sa@analog.com wrote on Mon, 6 Sep 2021 09:53:15 +0000:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Sunday, September 5, 2021 6:11 PM
> > > To: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Cc: Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; Thomas Petazzoni
> > > <thomas.petazzoni@bootlin.com>; Sa, Nuno <Nuno.Sa@analog.com>
> > > Subject: Re: [PATCH v2 15/16] iio: adc: max1027: Add support for
> > > external triggers
> > > 
> > > [External]
> > > 
> > > On Thu,  2 Sep 2021 23:14:36 +0200
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >     
> > > > So far the driver only supported to use the hardware cnvst trigger.    
> > > This    
> > > > was purely a software limitation.
> > > >
> > > > The IRQ handler is already registered as being a poll function and    
> > > thus    
> > > > can be called upon external triggering. In this case, a new conversion
> > > > must be started, and one must wait for the data to be ready before
> > > > reading the samples.
> > > >
> > > > As the same handler can be called from different places, we check    
> > > the    
> > > > value of the current IRQ with the value of the registered device
> > > > IRQ. Indeed, the first step is to get called with a different IRQ    
> > > number    
> > > > than ours, this is the "pullfunc" version which requests a new    
> > > 
> > > pullfunc?
> > >     
> > > > conversion. During the execution of the handler, we will wait for the
> > > > EOC interrupt to happen. This interrupt is handled by the same
> > > > helper. This time the IRQ number is the one we registered, we can in
> > > > this case call complete() to unlock the primary handler and return.    
> > > The    
> > > > primary handler continues executing by retrieving the data normally    
> > > and    
> > > > finally returns.    
> > > 
> > > Interesting to use the irq number..
> > > 
> > > I'm a little nervous about how this has ended up structured.
> > > I'm not 100% sure my understanding of how you've done it is correct.
> > > 
> > > We should have the following situation:
> > > 
> > > IRQ IN
> > >   |
> > >   v
> > > Trigger IRQ / EOC IRQ  (this is the spi->irq)  (currently
> > > iio_trigger_generic_data_poll_ready)
> > >   |              |
> > >   ---------      v
> > >   |        |   complete
> > >   v        v
> > > TrigH1    (TrigH2)   (these are the IRQs below the irq_chip IIO uses to
> > > demux triggers)
> > > 
> > > 
> > > So when using it's own trigger we are using an internal interrupt
> > > tree burried inside the IIO core.  When using it only as an EOC interrupt
> > > we shouldn't
> > > be anywhere near that internal interrupt chip.
> > > 
> > > So I'm surprised the IRQ matches with the spi->irq as
> > > those trigH1 and trigH2 will have their own IRQ numbers.
> > > 
> > > For reference I think your architecture is currently
> > > 
> > > IRQ IN
> > >   |
> > >   v
> > >   Trigger IRQ
> > >   |
> > >   v
> > >  TRIG H1
> > >  Either fills the buffer or does the completion.
> > > 
> > > I am a little confused how this works with an external trigger because
> > > the Trig H1 interrupt
> > > should be disabled unless we are using the trigger.  That control isn't
> > > exposed to the
> > > driver at all.
> > > 
> > > Is my understanding right or have I gotten confused somewhere?
> > > I also can't see a path in which the eoc interrupt will get fired for
> > > raw_reads.
> > > 
> > > Could you talk me through how that works currently?  
> 
> I 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

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

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sat, 18 Sep 2021 18:39:20 +0100:

> On Wed, 15 Sep 2021 17:45:07 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Nuno, Jonathan,
> > 
> > Nuno.Sa@analog.com wrote on Mon, 6 Sep 2021 09:53:15 +0000:
> >   
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Sunday, September 5, 2021 6:11 PM
> > > > To: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > Cc: Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; Thomas Petazzoni
> > > > <thomas.petazzoni@bootlin.com>; Sa, Nuno <Nuno.Sa@analog.com>
> > > > Subject: Re: [PATCH v2 15/16] iio: adc: max1027: Add support for
> > > > external triggers
> > > > 
> > > > [External]
> > > > 
> > > > On Thu,  2 Sep 2021 23:14:36 +0200
> > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >       
> > > > > So far the driver only supported to use the hardware cnvst trigger.      
> > > > This      
> > > > > was purely a software limitation.
> > > > >
> > > > > The IRQ handler is already registered as being a poll function and      
> > > > thus      
> > > > > can be called upon external triggering. In this case, a new conversion
> > > > > must be started, and one must wait for the data to be ready before
> > > > > reading the samples.
> > > > >
> > > > > As the same handler can be called from different places, we check      
> > > > the      
> > > > > value of the current IRQ with the value of the registered device
> > > > > IRQ. Indeed, the first step is to get called with a different IRQ      
> > > > number      
> > > > > than ours, this is the "pullfunc" version which requests a new      
> > > > 
> > > > pullfunc?
> > > >       
> > > > > conversion. During the execution of the handler, we will wait for the
> > > > > EOC interrupt to happen. This interrupt is handled by the same
> > > > > helper. This time the IRQ number is the one we registered, we can in
> > > > > this case call complete() to unlock the primary handler and return.      
> > > > The      
> > > > > primary handler continues executing by retrieving the data normally      
> > > > and      
> > > > > finally returns.      
> > > > 
> > > > Interesting to use the irq number..
> > > > 
> > > > I'm a little nervous about how this has ended up structured.
> > > > I'm not 100% sure my understanding of how you've done it is correct.
> > > > 
> > > > We should have the following situation:
> > > > 
> > > > IRQ IN
> > > >   |
> > > >   v
> > > > Trigger IRQ / EOC IRQ  (this is the spi->irq)  (currently
> > > > iio_trigger_generic_data_poll_ready)
> > > >   |              |
> > > >   ---------      v
> > > >   |        |   complete
> > > >   v        v
> > > > TrigH1    (TrigH2)   (these are the IRQs below the irq_chip IIO uses to
> > > > demux triggers)
> > > > 
> > > > 
> > > > So when using it's own trigger we are using an internal interrupt
> > > > tree burried inside the IIO core.  When using it only as an EOC interrupt
> > > > we shouldn't
> > > > be anywhere near that internal interrupt chip.
> > > > 
> > > > So I'm surprised the IRQ matches with the spi->irq as
> > > > those trigH1 and trigH2 will have their own IRQ numbers.
> > > > 
> > > > For reference I think your architecture is currently
> > > > 
> > > > IRQ IN
> > > >   |
> > > >   v
> > > >   Trigger IRQ
> > > >   |
> > > >   v
> > > >  TRIG H1
> > > >  Either fills the buffer or does the completion.
> > > > 
> > > > I am a little confused how this works with an external trigger because
> > > > the Trig H1 interrupt
> > > > should be disabled unless we are using the trigger.  That control isn't
> > > > exposed to the
> > > > driver at all.
> > > > 
> > > > Is my understanding right or have I gotten confused somewhere?
> > > > I also can't see a path in which the eoc interrupt will get fired for
> > > > raw_reads.
> > > > 
> > > > Could you talk me through how that works currently?    
> > 
> > I 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

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

[Miquel Raynal]

Hi Jonathan, Nuno,

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

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

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

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

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

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

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

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

[Jonathan Cameron]

On Wed, 15 Sep 2021 12:18:32 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

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

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

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sat, 18 Sep 2021 18:13:08 +0100:

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

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

[Jonathan Cameron]

On Mon, 20 Sep 2021 10:37:39 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

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

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

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Mon, 20 Sep 2021 18:43:38 +0100:

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

[PATCH v2 16/16] iio: adc: max1027: Don't reject external triggers when there is no IRQ

[Miquel Raynal]

External triggers do not necessarily need the EOC interrupt to be
populated to work properly. The end of conversion status may either come
from an interrupt or from a sufficient enough extra delay. IRQs are not
mandatory so move the triggered buffer setup out of the IRQ condition
and add the logic to wait enough time for all the requested conversions
to be in the device's FIFO.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index b9b7b9245509..a673cd468051 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -275,6 +275,9 @@ static int max1027_wait_eoc(struct iio_dev *indio_dev)
 
 		reinit_completion(&st->complete);
 	} else {
+		if (indio_dev->active_scan_mask)
+			conversion_time *= hweight32(*indio_dev->active_scan_mask);
+
 		usleep_range(conversion_time, conversion_time * 2);
 	}
 
@@ -573,16 +576,18 @@ static int max1027_probe(struct spi_device *spi)
 	if (!st->buffer)
 		return -ENOMEM;
 
+	/* Enable triggered buffers */
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+					      &iio_pollfunc_store_time,
+					      &max1027_threaded_handler,
+					      NULL);
+	if (ret < 0) {
+		dev_err(&indio_dev->dev, "Failed to setup buffer\n");
+		goto free_buffer;
+	}
+
+	/* If there is an EOC interrupt, register the hardware trigger (cnvst) */
 	if (spi->irq) {
-		ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
-						      &iio_pollfunc_store_time,
-						      &max1027_threaded_handler,
-						      NULL);
-		if (ret < 0) {
-			dev_err(&indio_dev->dev, "Failed to setup buffer\n");
-			goto free_buffer;
-		}
-
 		st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-trigger",
 						  indio_dev->name);
 		if (!st->trig) {
-- 
2.27.0