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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 9 Jun 2026 17:00:21 +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>

A couple of minor things inline.

The sashiko thing about not relying on how gpio resets work and so 
adding an explicit assert before deassert is best practice so make sure
to tidy that up as well.

> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 003431798498..f35e060b3643 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_AD5446) += ad5446.o
>  obj-$(CONFIG_AD5446_SPI) += ad5446-spi.o
>  obj-$(CONFIG_AD5446_I2C) += ad5446-i2c.o
>  obj-$(CONFIG_AD5449) += ad5449.o
> +obj-$(CONFIG_AD5529R) += ad5529r.o
>  obj-$(CONFIG_AD5592R_BASE) += ad5592r-base.o
>  obj-$(CONFIG_AD5592R) += ad5592r.o
>  obj-$(CONFIG_AD5593R) += ad5593r.o
> diff --git a/drivers/iio/dac/ad5529r.c b/drivers/iio/dac/ad5529r.c
> new file mode 100644
> index 000000000000..d2d0287d0f95
> --- /dev/null
> +++ b/drivers/iio/dac/ad5529r.c
> @@ -0,0 +1,517 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * AD5529R Digital-to-Analog Converter Driver
> + * 16-Channel, 12/16-Bit, 40V High Voltage Precision DAC
> + *
> + * Copyright 2026 Analog Devices Inc.
> + * Author: Janani Sunil <janani.sunil@analog.com>
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/iio/iio.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
> +#include <linux/spi/spi.h>
> +
> +#define AD5529R_REG_INTERFACE_CONFIG_A		0x00
> +#define AD5529R_REG_DEVICE_CONFIG		0x02
> +#define AD5529R_REG_CHIP_GRADE			0x06
> +#define AD5529R_REG_SCRATCH_PAD			0x0A
> +#define AD5529R_REG_SPI_REVISION		0x0B
> +#define AD5529R_REG_VENDOR_H			0x0D
> +#define AD5529R_REG_STREAM_MODE			0x0E
> +#define AD5529R_REG_INTERFACE_STATUS_A		0x11
> +#define AD5529R_REG_MULTI_DAC_CH_SEL		0x14
> +#define AD5529R_REG_OUT_RANGE_BASE		0x3C
> +#define AD5529R_REG_OUT_RANGE(ch)		(AD5529R_REG_OUT_RANGE_BASE + (ch) * 2)
> +#define AD5529R_REG_DAC_INPUT_A_BASE		0x148
> +#define AD5529R_REG_DAC_INPUT_A(ch)		(AD5529R_REG_DAC_INPUT_A_BASE + (ch) * 2)
> +#define AD5529R_REG_DAC_DATA_READBACK_BASE	0x16A
> +#define AD5529R_REG_TSENS_ALERT_FLAG		0x18C
> +#define AD5529R_REG_TSENS_SHTD_FLAG		0x18E
> +#define AD5529R_REG_FUNC_BUSY			0x1A0
> +#define AD5529R_REG_REF_SEL			0x1A2
> +#define AD5529R_REG_INIT_CRC_ERR_STAT		0x1A4
> +#define AD5529R_REG_MULTI_DAC_HOTPATH_SW_LDAC	0x1A8
> +
> +#define   AD5529R_INTERFACE_CONFIG_A_SW_RESET	(BIT(7) | BIT(0))
> +#define   AD5529R_INTERFACE_CONFIG_A_ADDR_ASCENSION	BIT(5)
> +#define   AD5529R_INTERFACE_CONFIG_A_SDO_ENABLE	BIT(4)
> +#define   AD5529R_REF_SEL_INTERNAL_REF		BIT(0)

This extra indent thing only makes sense if they are next to the register
address definitions - the intent is to make the visually different.

e.g.

