[PATCH 0/2] add AD8460 DAC driver

[PATCH 0/2] add AD8460 DAC driver

[Mariel Tinaco]

The AD8460 is a 14-bit, high power +-40V 1A, high-speed DAC,
with dual digital input modes, programmable supply current and
fault monitoring and protection settings for output current,
output voltage and junction temperature.

The fault monitoring and shutdown protection features were
supported in the earlier versions of the IIO driver but was
scrapped due to uncertainties if the functionalities belong to
the IIO driver. However, it would be best to implement it for
the device's quality of life. I'd like to know if it's better
suited as a stand-alone HWMON driver.

The following are the configurable and readable parameters
through SPI that could be implemented on the HWMON driver:
  * An enable bit to arm/protect the device on overcurrent,
overvoltage or overtemperature events. The device is shut down
upon detection.
  * A configurable range/threshold for voltage, current and
temperature that raises alarm when exceeded while the device is
armed.
  * Flags that can be polled to raise alarm upon detection of
overcurrent, overvoltage or overtemperature events, and apply
additional protective measures.
  * Programmable quiescent current (optional)
  * Thermal monitoring is done by measuring voltage on TMP pin
(unlikely to be included)

Mariel Tinaco (2):
  dt-bindings: iio: dac: add docs for ad8460
  iio: dac: support the ad8460 Waveform DAC

 .../bindings/iio/dac/adi,ad8460.yaml          |  67 ++
 MAINTAINERS                                   |   8 +
 drivers/iio/dac/Kconfig                       |  13 +
 drivers/iio/dac/Makefile                      |   1 +
 drivers/iio/dac/ad8460.c                      | 652 ++++++++++++++++++
 5 files changed, 741 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
 create mode 100644 drivers/iio/dac/ad8460.c


base-commit: 9900e7a54764998ba3a22f06ec629f7b5fe0b422
-- 
2.34.1

[PATCH 1/2] dt-bindings: iio: dac: add docs for ad8460

[Mariel Tinaco]

This adds the bindings documentation for the 14-bit
High Voltage, High Current, Waveform Generator
Digital-to-Analog converter.

Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
---
 .../bindings/iio/dac/adi,ad8460.yaml          | 67 +++++++++++++++++++
 MAINTAINERS                                   |  7 ++
 2 files changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
