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)

gastmaier

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

drivers/iio/dac/ad5413.c

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

drivers/iio/dac/ad5413.c

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

drivers/iio/dac/ad5413.c

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

drivers/iio/dac/ad5413.c

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

drivers/iio/dac/ad5413.c

gastmaier · 2025-07-04T12:31:19Z

Hi, sorry for the pr failure, I updated the checkout strategy to retry by deepening the fetch until the rebase succeeds.
(It is not mega clear to me why it was failing in the first place, the total depth should be 7 (ahead + behind), but rebase only succeeds with around 24 or so.)

please see CI output, checkpatch is failing for example

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

BruceTsaoADI · 2025-09-06T14:39:55Z

Hi @dlech, @mhennerich, @nunojsa,

All feedback so far has been addressed in the latest commits.
CI still shows some false positives (e.g. checkpatch whitespace), but local checks pass with --strict and no errors.

Could you kindly take a look and let me know if anything else is required for approval?

Thanks!

dlech

I think this is looking in pretty good shape now.

It looks like there are some defconfig changes that were made that should not be here that is causing CI to fail.

Also, there might be something wrong with the way you are running checkpatch.pl if it is passing for you event with --strict. There are clearly still some issues with spaces vs. tabs and there was one // comment that checkpatch should have caught.

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

dlech · 2025-09-15T15:04:40Z

drivers/iio/dac/ad5413.c

+		return PTR_ERR(st->gpio_reset);
+
+	/* Disable CRC checks */
+	ret = ad5413_crc_disable(st);


I think this could use some comments to explain why it is necessary to do it this way since it doesn't follow the usual pattern.

However, This would make more sense to me:

st->gpio_reset = devm_gpiod_get_optional(&st->spi->dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(st->gpio_reset)) return PTR_ERR(st->gpio_reset); if (st->gpio_reset) { /* Reset pin is wired - finish the hard reset. */ fsleep(100); gpiod_set_value(st->gpio_reset, 0); } else { /* No reset pin - best we can do is soft reset. */ /* * If chip was left with CRC enabled, the reset command won't work * so we need to disable it first. */ ret = ad5413_crc_disable(st); if (ret) return ret; /* TODO: replace this with regmap_bulk_write(). */ ret = ad5413_spi_reg_write(st, AD5413_REG_KEY, AD5413_REG_KEY_CODE_RESET_1); if (ret) return ret; ret = ad5413_spi_reg_write(st, AD5413_REG_KEY, AD5413_REG_KEY_CODE_RESET_2); if (ret) return ret; } fsleep(100);

If we can do a hard reset, we shouldn't need to bother disabling the CRC or do a soft reset.

drivers/iio/dac/ad5413.c

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

nunojsa

Here it goes my first round of review. Also since it's the first time I see you contributing here, the normal expectation (unless there's a very good reason not to) is for the driver to be first upstreamed before merging it in our tree.

nunojsa · 2025-09-22T14:03:15Z

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

+            dvdd-supply = <&dvdd>;
+            avss-supply = <&avss>;
+            adi,range-microamp = <0 24000>;
+            adi,slew-time-us = <125>;


The bindings patch should be one separate commit.

nunojsa · 2025-09-22T14:03:32Z

drivers/iio/Kconfig.adi

 	imply AD5360
 	imply AD5380
 	imply AD5421
+	imply AD5413


Also a separate commit. The last one typically...

nunojsa · 2025-09-22T14:04:43Z

drivers/iio/dac/ad5413.c

+	int max_uv;
+};
+
+/* struct ad5413_state - driver instance specific data */


This comment does not really add much. It could be removed...

Thanks for pointing this out. The redundant comment has been removed in the latest revision.

nunojsa · 2025-09-22T14:05:30Z

drivers/iio/dac/ad5413.c

+ * Output ranges corresponding to bits [3:0] from DAC_CONFIG register
+ * 0011: ±10.5 V voltage range
+ * 1001: 0 mA to 24 mA current range
+ */


Instead of the binary I would use the identifiers. That's how it's done typically

Thanks for catching that! The comment now references the symbolic identifiers (AD5413_RANGE_PLUSMINUS_10_5V and AD5413_RANGE_0MA_24MA) instead of the binary values, matching the typical convention in other IIO drivers.

nunojsa · 2025-09-22T14:07:54Z

drivers/iio/dac/ad5413.c

+	AD5413_RANGE_PLUSMINUS_10_5V,
+	-10500000,
+	10500000,
+} };


In the kernel we typically use designated initializer. So:

static const struct ad5413_range ad5413_voltage_range[] = { . reg = AD5413_RANGE_PLUSMINUS_10_5V, .min_uv = -10500000, .max_uv = 10500000, };

Ditto for the other places where this applies.

Thanks for the feedback!
The struct ad5413_range has been cleaned up and made unit-agnostic (.reg, .min, .max) so it can be shared between voltage and current modes.
The ad5413_read_raw() function has been updated accordingly to use these new fields for calculating the scale and offset.

nunojsa · 2025-09-22T14:37:37Z

drivers/iio/dac/ad5413.c

