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.
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.
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.
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.
Thanks,
Guenter