new file mode 100644
index 000000000..924f76209
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2024 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/dac/adi,ad8460.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD8460 DAC
+
+maintainers:
+  - Mariel Tinaco <mariel.tinaco@analog.com>
+
+description: |
+  Analog Devices AD8460 110 V High Voltage, 1 A High Current,
+  Arbitrary Waveform Generator with Integrated 14-Bit High Speed DAC
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad8460.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad8460
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 20000000
+
+  vref-supply:
+    description: Drive voltage in the range of 1.2V maximum to as low as
+      low as 0.12V through the REF_IO pin to adjust full scale output span
+
+  clocks:
+    description: The clock for the DAC. This is the sync clock
+
+  adi,rset-ohms:
+    description: Specify value of external resistor connected to FS_ADJ pin
+      to establish internal HVDAC's reference current I_REF
+    minimum: 2000
+    maximum: 20000
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+additionalProperties: false
+
+examples:
+  - |
+
+    spi {
+        dac@0 {
+            compatible = "adi,ad8460";
+            reg = <0>;
+            spi-max-frequency = <8000000>;
+            adi,rset-ohms = <2000>;
+
+            vref-supply = <&vrefio>;
+            clocks = <&sync_ext_clk>;
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 758c202ec..dae93df2a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1234,6 +1234,13 @@ W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
 F:	drivers/iio/adc/ad7780.c
 
+ANALOG DEVICES INC AD8460 DRIVER
+M:	Mariel Tinaco <Mariel.Tinaco@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,ad8460.yaml
+
 ANALOG DEVICES INC AD9739a DRIVER
 M:	Nuno Sa <nuno.sa@analog.com>
 M:	Dragos Bogdan <dragos.bogdan@analog.com>
-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: iio: dac: add docs for ad8460

[Rob Herring (Arm)]

On Fri, 10 May 2024 14:40:52 +0800, Mariel Tinaco wrote:
> This adds the bindings documentation for the 14-bit
> High Voltage, High Current, Waveform Generator
> Digital-to-Analog converter.
> 
> Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> ---
>  .../bindings/iio/dac/adi,ad8460.yaml          | 67 +++++++++++++++++++
>  MAINTAINERS                                   |  7 ++
>  2 files changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/iio/dac/adi,ad8460.example.dts:22.17-27: Warning (reg_format): /example-0/spi/dac@0:reg: property has invalid length (4 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/iio/dac/adi,ad8460.example.dtb: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/iio/dac/adi,ad8460.example.dtb: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/iio/dac/adi,ad8460.example.dtb: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/iio/dac/adi,ad8460.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/iio/dac/adi,ad8460.example.dts:19.13-29.11: Warning (spi_bus_bridge): /example-0/spi: incorrect #address-cells for SPI bus
Documentation/devicetree/bindings/iio/dac/adi,ad8460.example.dts:19.13-29.11: Warning (spi_bus_bridge): /example-0/spi: incorrect #size-cells for SPI bus
Documentation/devicetree/bindings/iio/dac/adi,ad8460.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/iio/dac/adi,ad8460.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'spi_bus_bridge'
Documentation/devicetree/bindings/iio/dac/adi,ad8460.example.dts:20.19-28.15: Warning (avoid_default_addr_size): /example-0/spi/dac@0: Relying on default #address-cells value
Documentation/devicetree/bindings/iio/dac/adi,ad8460.example.dts:20.19-28.15: Warning (avoid_default_addr_size): /example-0/spi/dac@0: Relying on default #size-cells value
Documentation/devicetree/bindings/iio/dac/adi,ad8460.example.dtb: Warning (unique_unit_address_if_enabled): Failed prerequisite 'avoid_default_addr_size'

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240510064053.278257-2-Mariel.Tinaco@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 1/2] dt-bindings: iio: dac: add docs for ad8460

[David Lechner]

On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
>
> This adds the bindings documentation for the 14-bit
> High Voltage, High Current, Waveform Generator
> Digital-to-Analog converter.
>
> Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> ---
>  .../bindings/iio/dac/adi,ad8460.yaml          | 67 +++++++++++++++++++
>  MAINTAINERS                                   |  7 ++
>  2 files changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> new file mode 100644
> index 000000000..924f76209
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2024 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/dac/adi,ad8460.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD8460 DAC
> +
> +maintainers:
> +  - Mariel Tinaco <mariel.tinaco@analog.com>
> +
> +description: |
> +  Analog Devices AD8460 110 V High Voltage, 1 A High Current,
> +  Arbitrary Waveform Generator with Integrated 14-Bit High Speed DAC
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad8460.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad8460
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 20000000
> +
> +  vref-supply:

It would be nice to make the property name match the pin name since
there is more than one reference voltage input.

refio-1p2v-supply:

> +    description: Drive voltage in the range of 1.2V maximum to as low as
> +      low as 0.12V through the REF_IO pin to adjust full scale output span

I don't seen anything in the datasheet named REF_IO. Is this a typo
and it should be REFIO_1P2V?

> +
> +  clocks:
> +    description: The clock for the DAC. This is the sync clock
> +
> +  adi,rset-ohms:
> +    description: Specify value of external resistor connected to FS_ADJ pin
> +      to establish internal HVDAC's reference current I_REF
> +    minimum: 2000
> +    maximum: 20000
> +

I see lots more pins on the datasheet, many of which should be trivial
to add bindings for (we prefer to have the bindings as complete as
possible even if the driver doesn't implement everything). Potential
candidates:

sdn-reset-gpios: (active high)
reset-gpios: (active low)
sdn-io-gpios: (active high)

hvcc-supply:
hvee-supply:
vcc-5v-supply:
vref-5v-supply:
dvdd-3p3v-supply:
avdd-3p3v-supply:

It also looks like there is a parallel interface for data, so I would
expect to see an io-backends property that links to the PHY used for
handling that.


> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    spi {
> +        dac@0 {
> +            compatible = "adi,ad8460";
> +            reg = <0>;
> +            spi-max-frequency = <8000000>;
> +            adi,rset-ohms = <2000>;
> +
> +            vref-supply = <&vrefio>;
> +            clocks = <&sync_ext_clk>;
> +        };
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 758c202ec..dae93df2a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1234,6 +1234,13 @@ W:       https://ez.analog.com/linux-software-drivers
>  F:     Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
>  F:     drivers/iio/adc/ad7780.c
>
> +ANALOG DEVICES INC AD8460 DRIVER
> +M:     Mariel Tinaco <Mariel.Tinaco@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,ad8460.yaml
> +
>  ANALOG DEVICES INC AD9739a DRIVER
>  M:     Nuno Sa <nuno.sa@analog.com>
>  M:     Dragos Bogdan <dragos.bogdan@analog.com>
> --
> 2.34.1
>
>

Re: [PATCH 1/2] dt-bindings: iio: dac: add docs for ad8460

[Jonathan Cameron]

On Fri, 10 May 2024 12:28:19 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
> >
> > This adds the bindings documentation for the 14-bit
> > High Voltage, High Current, Waveform Generator
> > Digital-to-Analog converter.
> >
> > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> > ---
> >  .../bindings/iio/dac/adi,ad8460.yaml          | 67 +++++++++++++++++++
> >  MAINTAINERS                                   |  7 ++
> >  2 files changed, 74 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > new file mode 100644
> > index 000000000..924f76209
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > @@ -0,0 +1,67 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2024 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/dac/adi,ad8460.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices AD8460 DAC
> > +
> > +maintainers:
> > +  - Mariel Tinaco <mariel.tinaco@analog.com>
> > +
> > +description: |
> > +  Analog Devices AD8460 110 V High Voltage, 1 A High Current,
> > +  Arbitrary Waveform Generator with Integrated 14-Bit High Speed DAC
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad8460.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,ad8460
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    maximum: 20000000
> > +
> > +  vref-supply:  
> 
> It would be nice to make the property name match the pin name since
> there is more than one reference voltage input.
> 
> refio-1p2v-supply:
> 
> > +    description: Drive voltage in the range of 1.2V maximum to as low as
> > +      low as 0.12V through the REF_IO pin to adjust full scale output span  
> 
> I don't seen anything in the datasheet named REF_IO. Is this a typo
> and it should be REFIO_1P2V?
> 
> > +
> > +  clocks:
> > +    description: The clock for the DAC. This is the sync clock
> > +
> > +  adi,rset-ohms:
> > +    description: Specify value of external resistor connected to FS_ADJ pin
> > +      to establish internal HVDAC's reference current I_REF
> > +    minimum: 2000
> > +    maximum: 20000
> > +  
> 
> I see lots more pins on the datasheet, many of which should be trivial
> to add bindings for (we prefer to have the bindings as complete as
> possible even if the driver doesn't implement everything). Potential
> candidates:
> 
> sdn-reset-gpios: (active high)
> reset-gpios: (active low)
> sdn-io-gpios: (active high)
> 
> hvcc-supply:
> hvee-supply:
> vcc-5v-supply:
> vref-5v-supply:
> dvdd-3p3v-supply:
> avdd-3p3v-supply:
> 
> It also looks like there is a parallel interface for data, so I would
> expect to see an io-backends property that links to the PHY used for
> handling that.
> 
Ultimately yes, but the parallel interface might require some decisions on 
binding that are non obvious until it's actually implemented. So maybe
don't need that bit from the start.  The rest I agree should be here.


> 
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +
> > +allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    spi {
> > +        dac@0 {
> > +            compatible = "adi,ad8460";
> > +            reg = <0>;
> > +            spi-max-frequency = <8000000>;
> > +            adi,rset-ohms = <2000>;
> > +
> > +            vref-supply = <&vrefio>;
> > +            clocks = <&sync_ext_clk>;
> > +        };
> > +    };
> > +
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 758c202ec..dae93df2a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1234,6 +1234,13 @@ W:       https://ez.analog.com/linux-software-drivers
> >  F:     Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> >  F:     drivers/iio/adc/ad7780.c
> >
> > +ANALOG DEVICES INC AD8460 DRIVER
> > +M:     Mariel Tinaco <Mariel.Tinaco@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,ad8460.yaml
> > +
> >  ANALOG DEVICES INC AD9739a DRIVER
> >  M:     Nuno Sa <nuno.sa@analog.com>
> >  M:     Dragos Bogdan <dragos.bogdan@analog.com>
> > --
> > 2.34.1
> >
> >  

Re: [PATCH 1/2] dt-bindings: iio: dac: add docs for ad8460

[David Lechner]

On Sat, May 11, 2024 at 11:25 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 10 May 2024 12:28:19 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
> > On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
> > >
> > > This adds the bindings documentation for the 14-bit
> > > High Voltage, High Current, Waveform Generator
> > > Digital-to-Analog converter.
> > >
> > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> > > ---
> > >  .../bindings/iio/dac/adi,ad8460.yaml          | 67 +++++++++++++++++++
> > >  MAINTAINERS                                   |  7 ++
> > >  2 files changed, 74 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > > new file mode 100644
> > > index 000000000..924f76209
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > > @@ -0,0 +1,67 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +# Copyright 2024 Analog Devices Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/dac/adi,ad8460.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Analog Devices AD8460 DAC
> > > +
> > > +maintainers:
> > > +  - Mariel Tinaco <mariel.tinaco@analog.com>
> > > +
> > > +description: |
> > > +  Analog Devices AD8460 110 V High Voltage, 1 A High Current,
> > > +  Arbitrary Waveform Generator with Integrated 14-Bit High Speed DAC
> > > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad8460.pdf
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - adi,ad8460
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  spi-max-frequency:
> > > +    maximum: 20000000
> > > +
> > > +  vref-supply:
> >
> > It would be nice to make the property name match the pin name since
> > there is more than one reference voltage input.
> >
> > refio-1p2v-supply:
> >
> > > +    description: Drive voltage in the range of 1.2V maximum to as low as
> > > +      low as 0.12V through the REF_IO pin to adjust full scale output span
> >
> > I don't seen anything in the datasheet named REF_IO. Is this a typo
> > and it should be REFIO_1P2V?
> >
> > > +
> > > +  clocks:
> > > +    description: The clock for the DAC. This is the sync clock
> > > +
> > > +  adi,rset-ohms:
> > > +    description: Specify value of external resistor connected to FS_ADJ pin
> > > +      to establish internal HVDAC's reference current I_REF
> > > +    minimum: 2000
> > > +    maximum: 20000
> > > +
> >
> > I see lots more pins on the datasheet, many of which should be trivial
> > to add bindings for (we prefer to have the bindings as complete as
> > possible even if the driver doesn't implement everything). Potential
> > candidates:
> >
> > sdn-reset-gpios: (active high)
> > reset-gpios: (active low)
> > sdn-io-gpios: (active high)
> >
> > hvcc-supply:
> > hvee-supply:
> > vcc-5v-supply:
> > vref-5v-supply:
> > dvdd-3p3v-supply:
> > avdd-3p3v-supply:
> >
> > It also looks like there is a parallel interface for data, so I would
> > expect to see an io-backends property that links to the PHY used for
> > handling that.
> >
> Ultimately yes, but the parallel interface might require some decisions on
> binding that are non obvious until it's actually implemented. So maybe
> don't need that bit from the start.  The rest I agree should be here.
>
>

Since the driver patch uses a DMA channel that isn't documented here,
I am assuming that the parallel interface is being used so we do need
to consider it now. :-)

Re: [PATCH 1/2] dt-bindings: iio: dac: add docs for ad8460

[Nuno Sá]

On Sat, 2024-05-11 at 13:47 -0500, David Lechner wrote:
> On Sat, May 11, 2024 at 11:25 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > 
> > On Fri, 10 May 2024 12:28:19 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> > 
> > > On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com>
> > > wrote:
> > > > 
> > > > This adds the bindings documentation for the 14-bit
> > > > High Voltage, High Current, Waveform Generator
> > > > Digital-to-Analog converter.
> > > > 
> > > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> > > > ---
> > > >  .../bindings/iio/dac/adi,ad8460.yaml          | 67 +++++++++++++++++++
> > > >  MAINTAINERS                                   |  7 ++
> > > >  2 files changed, 74 insertions(+)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > > > new file mode 100644
> > > > index 000000000..924f76209
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > > > @@ -0,0 +1,67 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +# Copyright 2024 Analog Devices Inc.
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/iio/dac/adi,ad8460.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Analog Devices AD8460 DAC
> > > > +
> > > > +maintainers:
> > > > +  - Mariel Tinaco <mariel.tinaco@analog.com>
> > > > +
> > > > +description: |
> > > > +  Analog Devices AD8460 110 V High Voltage, 1 A High Current,
> > > > +  Arbitrary Waveform Generator with Integrated 14-Bit High Speed DAC
> > > > + 
> > > > https://www.analog.com/media/en/technical-documentation/data-sheets/ad8460.pdf
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - adi,ad8460
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  spi-max-frequency:
> > > > +    maximum: 20000000
> > > > +
> > > > +  vref-supply:
> > > 
> > > It would be nice to make the property name match the pin name since
> > > there is more than one reference voltage input.
> > > 
> > > refio-1p2v-supply:
> > > 
> > > > +    description: Drive voltage in the range of 1.2V maximum to as low
> > > > as
> > > > +      low as 0.12V through the REF_IO pin to adjust full scale output
> > > > span
> > > 
> > > I don't seen anything in the datasheet named REF_IO. Is this a typo
> > > and it should be REFIO_1P2V?
> > > 
> > > > +
> > > > +  clocks:
> > > > +    description: The clock for the DAC. This is the sync clock
> > > > +
> > > > +  adi,rset-ohms:
> > > > +    description: Specify value of external resistor connected to FS_ADJ
> > > > pin
> > > > +      to establish internal HVDAC's reference current I_REF
> > > > +    minimum: 2000
> > > > +    maximum: 20000
> > > > +
> > > 
> > > I see lots more pins on the datasheet, many of which should be trivial
> > > to add bindings for (we prefer to have the bindings as complete as
> > > possible even if the driver doesn't implement everything). Potential
> > > candidates:
> > > 
> > > sdn-reset-gpios: (active high)
> > > reset-gpios: (active low)
> > > sdn-io-gpios: (active high)
> > > 
> > > hvcc-supply:
> > > hvee-supply:
> > > vcc-5v-supply:
> > > vref-5v-supply:
> > > dvdd-3p3v-supply:
> > > avdd-3p3v-supply:
> > > 
> > > It also looks like there is a parallel interface for data, so I would
> > > expect to see an io-backends property that links to the PHY used for
> > > handling that.
> > > 
> > Ultimately yes, but the parallel interface might require some decisions on
> > binding that are non obvious until it's actually implemented. So maybe
> > don't need that bit from the start.  The rest I agree should be here.
> > 
> > 
> 
> Since the driver patch uses a DMA channel that isn't documented here,
> I am assuming that the parallel interface is being used so we do need
> to consider it now. :-)

AFAIU, the way this is designed, the parallel interface is directly connected to
the DMA IP controller. So, at this point there's really no use for the DAC
backend. We may have different designs in the future but no use case for now.

- Nuno Sá

RE: [PATCH 1/2] dt-bindings: iio: dac: add docs for ad8460

[Tinaco, Mariel]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, May 12, 2024 12:25 AM
> To: David Lechner <dlechner@baylibre.com>
> Cc: Tinaco, Mariel <Mariel.Tinaco@analog.com>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Lars-Peter Clausen
> <lars@metafoo.de>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski
> <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Liam Girdwood
> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Marcelo Schmitt
> <marcelo.schmitt1@gmail.com>; Dimitri Fedrau <dima.fedrau@gmail.com>;
> Guenter Roeck <linux@roeck-us.net>
> Subject: Re: [PATCH 1/2] dt-bindings: iio: dac: add docs for ad8460
> 
> [External]
> 
> On Fri, 10 May 2024 12:28:19 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com>
> wrote:
> > >
> > > This adds the bindings documentation for the 14-bit High Voltage,
> > > High Current, Waveform Generator Digital-to-Analog converter.
> > >
> > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> > > ---
> > >  .../bindings/iio/dac/adi,ad8460.yaml          | 67 +++++++++++++++++++
> > >  MAINTAINERS                                   |  7 ++
> > >  2 files changed, 74 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > > b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > > new file mode 100644
> > > index 000000000..924f76209
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
> > > @@ -0,0 +1,67 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright
> > > +2024 Analog Devices Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id:
> > > +https://urldefense.com/v3/__http://devicetree.org/schemas/iio/dac/a
> > >
> +di,ad8460.yaml*__;Iw!!A3Ni8CS0y2Y!6gGs7KbAH3BGqwLcGM0Yihd9U4zdncT
> ai
> > > +eQzg6e_e5cN4fl2NnTYjro4r3D2bqynKuOHDTnLw-cBN74x$
> > > +$schema:
> > > +https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core
> > >
> +.yaml*__;Iw!!A3Ni8CS0y2Y!6gGs7KbAH3BGqwLcGM0Yihd9U4zdncTaieQzg6e_
> e5
> > > +cN4fl2NnTYjro4r3D2bqynKuOHDTnLw11zc2OH$
> > > +
> > > +title: Analog Devices AD8460 DAC
> > > +
> > > +maintainers:
> > > +  - Mariel Tinaco <mariel.tinaco@analog.com>
> > > +
> > > +description: |
> > > +  Analog Devices AD8460 110 V High Voltage, 1 A High Current,
> > > +  Arbitrary Waveform Generator with Integrated 14-Bit High Speed
> > > +DAC
> > > +
> > > +https://www.analog.com/media/en/technical-documentation/data-sheets
> > > +/ad8460.pdf
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - adi,ad8460
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  spi-max-frequency:
> > > +    maximum: 20000000
> > > +
> > > +  vref-supply:
> >
> > It would be nice to make the property name match the pin name since
> > there is more than one reference voltage input.
> >
> > refio-1p2v-supply:
> >
> > > +    description: Drive voltage in the range of 1.2V maximum to as low as
> > > +      low as 0.12V through the REF_IO pin to adjust full scale
> > > + output span
> >
> > I don't seen anything in the datasheet named REF_IO. Is this a typo
> > and it should be REFIO_1P2V?
> >
> > > +
> > > +  clocks:
> > > +    description: The clock for the DAC. This is the sync clock
> > > +
> > > +  adi,rset-ohms:
> > > +    description: Specify value of external resistor connected to FS_ADJ pin
> > > +      to establish internal HVDAC's reference current I_REF
> > > +    minimum: 2000
> > > +    maximum: 20000
> > > +
> >
> > I see lots more pins on the datasheet, many of which should be trivial
> > to add bindings for (we prefer to have the bindings as complete as
> > possible even if the driver doesn't implement everything). Potential
> > candidates:
> >
> > sdn-reset-gpios: (active high)
> > reset-gpios: (active low)
> > sdn-io-gpios: (active high)
> >
> > hvcc-supply:
> > hvee-supply:
> > vcc-5v-supply:
> > vref-5v-supply:
> > dvdd-3p3v-supply:
> > avdd-3p3v-supply:
> >
> > It also looks like there is a parallel interface for data, so I would
> > expect to see an io-backends property that links to the PHY used for
> > handling that.
> >
> Ultimately yes, but the parallel interface might require some decisions on binding
> that are non obvious until it's actually implemented. So maybe don't need that
> bit from the start.  The rest I agree should be here.
> 

Added these parameters to the device tree bindings. Not necessarily to the driver

> >
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +
> > > +    spi {
> > > +        dac@0 {
> > > +            compatible = "adi,ad8460";
> > > +            reg = <0>;
> > > +            spi-max-frequency = <8000000>;
> > > +            adi,rset-ohms = <2000>;
> > > +
> > > +            vref-supply = <&vrefio>;
> > > +            clocks = <&sync_ext_clk>;
> > > +        };
> > > +    };
> > > +
> > > +...
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 758c202ec..dae93df2a 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -1234,6 +1234,13 @@ W:       https://ez.analog.com/linux-software-
> drivers
> > >  F:     Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> > >  F:     drivers/iio/adc/ad7780.c
> > >
> > > +ANALOG DEVICES INC AD8460 DRIVER
> > > +M:     Mariel Tinaco <Mariel.Tinaco@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,ad8460.yaml
> > > +
> > >  ANALOG DEVICES INC AD9739a DRIVER
> > >  M:     Nuno Sa <nuno.sa@analog.com>
> > >  M:     Dragos Bogdan <dragos.bogdan@analog.com>
> > > --
> > > 2.34.1
> > >
> > >

[PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Mariel Tinaco]

The AD8460 is a “bits in, power out” high voltage, high-power,
highspeed driver optimized for large output current (up to ±1 A)
and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V)
into capacitive loads.

A digital engine implements user-configurable features: modes for
digital input, programmable supply current, and fault monitoring
and programmable protection settings for output current,
output voltage, and junction temperature. The AD8460 operates on
high voltage dual supplies up to ±55 V and a single low voltage
supply of 5 V.

Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
---
 MAINTAINERS              |   1 +
 drivers/iio/dac/Kconfig  |  13 +
 drivers/iio/dac/Makefile |   1 +
 drivers/iio/dac/ad8460.c | 652 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 667 insertions(+)
 create mode 100644 drivers/iio/dac/ad8460.c

diff --git a/MAINTAINERS b/MAINTAINERS
index dae93df2a..6134cac65 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1240,6 +1240,7 @@ L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
+F:	drivers/iio/dac/ad8460.c
 
 ANALOG DEVICES INC AD9739a DRIVER
 M:	Nuno Sa <nuno.sa@analog.com>
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 3c2bf620f..8da5cfe4f 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -300,6 +300,19 @@ config AD7303
 	  To compile this driver as module choose M here: the module will be called
 	  ad7303.
 
+config AD8460
+	tristate "Analog Devices AD8460 DAC driver"
+	depends on SPI
+	select REGMAP_SPI
+	select IIO_BUFFER
+	select IIO_BUFFER_DMAENGINE
+	help
+	  Say yes here to build support for Analog Devices AD8460 Digital to
+	  Analog Converters (DAC).
+
+	  To compile this driver as a module choose M here: the module will be called
+	  ad8460.
+
 config AD8801
 	tristate "Analog Devices AD8801/AD8803 DAC driver"
 	depends on SPI_MASTER
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 8432a81a1..0fa2849e1 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_AD5686_SPI) += ad5686-spi.o
 obj-$(CONFIG_AD5696_I2C) += ad5696-i2c.o
 obj-$(CONFIG_AD7293) += ad7293.o
 obj-$(CONFIG_AD7303) += ad7303.o
+obj-$(CONFIG_AD8460) += ad8460.o
 obj-$(CONFIG_AD8801) += ad8801.o
 obj-$(CONFIG_AD9739A) += ad9739a.o
 obj-$(CONFIG_ADI_AXI_DAC) += adi-axi-dac.o
diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c
new file mode 100644
index 000000000..4049be0f5
--- /dev/null
+++ b/drivers/iio/dac/ad8460.c
@@ -0,0 +1,652 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AD8460 Waveform generator DAC Driver
+ *
+ * Copyright (C) 2024 Analog Devices, Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/clk.h>
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/gpio/consumer.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/buffer-dma.h>
+#include <linux/iio/buffer-dmaengine.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#define AD8460_CTRL_REG(x)                      (x)
+#define AD8460_HVDAC_DATA_WORD_LOW(x)           (0x60 + (2 * (x)))
+#define AD8460_HVDAC_DATA_WORD_HIGH(x)          (0x61 + (2 * (x)))
+
+#define AD8460_HV_RESET_MSK                     BIT(7)
+#define AD8460_HV_SLEEP_MSK                     BIT(4)
+#define AD8460_WAVE_GEN_MODE_MSK                BIT(0)
+
+#define AD8460_HVDAC_SLEEP_MSK                  BIT(3)
+
+#define AD8460_APG_MODE_ENABLE_MSK              BIT(5)
+#define AD8460_PATTERN_DEPTH_MSK                GENMASK(3, 0)
+
+#define AD8460_SHUTDOWN_FLAG_MSK                BIT(7)
+#define AD8460_DATA_BYTE_LOW_MSK                GENMASK(7, 0)
+#define AD8460_DATA_BYTE_HIGH_MSK               GENMASK(5, 0)
+
+#define AD8460_NOMINAL_VOLTAGE_SPAN		80
+#define AD8460_MIN_VREFIO_UV			120000
+#define AD8460_MAX_VREFIO_UV			1200000
+#define	AD8460_MIN_RSET_OHMS			2000
+#define	AD8460_MAX_RSET_OHMS			20000
+
+struct ad8460_state {
+	struct spi_device *spi;
+	struct regmap *regmap;
+	struct clk *sync_clk;
+	/* lock to protect against multiple access to the device and shared data */
+	struct mutex lock;
+	u32 cache_apg_idx;
+	u32 rset_ohms;
+	int vref_mv;
+};
+
+static int ad8460_hv_reset(struct ad8460_state *state)
+{
+	int ret;
+
+	ret = regmap_update_bits(state->regmap,	AD8460_CTRL_REG(0x00),
+				 AD8460_HV_RESET_MSK,
+				 FIELD_PREP(AD8460_HV_RESET_MSK, 1));
+	if (ret)
+		return ret;
+
+	fsleep(20);
+
+	return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00),
+				  AD8460_HV_RESET_MSK,
+				  FIELD_PREP(AD8460_HV_RESET_MSK, 0));
+}
+
+static int ad8460_reset(const struct ad8460_state *state)
+{
+	struct device *dev = &state->spi->dev;
+	struct gpio_desc *reset;
+
+	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(reset))
+		return dev_err_probe(dev, PTR_ERR(reset),
+				     "Failed to get reset gpio");
+	if (reset) {
+		/* minimum duration of 10ns */
+		ndelay(10);
+		gpiod_set_value_cansleep(reset, 1);
+		return 0;
+	}
+
+	/* bring all registers to their default state */
+	return regmap_write(state->regmap, AD8460_CTRL_REG(0x03), 1);
+}
+
+static int ad8460_enable_apg_mode(struct ad8460_state *state, int val)
+{
+	int ret;
+
+	ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x02),
+				 AD8460_APG_MODE_ENABLE_MSK,
+				 FIELD_PREP(AD8460_APG_MODE_ENABLE_MSK, val));
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00),
+				  AD8460_WAVE_GEN_MODE_MSK,
+				  FIELD_PREP(AD8460_WAVE_GEN_MODE_MSK, val));
+}
+
+static int ad8460_read_shutdown_flag(struct ad8460_state *state, u64 *flag)
+{
+	int ret, val;
+
+	ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x0E), &val);
+	if (ret)
+		return ret;
+
+	*flag = FIELD_GET(AD8460_SHUTDOWN_FLAG_MSK, val);
+	return 0;
+}
+
+static ssize_t ad8460_read_powerdown(struct iio_dev *indio_dev,
+				     uintptr_t private,
+				     const struct iio_chan_spec *chan,
+				     char *buf)
+{
+	struct ad8460_state *state = iio_priv(indio_dev);
+	unsigned int reg;
+	int ret;
+
+	ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x01), &reg);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_HVDAC_SLEEP_MSK, reg));
+}
+
+static ssize_t ad8460_write_powerdown(struct iio_dev *indio_dev,
+				      uintptr_t private,
+				      const struct iio_chan_spec *chan,
+				      const char *buf,
+				      size_t len)
+{
+	struct ad8460_state *state = iio_priv(indio_dev);
+	bool pwr_down;
+	u64 sdn_flag;
+	int ret;
+
+	ret = kstrtobool(buf, &pwr_down);
+	if (ret)
+		return ret;
+
+	guard(mutex)(&state->lock);
+
+	ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
+				 AD8460_HVDAC_SLEEP_MSK,
+				 FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, pwr_down));
+	if (ret)
+		return ret;
+
+	if (!pwr_down) {
+		ret = ad8460_read_shutdown_flag(state, &sdn_flag);
+		if (ret)
+			return ret;
+
+		if (sdn_flag) {
+			ret = ad8460_hv_reset(state);
+			if (ret)
+				return ret;
+		}
+	}
+
+	ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00),
+				 AD8460_HV_SLEEP_MSK,
+				 FIELD_PREP(AD8460_HV_SLEEP_MSK, !pwr_down));
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static const char * const ad8460_powerdown_modes[] = {
+	"three_state",
+};
+
+static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan)
+{
+	return 0;
+}
+
+static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     unsigned int type)
+{
+	return 0;
+}
+
+static int ad8460_get_hvdac_byte(struct ad8460_state *state,
+				 int index,
+				 int *val)
+{
+	unsigned int high, low;
+	int ret;
+
+	ret = regmap_read(state->regmap, AD8460_HVDAC_DATA_WORD_HIGH(index),
+			  &high);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index),
+			  &low);
+	if (ret)
+		return ret;
+
+	*val = FIELD_GET(AD8460_DATA_BYTE_HIGH_MSK, high) << 8 | low;
+
+	return ret;
+}
+
+static int ad8460_set_hvdac_byte(struct ad8460_state *state,
+				 int index,
+				 int val)
+{
+	int ret;
+
+	ret = regmap_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index),
+			   (val & 0xFF));
+	if (ret)
+		return ret;
+
+	return regmap_write(state->regmap, AD8460_HVDAC_DATA_WORD_HIGH(index),
+			    ((val >> 8) & 0xFF));
+}
+
+static int ad8460_set_sample(struct ad8460_state *state, int val)
+{
+	int ret;
+
+	ret = ad8460_enable_apg_mode(state, 1);
+	if (ret)
+		return ret;
+
+	guard(mutex)(&state->lock);
+	ret = ad8460_set_hvdac_byte(state, 0, val);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(state->regmap,
+				  AD8460_CTRL_REG(0x02),
+				  AD8460_PATTERN_DEPTH_MSK,
+				  FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, 0));
+}
+
+static int ad8460_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
+			    long mask)
+{
+	struct ad8460_state *state = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+			return ad8460_set_sample(state, val);
+		unreachable();
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad8460_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long mask)
+{
+	struct ad8460_state *state = iio_priv(indio_dev);
+	unsigned int num, denom;
+	int data, ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		scoped_guard(mutex, &state->lock) {
+			ret = ad8460_get_hvdac_byte(state, 0, &data);
+			if (ret)
+				return ret;
+		}
+		*val = data;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = clk_get_rate(state->sync_clk);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		/* vCONV = vNOMINAL_SPAN * (DAC_CODE / 2**14) - 40V
+		 * vMAX  = vNOMINAL_SPAN * (2**14 / 2**14) - 40V
+		 * vMIN  = vNOMINAL_SPAN * (0 / 2**14) - 40V
+		 * vADJ  = vCONV * (2000 / rSET) * (vREF / 1.2)
+		 * vSPAN = vADJ_MAX - vADJ_MIN
+		 * See datasheet page 49, section FULL-SCALE REDUCTION
+		 */
+		num = AD8460_NOMINAL_VOLTAGE_SPAN * 2000 * state->vref_mv;
+		denom = state->rset_ohms * 1200;
+		*val = DIV_ROUND_CLOSEST_ULL(num, denom);
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad8460_reg_access(struct iio_dev *indio_dev,
+			     unsigned int reg, unsigned int writeval,
+			     unsigned int *readval)
+{
+	struct ad8460_state *state = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(state->regmap, reg, readval);
+
+	return regmap_write(state->regmap, reg, writeval);
+}
+
+static int ad8460_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct ad8460_state *state = iio_priv(indio_dev);
+
+	return ad8460_enable_apg_mode(state, 0);
+}
+
+static int ad8460_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct ad8460_state *state = iio_priv(indio_dev);
+
+	return ad8460_enable_apg_mode(state, 1);
+}
+
+static const struct iio_buffer_setup_ops ad8460_buffer_setup_ops = {
+	.preenable = &ad8460_buffer_preenable,
+	.postdisable = &ad8460_buffer_postdisable,
+};
+
+static const struct iio_info ad8460_info = {
+	.read_raw = &ad8460_read_raw,
+	.write_raw = &ad8460_write_raw,
+	.debugfs_reg_access = &ad8460_reg_access,
+};
+
+static const struct iio_enum ad8460_powerdown_mode_enum = {
+	.items = ad8460_powerdown_modes,
+	.num_items = ARRAY_SIZE(ad8460_powerdown_modes),
+	.get = ad8460_get_powerdown_mode,
+	.set = ad8460_set_powerdown_mode,
+};
+
+#define AD8460_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) {	\
+	.name = _name,							\
+	.read = (_read),						\
+	.write = (_write),						\
+	.private = (_what),						\
+	.shared = (_shared),						\
+}
+
+static struct iio_chan_spec_ext_info ad8460_ext_info[] = {
+	AD8460_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
+			     ad8460_read_powerdown, ad8460_write_powerdown),
+	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad8460_powerdown_mode_enum),
+	IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
+			   &ad8460_powerdown_mode_enum),
+	{}
+};
+
+#define AD8460_ALTVOLTAGE_CHAN(_chan) {				\
+	.type = IIO_ALTVOLTAGE,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ) |	\
+			      BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),	\
+	.output = 1,						\
+	.indexed = 1,						\
+	.channel = (_chan),					\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = 14,					\
+		.storagebits = 16,				\
+		.endianness = IIO_LE,				\
+	},                                                      \
+	.ext_info = ad8460_ext_info,                            \
+}
+
+static const struct iio_chan_spec ad8460_channels[] = {
+	AD8460_ALTVOLTAGE_CHAN(0),
+};
+
+static const struct regmap_config ad8460_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x7F,
+};
+
+static void ad8460_regulator_disable(void *data)
+{
+	regulator_disable(data);
+}
+
+static int ad8460_set_apg_pattern_depth(void *arg, u64 val)
+{
+	struct iio_dev *indio_dev = arg;
+
+	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+		struct ad8460_state *state = iio_priv(indio_dev);
+
+		return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x02),
+					  AD8460_PATTERN_DEPTH_MSK,
+					  FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, val));
+	}
+	unreachable();
+}
+
+static int ad8460_show_apg_pattern_depth(void *arg, u64 *val)
+{
+	struct iio_dev *indio_dev = arg;
+	struct ad8460_state *state = iio_priv(indio_dev);
+	unsigned int reg;
+	int ret;
+
+	ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), &reg);
+	if (ret)
+		return ret;
+
+	*val = FIELD_GET(AD8460_PATTERN_DEPTH_MSK, reg);
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ad8460_apg_pattern_depth_fops,
+			 ad8460_show_apg_pattern_depth,
+			 ad8460_set_apg_pattern_depth, "%llu\n");
+
+static ssize_t ad8460_apg_pattern_memory_write(struct file *file,
+					       const char __user *userbuf,
+					       size_t count, loff_t *ppos)
+{
+	struct iio_dev *indio_dev = file->private_data;
+	struct ad8460_state *state = iio_priv(indio_dev);
+	unsigned int reg;
+	char data[16];
+	int ret, val;
+
+	ret = simple_write_to_buffer(data, sizeof(data) - 1, ppos,
+				     userbuf, count);
+	if (ret <= 0)
+		return ret;
+
+	ret = sscanf(data, "%i 0x%X", &reg, &val);
+
+	switch (ret) {
+	case 1:
+		state->cache_apg_idx = reg;
+		break;
+	case 2:
+		state->cache_apg_idx = reg;
+		scoped_guard(mutex, &state->lock) {
+			ret = ad8460_set_hvdac_byte(state, reg, val);
+			if (ret) {
+				dev_err(indio_dev->dev.parent, "%s: write failed\n",
+					__func__);
+				return ret;
+			}
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return count;
+}
+
+static ssize_t ad8460_apg_pattern_memory_read(struct file *file,
+					      char __user *userbuf,
+					      size_t count, loff_t *ppos)
+{
+	struct iio_dev *indio_dev = file->private_data;
+	struct ad8460_state *state = iio_priv(indio_dev);
+	int ret, val;
+	char data[16];
+
+	scoped_guard(mutex, &state->lock) {
+		ret = ad8460_get_hvdac_byte(state, state->cache_apg_idx, &val);
+		if (ret)
+			return ret;
+	}
+
+	ret = scnprintf(data, sizeof(data), "%i 0x%X\n", state->cache_apg_idx, val);
+
+	return simple_read_from_buffer(userbuf, count, ppos, data, ret);
+}
+
+static const struct file_operations ad8460_apg_pattern_memory_fops = {
+	.open = simple_open,
+	.read = ad8460_apg_pattern_memory_read,
+	.write = ad8460_apg_pattern_memory_write,
+};
+
+static int ad8460_show_shutdown_flag(void *arg, u64 *val)
+{
+	struct iio_dev *indio_dev = arg;
+	struct ad8460_state *state = iio_priv(indio_dev);
+
+	return ad8460_read_shutdown_flag(state, val);
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ad8460_shutdown_flag_fops, ad8460_show_shutdown_flag,
+			 NULL, "%llu\n");
+
+static void ad8460_debugfs_init(struct iio_dev *indio_dev)
+{
+	struct dentry *d = iio_get_debugfs_dentry(indio_dev);
+
+	if (!IS_ENABLED(CONFIG_DEBUG_FS))
+		return;
+
+	debugfs_create_file_unsafe("apg_pattern_depth", 0600, d,
+				   indio_dev, &ad8460_apg_pattern_depth_fops);
+	debugfs_create_file_unsafe("shutdown_flag", 0600, d,
+				   indio_dev, &ad8460_shutdown_flag_fops);
+	debugfs_create_file("apg_pattern_memory", 0644, d,
+			    indio_dev, &ad8460_apg_pattern_memory_fops);
+}
+
+static int ad8460_probe(struct spi_device *spi)
+{
+	struct ad8460_state *state;
+	struct iio_dev *indio_dev;
+	struct regulator *vrefio;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	state = iio_priv(indio_dev);
+	mutex_init(&state->lock);
+
+	state->spi = spi;
+
+	state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
+	if (IS_ERR(state->regmap))
+		return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
+				     "Failed to initialize regmap");
+
+	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev, "tx",
+						  IIO_BUFFER_DIRECTION_OUT);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret,
+				     "Failed to get DMA buffer\n");
+
+	state->sync_clk = devm_clk_get_enabled(&spi->dev, "sync_clk");
+	if (IS_ERR(state->sync_clk))
+		return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk),
+				     "Failed to get sync clk\n");
+
+	vrefio = devm_regulator_get_optional(&spi->dev, "vrefio");
+	if (IS_ERR(vrefio)) {
+		if (PTR_ERR(vrefio) != -ENODEV)
+			return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
+					     "Failed to get vref regulator\n");
+
+		state->vref_mv = 1200;
+
+	} else {
+		ret = regulator_enable(vrefio);
+		if (ret)
+			return dev_err_probe(&spi->dev, ret,
+					     "Failed to enable vrefio regulator\n");
+
+		ret = devm_add_action_or_reset(&spi->dev,
+					       ad8460_regulator_disable,
+					       vrefio);
+		if (ret)
+			return ret;
+
+		ret = regulator_get_voltage(vrefio);
+		if (ret < 0)
+			return dev_err_probe(&spi->dev, ret,
+					     "Failed to get ref voltage\n");
+
+		if (!in_range(ret, AD8460_MIN_VREFIO_UV, AD8460_MAX_VREFIO_UV))
+			return dev_err_probe(&spi->dev, -EINVAL,
+					     "Invalid ref voltage range(%u) [%u, %u]\n",
+					     ret, AD8460_MIN_VREFIO_UV,
+					     AD8460_MAX_VREFIO_UV);
+
+		state->vref_mv = ret / 1000;
+	}
+
+	ret = device_property_read_u32(&spi->dev, "adi,rset-ohms",
+				       &state->rset_ohms);
+	if (!ret) {
+		if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS,
+			      AD8460_MAX_RSET_OHMS))
+			return dev_err_probe(&spi->dev, -EINVAL,
+					     "Invalid resistor set range(%u) [%u, %u]\n",
+					     state->rset_ohms,
+					     AD8460_MIN_RSET_OHMS,
+					     AD8460_MAX_RSET_OHMS);
+	}
+
+	ret = ad8460_reset(state);
+	if (ret)
+		return ret;
+
+	/* Enables DAC by default */
+	ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
+				 AD8460_HVDAC_SLEEP_MSK,
+				 FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0));
+	if (ret)
+		return ret;
+
+	indio_dev->name = "ad8460";
+	indio_dev->info = &ad8460_info;
+	indio_dev->channels = ad8460_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_HARDWARE;
+	indio_dev->setup_ops = &ad8460_buffer_setup_ops;
+
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret)
+		return ret;
+
+	ad8460_debugfs_init(indio_dev);
+
+	return 0;
+}
+
+static const struct of_device_id ad8460_of_match[] = {
+	{ .compatible = "adi, ad8460" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ad8460_of_match);
+
+static struct spi_driver ad8460_driver = {
+	.driver = {
+		.name = "ad8460",
+		.of_match_table = ad8460_of_match,
+	},
+	.probe = ad8460_probe,
+};
+module_spi_driver(ad8460_driver);
+
+MODULE_AUTHOR("Mariel Tinaco <mariel.tinaco@analog.com");
+MODULE_DESCRIPTION("AD8460 DAC driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_DMAENGINE_BUFFER);
-- 
2.34.1

Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[David Lechner]

On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
>
> The AD8460 is a “bits in, power out” high voltage, high-power,
> highspeed driver optimized for large output current (up to ±1 A)

high-speed

> and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V)
> into capacitive loads.
>
> A digital engine implements user-configurable features: modes for
> digital input, programmable supply current, and fault monitoring
> and programmable protection settings for output current,
> output voltage, and junction temperature. The AD8460 operates on
> high voltage dual supplies up to ±55 V and a single low voltage
> supply of 5 V.
>
> Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> ---

...

> diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c
> new file mode 100644
> index 000000000..4049be0f5
> --- /dev/null
> +++ b/drivers/iio/dac/ad8460.c

...

> +static int ad8460_probe(struct spi_device *spi)
> +{
> +       struct ad8460_state *state;
> +       struct iio_dev *indio_dev;
> +       struct regulator *vrefio;
> +       int ret;
> +
> +       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +       if (!indio_dev)
> +               return -ENOMEM;
> +
> +       state = iio_priv(indio_dev);
> +       mutex_init(&state->lock);
> +
> +       state->spi = spi;
> +
> +       state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
> +       if (IS_ERR(state->regmap))
> +               return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> +                                    "Failed to initialize regmap");
> +
> +       ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev, "tx",
> +                                                 IIO_BUFFER_DIRECTION_OUT);
> +       if (ret)
> +               return dev_err_probe(&spi->dev, ret,
> +                                    "Failed to get DMA buffer\n");

