Re: [PATCH v3 01/10] dt-bindings: iio: adc: Add AD4170

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 22 May 2025 12:07:41 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> ...
> >   
> > > +  sensor-node:
> > > +    type: object
> > > +    $ref: '#/$defs/ad4170-channel'  
> > 
> > I do not understand this binding. channel@ node is a channel and sensors
> > like rtd@ is also channel but also sensor. What is the point of channel@
> > which is not a sensor?
> >   
> The sensor node is meant to describe a channel that has extra setup
> configuration. For example, a common ADC channel could look like this
> 
>   -------- +VREF ------         +-------------------+
>     ´ `       ´ `              /                    |
>   /     \   /     \   /   --- <  IN+                |
>          `-´       `-´         |                    |
>   -------- -VREF ------        |                    |
>                                |            ADC     |
>   -------- +VREF ------        |                    |
>         ´ `       ´ `          |                    |
>   \   /     \   /     \   --- <  IN-                |
>    `-´       `-´               \       +VREF  -VREF |
>   -------- -VREF ------         +-------------------+
>                                          ^       ^
>                                          |       +---- External -VREF
>                                   External +VREF
> 
> The the channel@ node for that would look like
>     adc@0 {
>         ...
>         channel@0 {
>             reg = <0>;
>             bipolar;
>             diff-channels = <0 1>;
>         };
>     };
> 
> Though, some sigma-delta ADCs (including AD4170) are fancy and have features
> that relate to what is connected to the ADC inputs. For example, an RTD would
> be connected like
> 
>                                      External +VREF
>                                           |
>        +----------------------------------|-------+---- External -VREF
>        |           +----------------------+       |
>        +---Rref----+                +-----v-------v----+
>        |           |               /    REFIN+  REFIN- |
>    +---+-----------|------------> <  IN+               |
>    |               |              |                    |
> 3-wire RTD         |              |            ADC     |
>    |               |              |                    |
>    +--------------------+-------> <  IN-               |
>    |               |    |          \      GPIO2 GPIO3  |
>    v               |    |           +-------v-----v----+
>   GND              |    +-------------------+     |
>                    |       <--- IOUT1             |
>                    +------------------------------+
>                            <--- IOUT0
> 
> A better drawing can be found in AD4170 datasheet Figure 115. 3-Wire RTD Application.
> https://www.analog.com/media/en/technical-documentation/data-sheets/ad4170-4.pdf#unique_151_Connect_42_ID10430
> 
> Since the RTD sensor requires additional hardware connections, the proposed
> dt-binding describes those differently, e.g. 
> 
>     adc@0 {
>         ...
>         rtd@0 {
>             reg = <0>;
>             bipolar;
>             diff-channels = <0 1>;
>             adi,sensor-type = /bits/ 8 <1>;
>             adi,reference-select = <0>;
>             adi,excitation-pins = <19 20>;
>             adi,excitation-current-microamp = <500>;
>         };
> 
> That allows the ADC chip to be configured to provide the excitation signals to
> properly handle the RTD sensor. Because the hardware connections and
> operation requirements vary among those external sensor types (RTDs,
> thermocouples, load cell weigh scales), some properties are only applicable to
> specific sensor types. Also, because those are extra connections, the related
> properties are not meaningful to typical ADC channels.
> 
> Though, the supported features are not different from those described in
> Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml.
> In that binding, the properties related to external sensor support are set
> on the device node (not related to any channel). The binding proposed on AD4170
> RFC patch was more similar to adi,ad4130.yaml and somehow avoiding to introduce
> custom/new properties, but that lead to a lot of duplication.
> 
> The currently proposed dt-binding for ad4170 avoids repetition and, to some
> degree, tries to provide better description of connected bridge sensors and
> their related properties as that seemed to be one of Jonathan's suggestions
> to the RFC version.
> https://lore.kernel.org/linux-iio/20241219140353.787ffccc@jic23-huawei/

I don't always make good suggestions :(  DT maintainers have more experience
in making coherent and predictable bindings than I do.

One of the big problems is that we are fishing in the dark when trying to
make up 'generic' bindings.  Sometimes they just end up not extending well
at all.

> 
> Even though I'm naturally biased in favor of my own code, I see this is fairly
> different from bindings describing the exact same features and that can lead
> to confusion. I'll use the same types for properties that describe the same
> things/features. Would it also be preferred to drop the defs and just have 
> channel nodes?

If we did do channel nodes only. We have various options.
1) constraints against adi,sensor-types (which I think should probably be an enum)
   That should look similar to the node name constraints.
2) constrain only via documentation. (I don't like this at all)
3) have no constraints at all and rely on documenting what makes sense.
   I'm not entirely clear on whether we can ignore the sensor type etc and
   provide properties to control more directly everything to do with it?  So the
   excitation types etc

Jonathan

> 
> Thanks,
> Marcelo
>