[PATCH RFC 0/3] iio: adc: add new ad7625 driver

[PATCH RFC 0/3] iio: adc: add new ad7625 driver

[Trevor Gamblin]

This series adds a new driver for the Analog Devices Inc. AD7625,
AD7626, AD7960, and AD7961. These chips are part of a family of
LVDS-based SAR ADCs. The initial driver implementation does not support
the devices' self-clocked mode, although that can be added later.

One aspect that is still uncertain is whether there should be a
devicetree property indicating if the DCO+/- pins are connected, so
specific feedback on that is appreciated.

The devices make use of two offset PWM signals, one to trigger
conversions and the other as a burst signal for transferring data to the
host. These rely on the new PWM waveform functionality being
reviewed in [1].

This work is being done by BayLibre and on behalf of Analog Devices
Inc., hence the maintainers are @analog.com.

Special thanks to David Lechner for his guidance and reviews.

Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
Trevor Gamblin (3):
      dt-bindings: iio: adc: add AD762x/AD796x ADCs
      iio: adc: ad7625: add driver
      docs: iio: new docs for ad7625 driver

 .../devicetree/bindings/iio/adc/adi,ad7625.yaml    | 176 ++++++
 Documentation/iio/ad7625.rst                       |  91 +++
 MAINTAINERS                                        |  11 +
 drivers/iio/adc/Kconfig                            |  15 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/ad7625.c                           | 626 +++++++++++++++++++++
 6 files changed, 920 insertions(+)
---
base-commit: ac6a258892793f0a255fe7084ec2b612131c67fc
change-id: 20240730-ad7625_r1-60d17ea28958

Best regards,
-- 
Trevor Gamblin <tgamblin@baylibre.com>

[PATCH RFC 1/3] dt-bindings: iio: adc: add AD762x/AD796x ADCs

[Trevor Gamblin]

This adds a binding specification for the Analog Devices Inc. AD7625,
AD7626, AD7960, and AD7961 ADCs.

Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7625.yaml    | 176 +++++++++++++++++++++
 MAINTAINERS                                        |   9 ++
 2 files changed, 185 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
new file mode 100644
index 000000000000..e88db0ac2534
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
@@ -0,0 +1,176 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7625.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices Fast PulSAR Analog to Digital Converters
+
+maintainers:
+  - Michael Hennerich <Michael.Hennerich@analog.com>
+  - Nuno Sá <nuno.sa@analog.com>
+
+description: |
+  A family of single channel differential analog to digital converters
+  in a LFCSP package. Note that these bindings are for the device when
+  used with the PulSAR LVDS project:
+  http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html.
+
+  * https://www.analog.com/en/products/ad7625.html
+  * https://www.analog.com/en/products/ad7626.html
+  * https://www.analog.com/en/products/ad7960.html
+  * https://www.analog.com/en/products/ad7961.html
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7625
+      - adi,ad7626
+      - adi,ad7960
+      - adi,ad7961
+
+  vdd1-supply:
+    description: A supply that powers the analog and digital circuitry.
+
+  vdd2-supply:
+    description: A supply that powers the analog and digital circuitry.
+
+  vio-supply:
+    description: A supply for the inputs and outputs.
+
+  ref-supply:
+    description:
+      Voltage regulator for the external reference voltage (REF).
+
+  refin-supply:
+    description:
+      Voltage regulator for the reference buffer input (REFIN).
+
+  clocks:
+    description:
+      The clock connected to the CLK pins, gated by the clk_gate PWM.
+    maxItems: 1
+
+  pwms:
+    maxItems: 2
+
+  pwm-names:
+    maxItems: 2
+    items:
+      - const: cnv
+        description: PWM connected to the CNV input on the ADC.
+      - const: clk_gate
+        description: PWM that gates the clock connected to the ADC's CLK input.
+
+  io-backends:
+    description:
+      The AXI ADC IP block connected to the D+/- and DCO+/- lines of the ADC.
+    maxItems: 1
+
+  adi,en0-always-on:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Indicates if EN0 is hard-wired to the high state. If neither this
+      nor en0-gpios are present, then EN0 is hard-wired low.
+
+  adi,en1-always-on:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Indicates if EN1 is hard-wired to the high state. If neither this
+      nor en1-gpios are present, then EN1 is hard-wired low.
+
+  adi,en2-always-on:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Indicates if EN2 is hard-wired to the high state. If neither this
+      nor en2-gpios are present, then EN2 is hard-wired low.
+
+  adi,en3-always-on:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Indicates if EN3 is hard-wired to the high state. If neither this
+      nor en3-gpios are present, then EN3 is hard-wired low.
+
+  en0-gpios:
+    description:
+      Configurable EN0 pin.
+
+  en1-gpios:
+    description:
+      Configurable EN1 pin.
+
+  en2-gpios:
+    description:
+      Configurable EN2 pin.
+
+  en3-gpios:
+    description:
+      Configurable EN3 pin.
+
+required:
+  - compatible
+  - vdd1-supply
+  - vdd2-supply
+  - vio-supply
+  - clocks
+  - pwms
+  - pwm-names
+  - io-backends
+
+- if:
+  properties:
+    compatible:
+      contains:
+        enum:
+	  - adi,ad7625
+	  - adi,ad7626
+  then:
+    properties:
+      en2-gpios: false
+      en3-gpios: false
+      adi,en2-always-on: false
+      adi,en3-always-on: false
+    allOf:
+      # ref-supply and refin-supply are mutually-exclusive (neither is also
+      # valid)
+      - if:
+          required:
+            - ref-supply
+        then:
+          properties:
+            refin-supply: false
+      - if:
+          required:
+            - refin-supply
+        then:
+          properties:
+            ref-supply: false
+
+- if:
+  properties:
+    compatible:
+      contains:
+        enum:
+	  - adi,ad7960
+	  - adi,ad7961
+  then:
+    oneOf:
+      required:
+        - ref-supply
+      required:
+        - refin-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    adc {
+        compatible = "adi,ad7625";
+        vdd1-supply = <&supply_5V>;
+        vdd2-supply = <&supply_2_5V>;
+        vio-supply = <&supply_2_5V>;
+        io-backends = <&axi_adc>;
+        clock = <&ref_clk>;
+        pwms = <&axi_pwm_gen 0 0>, <&axi_pwm_gen 1 0>;
+        pwm-names = "cnv", "clk_gate";
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 42decde38320..2361f92751dd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1260,6 +1260,15 @@ F:	Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
 F:	drivers/iio/addac/ad74413r.c
 F:	include/dt-bindings/iio/addac/adi,ad74413r.h
 
+ANALOG DEVICES INC AD7625 DRIVER
+M:	Michael Hennerich <Michael.Hennerich@analog.com>
+M:	Nuno Sá <nuno.sa@analog.com>
+R:	Trevor Gamblin <tgamblin@baylibre.com>
+S:	Supported
+W:	https://ez.analog.com/linux-software-drivers
+W:	http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
+
 ANALOG DEVICES INC AD7768-1 DRIVER
 M:	Michael Hennerich <Michael.Hennerich@analog.com>
 L:	linux-iio@vger.kernel.org

-- 
2.39.2

Re: [PATCH RFC 1/3] dt-bindings: iio: adc: add AD762x/AD796x ADCs

[Krzysztof Kozlowski]

On 31/07/2024 15:48, Trevor Gamblin wrote:
> This adds a binding specification for the Analog Devices Inc. AD7625,
> AD7626, AD7960, and AD7961 ADCs.

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

Why this is not ready, but RFC? What exactly needs to be commented here?

> 
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7625.yaml    | 176 +++++++++++++++++++++
>  MAINTAINERS                                        |   9 ++
>  2 files changed, 185 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
> new file mode 100644
> index 000000000000..e88db0ac2534
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
> @@ -0,0 +1,176 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7625.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices Fast PulSAR Analog to Digital Converters
> +
> +maintainers:
> +  - Michael Hennerich <Michael.Hennerich@analog.com>
> +  - Nuno Sá <nuno.sa@analog.com>
> +
> +description: |
> +  A family of single channel differential analog to digital converters
> +  in a LFCSP package. Note that these bindings are for the device when
> +  used with the PulSAR LVDS project:
> +  http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html.

Eh? And what could be other case - used for what? What are the
differences? Why mentioning it?

> +
> +  * https://www.analog.com/en/products/ad7625.html
> +  * https://www.analog.com/en/products/ad7626.html
> +  * https://www.analog.com/en/products/ad7960.html
> +  * https://www.analog.com/en/products/ad7961.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7625
> +      - adi,ad7626
> +      - adi,ad7960
> +      - adi,ad7961
> +
> +  vdd1-supply:
> +    description: A supply that powers the analog and digital circuitry.
> +
> +  vdd2-supply:
> +    description: A supply that powers the analog and digital circuitry.
> +
> +  vio-supply:
> +    description: A supply for the inputs and outputs.
> +
> +  ref-supply:
> +    description:
> +      Voltage regulator for the external reference voltage (REF).
> +
> +  refin-supply:
> +    description:
> +      Voltage regulator for the reference buffer input (REFIN).
> +
> +  clocks:
> +    description:
> +      The clock connected to the CLK pins, gated by the clk_gate PWM.
> +    maxItems: 1
> +
> +  pwms:
> +    maxItems: 2
> +
> +  pwm-names:
> +    maxItems: 2
> +    items:
> +      - const: cnv
> +        description: PWM connected to the CNV input on the ADC.
> +      - const: clk_gate
> +        description: PWM that gates the clock connected to the ADC's CLK input.
> +
> +  io-backends:
> +    description:
> +      The AXI ADC IP block connected to the D+/- and DCO+/- lines of the ADC.
> +    maxItems: 1
> +
> +  adi,en0-always-on:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates if EN0 is hard-wired to the high state. If neither this
> +      nor en0-gpios are present, then EN0 is hard-wired low.
> +
> +  adi,en1-always-on:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates if EN1 is hard-wired to the high state. If neither this
> +      nor en1-gpios are present, then EN1 is hard-wired low.
> +
> +  adi,en2-always-on:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates if EN2 is hard-wired to the high state. If neither this
> +      nor en2-gpios are present, then EN2 is hard-wired low.
> +
> +  adi,en3-always-on:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates if EN3 is hard-wired to the high state. If neither this
> +      nor en3-gpios are present, then EN3 is hard-wired low.
> +
> +  en0-gpios:
> +    description:
> +      Configurable EN0 pin.
> +
> +  en1-gpios:
> +    description:
> +      Configurable EN1 pin.
> +
> +  en2-gpios:
> +    description:
> +      Configurable EN2 pin.
> +
> +  en3-gpios:
> +    description:
> +      Configurable EN3 pin.
> +
> +required:
> +  - compatible
> +  - vdd1-supply
> +  - vdd2-supply
> +  - vio-supply
> +  - clocks
> +  - pwms
> +  - pwm-names
> +  - io-backends
> +
> +- if:
> +  properties:

I don't think this was ever tested. Please use existing bindings or
example-schema as template.

> +    compatible:
> +      contains:
> +        enum:
> +	  - adi,ad7625
> +	  - adi,ad7626


Best regards,
Krzysztof

Re: [PATCH RFC 1/3] dt-bindings: iio: adc: add AD762x/AD796x ADCs

[Trevor Gamblin]

On 2024-07-31 10:11 a.m., Krzysztof Kozlowski wrote:
> On 31/07/2024 15:48, Trevor Gamblin wrote:
>> This adds a binding specification for the Analog Devices Inc. AD7625,
>> AD7626, AD7960, and AD7961 ADCs.
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
Will do.
>
> Why this is not ready, but RFC? What exactly needs to be commented here?
There's one outstanding question about whether or not there should be a 
DT property for specifying whether DCO+/- lines are connected (mentioned 
in the cover letter but not here). I guess it doesn't need to be an RFC 
just for that.
>
>> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
>> ---
>>   .../devicetree/bindings/iio/adc/adi,ad7625.yaml    | 176 +++++++++++++++++++++
>>   MAINTAINERS                                        |   9 ++
>>   2 files changed, 185 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
>> new file mode 100644
>> index 000000000000..e88db0ac2534
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
>> @@ -0,0 +1,176 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7625.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Analog Devices Fast PulSAR Analog to Digital Converters
>> +
>> +maintainers:
>> +  - Michael Hennerich <Michael.Hennerich@analog.com>
>> +  - Nuno Sá <nuno.sa@analog.com>
>> +
>> +description: |
>> +  A family of single channel differential analog to digital converters
>> +  in a LFCSP package. Note that these bindings are for the device when
>> +  used with the PulSAR LVDS project:
>> +  http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html.
> Eh? And what could be other case - used for what? What are the
> differences? Why mentioning it?
Poor wording on my part - I'm not aware of another configuration. Will 
fix on the resend.
>
>> +
>> +  * https://www.analog.com/en/products/ad7625.html
>> +  * https://www.analog.com/en/products/ad7626.html
>> +  * https://www.analog.com/en/products/ad7960.html
>> +  * https://www.analog.com/en/products/ad7961.html
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - adi,ad7625
>> +      - adi,ad7626
>> +      - adi,ad7960
>> +      - adi,ad7961
>> +
>> +  vdd1-supply:
>> +    description: A supply that powers the analog and digital circuitry.
>> +
>> +  vdd2-supply:
>> +    description: A supply that powers the analog and digital circuitry.
>> +
>> +  vio-supply:
>> +    description: A supply for the inputs and outputs.
>> +
>> +  ref-supply:
>> +    description:
>> +      Voltage regulator for the external reference voltage (REF).
>> +
>> +  refin-supply:
>> +    description:
>> +      Voltage regulator for the reference buffer input (REFIN).
>> +
>> +  clocks:
>> +    description:
>> +      The clock connected to the CLK pins, gated by the clk_gate PWM.
>> +    maxItems: 1
>> +
>> +  pwms:
>> +    maxItems: 2
>> +
>> +  pwm-names:
>> +    maxItems: 2
>> +    items:
>> +      - const: cnv
>> +        description: PWM connected to the CNV input on the ADC.
>> +      - const: clk_gate
>> +        description: PWM that gates the clock connected to the ADC's CLK input.
>> +
>> +  io-backends:
>> +    description:
>> +      The AXI ADC IP block connected to the D+/- and DCO+/- lines of the ADC.
>> +    maxItems: 1
>> +
>> +  adi,en0-always-on:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      Indicates if EN0 is hard-wired to the high state. If neither this
>> +      nor en0-gpios are present, then EN0 is hard-wired low.
>> +
>> +  adi,en1-always-on:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      Indicates if EN1 is hard-wired to the high state. If neither this
>> +      nor en1-gpios are present, then EN1 is hard-wired low.
>> +
>> +  adi,en2-always-on:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      Indicates if EN2 is hard-wired to the high state. If neither this
>> +      nor en2-gpios are present, then EN2 is hard-wired low.
>> +
>> +  adi,en3-always-on:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      Indicates if EN3 is hard-wired to the high state. If neither this
>> +      nor en3-gpios are present, then EN3 is hard-wired low.
>> +
>> +  en0-gpios:
>> +    description:
>> +      Configurable EN0 pin.
>> +
>> +  en1-gpios:
>> +    description:
>> +      Configurable EN1 pin.
>> +
>> +  en2-gpios:
>> +    description:
>> +      Configurable EN2 pin.
>> +
>> +  en3-gpios:
>> +    description:
>> +      Configurable EN3 pin.
>> +
>> +required:
>> +  - compatible
>> +  - vdd1-supply
>> +  - vdd2-supply
>> +  - vio-supply
>> +  - clocks
>> +  - pwms
>> +  - pwm-names
>> +  - io-backends
>> +
>> +- if:
>> +  properties:
> I don't think this was ever tested. Please use existing bindings or
> example-schema as template.

Ok, I'll look into it.

Thanks!

>
>> +    compatible:
>> +      contains:
>> +        enum:
>> +	  - adi,ad7625
>> +	  - adi,ad7626
>
> Best regards,
> Krzysztof
>

Re: [PATCH RFC 1/3] dt-bindings: iio: adc: add AD762x/AD796x ADCs

[Krzysztof Kozlowski]

On 31/07/2024 17:22, Trevor Gamblin wrote:
> 
> On 2024-07-31 10:11 a.m., Krzysztof Kozlowski wrote:
>> On 31/07/2024 15:48, Trevor Gamblin wrote:
>>> This adds a binding specification for the Analog Devices Inc. AD7625,
>>> AD7626, AD7960, and AD7961 ADCs.
>> Please do not use "This commit/patch/change", but imperative mood. See
>> longer explanation here:
>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> Will do.
>>
>> Why this is not ready, but RFC? What exactly needs to be commented here?
> There's one outstanding question about whether or not there should be a 
> DT property for specifying whether DCO+/- lines are connected (mentioned 
> in the cover letter but not here). I guess it doesn't need to be an RFC 
> just for that.

RFC means patch is not ready for review and you just ask for some
comments. Some maintainers even ignore RFC and wait till you send
something ready.

Best regards,
Krzysztof

Re: [PATCH RFC 1/3] dt-bindings: iio: adc: add AD762x/AD796x ADCs

[Rob Herring (Arm)]

On Wed, 31 Jul 2024 09:48:03 -0400, Trevor Gamblin wrote:
> This adds a binding specification for the Analog Devices Inc. AD7625,
> AD7626, AD7960, and AD7961 ADCs.
> 
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7625.yaml    | 176 +++++++++++++++++++++
>  MAINTAINERS                                        |   9 ++
>  2 files changed, 185 insertions(+)
> 

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

yamllint warnings/errors:
./Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml:120:1: [error] syntax error: expected <block end>, but found '-' (syntax)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/iio/adc/adi,ad7625.example.dts'
Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml:120:1: did not find expected key
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/iio/adc/adi,ad7625.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml:120:1: did not find expected key
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240731-ad7625_r1-v1-1-a1efef5a2ab9@baylibre.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 RFC 1/3] dt-bindings: iio: adc: add AD762x/AD796x ADCs

