| Message ID | 2176493.Icojqenx9y@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: Install notify or event handlers in ACPI button drivers | expand |
On 6/4/2023 5:19 PM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Rework the ACPI button driver to install notify handlers or fixed > event handlers for the devices it binds to by itself, reduce the > indentation level in its notify handler routine and drop its > notify callback. > > This will allow acpi_device_install_notify_handler() and > acpi_device_remove_notify_handler() to be simplified going forward > and it will allow the driver to use different notify handlers for the > lid and for the power and sleep buttons. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/acpi/button.c | 140 ++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 96 insertions(+), 44 deletions(-) > > Index: linux-pm/drivers/acpi/button.c > =================================================================== > --- linux-pm.orig/drivers/acpi/button.c > +++ linux-pm/drivers/acpi/button.c > @@ -135,7 +135,6 @@ static const struct dmi_system_id dmi_li > > static int acpi_button_add(struct acpi_device *device); > static void acpi_button_remove(struct acpi_device *device); > -static void acpi_button_notify(struct acpi_device *device, u32 event); > > #ifdef CONFIG_PM_SLEEP > static int acpi_button_suspend(struct device *dev); > @@ -153,7 +152,6 @@ static struct acpi_driver acpi_button_dr > .ops = { > .add = acpi_button_add, > .remove = acpi_button_remove, > - .notify = acpi_button_notify, > }, > .drv.pm = &acpi_button_pm, > }; > @@ -409,45 +407,55 @@ static void acpi_lid_initialize_state(st > button->lid_state_initialized = true; > } > > -static void acpi_button_notify(struct acpi_device *device, u32 event) > +static void acpi_button_notify(acpi_handle handle, u32 event, void *data) > { > - struct acpi_button *button = acpi_driver_data(device); > + struct acpi_device *device = data; > + struct acpi_button *button; > struct input_dev *input; > + int keycode; > > - switch (event) { > - case ACPI_FIXED_HARDWARE_EVENT: > - event = ACPI_BUTTON_NOTIFY_STATUS; > - fallthrough; > - case ACPI_BUTTON_NOTIFY_STATUS: > - input = button->input; > - if (button->type == ACPI_BUTTON_TYPE_LID) { > - if (button->lid_state_initialized) > - acpi_lid_update_state(device, true); > - } else { > - int keycode; > - > - acpi_pm_wakeup_event(&device->dev); > - if (button->suspended) > - break; > - > - keycode = test_bit(KEY_SLEEP, input->keybit) ? > - KEY_SLEEP : KEY_POWER; > - input_report_key(input, keycode, 1); > - input_sync(input); > - input_report_key(input, keycode, 0); > - input_sync(input); > - > - acpi_bus_generate_netlink_event( > - device->pnp.device_class, > - dev_name(&device->dev), > - event, ++button->pushed); > - } > - break; > - default: > + if (event != ACPI_BUTTON_NOTIFY_STATUS) { > acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n", > event); > - break; > + return; > + } > + > + button = acpi_driver_data(device); > + > + if (button->type == ACPI_BUTTON_TYPE_LID) { > + if (button->lid_state_initialized) > + acpi_lid_update_state(device, true); > + > + return; > } > + > + acpi_pm_wakeup_event(&device->dev); > + > + if (button->suspended) > + return; > + > + input = button->input; > + keycode = test_bit(KEY_SLEEP, input->keybit) ? KEY_SLEEP : KEY_POWER; > + > + input_report_key(input, keycode, 1); > + input_sync(input); > + input_report_key(input, keycode, 0); > + input_sync(input); > + > + acpi_bus_generate_netlink_event(device->pnp.device_class, > + dev_name(&device->dev), > + event, ++button->pushed); > +} > + > +static void acpi_button_notify_run(void *data) > +{ > + acpi_button_notify(NULL, ACPI_BUTTON_NOTIFY_STATUS, data); > +} > + > +static u32 acpi_button_event(void *data) > +{ > + acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_button_notify_run, data); > + return ACPI_INTERRUPT_HANDLED; > } > > #ifdef CONFIG_PM_SLEEP > @@ -492,8 +500,9 @@ static int acpi_button_add(struct acpi_d > struct acpi_button *button; > struct input_dev *input; > const char *hid = acpi_device_hid(device); > + acpi_status status; > char *name, *class; > - int error; > + int error = 0; > > if (!strcmp(hid, ACPI_BUTTON_HID_LID) && > lid_init_state == ACPI_BUTTON_LID_INIT_DISABLED) > @@ -535,12 +544,15 @@ static int acpi_button_add(struct acpi_d > } else { > pr_info("Unsupported hid [%s]\n", hid); > error = -ENODEV; ... > - goto err_free_input; > } > > - error = acpi_button_add_fs(device); > - if (error) > - goto err_free_input; > + if (!error) > + error = acpi_button_add_fs(device); > + > + if (error) { > + input_free_device(input); > + goto err_free_button; > + } This logic is correct, just a bit weird to read and it's saving just one call to input_free_device(). > > snprintf(button->phys, sizeof(button->phys), "%s/button/input0", hid); > > @@ -568,6 +580,30 @@ static int acpi_button_add(struct acpi_d > error = input_register_device(input); > if (error) > goto err_remove_fs; > + > + switch (device->device_type) { > + case ACPI_BUS_TYPE_POWER_BUTTON: > + status = acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON, > + acpi_button_event, > + device); > + break; > + case ACPI_BUS_TYPE_SLEEP_BUTTON: > + status = acpi_install_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON, > + acpi_button_event, > + device); > + break; > + default: > + status = acpi_install_notify_handler(device->handle, > + ACPI_DEVICE_NOTIFY, > + acpi_button_notify, > + device); > + break; > + } > + if (ACPI_FAILURE(status)) { > + error = -ENODEV; > + goto err_input_unregister; > + } > + > if (button->type == ACPI_BUTTON_TYPE_LID) { > /* > * This assumes there's only one lid device, or if there are > @@ -580,11 +616,11 @@ static int acpi_button_add(struct acpi_d > pr_info("%s [%s]\n", name, acpi_device_bid(device)); > return 0; > > - err_remove_fs: > +err_input_unregister: > + input_unregister_device(input); > +err_remove_fs: > acpi_button_remove_fs(device); > - err_free_input: > - input_free_device(input); > - err_free_button: > +err_free_button: > kfree(button); > return error; > } > @@ -593,6 +629,22 @@ static void acpi_button_remove(struct ac > { > struct acpi_button *button = acpi_driver_data(device); > > + switch (device->device_type) { > + case ACPI_BUS_TYPE_POWER_BUTTON: > + acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON, > + acpi_button_event); > + break; > + case ACPI_BUS_TYPE_SLEEP_BUTTON: > + acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON, > + acpi_button_event); > + break; > + default: > + acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, > + acpi_button_notify); > + break; > + } > + acpi_os_wait_events_complete(); > + > acpi_button_remove_fs(device); > input_unregister_device(button->input); > kfree(button); > > Reviewed-by: Michal Wilczynski <michal.wilczynski@intel.com>
Index: linux-pm/drivers/acpi/button.c =================================================================== --- linux-pm.orig/drivers/acpi/button.c +++ linux-pm/drivers/acpi/button.c @@ -135,7 +135,6 @@ static const struct dmi_system_id dmi_li static int acpi_button_add(struct acpi_device *device); static void acpi_button_remove(struct acpi_device *device); -static void acpi_button_notify(struct acpi_device *device, u32 event); #ifdef CONFIG_PM_SLEEP static int acpi_button_suspend(struct device *dev); @@ -153,7 +152,6 @@ static struct acpi_driver acpi_button_dr .ops = { .add = acpi_button_add, .remove = acpi_button_remove, - .notify = acpi_button_notify, }, .drv.pm = &acpi_button_pm, }; @@ -409,45 +407,55 @@ static void acpi_lid_initialize_state(st button->lid_state_initialized = true; } -static void acpi_button_notify(struct acpi_device *device, u32 event) +static void acpi_button_notify(acpi_handle handle, u32 event, void *data) { - struct acpi_button *button = acpi_driver_data(device); + struct acpi_device *device = data; + struct acpi_button *button; struct input_dev *input; + int keycode; - switch (event) { - case ACPI_FIXED_HARDWARE_EVENT: - event = ACPI_BUTTON_NOTIFY_STATUS; - fallthrough; - case ACPI_BUTTON_NOTIFY_STATUS: - input = button->input; - if (button->type == ACPI_BUTTON_TYPE_LID) { - if (button->lid_state_initialized) - acpi_lid_update_state(device, true); - } else { - int keycode; - - acpi_pm_wakeup_event(&device->dev); - if (button->suspended) - break; - - keycode = test_bit(KEY_SLEEP, input->keybit) ? - KEY_SLEEP : KEY_POWER; - input_report_key(input, keycode, 1); - input_sync(input); - input_report_key(input, keycode, 0); - input_sync(input); - - acpi_bus_generate_netlink_event( - device->pnp.device_class, - dev_name(&device->dev), - event, ++button->pushed); - } - break; - default: + if (event != ACPI_BUTTON_NOTIFY_STATUS) { acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n", event); - break; + return; + } + + button = acpi_driver_data(device); + + if (button->type == ACPI_BUTTON_TYPE_LID) { + if (button->lid_state_initialized) + acpi_lid_update_state(device, true); + + return; } + + acpi_pm_wakeup_event(&device->dev); + + if (button->suspended) + return; + + input = button->input; + keycode = test_bit(KEY_SLEEP, input->keybit) ? KEY_SLEEP : KEY_POWER; + + input_report_key(input, keycode, 1); + input_sync(input); + input_report_key(input, keycode, 0); + input_sync(input); + + acpi_bus_generate_netlink_event(device->pnp.device_class, + dev_name(&device->dev), + event, ++button->pushed); +} + +static void acpi_button_notify_run(void *data) +{ + acpi_button_notify(NULL, ACPI_BUTTON_NOTIFY_STATUS, data); +} + +static u32 acpi_button_event(void *data) +{ + acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_button_notify_run, data); + return ACPI_INTERRUPT_HANDLED; } #ifdef CONFIG_PM_SLEEP @@ -492,8 +500,9 @@ static int acpi_button_add(struct acpi_d struct acpi_button *button; struct input_dev *input; const char *hid = acpi_device_hid(device); + acpi_status status; char *name, *class; - int error; + int error = 0; if (!strcmp(hid, ACPI_BUTTON_HID_LID) && lid_init_state == ACPI_BUTTON_LID_INIT_DISABLED) @@ -535,12 +544,15 @@ static int acpi_button_add(struct acpi_d } else { pr_info("Unsupported hid [%s]\n", hid); error = -ENODEV; - goto err_free_input; } - error = acpi_button_add_fs(device); - if (error) - goto err_free_input; + if (!error) + error = acpi_button_add_fs(device); + + if (error) { + input_free_device(input); + goto err_free_button; + } snprintf(button->phys, sizeof(button->phys), "%s/button/input0", hid); @@ -568,6 +580,30 @@ static int acpi_button_add(struct acpi_d error = input_register_device(input); if (error) goto err_remove_fs; + + switch (device->device_type) { + case ACPI_BUS_TYPE_POWER_BUTTON: + status = acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON, + acpi_button_event, + device); + break; + case ACPI_BUS_TYPE_SLEEP_BUTTON: + status = acpi_install_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON, + acpi_button_event, + device); + break; + default: + status = acpi_install_notify_handler(device->handle, + ACPI_DEVICE_NOTIFY, + acpi_button_notify, + device); + break; + } + if (ACPI_FAILURE(status)) { + error = -ENODEV; + goto err_input_unregister; + } + if (button->type == ACPI_BUTTON_TYPE_LID) { /* * This assumes there's only one lid device, or if there are @@ -580,11 +616,11 @@ static int acpi_button_add(struct acpi_d pr_info("%s [%s]\n", name, acpi_device_bid(device)); return 0; - err_remove_fs: +err_input_unregister: + input_unregister_device(input); +err_remove_fs: acpi_button_remove_fs(device); - err_free_input: - input_free_device(input); - err_free_button: +err_free_button: kfree(button); return error; } @@ -593,6 +629,22 @@ static void acpi_button_remove(struct ac { struct acpi_button *button = acpi_driver_data(device); + switch (device->device_type) { + case ACPI_BUS_TYPE_POWER_BUTTON: + acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON, + acpi_button_event); + break; + case ACPI_BUS_TYPE_SLEEP_BUTTON: + acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON, + acpi_button_event); + break; + default: + acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, + acpi_button_notify); + break; + } + acpi_os_wait_events_complete(); + acpi_button_remove_fs(device); input_unregister_device(button->input); kfree(button);