[PATCH v3 0/3] Add driver for AD3530R and AD3531R DACs

[PATCH v3 0/3] Add driver for AD3530R and AD3531R DACs

[Kim Seer Paller]

The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
low-power, 16-bit, buffered voltage output DACs with software-
programmable gain controls, providing full-scale output spans of 2.5V or
5V for reference voltages of 2.5V. These devices operate from a single
2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
variants include a 2.5V, 5ppm/°C internal reference, which is disabled
by default.

The AD3531R/AD3531 is not yet released, so the only available datasheet
for now is the AD3530R/AD3530. The only differences between the two is
the number of channels, and register addresses of some registers.

Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
Changes in v3:
- Drop ABI docs.

Bindings:
- Drop reviewer's tag.
- Update commit message.
- Add non-r variants to compatible list.
- Add io-channels property to enable ADC channel support for MUXOUT
  readings.
- Switch to unevaluatedProperties: false.

ad3530r:
- Update commit message.
- Drop spi field from ad3530r_state and use regmap to retrieve the device
  pointer.
- Update mutex lock comment and use devm_mutex_init().
- Fix LDAC gpio pulse logic.
- Replace usleep_range() with fsleep().
- Use sizeof(reg_val) instead of hardcoded value in regmap_bulk_read.
- Drop reporting of zero offset.
- Add internal_ref_support chip_info parameter and modify reference
  handling.

- Link to v2: https://lore.kernel.org/r/20250324-togreg-v2-0-f211d781923e@analog.com

Changes in v2:
Bindings:
- Updated commit message.
- Changed adi,double-output-range to adi,range-double property.

ad3530r:
- Changed data type to __be16 to resolve sparse warnings related to
  type mismatches.

- Link to v1: https://lore.kernel.org/r/20250319-togreg-v1-0-d8244a502f2c@analog.com

---
Kim Seer Paller (3):
      iio: ABI: add new DAC powerdown mode
      dt-bindings: iio: dac: Add adi,ad3530r.yaml
      iio: dac: ad3530r: Add driver for AD3530R and AD3531R

 Documentation/ABI/testing/sysfs-bus-iio            |   2 +
 .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   |  99 ++++
 MAINTAINERS                                        |   8 +
 drivers/iio/dac/Kconfig                            |  11 +
 drivers/iio/dac/Makefile                           |   1 +
 drivers/iio/dac/ad3530r.c                          | 514 +++++++++++++++++++++
 6 files changed, 635 insertions(+)
---
base-commit: 8dbeb413806f9f810d97d25284f585b201aa3bdc
change-id: 20250319-togreg-fc6a0af961ed

Best regards,
-- 
Kim Seer Paller <kimseer.paller@analog.com>

[PATCH v3 1/3] iio: ABI: add new DAC powerdown mode

[Kim Seer Paller]

Add a new powerdown mode for DACs with 7.7kohm and 32kohm resistor
to GND.

Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 722aa989baac43f694076074b307d134867b4533..85790f943fd858021c75d67375abbd8b2976bb8b 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -740,7 +740,9 @@ Description:
 		1kohm_to_gnd: connected to ground via an 1kOhm resistor,
 		2.5kohm_to_gnd: connected to ground via a 2.5kOhm resistor,
 		6kohm_to_gnd: connected to ground via a 6kOhm resistor,
+		7.7kohm_to_gnd: connected to ground via a 7.7kOhm resistor,
 		20kohm_to_gnd: connected to ground via a 20kOhm resistor,
+		32kohm_to_gnd: connected to ground via a 32kOhm resistor,
 		42kohm_to_gnd: connected to ground via a 42kOhm resistor,
 		90kohm_to_gnd: connected to ground via a 90kOhm resistor,
 		100kohm_to_gnd: connected to ground via an 100kOhm resistor,

-- 
2.34.1

[PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[Kim Seer Paller]

Document the AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel)
low-power, 16-bit, buffered voltage output DACs with software-
programmable gain controls. They provide full-scale output spans of 2.5V
or 5V for reference voltages of 2.5V. These devices operate on a single
2.7V to 5.5V supply and are guaranteed to be monotonic by design.
The "R" variants include a 2.5V, 5ppm/°C internal reference, which is
disabled by default.

Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
 .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 99 ++++++++++++++++++++++
 MAINTAINERS                                        |  7 ++
 2 files changed, 106 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..cf4a3eb98f1fa30afdeb0740bba7f052d8ec2cd4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/dac/adi,ad3530r.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD3530R and Similar DACs
+
+maintainers:
+  - Kim Seer Paller <kimseer.paller@analog.com>
+
+description: |
+  The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are low-power,
+  16-bit, buffered voltage output digital-to-analog converters (DACs) with
+  software-programmable gain controls, providing full-scale output spans of 2.5V
+  or 5V for reference voltages of 2.5V. These devices operate from a single 2.7V
+  to 5.5V supply and are guaranteed monotonic by design. The "R" variants
+  include a 2.5V, 5ppm/°C internal reference, which is disabled by default.
+  Datasheet can be found here:
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad3530_ad530r.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad3530
+      - adi,ad3530r
+      - adi,ad3531
+      - adi,ad3531r
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 50000000
+
+  vdd-supply:
+    description: Power Supply Input.
+
+  iovdd-supply:
+    description: Digital Power Supply Input.
+
+  io-channels:
+    description:
+      ADC channel used to monitor internal die temperature, output voltages, and
+      current of a selected channel via the MUXOUT pin.
+    maxItems: 1
+
+  ref-supply:
+    description:
+      Reference Input/Output. The voltage at the REF pin sets the full-scale
+      range of all channels. If not provided the internal reference is used and
+      also provided on the VREF pin.
+
+  reset-gpios:
+    description:
+      Active low signal that is falling edge sensitive. When it is deasserted,
+      the digital core initialization is performed and all DAC registers except
+      the Interface Configuration A register are reset to their default values.
+    maxItems: 1
+
+  ldac-gpios:
+    description:
+      LDAC pin to be used as a hardware trigger to update the DAC channels. If
+      not present, the DAC channels are updated by Software LDAC.
+    maxItems: 1
+
+  adi,range-double:
+    description:
+      Configure the output range for all channels. If the property is present,
+      the output will range from 0V to 2Vref. If the property is not
+      present, the output will range from 0V to Vref.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - iovdd-supply
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        dac@0 {
+            compatible = "adi,ad3530r";
+            reg = <0>;
+            spi-max-frequency = <1000000>;
+
+            vdd-supply = <&vdd>;
+            iovdd-supply = <&iovdd>;
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index ffdb3f21fc4fb35b349449afbb30fecd4fe72978..9deaf2561ade5b1319cef3cb31b997a4297c0cff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1289,6 +1289,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
 F:	drivers/net/amt.c
 
+ANALOG DEVICES INC AD3530R DRIVER
+M:	Kim Seer Paller <kimseer.paller@analog.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
+
 ANALOG DEVICES INC AD3552R DRIVER
 M:	Nuno Sá <nuno.sa@analog.com>
 L:	linux-iio@vger.kernel.org

-- 
2.34.1

Re: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[Rob Herring (Arm)]

On Thu, 03 Apr 2025 13:33:56 +0800, Kim Seer Paller wrote:
> Document the AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel)
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls. They provide full-scale output spans of 2.5V
> or 5V for reference voltages of 2.5V. These devices operate on a single
> 2.7V to 5.5V supply and are guaranteed to be monotonic by design.
> The "R" variants include a 2.5V, 5ppm/°C internal reference, which is
> disabled by default.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---
>  .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 99 ++++++++++++++++++++++
>  MAINTAINERS                                        |  7 ++
>  2 files changed, 106 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.example.dtb: adc@0: pwm-names: ['convst1'] is too short
	from schema $id: http://devicetree.org/schemas/iio/adc/adi,ad7606.yaml#

doc reference errors (make refcheckdocs):
Warning: Documentation/arch/powerpc/cxl.rst references a file that doesn't exist: Documentation/ABI/testing/sysfs-class-cxl
Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
Warning: lib/Kconfig.debug references a file that doesn't exist: Documentation/dev-tools/fault-injection/fault-injection.rst
Documentation/arch/powerpc/cxl.rst: Documentation/ABI/testing/sysfs-class-cxl
MAINTAINERS: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
lib/Kconfig.debug: Documentation/dev-tools/fault-injection/fault-injection.rst

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250403-togreg-v3-2-d4b06a4af5a9@analog.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[Krzysztof Kozlowski]

On Thu, Apr 03, 2025 at 01:33:56PM +0800, Kim Seer Paller wrote:
> Document the AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel)
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls. They provide full-scale output spans of 2.5V
> or 5V for reference voltages of 2.5V. These devices operate on a single
> 2.7V to 5.5V supply and are guaranteed to be monotonic by design.
> The "R" variants include a 2.5V, 5ppm/°C internal reference, which is
> disabled by default.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---
>  .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 99 ++++++++++++++++++++++
>  MAINTAINERS                                        |  7 ++
>  2 files changed, 106 insertions(+)

You got report about error in v2, which you did not respond to.

You send v3... with same error. I don't understand this.

Best regards,
Krzysztof

RE: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[Paller, Kim Seer]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Thursday, April 3, 2025 2:44 PM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> Conor Dooley <conor+dt@kernel.org>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml
> 
> [External]
> 
> On Thu, Apr 03, 2025 at 01:33:56PM +0800, Kim Seer Paller wrote:
> > Document the AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-
> channel)
> > low-power, 16-bit, buffered voltage output DACs with software-
> > programmable gain controls. They provide full-scale output spans of
> > 2.5V or 5V for reference voltages of 2.5V. These devices operate on a
> > single 2.7V to 5.5V supply and are guaranteed to be monotonic by design.
> > The "R" variants include a 2.5V, 5ppm/°C internal reference, which is
> > disabled by default.
> >
> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> > ---
> >  .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 99
> ++++++++++++++++++++++
> >  MAINTAINERS                                        |  7 ++
> >  2 files changed, 106 insertions(+)
> 
> You got report about error in v2, which you did not respond to.
> 
> You send v3... with same error. I don't understand this.

I'm not sure, but based on the information provided, the errors might be
unrelated to the driver, or I might have missed something.

> 
> Best regards,
> Krzysztof

Re: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[Krzysztof Kozlowski]

On 03/04/2025 08:59, Paller, Kim Seer wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Thursday, April 3, 2025 2:44 PM
>> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
>> Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
>> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
>> Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
>> Conor Dooley <conor+dt@kernel.org>; linux-iio@vger.kernel.org; linux-
>> kernel@vger.kernel.org; devicetree@vger.kernel.org
>> Subject: Re: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml
>>
>> [External]
>>
>> On Thu, Apr 03, 2025 at 01:33:56PM +0800, Kim Seer Paller wrote:
>>> Document the AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-
>> channel)
>>> low-power, 16-bit, buffered voltage output DACs with software-
>>> programmable gain controls. They provide full-scale output spans of
>>> 2.5V or 5V for reference voltages of 2.5V. These devices operate on a
>>> single 2.7V to 5.5V supply and are guaranteed to be monotonic by design.
>>> The "R" variants include a 2.5V, 5ppm/°C internal reference, which is
>>> disabled by default.
>>>
>>> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
>>> ---
>>>  .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 99
>> ++++++++++++++++++++++
>>>  MAINTAINERS                                        |  7 ++
>>>  2 files changed, 106 insertions(+)
>>
>> You got report about error in v2, which you did not respond to.
>>
>> You send v3... with same error. I don't understand this.
> 
> I'm not sure, but based on the information provided, the errors might be
> unrelated to the driver, or I might have missed something.

