[PATCH v3 0/4] pmbus: extend configurability via sysfs

[PATCH v3 0/4] pmbus: extend configurability via sysfs

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

Hi,

This patch series extends pmbus core for two specific use cases we have:
- First three patches allows lm25066 driver to set number of samples for
  average values (by controlling manufacturer specific SAMPLES_FOR_AVG
  register). It is useful to be able to set this register when using any
  of the *_average registers, especially since the default value means
  we are averaging 1 sample which isn't that useful.
- Third patch exports m, b, R coefficients so that they can be adjusted
  by user space. We can't use default coefficients values and in order
  to achieve high accuracy, we calibrate them per unit so using
  device-tree or similar concepts (which are generally shared by all
  board of the same type) to store them is not an option too. Also,
  using device-tree (the only was to influence coeffs now) might not be
  possible on some architectures (like on x86, for example). Thus, we
  export it so that the logic of loading proper coeffs can be
  implemented in user space instead.

v3 changes:
- split ABI docs with the actual implementation
- better error handling in lm25066 implementation
- numeric permissions
- only add samples attributes for page 0

v2 changes:
- PMBUS_VIRT_* registers used instead of custom sysfs groups for
  configuring samples for average
- coeffs are only exported as sysfs attirbutes if the format used is
  "direct"
- fixed memory allocation issue in coeffs patch

Krzysztof Adamski (4):
  pmbus: introduce PMBUS_VIRT_*_SAMPLES registers
  hwmon: Document the samples attributes
  lm25066: support SAMPLES_FOR_AVG register
  pmbus_core: export coefficients via sysfs

 Documentation/hwmon/sysfs-interface |  18 +++
 drivers/hwmon/pmbus/lm25066.c       |  17 ++-
 drivers/hwmon/pmbus/pmbus.h         |  15 ++
 drivers/hwmon/pmbus/pmbus_core.c    | 214 +++++++++++++++++++++++++++-
 4 files changed, 261 insertions(+), 3 deletions(-)

-- 
2.20.1

[PATCH v3 1/4] pmbus: introduce PMBUS_VIRT_*_SAMPLES registers

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

