Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <66e6b131-274f-454b-44f6-17df879d71a9@roeck-us.net>
Date:   Mon, 21 Feb 2022 08:16:15 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Content-Language: en-US
To:     Krzysztof Adamski <krzysztof.adamski@nokia.com>
Cc:     linux-hwmon@vger.kernel.org, Agathe Porte <agathe.porte@nokia.com>,
        Jean Delvare <jdelvare@suse.com>,
        Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220218150908.1947772-1-linux@roeck-us.net>
 <YhNWVLHYVtCvdGhi@localhost.localdomain>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v5 1/2] dt-bindings: hwmon: add tmp464.yaml
In-Reply-To: <YhNWVLHYVtCvdGhi@localhost.localdomain>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@nokia.com>
>>
>> Add basic description of the tmp464 driver DT bindings.
>>
>> Signed-off-by: Agathe Porte <agathe.porte@nokia.com>
>> Cc: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> 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@nokia.com>
>> +
>> +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