Re: [PATCH v1 3/7] iio: adc: ad4170: Add support for buffered data capture

["Nuno Sá" <noname.nuno@gmail.com>]

On Wed, 2025-04-09 at 09:25 -0300, Marcelo Schmitt wrote:
> Extend the AD4170 driver to allow buffered data capture in continuous read
> mode. In continuous read mode, the chip skips the instruction phase and
> outputs just ADC sample data, enabling faster sample rates to be reached.
> The internal channel sequencer always starts sampling from channel 0 and
> channel 0 must be enabled if more than one channel is selected for data
> capture. The scan mask validation callback checks the aforementioned
> condition is met.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
>  drivers/iio/adc/ad4170.c | 170 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 169 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> index 0d24286ac2ab..5ffcdedf3e7f 100644
> --- a/drivers/iio/adc/ad4170.c
> +++ b/drivers/iio/adc/ad4170.c
> @@ -10,10 +10,12 @@
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
> @@ -323,6 +325,16 @@ struct ad4170_state {
>  	int
> sps_tbl[ARRAY_SIZE(ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE][2];
>  	struct completion completion;
>  	struct iio_trigger *trig;
> +
> +	struct spi_transfer xfer;
> +	struct spi_message msg;
> +	__be32 bounce_buffer[AD4170_MAX_CHANNELS];
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the transfer
> buffers
> +	 * to live in their own cache lines.
> +	 */
> +	__be32 rx_buf __aligned(IIO_DMA_MINALIGN);
> +	u8 tx_buf[2];
>  };
>  
>  static void ad4170_fill_sps_tbl(struct ad4170_state *st)
> @@ -882,6 +894,7 @@ static const struct iio_chan_spec ad4170_channel_template
> = {
>  	.scan_type = {
>  		.realbits = 24,
>  		.storagebits = 32,
> +		.shift = 8,
>  		.endianness = IIO_BE,
>  	},
>  };
> @@ -1480,11 +1493,29 @@ static int ad4170_write_raw_get_fmt(struct iio_dev
> *indio_dev,
>  	}
>  }
>  
> +static int ad4170_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *active_scan_mask)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	unsigned int chan_index;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +

Do we need this? In theory we should be fine since you do
iio_device_claim_direct() in ad4170_read_sample().

Otherwise I would expect the same locking rules in postenable() and
predisable().

> +	iio_for_each_active_channel(indio_dev, chan_index) {
> +		ret = ad4170_set_channel_enable(st, chan_index, true);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
>  static const struct iio_info ad4170_info = {
>  	.read_raw = ad4170_read_raw,
>  	.read_avail = ad4170_read_avail,
>  	.write_raw = ad4170_write_raw,
>  	.write_raw_get_fmt = ad4170_write_raw_get_fmt,
> +	.update_scan_mode = ad4170_update_scan_mode,
>  	.debugfs_reg_access = ad4170_debugfs_reg_access,
>  };
>  
> @@ -1759,6 +1790,130 @@ static int ad4170_initial_config(struct iio_dev
> *indio_dev)
>  				 AD4170_ADC_CTRL_MULTI_DATA_REG_SEL_MSK);
>  }
>  
> +static int ad4170_prepare_spi_message(struct ad4170_state *st)
> +{
> +	/*
> +	 * Continuous data register read is enabled on buffer postenable so
> +	 * no instruction phase is needed meaning we don't need to send the
> +	 * register address to read data. Transfer only needs the read
> buffer.
> +	 */
> +	st->xfer.rx_buf = &st->rx_buf;
> +	st->xfer.len =
> BITS_TO_BYTES(ad4170_channel_template.scan_type.realbits);
> +
> +	spi_message_init_with_transfers(&st->msg, &st->xfer, 1);
> +
> +	return devm_spi_optimize_message(&st->spi->dev, st->spi, &st->msg);
> +}
> +
> +static int ad4170_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4170_set_mode(st, AD4170_ADC_CTRL_MODE_CONT);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Enables continuous data register read.
> +	 * This enables continuous read of the ADC Data register. The ADC
> must
> +	 * be in a continuous conversion mode.
> +	 */
> +	return regmap_update_bits(st->regmap16, AD4170_ADC_CTRL_REG,
> +				  AD4170_ADC_CTRL_CONT_READ_MSK,
> +				  FIELD_PREP(AD4170_ADC_CTRL_CONT_READ_MSK,
> +					    
> AD4170_ADC_CTRL_CONT_READ_ENABLE));
> +}
> +
> +static int ad4170_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4170_state *st = iio_priv(indio_dev);
> +	int ret, i;
> +
> +	/*
> +	 * To exit continuous read, write 0xA5 to the ADC during the first 8
> +	 * SCLKs of the ADC data read.
> +	 */
> +	st->tx_buf[0] = AD4170_ADC_CTRL_CONT_READ_EXIT;
> +	st->tx_buf[1] = 0;
> +	ret = spi_write(st->spi, st->tx_buf, 2);
> +	if (ret)
> +		return ret;

Couldn't we still use regmap? Like 
regmap_write(st->regmap8, AD4170_ADC_CTRL_CONT_READ_EXIT, 0)?

Likely fails on the register ranges and not sure it's any better... Still not a
fan of mixing "raw" bus read/writes and regmap().

> +
> +	ret = regmap_update_bits(st->regmap16, AD4170_ADC_CTRL_REG,
> +				 AD4170_ADC_CTRL_CONT_READ_MSK,
> +				 FIELD_PREP(AD4170_ADC_CTRL_CONT_READ_MSK,
> +					   
> AD4170_ADC_CTRL_CONT_READ_DISABLE));
> +	if (ret)
> +		return ret;
> +
> +	ret =  ad4170_set_mode(st, AD4170_ADC_CTRL_MODE_IDLE);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * The ADC sequences through all the enabled channels (see datasheet
> +	 * page 95). That can lead to incorrect channel being read if a
> +	 * single-shot read (or buffered read with different
> active_scan_mask)
> +	 * is done after buffer disable. Disable all channels so only
> requested
> +	 * channels will be read.
> +	 */
> +	for (i = 0; i < indio_dev->num_channels; i++) {
> +		ret = ad4170_set_channel_enable(st, i, false);
> +		if (ret)
> +			return ret;
> +	}
> +	return ret;
> +}
> +
> +static bool ad4170_validate_scan_mask(struct iio_dev *indio_dev,
> +				      const unsigned long *scan_mask)
> +{
> +	unsigned int masklength = iio_get_masklength(indio_dev);
> +	unsigned long first, next;
> +
> +	/*
> +	 * The channel sequencer cycles through the enabled channels in
> +	 * sequential order, from channel 0 to channel 15, bypassing disabled
> +	 * channels. When more than one channel is enabled, channel 0 must
> +	 * always be enabled. See datasheet channel_en register description
> at
> +	 * page 95.
> +	 */
> +	first = find_next_bit(scan_mask, masklength, 0);
> +	next = find_next_bit(scan_mask, masklength, first + 1);
> +	if (next < masklength)
> +		return test_bit(0, scan_mask);

Hmm, maybe this is simpler?

	if (bitmap_weight(scan_mask, masklength) > 1)
		return test_bit(0, scan_mask);
	
	return true;


- Nuno Sá