Re: [PATCH v2 1/2] hwmon: (nct6775) Directly call ASUS ACPI WMI method

* Re: [PATCH v2 1/2] hwmon: (nct6775) Directly call ASUS ACPI WMI method
@ 2023-01-09 23:09 Guenter Roeck
  0 siblings, 0 replies; 3+ messages in thread
From: Guenter Roeck @ 2023-01-09 23:09 UTC (permalink / raw)
  To: Denis Pauk
  Cc: ahmad, chunkeey, greg, hubert.banas, igor, jaap.dehaan, jdelvare,
	jeroen, jonfarr87, jwp, kdudka, kernel, kpietrzak, linux-hwmon,
	linux-kernel, me, metalcaedes, michael, mikhail.v.gavrilov,
	mundanedefoliation, nephartyz, oleksandr, pehlm, renedis, robert,
	sahan.h.fernando, sebastian.arnhold, sefoci9222, sst, to.eivind,
	torvic9

On Mon, Jan 09, 2023 at 11:15:07PM +0200, Denis Pauk wrote:
> New ASUS B650/B660/X670 boards firmware have not exposed WMI monitoring
> GUID  and entrypoint method WMBD could be implemented for different device
> UID.
> 
> Implement the direct call to entrypoint method for monitoring the device
> UID of B550/X570 boards.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204807
> Signed-off-by: Denis Pauk <pauk.denis@gmail.com>
> Co-developed-by: Ahmad Khalifa <ahmad@khalifa.ws>
> Signed-off-by: Ahmad Khalifa <ahmad@khalifa.ws>
> ---
> Changes:
> v1:
>   rename each_port_arg to each_device_arg
>   rename nct6775_find_asus_acpi to nct6775_asuswmi_device_match
>   remove unrequired return -EEXIST, and iterate whole list of devices

Why ? More on that below.