[Jonathan Cameron]

On Wed, 31 Jul 2024 09:48:03 -0400
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> This adds a binding specification for the Analog Devices Inc. AD7625,
> AD7626, AD7960, and AD7961 ADCs.

Given the RFC question is effectively about the binding and may influence
it a lot - make sure it's talked about here!

> 
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7625.yaml    | 176 +++++++++++++++++++++
>  MAINTAINERS                                        |   9 ++
>  2 files changed, 185 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
> new file mode 100644
> index 000000000000..e88db0ac2534
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
> @@ -0,0 +1,176 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7625.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices Fast PulSAR Analog to Digital Converters
> +
> +maintainers:
> +  - Michael Hennerich <Michael.Hennerich@analog.com>
> +  - Nuno Sá <nuno.sa@analog.com>
> +
> +description: |
> +  A family of single channel differential analog to digital converters
> +  in a LFCSP package. Note that these bindings are for the device when
> +  used with the PulSAR LVDS project:
> +  http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html.

As per the discussion in the cover letter I think the need to represent
if the DCO+ is connected between ADC and LVDS converter strongly suggests
we shouldn't represent it as one aggregate device.

> +
> +  * https://www.analog.com/en/products/ad7625.html
> +  * https://www.analog.com/en/products/ad7626.html
> +  * https://www.analog.com/en/products/ad7960.html
> +  * https://www.analog.com/en/products/ad7961.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7625
> +      - adi,ad7626
> +      - adi,ad7960
> +      - adi,ad7961
> +
> +  vdd1-supply:
> +    description: A supply that powers the analog and digital circuitry.
Doesn't really tell us anything. I'd just go with
    vdd1-supply: true
    vdd2-supply: true
    vio-supply: true


> +
> +  vdd2-supply:
> +    description: A supply that powers the analog and digital circuitry.
> +
> +  vio-supply:
> +    description: A supply for the inputs and outputs.
> +
> +  ref-supply:
> +    description:
> +      Voltage regulator for the external reference voltage (REF).
> +
> +  refin-supply:
> +    description:
> +      Voltage regulator for the reference buffer input (REFIN).
> +
> +  clocks:
> +    description:
> +      The clock connected to the CLK pins, gated by the clk_gate PWM.
> +    maxItems: 1
> +
> +  pwms:
> +    maxItems: 2
> +
> +  pwm-names:
> +    maxItems: 2
> +    items:
> +      - const: cnv
> +        description: PWM connected to the CNV input on the ADC.
> +      - const: clk_gate
> +        description: PWM that gates the clock connected to the ADC's CLK input.
> +
> +  io-backends:
> +    description:
> +      The AXI ADC IP block connected to the D+/- and DCO+/- lines of the ADC.

So you have a backend. Great - we have something to indicate a connection
to or not for the DCO+/o lines.  It's a bit ugly to just repesent it as a clk
but that would I think work.

> +    maxItems: 1
> +
> +  adi,en0-always-on:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates if EN0 is hard-wired to the high state. If neither this
> +      nor en0-gpios are present, then EN0 is hard-wired low.
It's unfortunate there isn't a special 'fixed' gpio-chip option where we could
just query it is fixed and what the state of the pin is.  This is getting
quite common so would be good to have a better solution.

Linus, Bartosz - is there a better way to do this?

