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

[Jonathan Cameron]

> 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.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@xxxxxxxxxx>
>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 3b3c77d8b3e1..f108d5bac572 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -16,6 +16,7 @@ config AD3530R
>  	  - AD3530/AD3530R (8-channel)
>  	  - AD3531/AD3531R (4-channel)
>  	  - AD3532/AD3532R (16-channel)
> +	  - AD5710R/AD5711R (8-channel configurable IDAC/VDAC)
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad3530r.
> diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
> index 3056593a5ca0..dd8d5547fd16 100644
> --- a/drivers/iio/dac/ad3530r.c
> +++ b/drivers/iio/dac/ad3530r.c
> @@ -3,6 +3,8 @@
>   * 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 8-channel, 16-bit Configurable IDAC/VDAC Driver
> + * AD5711R 8-channel, 12-bit Configurable IDAC/VDAC Driver
>   *
>   * Copyright 2025 Analog Devices Inc.

FWIW this is a substantial enough change that I'd kind of expect any update
on the copywrite to 2025-26.

>   */
> @@ -27,6 +29,8 @@
>  #include <linux/types.h>
>  #include <linux/units.h>
>  
> +#include <dt-bindings/iio/addac/adi,ad74413r.h>
> +
>  #define AD3530R_INTERFACE_CONFIG_A		0x00
>  #define AD3530R_OUTPUT_OPERATING_MODE_0		0x20
>  #define AD3530R_OUTPUT_OPERATING_MODE_1		0x21
> @@ -39,6 +43,8 @@
>  #define AD3531R_SW_LDAC_TRIG_A			0xDD
>  #define AD3531R_INPUT_CH			0xE3
>  
> +#define AD5710R_CHN_VMODE_EN			0xFF
> +
>  /* AD3532R/AD3532 bank 0 registers (channels 0-7) */
>  #define AD3532R_INTERFACE_CONFIG_A_0		0x1000
>  #define AD3532R_OUTPUT_OPERATING_MODE_0		0x1020
> @@ -62,9 +68,11 @@
>  #define AD3530R_OUTPUT_CONTROL_RANGE		BIT(2)
>  #define AD3530R_REFERENCE_CONTROL_SEL		BIT(0)
>  #define AD3530R_OP_MODE_CHAN_MSK(chan)		(GENMASK(1, 0) << 2 * (chan))
> +#define AD5710R_CHN_VMODE_EN_BIT(chan)		BIT(chan)
>  
>  #define AD3530R_SW_RESET			(BIT(7) | BIT(0))
>  #define AD3530R_INTERNAL_VREF_mV		2500
> +#define AD5710R_INTERNAL_IREF_mA		50
>  #define AD3530R_LDAC_PULSE_US			100
>  
>  #define AD3530R_CH_PER_REG			4
> @@ -99,6 +107,7 @@ struct ad3530r_chip_info {
>  	unsigned int num_op_mode_regs;
>  	unsigned int resolution;
>  	bool internal_ref_support;
> +	bool channel_configurable;

If respinning for any other reason I think a more specific bool naming
would likely prove a better bet for the future.

>  };

> +static const char * const ad5710r_powerdown_modes[] = {
> +	"15kohm_to_gnd",
> +	"three_state",
> +};



> +static const struct iio_enum ad5710r_powerdown_mode_enum = {
> +	.items = ad5710r_powerdown_modes,
> +	.num_items = ARRAY_SIZE(ad5710r_powerdown_modes),
> +	.get = ad5710r_get_powerdown_mode,
> +};

> +static const struct iio_chan_spec_ext_info ad5710r_ext_info[] = {
> +	{
> +		.name = "powerdown",
> +		.shared = IIO_SEPARATE,
> +		.read = ad5710r_get_dac_powerdown,
> +		.write = ad5710r_set_dac_powerdown,
> +	},
> +	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad5710r_powerdown_mode_enum),
> +	{ }
> +};

This doesn't make sense.  The code is I advertising a choice of two power
own modes, but in reality as you state in the patch description there
is no choice as it comes from whether it is a voltage or a current
output channel.

Upshot, you can either not provide powerdown_mode_available (easiest
path here) or it for some reason userspace expects it, then you need
to make it only list the variant you want which means you can't use
the IIO_ENUM stuff to provide it.

> +
>  #define AD3530R_CHAN(_chan, _ext_info)				\
>  {								\
>  	.type = IIO_VOLTAGE,					\
> @@ -467,6 +574,17 @@ static const struct iio_chan_spec ad3532r_channels[] = {
>  	AD3530R_CHAN(15, ad3532r_ext_info),
>  };

...

> +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, -ENOENT, "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 mode_reg, mode_mask, ch_func;
> +		enum iio_chan_type chan_type;

I think reg is only used in the loop. If so, declare it here.

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

Rest looks good to me,

Thanks

Jonathan

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>