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

[Janani Sunil]

On 5/8/26 15:30, Jonathan Cameron wrote:
> On Fri, 8 May 2026 13:55:48 +0200
> Janani Sunil <janani.sunil@xxxxxxxxxx> wrote:
>
> > Add support for AD5529R 16-channel, 12/16 bit Digital to Analog Converter
> >
> > Signed-off-by: Janani Sunil <janani.sunil@xxxxxxxxxx>
> > +/* Register Map */
> > +#define AD5529R_REG_INTERFACE_CONFIG_A		0x00
> > +#define AD5529R_REG_INTERFACE_CONFIG_B		0x01
> > +#define AD5529R_REG_DEVICE_CONFIG		0x02
> > +#define AD5529R_REG_CHIP_TYPE			0x03
> > +#define AD5529R_REG_PRODUCT_ID_L		0x04
> > +#define AD5529R_REG_PRODUCT_ID_H		0x05
> > +#define AD5529R_REG_CHIP_GRADE			0x06
> > +#define AD5529R_REG_SCRATCH_PAD			0x0A
> > +#define AD5529R_REG_SPI_REVISION		0x0B
> > +#define AD5529R_REG_VENDOR_L			0x0C
> > +#define AD5529R_REG_VENDOR_H			0x0D
> > +#define AD5529R_REG_STREAM_MODE			0x0E
> > +#define AD5529R_REG_TRANSFER_CONFIG		0x0F
> > +#define AD5529R_REG_INTERFACE_CONFIG_C		0x10
> > +#define AD5529R_REG_INTERFACE_STATUS_A		0x11
> > +
> > +/* Configuration registers */
> > +#define AD5529R_REG_MULTI_DAC_CH_SEL		(0x14 + 1)
> Feels like this would all be simpler if you used autoincrement rather than
> default value of autdecrement.  What breaks if you do that?
> Superficially feels like all the +1 would go away - though with need
> for a byte swap?  Might be worth that pain for the simpler code.
> Should just be a regmap_config parameter.

Switching to auto increment is feasible. I'll switch to auto increment and
eliminate all the +1 offsets.

> > +
> > +static const struct regmap_range ad5529r_8bit_readable_ranges[] = {
> > +	regmap_reg_range(AD5529R_REG_INTERFACE_CONFIG_A, AD5529R_REG_CHIP_GRADE),
> > +	regmap_reg_range(AD5529R_REG_SCRATCH_PAD, AD5529R_REG_VENDOR_H),
> > +	regmap_reg_range(AD5529R_REG_STREAM_MODE, AD5529R_REG_INTERFACE_STATUS_A),
> > +};
> > +
> > +static const struct regmap_range ad5529r_16bit_readable_ranges[] = {
> Tricky bit here is you are saying it's a 16 bit regmap but then providing
> address ranges including the ones we shouldn't use. We need to hide those
> intermediate addresses.  Various things might work depending on the addresses.
> Can we hide the bottom bit of each address then write it to appropriate value
> under the hood. That is divide addresses by 2?

I'll address this by using reg_stride = 2 in the 16-bit regmap configuration,
which automatically handles the address spacing and eliminates the need for manual
address range exclusion.

> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		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:
> > +		/*
> > +		 * Using default 0-5V range: VOUTn = A × D/2^N + B
> > +		 * where A = 5V, B = 0V, D = digital code, N = resolution
> > +		 * Scale = 5000mV / 2^resolution
> See the comment on the dt-binding. I think we need support for
> dt described output ranges from the start. This is a rare multi range
> device where we could set a safe default but to me it makes little sense
> and the driver will be doing something unexpected if a newer DT is
> provided with a different range.

I will add devicetree properties for per channel output range configuration.

> > +
> > +static int ad5529r_probe(struct spi_device *spi)
> > +{
> > +	struct device *dev = &spi->dev;
> > +	struct iio_dev *indio_dev;
> > +	struct ad5529r_state *st;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +
> > +	st->spi = spi;
> > +
> > +	ret = devm_regulator_bulk_get_enable(dev, AD5529R_NUM_SUPPLIES,
> > +					     ad5529r_supply_names);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to get and enable regulators\n");
> > +
> > +	st->regmap_8bit = devm_regmap_init_spi(spi, &ad5529r_regmap_8bit_config);
> > +	if (IS_ERR(st->regmap_8bit))
> > +		return dev_err_probe(dev, PTR_ERR(st->regmap_8bit),
> > +				     "Failed to initialize 8-bit regmap\n");
> > +
> > +	st->regmap_16bit = devm_regmap_init_spi(spi, &ad5529r_regmap_16bit_config);
> > +	if (IS_ERR(st->regmap_16bit))
> > +		return dev_err_probe(dev, PTR_ERR(st->regmap_16bit),
> > +				     "Failed to initialize 16-bit regmap\n");
> > +
> > +	ret = ad5529r_reset(st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to reset device\n");
> > +
> > +	ret = ad5529r_detect_device(st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to detect device variant\n");
> No to this. It breaks the use of fallback device tree compatibles.  As such we
> never fail on an ID missmatch. Instead we just believe firmware when it says
> whatever is there is compatible with this device. See below on why I think
> we need to break this into separate compatibles.

I'll create separate compatibles and remove the device ID detection logic.

Best Regards,
Janani Sunil