Hi,
On 11/11/22 12:44, Maxim Mikityanskiy wrote:
> On Thu, 10 Nov 2022 at 21:18, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> Hi Eray,
>>
>> On 11/10/22 19:47, Eray Orçunus wrote:
>>> Hi,
>>>
>>> On 10 Nov 2022 at 21:09, Maxim Mikityanskiy <maxtram95@xxxxxxxxx> wrote:
>>>> A small note on the DMI allow-list: I don't think Z570 is the only
>>>> laptop where EC fails to disable the touchpad. While I would like this
>>>> hack to affect as few laptops as possible, I would expect that other
>>>> similar models produced in the same time period suffer from the same
>>>> issue, and I don't think we have the full list of them.
>>>
>>> I just checked Z570 ACPI table, and this is what it does when it receives
>>> VPCCMD_R_TOUCHPAD:
>>>
>>> VDAT = TPEN /* \_SB_.PCI0.LPCB.EC0_.TPEN */
>>> If ((TPEN == One))
>>> {
>>> GL04 |= 0x02
>>> }
>>> Else
>>> {
>>> GL04 &= 0xFD
>>> }
>>>
>>> VDAT is the data returned to user.
>>> So we can say that TPEN is the logical state of touchpad key, and GL04
>>> is state of touchpad LED or series of LEDs.
>>>
>>> VPCCMD_W_TOUCHPAD is nulled, it doesn't work.
>>>
>>> I also checked which DSDTs I have (13 DSDTs from 2008 to this year)
>>> contain TPEN, and turned out it was only S12, from 2009. It also had
>>> nulled VPCCMD_W_TOUCHPAD, and returns TPEN on VPCCMD_R_TOUCHPAD, except
>>> it doesn't have an LED or GL04.
>>>
>>> So, it's possible that we can only check if TPEN exists on ACPI table,
>>> instead of having a white-list.
>>
>> Hmm, lets keep that idea in case it turns out the allow-list based
>> approach turns out to cause issue/grow out of control. I would rather
>> not rely on ACPI variables having a specific name for something like
>> this, but you might be on to something.
>
> Maybe also add a module parameter to force the i8042 workaround? This
> change is likely to break the touchpad toggle for some devices similar
> to Z570 (maybe Z580? Y580? idk), so people would at least have an
> option to force enable it using the module parameter, before the
> relevant entry gets added to the DMI table, the patch gets merged, and
> the kernel gets updated.
That is a good idea. I'm about to send out this as a proper patch-series
and I've added a module parameter to the version.
> Another note on the comments in patch 4:
>
> /*
> * Some IdeaPads don't really turn off touchpad - they only
> * switch the LED state. We (de)activate KBC AUX port to turn
> * the touchpad on and off.
> */
> /*
> * On older models the EC controls the touchpad and toggles it
> * on/off itself, in this case we report KEY_TOUCHPAD_ON/_OFF.
> * If the EC did not toggle, report KEY_TOUCHPAD_TOGGLE.
> */
>
> These two comments would sound confusing to me if I read them without
> having context on Ideapads. I mean this part where "EC controls the
> touchpad and toggles it itself", but at the same time these laptops
> "don't really turn off the touchpad". Could you rephrase it somehow,
> so that it would be clear that there are models where the EC cares
> about the touchpad because it toggles *either* the touchpad or at
> least its LED?
To me the comments are clear the code under the first comment
block starts with a "if (priv->features.ctrl_ps2_aux_port)" that
+ the comment to me makes it clear this is a workaround for
some models where the EC fails to turn off the touchpad itself.
Where as the second comment is just about which key-presses are
being reported and why. With that said suggestions / patches to
improve the comments are certainly welcome.
BTW did you ever try simply writing back the value returned from
VPCCMD_R_TOUCHPAD to VPCCMD_W_TOUCHPAD ?
Eray has been looking at what the Windows tools do and according
to Eray they always call VPCCMD_W_TOUCHPAD on touchpad toggle
events. So maybe just writing back the value is enough to actually
disable the touchpad ?
Regards,
Hans
