iio: dac: ad5504: bindings, cleanups, locking, and scale fixes

This v4 series continues the cleanup and scale-fix work for the AD5504 high-voltage DAC driver, addressing all outstanding feedback from the v3 review.

Jonathan Cameron applied patches 2 (sort headers alphabetically) and 4 (introduce local dev pointer) from v3 directly to iio/testing. This series rebases on top of that state and does not re-send those patches.

Changes in v4:

* Split the formerly monolithic v3 patch 5 into four focused commits:
pdata removal, locking, ACPI/DT separation, and output-range support. Each commit leaves the driver in a clean, bisectable state.
* Fix ACPI probe path by forking on ACPI_COMPANION() instead of relying on the -ENODEV sentinel from the regulator framework, following the pattern established in adc/ti-ads7950.c (David Lechner).
* Introduce a mutex via devm_mutex_init() and guard(mutex) to fix three concurrency issues: shared DMA SPI buffers, non-atomic CTRL+NOOP write sequences, and unprotected pwr_down_mask/pwr_down_mode state.
* Use device_property_present() to cleanly distinguish "property absent" from "property present but malformed" when parsing output-range-microvolt, replacing the ambiguous -EINVAL sentinel.
* Add backward compatibility fallback via devm_regulator_get_enable_read_voltage() for old DTs that predate the output-range-microvolt property.
* Replace <linux/device.h> with <linux/dev_printk.h> in IWYU patch (Jonathan Cameron).

Testing:
Verified on ARM64 Raspberry Pi 5 (BCM2712) using a custom Device Tree overlay on spi10, without physical AD5504 hardware.

60V path (output-range-microvolt = <0 60000000>):

$ cat /sys/bus/iio/devices/iio:device0/out_voltage_scale
14.648437500  (60000 mV / 4096)

30V path (output-range-microvolt = <0 30000000>):

$ cat /sys/bus/iio/devices/iio:device0/out_voltage_scale
7.324218750   (30000 mV / 4096)

Invalid range (output-range-microvolt = <0 70000000>):

[   40.646862] ad5504 spi10.0: error -EINVAL: Invalid output-range-microvolt
[   40.646869] ad5504 spi10.0: probe with driver ad5504 failed with error -22

Backward compatibility (no output-range-microvolt, vcc-supply = 60V):

$ cat /sys/bus/iio/devices/iio:device0/out_voltage_scale
14.648437500  (60000 mV / 4096 via regulator fallback)

Powerdown sysfs read/write and powerdown mode switching (20kohm_to_gnd, three_state) verified on all four channels without errors.

Previous versions:
v3: https://lore.kernel.org/linux-iio/[email protected]/
v2: https://lore.kernel.org/linux-iio/[email protected]/
v1: https://lore.kernel.org/linux-iio/[email protected]/

Thank you to Jonathan Cameron, David Lechner, Andy Shevchenko, Nuno Sá, Krzysztof Kozlowski, and Conor Dooley for the continued reviews.

Taha Ed-Dafili (6):
dt-bindings: iio: dac: ad5504: add output-range and missing gpios
iio: dac: ad5504: Align headers with IWYU principle
iio: dac: ad5504: remove legacy platform data support
iio: dac: ad5504: introduce local lock to protect state and spi
transfers
iio: dac: ad5504: strictly separate ACPI and DT probe paths
iio: dac: ad5504: support scale via output-range-microvolt property

 .../bindings/iio/dac/adi,ad5504.yaml          | 39 +++++++++-
 drivers/iio/dac/ad5504.c                      | 76 ++++++++++++++-----
 include/linux/iio/dac/ad5504.h                | 15 ----
 3 files changed, 97 insertions(+), 33 deletions(-)
 delete mode 100644 include/linux/iio/dac/ad5504.h

2.55.0

The AD5504 output range (0-30V or 0-60V) is determined by the R_SEL pin.

