2023-10-19 03:27:54

by Ellie Hermaszewska

[permalink] [raw]
Subject: [PATCH] hwmon: (asus-ec-sensors) add ROG Crosshair X670E Gene.

Only the temp sensors that I can verify are present. T_Sensor is the
temperature reading of a 10kΩ β=3435K NTC thermistor optionally
connected to the T_SENSOR header.

Signed-off-by: Ellie Hermaszewska <[email protected]>
---
drivers/hwmon/asus-ec-sensors.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
index 51f9c2db403e..36f9e38000d5 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -244,6 +244,8 @@ static const struct ec_sensor_info sensors_family_amd_600[] = {
EC_SENSOR("Motherboard", hwmon_temp, 1, 0x00, 0x32),
[ec_sensor_temp_vrm] =
EC_SENSOR("VRM", hwmon_temp, 1, 0x00, 0x33),
+ [ec_sensor_temp_t_sensor] =
+ EC_SENSOR("T_Sensor", hwmon_temp, 1, 0x00, 0x36),
[ec_sensor_temp_water_in] =
EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x00),
[ec_sensor_temp_water_out] =
@@ -344,6 +346,14 @@ static const struct ec_board_info board_info_crosshair_x670e_hero = {
.family = family_amd_600_series,
};

+static const struct ec_board_info board_info_crosshair_x670e_gene = {
+ .sensors = SENSOR_TEMP_CPU | SENSOR_TEMP_CPU_PACKAGE |
+ SENSOR_TEMP_T_SENSOR |
+ SENSOR_TEMP_MB | SENSOR_TEMP_VRM,
+ .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 |
@@ -490,6 +500,8 @@ static const struct dmi_system_id dmi_table[] = {
&board_info_crosshair_viii_hero),
DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR X670E HERO",
&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("ROG MAXIMUS XI HERO",
&board_info_maximus_xi_hero),
DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG MAXIMUS XI HERO (WI-FI)",
--
2.42.0


2023-10-19 09:43:38

by Eugene Shalygin

[permalink] [raw]
Subject: Re: [PATCH] hwmon: (asus-ec-sensors) add ROG Crosshair X670E Gene.

Hi,

Thank you for submitting the patch! I don't understand how does your
note that only the T_Sensor presence can be verified correlate with
SENSOR_TEMP_CPU | SENSOR_TEMP_CPU_PACKAGE |
SENSOR_TEMP_MB | SENSOR_TEMP_VRM enabled. Could you clarify, please?

Based on the EC registers dump you provided [1], I believe it is safe
to enable Water_In and Water_Oout sensors as well.

And please add the board name to list in Documentation/hwmon/asus_ec_sensors.rst

Cheers,
Eugene

[1] https://github.com/zeule/asus-ec-sensors/issues/42#issuecomment-1742062260

2023-10-19 11:33:14

by Eugene Shalygin

[permalink] [raw]
Subject: Re: [PATCH] hwmon: (asus-ec-sensors) add ROG Crosshair X670E Gene.

Oh, sorry, I misread the board name. Please scratch the bits about water temps.

Cheers,
Eugene

On Thu, 19 Oct 2023 at 11:43, Eugene Shalygin <[email protected]> wrote:
>
> Hi,
>
> Thank you for submitting the patch! I don't understand how does your
> note that only the T_Sensor presence can be verified correlate with
> SENSOR_TEMP_CPU | SENSOR_TEMP_CPU_PACKAGE |
> SENSOR_TEMP_MB | SENSOR_TEMP_VRM enabled. Could you clarify, please?
>
> Based on the EC registers dump you provided [1], I believe it is safe
> to enable Water_In and Water_Oout sensors as well.
>
> And please add the board name to list in Documentation/hwmon/asus_ec_sensors.rst
>
> Cheers,
> Eugene
>
> [1] https://github.com/zeule/asus-ec-sensors/issues/42#issuecomment-1742062260

2023-10-19 13:57:58

by Ellie Hermaszewska

[permalink] [raw]
Subject: [PATCH v2] hwmon: (asus-ec-sensors) add ROG Crosshair X670E Gene.

These are two separate statements, describing the set of sensors
implemented (those I could verify) and the specifics of the T_Sensor new
to the X670E motherboards.

Best wishes,
Ellie

Only the temp sensors that I can verify are present.

T_Sensor is the temperature reading of a 10kΩ β=3435K NTC thermistor
optionally connected to the T_SENSOR header.

The other sensors are as found on the X670E HERO.

Signed-off-by: Ellie Hermaszewska <[email protected]>
---
Documentation/hwmon/asus_ec_sensors.rst | 1 +
drivers/hwmon/asus-ec-sensors.c | 12 ++++++++++++
2 files changed, 13 insertions(+)

diff --git a/Documentation/hwmon/asus_ec_sensors.rst b/Documentation/hwmon/asus_ec_sensors.rst
index 7e3cd5b6686f..0bf99ba406dd 100644
--- a/Documentation/hwmon/asus_ec_sensors.rst
+++ b/Documentation/hwmon/asus_ec_sensors.rst
@@ -15,6 +15,7 @@ Supported boards:
* ROG CROSSHAIR VIII HERO
* ROG CROSSHAIR VIII IMPACT
* ROG CROSSHAIR X670E HERO
+ * ROG CROSSHAIR X670E GENE
* 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 51f9c2db403e..36f9e38000d5 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -244,6 +244,8 @@ static const struct ec_sensor_info sensors_family_amd_600[] = {
EC_SENSOR("Motherboard", hwmon_temp, 1, 0x00, 0x32),
[ec_sensor_temp_vrm] =
EC_SENSOR("VRM", hwmon_temp, 1, 0x00, 0x33),
+ [ec_sensor_temp_t_sensor] =
+ EC_SENSOR("T_Sensor", hwmon_temp, 1, 0x00, 0x36),
[ec_sensor_temp_water_in] =
EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x00),
[ec_sensor_temp_water_out] =
@@ -344,6 +346,14 @@ static const struct ec_board_info board_info_crosshair_x670e_hero = {
.family = family_amd_600_series,
};

+static const struct ec_board_info board_info_crosshair_x670e_gene = {
+ .sensors = SENSOR_TEMP_CPU | SENSOR_TEMP_CPU_PACKAGE |
+ SENSOR_TEMP_T_SENSOR |
+ SENSOR_TEMP_MB | SENSOR_TEMP_VRM,
+ .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 |
@@ -490,6 +500,8 @@ static const struct dmi_system_id dmi_table[] = {
&board_info_crosshair_viii_hero),
DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR X670E HERO",
&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("ROG MAXIMUS XI HERO",
&board_info_maximus_xi_hero),
DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG MAXIMUS XI HERO (WI-FI)",
--
2.42.0

2023-10-25 19:36:08

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2] hwmon: (asus-ec-sensors) add ROG Crosshair X670E Gene.

On Thu, Oct 19, 2023 at 09:51:58PM +0800, Ellie Hermaszewska wrote:
> These are two separate statements, describing the set of sensors
> implemented (those I could verify) and the specifics of the T_Sensor new
> to the X670E motherboards.
>
> Best wishes,
> Ellie
>
> Only the temp sensors that I can verify are present.
>
> T_Sensor is the temperature reading of a 10kΩ β=3435K NTC thermistor
> optionally connected to the T_SENSOR header.
>
> The other sensors are as found on the X670E HERO.
>

This is not an acceptable commit description.

Guenter

2023-10-26 04:47:21

by Ellie Hermaszewska

[permalink] [raw]
Subject: Re: Re: [PATCH v2] hwmon: (asus-ec-sensors) add ROG Crosshair X670E Gene.

On 10/26/23 03:35, Guenter Roeck wrote:
> This is not an acceptable commit description.

This is not acceptable feedback.

I am unable to accept it because it is not clear to me what you think
should be changed.

Is it because I misplaced the message to Eugene? Is it because of the
Greek characters? Is is not descriptive enough of the change, or in the
wrong tense, or has incorrect punctuation? Do I need to include my
testing methodology?

If it's only something minor, then please also feel free to correct it
yourself before applying. If you can't or it's not something minor,
then please let me know what ought to change and I can try to correct it.

If you don't let me know, then I will have to guess and possibly waste
everyone's time further.

Ellie

2023-10-26 05:01:53

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2] hwmon: (asus-ec-sensors) add ROG Crosshair X670E Gene.

