在 2025/2/27 20:31, Guenter Roeck 写道:
On 2/27/25 00:21, lihuisong (C) wrote:
在 2025/2/26 21:26, Guenter Roeck 写道:
On 2/26/25 02:19, lihuisong (C) wrote:
Hi Guenter,
在 2025/2/25 21:01, Guenter Roeck 写道:
On 2/25/25 00:51, Huisong Li wrote:
When load this mode, we can see the following log:
"power_meter ACPI000D:00: hwmon_device_register() is deprecated.
Please
convert the driver to use hwmon_device_register_with_info()."
So replace hwmon_device_register with
hwmon_device_register_with_info.
To avoid any changes in the display of some sysfs interfaces,
some of
necessary changes in hwmon.c must be made:
1> For 'power1_average_interval_max/min' interface, insert
'average' to the
string corresponding to hwmon_power_average_interval_max/max in
hwmon_power_attr_templates[]. I guess that is what's missing.
2> Add some string attributes in power sensor type because of
below items:
a) power1_accuracy --> display like '90.0%'
b) power1_cap_hyst --> display 'unknown' when its value is
0xFFFFFFFF
c) power1_average_min/max --> display 'unknown' when its
value is
negative.
Note: All the attributes modified above in hwmon core are not
used by other
drivers.
That is not a reason to change the ABI, much less so hiding the
change
in a driver patch.
I am trying to replace the deprecated hwmon_device_register with
hwmon_device_register_with_info for acpi power meter driver.
To avoid any changes in the display of some sysfs interfaces, there
are two modifications in hwmon core as follows:
The only reason to change the hwmon core would be if it is wrong or
if it needs to
be amended. Matching driver expectations is not a valid reason.
Got it.
(1) The first modification in hwmon is as follows:
-->
@@ -646,8 +653,8 @@ static const char * const
hwmon_power_attr_templates[] = {
[hwmon_power_enable] = "power%d_enable",
[hwmon_power_average] = "power%d_average",
[hwmon_power_average_interval] = "power%d_average_interval",
- [hwmon_power_average_interval_max] = "power%d_interval_max",
- [hwmon_power_average_interval_min] = "power%d_interval_min",
+ [hwmon_power_average_interval_max] =
"power%d_average_interval_max",
+ [hwmon_power_average_interval_min] =
"power%d_average_interval_min",
[hwmon_power_average_highest] = "power%d_average_highest",
That is indeed a bug and should be fixed, but in a separate patch.
will do it.
The string names, "power%d_interval_max/min", are missing 'average'.
I think the meaning of these attributes are unclear If no this
word. It can be regarded as a fault.
And power attribute name in acpi power meter is
"power1_average_interval_min/max".
(2)The second modification changes the attribute of
'power_accuracy', 'power_cap_hyst', 'power_average_min' and
'power_average_max' from data to string.
It is appropriate to assign 'power_accuracy' to string attribute.
No. The ABI states that this is the accuracy in %. We don't append "mV"
to voltages, or "mA" to currents either. The unit is determined by
the ABI,
which states that the expected value is a number reflecting %. If a
driver
adds "%", it is a driver oddity, but not a hwmon bug. The whole
point of
providing numeric values is to simplify parsing from userspace.
Adding units
to the displayed value would not only be pointless (since the unit
is defined
by the ABI) but also make parsing more difficult.
Ack
Because it can be displayed as '%' and also include decimal point
like acpi power meter driver, which is more in line with the
meaning of this attribute.
Why ? Are you suggesting that all other attributes should provide
units as well
"to be more in line with the meaning of those attributes" ?
It is absolutely not common to add units to sysfs attributes. We are
not going
to do that, period.
ok.
What I mean is that is the display in power meter.
Again, that is a problem in the power meter driver, not a problem
in the hwmon core.
ok
It might be better to keep other attributes as data types. But it
breaks the cornor display of these attributes in acpi power meter
driver as said below.
a) power1_cap_hyst --> display 'unknown' when its value is
0xFFFFFFFF
b) power1_average_min/max --> display 'unknown' when its value
is negative.
That is a driver problem, not a subsystem problem. If it is so
important to retain
that (i.e., if for some reason some userspace program depends on
it), just
implement the attributes in the driver.
Yes
On a practical note, I do wonder what it means if ACPI reports those
values.
It might simply mean that they are not supported. If so, the attributes
should not be instantiated in the first place.
Agreed. But we still can't break ABI of this driver. will retain what
it was.
I want to say that all the attributes modified above in hwmon core
are not used by other drivers, so don't break ABI of some driver.
That is not a valid argument. Especially displaying values such as
"unknown" or
starting to display units as part of an attributes _is_ an API break
since that
is completely unexpected.
Ack
These can't be solved in this driver side.
That is incorrect.
AFAICS, acpi power meter driver can't replace the deprecated API
because their sysfs interfaces will be broken if there's no any
modification in hwmon core.
That is simply wrong. The _with_info API supports non-standard
attributes
with the extra_groups parameter. Just use that and implement the
non-standard
attributes in the driver and explain why you are doing it in a comment.
Ok, I will put these attributes above into extra_groups and add some
comments for them.
Many thanks for your good suggestion.
Hi Guenter,
BTW, I have another problem as commit log described:
-->
The path of these sysfs interfaces are modified accordingly if use
hwmon_device_register_with_info():
Old: all sysfs interfaces are under acpi device, namely, path is
"/sys/class/hwmon/hwmon1/device/" ('device' in the path is a soft
link of acpi device)
Now: all sysfs interfaces are under hwmon device, namely, path is
"/sys/class/hwmon/hwmon1/"
What do you think about this?
That is as intended. The ABI states that the attributes are under
/sys/class/hwmon/hwmonX _or_ /sys/class/hwmon/hwmonX/device/.
The ABI does not guarantee that the underlying path remains the same.
Yes
libsensors handles this automatically, as should any userspace programs
accessing the attributes directly.
So we don't need to care or handle this change of syfs path, right?
/Huisong
.
