iio: dac: ad5504: fix voltage scaling and add missing bindings

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 <[email protected]>

Suggested-by Krzysztof Kozlowski <[email protected]>

Signed-off-by Taha Ed-Dafili <[email protected]>

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

Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml +23 -1

index 9c2c038683b4..c0b2d470949a 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,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
+

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

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 <[email protected]>

Suggested-by Krzysztof Kozlowski <[email protected]>

Signed-off-by Taha Ed-Dafili <[email protected]>

---
 .../bindings/iio/dac/adi,ad5504.yaml          | 40 ++++++++++++++++++-

 1 file changed, 39 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml +0 -0

index 9c2c038683b4..c0b2d470949a 100644
@@ -10,8 +10,10 @@ maintainers:

   - Lars-Peter Clausen <[email protected]>
   - Jonathan Cameron <[email protected]>

Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml +1 -1

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

Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml +0 -0

@@ -27,16 +29,48 @@ properties:

     maxItems: 1

   vcc-supply: true
+  vlogic-supply: true

Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml +6 -0

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

+    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

Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml +6 -0

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

Documentation/devicetree/bindings/iio/dac/adi,ad5504.yaml +4 -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>;
          };
      };
...

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

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 <[email protected]>

Quoted 5 lines · 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 <[email protected]>

Where was this patch suggested by Andy?

Best regards,
Krzysztof

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.

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.

drivers/iio/dac/ad5504.c +0 -0

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

drivers/iio/dac/ad5504.c +3 -6

 	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,

Quoted 1 line

#include <linux/device.h>

Is this still being used directly.

Quoted 1 line

#include <linux/kernel.h>

And what is this for?

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

asm/byteorder.h

Might be more.

Quoted 8 lines · 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.

...

drivers/iio/dac/ad5504.c static const struct iio_chan_spec ad5504_channels[] = { · line 281

Quoted 9 lines

 static int ad5504_probe(struct spi_device *spi)
 {

drivers/iio/dac/ad5504.c +2 -1

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

drivers/iio/dac/ad5504.c +1 -0

+	u32 range[2];

Preserve the reversed xmas tree order.

drivers/iio/dac/ad5504.c static const struct iio_chan_spec ad5504_channels[] = { · line 283

Quoted 2 lines

drivers/iio/dac/ad5504.c +1 -1

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

drivers/iio/dac/ad5504.c static const struct iio_chan_spec ad5504_channels[] = { · line 288

Quoted 12 lines

  if (!indio_dev)
  return -ENOMEM;

  st = iio_priv(indio_dev);

drivers/iio/dac/ad5504.c +2 -2

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

drivers/iio/dac/ad5504.c +0 -1

-	if (ret == -ENODEV) {

Why remove this condition?

This might break use of the driver on ACPI systems.

drivers/iio/dac/ad5504.c static const struct iio_chan_spec ad5504_channels[] = { · line 295

Quoted 11 lines

drivers/iio/dac/ad5504.c +4 -7

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

drivers/iio/dac/ad5504.c static const struct iio_chan_spec ad5504_channels[] = { · line 297

Quoted 2 lines

drivers/iio/dac/ad5504.c +2 -0

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

drivers/iio/dac/ad5504.c static const struct iio_chan_spec ad5504_channels[] = { · line 300

Quoted 3 lines

drivers/iio/dac/ad5504.c +2 -0

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

No, pdata should go.

Quoted 3 lines

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

...

Quoted 21 lines

  indio_dev->modes = INDIO_DIRECT_MODE;

  if (spi->irq) {

drivers/iio/dac/ad5504.c +6 -6

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

drivers/iio/dac/ad5504.c +1 -1

-	return devm_iio_device_register(&spi->dev, indio_dev);
+	return devm_iio_device_register(dev, indio_dev);

Quoted 1 line

 }

Unrelated changes.

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 <[email protected]>

Suggested-by Andy Shevchenko <[email protected]>

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 <[email protected]>

---
 drivers/iio/dac/ad5504.c | 14 ++++++++++++++

 1 file changed, 14 insertions(+)

drivers/iio/dac/ad5504.c +0 -0

index cd563460fc0a..58538f5263fc 100644
@@ -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>

drivers/iio/dac/ad5504.c +4 -0

@@ -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);
 };

drivers/iio/dac/ad5504.c +0 -0

@@ -299,6 +305,14 @@ static int ad5504_probe(struct spi_device *spi)
  	if (pdata && pdata->vref_mv)
  		st->vref_mv = pdata->vref_mv;