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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 1 Jul 2026 08:40:41 +0200
Janani Sunil <janani.sunil@analog.com> 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.

This needs to respect the device address stuff in the dt binding.
It is fine to initially support just one device, but if it's address is 2 then
the driver should work.

> 
> Signed-off-by: Janani Sunil <janani.sunil@analog.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/iio/dac/Kconfig   |  17 ++
>  drivers/iio/dac/Makefile  |   1 +
>  drivers/iio/dac/ad5529r.c | 531 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 550 insertions(+)
A few other things inline.

Thanks,

> diff --git a/drivers/iio/dac/ad5529r.c b/drivers/iio/dac/ad5529r.c
> new file mode 100644
> index 000000000000..4841bd608482
> --- /dev/null
> +++ b/drivers/iio/dac/ad5529r.c

> +#define AD5529R_DAC_CHANNEL(chan, bits) {			\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.output = 1,						\
> +	.channel = (chan),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			      BIT(IIO_CHAN_INFO_SCALE) |	\
> +			      BIT(IIO_CHAN_INFO_OFFSET),	\
> +	.scan_type = {						\
> +		.format = 'u',					\
> +		.realbits = (bits),				\
> +		.storagebits = 16,				\

None of .scan_type is yet used.  I'd suggest removing that for now.
It can be brought back when it is needed for buffered output support.

> +	},							\
> +}

> +static const struct ad5529r_model_data ad5529r_16bit_model_data = {
> +	.model_name = "ad5529r-16",
> +	.resolution = 16,
> +	.channels = ad5529r_channels_16bit,
> +	.num_channels = ARRAY_SIZE(ad5529r_channels_16bit),
Sashiko picked up on this:
https://sashiko.dev/#/patchset/20260701-ad5529r-driver-v5-0-ed087900e642%40analog.com

When we have channel specific nodes in DT we have always taken the absence
of a given node to mean that it is not wired up and so the driver should not provide it.

I don't mind if for now we insist on all channels being present but we should
then check they are all there in DT or that there are dt-binding specified defaults.

The decision on whether to present all channels or just those in DT is a driver
one but really hard to change in future so much better to just present the
channels that have dt subnodes rather than all of them.


> +};

> +
> +static int ad5529r_probe(struct spi_device *spi)
> +{
> +	ret = devm_regulator_get_enable_optional(dev, "hvss");
> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get and enable hvss regulator\n");
> +
> +	/*
> +	 * The datasheet mentions a 4.096V external reference for correct
> +	 * operation.

I don't see the comment as adding much value given the person who needs
to see that is the board designer and they don't tend to read drivers :)
So probably drop this comment as noise to us.

> +	 */
> +	ret = devm_regulator_get_enable_optional(dev, "vref");
> +	if (ret == -ENODEV) {
> +		external_vref = false;
> +	} else if (ret) {
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get and enable vref regulator\n");
I'd just put that on one slightly long line.

> +	} else {
> +		external_vref = true;
> +	}

Those are all single statements so arguably don't need the {}  It is a bit
fuzzy when you have a line break like in the second one here though.

> +
> +	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 = regmap_assign_bits(st->regmap_16bit, AD5529R_REG_REF_SEL,
> +				 AD5529R_REF_SEL_INTERNAL_REF,
> +				 external_vref ? 0 : AD5529R_REF_SEL_INTERNAL_REF);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to configure reference\n");
> +
> +	ret = ad5529r_parse_channel_ranges(dev, st);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = st->model_data->model_name;
> +	indio_dev->info = &ad5529r_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = st->model_data->channels;
> +	indio_dev->num_channels = st->model_data->num_channels;

See above.  Sadly I think you need to generate these dynamically as we
have per channel dt sub nodes.

> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

>