#define AD5529R_REG_INTERFACE_CONFIG_A		0x00
#define   AD5529R_INTERFACE_CONFIG_A_SW_RESET	(BIT(7) | BIT(0))
#define   AD5529R_INTERFACE_CONFIG_A_ADDR_ASCENSION	BIT(5)
#define   AD5529R_INTERFACE_CONFIG_A_SDO_ENABLE	BIT(4)
#define AD5529R_REG_DEVICE_CONFIG		0x02
#define AD5529R_REG_CHIP_GRADE			0x06
#define AD5529R_REG_SCRATCH_PAD			0x0A
#define AD5529R_REG_SPI_REVISION		0x0B
#define AD5529R_REG_VENDOR_H			0x0D
#define AD5529R_REG_STREAM_MODE			0x0E
#define AD5529R_REG_INTERFACE_STATUS_A		0x11
#define AD5529R_REG_MULTI_DAC_CH_SEL		0x14
#define AD5529R_REG_OUT_RANGE_BASE		0x3C
#define AD5529R_REG_OUT_RANGE(ch)		(AD5529R_REG_OUT_RANGE_BASE + (ch) * 2)
#define AD5529R_REG_DAC_INPUT_A_BASE		0x148
#define AD5529R_REG_DAC_INPUT_A(ch)		(AD5529R_REG_DAC_INPUT_A_BASE + (ch) * 2)
#define AD5529R_REG_DAC_DATA_READBACK_BASE	0x16A
#define AD5529R_REG_TSENS_ALERT_FLAG		0x18C
#define AD5529R_REG_TSENS_SHTD_FLAG		0x18E
#define AD5529R_REG_FUNC_BUSY			0x1A0
#define AD5529R_REG_REF_SEL			0x1A2
#define   AD5529R_REF_SEL_INTERNAL_REF		BIT(0)


> +#define   AD5529R_MAX_REGISTER			0x232
> +#define   AD5529R_8BIT_REG_MAX			0x13
> +#define   AD5529R_SPI_READ_FLAG			0x80
There is no reason to do the extra indent for these 3.


> +static int ad5529r_parse_channel_ranges(struct device *dev,
> +					struct ad5529r_state *st)
> +{
> +	int ret, range_idx;
> +	u32 ch;
> +	s32 vals[2];
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		range_idx = AD5529R_RANGE_0V_5V;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &ch);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Missing reg property in channel node\n");
> +
> +		if (ch >= 16)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Invalid channel number: %u\n", ch);
> +
> +		/* Read u32 property into s32 to handle negative voltage ranges */

> +		if (!fwnode_property_read_u32_array(child,
> +						    "adi,output-range-microvolt",
> +						    (u32 *)vals, ARRAY_SIZE(vals))) {
> +			range_idx = ad5529r_find_output_range(vals);
> +			if (range_idx < 0)
> +				return dev_err_probe(dev, range_idx,
> +						     "Invalid range [%d %d] for ch %u\n",
> +						     vals[0], vals[1], ch);
> +		}

The indent is a little larger than ideal, but slight preference for doing explicit
checking for optional properties so as to separate buggy ones from deliberately not
there.

		if (fwnode_property_present("adi,output-range-microvolt")) {
			/* Read u32 property into s32 to handle negative voltage ranges */
			ret = fwnode_property_read_u32_array(child,
							    "adi,output-range-microvolt",
							    (u32 *)vals, ARRAY_SIZE(vals));
			if (ret < 0) // strictly if (ret) but the return value for this one is complex.
				return dev_err_probe();
			range_idx = ad5529r_find_output_range(vals);
			if (range_idx < 0)
				return dev_err_probe(dev, range_idx,
						     "Invalid range [%d %d] for ch %u\n",
						     vals[0], vals[1], ch);
		} else {
			range_idx = AD5529R_RANGE_0V_5V;
		}
> +
> +		st->output_range_idx[ch] = range_idx;
> +		ret = regmap_write(st->regmap_16bit,
> +				   AD5529R_REG_OUT_RANGE(ch), range_idx);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to configure range for ch %u\n",
> +					     ch);
> +	}
> +
> +	return 0;
> +}