[PATCH v4 0/6] iio: dac: ad5504: bindings, cleanups, locking, and scale fixes

[PATCH v4 0/6] iio: dac: ad5504: bindings, cleanups, locking, and scale fixes

[Taha Ed-Dafili]

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/20260509142047.30302-1-0rayn.dev@gmail.com/
v2: https://lore.kernel.org/linux-iio/20260310174835.24209-1-0rayn.dev@gmail.com/
v1: https://lore.kernel.org/linux-iio/20260212181955.42724-1-0rayn.dev@gmail.com/

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

[PATCH v4 1/6] dt-bindings: iio: dac: ad5504: add output-range and missing gpios

[Taha Ed-Dafili]

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.

Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/iio/dac/adi,ad5504.yaml          | 39 ++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml
index 9c2c038683b4..e0123dceaa33 100644
--- a/Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml
@@ -10,8 +10,10 @@ maintainers:
   - Lars-Peter Clausen <lars@metafoo.de>
   - Jonathan Cameron <jic23@kernel.org>
 
-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

[PATCH v4 2/6] iio: dac: ad5504: Align headers with IWYU principle

[Taha Ed-Dafili]

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>

Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
---
 drivers/iio/dac/ad5504.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
index ad1a74678cf2..689147cdaa14 100644
--- a/drivers/iio/dac/ad5504.c
+++ b/drivers/iio/dac/ad5504.c
@@ -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

Re: [PATCH v4 2/6] iio: dac: ad5504: Align headers with IWYU principle

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] API Contract Violation (IWYU) and commit message mismatch: improper removal of `<linux/device.h>`.

Pre-existing issues:
- [High] Potential NULL pointer dereference of `pdata` during probe when the `vcc` regulator is missing.
- [High] Missing locking leads to race conditions and SPI DMA buffer corruption across concurrent sysfs accesses.
--

--- Patch [2]: [PATCH v4 2/6] iio: dac: ad5504: Align headers with IWYU principle ---
commit 90816be5685af9f86bca164af4343c7bef759b65
Author: Taha Ed-Dafili <0rayn.dev@gmail.com>

iio: dac: ad5504: Align headers with IWYU principle

This commit updates header inclusions in the ad5504 driver to align with
the Include What You Use (IWYU) principle. It removes several unused
headers, replaces others with more specific ones, and sorts them
alphabetically.

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

[Severity: Low]
This isn't a bug, but this description might be inaccurate since
<linux/device.h> is still required for dev_get_platdata().

> 
>     Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
> 
> ---
>  drivers/iio/dac/ad5504.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
> index 5e586185d8579..1793af0781dc7 100644
> --- a/drivers/iio/dac/ad5504.c
> +++ b/drivers/iio/dac/ad5504.c
> @@ -5,22 +5,26 @@
>   * Copyright 2011 Analog Devices Inc.
>   */
>  
> -#include <linux/bitops.h>
> -#include <linux/device.h>

[Severity: Low]
Does this removal violate the IWYU principle? The file directly calls
dev_get_platdata() and uses struct device in ad5504_probe(), which both
rely on <linux/device.h> being included.

[ ... ]

[Severity: High]
This is a pre-existing issue, but could this oops if the device is
instantiated via DeviceTree without a "vcc" regulator?

If dev_get_platdata() returns NULL and the regulator is missing,
devm_regulator_get_enable_read_voltage() returns -ENODEV, leading to:

