On Thu, Apr 11, 2019 at 06:39:11AM -0700, Guenter Roeck wrote:
>On 4/11/19 12:45 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>>On Wed, Apr 10, 2019 at 05:30:08PM -0700, Guenter Roeck wrote:
>>>On 4/10/19 3:39 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>>>>In order to get best accuracy, in case of some devices it is required to
>>>>tweak coefficient values. This is especially true for devices using
>>>>some external shunt resistor or being operated in non-usual environment.
>>>>Those values may be measured during device production and stored as
>>>>calibration values in some place (like EEPROM or even a file).
>>>>
>>>>To support wide range of possible use cases, lets export those to
>>>>user space via sysfs attributes so that it can implement its own
>>>>policies about them. All those attributes are put into separate
>>>>attribute_group struct so that we can set its name to group all of them
>>>>in a "coefficients" subdirectory in sysfs.
>>>>
>>>
>>>Coefficients are hardcoded into the chip, and the hwmon ABI reports raw
>>>values. Any correction should be done using sensors3.conf.
>>
>>I'm not sure what you mean by the fact they are hardcoded into chip. I
>>am targeting a case where direct values are being converted to real
>>world values using coefficients by pmbus_reg2data_direct() function. My
>>understanding is that the reason why the devices does not report values
>>in real world units but requires using coefficients for calculation is to
>>ease the calibration. For example the LM5064[1] has a chapter called
>>"determining telemetry coefficients empirically with linear fit" which
>>describes how to calculate them, based on the sense resistor used. Some
>>drivers, like adm1275.c, have a custom way to indirectly influence the
>>coefficients values by using Devicetree ("shunt-resistor-micro-ohms")
>>but this is not really flexible nor general approach. In case of
>>adm1275, only "m" coefficient is adjusted this way. Depending on "m"
>>value, "R" might require adjustments as well and we also need "b" to
>>achieve best accuracy. Then, again, using Device Tree is not suitable
>>for per device calibration.
>>
>'
>Why not ? You'll have to explain that one. It is not as if coefficients
>would change on the fly. They are chip and/or board properties.
But the coefficients will depend on the exact value of the shunt
resistor. To get bet accuracy, the coefficients are calculated for each
board unit. While all units of the same type share the devicetree
(otherwise this devicetree would have to be generated by some firmware
software first).
>
>>My argument here is that the kernel does not return raw value in this
>>case - it returns (supposedly) real-world values that are calculated
>>internally based of coefficients according to the formula specified by
>>pmbus specification. In my opinion it would make sense to provide it
>>with proper coeffs if defaults are not suitable. Otherwise reporting
>>"real-values" doesn't make much sense. In other words, our
>>implementation would currently report real-world values if your case
>>happens to match default coeffs for some shunt resistor and
>>environment specified in datasheet of the device.
>>
>
>This is why I dislike making exceptions. It always creates a bad precedence.
>hwmon devices are supposed to report raw values, to be converted to real
>values using the configuration file.
>
>
>We can discuss scaling and possible ABI enhancements, but it will have
>to be generic. b/m/r is not generic. You also don't make the case why
>those values would have to be runtime-adjustable.
>
b/m/r are coefficients mentioned by PMBUS specification and the change
is PMBUS specific. My understanding is that all PMBUS devices report
values in real-world units (either in linear or direct mode) and this is
implemented this way in the code already.
In fact, if the device would report raw values, we wouldn't need this
patch as we would simply apply those coefficients in userspace. But if
drivers report real values which require correct coefficients to work, I
would expect to be able to tweak them.
>
>Note that there are several issues with the patch itself: There is no
>validation, the names are questionable ("p" ?), the scope is questionable
>(if an adjustment is needed, it would likely have to be per sensor, not
>per group), and the attributes are created unconditionally even if the
>chip does not report values in direct mode, just to name a few.
I can fix all that, of course, but we have to discuss the the mentioned
issues first. I agree with all those arguments except the scope one. As
we currently only support specifying coefficients per class and this is
exacly how I export them. Unless I misunderstood your comment, of
course.
>However, that is all irrelevant: At this point I'll resist further chip
>specific changes in that area. If we move from reporting raw values
>towards reporting adjusted values, the solution will have to be generic
>and must not be chip specific.
But this is not chip specific change, it is more like bus (PMBUS)
specific. All pmbus devices reporting in direct mode have those
coefficients, all of them use the same formula to calculate real values
out of raw values as this is specified in the PMBUS specification.
I agree this might not be useful for devices reporting in linear/vid
mode so I could exclude them when exporting coefficients.
Krzysztof
