Re: [PATCH v11 3/3] iio: dac: Add AD5529R DAC driver support

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 2 Sep 2026 15:56:49 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Sep 02, 2026 at 11:26:17AM +0200, Janani Sunil wrote:
> > Add support for AD5529R 16-channel, 12/16 bit Digital to Analog Converter
> > from Analog Devices.
> > 
> > The device communicates over SPI and supports per-channel output range
> > configuration. An optional external 4.096V reference can be used in
> > place of the internal reference.  
> 
> Since there might be a v12 (depends on Jonathan), a few more nit-picks.
> 
> ...
> 
> > +static const s32 ad5529r_output_ranges_mV[8][2] = {
> > +	[AD5529R_RANGE_0V_5V] = { 0, 5000 },
> > +	[AD5529R_RANGE_0V_10V] = { 0, 10000 },
> > +	[AD5529R_RANGE_0V_20V] = { 0, 20000 },
> > +	[AD5529R_RANGE_0V_40V] = { 0, 40000 },
> > +	[AD5529R_RANGE_M5V_5V] = { -5000, 5000 },
> > +	[AD5529R_RANGE_M10V_10V] = { -10000, 10000 },
> > +	[AD5529R_RANGE_M15V_15V] = { -15000, 15000 },
> > +	[AD5529R_RANGE_M20V_20V] = { -20000, 20000 },
> > +};  
> 
> Perhaps use MILLI multiplier?
I tried this and it looks pretty ugly so didn't adopt this one.
If it were many more zeros I'd agree.

> 
> ...
> 
> > +static int ad5529r_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int *val, int *val2, long mask)
> > +{
> > +	struct ad5529r_state *st = iio_priv(indio_dev);
> > +	unsigned int reg_addr, reg_val_h;
> > +	int ret, range_idx, span_mV;  
> 
> Why is 'range_idx' signed?

Why isn't it the enum like where it is coming from?
I changed it to the enum.

diff --git a/drivers/iio/dac/ad5529r.c b/drivers/iio/dac/ad5529r.c
index bc11d7671a76..8154b87ff831 100644
--- a/drivers/iio/dac/ad5529r.c
+++ b/drivers/iio/dac/ad5529r.c
@@ -228,8 +228,9 @@ static int ad5529r_read_raw(struct iio_dev *indio_dev,
                            int *val, int *val2, long mask)
 {
        struct ad5529r_state *st = iio_priv(indio_dev);
+       enum ad5529r_output_range range_idx;
        unsigned int reg_addr, reg_val_h;
-       int ret, range_idx, span_mV;
+       int ret, span_mV;
 
        switch (mask) {
        case IIO_CHAN_INFO_RAW:

> 
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		/*
> > +		 * Read from DAC_INPUT_A register rather than DAC_DATA_READBACK.
> > +		 * The DAC operates in transparent mode and directly reflects
> > +		 * whatever value is written to the INPUT_A register.
> > +		 */
> > +		reg_addr = AD5529R_REG_DAC_INPUT_A(chan->channel);
> > +		ret = regmap_read(st->regmap_16bit, reg_addr, &reg_val_h);
> > +		if (ret)
> > +			return ret;
> > +
> > +		*val = reg_val_h;
> > +
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		range_idx = st->output_range_idx[chan->channel];
> > +
> > +		/*
> > +		 * The datasheet specifies a 4.096 V external reference,
> > +		 * matching the nominal output voltage of the internal
> > +		 * reference.
> > +		 */
> > +		span_mV = ad5529r_output_ranges_mV[range_idx][1] -
> > +			  ad5529r_output_ranges_mV[range_idx][0];
> > +		*val = span_mV;
> > +		*val2 = st->model_data->resolution;
> > +
> > +		return IIO_VAL_FRACTIONAL_LOG2;
> > +	case IIO_CHAN_INFO_OFFSET:
> > +		range_idx = st->output_range_idx[chan->channel];
> > +
> > +		if (ad5529r_output_ranges_mV[range_idx][0] < 0)
> > +			*val = -BIT(st->model_data->resolution - 1);
> > +		else
> > +			*val = 0;
> > +
> > +		return IIO_VAL_INT;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}  
>