[PATCH v2 0/5] iio: dac: ad5504: fix voltage scaling and add missing bindings

[PATCH v2 0/5] iio: dac: ad5504: fix voltage scaling and add missing bindings

[Taha Ed-Dafili]

This series fixes the voltage scale calculation for the AD5504 DAC and
updates the devicetree bindings to support the necessary hardware
configuration.

Previously, the driver calculated the scale based on the VCC supply
voltage. However, the AD5504 has an integrated precision reference,
and the output range is determined strictly by the state of the
R_SEL pin (0-30V or 0-60V).

This series updates the bindings to use standard IIO DAC properties,
aligns the driver headers with the IWYU principle, and fixes the core
scaling bug while adding support for the hardware's optional GPIOs.

Changes in v2:
- Replaced vendor-specific adi,output-range-volts with the standard
  output-range-microvolt property array (Krzysztof).
- Replaced adi,output-range-gpios with standard range-sel-gpios
  (Krzysztof).
- Refactored allOf block in bindings to use not/required logic
  (Krzysztof).
- Split header updates into two patches: alphabetical sorting and IWYU
  compliance (Andy).
- Used MILLI and MICRO macros to replace hardcoded values (Andy).
- Used a local dev pointer to respect the 80-character limit and realigned
  function arguments accordingly (Andy).
- Added driver implementation for the optional CLR and LDAC
  GPIOs (new in v2)

Thank you to Jonathan Cameron, Krzysztof Kozlowski, Andy Shevchenko, and
David Lechner for the reviews, schema guidance, and C optimizations on v1.

Note: I do not have access to the physical AD5504 hardware, so this series
has been strictly compile-tested and validated against dt_binding_check
based on the datasheet specifications.

Taha Ed-Dafili (5):
  dt-bindings: iio: dac: ad5504: add output-range and missing gpios
  iio: dac: ad5504: sort headers alphabetically
  iio: dac: ad5504: Align headers with IWYU principle
  iio: dac: ad5504: fix scale via output-range-microvolt
  iio: dac: ad5504: add optional GPIO control for CLR and LDAC

 .../bindings/iio/dac/adi,ad5504.yaml          | 40 ++++++++++-
 drivers/iio/dac/ad5504.c                      | 70 ++++++++++++-------
 2 files changed, 84 insertions(+), 26 deletions(-)

-- 
2.47.3

[PATCH v2 1/5] 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, clear-gpios, ldac-gpios and datasheet links, and provide
a complete usage example.

Suggested-by: David Lechner <dlechner@baylibre.com>
Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
---
 .../bindings/iio/dac/adi,ad5504.yaml          | 40 ++++++++++++++++++-
 1 file changed, 39 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..c0b2d470949a 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,16 +29,48 @@ 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
+
+  clear-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
 
 required:
   - compatible
   - reg
+  - vcc-supply
+
+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 +79,10 @@ examples:
             compatible = "adi,ad5504";
             vcc-supply = <&dac_vcc>;
             interrupts = <55 IRQ_TYPE_EDGE_FALLING>;
+
+            output-range-microvolt = <0 60000000>;
+            clear-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
+            ldac-gpios = <&gpio 5 GPIO_ACTIVE_LOW>;
         };
     };
 ...
-- 
2.47.3

Re: [PATCH v2 1/5] dt-bindings: iio: dac: ad5504: add output-range and missing gpios

[Krzysztof Kozlowski]

On 10/03/2026 18:48, Taha Ed-Dafili wrote:
> 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, clear-gpios, ldac-gpios and datasheet links, and provide
> a complete usage example.
> 
> Suggested-by: David Lechner <dlechner@baylibre.com>
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>

Hm? Where did I suggest writing such patch?

Are you mixing terms here? I gave you review, no?

> Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
> ---
>  .../bindings/iio/dac/adi,ad5504.yaml          | 40 ++++++++++++++++++-
>  1 file changed, 39 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..c0b2d470949a 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,16 +29,48 @@ 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
> +
> +  clear-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
>  
>  required:
>    - compatible
>    - reg
> +  - vcc-supply