It's your job to respond to errors one way or another (unless there are
obviously false positives). Ignoring them means this is marked as
"changes requested".

Best regards,
Krzysztof

RE: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[Paller, Kim Seer]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Thursday, April 3, 2025 3:02 PM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> Conor Dooley <conor+dt@kernel.org>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml
> 
> [External]
> 
> On 03/04/2025 08:59, Paller, Kim Seer wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzk@kernel.org>
> >> Sent: Thursday, April 3, 2025 2:44 PM
> >> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> >> Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> >> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> >> Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> >> Conor Dooley <conor+dt@kernel.org>; linux-iio@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; devicetree@vger.kernel.org
> >> Subject: Re: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml
> >>
> >> [External]
> >>
> >> On Thu, Apr 03, 2025 at 01:33:56PM +0800, Kim Seer Paller wrote:
> >>> Document the AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-
> >> channel)
> >>> low-power, 16-bit, buffered voltage output DACs with software-
> >>> programmable gain controls. They provide full-scale output spans of
> >>> 2.5V or 5V for reference voltages of 2.5V. These devices operate on a
> >>> single 2.7V to 5.5V supply and are guaranteed to be monotonic by design.
> >>> The "R" variants include a 2.5V, 5ppm/°C internal reference, which is
> >>> disabled by default.
> >>>
> >>> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> >>> ---
> >>>  .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 99
> >> ++++++++++++++++++++++
> >>>  MAINTAINERS                                        |  7 ++
> >>>  2 files changed, 106 insertions(+)
> >>
> >> You got report about error in v2, which you did not respond to.
> >>
> >> You send v3... with same error. I don't understand this.
> >
> > I'm not sure, but based on the information provided, the errors might be
> > unrelated to the driver, or I might have missed something.
> 
> It's your job to respond to errors one way or another (unless there are
> obviously false positives). Ignoring them means this is marked as
> "changes requested".

My bad, I'll take note of that. I think the bindings error reported in v3 is
still unrelated to the current changes.

> 
> Best regards,
> Krzysztof

Re: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[Krzysztof Kozlowski]

On 03/04/2025 09:11, Paller, Kim Seer wrote:
>>>>>
>>>>> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
>>>>> ---
>>>>>  .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 99
>>>> ++++++++++++++++++++++
>>>>>  MAINTAINERS                                        |  7 ++
>>>>>  2 files changed, 106 insertions(+)
>>>>
>>>> You got report about error in v2, which you did not respond to.
>>>>
>>>> You send v3... with same error. I don't understand this.
>>>
>>> I'm not sure, but based on the information provided, the errors might be
>>> unrelated to the driver, or I might have missed something.
>>
>> It's your job to respond to errors one way or another (unless there are
>> obviously false positives). Ignoring them means this is marked as
>> "changes requested".
> 
> My bad, I'll take note of that. I think the bindings error reported in v3 is
> still unrelated to the current changes.
Indeed, looks like. So in the future just respond to that email that you
checked it is not related or put some short note in the changelog of
that patch (---).

Best regards,
Krzysztof

Re: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[Krzysztof Kozlowski]

On 03/04/2025 07:33, Kim Seer Paller wrote:
> Document the AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel)
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls. They provide full-scale output spans of 2.5V
> or 5V for reference voltages of 2.5V. These devices operate on a single
> 2.7V to 5.5V supply and are guaranteed to be monotonic by design.
> The "R" variants include a 2.5V, 5ppm/°C internal reference, which is
> disabled by default.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[Nuno Sá]

Hi Kim,

On Thu, 2025-04-03 at 13:33 +0800, Kim Seer Paller wrote:
> Document the AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel)
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls. They provide full-scale output spans of 2.5V
> or 5V for reference voltages of 2.5V. These devices operate on a single
> 2.7V to 5.5V supply and are guaranteed to be monotonic by design.
> The "R" variants include a 2.5V, 5ppm/°C internal reference, which is
> disabled by default.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---
>  .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 99
> ++++++++++++++++++++++
>  MAINTAINERS                                        |  7 ++
>  2 files changed, 106 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
> b/Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..cf4a3eb98f1fa30afdeb0740bba7f052d8ec
> 2cd4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
> @@ -0,0 +1,99 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/dac/adi,ad3530r.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD3530R and Similar DACs
> +
> +maintainers:
> +  - Kim Seer Paller <kimseer.paller@analog.com>
> +
> +description: |
> +  The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are low-
> power,
> +  16-bit, buffered voltage output digital-to-analog converters (DACs) with
> +  software-programmable gain controls, providing full-scale output spans of
> 2.5V
> +  or 5V for reference voltages of 2.5V. These devices operate from a single
> 2.7V
> +  to 5.5V supply and are guaranteed monotonic by design. The "R" variants
> +  include a 2.5V, 5ppm/°C internal reference, which is disabled by default.
> +  Datasheet can be found here:
> + 
> https://www.analog.com/media/en/technical-documentation/data-sheets/ad3530_ad530r.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad3530
> +      - adi,ad3530r
> +      - adi,ad3531
> +      - adi,ad3531r
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 50000000
> +
> +  vdd-supply:
> +    description: Power Supply Input.
> +
> +  iovdd-supply:
> +    description: Digital Power Supply Input.
> +
> +  io-channels:
> +    description:
> +      ADC channel used to monitor internal die temperature, output voltages,
> and
> +      current of a selected channel via the MUXOUT pin.
> +    maxItems: 1
> 

I'm a bit puzzled... Isn't this device the provider of such a channel?
Therefore, I believe we should have #io-channel-cells, right?

- Nuno Sá

Re: [PATCH v3 2/3] dt-bindings: iio: dac: Add adi,ad3530r.yaml

[David Lechner]

On 4/3/25 6:15 AM, Nuno Sá wrote:
> 
> Hi Kim,
> 
> On Thu, 2025-04-03 at 13:33 +0800, Kim Seer Paller wrote:
>> Document the AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel)
>> low-power, 16-bit, buffered voltage output DACs with software-
>> programmable gain controls. They provide full-scale output spans of 2.5V
>> or 5V for reference voltages of 2.5V. These devices operate on a single
>> 2.7V to 5.5V supply and are guaranteed to be monotonic by design.
>> The "R" variants include a 2.5V, 5ppm/°C internal reference, which is
>> disabled by default.
>>
>> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
>> ---
>>  .../devicetree/bindings/iio/dac/adi,ad3530r.yaml   | 99
>> ++++++++++++++++++++++
>>  MAINTAINERS                                        |  7 ++
>>  2 files changed, 106 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
>> b/Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
>> new file mode 100644
>> index
>> 0000000000000000000000000000000000000000..cf4a3eb98f1fa30afdeb0740bba7f052d8ec
>> 2cd4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
>> @@ -0,0 +1,99 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/dac/adi,ad3530r.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Analog Devices AD3530R and Similar DACs
>> +
>> +maintainers:
>> +  - Kim Seer Paller <kimseer.paller@analog.com>
>> +
>> +description: |
>> +  The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are low-
>> power,
>> +  16-bit, buffered voltage output digital-to-analog converters (DACs) with
>> +  software-programmable gain controls, providing full-scale output spans of
>> 2.5V
>> +  or 5V for reference voltages of 2.5V. These devices operate from a single
>> 2.7V
>> +  to 5.5V supply and are guaranteed monotonic by design. The "R" variants
>> +  include a 2.5V, 5ppm/°C internal reference, which is disabled by default.
>> +  Datasheet can be found here:
>> + 
>> https://www.analog.com/media/en/technical-documentation/data-sheets/ad3530_ad530r.pdf
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - adi,ad3530
>> +      - adi,ad3530r
>> +      - adi,ad3531
>> +      - adi,ad3531r
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  spi-max-frequency:
>> +    maximum: 50000000
>> +
>> +  vdd-supply:
>> +    description: Power Supply Input.
>> +
>> +  iovdd-supply:
>> +    description: Digital Power Supply Input.
>> +
>> +  io-channels:
>> +    description:
>> +      ADC channel used to monitor internal die temperature, output voltages,
>> and
>> +      current of a selected channel via the MUXOUT pin.
>> +    maxItems: 1
>>
> 
> I'm a bit puzzled... Isn't this device the provider of such a channel?
> Therefore, I believe we should have #io-channel-cells, right?

Nope. :-)

The MUXOUT is an analog output pin, so this property is to read back from
an external ADC what the analog value of MUXOUT is. The DAC doesn't have an
internal ADC for measuring these values.

Comes from Jonathan's suggestion on a similar chip:
https://lore.kernel.org/linux-iio/20240413160610.4cec010b@jic23-huawei/

> 
> - Nuno Sá
> 
> 

[PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Kim Seer Paller]

The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
low-power, 16-bit, buffered voltage output DACs with software-
programmable gain controls, providing full-scale output spans of 2.5V or
5V for reference voltages of 2.5V. These devices operate from a single
2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
variants include a 2.5V, 5ppm/°C internal reference, which is disabled
by default.

Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
 MAINTAINERS               |   1 +
 drivers/iio/dac/Kconfig   |  11 +
 drivers/iio/dac/Makefile  |   1 +
 drivers/iio/dac/ad3530r.c | 514 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 527 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9deaf2561ade5b1319cef3cb31b997a4297c0cff..6e64525fadd4ab5fea20279ce6b5cd80ff4c749c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1295,6 +1295,7 @@ L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
+F:	drivers/iio/dac/ad3530r.c
 
 ANALOG DEVICES INC AD3552R DRIVER
 M:	Nuno Sá <nuno.sa@analog.com>
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 4811ea973125a0dea1f8a9cdee1e0c045bc21981..e0996dc014a3d538ab6b4e0d50ff54ede50f1527 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -6,6 +6,17 @@
 
 menu "Digital to analog converters"
 
+config AD3530R
+	tristate "Analog Devices AD3530R and Similar DACs driver"
+	depends on SPI
+	select REGMAP_SPI
+	help
+	  Say yes here to build support for Analog Devices AD3530R, AD3531R
+	  Digital to Analog Converter.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad3530r.
+
 config AD3552R_HS
 	tristate "Analog Devices AD3552R DAC High Speed driver"
 	select AD3552R_LIB
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 8dd6cce81ed1152be4cf0af9ef877b5482ceb347..3684cd52b7fa9bc0ad9f855323dcbb2e4965c404 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -4,6 +4,7 @@
 #
 
 # When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_AD3530R) += ad3530r.o
 obj-$(CONFIG_AD3552R_HS) += ad3552r-hs.o
 obj-$(CONFIG_AD3552R_LIB) += ad3552r-common.o
 obj-$(CONFIG_AD3552R) += ad3552r.o
diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
new file mode 100644
index 0000000000000000000000000000000000000000..4b757e19f0c8349999f72e53abb1a4f483a44eb2
--- /dev/null
+++ b/drivers/iio/dac/ad3530r.c
@@ -0,0 +1,514 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AD3530R/AD3530 8-channel, 16-bit Voltage Output DAC Driver
+ * AD3531R/AD3531 4-channel, 16-bit Voltage Output DAC Driver
+ *
+ * Copyright 2025 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#define AD3530R_INTERFACE_CONFIG_A		0x00
+#define AD3530R_OUTPUT_OPERATING_MODE_0		0x20
+#define AD3530R_OUTPUT_OPERATING_MODE_1		0x21
+#define AD3530R_OUTPUT_CONTROL_0		0x2A
+#define AD3530R_REFERENCE_CONTROL_0		0x3C
+#define AD3530R_SW_LDAC_TRIG_A			0xE5
+#define AD3530R_INPUT_CH(c)			(2 * (c) + 0xEB)
+
+#define AD3531R_SW_LDAC_TRIG_A			0xDD
+#define AD3531R_INPUT_CH(c)			(2 * (c) + 0xE3)
+
+#define AD3530R_SW_LDAC_TRIG_MASK		BIT(7)
+#define AD3530R_OUTPUT_CONTROL_MASK		BIT(2)
+#define AD3530R_REFERENCE_CONTROL_MASK		BIT(0)
+#define AD3530R_REG_VAL_MASK			GENMASK(15, 0)
+
+#define AD3530R_SW_RESET			(BIT(7) | BIT(0))
+#define AD3530R_MAX_CHANNELS			8
+#define AD3531R_MAX_CHANNELS			4
+#define AD3530R_CH(c)				(c)
+#define AD3530R_32KOHM_POWERDOWN_MODE		3
+#define AD3530R_INTERNAL_VREF_MV		2500
+#define AD3530R_LDAC_PULSE_US			100
+
+struct ad3530r_chan {
+	unsigned int powerdown_mode;
+	bool powerdown;
+};
+
+struct ad3530r_chip_info {
+	const char *name;
+	const struct iio_chan_spec *channels;
+	int (*input_ch_reg)(unsigned int c);
+	const int iio_chan;
+	unsigned int num_channels;
+	unsigned int sw_ldac_trig_reg;
+	bool internal_ref_support;
+};
+
+struct ad3530r_state {
+	struct regmap *regmap;
+	/* lock to protect against multiple access to the device and shared data */
+	struct mutex lock;
+	struct ad3530r_chan chan[AD3530R_MAX_CHANNELS];
+	const struct ad3530r_chip_info *chip_info;
+	struct gpio_desc *ldac_gpio;
+	int vref_mv;
+	u8 ldac;
+	bool range_multiplier;
+};
+
+static int ad3530r_input_ch_reg(unsigned int c)
+{
+	return AD3530R_INPUT_CH(c);
+}
+
+static int ad3531r_input_ch_reg(unsigned int c)
+{
+	return AD3531R_INPUT_CH(c);
+}
+
+static const char * const ad3530r_powerdown_modes[] = {
+	"1kohm_to_gnd",
+	"7.7kohm_to_gnd",
+	"32kohm_to_gnd",
+};
+
+static int ad3530r_get_powerdown_mode(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+
+	guard(mutex)(&st->lock);
+	return st->chan[chan->channel].powerdown_mode - 1;
+}
+
+static int ad3530r_set_powerdown_mode(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      unsigned int mode)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+
+	guard(mutex)(&st->lock);
+	st->chan[chan->channel].powerdown_mode = mode + 1;
+
+	return 0;
+}
+
+static const struct iio_enum ad3530r_powerdown_mode_enum = {
+	.items = ad3530r_powerdown_modes,
+	.num_items = ARRAY_SIZE(ad3530r_powerdown_modes),
+	.get = ad3530r_get_powerdown_mode,
+	.set = ad3530r_set_powerdown_mode,
+};
+
+static ssize_t ad3530r_get_dac_powerdown(struct iio_dev *indio_dev,
+					 uintptr_t private,
+					 const struct iio_chan_spec *chan,
+					 char *buf)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+
+	guard(mutex)(&st->lock);
+	return sysfs_emit(buf, "%d\n", st->chan[chan->channel].powerdown);
+}
+
+static ssize_t ad3530r_set_dac_powerdown(struct iio_dev *indio_dev,
+					 uintptr_t private,
+					 const struct iio_chan_spec *chan,
+					 const char *buf, size_t len)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+	int ret;
+	unsigned int mask, val;
+	bool powerdown;
+
+	ret = kstrtobool(buf, &powerdown);
+	if (ret)
+		return ret;
+
+	guard(mutex)(&st->lock);
+	switch (chan->channel) {
+	case AD3530R_CH(0) ... AD3530R_CH(AD3531R_MAX_CHANNELS - 1):
+		mask = GENMASK(chan->channel * 2 + 1, chan->channel * 2);
+		val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
+		      << (chan->channel * 2);
+
+		ret = regmap_update_bits(st->regmap,
+					 AD3530R_OUTPUT_OPERATING_MODE_0,
+					 mask, val);
+		if (ret)
+			return ret;
+
+		st->chan[chan->channel].powerdown = powerdown;
+		return len;
+	case AD3530R_CH(AD3531R_MAX_CHANNELS) ...
+	     AD3530R_CH(AD3530R_MAX_CHANNELS - 1):
+		mask = GENMASK((chan->channel - 4) * 2 + 1,
+			       (chan->channel - 4) * 2);
+		val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
+		      << ((chan->channel - 4) * 2);
+
+		ret = regmap_update_bits(st->regmap,
+					 AD3530R_OUTPUT_OPERATING_MODE_1,
+					 mask, val);
+		if (ret)
+			return ret;
+
+		st->chan[chan->channel].powerdown = powerdown;
+		return len;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad3530r_trigger_hw_ldac(struct gpio_desc *ldac_gpio)
+{
+	gpiod_set_value_cansleep(ldac_gpio, 1);
+	fsleep(AD3530R_LDAC_PULSE_US);
+	gpiod_set_value_cansleep(ldac_gpio, 0);
+
+	return 0;
+}
+
+static int ad3530r_dac_write(struct ad3530r_state *st, unsigned int chan,
+			     unsigned int val)
+{
+	int ret;
+	__be16 reg_val;
+
+	guard(mutex)(&st->lock);
+	reg_val = cpu_to_be16(val);
+
+	ret = regmap_bulk_write(st->regmap, st->chip_info->input_ch_reg(chan),
+				&reg_val, sizeof(reg_val));
+	if (ret)
+		return ret;
+
+	if (st->ldac_gpio)
+		return ad3530r_trigger_hw_ldac(st->ldac_gpio);
+
+	return regmap_update_bits(st->regmap, st->chip_info->sw_ldac_trig_reg,
+				  AD3530R_SW_LDAC_TRIG_MASK,
+				  FIELD_PREP(AD3530R_SW_LDAC_TRIG_MASK, 1));
+}
+
+static int ad3530r_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long info)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+	int ret;
+	__be16 reg_val;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_bulk_read(st->regmap,
+				       st->chip_info->input_ch_reg(chan->channel),
+				       &reg_val, sizeof(reg_val));
+		if (ret)
+			return ret;
+
+		*val = FIELD_GET(AD3530R_REG_VAL_MASK, be16_to_cpu(reg_val));
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->vref_mv;
+		*val2 = 16;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad3530r_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long info)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		if (val < 0 || val > U16_MAX)
+			return -EINVAL;
+
+		return ad3530r_dac_write(st, chan->channel, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad3530r_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			      unsigned int writeval, unsigned int *readval)
+{
+	struct ad3530r_state *st = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+
+	return regmap_write(st->regmap, reg, writeval);
+}
+
+#define AD3530R_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) {	\
+	.name = (_name),						\
+	.read = (_read),						\
+	.write = (_write),						\
+	.private = (_what),						\
+	.shared = (_shared),						\
+}
+
+static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
+	AD3530R_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
+			      ad3530r_get_dac_powerdown,
+			      ad3530r_set_dac_powerdown),
+	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad3530r_powerdown_mode_enum),
+	IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
+			   &ad3530r_powerdown_mode_enum),
+	{ },
+};
+
+#define AD3530R_CHAN(_chan) {						\
+	.type = IIO_VOLTAGE,						\
+	.indexed = 1,							\
+	.channel = _chan,						\
+	.output = 1,							\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
+			      BIT(IIO_CHAN_INFO_SCALE),			\
+	.ext_info = ad3530r_ext_info,					\
+}
+
+static const struct iio_chan_spec ad3530r_channels[] = {
+	AD3530R_CHAN(0),
+	AD3530R_CHAN(1),
+	AD3530R_CHAN(2),
+	AD3530R_CHAN(3),
+	AD3530R_CHAN(4),
+	AD3530R_CHAN(5),
+	AD3530R_CHAN(6),
+	AD3530R_CHAN(7),
+};
+
+static const struct iio_chan_spec ad3531r_channels[] = {
+	AD3530R_CHAN(0),
+	AD3530R_CHAN(1),
+	AD3530R_CHAN(2),
+	AD3530R_CHAN(3),
+};
+
+static const struct ad3530r_chip_info ad3530_chip = {
+	.name = "ad3530",
+	.channels = ad3530r_channels,
+	.num_channels = ARRAY_SIZE(ad3530r_channels),
+	.sw_ldac_trig_reg = AD3530R_SW_LDAC_TRIG_A,
+	.input_ch_reg = ad3530r_input_ch_reg,
+	.internal_ref_support = false,
+};
+
+static const struct ad3530r_chip_info ad3530r_chip = {
+	.name = "ad3530r",
+	.channels = ad3530r_channels,
+	.num_channels = ARRAY_SIZE(ad3530r_channels),
+	.sw_ldac_trig_reg = AD3530R_SW_LDAC_TRIG_A,
+	.input_ch_reg = ad3530r_input_ch_reg,
+	.internal_ref_support = true,
+};
+
+static const struct ad3530r_chip_info ad3531_chip = {
+	.name = "ad3531",
+	.channels = ad3531r_channels,
+	.num_channels = ARRAY_SIZE(ad3531r_channels),
+	.sw_ldac_trig_reg = AD3531R_SW_LDAC_TRIG_A,
+	.input_ch_reg = ad3531r_input_ch_reg,
+	.internal_ref_support = false,
+};
+
+static const struct ad3530r_chip_info ad3531r_chip = {
+	.name = "ad3531r",
+	.channels = ad3531r_channels,
+	.num_channels = ARRAY_SIZE(ad3531r_channels),
+	.sw_ldac_trig_reg = AD3531R_SW_LDAC_TRIG_A,
+	.input_ch_reg = ad3531r_input_ch_reg,
+	.internal_ref_support = true,
+};
+
+static int ad3530r_setup(struct ad3530r_state *st)
+{
+	const struct ad3530r_chip_info *chip_info = st->chip_info;
+	struct device *dev = regmap_get_device(st->regmap);
+	struct gpio_desc *reset_gpio;
+	int i, ret;
+
+	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(reset_gpio),
+				     "Failed to get reset GPIO\n");
+
+	if (reset_gpio) {
+		/* Perform hardware reset */
+		fsleep(1000);
+		gpiod_set_value_cansleep(reset_gpio, 0);
+	} else {
+		/* Perform software reset */
+		ret = regmap_update_bits(st->regmap, AD3530R_INTERFACE_CONFIG_A,
+					 AD3530R_SW_RESET, AD3530R_SW_RESET);
+		if (ret)
+			return ret;
+	}
+
+	fsleep(10000);
+
+	/* Set operating mode to normal operation. */
+	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0, 0);
+	if (ret)
+		return ret;
+
+	if (chip_info->num_channels > AD3531R_MAX_CHANNELS) {
+		ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_1, 0);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < chip_info->num_channels; i++)
+		st->chan[i].powerdown_mode = AD3530R_32KOHM_POWERDOWN_MODE;
+
+	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_HIGH);
+	if (IS_ERR(st->ldac_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
+				     "Failed to get ldac GPIO\n");
+
+	if (device_property_present(dev, "adi,double-output-range")) {
+		st->range_multiplier = true;
+
+		return regmap_update_bits(st->regmap, AD3530R_OUTPUT_CONTROL_0,
+					  AD3530R_OUTPUT_CONTROL_MASK,
+					  FIELD_PREP(AD3530R_OUTPUT_CONTROL_MASK, 1));
+	}
+
+	return 0;
+}
+
+static const struct regmap_config ad3530r_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+};
+
+static const struct iio_info ad3530r_info = {
+	.read_raw = ad3530r_read_raw,
+	.write_raw = ad3530r_write_raw,
+	.debugfs_reg_access = &ad3530r_reg_access,
+};
+
+static int ad3530r_probe(struct spi_device *spi)
+{
+	static const char * const regulators[] = { "vdd", "iovdd" };
+	const struct ad3530r_chip_info *chip_info;
+	struct device *dev = &spi->dev;
+	struct iio_dev *indio_dev;
+	struct ad3530r_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	st->regmap = devm_regmap_init_spi(spi, &ad3530r_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap),
+				     "Failed to init regmap");
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	chip_info = spi_get_device_match_data(spi);
+	if (!chip_info)
+		return -ENODEV;
+
+	st->chip_info = chip_info;
+
+	ret = ad3530r_setup(st);
+	if (ret)
+		return ret;
+
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
+					     regulators);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
+
+	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
+	if (ret < 0 && ret != -ENODEV)
+		return ret;
+
+	if (chip_info->internal_ref_support && ret == -ENODEV) {
+		/* Internal reference. */
+		ret = regmap_update_bits(st->regmap, AD3530R_REFERENCE_CONTROL_0,
+					 AD3530R_REFERENCE_CONTROL_MASK,
+					 FIELD_PREP(AD3530R_REFERENCE_CONTROL_MASK, 1));
+		if (ret)
+			return ret;
+
+		st->vref_mv = st->range_multiplier ?
+			      2 * AD3530R_INTERNAL_VREF_MV :
+			      AD3530R_INTERNAL_VREF_MV;
+	} else {
+		st->vref_mv = st->range_multiplier ? 2 * ret / 1000 : ret / 1000;
+	}
+
+	indio_dev->name = chip_info->name;
+	indio_dev->info = &ad3530r_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = chip_info->channels;
+	indio_dev->num_channels = chip_info->num_channels;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id ad3530r_id[] = {
+	{ "ad3530", (kernel_ulong_t)&ad3530_chip },
+	{ "ad3530r", (kernel_ulong_t)&ad3530r_chip },
+	{ "ad3531", (kernel_ulong_t)&ad3531_chip },
+	{ "ad3531r", (kernel_ulong_t)&ad3531r_chip },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad3530r_id);
+
+static const struct of_device_id ad3530r_of_match[] = {
+	{ .compatible = "adi,ad3530", .data = &ad3530_chip },
+	{ .compatible = "adi,ad3530r", .data = &ad3530r_chip },
+	{ .compatible = "adi,ad3531", .data = &ad3531_chip },
+	{ .compatible = "adi,ad3531r", .data = &ad3531r_chip },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad3530r_of_match);
+
+static struct spi_driver ad3530r_driver = {
+	.driver = {
+		.name = "ad3530r",
+		.of_match_table = ad3530r_of_match,
+	},
+	.probe = ad3530r_probe,
+	.id_table = ad3530r_id,
+};
+module_spi_driver(ad3530r_driver);
+
+MODULE_AUTHOR("Kim Seer Paller <kimseer.paller@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD3530R and Similar DACs Driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Nuno Sá]

On Thu, 2025-04-03 at 13:33 +0800, Kim Seer Paller wrote:
> The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls, providing full-scale output spans of 2.5V or
> 5V for reference voltages of 2.5V. These devices operate from a single
> 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> by default.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/iio/dac/Kconfig   |  11 +
>  drivers/iio/dac/Makefile  |   1 +
>  drivers/iio/dac/ad3530r.c | 514
> ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 527 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index
> 9deaf2561ade5b1319cef3cb31b997a4297c0cff..6e64525fadd4ab5fea20279ce6b5cd80ff4c
> 749c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1295,6 +1295,7 @@ L:	linux-iio@vger.kernel.org
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/dac/adi,ad3530r.yaml
> +F:	drivers/iio/dac/ad3530r.c
>  
>  ANALOG DEVICES INC AD3552R DRIVER
>  M:	Nuno Sá <nuno.sa@analog.com>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index
> 4811ea973125a0dea1f8a9cdee1e0c045bc21981..e0996dc014a3d538ab6b4e0d50ff54ede50f
> 1527 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -6,6 +6,17 @@
>  
>  menu "Digital to analog converters"
>  
> +config AD3530R
> +	tristate "Analog Devices AD3530R and Similar DACs driver"
> +	depends on SPI
> +	select REGMAP_SPI
> +	help
> +	  Say yes here to build support for Analog Devices AD3530R, AD3531R
> +	  Digital to Analog Converter.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad3530r.
> +
>  config AD3552R_HS
>  	tristate "Analog Devices AD3552R DAC High Speed driver"
>  	select AD3552R_LIB
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index
> 8dd6cce81ed1152be4cf0af9ef877b5482ceb347..3684cd52b7fa9bc0ad9f855323dcbb2e4965
> c404 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -4,6 +4,7 @@
>  #
>  
>  # When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_AD3530R) += ad3530r.o
>  obj-$(CONFIG_AD3552R_HS) += ad3552r-hs.o
>  obj-$(CONFIG_AD3552R_LIB) += ad3552r-common.o
>  obj-$(CONFIG_AD3552R) += ad3552r.o
> diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..4b757e19f0c8349999f72e53abb1a4f483a4
> 4eb2
> --- /dev/null
> +++ b/drivers/iio/dac/ad3530r.c
> @@ -0,0 +1,514 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AD3530R/AD3530 8-channel, 16-bit Voltage Output DAC Driver
> + * AD3531R/AD3531 4-channel, 16-bit Voltage Output DAC Driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#define AD3530R_INTERFACE_CONFIG_A		0x00
> +#define AD3530R_OUTPUT_OPERATING_MODE_0		0x20
> +#define AD3530R_OUTPUT_OPERATING_MODE_1		0x21
> +#define AD3530R_OUTPUT_CONTROL_0		0x2A
> +#define AD3530R_REFERENCE_CONTROL_0		0x3C
> +#define AD3530R_SW_LDAC_TRIG_A			0xE5
> +#define AD3530R_INPUT_CH(c)			(2 * (c) + 0xEB)
> +
> +#define AD3531R_SW_LDAC_TRIG_A			0xDD
> +#define AD3531R_INPUT_CH(c)			(2 * (c) + 0xE3)
> +
> +#define AD3530R_SW_LDAC_TRIG_MASK		BIT(7)
> +#define AD3530R_OUTPUT_CONTROL_MASK		BIT(2)
> +#define AD3530R_REFERENCE_CONTROL_MASK		BIT(0)
> +#define AD3530R_REG_VAL_MASK			GENMASK(15, 0)
> +
> +#define AD3530R_SW_RESET			(BIT(7) | BIT(0))
> +#define AD3530R_MAX_CHANNELS			8
> +#define AD3531R_MAX_CHANNELS			4
> +#define AD3530R_CH(c)				(c)
> +#define AD3530R_32KOHM_POWERDOWN_MODE		3
> +#define AD3530R_INTERNAL_VREF_MV		2500
> +#define AD3530R_LDAC_PULSE_US			100
> +
> +struct ad3530r_chan {
> +	unsigned int powerdown_mode;
> +	bool powerdown;
> +};
> +
> +struct ad3530r_chip_info {
> +	const char *name;
> +	const struct iio_chan_spec *channels;
> +	int (*input_ch_reg)(unsigned int c);
> +	const int iio_chan;
> +	unsigned int num_channels;
> +	unsigned int sw_ldac_trig_reg;
> +	bool internal_ref_support;
> +};
> +
> +struct ad3530r_state {
> +	struct regmap *regmap;
> +	/* lock to protect against multiple access to the device and shared
> data */
> +	struct mutex lock;
> +	struct ad3530r_chan chan[AD3530R_MAX_CHANNELS];
> +	const struct ad3530r_chip_info *chip_info;
> +	struct gpio_desc *ldac_gpio;
> +	int vref_mv;
> +	u8 ldac;
> +	bool range_multiplier;
> +};
> +
> +static int ad3530r_input_ch_reg(unsigned int c)
> +{
> +	return AD3530R_INPUT_CH(c);
> +}
> +
> +static int ad3531r_input_ch_reg(unsigned int c)
> +{
> +	return AD3531R_INPUT_CH(c);
> +}
> +
> +static const char * const ad3530r_powerdown_modes[] = {
> +	"1kohm_to_gnd",
> +	"7.7kohm_to_gnd",
> +	"32kohm_to_gnd",
> +};
> +
> +static int ad3530r_get_powerdown_mode(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +
> +	guard(mutex)(&st->lock);
> +	return st->chan[chan->channel].powerdown_mode - 1;
> +}
> +
> +static int ad3530r_set_powerdown_mode(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      unsigned int mode)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +
> +	guard(mutex)(&st->lock);
> +	st->chan[chan->channel].powerdown_mode = mode + 1;
> +
> +	return 0;
> +}
> +
> +static const struct iio_enum ad3530r_powerdown_mode_enum = {
> +	.items = ad3530r_powerdown_modes,
> +	.num_items = ARRAY_SIZE(ad3530r_powerdown_modes),
> +	.get = ad3530r_get_powerdown_mode,
> +	.set = ad3530r_set_powerdown_mode,
> +};
> +
> +static ssize_t ad3530r_get_dac_powerdown(struct iio_dev *indio_dev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 char *buf)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +
> +	guard(mutex)(&st->lock);
> +	return sysfs_emit(buf, "%d\n", st->chan[chan->channel].powerdown);
> +}
> +
> +static ssize_t ad3530r_set_dac_powerdown(struct iio_dev *indio_dev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 const char *buf, size_t len)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	int ret;
> +	unsigned int mask, val;
> +	bool powerdown;
> +
> +	ret = kstrtobool(buf, &powerdown);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +	switch (chan->channel) {
> +	case AD3530R_CH(0) ... AD3530R_CH(AD3531R_MAX_CHANNELS - 1):
> +		mask = GENMASK(chan->channel * 2 + 1, chan->channel * 2);
> +		val = (powerdown ? st->chan[chan->channel].powerdown_mode :
> 0)
> +		      << (chan->channel * 2);
> +
> +		ret = regmap_update_bits(st->regmap,
> +					 AD3530R_OUTPUT_OPERATING_MODE_0,
> +					 mask, val);
> +		if (ret)
> +			return ret;
> +
> +		st->chan[chan->channel].powerdown = powerdown;
> +		return len;
> +	case AD3530R_CH(AD3531R_MAX_CHANNELS) ...
> +	     AD3530R_CH(AD3530R_MAX_CHANNELS - 1):
> +		mask = GENMASK((chan->channel - 4) * 2 + 1,
> +			       (chan->channel - 4) * 2);
> +		val = (powerdown ? st->chan[chan->channel].powerdown_mode :
> 0)
> +		      << ((chan->channel - 4) * 2);
> +
> +		ret = regmap_update_bits(st->regmap,
> +					 AD3530R_OUTPUT_OPERATING_MODE_1,
> +					 mask, val);
> +		if (ret)
> +			return ret;
> +
> +		st->chan[chan->channel].powerdown = powerdown;
> +		return len;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad3530r_trigger_hw_ldac(struct gpio_desc *ldac_gpio)
> +{
> +	gpiod_set_value_cansleep(ldac_gpio, 1);
> +	fsleep(AD3530R_LDAC_PULSE_US);
> +	gpiod_set_value_cansleep(ldac_gpio, 0);
> +
> +	return 0;
> +}
> +
> +static int ad3530r_dac_write(struct ad3530r_state *st, unsigned int chan,
> +			     unsigned int val)
> +{
> +	int ret;
> +	__be16 reg_val;
> +
> +	guard(mutex)(&st->lock);
> +	reg_val = cpu_to_be16(val);
> +
> +	ret = regmap_bulk_write(st->regmap, st->chip_info-
> >input_ch_reg(chan),
> +				&reg_val, sizeof(reg_val));
> +	if (ret)
> +		return ret;
> +
> +	if (st->ldac_gpio)
> +		return ad3530r_trigger_hw_ldac(st->ldac_gpio);
> +
> +	return regmap_update_bits(st->regmap, st->chip_info-
> >sw_ldac_trig_reg,
> +				  AD3530R_SW_LDAC_TRIG_MASK,
> +				  FIELD_PREP(AD3530R_SW_LDAC_TRIG_MASK, 1));
> +}
> +
> +static int ad3530r_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long info)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	int ret;
> +	__be16 reg_val;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_bulk_read(st->regmap,
> +				       st->chip_info->input_ch_reg(chan-
> >channel),
> +				       &reg_val, sizeof(reg_val));
> +		if (ret)
> +			return ret;
> +
> +		*val = FIELD_GET(AD3530R_REG_VAL_MASK, be16_to_cpu(reg_val));
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_mv;
> +		*val2 = 16;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad3530r_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long info)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val < 0 || val > U16_MAX)
> +			return -EINVAL;
> +
> +		return ad3530r_dac_write(st, chan->channel, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad3530r_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +			      unsigned int writeval, unsigned int *readval)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +
> +	if (readval)
> +		return regmap_read(st->regmap, reg, readval);
> +
> +	return regmap_write(st->regmap, reg, writeval);
> +}
> +
> +#define AD3530R_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) {	\
> +	.name = (_name),						\
> +	.read = (_read),						\
> +	.write = (_write),						\
> +	.private = (_what),						\
> +	.shared = (_shared),						\
> +}
> +
> +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> +	AD3530R_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
> +			      ad3530r_get_dac_powerdown,
> +			      ad3530r_set_dac_powerdown),
> +	IIO_ENUM("powerdown_mode", IIO_SEPARATE,
> &ad3530r_powerdown_mode_enum),
> +	IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> +			   &ad3530r_powerdown_mode_enum),
> +	{ },
> +};
> +
> +#define AD3530R_CHAN(_chan) {						\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _chan,						\
> +	.output = 1,							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +			     
> BIT(IIO_CHAN_INFO_SCALE),			\
> +	.ext_info = ad3530r_ext_info,					\
> +}
> +
> +static const struct iio_chan_spec ad3530r_channels[] = {
> +	AD3530R_CHAN(0),
> +	AD3530R_CHAN(1),
> +	AD3530R_CHAN(2),
> +	AD3530R_CHAN(3),
> +	AD3530R_CHAN(4),
> +	AD3530R_CHAN(5),
> +	AD3530R_CHAN(6),
> +	AD3530R_CHAN(7),
> +};
> +
> +static const struct iio_chan_spec ad3531r_channels[] = {
> +	AD3530R_CHAN(0),
> +	AD3530R_CHAN(1),
> +	AD3530R_CHAN(2),
> +	AD3530R_CHAN(3),
> +};
> +
> +static const struct ad3530r_chip_info ad3530_chip = {
> +	.name = "ad3530",
> +	.channels = ad3530r_channels,
> +	.num_channels = ARRAY_SIZE(ad3530r_channels),
> +	.sw_ldac_trig_reg = AD3530R_SW_LDAC_TRIG_A,
> +	.input_ch_reg = ad3530r_input_ch_reg,
> +	.internal_ref_support = false,
> +};
> +
> +static const struct ad3530r_chip_info ad3530r_chip = {
> +	.name = "ad3530r",
> +	.channels = ad3530r_channels,
> +	.num_channels = ARRAY_SIZE(ad3530r_channels),
> +	.sw_ldac_trig_reg = AD3530R_SW_LDAC_TRIG_A,
> +	.input_ch_reg = ad3530r_input_ch_reg,
> +	.internal_ref_support = true,
> +};
> +
> +static const struct ad3530r_chip_info ad3531_chip = {
> +	.name = "ad3531",
> +	.channels = ad3531r_channels,
> +	.num_channels = ARRAY_SIZE(ad3531r_channels),
> +	.sw_ldac_trig_reg = AD3531R_SW_LDAC_TRIG_A,
> +	.input_ch_reg = ad3531r_input_ch_reg,
> +	.internal_ref_support = false,
> +};
> +
> +static const struct ad3530r_chip_info ad3531r_chip = {
> +	.name = "ad3531r",
> +	.channels = ad3531r_channels,
> +	.num_channels = ARRAY_SIZE(ad3531r_channels),
> +	.sw_ldac_trig_reg = AD3531R_SW_LDAC_TRIG_A,
> +	.input_ch_reg = ad3531r_input_ch_reg,
> +	.internal_ref_support = true,
> +};
> +
> +static int ad3530r_setup(struct ad3530r_state *st)
> +{
> +	const struct ad3530r_chip_info *chip_info = st->chip_info;
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct gpio_desc *reset_gpio;
> +	int i, ret;
> +
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(reset_gpio),
> +				     "Failed to get reset GPIO\n");
> +
> +	if (reset_gpio) {
> +		/* Perform hardware reset */
> +		fsleep(1000);
> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +	} else {
> +		/* Perform software reset */
> +		ret = regmap_update_bits(st->regmap,
> AD3530R_INTERFACE_CONFIG_A,
> +					 AD3530R_SW_RESET, AD3530R_SW_RESET);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fsleep(10000);
> +
> +	/* Set operating mode to normal operation. */
> +	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0, 0);
> +	if (ret)
> +		return ret;
> +
> +	if (chip_info->num_channels > AD3531R_MAX_CHANNELS) {
> +		ret = regmap_write(st->regmap,
> AD3530R_OUTPUT_OPERATING_MODE_1, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < chip_info->num_channels; i++)
> +		st->chan[i].powerdown_mode = AD3530R_32KOHM_POWERDOWN_MODE;
> +
> +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->ldac_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> +				     "Failed to get ldac GPIO\n");
> +
> +	if (device_property_present(dev, "adi,double-output-range")) {
> +		st->range_multiplier = true;
> +
> +		return regmap_update_bits(st->regmap,
> AD3530R_OUTPUT_CONTROL_0,
> +					  AD3530R_OUTPUT_CONTROL_MASK,
> +					 
> FIELD_PREP(AD3530R_OUTPUT_CONTROL_MASK, 1));
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config ad3530r_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +};
> +
> +static const struct iio_info ad3530r_info = {
> +	.read_raw = ad3530r_read_raw,
> +	.write_raw = ad3530r_write_raw,
> +	.debugfs_reg_access = &ad3530r_reg_access,
> +};

