Message-ID: <SG2PR04MB55438AA9A2989F406012A4D4A1A0A@SG2PR04MB5543.apcprd04.prod.outlook.com>
Date:   Tue, 31 Oct 2023 06:25:00 +0000
From:   Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@...ynn.com>
To:     Guenter Roeck <linux@...ck-us.net>, Conor Dooley <conor@...nel.org>
CC:     Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@...ynn.com>,
        "patrick@...cx.xyz" <patrick@...cx.xyz>,
        Jean Delvare <jdelvare@...e.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: RE: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver
 bindings

> -----Original Message-----
> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
> Sent: Friday, October 27, 2023 12:49 AM
> To: Conor Dooley <conor@...nel.org>
> Cc: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@...ynn.com>;
> patrick@...cx.xyz; Jean Delvare <jdelvare@...e.com>; Rob Herring
> <robh+dt@...nel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; Conor Dooley <conor+dt@...nel.org>;
> Jonathan Corbet <corbet@....net>; linux-i2c@...r.kernel.org;
> linux-hwmon@...r.kernel.org; devicetree@...r.kernel.org;
> linux-kernel@...r.kernel.org; linux-doc@...r.kernel.org
> Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver
> bindings
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> 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@...ynn.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:
> >>>> +https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fd
> >>>>
> +evicetree.org%2Fschemas%2Fhwmon%2Flltc%2Cltc4286.yaml%23&data=05%
> 7
> >>>>
> +C01%7CWayne_SC_Liu%40wiwynn.com%7Cf555db9618454d17e0b508dbd64
> 36721
> >>>>
> +%7Cda6e0628fc834caf9dd273061cbab167%7C0%7C0%7C6383393572109674
> 95%7
> >>>>
> +CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJB
> TiI
> >>>>
> +6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=cPOOqJ5y3K%2B
> D%2Frsp
> >>>> +gVhpKDIC0gC5nKBbRp7Up0OxDpM%3D&reserved=0
> >>>> +$schema:
> >>>> +https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fd
> >>>>
> +evicetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7CWayne
> _S
> >>>>
> +C_Liu%40wiwynn.com%7Cf555db9618454d17e0b508dbd6436721%7Cda6e06
> 28fc
> >>>>
> +834caf9dd273061cbab167%7C0%7C0%7C638339357210967495%7CUnknown
> %7CTW
> >>>>
> +FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> >>>>
> +VCI6Mn0%3D%7C3000%7C%7C%7C&sdata=HjOhDm8bwPaNpWgumCSlak%2
> FiqoagwZc
> >>>> +0eb95J1wnKUE%3D&reserved=0
> >>>> +
> >>>> +title: LTC4286 power monitors
> >>>> +
> >>>> +maintainers:
> >>>> +  - Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>
> >>>> +
> >>>> +properties:
> >>>> +  compatible:
> >>>> +    enum:
> >>>> +      - lltc,ltc4286
> >>>> +      - lltc,ltc4287
> >>>
> >>> I don't recall seeing an answer to Guenter about this ltc4287 device:
> >>> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> >>>
> re.kernel.org%2Fall%2F22f6364c-611c-ffb6-451c-9ddc20418d0a%40roeck-u
> >>>
> s.net%2F&data=05%7C01%7CWayne_SC_Liu%40wiwynn.com%7Cf555db96184
> 54d17
> >>>
> e0b508dbd6436721%7Cda6e0628fc834caf9dd273061cbab167%7C0%7C0%7C6
> 38339
> >>>
> 357210967495%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQI
> joiV2l
> >>>
> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=oekIX
> %2FlP
> >>> %2B%2F4KhrSlK8Xp1zR0fdX1Emmr0Ox5GPS9Dz0%3D&reserved=0
> >>>
> >>
> >> At least the chip does officially exist now, and a datasheet is available.
> >>
> >> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> >> .analog.com%2Fen%2Fproducts%2Fltc4287.html&data=05%7C01%7CWayne
> _SC_Li
> >>
> u%40wiwynn.com%7Cf555db9618454d17e0b508dbd6436721%7Cda6e0628fc8
> 34caf9
> >>
> dd273061cbab167%7C0%7C0%7C638339357210967495%7CUnknown%7CTWF
> pbGZsb3d8
> >>
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C
> >>
> 3000%7C%7C%7C&sdata=FqRheGXFi%2FmSH3cnRZ7eSbwD3JfZj8powcGBCJcP
> wWQ%3D&
> >> reserved=0
> >>
> >> 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 :-(.
We are not the chip vendor, but we could forward your question to vendor.
Could you point out the exact places (which pages) where are the contradiction in LTC4286 datasheet?

> 
> >>>> +
> >>>> +  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.
We have sent an e-mail about this question.

> 
> Guenter

Powered by blists - more mailing lists