drivers/iio/dac/ad5504.c:ad5504_probe() {
...
	if (ret == -ENODEV) {
		if (pdata->vref_mv)
			st->vref_mv = pdata->vref_mv;
...
}

Should we verify pdata is not NULL before dereferencing it here?

[Severity: High]
This is a pre-existing issue, but could concurrent sysfs accesses corrupt
the SPI DMA buffer or the powerdown mask?

If two threads concurrently access sysfs endpoints (like
out_voltageX_powerdown or in_voltageX_raw), they can enter
ad5504_spi_write() without any locking. Since st->data is shared among
all SPI transactions to satisfy DMA alignment, could concurrent writes
overwrite st->data and corrupt the SPI payload?

drivers/iio/dac/ad5504.c:ad5504_spi_write() {
...
	st->data[0] = cpu_to_be16(AD5504_CMD_WRITE | AD5504_ADDR(addr) |
			      (val & AD5504_RES_MASK));

	return spi_write(st->spi, &st->data[0], 2);
}

Similarly, st->pwr_down_mask appears to be subject to an unprotected
read-modify-write:

drivers/iio/dac/ad5504.c:ad5504_write_dac_powerdown() {
...
	if (pwr_down)
		st->pwr_down_mask &= ~(1 << chan->channel);
	else
		st->pwr_down_mask |= (1 << chan->channel);
...
}

Does this state need a mutex to prevent race conditions during concurrent
accesses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817211118.21833-1-0rayn.dev@gmail.com?part=2

Re: [PATCH v4 2/6] iio: dac: ad5504: Align headers with IWYU principle

[Andy Shevchenko]

On Mon, Aug 17, 2026 at 05:11:11PM -0400, Taha Ed-Dafili wrote:
> 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.

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

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/6] iio: dac: ad5504: Align headers with IWYU principle

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 248 bytes --]

Hello,

On Mon, Aug 17, 2026 at 05:11:11PM -0400, Taha Ed-Dafili wrote:
> - Add <linux/mod_devicetable.h> for struct spi_device_id.

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

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v4 3/6] iio: dac: ad5504: remove legacy platform data support

[Taha Ed-Dafili]

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.

Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
---
 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

diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
index 689147cdaa14..55ce7e49e0e0 100644
--- a/drivers/iio/dac/ad5504.c
+++ b/drivers/iio/dac/ad5504.c
@@ -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;
diff --git a/include/linux/iio/dac/ad5504.h b/include/linux/iio/dac/ad5504.h
deleted file mode 100644
index 9f23c90486ee..000000000000
--- a/include/linux/iio/dac/ad5504.h
+++ /dev/null
@@ -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

Re: [PATCH v4 3/6] iio: dac: ad5504: remove legacy platform data support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The driver deletes legacy platform data for configuring the output range but fails to implement Device Tree parsing to replace it, leading to incorrect IIO scale reporting.
- [High] Silent failure to probe if the optional 'vcc-supply' regulator is omitted from the device tree.

Pre-existing issues:
- [High] Missing mutex lock protecting shared SPI transfer buffers and device state during concurrent sysfs accesses.
--

--- Patch [3]: [PATCH v4 3/6] iio: dac: ad5504: remove legacy platform data support ---
commit e23f620a135f8935a12128aed6888332ecec66f8
Author: Taha Ed-Dafili <0rayn.dev@gmail.com>
iio: dac: ad5504: remove legacy platform data support

This commit removes legacy platform data support from the AD5504 driver,
deleting the ad5504_platform_data structure and the fallback configuration
logic in the probe function.

> diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
> index 1793af0781dc7..79db277b776e2 100644
> --- a/drivers/iio/dac/ad5504.c
> +++ b/drivers/iio/dac/ad5504.c
[ ... ]
> @@ -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;

[Severity: High]
Will this result in a silent probe failure if the optional vcc-supply
regulator is omitted from the device tree?

Since devm_regulator_get_enable_read_voltage() internally uses
devm_regulator_get_optional(), it returns -ENODEV if the regulator is
absent.

Propagating this error directly without logging causes the driver core to
treat it as a silent match rejection, whereas before it gracefully continued.

> -	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;

[Severity: High]
Does this missing implementation of device tree parsing lead to incorrect IIO
scale reporting?

The code currently hardcodes the reference voltage to the vcc supply
voltage without providing a fallback to parse the newly added
output-range-microvolt property.

If a system has a 60V VCC supply but the R_SEL pin is configured for a
30V output range, the device will now incorrectly report a 60V scale to
userspace since the fallback logic was removed.

[ ... ]

[Severity: High]
This is a pre-existing issue, but is it possible for concurrent sysfs
accesses to corrupt memory or disrupt hardware state?

Looking at ad5504_spi_write() and ad5504_spi_read(), the shared spi buffers
are accessed without locking:

drivers/iio/dac/ad5504.c:ad5504_spi_write() {
    ...
    st->data[0] = cpu_to_be16(AD5504_CMD_WRITE | AD5504_ADDR(addr) |
                              (val & AD5504_RES_MASK));

    return spi_write(st->spi, &st->data[0], 2);
}

