| Message ID | 20210607144718.1724413-9-liambeguin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | iio: afe: add temperature rescaling support | expand |
On Mon, 7 Jun 2021 10:47:18 -0400 Liam Beguin <liambeguin@gmail.com> 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> > + > +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. Perhaps add something about this only covering the linear cases... > + > +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>. Whilst it may seem obvious I'd like to see a statement of how these are used somewhere in here. temp_celcius = voltage * gain-mult / gain-div + offset Mainly because those familiar with the IIO usage of offset would expect (voltage + offset) * gain-mult/gain-div which doesn't make sense for this device but might leave people confused! > + > +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 > F: Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml > F: drivers/iio/afe/iio-rescale.c >
On Wed Jun 9, 2021 at 4:52 PM EDT, Jonathan Cameron wrote: > On Mon, 7 Jun 2021 10:47:18 -0400 > Liam Beguin <liambeguin@gmail.com> 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> > > + > > +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. > > Perhaps add something about this only covering the linear cases... > Okay, will do. > > + > > +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>. > > Whilst it may seem obvious I'd like to see a statement of > how these are used somewhere in here. > > temp_celcius = voltage * gain-mult / gain-div + offset > > Mainly because those familiar with the IIO usage of offset > would expect > (voltage + offset) * gain-mult/gain-div > > which doesn't make sense for this device but might leave > people confused! > You're right, I'll add an explicit statement of how these parameters are used. Thanks, Liam > > + > > +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 > > F: Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml > > F: drivers/iio/afe/iio-rescale.c > >
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@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. > + > +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 :-) 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 > >
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 > > > >
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> + +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. + +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 F: Documentation/devicetree/bindings/iio/afe/voltage-divider.yaml F: drivers/iio/afe/iio-rescale.c