> -----Original Message----- > From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck > Sent: Friday, October 27, 2023 12:49 AM > To: Conor Dooley <conor@xxxxxxxxxx> > Cc: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@xxxxxxxxxx>; > patrick@xxxxxxxxx; Jean Delvare <jdelvare@xxxxxxxx>; Rob Herring > <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; > Jonathan Corbet <corbet@xxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx; > linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx > 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@xxxxxxxxxx> > >>>> > >>>> 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@xxxxxxxxxx> > >>>> + > >>>> +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
