diff mbox series

[v1,1/3] dt-bindings: iio: adc: add AD4130

Message ID 20220413094011.185269-1-cosmin.tanislav@analog.com
State Superseded
Headers show
Series [v1,1/3] dt-bindings: iio: adc: add AD4130 | expand

Commit Message

Cosmin Tanislav April 13, 2022, 9:40 a.m. UTC
AD4130-8 is an ultra-low power, high precision,
measurement solution for low bandwidth battery
operated applications.

The fully integrated AFE (Analog Front-End)
includes a multiplexer for up to 16 single-ended
or 8 differential inputs, PGA (Programmable Gain
Amplifier), 24-bit Sigma-Delta ADC, on-chip
reference and oscillator, selectable filter
options, smart sequencer, sensor biasing and
excitation options, diagnostics, and a FIFO
buffer.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 .../bindings/iio/adc/adi,ad4130.yaml          | 255 ++++++++++++++++++
 1 file changed, 255 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml

Comments

Rob Herring (Arm) April 13, 2022, 12:26 p.m. UTC | #1
On Wed, 13 Apr 2022 12:40:09 +0300, Cosmin Tanislav wrote:
> AD4130-8 is an ultra-low power, high precision,
> measurement solution for low bandwidth battery
> operated applications.
> 
> The fully integrated AFE (Analog Front-End)
> includes a multiplexer for up to 16 single-ended
> or 8 differential inputs, PGA (Programmable Gain
> Amplifier), 24-bit Sigma-Delta ADC, on-chip
> reference and oscillator, selectable filter
> options, smart sequencer, sensor biasing and
> excitation options, diagnostics, and a FIFO
> buffer.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad4130.yaml          | 255 ++++++++++++++++++
>  1 file changed, 255 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: patternProperties:^channel@([0-9]|1[0-5])$:properties:adi,excitation-pin-0: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('minimum', 'maximum', 'default' were unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: patternProperties:^channel@([0-9]|1[0-5])$:properties:adi,excitation-pin-0: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: patternProperties:^channel@([0-9]|1[0-5])$:properties:adi,excitation-pin-0: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:interrupts: 'anyOf' conditional failed, one must be fixed:
	'minItems' is not one of ['maxItems', 'description', 'deprecated']
		hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values.
	'minItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref']
	'maxItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref']
	1 is less than the minimum of 2
		hint: Arrays must be described with a combination of minItems/maxItems/items
	hint: cell array properties must define how many entries and what the entries are when there is more than one entry.
	from schema $id: http://devicetree.org/meta-schemas/interrupts.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,int-ref-en: 'oneOf' conditional failed, one must be fixed:
	Additional properties are not allowed ('default' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,int-ref-en: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	Additional properties are not allowed ('type', 'default' were unexpected)
		hint: A vendor string property with exact values has an implicit type
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,int-ref-en: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,vbias-pins: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('items' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,vbias-pins: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,vbias-pins: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,bipolar: 'oneOf' conditional failed, one must be fixed:
	Additional properties are not allowed ('default' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,bipolar: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	Additional properties are not allowed ('type', 'default' were unexpected)
		hint: A vendor string property with exact values has an implicit type
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,bipolar: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: ignoring, error in schema: patternProperties: ^channel@([0-9]|1[0-5])$: properties: adi,excitation-pin-0
Error: Documentation/devicetree/bindings/iio/adc/adi,ad4130.example.dts:35.30-31 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:364: Documentation/devicetree/bindings/iio/adc/adi,ad4130.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1401: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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.
Andy Shevchenko April 13, 2022, 2:51 p.m. UTC | #2
On Wed, Apr 13, 2022 at 4:17 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:

It's good you provided documentation, but I think the part "ABI:" is
not needed in the Subject.

> AD4130-8 is an ultra-low power, high precision,
> measurement solution for low bandwidth battery
> operated applications.
>
> The fully integrated AFE (Analog Front-End)
> includes a multiplexer for up to 16 single-ended
> or 8 differential inputs, PGA (Programmable Gain
> Amplifier), 24-bit Sigma-Delta ADC, on-chip
> reference and oscillator, selectable filter
> options, smart sequencer, sensor biasing and
> excitation options, diagnostics, and a FIFO
> buffer.

Indentation issue as per patch 1.

...

> +               Set the filter mode of the differential channel. When the filter
> +               mode changes, the in_voltageY-voltageZ_sampling_frequency and
> +               in_voltageY-voltageZ_sampling_frequency_available attributes
> +               might also change to accomodate the new filter mode.

accommodate

> +               If the current sampling frequency is out of range for the new
> +               filter mode, the sampling frequency will be changed to the
> +               closest valid one.
Cosmin Tanislav April 14, 2022, 7:41 a.m. UTC | #3
On 4/13/22 17:51, Andy Shevchenko wrote:
> On Wed, Apr 13, 2022 at 4:17 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 
> It's good you provided documentation, but I think the part "ABI:" is
> not needed in the Subject.
> 

Then I guess I could merge this patch into the driver patch?

>> AD4130-8 is an ultra-low power, high precision,
>> measurement solution for low bandwidth battery
>> operated applications.
>>
>> The fully integrated AFE (Analog Front-End)
>> includes a multiplexer for up to 16 single-ended
>> or 8 differential inputs, PGA (Programmable Gain
>> Amplifier), 24-bit Sigma-Delta ADC, on-chip
>> reference and oscillator, selectable filter
>> options, smart sequencer, sensor biasing and
>> excitation options, diagnostics, and a FIFO
>> buffer.
> 
> Indentation issue as per patch 1.
> 
> ...
> 
>> +               Set the filter mode of the differential channel. When the filter
>> +               mode changes, the in_voltageY-voltageZ_sampling_frequency and
>> +               in_voltageY-voltageZ_sampling_frequency_available attributes
>> +               might also change to accomodate the new filter mode.
> 
> accommodate
> 
>> +               If the current sampling frequency is out of range for the new
>> +               filter mode, the sampling frequency will be changed to the
>> +               closest valid one.
> 
>
Jonathan Cameron April 16, 2022, 3 p.m. UTC | #4
On Wed, 13 Apr 2022 12:40:09 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> AD4130-8 is an ultra-low power, high precision,
> measurement solution for low bandwidth battery
> operated applications.
> 
> The fully integrated AFE (Analog Front-End)
> includes a multiplexer for up to 16 single-ended
> or 8 differential inputs, PGA (Programmable Gain
> Amplifier), 24-bit Sigma-Delta ADC, on-chip
> reference and oscillator, selectable filter
> options, smart sequencer, sensor biasing and
> excitation options, diagnostics, and a FIFO
> buffer.

Hi Cosmin,

Please wrap at around 70-75 chars for patch descriptions.  That
leaves a bit of room for indenting due to automated tooling
/ email threads before we hit 80 chars.  Definitely don't need
30 chars of room for it!

It seems you hit a lot of things that Rob's bot had found that
you would have seen on doing a build test.  Please make sure
you do one in future to save time!

Please use a cover letter for a series like this. It provides several
advantages:

1) Somewhere to add a brief introduction to the whole series.
2) Place for people to give tags for whole series that the b4 tool
   I use to pick up patches can then automatically find them from.
3) Gives series a nice unified name in patchwork ;)
https://patchwork.kernel.org/project/linux-iio/list/?series=631830

> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad4130.yaml          | 255 ++++++++++++++++++
>  1 file changed, 255 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> new file mode 100644
> index 000000000000..e9dce54e9802
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> @@ -0,0 +1,255 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4130.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD4130 ADC device driver
> +
> +maintainers:
> +  - Cosmin Tanislav <cosmin.tanislav@analog.com>
> +
> +description: |
> +  Bindings for the Analog Devices AD4130 ADC. Datasheet can be found here:
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4130-8.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad4130-8-16-lfcsp
> +      - adi,ad4130-8-16-wlcsp
> +      - adi,ad4130-8-24-lfcsp
> +      - adi,ad4130-8-24-wlcsp

What are the variants?   They look to possibly be package differences?
+ resolution differences.
They definitely need some description here.
It may make more sense to have one compatible and then some extra
booleans to say what it supported.

Long shot, but do the different packages have different model IDs?
The datasheet says
Model ID: 24-bit WLCSP Model ID. These bits are set by default for each model and
are read only.  If there is one for each of these models then it would be
better to have a single compatible and do the detection of variant in
the driver.

I'm not immediately spotting the resolution information in the data sheet.
It is marked Preliminary but if there are details missing, please mention
in cover letter so we don't go looking for information that doesn't exist.

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +    description: phandle to the master clock (mclk)
> +
> +  clock-names:
> +    items:
> +      - const: mclk
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 1
> +
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 1
> +    description:
> +      Default if not supplied is dout-int.
> +    items:
> +      enum:
> +        - dout-int
> +        - clk
> +        - p1

This is unusual.  It is probably helpful to add more description to
explain that the data ready/ fifo interrupt can be routed to any of these
pins.

> +        - dout
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  refin1-supply:
> +    description: refin1 supply. Can be used as reference for conversion.
> +
> +  refin2-supply:
> +    description: refin2 supply. Can be used as reference for conversion.
> +
> +  avdd-supply:
> +    description: AVDD voltage supply. Can be used as reference for conversion.

Whilst these are optional in theory, you should call out that they must be
provided if any of the channels use them as a reference.

> +
> +  iovdd-supply:
> +    description: IOVDD voltage supply. Used for the chip interface.
> +
> +  spi-max-frequency:
> +    maximum: 5000000
> +
> +  adi,mclk-sel:
> +    description: |
> +      Select the clock.
> +      0: Internal 76.8kHz clock.
> +      1: Internal 76.8kHz clock, output to the CLK pin.
> +      2: External 76.8kHz clock.
> +      3. External 153.6kHz clock.

For the external clocks, can we use the fact that one is supplied
as enough to tell us we should be using them?  Then query the
frequency directly from that clock?

If no clock provided then clearly internal.  All that is
necessary after that is a boolean to control if the CLK output
is enabled or not (and ideally constrain that to only be possible
if in internal clock mode).

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3]
> +    default: 0
> +
> +  adi,int-ref-en:

Mentioned below...

> +    description: |
> +      Specify if internal reference should be enabled.
> +    type: boolean
> +    default: true
> +
> +  adi,bipolar:
> +    description: Specify if the device should be used in bipolar mode.
> +    type: boolean
> +    default: false
> +
> +  adi,vbias-pins:
> +    description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to.
> +    items:
> +      minimum: 0
> +      maximum: 15

