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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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?

...

> +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?

> +	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;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko