Re: [PATCH 0/3] hwmon: (pmbus) add power from energy readings

[Kallas, Pawel]

On 06-Jul-22 3:17 PM, Guenter Roeck wrote:
> On Wed, Jul 06, 2022 at 12:40:21PM +0200, Kallas, Pawel wrote:
> > Add support for reading EIN or EOUT registers and expose power calculated
> > from energy. This is more accurate than PIN and POUT power readings.
> > Readings are exposed in new hwmon files power1_average and power2_average.
> > Also add support for QUERY command that is needed to check availability
> > of EIN and EOUT reads and its data format. Only direct data format is
> > supported due to lack of test devices supporting other formats.
> I don't think this is a good idea. EIN/EOUT report energy consumption,
> not power.

According to PMBus-Specification-Rev-1-3-1-Part-II-20150313 "READ_EIN and
READ_EOUT commands provide information that can be used to calculate power
consumption". That is accumulator summing instantaneous input power
expressed in "watt-samples" and counter indicating number of samples.
The only reasonable thing that can be done with those values is 
calculating power.

> The "average" attributes as implemented don't really report
> a reliable number since the averaging period is not defined.

Agree, it is calculating average power since last read, which could be
incorrect with multiple consumers. However, this is the only possibility
without adding some timer logic.

> Also, kernel
> drivers should not make up such numbers. I don't mind adding energy
> attribute support, but that should be reported as what it is, energy.
> What userspace does with it would then be a userspace concern; it can
> calculate all kinds of averages from it as much as it wants.

Returning direct value of read registers would also work for our use case,
but it is not in line with sysfs interface.

> Also, new attributes should not depend on query command support.
> I don't mind adding support for that, but it would have to be independent
> of energy attribute support.
>
> Thanks,
> Guenter
>
> > Kallas, Pawel (3):
> >    hwmon: (pmbus) add support for QUERY command
> >    hwmon: (pmbus) refactor sensor initialization
> >    hwmon: (pmbus) add EIN and EOUT readings
> >
> >   Documentation/hwmon/pmbus-core.rst |   7 +
> >   drivers/hwmon/pmbus/pmbus.c        |  20 +++
> >   drivers/hwmon/pmbus/pmbus.h        |  19 +++
> >   drivers/hwmon/pmbus/pmbus_core.c   | 261 +++++++++++++++++++++++++++--
> >   4 files changed, 291 insertions(+), 16 deletions(-)
> >
> >
> > base-commit: 7c1de25c06f31b04744beae891baf147af9ba0cb