Re: [PATCH 2/3] iio: dac: ad5706r: Add support for AD5706R DAC

[Nuno Sá]

Hi Alexis,

Some initial feedback. I guess some discussion will be needed on the ABI you have.
>From a quick look it seems that at least some of it can fit in standard one and 
some might be DT.

On Fri, 2026-02-20 at 16:02 +0800, Alexis Czezar Torreno wrote:
> Add support for the Analog Devices AD5706R, a 4-channel 16-bit
> current output digital-to-analog converter with SPI interface.
> 
> Features:
>   - 4 independent DAC channels
>   - Hardware and software LDAC trigger
>   - Configurable output range
>   - PWM-based LDAC control
>   - Dither and toggle modes
>   - Dynamically configurable SPI speed
> 
> Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@xxxxxxxxxx>
> ---
>  drivers/iio/dac/Kconfig   |   11 +
>  drivers/iio/dac/Makefile  |    1 +
>  drivers/iio/dac/ad5706r.c | 2290 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 2302 insertions(+)
> 

...

> 
> +
> +static void ad5706r_debugs_init(struct iio_dev *indio_dev)
> +{
> +	struct dentry *d = iio_get_debugfs_dentry(indio_dev);
> +

It should have:

if (!IS_ENABLED(CONFIG_DEBUGFS))
	return

> +	debugfs_create_file_unsafe("streaming_addr", 0600, d,
> +				   indio_dev, &ad5706r_streaming_addr_fops);
> +	debugfs_create_file_unsafe("streaming_len", 0600, d,
> +				   indio_dev, &ad5706r_streaming_len_fops);
> +	debugfs_create_file_unsafe("streaming_data", 0600, d,
> +				   indio_dev, &ad5706r_streaming_data_fops);
> +	debugfs_create_file_unsafe("streaming_reg_access", 0600, d,
> +				   indio_dev, &ad5706r_streaming_reg_access_fops);
> +	debugfs_create_file_unsafe("spi_speed_hz_write", 0600, d,
> +				   indio_dev, &ad5706r_spi_speed_write_fops);
> +	debugfs_create_file_unsafe("spi_speed_hz_read", 0600, d,
> +				   indio_dev, &ad5706r_spi_speed_read_fops);
> +}

...