This is technically ABI break and nothing in the commit msg explained
why you are doing this, why this was not required before etc. Changes
you make must come with proper background.

> +
> +allOf:
> +  - not:
> +      required:
> +        - range-sel-gpios
> +        - output-range-microvolt


Best regards,
Krzysztof

Re: [PATCH v2 1/5] dt-bindings: iio: dac: ad5504: add output-range and missing gpios

[Taha Ed-Dafili]

Hi Krzysztof,

I misunderstood how the Suggested-by tag is used. Since
Andy explicitly asked me to sort the headers, and the DT properties were
provided during the v1 review, I thought I should credit those direct
suggestions. My reasoning was simply that those specific patches and
changes wouldn't exist in this series without that feedback.

I now realize this tag is meant for original feature ideas, not standard
review feedback. I will drop the tags across the entire series in v3.

Regarding vcc-supply: I made it required because the driver currently
fails to probe without it. I understand now that this causes an ABI break,
so I will revert it back to optional in v3.

Thank you for the clarification.

Best regards,
Taha

Re: [PATCH v2 1/5] dt-bindings: iio: dac: ad5504: add output-range and missing gpios

[Jonathan Cameron]

On Wed, 11 Mar 2026 09:49:02 +0000
Taha Ed-Dafili <0rayn.dev@gmail.com> wrote:

> Hi Krzysztof,
> 
> I misunderstood how the Suggested-by tag is used. Since
> Andy explicitly asked me to sort the headers, and the DT properties were
> provided during the v1 review, I thought I should credit those direct
> suggestions. My reasoning was simply that those specific patches and
> changes wouldn't exist in this series without that feedback.
> 
> I now realize this tag is meant for original feature ideas, not standard
> review feedback. I will drop the tags across the entire series in v3.
> 
> Regarding vcc-supply: I made it required because the driver currently
> fails to probe without it. I understand now that this causes an ABI break,
> so I will revert it back to optional in v3.
Are you sure it fails to probe?

	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vcc");
	if (ret < 0 && ret != -ENODEV)
		return ret;
	if (ret == -ENODEV) {
		if (pdata->vref_mv)
			st->vref_mv = pdata->vref_mv;
		else
			dev_warn(&spi->dev, "reference voltage unspecified\n");
	} else {
		st->vref_mv = ret / 1000;
	}

If it's not provided we should get ret == -ENODEV then fall through
to the other paths to either get it or warn if it's not there.

Logically it is required, but we have been papering over the DT
binding not listing that for a while.

Jonathan


> 
> Thank you for the clarification.
> 
> Best regards,
> Taha

Re: [PATCH v2 1/5] dt-bindings: iio: dac: ad5504: add output-range and missing gpios

[Nuno Sá]

On Tue, 2026-03-10 at 17:48 +0000, Taha Ed-Dafili wrote:
> 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, clear-gpios, ldac-gpios and datasheet links, and provide
> a complete usage example.
> 
> Suggested-by: David Lechner <dlechner@baylibre.com>
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
> ---
>  .../bindings/iio/dac/adi,ad5504.yaml          | 40 ++++++++++++++++++-
>  1 file changed, 39 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..c0b2d470949a 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,16 +29,48 @@ 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
> +
> +  clear-gpios:

I would match the datasheet name. So clr.

- Nuno Sá
> +    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
>  
>  required:
>    - compatible
>    - reg
> +  - vcc-supply
> +
> +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 +79,10 @@ examples:
>              compatible = "adi,ad5504";
>              vcc-supply = <&dac_vcc>;
>              interrupts = <55 IRQ_TYPE_EDGE_FALLING>;
> +
> +            output-range-microvolt = <0 60000000>;
> +            clear-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
> +            ldac-gpios = <&gpio 5 GPIO_ACTIVE_LOW>;
>          };
>      };
>  ...

[PATCH v2 2/5] iio: dac: ad5504: sort headers alphabetically

[Taha Ed-Dafili]

Rearrange the include headers in alphabetical order to follow the
standard kernel coding style. This is a preparatory cleanup with
no functional changes.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
---
 drivers/iio/dac/ad5504.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
index 355bcb6a8ba0..20a36e4a389e 100644
--- a/drivers/iio/dac/ad5504.c
+++ b/drivers/iio/dac/ad5504.c
@@ -5,16 +5,16 @@
  * Copyright 2011 Analog Devices Inc.
  */
 
-#include <linux/interrupt.h>
-#include <linux/fs.h>
+#include <linux/bitops.h>
 #include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/interrupt.h>
 #include <linux/kernel.h>
-#include <linux/spi/spi.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/regulator/consumer.h>
-#include <linux/module.h>
-#include <linux/bitops.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-- 
2.47.3

Re: [PATCH v2 2/5] iio: dac: ad5504: sort headers alphabetically

[Andy Shevchenko]

On Tue, Mar 10, 2026 at 05:48:32PM +0000, Taha Ed-Dafili wrote:
> Rearrange the include headers in alphabetical order to follow the
> standard kernel coding style. This is a preparatory cleanup with
> no functional changes.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] iio: dac: ad5504: sort headers alphabetically

[Krzysztof Kozlowski]

On 10/03/2026 18:48, Taha Ed-Dafili wrote:
> Rearrange the include headers in alphabetical order to follow the
> standard kernel coding style. This is a preparatory cleanup with
> no functional changes.
> 
> Suggested-by: Andy Shevchenko <andy@kernel.org>

Where was this patch suggested by Andy?

Best regards,
Krzysztof

Re: [PATCH v2 2/5] iio: dac: ad5504: sort headers alphabetically

[Jonathan Cameron]

On Tue, 10 Mar 2026 21:15:16 +0100
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 10/03/2026 18:48, Taha Ed-Dafili wrote:
> > Rearrange the include headers in alphabetical order to follow the
> > standard kernel coding style. This is a preparatory cleanup with
> > no functional changes.
> > 
> > Suggested-by: Andy Shevchenko <andy@kernel.org>  
> 
> Where was this patch suggested by Andy?
https://lore.kernel.org/all/aY40-0v9tbXC4tPY@smile.fi.intel.com/

It's one of Andy's favourite bits or 'whilst you are here
feedback' and was given for this specific driver.

> 
> Best regards,
> Krzysztof

[PATCH v2 3/5] iio: dac: ad5504: Align headers with IWYU principle

[Taha Ed-Dafili]

Update the header inclusions to follow the IWYU principle.

- Remove <linux/fs.h> and <linux/slab.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.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
---
 drivers/iio/dac/ad5504.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
index 20a36e4a389e..e71218c44982 100644
--- a/drivers/iio/dac/ad5504.c
+++ b/drivers/iio/dac/ad5504.c
@@ -5,16 +5,17 @@
  * Copyright 2011 Analog Devices Inc.
  */
 
-#include <linux/bitops.h>
+#include <linux/bits.h>
 #include <linux/device.h>
-#include <linux/fs.h>
+#include <linux/errno.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.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/iio.h>
 #include <linux/iio/sysfs.h>
-- 
2.47.3

Re: [PATCH v2 3/5] iio: dac: ad5504: Align headers with IWYU principle

[Andy Shevchenko]

On Tue, Mar 10, 2026 at 05:48:33PM +0000, Taha Ed-Dafili wrote:
> Update the header inclusions to follow the IWYU principle.
> 
> - Remove <linux/fs.h> and <linux/slab.h> as they are unused. The driver
> relies on devm_ managed allocations, so slab is not required.

...

> -#include <linux/bitops.h>
> +#include <linux/bits.h>

>  #include <linux/device.h>

Is this still being used directly.

> -#include <linux/fs.h>
> +#include <linux/errno.h>
>  #include <linux/interrupt.h>

>  #include <linux/kernel.h>

And what is this for?

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

The rest of the changes looks correct, but still missing headers:
linux/kstrtox.h

asm/byteorder.h

Might be more.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 4/5] iio: dac: ad5504: fix scale via output-range-microvolt

