Message-ID: <aCddgYRWrLPlGeuR@debian-BULLSEYE-live-builder-AMD64>
Date: Fri, 16 May 2025 12:45:05 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: David Lechner <dlechner@...libre.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

...
> > +
> > +    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.

...
> > +      adi,sensor-type:
> > +        description: |
> > +          Type of sensor connected to the device. Depending on the sensor type
> > +          (weigh scale, RTD, or thermocouple) the values of sensor-node
> > +          properties have slightly different constraints. This property
> > +          specifies which particular external sensor is connected to the ADC so
> > +          the sensor-node properties can be properly parsed and verified. The
> > +          possible sensor types are:
> > +          0: weigh scale;
> > +          1: RTD;
> > +          2: thermocouple.
> > +        $ref: /schemas/types.yaml#/definitions/uint8
> This property seems reduandant since it has to match the node name.
> 
> i.e. weighscale@... is is always adi,sensor-type = <0>; and so on.

Yes, can we rely on node names I'll do that for v4.

...
> > +
> > +      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?

...
> > +      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/

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

> 
> > +
> > +      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.

> 
> Also another case where string type would make more sense.
> 
> > +
> > +    required:
> > +      - reg
> 
> reg is already required by adc.yaml
> 
> > +
> > +    oneOf:
> > +      - required: [single-channel]
> 
> Is there a default for common-mode-channel if it isn't required in this case?

It should be required. Will make common-mode-channel required in this case. 


Thanks,
Marcelo

Powered by blists - more mailing lists