> 
> +
> +static int ad5706r_mux_out_sel_write(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     unsigned int item)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	unsigned int reg_value;
> +	int ret;
> +
> +	/* Validate index */
> +	if (item >= ARRAY_SIZE(mux_out_sel_reg_values))
> +		return -EINVAL;
> +
> +	/* Convert index to register value */
> +	reg_value = mux_out_sel_reg_values[item];
> +
> +	ret = ad5706r_spi_write(st, AD5706R_REG_MUX_OUT_SEL,
> +				reg_value << st->shift_val);
> +	if (ret)
> +		return ret;
> +
> +	st->mux_out_sel = item;
> +
> +	return 0;
> +}
> +
> +static int ad5706r_mux_out_sel_read(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	u16 reg_val;
> +	u8 reg_byte;
> +	int ret;
> +	int i;
> +
> +	ret = ad5706r_spi_read(st, AD5706R_REG_MUX_OUT_SEL, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	/* Extract the 8-bit value */
> +	reg_byte = (reg_val >> st->shift_val) & 0xFF;
> +
> +	/* Find which index has this register value */
> +	for (i = 0; i < ARRAY_SIZE(mux_out_sel_reg_values); i++) {
> +		if (mux_out_sel_reg_values[i] == reg_byte) {
> +			st->mux_out_sel = i;
> +			return i;  /* Return index, not register value */
> +		}
> +	}
> +
> +	/* Unknown value - default to disabled */
> +	st->mux_out_sel = MUX_OUT_SEL_DISABLED;
> +	return MUX_OUT_SEL_DISABLED;
> +}

We already have other parts with a similar setting (in frequency with this). There it
supported in DT:

https://elixir.bootlin.com/linux/v6.19-rc5/source/Documentation/devicetree/bindings/iio/frequency/adi,adf4350.yaml#L82


Any benefit for this to be a runtime toggle?

> +
> +static const struct iio_enum ad5706r_mux_out_sel_enum = {
> +	.items = mux_out_sel_iio_dev_attr_vals,
> +	.num_items = ARRAY_SIZE(mux_out_sel_iio_dev_attr_vals),
> +	.set = ad5706r_mux_out_sel_write,
> +	.get = ad5706r_mux_out_sel_read,
> +};
> +
> +static ssize_t ad5706r_multi_dac_input_a_write(struct iio_dev *indio_dev,
> +					       uintptr_t private, const struct iio_chan_spec
> *chan,
> +					       const char *buf, size_t len)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	unsigned int reg_val;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 16, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad5706r_spi_write(st, AD5706R_REG_MULTI_DAC_INPUT_A,
> +				AD5706R_MASK_MULTI_DAC_INPUT_A(reg_val));
> +	if (ret)
> +		return ret;
> +
> +	return ret ? ret : len;
> +}
> +
> +static ssize_t ad5706r_multi_dac_input_a_read(struct iio_dev *indio_dev,
> +					      uintptr_t private, const struct iio_chan_spec
> *chan,
> +					      char *buf)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	u16 reg_val;
> +	int ret;
> +
> +	ret = ad5706r_spi_read(st, AD5706R_REG_MULTI_DAC_INPUT_A, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "0x%lx\n", AD5706R_MASK_MULTI_DAC_INPUT_A(reg_val));
> +}
> +
> +static int ad5706r_multi_dac_sw_ldac_trigger_write(struct iio_dev *indio_dev,
> +						   const struct iio_chan_spec *chan,
> +						   unsigned int item)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad5706r_spi_write(st, AD5706R_REG_MULTI_DAC_SW_LDAC, item << st->shift_val);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int ad5706r_multi_dac_sw_ldac_trigger_read(struct iio_dev *indio_dev,
> +						  const struct iio_chan_spec *chan)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	u16 reg_val;
> +	int ret;
> +
> +	ret = ad5706r_spi_read(st, AD5706R_REG_MULTI_DAC_SW_LDAC, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	return reg_val;
> +}
> +
> +static const struct iio_enum ad5706r_multi_dac_sw_ldac_trigger_enum = {
> +	.items = multi_dac_sw_ldac_trigger_iio_dev_attr_vals,
> +	.num_items = ARRAY_SIZE(multi_dac_sw_ldac_trigger_iio_dev_attr_vals),
> +	.set = ad5706r_multi_dac_sw_ldac_trigger_write,
> +	.get = ad5706r_multi_dac_sw_ldac_trigger_read,
> +};
> +
> +static ssize_t ad5706r_reference_volts_write(struct iio_dev *indio_dev,
> +					     uintptr_t private, const struct iio_chan_spec *chan,
> +					     const char *buf, size_t len)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	unsigned int reg_val;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 10, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	st->reference_volts = reg_val;

Really, can we get anything the user gives us :)?

> +
> +	return ret ? ret : len;
> +}
> +
> +static ssize_t ad5706r_reference_volts_read(struct iio_dev *indio_dev,
> +					    uintptr_t private, const struct iio_chan_spec *chan,
> +					    char *buf)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +
> +	return sysfs_emit(buf, "%u\n", st->reference_volts);
> +}

Ditto for the above. For DACs, we typically don't mess with the reference level at runtime
at the device can be in control of other HW.

