Re: [RFC PATCH 5/5] dt-bindings: iio: adc: add bindings for TI ADS1262

[David Lechner <dlechner@baylibre.com>]

On 5/1/25 5:00 AM, Sayyad Abid wrote:
> Add the Device Tree binding documentation for the Texas Instruments ADS1262 ADC.
> 
> Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
> ---

For readers, it logically makes more sense to have this be the first patch in
the series.

>  .../bindings/iio/adc/ti,ads1262.yaml          | 189 ++++++++++++++++++
>  1 file changed, 189 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
> new file mode 100644
> index 000000000000..8c4cc2cf6467
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
> @@ -0,0 +1,189 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1262.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments' ADS1262 32-Bit Analog to Digital Converter
> +
> +maintainers:
> +  - Sayyad Abid <sayyad.abid16@gmail.com>
> +
> +description: |
> +  Texas Instruments ADS1262 32-Bit Analog to Digital Converter with,
> +  internal temperature sensor, GPIOs and PGAs
> +
> +  The ADS1262 is a 32-bit, 38-kSPS, precision ADC with a programmable gain
> +  amplifier (PGA) and internal voltage reference. It features:
> +  - 11 single-ended or 5 differential input channels
> +  - Internal temperature sensor
> +  - Programmable gain amplifier (PGA) with gains from 1 to 32
> +  - Internal voltage reference
> +  - GPIO pins for control and monitoring
> +  - SPI interface

Normally, I would say that we want to have the devicetree as complete as
possible, but this chip has so many ways to wire it up, it would take days
to do a through review, so not sure where to draw the line. But I think there
are a few obvious ones we could still add, even if the driver doesn't use them
yet.

interrupts: for the /DRDY output

clks: for the clock input (see adi,ad7173 and adi,ad7192 for inspiration)

gpio-controller and #gpio-cells: for AIN pins used as GPIO

And for later...
* voltage supply outputs
* DAC outputs
* common mode voltage inputs
* REF{P.N}{1,2,3} supplies
* per-channel reference selection


> +
> +  Specifications about the part can be found at:
> +  https://www.ti.com/product/ADS1262
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,ads1262
> +
> +  reg:
> +    maxItems: 1
> +    description: SPI chip select number
> +
> +  spi-max-frequency:
> +    maximum: 7372800
> +    description: Maximum SPI clock frequency in Hz (7.3728 MHz)

Datasheet says minimum period is 125 ns, so that would make the max 8 MHz.

> +
> +  spi-cpha:
> +    type: boolean
> +    description: Required for SPI mode 1 operation
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: GPIO specifier for the reset pin (active low)
> +
> +  vref-supply:
> +    description: |
> +      The regulator supply for ADC reference voltage. If not specified,
> +      the internal 2.5V reference will be used.
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  '#io-channel-cells':
> +    const: 1
> +
> +  ti,pga-bypass:
> +    type: boolean
> +    description: |
> +      If true, bypass the PGA. If false or not specified, PGA is enabled.

Why is this one global but the actual gain per-channel?

> +
> +  ti,data-rate:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 15
> +    description: |
> +      Data acquisition rate in samples per second
> +      0: 2.5
> +      1: 5
> +      2: 10
> +      3: 16.6
> +      4: 20
> +      5: 50
> +      6: 60
> +      7: 100
> +      8: 400
> +      9: 1200
> +      10: 2400
> +      11: 4800
> +      12: 7200
> +      13: 14400
> +      14: 19200
> +      15: 38400

This is something that should be implemented with a sampling_frequency attribute
so that it can be changed at runtime rather than be hard-coded in the devicetree.

> +
> +required:
> +  - compatible
> +  - reg
> +  - spi-cpha
> +  - '#address-cells'
> +  - '#size-cells'
> +  - '#io-channel-cells'
> +
> +additionalProperties: false
> +
> +patternProperties:
> +  "^channel@([0-9]|1[0-1])$":
> +    type: object
> +    additionalProperties: false
> +    description: |
> +      Represents the external channels which are connected to the ADC.
> +      Channels 0-9 are available for external signals, channel 10 is AINCOM,
> +      and channel 11 is the internal temperature sensor.
> +
> +    properties:
> +      reg:
> +        description: |
> +          Channel number. It can have up to 10 channels numbered from 0 to 9,
> +          channel 10 is AINCOM, and channel 11 is the internal temperature sensor.
> +        items:
> +          - minimum: 0
> +            maximum: 11

Why allow the temperature input here but not other diagnostic input? Would make
more sense to me to omit temperature from the devicetree since it doesn't have
different wiring possibilities like the AIN pins.

> +
> +      diff-channels:
> +        description: |
> +          List of two channel numbers for differential measurement.
> +          First number is positive input, second is negative input.
> +          Not applicable for temperature sensor (channel 11).
> +        items:
> +          - minimum: 0
> +            maximum: 9
> +          - minimum: 0
> +            maximum: 9

Shouldn't this have max of 10 to include AINCOM?

> +
> +      ti,gain:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 0
> +        maximum: 5
> +        description: |
> +          PGA gain setting. Not applicable for temperature sensor (channel 11).
> +          0: 1 (default)
> +          1: 2
> +          2: 4
> +          3: 8
> +          4: 16
> +          5: 32
> +
> +    required:
> +      - reg
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      ads1262: adc@0 {
> +        compatible = "ti,ads1262";
> +        reg = <0>;
> +        spi-max-frequency = <7372800>;
> +        vref-supply = <&adc_vref>;
> +        spi-cpha;
> +        reset-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
> +        ti,pga-bypass;

If PGA is bypassed...

> +        ti,data-rate = <15>; /* 38400 SPS */
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        #io-channel-cells = <1>;
> +
> +        /* Single-ended channel */
> +        channel@0 {
> +          reg = <0>;
> +        };
> +
> +        /* Differential channel */
> +        channel@1 {
> +          reg = <1>;
> +          diff-channels = <1 2>;
> +          ti,gain = <2>; /* Gain of 4 */

...then gain doesn't make sense here.

> +        };
> +
> +        /* Temperature sensor */
> +        channel@11 {
> +          reg = <11>;
> +        };
> +      };
> +    };
> +...