If I read things correctly VBIAS_CONTROL is a bitmap across the 16 input lines.
So why use a single value rather than either a list of pins, or a bitmap?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +patternProperties:
> +  "^channel@([0-9]|1[0-5])$":
> +    type: object
> +    $ref: adc.yaml
> +
> +    properties:
> +      reg:
> +        description: |
> +          The channel number.
> +        items:
> +          minimum: 0
> +          maximum: 15
> +
> +      diff-channels:
> +        description: |
> +          Besides the analog inputs available, internal inputs can be used.
> +          16: Internal temperature sensor.
> +          17: AVss
> +          18: Internal reference.
> +          19: DGND.
> +          20: (AVDD − AVSS)/6+
> +          21: (AVDD − AVSS)/6-
> +          22: (IOVDD − DGND)/6+
> +          23: (IOVDD − DGND)/6-
> +          24: (ALDO − AVSS)/6+
> +          25: (ALDO − AVSS)/6-
> +          26: (DLDO − DGND)/6+
> +          27: (DLDO − DGND)/6-
> +          28: V_MV_P
> +          29: V_MV_M
> +        $ref: adc.yaml
> +        items:
> +          minimum: 0
> +          maximum: 29

Interesting. So we have a part that has a 16 channel sequencer, but
can you have more channels as long as you don't want them all at once?
For example, I doubt anyone wants to permanently configure a device to monitor
the various supplies, but they will want to occasionally.

As such, perhaps we need to treat this device more flexibly?
There are obviously contraints on what channels + references make sense
but maybe we should allow more than 16 to be specified?

> +
> +      adi,reference-select:
> +        description: |
> +          Select the reference source to use when converting on the
> +          specific channel. Valid values are:
> +          0: REFIN1(+)/REFIN1(−).
> +          1: REFIN2(+)/REFIN2(−).
> +          2: REFOUT/AVSS (Internal reference)
> +          3: AVDD/AVSS
> +          If not specified, internal reference is used.

That's not a good idea if it might be turned off...
Perhaps a better approach would be to drop the int_ref_en and
simply walk the channels to find out if any of them use it.
If they don't turn it off, otherwise leave it on.

> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2, 3]
> +        default: 2
> +
> +      adi,excitation-pin-0:
> +        description: |
> +          Analog input to apply excitation current to while the channel
> +          is active.
> +        minimum: 0
> +        maximum: 15
> +        default: 0
> +
> +      adi,excitation-pin-1:
> +        description: |
> +          Analog input to apply excitation current to while this channel
> +          is active.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 0
> +        maximum: 15
> +        default: 0
> +
> +      adi,excitation-current-0-nanoamps:
> +        description: |
> +          Excitation current in nanoamps to be applied to pin specified in
> +          adi,excitation-pin-0 while this channel is active.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
> +        default: 0
> +
> +      adi,excitation-current-1-nanoamps:
> +        description: |
> +          Excitation current in nanoamps to be applied to pin specified in
> +          adi,excitation-pin-1 while this channel is active.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
> +        default: 0
> +
> +      adi,burnout-current-nanoamps:
> +        description: |
> +          Burnout current in nanoamps to be applied for this channel.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 500, 2000, 4000]
> +        default: 0
> +
> +      adi,buffered-positive:
> +        description: Enable buffered mode for positive input.
> +        type: boolean
> +
> +      adi,buffered-negative:
> +        description: Enable buffered mode for negative input.
> +        type: boolean
> +
> +    required:
> +      - reg
> +      - diff-channels
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      adc@0 {
> +        compatible = "adi,ad4130-8-24-wlcsp";
> +        reg = <0>;
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        spi-max-frequency = <5000000>;
> +        interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
> +        interrupt-parent = <&gpio>;
> +
> +        channel@0 {
> +          reg = <0>;
> +          /* AIN8, AIN9 */
> +          diff-channels = <8 9>;
> +        };
> +
> +        channel@1 {
> +          reg = <1>;
> +          /* AIN10, AIN11 */
> +          diff-channels = <10 11>;
> +        };
> +
> +        channel@2 {
> +          reg = <2>;
> +          /* Temperature Sensor, DGND */
> +          diff-channels = <16 19>;
> +        };
> +
> +        channel@3 {
> +          reg = <3>;
> +          /* Internal reference, DGND */
> +          diff-channels = <18 19>;
> +        };
> +
> +        channel@4 {
> +          reg = <4>;
> +          /* DGND, DGND */
> +          diff-channels = <19 19>;
> +        };
> +      };
> +    };
Cosmin Tanislav April 17, 2022, 10:16 a.m. UTC | #5
On 4/16/22 18:00, Jonathan Cameron wrote:
> On Wed, 13 Apr 2022 12:40:09 +0300
> Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 
>> AD4130-8 is an ultra-low power, high precision,
>> measurement solution for low bandwidth battery
>> operated applications.
>>
>> The fully integrated AFE (Analog Front-End)
>> includes a multiplexer for up to 16 single-ended
>> or 8 differential inputs, PGA (Programmable Gain
>> Amplifier), 24-bit Sigma-Delta ADC, on-chip
>> reference and oscillator, selectable filter
>> options, smart sequencer, sensor biasing and
>> excitation options, diagnostics, and a FIFO
>> buffer.
> 
> Hi Cosmin,
> 
> Please wrap at around 70-75 chars for patch descriptions.  That
> leaves a bit of room for indenting due to automated tooling
> / email threads before we hit 80 chars.  Definitely don't need
> 30 chars of room for it!
> 

