| Message ID | 1465230324-18035-1-git-send-email-jprvita@endlessm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Mon, Jun 06, 2016 at 12:25:24PM -0400, João Paulo Rechi Vita wrote: > Don't try to control the touchpad on laptops that do not support it, as > it leads to sending wrong KEY_TOUCHPAD_ON / KEY_TOUCHPAD_OFF events to > userspace. Other than some minor comment clarity/grammar, this looks like a reasonable approach to get the new models working without risking breakage to the older models. I'll correct. Maxim, Ike? OK with this approach? > > Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> > --- > drivers/platform/x86/ideapad-laptop.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index 06a837a..6270948 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -93,6 +93,7 @@ struct ideapad_private { > struct dentry *debug; > unsigned long cfg; > bool has_hw_rfkill_switch; > + bool has_tp_ctrl; > }; > > static bool no_bt_rfkill; > @@ -773,6 +774,9 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv) > { > unsigned long value; > > + if (!priv->has_tp_ctrl) > + 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 > @@ -930,6 +934,27 @@ static const struct dmi_system_id no_hw_rfkill_list[] = { > {} > }; > > +/* > + * Avoid sending wrong KEY_TOUCHPAD_ON / KEY_TOUCHPAD_OFF events to userspace. > + */ > +static const struct dmi_system_id no_tp_ctrl_list[] = { > + { > + .ident = "Lenovo 80HE", /* Yoga 3 Pro 2 */ > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > + DMI_MATCH(DMI_PRODUCT_NAME, "80HE"), > + }, > + }, > + { > + .ident = "Lenovo 80MK", /* Yoga 900 */ > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > + DMI_MATCH(DMI_PRODUCT_NAME, "80MK"), > + }, > + }, > + {} > +}; > + > static int ideapad_acpi_add(struct platform_device *pdev) > { > int ret, i; > @@ -953,6 +978,7 @@ static int ideapad_acpi_add(struct platform_device *pdev) > priv->adev = adev; > priv->platform_device = pdev; > priv->has_hw_rfkill_switch = !dmi_check_system(no_hw_rfkill_list); > + priv->has_tp_ctrl = !dmi_check_system(no_tp_ctrl_list); > > ret = ideapad_sysfs_init(priv); > if (ret) > -- > 2.5.0 > >
On Mon, Jun 06, 2016 at 12:25:24PM -0400, João Paulo Rechi Vita wrote: > Don't try to control the touchpad on laptops that do not support it, as > it leads to sending wrong KEY_TOUCHPAD_ON / KEY_TOUCHPAD_OFF events to > userspace. > > Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> > --- > drivers/platform/x86/ideapad-laptop.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index 06a837a..6270948 100644 > --- a/drivers/platform/x86/ideapad-laptop.c > +++ b/drivers/platform/x86/ideapad-laptop.c > @@ -93,6 +93,7 @@ struct ideapad_private { > struct dentry *debug; > unsigned long cfg; > bool has_hw_rfkill_switch; > + bool has_tp_ctrl; > }; > > static bool no_bt_rfkill; > @@ -773,6 +774,9 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv) > { > unsigned long value; > > + if (!priv->has_tp_ctrl) > + 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 > @@ -930,6 +934,27 @@ static const struct dmi_system_id no_hw_rfkill_list[] = { > {} > }; > > +/* > + * Avoid sending wrong KEY_TOUCHPAD_ON / KEY_TOUCHPAD_OFF events to userspace. > + */ > +static const struct dmi_system_id no_tp_ctrl_list[] = { > + { > + .ident = "Lenovo 80HE", /* Yoga 3 Pro 2 */ > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > + DMI_MATCH(DMI_PRODUCT_NAME, "80HE"), > + }, > + }, > + { > + .ident = "Lenovo 80MK", /* Yoga 900 */ > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > + DMI_MATCH(DMI_PRODUCT_NAME, "80MK"), > + }, Upon closer inspection, is there a reason you chose to match on PRODUCT_NAME instead of PRODUCT_VERSION like the no_hw_rfkill_list? The PRODUCT_VERSION tends to have the user recognizable name which you placed in a comment instead of in the .ident. For both consistency and legibility, it seems like it would be better to match on DMI_PRODUCT_VERSION and use that in the .ident.
2016-06-09 1:07 GMT+03:00 Darren Hart <dvhart@infradead.org>: > On Mon, Jun 06, 2016 at 12:25:24PM -0400, João Paulo Rechi Vita wrote: >> Don't try to control the touchpad on laptops that do not support it, as >> it leads to sending wrong KEY_TOUCHPAD_ON / KEY_TOUCHPAD_OFF events to >> userspace. > > Other than some minor comment clarity/grammar, this looks like a reasonable > approach to get the new models working without risking breakage to the older > models. I'll correct. > > Maxim, Ike? OK with this approach? Looks good for me. >> >> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> >> --- >> drivers/platform/x86/ideapad-laptop.c | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c >> index 06a837a..6270948 100644 >> --- a/drivers/platform/x86/ideapad-laptop.c >> +++ b/drivers/platform/x86/ideapad-laptop.c >> @@ -93,6 +93,7 @@ struct ideapad_private { >> struct dentry *debug; >> unsigned long cfg; >> bool has_hw_rfkill_switch; >> + bool has_tp_ctrl; >> }; >> >> static bool no_bt_rfkill; >> @@ -773,6 +774,9 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv) >> { >> unsigned long value; >> >> + if (!priv->has_tp_ctrl) >> + 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 >> @@ -930,6 +934,27 @@ static const struct dmi_system_id no_hw_rfkill_list[] = { >> {} >> }; >> >> +/* >> + * Avoid sending wrong KEY_TOUCHPAD_ON / KEY_TOUCHPAD_OFF events to userspace. >> + */ >> +static const struct dmi_system_id no_tp_ctrl_list[] = { >> + { >> + .ident = "Lenovo 80HE", /* Yoga 3 Pro 2 */ >> + .matches = { >> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "80HE"), >> + }, >> + }, >> + { >> + .ident = "Lenovo 80MK", /* Yoga 900 */ >> + .matches = { >> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "80MK"), >> + }, >> + }, >> + {} >> +}; >> + >> static int ideapad_acpi_add(struct platform_device *pdev) >> { >> int ret, i; >> @@ -953,6 +978,7 @@ static int ideapad_acpi_add(struct platform_device *pdev) >> priv->adev = adev; >> priv->platform_device = pdev; >> priv->has_hw_rfkill_switch = !dmi_check_system(no_hw_rfkill_list); >> + priv->has_tp_ctrl = !dmi_check_system(no_tp_ctrl_list); >> >> ret = ideapad_sysfs_init(priv); >> if (ret) >> -- >> 2.5.0 >> >> > > -- > Darren Hart > Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8 June 2016 at 18:15, Darren Hart <dvhart@infradead.org> wrote: > On Mon, Jun 06, 2016 at 12:25:24PM -0400, João Paulo Rechi Vita wrote: >> Don't try to control the touchpad on laptops that do not support it, as >> it leads to sending wrong KEY_TOUCHPAD_ON / KEY_TOUCHPAD_OFF events to >> userspace. >> >> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> >> --- >> drivers/platform/x86/ideapad-laptop.c | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c >> index 06a837a..6270948 100644 >> --- a/drivers/platform/x86/ideapad-laptop.c >> +++ b/drivers/platform/x86/ideapad-laptop.c >> @@ -93,6 +93,7 @@ struct ideapad_private { >> struct dentry *debug; >> unsigned long cfg; >> bool has_hw_rfkill_switch; >> + bool has_tp_ctrl; >> }; >> >> static bool no_bt_rfkill; >> @@ -773,6 +774,9 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv) >> { >> unsigned long value; >> >> + if (!priv->has_tp_ctrl) >> + 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 >> @@ -930,6 +934,27 @@ static const struct dmi_system_id no_hw_rfkill_list[] = { >> {} >> }; >> >> +/* >> + * Avoid sending wrong KEY_TOUCHPAD_ON / KEY_TOUCHPAD_OFF events to userspace. >> + */ >> +static const struct dmi_system_id no_tp_ctrl_list[] = { >> + { >> + .ident = "Lenovo 80HE", /* Yoga 3 Pro 2 */ >> + .matches = { >> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "80HE"), >> + }, >> + }, >> + { >> + .ident = "Lenovo 80MK", /* Yoga 900 */ >> + .matches = { >> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "80MK"), >> + }, > > Upon closer inspection, is there a reason you chose to match on PRODUCT_NAME > instead of PRODUCT_VERSION like the no_hw_rfkill_list? The PRODUCT_VERSION tends > to have the user recognizable name which you placed in a comment instead of in > the .ident. For both consistency and legibility, it seems like it would be better to > match on DMI_PRODUCT_VERSION and use that in the .ident. > The reason I chose PRODUCT_NAME is because on these laptops this seems to more specifically identify the machine. 80MK is the model name I see on the machine's box and at the website at the time. Now the website shows 80UE. So maybe this is a way to avoid the kind of clashing that led to reverting the previous quirk by Hans? If that does not make much sense I can change it to match against PRODUCT_VERSION, which on my Yoga 900 shows "Lenovo YOGA 900-13ISK". Also, let me know if you have a explicit suggestion to make the commit message clearer, or feel free to fix it when you pick the patch. -- João Paulo Rechi Vita http://about.me/jprvita -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c index 06a837a..6270948 100644 --- a/drivers/platform/x86/ideapad-laptop.c +++ b/drivers/platform/x86/ideapad-laptop.c @@ -93,6 +93,7 @@ struct ideapad_private { struct dentry *debug; unsigned long cfg; bool has_hw_rfkill_switch; + bool has_tp_ctrl; }; static bool no_bt_rfkill; @@ -773,6 +774,9 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv) { unsigned long value; + if (!priv->has_tp_ctrl) + 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 @@ -930,6 +934,27 @@ static const struct dmi_system_id no_hw_rfkill_list[] = { {} }; +/* + * Avoid sending wrong KEY_TOUCHPAD_ON / KEY_TOUCHPAD_OFF events to userspace. + */ +static const struct dmi_system_id no_tp_ctrl_list[] = { + { + .ident = "Lenovo 80HE", /* Yoga 3 Pro 2 */ + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), + DMI_MATCH(DMI_PRODUCT_NAME, "80HE"), + }, + }, + { + .ident = "Lenovo 80MK", /* Yoga 900 */ + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), + DMI_MATCH(DMI_PRODUCT_NAME, "80MK"), + }, + }, + {} +}; + static int ideapad_acpi_add(struct platform_device *pdev) { int ret, i; @@ -953,6 +978,7 @@ static int ideapad_acpi_add(struct platform_device *pdev) priv->adev = adev; priv->platform_device = pdev; priv->has_hw_rfkill_switch = !dmi_check_system(no_hw_rfkill_list); + priv->has_tp_ctrl = !dmi_check_system(no_tp_ctrl_list); ret = ideapad_sysfs_init(priv); if (ret)
Don't try to control the touchpad on laptops that do not support it, as it leads to sending wrong KEY_TOUCHPAD_ON / KEY_TOUCHPAD_OFF events to userspace. Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> --- drivers/platform/x86/ideapad-laptop.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)