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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 4 Jun 2026 15:13:45 +0800
Kim Seer Paller <kimseer.paller@analog.com> 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).
> 
> Introduce a table-driven register bank approach: per-chip register
> address arrays in chip_info are iterated by ad3530r_update_reg_banks()
> and ad3530r_write_reg_banks() helpers. This replaces the single-register
> setup calls for existing variants (AD3530R, AD3531R) and scales
> naturally to the AD3532R's dual-bank layout without per-variant
> conditionals in the setup path.
> 
> Convert sw_ldac_trig_reg from a static register address to a function
> pointer to handle the AD3532R's per-bank LDAC trigger registers.
> 
> Add AD3532R-specific powerdown modes (1kohm_to_gnd, 10kohm_to_gnd,
> three_state) matching the OUTPUT_OPERATING_MODE register encoding, and
> a dedicated ad3532r_set_dac_powerdown() using arithmetic channel-to-
> register mapping for the 16-channel address space.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
Hi

Some minor stuff inline. Biggest thing is split the patch into:
1) Patch that refactors existing code only
2) Patch that adds new device support.

Jonathan

> diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
> index d9db3226ecd6..2bdff438b2a0 100644
> --- a/drivers/iio/dac/ad3530r.c
> +++ b/drivers/iio/dac/ad3530r.c
> @@ -2,6 +2,7 @@

>  static ssize_t ad3530r_get_dac_powerdown(struct iio_dev *indio_dev,
>  					 uintptr_t private,
>  					 const struct iio_chan_spec *chan,
> @@ -163,9 +209,9 @@ static ssize_t ad3530r_set_dac_powerdown(struct iio_dev *indio_dev,
>  					 const char *buf, size_t len)
>  {
>  	struct ad3530r_state *st = iio_priv(indio_dev);
> -	int ret;
>  	unsigned int reg, pdmode, mask, val;
>  	bool powerdown;
> +	int ret;
>  
>  	ret = kstrtobool(buf, &powerdown);
>  	if (ret)
> @@ -190,6 +236,56 @@ static ssize_t ad3530r_set_dac_powerdown(struct iio_dev *indio_dev,
>  	return len;
>  }
>  
> +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);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +	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;
Wrap is rather hard to read.
	reg = (chan->channel < AD3530R_CH_PER_BANK ? AD3532R_OUTPUT_OPERATING_MODE_0 :	
	       AD3532R_OUTPUT_OPERATING_MODE_2) +
	      local_ch / AD3530R_CH_PER_REG;
helps a bit

> +	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;
> +}

> +static const struct ad3530r_chip_info ad3532_chip = {
> +	.name = "ad3532",
> +	.channels = ad3532r_channels,
> +	.num_channels = ARRAY_SIZE(ad3532r_channels),
> +	.sw_ldac_trig_reg = ad3532r_trigger_sw_ldac_reg,
> +	.input_ch_reg = ad3532r_input_ch_reg,
> +	.interface_config_a = ad3532r_if_config,
> +	.output_control = ad3532r_out_ctrl,
> +	.reference_control = ad3532r_ref_ctrl,
> +	.op_mode = ad3532r_op_mode,
> +	.num_banks = ARRAY_SIZE(ad3532r_if_config),
> +	.num_op_mode_regs = ARRAY_SIZE(ad3532r_op_mode),
> +	.internal_ref_support = false,
> +};
> +
> +static const struct ad3530r_chip_info ad3532r_chip = {
> +	.name = "ad3532r",
> +	.channels = ad3532r_channels,
> +	.num_channels = ARRAY_SIZE(ad3532r_channels),
> +	.sw_ldac_trig_reg = ad3532r_trigger_sw_ldac_reg,
> +	.input_ch_reg = ad3532r_input_ch_reg,
> +	.interface_config_a = ad3532r_if_config,
> +	.output_control = ad3532r_out_ctrl,
> +	.reference_control = ad3532r_ref_ctrl,
> +	.op_mode = ad3532r_op_mode,
> +	.num_banks = ARRAY_SIZE(ad3532r_if_config),
> +	.num_op_mode_regs = ARRAY_SIZE(ad3532r_op_mode),
> +	.internal_ref_support = true,
> +};

As mentioned below - split patch into refactors but no new parts, then
a patch just adding the new part support.