> +
> +static int ad5706r_ref_select_write(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    unsigned int item)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad5706r_spi_write(st, AD5706R_REG_BANDGAP_CONTROL, item);
> +	if (ret)
> +		return ret;
> +
> +	st->ref_select = item;
> +
> +	return 0;
> +}
> +
> +static int ad5706r_ref_select_read(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	u16 reg_val;
> +	int ret;
> +
> +	ret = ad5706r_spi_read(st, AD5706R_REG_BANDGAP_CONTROL, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	if (reg_val)
> +		st->ref_select = REF_SELECT_INTERNAL;
> +	else
> +		st->ref_select = REF_SELECT_EXTERNAL;
> +
> +	return st->ref_select;
> +}
> +
> +static const struct iio_enum ad5706r_ref_select_enum = {
> +	.items = ref_select_iio_dev_attr_vals,
> +	.num_items = ARRAY_SIZE(ref_select_iio_dev_attr_vals),
> +	.set = ad5706r_ref_select_write,
> +	.get = ad5706r_ref_select_read,
> +};

The above does not look like a good idea IIUC. It seems this needs to be supported using
regulators.

...

> 
> +
> +static int ad5706r_output_state_read(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	u16 reg_val;
> +	u16 reg_val2;
> +	u16 reg_val3;
> +	u16 gpio_val;
> +	u16 mask_val;
> +	int ret;
> +
> +	ret = ad5706r_spi_read(st, AD5706R_REG_OUT_OPERATING_MODE, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad5706r_spi_read(st, AD5706R_REG_OUT_SWITCH_EN, &reg_val2);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad5706r_spi_read(st, AD5706R_REG_SHDN_EN, &reg_val3);
> +	if (ret)
> +		return ret;
> +
> +	mask_val = 1 << (chan->channel + st->shift_val);
> +	reg_val = mask_val & reg_val;
> +	reg_val2 = mask_val & reg_val2;
> +	reg_val3 = mask_val & reg_val3;
> +	gpio_val = gpiod_get_value_cansleep(st->shdn_gpio);
> +
> +	if (reg_val && !reg_val3)
> +		st->output_state[chan->channel] = OUTPUT_STATE_NORMAL_SW;
> +	else if (!reg_val && !reg_val2)
> +		st->output_state[chan->channel] = OUTPUT_STATE_SHUTDOWN_TO_TRISTATE_SW;
> +	else if (!reg_val && reg_val2)
> +		st->output_state[chan->channel] = OUTPUT_STATE_SHUTDOWN_TO_GND_SW;
> +	else if (reg_val && reg_val3 && gpio_val)
> +		st->output_state[chan->channel] = OUTPUT_STATE_NORMAL_HW;
> +	else if (reg_val && !reg_val2 && reg_val3 && !gpio_val)
> +		st->output_state[chan->channel] = OUTPUT_STATE_SHUTDOWN_TO_TRISTATE_HW;
> +	else if (reg_val && reg_val2 && reg_val3 && !gpio_val)
> +		st->output_state[chan->channel] = OUTPUT_STATE_SHUTDOWN_TO_GND_HW;
> +
> +	return st->output_state[chan->channel];
> +}

Also think we already have some standard ABI that seems to fit the above:

https://elixir.bootlin.com/linux/v6.19-rc5/source/Documentation/ABI/testing/sysfs-bus-iio#L758

> +
> +static const struct iio_enum ad5706r_output_state_enum = {
> +	.items = output_state_iio_dev_attr_vals,
> +	.num_items = ARRAY_SIZE(output_state_iio_dev_attr_vals),
> +	.set = ad5706r_output_state_write,
> +	.get = ad5706r_output_state_read,
> +};
> +
> +static int ad5706r_ldac_trigger_chn_write(struct iio_dev *indio_dev,
> +					  const struct iio_chan_spec *chan,
> +					  unsigned int item)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	bool val_func_en = 0;
> +	bool val_func_mode = 0;
> +	bool val_sync_async = 0;
> +	bool val_hw_sw = 0;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	st->ldac_trigger_chn[chan->channel] = LDAC_TRIGGER_CHN_NONE;
> +	st->toggle_trigger_chn[chan->channel] = TOGGLE_TRIGGER_CHN_NONE;
> +	st->dither_trigger_chn[chan->channel] = DITHER_TRIGGER_CHN_NONE;
> +
> +	if (item != LDAC_TRIGGER_CHN_NONE)
> +		val_sync_async = 1;	/* Write 1 LDAC_SYNC_ASYNC */
> +
> +	if (item == LDAC_TRIGGER_CHN_SW_TRIGGER)
> +		val_hw_sw = 1;		/* Write 1 LDAC_HW_SW for SW */
> +
> +	ret = _set_reg_channel_mode(st, chan->channel, val_func_en, val_func_mode,
> +				    val_sync_async, val_hw_sw);
> +	if (ret)
> +		return ret;
> +
> +	st->ldac_trigger_chn[chan->channel] = item;
> +
> +	return 0;
> +}
> +
> +static int ad5706r_ldac_trigger_chn_read(struct iio_dev *indio_dev,
> +					 const struct iio_chan_spec *chan)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +
> +	return st->ldac_trigger_chn[chan->channel];
> +}
> +
> +static const struct iio_enum ad5706r_ldac_trigger_chn_enum = {
> +	.items = ldac_trigger_chn_iio_dev_attr_vals,
> +	.num_items = ARRAY_SIZE(ldac_trigger_chn_iio_dev_attr_vals),
> +	.set = ad5706r_ldac_trigger_chn_write,
> +	.get = ad5706r_ldac_trigger_chn_read,
> +};
> +
> +static int ad5706r_toggle_trigger_chn_write(struct iio_dev *indio_dev,
> +					    const struct iio_chan_spec *chan,
> +					    unsigned int item)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	bool val_func_en = 0;
> +	bool val_func_mode = 0;
> +	bool val_sync_async = 0;
> +	bool val_hw_sw = 0;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	st->ldac_trigger_chn[chan->channel] = LDAC_TRIGGER_CHN_NONE;
> +	st->toggle_trigger_chn[chan->channel] = TOGGLE_TRIGGER_CHN_NONE;
> +	st->dither_trigger_chn[chan->channel] = DITHER_TRIGGER_CHN_NONE;
> +
> +	if (item != TOGGLE_TRIGGER_CHN_NONE)
> +		val_func_en = 1;	/* Write 1 FUNC_EN */
> +	if (item == TOGGLE_TRIGGER_CHN_SW_TRIGGER)
> +		val_hw_sw = 1;		/* Write 1 LDAC_HW_SW for SW */
> +
> +	ret = _set_reg_channel_mode(st, chan->channel, val_func_en, val_func_mode,
> +				    val_sync_async, val_hw_sw);
> +	if (ret)
> +		return ret;
> +
> +	st->toggle_trigger_chn[chan->channel] = item;
> +
> +	return 0;
> +}
> +
> +static int ad5706r_toggle_trigger_chn_read(struct iio_dev *indio_dev,
> +					   const struct iio_chan_spec *chan)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +
> +	return st->toggle_trigger_chn[chan->channel];
> +}
> +
> +static const struct iio_enum ad5706r_toggle_trigger_chn_enum = {
> +	.items = toggle_trigger_chn_iio_dev_attr_vals,
> +	.num_items = ARRAY_SIZE(toggle_trigger_chn_iio_dev_attr_vals),
> +	.set = ad5706r_toggle_trigger_chn_write,
> +	.get = ad5706r_toggle_trigger_chn_read,
> +};
> +
> +static int ad5706r_dither_trigger_chn_write(struct iio_dev *indio_dev,
> +					    const struct iio_chan_spec *chan,
> +					    unsigned int item)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	bool val_func_en = 0;
> +	bool val_func_mode = 1;
> +	bool val_sync_async = 0;
> +	bool val_hw_sw = 0;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	st->ldac_trigger_chn[chan->channel] = LDAC_TRIGGER_CHN_NONE;
> +	st->toggle_trigger_chn[chan->channel] = TOGGLE_TRIGGER_CHN_NONE;
> +	st->dither_trigger_chn[chan->channel] = DITHER_TRIGGER_CHN_NONE;
> +
> +	if (item != DITHER_TRIGGER_CHN_NONE)
> +		val_func_en = 1;	/* Write 1 FUNC_EN */
> +	if (item == DITHER_TRIGGER_CHN_SW_TRIGGER)
> +		val_hw_sw = 1;		/* Write 1 LDAC_HW_SW for SW */
> +
> +	ret = _set_reg_channel_mode(st, chan->channel, val_func_en, val_func_mode,
> +				    val_sync_async, val_hw_sw);
> +	if (ret)
> +		return ret;
> +
> +	st->dither_trigger_chn[chan->channel] = item;
> +
> +	return 0;
> +}
> +
> +static int ad5706r_dither_trigger_chn_read(struct iio_dev *indio_dev,
> +					   const struct iio_chan_spec *chan)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +
> +	return st->dither_trigger_chn[chan->channel];
> +}
> +
> +static const struct iio_enum ad5706r_dither_trigger_chn_enum = {
> +	.items = dither_trigger_chn_iio_dev_attr_vals,
> +	.num_items = ARRAY_SIZE(dither_trigger_chn_iio_dev_attr_vals),
> +	.set = ad5706r_dither_trigger_chn_write,
> +	.get = ad5706r_dither_trigger_chn_read,
> +};


