[PATCH 0/3] pmbus: extend configurability via sysfs

[PATCH 0/3] 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 two patches allows lm25066 driver to export custom sysfs entry
  for 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 then per device so using
  device-tree or similar concepts to store them is not an option too.
  Thus, we export it so that the logic of loading proper coeffs can
  be implemented in user space instead.

Krzysztof Adamski (3):
  pmbus: support for custom sysfs attributes
  lm25066: export sysfs attribute for SAMPLES_FOR_AVG
  pmbus: export coefficients via sysfs

 drivers/hwmon/pmbus/lm25066.c    |  45 +++++++++++++
 drivers/hwmon/pmbus/pmbus.h      |   3 +
 drivers/hwmon/pmbus/pmbus_core.c | 109 ++++++++++++++++++++++++++++++-
 3 files changed, 155 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH 1/3] pmbus: support for custom sysfs attributes

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

This patch makes it possible to pass custom struct attribute_group array
via the pmbus_driver_info struct so that those can be added to the
attribute groups passed to hwmon_device_register_with_groups().

This makes it possible to register custom sysfs attributes by PMBUS
drivers similar to how you can do this with most other busses/classes.

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

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 1d24397d36ec..fb267ec11623 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -417,6 +417,9 @@ struct pmbus_driver_info {
 	/* Regulator functionality, if supported by this chip driver. */
 	int num_regulators;
 	const struct regulator_desc *reg_desc;
+
+	/* custom attributes */
+	const struct attribute_group **groups;
 };
 
 /* Regulator ops */
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 2e2b5851139c..7a7dcdc8acc9 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -103,7 +103,7 @@ struct pmbus_data {
 	int max_attributes;
 	int num_attributes;
 	struct attribute_group group;
-	const struct attribute_group *groups[2];
+	const struct attribute_group **groups;
 	struct dentry *debugfs;		/* debugfs device directory */
 
 	struct pmbus_sensor *sensors;
@@ -2305,6 +2305,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 	struct device *dev = &client->dev;
 	const struct pmbus_platform_data *pdata = dev_get_platdata(dev);
 	struct pmbus_data *data;
+	size_t groups_num = 0;
 	int ret;
 
 	if (!info)
@@ -2319,6 +2320,15 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 	if (!data)
 		return -ENOMEM;
 
+	if (info->groups)
+		while (info->groups[groups_num])
+			groups_num++;
+
+	data->groups = devm_kcalloc(dev, groups_num + 2, sizeof(void *),
+				    GFP_KERNEL);
+	if (!data->groups)
+		return -ENOMEM;
+
 	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
 	data->dev = dev;
@@ -2346,6 +2356,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 	}
 
 	data->groups[0] = &data->group;
+	memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num);
 	data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
 							    data, data->groups);
 	if (IS_ERR(data->hwmon_dev)) {
-- 
2.20.1

[PATCH 2/3] lm25066: export sysfs attribute for SAMPLES_FOR_AVG

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

Register custom sysfs attribute to be registered by pmbus allowing
read/write access to the manufacturer specific SAMPLES_FOR_AVG register.
This 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 | 45 +++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
index 53db78753a0d..c78af0a7e5ff 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 };
@@ -38,6 +39,7 @@ enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
 #define LM25066_READ_PIN_PEAK		0xd5
 #define LM25066_CLEAR_PIN_PEAK		0xd6
 #define LM25066_DEVICE_SETUP		0xd9
+#define LM25066_SAMPLES_FOR_AVG		0xdb
 #define LM25066_READ_AVG_VIN		0xdc
 #define LM25066_READ_AVG_VOUT		0xdd
 #define LM25066_READ_AVG_IIN		0xde
@@ -405,6 +407,47 @@ static int lm25066_write_word_data(struct i2c_client *client, int page, int reg,
 	return ret;
 }
 
+static ssize_t samples_for_avg_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret;
+
+	ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", 1 << ret);
+}
+
+static ssize_t samples_for_avg_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret, val;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret < 0)
+		return ret;
+
+	ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
+				    ilog2(val));
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(samples_for_avg);
+
+static struct attribute *lm25056_attrs[] = {
+	&dev_attr_samples_for_avg.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all
+			   // those attributes in subdirectory? Like "custom" ?
+
 static int lm25066_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
@@ -476,6 +519,8 @@ static int lm25066_probe(struct i2c_client *client,
 		info->b[PSC_POWER] = coeff[PSC_POWER].b;
 	}
 
+	info->groups = lm25056_groups;
+
 	return pmbus_do_probe(client, id, info);
 }
 
