On Mon, 21 Nov 2022 at 12:07, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi Максим,
>
> On 11/20/22 16:29, Максим wrote:
> > On Thu, 17 Nov 2022 at 13:03, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Recently there have been multiple patches to disable the ideapad-laptop's
> >> touchpad control code, because it is causing issues on various laptops:
> >>
> >> Commit d69cd7eea93e ("platform/x86: ideapad-laptop: Disable touchpad_switch for ELAN0634")
> >> Commit a231224a601c ("platform/x86: ideapad-laptop: Disable touchpad_switch")
> >>
> >> The turning on/off of the ps2 aux port was added specifically for
> >> the IdeaPad Z570, where the EC does toggle the touchpad on/off LED and
> >> toggles the value returned by reading VPCCMD_R_TOUCHPAD, but it does not
> >> actually turn on/off the touchpad.
> >>
> >> The ideapad-laptop code really should not be messing with the i8042
> >> controller on all devices just for this special case.
> >>
> >> Add a new ctrl_ps2_aux_port flag set based on a DMI based allow-list
> >> for devices which need this workaround, populating it with just
> >> the Ideapad Z570 for now.
> >>
> >> This also adds a module parameter so that this behavior can easily
> >> be enabled on other models which may need it.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >
> > On Z570:
> >
> > Tested-by: Maxim Mikityanskiy <maxtram95@xxxxxxxxx>
>
> Thank you for testing!
>
> > A few notes for Z570:
> >
> > 1. Touchpad toggle still works properly after this series of patches.
> >
> > 2. My laptop's EC reenables the touchpad on boot and on resume, and
> > this behavior still works after this series.
> >
> > 3. Patch 2 stops sending "spurious" key press events on resume, which
> > actually make sense on my laptop, when the touchpad is reenabled on
> > resume. Maybe we should send a key press, but only if the state
> > changed? (However, for some reason I don't see the OSD for this even
> > before this series.)
>
> I'm not sure if sending the key-press and thus showing the OSd makes
> sense in resume even if the state did change.
On one hand, if the state changes, it makes sense to send an event. On
the other hand, there was no real key press, so we shouldn't send
KEY_TOUCHPAD_ON, a key press event. I'm not sure which of these
behaviors is standard, so it's up to you to decide.
> The OSD typically is
> used to confirm an action by the user. If the touchpad is always
> re-enabled after resume then the user will get uses to that quick enough
> and showing an OSD for this will just be a distraction IMHO.
For some reason, I don't see the OSD on resume, even if the driver
sends KEY_TOUCHPAD_ON.
>
> > 4. The sysfs attribute for touchpad doesn't exist on my laptop, but it
> > would still make sense if we made it read only. Right now there is a
> > module parameter to force enable this sysfs attribute, but it's
> > created as read-write, and writes are no-op.
>
> Hmm, since the sysfs attr is normally not there at all (now) and
> since the module option help text says:
> "This may not work on all models."
>
> I think that having writing to the sysfs file succeed but not do anything
> is not really a problem.
>
> I guess we could change the sysfs attr visibility to 444 on models
> with priv->features.ctrl_ps2_aux_port set, but that seems making
> the code unnecessarily complicated given that the sysfs attr is hidden
> by default now anyways.
The initial default was to show the sysfs attribute everywhere, not
sure why it was hidden afterwards...
BTW, I forgot to mention that I found out that there is the
conservation_mode sysfs attribute that shows on Z570, but it's a
no-op. Probably the following condition is not enough to discover this
feature reliably:
if (acpi_has_method(handle, "GBMD") && acpi_has_method(handle, "SBMC"))
priv->features.conservation_mode = true;
> Regards,
>
> Hans
>
>
>
>
> >
> >> ---
> >> drivers/platform/x86/ideapad-laptop.c | 29 ++++++++++++++++++++++++++-
> >> 1 file changed, 28 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> >> index eb0b1ec32c13..1d86fb988d56 100644
> >> --- a/drivers/platform/x86/ideapad-laptop.c
> >> +++ b/drivers/platform/x86/ideapad-laptop.c
> >> @@ -143,6 +143,7 @@ struct ideapad_private {
> >> bool hw_rfkill_switch : 1;
> >> bool kbd_bl : 1;
> >> bool touchpad_ctrl_via_ec : 1;
> >> + bool ctrl_ps2_aux_port : 1;
> >> bool usb_charging : 1;
> >> } features;
> >> struct {
> >> @@ -174,6 +175,12 @@ MODULE_PARM_DESC(set_fn_lock_led,
> >> "Enable driver based updates of the fn-lock LED on fn-lock changes. "
> >> "If you need this please report this to: platform-driver-x86@xxxxxxxxxxxxxxx");
> >>
> >> +static bool ctrl_ps2_aux_port;
> >> +module_param(ctrl_ps2_aux_port, bool, 0444);
> >> +MODULE_PARM_DESC(ctrl_ps2_aux_port,
> >> + "Enable driver based PS/2 aux port en-/dis-abling on touchpad on/off toggle. "
> >> + "If you need this please report this to: platform-driver-x86@xxxxxxxxxxxxxxx");
> >> +
> >> /*
> >> * shared data
> >> */
> >> @@ -1507,7 +1514,8 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
> >> * 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);
> >> + if (priv->features.ctrl_ps2_aux_port)
> >> + i8042_command(¶m, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE);
> >>
> >> if (send_events) {
> >> ideapad_input_report(priv, value ? 67 : 66);
> >> @@ -1615,6 +1623,23 @@ static const struct dmi_system_id hw_rfkill_list[] = {
> >> {}
> >> };
> >>
> >> +/*
> >> + * On some models the EC toggles the touchpad muted LED on touchpad toggle
> >> + * hotkey presses, but the EC does not actually disable the touchpad itself.
> >> + * On these models the driver needs to explicitly enable/disable the i8042
> >> + * (PS/2) aux port.
> >> + */
> >> +static const struct dmi_system_id ctrl_ps2_aux_port_list[] = {
> >> + {
> >> + /* Lenovo Ideapad Z570 */
> >> + .matches = {
> >> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> >> + DMI_MATCH(DMI_PRODUCT_VERSION, "Ideapad Z570"),
> >> + },
> >> + },
> >> + {}
> >> +};
> >> +
> >> static const struct dmi_system_id no_touchpad_switch_list[] = {
> >> {
> >> .ident = "Lenovo Yoga 3 Pro 1370",
> >> @@ -1642,6 +1667,8 @@ static void ideapad_check_features(struct ideapad_private *priv)
> >> set_fn_lock_led || dmi_check_system(set_fn_lock_led_list);
> >> priv->features.hw_rfkill_switch =
> >> hw_rfkill_switch || dmi_check_system(hw_rfkill_list);
> >> + priv->features.ctrl_ps2_aux_port =
> >> + ctrl_ps2_aux_port || dmi_check_system(ctrl_ps2_aux_port_list);
> >>
> >> /* Most ideapads with ELAN0634 touchpad don't use EC touchpad switch */
> >> if (acpi_dev_present("ELAN0634", NULL, -1))
> >> --
> >> 2.38.1
> >>
> >
>