Additionally, in ad5504_write_dac_powerdown(), multiple SPI writes are
performed and st->pwr_down_mask is modified without serialization:

drivers/iio/dac/ad5504.c:ad5504_write_dac_powerdown() {
    ...
    if (pwr_down)
        st->pwr_down_mask &= ~(1 << chan->channel);
    else
        st->pwr_down_mask |= (1 << chan->channel);
    
    ret = ad5504_spi_write(st, AD5504_ADDR_CTRL,
                           AD5504_DAC_PWRDWN_MODE(st->pwr_down_mode) |
                           AD5504_DAC_PWR(st->pwr_down_mask));
    
    /* writes to the CTRL register must be followed by a NOOP */
    ad5504_spi_write(st, AD5504_ADDR_NOOP, 0);
    ...
}

Could concurrent reads or writes by userspace processes to sysfs attributes
interleave SPI commands or corrupt the DMA buffer in st->data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817211118.21833-1-0rayn.dev@gmail.com?part=3

Re: [PATCH v4 3/6] iio: dac: ad5504: remove legacy platform data support

[Andy Shevchenko]

On Mon, Aug 17, 2026 at 05:11:12PM -0400, Taha Ed-Dafili wrote:
> 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.

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

...

> -	if (ret == -ENODEV) {

> -		else
> -			dev_warn(dev, "reference voltage unspecified\n");


This one.

...

In general it's a good change.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 4/6] iio: dac: ad5504: introduce local lock to protect state and spi transfers

[Taha Ed-Dafili]

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.

Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
---
Note: The concurrency race conditions addressed in this patch were 
originally reported by the Sashiko bot:
https://sashiko.dev/#/patchset/20260509142047.30302-1-0rayn.dev@gmail.com

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

diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
index 55ce7e49e0e0..87946cdf69e7 100644
--- a/drivers/iio/dac/ad5504.c
+++ b/drivers/iio/dac/ad5504.c
@@ -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

Re: [PATCH v4 4/6] iio: dac: ad5504: introduce local lock to protect state and spi transfers

[Andy Shevchenko]

On Mon, Aug 17, 2026 at 05:11:13PM -0400, Taha Ed-Dafili wrote:
> 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.

Was this message written with a help with AI?

...

> +	case IIO_CHAN_INFO_RAW: {
> +		guard(mutex)(&st->lock);

Always use blank line(s) to separate the guard()() from the rest of the code.

>  		if (val >= (1 << chan->scan_type.realbits) || val < 0)
>  			return -EINVAL;

...

>  	struct ad5504_state *st = iio_priv(indio_dev);
>  
> +	guard(mutex)(&st->lock);

This is even stronger as we also require the blank line before return.

>  	return st->pwr_down_mode;

...

And so on...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/6] iio: dac: ad5504: introduce local lock to protect state and spi transfers

[Taha Ed-Dafili]

> Was this message written with a help with AI?

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.

> Always use blank line(s) to separate the guard()() from the rest of
> the code.

Will fix all guard() call sites in v5.

Best Regards,
Taha Ed-Dafili

[PATCH v4 5/6] iio: dac: ad5504: strictly separate ACPI and DT probe paths

[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.

Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
---
 drivers/iio/dac/ad5504.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
index 87946cdf69e7..546420f9afc0 100644
--- a/drivers/iio/dac/ad5504.c
+++ b/drivers/iio/dac/ad5504.c
@@ -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

Re: [PATCH v4 5/6] iio: dac: ad5504: strictly separate ACPI and DT probe paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Bypassing the regulator framework on ACPI platforms breaks PRP0001-based configurations.
- [Low] Missing inclusion of `<linux/acpi.h>` for `ACPI_COMPANION`.
--

--- Patch [5]: [PATCH v4 5/6] iio: dac: ad5504: strictly separate ACPI and DT probe paths ---
commit 6ac93c8870295bc478f5b446654ca1771bdebc78
Author: Taha Ed-Dafili <0rayn.dev@gmail.com>

iio: dac: ad5504: strictly separate ACPI and DT probe paths

Refactor the ad5504_probe() function to explicitly separate the ACPI
and Device Tree execution paths.

> diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
> index 7b52ec95150de..e124e7804f3fb 100644
> --- a/drivers/iio/dac/ad5504.c
> +++ b/drivers/iio/dac/ad5504.c
> @@ -19,6 +19,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/sysfs.h>
>  #include <linux/types.h>
> +#include <linux/units.h>

[Severity: Low]
This isn't a bug, but should <linux/acpi.h> be explicitly included here to
satisfy the 'Include What You Use' (IWYU) principle for the ACPI_COMPANION
macro used below? Building under configurations where it is not transitively
included could lead to a build failure.

>  #include <linux/iio/events.h>
>  #include <linux/iio/iio.h>

[ ... ]

> @@ -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)) {

[Severity: Medium]
Does bypassing the regulator framework on ACPI platforms break PRP0001-based
configurations?

On ACPI systems that use standard PRP0001 (Device Tree bindings wrapped in
ACPI) to provide a vcc regulator, this unconditional branch skips enabling
the regulator and reading its voltage. The device might remain unpowered or
use an incorrect hardcoded reference voltage.

Before this patch, ad5504_probe() attempted to fetch the regulator and fell
back to defaults only if it returned -ENODEV. Should the regulator fetch
be preserved for ACPI platforms to avoid defeating the unified device
property frameworks?

> +		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;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817211118.21833-1-0rayn.dev@gmail.com?part=5

Re: [PATCH v4 5/6] iio: dac: ad5504: strictly separate ACPI and DT probe paths

[Andy Shevchenko]

On Mon, Aug 17, 2026 at 05:11:14PM -0400, Taha Ed-Dafili wrote:
> 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.

...

> +/*
> + * In case of ACPI, we use the 60 V as default voltage reference.
> + */
> +#define AD5504_VA_MV_ACPI_DEFAULT	(60 * MILLI)

Name it

#define AD5504_VA_ACPI_DEFAULT_mV	(60 * MILLI)

What does VA stand for?

...

> +	if (ACPI_COMPANION(dev)) {

It's better to use has_acpi_companion() or is_acpi_device_node().
I prefer to see the latter as that one unifies the style of checking
across the drivers and subsystems.

For that you will need to use dev_fwnode() from property.h and acpi.h
for the macro itself.

> +		st->vref_mv = AD5504_VA_MV_ACPI_DEFAULT;

...

> -	st->vref_mv = ret / 1000;
> +		st->vref_mv = ret / MILLI;

This should be  (MICRO / MILLI) instead of MILLI.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 6/6] iio: dac: ad5504: support scale via output-range-microvolt property

[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.

Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
---
 drivers/iio/dac/ad5504.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
index 546420f9afc0..25c0744545f0 100644
--- a/drivers/iio/dac/ad5504.c
+++ b/drivers/iio/dac/ad5504.c
@@ -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

Re: [PATCH v4 6/6] iio: dac: ad5504: support scale via output-range-microvolt property

[Andy Shevchenko]

On Mon, Aug 17, 2026 at 05:11:15PM -0400, Taha Ed-Dafili wrote:
> 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.

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.

...

> -		st->vref_mv = ret / MILLI;

So, the change from 1000 to MILLI is in the line that is changed again in this
patch. So, no need to change it earlier. And again, it should be (MICRO / MILLI).

> +			st->vref_mv = range[1] / MILLI;

> +			st->vref_mv = ret / MILLI;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 6/6] iio: dac: ad5504: support scale via output-range-microvolt property

[Taha Ed-Dafili]

On Mon, Aug 18, 2026 at 06:30:00AM +0300, Andy Shevchenko wrote:
> 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.

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.

> So, the change from 1000 to MILLI is in the line that is changed
> again in this patch. So, no need to change it earlier. And again, it
> should be (MICRO / MILLI).

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

Re: [PATCH v4 6/6] iio: dac: ad5504: support scale via output-range-microvolt property

[Andy Shevchenko]

On Tue, Aug 18, 2026 at 04:23:47AM -0400, Taha Ed-Dafili wrote:
> On Mon, Aug 18, 2026 at 06:30:00AM +0300, Andy Shevchenko wrote:

...

> > So, the change from 1000 to MILLI is in the line that is changed
> > again in this patch. So, no need to change it earlier. And again, it
> > should be (MICRO / MILLI).
> 
> 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.

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

-- 
With Best Regards,
Andy Shevchenko