lkml.org 
[lkml]   [2021]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 3/3] hwmon: (nct6775) add MAXIMUS VII HERO.
On Mon, Nov 22, 2021 at 11:29 PM Denis Pauk <pauk.denis@gmail.com> wrote:
>
> ASUS MAXIMUS VII HERO board has got an nct6775 chip, but by default
> there's no use of it because of resource conflict with WMI method.
>
> This commit adds MAXIMUS VII HERO to the list of boards and provides
> ACPI mutex name that can be used as shared lock with ASUS WMI.
>
> Logic checks that mutex is available. If mutex is not available
> tries to get chip version by ACPI WMI interface.

a chip

...

> +struct acpi_board_info {
> + char *acpi_mutex_name;

Looking below the name of the "name" should be rather "path".

> };

...

> +static const struct dmi_system_id asus_wmi_info_table[] = {
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("MAXIMUS VII HERO", &acpi_board_MAXIMUS_VII_HERO),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B360-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B460-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME B550M-A (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("PRIME X570-PRO", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("Pro WS X570-ACE", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ProArt X570-CREATOR WIFI", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII DARK HERO", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII FORMULA", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII HERO", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG CROSSHAIR VIII IMPACT", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-E GAMING",
> + &acpi_board_ROG_STRIX_B550_E_GAMING),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-F GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-F GAMING (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX B550-I GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX X570-F GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX Z390-E GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("ROG STRIX Z490-I GAMING", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550-PRO", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550M-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING B550M-PLUS (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PLUS (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING X570-PRO (WI-FI)", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING Z490-PLUS", &acpi_board_ANY),
> + DMI_EXACT_MATCH_ASUS_BOARD_NAME("TUF GAMING Z490-PLUS (WI-FI)", &acpi_board_ANY),

So, is it possible to eliminate acpi_board_ANY and use some default in
the code instead?

> + {}
> +};

....

> - if (board_name && board_vendor &&
> - !strcmp(board_vendor, "ASUSTeK COMPUTER INC.")) {
> - err = match_string(asus_wmi_boards, ARRAY_SIZE(asus_wmi_boards),
> - board_name);

Do you need string_helpers.h after this change?

> - if (err >= 0) {
> - /* if reading chip id via WMI succeeds, use WMI */
> - if (!nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp)) {
> - pr_info("Using Asus WMI to access %#x chip.\n", tmp);
> - access = access_asuswmi;
> + if (board_info->acpi_mutex_name) {

> + status = acpi_get_handle(NULL, board_info->acpi_mutex_name,
> + &acpi_wmi_mutex);

One line?

> + if (ACPI_FAILURE(status)) {
> + pr_err("Could not get hardware access guard mutex.\n");
> } else {
> - pr_err("Can't read ChipID by Asus WMI.\n");
> + pr_info("Using Asus WMI mutex: %s\n", board_info->acpi_mutex_name);
> + access = access_direct;
> }
> }

...

> + /* if reading chip id via WMI succeeds, use WMI */

Be consistent with how you spell "ChipID" / "chip id" / etc everywhere
in the code.

...

> + if (access == access_asuswmi &&
> + nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp)) {
> + access = access_direct;
> + pr_err("Can't read ChipID by Asus WMI.\n");
> + }
> +
> + if (access == access_asuswmi) {
> + if (tmp)
> + pr_info("Using Asus WMI to access %#x chip.\n", tmp);
> + else
> + access = access_direct;

Why not:

if (access == access_asuswmi) {
access = access_direct;
if (nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp))
pr_err("Can't read ChipID by Asus WMI.\n");
if (tmp) {
pr_info("Using Asus WMI to access %#x chip.\n", tmp);
access = access_...; // do you have this?
}
...
}

?

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2021-11-24 17:25    [W:0.201 / U:0.592 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site