On 26 May 2016 at 15:44, Maxim Mikityanskiy <maxtram95@xxxxxxxxx> wrote:
> 2016-05-25 19:03 GMT+03:00 João Paulo Rechi Vita <jprvita@xxxxxxxxx>:
>> On 25 May 2016 at 02:26, Maxim Mikityanskiy <maxtram95@xxxxxxxxx> wrote:
>>> 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.
>>>
>>
>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
