diff mbox series

[v3,1/1] dt-bindings: iio: adc: add a7779 doc

Message ID 20240531133604.1380-2-ramona.nechita@analog.com (mailing list archive)
State Changes Requested
Headers show
Series [v3,1/1] dt-bindings: iio: adc: add a7779 doc | expand

Commit Message

Ramona Alexandra Nechita May 31, 2024, 1:35 p.m. UTC 
Add dt bindings for adc ad7779.

Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com>
---
 .../ABI/testing/sysfs-bus-iio-adc-ad777x      | 23 +++++
 .../bindings/iio/adc/adi,ad7779.yaml          | 87 +++++++++++++++++++
 2 files changed, 110 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml

Comments

Conor Dooley May 31, 2024, 2:51 p.m. UTC | #1 
On Fri, May 31, 2024 at 04:35:52PM +0300, Ramona Alexandra Nechita wrote:
> Add dt bindings for adc ad7779.
> 
> Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com>
> ---
>  .../ABI/testing/sysfs-bus-iio-adc-ad777x      | 23 +++++
>  .../bindings/iio/adc/adi,ad7779.yaml          | 87 +++++++++++++++++++
>  2 files changed, 110 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
> new file mode 100644
> index 000000000000..0a57fda598e6
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
> @@ -0,0 +1,23 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/filter_type_available
> +KernelVersion:  6.1
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Reading returns a list with the possible filter modes. Only supported by
> +		AD7771.
> +
> +		  * "sinc3"	- The digital sinc3 filter implements three main notches, one at
> +				the maximum ODR (128 kHz or 32 kHz, depending on the
> +				power mode) and another two at the ODR frequency selected to
> +				stop noise aliasing into the pass band.
> +
> +		  * "sinc5"	- The sinc5 filter implements five notches, one at
> +				the maximum ODR (128 kHz or 32 kHz, depending on the
> +				power mode) and another four at the ODR frequency
> +				selected to stop noise aliasing into the pass band.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/filter_type
> +KernelVersion:  6.1
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Set the filter mode of the differential channel. The current sampling_frequency
> +		is set according to the filter range. Only supported by AD7771.

This patch is really confusing to me. Why is there a file documenting
the sysfs interface for a driver that isn't in the tree? Shouldn't this
patch be part of a series that adds the driver? I suggest you speak to
Nuno or another collogue about how to submit a series.

Thanks,
Conor.
Conor Dooley May 31, 2024, 2:53 p.m. UTC | #2 
On Fri, May 31, 2024 at 03:51:56PM +0100, Conor Dooley wrote:
> On Fri, May 31, 2024 at 04:35:52PM +0300, Ramona Alexandra Nechita wrote:
> > Add dt bindings for adc ad7779.
> > 
> > Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com>
> > ---
> >  .../ABI/testing/sysfs-bus-iio-adc-ad777x      | 23 +++++
> >  .../bindings/iio/adc/adi,ad7779.yaml          | 87 +++++++++++++++++++
> >  2 files changed, 110 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
> > new file mode 100644
> > index 000000000000..0a57fda598e6
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
> > @@ -0,0 +1,23 @@
> > +What:		/sys/bus/iio/devices/iio:deviceX/filter_type_available
> > +KernelVersion:  6.1
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Reading returns a list with the possible filter modes. Only supported by
> > +		AD7771.
> > +
> > +		  * "sinc3"	- The digital sinc3 filter implements three main notches, one at
> > +				the maximum ODR (128 kHz or 32 kHz, depending on the
> > +				power mode) and another two at the ODR frequency selected to
> > +				stop noise aliasing into the pass band.
> > +
> > +		  * "sinc5"	- The sinc5 filter implements five notches, one at
> > +				the maximum ODR (128 kHz or 32 kHz, depending on the
> > +				power mode) and another four at the ODR frequency
> > +				selected to stop noise aliasing into the pass band.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/filter_type
> > +KernelVersion:  6.1
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Set the filter mode of the differential channel. The current sampling_frequency
> > +		is set according to the filter range. Only supported by AD7771.
> 
> This patch is really confusing to me. Why is there a file documenting
> the sysfs interface for a driver that isn't in the tree? Shouldn't this
> patch be part of a series that adds the driver? I suggest you speak to
> Nuno or another collogue about how to submit a series.

