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

[Guenter Roeck <linux@roeck-us.net>]

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