RE: [PATCH v3 2/4] iio: adc: ad4691: add initial driver for AD4691 family

[Sabau, Radu bogdan]

> -----Original Message-----
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Sent: Saturday, March 14, 2026 6:37 PM
> On 3/13/26 5:07 AM, Radu Sabau via B4 Relay wrote:
> > From: Radu Sabau <radu.sabau@xxxxxxxxxx>
> >

...

> > +
> > +static const struct iio_chan_spec ad4691_manual_channels[] = {
> > +	AD4691_CHANNEL(0, 0, 16, 24, 8),
> > +	AD4691_CHANNEL(1, 1, 16, 24, 8),
> > +	AD4691_CHANNEL(2, 2, 16, 24, 8),
> > +	AD4691_CHANNEL(3, 3, 16, 24, 8),
> > +	AD4691_CHANNEL(4, 4, 16, 24, 8),
> > +	AD4691_CHANNEL(5, 5, 16, 24, 8),
> > +	AD4691_CHANNEL(6, 6, 16, 24, 8),
> > +	AD4691_CHANNEL(7, 7, 16, 24, 8),
> > +	AD4691_CHANNEL(8, 8, 16, 24, 8),
> > +	AD4691_CHANNEL(9, 9, 16, 24, 8),
> > +	AD4691_CHANNEL(10, 10, 16, 24, 8),
> > +	AD4691_CHANNEL(11, 11, 16, 24, 8),
> > +	AD4691_CHANNEL(12, 12, 16, 24, 8),
> > +	AD4691_CHANNEL(13, 13, 16, 24, 8),
> > +	AD4691_CHANNEL(14, 14, 16, 24, 8),
> > +	AD4691_CHANNEL(15, 15, 16, 24, 8)
> > +};
> 
> Can't have 24-bit storage, but I guess that got fixed in a later patch.
> 

Will have this changed here on the next version.

> 
> > +
> > +static const struct iio_chan_spec ad4693_manual_channels[] = {
> > +	AD4691_CHANNEL(0, 0, 16, 24, 8),

...

> > +
> > +	ret = regmap_write(st->regmap, AD4691_REF_CTRL,
> > +			   FIELD_PREP(AD4691_REF_CTRL_MASK, ref_val));
> 
> As mentioned elsewhere, also expect to set REFBUF_EN here if needed.

You are also right about the REBUF_EN functionality, here and in probe.
The internal reference should be used if an external one is not present,
with the default reset value (no change to the ref_ctrl value in this case,
only to refbuf_en).
And if an external one is present, the reference voltage will be set
accordingly.

> 
> > +	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);
> 
> We should not be setting anything about the gpios without checking what the
> devicetree says is wired up.
> 
> I would defer adding all of the pwm/CNV_CLOCK/samling_freqnecy/GP0
> trigger
> stuff to the patch that actually adds SPI offload support for it to logically
> make sense.
> 

I understand your point, but these are also used in triggered buffer mode
as well so perhaps they should be moved there.