linux-kernel - Re: [PATCH] dt-bindings: iio: afe: voltage-divider: Add io-channel-cells

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: <20240127144920.455b6f0c@jic23-huawei>
Date: Sat, 27 Jan 2024 14:49:20 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Conor Dooley <conor@...nel.org>
Cc: Peter Rosin <peda@...ntia.se>, Naresh Solanki
 <naresh.solanki@...ements.com>, Lars-Peter Clausen <lars@...afoo.de>, Rob
 Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 mazziesaccount@...il.com, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: iio: afe: voltage-divider: Add
 io-channel-cells

On Sat, 27 Jan 2024 11:03:14 +0000
Conor Dooley <conor@...nel.org> wrote:

> On Sat, Jan 27, 2024 at 10:40:31AM +0100, Peter Rosin wrote:
> > 
> > 
> > 2024-01-26 at 23:14, Conor Dooley wrote:  
> > > On Fri, Jan 26, 2024 at 11:10:36PM +0530, Naresh Solanki wrote:  
> >   
> > > I did look at what you have there and I think your dts is wrong.
> > > 
> > > The iio-hwmon binding says:
> > > | description: >
> > > |   Bindings for hardware monitoring devices connected to ADC controllers
> > > |   supporting the Industrial I/O bindings.
> > > | 
> > > |   io-channels:
> > > |     minItems: 1
> > > |     maxItems: 51 # Should be enough
> > > |     description: >
> > > |       List of phandles to ADC channels to read the monitoring values
> > > 
> > > And then you have:
> > > |	iio-hwmon {
> > > |		compatible = "iio-hwmon";
> > > |		// Voltage sensors top to down
> > > |		io-channels = <&p12v_vd 0>, <&p5v_aux_vd 0>, <&p5v_bmc_aux_vd 0>, <&p3v3_aux_vd 0>,
> > > |			<&p3v3_bmc_aux_vd 0>, <&p1v8_bmc_aux_vd 0>, <&adc1 4>, <&adc0 2>, <&adc1 0>,
> > > |			<&p2V5_aux_vd 0>, <&p3v3_rtc_vd 0>;
> > > |	};
> > > |
> > > |	p12v_vd: voltage_divider1 {
> > > |		compatible = "voltage-divider";
> > > |		io-channels = <&adc1 3>;
> > > |		#io-channel-cells = <1>;
> > > |
> > > |		/* Scale the system voltage by 1127/127 to fit the ADC range.
> > > |		 * Use small nominator to prevent integer overflow.
> > > |		 */
> > > |		output-ohms = <15>;
> > > |		full-ohms = <133>;
> > > |	};
> > > 
> > > A voltage divider is _not_ an ADC channel, so I don't know why you are
> > > treating it as one in the iio-hwmon entry. Can you explain this please?  
> > 
> > This is the exact intent of the voltage divider (and the other bindings
> > handled by the iio-rescaler). The raw ADC reports the voltage at its input,
> > which is fine, but if there is an analog frontend in front of the ADC
> > such as a voltage divider the voltage at the ADC is not the interesting
> > property. You are likely to want the "real" voltage before the voltage
> > divider to better understand the value.
> > 
> > In this case it's much more interesting to see values such as 12.050V
> > which is presumably close to the nominal voltage (12V? guessing from
> > the node name) rather than some unscaled raw ADC voltage (in this
> > example it would be ~1.359V, which tells you rather little w/o rescaling
> > it first).  
> 
> Thanks for explaining it. Naresh, can you respin please with an
> explanation of why the property belongs in the binding please?

Agreed - the explanation of 'why' is needed.
As discussed, in this case the end consumer is iio-hwmon
(reflecting that the thing we are ultimately 'measuring' as a voltage
 of a wire that needs monitoring for hwmon usecases) and
that is measured via a voltage divider (hence that's providing
the measurement service) which in turn needs to consume the reading
from and ADC and hence the middle device is here acting as a
provider to iio-hwmon and a consumer of the ADC channel.

p.s. No one really likes the iio-hwmon binding because it is reflecting
     a usecase rather than hardware, but meh, it's been around a long time
     and we never figured out a better option.

    Things are much cleaner for circuits like the voltage divider.

> 
> > It's all in the description of the binding...  
> 
> Obviously it was not sufficiently clear, it's not as if I didn't look at
> it...

Given this device fits in both categories, perhaps a tiny bit of
additional documentation would help?

  '#io-channels-cells':
    description:
      In addition to consuming the measurement services of an ADC,
      the voltage divider can act as an provider of measurement
      services to other devices.
    const: 1

> 
> Cheers,
> Conor.