On Thu, 10 Nov 2022 at 19:54, Maxim Mikityanskiy <maxtram95@xxxxxxxxx> wrote: > > On Thu, 10 Nov 2022 at 18:42, Eray Orçunus <erayorcunus@xxxxxxxxx> wrote: > > > > Hi Hans, > > > > On 10 Nov 2022 at 15:37, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > > On 11/10/22 13:00, Eray Orçunus wrote: > > > > While I agree with your findings(and thanks for your effort, I also tried > > > > to solve this but later gave up), they doesn't apply to one type of > > > > IdeaPads (like my 520-15IKB), so I'm sure this patch won't work on me. > > > > This is because my IdeaPad has this features: > > > > > > > > - i8042.noaux doesn't affect touchpad, and it's connected over i2c > > > > - There is no touchpad LED, and touchpad hotkey only sends "key pressed" > > > > ACPI event, doesn't do anything else > > > > - VPCCMD_W_TOUCHPAD does nothing (also confirmed on Windows) > > > > - Sending 1 to VPCCMD_W_TOUCHPAD on boot is not needed > > > > - VPCCMD_R_TOUCHPAD always returns 1 (this is interesting) > > > > > > So if i8042.noaux does not do anything, then why do you want to add > > > "SYNA2B33" to the list of ACPI HIDs for which we set: > > > > > > features.touchpad_ctrl_via_ec=0; > > > > > > ? > > > > > > IOW what bad effects / behavior are you seeing with touchpad_ctrl_via_ec=1? > > > > > > Or are you seeing bad behavior on some other modes? If yes, then what > > > is the bad behavior you are seeing on other models ? > > > > It was just because I didn't want to have a not working "touchpad" > > attribute :) I used/still using several GNOME extensions and they show > > me "Touchpad" toggle just because I have "touchpad" attribute exposed > > there, which is doing nothing, and misleading. > > > > But I would understand if you don't want to touch it at that stage, and > > you would rather prefer not working "touchpad" attributes to not > > exposed "touchpad" attributes that would have been perfectly working. > > > > > I'm guessing that this part: > > > > > > unsigned char param; > > > /* > > > * Some IdeaPads don't really turn off touchpad - they only > > > * switch the LED state. We (de)activate KBC AUX port to turn > > > * touchpad off and on. We send KEY_TOUCHPAD_OFF and > > > * KEY_TOUCHPAD_ON to not to get out of sync with LED > > > */ > > > i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE); > > > > > > May cause issues on some models. It definitely feels fishy and > > > I would like to disable this except on models where: > > > > > > 1. There is a LED controlled by some touchpad on/off hotkey; and > > > 2. The EC fails to disable the touchpad itself > > > > > > Which would currently mean only enable this bit on Maxim's Z570 > > > using a DMI based allow list. > > 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. And another idea: i8042_command(I8042_CMD_AUX_DISABLE) can be replaced with installing a i8042_platform_filter that will filter out all I8042_STR_AUXDATA when the touchpad should be disabled. It's definitely less fishy, we don't touch the KBC, but simply filter out events in software. > > > Agreed, but do you mean "and" or "or"? I mean we can also include the > > models that toggle touchpad value while not really toggling the touchpad > > (just as you described below) and don't have a touchpad LED (but I don't > > know if such model exists really), this way they won't go out of sync > > regardless of is there a touchpad LED or not. > > > > > At least on Maxim's Z570 the laptop does toggle the value > > > returned by VPCCMD_R_TOUCHPAD and the LED it self, while at the > > > same time not actually disabling the touchpad. > > > The problem is this all relies on being able to detect i2c vs ps/2 > > > touchpads which is not as simple as it sounds. Specifically many > > > new touchpad are connected to both busses at the same time, offering > > > a ps/2 mode by default for compatibility with older software / os-es > > > and being able to switch to a modern i2c/smbus mode for better performance. > > > > > > I've asked Benjamin Tissoires, the kernel expert on this about this > > > and his answer was that it is almost impossible to determine if > > > a touchpad is going to be using ps/2 or i2c without first waiting > > > for the whole driver stack to have initialized and then see which > > > driver(s) are attached and I guess even then the touchpad might > > > show up as both ps/2 + i2c with only one of them actually generating > > > events: > > > > > > https://lore.kernel.org/linux-input/ae50236e-1ce8-b526-9c17-7bc0463ebb86@xxxxxxxxxx/T/#u > > > > > > So based on Benjamin's answer I'm afraid that trying to differentiate > > > between i2c vs ps2 is not really doable. > > > > Thanks for the explanation and the link, but as Benjamin said, I believe > > we can use ACPI table for detecting PS/2 devices. I believe the DSDTs > > with PS2M(and probably MSS[0-9] too) devices probably have PS/2 touchpad, > > and have working EC and i8042 commands. Yet this still needs > > confirmation/testing, and I think should be resorted if your suggestion > > below won't work - your suggestion looks better and easier. > > > > > 1: Fixing undesirable i8042_command(... I8042_CMD_AUX_DISABLE) calls > > > -------------------------------------------------------------------- > > > > > > My suggestion is to move to an allow-list for this and for now > > > populate that list with only the DMI strings for Maxim's Z570 and see > > > from there. > > > > Agreed. > > > > > > > > 2: Fixing wrong/spurious KEY_TOUCHPAD_OFF / KEY_TOUCHPAD_ON events > > > ------------------------------------------------------------------ > > > > > > There are 2 subcases here: > > > > > > 2.1: Fix sending of KEY_TOUCHPAD_OFF/_ON events at probe/resume > > > --------------------------------------------------------------- > > > > > > We can simply fix this by giving ideapad_sync_touchpad_state() > > > a parameter to let it know if events should be send at all and > > > set that parameter to false when called on probe/resume > > > > Agreed. > > > > > 2.2: Sending wrong KEY_TOUCHPAD_OFF/_ON events at toggle time > > > ------------------------------------------------------------- > > > > > > On models where the EC does not control the touchpad at all, > > > currently we still do ideapad_sync_touchpad_state() and then > > > send either KEY_TOUCHPAD_OFF or _ON based on the value read > > > from VPCCMD_R_TOUCHPAD. > > > > > > But on these models VPCCMD_R_TOUCHPAD always returns 0 or 1, > > > so we always send KEY_TOUCHPAD_OFF or always send KEY_TOUCHPAD_ON, > > > instead of toggling the state / asking userspace to toggle > > > its sw touchpad on/off state. > > > > > > I believe we can detect this case by checking that > > > the value read from VPCCMD_R_TOUCHPAD has not changed > > > despite us receiving a notify with bit 5 being set in > > > the value read from VPCCMD_R_VPC1. > > > > > > My suggestion to fix this case is to detect when the value > > > read from VPCCMD_R_TOUCHPAD does not change and in that > > > case send KEY_TOUCHPAD_TOGGLE to userspace. > > > > While this is an awesome idea, what about doing this at boot? > > Like we will send 0 first, then check if it reads 0, then send 1, > > and confirm if it reads 1. This would be the ultimate solution, and > > would also fix my "cosmetic" concerns :) > > > > Best, > > Eray
