Hi Eray, On 11/16/22 17:08, Eray Orçunus wrote: > Hi, > > On Thu, 16 Nov 2022 at 18:25, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >> Hi All, >> >> Here are my proposed changes from the "ideapad-laptop touchpad handling >> problems, request for help" email thread as proper patches: >> https://lore.kernel.org/platform-driver-x86/bc1202d1-d85d-4173-5979-237bb1ee9254@xxxxxxxxxx/ >> >> Note this applies on top of my review-hans branch which has seen a bunch >> of other ideapad-laptop changes land recently: >> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans >> >> As suggested by Maxim, the third patch now has a module parameter to >> allow users to easily re-enable the i8042 aux-port enabling/disabling >> on models other then the Z570. >> >> Eray, you mentioned in another email that you have some concerns about >> the approach in this series? > > Yes, thanks for mentioning. My concerns are these: > > - Users of laptops with ELAN0634 (like Yoga 14s and 720s), Lenovo > Yoga 3 Pro 1370 and ZhaoYang K4e-IML will start to see non-working > "touchpad" sysfs attribute on their ideapad-laptop driver. I see this > as a regression. Also it's easy to fix; we can just test for if > VPCCMD_W_TOUCHPAD works at the boot - with sending 0 first and verify the > VPCCMD_R_TOUCHPAD result, and then sending 1 and again verify the > VPCCMD_R_TOUCHPAD result. Later we can remove "touchpad" attribute if it > doesn't work, I'm not 100% sure if this will work everywhere and I'm a bit worried the VPCCMD_W_TOUCHPAD calls might cause problems on some devices. You did mention that the ideapad windows software seems to always make VPCCMD_W_TOUCHPAD calls. But still, one of my goals with this series is to have the ideapad-laptop driver poke less at the hw, to avoid the poking potentially causes issues. Where as your suggested auto-detection makes the driver poke the hw more, not less. Thinking more about this makes me wonder: "why not just entirely remove the whole touchpad sysfs attribute?" Just removing the entire touchpad sysfs attribute will greatly simplify things and normal userspace does not depend on it at all. Its only potential users are custom user scripts and ideapad specific control panels or some such. And the custom control panels already need to deal with the touchpad sysfs attribute not always being there. I do realize that removing it might be a bit too big of a hammer, so instead I plan to have it hidden by default and allow enabling it through a module parameter. Serr the next version of this patch-set (coming up soon). Gating this with a module parameter will reduce all the maintenance burden of having allow-lists are fragile auto-detect code for a feature which I believe hardly anyone uses at all. > with the exception of devices with ctrl_ps2_aux_port, > since these laptops have working VPCCMD_R_TOUCHPAD. I don't agree with having the touchpad sysfs attr visible on the ctrl_ps2_aux_port devices, writing it will do nothing and as you mentioned yourself some ideapad specific tools will show a touchpad on/off option when this file is visible, which then will not work. Note that just hiding the touchpad sysfs attr be default makes this whole discussion mute though. > - You removed sending 1 to VPCCMD_W_TOUCHPAD at the boot, are we sure > there are no laptops needing that? I don't think we talked about that > in previous e-mail thread. This was only added recently (until recently there was no touchpad_ctrl_via_ec flag at all) and seems to be copied and pasted from the rfkill code, where we know that manually disabling the hw-rfkill bit is actually needed on some models without an actual hw-rfkill switch... > - There is no i8042 cmd on touchpad_store, which may make the function > ineffective on laptops with ctrl_ps2_aux_port. This is not a problem introduced by this patch-set, touchpad-store never controlled the ps2-aux port. As mentioned above IMHO just hiding the touchpad sysfs attr here is better (avoids the need to make touch-store have a special case for this). These devices have a working touchpad-toggle hotkey + a LED to indicate the status, so there really is no need for the sysfs attr. (which makes me realize there is really little need for the sysfs attr at all, so just always hiding it is easier, avoiding all sorts of possible issues, see above). > - r_touchpad_val isn't set at touchpad_store and/or touchpad_read, which > can make it out of sync when "touchpad" attr is accessed. I've fixed this in the next version of my patch set. Regards, Hans