> +
> +  adi,en1-always-on:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates if EN1 is hard-wired to the high state. If neither this
> +      nor en1-gpios are present, then EN1 is hard-wired low.
> +
> +  adi,en2-always-on:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates if EN2 is hard-wired to the high state. If neither this
> +      nor en2-gpios are present, then EN2 is hard-wired low.
> +
> +  adi,en3-always-on:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Indicates if EN3 is hard-wired to the high state. If neither this
> +      nor en3-gpios are present, then EN3 is hard-wired low.
> +
> +  en0-gpios:
> +    description:
> +      Configurable EN0 pin.
> +
> +  en1-gpios:
> +    description:
> +      Configurable EN1 pin.
> +
> +  en2-gpios:
> +    description:
> +      Configurable EN2 pin.
> +
> +  en3-gpios:
> +    description:
> +      Configurable EN3 pin.
> +
> +required:
> +  - compatible
> +  - vdd1-supply
> +  - vdd2-supply
> +  - vio-supply
> +  - clocks
> +  - pwms
> +  - pwm-names
> +  - io-backends
> +
> +- if:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +	  - adi,ad7625
> +	  - adi,ad7626
> +  then:
> +    properties:
> +      en2-gpios: false
> +      en3-gpios: false
> +      adi,en2-always-on: false
> +      adi,en3-always-on: false
> +    allOf:
> +      # ref-supply and refin-supply are mutually-exclusive (neither is also
> +      # valid)
> +      - if:
> +          required:
> +            - ref-supply
> +        then:
> +          properties:
> +            refin-supply: false
> +      - if:
> +          required:
> +            - refin-supply
> +        then:
> +          properties:
> +            ref-supply: false
> +
> +- if:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +	  - adi,ad7960
> +	  - adi,ad7961
> +  then:
> +    oneOf:
> +      required:
> +        - ref-supply
> +      required:
> +        - refin-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    adc {
> +        compatible = "adi,ad7625";
> +        vdd1-supply = <&supply_5V>;
> +        vdd2-supply = <&supply_2_5V>;
> +        vio-supply = <&supply_2_5V>;
> +        io-backends = <&axi_adc>;
> +        clock = <&ref_clk>;
> +        pwms = <&axi_pwm_gen 0 0>, <&axi_pwm_gen 1 0>;
> +        pwm-names = "cnv", "clk_gate";
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 42decde38320..2361f92751dd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1260,6 +1260,15 @@ F:	Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
>  F:	drivers/iio/addac/ad74413r.c
>  F:	include/dt-bindings/iio/addac/adi,ad74413r.h
>  
> +ANALOG DEVICES INC AD7625 DRIVER
> +M:	Michael Hennerich <Michael.Hennerich@analog.com>
> +M:	Nuno Sá <nuno.sa@analog.com>
> +R:	Trevor Gamblin <tgamblin@baylibre.com>
> +S:	Supported
> +W:	https://ez.analog.com/linux-software-drivers
> +W:	http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html
> +F:	Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
> +
>  ANALOG DEVICES INC AD7768-1 DRIVER
>  M:	Michael Hennerich <Michael.Hennerich@analog.com>
>  L:	linux-iio@vger.kernel.org
> 

Re: [PATCH RFC 1/3] dt-bindings: iio: adc: add AD762x/AD796x ADCs

[Trevor Gamblin]

Hello,

On 2024-08-03 10:35 a.m., Jonathan Cameron wrote:
> On Wed, 31 Jul 2024 09:48:03 -0400
> Trevor Gamblin <tgamblin@baylibre.com> wrote:
>
>> This adds a binding specification for the Analog Devices Inc. AD7625,
>> AD7626, AD7960, and AD7961 ADCs.
> Given the RFC question is effectively about the binding and may influence
> it a lot - make sure it's talked about here!
>
>> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
>> ---
>>   .../devicetree/bindings/iio/adc/adi,ad7625.yaml    | 176 +++++++++++++++++++++
>>   MAINTAINERS                                        |   9 ++
>>   2 files changed, 185 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
>> new file mode 100644
>> index 000000000000..e88db0ac2534
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
>> @@ -0,0 +1,176 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7625.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Analog Devices Fast PulSAR Analog to Digital Converters
>> +
>> +maintainers:
>> +  - Michael Hennerich <Michael.Hennerich@analog.com>
>> +  - Nuno Sá <nuno.sa@analog.com>
>> +
>> +description: |
>> +  A family of single channel differential analog to digital converters
>> +  in a LFCSP package. Note that these bindings are for the device when
>> +  used with the PulSAR LVDS project:
>> +  http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html.
> As per the discussion in the cover letter I think the need to represent
> if the DCO+ is connected between ADC and LVDS converter strongly suggests
> we shouldn't represent it as one aggregate device.

Just to be sure, do you mean that the PulSAR LVDS functionality should 
be split into its own driver and then utilized by ad7625?

Thank you for the feedback. I'll work on updates for all of your replies.

- Trevor

>
>> +
>> +  * https://www.analog.com/en/products/ad7625.html
>> +  * https://www.analog.com/en/products/ad7626.html
>> +  * https://www.analog.com/en/products/ad7960.html
>> +  * https://www.analog.com/en/products/ad7961.html
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - adi,ad7625
>> +      - adi,ad7626
>> +      - adi,ad7960
>> +      - adi,ad7961
>> +
>> +  vdd1-supply:
>> +    description: A supply that powers the analog and digital circuitry.
> Doesn't really tell us anything. I'd just go with
>      vdd1-supply: true
>      vdd2-supply: true
>      vio-supply: true
>
>
>> +
>> +  vdd2-supply:
>> +    description: A supply that powers the analog and digital circuitry.
>> +
>> +  vio-supply:
>> +    description: A supply for the inputs and outputs.
>> +
>> +  ref-supply:
>> +    description:
>> +      Voltage regulator for the external reference voltage (REF).
>> +
>> +  refin-supply:
>> +    description:
>> +      Voltage regulator for the reference buffer input (REFIN).
>> +
>> +  clocks:
>> +    description:
>> +      The clock connected to the CLK pins, gated by the clk_gate PWM.
>> +    maxItems: 1
>> +
>> +  pwms:
>> +    maxItems: 2
>> +
>> +  pwm-names:
>> +    maxItems: 2
>> +    items:
>> +      - const: cnv
>> +        description: PWM connected to the CNV input on the ADC.
>> +      - const: clk_gate
>> +        description: PWM that gates the clock connected to the ADC's CLK input.
>> +
>> +  io-backends:
>> +    description:
>> +      The AXI ADC IP block connected to the D+/- and DCO+/- lines of the ADC.
> So you have a backend. Great - we have something to indicate a connection
> to or not for the DCO+/o lines.  It's a bit ugly to just repesent it as a clk
> but that would I think work.
>
>> +    maxItems: 1
>> +
>> +  adi,en0-always-on:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      Indicates if EN0 is hard-wired to the high state. If neither this
>> +      nor en0-gpios are present, then EN0 is hard-wired low.
> It's unfortunate there isn't a special 'fixed' gpio-chip option where we could
> just query it is fixed and what the state of the pin is.  This is getting
> quite common so would be good to have a better solution.
>
> Linus, Bartosz - is there a better way to do this?
>
>> +
>> +  adi,en1-always-on:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      Indicates if EN1 is hard-wired to the high state. If neither this
>> +      nor en1-gpios are present, then EN1 is hard-wired low.
>> +
>> +  adi,en2-always-on:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      Indicates if EN2 is hard-wired to the high state. If neither this
>> +      nor en2-gpios are present, then EN2 is hard-wired low.
>> +
>> +  adi,en3-always-on:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      Indicates if EN3 is hard-wired to the high state. If neither this
>> +      nor en3-gpios are present, then EN3 is hard-wired low.
>> +
>> +  en0-gpios:
>> +    description:
>> +      Configurable EN0 pin.
>> +
>> +  en1-gpios:
>> +    description:
>> +      Configurable EN1 pin.
>> +
>> +  en2-gpios:
>> +    description:
>> +      Configurable EN2 pin.
>> +
>> +  en3-gpios:
>> +    description:
>> +      Configurable EN3 pin.
>> +
>> +required:
>> +  - compatible
>> +  - vdd1-supply
>> +  - vdd2-supply
>> +  - vio-supply
>> +  - clocks
>> +  - pwms
>> +  - pwm-names
>> +  - io-backends
>> +
>> +- if:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        enum:
>> +	  - adi,ad7625
>> +	  - adi,ad7626
>> +  then:
>> +    properties:
>> +      en2-gpios: false
>> +      en3-gpios: false
>> +      adi,en2-always-on: false
>> +      adi,en3-always-on: false
>> +    allOf:
>> +      # ref-supply and refin-supply are mutually-exclusive (neither is also
>> +      # valid)
>> +      - if:
>> +          required:
>> +            - ref-supply
>> +        then:
>> +          properties:
>> +            refin-supply: false
>> +      - if:
>> +          required:
>> +            - refin-supply
>> +        then:
>> +          properties:
>> +            ref-supply: false
>> +
>> +- if:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        enum:
>> +	  - adi,ad7960
>> +	  - adi,ad7961
>> +  then:
>> +    oneOf:
>> +      required:
>> +        - ref-supply
>> +      required:
>> +        - refin-supply
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    adc {
>> +        compatible = "adi,ad7625";
>> +        vdd1-supply = <&supply_5V>;
>> +        vdd2-supply = <&supply_2_5V>;
>> +        vio-supply = <&supply_2_5V>;
>> +        io-backends = <&axi_adc>;
>> +        clock = <&ref_clk>;
>> +        pwms = <&axi_pwm_gen 0 0>, <&axi_pwm_gen 1 0>;
>> +        pwm-names = "cnv", "clk_gate";
>> +    };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 42decde38320..2361f92751dd 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1260,6 +1260,15 @@ F:	Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
>>   F:	drivers/iio/addac/ad74413r.c
>>   F:	include/dt-bindings/iio/addac/adi,ad74413r.h
>>   
>> +ANALOG DEVICES INC AD7625 DRIVER
>> +M:	Michael Hennerich <Michael.Hennerich@analog.com>
>> +M:	Nuno Sá <nuno.sa@analog.com>
>> +R:	Trevor Gamblin <tgamblin@baylibre.com>
>> +S:	Supported
>> +W:	https://ez.analog.com/linux-software-drivers
>> +W:	http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html
>> +F:	Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
>> +
>>   ANALOG DEVICES INC AD7768-1 DRIVER
>>   M:	Michael Hennerich <Michael.Hennerich@analog.com>
>>   L:	linux-iio@vger.kernel.org
>>

Re: [PATCH RFC 1/3] dt-bindings: iio: adc: add AD762x/AD796x ADCs

[Jonathan Cameron]

On Tue, 6 Aug 2024 09:17:45 -0400
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> Hello,
> 
> On 2024-08-03 10:35 a.m., Jonathan Cameron wrote:
> > On Wed, 31 Jul 2024 09:48:03 -0400
> > Trevor Gamblin <tgamblin@baylibre.com> wrote:
> >  
> >> This adds a binding specification for the Analog Devices Inc. AD7625,
> >> AD7626, AD7960, and AD7961 ADCs.  
> > Given the RFC question is effectively about the binding and may influence
> > it a lot - make sure it's talked about here!
> >  
> >> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> >> ---
> >>   .../devicetree/bindings/iio/adc/adi,ad7625.yaml    | 176 +++++++++++++++++++++
> >>   MAINTAINERS                                        |   9 ++
> >>   2 files changed, 185 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
> >> new file mode 100644
> >> index 000000000000..e88db0ac2534
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
> >> @@ -0,0 +1,176 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7625.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Analog Devices Fast PulSAR Analog to Digital Converters
> >> +
> >> +maintainers:
> >> +  - Michael Hennerich <Michael.Hennerich@analog.com>
> >> +  - Nuno Sá <nuno.sa@analog.com>
> >> +
> >> +description: |
> >> +  A family of single channel differential analog to digital converters
> >> +  in a LFCSP package. Note that these bindings are for the device when
> >> +  used with the PulSAR LVDS project:
> >> +  http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html.  
> > As per the discussion in the cover letter I think the need to represent
> > if the DCO+ is connected between ADC and LVDS converter strongly suggests
> > we shouldn't represent it as one aggregate device.  
> 
> Just to be sure, do you mean that the PulSAR LVDS functionality should 
> be split into its own driver and then utilized by ad7625?

I think we'd need that just to provide a 'hook' to describe the wiring.
That driver probably won't do anything other than give that information.

I'm not quite sure how it would all fit together though. Would need
some experimentation to figure out. I can't immediately think of
another bus with optional wires like this.  Anyone else have a suggestion
on where to look?

Jonathan

[PATCH RFC 2/3] iio: adc: ad7625: add driver

[Trevor Gamblin]

This adds a driver for the ad762x and ad796x family of ADCs. These are
pin-compatible devices using an LVDS interface for data transfer,
capable of sampling at rates of 6 and 10 MSPS, respectively. They also
feature multiple voltage reference options based on the configuration of
the EN1/EN0 pins, which can be set in the devicetree.

Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
 MAINTAINERS              |   1 +
 drivers/iio/adc/Kconfig  |  15 ++
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad7625.c | 626 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 643 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2361f92751dd..a90972e1c5c5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1268,6 +1268,7 @@ S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 W:	http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
+F:	drivers/iio/adc/ad7625.c
 
 ANALOG DEVICES INC AD7768-1 DRIVER
 M:	Michael Hennerich <Michael.Hennerich@analog.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f60fe85a30d5..e25fb505f545 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -219,6 +219,21 @@ config AD7606_IFACE_SPI
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad7606_spi.
 
+config AD7625
+        tristate "Analog Devices AD7625/AD7626 High Speed ADC driver"
+        select IIO_BACKEND
+        help
+          Say yes here to build support for Analog Devices:
+	  * AD7625 16-Bit, 6 MSPS PulSAR Analog-to-Digital Converter
+	  * AD7626 16-Bit, 10 MSPS PulSAR Analog-to-Digital Converter
+	  * AD7960 18-Bit, 5 MSPS PulSAR Analog-to-Digital Converter
+	  * AD7961 16-Bit, 5 MSPS PulSAR Analog-to-Digital Converter
+
+          The driver requires the assistance of the AXI ADC IP core to operate.
+
+          To compile this driver as a module, choose M here: the module will be
+          called ad7625.
+
 config AD7766
 	tristate "Analog Devices AD7766/AD7767 ADC driver"
 	depends on SPI_MASTER
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d370e066544e..6bf429ca24ea 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_AD7476) += ad7476.o
 obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
 obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
 obj-$(CONFIG_AD7606) += ad7606.o