I'm not sure if this is what Jonathan had in mind but I think you need to
implement .fwnode_xlate(). Not given too much thought on this but likely you'll
need more than one argument in your cells since you need to specify the type of
the channel (voltage, current or temperature).

Moreover, if the consumer is asking for a voltage channel in the muxout pin it
should be straightforward. But if it is temperature or current I would expect
that to be expressed in your read/write_raw() implementation? But this MUXOUT
thing is only about monitoring right? So write_raw() is not allowed...


- Nuno Sá

Re: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[David Lechner]

On 4/3/25 12:33 AM, Kim Seer Paller wrote:
> The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls, providing full-scale output spans of 2.5V or
> 5V for reference voltages of 2.5V. These devices operate from a single
> 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> by default.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---

...

> +#define AD3530R_INTERFACE_CONFIG_A		0x00
> +#define AD3530R_OUTPUT_OPERATING_MODE_0		0x20
> +#define AD3530R_OUTPUT_OPERATING_MODE_1		0x21
> +#define AD3530R_OUTPUT_CONTROL_0		0x2A
> +#define AD3530R_REFERENCE_CONTROL_0		0x3C
> +#define AD3530R_SW_LDAC_TRIG_A			0xE5
> +#define AD3530R_INPUT_CH(c)			(2 * (c) + 0xEB)
> +
> +#define AD3531R_SW_LDAC_TRIG_A			0xDD
> +#define AD3531R_INPUT_CH(c)			(2 * (c) + 0xE3)
> +
> +#define AD3530R_SW_LDAC_TRIG_MASK		BIT(7)
> +#define AD3530R_OUTPUT_CONTROL_MASK		BIT(2)
> +#define AD3530R_REFERENCE_CONTROL_MASK		BIT(0)
> +#define AD3530R_REG_VAL_MASK			GENMASK(15, 0)
> +
> +#define AD3530R_SW_RESET			(BIT(7) | BIT(0))
> +#define AD3530R_MAX_CHANNELS			8
> +#define AD3531R_MAX_CHANNELS			4
> +#define AD3530R_CH(c)				(c)
> +#define AD3530R_32KOHM_POWERDOWN_MODE		3
> +#define AD3530R_INTERNAL_VREF_MV		2500
> +#define AD3530R_LDAC_PULSE_US			100
> +
> +struct ad3530r_chan {
> +	unsigned int powerdown_mode;

IMHO, code would be easier to understand if this was an enum.

> +	bool powerdown;
> +};
> +
> +struct ad3530r_chip_info {
> +	const char *name;
> +	const struct iio_chan_spec *channels;
> +	int (*input_ch_reg)(unsigned int c);
> +	const int iio_chan;
> +	unsigned int num_channels;
> +	unsigned int sw_ldac_trig_reg;
> +	bool internal_ref_support;
> +};
> +
> +struct ad3530r_state {
> +	struct regmap *regmap;
> +	/* lock to protect against multiple access to the device and shared data */
> +	struct mutex lock;
> +	struct ad3530r_chan chan[AD3530R_MAX_CHANNELS];
> +	const struct ad3530r_chip_info *chip_info;
> +	struct gpio_desc *ldac_gpio;
> +	int vref_mv;
> +	u8 ldac;
> +	bool range_multiplier;

nit: call this has_range_multiplier or change to u8 and save the actual multipler value here.

> +};
> +

