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>
---
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];
 };
 
 static void ad4170_fill_sps_tbl(struct ad4170_state *st)
@@ -434,6 +444,10 @@ static int ad4170_reg_write(void *context, unsigned int reg, unsigned int val)
 	case 1:
 		st->tx_buf[2] = val;
 		break;
+	case 0:
+		/* Write continuous read exit code */
+		st->tx_buf[0] = AD4170_ADC_CTRL_CONT_READ_EXIT;
+		return spi_write(st->spi, st->tx_buf, 1);
 	default:
 		return -EINVAL;
 	}
@@ -843,6 +857,7 @@ static const struct iio_chan_spec ad4170_channel_template = {
 	.scan_type = {
 		.realbits = 24,
 		.storagebits = 32,
+		.shift = 8,
 		.endianness = IIO_BE,
 	},
 };
@@ -1451,11 +1466,27 @@ 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;
+
+	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,
 };
 
@@ -1731,16 +1762,166 @@ 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->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;
+}
+
+static bool ad4170_validate_scan_mask(struct iio_dev *indio_dev,
+				      const unsigned long *scan_mask)
+{
+	unsigned int masklength = iio_get_masklength(indio_dev);
+
+	/*
+	 * 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.
+	 */
+	if (bitmap_weight(scan_mask, masklength) > 1)
+		return test_bit(0, scan_mask);
+
+	return true;
+}
+
+static const struct iio_buffer_setup_ops ad4170_buffer_ops = {
+	.postenable = ad4170_buffer_postenable,
+	.predisable = ad4170_buffer_predisable,
+	.validate_scan_mask = ad4170_validate_scan_mask,
+};
+
+static irqreturn_t ad4170_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ad4170_state *st = iio_priv(indio_dev);
+	int i, ret;
+
+	iio_for_each_active_channel(indio_dev, i) {
+		ret = spi_sync(st->spi, &st->msg);
+		if (ret)
+			goto err_out;
+
+		st->bounce_buffer[i] = get_unaligned_be32(st->rx_buf);
+	}
+
+	iio_push_to_buffers(indio_dev, st->bounce_buffer);
+err_out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static const struct iio_trigger_ops ad4170_trigger_ops = {
+	.validate_device = iio_trigger_validate_own_device,
+};
+
 static irqreturn_t ad4170_irq_handler(int irq, void *dev_id)
 {
 	struct iio_dev *indio_dev = dev_id;
 	struct ad4170_state *st = iio_priv(indio_dev);
 
-	complete(&st->completion);
+	if (iio_buffer_enabled(indio_dev))
+		iio_trigger_poll(st->trig);
+	else
+		complete(&st->completion);
 
 	return IRQ_HANDLED;
 };
 
+static int ad4170_trigger_setup(struct iio_dev *indio_dev)
+{
+	struct ad4170_state *st = iio_priv(indio_dev);
+	int ret;
+
+	st->trig = devm_iio_trigger_alloc(indio_dev->dev.parent, "%s-trig%d",
+					  indio_dev->name,
+					  iio_device_id(indio_dev));
+	if (!st->trig)
+		return -ENOMEM;
+
+	st->trig->ops = &ad4170_trigger_ops;
+	st->trig->dev.parent = indio_dev->dev.parent;
+
+	iio_trigger_set_drvdata(st->trig, indio_dev);
+	ret = devm_iio_trigger_register(indio_dev->dev.parent, st->trig);
+	if (ret)
+		return dev_err_probe(&st->spi->dev, ret,
+				     "Failed to register trigger\n");
+
+	indio_dev->trig = iio_trigger_get(st->trig);
+
+	return 0;
+}
+
 static int ad4170_regulator_setup(struct ad4170_state *st)
 {
 	struct device *dev = &st->spi->dev;
@@ -1851,8 +2032,22 @@ static int ad4170_probe(struct spi_device *spi)
 				       indio_dev->name, indio_dev);
 		if (ret)
 			return ret;
+
+		ret = ad4170_trigger_setup(indio_dev);
+		if (ret)
+			return ret;
 	}
 
+	ret = ad4170_prepare_spi_message(st);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to prepare SPI message\n");
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+					      &ad4170_trigger_handler,
+					      &ad4170_buffer_ops);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to setup read buffer\n");
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 
-- 
2.47.2

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.

> +}

On Mon, Apr 28, 2025 at 3:28 PM 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

