On Wed, 2024-08-07 at 15:02 -0500, David Lechner wrote:
> This implements buffered reads for the ad4695 driver using the typical
> triggered buffer implementation, including adding a soft timestamp
> channel.
>
> The chip has 4 different modes for doing conversions. The driver is
> using the advanced sequencer mode since that is the only mode that
> allows individual configuration of all aspects each channel (e.g.
> bipolar config currently and oversampling to be added in the future).
>
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> ---
Hi David,
Just two nit comments...
Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> drivers/iio/adc/ad4695.c | 233 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 230 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> index 007ecb951bc3..a3bd5be36134 100644
> --- a/drivers/iio/adc/ad4695.c
> +++ b/drivers/iio/adc/ad4695.c
...
>
>
> +static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct ad4695_state *st = iio_priv(indio_dev);
> + struct spi_transfer *xfer;
> + u8 temp_chan_bit = st->chip_info->num_voltage_inputs;
> + bool temp_chan_en = false;
> + u32 reg, mask, val, bit, num_xfer, num_slots;
> + int ret;
> +
> + /*
> + * We are using the advanced sequencer since it is the only way to read
> + * multiple channels that allows individual configuration of each
> + * voltage input channel. Slot 0 in the advanced sequencer is used to
> + * account for the gap between trigger polls - we don't read data from
> + * this slot. Each enabled voltage channel is assigned a slot starting
> + * with slot 1.
> + */
> + num_slots = 1;
> +
> + memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer));
> +
> + /* First xfer is only to trigger conversion of slot 1, so no rx. */
> + xfer = &st->buf_read_xfer[0];
> + xfer->cs_change = 1;
> + xfer->delay.value = AD4695_T_CNVL_NS;
> + xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
> + xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
> + xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> + num_xfer = 1;
> +
> + iio_for_each_active_channel(indio_dev, bit) {
> + xfer = &st->buf_read_xfer[num_xfer];
> + xfer->bits_per_word = 16;
> + xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
> + xfer->len = 2;
> + xfer->cs_change = 1;
> + xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
> + xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> + if (bit == temp_chan_bit) {
> + temp_chan_en = true;
> + } else {
> + reg = AD4695_REG_AS_SLOT(num_slots);
> + val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit);
> +
> + ret = regmap_write(st->regmap, reg, val);
> + if (ret)
> + return ret;
> +
> + num_slots++;
> + }
> +
> + num_xfer++;
> + }
> +
> + /*
> + * Don't keep CS asserted after last xfer. Also triggers conversion of
> + * slot 0.
> + */
> + xfer->cs_change = 0;
> +
> + /**
> + * The advanced sequencer requires that at least 2 slots are enabled.
> + * Since slot 0 is always used for other purposes, we need only 1
> + * enabled voltage channel to meet this requirement. This error will
> + * only happen if only the temperature channel is enabled.
> + */
> + if (num_slots < 2) {
> + dev_err_ratelimited(&indio_dev->dev,
> + "Buffered read requires at least 1 voltage channel
> enabled\n");
This one is intriguing... Why the ratelimited variant? Normally you'd use that in IRQ
routines where the log could be flooded.
> + return -EINVAL;
> + }
> +
> + /*
> + * Temperature channel isn't included in the sequence, but rather
> + * controlled by setting a bit in the TEMP_CTRL register.
> + */
> +
> + reg = AD4695_REG_TEMP_CTRL;
> + mask = AD4695_REG_TEMP_CTRL_TEMP_EN;
> + val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
> +
> + ret = regmap_update_bits(st->regmap, reg, mask, val);
> + if (ret)
> + return ret;
> +
> + spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer,
> + num_xfer);
> +
> + ret = spi_optimize_message(st->spi, &st->buf_read_msg);
> + if (ret)
> + return ret;
> +
> + /* This triggers conversion of slot 0. */
> + ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
> + if (ret) {
> + spi_unoptimize_message(&st->buf_read_msg);
> + return ret;
> + }
Could save one line with (unless ad4695_enter_advanced_sequencer_mode() does not
return 0 on success)
ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
if (ret)
spi_unoptimize_message(&st->buf_read_msg);
return ret;
- Nuno Sá
