-----Original Message-----
From: Jonathan Cameron <jic23@kernel.org>
Sent: Monday, August 30, 2021 4:30 PM
To: Sa, Nuno <Nuno.Sa@analog.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter Clausen
[off-list ref]; Thomas Petazzoni
[off-list ref]; linux-iio@vger.kernel.org; linux-
kernel@vger.kernel.org
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" [off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: Jonathan Cameron <jic23@kernel.org>
Sent: Monday, August 30, 2021 12:08 PM
To: Sa, Nuno <Nuno.Sa@analog.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Lars-Peter

Clausen

quoted

quoted

[off-list ref]; Thomas Petazzoni
[off-list ref]; linux-iio@vger.kernel.org;

linux-

quoted

quoted

kernel@vger.kernel.org
Subject: Re: [PATCH 03/16] iio: adc: max1027: Push only the

requested

quoted

quoted

samples

[External]

On Fri, 20 Aug 2021 07:10:48 +0000
"Sa, Nuno" [off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: Miquel Raynal <miquel.raynal@bootlin.com>
Sent: Wednesday, August 18, 2021 1:11 PM
To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
[off-list ref]
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux-
iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel

Raynal

quoted

quoted

quoted

quoted

[off-list ref]
Subject: [PATCH 03/16] iio: adc: max1027: Push only the

requested

quoted

quoted

quoted

quoted

samples

[External]

When a triggered scan occurs, the identity of the desired

channels

quoted

quoted

is

quoted

quoted

known in indio_dev->active_scan_mask. Instead of reading and
pushing to
the IIO buffers all channels each time, scan the minimum

amount

quoted

quoted

of

quoted

quoted

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

quoted

quoted

0 to 5 will be scanned but only the desired channels will be

pushed

quoted

quoted

to

quoted

quoted

the IIO buffers.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 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

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

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-

quoted

quoted

quoted

quoted

quoted

masklength))

+		return -EINVAL;
+

I'm not sure this can actually happen. If you try to enable the

buffer

quoted

quoted

quoted

with no scan element, it should give you an error before you

reach

quoted

quoted

quoted

this point...

quoted

 	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

quoted

quoted

quoted

quoted

iio_trigger *trig, bool state)
 		if (ret < 0)
 			return ret;

-		/* Scan from 0 to max */
-		st->reg = MAX1027_CONV_REG |

MAX1027_CHAN(0) |

quoted

quoted

quoted

quoted

-			  MAX1027_SCAN_N_M |

MAX1027_TEMP;

quoted

quoted

quoted

quoted

+		/*
+		 * Scan from 0 to the highest requested

channel. The

quoted

quoted

quoted

quoted

temperature
+		 * could be avoided but it simplifies a bit the

logic.

quoted

quoted

quoted

quoted

+		 */
+		st->reg = MAX1027_CONV_REG |
MAX1027_SCAN_0_N | MAX1027_TEMP;
+		st->reg |= MAX1027_CHAN(fls(*indio_dev-

quoted

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-

quoted

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

quoted

+	unsigned int bit;

 	pr_debug("%s(irq=%d, private=0x%p)\n", __func__,

irq,

quoted

quoted

quoted

quoted

private);in/20210818_miquel_raynal_bring_software_triggers_support

quoted

quoted

_to_max1027_like_adcs.mbx

quoted

quoted

 	/* fill buffer with all channel */
-	spi_read(st->spi, st->buffer, indio_dev->masklength *

2);

quoted

quoted

quoted

quoted

+	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

quoted

quoted

quoted

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

quoted

quoted

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

quoted

is clearly wrong. Though I think the buffer might share the line with

the

quoted

mutex...

Pointer shares a line.  The buffer it points to doesn't as allocated
by separate heap allocation.

Ups, sure :facepalm:

- Nuno Sá