| Message ID | 4887ef85022d7ceb26905997c19a874d7d89087f.1555273192.git.krzysztof.adamski@nokia.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | pmbus: extend configurability via sysfs | expand |
On Sun, Apr 14, 2019 at 11:59:38PM +0200, Krzysztof Adamski wrote: >PMBUS devices report values in real-world units. Those using direct >format require conversion using standarised coefficients and formula. >This operation is already done by pmbus_core and the default values for >coefficients are configured by chip drivers. > >However those default values are not allways suitable as they depend on >the value of sense register and environment used. For that reason, in >order to get values that make sense or just to get best accuracy, in it >may be required to tweak coefficient values. The proper values may be >measured during device production (to get best accuracy, they might be >callibrated for each individual unit) and stored as calibration values >in some place (like EEPROM or even a file). > >To support wide range of possible use cases, lets export those to user >space via sysfs attributes so that it can implement its own policies >about them. All those attributes are put into separate attribute_group >struct so that we can set its name to group all of them in a >"coefficients" subdirectory in sysfs. > >Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> >--- >drivers/hwmon/pmbus/pmbus_core.c | 104 ++++++++++++++++++++++++++++++- >1 file changed, 102 insertions(+), 2 deletions(-) > [...] >+ pmbus_dev_attr_init(&ext_attr->attr, name, (S_IWUSR | S_IRUGO), I screwed up and did not fix this also, sorry for that. I'm not sending an updated version (yet) as I didn't get your approval for the concept itself (though I still hope you will reconsider taking into account my latest arguments). [...] > Krzysztof
On 4/14/19 3:37 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > On Sun, Apr 14, 2019 at 11:59:38PM +0200, Krzysztof Adamski wrote: >> PMBUS devices report values in real-world units. Those using direct >> format require conversion using standarised coefficients and formula. >> This operation is already done by pmbus_core and the default values for >> coefficients are configured by chip drivers. >> >> However those default values are not allways suitable as they depend on >> the value of sense register and environment used. For that reason, in >> order to get values that make sense or just to get best accuracy, in it >> may be required to tweak coefficient values. The proper values may be >> measured during device production (to get best accuracy, they might be >> callibrated for each individual unit) and stored as calibration values >> in some place (like EEPROM or even a file). >> >> To support wide range of possible use cases, lets export those to user >> space via sysfs attributes so that it can implement its own policies >> about them. All those attributes are put into separate attribute_group >> struct so that we can set its name to group all of them in a >> "coefficients" subdirectory in sysfs. >> >> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> >> --- >> drivers/hwmon/pmbus/pmbus_core.c | 104 ++++++++++++++++++++++++++++++- >> 1 file changed, 102 insertions(+), 2 deletions(-) >> > > [...] > >> + pmbus_dev_attr_init(&ext_attr->attr, name, (S_IWUSR | S_IRUGO), > > I screwed up and did not fix this also, sorry for that. I'm not sending > an updated version (yet) as I didn't get your approval for the concept > itself (though I still hope you will reconsider taking into account my > latest arguments). > I won't, sorry. As mentioned before, this is a generic problem that needs to be solved for all hwmon drivers, not just for pmbus devices, and also not just for pmbus devices using direct mode. Even worse, this doesn't even work for pmbus devices in general. PMBus supports commands such as VOUT_SCALE_LOOP and various other calibration commands such as VOUT_CAL_OFFSET, IOUT_CAL_GAIN and IOUT_CAL_OFFSET. Your approach doesn't even try to support those commands; you only look at r/m/b which may suit your needs but are hardly generic and even misleading since chip programming isn't adjusted even when that is possible. Addressing this problem in a generic way will require substantially more thought than just adding a couple of pmbus direct mode specific attributes. Guenter
On Sun, Apr 14, 2019 at 07:38:33PM -0700, Guenter Roeck wrote: >On 4/14/19 3:37 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>On Sun, Apr 14, 2019 at 11:59:38PM +0200, Krzysztof Adamski wrote: >>>PMBUS devices report values in real-world units. Those using direct >>>format require conversion using standarised coefficients and formula. >>>This operation is already done by pmbus_core and the default values for >>>coefficients are configured by chip drivers. >>> >>>However those default values are not allways suitable as they depend on >>>the value of sense register and environment used. For that reason, in >>>order to get values that make sense or just to get best accuracy, in it >>>may be required to tweak coefficient values. The proper values may be >>>measured during device production (to get best accuracy, they might be >>>callibrated for each individual unit) and stored as calibration values >>>in some place (like EEPROM or even a file). >>> >>>To support wide range of possible use cases, lets export those to user >>>space via sysfs attributes so that it can implement its own policies >>>about them. All those attributes are put into separate attribute_group >>>struct so that we can set its name to group all of them in a >>>"coefficients" subdirectory in sysfs. >>> >>>Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> >>>--- >>>drivers/hwmon/pmbus/pmbus_core.c | 104 ++++++++++++++++++++++++++++++- >>>1 file changed, 102 insertions(+), 2 deletions(-) >>> >> >>[...] >> >>>+ pmbus_dev_attr_init(&ext_attr->attr, name, (S_IWUSR | S_IRUGO), >> >>I screwed up and did not fix this also, sorry for that. I'm not sending >>an updated version (yet) as I didn't get your approval for the concept >>itself (though I still hope you will reconsider taking into account my >>latest arguments). >> > >I won't, sorry. As mentioned before, this is a generic problem that needs >to be solved for all hwmon drivers, not just for pmbus devices, and also >not just for pmbus devices using direct mode. > >Even worse, this doesn't even work for pmbus devices in general. >PMBus supports commands such as VOUT_SCALE_LOOP and various other >calibration commands such as VOUT_CAL_OFFSET, IOUT_CAL_GAIN and >IOUT_CAL_OFFSET. Your approach doesn't even try to support those >commands; you only look at r/m/b which may suit your needs but are >hardly generic I agree I don't see this problem as general as you. The mentioned registers can easily be supported if needed by just attaching attributes to them. That would allow doing things that cannot be done currently (configuring precision of the regulation of the devices) while I see coefficients problems as something totally different - more like a fix to the already supported functionality (reporting current measurements). > and even misleading since chip programming isn't adjusted even when > that is possible. I don't understand what you mean by that, could you explain? > >Addressing this problem in a generic way will require substantially >more thought than just adding a couple of pmbus direct mode specific >attributes. I'm willing to put more effort into that but I would need to better understand what is your view of the scope of problem here. Krzysztof
On Mon, Apr 15, 2019 at 08:34:48AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > On Sun, Apr 14, 2019 at 07:38:33PM -0700, Guenter Roeck wrote: > >On 4/14/19 3:37 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > >>On Sun, Apr 14, 2019 at 11:59:38PM +0200, Krzysztof Adamski wrote: > >>>PMBUS devices report values in real-world units. Those using direct > >>>format require conversion using standarised coefficients and formula. > >>>This operation is already done by pmbus_core and the default values for > >>>coefficients are configured by chip drivers. > >>> > >>>However those default values are not allways suitable as they depend on > >>>the value of sense register and environment used. For that reason, in > >>>order to get values that make sense or just to get best accuracy, in it > >>>may be required to tweak coefficient values. The proper values may be > >>>measured during device production (to get best accuracy, they might be > >>>callibrated for each individual unit) and stored as calibration values > >>>in some place (like EEPROM or even a file). > >>> > >>>To support wide range of possible use cases, lets export those to user > >>>space via sysfs attributes so that it can implement its own policies > >>>about them. All those attributes are put into separate attribute_group > >>>struct so that we can set its name to group all of them in a > >>>"coefficients" subdirectory in sysfs. > >>> > >>>Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > >>>--- > >>>drivers/hwmon/pmbus/pmbus_core.c | 104 ++++++++++++++++++++++++++++++- > >>>1 file changed, 102 insertions(+), 2 deletions(-) > >>> > >> > >>[...] > >> > >>>+ pmbus_dev_attr_init(&ext_attr->attr, name, (S_IWUSR | S_IRUGO), > >> > >>I screwed up and did not fix this also, sorry for that. I'm not sending > >>an updated version (yet) as I didn't get your approval for the concept > >>itself (though I still hope you will reconsider taking into account my > >>latest arguments). > >> > > > >I won't, sorry. As mentioned before, this is a generic problem that needs > >to be solved for all hwmon drivers, not just for pmbus devices, and also > >not just for pmbus devices using direct mode. > > > >Even worse, this doesn't even work for pmbus devices in general. > >PMBus supports commands such as VOUT_SCALE_LOOP and various other > >calibration commands such as VOUT_CAL_OFFSET, IOUT_CAL_GAIN and > >IOUT_CAL_OFFSET. Your approach doesn't even try to support those > >commands; you only look at r/m/b which may suit your needs but are > >hardly generic > > I agree I don't see this problem as general as you. The mentioned > registers can easily be supported if needed by just attaching attributes > to them. That would allow doing things that cannot be done currently > (configuring precision of the regulation of the devices) while I see > coefficients problems as something totally different - more like a fix > to the already supported functionality (reporting current measurements). > We are talking an ABI here. ABIs are supposed to be hardware independent. Directly mapping registers to attributes is not exactly hardware independent. > > and even misleading since chip programming isn't adjusted even when > > that is possible. > > I don't understand what you mean by that, could you explain? > "when that is possible" -> on chips supporting calibration commands, one should not manipulate b/m/R but write any adjustments into the chip using those commands. > > > >Addressing this problem in a generic way will require substantially > >more thought than just adding a couple of pmbus direct mode specific > >attributes. > > I'm willing to put more effort into that but I would need to better > understand what is your view of the scope of problem here. > There are a number of elements. First, a case has to be made why the current mechanism of using sensors3.conf for adjustments is insufficient. This may be well understood by some, but the case needs to be made (ie some chips do have HW registers to perform the adjustment, and in some cases there is accuracy loss by performing adjustments in user space). Second, we'll need to determine which standardized attributes are needed. As far as I can see this would probably be 'scale' (or maybe 'gain') and 'offset'. The question is how to express especially 'scale'. It would either require both a multiplier and a divider, or it would have to be expressed in fixed point arithmetic. I would prefer the latter, but I am open to suggestions. Third, we will have to determine how to apply this all to pmbus chips. It must not only apply to direct mode chips, but to all PMBus chips. Even for direct mode chips the case needs to be made if the attributes should apply per sensor class or per sensor (I would think per sensor). Just looking at VOUT, chapter 9.2 of the PMBus specification (Rev 1.1, part II) gives an example of what to look out for: Just for VOUT, there are VOUT_TRIM, VOUT_CAL_OFFSET, VOUT_DROOP (does this require yet another attribute ?), and VOUT_SCALE_LOOP. I should add that the above, for VOUT, are the primary means for PMBus devices to adjust voltage readings. This also applies to IOUT, which has similar standard PMBus registers available for calibration (IOUT_CAL_GAIN and IOUT_CAL_OFFSET). On top of that, some chips support vendor specific commands to calibrate other sensors. For example, MAX15301 supports a EXT_TEMP_CAL command to calibrate temperature sensor readings. Also note that direct mode PMBUs chips _should_ support a COEFFICIENTS command to retrieve actual coefficients from the chip (though I don't recall ever encountering a chip that actually supports it). Overall, adjusting readings through manipulation of b/m/R is at best a workaround. As such, the chip we are talking about here does pretty much everything wrong. I do understand the need for a more dynamic calibration support on the driver level, but the solution must not focus on such a chip and needs to be more generic. Thanks, Guenter
On Mon, Apr 15, 2019 at 09:33:10AM -0700, Guenter Roeck wrote: >We are talking an ABI here. ABIs are supposed to be hardware independent. >Directly mapping registers to attributes is not exactly hardware independent. That is true, I get that, even though I don't want to map registers. I want to map coefficients which are hardware independent but only as far as we take into account only PMBUS. And I guess this is the root of the difference in our opinions - I still only think about PMBUS devices while you think about all HWMON devices (I think). >> > and even misleading since chip programming isn't adjusted even when >> > that is possible. >> >> I don't understand what you mean by that, could you explain? >> >"when that is possible" -> on chips supporting calibration commands, >one should not manipulate b/m/R but write any adjustments into the chip >using those commands. > >> > >> >Addressing this problem in a generic way will require substantially >> >more thought than just adding a couple of pmbus direct mode specific >> >attributes. >> >> I'm willing to put more effort into that but I would need to better >> understand what is your view of the scope of problem here. >> >There are a number of elements. > >First, a case has to be made why the current mechanism of using >sensors3.conf for adjustments is insufficient. This may be well >understood by some, but the case needs to be made (ie some chips do >have HW registers to perform the adjustment, and in some cases there >is accuracy loss by performing adjustments in user space). My argument why you the solution of sensor3.conf can't be directly used is that the readings for PMBUS devices uses real values, that are already processed by some coefficient values. If you want to apply corrected coefficients, you would first need to know what default coeffs were used to "revese" the calculation. And the specification for devices I am working with (ADM1275 and LM25066 families) describe how to get such corrected coeffs so it is sensible to assume they might be available. >Second, we'll need to determine which standardized attributes are >needed. As far as I can see this would probably be 'scale' (or maybe >'gain') and 'offset'. The question is how to express especially 'scale'. >It would either require both a multiplier and a divider, or it would >have to be expressed in fixed point arithmetic. I would prefer the >latter, but I am open to suggestions. > >Third, we will have to determine how to apply this all to pmbus >chips. It must not only apply to direct mode chips, but to all >PMBus chips. Even for direct mode chips the case needs to be made >if the attributes should apply per sensor class or per sensor (I >would think per sensor). Just looking at VOUT, chapter 9.2 of the >PMBus specification (Rev 1.1, part II) gives an example of what >to look out for: Just for VOUT, there are VOUT_TRIM, VOUT_CAL_OFFSET, >VOUT_DROOP (does this require yet another attribute ?), and >VOUT_SCALE_LOOP. > >I should add that the above, for VOUT, are the primary means for >PMBus devices to adjust voltage readings. This also applies to IOUT, >which has similar standard PMBus registers available for calibration >(IOUT_CAL_GAIN and IOUT_CAL_OFFSET). On top of that, some chips support >vendor specific commands to calibrate other sensors. For example, >MAX15301 supports a EXT_TEMP_CAL command to calibrate temperature sensor >readings. > >Also note that direct mode PMBUs chips _should_ support a COEFFICIENTS >command to retrieve actual coefficients from the chip (though I don't >recall ever encountering a chip that actually supports it). > >Overall, adjusting readings through manipulation of b/m/R is at best >a workaround. > >As such, the chip we are talking about here does pretty much >everything wrong. I do understand the need for a more dynamic >calibration support on the driver level, but the solution must >not focus on such a chip and needs to be more generic. I need some more time to think about what you wrote here a little more to get the feeling about the most general solution. One thing that came into my mind, however, is that part of my problem would be solved if userspace could at least retrieve coeffs used to calcualte the values. Lets say the chip would support COEFFICIENTS command - how would we implement support for that? In the internal implementation, since all PMBUS devices should support COEFFICIENTS command, we could just add attribute for the PMBUS_COEFFICIENTS. Devices supporting this command, could just, well, pass this to device. Other could "intercept it" and just return values stored in the pmbus_driver_info structure. The only problem is - this command is _very_ generic - it lets you ask for separate coefficients for reading and writing and it can return different one for each of the supported PMBUS commands. This makes it impractical to probe for all supported combinations on probe. Some filtering would have to be provided by the client drivers instead. But still a long list of possible attributes would be created. The end result - ABI would look similara as in my patch and would, again, be pmbus specific and not really expandable to other hwmon devices. But I don't see any other way we could support such a command. But maybe this question - "how, from ABI standpoint, could we implement support for PMBUS_COEFFICIENTS command?" is easier to answer than the more general problem you described? Krzysztof
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 9b7d493c98b9..38493617d6a7 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -57,6 +57,8 @@ #define PMBUS_NAME_SIZE 24 +static const char * const coeff_name[] = {"m", "b", "R"}; + struct pmbus_sensor { struct pmbus_sensor *next; char name[PMBUS_NAME_SIZE]; /* sysfs sensor name */ @@ -98,12 +100,13 @@ struct pmbus_data { int exponent[PMBUS_PAGES]; /* linear mode: exponent for output voltages */ - const struct pmbus_driver_info *info; + struct pmbus_driver_info *info; int max_attributes; int num_attributes; struct attribute_group group; - const struct attribute_group *groups[2]; + const struct attribute_group *groups[3]; + struct attribute_group group_coeff; struct dentry *debugfs; /* debugfs device directory */ struct pmbus_sensor *sensors; @@ -2007,6 +2010,95 @@ static int pmbus_add_samples_attributes(struct i2c_client *client, return 0; } +static struct attribute *pmbus_init_coeff_attr(struct pmbus_data *data, + const char *prefix, + const char *coeff, int *target) +{ + struct dev_ext_attribute *ext_attr; + char *name; + + ext_attr = devm_kzalloc(data->dev, sizeof(*ext_attr), GFP_KERNEL); + if (!ext_attr) + return NULL; + + name = devm_kasprintf(data->dev, GFP_KERNEL, "%s_%s", prefix, coeff); + if (!name) + return NULL; + + pmbus_dev_attr_init(&ext_attr->attr, name, (S_IWUSR | S_IRUGO), + device_show_int, device_store_int); + ext_attr->var = target; + + return &ext_attr->attr.attr; +} + +static int pmbus_add_coeff_attributes_class(struct pmbus_data *data, + const char *prefix, + enum pmbus_sensor_classes class, + struct attribute **attrs) +{ + int *coeff_val[] = {data->info->m, data->info->b, data->info->R}; + struct attribute *ret; + int i, count = 0; + + for (i = 0; i < ARRAY_SIZE(coeff_name); i++) { + ret = pmbus_init_coeff_attr(data, prefix, coeff_name[i], + coeff_val[i]); + if (!ret) + return -ENOMEM; + attrs[count++] = ret; + } + + return count; +} + +static const char * const classes_names[] = { + [PSC_VOLTAGE_IN] = "vin", + [PSC_VOLTAGE_OUT] = "vout", + [PSC_CURRENT_IN] = "iin", + [PSC_CURRENT_OUT] = "iout", + [PSC_POWER] = "power", + [PSC_TEMPERATURE] = "temp", +}; + +static int pmbus_add_coeff_attributes(struct i2c_client *client, + struct pmbus_data *data) +{ + struct attribute **attrs; + int i, n = 0, ret = 0; + + attrs = kcalloc(ARRAY_SIZE(coeff_name) * ARRAY_SIZE(classes_names) + 1, + sizeof(void *), GFP_KERNEL); + if (!attrs) + return -ENOMEM; + + for (i = 0; i < ARRAY_SIZE(classes_names); i++) { + if (classes_names[i] == NULL) + continue; + + if (data->info->format[i] != direct) + continue; + + ret = pmbus_add_coeff_attributes_class(data, classes_names[i], + i, &attrs[n]); + if (ret < 0) { + kfree(attrs); + return ret; + } + + n += ret; + } + + if (n) { + data->group_coeff.name = "coefficients"; + data->group_coeff.attrs = attrs; + } else { + kfree(attrs); + } + + return 0; +} + static int pmbus_find_attributes(struct i2c_client *client, struct pmbus_data *data) { @@ -2042,6 +2134,10 @@ static int pmbus_find_attributes(struct i2c_client *client, return ret; ret = pmbus_add_samples_attributes(client, data); + if (ret) + return ret; + + ret = pmbus_add_coeff_attributes(client, data); return ret; } @@ -2456,6 +2552,8 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, } data->groups[0] = &data->group; + if (data->group_coeff.attrs) + data->groups[1] = &data->group_coeff; data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name, data, data->groups); if (IS_ERR(data->hwmon_dev)) { @@ -2478,6 +2576,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, hwmon_device_unregister(data->hwmon_dev); out_kfree: kfree(data->group.attrs); + kfree(data->group_coeff.attrs); return ret; } EXPORT_SYMBOL_GPL(pmbus_do_probe); @@ -2490,6 +2589,7 @@ int pmbus_do_remove(struct i2c_client *client) hwmon_device_unregister(data->hwmon_dev); kfree(data->group.attrs); + kfree(data->group_coeff.attrs); return 0; } EXPORT_SYMBOL_GPL(pmbus_do_remove);
PMBUS devices report values in real-world units. Those using direct format require conversion using standarised coefficients and formula. This operation is already done by pmbus_core and the default values for coefficients are configured by chip drivers. However those default values are not allways suitable as they depend on the value of sense register and environment used. For that reason, in order to get values that make sense or just to get best accuracy, in it may be required to tweak coefficient values. The proper values may be measured during device production (to get best accuracy, they might be callibrated for each individual unit) and stored as calibration values in some place (like EEPROM or even a file). To support wide range of possible use cases, lets export those to user space via sysfs attributes so that it can implement its own policies about them. All those attributes are put into separate attribute_group struct so that we can set its name to group all of them in a "coefficients" subdirectory in sysfs. Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> --- drivers/hwmon/pmbus/pmbus_core.c | 104 ++++++++++++++++++++++++++++++- 1 file changed, 102 insertions(+), 2 deletions(-)