It looks like the dmas property is missing in the DT bindings.
However, as I suggested in my comments on that patch, it might make
more sense to implement the parallel data part as an IIO backend. I
assume this is using one of ADI's FPGA IP blocks to get the data?

> +
> +       state->sync_clk = devm_clk_get_enabled(&spi->dev, "sync_clk");

The DT bindings don't specify clock-names, which is perfectly fine and
perhaps even preferred since there is only one clock. But that means
the ID here should be NULL instead of "sync_clk".

> +       if (IS_ERR(state->sync_clk))
> +               return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk),
> +                                    "Failed to get sync clk\n");
> +


> +       vrefio = devm_regulator_get_optional(&spi->dev, "vrefio");
> +       if (IS_ERR(vrefio)) {
> +               if (PTR_ERR(vrefio) != -ENODEV)
> +                       return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
> +                                            "Failed to get vref regulator\n");
> +
> +               state->vref_mv = 1200;
> +
> +       } else {
> +               ret = regulator_enable(vrefio);
> +               if (ret)
> +                       return dev_err_probe(&spi->dev, ret,
> +                                            "Failed to enable vrefio regulator\n");
> +
> +               ret = devm_add_action_or_reset(&spi->dev,
> +                                              ad8460_regulator_disable,
> +                                              vrefio);
> +               if (ret)
> +                       return ret;
> +
> +               ret = regulator_get_voltage(vrefio);
> +               if (ret < 0)
> +                       return dev_err_probe(&spi->dev, ret,
> +                                            "Failed to get ref voltage\n");
> +
> +               if (!in_range(ret, AD8460_MIN_VREFIO_UV, AD8460_MAX_VREFIO_UV))
> +                       return dev_err_probe(&spi->dev, -EINVAL,
> +                                            "Invalid ref voltage range(%u) [%u, %u]\n",
> +                                            ret, AD8460_MIN_VREFIO_UV,
> +                                            AD8460_MAX_VREFIO_UV);
> +
> +               state->vref_mv = ret / 1000;
> +       }

FYI, if all goes well, starting with kernel 6.10-rc1 (we'll have to
wait a few weeks for this), this regulator section can be simplified
to:

ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vrefio");
if (ret == -ENODEV) {
    /* no supply, using internal 1.2V reference */
    state->vref_mv = 1200;
} else if (ret < 0) {
    return  dev_err_probe(&spi->dev, ret, "Failed to get reference voltage\n");
} else {
    /* external reference */
    state->vref_mv = ret / 1000;
}

if (!in_range(...

We can always fix it up later though if you don't want to wait. :-)

> +
> +       ret = device_property_read_u32(&spi->dev, "adi,rset-ohms",
> +                                      &state->rset_ohms);

Since 0 isn't an allowable value for R_SET, it seems like we need to
return on error here or assign a default value if the property is
missing. If we do a default value, the DT bindings need to be updated
to reflect that as well.

> +       if (!ret) {
> +               if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS,
> +                             AD8460_MAX_RSET_OHMS))
> +                       return dev_err_probe(&spi->dev, -EINVAL,
> +                                            "Invalid resistor set range(%u) [%u, %u]\n",
> +                                            state->rset_ohms,
> +                                            AD8460_MIN_RSET_OHMS,
> +                                            AD8460_MAX_RSET_OHMS);
> +       }
> +
> +       ret = ad8460_reset(state);
> +       if (ret)
> +               return ret;
> +
> +       /* Enables DAC by default */
> +       ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),

I was going to suggest giving names to the registers instead of using
the number, but the datasheet doesn't do that! :-(

Oh well, I guess this is the best we can do.

> +                                AD8460_HVDAC_SLEEP_MSK,
> +                                FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0));
> +       if (ret)
> +               return ret;
> +
> +       indio_dev->name = "ad8460";
> +       indio_dev->info = &ad8460_info;
> +       indio_dev->channels = ad8460_channels;
> +       indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
> +       indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_HARDWARE;

devm_iio_dmaengine_buffer_setup_ext() already sets the
INDIO_BUFFER_HARDWARE flag so this is redundant. Also,
devm_iio_dmaengine_buffer_setup_ext() is usually called here rather
than early in probe because it needs a few things in indio_dev
populated, IIRC.

> +       indio_dev->setup_ops = &ad8460_buffer_setup_ops;
> +
> +       ret = devm_iio_device_register(&spi->dev, indio_dev);
> +       if (ret)
> +               return ret;
> +
> +       ad8460_debugfs_init(indio_dev);
> +
> +       return 0;
> +}
> +

...

Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Jonathan Cameron]

On Fri, 10 May 2024 14:40:53 +0800
Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:

> The AD8460 is a “bits in, power out” high voltage, high-power,
> highspeed driver optimized for large output current (up to ±1 A)
> and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V)
> into capacitive loads.
> 
> A digital engine implements user-configurable features: modes for
> digital input, programmable supply current, and fault monitoring
> and programmable protection settings for output current,
> output voltage, and junction temperature. The AD8460 operates on
> high voltage dual supplies up to ±55 V and a single low voltage
> supply of 5 V.
> 
> Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>

I'd like to see some ABI docs for the debugfs interface.
The device is unusual enough that a general intro document or a lot
more in the series cover letter would be useful.

I'm not sure what the dmaengine usage in here is doing for example?
Driving the parallel bus perhaps?  David was correct that the binding
should reflect that part as well. I was assuming you'd only implemented
the spi part.

How to handle the pattern generator is also an interesting question.
That probably wants a version of the symbol interfaces we use for PSK
and similar.  We did have some DDS drivers a long time back in staging
but they only did a few fixed waveforms so this is breaking new ground.

Having looked a little at the datasheet, we may want to hard code
some limits - or get them from DT. Probably a bit too easy to set things
on fire otherwise.

Also, seems that this is only partly implemented. Please make that
clear in the patch description.  Perhaps this should have been an RFC
with a list of questions?

Jonathan

> diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c
> new file mode 100644
> index 000000000..4049be0f5
> --- /dev/null
> +++ b/drivers/iio/dac/ad8460.c
> @@ -0,0 +1,652 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AD8460 Waveform generator DAC Driver
> + *
> + * Copyright (C) 2024 Analog Devices, Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/buffer-dma.h>
> +#include <linux/iio/buffer-dmaengine.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#define AD8460_CTRL_REG(x)                      (x)
> +#define AD8460_HVDAC_DATA_WORD_LOW(x)           (0x60 + (2 * (x)))
> +#define AD8460_HVDAC_DATA_WORD_HIGH(x)          (0x61 + (2 * (x)))
> +
> +#define AD8460_HV_RESET_MSK                     BIT(7)
> +#define AD8460_HV_SLEEP_MSK                     BIT(4)
> +#define AD8460_WAVE_GEN_MODE_MSK                BIT(0)
> +
> +#define AD8460_HVDAC_SLEEP_MSK                  BIT(3)
> +
> +#define AD8460_APG_MODE_ENABLE_MSK              BIT(5)
> +#define AD8460_PATTERN_DEPTH_MSK                GENMASK(3, 0)
> +
> +#define AD8460_SHUTDOWN_FLAG_MSK                BIT(7)
> +#define AD8460_DATA_BYTE_LOW_MSK                GENMASK(7, 0)
> +#define AD8460_DATA_BYTE_HIGH_MSK               GENMASK(5, 0)
> +
> +#define AD8460_NOMINAL_VOLTAGE_SPAN		80
> +#define AD8460_MIN_VREFIO_UV			120000
> +#define AD8460_MAX_VREFIO_UV			1200000
> +#define	AD8460_MIN_RSET_OHMS			2000
> +#define	AD8460_MAX_RSET_OHMS			20000
> +
> +struct ad8460_state {
> +	struct spi_device *spi;
> +	struct regmap *regmap;
> +	struct clk *sync_clk;
> +	/* lock to protect against multiple access to the device and shared data */
> +	struct mutex lock;
> +	u32 cache_apg_idx;
> +	u32 rset_ohms;
> +	int vref_mv;
> +};
> +

