Re: [PATCH v2 4/4] iio: dac: ad3530r: add support for AD5710R/AD5711R

[Andy Shevchenko]

On Tue, Jul 21, 2026 at 04:47:13PM +0800, Kim Seer Paller wrote:
> Add support for the AD5710R/AD5711R, 8-channel 16-/12-bit configurable
> IDAC/VDAC parts. They share the AD3530R register map and access model,
> so fold them into this driver.
> 
> Each channel is configured as voltage or current output from its DT
> channel@N node via adi,ch-func, building the iio_chan_spec dynamically.
> Voltage channels enable VMODE_EN and report the reference-derived scale,
> current channels report the 50 mA internal Iref scale. The powerdown
> mode is read-only and derived from the channel's configured type.

...

>   * AD3530R/AD3530 8-channel, 16-bit Voltage Output DAC Driver
>   * AD3531R/AD3531 4-channel, 16-bit Voltage Output DAC Driver
>   * AD3532R/AD3532 16-channel, 16-bit Voltage Output DAC Driver
> + * AD5710R/AD5711R 8-channel, 16-/12-bit Configurable IDAC/VDAC Driver

In the above only a single data width is mentioned, maybe split this one?

...

>  #define AD3531R_MAX_CHANNELS			4
>  #define AD3532R_MAX_CHANNELS			16
> +#define AD5710R_NUM_CHANNELS			8

Why NUM and not MAX?

...

> +static int ad5710r_get_powerdown_mode(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD5710R_CHN_VMODE_EN, &val);
> +	if (ret)
> +		return ret;
> +
> +	return !(val & AD5710R_CHN_VMODE_EN_BIT(chan->channel));

regmap_test_bits()

> +}

...

> +static ssize_t ad5710r_get_dac_powerdown(struct iio_dev *indio_dev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 char *buf)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	unsigned int reg_offset, ch_in_reg, reg, mode, mask;
> +	int ret;
> +
> +	reg_offset = chan->channel / AD3530R_CH_PER_REG;
> +	ch_in_reg = chan->channel % AD3530R_CH_PER_REG;
> +	reg = AD3530R_OUTPUT_OPERATING_MODE_0 + reg_offset;
> +	mask = AD3530R_OP_MODE_CHAN_MSK(ch_in_reg);
> +
> +	ret = regmap_read(st->regmap, reg, &mode);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%d\n", !!(mode & mask));

Ditto.

> +}

...

> +static const struct regmap_config ad5710r_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.max_register = AD5710R_CHN_VMODE_EN,
> +};

No cache?

...

> +static int ad3530r_parse_channel_cfg(struct ad3530r_state *st)
> +{
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct iio_chan_spec *channels;
> +	int ret, num_chan;

Why is 'num_chan' signed?


> +	int i = 0;

Signed? Also, split assignment and move it closer to its first user.

> +	u32 reg;
> +
> +	num_chan = device_get_child_node_count(dev);
> +	if (!num_chan)
> +		return dev_err_probe(dev, -ENODEV, "No channels configured\n");
> +
> +	channels = devm_kcalloc(dev, num_chan, sizeof(*channels), GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;


	i = 0;

> +	device_for_each_child_node_scoped(dev, child) {
> +		unsigned int reg_offset, ch_in_reg, mode_reg, mode_mask, ch_func;
> +		enum iio_chan_type chan_type;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to read reg property of %pfwP\n",
> +					     child);
> +
> +		if (reg >= AD5710R_NUM_CHANNELS)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "reg out of range in %pfwP\n",
> +					     child);
> +
> +		ret = fwnode_property_read_u32(child, "adi,ch-func", &ch_func);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Missing adi,ch-func property for %pfwP\n",
> +					     child);
> +
> +		switch (ch_func) {
> +		case CH_FUNC_VOLTAGE_OUTPUT:
> +			ret = regmap_set_bits(st->regmap, AD5710R_CHN_VMODE_EN,
> +					      AD5710R_CHN_VMODE_EN_BIT(reg));
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "Failed to set voltage output for %pfwP\n",
> +						     child);
> +
> +			chan_type = IIO_VOLTAGE;
> +			break;
> +		case CH_FUNC_CURRENT_OUTPUT:
> +			chan_type = IIO_CURRENT;
> +			break;
> +		default:
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Invalid adi,ch-func %u for %pfwP\n",
> +					     ch_func, child);
> +		}
> +
> +		channels[i] = ad5710r_channels[reg];
> +		channels[i].type = chan_type;
> +		i++;
> +
> +		reg_offset = reg / AD3530R_CH_PER_REG;
> +		ch_in_reg = reg % AD3530R_CH_PER_REG;
> +		mode_reg = AD3530R_OUTPUT_OPERATING_MODE_0 + reg_offset;
> +		mode_mask = AD3530R_OP_MODE_CHAN_MSK(ch_in_reg);
> +
> +		/* Enable the channel in normal operation mode */
> +		ret = regmap_update_bits(st->regmap, mode_reg, mode_mask,
> +					 field_prep(mode_mask, AD3530R_NORMAL_OP));
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to set normal operating mode for %pfwP\n",
> +					     child);
> +	}
> +
> +	st->channels = channels;
> +	st->num_channels = num_chan;
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko