linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs
@ 2021-09-21 11:53 Miquel Raynal
  2021-09-21 11:53 ` [PATCH v4 01/16] iio: adc: max1027: Fix style Miquel Raynal
                   ` (16 more replies)
  0 siblings, 17 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:53 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 internal triggers. When the
hardware trigger is not wired it is very convenient to use external
triggers. 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 v4:
* Full rework again of the last few patches bringing external triggers
  support according to Jonathan's reviews: trigger handling (internal
  or external) should be in a dedicated helper to keep the exchanges
  between the IIO core and the drivers standards. This also improves
  reusability and now the max1027 trigger can also be used to trigger
  other IIO devices.

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 (16):
  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: Stop requesting a threaded IRQ
  iio: adc: max1027: Use the EOC IRQ when populated for single reads
  iio: adc: max1027: Allow all kind of triggers to be used
  iio: adc: max1027: Don't reject external triggers when there is no IRQ

 drivers/iio/adc/max1027.c | 286 +++++++++++++++++++++++++++-----------
 1 file changed, 205 insertions(+), 81 deletions(-)

-- 
2.27.0


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

* [PATCH v4 01/16] iio: adc: max1027: Fix style
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
@ 2021-09-21 11:53 ` Miquel Raynal
  2021-09-21 11:53 ` [PATCH v4 02/16] iio: adc: max1027: Drop extra warning message Miquel Raynal
                   ` (15 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:53 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	[flat|nested] 20+ messages in thread

* [PATCH v4 02/16] iio: adc: max1027: Drop extra warning message
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
  2021-09-21 11:53 ` [PATCH v4 01/16] iio: adc: max1027: Fix style Miquel Raynal
@ 2021-09-21 11:53 ` Miquel Raynal
  2021-09-21 11:53 ` [PATCH v4 03/16] iio: adc: max1027: Drop useless debug messages Miquel Raynal
                   ` (14 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:53 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	[flat|nested] 20+ messages in thread

* [PATCH v4 03/16] iio: adc: max1027: Drop useless debug messages
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
  2021-09-21 11:53 ` [PATCH v4 01/16] iio: adc: max1027: Fix style Miquel Raynal
  2021-09-21 11:53 ` [PATCH v4 02/16] iio: adc: max1027: Drop extra warning message Miquel Raynal
@ 2021-09-21 11:53 ` Miquel Raynal
  2021-09-21 11:53 ` [PATCH v4 04/16] iio: adc: max1027: Minimize the number of converted channels Miquel Raynal
                   ` (13 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:53 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	[flat|nested] 20+ messages in thread

* [PATCH v4 04/16] iio: adc: max1027: Minimize the number of converted channels
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (2 preceding siblings ...)
  2021-09-21 11:53 ` [PATCH v4 03/16] iio: adc: max1027: Drop useless debug messages Miquel Raynal
@ 2021-09-21 11:53 ` Miquel Raynal
  2021-09-21 11:53 ` [PATCH v4 05/16] iio: adc: max1027: Rename a helper Miquel Raynal
                   ` (12 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:53 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	[flat|nested] 20+ messages in thread

* [PATCH v4 05/16] iio: adc: max1027: Rename a helper
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (3 preceding siblings ...)
  2021-09-21 11:53 ` [PATCH v4 04/16] iio: adc: max1027: Minimize the number of converted channels Miquel Raynal
@ 2021-09-21 11:53 ` Miquel Raynal
  2021-09-21 11:53 ` [PATCH v4 06/16] iio: adc: max1027: Create a helper to enable/disable the cnvst trigger Miquel Raynal
                   ` (11 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:53 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	[flat|nested] 20+ messages in thread

* [PATCH v4 06/16] iio: adc: max1027: Create a helper to enable/disable the cnvst trigger
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (4 preceding siblings ...)
  2021-09-21 11:53 ` [PATCH v4 05/16] iio: adc: max1027: Rename a helper Miquel Raynal
@ 2021-09-21 11:53 ` Miquel Raynal
  2021-09-21 11:53 ` [PATCH v4 07/16] iio: adc: max1027: Simplify the _set_trigger_state() helper Miquel Raynal
                   ` (10 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:53 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	[flat|nested] 20+ messages in thread

* [PATCH v4 07/16] iio: adc: max1027: Simplify the _set_trigger_state() helper
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (5 preceding siblings ...)
  2021-09-21 11:53 ` [PATCH v4 06/16] iio: adc: max1027: Create a helper to enable/disable the cnvst trigger Miquel Raynal
@ 2021-09-21 11:53 ` Miquel Raynal
  2021-09-21 11:54 ` [PATCH v4 08/16] iio: adc: max1027: Ensure a default cnvst trigger configuration Miquel Raynal
                   ` (9 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:53 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	[flat|nested] 20+ messages in thread

* [PATCH v4 08/16] iio: adc: max1027: Ensure a default cnvst trigger configuration
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (6 preceding siblings ...)
  2021-09-21 11:53 ` [PATCH v4 07/16] iio: adc: max1027: Simplify the _set_trigger_state() helper Miquel Raynal
@ 2021-09-21 11:54 ` Miquel Raynal
  2021-09-21 11:54 ` [PATCH v4 09/16] iio: adc: max1027: Create a helper to configure the channels to scan Miquel Raynal
                   ` (8 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:54 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	[flat|nested] 20+ messages in thread

* [PATCH v4 09/16] iio: adc: max1027: Create a helper to configure the channels to scan
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (7 preceding siblings ...)
  2021-09-21 11:54 ` [PATCH v4 08/16] iio: adc: max1027: Ensure a default cnvst trigger configuration Miquel Raynal
@ 2021-09-21 11:54 ` Miquel Raynal
  2021-09-21 11:54 ` [PATCH v4 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads Miquel Raynal
                   ` (7 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:54 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	[flat|nested] 20+ messages in thread

* [PATCH v4 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (8 preceding siblings ...)
  2021-09-21 11:54 ` [PATCH v4 09/16] iio: adc: max1027: Create a helper to configure the channels to scan Miquel Raynal
@ 2021-09-21 11:54 ` Miquel Raynal
  2021-09-21 11:54 ` [PATCH v4 11/16] iio: adc: max1027: Separate the IRQ handler from the read logic Miquel Raynal
                   ` (6 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:54 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	[flat|nested] 20+ messages in thread

* [PATCH v4 11/16] iio: adc: max1027: Separate the IRQ handler from the read logic
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (9 preceding siblings ...)
  2021-09-21 11:54 ` [PATCH v4 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads Miquel Raynal
@ 2021-09-21 11:54 ` Miquel Raynal
  2021-09-21 11:54 ` [PATCH v4 12/16] iio: adc: max1027: Introduce an end of conversion helper Miquel Raynal
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:54 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	[flat|nested] 20+ messages in thread

* [PATCH v4 12/16] iio: adc: max1027: Introduce an end of conversion helper
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (10 preceding siblings ...)
  2021-09-21 11:54 ` [PATCH v4 11/16] iio: adc: max1027: Separate the IRQ handler from the read logic Miquel Raynal
@ 2021-09-21 11:54 ` Miquel Raynal
  2021-09-21 11:54 ` [PATCH v4 13/16] iio: adc: max1027: Stop requesting a threaded IRQ Miquel Raynal
                   ` (4 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:54 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.

In practice, a "temperature measurement" will take twice this
time because it will also carry another analog conversion but as here we
will only care about the temperature conversion which happens first, we
can still only wait for a single sample and get the right data.

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	[flat|nested] 20+ messages in thread

* [PATCH v4 13/16] iio: adc: max1027: Stop requesting a threaded IRQ
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (11 preceding siblings ...)
  2021-09-21 11:54 ` [PATCH v4 12/16] iio: adc: max1027: Introduce an end of conversion helper Miquel Raynal
@ 2021-09-21 11:54 ` Miquel Raynal
  2021-09-21 11:54 ` [PATCH v4 14/16] iio: adc: max1027: Use the EOC IRQ when populated for single reads Miquel Raynal
                   ` (3 subsequent siblings)
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:54 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, Miquel Raynal

The threaded handler is not populated, this means there is nothing
running in process context so let's switch to the regular
devm_request_irq() call instead.

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

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index e0175448c899..84217e18ef70 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -560,12 +560,10 @@ static int max1027_probe(struct spi_device *spi)
 			return ret;
 		}
 
-		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
-						iio_trigger_generic_data_rdy_poll,
-						NULL,
-						IRQF_TRIGGER_FALLING,
-						spi->dev.driver->name,
-						st->trig);
+		ret = devm_request_irq(&spi->dev, spi->irq,
+				       iio_trigger_generic_data_rdy_poll,
+				       IRQF_TRIGGER_FALLING,
+				       spi->dev.driver->name, st->trig);
 		if (ret < 0) {
 			dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n");
 			return ret;
-- 
2.27.0


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

* [PATCH v4 14/16] iio: adc: max1027: Use the EOC IRQ when populated for single reads
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (12 preceding siblings ...)
  2021-09-21 11:54 ` [PATCH v4 13/16] iio: adc: max1027: Stop requesting a threaded IRQ Miquel Raynal
@ 2021-09-21 11:54 ` Miquel Raynal
  2021-09-26 14:36   ` Jonathan Cameron
  2021-09-21 11:54 ` [PATCH v4 15/16] iio: adc: max1027: Allow all kind of triggers to be used Miquel Raynal
                   ` (2 subsequent siblings)
  16 siblings, 1 reply; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:54 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, Miquel Raynal

So far the End-Of-Conversion interrupt was only used in conjunction with
the internal trigger to process the data. Let's extend the use of this
interrupt handler to support regular single-shot conversions as well.

Doing so requires writing our own hard IRQ handler. This handler has to
check if buffers are enabled or not:

*** Buffers disabled condition ***

  This means the user requested a single conversion and the sample is
  ready to be retrieved.

    -> This implies adding the relevant completion boilerplate.

*** Buffers enabled condition ***

  Triggers are used. So far there is only support for the internal
  trigger but this trigger might soon be attached to another device as
  well so it is the core duty to decide which handler to call in order
  to process the data. The core will decide to either:

  * Call the internal trigger handler which will extract the data that
    is already present in the ADC FIFOs

  or

  * Call the trigger handler of another driver when using this trigger
    with another device, even though this call will be slightly delayed
    by the fact that the max1027 IRQ is a data-ready interrupt rather
    than a real trigger:

  -> The new handler will manually inform the core about the trigger
     having transitioned by directly calling iio_trigger_poll() (which
     iio_trigger_generic_data_rdy_poll() initially did).

In order for the handler to be "source" agnostic, we also need to change
the private pointer and provide the IIO device instead of the trigger
object.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---

Jonathan,

I hope this fits the IIO model now. In order to be sure I got the big
picture I first refused to look at your code snippets. Just with your
"plain english" explanations I wrote most of these three patches, before
checking back that they were indeed fully aligned with your examples. I
truly hope they do now, but do not hesitate if I missed something.

Cheers,
Miquèl

 drivers/iio/adc/max1027.c | 43 +++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 84217e18ef70..0fa7b0fbdba0 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;
 }
@@ -473,6 +484,30 @@ static int max1027_read_scan(struct iio_dev *indio_dev)
 	return 0;
 }
 
+static irqreturn_t max1027_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct max1027_state *st = iio_priv(indio_dev);
+
+	/*
+	 * If buffers are disabled (raw read), we just need to unlock the
+	 * waiters which will then handle the data.
+	 *
+	 * When using the internal trigger, we must hand-off the choice of the
+	 * handler to the core which will then lookup through the interrupt tree
+	 * for the right handler registered with iio_triggered_buffer_setup()
+	 * to execute, as this trigger might very well be used in conjunction
+	 * with another device. The core will then call the relevant handler to
+	 * perform the data processing step.
+	 */
+	if (!iio_buffer_enabled(indio_dev))
+		complete(&st->complete);
+	else
+		iio_trigger_poll(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t max1027_trigger_handler(int irq, void *private)
 {
 	struct iio_poll_func *pf = private;
@@ -517,6 +552,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;
@@ -560,10 +596,9 @@ static int max1027_probe(struct spi_device *spi)
 			return ret;
 		}
 
-		ret = devm_request_irq(&spi->dev, spi->irq,
-				       iio_trigger_generic_data_rdy_poll,
+		ret = devm_request_irq(&spi->dev, spi->irq, max1027_handler,
 				       IRQF_TRIGGER_FALLING,
-				       spi->dev.driver->name, st->trig);
+				       spi->dev.driver->name, indio_dev);
 		if (ret < 0) {
 			dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n");
 			return ret;
-- 
2.27.0


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

* [PATCH v4 15/16] iio: adc: max1027: Allow all kind of triggers to be used
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (13 preceding siblings ...)
  2021-09-21 11:54 ` [PATCH v4 14/16] iio: adc: max1027: Use the EOC IRQ when populated for single reads Miquel Raynal
@ 2021-09-21 11:54 ` Miquel Raynal
  2021-09-21 11:54 ` [PATCH v4 16/16] iio: adc: max1027: Don't reject external triggers when there is no IRQ Miquel Raynal
  2021-09-26 14:38 ` [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Jonathan Cameron
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:54 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel
  Cc: Thomas Petazzoni, Nuno Sa, Miquel Raynal

There is no reason to limit this driver to its internal trigger. The
only difference being, when using an external trigger, the sample
conversion must be manually started.

Drop the ->validate_trigger() hook in order to allow other triggers to
be bound.

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

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 0fa7b0fbdba0..02735c604f82 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -429,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);
@@ -490,8 +479,8 @@ static irqreturn_t max1027_handler(int irq, void *private)
 	struct max1027_state *st = iio_priv(indio_dev);
 
 	/*
-	 * If buffers are disabled (raw read), we just need to unlock the
-	 * waiters which will then handle the data.
+	 * If buffers are disabled (raw read) or when using external triggers,
+	 * we just need to unlock the waiters which will then handle the data.
 	 *
 	 * When using the internal trigger, we must hand-off the choice of the
 	 * handler to the core which will then lookup through the interrupt tree
@@ -514,7 +503,19 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	int ret;
 
+	if (!iio_trigger_using_own(indio_dev)) {
+		ret = max1027_configure_chans_and_start(indio_dev);
+		if (ret)
+			goto out;
+
+		/* This is a threaded handler, it is fine to wait for an IRQ */
+		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);
@@ -531,7 +532,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


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

* [PATCH v4 16/16] iio: adc: max1027: Don't reject external triggers when there is no IRQ
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (14 preceding siblings ...)
  2021-09-21 11:54 ` [PATCH v4 15/16] iio: adc: max1027: Allow all kind of triggers to be used Miquel Raynal
@ 2021-09-21 11:54 ` Miquel Raynal
  2021-09-26 14:38 ` [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Jonathan Cameron
  16 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-21 11:54 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 02735c604f82..45dc8a625fa3 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);
 	}
 
@@ -567,16 +570,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_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 cnvst hardware trigger */
 	if (spi->irq) {
-		ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
-						      &iio_pollfunc_store_time,
-						      &max1027_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	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 14/16] iio: adc: max1027: Use the EOC IRQ when populated for single reads
  2021-09-21 11:54 ` [PATCH v4 14/16] iio: adc: max1027: Use the EOC IRQ when populated for single reads Miquel Raynal
@ 2021-09-26 14:36   ` Jonathan Cameron
  2021-09-27 14:30     ` Miquel Raynal
  0 siblings, 1 reply; 20+ messages in thread
From: Jonathan Cameron @ 2021-09-26 14:36 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Lars-Peter Clausen, linux-iio, linux-kernel, Thomas Petazzoni, Nuno Sa

On Tue, 21 Sep 2021 13:54:06 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> So far the End-Of-Conversion interrupt was only used in conjunction with
> the internal trigger to process the data. Let's extend the use of this
> interrupt handler to support regular single-shot conversions as well.
> 
> Doing so requires writing our own hard IRQ handler. This handler has to
> check if buffers are enabled or not:
> 
> *** Buffers disabled condition ***
> 
>   This means the user requested a single conversion and the sample is
>   ready to be retrieved.
> 
>     -> This implies adding the relevant completion boilerplate.  
> 
> *** Buffers enabled condition ***
> 
>   Triggers are used. So far there is only support for the internal
>   trigger but this trigger might soon be attached to another device as
>   well so it is the core duty to decide which handler to call in order
>   to process the data. The core will decide to either:
> 
>   * Call the internal trigger handler which will extract the data that
>     is already present in the ADC FIFOs
> 
>   or
> 
>   * Call the trigger handler of another driver when using this trigger
>     with another device, even though this call will be slightly delayed
>     by the fact that the max1027 IRQ is a data-ready interrupt rather
>     than a real trigger:
> 
>   -> The new handler will manually inform the core about the trigger  
>      having transitioned by directly calling iio_trigger_poll() (which
>      iio_trigger_generic_data_rdy_poll() initially did).
> 
> In order for the handler to be "source" agnostic, we also need to change
> the private pointer and provide the IIO device instead of the trigger
> object.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> 
> Jonathan,
> 
> I hope this fits the IIO model now. In order to be sure I got the big
> picture I first refused to look at your code snippets. Just with your
> "plain english" explanations I wrote most of these three patches, before
> checking back that they were indeed fully aligned with your examples. I
> truly hope they do now, but do not hesitate if I missed something.

Looks great to me (in fact I just applied it but I'll reply to the cover letter
shortly for that).

Thanks for persisting with this and I'm looking forward to that blog you
mentioned.  If you have time / inclination to help improve the documentation
in the kernel tree that would also be great.  This discussion has made it
clear to me that it would be great to have a set of 'patterns' for common
types of device + how we map them onto the model of IIO (particularly
when they don't quite fit that idealised model).  There are similar
compromises around when to use multiple buffers for instance.

It is always on the list of things to work on but somehow there is always
something else more urgent :(


Thanks,

Jonathan

> 
> Cheers,
> Miquèl
> 
>  drivers/iio/adc/max1027.c | 43 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 84217e18ef70..0fa7b0fbdba0 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;
>  }
> @@ -473,6 +484,30 @@ static int max1027_read_scan(struct iio_dev *indio_dev)
>  	return 0;
>  }
>  
> +static irqreturn_t max1027_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct max1027_state *st = iio_priv(indio_dev);
> +
> +	/*
> +	 * If buffers are disabled (raw read), we just need to unlock the
> +	 * waiters which will then handle the data.
> +	 *
> +	 * When using the internal trigger, we must hand-off the choice of the
> +	 * handler to the core which will then lookup through the interrupt tree
> +	 * for the right handler registered with iio_triggered_buffer_setup()
> +	 * to execute, as this trigger might very well be used in conjunction
> +	 * with another device. The core will then call the relevant handler to
> +	 * perform the data processing step.
> +	 */
> +	if (!iio_buffer_enabled(indio_dev))
> +		complete(&st->complete);
> +	else
> +		iio_trigger_poll(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t max1027_trigger_handler(int irq, void *private)
>  {
>  	struct iio_poll_func *pf = private;
> @@ -517,6 +552,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;
> @@ -560,10 +596,9 @@ static int max1027_probe(struct spi_device *spi)
>  			return ret;
>  		}
>  
> -		ret = devm_request_irq(&spi->dev, spi->irq,
> -				       iio_trigger_generic_data_rdy_poll,
> +		ret = devm_request_irq(&spi->dev, spi->irq, max1027_handler,
>  				       IRQF_TRIGGER_FALLING,
> -				       spi->dev.driver->name, st->trig);
> +				       spi->dev.driver->name, indio_dev);
>  		if (ret < 0) {
>  			dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n");
>  			return ret;


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

* Re: [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs
  2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
                   ` (15 preceding siblings ...)
  2021-09-21 11:54 ` [PATCH v4 16/16] iio: adc: max1027: Don't reject external triggers when there is no IRQ Miquel Raynal
@ 2021-09-26 14:38 ` Jonathan Cameron
  16 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2021-09-26 14:38 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Lars-Peter Clausen, linux-iio, linux-kernel, Thomas Petazzoni, Nuno Sa

On Tue, 21 Sep 2021 13:53:52 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Until now the max1027.c driver, which handles 10-bit devices (max10xx)
> and 12-bit devices (max12xx), only supported internal triggers. When the
> hardware trigger is not wired it is very convenient to use external
> triggers. 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
> 

Series applied to the togreg branch of iio.git and pushed out as testing
to see if 0-day can find anything we missed.

Thanks for persisting on this one.

Jonathan

> Cheers,
> Miquèl
> 
> Changes in v4:
> * Full rework again of the last few patches bringing external triggers
>   support according to Jonathan's reviews: trigger handling (internal
>   or external) should be in a dedicated helper to keep the exchanges
>   between the IIO core and the drivers standards. This also improves
>   reusability and now the max1027 trigger can also be used to trigger
>   other IIO devices.
> 
> 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 (16):
>   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: Stop requesting a threaded IRQ
>   iio: adc: max1027: Use the EOC IRQ when populated for single reads
>   iio: adc: max1027: Allow all kind of triggers to be used
>   iio: adc: max1027: Don't reject external triggers when there is no IRQ
> 
>  drivers/iio/adc/max1027.c | 286 +++++++++++++++++++++++++++-----------
>  1 file changed, 205 insertions(+), 81 deletions(-)
> 


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

* Re: [PATCH v4 14/16] iio: adc: max1027: Use the EOC IRQ when populated for single reads
  2021-09-26 14:36   ` Jonathan Cameron
@ 2021-09-27 14:30     ` Miquel Raynal
  0 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2021-09-27 14:30 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Lars-Peter Clausen, linux-iio, linux-kernel, Thomas Petazzoni, Nuno Sa

Hi Jonathan,

jic23@kernel.org wrote on Sun, 26 Sep 2021 15:36:59 +0100:

> On Tue, 21 Sep 2021 13:54:06 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > So far the End-Of-Conversion interrupt was only used in conjunction with
> > the internal trigger to process the data. Let's extend the use of this
> > interrupt handler to support regular single-shot conversions as well.
> > 
> > Doing so requires writing our own hard IRQ handler. This handler has to
> > check if buffers are enabled or not:
> > 
> > *** Buffers disabled condition ***
> > 
> >   This means the user requested a single conversion and the sample is
> >   ready to be retrieved.
> >   
> >     -> This implies adding the relevant completion boilerplate.    
> > 
> > *** Buffers enabled condition ***
> > 
> >   Triggers are used. So far there is only support for the internal
> >   trigger but this trigger might soon be attached to another device as
> >   well so it is the core duty to decide which handler to call in order
> >   to process the data. The core will decide to either:
> > 
> >   * Call the internal trigger handler which will extract the data that
> >     is already present in the ADC FIFOs
> > 
> >   or
> > 
> >   * Call the trigger handler of another driver when using this trigger
> >     with another device, even though this call will be slightly delayed
> >     by the fact that the max1027 IRQ is a data-ready interrupt rather
> >     than a real trigger:
> >   
> >   -> The new handler will manually inform the core about the trigger    
> >      having transitioned by directly calling iio_trigger_poll() (which
> >      iio_trigger_generic_data_rdy_poll() initially did).
> > 
> > In order for the handler to be "source" agnostic, we also need to change
> > the private pointer and provide the IIO device instead of the trigger
> > object.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> > 
> > Jonathan,
> > 
> > I hope this fits the IIO model now. In order to be sure I got the big
> > picture I first refused to look at your code snippets. Just with your
> > "plain english" explanations I wrote most of these three patches, before
> > checking back that they were indeed fully aligned with your examples. I
> > truly hope they do now, but do not hesitate if I missed something.  
> 
> Looks great to me (in fact I just applied it but I'll reply to the cover letter
> shortly for that).

Great, thanks!

> Thanks for persisting with this and I'm looking forward to that blog you
> mentioned.

Thank you for all the time you put in these reviews.

I wrote most of it, it's not very long but I tried to cover most of the
common interactions with the core. It is under internal review, I'll
certainly show you its content before publishing in case you have
comments or spot any mistake.

>  If you have time / inclination to help improve the documentation
> in the kernel tree that would also be great.  This discussion has made it
> clear to me that it would be great to have a set of 'patterns' for common
> types of device + how we map them onto the model of IIO (particularly
> when they don't quite fit that idealised model).  There are similar
> compromises around when to use multiple buffers for instance.
> 
> It is always on the list of things to work on but somehow there is always
> something else more urgent :(

Hehe :) I know, documentation is always a good-to-have item that is not
often reached... I feel I need more occasions to work in the IIO
area (for example with hardware fifos/buffers or events) before being
able to fully appreciate the internal design, but I agree having a set
of 'usual patterns' that drivers should conform with depending on the
devices capabilities/constraints would be a must.

In the mean time, let's start by clarifying this 'iio_dev->modes'
property :)

Thanks,
Miquèl

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

end of thread, other threads:[~2021-09-27 14:30 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-21 11:53 [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 01/16] iio: adc: max1027: Fix style Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 02/16] iio: adc: max1027: Drop extra warning message Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 03/16] iio: adc: max1027: Drop useless debug messages Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 04/16] iio: adc: max1027: Minimize the number of converted channels Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 05/16] iio: adc: max1027: Rename a helper Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 06/16] iio: adc: max1027: Create a helper to enable/disable the cnvst trigger Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 07/16] iio: adc: max1027: Simplify the _set_trigger_state() helper Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 08/16] iio: adc: max1027: Ensure a default cnvst trigger configuration Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 09/16] iio: adc: max1027: Create a helper to configure the channels to scan Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 11/16] iio: adc: max1027: Separate the IRQ handler from the read logic Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 12/16] iio: adc: max1027: Introduce an end of conversion helper Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 13/16] iio: adc: max1027: Stop requesting a threaded IRQ Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 14/16] iio: adc: max1027: Use the EOC IRQ when populated for single reads Miquel Raynal
2021-09-26 14:36   ` Jonathan Cameron
2021-09-27 14:30     ` Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 15/16] iio: adc: max1027: Allow all kind of triggers to be used Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 16/16] iio: adc: max1027: Don't reject external triggers when there is no IRQ Miquel Raynal
2021-09-26 14:38 ` [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs Jonathan Cameron

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).