mbox series

[0/5] ad7380: add support for single-ended parts

Message ID 20240726-ad7380-add-single-ended-chips-v1-0-2d628b60ccd1@baylibre.com
Headers show
Series ad7380: add support for single-ended parts | expand

Message

Julien Stephan July 26, 2024, 3:20 p.m. UTC
This series will add support for ad7386/7/8 (16/14/12 bits) unsigned,
dual simultaneous sampling, single-ended compatible parts, and the
corresponding ad7386-4/7-4/8-4 4 channels to ad7380 driver.

These parts have a 2:1 multiplexer in front of each ADC. They also include
additional configuration registers that allow for either manual selection
or automatic switching (sequencer mode), of the multiplexer inputs.

From an IIO point of view, all inputs are exported, i.e ad7386/7/8
export 4 channels and ad7386-4/7-4/8-4 export 8 channels.

Inputs AinX0 of multiplexers correspond to the first half of IIO channels
(i.e 0-1 or 0-3) and inputs AinX1 correspond to second half (i.e 2-3 or
4-7). Example for AD7386/7/8 (2 channels parts):

          IIO   | AD7386/7/8
                |         +----------------------------
                |         |     _____        ______
                |         |    |     |      |      |
       voltage0 | AinA0 --|--->|     |      |      |
                |         |    | mux |----->| ADCA |---
       voltage2 | AinA1 --|--->|     |      |      |
                |         |    |_____|      |_____ |
                |         |     _____        ______
                |         |    |     |      |      |
       voltage1 | AinB0 --|--->|     |      |      |
                |         |    | mux |----->| ADCB |---
       voltage3 | AinB1 --|--->|     |      |      |
                |         |    |_____|      |______|
                |         |
                |         +----------------------------

To ease the review, this series is split on several commits, in
particular, sequencer mode is added as an additional commit.

Cheers
Julien

Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
Julien Stephan (5):
      dt-bindings: iio: adc: ad7380: add single-ended compatible parts
      ad7380: prepare driver for single-ended parts support
      ad7380: add support for single-ended parts
      ad7380: enable sequencer for single-ended parts
      docs: iio: ad7380: add support for single-ended parts

 .../devicetree/bindings/iio/adc/adi,ad7380.yaml    |  13 +
 Documentation/iio/ad7380.rst                       |  42 ++
 drivers/iio/adc/ad7380.c                           | 511 +++++++++++++++++----
 3 files changed, 488 insertions(+), 78 deletions(-)
---
base-commit: 472438c7e0e2261c6737a8321f46ef176eef1c8f
change-id: 20240726-ad7380-add-single-ended-chips-b437d1cc8b8b

Best regards,

Comments

Jonathan Cameron July 28, 2024, 4:23 p.m. UTC | #1
On Fri, 26 Jul 2024 17:20:08 +0200
Julien Stephan <jstephan@baylibre.com> wrote:

> Adding ad7386/7/8 (16/14/12 bits) unsigned, dual simultaneous sampling,
> single-ended compatible parts, and the corresponding ad7386-4/7-4/8-4
> 4 channels.
> 
> These parts have a 2:1 multiplexer in front of each ADC. They also include
> additional configuration registers that allow for either manual selection
> or automatic switching (sequencer mode), of the multiplexer inputs.
> This commit focus on integrating manual selection. Sequencer mode will
> be implemented later.
> 
> From an IIO point of view, all inputs are exported, i.e ad7386/7/8
> export 4 channels and ad7386-4/7-4/8-4 export 8 channels.
> 
> Inputs AinX0 of multiplexers correspond to the first half of IIO channels
> (i.e 0-1 or 0-3) and inputs AinX1 correspond to second half (i.e 2-3 or
> 4-7). Example for AD7386/7/8 (2 channels parts):
> 
>           IIO   | AD7386/7/8
>                 |         +----------------------------
>                 |         |     _____        ______
>                 |         |    |     |      |      |
>        voltage0 | AinA0 --|--->|     |      |      |
>                 |         |    | mux |----->| ADCA |---
>        voltage2 | AinA1 --|--->|     |      |      |
>                 |         |    |_____|      |_____ |
>                 |         |     _____        ______
>                 |         |    |     |      |      |
>        voltage1 | AinB0 --|--->|     |      |      |
>                 |         |    | mux |----->| ADCB |---
>        voltage3 | AinB1 --|--->|     |      |      |
>                 |         |    |_____|      |______|
>                 |         |
>                 |         +----------------------------
> 
> When switching channel, the ADC require an additional settling time.
> According to the datasheet, data is valid on the third CS low. We already
> have an extra toggle before each read (either direct reads or buffered
> reads) to sample correct data, so we just add a single CS toggle at the
> end of the register write.
> 
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
Hi Julien

