diff mbox series

[v15,1/3] dt-bindings: adc: add AD7173

Message ID 20240223133758.9787-1-mitrutzceclan@gmail.com
State New
Headers show
Series [v15,1/3] dt-bindings: adc: add AD7173 | expand

Commit Message

Ceclan, Dumitru Feb. 23, 2024, 1:37 p.m. UTC
The AD7173 family offer a complete integrated Sigma-Delta ADC solution
which can be used in high precision, low noise single channel applications
or higher speed multiplexed applications. The Sigma-Delta ADC is intended
primarily for measurement of signals close to DC but also delivers
outstanding performance with input bandwidths out to ~10kHz.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
V14->V15 <no changes>
V13->V14
 - Refer in descriptions of the avdd-supply as AVDD1 in concordance to datasheet
 - Fix typo
 - Place interrupts descriptions separately for each item
 - Restrict max channel reg to 3 for models AD717x-2
V12->V13
 - Remove adi,clock-select
 - Update avdd and avdd2 supply descriptions
 - Update adi,reference-select description to suggest that it is referenced to avss
 - Make clocks/clock-names and clock-controller mutually exclusive
V11->V12
 - Drop "binding", describe hardware in binding description
 - Rename refin and refin2 to vref and vref2
 - Add better description to external references to better show that the voltage
    value that needs to be specified is the difference between the positive and
    negative reference pins
 - Add optional clocks properties
 - Add optional adi,clock-select property
 - Add option for second interrupt, error
 - Add description to interrupts
V10->V11
 - Fix example warning: '#gpio-cells' is a dependency of 'gpio-controller'
 - Add description to #gpio-cells property
V9->V10
 - Fix dt_binding_check type warning from adi,reference-select
V8->v9
 - Add gpio-controller and "#gpio-cells" properties
 - Add missing avdd2 and iovdd supplies
 - Add string type to reference-select
V7->V8
 - include missing fix from V6
V6->V7 <no changes>
V5->V6
 - Moved global required property to proper placement
V4 -> V5
 - Use string enum instead of integers for "adi,reference-select"
 - Fix conditional checking in regards to compatible
V3 -> V4
 - include supply attributes
 - add channel attribute for selecting conversion reference
V2 -> V3
 - remove redundant descriptions
 - use referenced 'bipolar' property
 - remove newlines from example
V1 -> V2 <no changes>
 .../bindings/iio/adc/adi,ad7173.yaml          | 246 ++++++++++++++++++
 1 file changed, 246 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml

Comments

Jonathan Cameron Feb. 24, 2024, 5:30 p.m. UTC | #1
On Fri, 23 Feb 2024 15:37:28 +0200
Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel applications
> or higher speed multiplexed applications. The Sigma-Delta ADC is intended
> primarily for measurement of signals close to DC but also delivers
> outstanding performance with input bandwidths out to ~10kHz.
> 
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>

Ok, in the interests of perfect not being the enemy of good enough.
I'll leave the supplies for now.  There are lots of existing drivers
where we don't list them as required (because my understanding of this
changed in more recent times).

It's been on my list of jobs for a really boring Friday afternoon
to bring them all inline with the convention of if it needs power
on the pin, it's required, so what's one more? :)

As Nuno pointed out, patch 2 clashed with work already upstream to
allow firmware to have the final say on interrupt types. I think
I've resolved that correctly.

I tidied up the docs ordering issue Andy noted.

Also, ad_sigma_delta is namespaced. So added
MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA).

Make sure you test your patches with a modular build
on a more recent tree - that change was early last in 2022!

A few lines in the driver were too long.
I don't mind them going over 80 for readability reasons, but
not over 100.

