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 starts with two small fixes, then does a bit of
cleaning/code reorganization before actually adding support for software
triggers.
This series has been developed and tested on a custom board with a 4.14
kernel. I then rebased the series on top of a mainline kernel
(v5.14-rc1) but unfortunately after quite some time debugging it I was
unable to get all the necessary blocks running in order to properly test
it. Anyway, there was very little changes in that series when rebasing
it from v4.14 to v5.14-rc1 so I am pretty confident it will smoothly
work with a more recent kernel.
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
Miquel Raynal (16):
iio: adc: max1027: Fix wrong shift with 12-bit devices
iio: adc: max1027: Fix the number of max1X31 channels
iio: adc: max1027: Push only the requested samples
iio: adc: max1027: Lower conversion time
iio: adc: max1027: Drop extra warning message
iio: adc: max1027: Rename a helper
iio: adc: max1027: Create a helper to configure the trigger
iio: adc: max1027: Explain better how the trigger state gets changed
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: Prepare re-using the EOC interrupt
iio: adc: max1027: Consolidate the end of conversion helper
iio: adc: max1027: Support software triggers
iio: adc: max1027: Enable software triggers to be used without IRQ
drivers/iio/adc/max1027.c | 236 ++++++++++++++++++++++++++++----------
1 file changed, 177 insertions(+), 59 deletions(-)
--
2.27.0
When a triggered scan occurs, the identity of the desired channels is
known in indio_dev->active_scan_mask. Instead of reading and pushing to
the IIO buffers all channels each time, scan the minimum amount of
channels (0 to maximum requested chan, to be exact) and only provide the
samples requested by the user.
For example, if the user wants channels 1, 4 and 5, all channels from
0 to 5 will be scanned but only the desired channels will be pushed to
the IIO buffers.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index b753658bb41e..8ab660f596b5 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -360,6 +360,9 @@ static int max1027_set_trigger_state(struct iio_trigger *trig, bool state)
struct max1027_state *st = iio_priv(indio_dev);
int ret;
+ if (bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength))
+ return -EINVAL;
+
if (state) {
/* Start acquisition on cnvst */
st->reg = MAX1027_SETUP_REG | MAX1027_CKS_MODE0 |
@@ -368,9 +371,12 @@ 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 0 to the highest requested channel. The temperature
+ * could be avoided but it simplifies a bit the logic.
+ */
+ st->reg = MAX1027_CONV_REG | MAX1027_SCAN_0_N | MAX1027_TEMP;
+ st->reg |= MAX1027_CHAN(fls(*indio_dev->active_scan_mask) - 2);
ret = spi_write(st->spi, &st->reg, 1);
if (ret < 0)
return ret;
@@ -391,11 +397,22 @@ 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 = fls(*indio_dev->active_scan_mask);
+ u16 *buf = st->buffer;
+ unsigned int bit;
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);
+ spi_read(st->spi, st->buffer, scanned_chans * 2);
+
+ /* Only keep the channels selected by the user */
+ for_each_set_bit(bit, indio_dev->active_scan_mask,
+ indio_dev->masklength) {
+ if (buf[0] != st->buffer[bit])
+ buf[0] = st->buffer[bit];
+ buf++;
+ }
iio_push_to_buffers(indio_dev, st->buffer);
--
2.27.0
When only a few channels are requested, the core will request all of
them as long as ->available_scan_masks is present, thus reducing the
impact of any driver side optimization to avoid converting the unneeded
channels.
Do not share this bitmap with the core, which is optional anyway.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/iio/adc/max1027.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 8ab660f596b5..d79dabf20567 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -457,7 +457,6 @@ static int max1027_probe(struct spi_device *spi)
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = st->info->channels;
indio_dev->num_channels = st->info->num_channels;
- indio_dev->available_scan_masks = st->info->available_scan_masks;
st->buffer = devm_kmalloc_array(&indio_dev->dev,
indio_dev->num_channels, 2,
--
2.27.0
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 <[email protected]>
---
drivers/iio/adc/max1027.c | 52 ++++++++++++++++++++++++++-------------
1 file changed, 35 insertions(+), 17 deletions(-)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index a6ebc951c09a..59914fcfd320 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -232,10 +232,37 @@ struct max1027_state {
struct iio_trigger *trig;
__be16 *buffer;
struct mutex lock;
-
+ bool cnvst_trigger;
u8 reg ____cacheline_aligned;
};
+static int max1027_configure_trigger(struct iio_dev *indio_dev)
+{
+ struct max1027_state *st = iio_priv(indio_dev);
+ int ret;
+
+ 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 (st->cnvst_trigger)
+ st->reg |= MAX1027_CKS_MODE0;
+ else
+ st->reg |= MAX1027_CKS_MODE2;
+
+ ret = spi_write(st->spi, &st->reg, 1);
+ if (ret < 0) {
+ dev_err(&indio_dev->dev,
+ "Failed to configure register (%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
static int max1027_read_single_value(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val)
@@ -248,14 +275,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_configure_trigger(indio_dev);
+ if (ret)
return ret;
- }
/* Configure conversion register with the requested chan */
st->reg = MAX1027_CONV_REG | MAX1027_CHAN(chan->channel) |
@@ -363,12 +385,10 @@ static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
if (bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength))
return -EINVAL;
+ st->cnvst_trigger = state;
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_configure_trigger(indio_dev);
+ if (ret)
return ret;
/*
@@ -382,10 +402,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_configure_trigger(indio_dev);
+ if (ret)
return ret;
}
--
2.27.0
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 <[email protected]>
---
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 ac603b4ca787..a6ebc951c09a 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -354,7 +354,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);
@@ -423,7 +423,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
Memory allocation errors automatically trigger the right logs, no need
to have our own.
Signed-off-by: Miquel Raynal <[email protected]>
---
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 d79dabf20567..ac603b4ca787 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -461,10 +461,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 == NULL) {
- 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
These bits are meant to be reused for triggered buffers setup.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/iio/adc/max1027.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 4a78c9cbc339..223c9e4abd86 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -236,6 +236,22 @@ struct max1027_state {
u8 reg ____cacheline_aligned;
};
+/* Scan from 0 to the highest requested channel */
+static int max1027_configure_chans_to_scan(struct iio_dev *indio_dev)
+{
+ struct max1027_state *st = iio_priv(indio_dev);
+ int ret;
+
+ /* The temperature could be avoided but it simplifies a bit the logic */
+ st->reg = MAX1027_CONV_REG | MAX1027_SCAN_0_N | MAX1027_TEMP;
+ st->reg |= MAX1027_CHAN(fls(*indio_dev->active_scan_mask) - 2);
+ ret = spi_write(st->spi, &st->reg, 1);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
static int max1027_configure_trigger(struct iio_dev *indio_dev)
{
struct max1027_state *st = iio_priv(indio_dev);
@@ -391,14 +407,8 @@ static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
if (ret)
return ret;
- /*
- * Scan from 0 to the highest requested channel. The temperature
- * could be avoided but it simplifies a bit the logic.
- */
- st->reg = MAX1027_CONV_REG | MAX1027_SCAN_0_N | MAX1027_TEMP;
- st->reg |= MAX1027_CHAN(fls(*indio_dev->active_scan_mask) - 2);
- ret = spi_write(st->spi, &st->reg, 1);
- if (ret < 0)
+ ret = max1027_configure_chans_to_scan(indio_dev);
+ if (ret)
return ret;
} else {
/*
--
2.27.0
It may appear strange to configure the trigger to start upon register
write while we want to disable the trigger. Explain why we do this in
the comment.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/iio/adc/max1027.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 59914fcfd320..4a78c9cbc339 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -401,7 +401,12 @@ static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
if (ret < 0)
return ret;
} else {
- /* Start acquisition on conversion register write */
+ /*
+ * 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_configure_trigger(indio_dev);
if (ret)
return ret;
--
2.27.0
Now that max1027_trigger_handler() has been freed from handling hardware
triggers EOC situations, we can use it for what it has been designed in
the first place: trigger software originated conversions. In other
words, when userspace initiates a conversion with a sysfs trigger or a
hrtimer trigger, we must do all configuration steps, ie:
1- Configuring the trigger
2- Configuring the channels to scan
3- Starting the conversion (actually done automatically by step 2 in
this case)
4- Waiting for the conversion to end
5- Retrieving the data from the ADC
6- Push the data to the IIO core and notify it
Add the missing steps to this helper and drop the trigger verification
hook otherwise software triggers would simply not be accepted at all.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/iio/adc/max1027.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 8c5995ae59f2..bb437e43adaf 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -413,17 +413,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);
@@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private)
pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private);
+ ret = max1027_configure_trigger(indio_dev);
+ if (ret)
+ goto out;
+
+ ret = max1027_configure_chans_to_scan(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);
@@ -529,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
Right now the driver only has hardware trigger support, which makes use
of the End Of Conversion (EOC) interrupt by:
- Enabling the external trigger
- At completion of the conversion, run a generic handler
- Push the data to the IIO subsystem by using the triggered buffer
infrastructure, which may not only serve this purpose.
In fact, the interrupt will fire for more reasons than just a hardware
trigger condition, so make a dedicated interrupt handler which will be
enhanced by the upcoming changes to handle more situations.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/iio/adc/max1027.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 2d6485591761..8d86e77fb5db 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -472,6 +472,24 @@ static int max1027_read_scan(struct iio_dev *indio_dev)
return 0;
}
+static irqreturn_t max1027_eoc_irq_handler(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct max1027_state *st = iio_priv(indio_dev);
+ int ret = 0;
+
+ if (st->cnvst_trigger) {
+ ret = max1027_read_scan(indio_dev);
+ iio_trigger_notify_done(indio_dev->trig);
+ }
+
+ if (ret)
+ dev_err(&indio_dev->dev,
+ "Cannot read scanned values (%d)\n", ret);
+
+ return IRQ_HANDLED;
+}
+
static irqreturn_t max1027_trigger_handler(int irq, void *private)
{
struct iio_poll_func *pf = private;
@@ -563,11 +581,11 @@ static int max1027_probe(struct spi_device *spi)
}
ret = devm_request_threaded_irq(&spi->dev, spi->irq,
- iio_trigger_generic_data_rdy_poll,
+ max1027_eoc_irq_handler,
NULL,
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
Software triggers do not need a device IRQ to work. As opposed to
hardware triggers which need it to yield the data to the IIO core,
software triggers run a dedicated thread which does all the tasks on
their behalf. Then, 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.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/iio/adc/max1027.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index bb437e43adaf..e767437a578e 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -567,16 +567,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, enable the hardware trigger (cnvst) */
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 == NULL) {
--
2.27.0
Now that we have a dedicated handler for End Of Conversion interrupts,
let's create a second path:
- Situation 1: we are using the external hardware trigger, a conversion
has been triggered and the ADC pushed the data to its FIFO, we need to
retrieve the data and push it to the IIO buffers.
- Situation 2: we are not using the external hardware trigger, hence we
are likely waiting in a blocked thread waiting for this interrupt to
happen: in this case we just wake up the waiting thread.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/iio/adc/max1027.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 8d86e77fb5db..8c5995ae59f2 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -235,6 +235,7 @@ struct max1027_state {
struct iio_trigger *trig;
__be16 *buffer;
struct mutex lock;
+ bool data_rdy;
bool cnvst_trigger;
u8 reg ____cacheline_aligned;
};
@@ -243,12 +244,22 @@ static DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
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;
- if (indio_dev->active_scan_mask)
- conversion_time *= hweight32(*indio_dev->active_scan_mask);
+ if (st->spi->irq) {
+ ret = wait_event_interruptible_timeout(max1027_queue,
+ st->data_rdy, HZ / 1000);
+ st->data_rdy = false;
+ if (ret == -ERESTARTSYS)
+ return ret;
+ } else {
+ if (indio_dev->active_scan_mask)
+ conversion_time *= hweight32(*indio_dev->active_scan_mask);
- usleep_range(conversion_time, conversion_time * 2);
+ usleep_range(conversion_time, conversion_time * 2);
+ }
return 0;
}
@@ -481,6 +492,9 @@ static irqreturn_t max1027_eoc_irq_handler(int irq, void *private)
if (st->cnvst_trigger) {
ret = max1027_read_scan(indio_dev);
iio_trigger_notify_done(indio_dev->trig);
+ } else {
+ st->data_rdy = true;
+ wake_up(&max1027_queue);
}
if (ret)
--
2.27.0
Create a max1027_read_scan() helper which will make clearer the future IRQ
handler updates (no functional change).
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/iio/adc/max1027.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 83526f3d7d3a..afc3ce69f7ea 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -427,19 +427,18 @@ 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 = fls(*indio_dev->active_scan_mask);
u16 *buf = st->buffer;
unsigned int bit;
-
- pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private);
+ int ret;
/* 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;
/* Only keep the channels selected by the user */
for_each_set_bit(bit, indio_dev->active_scan_mask,
@@ -451,6 +450,22 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private)
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;
+
+ pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private);
+
+ 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
For now this helper only waits for the maximum duration of a conversion,
but it will soon be improved to properly handle the end of conversion
interrupt.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/iio/adc/max1027.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index afc3ce69f7ea..2d6485591761 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,
@@ -236,6 +239,20 @@ struct max1027_state {
u8 reg ____cacheline_aligned;
};
+static DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
+
+static int max1027_wait_eoc(struct iio_dev *indio_dev)
+{
+ unsigned int conversion_time = MAX1027_CONVERSION_UDELAY;
+
+ if (indio_dev->active_scan_mask)
+ conversion_time *= hweight32(*indio_dev->active_scan_mask);
+
+ usleep_range(conversion_time, conversion_time * 2);
+
+ return 0;
+}
+
/* Scan from 0 to the highest requested channel */
static int max1027_configure_chans_to_scan(struct iio_dev *indio_dev)
{
@@ -310,9 +327,11 @@ static int max1027_read_single_value(struct iio_dev *indio_dev,
/*
* For an unknown reason, when we use the mode "10" (write
* conversion register), the interrupt doesn't occur every time.
- * So we just wait 1 ms.
+ * So we just wait the maximum conversion time and deliver the value.
*/
- mdelay(1);
+ ret = max1027_wait_eoc(indio_dev);
+ if (ret)
+ return ret;
/* Read result */
ret = spi_read(st->spi, st->buffer, (chan->type == IIO_TEMP) ? 4 : 2);
--
2.27.0
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. Prevent it with a iio_buffer_enabled() check.
Signed-off-by: Miquel Raynal <[email protected]>
---
drivers/iio/adc/max1027.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 223c9e4abd86..83526f3d7d3a 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -335,6 +335,8 @@ static int max1027_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
+ if (iio_buffer_enabled(indio_dev))
+ return -EBUSY;
ret = max1027_read_single_value(indio_dev, chan, val);
break;
case IIO_CHAN_INFO_SCALE:
--
2.27.0
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:11 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 03/16] iio: adc: max1027: Push only the requested
> samples
>
> [External]
>
> When a triggered scan occurs, the identity of the desired channels is
> known in indio_dev->active_scan_mask. Instead of reading and
> pushing to
> the IIO buffers all channels each time, scan the minimum amount of
> channels (0 to maximum requested chan, to be exact) and only
> provide the
> samples requested by the user.
>
> For example, if the user wants channels 1, 4 and 5, all channels from
> 0 to 5 will be scanned but only the desired channels will be pushed to
> the IIO buffers.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index b753658bb41e..8ab660f596b5 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -360,6 +360,9 @@ static int max1027_set_trigger_state(struct
> iio_trigger *trig, bool state)
> struct max1027_state *st = iio_priv(indio_dev);
> int ret;
>
> + if (bitmap_empty(indio_dev->active_scan_mask, indio_dev-
> >masklength))
> + return -EINVAL;
> +
I'm not sure this can actually happen. If you try to enable the buffer
with no scan element, it should give you an error before you reach
this point...
> if (state) {
> /* Start acquisition on cnvst */
> st->reg = MAX1027_SETUP_REG |
> MAX1027_CKS_MODE0 |
> @@ -368,9 +371,12 @@ 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 0 to the highest requested channel. The
> temperature
> + * could be avoided but it simplifies a bit the logic.
> + */
> + st->reg = MAX1027_CONV_REG |
> MAX1027_SCAN_0_N | MAX1027_TEMP;
> + st->reg |= MAX1027_CHAN(fls(*indio_dev-
> >active_scan_mask) - 2);
> ret = spi_write(st->spi, &st->reg, 1);
> if (ret < 0)
> return ret;
> @@ -391,11 +397,22 @@ 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 = fls(*indio_dev-
> >active_scan_mask);
> + u16 *buf = st->buffer;
I think sparse will complain here. buffer is a __be16 restricted
type so you should not mix those...
> + unsigned int bit;
>
> 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);
> + spi_read(st->spi, st->buffer, scanned_chans * 2);
> +
> + /* Only keep the channels selected by the user */
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> + if (buf[0] != st->buffer[bit])
> + buf[0] = st->buffer[bit];
Since we are here, when looking into the driver, I realized
that st->buffer is not DMA safe. In IIO, we kind of want to enforce
that all buffers that are passed to spi/i2c buses are safe... Maybe
this is something you can include in your series.
- Nuno Sá
> + buf++;
> + }
>
> iio_push_to_buffers(indio_dev, st->buffer);
>
> --
> 2.27.0
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:11 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 04/16] iio: adc: max1027: Lower conversion time
>
> [External]
>
> When only a few channels are requested, the core will request all of
> them as long as ->available_scan_masks is present, thus reducing the
> impact of any driver side optimization to avoid converting the
> unneeded
> channels.
>
> Do not share this bitmap with the core, which is optional anyway.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 8ab660f596b5..d79dabf20567 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -457,7 +457,6 @@ static int max1027_probe(struct spi_device
> *spi)
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->channels = st->info->channels;
> indio_dev->num_channels = st->info->num_channels;
> - indio_dev->available_scan_masks = st->info-
> >available_scan_masks;
>
> st->buffer = devm_kmalloc_array(&indio_dev->dev,
> indio_dev->num_channels, 2,
> --
> 2.27.0
Reviewed-by: Nuno Sá <[email protected]>
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:11 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 06/16] iio: adc: max1027: Rename a helper
>
> [External]
>
> 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 <[email protected]>
> ---
> 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 ac603b4ca787..a6ebc951c09a 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -354,7 +354,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);
> @@ -423,7 +423,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
Reviewed-by: Nuno Sá <[email protected]>
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:11 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 05/16] iio: adc: max1027: Drop extra warning message
>
> [External]
>
> Memory allocation errors automatically trigger the right logs, no need
> to have our own.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> 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 d79dabf20567..ac603b4ca787 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -461,10 +461,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 == NULL) {
> - 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
Reviewed-by: Nuno Sá <[email protected]>
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:12 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 07/16] iio: adc: max1027: Create a helper to configure
> the trigger
>
> [External]
>
> 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 <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 52 ++++++++++++++++++++++++++------
> -------
> 1 file changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index a6ebc951c09a..59914fcfd320 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -232,10 +232,37 @@ struct max1027_state {
> struct iio_trigger *trig;
> __be16 *buffer;
> struct mutex lock;
> -
> + bool cnvst_trigger;
> u8 reg ____cacheline_aligned;
> };
>
> +static int max1027_configure_trigger(struct iio_dev *indio_dev)
> +{
> + struct max1027_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + 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 (st->cnvst_trigger)
> + st->reg |= MAX1027_CKS_MODE0;
> + else
> + st->reg |= MAX1027_CKS_MODE2;
> +
> + ret = spi_write(st->spi, &st->reg, 1);
> + if (ret < 0) {
> + dev_err(&indio_dev->dev,
> + "Failed to configure register (%d)\n", ret);
> + return ret;
> + }
I do not think the error message is so important here that we cannot
make this more simple and just do:
return spi_write(st->spi, &st->reg, 1);
Anyways, not that important so:
Reviewed-by: Nuno Sá <[email protected]>
> + return 0;
> +}
> +
> static int max1027_read_single_value(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val)
> @@ -248,14 +275,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_configure_trigger(indio_dev);
> + if (ret)
> return ret;
> - }
>
> /* Configure conversion register with the requested chan */
> st->reg = MAX1027_CONV_REG | MAX1027_CHAN(chan-
> >channel) |
> @@ -363,12 +385,10 @@ static int
> max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
> if (bitmap_empty(indio_dev->active_scan_mask, indio_dev-
> >masklength))
> return -EINVAL;
>
> + st->cnvst_trigger = state;
> 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_configure_trigger(indio_dev);
> + if (ret)
> return ret;
>
> /*
> @@ -382,10 +402,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_configure_trigger(indio_dev);
> + if (ret)
> return ret;
> }
>
> --
> 2.27.0
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:12 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 08/16] iio: adc: max1027: Explain better how the
> trigger state gets changed
>
> [External]
>
> It may appear strange to configure the trigger to start upon register
> write while we want to disable the trigger. Explain why we do this in
> the comment.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 59914fcfd320..4a78c9cbc339 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -401,7 +401,12 @@ static int
> max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
> if (ret < 0)
> return ret;
> } else {
> - /* Start acquisition on conversion register write */
> + /*
> + * 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_configure_trigger(indio_dev);
> if (ret)
> return ret;
> --
> 2.27.0
Reviewed-by: Nuno Sá <[email protected]>
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:12 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 09/16] iio: adc: max1027: Create a helper to configure
> the channels to scan
>
> [External]
>
> These bits are meant to be reused for triggered buffers setup.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 26 ++++++++++++++++++--------
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 4a78c9cbc339..223c9e4abd86 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -236,6 +236,22 @@ struct max1027_state {
> u8 reg ____cacheline_aligned;
> };
>
> +/* Scan from 0 to the highest requested channel */
> +static int max1027_configure_chans_to_scan(struct iio_dev
> *indio_dev)
> +{
> + struct max1027_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + /* The temperature could be avoided but it simplifies a bit the
> logic */
> + st->reg = MAX1027_CONV_REG | MAX1027_SCAN_0_N |
> MAX1027_TEMP;
> + st->reg |= MAX1027_CHAN(fls(*indio_dev-
> >active_scan_mask) - 2);
> + ret = spi_write(st->spi, &st->reg, 1);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
return spi_write(st->spi, &st->reg, 1);?
> +}
> +
> static int max1027_configure_trigger(struct iio_dev *indio_dev)
> {
> struct max1027_state *st = iio_priv(indio_dev);
> @@ -391,14 +407,8 @@ static int
> max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
> if (ret)
> return ret;
>
> - /*
> - * Scan from 0 to the highest requested channel. The
> temperature
> - * could be avoided but it simplifies a bit the logic.
> - */
> - st->reg = MAX1027_CONV_REG |
> MAX1027_SCAN_0_N | MAX1027_TEMP;
> - st->reg |= MAX1027_CHAN(fls(*indio_dev-
> >active_scan_mask) - 2);
> - ret = spi_write(st->spi, &st->reg, 1);
> - if (ret < 0)
> + ret = max1027_configure_chans_to_scan(indio_dev);
> + if (ret)
> return ret;
> } else {
> /*
> --
> 2.27.0
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:12 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 11/16] iio: adc: max1027: Separate the IRQ handler
> from the read logic
>
> [External]
>
> Create a max1027_read_scan() helper which will make clearer the
> future IRQ
> handler updates (no functional change).
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 83526f3d7d3a..afc3ce69f7ea 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -427,19 +427,18 @@ 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 = fls(*indio_dev-
> >active_scan_mask);
> u16 *buf = st->buffer;
> unsigned int bit;
> -
> - pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq,
> private);
> + int ret;
>
> /* 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;
>
> /* Only keep the channels selected by the user */
> for_each_set_bit(bit, indio_dev->active_scan_mask,
> @@ -451,6 +450,22 @@ static irqreturn_t max1027_trigger_handler(int
> irq, void *private)
>
> 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;
> +
> + pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq,
> private);
> +
This should be more consistent... use 'dev_err()'. I would also
argue to use the spi dev as the driver state structure holds a
pointer to it...
- Nuno Sá
> + 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
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:12 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> accesses during buffer reads
>
> [External]
>
> 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. Prevent it with a iio_buffer_enabled() check.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 223c9e4abd86..83526f3d7d3a 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -335,6 +335,8 @@ static int max1027_read_raw(struct iio_dev
> *indio_dev,
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> + if (iio_buffer_enabled(indio_dev))
> + return -EBUSY;
I guess 'iio_device_claim_direct_mode()' would be a better option
here? There's nothing preventing this check to pass and then, concurrently
someone enables the buffer...
- Nuno Sá
> ret = max1027_read_single_value(indio_dev, chan,
> val);
> break;
> case IIO_CHAN_INFO_SCALE:
> --
> 2.27.0
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:12 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 12/16] iio: adc: max1027: Introduce an end of
> conversion helper
>
> [External]
>
> For now this helper only waits for the maximum duration of a
> conversion,
> but it will soon be improved to properly handle the end of conversion
> interrupt.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index afc3ce69f7ea..2d6485591761 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,
> @@ -236,6 +239,20 @@ struct max1027_state {
> u8 reg ____cacheline_aligned;
> };
>
> +static DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
> +
> +static int max1027_wait_eoc(struct iio_dev *indio_dev)
> +{
> + unsigned int conversion_time =
> MAX1027_CONVERSION_UDELAY;
> +
> + if (indio_dev->active_scan_mask)
> + conversion_time *= hweight32(*indio_dev-
> >active_scan_mask);
> +
> + usleep_range(conversion_time, conversion_time * 2);
> +
> + return 0;
> +}
> +
> /* Scan from 0 to the highest requested channel */
> static int max1027_configure_chans_to_scan(struct iio_dev
> *indio_dev)
> {
> @@ -310,9 +327,11 @@ static int max1027_read_single_value(struct
> iio_dev *indio_dev,
> /*
> * For an unknown reason, when we use the mode "10" (write
> * conversion register), the interrupt doesn't occur every time.
> - * So we just wait 1 ms.
> + * So we just wait the maximum conversion time and deliver
> the value.
> */
> - mdelay(1);
> + ret = max1027_wait_eoc(indio_dev);
> + if (ret)
> + return ret;
I'm a bit confused here... Why are we looking at the active_scan_mask?
Aren't we preventing this for running concurrently with buffering?
- Nuno Sá
>
> /* Read result */
> ret = spi_read(st->spi, st->buffer, (chan->type == IIO_TEMP) ?
> 4 : 2);
> --
> 2.27.0
> -----Original Message-----
> From: Sa, Nuno <[email protected]>
> Sent: Friday, August 20, 2021 9:21 AM
> To: Miquel Raynal <[email protected]>; Jonathan Cameron
> <[email protected]>; Lars-Peter Clausen <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]
> Subject: RE: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> accesses during buffer reads
>
> [External]
>
>
>
> > -----Original Message-----
> > From: Miquel Raynal <[email protected]>
> > Sent: Wednesday, August 18, 2021 1:12 PM
> > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > <[email protected]>
> > Cc: Thomas Petazzoni <[email protected]>; linux-
> > [email protected]; [email protected]; Miquel Raynal
> > <[email protected]>
> > Subject: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> > accesses during buffer reads
> >
> > [External]
> >
> > 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. Prevent it with a iio_buffer_enabled() check.
> >
> > Signed-off-by: Miquel Raynal <[email protected]>
> > ---
> > drivers/iio/adc/max1027.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > index 223c9e4abd86..83526f3d7d3a 100644
> > --- a/drivers/iio/adc/max1027.c
> > +++ b/drivers/iio/adc/max1027.c
> > @@ -335,6 +335,8 @@ static int max1027_read_raw(struct iio_dev
> > *indio_dev,
> >
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > + if (iio_buffer_enabled(indio_dev))
> > + return -EBUSY;
>
> I guess 'iio_device_claim_direct_mode()' would be a better option
> here? There's nothing preventing this check to pass and then,
> concurrently
> someone enables the buffer...
>
Taking a second look, it seems that this check is already done [1]? Am I missing
I missing something?
Also, I think we are returning with the 'st->lock' held...
[1]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/max1027.c#L247
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:12 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 13/16] iio: adc: max1027: Prepare re-using the EOC
> interrupt
>
> [External]
>
> Right now the driver only has hardware trigger support, which makes
> use
> of the End Of Conversion (EOC) interrupt by:
> - Enabling the external trigger
> - At completion of the conversion, run a generic handler
> - Push the data to the IIO subsystem by using the triggered buffer
> infrastructure, which may not only serve this purpose.
>
> In fact, the interrupt will fire for more reasons than just a hardware
> trigger condition, so make a dedicated interrupt handler which will be
> enhanced by the upcoming changes to handle more situations.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 2d6485591761..8d86e77fb5db 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -472,6 +472,24 @@ static int max1027_read_scan(struct iio_dev
> *indio_dev)
> return 0;
> }
>
> +static irqreturn_t max1027_eoc_irq_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct max1027_state *st = iio_priv(indio_dev);
> + int ret = 0;
> +
> + if (st->cnvst_trigger) {
> + ret = max1027_read_scan(indio_dev);
> + iio_trigger_notify_done(indio_dev->trig);
> + }
> +
> + if (ret)
> + dev_err(&indio_dev->dev,
> + "Cannot read scanned values (%d)\n", ret);
Huh? Shouldn't this be right after 'max1027_read_scan()'?
- Nuno Sá
> + return IRQ_HANDLED;
> +}
> +
> static irqreturn_t max1027_trigger_handler(int irq, void *private)
> {
> struct iio_poll_func *pf = private;
> @@ -563,11 +581,11 @@ static int max1027_probe(struct spi_device
> *spi)
> }
>
> ret = devm_request_threaded_irq(&spi->dev, spi->irq,
> -
> iio_trigger_generic_data_rdy_poll,
> +
> max1027_eoc_irq_handler,
> NULL,
>
> 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
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:12 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 14/16] iio: adc: max1027: Consolidate the end of
> conversion helper
>
> [External]
>
> Now that we have a dedicated handler for End Of Conversion
> interrupts,
> let's create a second path:
> - Situation 1: we are using the external hardware trigger, a conversion
> has been triggered and the ADC pushed the data to its FIFO, we need
> to
> retrieve the data and push it to the IIO buffers.
> - Situation 2: we are not using the external hardware trigger, hence we
> are likely waiting in a blocked thread waiting for this interrupt to
> happen: in this case we just wake up the waiting thread.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 8d86e77fb5db..8c5995ae59f2 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -235,6 +235,7 @@ struct max1027_state {
> struct iio_trigger *trig;
> __be16 *buffer;
> struct mutex lock;
> + bool data_rdy;
> bool cnvst_trigger;
> u8 reg ____cacheline_aligned;
> };
> @@ -243,12 +244,22 @@ static
> DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
> 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;
>
> - if (indio_dev->active_scan_mask)
> - conversion_time *= hweight32(*indio_dev-
> >active_scan_mask);
> + if (st->spi->irq) {
Wouldn't it be more clear looking at 'st-> cnvst_trigger'? since that
is what we are using the wake up or nor the waiters...
- Nuno Sá
> + ret =
> wait_event_interruptible_timeout(max1027_queue,
> + st->data_rdy, HZ /
> 1000);
> + st->data_rdy = false;
> + if (ret == -ERESTARTSYS)
> + return ret;
> + } else {
> + if (indio_dev->active_scan_mask)
> + conversion_time *= hweight32(*indio_dev-
> >active_scan_mask);
>
> - usleep_range(conversion_time, conversion_time * 2);
> + usleep_range(conversion_time, conversion_time * 2);
> + }
>
> return 0;
> }
> @@ -481,6 +492,9 @@ static irqreturn_t max1027_eoc_irq_handler(int
> irq, void *private)
> if (st->cnvst_trigger) {
> ret = max1027_read_scan(indio_dev);
> iio_trigger_notify_done(indio_dev->trig);
> + } else {
> + st->data_rdy = true;
> + wake_up(&max1027_queue);
> }
>
> if (ret)
> --
> 2.27.0
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, August 18, 2021 1:12 PM
> To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>; linux-
> [email protected]; [email protected]; Miquel Raynal
> <[email protected]>
> Subject: [PATCH 15/16] iio: adc: max1027: Support software triggers
>
> [External]
>
> Now that max1027_trigger_handler() has been freed from handling
> hardware
> triggers EOC situations, we can use it for what it has been designed in
> the first place: trigger software originated conversions. In other
> words, when userspace initiates a conversion with a sysfs trigger or a
> hrtimer trigger, we must do all configuration steps, ie:
> 1- Configuring the trigger
> 2- Configuring the channels to scan
> 3- Starting the conversion (actually done automatically by step 2 in
> this case)
> 4- Waiting for the conversion to end
> 5- Retrieving the data from the ADC
> 6- Push the data to the IIO core and notify it
>
> Add the missing steps to this helper and drop the trigger verification
> hook otherwise software triggers would simply not be accepted at all.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 8c5995ae59f2..bb437e43adaf 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -413,17 +413,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);
> @@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int
> irq, void *private)
>
> pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq,
> private);
>
> + ret = max1027_configure_trigger(indio_dev);
> + if (ret)
> + goto out;
> +
> + ret = max1027_configure_chans_to_scan(indio_dev);
> + if (ret)
> + goto out;
> +
> + ret = max1027_wait_eoc(indio_dev);
> + if (ret)
> + goto out;
> +
> ret = max1027_read_scan(indio_dev);
There's something that I'm not getting... How are we checking that
we have software triggers? This API is called only if the device
allocates it's own trigger which will happen if there's a spi IRQ.
I'm probably missing something as this series is fairly big but the way
I would do it is (in the probe):
- always call 'devm_iio_triggered_buffer_setup()' function and properly use
buffer ops [1] (for example, you can use 'validate_scan_mask()' to setup the
channels to read);
- only allocate a trigger if an IRQ is present in which case, we assume HW
triggering is supported.
Does this makes sense?
- Nuno Sá
On Wed, 18 Aug 2021 13:11:26 +0200
Miquel Raynal <[email protected]> wrote:
> When a triggered scan occurs, the identity of the desired channels is
> known in indio_dev->active_scan_mask. Instead of reading and pushing to
> the IIO buffers all channels each time, scan the minimum amount of
> channels (0 to maximum requested chan, to be exact) and only provide the
> samples requested by the user.
>
> For example, if the user wants channels 1, 4 and 5, all channels from
> 0 to 5 will be scanned
That's a reasonably optimisation
> but only the desired channels will be pushed to
> the IIO buffers.
Don't do this last bit. The core handles demuxing the channels via
active_scan_masks. As a general rule it does a more efficient job of this
than a hand coded version in a driver (precached copy rules etc).
The core has to have the logic anyway to support multiple consumers
(e.g. in kernel consumer and userspace) so we reuse it for these cases.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index b753658bb41e..8ab660f596b5 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -360,6 +360,9 @@ static int max1027_set_trigger_state(struct iio_trigger *trig, bool state)
> struct max1027_state *st = iio_priv(indio_dev);
> int ret;
>
> + if (bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength))
> + return -EINVAL;
> +
> if (state) {
> /* Start acquisition on cnvst */
> st->reg = MAX1027_SETUP_REG | MAX1027_CKS_MODE0 |
> @@ -368,9 +371,12 @@ 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 0 to the highest requested channel. The temperature
> + * could be avoided but it simplifies a bit the logic.
> + */
> + st->reg = MAX1027_CONV_REG | MAX1027_SCAN_0_N | MAX1027_TEMP;
> + st->reg |= MAX1027_CHAN(fls(*indio_dev->active_scan_mask) - 2);
This should be combined with appropriate additions to available_scan_masks arrays
so the IIO core can handle choosing the right one to match enabled channels.
> ret = spi_write(st->spi, &st->reg, 1);
> if (ret < 0)
> return ret;
> @@ -391,11 +397,22 @@ 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 = fls(*indio_dev->active_scan_mask);
> + u16 *buf = st->buffer;
> + unsigned int bit;
>
> 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);
> + spi_read(st->spi, st->buffer, scanned_chans * 2);
> +
> + /* Only keep the channels selected by the user */
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> + if (buf[0] != st->buffer[bit])
> + buf[0] = st->buffer[bit];
> + buf++;
> + }
>
> iio_push_to_buffers(indio_dev, st->buffer);
>
On Fri, 20 Aug 2021 07:10:48 +0000
"Sa, Nuno" <[email protected]> wrote:
> > -----Original Message-----
> > From: Miquel Raynal <[email protected]>
> > Sent: Wednesday, August 18, 2021 1:11 PM
> > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > <[email protected]>
> > Cc: Thomas Petazzoni <[email protected]>; linux-
> > [email protected]; [email protected]; Miquel Raynal
> > <[email protected]>
> > Subject: [PATCH 03/16] iio: adc: max1027: Push only the requested
> > samples
> >
> > [External]
> >
> > When a triggered scan occurs, the identity of the desired channels is
> > known in indio_dev->active_scan_mask. Instead of reading and
> > pushing to
> > the IIO buffers all channels each time, scan the minimum amount of
> > channels (0 to maximum requested chan, to be exact) and only
> > provide the
> > samples requested by the user.
> >
> > For example, if the user wants channels 1, 4 and 5, all channels from
> > 0 to 5 will be scanned but only the desired channels will be pushed to
> > the IIO buffers.
> >
> > Signed-off-by: Miquel Raynal <[email protected]>
> > ---
> > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++----
> > 1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > index b753658bb41e..8ab660f596b5 100644
> > --- a/drivers/iio/adc/max1027.c
> > +++ b/drivers/iio/adc/max1027.c
> > @@ -360,6 +360,9 @@ static int max1027_set_trigger_state(struct
> > iio_trigger *trig, bool state)
> > struct max1027_state *st = iio_priv(indio_dev);
> > int ret;
> >
> > + if (bitmap_empty(indio_dev->active_scan_mask, indio_dev-
> > >masklength))
> > + return -EINVAL;
> > +
>
> I'm not sure this can actually happen. If you try to enable the buffer
> with no scan element, it should give you an error before you reach
> this point...
>
> > if (state) {
> > /* Start acquisition on cnvst */
> > st->reg = MAX1027_SETUP_REG |
> > MAX1027_CKS_MODE0 |
> > @@ -368,9 +371,12 @@ 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 0 to the highest requested channel. The
> > temperature
> > + * could be avoided but it simplifies a bit the logic.
> > + */
> > + st->reg = MAX1027_CONV_REG |
> > MAX1027_SCAN_0_N | MAX1027_TEMP;
> > + st->reg |= MAX1027_CHAN(fls(*indio_dev-
> > >active_scan_mask) - 2);
> > ret = spi_write(st->spi, &st->reg, 1);
> > if (ret < 0)
> > return ret;
> > @@ -391,11 +397,22 @@ 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 = fls(*indio_dev-
> > >active_scan_mask);
> > + u16 *buf = st->buffer;
>
> I think sparse will complain here. buffer is a __be16 restricted
> type so you should not mix those...
> > + unsigned int bit;
> >
> > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq,
> > private);in/20210818_miquel_raynal_bring_software_triggers_support_to_max1027_like_adcs.mbx
> >
> > /* fill buffer with all channel */
> > - spi_read(st->spi, st->buffer, indio_dev->masklength * 2);
> > + spi_read(st->spi, st->buffer, scanned_chans * 2);
> > +
> > + /* Only keep the channels selected by the user */
> > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > + indio_dev->masklength) {
> > + if (buf[0] != st->buffer[bit])
> > + buf[0] = st->buffer[bit];
>
> Since we are here, when looking into the driver, I realized
> that st->buffer is not DMA safe. In IIO, we kind of want to enforce
> that all buffers that are passed to spi/i2c buses are safe... Maybe
> this is something you can include in your series.
Why is it not? st->buffer is result of a devm_kmalloc_array() call and
that should provide a DMA safe buffer as I understand it.
>
> - Nuno Sá
>
> > + buf++;
> > + }
> >
> > iio_push_to_buffers(indio_dev, st->buffer);
> >
> > --
> > 2.27.0
>
On Wed, 18 Aug 2021 13:11:27 +0200
Miquel Raynal <[email protected]> wrote:
> When only a few channels are requested, the core will request all of
> them as long as ->available_scan_masks is present, thus reducing the
> impact of any driver side optimization to avoid converting the unneeded
> channels.
>
> Do not share this bitmap with the core, which is optional anyway.
As mentioned in previous patch - do share it with the core, but update
it to include the masks you now support.
As a side note, you should have done this in previous patch anyway
given that patch is effectively a noop with out it. (assuming it was
a good thing to do at all which I'm not sure it is!)
Jonathan
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 8ab660f596b5..d79dabf20567 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -457,7 +457,6 @@ static int max1027_probe(struct spi_device *spi)
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->channels = st->info->channels;
> indio_dev->num_channels = st->info->num_channels;
> - indio_dev->available_scan_masks = st->info->available_scan_masks;
>
> st->buffer = devm_kmalloc_array(&indio_dev->dev,
> indio_dev->num_channels, 2,
On Wed, 18 Aug 2021 13:11:28 +0200
Miquel Raynal <[email protected]> wrote:
> Memory allocation errors automatically trigger the right logs, no need
> to have our own.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> 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 d79dabf20567..ac603b4ca787 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -461,10 +461,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 == NULL) {
> - dev_err(&indio_dev->dev, "Can't allocate buffer\n");
> + if (!st->buffer)
Don't change the form of the check. Whilst it doesn't matter in general
which of these is used (and I actually prefer the one you end up with) this
change makes this case inconsistent with the style elsewhere in the driver.
If you want to clean these all up then I don't mind that, but please do
the lot in one patch doing just that.
Thanks,
Jonathan
> return -ENOMEM;
> - }
>
> if (spi->irq) {
> ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
On Wed, 18 Aug 2021 13:11:30 +0200
Miquel Raynal <[email protected]> wrote:
> 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 <[email protected]>
Trivial comments inline.
Thanks,
Jonathan
> ---
> drivers/iio/adc/max1027.c | 52 ++++++++++++++++++++++++++-------------
> 1 file changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index a6ebc951c09a..59914fcfd320 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -232,10 +232,37 @@ struct max1027_state {
> struct iio_trigger *trig;
> __be16 *buffer;
> struct mutex lock;
> -
Avoid white space changes in a patch doing something else.
In this case, I'd imagine the reg had a blank line before it to more
strongly indicate the cacheline_aligned nature meaning there would be
a bit of padding inserted here.
> + bool cnvst_trigger;
I'd expect a cnvst_trigger named variable to be a pointer to the trigger. Perhaps
call it use_cnvst_trigger or something like that?
> u8 reg ____cacheline_aligned;
> };
>
> +static int max1027_configure_trigger(struct iio_dev *indio_dev)
> +{
> + struct max1027_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + 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 (st->cnvst_trigger)
> + st->reg |= MAX1027_CKS_MODE0;
> + else
> + st->reg |= MAX1027_CKS_MODE2;
> +
> + ret = spi_write(st->spi, &st->reg, 1);
> + if (ret < 0) {
> + dev_err(&indio_dev->dev,
> + "Failed to configure register (%d)\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int max1027_read_single_value(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val)
> @@ -248,14 +275,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_configure_trigger(indio_dev);
> + if (ret)
> return ret;
> - }
>
> /* Configure conversion register with the requested chan */
> st->reg = MAX1027_CONV_REG | MAX1027_CHAN(chan->channel) |
> @@ -363,12 +385,10 @@ static int max1027_set_cnvst_trigger_state(struct iio_trigger *trig, bool state)
> if (bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength))
> return -EINVAL;
>
> + st->cnvst_trigger = state;
> 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_configure_trigger(indio_dev);
> + if (ret)
> return ret;
>
> /*
> @@ -382,10 +402,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_configure_trigger(indio_dev);
> + if (ret)
> return ret;
> }
>
On Fri, 20 Aug 2021 07:30:07 +0000
"Sa, Nuno" <[email protected]> wrote:
> > -----Original Message-----
> > From: Sa, Nuno <[email protected]>
> > Sent: Friday, August 20, 2021 9:21 AM
> > To: Miquel Raynal <[email protected]>; Jonathan Cameron
> > <[email protected]>; Lars-Peter Clausen <[email protected]>
> > Cc: Thomas Petazzoni <[email protected]>; linux-
> > [email protected]; [email protected]
> > Subject: RE: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> > accesses during buffer reads
> >
> > [External]
> >
> >
> >
> > > -----Original Message-----
> > > From: Miquel Raynal <[email protected]>
> > > Sent: Wednesday, August 18, 2021 1:12 PM
> > > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > <[email protected]>
> > > Cc: Thomas Petazzoni <[email protected]>; linux-
> > > [email protected]; [email protected]; Miquel Raynal
> > > <[email protected]>
> > > Subject: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> > > accesses during buffer reads
> > >
> > > [External]
> > >
> > > 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. Prevent it with a iio_buffer_enabled() check.
> > >
> > > Signed-off-by: Miquel Raynal <[email protected]>
> > > ---
> > > drivers/iio/adc/max1027.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > > index 223c9e4abd86..83526f3d7d3a 100644
> > > --- a/drivers/iio/adc/max1027.c
> > > +++ b/drivers/iio/adc/max1027.c
> > > @@ -335,6 +335,8 @@ static int max1027_read_raw(struct iio_dev
> > > *indio_dev,
> > >
> > > switch (mask) {
> > > case IIO_CHAN_INFO_RAW:
> > > + if (iio_buffer_enabled(indio_dev))
> > > + return -EBUSY;
> >
> > I guess 'iio_device_claim_direct_mode()' would be a better option
> > here? There's nothing preventing this check to pass and then,
> > concurrently
> > someone enables the buffer...
> >
>
> Taking a second look, it seems that this check is already done [1]? Am I missing
> I missing something?
>
> Also, I think we are returning with the 'st->lock' held...
>
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/max1027.c#L247
Absolutely agree this should be done with iio_device_claim_direct_mode() to close the
possible races.
I wonder why this one has been missed in all the cleanups of that stuff? Looks like
a simple case, but I guess it wasn't immediately visible in the read_raw() function
so no one noticed.
Jonathan
On Wed, 18 Aug 2021 13:11:36 +0200
Miquel Raynal <[email protected]> wrote:
> Right now the driver only has hardware trigger support, which makes use
> of the End Of Conversion (EOC) interrupt by:
> - Enabling the external trigger
> - At completion of the conversion, run a generic handler
> - Push the data to the IIO subsystem by using the triggered buffer
> infrastructure, which may not only serve this purpose.
>
> In fact, the interrupt will fire for more reasons than just a hardware
> trigger condition, so make a dedicated interrupt handler which will be
> enhanced by the upcoming changes to handle more situations.
>
> Signed-off-by: Miquel Raynal <[email protected]>
one suggestion inline.
> ---
> drivers/iio/adc/max1027.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 2d6485591761..8d86e77fb5db 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -472,6 +472,24 @@ static int max1027_read_scan(struct iio_dev *indio_dev)
> return 0;
> }
>
> +static irqreturn_t max1027_eoc_irq_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct max1027_state *st = iio_priv(indio_dev);
> + int ret = 0;
> +
> + if (st->cnvst_trigger) {
I missed this earlier, but it is very similar in purpose to the
logic used in iio_trigger_validate_own_device. Perhaps you could reuse that
rather than carrying an explicit variable to check for this?
> + ret = max1027_read_scan(indio_dev);
> + iio_trigger_notify_done(indio_dev->trig);
> + }
> +
> + if (ret)
> + dev_err(&indio_dev->dev,
> + "Cannot read scanned values (%d)\n", ret);
Perhaps better to move that into the if statement. I guess this may
make more sense later though!
> +
> + return IRQ_HANDLED;
> +}
> +
> static irqreturn_t max1027_trigger_handler(int irq, void *private)
> {
> struct iio_poll_func *pf = private;
> @@ -563,11 +581,11 @@ static int max1027_probe(struct spi_device *spi)
> }
>
> ret = devm_request_threaded_irq(&spi->dev, spi->irq,
> - iio_trigger_generic_data_rdy_poll,
> + max1027_eoc_irq_handler,
> NULL,
> 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;
On Wed, 18 Aug 2021 13:11:35 +0200
Miquel Raynal <[email protected]> wrote:
> For now this helper only waits for the maximum duration of a conversion,
> but it will soon be improved to properly handle the end of conversion
> interrupt.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index afc3ce69f7ea..2d6485591761 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,
> @@ -236,6 +239,20 @@ struct max1027_state {
> u8 reg ____cacheline_aligned;
> };
>
> +static DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
Why here? This had me confused when I couldn't find it being introduced
in the later patch that uses it. More comments in that patch.
> +
> +static int max1027_wait_eoc(struct iio_dev *indio_dev)
> +{
> + unsigned int conversion_time = MAX1027_CONVERSION_UDELAY;
> +
> + if (indio_dev->active_scan_mask)
> + conversion_time *= hweight32(*indio_dev->active_scan_mask);
> +
> + usleep_range(conversion_time, conversion_time * 2);
> +
> + return 0;
> +}
> +
> /* Scan from 0 to the highest requested channel */
> static int max1027_configure_chans_to_scan(struct iio_dev *indio_dev)
> {
> @@ -310,9 +327,11 @@ static int max1027_read_single_value(struct iio_dev *indio_dev,
> /*
> * For an unknown reason, when we use the mode "10" (write
> * conversion register), the interrupt doesn't occur every time.
> - * So we just wait 1 ms.
> + * So we just wait the maximum conversion time and deliver the value.
> */
> - mdelay(1);
> + ret = max1027_wait_eoc(indio_dev);
> + if (ret)
> + return ret;
>
> /* Read result */
> ret = spi_read(st->spi, st->buffer, (chan->type == IIO_TEMP) ? 4 : 2);
On Wed, 18 Aug 2021 13:11:37 +0200
Miquel Raynal <[email protected]> wrote:
> Now that we have a dedicated handler for End Of Conversion interrupts,
> let's create a second path:
> - Situation 1: we are using the external hardware trigger, a conversion
> has been triggered and the ADC pushed the data to its FIFO, we need to
> retrieve the data and push it to the IIO buffers.
> - Situation 2: we are not using the external hardware trigger, hence we
> are likely waiting in a blocked thread waiting for this interrupt to
> happen: in this case we just wake up the waiting thread.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 8d86e77fb5db..8c5995ae59f2 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -235,6 +235,7 @@ struct max1027_state {
> struct iio_trigger *trig;
> __be16 *buffer;
> struct mutex lock;
> + bool data_rdy;
> bool cnvst_trigger;
> u8 reg ____cacheline_aligned;
> };
> @@ -243,12 +244,22 @@ static DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
>
> 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;
>
> - if (indio_dev->active_scan_mask)
> - conversion_time *= hweight32(*indio_dev->active_scan_mask);
> + if (st->spi->irq) {
> + ret = wait_event_interruptible_timeout(max1027_queue,
> + st->data_rdy, HZ / 1000);
> + st->data_rdy = false;
> + if (ret == -ERESTARTSYS)
> + return ret;
> + } else {
> + if (indio_dev->active_scan_mask)
> + conversion_time *= hweight32(*indio_dev->active_scan_mask);
>
> - usleep_range(conversion_time, conversion_time * 2);
> + usleep_range(conversion_time, conversion_time * 2);
> + }
>
> return 0;
> }
> @@ -481,6 +492,9 @@ static irqreturn_t max1027_eoc_irq_handler(int irq, void *private)
> if (st->cnvst_trigger) {
> ret = max1027_read_scan(indio_dev);
> iio_trigger_notify_done(indio_dev->trig);
> + } else {
> + st->data_rdy = true;
> + wake_up(&max1027_queue);
I can't see why a queue is appropriate for this. Use a completion and have
one per instance of the device. No need for the flag etc in that case as
complete() means we have had an interrupt.
> }
>
> if (ret)
On Wed, 18 Aug 2021 13:11:36 +0200
Miquel Raynal <[email protected]> wrote:
> Right now the driver only has hardware trigger support, which makes use
> of the End Of Conversion (EOC) interrupt by:
> - Enabling the external trigger
> - At completion of the conversion, run a generic handler
> - Push the data to the IIO subsystem by using the triggered buffer
> infrastructure, which may not only serve this purpose.
>
> In fact, the interrupt will fire for more reasons than just a hardware
> trigger condition, so make a dedicated interrupt handler which will be
> enhanced by the upcoming changes to handle more situations.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 2d6485591761..8d86e77fb5db 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -472,6 +472,24 @@ static int max1027_read_scan(struct iio_dev *indio_dev)
> return 0;
> }
>
> +static irqreturn_t max1027_eoc_irq_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct max1027_state *st = iio_priv(indio_dev);
> + int ret = 0;
> +
> + if (st->cnvst_trigger) {
> + ret = max1027_read_scan(indio_dev);
> + iio_trigger_notify_done(indio_dev->trig);
Hmm. Having read on I now realise how you have decided to handle this.
For the 'data ready' type interrupts you are bypassing the triggering
framework entirely. in which case iio_trigger_notify_done() is irrelevant
as the iio_trigger_poll() has never been called.
Normally we only do this sort of bypassing of the trigger infrastructure
when there is a hw fifo involved and hence a given 'interrupt' doesn't
represent a single trigger. Where possible we do expose the trigger
because it can in turn be used to trigger other devices.
In this driver that is currently prevented by the validate_device() callback
for the trigger so arguably the change you make here has no affect.
I wonder how hard it would be to change things to allow the validate_device()
protection to be dropped. There is a slightly nasty corner case where another
device is attached to this trigger but this device is not. We don't need to
make that 'work' as such because it makes little sense, but we do need to ensure
that things don't blow up if someone configure it like that.
> + }
> +
> + if (ret)
> + dev_err(&indio_dev->dev,
> + "Cannot read scanned values (%d)\n", ret);
> +
> + return IRQ_HANDLED;
> +}
> +
> static irqreturn_t max1027_trigger_handler(int irq, void *private)
> {
> struct iio_poll_func *pf = private;
> @@ -563,11 +581,11 @@ static int max1027_probe(struct spi_device *spi)
> }
>
> ret = devm_request_threaded_irq(&spi->dev, spi->irq,
> - iio_trigger_generic_data_rdy_poll,
> + max1027_eoc_irq_handler,
> NULL,
> 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;
On Wed, 18 Aug 2021 13:11:38 +0200
Miquel Raynal <[email protected]> wrote:
> Now that max1027_trigger_handler() has been freed from handling hardware
> triggers EOC situations, we can use it for what it has been designed in
> the first place: trigger software originated conversions.
As mentioned earlier, this is not how I'd normally expect this sort of
case to be handled. I'd be expecting the cnvst trigger to still be calling
this function and the function to do the relevant check to ensure it
knows the data is already available in that case.
> In other
> words, when userspace initiates a conversion with a sysfs trigger or a
> hrtimer trigger, we must do all configuration steps, ie:
> 1- Configuring the trigger
> 2- Configuring the channels to scan
> 3- Starting the conversion (actually done automatically by step 2 in
> this case)
> 4- Waiting for the conversion to end
> 5- Retrieving the data from the ADC
> 6- Push the data to the IIO core and notify it
>
> Add the missing steps to this helper and drop the trigger verification
> hook otherwise software triggers would simply not be accepted at all.
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 8c5995ae59f2..bb437e43adaf 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -413,17 +413,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);
> @@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private)
>
> pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private);
>
> + ret = max1027_configure_trigger(indio_dev);
I'd not expect to see this ever time. The configuration shouldn't change
from one call of this function to the next.
> + if (ret)
> + goto out;
> +
> + ret = max1027_configure_chans_to_scan(indio_dev);
This should also not change unless it is also responsible for the 'go' signal.
If that's true then it is badly named.
> + 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);
> @@ -529,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,
> };
>
> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Monday, August 30, 2021 12:08 PM
> To: Sa, Nuno <[email protected]>
> Cc: Miquel Raynal <[email protected]>; Lars-Peter Clausen
> <[email protected]>; Thomas Petazzoni
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested
> samples
>
> [External]
>
> On Fri, 20 Aug 2021 07:10:48 +0000
> "Sa, Nuno" <[email protected]> wrote:
>
> > > -----Original Message-----
> > > From: Miquel Raynal <[email protected]>
> > > Sent: Wednesday, August 18, 2021 1:11 PM
> > > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > <[email protected]>
> > > Cc: Thomas Petazzoni <[email protected]>; linux-
> > > [email protected]; [email protected]; Miquel Raynal
> > > <[email protected]>
> > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the requested
> > > samples
> > >
> > > [External]
> > >
> > > When a triggered scan occurs, the identity of the desired channels
> is
> > > known in indio_dev->active_scan_mask. Instead of reading and
> > > pushing to
> > > the IIO buffers all channels each time, scan the minimum amount
> of
> > > channels (0 to maximum requested chan, to be exact) and only
> > > provide the
> > > samples requested by the user.
> > >
> > > For example, if the user wants channels 1, 4 and 5, all channels
> from
> > > 0 to 5 will be scanned but only the desired channels will be pushed
> to
> > > the IIO buffers.
> > >
> > > Signed-off-by: Miquel Raynal <[email protected]>
> > > ---
> > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++----
> > > 1 file changed, 21 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > > index b753658bb41e..8ab660f596b5 100644
> > > --- a/drivers/iio/adc/max1027.c
> > > +++ b/drivers/iio/adc/max1027.c
> > > @@ -360,6 +360,9 @@ static int max1027_set_trigger_state(struct
> > > iio_trigger *trig, bool state)
> > > struct max1027_state *st = iio_priv(indio_dev);
> > > int ret;
> > >
> > > + if (bitmap_empty(indio_dev->active_scan_mask, indio_dev-
> > > >masklength))
> > > + return -EINVAL;
> > > +
> >
> > I'm not sure this can actually happen. If you try to enable the buffer
> > with no scan element, it should give you an error before you reach
> > this point...
> >
> > > if (state) {
> > > /* Start acquisition on cnvst */
> > > st->reg = MAX1027_SETUP_REG |
> > > MAX1027_CKS_MODE0 |
> > > @@ -368,9 +371,12 @@ 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 0 to the highest requested channel. The
> > > temperature
> > > + * could be avoided but it simplifies a bit the logic.
> > > + */
> > > + st->reg = MAX1027_CONV_REG |
> > > MAX1027_SCAN_0_N | MAX1027_TEMP;
> > > + st->reg |= MAX1027_CHAN(fls(*indio_dev-
> > > >active_scan_mask) - 2);
> > > ret = spi_write(st->spi, &st->reg, 1);
> > > if (ret < 0)
> > > return ret;
> > > @@ -391,11 +397,22 @@ 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 = fls(*indio_dev-
> > > >active_scan_mask);
> > > + u16 *buf = st->buffer;
> >
> > I think sparse will complain here. buffer is a __be16 restricted
> > type so you should not mix those...
> > > + unsigned int bit;
> > >
> > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq,
> > >
> private);in/20210818_miquel_raynal_bring_software_triggers_support
> _to_max1027_like_adcs.mbx
> > >
> > > /* fill buffer with all channel */
> > > - spi_read(st->spi, st->buffer, indio_dev->masklength * 2);
> > > + spi_read(st->spi, st->buffer, scanned_chans * 2);
> > > +
> > > + /* Only keep the channels selected by the user */
> > > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > + indio_dev->masklength) {
> > > + if (buf[0] != st->buffer[bit])
> > > + buf[0] = st->buffer[bit];
> >
> > Since we are here, when looking into the driver, I realized
> > that st->buffer is not DMA safe. In IIO, we kind of want to enforce
> > that all buffers that are passed to spi/i2c buses are safe... Maybe
> > this is something you can include in your series.
>
> Why is it not? st->buffer is result of a devm_kmalloc_array() call and
> that should provide a DMA safe buffer as I understand it.
>
That's a good question. I'm not sure how I came to that conclusion which
is clearly wrong. Though I think the buffer might share the line with the
mutex...
- Nuno Sá
On Wed, 18 Aug 2021 13:11:39 +0200
Miquel Raynal <[email protected]> wrote:
> Software triggers do not need a device IRQ to work. As opposed to
> hardware triggers which need it to yield the data to the IIO core,
> software triggers run a dedicated thread which does all the tasks on
> their behalf. Then, 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.
I'd stop referring to software triggers in these descriptions. This
could just as easily be about enabling a different hardware trigger
such as a gpio trigger or indeed on a dataready trigger provided by
an entirely different device.
Otherwise the logic is correct.
Good to get this more flexible support into the driver. If we can
make it a tiny bit more flexible by enabling use of the cnvst trigger
to drive this 'and' other drivers that would be even better and
conform more closely to the normal way an IIO driver work.
The validate_device / validate_trigger callbacks are often about
making it easier to bring a device driver up in the first place, so
it's great to see them go away in later improvements like this.
(note I'm not saying there aren't complex cases where we can't remove
them though!)
Jonathan
>
> Signed-off-by: Miquel Raynal <[email protected]>
> ---
> drivers/iio/adc/max1027.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index bb437e43adaf..e767437a578e 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -567,16 +567,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, enable the hardware trigger (cnvst) */
> 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 == NULL) {
> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Monday, August 30, 2021 12:37 PM
> To: Miquel Raynal <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>; Thomas Petazzoni
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end of
> conversion helper
>
> On Wed, 18 Aug 2021 13:11:37 +0200
> Miquel Raynal <[email protected]> wrote:
>
> > Now that we have a dedicated handler for End Of Conversion
> interrupts,
> > let's create a second path:
> > - Situation 1: we are using the external hardware trigger, a
> conversion
> > has been triggered and the ADC pushed the data to its FIFO, we
> need to
> > retrieve the data and push it to the IIO buffers.
> > - Situation 2: we are not using the external hardware trigger, hence
> we
> > are likely waiting in a blocked thread waiting for this interrupt to
> > happen: in this case we just wake up the waiting thread.
> >
> > Signed-off-by: Miquel Raynal <[email protected]>
> > ---
> > drivers/iio/adc/max1027.c | 20 +++++++++++++++++---
> > 1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > index 8d86e77fb5db..8c5995ae59f2 100644
> > --- a/drivers/iio/adc/max1027.c
> > +++ b/drivers/iio/adc/max1027.c
> > @@ -235,6 +235,7 @@ struct max1027_state {
> > struct iio_trigger *trig;
> > __be16 *buffer;
> > struct mutex lock;
> > + bool data_rdy;
> > bool cnvst_trigger;
> > u8 reg ____cacheline_aligned;
> > };
> > @@ -243,12 +244,22 @@ static
> DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
> >
> > 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;
> >
> > - if (indio_dev->active_scan_mask)
> > - conversion_time *= hweight32(*indio_dev-
> >active_scan_mask);
> > + if (st->spi->irq) {
> > + ret =
> wait_event_interruptible_timeout(max1027_queue,
> > + st->data_rdy, HZ /
> 1000);
> > + st->data_rdy = false;
> > + if (ret == -ERESTARTSYS)
> > + return ret;
> > + } else {
> > + if (indio_dev->active_scan_mask)
> > + conversion_time *= hweight32(*indio_dev-
> >active_scan_mask);
> >
> > - usleep_range(conversion_time, conversion_time * 2);
> > + usleep_range(conversion_time, conversion_time * 2);
> > + }
> >
> > return 0;
> > }
> > @@ -481,6 +492,9 @@ static irqreturn_t
> max1027_eoc_irq_handler(int irq, void *private)
> > if (st->cnvst_trigger) {
> > ret = max1027_read_scan(indio_dev);
> > iio_trigger_notify_done(indio_dev->trig);
> > + } else {
> > + st->data_rdy = true;
> > + wake_up(&max1027_queue);
>
> I can't see why a queue is appropriate for this. Use a completion and
> have
> one per instance of the device. No need for the flag etc in that case as
> complete() means we have had an interrupt.
>
In the case that 'st-> cnvst_trigger' is not set but the spi IRQ
is present, we will wait until we get 'wake_up()' called from here. I wonder if
that is a good idea as the device own trigger is not being used. FWIW, I think this
sync logic is a bit confusing... I would still use the normal trigger infrastructure
('iio_trigger_generic_data_rdy_poll()') and use the 'cnvst_trigger' flag in the
trigger handler to manually start conversions + wait till eoc. But I might be missing
something though.
Regarding this handler, I just realized that this is the hard IRQ handler which
might end up calling 'max1027_read_scan()' which in turn calls 'spi_read()'. Am I
missing something here?
- Nuno S?
On Mon, 30 Aug 2021 10:49:50 +0000
"Sa, Nuno" <[email protected]> wrote:
> > -----Original Message-----
> > From: Jonathan Cameron <[email protected]>
> > Sent: Monday, August 30, 2021 12:08 PM
> > To: Sa, Nuno <[email protected]>
> > Cc: Miquel Raynal <[email protected]>; Lars-Peter Clausen
> > <[email protected]>; Thomas Petazzoni
> > <[email protected]>; [email protected]; linux-
> > [email protected]
> > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested
> > samples
> >
> > [External]
> >
> > On Fri, 20 Aug 2021 07:10:48 +0000
> > "Sa, Nuno" <[email protected]> wrote:
> >
> > > > -----Original Message-----
> > > > From: Miquel Raynal <[email protected]>
> > > > Sent: Wednesday, August 18, 2021 1:11 PM
> > > > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > > <[email protected]>
> > > > Cc: Thomas Petazzoni <[email protected]>; linux-
> > > > [email protected]; [email protected]; Miquel Raynal
> > > > <[email protected]>
> > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the requested
> > > > samples
> > > >
> > > > [External]
> > > >
> > > > When a triggered scan occurs, the identity of the desired channels
> > is
> > > > known in indio_dev->active_scan_mask. Instead of reading and
> > > > pushing to
> > > > the IIO buffers all channels each time, scan the minimum amount
> > of
> > > > channels (0 to maximum requested chan, to be exact) and only
> > > > provide the
> > > > samples requested by the user.
> > > >
> > > > For example, if the user wants channels 1, 4 and 5, all channels
> > from
> > > > 0 to 5 will be scanned but only the desired channels will be pushed
> > to
> > > > the IIO buffers.
> > > >
> > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > ---
> > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++----
> > > > 1 file changed, 21 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > > > index b753658bb41e..8ab660f596b5 100644
> > > > --- a/drivers/iio/adc/max1027.c
> > > > +++ b/drivers/iio/adc/max1027.c
> > > > @@ -360,6 +360,9 @@ static int max1027_set_trigger_state(struct
> > > > iio_trigger *trig, bool state)
> > > > struct max1027_state *st = iio_priv(indio_dev);
> > > > int ret;
> > > >
> > > > + if (bitmap_empty(indio_dev->active_scan_mask, indio_dev-
> > > > >masklength))
> > > > + return -EINVAL;
> > > > +
> > >
> > > I'm not sure this can actually happen. If you try to enable the buffer
> > > with no scan element, it should give you an error before you reach
> > > this point...
> > >
> > > > if (state) {
> > > > /* Start acquisition on cnvst */
> > > > st->reg = MAX1027_SETUP_REG |
> > > > MAX1027_CKS_MODE0 |
> > > > @@ -368,9 +371,12 @@ 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 0 to the highest requested channel. The
> > > > temperature
> > > > + * could be avoided but it simplifies a bit the logic.
> > > > + */
> > > > + st->reg = MAX1027_CONV_REG |
> > > > MAX1027_SCAN_0_N | MAX1027_TEMP;
> > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev-
> > > > >active_scan_mask) - 2);
> > > > ret = spi_write(st->spi, &st->reg, 1);
> > > > if (ret < 0)
> > > > return ret;
> > > > @@ -391,11 +397,22 @@ 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 = fls(*indio_dev-
> > > > >active_scan_mask);
> > > > + u16 *buf = st->buffer;
> > >
> > > I think sparse will complain here. buffer is a __be16 restricted
> > > type so you should not mix those...
> > > > + unsigned int bit;
> > > >
> > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq,
> > > >
> > private);in/20210818_miquel_raynal_bring_software_triggers_support
> > _to_max1027_like_adcs.mbx
> > > >
> > > > /* fill buffer with all channel */
> > > > - spi_read(st->spi, st->buffer, indio_dev->masklength * 2);
> > > > + spi_read(st->spi, st->buffer, scanned_chans * 2);
> > > > +
> > > > + /* Only keep the channels selected by the user */
> > > > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > > + indio_dev->masklength) {
> > > > + if (buf[0] != st->buffer[bit])
> > > > + buf[0] = st->buffer[bit];
> > >
> > > Since we are here, when looking into the driver, I realized
> > > that st->buffer is not DMA safe. In IIO, we kind of want to enforce
> > > that all buffers that are passed to spi/i2c buses are safe... Maybe
> > > this is something you can include in your series.
> >
> > Why is it not? st->buffer is result of a devm_kmalloc_array() call and
> > that should provide a DMA safe buffer as I understand it.
> >
>
> That's a good question. I'm not sure how I came to that conclusion which
> is clearly wrong. Though I think the buffer might share the line with the
> mutex...
Pointer shares a line. The buffer it points to doesn't as allocated
by separate heap allocation.
J
>
> - Nuno Sá
>
On Mon, 30 Aug 2021 12:44:48 +0000
"Sa, Nuno" <[email protected]> wrote:
> > -----Original Message-----
> > From: Jonathan Cameron <[email protected]>
> > Sent: Monday, August 30, 2021 12:37 PM
> > To: Miquel Raynal <[email protected]>
> > Cc: Lars-Peter Clausen <[email protected]>; Thomas Petazzoni
> > <[email protected]>; [email protected]; linux-
> > [email protected]
> > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end of
> > conversion helper
> >
> > On Wed, 18 Aug 2021 13:11:37 +0200
> > Miquel Raynal <[email protected]> wrote:
> >
> > > Now that we have a dedicated handler for End Of Conversion
> > interrupts,
> > > let's create a second path:
> > > - Situation 1: we are using the external hardware trigger, a
> > conversion
> > > has been triggered and the ADC pushed the data to its FIFO, we
> > need to
> > > retrieve the data and push it to the IIO buffers.
> > > - Situation 2: we are not using the external hardware trigger, hence
> > we
> > > are likely waiting in a blocked thread waiting for this interrupt to
> > > happen: in this case we just wake up the waiting thread.
> > >
> > > Signed-off-by: Miquel Raynal <[email protected]>
> > > ---
> > > drivers/iio/adc/max1027.c | 20 +++++++++++++++++---
> > > 1 file changed, 17 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > > index 8d86e77fb5db..8c5995ae59f2 100644
> > > --- a/drivers/iio/adc/max1027.c
> > > +++ b/drivers/iio/adc/max1027.c
> > > @@ -235,6 +235,7 @@ struct max1027_state {
> > > struct iio_trigger *trig;
> > > __be16 *buffer;
> > > struct mutex lock;
> > > + bool data_rdy;
> > > bool cnvst_trigger;
> > > u8 reg ____cacheline_aligned;
> > > };
> > > @@ -243,12 +244,22 @@ static
> > DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
> > >
> > > 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;
> > >
> > > - if (indio_dev->active_scan_mask)
> > > - conversion_time *= hweight32(*indio_dev-
> > >active_scan_mask);
> > > + if (st->spi->irq) {
> > > + ret =
> > wait_event_interruptible_timeout(max1027_queue,
> > > + st->data_rdy, HZ /
> > 1000);
> > > + st->data_rdy = false;
> > > + if (ret == -ERESTARTSYS)
> > > + return ret;
> > > + } else {
> > > + if (indio_dev->active_scan_mask)
> > > + conversion_time *= hweight32(*indio_dev-
> > >active_scan_mask);
> > >
> > > - usleep_range(conversion_time, conversion_time * 2);
> > > + usleep_range(conversion_time, conversion_time * 2);
> > > + }
> > >
> > > return 0;
> > > }
> > > @@ -481,6 +492,9 @@ static irqreturn_t
> > max1027_eoc_irq_handler(int irq, void *private)
> > > if (st->cnvst_trigger) {
> > > ret = max1027_read_scan(indio_dev);
> > > iio_trigger_notify_done(indio_dev->trig);
> > > + } else {
> > > + st->data_rdy = true;
> > > + wake_up(&max1027_queue);
> >
> > I can't see why a queue is appropriate for this. Use a completion and
> > have
> > one per instance of the device. No need for the flag etc in that case as
> > complete() means we have had an interrupt.
> >
>
> In the case that 'st-> cnvst_trigger' is not set but the spi IRQ
> is present, we will wait until we get 'wake_up()' called from here. I wonder if
> that is a good idea as the device own trigger is not being used. FWIW, I think this
> sync logic is a bit confusing... I would still use the normal trigger infrastructure
> ('iio_trigger_generic_data_rdy_poll()') and use the 'cnvst_trigger' flag in the
> trigger handler to manually start conversions + wait till eoc. But I might be missing
> something though.
>
> Regarding this handler, I just realized that this is the hard IRQ handler which
> might end up calling 'max1027_read_scan()' which in turn calls 'spi_read()'. Am I
> missing something here?
Good point. This will need to be the threaded handler if done like this.
J
>
> - Nuno Sá
> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Monday, August 30, 2021 4:30 PM
> To: Sa, Nuno <[email protected]>
> Cc: Miquel Raynal <[email protected]>; Lars-Peter Clausen
> <[email protected]>; Thomas Petazzoni
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested
> samples
>
> [External]
>
> On Mon, 30 Aug 2021 10:49:50 +0000
> "Sa, Nuno" <[email protected]> wrote:
>
> > > -----Original Message-----
> > > From: Jonathan Cameron <[email protected]>
> > > Sent: Monday, August 30, 2021 12:08 PM
> > > To: Sa, Nuno <[email protected]>
> > > Cc: Miquel Raynal <[email protected]>; Lars-Peter
> Clausen
> > > <[email protected]>; Thomas Petazzoni
> > > <[email protected]>; [email protected];
> linux-
> > > [email protected]
> > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the
> requested
> > > samples
> > >
> > > [External]
> > >
> > > On Fri, 20 Aug 2021 07:10:48 +0000
> > > "Sa, Nuno" <[email protected]> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Miquel Raynal <[email protected]>
> > > > > Sent: Wednesday, August 18, 2021 1:11 PM
> > > > > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > > > <[email protected]>
> > > > > Cc: Thomas Petazzoni <[email protected]>; linux-
> > > > > [email protected]; [email protected]; Miquel
> Raynal
> > > > > <[email protected]>
> > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the
> requested
> > > > > samples
> > > > >
> > > > > [External]
> > > > >
> > > > > When a triggered scan occurs, the identity of the desired
> channels
> > > is
> > > > > known in indio_dev->active_scan_mask. Instead of reading and
> > > > > pushing to
> > > > > the IIO buffers all channels each time, scan the minimum
> amount
> > > of
> > > > > channels (0 to maximum requested chan, to be exact) and only
> > > > > provide the
> > > > > samples requested by the user.
> > > > >
> > > > > For example, if the user wants channels 1, 4 and 5, all channels
> > > from
> > > > > 0 to 5 will be scanned but only the desired channels will be
> pushed
> > > to
> > > > > the IIO buffers.
> > > > >
> > > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > > ---
> > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++----
> > > > > 1 file changed, 21 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/adc/max1027.c
> b/drivers/iio/adc/max1027.c
> > > > > index b753658bb41e..8ab660f596b5 100644
> > > > > --- a/drivers/iio/adc/max1027.c
> > > > > +++ b/drivers/iio/adc/max1027.c
> > > > > @@ -360,6 +360,9 @@ static int
> max1027_set_trigger_state(struct
> > > > > iio_trigger *trig, bool state)
> > > > > struct max1027_state *st = iio_priv(indio_dev);
> > > > > int ret;
> > > > >
> > > > > + if (bitmap_empty(indio_dev->active_scan_mask,
> indio_dev-
> > > > > >masklength))
> > > > > + return -EINVAL;
> > > > > +
> > > >
> > > > I'm not sure this can actually happen. If you try to enable the
> buffer
> > > > with no scan element, it should give you an error before you
> reach
> > > > this point...
> > > >
> > > > > if (state) {
> > > > > /* Start acquisition on cnvst */
> > > > > st->reg = MAX1027_SETUP_REG |
> > > > > MAX1027_CKS_MODE0 |
> > > > > @@ -368,9 +371,12 @@ 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 0 to the highest requested
> channel. The
> > > > > temperature
> > > > > + * could be avoided but it simplifies a bit the
> logic.
> > > > > + */
> > > > > + st->reg = MAX1027_CONV_REG |
> > > > > MAX1027_SCAN_0_N | MAX1027_TEMP;
> > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev-
> > > > > >active_scan_mask) - 2);
> > > > > ret = spi_write(st->spi, &st->reg, 1);
> > > > > if (ret < 0)
> > > > > return ret;
> > > > > @@ -391,11 +397,22 @@ 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 = fls(*indio_dev-
> > > > > >active_scan_mask);
> > > > > + u16 *buf = st->buffer;
> > > >
> > > > I think sparse will complain here. buffer is a __be16 restricted
> > > > type so you should not mix those...
> > > > > + unsigned int bit;
> > > > >
> > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__,
> irq,
> > > > >
> > >
> private);in/20210818_miquel_raynal_bring_software_triggers_support
> > > _to_max1027_like_adcs.mbx
> > > > >
> > > > > /* fill buffer with all channel */
> > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength *
> 2);
> > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2);
> > > > > +
> > > > > + /* Only keep the channels selected by the user */
> > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > > > + indio_dev->masklength) {
> > > > > + if (buf[0] != st->buffer[bit])
> > > > > + buf[0] = st->buffer[bit];
> > > >
> > > > Since we are here, when looking into the driver, I realized
> > > > that st->buffer is not DMA safe. In IIO, we kind of want to
> enforce
> > > > that all buffers that are passed to spi/i2c buses are safe... Maybe
> > > > this is something you can include in your series.
> > >
> > > Why is it not? st->buffer is result of a devm_kmalloc_array() call
> and
> > > that should provide a DMA safe buffer as I understand it.
> > >
> >
> > That's a good question. I'm not sure how I came to that conclusion
> which
> > is clearly wrong. Though I think the buffer might share the line with
> the
> > mutex...
> Pointer shares a line. The buffer it points to doesn't as allocated
> by separate heap allocation.
>
Ups, sure :facepalm:
- Nuno Sá
Hello,
"Sa, Nuno" <[email protected]> wrote on Mon, 30 Aug 2021 15:02:26
+0000:
> > -----Original Message-----
> > From: Jonathan Cameron <[email protected]>
> > Sent: Monday, August 30, 2021 4:30 PM
> > To: Sa, Nuno <[email protected]>
> > Cc: Miquel Raynal <[email protected]>; Lars-Peter Clausen
> > <[email protected]>; Thomas Petazzoni
> > <[email protected]>; [email protected]; linux-
> > [email protected]
> > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested
> > samples
> >
> > [External]
> >
> > On Mon, 30 Aug 2021 10:49:50 +0000
> > "Sa, Nuno" <[email protected]> wrote:
> >
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <[email protected]>
> > > > Sent: Monday, August 30, 2021 12:08 PM
> > > > To: Sa, Nuno <[email protected]>
> > > > Cc: Miquel Raynal <[email protected]>; Lars-Peter
> > Clausen
> > > > <[email protected]>; Thomas Petazzoni
> > > > <[email protected]>; [email protected];
> > linux-
> > > > [email protected]
> > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the
> > requested
> > > > samples
> > > >
> > > > [External]
> > > >
> > > > On Fri, 20 Aug 2021 07:10:48 +0000
> > > > "Sa, Nuno" <[email protected]> wrote:
> > > >
> > > > > > -----Original Message-----
> > > > > > From: Miquel Raynal <[email protected]>
> > > > > > Sent: Wednesday, August 18, 2021 1:11 PM
> > > > > > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > > > > <[email protected]>
> > > > > > Cc: Thomas Petazzoni <[email protected]>; linux-
> > > > > > [email protected]; [email protected]; Miquel
> > Raynal
> > > > > > <[email protected]>
> > > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the
> > requested
> > > > > > samples
> > > > > >
> > > > > > [External]
> > > > > >
> > > > > > When a triggered scan occurs, the identity of the desired
> > channels
> > > > is
> > > > > > known in indio_dev->active_scan_mask. Instead of reading and
> > > > > > pushing to
> > > > > > the IIO buffers all channels each time, scan the minimum
> > amount
> > > > of
> > > > > > channels (0 to maximum requested chan, to be exact) and only
> > > > > > provide the
> > > > > > samples requested by the user.
> > > > > >
> > > > > > For example, if the user wants channels 1, 4 and 5, all channels
> > > > from
> > > > > > 0 to 5 will be scanned but only the desired channels will be
> > pushed
> > > > to
> > > > > > the IIO buffers.
> > > > > >
> > > > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > > > ---
> > > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++----
> > > > > > 1 file changed, 21 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iio/adc/max1027.c
> > b/drivers/iio/adc/max1027.c
> > > > > > index b753658bb41e..8ab660f596b5 100644
> > > > > > --- a/drivers/iio/adc/max1027.c
> > > > > > +++ b/drivers/iio/adc/max1027.c
> > > > > > @@ -360,6 +360,9 @@ static int
> > max1027_set_trigger_state(struct
> > > > > > iio_trigger *trig, bool state)
> > > > > > struct max1027_state *st = iio_priv(indio_dev);
> > > > > > int ret;
> > > > > >
> > > > > > + if (bitmap_empty(indio_dev->active_scan_mask,
> > indio_dev-
> > > > > > >masklength))
> > > > > > + return -EINVAL;
> > > > > > +
> > > > >
> > > > > I'm not sure this can actually happen. If you try to enable the
> > buffer
> > > > > with no scan element, it should give you an error before you
> > reach
> > > > > this point...
> > > > >
> > > > > > if (state) {
> > > > > > /* Start acquisition on cnvst */
> > > > > > st->reg = MAX1027_SETUP_REG |
> > > > > > MAX1027_CKS_MODE0 |
> > > > > > @@ -368,9 +371,12 @@ 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 0 to the highest requested
> > channel. The
> > > > > > temperature
> > > > > > + * could be avoided but it simplifies a bit the
> > logic.
> > > > > > + */
> > > > > > + st->reg = MAX1027_CONV_REG |
> > > > > > MAX1027_SCAN_0_N | MAX1027_TEMP;
> > > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev-
> > > > > > >active_scan_mask) - 2);
> > > > > > ret = spi_write(st->spi, &st->reg, 1);
> > > > > > if (ret < 0)
> > > > > > return ret;
> > > > > > @@ -391,11 +397,22 @@ 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 = fls(*indio_dev-
> > > > > > >active_scan_mask);
> > > > > > + u16 *buf = st->buffer;
> > > > >
> > > > > I think sparse will complain here. buffer is a __be16 restricted
> > > > > type so you should not mix those...
> > > > > > + unsigned int bit;
> > > > > >
> > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__,
> > irq,
> > > > > >
> > > >
> > private);in/20210818_miquel_raynal_bring_software_triggers_support
> > > > _to_max1027_like_adcs.mbx
> > > > > >
> > > > > > /* fill buffer with all channel */
> > > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength *
> > 2);
> > > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2);
> > > > > > +
> > > > > > + /* Only keep the channels selected by the user */
> > > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > > > > + indio_dev->masklength) {
> > > > > > + if (buf[0] != st->buffer[bit])
> > > > > > + buf[0] = st->buffer[bit];
> > > > >
> > > > > Since we are here, when looking into the driver, I realized
> > > > > that st->buffer is not DMA safe. In IIO, we kind of want to
> > enforce
> > > > > that all buffers that are passed to spi/i2c buses are safe... Maybe
> > > > > this is something you can include in your series.
> > > >
> > > > Why is it not? st->buffer is result of a devm_kmalloc_array() call
> > and
> > > > that should provide a DMA safe buffer as I understand it.
> > > >
> > >
> > > That's a good question. I'm not sure how I came to that conclusion
> > which
> > > is clearly wrong. Though I think the buffer might share the line with
> > the
> > > mutex...
> > Pointer shares a line. The buffer it points to doesn't as allocated
> > by separate heap allocation.
> >
>
> Ups, sure :facepalm:
My understanding [1] was that devm_ allocations were generally not
suitable for DMA and should not be used for this particular purpose
because of the extra 16 bytes allocated for storing the devm magic
somewhere, which shifts the entire buffer and prevents it to always be
aligned on a cache line. I will propose a patch to switch to
kmalloc_array() instead.
[1] https://linux-arm-kernel.infradead.narkive.com/vyJqy0RQ/question-devm-kmalloc-for-dma
Thanks,
Miquèl
Hi Nuno,
"Sa, Nuno" <[email protected]> wrote on Fri, 20 Aug 2021 07:23:33
+0000:
> > -----Original Message-----
> > From: Miquel Raynal <[email protected]>
> > Sent: Wednesday, August 18, 2021 1:12 PM
> > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > <[email protected]>
> > Cc: Thomas Petazzoni <[email protected]>; linux-
> > [email protected]; [email protected]; Miquel Raynal
> > <[email protected]>
> > Subject: [PATCH 11/16] iio: adc: max1027: Separate the IRQ handler
> > from the read logic
> >
> > [External]
> >
> > Create a max1027_read_scan() helper which will make clearer the
> > future IRQ
> > handler updates (no functional change).
> >
> > Signed-off-by: Miquel Raynal <[email protected]>
> > ---
> > drivers/iio/adc/max1027.c | 27 +++++++++++++++++++++------
> > 1 file changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > index 83526f3d7d3a..afc3ce69f7ea 100644
> > --- a/drivers/iio/adc/max1027.c
> > +++ b/drivers/iio/adc/max1027.c
> > @@ -427,19 +427,18 @@ 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 = fls(*indio_dev-
> > >active_scan_mask);
> > u16 *buf = st->buffer;
> > unsigned int bit;
> > -
> > - pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq,
> > private);
> > + int ret;
> >
> > /* 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;
> >
> > /* Only keep the channels selected by the user */
> > for_each_set_bit(bit, indio_dev->active_scan_mask,
> > @@ -451,6 +450,22 @@ static irqreturn_t max1027_trigger_handler(int
> > irq, void *private)
> >
> > 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;
> > +
> > + pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq,
> > private);
> > +
>
> This should be more consistent... use 'dev_err()'. I would also
> argue to use the spi dev as the driver state structure holds a
> pointer to it...
I honestly don't see the point of these debug messages (there is
another useless pr_debug in probe). I kept it here as I am just moving
code around without any changes, but if you don't like it (me neither)
I'll add a simple patch dropping them.
Thanks,
Miquèl
Hi Jonathan,
Jonathan Cameron <[email protected]> wrote on Mon, 30 Aug 2021 11:20:24
+0100:
> On Fri, 20 Aug 2021 07:30:07 +0000
> "Sa, Nuno" <[email protected]> wrote:
>
> > > -----Original Message-----
> > > From: Sa, Nuno <[email protected]>
> > > Sent: Friday, August 20, 2021 9:21 AM
> > > To: Miquel Raynal <[email protected]>; Jonathan Cameron
> > > <[email protected]>; Lars-Peter Clausen <[email protected]>
> > > Cc: Thomas Petazzoni <[email protected]>; linux-
> > > [email protected]; [email protected]
> > > Subject: RE: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> > > accesses during buffer reads
> > >
> > > [External]
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Miquel Raynal <[email protected]>
> > > > Sent: Wednesday, August 18, 2021 1:12 PM
> > > > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > > <[email protected]>
> > > > Cc: Thomas Petazzoni <[email protected]>; linux-
> > > > [email protected]; [email protected]; Miquel Raynal
> > > > <[email protected]>
> > > > Subject: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> > > > accesses during buffer reads
> > > >
> > > > [External]
> > > >
> > > > 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. Prevent it with a iio_buffer_enabled() check.
> > > >
> > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > ---
> > > > drivers/iio/adc/max1027.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > > > index 223c9e4abd86..83526f3d7d3a 100644
> > > > --- a/drivers/iio/adc/max1027.c
> > > > +++ b/drivers/iio/adc/max1027.c
> > > > @@ -335,6 +335,8 @@ static int max1027_read_raw(struct iio_dev
> > > > *indio_dev,
> > > >
> > > > switch (mask) {
> > > > case IIO_CHAN_INFO_RAW:
> > > > + if (iio_buffer_enabled(indio_dev))
> > > > + return -EBUSY;
> > >
> > > I guess 'iio_device_claim_direct_mode()' would be a better option
> > > here? There's nothing preventing this check to pass and then,
> > > concurrently
> > > someone enables the buffer...
> > >
> >
> > Taking a second look, it seems that this check is already done [1]? Am I missing
> > I missing something?
You're right, I missed that too.
> > Also, I think we are returning with the 'st->lock' held...
> >
> > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/max1027.c#L247
> Absolutely agree this should be done with iio_device_claim_direct_mode() to close the
> possible races.
Didn't know this helper, nice.
> I wonder why this one has been missed in all the cleanups of that stuff? Looks like
> a simple case, but I guess it wasn't immediately visible in the read_raw() function
> so no one noticed.
>
> Jonathan
>
Thanks,
Miquèl
Hi Nuno,
"Sa, Nuno" <[email protected]> wrote on Fri, 20 Aug 2021 07:28:17
+0000:
> > -----Original Message-----
> > From: Miquel Raynal <[email protected]>
> > Sent: Wednesday, August 18, 2021 1:12 PM
> > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > <[email protected]>
> > Cc: Thomas Petazzoni <[email protected]>; linux-
> > [email protected]; [email protected]; Miquel Raynal
> > <[email protected]>
> > Subject: [PATCH 12/16] iio: adc: max1027: Introduce an end of
> > conversion helper
> >
> > [External]
> >
> > For now this helper only waits for the maximum duration of a
> > conversion,
> > but it will soon be improved to properly handle the end of conversion
> > interrupt.
> >
> > Signed-off-by: Miquel Raynal <[email protected]>
> > ---
> > drivers/iio/adc/max1027.c | 23 +++++++++++++++++++++--
> > 1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > index afc3ce69f7ea..2d6485591761 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,
> > @@ -236,6 +239,20 @@ struct max1027_state {
> > u8 reg ____cacheline_aligned;
> > };
> >
> > +static DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
> > +
> > +static int max1027_wait_eoc(struct iio_dev *indio_dev)
> > +{
> > + unsigned int conversion_time =
> > MAX1027_CONVERSION_UDELAY;
> > +
> > + if (indio_dev->active_scan_mask)
> > + conversion_time *= hweight32(*indio_dev-
> > >active_scan_mask);
> > +
> > + usleep_range(conversion_time, conversion_time * 2);
> > +
> > + return 0;
> > +}
> > +
> > /* Scan from 0 to the highest requested channel */
> > static int max1027_configure_chans_to_scan(struct iio_dev
> > *indio_dev)
> > {
> > @@ -310,9 +327,11 @@ static int max1027_read_single_value(struct
> > iio_dev *indio_dev,
> > /*
> > * For an unknown reason, when we use the mode "10" (write
> > * conversion register), the interrupt doesn't occur every time.
> > - * So we just wait 1 ms.
> > + * So we just wait the maximum conversion time and deliver
> > the value.
> > */
> > - mdelay(1);
> > + ret = max1027_wait_eoc(indio_dev);
> > + if (ret)
> > + return ret;
>
> I'm a bit confused here... Why are we looking at the active_scan_mask?
> Aren't we preventing this for running concurrently with buffering?
Sorry for the confusion, at this stage the "if (active_masks)" is
useless, I moved it later in the series when we actually start to use
this EOC helper for the buffered reads as well.
Thanks,
Miquèl
Hi Nuno,
"Sa, Nuno" <[email protected]> wrote on Fri, 20 Aug 2021 07:58:25
+0000:
> > -----Original Message-----
> > From: Miquel Raynal <[email protected]>
> > Sent: Wednesday, August 18, 2021 1:12 PM
> > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > <[email protected]>
> > Cc: Thomas Petazzoni <[email protected]>; linux-
> > [email protected]; [email protected]; Miquel Raynal
> > <[email protected]>
> > Subject: [PATCH 15/16] iio: adc: max1027: Support software triggers
> >
> > [External]
> >
> > Now that max1027_trigger_handler() has been freed from handling
> > hardware
> > triggers EOC situations, we can use it for what it has been designed in
> > the first place: trigger software originated conversions. In other
> > words, when userspace initiates a conversion with a sysfs trigger or a
> > hrtimer trigger, we must do all configuration steps, ie:
> > 1- Configuring the trigger
> > 2- Configuring the channels to scan
> > 3- Starting the conversion (actually done automatically by step 2 in
> > this case)
> > 4- Waiting for the conversion to end
> > 5- Retrieving the data from the ADC
> > 6- Push the data to the IIO core and notify it
> >
> > Add the missing steps to this helper and drop the trigger verification
> > hook otherwise software triggers would simply not be accepted at all.
> >
> > Signed-off-by: Miquel Raynal <[email protected]>
> > ---
> > drivers/iio/adc/max1027.c | 26 ++++++++++++++------------
> > 1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > index 8c5995ae59f2..bb437e43adaf 100644
> > --- a/drivers/iio/adc/max1027.c
> > +++ b/drivers/iio/adc/max1027.c
> > @@ -413,17 +413,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);
> > @@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int
> > irq, void *private)
> >
> > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq,
> > private);
> >
> > + ret = max1027_configure_trigger(indio_dev);
> > + if (ret)
> > + goto out;
> > +
> > + ret = max1027_configure_chans_to_scan(indio_dev);
> > + if (ret)
> > + goto out;
> > +
> > + ret = max1027_wait_eoc(indio_dev);
> > + if (ret)
> > + goto out;
> > +
> > ret = max1027_read_scan(indio_dev);
>
> There's something that I'm not getting... How are we checking that
> we have software triggers? This API is called only if the device
> allocates it's own trigger which will happen if there's a spi IRQ.
>
> I'm probably missing something as this series is fairly big but the way
> I would do it is (in the probe):
>
> - always call 'devm_iio_triggered_buffer_setup()' function and properly use
> buffer ops [1] (for example, you can use 'validate_scan_mask()' to setup the
> channels to read);
> - only allocate a trigger if an IRQ is present in which case, we assume HW
> triggering is supported.
I think these are the exact steps that are enforced in the next patch,
I can squash them if you wish but I think it makes sense to have it in
two steps.
Thanks,
Miquèl
Hi Nuno,
"Sa, Nuno" <[email protected]> wrote on Mon, 30 Aug 2021 12:44:48
+0000:
> > -----Original Message-----
> > From: Jonathan Cameron <[email protected]>
> > Sent: Monday, August 30, 2021 12:37 PM
> > To: Miquel Raynal <[email protected]>
> > Cc: Lars-Peter Clausen <[email protected]>; Thomas Petazzoni
> > <[email protected]>; [email protected]; linux-
> > [email protected]
> > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end of
> > conversion helper
> >
> > On Wed, 18 Aug 2021 13:11:37 +0200
> > Miquel Raynal <[email protected]> wrote:
> >
> > > Now that we have a dedicated handler for End Of Conversion
> > interrupts,
> > > let's create a second path:
> > > - Situation 1: we are using the external hardware trigger, a
> > conversion
> > > has been triggered and the ADC pushed the data to its FIFO, we
> > need to
> > > retrieve the data and push it to the IIO buffers.
> > > - Situation 2: we are not using the external hardware trigger, hence
> > we
> > > are likely waiting in a blocked thread waiting for this interrupt to
> > > happen: in this case we just wake up the waiting thread.
> > >
> > > Signed-off-by: Miquel Raynal <[email protected]>
> > > ---
> > > drivers/iio/adc/max1027.c | 20 +++++++++++++++++---
> > > 1 file changed, 17 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > > index 8d86e77fb5db..8c5995ae59f2 100644
> > > --- a/drivers/iio/adc/max1027.c
> > > +++ b/drivers/iio/adc/max1027.c
> > > @@ -235,6 +235,7 @@ struct max1027_state {
> > > struct iio_trigger *trig;
> > > __be16 *buffer;
> > > struct mutex lock;
> > > + bool data_rdy;
> > > bool cnvst_trigger;
> > > u8 reg ____cacheline_aligned;
> > > };
> > > @@ -243,12 +244,22 @@ static
> > DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
> > >
> > > 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;
> > >
> > > - if (indio_dev->active_scan_mask)
> > > - conversion_time *= hweight32(*indio_dev-
> > >active_scan_mask);
> > > + if (st->spi->irq) {
> > > + ret =
> > wait_event_interruptible_timeout(max1027_queue,
> > > + st->data_rdy, HZ /
> > 1000);
> > > + st->data_rdy = false;
> > > + if (ret == -ERESTARTSYS)
> > > + return ret;
> > > + } else {
> > > + if (indio_dev->active_scan_mask)
> > > + conversion_time *= hweight32(*indio_dev-
> > >active_scan_mask);
> > >
> > > - usleep_range(conversion_time, conversion_time * 2);
> > > + usleep_range(conversion_time, conversion_time * 2);
> > > + }
> > >
> > > return 0;
> > > }
> > > @@ -481,6 +492,9 @@ static irqreturn_t
> > max1027_eoc_irq_handler(int irq, void *private)
> > > if (st->cnvst_trigger) {
> > > ret = max1027_read_scan(indio_dev);
> > > iio_trigger_notify_done(indio_dev->trig);
> > > + } else {
> > > + st->data_rdy = true;
> > > + wake_up(&max1027_queue);
> >
> > I can't see why a queue is appropriate for this. Use a completion and
> > have
> > one per instance of the device. No need for the flag etc in that case as
> > complete() means we have had an interrupt.
> >
>
> In the case that 'st-> cnvst_trigger' is not set but the spi IRQ
> is present, we will wait until we get 'wake_up()' called from here. I wonder if
> that is a good idea as the device own trigger is not being used. FWIW, I think this
> sync logic is a bit confusing... I would still use the normal trigger infrastructure
> ('iio_trigger_generic_data_rdy_poll()') and use the 'cnvst_trigger' flag in the
> trigger handler to manually start conversions + wait till eoc. But I might be missing
> something though.
I implemented it your way, but I think I found a situation that was not
fully handled (the 3rd), which makes the handler very complicated
as we need to handle all the following cases:
1/ no trigger, irq enabled -> single read EOC interrupt
2/ external trigger, no irq -> handle the whole conversion process
3/ external trigger, irq enabled -> handle the whole conversion process
but also have a dedicated condition to handle the EOC interrupt
properly (fortunately this is a threaded handler that can be
preempted): we need to wait from the handler itself that the
handler gets called again: the first time it is executed as
"pollfunc", the second time as "EOC interrupt". In the second
instance, call complete() in order to deliver the first running
instance of the handler and continue until the reading part.
4/ cnvst trigger, irq enabled -> only reads the data.
5/ cnvst trigger, irq disabled -> not possible.
I added a lot of comments to make it clearer.
> Regarding this handler, I just realized that this is the hard IRQ handler which
> might end up calling 'max1027_read_scan()' which in turn calls 'spi_read()'. Am I
> missing something here?
I renamed it to make it clear, but this is already a threaded handler.
Thanks,
Miquèl
Hi Jonathan,
Jonathan Cameron <[email protected]> wrote on Mon, 30 Aug 2021 11:50:46
+0100:
> On Wed, 18 Aug 2021 13:11:38 +0200
> Miquel Raynal <[email protected]> wrote:
>
> > Now that max1027_trigger_handler() has been freed from handling hardware
> > triggers EOC situations, we can use it for what it has been designed in
> > the first place: trigger software originated conversions.
>
> As mentioned earlier, this is not how I'd normally expect this sort of
> case to be handled. I'd be expecting the cnvst trigger to still be calling
> this function and the function to do the relevant check to ensure it
> knows the data is already available in that case.
I tried to follow your advice and Nuno's regarding this, I hope the new
version will match your expectations (new version coming soon).
However if my changes do not match, I will probably need more guidance
to understand in deep what you suggest.
> > In other
> > words, when userspace initiates a conversion with a sysfs trigger or a
> > hrtimer trigger, we must do all configuration steps, ie:
> > 1- Configuring the trigger
> > 2- Configuring the channels to scan
> > 3- Starting the conversion (actually done automatically by step 2 in
> > this case)
> > 4- Waiting for the conversion to end
> > 5- Retrieving the data from the ADC
> > 6- Push the data to the IIO core and notify it
> >
> > Add the missing steps to this helper and drop the trigger verification
> > hook otherwise software triggers would simply not be accepted at all.
> >
> > Signed-off-by: Miquel Raynal <[email protected]>
> > ---
> > drivers/iio/adc/max1027.c | 26 ++++++++++++++------------
> > 1 file changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > index 8c5995ae59f2..bb437e43adaf 100644
> > --- a/drivers/iio/adc/max1027.c
> > +++ b/drivers/iio/adc/max1027.c
> > @@ -413,17 +413,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);
> > @@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private)
> >
> > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private);
> >
> > + ret = max1027_configure_trigger(indio_dev);
>
> I'd not expect to see this ever time. The configuration shouldn't change
> from one call of this function to the next.
True, this is not needed.
> > + if (ret)
> > + goto out;
> > +
> > + ret = max1027_configure_chans_to_scan(indio_dev);
>
> This should also not change unless it is also responsible for the 'go' signal.
> If that's true then it is badly named.
It's responsible for the go signal, I renamed it "configure_and_start".
However, just for my own understanding, when would I be supposed to
configure the channels requested by the user otherwise?
Thanks,
Miquèl
Hi Miquel,
> From: Miquel Raynal <[email protected]>
> Sent: Thursday, September 2, 2021 5:13 PM
> To: Sa, Nuno <[email protected]>
> Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>; Thomas Petazzoni
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end of
> conversion helper
>
> Hi Nuno,
>
> "Sa, Nuno" <[email protected]> wrote on Mon, 30 Aug 2021
> 12:44:48
> +0000:
>
> > > -----Original Message-----
> > > From: Jonathan Cameron <[email protected]>
> > > Sent: Monday, August 30, 2021 12:37 PM
> > > To: Miquel Raynal <[email protected]>
> > > Cc: Lars-Peter Clausen <[email protected]>; Thomas Petazzoni
> > > <[email protected]>; [email protected];
> linux-
> > > [email protected]
> > > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end
> of
> > > conversion helper
> > >
> > > On Wed, 18 Aug 2021 13:11:37 +0200
> > > Miquel Raynal <[email protected]> wrote:
> > >
> > > > Now that we have a dedicated handler for End Of Conversion
> > > interrupts,
> > > > let's create a second path:
> > > > - Situation 1: we are using the external hardware trigger, a
> > > conversion
> > > > has been triggered and the ADC pushed the data to its FIFO, we
> > > need to
> > > > retrieve the data and push it to the IIO buffers.
> > > > - Situation 2: we are not using the external hardware trigger,
> hence
> > > we
> > > > are likely waiting in a blocked thread waiting for this interrupt to
> > > > happen: in this case we just wake up the waiting thread.
> > > >
> > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > ---
> > > > drivers/iio/adc/max1027.c | 20 +++++++++++++++++---
> > > > 1 file changed, 17 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/adc/max1027.c
> b/drivers/iio/adc/max1027.c
> > > > index 8d86e77fb5db..8c5995ae59f2 100644
> > > > --- a/drivers/iio/adc/max1027.c
> > > > +++ b/drivers/iio/adc/max1027.c
> > > > @@ -235,6 +235,7 @@ struct max1027_state {
> > > > struct iio_trigger *trig;
> > > > __be16 *buffer;
> > > > struct mutex lock;
> > > > + bool data_rdy;
> > > > bool cnvst_trigger;
> > > > u8 reg ____cacheline_aligned;
> > > > };
> > > > @@ -243,12 +244,22 @@ static
> > > DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
> > > >
> > > > 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;
> > > >
> > > > - if (indio_dev->active_scan_mask)
> > > > - conversion_time *= hweight32(*indio_dev-
> > > >active_scan_mask);
> > > > + if (st->spi->irq) {
> > > > + ret =
> > > wait_event_interruptible_timeout(max1027_queue,
> > > > + st->data_rdy, HZ /
> > > 1000);
> > > > + st->data_rdy = false;
> > > > + if (ret == -ERESTARTSYS)
> > > > + return ret;
> > > > + } else {
> > > > + if (indio_dev->active_scan_mask)
> > > > + conversion_time *= hweight32(*indio_dev-
> > > >active_scan_mask);
> > > >
> > > > - usleep_range(conversion_time, conversion_time * 2);
> > > > + usleep_range(conversion_time, conversion_time * 2);
> > > > + }
> > > >
> > > > return 0;
> > > > }
> > > > @@ -481,6 +492,9 @@ static irqreturn_t
> > > max1027_eoc_irq_handler(int irq, void *private)
> > > > if (st->cnvst_trigger) {
> > > > ret = max1027_read_scan(indio_dev);
> > > > iio_trigger_notify_done(indio_dev->trig);
> > > > + } else {
> > > > + st->data_rdy = true;
> > > > + wake_up(&max1027_queue);
> > >
> > > I can't see why a queue is appropriate for this. Use a completion
> and
> > > have
> > > one per instance of the device. No need for the flag etc in that
> case as
> > > complete() means we have had an interrupt.
> > >
> >
> > In the case that 'st-> cnvst_trigger' is not set but the spi IRQ
> > is present, we will wait until we get 'wake_up()' called from here. I
> wonder if
> > that is a good idea as the device own trigger is not being used. FWIW,
> I think this
> > sync logic is a bit confusing... I would still use the normal trigger
> infrastructure
> > ('iio_trigger_generic_data_rdy_poll()') and use the 'cnvst_trigger'
> flag in the
> > trigger handler to manually start conversions + wait till eoc. But I
> might be missing
> > something though.
>
> I implemented it your way, but I think I found a situation that was not
> fully handled (the 3rd), which makes the handler very complicated
> as we need to handle all the following cases:
> 1/ no trigger, irq enabled -> single read EOC interrupt
> 2/ external trigger, no irq -> handle the whole conversion process
> 3/ external trigger, irq enabled -> handle the whole conversion process
> but also have a dedicated condition to handle the EOC interrupt
> properly (fortunately this is a threaded handler that can be
> preempted): we need to wait from the handler itself that the
> handler gets called again: the first time it is executed as
> "pollfunc", the second time as "EOC interrupt". In the second
> instance, call complete() in order to deliver the first running
> instance of the handler and continue until the reading part.
> 4/ cnvst trigger, irq enabled -> only reads the data.
> 5/ cnvst trigger, irq disabled -> not possible.
>
> I added a lot of comments to make it clearer.
>
> > Regarding this handler, I just realized that this is the hard IRQ handler
> which
> > might end up calling 'max1027_read_scan()' which in turn calls
> 'spi_read()'. Am I
> > missing something here?
>
> I renamed it to make it clear, but this is already a threaded handler.
>
Hmm, I think I get what you're trying to do.... FWIW, I think you're just going
into a lot of trouble here for scenario 3 (I assume external trigger is something
else other the device own one). IMO, I would just assume that if we are using
an external trigger we have to wait (sleep) for the end of conversion (i.e, I would
not care about the IRQ in this case). It would make things much more simpler and
I guess it should be expected that if some user is deliberately not using the
device own trigger, will have to wait more for scans.
I cannot also see a reason why someone would want to use some
external trigger if the device one is available... Does it really make sense?
- Nuno Sá
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Thursday, September 2, 2021 2:25 PM
> To: Sa, Nuno <[email protected]>
> Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>; Thomas Petazzoni
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 15/16] iio: adc: max1027: Support software
> triggers
>
> [External]
>
> Hi Nuno,
>
> "Sa, Nuno" <[email protected]> wrote on Fri, 20 Aug 2021 07:58:25
> +0000:
>
> > > -----Original Message-----
> > > From: Miquel Raynal <[email protected]>
> > > Sent: Wednesday, August 18, 2021 1:12 PM
> > > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > <[email protected]>
> > > Cc: Thomas Petazzoni <[email protected]>; linux-
> > > [email protected]; [email protected]; Miquel Raynal
> > > <[email protected]>
> > > Subject: [PATCH 15/16] iio: adc: max1027: Support software triggers
> > >
> > > [External]
> > >
> > > Now that max1027_trigger_handler() has been freed from
> handling
> > > hardware
> > > triggers EOC situations, we can use it for what it has been designed
> in
> > > the first place: trigger software originated conversions. In other
> > > words, when userspace initiates a conversion with a sysfs trigger or
> a
> > > hrtimer trigger, we must do all configuration steps, ie:
> > > 1- Configuring the trigger
> > > 2- Configuring the channels to scan
> > > 3- Starting the conversion (actually done automatically by step 2 in
> > > this case)
> > > 4- Waiting for the conversion to end
> > > 5- Retrieving the data from the ADC
> > > 6- Push the data to the IIO core and notify it
> > >
> > > Add the missing steps to this helper and drop the trigger
> verification
> > > hook otherwise software triggers would simply not be accepted at
> all.
> > >
> > > Signed-off-by: Miquel Raynal <[email protected]>
> > > ---
> > > drivers/iio/adc/max1027.c | 26 ++++++++++++++------------
> > > 1 file changed, 14 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > > index 8c5995ae59f2..bb437e43adaf 100644
> > > --- a/drivers/iio/adc/max1027.c
> > > +++ b/drivers/iio/adc/max1027.c
> > > @@ -413,17 +413,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);
> > > @@ -512,7 +501,21 @@ static irqreturn_t
> max1027_trigger_handler(int
> > > irq, void *private)
> > >
> > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq,
> > > private);
> > >
> > > + ret = max1027_configure_trigger(indio_dev);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + ret = max1027_configure_chans_to_scan(indio_dev);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + ret = max1027_wait_eoc(indio_dev);
> > > + if (ret)
> > > + goto out;
> > > +
> > > ret = max1027_read_scan(indio_dev);
> >
> > There's something that I'm not getting... How are we checking that
> > we have software triggers? This API is called only if the device
> > allocates it's own trigger which will happen if there's a spi IRQ.
> >
> > I'm probably missing something as this series is fairly big but the way
> > I would do it is (in the probe):
> >
> > - always call 'devm_iio_triggered_buffer_setup()' function and
> properly use
> > buffer ops [1] (for example, you can use 'validate_scan_mask()' to
> setup the
> > channels to read);
> > - only allocate a trigger if an IRQ is present in which case, we assume
> HW
> > triggering is supported.
>
> I think these are the exact steps that are enforced in the next patch,
> I can squash them if you wish but I think it makes sense to have it in
> two steps.
>
Yeah, my bad here...
- Nuno Sá
Hi Nuno,
[email protected] wrote on Fri, 3 Sep 2021 14:28:52 +0000:
> Hi Miquel,
>
> > From: Miquel Raynal <[email protected]>
> > Sent: Thursday, September 2, 2021 5:13 PM
> > To: Sa, Nuno <[email protected]>
> > Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > <[email protected]>; Thomas Petazzoni
> > <[email protected]>; [email protected]; linux-
> > [email protected]
> > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end of
> > conversion helper
> >
> > Hi Nuno,
> >
> > "Sa, Nuno" <[email protected]> wrote on Mon, 30 Aug 2021
> > 12:44:48
> > +0000:
> >
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <[email protected]>
> > > > Sent: Monday, August 30, 2021 12:37 PM
> > > > To: Miquel Raynal <[email protected]>
> > > > Cc: Lars-Peter Clausen <[email protected]>; Thomas Petazzoni
> > > > <[email protected]>; [email protected];
> > linux-
> > > > [email protected]
> > > > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end
> > of
> > > > conversion helper
> > > >
> > > > On Wed, 18 Aug 2021 13:11:37 +0200
> > > > Miquel Raynal <[email protected]> wrote:
> > > >
> > > > > Now that we have a dedicated handler for End Of Conversion
> > > > interrupts,
> > > > > let's create a second path:
> > > > > - Situation 1: we are using the external hardware trigger, a
> > > > conversion
> > > > > has been triggered and the ADC pushed the data to its FIFO, we
> > > > need to
> > > > > retrieve the data and push it to the IIO buffers.
> > > > > - Situation 2: we are not using the external hardware trigger,
> > hence
> > > > we
> > > > > are likely waiting in a blocked thread waiting for this interrupt to
> > > > > happen: in this case we just wake up the waiting thread.
> > > > >
> > > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > > ---
> > > > > drivers/iio/adc/max1027.c | 20 +++++++++++++++++---
> > > > > 1 file changed, 17 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/adc/max1027.c
> > b/drivers/iio/adc/max1027.c
> > > > > index 8d86e77fb5db..8c5995ae59f2 100644
> > > > > --- a/drivers/iio/adc/max1027.c
> > > > > +++ b/drivers/iio/adc/max1027.c
> > > > > @@ -235,6 +235,7 @@ struct max1027_state {
> > > > > struct iio_trigger *trig;
> > > > > __be16 *buffer;
> > > > > struct mutex lock;
> > > > > + bool data_rdy;
> > > > > bool cnvst_trigger;
> > > > > u8 reg ____cacheline_aligned;
> > > > > };
> > > > > @@ -243,12 +244,22 @@ static
> > > > DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
> > > > >
> > > > > 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;
> > > > >
> > > > > - if (indio_dev->active_scan_mask)
> > > > > - conversion_time *= hweight32(*indio_dev-
> > > > >active_scan_mask);
> > > > > + if (st->spi->irq) {
> > > > > + ret =
> > > > wait_event_interruptible_timeout(max1027_queue,
> > > > > + st->data_rdy, HZ /
> > > > 1000);
> > > > > + st->data_rdy = false;
> > > > > + if (ret == -ERESTARTSYS)
> > > > > + return ret;
> > > > > + } else {
> > > > > + if (indio_dev->active_scan_mask)
> > > > > + conversion_time *= hweight32(*indio_dev-
> > > > >active_scan_mask);
> > > > >
> > > > > - usleep_range(conversion_time, conversion_time * 2);
> > > > > + usleep_range(conversion_time, conversion_time * 2);
> > > > > + }
> > > > >
> > > > > return 0;
> > > > > }
> > > > > @@ -481,6 +492,9 @@ static irqreturn_t
> > > > max1027_eoc_irq_handler(int irq, void *private)
> > > > > if (st->cnvst_trigger) {
> > > > > ret = max1027_read_scan(indio_dev);
> > > > > iio_trigger_notify_done(indio_dev->trig);
> > > > > + } else {
> > > > > + st->data_rdy = true;
> > > > > + wake_up(&max1027_queue);
> > > >
> > > > I can't see why a queue is appropriate for this. Use a completion
> > and
> > > > have
> > > > one per instance of the device. No need for the flag etc in that
> > case as
> > > > complete() means we have had an interrupt.
> > > >
> > >
> > > In the case that 'st-> cnvst_trigger' is not set but the spi IRQ
> > > is present, we will wait until we get 'wake_up()' called from here. I
> > wonder if
> > > that is a good idea as the device own trigger is not being used. FWIW,
> > I think this
> > > sync logic is a bit confusing... I would still use the normal trigger
> > infrastructure
> > > ('iio_trigger_generic_data_rdy_poll()') and use the 'cnvst_trigger'
> > flag in the
> > > trigger handler to manually start conversions + wait till eoc. But I
> > might be missing
> > > something though.
> >
> > I implemented it your way, but I think I found a situation that was not
> > fully handled (the 3rd), which makes the handler very complicated
> > as we need to handle all the following cases:
> > 1/ no trigger, irq enabled -> single read EOC interrupt
> > 2/ external trigger, no irq -> handle the whole conversion process
> > 3/ external trigger, irq enabled -> handle the whole conversion process
> > but also have a dedicated condition to handle the EOC interrupt
> > properly (fortunately this is a threaded handler that can be
> > preempted): we need to wait from the handler itself that the
> > handler gets called again: the first time it is executed as
> > "pollfunc", the second time as "EOC interrupt". In the second
> > instance, call complete() in order to deliver the first running
> > instance of the handler and continue until the reading part.
> > 4/ cnvst trigger, irq enabled -> only reads the data.
> > 5/ cnvst trigger, irq disabled -> not possible.
> >
> > I added a lot of comments to make it clearer.
> >
> > > Regarding this handler, I just realized that this is the hard IRQ handler
> > which
> > > might end up calling 'max1027_read_scan()' which in turn calls
> > 'spi_read()'. Am I
> > > missing something here?
> >
> > I renamed it to make it clear, but this is already a threaded handler.
> >
>
> Hmm, I think I get what you're trying to do.... FWIW, I think you're just going
> into a lot of trouble here for scenario 3 (I assume external trigger is something
> else other the device own one). IMO, I would just assume that if we are using
> an external trigger we have to wait (sleep) for the end of conversion (i.e, I would
> not care about the IRQ in this case). It would make things much more simpler and
> I guess it should be expected that if some user is deliberately not using the
> device own trigger, will have to wait more for scans.
I thought about that, but this means playing with enabling/disabling
IRQs, which in the end I fear could be almost as verbose :/
Can you look at the new implementation that I proposed and give me your
feedback on it?
[PATCH v2 15/16] iio: adc: max1027: Add support for external triggers
> I cannot also see a reason why someone would want to use some
> external trigger if the device one is available... Does it really make sense?
I would say that "genericity" is a good enough reason for that, but in
practice I agree with you.
Thanks,
Miquèl
On Wed, 1 Sep 2021 10:12:09 +0200
Miquel Raynal <[email protected]> wrote:
> Hello,
>
> "Sa, Nuno" <[email protected]> wrote on Mon, 30 Aug 2021 15:02:26
> +0000:
>
> > > -----Original Message-----
> > > From: Jonathan Cameron <[email protected]>
> > > Sent: Monday, August 30, 2021 4:30 PM
> > > To: Sa, Nuno <[email protected]>
> > > Cc: Miquel Raynal <[email protected]>; Lars-Peter Clausen
> > > <[email protected]>; Thomas Petazzoni
> > > <[email protected]>; [email protected]; linux-
> > > [email protected]
> > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested
> > > samples
> > >
> > > [External]
> > >
> > > On Mon, 30 Aug 2021 10:49:50 +0000
> > > "Sa, Nuno" <[email protected]> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Jonathan Cameron <[email protected]>
> > > > > Sent: Monday, August 30, 2021 12:08 PM
> > > > > To: Sa, Nuno <[email protected]>
> > > > > Cc: Miquel Raynal <[email protected]>; Lars-Peter
> > > Clausen
> > > > > <[email protected]>; Thomas Petazzoni
> > > > > <[email protected]>; [email protected];
> > > linux-
> > > > > [email protected]
> > > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the
> > > requested
> > > > > samples
> > > > >
> > > > > [External]
> > > > >
> > > > > On Fri, 20 Aug 2021 07:10:48 +0000
> > > > > "Sa, Nuno" <[email protected]> wrote:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Miquel Raynal <[email protected]>
> > > > > > > Sent: Wednesday, August 18, 2021 1:11 PM
> > > > > > > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > > > > > <[email protected]>
> > > > > > > Cc: Thomas Petazzoni <[email protected]>; linux-
> > > > > > > [email protected]; [email protected]; Miquel
> > > Raynal
> > > > > > > <[email protected]>
> > > > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the
> > > requested
> > > > > > > samples
> > > > > > >
> > > > > > > [External]
> > > > > > >
> > > > > > > When a triggered scan occurs, the identity of the desired
> > > channels
> > > > > is
> > > > > > > known in indio_dev->active_scan_mask. Instead of reading and
> > > > > > > pushing to
> > > > > > > the IIO buffers all channels each time, scan the minimum
> > > amount
> > > > > of
> > > > > > > channels (0 to maximum requested chan, to be exact) and only
> > > > > > > provide the
> > > > > > > samples requested by the user.
> > > > > > >
> > > > > > > For example, if the user wants channels 1, 4 and 5, all channels
> > > > > from
> > > > > > > 0 to 5 will be scanned but only the desired channels will be
> > > pushed
> > > > > to
> > > > > > > the IIO buffers.
> > > > > > >
> > > > > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > > > > ---
> > > > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++----
> > > > > > > 1 file changed, 21 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/iio/adc/max1027.c
> > > b/drivers/iio/adc/max1027.c
> > > > > > > index b753658bb41e..8ab660f596b5 100644
> > > > > > > --- a/drivers/iio/adc/max1027.c
> > > > > > > +++ b/drivers/iio/adc/max1027.c
> > > > > > > @@ -360,6 +360,9 @@ static int
> > > max1027_set_trigger_state(struct
> > > > > > > iio_trigger *trig, bool state)
> > > > > > > struct max1027_state *st = iio_priv(indio_dev);
> > > > > > > int ret;
> > > > > > >
> > > > > > > + if (bitmap_empty(indio_dev->active_scan_mask,
> > > indio_dev-
> > > > > > > >masklength))
> > > > > > > + return -EINVAL;
> > > > > > > +
> > > > > >
> > > > > > I'm not sure this can actually happen. If you try to enable the
> > > buffer
> > > > > > with no scan element, it should give you an error before you
> > > reach
> > > > > > this point...
> > > > > >
> > > > > > > if (state) {
> > > > > > > /* Start acquisition on cnvst */
> > > > > > > st->reg = MAX1027_SETUP_REG |
> > > > > > > MAX1027_CKS_MODE0 |
> > > > > > > @@ -368,9 +371,12 @@ 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 0 to the highest requested
> > > channel. The
> > > > > > > temperature
> > > > > > > + * could be avoided but it simplifies a bit the
> > > logic.
> > > > > > > + */
> > > > > > > + st->reg = MAX1027_CONV_REG |
> > > > > > > MAX1027_SCAN_0_N | MAX1027_TEMP;
> > > > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev-
> > > > > > > >active_scan_mask) - 2);
> > > > > > > ret = spi_write(st->spi, &st->reg, 1);
> > > > > > > if (ret < 0)
> > > > > > > return ret;
> > > > > > > @@ -391,11 +397,22 @@ 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 = fls(*indio_dev-
> > > > > > > >active_scan_mask);
> > > > > > > + u16 *buf = st->buffer;
> > > > > >
> > > > > > I think sparse will complain here. buffer is a __be16 restricted
> > > > > > type so you should not mix those...
> > > > > > > + unsigned int bit;
> > > > > > >
> > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__,
> > > irq,
> > > > > > >
> > > > >
> > > private);in/20210818_miquel_raynal_bring_software_triggers_support
> > > > > _to_max1027_like_adcs.mbx
> > > > > > >
> > > > > > > /* fill buffer with all channel */
> > > > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength *
> > > 2);
> > > > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2);
> > > > > > > +
> > > > > > > + /* Only keep the channels selected by the user */
> > > > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > > > > > + indio_dev->masklength) {
> > > > > > > + if (buf[0] != st->buffer[bit])
> > > > > > > + buf[0] = st->buffer[bit];
> > > > > >
> > > > > > Since we are here, when looking into the driver, I realized
> > > > > > that st->buffer is not DMA safe. In IIO, we kind of want to
> > > enforce
> > > > > > that all buffers that are passed to spi/i2c buses are safe... Maybe
> > > > > > this is something you can include in your series.
> > > > >
> > > > > Why is it not? st->buffer is result of a devm_kmalloc_array() call
> > > and
> > > > > that should provide a DMA safe buffer as I understand it.
> > > > >
> > > >
> > > > That's a good question. I'm not sure how I came to that conclusion
> > > which
> > > > is clearly wrong. Though I think the buffer might share the line with
> > > the
> > > > mutex...
> > > Pointer shares a line. The buffer it points to doesn't as allocated
> > > by separate heap allocation.
> > >
> >
> > Ups, sure :facepalm:
>
> My understanding [1] was that devm_ allocations were generally not
> suitable for DMA and should not be used for this particular purpose
> because of the extra 16 bytes allocated for storing the devm magic
> somewhere, which shifts the entire buffer and prevents it to always be
> aligned on a cache line. I will propose a patch to switch to
> kmalloc_array() instead.
>
> [1] https://linux-arm-kernel.infradead.narkive.com/vyJqy0RQ/question-devm-kmalloc-for-dma
That shouldn't actually matter because here we don't care about
it being aligned on a cacheline - but we do care about it being
aligned so that nothing else we might touch is in the same cacheline.
Note the thread you link talks about this.
If we were possibly doing additional devm_ managed allocations
whilst DMA was active then it might be a problem, but
I'm fairly sure we aren't doing that here.
Not there might be a bus controller that has stricter alignment
requirements - whether we need to be careful of those isn't
particularly clear to me. There are lots of places in IIO
where we cachealign a set of buffers, but for example the
rx is after the tx buffers and isn't aligned as would be
required. As such I'm fairly sure it's not a problem.
Jonathan
>
> Thanks,
> Miquèl
On Thu, 2 Sep 2021 10:55:19 +0200
Miquel Raynal <[email protected]> wrote:
> Hi Nuno,
>
> "Sa, Nuno" <[email protected]> wrote on Fri, 20 Aug 2021 07:23:33
> +0000:
>
> > > -----Original Message-----
> > > From: Miquel Raynal <[email protected]>
> > > Sent: Wednesday, August 18, 2021 1:12 PM
> > > To: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > <[email protected]>
> > > Cc: Thomas Petazzoni <[email protected]>; linux-
> > > [email protected]; [email protected]; Miquel Raynal
> > > <[email protected]>
> > > Subject: [PATCH 11/16] iio: adc: max1027: Separate the IRQ handler
> > > from the read logic
> > >
> > > [External]
> > >
> > > Create a max1027_read_scan() helper which will make clearer the
> > > future IRQ
> > > handler updates (no functional change).
> > >
> > > Signed-off-by: Miquel Raynal <[email protected]>
> > > ---
> > > drivers/iio/adc/max1027.c | 27 +++++++++++++++++++++------
> > > 1 file changed, 21 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > > index 83526f3d7d3a..afc3ce69f7ea 100644
> > > --- a/drivers/iio/adc/max1027.c
> > > +++ b/drivers/iio/adc/max1027.c
> > > @@ -427,19 +427,18 @@ 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 = fls(*indio_dev-
> > > >active_scan_mask);
> > > u16 *buf = st->buffer;
> > > unsigned int bit;
> > > -
> > > - pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq,
> > > private);
> > > + int ret;
> > >
> > > /* 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;
> > >
> > > /* Only keep the channels selected by the user */
> > > for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > @@ -451,6 +450,22 @@ static irqreturn_t max1027_trigger_handler(int
> > > irq, void *private)
> > >
> > > 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;
> > > +
> > > + pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq,
> > > private);
> > > +
> >
> > This should be more consistent... use 'dev_err()'. I would also
> > argue to use the spi dev as the driver state structure holds a
> > pointer to it...
>
> I honestly don't see the point of these debug messages (there is
> another useless pr_debug in probe). I kept it here as I am just moving
> code around without any changes, but if you don't like it (me neither)
> I'll add a simple patch dropping them.
Go for it :)
>
> Thanks,
> Miquèl
On Fri, 3 Sep 2021 16:46:50 +0200
Miquel Raynal <[email protected]> wrote:
> Hi Nuno,
>
> [email protected] wrote on Fri, 3 Sep 2021 14:28:52 +0000:
>
> > Hi Miquel,
> >
> > > From: Miquel Raynal <[email protected]>
> > > Sent: Thursday, September 2, 2021 5:13 PM
> > > To: Sa, Nuno <[email protected]>
> > > Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > <[email protected]>; Thomas Petazzoni
> > > <[email protected]>; [email protected]; linux-
> > > [email protected]
> > > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end of
> > > conversion helper
> > >
> > > Hi Nuno,
> > >
> > > "Sa, Nuno" <[email protected]> wrote on Mon, 30 Aug 2021
> > > 12:44:48
> > > +0000:
> > >
> > > > > -----Original Message-----
> > > > > From: Jonathan Cameron <[email protected]>
> > > > > Sent: Monday, August 30, 2021 12:37 PM
> > > > > To: Miquel Raynal <[email protected]>
> > > > > Cc: Lars-Peter Clausen <[email protected]>; Thomas Petazzoni
> > > > > <[email protected]>; [email protected];
> > > linux-
> > > > > [email protected]
> > > > > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end
> > > of
> > > > > conversion helper
> > > > >
> > > > > On Wed, 18 Aug 2021 13:11:37 +0200
> > > > > Miquel Raynal <[email protected]> wrote:
> > > > >
> > > > > > Now that we have a dedicated handler for End Of Conversion
> > > > > interrupts,
> > > > > > let's create a second path:
> > > > > > - Situation 1: we are using the external hardware trigger, a
> > > > > conversion
> > > > > > has been triggered and the ADC pushed the data to its FIFO, we
> > > > > need to
> > > > > > retrieve the data and push it to the IIO buffers.
> > > > > > - Situation 2: we are not using the external hardware trigger,
> > > hence
> > > > > we
> > > > > > are likely waiting in a blocked thread waiting for this interrupt to
> > > > > > happen: in this case we just wake up the waiting thread.
> > > > > >
> > > > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > > > ---
> > > > > > drivers/iio/adc/max1027.c | 20 +++++++++++++++++---
> > > > > > 1 file changed, 17 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iio/adc/max1027.c
> > > b/drivers/iio/adc/max1027.c
> > > > > > index 8d86e77fb5db..8c5995ae59f2 100644
> > > > > > --- a/drivers/iio/adc/max1027.c
> > > > > > +++ b/drivers/iio/adc/max1027.c
> > > > > > @@ -235,6 +235,7 @@ struct max1027_state {
> > > > > > struct iio_trigger *trig;
> > > > > > __be16 *buffer;
> > > > > > struct mutex lock;
> > > > > > + bool data_rdy;
> > > > > > bool cnvst_trigger;
> > > > > > u8 reg ____cacheline_aligned;
> > > > > > };
> > > > > > @@ -243,12 +244,22 @@ static
> > > > > DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
> > > > > >
> > > > > > 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;
> > > > > >
> > > > > > - if (indio_dev->active_scan_mask)
> > > > > > - conversion_time *= hweight32(*indio_dev-
> > > > > >active_scan_mask);
> > > > > > + if (st->spi->irq) {
> > > > > > + ret =
> > > > > wait_event_interruptible_timeout(max1027_queue,
> > > > > > + st->data_rdy, HZ /
> > > > > 1000);
> > > > > > + st->data_rdy = false;
> > > > > > + if (ret == -ERESTARTSYS)
> > > > > > + return ret;
> > > > > > + } else {
> > > > > > + if (indio_dev->active_scan_mask)
> > > > > > + conversion_time *= hweight32(*indio_dev-
> > > > > >active_scan_mask);
> > > > > >
> > > > > > - usleep_range(conversion_time, conversion_time * 2);
> > > > > > + usleep_range(conversion_time, conversion_time * 2);
> > > > > > + }
> > > > > >
> > > > > > return 0;
> > > > > > }
> > > > > > @@ -481,6 +492,9 @@ static irqreturn_t
> > > > > max1027_eoc_irq_handler(int irq, void *private)
> > > > > > if (st->cnvst_trigger) {
> > > > > > ret = max1027_read_scan(indio_dev);
> > > > > > iio_trigger_notify_done(indio_dev->trig);
> > > > > > + } else {
> > > > > > + st->data_rdy = true;
> > > > > > + wake_up(&max1027_queue);
> > > > >
> > > > > I can't see why a queue is appropriate for this. Use a completion
> > > and
> > > > > have
> > > > > one per instance of the device. No need for the flag etc in that
> > > case as
> > > > > complete() means we have had an interrupt.
> > > > >
> > > >
> > > > In the case that 'st-> cnvst_trigger' is not set but the spi IRQ
> > > > is present, we will wait until we get 'wake_up()' called from here. I
> > > wonder if
> > > > that is a good idea as the device own trigger is not being used. FWIW,
> > > I think this
> > > > sync logic is a bit confusing... I would still use the normal trigger
> > > infrastructure
> > > > ('iio_trigger_generic_data_rdy_poll()') and use the 'cnvst_trigger'
> > > flag in the
> > > > trigger handler to manually start conversions + wait till eoc. But I
> > > might be missing
> > > > something though.
> > >
> > > I implemented it your way, but I think I found a situation that was not
> > > fully handled (the 3rd), which makes the handler very complicated
> > > as we need to handle all the following cases:
> > > 1/ no trigger, irq enabled -> single read EOC interrupt
> > > 2/ external trigger, no irq -> handle the whole conversion process
> > > 3/ external trigger, irq enabled -> handle the whole conversion process
> > > but also have a dedicated condition to handle the EOC interrupt
> > > properly (fortunately this is a threaded handler that can be
> > > preempted): we need to wait from the handler itself that the
> > > handler gets called again: the first time it is executed as
> > > "pollfunc", the second time as "EOC interrupt". In the second
> > > instance, call complete() in order to deliver the first running
> > > instance of the handler and continue until the reading part.
> > > 4/ cnvst trigger, irq enabled -> only reads the data.
> > > 5/ cnvst trigger, irq disabled -> not possible.
> > >
> > > I added a lot of comments to make it clearer.
> > >
> > > > Regarding this handler, I just realized that this is the hard IRQ handler
> > > which
> > > > might end up calling 'max1027_read_scan()' which in turn calls
> > > 'spi_read()'. Am I
> > > > missing something here?
> > >
> > > I renamed it to make it clear, but this is already a threaded handler.
> > >
> >
> > Hmm, I think I get what you're trying to do.... FWIW, I think you're just going
> > into a lot of trouble here for scenario 3 (I assume external trigger is something
> > else other the device own one). IMO, I would just assume that if we are using
> > an external trigger we have to wait (sleep) for the end of conversion (i.e, I would
> > not care about the IRQ in this case). It would make things much more simpler and
> > I guess it should be expected that if some user is deliberately not using the
> > device own trigger, will have to wait more for scans.
>
> I thought about that, but this means playing with enabling/disabling
> IRQs, which in the end I fear could be almost as verbose :/
>
> Can you look at the new implementation that I proposed and give me your
> feedback on it?
> [PATCH v2 15/16] iio: adc: max1027: Add support for external triggers
>
> > I cannot also see a reason why someone would want to use some
> > external trigger if the device one is available... Does it really make sense?
>
> I would say that "genericity" is a good enough reason for that, but in
> practice I agree with you.
Synchronising capture across a number of devices is usual reason for
this. It's not perfect, but particularly if you have a device
with a complex trigger that is not occurring on at an even 'tick' of
time, then in those cases there isn't really any other way of doing
it. However that irq is unrelated to this driver. This driver
sees a trigger which it then identifies as not being it's own and
from that fires the conversion and waits for a completion.
The EOC interrupt handler knows we are in this mode and calls
complete() rather than iio_poll_trigger()
Jonathan
>
> Thanks,
> Miquèl
On Thu, 2 Sep 2021 17:21:00 +0200
Miquel Raynal <[email protected]> wrote:
> Hi Jonathan,
>
> Jonathan Cameron <[email protected]> wrote on Mon, 30 Aug 2021 11:50:46
> +0100:
>
> > On Wed, 18 Aug 2021 13:11:38 +0200
> > Miquel Raynal <[email protected]> wrote:
> >
> > > Now that max1027_trigger_handler() has been freed from handling hardware
> > > triggers EOC situations, we can use it for what it has been designed in
> > > the first place: trigger software originated conversions.
> >
> > As mentioned earlier, this is not how I'd normally expect this sort of
> > case to be handled. I'd be expecting the cnvst trigger to still be calling
> > this function and the function to do the relevant check to ensure it
> > knows the data is already available in that case.
>
> I tried to follow your advice and Nuno's regarding this, I hope the new
> version will match your expectations (new version coming soon).
> However if my changes do not match, I will probably need more guidance
> to understand in deep what you suggest.
>
> > > In other
> > > words, when userspace initiates a conversion with a sysfs trigger or a
> > > hrtimer trigger, we must do all configuration steps, ie:
> > > 1- Configuring the trigger
> > > 2- Configuring the channels to scan
> > > 3- Starting the conversion (actually done automatically by step 2 in
> > > this case)
> > > 4- Waiting for the conversion to end
> > > 5- Retrieving the data from the ADC
> > > 6- Push the data to the IIO core and notify it
> > >
> > > Add the missing steps to this helper and drop the trigger verification
> > > hook otherwise software triggers would simply not be accepted at all.
> > >
> > > Signed-off-by: Miquel Raynal <[email protected]>
> > > ---
> > > drivers/iio/adc/max1027.c | 26 ++++++++++++++------------
> > > 1 file changed, 14 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > > index 8c5995ae59f2..bb437e43adaf 100644
> > > --- a/drivers/iio/adc/max1027.c
> > > +++ b/drivers/iio/adc/max1027.c
> > > @@ -413,17 +413,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);
> > > @@ -512,7 +501,21 @@ static irqreturn_t max1027_trigger_handler(int irq, void *private)
> > >
> > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__, irq, private);
> > >
> > > + ret = max1027_configure_trigger(indio_dev);
> >
> > I'd not expect to see this ever time. The configuration shouldn't change
> > from one call of this function to the next.
>
> True, this is not needed.
>
> > > + if (ret)
> > > + goto out;
> > > +
> > > + ret = max1027_configure_chans_to_scan(indio_dev);
> >
> > This should also not change unless it is also responsible for the 'go' signal.
> > If that's true then it is badly named.
>
> It's responsible for the go signal, I renamed it "configure_and_start".
>
> However, just for my own understanding, when would I be supposed to
> configure the channels requested by the user otherwise?
For buffered operation update_scan_mask callback.
There is a quirk where that's not called on the buffer disable path because
it's not always obvious what to 'reset to'.
J
>
> Thanks,
> Miquèl
> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Sunday, September 5, 2021 11:41 AM
> To: Miquel Raynal <[email protected]>
> Cc: Sa, Nuno <[email protected]>; Lars-Peter Clausen
> <[email protected]>; Thomas Petazzoni
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end of
> conversion helper
>
> [External]
>
> On Fri, 3 Sep 2021 16:46:50 +0200
> Miquel Raynal <[email protected]> wrote:
>
> > Hi Nuno,
> >
> > [email protected] wrote on Fri, 3 Sep 2021 14:28:52 +0000:
> >
> > > Hi Miquel,
> > >
> > > > From: Miquel Raynal <[email protected]>
> > > > Sent: Thursday, September 2, 2021 5:13 PM
> > > > To: Sa, Nuno <[email protected]>
> > > > Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > > <[email protected]>; Thomas Petazzoni
> > > > <[email protected]>; [email protected];
> linux-
> > > > [email protected]
> > > > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the
> end of
> > > > conversion helper
> > > >
> > > > Hi Nuno,
> > > >
> > > > "Sa, Nuno" <[email protected]> wrote on Mon, 30 Aug 2021
> > > > 12:44:48
> > > > +0000:
> > > >
> > > > > > -----Original Message-----
> > > > > > From: Jonathan Cameron <[email protected]>
> > > > > > Sent: Monday, August 30, 2021 12:37 PM
> > > > > > To: Miquel Raynal <[email protected]>
> > > > > > Cc: Lars-Peter Clausen <[email protected]>; Thomas
> Petazzoni
> > > > > > <[email protected]>; [email protected];
> > > > linux-
> > > > > > [email protected]
> > > > > > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the
> end
> > > > of
> > > > > > conversion helper
> > > > > >
> > > > > > On Wed, 18 Aug 2021 13:11:37 +0200
> > > > > > Miquel Raynal <[email protected]> wrote:
> > > > > >
> > > > > > > Now that we have a dedicated handler for End Of
> Conversion
> > > > > > interrupts,
> > > > > > > let's create a second path:
> > > > > > > - Situation 1: we are using the external hardware trigger, a
> > > > > > conversion
> > > > > > > has been triggered and the ADC pushed the data to its
> FIFO, we
> > > > > > need to
> > > > > > > retrieve the data and push it to the IIO buffers.
> > > > > > > - Situation 2: we are not using the external hardware
> trigger,
> > > > hence
> > > > > > we
> > > > > > > are likely waiting in a blocked thread waiting for this
> interrupt to
> > > > > > > happen: in this case we just wake up the waiting thread.
> > > > > > >
> > > > > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > > > > ---
> > > > > > > drivers/iio/adc/max1027.c | 20 +++++++++++++++++---
> > > > > > > 1 file changed, 17 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/iio/adc/max1027.c
> > > > b/drivers/iio/adc/max1027.c
> > > > > > > index 8d86e77fb5db..8c5995ae59f2 100644
> > > > > > > --- a/drivers/iio/adc/max1027.c
> > > > > > > +++ b/drivers/iio/adc/max1027.c
> > > > > > > @@ -235,6 +235,7 @@ struct max1027_state {
> > > > > > > struct iio_trigger *trig;
> > > > > > > __be16 *buffer;
> > > > > > > struct mutex lock;
> > > > > > > + bool data_rdy;
> > > > > > > bool cnvst_trigger;
> > > > > > > u8 reg
> ____cacheline_aligned;
> > > > > > > };
> > > > > > > @@ -243,12 +244,22 @@ static
> > > > > > DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
> > > > > > >
> > > > > > > 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;
> > > > > > >
> > > > > > > - if (indio_dev->active_scan_mask)
> > > > > > > - conversion_time *= hweight32(*indio_dev-
> > > > > > >active_scan_mask);
> > > > > > > + if (st->spi->irq) {
> > > > > > > + ret =
> > > > > > wait_event_interruptible_timeout(max1027_queue,
> > > > > > > + st-
> >data_rdy, HZ /
> > > > > > 1000);
> > > > > > > + st->data_rdy = false;
> > > > > > > + if (ret == -ERESTARTSYS)
> > > > > > > + return ret;
> > > > > > > + } else {
> > > > > > > + if (indio_dev->active_scan_mask)
> > > > > > > + conversion_time *=
> hweight32(*indio_dev-
> > > > > > >active_scan_mask);
> > > > > > >
> > > > > > > - usleep_range(conversion_time, conversion_time * 2);
> > > > > > > + usleep_range(conversion_time,
> conversion_time * 2);
> > > > > > > + }
> > > > > > >
> > > > > > > return 0;
> > > > > > > }
> > > > > > > @@ -481,6 +492,9 @@ static irqreturn_t
> > > > > > max1027_eoc_irq_handler(int irq, void *private)
> > > > > > > if (st->cnvst_trigger) {
> > > > > > > ret = max1027_read_scan(indio_dev);
> > > > > > > iio_trigger_notify_done(indio_dev->trig);
> > > > > > > + } else {
> > > > > > > + st->data_rdy = true;
> > > > > > > + wake_up(&max1027_queue);
> > > > > >
> > > > > > I can't see why a queue is appropriate for this. Use a
> completion
> > > > and
> > > > > > have
> > > > > > one per instance of the device. No need for the flag etc in
> that
> > > > case as
> > > > > > complete() means we have had an interrupt.
> > > > > >
> > > > >
> > > > > In the case that 'st-> cnvst_trigger' is not set but the spi IRQ
> > > > > is present, we will wait until we get 'wake_up()' called from
> here. I
> > > > wonder if
> > > > > that is a good idea as the device own trigger is not being used.
> FWIW,
> > > > I think this
> > > > > sync logic is a bit confusing... I would still use the normal trigger
> > > > infrastructure
> > > > > ('iio_trigger_generic_data_rdy_poll()') and use the
> 'cnvst_trigger'
> > > > flag in the
> > > > > trigger handler to manually start conversions + wait till eoc. But I
> > > > might be missing
> > > > > something though.
> > > >
> > > > I implemented it your way, but I think I found a situation that was
> not
> > > > fully handled (the 3rd), which makes the handler very
> complicated
> > > > as we need to handle all the following cases:
> > > > 1/ no trigger, irq enabled -> single read EOC interrupt
> > > > 2/ external trigger, no irq -> handle the whole conversion process
> > > > 3/ external trigger, irq enabled -> handle the whole conversion
> process
> > > > but also have a dedicated condition to handle the EOC interrupt
> > > > properly (fortunately this is a threaded handler that can be
> > > > preempted): we need to wait from the handler itself that the
> > > > handler gets called again: the first time it is executed as
> > > > "pollfunc", the second time as "EOC interrupt". In the second
> > > > instance, call complete() in order to deliver the first running
> > > > instance of the handler and continue until the reading part.
> > > > 4/ cnvst trigger, irq enabled -> only reads the data.
> > > > 5/ cnvst trigger, irq disabled -> not possible.
> > > >
> > > > I added a lot of comments to make it clearer.
> > > >
> > > > > Regarding this handler, I just realized that this is the hard IRQ
> handler
> > > > which
> > > > > might end up calling 'max1027_read_scan()' which in turn calls
> > > > 'spi_read()'. Am I
> > > > > missing something here?
> > > >
> > > > I renamed it to make it clear, but this is already a threaded
> handler.
> > > >
> > >
> > > Hmm, I think I get what you're trying to do.... FWIW, I think you're
> just going
> > > into a lot of trouble here for scenario 3 (I assume external trigger is
> something
> > > else other the device own one). IMO, I would just assume that if
> we are using
> > > an external trigger we have to wait (sleep) for the end of
> conversion (i.e, I would
> > > not care about the IRQ in this case). It would make things much
> more simpler and
> > > I guess it should be expected that if some user is deliberately not
> using the
> > > device own trigger, will have to wait more for scans.
> >
> > I thought about that, but this means playing with enabling/disabling
> > IRQs, which in the end I fear could be almost as verbose :/
> >
> > Can you look at the new implementation that I proposed and give
> me your
> > feedback on it?
> > [PATCH v2 15/16] iio: adc: max1027: Add support for external triggers
> >
> > > I cannot also see a reason why someone would want to use some
> > > external trigger if the device one is available... Does it really make
> sense?
> >
> > I would say that "genericity" is a good enough reason for that, but in
> > practice I agree with you.
>
> Synchronising capture across a number of devices is usual reason for
> this. It's not perfect, but particularly if you have a device
> with a complex trigger that is not occurring on at an even 'tick' of
> time, then in those cases there isn't really any other way of doing
I see. That is indeed a valid use case for this...
> it. However that irq is unrelated to this driver. This driver
> sees a trigger which it then identifies as not being it's own and
> from that fires the conversion and waits for a completion.
> The EOC interrupt handler knows we are in this mode and calls
> complete() rather than iio_poll_trigger()
>
Yes, I do understand the logic. I'm just not 100% convinced that
the added complexity for the syncing brings much added value. It
evens looks like that the IRQ is not being used for normal raw reads?
So, I would [probably] just act as no IRQ is present for external triggers
(I do not think it's not that bad to enable/disable the IRQ on the trigger
state cb). Alternatively, starting to us the IRQ for raw reads would
make a stronger argument to enforce this logic as it would be more
consistent...
But I guess this also boils down to personal preference so as long as you
are fine with this logic :) ...
- Nuno Sá
> -----Original Message-----
> From: Sa, Nuno <[email protected]>
> Sent: Monday, September 6, 2021 11:12 AM
> To: Jonathan Cameron <[email protected]>; Miquel Raynal
> <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>; Thomas Petazzoni
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: RE: [PATCH 14/16] iio: adc: max1027: Consolidate the end of
> conversion helper
>
> [External]
>
>
>
> > -----Original Message-----
> > From: Jonathan Cameron <[email protected]>
> > Sent: Sunday, September 5, 2021 11:41 AM
> > To: Miquel Raynal <[email protected]>
> > Cc: Sa, Nuno <[email protected]>; Lars-Peter Clausen
> > <[email protected]>; Thomas Petazzoni
> > <[email protected]>; [email protected]; linux-
> > [email protected]
> > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the end of
> > conversion helper
> >
> > [External]
> >
> > On Fri, 3 Sep 2021 16:46:50 +0200
> > Miquel Raynal <[email protected]> wrote:
> >
> > > Hi Nuno,
> > >
> > > [email protected] wrote on Fri, 3 Sep 2021 14:28:52 +0000:
> > >
> > > > Hi Miquel,
> > > >
> > > > > From: Miquel Raynal <[email protected]>
> > > > > Sent: Thursday, September 2, 2021 5:13 PM
> > > > > To: Sa, Nuno <[email protected]>
> > > > > Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > > > <[email protected]>; Thomas Petazzoni
> > > > > <[email protected]>; [email protected];
> > linux-
> > > > > [email protected]
> > > > > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate the
> > end of
> > > > > conversion helper
> > > > >
> > > > > Hi Nuno,
> > > > >
> > > > > "Sa, Nuno" <[email protected]> wrote on Mon, 30 Aug
> 2021
> > > > > 12:44:48
> > > > > +0000:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jonathan Cameron <[email protected]>
> > > > > > > Sent: Monday, August 30, 2021 12:37 PM
> > > > > > > To: Miquel Raynal <[email protected]>
> > > > > > > Cc: Lars-Peter Clausen <[email protected]>; Thomas
> > Petazzoni
> > > > > > > <[email protected]>; linux-
> [email protected];
> > > > > linux-
> > > > > > > [email protected]
> > > > > > > Subject: Re: [PATCH 14/16] iio: adc: max1027: Consolidate
> the
> > end
> > > > > of
> > > > > > > conversion helper
> > > > > > >
> > > > > > > On Wed, 18 Aug 2021 13:11:37 +0200
> > > > > > > Miquel Raynal <[email protected]> wrote:
> > > > > > >
> > > > > > > > Now that we have a dedicated handler for End Of
> > Conversion
> > > > > > > interrupts,
> > > > > > > > let's create a second path:
> > > > > > > > - Situation 1: we are using the external hardware trigger, a
> > > > > > > conversion
> > > > > > > > has been triggered and the ADC pushed the data to its
> > FIFO, we
> > > > > > > need to
> > > > > > > > retrieve the data and push it to the IIO buffers.
> > > > > > > > - Situation 2: we are not using the external hardware
> > trigger,
> > > > > hence
> > > > > > > we
> > > > > > > > are likely waiting in a blocked thread waiting for this
> > interrupt to
> > > > > > > > happen: in this case we just wake up the waiting thread.
> > > > > > > >
> > > > > > > > Signed-off-by: Miquel Raynal
> <[email protected]>
> > > > > > > > ---
> > > > > > > > drivers/iio/adc/max1027.c | 20 +++++++++++++++++---
> > > > > > > > 1 file changed, 17 insertions(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/iio/adc/max1027.c
> > > > > b/drivers/iio/adc/max1027.c
> > > > > > > > index 8d86e77fb5db..8c5995ae59f2 100644
> > > > > > > > --- a/drivers/iio/adc/max1027.c
> > > > > > > > +++ b/drivers/iio/adc/max1027.c
> > > > > > > > @@ -235,6 +235,7 @@ struct max1027_state {
> > > > > > > > struct iio_trigger *trig;
> > > > > > > > __be16 *buffer;
> > > > > > > > struct mutex lock;
> > > > > > > > + bool data_rdy;
> > > > > > > > bool cnvst_trigger;
> > > > > > > > u8 reg
> > ____cacheline_aligned;
> > > > > > > > };
> > > > > > > > @@ -243,12 +244,22 @@ static
> > > > > > > DECLARE_WAIT_QUEUE_HEAD(max1027_queue);
> > > > > > > >
> > > > > > > > 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;
> > > > > > > >
> > > > > > > > - if (indio_dev->active_scan_mask)
> > > > > > > > - conversion_time *= hweight32(*indio_dev-
> > > > > > > >active_scan_mask);
> > > > > > > > + if (st->spi->irq) {
> > > > > > > > + ret =
> > > > > > > wait_event_interruptible_timeout(max1027_queue,
> > > > > > > > + st-
> > >data_rdy, HZ /
> > > > > > > 1000);
> > > > > > > > + st->data_rdy = false;
> > > > > > > > + if (ret == -ERESTARTSYS)
> > > > > > > > + return ret;
> > > > > > > > + } else {
> > > > > > > > + if (indio_dev->active_scan_mask)
> > > > > > > > + conversion_time *=
> > hweight32(*indio_dev-
> > > > > > > >active_scan_mask);
> > > > > > > >
> > > > > > > > - usleep_range(conversion_time, conversion_time * 2);
> > > > > > > > + usleep_range(conversion_time,
> > conversion_time * 2);
> > > > > > > > + }
> > > > > > > >
> > > > > > > > return 0;
> > > > > > > > }
> > > > > > > > @@ -481,6 +492,9 @@ static irqreturn_t
> > > > > > > max1027_eoc_irq_handler(int irq, void *private)
> > > > > > > > if (st->cnvst_trigger) {
> > > > > > > > ret = max1027_read_scan(indio_dev);
> > > > > > > > iio_trigger_notify_done(indio_dev->trig);
> > > > > > > > + } else {
> > > > > > > > + st->data_rdy = true;
> > > > > > > > + wake_up(&max1027_queue);
> > > > > > >
> > > > > > > I can't see why a queue is appropriate for this. Use a
> > completion
> > > > > and
> > > > > > > have
> > > > > > > one per instance of the device. No need for the flag etc in
> > that
> > > > > case as
> > > > > > > complete() means we have had an interrupt.
> > > > > > >
> > > > > >
> > > > > > In the case that 'st-> cnvst_trigger' is not set but the spi IRQ
> > > > > > is present, we will wait until we get 'wake_up()' called from
> > here. I
> > > > > wonder if
> > > > > > that is a good idea as the device own trigger is not being used.
> > FWIW,
> > > > > I think this
> > > > > > sync logic is a bit confusing... I would still use the normal
> trigger
> > > > > infrastructure
> > > > > > ('iio_trigger_generic_data_rdy_poll()') and use the
> > 'cnvst_trigger'
> > > > > flag in the
> > > > > > trigger handler to manually start conversions + wait till eoc.
> But I
> > > > > might be missing
> > > > > > something though.
> > > > >
> > > > > I implemented it your way, but I think I found a situation that
> was
> > not
> > > > > fully handled (the 3rd), which makes the handler very
> > complicated
> > > > > as we need to handle all the following cases:
> > > > > 1/ no trigger, irq enabled -> single read EOC interrupt
> > > > > 2/ external trigger, no irq -> handle the whole conversion
> process
> > > > > 3/ external trigger, irq enabled -> handle the whole conversion
> > process
> > > > > but also have a dedicated condition to handle the EOC
> interrupt
> > > > > properly (fortunately this is a threaded handler that can be
> > > > > preempted): we need to wait from the handler itself that the
> > > > > handler gets called again: the first time it is executed as
> > > > > "pollfunc", the second time as "EOC interrupt". In the second
> > > > > instance, call complete() in order to deliver the first running
> > > > > instance of the handler and continue until the reading part.
> > > > > 4/ cnvst trigger, irq enabled -> only reads the data.
> > > > > 5/ cnvst trigger, irq disabled -> not possible.
> > > > >
> > > > > I added a lot of comments to make it clearer.
> > > > >
> > > > > > Regarding this handler, I just realized that this is the hard IRQ
> > handler
> > > > > which
> > > > > > might end up calling 'max1027_read_scan()' which in turn calls
> > > > > 'spi_read()'. Am I
> > > > > > missing something here?
> > > > >
> > > > > I renamed it to make it clear, but this is already a threaded
> > handler.
> > > > >
> > > >
> > > > Hmm, I think I get what you're trying to do.... FWIW, I think
> you're
> > just going
> > > > into a lot of trouble here for scenario 3 (I assume external trigger
> is
> > something
> > > > else other the device own one). IMO, I would just assume that if
> > we are using
> > > > an external trigger we have to wait (sleep) for the end of
> > conversion (i.e, I would
> > > > not care about the IRQ in this case). It would make things much
> > more simpler and
> > > > I guess it should be expected that if some user is deliberately not
> > using the
> > > > device own trigger, will have to wait more for scans.
> > >
> > > I thought about that, but this means playing with enabling/disabling
> > > IRQs, which in the end I fear could be almost as verbose :/
> > >
> > > Can you look at the new implementation that I proposed and give
> > me your
> > > feedback on it?
> > > [PATCH v2 15/16] iio: adc: max1027: Add support for external
> triggers
> > >
> > > > I cannot also see a reason why someone would want to use some
> > > > external trigger if the device one is available... Does it really make
> > sense?
> > >
> > > I would say that "genericity" is a good enough reason for that, but
> in
> > > practice I agree with you.
> >
> > Synchronising capture across a number of devices is usual reason for
> > this. It's not perfect, but particularly if you have a device
> > with a complex trigger that is not occurring on at an even 'tick' of
> > time, then in those cases there isn't really any other way of doing
>
> I see. That is indeed a valid use case for this...
>
> > it. However that irq is unrelated to this driver. This driver
> > sees a trigger which it then identifies as not being it's own and
> > from that fires the conversion and waits for a completion.
> > The EOC interrupt handler knows we are in this mode and calls
> > complete() rather than iio_poll_trigger()
> >
>
> Yes, I do understand the logic. I'm just not 100% convinced that
> the added complexity for the syncing brings much added value. It
> evens looks like that the IRQ is not being used for normal raw reads?
Ups, my bad. Just reading the new version and the EOC is also used in
raw reads... You can ignore my other email :)
- Nuno Sá
> -----Original Message-----
> From: Miquel Raynal <[email protected]>
> Sent: Wednesday, September 1, 2021 10:12 AM
> To: Sa, Nuno <[email protected]>
> Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> <[email protected]>; Thomas Petazzoni
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested
> samples
>
> [External]
>
> Hello,
>
> "Sa, Nuno" <[email protected]> wrote on Mon, 30 Aug 2021
> 15:02:26
> +0000:
>
> > > -----Original Message-----
> > > From: Jonathan Cameron <[email protected]>
> > > Sent: Monday, August 30, 2021 4:30 PM
> > > To: Sa, Nuno <[email protected]>
> > > Cc: Miquel Raynal <[email protected]>; Lars-Peter
> Clausen
> > > <[email protected]>; Thomas Petazzoni
> > > <[email protected]>; [email protected];
> linux-
> > > [email protected]
> > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the
> requested
> > > samples
> > >
> > > [External]
> > >
> > > On Mon, 30 Aug 2021 10:49:50 +0000
> > > "Sa, Nuno" <[email protected]> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Jonathan Cameron <[email protected]>
> > > > > Sent: Monday, August 30, 2021 12:08 PM
> > > > > To: Sa, Nuno <[email protected]>
> > > > > Cc: Miquel Raynal <[email protected]>; Lars-Peter
> > > Clausen
> > > > > <[email protected]>; Thomas Petazzoni
> > > > > <[email protected]>; [email protected];
> > > linux-
> > > > > [email protected]
> > > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the
> > > requested
> > > > > samples
> > > > >
> > > > > [External]
> > > > >
> > > > > On Fri, 20 Aug 2021 07:10:48 +0000
> > > > > "Sa, Nuno" <[email protected]> wrote:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Miquel Raynal <[email protected]>
> > > > > > > Sent: Wednesday, August 18, 2021 1:11 PM
> > > > > > > To: Jonathan Cameron <[email protected]>; Lars-Peter
> Clausen
> > > > > > > <[email protected]>
> > > > > > > Cc: Thomas Petazzoni <[email protected]>;
> linux-
> > > > > > > [email protected]; [email protected]; Miquel
> > > Raynal
> > > > > > > <[email protected]>
> > > > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the
> > > requested
> > > > > > > samples
> > > > > > >
> > > > > > > [External]
> > > > > > >
> > > > > > > When a triggered scan occurs, the identity of the desired
> > > channels
> > > > > is
> > > > > > > known in indio_dev->active_scan_mask. Instead of reading
> and
> > > > > > > pushing to
> > > > > > > the IIO buffers all channels each time, scan the minimum
> > > amount
> > > > > of
> > > > > > > channels (0 to maximum requested chan, to be exact) and
> only
> > > > > > > provide the
> > > > > > > samples requested by the user.
> > > > > > >
> > > > > > > For example, if the user wants channels 1, 4 and 5, all
> channels
> > > > > from
> > > > > > > 0 to 5 will be scanned but only the desired channels will be
> > > pushed
> > > > > to
> > > > > > > the IIO buffers.
> > > > > > >
> > > > > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > > > > ---
> > > > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++-
> ---
> > > > > > > 1 file changed, 21 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/iio/adc/max1027.c
> > > b/drivers/iio/adc/max1027.c
> > > > > > > index b753658bb41e..8ab660f596b5 100644
> > > > > > > --- a/drivers/iio/adc/max1027.c
> > > > > > > +++ b/drivers/iio/adc/max1027.c
> > > > > > > @@ -360,6 +360,9 @@ static int
> > > max1027_set_trigger_state(struct
> > > > > > > iio_trigger *trig, bool state)
> > > > > > > struct max1027_state *st = iio_priv(indio_dev);
> > > > > > > int ret;
> > > > > > >
> > > > > > > + if (bitmap_empty(indio_dev->active_scan_mask,
> > > indio_dev-
> > > > > > > >masklength))
> > > > > > > + return -EINVAL;
> > > > > > > +
> > > > > >
> > > > > > I'm not sure this can actually happen. If you try to enable the
> > > buffer
> > > > > > with no scan element, it should give you an error before you
> > > reach
> > > > > > this point...
> > > > > >
> > > > > > > if (state) {
> > > > > > > /* Start acquisition on cnvst */
> > > > > > > st->reg = MAX1027_SETUP_REG |
> > > > > > > MAX1027_CKS_MODE0 |
> > > > > > > @@ -368,9 +371,12 @@ 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 0 to the highest requested
> > > channel. The
> > > > > > > temperature
> > > > > > > + * could be avoided but it simplifies a bit the
> > > logic.
> > > > > > > + */
> > > > > > > + st->reg = MAX1027_CONV_REG |
> > > > > > > MAX1027_SCAN_0_N | MAX1027_TEMP;
> > > > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev-
> > > > > > > >active_scan_mask) - 2);
> > > > > > > ret = spi_write(st->spi, &st->reg, 1);
> > > > > > > if (ret < 0)
> > > > > > > return ret;
> > > > > > > @@ -391,11 +397,22 @@ 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 = fls(*indio_dev-
> > > > > > > >active_scan_mask);
> > > > > > > + u16 *buf = st->buffer;
> > > > > >
> > > > > > I think sparse will complain here. buffer is a __be16 restricted
> > > > > > type so you should not mix those...
> > > > > > > + unsigned int bit;
> > > > > > >
> > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__,
> > > irq,
> > > > > > >
> > > > >
> > >
> private);in/20210818_miquel_raynal_bring_software_triggers_support
> > > > > _to_max1027_like_adcs.mbx
> > > > > > >
> > > > > > > /* fill buffer with all channel */
> > > > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength *
> > > 2);
> > > > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2);
> > > > > > > +
> > > > > > > + /* Only keep the channels selected by the user */
> > > > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > > > > > + indio_dev->masklength) {
> > > > > > > + if (buf[0] != st->buffer[bit])
> > > > > > > + buf[0] = st->buffer[bit];
> > > > > >
> > > > > > Since we are here, when looking into the driver, I realized
> > > > > > that st->buffer is not DMA safe. In IIO, we kind of want to
> > > enforce
> > > > > > that all buffers that are passed to spi/i2c buses are safe...
> Maybe
> > > > > > this is something you can include in your series.
> > > > >
> > > > > Why is it not? st->buffer is result of a devm_kmalloc_array()
> call
> > > and
> > > > > that should provide a DMA safe buffer as I understand it.
> > > > >
> > > >
> > > > That's a good question. I'm not sure how I came to that
> conclusion
> > > which
> > > > is clearly wrong. Though I think the buffer might share the line
> with
> > > the
> > > > mutex...
> > > Pointer shares a line. The buffer it points to doesn't as allocated
> > > by separate heap allocation.
> > >
> >
> > Ups, sure :facepalm:
>
> My understanding [1] was that devm_ allocations were generally not
> suitable for DMA and should not be used for this particular purpose
> because of the extra 16 bytes allocated for storing the devm magic
> somewhere, which shifts the entire buffer and prevents it to always
> be
> aligned on a cache line. I will propose a patch to switch to
> kmalloc_array() instead.
I do not think this is a problem anymore [1]. Nowadays, 'devm_kmalloc'
should give you the same alignment guarantees as 'kmalloc'
[1]: https://elixir.bootlin.com/linux/latest/source/drivers/base/devres.c#L35
- Nuno Sá
On Mon, 6 Sep 2021 08:59:55 +0000
"Sa, Nuno" <[email protected]> wrote:
> > -----Original Message-----
> > From: Miquel Raynal <[email protected]>
> > Sent: Wednesday, September 1, 2021 10:12 AM
> > To: Sa, Nuno <[email protected]>
> > Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > <[email protected]>; Thomas Petazzoni
> > <[email protected]>; [email protected]; linux-
> > [email protected]
> > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested
> > samples
> >
> > [External]
> >
> > Hello,
> >
> > "Sa, Nuno" <[email protected]> wrote on Mon, 30 Aug 2021
> > 15:02:26
> > +0000:
> >
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <[email protected]>
> > > > Sent: Monday, August 30, 2021 4:30 PM
> > > > To: Sa, Nuno <[email protected]>
> > > > Cc: Miquel Raynal <[email protected]>; Lars-Peter
> > Clausen
> > > > <[email protected]>; Thomas Petazzoni
> > > > <[email protected]>; [email protected];
> > linux-
> > > > [email protected]
> > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the
> > requested
> > > > samples
> > > >
> > > > [External]
> > > >
> > > > On Mon, 30 Aug 2021 10:49:50 +0000
> > > > "Sa, Nuno" <[email protected]> wrote:
> > > >
> > > > > > -----Original Message-----
> > > > > > From: Jonathan Cameron <[email protected]>
> > > > > > Sent: Monday, August 30, 2021 12:08 PM
> > > > > > To: Sa, Nuno <[email protected]>
> > > > > > Cc: Miquel Raynal <[email protected]>; Lars-Peter
> > > > Clausen
> > > > > > <[email protected]>; Thomas Petazzoni
> > > > > > <[email protected]>; [email protected];
> > > > linux-
> > > > > > [email protected]
> > > > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the
> > > > requested
> > > > > > samples
> > > > > >
> > > > > > [External]
> > > > > >
> > > > > > On Fri, 20 Aug 2021 07:10:48 +0000
> > > > > > "Sa, Nuno" <[email protected]> wrote:
> > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Miquel Raynal <[email protected]>
> > > > > > > > Sent: Wednesday, August 18, 2021 1:11 PM
> > > > > > > > To: Jonathan Cameron <[email protected]>; Lars-Peter
> > Clausen
> > > > > > > > <[email protected]>
> > > > > > > > Cc: Thomas Petazzoni <[email protected]>;
> > linux-
> > > > > > > > [email protected]; [email protected]; Miquel
> > > > Raynal
> > > > > > > > <[email protected]>
> > > > > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the
> > > > requested
> > > > > > > > samples
> > > > > > > >
> > > > > > > > [External]
> > > > > > > >
> > > > > > > > When a triggered scan occurs, the identity of the desired
> > > > channels
> > > > > > is
> > > > > > > > known in indio_dev->active_scan_mask. Instead of reading
> > and
> > > > > > > > pushing to
> > > > > > > > the IIO buffers all channels each time, scan the minimum
> > > > amount
> > > > > > of
> > > > > > > > channels (0 to maximum requested chan, to be exact) and
> > only
> > > > > > > > provide the
> > > > > > > > samples requested by the user.
> > > > > > > >
> > > > > > > > For example, if the user wants channels 1, 4 and 5, all
> > channels
> > > > > > from
> > > > > > > > 0 to 5 will be scanned but only the desired channels will be
> > > > pushed
> > > > > > to
> > > > > > > > the IIO buffers.
> > > > > > > >
> > > > > > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > > > > > ---
> > > > > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++-
> > ---
> > > > > > > > 1 file changed, 21 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/iio/adc/max1027.c
> > > > b/drivers/iio/adc/max1027.c
> > > > > > > > index b753658bb41e..8ab660f596b5 100644
> > > > > > > > --- a/drivers/iio/adc/max1027.c
> > > > > > > > +++ b/drivers/iio/adc/max1027.c
> > > > > > > > @@ -360,6 +360,9 @@ static int
> > > > max1027_set_trigger_state(struct
> > > > > > > > iio_trigger *trig, bool state)
> > > > > > > > struct max1027_state *st = iio_priv(indio_dev);
> > > > > > > > int ret;
> > > > > > > >
> > > > > > > > + if (bitmap_empty(indio_dev->active_scan_mask,
> > > > indio_dev-
> > > > > > > > >masklength))
> > > > > > > > + return -EINVAL;
> > > > > > > > +
> > > > > > >
> > > > > > > I'm not sure this can actually happen. If you try to enable the
> > > > buffer
> > > > > > > with no scan element, it should give you an error before you
> > > > reach
> > > > > > > this point...
> > > > > > >
> > > > > > > > if (state) {
> > > > > > > > /* Start acquisition on cnvst */
> > > > > > > > st->reg = MAX1027_SETUP_REG |
> > > > > > > > MAX1027_CKS_MODE0 |
> > > > > > > > @@ -368,9 +371,12 @@ 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 0 to the highest requested
> > > > channel. The
> > > > > > > > temperature
> > > > > > > > + * could be avoided but it simplifies a bit the
> > > > logic.
> > > > > > > > + */
> > > > > > > > + st->reg = MAX1027_CONV_REG |
> > > > > > > > MAX1027_SCAN_0_N | MAX1027_TEMP;
> > > > > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev-
> > > > > > > > >active_scan_mask) - 2);
> > > > > > > > ret = spi_write(st->spi, &st->reg, 1);
> > > > > > > > if (ret < 0)
> > > > > > > > return ret;
> > > > > > > > @@ -391,11 +397,22 @@ 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 = fls(*indio_dev-
> > > > > > > > >active_scan_mask);
> > > > > > > > + u16 *buf = st->buffer;
> > > > > > >
> > > > > > > I think sparse will complain here. buffer is a __be16 restricted
> > > > > > > type so you should not mix those...
> > > > > > > > + unsigned int bit;
> > > > > > > >
> > > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__,
> > > > irq,
> > > > > > > >
> > > > > >
> > > >
> > private);in/20210818_miquel_raynal_bring_software_triggers_support
> > > > > > _to_max1027_like_adcs.mbx
> > > > > > > >
> > > > > > > > /* fill buffer with all channel */
> > > > > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength *
> > > > 2);
> > > > > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2);
> > > > > > > > +
> > > > > > > > + /* Only keep the channels selected by the user */
> > > > > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > > > > > > + indio_dev->masklength) {
> > > > > > > > + if (buf[0] != st->buffer[bit])
> > > > > > > > + buf[0] = st->buffer[bit];
> > > > > > >
> > > > > > > Since we are here, when looking into the driver, I realized
> > > > > > > that st->buffer is not DMA safe. In IIO, we kind of want to
> > > > enforce
> > > > > > > that all buffers that are passed to spi/i2c buses are safe...
> > Maybe
> > > > > > > this is something you can include in your series.
> > > > > >
> > > > > > Why is it not? st->buffer is result of a devm_kmalloc_array()
> > call
> > > > and
> > > > > > that should provide a DMA safe buffer as I understand it.
> > > > > >
> > > > >
> > > > > That's a good question. I'm not sure how I came to that
> > conclusion
> > > > which
> > > > > is clearly wrong. Though I think the buffer might share the line
> > with
> > > > the
> > > > > mutex...
> > > > Pointer shares a line. The buffer it points to doesn't as allocated
> > > > by separate heap allocation.
> > > >
> > >
> > > Ups, sure :facepalm:
> >
> > My understanding [1] was that devm_ allocations were generally not
> > suitable for DMA and should not be used for this particular purpose
> > because of the extra 16 bytes allocated for storing the devm magic
> > somewhere, which shifts the entire buffer and prevents it to always
> > be
> > aligned on a cache line. I will propose a patch to switch to
> > kmalloc_array() instead.
>
> I do not think this is a problem anymore [1]. Nowadays, 'devm_kmalloc'
> should give you the same alignment guarantees as 'kmalloc'
>
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/base/devres.c#L35
Great info. I remembered a discussion about fixing that, but couldn't find
the patch. For some reason I didn't just check the code :)
Thanks.
Jonathan
>
> - Nuno Sá
Hi Jonathan,
[email protected] wrote on Mon, 6 Sep 2021 17:56:57 +0100:
> On Mon, 6 Sep 2021 08:59:55 +0000
> "Sa, Nuno" <[email protected]> wrote:
>
> > > -----Original Message-----
> > > From: Miquel Raynal <[email protected]>
> > > Sent: Wednesday, September 1, 2021 10:12 AM
> > > To: Sa, Nuno <[email protected]>
> > > Cc: Jonathan Cameron <[email protected]>; Lars-Peter Clausen
> > > <[email protected]>; Thomas Petazzoni
> > > <[email protected]>; [email protected]; linux-
> > > [email protected]
> > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the requested
> > > samples
> > >
> > > [External]
> > >
> > > Hello,
> > >
> > > "Sa, Nuno" <[email protected]> wrote on Mon, 30 Aug 2021
> > > 15:02:26
> > > +0000:
> > >
> > > > > -----Original Message-----
> > > > > From: Jonathan Cameron <[email protected]>
> > > > > Sent: Monday, August 30, 2021 4:30 PM
> > > > > To: Sa, Nuno <[email protected]>
> > > > > Cc: Miquel Raynal <[email protected]>; Lars-Peter
> > > Clausen
> > > > > <[email protected]>; Thomas Petazzoni
> > > > > <[email protected]>; [email protected];
> > > linux-
> > > > > [email protected]
> > > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the
> > > requested
> > > > > samples
> > > > >
> > > > > [External]
> > > > >
> > > > > On Mon, 30 Aug 2021 10:49:50 +0000
> > > > > "Sa, Nuno" <[email protected]> wrote:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jonathan Cameron <[email protected]>
> > > > > > > Sent: Monday, August 30, 2021 12:08 PM
> > > > > > > To: Sa, Nuno <[email protected]>
> > > > > > > Cc: Miquel Raynal <[email protected]>; Lars-Peter
> > > > > Clausen
> > > > > > > <[email protected]>; Thomas Petazzoni
> > > > > > > <[email protected]>; [email protected];
> > > > > linux-
> > > > > > > [email protected]
> > > > > > > Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the
> > > > > requested
> > > > > > > samples
> > > > > > >
> > > > > > > [External]
> > > > > > >
> > > > > > > On Fri, 20 Aug 2021 07:10:48 +0000
> > > > > > > "Sa, Nuno" <[email protected]> wrote:
> > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Miquel Raynal <[email protected]>
> > > > > > > > > Sent: Wednesday, August 18, 2021 1:11 PM
> > > > > > > > > To: Jonathan Cameron <[email protected]>; Lars-Peter
> > > Clausen
> > > > > > > > > <[email protected]>
> > > > > > > > > Cc: Thomas Petazzoni <[email protected]>;
> > > linux-
> > > > > > > > > [email protected]; [email protected]; Miquel
> > > > > Raynal
> > > > > > > > > <[email protected]>
> > > > > > > > > Subject: [PATCH 03/16] iio: adc: max1027: Push only the
> > > > > requested
> > > > > > > > > samples
> > > > > > > > >
> > > > > > > > > [External]
> > > > > > > > >
> > > > > > > > > When a triggered scan occurs, the identity of the desired
> > > > > channels
> > > > > > > is
> > > > > > > > > known in indio_dev->active_scan_mask. Instead of reading
> > > and
> > > > > > > > > pushing to
> > > > > > > > > the IIO buffers all channels each time, scan the minimum
> > > > > amount
> > > > > > > of
> > > > > > > > > channels (0 to maximum requested chan, to be exact) and
> > > only
> > > > > > > > > provide the
> > > > > > > > > samples requested by the user.
> > > > > > > > >
> > > > > > > > > For example, if the user wants channels 1, 4 and 5, all
> > > channels
> > > > > > > from
> > > > > > > > > 0 to 5 will be scanned but only the desired channels will be
> > > > > pushed
> > > > > > > to
> > > > > > > > > the IIO buffers.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Miquel Raynal <[email protected]>
> > > > > > > > > ---
> > > > > > > > > drivers/iio/adc/max1027.c | 25 +++++++++++++++++++++-
> > > ---
> > > > > > > > > 1 file changed, 21 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/iio/adc/max1027.c
> > > > > b/drivers/iio/adc/max1027.c
> > > > > > > > > index b753658bb41e..8ab660f596b5 100644
> > > > > > > > > --- a/drivers/iio/adc/max1027.c
> > > > > > > > > +++ b/drivers/iio/adc/max1027.c
> > > > > > > > > @@ -360,6 +360,9 @@ static int
> > > > > max1027_set_trigger_state(struct
> > > > > > > > > iio_trigger *trig, bool state)
> > > > > > > > > struct max1027_state *st = iio_priv(indio_dev);
> > > > > > > > > int ret;
> > > > > > > > >
> > > > > > > > > + if (bitmap_empty(indio_dev->active_scan_mask,
> > > > > indio_dev-
> > > > > > > > > >masklength))
> > > > > > > > > + return -EINVAL;
> > > > > > > > > +
> > > > > > > >
> > > > > > > > I'm not sure this can actually happen. If you try to enable the
> > > > > buffer
> > > > > > > > with no scan element, it should give you an error before you
> > > > > reach
> > > > > > > > this point...
> > > > > > > >
> > > > > > > > > if (state) {
> > > > > > > > > /* Start acquisition on cnvst */
> > > > > > > > > st->reg = MAX1027_SETUP_REG |
> > > > > > > > > MAX1027_CKS_MODE0 |
> > > > > > > > > @@ -368,9 +371,12 @@ 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 0 to the highest requested
> > > > > channel. The
> > > > > > > > > temperature
> > > > > > > > > + * could be avoided but it simplifies a bit the
> > > > > logic.
> > > > > > > > > + */
> > > > > > > > > + st->reg = MAX1027_CONV_REG |
> > > > > > > > > MAX1027_SCAN_0_N | MAX1027_TEMP;
> > > > > > > > > + st->reg |= MAX1027_CHAN(fls(*indio_dev-
> > > > > > > > > >active_scan_mask) - 2);
> > > > > > > > > ret = spi_write(st->spi, &st->reg, 1);
> > > > > > > > > if (ret < 0)
> > > > > > > > > return ret;
> > > > > > > > > @@ -391,11 +397,22 @@ 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 = fls(*indio_dev-
> > > > > > > > > >active_scan_mask);
> > > > > > > > > + u16 *buf = st->buffer;
> > > > > > > >
> > > > > > > > I think sparse will complain here. buffer is a __be16 restricted
> > > > > > > > type so you should not mix those...
> > > > > > > > > + unsigned int bit;
> > > > > > > > >
> > > > > > > > > pr_debug("%s(irq=%d, private=0x%p)\n", __func__,
> > > > > irq,
> > > > > > > > >
> > > > > > >
> > > > >
> > > private);in/20210818_miquel_raynal_bring_software_triggers_support
> > > > > > > _to_max1027_like_adcs.mbx
> > > > > > > > >
> > > > > > > > > /* fill buffer with all channel */
> > > > > > > > > - spi_read(st->spi, st->buffer, indio_dev->masklength *
> > > > > 2);
> > > > > > > > > + spi_read(st->spi, st->buffer, scanned_chans * 2);
> > > > > > > > > +
> > > > > > > > > + /* Only keep the channels selected by the user */
> > > > > > > > > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > > > > > > > + indio_dev->masklength) {
> > > > > > > > > + if (buf[0] != st->buffer[bit])
> > > > > > > > > + buf[0] = st->buffer[bit];
> > > > > > > >
> > > > > > > > Since we are here, when looking into the driver, I realized
> > > > > > > > that st->buffer is not DMA safe. In IIO, we kind of want to
> > > > > enforce
> > > > > > > > that all buffers that are passed to spi/i2c buses are safe...
> > > Maybe
> > > > > > > > this is something you can include in your series.
> > > > > > >
> > > > > > > Why is it not? st->buffer is result of a devm_kmalloc_array()
> > > call
> > > > > and
> > > > > > > that should provide a DMA safe buffer as I understand it.
> > > > > > >
> > > > > >
> > > > > > That's a good question. I'm not sure how I came to that
> > > conclusion
> > > > > which
> > > > > > is clearly wrong. Though I think the buffer might share the line
> > > with
> > > > > the
> > > > > > mutex...
> > > > > Pointer shares a line. The buffer it points to doesn't as allocated
> > > > > by separate heap allocation.
> > > > >
> > > >
> > > > Ups, sure :facepalm:
> > >
> > > My understanding [1] was that devm_ allocations were generally not
> > > suitable for DMA and should not be used for this particular purpose
> > > because of the extra 16 bytes allocated for storing the devm magic
> > > somewhere, which shifts the entire buffer and prevents it to always
> > > be
> > > aligned on a cache line. I will propose a patch to switch to
> > > kmalloc_array() instead.
> >
> > I do not think this is a problem anymore [1]. Nowadays, 'devm_kmalloc'
> > should give you the same alignment guarantees as 'kmalloc'
> >
> > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/base/devres.c#L35
> Great info. I remembered a discussion about fixing that, but couldn't find
> the patch. For some reason I didn't just check the code :)
Nice! I didn't know about that, thanks a lot for sharing. So this patch
can be safely discarded then.
Thanks,
Miquèl