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

Andy Shevchenko
2025-04-29T22:01:14Z

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