-- 
2.20.1

[PATCH 3/3] pmbus: export coefficients via sysfs

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

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.

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

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 7a7dcdc8acc9..8cb61fc977db 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,11 +100,12 @@ 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;
+	struct attribute_group group_coeff;
 	const struct attribute_group **groups;
 	struct dentry *debugfs;		/* debugfs device directory */
 
@@ -1901,6 +1904,88 @@ static int pmbus_add_fan_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] = "p",
+	[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) * (PSC_NUM_CLASSES + 1),
+			sizeof(void *), GFP_KERNEL);
+	if (!attrs)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(classes_names); i++) {
+		if (classes_names[i] == NULL)
+			continue;
+
+		ret = pmbus_add_coeff_attributes_class(data, classes_names[i],
+						       i, &attrs[n]);
+		if (ret < 0) {
+			kfree(attrs);
+			return ret;
+		}
+
+		n += ret;
+	}
+
+	data->group_coeff.name = "coefficients";
+	data->group_coeff.attrs =  attrs;
+
+	return 0;
+}
+
 static int pmbus_find_attributes(struct i2c_client *client,
 				 struct pmbus_data *data)
 {
@@ -1932,6 +2017,11 @@ static int pmbus_find_attributes(struct i2c_client *client,
 
 	/* Fans */
 	ret = pmbus_add_fan_attributes(client, data);
+	if (ret)
+		return ret;
+
+	/* Coefficients */
+	ret = pmbus_add_coeff_attributes(client, data);
 	return ret;
 }
 
@@ -2324,7 +2414,8 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		while (info->groups[groups_num])
 			groups_num++;
 
-	data->groups = devm_kcalloc(dev, groups_num + 2, sizeof(void *),
+	/* Two default groups + ending NULL makes 3 groups minimum */
+	data->groups = devm_kcalloc(dev, groups_num + 3, sizeof(void *),
 				    GFP_KERNEL);
 	if (!data->groups)
 		return -ENOMEM;
@@ -2356,7 +2447,8 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 	}
 
 	data->groups[0] = &data->group;
-	memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num);
+	data->groups[1] = &data->group_coeff;
+	memcpy(data->groups + 2, info->groups, sizeof(void *) * groups_num);
 	data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
 							    data, data->groups);
 	if (IS_ERR(data->hwmon_dev)) {
@@ -2379,6 +2471,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);
@@ -2391,6 +2484,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 3/3] pmbus: export coefficients via sysfs

[Guenter Roeck]

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.

Guenter

> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > ---
>   drivers/hwmon/pmbus/pmbus_core.c | 100 ++++++++++++++++++++++++++++++-
>   1 file changed, 97 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 7a7dcdc8acc9..8cb61fc977db 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,11 +100,12 @@ 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;
> +	struct attribute_group group_coeff;
>   	const struct attribute_group **groups;
>   	struct dentry *debugfs;		/* debugfs device directory */
>   
> @@ -1901,6 +1904,88 @@ static int pmbus_add_fan_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] = "p",
> +	[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) * (PSC_NUM_CLASSES + 1),
> +			sizeof(void *), GFP_KERNEL);
> +	if (!attrs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < ARRAY_SIZE(classes_names); i++) {
> +		if (classes_names[i] == NULL)
> +			continue;
> +
> +		ret = pmbus_add_coeff_attributes_class(data, classes_names[i],
> +						       i, &attrs[n]);
> +		if (ret < 0) {
> +			kfree(attrs);
> +			return ret;
> +		}
> +
> +		n += ret;
> +	}
> +
> +	data->group_coeff.name = "coefficients";
> +	data->group_coeff.attrs =  attrs;
> +
> +	return 0;
> +}
> +
>   static int pmbus_find_attributes(struct i2c_client *client,
>   				 struct pmbus_data *data)
>   {
> @@ -1932,6 +2017,11 @@ static int pmbus_find_attributes(struct i2c_client *client,
>   
>   	/* Fans */
>   	ret = pmbus_add_fan_attributes(client, data);
> +	if (ret)
> +		return ret;
> +
> +	/* Coefficients */
> +	ret = pmbus_add_coeff_attributes(client, data);
>   	return ret;
>   }
>   
> @@ -2324,7 +2414,8 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>   		while (info->groups[groups_num])
>   			groups_num++;
>   
> -	data->groups = devm_kcalloc(dev, groups_num + 2, sizeof(void *),
> +	/* Two default groups + ending NULL makes 3 groups minimum */
> +	data->groups = devm_kcalloc(dev, groups_num + 3, sizeof(void *),
>   				    GFP_KERNEL);
>   	if (!data->groups)
>   		return -ENOMEM;
> @@ -2356,7 +2447,8 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>   	}
>   
>   	data->groups[0] = &data->group;
> -	memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num);
> +	data->groups[1] = &data->group_coeff;
> +	memcpy(data->groups + 2, info->groups, sizeof(void *) * groups_num);
>   	data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
>   							    data, data->groups);
>   	if (IS_ERR(data->hwmon_dev)) {
> @@ -2379,6 +2471,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);
> @@ -2391,6 +2484,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);
> 

