RE: [PATCH v2 1/2] dt-bindings: hwmon: Add lltc ltc4286 driver bindings

Delphine_CC_Chiu/WYHQ/Wiwynn
2023-10-31T06:25:12Z

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Friday, October 27, 2023 12:49 AM
> To: Conor Dooley <conor@kernel.org>
> Cc: Delphine_CC_Chiu/WYHQ/Wiwynn <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
> 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@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:
> >>>> +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@Wiwynn.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