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

[Janani Sunil <jan.sun97@gmail.com>]

On 5/8/26 15:30, Jonathan Cameron wrote:
> On Fri, 8 May 2026 13:55:48 +0200
> Janani Sunil <janani.sunil@analog.com> wrote:
>
>> Add support for AD5529R 16-channel, 12/16 bit Digital to Analog Converter
>>
>> Signed-off-by: Janani Sunil <janani.sunil@analog.com>
>> +/* 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