Re: [PATCH v2] hwmon: (asus-ec-sensors) add TUF GAMING X670E PLUS

[Li XingYang <yanhuoguifan@gmail.com>]

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.