LGTM - only one trivial comment inline.
If nothing else comes up I can change that whilst applying.
I won't be applying today however given this is a new series and has only been
on the list since Friday.

...

> @@ -92,8 +96,24 @@ enum {
>  	AD7380_SCAN_TYPE_RESOLUTION_BOOST,
>  };
>  
> -/* Extended scan types for 14-bit chips. */
> -static const struct iio_scan_type ad7380_scan_type_14[] = {
> +/* Extended scan types for 12-bit unsigned chips. */
> +static const struct iio_scan_type ad7380_scan_type_12_u[] = {
> +	[AD7380_SCAN_TYPE_NORMAL] = {
> +		.sign = 'u',
> +		.realbits = 12,
> +		.storagebits = 16,
> +		.endianness = IIO_CPU

Add trailing commas.  In theory we might expand this structure
in the future.   The only time we don't add trailing commas is
for 'null' terminator type entries where we know anything added
must come before them.


> +	},
> +	[AD7380_SCAN_TYPE_RESOLUTION_BOOST] = {
> +		.sign = 'u',
> +		.realbits = 14,
> +		.storagebits = 16,
> +		.endianness = IIO_CPU
> +	},
> +};

>  
> +/*
> + * Single ended parts have a 2:1 multiplexer in front of each ADC.
> + *
> + * From an IIO point of view, all inputs are exported, i.e ad7386/7/8
> + * export 4 channels and ad7386-4/7-4/8-4 export 8 channels.
> + *
> + * Inputs AinX0 of multiplexers correspond to the first half of IIO channels
> + * (i.e 0-1 or 0-3) and inputs AinX1 correspond to second half (i.e 2-3 or
> + * 4-7). Example for AD7386/7/8 (2 channels parts):
> + *
> + *           IIO   | AD7386/7/8
> + *                 |         +----------------------------
> + *                 |         |     _____        ______
> + *                 |         |    |     |      |      |
> + *        voltage0 | AinA0 --|--->|     |      |      |
> + *                 |         |    | mux |----->| ADCA |---
> + *        voltage2 | AinA1 --|--->|     |      |      |
> + *                 |         |    |_____|      |_____ |
> + *                 |         |     _____        ______
> + *                 |         |    |     |      |      |
> + *        voltage1 | AinB0 --|--->|     |      |      |
> + *                 |         |    | mux |----->| ADCB |---
> + *        voltage3 | AinB1 --|--->|     |      |      |
> + *                 |         |    |_____|      |______|
> + *                 |         |
> + *                 |         +----------------------------
> + *
> + * Since this is simultaneous sampling for AinX0 OR AinX1 we have two separate
> + * scan masks.
> + */

Good. I always like some nice art :)
+ your implementation takes the same approach I would have done.
Jonathan Cameron July 28, 2024, 4:35 p.m. UTC | #2
On Fri, 26 Jul 2024 17:20:09 +0200
Julien Stephan <jstephan@baylibre.com> wrote:

> ad7386/7/8(-4) single-ended parts have a 2:1 mux in front of each ADC.
> 
> From an IIO point of view, all inputs are exported, i.e ad7386/7/8
> export 4 channels and ad7386-4/7-4/8-4 export 8 channels. First inputs
> of muxes correspond to the first half of IIO channels (i.e 0-1 or 0-3)
> and second inputs correspond to second half (i.e 2-3 or 4-7)
> 
> Currently, the driver supports only sampling first half OR second half of
> the IIO channels. To enable sampling all channels simultaneously, these
> parts have an internal sequencer that automatically cycle through the
> mux entries.
> 
> When enabled, the maximum throughput is divided by two. Moreover, the ADCs
> need additional settling time, so we add an extra CS toggle to correctly
> propagate setting, and an additional spi transfer to read the second
> half.
> 
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
Hi Julien,

