[v3,4/4] pmbus_core: export coefficients via sysfs
diff mbox series

Message ID 4887ef85022d7ceb26905997c19a874d7d89087f.1555273192.git.krzysztof.adamski@nokia.com
State Under Review
Headers show
Series
  • pmbus: extend configurability via sysfs
Related show

Commit Message

Adamski, Krzysztof (Nokia - PL/Wroclaw) April 14, 2019, 9:59 p.m. UTC
PMBUS devices report values in real-world units. Those using direct
format require conversion using standarised coefficients and formula.
This operation is already done by pmbus_core and the default values for
coefficients are configured by chip drivers.

However those default values are not allways suitable as they depend on
the value of sense register and environment used. For that reason, in
order to get values that make sense or just to get best accuracy, in it
may be required to tweak coefficient values. The proper values may be
measured during device production (to get best accuracy, they might be
callibrated for each individual unit) 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.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 104 ++++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 2 deletions(-)

Comments

Adamski, Krzysztof (Nokia - PL/Wroclaw) April 14, 2019, 10:37 p.m. UTC | #1
On Sun, Apr 14, 2019 at 11:59:38PM +0200, Krzysztof Adamski wrote:
>PMBUS devices report values in real-world units. Those using direct
>format require conversion using standarised coefficients and formula.
>This operation is already done by pmbus_core and the default values for
>coefficients are configured by chip drivers.
>
>However those default values are not allways suitable as they depend on
>the value of sense register and environment used. For that reason, in
>order to get values that make sense or just to get best accuracy, in it
>may be required to tweak coefficient values. The proper values may be
>measured during device production (to get best accuracy, they might be
>callibrated for each individual unit) 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.
>
>Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>---
>drivers/hwmon/pmbus/pmbus_core.c | 104 ++++++++++++++++++++++++++++++-
>1 file changed, 102 insertions(+), 2 deletions(-)
>

[...]

