On 1 June 2016 at 00:37, Maxim Mikityanskiy <maxtram95@xxxxxxxxx> wrote:
> 2016-06-01 1:43 GMT+03:00 João Paulo Rechi Vita <jprvita@xxxxxxxxx>:
>> 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.
>
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
