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

[lihuisong (C)]

在 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@xxxxxxxxxx>
> > ---
> >   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
> .