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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 28 Apr 2025 09:28:20 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> 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>
A few minor things spotted inline.

Thanks,

Jonathan

> ---
> changes since v1
> - Using bitmap_weight().
> - rx_buf changed from __be32 to u8 array to better cope with new regmap config.
> 
>  drivers/iio/adc/Kconfig  |   2 +
>  drivers/iio/adc/ad4170.c | 199 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 199 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index d5d0308da007..9b4787c127fc 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -75,6 +75,8 @@ config AD4170
>  	tristate "Analog Device AD4170 ADC Driver"
>  	depends on SPI
>  	select REGMAP_SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for Analog Devices AD4170 SPI analog
>  	  to digital converters (ADC).
> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> index 4d0af15cb48d..5fcf1c023ac2 100644
> --- a/drivers/iio/adc/ad4170.c
> +++ b/drivers/iio/adc/ad4170.c
> @@ -10,8 +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>
> @@ -54,6 +58,7 @@
>  #define AD4170_FILTER_FS_REG(x)				(0xC7 + 14 * (x))
>  #define AD4170_OFFSET_REG(x)				(0xCA + 14 * (x))
>  #define AD4170_GAIN_REG(x)				(0xCD + 14 * (x))
> +#define AD4170_ADC_CTRL_CONT_READ_EXIT_REG		0x200 /* virtual reg */
>  
>  #define AD4170_REG_READ_MASK				BIT(14)
>  
> @@ -221,6 +226,7 @@ static const unsigned int ad4170_reg_size[] = {
>  	[AD4170_OFFSET_REG(5) ... AD4170_GAIN_REG(5)] = 3,
>  	[AD4170_OFFSET_REG(6) ... AD4170_GAIN_REG(6)] = 3,
>  	[AD4170_OFFSET_REG(7) ... AD4170_GAIN_REG(7)] = 3,
> +	[AD4170_ADC_CTRL_CONT_READ_EXIT_REG] = 0,
>  };
>  
>  enum ad4170_ref_buf {
> @@ -347,12 +353,16 @@ struct ad4170_state {
>  	u32 int_pin_sel;
>  	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.
>  	 */
>  	u8 tx_buf[AD4170_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> -	u8 rx_buf[AD4170_SPI_MAX_XFER_LEN];
> +	u8 rx_buf[4];

Why this change?

>  };

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

First two sentences seem to say the same thing.

> +	 * be in a continuous conversion mode.
> +	 */
> +	return regmap_update_bits(st->regmap, 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;
> +
> +	/*
> +	 * Use a high register address (virtual register) to request a write of
> +	 * 0xA5 to the ADC during the first 8 SCLKs of the ADC data read cycle,
> +	 * thus exiting continuous read.
> +	 */
> +	ret = regmap_write(st->regmap, AD4170_ADC_CTRL_CONT_READ_EXIT_REG, 0);
> +
> +	ret = regmap_update_bits(st->regmap, 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;

return 0;  When we know we are in a good path we should make that clear.

> +}