Those virtual registers can be used to export manufacturer specific
functionality for controlling the number of samples for average values
reported by the device.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/pmbus/pmbus.h      |  15 +++++
 drivers/hwmon/pmbus/pmbus_core.c | 110 +++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 1d24397d36ec..e73289cc867d 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -217,6 +217,20 @@ enum pmbus_regs {
 	PMBUS_VIRT_PWM_ENABLE_2,
 	PMBUS_VIRT_PWM_ENABLE_3,
 	PMBUS_VIRT_PWM_ENABLE_4,
+
+	/* Samples for average
+	 *
+	 * Drivers wanting to expose functionality for changing the number of
+	 * samples used for average values should implement support in
+	 * {read,write}_word_data callback for either PMBUS_VIRT_SAMPLES if it
+	 * applies to all types of measurements, or any number of specific
+	 * PMBUS_VIRT_*_SAMPLES registers to allow for individual control.
+	 */
+	PMBUS_VIRT_SAMPLES,
+	PMBUS_VIRT_IN_SAMPLES,
+	PMBUS_VIRT_CURR_SAMPLES,
+	PMBUS_VIRT_POWER_SAMPLES,
+	PMBUS_VIRT_TEMP_SAMPLES,
 };
 
 /*
@@ -371,6 +385,7 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_STATUS_VMON	BIT(19)
 #define PMBUS_HAVE_PWM12	BIT(20)
 #define PMBUS_HAVE_PWM34	BIT(21)
+#define PMBUS_HAVE_SAMPLES	BIT(22)
 
 #define PMBUS_PAGE_VIRTUAL	BIT(31)
 
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 2e2b5851139c..9b7d493c98b9 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -1901,6 +1901,112 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
 	return 0;
 }
 
+struct pmbus_samples_attr {
+	int reg;
+	char *name;
+};
+
+struct pmbus_samples_reg {
+	int page;
+	struct pmbus_samples_attr *attr;
+	struct device_attribute dev_attr;
+};
+
+static struct pmbus_samples_attr pmbus_samples_registers[] = {
+	{
+		.reg = PMBUS_VIRT_SAMPLES,
+		.name = "samples",
+	}, {
+		.reg = PMBUS_VIRT_IN_SAMPLES,
+		.name = "in_samples",
+	}, {
+		.reg = PMBUS_VIRT_CURR_SAMPLES,
+		.name = "curr_samples",
+	}, {
+		.reg = PMBUS_VIRT_POWER_SAMPLES,
+		.name = "power_samples",
+	}, {
+		.reg = PMBUS_VIRT_TEMP_SAMPLES,
+		.name = "temp_samples",
+	}
+};
+
+#define to_samples_reg(x) container_of(x, struct pmbus_samples_reg, dev_attr)
+
+static ssize_t pmbus_show_samples(struct device *dev,
+				  struct device_attribute *devattr, char *buf)
+{
+	int val;
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	struct pmbus_samples_reg *reg = to_samples_reg(devattr);
+
+	val = _pmbus_read_word_data(client, reg->page, reg->attr->reg);
+	if (val < 0)
+		return val;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static ssize_t pmbus_set_samples(struct device *dev,
+				 struct device_attribute *devattr,
+				 const char *buf, size_t count)
+{
+	int ret;
+	long val;
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	struct pmbus_samples_reg *reg = to_samples_reg(devattr);
+
+	if (kstrtol(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	ret = _pmbus_write_word_data(client, reg->page, reg->attr->reg, val);
+
+	return ret ? : count;
+}
+
+static int pmbus_add_samples_attr(struct pmbus_data *data, int page,
+				  struct pmbus_samples_attr *attr)
+{
+	struct pmbus_samples_reg *reg;
+
+	reg = devm_kzalloc(data->dev, sizeof(*reg), GFP_KERNEL);
+	if (!reg)
+		return -ENOMEM;
+
+	reg->attr = attr;
+	reg->page = page;
+
+	pmbus_dev_attr_init(&reg->dev_attr, attr->name, 0644,
+			    pmbus_show_samples, pmbus_set_samples);
+
+	return pmbus_add_attribute(data, &reg->dev_attr.attr);
+}
+
+static int pmbus_add_samples_attributes(struct i2c_client *client,
+					struct pmbus_data *data)
+{
+	const struct pmbus_driver_info *info = data->info;
+	int s;
+
+	if (!(info->func[0] & PMBUS_HAVE_SAMPLES))
+		return 0;
+
+	for (s = 0; s < ARRAY_SIZE(pmbus_samples_registers); s++) {
+		struct pmbus_samples_attr *attr;
+		int ret;
+
+		attr = &pmbus_samples_registers[s];
+		if (!pmbus_check_word_register(client, 0, attr->reg))
+			continue;
+
+		ret = pmbus_add_samples_attr(data, 0, attr);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int pmbus_find_attributes(struct i2c_client *client,
 				 struct pmbus_data *data)
 {
@@ -1932,6 +2038,10 @@ static int pmbus_find_attributes(struct i2c_client *client,
 
 	/* Fans */
 	ret = pmbus_add_fan_attributes(client, data);
+	if (ret)
+		return ret;
+
+	ret = pmbus_add_samples_attributes(client, data);
 	return ret;
 }
 
-- 
2.20.1

[PATCH v3 2/4] hwmon: Document the samples attributes

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

Document new ABI attributes: {in,power,curr,temp}_samples and samples.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 Documentation/hwmon/sysfs-interface | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
index 2b9e1005d88b..7b91706d01c8 100644
--- a/Documentation/hwmon/sysfs-interface
+++ b/Documentation/hwmon/sysfs-interface
@@ -756,6 +756,24 @@ intrusion[0-*]_beep
 		1: enable
 		RW
 
+********************************
+* Average sample configuration *
+********************************
+
+Devices allowing for reading {in,power,curr,temp}_average values may export
+attributes for controlling number of samples used to compute average.
+
+samples		Sets number of average samples for all types of measurements.
+		RW
+
+in_samples
+power_samples
+curr_samples
+temp_samples    Sets number of average samples for specific type of measurements.
+		Note that on some devices it won't be possible to set all of them
+		to different values so changing one might also change some others.
+		RW
+
 
 sysfs attribute writes interpretation
 -------------------------------------
-- 
2.20.1

[PATCH v3 3/4] lm25066: support SAMPLES_FOR_AVG register

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

Manufacturer specific SAMPLES_FOR_AVG register allows setting the number
of samples used in computing the average values (PMBUS_VIRT_READ_*_AVG).
The number we write is an exponent of base 2 of the number of samples so
for example writing 3 will result in 8 samples average.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/hwmon/pmbus/lm25066.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
index 53db78753a0d..715d4ab57516 100644
--- a/drivers/hwmon/pmbus/lm25066.c
+++ b/drivers/hwmon/pmbus/lm25066.c
@@ -26,6 +26,7 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
+#include <linux/log2.h>
 #include "pmbus.h"
 
 enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