Re: [PATCH 1/3] pmbus: support for custom sysfs attributes

[Guenter Roeck]

On 4/10/19 3:38 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> This patch makes it possible to pass custom struct attribute_group array
> via the pmbus_driver_info struct so that those can be added to the
> attribute groups passed to hwmon_device_register_with_groups().
> 
> This makes it possible to register custom sysfs attributes by PMBUS
> drivers similar to how you can do this with most other busses/classes.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
>   drivers/hwmon/pmbus/pmbus.h      |  3 +++
>   drivers/hwmon/pmbus/pmbus_core.c | 13 ++++++++++++-
>   2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index 1d24397d36ec..fb267ec11623 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -417,6 +417,9 @@ struct pmbus_driver_info {
>   	/* Regulator functionality, if supported by this chip driver. */
>   	int num_regulators;
>   	const struct regulator_desc *reg_desc;
> +
> +	/* custom attributes */
> +	const struct attribute_group **groups;

I can understand the need and desire for one additional group. More than one
is highly questionable. Please explain why you think that more than one extra
attribute would ever be needed. It does add substantial complexity, so
there should be a good reason.

Thanks,
Guenter

>   };
>   
>   /* Regulator ops */
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 2e2b5851139c..7a7dcdc8acc9 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -103,7 +103,7 @@ struct pmbus_data {
>   	int max_attributes;
>   	int num_attributes;
>   	struct attribute_group group;
> -	const struct attribute_group *groups[2];
> +	const struct attribute_group **groups;
>   	struct dentry *debugfs;		/* debugfs device directory */
>   
>   	struct pmbus_sensor *sensors;
> @@ -2305,6 +2305,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>   	struct device *dev = &client->dev;
>   	const struct pmbus_platform_data *pdata = dev_get_platdata(dev);
>   	struct pmbus_data *data;
> +	size_t groups_num = 0;
>   	int ret;
>   
>   	if (!info)
> @@ -2319,6 +2320,15 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>   	if (!data)
>   		return -ENOMEM;
>   
> +	if (info->groups)
> +		while (info->groups[groups_num])
> +			groups_num++;
> +
> +	data->groups = devm_kcalloc(dev, groups_num + 2, sizeof(void *),
> +				    GFP_KERNEL);
> +	if (!data->groups)
> +		return -ENOMEM;
> +
>   	i2c_set_clientdata(client, data);
>   	mutex_init(&data->update_lock);
>   	data->dev = dev;
> @@ -2346,6 +2356,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>   	}
>   
>   	data->groups[0] = &data->group;
> +	memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num);
>   	data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
>   							    data, data->groups);
>   	if (IS_ERR(data->hwmon_dev)) {
> 

Re: [PATCH 2/3] lm25066: export sysfs attribute for SAMPLES_FOR_AVG

[Guenter Roeck]

On 4/10/19 3:39 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> Register custom sysfs attribute to be registered by pmbus allowing
> read/write access to the manufacturer specific SAMPLES_FOR_AVG register.
> This 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 | 45 +++++++++++++++++++++++++++++++++++
>   1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
> index 53db78753a0d..c78af0a7e5ff 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 };
> @@ -38,6 +39,7 @@ enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
>   #define LM25066_READ_PIN_PEAK		0xd5
>   #define LM25066_CLEAR_PIN_PEAK		0xd6
>   #define LM25066_DEVICE_SETUP		0xd9
> +#define LM25066_SAMPLES_FOR_AVG		0xdb
>   #define LM25066_READ_AVG_VIN		0xdc
>   #define LM25066_READ_AVG_VOUT		0xdd
>   #define LM25066_READ_AVG_IIN		0xde
> @@ -405,6 +407,47 @@ static int lm25066_write_word_data(struct i2c_client *client, int page, int reg,
>   	return ret;
>   }
>   
> +static ssize_t samples_for_avg_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	int ret;
> +
> +	ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", 1 << ret);
> +}
> +
> +static ssize_t samples_for_avg_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	int ret, val;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
> +				    ilog2(val));
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(samples_for_avg);
> +
> +static struct attribute *lm25056_attrs[] = {
> +	&dev_attr_samples_for_avg.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all
> +			   // those attributes in subdirectory? Like "custom" ?
> +
We don't add subdirectories for other chips, and we won't start it here.

I don't mind the attribute itself, but I do mind its name. We'll have
to find something more generic, such as 'num_samples' or just 'samples'.
I am open to suggestions. We'll also have to decide if the attribute(s)
should be limited to one per chip, or if there can be multiple attributes.
For example, MAX34462 has separate values for iout_samples and adc_average.
Do we want samples, {curr,in,power,...}_samples, or both ? Or even
currX_samples ?

Either case, this is needed for more than one pmbus chip (and I know
it is needed for some non-PMBus chips). I am inclined to add it as generic
attribute into the pmbus core instead (and possibly add it to the ABI).

Thanks,
Guenter

>   static int lm25066_probe(struct i2c_client *client,
>   			  const struct i2c_device_id *id)
>   {
> @@ -476,6 +519,8 @@ static int lm25066_probe(struct i2c_client *client,
>   		info->b[PSC_POWER] = coeff[PSC_POWER].b;
>   	}
>   
> +	info->groups = lm25056_groups;
> +
>   	return pmbus_do_probe(client, id, info);
>   }
>   
> 

Re: [PATCH 2/3] lm25066: export sysfs attribute for SAMPLES_FOR_AVG

[Nicolin Chen]

On Wed, Apr 10, 2019 at 05:55:19PM -0700, Guenter Roeck wrote:

> > +static ssize_t samples_for_avg_show(struct device *dev,
> > +				    struct device_attribute *attr, char *buf)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev->parent);
> > +	int ret;
> > +
> > +	ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return sprintf(buf, "%d\n", 1 << ret);
> > +}
> > +
> > +static ssize_t samples_for_avg_store(struct device *dev,
> > +				     struct device_attribute *attr,
> > +				     const char *buf, size_t count)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev->parent);
> > +	int ret, val;
> > +
> > +	ret = kstrtoint(buf, 0, &val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
> > +				    ilog2(val));
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return count;
> > +}
> > +
> > +static DEVICE_ATTR_RW(samples_for_avg);
> > +
> > +static struct attribute *lm25056_attrs[] = {
> > +	&dev_attr_samples_for_avg.attr,
> > +	NULL,
> > +};
> > +ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all
> > +			   // those attributes in subdirectory? Like "custom" ?
> > +
> We don't add subdirectories for other chips, and we won't start it here.
> 
> I don't mind the attribute itself, but I do mind its name. We'll have
> to find something more generic, such as 'num_samples' or just 'samples'.
> I am open to suggestions. We'll also have to decide if the attribute(s)
> should be limited to one per chip, or if there can be multiple attributes.
> For example, MAX34462 has separate values for iout_samples and adc_average.
> Do we want samples, {curr,in,power,...}_samples, or both ? Or even
> currX_samples ?

