linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs
@ 2021-09-15 15:51 Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 01/14] iio: adc: max1027: Fix style Miquel Raynal
                   ` (13 more replies)
  0 siblings, 14 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, 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 even without the IRQ line wired.

This series is currently based on a v5.15-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 v3:
* Rebased on top of v5.15-rc1.
* Dropped the useless change from devm_kmalloc to kmalloc because I
  thought devm_kmalloc allocations were still not suitable for DMA
  purposes.
* Added a comment explaining the use of the available and active masks
  in the code as suggested by Jonathan.
* Released the lock used in iio_device_claim_direct_mode() in the two
  error paths.
* Did not move the call to reinit_completion before
  wait_for_completion_timeout() as advised by Nuno because the
  triggering is done before entering the waiting thread, so there is a
  world were we reinit a completion object right before waiting for it
  (which would lead to a timeout).
* Deeply rewored the various handlers (see my answer to
  "[PATCH v2 15/16] iio: adc: max1027: Add support for external  triggers"

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 (14):
  iio: adc: max1027: Fix style
  iio: adc: max1027: Drop extra warning message
  iio: adc: max1027: Drop useless debug messages
  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: Deeply rework interrupt handling
  iio: adc: max1027: Don't reject external triggers when there is no IRQ

 drivers/iio/adc/max1027.c | 301 ++++++++++++++++++++++++++++----------
 1 file changed, 223 insertions(+), 78 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3 01/14] iio: adc: max1027: Fix style
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 02/14] iio: adc: max1027: Drop extra warning message Miquel Raynal
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 02/14] iio: adc: max1027: Drop extra warning message
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 01/14] iio: adc: max1027: Fix style Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 03/14] iio: adc: max1027: Drop useless debug messages Miquel Raynal
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, Miquel Raynal, Nuno Sá

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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 03/14] iio: adc: max1027: Drop useless debug messages
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 01/14] iio: adc: max1027: Fix style Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 02/14] iio: adc: max1027: Drop extra warning message Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 04/14] iio: adc: max1027: Minimize the number of converted channels Miquel Raynal
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 04/14] iio: adc: max1027: Minimize the number of converted channels
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (2 preceding siblings ...)
  2021-09-15 15:51 ` [PATCH v3 03/14] iio: adc: max1027: Drop useless debug messages Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 05/14] iio: adc: max1027: Rename a helper Miquel Raynal
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, Miquel Raynal

Provide a list of ->available_scan_masks which match the device's
capabilities. Basically, these devices are 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.

Provide a comment in the code explaining this logic.

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

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index f27044324141..7c1c76673be2 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -172,18 +172,53 @@ static const struct iio_chan_spec max1231_channels[] = {
 	MAX1X31_CHANNELS(12),
 };
 