@@ -39,12 +40,15 @@ enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
 #define LM25066_CLEAR_PIN_PEAK		0xd6
 #define LM25066_DEVICE_SETUP		0xd9
 #define LM25066_READ_AVG_VIN		0xdc
+#define LM25066_SAMPLES_FOR_AVG		0xdb
 #define LM25066_READ_AVG_VOUT		0xdd
 #define LM25066_READ_AVG_IIN		0xde
 #define LM25066_READ_AVG_PIN		0xdf
 
 #define LM25066_DEV_SETUP_CL		BIT(4)	/* Current limit */
 
+#define LM25066_SAMPLES_FOR_AVG_MAX	4096
+
 /* LM25056 only */
 
 #define LM25056_VAUX_OV_WARN_LIMIT	0xe3
@@ -284,6 +288,12 @@ static int lm25066_read_word_data(struct i2c_client *client, int page, int reg)
 	case PMBUS_VIRT_RESET_PIN_HISTORY:
 		ret = 0;
 		break;
+	case PMBUS_VIRT_SAMPLES:
+		ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG);
+		if (ret < 0)
+			break;
+		ret = 1 << ret;
+		break;
 	default:
 		ret = -ENODATA;
 		break;
@@ -398,6 +408,11 @@ static int lm25066_write_word_data(struct i2c_client *client, int page, int reg,
 	case PMBUS_VIRT_RESET_PIN_HISTORY:
 		ret = pmbus_write_byte(client, 0, LM25066_CLEAR_PIN_PEAK);
 		break;
+	case PMBUS_VIRT_SAMPLES:
+		word = clamp_val(word, 1, LM25066_SAMPLES_FOR_AVG_MAX);
+		ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
+					    ilog2(word));
+		break;
 	default:
 		ret = -ENODATA;
 		break;
