Message-ID: <eeb66815-3f7d-41fc-9d32-c28a3dda7749@baylibre.com>
Date: Mon, 16 Jun 2025 15:41:08 -0500
From: David Lechner <dlechner@...libre.com>
To: Conor Dooley <conor@...nel.org>,
 Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, jic23@...nel.org,
 lars@...afoo.de, Michael.Hennerich@...log.com, nuno.sa@...log.com,
 andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 linus.walleij@...aro.org, brgl@...ev.pl, marcelo.schmitt1@...il.com
Subject: Re: [PATCH v5 01/11] dt-bindings: iio: adc: Add AD4170

On 6/16/25 10:41 AM, Conor Dooley wrote:
> On Tue, Jun 10, 2025 at 05:31:04PM -0300, Marcelo Schmitt wrote:
>> Add device tree documentation for AD4170 and similar sigma-delta ADCs.
>> The AD4170 is a 24-bit, multichannel, sigma-delta ADC.
>>
>> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
>> ---
>> Change log v4 -> v5
>> - Dropped interrupt maxItems constraint.
>> - Spelled out RC acronym in reference-buffer description.
>> - Require to specify interrupt-names when using interrupts.
>> - Added interrupt-names to the examples.
>> - Made adi,excitation-pin properties identical to adi,ad4130.
>> - Removed interrupt-parent props from the examples.
>>
>> Proposing new types and ways of describing hardware for weigh scale load cells
>> and related sensors external to ADCs can lead to potential better description of
>> how those components connect to the ADC. However, we must use what already
>> exists for properties documenting features that are the same across different
>> devices. 
>>
>> Maybe, we could use generic defs to define adi,excitation-current-n-microamp and
>> adi,excitation-pin and avoid repetition with those. Though, that triggers a
>> dt_binding_check warning. Also, having mixed notation (some prop declarations
>> using defines and others not) seems to not be desirable.
>>
>> It looks like the only option left is making adi,excitation-pin properties
>> identical to adi,ad4130.
>>
>> On one hand, dropping adi,excitation-pin defs and making those properties
>> identical to adi,ad4130 preserves their syntax and semantics accross
>> dt-bindings. OTOH, we end up with more text repetition in the doc.
>>
>>
>>  .../bindings/iio/adc/adi,ad4170.yaml          | 564 ++++++++++++++++++
>>  MAINTAINERS                                   |   7 +
>>  2 files changed, 571 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
>> new file mode 100644
>> index 000000000000..e3249ec56a14
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
>> @@ -0,0 +1,564 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4170.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Analog Devices AD4170 and similar Analog to Digital Converters
>> +
>> +maintainers:
>> +  - Marcelo Schmitt <marcelo.schmitt@...log.com>
>> +
>> +description: |
>> +  Analog Devices AD4170 series of Sigma-delta Analog to Digital Converters.
>> +  Specifications can be found at:
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4170-4.pdf
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4190-4.pdf
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4195-4.pdf
>> +
>> +$ref: /schemas/spi/spi-peripheral-props.yaml#
>> +
>> +$defs:
>> +  reference-buffer:
>> +    description: |
>> +      Enable precharge buffer, full buffer, or skip reference buffering of
>> +      the positive/negative voltage reference. Because the output impedance
>> +      of the source driving the voltage reference inputs may be dynamic,
>> +      resistive/capacitive combinations of those inputs can cause DC gain
>> +      errors if the reference inputs go unbuffered into the ADC. Enable
>> +      reference buffering if the provided reference source has dynamic high
>> +      impedance output. Note the absolute voltage allowed on REFINn+ and REFINn-
>> +      inputs is from AVSS - 50 mV to AVDD + 50 mV when the reference buffers are
>> +      disabled but narrows to AVSS to AVDD when reference buffering is enabled
>> +      or in precharge mode. The valid options for this property are:
>> +      0: Reference precharge buffer.
>> +      1: Full reference buffering.
>> +      2: Bypass reference buffers (buffering disabled).
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [0, 1, 2]
>> +    default: 1
> 
> Why make this property a uint32, rather than a string where you can use
> something like "precharge", "full" and "bypass" (or "disabled")? The
> next similar device could use something slightly different then the
> binding becomes pretty clunky.
> Can you explain why this is a dt property rather than something
> adjustable at runtime?
> 
> Otherwise, what you have here looks sane enough to me - but I'd like to
> see some comments from Jonathan or David etc about your approach to the
> excitation properties.

This looks like something that should be in the devicetree to me. For example
if the external reference supply is high impedance, buffering is pretty
much required. And using precharge is an application design choice to
reduce THD at the expense of other limitations.


> 
> Cheers,
> Conor.
> 
>> +
>> +properties:

...

>> +allOf:
>> +  # Some devices don't have integrated DAC
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - adi,ad4190
>> +              - adi,ad4195
>> +    then:
>> +      properties:
>> +        ldac-gpios: false
>> +
>> +  # Require to specify the interrupt pin when using interrupts
>> +  - if:
>> +      required:
>> +        - interrupts
>> +    then:
>> +      required:
>> +        - interrupt-names
>> +
>> +  # If an external clock is set, the internal clock cannot go out and vice versa
>> +  - oneOf:
>> +      - required: [clocks]
>> +        properties:
>> +          '#clock-cells': false
>> +      - required: ['#clock-cells']
>> +        properties:
>> +          clocks: false
>> +
>> +patternProperties:

...

>> +required:
>> +  - compatible
>> +  - reg
>> +  - avdd-supply
>> +  - iovdd-supply
>> +  - spi-cpol
>> +  - spi-cpha
>> +
>> +unevaluatedProperties: false
>> +

It would be more logical to place these before patternProperties (actually
really before allOf) so that they are close to the properties that they are
referencing.

Powered by blists - more mailing lists