iio: dac: add ad5706r by actorreno · Pull Request #3096 · analogdevicesinc/linux

actorreno · 2026-01-28T10:10:47Z

PR Description

Adding driver for AD5706R, a quad 16-bit current output digital-to-analog converter with integrated precision reference.
Datasheet of AD5706R.
Features Implemented:
- 4 independent current output channels (16-bit resolution)
- Programmable output ranges: 50mA, 150mA, 200mA, 300mA
- Hardware and software LDAC triggering via PWM
- Toggle mode for dynamic current switching
- Dither mode for improved linearity
- Channel monitoring (voltage, current, temperature, PVDD)
- Configurable output states (normal, shutdown, tristate)
- Dynamic SPI interface up to 100MHz. Separate read and write speed.
- Debugfs streaming for register write of 32 bytes

PR Type

Bug fix (a change that fixes an issue)
New feature (a change that adds new functionality)
Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

I have conducted a self-review of my own code changes
I have compiled my changes, including the documentation
I have tested the changes on the relevant hardware
I have updated the documentation outside this repo accordingly
I have provided links for the relevant upstream lore

Add device tree bindings for the Analog Devices AD5706R 16-bit 4-channel current output DAC. The AD5706R features: - 4 independent current output channels - 16-bit resolution - SPI interface (up to 50MHz write, 25MHz read) - Multiple LDAC trigger modes (hardware/software) - Toggle and dither function generators - Programmable output ranges (50mA to 300mA) Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

stefpopa

A few small issues and some things that need to be clarified. Overall looks good!

stefpopa · 2026-02-10T08:17:18Z

drivers/iio/dac/ad5706r.c

+	ret = clk_set_rate(st->reference_clk, rate * 2);
+
+	/* Re-enable the clock after setting the rate */
+	clk_prepare_enable(st->reference_clk);


Should we check the return value of the clk_prepare_enable()? Otherwise, in case it fails, the clock remains disabled, but we move forward.

Yes i've checked a few other codes, will need to edit this to have error check.

stefpopa · 2026-02-10T08:20:31Z

drivers/iio/dac/ad5706r.c

+			break;
+		case RANGE_SEL_300:
+		default:
+			*val = 300 * MEGA / AD5706R_DAC_MAX_CODE;


For the other ranges, HZ_PER_MHZ is used, but for RANGE_SEL_300, MEGA is used, should we use the same constant everywhere? Otherwise, add a comment here explaining why a different constant is used.