For my use case -- TI's INA chips, there is only one "samples"
configuration being used for all currX_inputs and inX_inputs.
So having a "samples" node would certainly fit better than an
in0_samples. So I vote for having both.

Thank you
Nicolin

Re: [PATCH 3/3] pmbus: export coefficients via sysfs

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

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.

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.

[1]: http://www.ti.com/lit/ds/symlink/lm5064.pdf

Krzysztof

Re: [PATCH 1/3] pmbus: support for custom sysfs attributes

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

On Wed, Apr 10, 2019 at 05:35:21PM -0700, Guenter Roeck wrote:
>On 4/10/19 3:38 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>>This patch makes it possible to pass custom struct attribute_group array
>>via the pmbus_driver_info struct so that those can be added to the
>>attribute groups passed to hwmon_device_register_with_groups().
>>
>>This makes it possible to register custom sysfs attributes by PMBUS
>>drivers similar to how you can do this with most other busses/classes.
>>
>>Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>>---
>>  drivers/hwmon/pmbus/pmbus.h      |  3 +++
>>  drivers/hwmon/pmbus/pmbus_core.c | 13 ++++++++++++-
>>  2 files changed, 15 insertions(+), 1 deletion(-)
>>
>>diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
>>index 1d24397d36ec..fb267ec11623 100644
>>--- a/drivers/hwmon/pmbus/pmbus.h
>>+++ b/drivers/hwmon/pmbus/pmbus.h
>>@@ -417,6 +417,9 @@ struct pmbus_driver_info {
>>  	/* Regulator functionality, if supported by this chip driver. */
>>  	int num_regulators;
>>  	const struct regulator_desc *reg_desc;
>>+
>>+	/* custom attributes */
>>+	const struct attribute_group **groups;
>
>I can understand the need and desire for one additional group. More than one
>is highly questionable. Please explain why you think that more than one extra
>attribute would ever be needed. It does add substantial complexity, so
>there should be a good reason.

The only situation I could come up is if the driver would want to group
attributes in different directories by setting different name for each
of them. One other reason I choose to use this approach is that this
seems to be standard way for passing this information on other
layers/frameworks.  For example, this is the same "format" you would
pass this kind of data when creating a class, a bus, a driver or when
you use any of the *_register_with_groups().

This approach is simply more generic with (to my opinion) low cost of
implementation. But if we don't want to support that, I'm fine to change
this to single custom group.

Krzysztof

Re: [PATCH 2/3] lm25066: export sysfs attribute for SAMPLES_FOR_AVG

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

On Wed, Apr 10, 2019 at 09:24:29PM -0700, Nicolin Chen wrote:
>On Wed, Apr 10, 2019 at 05:55:19PM -0700, Guenter Roeck wrote:
>
>> > +static ssize_t samples_for_avg_show(struct device *dev,
>> > +				    struct device_attribute *attr, char *buf)
>> > +{
>> > +	struct i2c_client *client = to_i2c_client(dev->parent);
>> > +	int ret;
>> > +
>> > +	ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG);
>> > +	if (ret < 0)
>> > +		return ret;
>> > +
>> > +	return sprintf(buf, "%d\n", 1 << ret);
>> > +}
>> > +
>> > +static ssize_t samples_for_avg_store(struct device *dev,
>> > +				     struct device_attribute *attr,
>> > +				     const char *buf, size_t count)
>> > +{
>> > +	struct i2c_client *client = to_i2c_client(dev->parent);
>> > +	int ret, val;
>> > +
>> > +	ret = kstrtoint(buf, 0, &val);
>> > +	if (ret < 0)
>> > +		return ret;
>> > +
>> > +	ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
>> > +				    ilog2(val));
>> > +	if (ret < 0)
>> > +		return ret;
>> > +
>> > +	return count;
>> > +}
>> > +
>> > +static DEVICE_ATTR_RW(samples_for_avg);
>> > +
>> > +static struct attribute *lm25056_attrs[] = {
>> > +	&dev_attr_samples_for_avg.attr,
>> > +	NULL,
>> > +};
>> > +ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all
>> > +			   // those attributes in subdirectory? Like "custom" ?
>> > +
>> We don't add subdirectories for other chips, and we won't start it here.
>>
>> I don't mind the attribute itself, but I do mind its name. We'll have
>> to find something more generic, such as 'num_samples' or just 'samples'.
>> I am open to suggestions. We'll also have to decide if the attribute(s)
>> should be limited to one per chip, or if there can be multiple attributes.
>> For example, MAX34462 has separate values for iout_samples and adc_average.
>> Do we want samples, {curr,in,power,...}_samples, or both ? Or even
>> currX_samples ?
>
>For my use case -- TI's INA chips, there is only one "samples"
>configuration being used for all currX_inputs and inX_inputs.
>So having a "samples" node would certainly fit better than an
>in0_samples. So I vote for having both.