[Taha Ed-Dafili]

The AD5504 full-scale range is hardware-determined by the
R_SEL pin (0-30V or 0-60V). Previously, the driver incorrectly used the
VCC regulator voltage to calculate the scale.

Update the probe function to read the standard "output-range-microvolt"
property as a two-element array to determine the correct full-scale range.
Use the MILLI macro for clearer millivolt assignments and simplify the
probe logic using a local device pointer.

Suggested-by: David Lechner <dlechner@baylibre.com>
Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
---
 drivers/iio/dac/ad5504.c | 43 ++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
index e71218c44982..cd563460fc0a 100644
--- a/drivers/iio/dac/ad5504.c
+++ b/drivers/iio/dac/ad5504.c
@@ -12,10 +12,12 @@
 #include <linux/kernel.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
+#include <linux/units.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -271,28 +273,31 @@ static const struct iio_chan_spec ad5504_channels[] = {
 
 static int ad5504_probe(struct spi_device *spi)
 {
-	const struct ad5504_platform_data *pdata = dev_get_platdata(&spi->dev);
+	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;
+	u32 range[2];
 
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
 
 	st = iio_priv(indio_dev);
 
-	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vcc");
-	if (ret < 0 && ret != -ENODEV)
+	ret = devm_regulator_get_enable(dev, "vcc");
+	if (ret)
 		return ret;
-	if (ret == -ENODEV) {
-		if (pdata->vref_mv)
-			st->vref_mv = pdata->vref_mv;
-		else
-			dev_warn(&spi->dev, "reference voltage unspecified\n");
-	} else {
-		st->vref_mv = ret / 1000;
-	}
+
+	st->vref_mv = 60 * MILLI;
+	ret = device_property_read_u32_array(dev, "output-range-microvolt",
+					     range, 2);
+	if (!ret && range[1] == 30 * MICRO)
+		st->vref_mv = 30 * MILLI;
+
+	if (pdata && pdata->vref_mv)
+		st->vref_mv = pdata->vref_mv;
 
 	st->spi = spi;
 	indio_dev->name = spi_get_device_id(st->spi)->name;
@@ -305,17 +310,17 @@ static int ad5504_probe(struct spi_device *spi)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	if (spi->irq) {
-		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
-					   NULL,
-					   &ad5504_event_handler,
-					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-					   spi_get_device_id(st->spi)->name,
-					   indio_dev);
+		ret = devm_request_threaded_irq(dev, spi->irq,
+						NULL,
+						&ad5504_event_handler,
+						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+						spi_get_device_id(st->spi)->name,
+						indio_dev);
 		if (ret)
 			return ret;
 	}
 
-	return devm_iio_device_register(&spi->dev, indio_dev);
+	return devm_iio_device_register(dev, indio_dev);
 }
 
 static const struct spi_device_id ad5504_id[] = {
-- 
2.47.3

Re: [PATCH v2 4/5] iio: dac: ad5504: fix scale via output-range-microvolt

[Andy Shevchenko]

On Tue, Mar 10, 2026 at 05:48:34PM +0000, Taha Ed-Dafili wrote:
> The AD5504 full-scale range is hardware-determined by the
> R_SEL pin (0-30V or 0-60V). Previously, the driver incorrectly used the
> VCC regulator voltage to calculate the scale.
> 
> Update the probe function to read the standard "output-range-microvolt"
> property as a two-element array to determine the correct full-scale range.
> Use the MILLI macro for clearer millivolt assignments and simplify the
> probe logic using a local device pointer.

...

>  static int ad5504_probe(struct spi_device *spi)
>  {
> -	const struct ad5504_platform_data *pdata = dev_get_platdata(&spi->dev);
> +	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;
> +	u32 range[2];

Preserve the reversed xmas tree order.

> -	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));

Unrelated change. This should be split into another patch that makes use
of it here and there.

I have a déjà vu about these comments...

