| Message ID | 20230310041726.217447-3-kallmeyeras@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: Add driver for Yoga Tablet mode switch | expand |
Am 10.03.23 um 05:17 schrieb Andrew Kallmeyer: > This WMI driver for the tablet mode control switch for Lenovo Yoga > notebooks was originally written by Gergo Koteles. The mode is mapped to > a SW_TABLET_MODE switch capable input device. > > I (Andrew) followed the suggestions that were posted in reply to Gergo's > RFC patch to follow-up and get it merged. Additionally I merged the > lenovo_ymc_trigger_ec function with the ideapad_trigger_ymc_next_read > function since it was no longer external. I have also added the word > "Tablet" to the driver description to hopefully make it more clear. > > As Gergo said: It should work with models like the Yoga C940, Ideapad > flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7. > The 14ARB7 model must trigger the EC to send mode change events. This > might be a firmware bug. > > I have additionally tested this on the Yoga 7 14IAL7. > > Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/ > Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com> > Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com> > --- > drivers/platform/x86/Kconfig | 10 ++ > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/ideapad-laptop.h | 1 + > drivers/platform/x86/lenovo-ymc.c | 177 ++++++++++++++++++++++++++ > 4 files changed, 189 insertions(+) > create mode 100644 drivers/platform/x86/lenovo-ymc.c > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 5692385e2..858be0c65 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -470,6 +470,16 @@ config IDEAPAD_LAPTOP > This is a driver for Lenovo IdeaPad netbooks contains drivers for > rfkill switch, hotkey, fan control and backlight control. > > +config LENOVO_YMC > + tristate "Lenovo Yoga Tablet Mode Control" > + depends on ACPI_WMI > + depends on INPUT > + depends on IDEAPAD_LAPTOP > + select INPUT_SPARSEKMAP > + help > + This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input > + events for Lenovo Yoga notebooks. > + > config SENSORS_HDAPS > tristate "Thinkpad Hard Drive Active Protection System (hdaps)" > depends on INPUT > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 1d3d1b025..10054cdea 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -63,6 +63,7 @@ obj-$(CONFIG_UV_SYSFS) += uv_sysfs.o > # IBM Thinkpad and Lenovo > obj-$(CONFIG_IBM_RTL) += ibm_rtl.o > obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o > +obj-$(CONFIG_LENOVO_YMC) += lenovo-ymc.o > obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o > obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o > obj-$(CONFIG_THINKPAD_LMI) += think-lmi.o > diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h > index 7dd8ce027..2564cb1cd 100644 > --- a/drivers/platform/x86/ideapad-laptop.h > +++ b/drivers/platform/x86/ideapad-laptop.h > @@ -35,6 +35,7 @@ enum { > VPCCMD_W_FAN, > VPCCMD_R_RF, > VPCCMD_W_RF, > + VPCCMD_W_YMC = 0x2A, > VPCCMD_R_FAN = 0x2B, > VPCCMD_R_SPECIAL_BUTTONS = 0x31, > VPCCMD_W_BL_POWER = 0x33, > diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c > new file mode 100644 > index 000000000..e29ada1b4 > --- /dev/null > +++ b/drivers/platform/x86/lenovo-ymc.c > @@ -0,0 +1,177 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * lenovo-ymc.c - Lenovo Yoga Mode Control driver > + * > + * Copyright © 2022 Gergo Koteles <soyer@irl.hu> > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/acpi.h> > +#include <linux/dmi.h> > +#include <linux/input.h> > +#include <linux/input/sparse-keymap.h> > +#include <linux/wmi.h> > +#include "ideapad-laptop.h" > + > +#define LENOVO_YMC_EVENT_GUID "06129D99-6083-4164-81AD-F092F9D773A6" > +#define LENOVO_YMC_QUERY_GUID "09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C" > + > +#define LENOVO_YMC_QUERY_INSTANCE 0 > +#define LENOVO_YMC_QUERY_METHOD 0xAB Hi, according to the embedded MOF data, the method id for retrieving the mode data is 0x1, not 0xAB. > + > +static bool ec_trigger __read_mostly; > +module_param(ec_trigger, bool, 0644); > +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events"); > + > +static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = { > + { > + // Lenovo Yoga 7 14ARB7 > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > + DMI_MATCH(DMI_PRODUCT_NAME, "82QF"), > + }, > + }, > + { }, > +}; > + > +struct lenovo_ymc_private { > + struct input_dev *input_dev; > + struct acpi_device *ec_acpi_dev; > +}; > + > +static void lenovo_ymc_trigger_ec(struct wmi_device *wdev, struct lenovo_ymc_private *priv) > +{ > + int err; > + > + if (!ec_trigger || !priv || !priv->ec_acpi_dev) > + return; > + err = write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1); > + if (err) > + dev_warn(&wdev->dev, "Could not write YMC: %d\n", err); > +} > + > +static const struct key_entry lenovo_ymc_keymap[] = { > + // Laptop > + { KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } }, > + // Tablet > + { KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } }, > + // Drawing Board > + { KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } }, > + // Tent > + { KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } }, > + { KE_END }, > +}; > + > +static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data) > +{ > + struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev); > + > + u32 input_val = 0; > + struct acpi_buffer input = {sizeof(input_val), &input_val}; > + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL}; > + acpi_status status; > + union acpi_object *obj; > + int code; > + > + status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID, > + LENOVO_YMC_QUERY_INSTANCE, > + LENOVO_YMC_QUERY_METHOD, > + &input, &output); Using wmi_evaluate_method() is deprecated as WMI GUIDs are not guaranteed to be unique for a given machine. I think both WMI GUIDs should be handled by separate drivers, and the driver for the event GUID uses a notifier call chain to inform the driver for the data GUID that the usage mode changed, which then handles the input device stuff. This way the driver does not rely on deprecated WMI APIs, and everything would still work should Lenovo decide to duplicate some WMI GUIDs. > + > + if (ACPI_FAILURE(status)) { > + dev_warn(&wdev->dev, > + "Failed to evaluate query method: %s\n", > + acpi_format_exception(status)); > + return; > + } > + > + obj = output.pointer; > + > + if (obj->type != ACPI_TYPE_INTEGER) { > + dev_warn(&wdev->dev, > + "WMI event data is not an integer\n"); > + goto free_obj; > + } > + code = obj->integer.value; > + > + if (!sparse_keymap_report_event(priv->input_dev, code, 1, true)) > + dev_warn(&wdev->dev, "Unknown key %d pressed\n", code); > + > +free_obj: > + kfree(obj); > + lenovo_ymc_trigger_ec(wdev, priv); > +} > + > +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx) > +{ > + struct input_dev *input_dev; > + struct lenovo_ymc_private *priv; > + int err; > + > + ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table); > + > + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + if (ec_trigger) { > + pr_debug("Lenovo YMC enable EC triggering.\n"); > + priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1); acpi_dev_put() is missing. > + if (!priv->ec_acpi_dev) { > + dev_err(&wdev->dev, "Could not find EC ACPI device.\n"); > + return -ENODEV; > + } > + } > + > + input_dev = devm_input_allocate_device(&wdev->dev); > + if (!input_dev) > + return -ENOMEM; > + > + input_dev->name = "Lenovo Yoga Tablet Mode Control switch"; > + input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0"; > + input_dev->id.bustype = BUS_HOST; > + input_dev->dev.parent = &wdev->dev; > + > + input_set_capability(input_dev, EV_SW, SW_TABLET_MODE); > + > + err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL); > + if (err) { > + dev_err(&wdev->dev, > + "Could not set up input device keymap: %d\n", err); > + return err; > + } > + > + err = input_register_device(input_dev); > + if (err) { > + dev_err(&wdev->dev, > + "Could not register input device: %d\n", err); > + return err; > + } Maybe it would be beneficial to allow userspace to get the current usage mode without having to wait for an WMI event. This way, userspace could still react to situations like the device already being in tablet mode when this driver is loaded. > + > + priv->input_dev = input_dev; > + dev_set_drvdata(&wdev->dev, priv); > + lenovo_ymc_trigger_ec(wdev, priv); > + return 0; > +} > + > +static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = { > + { .guid_string = LENOVO_YMC_EVENT_GUID }, > + { } > +}; > +MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table); If the drivers handling the event and data GUIDs are fine with being instantiated multiple times, then adding the WMI GUIDs to the allow_duplicates[] list in drivers/platform/x86/wmi.c will allow the WMI driver core to handle duplicated event and data GUIDs. Armin Wolf > + > +static struct wmi_driver lenovo_ymc_driver = { > + .driver = { > + .name = "lenovo-ymc", > + }, > + .id_table = lenovo_ymc_wmi_id_table, > + .probe = lenovo_ymc_probe, > + .notify = lenovo_ymc_notify, > +}; > + > +module_wmi_driver(lenovo_ymc_driver); > + > +MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>"); > +MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver"); > +MODULE_LICENSE("GPL");
On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf <W_Armin@gmx.de> wrote: > > Hi, > > according to the embedded MOF data, the method id for retrieving the mode data > is 0x1, not 0xAB. Thanks! I can see from your earlier email in the other thread that this is right. Strangely, when I tested 0xAB had almost exactly the same behavior as 0x01. I think you're right though, I have updated my local copy to 0x01. I have also fixed the missing cleanup calls and > Using wmi_evaluate_method() is deprecated as WMI GUIDs are not guaranteed to be > unique for a given machine. I think both WMI GUIDs should be handled by separate > drivers, and the driver for the event GUID uses a notifier call chain to inform > the driver for the data GUID that the usage mode changed, which then handles the > input device stuff. > > This way the driver does not rely on deprecated WMI APIs, and everything would > still work should Lenovo decide to duplicate some WMI GUIDs. After reading many times, I believe I understand this and can figure out the implementation. How should I attribute the commits? Should this be a 3rd patch in a v2 patch series with myself as the author? Or should these drivers be introduced in one commit without the intermediate single driver that uses the deprecated API? Also have I correctly set Gergo as the commit author for this PATCH 2/2 email like Hans said to? I wasn't sure if I had it right and I could fix it in the v2 series. > acpi_dev_put() is missing. Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove but I have added that back in with the input_unregister_device call. > Maybe it would be beneficial to allow userspace to get the current usage mode without having > to wait for an WMI event. This way, userspace could still react to situations like the device > already being in tablet mode when this driver is loaded. I'm not following how to implement this, not familiar enough with WMI. Could you explain more? > If the drivers handling the event and data GUIDs are fine with being instantiated multiple > times, then adding the WMI GUIDs to the allow_duplicates[] list in drivers/platform/x86/wmi.c > will allow the WMI driver core to handle duplicated event and data GUIDs. Is there an easy way to test if it's fine to run multiple copies? Currently testing by compiling the module with an inlined ideapad-laptop.h out of the kernel tree and using the insmod command to load it.
Am 15.03.23 um 04:37 schrieb Andrew Kallmeyer: > On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf <W_Armin@gmx.de> wrote: >> Hi, >> >> according to the embedded MOF data, the method id for retrieving the mode data >> is 0x1, not 0xAB. > Thanks! I can see from your earlier email in the other thread that > this is right. Strangely, when I tested 0xAB had almost exactly the > same behavior as 0x01. I think you're right though, I have updated my > local copy to 0x01. Hi, the reason for this is that most hardware vendors will omit any input checks if they think they are unnecessary. The WMI interface on my Dell notebook does the same, sadly. > I have also fixed the missing cleanup calls and > >> Using wmi_evaluate_method() is deprecated as WMI GUIDs are not guaranteed to be >> unique for a given machine. I think both WMI GUIDs should be handled by separate >> drivers, and the driver for the event GUID uses a notifier call chain to inform >> the driver for the data GUID that the usage mode changed, which then handles the >> input device stuff. >> >> This way the driver does not rely on deprecated WMI APIs, and everything would >> still work should Lenovo decide to duplicate some WMI GUIDs. > After reading many times, I believe I understand this and can figure > out the implementation. How should I attribute the commits? Should > this be a 3rd patch in a v2 patch series with myself as the author? Or > should these drivers be introduced in one commit without the > intermediate single driver that uses the deprecated API? I thing the new driver(s) should be introduced in one commit. I am not sure about the new author of the commit. If the original driver was significantly altered, then AFAIK you should be the new author. Still, the original author should at least be mentioned with a "Co-developed by" tag. You can then omit your "Co-developed by" tag. As a site note, i recommend to use the get_maintainer.pl scripts under scripts/ to find any additional maintainers which should be CC-ed. Since your patch series touches the ideapad-laptop driver, its maintainer (Ike Panhc <ike.pan@canonical.com>) should also be notified about this patch series. > Also have I correctly set Gergo as the commit author for this PATCH > 2/2 email like Hans said to? I wasn't sure if I had it right and I > could fix it in the v2 series. > You are still the author of the second patch. Also you should not send a patch series as a reply to any previous emails. >> acpi_dev_put() is missing. > Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove > but I have added that back in with the input_unregister_device call. > >> Maybe it would be beneficial to allow userspace to get the current usage mode without having >> to wait for an WMI event. This way, userspace could still react to situations like the device >> already being in tablet mode when this driver is loaded. > I'm not following how to implement this, not familiar enough with WMI. > Could you explain more? I meant that your driver should, after (!) setting up the input device and event handling, call sparse_keymap_report_event() with the code obtained from wmidev_evaluate_method() so that userspace knows about the initial state of the input device. You might also CC linux-input@vger.kernel.org for the next patch series, so that the input driver maintainers can also review your patch series. >> If the drivers handling the event and data GUIDs are fine with being instantiated multiple >> times, then adding the WMI GUIDs to the allow_duplicates[] list in drivers/platform/x86/wmi.c >> will allow the WMI driver core to handle duplicated event and data GUIDs. > Is there an easy way to test if it's fine to run multiple copies? > Currently testing by compiling the module with an inlined > ideapad-laptop.h out of the kernel tree and using the insmod command > to load it. Drivers can be instantiated multiple times, and each time their probe callback is invoked, and many older WMI drivers cannot do this, so the allowlist exists. The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst explains how to write drivers which can be instantiated multiple times. If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then you can add its WMI GUID to the allowlist.
Am 15.03.23 um 23:33 schrieb Armin Wolf: > Am 15.03.23 um 04:37 schrieb Andrew Kallmeyer: > >> On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf <W_Armin@gmx.de> wrote: >>> Hi, >>> >>> according to the embedded MOF data, the method id for retrieving the >>> mode data >>> is 0x1, not 0xAB. >> Thanks! I can see from your earlier email in the other thread that >> this is right. Strangely, when I tested 0xAB had almost exactly the >> same behavior as 0x01. I think you're right though, I have updated my >> local copy to 0x01. > > Hi, > > the reason for this is that most hardware vendors will omit any input > checks > if they think they are unnecessary. The WMI interface on my Dell notebook > does the same, sadly. > >> I have also fixed the missing cleanup calls and >> >>> Using wmi_evaluate_method() is deprecated as WMI GUIDs are not >>> guaranteed to be >>> unique for a given machine. I think both WMI GUIDs should be handled >>> by separate >>> drivers, and the driver for the event GUID uses a notifier call >>> chain to inform >>> the driver for the data GUID that the usage mode changed, which then >>> handles the >>> input device stuff. >>> >>> This way the driver does not rely on deprecated WMI APIs, and >>> everything would >>> still work should Lenovo decide to duplicate some WMI GUIDs. >> After reading many times, I believe I understand this and can figure >> out the implementation. How should I attribute the commits? Should >> this be a 3rd patch in a v2 patch series with myself as the author? Or >> should these drivers be introduced in one commit without the >> intermediate single driver that uses the deprecated API? > > I thing the new driver(s) should be introduced in one commit. I am not > sure about the > new author of the commit. If the original driver was significantly > altered, then AFAIK > you should be the new author. Still, the original author should at > least be mentioned > with a "Co-developed by" tag. You can then omit your "Co-developed by" > tag. > > As a site note, i recommend to use the get_maintainer.pl scripts under > scripts/ to find > any additional maintainers which should be CC-ed. Since your patch > series touches the > ideapad-laptop driver, its maintainer (Ike Panhc > <ike.pan@canonical.com>) should also be > notified about this patch series. > I forgot to mention that your patches have to title, please add one for the next revision. Armin Wolf >> Also have I correctly set Gergo as the commit author for this PATCH >> 2/2 email like Hans said to? I wasn't sure if I had it right and I >> could fix it in the v2 series. >> > You are still the author of the second patch. Also you should not send > a patch series as > a reply to any previous emails. > >>> acpi_dev_put() is missing. >> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove >> but I have added that back in with the input_unregister_device call. >> >>> Maybe it would be beneficial to allow userspace to get the current >>> usage mode without having >>> to wait for an WMI event. This way, userspace could still react to >>> situations like the device >>> already being in tablet mode when this driver is loaded. >> I'm not following how to implement this, not familiar enough with WMI. >> Could you explain more? > > I meant that your driver should, after (!) setting up the input device > and event handling, call > sparse_keymap_report_event() with the code obtained from > wmidev_evaluate_method() so that userspace knows about the initial state > of the input device. You might also CC linux-input@vger.kernel.org for > the next patch series, so that the input driver maintainers can also > review your patch series. > >>> If the drivers handling the event and data GUIDs are fine with being >>> instantiated multiple >>> times, then adding the WMI GUIDs to the allow_duplicates[] list in >>> drivers/platform/x86/wmi.c >>> will allow the WMI driver core to handle duplicated event and data >>> GUIDs. >> Is there an easy way to test if it's fine to run multiple copies? >> Currently testing by compiling the module with an inlined >> ideapad-laptop.h out of the kernel tree and using the insmod command >> to load it. > > Drivers can be instantiated multiple times, and each time their probe > callback is invoked, > and many older WMI drivers cannot do this, so the allowlist exists. > The section "State Container" in > Documentation/driver-api/driver-model/design-patterns.rst > explains how to write drivers which can be instantiated multiple times. > > If your driver is not a singleton, i.e. it can safely be instantiated > multiple times, then > you can add its WMI GUID to the allowlist. >
Hi, Sorry for being a bit late with replying. Andrew, thank you for your work on this, at a quick glance it looks great. Armin, thank you for your help / input on this. On 3/15/23 23:33, Armin Wolf wrote: > Am 15.03.23 um 04:37 schrieb Andrew Kallmeyer: > >> On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf <W_Armin@gmx.de> wrote: >>> Hi, >>> >>> according to the embedded MOF data, the method id for retrieving the mode data >>> is 0x1, not 0xAB. >> Thanks! I can see from your earlier email in the other thread that >> this is right. Strangely, when I tested 0xAB had almost exactly the >> same behavior as 0x01. I think you're right though, I have updated my >> local copy to 0x01. > > Hi, > > the reason for this is that most hardware vendors will omit any input checks > if they think they are unnecessary. The WMI interface on my Dell notebook > does the same, sadly. Ack. >> I have also fixed the missing cleanup calls and >> >>> Using wmi_evaluate_method() is deprecated as WMI GUIDs are not guaranteed to be >>> unique for a given machine. I think both WMI GUIDs should be handled by separate >>> drivers, and the driver for the event GUID uses a notifier call chain to inform >>> the driver for the data GUID that the usage mode changed, which then handles the >>> input device stuff. >>> >>> This way the driver does not rely on deprecated WMI APIs, and everything would >>> still work should Lenovo decide to duplicate some WMI GUIDs. Jumping into the discussion a bit late here, but I disagree with this, using wmi_evaluate_method() here is fine. The chances of there ever being 2 instances of the LENOVO_YMC_QUERY_GUID are very small and we can deal with that if it ever happens. So I really so no need to make the code needlessly complicated with 2 sub-drivers which then notify each other. Let keep things KISS and keep this as is, so for the next version only change the method_id from 0xab to 0x01. >> After reading many times, I believe I understand this and can figure >> out the implementation. How should I attribute the commits? Should >> this be a 3rd patch in a v2 patch series with myself as the author? Or >> should these drivers be introduced in one commit without the >> intermediate single driver that uses the deprecated API? > > I thing the new driver(s) should be introduced in one commit. I am not sure about the > new author of the commit. If the original driver was significantly altered, then AFAIK > you should be the new author. Still, the original author should at least be mentioned > with a "Co-developed by" tag. You can then omit your "Co-developed by" tag. Ack to introducing the new driver in 1 commit. Since compared to this version only the method_id is going to make change t make it makes sense to keep Gergo as author and Andrew as Co-developed-by . > As a site note, i recommend to use the get_maintainer.pl scripts under scripts/ to find > any additional maintainers which should be CC-ed. Since your patch series touches the > ideapad-laptop driver, its maintainer (Ike Panhc <ike.pan@canonical.com>) should also be > notified about this patch series. > >> Also have I correctly set Gergo as the commit author for this PATCH >> 2/2 email like Hans said to? I wasn't sure if I had it right and I >> could fix it in the v2 series. >> > You are still the author of the second patch. Also you should not send a patch series as > a reply to any previous emails. > >>> acpi_dev_put() is missing. >> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove >> but I have added that back in with the input_unregister_device call. >> >>> Maybe it would be beneficial to allow userspace to get the current usage mode without having >>> to wait for an WMI event. This way, userspace could still react to situations like the device >>> already being in tablet mode when this driver is loaded. >> I'm not following how to implement this, not familiar enough with WMI. >> Could you explain more? > > I meant that your driver should, after (!) setting up the input device and event handling, call > sparse_keymap_report_event() with the code obtained from > wmidev_evaluate_method() so that userspace knows about the initial state > of the input device. You might also CC linux-input@vger.kernel.org for > the next patch series, so that the input driver maintainers can also > review your patch series. Ack, reporting the initial state would be a good addition. To keep the authorship clear I think this can be added as a follow-up 3/3 patch in the next version. I realize this contradicts a bit with my previous lets introduce the driver in 1 commit thing, but in this case I think this makes the most sense. >>> If the drivers handling the event and data GUIDs are fine with being instantiated multiple >>> times, then adding the WMI GUIDs to the allow_duplicates[] list in drivers/platform/x86/wmi.c >>> will allow the WMI driver core to handle duplicated event and data GUIDs. >> Is there an easy way to test if it's fine to run multiple copies? >> Currently testing by compiling the module with an inlined >> ideapad-laptop.h out of the kernel tree and using the insmod command >> to load it. > > Drivers can be instantiated multiple times, and each time their probe callback is invoked, > and many older WMI drivers cannot do this, so the allowlist exists. > The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst > explains how to write drivers which can be instantiated multiple times. > > If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then > you can add its WMI GUID to the allowlist. I'm not sure about adding this to the allowlist, using the new API is good (and nice and clean) but this is still expected to be a singleton. Regards, Hans
Hi On 3/15/23 23:39, Armin Wolf wrote: <snip> >> As a site note, i recommend to use the get_maintainer.pl scripts under >> scripts/ to find >> any additional maintainers which should be CC-ed. Since your patch >> series touches the >> ideapad-laptop driver, its maintainer (Ike Panhc >> <ike.pan@canonical.com>) should also be >> notified about this patch series. >> > I forgot to mention that your patches have to title, please add one for the next revision. Armin, I'm not sure what you mean with this ? For me the patches have a good $subject / first line of the commit message ? Regards, Hans > > Armin Wolf > >>> Also have I correctly set Gergo as the commit author for this PATCH >>> 2/2 email like Hans said to? I wasn't sure if I had it right and I >>> could fix it in the v2 series. >>> >> You are still the author of the second patch. Also you should not send >> a patch series as >> a reply to any previous emails. >> >>>> acpi_dev_put() is missing. >>> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove >>> but I have added that back in with the input_unregister_device call. >>> >>>> Maybe it would be beneficial to allow userspace to get the current >>>> usage mode without having >>>> to wait for an WMI event. This way, userspace could still react to >>>> situations like the device >>>> already being in tablet mode when this driver is loaded. >>> I'm not following how to implement this, not familiar enough with WMI. >>> Could you explain more? >> >> I meant that your driver should, after (!) setting up the input device >> and event handling, call >> sparse_keymap_report_event() with the code obtained from >> wmidev_evaluate_method() so that userspace knows about the initial state >> of the input device. You might also CC linux-input@vger.kernel.org for >> the next patch series, so that the input driver maintainers can also >> review your patch series. >> >>>> If the drivers handling the event and data GUIDs are fine with being >>>> instantiated multiple >>>> times, then adding the WMI GUIDs to the allow_duplicates[] list in >>>> drivers/platform/x86/wmi.c >>>> will allow the WMI driver core to handle duplicated event and data >>>> GUIDs. >>> Is there an easy way to test if it's fine to run multiple copies? >>> Currently testing by compiling the module with an inlined >>> ideapad-laptop.h out of the kernel tree and using the insmod command >>> to load it. >> >> Drivers can be instantiated multiple times, and each time their probe >> callback is invoked, >> and many older WMI drivers cannot do this, so the allowlist exists. >> The section "State Container" in >> Documentation/driver-api/driver-model/design-patterns.rst >> explains how to write drivers which can be instantiated multiple times. >> >> If your driver is not a singleton, i.e. it can safely be instantiated >> multiple times, then >> you can add its WMI GUID to the allowlist. >> >
Am 16.03.23 um 10:02 schrieb Hans de Goede: > Hi > > On 3/15/23 23:39, Armin Wolf wrote: > > <snip> > >>> As a site note, i recommend to use the get_maintainer.pl scripts under >>> scripts/ to find >>> any additional maintainers which should be CC-ed. Since your patch >>> series touches the >>> ideapad-laptop driver, its maintainer (Ike Panhc >>> <ike.pan@canonical.com>) should also be >>> notified about this patch series. >>> >> I forgot to mention that your patches have to title, please add one for the next revision. > Armin, I'm not sure what you mean with this ? > > For me the patches have a good $subject / first line of the commit message ? > > Regards, > > Hans > > My fault, i messed up. Sorry for the unnecessary noise. Armin Wolf > > >> Armin Wolf >> >>>> Also have I correctly set Gergo as the commit author for this PATCH >>>> 2/2 email like Hans said to? I wasn't sure if I had it right and I >>>> could fix it in the v2 series. >>>> >>> You are still the author of the second patch. Also you should not send >>> a patch series as >>> a reply to any previous emails. >>> >>>>> acpi_dev_put() is missing. >>>> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove >>>> but I have added that back in with the input_unregister_device call. >>>> >>>>> Maybe it would be beneficial to allow userspace to get the current >>>>> usage mode without having >>>>> to wait for an WMI event. This way, userspace could still react to >>>>> situations like the device >>>>> already being in tablet mode when this driver is loaded. >>>> I'm not following how to implement this, not familiar enough with WMI. >>>> Could you explain more? >>> I meant that your driver should, after (!) setting up the input device >>> and event handling, call >>> sparse_keymap_report_event() with the code obtained from >>> wmidev_evaluate_method() so that userspace knows about the initial state >>> of the input device. You might also CC linux-input@vger.kernel.org for >>> the next patch series, so that the input driver maintainers can also >>> review your patch series. >>> >>>>> If the drivers handling the event and data GUIDs are fine with being >>>>> instantiated multiple >>>>> times, then adding the WMI GUIDs to the allow_duplicates[] list in >>>>> drivers/platform/x86/wmi.c >>>>> will allow the WMI driver core to handle duplicated event and data >>>>> GUIDs. >>>> Is there an easy way to test if it's fine to run multiple copies? >>>> Currently testing by compiling the module with an inlined >>>> ideapad-laptop.h out of the kernel tree and using the insmod command >>>> to load it. >>> Drivers can be instantiated multiple times, and each time their probe >>> callback is invoked, >>> and many older WMI drivers cannot do this, so the allowlist exists. >>> The section "State Container" in >>> Documentation/driver-api/driver-model/design-patterns.rst >>> explains how to write drivers which can be instantiated multiple times. >>> >>> If your driver is not a singleton, i.e. it can safely be instantiated >>> multiple times, then >>> you can add its WMI GUID to the allowlist. >>>
Am 16.03.23 um 10:00 schrieb Hans de Goede: > Hi, > > Sorry for being a bit late with replying. > > Andrew, thank you for your work on this, at a quick glance it looks great. > > Armin, thank you for your help / input on this. > > On 3/15/23 23:33, Armin Wolf wrote: >> Am 15.03.23 um 04:37 schrieb Andrew Kallmeyer: >> >>> On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf<W_Armin@gmx.de> wrote: >>>> Hi, >>>> >>>> according to the embedded MOF data, the method id for retrieving the mode data >>>> is 0x1, not 0xAB. >>> Thanks! I can see from your earlier email in the other thread that >>> this is right. Strangely, when I tested 0xAB had almost exactly the >>> same behavior as 0x01. I think you're right though, I have updated my >>> local copy to 0x01. >> Hi, >> >> the reason for this is that most hardware vendors will omit any input checks >> if they think they are unnecessary. The WMI interface on my Dell notebook >> does the same, sadly. > Ack. > > >>> I have also fixed the missing cleanup calls and >>> >>>> Using wmi_evaluate_method() is deprecated as WMI GUIDs are not guaranteed to be >>>> unique for a given machine. I think both WMI GUIDs should be handled by separate >>>> drivers, and the driver for the event GUID uses a notifier call chain to inform >>>> the driver for the data GUID that the usage mode changed, which then handles the >>>> input device stuff. >>>> >>>> This way the driver does not rely on deprecated WMI APIs, and everything would >>>> still work should Lenovo decide to duplicate some WMI GUIDs. > Jumping into the discussion a bit late here, but I disagree with this, using wmi_evaluate_method() here is fine. The chances of there ever being 2 instances of the LENOVO_YMC_QUERY_GUID are very small and we can deal with that if it ever happens. > > So I really so no need to make the code needlessly complicated with 2 sub-drivers which then notify each other. Let keep things KISS and keep this as is, so for the next version only change > the method_id from 0xab to 0x01. I think that using wmi_evaluate_method() is deprecated and we should try to minimize its usage whenever possible. As for the handling of the WMI GUIDs, i believe that just using the first matching WMI device is not a stable solution. We simply do not know if Lenovo considers both WMI GUIDs singletons or not. This means they could for example decide to have multiple independent data sources for tablet mode events. The chances for this are indeed small, but it will still create a problem for users using such machines. By having two drivers and maybe a global notifier call chain, we would enable the driver to handle such "unlikely situations" correctly. This would also allow the driver to work on machines missing the WMI event GUID. In such a case, userspace could then just poll the data WMI GUID for input, but this is just an idea. >>> After reading many times, I believe I understand this and can figure >>> out the implementation. How should I attribute the commits? Should >>> this be a 3rd patch in a v2 patch series with myself as the author? Or >>> should these drivers be introduced in one commit without the >>> intermediate single driver that uses the deprecated API? >> I thing the new driver(s) should be introduced in one commit. I am not sure about the >> new author of the commit. If the original driver was significantly altered, then AFAIK >> you should be the new author. Still, the original author should at least be mentioned >> with a "Co-developed by" tag. You can then omit your "Co-developed by" tag. > Ack to introducing the new driver in 1 commit. Since compared to this version only the method_id is going to make change t make it makes sense to keep Gergo as author and Andrew as Co-developed-by . > >> As a site note, i recommend to use the get_maintainer.pl scripts under scripts/ to find >> any additional maintainers which should be CC-ed. Since your patch series touches the >> ideapad-laptop driver, its maintainer (Ike Panhc<ike.pan@canonical.com>) should also be >> notified about this patch series. >> >>> Also have I correctly set Gergo as the commit author for this PATCH >>> 2/2 email like Hans said to? I wasn't sure if I had it right and I >>> could fix it in the v2 series. >>> >> You are still the author of the second patch. Also you should not send a patch series as >> a reply to any previous emails. >> >>>> acpi_dev_put() is missing. >>> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove >>> but I have added that back in with the input_unregister_device call. >>> >>>> Maybe it would be beneficial to allow userspace to get the current usage mode without having >>>> to wait for an WMI event. This way, userspace could still react to situations like the device >>>> already being in tablet mode when this driver is loaded. >>> I'm not following how to implement this, not familiar enough with WMI. >>> Could you explain more? >> I meant that your driver should, after (!) setting up the input device and event handling, call >> sparse_keymap_report_event() with the code obtained from >> wmidev_evaluate_method() so that userspace knows about the initial state >> of the input device. You might also CClinux-input@vger.kernel.org for >> the next patch series, so that the input driver maintainers can also >> review your patch series. > Ack, reporting the initial state would be a good addition. To keep the authorship clear I think this can be added as a follow-up 3/3 patch in the next version. > > I realize this contradicts a bit with my previous lets introduce the driver in 1 commit thing, but in this case I think this makes the most sense. > >>>> If the drivers handling the event and data GUIDs are fine with being instantiated multiple >>>> times, then adding the WMI GUIDs to the allow_duplicates[] list in drivers/platform/x86/wmi.c >>>> will allow the WMI driver core to handle duplicated event and data GUIDs. >>> Is there an easy way to test if it's fine to run multiple copies? >>> Currently testing by compiling the module with an inlined >>> ideapad-laptop.h out of the kernel tree and using the insmod command >>> to load it. >> Drivers can be instantiated multiple times, and each time their probe callback is invoked, >> and many older WMI drivers cannot do this, so the allowlist exists. >> The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst >> explains how to write drivers which can be instantiated multiple times. >> >> If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then >> you can add its WMI GUID to the allowlist. > I'm not sure about adding this to the allowlist, using the new API is good (and nice and clean) but this is still expected to be a singleton. The allowlist is dealing with drivers not jet converted to the WMI bus model. The allowlist should ideally disappear once the conversion has been completed, something which would become difficult if WMI drivers would continue to rely on the older GUID singleton behavior which is not compliant with the ACPI WMI spec AFAIK. If we know that our WMI GUID is a singleton (which we do not), then we should handle this inside our driver, not inside the WMI probing code. > Regards, > > Hans >
On Fri, Mar 17, 2023 at 5:39 AM Armin Wolf <W_Armin@gmx.de> wrote: > > Am 16.03.23 um 10:00 schrieb Hans de Goede: >> >> So I really so no need to make the code needlessly complicated with 2 sub-drivers which then notify each other. Let keep things KISS and keep this as is, so for the next version only change >> the method_id from 0xab to 0x01. > > I think that using wmi_evaluate_method() is deprecated and we should try to minimize its usage whenever possible. As for the handling of the WMI GUIDs, i believe that > just using the first matching WMI device is not a stable solution. We simply do not know if Lenovo considers both WMI GUIDs singletons or not. This means they could > for example decide to have multiple independent data sources for tablet mode events. The chances for this are indeed small, but it will still create a problem for users > using such machines. By having two drivers and maybe a global notifier call chain, we would enable the driver to handle such "unlikely situations" correctly. > > This would also allow the driver to work on machines missing the WMI event GUID. In such a case, userspace could then just poll the data WMI GUID for input, but > this is just an idea. Hi Armin, would it work to add the second GUID to the existing wmi_driver wmi_device_id array? Then I could save the wmi_device in the driver data on probe. Later when I get the notification on the other GUID I would just call wmidev_evaluate_method on the saved pointer out of the private data. I would just need a way to distinguish the two wmi_device structs. Seems like the notifier setup wouldn't be needed and it could stay as one module for one feature. I have the code ready to mail a v2 patch series with the remove function added and the fixed method id and the input triggering on probe, but still using wmi_evaluate_method. Without having much kernel experience, I sort of agree with Hans that it would be best to be simpler and not have two modules for one feature, the notifier setup looks somewhat involved. However if we can do something like the above idea, that doesn't seem to make it much more complicated to avoid the deprecated API and I can mail that out instead. So let me know what you think. >>> Drivers can be instantiated multiple times, and each time their probe callback is invoked, >>> and many older WMI drivers cannot do this, so the allowlist exists. >>> The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst >>> explains how to write drivers which can be instantiated multiple times. >>> >>> If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then >>> you can add its WMI GUID to the allowlist. >> I'm not sure about adding this to the allowlist, using the new API is good (and nice and clean) but this is still expected to be a singleton. > > The allowlist is dealing with drivers not jet converted to the WMI bus model. The allowlist should ideally disappear once the conversion has been > completed, something which would become difficult if WMI drivers would continue to rely on the older GUID singleton behavior which is not compliant > with the ACPI WMI spec AFAIK. If we know that our WMI GUID is a singleton (which we do not), then we should handle this inside our driver, not inside > the WMI probing code. Would it not work to convert this driver to the WMI bus model now? I wasn't able to find anything about this bus model code. If I'm understanding Documentation/driver-api/driver-model/design-patterns.rst correctly then I think this driver would be okay to have the same GUID probed multiple times. Each device would get its own private data allocated for it and that would be cleaned up which each device is removed. Although, it does look like you would get double input events. I'm not sure if that's the correct behavior without access to a machine with two of these switch devices to test on. - Andrew
On Sat, Mar 18, 2023 at 10:50 AM Andrew Kallmeyer <kallmeyeras@gmail.com> wrote: > > Hi Armin, would it work to add the second GUID to the existing > wmi_driver wmi_device_id array? Then I could save the wmi_device in > the driver data on probe. Later when I get the notification on the > other GUID I would just call wmidev_evaluate_method on the saved > pointer out of the private data. Now that I have understood the multiple probe calls and went back to read this, I realized that I would not be able to access the private data of the notify device when handling the probe call for the query device. Maybe you will have a good idea to solve this problem. If not I do have this working with the deprecated call still. - Andrew
Hi Armin, On 3/17/23 13:39, Armin Wolf wrote: > Am 16.03.23 um 10:00 schrieb Hans de Goede: > >> Hi, >> >> Sorry for being a bit late with replying. >> >> Andrew, thank you for your work on this, at a quick glance it looks great. >> >> Armin, thank you for your help / input on this. >> >> On 3/15/23 23:33, Armin Wolf wrote: >>> Am 15.03.23 um 04:37 schrieb Andrew Kallmeyer: >>> >>>> On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf<W_Armin@gmx.de> wrote: >>>>> Hi, >>>>> >>>>> according to the embedded MOF data, the method id for retrieving the mode data >>>>> is 0x1, not 0xAB. >>>> Thanks! I can see from your earlier email in the other thread that >>>> this is right. Strangely, when I tested 0xAB had almost exactly the >>>> same behavior as 0x01. I think you're right though, I have updated my >>>> local copy to 0x01. >>> Hi, >>> >>> the reason for this is that most hardware vendors will omit any input checks >>> if they think they are unnecessary. The WMI interface on my Dell notebook >>> does the same, sadly. >> Ack. >> >> >>>> I have also fixed the missing cleanup calls and >>>> >>>>> Using wmi_evaluate_method() is deprecated as WMI GUIDs are not guaranteed to be >>>>> unique for a given machine. I think both WMI GUIDs should be handled by separate >>>>> drivers, and the driver for the event GUID uses a notifier call chain to inform >>>>> the driver for the data GUID that the usage mode changed, which then handles the >>>>> input device stuff. >>>>> >>>>> This way the driver does not rely on deprecated WMI APIs, and everything would >>>>> still work should Lenovo decide to duplicate some WMI GUIDs. >> Jumping into the discussion a bit late here, but I disagree with this, using wmi_evaluate_method() here is fine. The chances of there ever being 2 instances of the LENOVO_YMC_QUERY_GUID are very small and we can deal with that if it ever happens. >> >> So I really so no need to make the code needlessly complicated with 2 sub-drivers which then notify each other. Let keep things KISS and keep this as is, so for the next version only change >> the method_id from 0xab to 0x01. > > I think that using wmi_evaluate_method() is deprecated and we should try to minimize its usage whenever possible. As for the handling of the WMI GUIDs, i believe that > just using the first matching WMI device is not a stable solution. We simply do not know if Lenovo considers both WMI GUIDs singletons or not. This means they could > for example decide to have multiple independent data sources for tablet mode events. The chances for this are indeed small, but it will still create a problem for users > using such machines. By having two drivers and maybe a global notifier call chain, we would enable the driver to handle such "unlikely situations" correctly. Armin, first of all let me say tht I really appreciate your input / help with reviewing pdx86 patches. The problem is that your suggestion with 2 drivers attaching themselves to 2 different wmi_device-s is that we will need to introduce some shared global data / variable for the 2 to communicate. Notifiers don't magically work. They need a shared notifier head between the producer and consumer(s) to work. So you are still introducing a singleton. Sometimes this is a good solution, but in this case this really seems like an overly complicated solution to me. Making the code needlessly complicated to deal with a theoretical scenario which I don't believe will ever happen. Also see below. > This would also allow the driver to work on machines missing the WMI event GUID. In such a case, userspace could then just poll the data WMI GUID for input, but > this is just an idea. > >>>> After reading many times, I believe I understand this and can figure >>>> out the implementation. How should I attribute the commits? Should >>>> this be a 3rd patch in a v2 patch series with myself as the author? Or >>>> should these drivers be introduced in one commit without the >>>> intermediate single driver that uses the deprecated API? >>> I thing the new driver(s) should be introduced in one commit. I am not sure about the >>> new author of the commit. If the original driver was significantly altered, then AFAIK >>> you should be the new author. Still, the original author should at least be mentioned >>> with a "Co-developed by" tag. You can then omit your "Co-developed by" tag. >> Ack to introducing the new driver in 1 commit. Since compared to this version only the method_id is going to make change t make it makes sense to keep Gergo as author and Andrew as Co-developed-by . >> >>> As a site note, i recommend to use the get_maintainer.pl scripts under scripts/ to find >>> any additional maintainers which should be CC-ed. Since your patch series touches the >>> ideapad-laptop driver, its maintainer (Ike Panhc<ike.pan@canonical.com>) should also be >>> notified about this patch series. >>> >>>> Also have I correctly set Gergo as the commit author for this PATCH >>>> 2/2 email like Hans said to? I wasn't sure if I had it right and I >>>> could fix it in the v2 series. >>>> >>> You are still the author of the second patch. Also you should not send a patch series as >>> a reply to any previous emails. >>> >>>>> acpi_dev_put() is missing. >>>> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove >>>> but I have added that back in with the input_unregister_device call. >>>> >>>>> Maybe it would be beneficial to allow userspace to get the current usage mode without having >>>>> to wait for an WMI event. This way, userspace could still react to situations like the device >>>>> already being in tablet mode when this driver is loaded. >>>> I'm not following how to implement this, not familiar enough with WMI. >>>> Could you explain more? >>> I meant that your driver should, after (!) setting up the input device and event handling, call >>> sparse_keymap_report_event() with the code obtained from >>> wmidev_evaluate_method() so that userspace knows about the initial state >>> of the input device. You might also CClinux-input@vger.kernel.org for >>> the next patch series, so that the input driver maintainers can also >>> review your patch series. >> Ack, reporting the initial state would be a good addition. To keep the authorship clear I think this can be added as a follow-up 3/3 patch in the next version. >> >> I realize this contradicts a bit with my previous lets introduce the driver in 1 commit thing, but in this case I think this makes the most sense. >> >>>>> If the drivers handling the event and data GUIDs are fine with being instantiated multiple >>>>> times, then adding the WMI GUIDs to the allow_duplicates[] list in drivers/platform/x86/wmi.c >>>>> will allow the WMI driver core to handle duplicated event and data GUIDs. >>>> Is there an easy way to test if it's fine to run multiple copies? >>>> Currently testing by compiling the module with an inlined >>>> ideapad-laptop.h out of the kernel tree and using the insmod command >>>> to load it. >>> Drivers can be instantiated multiple times, and each time their probe callback is invoked, >>> and many older WMI drivers cannot do this, so the allowlist exists. >>> The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst >>> explains how to write drivers which can be instantiated multiple times. >>> >>> If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then >>> you can add its WMI GUID to the allowlist. >> I'm not sure about adding this to the allowlist, using the new API is good (and nice and clean) but this is still expected to be a singleton. > > The allowlist is dealing with drivers not jet converted to the WMI bus model. The allowlist should ideally disappear once the conversion has been > completed, something which would become difficult if WMI drivers would continue to rely on the older GUID singleton behavior which is not compliant > with the ACPI WMI spec AFAIK. If we know that our WMI GUID is a singleton (which we do not), then we should handle this inside our driver, not inside > the WMI probing code. Ah I see (wrt the above discussion), your aiming for some ideal scenario here were all WMI code gets converted to the new model and the singleton assuming functions can simply be removed. I'm afraid that this will never happen. The old way of doing things is deprecated but if you look at drivers like e.g. acer-wmi not only does it deal with 7 different WMI GUIDs in a single driver. It also exposes sysfs interface under a platform_device which its registers. Moving drivers like this over to binding to a wmi_device instead of to a self-created platform-device, will break userspace API so this simply cannot be done. For reasons of these I have let go the idea of the ideal scenario being converting all existing WMI code to the new model. I don't believe that this is a realistic plan. As such although I don't want to see new WMI drivers which completely use the old model. I'm ok with using some of the old model when single driver needs to deal with more then 1 GUID and the expectations are that the involved GUIDs will always be and stay singletons. Andrew, sorry for the conflicting advice you are getting here. Unfortunately this sometimes happens. In the end I think both approaches (2 separate driver for the GUIDs; or just keep using wmi_evaluate_method()) are fine. So since you are doing the work here, you get to choice which solution you want to pick for the next version. Regards, Hans
Hi Andrew, On 3/18/23 18:50, Andrew Kallmeyer wrote: > On Fri, Mar 17, 2023 at 5:39 AM Armin Wolf <W_Armin@gmx.de> wrote: >> >> Am 16.03.23 um 10:00 schrieb Hans de Goede: >>> >>> So I really so no need to make the code needlessly complicated with 2 sub-drivers which then notify each other. Let keep things KISS and keep this as is, so for the next version only change >>> the method_id from 0xab to 0x01. >> >> I think that using wmi_evaluate_method() is deprecated and we should try to minimize its usage whenever possible. As for the handling of the WMI GUIDs, i believe that >> just using the first matching WMI device is not a stable solution. We simply do not know if Lenovo considers both WMI GUIDs singletons or not. This means they could >> for example decide to have multiple independent data sources for tablet mode events. The chances for this are indeed small, but it will still create a problem for users >> using such machines. By having two drivers and maybe a global notifier call chain, we would enable the driver to handle such "unlikely situations" correctly. >> >> This would also allow the driver to work on machines missing the WMI event GUID. In such a case, userspace could then just poll the data WMI GUID for input, but >> this is just an idea. > > Hi Armin, would it work to add the second GUID to the existing > wmi_driver wmi_device_id array? Then I could save the wmi_device in > the driver data on probe. Later when I get the notification on the > other GUID I would just call wmidev_evaluate_method on the saved > pointer out of the private data. > > I would just need a way to distinguish the two wmi_device structs. > Seems like the notifier setup wouldn't be needed and it could stay as > one module for one feature. > > I have the code ready to mail a v2 patch series with the remove > function added and the fixed method id and the input triggering on > probe, but still using wmi_evaluate_method. Without having much kernel > experience, I sort of agree with Hans that it would be best to be > simpler and not have two modules for one feature, the notifier setup > looks somewhat involved. Yes the notifier setup is somewhat involved. I believe posting the v2 which you have ready to post as is is fine. > However if we can do something like the above > idea, that doesn't seem to make it much more complicated to avoid the > deprecated API and I can mail that out instead. > > So let me know what you think. > >>>> Drivers can be instantiated multiple times, and each time their probe callback is invoked, >>>> and many older WMI drivers cannot do this, so the allowlist exists. >>>> The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst >>>> explains how to write drivers which can be instantiated multiple times. >>>> >>>> If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then >>>> you can add its WMI GUID to the allowlist. >>> I'm not sure about adding this to the allowlist, using the new API is good (and nice and clean) but this is still expected to be a singleton. >> >> The allowlist is dealing with drivers not jet converted to the WMI bus model. The allowlist should ideally disappear once the conversion has been >> completed, something which would become difficult if WMI drivers would continue to rely on the older GUID singleton behavior which is not compliant >> with the ACPI WMI spec AFAIK. If we know that our WMI GUID is a singleton (which we do not), then we should handle this inside our driver, not inside >> the WMI probing code. > > Would it not work to convert this driver to the WMI bus model now? I > wasn't able to find anything about this bus model code. Assuming that with "this driver" you mean the Yoga Tablet Mode switch driver you are working on that already is a WMI BUS driver since it is a wmi_driver which attached itself to a wmi_device. The old way of doing things was with drivers which instantiated their own platform_device-s and then attached to those. These used the old specify a WMI object to operate on through GUID only methods like wmi_evaluate_method() everywhere. Regards, Hans
Hi, On 3/18/23 18:55, Andrew Kallmeyer wrote: > On Sat, Mar 18, 2023 at 10:50 AM Andrew Kallmeyer <kallmeyeras@gmail.com> wrote: >> >> Hi Armin, would it work to add the second GUID to the existing >> wmi_driver wmi_device_id array? Then I could save the wmi_device in >> the driver data on probe. Later when I get the notification on the >> other GUID I would just call wmidev_evaluate_method on the saved >> pointer out of the private data. > > Now that I have understood the multiple probe calls and went back to > read this, I realized that I would not be able to access the private > data of the notify device when handling the probe call for the query > device. Maybe you will have a good idea to solve this problem. Right in this model you would need a global shared notifier_head variable on which the event GUID driver would do a notify and on which the other GUId driver would then register a notifier to get called when the event GUID driver does the notify. So as said this is somewhat involved and I'm fine with the simpler current solution. Regards, Hans
Hi Andrew, Thanks for picking this up. Sorry for the late reply. I no longer think this driver should do the same workaround as ymc.exe does, it shouldn't make non-WMI calls. I think the 14ARB7 should be fixed in the BIOS to work like the other Yogas with the same WMI IDs. So PATCH 1/2 and codes related to ec_trigger are unnecessary. I only have the 14ARB7 and I can't test this without the ec_trigger. For this reason, I don't want to be the author of this module. Could you be the author? The working C940, 14API reports are from https://github.com/lukas-w/yoga-usage-mode module, which uses the same WMI IDs. Regards, Gergo Co-developed-by: Gergo Koteles <soyer@irl.hu> Signed-off-by: Gergo Koteles <soyer@irl.hu> On 2023. 03. 10. 5:17, Andrew Kallmeyer wrote: > This WMI driver for the tablet mode control switch for Lenovo Yoga > notebooks was originally written by Gergo Koteles. The mode is mapped to > a SW_TABLET_MODE switch capable input device. > > I (Andrew) followed the suggestions that were posted in reply to Gergo's > RFC patch to follow-up and get it merged. Additionally I merged the > lenovo_ymc_trigger_ec function with the ideapad_trigger_ymc_next_read > function since it was no longer external. I have also added the word > "Tablet" to the driver description to hopefully make it more clear. > > As Gergo said: It should work with models like the Yoga C940, Ideapad > flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7. > The 14ARB7 model must trigger the EC to send mode change events. This > might be a firmware bug. > > I have additionally tested this on the Yoga 7 14IAL7. > > Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/ > Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com> > Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com> > --- > drivers/platform/x86/Kconfig | 10 ++ > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/ideapad-laptop.h | 1 + > drivers/platform/x86/lenovo-ymc.c | 177 ++++++++++++++++++++++++++ > 4 files changed, 189 insertions(+) > create mode 100644 drivers/platform/x86/lenovo-ymc.c > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 5692385e2..858be0c65 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -470,6 +470,16 @@ config IDEAPAD_LAPTOP > This is a driver for Lenovo IdeaPad netbooks contains drivers for > rfkill switch, hotkey, fan control and backlight control. > > +config LENOVO_YMC > + tristate "Lenovo Yoga Tablet Mode Control" > + depends on ACPI_WMI > + depends on INPUT > + depends on IDEAPAD_LAPTOP > + select INPUT_SPARSEKMAP > + help > + This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input > + events for Lenovo Yoga notebooks. > + > config SENSORS_HDAPS > tristate "Thinkpad Hard Drive Active Protection System (hdaps)" > depends on INPUT > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 1d3d1b025..10054cdea 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -63,6 +63,7 @@ obj-$(CONFIG_UV_SYSFS) += uv_sysfs.o > # IBM Thinkpad and Lenovo > obj-$(CONFIG_IBM_RTL) += ibm_rtl.o > obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o > +obj-$(CONFIG_LENOVO_YMC) += lenovo-ymc.o > obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o > obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o > obj-$(CONFIG_THINKPAD_LMI) += think-lmi.o > diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h > index 7dd8ce027..2564cb1cd 100644 > --- a/drivers/platform/x86/ideapad-laptop.h > +++ b/drivers/platform/x86/ideapad-laptop.h > @@ -35,6 +35,7 @@ enum { > VPCCMD_W_FAN, > VPCCMD_R_RF, > VPCCMD_W_RF, > + VPCCMD_W_YMC = 0x2A, > VPCCMD_R_FAN = 0x2B, > VPCCMD_R_SPECIAL_BUTTONS = 0x31, > VPCCMD_W_BL_POWER = 0x33, > diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c > new file mode 100644 > index 000000000..e29ada1b4 > --- /dev/null > +++ b/drivers/platform/x86/lenovo-ymc.c > @@ -0,0 +1,177 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * lenovo-ymc.c - Lenovo Yoga Mode Control driver > + * > + * Copyright © 2022 Gergo Koteles <soyer@irl.hu> > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/acpi.h> > +#include <linux/dmi.h> > +#include <linux/input.h> > +#include <linux/input/sparse-keymap.h> > +#include <linux/wmi.h> > +#include "ideapad-laptop.h" > + > +#define LENOVO_YMC_EVENT_GUID "06129D99-6083-4164-81AD-F092F9D773A6" > +#define LENOVO_YMC_QUERY_GUID "09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C" > + > +#define LENOVO_YMC_QUERY_INSTANCE 0 > +#define LENOVO_YMC_QUERY_METHOD 0xAB > + > +static bool ec_trigger __read_mostly; > +module_param(ec_trigger, bool, 0644); > +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events"); > + > +static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = { > + { > + // Lenovo Yoga 7 14ARB7 > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > + DMI_MATCH(DMI_PRODUCT_NAME, "82QF"), > + }, > + }, > + { }, > +}; > + > +struct lenovo_ymc_private { > + struct input_dev *input_dev; > + struct acpi_device *ec_acpi_dev; > +}; > + > +static void lenovo_ymc_trigger_ec(struct wmi_device *wdev, struct lenovo_ymc_private *priv) > +{ > + int err; > + > + if (!ec_trigger || !priv || !priv->ec_acpi_dev) > + return; > + err = write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1); > + if (err) > + dev_warn(&wdev->dev, "Could not write YMC: %d\n", err); > +} > + > +static const struct key_entry lenovo_ymc_keymap[] = { > + // Laptop > + { KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } }, > + // Tablet > + { KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } }, > + // Drawing Board > + { KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } }, > + // Tent > + { KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } }, > + { KE_END }, > +}; > + > +static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data) > +{ > + struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev); > + > + u32 input_val = 0; > + struct acpi_buffer input = {sizeof(input_val), &input_val}; > + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL}; > + acpi_status status; > + union acpi_object *obj; > + int code; > + > + status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID, > + LENOVO_YMC_QUERY_INSTANCE, > + LENOVO_YMC_QUERY_METHOD, > + &input, &output); > + > + if (ACPI_FAILURE(status)) { > + dev_warn(&wdev->dev, > + "Failed to evaluate query method: %s\n", > + acpi_format_exception(status)); > + return; > + } > + > + obj = output.pointer; > + > + if (obj->type != ACPI_TYPE_INTEGER) { > + dev_warn(&wdev->dev, > + "WMI event data is not an integer\n"); > + goto free_obj; > + } > + code = obj->integer.value; > + > + if (!sparse_keymap_report_event(priv->input_dev, code, 1, true)) > + dev_warn(&wdev->dev, "Unknown key %d pressed\n", code); > + > +free_obj: > + kfree(obj); > + lenovo_ymc_trigger_ec(wdev, priv); > +} > + > +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx) > +{ > + struct input_dev *input_dev; > + struct lenovo_ymc_private *priv; > + int err; > + > + ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table); > + > + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + if (ec_trigger) { > + pr_debug("Lenovo YMC enable EC triggering.\n"); > + priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1); > + if (!priv->ec_acpi_dev) { > + dev_err(&wdev->dev, "Could not find EC ACPI device.\n"); > + return -ENODEV; > + } > + } > + > + input_dev = devm_input_allocate_device(&wdev->dev); > + if (!input_dev) > + return -ENOMEM; > + > + input_dev->name = "Lenovo Yoga Tablet Mode Control switch"; > + input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0"; > + input_dev->id.bustype = BUS_HOST; > + input_dev->dev.parent = &wdev->dev; > + > + input_set_capability(input_dev, EV_SW, SW_TABLET_MODE); > + > + err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL); > + if (err) { > + dev_err(&wdev->dev, > + "Could not set up input device keymap: %d\n", err); > + return err; > + } > + > + err = input_register_device(input_dev); > + if (err) { > + dev_err(&wdev->dev, > + "Could not register input device: %d\n", err); > + return err; > + } > + > + priv->input_dev = input_dev; > + dev_set_drvdata(&wdev->dev, priv); > + lenovo_ymc_trigger_ec(wdev, priv); > + return 0; > +} > + > +static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = { > + { .guid_string = LENOVO_YMC_EVENT_GUID }, > + { } > +}; > +MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table); > + > +static struct wmi_driver lenovo_ymc_driver = { > + .driver = { > + .name = "lenovo-ymc", > + }, > + .id_table = lenovo_ymc_wmi_id_table, > + .probe = lenovo_ymc_probe, > + .notify = lenovo_ymc_notify, > +}; > + > +module_wmi_driver(lenovo_ymc_driver); > + > +MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>"); > +MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver"); > +MODULE_LICENSE("GPL");
On Mon, Mar 20, 2023 at 6:05 PM Gergo Koteles <soyer@irl.hu> wrote: > > Hi Andrew, > > Thanks for picking this up. Sorry for the late reply. > I no longer think this driver should do the same workaround as ymc.exe > does, it shouldn't make non-WMI calls. > I think the 14ARB7 should be fixed in the BIOS to work like the other > Yogas with the same WMI IDs. > > So PATCH 1/2 and codes related to ec_trigger are unnecessary. > > I only have the 14ARB7 and I can't test this without the ec_trigger. > For this reason, I don't want to be the author of this module. > Could you be the author? > > The working C940, 14API reports are from > https://github.com/lukas-w/yoga-usage-mode module, which uses the same > WMI IDs. > > Regards, > Gergo > > Co-developed-by: Gergo Koteles <soyer@irl.hu> > Signed-off-by: Gergo Koteles <soyer@irl.hu> Hi Gergo, happy to hear from you! Now it makes sense why this never got submitted. I suppose the 0xAB method ID came from decompiling that ymc.exe as well? It looks like the github repo uses 0x01 which is what we found in the acpidump. I will remove that code like you say, make myself the module author, and add your Co-developed-by and Signed-off-by tags to the commit. Thanks, Andrew
Hi Gergo, On 3/21/23 02:05, Gergo Koteles wrote: > Hi Andrew, > > Thanks for picking this up. Sorry for the late reply. > I no longer think this driver should do the same workaround as ymc.exe does, it shouldn't make non-WMI calls. > I think the 14ARB7 should be fixed in the BIOS to work like the other Yogas with the same WMI IDs. > > So PATCH 1/2 and codes related to ec_trigger are unnecessary. Good to hear from you. May I ask why you think that the ec_trigger is unnecessary ? I agree that it should be limited to only models which need it, but if it is necessary on some models to make things work and if it is what the Lenovo Windows software does on this model, then I think we should do this in Linux too to make things work on models which need the EC trigger. In my experience waiting for a BIOS update to fix this properly is idle hope and such a BIOS update is very unlikely to happen. Lenovo seems to have worked around this in their Windows software with the EC trigger thing, so there is no reason for Lenovo to change the BIOS. > I only have the 14ARB7 and I can't test this without the ec_trigger. > For this reason, I don't want to be the author of this module. > Could you be the author? Note that making Andrew the primary author of the patch is fine regardless of if we keep the EC trigger or not. Regards, Hans > > The working C940, 14API reports are from https://github.com/lukas-w/yoga-usage-mode module, which uses the same WMI IDs. > > Regards, > Gergo > > Co-developed-by: Gergo Koteles <soyer@irl.hu> > Signed-off-by: Gergo Koteles <soyer@irl.hu> > > > On 2023. 03. 10. 5:17, Andrew Kallmeyer wrote: >> This WMI driver for the tablet mode control switch for Lenovo Yoga >> notebooks was originally written by Gergo Koteles. The mode is mapped to >> a SW_TABLET_MODE switch capable input device. >> >> I (Andrew) followed the suggestions that were posted in reply to Gergo's >> RFC patch to follow-up and get it merged. Additionally I merged the >> lenovo_ymc_trigger_ec function with the ideapad_trigger_ymc_next_read >> function since it was no longer external. I have also added the word >> "Tablet" to the driver description to hopefully make it more clear. >> >> As Gergo said: It should work with models like the Yoga C940, Ideapad >> flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7. >> The 14ARB7 model must trigger the EC to send mode change events. This >> might be a firmware bug. >> >> I have additionally tested this on the Yoga 7 14IAL7. >> >> Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/ >> Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com> >> Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com> >> --- >> drivers/platform/x86/Kconfig | 10 ++ >> drivers/platform/x86/Makefile | 1 + >> drivers/platform/x86/ideapad-laptop.h | 1 + >> drivers/platform/x86/lenovo-ymc.c | 177 ++++++++++++++++++++++++++ >> 4 files changed, 189 insertions(+) >> create mode 100644 drivers/platform/x86/lenovo-ymc.c >> >> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig >> index 5692385e2..858be0c65 100644 >> --- a/drivers/platform/x86/Kconfig >> +++ b/drivers/platform/x86/Kconfig >> @@ -470,6 +470,16 @@ config IDEAPAD_LAPTOP >> This is a driver for Lenovo IdeaPad netbooks contains drivers for >> rfkill switch, hotkey, fan control and backlight control. >> +config LENOVO_YMC >> + tristate "Lenovo Yoga Tablet Mode Control" >> + depends on ACPI_WMI >> + depends on INPUT >> + depends on IDEAPAD_LAPTOP >> + select INPUT_SPARSEKMAP >> + help >> + This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input >> + events for Lenovo Yoga notebooks. >> + >> config SENSORS_HDAPS >> tristate "Thinkpad Hard Drive Active Protection System (hdaps)" >> depends on INPUT >> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile >> index 1d3d1b025..10054cdea 100644 >> --- a/drivers/platform/x86/Makefile >> +++ b/drivers/platform/x86/Makefile >> @@ -63,6 +63,7 @@ obj-$(CONFIG_UV_SYSFS) += uv_sysfs.o >> # IBM Thinkpad and Lenovo >> obj-$(CONFIG_IBM_RTL) += ibm_rtl.o >> obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o >> +obj-$(CONFIG_LENOVO_YMC) += lenovo-ymc.o >> obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o >> obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o >> obj-$(CONFIG_THINKPAD_LMI) += think-lmi.o >> diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h >> index 7dd8ce027..2564cb1cd 100644 >> --- a/drivers/platform/x86/ideapad-laptop.h >> +++ b/drivers/platform/x86/ideapad-laptop.h >> @@ -35,6 +35,7 @@ enum { >> VPCCMD_W_FAN, >> VPCCMD_R_RF, >> VPCCMD_W_RF, >> + VPCCMD_W_YMC = 0x2A, >> VPCCMD_R_FAN = 0x2B, >> VPCCMD_R_SPECIAL_BUTTONS = 0x31, >> VPCCMD_W_BL_POWER = 0x33, >> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c >> new file mode 100644 >> index 000000000..e29ada1b4 >> --- /dev/null >> +++ b/drivers/platform/x86/lenovo-ymc.c >> @@ -0,0 +1,177 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +/* >> + * lenovo-ymc.c - Lenovo Yoga Mode Control driver >> + * >> + * Copyright © 2022 Gergo Koteles <soyer@irl.hu> >> + */ >> + >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> + >> +#include <linux/acpi.h> >> +#include <linux/dmi.h> >> +#include <linux/input.h> >> +#include <linux/input/sparse-keymap.h> >> +#include <linux/wmi.h> >> +#include "ideapad-laptop.h" >> + >> +#define LENOVO_YMC_EVENT_GUID "06129D99-6083-4164-81AD-F092F9D773A6" >> +#define LENOVO_YMC_QUERY_GUID "09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C" >> + >> +#define LENOVO_YMC_QUERY_INSTANCE 0 >> +#define LENOVO_YMC_QUERY_METHOD 0xAB >> + >> +static bool ec_trigger __read_mostly; >> +module_param(ec_trigger, bool, 0644); >> +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events"); >> + >> +static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = { >> + { >> + // Lenovo Yoga 7 14ARB7 >> + .matches = { >> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "82QF"), >> + }, >> + }, >> + { }, >> +}; >> + >> +struct lenovo_ymc_private { >> + struct input_dev *input_dev; >> + struct acpi_device *ec_acpi_dev; >> +}; >> + >> +static void lenovo_ymc_trigger_ec(struct wmi_device *wdev, struct lenovo_ymc_private *priv) >> +{ >> + int err; >> + >> + if (!ec_trigger || !priv || !priv->ec_acpi_dev) >> + return; >> + err = write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1); >> + if (err) >> + dev_warn(&wdev->dev, "Could not write YMC: %d\n", err); >> +} >> + >> +static const struct key_entry lenovo_ymc_keymap[] = { >> + // Laptop >> + { KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } }, >> + // Tablet >> + { KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } }, >> + // Drawing Board >> + { KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } }, >> + // Tent >> + { KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } }, >> + { KE_END }, >> +}; >> + >> +static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data) >> +{ >> + struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev); >> + >> + u32 input_val = 0; >> + struct acpi_buffer input = {sizeof(input_val), &input_val}; >> + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL}; >> + acpi_status status; >> + union acpi_object *obj; >> + int code; >> + >> + status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID, >> + LENOVO_YMC_QUERY_INSTANCE, >> + LENOVO_YMC_QUERY_METHOD, >> + &input, &output); >> + >> + if (ACPI_FAILURE(status)) { >> + dev_warn(&wdev->dev, >> + "Failed to evaluate query method: %s\n", >> + acpi_format_exception(status)); >> + return; >> + } >> + >> + obj = output.pointer; >> + >> + if (obj->type != ACPI_TYPE_INTEGER) { >> + dev_warn(&wdev->dev, >> + "WMI event data is not an integer\n"); >> + goto free_obj; >> + } >> + code = obj->integer.value; >> + >> + if (!sparse_keymap_report_event(priv->input_dev, code, 1, true)) >> + dev_warn(&wdev->dev, "Unknown key %d pressed\n", code); >> + >> +free_obj: >> + kfree(obj); >> + lenovo_ymc_trigger_ec(wdev, priv); >> +} >> + >> +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx) >> +{ >> + struct input_dev *input_dev; >> + struct lenovo_ymc_private *priv; >> + int err; >> + >> + ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table); >> + >> + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + if (ec_trigger) { >> + pr_debug("Lenovo YMC enable EC triggering.\n"); >> + priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1); >> + if (!priv->ec_acpi_dev) { >> + dev_err(&wdev->dev, "Could not find EC ACPI device.\n"); >> + return -ENODEV; >> + } >> + } >> + >> + input_dev = devm_input_allocate_device(&wdev->dev); >> + if (!input_dev) >> + return -ENOMEM; >> + >> + input_dev->name = "Lenovo Yoga Tablet Mode Control switch"; >> + input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0"; >> + input_dev->id.bustype = BUS_HOST; >> + input_dev->dev.parent = &wdev->dev; >> + >> + input_set_capability(input_dev, EV_SW, SW_TABLET_MODE); >> + >> + err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL); >> + if (err) { >> + dev_err(&wdev->dev, >> + "Could not set up input device keymap: %d\n", err); >> + return err; >> + } >> + >> + err = input_register_device(input_dev); >> + if (err) { >> + dev_err(&wdev->dev, >> + "Could not register input device: %d\n", err); >> + return err; >> + } >> + >> + priv->input_dev = input_dev; >> + dev_set_drvdata(&wdev->dev, priv); >> + lenovo_ymc_trigger_ec(wdev, priv); >> + return 0; >> +} >> + >> +static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = { >> + { .guid_string = LENOVO_YMC_EVENT_GUID }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table); >> + >> +static struct wmi_driver lenovo_ymc_driver = { >> + .driver = { >> + .name = "lenovo-ymc", >> + }, >> + .id_table = lenovo_ymc_wmi_id_table, >> + .probe = lenovo_ymc_probe, >> + .notify = lenovo_ymc_notify, >> +}; >> + >> +module_wmi_driver(lenovo_ymc_driver); >> + >> +MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>"); >> +MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver"); >> +MODULE_LICENSE("GPL"); >
Hi Andrew, On 2023. 03. 21. 3:14, Andrew Kallmeyer wrote: > On Mon, Mar 20, 2023 at 6:05 PM Gergo Koteles <soyer@irl.hu> wrote: >> >> Hi Andrew, >> >> Thanks for picking this up. Sorry for the late reply. >> I no longer think this driver should do the same workaround as ymc.exe >> does, it shouldn't make non-WMI calls. >> I think the 14ARB7 should be fixed in the BIOS to work like the other >> Yogas with the same WMI IDs. >> >> So PATCH 1/2 and codes related to ec_trigger are unnecessary. >> >> I only have the 14ARB7 and I can't test this without the ec_trigger. >> For this reason, I don't want to be the author of this module. >> Could you be the author? >> >> The working C940, 14API reports are from >> https://github.com/lukas-w/yoga-usage-mode module, which uses the same >> WMI IDs. >> >> Regards, >> Gergo >> >> Co-developed-by: Gergo Koteles <soyer@irl.hu> >> Signed-off-by: Gergo Koteles <soyer@irl.hu> > > Hi Gergo, happy to hear from you! > > Now it makes sense why this never got submitted. I suppose the 0xAB > method ID came from decompiling that ymc.exe as well? It looks like > the github repo uses 0x01 which is what we found in the acpidump. > I didn't decompile the ymc.exe, just watched the acpi/wmi trace logs, while stopped and started the ymc service, and figured out what it can do. After mode changes I saw the VPCW,VPCW,VPCR pattern, what I saw before in the ideapad-laptop.c as write_ec_cmd. Then tried with the unknown cmds, and the 0x2A worked. The 0xAB came from the object_id of debug_dump_wdg by mistake. [ 1.562801] wmi: 09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C: [ 1.562802] wmi: object_id: AB [ 1.562803] wmi: instance_count: 1 [ 1.562804] wmi: flags: 0x2 ACPI_WMI_METHOD The correct one is the 0x01 here also. [WMI, Dynamic, Provider("WmiProv"), Locale("MS\\0x409"), Description("LENOVO_GSENSOR_DATA class"), guid("{09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C}")] class LENOVO_GSENSOR_DATA { [key, read] string InstanceName; [read] boolean Active; [WmiMethodId(1), Implemented, Description("Mode Data")] void GetUsageMode([out, Description("Mode Data")] uint32 Data); [WmiMethodId(2), Implemented, Description("Get Xaxis Value")] void GetXaxisValue([out, Description("Get Xaxis Value")] uint32 Data); [WmiMethodId(3), Implemented, Description("Get Yaxis Value")] void GetYaxisValue([out, Description("Get Yaxis Value")] uint32 Data); [WmiMethodId(4), Implemented, Description("Get Zaxis Value")] void GetZaxisValue([out, Description("Get Zaxis Value")] uint32 Data); [WmiMethodId(5), Implemented, Description("Base to Ground")] void GetAngle4Value([out, Description("Base to Ground")] uint32 Data); [WmiMethodId(6), Implemented, Description("Screen to Ground")] void GetAngle5Value([out, Description("Screen to Ground")] uint32 Data); [WmiMethodId(7), Implemented, Description("Screen to Base")] void GetAngle6Value([out, Description("Screen to Base")] uint32 Data); }; It works with any id Method (WMAB, 3, NotSerialized) { Return (^^PCI0.LPC0.EC0.YGAM) /* \_SB_.PCI0.LPC0.EC0_.YGAM */ } I've found one thing, in this line priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1); the correct hw id is "VPC2004", not "VCP2004". With this modification, it works well. > I will remove that code like you say, make myself the module author, > and add your Co-developed-by and Signed-off-by tags to the commit. According to Hans, it's unrealistic that Lenovo will change this triggering behavior, so it can remain. Thanks, Gergo > > Thanks, > Andrew
Hi Hans, On 2023. 03. 21. 10:13, Hans de Goede wrote: > Hi Gergo, > > On 3/21/23 02:05, Gergo Koteles wrote: >> Hi Andrew, >> >> Thanks for picking this up. Sorry for the late reply. >> I no longer think this driver should do the same workaround as ymc.exe does, it shouldn't make non-WMI calls. >> I think the 14ARB7 should be fixed in the BIOS to work like the other Yogas with the same WMI IDs. >> >> So PATCH 1/2 and codes related to ec_trigger are unnecessary. > > Good to hear from you. May I ask why you think that the ec_trigger is unnecessary ? > Just the usual struggle for simplicity. > I agree that it should be limited to only models which need it, > but if it is necessary on some models to make things work and > if it is what the Lenovo Windows software does on this model, > then I think we should do this in Linux too to make things > work on models which need the EC trigger. > > In my experience waiting for a BIOS update to fix this properly > is idle hope and such a BIOS update is very unlikely to happen. > > Lenovo seems to have worked around this in their Windows software > with the EC trigger thing, so there is no reason for Lenovo > to change the BIOS. > You are right, it's unlikely they'll change this. Thanks for bringing me back to reality, Gergo >> I only have the 14ARB7 and I can't test this without the ec_trigger. >> For this reason, I don't want to be the author of this module. >> Could you be the author? > > Note that making Andrew the primary author of the patch is > fine regardless of if we keep the EC trigger or not. > > Regards, > > Hans > > > > >> >> The working C940, 14API reports are from https://github.com/lukas-w/yoga-usage-mode module, which uses the same WMI IDs. >> >> Regards, >> Gergo >> >> Co-developed-by: Gergo Koteles <soyer@irl.hu> >> Signed-off-by: Gergo Koteles <soyer@irl.hu> >> >> >> On 2023. 03. 10. 5:17, Andrew Kallmeyer wrote: >>> This WMI driver for the tablet mode control switch for Lenovo Yoga >>> notebooks was originally written by Gergo Koteles. The mode is mapped to >>> a SW_TABLET_MODE switch capable input device. >>> >>> I (Andrew) followed the suggestions that were posted in reply to Gergo's >>> RFC patch to follow-up and get it merged. Additionally I merged the >>> lenovo_ymc_trigger_ec function with the ideapad_trigger_ymc_next_read >>> function since it was no longer external. I have also added the word >>> "Tablet" to the driver description to hopefully make it more clear. >>> >>> As Gergo said: It should work with models like the Yoga C940, Ideapad >>> flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7. >>> The 14ARB7 model must trigger the EC to send mode change events. This >>> might be a firmware bug. >>> >>> I have additionally tested this on the Yoga 7 14IAL7. >>> >>> Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/ >>> Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com> >>> Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com> >>> --- >>> drivers/platform/x86/Kconfig | 10 ++ >>> drivers/platform/x86/Makefile | 1 + >>> drivers/platform/x86/ideapad-laptop.h | 1 + >>> drivers/platform/x86/lenovo-ymc.c | 177 ++++++++++++++++++++++++++ >>> 4 files changed, 189 insertions(+) >>> create mode 100644 drivers/platform/x86/lenovo-ymc.c >>> >>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig >>> index 5692385e2..858be0c65 100644 >>> --- a/drivers/platform/x86/Kconfig >>> +++ b/drivers/platform/x86/Kconfig >>> @@ -470,6 +470,16 @@ config IDEAPAD_LAPTOP >>> This is a driver for Lenovo IdeaPad netbooks contains drivers for >>> rfkill switch, hotkey, fan control and backlight control. >>> +config LENOVO_YMC >>> + tristate "Lenovo Yoga Tablet Mode Control" >>> + depends on ACPI_WMI >>> + depends on INPUT >>> + depends on IDEAPAD_LAPTOP >>> + select INPUT_SPARSEKMAP >>> + help >>> + This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input >>> + events for Lenovo Yoga notebooks. >>> + >>> config SENSORS_HDAPS >>> tristate "Thinkpad Hard Drive Active Protection System (hdaps)" >>> depends on INPUT >>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile >>> index 1d3d1b025..10054cdea 100644 >>> --- a/drivers/platform/x86/Makefile >>> +++ b/drivers/platform/x86/Makefile >>> @@ -63,6 +63,7 @@ obj-$(CONFIG_UV_SYSFS) += uv_sysfs.o >>> # IBM Thinkpad and Lenovo >>> obj-$(CONFIG_IBM_RTL) += ibm_rtl.o >>> obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o >>> +obj-$(CONFIG_LENOVO_YMC) += lenovo-ymc.o >>> obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o >>> obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o >>> obj-$(CONFIG_THINKPAD_LMI) += think-lmi.o >>> diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h >>> index 7dd8ce027..2564cb1cd 100644 >>> --- a/drivers/platform/x86/ideapad-laptop.h >>> +++ b/drivers/platform/x86/ideapad-laptop.h >>> @@ -35,6 +35,7 @@ enum { >>> VPCCMD_W_FAN, >>> VPCCMD_R_RF, >>> VPCCMD_W_RF, >>> + VPCCMD_W_YMC = 0x2A, >>> VPCCMD_R_FAN = 0x2B, >>> VPCCMD_R_SPECIAL_BUTTONS = 0x31, >>> VPCCMD_W_BL_POWER = 0x33, >>> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c >>> new file mode 100644 >>> index 000000000..e29ada1b4 >>> --- /dev/null >>> +++ b/drivers/platform/x86/lenovo-ymc.c >>> @@ -0,0 +1,177 @@ >>> +// SPDX-License-Identifier: GPL-2.0-or-later >>> +/* >>> + * lenovo-ymc.c - Lenovo Yoga Mode Control driver >>> + * >>> + * Copyright © 2022 Gergo Koteles <soyer@irl.hu> >>> + */ >>> + >>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >>> + >>> +#include <linux/acpi.h> >>> +#include <linux/dmi.h> >>> +#include <linux/input.h> >>> +#include <linux/input/sparse-keymap.h> >>> +#include <linux/wmi.h> >>> +#include "ideapad-laptop.h" >>> + >>> +#define LENOVO_YMC_EVENT_GUID "06129D99-6083-4164-81AD-F092F9D773A6" >>> +#define LENOVO_YMC_QUERY_GUID "09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C" >>> + >>> +#define LENOVO_YMC_QUERY_INSTANCE 0 >>> +#define LENOVO_YMC_QUERY_METHOD 0xAB >>> + >>> +static bool ec_trigger __read_mostly; >>> +module_param(ec_trigger, bool, 0644); >>> +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events"); >>> + >>> +static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = { >>> + { >>> + // Lenovo Yoga 7 14ARB7 >>> + .matches = { >>> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >>> + DMI_MATCH(DMI_PRODUCT_NAME, "82QF"), >>> + }, >>> + }, >>> + { }, >>> +}; >>> + >>> +struct lenovo_ymc_private { >>> + struct input_dev *input_dev; >>> + struct acpi_device *ec_acpi_dev; >>> +}; >>> + >>> +static void lenovo_ymc_trigger_ec(struct wmi_device *wdev, struct lenovo_ymc_private *priv) >>> +{ >>> + int err; >>> + >>> + if (!ec_trigger || !priv || !priv->ec_acpi_dev) >>> + return; >>> + err = write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1); >>> + if (err) >>> + dev_warn(&wdev->dev, "Could not write YMC: %d\n", err); >>> +} >>> + >>> +static const struct key_entry lenovo_ymc_keymap[] = { >>> + // Laptop >>> + { KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } }, >>> + // Tablet >>> + { KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } }, >>> + // Drawing Board >>> + { KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } }, >>> + // Tent >>> + { KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } }, >>> + { KE_END }, >>> +}; >>> + >>> +static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data) >>> +{ >>> + struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev); >>> + >>> + u32 input_val = 0; >>> + struct acpi_buffer input = {sizeof(input_val), &input_val}; >>> + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL}; >>> + acpi_status status; >>> + union acpi_object *obj; >>> + int code; >>> + >>> + status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID, >>> + LENOVO_YMC_QUERY_INSTANCE, >>> + LENOVO_YMC_QUERY_METHOD, >>> + &input, &output); >>> + >>> + if (ACPI_FAILURE(status)) { >>> + dev_warn(&wdev->dev, >>> + "Failed to evaluate query method: %s\n", >>> + acpi_format_exception(status)); >>> + return; >>> + } >>> + >>> + obj = output.pointer; >>> + >>> + if (obj->type != ACPI_TYPE_INTEGER) { >>> + dev_warn(&wdev->dev, >>> + "WMI event data is not an integer\n"); >>> + goto free_obj; >>> + } >>> + code = obj->integer.value; >>> + >>> + if (!sparse_keymap_report_event(priv->input_dev, code, 1, true)) >>> + dev_warn(&wdev->dev, "Unknown key %d pressed\n", code); >>> + >>> +free_obj: >>> + kfree(obj); >>> + lenovo_ymc_trigger_ec(wdev, priv); >>> +} >>> + >>> +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx) >>> +{ >>> + struct input_dev *input_dev; >>> + struct lenovo_ymc_private *priv; >>> + int err; >>> + >>> + ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table); >>> + >>> + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); >>> + if (!priv) >>> + return -ENOMEM; >>> + >>> + if (ec_trigger) { >>> + pr_debug("Lenovo YMC enable EC triggering.\n"); >>> + priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1); >>> + if (!priv->ec_acpi_dev) { >>> + dev_err(&wdev->dev, "Could not find EC ACPI device.\n"); >>> + return -ENODEV; >>> + } >>> + } >>> + >>> + input_dev = devm_input_allocate_device(&wdev->dev); >>> + if (!input_dev) >>> + return -ENOMEM; >>> + >>> + input_dev->name = "Lenovo Yoga Tablet Mode Control switch"; >>> + input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0"; >>> + input_dev->id.bustype = BUS_HOST; >>> + input_dev->dev.parent = &wdev->dev; >>> + >>> + input_set_capability(input_dev, EV_SW, SW_TABLET_MODE); >>> + >>> + err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL); >>> + if (err) { >>> + dev_err(&wdev->dev, >>> + "Could not set up input device keymap: %d\n", err); >>> + return err; >>> + } >>> + >>> + err = input_register_device(input_dev); >>> + if (err) { >>> + dev_err(&wdev->dev, >>> + "Could not register input device: %d\n", err); >>> + return err; >>> + } >>> + >>> + priv->input_dev = input_dev; >>> + dev_set_drvdata(&wdev->dev, priv); >>> + lenovo_ymc_trigger_ec(wdev, priv); >>> + return 0; >>> +} >>> + >>> +static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = { >>> + { .guid_string = LENOVO_YMC_EVENT_GUID }, >>> + { } >>> +}; >>> +MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table); >>> + >>> +static struct wmi_driver lenovo_ymc_driver = { >>> + .driver = { >>> + .name = "lenovo-ymc", >>> + }, >>> + .id_table = lenovo_ymc_wmi_id_table, >>> + .probe = lenovo_ymc_probe, >>> + .notify = lenovo_ymc_notify, >>> +}; >>> + >>> +module_wmi_driver(lenovo_ymc_driver); >>> + >>> +MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>"); >>> +MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver"); >>> +MODULE_LICENSE("GPL"); >> >
On Wed, Mar 22, 2023 at 1:39 PM Gergo Koteles <soyer@irl.hu> wrote: > > According to Hans, it's unrealistic that Lenovo will change this > triggering behavior, so it can remain. In that case I will leave you as the module and commit author but add your signed-off, unless you'd still prefer not to be the module author. You wrote most of this code, I only did a few code review changes :)
On 2023. 03. 22. 22:03, Andrew Kallmeyer wrote: > On Wed, Mar 22, 2023 at 1:39 PM Gergo Koteles <soyer@irl.hu> wrote: >> >> According to Hans, it's unrealistic that Lenovo will change this >> triggering behavior, so it can remain. > > In that case I will leave you as the module and commit author but add > your signed-off, unless you'd still prefer not to be the module > author. You wrote most of this code, I only did a few code review > changes :) Alright.
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 5692385e2..858be0c65 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -470,6 +470,16 @@ config IDEAPAD_LAPTOP This is a driver for Lenovo IdeaPad netbooks contains drivers for rfkill switch, hotkey, fan control and backlight control. +config LENOVO_YMC + tristate "Lenovo Yoga Tablet Mode Control" + depends on ACPI_WMI + depends on INPUT + depends on IDEAPAD_LAPTOP + select INPUT_SPARSEKMAP + help + This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input + events for Lenovo Yoga notebooks. + config SENSORS_HDAPS tristate "Thinkpad Hard Drive Active Protection System (hdaps)" depends on INPUT diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 1d3d1b025..10054cdea 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -63,6 +63,7 @@ obj-$(CONFIG_UV_SYSFS) += uv_sysfs.o # IBM Thinkpad and Lenovo obj-$(CONFIG_IBM_RTL) += ibm_rtl.o obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o +obj-$(CONFIG_LENOVO_YMC) += lenovo-ymc.o obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o obj-$(CONFIG_THINKPAD_LMI) += think-lmi.o diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h index 7dd8ce027..2564cb1cd 100644 --- a/drivers/platform/x86/ideapad-laptop.h +++ b/drivers/platform/x86/ideapad-laptop.h @@ -35,6 +35,7 @@ enum { VPCCMD_W_FAN, VPCCMD_R_RF, VPCCMD_W_RF, + VPCCMD_W_YMC = 0x2A, VPCCMD_R_FAN = 0x2B, VPCCMD_R_SPECIAL_BUTTONS = 0x31, VPCCMD_W_BL_POWER = 0x33, diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c new file mode 100644 index 000000000..e29ada1b4 --- /dev/null +++ b/drivers/platform/x86/lenovo-ymc.c @@ -0,0 +1,177 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * lenovo-ymc.c - Lenovo Yoga Mode Control driver + * + * Copyright © 2022 Gergo Koteles <soyer@irl.hu> + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/acpi.h> +#include <linux/dmi.h> +#include <linux/input.h> +#include <linux/input/sparse-keymap.h> +#include <linux/wmi.h> +#include "ideapad-laptop.h" + +#define LENOVO_YMC_EVENT_GUID "06129D99-6083-4164-81AD-F092F9D773A6" +#define LENOVO_YMC_QUERY_GUID "09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C" + +#define LENOVO_YMC_QUERY_INSTANCE 0 +#define LENOVO_YMC_QUERY_METHOD 0xAB + +static bool ec_trigger __read_mostly; +module_param(ec_trigger, bool, 0644); +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events"); + +static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = { + { + // Lenovo Yoga 7 14ARB7 + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), + DMI_MATCH(DMI_PRODUCT_NAME, "82QF"), + }, + }, + { }, +}; + +struct lenovo_ymc_private { + struct input_dev *input_dev; + struct acpi_device *ec_acpi_dev; +}; + +static void lenovo_ymc_trigger_ec(struct wmi_device *wdev, struct lenovo_ymc_private *priv) +{ + int err; + + if (!ec_trigger || !priv || !priv->ec_acpi_dev) + return; + err = write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1); + if (err) + dev_warn(&wdev->dev, "Could not write YMC: %d\n", err); +} + +static const struct key_entry lenovo_ymc_keymap[] = { + // Laptop + { KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } }, + // Tablet + { KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } }, + // Drawing Board + { KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } }, + // Tent + { KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } }, + { KE_END }, +}; + +static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data) +{ + struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev); + + u32 input_val = 0; + struct acpi_buffer input = {sizeof(input_val), &input_val}; + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL}; + acpi_status status; + union acpi_object *obj; + int code; + + status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID, + LENOVO_YMC_QUERY_INSTANCE, + LENOVO_YMC_QUERY_METHOD, + &input, &output); + + if (ACPI_FAILURE(status)) { + dev_warn(&wdev->dev, + "Failed to evaluate query method: %s\n", + acpi_format_exception(status)); + return; + } + + obj = output.pointer; + + if (obj->type != ACPI_TYPE_INTEGER) { + dev_warn(&wdev->dev, + "WMI event data is not an integer\n"); + goto free_obj; + } + code = obj->integer.value; + + if (!sparse_keymap_report_event(priv->input_dev, code, 1, true)) + dev_warn(&wdev->dev, "Unknown key %d pressed\n", code); + +free_obj: + kfree(obj); + lenovo_ymc_trigger_ec(wdev, priv); +} + +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx) +{ + struct input_dev *input_dev; + struct lenovo_ymc_private *priv; + int err; + + ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table); + + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + if (ec_trigger) { + pr_debug("Lenovo YMC enable EC triggering.\n"); + priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1); + if (!priv->ec_acpi_dev) { + dev_err(&wdev->dev, "Could not find EC ACPI device.\n"); + return -ENODEV; + } + } + + input_dev = devm_input_allocate_device(&wdev->dev); + if (!input_dev) + return -ENOMEM; + + input_dev->name = "Lenovo Yoga Tablet Mode Control switch"; + input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0"; + input_dev->id.bustype = BUS_HOST; + input_dev->dev.parent = &wdev->dev; + + input_set_capability(input_dev, EV_SW, SW_TABLET_MODE); + + err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL); + if (err) { + dev_err(&wdev->dev, + "Could not set up input device keymap: %d\n", err); + return err; + } + + err = input_register_device(input_dev); + if (err) { + dev_err(&wdev->dev, + "Could not register input device: %d\n", err); + return err; + } + + priv->input_dev = input_dev; + dev_set_drvdata(&wdev->dev, priv); + lenovo_ymc_trigger_ec(wdev, priv); + return 0; +} + +static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = { + { .guid_string = LENOVO_YMC_EVENT_GUID }, + { } +}; +MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table); + +static struct wmi_driver lenovo_ymc_driver = { + .driver = { + .name = "lenovo-ymc", + }, + .id_table = lenovo_ymc_wmi_id_table, + .probe = lenovo_ymc_probe, + .notify = lenovo_ymc_notify, +}; + +module_wmi_driver(lenovo_ymc_driver); + +MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>"); +MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver"); +MODULE_LICENSE("GPL");
This WMI driver for the tablet mode control switch for Lenovo Yoga notebooks was originally written by Gergo Koteles. The mode is mapped to a SW_TABLET_MODE switch capable input device. I (Andrew) followed the suggestions that were posted in reply to Gergo's RFC patch to follow-up and get it merged. Additionally I merged the lenovo_ymc_trigger_ec function with the ideapad_trigger_ymc_next_read function since it was no longer external. I have also added the word "Tablet" to the driver description to hopefully make it more clear. As Gergo said: It should work with models like the Yoga C940, Ideapad flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7. The 14ARB7 model must trigger the EC to send mode change events. This might be a firmware bug. I have additionally tested this on the Yoga 7 14IAL7. Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/ Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com> Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com> --- drivers/platform/x86/Kconfig | 10 ++ drivers/platform/x86/Makefile | 1 + drivers/platform/x86/ideapad-laptop.h | 1 + drivers/platform/x86/lenovo-ymc.c | 177 ++++++++++++++++++++++++++ 4 files changed, 189 insertions(+) create mode 100644 drivers/platform/x86/lenovo-ymc.c