search

zeule / asus-ec-sensors

Linux HWMON sensors driver for ASUS motherboards to get sensor readings from the embedded controller
GNU General Public License v2.0
46 stars 21 forks source link

Add support for Maximus XI Hero (WiFi). #25

Closed kyndigen closed 2 years ago

kyndigen commented 2 years ago

Also fix missing comma in initializer list.

zeule commented 2 years ago

Great! Thank you! Please verify the mutex name is correct (or upload here dump of the DSDT ACPI table and I will do that). You can obtain the dump from /sys/firmware/acpi/tables/DSDT file, and to decompile it you need the iasl tool from the acpica package.

Also please split the commit in two: a fix for the missing comma and the new board data. Also please add trailing comma for ROG MAXIMUS XI HERO (WI-FI) to avoid repeating the mistake. If you want your commits to be submitted to the mainline kernel, please sing them off (git commit -s).

zeule commented 2 years ago

And add your board to the list in README.md, please.

kyndigen commented 2 years ago

The mutex name is correct. I'll clean up the pull request when I'm back in town on Monday.

zeule commented 2 years ago

The mutex name is correct.

Could you, please, share the DSDT dump nevertheless? I want to add it to my collection and will probably need it for future refactoring.

I'll clean up the pull request when I'm back in town on Monday.

Thanks!

kyndigen commented 2 years ago

Changes have been split as requested. DSDT dump is attached to the open issue on your project.

zeule commented 2 years ago

Thank you! Let me ask the last question for this submission: which sensors can you confirm to be working?

kyndigen commented 2 years ago

Broadly speaking, the temperature and fan speed sensors seem fine and match the Monitoring screen in BIOS. The voltage and current sensors don't appear to work. That being said, I don't have sensors or fans on every single header, so here's a detailed breakdown:

asusec-isa-0000 Adapter: ISA adapter CPU Core: 0.00 V # Doesn't make sense. CPU_Opt: 859 RPM # Matches BIOS "CPU Opt" fan Chipset: 0 RPM # Not sure what this refers to. There are 11 different fan/pump readings in BIOS, none of them labeled "chipset" This could be reading a non-connected fan. Water_Flow: 0 RPM # Matches BIOS AIO_Pump (unused in my chassis) Chipset: +47.0°C # Matches BIOS PCH temperature CPU: +33.0°C # Matches BIOS CPU temperature Motherboard: +32.0°C # Matches BIOS Motherboard temperature T_Sensor: -40.0°C # Matches BIOS T_Sensor (assuming -40c here is N/A there) VRM: +35.0°C # Don't see an explicit VRM called out in BIOS, but this makes sense given the chipset & motherboard temp. Water_In: +29.0°C # Matches BIOS Water In temp Water_Out: -40.0°C # Matches BIOS Water Out (N/A) CPU: 0.00 A # Doesn't make sense.

As a side note, the reason I'm down this rabbit hole adding support for my board is that this is the only way for me to get access to the Water_In temp.

On Wed, Jun 15, 2022 at 1:23 AM Eugene Shalygin @.***> wrote:

Thank you! Let me ask the last question for this submission: which sensors can you confirm to be working?

— Reply to this email directly, view it on GitHub https://github.com/zeule/asus-ec-sensors/pull/25#issuecomment-1156154064, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADF2HBTDXX3T4KEJQ7TDUDDVPGHJHANCNFSM5YPI2MUQ . You are receiving this because you authored the thread.Message ID: @.***>

zeule commented 2 years ago

Then I submit the first commit (the fix) upstream right away and you will need to change the second commit providing specific board definition for this model.

This board probably has no fan for the chipset. I remember when AMD said there will be chipset fans for X570 the general public was quite surprised, as nobody was using so power-hungry chipsets at that time. This sensor needs to be removed.

The voltage and current sensors don't appear to work.

Remove them as well, please.

That being said, I don't have sensors or fans on every single header, so here's a detailed breakdown:

The blank values seems to be OK, let's keep the rest of them.

As a side note, the reason I'm down this rabbit hole adding support for my board is that this is the only way for me to get access to the Water_In temp.

Very well understand you, the same reason motivated me to start this journey and it kept me motivated for almost a whole year, which took me to learn how to read all these information from ASUS EC controllers.

zeule commented 2 years ago

I propose you to copy over the sensors_family_amd_500 array into sensors_family_intel_300 (or sensors_family_intel_coffee_lake), remove CPU current, voltage, and chipset fan sensors from the new array, reference that array in the board definition. You will also need to add the new board family to the switch in asus_ec_probe() at the bottom of the file. And I will refactor those family definitions merging duplicates when we better understand how ASUS themselves handle board families.

zeule commented 2 years ago

I cherry-picked the first commit to the master branch already.

zeule commented 2 years ago

BTW, the board specs page says in provides "1 x W_FLOW header", thus the Water_Flow sensor should match that and not the AIO pump header.

zeule commented 2 years ago

Ping? Maybe it would be more convenient for you if I did the PR and you tested it?

kyndigen commented 2 years ago

Sorry for the delay. I'm comfortable doing the patch, it's the testing that I need to find the time for. It's crunch time at work, so most of my evenings are occupied. I need to find a chunk of time where I can open up my machine and connect some spare fans and temp sensors to the unused headers to verify that even the ones I don't normally use are mapped correctly. I'm hopeful I can test it either this weekend or next.

Thanks

On Sat, Jun 25, 2022 at 5:22 AM Eugene Shalygin @.***> wrote:

Ping? Maybe it would be more convenient for you if I did the PR and you tested it?

— Reply to this email directly, view it on GitHub https://github.com/zeule/asus-ec-sensors/pull/25#issuecomment-1166272277, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADF2HBRUZYHBD3H5CXRJRA3VQ32W3ANCNFSM5YPI2MUQ . You are receiving this because you authored the thread.Message ID: @.***>

kyndigen commented 2 years ago

The latest candidate patch has its own board definition, with no CPU voltage, CPU current, or chipset fan sensors. I've verified all the remaining fan and temperature sensors are mapped correctly.

zeule commented 2 years ago

Thank you!

zeule commented 2 years ago

It would be nice if you share DSDT code for the board (# cat /sys/firmware/acpi/tables/DSDT), which might come of use for future refactoring.

kyndigen commented 2 years ago

I attached it to the original issue I opened (#24) 13 days ago: [https://github.com/zeule/asus-ec-sensors/issues/24], unless you want a binary copy not processed with iasl.

zeule commented 2 years ago

Oh, my bad... I'm happy with the decompiled code, of course.