[1/1] ideapad-laptop: Handle Yoga in tablet mode

Commit Message

Christian Hesse April 1, 2016, 9:02 p.m. UTC

From: Christian Hesse <[e-mail]>

When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
send touchpad key codes so software can disable touchpad.

Signed-off-by: Christian Hesse <[e-mail]>
Signed-off-by: Michael Gisbers <[e-mail]>
---
 drivers/platform/x86/ideapad-laptop.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Darren Hart May 5, 2016, 11:42 p.m. UTC | #1

On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
> From: Christian Hesse <[e-mail]>
> 
> When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
> send touchpad key codes so software can disable touchpad.
> 
> Signed-off-by: Christian Hesse <[e-mail]>
> Signed-off-by: Michael Gisbers <[e-mail]>

Queued, thanks.

> ---
>  drivers/platform/x86/ideapad-laptop.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index be3bc2f..1d49db1 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -809,6 +809,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>  			case 6:
>  				ideapad_input_report(priv, vpc_bit);
>  				break;
> +			case 10:
>  			case 5:
>  				ideapad_sync_touchpad_state(priv);
>  				break;
> -- 
> 2.8.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

João Paulo Rechi Vita May 16, 2016, 4:04 p.m. UTC | #2

Adding Maxim Mikityanskiy and Hans de Goede to CC.

On 5 May 2016 at 19:42, Darren Hart <[e-mail]> wrote:
> On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
>> From: Christian Hesse <[e-mail]>
>>
>> When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
>> send touchpad key codes so software can disable touchpad.
>>
>> Signed-off-by: Christian Hesse <[e-mail]>
>> Signed-off-by: Michael Gisbers <[e-mail]>
>
> Queued, thanks.
>
>> ---
>>  drivers/platform/x86/ideapad-laptop.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>> index be3bc2f..1d49db1 100644
>> --- a/drivers/platform/x86/ideapad-laptop.c
>> +++ b/drivers/platform/x86/ideapad-laptop.c
>> @@ -809,6 +809,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>>                       case 6:
>>                               ideapad_input_report(priv, vpc_bit);
>>                               break;
>> +                     case 10:
>>                       case 5:
>>                               ideapad_sync_touchpad_state(priv);

I'm not sure this is the right action here: ideapad_sync_touchpad()
sends a sequence of KEY_TOUCHPAD_DISABLE and KEY_TOUCHPAD_ENABLED,
which are interpreted by userspace as "the touchpad has been
{dis,en}abled by the hardware". The way userspace is expected to react
to it is by showing a notification of the fact to the user, but not
disabling the touchpad. This behavior can be confirmed in the original
commit message that introduced this change (I couldn't find any actual
documentation on this): "Input: add keycodes for touchpad on/off keys"
(0417596f66). I was actually going to propose a patch similar to Hans'
"ideapad-laptop: Disable touchpad interface on Yoga models"
(f79a901331) for the Yoga 900, to avoid an "touchpad ON" OSD
notification being shown to the user when returning for suspend.

Also, I've been investigating touchpad-related problems in the Lenovo
Yoga 900 recently, and trying to understand this code. IIUC
ideadpad_sync_touchpad_state() was created as part of "ideapad: add
Lenovo IdeaPad Z570 support (part 2)" (07a4a4fc8) to allow userspace
to syncronize a touchpad-state-indicator LED with the actual touchpad
state, although it seems to me it only works if the touchpad is always
enabled by the hardware on resume (cc'ing the original patch author
here for maybe a confirmation on how this is expected to work).

Additionally, while I indeed see an unknown event reported by
ideapad_laptop when closing the lid on the Yoga 900, it is not
vpc_bit=10, but vpc_bit=1. But I also see the keyboard device sending
the same scancode that is sent when the actual touchpad-toggle media
key is pressed (F6 on the Yoga 900), so I'm not 100% sure how all of
this play together yet. I think it would be great if we could
understand the problem a bit better and find a more generic solution.
Were you able to trace the DSDT code that generates this notification?

Best regards,

--
João Paulo Rechi Vita
http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Maxim Mikityanskiy May 19, 2016, 9:17 a.m. UTC | #3

2016-05-16 19:04 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>
> Adding Maxim Mikityanskiy and Hans de Goede to CC.
>
> On 5 May 2016 at 19:42, Darren Hart <[e-mail]> wrote:
> > On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
> >> From: Christian Hesse <[e-mail]>
> >>
> >> When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
> >> send touchpad key codes so software can disable touchpad.
> >>
> >> Signed-off-by: Christian Hesse <[e-mail]>
> >> Signed-off-by: Michael Gisbers <[e-mail]>
> >
> > Queued, thanks.
> >
> >> ---
> >>  drivers/platform/x86/ideapad-laptop.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> >> index be3bc2f..1d49db1 100644
> >> --- a/drivers/platform/x86/ideapad-laptop.c
> >> +++ b/drivers/platform/x86/ideapad-laptop.c
> >> @@ -809,6 +809,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
> >>                       case 6:
> >>                               ideapad_input_report(priv, vpc_bit);
> >>                               break;
> >> +                     case 10:
> >>                       case 5:
> >>                               ideapad_sync_touchpad_state(priv);
>
> I'm not sure this is the right action here: ideapad_sync_touchpad()
> sends a sequence of KEY_TOUCHPAD_DISABLE and KEY_TOUCHPAD_ENABLED,
> which are interpreted by userspace as "the touchpad has been
> {dis,en}abled by the hardware". The way userspace is expected to react
> to it is by showing a notification of the fact to the user, but not
> disabling the touchpad. This behavior can be confirmed in the original
> commit message that introduced this change (I couldn't find any actual
> documentation on this): "Input: add keycodes for touchpad on/off keys"
> (0417596f66). I was actually going to propose a patch similar to Hans'
> "ideapad-laptop: Disable touchpad interface on Yoga models"
> (f79a901331) for the Yoga 900, to avoid an "touchpad ON" OSD
> notification being shown to the user when returning for suspend.
>
> Also, I've been investigating touchpad-related problems in the Lenovo
> Yoga 900 recently, and trying to understand this code. IIUC
> ideadpad_sync_touchpad_state() was created as part of "ideapad: add
> Lenovo IdeaPad Z570 support (part 2)" (07a4a4fc8) to allow userspace
> to syncronize a touchpad-state-indicator LED with the actual touchpad
> state, although it seems to me it only works if the touchpad is always
> enabled by the hardware on resume (cc'ing the original patch author
> here for maybe a confirmation on how this is expected to work).

This is a short explanation of this function.