+/*
+ * These devices are 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. Based on the below
+ * ->available_scan_masks, the core will select the most appropriate
+ * ->active_scan_mask and the "minimum" number of channels will be
+ * scanned and pushed to the buffers.
+ *
+ * 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.
+ */
+#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 +403,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 +432,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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 05/14] iio: adc: max1027: Rename a helper
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (3 preceding siblings ...)
  2021-09-15 15:51 ` [PATCH v3 04/14] iio: adc: max1027: Minimize the number of converted channels Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 06/14] iio: adc: max1027: Create a helper to enable/disable the cnvst trigger Miquel Raynal
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, Miquel Raynal, Nuno Sá

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 7c1c76673be2..e84f49f21778 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -389,7 +389,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);
@@ -450,7 +450,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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 06/14] iio: adc: max1027: Create a helper to enable/disable the cnvst trigger
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (4 preceding siblings ...)
  2021-09-15 15:51 ` [PATCH v3 05/14] iio: adc: max1027: Rename a helper Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 07/14] iio: adc: max1027: Simplify the _set_trigger_state() helper Miquel Raynal
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, Miquel Raynal, Nuno Sá

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 e84f49f21778..d1a9ffa68c38 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -271,6 +271,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)
@@ -283,14 +302,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) |
@@ -396,11 +410,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;
 
 		/*
@@ -417,10 +428,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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 07/14] iio: adc: max1027: Simplify the _set_trigger_state() helper
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (5 preceding siblings ...)
  2021-09-15 15:51 ` [PATCH v3 06/14] iio: adc: max1027: Create a helper to enable/disable the cnvst trigger Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 08/14] iio: adc: max1027: Ensure a default cnvst trigger configuration Miquel Raynal
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, 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 d1a9ffa68c38..b7cc77c2c01d 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -409,11 +409,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.
@@ -426,11 +432,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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 08/14] iio: adc: max1027: Ensure a default cnvst trigger configuration
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (6 preceding siblings ...)
  2021-09-15 15:51 ` [PATCH v3 07/14] iio: adc: max1027: Simplify the _set_trigger_state() helper Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 09/14] iio: adc: max1027: Create a helper to configure the channels to scan Miquel Raynal
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, 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 b7cc77c2c01d..62570953653a 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -302,10 +302,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;
@@ -557,6 +553,11 @@ static int max1027_probe(struct spi_device *spi)
 		return ret;
 	}
 
+	/* 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);
 }
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 09/14] iio: adc: max1027: Create a helper to configure the channels to scan
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (7 preceding siblings ...)
  2021-09-15 15:51 ` [PATCH v3 08/14] iio: adc: max1027: Ensure a default cnvst trigger configuration Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 10/14] iio: adc: max1027: Prevent single channel accesses during buffer reads Miquel Raynal
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, 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 62570953653a..254df6a28cd8 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -271,6 +271,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);
@@ -402,7 +415,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;
 
 	/*
@@ -416,17 +428,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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 10/14] iio: adc: max1027: Prevent single channel accesses during buffer reads
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (8 preceding siblings ...)
  2021-09-15 15:51 ` [PATCH v3 09/14] iio: adc: max1027: Create a helper to configure the channels to scan Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 11/14] iio: adc: max1027: Separate the IRQ handler from the read logic Miquel Raynal
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, 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 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 254df6a28cd8..c7663e5dd38a 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -310,10 +310,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) |
@@ -324,6 +323,7 @@ static int max1027_read_single_value(struct iio_dev *indio_dev,
 	if (ret < 0) {
 		dev_err(&indio_dev->dev,
 			"Failed to configure conversion register\n");
+		iio_device_release_direct_mode(indio_dev);
 		return ret;
 	}
 
@@ -336,6 +336,9 @@ static int max1027_read_single_value(struct iio_dev *indio_dev,
 
 	/* Read result */
 	ret = spi_read(st->spi, st->buffer, (chan->type == IIO_TEMP) ? 4 : 2);
+
+	iio_device_release_direct_mode(indio_dev);
+
 	if (ret < 0)
 		return ret;
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 11/14] iio: adc: max1027: Separate the IRQ handler from the read logic
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (9 preceding siblings ...)
  2021-09-15 15:51 ` [PATCH v3 10/14] iio: adc: max1027: Prevent single channel accesses during buffer reads Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 12/14] iio: adc: max1027: Introduce an end of conversion helper Miquel Raynal
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, 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 c7663e5dd38a..a044f4b598e0 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -439,22 +439,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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 12/14] iio: adc: max1027: Introduce an end of conversion helper
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (10 preceding siblings ...)
  2021-09-15 15:51 ` [PATCH v3 11/14] iio: adc: max1027: Separate the IRQ handler from the read logic Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 13/14] iio: adc: max1027: Deeply rework interrupt handling Miquel Raynal
  2021-09-15 15:51 ` [PATCH v3 14/14] iio: adc: max1027: Don't reject external triggers when there is no IRQ Miquel Raynal
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, 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 a044f4b598e0..e0175448c899 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,
@@ -271,6 +274,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)
 {
@@ -330,9 +342,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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 13/14] iio: adc: max1027: Deeply rework interrupt handling
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (11 preceding siblings ...)
  2021-09-15 15:51 ` [PATCH v3 12/14] iio: adc: max1027: Introduce an end of conversion helper Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  2021-09-18 17:09   ` Jonathan Cameron
  2021-09-15 15:51 ` [PATCH v3 14/14] iio: adc: max1027: Don't reject external triggers when there is no IRQ Miquel Raynal
  13 siblings, 1 reply; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, Miquel Raynal