On 10/25/23 21:46, Ellie Hermaszewska wrote:
> On 10/26/23 03:35, Guenter Roeck wrote:
> > This is not an acceptable commit description.
>
> This is not acceptable feedback.
>
> I am unable to accept it because it is not clear to me what you think
> should be changed.
>
> Is it because I misplaced the message to Eugene? Is it because of the
> Greek characters? Is is not descriptive enough of the change, or in the
> wrong tense, or has incorrect punctuation? Do I need to include my
> testing methodology?
>
> If it's only something minor, then please also feel free to correct it
> yourself before applying. If you can't or it's not something minor,
> then please let me know what ought to change and I can try to correct it.
>
> If you don't let me know, then I will have to guess and possibly waste everyone's time further.
>

Please consider reading and following the directions in
Documentation/process/submitting-patches.rst.

Thanks,
Guenter

2023-10-26 10:41:31

by Ellie Hermaszewska

[permalink] [raw]
Subject: Re: [PATCH v2] hwmon: (asus-ec-sensors) add ROG Crosshair X670E Gene.

On 10/26/23 13:01, Guenter Roeck wrote:
> On 10/25/23 21:46, Ellie Hermaszewska wrote:
>> On 10/26/23 03:35, Guenter Roeck wrote:
>>  > This is not an acceptable commit description.
>>
>> This is not acceptable feedback.
>>
>> I am unable to accept it because it is not clear to me what you think
>> should be changed.
>>
>> Is it because I misplaced the message to Eugene? Is it because of the
>> Greek characters? Is is not descriptive enough of the change, or in the
>> wrong tense, or has incorrect punctuation? Do I need to include my
>> testing methodology?
>>
>> If it's only something minor, then please also feel free to correct it
>> yourself before applying. If you can't or it's not something minor,
>> then please let me know what ought to change and I can try to correct it.
>>
>> If you don't let me know, then I will have to guess and possibly waste
>> everyone's time further.
>>
>
> Please consider reading and following the directions in
> Documentation/process/submitting-patches.rst.

