diff mbox series

platform/x86: add Gigabyte WMI temperature driver

Message ID 20210405132007.290275-1-linux@weissschuh.net (mailing list archive)
State Superseded, archived
Headers show
Series platform/x86: add Gigabyte WMI temperature driver | expand

Commit Message

Thomas Weißschuh April 5, 2021, 1:20 p.m. UTC 
Hi,

this patch adds support for temperature readings on Gigabyte Mainboards.
(At least on mine)

The current code should be usable as is.
I'd like for people to test it though and report their results with different
hardware.

Further development I have some general questions:

The ASL IndexField does not cover all relevant registers, can it be extended by
driver code?

* Not all registers are exposed via ACPI methods, can they be accessed via ACPI directly?
* Some registers are exposed via ACPI methods but are not reachable directly from the WMI dispatcher.
  * Does ASL have some sort of reflection that could enable those methods?
  * Is it possible to call those methods directly, bypassing WMI?

I suspect the answer to be "no" for all of these, but maybe I am wrong.

Furthermore there are WMI methods to return information about the installed
firmware.

* Version
* Build-date
* Motherboard name

Would it make sense to add this information as attributes to the
platform_device?

The ACPI tables can be found here:
https://github.com/t-8ch/linux-gigabyte-wmi-driver/blob/main/ssdt8.dsl

Thanks,
Thomas

-- >8 --

Tested with a X570 I Aorus Pro Wifi.
The mainboard contains a ITE IT8688E chip for management.
This chips is also handled by drivers/hwmon/i87.c but as it is also used
by the firmware itself it needs an ACPI driver.

Unfortunately not all sensor registers are handled by the firmware and even
less are exposed via WMI.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/x86/Kconfig        |  10 ++
 drivers/platform/x86/Makefile       |   1 +
 drivers/platform/x86/gigabyte-wmi.c | 178 ++++++++++++++++++++++++++++
 3 files changed, 189 insertions(+)
 create mode 100644 drivers/platform/x86/gigabyte-wmi.c


base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32

Comments

Barnabás Pőcze April 5, 2021, 5:13 p.m. UTC | #1 
Hi


If you wanted to get some feedback regarding the code, then I added some comments;
otherwise please disregard this email.

I think the two most important things are:

 * consider using `devm_hwmon_device_register_with_info()` instead of manually
   specifying the attributes;
 * consider getting rid of the platform {driver,device} and use a single
   WMI driver.


2021. április 5., hétfő 15:20 keltezéssel, Thomas Weißschuh írta:

> Hi,
>
> this patch adds support for temperature readings on Gigabyte Mainboards.
> (At least on mine)
>
> The current code should be usable as is.
> I'd like for people to test it though and report their results with different
> hardware.
>
> Further development I have some general questions:
>
> The ASL IndexField does not cover all relevant registers, can it be extended by
> driver code?
>
> * Not all registers are exposed via ACPI methods, can they be accessed via ACPI directly?
> * Some registers are exposed via ACPI methods but are not reachable directly from the WMI dispatcher.
>   * Does ASL have some sort of reflection that could enable those methods?
>   * Is it possible to call those methods directly, bypassing WMI?
>
> I suspect the answer to be "no" for all of these, but maybe I am wrong.
>
> Furthermore there are WMI methods to return information about the installed
> firmware.
>
> * Version
> * Build-date
> * Motherboard name
>
> Would it make sense to add this information as attributes to the
> platform_device?

I think if there aren't really users of the data, then just printing them
to the kernel message buffer is fine until there's a need
(in the probe function, for example). Does it provide information
that DMI doesn't?


>
> The ACPI tables can be found here:
> https://github.com/t-8ch/linux-gigabyte-wmi-driver/blob/main/ssdt8.dsl
>
> Thanks,
> Thomas
>
> -- >8 --
>
> Tested with a X570 I Aorus Pro Wifi.
> The mainboard contains a ITE IT8688E chip for management.
> This chips is also handled by drivers/hwmon/i87.c but as it is also used
> by the firmware itself it needs an ACPI driver.
>
> Unfortunately not all sensor registers are handled by the firmware and even
> less are exposed via WMI.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/platform/x86/Kconfig        |  10 ++
>  drivers/platform/x86/Makefile       |   1 +
>  drivers/platform/x86/gigabyte-wmi.c | 178 ++++++++++++++++++++++++++++
>  3 files changed, 189 insertions(+)
>  create mode 100644 drivers/platform/x86/gigabyte-wmi.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index ad4e630e73e2..40d593ff1f01 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -123,6 +123,16 @@ config XIAOMI_WMI
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called xiaomi-wmi.
>
> +config GIGABYTE_WMI
> +	tristate "Gigabyte WMI temperature driver"
> +	depends on ACPI_WMI
> +	depends on HWMON
> +	help
> +	  Say Y here if you want to support WMI-based temperature on Gigabyte mainboards.

