Re: [PATCH v2] hwmon: (pmbus/adm1275) Fix power sampling support
On Thu, Jun 13, 2019 at 05:42:34AM -0700, Guenter Roeck wrote:
>Not every chip supported by this driver supports setting the number
>of samples for power averaging. Also, the power monitoring register
>is not always a 16-bit register, and the configuration bits used for
>voltage sampling are different depending on the register width.
>Some conditional code is needed to fix the problem.
>
>On top of all that, the compiler complains about problems with
>FIELD_GET and FIELD_PREP macros if the file is built with W=1.
>Avoid using those macros to silence the warning.
>
>Cc: Krzysztof Adamski
>Cc: Alexander Sverdlin
>Signed-off-by: Guenter Roeck
Reviewed-by: Krzysztof Adamski
>---
>v2: s/PRW/PWR/g
>Drop inline from function declarations
>
> drivers/hwmon/pmbus/adm1275.c | 84 +--
> 1 file changed, 65 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
>index a477ce0474bb..5caa37fbfc18 100644
>--- a/drivers/hwmon/pmbus/adm1275.c
>+++ b/drivers/hwmon/pmbus/adm1275.c
>@@ -71,9 +71,17 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278,
>adm1293, adm1294 };
> #define ADM1075_VAUX_OV_WARN BIT(7)
> #define ADM1075_VAUX_UV_WARN BIT(6)
>
>-#define ADM1275_PWR_AVG_MASK GENMASK(13, 11)
>-#define ADM1275_VI_AVG_MASK GENMASK(10, 8)
>-#define ADM1275_SAMPLES_AVG_MAX 128
>+#define ADM1275_VI_AVG_SHIFT 0
>+#define ADM1275_VI_AVG_MASK GENMASK(ADM1275_VI_AVG_SHIFT + 2, \
>+ ADM1275_VI_AVG_SHIFT)
>+#define ADM1275_SAMPLES_AVG_MAX 128
>+
>+#define ADM1278_PWR_AVG_SHIFT 11
>+#define ADM1278_PWR_AVG_MASK GENMASK(ADM1278_PWR_AVG_SHIFT + 2, \
>+ ADM1278_PWR_AVG_SHIFT)
>+#define ADM1278_VI_AVG_SHIFT 8
>+#define ADM1278_VI_AVG_MASK GENMASK(ADM1278_VI_AVG_SHIFT + 2, \
>+ ADM1278_VI_AVG_SHIFT)
>
> struct adm1275_data {
> int id;
>@@ -86,6 +94,7 @@ struct adm1275_data {
> bool have_pin_min;
> bool have_pin_max;
> bool have_temp_max;
>+ bool have_power_sampling;
> struct pmbus_driver_info info;
> };
>
>@@ -161,28 +170,58 @@ static const struct coefficients adm1293_coefficients[]
>= {
> [18] = { 7658, 0, -3 }, /* power, 21V, irange200 */
> };
>
>-static inline int adm1275_read_pmon_config(struct i2c_client *client, u16
>mask)
>+static int adm1275_read_pmon_config(const struct adm1275_data *data,
>+ struct i2c_client *client, bool is_power)
> {
>- int ret;
>-
>- ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>+ int shift, ret;
>+ u16 mask;
>+
>+ /*
>+ * The PMON configuration register is a 16-bit register only on chips
>+ * supporting power average sampling. On other chips it is an 8-bit
>+ * register.
>+ */
>+ if (data->have_power_sampling) {
>+ ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>+ mask = is_power ? ADM1278_PWR_AVG_MASK : ADM1278_VI_AVG_MASK;
>+ shift = is_power ? ADM1278_PWR_AVG_SHIFT : ADM1278_VI_AVG_SHIFT;
>+ } else {
>+ ret = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
>+ mask = ADM1275_VI_AVG_MASK;
>+ shift = ADM1275_VI_AVG_SHIFT;
>+ }
> if (ret < 0)
> return ret;
>
>- return FIELD_GET(mask, (u16)ret);
>+ return (ret & mask) >> shift;
> }
>
>-static inline int adm1275_write_pmon_config(struct i2c_client *client, u16
>mask,
>- u16 word)
>+static int adm1275_write_pmon_config(const struct adm1275_data *data,
>+ struct i2c_client *client,
>+ bool is_power, u16 word)
> {
>- int ret;
>-
>- ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>+ int shift, ret;
>+ u16 mask;
>+
>+ if (data->have_power_sampling) {
>+ ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>+ mask = is_power ? ADM1278_PWR_AVG_MASK : ADM1278_VI_AVG_MASK;
>+ shift = is_power ? ADM1278_PWR_AVG_SHIFT : ADM1278_VI_AVG_SHIFT;
>+ } else {
>+ ret = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
>+ mask = ADM1275_VI_AVG_MASK;
>+ shift = ADM1275_VI_AVG_SHIFT;
>+ }
> if (ret < 0)
> return ret;
>
>- word = FIELD_PREP(mask, word) | (ret & ~mask);
>- ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word);
>+ word = (ret & ~mask) | ((word << shift) & mask);
>+ if (data->have_power_sampling)
>+ ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG,
>+ word);
>+
Re: [PATCH] hwmon: (pmbus/adm1275) Fix power sampling support
On Wed, Jun 12, 2019 at 07:58:24AM -0700, Guenter Roeck wrote:
>Not every chip supported by this driver supports setting the number
>of samples for power averaging. Also, the power monitoring register
>is not always a 16-bit register, and the configuration bits used for
>voltage sampling are different depending on the register width.
>Some conditional code is needed to fix the problem.
>
>On top of all that, the compiler complains about problems with
>FIELD_GET and FIELD_PREP macros if the file is built with W=1.
>Avoid using those macros to silence the warning.
>
>Cc: Krzysztof Adamski
>Cc: Alexander Sverdlin
>Signed-off-by: Guenter Roeck
>---
> drivers/hwmon/pmbus/adm1275.c | 84 +--
> 1 file changed, 66 insertions(+), 18 deletions(-)
>
>diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
>index a477ce0474bb..b23c2dd95893 100644
>--- a/drivers/hwmon/pmbus/adm1275.c
>+++ b/drivers/hwmon/pmbus/adm1275.c
>@@ -71,9 +71,17 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278,
>adm1293, adm1294 };
> #define ADM1075_VAUX_OV_WARN BIT(7)
> #define ADM1075_VAUX_UV_WARN BIT(6)
>
>-#define ADM1275_PWR_AVG_MASK GENMASK(13, 11)
>-#define ADM1275_VI_AVG_MASK GENMASK(10, 8)
>-#define ADM1275_SAMPLES_AVG_MAX 128
>+#define ADM1275_VI_AVG_SHIFT 0
>+#define ADM1275_VI_AVG_MASK GENMASK(ADM1275_VI_AVG_SHIFT + 2, \
>+ ADM1275_VI_AVG_SHIFT)
>+#define ADM1275_SAMPLES_AVG_MAX 128
>+
>+#define ADM1278_PRW_AVG_SHIFT 11
>+#define ADM1278_PWR_AVG_MASK GENMASK(ADM1278_PRW_AVG_SHIFT + 2, \
>+ ADM1278_PRW_AVG_SHIFT)
s/PRW/PWR/
>+#define ADM1278_VI_AVG_SHIFT 8
>+#define ADM1278_VI_AVG_MASK GENMASK(ADM1278_VI_AVG_SHIFT + 2, \
>+ ADM1278_VI_AVG_SHIFT)
>
> struct adm1275_data {
> int id;
>@@ -86,6 +94,7 @@ struct adm1275_data {
> bool have_pin_min;
> bool have_pin_max;
> bool have_temp_max;
>+ bool have_power_sampling;
> struct pmbus_driver_info info;
> };
>
>@@ -161,28 +170,60 @@ static const struct coefficients adm1293_coefficients[]
>= {
> [18] = { 7658, 0, -3 }, /* power, 21V, irange200 */
> };
>
>-static inline int adm1275_read_pmon_config(struct i2c_client *client, u16
>mask)
>+static inline int adm1275_read_pmon_config(const struct adm1275_data *data,
This function does not have to be inline anymore. It had to be inline
before because of FIELD_GET but this is not used anymore.
>+ struct i2c_client *client,
>+ bool is_power)
> {
>- int ret;
>-
>- ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>+ int shift, ret;
>+ u16 mask;
>+
>+ /*
>+ * The PMON configuration register is a 16-bit register only on chips
>+ * supporting power average sampling. On other chips it is an 8-bit
>+ * register.
>+ */
>+ if (data->have_power_sampling) {
>+ ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>+ mask = is_power ? ADM1278_PWR_AVG_MASK : ADM1278_VI_AVG_MASK;
>+ shift = is_power ? ADM1278_PRW_AVG_SHIFT : ADM1278_VI_AVG_SHIFT;
>+ } else {
>+ ret = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
>+ mask = ADM1275_VI_AVG_MASK;
>+ shift = ADM1275_VI_AVG_SHIFT;
>+ }
> if (ret < 0)
> return ret;
>
>- return FIELD_GET(mask, (u16)ret);
>+ return (ret & mask) >> shift;
> }
>
>-static inline int adm1275_write_pmon_config(struct i2c_client *client, u16
>mask,
>+static inline int adm1275_write_pmon_config(const struct adm1275_data *data,
>+ struct i2c_client *client,
>+ bool is_power,
> u16 word)
Same comment about inline here.
> {
>- int ret;
>-
>- ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>+ int shift, ret;
>+ u16 mask;
>+
>+ if (data->have_power_sampling) {
>+ ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>+ mask = is_power ? ADM1278_PWR_AVG_MASK : ADM1278_VI_AVG_MASK;
>+ shift = is_power ? ADM1278_PRW_AVG_SHIFT : ADM1278_VI_AVG_SHIFT;
>+ } else {
>+ ret = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
>+ mask = ADM1275_VI_AVG_MASK;
>+ shift = ADM1275_VI_AVG_SHIFT;
>+ }
> if (ret < 0)
> return ret;
>
>- word = FIELD_PREP(mask, word) | (ret & ~mask);
>- ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word);
>+ word = (ret & ~mask) | ((word << shift) & mask);
>+ if
Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock
On Thu, May 30, 2019 at 10:21:20AM -0700, Guenter Roeck wrote: >Hi, > >On Thu, May 30, 2019 at 06:45:48AM +, Adamski, Krzysztof (Nokia - >PL/Wroclaw) wrote: >> The operation done in the pmbus_update_fan() function is a >> read-modify-write operation but it lacks any kind of lock protection >> which may cause problems if run more than once simultaneously. This >> patch uses an existing update_lock mutex to fix this problem. >> >> Signed-off-by: Krzysztof Adamski >> --- >> >> I'm resending this patch to proper recipients this time. Sorry if the >> previous submission confused anybody. >> >> drivers/hwmon/pmbus/pmbus_core.c | 11 --- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/hwmon/pmbus/pmbus_core.c >> b/drivers/hwmon/pmbus/pmbus_core.c >> index ef7ee90ee785..94adbede7912 100644 >> --- a/drivers/hwmon/pmbus/pmbus_core.c >> +++ b/drivers/hwmon/pmbus/pmbus_core.c >> @@ -268,6 +268,7 @@ int pmbus_update_fan(struct i2c_client *client, int >> page, int id, >> int rv; >> u8 to; >> >> +mutex_lock(>update_lock); >> from = pmbus_read_byte_data(client, page, >> pmbus_fan_config_registers[id]); >> if (from < 0) >> @@ -278,11 +279,15 @@ int pmbus_update_fan(struct i2c_client *client, int >> page, int id, >> rv = pmbus_write_byte_data(client, page, >> pmbus_fan_config_registers[id], to); >> if (rv < 0) >> -return rv; >> +goto out; >> } >> >> -return _pmbus_write_word_data(client, page, >> - pmbus_fan_command_registers[id], command); >> +rv = _pmbus_write_word_data(client, page, >> +pmbus_fan_command_registers[id], command); >> + >> +out: >> +mutex_lock(>update_lock); > >Should be mutex_unlock(), meaning you have not tested this ;-). > >Either case, I think this is unnecessary. The function is (or should be) >always called with the lock already taken (ie with pmbus_set_sensor() >in the call path). If not, we would need a locked and an unlocked version >of this function to avoid lock recursion. You've got me :) I did not test that as I do not have a workflow using this. I just stumbled opon this when looking at the code related to my other patches. So it was more like a - "hey, shouldn't there be a lock here?". But I was wrong, thanks. Krzysztof
[PATCH] hwmon: pmbus: protect read-modify-write with lock
The operation done in the pmbus_update_fan() function is a read-modify-write operation but it lacks any kind of lock protection which may cause problems if run more than once simultaneously. This patch uses an existing update_lock mutex to fix this problem. Signed-off-by: Krzysztof Adamski --- I'm resending this patch to proper recipients this time. Sorry if the previous submission confused anybody. drivers/hwmon/pmbus/pmbus_core.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index ef7ee90ee785..94adbede7912 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -268,6 +268,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id, int rv; u8 to; + mutex_lock(>update_lock); from = pmbus_read_byte_data(client, page, pmbus_fan_config_registers[id]); if (from < 0) @@ -278,11 +279,15 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id, rv = pmbus_write_byte_data(client, page, pmbus_fan_config_registers[id], to); if (rv < 0) - return rv; + goto out; } - return _pmbus_write_word_data(client, page, - pmbus_fan_command_registers[id], command); + rv = _pmbus_write_word_data(client, page, + pmbus_fan_command_registers[id], command); + +out: + mutex_lock(>update_lock); + return rv; } EXPORT_SYMBOL_GPL(pmbus_update_fan); -- 2.20.1
[PATCH v2 2/2] adm1275: support PMBUS_VIRT_*_SAMPLES
The device supports setting the number of samples for averaging the
measurements. There are two separate settings - PWR_AVG for averaging
PIN and VI_AVG for averaging VIN/VAUX/IOUT, both being part of
PMON_CONFIG register. The values are stored as exponent of base 2 of the
actual number of samples that will be taken.
Signed-off-by: Krzysztof Adamski
---
Changes in v2:
- Moved mutex lock to pmbus_set_samples (see patch 1/2)
- Changed mask type passed as argument to adm1275_read_pmon_config and
adm1275_write_pmon_config to u16
- Changed 1 << ret to BIT(ret)
drivers/hwmon/pmbus/adm1275.c | 61 ++-
1 file changed, 60 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
index f569372c9204..0c3493fa53ea 100644
--- a/drivers/hwmon/pmbus/adm1275.c
+++ b/drivers/hwmon/pmbus/adm1275.c
@@ -23,6 +23,8 @@
#include
#include
#include
+#include
+#include
#include "pmbus.h"
enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294 };
@@ -78,6 +80,10 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278,
adm1293, adm1294 };
#define ADM1075_VAUX_OV_WARN BIT(7)
#define ADM1075_VAUX_UV_WARN BIT(6)
+#define ADM1275_PWR_AVG_MASK GENMASK(13, 11)
+#define ADM1275_VI_AVG_MASKGENMASK(10, 8)
+#define ADM1275_SAMPLES_AVG_MAX128
+
struct adm1275_data {
int id;
bool have_oc_fault;
@@ -164,6 +170,34 @@ static const struct coefficients adm1293_coefficients[] = {
[18] = { 7658, 0, -3 }, /* power, 21V, irange200 */
};
+static inline int adm1275_read_pmon_config(struct i2c_client *client, u16 mask)
+{
+ int ret;
+
+ ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
+ if (ret < 0)
+ return ret;
+
+ return FIELD_GET(mask, (u16)ret);
+}
+
+static inline int adm1275_write_pmon_config(struct i2c_client *client, u16
mask,
+ u16 word)
+{
+ const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+ struct adm1275_data *data = to_adm1275_data(info);
+ int ret;
+
+ ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
+ if (ret < 0)
+ return ret;
+
+ word = FIELD_PREP(mask, word) | (ret & ~mask);
+ ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word);
+
+ return ret;
+}
+
static int adm1275_read_word_data(struct i2c_client *client, int page, int reg)
{
const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
@@ -242,6 +276,19 @@ static int adm1275_read_word_data(struct i2c_client
*client, int page, int reg)
if (!data->have_temp_max)
return -ENXIO;
break;
+ case PMBUS_VIRT_POWER_SAMPLES:
+ ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK);
+ if (ret < 0)
+ break;
+ ret = BIT(ret);
+ break;
+ case PMBUS_VIRT_IN_SAMPLES:
+ case PMBUS_VIRT_CURR_SAMPLES:
+ ret = adm1275_read_pmon_config(client, ADM1275_VI_AVG_MASK);
+ if (ret < 0)
+ break;
+ ret = BIT(ret);
+ break;
default:
ret = -ENODATA;
break;
@@ -286,6 +333,17 @@ static int adm1275_write_word_data(struct i2c_client
*client, int page, int reg,
case PMBUS_VIRT_RESET_TEMP_HISTORY:
ret = pmbus_write_word_data(client, 0, ADM1278_PEAK_TEMP, 0);
break;
+ case PMBUS_VIRT_POWER_SAMPLES:
+ word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX);
+ ret = adm1275_write_pmon_config(client, ADM1275_PWR_AVG_MASK,
+ ilog2(word));
+ break;
+ case PMBUS_VIRT_IN_SAMPLES:
+ case PMBUS_VIRT_CURR_SAMPLES:
+ word = clamp_val(word, 1, ADM1275_SAMPLES_AVG_MAX);
+ ret = adm1275_write_pmon_config(client, ADM1275_VI_AVG_MASK,
+ ilog2(word));
+ break;
default:
ret = -ENODATA;
break;
@@ -439,7 +497,8 @@ static int adm1275_probe(struct i2c_client *client,
info->format[PSC_CURRENT_OUT] = direct;
info->format[PSC_POWER] = direct;
info->format[PSC_TEMPERATURE] = direct;
- info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT;
+ info->func[0] = PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
+ PMBUS_HAVE_SAMPLES;
info->read_word_data = adm1275_read_word_data;
info->read_byte_data = adm1275_read_byte_data;
--
2.20.1
[PATCH v2 1/2] pmbus: (core) mutex_lock write in pmbus_set_samples
update_lock is a mutex intended to protect write operations. It was not taken, however, when _pmbus_write_word_data is called from pmbus_set_samples() function which may cause problems especially when some PMBUS_VIRT_* operation is implemented as a read-modify-write cycle. This patch makes sure the lock is held during the operation. Signed-off-by: Krzysztof Adamski --- drivers/hwmon/pmbus/pmbus_core.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index ef7ee90ee785..62cd213cc260 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -1946,7 +1946,9 @@ static ssize_t pmbus_set_samples(struct device *dev, if (kstrtol(buf, 0, ) < 0) return -EINVAL; + mutex_lock(>update_lock); ret = _pmbus_write_word_data(client, reg->page, reg->attr->reg, val); + mutex_unlock(>update_lock); return ret ? : count; } -- 2.20.1
Re: [PATCH] adm1275: support PMBUS_VIRT_*_SAMPLES
On Wed, May 29, 2019 at 05:17:47AM -0700, Guenter Roeck wrote:
>On 5/29/19 12:11 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>>On Tue, May 28, 2019 at 12:46:52PM -0700, Guenter Roeck wrote:
>>>On Fri, May 24, 2019 at 12:49:13PM +, Adamski, Krzysztof (Nokia -
>>>PL/Wroclaw) wrote:
>>>>The device supports setting the number of samples for averaging the
>>>>measurements. There are two separate settings - PWR_AVG for averaging
>>>>PIN and VI_AVG for averaging VIN/VAUX/IOUT, both being part of
>>>>PMON_CONFIG register. The values are stored as exponent of base 2 of the
>>>>actual number of samples that will be taken.
>>>>
>>>>Signed-off-by: Krzysztof Adamski
>>>>---
>>>> drivers/hwmon/pmbus/adm1275.c | 68 ++-
>>>> 1 file changed, 67 insertions(+), 1 deletion(-)
>>>>
>>>>diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
>>>>index f569372c9204..4efe1a9df563 100644
>>>>--- a/drivers/hwmon/pmbus/adm1275.c
>>>>+++ b/drivers/hwmon/pmbus/adm1275.c
>>>>@@ -23,6 +23,8 @@
>>>> #include
>>>> #include
>>>> #include
>>>>+#include
>>>>+#include
>>>> #include "pmbus.h"
>>>>
>>>> enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293,
>>>> adm1294 };
>>>>@@ -78,6 +80,10 @@ enum chips { adm1075, adm1272, adm1275, adm1276,
>>>>adm1278, adm1293, adm1294 };
>>>> #define ADM1075_VAUX_OV_WARN BIT(7)
>>>> #define ADM1075_VAUX_UV_WARN BIT(6)
>>>>
>>>>+#define ADM1275_PWR_AVG_MASK GENMASK(13, 11)
>>>>+#define ADM1275_VI_AVG_MASKGENMASK(10, 8)
>>>>+#define ADM1275_SAMPLES_AVG_MAX128
>>>>+
>>>> struct adm1275_data {
>>>>int id;
>>>>bool have_oc_fault;
>>>>@@ -90,6 +96,7 @@ struct adm1275_data {
>>>>bool have_pin_max;
>>>>bool have_temp_max;
>>>>struct pmbus_driver_info info;
>>>>+ struct mutex lock;
>>>> };
>>>>
>>>> #define to_adm1275_data(x) container_of(x, struct adm1275_data, info)
>>>>@@ -164,6 +171,38 @@ static const struct coefficients
>>>>adm1293_coefficients[] = {
>>>>[18] = { 7658, 0, -3 }, /* power, 21V, irange200 */
>>>> };
>>>>
>>>>+static inline int adm1275_read_pmon_config(struct i2c_client *client, u64
>>>>mask)
>>>
>>>Why is the mask passed through as u64 ?
>>
>>Good point. I used u64 as this is the type used by bitfield machinery
>>under the hood but I agree it doesn't make sense and is even confusing
>>to have this in the function prototype as we are using this to mask 16
>>bit word anyways. I will fix that in v2. I am gonna have to cast the ret
>>to u16 when passing to FIELD_GET() to make sure the __BF_FIELD_CHECK is
>>not complaining (since it is signed right now), though.
>>
>
>Not sure I understand what you are talking about. FIELD_GET() uses typeof().
>FIELD_GET() is used by other callers even with u8 and without any typecasts.
>Why would it be a problem here ?
So I basically agree with you but just wanted to note why there will be
additional cast needed in my code. The:
return FIELD_GET(mask, ret);
will be changed to:
return FIELD_GET(mask, (u16)ret);
And the reason for that is that the __BF_FIELD_CHECK does this check at
compile time (and breaks if this is true)
(_mask) > (typeof(_reg))~0ull
In my case typeof(_reg) is int, so (typeof(_reg))~0ull = -1 which is
signed. _mask is unsigned. Depending on the type promotion, this might
or might not be true depending on the size of _mask. When _mask was u64,
it always worked. For _mask being u16, it will fail. For u32, this will
fail depending on if we are compiling for 32 or 64 bit architecture.
All this might be obvious to you but it wasn't to me, thus this note.
>>>
>>>>+{
>>>>+ int ret;
>>>>+
>>>>+ ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>>>>+ if (ret < 0)
>>>>+ return ret;
>>>>+
>>>>+ return FIELD_GET(mask, ret);
>>>>+}
>>>>+
>>>>+static inline int adm1275_write_pmon_config(struct i2c_client *client, u64
>>>>mask,
>>>>+ u16 word)
>>>>+{
>>>>+ const struct pmbus_
Re: [PATCH] adm1275: support PMBUS_VIRT_*_SAMPLES
On Tue, May 28, 2019 at 12:46:52PM -0700, Guenter Roeck wrote:
>On Fri, May 24, 2019 at 12:49:13PM +0000, Adamski, Krzysztof (Nokia -
>PL/Wroclaw) wrote:
>> The device supports setting the number of samples for averaging the
>> measurements. There are two separate settings - PWR_AVG for averaging
>> PIN and VI_AVG for averaging VIN/VAUX/IOUT, both being part of
>> PMON_CONFIG register. The values are stored as exponent of base 2 of the
>> actual number of samples that will be taken.
>>
>> Signed-off-by: Krzysztof Adamski
>> ---
>> drivers/hwmon/pmbus/adm1275.c | 68 ++-
>> 1 file changed, 67 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
>> index f569372c9204..4efe1a9df563 100644
>> --- a/drivers/hwmon/pmbus/adm1275.c
>> +++ b/drivers/hwmon/pmbus/adm1275.c
>> @@ -23,6 +23,8 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> #include "pmbus.h"
>>
>> enum chips { adm1075, adm1272, adm1275, adm1276, adm1278, adm1293, adm1294
>> };
>> @@ -78,6 +80,10 @@ enum chips { adm1075, adm1272, adm1275, adm1276, adm1278,
>> adm1293, adm1294 };
>> #define ADM1075_VAUX_OV_WARNBIT(7)
>> #define ADM1075_VAUX_UV_WARNBIT(6)
>>
>> +#define ADM1275_PWR_AVG_MASKGENMASK(13, 11)
>> +#define ADM1275_VI_AVG_MASK GENMASK(10, 8)
>> +#define ADM1275_SAMPLES_AVG_MAX 128
>> +
>> struct adm1275_data {
>> int id;
>> bool have_oc_fault;
>> @@ -90,6 +96,7 @@ struct adm1275_data {
>> bool have_pin_max;
>> bool have_temp_max;
>> struct pmbus_driver_info info;
>> +struct mutex lock;
>> };
>>
>> #define to_adm1275_data(x) container_of(x, struct adm1275_data, info)
>> @@ -164,6 +171,38 @@ static const struct coefficients adm1293_coefficients[]
>> = {
>> [18] = { 7658, 0, -3 }, /* power, 21V, irange200 */
>> };
>>
>> +static inline int adm1275_read_pmon_config(struct i2c_client *client, u64
>> mask)
>
>Why is the mask passed through as u64 ?
Good point. I used u64 as this is the type used by bitfield machinery
under the hood but I agree it doesn't make sense and is even confusing
to have this in the function prototype as we are using this to mask 16
bit word anyways. I will fix that in v2. I am gonna have to cast the ret
to u16 when passing to FIELD_GET() to make sure the __BF_FIELD_CHECK is
not complaining (since it is signed right now), though.
>
>> +{
>> +int ret;
>> +
>> +ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>> +if (ret < 0)
>> +return ret;
>> +
>> +return FIELD_GET(mask, ret);
>> +}
>> +
>> +static inline int adm1275_write_pmon_config(struct i2c_client *client, u64
>> mask,
>> +u16 word)
>> +{
>> +const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
>> +struct adm1275_data *data = to_adm1275_data(info);
>> +int ret;
>> +
>> +mutex_lock(>lock);
>
>Why is another lock on top of the lock provided by the pmbus core required ?
>
Good point, I was considering if I should instead add mutex_lock on
update_lock in the pmbus_set_samples() function inside of pmbus_core.c
instead (as this function is missing it) but figured that not all
devices will need that (lm25066 didn't) so it might be a waste in most
cases. But this may be cleaner approach indeed.
Is this what you mean or there is some other lock I missed?
>> +ret = i2c_smbus_read_word_data(client, ADM1275_PMON_CONFIG);
>> +if (ret < 0) {
>> +mutex_unlock(>lock);
>> +return ret;
>> +}
>> +
>> +word = FIELD_PREP(mask, word) | (ret & ~mask);
>> +ret = i2c_smbus_write_word_data(client, ADM1275_PMON_CONFIG, word);
>> +mutex_unlock(>lock);
>> +
>> +return ret;
>> +}
>> +
>> static int adm1275_read_word_data(struct i2c_client *client, int page, int
>> reg)
>> {
>> const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
>> @@ -242,6 +281,19 @@ static int adm1275_read_word_data(struct i2c_client
>> *client, int page, int reg)
>> if (!data->have_temp_max)
>> return -ENXIO;
>> break;
>> +case PMBUS_VIRT_POWER_SAMPLES:
>> +ret = adm1275_read_pmon_config(client, ADM1275_PWR_AVG_MASK);
>> +
Re: [PATCH v3 4/4] pmbus_core: export coefficients via sysfs
On Mon, Apr 15, 2019 at 09:33:10AM -0700, Guenter Roeck wrote: >We are talking an ABI here. ABIs are supposed to be hardware independent. >Directly mapping registers to attributes is not exactly hardware independent. That is true, I get that, even though I don't want to map registers. I want to map coefficients which are hardware independent but only as far as we take into account only PMBUS. And I guess this is the root of the difference in our opinions - I still only think about PMBUS devices while you think about all HWMON devices (I think). >> > and even misleading since chip programming isn't adjusted even when >> > that is possible. >> >> I don't understand what you mean by that, could you explain? >> >"when that is possible" -> on chips supporting calibration commands, >one should not manipulate b/m/R but write any adjustments into the chip >using those commands. > >> > >> >Addressing this problem in a generic way will require substantially >> >more thought than just adding a couple of pmbus direct mode specific >> >attributes. >> >> I'm willing to put more effort into that but I would need to better >> understand what is your view of the scope of problem here. >> >There are a number of elements. > >First, a case has to be made why the current mechanism of using >sensors3.conf for adjustments is insufficient. This may be well >understood by some, but the case needs to be made (ie some chips do >have HW registers to perform the adjustment, and in some cases there >is accuracy loss by performing adjustments in user space). My argument why you the solution of sensor3.conf can't be directly used is that the readings for PMBUS devices uses real values, that are already processed by some coefficient values. If you want to apply corrected coefficients, you would first need to know what default coeffs were used to "revese" the calculation. And the specification for devices I am working with (ADM1275 and LM25066 families) describe how to get such corrected coeffs so it is sensible to assume they might be available. >Second, we'll need to determine which standardized attributes are >needed. As far as I can see this would probably be 'scale' (or maybe >'gain') and 'offset'. The question is how to express especially 'scale'. >It would either require both a multiplier and a divider, or it would >have to be expressed in fixed point arithmetic. I would prefer the >latter, but I am open to suggestions. > >Third, we will have to determine how to apply this all to pmbus >chips. It must not only apply to direct mode chips, but to all >PMBus chips. Even for direct mode chips the case needs to be made >if the attributes should apply per sensor class or per sensor (I >would think per sensor). Just looking at VOUT, chapter 9.2 of the >PMBus specification (Rev 1.1, part II) gives an example of what >to look out for: Just for VOUT, there are VOUT_TRIM, VOUT_CAL_OFFSET, >VOUT_DROOP (does this require yet another attribute ?), and >VOUT_SCALE_LOOP. > >I should add that the above, for VOUT, are the primary means for >PMBus devices to adjust voltage readings. This also applies to IOUT, >which has similar standard PMBus registers available for calibration >(IOUT_CAL_GAIN and IOUT_CAL_OFFSET). On top of that, some chips support >vendor specific commands to calibrate other sensors. For example, >MAX15301 supports a EXT_TEMP_CAL command to calibrate temperature sensor >readings. > >Also note that direct mode PMBUs chips _should_ support a COEFFICIENTS >command to retrieve actual coefficients from the chip (though I don't >recall ever encountering a chip that actually supports it). > >Overall, adjusting readings through manipulation of b/m/R is at best >a workaround. > >As such, the chip we are talking about here does pretty much >everything wrong. I do understand the need for a more dynamic >calibration support on the driver level, but the solution must >not focus on such a chip and needs to be more generic. I need some more time to think about what you wrote here a little more to get the feeling about the most general solution. One thing that came into my mind, however, is that part of my problem would be solved if userspace could at least retrieve coeffs used to calcualte the values. Lets say the chip would support COEFFICIENTS command - how would we implement support for that? In the internal implementation, since all PMBUS devices should support COEFFICIENTS command, we could just add attribute for the PMBUS_COEFFICIENTS. Devices supporting this command, could just, well, pass this to device. Other could "intercept it" and just return values stored in the pmbus_driver_info structure. The only problem is - this command is _very_ generic - it lets you ask for separate coefficients for reading and writing and it can return different one for each of the supported PMBUS commands. This makes it impractical to probe for all supported combinations on probe. Some filtering would have to be provided by the client drivers instead.
Re: [PATCH v3 4/4] pmbus_core: export coefficients via sysfs
On Sun, Apr 14, 2019 at 07:38:33PM -0700, Guenter Roeck wrote: >On 4/14/19 3:37 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>On Sun, Apr 14, 2019 at 11:59:38PM +0200, Krzysztof Adamski wrote: >>>PMBUS devices report values in real-world units. Those using direct >>>format require conversion using standarised coefficients and formula. >>>This operation is already done by pmbus_core and the default values for >>>coefficients are configured by chip drivers. >>> >>>However those default values are not allways suitable as they depend on >>>the value of sense register and environment used. For that reason, in >>>order to get values that make sense or just to get best accuracy, in it >>>may be required to tweak coefficient values. The proper values may be >>>measured during device production (to get best accuracy, they might be >>>callibrated for each individual unit) and stored as calibration values >>>in some place (like EEPROM or even a file). >>> >>>To support wide range of possible use cases, lets export those to user >>>space via sysfs attributes so that it can implement its own policies >>>about them. All those attributes are put into separate attribute_group >>>struct so that we can set its name to group all of them in a >>>"coefficients" subdirectory in sysfs. >>> >>>Signed-off-by: Krzysztof Adamski >>>--- >>>drivers/hwmon/pmbus/pmbus_core.c | 104 ++- >>>1 file changed, 102 insertions(+), 2 deletions(-) >>> >> >>[...] >> >>>+pmbus_dev_attr_init(_attr->attr, name, (S_IWUSR | S_IRUGO), >> >>I screwed up and did not fix this also, sorry for that. I'm not sending >>an updated version (yet) as I didn't get your approval for the concept >>itself (though I still hope you will reconsider taking into account my >>latest arguments). >> > >I won't, sorry. As mentioned before, this is a generic problem that needs >to be solved for all hwmon drivers, not just for pmbus devices, and also >not just for pmbus devices using direct mode. > >Even worse, this doesn't even work for pmbus devices in general. >PMBus supports commands such as VOUT_SCALE_LOOP and various other >calibration commands such as VOUT_CAL_OFFSET, IOUT_CAL_GAIN and >IOUT_CAL_OFFSET. Your approach doesn't even try to support those >commands; you only look at r/m/b which may suit your needs but are >hardly generic I agree I don't see this problem as general as you. The mentioned registers can easily be supported if needed by just attaching attributes to them. That would allow doing things that cannot be done currently (configuring precision of the regulation of the devices) while I see coefficients problems as something totally different - more like a fix to the already supported functionality (reporting current measurements). > and even misleading since chip programming isn't adjusted even when > that is possible. I don't understand what you mean by that, could you explain? > >Addressing this problem in a generic way will require substantially >more thought than just adding a couple of pmbus direct mode specific >attributes. I'm willing to put more effort into that but I would need to better understand what is your view of the scope of problem here. Krzysztof
Re: [PATCH v3 4/4] pmbus_core: export coefficients via sysfs
On Sun, Apr 14, 2019 at 11:59:38PM +0200, Krzysztof Adamski wrote: >PMBUS devices report values in real-world units. Those using direct >format require conversion using standarised coefficients and formula. >This operation is already done by pmbus_core and the default values for >coefficients are configured by chip drivers. > >However those default values are not allways suitable as they depend on >the value of sense register and environment used. For that reason, in >order to get values that make sense or just to get best accuracy, in it >may be required to tweak coefficient values. The proper values may be >measured during device production (to get best accuracy, they might be >callibrated for each individual unit) and stored as calibration values >in some place (like EEPROM or even a file). > >To support wide range of possible use cases, lets export those to user >space via sysfs attributes so that it can implement its own policies >about them. All those attributes are put into separate attribute_group >struct so that we can set its name to group all of them in a >"coefficients" subdirectory in sysfs. > >Signed-off-by: Krzysztof Adamski >--- >drivers/hwmon/pmbus/pmbus_core.c | 104 ++- >1 file changed, 102 insertions(+), 2 deletions(-) > [...] >+ pmbus_dev_attr_init(_attr->attr, name, (S_IWUSR | S_IRUGO), I screwed up and did not fix this also, sorry for that. I'm not sending an updated version (yet) as I didn't get your approval for the concept itself (though I still hope you will reconsider taking into account my latest arguments). [...] > Krzysztof
Re: [PATCH] hwmon: (pmbus_core) Replace S_ with octal values
On Sun, Apr 14, 2019 at 02:57:24PM -0700, Guenter Roeck wrote: >Replace S_ with octal values. > >The conversion was done automatically with coccinelle. The semantic patches >and the scripts used to generate this commit log are available at >https://github.com/groeck/coccinelle-patches/hwmon/. The URL is 404. blob/master/hwmon instead of just hwmon at the end would work. >This patch does not introduce functional changes. It was verified by >compiling the old and new files and comparing text and data sizes. I don't think that the sizes would change even if there was some error introuced if it still compiles. But of course both the conccinele script is ok and I manually verified the patch. Also, git grep "\bS_" pmbus* does not return anything meaning all the places were converted. >Cc: Krzysztof Adamski >Signed-off-by: Guenter Roeck >--- > drivers/hwmon/pmbus/pmbus_core.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > >diff --git a/drivers/hwmon/pmbus/pmbus_core.c >b/drivers/hwmon/pmbus/pmbus_core.c >index 2e2b5851139c..e2366428a9a9 100644 >--- a/drivers/hwmon/pmbus/pmbus_core.c >+++ b/drivers/hwmon/pmbus/pmbus_core.c >@@ -1073,7 +1073,7 @@ static int pmbus_add_boolean(struct pmbus_data *data, >name, seq, type); > boolean->s1 = s1; > boolean->s2 = s2; >- pmbus_attr_init(a, boolean->name, S_IRUGO, pmbus_show_boolean, NULL, >+ pmbus_attr_init(a, boolean->name, 0444, pmbus_show_boolean, NULL, > (reg << 16) | mask); > > return pmbus_add_attribute(data, >dev_attr.attr); >@@ -1107,7 +1107,7 @@ static struct pmbus_sensor *pmbus_add_sensor(struct >pmbus_data *data, > sensor->update = update; > sensor->convert = convert; > pmbus_dev_attr_init(a, sensor->name, >- readonly ? S_IRUGO : S_IRUGO | S_IWUSR, >+ readonly ? 0444 : 0644, > pmbus_show_sensor, pmbus_set_sensor); > > if (pmbus_add_attribute(data, >attr)) >@@ -1139,7 +1139,7 @@ static int pmbus_add_label(struct pmbus_data *data, > snprintf(label->label, sizeof(label->label), "%s%d", lstring, >index); > >- pmbus_dev_attr_init(a, label->name, S_IRUGO, pmbus_show_label, NULL); >+ pmbus_dev_attr_init(a, label->name, 0444, pmbus_show_label, NULL); > return pmbus_add_attribute(data, >attr); > } > >-- >2.7.4 > Krzysztof
[PATCH v3 4/4] pmbus_core: export coefficients via sysfs
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
---
drivers/hwmon/pmbus/pmbus_core.c | 104 ++-
1 file changed, 102 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 9b7d493c98b9..38493617d6a7 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -57,6 +57,8 @@
#define PMBUS_NAME_SIZE24
+static const char * const coeff_name[] = {"m", "b", "R"};
+
struct pmbus_sensor {
struct pmbus_sensor *next;
char name[PMBUS_NAME_SIZE]; /* sysfs sensor name */
@@ -98,12 +100,13 @@ struct pmbus_data {
int exponent[PMBUS_PAGES];
/* linear mode: exponent for output voltages */
- const struct pmbus_driver_info *info;
+ struct pmbus_driver_info *info;
int max_attributes;
int num_attributes;
struct attribute_group group;
- const struct attribute_group *groups[2];
+ const struct attribute_group *groups[3];
+ struct attribute_group group_coeff;
struct dentry *debugfs; /* debugfs device directory */
struct pmbus_sensor *sensors;
@@ -2007,6 +2010,95 @@ static int pmbus_add_samples_attributes(struct
i2c_client *client,
return 0;
}
+static struct attribute *pmbus_init_coeff_attr(struct pmbus_data *data,
+ const char *prefix,
+ const char *coeff, int *target)
+{
+ struct dev_ext_attribute *ext_attr;
+ char *name;
+
+ ext_attr = devm_kzalloc(data->dev, sizeof(*ext_attr), GFP_KERNEL);
+ if (!ext_attr)
+ return NULL;
+
+ name = devm_kasprintf(data->dev, GFP_KERNEL, "%s_%s", prefix, coeff);
+ if (!name)
+ return NULL;
+
+ pmbus_dev_attr_init(_attr->attr, name, (S_IWUSR | S_IRUGO),
+ device_show_int, device_store_int);
+ ext_attr->var = target;
+
+ return _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, [n]);
+ if (ret < 0) {
+ kfree(attrs);
+ return ret;
+ }
+
+
[PATCH v3 3/4] lm25066: support SAMPLES_FOR_AVG register
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
---
drivers/hwmon/pmbus/lm25066.c | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
index 53db78753a0d..715d4ab57516 100644
--- a/drivers/hwmon/pmbus/lm25066.c
+++ b/drivers/hwmon/pmbus/lm25066.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#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_AVG0xdb
#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_MAX4096
+
/* LM25056 only */
#define LM25056_VAUX_OV_WARN_LIMIT 0xe3
@@ -284,6 +288,12 @@ static int lm25066_read_word_data(struct i2c_client
*client, int page, int reg)
case PMBUS_VIRT_RESET_PIN_HISTORY:
ret = 0;
break;
+ case PMBUS_VIRT_SAMPLES:
+ ret = pmbus_read_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG);
+ if (ret < 0)
+ break;
+ ret = 1 << ret;
+ break;
default:
ret = -ENODATA;
break;
@@ -398,6 +408,11 @@ static int lm25066_write_word_data(struct i2c_client
*client, int page, int reg,
case PMBUS_VIRT_RESET_PIN_HISTORY:
ret = pmbus_write_byte(client, 0, LM25066_CLEAR_PIN_PEAK);
break;
+ case PMBUS_VIRT_SAMPLES:
+ word = clamp_val(word, 1, LM25066_SAMPLES_FOR_AVG_MAX);
+ ret = pmbus_write_byte_data(client, 0, LM25066_SAMPLES_FOR_AVG,
+ ilog2(word));
+ break;
default:
ret = -ENODATA;
break;
@@ -438,7 +453,7 @@ static int lm25066_probe(struct i2c_client *client,
info->func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VMON
| PMBUS_HAVE_PIN | PMBUS_HAVE_IIN | PMBUS_HAVE_STATUS_INPUT
- | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
+ | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_SAMPLES;
if (data->id == lm25056) {
info->func[0] |= PMBUS_HAVE_STATUS_VMON;
--
2.20.1
[PATCH v3 2/4] hwmon: Document the samples attributes
Document new ABI attributes: {in,power,curr,temp}_samples and samples.
Signed-off-by: Krzysztof Adamski
---
Documentation/hwmon/sysfs-interface | 18 ++
1 file changed, 18 insertions(+)
diff --git a/Documentation/hwmon/sysfs-interface
b/Documentation/hwmon/sysfs-interface
index 2b9e1005d88b..7b91706d01c8 100644
--- a/Documentation/hwmon/sysfs-interface
+++ b/Documentation/hwmon/sysfs-interface
@@ -756,6 +756,24 @@ intrusion[0-*]_beep
1: enable
RW
+
+* Average sample configuration *
+
+
+Devices allowing for reading {in,power,curr,temp}_average values may export
+attributes for controlling number of samples used to compute average.
+
+samplesSets number of average samples for all types of
measurements.
+ RW
+
+in_samples
+power_samples
+curr_samples
+temp_samplesSets number of average samples for specific type of
measurements.
+ Note that on some devices it won't be possible to set all of
them
+ to different values so changing one might also change some
others.
+ RW
+
sysfs attribute writes interpretation
-
--
2.20.1
[PATCH v3 1/4] pmbus: introduce PMBUS_VIRT_*_SAMPLES registers
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
---
drivers/hwmon/pmbus/pmbus.h | 15 +
drivers/hwmon/pmbus/pmbus_core.c | 110 +++
2 files changed, 125 insertions(+)
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 1d24397d36ec..e73289cc867d 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -217,6 +217,20 @@ enum pmbus_regs {
PMBUS_VIRT_PWM_ENABLE_2,
PMBUS_VIRT_PWM_ENABLE_3,
PMBUS_VIRT_PWM_ENABLE_4,
+
+ /* Samples for average
+*
+* Drivers wanting to expose functionality for changing the number of
+* samples used for average values should implement support in
+* {read,write}_word_data callback for either PMBUS_VIRT_SAMPLES if it
+* applies to all types of measurements, or any number of specific
+* PMBUS_VIRT_*_SAMPLES registers to allow for individual control.
+*/
+ PMBUS_VIRT_SAMPLES,
+ PMBUS_VIRT_IN_SAMPLES,
+ PMBUS_VIRT_CURR_SAMPLES,
+ PMBUS_VIRT_POWER_SAMPLES,
+ PMBUS_VIRT_TEMP_SAMPLES,
};
/*
@@ -371,6 +385,7 @@ enum pmbus_sensor_classes {
#define PMBUS_HAVE_STATUS_VMON BIT(19)
#define PMBUS_HAVE_PWM12 BIT(20)
#define PMBUS_HAVE_PWM34 BIT(21)
+#define PMBUS_HAVE_SAMPLES BIT(22)
#define PMBUS_PAGE_VIRTUAL BIT(31)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 2e2b5851139c..9b7d493c98b9 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -1901,6 +1901,112 @@ static int pmbus_add_fan_attributes(struct i2c_client
*client,
return 0;
}
+struct pmbus_samples_attr {
+ int reg;
+ char *name;
+};
+
+struct pmbus_samples_reg {
+ int page;
+ struct pmbus_samples_attr *attr;
+ struct device_attribute dev_attr;
+};
+
+static struct pmbus_samples_attr pmbus_samples_registers[] = {
+ {
+ .reg = PMBUS_VIRT_SAMPLES,
+ .name = "samples",
+ }, {
+ .reg = PMBUS_VIRT_IN_SAMPLES,
+ .name = "in_samples",
+ }, {
+ .reg = PMBUS_VIRT_CURR_SAMPLES,
+ .name = "curr_samples",
+ }, {
+ .reg = PMBUS_VIRT_POWER_SAMPLES,
+ .name = "power_samples",
+ }, {
+ .reg = PMBUS_VIRT_TEMP_SAMPLES,
+ .name = "temp_samples",
+ }
+};
+
+#define to_samples_reg(x) container_of(x, struct pmbus_samples_reg, dev_attr)
+
+static ssize_t pmbus_show_samples(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ int val;
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct pmbus_samples_reg *reg = to_samples_reg(devattr);
+
+ val = _pmbus_read_word_data(client, reg->page, reg->attr->reg);
+ if (val < 0)
+ return val;
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static ssize_t pmbus_set_samples(struct device *dev,
+struct device_attribute *devattr,
+const char *buf, size_t count)
+{
+ int ret;
+ long val;
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct pmbus_samples_reg *reg = to_samples_reg(devattr);
+
+ if (kstrtol(buf, 0, ) < 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(>dev_attr, attr->name, 0644,
+ pmbus_show_samples, pmbus_set_samples);
+
+ return pmbus_add_attribute(data, >dev_attr.attr);
+}
+
+static int pmbus_add_samples_attributes(struct i2c_client *client,
+ struct pmbus_data *data)
+{
+ const struct pmbus_driver_info *info = data->info;
+ int s;
+
+ if (!(info->func[0] & PMBUS_HAVE_SAMPLES))
+ return 0;
+
+ for (s = 0; s < ARRAY_SIZE(pmbus_samples_registers); s++) {
+ struct pmbus_samples_attr *attr;
+ int ret;
+
+ attr = _samples_registers[s];
+ if (!pmbus_check_word_register(client, 0, attr->reg))
+ continue;
+
+ ret = pmbus_add_samples_attr(data, 0, attr);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
static
[PATCH v3 0/4] pmbus: extend configurability via sysfs
Hi, This patch series extends pmbus core for two specific use cases we have: - First three patches allows lm25066 driver to set number of samples for average values (by controlling manufacturer specific SAMPLES_FOR_AVG register). It is useful to be able to set this register when using any of the *_average registers, especially since the default value means we are averaging 1 sample which isn't that useful. - Third patch exports m, b, R coefficients so that they can be adjusted by user space. We can't use default coefficients values and in order to achieve high accuracy, we calibrate them per unit so using device-tree or similar concepts (which are generally shared by all board of the same type) to store them is not an option too. Also, using device-tree (the only was to influence coeffs now) might not be possible on some architectures (like on x86, for example). Thus, we export it so that the logic of loading proper coeffs can be implemented in user space instead. v3 changes: - split ABI docs with the actual implementation - better error handling in lm25066 implementation - numeric permissions - only add samples attributes for page 0 v2 changes: - PMBUS_VIRT_* registers used instead of custom sysfs groups for configuring samples for average - coeffs are only exported as sysfs attirbutes if the format used is "direct" - fixed memory allocation issue in coeffs patch Krzysztof Adamski (4): pmbus: introduce PMBUS_VIRT_*_SAMPLES registers hwmon: Document the samples attributes lm25066: support SAMPLES_FOR_AVG register pmbus_core: export coefficients via sysfs Documentation/hwmon/sysfs-interface | 18 +++ drivers/hwmon/pmbus/lm25066.c | 17 ++- drivers/hwmon/pmbus/pmbus.h | 15 ++ drivers/hwmon/pmbus/pmbus_core.c| 214 +++- 4 files changed, 261 insertions(+), 3 deletions(-) -- 2.20.1
Re: [PATCH v2 1/3] pmbus: introduce PMBUS_VIRT_*_SAMPLES registers
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
>>---
>> 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_samplesSets 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_PWM12BIT(20)
>> #define PMBUS_HAVE_PWM34BIT(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)
>>+
>>
Re: [PATCH v2 2/3] lm25066: support SAMPLES_FOR_AVG register
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
>>---
>> 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
>> #include
>> #include
>>+#include
>> #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_SETUP0xd9
>> #define LM25066_READ_AVG_VIN0xdc
>>+#define LM25066_SAMPLES_FOR_AVG 0xdb
>> #define LM25066_READ_AVG_VOUT 0xdd
>> #define LM25066_READ_AVG_IIN0xde
>> #define LM25066_READ_AVG_PIN0xdf
>> #define LM25066_DEV_SETUP_CLBIT(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
[PATCH v2 0/3] pmbus: extend configurability via sysfs
Hi, I'm posting updated version of this patchet. I think first two patches implement things in the way that where suggested by Guenter (using PMBUS_VIRT_*). I have to admit, I like this better than using custom sysfs approach from v1. I'm also posting updated version of the patch exporting coefficients even though it was not liked by Guenter. I hope you could reconsider as I think your arguments would be completely valid if it was "raw" hwmon device but for pmbus devices, values are already reported in real-world units and all drivers using "direct" mode uses those coefficients and the only way userspace could adjust those reported values knowing proper coefficients would be to also know the coefficients used and basically reverse the calculations made by kernel and made them with proper coeffs. While doable, I don't find this elegant or practical. If we already do conversion in the kernel, I think there should be a way to make sure this conversion produces sane values. This patch series extends pmbus core for two specific use cases we have: - First two patches allows lm25066 driver to set number of samples for average values (by controlling manufacturer specific SAMPLES_FOR_AVG register). It is useful to be able to set this register when using any of the *_average registers, especially since the default value means we are averaging 1 sample which isn't that useful. - Third patch exports m, b, R coefficients so that they can be adjusted by user space. We can't use default coefficients values and in order to achieve high accuracy, we calibrate then per unit so using device-tree or similar concepts (which are generally shared by all board of the same type) to store them is not an option too. Thus, we export it so that the logic of loading proper coeffs can be implemented in user space instead. v2 changes: - PMBUS_VIRT_* registers used instead of custom sysfs groups for configuring samples for average - coeffs are only exported as sysfs attirbutes if the format used is "direct" - fixed memory allocation issue in coeffs patch Krzysztor Adamski (3): pmbus: introduce PMBUS_VIRT_*_SAMPLES registers lm25066: support SAMPLES_FOR_AVG register pmbus_core: export coefficients via sysfs Documentation/hwmon/sysfs-interface | 18 +++ drivers/hwmon/pmbus/lm25066.c | 15 +- drivers/hwmon/pmbus/pmbus.h | 15 ++ drivers/hwmon/pmbus/pmbus_core.c| 213 +++- 4 files changed, 258 insertions(+), 3 deletions(-) -- 2.20.1
[PATCH v2 2/3] lm25066: support SAMPLES_FOR_AVG register
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
---
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
#include
#include
+#include
#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_AVG0xdb
#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_MAX4096
+
/* 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
[PATCH v2 1/3] pmbus: introduce PMBUS_VIRT_*_SAMPLES registers
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
---
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.
+
+samplesSets number of average samples for all types of
measurements.
+ RW
+
+in_samples
+power_samples
+curr_samples
+temp_samplesSets 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, ) < 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;
+
+
Re: [PATCH 2/3] lm25066: export sysfs attribute for SAMPLES_FOR_AVG
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, );
>> > + 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[] = {
>> > + _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 1/3] pmbus: support for custom sysfs attributes
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
>>---
>> 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 3/3] pmbus: export coefficients via sysfs
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
[PATCH 3/3] pmbus: export coefficients via sysfs
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
---
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_SIZE24
+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(_attr->attr, name, (S_IWUSR | S_IRUGO),
+ device_show_int, device_store_int);
+ ext_attr->var = target;
+
+ return _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, [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
[PATCH 2/3] lm25066: export sysfs attribute for SAMPLES_FOR_AVG
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
---
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
#include
#include
+#include
#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_AVG0xdb
#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, );
+ 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[] = {
+ _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 0/3] pmbus: extend configurability via sysfs
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
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
---
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 = >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(>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] = >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