> +
> +static const char * const ad8460_powerdown_modes[] = {
> +	"three_state",
> +};
> +
> +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan)
> +{
> +	return 0;

Why have the stubs in here?

> +}
> +
> +static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     unsigned int type)
> +{
> +	return 0;
> +}
> +
> +static int ad8460_get_hvdac_byte(struct ad8460_state *state,

byte? Seems to be getting a 14 bit value.

> +				 int index,
> +				 int *val)
> +{
> +	unsigned int high, low;
> +	int ret;
> +
> +	ret = regmap_read(state->regmap, AD8460_HVDAC_DATA_WORD_HIGH(index),
> +			  &high);

Use a bulk read? Then byteswap if necessary and mask the result.

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index),
> +			  &low);
> +	if (ret)
> +		return ret;
> +
> +	*val = FIELD_GET(AD8460_DATA_BYTE_HIGH_MSK, high) << 8 | low;
> +
> +	return ret;
> +}
> +
> +static int ad8460_set_hvdac_byte(struct ad8460_state *state,
> +				 int index,
> +				 int val)
> +{
> +	int ret;
> +
> +	ret = regmap_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index),
> +			   (val & 0xFF));
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(state->regmap, AD8460_HVDAC_DATA_WORD_HIGH(index),
> +			    ((val >> 8) & 0xFF));

bulk write? or do these need to be ordered?

> +}
> +
> +static int ad8460_set_sample(struct ad8460_state *state, int val)
> +{
> +	int ret;
> +
> +	ret = ad8460_enable_apg_mode(state, 1);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&state->lock);
> +	ret = ad8460_set_hvdac_byte(state, 0, val);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(state->regmap,
> +				  AD8460_CTRL_REG(0x02),
> +				  AD8460_PATTERN_DEPTH_MSK,
> +				  FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, 0));
> +}
> +
> +static int ad8460_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val,
> +			    int val2,
> +			    long mask)
> +{
> +	struct ad8460_state *state = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> +			return ad8460_set_sample(state, val);
> +		unreachable();
> +	default:
> +		return -EINVAL;
> +	}
> +}



> +static int ad8460_probe(struct spi_device *spi)
> +{
> +	struct ad8460_state *state;
> +	struct iio_dev *indio_dev;
> +	struct regulator *vrefio;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	state = iio_priv(indio_dev);
> +	mutex_init(&state->lock);
> +
> +	state->spi = spi;
> +
> +	state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
> +	if (IS_ERR(state->regmap))
> +		return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> +				     "Failed to initialize regmap");
> +
> +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev, "tx",
> +						  IIO_BUFFER_DIRECTION_OUT);

Ah. I take back my binding comment. I assume this is mapping some non standard
interface for the parallel data flow?

> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "Failed to get DMA buffer\n");
> +
> +	state->sync_clk = devm_clk_get_enabled(&spi->dev, "sync_clk");
> +	if (IS_ERR(state->sync_clk))
> +		return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk),
> +				     "Failed to get sync clk\n");
> +
> +	vrefio = devm_regulator_get_optional(&spi->dev, "vrefio");
> +	if (IS_ERR(vrefio)) {
> +		if (PTR_ERR(vrefio) != -ENODEV)
> +			return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
> +					     "Failed to get vref regulator\n");
> +
> +		state->vref_mv = 1200;
> +
> +	} else {
> +		ret = regulator_enable(vrefio);
> +		if (ret)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Failed to enable vrefio regulator\n");
> +
> +		ret = devm_add_action_or_reset(&spi->dev,
> +					       ad8460_regulator_disable,
> +					       vrefio);
> +		if (ret)
> +			return ret;
> +
> +		ret = regulator_get_voltage(vrefio);
> +		if (ret < 0)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Failed to get ref voltage\n");
> +
> +		if (!in_range(ret, AD8460_MIN_VREFIO_UV, AD8460_MAX_VREFIO_UV))
> +			return dev_err_probe(&spi->dev, -EINVAL,
> +					     "Invalid ref voltage range(%u) [%u, %u]\n",
> +					     ret, AD8460_MIN_VREFIO_UV,
> +					     AD8460_MAX_VREFIO_UV);
> +
> +		state->vref_mv = ret / 1000;
> +	}
> +
> +	ret = device_property_read_u32(&spi->dev, "adi,rset-ohms",
> +				       &state->rset_ohms);
> +	if (!ret) {
> +		if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS,
> +			      AD8460_MAX_RSET_OHMS))
> +			return dev_err_probe(&spi->dev, -EINVAL,
> +					     "Invalid resistor set range(%u) [%u, %u]\n",
> +					     state->rset_ohms,
> +					     AD8460_MIN_RSET_OHMS,
> +					     AD8460_MAX_RSET_OHMS);
> +	}
> +
> +	ret = ad8460_reset(state);
> +	if (ret)
> +		return ret;
> +
> +	/* Enables DAC by default */
> +	ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
> +				 AD8460_HVDAC_SLEEP_MSK,
> +				 FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0));
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = "ad8460";
> +	indio_dev->info = &ad8460_info;
> +	indio_dev->channels = ad8460_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_HARDWARE;
> +	indio_dev->setup_ops = &ad8460_buffer_setup_ops;
> +
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ad8460_debugfs_init(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ad8460_of_match[] = {
> +	{ .compatible = "adi, ad8460" },
> +	{ },
No comma on 'null' terminators like this as we don't want to let
people put anything after them.

> +};
> +MODULE_DEVICE_TABLE(of, ad8460_of_match);

RE: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Tinaco, Mariel]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, May 12, 2024 12:44 AM
> To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring
> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> <broonie@kernel.org>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> 
> [External]
> 
> On Fri, 10 May 2024 14:40:53 +0800
> Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
> 
> > The AD8460 is a “bits in, power out” high voltage, high-power,
> > highspeed driver optimized for large output current (up to ±1 A) and
> > high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) into
> > capacitive loads.
> >
> > A digital engine implements user-configurable features: modes for
> > digital input, programmable supply current, and fault monitoring and
> > programmable protection settings for output current, output voltage,
> > and junction temperature. The AD8460 operates on high voltage dual
> > supplies up to ±55 V and a single low voltage supply of 5 V.
> >
> > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> 
> I'd like to see some ABI docs for the debugfs interface.
> The device is unusual enough that a general intro document or a lot more in the
> series cover letter would be useful.
> 
> I'm not sure what the dmaengine usage in here is doing for example?
> Driving the parallel bus perhaps?  David was correct that the binding should
> reflect that part as well. I was assuming you'd only implemented the spi part.
> 
> How to handle the pattern generator is also an interesting question.
> That probably wants a version of the symbol interfaces we use for PSK and
> similar.  We did have some DDS drivers a long time back in staging but they only
> did a few fixed waveforms so this is breaking new ground.
> 
> Having looked a little at the datasheet, we may want to hard code some limits -
> or get them from DT. Probably a bit too easy to set things on fire otherwise.
> 
> Also, seems that this is only partly implemented. Please make that clear in the
> patch description.  Perhaps this should have been an RFC with a list of
> questions?
> 
> Jonathan
> 
> > diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c new
> > file mode 100644 index 000000000..4049be0f5
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad8460.c
> > @@ -0,0 +1,652 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * AD8460 Waveform generator DAC Driver
> > + *
> > + * Copyright (C) 2024 Analog Devices, Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/clk.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/buffer-dma.h>
> > +#include <linux/iio/buffer-dmaengine.h> #include <linux/iio/iio.h>
> > +#include <linux/module.h> #include <linux/mod_devicetable.h> #include
> > +<linux/regmap.h> #include <linux/regulator/consumer.h> #include
> > +<linux/spi/spi.h>
> > +
> > +#define AD8460_CTRL_REG(x)                      (x)
> > +#define AD8460_HVDAC_DATA_WORD_LOW(x)           (0x60 + (2 * (x)))
> > +#define AD8460_HVDAC_DATA_WORD_HIGH(x)          (0x61 + (2 * (x)))
> > +
> > +#define AD8460_HV_RESET_MSK                     BIT(7)
> > +#define AD8460_HV_SLEEP_MSK                     BIT(4)
> > +#define AD8460_WAVE_GEN_MODE_MSK                BIT(0)
> > +
> > +#define AD8460_HVDAC_SLEEP_MSK                  BIT(3)
> > +
> > +#define AD8460_APG_MODE_ENABLE_MSK              BIT(5)
> > +#define AD8460_PATTERN_DEPTH_MSK                GENMASK(3, 0)
> > +
> > +#define AD8460_SHUTDOWN_FLAG_MSK                BIT(7)
> > +#define AD8460_DATA_BYTE_LOW_MSK                GENMASK(7, 0)
> > +#define AD8460_DATA_BYTE_HIGH_MSK               GENMASK(5, 0)
> > +
> > +#define AD8460_NOMINAL_VOLTAGE_SPAN		80
> > +#define AD8460_MIN_VREFIO_UV			120000
> > +#define AD8460_MAX_VREFIO_UV			1200000
> > +#define	AD8460_MIN_RSET_OHMS			2000
> > +#define	AD8460_MAX_RSET_OHMS			20000
> > +
> > +struct ad8460_state {
> > +	struct spi_device *spi;
> > +	struct regmap *regmap;
> > +	struct clk *sync_clk;
> > +	/* lock to protect against multiple access to the device and shared data
> */
> > +	struct mutex lock;
> > +	u32 cache_apg_idx;
> > +	u32 rset_ohms;
> > +	int vref_mv;
> > +};
> > +
> 
> > +
> > +static const char * const ad8460_powerdown_modes[] = {
> > +	"three_state",
> > +};
> > +
> > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> > +				     const struct iio_chan_spec *chan) {
> > +	return 0;
> 
> Why have the stubs in here?

Should I move the stubs to a different place in the code or remove them
altogether since there is only a single powerdown mode available

> > +}
> > +
> > +static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev,
> > +				     const struct iio_chan_spec *chan,
> > +				     unsigned int type)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int ad8460_get_hvdac_byte(struct ad8460_state *state,
> 
> byte? Seems to be getting a 14 bit value.
> 
> > +				 int index,
> > +				 int *val)
> > +{
> > +	unsigned int high, low;
> > +	int ret;
> > +
> > +	ret = regmap_read(state->regmap,
> AD8460_HVDAC_DATA_WORD_HIGH(index),
> > +			  &high);

I replaced the name with hvdac data word instead like in the datasheet

> Use a bulk read? Then byteswap if necessary and mask the result.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read(state->regmap,
> AD8460_HVDAC_DATA_WORD_LOW(index),
> > +			  &low);
> > +	if (ret)
> > +		return ret;
> > +
> > +	*val = FIELD_GET(AD8460_DATA_BYTE_HIGH_MSK, high) << 8 | low;
> > +
> > +	return ret;
> > +}
> > +
> > +static int ad8460_set_hvdac_byte(struct ad8460_state *state,
> > +				 int index,
> > +				 int val)
> > +{
> > +	int ret;
> > +
> > +	ret = regmap_write(state->regmap,
> AD8460_HVDAC_DATA_WORD_LOW(index),
> > +			   (val & 0xFF));
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_write(state->regmap,
> AD8460_HVDAC_DATA_WORD_HIGH(index),
> > +			    ((val >> 8) & 0xFF));
> 
> bulk write? or do these need to be ordered?

For this I used bulk read/write this way.

static int ad8460_set_hvdac_word(struct ad8460_state *state,
				 int index,
				 int val)
{
	u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH];

	regvals[0] = val & 0xFF;
	regvals[1] = (val >> 8) & 0xFF;

	return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index),
				 regvals, AD8460_DATA_BYTE_WORD_LENGTH);
}


> > +}
> > +
> > +static int ad8460_set_sample(struct ad8460_state *state, int val) {
> > +	int ret;
> > +
> > +	ret = ad8460_enable_apg_mode(state, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	guard(mutex)(&state->lock);
> > +	ret = ad8460_set_hvdac_byte(state, 0, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_update_bits(state->regmap,
> > +				  AD8460_CTRL_REG(0x02),
> > +				  AD8460_PATTERN_DEPTH_MSK,
> > +				  FIELD_PREP(AD8460_PATTERN_DEPTH_MSK,
> 0)); }
> > +
> > +static int ad8460_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int val,
> > +			    int val2,
> > +			    long mask)
> > +{
> > +	struct ad8460_state *state = iio_priv(indio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> > +			return ad8460_set_sample(state, val);
> > +		unreachable();
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> 
> 
> 
> > +static int ad8460_probe(struct spi_device *spi) {
> > +	struct ad8460_state *state;
> > +	struct iio_dev *indio_dev;
> > +	struct regulator *vrefio;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	state = iio_priv(indio_dev);
> > +	mutex_init(&state->lock);
> > +
> > +	state->spi = spi;
> > +
> > +	state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
> > +	if (IS_ERR(state->regmap))
> > +		return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> > +				     "Failed to initialize regmap");
> > +
> > +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev, "tx",
> > +
> IIO_BUFFER_DIRECTION_OUT);
> 
> Ah. I take back my binding comment. I assume this is mapping some non
> standard interface for the parallel data flow?

Yes, the HDL side doesn't follow yet the standard IIO backend from which this
driver was tested

> > +	if (ret)
> > +		return dev_err_probe(&spi->dev, ret,
> > +				     "Failed to get DMA buffer\n");
> > +
> > +	state->sync_clk = devm_clk_get_enabled(&spi->dev, "sync_clk");
> > +	if (IS_ERR(state->sync_clk))
> > +		return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk),
> > +				     "Failed to get sync clk\n");
> > +
> > +	vrefio = devm_regulator_get_optional(&spi->dev, "vrefio");
> > +	if (IS_ERR(vrefio)) {
> > +		if (PTR_ERR(vrefio) != -ENODEV)
> > +			return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
> > +					     "Failed to get vref regulator\n");
> > +
> > +		state->vref_mv = 1200;
> > +
> > +	} else {
> > +		ret = regulator_enable(vrefio);
> > +		if (ret)
> > +			return dev_err_probe(&spi->dev, ret,
> > +					     "Failed to enable vrefio
> regulator\n");
> > +
> > +		ret = devm_add_action_or_reset(&spi->dev,
> > +					       ad8460_regulator_disable,
> > +					       vrefio);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regulator_get_voltage(vrefio);
> > +		if (ret < 0)
> > +			return dev_err_probe(&spi->dev, ret,
> > +					     "Failed to get ref voltage\n");
> > +
> > +		if (!in_range(ret, AD8460_MIN_VREFIO_UV,
> AD8460_MAX_VREFIO_UV))
> > +			return dev_err_probe(&spi->dev, -EINVAL,
> > +					     "Invalid ref voltage range(%u) [%u,
> %u]\n",
> > +					     ret, AD8460_MIN_VREFIO_UV,
> > +					     AD8460_MAX_VREFIO_UV);
> > +
> > +		state->vref_mv = ret / 1000;
> > +	}
> > +
> > +	ret = device_property_read_u32(&spi->dev, "adi,rset-ohms",
> > +				       &state->rset_ohms);
> > +	if (!ret) {
> > +		if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS,
> > +			      AD8460_MAX_RSET_OHMS))
> > +			return dev_err_probe(&spi->dev, -EINVAL,
> > +					     "Invalid resistor set range(%u) [%u,
> %u]\n",
> > +					     state->rset_ohms,
> > +					     AD8460_MIN_RSET_OHMS,
> > +					     AD8460_MAX_RSET_OHMS);
> > +	}
> > +
> > +	ret = ad8460_reset(state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Enables DAC by default */
> > +	ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
> > +				 AD8460_HVDAC_SLEEP_MSK,
> > +				 FIELD_PREP(AD8460_HVDAC_SLEEP_MSK,
> 0));
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->name = "ad8460";
> > +	indio_dev->info = &ad8460_info;
> > +	indio_dev->channels = ad8460_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE |
> INDIO_BUFFER_HARDWARE;
> > +	indio_dev->setup_ops = &ad8460_buffer_setup_ops;
> > +
> > +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ad8460_debugfs_init(indio_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ad8460_of_match[] = {
> > +	{ .compatible = "adi, ad8460" },
> > +	{ },
> No comma on 'null' terminators like this as we don't want to let people put
> anything after them.
> 
> > +};
> > +MODULE_DEVICE_TABLE(of, ad8460_of_match);

Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Jonathan Cameron]

> > > +};
> > > +
> > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> > > +				     const struct iio_chan_spec *chan) {
> > > +	return 0;  
> > 
> > Why have the stubs in here?  
> 
> Should I move the stubs to a different place in the code or remove them
> altogether since there is only a single powerdown mode available
Ah. I'd not really understood what was going on here.  This is fine as
is.

> > AD8460_HVDAC_DATA_WORD_HIGH(index),  
> > > +			    ((val >> 8) & 0xFF));  
> > 
> > bulk write? or do these need to be ordered?  
> 
> For this I used bulk read/write this way.
> 
> static int ad8460_set_hvdac_word(struct ad8460_state *state,
> 				 int index,
> 				 int val)
> {
> 	u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH];
regmap bulk accesses (when spi anyway) should be provided with DMA safe buffers.
Easiest way to do that is add one with __aligned(IIO_DMA_MINALIGN) to the end
of the ad8460_state structure.  Possibly you'll need a lock to protect it - I haven't
checked.
> 
> 	regvals[0] = val & 0xFF;
> 	regvals[1] = (val >> 8) & 0xFF;

That is an endian conversion so use appropriate endian function to fill it efficiently
and document clearly what is going on.


	put_unaligned_le16()