>  	if (!indio_dev)
>  		return -ENOMEM;
>  
>  	st = iio_priv(indio_dev);
>  
> -	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vcc");
> -	if (ret < 0 && ret != -ENODEV)
> +	ret = devm_regulator_get_enable(dev, "vcc");
> +	if (ret)
>  		return ret;

> -	if (ret == -ENODEV) {

Why remove this condition?

This might break use of the driver on ACPI systems.

> -		if (pdata->vref_mv)
> -			st->vref_mv = pdata->vref_mv;
> -		else
> -			dev_warn(&spi->dev, "reference voltage unspecified\n");
> -	} else {
> -		st->vref_mv = ret / 1000;
> -	}
> +
> +	st->vref_mv = 60 * MILLI;
> +	ret = device_property_read_u32_array(dev, "output-range-microvolt",
> +					     range, 2);

ARRAY_SIZE()
(will require array_size.h)

> +	if (!ret && range[1] == 30 * MICRO)
> +		st->vref_mv = 30 * MILLI;

This looks unusual and hard to follow. It also misses the validation
of the min of the range.

> +	if (pdata && pdata->vref_mv)
> +		st->vref_mv = pdata->vref_mv;

No, pdata should go.

>  
>  	st->spi = spi;
>  	indio_dev->name = spi_get_device_id(st->spi)->name;

...

>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	if (spi->irq) {
> -		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
> -					   NULL,
> -					   &ad5504_event_handler,
> -					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -					   spi_get_device_id(st->spi)->name,
> -					   indio_dev);
> +		ret = devm_request_threaded_irq(dev, spi->irq,
> +						NULL,
> +						&ad5504_event_handler,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +						spi_get_device_id(st->spi)->name,
> +						indio_dev);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	return devm_iio_device_register(&spi->dev, indio_dev);
> +	return devm_iio_device_register(dev, indio_dev);
>  }

Unrelated changes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/5] iio: dac: ad5504: Align headers with IWYU principle

[Taha Ed-Dafili]

Hi Andy,

Thank you for the detailed review across the series.

> >  #include <linux/device.h>
> Is this still being used directly.

Yes, it is still needed for `struct device` and `dev_get_platdata()`.

> >  #include <linux/kernel.h>
> And what is this for?

You are right, this appears to be a leftover. I will double-check its
usage and drop it in v3 if it's no longer needed.

> The rest of the changes looks correct, but still missing headers:
> linux/kstrtox.h
> 
> asm/byteorder.h
> 
> Might be more.

Good catch on these. I will add <asm/byteorder.h> (for cpu_to_be16()) and
<linux/kstrtox.h>. I will also do another general pass over the includes
to better align with IWYU principles before sending v3.

Regarding the probe function (Patch 4):

I will split the `struct device *dev = &spi->dev;` refactoring into its
own preparatory patch. For the scale calculation, I will restore the
`-ENODEV` check to maintain ACPI compatibility, use `ARRAY_SIZE()`
(adding <linux/array_size.h>), validate the range array indices properly,
and drop the legacy `pdata` fallback.

I will include all of these fixes in v3.

Best regards,
Taha

Re: [PATCH v2 4/5] iio: dac: ad5504: fix scale via output-range-microvolt

[Nuno Sá]

