| Message ID | 20210101061140.27547-1-jiaxun.yang@flygoat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: ideapad-laptop: Add has_touchpad_switch | expand |
Hi 2021. január 1., péntek 7:11 keltezéssel, Jiaxun Yang írta: > Newer ideapads (e.g.: Yoga 14s, 720S 14) comes with I2C HID > Touchpad and do not use EC to switch touchpad. Reading VPCCMD_R_TOUCHPAD > will return zero thus touchpad may be blocked. Writing VPCCMD_W_TOUCHPAD > may cause a spurious key press. > > Add has_touchpad_switch to workaround these machines. > > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > Cc: stable@vger.kernel.org # 5.4+ Interestingly, the Lenovo Yoga 540-14IKB 80X8 has an HID-over-I2C touchpad, and yet it can be controlled by reading/writing the appropriate EC registers. > --- > drivers/platform/x86/ideapad-laptop.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index 7598cd46cf60..b6a4db37d0fc 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -92,6 +92,7 @@ struct ideapad_private { > struct dentry *debug; > unsigned long cfg; > bool has_hw_rfkill_switch; > + bool has_touchpad_switch; > const char *fnesc_guid; > }; > > @@ -535,7 +536,9 @@ static umode_t ideapad_is_visible(struct kobject *kobj, > } else if (attr == &dev_attr_fn_lock.attr) { > supported = acpi_has_method(priv->adev->handle, "HALS") && > acpi_has_method(priv->adev->handle, "SALS"); > - } else > + } else if (attr == &dev_attr_touchpad.attr) > + supported = priv->has_touchpad_switch; > + else > supported = true; > > return supported ? attr->mode : 0; > @@ -867,6 +870,9 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv) > { > unsigned long value; > > + if (!priv->has_touchpad_switch) > + return; > + > /* Without reading from EC touchpad LED doesn't switch state */ > if (!read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &value)) { > /* Some IdeaPads don't really turn off touchpad - they only > @@ -989,6 +995,12 @@ static int ideapad_acpi_add(struct platform_device *pdev) > priv->platform_device = pdev; > priv->has_hw_rfkill_switch = dmi_check_system(hw_rfkill_list); > > + /* Most ideapads with I2C HID don't use EC touchpad switch */ > + if (acpi_dev_present("PNP0C50", NULL, -1)) > + priv->has_touchpad_switch = false; > + else > + priv->has_touchpad_switch = true; > + `priv->has_touchpad_switch = !acpi_dev_present(...)` ? > ret = ideapad_sysfs_init(priv); > if (ret) > return ret; > @@ -1006,6 +1018,10 @@ static int ideapad_acpi_add(struct platform_device *pdev) > if (!priv->has_hw_rfkill_switch) > write_ec_cmd(priv->adev->handle, VPCCMD_W_RF, 1); > > + /* The same for Touchpad */ > + if (!priv->has_touchpad_switch) > + write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, 1); > + Shouldn't it be the other way around: `if (priv->has_touchpad_switch)`? > for (i = 0; i < IDEAPAD_RFKILL_DEV_NUM; i++) > if (test_bit(ideapad_rfk_data[i].cfgbit, &priv->cfg)) > ideapad_register_rfkill(priv, i); > -- > 2.30.0 Regards, Barnabás Pőcze
On Fri, Jan 1, 2021, at 10:20 PM, Barnabás Pőcze wrote: > Hi > > > 2021. január 1., péntek 7:11 keltezéssel, Jiaxun Yang írta: > > > Newer ideapads (e.g.: Yoga 14s, 720S 14) comes with I2C HID > > Touchpad and do not use EC to switch touchpad. Reading VPCCMD_R_TOUCHPAD > > will return zero thus touchpad may be blocked. Writing VPCCMD_W_TOUCHPAD > > may cause a spurious key press. > > > > Add has_touchpad_switch to workaround these machines. > > > > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > > Cc: stable@vger.kernel.org # 5.4+ > > Interestingly, the Lenovo Yoga 540-14IKB 80X8 has an HID-over-I2C touchpad, > and yet it can be controlled by reading/writing the appropriate EC registers. > > > > --- > > drivers/platform/x86/ideapad-laptop.c | 18 +++++++++++++++++- > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > > index 7598cd46cf60..b6a4db37d0fc 100644 > > --- a/drivers/platform/x86/ideapad-laptop.c > > +++ b/drivers/platform/x86/ideapad-laptop.c > > @@ -92,6 +92,7 @@ struct ideapad_private { > > struct dentry *debug; > > unsigned long cfg; > > bool has_hw_rfkill_switch; > > + bool has_touchpad_switch; > > const char *fnesc_guid; > > }; > > > > @@ -535,7 +536,9 @@ static umode_t ideapad_is_visible(struct kobject *kobj, > > } else if (attr == &dev_attr_fn_lock.attr) { > > supported = acpi_has_method(priv->adev->handle, "HALS") && > > acpi_has_method(priv->adev->handle, "SALS"); > > - } else > > + } else if (attr == &dev_attr_touchpad.attr) > > + supported = priv->has_touchpad_switch; > > + else > > supported = true; > > > > return supported ? attr->mode : 0; > > @@ -867,6 +870,9 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv) > > { > > unsigned long value; > > > > + if (!priv->has_touchpad_switch) > > + return; > > + > > /* Without reading from EC touchpad LED doesn't switch state */ > > if (!read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &value)) { > > /* Some IdeaPads don't really turn off touchpad - they only > > @@ -989,6 +995,12 @@ static int ideapad_acpi_add(struct platform_device *pdev) > > priv->platform_device = pdev; > > priv->has_hw_rfkill_switch = dmi_check_system(hw_rfkill_list); > > > > + /* Most ideapads with I2C HID don't use EC touchpad switch */ > > + if (acpi_dev_present("PNP0C50", NULL, -1)) > > + priv->has_touchpad_switch = false; > > + else > > + priv->has_touchpad_switch = true; > > + > > `priv->has_touchpad_switch = !acpi_dev_present(...)` > ? Will fix. > > > > ret = ideapad_sysfs_init(priv); > > if (ret) > > return ret; > > @@ -1006,6 +1018,10 @@ static int ideapad_acpi_add(struct platform_device *pdev) > > if (!priv->has_hw_rfkill_switch) > > write_ec_cmd(priv->adev->handle, VPCCMD_W_RF, 1); > > > > + /* The same for Touchpad */ > > + if (!priv->has_touchpad_switch) > > + write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, 1); > > + > > Shouldn't it be the other way around: `if (priv->has_touchpad_switch)`? It is to prevent accidentally disable touchpad on machines that do have EC switch, so it's intentional. Thanks. - Jiaxun > > > > > for (i = 0; i < IDEAPAD_RFKILL_DEV_NUM; i++) > > if (test_bit(ideapad_rfk_data[i].cfgbit, &priv->cfg)) > > ideapad_register_rfkill(priv, i); > > -- > > 2.30.0 > > > Regards, > Barnabás Pőcze >
Hi 2021. január 1., péntek 17:08 keltezéssel, Jiaxun Yang írta: > [...] > > > @@ -1006,6 +1018,10 @@ static int ideapad_acpi_add(struct platform_device *pdev) > > > if (!priv->has_hw_rfkill_switch) > > > write_ec_cmd(priv->adev->handle, VPCCMD_W_RF, 1); > > > > > > + /* The same for Touchpad */ > > > + if (!priv->has_touchpad_switch) > > > + write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, 1); > > > + > > > > Shouldn't it be the other way around: `if (priv->has_touchpad_switch)`? > > It is to prevent accidentally disable touchpad on machines that do have EC switch, > so it's intentional. > [...] Sorry, but the explanation not fully clear to me. The commit message seems to indicate that some models "do not use EC to switch touchpad", and I take that means that reading from VPCCMD_R_TOUCHPAD will not reflect the actual state of the touchpad and writing to VPCCMD_W_TOUCHPAD will not change the state of the touchpad. But then why do you still write to VPCCMD_W_TOUCHPAD on devices where supposedly this does not have any effect (at least not the desired one)? And the part of the code I made my comment about only runs on machines on which the touchpad supposedly cannot be controlled by the EC. What am I missing? And there is the other problem: on some machines, this patch removes working functionality. Regards, Barnabás Pőcze
在 2021/1/2 上午1:09, Barnabás Pőcze 写道: > Hi > > > 2021. január 1., péntek 17:08 keltezéssel, Jiaxun Yang írta: > >> [...] >>>> @@ -1006,6 +1018,10 @@ static int ideapad_acpi_add(struct platform_device *pdev) >>>> if (!priv->has_hw_rfkill_switch) >>>> write_ec_cmd(priv->adev->handle, VPCCMD_W_RF, 1); >>>> >>>> + /* The same for Touchpad */ >>>> + if (!priv->has_touchpad_switch) >>>> + write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, 1); >>>> + >>> Shouldn't it be the other way around: `if (priv->has_touchpad_switch)`? >> It is to prevent accidentally disable touchpad on machines that do have EC switch, >> so it's intentional. >> [...] > Sorry, but the explanation not fully clear to me. The commit message seems to > indicate that some models "do not use EC to switch touchpad", and I take that > means that reading from VPCCMD_R_TOUCHPAD will not reflect the actual state of the > touchpad and writing to VPCCMD_W_TOUCHPAD will not change the state of the touchpad. I'm just trying to prevent removing functionality on machines that touchpad can be controlled by EC but also equipped I2C HID touchpad. At least users will have a functional touchpad after that. > > But then why do you still write to VPCCMD_W_TOUCHPAD on devices where supposedly > this does not have any effect (at least not the desired one)? And the part of the > code I made my comment about only runs on machines on which the touchpad supposedly > cannot be controlled by the EC. What am I missing? > > And there is the other problem: on some machines, this patch removes working > functionality. Yeah that's a problem. I just don't want to repeat the story of rfkill whitelist, it ends up with countless machine to be added. Maybe I should specify HID of touchpad as well. Two machines that known to be problematic all have ELAN0634 touchpad. Thanks. - Jiaxun > > > Regards, > Barnabás Pőcze
2021. január 2., szombat 3:36 keltezéssel, Jiaxun Yang írta: > 在 2021/1/2 上午1:09, Barnabás Pőcze 写道: > > Hi > > > > > > 2021. január 1., péntek 17:08 keltezéssel, Jiaxun Yang írta: > > > >> [...] > >>>> @@ -1006,6 +1018,10 @@ static int ideapad_acpi_add(struct platform_device *pdev) > >>>> if (!priv->has_hw_rfkill_switch) > >>>> write_ec_cmd(priv->adev->handle, VPCCMD_W_RF, 1); > >>>> > >>>> + /* The same for Touchpad */ > >>>> + if (!priv->has_touchpad_switch) > >>>> + write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, 1); > >>>> + > >>> Shouldn't it be the other way around: `if (priv->has_touchpad_switch)`? > >> It is to prevent accidentally disable touchpad on machines that do have EC switch, > >> so it's intentional. > >> [...] > > Sorry, but the explanation not fully clear to me. The commit message seems to > > indicate that some models "do not use EC to switch touchpad", and I take that > > means that reading from VPCCMD_R_TOUCHPAD will not reflect the actual state of the > > touchpad and writing to VPCCMD_W_TOUCHPAD will not change the state of the touchpad. > > I'm just trying to prevent removing functionality on machines that > touchpad can be controlled > by EC but also equipped I2C HID touchpad. At least users will have a > functional touchpad > after that. > Thanks for the clarification. > > > > But then why do you still write to VPCCMD_W_TOUCHPAD on devices where supposedly > > this does not have any effect (at least not the desired one)? And the part of the > > code I made my comment about only runs on machines on which the touchpad supposedly > > cannot be controlled by the EC. What am I missing? > > > > And there is the other problem: on some machines, this patch removes working > > functionality. > Yeah that's a problem. I just don't want to repeat the story of rfkill > whitelist, it ends up with > countless machine to be added. > > Maybe I should specify HID of touchpad as well. Two machines that known > to be problematic > all have ELAN0634 touchpad. I think that would be better since the Lenovo Yoga 520-14IKB 80X8 device I'm concerned about has a SYNA2B2C touchpad device, so at least that wouldn't be affected. Regards, Barnabás Pőcze
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c index 7598cd46cf60..b6a4db37d0fc 100644 --- a/drivers/platform/x86/ideapad-laptop.c +++ b/drivers/platform/x86/ideapad-laptop.c @@ -92,6 +92,7 @@ struct ideapad_private { struct dentry *debug; unsigned long cfg; bool has_hw_rfkill_switch; + bool has_touchpad_switch; const char *fnesc_guid; }; @@ -535,7 +536,9 @@ static umode_t ideapad_is_visible(struct kobject *kobj, } else if (attr == &dev_attr_fn_lock.attr) { supported = acpi_has_method(priv->adev->handle, "HALS") && acpi_has_method(priv->adev->handle, "SALS"); - } else + } else if (attr == &dev_attr_touchpad.attr) + supported = priv->has_touchpad_switch; + else supported = true; return supported ? attr->mode : 0; @@ -867,6 +870,9 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv) { unsigned long value; + if (!priv->has_touchpad_switch) + return; + /* Without reading from EC touchpad LED doesn't switch state */ if (!read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &value)) { /* Some IdeaPads don't really turn off touchpad - they only @@ -989,6 +995,12 @@ static int ideapad_acpi_add(struct platform_device *pdev) priv->platform_device = pdev; priv->has_hw_rfkill_switch = dmi_check_system(hw_rfkill_list); + /* Most ideapads with I2C HID don't use EC touchpad switch */ + if (acpi_dev_present("PNP0C50", NULL, -1)) + priv->has_touchpad_switch = false; + else + priv->has_touchpad_switch = true; + ret = ideapad_sysfs_init(priv); if (ret) return ret; @@ -1006,6 +1018,10 @@ static int ideapad_acpi_add(struct platform_device *pdev) if (!priv->has_hw_rfkill_switch) write_ec_cmd(priv->adev->handle, VPCCMD_W_RF, 1); + /* The same for Touchpad */ + if (!priv->has_touchpad_switch) + write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, 1); + for (i = 0; i < IDEAPAD_RFKILL_DEV_NUM; i++) if (test_bit(ideapad_rfk_data[i].cfgbit, &priv->cfg)) ideapad_register_rfkill(priv, i);
Newer ideapads (e.g.: Yoga 14s, 720S 14) comes with I2C HID Touchpad and do not use EC to switch touchpad. Reading VPCCMD_R_TOUCHPAD will return zero thus touchpad may be blocked. Writing VPCCMD_W_TOUCHPAD may cause a spurious key press. Add has_touchpad_switch to workaround these machines. Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> Cc: stable@vger.kernel.org # 5.4+ --- drivers/platform/x86/ideapad-laptop.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)