> +
> +static int ad3530r_update_reg_banks(const struct ad3530r_state *st,
> +				    const unsigned int *regs,
> +				    unsigned int num_regs,
> +				    unsigned int mask, unsigned int val)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < num_regs; i++) {
	for (unsigned int i...
> +		ret = regmap_update_bits(st->regmap, regs[i], mask, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad3530r_write_reg_banks(const struct ad3530r_state *st,
> +				   const unsigned int *regs,
> +				   unsigned int num_regs,
> +				   unsigned int val)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < num_regs; i++) {
	for (unsigned int i = 0;
> +		ret = regmap_write(st->regmap, regs[i], val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int ad3530r_setup(struct ad3530r_state *st, int external_vref_uV)
>  {
> +	const struct ad3530r_chip_info *chip_info = st->chip_info;
>  	struct device *dev = regmap_get_device(st->regmap);
>  	struct gpio_desc *reset_gpio;
> -	int i, ret;
>  	u8 range_multiplier, val;
> +	int i, ret;

Not in this patch. If you want to tidy up existing code ordering separate patch.

>  
>  	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>  	if (IS_ERR(reset_gpio))
> @@ -384,9 +644,10 @@ static int ad3530r_setup(struct ad3530r_state *st, int external_vref_uV)
>  		fsleep(1 * USEC_PER_MSEC);
>  		gpiod_set_value_cansleep(reset_gpio, 0);
>  	} else {
> -		/* Perform software reset */
> -		ret = regmap_update_bits(st->regmap, AD3530R_INTERFACE_CONFIG_A,
> -					 AD3530R_SW_RESET, AD3530R_SW_RESET);
> +		ret = ad3530r_update_reg_banks(st, chip_info->interface_config_a,
> +					       chip_info->num_banks,
> +					       AD3530R_SW_RESET,
> +					       AD3530R_SW_RESET);
>  		if (ret)
>  			return ret;
>  	}
> @@ -395,8 +656,10 @@ static int ad3530r_setup(struct ad3530r_state *st, int external_vref_uV)
>  
>  	range_multiplier = 1;
>  	if (device_property_read_bool(dev, "adi,range-double")) {
> -		ret = regmap_set_bits(st->regmap, AD3530R_OUTPUT_CONTROL_0,
> -				      AD3530R_OUTPUT_CONTROL_RANGE);
> +		ret = ad3530r_update_reg_banks(st, chip_info->output_control,
> +					       chip_info->num_banks,
> +					       AD3530R_OUTPUT_CONTROL_RANGE,
> +					       AD3530R_OUTPUT_CONTROL_RANGE);

Maybe worth a helper for 
ad3530r_set_reg_bank_bits() or something like that.

>  		if (ret)
>  			return ret;
>  
> @@ -406,8 +669,10 @@ static int ad3530r_setup(struct ad3530r_state *st, int external_vref_uV)
>  	if (external_vref_uV) {
>  		st->vref_mV = range_multiplier * external_vref_uV / MILLI;
>  	} else {
> -		ret = regmap_set_bits(st->regmap, AD3530R_REFERENCE_CONTROL_0,
> -				      AD3530R_REFERENCE_CONTROL_SEL);
> +		ret = ad3530r_update_reg_banks(st, chip_info->reference_control,
> +					       chip_info->num_banks,
> +					       AD3530R_REFERENCE_CONTROL_SEL,
> +					       AD3530R_REFERENCE_CONTROL_SEL);
>  		if (ret)
>  			return ret;
>  
> @@ -420,17 +685,11 @@ static int ad3530r_setup(struct ad3530r_state *st, int external_vref_uV)
>  	      FIELD_PREP(AD3530R_OP_MODE_CHAN_MSK(2), AD3530R_NORMAL_OP) |
>  	      FIELD_PREP(AD3530R_OP_MODE_CHAN_MSK(3), AD3530R_NORMAL_OP);
>  
> -	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0, val);
> +	ret = ad3530r_write_reg_banks(st, st->chip_info->op_mode,
> +				      st->chip_info->num_op_mode_regs, val);
>  	if (ret)

Given there is non trivial refactoring in here to make it easier to support the new
device, the patch should be spilt.  Refactors and no new support in first patch
so that we can just verify it does the same thing, then second patch adding new
device support.

>  		return ret;
>  
> -	if (st->chip_info->num_channels > 4) {
> -		ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_1,
> -				   val);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	for (i = 0; i < st->chip_info->num_channels; i++)
>  		st->chan[i].powerdown_mode = AD3530R_POWERDOWN_32K;
>  
> @@ -445,7 +704,7 @@ static int ad3530r_setup(struct ad3530r_state *st, int external_vref_uV)
>  static const struct regmap_config ad3530r_regmap_config = {
>  	.reg_bits = 16,
>  	.val_bits = 8,
> -	.max_register = AD3530R_MAX_REG_ADDR,
> +	.max_register = AD3532R_MAX_REG_ADDR,

What happens if we read off the end (via debugfs) for the smaller parts?

>  };
>  
>  static const struct iio_info ad3530r_info = {
> @@ -514,6 +773,8 @@ static const struct spi_device_id ad3530r_id[] = {
>  	{ "ad3530r", (kernel_ulong_t)&ad3530r_chip },
>  	{ "ad3531", (kernel_ulong_t)&ad3531_chip },
>  	{ "ad3531r", (kernel_ulong_t)&ad3531r_chip },
> +	{ "ad3532", (kernel_ulong_t)&ad3532_chip },
> +	{ "ad3532r", (kernel_ulong_t)&ad3532r_chip },

Add a precursor patch to switch this to named initializers. Otherwise
this will clash with the work Uwe is doing to ensure these are all done that
way.

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, ad3530r_id);
> @@ -523,6 +784,8 @@ static const struct of_device_id ad3530r_of_match[] = {
>  	{ .compatible = "adi,ad3530r", .data = &ad3530r_chip },
>  	{ .compatible = "adi,ad3531", .data = &ad3531_chip },
>  	{ .compatible = "adi,ad3531r", .data = &ad3531r_chip },
> +	{ .compatible = "adi,ad3532", .data = &ad3532_chip },
> +	{ .compatible = "adi,ad3532r", .data = &ad3532r_chip },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, ad3530r_of_match);
>