Re: [PATCH] platform/x86: ideapad-laptop: Stop calling i8042_command()

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Mon, Aug 12, 2024 at 04:41:50PM +0200, Hans de Goede wrote:
> Hi Maxim,
> 
> On 8/12/24 4:37 PM, Maxim Mikityanskiy wrote:
> > On Mon, 05 Aug 2024 at 17:45:19 +0200, Hans de Goede wrote:
> >> On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote:
> >>> That means, userspace is not filtering out events upon receiving
> >>> KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send
> >>> KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570
> >>> is weird. It maintains the touchpad state in firmware to light up the
> >>> status LED, but the firmware doesn't do the actual touchpad disablement.
> >>>
> >>> That is, if we use TOGGLE, the LED will get out of sync. If we use
> >>> ON/OFF, the touchpad won't be disabled, unless we do it in the kernel.
> >>
> >> Ack.
> >>
> >> So how about this instead:
> >>
> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> >> index 1ace711f7442..b7fa06f793cb 100644
> >> --- a/drivers/platform/x86/ideapad-laptop.c
> >> +++ b/drivers/platform/x86/ideapad-laptop.c
> >> @@ -1574,7 +1574,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
> >>  	 * touchpad off and on. We send KEY_TOUCHPAD_OFF and
> >>  	 * KEY_TOUCHPAD_ON to not to get out of sync with LED
> >>  	 */
> >> -	if (priv->features.ctrl_ps2_aux_port)
> >> +	if (send_events && priv->features.ctrl_ps2_aux_port)
> >>  		i8042_command(&param, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE);
> >>  
> >>  	/*
> >>
> >> Maxmime, if you still have your Z570 can you check if the touchpad state after a suspend/resume
> >> correctly reflects the state before suspend/resume in both touchpad on / off states ?
> > 
> > *Maxim
> 
> Oops, sorry.
> 
> > Just a heads-up, my Z570 now belongs to a family member, we'll test what
> > you asked, but right now there is a btrfs corruption on that laptop that
> > we need to fix first, it interferes with kernel compilation =/
> 
> Note as discussed in another part of the thread the original bug report
> actually was not on a Z570, so the whole usage of i8042_command() on
> suspend/resume was a bit of a red herring. And the suspend/resume issue
> has been fixed in another way in the mean time.
> 
> So there really is no need to test this change anymore. At the moment
> there are no planned changes to ideapad-laptop related to this.

I think we still need to stop ideapad-laptop poking into 8042,
especially ahead of time. If we do not want to wait for userspace to
handle this properly, I wonder if we could not create an
input_handler that would attach to the touchpad device and filter out
all events coming from the touchpad if touchpad is supposed to be off.

Thanks.

-- 
Dmitry