checks if the

> condition is met.

...

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

This will give, e.g., 3 for the realbits == 24. Is this expected?

> +       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_predisable(struct iio_dev *indio_dev)
> +{
> +       struct ad4170_state *st = iio_priv(indio_dev);
> +       int ret, i;

Why is 'i' signed?

> +       /*
> +        * 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);

No error check.

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

Wouldn't return 0; suffice?

> +}

...

> +static bool ad4170_validate_scan_mask(struct iio_dev *indio_dev,
> +                                     const unsigned long *scan_mask)
> +{
> +       unsigned int masklength = iio_get_masklength(indio_dev);
> +
> +       /*
> +        * 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.
> +        */
> +       if (bitmap_weight(scan_mask, masklength) > 1)

> +               return test_bit(0, scan_mask);

> +       return true;

Hmm... Is it possible to get weight == 0 and true here?

> +}

...

> +static irqreturn_t ad4170_trigger_handler(int irq, void *p)
> +{
> +       struct iio_poll_func *pf = p;
> +       struct iio_dev *indio_dev = pf->indio_dev;
> +       struct ad4170_state *st = iio_priv(indio_dev);
> +       int i, ret;

Why is 'i' signed? (And even in the original case it's inconsistent to
the similar in another function)

> +       iio_for_each_active_channel(indio_dev, i) {
> +               ret = spi_sync(st->spi, &st->msg);
> +               if (ret)
> +                       goto err_out;
> +
> +               st->bounce_buffer[i] = get_unaligned_be32(st->rx_buf);
> +       }
> +
> +       iio_push_to_buffers(indio_dev, st->bounce_buffer);
> +err_out:
> +       iio_trigger_notify_done(indio_dev->trig);
> +       return IRQ_HANDLED;
> +}

...

> +               return dev_err_probe(&st->spi->dev, ret,
> +                                    "Failed to register trigger\n");

One line?

-- 
With Best Regards,
Andy Shevchenko

Hi Andy, thank you for your review.

...
> > +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);
> 
> This will give, e.g., 3 for the realbits == 24. Is this expected?

Yes, in continuous read mode the ADC outputs just the conversion result bits
(24-bits) so a 3-byte length transfer is enough to get the conversion data for a
channel.

> 
...
> 
> > +               return dev_err_probe(&st->spi->dev, ret,
> > +                                    "Failed to register trigger\n");
> 
> One line?

It goes up to 89 columns if make in one line. I know there are other places in
this driver where 80 columns are exceeded, but in this case it's easier to
avoid going beyond 80 columns without drying up the error message.
Anyway, I'll make it one line if it's confirmed to be the preferable way to have
it.

Thanks,
Marcelo

On Wed, 30 Apr 2025 10:40:21 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> Hi Andy, thank you for your review.
> 
> ...
> > > +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);  
> > 
> > This will give, e.g., 3 for the realbits == 24. Is this expected?  
> 
> Yes, in continuous read mode the ADC outputs just the conversion result bits
> (24-bits) so a 3-byte length transfer is enough to get the conversion data for a
> channel.
> 
> >   
> ...
> >   
> > > +               return dev_err_probe(&st->spi->dev, ret,
> > > +                                    "Failed to register trigger\n");  
> > 
> > One line?  
> 
> It goes up to 89 columns if make in one line. I know there are other places in
> this driver where 80 columns are exceeded, but in this case it's easier to
> avoid going beyond 80 columns without drying up the error message.
> Anyway, I'll make it one line if it's confirmed to be the preferable way to have
> it.
In here there are what I think are multiple ways to get to the same ultimate
device. (indio->dev.parent is used the line above).  Better perhaps to
have one 'dev' that is appropriate for use in both places.

> 
> Thanks,
> Marcelo
>

On Wed, Apr 30, 2025 at 10:40:21AM -0300, Marcelo Schmitt wrote:

...

> > > +               return dev_err_probe(&st->spi->dev, ret,
> > > +                                    "Failed to register trigger\n");
> > 
> > One line?
> 
> It goes up to 89 columns if make in one line. I know there are other places in
> this driver where 80 columns are exceeded, but in this case it's easier to
> avoid going beyond 80 columns without drying up the error message.
> Anyway, I'll make it one line if it's confirmed to be the preferable way to have
> it.

The string literal endings are relaxed in the line limit for 10+ years...

-- 
With Best Regards,
Andy Shevchenko