The name is family specific. The data sheet calls this register exactly
like that so I used this name. But I agree that we could standardise on
some common name, even if the actual implementation will be
device-specific.

The LM5064 has one value for all readings, ADM1293 would have one
setting for PIN and separate one for VIN/VAUX/IOUT.

So maybe it makes sense to allow for some device specific naming (with
prefixes) where it makes sense but default to "samples" in simple case
of shared value? In this case, if there is specific value like
"curr_samples", user knows exactly which readings are influenced but
when using "samples" it needs to have device specific knowledge to
understand this.

I'm just not sure what would be the best way to express situation for
adm1293 for example - the PIN case is simple but what to do with
"shared" settings - expose both curr_samples/in_samples and writing to
one would change the other as well? Or just default to "samples" for
this case and have separate "power_samples" just for PIN?

Krzysztof

Re: [PATCH 2/3] lm25066: export sysfs attribute for SAMPLES_FOR_AVG

[Guenter Roeck]

On 4/11/19 1:09 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> On Wed, Apr 10, 2019 at 09:24:29PM -0700, Nicolin Chen wrote:
>> On Wed, Apr 10, 2019 at 05:55:19PM -0700, Guenter Roeck wrote:
>>
>>>> +static ssize_t samples_for_avg_show(struct device *dev,
>>>> +				    struct device_attribute *attr, char *buf)
>>>> +{
>>>> +	struct i2c_client *client = to_i2c_client(dev->parent);
>>>> +	int ret;
>>>> +
>>>> +	ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	return sprintf(buf, "%d\n", 1 << ret);
>>>> +}
>>>> +
>>>> +static ssize_t samples_for_avg_store(struct device *dev,
>>>> +				     struct device_attribute *attr,
>>>> +				     const char *buf, size_t count)
>>>> +{
>>>> +	struct i2c_client *client = to_i2c_client(dev->parent);
>>>> +	int ret, val;
>>>> +
>>>> +	ret = kstrtoint(buf, 0, &val);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
>>>> +				    ilog2(val));
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	return count;
>>>> +}
>>>> +
>>>> +static DEVICE_ATTR_RW(samples_for_avg);
>>>> +
>>>> +static struct attribute *lm25056_attrs[] = {
>>>> +	&dev_attr_samples_for_avg.attr,
>>>> +	NULL,
>>>> +};
>>>> +ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all
>>>> +			   // those attributes in subdirectory? Like "custom" ?
>>>> +
>>> We don't add subdirectories for other chips, and we won't start it here.
>>>
>>> I don't mind the attribute itself, but I do mind its name. We'll have
>>> to find something more generic, such as 'num_samples' or just 'samples'.
>>> I am open to suggestions. We'll also have to decide if the attribute(s)
>>> should be limited to one per chip, or if there can be multiple attributes.
>>> For example, MAX34462 has separate values for iout_samples and adc_average.
>>> Do we want samples, {curr,in,power,...}_samples, or both ? Or even
>>> currX_samples ?
>>
>> For my use case -- TI's INA chips, there is only one "samples"
>> configuration being used for all currX_inputs and inX_inputs.
>> So having a "samples" node would certainly fit better than an
>> in0_samples. So I vote for having both.
> 
> The name is family specific. The data sheet calls this register exactly
> like that so I used this name. But I agree that we could standardise on