I'm not a native speaker, but maybe "WMI-based temperature reporting" would be better?


> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called gigabyte-wmi.
> +
>  config ACERHDF
>  	tristate "Acer Aspire One temperature and fan driver"
>  	depends on ACPI && THERMAL
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 60d554073749..1621ebfd04fd 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT)	+= intel-wmi-thunderbolt.o
>  obj-$(CONFIG_MXM_WMI)			+= mxm-wmi.o
>  obj-$(CONFIG_PEAQ_WMI)			+= peaq-wmi.o
>  obj-$(CONFIG_XIAOMI_WMI)		+= xiaomi-wmi.o
> +obj-$(CONFIG_GIGABYTE_WMI)		+= gigabyte-wmi.o
>
>  # Acer
>  obj-$(CONFIG_ACERHDF)		+= acerhdf.o
> diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
> new file mode 100644
> index 000000000000..a3749cf248cb
> --- /dev/null
> +++ b/drivers/platform/x86/gigabyte-wmi.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Gigabyte WMI temperature driver
> + *
> + * Copyright (C) 2021 Thomas Weißschuh <linux@weissschuh.net>
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/acpi.h>
> +#include <linux/platform_device.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>

It is usually preferred if the list is sorted alphabetically.


> +
> +#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
> +#define DRVNAME "gigabyte-wmi"
> +
> +MODULE_AUTHOR("Thomas Weißschuh <thomas@weissschuh.net>");
> +MODULE_DESCRIPTION("Gigabyte Generic WMI Driver");

The Kconfig says "Gigabyte WMI **temperature** driver". Which one is it?


> +MODULE_LICENSE("GPL");
> +
> +MODULE_ALIAS("wmi:" GIGABYTE_WMI_GUID);
> +
> +enum gigabyte_wmi_commandtype {
> +	GIGABYTE_WMI_BUILD_DATE_QUERY       =   0x1,
> +	GIGABYTE_WMI_MAINBOARD_TYPE_QUERY   =   0x2,
> +	GIGABYTE_WMI_FIRMWARE_VERSION_QUERY =   0x4,
> +	GIGABYTE_WMI_MAINBOARD_NAME_QUERY   =   0x5,
> +	GIGABYTE_WMI_TEMPERATURE_QUERY      = 0x125,
> +};
> +
> +static int gigabyte_wmi_temperature(u8 sensor, s8 *res);

If you change the order of functions, you can get rid of this forward declaration.


> +
> +static ssize_t temp_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> +	int index = sattr->index;
> +	s8 temp;
> +	acpi_status res;
> +
> +	res = gigabyte_wmi_temperature(index, &temp);
> +	if (ACPI_FAILURE(res))
> +		return -res;

ACPI errors and errnos are not compatible, you can't return it like that. Or,
technically, you can, but it does not make sense. E.g. it'd "convert"
AE_NO_MEMORY to EINTR since both have the numeric value 4.


> +
> +	return sprintf(buf, "%d\n", temp * 1000);

Use `sysfs_emit()`.


