在 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.
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?
/Huisong
.