The interrupt will fire upon end of conversion. This currently can
happen in three situations:
* a single read was requested and the data is ready
* the cnvst (internal) trigger was enabled and toggled
* an external trigger was enabled and toggled

So far, the driver only supported raw reads without involving the IRQ
and internal triggering. The internal trigger was actually the only
possible trigger, leading to shortcuts in the implementation.

In order to clarify the interrupt handling mechanism and extend the
software support to external triggers we must do all the following at
the same time:
* Create a hard IRQ handler only handling the EOC condition:
  In this handler, check if we are doing a raw read or a triggered
  read: maybe we just need to call complete() to unlock the waiting
  process, maybe we also need to push samples.
* Create a threaded IRQ handler only executed upon EOC condition only if
  the internal trigger is used: as said above, the goal of this threaded
  handler is to retrieve the data and push it to the buffers.
* Create another threaded IRQ handler that will be registered with
  devm_iio_triggered_buffer_setup(), in order to fully handle an
  external triggering event (start conversion, wait for EOC either by
  busy-waiting or with the completion object unlocked by the hard IRQ
  handler, retrieve the data, push it to the buffers).

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 | 90 +++++++++++++++++++++++++++++++--------
 1 file changed, 72 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index e0175448c899..9bf1c563042f 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -270,15 +270,26 @@ 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));
+		reinit_completion(&st->complete);
+		if (!ret)
+			return ret;
+	} else {
+		usleep_range(conversion_time, conversion_time * 2);
+	}
 
 	return 0;
 }
@@ -418,17 +429,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);
@@ -473,13 +473,67 @@ static int max1027_read_scan(struct iio_dev *indio_dev)
 	return 0;
 }
 
-static irqreturn_t max1027_trigger_handler(int irq, void *private)
+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_eoc_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct max1027_state *st = iio_priv(indio_dev);
+
+	/*
+	 * If the buffers are disabled (raw read) or an external trigger is
+	 * used, we just need to call complete() to unlock the waiters
+	 * which will themselves handle the data.
+	 */
+	if (!iio_buffer_enabled(indio_dev) ||
+	    !max1027_own_trigger_enabled(indio_dev)) {
+		complete(&st->complete);
+		return IRQ_HANDLED;
+	}
+
+	/*
+	 * When using the internal trigger, the data handling is done in
+	 * the threaded interrupt handler.
+	 */
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t max1027_int_trigger_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	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;
+}
+
+static irqreturn_t max1027_ext_trigger_handler(int irq, void *private)
 {
 	struct iio_poll_func *pf = private;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	int ret;
 
+	ret = max1027_configure_chans_and_start(indio_dev);
+	if (ret)
+		goto out;
+
+	ret = max1027_wait_eoc(indio_dev);
+	if (ret)
+		goto out;
+
 	ret = max1027_read_scan(indio_dev);
+out:
 	if (ret)
 		dev_err(&indio_dev->dev,
 			"Cannot read scanned values (%d)\n", ret);
@@ -496,7 +550,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,
 };
 
@@ -517,6 +570,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;
@@ -534,7 +588,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_ext_trigger_handler,
 						      NULL);
 		if (ret < 0) {
 			dev_err(&indio_dev->dev, "Failed to setup buffer\n");
@@ -561,11 +615,11 @@ static int max1027_probe(struct spi_device *spi)
 		}
 
 		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
-						iio_trigger_generic_data_rdy_poll,
-						NULL,
+						max1027_eoc_handler,
+						max1027_int_trigger_handler,
 						IRQF_TRIGGER_FALLING,
 						spi->dev.driver->name,
-						st->trig);
+						indio_dev);
 		if (ret < 0) {
 			dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n");
 			return ret;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 14/14] iio: adc: max1027: Don't reject external triggers when there is no IRQ
  2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (12 preceding siblings ...)
  2021-09-15 15:51 ` [PATCH v3 13/14] iio: adc: max1027: Deeply rework interrupt handling Miquel Raynal
@ 2021-09-15 15:51 ` Miquel Raynal
  13 siblings, 0 replies; 16+ messages in thread