Colleague*, auto accepting vim's first spelling suggestion strikes again
:)
Krzysztof Kozlowski May 31, 2024, 3:25 p.m. UTC | #3 
On 31/05/2024 15:35, Ramona Alexandra Nechita wrote:
> Add dt bindings for adc ad7779.
> 
> Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com>
> ---
>  .../ABI/testing/sysfs-bus-iio-adc-ad777x      | 23 +++++
>  .../bindings/iio/adc/adi,ad7779.yaml          | 87 +++++++++++++++++++
>  2 files changed, 110 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml

Why is this sent separately troubles us all... I saw many patches from
analog.com, so maybe there is a person who will help you, in case you do
not want to read submitting patches?

...

> +
> +  reg:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  spi-max-frequency: true

Drop

> +
> +  clocks:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vref-supply:
> +    description:
> +      ADC reference voltage supply
> +
> +  start-gpios:
> +    description:
> +      Pin that controls start synchronization pulse.
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0 {
> +          compatible = "adi,ad7779";

Messed indentation. Keep same for entire example, e.g.
Use 4 spaces for example indentation.

Best regards,
Krzysztof
Jonathan Cameron June 2, 2024, 9:11 a.m. UTC | #4 
On Fri, 31 May 2024 16:35:52 +0300
Ramona Alexandra Nechita <ramona.nechita@analog.com> wrote:

> Add dt bindings for adc ad7779.
> 
> Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com>
> ---
>  .../ABI/testing/sysfs-bus-iio-adc-ad777x      | 23 +++++
>  .../bindings/iio/adc/adi,ad7779.yaml          | 87 +++++++++++++++++++
>  2 files changed, 110 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
> new file mode 100644
> index 000000000000..0a57fda598e6
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
> @@ -0,0 +1,23 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/filter_type_available

This is a binding patch - ABI docs should be in the driver code patch not here.
A separate patch for ABI docs is also fine.

Also, it is also a documentation bug to have more than one sysfs docs file
for an attribute with the same What line.  We already have this one in ad4130.

So the documentation needs to be combined into Documentation/ABI/testing/sysfs-bus-iio-adc
or sysfs-bus-iio

That means you need to write the doc so that is as generic as possible.
There are a few places where we have previously documented device specific
aspects - copy one of those if it is absolutely necessary, but there is info
here that doesn't matter in ABI docs such as what the maximum ODR is for
a particular part - that should be possible to read back from existing ABI.

> +KernelVersion:  6.1
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Reading returns a list with the possible filter modes. Only supported by
> +		AD7771.
> +
> +		  * "sinc3"	- The digital sinc3 filter implements three main notches, one at
> +				the maximum ODR (128 kHz or 32 kHz, depending on the
> +				power mode) and another two at the ODR frequency selected to
> +				stop noise aliasing into the pass band.
> +
> +		  * "sinc5"	- The sinc5 filter implements five notches, one at
> +				the maximum ODR (128 kHz or 32 kHz, depending on the
> +				power mode) and another four at the ODR frequency
> +				selected to stop noise aliasing into the pass band.

This one sounds nice - so why don't I select it all the time?
Helpful to state the disadvantages.

I'm also a little confused by 4 notches at the ODR frequency. I see that
text comes from the datasheet but that doesn't mean you can't improve it :)
I think this really means notches at multiples of the sampling frequency.



