Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3edab51a-fc1a-3657-88b5-02ab86aeaffb@redhat.com>
Date:   Wed, 12 Jul 2023 17:29:04 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH] platform/x86: wmi: add Positivo WMI key driver
Content-Language: en-US, nl
To:     Edson Juliano Drosdeck <edson.drosdeck@gmail.com>
Cc:     markgross@kernel.org, linux-kernel@vger.kernel.org,
        platform-driver-x86@vger.kernel.org
References: <20230628214015.68558-1-edson.drosdeck@gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230628214015.68558-1-edson.drosdeck@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Edson,

On 6/28/23 23:40, Edson Juliano Drosdeck wrote:
> Some function keys on the built in keyboard on Positivo's notebooks does
> not produce any key events when pressed in combination with the function
> key. Some of these keys do report that they are being pressed via WMI
> events.
> 
> This driver reports key events for Fn+F10,Fn+F11  and a custom key to
> launch a custom program.
> 
> Other WMI events that are reported by the hardware but not utilized by
> this driver are Caps Lock(which already work).
> 
> Signed-off-by: Edson Juliano Drosdeck <edson.drosdeck@gmail.com>

Thank you for your patch.

As Armin has mentioned, please change the driver to use the
WMI bus model and make it a wmi_driver.

A simple example (except for it using multiple WMI GUIDs) of such
a driver would be drivers/platform/x86/xiaomi-wmi.c

Please model your driver after this. This means you will be able
to drop the wmi_has_guid(POSITIVO_WMI_EVENT_GUID) from your
module_init() function (which will now become a probe function).

You do still need to keep the DMI check, since as Armin pointed
out the GUID unfortunately is not unique, but instead used the
Microsoft WMI docs example GUID.

Armin, we have seen this (using Microsoft WMI example GUID)
before and there is nothing we can do about this except add
a DMI check. This works for Windows because the WMI driver
for this for Windows is part of the factory image and presumably
has never been added to Windows Updates avoiding the GUID conflict.

Regards,

Hans


> ---
>  drivers/platform/x86/Kconfig        |  11 +++
>  drivers/platform/x86/Makefile       |   1 +
>  drivers/platform/x86/positivo-wmi.c | 136 ++++++++++++++++++++++++++++
>  3 files changed, 148 insertions(+)
>  create mode 100644 drivers/platform/x86/positivo-wmi.c
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 22052031c719..f3ad84479460 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -134,6 +134,17 @@ config YOGABOOK_WMI
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called lenovo-yogabook-wmi.
>  
> +config POSITIVO_WMI
> +	tristate "Positivo WMI key driver"
> +	depends on ACPI_WMI
> +	depends on INPUT
> +	select INPUT_SPARSEKMAP
> +	help
> +	  This driver provides support for Positvo WMI hotkeys.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called positivo-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 2cafe51ec4d8..5458bb9a56d3 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_NVIDIA_WMI_EC_BACKLIGHT)	+= nvidia-wmi-ec-backlight.o
>  obj-$(CONFIG_XIAOMI_WMI)		+= xiaomi-wmi.o
>  obj-$(CONFIG_GIGABYTE_WMI)		+= gigabyte-wmi.o
>  obj-$(CONFIG_YOGABOOK_WMI)		+= lenovo-yogabook-wmi.o
> +obj-$(CONFIG_POSITIVO_WMI)		+= positivo-wmi.o
>  
>  # Acer
>  obj-$(CONFIG_ACERHDF)		+= acerhdf.o
> diff --git a/drivers/platform/x86/positivo-wmi.c b/drivers/platform/x86/positivo-wmi.c
> new file mode 100644
> index 000000000000..5fbb4cf42154
> --- /dev/null
> +++ b/drivers/platform/x86/positivo-wmi.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/* WMI driver for Positivo Laptops
> + *
> + * Copyright (C) 2023 Edson Juliano Drosdeck <edson.drosdeck@gmail.com>
> + * 
> + * */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/acpi.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/dmi.h>
> +#include <linux/module.h>
> +
> +MODULE_AUTHOR("Edson Juliano Drosdeck");
> +MODULE_DESCRIPTION("Positivo WMI Hotkey Driver");
> +MODULE_LICENSE("GPL");
> +
> +#define POSITIVO_WMI_EVENT_GUID "ABBC0F72-8EA1-11D1-00A0-C90629100000"
> +
> +MODULE_ALIAS("wmi:"POSITIVO_WMI_EVENT_GUID);
> +
> +static const struct key_entry positivo_wmi_keymap[] = {
> +	{ KE_KEY, 0x1c, { KEY_PROG1} },
> +	{ KE_KEY, 0x36, { KEY_WLAN } },
> +	{ KE_KEY, 0x37, { KEY_BLUETOOTH } },
> +	{ KE_END, 0},
> +};
> +
> +static struct input_dev *positivo_wmi_input_dev;
> +
> +static void positivo_wmi_notify(u32 value, void *context)
> +{
> +	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	int eventcode;
> +	acpi_status status;
> +
> +	status = wmi_get_event_data(value, &response);
> +	if (status != AE_OK) {
> +		pr_err("bad event status 0x%x\n", status);
> +		return;
> +	}
> +
> +	obj = (union acpi_object *)response.pointer;
> +	if (obj && obj->type == ACPI_TYPE_INTEGER) {
> +		eventcode = obj->integer.value;
> +
> +		if (!sparse_keymap_report_event(positivo_wmi_input_dev,
> +						eventcode, 1, true))
> +			pr_err("Unknown key %x pressed\n", eventcode);
> +	}
> +
> +	kfree(response.pointer);
> +}
> +
> +static int positivo_wmi_input_setup(void)
> +{
> +
> +	int err;
> +
> +	positivo_wmi_input_dev = input_allocate_device();
> +	if (!positivo_wmi_input_dev)
> +		return -ENOMEM;
> +
> +	positivo_wmi_input_dev->name = "Positivo laptop WMI hotkeys";
> +	positivo_wmi_input_dev->phys = "wmi/input0";
> +	positivo_wmi_input_dev->id.bustype = BUS_HOST;
> +
> +	err = sparse_keymap_setup(positivo_wmi_input_dev,
> +				  positivo_wmi_keymap, NULL);
> +	if (err)
> +		goto err_free_dev;
> +	
> +	err = input_register_device(positivo_wmi_input_dev);
> +	
> +	if (err){
> +		pr_info("Unable to register input device\n");
> +		goto err_free_dev;
> +	}
> +
> +	return 0;
> +
> +err_free_dev:
> +	input_free_device(positivo_wmi_input_dev);
> +	return err;
> +}
> +
> +static const struct dmi_system_id positivo_wmi_dmi_table[] __initconst = {
> +	{
> +		.ident = "Positivo laptop",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Positivo Tecnologia SA"),
> +		},
> +	},
> +	{}
> +};
> +
> +static int __init positivo_wmi_init(void)
> +{
> +	int err;
> +
> +	if (!wmi_has_guid(POSITIVO_WMI_EVENT_GUID) ||
> +	    !dmi_check_system(positivo_wmi_dmi_table))
> +		return -ENODEV;
> +
> +	err = positivo_wmi_input_setup();
> +	if (err)
> +		return err;
> +
> +	err = wmi_install_notify_handler(POSITIVO_WMI_EVENT_GUID,
> +					positivo_wmi_notify, NULL);
> +		if (ACPI_FAILURE(err)) {
> +			pr_err("Unable to setup WMI notify handler\n");
> +			goto err_free_input;
> +		}
> +
> +	return 0;
> +
> +err_free_input:
> +	input_unregister_device(positivo_wmi_input_dev);
> +	return err;
> +
> +}
> +
> +static void __exit positivo_wmi_exit(void)
> +{
> +       wmi_remove_notify_handler(POSITIVO_WMI_EVENT_GUID);
> +       input_free_device(positivo_wmi_input_dev);
> +}
> +
> +module_init(positivo_wmi_init);
> +module_exit(positivo_wmi_exit);