Yeah, I'll do it.

> It seems you hit a lot of things that Rob's bot had found that
> you would have seen on doing a build test.  Please make sure
> you do one in future to save time!
> 

Yeah, sorry. I already fixed them for V2.

> Please use a cover letter for a series like this. It provides several
> advantages:
> 
> 1) Somewhere to add a brief introduction to the whole series.
> 2) Place for people to give tags for whole series that the b4 tool
>     I use to pick up patches can then automatically find them from.
> 3) Gives series a nice unified name in patchwork ;)
> https://patchwork.kernel.org/project/linux-iio/list/?series=631830
> 

Sure thing. I'll have it there for V2 anyway.

>>
>> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
>> ---
>>   .../bindings/iio/adc/adi,ad4130.yaml          | 255 ++++++++++++++++++
>>   1 file changed, 255 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
>> new file mode 100644
>> index 000000000000..e9dce54e9802
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
>> @@ -0,0 +1,255 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +# Copyright 2022 Analog Devices Inc.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4130.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Analog Devices AD4130 ADC device driver
>> +
>> +maintainers:
>> +  - Cosmin Tanislav <cosmin.tanislav@analog.com>
>> +
>> +description: |
>> +  Bindings for the Analog Devices AD4130 ADC. Datasheet can be found here:
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4130-8.pdf
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - adi,ad4130-8-16-lfcsp
>> +      - adi,ad4130-8-16-wlcsp
>> +      - adi,ad4130-8-24-lfcsp
>> +      - adi,ad4130-8-24-wlcsp
> 
> What are the variants?   They look to possibly be package differences?
> + resolution differences.
> They definitely need some description here.
> It may make more sense to have one compatible and then some extra
> booleans to say what it supported.
>

Packaging + available interrupt pins + resolution. Is having extra
booleans to describe what is supported really the best approach?
It's not really about how the hardware is configured anymore, is it?
They're different chips.

> Long shot, but do the different packages have different model IDs?
> The datasheet says
> Model ID: 24-bit WLCSP Model ID. These bits are set by default for each model and
> are read only.  If there is one for each of these models then it would be
> better to have a single compatible and do the detection of variant in
> the driver.
> 
> I'm not immediately spotting the resolution information in the data sheet.
> It is marked Preliminary but if there are details missing, please mention
> in cover letter so we don't go looking for information that doesn't exist.
>

I don't have enough information about the other models to know what
Model IDs they will have. That's why I took this approach.

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: phandle to the master clock (mclk)
>> +
>> +  clock-names:
>> +    items:
>> +      - const: mclk
>> +
>> +  interrupts:
>> +    minItems: 1
>> +    maxItems: 1
>> +
>> +  interrupt-names:
>> +    minItems: 1
>> +    maxItems: 1
>> +    description:
>> +      Default if not supplied is dout-int.
>> +    items:
>> +      enum:
>> +        - dout-int
>> +        - clk
>> +        - p1
> 
> This is unusual.  It is probably helpful to add more description to
> explain that the data ready/ fifo interrupt can be routed to any of these
> pins.

Is it unusual? ADIS16480 follows a similar approach.

description: |
   Specify which interrupt pin should be configured as Data Ready / FIFO
   interrupt.
   Default if not supplied is dout-int.

How does this sound?

> 
>> +        - dout
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +  refin1-supply:
>> +    description: refin1 supply. Can be used as reference for conversion.
>> +
>> +  refin2-supply:
>> +    description: refin2 supply. Can be used as reference for conversion.
>> +
>> +  avdd-supply:
>> +    description: AVDD voltage supply. Can be used as reference for conversion.
> 
> Whilst these are optional in theory, you should call out that they must be
> provided if any of the channels use them as a reference.
> 

I thought that "Can be used as reference for conversion." + it being an
option in adi,reference-select property would make it obvious, no?

>> +
>> +  iovdd-supply:
>> +    description: IOVDD voltage supply. Used for the chip interface.
>> +
>> +  spi-max-frequency:
>> +    maximum: 5000000
>> +
>> +  adi,mclk-sel:
>> +    description: |
>> +      Select the clock.
>> +      0: Internal 76.8kHz clock.
>> +      1: Internal 76.8kHz clock, output to the CLK pin.
>> +      2: External 76.8kHz clock.
>> +      3. External 153.6kHz clock.
> 
> For the external clocks, can we use the fact that one is supplied
> as enough to tell us we should be using them?  Then query the
> frequency directly from that clock?
> 

Aren't we supposed to set the frequency of that clock ourselves,
in the driver?

> If no clock provided then clearly internal.  All that is
> necessary after that is a boolean to control if the CLK output
> is enabled or not (and ideally constrain that to only be possible
> if in internal clock mode).
> 

Well...

So, mclk present => external, not present => internal.
adi,int-clk-out-enable to specify if the internal clock should be
exposed? adi,ext-clk-freq to specify the desired clock speed of the
external clk?