Well, yes, but the whole point of an ABI is to make it chip independent.

> some common name, even if the actual implementation will be
> device-specific.
> 
> The LM5064 has one value for all readings, ADM1293 would have one
> setting for PIN and separate one for VIN/VAUX/IOUT.
> 
> So maybe it makes sense to allow for some device specific naming (with
> prefixes) where it makes sense but default to "samples" in simple case
> of shared value? In this case, if there is specific value like
> "curr_samples", user knows exactly which readings are influenced but
> when using "samples" it needs to have device specific knowledge to
> understand this.
> 
Let's go for "samples" and {in,curr,power,temp,...}_samples. "samples"
should be used if the attribute applies to all sensors.

> I'm just not sure what would be the best way to express situation for
> adm1293 for example - the PIN case is simple but what to do with
> "shared" settings - expose both curr_samples/in_samples and writing to
> one would change the other as well? Or just default to "samples" for
> this case and have separate "power_samples" just for PIN?
> 
Both "samples" and "power_samples" at the same time would be confusing.
Common implementation in situations like this is to have both curr_samples
and in_samples, and changing one would also change the other (or only one
would be writable, but that is just an implementation detail).

So what we need is virtual registers (PMBUS_VIRT_SAMPLES, PMBUS_VIRT_IN_SAMPLES,
and so on), plus the necessary code in pmbus_core.c and the implementation
in the chip driver. We'll also need to document new ABI attributes (samples,
in_samples, temp_samples, ...).