+	if (ret == -ENODATA || ret == -EINVAL) {
+		slew_time_us = 0;
+	} else if (ret < 0) {
+		dev_err(&st->spi->dev, "Failed to read adi,slew-time-us: %d\n",


dev_err_probe()

Updated this section to use dev_err_probe() properly — removed the explicit %d since the helper already prints the error code automatically. The function now directly returns the result of dev_err_probe() for cleaner error handling.

nunojsa · 2025-09-22T14:37:58Z

drivers/iio/dac/ad5413.c

+				 const struct ad5413_range *range,
+				 unsigned int size, int min, int max)
+{
+	int i;


Good point—updated the loop index to unsigned int to match the size parameter (while keeping min/max as signed since ranges can be negative).

nunojsa · 2025-09-22T14:38:56Z

drivers/iio/dac/ad5413.c

+	unsigned int mask;
+
+	mask = (AD5413_WR_FLAG_MSK(AD5413_REG_DIGITAL_DIAG_CONFIG) << 24) |
+	       AD5413_DIGITAL_DIAG_CRC_DISABLE_VAL;


The above could be a macro right? Also I would expect this to a normal ad5413_spi_reg_write() call

Updated this function to use the regular ad5413_spi_reg_write() helper instead of manually composing the SPI frame. Also introduced a macro (AD5413_CRC_DISABLE_CMD) to hold the constant for clarity, as suggested.

nunojsa · 2025-09-22T14:40:41Z

drivers/iio/dac/ad5413.c

+	IIO_VOLTAGE) };
+
+static const struct iio_chan_spec ad5413_current_ch[] = { AD5413_DAC_CHAN(
+	IIO_CURRENT) };


please fixup style

Fixed the channel array formatting to follow kernel style — each array element is now on its own line without breaking the macro call.

Am i missing something? It does not look fixed...

nunojsa · 2025-09-22T14:42:16Z

drivers/iio/dac/ad5413.c

+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&st->lock);
+		ret = ad5413_spi_reg_write(st, AD5413_REG_DAC_INPUT, val);
+		mutex_unlock(&st->lock);


be consistent. Use the cleanup logic everywhere. Here you might need scoped_guard()

Updated the critical sections to use guard(mutex) for consistency with the rest of the driver. This keeps the lock/unlock handling automatic without requiring <linux/cleanup.h>, and aligns with the existing style used elsewhere in the code.

Add device-tree schema for AD5413: - Supplies (avdd/dvdd/avss), SPI, optional reset GPIO. - Range properties with quoted keys when listed in arrays (e.g. 'adi,range-microamp'). - Example node. Signed-off-by: Bruce Tsao <bruce.tsao@analog.com>

Introduce AD5413 IIO DAC driver: - guard(mutex) for locking - dev_err_probe() on probe-path failures - Unit-agnostic range tables (.min/.max) with designated initializers - CRC disable/refactor via helpers and macros - Wire into drivers/iio/dac/Kconfig and Makefile Signed-off-by: Bruce Tsao <bruce.tsao@analog.com>

Make the meta option imply AD5413 so it is built when selecting all ADI IIO drivers. Signed-off-by: Bruce Tsao <bruce.tsao@analog.com>

BruceTsaoADI · 2025-10-10T10:30:54Z

Hi @nunojsa
I force-pushed the branch to split the change set into three commits as suggested:

dt-bindings for AD5413,
the AD5413 driver (+ drivers/iio/dac/ Kconfig/Makefile), and
drivers/iio/Kconfig.adi update to make IIO_ALL_ADI_DRIVERS imply AD5413.
Also incorporated the .c feedback (locking with guard(mutex), dev_err_probe() on probe-path errors, designated initializers for ranges, style cleanups).
When you have a chance, could you please take another look? Thank you for your help!

nunojsa

Hi Bruce,

I still see a lot of comments I've done not handled at all. Please address those or argue back why you think you should not change it.

Also, take care about over commenting your code. Save comments for non obvious things

nunojsa · 2025-10-20T06:42:13Z

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

+required:
+  - compatible
+  - reg
+  - spi-max-frequency


Is this really mandatory? Typically not...

nunojsa · 2025-10-20T06:46:27Z

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

+            adi,range-microamp = <0 24000>;
+            adi,slew-time-us = <125>;
+        };
+    };


Just about the commit message. Do not mention "bindings" in the git subject, that's implied :). Instead, something like
"dt-bindings: iio: dac: Document AD5413 properties"

nunojsa · 2025-10-20T10:38:12Z

drivers/iio/dac/ad5413.c

+static const struct of_device_id ad5413_of_match[] = { { .compatible =
+								 "adi,ad5413" },
+						       {} };
+MODULE_DEVICE_TABLE(of, ad5413_of_match);


still not addressed

nunojsa · 2025-10-20T10:38:24Z

drivers/iio/dac/ad5413.c

+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id ad5413_id[] = { { "ad5413", 0 }, {} };


nunojsa · 2025-10-20T10:38:54Z

drivers/iio/dac/ad5413.c