>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [0, 1, 2, 3]
>> +    default: 0
>> +
>> +  adi,int-ref-en:
> 
> Mentioned below...
> 
>> +    description: |
>> +      Specify if internal reference should be enabled.
>> +    type: boolean
>> +    default: true
>> +
>> +  adi,bipolar:
>> +    description: Specify if the device should be used in bipolar mode.
>> +    type: boolean
>> +    default: false
>> +
>> +  adi,vbias-pins:
>> +    description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to.
>> +    items:
>> +      minimum: 0
>> +      maximum: 15
> 
> If I read things correctly VBIAS_CONTROL is a bitmap across the 16 input lines.
> So why use a single value rather than either a list of pins, or a bitmap?
> 

Umm. Isn't this a list of pins? That's why everything is plural here.
I guess I should add `maxItems: 16`?
I already added `$ref: /schemas/types.yaml#/definitions/uint32-array`.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +patternProperties:
>> +  "^channel@([0-9]|1[0-5])$":
>> +    type: object
>> +    $ref: adc.yaml
>> +
>> +    properties:
>> +      reg:
>> +        description: |
>> +          The channel number.
>> +        items:
>> +          minimum: 0
>> +          maximum: 15
>> +
>> +      diff-channels:
>> +        description: |
>> +          Besides the analog inputs available, internal inputs can be used.
>> +          16: Internal temperature sensor.
>> +          17: AVss
>> +          18: Internal reference.
>> +          19: DGND.
>> +          20: (AVDD − AVSS)/6+
>> +          21: (AVDD − AVSS)/6-
>> +          22: (IOVDD − DGND)/6+
>> +          23: (IOVDD − DGND)/6-
>> +          24: (ALDO − AVSS)/6+
>> +          25: (ALDO − AVSS)/6-
>> +          26: (DLDO − DGND)/6+
>> +          27: (DLDO − DGND)/6-
>> +          28: V_MV_P
>> +          29: V_MV_M
>> +        $ref: adc.yaml
>> +        items:
>> +          minimum: 0
>> +          maximum: 29
> 
> Interesting. So we have a part that has a 16 channel sequencer, but
> can you have more channels as long as you don't want them all at once?
> For example, I doubt anyone wants to permanently configure a device to monitor
> the various supplies, but they will want to occasionally.
> 
> As such, perhaps we need to treat this device more flexibly?
> There are obviously contraints on what channels + references make sense
> but maybe we should allow more than 16 to be specified?
> 

Ehhhhh. Look at the driver. It's already pushing 2k+ lines with
the 8 setups for 16 channels situation + all the extra options the
chip provides. If we also make it so that channels are rewritten at
runtime, it will turn into a mess. Or at least I don't see a clean
way of adding that. Besides, then I'd have to do all these extra
allocations depending on the number of channels in the device tree...
It gets complicated. If a customer expresses his interest in this,
I guess I'll have to add it.
Also, presumably the extra inputs are marketed as diagnostics.

>> +
>> +      adi,reference-select:
>> +        description: |
>> +          Select the reference source to use when converting on the
>> +          specific channel. Valid values are:
>> +          0: REFIN1(+)/REFIN1(−).
>> +          1: REFIN2(+)/REFIN2(−).
>> +          2: REFOUT/AVSS (Internal reference)
>> +          3: AVDD/AVSS
>> +          If not specified, internal reference is used.
> 
> That's not a good idea if it might be turned off...
> Perhaps a better approach would be to drop the int_ref_en and
> simply walk the channels to find out if any of them use it.
> If they don't turn it off, otherwise leave it on.
> 

Yeah, I guess I could do that.

