Re: [PATCH v3 2/4] iio: adc: ad4691: add initial driver for AD4691 family
From: Nuno Sá
Date: Sat Mar 14 2026 - 07:04:26 EST
Hi Radu,
Some comments from me. Unfortunately, only have time for the first patch :)
On Fri, 2026-03-13 at 12:07 +0200, Radu Sabau via B4 Relay wrote:
> From: Radu Sabau <radu.sabau@xxxxxxxxxx>
>
> Add support for the Analog Devices AD4691 family of high-speed,
> low-power multichannel SAR ADCs: AD4691 (16-ch, 500 kSPS),
> AD4692 (16-ch, 1 MSPS), AD4693 (8-ch, 500 kSPS) and
> AD4694 (8-ch, 1 MSPS).
>
> The driver implements a custom regmap layer over raw SPI to handle the
> device's mixed 1/2/3/4-byte register widths and uses the standard IIO
> read_raw/write_raw interface for single-channel reads.
>
> Two buffered operating modes are supported, auto-detected from the
> device tree:
>
> - CNV Clock Mode: an external PWM drives the CNV pin; the sampling
> rate is controlled via the PWM period. Requires a
> reference clock and a DATA_READY interrupt.
>
> - Manual Mode: CNV is tied to SPI CS; each SPI transfer triggers
> a conversion and returns the previous result
> (pipelined). No external clock or interrupt needed.
>
> In both modes the chip idles in Autonomous Mode so that single-shot
> read_raw can use the internal oscillator without disturbing the
> hardware configuration.
>
> Signed-off-by: Radu Sabau <radu.sabau@xxxxxxxxxx>
> ---
...
>
> +
> +static int ad4691_single_shot_read(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val)
> +{
> + struct ad4691_state *st = iio_priv(indio_dev);
> + u16 mask = ~BIT(chan->channel);
> + u32 acc_mask[2] = { mask & 0xFF, mask >> 8 };
> + unsigned int reg_val;
> + int ret;
> +
> + /*
> + * Always use AUTONOMOUS mode for single-shot reads, regardless
> + * of the buffer mode (CNV_CLOCK or MANUAL). The chip is kept
> + * in AUTONOMOUS mode during idle; enter_conversion_mode() and
> + * exit_conversion_mode() handle the switch for buffer operation.
> + */
> + ret = regmap_write(st->regmap, AD4691_STATE_RESET_REG,
> + AD4691_STATE_RESET_ALL);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> + BIT(chan->channel));
> + if (ret)
> + return ret;
> +
> + ret = regmap_bulk_write(st->regmap, AD4691_ACC_MASK1_REG, acc_mask, 2);
> + if (ret)
> + return ret;
Not DMA safe... Not sure if things changed in regmap_bulk_write() but before it was
not guaranteed that a safe buffer was going to be used.
> +
> + ret = regmap_write(st->regmap, AD4691_OSC_EN_REG, 1);
> + if (ret)
> + return ret;
> +
> + /*
> + * Wait for conversion to complete using a timed delay.
> + * A single read needs 2 internal oscillator periods.
> + * OSC_FREQ_REG is never modified by the driver, so the
> + * oscillator runs at reset-default speed. Use chip->max_rate
> + * as a conservative proxy: it is always <= the OSC frequency,
> + * so the computed delay is >= the actual conversion time.
> + */
> + unsigned long conv_us = DIV_ROUND_UP(2 * USEC_PER_SEC,
> + st->chip->max_rate);
> + fsleep(conv_us);
> +
> + ret = regmap_write(st->regmap, AD4691_OSC_EN_REG, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD4691_AVG_IN(chan->channel), ®_val);
> + if (ret)
> + return ret;
> +
> + *val = reg_val;
> + regmap_write(st->regmap, AD4691_STATE_RESET_REG, AD4691_STATE_RESET_ALL);
> +
No error handling.
> + return IIO_VAL_INT;
> +}
> +
> +static int ad4691_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long info)
> +{
> + struct ad4691_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW: {
> + IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +
> + if (IIO_DEV_ACQUIRE_FAILED(claim))
> + return -EBUSY;
> +
Side note. The above turned out to be a cool addition!
> + return ad4691_single_shot_read(indio_dev, chan, val);
> + }
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = ad4691_get_sampling_freq(st);
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + *val = st->vref_uV / 1000;
> + *val2 = chan->scan_type.realbits;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad4691_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return ad4691_set_sampling_freq(indio_dev, val);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad4691_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> + unsigned int writeval, unsigned int *readval)
> +{
> + struct ad4691_state *st = iio_priv(indio_dev);
> +
> + guard(mutex)(&st->lock);
> +
> + if (readval)
> + return regmap_read(st->regmap, reg, readval);
> +
> + return regmap_write(st->regmap, reg, writeval);
> +}
> +
> +static const struct iio_info ad4691_info = {
> + .read_raw = &ad4691_read_raw,
> + .write_raw = &ad4691_write_raw,
> + .debugfs_reg_access = &ad4691_reg_access,
> +};
> +
> +static int ad4691_reset(struct ad4691_state *st)
> +{
> + struct device *dev = regmap_get_device(st->regmap);
> + struct reset_control *rst;
> +
> + rst = devm_reset_control_get_optional_exclusive(dev, NULL);
> + if (IS_ERR(rst))
> + return dev_err_probe(dev, PTR_ERR(rst),
> + "Failed to get reset\n");
> +
> + if (!rst)
> + return 0;
> +
> + reset_control_assert(rst);
You can get the reset in the asserted state already.
> + /* Reset delay required. See datasheet Table 5. */
> + fsleep(300);
> + reset_control_deassert(rst);
> +
> + return 0;
> +}
> +
> +static int ad4691_config(struct ad4691_state *st)
> +{
> + struct device *dev = regmap_get_device(st->regmap);
> + enum ad4691_ref_ctrl ref_val;
> + unsigned int reg_val;
> + int ret;
> +
> + /*
> + * Determine buffer conversion mode from DT: if a PWM is provided it
> + * drives the CNV pin (CNV_CLOCK_MODE); otherwise CNV is tied to CS
> + * and each SPI transfer triggers a conversion (MANUAL_MODE).
> + * Both modes idle in AUTONOMOUS mode so that read_raw can use the
> + * internal oscillator without disturbing the hardware configuration.
> + */
> + if (device_property_present(dev, "pwms")) {
> + st->adc_mode = AD4691_CNV_CLOCK_MODE;
> + ret = ad4691_pwm_get(st);
> + if (ret)
> + return ret;
> + } else {
> + st->adc_mode = AD4691_MANUAL_MODE;
> + st->sampling_period =
> ns_to_ktime(DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,
> + AD4691_MANUAL_MODE_STD_FREQ(st->chip->num_channels,
> + to_spi_device(dev)->max_speed_hz)));
> + }
> +
> + /* Perform a state reset on the channels at start-up. */
> + ret = regmap_write(st->regmap, AD4691_STATE_RESET_REG,
> + AD4691_STATE_RESET_ALL);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to write state reset\n");
> +
> + /* Clear STATUS register by reading from the STATUS register. */
> + ret = regmap_read(st->regmap, AD4691_STATUS_REG, ®_val);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to read status
> register\n");
> +
> + switch (st->vref_uV) {
> + case AD4691_VREF_uV_MIN ... 2750000:
> + ref_val = AD4691_VREF_2P5;
> + break;
> + case 2750001 ... 3250000:
> + ref_val = AD4691_VREF_3P0;
> + break;
> + case 3250001 ... 3750000:
> + ref_val = AD4691_VREF_3P3;
> + break;
> + case 3750001 ... 4500000:
> + ref_val = AD4691_VREF_4P096;
> + break;
> + case 4500001 ... AD4691_VREF_uV_MAX:
> + ref_val = AD4691_VREF_5P0;
> + break;
> + default:
> + return dev_err_probe(dev, -EINVAL,
> + "Unsupported vref voltage: %d uV\n",
> + st->vref_uV);
> + }
> +
> + ret = regmap_write(st->regmap, AD4691_REF_CTRL,
> + FIELD_PREP(AD4691_REF_CTRL_MASK, ref_val));
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to write REF_CTRL\n");
> +
> + /* Both CNV_CLOCK and MANUAL devices start in AUTONOMOUS mode. */
> + ret = regmap_write(st->regmap, AD4691_ADC_SETUP,
> AD4691_AUTONOMOUS_MODE_VAL);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to write ADC_SETUP\n");
> +
> + return regmap_write(st->regmap, AD4691_GPIO_MODE1_REG, AD4691_ADC_BUSY);
> +}
> +
> +static int ad4691_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct ad4691_state *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + ret = devm_mutex_init(dev, &st->lock);
> + if (ret)
> + return ret;
> +
Agree with Andy here.
> + st->regmap = devm_regmap_init(dev, NULL, st, &ad4691_regmap_config);
> + if (IS_ERR(st->regmap))
> + return dev_err_probe(dev, PTR_ERR(st->regmap),
> + "Failed to initialize regmap\n");
> +
> + st->chip = spi_get_device_match_data(spi);
> +
> + ret = ad4691_regulator_get(st);
> + if (ret)
> + return ret;
> +
> + ret = ad4691_reset(st);
> + if (ret)
> + return ret;
> +
> + ret = ad4691_config(st);
> + if (ret)
> + return ret;
> +
> + indio_dev->name = st->chip->name;
> + indio_dev->info = &ad4691_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + indio_dev->channels = (st->adc_mode == AD4691_MANUAL_MODE) ?
> + st->chip->manual_channels : st->chip->channels;
> + indio_dev->num_channels = st->chip->num_channels;
nit: I would prefer explicit if() else
- Nuno Sá