2016-05-24 23:32 GMT+03:00 João Paulo Rechi Vita <jprvita@xxxxxxxxx>:
> On 19 May 2016 at 05:17, Maxim Mikityanskiy <maxtram95@xxxxxxxxx> wrote:
>> 2016-05-16 19:04 GMT+03:00 João Paulo Rechi Vita <jprvita@xxxxxxxxx>:
>>>
>>> Adding Maxim Mikityanskiy and Hans de Goede to CC.
>>>
>>> On 5 May 2016 at 19:42, Darren Hart <dvhart@xxxxxxxxxxxxx> wrote:
>>> > On Fri, Apr 01, 2016 at 11:02:49PM +0200, Christian Hesse wrote:
>>> >> From: Christian Hesse <mail@xxxxxxxx>
>>> >>
>>> >> 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 <mail@xxxxxxxx>
>>> >> Signed-off-by: Michael Gisbers <michael@xxxxxxxxxx>
>>> >
>>> > 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
