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

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

[Taha Ed-Dafili]

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

I see your point. My plan for v5 is to reorder: introduce the ACPI/DT
fork first, then remove pdata on top of it. That way the -ENODEV case
is already handled by the ACPI fork when pdata is removed. What do you
think of this approach?

Best Regards,
Taha Ed-dafili

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

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

[Taha Ed-Dafili]

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

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

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

[Andy Shevchenko]

On Wed, Aug 19, 2026 at 04:22:21AM -0400, Taha Ed-Dafili wrote:
> > This breaks the case with dummy regulators as far as I can see.
> > Has this been discussed in the previous rounds?
> 
> 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.

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

-- 
With Best Regards,
Andy Shevchenko