I will guess that it was my misplaced reply, and submit again without
that part.

Thank you for your time.

2023-10-26 10:44:24

by Ellie Hermaszewska

[permalink] [raw]
Subject: [PATCH v3] hwmon: (asus-ec-sensors) add ROG Crosshair X670E Gene.

Only the temp sensors that I can verify are present.

T_Sensor is the temperature reading of a 10kΩ β=3435K NTC thermistor
optionally connected to the T_SENSOR header.

The other sensors are as found on the X670E Hero.

Signed-off-by: Ellie Hermaszewska <[email protected]>
---
Documentation/hwmon/asus_ec_sensors.rst | 1 +
drivers/hwmon/asus-ec-sensors.c | 12 ++++++++++++
2 files changed, 13 insertions(+)

diff --git a/Documentation/hwmon/asus_ec_sensors.rst b/Documentation/hwmon/asus_ec_sensors.rst
index 7e3cd5b6686f..0bf99ba406dd 100644
--- a/Documentation/hwmon/asus_ec_sensors.rst
+++ b/Documentation/hwmon/asus_ec_sensors.rst
@@ -15,6 +15,7 @@ Supported boards:
* ROG CROSSHAIR VIII HERO
* ROG CROSSHAIR VIII IMPACT
* ROG CROSSHAIR X670E HERO
+ * ROG CROSSHAIR X670E GENE
* 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 51f9c2db403e..36f9e38000d5 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -244,6 +244,8 @@ static const struct ec_sensor_info sensors_family_amd_600[] = {
EC_SENSOR("Motherboard", hwmon_temp, 1, 0x00, 0x32),
[ec_sensor_temp_vrm] =
EC_SENSOR("VRM", hwmon_temp, 1, 0x00, 0x33),
+ [ec_sensor_temp_t_sensor] =
+ EC_SENSOR("T_Sensor", hwmon_temp, 1, 0x00, 0x36),
[ec_sensor_temp_water_in] =
EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x00),
[ec_sensor_temp_water_out] =
@@ -344,6 +346,14 @@ static const struct ec_board_info board_info_crosshair_x670e_hero = {
.family = family_amd_600_series,
};

