Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <c734e26a-6fad-bc23-ec58-10c6a440ec83@roeck-us.net>
Date:   Thu, 26 Oct 2023 09:48:33 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Content-Language: en-US
To:     Conor Dooley <conor@kernel.org>
Cc:     Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>, patrick@stwcx.xyz,
        Jean Delvare <jdelvare@suse.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>, linux-i2c@vger.kernel.org,
        linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org
References: <20231026081514.3610343-1-Delphine_CC_Chiu@Wiwynn.com>
 <20231026081514.3610343-2-Delphine_CC_Chiu@Wiwynn.com>
 <20231026-dicing-crispy-a10af575d3e5@spud>
 <fffa4330-8d01-8498-4c5f-772ebf2a6b5a@roeck-us.net>
 <20231026-poison-encrypt-1df55e023867@spud>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver
 bindings
In-Reply-To: <20231026-poison-encrypt-1df55e023867@spud>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/26/23 08:26, Conor Dooley wrote:
> On Thu, Oct 26, 2023 at 08:09:52AM -0700, Guenter Roeck wrote:
>> On 10/26/23 07:25, Conor Dooley wrote:
>>> Hey,
>>>
>>> On Thu, Oct 26, 2023 at 04:15:11PM +0800, Delphine CC Chiu wrote:
>>>> Add a device tree bindings for ltc4286 driver.
>>>
>>> Bindings are for devices, not for drivers.
>>>
>>>>
>>>> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
>>>>
>>>> Changelog:
>>>>     v2 - Revise vrange_select_25p6 to adi,vrange-select-25p6
>>>>        - Add type for adi,vrange-select-25p6
>>>>        - Revise rsense-micro-ohms to shunt-resistor-micro-ohms
>>>> ---
>>>>    .../bindings/hwmon/lltc,ltc4286.yaml          | 50 +++++++++++++++++++
>>>>    MAINTAINERS                                   | 10 ++++
>>>>    2 files changed, 60 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>>>> new file mode 100644
>>>> index 000000000000..17022de657bb
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/hwmon/lltc,ltc4286.yaml
>>>> @@ -0,0 +1,50 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/hwmon/lltc,ltc4286.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: LTC4286 power monitors
>>>> +
>>>> +maintainers:
>>>> +  - Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - lltc,ltc4286
>>>> +      - lltc,ltc4287
>>>
>>> I don't recall seeing an answer to Guenter about this ltc4287 device:
>>> https://lore.kernel.org/all/22f6364c-611c-ffb6-451c-9ddc20418d0a@roeck-us.net/
>>>
>>
>> At least the chip does officially exist now, and a datasheet is available.
>>
>> https://www.analog.com/en/products/ltc4287.html
>>
>> It shows that coefficients for the telemetry commands are different,
>> meaning that indeed both chips need to be explicitly referenced
>> in the properties description (and handled in the driver, which proves
>> my point of needing a datasheet before accepting such a driver).
> 
> Oh neat, would've been good if that'd been mentioned!
> 

Actually, turns out there is some contradiction in the LTC4286 datasheet.
It mentions different coefficients in different places. It is all but
impossible to determine if the datasheet is wrong or if the chip uses
a variety of coefficients unless one has a real chip available. Sigh :-(.

>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  adi,vrange-select-25p6:
>>>> +    description:
>>>> +      This property is a bool parameter to represent the
>>>> +      voltage range is 25.6 or not for this chip.
>>>
>>> 25.6 what? Volts? microvolts?
>>> What about Guenter's suggestion to name this so that it better matches
>>> the other, similar properties?
>>>
>>
>> I still would prefer one of the more common properties.
>> I still prefer adi,vrange-high-enable.
> 
> I think, from reading the previous version, that this is actually the
> lower voltage range, as there is a 102.x $unit range too that is the
> default in the hardware. Obviously, the use of the property could be
> inverted to match that naming however.
> 

It needs to be programmed either way because it is unknown how the chip
has been programmed before. That means some action is needed no matter
if the property is provided or not.

Sure, we could add something like adi,vrange-low-enable. Not sure if
that would be any better.

Actually, after looking at the datasheets, I'd be more interested
in the impact of other configuration buts such as VPWR_SELECT
which selects the voltage used for power calculations, or
all the settings in MFR_ADC_CONFIG. The datasheet doesn't really
explain (or I can't figure out how it does) how the different
configurations affect the reported telemetry values. But that is
a question for the driver, not for the property description.

Guenter