...

> +static ssize_t ad3530r_set_dac_powerdown(struct iio_dev *indio_dev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 const char *buf, size_t len)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	int ret;
> +	unsigned int mask, val;
> +	bool powerdown;
> +
> +	ret = kstrtobool(buf, &powerdown);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +	switch (chan->channel) {
> +	case AD3530R_CH(0) ... AD3530R_CH(AD3531R_MAX_CHANNELS - 1):

Switch seems hard to read and not needed since there aren't any gaps.

	if (chan->channel < AD3531R_MAX_CHANNELS)

shoud work, no?


> +		mask = GENMASK(chan->channel * 2 + 1, chan->channel * 2);

Could use the chan->address field to hide the calucation a bit.

> +		val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
> +		      << (chan->channel * 2);

I saw a series that proposed a non-const field_prep function recently. Not sure
that made it through the bikeshedding though. Could be nice to hide some of this
in a macro for readability if that isn't available.

> +
> +		ret = regmap_update_bits(st->regmap,
> +					 AD3530R_OUTPUT_OPERATING_MODE_0,
> +					 mask, val);
> +		if (ret)
> +			return ret;
> +
> +		st->chan[chan->channel].powerdown = powerdown;
> +		return len;
> +	case AD3530R_CH(AD3531R_MAX_CHANNELS) ...
> +	     AD3530R_CH(AD3530R_MAX_CHANNELS - 1):
> +		mask = GENMASK((chan->channel - 4) * 2 + 1,
> +			       (chan->channel - 4) * 2);
> +		val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
> +		      << ((chan->channel - 4) * 2);
> +
> +		ret = regmap_update_bits(st->regmap,
> +					 AD3530R_OUTPUT_OPERATING_MODE_1,
> +					 mask, val);
> +		if (ret)
> +			return ret;
> +
> +		st->chan[chan->channel].powerdown = powerdown;
> +		return len;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad3530r_trigger_hw_ldac(struct gpio_desc *ldac_gpio)
> +{
> +	gpiod_set_value_cansleep(ldac_gpio, 1);
> +	fsleep(AD3530R_LDAC_PULSE_US);
> +	gpiod_set_value_cansleep(ldac_gpio, 0);
> +
> +	return 0;
> +}
> +
> +static int ad3530r_dac_write(struct ad3530r_state *st, unsigned int chan,
> +			     unsigned int val)
> +{
> +	int ret;
> +	__be16 reg_val;
> +
> +	guard(mutex)(&st->lock);
> +	reg_val = cpu_to_be16(val);
> +
> +	ret = regmap_bulk_write(st->regmap, st->chip_info->input_ch_reg(chan),
> +				&reg_val, sizeof(reg_val));
> +	if (ret)
> +		return ret;
> +
> +	if (st->ldac_gpio)
> +		return ad3530r_trigger_hw_ldac(st->ldac_gpio);
> +
> +	return regmap_update_bits(st->regmap, st->chip_info->sw_ldac_trig_reg,
> +				  AD3530R_SW_LDAC_TRIG_MASK,
> +				  FIELD_PREP(AD3530R_SW_LDAC_TRIG_MASK, 1));

Can be simplifed with regmap_set_bits().


> +}
> +
> +static int ad3530r_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long info)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	int ret;
> +	__be16 reg_val;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_bulk_read(st->regmap,
> +				       st->chip_info->input_ch_reg(chan->channel),
> +				       &reg_val, sizeof(reg_val));
> +		if (ret)
> +			return ret;
> +
> +		*val = FIELD_GET(AD3530R_REG_VAL_MASK, be16_to_cpu(reg_val));
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_mv;
> +		*val2 = 16;

