Re: [PATCH v2] platform/x86: peaq-wmi: switch to using polled mode of input devices

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 01-10-2019 20:58, Dmitry Torokhov wrote:
> We have added polled mode to the normal input devices with the intent of
> retiring input_polled_dev. This converts peaq-wmi driver to use the
> polling mode of standard input devices and removes dependency on
> INPUT_POLLDEV.
>
> Because the new polling coded does not allow peeking inside the poller
> structure to get the poll interval, we change the "debounce" process to
> operate on the time basis, instead of counting events.
>
> We also fix error handling during initialization, as previously we leaked
> input device structure when we failed to register it.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>

Patch looks good to me and I've also given this a test-run on the
hw which uses this driver:

Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Tested-by: Hans de Goede <hdegoede@xxxxxxxxxx>

Regards,

Hans



> ---
>
> v2: include input.h instead of input-polldev.h
>
>   drivers/platform/x86/Kconfig    |  1 -
>   drivers/platform/x86/peaq-wmi.c | 66 +++++++++++++++++++++------------
>   2 files changed, 42 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index f0a93f630455..c703c78c59f3 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -804,7 +804,6 @@ config PEAQ_WMI
>   	tristate "PEAQ 2-in-1 WMI hotkey driver"
>   	depends on ACPI_WMI
>   	depends on INPUT
> -	select INPUT_POLLDEV
>   	help
>   	 Say Y here if you want to support WMI-based hotkeys on PEAQ 2-in-1s.
>   
> diff --git a/drivers/platform/x86/peaq-wmi.c b/drivers/platform/x86/peaq-wmi.c
> index fdeb3624c529..cf9c44c20a82 100644
> --- a/drivers/platform/x86/peaq-wmi.c
> +++ b/drivers/platform/x86/peaq-wmi.c
> @@ -6,7 +6,7 @@
>   
>   #include <linux/acpi.h>
>   #include <linux/dmi.h>
> -#include <linux/input-polldev.h>
> +#include <linux/input.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   
> @@ -18,8 +18,7 @@
>   
>   MODULE_ALIAS("wmi:"PEAQ_DOLBY_BUTTON_GUID);
>   
> -static unsigned int peaq_ignore_events_counter;
> -static struct input_polled_dev *peaq_poll_dev;
> +static struct input_dev *peaq_poll_dev;
>   
>   /*
>    * The Dolby button (yes really a Dolby button) causes an ACPI variable to get
> @@ -28,8 +27,10 @@ static struct input_polled_dev *peaq_poll_dev;
>    * (if polling after the release) or twice (polling between press and release).
>    * We ignore events for 0.5s after the first event to avoid reporting 2 presses.
>    */
> -static void peaq_wmi_poll(struct input_polled_dev *dev)
> +static void peaq_wmi_poll(struct input_dev *input_dev)
>   {
> +	static unsigned long last_event_time;
> +	static bool had_events;
>   	union acpi_object obj;
>   	acpi_status status;
>   	u32 dummy = 0;
> @@ -44,22 +45,25 @@ static void peaq_wmi_poll(struct input_polled_dev *dev)
>   		return;
>   
>   	if (obj.type != ACPI_TYPE_INTEGER) {
> -		dev_err(&peaq_poll_dev->input->dev,
> +		dev_err(&input_dev->dev,
>   			"Error WMBC did not return an integer\n");
>   		return;
>   	}
>   
> -	if (peaq_ignore_events_counter && peaq_ignore_events_counter--)
> +	if (!obj.integer.value)
>   		return;
>   
> -	if (obj.integer.value) {
> -		input_event(peaq_poll_dev->input, EV_KEY, KEY_SOUND, 1);
> -		input_sync(peaq_poll_dev->input);
> -		input_event(peaq_poll_dev->input, EV_KEY, KEY_SOUND, 0);
> -		input_sync(peaq_poll_dev->input);
> -		peaq_ignore_events_counter = max(1u,
> -			PEAQ_POLL_IGNORE_MS / peaq_poll_dev->poll_interval);
> -	}
> +	if (had_events && time_before(jiffies, last_event_time +
> +					msecs_to_jiffies(PEAQ_POLL_IGNORE_MS)))
> +		return;
> +
> +	input_event(input_dev, EV_KEY, KEY_SOUND, 1);
> +	input_sync(input_dev);
> +	input_event(input_dev, EV_KEY, KEY_SOUND, 0);
> +	input_sync(input_dev);
> +
> +	last_event_time = jiffies;
> +	had_events = true;
>   }
>   
>   /* Some other devices (Shuttle XS35) use the same WMI GUID for other purposes */
> @@ -75,6 +79,8 @@ static const struct dmi_system_id peaq_dmi_table[] __initconst = {
>   
>   static int __init peaq_wmi_init(void)
>   {
> +	int err;
> +
>   	/* WMI GUID is not unique, also check for a DMI match */
>   	if (!dmi_check_system(peaq_dmi_table))
>   		return -ENODEV;
> @@ -82,24 +88,36 @@ static int __init peaq_wmi_init(void)
>   	if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
>   		return -ENODEV;
>   
> -	peaq_poll_dev = input_allocate_polled_device();
> +	peaq_poll_dev = input_allocate_device();
>   	if (!peaq_poll_dev)
>   		return -ENOMEM;
>   
> -	peaq_poll_dev->poll = peaq_wmi_poll;
> -	peaq_poll_dev->poll_interval = PEAQ_POLL_INTERVAL_MS;
> -	peaq_poll_dev->poll_interval_max = PEAQ_POLL_MAX_MS;
> -	peaq_poll_dev->input->name = "PEAQ WMI hotkeys";
> -	peaq_poll_dev->input->phys = "wmi/input0";
> -	peaq_poll_dev->input->id.bustype = BUS_HOST;
> -	input_set_capability(peaq_poll_dev->input, EV_KEY, KEY_SOUND);
> +	peaq_poll_dev->name = "PEAQ WMI hotkeys";
> +	peaq_poll_dev->phys = "wmi/input0";
> +	peaq_poll_dev->id.bustype = BUS_HOST;
> +	input_set_capability(peaq_poll_dev, EV_KEY, KEY_SOUND);
> +
> +	err = input_setup_polling(peaq_poll_dev, peaq_wmi_poll);
> +	if (err)
> +		goto err_out;
> +
> +	input_set_poll_interval(peaq_poll_dev, PEAQ_POLL_INTERVAL_MS);
> +	input_set_max_poll_interval(peaq_poll_dev, PEAQ_POLL_MAX_MS);
> +
> +	err = input_register_device(peaq_poll_dev);
> +	if (err)
> +		goto err_out;
> +
> +	return 0;
>   
> -	return input_register_polled_device(peaq_poll_dev);
> +err_out:
> +	input_free_device(peaq_poll_dev);
> +	return err;
>   }
>   
>   static void __exit peaq_wmi_exit(void)
>   {
> -	input_unregister_polled_device(peaq_poll_dev);
> +	input_unregister_device(peaq_poll_dev);
>   }
>   
>   module_init(peaq_wmi_init);