On 1/21/25 06:12, Armin Wolf wrote:

Am 21.01.25 um 07:44 schrieb Huisong Li:

quoted

The hwmon_device_register() is deprecated. When I try to repace it with
hwmon_device_register_with_info() for acpi_power_meter driver, I found that
the power channel attribute in linux/hwmon.h have to extend and is more
than 32 after this replacement.

However, the maximum number of hwmon channel attributes is 32 which is
limited by current hwmon codes. This is not good to add new channel
attribute for some hwmon sensor type and support more channel attribute.

This series are aimed to do this. And also make sure that acpi_power_meter
driver can successfully replace the deprecated hwmon_device_register()
later.

This explanation completely misses the point. The series tries to make space
for additional "standard" attributes. Such a change should be independent
of a driver conversion and be discussed on its own, not in the context
of a new driver or a driver conversion.

Hi,

what kind of new power attributes do you want to add to the hwmon API?

AFAIK the acpi-power-meter driver supports the following attributes:

     power1_accuracy            -> HWMON_P_ACCURACY
     power1_cap_min            -> HWMON_P_CAP_MIN
     power1_cap_max            -> HWMON_P_CAP_MAX
     power1_cap_hyst            -> HWMON_P_CAP_HYST
     power1_cap            -> HWMON_P_CAP
     power1_average            -> HWMON_P_AVERAGE
     power1_average_min        -> HWMON_P_AVERAGE_MIN
     power1_average_max        -> HWMON_P_AVERAGE_MAX
     power1_average_interval        -> HWMON_P_AVERAGE_INTERVAL
     power1_average_interval_min    -> HWMON_P_AVERAGE_INTERVAL_MIN
     power1_average_interval_max    -> HWMON_P_AVERAGE_INTERVAL_MAX
     power1_alarm            -> HWMON_P_ALARM
     power1_model_number
     power1_oem_info
     power1_serial_number
     power1_is_battery
     name                -> handled by hwmon core

The remaining attributes are in my opinion not generic enough to add them to the generic
hwmon power attributes. I think you should implement them as a attribute_group which can
be passed to hwmon_device_register_with_info() using the "extra_groups" parameter.

I absolutely agree. More specifically, it looks like this is about the following
attributes.

 >      power1_model_number
 >      power1_oem_info
 >      power1_serial_number
 >      power1_is_battery

Those are not hwmon attributes and should not be (or have been) exposed
as sysfs attributes in the first place, but (if really wanted/needed)
through debugfs files. Even _if_ exposed as sysfs attributes they should
not have the power1_ prefix (except maybe for the last one).

On top of that, doubling the size of configuration bits for everything
because one sensor type needs more than 32 bits seems excessive.
If we ever get to that point I think I'd rather introduce a second
sensor type for power sensor attributes.

Guenter