Skip to content

iio: dac: add ad5706r (part 1)#3325

Open
actorreno wants to merge 3 commits into
mainfrom
dev_ad5706r_1
Open

iio: dac: add ad5706r (part 1)#3325
actorreno wants to merge 3 commits into
mainfrom
dev_ad5706r_1

Conversation

@actorreno
Copy link
Copy Markdown
Contributor

@actorreno actorreno commented May 19, 2026

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:
    • custom SPI in prep for future features of this part
    • simple raw/scale

This PR is related to #3096
The upstream asked this to be split.
Part 1 - basic driver
Part 2 - device attributes
Part 3 - Channel attributes
Part 4 - debugfs attributes

The commits in this PR are already accepted upstream and is cherry picked.

This PR is for merging, the next feature will be PRed on top of this.

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

actorreno added 3 commits May 19, 2026 10:13
Add device tree binding documentation for the Analog Devices
AD5706R 4-channel 16-bit current output digital-to-analog converter.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
Signed-off-by: Jonathan Cameron <jic23@kernel.org>
Add support for the Analog Devices AD5706R, a 4-channel 16-bit
current output digital-to-analog converter with SPI interface.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
Signed-off-by: Jonathan Cameron <jic23@kernel.org>
Imply ad5706r for build

Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
Copy link
Copy Markdown
Collaborator

@gastmaier gastmaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick driver check only

Comment thread drivers/iio/dac/ad5706r.c
if (num_bytes == 1)
st->tx_buf[2] = (u8)val;
else if (num_bytes == 2)
put_unaligned_be16(val, &st->tx_buf[2]);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer be16_to_cpu, cpu_to_be16 calls

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

As this was already accepted upstream, I don't think it's worth editing further. Though I think I understand the merit of the suggestion.

Comment thread drivers/iio/dac/ad5706r.c
u8 rx_buf[4];
};

static int ad5706r_reg_len(unsigned int reg)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline this method to allow the compiler to optimize

@gastmaier gastmaier added the llm review Request a review from a LLM Reviewer label May 19, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 19, 2026

LLM review

This series adds a new IIO DAC driver for the Analog Devices AD5706R, a 4-channel 16-bit current-output DAC with SPI interface.

run: 26096321359

b5370bfd6 - iio: dac: ad5706r: Add support for AD5706R DAC

Critical — Wrong register addresses for multibyte registers:

The AD5706R defaults to descending address mode (ADDR_ASCENSION = 0 in INTERFACE_CONFIG_A). In this mode, a two-byte register access must use the MSB byte address in the instruction phase; the device then auto-decrements to reach the LSB byte.

From the datasheet (§ Multibyte Registers):

"The DAC_INPUT_A_CH0 register is two bytes long, with the least significant byte at Address 0x60 and the most significant byte at Address 0x61. If the ADDR_ASCENSION bit is set to decrement, the address must be 0x61."

The driver macros use the LSB addresses:

#define AD5706R_REG_DAC_INPUT_A_CH(x)       (0x60 + ((x) * 2))  // wrong
#define AD5706R_REG_DAC_DATA_READBACK_CH(x) (0x68 + ((x) * 2))  // wrong

With address 0x60 in descending mode and put_unaligned_be16 (MSB first):

  • MSB of value → address 0x60 (should go to 0x61)
  • LSB of value → address 0x5F (outside the DAC register entirely)

No DAC write or readback produces a correct result. The fix:

#define AD5706R_REG_DAC_INPUT_A_CH(x)       (0x61 + ((x) * 2))
#define AD5706R_REG_DAC_DATA_READBACK_CH(x) (0x69 + ((x) * 2))

false positive (please ignore!) Minor — rx_buf missing DMA alignment:

tx_buf has __aligned(IIO_DMA_MINALIGN) but rx_buf does not. SPI controllers using DMA for the RX path require an aligned receive buffer. (false! next buffers to a already aligned buffers are already aligned!)

