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

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

On Mon, Jun 29, 2026 at 04:31:07PM +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.

...

> +/* AD3532R/AD3532 has two register banks: bank 0 at 0x10xx, bank 1 at 0x30xx */

Split this to two comments, see below.

> +#define AD3532R_INTERFACE_CONFIG_A_0		0x1000
> +#define AD3532R_OUTPUT_OPERATING_MODE_0		0x1020
> +#define AD3532R_OUTPUT_OPERATING_MODE_1		0x1021
> +#define AD3532R_OUTPUT_CONTROL_0		0x102A
> +#define AD3532R_REFERENCE_CONTROL_0		0x103C
> +#define AD3532R_SW_LDAC_TRIG_0			0x10E5
> +#define AD3532R_INPUT_CH_0			0x10EB

+ Blank line and a comment.

> +#define AD3532R_INTERFACE_CONFIG_A_1		0x3000
> +#define AD3532R_OUTPUT_OPERATING_MODE_2		0x3020
> +#define AD3532R_OUTPUT_OPERATING_MODE_3		0x3021
> +#define AD3532R_OUTPUT_CONTROL_1		0x302A
> +#define AD3532R_REFERENCE_CONTROL_1		0x303C
> +#define AD3532R_SW_LDAC_TRIG_1			0x30E5
> +#define AD3532R_INPUT_CH_1			0x30EB
> +#define AD3532R_MAX_REG_ADDR			0x30F9

...

> +static int ad3532r_input_ch_reg(unsigned int channel)
> +{

Maybe

	unsigned int bank = channel / 8;
	unsigned int ch_in_reg = channel % 8;

> +	if (channel < 8)
> +		return 2 * channel + AD3532R_INPUT_CH_0;
> +
> +	return 2 * (channel - 8) + AD3532R_INPUT_CH_1;

	return 2 * ch_in_reg + (bank ? AD3532R_INPUT_CH_1 : AD3532R_INPUT_CH_0);

? This might need the correction in variable names. I tried to deduce them from
the _dac_powerdown() below. But if you think it makes things more complicated,
don't refactor.

> +}

...

> +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 bank, local_ch, reg_in_bank, ch_in_reg;
> +	unsigned int reg, pdmode, mask, val;
> +	bool powerdown;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &powerdown);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);

May chan->channel be modified behind our back here?
If not, what's the point of protecting the below lines
(till IO)?

> +	bank = chan->channel / AD3530R_CH_PER_BANK;
> +	local_ch = chan->channel % AD3530R_CH_PER_BANK;
> +	reg_in_bank = local_ch / AD3530R_CH_PER_REG;
> +	ch_in_reg = local_ch % AD3530R_CH_PER_REG;
> +
> +	reg = bank ? AD3532R_OUTPUT_OPERATING_MODE_2 :
> +		     AD3532R_OUTPUT_OPERATING_MODE_0;
> +	reg += reg_in_bank;

	reg = reg_in_bank + bank ? AD3532R_OUTPUT_OPERATING_MODE_2 :
				   AD3532R_OUTPUT_OPERATING_MODE_0;

> +	mask = AD3530R_OP_MODE_CHAN_MSK(ch_in_reg);

> +	pdmode = powerdown ? st->chan[chan->channel].powerdown_mode : 0;
> +	val = field_prep(mask, pdmode);
> +
> +	ret = regmap_update_bits(st->regmap, reg, mask, val);

Okay, now it's cleaner and we may make it even clearer:

	if (powerdown) {
		val = field_prep(mask, st->chan[chan->channel].powerdown_mode);
// Here is the question, do we even need a field_prep()?
		ret = regmap_update_bits(st->regmap, reg, mask, val);
	} else {
		ret = regmap_clear_bits(st->regmap, reg, mask);
	}

so pdmode variable is not needed.

> +	if (ret)
> +		return ret;
> +
> +	st->chan[chan->channel].powerdown = powerdown;
> +
> +	return len;
> +}

...

> +static int ad3532r_trigger_sw_ldac_reg(unsigned int channel)
> +{
> +	if (channel < 8)
> +		return AD3532R_SW_LDAC_TRIG_0;
> +
> +	return AD3532R_SW_LDAC_TRIG_1;
> +}

Taking the above, not sure if we benefit from the parametrized macros like

#define AD3532R_SW_LDAC_TRIG(channel)	\
	(((channel) < 8) ? AD3532R_SW_LDAC_TRIG_0 : AD3532R_SW_LDAC_TRIG_1)

...

> +	st->chip_info = spi_get_device_match_data(spi);
> +	if (!st->chip_info)
> +		return -ENODEV;

> -	st->regmap = devm_regmap_init_spi(spi, &ad3530r_regmap_config);
> +	st->regmap = devm_regmap_init_spi(spi, st->chip_info->regmap_config);
>  	if (IS_ERR(st->regmap))
>  		return dev_err_probe(dev, PTR_ERR(st->regmap),
>  				     "Failed to init regmap");

> -	st->chip_info = spi_get_device_match_data(spi);
> -	if (!st->chip_info)
> -		return -ENODEV;

This is simply moved up, make it happen in a separate patch.

-- 
With Best Regards,
Andy Shevchenko