Hi Philipp,
On 11/10/22 21:02, Philipp Jungkamp wrote:
> On Mon, 2022-09-19 at 08:54 +0100, Hans de Goede wrote:
>> Hi again,
>>
>> On 9/11/22 17:18, Hans de Goede wrote:
>>> Hi Philipp,
>>>
>>> On 9/11/22 18:04, Philipp Jungkamp wrote:
>>>> Create an input device for WMI events corresponding to some
>>>> special
>>>> keys on the 'Lenovo Yoga' line.
>>>>
>>>> This include the 3 keys to the right on the 'Lenovo Yoga9 14IAP7'
>>>> and
>>>> additionally the 'Lenovo Support' and 'Lenovo Favorites' (star
>>>> with 'S'
>>>> inside) in the fn key row as well as the event emitted on 'Fn+R'
>>>> which
>>>> toggles between 60Hz and 90Hz display refresh rate on windows.
>>>>
>>>> Signed-off-by: Philipp Jungkamp <p.jungkamp@xxxxxxx>
>>>> ---
>>>> I found this patch by poking in the DSDT. I have not submitted
>>>> any
>>>> notable patches yet and hope you can help me improve in case I
>>>> make
>>>> unfortunate choices during submission.
>>>
>>> No worries at a first glance (I have not looked at this in any
>>> detail yet) this looks pretty good for a first submission.
>>>
>>> And thank you for contributing to the Linux kernel!
>>>
>>>
>>>> Philipp Jungkamp
>>>>
>>>> drivers/platform/x86/Kconfig | 13 +++
>>>> drivers/platform/x86/Makefile | 1 +
>>>> drivers/platform/x86/ideapad-wmi.c | 153
>>>> +++++++++++++++++++++++++++++
>>>> 3 files changed, 167 insertions(+)
>>>> create mode 100644 drivers/platform/x86/ideapad-wmi.c
>>>>
>>>> diff --git a/drivers/platform/x86/Kconfig
>>>> b/drivers/platform/x86/Kconfig
>>>> index f2f98e942cf2..e7c5148e5cb4 100644
>>>> --- a/drivers/platform/x86/Kconfig
>>>> +++ b/drivers/platform/x86/Kconfig
>>>> @@ -140,6 +140,19 @@ config YOGABOOK_WMI
>>>> To compile this driver as a module, choose M here: the
>>>> module will
>>>> be called lenovo-yogabook-wmi.
>>>>
>>>> +config IDEAPAD_WMI
>>>> + tristate "Lenovo IdeaPad WMI Fn Keys"
>>>> + depends on ACPI_WMI
>>>> + depends on INPUT
>>>> + select INPUT_SPARSEKMAP
>>>> + help
>>>> + Say Y here if you want to receive key presses from some
>>>> lenovo
>>>> + specific keys. (Star Key, Support Key, Virtual
>>>> Background,
>>>> + Dark Mode Toggle, ...)
>>>> +
>>>> + To compile this driver as a module, choose M here: the
>>>> module will
>>>> + be called ideapad-wmi.
>>>> +
>>>> config ACERHDF
>>>> tristate "Acer Aspire One temperature and fan driver"
>>>> depends on ACPI && THERMAL
>>>> diff --git a/drivers/platform/x86/Makefile
>>>> b/drivers/platform/x86/Makefile
>>>> index 5a428caa654a..d8bec884d6bc 100644
>>>> --- a/drivers/platform/x86/Makefile
>>>> +++ b/drivers/platform/x86/Makefile
>>>> @@ -16,6 +16,7 @@ obj-
>>>> $(CONFIG_PEAQ_WMI) += peaq-wmi.o
>>>> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
>>>> obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
>>>> obj-$(CONFIG_YOGABOOK_WMI) += lenovo-yogabook-wmi.o
>>>> +obj-$(CONFIG_IDEAPAD_WMI) += ideapad-wmi.o
>>>>
>>>> # Acer
>>>> obj-$(CONFIG_ACERHDF) += acerhdf.o
>>>> diff --git a/drivers/platform/x86/ideapad-wmi.c
>>>> b/drivers/platform/x86/ideapad-wmi.c
>>>> new file mode 100644
>>>> index 000000000000..38f7b3d0c171
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/ideapad-wmi.c
>>>> @@ -0,0 +1,153 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>>> +/*
>>>> + * ideapad-wmi.c - Ideapad WMI fn keys driver
>>>> + *
>>>> + * Copyright (C) 2022 Philipp Jungkamp <p.jungkamp@xxxxxxx>
>>>> + */
>>>> +
>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>> +
>>>> +#include <linux/acpi.h>
>>>> +#include <linux/input.h>
>>>> +#include <linux/input/sparse-keymap.h>
>>>> +#include <linux/list.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/wmi.h>
>>>> +
>>>> +#define IDEAPAD_FN_KEY_EVENT_GUID "8FC0DE0C-B4E4-43FD-B0F3-
>>>> 8871711C1294"
>>>
>>> At a first hunch (basically huh, don't we have a driver for that
>>> already?) I grepped through the kernel sources and found:
>>>
>>> drivers/platform/x86/ideapad-laptop.c
>>>
>>> can you see if you can make things work with that driver?
>>
>> So I have taken a quick look at this and it seems to me that this
>> really should be able to work with the existing ideapad-laptop.c code
>> ?
>>
>> For starters you could add a debug printk / dev_info to this block,
>>
>> #if IS_ENABLED(CONFIG_ACPI_WMI)
>> for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
>> status =
>> wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
>>
>> ideapad_wmi_notify, priv);
>> if (ACPI_SUCCESS(status)) {
>> priv->fnesc_guid =
>> ideapad_wmi_fnesc_events[i];
>> break;
>> }
>> }
>>
>> if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
>> err = -EIO;
>> goto notification_failed_wmi;
>> }
>> #endif
>>
>> checking which event GUID ideapad-laptop binds to on your laptop.
>> Assuming that
>> it does bind to the GUID this driver is binding to too, then it would
>> be
>> a matter of extending the existing ideapad_wmi_notify() to do the
>> same
>> as your notify function in this stand-alone driver. Note you can get
>> the
>> the equivalend of the union acpi_object *data argument in your wmi
>> handler
>> by calling wmi_get_event_data().
>>
>> Regards,
>>
>> Hans
>>
>
> Hello Hans,
>
> I did actually start by doing that.
> The problem lies with the wmi_get_event_data() function not working
> for my ACPI.
>
> wmi_get_event_data() takes the event notify_id as input and is supposed
> to give the data associated with the event back. This occures by
> calling _WED on the first WMI block that contains said notify_id.
>
> drivers/platform/x86/wmi.c:657:
> list_for_each_entry(wblock, &wmi_block_list, list) {
> struct guid_block *gblock = &wblock->gblock;
>
> if ((gblock->flags & ACPI_WMI_EVENT) && gblock-
>> notify_id == event)
> return get_event_data(wblock, out);
> }
>
> The ACPI of the Lenovo Yoga 9 14IAP7 contains multiple WMI event
> blocks hat contain the same notify_id 0xD0.
>
> Here are two of the four WMI objects found in the DSDT:
>
> in _WDG of block WMIY:
> 06129D99-6083-4164-81AD-F092F9D773A6:
> notify_id: 0xD0
> instance_count: 1
> flags: 0x8 ACPI_WMI_EVENT
>
> in _WDG of block WMIU:
> 8FC0DE0C-B4E4-43FD-B0F3-8871711C1294:
> notify_id: 0xD0
> instance_count: 1
> flags: 0x8 ACPI_WMI_EVENT
>
> These event block belong to different WMI devices and report
> unrelated values from different _WED handlers. WMIY for example
> triggers its event on "mode changes", e.g. laptop/tablet/tent based
> on the accelometers/hinge.
> WMIU is the WMI block with the event which reports the special keys
> I'm interested in.
>
> WMIY comes before WMIU in the wmi_block_list.
> Calling wmi_get_event_data() in ideapad_wmi_notify() calls the wrong
> _WED (the one of WMIY) and thus returns the wrong event data.
>
> I noticed that the wmi_driver interface does not incur the problem
> with the event because it binds to a wmi_block and calls the _WED
> directly without searching through other WMI devices.
Right, I see in that case I guess that using the wmi_driver
interface does make sense.
> I thought of changing the signature of wmi_get_event_data() to include
> the GUID of the correct WMI block, but chose wmi_driver instead.
> Would you consider adding a wmi_get_event_data_for_guid() function to
> the wmi module and using that in the ideapad_wmi_notify function to be
> a better solution than the one in the patch presented here?
So the problem is that ideapad-laptop is using the old interface
and it does:
status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
ideapad_wmi_notify, priv);
And then the code to call that notifier in wmi.c goes like this:
if (test_bit(WMI_PROBED, &wblock->flags) && wblock->dev.dev.driver) {
struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
struct acpi_buffer evdata = { ACPI_ALLOCATE_BUFFER, NULL };
acpi_status status;
if (!driver->no_notify_data) {
status = get_event_data(wblock, &evdata);
if (ACPI_FAILURE(status)) {
dev_warn(&wblock->dev.dev, "failed to get event data\n");
return;
}
}
if (driver->notify)
driver->notify(&wblock->dev, evdata.pointer);
kfree(evdata.pointer);
} else if (wblock->handler) {
/* Legacy handler */
wblock->handler(event, wblock->handler_data);
}
So if we move ahead with your new style wmi-driver for GUID
8FC0DE0C-B4E4-43FD-B0F3-8871711C1294.
Then we hit the "true" path of the if and the legacy handler
(registered by ideapad-laptop) will never trigger.
I guess you could argue that this is a feature since your
new-style driver "replaces" ideapad-laptop, but ending up with
2 drivers for ideapad laptops / for GUID
8FC0DE0C-B4E4-43FD-B0F3-8871711C1294 and them having them have
to coordinate to decide which one to load sounds less then ideal.
So I think that your idea to add a wmi_get_event_data_for_guid()
function to wmi.c is actually a good idea.
Can you give that a try please and see how that goes ?
Regards,
Hans