> +}
> +
> +static SENSOR_DEVICE_ATTR_2_RO(temp1_input, temp, 0, 0);
> +static SENSOR_DEVICE_ATTR_2_RO(temp2_input, temp, 0, 1);
> +static SENSOR_DEVICE_ATTR_2_RO(temp3_input, temp, 0, 2);
> +static SENSOR_DEVICE_ATTR_2_RO(temp4_input, temp, 0, 3);
> +static SENSOR_DEVICE_ATTR_2_RO(temp5_input, temp, 0, 4);
> +static SENSOR_DEVICE_ATTR_2_RO(temp6_input, temp, 0, 5);
> +
> +static struct platform_device *gigabyte_wmi_pdev;
> +
> +
> +static struct attribute *gigabyte_wmi_hwmon_attributes_temp[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_temp2_input.dev_attr.attr,
> +	&sensor_dev_attr_temp3_input.dev_attr.attr,
> +	&sensor_dev_attr_temp4_input.dev_attr.attr,
> +	&sensor_dev_attr_temp5_input.dev_attr.attr,
> +	&sensor_dev_attr_temp6_input.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group gigabyte_wmi_hwmon_group_temp = {
> +	.attrs = gigabyte_wmi_hwmon_attributes_temp,
> +};
> +
> +static const struct attribute_group *gigabyte_wmi_hwmon_groups[] = {
> +	&gigabyte_wmi_hwmon_group_temp,
> +	NULL,
> +};

If you want to, you can get rid of these two definitions if you rename
`gigabyte_wmi_hwmon_attributes_temp` to `gigabyte_wmi_hwmon_temp_attrs`
and use

  ATTRIBUTE_GROUPS(gigabyte_wmi_hwmon_temp); // linux/sysfs.h

that'll define `gigabyte_wmi_hwmon_temp_group` and `gigabyte_wmi_hwmon_temp_groups`.


> +
> +static int gigabyte_wmi_probe(struct platform_device *pdev)
> +{
> +	struct device *hwmon_dev;
> +	struct device *dev = &pdev->dev;
> +
> +	if (!wmi_has_guid(GIGABYTE_WMI_GUID))
> +		return -ENODEV;
> +	gigabyte_wmi_pdev = pdev;
> +
> +	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> +					"gigabyte_wmi",
> +					NULL, gigabyte_wmi_hwmon_groups);

Is there a reason for not using `devm_hwmon_device_register_with_info()` and
the hwmon_{chip,channel}_info, hwmon_ops stuff? That way you wouldn't need to
touch any of the sysfs things.


> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static struct platform_driver gigabyte_wmi_driver = {
> +	.driver = {
> +		.name	= DRVNAME,
> +	},
> +	.probe	= gigabyte_wmi_probe,
> +};

Is there any reason for using a platform driver instead of a `wmi_driver`?
I think you could get rid of both the `platform_driver` and the `platform_device`
and simply use a `wmi_driver`.


> +
> +struct args {
> +	u32 arg1;
> +	u32 arg2;
> +	u32 arg3;
> +};
> +
> +static acpi_status gigabyte_wmi_perform_query(
> +		enum gigabyte_wmi_commandtype command, struct args *args, struct acpi_buffer *out)

Long function signatures are usually split up in such a way that the first argument
remains in line with the function name.


> +{
> +	struct acpi_buffer in = {};
> +
> +	if (!args) {
> +		struct args empty_args = {};
> +
> +		args = &empty_args;

I don't think this is correct. `args` will be a dangling pointer since `empty_args`
goes out of scope - if I'm not mistaken.


> +	}
> +	in.length = sizeof(*args);
> +	in.pointer = args;
> +	return wmi_evaluate_method(GIGABYTE_WMI_GUID, 0x0, command, &in, out);
> +}
> +
> +static int gigabyte_wmi_query_integer(
> +		enum gigabyte_wmi_commandtype command, struct args *args, u64 *res)
> +{
> +	union acpi_object *obj;
> +	struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };

The allocated buffer is not freed.


> +	acpi_status ret;
> +
> +	ret = gigabyte_wmi_perform_query(command, args, &result);
> +	if (ACPI_FAILURE(ret))
> +		return -ENXIO;
> +	obj = result.pointer;
> +	if (!obj || obj->type != ACPI_TYPE_INTEGER) {
> +		pr_warn("Unexpected result type %d for command %d", obj->type, command);
> +		return -ENXIO;
> +	}
> +	*res = obj->integer.value;
> +	return AE_OK;
> +}
> +
> +static int gigabyte_wmi_temperature(u8 sensor, s8 *res)
> +{
> +	struct args args = {
> +		.arg1 = sensor,
> +	};
> +	u64 temp;
> +	acpi_status ret;
> +
> +	ret = gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
> +	if (ret == 0)
> +		*res = (s8) temp;

That cast will be done no matter if it's explicitly specified,
so you might want to drop it.


> +	return ret;
> +}
> +
> +static int __init gigabyte_wmi_init(void)
> +{
> +	struct platform_device *pdev;
> +	int err;
> +
> +	err = platform_driver_register(&gigabyte_wmi_driver);
> +	if (err)
> +		return err;
> +	pdev = platform_device_alloc(DRVNAME, -1);

Prefer `PLATFORM_DEVID_NONE` instead of -1.


> +	if (!pdev) {
> +		platform_driver_unregister(&gigabyte_wmi_driver);
> +		return -ENOMEM;
> +	}
> +	return platform_device_add(pdev);

The driver is not unregistered if this fails.


> +}
> +module_init(gigabyte_wmi_init);
> +
> +static void __exit gigabyte_wmi_exit(void)
> +{
> +	platform_device_unregister(gigabyte_wmi_pdev);
> +	platform_driver_unregister(&gigabyte_wmi_driver);
> +}
> +module_exit(gigabyte_wmi_exit);
>
> base-commit: 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
> --
> 2.31.1
>


