Re: [PATCH v5 1/2] dt-bindings: hwmon: add tmp464.yaml

[Guenter Roeck]

On 2/21/22 01:07, Krzysztof Adamski wrote:
> Dnia Fri, Feb 18, 2022 at 07:09:07AM -0800, Guenter Roeck napisał(a):
> > From: Agathe Porte <agathe.porte@xxxxxxxxx>
> >
> > Add basic description of the tmp464 driver DT bindings.
> >
> > Signed-off-by: Agathe Porte <agathe.porte@xxxxxxxxx>
> > Cc: Krzysztof Adamski <krzysztof.adamski@xxxxxxxxx>
> > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > ---
> > v5:
> > - Dropped ti,n-factor from channel@0 example. Added additional
> >  channel to examples to show positive ti,n-factor property.
> >
> > v4:
> > - No changes
> >
> > v3:
> > - Addedd support for TMP468.
> > - Changed number of channels from 0..3 (which was wrong anyway) to 0..8.
> > - Changed value range for ti,n-factor to int8, with an example for
> >  a negative value.
> > - Added myself as driver maintainer.
> >
> > .../devicetree/bindings/hwmon/ti,tmp464.yaml  | 114 ++++++++++++++++++
> > MAINTAINERS                                   |   7 ++
> > 2 files changed, 121 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
> > new file mode 100644
> > index 000000000000..14f6a3412b8c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/ti,tmp464.yaml
> > @@ -0,0 +1,114 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/ti,tmp464.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: TMP464 and TMP468 temperature sensors
> > +
> > +maintainers:
> > +  - Agathe Porte <agathe.porte@xxxxxxxxx>
> > +
> > +description: |
> > +  ±0.0625°C Remote and Local temperature sensor
> > +  https://www.ti.com/lit/ds/symlink/tmp464.pdf
> > +  https://www.ti.com/lit/ds/symlink/tmp468.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ti,tmp464
> > +      - ti,tmp468
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +patternProperties:
> > +  "^channel@([0-8])$":
> > +    type: object
> > +    description: |
> > +      Represents channels of the device and their specific configuration.
> > +
> > +    properties:
> > +      reg:
> > +        description: |
> > +          The channel number. 0 is local channel, 1-8 are remote channels.
> > +        items:
> > +          minimum: 0
> > +          maximum: 8
> > +
> > +      label:
> > +        description: |
> > +          A descriptive name for this channel, like "ambient" or "psu".
> > +
> > +      ti,n-factor:
> > +        description: |
> > +          The value (two's complement) to be programmed in the channel specific N correction register.
> > +          For remote channels only.
> > +        $ref: /schemas/types.yaml#/definitions/int8
> > +        items:
> > +          minimum: -128
> > +          maximum: 127
>
> I still thing we should have the same format here and in tmp421, for
> consistency. If use the same property name, "ti,n-factor" but on tmp421
> you have use 32bit value while here you have to use 8bit (which is weird
> in DT, BTW), it might be confusing.
> Back when we did this for TMP421, there was some discussion and we
> settled on this approach, why do it differently now?

I seem to recall from that discussion that there was supposedly no way to
express negative numbers in devicetree. Obviously that is incorrect.
In addition to that, I strongly suspect that the tmp421 code as written
does not work. Its value range is specified as 0..255, but it is read with
	err = of_property_read_s32(child, "ti,n-factor", &val);
and range checked with
	if (val > 127 || val < -128) {
                dev_err(dev, "n-factor for channel %d invalid (%d)\n",
                       i, val);
                return -EINVAL;
        }

That just looks wrong. Either the value range is 0..255 and checked
as 0 .. 255, or it is -128 .. 127 and must be both checked and specified
accordingly. This made me look into the code and I found how negative
numbers are supposed to be handled.

We can go either way, but whatever it is should be correct and be confirmed
to work. Rob, any thoughts ?

Guenter