> 
> 	return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index),
> 				 regvals, AD8460_DATA_BYTE_WORD_LENGTH);
> }
> 
> 
> > > +}

> > > +	state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
> > > +	if (IS_ERR(state->regmap))
> > > +		return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> > > +				     "Failed to initialize regmap");
> > > +
> > > +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev, "tx",
> > > +  
> > IIO_BUFFER_DIRECTION_OUT);
> > 
> > Ah. I take back my binding comment. I assume this is mapping some non
> > standard interface for the parallel data flow?  
> 
> Yes, the HDL side doesn't follow yet the standard IIO backend from which this
> driver was tested

Hmm. I'd like to see this brought inline with the other iio backend
drivers if possible.

Jonathan

RE: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Tinaco, Mariel]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, June 29, 2024 2:46 AM
> To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring
> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> <broonie@kernel.org>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> 
> [External]
> 
> > > > +};
> > > > +
> > > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> > > > +				     const struct iio_chan_spec *chan) {
> > > > +	return 0;
> > >
> > > Why have the stubs in here?
> >
> > Should I move the stubs to a different place in the code or remove
> > them altogether since there is only a single powerdown mode available
> Ah. I'd not really understood what was going on here.  This is fine as is.
> 
> > > AD8460_HVDAC_DATA_WORD_HIGH(index),
> > > > +			    ((val >> 8) & 0xFF));
> > >
> > > bulk write? or do these need to be ordered?
> >
> > For this I used bulk read/write this way.
> >
> > static int ad8460_set_hvdac_word(struct ad8460_state *state,
> > 				 int index,
> > 				 int val)
> > {
> > 	u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH];
> regmap bulk accesses (when spi anyway) should be provided with DMA safe
> buffers.
> Easiest way to do that is add one with __aligned(IIO_DMA_MINALIGN) to the
> end of the ad8460_state structure.  Possibly you'll need a lock to protect it - I
> haven't checked.
> >
> > 	regvals[0] = val & 0xFF;
> > 	regvals[1] = (val >> 8) & 0xFF;
> 
> That is an endian conversion so use appropriate endian function to fill it
> efficiently and document clearly what is going on.
> 
> 
> 	put_unaligned_le16()
> 
> >
> > 	return regmap_bulk_write(state->regmap,
> AD8460_HVDAC_DATA_WORD_LOW(index),
> > 				 regvals,
> AD8460_DATA_BYTE_WORD_LENGTH); }
> >
> >
> > > > +}
> 
> > > > +	state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
> > > > +	if (IS_ERR(state->regmap))
> > > > +		return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> > > > +				     "Failed to initialize regmap");
> > > > +
> > > > +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev,
> > > > +"tx",
> > > > +
> > > IIO_BUFFER_DIRECTION_OUT);
> > >
> > > Ah. I take back my binding comment. I assume this is mapping some
> > > non standard interface for the parallel data flow?
> >
> > Yes, the HDL side doesn't follow yet the standard IIO backend from
> > which this driver was tested
> 
> Hmm. I'd like to see this brought inline with the other iio backend drivers if
> possible.

Does this mean that we would need to implement an AXI IP core on the
FPGA side to be able to test this?

> 
> Jonathan
> 

Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Jonathan Cameron]

On Mon, 8 Jul 2024 05:17:55 +0000
"Tinaco, Mariel" <Mariel.Tinaco@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, June 29, 2024 2:46 AM
> > To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring
> > <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> > <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> > <broonie@kernel.org>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> > Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> > <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> > Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> > 
> > [External]
> >   
> > > > > +};
> > > > > +
> > > > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> > > > > +				     const struct iio_chan_spec *chan) {
> > > > > +	return 0;  
> > > >
> > > > Why have the stubs in here?  
> > >
> > > Should I move the stubs to a different place in the code or remove
> > > them altogether since there is only a single powerdown mode available  
> > Ah. I'd not really understood what was going on here.  This is fine as is.
> >   
> > > > AD8460_HVDAC_DATA_WORD_HIGH(index),  
> > > > > +			    ((val >> 8) & 0xFF));  
> > > >
> > > > bulk write? or do these need to be ordered?  
> > >
> > > For this I used bulk read/write this way.
> > >
> > > static int ad8460_set_hvdac_word(struct ad8460_state *state,
> > > 				 int index,
> > > 				 int val)
> > > {
> > > 	u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH];  
> > regmap bulk accesses (when spi anyway) should be provided with DMA safe
> > buffers.
> > Easiest way to do that is add one with __aligned(IIO_DMA_MINALIGN) to the
> > end of the ad8460_state structure.  Possibly you'll need a lock to protect it - I
> > haven't checked.  
> > >
> > > 	regvals[0] = val & 0xFF;
> > > 	regvals[1] = (val >> 8) & 0xFF;  
> > 
> > That is an endian conversion so use appropriate endian function to fill it
> > efficiently and document clearly what is going on.
> > 
> > 
> > 	put_unaligned_le16()
> >   
> > >
> > > 	return regmap_bulk_write(state->regmap,  
> > AD8460_HVDAC_DATA_WORD_LOW(index),  
> > > 				 regvals,  
> > AD8460_DATA_BYTE_WORD_LENGTH); }  
> > >
> > >  
> > > > > +}  
> >   
> > > > > +	state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
> > > > > +	if (IS_ERR(state->regmap))
> > > > > +		return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> > > > > +				     "Failed to initialize regmap");
> > > > > +
> > > > > +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev,
> > > > > +"tx",
> > > > > +  
> > > > IIO_BUFFER_DIRECTION_OUT);
> > > >
> > > > Ah. I take back my binding comment. I assume this is mapping some
> > > > non standard interface for the parallel data flow?  
> > >
> > > Yes, the HDL side doesn't follow yet the standard IIO backend from
> > > which this driver was tested  
> > 
> > Hmm. I'd like to see this brought inline with the other iio backend drivers if
> > possible.  
> 
> Does this mean that we would need to implement an AXI IP core on the
> FPGA side to be able to test this?

Don't think so.  That framework is meant to support any equivalent IP.
So whatever you have should be supportable. Maybe it's somewhat of a stub
driver though if there isn't anything controllable.

It's Nuno's area of expertise though +CC.

> 
> > 
> > Jonathan
> >   
> 
> 

Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Nuno Sá]

On Mon, 2024-07-08 at 17:05 +0100, Jonathan Cameron wrote:
> On Mon, 8 Jul 2024 05:17:55 +0000
> "Tinaco, Mariel" <Mariel.Tinaco@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Saturday, June 29, 2024 2:46 AM
> > > To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring
> > > <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> > > <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> > > <broonie@kernel.org>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> > > Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> > > <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> > > Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> > > 
> > > [External]
> > >   
> > > > > > +};
> > > > > > +
> > > > > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> > > > > > +				     const struct iio_chan_spec *chan) {
> > > > > > +	return 0;  
> > > > > 
> > > > > Why have the stubs in here?  
> > > > 
> > > > Should I move the stubs to a different place in the code or remove
> > > > them altogether since there is only a single powerdown mode available  
> > > Ah. I'd not really understood what was going on here.  This is fine as is.
> > >   
> > > > > AD8460_HVDAC_DATA_WORD_HIGH(index),  
> > > > > > +			    ((val >> 8) & 0xFF));  
> > > > > 
> > > > > bulk write? or do these need to be ordered?  
> > > > 
> > > > For this I used bulk read/write this way.
> > > > 
> > > > static int ad8460_set_hvdac_word(struct ad8460_state *state,
> > > > 				 int index,
> > > > 				 int val)
> > > > {
> > > > 	u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH];  
> > > regmap bulk accesses (when spi anyway) should be provided with DMA safe
> > > buffers.
> > > Easiest way to do that is add one with __aligned(IIO_DMA_MINALIGN) to the
> > > end of the ad8460_state structure.  Possibly you'll need a lock to protect it -
> > > I
> > > haven't checked.  
> > > > 
> > > > 	regvals[0] = val & 0xFF;
> > > > 	regvals[1] = (val >> 8) & 0xFF;  
> > > 
> > > That is an endian conversion so use appropriate endian function to fill it
> > > efficiently and document clearly what is going on.
> > > 
> > > 
> > > 	put_unaligned_le16()
> > >   
> > > > 
> > > > 	return regmap_bulk_write(state->regmap,  
> > > AD8460_HVDAC_DATA_WORD_LOW(index),  
> > > > 				 regvals,  
> > > AD8460_DATA_BYTE_WORD_LENGTH); }  
> > > > 
> > > >  
> > > > > > +}  
> > >   
> > > > > > +	state->regmap = devm_regmap_init_spi(spi,
> > > > > > &ad8460_regmap_config);
> > > > > > +	if (IS_ERR(state->regmap))
> > > > > > +		return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> > > > > > +				     "Failed to initialize regmap");
> > > > > > +
> > > > > > +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev,
> > > > > > +"tx",
> > > > > > +  
> > > > > IIO_BUFFER_DIRECTION_OUT);
> > > > > 
> > > > > Ah. I take back my binding comment. I assume this is mapping some
> > > > > non standard interface for the parallel data flow?  
> > > > 
> > > > Yes, the HDL side doesn't follow yet the standard IIO backend from
> > > > which this driver was tested  
> > > 
> > > Hmm. I'd like to see this brought inline with the other iio backend drivers if
> > > possible.  
> > 
> > Does this mean that we would need to implement an AXI IP core on the
> > FPGA side to be able to test this?
> 
> Don't think so.  That framework is meant to support any equivalent IP.
> So whatever you have should be supportable. Maybe it's somewhat of a stub
> driver though if there isn't anything controllable.
> 
> It's Nuno's area of expertise though +CC.
> 

Hi Jonathan,

Yeah, I did reply David (IIRC) about the very same question. In the design/HW Mariel
is working on the DAC is directly connected to the DMA core which is handled already
by a proper dma controller driver. So in this case I'm really not seeing the backend
need right now (maybe in the future we may have another design for this device that
could justify for a backend device but no idea on that).

As you mention, we could very well do a stub platform driver so we can use the
backend framework (like dma-backend or something) that could pretty much be a stub
for the DMA controller. But is it worth it though? We'd actually be "lying" in terms
of HW description as the DMA is a property of the actual converter.

- Nuno Sá

Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[David Lechner]

On 7/11/24 4:20 AM, Nuno Sá wrote:
> On Mon, 2024-07-08 at 17:05 +0100, Jonathan Cameron wrote:
>> On Mon, 8 Jul 2024 05:17:55 +0000
>> "Tinaco, Mariel" <Mariel.Tinaco@analog.com> wrote:
>>
>>>> -----Original Message-----
>>>> From: Jonathan Cameron <jic23@kernel.org>
>>>> Sent: Saturday, June 29, 2024 2:46 AM
>>>> To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
>>>> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
>>>> kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring
>>>> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
>>>> <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
>>>> <broonie@kernel.org>; Hennerich, Michael <Michael.Hennerich@analog.com>;
>>>> Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
>>>> <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
>>>> Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
>>>>
>>>> [External]
>>>>   
>>>>>>> +};
>>>>>>> +
>>>>>>> +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
>>>>>>> +				     const struct iio_chan_spec *chan) {
>>>>>>> +	return 0;  
>>>>>>
>>>>>> Why have the stubs in here?  
>>>>>
>>>>> Should I move the stubs to a different place in the code or remove
>>>>> them altogether since there is only a single powerdown mode available  
>>>> Ah. I'd not really understood what was going on here.  This is fine as is.
>>>>   
>>>>>> AD8460_HVDAC_DATA_WORD_HIGH(index),  
>>>>>>> +			    ((val >> 8) & 0xFF));  
>>>>>>
>>>>>> bulk write? or do these need to be ordered?  
>>>>>
>>>>> For this I used bulk read/write this way.
>>>>>
>>>>> static int ad8460_set_hvdac_word(struct ad8460_state *state,
>>>>> 				 int index,
>>>>> 				 int val)
>>>>> {
>>>>> 	u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH];  
>>>> regmap bulk accesses (when spi anyway) should be provided with DMA safe
>>>> buffers.
>>>> Easiest way to do that is add one with __aligned(IIO_DMA_MINALIGN) to the
>>>> end of the ad8460_state structure.  Possibly you'll need a lock to protect it -
>>>> I
>>>> haven't checked.  
>>>>>
>>>>> 	regvals[0] = val & 0xFF;
>>>>> 	regvals[1] = (val >> 8) & 0xFF;  
>>>>
>>>> That is an endian conversion so use appropriate endian function to fill it
>>>> efficiently and document clearly what is going on.
>>>>
>>>>
>>>> 	put_unaligned_le16()
>>>>   
>>>>>
>>>>> 	return regmap_bulk_write(state->regmap,  
>>>> AD8460_HVDAC_DATA_WORD_LOW(index),  
>>>>> 				 regvals,  
>>>> AD8460_DATA_BYTE_WORD_LENGTH); }  
>>>>>
>>>>>  
>>>>>>> +}  
>>>>   
>>>>>>> +	state->regmap = devm_regmap_init_spi(spi,
>>>>>>> &ad8460_regmap_config);
>>>>>>> +	if (IS_ERR(state->regmap))
>>>>>>> +		return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
>>>>>>> +				     "Failed to initialize regmap");
>>>>>>> +
>>>>>>> +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev,
>>>>>>> +"tx",
>>>>>>> +  
>>>>>> IIO_BUFFER_DIRECTION_OUT);
>>>>>>
>>>>>> Ah. I take back my binding comment. I assume this is mapping some
>>>>>> non standard interface for the parallel data flow?  
>>>>>
>>>>> Yes, the HDL side doesn't follow yet the standard IIO backend from
>>>>> which this driver was tested  
>>>>
>>>> Hmm. I'd like to see this brought inline with the other iio backend drivers if
>>>> possible.  
>>>
>>> Does this mean that we would need to implement an AXI IP core on the
>>> FPGA side to be able to test this?
>>
>> Don't think so.  That framework is meant to support any equivalent IP.
>> So whatever you have should be supportable. Maybe it's somewhat of a stub
>> driver though if there isn't anything controllable.
>>
>> It's Nuno's area of expertise though +CC.
>>
> 
> Hi Jonathan,
> 
> Yeah, I did reply David (IIRC) about the very same question. In the design/HW Mariel
> is working on the DAC is directly connected to the DMA core which is handled already
> by a proper dma controller driver. So in this case I'm really not seeing the backend
> need right now (maybe in the future we may have another design for this device that
> could justify for a backend device but no idea on that).
> 
> As you mention, we could very well do a stub platform driver so we can use the
> backend framework (like dma-backend or something) that could pretty much be a stub
> for the DMA controller. But is it worth it though? We'd actually be "lying" in terms
> of HW description as the DMA is a property of the actual converter.
> 
> - Nuno Sá
> 
> 

I'm a bit inclined to agree with Jonathan here. I could see someone in the future,
wanting to, e.g., use DMA + a GPIO controller for the parallel interface if they
didn't have an FPGA. So it seems a bit more future-proof to just always use the
IIO backend framework for the parallel interface.

FWIW, I don't think it would be "lying" since the io-backend DT node would be
representing physical parallel bus between the DMA controller and the ADC
chip.

But if DT maintainers are OK with the idea that a DMA channel can be directly
wired to an external chip, I guess I won't complain. :-)

Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Nuno Sá]

On Thu, 2024-07-11 at 16:31 -0500, David Lechner wrote:
> On 7/11/24 4:20 AM, Nuno Sá wrote:
> > On Mon, 2024-07-08 at 17:05 +0100, Jonathan Cameron wrote:
> > > On Mon, 8 Jul 2024 05:17:55 +0000
> > > "Tinaco, Mariel" <Mariel.Tinaco@analog.com> wrote:
> > > 
> > > > > -----Original Message-----
> > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > Sent: Saturday, June 29, 2024 2:46 AM
> > > > > To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> > > > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > > > > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob
> > > > > Herring
> > > > > <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor
> > > > > Dooley
> > > > > <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> > > > > <broonie@kernel.org>; Hennerich, Michael
> > > > > <Michael.Hennerich@analog.com>;
> > > > > Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> > > > > <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> > > > > Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> > > > > 
> > > > > [External]
> > > > >   
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> > > > > > > > +				     const struct iio_chan_spec
> > > > > > > > *chan) {
> > > > > > > > +	return 0;  
> > > > > > > 
> > > > > > > Why have the stubs in here?  
> > > > > > 
> > > > > > Should I move the stubs to a different place in the code or remove
> > > > > > them altogether since there is only a single powerdown mode
> > > > > > available  
> > > > > Ah. I'd not really understood what was going on here.  This is fine as
> > > > > is.
> > > > >   
> > > > > > > AD8460_HVDAC_DATA_WORD_HIGH(index),  
> > > > > > > > +			    ((val >> 8) & 0xFF));  
> > > > > > > 
> > > > > > > bulk write? or do these need to be ordered?  
> > > > > > 
> > > > > > For this I used bulk read/write this way.
> > > > > > 
> > > > > > static int ad8460_set_hvdac_word(struct ad8460_state *state,
> > > > > > 				 int index,
> > > > > > 				 int val)
> > > > > > {
> > > > > > 	u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH];  
> > > > > regmap bulk accesses (when spi anyway) should be provided with DMA
> > > > > safe
> > > > > buffers.
> > > > > Easiest way to do that is add one with __aligned(IIO_DMA_MINALIGN) to
> > > > > the
> > > > > end of the ad8460_state structure.  Possibly you'll need a lock to
> > > > > protect it -
> > > > > I
> > > > > haven't checked.  
> > > > > > 
> > > > > > 	regvals[0] = val & 0xFF;
> > > > > > 	regvals[1] = (val >> 8) & 0xFF;  
> > > > > 
> > > > > That is an endian conversion so use appropriate endian function to
> > > > > fill it
> > > > > efficiently and document clearly what is going on.
> > > > > 
> > > > > 
> > > > > 	put_unaligned_le16()
> > > > >   
> > > > > > 
> > > > > > 	return regmap_bulk_write(state->regmap,  
> > > > > AD8460_HVDAC_DATA_WORD_LOW(index),  
> > > > > > 				 regvals,  
> > > > > AD8460_DATA_BYTE_WORD_LENGTH); }  
> > > > > > 
> > > > > >  
> > > > > > > > +}  
> > > > >   
> > > > > > > > +	state->regmap = devm_regmap_init_spi(spi,
> > > > > > > > &ad8460_regmap_config);
> > > > > > > > +	if (IS_ERR(state->regmap))
> > > > > > > > +		return dev_err_probe(&spi->dev, PTR_ERR(state-
> > > > > > > > >regmap),
> > > > > > > > +				     "Failed to initialize
> > > > > > > > regmap");
> > > > > > > > +
> > > > > > > > +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev,
> > > > > > > > indio_dev,
> > > > > > > > +"tx",
> > > > > > > > +  
> > > > > > > IIO_BUFFER_DIRECTION_OUT);
> > > > > > > 
> > > > > > > Ah. I take back my binding comment. I assume this is mapping some
> > > > > > > non standard interface for the parallel data flow?  
> > > > > > 
> > > > > > Yes, the HDL side doesn't follow yet the standard IIO backend from
> > > > > > which this driver was tested  
> > > > > 
> > > > > Hmm. I'd like to see this brought inline with the other iio backend
> > > > > drivers if
> > > > > possible.  
> > > > 
> > > > Does this mean that we would need to implement an AXI IP core on the
> > > > FPGA side to be able to test this?
> > > 
> > > Don't think so.  That framework is meant to support any equivalent IP.
> > > So whatever you have should be supportable. Maybe it's somewhat of a stub
> > > driver though if there isn't anything controllable.
> > > 
> > > It's Nuno's area of expertise though +CC.
> > > 
> > 
> > Hi Jonathan,
> > 
> > Yeah, I did reply David (IIRC) about the very same question. In the
> > design/HW Mariel
> > is working on the DAC is directly connected to the DMA core which is handled
> > already
> > by a proper dma controller driver. So in this case I'm really not seeing the
> > backend
> > need right now (maybe in the future we may have another design for this
> > device that
> > could justify for a backend device but no idea on that).
> > 
> > As you mention, we could very well do a stub platform driver so we can use
> > the
> > backend framework (like dma-backend or something) that could pretty much be
> > a stub
> > for the DMA controller. But is it worth it though? We'd actually be "lying"
> > in terms
> > of HW description as the DMA is a property of the actual converter.
> > 
> > - Nuno Sá
> > 
> > 
> 
> I'm a bit inclined to agree with Jonathan here. I could see someone in the
> future,
> wanting to, e.g., use DMA + a GPIO controller for the parallel interface if
> they
> didn't have an FPGA. So it seems a bit more future-proof to just always use
> the
> IIO backend framework for the parallel interface.

