Re: [PATCH 4/5] ad7380: enable sequencer for single-ended parts

[Jonathan Cameron]

On Fri, 26 Jul 2024 17:20:09 +0200
Julien Stephan <jstephan@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
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);
>  }