From: Miquel Raynal @ 2021-09-15 15:51 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, 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 9bf1c563042f..42b76ac0e584 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -288,6 +288,9 @@ static int max1027_wait_eoc(struct iio_dev *indio_dev)
 		if (!ret)
 			return ret;
 	} else {
+		if (indio_dev->active_scan_mask)
+			conversion_time *= hweight32(*indio_dev->active_scan_mask);
+
 		usleep_range(conversion_time, conversion_time * 2);
 	}
 
@@ -585,16 +588,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_ext_trigger_handler,
+					      NULL);
+	if (ret < 0) {
+		dev_err(&indio_dev->dev, "Failed to setup buffer\n");
+		return ret;
+	}
+
+	/* 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_ext_trigger_handler,
-						      NULL);
-		if (ret < 0) {
-			dev_err(&indio_dev->dev, "Failed to setup buffer\n");
-			return ret;
-		}
-
 		st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-trigger",
 						  indio_dev->name);
 		if (!st->trig) {
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 13/14] iio: adc: max1027: Deeply rework interrupt handling
  2021-09-15 15:51 ` [PATCH v3 13/14] iio: adc: max1027: Deeply rework interrupt handling Miquel Raynal
@ 2021-09-18 17:09   ` Jonathan Cameron
  0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2021-09-18 17:09 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Lars-Peter Clausen, linux-iio, linux-kernel, Thomas Petazzoni, Nuno Sa

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

> The interrupt will fire upon end of conversion. This currently can
> happen in three situations:
> * a single read was requested and the data is ready
> * the cnvst (internal) trigger was enabled and toggled
> * an external trigger was enabled and toggled
> 
> So far, the driver only supported raw reads without involving the IRQ
> and internal triggering. The internal trigger was actually the only
> possible trigger, leading to shortcuts in the implementation.
> 
> In order to clarify the interrupt handling mechanism and extend the
> software support to external triggers we must do all the following at
> the same time:
> * Create a hard IRQ handler only handling the EOC condition:
>   In this handler, check if we are doing a raw read or a triggered
>   read: maybe we just need to call complete() to unlock the waiting
>   process, maybe we also need to push samples.

This doesn't sound quite right.  Should be either complete, or all iio_trigger_poll()
to tell any trigger consumers that the trigger has occured.

> * Create a threaded IRQ handler only executed upon EOC condition only if
>   the internal trigger is used: as said above, the goal of this threaded
>   handler is to retrieve the data and push it to the buffers.

Again, not quite right..

> * Create another threaded IRQ handler that will be registered with
>   devm_iio_triggered_buffer_setup(), in order to fully handle an
>   external triggering event (start conversion, wait for EOC either by
>   busy-waiting or with the completion object unlocked by the hard IRQ
>   handler, retrieve the data, push it to the buffers).
> 
> In order to authorize external triggers, we need to drop the
> ->validate_trigger() verification.  

I've tried to suggest how you need to change this to bring it inline
with the normal trigger / device split model of IIO.

Thanks,

Jonathan

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/iio/adc/max1027.c | 90 +++++++++++++++++++++++++++++++--------
>  1 file changed, 72 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index e0175448c899..9bf1c563042f 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -270,15 +270,26 @@ 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));
> +		reinit_completion(&st->complete);
> +		if (!ret)
> +			return ret;
> +	} else {
> +		usleep_range(conversion_time, conversion_time * 2);
> +	}
>  
>  	return 0;
>  }
> @@ -418,17 +429,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);
> @@ -473,13 +473,67 @@ static int max1027_read_scan(struct iio_dev *indio_dev)
>  	return 0;
>  }
>  
> -static irqreturn_t max1027_trigger_handler(int irq, void *private)
> +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_eoc_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct max1027_state *st = iio_priv(indio_dev);
> +
> +	/*
> +	 * If the buffers are disabled (raw read) or an external trigger is
> +	 * used, we just need to call complete() to unlock the waiters
> +	 * which will themselves handle the data.
> +	 */
> +	if (!iio_buffer_enabled(indio_dev) ||
> +	    !max1027_own_trigger_enabled(indio_dev)) {

This looks like what I'd expect here.  Should be able to use
!iio_trigger_using_own(indio_dev) for the second condition I think...


> +		complete(&st->complete);
> +		return IRQ_HANDLED;
> +	}