Any takers ?

Nicolin, I think with that you can move forward with the TI INA chip
implementation. I agree that 'samples' would be most appropriate for
this chip.

Thanks,
Guenter

Re: [PATCH 1/3] pmbus: support for custom sysfs attributes

[Guenter Roeck]

On 4/11/19 12:53 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> On Wed, Apr 10, 2019 at 05:35:21PM -0700, Guenter Roeck wrote:
>> On 4/10/19 3:38 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>>> This patch makes it possible to pass custom struct attribute_group array
>>> via the pmbus_driver_info struct so that those can be added to the
>>> attribute groups passed to hwmon_device_register_with_groups().
>>>
>>> This makes it possible to register custom sysfs attributes by PMBUS
>>> drivers similar to how you can do this with most other busses/classes.
>>>
>>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>>> ---
>>>   drivers/hwmon/pmbus/pmbus.h      |  3 +++
>>>   drivers/hwmon/pmbus/pmbus_core.c | 13 ++++++++++++-
>>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
>>> index 1d24397d36ec..fb267ec11623 100644
>>> --- a/drivers/hwmon/pmbus/pmbus.h
>>> +++ b/drivers/hwmon/pmbus/pmbus.h
>>> @@ -417,6 +417,9 @@ struct pmbus_driver_info {
>>>   	/* Regulator functionality, if supported by this chip driver. */
>>>   	int num_regulators;
>>>   	const struct regulator_desc *reg_desc;
>>> +
>>> +	/* custom attributes */
>>> +	const struct attribute_group **groups;
>>
>> I can understand the need and desire for one additional group. More than one
>> is highly questionable. Please explain why you think that more than one extra
>> attribute would ever be needed. It does add substantial complexity, so
>> there should be a good reason.
> 
> The only situation I could come up is if the driver would want to group
> attributes in different directories by setting different name for each
> of them. One other reason I choose to use this approach is that this
> seems to be standard way for passing this information on other
> layers/frameworks.  For example, this is the same "format" you would
> pass this kind of data when creating a class, a bus, a driver or when
> you use any of the *_register_with_groups().
> 
> This approach is simply more generic with (to my opinion) low cost of
> implementation. But if we don't want to support that, I'm fine to change
> this to single custom group.
> 

I think we can postpone this discussion for now, as we'll go with standard
attributes to be implemented in the pmbus core.

Thanks,
Guenter

Re: [PATCH 3/3] pmbus: export coefficients via sysfs

[Guenter Roeck]

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

Re: [PATCH 3/3] pmbus: export coefficients via sysfs

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

On Thu, Apr 11, 2019 at 06:39:11AM -0700, Guenter Roeck wrote:
>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.

But the coefficients will depend on the exact value of the shunt
resistor. To get bet accuracy, the coefficients are calculated for each
board unit. While all units of the same type share the devicetree
(otherwise this devicetree would have to be generated by some firmware
software first).

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

b/m/r are coefficients mentioned by PMBUS specification and the change
is PMBUS specific. My understanding is that all PMBUS devices report
values in real-world units (either in linear or direct mode) and this is
implemented this way in the code already.
In fact, if the device would report raw values, we wouldn't need this
patch as we would simply apply those coefficients in userspace. But if
drivers report real values which require correct coefficients to work, I
would expect to be able to tweak them.

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

I can fix all that, of course, but we have to discuss the the mentioned
issues first. I agree with all those arguments except the scope one. As
we currently only support specifying coefficients per class and this is
exacly how I export them. Unless I misunderstood your comment, of
course.

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

But this is not chip specific change, it is more like bus (PMBUS)
specific. All pmbus devices reporting in direct mode have those
coefficients, all of them use the same formula to calculate real values
out of raw values as this is specified in the PMBUS specification.

I agree this might not be useful for devices reporting in linear/vid
mode so I could exclude them when exporting coefficients.

Krzysztof

Re: [PATCH 2/3] lm25066: export sysfs attribute for SAMPLES_FOR_AVG

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

On Thu, Apr 11, 2019 at 06:07:47AM -0700, Guenter Roeck wrote:
>On 4/11/19 1:09 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>>On Wed, Apr 10, 2019 at 09:24:29PM -0700, Nicolin Chen wrote:
>>>On Wed, Apr 10, 2019 at 05:55:19PM -0700, Guenter Roeck wrote:
>>>
>>>>>+static ssize_t samples_for_avg_show(struct device *dev,
>>>>>+				    struct device_attribute *attr, char *buf)
>>>>>+{
>>>>>+	struct i2c_client *client = to_i2c_client(dev->parent);
>>>>>+	int ret;
>>>>>+
>>>>>+	ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG);
>>>>>+	if (ret < 0)
>>>>>+		return ret;
>>>>>+
>>>>>+	return sprintf(buf, "%d\n", 1 << ret);
>>>>>+}
>>>>>+
>>>>>+static ssize_t samples_for_avg_store(struct device *dev,
>>>>>+				     struct device_attribute *attr,
>>>>>+				     const char *buf, size_t count)
>>>>>+{
>>>>>+	struct i2c_client *client = to_i2c_client(dev->parent);
>>>>>+	int ret, val;
>>>>>+
>>>>>+	ret = kstrtoint(buf, 0, &val);
>>>>>+	if (ret < 0)
>>>>>+		return ret;
>>>>>+
>>>>>+	ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
>>>>>+				    ilog2(val));
>>>>>+	if (ret < 0)
>>>>>+		return ret;
>>>>>+
>>>>>+	return count;
>>>>>+}
>>>>>+
>>>>>+static DEVICE_ATTR_RW(samples_for_avg);
>>>>>+
>>>>>+static struct attribute *lm25056_attrs[] = {
>>>>>+	&dev_attr_samples_for_avg.attr,
>>>>>+	NULL,
>>>>>+};
>>>>>+ATTRIBUTE_GROUPS(lm25056); // should we set a name of this group to put all
>>>>>+			   // those attributes in subdirectory? Like "custom" ?
>>>>>+
>>>>We don't add subdirectories for other chips, and we won't start it here.
>>>>
>>>>I don't mind the attribute itself, but I do mind its name. We'll have
>>>>to find something more generic, such as 'num_samples' or just 'samples'.
>>>>I am open to suggestions. We'll also have to decide if the attribute(s)
>>>>should be limited to one per chip, or if there can be multiple attributes.
>>>>For example, MAX34462 has separate values for iout_samples and adc_average.
>>>>Do we want samples, {curr,in,power,...}_samples, or both ? Or even
>>>>currX_samples ?
>>>
>>>For my use case -- TI's INA chips, there is only one "samples"
>>>configuration being used for all currX_inputs and inX_inputs.
>>>So having a "samples" node would certainly fit better than an
>>>in0_samples. So I vote for having both.
>>
>>The name is family specific. The data sheet calls this register exactly
>>like that so I used this name. But I agree that we could standardise on
>
>Well, yes, but the whole point of an ABI is to make it chip independent.
>
>>some common name, even if the actual implementation will be
>>device-specific.
>>
>>The LM5064 has one value for all readings, ADM1293 would have one
>>setting for PIN and separate one for VIN/VAUX/IOUT.
>>
>>So maybe it makes sense to allow for some device specific naming (with
>>prefixes) where it makes sense but default to "samples" in simple case
>>of shared value? In this case, if there is specific value like
>>"curr_samples", user knows exactly which readings are influenced but
>>when using "samples" it needs to have device specific knowledge to
>>understand this.
>>
>Let's go for "samples" and {in,curr,power,temp,...}_samples. "samples"
>should be used if the attribute applies to all sensors.
>
>>I'm just not sure what would be the best way to express situation for
>>adm1293 for example - the PIN case is simple but what to do with
>>"shared" settings - expose both curr_samples/in_samples and writing to
>>one would change the other as well? Or just default to "samples" for
>>this case and have separate "power_samples" just for PIN?
>>
>Both "samples" and "power_samples" at the same time would be confusing.
>Common implementation in situations like this is to have both curr_samples
>and in_samples, and changing one would also change the other (or only one
>would be writable, but that is just an implementation detail).
>
>So what we need is virtual registers (PMBUS_VIRT_SAMPLES, PMBUS_VIRT_IN_SAMPLES,
>and so on), plus the necessary code in pmbus_core.c and the implementation
>in the chip driver. We'll also need to document new ABI attributes (samples,
>in_samples, temp_samples, ...).
>
>Any takers ?
>
>Nicolin, I think with that you can move forward with the TI INA chip
>implementation. I agree that 'samples' would be most appropriate for
>this chip.

I will try implementing this the way you suggested and resubmit this
soon.

Krzysztof