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@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > ---
> > > 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...
--
Dmitry
