| Message ID | 8087920a011662114ca151d4e04186c121a9a737.1554934898.git.krzysztof.adamski@nokia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pmbus: extend configurability via sysfs | expand |
On 4/10/19 3:39 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > In order to get best accuracy, in case of some devices it is required to > tweak coefficient values. This is especially true for devices using > some external shunt resistor or being operated in non-usual environment. > Those values may be measured during device production and stored as > calibration values in some place (like EEPROM or even a file). > > To support wide range of possible use cases, lets export those to > user space via sysfs attributes so that it can implement its own > policies about them. All those attributes are put into separate > attribute_group struct so that we can set its name to group all of them > in a "coefficients" subdirectory in sysfs. > Coefficients are hardcoded into the chip, and the hwmon ABI reports raw values. Any correction should be done using sensors3.conf. Guenter > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > --- > drivers/hwmon/pmbus/pmbus_core.c | 100 ++++++++++++++++++++++++++++++- > 1 file changed, 97 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 7a7dcdc8acc9..8cb61fc977db 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -57,6 +57,8 @@ > > #define PMBUS_NAME_SIZE 24 > > +static const char * const coeff_name[] = {"m", "b", "R"}; > + > struct pmbus_sensor { > struct pmbus_sensor *next; > char name[PMBUS_NAME_SIZE]; /* sysfs sensor name */ > @@ -98,11 +100,12 @@ struct pmbus_data { > int exponent[PMBUS_PAGES]; > /* linear mode: exponent for output voltages */ > > - const struct pmbus_driver_info *info; > + struct pmbus_driver_info *info; > > int max_attributes; > int num_attributes; > struct attribute_group group; > + struct attribute_group group_coeff; > const struct attribute_group **groups; > struct dentry *debugfs; /* debugfs device directory */ > > @@ -1901,6 +1904,88 @@ static int pmbus_add_fan_attributes(struct i2c_client *client, > return 0; > } > > +static struct attribute *pmbus_init_coeff_attr(struct pmbus_data *data, > + const char *prefix, > + const char *coeff, int *target) > +{ > + struct dev_ext_attribute *ext_attr; > + char *name; > + > + ext_attr = devm_kzalloc(data->dev, sizeof(ext_attr), GFP_KERNEL); > + if (!ext_attr) > + return NULL; > + > + name = devm_kasprintf(data->dev, GFP_KERNEL, "%s_%s", prefix, coeff); > + if (!name) > + return NULL; > + > + pmbus_dev_attr_init(&ext_attr->attr, name, (S_IWUSR | S_IRUGO), > + device_show_int, device_store_int); > + ext_attr->var = target; > + > + return &ext_attr->attr.attr; > +} > + > +static int pmbus_add_coeff_attributes_class(struct pmbus_data *data, > + const char *prefix, > + enum pmbus_sensor_classes class, > + struct attribute **attrs) > +{ > + int *coeff_val[] = {data->info->m, data->info->b, data->info->R}; > + struct attribute *ret; > + int i, count = 0; > + > + for (i = 0; i < ARRAY_SIZE(coeff_name); i++) { > + ret = pmbus_init_coeff_attr(data, prefix, coeff_name[i], > + coeff_val[i]); > + if (!ret) > + return -ENOMEM; > + attrs[count++] = ret; > + } > + > + return count; > +} > + > +static const char * const classes_names[] = { > + [PSC_VOLTAGE_IN] = "vin", > + [PSC_VOLTAGE_OUT] = "vout", > + [PSC_CURRENT_IN] = "iin", > + [PSC_CURRENT_OUT] = "iout", > + [PSC_POWER] = "p", > + [PSC_TEMPERATURE] = "temp", > +}; > + > +static int pmbus_add_coeff_attributes(struct i2c_client *client, > + struct pmbus_data *data) > +{ > + struct attribute **attrs; > + int i, n = 0, ret = 0; > + > + attrs = kcalloc(ARRAY_SIZE(coeff_name) * (PSC_NUM_CLASSES + 1), > + sizeof(void *), GFP_KERNEL); > + if (!attrs) > + return -ENOMEM; > + > + for (i = 0; i < ARRAY_SIZE(classes_names); i++) { > + if (classes_names[i] == NULL) > + continue; > + > + ret = pmbus_add_coeff_attributes_class(data, classes_names[i], > + i, &attrs[n]); > + if (ret < 0) { > + kfree(attrs); > + return ret; > + } > + > + n += ret; > + } > + > + data->group_coeff.name = "coefficients"; > + data->group_coeff.attrs = attrs; > + > + return 0; > +} > + > static int pmbus_find_attributes(struct i2c_client *client, > struct pmbus_data *data) > { > @@ -1932,6 +2017,11 @@ static int pmbus_find_attributes(struct i2c_client *client, > > /* Fans */ > ret = pmbus_add_fan_attributes(client, data); > + if (ret) > + return ret; > + > + /* Coefficients */ > + ret = pmbus_add_coeff_attributes(client, data); > return ret; > } > > @@ -2324,7 +2414,8 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, > while (info->groups[groups_num]) > groups_num++; > > - data->groups = devm_kcalloc(dev, groups_num + 2, sizeof(void *), > + /* Two default groups + ending NULL makes 3 groups minimum */ > + data->groups = devm_kcalloc(dev, groups_num + 3, sizeof(void *), > GFP_KERNEL); > if (!data->groups) > return -ENOMEM; > @@ -2356,7 +2447,8 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, > } > > data->groups[0] = &data->group; > - memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num); > + data->groups[1] = &data->group_coeff; > + memcpy(data->groups + 2, info->groups, sizeof(void *) * groups_num); > data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name, > data, data->groups); > if (IS_ERR(data->hwmon_dev)) { > @@ -2379,6 +2471,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, > hwmon_device_unregister(data->hwmon_dev); > out_kfree: > kfree(data->group.attrs); > + kfree(data->group_coeff.attrs); > return ret; > } > EXPORT_SYMBOL_GPL(pmbus_do_probe); > @@ -2391,6 +2484,7 @@ int pmbus_do_remove(struct i2c_client *client) > > hwmon_device_unregister(data->hwmon_dev); > kfree(data->group.attrs); > + kfree(data->group_coeff.attrs); > return 0; > } > EXPORT_SYMBOL_GPL(pmbus_do_remove); >
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
On 4/11/19 12:45 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > On Wed, Apr 10, 2019 at 05:30:08PM -0700, Guenter Roeck wrote: >> On 4/10/19 3:39 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>> In order to get best accuracy, in case of some devices it is required to >>> tweak coefficient values. This is especially true for devices using >>> some external shunt resistor or being operated in non-usual environment. >>> Those values may be measured during device production and stored as >>> calibration values in some place (like EEPROM or even a file). >>> >>> To support wide range of possible use cases, lets export those to >>> user space via sysfs attributes so that it can implement its own >>> policies about them. All those attributes are put into separate >>> attribute_group struct so that we can set its name to group all of them >>> in a "coefficients" subdirectory in sysfs. >>> >> >> Coefficients are hardcoded into the chip, and the hwmon ABI reports raw >> values. Any correction should be done using sensors3.conf. > > I'm not sure what you mean by the fact they are hardcoded into chip. I > am targeting a case where direct values are being converted to real > world values using coefficients by pmbus_reg2data_direct() function. My > understanding is that the reason why the devices does not report values > in real world units but requires using coefficients for calculation is to > ease the calibration. For example the LM5064[1] has a chapter called > "determining telemetry coefficients empirically with linear fit" which > describes how to calculate them, based on the sense resistor used. Some > drivers, like adm1275.c, have a custom way to indirectly influence the > coefficients values by using Devicetree ("shunt-resistor-micro-ohms") > but this is not really flexible nor general approach. In case of > adm1275, only "m" coefficient is adjusted this way. Depending on "m" > value, "R" might require adjustments as well and we also need "b" to > achieve best accuracy. Then, again, using Device Tree is not suitable > for per device calibration. > ' Why not ? You'll have to explain that one. It is not as if coefficients would change on the fly. They are chip and/or board properties. > My argument here is that the kernel does not return raw value in this > case - it returns (supposedly) real-world values that are calculated > internally based of coefficients according to the formula specified by > pmbus specification. In my opinion it would make sense to provide it > with proper coeffs if defaults are not suitable. Otherwise reporting > "real-values" doesn't make much sense. In other words, our > implementation would currently report real-world values if your case > happens to match default coeffs for some shunt resistor and > environment specified in datasheet of the device. > This is why I dislike making exceptions. It always creates a bad precedence. hwmon devices are supposed to report raw values, to be converted to real values using the configuration file. We can discuss scaling and possible ABI enhancements, but it will have to be generic. b/m/r is not generic. You also don't make the case why those values would have to be runtime-adjustable. Note that there are several issues with the patch itself: There is no validation, the names are questionable ("p" ?), the scope is questionable (if an adjustment is needed, it would likely have to be per sensor, not per group), and the attributes are created unconditionally even if the chip does not report values in direct mode, just to name a few. However, that is all irrelevant: At this point I'll resist further chip specific changes in that area. If we move from reporting raw values towards reporting adjusted values, the solution will have to be generic and must not be chip specific. Thanks, Guenter
On Thu, Apr 11, 2019 at 06:39:11AM -0700, Guenter Roeck wrote: >On 4/11/19 12:45 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>On Wed, Apr 10, 2019 at 05:30:08PM -0700, Guenter Roeck wrote: >>>On 4/10/19 3:39 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>>>In order to get best accuracy, in case of some devices it is required to >>>>tweak coefficient values. This is especially true for devices using >>>>some external shunt resistor or being operated in non-usual environment. >>>>Those values may be measured during device production and stored as >>>>calibration values in some place (like EEPROM or even a file). >>>> >>>>To support wide range of possible use cases, lets export those to >>>>user space via sysfs attributes so that it can implement its own >>>>policies about them. All those attributes are put into separate >>>>attribute_group struct so that we can set its name to group all of them >>>>in a "coefficients" subdirectory in sysfs. >>>> >>> >>>Coefficients are hardcoded into the chip, and the hwmon ABI reports raw >>>values. Any correction should be done using sensors3.conf. >> >>I'm not sure what you mean by the fact they are hardcoded into chip. I >>am targeting a case where direct values are being converted to real >>world values using coefficients by pmbus_reg2data_direct() function. My >>understanding is that the reason why the devices does not report values >>in real world units but requires using coefficients for calculation is to >>ease the calibration. For example the LM5064[1] has a chapter called >>"determining telemetry coefficients empirically with linear fit" which >>describes how to calculate them, based on the sense resistor used. Some >>drivers, like adm1275.c, have a custom way to indirectly influence the >>coefficients values by using Devicetree ("shunt-resistor-micro-ohms") >>but this is not really flexible nor general approach. In case of >>adm1275, only "m" coefficient is adjusted this way. Depending on "m" >>value, "R" might require adjustments as well and we also need "b" to >>achieve best accuracy. Then, again, using Device Tree is not suitable >>for per device calibration. >> >' >Why not ? You'll have to explain that one. It is not as if coefficients >would change on the fly. They are chip and/or board properties. But the coefficients will depend on the exact value of the shunt resistor. To get bet accuracy, the coefficients are calculated for each board unit. While all units of the same type share the devicetree (otherwise this devicetree would have to be generated by some firmware software first). > >>My argument here is that the kernel does not return raw value in this >>case - it returns (supposedly) real-world values that are calculated >>internally based of coefficients according to the formula specified by >>pmbus specification. In my opinion it would make sense to provide it >>with proper coeffs if defaults are not suitable. Otherwise reporting >>"real-values" doesn't make much sense. In other words, our >>implementation would currently report real-world values if your case >>happens to match default coeffs for some shunt resistor and >>environment specified in datasheet of the device. >> > >This is why I dislike making exceptions. It always creates a bad precedence. >hwmon devices are supposed to report raw values, to be converted to real >values using the configuration file. > > >We can discuss scaling and possible ABI enhancements, but it will have >to be generic. b/m/r is not generic. You also don't make the case why >those values would have to be runtime-adjustable. > b/m/r are coefficients mentioned by PMBUS specification and the change is PMBUS specific. My understanding is that all PMBUS devices report values in real-world units (either in linear or direct mode) and this is implemented this way in the code already. In fact, if the device would report raw values, we wouldn't need this patch as we would simply apply those coefficients in userspace. But if drivers report real values which require correct coefficients to work, I would expect to be able to tweak them. > >Note that there are several issues with the patch itself: There is no >validation, the names are questionable ("p" ?), the scope is questionable >(if an adjustment is needed, it would likely have to be per sensor, not >per group), and the attributes are created unconditionally even if the >chip does not report values in direct mode, just to name a few. I can fix all that, of course, but we have to discuss the the mentioned issues first. I agree with all those arguments except the scope one. As we currently only support specifying coefficients per class and this is exacly how I export them. Unless I misunderstood your comment, of course. >However, that is all irrelevant: At this point I'll resist further chip >specific changes in that area. If we move from reporting raw values >towards reporting adjusted values, the solution will have to be generic >and must not be chip specific. But this is not chip specific change, it is more like bus (PMBUS) specific. All pmbus devices reporting in direct mode have those coefficients, all of them use the same formula to calculate real values out of raw values as this is specified in the PMBUS specification. I agree this might not be useful for devices reporting in linear/vid mode so I could exclude them when exporting coefficients. Krzysztof
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 7a7dcdc8acc9..8cb61fc977db 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -57,6 +57,8 @@ #define PMBUS_NAME_SIZE 24 +static const char * const coeff_name[] = {"m", "b", "R"}; + struct pmbus_sensor { struct pmbus_sensor *next; char name[PMBUS_NAME_SIZE]; /* sysfs sensor name */ @@ -98,11 +100,12 @@ struct pmbus_data { int exponent[PMBUS_PAGES]; /* linear mode: exponent for output voltages */ - const struct pmbus_driver_info *info; + struct pmbus_driver_info *info; int max_attributes; int num_attributes; struct attribute_group group; + struct attribute_group group_coeff; const struct attribute_group **groups; struct dentry *debugfs; /* debugfs device directory */ @@ -1901,6 +1904,88 @@ static int pmbus_add_fan_attributes(struct i2c_client *client, return 0; } +static struct attribute *pmbus_init_coeff_attr(struct pmbus_data *data, + const char *prefix, + const char *coeff, int *target) +{ + struct dev_ext_attribute *ext_attr; + char *name; + + ext_attr = devm_kzalloc(data->dev, sizeof(ext_attr), GFP_KERNEL); + if (!ext_attr) + return NULL; + + name = devm_kasprintf(data->dev, GFP_KERNEL, "%s_%s", prefix, coeff); + if (!name) + return NULL; + + pmbus_dev_attr_init(&ext_attr->attr, name, (S_IWUSR | S_IRUGO), + device_show_int, device_store_int); + ext_attr->var = target; + + return &ext_attr->attr.attr; +} + +static int pmbus_add_coeff_attributes_class(struct pmbus_data *data, + const char *prefix, + enum pmbus_sensor_classes class, + struct attribute **attrs) +{ + int *coeff_val[] = {data->info->m, data->info->b, data->info->R}; + struct attribute *ret; + int i, count = 0; + + for (i = 0; i < ARRAY_SIZE(coeff_name); i++) { + ret = pmbus_init_coeff_attr(data, prefix, coeff_name[i], + coeff_val[i]); + if (!ret) + return -ENOMEM; + attrs[count++] = ret; + } + + return count; +} + +static const char * const classes_names[] = { + [PSC_VOLTAGE_IN] = "vin", + [PSC_VOLTAGE_OUT] = "vout", + [PSC_CURRENT_IN] = "iin", + [PSC_CURRENT_OUT] = "iout", + [PSC_POWER] = "p", + [PSC_TEMPERATURE] = "temp", +}; + +static int pmbus_add_coeff_attributes(struct i2c_client *client, + struct pmbus_data *data) +{ + struct attribute **attrs; + int i, n = 0, ret = 0; + + attrs = kcalloc(ARRAY_SIZE(coeff_name) * (PSC_NUM_CLASSES + 1), + sizeof(void *), GFP_KERNEL); + if (!attrs) + return -ENOMEM; + + for (i = 0; i < ARRAY_SIZE(classes_names); i++) { + if (classes_names[i] == NULL) + continue; + + ret = pmbus_add_coeff_attributes_class(data, classes_names[i], + i, &attrs[n]); + if (ret < 0) { + kfree(attrs); + return ret; + } + + n += ret; + } + + data->group_coeff.name = "coefficients"; + data->group_coeff.attrs = attrs; + + return 0; +} + static int pmbus_find_attributes(struct i2c_client *client, struct pmbus_data *data) { @@ -1932,6 +2017,11 @@ static int pmbus_find_attributes(struct i2c_client *client, /* Fans */ ret = pmbus_add_fan_attributes(client, data); + if (ret) + return ret; + + /* Coefficients */ + ret = pmbus_add_coeff_attributes(client, data); return ret; } @@ -2324,7 +2414,8 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, while (info->groups[groups_num]) groups_num++; - data->groups = devm_kcalloc(dev, groups_num + 2, sizeof(void *), + /* Two default groups + ending NULL makes 3 groups minimum */ + data->groups = devm_kcalloc(dev, groups_num + 3, sizeof(void *), GFP_KERNEL); if (!data->groups) return -ENOMEM; @@ -2356,7 +2447,8 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, } data->groups[0] = &data->group; - memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num); + data->groups[1] = &data->group_coeff; + memcpy(data->groups + 2, info->groups, sizeof(void *) * groups_num); data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name, data, data->groups); if (IS_ERR(data->hwmon_dev)) { @@ -2379,6 +2471,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, hwmon_device_unregister(data->hwmon_dev); out_kfree: kfree(data->group.attrs); + kfree(data->group_coeff.attrs); return ret; } EXPORT_SYMBOL_GPL(pmbus_do_probe); @@ -2391,6 +2484,7 @@ int pmbus_do_remove(struct i2c_client *client) hwmon_device_unregister(data->hwmon_dev); kfree(data->group.attrs); + kfree(data->group_coeff.attrs); return 0; } EXPORT_SYMBOL_GPL(pmbus_do_remove);
In order to get best accuracy, in case of some devices it is required to tweak coefficient values. This is especially true for devices using some external shunt resistor or being operated in non-usual environment. Those values may be measured during device production and stored as calibration values in some place (like EEPROM or even a file). To support wide range of possible use cases, lets export those to user space via sysfs attributes so that it can implement its own policies about them. All those attributes are put into separate attribute_group struct so that we can set its name to group all of them in a "coefficients" subdirectory in sysfs. Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> --- drivers/hwmon/pmbus/pmbus_core.c | 100 ++++++++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 3 deletions(-)