There are already some defined ABI for DACs which have toggle and dither modes:

https://elixir.bootlin.com/linux/v6.19-rc5/source/Documentation/ABI/testing/sysfs-bus-iio-dac

Make sure to see what can be reused and what needs to be added.

> +
> +static int ad5706r_multi_dac_sel_ch_write(struct iio_dev *indio_dev,
> +					  const struct iio_chan_spec *chan,
> +					  unsigned int item)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	u16 reg_val;
> +	u16 mask_val;
> +	int ret;
> +
> +	ret = ad5706r_spi_read(st, AD5706R_REG_MULTI_DAC_SEL_CH, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	mask_val = BIT(chan->channel + st->shift_val);
> +	reg_val = ~mask_val & reg_val;
> +
> +	if (item == MULTI_DAC_SEL_CH_EXCLUDE)
> +		ret = ad5706r_spi_write(st, AD5706R_REG_MULTI_DAC_SEL_CH,
> +					reg_val);
> +	else
> +		ret = ad5706r_spi_write(st, AD5706R_REG_MULTI_DAC_SEL_CH,
> +					reg_val | mask_val);
> +
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int ad5706r_multi_dac_sel_ch_read(struct iio_dev *indio_dev,
> +					 const struct iio_chan_spec *chan)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	u16 reg_val;
> +	u16 mask_val;
> +	int ret;
> +
> +	ret = ad5706r_spi_read(st, AD5706R_REG_MULTI_DAC_SEL_CH, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	mask_val = BIT(chan->channel + st->shift_val);
> +	reg_val = mask_val & reg_val;
> +
> +	if (reg_val)
> +		st->multi_dac_sel_ch[chan->channel] = MULTI_DAC_SEL_CH_INCLUDE;
> +	else
> +		st->multi_dac_sel_ch[chan->channel] = MULTI_DAC_SEL_CH_EXCLUDE;
> +
> +	return st->multi_dac_sel_ch[chan->channel];
> +}
> +
> +static const struct iio_enum ad5706r_multi_dac_sel_ch_enum = {
> +	.items = multi_dac_sel_ch_iio_chan_attr_vals,
> +	.num_items = ARRAY_SIZE(multi_dac_sel_ch_iio_chan_attr_vals),
> +	.set = ad5706r_multi_dac_sel_ch_write,
> +	.get = ad5706r_multi_dac_sel_ch_read,
> +};
> +
> +#define AD5706R_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) {	\
> +	.name = _name,							\
> +	.read = (_read),						\
> +	.write = (_write),						\
> +	.private = (_what),						\
> +	.shared = (_shared),						\
> +}
> +
> +static struct iio_chan_spec_ext_info ad5706r_ext_info[] = {
> +	/* device_attribute */
> +	AD5706R_CHAN_EXT_INFO("dev_addr", 0, IIO_SHARED_BY_ALL,
> +			      ad5706r_dev_addr_read, ad5706r_dev_addr_write),
> +
> +	IIO_ENUM("addr_ascension", IIO_SHARED_BY_ALL, &ad5706r_addr_ascension_enum),
> +	IIO_ENUM_AVAILABLE("addr_ascension", IIO_SHARED_BY_ALL, &ad5706r_addr_ascension_enum),
> +
> +	IIO_ENUM("single_instr", IIO_SHARED_BY_ALL, &ad5706r_single_instr_enum),
> +	IIO_ENUM_AVAILABLE("single_instr", IIO_SHARED_BY_ALL, &ad5706r_single_instr_enum),
> +
> +	IIO_ENUM("hw_ldac_tg_state", IIO_SHARED_BY_ALL, &ad5706r_hw_ldac_tg_state_enum),
> +	IIO_ENUM_AVAILABLE("hw_ldac_tg_state", IIO_SHARED_BY_ALL,
> &ad5706r_hw_ldac_tg_state_enum),
> +
> +	/* Sampling Frequency part of read/write RAW */
> +
> +	IIO_ENUM("hw_ldac_tg_pwm", IIO_SHARED_BY_ALL, &ad5706r_hw_ldac_tg_pwm_enum),
> +	IIO_ENUM_AVAILABLE("hw_ldac_tg_pwm", IIO_SHARED_BY_ALL, &ad5706r_hw_ldac_tg_pwm_enum),
> +
> +	IIO_ENUM("mux_out_sel", IIO_SHARED_BY_ALL, &ad5706r_mux_out_sel_enum),
> +	IIO_ENUM_AVAILABLE("mux_out_sel", IIO_SHARED_BY_ALL, &ad5706r_mux_out_sel_enum),
> +
> +	AD5706R_CHAN_EXT_INFO("multi_dac_input_a", 0, IIO_SHARED_BY_ALL,
> +			      ad5706r_multi_dac_input_a_read, ad5706r_multi_dac_input_a_write),
> +
> +	IIO_ENUM("multi_dac_sw_ldac_trigger", IIO_SHARED_BY_ALL,
> +		 &ad5706r_multi_dac_sw_ldac_trigger_enum),
> +	IIO_ENUM_AVAILABLE("multi_dac_sw_ldac_trigger", IIO_SHARED_BY_ALL,
> +			   &ad5706r_multi_dac_sw_ldac_trigger_enum),
> +
> +	AD5706R_CHAN_EXT_INFO("reference_volts", 0, IIO_SHARED_BY_ALL,
> +			      ad5706r_reference_volts_read, ad5706r_reference_volts_write),
> +
> +	IIO_ENUM("ref_select", IIO_SHARED_BY_ALL, &ad5706r_ref_select_enum),
> +	IIO_ENUM_AVAILABLE("ref_select", IIO_SHARED_BY_ALL, &ad5706r_ref_select_enum),
> +
> +	IIO_ENUM("hw_shutdown_state", IIO_SHARED_BY_ALL, &ad5706r_hw_shutdown_state_enum),
> +	IIO_ENUM_AVAILABLE("hw_shutdown_state", IIO_SHARED_BY_ALL,
> &ad5706r_hw_shutdown_state_enum),
> +
> +	/* Channel Attributes */
> +	AD5706R_CHAN_EXT_INFO("input_register_a", 0, IIO_SEPARATE,
> +			      ad5706r_input_register_a_read, ad5706r_input_register_a_write),
> +
> +	AD5706R_CHAN_EXT_INFO("input_register_b", 0, IIO_SEPARATE,
> +			      ad5706r_input_register_b_read, ad5706r_input_register_b_write),
> +
> +	IIO_ENUM("hw_active_edge", IIO_SEPARATE, &ad5706r_hw_active_edge_enum),
> +	IIO_ENUM_AVAILABLE("hw_active_edge", IIO_SEPARATE, &ad5706r_hw_active_edge_enum),
> +
> +	IIO_ENUM("range_sel", IIO_SEPARATE, &ad5706r_range_sel_enum),
> +	IIO_ENUM_AVAILABLE("range_sel", IIO_SEPARATE, &ad5706r_range_sel_enum),
> +
> +	IIO_ENUM("output_state", IIO_SEPARATE, &ad5706r_output_state_enum),
> +	IIO_ENUM_AVAILABLE("output_state", IIO_SEPARATE, &ad5706r_output_state_enum),
> +
> +	IIO_ENUM("ldac_trigger_chn", IIO_SEPARATE, &ad5706r_ldac_trigger_chn_enum),
> +	IIO_ENUM_AVAILABLE("ldac_trigger_chn", IIO_SEPARATE, &ad5706r_ldac_trigger_chn_enum),
> +
> +	IIO_ENUM("toggle_trigger_chn", IIO_SEPARATE, &ad5706r_toggle_trigger_chn_enum),
> +	IIO_ENUM_AVAILABLE("toggle_trigger_chn", IIO_SEPARATE, &ad5706r_toggle_trigger_chn_enum),
> +
> +	IIO_ENUM("dither_trigger_chn", IIO_SEPARATE, &ad5706r_dither_trigger_chn_enum),
> +	IIO_ENUM_AVAILABLE("dither_trigger_chn", IIO_SEPARATE, &ad5706r_dither_trigger_chn_enum),
> +
> +	IIO_ENUM("multi_dac_sel_ch", IIO_SEPARATE, &ad5706r_multi_dac_sel_ch_enum),
> +	IIO_ENUM_AVAILABLE("multi_dac_sel_ch", IIO_SEPARATE, &ad5706r_multi_dac_sel_ch_enum),

Oh boy, not going through all of the above now but that's a lot of custom ABI. It definitely needs
and ABI doc explaining what's being done.

> +
> +	{},
> +};
> +
> +/* Channel */
> +static int ad5706r_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val,
> +			    int *val2,
> +			    long mask)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	u16 reg_val;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		scoped_guard(mutex, &st->lock) {
> +			ret = ad5706r_spi_read(st, AD5706R_REG_DAC_DATA_READBACK_CH(chan-
> >channel),
> +					       &reg_val);
> +
> +			if (ret)
> +				return ret;
> +
> +			*val = reg_val;
> +		}
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (st->range_sel[chan->channel]) {
> +		case RANGE_SEL_50:
> +			*val = 50 * HZ_PER_MHZ / AD5706R_DAC_MAX_CODE;
> +			break;
> +		case RANGE_SEL_150:
> +			*val = 150 * HZ_PER_MHZ / AD5706R_DAC_MAX_CODE;
> +			break;
> +		case RANGE_SEL_200:
> +			*val = 200 * HZ_PER_MHZ / AD5706R_DAC_MAX_CODE;
> +			break;
> +		case RANGE_SEL_300:

Same comment about the prefix.

> +		default:
> +			*val = 300 * HZ_PER_MHZ / AD5706R_DAC_MAX_CODE;
> +			break;
> +		}
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = 0;
> +		return IIO_VAL_INT;