Use standard output-range-microvolt and range-sel-gpios properties to describe the hardware configuration of the R_SEL pin. Ensure mutual exclusivity using the not/required logic. Additionally, add missing vlogic-supply, clr-gpios, ldac-gpios and datasheet links, and provide a complete usage example.

---
 .../bindings/iio/dac/adi,ad5504.yaml          | 39 ++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

index 9c2c038683b4..e0123dceaa33 100644
@@ -10,8 +10,10 @@ maintainers:
   - Lars-Peter Clausen <[email protected]>
   - Jonathan Cameron <[email protected]>
 
-description:
+description: |
   High voltage (up to 60V) DACs with temperature sensor alarm function
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad5504.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad5501.pdf
 
 properties:
   compatible:

@@ -27,6 +29,29 @@ properties:
     maxItems: 1
 
   vcc-supply: true
+  vlogic-supply: true
+
+  output-range-microvolt:
+    description: |
+      Specify the channel output full scale range. The R_SEL pin
+      determines if the range is 0-30V or 0-60V.
+    items:
+      - const: 0
+      - enum: [30000000, 60000000]
+    default: [0, 60000000]
+
+  range-sel-gpios:
+    description:
+      GPIO connected to the R_SEL pin to select the output voltage range.
+    maxItems: 1
+
+  clr-gpios:
+    description: GPIO that controls the /CLR pin (active low).
+    maxItems: 1
+
+  ldac-gpios:
+    description: GPIO that controls the /LDAC pin (active low).
+    maxItems: 1
 
 additionalProperties: false
 

@@ -34,9 +59,17 @@ required:
   - compatible
   - reg
 
+allOf:
+  - not:
+      required:
+        - range-sel-gpios
+        - output-range-microvolt
+
 examples:
   - |
     #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
     spi {
         #address-cells = <1>;
         #size-cells = <0>;

@@ -45,6 +78,10 @@ examples:
             compatible = "adi,ad5504";
             vcc-supply = <&dac_vcc>;
             interrupts = <55 IRQ_TYPE_EDGE_FALLING>;
+
+            output-range-microvolt = <0 60000000>;
+            clr-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
+            ldac-gpios = <&gpio 5 GPIO_ACTIVE_LOW>;
         };
     };
 ...

2.55.0

Update the header inclusions to follow the IWYU principle and ensure they are sorted alphabetically:

- Remove <linux/fs.h>, <linux/slab.h>, and <linux/kernel.h> as they are unused. The driver relies on devm_ managed allocations, so slab is not required.
- Replace <linux/bitops.h> with <linux/bits.h> as only the BIT() and GENMASK() macros are used.
- Add <linux/mod_devicetable.h> for struct spi_device_id.
- Add <linux/errno.h> and <linux/types.h> for error codes and data types.
- Add <asm/byteorder.h> for cpu_to_be16().
- Add <linux/array_size.h> for ARRAY_SIZE().
- Add <linux/kstrtox.h> for string to integer conversions.
- Replaced <linux/device.h> with <linux/dev_printk.h>

---
 drivers/iio/dac/ad5504.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

index ad1a74678cf2..689147cdaa14 100644
@@ -5,22 +5,26 @@
  * Copyright 2011 Analog Devices Inc.
  */
 
-#include <linux/bitops.h>
-#include <linux/device.h>
-#include <linux/fs.h>
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/dev_printk.h>
+#include <linux/errno.h>
 #include <linux/interrupt.h>
-#include <linux/kernel.h>
+#include <linux/kstrtox.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
-#include <linux/slab.h>
 #include <linux/spi/spi.h>
 #include <linux/sysfs.h>
+#include <linux/types.h>
 
 #include <linux/iio/dac/ad5504.h>
 #include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
+#include <asm/byteorder.h>
+
 #define AD5504_RES_MASK			GENMASK(11, 0)
 #define AD5504_CMD_READ			BIT(15)
 #define AD5504_CMD_WRITE		0

2.55.0

