diff mbox series

[3/3] hwmon: (nct6775) add MAXIMUS VII HERO.

Message ID 20211122212850.321542-4-pauk.denis@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series hwmon: (nct6775) Support lock by ACPI mutex | expand

Commit Message

Denis Pauk Nov. 22, 2021, 9:28 p.m. UTC 
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.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
Signed-off-by: Denis Pauk <pauk.denis@gmail.com>
Tested-by: Olli Asikainen <olli.asikainen@gmail.com>
---
 drivers/hwmon/nct6775.c | 121 +++++++++++++++++++++++++++-------------
 1 file changed, 81 insertions(+), 40 deletions(-)

Comments

Andy Shevchenko Nov. 24, 2021, 4:21 p.m. UTC | #1 
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?
               }
               ...
        }

?
Andy Shevchenko Nov. 24, 2021, 4:24 p.m. UTC | #2 
On Wed, Nov 24, 2021 at 6:21 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Nov 22, 2021 at 11:29 PM Denis Pauk <pauk.denis@gmail.com> wrote:

...

> > +               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;

Oh, just noticed above... Looks not good due to possible confusion
which means this part needs to be thought through and refactored,
perhaps by intermediate variable that defines the access and then you
assign access= to it if it satisfies the conditions.

>                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?
>                }
>                ...
>         }
>
> ?
diff mbox series

Patch

diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 3aeb32093c35..c7a476bc4693 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -5100,34 +5100,60 @@  static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
  */
 static struct platform_device *pdev[2];
 
-static const char * const asus_wmi_boards[] = {
-	"ProArt X570-CREATOR WIFI",
-	"Pro WS X570-ACE",
-	"PRIME B360-PLUS",
-	"PRIME B460-PLUS",
-	"PRIME X570-PRO",
-	"ROG CROSSHAIR VIII DARK HERO",
-	"ROG CROSSHAIR VIII FORMULA",
-	"ROG CROSSHAIR VIII HERO",
-	"ROG CROSSHAIR VIII IMPACT",
-	"ROG STRIX B550-E GAMING",
-	"ROG STRIX B550-F GAMING",
-	"ROG STRIX B550-F GAMING (WI-FI)",
-	"ROG STRIX B550-I GAMING",
-	"ROG STRIX X570-F GAMING",
-	"ROG STRIX Z390-E GAMING",
-	"ROG STRIX Z490-I GAMING",
-	"TUF GAMING B550M-PLUS",
-	"TUF GAMING B550M-PLUS (WI-FI)",
-	"TUF GAMING B550-PLUS",
-	"TUF GAMING B550-PRO",
-	"TUF GAMING X570-PLUS",
-	"TUF GAMING X570-PLUS (WI-FI)",
-	"TUF GAMING X570-PRO (WI-FI)",
-	"TUF GAMING Z490-PLUS",
-	"TUF GAMING Z490-PLUS (WI-FI)",
+struct acpi_board_info {
+	char *acpi_mutex_name;
 };
 
+#define DMI_ASUS_BOARD_INFO(name, mutex_name)			\
+static struct acpi_board_info name = {				\
+	.acpi_mutex_name = mutex_name,				\
+}
+
+DMI_ASUS_BOARD_INFO(acpi_board_ANY, NULL);
+DMI_ASUS_BOARD_INFO(acpi_board_MAXIMUS_VII_HERO, "\\_SB_.PCI0.LPCB.SIO1.MUT0");
+DMI_ASUS_BOARD_INFO(acpi_board_ROG_STRIX_B550_E_GAMING, "\\_SB.PCI0.SBRG.SIO1.MUT0");
+
+#define DMI_EXACT_MATCH_ASUS_BOARD_NAME(name, info) {			\
+	.matches = {								\
+		DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "ASUSTeK COMPUTER INC."),	\
+		DMI_EXACT_MATCH(DMI_BOARD_NAME, name),				\
+	},									\
+	.driver_data = info,							\
+}
+
+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),
+	{}
+};
+MODULE_DEVICE_TABLE(dmi, asus_wmi_info_table);
+
 static int __init sensors_nct6775_init(void)
 {
 	int i, err;
@@ -5137,30 +5163,45 @@  static int __init sensors_nct6775_init(void)
 	struct nct6775_sio_data sio_data;
 	int sioaddr[2] = { 0x2e, 0x4e };
 	enum sensor_access access = access_direct;
-	const char *board_vendor, *board_name;
+	const struct dmi_system_id *dmi_id;
+	struct acpi_board_info *board_info;
 	acpi_handle acpi_wmi_mutex = NULL;
-	u8 tmp;
+	acpi_status status;
+	u8 tmp = 0;
 
 	err = platform_driver_register(&nct6775_driver);
 	if (err)
 		return err;
 
-	board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
-	board_name = dmi_get_system_info(DMI_BOARD_NAME);
+	dmi_id = dmi_first_match(asus_wmi_info_table);
+	if (dmi_id && dmi_id->driver_data) {
+		board_info = dmi_id->driver_data;
+		access = access_asuswmi;
 
-	if (board_name && board_vendor &&
-	    !strcmp(board_vendor, "ASUSTeK COMPUTER INC.")) {
-		err = match_string(asus_wmi_boards, ARRAY_SIZE(asus_wmi_boards),
-				   board_name);
-		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);
+			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 */
+		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;
+		}
 	}
 
 	/*