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: <238e6fb0-cd77-4772-3e92-23941dc74403@roeck-us.net>
Date:   Mon, 21 Feb 2022 14:11:17 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v5 1/2] dt-bindings: hwmon: add tmp464.yaml
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>
 <66e6b131-274f-454b-44f6-17df879d71a9@roeck-us.net>
 <YhQDGHD3S0qwP2OB@localhost.localdomain>
From:   Guenter Roeck <linux@roeck-us.net>
In-Reply-To: <YhQDGHD3S0qwP2OB@localhost.localdomain>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 2/21/22 13:24, Krzysztof Adamski wrote:
> Dnia Mon, Feb 21, 2022 at 08:16:15AM -0800, Guenter Roeck napisał(a):
>>> 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.
> 
> Well, I would still argue it *is* correct. DT only support unsigned
> numbers and, really, only 32 or 64 bit. See the chapter 2.2.4 Properties
> in:
> https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4-rc1/devicetree-specification-v0.4-rc1.pdf
> 
> Devicetree also supports array of bytes, and this is how we get the
> /bits/ magic but this is just a syntactic suggar. The same is true about
> negative values. Just decompile your compiled DTB and you will see.
> To put it in other words - DTS does support negative values, DTB don't.j
> 
>> 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.
> 
> It worked for me when I tested that. I could redo the test, if you are
> unsure. The code also looks good to me. I wasn't convinced for this
> format in yaml but after the whole discussion we had, we settled on
> that, with Robs blessing :)
> 

It is hard for me to believe that you can enter, say, 255 into the dts file
and of_property_read_s32() reads it as -1. If so, of_property_read_s32()
could never support values of 128 and above, which seems unlikely.

Now, I can imagine that one can enter 0xffffffff and of_property_read_s32()
returns -1, but that isn't what is documented for tmp421.

Guenter

> Here's the actual discussion where all this was considered:
> https://patchwork.kernel.org/project/linux-hwmon/patch/3ff7b4cc57dab2073fa091072366c1e524631729.1632984254.git.krzysztof.adamski@nokia.com/
> 
> I'm not saying the way we do this for tmp421 is better or even right,
> all I say is that it would make sense to be consistent and not redo this
> discussion every time we have this problem.
> 
> Krzysztof