+obj-$(CONFIG_AD7625) += ad7625.o
 obj-$(CONFIG_AD7766) += ad7766.o
 obj-$(CONFIG_AD7768_1) += ad7768-1.o
 obj-$(CONFIG_AD7780) += ad7780.o
diff --git a/drivers/iio/adc/ad7625.c b/drivers/iio/adc/ad7625.c
new file mode 100644
index 000000000000..b74760c2fee2
--- /dev/null
+++ b/drivers/iio/adc/ad7625.c
@@ -0,0 +1,626 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+/*
+ * Analog Devices Inc. AD7625 ADC driver
+ *
+ * Copyright 2024 Analog Devices Inc.
+ * Copyright 2024 BayLibre, SAS
+ *
+ * Note that this driver requires the AXI ADC IP block configured for
+ * LVDS to function. See Documentation/iio/ad7625.rst for more information.
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/iio/backend.h>
+#include <linux/iio/iio.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.h>
+
+#define AD7625_INTERNAL_REF_MV 4096
+#define AD7960_MAX_NBW_FREQ (2 * MEGA)
+
+struct ad7625_timing_spec {
+	/* Max conversion high time (t_{CNVH}). */
+	unsigned int conv_high_ns;
+	/* Max conversion to MSB delay (t_{MSB}). */
+	unsigned int conv_msb_ns;
+};
+
+struct ad7625_chip_info {
+	const char *name;
+	const unsigned int max_sample_rate_hz;
+	const struct ad7625_timing_spec *timing_spec;
+	const struct iio_chan_spec chan_spec;
+	const bool has_power_down_state;
+	const bool has_bandwidth_control;
+	const bool has_internal_vref;
+};
+
+/* AD7625_CHAN_SPEC - Define a chan spec structure for a specific chip */
+#define AD7625_CHAN_SPEC(_bits) {					\
+	.type = IIO_VOLTAGE,						\
+	.indexed = 1,							\
+	.differential = 1,						\
+	.channel = 0,							\
+	.channel2 = 1,							\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_SCALE),			\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = 0,						\
+	.scan_type.sign = 's',						\
+	.scan_type.storagebits = _bits > 16 ? 32 : 16,			\
+	.scan_type.realbits = _bits,					\
+}
+
+struct ad7625_state {
+	const struct ad7625_chip_info *info;
+	struct iio_backend *back;
+	/* rate of the clock gated by the "clk_gate" PWM */
+	unsigned long ref_clk_rate_hz;
+	/* PWM burst signal for transferring acquired data to the host */
+	struct pwm_device *clk_gate_pwm;
+	/*
+	 * PWM control signal for initiating data conversion. Analog
+	 * inputs are sampled beginning on this signal's rising edge.
+	 */
+	struct pwm_device *cnv_pwm;
+	unsigned int vref_mv;
+	int sampling_freq_hz;
+	/*
+	 * Optional GPIOs for controlling device state. EN0 and EN1
+	 * determine voltage reference configuration and on/off state.
+	 * EN2 controls the device -3dB bandwidth (and by extension, max
+	 * sample rate). EN3 controls the VCM reference output. EN2 and
+	 * EN3 are only present for the AD796x devices.
+	 */
+	struct gpio_desc *en_gpios[4];
+	bool can_power_down;
+	bool can_refin;
+	bool can_ref_4v096;
+	/*
+	 * Indicate whether the bandwidth can be narrow (9MHz).
+	 * When true, device sample rate must also be < 2MSPS.
+	 */
+	bool can_narrow_bandwidth;
+	/* Indicate whether the bandwidth can be wide (28MHz). */
+	bool can_wide_bandwidth;
+	bool can_ref_5v;
+	bool can_snooze;
+	bool can_test_pattern;
+};
+
+static const struct ad7625_timing_spec ad7625_timing_spec = {
+	.conv_high_ns = 40,
+	.conv_msb_ns = 145,
+};
+
+static const struct ad7625_timing_spec ad7626_timing_spec = {
+	.conv_high_ns = 40,
+	.conv_msb_ns = 80,
+};
+
+/*
+ * conv_msb_ns is set to 0 instead of the datasheet maximum of 200ns to
+ * avoid exceeding the minimum conversion time, i.e. it is effectively
+ * modulo 200 and offset by a full period. Values greater than or equal
+ * to the period would be rejected by the PWM API.
+ */
+static const struct ad7625_timing_spec ad7960_timing_spec = {
+	.conv_high_ns = 80,
+	.conv_msb_ns = 0,
+};
+
+static const struct ad7625_chip_info ad7625_chip_info = {
+	.name = "ad7625",
+	.max_sample_rate_hz = 6 * MEGA,
+	.timing_spec = &ad7625_timing_spec,
+	.chan_spec = AD7625_CHAN_SPEC(16),
+	.has_power_down_state = false,
+	.has_bandwidth_control = false,
+	.has_internal_vref = true,
+};
+
+static const struct ad7625_chip_info ad7626_chip_info = {
+	.name = "ad7626",
+	.max_sample_rate_hz = 10 * MEGA,
+	.timing_spec = &ad7626_timing_spec,
+	.chan_spec = AD7625_CHAN_SPEC(16),
+	.has_power_down_state = true,
+	.has_bandwidth_control = false,
+	.has_internal_vref = true,
+};
+
+static const struct ad7625_chip_info ad7960_chip_info = {
+	.name = "ad7960",
+	.max_sample_rate_hz = 5 * MEGA,
+	.timing_spec = &ad7960_timing_spec,
+	.chan_spec = AD7625_CHAN_SPEC(18),
+	.has_power_down_state = true,
+	.has_bandwidth_control = true,
+	.has_internal_vref = false,
+};
+
+static const struct ad7625_chip_info ad7961_chip_info = {
+	.name = "ad7961",
+	.max_sample_rate_hz = 5 * MEGA,
+	.timing_spec = &ad7960_timing_spec,
+	.chan_spec = AD7625_CHAN_SPEC(16),
+	.has_power_down_state = true,
+	.has_bandwidth_control = true,
+	.has_internal_vref = false,
+};
+
+enum ad7960_mode {
+	AD7960_MODE_POWER_DOWN,
+	AD7960_MODE_SNOOZE,
+	AD7960_MODE_NARROW_BANDWIDTH,
+	AD7960_MODE_WIDE_BANDWIDTH,
+	AD7960_MODE_TEST_PATTERN,
+};
+
+static void ad7625_pwm_disable(void *data)
+{
+	pwm_disable(data);
+}
+
+static int ad7625_set_sampling_freq(struct ad7625_state *st, int freq)
+{
+	u64 target;
+	struct pwm_waveform clk_gate_wf = { }, cnv_wf = { };
+	int ret;
+
+	cnv_wf.period_length_ns = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
+
+	/*
+	 * Make sure that the CNV period doesn't exceed the datasheet
+	 * maximum
+	 */
+	cnv_wf.period_length_ns = clamp(cnv_wf.period_length_ns, 100, 10 * KILO);
+
+	/*
+	 * Use the maximum conversion time t_CNVH from the datasheet as
+	 * the duty_cycle for ref_clk, cnv, and clk_gate
+	 */
+	cnv_wf.duty_length_ns = st->info->timing_spec->conv_high_ns;
+
+	ret = pwm_set_waveform_might_sleep(st->cnv_pwm, &cnv_wf, false);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Set up the burst signal for transferring data. period and
+	 * offset should mirror the CNV signal
+	 */
+	clk_gate_wf.period_length_ns = cnv_wf.period_length_ns;
+
+	clk_gate_wf.duty_length_ns = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC *
+		st->info->chan_spec.scan_type.realbits,
+		st->ref_clk_rate_hz);
+
+	/* max t_MSB from datasheet */
+	clk_gate_wf.duty_offset_ns = st->info->timing_spec->conv_msb_ns;
+
+	ret = pwm_set_waveform_might_sleep(st->clk_gate_pwm, &clk_gate_wf, false);
+	if (ret < 0)
+		return ret;
+
+	/* TODO: Add a rounding API for PWMs that can simplify this */
+	target = DIV_ROUND_CLOSEST_ULL(st->ref_clk_rate_hz, freq);
+	st->sampling_freq_hz = DIV_ROUND_CLOSEST_ULL(st->ref_clk_rate_hz, target);
+
+	return 0;
+}
+
+static int devm_ad7625_pwm_get(struct device *dev, struct clk *ref_clk,
+				       struct ad7625_state *st)
+{
+	unsigned long ref_clk_rate_hz;
+	int ret;
+
+	st->cnv_pwm = devm_pwm_get(dev, "cnv");
+	if (IS_ERR(st->cnv_pwm))
+		return dev_err_probe(dev, PTR_ERR(st->cnv_pwm),
+				     "failed to get cnv pwm\n");
+
+	ret = devm_add_action_or_reset(dev, ad7625_pwm_disable, st->cnv_pwm);
+	if (ret)
+		return ret;
+
+	st->clk_gate_pwm = devm_pwm_get(dev, "clk_gate");
+	if (IS_ERR(st->clk_gate_pwm))
+		return dev_err_probe(dev, PTR_ERR(st->clk_gate_pwm),
+				     "failed to get clk_gate pwm\n");
+
+	ret = devm_add_action_or_reset(dev, ad7625_pwm_disable,
+				       st->clk_gate_pwm);
+	if (ret)
+		return ret;
+
+	ref_clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(ref_clk))
+		return dev_err_probe(dev, PTR_ERR(ref_clk),
+				     "failed to get ref_clk");
+
+	ref_clk_rate_hz = clk_get_rate(ref_clk);
+	if (!ref_clk_rate_hz)
+		return dev_err_probe(dev, -EINVAL,
+				     "failed to get ref_clk rate");
+
+	st->ref_clk_rate_hz = ref_clk_rate_hz;
+
+	return 0;
+}
+
+static int ad7625_read_raw(struct iio_dev *indio_dev,
+			   const struct iio_chan_spec *chan,
+			   int *val, int *val2, long info)
+{
+	struct ad7625_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->sampling_freq_hz;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->vref_mv;
+		*val2 = chan->scan_type.realbits - 1;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad7625_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long info)
+{
+	struct ad7625_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+			return ad7625_set_sampling_freq(st, val);
+		unreachable();
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info ad7625_info = {
+	.read_raw = ad7625_read_raw,
+	.write_raw = ad7625_write_raw,
+};
+
+static int ad7625_parse_mode(struct device *dev, struct ad7625_state *st, int num_gpios)
+{
+	bool en_always_on[4], en_always_off[4],
+	     en_may_be_on[4], en_may_be_off[4];
+	char en_gpio_buf[4];
+	char always_on_buf[18];
+	int i;
+
+	for (i = 0; i < num_gpios; i++) {
+		snprintf(en_gpio_buf, sizeof(en_gpio_buf), "en%d", i);
+		snprintf(always_on_buf, sizeof(always_on_buf),
+			 "adi,en%d-always-on", i);
+		/* Set the device to 0b0000 (power-down mode) by default */
+		st->en_gpios[i] = devm_gpiod_get_optional(dev, en_gpio_buf,
+							  GPIOD_OUT_LOW);
+		if (IS_ERR(st->en_gpios[i]))
+			return dev_err_probe(dev, PTR_ERR(st->en_gpios[i]),
+					     "failed to get EN%d GPIO\n", i);
+
+		en_always_on[i] = device_property_present(dev, always_on_buf);
+		if (st->en_gpios[i] && en_always_on[i])
+			return dev_err_probe(dev, -EINVAL,
+				"cannot have both adi,en%d-always-on and en%d-gpios\n", i, i);
+
+		en_may_be_off[i] = !en_always_on[i];
+		en_may_be_on[i] = en_always_on[i] || st->en_gpios[i];
+		en_always_off[i] = !en_always_on[i] && !st->en_gpios[i];
+	}
+
+	/*
+	 * Power down is mode 0bXX00, but not all devices have a valid
+	 * power down state.
+	 */
+	st->can_power_down = en_may_be_off[1] && en_may_be_off[0] &&
+			     st->info->has_power_down_state;
+	/*
+	 * The REFIN pin can take a 1.2V (AD762x) or 2.048V (AD796x)
+	 * external reference when the mode is 0bXX01.
+	 */
+	st->can_refin = en_may_be_off[1] && en_may_be_on[0];
+	/* 4.096V can be applied to REF when the EN mode is 0bXX10. */
+	st->can_ref_4v096 = en_may_be_on[1] && en_may_be_off[0];
+
+	/* Avoid AD796x-specific setup if the part is an AD762x */
+	if (num_gpios == 2)
+		return 0;
+
+	/* mode 0b1100 (AD796x) is invalid */
+	if (en_always_on[3] && en_always_on[2] && en_always_off[1] && en_always_off[0])
+		return dev_err_probe(dev, -EINVAL,
+				     "EN GPIOs set to invalid mode 0b1100\n");
+	/*
+	 * 5V can be applied to the AD796x REF pin when the EN mode is
+	 * the same (0bX001 or 0bX101) as for can_refin, and REFIN is
+	 * 0V.
+	 */
+	st->can_ref_5v = st->can_refin;
+	/*
+	 * Bandwidth (AD796x) is controlled solely by EN2. If it's
+	 * specified and not hard-wired, then we can configure it to
+	 * change the bandwidth between 28MHz and 9MHz.
+	 */
+	st->can_narrow_bandwidth = en_may_be_on[2];
+	/* Wide bandwidth mode is possible if EN2 can be 0. */
+	st->can_wide_bandwidth = en_may_be_off[2];
+	/* Snooze mode (AD796x) is 0bXX11 when REFIN = 0V. */
+	st->can_snooze = en_may_be_on[1] && en_may_be_on[0];
+	/* Test pattern mode (AD796x) is 0b0100. */
+	st->can_test_pattern = en_may_be_off[3] && en_may_be_on[2] &&
+			       en_may_be_off[1] && en_may_be_off[0];
+
+	return 0;
+}
+
+/* Set EN1 and EN0 based on reference voltage source */
+static void ad7625_set_en_gpios_for_vref(struct ad7625_state *st, bool have_refin, int ref_mv)
+{
+	if (have_refin || ref_mv == 5000) {
+		gpiod_set_value_cansleep(st->en_gpios[1], 0);
+		gpiod_set_value_cansleep(st->en_gpios[0], 1);
+	} else if (ref_mv == 4096) {
+		gpiod_set_value_cansleep(st->en_gpios[1], 1);
+		gpiod_set_value_cansleep(st->en_gpios[0], 0);
+	/*
+	 * Unreachable by AD796x, since the driver will error if neither
+	 * REF nor REFIN is provided
+	 */
+	} else {
+		gpiod_set_value_cansleep(st->en_gpios[1], 1);
+		gpiod_set_value_cansleep(st->en_gpios[0], 1);
+	}
+}
+
+static int ad7960_set_mode(struct ad7625_state *st, enum ad7960_mode mode,
+			   bool have_refin, int ref_mv)
+{
+	switch (mode) {
+	case AD7960_MODE_POWER_DOWN:
+		if (!st->can_power_down)
+			return -EINVAL;
+
+		gpiod_set_value_cansleep(st->en_gpios[2], 0);
+		gpiod_set_value_cansleep(st->en_gpios[1], 0);
+		gpiod_set_value_cansleep(st->en_gpios[0], 0);
+		return 0;
+
+	case AD7960_MODE_SNOOZE:
+		if (!st->can_snooze)
+			return -EINVAL;
+
+		gpiod_set_value_cansleep(st->en_gpios[1], 1);
+		gpiod_set_value_cansleep(st->en_gpios[0], 1);
+		return 0;
+
+	case AD7960_MODE_NARROW_BANDWIDTH:
+		if (!st->can_narrow_bandwidth)
+			return -EINVAL;
+
+		gpiod_set_value_cansleep(st->en_gpios[2], 1);
+		ad7625_set_en_gpios_for_vref(st, have_refin, ref_mv);
+		return 0;
+
+	case AD7960_MODE_WIDE_BANDWIDTH:
+		if (!st->can_wide_bandwidth)
+			return -EINVAL;
+
+		gpiod_set_value_cansleep(st->en_gpios[2], 0);
+		ad7625_set_en_gpios_for_vref(st, have_refin, ref_mv);
+		return 0;
+
+	case AD7960_MODE_TEST_PATTERN:
+		if (!st->can_test_pattern)
+			return -EINVAL;
+
+		gpiod_set_value_cansleep(st->en_gpios[3], 0);
+		gpiod_set_value_cansleep(st->en_gpios[2], 1);
+		gpiod_set_value_cansleep(st->en_gpios[1], 0);
+		gpiod_set_value_cansleep(st->en_gpios[0], 0);
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad7625_probe(struct platform_device *pdev)
+{
+	/*
+	 * Power-up info for the device says to bring up vio, then
+	 * vdd2, then vdd1
+	 */
+	static const char * const regulator_names[] = { "vio", "vdd2", "vdd1" };
+	struct device *dev = &pdev->dev;
+	struct iio_dev *indio_dev;
+	struct ad7625_state *st;
+	struct clk *ref_clk;
+	int ret, ref_mv;
+	int default_sample_freq;
+	bool have_refin;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	st->info = device_get_match_data(dev);
+	if (!st->info)
+		return dev_err_probe(dev, -EINVAL, "no chip info\n");
+
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
+		regulator_names);
+	if (ret)
+		return ret;
+
+	if (st->info->has_bandwidth_control)
+		ret = ad7625_parse_mode(dev, st, 4);
+	else
+		ret = ad7625_parse_mode(dev, st, 2);
+
+	if (ret)
+		return ret;
+
+	/*
+	 * Determine the source of the reference voltage:
+	 * - internal reference: neither REF or REFIN is connected
+	 *   (invalid for AD796x)
+	 * - internal buffer, external reference: REF not connected,
+	 *   is REFIN connected
+	 * - external reference: REF connected, REFIN not connected
+	 */
+	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "failed to get REF voltage\n");
+
+	ref_mv = ret == -ENODEV ? 0 : ret / 1000;
+
+	ret = devm_regulator_get_enable_optional(dev, "refin");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "failed to get REFIN voltage\n");
+
+	have_refin = ret != -ENODEV;
+
+	if (have_refin && !st->can_refin)
+		return dev_err_probe(dev, -EINVAL,
+				     "REFIN provided in unsupported mode\n");
+
+	if (!st->info->has_internal_vref && !have_refin && !ref_mv)
+		return dev_err_probe(dev, -EINVAL,
+				     "Need either REFIN or REF");
+
+	if (have_refin && ref_mv)
+		return dev_err_probe(dev, -EINVAL,
+				     "cannot have both REFIN and REF supplies\n");
+
+	st->vref_mv = ref_mv ?: AD7625_INTERNAL_REF_MV;
+
+	if (ref_mv == 4096 && !st->can_ref_4v096)
+		return dev_err_probe(dev, -EINVAL,
+				     "REF is 4.096V in unsupported mode\n");
+
+	if (ref_mv == 5000 && !st->can_ref_5v)
+		return dev_err_probe(dev, -EINVAL,
+				     "REF is 5V in unsupported mode\n");
+
+	/* Set the device mode based on detected EN configuration. */
+	if (!st->info->has_bandwidth_control) {
+		ad7625_set_en_gpios_for_vref(st, have_refin, ref_mv);
+	} else {
+		/*
+		 * If neither sampling mode is available, then report an error,
+		 * since the other modes are not useful defaults.
+		 */
+		if (st->can_wide_bandwidth) {
+			ret = ad7960_set_mode(st, AD7960_MODE_WIDE_BANDWIDTH,
+				have_refin, ref_mv);
+		} else if (st->can_narrow_bandwidth) {
+			ret = ad7960_set_mode(st, AD7960_MODE_NARROW_BANDWIDTH,
+				have_refin, ref_mv);
+		} else {
+			return dev_err_probe(dev, -EINVAL,
+				"couldn't set device to wide or narrow bandwidth modes\n");
+		}
+
+		if (ret)
+			return dev_err_probe(dev, -EINVAL,
+					     "failed to set EN pins\n");
+	}
+
+	ret = devm_ad7625_pwm_get(dev, ref_clk, st);
+	if (ret)
+		return dev_err_probe(dev, -EINVAL,
+				     "failed to set ref_clk_rate_hz\n");
+
+	indio_dev->channels = &st->info->chan_spec;
+	indio_dev->num_channels = 1;
+	indio_dev->name = st->info->name;
+	indio_dev->info = &ad7625_info;
+
+	st->back = devm_iio_backend_get(dev, NULL);
+	if (IS_ERR(st->back))
+		return dev_err_probe(dev, PTR_ERR(st->back),
+				     "failed to get IIO backend");
+
+	ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_backend_enable(dev, st->back);
+	if (ret)
+		return ret;
+
+	/*
+	 * Set the initial sampling frequency to the maximum, unless the
+	 * AD796x device is limited to narrow bandwidth, in which case
+	 * the sampling frequency should be limited to 2MSPS
+	 */
+	if (!st->info->has_bandwidth_control) {
+		default_sample_freq = st->info->max_sample_rate_hz;
+	} else {
+		default_sample_freq = !st->can_wide_bandwidth ? AD7960_MAX_NBW_FREQ :
+				      st->info->max_sample_rate_hz;
+	}
+
+	ret = ad7625_set_sampling_freq(st, default_sample_freq);
+	if (ret < 0)
+		dev_err_probe(dev, ret, "failed to set sampling frequency\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id ad7625_of_match[] = {
+	{ .compatible = "adi,ad7625", .data = &ad7625_chip_info, },
+	{ .compatible = "adi,ad7626", .data = &ad7626_chip_info, },
+	{ .compatible = "adi,ad7960", .data = &ad7960_chip_info, },
+	{ .compatible = "adi,ad7961", .data = &ad7961_chip_info, },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad7625_of_match);
+
+static const struct platform_device_id ad7625_device_ids[] = {
+	{ .name = "ad7625", .driver_data = (kernel_ulong_t)&ad7625_chip_info },
+	{ .name = "ad7626", .driver_data = (kernel_ulong_t)&ad7626_chip_info },
+	{ .name = "ad7960", .driver_data = (kernel_ulong_t)&ad7960_chip_info },
+	{ .name = "ad7961", .driver_data = (kernel_ulong_t)&ad7961_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, ad7625_device_ids);
+
+static struct platform_driver ad7625_driver = {
+	.probe = ad7625_probe,
+	.driver = {
+		.name = "ad7625",
+		.of_match_table = ad7625_of_match,
+	},
+	.id_table = ad7625_device_ids,
+};
+module_platform_driver(ad7625_driver);
+
+MODULE_AUTHOR("Trevor Gamblin <tgamblin@baylibre.com>");
+MODULE_DESCRIPTION("Analog Devices AD7625 ADC");
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_IMPORT_NS(IIO_BACKEND);

-- 
2.39.2

Re: [PATCH RFC 2/3] iio: adc: ad7625: add driver

[Jonathan Cameron]

On Wed, 31 Jul 2024 09:48:04 -0400
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> This adds a driver for the ad762x and ad796x family of ADCs. These are
> pin-compatible devices using an LVDS interface for data transfer,
> capable of sampling at rates of 6 and 10 MSPS, respectively. They also
> feature multiple voltage reference options based on the configuration of
> the EN1/EN0 pins, which can be set in the devicetree.
> 
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
Hi Trevor

A few comments inline,

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d370e066544e..6bf429ca24ea 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_AD7476) += ad7476.o
>  obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
>  obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
>  obj-$(CONFIG_AD7606) += ad7606.o
> +obj-$(CONFIG_AD7625) += ad7625.o
>  obj-$(CONFIG_AD7766) += ad7766.o
>  obj-$(CONFIG_AD7768_1) += ad7768-1.o
>  obj-$(CONFIG_AD7780) += ad7780.o
> diff --git a/drivers/iio/adc/ad7625.c b/drivers/iio/adc/ad7625.c
> new file mode 100644
> index 000000000000..b74760c2fee2
> --- /dev/null
> +++ b/drivers/iio/adc/ad7625.c
> @@ -0,0 +1,626 @@


> +
> +static int devm_ad7625_pwm_get(struct device *dev, struct clk *ref_clk,
> +				       struct ad7625_state *st)
> +{
> +	unsigned long ref_clk_rate_hz;
> +	int ret;
> +
> +	st->cnv_pwm = devm_pwm_get(dev, "cnv");
> +	if (IS_ERR(st->cnv_pwm))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv_pwm),
> +				     "failed to get cnv pwm\n");
> +
> +	ret = devm_add_action_or_reset(dev, ad7625_pwm_disable, st->cnv_pwm);