I do agree it's more future but guessing usecases is not something I tend to
like much (often just results in code that never gets __really__ used). We can
very well take care of it when a usecase pops up and we have an actual device
that can be represented by a backend :).

> 
> FWIW, I don't think it would be "lying" since the io-backend DT node would be
> representing physical parallel bus between the DMA controller and the ADC
> chip.

To me, it's really a stretch having a backend with the only reason (op) of
requesting the DMA channel. I still think you're pushing to much and going
around with wording to justify for the DMA property :). The parallel bus is part
of the DAC and directly connects to the DMA data lines so it really looks to me
the dma is a property of the actual DAC.

That said and Mariel can help here, I did not really looked into the design
myself and I'm just stating (or what I understood) what Mariel told me. But if
there's some other piece of HW sitting between the DMA and the bus then it would
be easier for me to agree even if we don't have any real control over that
device.

> But if DT maintainers are OK with the idea that a DMA channel can be directly
> wired to an external chip, I guess I won't complain. :-)

That's the case in here so I don't see why it should be a problem :). It's the
same with the axi-dac/adc. It's all inside the FPGA but different cores/IPS.

FWIW, I'm ok if we go the backend direction even if I don't fully agree with it
(at least with the understanding I have so far about the design). I definitely
want to see more users of it but I also don't think it should be a rule for any
fairly high speed converter to have a backend associated.

- Nuno Sá

Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Jonathan Cameron]

On Fri, 12 Jul 2024 08:57:00 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Thu, 2024-07-11 at 16:31 -0500, David Lechner wrote:
> > On 7/11/24 4:20 AM, Nuno Sá wrote:  
> > > On Mon, 2024-07-08 at 17:05 +0100, Jonathan Cameron wrote:  
> > > > On Mon, 8 Jul 2024 05:17:55 +0000
> > > > "Tinaco, Mariel" <Mariel.Tinaco@analog.com> wrote:
> > > >   
> > > > > > -----Original Message-----
> > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > Sent: Saturday, June 29, 2024 2:46 AM
> > > > > > To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> > > > > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > > > > > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob
> > > > > > Herring
> > > > > > <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor
> > > > > > Dooley
> > > > > > <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> > > > > > <broonie@kernel.org>; Hennerich, Michael
> > > > > > <Michael.Hennerich@analog.com>;
> > > > > > Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> > > > > > <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> > > > > > Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> > > > > > 
> > > > > > [External]
> > > > > >     
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> > > > > > > > > +				     const struct iio_chan_spec
> > > > > > > > > *chan) {
> > > > > > > > > +	return 0;    
> > > > > > > > 
> > > > > > > > Why have the stubs in here?    
> > > > > > > 
> > > > > > > Should I move the stubs to a different place in the code or remove
> > > > > > > them altogether since there is only a single powerdown mode
> > > > > > > available    
> > > > > > Ah. I'd not really understood what was going on here.  This is fine as
> > > > > > is.
> > > > > >     
> > > > > > > > AD8460_HVDAC_DATA_WORD_HIGH(index),    
> > > > > > > > > +			    ((val >> 8) & 0xFF));    
> > > > > > > > 
> > > > > > > > bulk write? or do these need to be ordered?    
> > > > > > > 
> > > > > > > For this I used bulk read/write this way.
> > > > > > > 
> > > > > > > static int ad8460_set_hvdac_word(struct ad8460_state *state,
> > > > > > > 				 int index,
> > > > > > > 				 int val)
> > > > > > > {
> > > > > > > 	u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH];    
> > > > > > regmap bulk accesses (when spi anyway) should be provided with DMA
> > > > > > safe
> > > > > > buffers.
> > > > > > Easiest way to do that is add one with __aligned(IIO_DMA_MINALIGN) to
> > > > > > the
> > > > > > end of the ad8460_state structure.  Possibly you'll need a lock to
> > > > > > protect it -
> > > > > > I
> > > > > > haven't checked.    
> > > > > > > 
> > > > > > > 	regvals[0] = val & 0xFF;
> > > > > > > 	regvals[1] = (val >> 8) & 0xFF;    
> > > > > > 
> > > > > > That is an endian conversion so use appropriate endian function to
> > > > > > fill it
> > > > > > efficiently and document clearly what is going on.
> > > > > > 
> > > > > > 
> > > > > > 	put_unaligned_le16()
> > > > > >     
> > > > > > > 
> > > > > > > 	return regmap_bulk_write(state->regmap,    
> > > > > > AD8460_HVDAC_DATA_WORD_LOW(index),    
> > > > > > > 				 regvals,    
> > > > > > AD8460_DATA_BYTE_WORD_LENGTH); }    
> > > > > > > 
> > > > > > >    
> > > > > > > > > +}    
> > > > > >     
> > > > > > > > > +	state->regmap = devm_regmap_init_spi(spi,
> > > > > > > > > &ad8460_regmap_config);
> > > > > > > > > +	if (IS_ERR(state->regmap))
> > > > > > > > > +		return dev_err_probe(&spi->dev, PTR_ERR(state-  
> > > > > > > > > >regmap),  
> > > > > > > > > +				     "Failed to initialize
> > > > > > > > > regmap");
> > > > > > > > > +
> > > > > > > > > +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev,
> > > > > > > > > indio_dev,
> > > > > > > > > +"tx",
> > > > > > > > > +    
> > > > > > > > IIO_BUFFER_DIRECTION_OUT);
> > > > > > > > 
> > > > > > > > Ah. I take back my binding comment. I assume this is mapping some
> > > > > > > > non standard interface for the parallel data flow?    
> > > > > > > 
> > > > > > > Yes, the HDL side doesn't follow yet the standard IIO backend from
> > > > > > > which this driver was tested    
> > > > > > 
> > > > > > Hmm. I'd like to see this brought inline with the other iio backend
> > > > > > drivers if
> > > > > > possible.    
> > > > > 
> > > > > Does this mean that we would need to implement an AXI IP core on the
> > > > > FPGA side to be able to test this?  
> > > > 
> > > > Don't think so.  That framework is meant to support any equivalent IP.
> > > > So whatever you have should be supportable. Maybe it's somewhat of a stub
> > > > driver though if there isn't anything controllable.
> > > > 
> > > > It's Nuno's area of expertise though +CC.
> > > >   
> > > 
> > > Hi Jonathan,
> > > 
> > > Yeah, I did reply David (IIRC) about the very same question. In the
> > > design/HW Mariel
> > > is working on the DAC is directly connected to the DMA core which is handled
> > > already
> > > by a proper dma controller driver. So in this case I'm really not seeing the
> > > backend
> > > need right now (maybe in the future we may have another design for this
> > > device that
> > > could justify for a backend device but no idea on that).
> > > 
> > > As you mention, we could very well do a stub platform driver so we can use
> > > the
> > > backend framework (like dma-backend or something) that could pretty much be
> > > a stub
> > > for the DMA controller. But is it worth it though? We'd actually be "lying"
> > > in terms
> > > of HW description as the DMA is a property of the actual converter.
> > > 
> > > - Nuno Sá
> > > 
> > >   
> > 
> > I'm a bit inclined to agree with Jonathan here. I could see someone in the
> > future,
> > wanting to, e.g., use DMA + a GPIO controller for the parallel interface if
> > they
> > didn't have an FPGA. So it seems a bit more future-proof to just always use
> > the
> > IIO backend framework for the parallel interface.  
> 
> I do agree it's more future but guessing usecases is not something I tend to
> like much (often just results in code that never gets __really__ used). We can
> very well take care of it when a usecase pops up and we have an actual device
> that can be represented by a backend :).
> 
> > 
> > FWIW, I don't think it would be "lying" since the io-backend DT node would be
> > representing physical parallel bus between the DMA controller and the ADC
> > chip.  
> 
> To me, it's really a stretch having a backend with the only reason (op) of
> requesting the DMA channel. I still think you're pushing to much and going
> around with wording to justify for the DMA property :). The parallel bus is part
> of the DAC and directly connects to the DMA data lines so it really looks to me
> the dma is a property of the actual DAC.
> 
> That said and Mariel can help here, I did not really looked into the design
> myself and I'm just stating (or what I understood) what Mariel told me. But if
> there's some other piece of HW sitting between the DMA and the bus then it would
> be easier for me to agree even if we don't have any real control over that
> device.
> 
> > But if DT maintainers are OK with the idea that a DMA channel can be directly
> > wired to an external chip, I guess I won't complain. :-)  
> 
> That's the case in here so I don't see why it should be a problem :). It's the
> same with the axi-dac/adc. It's all inside the FPGA but different cores/IPS.
> 
> FWIW, I'm ok if we go the backend direction even if I don't fully agree with it
> (at least with the understanding I have so far about the design). I definitely
> want to see more users of it but I also don't think it should be a rule for any
> fairly high speed converter to have a backend associated.

ok. So short term, DT bindings that document the dma-channel
and see what the DT maintainers think. Longer term, if things get more
complex, that can become optional and a backend added.

Jonathan

> 
> - Nuno Sá
> 

Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Nuno Sá]

On Sat, 2024-07-13 at 10:57 +0100, Jonathan Cameron wrote:
> On Fri, 12 Jul 2024 08:57:00 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Thu, 2024-07-11 at 16:31 -0500, David Lechner wrote:
> > > On 7/11/24 4:20 AM, Nuno Sá wrote:  
> > > > On Mon, 2024-07-08 at 17:05 +0100, Jonathan Cameron wrote:  
> > > > > On Mon, 8 Jul 2024 05:17:55 +0000
> > > > > "Tinaco, Mariel" <Mariel.Tinaco@analog.com> wrote:
> > > > >   
> > > > > > > -----Original Message-----
> > > > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > > > Sent: Saturday, June 29, 2024 2:46 AM
> > > > > > > To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> > > > > > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > > > > > > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob
> > > > > > > Herring
> > > > > > > <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor
> > > > > > > Dooley
> > > > > > > <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> > > > > > > <broonie@kernel.org>; Hennerich, Michael
> > > > > > > <Michael.Hennerich@analog.com>;
> > > > > > > Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> > > > > > > <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> > > > > > > Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> > > > > > > 
> > > > > > > [External]
> > > > > > >     
> > > > > > > > > > +};
> > > > > > > > > > +
> > > > > > > > > > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> > > > > > > > > > +				     const struct iio_chan_spec
> > > > > > > > > > *chan) {
> > > > > > > > > > +	return 0;    
> > > > > > > > > 
> > > > > > > > > Why have the stubs in here?    
> > > > > > > > 
> > > > > > > > Should I move the stubs to a different place in the code or remove
> > > > > > > > them altogether since there is only a single powerdown mode
> > > > > > > > available    
> > > > > > > Ah. I'd not really understood what was going on here.  This is fine as
> > > > > > > is.
> > > > > > >     
> > > > > > > > > AD8460_HVDAC_DATA_WORD_HIGH(index),    
> > > > > > > > > > +			    ((val >> 8) & 0xFF));    
> > > > > > > > > 
> > > > > > > > > bulk write? or do these need to be ordered?    
> > > > > > > > 
> > > > > > > > For this I used bulk read/write this way.
> > > > > > > > 
> > > > > > > > static int ad8460_set_hvdac_word(struct ad8460_state *state,
> > > > > > > > 				 int index,
> > > > > > > > 				 int val)
> > > > > > > > {
> > > > > > > > 	u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH];    
> > > > > > > regmap bulk accesses (when spi anyway) should be provided with DMA
> > > > > > > safe
> > > > > > > buffers.
> > > > > > > Easiest way to do that is add one with __aligned(IIO_DMA_MINALIGN) to
> > > > > > > the
> > > > > > > end of the ad8460_state structure.  Possibly you'll need a lock to
> > > > > > > protect it -
> > > > > > > I
> > > > > > > haven't checked.    
> > > > > > > > 
> > > > > > > > 	regvals[0] = val & 0xFF;
> > > > > > > > 	regvals[1] = (val >> 8) & 0xFF;    
> > > > > > > 
> > > > > > > That is an endian conversion so use appropriate endian function to
> > > > > > > fill it
> > > > > > > efficiently and document clearly what is going on.
> > > > > > > 
> > > > > > > 
> > > > > > > 	put_unaligned_le16()
> > > > > > >     
> > > > > > > > 
> > > > > > > > 	return regmap_bulk_write(state->regmap,    
> > > > > > > AD8460_HVDAC_DATA_WORD_LOW(index),    
> > > > > > > > 				 regvals,    
> > > > > > > AD8460_DATA_BYTE_WORD_LENGTH); }    
> > > > > > > > 
> > > > > > > >    
> > > > > > > > > > +}    
> > > > > > >     
> > > > > > > > > > +	state->regmap = devm_regmap_init_spi(spi,
> > > > > > > > > > &ad8460_regmap_config);
> > > > > > > > > > +	if (IS_ERR(state->regmap))
> > > > > > > > > > +		return dev_err_probe(&spi->dev, PTR_ERR(state-  
> > > > > > > > > > > regmap),  
> > > > > > > > > > +				     "Failed to initialize
> > > > > > > > > > regmap");
> > > > > > > > > > +
> > > > > > > > > > +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev,
> > > > > > > > > > indio_dev,
> > > > > > > > > > +"tx",
> > > > > > > > > > +    
> > > > > > > > > IIO_BUFFER_DIRECTION_OUT);
> > > > > > > > > 
> > > > > > > > > Ah. I take back my binding comment. I assume this is mapping some
> > > > > > > > > non standard interface for the parallel data flow?    
> > > > > > > > 
> > > > > > > > Yes, the HDL side doesn't follow yet the standard IIO backend from
> > > > > > > > which this driver was tested    
> > > > > > > 
> > > > > > > Hmm. I'd like to see this brought inline with the other iio backend
> > > > > > > drivers if
> > > > > > > possible.    
> > > > > > 
> > > > > > Does this mean that we would need to implement an AXI IP core on the
> > > > > > FPGA side to be able to test this?  
> > > > > 
> > > > > Don't think so.  That framework is meant to support any equivalent IP.
> > > > > So whatever you have should be supportable. Maybe it's somewhat of a stub
> > > > > driver though if there isn't anything controllable.
> > > > > 
> > > > > It's Nuno's area of expertise though +CC.
> > > > >   
> > > > 
> > > > Hi Jonathan,
> > > > 
> > > > Yeah, I did reply David (IIRC) about the very same question. In the
> > > > design/HW Mariel
> > > > is working on the DAC is directly connected to the DMA core which is handled
> > > > already
> > > > by a proper dma controller driver. So in this case I'm really not seeing the
> > > > backend
> > > > need right now (maybe in the future we may have another design for this
> > > > device that
> > > > could justify for a backend device but no idea on that).
> > > > 
> > > > As you mention, we could very well do a stub platform driver so we can use
> > > > the
> > > > backend framework (like dma-backend or something) that could pretty much be
> > > > a stub
> > > > for the DMA controller. But is it worth it though? We'd actually be "lying"
> > > > in terms
> > > > of HW description as the DMA is a property of the actual converter.
> > > > 
> > > > - Nuno Sá
> > > > 
> > > >   
> > > 
> > > I'm a bit inclined to agree with Jonathan here. I could see someone in the
> > > future,
> > > wanting to, e.g., use DMA + a GPIO controller for the parallel interface if
> > > they
> > > didn't have an FPGA. So it seems a bit more future-proof to just always use
> > > the
> > > IIO backend framework for the parallel interface.  
> > 
> > I do agree it's more future but guessing usecases is not something I tend to
> > like much (often just results in code that never gets __really__ used). We can
> > very well take care of it when a usecase pops up and we have an actual device
> > that can be represented by a backend :).
> > 
> > > 
> > > FWIW, I don't think it would be "lying" since the io-backend DT node would be
> > > representing physical parallel bus between the DMA controller and the ADC
> > > chip.  
> > 
> > To me, it's really a stretch having a backend with the only reason (op) of
> > requesting the DMA channel. I still think you're pushing to much and going
> > around with wording to justify for the DMA property :). The parallel bus is part
> > of the DAC and directly connects to the DMA data lines so it really looks to me
> > the dma is a property of the actual DAC.
> > 
> > That said and Mariel can help here, I did not really looked into the design
> > myself and I'm just stating (or what I understood) what Mariel told me. But if
> > there's some other piece of HW sitting between the DMA and the bus then it would
> > be easier for me to agree even if we don't have any real control over that
> > device.
> > 
> > > But if DT maintainers are OK with the idea that a DMA channel can be directly
> > > wired to an external chip, I guess I won't complain. :-)  
> > 
> > That's the case in here so I don't see why it should be a problem :). It's the
> > same with the axi-dac/adc. It's all inside the FPGA but different cores/IPS.
> > 
> > FWIW, I'm ok if we go the backend direction even if I don't fully agree with it
> > (at least with the understanding I have so far about the design). I definitely
> > want to see more users of it but I also don't think it should be a rule for any
> > fairly high speed converter to have a backend associated.
> 
> ok. So short term, DT bindings that document the dma-channel
> and see what the DT maintainers think. Longer term, if things get more
> complex, that can become optional and a backend added.
> 

Ack. Yeah, we'll eventually need an optional get on backends. We do have some out of
tree examples where we have converters working together with an external IP (backend
usecase) or being used in an "independent" mode.

- Nuno Sá

RE: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Tinaco, Mariel]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, May 12, 2024 12:44 AM
> To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring
> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> <broonie@kernel.org>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> 
> [External]
> 
> On Fri, 10 May 2024 14:40:53 +0800
> Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
> 
> > The AD8460 is a “bits in, power out” high voltage, high-power,
> > highspeed driver optimized for large output current (up to ±1 A) and
> > high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) into
> > capacitive loads.
> >
> > A digital engine implements user-configurable features: modes for
> > digital input, programmable supply current, and fault monitoring and
> > programmable protection settings for output current, output voltage,
> > and junction temperature. The AD8460 operates on high voltage dual
> > supplies up to ±55 V and a single low voltage supply of 5 V.
> >
> > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> 
> I'd like to see some ABI docs for the debugfs interface.
> The device is unusual enough that a general intro document or a lot more in the
> series cover letter would be useful.
> 
> I'm not sure what the dmaengine usage in here is doing for example?
> Driving the parallel bus perhaps?  David was correct that the binding should
> reflect that part as well. I was assuming you'd only implemented the spi part.
> 
> How to handle the pattern generator is also an interesting question.
> That probably wants a version of the symbol interfaces we use for PSK and
> similar.  We did have some DDS drivers a long time back in staging but they only
> did a few fixed waveforms so this is breaking new ground.

I also thought about how should the pattern generator be handled. IN the last
revision, there were two debug attributes that make up this feature. Pattern depth
and pattern memory. Ultimately I found a way to combine these two attributes
into one called "test_pattern". The attribute is a string containing an array
of values with a maximum of 16 data words, which the DAC will cycle through
to generate a pattern. The number of values within the string can be anywhere
between 1 to 16 and have a space in between. I also added a "test_pattern_enable"
debug attribute. For the ABI file, should I create one alongside other "debugfs-*"
files and just mention the name of the part? e.g. "debugfs-driver-ad8460" 

> Having looked a little at the datasheet, we may want to hard code some limits -
> or get them from DT. Probably a bit too easy to set things on fire otherwise.

Yes, with the inclusion of the threshold events, I was able to input some limits onto
the device and enable protection upon probing. 

> Also, seems that this is only partly implemented. Please make that clear in the
> patch description.  Perhaps this should have been an RFC with a list of
> questions?
> 
> Jonathan
> 
> > diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c new
> > file mode 100644 index 000000000..4049be0f5
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad8460.c
> > @@ -0,0 +1,652 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * AD8460 Waveform generator DAC Driver
> > + *
> > + * Copyright (C) 2024 Analog Devices, Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/clk.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/buffer-dma.h>
> > +#include <linux/iio/buffer-dmaengine.h> #include <linux/iio/iio.h>
> > +#include <linux/module.h> #include <linux/mod_devicetable.h> #include
> > +<linux/regmap.h> #include <linux/regulator/consumer.h> #include
> > +<linux/spi/spi.h>
> > +
> > +#define AD8460_CTRL_REG(x)                      (x)
> > +#define AD8460_HVDAC_DATA_WORD_LOW(x)           (0x60 + (2 * (x)))
> > +#define AD8460_HVDAC_DATA_WORD_HIGH(x)          (0x61 + (2 * (x)))
> > +
> > +#define AD8460_HV_RESET_MSK                     BIT(7)
> > +#define AD8460_HV_SLEEP_MSK                     BIT(4)
> > +#define AD8460_WAVE_GEN_MODE_MSK                BIT(0)
> > +
> > +#define AD8460_HVDAC_SLEEP_MSK                  BIT(3)
> > +
> > +#define AD8460_APG_MODE_ENABLE_MSK              BIT(5)
> > +#define AD8460_PATTERN_DEPTH_MSK                GENMASK(3, 0)
> > +
> > +#define AD8460_SHUTDOWN_FLAG_MSK                BIT(7)
> > +#define AD8460_DATA_BYTE_LOW_MSK                GENMASK(7, 0)
> > +#define AD8460_DATA_BYTE_HIGH_MSK               GENMASK(5, 0)
> > +
> > +#define AD8460_NOMINAL_VOLTAGE_SPAN		80
> > +#define AD8460_MIN_VREFIO_UV			120000
> > +#define AD8460_MAX_VREFIO_UV			1200000
> > +#define	AD8460_MIN_RSET_OHMS			2000
> > +#define	AD8460_MAX_RSET_OHMS			20000
> > +
> > +struct ad8460_state {
> > +	struct spi_device *spi;
> > +	struct regmap *regmap;
> > +	struct clk *sync_clk;
> > +	/* lock to protect against multiple access to the device and shared data
> */
> > +	struct mutex lock;
> > +	u32 cache_apg_idx;
> > +	u32 rset_ohms;
> > +	int vref_mv;
> > +};
> > +
> 
> > +
> > +static const char * const ad8460_powerdown_modes[] = {
> > +	"three_state",
> > +};
> > +
> > +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
> > +				     const struct iio_chan_spec *chan) {
> > +	return 0;
> 
> Why have the stubs in here?
> 
> > +}
> > +
> > +static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev,
> > +				     const struct iio_chan_spec *chan,
> > +				     unsigned int type)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int ad8460_get_hvdac_byte(struct ad8460_state *state,
> 
> byte? Seems to be getting a 14 bit value.
> 
> > +				 int index,
> > +				 int *val)
> > +{
> > +	unsigned int high, low;
> > +	int ret;
> > +
> > +	ret = regmap_read(state->regmap,
> AD8460_HVDAC_DATA_WORD_HIGH(index),
> > +			  &high);
> 
> Use a bulk read? Then byteswap if necessary and mask the result.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read(state->regmap,
> AD8460_HVDAC_DATA_WORD_LOW(index),
> > +			  &low);
> > +	if (ret)
> > +		return ret;
> > +
> > +	*val = FIELD_GET(AD8460_DATA_BYTE_HIGH_MSK, high) << 8 | low;
> > +
> > +	return ret;
> > +}
> > +
> > +static int ad8460_set_hvdac_byte(struct ad8460_state *state,
> > +				 int index,
> > +				 int val)
> > +{
> > +	int ret;
> > +
> > +	ret = regmap_write(state->regmap,
> AD8460_HVDAC_DATA_WORD_LOW(index),
> > +			   (val & 0xFF));
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_write(state->regmap,
> AD8460_HVDAC_DATA_WORD_HIGH(index),
> > +			    ((val >> 8) & 0xFF));
> 
> bulk write? or do these need to be ordered?
> 
> > +}
> > +
> > +static int ad8460_set_sample(struct ad8460_state *state, int val) {
> > +	int ret;
> > +
> > +	ret = ad8460_enable_apg_mode(state, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	guard(mutex)(&state->lock);
> > +	ret = ad8460_set_hvdac_byte(state, 0, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_update_bits(state->regmap,
> > +				  AD8460_CTRL_REG(0x02),
> > +				  AD8460_PATTERN_DEPTH_MSK,
> > +				  FIELD_PREP(AD8460_PATTERN_DEPTH_MSK,
> 0)); }
> > +
> > +static int ad8460_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int val,
> > +			    int val2,
> > +			    long mask)
> > +{
> > +	struct ad8460_state *state = iio_priv(indio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> > +			return ad8460_set_sample(state, val);
> > +		unreachable();
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> 
> 
> 
> > +static int ad8460_probe(struct spi_device *spi) {
> > +	struct ad8460_state *state;
> > +	struct iio_dev *indio_dev;
> > +	struct regulator *vrefio;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	state = iio_priv(indio_dev);
> > +	mutex_init(&state->lock);
> > +
> > +	state->spi = spi;
> > +
> > +	state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
> > +	if (IS_ERR(state->regmap))
> > +		return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> > +				     "Failed to initialize regmap");
> > +
> > +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev, "tx",
> > +
> IIO_BUFFER_DIRECTION_OUT);
> 
> Ah. I take back my binding comment. I assume this is mapping some non
> standard interface for the parallel data flow?
> 
> > +	if (ret)
> > +		return dev_err_probe(&spi->dev, ret,
> > +				     "Failed to get DMA buffer\n");
> > +
> > +	state->sync_clk = devm_clk_get_enabled(&spi->dev, "sync_clk");
> > +	if (IS_ERR(state->sync_clk))
> > +		return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk),
> > +				     "Failed to get sync clk\n");
> > +
> > +	vrefio = devm_regulator_get_optional(&spi->dev, "vrefio");
> > +	if (IS_ERR(vrefio)) {
> > +		if (PTR_ERR(vrefio) != -ENODEV)
> > +			return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
> > +					     "Failed to get vref regulator\n");
> > +
> > +		state->vref_mv = 1200;
> > +
> > +	} else {
> > +		ret = regulator_enable(vrefio);
> > +		if (ret)
> > +			return dev_err_probe(&spi->dev, ret,
> > +					     "Failed to enable vrefio
> regulator\n");
> > +
> > +		ret = devm_add_action_or_reset(&spi->dev,
> > +					       ad8460_regulator_disable,
> > +					       vrefio);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regulator_get_voltage(vrefio);
> > +		if (ret < 0)
> > +			return dev_err_probe(&spi->dev, ret,
> > +					     "Failed to get ref voltage\n");
> > +
> > +		if (!in_range(ret, AD8460_MIN_VREFIO_UV,
> AD8460_MAX_VREFIO_UV))
> > +			return dev_err_probe(&spi->dev, -EINVAL,
> > +					     "Invalid ref voltage range(%u) [%u,
> %u]\n",
> > +					     ret, AD8460_MIN_VREFIO_UV,
> > +					     AD8460_MAX_VREFIO_UV);
> > +
> > +		state->vref_mv = ret / 1000;
> > +	}
> > +
> > +	ret = device_property_read_u32(&spi->dev, "adi,rset-ohms",
> > +				       &state->rset_ohms);
> > +	if (!ret) {
> > +		if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS,
> > +			      AD8460_MAX_RSET_OHMS))
> > +			return dev_err_probe(&spi->dev, -EINVAL,
> > +					     "Invalid resistor set range(%u) [%u,
> %u]\n",
> > +					     state->rset_ohms,
> > +					     AD8460_MIN_RSET_OHMS,
> > +					     AD8460_MAX_RSET_OHMS);
> > +	}
> > +
> > +	ret = ad8460_reset(state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Enables DAC by default */
> > +	ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
> > +				 AD8460_HVDAC_SLEEP_MSK,
> > +				 FIELD_PREP(AD8460_HVDAC_SLEEP_MSK,
> 0));
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->name = "ad8460";
> > +	indio_dev->info = &ad8460_info;
> > +	indio_dev->channels = ad8460_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE |
> INDIO_BUFFER_HARDWARE;
> > +	indio_dev->setup_ops = &ad8460_buffer_setup_ops;
> > +
> > +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ad8460_debugfs_init(indio_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ad8460_of_match[] = {
> > +	{ .compatible = "adi, ad8460" },
> > +	{ },
> No comma on 'null' terminators like this as we don't want to let people put
> anything after them.
> 
> > +};
> > +MODULE_DEVICE_TABLE(of, ad8460_of_match);

Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Jonathan Cameron]

On Mon, 24 Jun 2024 04:56:57 +0000
"Tinaco, Mariel" <Mariel.Tinaco@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, May 12, 2024 12:44 AM
> > To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring
> > <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> > <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> > <broonie@kernel.org>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> > Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> > <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> > Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> > 
> > [External]
> > 
> > On Fri, 10 May 2024 14:40:53 +0800
> > Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
> >   
> > > The AD8460 is a “bits in, power out” high voltage, high-power,
> > > highspeed driver optimized for large output current (up to ±1 A) and
> > > high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V) into
> > > capacitive loads.
> > >
> > > A digital engine implements user-configurable features: modes for
> > > digital input, programmable supply current, and fault monitoring and
> > > programmable protection settings for output current, output voltage,
> > > and junction temperature. The AD8460 operates on high voltage dual
> > > supplies up to ±55 V and a single low voltage supply of 5 V.
> > >
> > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>  
> > 
> > I'd like to see some ABI docs for the debugfs interface.
> > The device is unusual enough that a general intro document or a lot more in the
> > series cover letter would be useful.
> > 
> > I'm not sure what the dmaengine usage in here is doing for example?
> > Driving the parallel bus perhaps?  David was correct that the binding should
> > reflect that part as well. I was assuming you'd only implemented the spi part.
> > 
> > How to handle the pattern generator is also an interesting question.
> > That probably wants a version of the symbol interfaces we use for PSK and
> > similar.  We did have some DDS drivers a long time back in staging but they only
> > did a few fixed waveforms so this is breaking new ground.  
> 
> I also thought about how should the pattern generator be handled. IN the last
> revision, there were two debug attributes that make up this feature. Pattern depth
> and pattern memory. Ultimately I found a way to combine these two attributes
> into one called "test_pattern". The attribute is a string containing an array
> of values with a maximum of 16 data words, which the DAC will cycle through
> to generate a pattern. The number of values within the string can be anywhere
> between 1 to 16 and have a space in between. I also added a "test_pattern_enable"
> debug attribute. For the ABI file, should I create one alongside other "debugfs-*"
> files and just mention the name of the part? e.g. "debugfs-driver-ad8460" 

Doing this in debugfs basically means you aren't intending it to get used in real
usecases.  So we need some sysfs ABI.

That probably means a mode switch similar to the ones we have for devices that use
an external toggle (typically for Frequency Shift Keying or similar or sometimes just
to flip between two DC voltages).  We need a new term though as this isn't a toggle.

For the values we could map it to that interface which is something like

out_voltage0_raw0
out_voltage0_raw1

etc.  That avoids need for a new ABI for the values, but perhaps isn't that
elegant if the patterns on other devices we eventually support this on get
large.

Anyone know how large these typically get? I'm being lazy and don't want to
datasheet dive this evening!

Jonathan

RE: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Tinaco, Mariel]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, June 29, 2024 2:51 AM
> To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring
> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> <broonie@kernel.org>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> 
> [External]
> 
> On Mon, 24 Jun 2024 04:56:57 +0000
> "Tinaco, Mariel" <Mariel.Tinaco@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Sunday, May 12, 2024 12:44 AM
> > > To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob
> > > Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> > > Conor Dooley <conor+dt@kernel.org>; Liam Girdwood
> > > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; Hennerich,
> > > Michael <Michael.Hennerich@analog.com>; Marcelo Schmitt
> > > <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> > > <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> > > Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> > >
> > > [External]
> > >
> > > On Fri, 10 May 2024 14:40:53 +0800
> > > Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
> > >
> > > > The AD8460 is a “bits in, power out” high voltage, high-power,
> > > > highspeed driver optimized for large output current (up to ±1 A)
> > > > and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40
> > > > V) into capacitive loads.
> > > >
> > > > A digital engine implements user-configurable features: modes for
> > > > digital input, programmable supply current, and fault monitoring
> > > > and programmable protection settings for output current, output
> > > > voltage, and junction temperature. The AD8460 operates on high
> > > > voltage dual supplies up to ±55 V and a single low voltage supply of 5 V.
> > > >
> > > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> > >
> > > I'd like to see some ABI docs for the debugfs interface.
> > > The device is unusual enough that a general intro document or a lot
> > > more in the series cover letter would be useful.
> > >
> > > I'm not sure what the dmaengine usage in here is doing for example?
> > > Driving the parallel bus perhaps?  David was correct that the
> > > binding should reflect that part as well. I was assuming you'd only
> implemented the spi part.
> > >
> > > How to handle the pattern generator is also an interesting question.
> > > That probably wants a version of the symbol interfaces we use for
> > > PSK and similar.  We did have some DDS drivers a long time back in
> > > staging but they only did a few fixed waveforms so this is breaking new
> ground.
> >
> > I also thought about how should the pattern generator be handled. IN
> > the last revision, there were two debug attributes that make up this
> > feature. Pattern depth and pattern memory. Ultimately I found a way to
> > combine these two attributes into one called "test_pattern". The
> > attribute is a string containing an array of values with a maximum of
> > 16 data words, which the DAC will cycle through to generate a pattern.
> > The number of values within the string can be anywhere between 1 to 16 and
> have a space in between. I also added a "test_pattern_enable"
> > debug attribute. For the ABI file, should I create one alongside other "debugfs-
> *"
> > files and just mention the name of the part? e.g. "debugfs-driver-ad8460"
> 
> Doing this in debugfs basically means you aren't intending it to get used in real
> usecases.  So we need some sysfs ABI.
> 
> That probably means a mode switch similar to the ones we have for devices that
> use an external toggle (typically for Frequency Shift Keying or similar or
> sometimes just to flip between two DC voltages).  We need a new term though
> as this isn't a toggle.
> 
> For the values we could map it to that interface which is something like
> 
> out_voltage0_raw0
> out_voltage0_raw1
> 
> etc.  That avoids need for a new ABI for the values, but perhaps isn't that elegant
> if the patterns on other devices we eventually support this on get large.

