Re: [PATCH v6 5/5] iio: dac: Add AD5529R DAC driver support

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

On Wed, Jul 15, 2026 at 01:41:08PM +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.

You haven't added a given tag, why?

In any case I have noticed that there are a number of (not so critical) issues
are still exists, so I have to withdraw my tag. But please, answer the above Q.

...

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

The second one is not needed in this case.

> +#include <linux/iio/iio.h>

> +#include <linux/mod_devicetable.h>

No new code with this header. Uwe did some rework WRT this header.

> +#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>
> +#include <linux/types.h>
> +#include <linux/units.h>

...

> +#define AD5529R_REG_INTERFACE_CONFIG_A		0x00

Use fixed-width values for all register offsets, e.g., here 0x000.

...

> +#define AD5529R_SPI_READ_FLAG			0x80

Isn't this a default in regmap SPI? Just asking, I don't remember by heart
the answer.

...

> +#define AD5529R_DAC_CHANNEL(chan) ((struct iio_chan_spec) {		\
> +	.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),		\
> +})

Please, remove unneeded (outer) parentheses.

...

> +struct ad5529r_state {
> +	struct spi_device *spi;

Not used. And the device pointer may be derived from regmap, so drop this.

> +	const struct ad5529r_model_data *model_data;
> +	struct regmap *regmap_8bit;
> +	struct regmap *regmap_16bit;
> +	struct iio_chan_spec channels[16];
> +	unsigned int num_channels;
> +	enum ad5529r_output_range output_range_idx[16];
> +};

...

> +static int ad5529r_reset(struct ad5529r_state *st)
> +{

So, basically here you can

	struct regmap *map = ad5529r_get_regmap(st, AD5529R_REG_INTERFACE_CONFIG_A);
	struct device *dev = regmap_get_device(regmap);

> +	struct reset_control *rst;
> +	int ret;
> +
> +	rst = devm_reset_control_get_optional_exclusive(&st->spi->dev, NULL);
> +	if (IS_ERR(rst))
> +		return PTR_ERR(rst);
> +
> +	if (rst) {
> +		ret = reset_control_assert(rst);
> +		if (ret)
> +			return ret;
> +
> +		ret = reset_control_deassert(rst);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regmap_write(st->regmap_8bit, AD5529R_REG_INTERFACE_CONFIG_A,
> +				   AD5529R_INTERFACE_CONFIG_A_SW_RESET);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Wait 10 ms for digital initialization to complete.
> +	 * Per datasheet, Interface Status A register NOT_READY_ERR bit is
> +	 * set if SPI transactions are attempted before digital initialization
> +	 * completes.
> +	 */
> +	fsleep(10 * USEC_PER_MSEC);
> +
> +	return regmap_write(st->regmap_8bit, AD5529R_REG_INTERFACE_CONFIG_A,
> +			    AD5529R_INTERFACE_CONFIG_A_SDO_ENABLE |
> +			    AD5529R_INTERFACE_CONFIG_A_ADDR_ASCENSION);
> +}

...

This is an unfinished review of some old submission. If anything, please check
it and update the driver either in a new round or consider followups.

-- 
With Best Regards,
Andy Shevchenko