>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        enum: [0, 1, 2, 3]
>> +        default: 2
>> +
>> +      adi,excitation-pin-0:
>> +        description: |
>> +          Analog input to apply excitation current to while the channel
>> +          is active.
>> +        minimum: 0
>> +        maximum: 15
>> +        default: 0
>> +
>> +      adi,excitation-pin-1:
>> +        description: |
>> +          Analog input to apply excitation current to while this channel
>> +          is active.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        minimum: 0
>> +        maximum: 15
>> +        default: 0
>> +
>> +      adi,excitation-current-0-nanoamps:
>> +        description: |
>> +          Excitation current in nanoamps to be applied to pin specified in
>> +          adi,excitation-pin-0 while this channel is active.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
>> +        default: 0
>> +
>> +      adi,excitation-current-1-nanoamps:
>> +        description: |
>> +          Excitation current in nanoamps to be applied to pin specified in
>> +          adi,excitation-pin-1 while this channel is active.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
>> +        default: 0
>> +
>> +      adi,burnout-current-nanoamps:
>> +        description: |
>> +          Burnout current in nanoamps to be applied for this channel.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        enum: [0, 500, 2000, 4000]
>> +        default: 0
>> +
>> +      adi,buffered-positive:
>> +        description: Enable buffered mode for positive input.
>> +        type: boolean
>> +
>> +      adi,buffered-negative:
>> +        description: Enable buffered mode for negative input.
>> +        type: boolean
>> +
>> +    required:
>> +      - reg
>> +      - diff-channels
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    spi {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      adc@0 {
>> +        compatible = "adi,ad4130-8-24-wlcsp";
>> +        reg = <0>;
>> +
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        spi-max-frequency = <5000000>;
>> +        interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
>> +        interrupt-parent = <&gpio>;
>> +
>> +        channel@0 {
>> +          reg = <0>;
>> +          /* AIN8, AIN9 */
>> +          diff-channels = <8 9>;
>> +        };
>> +
>> +        channel@1 {
>> +          reg = <1>;
>> +          /* AIN10, AIN11 */
>> +          diff-channels = <10 11>;
>> +        };
>> +
>> +        channel@2 {
>> +          reg = <2>;
>> +          /* Temperature Sensor, DGND */
>> +          diff-channels = <16 19>;
>> +        };
>> +
>> +        channel@3 {
>> +          reg = <3>;
>> +          /* Internal reference, DGND */
>> +          diff-channels = <18 19>;
>> +        };
>> +
>> +        channel@4 {
>> +          reg = <4>;
>> +          /* DGND, DGND */
>> +          diff-channels = <19 19>;
>> +        };
>> +      };
>> +    };
>
Jonathan Cameron April 24, 2022, 4:05 p.m. UTC | #6
> 
> >>
> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> >> ---
> >>   .../bindings/iio/adc/adi,ad4130.yaml          | 255 ++++++++++++++++++
> >>   1 file changed, 255 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> >> new file mode 100644
> >> index 000000000000..e9dce54e9802
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> >> @@ -0,0 +1,255 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +# Copyright 2022 Analog Devices Inc.
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4130.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Analog Devices AD4130 ADC device driver
> >> +
> >> +maintainers:
> >> +  - Cosmin Tanislav <cosmin.tanislav@analog.com>
> >> +
> >> +description: |
> >> +  Bindings for the Analog Devices AD4130 ADC. Datasheet can be found here:
> >> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4130-8.pdf
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - adi,ad4130-8-16-lfcsp
> >> +      - adi,ad4130-8-16-wlcsp
> >> +      - adi,ad4130-8-24-lfcsp
> >> +      - adi,ad4130-8-24-wlcsp  
> > 
> > What are the variants?   They look to possibly be package differences?
> > + resolution differences.
> > They definitely need some description here.
> > It may make more sense to have one compatible and then some extra
> > booleans to say what it supported.
> >  
> 
> Packaging + available interrupt pins + resolution. Is having extra
> booleans to describe what is supported really the best approach?
> It's not really about how the hardware is configured anymore, is it?
> They're different chips.

It's unusual to have compatibles for packaging alone and I couldn't find
any clear references to the variants.  Maybe best we can do here is
add a bunch of documentation.

> 
> > Long shot, but do the different packages have different model IDs?
> > The datasheet says
> > Model ID: 24-bit WLCSP Model ID. These bits are set by default for each model and
> > are read only.  If there is one for each of these models then it would be
> > better to have a single compatible and do the detection of variant in
> > the driver.
> > 
> > I'm not immediately spotting the resolution information in the data sheet.
> > It is marked Preliminary but if there are details missing, please mention
> > in cover letter so we don't go looking for information that doesn't exist.
> >  
> 
> I don't have enough information about the other models to know what
> Model IDs they will have. That's why I took this approach.

My inclination is to probably not add the compatibles for those until we
do have that information.

> 
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  clocks:
> >> +    maxItems: 1
> >> +    description: phandle to the master clock (mclk)
> >> +
> >> +  clock-names:
> >> +    items:
> >> +      - const: mclk
> >> +
> >> +  interrupts:
> >> +    minItems: 1
> >> +    maxItems: 1
> >> +
> >> +  interrupt-names:
> >> +    minItems: 1
> >> +    maxItems: 1
> >> +    description:
> >> +      Default if not supplied is dout-int.
> >> +    items:
> >> +      enum:
> >> +        - dout-int
> >> +        - clk
> >> +        - p1  
> > 
> > This is unusual.  It is probably helpful to add more description to
> > explain that the data ready/ fifo interrupt can be routed to any of these
> > pins.  
> 
> Is it unusual? ADIS16480 follows a similar approach.

I think I got thrown by the large number of choices for a single interrupt.
Flexibility of interrupt routing is common but I've never seen 4 choices for
a single interrupt before :)

> 
> description: |
>    Specify which interrupt pin should be configured as Data Ready / FIFO
>    interrupt.
>    Default if not supplied is dout-int.
> 
> How does this sound?

Sounds good to me.

> 
> >   
> >> +        - dout
> >> +
> >> +  '#address-cells':
> >> +    const: 1
> >> +
> >> +  '#size-cells':
> >> +    const: 0
> >> +
> >> +  refin1-supply:
> >> +    description: refin1 supply. Can be used as reference for conversion.
> >> +
> >> +  refin2-supply:
> >> +    description: refin2 supply. Can be used as reference for conversion.
> >> +
> >> +  avdd-supply:
> >> +    description: AVDD voltage supply. Can be used as reference for conversion.  
> > 
> > Whilst these are optional in theory, you should call out that they must be
> > provided if any of the channels use them as a reference.
> >   
> 
> I thought that "Can be used as reference for conversion." + it being an
> option in adi,reference-select property would make it obvious, no?

More obvious is always good, but you are probably right and I was just
not terribly awake when I wrote that :)

