Hi, I ended up looking at this because of: https://patchwork.kernel.org/project/platform-driver-x86/patch/2655386.mvXUDI8C0e@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ so here is a full review. I have just pushed a slightly modified version of the above patch to: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes see: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=fixes&id=a3c272fae1468984d2ec4499d665ad8989d18388 this should also answer the concerns you mentioned in a previous email about the : 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); } part, since this will now be behind a DMI id based allow list, since doing this everywhere clearly is a bad idea. Also since you indicate that data->integer.value == 0x02 for FnLock toggle, I believe that in your new code, this should look something like this: static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data) { ... switch (wpriv->event) { case IDEAPAD_WMI_EVENT_ESC: ideapad_input_report(priv, 128); break; case IDEAPAD_WMI_EVENT_FN_KEYS: 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); if (priv->features.set_fn_lock_led && data->integer.value == 0x02 && !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); } ideapad_input_report(priv, data->integer.value | IDEAPAD_WMI_KEY); break; } Note I have also pushed some other pending ideapad-laptop changes to my review-hans branch, so please base v5 on top of my latest review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans I have some more comments inline below: On 11/15/22 01:09, 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> > --- > This now uses the WMI bus infrastructure with both the platform driver and the > wmi driver registered on module load. Is the synchronization used here > sufficient? Yes this looks good to me, thank you for doing it this way. > Thanks for all the help! You're welcome. Some small remarks in line. > > Regards, > Philipp Jungkamp > > drivers/platform/x86/ideapad-laptop.c | 240 ++++++++++++++++++++------ > 1 file changed, 188 insertions(+), 52 deletions(-) > > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c > index 33b3dfdd1b08..803bf5cf50f9 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,13 @@ > #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, > +}; > + > +struct ideapad_wmi_private { > + enum ideapad_wmi_event_type event; > }; > #endif > > @@ -130,7 +134,6 @@ struct ideapad_private { > struct ideapad_dytc_priv *dytc; > struct dentry *debug; > unsigned long cfg; > - const char *fnesc_guid; > struct { > bool conservation_mode : 1; > bool dytc : 1; > @@ -156,6 +159,42 @@ static bool allow_v4_dytc; > module_param(allow_v4_dytc, bool, 0444); > MODULE_PARM_DESC(allow_v4_dytc, "Enable DYTC version 4 platform-profile support."); > > +/* > + * shared data > + */ > + > +static struct ideapad_private *ideapad_shared; > +static DEFINE_MUTEX(ideapad_shared_mutex); > + > +static int ideapad_shared_init(struct ideapad_private *priv) > +{ > + int ret; > + > + mutex_lock(&ideapad_shared_mutex); > + > + if (!ideapad_shared) { > + ideapad_shared = priv; > + ret = 0; > + } else { > + dev_warn(&priv->adev->dev, "found multiple platform devices\n"); > + ret = -EINVAL; > + } > + > + mutex_unlock(&ideapad_shared_mutex); > + > + return ret; > +} > + > +static void ideapad_shared_exit(struct ideapad_private *priv) > +{ > + mutex_lock(&ideapad_shared_mutex); > + > + if (ideapad_shared == priv) > + ideapad_shared = NULL; > + > + mutex_unlock(&ideapad_shared_mutex); > +} > + > /* > * ACPI Helpers > */ > @@ -1074,6 +1113,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 +1127,28 @@ 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 }, > + /* Esc (handled by the firmware) */ > + { KE_IGNORE, 0x03 | 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 }, > }; > > @@ -1490,30 +1552,6 @@ 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) > -{ > - struct ideapad_private *priv = context; > - unsigned long result; > - > - switch (value) { > - case 128: > - ideapad_input_report(priv, value); > - break; > - case 208: > - 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); > - } > - break; > - default: > - dev_info(&priv->platform_device->dev, > - "Unknown WMI event: %u\n", value); > - } > -} > -#endif > - > /* > * Some ideapads have a hardware rfkill switch, but most do not have one. > * Reading VPCCMD_R_RF always results in 0 on models without a hardware rfkill, > @@ -1589,6 +1627,95 @@ static void ideapad_check_features(struct ideapad_private *priv) > } > } > > +#if IS_ENABLED(CONFIG_ACPI_WMI) > +/* > + * WMI driver > + */ > +static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context) > +{ > + struct ideapad_wmi_private *wpriv; > + > + wpriv = devm_kzalloc(&wdev->dev, sizeof(*wpriv), GFP_KERNEL); > + if (!wpriv) > + return -ENOMEM; > + > + *wpriv = *(struct ideapad_wmi_private *)context; > + > + dev_set_drvdata(&wdev->dev, wpriv); > + return 0; > +} > + > +static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data) > +{ > + struct ideapad_wmi_private *wpriv = dev_get_drvdata(&wdev->dev); > + struct ideapad_private *priv; > + unsigned long result; > + > + mutex_lock(&ideapad_shared_mutex); > + > + priv = ideapad_shared; > + if (!priv) > + goto unlock; > + > + switch (wpriv->event) { > + case IDEAPAD_WMI_EVENT_ESC: > + ideapad_input_report(priv, 128); > + break; > + 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; > + } > +unlock: > + mutex_unlock(&ideapad_shared_mutex); > +} > + > +struct ideapad_wmi_private ideapad_wmi_context_esc = { > + .event = IDEAPAD_WMI_EVENT_ESC > +}; > + > +struct ideapad_wmi_private ideapad_wmi_context_fn_keys = { > + .event = IDEAPAD_WMI_EVENT_FN_KEYS > +}; > + > +static const struct wmi_device_id ideapad_wmi_ids[] = { > + { "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_context_esc }, /* Yoga 3 */ > + { "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_context_esc }, /* Yoga 700 */ > + { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* Legion 5 */ > + {}, > +}; > +MODULE_DEVICE_TABLE(wmi, ideapad_wmi_ids); > + > +static struct wmi_driver ideapad_wmi_driver = { > + .driver = { > + .name = "ideapad_wmi", > + }, > + .id_table = ideapad_wmi_ids, > + .probe = ideapad_wmi_probe, > + .notify = ideapad_wmi_notify, > +}; > + Please add the following here: static int ideapad_wmi_register_driver(void) { return wmi_driver_register(&ideapad_wmi_driver); } static void ideapad_wmi_unregister_driver(void) { return wmi_driver_unregister(&ideapad_wmi_driver); } #else static inline int ideapad_wmi_register_driver(void) { return 0; } static inline void ideapad_wmi_unregister_driver(void) { } > +#endif > + > +/* > + * ACPI driver > + */ > static int ideapad_acpi_add(struct platform_device *pdev) > { > struct acpi_device *adev = ACPI_COMPANION(&pdev->dev); > @@ -1670,30 +1797,16 @@ static int ideapad_acpi_add(struct platform_device *pdev) > goto notification_failed; > } > > -#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 > + err = ideapad_shared_init(priv); > + if (err) > + goto shared_init_failed; > > return 0; > > -#if IS_ENABLED(CONFIG_ACPI_WMI) > -notification_failed_wmi: > +shared_init_failed: > acpi_remove_notify_handler(priv->adev->handle, > ACPI_DEVICE_NOTIFY, > ideapad_acpi_notify); > -#endif > > notification_failed: > ideapad_backlight_exit(priv); > @@ -1719,10 +1832,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev) > struct ideapad_private *priv = dev_get_drvdata(&pdev->dev); > int i; > > -#if IS_ENABLED(CONFIG_ACPI_WMI) > - if (priv->fnesc_guid) > - wmi_remove_notify_handler(priv->fnesc_guid); > -#endif > + ideapad_shared_exit(priv); > > acpi_remove_notify_handler(priv->adev->handle, > ACPI_DEVICE_NOTIFY, > @@ -1774,7 +1884,33 @@ static struct platform_driver ideapad_acpi_driver = { > }, > }; > > -module_platform_driver(ideapad_acpi_driver); > +static int __init ideapad_laptop_init(void) > +{ > + int err; > + > +#if IS_ENABLED(CONFIG_ACPI_WMI) > + err = wmi_driver_register(&ideapad_wmi_driver); > + if (err) > + return err; > +#endif And use ideapad_wmi_register_driver() here and drop the #ifdef-s. This using a little helper in the existing/main #ifdef block + static inline no-op versions in a #else there is a standard pattern in the kernel to avoid sprinkling #ifdef-s everywhere. > + > + err = platform_driver_register(&ideapad_acpi_driver); > + if (err) > + return err; This needs to be: if (err) { ideapad_wmi__driver_unregister(); return err; } And the unregister here is also much cleaner with the wrappers ,avoiding another #ifdef :) > + > + return 0; > +} > +module_init(ideapad_laptop_init) > + > +static void __exit ideapad_laptop_exit(void) > +{ > +#if IS_ENABLED(CONFIG_ACPI_WMI) > + wmi_driver_unregister(&ideapad_wmi_driver); > +#endif And use ideapad_wmi__driver_unregister here too. Apart from my few small remarks above this looks good and I expect that I can merge a v5 addressing these remarks pretty quickly (so in time for the 6.2 merge window). Regards, Hans > + > + platform_driver_unregister(&ideapad_acpi_driver); > +} > +module_exit(ideapad_laptop_exit) > > MODULE_AUTHOR("David Woodhouse <dwmw2@xxxxxxxxxxxxx>"); > MODULE_DESCRIPTION("IdeaPad ACPI Extras"); > -- > 2.38.1 >