>   make asus_acpi_dev static
> 
>  drivers/hwmon/Kconfig            |  2 +-
>  drivers/hwmon/nct6775-platform.c | 97 ++++++++++++++++++++++----------
>  2 files changed, 69 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 3176c33af6c6..300ce8115ce4 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1516,7 +1516,7 @@ config SENSORS_NCT6775_CORE
>  config SENSORS_NCT6775
>  	tristate "Platform driver for Nuvoton NCT6775F and compatibles"
>  	depends on !PPC
> -	depends on ACPI_WMI || ACPI_WMI=n
> +	depends on ACPI || ACPI=n
>  	select HWMON_VID
>  	select SENSORS_NCT6775_CORE
>  	help
> diff --git a/drivers/hwmon/nct6775-platform.c b/drivers/hwmon/nct6775-platform.c
> index bf43f73dc835..1f7885af524e 100644
> --- a/drivers/hwmon/nct6775-platform.c
> +++ b/drivers/hwmon/nct6775-platform.c
> @@ -17,7 +17,6 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> -#include <linux/wmi.h>
>  
>  #include "nct6775.h"
>  
> @@ -107,40 +106,44 @@ struct nct6775_sio_data {
>  	void (*sio_exit)(struct nct6775_sio_data *sio_data);
>  };
>  
> -#define ASUSWMI_MONITORING_GUID		"466747A0-70EC-11DE-8A39-0800200C9A66"
> +#define ASUSWMI_METHOD			"WMBD"
>  #define ASUSWMI_METHODID_RSIO		0x5253494F
>  #define ASUSWMI_METHODID_WSIO		0x5753494F
>  #define ASUSWMI_METHODID_RHWM		0x5248574D
>  #define ASUSWMI_METHODID_WHWM		0x5748574D
>  #define ASUSWMI_UNSUPPORTED_METHOD	0xFFFFFFFE
> +#define ASUSWMI_DEVICE_HID		"PNP0C14"
> +#define ASUSWMI_DEVICE_UID		"ASUSWMI"
> +
> +static struct acpi_device *asus_acpi_dev;
>  
>  static int nct6775_asuswmi_evaluate_method(u32 method_id, u8 bank, u8 reg, u8 val, u32 *retval)
>  {
> -#if IS_ENABLED(CONFIG_ACPI_WMI)
> +#if IS_ENABLED(CONFIG_ACPI)
> +	acpi_handle handle = acpi_device_handle(asus_acpi_dev);
>  	u32 args = bank | (reg << 8) | (val << 16);
> -	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
> -	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	struct acpi_object_list input;
> +	union acpi_object params[3];
> +	unsigned long long result;
>  	acpi_status status;
> -	union acpi_object *obj;
> -	u32 tmp = ASUSWMI_UNSUPPORTED_METHOD;
> -
> -	status = wmi_evaluate_method(ASUSWMI_MONITORING_GUID, 0,
> -				     method_id, &input, &output);
>  
> +	params[0].type = ACPI_TYPE_INTEGER;
> +	params[0].integer.value = 0;
> +	params[1].type = ACPI_TYPE_INTEGER;
> +	params[1].integer.value = method_id;
> +	params[2].type = ACPI_TYPE_BUFFER;
> +	params[2].buffer.length = sizeof(args);
> +	params[2].buffer.pointer = (void *)&args;
> +	input.count = 3;
> +	input.pointer = params;
> +
> +	status = acpi_evaluate_integer(handle, ASUSWMI_METHOD, &input, &result);
>  	if (ACPI_FAILURE(status))
>  		return -EIO;
>  
> -	obj = output.pointer;
> -	if (obj && obj->type == ACPI_TYPE_INTEGER)
> -		tmp = obj->integer.value;
> -
>  	if (retval)
> -		*retval = tmp;
> -
> -	kfree(obj);
> +		*retval = (u32)result & 0xFFFFFFFF;
>  
> -	if (tmp == ASUSWMI_UNSUPPORTED_METHOD)
> -		return -ENODEV;
>  	return 0;
>  #else
>  	return -EOPNOTSUPP;
> @@ -1099,6 +1102,50 @@ static const char * const asus_wmi_boards[] = {
>  	"TUF GAMING Z490-PLUS (WI-FI)",
>  };
>  
> +struct each_device_arg {
> +	struct acpi_device *adev;
> +	const char *match;
> +};
> +
> +/*
> + * Callback for acpi_bus_for_each_dev() to find the right device
> + * by _UID and _HID and store to each_device_arg.
> + */
> +static int nct6775_asuswmi_device_match(struct device *dev, void *data)
> +{
> +	struct acpi_device *adev = to_acpi_device(dev);
> +	const char *uid = acpi_device_uid(adev);
> +	const char *hid = acpi_device_hid(adev);
> +	struct each_device_arg *arg = data;
> +
> +	if (hid && !strcmp(hid, ASUSWMI_DEVICE_HID) &&
> +		uid && !strcmp(uid, arg->match)) {
> +		arg->adev = adev;

Why not return 1 for match here ? If there is a reason to look for
the last match instead of the first match, it needs to be explained.

> +	}
> +
> +	return 0;
> +}
> +
> +static enum sensor_access nct6775_determine_access(const char *device_uid)
> +{
> +	struct each_device_arg arg;
> +	u8 tmp;
> +
> +	arg.match = device_uid;
> +	acpi_bus_for_each_dev(nct6775_asuswmi_device_match, &arg);
> +	if (!arg.adev)
> +		return access_direct;
> +
> +	asus_acpi_dev = arg.adev;

The use of the static variable made me look into this further. Why
all that complexity with struct each_device_arg and passing the structure
and adev to/from the match function instead of just passing the 
match string as parameter and storing the matching adev directly in
asus_acpi_dev instead of passing it back and storing it here ?

Also, the use of a static variable makes me wonder: would asus_acpi_dev
be the same for both chips if there are two Super-IO chips in the system ?

Thanks,
Guenter

> +	/* if reading chip id via ACPI succeeds, use WMI "WMBD" method for access */
> +	if (!nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp) && tmp) {
> +		pr_debug("Using Asus WMBD method of %s to access %#x chip.\n", device_uid, tmp);
> +		return access_asuswmi;
> +	}
> +
> +	return access_direct;
> +}
> +
>  static int __init sensors_nct6775_platform_init(void)
>  {
>  	int i, err;
> @@ -1109,7 +1156,6 @@ static int __init sensors_nct6775_platform_init(void)
>  	int sioaddr[2] = { 0x2e, 0x4e };
>  	enum sensor_access access = access_direct;
>  	const char *board_vendor, *board_name;
> -	u8 tmp;
>  
>  	err = platform_driver_register(&nct6775_driver);
>  	if (err)
> @@ -1122,15 +1168,8 @@ static int __init sensors_nct6775_platform_init(void)
>  	    !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) && tmp) {
> -				pr_info("Using Asus WMI to access %#x chip.\n", tmp);
> -				access = access_asuswmi;
> -			} else {
> -				pr_err("Can't read ChipID by Asus WMI.\n");
> -			}
> -		}
> +		if (err >= 0)
> +			access = nct6775_determine_access(ASUSWMI_DEVICE_UID);
>  	}
>  
>  	/*
> 
> base-commit: b0587c87abc891e313d63946ff8c9f4939d1ea1a
> -- 
> 2.39.0
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread