From: "Barnabás Pőcze" <pobrn@protonmail.com>
To: "Thomas Weißschuh" <linux@weissschuh.net>
Cc: "platform-driver-x86@vger.kernel.org"
<platform-driver-x86@vger.kernel.org>,
Mark Gross <mgross@linux.intel.com>,
Hans de Goede <hdegoede@redhat.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Matthew Garrett <mjg59@srcf.ucam.org>
Subject: Re: [PATCH] platform/x86: add Gigabyte WMI temperature driver
Date: Mon, 05 Apr 2021 17:13:48 +0000 [thread overview]
Message-ID: <N6sOrC__lJeA1mtEKUtB18DPy9hp5bSjL9rq1TfOXiRE7IAO5aih5oyPEpq-vyqdZZsF4W8FIe-9GWB15lO-3fQlqjWQrMTlTJvqLBBGYOQ=@protonmail.com> (raw)
In-Reply-To: <20210405132007.290275-1-linux@weissschuh.net>
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
next prev parent reply other threads:[~2021-04-05 17:13 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-05 13:20 [PATCH] platform/x86: add Gigabyte WMI temperature driver Thomas Weißschuh
2021-04-05 17:13 ` Barnabás Pőcze [this message]
2021-04-05 20:48 ` [PATCH v2] " Thomas Weißschuh
2021-04-07 15:54 ` Hans de Goede
2021-04-07 19:43 ` Thomas Weißschuh
2021-04-08 9:36 ` Hans de Goede
2021-04-08 14:54 ` Thomas Weißschuh
2021-04-08 15:00 ` Guenter Roeck
2021-04-09 6:02 ` Thomas Weißschuh
2021-04-10 6:56 ` Guenter Roeck
2021-04-10 14:21 ` Hans de Goede
2021-04-10 14:40 ` [PATCH v3] " Thomas Weißschuh
2021-04-10 14:57 ` Hans de Goede
2021-04-10 15:21 ` Guenter Roeck
2021-04-10 15:15 ` Guenter Roeck
2021-04-10 15:23 ` Hans de Goede
2021-04-10 18:18 ` [PATCH v4] " Thomas Weißschuh
2021-04-11 0:58 ` Guenter Roeck
2021-04-11 14:05 ` Barnabás Pőcze
2021-04-12 12:35 ` [PATCH v5] " Thomas Weißschuh
2021-04-13 8:14 ` Hans de Goede
2021-04-07 18:27 ` [PATCH v2] " Barnabás Pőcze
2021-04-08 14:39 ` Thomas Weißschuh
2021-04-08 15:08 ` Guenter Roeck
2021-04-08 16:07 ` Hans de Goede
2021-04-10 6:40 ` Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='N6sOrC__lJeA1mtEKUtB18DPy9hp5bSjL9rq1TfOXiRE7IAO5aih5oyPEpq-vyqdZZsF4W8FIe-9GWB15lO-3fQlqjWQrMTlTJvqLBBGYOQ=@protonmail.com' \
--to=pobrn@protonmail.com \
--cc=hdegoede@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@weissschuh.net \
--cc=mgross@linux.intel.com \
--cc=mjg59@srcf.ucam.org \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).