MIME-Version: 1.0
References: <20211122212850.321542-1-pauk.denis@gmail.com> <20211122212850.321542-4-pauk.denis@gmail.com>
In-Reply-To: <20211122212850.321542-4-pauk.denis@gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 24 Nov 2021 18:21:30 +0200
Message-ID: <CAHp75VeKosontsmFJSp-fbV9mPiSWJeLUCpx90=RHy1HFfBDnA@mail.gmail.com>
Subject: Re: [PATCH 3/3] hwmon: (nct6775) add MAXIMUS VII HERO.
To:     Denis Pauk <pauk.denis@gmail.com>
Cc:     Eugene Shalygin <eugene.shalygin@gmail.com>,
        Platform Driver <platform-driver-x86@vger.kernel.org>,
        thomas@weissschuh.net, Olli Asikainen <olli.asikainen@gmail.com>,
        Guenter Roeck <linux@roeck-us.net>,
        Jean Delvare <jdelvare@suse.com>, linux-hwmon@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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