| Message ID | 20250121064519.18974-1-lihuisong@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hwmon: Fix the type of 'config' in struct hwmon_channel_info to u64 | expand |
On 21/01/2025 07:44, Huisong Li wrote: > 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. Avoid combining independent patches into one patch bomb. Or explain the dependencies and how is it supposed to be merged - that's why you have cover letter here. Best regards, Krzysztof
在 2025/1/21 15:47, Krzysztof Kozlowski 写道: > On 21/01/2025 07:44, Huisong Li wrote: >> 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. > Avoid combining independent patches into one patch bomb. Or explain the > dependencies and how is it supposed to be merged - that's why you have > cover letter here. These patches having a title ('Use HWMON_CHANNEL_INFO macro to simplify code') are also for this series. Or we need to modify the type of the 'xxx_config' array in these patches. If we directly use the macro HWMON_CHANNEL_INFO, the type of 'config' has been modifyed in patch 1/21 and these driver don't need to care this change. /Huisong > > > .
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. 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. Thanks, Armin Wolf > > Huisong Li (21): > hwmon: Fix the type of 'config' in struct hwmon_channel_info to u64 > media: video-i2c: Use HWMON_CHANNEL_INFO macro to simplify code > net: aquantia: Use HWMON_CHANNEL_INFO macro to simplify code > net: nfp: Use HWMON_CHANNEL_INFO macro to simplify code > net: phy: marvell: Use HWMON_CHANNEL_INFO macro to simplify code > net: phy: marvell10g: Use HWMON_CHANNEL_INFO macro to simplify code > rtc: ab-eoz9: Use HWMON_CHANNEL_INFO macro to simplify code > rtc: ds3232: Use HWMON_CHANNEL_INFO macro to simplify code > w1: w1_therm: w1: Use HWMON_CHANNEL_INFO macro to simplify code > net: phy: aquantia: Use HWMON_CHANNEL_INFO macro to simplify code > hwmon: (asus_wmi_sensors) Fix type of 'config' in struct > hwmon_channel_info to u64 > hwmon: (hp-wmi-sensors) Fix type of 'config' in struct > hwmon_channel_info to u64 > hwmon: (mr75203) Fix the type of 'config' in struct hwmon_channel_info > to u64 > hwmon: (pwm-fan) Fix the type of 'config' in struct hwmon_channel_info > to u64 > hwmon: (scmi-hwmon) Fix the type of 'config' in struct > hwmon_channel_info to u64 > hwmon: (tmp401) Fix the type of 'config' in struct hwmon_channel_info > to u64 > hwmon: (tmp421) Fix the type of 'config' in struct hwmon_channel_info > to u64 > net/mlx5: Fix the type of 'config' in struct hwmon_channel_info to u64 > platform/x86: dell-ddv: Fix the type of 'config' in struct > hwmon_channel_info to u64 > hwmon: (asus-ec-sensors) Fix the type of 'config' in struct > hwmon_channel_info to u64 > hwmon: (lm90) Fix the type of 'config' in struct hwmon_channel_info to > u64 > > drivers/hwmon/asus-ec-sensors.c | 6 +- > drivers/hwmon/asus_wmi_sensors.c | 8 +- > drivers/hwmon/hp-wmi-sensors.c | 6 +- > drivers/hwmon/hwmon.c | 4 +- > drivers/hwmon/lm90.c | 4 +- > drivers/hwmon/mr75203.c | 6 +- > drivers/hwmon/pwm-fan.c | 4 +- > drivers/hwmon/scmi-hwmon.c | 6 +- > drivers/hwmon/tmp401.c | 4 +- > drivers/hwmon/tmp421.c | 2 +- > drivers/media/i2c/video-i2c.c | 12 +- > .../ethernet/aquantia/atlantic/aq_drvinfo.c | 14 +- > .../net/ethernet/mellanox/mlx5/core/hwmon.c | 8 +- > .../net/ethernet/netronome/nfp/nfp_hwmon.c | 40 +-- > drivers/net/phy/aquantia/aquantia_hwmon.c | 32 +- > drivers/net/phy/marvell.c | 24 +- > drivers/net/phy/marvell10g.c | 24 +- > drivers/platform/x86/dell/dell-wmi-ddv.c | 6 +- > drivers/rtc/rtc-ab-eoz9.c | 24 +- > drivers/rtc/rtc-ds3232.c | 24 +- > drivers/w1/slaves/w1_therm.c | 12 +- > include/linux/hwmon.h | 300 +++++++++--------- > 22 files changed, 205 insertions(+), 365 deletions(-) >
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
On Tue, Jan 21, 2025 at 06:50:00AM -0800, Guenter Roeck wrote: > 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. I think something needs to budge here, because I think what you're asking is actually impossible! One can't change the type of struct hwmon_channel_info.config to be a u64 without also updating every hwmon driver that assigns to that member. This is not possible: struct hwmon_channel_info { enum hwmon_sensor_types type; - const u32 *config; + const u64 *config; }; static u32 marvell_hwmon_chip_config[] = { ... }; static const struct hwmon_channel_info marvell_hwmon_chip = { .type = hwmon_chip, .config = marvell_hwmon_chip_config, }; This assignment to .config will cause a warning/error with the above change. If instead we do: - .config = marvell_hwmon_chip_config, + .config = (u64 *)marvell_hwmon_chip_config, which would have to happen to every driver, then no, this also doesn't work, because config[1] now points beyond the bounds of marvell_hwmon_chip_config, which only has two u32 entries. You can't just change the type of struct hwmon_channel_info.config without patching every driver that assigns to struct hwmon_channel_info.config as things currently stand. The only way out of that would be: 1. convert *all* drivers that defines a config array to be defined by their own macro in hwmon.h, and then switch that macro to make the definitions be a u64 array at the same time as switching struct hwmon_channel_info.config 2. convert *all* drivers to use HWMON_CHANNEL_INFO() unconditionally, and switch that along with struct hwmon_channel_info.config. 3. add a new member to struct hwmon_channel_info such as "const u64 *config64" and then gradually convert drivers to use it. Once everyone is converted over, then remove "const u32 *config", optionally rename "config64" back to "config" and then re-patch all drivers. That'll be joyful, with multiple patches to drivers that need to be merged in sync with hwmon changes - and last over several kernel release cycles. This is not going to be an easy change!
On 1/21/25 09:20, Russell King (Oracle) wrote: [ ... ] > > 1. convert *all* drivers that defines a config array to be defined by > their own macro in hwmon.h, and then switch that macro to make the > definitions be a u64 array at the same time as switching struct > hwmon_channel_info.config > > 2. convert *all* drivers to use HWMON_CHANNEL_INFO() unconditionally, > and switch that along with struct hwmon_channel_info.config. > > 3. add a new member to struct hwmon_channel_info such as > "const u64 *config64" and then gradually convert drivers to use it. > Once everyone is converted over, then remove "const u32 *config", > optionally rename "config64" back to "config" and then re-patch all > drivers. That'll be joyful, with multiple patches to drivers that > need to be merged in sync with hwmon changes - and last over several > kernel release cycles. > Alternatively, add another sensor type for the overflowing field, such as hwmon_power_2 (or whatever), and use it for the additional attributes. > This is not going to be an easy change! > Neither is it necessary at this time. Guenter
在 2025/1/21 22:12, Armin Wolf 写道: > 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. > > Hi, > > what kind of new power attributes do you want to add to the hwmon API? The attributes you list is right. > > 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. > This is a good idea. Thanks. > >> >> Huisong Li (21): >> hwmon: Fix the type of 'config' in struct hwmon_channel_info to u64 >> media: video-i2c: Use HWMON_CHANNEL_INFO macro to simplify code >> net: aquantia: Use HWMON_CHANNEL_INFO macro to simplify code >> net: nfp: Use HWMON_CHANNEL_INFO macro to simplify code >> net: phy: marvell: Use HWMON_CHANNEL_INFO macro to simplify code >> net: phy: marvell10g: Use HWMON_CHANNEL_INFO macro to simplify code >> rtc: ab-eoz9: Use HWMON_CHANNEL_INFO macro to simplify code >> rtc: ds3232: Use HWMON_CHANNEL_INFO macro to simplify code >> w1: w1_therm: w1: Use HWMON_CHANNEL_INFO macro to simplify code >> net: phy: aquantia: Use HWMON_CHANNEL_INFO macro to simplify code >> hwmon: (asus_wmi_sensors) Fix type of 'config' in struct >> hwmon_channel_info to u64 >> hwmon: (hp-wmi-sensors) Fix type of 'config' in struct >> hwmon_channel_info to u64 >> hwmon: (mr75203) Fix the type of 'config' in struct >> hwmon_channel_info >> to u64 >> hwmon: (pwm-fan) Fix the type of 'config' in struct >> hwmon_channel_info >> to u64 >> hwmon: (scmi-hwmon) Fix the type of 'config' in struct >> hwmon_channel_info to u64 >> hwmon: (tmp401) Fix the type of 'config' in struct hwmon_channel_info >> to u64 >> hwmon: (tmp421) Fix the type of 'config' in struct hwmon_channel_info >> to u64 >> net/mlx5: Fix the type of 'config' in struct hwmon_channel_info to >> u64 >> platform/x86: dell-ddv: Fix the type of 'config' in struct >> hwmon_channel_info to u64 >> hwmon: (asus-ec-sensors) Fix the type of 'config' in struct >> hwmon_channel_info to u64 >> hwmon: (lm90) Fix the type of 'config' in struct >> hwmon_channel_info to >> u64 >> >> drivers/hwmon/asus-ec-sensors.c | 6 +- >> drivers/hwmon/asus_wmi_sensors.c | 8 +- >> drivers/hwmon/hp-wmi-sensors.c | 6 +- >> drivers/hwmon/hwmon.c | 4 +- >> drivers/hwmon/lm90.c | 4 +- >> drivers/hwmon/mr75203.c | 6 +- >> drivers/hwmon/pwm-fan.c | 4 +- >> drivers/hwmon/scmi-hwmon.c | 6 +- >> drivers/hwmon/tmp401.c | 4 +- >> drivers/hwmon/tmp421.c | 2 +- >> drivers/media/i2c/video-i2c.c | 12 +- >> .../ethernet/aquantia/atlantic/aq_drvinfo.c | 14 +- >> .../net/ethernet/mellanox/mlx5/core/hwmon.c | 8 +- >> .../net/ethernet/netronome/nfp/nfp_hwmon.c | 40 +-- >> drivers/net/phy/aquantia/aquantia_hwmon.c | 32 +- >> drivers/net/phy/marvell.c | 24 +- >> drivers/net/phy/marvell10g.c | 24 +- >> drivers/platform/x86/dell/dell-wmi-ddv.c | 6 +- >> drivers/rtc/rtc-ab-eoz9.c | 24 +- >> drivers/rtc/rtc-ds3232.c | 24 +- >> drivers/w1/slaves/w1_therm.c | 12 +- >> include/linux/hwmon.h | 300 +++++++++--------- >> 22 files changed, 205 insertions(+), 365 deletions(-) >> > .
Hi Guenter, 在 2025/1/21 22:50, 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. Making space for new attributes later. After all hwmon core have ability to do that. > >> 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). I plan to accept the suggestion Armin proposed that acpi_power_meter can use the "extra_groups" parameter for these "not generic hwmon power attributes". Should we drop the power1_ prefix? But this will lead to the change of these attributes. Do you mean 'power1_is_battery' can be added to hwmon power attributes in hwmon.h? > > 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. > The bigest obstacle is that drivers which not use HWMON_CHANNEL_INFO hwmon provided need to be modifyed.
On 1/21/25 18:52, lihuisong (C) wrote: >> 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). > I plan to accept the suggestion Armin proposed that acpi_power_meter can use the "extra_groups" parameter for these "not generic hwmon power attributes". > Should we drop the power1_ prefix? But this will lead to the change of these attributes. > Do you mean 'power1_is_battery' can be added to hwmon power attributes in hwmon.h? No; power1_is_battery is still not a hardware monitoring attribute, much less one to standardize. Also, don't change any attribute names. They are wrong, but they are the ABI for this driver. Guenter
在 2025/1/22 11:19, Guenter Roeck 写道: > On 1/21/25 18:52, lihuisong (C) wrote: > >>> 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). >> I plan to accept the suggestion Armin proposed that acpi_power_meter >> can use the "extra_groups" parameter for these "not generic hwmon >> power attributes". >> Should we drop the power1_ prefix? But this will lead to the change >> of these attributes. >> Do you mean 'power1_is_battery' can be added to hwmon power >> attributes in hwmon.h? > > No; power1_is_battery is still not a hardware monitoring attribute, > much less one to standardize. Also, don't change any attribute names. > They are wrong, but they are the ABI for this driver. > Ok, it's clear now. > .
Hi 在 2025/1/22 1:20, Russell King (Oracle) 写道: > On Tue, Jan 21, 2025 at 06:50:00AM -0800, Guenter Roeck wrote: >> 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. > I think something needs to budge here, because I think what you're > asking is actually impossible! > > One can't change the type of struct hwmon_channel_info.config to be a > u64 without also updating every hwmon driver that assigns to that > member. > > This is not possible: > > struct hwmon_channel_info { > enum hwmon_sensor_types type; > - const u32 *config; > + const u64 *config; > }; > > static u32 marvell_hwmon_chip_config[] = { > ... > }; > > static const struct hwmon_channel_info marvell_hwmon_chip = { > .type = hwmon_chip, > .config = marvell_hwmon_chip_config, > }; > > This assignment to .config will cause a warning/error with the above > change. If instead we do: > > - .config = marvell_hwmon_chip_config, > + .config = (u64 *)marvell_hwmon_chip_config, > > which would have to happen to every driver, then no, this also doesn't > work, because config[1] now points beyond the bounds of > marvell_hwmon_chip_config, which only has two u32 entries. > > You can't just change the type of struct hwmon_channel_info.config > without patching every driver that assigns to > struct hwmon_channel_info.config as things currently stand. > > The only way out of that would be: > > 1. convert *all* drivers that defines a config array to be defined by > their own macro in hwmon.h, and then switch that macro to make the > definitions be a u64 array at the same time as switching struct > hwmon_channel_info.config > > 2. convert *all* drivers to use HWMON_CHANNEL_INFO() unconditionally, > and switch that along with struct hwmon_channel_info.config. > > 3. add a new member to struct hwmon_channel_info such as > "const u64 *config64" and then gradually convert drivers to use it. > Once everyone is converted over, then remove "const u32 *config", > optionally rename "config64" back to "config" and then re-patch all > drivers. That'll be joyful, with multiple patches to drivers that > need to be merged in sync with hwmon changes - and last over several > kernel release cycles. > > This is not going to be an easy change! Yeah, it's a very time-consuming method and not easy to change. Although some attributes of acpi_power_meter, like power1_model_number, can not add to the generic hwmon power attributes, I still don't think the maximum attribute number of one sensor type doesn't need to limit to 32. We can not make sure that the current generic attributes can fully satisfy the useage in furture. I got an idea. it may just need one patch in hwmon core, like the following: --> struct hwmon_channel_info { enum hwmon_sensor_types type; - const u32 *config; + union { + const u32 *config; + const u64 *config64; + } }; #define HWMON_CHANNEL_INFO(stype, ...) \ @@ -444,12 +447,22 @@ struct hwmon_channel_info { } \ }) +#define HWMON_CHANNEL_INFO64(stype, ...) \ + (&(const struct hwmon_channel_info) { \ + .type = hwmon_##stype, \ + .config64 = (const u64 []) { \ + __VA_ARGS__, 0 \ + } \ + }) + + /** * struct hwmon_chip_info - Chip configuration * @ops: Pointer to hwmon operations. * @info: Null-terminated list of channel information. */ struct hwmon_chip_info { + bool hwmon_attribute_bit64; // use config64 pointer if it is true. const struct hwmon_ops *ops; const struct hwmon_channel_info * const *info; }; For hwmon core code, we can use the 'config' or 'confit64' and compute attribute number based on the 'hwmon_attribute_bit64' value. New driver can use HWMON_CHANNEL_INFO64 macro. Old drivers are not supposed to need to have any modification. /Huisong >
On 21/01/2025 09:14, lihuisong (C) wrote: > > 在 2025/1/21 15:47, Krzysztof Kozlowski 写道: >> On 21/01/2025 07:44, Huisong Li wrote: >>> 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. >> Avoid combining independent patches into one patch bomb. Or explain the >> dependencies and how is it supposed to be merged - that's why you have >> cover letter here. > These patches having a title ('Use HWMON_CHANNEL_INFO macro to simplify > code') are also for this series. > Or we need to modify the type of the 'xxx_config' array in these patches. > If we directly use the macro HWMON_CHANNEL_INFO, the type of 'config' > has been modifyed in patch 1/21 and these driver don't need to care this > change. None of above addresses my concern. I am dropping the series from my inbox/to-review box. Best regards, Krzysztof
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. Huisong Li (21): hwmon: Fix the type of 'config' in struct hwmon_channel_info to u64 media: video-i2c: Use HWMON_CHANNEL_INFO macro to simplify code net: aquantia: Use HWMON_CHANNEL_INFO macro to simplify code net: nfp: Use HWMON_CHANNEL_INFO macro to simplify code net: phy: marvell: Use HWMON_CHANNEL_INFO macro to simplify code net: phy: marvell10g: Use HWMON_CHANNEL_INFO macro to simplify code rtc: ab-eoz9: Use HWMON_CHANNEL_INFO macro to simplify code rtc: ds3232: Use HWMON_CHANNEL_INFO macro to simplify code w1: w1_therm: w1: Use HWMON_CHANNEL_INFO macro to simplify code net: phy: aquantia: Use HWMON_CHANNEL_INFO macro to simplify code hwmon: (asus_wmi_sensors) Fix type of 'config' in struct hwmon_channel_info to u64 hwmon: (hp-wmi-sensors) Fix type of 'config' in struct hwmon_channel_info to u64 hwmon: (mr75203) Fix the type of 'config' in struct hwmon_channel_info to u64 hwmon: (pwm-fan) Fix the type of 'config' in struct hwmon_channel_info to u64 hwmon: (scmi-hwmon) Fix the type of 'config' in struct hwmon_channel_info to u64 hwmon: (tmp401) Fix the type of 'config' in struct hwmon_channel_info to u64 hwmon: (tmp421) Fix the type of 'config' in struct hwmon_channel_info to u64 net/mlx5: Fix the type of 'config' in struct hwmon_channel_info to u64 platform/x86: dell-ddv: Fix the type of 'config' in struct hwmon_channel_info to u64 hwmon: (asus-ec-sensors) Fix the type of 'config' in struct hwmon_channel_info to u64 hwmon: (lm90) Fix the type of 'config' in struct hwmon_channel_info to u64 drivers/hwmon/asus-ec-sensors.c | 6 +- drivers/hwmon/asus_wmi_sensors.c | 8 +- drivers/hwmon/hp-wmi-sensors.c | 6 +- drivers/hwmon/hwmon.c | 4 +- drivers/hwmon/lm90.c | 4 +- drivers/hwmon/mr75203.c | 6 +- drivers/hwmon/pwm-fan.c | 4 +- drivers/hwmon/scmi-hwmon.c | 6 +- drivers/hwmon/tmp401.c | 4 +- drivers/hwmon/tmp421.c | 2 +- drivers/media/i2c/video-i2c.c | 12 +- .../ethernet/aquantia/atlantic/aq_drvinfo.c | 14 +- .../net/ethernet/mellanox/mlx5/core/hwmon.c | 8 +- .../net/ethernet/netronome/nfp/nfp_hwmon.c | 40 +-- drivers/net/phy/aquantia/aquantia_hwmon.c | 32 +- drivers/net/phy/marvell.c | 24 +- drivers/net/phy/marvell10g.c | 24 +- drivers/platform/x86/dell/dell-wmi-ddv.c | 6 +- drivers/rtc/rtc-ab-eoz9.c | 24 +- drivers/rtc/rtc-ds3232.c | 24 +- drivers/w1/slaves/w1_therm.c | 12 +- include/linux/hwmon.h | 300 +++++++++--------- 22 files changed, 205 insertions(+), 365 deletions(-)