[PATCH v2 1/3] pmbus: introduce PMBUS_VIRT_*_SAMPLES registers

[PATCH v2 1/3] 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>
---
 Documentation/hwmon/sysfs-interface |  18 +++++
 drivers/hwmon/pmbus/pmbus.h         |  15 ++++
 drivers/hwmon/pmbus/pmbus_core.c    | 114 ++++++++++++++++++++++++++++
 3 files changed, 147 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
 -------------------------------------
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..31b6bf49518c 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -1901,6 +1901,116 @@ 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, (S_IWUSR | S_IRUGO),
+			    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 page;
+
+	for (page = 0; page < info->pages; page++) {
+		int s;
+
+		if (!(info->func[page] & PMBUS_HAVE_SAMPLES))
+			continue;
+
+		for (s = 0; s < ARRAY_SIZE(pmbus_samples_registers); s++) {
+			int ret;
+			struct pmbus_samples_attr *attr;
+
+			attr = &pmbus_samples_registers[s];
+			if (!pmbus_check_word_register(client, page, attr->reg))
+				continue;
+
+			ret = pmbus_add_samples_attr(data, page, attr);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int pmbus_find_attributes(struct i2c_client *client,
 				 struct pmbus_data *data)
 {
@@ -1932,6 +2042,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

Re: [PATCH v2 1/3] pmbus: introduce PMBUS_VIRT_*_SAMPLES registers

[Guenter Roeck]

On 4/13/19 9:03 AM, 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>
> ---
>   Documentation/hwmon/sysfs-interface |  18 +++++
>   drivers/hwmon/pmbus/pmbus.h         |  15 ++++
>   drivers/hwmon/pmbus/pmbus_core.c    | 114 ++++++++++++++++++++++++++++
>   3 files changed, 147 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
> +
>   

Separate patch for the above, please. Adding the ABI is one subject, adding support
for it into a driver (or driver core) is another.


>   sysfs attribute writes interpretation
>   -------------------------------------
> 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..31b6bf49518c 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -1901,6 +1901,116 @@ 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, (S_IWUSR | S_IRUGO),

S_IWUSR ran out of favor and generates a checkpatch warning.
Please use 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 page;
> +
> +	for (page = 0; page < info->pages; page++) {

This won't work as intended on multi-page chips. If the driver writer
is not careful, it will try to generate multiple similar attributes,
one per page. I would suggest to only check this for page 0.
At some point we may need to add a separate func pointer for
chip specific attributes, but that should do for now.

> +		int s;:
> +
> +		if (!(info->func[page] & PMBUS_HAVE_SAMPLES))
> +			continue;
> +
> +		for (s = 0; s < ARRAY_SIZE(pmbus_samples_registers); s++) {
> +			int ret;
> +			struct pmbus_samples_attr *attr;
> +

Please use reverse christmas tree here

			struct pmbus_samples_attr *attr;
			int ret;

> +			attr = &pmbus_samples_registers[s];
> +			if (!pmbus_check_word_register(client, page, attr->reg))
> +				continue;
> +
> +			ret = pmbus_add_samples_attr(data, page, attr);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static int pmbus_find_attributes(struct i2c_client *client,
>   				 struct pmbus_data *data)
>   {
> @@ -1932,6 +2042,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 v2 1/3] pmbus: introduce PMBUS_VIRT_*_SAMPLES registers

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

On Sun, Apr 14, 2019 at 07:27:47AM -0700, Guenter Roeck wrote:
>On 4/13/19 9:03 AM, 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>
>>---
>>  Documentation/hwmon/sysfs-interface |  18 +++++
>>  drivers/hwmon/pmbus/pmbus.h         |  15 ++++
>>  drivers/hwmon/pmbus/pmbus_core.c    | 114 ++++++++++++++++++++++++++++
>>  3 files changed, 147 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
>>+
>
>Separate patch for the above, please. Adding the ABI is one subject, adding support
>for it into a driver (or driver core) is another.

Sure, I'll do that.

>
>>  sysfs attribute writes interpretation
>>  -------------------------------------
>>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..31b6bf49518c 100644
>>--- a/drivers/hwmon/pmbus/pmbus_core.c
>>+++ b/drivers/hwmon/pmbus/pmbus_core.c
>>@@ -1901,6 +1901,116 @@ 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, (S_IWUSR | S_IRUGO),
>
>S_IWUSR ran out of favor and generates a checkpatch warning.
>Please use 0644.

I wasn't sure if its more important to care about checkpatch warning or
to keep convention in the file. I'll change to numerical value as
advised.
>
>>+			    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 page;
>>+
>>+	for (page = 0; page < info->pages; page++) {
>
>This won't work as intended on multi-page chips. If the driver writer
>is not careful, it will try to generate multiple similar attributes,
>one per page. I would suggest to only check this for page 0.
>At some point we may need to add a separate func pointer for
>chip specific attributes, but that should do for now.
>
Good point. I'll do that.

>>+		int s;:
>>+
>>+		if (!(info->func[page] & PMBUS_HAVE_SAMPLES))
>>+			continue;
>>+
>>+		for (s = 0; s < ARRAY_SIZE(pmbus_samples_registers); s++) {
>>+			int ret;
>>+			struct pmbus_samples_attr *attr;
>>+
>
>Please use reverse christmas tree here
>
>			struct pmbus_samples_attr *attr;
>			int ret;

Ok.

>>+			attr = &pmbus_samples_registers[s];
>>+			if (!pmbus_check_word_register(client, page, attr->reg))
>>+				continue;
>>+
>>+			ret = pmbus_add_samples_attr(data, page, attr);
>>+			if (ret)
>>+				return ret;
>>+		}
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>  static int pmbus_find_attributes(struct i2c_client *client,
>>  				 struct pmbus_data *data)
>>  {
>>@@ -1932,6 +2042,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;
>>  }
>>
>

Krzysztof

Re: [PATCH v2 1/3] pmbus: introduce PMBUS_VIRT_*_SAMPLES registers

[Guenter Roeck]

On 4/14/19 12:08 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> On Sun, Apr 14, 2019 at 07:27:47AM -0700, Guenter Roeck wrote:
>> On 4/13/19 9:03 AM, 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>
>>> ---
>>>   Documentation/hwmon/sysfs-interface |  18 +++++
>>>   drivers/hwmon/pmbus/pmbus.h         |  15 ++++
>>>   drivers/hwmon/pmbus/pmbus_core.c    | 114 ++++++++++++++++++++++++++++
>>>   3 files changed, 147 insertions(+)
>>>

[ ... ]

>>> +
>>> +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, (S_IWUSR | S_IRUGO),
>>
>> S_IWUSR ran out of favor and generates a checkpatch warning.
>> Please use 0644.
> 
> I wasn't sure if its more important to care about checkpatch warning or
> to keep convention in the file. I'll change to numerical value as
> advised.

My plan was to convert all remaining S_xxx in hwmon drivers to numericals
in one go, probably right after the next release. I did have the patch for
this file ready, actually. I just sent it out for review, and added you
to Cc:.

Guenter

[PATCH v2 2/3] 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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
index 53db78753a0d..b560d33db459 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,10 @@ 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);
+		ret = 1 << ret;
+		break;
 	default:
 		ret = -ENODATA;
 		break;
@@ -398,6 +406,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, 0, 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 +451,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

Re: [PATCH v2 2/3] lm25066: support SAMPLES_FOR_AVG register

[Guenter Roeck]

On 4/13/19 9:03 AM, 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>
> ---
>   drivers/hwmon/pmbus/lm25066.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
> index 53db78753a0d..b560d33db459 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,10 @@ 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)
			return ret;

> +		ret = 1 << ret;
> +		break;
>   	default:
>   		ret = -ENODATA;
>   		break;
> @@ -398,6 +406,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, 0, LM25066_SAMPLES_FOR_AVG_MAX);

0 is not valid, and ilog2(0) returns -1 (or is undefined).

> +		ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
> +					    ilog2(word));
> +		break;
>   	default:
>   		ret = -ENODATA;
>   		break;
> @@ -438,7 +451,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 v2 2/3] lm25066: support SAMPLES_FOR_AVG register

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

On Sun, Apr 14, 2019 at 07:37:35AM -0700, Guenter Roeck wrote:
>On 4/13/19 9:03 AM, 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>
>>---
>>  drivers/hwmon/pmbus/lm25066.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>>diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
>>index 53db78753a0d..b560d33db459 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,10 @@ 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)
>			return ret;
>

Good point. However the convention for this switch is to break instead
of returning early. I would stick with that if you don't mind.

>>+		ret = 1 << ret;
>>+		break;
>>  	default:
>>  		ret = -ENODATA;
>>  		break;
>>@@ -398,6 +406,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, 0, LM25066_SAMPLES_FOR_AVG_MAX);
>
>0 is not valid, and ilog2(0) returns -1 (or is undefined).

True, I'll clamp with minimum 1 to fix that. I don't think it's worth
returning an error for 0. Do you?

>
>>+		ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
>>+					    ilog2(word));
>>+		break;
>>  	default:
>>  		ret = -ENODATA;
>>  		break;
>>@@ -438,7 +451,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;
>>
>

Krzysztof

Re: [PATCH v2 2/3] lm25066: support SAMPLES_FOR_AVG register

[Guenter Roeck]

On 4/14/19 12:00 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> On Sun, Apr 14, 2019 at 07:37:35AM -0700, Guenter Roeck wrote:
>> On 4/13/19 9:03 AM, 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>
>>> ---
>>>   drivers/hwmon/pmbus/lm25066.c | 15 ++++++++++++++-
>>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
>>> index 53db78753a0d..b560d33db459 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,10 @@ 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)
>> 			return ret;
>>
> 
> Good point. However the convention for this switch is to break instead
> of returning early. I would stick with that if you don't mind.
> 

I don't mind if you want to use
		if (ret < 0)
			break;

instead, but I do mind if you want to convert an error code into an undefined value.

>>> +		ret = 1 << ret;
>>> +		break;
>>>   	default:
>>>   		ret = -ENODATA;
>>>   		break;
>>> @@ -398,6 +406,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, 0, LM25066_SAMPLES_FOR_AVG_MAX);
>>
>> 0 is not valid, and ilog2(0) returns -1 (or is undefined).
> 
> True, I'll clamp with minimum 1 to fix that. I don't think it's worth
> returning an error for 0. Do you?
> 

The whole point of using clamp() is to avoid returning an error. Returning an error
for values <= 0 but clamping large positive values to 4096 seems inconsistent.

Guenter

>>
>>> +		ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
>>> +					    ilog2(word));
>>> +		break;
>>>   	default:
>>>   		ret = -ENODATA;
>>>   		break;
>>> @@ -438,7 +451,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;
>>>
>>
> 
> Krzysztof
> 

[PATCH v2 3/3] 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 31b6bf49518c..3f94200bf793 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;
@@ -2011,6 +2014,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)
 {
@@ -2046,6 +2138,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;
 }
 
@@ -2460,6 +2556,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)) {
@@ -2482,6 +2580,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);
@@ -2494,6 +2593,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