This needs to take into account the range multipler.

> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> +	AD3530R_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
> +			      ad3530r_get_dac_powerdown,
> +			      ad3530r_set_dac_powerdown),
> +	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad3530r_powerdown_mode_enum),
> +	IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> +			   &ad3530r_powerdown_mode_enum),
> +	{ },

iio style is no trailing comma on sentinil  

	{ }

> +};
> +

...

> +static int ad3530r_setup(struct ad3530r_state *st)
> +{
> +	const struct ad3530r_chip_info *chip_info = st->chip_info;
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct gpio_desc *reset_gpio;
> +	int i, ret;
> +
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(reset_gpio),
> +				     "Failed to get reset GPIO\n");
> +
> +	if (reset_gpio) {
> +		/* Perform hardware reset */
> +		fsleep(1000);
> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +	} else {
> +		/* Perform software reset */
> +		ret = regmap_update_bits(st->regmap, AD3530R_INTERFACE_CONFIG_A,
> +					 AD3530R_SW_RESET, AD3530R_SW_RESET);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fsleep(10000);
> +
> +	/* Set operating mode to normal operation. */
> +	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0, 0);
> +	if (ret)
> +		return ret;
> +
> +	if (chip_info->num_channels > AD3531R_MAX_CHANNELS) {
> +		ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_1, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < chip_info->num_channels; i++)
> +		st->chan[i].powerdown_mode = AD3530R_32KOHM_POWERDOWN_MODE;
> +
> +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->ldac_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> +				     "Failed to get ldac GPIO\n");
> +
> +	if (device_property_present(dev, "adi,double-output-range")) {

This is not documented in the DT bindings.

This could instead be implemented by making the out_voltage_scale attribute
writeable.

If we really do need it in the devicetree because we want to protect against
someone accidentally setting it too high, then the property should be the
actual multipler value with an enum specifier of 1, 2 and a default of 1
rather than a flag (e.g. adi,output-multipler).

> +		st->range_multiplier = true;
> +
> +		return regmap_update_bits(st->regmap, AD3530R_OUTPUT_CONTROL_0,
> +					  AD3530R_OUTPUT_CONTROL_MASK,
> +					  FIELD_PREP(AD3530R_OUTPUT_CONTROL_MASK, 1));

nit: can be simplified with regmap_set_bits().

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config ad3530r_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,

Should have at least a .max_register.

.reg_read and .reg_write tables can make debuging easier too.

> +};
> +
> +static const struct iio_info ad3530r_info = {
> +	.read_raw = ad3530r_read_raw,
> +	.write_raw = ad3530r_write_raw,
> +	.debugfs_reg_access = &ad3530r_reg_access,

nit: style is not consistent - can omit &.

> +};
> +
> +static int ad3530r_probe(struct spi_device *spi)
> +{
> +	static const char * const regulators[] = { "vdd", "iovdd" };
> +	const struct ad3530r_chip_info *chip_info;
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad3530r_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	st->regmap = devm_regmap_init_spi(spi, &ad3530r_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "Failed to init regmap");
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	chip_info = spi_get_device_match_data(spi);
> +	if (!chip_info)
> +		return -ENODEV;
> +
> +	st->chip_info = chip_info;

nit: local variable isn't that useful since st->chip_info is short enough.

> +
> +	ret = ad3530r_setup(st);
> +	if (ret)
> +		return ret;

Setting up the chip before turning on power would not work well if the
regulators are actually controlled. :-)

> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
> +					     regulators);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (ret < 0 && ret != -ENODEV)
> +		return ret;
> +
> +	if (chip_info->internal_ref_support && ret == -ENODEV) {
> +		/* Internal reference. */
> +		ret = regmap_update_bits(st->regmap, AD3530R_REFERENCE_CONTROL_0,
> +					 AD3530R_REFERENCE_CONTROL_MASK,
> +					 FIELD_PREP(AD3530R_REFERENCE_CONTROL_MASK, 1));
> +		if (ret)
> +			return ret;
> +
> +		st->vref_mv = st->range_multiplier ?
> +			      2 * AD3530R_INTERNAL_VREF_MV :
> +			      AD3530R_INTERNAL_VREF_MV;
> +	} else {
> +		st->vref_mv = st->range_multiplier ? 2 * ret / 1000 : ret / 1000;

ret can be -ENODEV here if !chip_info->internal_ref_support

> +	}> +
> +	indio_dev->name = chip_info->name;
> +	indio_dev->info = &ad3530r_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = chip_info->channels;
> +	indio_dev->num_channels = chip_info->num_channels;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +

RE: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Paller, Kim Seer]

> > +	for (i = 0; i < chip_info->num_channels; i++)
> > +		st->chan[i].powerdown_mode =
> AD3530R_32KOHM_POWERDOWN_MODE;
> > +
> > +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac",
> GPIOD_OUT_HIGH);
> > +	if (IS_ERR(st->ldac_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> > +				     "Failed to get ldac GPIO\n");
> > +
> > +	if (device_property_present(dev, "adi,double-output-range")) {
> 
> This is not documented in the DT bindings.
> 
> This could instead be implemented by making the out_voltage_scale attribute
> writeable.
> 
> If we really do need it in the devicetree because we want to protect against
> someone accidentally setting it too high, then the property should be the actual
> multipler value with an enum specifier of 1, 2 and a default of 1 rather than a
> flag (e.g. adi,output-multipler).

Thank you for the feedback. This should be adi,range-double, which is already
documented in the DT bindings and is also used as a boolean type in other drivers.
I just forgot to update it here. This is a one-bit configuration that doubles the
output range (multiplier of 2).  Should I proceed with the suggested approach?

> 
> > +		st->range_multiplier = true;
> > +
> > +		return regmap_update_bits(st->regmap,
> AD3530R_OUTPUT_CONTROL_0,
> > +					  AD3530R_OUTPUT_CONTROL_MASK,
> > +
> FIELD_PREP(AD3530R_OUTPUT_CONTROL_MASK, 1));
> 
> nit: can be simplified with regmap_set_bits().
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct regmap_config ad3530r_regmap_config = {
> > +	.reg_bits = 16,
> > +	.val_bits = 8,
> 
> Should have at least a .max_register.
> 
> .reg_read and .reg_write tables can make debuging easier too.
> 
> > +};
> > +
> > +static const struct iio_info ad3530r_info = {
> > +	.read_raw = ad3530r_read_raw,
> > +	.write_raw = ad3530r_write_raw,
> > +	.debugfs_reg_access = &ad3530r_reg_access,
> 
> nit: style is not consistent - can omit &.
> 
> > +};
> > +
> > +static int ad3530r_probe(struct spi_device *spi) {
> > +	static const char * const regulators[] = { "vdd", "iovdd" };
> > +	const struct ad3530r_chip_info *chip_info;
> > +	struct device *dev = &spi->dev;
> > +	struct iio_dev *indio_dev;
> > +	struct ad3530r_state *st;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +
> > +	st->regmap = devm_regmap_init_spi(spi, &ad3530r_regmap_config);
> > +	if (IS_ERR(st->regmap))
> > +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> > +				     "Failed to init regmap");
> > +
> > +	ret = devm_mutex_init(dev, &st->lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	chip_info = spi_get_device_match_data(spi);
> > +	if (!chip_info)
> > +		return -ENODEV;
> > +
> > +	st->chip_info = chip_info;
> 
> nit: local variable isn't that useful since st->chip_info is short enough.
> 
> > +
> > +	ret = ad3530r_setup(st);
> > +	if (ret)
> > +		return ret;
> 
> Setting up the chip before turning on power would not work well if the
> regulators are actually controlled. :-)
> 
> > +
> > +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
> > +					     regulators);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to enable
> regulators\n");
> > +
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> > +	if (ret < 0 && ret != -ENODEV)
> > +		return ret;
> > +
> > +	if (chip_info->internal_ref_support && ret == -ENODEV) {
> > +		/* Internal reference. */
> > +		ret = regmap_update_bits(st->regmap,
> AD3530R_REFERENCE_CONTROL_0,
> > +
> AD3530R_REFERENCE_CONTROL_MASK,
> > +
> FIELD_PREP(AD3530R_REFERENCE_CONTROL_MASK, 1));
> > +		if (ret)
> > +			return ret;
> > +
> > +		st->vref_mv = st->range_multiplier ?
> > +			      2 * AD3530R_INTERNAL_VREF_MV :
> > +			      AD3530R_INTERNAL_VREF_MV;
> > +	} else {
> > +		st->vref_mv = st->range_multiplier ? 2 * ret / 1000 : ret / 1000;
> 
> ret can be -ENODEV here if !chip_info->internal_ref_support
> 
> > +	}> +
> > +	indio_dev->name = chip_info->name;
> > +	indio_dev->info = &ad3530r_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->channels = chip_info->channels;
> > +	indio_dev->num_channels = chip_info->num_channels;
> > +
> > +	return devm_iio_device_register(&spi->dev, indio_dev); }
> > +