Add a comment on why we are disabling the pwm but don't seem to have enabled it.
If this is unwinding something later in probe, move it there.

btw, your cover letter has a link to that patch set mentioned, but I'm not
seeing an actual link.

> +	if (ret)
> +		return ret;
> +
> +	st->clk_gate_pwm = devm_pwm_get(dev, "clk_gate");
> +	if (IS_ERR(st->clk_gate_pwm))
> +		return dev_err_probe(dev, PTR_ERR(st->clk_gate_pwm),
> +				     "failed to get clk_gate pwm\n");
> +
> +	ret = devm_add_action_or_reset(dev, ad7625_pwm_disable,
> +				       st->clk_gate_pwm);
> +	if (ret)
> +		return ret;
> +
> +	ref_clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(ref_clk))
> +		return dev_err_probe(dev, PTR_ERR(ref_clk),
> +				     "failed to get ref_clk");
> +
> +	ref_clk_rate_hz = clk_get_rate(ref_clk);
> +	if (!ref_clk_rate_hz)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "failed to get ref_clk rate");
> +
> +	st->ref_clk_rate_hz = ref_clk_rate_hz;
> +
> +	return 0;
> +}


...

> +static int ad7625_parse_mode(struct device *dev, struct ad7625_state *st, int num_gpios)
> +{
> +	bool en_always_on[4], en_always_off[4],
> +	     en_may_be_on[4], en_may_be_off[4];
Add another bool. Same number of chars and I think a tiny bit easier to read.

> +	char en_gpio_buf[4];
> +	char always_on_buf[18];
> +	int i;
> +
> +	for (i = 0; i < num_gpios; i++) {
> +		snprintf(en_gpio_buf, sizeof(en_gpio_buf), "en%d", i);
> +		snprintf(always_on_buf, sizeof(always_on_buf),
> +			 "adi,en%d-always-on", i);
> +		/* Set the device to 0b0000 (power-down mode) by default */
> +		st->en_gpios[i] = devm_gpiod_get_optional(dev, en_gpio_buf,
> +							  GPIOD_OUT_LOW);
> +		if (IS_ERR(st->en_gpios[i]))
> +			return dev_err_probe(dev, PTR_ERR(st->en_gpios[i]),
> +					     "failed to get EN%d GPIO\n", i);
> +
> +		en_always_on[i] = device_property_present(dev, always_on_buf);
> +		if (st->en_gpios[i] && en_always_on[i])
> +			return dev_err_probe(dev, -EINVAL,
> +				"cannot have both adi,en%d-always-on and en%d-gpios\n", i, i);
> +
> +		en_may_be_off[i] = !en_always_on[i];
> +		en_may_be_on[i] = en_always_on[i] || st->en_gpios[i];
> +		en_always_off[i] = !en_always_on[i] && !st->en_gpios[i];
> +	}
> +
> +	/*
> +	 * Power down is mode 0bXX00, but not all devices have a valid
> +	 * power down state.
> +	 */
> +	st->can_power_down = en_may_be_off[1] && en_may_be_off[0] &&
> +			     st->info->has_power_down_state;
> +	/*
> +	 * The REFIN pin can take a 1.2V (AD762x) or 2.048V (AD796x)
> +	 * external reference when the mode is 0bXX01.
> +	 */
> +	st->can_refin = en_may_be_off[1] && en_may_be_on[0];
> +	/* 4.096V can be applied to REF when the EN mode is 0bXX10. */
> +	st->can_ref_4v096 = en_may_be_on[1] && en_may_be_off[0];
> +
> +	/* Avoid AD796x-specific setup if the part is an AD762x */
> +	if (num_gpios == 2)
> +		return 0;
> +
> +	/* mode 0b1100 (AD796x) is invalid */
> +	if (en_always_on[3] && en_always_on[2] && en_always_off[1] && en_always_off[0])
> +		return dev_err_probe(dev, -EINVAL,
> +				     "EN GPIOs set to invalid mode 0b1100\n");
> +	/*
> +	 * 5V can be applied to the AD796x REF pin when the EN mode is
> +	 * the same (0bX001 or 0bX101) as for can_refin, and REFIN is
> +	 * 0V.
> +	 */
> +	st->can_ref_5v = st->can_refin;
> +	/*
> +	 * Bandwidth (AD796x) is controlled solely by EN2. If it's
> +	 * specified and not hard-wired, then we can configure it to
> +	 * change the bandwidth between 28MHz and 9MHz.
> +	 */
> +	st->can_narrow_bandwidth = en_may_be_on[2];
> +	/* Wide bandwidth mode is possible if EN2 can be 0. */
> +	st->can_wide_bandwidth = en_may_be_off[2];
> +	/* Snooze mode (AD796x) is 0bXX11 when REFIN = 0V. */
> +	st->can_snooze = en_may_be_on[1] && en_may_be_on[0];
> +	/* Test pattern mode (AD796x) is 0b0100. */
> +	st->can_test_pattern = en_may_be_off[3] && en_may_be_on[2] &&
> +			       en_may_be_off[1] && en_may_be_off[0];
> +
> +	return 0;
> +}//