@@ -438,7 +453,7 @@ static int lm25066_probe(struct i2c_client *client,
 
 	info->func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VMON
 	  | PMBUS_HAVE_PIN | PMBUS_HAVE_IIN | PMBUS_HAVE_STATUS_INPUT
-	  | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
+	  | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_SAMPLES;
 
 	if (data->id == lm25056) {
 		info->func[0] |= PMBUS_HAVE_STATUS_VMON;
-- 
2.20.1

[PATCH v3 4/4] pmbus_core: export coefficients via sysfs

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

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

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);
-- 
2.20.1

Re: [PATCH v3 4/4] pmbus_core: export coefficients via sysfs

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

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

Re: [PATCH v3 4/4] pmbus_core: export coefficients via sysfs

[Guenter Roeck]

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

Re: [PATCH v3 4/4] pmbus_core: export coefficients via sysfs

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

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

Re: [PATCH v3 4/4] pmbus_core: export coefficients via sysfs

[Guenter Roeck]

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

Re: [PATCH v3 1/4] pmbus: introduce PMBUS_VIRT_*_SAMPLES registers

[Guenter Roeck]

On Sun, Apr 14, 2019 at 09:58:18PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> Those virtual registers can be used to export manufacturer specific
> functionality for controlling the number of samples for average values
> reported by the device.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/pmbus.h      |  15 +++++
>  drivers/hwmon/pmbus/pmbus_core.c | 110 +++++++++++++++++++++++++++++++
>  2 files changed, 125 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index 1d24397d36ec..e73289cc867d 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -217,6 +217,20 @@ enum pmbus_regs {
>  	PMBUS_VIRT_PWM_ENABLE_2,
>  	PMBUS_VIRT_PWM_ENABLE_3,
>  	PMBUS_VIRT_PWM_ENABLE_4,
> +
> +	/* Samples for average
> +	 *
> +	 * Drivers wanting to expose functionality for changing the number of
> +	 * samples used for average values should implement support in
> +	 * {read,write}_word_data callback for either PMBUS_VIRT_SAMPLES if it
> +	 * applies to all types of measurements, or any number of specific
> +	 * PMBUS_VIRT_*_SAMPLES registers to allow for individual control.
> +	 */
> +	PMBUS_VIRT_SAMPLES,
> +	PMBUS_VIRT_IN_SAMPLES,
> +	PMBUS_VIRT_CURR_SAMPLES,
> +	PMBUS_VIRT_POWER_SAMPLES,
> +	PMBUS_VIRT_TEMP_SAMPLES,
>  };
>  
>  /*
> @@ -371,6 +385,7 @@ enum pmbus_sensor_classes {
>  #define PMBUS_HAVE_STATUS_VMON	BIT(19)
>  #define PMBUS_HAVE_PWM12	BIT(20)
>  #define PMBUS_HAVE_PWM34	BIT(21)
> +#define PMBUS_HAVE_SAMPLES	BIT(22)
>  
>  #define PMBUS_PAGE_VIRTUAL	BIT(31)
>  
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 2e2b5851139c..9b7d493c98b9 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -1901,6 +1901,112 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
>  	return 0;
>  }
>  
> +struct pmbus_samples_attr {
> +	int reg;
> +	char *name;
> +};
> +
> +struct pmbus_samples_reg {
> +	int page;
> +	struct pmbus_samples_attr *attr;
> +	struct device_attribute dev_attr;
> +};
> +
> +static struct pmbus_samples_attr pmbus_samples_registers[] = {
> +	{
> +		.reg = PMBUS_VIRT_SAMPLES,
> +		.name = "samples",
> +	}, {
> +		.reg = PMBUS_VIRT_IN_SAMPLES,
> +		.name = "in_samples",
> +	}, {
> +		.reg = PMBUS_VIRT_CURR_SAMPLES,
> +		.name = "curr_samples",
> +	}, {
> +		.reg = PMBUS_VIRT_POWER_SAMPLES,
> +		.name = "power_samples",
> +	}, {
> +		.reg = PMBUS_VIRT_TEMP_SAMPLES,
> +		.name = "temp_samples",
> +	}
> +};
> +
> +#define to_samples_reg(x) container_of(x, struct pmbus_samples_reg, dev_attr)
> +
> +static ssize_t pmbus_show_samples(struct device *dev,
> +				  struct device_attribute *devattr, char *buf)
> +{
> +	int val;
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	struct pmbus_samples_reg *reg = to_samples_reg(devattr);
> +
> +	val = _pmbus_read_word_data(client, reg->page, reg->attr->reg);
> +	if (val < 0)
> +		return val;
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", val);
> +}
> +
> +static ssize_t pmbus_set_samples(struct device *dev,
> +				 struct device_attribute *devattr,
> +				 const char *buf, size_t count)
> +{
> +	int ret;
> +	long val;
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	struct pmbus_samples_reg *reg = to_samples_reg(devattr);
> +
> +	if (kstrtol(buf, 0, &val) < 0)
> +		return -EINVAL;
> +
> +	ret = _pmbus_write_word_data(client, reg->page, reg->attr->reg, val);
> +
> +	return ret ? : count;
> +}
> +
> +static int pmbus_add_samples_attr(struct pmbus_data *data, int page,
> +				  struct pmbus_samples_attr *attr)
> +{
> +	struct pmbus_samples_reg *reg;
> +
> +	reg = devm_kzalloc(data->dev, sizeof(*reg), GFP_KERNEL);
> +	if (!reg)
> +		return -ENOMEM;
> +
> +	reg->attr = attr;
> +	reg->page = page;
> +
> +	pmbus_dev_attr_init(&reg->dev_attr, attr->name, 0644,
> +			    pmbus_show_samples, pmbus_set_samples);
> +
> +	return pmbus_add_attribute(data, &reg->dev_attr.attr);
> +}
> +
> +static int pmbus_add_samples_attributes(struct i2c_client *client,
> +					struct pmbus_data *data)
> +{
> +	const struct pmbus_driver_info *info = data->info;
> +	int s;
> +
> +	if (!(info->func[0] & PMBUS_HAVE_SAMPLES))
> +		return 0;
> +
> +	for (s = 0; s < ARRAY_SIZE(pmbus_samples_registers); s++) {
> +		struct pmbus_samples_attr *attr;
> +		int ret;
> +
> +		attr = &pmbus_samples_registers[s];
> +		if (!pmbus_check_word_register(client, 0, attr->reg))
> +			continue;
> +
> +		ret = pmbus_add_samples_attr(data, 0, attr);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int pmbus_find_attributes(struct i2c_client *client,
>  				 struct pmbus_data *data)
>  {
> @@ -1932,6 +2038,10 @@ static int pmbus_find_attributes(struct i2c_client *client,
>  
>  	/* Fans */
>  	ret = pmbus_add_fan_attributes(client, data);
> +	if (ret)
> +		return ret;
> +
> +	ret = pmbus_add_samples_attributes(client, data);
>  	return ret;
>  }
>  

