lkml.org 
[lkml]   [2021]   [Jun]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2 8/8] dt-bindings: iio: afe: add binding for temperature-sense-amplifier
    From
    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@xiphos.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@xiphos.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@xiphos.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@vger.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@gmail.com>
    R: Peter Rosin <peda@axentia.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
    > >
    > >

    \
     
     \ /
      Last update: 2021-06-11 18:13    [W:3.348 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site