// drivers/iio/dac/ad5706r.c:39
u8 rx_buf[4];  // should be __aligned(IIO_DMA_MINALIGN) (**false**)

5b7a3f805 - dt-bindings: iio: dac: Add ADI AD5706R

Minor — spi-max-frequency in example is too high:

The example uses spi-max-frequency = <50000000> (50 MHz), which is only valid for write-only transactions. The datasheet specifies 25 MHz maximum for combined read+write operations (Table 4). Since the driver performs both reads and writes, the example should use <25000000>.

CI warnings

The CI warnings are all Node.js 20 deprecation notices unrelated to this series.

Verification data

The datasheet ad5705r-ad5706r.pdf was fetched from analog.com; the pre-converted markdown from https://raw.githubusercontent.com/analogdevicesinc/doctools/refs/heads/docling/media/en/technical-documentation/data-sheets/ad5705r-ad5706r.md was used to verify register addresses, multibyte access rules, and SPI timing specifications. The driver was compiled for arm and arm64 with W=1 — no warnings.

Suggested patches

Apply the suggested patches with:

cd path/to/repository
export GITHUB_TOKEN=ghp_***
apply-patches --repo=analogdevicesinc/linux 26096321359
Install instructions

The following one-liner installs the script if not present already:

curl -fSsL "https://raw.githubusercontent.com/analogdevicesinc/doctools/refs/heads/main/ci/scripts/apply-patches.sh" \
     -o ~/.local/bin/apply-patches.sh && \
  grep -q "/apply-patches.sh" ~/.bashrc || echo "source ~/.local/bin/apply-patches.sh" >> $_ ; . $_

More information at AI Usage.

@actorreno
Copy link
Copy Markdown
Contributor Author

the LLM review above makes a good point about the register address and such.

although this patch is already reflected upstream, the part 2 PR that adds other features,
fixes or considers this issue.

#3327

reset-gpios = <&gpio0 10 GPIO_ACTIVE_LOW>;
enable-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
};
};
Copy link
Copy Markdown
Collaborator

@rodrigo455 rodrigo455 May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe others can comment on that, but that is my view on the multi-device support

spi {
	#address-cells = <1>;
	#size-cells = <0>;

	multi-dac@0 {
		compatible = "adi,ad5706r";
		reg = <0>;
		spi-max-frequency = <50000000>;

		#address-cells = <1>;
		#size-cells = <0>;

		dac@0 {
  			reg = <0>
  			avdd-supply = <&avdd>;
  			iovdd-supply = <&iovdd>;
  			pvdd0-supply = <&pvdd>;
  			pvdd1-supply = <&pvdd>;
  			pvdd2-supply = <&pvdd>;
  			pvdd3-supply = <&pvdd>;
  			vref-supply = <&vref>;
  			pwms = <&pwm0 0 1000000 0>;
  			reset-gpios = <&gpio0 10 GPIO_ACTIVE_LOW>;
  			enable-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
		};

		dac@1 {
  			reg = <1>
  			avdd-supply = <&avdd>;
  			iovdd-supply = <&iovdd>;
  			pvdd0-supply = <&pvdd>;
  			pvdd1-supply = <&pvdd>;
  			pvdd2-supply = <&pvdd>;
  			pvdd3-supply = <&pvdd>;
  			vref-supply = <&vref>;
  			pwms = <&pwm0 0 1000000 0>;
  			reset-gpios = <&gpio0 10 GPIO_ACTIVE_LOW>;
  			enable-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
		};
};

each dac node provides 4 channels, this example could be a single 8-channel iio device or two 4-channel iio devices... need to evaluate which one is best.
Another point here is to figure out resource sharing... maybe we want to set pwms, reset-gpios or enable-gpios to the parent node? and child nodes can inherit them being able to override...

properties:
compatible:
enum:
- adi,ad5706r
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider adding support for ad5705r

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llm review Request a review from a LLM Reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants