Hi,
On 11/14/22 15:41, Philipp Jungkamp wrote:
> The event data of the WMI event 0xD0, which is assumed to be the
> fn_lock, is used to indicate several special keys on newer Yoga 7/9
> laptops.
>
> The notify_id 0xD0 is non-unique in the DSDT of the Yoga 9 14IAP7, this
> causes wmi_get_event_data() to report wrong values.
> Port the ideapad-laptop WMI code to the wmi bus infrastructure which
> does not suffer from the shortcomings of wmi_get_event_data().
>
> Signed-off-by: Philipp Jungkamp <p.jungkamp@xxxxxxx>
> ---
> Hello,
>
> is this about right? It works for me.
>
> What I don't really like here is the dev_set_drvdata() which takes a non-const
> void * and I pass it a const pointer. I do cast the value of dev_get_drvdata()
> back to a const pointer, but this seems rather ugly.
> I preferred it over allocating a single int for the device or casting an enum
> to a void *. This additionally removes the need for a remove funtion.
I decided just take quick peek and I think the cleanest solution here would
be to add a driver-data struct with the lookup result of:
struct ideapad_private *priv = container_of(wdrv,
struct ideapad_private,
wmi_drv);
Stored in there + the enum value and then alloc it with devm_kzalloc
to avoid the need for a remove callback.
I'm also wondering if we could then maybe not move some other variables
only used in the wmi_notify callback to that driver-data struct ?
(I did not check if there are any candidates).
Also this is going to need a big fat warning (comment) that the cute trick
with registering a wmi_driver struct embedded inside the platform_driver
data struct very much relies on there being only one platform_device
instance to which the platform_driver will bind ever, otherwise
we will get multiple wmi_driver's registered for the same WMI GUIDs
and then the container-off will likely return the driver-data of the
first platform device ...
Which makes me wonder if it would not be cleaner to just use a global
pointer for this ? Note this is an honest open question.
Actually since the platform_device gets instantiated by the ACPI
core there is no guarantee there will be only 1. So I think that
the container_of on the wmi-driver trick needs to go, instead
introduce:
1. A global ideapad_private_data_mutex mutex
2. A global ideapad_private_data pointer
And:
1. In ideapad_acpi_add:
lock the mutex
check that ideapad_private_data is not already set and if it is bail with an error
set ideapad_private_data
unlock the mutex
2. in ideapad_acpi_remove:
lock the mutex
clear the golbal pointer
unlock the mutex
3. In the wmi-driver's notify method:
lock the mutex
check ideapad_private_data is not NULL
process event
unlock the mutex
4. replace module_platform_driver with normal module init + exit
functions which register both drivers / unregister both drivers.
I believe that this will be a more clean approach then the embedded
wmi_driver struct cuteness.
Regards,
Hans
>
> Regards,
> Philipp
>
> drivers/platform/x86/ideapad-laptop.c | 109 +++++++++++++++++++-------
> 1 file changed, 80 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 33b3dfdd1b08..6d35a9e961cf 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -30,6 +30,7 @@
> #include <linux/seq_file.h>
> #include <linux/sysfs.h>
> #include <linux/types.h>
> +#include <linux/wmi.h>
>
> #include <acpi/video.h>
>
> @@ -38,10 +39,19 @@
> #define IDEAPAD_RFKILL_DEV_NUM 3
>
> #if IS_ENABLED(CONFIG_ACPI_WMI)
> -static const char *const ideapad_wmi_fnesc_events[] = {
> - "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", /* Yoga 3 */
> - "56322276-8493-4CE8-A783-98C991274F5E", /* Yoga 700 */
> - "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", /* Legion 5 */
> +enum ideapad_wmi_event_type {
> + IDEAPAD_WMI_EVENT_ESC,
> + IDEAPAD_WMI_EVENT_FN_KEYS,
> +};
> +
> +enum ideapad_wmi_event_type ideapad_wmi_esc = IDEAPAD_WMI_EVENT_ESC,
> +enum ideapad_wmi_event_type ideapad_wmi_fn_keys = IDEAPAD_WMI_EVENT_FN_KEYS;
> +
> +static const struct wmi_device_id ideapad_wmi_id_table[] = {
> + { "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_esc }, /* Yoga 3 */
> + { "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_esc }, /* Yoga 700 */
> + { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_fn_keys }, /* Legion 5 */
> + {}
> };
> #endif
>
> @@ -130,7 +140,7 @@ struct ideapad_private {
> struct ideapad_dytc_priv *dytc;
> struct dentry *debug;
> unsigned long cfg;
> - const char *fnesc_guid;
> + struct wmi_driver wmi_drv;
> struct {
> bool conservation_mode : 1;
> bool dytc : 1;
> @@ -1074,6 +1084,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
> /*
> * input device
> */
> +#define IDEAPAD_WMI_KEY 0x100
> static const struct key_entry ideapad_keymap[] = {
> { KE_KEY, 6, { KEY_SWITCHVIDEOMODE } },
> { KE_KEY, 7, { KEY_CAMERA } },
> @@ -1087,6 +1098,26 @@ static const struct key_entry ideapad_keymap[] = {
> { KE_KEY, 66, { KEY_TOUCHPAD_OFF } },
> { KE_KEY, 67, { KEY_TOUCHPAD_ON } },
> { KE_KEY, 128, { KEY_ESC } },
> +
> + /*
> + * WMI keys
> + */
> +
> + /* FnLock (handled by the firmware) */
> + { KE_IGNORE, 0x02 | IDEAPAD_WMI_KEY },
> + /* Customizable Lenovo Hotkey ("star" with 'S' inside) */
> + { KE_KEY, 0x01 | IDEAPAD_WMI_KEY, { KEY_FAVORITES } },
> + /* Dark mode toggle */
> + { KE_KEY, 0x13 | IDEAPAD_WMI_KEY, { KEY_PROG1 } },
> + /* Sound profile switch */
> + { KE_KEY, 0x12 | IDEAPAD_WMI_KEY, { KEY_PROG2 } },
> + /* Lenovo Virtual Background application */
> + { KE_KEY, 0x28 | IDEAPAD_WMI_KEY, { KEY_PROG3 } },
> + /* Lenovo Support */
> + { KE_KEY, 0x27 | IDEAPAD_WMI_KEY, { KEY_HELP } },
> + /* Refresh Rate Toggle */
> + { KE_KEY, 0x0a | IDEAPAD_WMI_KEY, { KEY_DISPLAYTOGGLE } },
> +
> { KE_END },
> };
>
> @@ -1491,25 +1522,47 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
> }
>
> #if IS_ENABLED(CONFIG_ACPI_WMI)
> -static void ideapad_wmi_notify(u32 value, void *context)
> +static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context)
> {
> - struct ideapad_private *priv = context;
> + dev_set_drvdata(&wdev->dev, (void *) context);
> + return 0;
> +}
> +
> +static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> +{
> + struct wmi_driver *wdrv = container_of(wdev->dev.driver,
> + struct wmi_driver,
> + driver);
> + struct ideapad_private *priv = container_of(wdrv,
> + struct ideapad_private,
> + wmi_drv);
> + const enum ideapad_wmi_event_type *event = dev_get_drvdata(&wdev->dev);
> unsigned long result;
>
> - switch (value) {
> - case 128:
> - ideapad_input_report(priv, value);
> + switch (*event) {
> + case IDEAPAD_WMI_EVENT_ESC:
> + ideapad_input_report(priv, 128);
> break;
> - case 208:
> + case IDEAPAD_WMI_EVENT_FN_KEYS:
> if (!eval_hals(priv->adev->handle, &result)) {
> bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
>
> exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
> }
> +
> + if (data->type != ACPI_TYPE_INTEGER) {
> + dev_warn(&wdev->dev,
> + "WMI event data is not an integer\n");
> + break;
> + }
> +
> + dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
> + data->integer.value);
> +
> + ideapad_input_report(priv,
> + data->integer.value | IDEAPAD_WMI_KEY);
> +
> break;
> - default:
> - dev_info(&priv->platform_device->dev,
> - "Unknown WMI event: %u\n", value);
> }
> }
> #endif
> @@ -1671,25 +1724,24 @@ static int ideapad_acpi_add(struct platform_device *pdev)
> }
>
> #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;
> - }
> - }
> + priv->wmi_drv = (struct wmi_driver) {
> + .driver = {
> + .name = "ideapad-wmi-fn-keys",
> + },
> + .id_table = ideapad_wmi_id_table,
> + .probe = ideapad_wmi_probe,
> + .notify = ideapad_wmi_notify,
> + };
>
> - if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
> - err = -EIO;
> - goto notification_failed_wmi;
> - }
> + err = wmi_driver_register(&priv->wmi_drv);
> + if (err)
> + goto register_failed_wmi;
> #endif
>
> return 0;
>
> #if IS_ENABLED(CONFIG_ACPI_WMI)
> -notification_failed_wmi:
> +register_failed_wmi:
> acpi_remove_notify_handler(priv->adev->handle,
> ACPI_DEVICE_NOTIFY,
> ideapad_acpi_notify);
> @@ -1720,8 +1772,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
> int i;
>
> #if IS_ENABLED(CONFIG_ACPI_WMI)
> - if (priv->fnesc_guid)
> - wmi_remove_notify_handler(priv->fnesc_guid);
> + wmi_driver_unregister(&priv->wmi_drv);
> #endif
>
> acpi_remove_notify_handler(priv->adev->handle,
> --
> 2.38.1
>