+	if (ret < 0)
+		return ret;
+
+	/* By output range, decide voltage or current channel */


the comment does not bring any value IMO

nunojsa · 2025-10-20T10:41:44Z

drivers/iio/dac/ad5413.c

+	if (st->gpio_reset) {
+		/* Hard reset via GPIO (active low) */
+		fsleep(100);
+		gpiod_set_value(st->gpio_reset, 0); /* assert reset */


nunojsa · 2025-10-20T10:43:09Z

drivers/iio/dac/ad5413.c

+	/* Finally, enable the voltage/current output */
+	return ad5413_spi_write_mask(
+		st, AD5413_REG_DAC_CONFIG, AD5413_REG_DAC_CONFIG_OUT_EN_MSK,
+		FIELD_PREP(AD5413_REG_DAC_CONFIG_OUT_EN_MSK, 1));


the lines breaks are not really ok. Also don´t comment every line of code. Comments are for non obvious stuff

nunojsa · 2025-10-20T10:55:37Z

drivers/iio/dac/ad5413.c

+
+	/* 1) voltage or current range */
+	ret = device_property_read_u32_array(
+		&st->spi->dev, "adi,range-microvolt", tmparray, 2);


line breaking also nok.

nunojsa · 2025-10-20T10:55:56Z

drivers/iio/dac/ad5413.c

+	} else {
+		/* current path */
+		ret = device_property_read_u32_array(
+			&st->spi->dev, "adi,range-microamp", tmparray, 2);


line breaking again

nunojsa · 2025-10-20T10:56:36Z

drivers/iio/dac/ad5413.c

+	IIO_VOLTAGE) };
+
+static const struct iio_chan_spec ad5413_current_ch[] = { AD5413_DAC_CHAN(
+	IIO_CURRENT) };


Am i missing something? It does not look fixed...

nunojsa · 2025-10-20T11:00:35Z

Also, for the driver commit message, do not specify changes you've made on the PR. Since it's the first patch for this driver just add some information about the HW itself. Typically I use the datasheet for this. Every this some uncommon or that you feel that really stands out in the code, then mention it.

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

gastmaier reviewed May 19, 2025

View reviewed changes

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

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

dlech reviewed May 22, 2025

View reviewed changes

dlech reviewed Jun 16, 2025

View reviewed changes

BruceTsaoADI force-pushed the add-ad5413-driver branch 3 times, most recently from ba5d87d to 1401608 Compare June 29, 2025 05:51

dlech reviewed Jul 1, 2025

View reviewed changes

dlech reviewed Jul 4, 2025

View reviewed changes

Documentation/devicetree/bindings/iio/dac/adi,ad5413.yaml Outdated Show resolved Hide resolved

BruceTsaoADI force-pushed the add-ad5413-driver branch 9 times, most recently from 15694b1 to 8174327 Compare September 14, 2025 09:41

BruceTsaoADI force-pushed the add-ad5413-driver branch from 8174327 to b224d69 Compare September 14, 2025 10:13

dlech reviewed Sep 15, 2025

View reviewed changes

BruceTsaoADI force-pushed the add-ad5413-driver branch 2 times, most recently from fb3697f to 8b0e6f5 Compare September 16, 2025 11:58

dlech reviewed Sep 16, 2025

View reviewed changes

Documentation/devicetree/bindings/iio/dac/adi,ad5413.yaml Outdated Show resolved Hide resolved

BruceTsaoADI force-pushed the add-ad5413-driver branch 4 times, most recently from 5bfd6a8 to 4e5b988 Compare September 17, 2025 07:07

nunojsa reviewed Sep 22, 2025

View reviewed changes

BruceTsaoADI added 3 commits October 10, 2025 17:57

dt-bindings: iio: dac: Add AD5413 bindings

d7200d9

Add device-tree schema for AD5413: - Supplies (avdd/dvdd/avss), SPI, optional reset GPIO. - Range properties with quoted keys when listed in arrays (e.g. 'adi,range-microamp'). - Example node. Signed-off-by: Bruce Tsao <bruce.tsao@analog.com>

iio: Kconfig.adi: Add AD5413 to IIO_ALL_ADI_DRIVERS

84479a5

Make the meta option imply AD5413 so it is built when selecting all ADI IIO drivers. Signed-off-by: Bruce Tsao <bruce.tsao@analog.com>

BruceTsaoADI force-pushed the add-ad5413-driver branch from 4e5b988 to 84479a5 Compare October 10, 2025 10:27

nunojsa reviewed Oct 20, 2025

View reviewed changes

iio: dac: Add AD5413 support #2786

Are you sure you want to change the base?

iio: dac: Add AD5413 support #2786

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gastmaier commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

BruceTsaoADI commented Sep 6, 2025

Uh oh!

dlech left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dlech Sep 15, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

BruceTsaoADI Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

gastmaier commented Jul 4, 2025 •

edited

Loading

dlech Sep 15, 2025 •

edited

Loading

BruceTsaoADI Oct 10, 2025 •

edited

Loading

BruceTsaoADI commented Oct 10, 2025 •

edited

Loading