[PATCH v1 00/21] hwmon: Fix the type of 'config' in struct hwmon_channel_info to u64

[Guenter Roeck]

On 1/21/25 06:12, Armin Wolf wrote:
> Am 21.01.25 um 07:44 schrieb Huisong Li:
> 
>> 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