We call ideapad_sync_touchpad_state when a touchpad state changed
event arrives (touchpad on/off button pressed) or we need to read  out
the initial touchpad state. It is also called on resume because
possibly the touchpad state may be changed after resume, but I don't
know which devices behave like that (and unfortunately I'm unable to
test it on my Z570 because now it's dead).

ideadpad_sync_touchpad_state serves several purposes:

- It actually reads out current touchpad state. It is necessary when
we are reading the initial state, and also this read is necessary on
touchpad state changed events, because on some laptops (e.g. Z570)
touchpad LED only changes its state after this read.

- Then it disables/enables i8042 AUX port to disable/enable touchpad.
It is also necessary, because some laptops (Z570) do not disable
touchpad in hardware although they are storing touchpad state.

- Finally it sends userspace event with new touchpad state. It does
not send a sequence of KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON as you
mentioned, but only one of these key codes.

The second point is kinda hack. In Linux there are two ways of
reporting touchpad state events to the userspace:

- KEY_TOUCHPAD_TOGGLE for those laptops that do not disable touchpad
in hardware and thus don't store touchpad state. The userspace should
listen for those key presses and disable/enable touchpad
programmatically (e.g. TouchpadOff in synaptics X11 driver).

- KEY_TOUCHPAD_ON and KEY_TOUCHPAD_OFF for those laptops that fully
manage touchpad state in hardware and just report its state to the
userspace so that notification can be shown. Userspace should not
disable touchpad programmatically on these events, because it is
managed by hardware and may easily get out of sync if using multiple
user sessions.

Ideapad Z570 matches none of these options. It looks more like the
second one, it stores the touchpad state in hardware, controls the
LED, but it lacks the ability to disable the touchpad in hardware. So
we need to keep track of the hardware state and disable i8042 AUX port
from the driver when necessary.

So if on event 10 the touchpad state may be changed, we need to call
ideadpad_sync_touchpad_state, otherwise I don't see why it is
necessary.

Fell free to ask me any questions if something is still not clear,
I'll try to assist.

Best regards,
Maxim
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

João Paulo Rechi Vita May 24, 2016, 8:32 p.m. UTC | #4

On 19 May 2016 at 05:17, Maxim Mikityanskiy <[e-mail]> wrote:
> 2016-05-16 19:04 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>
>> Adding Maxim Mikityanskiy and Hans de Goede to CC.
>>
>> On 5 May 2016 at 19:42, Darren Hart <[e-mail]> wrote:
>> > On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
>> >> From: Christian Hesse <[e-mail]>
>> >>
>> >> When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
>> >> send touchpad key codes so software can disable touchpad.
>> >>
>> >> Signed-off-by: Christian Hesse <[e-mail]>
>> >> Signed-off-by: Michael Gisbers <[e-mail]>
>> >
>> > Queued, thanks.
>> >
>> >> ---
>> >>  drivers/platform/x86/ideapad-laptop.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>> >> index be3bc2f..1d49db1 100644
>> >> --- a/drivers/platform/x86/ideapad-laptop.c
>> >> +++ b/drivers/platform/x86/ideapad-laptop.c
>> >> @@ -809,6 +809,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>> >>                       case 6:
>> >>                               ideapad_input_report(priv, vpc_bit);
>> >>                               break;
>> >> +                     case 10:
>> >>                       case 5:
>> >>                               ideapad_sync_touchpad_state(priv);
>>
>> I'm not sure this is the right action here: ideapad_sync_touchpad()
>> sends a sequence of KEY_TOUCHPAD_DISABLE and KEY_TOUCHPAD_ENABLED,
>> which are interpreted by userspace as "the touchpad has been
>> {dis,en}abled by the hardware". The way userspace is expected to react
>> to it is by showing a notification of the fact to the user, but not
>> disabling the touchpad. This behavior can be confirmed in the original
>> commit message that introduced this change (I couldn't find any actual
>> documentation on this): "Input: add keycodes for touchpad on/off keys"
>> (0417596f66). I was actually going to propose a patch similar to Hans'
>> "ideapad-laptop: Disable touchpad interface on Yoga models"
>> (f79a901331) for the Yoga 900, to avoid an "touchpad ON" OSD
>> notification being shown to the user when returning for suspend.
>>
>> Also, I've been investigating touchpad-related problems in the Lenovo
>> Yoga 900 recently, and trying to understand this code. IIUC
>> ideadpad_sync_touchpad_state() was created as part of "ideapad: add
>> Lenovo IdeaPad Z570 support (part 2)" (07a4a4fc8) to allow userspace
>> to syncronize a touchpad-state-indicator LED with the actual touchpad
>> state, although it seems to me it only works if the touchpad is always
>> enabled by the hardware on resume (cc'ing the original patch author
>> here for maybe a confirmation on how this is expected to work).
>
> This is a short explanation of this function.
>
> We call ideapad_sync_touchpad_state when a touchpad state changed
> event arrives (touchpad on/off button pressed) or we need to read  out
> the initial touchpad state. It is also called on resume because
> possibly the touchpad state may be changed after resume, but I don't
> know which devices behave like that (and unfortunately I'm unable to
> test it on my Z570 because now it's dead).
>

So if I'm following this correctly, on some ideapads (Z570 and maybe
others) there is a ACPI notification when the touchpad on/off hotkey
is pressed, is that correct? This is not what I see on the Yoga 900 or
the Yoga 3, but a special scancode (0xbf on the Yoga 900, 0xbe on the
Yoga 3) come through the AT keyboard device. So here I'm using hwdb to
map these to KEY_TOUCHPAD_TOGGLE (pending upstream submission to
udev).

> ideadpad_sync_touchpad_state serves several purposes:
>
> - It actually reads out current touchpad state. It is necessary when
> we are reading the initial state, and also this read is necessary on
> touchpad state changed events, because on some laptops (e.g. Z570)
> touchpad LED only changes its state after this read.
>

On the Yogas I've been testing this the state being read on
ideapad_sync_touchpad_state() never changes, so even if I press the
touchpad disable/enable hotkey before suspending, I always get
KEY_TOUCHPAD_ON.

> - Then it disables/enables i8042 AUX port to disable/enable touchpad.
> It is also necessary, because some laptops (Z570) do not disable
> touchpad in hardware although they are storing touchpad state.
>

Ok, but this will only work if the touchpad is connected through that
bus. On the Yoga 900 and Yoga 3 the are connected via I2C, and I guess
this might be true for other machines as well.

> - Finally it sends userspace event with new touchpad state. It does
> not send a sequence of KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON as you
> mentioned, but only one of these key codes.
>

Right, I got confused when I wrote this email by the comments in the
function implementation, but when I checked with evtest I did see only
one event being sent, indeed.

> The second point is kinda hack. In Linux there are two ways of
> reporting touchpad state events to the userspace:
>
> - KEY_TOUCHPAD_TOGGLE for those laptops that do not disable touchpad
> in hardware and thus don't store touchpad state. The userspace should
> listen for those key presses and disable/enable touchpad
> programmatically (e.g. TouchpadOff in synaptics X11 driver).
>

Yes, when the notification for the disable/enable touchpad hotkey goes
through ACPI.

> - KEY_TOUCHPAD_ON and KEY_TOUCHPAD_OFF for those laptops that fully
> manage touchpad state in hardware and just report its state to the
> userspace so that notification can be shown. Userspace should not
> disable touchpad programmatically on these events, because it is
> managed by hardware and may easily get out of sync if using multiple
> user sessions.
>

Yes, and that is what I tried to explain on my previous comment. In
this case userspace will usually want to notify the user that the
touchpad state has been changed.

> Ideapad Z570 matches none of these options. It looks more like the
> second one, it stores the touchpad state in hardware, controls the
> LED, but it lacks the ability to disable the touchpad in hardware. So
> we need to keep track of the hardware state and disable i8042 AUX port
> from the driver when necessary.
>

Isn't the case that what it actually stores under VPCCMD_R_TOUCHPAD is
only the touchpad LED state? If so, wouldn't it work if when ACPI
notifies the kernel of event 5 (the touchpad disable/enable key has
been pressed) we simply sent KEY_TOUCHPAD_TOGGLE to userspace? I'm not
entirely sure what would be best way to keep the LED in sync with the
touchpad state in that case, the only way I can think of if to expose
the LED to userspace so it can update it accordingly.

> So if on event 10 the touchpad state may be changed, we need to call
> ideadpad_sync_touchpad_state, otherwise I don't see why it is
> necessary.
>

The commit message on the original commit on this thread says "Let's
send touchpad key codes so software can disable touchpad.", but as we
just discussed userspace is not supposed to disable the touchpad on
KEY_TOUCHPAD_DISABLE, so I guess it falls under the same category as
the IdeaPad Z570: the touchpad is connected via i8042 and it has a
variable in ACPI to store the touchpad LED state.

It seems to me that at least we should have some quirks in place so
this only gets executed on hardware where it will actually work, to
avoid notifying userspace of touchpad events when there were actually
none. Darren, what do you think is the best way to move forward here?
If we can reach an agreement I can provide some patches for this.

> Fell free to ask me any questions if something is still not clear,
> I'll try to assist.
>

Thank you very much for taking the time to explain how this piece of
code currently works, and sorry for a bit of confusion in my previous
message.

Regards,

--
João Paulo Rechi Vita
http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Maxim Mikityanskiy May 25, 2016, 6:26 a.m. UTC | #5

2016-05-24 23:32 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
> On 19 May 2016 at 05:17, Maxim Mikityanskiy <[e-mail]> wrote:
>> 2016-05-16 19:04 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>
>>> Adding Maxim Mikityanskiy and Hans de Goede to CC.
>>>
>>> On 5 May 2016 at 19:42, Darren Hart <[e-mail]> wrote:
>>> > On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
>>> >> From: Christian Hesse <[e-mail]>
>>> >>
>>> >> When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
>>> >> send touchpad key codes so software can disable touchpad.
>>> >>
>>> >> Signed-off-by: Christian Hesse <[e-mail]>
>>> >> Signed-off-by: Michael Gisbers <[e-mail]>
>>> >
>>> > Queued, thanks.
>>> >
>>> >> ---
>>> >>  drivers/platform/x86/ideapad-laptop.c | 1 +
>>> >>  1 file changed, 1 insertion(+)
>>> >>
>>> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>>> >> index be3bc2f..1d49db1 100644
>>> >> --- a/drivers/platform/x86/ideapad-laptop.c
>>> >> +++ b/drivers/platform/x86/ideapad-laptop.c
>>> >> @@ -809,6 +809,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>>> >>                       case 6:
>>> >>                               ideapad_input_report(priv, vpc_bit);
>>> >>                               break;
>>> >> +                     case 10:
>>> >>                       case 5:
>>> >>                               ideapad_sync_touchpad_state(priv);
>>>
>>> I'm not sure this is the right action here: ideapad_sync_touchpad()
>>> sends a sequence of KEY_TOUCHPAD_DISABLE and KEY_TOUCHPAD_ENABLED,
>>> which are interpreted by userspace as "the touchpad has been
>>> {dis,en}abled by the hardware". The way userspace is expected to react
>>> to it is by showing a notification of the fact to the user, but not
>>> disabling the touchpad. This behavior can be confirmed in the original
>>> commit message that introduced this change (I couldn't find any actual
>>> documentation on this): "Input: add keycodes for touchpad on/off keys"
>>> (0417596f66). I was actually going to propose a patch similar to Hans'
>>> "ideapad-laptop: Disable touchpad interface on Yoga models"
>>> (f79a901331) for the Yoga 900, to avoid an "touchpad ON" OSD
>>> notification being shown to the user when returning for suspend.
>>>
>>> Also, I've been investigating touchpad-related problems in the Lenovo
>>> Yoga 900 recently, and trying to understand this code. IIUC
>>> ideadpad_sync_touchpad_state() was created as part of "ideapad: add
>>> Lenovo IdeaPad Z570 support (part 2)" (07a4a4fc8) to allow userspace
>>> to syncronize a touchpad-state-indicator LED with the actual touchpad
>>> state, although it seems to me it only works if the touchpad is always
>>> enabled by the hardware on resume (cc'ing the original patch author
>>> here for maybe a confirmation on how this is expected to work).
>>
>> This is a short explanation of this function.
>>
>> We call ideapad_sync_touchpad_state when a touchpad state changed
>> event arrives (touchpad on/off button pressed) or we need to read  out
>> the initial touchpad state. It is also called on resume because
>> possibly the touchpad state may be changed after resume, but I don't
>> know which devices behave like that (and unfortunately I'm unable to
>> test it on my Z570 because now it's dead).
>>
>
> So if I'm following this correctly, on some ideapads (Z570 and maybe
> others) there is a ACPI notification when the touchpad on/off hotkey
> is pressed, is that correct?

Yes, exactly.

> This is not what I see on the Yoga 900 or
> the Yoga 3, but a special scancode (0xbf on the Yoga 900, 0xbe on the
> Yoga 3) come through the AT keyboard device. So here I'm using hwdb to
> map these to KEY_TOUCHPAD_TOGGLE (pending upstream submission to
> udev).

If KEY_TOUCHPAD_TOGGLE is sent to the userspace on the Yoga, no
KEY_TOUCHPAD_ON or KEY_TOUCHPAD_OFF should be ever sent. Only one of
these mechanisms should be used for userspace to handle this
correctly.

>> ideadpad_sync_touchpad_state serves several purposes:
>>
>> - It actually reads out current touchpad state. It is necessary when
>> we are reading the initial state, and also this read is necessary on
>> touchpad state changed events, because on some laptops (e.g. Z570)
>> touchpad LED only changes its state after this read.
>>
>
> On the Yogas I've been testing this the state being read on
> ideapad_sync_touchpad_state() never changes, so even if I press the
> touchpad disable/enable hotkey before suspending, I always get
> KEY_TOUCHPAD_ON.

So if VPCCMD_R_TOUCHPAD always returns the same value and not the
actual touchpad state, we should not send KEY_TOUCHPAD_ON on these
devices as it conflicts with KEY_TOUCHPAD_TOGGLE and doesn't reflect
the real touchpad state. Is there any way to determine in runtime if
VPCCMD_R_TOUCHPAD works correctly on the device? BTW, could you send
me a DSDT dump from Yoga where it always shows touchpad on?

>> - Then it disables/enables i8042 AUX port to disable/enable touchpad.
>> It is also necessary, because some laptops (Z570) do not disable
>> touchpad in hardware although they are storing touchpad state.
>>
>
> Ok, but this will only work if the touchpad is connected through that
> bus. On the Yoga 900 and Yoga 3 the are connected via I2C, and I guess
> this might be true for other machines as well.

If I remember correctly, some ideapads actually disable the touchpad
in hardware and some (e.g. Z570) only toggle the LED. For those ones
that have PS/2 touchpad and don't disable it in hardware,
I8042_CMD_AUX_ENABLE/DISABLE works. I'm not sure if any ideapad exists
that has e.g. I2C touchpad and doesn't disable it in hardware, but
only toggles the LED. If such laptop is a case, i8042 method will not
work. Is there any better and universal way to disable touchpad from
the driver?

>> - Finally it sends userspace event with new touchpad state. It does
>> not send a sequence of KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON as you
>> mentioned, but only one of these key codes.
>>
>
> Right, I got confused when I wrote this email by the comments in the
> function implementation, but when I checked with evtest I did see only
> one event being sent, indeed.

OK, when I read those comments now, they look really confusing,
espessially "We send KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON" part. Sorry
for this confusion, it should mean that we send one of those codes
depending on the touchpad state, not that we send both of them.

>> The second point is kinda hack. In Linux there are two ways of
>> reporting touchpad state events to the userspace:
>>
>> - KEY_TOUCHPAD_TOGGLE for those laptops that do not disable touchpad
>> in hardware and thus don't store touchpad state. The userspace should
>> listen for those key presses and disable/enable touchpad
>> programmatically (e.g. TouchpadOff in synaptics X11 driver).
>>
>
> Yes, when the notification for the disable/enable touchpad hotkey goes
> through ACPI.

Sorry, why is ACPI important in this case?

>> - KEY_TOUCHPAD_ON and KEY_TOUCHPAD_OFF for those laptops that fully
>> manage touchpad state in hardware and just report its state to the
>> userspace so that notification can be shown. Userspace should not
>> disable touchpad programmatically on these events, because it is
>> managed by hardware and may easily get out of sync if using multiple
>> user sessions.
>>
>
> Yes, and that is what I tried to explain on my previous comment. In
> this case userspace will usually want to notify the user that the
> touchpad state has been changed.
>
>> Ideapad Z570 matches none of these options. It looks more like the
>> second one, it stores the touchpad state in hardware, controls the
>> LED, but it lacks the ability to disable the touchpad in hardware. So
>> we need to keep track of the hardware state and disable i8042 AUX port
>> from the driver when necessary.
>>
>
> Isn't the case that what it actually stores under VPCCMD_R_TOUCHPAD is
> only the touchpad LED state?

Yes, Z570 only stores the LED state.

> If so, wouldn't it work if when ACPI
> notifies the kernel of event 5 (the touchpad disable/enable key has
> been pressed) we simply sent KEY_TOUCHPAD_TOGGLE to userspace? I'm not
> entirely sure what would be best way to keep the LED in sync with the
> touchpad state in that case, the only way I can think of if to expose
> the LED to userspace so it can update it accordingly.

No, sadly it wouldn't work correctly, because on Z570 we can't change
the LED state programmatically. There is VPCCMD_W_TOUCHPAD, but as far
as I remember it doesn't work at least on Z570.

>> So if on event 10 the touchpad state may be changed, we need to call
>> ideadpad_sync_touchpad_state, otherwise I don't see why it is
>> necessary.
>>
>
> The commit message on the original commit on this thread says "Let's
> send touchpad key codes so software can disable touchpad.", but as we
> just discussed userspace is not supposed to disable the touchpad on
> KEY_TOUCHPAD_DISABLE, so I guess it falls under the same category as
> the IdeaPad Z570: the touchpad is connected via i8042 and it has a
> variable in ACPI to store the touchpad LED state.

Yes, sure, those key codes sent from ideapad_sync_touchpad_state will
not be considered in userspace as a command to disable touchpad in
software.

> It seems to me that at least we should have some quirks in place so
> this only gets executed on hardware where it will actually work, to
> avoid notifying userspace of touchpad events when there were actually
> none. Darren, what do you think is the best way to move forward here?
> If we can reach an agreement I can provide some patches for this.
>
>> Fell free to ask me any questions if something is still not clear,
>> I'll try to assist.
>>
>
> Thank you very much for taking the time to explain how this piece of
> code currently works, and sorry for a bit of confusion in my previous
> message.
>
> Regards,
>
> --
> João Paulo Rechi Vita
> http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

João Paulo Rechi Vita May 25, 2016, 4:03 p.m. UTC | #6

On 25 May 2016 at 02:26, Maxim Mikityanskiy <[e-mail]> wrote:
> 2016-05-24 23:32 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>> On 19 May 2016 at 05:17, Maxim Mikityanskiy <[e-mail]> wrote:
>>> 2016-05-16 19:04 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>
>>>> Adding Maxim Mikityanskiy and Hans de Goede to CC.
>>>>
>>>> On 5 May 2016 at 19:42, Darren Hart <[e-mail]> wrote:
>>>> > On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
>>>> >> From: Christian Hesse <[e-mail]>
>>>> >>
>>>> >> When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
>>>> >> send touchpad key codes so software can disable touchpad.
>>>> >>
>>>> >> Signed-off-by: Christian Hesse <[e-mail]>
>>>> >> Signed-off-by: Michael Gisbers <[e-mail]>
>>>> >
>>>> > Queued, thanks.
>>>> >
>>>> >> ---
>>>> >>  drivers/platform/x86/ideapad-laptop.c | 1 +
>>>> >>  1 file changed, 1 insertion(+)
>>>> >>
>>>> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>>>> >> index be3bc2f..1d49db1 100644
>>>> >> --- a/drivers/platform/x86/ideapad-laptop.c
>>>> >> +++ b/drivers/platform/x86/ideapad-laptop.c
>>>> >> @@ -809,6 +809,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>>>> >>                       case 6:
>>>> >>                               ideapad_input_report(priv, vpc_bit);
>>>> >>                               break;
>>>> >> +                     case 10:
>>>> >>                       case 5:
>>>> >>                               ideapad_sync_touchpad_state(priv);
>>>>
>>>> I'm not sure this is the right action here: ideapad_sync_touchpad()
>>>> sends a sequence of KEY_TOUCHPAD_DISABLE and KEY_TOUCHPAD_ENABLED,
>>>> which are interpreted by userspace as "the touchpad has been
>>>> {dis,en}abled by the hardware". The way userspace is expected to react
>>>> to it is by showing a notification of the fact to the user, but not
>>>> disabling the touchpad. This behavior can be confirmed in the original
>>>> commit message that introduced this change (I couldn't find any actual
>>>> documentation on this): "Input: add keycodes for touchpad on/off keys"
>>>> (0417596f66). I was actually going to propose a patch similar to Hans'
>>>> "ideapad-laptop: Disable touchpad interface on Yoga models"
>>>> (f79a901331) for the Yoga 900, to avoid an "touchpad ON" OSD
>>>> notification being shown to the user when returning for suspend.
>>>>
>>>> Also, I've been investigating touchpad-related problems in the Lenovo
>>>> Yoga 900 recently, and trying to understand this code. IIUC
>>>> ideadpad_sync_touchpad_state() was created as part of "ideapad: add
>>>> Lenovo IdeaPad Z570 support (part 2)" (07a4a4fc8) to allow userspace
>>>> to syncronize a touchpad-state-indicator LED with the actual touchpad
>>>> state, although it seems to me it only works if the touchpad is always
>>>> enabled by the hardware on resume (cc'ing the original patch author
>>>> here for maybe a confirmation on how this is expected to work).
>>>
>>> This is a short explanation of this function.
>>>
>>> We call ideapad_sync_touchpad_state when a touchpad state changed
>>> event arrives (touchpad on/off button pressed) or we need to read  out
>>> the initial touchpad state. It is also called on resume because
>>> possibly the touchpad state may be changed after resume, but I don't
>>> know which devices behave like that (and unfortunately I'm unable to
>>> test it on my Z570 because now it's dead).
>>>
>>
>> So if I'm following this correctly, on some ideapads (Z570 and maybe
>> others) there is a ACPI notification when the touchpad on/off hotkey
>> is pressed, is that correct?
>
> Yes, exactly.
>
>> This is not what I see on the Yoga 900 or
>> the Yoga 3, but a special scancode (0xbf on the Yoga 900, 0xbe on the
>> Yoga 3) come through the AT keyboard device. So here I'm using hwdb to
>> map these to KEY_TOUCHPAD_TOGGLE (pending upstream submission to
>> udev).
>
> If KEY_TOUCHPAD_TOGGLE is sent to the userspace on the Yoga, no
> KEY_TOUCHPAD_ON or KEY_TOUCHPAD_OFF should be ever sent. Only one of
> these mechanisms should be used for userspace to handle this
> correctly.
>

Agreed. That's why I was planning to propose a patch very similar to
Hans' (f79a901331 "ideapad-laptop: Disable touchpad interface on Yoga
models"), but it was later reverted by 3b264d279e. I wonder if there
is a more specific way to tell one model from the other.

>>> ideadpad_sync_touchpad_state serves several purposes:
>>>
>>> - It actually reads out current touchpad state. It is necessary when
>>> we are reading the initial state, and also this read is necessary on
>>> touchpad state changed events, because on some laptops (e.g. Z570)
>>> touchpad LED only changes its state after this read.
>>>
>>
>> On the Yogas I've been testing this the state being read on
>> ideapad_sync_touchpad_state() never changes, so even if I press the
>> touchpad disable/enable hotkey before suspending, I always get
>> KEY_TOUCHPAD_ON.
>
> So if VPCCMD_R_TOUCHPAD always returns the same value and not the
> actual touchpad state, we should not send KEY_TOUCHPAD_ON on these
> devices as it conflicts with KEY_TOUCHPAD_TOGGLE and doesn't reflect
> the real touchpad state. Is there any way to determine in runtime if
> VPCCMD_R_TOUCHPAD works correctly on the device? BTW, could you send
> me a DSDT dump from Yoga where it always shows touchpad on?
>

I imagine it is expected to return the same value because, from the
hardware perspective, the touchpad is always enabled. I'm not sure of
a good way to verify this, maybe disabling/re-enabling the i8042 AUX
and checking its value when the module first loads (not sure if this
is a good idea). The Yoga 900 DSDT can be found here:
https://gist.githubusercontent.com/jprvita/5737de3cbb670e80973b7d4e51c38ab6/raw/189220cb746a719026bde2fa3e75ef5cb24e1931/yoga-900-dsdt.dsl

>>> - Then it disables/enables i8042 AUX port to disable/enable touchpad.
>>> It is also necessary, because some laptops (Z570) do not disable
>>> touchpad in hardware although they are storing touchpad state.
>>>
>>
>> Ok, but this will only work if the touchpad is connected through that
>> bus. On the Yoga 900 and Yoga 3 the are connected via I2C, and I guess
>> this might be true for other machines as well.
>
> If I remember correctly, some ideapads actually disable the touchpad
> in hardware and some (e.g. Z570) only toggle the LED. For those ones
> that have PS/2 touchpad and don't disable it in hardware,
> I8042_CMD_AUX_ENABLE/DISABLE works. I'm not sure if any ideapad exists
> that has e.g. I2C touchpad and doesn't disable it in hardware, but
> only toggles the LED. If such laptop is a case, i8042 method will not
> work. Is there any better and universal way to disable touchpad from
> the driver?
>

I don't know if such laptop model exists either, but we can probably
leave this case for when (if) it shows up.

>>> - Finally it sends userspace event with new touchpad state. It does
>>> not send a sequence of KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON as you
>>> mentioned, but only one of these key codes.
>>>
>>
>> Right, I got confused when I wrote this email by the comments in the
>> function implementation, but when I checked with evtest I did see only
>> one event being sent, indeed.
>
> OK, when I read those comments now, they look really confusing,
> espessially "We send KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON" part. Sorry
> for this confusion, it should mean that we send one of those codes
> depending on the touchpad state, not that we send both of them.
>
>>> The second point is kinda hack. In Linux there are two ways of
>>> reporting touchpad state events to the userspace:
>>>
>>> - KEY_TOUCHPAD_TOGGLE for those laptops that do not disable touchpad
>>> in hardware and thus don't store touchpad state. The userspace should
>>> listen for those key presses and disable/enable touchpad
>>> programmatically (e.g. TouchpadOff in synaptics X11 driver).
>>>
>>
>> Yes, when the notification for the disable/enable touchpad hotkey goes
>> through ACPI.
>
> Sorry, why is ACPI important in this case?
>

I mean, if the hardware sends a notification through ACPI that needs
to be handled by the kernel, instead of a keypress event on the
keyboard device with a special scancode. But anyway, my comment here
didn't really add anything.

>>> - KEY_TOUCHPAD_ON and KEY_TOUCHPAD_OFF for those laptops that fully
>>> manage touchpad state in hardware and just report its state to the
>>> userspace so that notification can be shown. Userspace should not
>>> disable touchpad programmatically on these events, because it is
>>> managed by hardware and may easily get out of sync if using multiple
>>> user sessions.
>>>
>>
>> Yes, and that is what I tried to explain on my previous comment. In
>> this case userspace will usually want to notify the user that the
>> touchpad state has been changed.
>>
>>> Ideapad Z570 matches none of these options. It looks more like the
>>> second one, it stores the touchpad state in hardware, controls the
>>> LED, but it lacks the ability to disable the touchpad in hardware. So
>>> we need to keep track of the hardware state and disable i8042 AUX port
>>> from the driver when necessary.
>>>
>>
>> Isn't the case that what it actually stores under VPCCMD_R_TOUCHPAD is
>> only the touchpad LED state?
>
> Yes, Z570 only stores the LED state.
>
>> If so, wouldn't it work if when ACPI
>> notifies the kernel of event 5 (the touchpad disable/enable key has
>> been pressed) we simply sent KEY_TOUCHPAD_TOGGLE to userspace? I'm not
>> entirely sure what would be best way to keep the LED in sync with the
>> touchpad state in that case, the only way I can think of if to expose
>> the LED to userspace so it can update it accordingly.
>
> No, sadly it wouldn't work correctly, because on Z570 we can't change
> the LED state programmatically. There is VPCCMD_W_TOUCHPAD, but as far
> as I remember it doesn't work at least on Z570.
>

So what you are saying is that the only way to update the LED state is
to kill the i8042 AUX port. In this case I can't think of a different
way for this to work on the Z570 and other models that have a touchpad
LED. So it seems to me we should keep the current logic but make sure
it only runs on machines that actually need it. I'm now wondering if
this actually really needs to be called on resume, tho. Because if we
remove only that call, it will never be called on machines where there
is no event 5 notification (well, except on startup, but that is not a
problem in practice since there will probably be no userspace handler
active yet to notify anything to the user).

>>> So if on event 10 the touchpad state may be changed, we need to call
>>> ideadpad_sync_touchpad_state, otherwise I don't see why it is
>>> necessary.
>>>
>>
>> The commit message on the original commit on this thread says "Let's
>> send touchpad key codes so software can disable touchpad.", but as we
>> just discussed userspace is not supposed to disable the touchpad on
>> KEY_TOUCHPAD_DISABLE, so I guess it falls under the same category as
>> the IdeaPad Z570: the touchpad is connected via i8042 and it has a
>> variable in ACPI to store the touchpad LED state.
>
> Yes, sure, those key codes sent from ideapad_sync_touchpad_state will
> not be considered in userspace as a command to disable touchpad in
> software.
>

Christian, can you detail a bit more what do you see on the Yoga 700
opening the lid all the way into tablet mode, and when pressing the
disable/enable hotkey? On the Yoga 900 I see an unhandled ACPI
notification when opening the lid all the way (and when flipping it
back to normal "laptop" position), but with value 1 instead of 10. But
I also see the same scancode sent by the touchpad disable/enable
hotkey, so at least in my case nothing specific about the touchpad
should be sent to userspace. Is there any generic "tablet mode" event
(I see something in the surface driver, but it seems specific for that
laptop)? I'm also interested in having this working smoothly in as
much convertible models as possible.

Regards,

--
João Paulo Rechi Vita
http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Maxim Mikityanskiy May 26, 2016, 7:44 p.m. UTC | #7

2016-05-25 19:03 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
> On 25 May 2016 at 02:26, Maxim Mikityanskiy <[e-mail]> wrote:
>> 2016-05-24 23:32 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>> On 19 May 2016 at 05:17, Maxim Mikityanskiy <[e-mail]> wrote:
>>>> 2016-05-16 19:04 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>>
>>>>> Adding Maxim Mikityanskiy and Hans de Goede to CC.
>>>>>
>>>>> On 5 May 2016 at 19:42, Darren Hart <[e-mail]> wrote:
>>>>> > On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
>>>>> >> From: Christian Hesse <[e-mail]>
>>>>> >>
>>>>> >> When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
>>>>> >> send touchpad key codes so software can disable touchpad.
>>>>> >>
>>>>> >> Signed-off-by: Christian Hesse <[e-mail]>
>>>>> >> Signed-off-by: Michael Gisbers <[e-mail]>
>>>>> >
>>>>> > Queued, thanks.
>>>>> >
>>>>> >> ---
>>>>> >>  drivers/platform/x86/ideapad-laptop.c | 1 +
>>>>> >>  1 file changed, 1 insertion(+)
>>>>> >>
>>>>> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>>>>> >> index be3bc2f..1d49db1 100644
>>>>> >> --- a/drivers/platform/x86/ideapad-laptop.c
>>>>> >> +++ b/drivers/platform/x86/ideapad-laptop.c
>>>>> >> @@ -809,6 +809,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>>>>> >>                       case 6:
>>>>> >>                               ideapad_input_report(priv, vpc_bit);
>>>>> >>                               break;
>>>>> >> +                     case 10:
>>>>> >>                       case 5:
>>>>> >>                               ideapad_sync_touchpad_state(priv);
>>>>>
>>>>> I'm not sure this is the right action here: ideapad_sync_touchpad()
>>>>> sends a sequence of KEY_TOUCHPAD_DISABLE and KEY_TOUCHPAD_ENABLED,
>>>>> which are interpreted by userspace as "the touchpad has been
>>>>> {dis,en}abled by the hardware". The way userspace is expected to react
>>>>> to it is by showing a notification of the fact to the user, but not
>>>>> disabling the touchpad. This behavior can be confirmed in the original
>>>>> commit message that introduced this change (I couldn't find any actual
>>>>> documentation on this): "Input: add keycodes for touchpad on/off keys"
>>>>> (0417596f66). I was actually going to propose a patch similar to Hans'
>>>>> "ideapad-laptop: Disable touchpad interface on Yoga models"
>>>>> (f79a901331) for the Yoga 900, to avoid an "touchpad ON" OSD
>>>>> notification being shown to the user when returning for suspend.
>>>>>
>>>>> Also, I've been investigating touchpad-related problems in the Lenovo
>>>>> Yoga 900 recently, and trying to understand this code. IIUC
>>>>> ideadpad_sync_touchpad_state() was created as part of "ideapad: add
>>>>> Lenovo IdeaPad Z570 support (part 2)" (07a4a4fc8) to allow userspace
>>>>> to syncronize a touchpad-state-indicator LED with the actual touchpad
>>>>> state, although it seems to me it only works if the touchpad is always
>>>>> enabled by the hardware on resume (cc'ing the original patch author
>>>>> here for maybe a confirmation on how this is expected to work).
>>>>
>>>> This is a short explanation of this function.
>>>>
>>>> We call ideapad_sync_touchpad_state when a touchpad state changed
>>>> event arrives (touchpad on/off button pressed) or we need to read  out
>>>> the initial touchpad state. It is also called on resume because
>>>> possibly the touchpad state may be changed after resume, but I don't
>>>> know which devices behave like that (and unfortunately I'm unable to
>>>> test it on my Z570 because now it's dead).
>>>>
>>>
>>> So if I'm following this correctly, on some ideapads (Z570 and maybe
>>> others) there is a ACPI notification when the touchpad on/off hotkey
>>> is pressed, is that correct?
>>
>> Yes, exactly.
>>
>>> This is not what I see on the Yoga 900 or
>>> the Yoga 3, but a special scancode (0xbf on the Yoga 900, 0xbe on the
>>> Yoga 3) come through the AT keyboard device. So here I'm using hwdb to
>>> map these to KEY_TOUCHPAD_TOGGLE (pending upstream submission to
>>> udev).
>>
>> If KEY_TOUCHPAD_TOGGLE is sent to the userspace on the Yoga, no
>> KEY_TOUCHPAD_ON or KEY_TOUCHPAD_OFF should be ever sent. Only one of
>> these mechanisms should be used for userspace to handle this
>> correctly.
>>
>
> Agreed. That's why I was planning to propose a patch very similar to
> Hans' (f79a901331 "ideapad-laptop: Disable touchpad interface on Yoga
> models"), but it was later reverted by 3b264d279e. I wonder if there
> is a more specific way to tell one model from the other.
>
>>>> ideadpad_sync_touchpad_state serves several purposes:
>>>>
>>>> - It actually reads out current touchpad state. It is necessary when
>>>> we are reading the initial state, and also this read is necessary on
>>>> touchpad state changed events, because on some laptops (e.g. Z570)
>>>> touchpad LED only changes its state after this read.
>>>>
>>>
>>> On the Yogas I've been testing this the state being read on
>>> ideapad_sync_touchpad_state() never changes, so even if I press the
>>> touchpad disable/enable hotkey before suspending, I always get
>>> KEY_TOUCHPAD_ON.
>>
>> So if VPCCMD_R_TOUCHPAD always returns the same value and not the
>> actual touchpad state, we should not send KEY_TOUCHPAD_ON on these
>> devices as it conflicts with KEY_TOUCHPAD_TOGGLE and doesn't reflect
>> the real touchpad state. Is there any way to determine in runtime if
>> VPCCMD_R_TOUCHPAD works correctly on the device? BTW, could you send
>> me a DSDT dump from Yoga where it always shows touchpad on?
>>
>
> I imagine it is expected to return the same value because, from the
> hardware perspective, the touchpad is always enabled. I'm not sure of
> a good way to verify this, maybe disabling/re-enabling the i8042 AUX
> and checking its value when the module first loads (not sure if this
> is a good idea).

Not sure how disabling/enabling AUX port will help us determine if EC
is capable of disabling touchpad and reporting disabled state.

> The Yoga 900 DSDT can be found here:
> https://gist.githubusercontent.com/jprvita/5737de3cbb670e80973b7d4e51c38ab6/raw/189220cb746a719026bde2fa3e75ef5cb24e1931/yoga-900-dsdt.dsl

Okay, I took a look at this DSDT, and the VPC interface used by
ideapad-laptop driver is completely opaque here. On my Z570 there was
XCMD method that was called from VPCW and handled all the commands. I
was hoping to find this method in Yoga DSDT to examine its code and
check if VPCCMD_R_TOUCHPAD actually returns any non-constant value or
just reports a constant, but on this device VPCW just writes the
command to the register, and maybe EC handles it in its firmware. So
there is no XCMD method and the code that handles VPC commands is
unavailable.

My idea was that it could be possible that on the Yoga
VPCCMD_R_TOUCHPAD returns always the same value, but the value is
neither 0 nor 1, but some other non-zero constant indicating that
touchpad state reporting does not work. Unfortunately DSDT did not
help me to check this hypothesis, but we still can check it by looking
at the value of the variable value in ideapad_sync_touchpad_state. I
think that most likely we will just get 1 there, but if we get another
value it will be the nice way to distinguish devices where we need
touchpad control from others. (Actually we need to check also if it is
actually 1 on the devices with working touchpad control, because I
don't remember for sure and have no device to test it.)

>>>> - Then it disables/enables i8042 AUX port to disable/enable touchpad.
>>>> It is also necessary, because some laptops (Z570) do not disable
>>>> touchpad in hardware although they are storing touchpad state.
>>>>
>>>
>>> Ok, but this will only work if the touchpad is connected through that
>>> bus. On the Yoga 900 and Yoga 3 the are connected via I2C, and I guess
>>> this might be true for other machines as well.
>>
>> If I remember correctly, some ideapads actually disable the touchpad
>> in hardware and some (e.g. Z570) only toggle the LED. For those ones
>> that have PS/2 touchpad and don't disable it in hardware,
>> I8042_CMD_AUX_ENABLE/DISABLE works. I'm not sure if any ideapad exists
>> that has e.g. I2C touchpad and doesn't disable it in hardware, but
>> only toggles the LED. If such laptop is a case, i8042 method will not
>> work. Is there any better and universal way to disable touchpad from
>> the driver?
>>
>
> I don't know if such laptop model exists either, but we can probably
> leave this case for when (if) it shows up.
>
>>>> - Finally it sends userspace event with new touchpad state. It does
>>>> not send a sequence of KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON as you
>>>> mentioned, but only one of these key codes.
>>>>
>>>
>>> Right, I got confused when I wrote this email by the comments in the
>>> function implementation, but when I checked with evtest I did see only
>>> one event being sent, indeed.
>>
>> OK, when I read those comments now, they look really confusing,
>> espessially "We send KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON" part. Sorry
>> for this confusion, it should mean that we send one of those codes
>> depending on the touchpad state, not that we send both of them.
>>
>>>> The second point is kinda hack. In Linux there are two ways of
>>>> reporting touchpad state events to the userspace:
>>>>
>>>> - KEY_TOUCHPAD_TOGGLE for those laptops that do not disable touchpad
>>>> in hardware and thus don't store touchpad state. The userspace should
>>>> listen for those key presses and disable/enable touchpad
>>>> programmatically (e.g. TouchpadOff in synaptics X11 driver).
>>>>
>>>
>>> Yes, when the notification for the disable/enable touchpad hotkey goes
>>> through ACPI.
>>
>> Sorry, why is ACPI important in this case?
>>
>
> I mean, if the hardware sends a notification through ACPI that needs
> to be handled by the kernel, instead of a keypress event on the
> keyboard device with a special scancode. But anyway, my comment here
> didn't really add anything.
>
>>>> - KEY_TOUCHPAD_ON and KEY_TOUCHPAD_OFF for those laptops that fully
>>>> manage touchpad state in hardware and just report its state to the
>>>> userspace so that notification can be shown. Userspace should not
>>>> disable touchpad programmatically on these events, because it is
>>>> managed by hardware and may easily get out of sync if using multiple
>>>> user sessions.
>>>>
>>>
>>> Yes, and that is what I tried to explain on my previous comment. In
>>> this case userspace will usually want to notify the user that the
>>> touchpad state has been changed.
>>>
>>>> Ideapad Z570 matches none of these options. It looks more like the
>>>> second one, it stores the touchpad state in hardware, controls the
>>>> LED, but it lacks the ability to disable the touchpad in hardware. So
>>>> we need to keep track of the hardware state and disable i8042 AUX port
>>>> from the driver when necessary.
>>>>
>>>
>>> Isn't the case that what it actually stores under VPCCMD_R_TOUCHPAD is
>>> only the touchpad LED state?
>>
>> Yes, Z570 only stores the LED state.
>>
>>> If so, wouldn't it work if when ACPI
>>> notifies the kernel of event 5 (the touchpad disable/enable key has
>>> been pressed) we simply sent KEY_TOUCHPAD_TOGGLE to userspace? I'm not
>>> entirely sure what would be best way to keep the LED in sync with the
>>> touchpad state in that case, the only way I can think of if to expose
>>> the LED to userspace so it can update it accordingly.
>>
>> No, sadly it wouldn't work correctly, because on Z570 we can't change
>> the LED state programmatically. There is VPCCMD_W_TOUCHPAD, but as far
>> as I remember it doesn't work at least on Z570.
>>
>
> So what you are saying is that the only way to update the LED state is
> to kill the i8042 AUX port.

Not exactly. We can't update the LED state, so the firmware updates
it, and we just need to obey it and switch touchpad on/off
accordingly.

> In this case I can't think of a different
> way for this to work on the Z570 and other models that have a touchpad
> LED. So it seems to me we should keep the current logic but make sure
> it only runs on machines that actually need it. I'm now wondering if
> this actually really needs to be called on resume, tho.

Interesting question, it's really worth checking if it is necessary on
resume, but unfortunately I can't do this test because my Z570 is
dead, it does not turn on.

> Because if we
> remove only that call, it will never be called on machines where there
> is no event 5 notification (well, except on startup, but that is not a
> problem in practice since there will probably be no userspace handler
> active yet to notify anything to the user).
>
>>>> So if on event 10 the touchpad state may be changed, we need to call
>>>> ideadpad_sync_touchpad_state, otherwise I don't see why it is
>>>> necessary.
>>>>
>>>
>>> The commit message on the original commit on this thread says "Let's
>>> send touchpad key codes so software can disable touchpad.", but as we
>>> just discussed userspace is not supposed to disable the touchpad on
>>> KEY_TOUCHPAD_DISABLE, so I guess it falls under the same category as
>>> the IdeaPad Z570: the touchpad is connected via i8042 and it has a
>>> variable in ACPI to store the touchpad LED state.
>>
>> Yes, sure, those key codes sent from ideapad_sync_touchpad_state will
>> not be considered in userspace as a command to disable touchpad in
>> software.
>>
>
> Christian, can you detail a bit more what do you see on the Yoga 700
> opening the lid all the way into tablet mode, and when pressing the
> disable/enable hotkey? On the Yoga 900 I see an unhandled ACPI
> notification when opening the lid all the way (and when flipping it
> back to normal "laptop" position), but with value 1 instead of 10. But
> I also see the same scancode sent by the touchpad disable/enable
> hotkey, so at least in my case nothing specific about the touchpad
> should be sent to userspace. Is there any generic "tablet mode" event
> (I see something in the surface driver, but it seems specific for that
> laptop)? I'm also interested in having this working smoothly in as
> much convertible models as possible.
>
> Regards,
>
> --
> João Paulo Rechi Vita
> http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Darren Hart May 27, 2016, 6:51 p.m. UTC | #8

On Tue, May 24, 2016 at 04:32:48PM -0400, João Paulo Rechi Vita wrote:
> On 19 May 2016 at 05:17, Maxim Mikityanskiy <[e-mail]> wrote:
> > 2016-05-16 19:04 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
> >>
> >> Adding Maxim Mikityanskiy and Hans de Goede to CC.
> >>
> >> On 5 May 2016 at 19:42, Darren Hart <[e-mail]> wrote:
> >> > On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
> >> >> From: Christian Hesse <[e-mail]>
> >> >>

...

> It seems to me that at least we should have some quirks in place so
> this only gets executed on hardware where it will actually work, to
> avoid notifying userspace of touchpad events when there were actually
> none. Darren, what do you think is the best way to move forward here?
> If we can reach an agreement I can provide some patches for this.
> 

Based on this discussion, I have dropped the patch from the 4.7 queue.

I do not have a good plan-of-attach, or preferred-approach to offer
unfortunately. You and Maxim have already started the additional information
gathering process, which would be my only guidance at this point.

We need to characterize the hardware and the interfaces as exposed through ACPI.
Quirks would be OK if we can't just match on different HIDs (it appears the same
HID is being used for radically different hardware unfortunately).

João Paulo Rechi Vita May 31, 2016, 10:43 p.m. UTC | #9

On 26 May 2016 at 15:44, Maxim Mikityanskiy <[e-mail]> wrote:
> 2016-05-25 19:03 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>> On 25 May 2016 at 02:26, Maxim Mikityanskiy <[e-mail]> wrote:
>>> 2016-05-24 23:32 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>> On 19 May 2016 at 05:17, Maxim Mikityanskiy <[e-mail]> wrote:
>>>>> 2016-05-16 19:04 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>>>
>>>>>> Adding Maxim Mikityanskiy and Hans de Goede to CC.
>>>>>>
>>>>>> On 5 May 2016 at 19:42, Darren Hart <[e-mail]> wrote:
>>>>>> > On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
>>>>>> >> From: Christian Hesse <[e-mail]>
>>>>>> >>
>>>>>> >> When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
>>>>>> >> send touchpad key codes so software can disable touchpad.
>>>>>> >>
>>>>>> >> Signed-off-by: Christian Hesse <[e-mail]>
>>>>>> >> Signed-off-by: Michael Gisbers <[e-mail]>
>>>>>> >
>>>>>> > Queued, thanks.
>>>>>> >
>>>>>> >> ---
>>>>>> >>  drivers/platform/x86/ideapad-laptop.c | 1 +
>>>>>> >>  1 file changed, 1 insertion(+)
>>>>>> >>
>>>>>> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>>>>>> >> index be3bc2f..1d49db1 100644
>>>>>> >> --- a/drivers/platform/x86/ideapad-laptop.c
>>>>>> >> +++ b/drivers/platform/x86/ideapad-laptop.c
>>>>>> >> @@ -809,6 +809,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>>>>>> >>                       case 6:
>>>>>> >>                               ideapad_input_report(priv, vpc_bit);
>>>>>> >>                               break;
>>>>>> >> +                     case 10:
>>>>>> >>                       case 5:
>>>>>> >>                               ideapad_sync_touchpad_state(priv);
>>>>>>
>>>>>> I'm not sure this is the right action here: ideapad_sync_touchpad()
>>>>>> sends a sequence of KEY_TOUCHPAD_DISABLE and KEY_TOUCHPAD_ENABLED,
>>>>>> which are interpreted by userspace as "the touchpad has been
>>>>>> {dis,en}abled by the hardware". The way userspace is expected to react
>>>>>> to it is by showing a notification of the fact to the user, but not
>>>>>> disabling the touchpad. This behavior can be confirmed in the original
>>>>>> commit message that introduced this change (I couldn't find any actual
>>>>>> documentation on this): "Input: add keycodes for touchpad on/off keys"
>>>>>> (0417596f66). I was actually going to propose a patch similar to Hans'
>>>>>> "ideapad-laptop: Disable touchpad interface on Yoga models"
>>>>>> (f79a901331) for the Yoga 900, to avoid an "touchpad ON" OSD
>>>>>> notification being shown to the user when returning for suspend.
>>>>>>
>>>>>> Also, I've been investigating touchpad-related problems in the Lenovo
>>>>>> Yoga 900 recently, and trying to understand this code. IIUC
>>>>>> ideadpad_sync_touchpad_state() was created as part of "ideapad: add
>>>>>> Lenovo IdeaPad Z570 support (part 2)" (07a4a4fc8) to allow userspace
>>>>>> to syncronize a touchpad-state-indicator LED with the actual touchpad
>>>>>> state, although it seems to me it only works if the touchpad is always
>>>>>> enabled by the hardware on resume (cc'ing the original patch author
>>>>>> here for maybe a confirmation on how this is expected to work).
>>>>>
>>>>> This is a short explanation of this function.
>>>>>
>>>>> We call ideapad_sync_touchpad_state when a touchpad state changed
>>>>> event arrives (touchpad on/off button pressed) or we need to read  out
>>>>> the initial touchpad state. It is also called on resume because
>>>>> possibly the touchpad state may be changed after resume, but I don't
>>>>> know which devices behave like that (and unfortunately I'm unable to
>>>>> test it on my Z570 because now it's dead).
>>>>>
>>>>
>>>> So if I'm following this correctly, on some ideapads (Z570 and maybe
>>>> others) there is a ACPI notification when the touchpad on/off hotkey
>>>> is pressed, is that correct?
>>>
>>> Yes, exactly.
>>>
>>>> This is not what I see on the Yoga 900 or
>>>> the Yoga 3, but a special scancode (0xbf on the Yoga 900, 0xbe on the
>>>> Yoga 3) come through the AT keyboard device. So here I'm using hwdb to
>>>> map these to KEY_TOUCHPAD_TOGGLE (pending upstream submission to
>>>> udev).
>>>
>>> If KEY_TOUCHPAD_TOGGLE is sent to the userspace on the Yoga, no
>>> KEY_TOUCHPAD_ON or KEY_TOUCHPAD_OFF should be ever sent. Only one of
>>> these mechanisms should be used for userspace to handle this
>>> correctly.
>>>
>>
>> Agreed. That's why I was planning to propose a patch very similar to
>> Hans' (f79a901331 "ideapad-laptop: Disable touchpad interface on Yoga
>> models"), but it was later reverted by 3b264d279e. I wonder if there
>> is a more specific way to tell one model from the other.
>>
>>>>> ideadpad_sync_touchpad_state serves several purposes:
>>>>>
>>>>> - It actually reads out current touchpad state. It is necessary when
>>>>> we are reading the initial state, and also this read is necessary on
>>>>> touchpad state changed events, because on some laptops (e.g. Z570)
>>>>> touchpad LED only changes its state after this read.
>>>>>
>>>>
>>>> On the Yogas I've been testing this the state being read on
>>>> ideapad_sync_touchpad_state() never changes, so even if I press the
>>>> touchpad disable/enable hotkey before suspending, I always get
>>>> KEY_TOUCHPAD_ON.
>>>
>>> So if VPCCMD_R_TOUCHPAD always returns the same value and not the
>>> actual touchpad state, we should not send KEY_TOUCHPAD_ON on these
>>> devices as it conflicts with KEY_TOUCHPAD_TOGGLE and doesn't reflect
>>> the real touchpad state. Is there any way to determine in runtime if
>>> VPCCMD_R_TOUCHPAD works correctly on the device? BTW, could you send
>>> me a DSDT dump from Yoga where it always shows touchpad on?
>>>
>>
>> I imagine it is expected to return the same value because, from the
>> hardware perspective, the touchpad is always enabled. I'm not sure of
>> a good way to verify this, maybe disabling/re-enabling the i8042 AUX
>> and checking its value when the module first loads (not sure if this
>> is a good idea).
>
> Not sure how disabling/enabling AUX port will help us determine if EC
> is capable of disabling touchpad and reporting disabled state.
>

If you disable the AUX port and read the touchpad state, on the Yoga
you will still read the "enabled" value (1).

>> The Yoga 900 DSDT can be found here:
>> https://gist.githubusercontent.com/jprvita/5737de3cbb670e80973b7d4e51c38ab6/raw/189220cb746a719026bde2fa3e75ef5cb24e1931/yoga-900-dsdt.dsl
>
> Okay, I took a look at this DSDT, and the VPC interface used by
> ideapad-laptop driver is completely opaque here. On my Z570 there was
> XCMD method that was called from VPCW and handled all the commands. I
> was hoping to find this method in Yoga DSDT to examine its code and
> check if VPCCMD_R_TOUCHPAD actually returns any non-constant value or
> just reports a constant, but on this device VPCW just writes the
> command to the register, and maybe EC handles it in its firmware. So
> there is no XCMD method and the code that handles VPC commands is
> unavailable.
>
> My idea was that it could be possible that on the Yoga
> VPCCMD_R_TOUCHPAD returns always the same value, but the value is
> neither 0 nor 1, but some other non-zero constant indicating that
> touchpad state reporting does not work. Unfortunately DSDT did not
> help me to check this hypothesis, but we still can check it by looking
> at the value of the variable value in ideapad_sync_touchpad_state. I
> think that most likely we will just get 1 there, but if we get another
> value it will be the nice way to distinguish devices where we need
> touchpad control from others. (Actually we need to check also if it is
> actually 1 on the devices with working touchpad control, because I
> don't remember for sure and have no device to test it.)
>

It is always 1 on the Yoga 900.

>>>>> - Then it disables/enables i8042 AUX port to disable/enable touchpad.
>>>>> It is also necessary, because some laptops (Z570) do not disable
>>>>> touchpad in hardware although they are storing touchpad state.
>>>>>
>>>>
>>>> Ok, but this will only work if the touchpad is connected through that
>>>> bus. On the Yoga 900 and Yoga 3 the are connected via I2C, and I guess
>>>> this might be true for other machines as well.
>>>
>>> If I remember correctly, some ideapads actually disable the touchpad
>>> in hardware and some (e.g. Z570) only toggle the LED. For those ones
>>> that have PS/2 touchpad and don't disable it in hardware,
>>> I8042_CMD_AUX_ENABLE/DISABLE works. I'm not sure if any ideapad exists
>>> that has e.g. I2C touchpad and doesn't disable it in hardware, but
>>> only toggles the LED. If such laptop is a case, i8042 method will not
>>> work. Is there any better and universal way to disable touchpad from
>>> the driver?
>>>
>>
>> I don't know if such laptop model exists either, but we can probably
>> leave this case for when (if) it shows up.
>>
>>>>> - Finally it sends userspace event with new touchpad state. It does
>>>>> not send a sequence of KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON as you
>>>>> mentioned, but only one of these key codes.
>>>>>
>>>>
>>>> Right, I got confused when I wrote this email by the comments in the
>>>> function implementation, but when I checked with evtest I did see only
>>>> one event being sent, indeed.
>>>
>>> OK, when I read those comments now, they look really confusing,
>>> espessially "We send KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON" part. Sorry
>>> for this confusion, it should mean that we send one of those codes
>>> depending on the touchpad state, not that we send both of them.
>>>
>>>>> The second point is kinda hack. In Linux there are two ways of
>>>>> reporting touchpad state events to the userspace:
>>>>>
>>>>> - KEY_TOUCHPAD_TOGGLE for those laptops that do not disable touchpad
>>>>> in hardware and thus don't store touchpad state. The userspace should
>>>>> listen for those key presses and disable/enable touchpad
>>>>> programmatically (e.g. TouchpadOff in synaptics X11 driver).
>>>>>
>>>>
>>>> Yes, when the notification for the disable/enable touchpad hotkey goes
>>>> through ACPI.
>>>
>>> Sorry, why is ACPI important in this case?
>>>
>>
>> I mean, if the hardware sends a notification through ACPI that needs
>> to be handled by the kernel, instead of a keypress event on the
>> keyboard device with a special scancode. But anyway, my comment here
>> didn't really add anything.
>>
>>>>> - KEY_TOUCHPAD_ON and KEY_TOUCHPAD_OFF for those laptops that fully
>>>>> manage touchpad state in hardware and just report its state to the
>>>>> userspace so that notification can be shown. Userspace should not
>>>>> disable touchpad programmatically on these events, because it is
>>>>> managed by hardware and may easily get out of sync if using multiple
>>>>> user sessions.
>>>>>
>>>>
>>>> Yes, and that is what I tried to explain on my previous comment. In
>>>> this case userspace will usually want to notify the user that the
>>>> touchpad state has been changed.
>>>>
>>>>> Ideapad Z570 matches none of these options. It looks more like the
>>>>> second one, it stores the touchpad state in hardware, controls the
>>>>> LED, but it lacks the ability to disable the touchpad in hardware. So
>>>>> we need to keep track of the hardware state and disable i8042 AUX port
>>>>> from the driver when necessary.
>>>>>
>>>>
>>>> Isn't the case that what it actually stores under VPCCMD_R_TOUCHPAD is
>>>> only the touchpad LED state?
>>>
>>> Yes, Z570 only stores the LED state.
>>>
>>>> If so, wouldn't it work if when ACPI
>>>> notifies the kernel of event 5 (the touchpad disable/enable key has
>>>> been pressed) we simply sent KEY_TOUCHPAD_TOGGLE to userspace? I'm not
>>>> entirely sure what would be best way to keep the LED in sync with the
>>>> touchpad state in that case, the only way I can think of if to expose
>>>> the LED to userspace so it can update it accordingly.
>>>
>>> No, sadly it wouldn't work correctly, because on Z570 we can't change
>>> the LED state programmatically. There is VPCCMD_W_TOUCHPAD, but as far
>>> as I remember it doesn't work at least on Z570.
>>>
>>
>> So what you are saying is that the only way to update the LED state is
>> to kill the i8042 AUX port.
>
> Not exactly. We can't update the LED state, so the firmware updates
> it, and we just need to obey it and switch touchpad on/off
> accordingly.
>
>> In this case I can't think of a different
>> way for this to work on the Z570 and other models that have a touchpad
>> LED. So it seems to me we should keep the current logic but make sure
>> it only runs on machines that actually need it. I'm now wondering if
>> this actually really needs to be called on resume, tho.
>
> Interesting question, it's really worth checking if it is necessary on
> resume, but unfortunately I can't do this test because my Z570 is
> dead, it does not turn on.
>

Maybe we could remove that call and see if someone complains? Not a
very nice policy, tho.

--
João Paulo Rechi Vita
http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Maxim Mikityanskiy June 1, 2016, 4:37 a.m. UTC | #10

2016-06-01 1:43 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
> On 26 May 2016 at 15:44, Maxim Mikityanskiy <[e-mail]> wrote:
>> 2016-05-25 19:03 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>> On 25 May 2016 at 02:26, Maxim Mikityanskiy <[e-mail]> wrote:
>>>> 2016-05-24 23:32 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>> On 19 May 2016 at 05:17, Maxim Mikityanskiy <[e-mail]> wrote:
>>>>>> 2016-05-16 19:04 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>>>>
>>>>>>> Adding Maxim Mikityanskiy and Hans de Goede to CC.
>>>>>>>
>>>>>>> On 5 May 2016 at 19:42, Darren Hart <[e-mail]> wrote:
>>>>>>> > On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
>>>>>>> >> From: Christian Hesse <[e-mail]>
>>>>>>> >>
>>>>>>> >> When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
>>>>>>> >> send touchpad key codes so software can disable touchpad.
>>>>>>> >>
>>>>>>> >> Signed-off-by: Christian Hesse <[e-mail]>
>>>>>>> >> Signed-off-by: Michael Gisbers <[e-mail]>
>>>>>>> >
>>>>>>> > Queued, thanks.
>>>>>>> >
>>>>>>> >> ---
>>>>>>> >>  drivers/platform/x86/ideapad-laptop.c | 1 +
>>>>>>> >>  1 file changed, 1 insertion(+)
>>>>>>> >>
>>>>>>> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>>>>>>> >> index be3bc2f..1d49db1 100644
>>>>>>> >> --- a/drivers/platform/x86/ideapad-laptop.c
>>>>>>> >> +++ b/drivers/platform/x86/ideapad-laptop.c
>>>>>>> >> @@ -809,6 +809,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>>>>>>> >>                       case 6:
>>>>>>> >>                               ideapad_input_report(priv, vpc_bit);
>>>>>>> >>                               break;
>>>>>>> >> +                     case 10:
>>>>>>> >>                       case 5:
>>>>>>> >>                               ideapad_sync_touchpad_state(priv);
>>>>>>>
>>>>>>> I'm not sure this is the right action here: ideapad_sync_touchpad()
>>>>>>> sends a sequence of KEY_TOUCHPAD_DISABLE and KEY_TOUCHPAD_ENABLED,
>>>>>>> which are interpreted by userspace as "the touchpad has been
>>>>>>> {dis,en}abled by the hardware". The way userspace is expected to react
>>>>>>> to it is by showing a notification of the fact to the user, but not
>>>>>>> disabling the touchpad. This behavior can be confirmed in the original
>>>>>>> commit message that introduced this change (I couldn't find any actual
>>>>>>> documentation on this): "Input: add keycodes for touchpad on/off keys"
>>>>>>> (0417596f66). I was actually going to propose a patch similar to Hans'
>>>>>>> "ideapad-laptop: Disable touchpad interface on Yoga models"
>>>>>>> (f79a901331) for the Yoga 900, to avoid an "touchpad ON" OSD
>>>>>>> notification being shown to the user when returning for suspend.
>>>>>>>
>>>>>>> Also, I've been investigating touchpad-related problems in the Lenovo
>>>>>>> Yoga 900 recently, and trying to understand this code. IIUC
>>>>>>> ideadpad_sync_touchpad_state() was created as part of "ideapad: add
>>>>>>> Lenovo IdeaPad Z570 support (part 2)" (07a4a4fc8) to allow userspace
>>>>>>> to syncronize a touchpad-state-indicator LED with the actual touchpad
>>>>>>> state, although it seems to me it only works if the touchpad is always
>>>>>>> enabled by the hardware on resume (cc'ing the original patch author
>>>>>>> here for maybe a confirmation on how this is expected to work).
>>>>>>
>>>>>> This is a short explanation of this function.
>>>>>>
>>>>>> We call ideapad_sync_touchpad_state when a touchpad state changed
>>>>>> event arrives (touchpad on/off button pressed) or we need to read  out
>>>>>> the initial touchpad state. It is also called on resume because
>>>>>> possibly the touchpad state may be changed after resume, but I don't
>>>>>> know which devices behave like that (and unfortunately I'm unable to
>>>>>> test it on my Z570 because now it's dead).
>>>>>>
>>>>>
>>>>> So if I'm following this correctly, on some ideapads (Z570 and maybe
>>>>> others) there is a ACPI notification when the touchpad on/off hotkey
>>>>> is pressed, is that correct?
>>>>
>>>> Yes, exactly.
>>>>
>>>>> This is not what I see on the Yoga 900 or
>>>>> the Yoga 3, but a special scancode (0xbf on the Yoga 900, 0xbe on the
>>>>> Yoga 3) come through the AT keyboard device. So here I'm using hwdb to
>>>>> map these to KEY_TOUCHPAD_TOGGLE (pending upstream submission to
>>>>> udev).
>>>>
>>>> If KEY_TOUCHPAD_TOGGLE is sent to the userspace on the Yoga, no
>>>> KEY_TOUCHPAD_ON or KEY_TOUCHPAD_OFF should be ever sent. Only one of
>>>> these mechanisms should be used for userspace to handle this
>>>> correctly.
>>>>
>>>
>>> Agreed. That's why I was planning to propose a patch very similar to
>>> Hans' (f79a901331 "ideapad-laptop: Disable touchpad interface on Yoga
>>> models"), but it was later reverted by 3b264d279e. I wonder if there
>>> is a more specific way to tell one model from the other.
>>>
>>>>>> ideadpad_sync_touchpad_state serves several purposes:
>>>>>>
>>>>>> - It actually reads out current touchpad state. It is necessary when
>>>>>> we are reading the initial state, and also this read is necessary on
>>>>>> touchpad state changed events, because on some laptops (e.g. Z570)
>>>>>> touchpad LED only changes its state after this read.
>>>>>>
>>>>>
>>>>> On the Yogas I've been testing this the state being read on
>>>>> ideapad_sync_touchpad_state() never changes, so even if I press the
>>>>> touchpad disable/enable hotkey before suspending, I always get
>>>>> KEY_TOUCHPAD_ON.
>>>>
>>>> So if VPCCMD_R_TOUCHPAD always returns the same value and not the
>>>> actual touchpad state, we should not send KEY_TOUCHPAD_ON on these
>>>> devices as it conflicts with KEY_TOUCHPAD_TOGGLE and doesn't reflect
>>>> the real touchpad state. Is there any way to determine in runtime if
>>>> VPCCMD_R_TOUCHPAD works correctly on the device? BTW, could you send
>>>> me a DSDT dump from Yoga where it always shows touchpad on?
>>>>
>>>
>>> I imagine it is expected to return the same value because, from the
>>> hardware perspective, the touchpad is always enabled. I'm not sure of
>>> a good way to verify this, maybe disabling/re-enabling the i8042 AUX
>>> and checking its value when the module first loads (not sure if this
>>> is a good idea).
>>
>> Not sure how disabling/enabling AUX port will help us determine if EC
>> is capable of disabling touchpad and reporting disabled state.
>>
>
> If you disable the AUX port and read the touchpad state, on the Yoga
> you will still read the "enabled" value (1).
>
>>> The Yoga 900 DSDT can be found here:
>>> https://gist.githubusercontent.com/jprvita/5737de3cbb670e80973b7d4e51c38ab6/raw/189220cb746a719026bde2fa3e75ef5cb24e1931/yoga-900-dsdt.dsl
>>
>> Okay, I took a look at this DSDT, and the VPC interface used by
>> ideapad-laptop driver is completely opaque here. On my Z570 there was
>> XCMD method that was called from VPCW and handled all the commands. I
>> was hoping to find this method in Yoga DSDT to examine its code and
>> check if VPCCMD_R_TOUCHPAD actually returns any non-constant value or
>> just reports a constant, but on this device VPCW just writes the
>> command to the register, and maybe EC handles it in its firmware. So
>> there is no XCMD method and the code that handles VPC commands is
>> unavailable.
>>
>> My idea was that it could be possible that on the Yoga
>> VPCCMD_R_TOUCHPAD returns always the same value, but the value is
>> neither 0 nor 1, but some other non-zero constant indicating that
>> touchpad state reporting does not work. Unfortunately DSDT did not
>> help me to check this hypothesis, but we still can check it by looking
>> at the value of the variable value in ideapad_sync_touchpad_state. I
>> think that most likely we will just get 1 there, but if we get another
>> value it will be the nice way to distinguish devices where we need
>> touchpad control from others. (Actually we need to check also if it is
>> actually 1 on the devices with working touchpad control, because I
>> don't remember for sure and have no device to test it.)
>>
>
> It is always 1 on the Yoga 900.
>
>>>>>> - Then it disables/enables i8042 AUX port to disable/enable touchpad.
>>>>>> It is also necessary, because some laptops (Z570) do not disable
>>>>>> touchpad in hardware although they are storing touchpad state.
>>>>>>
>>>>>
>>>>> Ok, but this will only work if the touchpad is connected through that
>>>>> bus. On the Yoga 900 and Yoga 3 the are connected via I2C, and I guess
>>>>> this might be true for other machines as well.
>>>>
>>>> If I remember correctly, some ideapads actually disable the touchpad
>>>> in hardware and some (e.g. Z570) only toggle the LED. For those ones
>>>> that have PS/2 touchpad and don't disable it in hardware,
>>>> I8042_CMD_AUX_ENABLE/DISABLE works. I'm not sure if any ideapad exists
>>>> that has e.g. I2C touchpad and doesn't disable it in hardware, but
>>>> only toggles the LED. If such laptop is a case, i8042 method will not
>>>> work. Is there any better and universal way to disable touchpad from
>>>> the driver?
>>>>
>>>
>>> I don't know if such laptop model exists either, but we can probably
>>> leave this case for when (if) it shows up.
>>>
>>>>>> - Finally it sends userspace event with new touchpad state. It does
>>>>>> not send a sequence of KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON as you
>>>>>> mentioned, but only one of these key codes.
>>>>>>
>>>>>
>>>>> Right, I got confused when I wrote this email by the comments in the
>>>>> function implementation, but when I checked with evtest I did see only
>>>>> one event being sent, indeed.
>>>>
>>>> OK, when I read those comments now, they look really confusing,
>>>> espessially "We send KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON" part. Sorry
>>>> for this confusion, it should mean that we send one of those codes
>>>> depending on the touchpad state, not that we send both of them.
>>>>
>>>>>> The second point is kinda hack. In Linux there are two ways of
>>>>>> reporting touchpad state events to the userspace:
>>>>>>
>>>>>> - KEY_TOUCHPAD_TOGGLE for those laptops that do not disable touchpad
>>>>>> in hardware and thus don't store touchpad state. The userspace should
>>>>>> listen for those key presses and disable/enable touchpad
>>>>>> programmatically (e.g. TouchpadOff in synaptics X11 driver).
>>>>>>
>>>>>
>>>>> Yes, when the notification for the disable/enable touchpad hotkey goes
>>>>> through ACPI.
>>>>
>>>> Sorry, why is ACPI important in this case?
>>>>
>>>
>>> I mean, if the hardware sends a notification through ACPI that needs
>>> to be handled by the kernel, instead of a keypress event on the
>>> keyboard device with a special scancode. But anyway, my comment here
>>> didn't really add anything.
>>>
>>>>>> - KEY_TOUCHPAD_ON and KEY_TOUCHPAD_OFF for those laptops that fully
>>>>>> manage touchpad state in hardware and just report its state to the
>>>>>> userspace so that notification can be shown. Userspace should not
>>>>>> disable touchpad programmatically on these events, because it is
>>>>>> managed by hardware and may easily get out of sync if using multiple
>>>>>> user sessions.
>>>>>>
>>>>>
>>>>> Yes, and that is what I tried to explain on my previous comment. In
>>>>> this case userspace will usually want to notify the user that the
>>>>> touchpad state has been changed.
>>>>>
>>>>>> Ideapad Z570 matches none of these options. It looks more like the
>>>>>> second one, it stores the touchpad state in hardware, controls the
>>>>>> LED, but it lacks the ability to disable the touchpad in hardware. So
>>>>>> we need to keep track of the hardware state and disable i8042 AUX port
>>>>>> from the driver when necessary.
>>>>>>
>>>>>
>>>>> Isn't the case that what it actually stores under VPCCMD_R_TOUCHPAD is
>>>>> only the touchpad LED state?
>>>>
>>>> Yes, Z570 only stores the LED state.
>>>>
>>>>> If so, wouldn't it work if when ACPI
>>>>> notifies the kernel of event 5 (the touchpad disable/enable key has
>>>>> been pressed) we simply sent KEY_TOUCHPAD_TOGGLE to userspace? I'm not
>>>>> entirely sure what would be best way to keep the LED in sync with the
>>>>> touchpad state in that case, the only way I can think of if to expose
>>>>> the LED to userspace so it can update it accordingly.
>>>>
>>>> No, sadly it wouldn't work correctly, because on Z570 we can't change
>>>> the LED state programmatically. There is VPCCMD_W_TOUCHPAD, but as far
>>>> as I remember it doesn't work at least on Z570.
>>>>
>>>
>>> So what you are saying is that the only way to update the LED state is
>>> to kill the i8042 AUX port.
>>
>> Not exactly. We can't update the LED state, so the firmware updates
>> it, and we just need to obey it and switch touchpad on/off
>> accordingly.
>>
>>> In this case I can't think of a different
>>> way for this to work on the Z570 and other models that have a touchpad
>>> LED. So it seems to me we should keep the current logic but make sure
>>> it only runs on machines that actually need it. I'm now wondering if
>>> this actually really needs to be called on resume, tho.
>>
>> Interesting question, it's really worth checking if it is necessary on
>> resume, but unfortunately I can't do this test because my Z570 is
>> dead, it does not turn on.
>>
>
> Maybe we could remove that call and see if someone complains? Not a
> very nice policy, tho.

I'd prefer testing it in advance on a potentially affected device.
Things may break for users, but only a few of them would file a bug
report or investigate the problem by themselves and post to the mail
list.

> --
> João Paulo Rechi Vita
> http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

João Paulo Rechi Vita June 2, 2016, 2:38 p.m. UTC | #11

On 1 June 2016 at 00:37, Maxim Mikityanskiy <[e-mail]> wrote:
> 2016-06-01 1:43 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>> On 26 May 2016 at 15:44, Maxim Mikityanskiy <[e-mail]> wrote:
>>> 2016-05-25 19:03 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>> On 25 May 2016 at 02:26, Maxim Mikityanskiy <[e-mail]> wrote:
>>>>> 2016-05-24 23:32 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>>> On 19 May 2016 at 05:17, Maxim Mikityanskiy <[e-mail]> wrote:
>>>>>>> 2016-05-16 19:04 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>>>>>
>>>>>>>> Adding Maxim Mikityanskiy and Hans de Goede to CC.
>>>>>>>>
>>>>>>>> On 5 May 2016 at 19:42, Darren Hart <[e-mail]> wrote:
>>>>>>>> > On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
>>>>>>>> >> From: Christian Hesse <[e-mail]>
>>>>>>>> >>
>>>>>>>> >> When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
>>>>>>>> >> send touchpad key codes so software can disable touchpad.
>>>>>>>> >>
>>>>>>>> >> Signed-off-by: Christian Hesse <[e-mail]>
>>>>>>>> >> Signed-off-by: Michael Gisbers <[e-mail]>
>>>>>>>> >
>>>>>>>> > Queued, thanks.
>>>>>>>> >
>>>>>>>> >> ---
>>>>>>>> >>  drivers/platform/x86/ideapad-laptop.c | 1 +
>>>>>>>> >>  1 file changed, 1 insertion(+)
>>>>>>>> >>
>>>>>>>> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>>>>>>>> >> index be3bc2f..1d49db1 100644
>>>>>>>> >> --- a/drivers/platform/x86/ideapad-laptop.c
>>>>>>>> >> +++ b/drivers/platform/x86/ideapad-laptop.c
>>>>>>>> >> @@ -809,6 +809,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>>>>>>>> >>                       case 6:
>>>>>>>> >>                               ideapad_input_report(priv, vpc_bit);
>>>>>>>> >>                               break;
>>>>>>>> >> +                     case 10:
>>>>>>>> >>                       case 5:
>>>>>>>> >>                               ideapad_sync_touchpad_state(priv);
>>>>>>>>
>>>>>>>> I'm not sure this is the right action here: ideapad_sync_touchpad()
>>>>>>>> sends a sequence of KEY_TOUCHPAD_DISABLE and KEY_TOUCHPAD_ENABLED,
>>>>>>>> which are interpreted by userspace as "the touchpad has been
>>>>>>>> {dis,en}abled by the hardware". The way userspace is expected to react
>>>>>>>> to it is by showing a notification of the fact to the user, but not
>>>>>>>> disabling the touchpad. This behavior can be confirmed in the original
>>>>>>>> commit message that introduced this change (I couldn't find any actual
>>>>>>>> documentation on this): "Input: add keycodes for touchpad on/off keys"
>>>>>>>> (0417596f66). I was actually going to propose a patch similar to Hans'
>>>>>>>> "ideapad-laptop: Disable touchpad interface on Yoga models"
>>>>>>>> (f79a901331) for the Yoga 900, to avoid an "touchpad ON" OSD
>>>>>>>> notification being shown to the user when returning for suspend.
>>>>>>>>
>>>>>>>> Also, I've been investigating touchpad-related problems in the Lenovo
>>>>>>>> Yoga 900 recently, and trying to understand this code. IIUC
>>>>>>>> ideadpad_sync_touchpad_state() was created as part of "ideapad: add
>>>>>>>> Lenovo IdeaPad Z570 support (part 2)" (07a4a4fc8) to allow userspace
>>>>>>>> to syncronize a touchpad-state-indicator LED with the actual touchpad
>>>>>>>> state, although it seems to me it only works if the touchpad is always
>>>>>>>> enabled by the hardware on resume (cc'ing the original patch author
>>>>>>>> here for maybe a confirmation on how this is expected to work).
>>>>>>>
>>>>>>> This is a short explanation of this function.
>>>>>>>
>>>>>>> We call ideapad_sync_touchpad_state when a touchpad state changed
>>>>>>> event arrives (touchpad on/off button pressed) or we need to read  out
>>>>>>> the initial touchpad state. It is also called on resume because
>>>>>>> possibly the touchpad state may be changed after resume, but I don't
>>>>>>> know which devices behave like that (and unfortunately I'm unable to
>>>>>>> test it on my Z570 because now it's dead).
>>>>>>>
>>>>>>
>>>>>> So if I'm following this correctly, on some ideapads (Z570 and maybe
>>>>>> others) there is a ACPI notification when the touchpad on/off hotkey
>>>>>> is pressed, is that correct?
>>>>>
>>>>> Yes, exactly.
>>>>>
>>>>>> This is not what I see on the Yoga 900 or
>>>>>> the Yoga 3, but a special scancode (0xbf on the Yoga 900, 0xbe on the
>>>>>> Yoga 3) come through the AT keyboard device. So here I'm using hwdb to
>>>>>> map these to KEY_TOUCHPAD_TOGGLE (pending upstream submission to
>>>>>> udev).
>>>>>
>>>>> If KEY_TOUCHPAD_TOGGLE is sent to the userspace on the Yoga, no
>>>>> KEY_TOUCHPAD_ON or KEY_TOUCHPAD_OFF should be ever sent. Only one of
>>>>> these mechanisms should be used for userspace to handle this
>>>>> correctly.
>>>>>
>>>>
>>>> Agreed. That's why I was planning to propose a patch very similar to
>>>> Hans' (f79a901331 "ideapad-laptop: Disable touchpad interface on Yoga
>>>> models"), but it was later reverted by 3b264d279e. I wonder if there
>>>> is a more specific way to tell one model from the other.
>>>>
>>>>>>> ideadpad_sync_touchpad_state serves several purposes:
>>>>>>>
>>>>>>> - It actually reads out current touchpad state. It is necessary when
>>>>>>> we are reading the initial state, and also this read is necessary on
>>>>>>> touchpad state changed events, because on some laptops (e.g. Z570)
>>>>>>> touchpad LED only changes its state after this read.
>>>>>>>
>>>>>>
>>>>>> On the Yogas I've been testing this the state being read on
>>>>>> ideapad_sync_touchpad_state() never changes, so even if I press the
>>>>>> touchpad disable/enable hotkey before suspending, I always get
>>>>>> KEY_TOUCHPAD_ON.
>>>>>
>>>>> So if VPCCMD_R_TOUCHPAD always returns the same value and not the
>>>>> actual touchpad state, we should not send KEY_TOUCHPAD_ON on these
>>>>> devices as it conflicts with KEY_TOUCHPAD_TOGGLE and doesn't reflect
>>>>> the real touchpad state. Is there any way to determine in runtime if
>>>>> VPCCMD_R_TOUCHPAD works correctly on the device? BTW, could you send
>>>>> me a DSDT dump from Yoga where it always shows touchpad on?
>>>>>
>>>>
>>>> I imagine it is expected to return the same value because, from the
>>>> hardware perspective, the touchpad is always enabled. I'm not sure of
>>>> a good way to verify this, maybe disabling/re-enabling the i8042 AUX
>>>> and checking its value when the module first loads (not sure if this
>>>> is a good idea).
>>>
>>> Not sure how disabling/enabling AUX port will help us determine if EC
>>> is capable of disabling touchpad and reporting disabled state.
>>>
>>
>> If you disable the AUX port and read the touchpad state, on the Yoga
>> you will still read the "enabled" value (1).
>>
>>>> The Yoga 900 DSDT can be found here:
>>>> https://gist.githubusercontent.com/jprvita/5737de3cbb670e80973b7d4e51c38ab6/raw/189220cb746a719026bde2fa3e75ef5cb24e1931/yoga-900-dsdt.dsl
>>>
>>> Okay, I took a look at this DSDT, and the VPC interface used by
>>> ideapad-laptop driver is completely opaque here. On my Z570 there was
>>> XCMD method that was called from VPCW and handled all the commands. I
>>> was hoping to find this method in Yoga DSDT to examine its code and
>>> check if VPCCMD_R_TOUCHPAD actually returns any non-constant value or
>>> just reports a constant, but on this device VPCW just writes the
>>> command to the register, and maybe EC handles it in its firmware. So
>>> there is no XCMD method and the code that handles VPC commands is
>>> unavailable.
>>>
>>> My idea was that it could be possible that on the Yoga
>>> VPCCMD_R_TOUCHPAD returns always the same value, but the value is
>>> neither 0 nor 1, but some other non-zero constant indicating that
>>> touchpad state reporting does not work. Unfortunately DSDT did not
>>> help me to check this hypothesis, but we still can check it by looking
>>> at the value of the variable value in ideapad_sync_touchpad_state. I
>>> think that most likely we will just get 1 there, but if we get another
>>> value it will be the nice way to distinguish devices where we need
>>> touchpad control from others. (Actually we need to check also if it is
>>> actually 1 on the devices with working touchpad control, because I
>>> don't remember for sure and have no device to test it.)
>>>
>>
>> It is always 1 on the Yoga 900.
>>
>>>>>>> - Then it disables/enables i8042 AUX port to disable/enable touchpad.
>>>>>>> It is also necessary, because some laptops (Z570) do not disable
>>>>>>> touchpad in hardware although they are storing touchpad state.
>>>>>>>
>>>>>>
>>>>>> Ok, but this will only work if the touchpad is connected through that
>>>>>> bus. On the Yoga 900 and Yoga 3 the are connected via I2C, and I guess
>>>>>> this might be true for other machines as well.
>>>>>
>>>>> If I remember correctly, some ideapads actually disable the touchpad
>>>>> in hardware and some (e.g. Z570) only toggle the LED. For those ones
>>>>> that have PS/2 touchpad and don't disable it in hardware,
>>>>> I8042_CMD_AUX_ENABLE/DISABLE works. I'm not sure if any ideapad exists
>>>>> that has e.g. I2C touchpad and doesn't disable it in hardware, but
>>>>> only toggles the LED. If such laptop is a case, i8042 method will not
>>>>> work. Is there any better and universal way to disable touchpad from
>>>>> the driver?
>>>>>
>>>>
>>>> I don't know if such laptop model exists either, but we can probably
>>>> leave this case for when (if) it shows up.
>>>>
>>>>>>> - Finally it sends userspace event with new touchpad state. It does
>>>>>>> not send a sequence of KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON as you
>>>>>>> mentioned, but only one of these key codes.
>>>>>>>
>>>>>>
>>>>>> Right, I got confused when I wrote this email by the comments in the
>>>>>> function implementation, but when I checked with evtest I did see only
>>>>>> one event being sent, indeed.
>>>>>
>>>>> OK, when I read those comments now, they look really confusing,
>>>>> espessially "We send KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON" part. Sorry
>>>>> for this confusion, it should mean that we send one of those codes
>>>>> depending on the touchpad state, not that we send both of them.
>>>>>
>>>>>>> The second point is kinda hack. In Linux there are two ways of
>>>>>>> reporting touchpad state events to the userspace:
>>>>>>>
>>>>>>> - KEY_TOUCHPAD_TOGGLE for those laptops that do not disable touchpad
>>>>>>> in hardware and thus don't store touchpad state. The userspace should
>>>>>>> listen for those key presses and disable/enable touchpad
>>>>>>> programmatically (e.g. TouchpadOff in synaptics X11 driver).
>>>>>>>
>>>>>>
>>>>>> Yes, when the notification for the disable/enable touchpad hotkey goes
>>>>>> through ACPI.
>>>>>
>>>>> Sorry, why is ACPI important in this case?
>>>>>
>>>>
>>>> I mean, if the hardware sends a notification through ACPI that needs
>>>> to be handled by the kernel, instead of a keypress event on the
>>>> keyboard device with a special scancode. But anyway, my comment here
>>>> didn't really add anything.
>>>>
>>>>>>> - KEY_TOUCHPAD_ON and KEY_TOUCHPAD_OFF for those laptops that fully
>>>>>>> manage touchpad state in hardware and just report its state to the
>>>>>>> userspace so that notification can be shown. Userspace should not
>>>>>>> disable touchpad programmatically on these events, because it is
>>>>>>> managed by hardware and may easily get out of sync if using multiple
>>>>>>> user sessions.
>>>>>>>
>>>>>>
>>>>>> Yes, and that is what I tried to explain on my previous comment. In
>>>>>> this case userspace will usually want to notify the user that the
>>>>>> touchpad state has been changed.
>>>>>>
>>>>>>> Ideapad Z570 matches none of these options. It looks more like the
>>>>>>> second one, it stores the touchpad state in hardware, controls the
>>>>>>> LED, but it lacks the ability to disable the touchpad in hardware. So
>>>>>>> we need to keep track of the hardware state and disable i8042 AUX port
>>>>>>> from the driver when necessary.
>>>>>>>
>>>>>>
>>>>>> Isn't the case that what it actually stores under VPCCMD_R_TOUCHPAD is
>>>>>> only the touchpad LED state?
>>>>>
>>>>> Yes, Z570 only stores the LED state.
>>>>>
>>>>>> If so, wouldn't it work if when ACPI
>>>>>> notifies the kernel of event 5 (the touchpad disable/enable key has
>>>>>> been pressed) we simply sent KEY_TOUCHPAD_TOGGLE to userspace? I'm not
>>>>>> entirely sure what would be best way to keep the LED in sync with the
>>>>>> touchpad state in that case, the only way I can think of if to expose
>>>>>> the LED to userspace so it can update it accordingly.
>>>>>
>>>>> No, sadly it wouldn't work correctly, because on Z570 we can't change
>>>>> the LED state programmatically. There is VPCCMD_W_TOUCHPAD, but as far
>>>>> as I remember it doesn't work at least on Z570.
>>>>>
>>>>
>>>> So what you are saying is that the only way to update the LED state is
>>>> to kill the i8042 AUX port.
>>>
>>> Not exactly. We can't update the LED state, so the firmware updates
>>> it, and we just need to obey it and switch touchpad on/off
>>> accordingly.
>>>
>>>> In this case I can't think of a different
>>>> way for this to work on the Z570 and other models that have a touchpad
>>>> LED. So it seems to me we should keep the current logic but make sure
>>>> it only runs on machines that actually need it. I'm now wondering if
>>>> this actually really needs to be called on resume, tho.
>>>
>>> Interesting question, it's really worth checking if it is necessary on
>>> resume, but unfortunately I can't do this test because my Z570 is
>>> dead, it does not turn on.
>>>
>>
>> Maybe we could remove that call and see if someone complains? Not a
>> very nice policy, tho.
>
> I'd prefer testing it in advance on a potentially affected device.
> Things may break for users, but only a few of them would file a bug
> report or investigate the problem by themselves and post to the mail
> list.
>

Yes, I would prefer that too, but I don't have access to any atm (but
I'm seeking one, as I mentioned in a separate message). Going back to
the matter of whether or not the Z570 & co need to call
ideapad_sync_touchpad_state() on resume, the comments in the code may
indicate you need to read VPCCMD_W_TOUCHPAD in order to have the LED
be updated with the current touchpad status. So if the LED goes OFF
during suspend (which it probably does), you may need to read the
value in order to sync the LED state. Do you recall if that was the
behavior? In that case I don't think we need to send a notification to
userspace, so we may split the KEY_TOUCHPAD_{ON,OFF} notification out
of ideapad_sync_touchpad_state() and not send it on resume or when the
driver is loaded. What do you think?

--
João Paulo Rechi Vita
http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Maxim Mikityanskiy June 2, 2016, 6:10 p.m. UTC | #12

2016-06-02 17:38 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
> On 1 June 2016 at 00:37, Maxim Mikityanskiy <[e-mail]> wrote:
>> 2016-06-01 1:43 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>> On 26 May 2016 at 15:44, Maxim Mikityanskiy <[e-mail]> wrote:
>>>> 2016-05-25 19:03 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>> On 25 May 2016 at 02:26, Maxim Mikityanskiy <[e-mail]> wrote:
>>>>>> 2016-05-24 23:32 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>>>> On 19 May 2016 at 05:17, Maxim Mikityanskiy <[e-mail]> wrote:
>>>>>>>> 2016-05-16 19:04 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>>>>>>
>>>>>>>>> Adding Maxim Mikityanskiy and Hans de Goede to CC.
>>>>>>>>>
>>>>>>>>> On 5 May 2016 at 19:42, Darren Hart <[e-mail]> wrote:
>>>>>>>>> > On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
>>>>>>>>> >> From: Christian Hesse <[e-mail]>
>>>>>>>>> >>
>>>>>>>>> >> When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
>>>>>>>>> >> send touchpad key codes so software can disable touchpad.
>>>>>>>>> >>
>>>>>>>>> >> Signed-off-by: Christian Hesse <[e-mail]>
>>>>>>>>> >> Signed-off-by: Michael Gisbers <[e-mail]>
>>>>>>>>> >
>>>>>>>>> > Queued, thanks.
>>>>>>>>> >
>>>>>>>>> >> ---
>>>>>>>>> >>  drivers/platform/x86/ideapad-laptop.c | 1 +
>>>>>>>>> >>  1 file changed, 1 insertion(+)
>>>>>>>>> >>
>>>>>>>>> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>>>>>>>>> >> index be3bc2f..1d49db1 100644
>>>>>>>>> >> --- a/drivers/platform/x86/ideapad-laptop.c
>>>>>>>>> >> +++ b/drivers/platform/x86/ideapad-laptop.c
>>>>>>>>> >> @@ -809,6 +809,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>>>>>>>>> >>                       case 6:
>>>>>>>>> >>                               ideapad_input_report(priv, vpc_bit);
>>>>>>>>> >>                               break;
>>>>>>>>> >> +                     case 10:
>>>>>>>>> >>                       case 5:
>>>>>>>>> >>                               ideapad_sync_touchpad_state(priv);
>>>>>>>>>
>>>>>>>>> I'm not sure this is the right action here: ideapad_sync_touchpad()
>>>>>>>>> sends a sequence of KEY_TOUCHPAD_DISABLE and KEY_TOUCHPAD_ENABLED,
>>>>>>>>> which are interpreted by userspace as "the touchpad has been
>>>>>>>>> {dis,en}abled by the hardware". The way userspace is expected to react
>>>>>>>>> to it is by showing a notification of the fact to the user, but not
>>>>>>>>> disabling the touchpad. This behavior can be confirmed in the original
>>>>>>>>> commit message that introduced this change (I couldn't find any actual
>>>>>>>>> documentation on this): "Input: add keycodes for touchpad on/off keys"
>>>>>>>>> (0417596f66). I was actually going to propose a patch similar to Hans'
>>>>>>>>> "ideapad-laptop: Disable touchpad interface on Yoga models"
>>>>>>>>> (f79a901331) for the Yoga 900, to avoid an "touchpad ON" OSD
>>>>>>>>> notification being shown to the user when returning for suspend.
>>>>>>>>>
>>>>>>>>> Also, I've been investigating touchpad-related problems in the Lenovo
>>>>>>>>> Yoga 900 recently, and trying to understand this code. IIUC
>>>>>>>>> ideadpad_sync_touchpad_state() was created as part of "ideapad: add
>>>>>>>>> Lenovo IdeaPad Z570 support (part 2)" (07a4a4fc8) to allow userspace
>>>>>>>>> to syncronize a touchpad-state-indicator LED with the actual touchpad
>>>>>>>>> state, although it seems to me it only works if the touchpad is always
>>>>>>>>> enabled by the hardware on resume (cc'ing the original patch author
>>>>>>>>> here for maybe a confirmation on how this is expected to work).
>>>>>>>>
>>>>>>>> This is a short explanation of this function.
>>>>>>>>
>>>>>>>> We call ideapad_sync_touchpad_state when a touchpad state changed
>>>>>>>> event arrives (touchpad on/off button pressed) or we need to read  out
>>>>>>>> the initial touchpad state. It is also called on resume because
>>>>>>>> possibly the touchpad state may be changed after resume, but I don't
>>>>>>>> know which devices behave like that (and unfortunately I'm unable to
>>>>>>>> test it on my Z570 because now it's dead).
>>>>>>>>
>>>>>>>
>>>>>>> So if I'm following this correctly, on some ideapads (Z570 and maybe
>>>>>>> others) there is a ACPI notification when the touchpad on/off hotkey
>>>>>>> is pressed, is that correct?
>>>>>>
>>>>>> Yes, exactly.
>>>>>>
>>>>>>> This is not what I see on the Yoga 900 or
>>>>>>> the Yoga 3, but a special scancode (0xbf on the Yoga 900, 0xbe on the
>>>>>>> Yoga 3) come through the AT keyboard device. So here I'm using hwdb to
>>>>>>> map these to KEY_TOUCHPAD_TOGGLE (pending upstream submission to
>>>>>>> udev).
>>>>>>
>>>>>> If KEY_TOUCHPAD_TOGGLE is sent to the userspace on the Yoga, no
>>>>>> KEY_TOUCHPAD_ON or KEY_TOUCHPAD_OFF should be ever sent. Only one of
>>>>>> these mechanisms should be used for userspace to handle this
>>>>>> correctly.
>>>>>>
>>>>>
>>>>> Agreed. That's why I was planning to propose a patch very similar to
>>>>> Hans' (f79a901331 "ideapad-laptop: Disable touchpad interface on Yoga
>>>>> models"), but it was later reverted by 3b264d279e. I wonder if there
>>>>> is a more specific way to tell one model from the other.
>>>>>
>>>>>>>> ideadpad_sync_touchpad_state serves several purposes:
>>>>>>>>
>>>>>>>> - It actually reads out current touchpad state. It is necessary when
>>>>>>>> we are reading the initial state, and also this read is necessary on
>>>>>>>> touchpad state changed events, because on some laptops (e.g. Z570)
>>>>>>>> touchpad LED only changes its state after this read.
>>>>>>>>
>>>>>>>
>>>>>>> On the Yogas I've been testing this the state being read on
>>>>>>> ideapad_sync_touchpad_state() never changes, so even if I press the
>>>>>>> touchpad disable/enable hotkey before suspending, I always get
>>>>>>> KEY_TOUCHPAD_ON.
>>>>>>
>>>>>> So if VPCCMD_R_TOUCHPAD always returns the same value and not the
>>>>>> actual touchpad state, we should not send KEY_TOUCHPAD_ON on these
>>>>>> devices as it conflicts with KEY_TOUCHPAD_TOGGLE and doesn't reflect
>>>>>> the real touchpad state. Is there any way to determine in runtime if
>>>>>> VPCCMD_R_TOUCHPAD works correctly on the device? BTW, could you send
>>>>>> me a DSDT dump from Yoga where it always shows touchpad on?
>>>>>>
>>>>>
>>>>> I imagine it is expected to return the same value because, from the
>>>>> hardware perspective, the touchpad is always enabled. I'm not sure of
>>>>> a good way to verify this, maybe disabling/re-enabling the i8042 AUX
>>>>> and checking its value when the module first loads (not sure if this
>>>>> is a good idea).
>>>>
>>>> Not sure how disabling/enabling AUX port will help us determine if EC
>>>> is capable of disabling touchpad and reporting disabled state.
>>>>
>>>
>>> If you disable the AUX port and read the touchpad state, on the Yoga
>>> you will still read the "enabled" value (1).
>>>
>>>>> The Yoga 900 DSDT can be found here:
>>>>> https://gist.githubusercontent.com/jprvita/5737de3cbb670e80973b7d4e51c38ab6/raw/189220cb746a719026bde2fa3e75ef5cb24e1931/yoga-900-dsdt.dsl
>>>>
>>>> Okay, I took a look at this DSDT, and the VPC interface used by
>>>> ideapad-laptop driver is completely opaque here. On my Z570 there was
>>>> XCMD method that was called from VPCW and handled all the commands. I
>>>> was hoping to find this method in Yoga DSDT to examine its code and
>>>> check if VPCCMD_R_TOUCHPAD actually returns any non-constant value or
>>>> just reports a constant, but on this device VPCW just writes the
>>>> command to the register, and maybe EC handles it in its firmware. So
>>>> there is no XCMD method and the code that handles VPC commands is
>>>> unavailable.
>>>>
>>>> My idea was that it could be possible that on the Yoga
>>>> VPCCMD_R_TOUCHPAD returns always the same value, but the value is
>>>> neither 0 nor 1, but some other non-zero constant indicating that
>>>> touchpad state reporting does not work. Unfortunately DSDT did not
>>>> help me to check this hypothesis, but we still can check it by looking
>>>> at the value of the variable value in ideapad_sync_touchpad_state. I
>>>> think that most likely we will just get 1 there, but if we get another
>>>> value it will be the nice way to distinguish devices where we need
>>>> touchpad control from others. (Actually we need to check also if it is
>>>> actually 1 on the devices with working touchpad control, because I
>>>> don't remember for sure and have no device to test it.)
>>>>
>>>
>>> It is always 1 on the Yoga 900.
>>>
>>>>>>>> - Then it disables/enables i8042 AUX port to disable/enable touchpad.
>>>>>>>> It is also necessary, because some laptops (Z570) do not disable
>>>>>>>> touchpad in hardware although they are storing touchpad state.
>>>>>>>>
>>>>>>>
>>>>>>> Ok, but this will only work if the touchpad is connected through that
>>>>>>> bus. On the Yoga 900 and Yoga 3 the are connected via I2C, and I guess
>>>>>>> this might be true for other machines as well.
>>>>>>
>>>>>> If I remember correctly, some ideapads actually disable the touchpad
>>>>>> in hardware and some (e.g. Z570) only toggle the LED. For those ones
>>>>>> that have PS/2 touchpad and don't disable it in hardware,
>>>>>> I8042_CMD_AUX_ENABLE/DISABLE works. I'm not sure if any ideapad exists
>>>>>> that has e.g. I2C touchpad and doesn't disable it in hardware, but
>>>>>> only toggles the LED. If such laptop is a case, i8042 method will not
>>>>>> work. Is there any better and universal way to disable touchpad from
>>>>>> the driver?
>>>>>>
>>>>>
>>>>> I don't know if such laptop model exists either, but we can probably
>>>>> leave this case for when (if) it shows up.
>>>>>
>>>>>>>> - Finally it sends userspace event with new touchpad state. It does
>>>>>>>> not send a sequence of KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON as you
>>>>>>>> mentioned, but only one of these key codes.
>>>>>>>>
>>>>>>>
>>>>>>> Right, I got confused when I wrote this email by the comments in the
>>>>>>> function implementation, but when I checked with evtest I did see only
>>>>>>> one event being sent, indeed.
>>>>>>
>>>>>> OK, when I read those comments now, they look really confusing,
>>>>>> espessially "We send KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON" part. Sorry
>>>>>> for this confusion, it should mean that we send one of those codes
>>>>>> depending on the touchpad state, not that we send both of them.
>>>>>>
>>>>>>>> The second point is kinda hack. In Linux there are two ways of
>>>>>>>> reporting touchpad state events to the userspace:
>>>>>>>>
>>>>>>>> - KEY_TOUCHPAD_TOGGLE for those laptops that do not disable touchpad
>>>>>>>> in hardware and thus don't store touchpad state. The userspace should
>>>>>>>> listen for those key presses and disable/enable touchpad
>>>>>>>> programmatically (e.g. TouchpadOff in synaptics X11 driver).
>>>>>>>>
>>>>>>>
>>>>>>> Yes, when the notification for the disable/enable touchpad hotkey goes
>>>>>>> through ACPI.
>>>>>>
>>>>>> Sorry, why is ACPI important in this case?
>>>>>>
>>>>>
>>>>> I mean, if the hardware sends a notification through ACPI that needs
>>>>> to be handled by the kernel, instead of a keypress event on the
>>>>> keyboard device with a special scancode. But anyway, my comment here
>>>>> didn't really add anything.
>>>>>
>>>>>>>> - KEY_TOUCHPAD_ON and KEY_TOUCHPAD_OFF for those laptops that fully
>>>>>>>> manage touchpad state in hardware and just report its state to the
>>>>>>>> userspace so that notification can be shown. Userspace should not
>>>>>>>> disable touchpad programmatically on these events, because it is
>>>>>>>> managed by hardware and may easily get out of sync if using multiple
>>>>>>>> user sessions.
>>>>>>>>
>>>>>>>
>>>>>>> Yes, and that is what I tried to explain on my previous comment. In
>>>>>>> this case userspace will usually want to notify the user that the
>>>>>>> touchpad state has been changed.
>>>>>>>
>>>>>>>> Ideapad Z570 matches none of these options. It looks more like the
>>>>>>>> second one, it stores the touchpad state in hardware, controls the
>>>>>>>> LED, but it lacks the ability to disable the touchpad in hardware. So
>>>>>>>> we need to keep track of the hardware state and disable i8042 AUX port
>>>>>>>> from the driver when necessary.
>>>>>>>>
>>>>>>>
>>>>>>> Isn't the case that what it actually stores under VPCCMD_R_TOUCHPAD is
>>>>>>> only the touchpad LED state?
>>>>>>
>>>>>> Yes, Z570 only stores the LED state.
>>>>>>
>>>>>>> If so, wouldn't it work if when ACPI
>>>>>>> notifies the kernel of event 5 (the touchpad disable/enable key has
>>>>>>> been pressed) we simply sent KEY_TOUCHPAD_TOGGLE to userspace? I'm not
>>>>>>> entirely sure what would be best way to keep the LED in sync with the
>>>>>>> touchpad state in that case, the only way I can think of if to expose
>>>>>>> the LED to userspace so it can update it accordingly.
>>>>>>
>>>>>> No, sadly it wouldn't work correctly, because on Z570 we can't change
>>>>>> the LED state programmatically. There is VPCCMD_W_TOUCHPAD, but as far
>>>>>> as I remember it doesn't work at least on Z570.
>>>>>>
>>>>>
>>>>> So what you are saying is that the only way to update the LED state is
>>>>> to kill the i8042 AUX port.
>>>>
>>>> Not exactly. We can't update the LED state, so the firmware updates
>>>> it, and we just need to obey it and switch touchpad on/off
>>>> accordingly.
>>>>
>>>>> In this case I can't think of a different
>>>>> way for this to work on the Z570 and other models that have a touchpad
>>>>> LED. So it seems to me we should keep the current logic but make sure
>>>>> it only runs on machines that actually need it. I'm now wondering if
>>>>> this actually really needs to be called on resume, tho.
>>>>
>>>> Interesting question, it's really worth checking if it is necessary on
>>>> resume, but unfortunately I can't do this test because my Z570 is
>>>> dead, it does not turn on.
>>>>
>>>
>>> Maybe we could remove that call and see if someone complains? Not a
>>> very nice policy, tho.
>>
>> I'd prefer testing it in advance on a potentially affected device.
>> Things may break for users, but only a few of them would file a bug
>> report or investigate the problem by themselves and post to the mail
>> list.
>>
>
> Yes, I would prefer that too, but I don't have access to any atm (but
> I'm seeking one, as I mentioned in a separate message).

Another option is to repair my broken Z570, but it would take some
time. Also it was already repaired once but broke again with the
similar symptoms so I don't think it would live for long after the
repair.

> Going back to
> the matter of whether or not the Z570 & co need to call
> ideapad_sync_touchpad_state() on resume, the comments in the code may
> indicate you need to read VPCCMD_W_TOUCHPAD

We read from VPCCMD_R_TOUCHPAD, not _W_. W is not used in the driver
and has no effect on Z570.

> in order to have the LED
> be updated with the current touchpad status. So if the LED goes OFF
> during suspend (which it probably does), you may need to read the
> value in order to sync the LED state. Do you recall if that was the
> behavior? In that case I don't think we need to send a notification to
> userspace, so we may split the KEY_TOUCHPAD_{ON,OFF} notification out
> of ideapad_sync_touchpad_state() and not send it on resume or when the
> driver is loaded. What do you think?

OK, I can't recall what was the behavior on resume, but I have re-read
the commit message of
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=07a4a4fc83dd95bc7eb842cf9510ddcb45691a88
and it says "and corrects touchpad behavior on resume from suspend".
So there definitely were some problems without
ideapad_sync_touchpad_state call from ideapad_acpi_resume. I don't
remember exactly, but maybe it could be touchpad enabling (and LED
turning off) after every resume, i.e. not preserving touchpad state
across suspends and resumes.

What about notifications, I don't know for sure who needs to filter
identical touchpad events in a row, either the driver or the
userspace. Maybe it's worth looking at other drivers' code to see if
they try to keep track of touchpad state and don't send several
identical events in a row, and also to look at GNOME and KDE code to
see whether they just display notifications every time the keycode
arrives or they try to keep track of the touchpad state.

Finally, I don't remember if I mentioned it already, but the usage of
i8042_command is wrong here and should be fixed. It should be wrapped
in i8042_lock_chip/i8042_unlock_chip, that's the thing that should be
fixed in the ideapad-laptop and some other platform drivers. It is
mentioned here:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/input/serio/i8042.c?id=refs/tags/v4.7-rc1#n111

> --
> João Paulo Rechi Vita
> http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

João Paulo Rechi Vita June 6, 2016, 4:25 p.m. UTC | #13

On 2 June 2016 at 14:10, Maxim Mikityanskiy <[e-mail]> wrote:
> 2016-06-02 17:38 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>> On 1 June 2016 at 00:37, Maxim Mikityanskiy <[e-mail]> wrote:
>>> 2016-06-01 1:43 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>> On 26 May 2016 at 15:44, Maxim Mikityanskiy <[e-mail]> wrote:
>>>>> 2016-05-25 19:03 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>>> On 25 May 2016 at 02:26, Maxim Mikityanskiy <[e-mail]> wrote:
>>>>>>> 2016-05-24 23:32 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>>>>> On 19 May 2016 at 05:17, Maxim Mikityanskiy <[e-mail]> wrote:
>>>>>>>>> 2016-05-16 19:04 GMT+03:00 João Paulo Rechi Vita <[e-mail]>:
>>>>>>>>>>
>>>>>>>>>> Adding Maxim Mikityanskiy and Hans de Goede to CC.
>>>>>>>>>>
>>>>>>>>>> On 5 May 2016 at 19:42, Darren Hart <[e-mail]> wrote:
>>>>>>>>>> > On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
>>>>>>>>>> >> From: Christian Hesse <[e-mail]>
>>>>>>>>>> >>
>>>>>>>>>> >> When Lenovo Yoga 700 is flipped to tablet mode it emmits event 10. Let's
>>>>>>>>>> >> send touchpad key codes so software can disable touchpad.
>>>>>>>>>> >>
>>>>>>>>>> >> Signed-off-by: Christian Hesse <[e-mail]>
>>>>>>>>>> >> Signed-off-by: Michael Gisbers <[e-mail]>
>>>>>>>>>> >
>>>>>>>>>> > Queued, thanks.
>>>>>>>>>> >
>>>>>>>>>> >> ---
>>>>>>>>>> >>  drivers/platform/x86/ideapad-laptop.c | 1 +
>>>>>>>>>> >>  1 file changed, 1 insertion(+)
>>>>>>>>>> >>
>>>>>>>>>> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>>>>>>>>>> >> index be3bc2f..1d49db1 100644
>>>>>>>>>> >> --- a/drivers/platform/x86/ideapad-laptop.c
>>>>>>>>>> >> +++ b/drivers/platform/x86/ideapad-laptop.c
>>>>>>>>>> >> @@ -809,6 +809,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>>>>>>>>>> >>                       case 6:
>>>>>>>>>> >>                               ideapad_input_report(priv, vpc_bit);
>>>>>>>>>> >>                               break;
>>>>>>>>>> >> +                     case 10:
>>>>>>>>>> >>                       case 5:
>>>>>>>>>> >>                               ideapad_sync_touchpad_state(priv);
>>>>>>>>>>
>>>>>>>>>> I'm not sure this is the right action here: ideapad_sync_touchpad()
>>>>>>>>>> sends a sequence of KEY_TOUCHPAD_DISABLE and KEY_TOUCHPAD_ENABLED,
>>>>>>>>>> which are interpreted by userspace as "the touchpad has been
>>>>>>>>>> {dis,en}abled by the hardware". The way userspace is expected to react
>>>>>>>>>> to it is by showing a notification of the fact to the user, but not
>>>>>>>>>> disabling the touchpad. This behavior can be confirmed in the original
>>>>>>>>>> commit message that introduced this change (I couldn't find any actual
>>>>>>>>>> documentation on this): "Input: add keycodes for touchpad on/off keys"
>>>>>>>>>> (0417596f66). I was actually going to propose a patch similar to Hans'
>>>>>>>>>> "ideapad-laptop: Disable touchpad interface on Yoga models"
>>>>>>>>>> (f79a901331) for the Yoga 900, to avoid an "touchpad ON" OSD
>>>>>>>>>> notification being shown to the user when returning for suspend.
>>>>>>>>>>
>>>>>>>>>> Also, I've been investigating touchpad-related problems in the Lenovo
>>>>>>>>>> Yoga 900 recently, and trying to understand this code. IIUC
>>>>>>>>>> ideadpad_sync_touchpad_state() was created as part of "ideapad: add
>>>>>>>>>> Lenovo IdeaPad Z570 support (part 2)" (07a4a4fc8) to allow userspace
>>>>>>>>>> to syncronize a touchpad-state-indicator LED with the actual touchpad
>>>>>>>>>> state, although it seems to me it only works if the touchpad is always
>>>>>>>>>> enabled by the hardware on resume (cc'ing the original patch author
>>>>>>>>>> here for maybe a confirmation on how this is expected to work).
>>>>>>>>>
>>>>>>>>> This is a short explanation of this function.
>>>>>>>>>
>>>>>>>>> We call ideapad_sync_touchpad_state when a touchpad state changed
>>>>>>>>> event arrives (touchpad on/off button pressed) or we need to read  out
>>>>>>>>> the initial touchpad state. It is also called on resume because
>>>>>>>>> possibly the touchpad state may be changed after resume, but I don't
>>>>>>>>> know which devices behave like that (and unfortunately I'm unable to
>>>>>>>>> test it on my Z570 because now it's dead).
>>>>>>>>>
>>>>>>>>
>>>>>>>> So if I'm following this correctly, on some ideapads (Z570 and maybe
>>>>>>>> others) there is a ACPI notification when the touchpad on/off hotkey
>>>>>>>> is pressed, is that correct?
>>>>>>>
>>>>>>> Yes, exactly.
>>>>>>>
>>>>>>>> This is not what I see on the Yoga 900 or
>>>>>>>> the Yoga 3, but a special scancode (0xbf on the Yoga 900, 0xbe on the
>>>>>>>> Yoga 3) come through the AT keyboard device. So here I'm using hwdb to
>>>>>>>> map these to KEY_TOUCHPAD_TOGGLE (pending upstream submission to
>>>>>>>> udev).
>>>>>>>
>>>>>>> If KEY_TOUCHPAD_TOGGLE is sent to the userspace on the Yoga, no
>>>>>>> KEY_TOUCHPAD_ON or KEY_TOUCHPAD_OFF should be ever sent. Only one of
>>>>>>> these mechanisms should be used for userspace to handle this
>>>>>>> correctly.
>>>>>>>
>>>>>>
>>>>>> Agreed. That's why I was planning to propose a patch very similar to
>>>>>> Hans' (f79a901331 "ideapad-laptop: Disable touchpad interface on Yoga
>>>>>> models"), but it was later reverted by 3b264d279e. I wonder if there
>>>>>> is a more specific way to tell one model from the other.
>>>>>>
>>>>>>>>> ideadpad_sync_touchpad_state serves several purposes:
>>>>>>>>>
>>>>>>>>> - It actually reads out current touchpad state. It is necessary when
>>>>>>>>> we are reading the initial state, and also this read is necessary on
>>>>>>>>> touchpad state changed events, because on some laptops (e.g. Z570)
>>>>>>>>> touchpad LED only changes its state after this read.
>>>>>>>>>
>>>>>>>>
>>>>>>>> On the Yogas I've been testing this the state being read on
>>>>>>>> ideapad_sync_touchpad_state() never changes, so even if I press the
>>>>>>>> touchpad disable/enable hotkey before suspending, I always get
>>>>>>>> KEY_TOUCHPAD_ON.
>>>>>>>
>>>>>>> So if VPCCMD_R_TOUCHPAD always returns the same value and not the
>>>>>>> actual touchpad state, we should not send KEY_TOUCHPAD_ON on these
>>>>>>> devices as it conflicts with KEY_TOUCHPAD_TOGGLE and doesn't reflect
>>>>>>> the real touchpad state. Is there any way to determine in runtime if
>>>>>>> VPCCMD_R_TOUCHPAD works correctly on the device? BTW, could you send
>>>>>>> me a DSDT dump from Yoga where it always shows touchpad on?
>>>>>>>
>>>>>>
>>>>>> I imagine it is expected to return the same value because, from the
>>>>>> hardware perspective, the touchpad is always enabled. I'm not sure of
>>>>>> a good way to verify this, maybe disabling/re-enabling the i8042 AUX
>>>>>> and checking its value when the module first loads (not sure if this
>>>>>> is a good idea).
>>>>>
>>>>> Not sure how disabling/enabling AUX port will help us determine if EC
>>>>> is capable of disabling touchpad and reporting disabled state.
>>>>>
>>>>
>>>> If you disable the AUX port and read the touchpad state, on the Yoga
>>>> you will still read the "enabled" value (1).
>>>>
>>>>>> The Yoga 900 DSDT can be found here:
>>>>>> https://gist.githubusercontent.com/jprvita/5737de3cbb670e80973b7d4e51c38ab6/raw/189220cb746a719026bde2fa3e75ef5cb24e1931/yoga-900-dsdt.dsl
>>>>>
>>>>> Okay, I took a look at this DSDT, and the VPC interface used by
>>>>> ideapad-laptop driver is completely opaque here. On my Z570 there was
>>>>> XCMD method that was called from VPCW and handled all the commands. I
>>>>> was hoping to find this method in Yoga DSDT to examine its code and
>>>>> check if VPCCMD_R_TOUCHPAD actually returns any non-constant value or
>>>>> just reports a constant, but on this device VPCW just writes the
>>>>> command to the register, and maybe EC handles it in its firmware. So
>>>>> there is no XCMD method and the code that handles VPC commands is
>>>>> unavailable.
>>>>>
>>>>> My idea was that it could be possible that on the Yoga
>>>>> VPCCMD_R_TOUCHPAD returns always the same value, but the value is
>>>>> neither 0 nor 1, but some other non-zero constant indicating that
>>>>> touchpad state reporting does not work. Unfortunately DSDT did not
>>>>> help me to check this hypothesis, but we still can check it by looking
>>>>> at the value of the variable value in ideapad_sync_touchpad_state. I
>>>>> think that most likely we will just get 1 there, but if we get another
>>>>> value it will be the nice way to distinguish devices where we need
>>>>> touchpad control from others. (Actually we need to check also if it is
>>>>> actually 1 on the devices with working touchpad control, because I
>>>>> don't remember for sure and have no device to test it.)
>>>>>
>>>>
>>>> It is always 1 on the Yoga 900.
>>>>
>>>>>>>>> - Then it disables/enables i8042 AUX port to disable/enable touchpad.
>>>>>>>>> It is also necessary, because some laptops (Z570) do not disable
>>>>>>>>> touchpad in hardware although they are storing touchpad state.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Ok, but this will only work if the touchpad is connected through that
>>>>>>>> bus. On the Yoga 900 and Yoga 3 the are connected via I2C, and I guess
>>>>>>>> this might be true for other machines as well.
>>>>>>>
>>>>>>> If I remember correctly, some ideapads actually disable the touchpad
>>>>>>> in hardware and some (e.g. Z570) only toggle the LED. For those ones
>>>>>>> that have PS/2 touchpad and don't disable it in hardware,
>>>>>>> I8042_CMD_AUX_ENABLE/DISABLE works. I'm not sure if any ideapad exists
>>>>>>> that has e.g. I2C touchpad and doesn't disable it in hardware, but
>>>>>>> only toggles the LED. If such laptop is a case, i8042 method will not
>>>>>>> work. Is there any better and universal way to disable touchpad from
>>>>>>> the driver?
>>>>>>>
>>>>>>
>>>>>> I don't know if such laptop model exists either, but we can probably
>>>>>> leave this case for when (if) it shows up.
>>>>>>
>>>>>>>>> - Finally it sends userspace event with new touchpad state. It does
>>>>>>>>> not send a sequence of KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON as you
>>>>>>>>> mentioned, but only one of these key codes.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Right, I got confused when I wrote this email by the comments in the
>>>>>>>> function implementation, but when I checked with evtest I did see only
>>>>>>>> one event being sent, indeed.
>>>>>>>
>>>>>>> OK, when I read those comments now, they look really confusing,
>>>>>>> espessially "We send KEY_TOUCHPAD_OFF and KEY_TOUCHPAD_ON" part. Sorry
>>>>>>> for this confusion, it should mean that we send one of those codes
>>>>>>> depending on the touchpad state, not that we send both of them.
>>>>>>>
>>>>>>>>> The second point is kinda hack. In Linux there are two ways of
>>>>>>>>> reporting touchpad state events to the userspace:
>>>>>>>>>
>>>>>>>>> - KEY_TOUCHPAD_TOGGLE for those laptops that do not disable touchpad
>>>>>>>>> in hardware and thus don't store touchpad state. The userspace should
>>>>>>>>> listen for those key presses and disable/enable touchpad
>>>>>>>>> programmatically (e.g. TouchpadOff in synaptics X11 driver).
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yes, when the notification for the disable/enable touchpad hotkey goes
>>>>>>>> through ACPI.
>>>>>>>
>>>>>>> Sorry, why is ACPI important in this case?
>>>>>>>
>>>>>>
>>>>>> I mean, if the hardware sends a notification through ACPI that needs
>>>>>> to be handled by the kernel, instead of a keypress event on the
>>>>>> keyboard device with a special scancode. But anyway, my comment here
>>>>>> didn't really add anything.
>>>>>>
>>>>>>>>> - KEY_TOUCHPAD_ON and KEY_TOUCHPAD_OFF for those laptops that fully
>>>>>>>>> manage touchpad state in hardware and just report its state to the
>>>>>>>>> userspace so that notification can be shown. Userspace should not
>>>>>>>>> disable touchpad programmatically on these events, because it is
>>>>>>>>> managed by hardware and may easily get out of sync if using multiple
>>>>>>>>> user sessions.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yes, and that is what I tried to explain on my previous comment. In
>>>>>>>> this case userspace will usually want to notify the user that the
>>>>>>>> touchpad state has been changed.
>>>>>>>>
>>>>>>>>> Ideapad Z570 matches none of these options. It looks more like the
>>>>>>>>> second one, it stores the touchpad state in hardware, controls the
>>>>>>>>> LED, but it lacks the ability to disable the touchpad in hardware. So
>>>>>>>>> we need to keep track of the hardware state and disable i8042 AUX port
>>>>>>>>> from the driver when necessary.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Isn't the case that what it actually stores under VPCCMD_R_TOUCHPAD is
>>>>>>>> only the touchpad LED state?
>>>>>>>
>>>>>>> Yes, Z570 only stores the LED state.
>>>>>>>
>>>>>>>> If so, wouldn't it work if when ACPI
>>>>>>>> notifies the kernel of event 5 (the touchpad disable/enable key has
>>>>>>>> been pressed) we simply sent KEY_TOUCHPAD_TOGGLE to userspace? I'm not
>>>>>>>> entirely sure what would be best way to keep the LED in sync with the
>>>>>>>> touchpad state in that case, the only way I can think of if to expose
>>>>>>>> the LED to userspace so it can update it accordingly.
>>>>>>>
>>>>>>> No, sadly it wouldn't work correctly, because on Z570 we can't change
>>>>>>> the LED state programmatically. There is VPCCMD_W_TOUCHPAD, but as far
>>>>>>> as I remember it doesn't work at least on Z570.
>>>>>>>
>>>>>>
>>>>>> So what you are saying is that the only way to update the LED state is
>>>>>> to kill the i8042 AUX port.
>>>>>
>>>>> Not exactly. We can't update the LED state, so the firmware updates
>>>>> it, and we just need to obey it and switch touchpad on/off
>>>>> accordingly.
>>>>>
>>>>>> In this case I can't think of a different
>>>>>> way for this to work on the Z570 and other models that have a touchpad
>>>>>> LED. So it seems to me we should keep the current logic but make sure
>>>>>> it only runs on machines that actually need it. I'm now wondering if
>>>>>> this actually really needs to be called on resume, tho.
>>>>>
>>>>> Interesting question, it's really worth checking if it is necessary on
>>>>> resume, but unfortunately I can't do this test because my Z570 is
>>>>> dead, it does not turn on.
>>>>>
>>>>
>>>> Maybe we could remove that call and see if someone complains? Not a
>>>> very nice policy, tho.
>>>
>>> I'd prefer testing it in advance on a potentially affected device.
>>> Things may break for users, but only a few of them would file a bug
>>> report or investigate the problem by themselves and post to the mail
>>> list.
>>>
>>
>> Yes, I would prefer that too, but I don't have access to any atm (but
>> I'm seeking one, as I mentioned in a separate message).
>
> Another option is to repair my broken Z570, but it would take some
> time. Also it was already repaired once but broke again with the
> similar symptoms so I don't think it would live for long after the
> repair.
>
>> Going back to
>> the matter of whether or not the Z570 & co need to call
>> ideapad_sync_touchpad_state() on resume, the comments in the code may
>> indicate you need to read VPCCMD_W_TOUCHPAD
>
> We read from VPCCMD_R_TOUCHPAD, not _W_. W is not used in the driver
> and has no effect on Z570.
>

Sorry, typo here, I meant VPCCMD_R_TOUCHPAD.

>> in order to have the LED
>> be updated with the current touchpad status. So if the LED goes OFF
>> during suspend (which it probably does), you may need to read the
>> value in order to sync the LED state. Do you recall if that was the
>> behavior? In that case I don't think we need to send a notification to
>> userspace, so we may split the KEY_TOUCHPAD_{ON,OFF} notification out
>> of ideapad_sync_touchpad_state() and not send it on resume or when the
>> driver is loaded. What do you think?
>
> OK, I can't recall what was the behavior on resume, but I have re-read
> the commit message of
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=07a4a4fc83dd95bc7eb842cf9510ddcb45691a88
> and it says "and corrects touchpad behavior on resume from suspend".
> So there definitely were some problems without
> ideapad_sync_touchpad_state call from ideapad_acpi_resume. I don't
> remember exactly, but maybe it could be touchpad enabling (and LED
> turning off) after every resume, i.e. not preserving touchpad state
> across suspends and resumes.
>

OK, so lets not make any change on the current behavior for all
laptops and go with a DMI quirk. I'm sending a patch in new thread so
it is easier for more people to spot it, but copying everyone from
this thread.

> What about notifications, I don't know for sure who needs to filter
> identical touchpad events in a row, either the driver or the
> userspace. Maybe it's worth looking at other drivers' code to see if
> they try to keep track of touchpad state and don't send several
> identical events in a row, and also to look at GNOME and KDE code to
> see whether they just display notifications every time the keycode
> arrives or they try to keep track of the touchpad state.
>

I was not referring to repeating notifications, but for the laptops
who do not have touchpad control (in which case the notifications are
wrong).

> Finally, I don't remember if I mentioned it already, but the usage of
> i8042_command is wrong here and should be fixed. It should be wrapped
> in i8042_lock_chip/i8042_unlock_chip, that's the thing that should be
> fixed in the ideapad-laptop and some other platform drivers. It is
> mentioned here:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/input/serio/i8042.c?id=refs/tags/v4.7-rc1#n111
>

Not really part of this discussion, but thanks for raising this to attention.

--
João Paulo Rechi Vita
http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index be3bc2f..1d49db1 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -809,6 +809,7 @@  static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 			case 6:
 				ideapad_input_report(priv, vpc_bit);
 				break;
+			case 10:
 			case 5:
 				ideapad_sync_touchpad_state(priv);
 				break;