I saw some drivers making use of "test_pattern" attribute. I think that might fit.
But I'd look into "raw0" and "raw1" as well

> 
> Anyone know how large these typically get? I'm being lazy and don't want to
> datasheet dive this evening!

It only gets up to 16 values. 

> 
> Jonathan

RE: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[Tinaco, Mariel]

> -----Original Message-----
> From: Tinaco, Mariel
> Sent: Monday, July 8, 2024 7:32 AM
> To: Jonathan Cameron <jic23@kernel.org>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring
> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> <conor+dt@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> <broonie@kernel.org>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> Marcelo Schmitt <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> Subject: RE: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> 
> 
> 
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, June 29, 2024 2:51 AM
> > To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob
> > Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> > Conor Dooley <conor+dt@kernel.org>; Liam Girdwood
> > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; Hennerich,
> > Michael <Michael.Hennerich@analog.com>; Marcelo Schmitt
> > <marcelo.schmitt1@gmail.com>; Dimitri Fedrau <dima.fedrau@gmail.com>;
> > Guenter Roeck <linux@roeck-us.net>
> > Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> >
> > [External]
> >
> > On Mon, 24 Jun 2024 04:56:57 +0000
> > "Tinaco, Mariel" <Mariel.Tinaco@analog.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Sunday, May 12, 2024 12:44 AM
> > > > To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> > > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob
> > > > Herring <robh@kernel.org>; Krzysztof Kozlowski
> > > > <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Liam
> > > > Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> > > > Hennerich, Michael <Michael.Hennerich@analog.com>; Marcelo Schmitt
> > > > <marcelo.schmitt1@gmail.com>; Dimitri Fedrau
> > > > <dima.fedrau@gmail.com>; Guenter Roeck <linux@roeck-us.net>
> > > > Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
> > > >
> > > > [External]
> > > >
> > > > On Fri, 10 May 2024 14:40:53 +0800 Mariel Tinaco
> > > > <Mariel.Tinaco@analog.com> wrote:
> > > >
> > > > > The AD8460 is a “bits in, power out” high voltage, high-power,
> > > > > highspeed driver optimized for large output current (up to ±1 A)
> > > > > and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40
> > > > > V) into capacitive loads.
> > > > >
> > > > > A digital engine implements user-configurable features: modes
> > > > > for digital input, programmable supply current, and fault
> > > > > monitoring and programmable protection settings for output
> > > > > current, output voltage, and junction temperature. The AD8460
> > > > > operates on high voltage dual supplies up to ±55 V and a single low
> voltage supply of 5 V.
> > > > >
> > > > > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> > > >
> > > > I'd like to see some ABI docs for the debugfs interface.
> > > > The device is unusual enough that a general intro document or a
> > > > lot more in the series cover letter would be useful.
> > > >
> > > > I'm not sure what the dmaengine usage in here is doing for example?
> > > > Driving the parallel bus perhaps?  David was correct that the
> > > > binding should reflect that part as well. I was assuming you'd
> > > > only
> > implemented the spi part.
> > > >
> > > > How to handle the pattern generator is also an interesting question.
> > > > That probably wants a version of the symbol interfaces we use for
> > > > PSK and similar.  We did have some DDS drivers a long time back in
> > > > staging but they only did a few fixed waveforms so this is
> > > > breaking new
> > ground.
> > >
> > > I also thought about how should the pattern generator be handled. IN
> > > the last revision, there were two debug attributes that make up this
> > > feature. Pattern depth and pattern memory. Ultimately I found a way
> > > to combine these two attributes into one called "test_pattern". The
> > > attribute is a string containing an array of values with a maximum
> > > of
> > > 16 data words, which the DAC will cycle through to generate a pattern.
> > > The number of values within the string can be anywhere between 1 to
> > > 16 and
> > have a space in between. I also added a "test_pattern_enable"
> > > debug attribute. For the ABI file, should I create one alongside
> > > other "debugfs-
> > *"
> > > files and just mention the name of the part? e.g. "debugfs-driver-ad8460"
> >
> > Doing this in debugfs basically means you aren't intending it to get
> > used in real usecases.  So we need some sysfs ABI.
> >
> > That probably means a mode switch similar to the ones we have for
> > devices that use an external toggle (typically for Frequency Shift
> > Keying or similar or sometimes just to flip between two DC voltages).
> > We need a new term though as this isn't a toggle.
> >
> > For the values we could map it to that interface which is something
> > like
> >
> > out_voltage0_raw0
> > out_voltage0_raw1
> >
> > etc.  That avoids need for a new ABI for the values, but perhaps isn't
> > that elegant if the patterns on other devices we eventually support this on get
> large.
> 
> I saw some drivers making use of "test_pattern" attribute. I think that might fit.
> But I'd look into "raw0" and "raw1" as well

I'm sorry about mentioning "test_pattern" again. I forgot I already mentioned it in
An earlier reply

> 
> >
> > Anyone know how large these typically get? I'm being lazy and don't
> > want to datasheet dive this evening!
> 
> It only gets up to 16 values.
> 
> >
> > Jonathan

Re: [PATCH 0/2] add AD8460 DAC driver

[David Lechner]

On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
>
> The AD8460 is a 14-bit, high power +-40V 1A, high-speed DAC,
> with dual digital input modes, programmable supply current and
> fault monitoring and protection settings for output current,
> output voltage and junction temperature.
>
> The fault monitoring and shutdown protection features were
> supported in the earlier versions of the IIO driver but was
> scrapped due to uncertainties if the functionalities belong to
> the IIO driver. However, it would be best to implement it for
> the device's quality of life. I'd like to know if it's better
> suited as a stand-alone HWMON driver.
>
> The following are the configurable and readable parameters
> through SPI that could be implemented on the HWMON driver:
>   * An enable bit to arm/protect the device on overcurrent,
> overvoltage or overtemperature events. The device is shut down
> upon detection.
>   * A configurable range/threshold for voltage, current and
> temperature that raises alarm when exceeded while the device is
> armed.
>   * Flags that can be polled to raise alarm upon detection of
> overcurrent, overvoltage or overtemperature events, and apply
> additional protective measures.
>   * Programmable quiescent current (optional)
>   * Thermal monitoring is done by measuring voltage on TMP pin
> (unlikely to be included)
>

Adding myself to the cc: here since I'm interested to see what
Jonathan (or anyone else) has to say about the fault monitoring.

Re: [PATCH 0/2] add AD8460 DAC driver

[Jonathan Cameron]

On Fri, 10 May 2024 12:30:46 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
> >
> > The AD8460 is a 14-bit, high power +-40V 1A, high-speed DAC,
> > with dual digital input modes, programmable supply current and
> > fault monitoring and protection settings for output current,
> > output voltage and junction temperature.
> >
> > The fault monitoring and shutdown protection features were
> > supported in the earlier versions of the IIO driver but was
> > scrapped due to uncertainties if the functionalities belong to
> > the IIO driver. However, it would be best to implement it for
> > the device's quality of life. I'd like to know if it's better
> > suited as a stand-alone HWMON driver.

That probably doesn't make sense.  This reply btw is based on the
text here. I haven't yet looked in detail at the datasheet.

Some fault conditions are relatively easy to map to threshold events
on an input channel.  The ones that are harder are things
like short circuit detectors where it's hard to know what the
actual threshold is.

The other place we are limited is in multiple levels for the
same thing.  So warning and fault levels. That stuff is handled
much better by hwmon where these things are more common.
The issue we have is that the event encoding is a bit tight
but we could see if there is a way to add these.

> >
> > The following are the configurable and readable parameters
> > through SPI that could be implemented on the HWMON driver:
> >   * An enable bit to arm/protect the device on overcurrent,
> > overvoltage or overtemperature events. The device is shut down
> > upon detection.

We don't have a good way to handle restarting from shutdown, but
perhaps we could use another action to signal that - maybe even
going as far as saying that the driver should be reloaded.

As for the thresholds, they sound like the map to IIO events
reasonably well.  

> >   * A configurable range/threshold for voltage, current and
> > temperature that raises alarm when exceeded while the device is
> > armed.

That maps fine to the IIO event threshold controls.

> >   * Flags that can be polled to raise alarm upon detection of
> > overcurrent, overvoltage or overtemperature events, and apply
> > additional protective measures.
The specific need to poll is awkward but you could do it easily
enough in driver and use the IIO event stuff to signal any
events detected.


> >   * Programmable quiescent current (optional)
Could probably figure out a suitable control for this, but I'm not
entirely sure what it is :)

> >   * Thermal monitoring is done by measuring voltage on TMP pin
> > (unlikely to be included)

If you did want to, the usual trick for these is to include
an optional use as a consumer of an IIO provider which would be
a separate ADC. 

> >  
> 
> Adding myself to the cc: here since I'm interested to see what
> Jonathan (or anyone else) has to say about the fault monitoring.

Jonathan

RE: [PATCH 0/2] add AD8460 DAC driver

[Tinaco, Mariel]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, May 12, 2024 12:22 AM
> To: David Lechner <dlechner@baylibre.com>
> Cc: Tinaco, Mariel <Mariel.Tinaco@analog.com>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Lars-Peter Clausen
> <lars@metafoo.de>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski
> <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Liam Girdwood
> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Marcelo Schmitt
> <marcelo.schmitt1@gmail.com>; Dimitri Fedrau <dima.fedrau@gmail.com>;
> Guenter Roeck <linux@roeck-us.net>
> Subject: Re: [PATCH 0/2] add AD8460 DAC driver
> 
> [External]
> 
> On Fri, 10 May 2024 12:30:46 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > On Fri, May 10, 2024 at 1:42 AM Mariel Tinaco <Mariel.Tinaco@analog.com>
> wrote:
> > >
> > > The AD8460 is a 14-bit, high power +-40V 1A, high-speed DAC, with
> > > dual digital input modes, programmable supply current and fault
> > > monitoring and protection settings for output current, output
> > > voltage and junction temperature.
> > >
> > > The fault monitoring and shutdown protection features were supported
> > > in the earlier versions of the IIO driver but was scrapped due to
> > > uncertainties if the functionalities belong to the IIO driver.
> > > However, it would be best to implement it for the device's quality
> > > of life. I'd like to know if it's better suited as a stand-alone
> > > HWMON driver.
> 
> That probably doesn't make sense.  This reply btw is based on the text here. I
> haven't yet looked in detail at the datasheet.
> 
> Some fault conditions are relatively easy to map to threshold events on an input
> channel.  The ones that are harder are things like short circuit detectors where it's
> hard to know what the actual threshold is.

Upon checking the IIO Threshold events, the fault conditions can easily be mapped.
But I had to add Current and Temperature channels

> The other place we are limited is in multiple levels for the same thing.  So
> warning and fault levels. That stuff is handled much better by hwmon where
> these things are more common.
> The issue we have is that the event encoding is a bit tight but we could see if
> there is a way to add these.

Fortunately, there weren't multiple levels of fault monitoring as checked in the
datasheet

> > >
> > > The following are the configurable and readable parameters through
> > > SPI that could be implemented on the HWMON driver:
> > >   * An enable bit to arm/protect the device on overcurrent,
> > > overvoltage or overtemperature events. The device is shut down upon
> > > detection.
> 
> We don't have a good way to handle restarting from shutdown, but perhaps we
> could use another action to signal that - maybe even going as far as saying that
> the driver should be reloaded.
> 
> As for the thresholds, they sound like the map to IIO events reasonably well.
> 
> > >   * A configurable range/threshold for voltage, current and
> > > temperature that raises alarm when exceeded while the device is
> > > armed.
> 
> That maps fine to the IIO event threshold controls.
> 
> > >   * Flags that can be polled to raise alarm upon detection of
> > > overcurrent, overvoltage or overtemperature events, and apply
> > > additional protective measures.
> The specific need to poll is awkward but you could do it easily enough in driver
> and use the IIO event stuff to signal any events detected.
> 
> 
> > >   * Programmable quiescent current (optional)
> Could probably figure out a suitable control for this, but I'm not entirely sure
> what it is :)

Thinking about it, wouldn't the raw attribute be a suitable control for this? This 
Value is relative to nominal supply current and acts as a "monotonic but nonlinear"
multiplier. A register value maps to a current level from 0 to 2 times the nominal
current supplied. I also thought that it could be hardware gain but the gain
computation wasn't explicitly indicated in the datasheet and there is not yet
"current_hardwaregain" attribute available in the ABI. So I settled with raw. I
Think there would only be an issue of we expose the "processed" attribute
Because it has a particular computation. But I'm not planning to expose the 
Processed attribute

> > >   * Thermal monitoring is done by measuring voltage on TMP pin
> > > (unlikely to be included)
>
> If you did want to, the usual trick for these is to include an optional use as a
> consumer of an IIO provider which would be a separate ADC.

I included this in my current revision, thanks for the idea. Although the optional use
Isn’t yet available in the consumer API. My question is, in case the ADC channel to read
The TMP pin is not available, should I still make the temp raw value available and
Set to 0? Or should the temp raw attribute be unavailable or unlisted completely from
IIO Info.
> > >
> >
> > Adding myself to the cc: here since I'm interested to see what
> > Jonathan (or anyone else) has to say about the fault monitoring.
> 
> Jonathan

Re: [PATCH 0/2] add AD8460 DAC driver

[Jonathan Cameron]

> >   
> > > >   * Programmable quiescent current (optional)  
> > Could probably figure out a suitable control for this, but I'm not entirely sure
> > what it is :)  
> 
> Thinking about it, wouldn't the raw attribute be a suitable control for this? This 
> Value is relative to nominal supply current and acts as a "monotonic but nonlinear"
> multiplier. 
> A register value maps to a current level from 0 to 2 times the nominal
> current supplied. I also thought that it could be hardware gain but the gain
> computation wasn't explicitly indicated in the datasheet and there is not yet
> "current_hardwaregain" attribute available in the ABI. So I settled with raw. 

I don't entirely understand what is actually for, but a raw current output
might be appropriate.

>I
> Think there would only be an issue of we expose the "processed" attribute
> Because it has a particular computation. But I'm not planning to expose the 
> Processed attribute

Is there any reason someone might in future though?

> 
> > > >   * Thermal monitoring is done by measuring voltage on TMP pin
> > > > (unlikely to be included)  
> >
> > If you did want to, the usual trick for these is to include an optional use as a
> > consumer of an IIO provider which would be a separate ADC.  
> 
> I included this in my current revision, thanks for the idea. Although the optional use
> Isn’t yet available in the consumer API. My question is, in case the ADC channel to read
> The TMP pin is not available, should I still make the temp raw value available and
> Set to 0? Or should the temp raw attribute be unavailable or unlisted completely from
> IIO Info.
If no ADC channel then remove it from the chan_spec.  That probably means you
need separate arrays of struct iio_chan_spec for the two case.

Jonathan

> > > >  
> > >
> > > Adding myself to the cc: here since I'm interested to see what
> > > Jonathan (or anyone else) has to say about the fault monitoring.  
> > 
> > Jonathan  

RE: [PATCH 0/2] add AD8460 DAC driver

[Tinaco, Mariel]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, June 29, 2024 2:40 AM
> To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> Cc: David Lechner <dlechner@baylibre.com>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Lars-Peter Clausen
> <lars@metafoo.de>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski
> <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Liam Girdwood
> <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Marcelo Schmitt
> <marcelo.schmitt1@gmail.com>; Dimitri Fedrau <dima.fedrau@gmail.com>;
> Guenter Roeck <linux@roeck-us.net>
> Subject: Re: [PATCH 0/2] add AD8460 DAC driver
> 
> [External]
> 
> 
> > >
> > > > >   * Programmable quiescent current (optional)
> > > Could probably figure out a suitable control for this, but I'm not
> > > entirely sure what it is :)
> >
> > Thinking about it, wouldn't the raw attribute be a suitable control
> > for this? This Value is relative to nominal supply current and acts as a
> "monotonic but nonlinear"
> > multiplier.
> > A register value maps to a current level from 0 to 2 times the nominal
> > current supplied. I also thought that it could be hardware gain but
> > the gain computation wasn't explicitly indicated in the datasheet and
> > there is not yet "current_hardwaregain" attribute available in the ABI. So I
> settled with raw.
> 
> I don't entirely understand what is actually for, but a raw current output might
> be appropriate.
> 
> >I
> > Think there would only be an issue of we expose the "processed"
> >attribute  Because it has a particular computation. But I'm not
> >planning to expose the  Processed attribute
> 
> Is there any reason someone might in future though?

They would have to measure the supply current to see the effect of
This attribute, then that would be the "processed" value. I don't think
that would be necessary

> 
> >
> > > > >   * Thermal monitoring is done by measuring voltage on TMP pin
> > > > > (unlikely to be included)
> > >
> > > If you did want to, the usual trick for these is to include an
> > > optional use as a consumer of an IIO provider which would be a separate
> ADC.
> >
> > I included this in my current revision, thanks for the idea. Although
> > the optional use Isn’t yet available in the consumer API. My question
> > is, in case the ADC channel to read The TMP pin is not available,
> > should I still make the temp raw value available and Set to 0? Or
> > should the temp raw attribute be unavailable or unlisted completely from IIO
> Info.
> If no ADC channel then remove it from the chan_spec.  That probably means you
> need separate arrays of struct iio_chan_spec for the two case.
> 
> Jonathan
> 
> > > > >
> > > >
> > > > Adding myself to the cc: here since I'm interested to see what
> > > > Jonathan (or anyone else) has to say about the fault monitoring.
> > >
> > > Jonathan