On Tue, 2026-03-10 at 21:16 +0200, Andy Shevchenko wrote:
> On Tue, Mar 10, 2026 at 05:48:34PM +0000, Taha Ed-Dafili wrote:
> > The AD5504 full-scale range is hardware-determined by the
> > R_SEL pin (0-30V or 0-60V). Previously, the driver incorrectly used the
> > VCC regulator voltage to calculate the scale.
> > 
> > Update the probe function to read the standard "output-range-microvolt"
> > property as a two-element array to determine the correct full-scale range.
> > Use the MILLI macro for clearer millivolt assignments and simplify the
> > probe logic using a local device pointer.
> 
> ...
> 
> >  static int ad5504_probe(struct spi_device *spi)
> >  {
> > -	const struct ad5504_platform_data *pdata = dev_get_platdata(&spi->dev);
> > +	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;
> > +	u32 range[2];
> 
> Preserve the reversed xmas tree order.
> 
> > -	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> 
> Unrelated change. This should be split into another patch that makes use
> of it here and there.
> 
> I have a déjà vu about these comments...
> 
> >  	if (!indio_dev)
> >  		return -ENOMEM;
> >  
> >  	st = iio_priv(indio_dev);
> >  
> > -	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vcc");
> > -	if (ret < 0 && ret != -ENODEV)
> > +	ret = devm_regulator_get_enable(dev, "vcc");
> > +	if (ret)
> >  		return ret;
> 
> > -	if (ret == -ENODEV) {
> 
> Why remove this condition?
> 
> This might break use of the driver on ACPI systems.
> 
> > -		if (pdata->vref_mv)
> > -			st->vref_mv = pdata->vref_mv;
> > -		else
> > -			dev_warn(&spi->dev, "reference voltage unspecified\n");
> > -	} else {
> > -		st->vref_mv = ret / 1000;
> > -	}
> > +
> > +	st->vref_mv = 60 * MILLI;
> > +	ret = device_property_read_u32_array(dev, "output-range-microvolt",
> > +					     range, 2);
> 
> ARRAY_SIZE()
> (will require array_size.h)
> 
> > +	if (!ret && range[1] == 30 * MICRO)
> > +		st->vref_mv = 30 * MILLI;
> 
> This looks unusual and hard to follow. It also misses the validation
> of the min of the range.

And the max FWIW. If the property is given we should be strict about it and make
sure only valid inputs are given. 

- Nuno Sá
> 
> > +	if (pdata && pdata->vref_mv)
> > +		st->vref_mv = pdata->vref_mv;
> 
> No, pdata should go.
> 
> >  
> >  	st->spi = spi;
> >  	indio_dev->name = spi_get_device_id(st->spi)->name;
> 
> ...
> 
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  
> >  	if (spi->irq) {
> > -		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
> > -					   NULL,
> > -					   &ad5504_event_handler,
> > -					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > -					   spi_get_device_id(st->spi)->name,
> > -					   indio_dev);
> > +		ret = devm_request_threaded_irq(dev, spi->irq,
> > +						NULL,
> > +						&ad5504_event_handler,
> > +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > +						spi_get_device_id(st->spi)->name,
> > +						indio_dev);
> >  		if (ret)
> >  			return ret;
> >  	}
> >  
> > -	return devm_iio_device_register(&spi->dev, indio_dev);
> > +	return devm_iio_device_register(dev, indio_dev);
> >  }
> 
> Unrelated changes.

Re: [PATCH v2 4/5] iio: dac: ad5504: fix scale via output-range-microvolt

[Krzysztof Kozlowski]

On 10/03/2026 18:48, Taha Ed-Dafili wrote:
> The AD5504 full-scale range is hardware-determined by the
> R_SEL pin (0-30V or 0-60V). Previously, the driver incorrectly used the
> VCC regulator voltage to calculate the scale.
> 
> Update the probe function to read the standard "output-range-microvolt"
> property as a two-element array to determine the correct full-scale range.
> Use the MILLI macro for clearer millivolt assignments and simplify the
> probe logic using a local device pointer.
> 
> Suggested-by: David Lechner <dlechner@baylibre.com>
> Suggested-by: Andy Shevchenko <andy@kernel.org>

Where were these suggestions given?

Best regards,
Krzysztof

[PATCH v2 5/5] iio: dac: ad5504: add optional GPIO control for CLR and LDAC

[Taha Ed-Dafili]

Add support for the optional 'clear-gpios' and 'ldac-gpios' properties
defined in the device tree bindings.

Use devm_gpiod_get_optional() with GPIOD_OUT_LOW to ensure the pins are
initialized to their inactive state.

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

diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
index cd563460fc0a..58538f5263fc 100644
--- a/drivers/iio/dac/ad5504.c
+++ b/drivers/iio/dac/ad5504.c
@@ -7,7 +7,9 @@
 
 #include <linux/bits.h>
 #include <linux/device.h>
