Message-ID: <6973b6b1-eb49-24f5-d9fb-053cb4744196@axentia.se>
Date:   Fri, 11 Jun 2021 09:37:32 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Liam Beguin <liambeguin@...il.com>, 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!

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?

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.

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.

> +
> +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 :-)
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