Re: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[David Lechner]

On 4/7/25 3:01 AM, Paller, Kim Seer wrote:
>>> +	for (i = 0; i < chip_info->num_channels; i++)
>>> +		st->chan[i].powerdown_mode =
>> AD3530R_32KOHM_POWERDOWN_MODE;
>>> +
>>> +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac",
>> GPIOD_OUT_HIGH);
>>> +	if (IS_ERR(st->ldac_gpio))
>>> +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
>>> +				     "Failed to get ldac GPIO\n");
>>> +
>>> +	if (device_property_present(dev, "adi,double-output-range")) {
>>
>> This is not documented in the DT bindings.
>>
>> This could instead be implemented by making the out_voltage_scale attribute
>> writeable.
>>
>> If we really do need it in the devicetree because we want to protect against
>> someone accidentally setting it too high, then the property should be the actual
>> multipler value with an enum specifier of 1, 2 and a default of 1 rather than a
>> flag (e.g. adi,output-multipler).
> 
> Thank you for the feedback. This should be adi,range-double, which is already
> documented in the DT bindings and is also used as a boolean type in other drivers.
> I just forgot to update it here. This is a one-bit configuration that doubles the
> output range (multiplier of 2).  Should I proceed with the suggested approach?
> 

I see adi,range-double in
Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml

We would need to add the same in the new .yaml file added in this
series along with a justification in the commit message of why this
needs to be set in the devicetree rather than at runtime.

Ideally, we would ask the applications engineer for this chip to
find out how real users would want to use this feature to make sure
setting it in the devicetree aligns with that and is not too
restrictive.

RE: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Paller, Kim Seer]

> -----Original Message-----
> From: David Lechner <dlechner@baylibre.com>
> Sent: Tuesday, April 8, 2025 10:06 PM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>; Jonathan Cameron
> <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Rob Herring <robh@kernel.org>;
> Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> <conor+dt@kernel.org>
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and
> AD3531R
> 
> [External]
> 
> On 4/7/25 3:01 AM, Paller, Kim Seer wrote:
> >>> +	for (i = 0; i < chip_info->num_channels; i++)
> >>> +		st->chan[i].powerdown_mode =
> >> AD3530R_32KOHM_POWERDOWN_MODE;
> >>> +
> >>> +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac",
> >> GPIOD_OUT_HIGH);
> >>> +	if (IS_ERR(st->ldac_gpio))
> >>> +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> >>> +				     "Failed to get ldac GPIO\n");
> >>> +
> >>> +	if (device_property_present(dev, "adi,double-output-range")) {
> >>
> >> This is not documented in the DT bindings.
> >>
> >> This could instead be implemented by making the out_voltage_scale
> attribute
> >> writeable.
> >>
> >> If we really do need it in the devicetree because we want to protect against
> >> someone accidentally setting it too high, then the property should be the
> actual
> >> multipler value with an enum specifier of 1, 2 and a default of 1 rather than
> a
> >> flag (e.g. adi,output-multipler).
> >
> > Thank you for the feedback. This should be adi,range-double, which is already
> > documented in the DT bindings and is also used as a boolean type in other
> drivers.
> > I just forgot to update it here. This is a one-bit configuration that doubles the
> > output range (multiplier of 2).  Should I proceed with the suggested
> approach?
> >
> 
> I see adi,range-double in
> Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
> 
> We would need to add the same in the new .yaml file added in this
> series along with a justification in the commit message of why this
> needs to be set in the devicetree rather than at runtime.

I can confirm that the adi,range-double property is already present in the adi,ad3530r.yaml
file as a boolean type, and consistent with its usage in adi,ad7923.yaml.

> 
> Ideally, we would ask the applications engineer for this chip to
> find out how real users would want to use this feature to make sure
> setting it in the devicetree aligns with that and is not too
> restrictive.

The apps engineer for this chip indicated that the boolean type format aligns better with
how users are expected to configure this feature, especially for users who may not be
familiar with specific range values or enum specifiers.

Re: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[David Lechner]

On 4/10/25 3:39 AM, Paller, Kim Seer wrote:
> 
> 
>> -----Original Message-----
>> From: David Lechner <dlechner@baylibre.com>
>> Sent: Tuesday, April 8, 2025 10:06 PM
>> To: Paller, Kim Seer <KimSeer.Paller@analog.com>; Jonathan Cameron
>> <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; Hennerich,
>> Michael <Michael.Hennerich@analog.com>; Rob Herring <robh@kernel.org>;
>> Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
>> <conor+dt@kernel.org>
>> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicetree@vger.kernel.org
>> Subject: Re: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and
>> AD3531R
>>
>> [External]
>>
>> On 4/7/25 3:01 AM, Paller, Kim Seer wrote:
>>>>> +	for (i = 0; i < chip_info->num_channels; i++)
>>>>> +		st->chan[i].powerdown_mode =
>>>> AD3530R_32KOHM_POWERDOWN_MODE;
>>>>> +
>>>>> +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac",
>>>> GPIOD_OUT_HIGH);
>>>>> +	if (IS_ERR(st->ldac_gpio))
>>>>> +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
>>>>> +				     "Failed to get ldac GPIO\n");
>>>>> +
>>>>> +	if (device_property_present(dev, "adi,double-output-range")) {
>>>>
>>>> This is not documented in the DT bindings.
>>>>
>>>> This could instead be implemented by making the out_voltage_scale
>> attribute
>>>> writeable.
>>>>
>>>> If we really do need it in the devicetree because we want to protect against
>>>> someone accidentally setting it too high, then the property should be the
>> actual
>>>> multipler value with an enum specifier of 1, 2 and a default of 1 rather than
>> a
>>>> flag (e.g. adi,output-multipler).
>>>
>>> Thank you for the feedback. This should be adi,range-double, which is already
>>> documented in the DT bindings and is also used as a boolean type in other
>> drivers.
>>> I just forgot to update it here. This is a one-bit configuration that doubles the
>>> output range (multiplier of 2).  Should I proceed with the suggested
>> approach?
>>>
>>
>> I see adi,range-double in
>> Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
>>
>> We would need to add the same in the new .yaml file added in this
>> series along with a justification in the commit message of why this
>> needs to be set in the devicetree rather than at runtime.
> 
> I can confirm that the adi,range-double property is already present in the adi,ad3530r.yaml
> file as a boolean type, and consistent with its usage in adi,ad7923.yaml.

Sorry, I don't know why I didn't see it the first 2 times I looked. :-(
It is there.

> 
>>
>> Ideally, we would ask the applications engineer for this chip to
>> find out how real users would want to use this feature to make sure
>> setting it in the devicetree aligns with that and is not too
>> restrictive.
> 
> The apps engineer for this chip indicated that the boolean type format aligns better with
> how users are expected to configure this feature, especially for users who may not be
> familiar with specific range values or enum specifiers.
> 

Very good.

Re: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Jonathan Cameron]

On Thu, 3 Apr 2025 13:33:57 +0800
Kim Seer Paller <kimseer.paller@analog.com> wrote:

Hi Kim,
> The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls, providing full-scale output spans of 2.5V or
> 5V for reference voltages of 2.5V. These devices operate from a single
> 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> by default.


As below. Given the bindings provide for use with an external ADC to read
a wide range of signals, if intent is not to provide that support 'yet'
(which is fine) then add a paragraph here to say that.

> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>


A few additional comments from me inline.  Particular fun is the
long running SPI regmap DMA safety assumptions question.
For now I think we have to assume bulk read/write still need
a DMA safe buffer.

Jonathan

> diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..4b757e19f0c8349999f72e53abb1a4f483a44eb2
> --- /dev/null
> +++ b/drivers/iio/dac/ad3530r.c
> @@ -0,0 +1,514 @@

> +
> +#define AD3530R_INTERFACE_CONFIG_A		0x00
> +#define AD3530R_OUTPUT_OPERATING_MODE_0		0x20
> +#define AD3530R_OUTPUT_OPERATING_MODE_1		0x21
> +#define AD3530R_OUTPUT_CONTROL_0		0x2A
> +#define AD3530R_REFERENCE_CONTROL_0		0x3C
> +#define AD3530R_SW_LDAC_TRIG_A			0xE5
> +#define AD3530R_INPUT_CH(c)			(2 * (c) + 0xEB)
> +
> +#define AD3531R_SW_LDAC_TRIG_A			0xDD
> +#define AD3531R_INPUT_CH(c)			(2 * (c) + 0xE3)

I'd add a define for the first channel and then just have the
rest of this in the device specific function that uses it.

> +
> +#define AD3530R_SW_LDAC_TRIG_MASK		BIT(7)
> +#define AD3530R_OUTPUT_CONTROL_MASK		BIT(2)
> +#define AD3530R_REFERENCE_CONTROL_MASK		BIT(0)
> +#define AD3530R_REG_VAL_MASK			GENMASK(15, 0)
> +
> +#define AD3530R_SW_RESET			(BIT(7) | BIT(0))
> +#define AD3530R_MAX_CHANNELS			8
> +#define AD3531R_MAX_CHANNELS			4
> +#define AD3530R_CH(c)				(c)

This seems a little unnecessary...
Given it is only called in device specific functions I'd just encode that
there.

> +#define AD3530R_32KOHM_POWERDOWN_MODE		3
> +#define AD3530R_INTERNAL_VREF_MV		2500
> +#define AD3530R_LDAC_PULSE_US			100

> +static int ad3530r_dac_write(struct ad3530r_state *st, unsigned int chan,
> +			     unsigned int val)
> +{
> +	int ret;
> +	__be16 reg_val;
> +
> +	guard(mutex)(&st->lock);
> +	reg_val = cpu_to_be16(val);
> +
> +	ret = regmap_bulk_write(st->regmap, st->chip_info->input_ch_reg(chan),
> +				&reg_val, sizeof(reg_val));

As below. regmap_bulk_write() shouldn't assume the buffer is bounced so
needs a DMA safe buffer.

> +	if (ret)
> +		return ret;
> +
> +	if (st->ldac_gpio)
> +		return ad3530r_trigger_hw_ldac(st->ldac_gpio);
> +
> +	return regmap_update_bits(st->regmap, st->chip_info->sw_ldac_trig_reg,
> +				  AD3530R_SW_LDAC_TRIG_MASK,
> +				  FIELD_PREP(AD3530R_SW_LDAC_TRIG_MASK, 1));
> +}
> +
> +static int ad3530r_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long info)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	int ret;
> +	__be16 reg_val;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_bulk_read(st->regmap,
> +				       st->chip_info->input_ch_reg(chan->channel),
> +				       &reg_val, sizeof(reg_val));

So this runs into the old question of whether we need a DMA safe buffer
for a regmap_bulk_read when the bus is SPI.  In practice we probably
don't because of internal details of the regmap but I believe nothing has
changed on the guidance to not assume that in drivers.  So this regval
should be DMA safe. Easiest option is a
__be16 val __aligned(IIO_DMA_MINALIGN) at then end of the st structure
though we will also need to take a mutex to prevent multiple uses of that
buffer.

Maybe we should revisit this with Mark. I checked briefly and 'think'
there is always a copy.

