Message-Id: <CC0X6APQPBSY.1NUHSF8F0HWE1@shaak>
Date:   Fri, 11 Jun 2021 12:12:17 -0400
From:   "Liam Beguin" <liambeguin@...il.com>
To:     "Peter Rosin" <peda@...ntia.se>, <jic23@...nel.org>,
        <lars@...afoo.de>, <pmeerw@...erw.net>
Cc:     <linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <robh+dt@...nel.org>
Subject: Re: [PATCH v2 8/8] dt-bindings: iio: afe: add binding for
 temperature-sense-amplifier

Hi Peter,

On Fri Jun 11, 2021 at 3:37 AM EDT, Peter Rosin wrote:
> Hi!
>
> Should "amplifier" really be part of the name for this binding when it's
> now
> just a generic voltage-to-temperature rescale binding? Or, perhaps a
> better
> description is THE generic voltage-to-temperature rescale binding?

I went with temperature-sense-amplifier because it resembled what you
had for the current sense binding, and because of the generic scaling
factor in v2.

>
> But that's not a strong opinion, I know next to nothing about these
> things
> and it might be that an amplifier is involved in the vast majority of
> cases?
> Maybe it's enough to be more explicit in the describing text that the
> binding
> is intended for analog front ends lacking an amplifier as well? I just
> find
> it a bit confusing since there are actual HW that calls itself
> "temperature
> sence amplifiers" that I think this binding targets but then isn't
> exemplified anywhere.
>
> Also, it disturbs my sense of symmetry if volt->temp gets a generic
> binding like this, when volt->current and volt->volt have bindings for
> specific front ends. Again, it's not a strong opinion, I'm just pointing
> it
> out. For the record, I started out with a generic volt->current binding
> similar to this volt->temp binding, but got push-back so that we now
> have
> two specific volt->current bindings. Again, I'm just pointing this out,
> and
> I'm perfectly aware that "rules" and opinions change over time.

I agree with you that it can be confusing given that some temperature
sensors call themselves "temperature sense amplifiers".

In v1, temperature-sense-amplifier was used for that kind of device.

I liked having one binding per front end type like we had in v1 because
the devicetree ends up listing hardware parameters which I find is more
explicit.

I went with a generic one for v2 because I thought it would make it
applicable to other use-cases and simplify implementation.

I don't have strong feelings about keeping v2 over v1 bindings, and
given what we talked about v1 might be better here.

Do you have a preference?

>
> On 2021-06-07 16:47, Liam Beguin wrote:
> > From: Liam Beguin <lvb@...hos.com>
> > 
> > An ADC is often used to measure other quantities indirectly. This
> > binding describe such a use case, the measurement of a temperature
> > through an analog front end connected to a voltage channel.
> > 
> > Signed-off-by: Liam Beguin <lvb@...hos.com>
> > ---
> >  .../iio/afe/temperature-sense-amplifier.yaml  | 57 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 58 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml b/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> > new file mode 100644
> > index 000000000000..08f97f052a91
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/afe/temperature-sense-amplifier.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Temperature Sense Amplifier
> > +
> > +maintainers:
> > +  - Liam Beguin <lvb@...hos.com>
>
> Here, you claim maintainership...
>
> > +
> > +description: |
> > +  When an io-channel measures the output voltage of a temperature analog front
> > +  end such as an RTD (resistance thermometer) or a temperature to current
> > +  sensor, the interesting measurement is almost always the corresponding
> > +  temperature, not the voltage output. This binding describes such a circuit.
>
> Why would you convert from a voltage if you have a "temperature to
> current
> sensor"? Such a sensor should give you a current. Yeah yeah, I get it,
> you
> bake some resistance into the "gain" and you are done. But I think these
> things should be explicitly mentioned with examples. I think it would be
> a
> lot less terse if you spell out a couple of common ways to connect one
> of
> these linear temperature sensors and how that then maps to the gain that
> the
> consumer of the binding needs to use.
>
> It would also be a good thing to mention sensors by name, so that
> someone
> grepping for them finds this binding. It's a djungle out there.
>

You're right adding sensors names here would be very useful.

I'll rework the description with your suggestions and add specific
examples with maybe schematic drawings like in voltage-divider.yaml.

> > +
> > +properties:
> > +  compatible:
> > +    const: temperature-sense-amplifier
> > +
> > +  io-channels:
> > +    maxItems: 1
> > +    description: |
> > +      Channel node of a voltage io-channel.
> > +
> > +  '#io-channel-cells':
> > +    const: 1
> > +
> > +  sense-gain-mult:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Amplifier gain multiplier. The default is <1>.
> > +
> > +  sense-gain-div:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Amplifier gain divider. The default is <1>.
> > +
> > +  sense-offset-millicelsius:
> > +    description: Amplifier offset. The default is <0>.
> > +
> > +additionalProperties: false
> > +required:
> > +  - compatible
> > +  - io-channels
> > +
> > +examples:
> > +  - |
> > +    pt1000_1: temperature-sensor {
> > +        compatible = "temperature-sense-amplifier";
> > +        #io-channel-cells = <1>;
> > +        io-channels = <&temp_adc 3>;
> > +
> > +        sense-gain-mult = <1000000>;
> > +        sense-gain-div = <3908>;
> > +        sense-offset-millicelsius = <(-255885)>;
> > +    };
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e679d422b472..4f7b4ee9f19b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8887,6 +8887,7 @@ L:	linux-iio@...r.kernel.org
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/iio/afe/current-sense-amplifier.yaml
> >  F:	Documentation/devicetree/bindings/iio/afe/current-sense-shunt.yaml
> > +F:	Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml
>
> ...and here, you give maintenance to me. I didn't want all afe bindings
> so I
> didn't put an asterisk there for a reason :-)

Sorry about that :-)

I don't really know what the convention is here. I put myself as a
maintainer on the yaml file since I created it.

For the MAINTAINERS patch, would something like this be better?

IIO UNIT CONVERTER (TEMPERATURE)
M:	Liam Beguin <liambeguin@...il.com>
R:	Peter Rosin <peda@...ntia.se>
F:	Documentation/devicetree/bindings/iio/afe/temperature-sense-amplifier.yaml

Should I also add myself as a reviewer to the iio-rescaler driver for
the temperature related changes?

Also, I noticed an issue with the offset. When we're not using a
processed channel, the upstream channel scale has to be applied to the
offset which I forgot to do. I'm working on this for v3.

Thanks for your time,
Liam

> This binding is backed by the iio-rescale driver, so it's not totally
> alien
> for me to maintain it, but I'd be more happy if you listed yourself as I
> think
> you intended to?
>
> Cheers,
> Peter
>
> >  F:	Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml
> >  F:	drivers/iio/afe/iio-rescale.c
> >  
> > 

Powered by blists - more mailing lists