will make this consistent (I'm also wondering why I did this)

stefpopa · 2026-02-10T08:24:14Z

drivers/iio/dac/ad5706r.c

+	mutex_init(&st->lock);
+	st->spi = spi;
+
+	_ad5706r_setup(st);


should we acquire a lock during the ad5706r_setup? It seems that inside the function we are accessing the HW.

will add a lock in the function

stefpopa · 2026-02-10T08:25:48Z

drivers/iio/dac/ad5706r.c

+			break;
+		case RANGE_SEL_300:
+		default:
+			*val = 300 * MEGA / AD5706R_DAC_MAX_CODE;


Why are we using a different range for RANGE_SEL_300? Either use MAGA everywhere or please add a comment here explaining why a different constant is needed.

already acknowledged similar comment above, will change for consistency

stefpopa · 2026-02-10T08:26:45Z

drivers/iio/dac/ad5706r.c

+	ret = clk_set_rate(st->reference_clk, rate * 2);
+
+	/* Re-enable the clock after setting the rate */
+	clk_prepare_enable(st->reference_clk);


Should we check the return value of the clk_prepare_enable()? Otherwise, if enabling the clock fails, we continue while the clock remains disabled.

ditto, similar comment above

drivers/iio/dac/ad5706r.c

stefpopa · 2026-02-10T08:38:17Z

drivers/iio/dac/ad5706r.c

+		if (val < 1)
+			val = 1;
+		if (val > 10000000)
+			val = 10000000;


We could define the minimum and maximum frequencies as constants at the top of the file with other defines and use clamp() to enforce the valid range for the PWM generator.

will figure out how to use this but yes noted on this.

stefpopa · 2026-02-10T08:48:35Z

drivers/iio/dac/ad5706r.c

+	ldacb_pwm_state.enabled = true;
+	ret = pwm_apply_might_sleep(st->ldacb_pwm, &ldacb_pwm_state);
+	if (ret)
+		return ret;


should we use dev_err_probe() here? Check everywhere in _ad5706r_setup(). Sometimes ret is returned directly, sometimes dev_err_probe() is returned. For consistency sake, we can return dev_err_probe() everywhere.

am not sure if its a style but

when the function is something like xxxx = devm_yyyy_get, this is where dev_err_probe is used
others just return ret

or is it any function in the probe must receieve the dev_err treatment

edit: i added the dev_err to everything inside the function for consistency

stefpopa · 2026-02-10T08:58:59Z

drivers/iio/dac/ad5706r.c

+		st->debug_spi_speed_hz_read = current_rate;
+
+	/* Wait for clock to stabilize */
+	usleep_range(3000, 3100);


This is concerning to me. It adds a 3ms delay with every SPI transaction. Is this a real HW requirement? Isn't it enough to add a short delay after initial setup? I assume 100-200us should be plenty.
Or is it necessary to have a _ad5706r_set_clk_rate() call with each read/write?

Yes, this is related to the not so common feature the requestor wants.

They want to have runtime configurable spi_speeds, and spi_read and spi_write should be separate speeds. The use case something like spi_write at 50MHz and read is 25Mhz.

After testing on higher frequencies, the data fetch fails if spi read/write are fast, not the same speed, and alternating. when read/write alternates the spi fails on high frequency setting.

This 3ms was manually tested as the minimum to stabilize the clock when alternating read/write.

If reads are infrequent, the 3ms delay on speed changes is acceptable, but if we have to perform frequent alternating read/writes, it's a heavy penalty.

Were you able to identify the root cause of the clock stabilization? Is it something related to the SPI Engine?

Anyway, I guess it's not so bad, if we do multiple consecutive writes and an occasional read.

my best guess here is it's caused by changing the actual clock rate of the spi_engine

i mean normally the way it goes is like
clk_gen -> spi_engine (regmap -> interconnect -> execution module) -> spi transaction

changing spi_speeds can be done in the execution module but this is only an integer divider (a 50Mhz input makes 25MHz, 16.66, 12.5, 10, etc)

requestor wanted more fine changes so I controlled the speed on the clk_gen itself

Aha! So, it must be related to the PLL lock time. Do you know what specific frequencies they actually need?

To quote the requestor's message to us:

"Reason is different customers have different clock requirements. We have seen numbers like 48MHz, 16.384MHz, 33MHz in optical modules where this part goes."

Ok, then the delay makes sense, as dynamic clock reconfiguration is necessary for these requirements. Sorry for asking so many questions, but I am a bit worried about making this acceptable upstream. In the best case scenario, we will need to provide an explanation.

drivers/iio/dac/ad5706r.c

actorreno · 2026-02-12T04:53:26Z

v2:

Error check with clk_prepare_enable
Consistency to use HZ_PER_MHZ
Added lock on setup function
Kept %lx as there was compiler warning using %x. Apparently GENMASK returns unsigned long
Added define for min/max of sampling freq, added clamp() in code to simplify if/else
Used dev_err in all error check inside the setup function
minor style issue fixed

stefpopa

The PR looks good overall, there is only one outstanding topic regarding the 3ms delay.
It seems that this delay is required for PLL reconfiguration which is needed for special speeds such as 48/16/33MHz.
However, PLL reconfiguration is not required for standard speeds such as 50/25MHz. Therefore, customers using these speeds should not experience any performance penalty.
One suggestion is to use a hybrid approach by trying to achieve a target frequency via the SPI Engine integer division (inside the _ad5706r_set_clk_rate() function). This eliminates the 3ms delay.
If this is not possible, then we must go on the "slow path" which needs precise frequency not achievable via integer division. This is the current implementation with the 3ms delay.
We also need to document this properly.
Additional suggestion is to add a device tree property to let customers choose the path. It can be something like this:

properties:
  adi,spi-speed-mode:
    description: |
      SPI clock speed configuration mode.
      
      "auto" (default): Automatically use integer division for standard 
      frequencies (faster), fall back to PLL reconfiguration for exact
      frequencies not achievable via division.
      
      "precise": Always use PLL reconfiguration for exact requested 
      frequency (adds 3ms delay on speed changes).
      
      "fast-only": Restrict to frequencies achievable via integer division
      from 100MHz base clock. Reject attempts to set other frequencies.
    enum: [auto, precise, fast-only]
    default: auto

actorreno · 2026-02-18T07:49:32Z

v3:

added automated clock mode switching
each time the user changes an SPI speed it goes to a clock mode of UNKNOWN
the driver will look at the desired write/read speeds. if in relation to the devicetree max-spi-frequency, the write and read values can be produced from the SPI engine clock divider, it goes to that mode. essentially simplifying frequency differences between read and write (sample: max_freq = 100MHz, write 50Mhz, read 25Mhz)
if read and write values are not simple, fall back and use the CLKGEN for spi clock changes, this incurs a manually tested 3ms delay each time the clock changes speed to accommodate clock stabilization especially in higher frequencies.

The SPI Engine mode was added to avoid the 3ms penalty of purely using the clkgen for SPI speed changes.

Regarding the Additional suggestion to add a device tree property, I currently opted not and did the automatic version.
aside from a more complex code, it feels more cumbersome to shift modes from/to 'fast-only' to/from 'precise' and both have their own cons. The automatic mode is still the best of both.

stefpopa

One small refactoring to improve readability, rest looks good.

drivers/iio/dac/ad5706r.c

Add driver for the Analog Devices AD5706R 16-bit 4-channel current output DAC with SPI interface. The device features: - 4 independent current output channels (OUT0-OUT3) - 16-bit resolution - Programmable output ranges: 50mA, 150mA, 200mA, 300mA - Hardware and software LDAC trigger modes - Toggle and dither function generators - Output shutdown modes (tri-state or ground) - Asynchronous/synchronous LDAC operation - Dynamic SPI clock frequency adjustment The driver provides IIO standard interfaces for: - DAC output control via raw values - Output range selection (scale) - LDAC trigger configuration - Output state control (normal/shutdown) - Function generator modes (toggle/dither) Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

Adding maintainer entry for AD5706R. Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

Editing kconfig.adi to imply AD5706R Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

stefpopa

LGTM! Good job!

actorreno · 2026-02-19T23:51:32Z

Will Proceed to upstream, thanks!

actorreno force-pushed the dev_ad5706r branch 3 times, most recently from d8113bb to 620846b Compare January 29, 2026 03:31

actorreno marked this pull request as ready for review January 29, 2026 07:15

actorreno requested review from CiprianRegus, amiclaus, btogorean, buha, chegbeli, danmois, dbogdan, machschmitt, mhennerich, nunojsa and rbolboac as code owners January 29, 2026 07:15

stefpopa requested changes Feb 10, 2026

View reviewed changes

actorreno force-pushed the dev_ad5706r branch 2 times, most recently from 98610ab to 6704915 Compare February 12, 2026 03:34

actorreno requested a review from stefpopa February 12, 2026 04:53

actorreno force-pushed the dev_ad5706r branch from 6704915 to 736645d Compare February 12, 2026 06:14

stefpopa requested changes Feb 12, 2026

View reviewed changes

actorreno force-pushed the dev_ad5706r branch 2 times, most recently from 398a3a6 to 36982ac Compare February 18, 2026 07:08

actorreno requested a review from stefpopa February 18, 2026 07:49

stefpopa reviewed Feb 18, 2026

View reviewed changes

drivers/iio/dac/ad5706r.c Outdated Show resolved Hide resolved

actorreno force-pushed the dev_ad5706r branch from 36982ac to 3262009 Compare February 19, 2026 01:52

actorreno added 2 commits February 19, 2026 10:26

MAINTAINERS: Add entry for AD5706R driver

b5e7589

Adding maintainer entry for AD5706R. Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

iio: dac: Kconfig.adi: imply ad5706r

8f5a813

Editing kconfig.adi to imply AD5706R Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

actorreno force-pushed the dev_ad5706r branch from 3262009 to 8f5a813 Compare February 19, 2026 02:27

actorreno requested a review from stefpopa February 19, 2026 04:53

stefpopa approved these changes Feb 19, 2026

View reviewed changes

Conversation

actorreno commented Jan 28, 2026

PR Description

PR Type

PR Checklist

Uh oh!

stefpopa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

actorreno Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

actorreno commented Feb 12, 2026

Uh oh!

stefpopa left a comment

Choose a reason for hiding this comment

Uh oh!

actorreno commented Feb 18, 2026

Uh oh!

stefpopa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stefpopa left a comment

Choose a reason for hiding this comment

Uh oh!

actorreno commented Feb 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

actorreno Feb 11, 2026 •

edited

Loading