Not anymore, now spi.h is responsible for that.

With Best Regards,
Andy Shevchenko

Hello,

no, please rely on linux/spi/spi.h to provide that or use <linux/device-id/spi.h> if you insist.

Best regards
Uwe

The AD5504 driver contains fallback logic to use legacy pdata for configuration. Since modern systems configure hardware exclusively using device tree or ACPI, and there are no in tree users for it. It can be safely removed.

Drop the ad5504_platform_data structure, remove the fallback logic from the probe function, and delete the dedicated include/linux/iio/dac/ad5504.h header file.

---
 drivers/iio/dac/ad5504.c       | 14 +++-----------
 include/linux/iio/dac/ad5504.h | 15 ---------------
 2 files changed, 3 insertions(+), 26 deletions(-)
 delete mode 100644 include/linux/iio/dac/ad5504.h

index 689147cdaa14..55ce7e49e0e0 100644
@@ -18,7 +18,6 @@
 #include <linux/sysfs.h>
 #include <linux/types.h>
 
-#include <linux/iio/dac/ad5504.h>
 #include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>

@@ -275,7 +274,6 @@ static const struct iio_chan_spec ad5504_channels[] = {
 static int ad5504_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
-	const struct ad5504_platform_data *pdata = dev_get_platdata(dev);
 	struct iio_dev *indio_dev;
 	struct ad5504_state *st;
 	int ret;

@@ -287,16 +285,10 @@ static int ad5504_probe(struct spi_device *spi)
 	st = iio_priv(indio_dev);
 
 	ret = devm_regulator_get_enable_read_voltage(dev, "vcc");
-	if (ret < 0 && ret != -ENODEV)
+	if (ret < 0)
 		return ret;
-	if (ret == -ENODEV) {
-		if (pdata->vref_mv)
-			st->vref_mv = pdata->vref_mv;
-		else
-			dev_warn(dev, "reference voltage unspecified\n");
-	} else {
-		st->vref_mv = ret / 1000;
-	}
+
+	st->vref_mv = ret / 1000;
 
 	st->spi = spi;
 	indio_dev->name = spi_get_device_id(st->spi)->name;

deleted file mode 100644
index 9f23c90486ee..000000000000
@@ -1,15 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * AD5504 SPI DAC driver
- *
- * Copyright 2011 Analog Devices Inc.
- */
-
-#ifndef SPI_AD5504_H_
-#define SPI_AD5504_H_
-
-struct ad5504_platform_data {
-	u16				vref_mv;
-};
-
-#endif /* SPI_AD5504_H_ */

2.55.0

This breaks the case with dummy regulators as far as I can see. Has this been discussed in the previous rounds?

...

With Best Regards,
Andy Shevchenko

Yes, David Lechner suggested in v3 to follow the pattern in adc/ti-ads7950.c. Patch 3 is an intermediate state. Patch 5 replaces this block with ACPI_COMPANION() fork.

Best Regards,
Taha Ed-dafili

But this is still a runtime bisectability issue.
Not critical most likely, but still...

With Best Regards,
Andy Shevchenko

The AD5504 driver currently lacks locking, exposing it to several multi-threading race conditions:
1. The shared DMA-safe SPI transfer buffers (st->data) can be corrupted if multiple threads trigger read_raw or write_raw simultaneously.
2. The ad5504_write_dac_powerdown() routine executes a sequence of back-to-back SPI writes (a CTRL register update followed by a mandatory NOOP). This entire sequence must be atomic.
3. Internal state variables like pwr_down_mask and pwr_down_mode can be read and modified concurrently.

Introduce a mutex in the ad5504_state structure and initialize it via devm_mutex_init() in probe. Use the modern scoped guard(mutex) macro at the top-level public IIO callbacks (read_raw, write_raw, and the powerdown attributes) to safely serialize access to the device state and the SPI bus.

In ad5504_read_raw() and ad5504_write_raw(), guard(mutex) is scoped to the IIO_CHAN_INFO_RAW case only, since IIO_CHAN_INFO_SCALE merely reads vref_mv, which is fixed at probe time and never modified afterward and therefore needs no serialization. Because guard(mutex) declares a cleanup-scoped variable, it cannot appear directly after a case label; wrap the case body in a compound statement (case IIO_CHAN_INFO_RAW: { ... }) to give it the block scope it requires.

---
Note: The concurrency race conditions addressed in this patch were originally reported by the Sashiko bot:
https://sashiko.dev/#/patchset/[email protected]

 drivers/iio/dac/ad5504.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

index 55ce7e49e0e0..87946cdf69e7 100644
@@ -7,12 +7,14 @@
 
 #include <linux/array_size.h>
 #include <linux/bits.h>
+#include <linux/cleanup.h>
 #include <linux/dev_printk.h>
 #include <linux/errno.h>
 #include <linux/interrupt.h>
 #include <linux/kstrtox.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 #include <linux/sysfs.h>

@@ -49,10 +51,12 @@
  * @pwr_down_mask:	power down mask
  * @pwr_down_mode:	current power down mode
  * @data:		transfer buffer
+ * @lock:		lock to protect state and spi transfers
  */
 struct ad5504_state {
 	struct spi_device		*spi;
 	struct regulator		*reg;
+	struct mutex			lock;
 	unsigned short			vref_mv;
 	unsigned			pwr_down_mask;
 	unsigned			pwr_down_mode;

@@ -103,7 +107,8 @@ static int ad5504_read_raw(struct iio_dev *indio_dev,
 	int ret;
 
 	switch (m) {
-	case IIO_CHAN_INFO_RAW:
+	case IIO_CHAN_INFO_RAW: {
+		guard(mutex)(&st->lock);
 		ret = ad5504_spi_read(st, chan->address);
 		if (ret < 0)
 			return ret;

@@ -111,6 +116,7 @@ static int ad5504_read_raw(struct iio_dev *indio_dev,
 		*val = ret;
 
 		return IIO_VAL_INT;
+	}
 	case IIO_CHAN_INFO_SCALE:
 		*val = st->vref_mv;
 		*val2 = chan->scan_type.realbits;

@@ -128,11 +134,13 @@ static int ad5504_write_raw(struct iio_dev *indio_dev,
 	struct ad5504_state *st = iio_priv(indio_dev);
 
 	switch (mask) {
-	case IIO_CHAN_INFO_RAW:
+	case IIO_CHAN_INFO_RAW: {
+		guard(mutex)(&st->lock);
 		if (val >= (1 << chan->scan_type.realbits) || val < 0)
 			return -EINVAL;
 
 		return ad5504_spi_write(st, chan->address, val);
+	}
 	default:
 		return -EINVAL;
 	}

@@ -148,6 +156,7 @@ static int ad5504_get_powerdown_mode(struct iio_dev *indio_dev,
 {
 	struct ad5504_state *st = iio_priv(indio_dev);
 
+	guard(mutex)(&st->lock);
 	return st->pwr_down_mode;
 }
 

@@ -156,6 +165,7 @@ static int ad5504_set_powerdown_mode(struct iio_dev *indio_dev,
 {
 	struct ad5504_state *st = iio_priv(indio_dev);
 
+	guard(mutex)(&st->lock);
 	st->pwr_down_mode = mode;
 
 	return 0;

@@ -173,6 +183,7 @@ static ssize_t ad5504_read_dac_powerdown(struct iio_dev *indio_dev,
 {
 	struct ad5504_state *st = iio_priv(indio_dev);
 
+	guard(mutex)(&st->lock);
 	return sysfs_emit(buf, "%d\n",
 			  !(st->pwr_down_mask & (1 << chan->channel)));
 }

@@ -185,6 +196,7 @@ static ssize_t ad5504_write_dac_powerdown(struct iio_dev *indio_dev,
 	int ret;
 	struct ad5504_state *st = iio_priv(indio_dev);
 
+	guard(mutex)(&st->lock);
 	ret = kstrtobool(buf, &pwr_down);
 	if (ret)
 		return ret;

@@ -284,6 +296,10 @@ static int ad5504_probe(struct spi_device *spi)
 
 	st = iio_priv(indio_dev);
 
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
 	ret = devm_regulator_get_enable_read_voltage(dev, "vcc");
 	if (ret < 0)
 		return ret;

2.55.0

Was this message written with a help with AI?

...

...

...

And so on...

With Best Regards,
Andy Shevchenko

Yes, I used AI to help with clarity and kernel mailing list conventions. The technical content and decisions are mine, learned from reading other drivers and review feedback. I'll keep the messages more concise going forward.

Will fix all guard() call sites in v5.

Best Regards,
Taha Ed-Dafili

Refactor the ad5504_probe() function to explicitly separate the ACPI and Device Tree execution paths. Previously, the driver relied on a fragile -ENODEV return value check from the regulator framework to bypass the voltage check on ACPI platforms.

Following modern IIO subsystem design patterns (such as those found in adc/ti-ads7950.c), fork the logic using ACPI_COMPANION(). On ACPI systems, where dedicated voltage regulators are typically omitted from the firmware description, bypass the regulator subsystem entirely and initialize the reference voltage to the hardware default 60V scale via a new macro AD5504_VA_MV_ACPI_DEFAULT.

For Device Tree platforms, treat the VCC regulator as mandatory and wrap the allocation in dev_err_probe() to cleanly handle potential deferrals and error propagation.

---
 drivers/iio/dac/ad5504.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

index 87946cdf69e7..546420f9afc0 100644
@@ -19,6 +19,7 @@
 #include <linux/spi/spi.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
+#include <linux/units.h>
 
 #include <linux/iio/events.h>
 #include <linux/iio/iio.h>

@@ -26,6 +27,10 @@
 
 #include <asm/byteorder.h>
 
+/*
+ * In case of ACPI, we use the 60 V as default voltage reference.
+ */
+#define AD5504_VA_MV_ACPI_DEFAULT	(60 * MILLI)
 #define AD5504_RES_MASK			GENMASK(11, 0)
 #define AD5504_CMD_READ			BIT(15)
 #define AD5504_CMD_WRITE		0

@@ -300,11 +305,16 @@ static int ad5504_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = devm_regulator_get_enable_read_voltage(dev, "vcc");
-	if (ret < 0)
-		return ret;
+	if (ACPI_COMPANION(dev)) {
+		st->vref_mv = AD5504_VA_MV_ACPI_DEFAULT;
+	} else {
+		ret = devm_regulator_get_enable_read_voltage(dev, "vcc");
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "Failed to get vcc regulator\n");
 
-	st->vref_mv = ret / 1000;
+		st->vref_mv = ret / MILLI;
+	}
 
 	st->spi = spi;
 	indio_dev->name = spi_get_device_id(st->spi)->name;

2.55.0

...

...

With Best Regards,
Andy Shevchenko

VA stands for Analog supply Voltage as far as I know, following Analog Devices datasheet convention. Will rename to AD5504_VREF_ACPI_DEFAULT_mV to make it self-explanatory in v5.

Will switch to is_acpi_device_node(dev_fwnode(dev)) in v5 and add <linux/acpi.h> accordingly.

Will fix in v5.

Best Regards,
Taha Ed-Dafili

The AD5504/AD5501 output range (0-30V or 0-60V) is determined by the hardware R_SEL pin and is required to compute IIO_CHAN_INFO_SCALE correctly. Previously the driver derived this solely from the vcc regulator's configured voltage, which conflated the supply voltage with the DAC's actual output range and offered no way to express the range explicitly in firmware.

Add support for the standard 'output-range-microvolt' device property, validating that it specifies one of the two supported ranges (0-30V or 0-60V) and using it to set st->vref_mv directly. When this property is present, the vcc regulator is only enabled (not read) via devm_regulator_get_enable(), since the regulator's actual voltage is no longer the source of truth for the output range.

For backward compatibility with older device trees that predate this property, fall back to reading the vcc regulator's configured voltage via devm_regulator_get_enable_read_voltage() when 'output-range-microvolt' is absent.

Use device_property_present() to explicitly distinguish "property absent" from "property present but malformed", rather than relying on the -EINVAL return from device_property_read_u32_array() as an absence sentinel. That return code is ambiguous: it is also returned when the property exists but the parsed array size does not match the expected length, which would have silently and incorrectly routed a malformed property through the legacy regulator-voltage fallback instead of surfacing a clear validation error.

---
 drivers/iio/dac/ad5504.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

index 546420f9afc0..25c0744545f0 100644
@@ -15,6 +15,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 #include <linux/sysfs.h>

@@ -293,6 +294,7 @@ static int ad5504_probe(struct spi_device *spi)
 	struct device *dev = &spi->dev;
 	struct iio_dev *indio_dev;
 	struct ad5504_state *st;
+	u32 range[2];
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));

@@ -308,12 +310,30 @@ static int ad5504_probe(struct spi_device *spi)
 	if (ACPI_COMPANION(dev)) {
 		st->vref_mv = AD5504_VA_MV_ACPI_DEFAULT;
 	} else {
-		ret = devm_regulator_get_enable_read_voltage(dev, "vcc");
-		if (ret < 0)
-			return dev_err_probe(dev, ret,
-					     "Failed to get vcc regulator\n");
-
-		st->vref_mv = ret / MILLI;
+		if (device_property_present(dev, "output-range-microvolt")) {
+			ret = device_property_read_u32_array(dev, "output-range-microvolt",
+							     range, ARRAY_SIZE(range));
+			if (ret)
+				return dev_err_probe(dev, ret,
+							"Error parsing output-range-microvolt\n");
+
+			if (range[0] != 0 || (range[1] != 30 * MICRO && range[1] != 60 * MICRO))
+				return dev_err_probe(dev, -EINVAL,
+					"Invalid output-range-microvolt\n");
+
+			st->vref_mv = range[1] / MILLI;
+
+			ret = devm_regulator_get_enable(dev, "vcc");
+			if (ret < 0)
+				return dev_err_probe(dev, ret, "Failed to enable vcc regulator\n");
+		} else {
+			/* Backward compat: old DTs without output-range-microvolt */
+			ret = devm_regulator_get_enable_read_voltage(dev, "vcc");
+			if (ret < 0)
+				return dev_err_probe(dev, ret, "Failed to get vcc regulator\n");
+
+			st->vref_mv = ret / MILLI;
+		}
 	}
 
 	st->spi = spi;

2.55.0

We do not need a Dostoevsky novel in the commit message. Please, make it straight to the point. Don't blindly use AI, you should understand what the code is doing. Same comment to all commit messages in the series.

...

With Best Regards,
Andy Shevchenko

Understood, will trim all commit messages in v5. My workflow is to draft the messages myself then use AI to check clarity and kernel mailing list conventions. I'll make sure that doesn't result in over-explained prose going forward.

I started the code with a bare / 1000, then replaced it with MILLI thinking it was the more readable form. I wasn't aware of the (MICRO / MILLI) idiom for expressing unit conversions explicitly. Will fix in v5 and drop the intermediate change from patch 5.

Best Regards,
Taha Ed-dafili

...

Since you want to get a millivolts from microvolts the 1000 is semantically (MICRO / MILLI).

With Best Regards,
Andy Shevchenko

I took a look through and didn't have anything to add to Andy's as normal excellent review. As he said, the patch descriptions are too verbose. It can help to ask the AI to be terse in how it describes things
+ edit what comes out to drop any stuff that is too detailed.

Jonathan