Regards,
Barnabás Pőcze
diff mbox series

Patch

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index ad4e630e73e2..40d593ff1f01 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -123,6 +123,16 @@  config XIAOMI_WMI
 	  To compile this driver as a module, choose M here: the module will
 	  be called xiaomi-wmi.
 
+config GIGABYTE_WMI
+	tristate "Gigabyte WMI temperature driver"
+	depends on ACPI_WMI
+	depends on HWMON
+	help
+	  Say Y here if you want to support WMI-based temperature on Gigabyte mainboards.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gigabyte-wmi.
+
 config ACERHDF
 	tristate "Acer Aspire One temperature and fan driver"
 	depends on ACPI && THERMAL
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 60d554073749..1621ebfd04fd 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -15,6 +15,7 @@  obj-$(CONFIG_INTEL_WMI_THUNDERBOLT)	+= intel-wmi-thunderbolt.o
 obj-$(CONFIG_MXM_WMI)			+= mxm-wmi.o
 obj-$(CONFIG_PEAQ_WMI)			+= peaq-wmi.o
 obj-$(CONFIG_XIAOMI_WMI)		+= xiaomi-wmi.o
+obj-$(CONFIG_GIGABYTE_WMI)		+= gigabyte-wmi.o
 
 # Acer
 obj-$(CONFIG_ACERHDF)		+= acerhdf.o
