Message-ID: <bfd606f0-e253-4685-9025-5bdc7aafacbe@baylibre.com>
Date: Fri, 16 May 2025 11:06:30 -0500
From: David Lechner <dlechner@...libre.com>
To: Marcelo Schmitt <marcelo.schmitt1@...il.com>
Cc: Marcelo Schmitt <marcelo.schmitt@...log.com>, 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
Subject: Re: [PATCH v3 01/10] dt-bindings: iio: adc: Add AD4170

On 5/16/25 10:45 AM, Marcelo Schmitt wrote:
> ...
>>> +
>>> +    properties:
>>> +      adi,reference-select:
>>> +        description: |
>>> +          Selects the reference source to use when converting on the specific
>>> +          channel. Valid values are:
>>> +          0: Differential reference voltage REFIN+ - REFIN−.
>>> +          1: Differential reference voltage REFIN2+ - REFIN2−.
>>> +          2: Internal 2.5V referece (REFOUT) relative to AVSS.
>>> +          3: Analog supply voltage (AVDD) relative AVSS.
>>> +        $ref: /schemas/types.yaml#/definitions/uint8
>>> +        enum: [0, 1, 2, 3]
>> Using strings instead of int for this and most of the other custom enums here
>> would make them self-documenting and easier to use.
> 
> The numbers match the values that are documented in the datasheet for each
> option of voltage reference available to use with a channel. So we would be
> using numbers mostly to define values of some unit and pin numbers (e.g. 100 for
> the microamp property)? Not really excited about doing this change because I
> think it will make the dtb a bit larger and the driver code a bit more lengthy,
> but can do that for v4.

I don't think it is too bad since we have match_string() to convert the strings
to an enum value. So it would just be a matter of adding the string tables.

But I don't feel terribly strongly about it anyway.

> ...
>>> +
>>> +      adi,power-down-switch-pin:
>>> +        description:
>>> +          Number of the GPIO used as power-down switch for the bridge circuit.
>>> +        $ref: /schemas/types.yaml#/definitions/uint8
>>> +        enum: [0, 1]
>> This isn't required, so what is the default if omitted?
> 
> We don't care about it when the property is omitted.
> Do we need a default even when the property is not required and we don't care
> when it's not set?

Ah, in that case, maybe add a bit to the description to say that this
is omitted when there isn't a bridge circuit wired up.

> 
> ...
>>> +      diff-channels:
>>> +        description: |
>>> +          This property is used for defining the inputs of a differential
>>> +          voltage channel. The first value is the positive input and the second
>>> +          value is the negative input of the channel.
>>> +
>>> +          Besides the analog input pins AIN0 to AIN8, there are special inputs
>>> +          that can be selected with the following values:
>>> +          17: Internal temperature sensor
>>> +          18: (AVDD-AVSS)/5
>>> +          19: (IOVDD-DGND)/5
>>> +          20: DAC output
>>> +          21: ALDO
>>> +          22: DLDO
>>> +          23: AVSS
>>> +          24: DGND
>>> +          25: REFIN+
>>> +          26: REFIN-
>>> +          27: REFIN2+
>>> +          28: REFIN2-
>>> +          29: REFOUT
>>> +          For the internal temperature sensor, use the input number for both
>>> +          inputs (i.e. diff-channels = <17 17>).
>>> +        items:
>>> +          enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20, 21, 22, 23, 24, 25,
>>> +                 26, 27, 28, 29]
>>
>> A Header file with macros for these would be nice since it seems like we
>> have to use the higher-numbered ones a lot with the common-mode-channel
>> properties in the examples.
> 
> The RFC set had a header with macros for those numbers, but making dt properties
> "look nice" was said to no be a reason to have binding headers. 
> 
> https://lore.kernel.org/linux-iio/ikq55kcfu2lmxzeeobu4zwf67xypyikadnpycw2m4d7o6gvmi2@tkepvcvzqzoh/
> 

Hmm, OK I never got that complaint before. Although the headers I have made before
were defining arbitrary numbers for phandle cells, and not something from the
datasheet like this.

> Also, no other binding would use those values. So, we would have a header
> specific for adi,ad4170?

Yes.

> 
>>
>>> +
>>> +      single-channel: true
>>> +
>>> +      common-mode-channel: true
>>> +
>>> +      bipolar: true
>>> +
>>> +      adi,buffered-positive:
>>> +        description: |
>>> +          Enable precharge buffer, full buffer, or skip reference buffering of
>>> +          the positive voltage reference. Because the output impedance of the
>>> +          source driving the voltage reference inputs may be dynamic, RC
>>> +          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 positive reference inputs (REFIN+,
>>> +          REFIN2+) 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.
>>> +          0: Reference precharge buffer.
>>> +          1: Full Buffer.
>>> +          2: Bypass reference buffers (buffering disabled).
>>> +        $ref: /schemas/types.yaml#/definitions/uint8
>>> +        enum: [0, 1, 2]
>>> +        default: 1
>>> +
>>> +      adi,buffered-negative:
>>> +        description: |
>>> +          Enable precharge buffer, full buffer, or skip reference buffering of
>>> +          the negative voltage reference. Because the output impedance of the
>>> +          source driving the voltage reference inputs may be dynamic, RC
>>> +          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 negative reference inputs (REFIN-,
>>> +          REFIN2-) 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.
>>> +          0: Reference precharge buffer.
>>> +          1: Full Buffer.
>>> +          2: Bypass reference buffers (buffering disabled).
>>> +        $ref: /schemas/types.yaml#/definitions/uint8
>>> +        enum: [0, 1, 2]
>>> +        default: 1
>> Could make a $def for these too to reduce duplication.
> 
> I think so, but how? They are only documented here. I can merge them into a
> single adi,buffered property. That will also reduce duplication.

You already have $defs:, so just add precharge-buffer: there with
the description:, etc., then here, just:

	adi,buffered-positive:
	  $ref: '#/$defs/precharge-buffer'
	adi,buffered-negative:
	  $ref: '#/$defs/precharge-buffer'

Powered by blists - more mailing lists