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

[Kim Seer Paller <kimseer.paller@analog.com>]

On Tue Jul 21, 2026 at 6:38 PM PST, Andy Shevchenko wrote:
> 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?

I agree, will split this into 2 lines.

>
> ...
>
> >  #define AD3531R_MAX_CHANNELS			4
> >  #define AD3532R_MAX_CHANNELS			16
> > +#define AD5710R_NUM_CHANNELS			8
>
> Why NUM and not MAX?

I think this can be drop since it was only used once in ad3530r_parse_channel_cfg()
and that check can used chip_info->num_channels instead.

>
> ...
>
> > +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?

The existing ad3530r/ad3532r regmap configs don't use a cache either,
so kept this one consistent. Can add it as a separate cleanup across
all 3 configs if preferred.

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