> +
> +static int ad7960_set_mode(struct ad7625_state *st, enum ad7960_mode mode,
> +			   bool have_refin, int ref_mv)
> +{
> +	switch (mode) {
> +	case AD7960_MODE_POWER_DOWN:
> +		if (!st->can_power_down)
> +			return -EINVAL;
> +
> +		gpiod_set_value_cansleep(st->en_gpios[2], 0);
> +		gpiod_set_value_cansleep(st->en_gpios[1], 0);
> +		gpiod_set_value_cansleep(st->en_gpios[0], 0);

Blank lines before simple 'good' returns can make things a little easier to read.
So here.

> +		return 0;
> +
> +	case AD7960_MODE_SNOOZE:
> +		if (!st->can_snooze)
> +			return -EINVAL;
> +
> +		gpiod_set_value_cansleep(st->en_gpios[1], 1);
> +		gpiod_set_value_cansleep(st->en_gpios[0], 1);
here
> +		return 0;
> +
> +	case AD7960_MODE_NARROW_BANDWIDTH:
> +		if (!st->can_narrow_bandwidth)
> +			return -EINVAL;
> +
> +		gpiod_set_value_cansleep(st->en_gpios[2], 1);
> +		ad7625_set_en_gpios_for_vref(st, have_refin, ref_mv);
here etc

> +		return 0;
> +
> +	case AD7960_MODE_WIDE_BANDWIDTH:
> +		if (!st->can_wide_bandwidth)
> +			return -EINVAL;
> +
> +		gpiod_set_value_cansleep(st->en_gpios[2], 0);
> +		ad7625_set_en_gpios_for_vref(st, have_refin, ref_mv);
> +		return 0;
> +
> +	case AD7960_MODE_TEST_PATTERN:
> +		if (!st->can_test_pattern)
> +			return -EINVAL;
> +
> +		gpiod_set_value_cansleep(st->en_gpios[3], 0);
> +		gpiod_set_value_cansleep(st->en_gpios[2], 1);
> +		gpiod_set_value_cansleep(st->en_gpios[1], 0);
> +		gpiod_set_value_cansleep(st->en_gpios[0], 0);
> +		return 0;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad7625_probe(struct platform_device *pdev)
> +{
> +	/*
> +	 * Power-up info for the device says to bring up vio, then
> +	 * vdd2, then vdd1
> +	 */
> +	static const char * const regulator_names[] = { "vio", "vdd2", "vdd1" };
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad7625_state *st;
> +	struct clk *ref_clk;
> +	int ret, ref_mv;
> +	int default_sample_freq;
> +	bool have_refin;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	st->info = device_get_match_data(dev);
> +	if (!st->info)
> +		return dev_err_probe(dev, -EINVAL, "no chip info\n");
> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
> +		regulator_names);
Align after (

> +	if (ret)
> +		return ret;
> +
> +	if (st->info->has_bandwidth_control)
> +		ret = ad7625_parse_mode(dev, st, 4);
> +	else
> +		ret = ad7625_parse_mode(dev, st, 2);
> +
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Determine the source of the reference voltage:
> +	 * - internal reference: neither REF or REFIN is connected
> +	 *   (invalid for AD796x)
> +	 * - internal buffer, external reference: REF not connected,
> +	 *   is REFIN connected
> +	 * - external reference: REF connected, REFIN not connected
> +	 */
> +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");

Given this regulator related block of code is complex, perhaps lift it
to a separate function where you can add docs for the function as
a whole?

> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "failed to get REF voltage\n");
> +
> +	ref_mv = ret == -ENODEV ? 0 : ret / 1000;
> +
> +	ret = devm_regulator_get_enable_optional(dev, "refin");
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "failed to get REFIN voltage\n");
> +
> +	have_refin = ret != -ENODEV;
> +
> +	if (have_refin && !st->can_refin)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "REFIN provided in unsupported mode\n");
> +
> +	if (!st->info->has_internal_vref && !have_refin && !ref_mv)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Need either REFIN or REF");
> +
> +	if (have_refin && ref_mv)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "cannot have both REFIN and REF supplies\n");
> +
> +	st->vref_mv = ref_mv ?: AD7625_INTERNAL_REF_MV;
> +
> +	if (ref_mv == 4096 && !st->can_ref_4v096)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "REF is 4.096V in unsupported mode\n");
> +
> +	if (ref_mv == 5000 && !st->can_ref_5v)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "REF is 5V in unsupported mode\n");
> +
> +	/* Set the device mode based on detected EN configuration. */
> +	if (!st->info->has_bandwidth_control) {
> +		ad7625_set_en_gpios_for_vref(st, have_refin, ref_mv);
> +	} else {
> +		/*
> +		 * If neither sampling mode is available, then report an error,
> +		 * since the other modes are not useful defaults.
> +		 */
> +		if (st->can_wide_bandwidth) {
> +			ret = ad7960_set_mode(st, AD7960_MODE_WIDE_BANDWIDTH,
> +				have_refin, ref_mv);

Align just after (

> +		} else if (st->can_narrow_bandwidth) {
> +			ret = ad7960_set_mode(st, AD7960_MODE_NARROW_BANDWIDTH,
> +				have_refin, ref_mv);
> +		} else {
> +			return dev_err_probe(dev, -EINVAL,
> +				"couldn't set device to wide or narrow bandwidth modes\n");
Fine to not align this one as it won't fit on a reasonable short line if you do.
For other cases, aligning is preferred.

> +		}
> +
> +		if (ret)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "failed to set EN pins\n");
> +	}
> +
> +	ret = devm_ad7625_pwm_get(dev, ref_clk, st);
> +	if (ret)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "failed to set ref_clk_rate_hz\n");
> +
> +	indio_dev->channels = &st->info->chan_spec;
> +	indio_dev->num_channels = 1;
> +	indio_dev->name = st->info->name;
> +	indio_dev->info = &ad7625_info;
> +
> +	st->back = devm_iio_backend_get(dev, NULL);
> +	if (IS_ERR(st->back))
> +		return dev_err_probe(dev, PTR_ERR(st->back),
> +				     "failed to get IIO backend");
> +
> +	ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_backend_enable(dev, st->back);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Set the initial sampling frequency to the maximum, unless the
> +	 * AD796x device is limited to narrow bandwidth, in which case