diff --git a/drivers/platform/x86/gigabyte-wmi.c b/drivers/platform/x86/gigabyte-wmi.c
new file mode 100644
index 000000000000..a3749cf248cb
--- /dev/null
+++ b/drivers/platform/x86/gigabyte-wmi.c
@@ -0,0 +1,178 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Gigabyte WMI temperature driver
+ *
+ * Copyright (C) 2021 Thomas Weißschuh <linux@weissschuh.net>
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+#define GIGABYTE_WMI_GUID "DEADBEEF-2001-0000-00A0-C90629100000"
+#define DRVNAME "gigabyte-wmi"
+
+MODULE_AUTHOR("Thomas Weißschuh <thomas@weissschuh.net>");
+MODULE_DESCRIPTION("Gigabyte Generic WMI Driver");
+MODULE_LICENSE("GPL");
+
+MODULE_ALIAS("wmi:" GIGABYTE_WMI_GUID);
+
+enum gigabyte_wmi_commandtype {
+	GIGABYTE_WMI_BUILD_DATE_QUERY       =   0x1,
+	GIGABYTE_WMI_MAINBOARD_TYPE_QUERY   =   0x2,
+	GIGABYTE_WMI_FIRMWARE_VERSION_QUERY =   0x4,
+	GIGABYTE_WMI_MAINBOARD_NAME_QUERY   =   0x5,
+	GIGABYTE_WMI_TEMPERATURE_QUERY      = 0x125,
+};
+
+static int gigabyte_wmi_temperature(u8 sensor, s8 *res);
+
+static ssize_t temp_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	int index = sattr->index;
+	s8 temp;
+	acpi_status res;
+
+	res = gigabyte_wmi_temperature(index, &temp);
+	if (ACPI_FAILURE(res))
+		return -res;
+
+	return sprintf(buf, "%d\n", temp * 1000);
+}
+
+static SENSOR_DEVICE_ATTR_2_RO(temp1_input, temp, 0, 0);
+static SENSOR_DEVICE_ATTR_2_RO(temp2_input, temp, 0, 1);
+static SENSOR_DEVICE_ATTR_2_RO(temp3_input, temp, 0, 2);
+static SENSOR_DEVICE_ATTR_2_RO(temp4_input, temp, 0, 3);
+static SENSOR_DEVICE_ATTR_2_RO(temp5_input, temp, 0, 4);
+static SENSOR_DEVICE_ATTR_2_RO(temp6_input, temp, 0, 5);
+
+static struct platform_device *gigabyte_wmi_pdev;
+
+
+static struct attribute *gigabyte_wmi_hwmon_attributes_temp[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp3_input.dev_attr.attr,
+	&sensor_dev_attr_temp4_input.dev_attr.attr,
+	&sensor_dev_attr_temp5_input.dev_attr.attr,
+	&sensor_dev_attr_temp6_input.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group gigabyte_wmi_hwmon_group_temp = {
+	.attrs = gigabyte_wmi_hwmon_attributes_temp,
+};
+
+static const struct attribute_group *gigabyte_wmi_hwmon_groups[] = {
+	&gigabyte_wmi_hwmon_group_temp,
+	NULL,
+};
+
+static int gigabyte_wmi_probe(struct platform_device *pdev)
+{
+	struct device *hwmon_dev;
+	struct device *dev = &pdev->dev;
+
+	if (!wmi_has_guid(GIGABYTE_WMI_GUID))
+		return -ENODEV;
+	gigabyte_wmi_pdev = pdev;
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
+					"gigabyte_wmi",
+					NULL, gigabyte_wmi_hwmon_groups);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct platform_driver gigabyte_wmi_driver = {
+	.driver = {
+		.name	= DRVNAME,
+	},
+	.probe	= gigabyte_wmi_probe,
+};
+
+struct args {
+	u32 arg1;
+	u32 arg2;
+	u32 arg3;
+};
+
+static acpi_status gigabyte_wmi_perform_query(
+		enum gigabyte_wmi_commandtype command, struct args *args, struct acpi_buffer *out)
+{
+	struct acpi_buffer in = {};
+
+	if (!args) {
+		struct args empty_args = {};
+
+		args = &empty_args;
+	}
+	in.length = sizeof(*args);
+	in.pointer = args;
+	return wmi_evaluate_method(GIGABYTE_WMI_GUID, 0x0, command, &in, out);
+}
+
+static int gigabyte_wmi_query_integer(
+		enum gigabyte_wmi_commandtype command, struct args *args, u64 *res)
+{
+	union acpi_object *obj;
+	struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
+	acpi_status ret;
+
+	ret = gigabyte_wmi_perform_query(command, args, &result);
+	if (ACPI_FAILURE(ret))
+		return -ENXIO;
+	obj = result.pointer;
+	if (!obj || obj->type != ACPI_TYPE_INTEGER) {
+		pr_warn("Unexpected result type %d for command %d", obj->type, command);
+		return -ENXIO;
+	}
+	*res = obj->integer.value;
+	return AE_OK;
+}
+
+static int gigabyte_wmi_temperature(u8 sensor, s8 *res)
+{
+	struct args args = {
+		.arg1 = sensor,
+	};
+	u64 temp;
+	acpi_status ret;
+
+	ret = gigabyte_wmi_query_integer(GIGABYTE_WMI_TEMPERATURE_QUERY, &args, &temp);
+	if (ret == 0)
+		*res = (s8) temp;
+	return ret;
+}
+
+static int __init gigabyte_wmi_init(void)
+{
+	struct platform_device *pdev;
+	int err;
+
+	err = platform_driver_register(&gigabyte_wmi_driver);
+	if (err)
+		return err;
+	pdev = platform_device_alloc(DRVNAME, -1);
+	if (!pdev) {
+		platform_driver_unregister(&gigabyte_wmi_driver);
+		return -ENOMEM;
+	}
+	return platform_device_add(pdev);
+}
+module_init(gigabyte_wmi_init);
+
+static void __exit gigabyte_wmi_exit(void)
+{
+	platform_device_unregister(gigabyte_wmi_pdev);
+	platform_driver_unregister(&gigabyte_wmi_driver);
+}
+module_exit(gigabyte_wmi_exit);