Here we should see the same as you find in the generic handler which is just

	iio_trigger_poll(private);

	return IRQ_HANDLED;

> +
> +	/*
> +	 * When using the internal trigger, the data handling is done in
> +	 * the threaded interrupt handler.

Wrong handler. It needs to be done in the one of the device side of the trigger / device split
not here which is on the trigger side.

> +	 */
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t max1027_int_trigger_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	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);

This is acknowledging the trigger in a patch not called via the trigger.
It might work but it definitely isn't the right model to use.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t max1027_ext_trigger_handler(int irq, void *private)
>  {
>  	struct iio_poll_func *pf = private;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	int ret;
> 

Here there should be a

	if (iio_trigger_using_own(indio_dev)) {

		/* Just read the data and push to the buffer as we know we are using the EOC trigger*/
		/* I think that will be what you have in max1027_int_trigger_handler above */
		/* You may also want to provide a top half for the trigger handler to grab a timestamp
		   nearer the point of the EOC interrupt for this path...
		*/

	} else {
		/* Start the capture and wait for completion */

		ret = max1027_configure_chans_and_start(indio_dev);
		if (ret)
			goto out;
	
		ret = max1027_wait_eoc(indio_dev);
		if (ret)
			goto out;

	 	ret = max1027_read_scan(indio_dev);
...		
	}

> +	ret = max1027_configure_chans_and_start(indio_dev);
> +	if (ret)
> +		goto out;
> +
> +	ret = max1027_wait_eoc(indio_dev);
> +	if (ret)
> +		goto out;
> +
>  	ret = max1027_read_scan(indio_dev);
> +out:
>  	if (ret)
>  		dev_err(&indio_dev->dev,
>  			"Cannot read scanned values (%d)\n", ret);
> @@ -496,7 +550,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,
>  };
>  
> @@ -517,6 +570,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;
> @@ -534,7 +588,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_ext_trigger_handler,

This isn't how this would normally be done.
Whatever trigger we are using, the handling should occur in the callback registered here.
We can do 'different' things depending on the trigger in use however.
The reason is that we want a model that allows us to use the EOC trigger for this device
and other devices at the same time.


>  						      NULL);
>  		if (ret < 0) {
>  			dev_err(&indio_dev->dev, "Failed to setup buffer\n");
> @@ -561,11 +615,11 @@ static int max1027_probe(struct spi_device *spi)
>  		}
>  
>  		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
> -						iio_trigger_generic_data_rdy_poll,
> -						NULL,
> +						max1027_eoc_handler,
> +						max1027_int_trigger_handler,
>  						IRQF_TRIGGER_FALLING,
>  						spi->dev.driver->name,
> -						st->trig);
> +						indio_dev);
>  		if (ret < 0) {
>  			dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n");
>  			return ret;


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-09-18 17:05 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-15 15:51 [PATCH v3 00/14] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 01/14] iio: adc: max1027: Fix style Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 02/14] iio: adc: max1027: Drop extra warning message Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 03/14] iio: adc: max1027: Drop useless debug messages Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 04/14] iio: adc: max1027: Minimize the number of converted channels Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 05/14] iio: adc: max1027: Rename a helper Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 06/14] iio: adc: max1027: Create a helper to enable/disable the cnvst trigger Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 07/14] iio: adc: max1027: Simplify the _set_trigger_state() helper Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 08/14] iio: adc: max1027: Ensure a default cnvst trigger configuration Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 09/14] iio: adc: max1027: Create a helper to configure the channels to scan Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 10/14] iio: adc: max1027: Prevent single channel accesses during buffer reads Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 11/14] iio: adc: max1027: Separate the IRQ handler from the read logic Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 12/14] iio: adc: max1027: Introduce an end of conversion helper Miquel Raynal
2021-09-15 15:51 ` [PATCH v3 13/14] iio: adc: max1027: Deeply rework interrupt handling Miquel Raynal
2021-09-18 17:09   ` Jonathan Cameron
2021-09-15 15:51 ` [PATCH v3 14/14] iio: adc: max1027: Don't reject external triggers when there is no IRQ Miquel Raynal

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