>+	pmbus_dev_attr_init(&ext_attr->attr, name, (S_IWUSR | S_IRUGO),

I screwed up and did not fix this also, sorry for that. I'm not sending
an updated version (yet) as I didn't get your approval for the concept
itself (though I still hope you will reconsider taking into account my
latest arguments).

[...]
>

Krzysztof
Guenter Roeck April 15, 2019, 2:38 a.m. UTC | #2
On 4/14/19 3:37 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> On Sun, Apr 14, 2019 at 11:59:38PM +0200, Krzysztof Adamski wrote:
>> PMBUS devices report values in real-world units. Those using direct
>> format require conversion using standarised coefficients and formula.
>> This operation is already done by pmbus_core and the default values for
>> coefficients are configured by chip drivers.
>>
>> However those default values are not allways suitable as they depend on
>> the value of sense register and environment used. For that reason, in
>> order to get values that make sense or just to get best accuracy, in it
>> may be required to tweak coefficient values. The proper values may be
>> measured during device production (to get best accuracy, they might be
>> callibrated for each individual unit) 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.
>>
>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>> ---
>> drivers/hwmon/pmbus/pmbus_core.c | 104 ++++++++++++++++++++++++++++++-
>> 1 file changed, 102 insertions(+), 2 deletions(-)
>>
> 
> [...]
> 
>> +	pmbus_dev_attr_init(&ext_attr->attr, name, (S_IWUSR | S_IRUGO),
> 
> I screwed up and did not fix this also, sorry for that. I'm not sending
> an updated version (yet) as I didn't get your approval for the concept
> itself (though I still hope you will reconsider taking into account my
> latest arguments).
> 

I won't, sorry. As mentioned before, this is a generic problem that needs
to be solved for all hwmon drivers, not just for pmbus devices, and also
not just for pmbus devices using direct mode.

Even worse, this doesn't even work for pmbus devices in general.
PMBus supports commands such as VOUT_SCALE_LOOP and various other
calibration commands such as VOUT_CAL_OFFSET, IOUT_CAL_GAIN and
IOUT_CAL_OFFSET. Your approach doesn't even try to support those
commands; you only look at r/m/b which may suit your needs but are
hardly generic and even misleading since chip programming isn't
adjusted even when that is possible.

Addressing this problem in a generic way will require substantially
more thought than just adding a couple of pmbus direct mode specific
attributes.

Guenter
Adamski, Krzysztof (Nokia - PL/Wroclaw) April 15, 2019, 8:34 a.m. UTC | #3
On Sun, Apr 14, 2019 at 07:38:33PM -0700, Guenter Roeck wrote:
>On 4/14/19 3:37 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>>On Sun, Apr 14, 2019 at 11:59:38PM +0200, Krzysztof Adamski wrote:
>>>PMBUS devices report values in real-world units. Those using direct
>>>format require conversion using standarised coefficients and formula.
>>>This operation is already done by pmbus_core and the default values for
>>>coefficients are configured by chip drivers.
>>>
>>>However those default values are not allways suitable as they depend on
>>>the value of sense register and environment used. For that reason, in
>>>order to get values that make sense or just to get best accuracy, in it
>>>may be required to tweak coefficient values. The proper values may be
>>>measured during device production (to get best accuracy, they might be
>>>callibrated for each individual unit) 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.
>>>
>>>Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>>>---
>>>drivers/hwmon/pmbus/pmbus_core.c | 104 ++++++++++++++++++++++++++++++-
>>>1 file changed, 102 insertions(+), 2 deletions(-)
>>>
>>
>>[...]
>>
>>>+	pmbus_dev_attr_init(&ext_attr->attr, name, (S_IWUSR | S_IRUGO),
>>
>>I screwed up and did not fix this also, sorry for that. I'm not sending
>>an updated version (yet) as I didn't get your approval for the concept
>>itself (though I still hope you will reconsider taking into account my
>>latest arguments).
>>
>
>I won't, sorry. As mentioned before, this is a generic problem that needs
>to be solved for all hwmon drivers, not just for pmbus devices, and also
>not just for pmbus devices using direct mode.
>
>Even worse, this doesn't even work for pmbus devices in general.
>PMBus supports commands such as VOUT_SCALE_LOOP and various other
>calibration commands such as VOUT_CAL_OFFSET, IOUT_CAL_GAIN and
>IOUT_CAL_OFFSET. Your approach doesn't even try to support those
>commands; you only look at r/m/b which may suit your needs but are
>hardly generic

I agree I don't see this problem as general as you. The mentioned
registers can easily be supported if needed by just attaching attributes
to them. That would allow doing things that cannot be done currently
(configuring precision of the regulation of the devices) while I see
coefficients problems as something totally different - more like a fix
to the already supported functionality (reporting current measurements).

> 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?

>
>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.

Krzysztof
Guenter Roeck April 15, 2019, 4:33 p.m. UTC | #4
On Mon, Apr 15, 2019 at 08:34:48AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> On Sun, Apr 14, 2019 at 07:38:33PM -0700, Guenter Roeck wrote:
> >On 4/14/19 3:37 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> >>On Sun, Apr 14, 2019 at 11:59:38PM +0200, Krzysztof Adamski wrote:
> >>>PMBUS devices report values in real-world units. Those using direct
> >>>format require conversion using standarised coefficients and formula.
> >>>This operation is already done by pmbus_core and the default values for
> >>>coefficients are configured by chip drivers.
> >>>
> >>>However those default values are not allways suitable as they depend on
> >>>the value of sense register and environment used. For that reason, in
> >>>order to get values that make sense or just to get best accuracy, in it
> >>>may be required to tweak coefficient values. The proper values may be
> >>>measured during device production (to get best accuracy, they might be
> >>>callibrated for each individual unit) 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.
> >>>
> >>>Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> >>>---
> >>>drivers/hwmon/pmbus/pmbus_core.c | 104 ++++++++++++++++++++++++++++++-
> >>>1 file changed, 102 insertions(+), 2 deletions(-)
> >>>
> >>
> >>[...]
> >>
> >>>+	pmbus_dev_attr_init(&ext_attr->attr, name, (S_IWUSR | S_IRUGO),
> >>
> >>I screwed up and did not fix this also, sorry for that. I'm not sending
> >>an updated version (yet) as I didn't get your approval for the concept
> >>itself (though I still hope you will reconsider taking into account my
> >>latest arguments).
> >>
> >
> >I won't, sorry. As mentioned before, this is a generic problem that needs
> >to be solved for all hwmon drivers, not just for pmbus devices, and also
> >not just for pmbus devices using direct mode.
> >
> >Even worse, this doesn't even work for pmbus devices in general.
> >PMBus supports commands such as VOUT_SCALE_LOOP and various other
> >calibration commands such as VOUT_CAL_OFFSET, IOUT_CAL_GAIN and
> >IOUT_CAL_OFFSET. Your approach doesn't even try to support those
> >commands; you only look at r/m/b which may suit your needs but are
> >hardly generic
> 
> I agree I don't see this problem as general as you. The mentioned
> registers can easily be supported if needed by just attaching attributes
> to them. That would allow doing things that cannot be done currently
> (configuring precision of the regulation of the devices) while I see
> coefficients problems as something totally different - more like a fix
> to the already supported functionality (reporting current measurements).
> 

We are talking an ABI here. ABIs are supposed to be hardware independent.
Directly mapping registers to attributes is not exactly hardware independent.

> > 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).

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.

Thanks,
Guenter
Adamski, Krzysztof (Nokia - PL/Wroclaw) April 15, 2019, 8:26 p.m. UTC | #5
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

Patch
diff mbox series

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 9b7d493c98b9..38493617d6a7 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -57,6 +57,8 @@ 
 
 #define PMBUS_NAME_SIZE		24
 
+static const char * const coeff_name[] = {"m", "b", "R"};
+
 struct pmbus_sensor {
 	struct pmbus_sensor *next;
 	char name[PMBUS_NAME_SIZE];	/* sysfs sensor name */
@@ -98,12 +100,13 @@  struct pmbus_data {
 	int exponent[PMBUS_PAGES];
 				/* linear mode: exponent for output voltages */
 
-	const struct pmbus_driver_info *info;
+	struct pmbus_driver_info *info;
 
 	int max_attributes;
 	int num_attributes;
 	struct attribute_group group;
-	const struct attribute_group *groups[2];
+	const struct attribute_group *groups[3];
+	struct attribute_group group_coeff;
 	struct dentry *debugfs;		/* debugfs device directory */
 
 	struct pmbus_sensor *sensors;
@@ -2007,6 +2010,95 @@  static int pmbus_add_samples_attributes(struct i2c_client *client,
 	return 0;
 }
 
+static struct attribute *pmbus_init_coeff_attr(struct pmbus_data *data,
+					       const char *prefix,
+					       const char *coeff, int *target)
+{
+	struct dev_ext_attribute *ext_attr;
+	char *name;
+
+	ext_attr = devm_kzalloc(data->dev, sizeof(*ext_attr), GFP_KERNEL);
+	if (!ext_attr)
+		return NULL;
+
+	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s_%s", prefix, coeff);
+	if (!name)
+		return NULL;
+
+	pmbus_dev_attr_init(&ext_attr->attr, name, (S_IWUSR | S_IRUGO),
+			    device_show_int, device_store_int);
+	ext_attr->var = target;
+
+	return &ext_attr->attr.attr;
+}
+
+static int pmbus_add_coeff_attributes_class(struct pmbus_data *data,
+					    const char *prefix,
+					    enum pmbus_sensor_classes class,
+					    struct attribute **attrs)
+{
+	int *coeff_val[] = {data->info->m, data->info->b, data->info->R};
+	struct attribute *ret;
+	int i, count = 0;
+
+	for (i = 0; i < ARRAY_SIZE(coeff_name); i++) {
+		ret = pmbus_init_coeff_attr(data, prefix, coeff_name[i],
+					    coeff_val[i]);
+		if (!ret)
+			return -ENOMEM;
+		attrs[count++] = ret;
+	}
+
+	return count;
+}
+
+static const char * const classes_names[] = {
+	[PSC_VOLTAGE_IN] = "vin",
+	[PSC_VOLTAGE_OUT] = "vout",
+	[PSC_CURRENT_IN] = "iin",
+	[PSC_CURRENT_OUT] = "iout",
+	[PSC_POWER] = "power",
+	[PSC_TEMPERATURE] = "temp",
+};
+
+static int pmbus_add_coeff_attributes(struct i2c_client *client,
+				      struct pmbus_data *data)
+{
+	struct attribute **attrs;
+	int i, n = 0, ret = 0;
+
+	attrs = kcalloc(ARRAY_SIZE(coeff_name) * ARRAY_SIZE(classes_names) + 1,
+			sizeof(void *), GFP_KERNEL);
+	if (!attrs)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(classes_names); i++) {
+		if (classes_names[i] == NULL)
+			continue;
+
+		if (data->info->format[i] != direct)
+			continue;
+
+		ret = pmbus_add_coeff_attributes_class(data, classes_names[i],
+						       i, &attrs[n]);
+		if (ret < 0) {
+			kfree(attrs);
+			return ret;
+		}
+
+		n += ret;
+	}
+
+	if (n) {
+		data->group_coeff.name = "coefficients";
+		data->group_coeff.attrs = attrs;
+	} else {
+		kfree(attrs);
+	}
+
+	return 0;
+}
+
 static int pmbus_find_attributes(struct i2c_client *client,
 				 struct pmbus_data *data)
 {
@@ -2042,6 +2134,10 @@  static int pmbus_find_attributes(struct i2c_client *client,
 		return ret;
 
 	ret = pmbus_add_samples_attributes(client, data);
+	if (ret)
+		return ret;
+
+	ret = pmbus_add_coeff_attributes(client, data);
 	return ret;
 }
 
@@ -2456,6 +2552,8 @@  int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 	}
 
 	data->groups[0] = &data->group;
+	if (data->group_coeff.attrs)
+		data->groups[1] = &data->group_coeff;
 	data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
 							    data, data->groups);
 	if (IS_ERR(data->hwmon_dev)) {
@@ -2478,6 +2576,7 @@  int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 	hwmon_device_unregister(data->hwmon_dev);
 out_kfree:
 	kfree(data->group.attrs);
+	kfree(data->group_coeff.attrs);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(pmbus_do_probe);
@@ -2490,6 +2589,7 @@  int pmbus_do_remove(struct i2c_client *client)
 
 	hwmon_device_unregister(data->hwmon_dev);
 	kfree(data->group.attrs);
+	kfree(data->group_coeff.attrs);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pmbus_do_remove);