On Tue, 03 Sep 2024 at 16:55:54 -0700, Dmitry Torokhov wrote:
On Tue, Aug 20, 2024 at 10:28:24PM -0700, Dmitry Torokhov wrote:
On Wed, Aug 21, 2024 at 12:40:34AM +0300, Maxim Mikityanskiy wrote:
On Tue, 20 Aug 2024 at 13:46:53 +0300, Maxim Mikityanskiy wrote:
On Sun, 18 Aug 2024 at 13:30:37 -0700, Dmitry Torokhov wrote:
Maybe something like below can work?
Great patch, thank you, I'll test it and report the results. See some minor comments below.
platform/x86: ideapad-laptop: do not poke keyboard controller
From: Dmitry Torokhov dmitry.torokhov@gmail.com
On Ideapad Z570 the driver tries to disable and reenable data coming from the touchpad by poking directly into 8042 keyboard controller. This may coincide with the controller resuming and leads to spews in dmesg and potentially other instabilities.
Instead of using i8042_command() to control the touchpad state create a input handler that serves as a filter and drop events coming from the touchpad when it is supposed to be off.
Signed-off-by: Dmitry Torokhov dmitry.torokhov@gmail.com
drivers/platform/x86/ideapad-laptop.c | 171 ++++++++++++++++++++++++++++++++- 1 file changed, 168 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c index fcf13d88fd6e..2f40feefd5e3 100644 --- a/drivers/platform/x86/ideapad-laptop.c +++ b/drivers/platform/x86/ideapad-laptop.c @@ -17,7 +17,6 @@ #include <linux/device.h> #include <linux/dmi.h> #include <linux/fb.h> -#include <linux/i8042.h> #include <linux/init.h> #include <linux/input.h> #include <linux/input/sparse-keymap.h> @@ -157,6 +156,13 @@ struct ideapad_private { struct led_classdev led; unsigned int last_brightness; } fn_lock;
- struct {
bool initialized;bool active;struct input_handler handler;struct input_dev *tp_dev;spinlock_t lock;- } tp_switch;
}; static bool no_bt_rfkill; @@ -1236,6 +1242,158 @@ static void ideapad_check_special_buttons(struct ideapad_private *priv) } } +struct ideapad_tpswitch_handle {
- struct input_handle handle;
- struct ideapad_private *priv;
+};
+#define to_tpswitch_handle(h) \
- container_of(h, struct ideapad_tpswitch_handle, handle);
+static int ideapad_tpswitch_connect(struct input_handler *handler,
struct input_dev *dev,const struct input_device_id *id)+{
- struct ideapad_private *priv =
container_of(handler, struct ideapad_private, tp_switch.handler);- struct ideapad_tpswitch_handle *h;
- int error;
- h = kzalloc(sizeof(*h), GFP_KERNEL);
- if (!h)
return -ENOMEM;- h->priv = priv;
- h->handle.dev = dev;
- h->handle.handler = handler;
- h->handle.name = "ideapad-tpswitch";
- error = input_register_handle(&h->handle);
- if (error)
goto err_free_handle;- /*
* FIXME: ideally we do not want to open the input device here* if there are no other users. We need a notion of "observer"* handlers in the input core.*/- error = input_open_device(&h->handle);
- if (error)
goto err_unregister_handle;- scoped_guard(spinlock_irq, &priv->tp_switch.lock)
priv->tp_switch.tp_dev = dev;- return 0;
- err_unregister_handle:
- input_unregister_handle(&h->handle);
+err_free_handle:
- kfree(h);
- return error;
+}
+static void ideapad_tpswitch_disconnect(struct input_handle *handle) +{
- struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle);
- struct ideapad_private *priv = h->priv;
- scoped_guard(spinlock_irq, &priv->tp_switch.lock)
Nice syntax, I didn't know about it before.
priv->tp_switch.tp_dev = NULL;- input_close_device(handle);
- input_unregister_handle(handle);
- kfree(h);
+}
+static bool ideapad_tpswitch_filter(struct input_handle *handle,
unsigned int type, unsigned int code,int value)+{
- struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle);
- struct ideapad_private *priv = h->priv;
- if (!priv->tp_switch.active)
This check seems inverted. ideapad_tpswitch_toggle assigns true when the touchpad is enabled.
I tested the patch on Z570 (with this check inverted), and it seems to work great.
Also tested what happens on resume from suspend: the laptop reenables the touchpad (the LED turns off on suspend and blinks briefly on resume), and the driver handles it properly.
Great, thank you! Give me a couple of days and I think I will implement observer/passive handler support and we can figure out how to merge this...
OK, so if you could try the patch below that would be great. Don't forget to set ".passive_observer = 1" in the ideapad handler.
Sorry for the slow response. I tested the patches, setting passive_observer in ideapad_tpswitch_init and inverting the check in ideapad_tpswitch_filter - all seems to work perfectly!
Thanks!
-- Dmitry
Input: introduce notion of passive observers for input handlers
Signed-off-by: Dmitry Torokhov dmitry.torokhov@gmail.com
drivers/input/input.c | 7 +++++++ include/linux/input.h | 5 +++++ 2 files changed, 12 insertions(+)
diff --git a/drivers/input/input.c b/drivers/input/input.c index 54c57b267b25..60a9445d78d5 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -605,6 +605,9 @@ int input_open_device(struct input_handle *handle) handle->open++;
- if (handle->handler->passive_observer)
goto out;- if (dev->users++ || dev->inhibited) { /*
- Device is already opened and/or inhibited,
@@ -668,6 +671,9 @@ void input_close_device(struct input_handle *handle) __input_release_device(handle);
- if (handle->handler->passive_observer)
goto out;- if (!--dev->users && !dev->inhibited) { if (dev->poller) input_dev_poller_stop(dev->poller);
@@ -684,6 +690,7 @@ void input_close_device(struct input_handle *handle) synchronize_rcu(); } +out: mutex_unlock(&dev->mutex); } EXPORT_SYMBOL(input_close_device); diff --git a/include/linux/input.h b/include/linux/input.h index 89a0be6ee0e2..6437c35f0796 100644 --- a/include/linux/input.h +++ b/include/linux/input.h @@ -286,6 +286,10 @@ struct input_handle;
- @start: starts handler for given handle. This function is called by
- input core right after connect() method and also when a process
- that "grabbed" a device releases it
- @passive_observer: set to %true by drivers only interested in observing
- data stream from devices if there are other users present. Such
- drivers will not result in starting underlying hardware device
- when input_open_device() is called for their handles
- @legacy_minors: set to %true by drivers using legacy minor ranges
- @minor: beginning of range of 32 legacy minors for devices this driver
- can provide
@@ -321,6 +325,7 @@ struct input_handler { void (*disconnect)(struct input_handle *handle); void (*start)(struct input_handle *handle);
- bool passive_observer; bool legacy_minors; int minor; const char *name;