> +
> +What:		/sys/bus/iio/devices/iio:deviceX/filter_type
> +KernelVersion:  6.1
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Set the filter mode of the differential channel. The current sampling_frequency
> +		is set according to the filter range. Only supported by AD7771.
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml
> new file mode 100644
> index 000000000000..632e9ec0ab44
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7779.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD777X family 8-Channel, 24-Bit, Simultaneous Sampling ADCs
> +
> +maintainers:
> +  - Ramona Nechita <ramona.nechita@analog.com>
> +
> +description: |
> +  The AD777X family consist of 8-channel, simultaneous sampling analog-to-
> +  digital converter (ADC). Eight full Σ-Δ ADCs are on-chip. The
> +  AD7771 provides an ultralow input current to allow direct sensor
> +  connection. Each input channel has a programmable gain stage
> +  allowing gains of 1, 2, 4, and 8 to map lower amplitude sensor
> +  outputs into the full-scale ADC input range, maximizing the
> +  dynamic range of the signal chain.
> +
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7770.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7771.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7779.pdf
> +
> +$ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7770
> +      - adi,ad7771
> +      - adi,ad7779
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  spi-max-frequency: true
> +
> +  clocks:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vref-supply:
> +    description:
> +      ADC reference voltage supply
> +
> +  start-gpios:
> +    description:
> +      Pin that controls start synchronization pulse.
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
You seem to be missing a whole raft of power supplies.
AVDD1x, AVDD2,, IOVDD - maybe more.
Some of those at least will be 'required'.
Note that required doesn't rule out using fixed or dummy regulators 
(those supplied by the regulator framework if nothing is found in DT)
but the binding should still reflect we need to provide power for the chip
to function.

> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0 {
> +          compatible = "adi,ad7779";
> +          reg = <0>;
> +          spi-max-frequency = <20000000>;
> +          vref-supply = <&vref>;
> +          start-gpios = <&gpio0 87 GPIO_ACTIVE_LOW>;
> +          reset-gpios = <&gpio0 93 GPIO_ACTIVE_LOW>;
> +          clocks = <&adc_clk>;
> +        };
> +    };
> +...
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
new file mode 100644
index 000000000000..0a57fda598e6
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad777x
@@ -0,0 +1,23 @@ 
+What:		/sys/bus/iio/devices/iio:deviceX/filter_type_available
+KernelVersion:  6.1
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Reading returns a list with the possible filter modes. Only supported by
+		AD7771.
+
+		  * "sinc3"	- The digital sinc3 filter implements three main notches, one at
+				the maximum ODR (128 kHz or 32 kHz, depending on the
+				power mode) and another two at the ODR frequency selected to
+				stop noise aliasing into the pass band.
+
+		  * "sinc5"	- The sinc5 filter implements five notches, one at
+				the maximum ODR (128 kHz or 32 kHz, depending on the
+				power mode) and another four at the ODR frequency
+				selected to stop noise aliasing into the pass band.
+
+What:		/sys/bus/iio/devices/iio:deviceX/filter_type
+KernelVersion:  6.1
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Set the filter mode of the differential channel. The current sampling_frequency
+		is set according to the filter range. Only supported by AD7771.
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml
new file mode 100644
index 000000000000..632e9ec0ab44
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml
@@ -0,0 +1,87 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7779.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD777X family 8-Channel, 24-Bit, Simultaneous Sampling ADCs
+
+maintainers:
+  - Ramona Nechita <ramona.nechita@analog.com>
+
+description: |
+  The AD777X family consist of 8-channel, simultaneous sampling analog-to-
+  digital converter (ADC). Eight full Σ-Δ ADCs are on-chip. The
+  AD7771 provides an ultralow input current to allow direct sensor
+  connection. Each input channel has a programmable gain stage
+  allowing gains of 1, 2, 4, and 8 to map lower amplitude sensor
+  outputs into the full-scale ADC input range, maximizing the
+  dynamic range of the signal chain.
+
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7770.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7771.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7779.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7770
+      - adi,ad7771
+      - adi,ad7779
+
+  reg:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  spi-max-frequency: true
+
+  clocks:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vref-supply:
+    description:
+      ADC reference voltage supply
+
+  start-gpios:
+    description:
+      Pin that controls start synchronization pulse.
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+          compatible = "adi,ad7779";
+          reg = <0>;
+          spi-max-frequency = <20000000>;
+          vref-supply = <&vref>;
+          start-gpios = <&gpio0 87 GPIO_ACTIVE_LOW>;
+          reset-gpios = <&gpio0 93 GPIO_ACTIVE_LOW>;
+          clocks = <&adc_clk>;
+        };
+    };
+...