Re: [PATCH v2] hwmon: (acpi_power_meter) Replace the deprecated hwmon_device_register

["lihuisong (C)" <lihuisong@huawei.com>]

在 2025/3/14 23:01, Guenter Roeck 写道:
> On 3/14/25 01:18, 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.
>>
>> These attributes, 'power_accuracy', 'power_cap_hyst', 
>> 'power_average_min'
>> and 'power_average_max', should have been placed in hwmon_chip_info as
>> power data type. But these attributes are displayed as string format on
>> the following case:
>> 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.
>> To avoid any changes in the display of these sysfs interfaces, we can't
>> modifiy the type of these attributes in hwmon core and have to put them
>> to extra_groups.
>>
>> Please note that 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,
>>       /sys/class/hwmon/hwmonX/device/
>> now: all sysfs interfaces are under hwmon device, namely,
>>       /sys/class/hwmon/hwmonX/
>> The new ABI does not guarantee that the underlying path remains the 
>> same.
>> But we have to accept this change so as to replace the deprecated API.
>> Fortunately, some userspace application, like libsensors, would scan
>> the two path and handles this automatically. So to drop the deprecated
>> message, we can accept this change.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   drivers/hwmon/acpi_power_meter.c | 861 +++++++++++++++----------------
>>   1 file changed, 427 insertions(+), 434 deletions(-)
>>
>> diff --git a/drivers/hwmon/acpi_power_meter.c 
>> b/drivers/hwmon/acpi_power_meter.c
>> index f05986e4f379..46e8a0b6b210 100644
>> --- a/drivers/hwmon/acpi_power_meter.c
>> +++ b/drivers/hwmon/acpi_power_meter.c
> ...
>> +
>> +// depend on POWER_METER_CAN_TRIP
>
> Please di not intermix C++ comments with a driver using C comments.
Ack
>
>> +static DEVICE_ATTR_RW(power1_average_max);
>> +static DEVICE_ATTR_RW(power1_average_min);
>> +
>> +// depend on POWER_METER_CAN_CAP
>> +static DEVICE_ATTR_RO(power1_cap_hyst);
>> +
>> +// depend on POWER_METER_CAN_MEASURE
>> +static DEVICE_ATTR_RO(power1_accuracy);
>> +static DEVICE_ATTR_RO(power1_is_battery);
>> +
>> +static DEVICE_ATTR_RO(power1_model_number);
>> +static DEVICE_ATTR_RO(power1_oem_info);
>> +static DEVICE_ATTR_RO(power1_serial_number);
>> +
>> +#define EXTRA_FIRST_DYNAMIC_ATTR_ID    3
>> +#define EXTRA_ATTR_MAX    10
>> +
>> +static struct attribute *power_extra_attrs[EXTRA_ATTR_MAX] = {
>> +    &dev_attr_power1_model_number.attr,
>> +    &dev_attr_power1_oem_info.attr,
>> +    &dev_attr_power1_serial_number.attr,
>> +    NULL
>> +};
>> +
>> +ATTRIBUTE_GROUPS(power_extra);
>> +
>> +static void fill_extra_dynamic_attr(struct attribute *attr)
>> +{
>> +    int idx = EXTRA_FIRST_DYNAMIC_ATTR_ID;
>> +
>> +    for (idx = EXTRA_FIRST_DYNAMIC_ATTR_ID; idx < EXTRA_ATTR_MAX; 
>> idx++) {
>> +        if (!power_extra_attrs[idx])
>> +            break;
>> +    }
>> +
>> +    power_extra_attrs[idx] = attr;
>> +}
>> +
>
> Please use the .is_visible() callback in attribute_group to determine
> attribute visibility.
>
> That means you'll have to code attribute_group manually, but that is
> still better than re-implementing the is_visible() callback.
> Something like
>
> static const struct attribute_group power_extra_group = {
>     .attrs = power_extra_attrs,
>     .is_visible = power_extra_is_visible;
> };
>
> __ATTRIBUTE_GROUPS(power_extra);
>
> should do.
>
Hi Guenter,

Already modified it. Please review patch v3.
Thanks.

/Huisong
> .