+#include <linux/err.h>
 #include <linux/errno.h>
+#include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/mod_devicetable.h>
@@ -48,6 +50,8 @@
  * @vref_mv:		actual reference voltage used
  * @pwr_down_mask:	power down mask
  * @pwr_down_mode:	current power down mode
+ * @gpio_clear:	GPIO descriptor for the /CLR pin
+ * @gpio_ldac:		GPIO descriptor for the /LDAC pin
  * @data:		transfer buffer
  */
 struct ad5504_state {
@@ -56,6 +60,8 @@ struct ad5504_state {
 	unsigned short			vref_mv;
 	unsigned			pwr_down_mask;
 	unsigned			pwr_down_mode;
+	struct gpio_desc		*gpio_clear;
+	struct gpio_desc		*gpio_ldac;
 
 	__be16				data[2] __aligned(IIO_DMA_MINALIGN);
 };
@@ -299,6 +305,14 @@ static int ad5504_probe(struct spi_device *spi)
 	if (pdata && pdata->vref_mv)
 		st->vref_mv = pdata->vref_mv;
 
+	st->gpio_clear = devm_gpiod_get_optional(dev, "clear", GPIOD_OUT_LOW);
+	if (IS_ERR(st->gpio_clear))
+		return PTR_ERR(st->gpio_clear);
+
+	st->gpio_ldac = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_LOW);
+	if (IS_ERR(st->gpio_ldac))
+		return PTR_ERR(st->gpio_ldac);
+
 	st->spi = spi;
 	indio_dev->name = spi_get_device_id(st->spi)->name;
 	indio_dev->info = &ad5504_info;
-- 
2.47.3

Re: [PATCH v2 5/5] iio: dac: ad5504: add optional GPIO control for CLR and LDAC

[Nuno Sá]

On Tue, 2026-03-10 at 17:48 +0000, Taha Ed-Dafili wrote:
> Add support for the optional 'clear-gpios' and 'ldac-gpios' properties
> defined in the device tree bindings.
> 
> Use devm_gpiod_get_optional() with GPIOD_OUT_LOW to ensure the pins are
> initialized to their inactive state.
> 
> Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
> ---
>  drivers/iio/dac/ad5504.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
> index cd563460fc0a..58538f5263fc 100644
> --- a/drivers/iio/dac/ad5504.c
> +++ b/drivers/iio/dac/ad5504.c
> @@ -7,7 +7,9 @@
>  
>  #include <linux/bits.h>
>  #include <linux/device.h>
> +#include <linux/err.h>
>  #include <linux/errno.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/mod_devicetable.h>
> @@ -48,6 +50,8 @@
>   * @vref_mv:		actual reference voltage used
>   * @pwr_down_mask:	power down mask
>   * @pwr_down_mode:	current power down mode
> + * @gpio_clear:	GPIO descriptor for the /CLR pin
> + * @gpio_ldac:		GPIO descriptor for the /LDAC pin
>   * @data:		transfer buffer
>   */
>  struct ad5504_state {
> @@ -56,6 +60,8 @@ struct ad5504_state {
>  	unsigned short			vref_mv;
>  	unsigned			pwr_down_mask;
>  	unsigned			pwr_down_mode;
> +	struct gpio_desc		*gpio_clear;
> +	struct gpio_desc		*gpio_ldac;
>  
>  	__be16				data[2] __aligned(IIO_DMA_MINALIGN);
>  };
> @@ -299,6 +305,14 @@ static int ad5504_probe(struct spi_device *spi)
>  	if (pdata && pdata->vref_mv)
>  		st->vref_mv = pdata->vref_mv;
>  
> +	st->gpio_clear = devm_gpiod_get_optional(dev, "clear", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->gpio_clear))
> +		return PTR_ERR(st->gpio_clear);
> +
> +	st->gpio_ldac = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->gpio_ldac))
> +		return PTR_ERR(st->gpio_ldac);
> +

Fine having the above in bindings but as it stands now, not seeing the purpose of
having it in the driver. We're not handling these gpios at all.

- Nuno Sá
>