iio: dac: Add AD5413 support #2786

BruceTsaoADI · 2025-05-18T17:35:45Z

Summary

This PR adds initial support for the Analog Devices AD5413, a 14-bit single-channel DAC capable of voltage and current output.

Key changes

New driver: ad5413.c under drivers/iio/dac/
Devicetree binding: adi,ad5413.yaml under Documentation/devicetree/bindings/iio/dac/
Integrated with Kconfig and Makefile

Datasheet:

https://www.analog.com/media/en/technical-documentation/data-sheets/ad5413.pdf

PR Description

Please replace this comment with a summary of your changes, and add any context
necessary to understand them. List any dependencies required for this change.
To check the checkboxes below, insert a 'x' between square brackets (without
any space), or simply check them after publishing the PR.
If you changes include a breaking change, please specify dependent PRs in the
description and try to push all related PRs simultaneously.

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 tested the changes on the relevant hardware
I have updated the documentation outside this repo accordingly (if there is the case)

The AD5413 is a single-channel, 14-bit DAC with programmable current or voltage output. It communicates via SPI at clock rates up to 50 MHz. The output can be configured as either voltage or current and is available on a single terminal. Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5413.pdf Signed-off-by: Bruce Tsao <bruce.tsao@analog.com>

gastmaier

Added the warnings that caused the ci to exit with an error but were not logged

gastmaier · 2025-05-19T07:58:38Z

drivers/iio/dac/ad5413.c