Anyhow, with those changes (and please check I didn't mess things up!)
applied to the togreg branch of iio.git and pushed for now as testing
for 0-day to get a look in.

Thanks,

Jonathan
Jonathan Cameron Feb. 26, 2024, 2:55 p.m. UTC | #2
On Sat, 24 Feb 2024 17:30:55 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 23 Feb 2024 15:37:28 +0200
> Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:
> 
> > The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> > which can be used in high precision, low noise single channel applications
> > or higher speed multiplexed applications. The Sigma-Delta ADC is intended
> > primarily for measurement of signals close to DC but also delivers
> > outstanding performance with input bandwidths out to ~10kHz.
> > 
> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>  
> 
> Ok, in the interests of perfect not being the enemy of good enough.
> I'll leave the supplies for now.  There are lots of existing drivers
> where we don't list them as required (because my understanding of this
> changed in more recent times).
> 
> It's been on my list of jobs for a really boring Friday afternoon
> to bring them all inline with the convention of if it needs power
> on the pin, it's required, so what's one more? :)
> 
> As Nuno pointed out, patch 2 clashed with work already upstream to
> allow firmware to have the final say on interrupt types. I think
> I've resolved that correctly.
> 
> I tidied up the docs ordering issue Andy noted.
> 
> Also, ad_sigma_delta is namespaced. So added
> MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA).
> 
> Make sure you test your patches with a modular build
> on a more recent tree - that change was early last in 2022!
> 
> A few lines in the driver were too long.
> I don't mind them going over 80 for readability reasons, but
> not over 100.
> 
> Anyhow, with those changes (and please check I didn't mess things up!)
> applied to the togreg branch of iio.git and pushed for now as testing
> for 0-day to get a look in.

Not good news.  There are 2 issues.
>> drivers/iio/adc/ad7173.c:854:3: warning: variable 'chan_arr' is uninitialized when used here [-Wuninitialized]  
     854 |                 chan_arr[chan_index] = ad7173_temp_iio_channel_template;
         |                 ^~~~~~~~
   drivers/iio/adc/ad7173.c:848:32: note: initialize the variable 'chan_arr' to silence this warning
     848 |         struct iio_chan_spec *chan_arr, *chan;
         |                                       ^
         |                                        = NULL
>> drivers/iio/adc/ad7173.c:855:19: warning: variable 'chans_st_arr' is uninitialized when used here [-Wuninitialized]  
     855 |                 chan_st_priv = &chans_st_arr[chan_index];
         |                                 ^~~~~~~~~~~~
   drivers/iio/adc/ad7173.c:845:37: note: initialize the variable 'chans_st_arr' to silence this warning
     845 |         struct ad7173_channel *chans_st_arr, *chan_st_priv;
         |                                            ^
         |                                             = NULL

+ if you build with !CONFIG_GPIOLIB

ad7173_gpio_init() isn't defined.  That needs a stub.

I'll back this driver out for now as fixing the first issue is a little fiddly because
indio_dev->channels is const so the code should allocate and fill the array via a local pointer
before assigning it to indio_dev.

Please send a new version with these resolved + make sure you run some build tests.

Thanks,

Jonathan

> 
> Thanks,
> 
> Jonathan
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
new file mode 100644
index 000000000000..36f16a325bc5
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
@@ -0,0 +1,246 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7173.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD7173 ADC
+
+maintainers:
+  - Ceclan Dumitru <dumitru.ceclan@analog.com>
+
+description: |
+  Analog Devices AD717x ADC's:
+  The AD717x family offer a complete integrated Sigma-Delta ADC solution which
+  can be used in high precision, low noise single channel applications
+  (Life Science measurements) or higher speed multiplexed applications
+  (Factory Automation PLC Input modules). The Sigma-Delta ADC is intended
+  primarily for measurement of signals close to DC but also delivers
+  outstanding performance with input bandwidths out to ~10kHz.
+
+  Datasheets for supported chips:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7172-2
+      - adi,ad7173-8
+      - adi,ad7175-2
+      - adi,ad7176-2
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    items:
+      - description: |
+          Ready: multiplexed with SPI data out. While SPI CS is low,
+          can be used to indicate the completion of a conversion.
+
+      - description: |
+          Error: The three error bits in the status register (ADC_ERROR, CRC_ERROR,
+          and REG_ERROR) are OR'ed, inverted, and mapped to the ERROR pin.
+          Therefore, the ERROR pin indicates that an error has occurred.
+
+  interrupt-names:
+    minItems: 1
+    items:
+      - const: rdy
+      - const: err
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  spi-max-frequency:
+    maximum: 20000000
+
+  gpio-controller:
+    description: Marks the device node as a GPIO controller.
+
+  '#gpio-cells':
+    const: 2
+    description:
+      The first cell is the GPIO number and the second cell specifies
+      GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.
+
+  vref-supply:
+    description: |
+      Differential external reference supply used for conversion. The reference
+      voltage (Vref) specified here must be the voltage difference between the
+      REF+ and REF- pins: Vref = (REF+) - (REF-).
+
+  vref2-supply:
+    description: |
+      Differential external reference supply used for conversion. The reference
+      voltage (Vref2) specified here must be the voltage difference between the
+      REF2+ and REF2- pins: Vref2 = (REF2+) - (REF2-).
+
+  avdd-supply:
+    description: Avdd supply, can be used as reference for conversion.
+                 This supply is referenced to AVSS, voltage specified here
+                 represents (AVDD1 - AVSS).
+
+  avdd2-supply:
+    description: Avdd2 supply, used as the input to the internal voltage regulator.
+                 This supply is referenced to AVSS, voltage specified here
+                 represents (AVDD2 - AVSS).
+
+  iovdd-supply:
+    description: iovdd supply, used for the chip digital interface.
+
+  clocks:
+    maxItems: 1
+    description: |
+      Optional external clock source. Can include one clock source: external
+      clock or external crystal.
+
+  clock-names:
+    enum:
+      - ext-clk
+      - xtal
+
+  '#clock-cells':
+    const: 0
+
+patternProperties:
+  "^channel@[0-9a-f]$":
+    type: object
+    $ref: adc.yaml
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 15
+
+      diff-channels:
+        items:
+          minimum: 0
+          maximum: 31
+
+      adi,reference-select:
+        description: |
+          Select the reference source to use when converting on
+          the specific channel. Valid values are:
+          vref       : REF+  /REF−
+          vref2      : REF2+ /REF2−
+          refout-avss: REFOUT/AVSS (Internal reference)
+          avdd       : AVDD  /AVSS
+
+          External reference ref2 only available on ad7173-8.
+          If not specified, internal reference used.
+        $ref: /schemas/types.yaml#/definitions/string
+        enum:
+          - vref
+          - vref2
+          - refout-avss
+          - avdd
+        default: refout-avss
+
+    required:
+      - reg
+      - diff-channels
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+  - if:
+      properties:
+        compatible:
+          not:
+            contains:
+              const: adi,ad7173-8
+    then:
+      properties:
+        vref2-supply: false
+      patternProperties:
+        "^channel@[0-9a-f]$":
+          properties:
+            adi,reference-select:
+              enum:
+                - vref
+                - refout-avss
+                - avdd
+            reg:
+              maximum: 3
+
+  - if:
+      anyOf:
+        - required: [clock-names]
+        - required: [clocks]
+    then:
+      properties:
+        '#clock-cells': false
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      adc@0 {
+        compatible = "adi,ad7173-8";
+        reg = <0>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+        interrupt-names = "rdy";
+        interrupt-parent = <&gpio>;
+        spi-max-frequency = <5000000>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        #clock-cells = <0>;
+
+        vref-supply = <&dummy_regulator>;
+
+        channel@0 {
+          reg = <0>;
+          bipolar;
+          diff-channels = <0 1>;
+          adi,reference-select = "vref";
+        };
+
+        channel@1 {
+          reg = <1>;
+          diff-channels = <2 3>;
+        };
+
+        channel@2 {
+          reg = <2>;
+          bipolar;
+          diff-channels = <4 5>;
+        };
+
+        channel@3 {
+          reg = <3>;
+          bipolar;
+          diff-channels = <6 7>;
+        };
+
+        channel@4 {
+          reg = <4>;
+          diff-channels = <8 9>;
+          adi,reference-select = "avdd";
+        };
+      };
+    };