| Message ID | 20231026081514.3610343-1-Delphine_CC_Chiu@Wiwynn.com |
|---|---|
| Headers | show |
| Series | LTC4286 and LTC4287 driver support | expand |
> -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Thursday, October 26, 2023 11:10 PM > To: Conor Dooley <conor@kernel.org>; Delphine_CC_Chiu/WYHQ/Wiwynn > <Delphine_CC_Chiu@wiwynn.com> > Cc: 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 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://dev/ > >> > +icetree.org%2Fschemas%2Fhwmon%2Flltc%2Cltc4286.yaml%23&data=05%7C > 01% > >> > +7CWayne_SC_Liu%40wiwynn.com%7Cb250e206c9ef48fbc1c108dbd6359e51 > %7Cda6 > >> > +e0628fc834caf9dd273061cbab167%7C0%7C0%7C638339298041650948%7CU > nknown > >> > +%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW > wiL > >> > +CJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=gGQEDl33zRIJbfbinu2%2Bi2cC > Ay6y0o > >> +DSLzBpLL7hA%2F8%3D&reserved=0 > >> +$schema: > >> +http://dev/ > >> > +icetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7CWayne_S > C_Li > >> > +u%40wiwynn.com%7Cb250e206c9ef48fbc1c108dbd6359e51%7Cda6e0628fc8 > 34caf > >> > +9dd273061cbab167%7C0%7C0%7C638339298041650948%7CUnknown%7CT > WFpbGZsb3 > >> > +d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 > %3D > >> > +%7C3000%7C%7C%7C&sdata=nl1IM1HpYptsJOHfiuXtKFmD%2FVlGMCW1IkoK > HYco0sk > >> +%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://lore/ > > .kernel.org%2Fall%2F22f6364c-611c-ffb6-451c-9ddc20418d0a%40roeck-us.n > e > > > t%2F&data=05%7C01%7CWayne_SC_Liu%40wiwynn.com%7Cb250e206c9ef48f > bc1c108 > > > dbd6359e51%7Cda6e0628fc834caf9dd273061cbab167%7C0%7C0%7C6383392 > 9804165 > > > 0948%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMz > IiLCJBT > > > iI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=OpwfD3rBS0vQBF > jUhszrMg > > 4mq581jU7gx54Ln8V3gUA%3D&reserved=0 > > > > At least the chip does officially exist now, and a datasheet is available. > > https://www.ana/ > log.com%2Fen%2Fproducts%2Fltc4287.html&data=05%7C01%7CWayne_SC_Li > u%40wiwynn.com%7Cb250e206c9ef48fbc1c108dbd6359e51%7Cda6e0628fc83 > 4caf9dd273061cbab167%7C0%7C0%7C638339298041650948%7CUnknown%7 > CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL > CJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=CwT16Uipl8RymnFZ1WuBJzUJv > fLCKdK3VlTcTBN0xxk%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). We will check the difference of coefficients for the telemetry commands with vendor. > > >> + > >> + 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. > > Guenter
> -----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
On 10/30/23 23:25, Delphine_CC_Chiu/WYHQ/Wiwynn wrote: [ ... ] >> >> 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? > See "PMBUS COMMAND SUMMARY", default values: "IOUT_OC_WARN_LIMIT" says "21.3 mV/RSENSE" "PIN_OP_WARN_LIMIT" says "2.8/RSENSE" This seems to contradict "Table 8. PMBus M, B, and R Parameters". But then, reading it again (and again), I think it is just an odd and confusing way of trying to describe the 0x7fff default register values. Sorry for the noise. Guenter