Re: [PATCH v2 4/4] iio: dac: ad3530r: Add support for AD3532R/AD3532

[Andy Shevchenko]

On Mon, Jun 15, 2026 at 02:20:18PM +0800, Kim Seer Paller wrote:
> The AD3532R/AD3532 is a 16-channel, 16-bit voltage output DAC with a
> dual-bank register architecture (bank 0 at 0x1000 for channels 0-7,
> bank 1 at 0x3000 for channels 8-15). It shares similar functionality
> with AD3530R (channel configuration, LDAC triggering, powerdown control),
> the main difference being the register address map due to the dual-bank
> architecture, handled by table-driven helpers.
> 
> Add AD3532R-specific register definitions, channel specs, per-bank
> register arrays, a dedicated ad3532r_set_dac_powerdown(), and per-chip
> regmap_config to limit debugfs-exposed register space to each variant's
> actual address range.

...


>  	help
> -	  Say yes here to build support for Analog Devices AD3530R, AD3531R
> -	  Digital to Analog Converter.
> +	  Say yes here to build support for Analog Devices AD3530/AD3530R,
> +	  AD3531/AD3531R, and AD3532/AD3532R Digital to Analog Converters.

This just shows how unscalable the above text is. That's why we usually
recommend to make the list explicit and separated.

	  Say yes here to build support for the following Analog Devices
	  Digital to Analog Converters:
	  - AD3530/AD3530R (8-channel)
	  - AD3531/AD3531R (4-channel)
	  - AD3532/AD3532R (16-channel)

(and looking into the C-file change, perhaps add here as well distinctive
 information, such as number of channels, in the parentheses).

>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad3530r.

...

> +#define AD3532R_INTERFACE_CONFIG_A_0		0x1000
> +#define AD3532R_INTERFACE_CONFIG_A_1		0x3000
> +#define AD3532R_OUTPUT_OPERATING_MODE_0		0x1020
> +#define AD3532R_OUTPUT_OPERATING_MODE_1		0x1021
> +#define AD3532R_OUTPUT_OPERATING_MODE_2		0x3020
> +#define AD3532R_OUTPUT_OPERATING_MODE_3		0x3021
> +#define AD3532R_OUTPUT_CONTROL_0		0x102A
> +#define AD3532R_OUTPUT_CONTROL_1		0x302A
> +#define AD3532R_REFERENCE_CONTROL_0		0x103C
> +#define AD3532R_REFERENCE_CONTROL_1		0x303C
> +#define AD3532R_SW_LDAC_TRIG_0			0x10E5
> +#define AD3532R_SW_LDAC_TRIG_1			0x30E5
> +#define AD3532R_INPUT_CH_0			0x10EB
> +#define AD3532R_INPUT_CH_1			0x30EB
> +#define AD3532R_MAX_REG_ADDR			0x30F9

Hmm... I dunno if it's better to sort by values (so the "bank" 0 goes together
followed by "bank" 1). Jonathan, what's your preference here? Nuno, David?

...

> +static ssize_t ad3532r_set_dac_powerdown(struct iio_dev *indio_dev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 const char *buf, size_t len)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	unsigned int reg, pdmode, mask, val, local_ch;
> +	bool powerdown;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &powerdown);

Do you need to include kstrtox.h?

> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);

+ blank line here.

> +	local_ch = chan->channel % AD3530R_CH_PER_BANK;

> +	reg = (chan->channel < AD3530R_CH_PER_BANK ? AD3532R_OUTPUT_OPERATING_MODE_0 :
> +	       AD3532R_OUTPUT_OPERATING_MODE_2) +
> +	       local_ch / AD3530R_CH_PER_REG;

This is unreadable. Can you refactor it?

> +	mask = AD3530R_OP_MODE_CHAN_MSK(local_ch % AD3530R_CH_PER_REG);
> +
> +	pdmode = powerdown ? st->chan[chan->channel].powerdown_mode : 0;
> +	val = field_prep(mask, pdmode);
> +
> +	ret = regmap_update_bits(st->regmap, reg, mask, val);
> +	if (ret)
> +		return ret;
> +
> +	st->chan[chan->channel].powerdown = powerdown;
> +
> +	return len;
> +}

...

> +	.num_banks = ARRAY_SIZE(ad3532r_if_config),

Also check if array_size.h is included.

-- 
With Best Regards,
Andy Shevchenko