All looks good. Main comment is a suggestion that we add a core
interface to get the index of the active_scan_mask if it is built
from available_scan_masks.  That will avoid the mask matching code
in here.

Implementation for now would be a simple bit of pointer
arithmetic after checking available_scan_masks is set.

Jonathan

> ---
>  drivers/iio/adc/ad7380.c | 164 ++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 121 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index 25d42fff1839..11ed010431cf 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -33,7 +33,7 @@

> @@ -290,16 +291,22 @@ static const unsigned long ad7380_4_channel_scan_masks[] = {
>   *
>   * Since this is simultaneous sampling for AinX0 OR AinX1 we have two separate
>   * scan masks.
> + * When sequencer mode is enabled, chip automatically cycle through

cycles 

> + * AinX0 and AinX1 channels. From an IIO point of view, we ca enable all
> + * channels, at the cost of an extra read, thus dividing the maximum rate by
> + * two.
>   */

...

>  	 * DMA (thus cache coherency maintenance) requires the transfer buffers
>  	 * to live in their own cache lines.
> @@ -609,33 +619,47 @@ static int ad7380_set_ch(struct ad7380_state *st, unsigned int ch)
>  static void ad7380_update_xfers(struct ad7380_state *st,
>  				const struct iio_scan_type *scan_type)
>  {
> -	/*
> -	 * First xfer only triggers conversion and has to be long enough for
> -	 * all conversions to complete, which can be multiple conversion in the
> -	 * case of oversampling. Technically T_CONVERT_X_NS is lower for some
> -	 * chips, but we use the maximum value for simplicity for now.
> -	 */
> -	if (st->oversampling_ratio > 1)
> -		st->xfer[0].delay.value = T_CONVERT_0_NS + T_CONVERT_X_NS *
> -						(st->oversampling_ratio - 1);
> -	else
> -		st->xfer[0].delay.value = T_CONVERT_NS;
> -
> -	st->xfer[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> +	struct spi_transfer *xfer = st->seq ? st->seq_xfer : st->normal_xfer;
> +	unsigned int t_convert = T_CONVERT_NS;
>  
>  	/*
> -	 * Second xfer reads all channels. Data size depends on if resolution
> -	 * boost is enabled or not.
> +	 * In the case of oversampling, conversion time is higher than in normal
> +	 * mode. Technically T_CONVERT_X_NS is lower for some chips, but we use
> +	 * the maximum value for simplicity for now.
>  	 */
> -	st->xfer[1].bits_per_word = scan_type->realbits;
> -	st->xfer[1].len = BITS_TO_BYTES(scan_type->storagebits) *
> -			  st->chip_info->num_simult_channels;
> +	if (st->oversampling_ratio > 1)
> +		t_convert = T_CONVERT_0_NS + T_CONVERT_X_NS *
> +			(st->oversampling_ratio - 1);
> +
> +	if (st->seq) {
> +		xfer[0].delay.value = xfer[1].delay.value = t_convert;
> +		xfer[0].delay.unit = xfer[1].delay.unit = SPI_DELAY_UNIT_NSECS;
> +		xfer[2].bits_per_word = xfer[3].bits_per_word =
> +			scan_type->realbits;
> +		xfer[2].len = xfer[3].len =
> +			BITS_TO_BYTES(scan_type->storagebits) *
> +			st->chip_info->num_simult_channels;
> +		xfer[3].rx_buf = xfer[2].rx_buf + xfer[2].len;
> +		/* Additional delay required here when oversampling is enabled */
> +		if (st->oversampling_ratio > 1)
> +			xfer[2].delay.value = t_convert;
> +		else
> +			xfer[2].delay.value = 0;
> +		xfer[2].delay.unit = SPI_DELAY_UNIT_NSECS;
> +	} else {
> +		xfer[0].delay.value = t_convert;
> +		xfer[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> +		xfer[1].bits_per_word = scan_type->realbits;
> +		xfer[1].len = BITS_TO_BYTES(scan_type->storagebits) *
> +			st->chip_info->num_simult_channels;
> +	}
>  }
>  
>  static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
>  {
>  	struct ad7380_state *st = iio_priv(indio_dev);
>  	const struct iio_scan_type *scan_type;
> +	struct spi_message *msg = &st->normal_msg;
>  
>  	/*
>  	 * Currently, we always read all channels at the same time. The scan_type
> @@ -646,34 +670,62 @@ static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
>  		return PTR_ERR(scan_type);
>  
>  	if (st->chip_info->has_mux) {
> -		unsigned int num_simult_channels = st->chip_info->num_simult_channels;
> +		unsigned int num_simult_channels =
> +			st->chip_info->num_simult_channels;

Unrelated change. Push this back to the earlier patch (or leave it alone - whether
it matters for readability is debatable anyway, so I think this is fine either way).

>  		unsigned long active_scan_mask = *indio_dev->active_scan_mask;
>  		unsigned int ch = 0;
>  		int ret;
>  
>  		/*
>  		 * Depending on the requested scan_mask and current state,
> -		 * we need to change CH bit to sample correct data.
> +		 * we need to either change CH bit, or enable sequencer mode
> +		 * to sample correct data.
> +		 * Sequencer mode is enabled if active mask corresponds to all
> +		 * IIO channels enabled. Otherwise, CH bit is set.
>  		 */
> -		if (active_scan_mask == GENMASK(2 * num_simult_channels - 1,
> -						num_simult_channels))
> -			ch = 1;
> +		if (active_scan_mask == GENMASK(2 * num_simult_channels - 1, 0)) {

Whilst it's an implementation detail that you can (IIRC) just compare the active_scan_mask
address with that of your available_scan_masks array entries, maybe it's worth providing
an interface that gets the index of that array?

int iio_active_scan_mask_index(struct iio_dev *)
that returns an error if available_scan_masks isn't set.

We know the active_scan_mask will always be selected from the available ones
so this interface should be fine even if we change how they are handled internally
in the future.

That would then make all these matches simpler.

> +			ret = regmap_update_bits(st->regmap,
> +						 AD7380_REG_ADDR_CONFIG1,
> +						 AD7380_CONFIG1_SEQ,
> +						 FIELD_PREP(AD7380_CONFIG1_SEQ, 1));
> +			msg = &st->seq_msg;
> +			st->seq = true;
> +		} else {
> +			if (active_scan_mask == GENMASK(2 * num_simult_channels - 1,
> +							num_simult_channels))
> +				ch = 1;
> +
> +			ret = ad7380_set_ch(st, ch);
> +		}
>  
> -		ret = ad7380_set_ch(st, ch);
>  		if (ret)
>  			return ret;

I'd just duplicate this if (ret) check as the two calls are very different so to
me this doesn't make logical sense (even if it works).

>  	}
>  
>  	ad7380_update_xfers(st, scan_type);
>  
> -	return spi_optimize_message(st->spi, &st->msg);
> +	return spi_optimize_message(st->spi, msg);
>  }
Jonathan Cameron July 28, 2024, 4:37 p.m. UTC | #3
On Fri, 26 Jul 2024 17:20:10 +0200
Julien Stephan <jstephan@baylibre.com> wrote:

> The AD7380 family has some compatible single-ended chips: AD7386/7/8(-4).
> These single-ended chips have a  2:1 multiplexer in front of each ADC.
> They also include additional configuration registers that allow for either
> manual selection or automatic switching (sequencer mode), of the
> multiplexer inputs. Add a section to describe this.
> 
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
Just one trivial missing space.

Nice patch set. Thanks,

Jonathan


> ---
>  Documentation/iio/ad7380.rst | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/Documentation/iio/ad7380.rst b/Documentation/iio/ad7380.rst
> index 061cd632b5df..81dfa39519fb 100644
> --- a/Documentation/iio/ad7380.rst
> +++ b/Documentation/iio/ad7380.rst
> @@ -17,10 +17,16 @@ The following chips are supported by this driver:
>  * `AD7381 <https://www.analog.com/en/products/ad7381.html>`_
>  * `AD7383 <https://www.analog.com/en/products/ad7383.html>`_
>  * `AD7384 <https://www.analog.com/en/products/ad7384.html>`_
> +* `AD7386 <https://www.analog.com/en/products/ad7386.html>`_
> +* `AD7387 <https://www.analog.com/en/products/ad7387.html>`_
> +* `AD7388 <https://www.analog.com/en/products/ad7388.html>`_
>  * `AD7380-4 <https://www.analog.com/en/products/ad7380-4.html>`_
>  * `AD7381-4 <https://www.analog.com/en/products/ad7381-4.html>`_
>  * `AD7383-4 <https://www.analog.com/en/products/ad7383-4.html>`_
>  * `AD7384-4 <https://www.analog.com/en/products/ad7384-4.html>`_
> +* `AD7386-4 <https://www.analog.com/en/products/ad7386-4.html>`_
> +* `AD7387-4 <https://www.analog.com/en/products/ad7387-4.html>`_
> +* `AD7388-4 <https://www.analog.com/en/products/ad7388-4.html>`_
>  
>  
>  Supported features
> @@ -69,6 +75,42 @@ must restart iiod using the following command:
>  
>  	root:~# systemctl restart iiod
>  
> +Channel selection and sequencer (single-end chips only)
> +-------------------------------------------------------
> +
> +Single-ended chips of this family (ad7386/7/8(-4)) have a 2:1 multiplexer in
> +front of each ADC. They also include additional configuration registers that
allow for either manual selection or automatic switching (sequencer mode),of the
space after ,
plus adjust the wrap as that'll make it 81 chars I think.

> +multiplexer inputs.
> +
> +From an IIO point of view, all inputs are exported, i.e ad7386/7/8
> +export 4 channels and ad7386-4/7-4/8-4 export 8 channels.
> +
> +Inputs ``AinX0`` of multiplexers correspond to the first half of IIO channels (i.e
> +0-1 or 0-3) and inputs ``AinX1`` correspond to second half (i.e 2-3 or 4-7).
> +Example for AD7386/7/8 (2 channels parts):
> +
> +.. code-block::
> +
> +	   IIO   | AD7386/7/8
> +	         |         +----------------------------
> +	         |         |     _____        ______
> +	         |         |    |     |      |      |
> +	voltage0 | AinA0 --|--->|     |      |      |
> +	         |         |    | mux |----->| ADCA |---
> +	voltage2 | AinA1 --|--->|     |      |      |
> +	         |         |    |_____|      |_____ |
> +	         |         |     _____        ______
> +	         |         |    |     |      |      |
> +	voltage1 | AinB0 --|--->|     |      |      |
> +	         |         |    | mux |----->| ADCB |---
> +	voltage3 | AinB1 --|--->|     |      |      |
> +	         |         |    |_____|      |______|
> +	         |         |
> +	         |         +----------------------------
> +
> +
> +When enabling sequencer mode, the effective sampling rate is divided by two.
>  
>  Unimplemented features
>  ----------------------
>
Julien Stephan July 30, 2024, 7:34 a.m. UTC | #4
Le dim. 28 juil. 2024 à 18:36, Jonathan Cameron <jic23@kernel.org> a écrit :
>
> On Fri, 26 Jul 2024 17:20:09 +0200
> Julien Stephan <jstephan@baylibre.com> wrote:
>
> > ad7386/7/8(-4) single-ended parts have a 2:1 mux in front of each ADC.
> >
> > From an IIO point of view, all inputs are exported, i.e ad7386/7/8
> > export 4 channels and ad7386-4/7-4/8-4 export 8 channels. First inputs
> > of muxes correspond to the first half of IIO channels (i.e 0-1 or 0-3)
> > and second inputs correspond to second half (i.e 2-3 or 4-7)
> >
> > Currently, the driver supports only sampling first half OR second half of
> > the IIO channels. To enable sampling all channels simultaneously, these
> > parts have an internal sequencer that automatically cycle through the
> > mux entries.
> >
> > When enabled, the maximum throughput is divided by two. Moreover, the ADCs
> > need additional settling time, so we add an extra CS toggle to correctly
> > propagate setting, and an additional spi transfer to read the second
> > half.
> >
> > Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> Hi Julien,
>
> All looks good. Main comment is a suggestion that we add a core
> interface to get the index of the active_scan_mask if it is built
> from available_scan_masks.  That will avoid the mask matching code
> in here.
>
> Implementation for now would be a simple bit of pointer
> arithmetic after checking available_scan_masks is set.
>
> Jonathan
>
> > ---
> >  drivers/iio/adc/ad7380.c | 164 ++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 121 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> > index 25d42fff1839..11ed010431cf 100644
> > --- a/drivers/iio/adc/ad7380.c
> > +++ b/drivers/iio/adc/ad7380.c
> > @@ -33,7 +33,7 @@
>
> > @@ -290,16 +291,22 @@ static const unsigned long ad7380_4_channel_scan_masks[] = {
> >   *
> >   * Since this is simultaneous sampling for AinX0 OR AinX1 we have two separate
> >   * scan masks.
> > + * When sequencer mode is enabled, chip automatically cycle through
>
> cycles
>
> > + * AinX0 and AinX1 channels. From an IIO point of view, we ca enable all
> > + * channels, at the cost of an extra read, thus dividing the maximum rate by
> > + * two.
> >   */
>
> ...
>
> >        * DMA (thus cache coherency maintenance) requires the transfer buffers
> >        * to live in their own cache lines.
> > @@ -609,33 +619,47 @@ static int ad7380_set_ch(struct ad7380_state *st, unsigned int ch)
> >  static void ad7380_update_xfers(struct ad7380_state *st,
> >                               const struct iio_scan_type *scan_type)
> >  {
> > -     /*
> > -      * First xfer only triggers conversion and has to be long enough for
> > -      * all conversions to complete, which can be multiple conversion in the
> > -      * case of oversampling. Technically T_CONVERT_X_NS is lower for some
> > -      * chips, but we use the maximum value for simplicity for now.
> > -      */
> > -     if (st->oversampling_ratio > 1)
> > -             st->xfer[0].delay.value = T_CONVERT_0_NS + T_CONVERT_X_NS *
> > -                                             (st->oversampling_ratio - 1);
> > -     else
> > -             st->xfer[0].delay.value = T_CONVERT_NS;
> > -
> > -     st->xfer[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> > +     struct spi_transfer *xfer = st->seq ? st->seq_xfer : st->normal_xfer;
> > +     unsigned int t_convert = T_CONVERT_NS;
> >
> >       /*
> > -      * Second xfer reads all channels. Data size depends on if resolution
> > -      * boost is enabled or not.
> > +      * In the case of oversampling, conversion time is higher than in normal
> > +      * mode. Technically T_CONVERT_X_NS is lower for some chips, but we use
> > +      * the maximum value for simplicity for now.
> >        */
> > -     st->xfer[1].bits_per_word = scan_type->realbits;
> > -     st->xfer[1].len = BITS_TO_BYTES(scan_type->storagebits) *
> > -                       st->chip_info->num_simult_channels;
> > +     if (st->oversampling_ratio > 1)
> > +             t_convert = T_CONVERT_0_NS + T_CONVERT_X_NS *
> > +                     (st->oversampling_ratio - 1);
> > +
> > +     if (st->seq) {
> > +             xfer[0].delay.value = xfer[1].delay.value = t_convert;
> > +             xfer[0].delay.unit = xfer[1].delay.unit = SPI_DELAY_UNIT_NSECS;
> > +             xfer[2].bits_per_word = xfer[3].bits_per_word =
> > +                     scan_type->realbits;
> > +             xfer[2].len = xfer[3].len =
> > +                     BITS_TO_BYTES(scan_type->storagebits) *
> > +                     st->chip_info->num_simult_channels;
> > +             xfer[3].rx_buf = xfer[2].rx_buf + xfer[2].len;
> > +             /* Additional delay required here when oversampling is enabled */
> > +             if (st->oversampling_ratio > 1)
> > +                     xfer[2].delay.value = t_convert;
> > +             else
> > +                     xfer[2].delay.value = 0;
> > +             xfer[2].delay.unit = SPI_DELAY_UNIT_NSECS;
> > +     } else {
> > +             xfer[0].delay.value = t_convert;
> > +             xfer[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> > +             xfer[1].bits_per_word = scan_type->realbits;
> > +             xfer[1].len = BITS_TO_BYTES(scan_type->storagebits) *
> > +                     st->chip_info->num_simult_channels;
> > +     }
> >  }
> >
> >  static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
> >  {
> >       struct ad7380_state *st = iio_priv(indio_dev);
> >       const struct iio_scan_type *scan_type;
> > +     struct spi_message *msg = &st->normal_msg;
> >
> >       /*
> >        * Currently, we always read all channels at the same time. The scan_type
> > @@ -646,34 +670,62 @@ static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
> >               return PTR_ERR(scan_type);
> >
> >       if (st->chip_info->has_mux) {
> > -             unsigned int num_simult_channels = st->chip_info->num_simult_channels;
> > +             unsigned int num_simult_channels =
> > +                     st->chip_info->num_simult_channels;
>
> Unrelated change. Push this back to the earlier patch (or leave it alone - whether
> it matters for readability is debatable anyway, so I think this is fine either way).
>
> >               unsigned long active_scan_mask = *indio_dev->active_scan_mask;
> >               unsigned int ch = 0;
> >               int ret;
> >
> >               /*
> >                * Depending on the requested scan_mask and current state,
> > -              * we need to change CH bit to sample correct data.
> > +              * we need to either change CH bit, or enable sequencer mode
> > +              * to sample correct data.
> > +              * Sequencer mode is enabled if active mask corresponds to all
> > +              * IIO channels enabled. Otherwise, CH bit is set.
> >                */
> > -             if (active_scan_mask == GENMASK(2 * num_simult_channels - 1,
> > -                                             num_simult_channels))
> > -                     ch = 1;
> > +             if (active_scan_mask == GENMASK(2 * num_simult_channels - 1, 0)) {
>
> Whilst it's an implementation detail that you can (IIRC) just compare the active_scan_mask
> address with that of your available_scan_masks array entries, maybe it's worth providing
> an interface that gets the index of that array?
>
> int iio_active_scan_mask_index(struct iio_dev *)
> that returns an error if available_scan_masks isn't set.

Hi Jonathan,

I'll send a v2 of this series in a couple of days, with all comments
fixed and I'll try to implement an iio_active_scan_mask_index
function.

Cheers
Julien

>
> We know the active_scan_mask will always be selected from the available ones
> so this interface should be fine even if we change how they are handled internally
> in the future.
>
> That would then make all these matches simpler.
>
> > +                     ret = regmap_update_bits(st->regmap,
> > +                                              AD7380_REG_ADDR_CONFIG1,
> > +                                              AD7380_CONFIG1_SEQ,
> > +                                              FIELD_PREP(AD7380_CONFIG1_SEQ, 1));
> > +                     msg = &st->seq_msg;
> > +                     st->seq = true;
> > +             } else {
> > +                     if (active_scan_mask == GENMASK(2 * num_simult_channels - 1,
> > +                                                     num_simult_channels))
> > +                             ch = 1;
> > +
> > +                     ret = ad7380_set_ch(st, ch);
> > +             }
> >
> > -             ret = ad7380_set_ch(st, ch);
> >               if (ret)
> >                       return ret;
>
> I'd just duplicate this if (ret) check as the two calls are very different so to
> me this doesn't make logical sense (even if it works).
>
> >       }
> >
> >       ad7380_update_xfers(st, scan_type);
> >
> > -     return spi_optimize_message(st->spi, &st->msg);
> > +     return spi_optimize_message(st->spi, msg);
> >  }