> 
> >> +
> >> +  iovdd-supply:
> >> +    description: IOVDD voltage supply. Used for the chip interface.
> >> +
> >> +  spi-max-frequency:
> >> +    maximum: 5000000
> >> +
> >> +  adi,mclk-sel:
> >> +    description: |
> >> +      Select the clock.
> >> +      0: Internal 76.8kHz clock.
> >> +      1: Internal 76.8kHz clock, output to the CLK pin.
> >> +      2: External 76.8kHz clock.
> >> +      3. External 153.6kHz clock.  
> > 
> > For the external clocks, can we use the fact that one is supplied
> > as enough to tell us we should be using them?  Then query the
> > frequency directly from that clock?
> >   
> 
> Aren't we supposed to set the frequency of that clock ourselves,
> in the driver?

True, I got that backwards.

> 
> > If no clock provided then clearly internal.  All that is
> > necessary after that is a boolean to control if the CLK output
> > is enabled or not (and ideally constrain that to only be possible
> > if in internal clock mode).
> >   
> 
> Well...
> 
> So, mclk present => external, not present => internal.
> adi,int-clk-out-enable to specify if the internal clock should be
> exposed? adi,ext-clk-freq to specify the desired clock speed of the
> external clk?

Yes that sounds good to me.  I just don't like magic numbers
in bindings if we can avoid them.   With the above a reader should
be able to figure out what is going on without reading this doc.

> 
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    enum: [0, 1, 2, 3]
> >> +    default: 0
> >> +
> >> +  adi,int-ref-en:  
> > 
> > Mentioned below...
> >   
> >> +    description: |
> >> +      Specify if internal reference should be enabled.
> >> +    type: boolean
> >> +    default: true
> >> +
> >> +  adi,bipolar:
> >> +    description: Specify if the device should be used in bipolar mode.
> >> +    type: boolean
> >> +    default: false
> >> +
> >> +  adi,vbias-pins:
> >> +    description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to.
> >> +    items:
> >> +      minimum: 0
> >> +      maximum: 15  
> > 
> > If I read things correctly VBIAS_CONTROL is a bitmap across the 16 input lines.
> > So why use a single value rather than either a list of pins, or a bitmap?
> >   
> 
> Umm. Isn't this a list of pins? That's why everything is plural here.
> I guess I should add `maxItems: 16`?
> I already added `$ref: /schemas/types.yaml#/definitions/uint32-array`.

Ah. Yes, with an array and maxItems this will be fine I think.

> 
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +  - interrupts
> >> +
> >> +patternProperties:
> >> +  "^channel@([0-9]|1[0-5])$":
> >> +    type: object
> >> +    $ref: adc.yaml
> >> +
> >> +    properties:
> >> +      reg:
> >> +        description: |
> >> +          The channel number.
> >> +        items:
> >> +          minimum: 0
> >> +          maximum: 15
> >> +
> >> +      diff-channels:
> >> +        description: |
> >> +          Besides the analog inputs available, internal inputs can be used.
> >> +          16: Internal temperature sensor.
> >> +          17: AVss
> >> +          18: Internal reference.
> >> +          19: DGND.
> >> +          20: (AVDD − AVSS)/6+
> >> +          21: (AVDD − AVSS)/6-
> >> +          22: (IOVDD − DGND)/6+
> >> +          23: (IOVDD − DGND)/6-
> >> +          24: (ALDO − AVSS)/6+
> >> +          25: (ALDO − AVSS)/6-
> >> +          26: (DLDO − DGND)/6+
> >> +          27: (DLDO − DGND)/6-
> >> +          28: V_MV_P
> >> +          29: V_MV_M
> >> +        $ref: adc.yaml
> >> +        items:
> >> +          minimum: 0
> >> +          maximum: 29  
> > 
> > Interesting. So we have a part that has a 16 channel sequencer, but
> > can you have more channels as long as you don't want them all at once?
> > For example, I doubt anyone wants to permanently configure a device to monitor
> > the various supplies, but they will want to occasionally.
> > 
> > As such, perhaps we need to treat this device more flexibly?
> > There are obviously contraints on what channels + references make sense
> > but maybe we should allow more than 16 to be specified?
> >   
> 
> Ehhhhh. Look at the driver. It's already pushing 2k+ lines with
> the 8 setups for 16 channels situation + all the extra options the
> chip provides. If we also make it so that channels are rewritten at
> runtime, it will turn into a mess. Or at least I don't see a clean
> way of adding that. Besides, then I'd have to do all these extra
> allocations depending on the number of channels in the device tree...
> It gets complicated. If a customer expresses his interest in this,
> I guess I'll have to add it.
> Also, presumably the extra inputs are marketed as diagnostics.

