| Message ID | 20241130144733.51627-1-yanhuoguifan@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] hwmon: (asus-ec-sensors) add TUF GAMING X670E PLUS | expand |
On 11/30/24 06:47, Li XingYang wrote: > add asus-ec-sensors on the mainboard TUF GAMING X670E PLUS as/add/Add/ Same for subject. > support these sensors: > SENSOR_TEMP_CPU, SENSOR_TEMP_CPU_PACKAGE, SENSOR_TEMP_MB > SENSOR_TEMP_VRM, SENSOR_TEMP_WATER_IN, SENSOR_TEMP_WATER_OUT > and SENSOR_FAN_CPU_OPT > The individual sensors supported by this board are irrelevant for the patch description. > Signed-off-by: Li XingYang <yanhuoguifan@gmail.com> Please do not send new revisions of a patch as response of an older series, and please always provide a change log. > --- > Documentation/hwmon/asus_ec_sensors.rst | 1 + > drivers/hwmon/asus-ec-sensors.c | 13 +++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/Documentation/hwmon/asus_ec_sensors.rst b/Documentation/hwmon/asus_ec_sensors.rst > index ca38922f4ec5..d049a62719b0 100644 > --- a/Documentation/hwmon/asus_ec_sensors.rst > +++ b/Documentation/hwmon/asus_ec_sensors.rst > @@ -17,6 +17,7 @@ Supported boards: > * ROG CROSSHAIR VIII IMPACT > * ROG CROSSHAIR X670E HERO > * ROG CROSSHAIR X670E GENE > + * TUF GAMING X670E PLUS > * ROG MAXIMUS XI HERO > * ROG MAXIMUS XI HERO (WI-FI) > * ROG STRIX B550-E GAMING I don't understand how this is "sorted". What is the sorting criteria ? > diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c > index 9555366aeaf0..f02e4f5cc6db 100644 > --- a/drivers/hwmon/asus-ec-sensors.c > +++ b/drivers/hwmon/asus-ec-sensors.c > @@ -250,6 +250,8 @@ static const struct ec_sensor_info sensors_family_amd_600[] = { > EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x00), > [ec_sensor_temp_water_out] = > EC_SENSOR("Water_Out", hwmon_temp, 1, 0x01, 0x01), > + [ec_sensor_fan_cpu_opt] = > + EC_SENSOR("CPU_Opt", hwmon_fan, 2, 0x00, 0xb0), This is an unrelated change. It affects other boards of the same family. It needs to be a separate patch, it needs to be explained, and it needs to get some confirmation that it works on the other boards of the same series. Thanks, Guenter > }; > > static const struct ec_sensor_info sensors_family_intel_300[] = { > @@ -362,6 +364,15 @@ static const struct ec_board_info board_info_crosshair_x670e_gene = { > .family = family_amd_600_series, > }; > > +static const struct ec_board_info board_info_tuf_gaming_x670e_plus = { > + .sensors = SENSOR_TEMP_CPU | SENSOR_TEMP_CPU_PACKAGE | > + SENSOR_TEMP_MB | SENSOR_TEMP_VRM | > + SENSOR_TEMP_WATER_IN | SENSOR_TEMP_WATER_OUT | > + SENSOR_FAN_CPU_OPT, > + .mutex_path = ACPI_GLOBAL_LOCK_PSEUDO_PATH, > + .family = family_amd_600_series, > +}; > + > static const struct ec_board_info board_info_crosshair_viii_dark_hero = { > .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | > SENSOR_TEMP_T_SENSOR | > @@ -512,6 +523,8 @@ static const struct dmi_system_id dmi_table[] = { > &board_info_crosshair_x670e_hero), > DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR X670E GENE", > &board_info_crosshair_x670e_gene), > + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X670E-PLUS", > + &board_info_tuf_gaming_x670e_plus), > DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG MAXIMUS XI HERO", > &board_info_maximus_xi_hero), > DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG MAXIMUS XI HERO (WI-FI)",
Hi Günter, > > diff --git a/Documentation/hwmon/asus_ec_sensors.rst b/Documentation/hwmon/asus_ec_sensors.rst > > index ca38922f4ec5..d049a62719b0 100644 > > --- a/Documentation/hwmon/asus_ec_sensors.rst > > +++ b/Documentation/hwmon/asus_ec_sensors.rst > > @@ -17,6 +17,7 @@ Supported boards: > > * ROG CROSSHAIR VIII IMPACT > > * ROG CROSSHAIR X670E HERO > > * ROG CROSSHAIR X670E GENE > > + * TUF GAMING X670E PLUS > > * ROG MAXIMUS XI HERO > > * ROG MAXIMUS XI HERO (WI-FI) > > * ROG STRIX B550-E GAMING > > I don't understand how this is "sorted". What is the sorting criteria ? I believe the list in static const struct dmi_system_id dmi_table[] and the list in the .rst file are in the same order, and I want the board declarations to follow that. > > diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c > > index 9555366aeaf0..f02e4f5cc6db 100644 > > --- a/drivers/hwmon/asus-ec-sensors.c > > +++ b/drivers/hwmon/asus-ec-sensors.c > > @@ -250,6 +250,8 @@ static const struct ec_sensor_info sensors_family_amd_600[] = { > > EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x00), > > [ec_sensor_temp_water_out] = > > EC_SENSOR("Water_Out", hwmon_temp, 1, 0x01, 0x01), > > + [ec_sensor_fan_cpu_opt] = > > + EC_SENSOR("CPU_Opt", hwmon_fan, 2, 0x00, 0xb0), > > This is an unrelated change. It affects other boards of the same family. > It needs to be a separate patch, it needs to be explained, and it needs to > get some confirmation that it works on the other boards of the same series. Well, it is the same register as in the previous generation, and while it would be nice to confirm that it works in other models of the 600th family, I can't see how XingYang can do that. I can check with the AMD 800th series though... Kind regards, Eugene
On 11/30/24 07:47, Eugene Shalygin wrote: > Hi Günter, > >>> diff --git a/Documentation/hwmon/asus_ec_sensors.rst b/Documentation/hwmon/asus_ec_sensors.rst >>> index ca38922f4ec5..d049a62719b0 100644 >>> --- a/Documentation/hwmon/asus_ec_sensors.rst >>> +++ b/Documentation/hwmon/asus_ec_sensors.rst >>> @@ -17,6 +17,7 @@ Supported boards: >>> * ROG CROSSHAIR VIII IMPACT >>> * ROG CROSSHAIR X670E HERO >>> * ROG CROSSHAIR X670E GENE >>> + * TUF GAMING X670E PLUS >>> * ROG MAXIMUS XI HERO >>> * ROG MAXIMUS XI HERO (WI-FI) >>> * ROG STRIX B550-E GAMING >> >> I don't understand how this is "sorted". What is the sorting criteria ? > > I believe the list in static const struct dmi_system_id dmi_table[] > and the list in the .rst file are in the same order, and I want the > board declarations to follow that. > So you don't care about alphabetic order, just about using the same order in both files ? Fine with me, and I don't have to understand it, but it is a deviation from the current model and should be documented for reference to ensure that I don't call out people for not using non-alphabetic order in the future. If there is some other order, it would be even more important to document it to help people understand what it is supposed to be. >>> diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c >>> index 9555366aeaf0..f02e4f5cc6db 100644 >>> --- a/drivers/hwmon/asus-ec-sensors.c >>> +++ b/drivers/hwmon/asus-ec-sensors.c >>> @@ -250,6 +250,8 @@ static const struct ec_sensor_info sensors_family_amd_600[] = { >>> EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x00), >>> [ec_sensor_temp_water_out] = >>> EC_SENSOR("Water_Out", hwmon_temp, 1, 0x01, 0x01), >>> + [ec_sensor_fan_cpu_opt] = >>> + EC_SENSOR("CPU_Opt", hwmon_fan, 2, 0x00, 0xb0), >> >> This is an unrelated change. It affects other boards of the same family. >> It needs to be a separate patch, it needs to be explained, and it needs to >> get some confirmation that it works on the other boards of the same series. > > Well, it is the same register as in the previous generation, and while > it would be nice to confirm that it works in other models of the 600th > family, I can't see how XingYang can do that. I can check with the AMD > 800th series though... > Ok with me if you confirm it, but it still needs to be a separate patch since it it not about adding support for a specific board. Guenter
On Sat, Nov 30, 2024 at 07:29:21AM -0800, Guenter Roeck wrote: > > support these sensors: > > SENSOR_TEMP_CPU, SENSOR_TEMP_CPU_PACKAGE, SENSOR_TEMP_MB > > SENSOR_TEMP_VRM, SENSOR_TEMP_WATER_IN, SENSOR_TEMP_WATER_OUT > > and SENSOR_FAN_CPU_OPT > > > > The individual sensors supported by this board are irrelevant for the > patch description. > My original intention was to describe the sensors supported by this new motherboard. If you think it's irrelevant, I can delete the descriptions of these sensors. > > Signed-off-by: Li XingYang <yanhuoguifan@gmail.com> > > Please do not send new revisions of a patch as response of an older > series, and please always provide a change log. > Sorry, I cannot fully understand this meaning. Should I use the new version of the patch to reply to the old version of the patch instead of responding to the questions raised > > --- > > Documentation/hwmon/asus_ec_sensors.rst | 1 + > > drivers/hwmon/asus-ec-sensors.c | 13 +++++++++++++ > > 2 files changed, 14 insertions(+) > > > > diff --git a/Documentation/hwmon/asus_ec_sensors.rst b/Documentation/hwmon/asus_ec_sensors.rst > > index ca38922f4ec5..d049a62719b0 100644 > > --- a/Documentation/hwmon/asus_ec_sensors.rst > > +++ b/Documentation/hwmon/asus_ec_sensors.rst > > @@ -17,6 +17,7 @@ Supported boards: > > * ROG CROSSHAIR VIII IMPACT > > * ROG CROSSHAIR X670E HERO > > * ROG CROSSHAIR X670E GENE > > + * TUF GAMING X670E PLUS > > * ROG MAXIMUS XI HERO > > * ROG MAXIMUS XI HERO (WI-FI) > > * ROG STRIX B550-E GAMING > > I don't understand how this is "sorted". What is the sorting criteria ? > > At first, I saw that the ROG CROSSHAIR X670E GENE was the last motherboard in the x670e series on this list, so I placed the new x670e motherboard after it. Now I realize that this list originally followed alphabetical order, perhaps it would be better for me to put the new motherboard first, as well as the source files first > > diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c > > index 9555366aeaf0..f02e4f5cc6db 100644 > > --- a/drivers/hwmon/asus-ec-sensors.c > > +++ b/drivers/hwmon/asus-ec-sensors.c > > @@ -250,6 +250,8 @@ static const struct ec_sensor_info sensors_family_amd_600[] = { > > EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x00), > > [ec_sensor_temp_water_out] = > > EC_SENSOR("Water_Out", hwmon_temp, 1, 0x01, 0x01), > > + [ec_sensor_fan_cpu_opt] = > > + EC_SENSOR("CPU_Opt", hwmon_fan, 2, 0x00, 0xb0), > > This is an unrelated change. It affects other boards of the same family. > It needs to be a separate patch, it needs to be explained, and it needs to > get some confirmation that it works on the other boards of the same series. > > Thanks, > Guenter I found that in the LibreHardwareMonitor project, the registers used by Amd600 to operate FanCPUOpt are described: https://github.com/LibreHardwareMonitor/LibreHardwareMonitor/blob/master/LibreHardwareMonitorLib/Hardware/Motherboard/Lpc/EC/EmbeddedController.cs BoardFamily.Amd600, new Dictionary<ECSensor, EmbeddedControllerSource> { { ECSensor.FanCPUOpt, new EmbeddedControllerSource("CPU Optional Fan", SensorType.Fan, 0x00b0, 2) }, } I think this is suitable for the AMD 600 motherboard, and it does work on my motherboard as well. Of course, I cannot guarantee that all ASUS AMD600 motherboards can use this interface, In fact, the ec_sensor_temp_t_sensor originally defined in sensors_family_amd_600 is also not applicable on my motherboard. I think sensors_family_amd_600 only provides a common interface, and the specific motherboard selection still needs to be tested. I will dismantle this part into a separate patch.
On 11/30/24 17:17, Li XingYang wrote: [ ... ] >> Please do not send new revisions of a patch as response of an older >> series, and please always provide a change log. >> > Sorry, I cannot fully understand this meaning. > Should I use the new version of the patch to reply to the old version of > the patch instead of responding to the questions raised If you send new revisions of a patch or patch series as reply to older versions of that patch series, it may get lost because it is not identified as updated patch but as reply to an older patch or patch series. Also see "Explicit In-Reply-To headers" in Documentation/process/submitting-patches.rst [ ... ] >> This is an unrelated change. It affects other boards of the same family. >> It needs to be a separate patch, it needs to be explained, and it needs to >> get some confirmation that it works on the other boards of the same series. >> >> Thanks, >> Guenter > I found that in the LibreHardwareMonitor project, > the registers used by Amd600 to operate FanCPUOpt are described: > https://github.com/LibreHardwareMonitor/LibreHardwareMonitor/blob/master/LibreHardwareMonitorLib/Hardware/Motherboard/Lpc/EC/EmbeddedController.cs > BoardFamily.Amd600, new Dictionary<ECSensor, EmbeddedControllerSource> > { > { ECSensor.FanCPUOpt, new EmbeddedControllerSource("CPU Optional Fan", SensorType.Fan, 0x00b0, 2) }, > } > > I think this is suitable for the AMD 600 motherboard, and it does work on my motherboard as well. That makes sense, but it is still unrelated to this patch and, worse, not even mentioned in the patch description. See "Separate your changes" in Documentation/process/submitting-patches.rst. Guenter
diff --git a/Documentation/hwmon/asus_ec_sensors.rst b/Documentation/hwmon/asus_ec_sensors.rst index ca38922f4ec5..d049a62719b0 100644 --- a/Documentation/hwmon/asus_ec_sensors.rst +++ b/Documentation/hwmon/asus_ec_sensors.rst @@ -17,6 +17,7 @@ Supported boards: * ROG CROSSHAIR VIII IMPACT * ROG CROSSHAIR X670E HERO * ROG CROSSHAIR X670E GENE + * TUF GAMING X670E PLUS * ROG MAXIMUS XI HERO * ROG MAXIMUS XI HERO (WI-FI) * ROG STRIX B550-E GAMING diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c index 9555366aeaf0..f02e4f5cc6db 100644 --- a/drivers/hwmon/asus-ec-sensors.c +++ b/drivers/hwmon/asus-ec-sensors.c @@ -250,6 +250,8 @@ static const struct ec_sensor_info sensors_family_amd_600[] = { EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x00), [ec_sensor_temp_water_out] = EC_SENSOR("Water_Out", hwmon_temp, 1, 0x01, 0x01), + [ec_sensor_fan_cpu_opt] = + EC_SENSOR("CPU_Opt", hwmon_fan, 2, 0x00, 0xb0), }; static const struct ec_sensor_info sensors_family_intel_300[] = { @@ -362,6 +364,15 @@ static const struct ec_board_info board_info_crosshair_x670e_gene = { .family = family_amd_600_series, }; +static const struct ec_board_info board_info_tuf_gaming_x670e_plus = { + .sensors = SENSOR_TEMP_CPU | SENSOR_TEMP_CPU_PACKAGE | + SENSOR_TEMP_MB | SENSOR_TEMP_VRM | + SENSOR_TEMP_WATER_IN | SENSOR_TEMP_WATER_OUT | + SENSOR_FAN_CPU_OPT, + .mutex_path = ACPI_GLOBAL_LOCK_PSEUDO_PATH, + .family = family_amd_600_series, +}; + static const struct ec_board_info board_info_crosshair_viii_dark_hero = { .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR | @@ -512,6 +523,8 @@ static const struct dmi_system_id dmi_table[] = { &board_info_crosshair_x670e_hero), DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR X670E GENE", &board_info_crosshair_x670e_gene), + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X670E-PLUS", + &board_info_tuf_gaming_x670e_plus), DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG MAXIMUS XI HERO", &board_info_maximus_xi_hero), DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG MAXIMUS XI HERO (WI-FI)",
add asus-ec-sensors on the mainboard TUF GAMING X670E PLUS support these sensors: SENSOR_TEMP_CPU, SENSOR_TEMP_CPU_PACKAGE, SENSOR_TEMP_MB SENSOR_TEMP_VRM, SENSOR_TEMP_WATER_IN, SENSOR_TEMP_WATER_OUT and SENSOR_FAN_CPU_OPT Signed-off-by: Li XingYang <yanhuoguifan@gmail.com> --- Documentation/hwmon/asus_ec_sensors.rst | 1 + drivers/hwmon/asus-ec-sensors.c | 13 +++++++++++++ 2 files changed, 14 insertions(+)