If offset is 0 we should not need it.

> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->sampling_frequency;
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad5706r_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val,
> +			     int val2,
> +			     long mask)
> +{
> +	struct ad5706r_state *st = iio_priv(indio_dev);
> +	struct pwm_state ldacb_pwm_state;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		/* Sets minimum and maximum frequency */
> +		val = clamp(val, SAMPLING_FREQUENCY_MIN_HZ, SAMPLING_FREQUENCY_MAX_HZ);


Typically the macros should have the device name as prefix.

> +
> +		scoped_guard(mutex, &st->lock) {
> +			pwm_get_state(st->ldacb_pwm, &ldacb_pwm_state);
> +			ldacb_pwm_state.duty_cycle = DIV_ROUND_CLOSEST_ULL(NANO, 2 * val);
> +			ldacb_pwm_state.period = DIV_ROUND_CLOSEST_ULL(NANO, val);
> +			ldacb_pwm_state.enabled = true;
> +
> +			ret = pwm_apply_might_sleep(st->ldacb_pwm, &ldacb_pwm_state);
> +			if (ret)
> +				return ret;
> +
> +			st->sampling_frequency = val;
> +		}
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info ad5706r_info = {
> +	.read_raw = &ad5706r_read_raw,
> +	.write_raw = &ad5706r_write_raw,
> +	.debugfs_reg_access = &ad5706r_reg_access,
> +};
> +
> +#define AD5706R_CHAN(_channel) {				\
> +	.type = IIO_CURRENT,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			      BIT(IIO_CHAN_INFO_SCALE) |	\
> +			      BIT(IIO_CHAN_INFO_OFFSET),	\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> +	.output = 1,						\
> +	.indexed = 1,						\
> +	.channel = _channel,					\
> +	.ext_info = ad5706r_ext_info,				\
> +}
> +
> +static const struct iio_chan_spec ad5706r_channels[] = {
> +	AD5706R_CHAN(0),
> +	AD5706R_CHAN(1),
> +	AD5706R_CHAN(2),
> +	AD5706R_CHAN(3),
> +};
> +
> +static int _ad5706r_setup(struct ad5706r_state *st)
> +{
> +	struct pwm_state ldacb_pwm_state;
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +	int i;
> +
> +	guard(mutex)(&st->lock);

Hmm, why the above?

> +
> +	st->debug_streaming_len = 0;
> +	st->debug_streaming_data = 0;
> +	st->debug_streaming_addr = 0;

The above should not be needed.

> +	st->debug_spi_speed_hz_write = 10000000;
> +	st->debug_spi_speed_hz_read = 10000000;
> +
> +	st->dev_addr = 0x00;
> +	st->addr_ascension = ADDR_ASCENSION_DECREMENT;
> +	st->single_instr = SINGLE_INSTR_STREAMING;
> +	st->shift_val = 0;
> +	st->addr_desc = 1;
> +	st->hw_ldac_tg_state = HW_LDAC_TG_STATE_LOW;
> +	st->sampling_frequency = 1000000;
> +	st->hw_ldac_tg_pwm = HW_LDAC_TG_PWM_DISABLED;
> +	st->mux_out_sel = MUX_OUT_SEL_DISABLED;
> +	st->multi_dac_input_a = 0;
> +	st->reference_volts = 2500;
> +	st->ref_select = REF_SELECT_EXTERNAL;
> +	st->hw_shutdown_state = HW_SHUTDOWN_STATE_LOW;
> +
> +	for (i = 0; i < 4; i++) {
> +		st->hw_active_edge[i] = HW_ACTIVE_EDGE_RISING_EDGE;
> +		st->range_sel[i] = RANGE_SEL_50;
> +		st->output_state[i] = OUTPUT_STATE_NORMAL_SW;
> +		st->ldac_trigger_chn[i] = LDAC_TRIGGER_CHN_HW_TRIGGER;
> +		st->toggle_trigger_chn[i] = TOGGLE_TRIGGER_CHN_HW_TRIGGER;
> +		st->dither_trigger_chn[i] = DITHER_TRIGGER_CHN_HW_TRIGGER;
> +		st->multi_dac_sel_ch[i] = MULTI_DAC_SEL_CH_EXCLUDE;
> +	}
> +
> +	/* get spi_clk axi_clkgen, no enable as spi_engine driver enables it */
> +	st->reference_clk = devm_clk_get(dev, "spi_clk");
> +	if (IS_ERR(st->reference_clk))
> +		return dev_err_probe(dev, PTR_ERR(st->reference_clk),
> +				     "Failed to get AXI CLKGEN clock\n");
> +
> +	st->ldacb_pwm = devm_pwm_get(dev, "ad5706r_ldacb");
> +	if (IS_ERR(st->ldacb_pwm))
> +		return dev_err_probe(dev, PTR_ERR(st->ldacb_pwm),
> +				     "Failed to get LDACB PWM\n");

nit: new line

> +	pwm_get_state(st->ldacb_pwm, &ldacb_pwm_state);
> +	ldacb_pwm_state.duty_cycle = 0;
> +	ldacb_pwm_state.period = DIV_ROUND_CLOSEST_ULL(NANO, st->sampling_frequency);
> +	ldacb_pwm_state.enabled = true;
> +	ret = pwm_apply_might_sleep(st->ldacb_pwm, &ldacb_pwm_state);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to apply PWM state\n");
> +
> +	st->resetb_gpio = devm_gpiod_get_optional(dev, "dac-resetb", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->resetb_gpio)) {
> +		return dev_err_probe(dev, PTR_ERR(st->resetb_gpio),
> +				     "Failed to get RESET_B GPIO\n");
> +	}


