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

Nuno Sá
2026-02-20T10:48:19Z

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@analog.com>
> ---
>  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á
> ;