> +		if (ret)
> +			return ret;
> +
> +		*val = FIELD_GET(AD3530R_REG_VAL_MASK, be16_to_cpu(reg_val));
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_mv;
> +		*val2 = 16;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +#define AD3530R_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) {	\
> +	.name = (_name),						\
> +	.read = (_read),						\
> +	.write = (_write),						\
> +	.private = (_what),						\
> +	.shared = (_shared),						\
> +}
> +
> +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> +	AD3530R_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
> +			      ad3530r_get_dac_powerdown,
> +			      ad3530r_set_dac_powerdown),
As there is only one don't use a macro.


	{
		.name = "powerdown",
		.shared = IIO_SEPARATE,
		.read = ...
		.write= ...
	},

> +	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad3530r_powerdown_mode_enum),
> +	IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> +			   &ad3530r_powerdown_mode_enum),
> +	{ },
> +};
> +
> +#define AD3530R_CHAN(_chan) {						\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _chan,						\
> +	.output = 1,							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +			      BIT(IIO_CHAN_INFO_SCALE),			\
> +	.ext_info = ad3530r_ext_info,					\
> +}
> +
> +static const struct iio_chan_spec ad3530r_channels[] = {

Hmm. You only have output channels but we have the stuff
to read the mux value on a pin.  (Nuno mentioned this as well).
If that is enabled I'd expect to see all the input channels to reflect what
we might read by putting it on that pin and reading it with an ADC we
are consumer.

I'm fine if we make that a job for another day though
just state that really clearly in the patch description.

> +	AD3530R_CHAN(0),
> +	AD3530R_CHAN(1),
> +	AD3530R_CHAN(2),
> +	AD3530R_CHAN(3),
> +	AD3530R_CHAN(4),
> +	AD3530R_CHAN(5),
> +	AD3530R_CHAN(6),
> +	AD3530R_CHAN(7),
> +};
> +
> +static const struct iio_chan_spec ad3531r_channels[] = {
> +	AD3530R_CHAN(0),
> +	AD3530R_CHAN(1),
> +	AD3530R_CHAN(2),
> +	AD3530R_CHAN(3),
> +};

RE: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Paller, Kim Seer]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, April 6, 2025 12:12 AM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Rob Herring <robh@kernel.org>; Krzysztof
> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and
> AD3531R
> 
> [External]
> 
> On Thu, 3 Apr 2025 13:33:57 +0800
> Kim Seer Paller <kimseer.paller@analog.com> wrote:
> 
> Hi Kim,
> > The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> > low-power, 16-bit, buffered voltage output DACs with software-
> > programmable gain controls, providing full-scale output spans of 2.5V
> > or 5V for reference voltages of 2.5V. These devices operate from a
> > single 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> > variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> > by default.
> 
> 
> As below. Given the bindings provide for use with an external ADC to read a
> wide range of signals, if intent is not to provide that support 'yet'
> (which is fine) then add a paragraph here to say that.
> 
> >
> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> 
> 
> A few additional comments from me inline.  Particular fun is the long running
> SPI regmap DMA safety assumptions question.
> For now I think we have to assume bulk read/write still need a DMA safe
> buffer.
> 
> Jonathan
> 
> > diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c new
> > file mode 100644 index
> >
> 0000000000000000000000000000000000000000..4b757e19f0c8349999f72e
> 53abb1
> > a4f483a44eb2
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad3530r.c
> > @@ -0,0 +1,514 @@
> 
> > +
> > +#define AD3530R_INTERFACE_CONFIG_A		0x00
> > +#define AD3530R_OUTPUT_OPERATING_MODE_0		0x20
> > +#define AD3530R_OUTPUT_OPERATING_MODE_1		0x21
> > +#define AD3530R_OUTPUT_CONTROL_0		0x2A
> > +#define AD3530R_REFERENCE_CONTROL_0		0x3C
> > +#define AD3530R_SW_LDAC_TRIG_A			0xE5
> > +#define AD3530R_INPUT_CH(c)			(2 * (c) + 0xEB)
> > +
> > +#define AD3531R_SW_LDAC_TRIG_A			0xDD
> > +#define AD3531R_INPUT_CH(c)			(2 * (c) + 0xE3)
> 
> I'd add a define for the first channel and then just have the rest of this in the
> device specific function that uses it.
> 
> > +
> > +#define AD3530R_SW_LDAC_TRIG_MASK		BIT(7)
> > +#define AD3530R_OUTPUT_CONTROL_MASK		BIT(2)
> > +#define AD3530R_REFERENCE_CONTROL_MASK		BIT(0)
> > +#define AD3530R_REG_VAL_MASK			GENMASK(15, 0)
> > +
> > +#define AD3530R_SW_RESET			(BIT(7) | BIT(0))
> > +#define AD3530R_MAX_CHANNELS			8
> > +#define AD3531R_MAX_CHANNELS			4
> > +#define AD3530R_CH(c)				(c)
> 
> This seems a little unnecessary...
> Given it is only called in device specific functions I'd just encode that there.
> 
> > +#define AD3530R_32KOHM_POWERDOWN_MODE		3
> > +#define AD3530R_INTERNAL_VREF_MV		2500
> > +#define AD3530R_LDAC_PULSE_US			100
> 
> > +static int ad3530r_dac_write(struct ad3530r_state *st, unsigned int chan,
> > +			     unsigned int val)
> > +{
> > +	int ret;
> > +	__be16 reg_val;
> > +
> > +	guard(mutex)(&st->lock);
> > +	reg_val = cpu_to_be16(val);
> > +
> > +	ret = regmap_bulk_write(st->regmap, st->chip_info-
> >input_ch_reg(chan),
> > +				&reg_val, sizeof(reg_val));
> 
> As below. regmap_bulk_write() shouldn't assume the buffer is bounced so
> needs a DMA safe buffer.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->ldac_gpio)
> > +		return ad3530r_trigger_hw_ldac(st->ldac_gpio);
> > +
> > +	return regmap_update_bits(st->regmap, st->chip_info-
> >sw_ldac_trig_reg,
> > +				  AD3530R_SW_LDAC_TRIG_MASK,
> > +
> FIELD_PREP(AD3530R_SW_LDAC_TRIG_MASK, 1)); }
> > +
> > +static int ad3530r_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int *val, int *val2, long info) {
> > +	struct ad3530r_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +	__be16 reg_val;
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = regmap_bulk_read(st->regmap,
> > +				       st->chip_info->input_ch_reg(chan-
> >channel),
> > +				       &reg_val, sizeof(reg_val));
> 
> So this runs into the old question of whether we need a DMA safe buffer for a
> regmap_bulk_read when the bus is SPI.  In practice we probably don't because
> of internal details of the regmap but I believe nothing has changed on the
> guidance to not assume that in drivers.  So this regval should be DMA safe.
> Easiest option is a
> __be16 val __aligned(IIO_DMA_MINALIGN) at then end of the st structure
> though we will also need to take a mutex to prevent multiple uses of that
> buffer.
> 
> Maybe we should revisit this with Mark. I checked briefly and 'think'
> there is always a copy.
> 
> > +		if (ret)
> > +			return ret;
> > +
> > +		*val = FIELD_GET(AD3530R_REG_VAL_MASK,
> be16_to_cpu(reg_val));
> > +
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = st->vref_mv;
> > +		*val2 = 16;
> > +
> > +		return IIO_VAL_FRACTIONAL_LOG2;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> 
> > +
> > +#define AD3530R_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) {
> 	\
> > +	.name = (_name),						\
> > +	.read = (_read),						\
> > +	.write = (_write),						\
> > +	.private = (_what),						\
> > +	.shared = (_shared),						\
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> > +	AD3530R_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
> > +			      ad3530r_get_dac_powerdown,
> > +			      ad3530r_set_dac_powerdown),
> As there is only one don't use a macro.
> 
> 
> 	{
> 		.name = "powerdown",
> 		.shared = IIO_SEPARATE,
> 		.read = ...
> 		.write= ...
> 	},
> 
> > +	IIO_ENUM("powerdown_mode", IIO_SEPARATE,
> &ad3530r_powerdown_mode_enum),
> > +	IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> > +			   &ad3530r_powerdown_mode_enum),
> > +	{ },
> > +};
> > +
> > +#define AD3530R_CHAN(_chan) {
> 	\
> > +	.type = IIO_VOLTAGE,						\
> > +	.indexed = 1,							\
> > +	.channel = _chan,						\
> > +	.output = 1,							\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 	\
> > +			      BIT(IIO_CHAN_INFO_SCALE),			\
> > +	.ext_info = ad3530r_ext_info,					\
> > +}
> > +
> > +static const struct iio_chan_spec ad3530r_channels[] = {
> 
> Hmm. You only have output channels but we have the stuff to read the mux
> value on a pin.  (Nuno mentioned this as well).
> If that is enabled I'd expect to see all the input channels to reflect what we
> might read by putting it on that pin and reading it with an ADC we are
> consumer.
> 
> I'm fine if we make that a job for another day though just state that really
> clearly in the patch description.

Thank you for the feedback. I agree to not support this for now, as I need to
further understand the implementation on my end. I'll state this limitation
in the patch description.

> > +	AD3530R_CHAN(0),
> > +	AD3530R_CHAN(1),
> > +	AD3530R_CHAN(2),
> > +	AD3530R_CHAN(3),
> > +	AD3530R_CHAN(4),
> > +	AD3530R_CHAN(5),
> > +	AD3530R_CHAN(6),
> > +	AD3530R_CHAN(7),
> > +};
> > +
> > +static const struct iio_chan_spec ad3531r_channels[] = {
> > +	AD3530R_CHAN(0),
> > +	AD3530R_CHAN(1),
> > +	AD3530R_CHAN(2),
> > +	AD3530R_CHAN(3),
> > +};

Re: [PATCH v3 0/3] Add driver for AD3530R and AD3531R DACs

[Krzysztof Kozlowski]

On Thu, Apr 03, 2025 at 01:33:54PM +0800, Kim Seer Paller wrote:
> The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls, providing full-scale output spans of 2.5V or
> 5V for reference voltages of 2.5V. These devices operate from a single
> 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> by default.
> 
> The AD3531R/AD3531 is not yet released, so the only available datasheet
> for now is the AD3530R/AD3530. The only differences between the two is
> the number of channels, and register addresses of some registers.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---
> Changes in v3:
> - Drop ABI docs.
> 
> Bindings:
> - Drop reviewer's tag.

Why? Explain why you are dropping.

Best regards,
Krzysztof

RE: [PATCH v3 0/3] Add driver for AD3530R and AD3531R DACs

[Paller, Kim Seer]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Thursday, April 3, 2025 2:44 PM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> Conor Dooley <conor+dt@kernel.org>; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v3 0/3] Add driver for AD3530R and AD3531R DACs
> 
> [External]
> 
> On Thu, Apr 03, 2025 at 01:33:54PM +0800, Kim Seer Paller wrote:
> > The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> > low-power, 16-bit, buffered voltage output DACs with software-
> > programmable gain controls, providing full-scale output spans of 2.5V
> > or 5V for reference voltages of 2.5V. These devices operate from a
> > single 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> > variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> > by default.
> >
> > The AD3531R/AD3531 is not yet released, so the only available
> > datasheet for now is the AD3530R/AD3530. The only differences between
> > the two is the number of channels, and register addresses of some registers.
> >
> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> > ---
> > Changes in v3:
> > - Drop ABI docs.
> >
> > Bindings:
> > - Drop reviewer's tag.
> 
> Why? Explain why you are dropping.

I introduce modifications that might require re-review of the updated implementation
(e.g, adding io-channels property, switching to unevaluatedProperties: false, and adding
non-r variants to the compatible list). Is the tag still applicable?

> 
> Best regards,
> Krzysztof