Re: [PATCH v3 4/4] pmbus_core: export coefficients via sysfs - Adamski, Krzysztof (Nokia

From: "Adamski, Krzysztof (Nokia - PL/Wroclaw)"  <krzysztof.adamski@nokia.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>,
	"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
	"Sverdlin,
	Alexander (Nokia - DE/Ulm)" <alexander.sverdlin@nokia.com>
Subject: Re: [PATCH v3 4/4] pmbus_core: export coefficients via sysfs
Date: Mon, 15 Apr 2019 20:26:21 +0000	[thread overview]
Message-ID: <20190415202606.GA990@localhost.localdomain> (raw)
In-Reply-To: <20190415163310.GA7933@roeck-us.net>

On Mon, Apr 15, 2019 at 09:33:10AM -0700, Guenter Roeck wrote:
>We are talking an ABI here. ABIs are supposed to be hardware independent.
>Directly mapping registers to attributes is not exactly hardware independent.

That is true, I get that, even though I don't want to map registers. I
want to map coefficients which are hardware independent but only as far
as we take into account only PMBUS. And I guess this is the root of the
difference in our opinions - I still only think about PMBUS devices
while you think about all HWMON devices (I think).

>> > and even misleading since chip programming isn't adjusted even when
>> > that is possible.
>>
>> I don't understand what you mean by that, could you explain?
>>
>"when that is possible" -> on chips supporting calibration commands,
>one should not manipulate b/m/R but write any adjustments into the chip
>using those commands.
>
>> >
>> >Addressing this problem in a generic way will require substantially
>> >more thought than just adding a couple of pmbus direct mode specific
>> >attributes.
>>
>> I'm willing to put more effort into that but I would need to better
>> understand what is your view of the scope of problem here.
>>
>There are a number of elements.
>
>First, a case has to be made why the current mechanism of using
>sensors3.conf for adjustments is insufficient. This may be well
>understood by some, but the case needs to be made (ie some chips do
>have HW registers to perform the adjustment, and in some cases there
>is accuracy loss by performing adjustments in user space).

My argument why you the solution of sensor3.conf can't be directly used
is that the readings for PMBUS devices uses real values, that are
already processed by some coefficient values. If you want to apply
corrected coefficients, you would first need to know what default coeffs
were used to "revese" the calculation. And the specification for devices
I am working with (ADM1275 and LM25066 families) describe how to get
such corrected coeffs so it is sensible to assume they might be
available.

>Second, we'll need to determine which standardized attributes are
>needed. As far as I can see this would probably be 'scale' (or maybe
>'gain') and 'offset'. The question is how to express especially 'scale'.
>It would either require both a multiplier and a divider, or it would
>have to be expressed in fixed point arithmetic. I would prefer the
>latter, but I am open to suggestions.
>
>Third, we will have to determine how to apply this all to pmbus
>chips. It must not only apply to direct mode chips, but to all
>PMBus chips. Even for direct mode chips the case needs to be made
>if the attributes should apply per sensor class or per sensor (I
>would think per sensor). Just looking at VOUT, chapter 9.2 of the
>PMBus specification (Rev 1.1, part II) gives an example of what
>to look out for: Just for VOUT, there are VOUT_TRIM, VOUT_CAL_OFFSET,
>VOUT_DROOP (does this require yet another attribute ?), and
>VOUT_SCALE_LOOP.
>
>I should add that the above, for VOUT, are the primary means for
>PMBus devices to adjust voltage readings. This also applies to IOUT,
>which has similar standard PMBus registers available for calibration
>(IOUT_CAL_GAIN and IOUT_CAL_OFFSET). On top of that, some chips support
>vendor specific commands to calibrate other sensors. For example,
>MAX15301 supports a EXT_TEMP_CAL command to calibrate temperature sensor
>readings.
>
>Also note that direct mode PMBUs chips _should_ support a COEFFICIENTS
>command to retrieve actual coefficients from the chip (though I don't
>recall ever encountering a chip that actually supports it).
>
>Overall, adjusting readings through manipulation of b/m/R is at best
>a workaround.
>
>As such, the chip we are talking about here does pretty much
>everything wrong. I do understand the need for a more dynamic
>calibration support on the driver level, but the solution must
>not focus on such a chip and needs to be more generic.

I need some more time to think about what you wrote here a little more
to get the feeling about the most general solution. One thing that came
into my mind, however, is that part of my problem would be solved if
userspace could at least retrieve coeffs used to calcualte the values.
Lets say the chip would support COEFFICIENTS command - how would we
implement support for that?

In the internal implementation, since all PMBUS devices should support
COEFFICIENTS command, we could just add attribute for the
PMBUS_COEFFICIENTS. Devices supporting this command, could just, well,
pass this to device. Other could "intercept it" and just return values
stored in the pmbus_driver_info structure.

The only problem is - this command is _very_ generic - it lets you ask
for separate coefficients for reading and writing and it can return
different one for each of the supported PMBUS commands. This makes it
impractical to probe for all supported combinations on probe. Some
filtering would have to be provided by the client drivers instead. But
still a long list of possible attributes would be created.

The end result - ABI would look similara as in my patch and would,
again, be pmbus specific and not really expandable to other hwmon
devices. But I don't see any other way we could support such a command.
But maybe this question - "how, from ABI standpoint, could we implement
support for PMBUS_COEFFICIENTS command?" is easier to answer than the
more general problem you described?

Krzysztof