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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aC89ve8FgIK4gej4@debian-BULLSEYE-live-builder-AMD64>
Date: Thu, 22 May 2025 12:07:41 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
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, dlechner@...libre.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

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

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?

Thanks,
Marcelo