+{
+	unsigned int tmp, tmparray[2], size;
+	const struct ad5413_range *range;
+	int *index, ret;


drivers/iio/dac/ad5413.c: In function 'ad5413_parse_dt': ::warning file=drivers/iio/dac/ad5413.c,line=590,col=14::gcc_fanalayzer: unused variable 'index' [-Wunused-variable] 590 | int *index, ret; | ^~~~~

I’ve removed the unused index variable from the ad5413_parse_dt() signature so that only int ret remains. This eliminates the -Wunused-variable warning. Please let me know if you spot any other unused declarations.

gastmaier · 2025-05-19T08:02:02Z

drivers/iio/dac/ad5413.c

+    else
+        indio_dev->channels = ad5413_current_ch;
+
+	ret = ad5413_init(st);
+	if (ret < 0) {
+		dev_err(&spi->dev, "AD5413 init failed\n");
+		return ret;
+	}
+
+	return devm_iio_device_register(&st->spi->dev, indio_dev);


drivers/iio/dac/ad5413.c: In function 'ad5413_probe': ::warning file=drivers/iio/dac/ad5413.c,line=724,col=5::gcc_fanalayzer: this 'else' clause does not guard... [-Wmisleading-indentation] 724 | else | ^~~~ drivers/iio/dac/ad5413.c:727:9: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'else' 727 | ret = ad5413_init(st); | ^~~

Thanks for spotting that. I’ve refactored the if/else block to use braces so the indentation matches the actual control flow. This removes the misleading indentation and eliminates the warning. Let me know if you’d like any further adjustments.

dlech · 2025-05-22T22:33:54Z

Documentation/devicetree/bindings/iio/dac/adi,ad5413.yaml

+    enum:
+      - [-10500000, 10500000]
+      - [-12600000, 12600000]


enum doesn't work like that. See other bindings that already have this same property for what works.

Suggested change

enum:

- [-10500000, 10500000]

- [-12600000, 12600000]

adi,range-microvolt:

description: Voltage output range specified as <minimum, maximum>

oneOf:

- items:

- const: -10500000

- const: 10500000

- items:

- const: -12600000

- const: 12600000

I’ve updated the adi,range-microvolt binding to use oneOf instead of enum.

dlech · 2025-05-22T22:34:44Z

Documentation/devicetree/bindings/iio/dac/adi,ad5413.yaml

+
+  adi,range-microamp:
+    description: Current output range <min, max> in microamps
+    enum:


Same here.

Will also want to call out in the commit message why there is only one option here (and why adi,range-microvolt doesn't have a default: - the same reason - but it isn't obvious until later, so best to mention it before)

I’ve updated adi,range-microamp to use oneOf with the single supported span (0–24 000 μA).

dlech · 2025-05-22T22:35:04Z

Documentation/devicetree/bindings/iio/dac/adi,ad5413.yaml

+    description: |
+      Output digital slew control time in microseconds


Suggested change

description: |

Output digital slew control time in microseconds

description: Output digital slew control time in microseconds

I’ve changed the adi,slew-time-us property to use an inline description instead of a multi-line block.

dlech · 2025-05-22T22:49:21Z

Documentation/devicetree/bindings/iio/dac/adi,ad5413.yaml

+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - if:
+      required: [adi, range-microamp]


Suggested change

required: [adi, range-microamp]

required: [ 'adi,range-microamp' ]

I’ve updated the required array to quote the property name as ['adi,range-microamp'].

dlech · 2025-05-22T22:49:52Z

Documentation/devicetree/bindings/iio/dac/adi,ad5413.yaml

+      properties:
+        adi,range-microvolt: false
+  - if:
+      required: [adi, range-microvolt]


Suggested change

required: [adi, range-microvolt]

required: [ 'adi,range-microvolt' ]

drivers/iio/dac/ad5413.c

dlech · 2025-05-22T23:17:44Z

drivers/iio/dac/ad5413.c

+    /* 3) read slew-time（optional） */
+    ret = device_property_read_u32(&st->spi->dev,
+                                  "adi,slew-time-us", &tmp);
+    if (ret) {


Checking all of the device_property_ returns in this function could be more strict. There are different errors for "property is missing" (-EINAL) and other error caused, e.g. by typos in the .dts file (e.g. a not enough items in the array).

Thanks for the catch. I’ve updated the adi,slew-time-us handling so that:

-ENODATA and -EINVAL are treated as “property not provided” and default to 0

Any other negative return is treated as a real parsing error (logged via dev_err) and aborts probe

Please let me know if this covers all cases or if further tweaks are needed.

dlech · 2025-05-22T23:21:13Z

drivers/iio/dac/ad5413.c

+
+#define AD5413_DAC_CHAN(_chan_type) {				\
+	.type = (_chan_type),					\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_RAW) |	\


IIO_CHAN_INFO_RAW is for sure separate, not shared_by_type. Since there aren't versions of this chip with more than one channel, it isn't clear if SCALE and OFFSET should also be separate, so could go either way on those.

Thank you for pointing that out. I’ve removed IIO_CHAN_INFO_RAW from info_mask_shared_by_type and added it to info_mask_separate to ensure RAW is treated as a separate attribute. SCALE and OFFSET remain in info_mask_shared_by_type since this device only has one channel. Please let me know if you’d prefer SCALE and OFFSET to be marked separate as well.

dlech · 2025-05-22T23:40:27Z

drivers/iio/dac/ad5413.c

+	int *index, ret;
+
+    /* 1)  voltage  */
+    ret = device_property_read_u32_array(&st->spi->dev,


This will allow the +/- 12.6 range to be given in the devicetree but set up the chip for +/-10.5V, but OVRNG_EN isn't implemented yet. So we should not allow that or now or implement OVRNG_EN.

It also wouldn't hurt to check the current values as well (even if there is only one option, someone might try to put something else in the .dts file)

Thank you for catching that. I’ve made the following updates:

Voltage range: The DT binding and driver now only allow ±10.5 V. Any attempt to use ±12.6 V is rejected with -EINVAL until we implement the OVRNG_EN bit.

Current range: Added explicit validation so that adi,range-microamp must be exactly 0–24 000 µA; anything else also returns -EINVAL.

With these checks in place, we prevent the chip from being configured for unsupported ranges. OVRNG_EN support can be added in a follow-up patch once fully tested.

dlech · 2025-05-22T23:44:11Z

drivers/iio/dac/ad5413.c

+
+struct ad5413_range {
+	int reg;
+	int min;


Upstream prefers units on macros and variables, e.g. min_uv here.

Same applies throughout the driver. (It would especially help to make the raw_read function easier to understand)

Thank you for the suggestion. I’ve renamed all range struct members and related macros to include explicit unit suffixes (e.g. min_uv/max_uv for microvolt ranges), which should improve readability and prevent confusion. Please let me know if any other identifiers need unit annotations.

This patch addresses various improvements based on reviewer suggestions: - Removed unused index variable in parse_dt() - Fixed misleading indentation in probe() conditional blocks - Reworked DT bindings: switched to enum/oneOf and restricted voltage/current ranges - Added driver-side range validation for voltage and current modes - Replaced manual bit macros with FIELD_PREP() for DAC_CONFIG - Removed unnecessary include: <linux/kernel.h> - Sorted entries in Kconfig and Makefile - Quoted DT required property names and simplified mutual-exclusion logic - Tidied up whitespace, blank lines, and long lines Signed-off-by: Bruce Tsao <bruce.tsao@analog.com>

dlech · 2025-06-16T16:35:24Z

Documentation/devicetree/bindings/iio/dac/adi,ad5413.yaml

+      - items:
+          - const: -10500000
+          - const: 10500000


Suggested change

- items:

- const: -10500000

- const: 10500000

- items:

- const: -10500000

- const: 10500000

- items:

- const: -12600000

- const: 12600000

This needs to include all possibilities.

dlech · 2025-06-16T16:36:04Z

Documentation/devicetree/bindings/iio/dac/adi,ad5413.yaml

+    oneOf:
+      - items:
+          - const: 0
+          - const: 24000


Suggested change

oneOf:

- items:

- const: 0

- const: 24000

items:

- const: 0

- const: 24000

oneOf doesn't make sense when there is only one possibility.

dlech · 2025-06-16T16:38:38Z

drivers/iio/dac/ad5413.c

+/* AD5413 DAC_CONFIG Register (Addr = 0x06) Bitfield Macros */
+/* Slew Rate Step (Bits[15:13]) */
+#define AD5413_REG_DAC_CONFIG_SR_STEP_MSK        GENMASK(15, 13)
+#define AD5413_REG_DAC_CONFIG_SR_STEP_MODE(x)    FIELD_PREP(AD5413_REG_DAC_CONFIG_SR_STEP_MSK, (x))


The IIO maintainer prefers to just use FIELD_PREP() directly instead of making macros to hide it.

dlech · 2025-06-16T16:46:04Z

drivers/iio/dac/ad5413.c

+
+
+/* AD5413 Key Register (Address: 0x08) */
+#define AD5413_REG_KEY_REG_ADDR_SHIFT     16


Suggested change

#define AD5413_REG_KEY_REG_ADDR_SHIFT 16

unused

dlech · 2025-06-16T16:48:03Z

drivers/iio/dac/ad5413.c

+
+
+
+/* AD5413 DAC_CONFIG Register (Addr = 0x06) Bitfield Macros */


Instead of comments like this, the IIO maintainer prefers to have masks like this right below the corresponding register address (with a few extra spaces indent).

I.e. above we would have...

... #define AD5413_REG_USER_OFFSET 0x05 #define AD5413_REG_DAC_CONFIG 0x06 #define AD5413_REG_DAC_CONFIG_SR_STEP_MSK GENMASK(15, 13) #define AD5413_REG_DAC_CONFIG_SR_CLOCK_MSK GENMASK(12, 9) #define AD5413_REG_DAC_CONFIG_SR_EN_MSK BIT(8) #define AD5413_REG_DAC_CONFIG_RSET_EXT_EN_MSK BIT(7) #define AD5413_REG_DAC_CONFIG_OUT_EN_MSK BIT(6) #define AD5413_REG_DAC_CONFIG_INT_EN_MSK BIT(5) #define AD5413_REG_DAC_CONFIG_OVRNG_EN_MSK BIT(4) #define AD5413_REG_DAC_CONFIG_RANGE_MSK GENMASK(3, 0) #define AD5413_REG_SW_LDAC 0x07 #define AD5413_REG_KEY 0x08 #define AD5413_REG_KEY_REG_ADDR_MASK GENMASK(20, 16) #define AD5413_REG_KEY_CODE_MASK GENMASK(15, 0) ...

dlech · 2025-06-16T17:23:00Z

drivers/iio/dac/ad5413.c

+	if (ret < 0)
+		return ret;
+
+	/* Disable CRC checks */


Suggested change

/* Disable CRC checks */

dlech · 2025-06-16T17:23:10Z

drivers/iio/dac/ad5413.c

+	if (ret < 0)
+		return ret;
+
+	/* Perform a calibration memory refresh */


Suggested change

/* Perform a calibration memory refresh */

drivers/iio/dac/ad5413.c

dlech · 2025-06-16T17:29:55Z

drivers/iio/dac/ad5413.c

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


Suggested change

mutex_init(&st->lock);

ret = devm_mutex_init(&st->lock);

if (ret)

return ret;

Not sure which kernel version this was introduced in, but is preferred in new code.

dlech · 2025-06-16T17:31:25Z

drivers/iio/dac/ad5413.c

+    if (st->out_range.reg == AD5413_RANGE_PLUSMINUS_10_5V) {
+        indio_dev->channels = ad5413_voltage_ch;
+	} else {
+        indio_dev->channels = ad5413_current_ch;
+	}


indentation needs fixing

BruceTsaoADI requested review from nunojsa, mhennerich, dbogdan, btogorean, chegbeli, buha, amiclaus, rbolboac, danmois, CiprianRegus and machschmitt as code owners May 18, 2025 17:35

gastmaier reviewed May 19, 2025

View reviewed changes

dlech reviewed May 22, 2025

View reviewed changes

dlech reviewed Jun 16, 2025

View reviewed changes

		description: \|
		Output digital slew control time in microseconds

	description: \|
	Output digital slew control time in microseconds
	description: Output digital slew control time in microseconds

	required: [adi, range-microamp]
	required: [ 'adi,range-microamp' ]

	required: [adi, range-microvolt]
	required: [ 'adi,range-microvolt' ]



		/* AD5413 Key Register (Address: 0x08) */
		#define AD5413_REG_KEY_REG_ADDR_SHIFT 16




		/* AD5413 DAC_CONFIG Register (Addr = 0x06) Bitfield Macros */

iio: dac: Add AD5413 support #2786

Are you sure you want to change the base?

iio: dac: Add AD5413 support #2786

Conversation

BruceTsaoADI commented May 18, 2025

Summary

Key changes

Datasheet:

PR Description

PR Type

PR Checklist

Uh oh!

gastmaier 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!

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!

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!

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!

Uh oh!