Re: [PATCH v3 2/4] hwmon: Document the samples attributes

[Guenter Roeck]

On Sun, Apr 14, 2019 at 09:58:40PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> Document new ABI attributes: {in,power,curr,temp}_samples and samples.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  Documentation/hwmon/sysfs-interface | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> index 2b9e1005d88b..7b91706d01c8 100644
> --- a/Documentation/hwmon/sysfs-interface
> +++ b/Documentation/hwmon/sysfs-interface
> @@ -756,6 +756,24 @@ intrusion[0-*]_beep
>  		1: enable
>  		RW
>  
> +********************************
> +* Average sample configuration *
> +********************************
> +
> +Devices allowing for reading {in,power,curr,temp}_average values may export
> +attributes for controlling number of samples used to compute average.
> +
> +samples		Sets number of average samples for all types of measurements.
> +		RW
> +
> +in_samples
> +power_samples
> +curr_samples
> +temp_samples    Sets number of average samples for specific type of measurements.
> +		Note that on some devices it won't be possible to set all of them
> +		to different values so changing one might also change some others.
> +		RW
> +
>  
>  sysfs attribute writes interpretation
>  -------------------------------------

Re: [PATCH v3 3/4] lm25066: support SAMPLES_FOR_AVG register

[Guenter Roeck]

On Sun, Apr 14, 2019 at 09:59:19PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> Manufacturer specific SAMPLES_FOR_AVG register allows setting the number
> of samples used in computing the average values (PMBUS_VIRT_READ_*_AVG).
> The number we write is an exponent of base 2 of the number of samples so
> for example writing 3 will result in 8 samples average.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/lm25066.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
> index 53db78753a0d..715d4ab57516 100644
> --- a/drivers/hwmon/pmbus/lm25066.c
> +++ b/drivers/hwmon/pmbus/lm25066.c
> @@ -26,6 +26,7 @@
>  #include <linux/err.h>
>  #include <linux/slab.h>
>  #include <linux/i2c.h>
> +#include <linux/log2.h>
>  #include "pmbus.h"
>  
>  enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
> @@ -39,12 +40,15 @@ enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
>  #define LM25066_CLEAR_PIN_PEAK		0xd6
>  #define LM25066_DEVICE_SETUP		0xd9
>  #define LM25066_READ_AVG_VIN		0xdc
> +#define LM25066_SAMPLES_FOR_AVG		0xdb
>  #define LM25066_READ_AVG_VOUT		0xdd
>  #define LM25066_READ_AVG_IIN		0xde
>  #define LM25066_READ_AVG_PIN		0xdf
>  
>  #define LM25066_DEV_SETUP_CL		BIT(4)	/* Current limit */
>  
> +#define LM25066_SAMPLES_FOR_AVG_MAX	4096
> +
>  /* LM25056 only */
>  
>  #define LM25056_VAUX_OV_WARN_LIMIT	0xe3
> @@ -284,6 +288,12 @@ static int lm25066_read_word_data(struct i2c_client *client, int page, int reg)
>  	case PMBUS_VIRT_RESET_PIN_HISTORY:
>  		ret = 0;
>  		break;
> +	case PMBUS_VIRT_SAMPLES:
> +		ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG);
> +		if (ret < 0)
> +			break;
> +		ret = 1 << ret;
> +		break;
>  	default:
>  		ret = -ENODATA;
>  		break;
> @@ -398,6 +408,11 @@ static int lm25066_write_word_data(struct i2c_client *client, int page, int reg,
>  	case PMBUS_VIRT_RESET_PIN_HISTORY:
>  		ret = pmbus_write_byte(client, 0, LM25066_CLEAR_PIN_PEAK);
>  		break;
> +	case PMBUS_VIRT_SAMPLES:
> +		word = clamp_val(word, 1, LM25066_SAMPLES_FOR_AVG_MAX);
> +		ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
> +					    ilog2(word));
> +		break;
>  	default:
>  		ret = -ENODATA;
>  		break;
> @@ -438,7 +453,7 @@ static int lm25066_probe(struct i2c_client *client,
>  
>  	info->func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VMON
>  	  | PMBUS_HAVE_PIN | PMBUS_HAVE_IIN | PMBUS_HAVE_STATUS_INPUT
> -	  | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
> +	  | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_SAMPLES;
>  
>  	if (data->id == lm25056) {
>  		info->func[0] |= PMBUS_HAVE_STATUS_VMON;

Re: [PATCH v3 4/4] pmbus_core: export coefficients via sysfs

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

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