Another approach would be to 'always' provide those diagnostic channels
but not via buffered interface (if that's sensible to do).  Then potentially
drop them from this binding?

Maybe waiting for customer demand is the best way to go.  Even if you
added a path to read them without them being in DT later it should be
easy enough to maintain backwards compatibility.

Jonathan
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
new file mode 100644
index 000000000000..e9dce54e9802
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
@@ -0,0 +1,255 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2022 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4130.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4130 ADC device driver
+
+maintainers:
+  - Cosmin Tanislav <cosmin.tanislav@analog.com>
+
+description: |
+  Bindings for the Analog Devices AD4130 ADC. Datasheet can be found here:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4130-8.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad4130-8-16-lfcsp
+      - adi,ad4130-8-16-wlcsp
+      - adi,ad4130-8-24-lfcsp
+      - adi,ad4130-8-24-wlcsp
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: phandle to the master clock (mclk)
+
+  clock-names:
+    items:
+      - const: mclk
+
+  interrupts:
+    minItems: 1
+    maxItems: 1
+
+  interrupt-names:
+    minItems: 1
+    maxItems: 1
+    description:
+      Default if not supplied is dout-int.
+    items:
+      enum:
+        - dout-int
+        - clk
+        - p1
+        - dout
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  refin1-supply:
+    description: refin1 supply. Can be used as reference for conversion.
+
+  refin2-supply:
+    description: refin2 supply. Can be used as reference for conversion.
+
+  avdd-supply:
+    description: AVDD voltage supply. Can be used as reference for conversion.
+
+  iovdd-supply:
+    description: IOVDD voltage supply. Used for the chip interface.
+
+  spi-max-frequency:
+    maximum: 5000000
+
+  adi,mclk-sel:
+    description: |
+      Select the clock.
+      0: Internal 76.8kHz clock.
+      1: Internal 76.8kHz clock, output to the CLK pin.
+      2: External 76.8kHz clock.
+      3. External 153.6kHz clock.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3]
+    default: 0
+
+  adi,int-ref-en:
+    description: |
+      Specify if internal reference should be enabled.
+    type: boolean
+    default: true
+
+  adi,bipolar:
+    description: Specify if the device should be used in bipolar mode.
+    type: boolean
+    default: false
+
+  adi,vbias-pins:
+    description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to.
+    items:
+      minimum: 0
+      maximum: 15
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+patternProperties:
+  "^channel@([0-9]|1[0-5])$":
+    type: object
+    $ref: adc.yaml
+
+    properties:
+      reg:
+        description: |
+          The channel number.
+        items:
+          minimum: 0
+          maximum: 15
+
+      diff-channels:
+        description: |
+          Besides the analog inputs available, internal inputs can be used.
+          16: Internal temperature sensor.
+          17: AVss
+          18: Internal reference.
+          19: DGND.
+          20: (AVDD − AVSS)/6+
+          21: (AVDD − AVSS)/6-
+          22: (IOVDD − DGND)/6+
+          23: (IOVDD − DGND)/6-
+          24: (ALDO − AVSS)/6+
+          25: (ALDO − AVSS)/6-
+          26: (DLDO − DGND)/6+
+          27: (DLDO − DGND)/6-
+          28: V_MV_P
+          29: V_MV_M
+        $ref: adc.yaml
+        items:
+          minimum: 0
+          maximum: 29
+
+      adi,reference-select:
+        description: |
+          Select the reference source to use when converting on the
+          specific channel. Valid values are:
+          0: REFIN1(+)/REFIN1(−).
+          1: REFIN2(+)/REFIN2(−).
+          2: REFOUT/AVSS (Internal reference)
+          3: AVDD/AVSS
+          If not specified, internal reference is used.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 1, 2, 3]
+        default: 2
+
+      adi,excitation-pin-0:
+        description: |
+          Analog input to apply excitation current to while the channel
+          is active.
+        minimum: 0
+        maximum: 15
+        default: 0
+
+      adi,excitation-pin-1:
+        description: |
+          Analog input to apply excitation current to while this channel
+          is active.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        minimum: 0
+        maximum: 15
+        default: 0
+
+      adi,excitation-current-0-nanoamps:
+        description: |
+          Excitation current in nanoamps to be applied to pin specified in
+          adi,excitation-pin-0 while this channel is active.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
+        default: 0
+
+      adi,excitation-current-1-nanoamps:
+        description: |
+          Excitation current in nanoamps to be applied to pin specified in
+          adi,excitation-pin-1 while this channel is active.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
+        default: 0
+
+      adi,burnout-current-nanoamps:
+        description: |
+          Burnout current in nanoamps to be applied for this channel.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 500, 2000, 4000]
+        default: 0
+
+      adi,buffered-positive:
+        description: Enable buffered mode for positive input.
+        type: boolean
+
+      adi,buffered-negative:
+        description: Enable buffered mode for negative input.
+        type: boolean
+
+    required:
+      - reg
+      - diff-channels
+
+additionalProperties: false
+
+examples:
+  - |
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      adc@0 {
+        compatible = "adi,ad4130-8-24-wlcsp";
+        reg = <0>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        spi-max-frequency = <5000000>;
+        interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
+        interrupt-parent = <&gpio>;
+
+        channel@0 {
+          reg = <0>;
+          /* AIN8, AIN9 */
+          diff-channels = <8 9>;
+        };
+
+        channel@1 {
+          reg = <1>;
+          /* AIN10, AIN11 */
+          diff-channels = <10 11>;
+        };
+
+        channel@2 {
+          reg = <2>;
+          /* Temperature Sensor, DGND */
+          diff-channels = <16 19>;
+        };
+
+        channel@3 {
+          reg = <3>;
+          /* Internal reference, DGND */
+          diff-channels = <18 19>;
+        };
+
+        channel@4 {
+          reg = <4>;
+          /* DGND, DGND */
+          diff-channels = <19 19>;
+        };
+      };
+    };