+static const struct ec_board_info board_info_crosshair_x670e_gene = {
+ .sensors = SENSOR_TEMP_CPU | SENSOR_TEMP_CPU_PACKAGE |
+ SENSOR_TEMP_T_SENSOR |
+ SENSOR_TEMP_MB | SENSOR_TEMP_VRM,
+ .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 |
@@ -490,6 +500,8 @@ static const struct dmi_system_id dmi_table[] = {
&board_info_crosshair_viii_hero),
DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR X670E HERO",
&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("ROG MAXIMUS XI HERO",
&board_info_maximus_xi_hero),
DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG MAXIMUS XI HERO (WI-FI)",
--
2.42.0

2023-10-26 14:42:36

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2] hwmon: (asus-ec-sensors) add ROG Crosshair X670E Gene.

On 10/26/23 03:41, Ellie Hermaszewska wrote:
> On 10/26/23 13:01, Guenter Roeck wrote:
>> On 10/25/23 21:46, Ellie Hermaszewska wrote:
>>> On 10/26/23 03:35, Guenter Roeck wrote:
>>>  > This is not an acceptable commit description.
>>>
>>> This is not acceptable feedback.
>>>
>>> I am unable to accept it because it is not clear to me what you think
>>> should be changed.
>>>
>>> Is it because I misplaced the message to Eugene? Is it because of the
>>> Greek characters? Is is not descriptive enough of the change, or in the
>>> wrong tense, or has incorrect punctuation? Do I need to include my
>>> testing methodology?
>>>
>>> If it's only something minor, then please also feel free to correct it
>>> yourself before applying. If you can't or it's not something minor,
>>> then please let me know what ought to change and I can try to correct it.
>>>
>>> If you don't let me know, then I will have to guess and possibly waste everyone's time further.
>>>
>>
>> Please consider reading and following the directions in
>> Documentation/process/submitting-patches.rst.
>
> I will guess that it was my misplaced reply, and submit again without that part.
>

From the document:

> Other comments relevant only to the moment or the maintainer, not
> suitable for the permanent changelog, should also go here. A good
> example of such comments might be ``patch changelogs`` which describe
> what has changed between the v1 and v2 version of the patch.

> Please put this information **after** the ``---`` line which separates
> the changelog from the rest of the patch. The version information is
> not part of the changelog which gets committed to the git tree. It is
> additional information for the reviewers. If it's placed above the
> commit tags, it needs manual interaction to remove it. If it is below
> the separator line, it gets automatically stripped off when applying the
> patch::
> ...
> [ ... ] When sending a next
> version, add a ``patch changelog`` to the cover letter or to individual patches
> explaining difference against previous submission

Keeping the patch description clean does not mean to _drop_ the changelog
or additional information not intended to be added to the commit.

Guenter

2023-10-26 14:46:34

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v3] hwmon: (asus-ec-sensors) add ROG Crosshair X670E Gene.

On Thu, Oct 26, 2023 at 06:43:22PM +0800, Ellie Hermaszewska wrote:
> Only the temp sensors that I can verify are present.
>
> T_Sensor is the temperature reading of a 10kΩ β=3435K NTC thermistor
> optionally connected to the T_SENSOR header.
>
> The other sensors are as found on the X670E Hero.
>
> Signed-off-by: Ellie Hermaszewska <[email protected]>

Applied, but please provide changelogs in the future.

Guenter