Maybe add 'why' it is limited here - which I think is down to one of the
gpios not being wired up in a controllable fashion.

> +	 * the sampling frequency should be limited to 2MSPS
> +	 */
> +	if (!st->info->has_bandwidth_control) {
> +		default_sample_freq = st->info->max_sample_rate_hz;
> +	} else {
> +		default_sample_freq = !st->can_wide_bandwidth ? AD7960_MAX_NBW_FREQ :
> +				      st->info->max_sample_rate_hz;
> +	}
> +
> +	ret = ad7625_set_sampling_freq(st, default_sample_freq);
> +	if (ret < 0)
> +		dev_err_probe(dev, ret, "failed to set sampling frequency\n");

return dev_err_probe();
If it's not a fatal error, then dev_err* is not appropriate.

> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct of_device_id ad7625_of_match[] = {
> +	{ .compatible = "adi,ad7625", .data = &ad7625_chip_info, },

Trivial but inconsistent to have , after last entry here, but not in platform_device_id
entries below.  I don't mind which but pick one.

> +	{ .compatible = "adi,ad7626", .data = &ad7626_chip_info, },
> +	{ .compatible = "adi,ad7960", .data = &ad7960_chip_info, },
> +	{ .compatible = "adi,ad7961", .data = &ad7961_chip_info, },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ad7625_of_match);
> +
> +static const struct platform_device_id ad7625_device_ids[] = {
> +	{ .name = "ad7625", .driver_data = (kernel_ulong_t)&ad7625_chip_info },
> +	{ .name = "ad7626", .driver_data = (kernel_ulong_t)&ad7626_chip_info },
> +	{ .name = "ad7960", .driver_data = (kernel_ulong_t)&ad7960_chip_info },
> +	{ .name = "ad7961", .driver_data = (kernel_ulong_t)&ad7961_chip_info },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, ad7625_device_ids);
> +
> +static struct platform_driver ad7625_driver = {
> +	.probe = ad7625_probe,
> +	.driver = {
> +		.name = "ad7625",
> +		.of_match_table = ad7625_of_match,
> +	},
> +	.id_table = ad7625_device_ids,
> +};
> +module_platform_driver(ad7625_driver);
> +
> +MODULE_AUTHOR("Trevor Gamblin <tgamblin@baylibre.com>");
> +MODULE_DESCRIPTION("Analog Devices AD7625 ADC");
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_IMPORT_NS(IIO_BACKEND);
> 

[PATCH RFC 3/3] docs: iio: new docs for ad7625 driver

[Trevor Gamblin]

Add documentation for the AD7625/AD7626/AD7960/AD7961 ADCs.

Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
 Documentation/iio/ad7625.rst | 91 ++++++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS                  |  1 +
 2 files changed, 92 insertions(+)

diff --git a/Documentation/iio/ad7625.rst b/Documentation/iio/ad7625.rst
new file mode 100644
index 000000000000..61761e3b75c3
--- /dev/null
+++ b/Documentation/iio/ad7625.rst
@@ -0,0 +1,91 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+====================
+AD7625 driver
+====================
+
+ADC driver for Analog Devices Inc. AD7625, AD7626, AD7960, and AD7961
+devices. The module name is ``ad7625``.
+
+Supported devices
+=================
+
+The following chips are supported by this driver:
+
+* `AD7625 <https://www.analog.com/AD7625>`_
+* `AD7626 <https://www.analog.com/AD7626>`_
+* `AD7960 <https://www.analog.com/AD7960>`_
+* `AD7961 <https://www.analog.com/AD7961>`_
+
+The driver requires use of the Pulsar LVDS HDL project:
+
+* `Pulsar LVDS HDL <http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html>`_
+
+To trigger conversions and enable subsequent data transfer, the devices
+require coupled PWM signals with a phase offset.
+
+Supported features
+==================
+
+Conversion control modes
+------------------------
+
+The driver currently supports one of two possible LVDS conversion control methods.
+
+Echoed-Clock interface mode
+^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+.. code-block::
+
+                                                +----------------+
+                     +xxxxxxxxxxxxxxxxxxxxxxxxxx| CNV            |
+                     X                          |                |
+                     v                          |    HOST        |
+          +----------------------------+        |                |
+          |      CNV+/CNV-   DCO+/DCO- |xxxxxxx>| CLK_IN         |
+          |                            |        |                |
+          |                            |        |                |
+          |       AD7625         D+/D- |xxxxxxx>| DATA_IN        |
+          |                            |        |                |
+          |                            |        |                |
+          |                  CLK+/CLK- |<xxxxxxx| CLK & CLK_GATE |
+          +----------------------------+        |                |
+                                                +----------------+
+
+Reference voltage
+-----------------
+
+Three possible reference voltage sources are supported:
+
+- Internal reference (only available on AD7625 and AD7626)
+- External reference and internal buffer
+- External reference
+
+The source is determined by the device tree. If ``ref-supply`` is present, then
+the external reference is used. If ``refin-supply`` is present, then the internal
+buffer is used. If neither is present, then the internal reference is used.
+
+Unimplemented features
+----------------------
+
+- Self-clocked mode
+
+
+Device attributes
+=================
+
+The AD762x is a fully-differential ADC and has the following attributes:
+
++---------------------------------------+--------------------------------------------------------------+
+| Attribute                             | Description                                                  |
++=======================================+==============================================================+
+| ``scale``                             | Scale factor to convert raw value from buffered reads to mV. |
++---------------------------------------+--------------------------------------------------------------+
+
+
+Device buffers
+==============
+
+This driver supports IIO triggered buffers.
+
+See :doc:`iio_devbuf` for more information.
diff --git a/MAINTAINERS b/MAINTAINERS
index a90972e1c5c5..97c9b03e1cf0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1268,6 +1268,7 @@ S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 W:	http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
+F:	Documentation/iio/ad7625.rst
 F:	drivers/iio/adc/ad7625.c
 
 ANALOG DEVICES INC AD7768-1 DRIVER

-- 
2.39.2

Re: [PATCH RFC 3/3] docs: iio: new docs for ad7625 driver

[Jonathan Cameron]

On Wed, 31 Jul 2024 09:48:05 -0400
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> Add documentation for the AD7625/AD7626/AD7960/AD7961 ADCs.
> 
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
LGTM.  Includes some of the detail I'd like to also see alongside that
RFC question on DCO.

> ---
>  Documentation/iio/ad7625.rst | 91 ++++++++++++++++++++++++++++++++++++++++++++
>  MAINTAINERS                  |  1 +
>  2 files changed, 92 insertions(+)
> 
> diff --git a/Documentation/iio/ad7625.rst b/Documentation/iio/ad7625.rst
> new file mode 100644
> index 000000000000..61761e3b75c3
> --- /dev/null
> +++ b/Documentation/iio/ad7625.rst
> @@ -0,0 +1,91 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +====================
> +AD7625 driver
> +====================
> +
> +ADC driver for Analog Devices Inc. AD7625, AD7626, AD7960, and AD7961
> +devices. The module name is ``ad7625``.
> +
> +Supported devices
> +=================
> +
> +The following chips are supported by this driver:
> +
> +* `AD7625 <https://www.analog.com/AD7625>`_
> +* `AD7626 <https://www.analog.com/AD7626>`_
> +* `AD7960 <https://www.analog.com/AD7960>`_
> +* `AD7961 <https://www.analog.com/AD7961>`_
> +
> +The driver requires use of the Pulsar LVDS HDL project:
> +
> +* `Pulsar LVDS HDL <http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html>`_
> +
> +To trigger conversions and enable subsequent data transfer, the devices
> +require coupled PWM signals with a phase offset.
> +
> +Supported features
> +==================
> +
> +Conversion control modes
> +------------------------
> +
> +The driver currently supports one of two possible LVDS conversion control methods.
> +
> +Echoed-Clock interface mode
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +.. code-block::
> +
> +                                                +----------------+
> +                     +xxxxxxxxxxxxxxxxxxxxxxxxxx| CNV            |
> +                     X                          |                |
> +                     v                          |    HOST        |
> +          +----------------------------+        |                |
> +          |      CNV+/CNV-   DCO+/DCO- |xxxxxxx>| CLK_IN         |
> +          |                            |        |                |
> +          |                            |        |                |
> +          |       AD7625         D+/D- |xxxxxxx>| DATA_IN        |
> +          |                            |        |                |
> +          |                            |        |                |
> +          |                  CLK+/CLK- |<xxxxxxx| CLK & CLK_GATE |
> +          +----------------------------+        |                |
> +                                                +----------------+
> +
> +Reference voltage
> +-----------------
> +
> +Three possible reference voltage sources are supported:
> +
> +- Internal reference (only available on AD7625 and AD7626)
> +- External reference and internal buffer
> +- External reference
> +
> +The source is determined by the device tree. If ``ref-supply`` is present, then
> +the external reference is used. If ``refin-supply`` is present, then the internal
> +buffer is used. If neither is present, then the internal reference is used.
> +
> +Unimplemented features
> +----------------------
> +
> +- Self-clocked mode
> +
> +
> +Device attributes
> +=================
> +
> +The AD762x is a fully-differential ADC and has the following attributes:
> +
> ++---------------------------------------+--------------------------------------------------------------+
> +| Attribute                             | Description                                                  |
> ++=======================================+==============================================================+
> +| ``scale``                             | Scale factor to convert raw value from buffered reads to mV. |
> ++---------------------------------------+--------------------------------------------------------------+
> +
> +
> +Device buffers
> +==============
> +
> +This driver supports IIO triggered buffers.
> +
> +See :doc:`iio_devbuf` for more information.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a90972e1c5c5..97c9b03e1cf0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1268,6 +1268,7 @@ S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  W:	http://analogdevicesinc.github.io/hdl/projects/pulsar_lvds/index.html
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad7625.yaml
> +F:	Documentation/iio/ad7625.rst
>  F:	drivers/iio/adc/ad7625.c
>  
>  ANALOG DEVICES INC AD7768-1 DRIVER
> 

Re: [PATCH RFC 0/3] iio: adc: add new ad7625 driver

[Trevor Gamblin]

On 2024-07-31 9:48 a.m., Trevor Gamblin wrote:
> This series adds a new driver for the Analog Devices Inc. AD7625,
> AD7626, AD7960, and AD7961. These chips are part of a family of
> LVDS-based SAR ADCs. The initial driver implementation does not support
> the devices' self-clocked mode, although that can be added later.
>
> One aspect that is still uncertain is whether there should be a
> devicetree property indicating if the DCO+/- pins are connected, so
> specific feedback on that is appreciated.
>
> The devices make use of two offset PWM signals, one to trigger
> conversions and the other as a burst signal for transferring data to the
> host. These rely on the new PWM waveform functionality being
> reviewed in [1].
>
> This work is being done by BayLibre and on behalf of Analog Devices
> Inc., hence the maintainers are @analog.com.
>
> Special thanks to David Lechner for his guidance and reviews.

I forgot to actually include:

[1] 
https://lore.kernel.org/linux-pwm/cover.1722261050.git.u.kleine-koenig@baylibre.com

>
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
> Trevor Gamblin (3):
>        dt-bindings: iio: adc: add AD762x/AD796x ADCs
>        iio: adc: ad7625: add driver
>        docs: iio: new docs for ad7625 driver
>
>   .../devicetree/bindings/iio/adc/adi,ad7625.yaml    | 176 ++++++
>   Documentation/iio/ad7625.rst                       |  91 +++
>   MAINTAINERS                                        |  11 +
>   drivers/iio/adc/Kconfig                            |  15 +
>   drivers/iio/adc/Makefile                           |   1 +
>   drivers/iio/adc/ad7625.c                           | 626 +++++++++++++++++++++
>   6 files changed, 920 insertions(+)
> ---
> base-commit: ac6a258892793f0a255fe7084ec2b612131c67fc
> change-id: 20240730-ad7625_r1-60d17ea28958
>
> Best regards,

Re: [PATCH RFC 0/3] iio: adc: add new ad7625 driver

[Jonathan Cameron]

On Wed, 31 Jul 2024 09:48:02 -0400
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> This series adds a new driver for the Analog Devices Inc. AD7625,
> AD7626, AD7960, and AD7961. These chips are part of a family of
> LVDS-based SAR ADCs. The initial driver implementation does not support
> the devices' self-clocked mode, although that can be added later.
> 
> One aspect that is still uncertain is whether there should be a
> devicetree property indicating if the DCO+/- pins are connected, so
> specific feedback on that is appreciated.

Would be good to give more detail. What is DCO?
Seems to be a delayed clock skewed so it aligns with the data being
out in response to clk. Host drives clk, but samples on dco.

Given the device needs to do slightly different things depending
on whether that is what the host is using, I think it definitely does
need to be in DT.

Maybe you need to represent it as the ADC also having a PWM
output that the LVDS DT node binds to if present.  That binding
then indicates to the ADC driver that it needs to operating in the
mode that doesn't send the synchronisation 101 pattern.
If you are always representing the ADC and the lvds side of things
as a single node, then need a flag in here somewhere so we can
tell if they are in use or not.

Given this exists as a potential difference between two separate
parts pf a system I'd definitely think about whether we can give them separate
representations with clear 'connectivity' between them

One of those cases were a bit of ascii art would probably be good
to put the problem clearly for the DT reviewers.

Jonathan


> 
> The devices make use of two offset PWM signals, one to trigger
> conversions and the other as a burst signal for transferring data to the
> host. These rely on the new PWM waveform functionality being
> reviewed in [1].
> 
> This work is being done by BayLibre and on behalf of Analog Devices
> Inc., hence the maintainers are @analog.com.
> 
> Special thanks to David Lechner for his guidance and reviews.
> 
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
> Trevor Gamblin (3):
>       dt-bindings: iio: adc: add AD762x/AD796x ADCs
>       iio: adc: ad7625: add driver
>       docs: iio: new docs for ad7625 driver
> 
>  .../devicetree/bindings/iio/adc/adi,ad7625.yaml    | 176 ++++++
>  Documentation/iio/ad7625.rst                       |  91 +++
>  MAINTAINERS                                        |  11 +
>  drivers/iio/adc/Kconfig                            |  15 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/ad7625.c                           | 626 +++++++++++++++++++++
>  6 files changed, 920 insertions(+)
> ---
> base-commit: ac6a258892793f0a255fe7084ec2b612131c67fc
> change-id: 20240730-ad7625_r1-60d17ea28958
> 
> Best regards,