reset gpios can now be handled using the reset subsystem.

> +
> +	st->shdn_gpio = devm_gpiod_get_optional(dev, "dac-shdn", GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->shdn_gpio)) {
> +		return dev_err_probe(dev, PTR_ERR(st->shdn_gpio),
> +				     "Failed to get SHDN GPIO\n");
> +	}
> +
> +	/*
> +	 * Get SPI max speed from device tree. Allows up to 100MHz.
> +	 * If value is taken from spi->max_speed_hz, it is capped at 25MHz.
> +	 */
> +	ret = device_property_read_u32(dev, "spi-max-frequency", &st->spi_max_speed_hz);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set SPI Max Speed\n");
> +
> +	st->spi_max_speed_hz = clamp(st->spi_max_speed_hz, SPI_MIN_SPEED_HZ, SPI_MAX_SPEED_HZ);
> +

Hmm, why do we need the above? The spi core should handle it. Why is it capped? Maybe because
the controller you tested this on can't handle it?

> +	return 0;
> +}
> +
> +static int ad5706r_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ad5706r_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	mutex_init(&st->lock);

These days, devm_mutex_init()

> +	st->spi = spi;
> +
> +	ret = _ad5706r_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = "ad5706r";
> +	indio_dev->info = &ad5706r_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ad5706r_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ad5706r_channels);
> +
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ad5706r_debugs_init(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ad5706r_of_match[] = {
> +	{ .compatible = "adi,ad5706r" },
> +	{ },
> +};

No need for comma.

> +MODULE_DEVICE_TABLE(of, ad5706r_of_match);
> +
> +static const struct spi_device_id ad5706r_id[] = {
> +	{ "ad5706r", 0 },
> +	{ }
> +};


Drop the 0.

- Nuno Sá
> ;