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

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

On Mon, Aug 17, 2026 at 02:37:14PM +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.

...

> +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);
> +	int ret;
> +
> +	ret = regmap_test_bits(st->regmap, AD5710R_CHN_VMODE_EN,
> +			       AD5710R_CHN_VMODE_EN_BIT(chan->channel));
> +	if (ret < 0)
> +		return ret;

> +	return !ret;

I think this '!' requires for a comment.
	/* Negate the returned value as 0 represents ..., and 1 ... */

> +}

...

> +static int ad3530r_parse_channel_cfg(struct iio_dev *indio_dev)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct iio_chan_spec *channels;
> +	unsigned int num_chan, i;
> +	int ret;
> +	u32 reg;
> +
> +	num_chan = device_get_child_node_count(dev);
> +	if (!num_chan)
> +		return dev_err_probe(dev, -ENODEV, "No channels configured\n");

Optionally this can be -ENOENT, as we usually return in other APIs for
count == 0. (-ENODEV semantically seems also okay, but I just shared
an additional info for making a decision.)

> +	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 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 >= st->chip_info->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++;
> +
> +		ad5710r_get_op_mode_reg(reg, &mode_reg, &mode_mask);
> +
> +		/* 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);
> +	}
> +
> +	indio_dev->channels = channels;
> +	indio_dev->num_channels = num_chan;
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko