| Message ID | 20230601131739.300760-22-michal.wilczynski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Remove .notify callback in acpi_device_ops | expand |
On Thu, 1 Jun 2023, Michal Wilczynski wrote: > Currently logic for installing notifications from ACPI devices is > implemented using notify callback in struct acpi_driver. Preparations > are being made to replace acpi_driver with more generic struct > platform_driver, which doesn't contain notify callback. Furthermore > as of now handlers are being called indirectly through > acpi_notify_device(), which decreases performance. > > Call acpi_device_install_event_handler() at the end of .add() callback. > Call acpi_device_remove_event_handler() at the beginning of .remove() > callback. Change arguments passed to the notify callback to match with > what's required by acpi_device_install_event_handler(). > > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > --- > drivers/platform/x86/fujitsu-laptop.c | 86 +++++++++++++++++---------- > 1 file changed, 54 insertions(+), 32 deletions(-) > > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c > index 085e044e888e..001333edba9f 100644 > --- a/drivers/platform/x86/fujitsu-laptop.c > +++ b/drivers/platform/x86/fujitsu-laptop.c > @@ -136,6 +136,8 @@ struct fujitsu_laptop { > > static struct acpi_device *fext; > > +static void acpi_fujitsu_laptop_notify(acpi_handle handle, u32 event, void *data); > + > /* Fujitsu ACPI interface function */ > > static int call_fext_func(struct acpi_device *device, > @@ -382,6 +384,37 @@ static int fujitsu_backlight_register(struct acpi_device *device) > return 0; > } > > +static void acpi_fujitsu_bl_notify(acpi_handle handle, u32 event, void *data) > +{ > + struct acpi_device *device = data; > + struct fujitsu_bl *priv; > + int oldb, newb; > + > + priv = acpi_driver_data(device); > + > + if (event != ACPI_FUJITSU_NOTIFY_CODE) { > + acpi_handle_info(device->handle, "unsupported event [0x%x]\n", > + event); > + sparse_keymap_report_event(priv->input, -1, 1, true); > + return; > + } > + > + oldb = priv->brightness_level; > + get_lcd_level(device); > + newb = priv->brightness_level; > + > + acpi_handle_debug(device->handle, > + "brightness button event [%i -> %i]\n", oldb, newb); > + > + if (oldb == newb) > + return; > + > + if (!disable_brightness_adjust) > + set_lcd_level(device, newb); > + > + sparse_keymap_report_event(priv->input, oldb < newb, 1, true); > +} > + > static int acpi_fujitsu_bl_add(struct acpi_device *device) > { > struct fujitsu_bl *priv; > @@ -410,37 +443,17 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device) > if (ret) > return ret; > > - return fujitsu_backlight_register(device); > -} > + ret = fujitsu_backlight_register(device); > + if (ret) > + return ret; > > -/* Brightness notify */ > + return acpi_device_install_event_handler(device, ACPI_DEVICE_NOTIFY, > + acpi_fujitsu_bl_notify); > +} > > -static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event) > +static void acpi_fujitsu_bl_remove(struct acpi_device *device) > { > - struct fujitsu_bl *priv = acpi_driver_data(device); > - int oldb, newb; > - > - if (event != ACPI_FUJITSU_NOTIFY_CODE) { > - acpi_handle_info(device->handle, "unsupported event [0x%x]\n", > - event); > - sparse_keymap_report_event(priv->input, -1, 1, true); > - return; > - } > - > - oldb = priv->brightness_level; > - get_lcd_level(device); > - newb = priv->brightness_level; > - > - acpi_handle_debug(device->handle, > - "brightness button event [%i -> %i]\n", oldb, newb); > - > - if (oldb == newb) > - return; > - > - if (!disable_brightness_adjust) > - set_lcd_level(device, newb); > - > - sparse_keymap_report_event(priv->input, oldb < newb, 1, true); > + acpi_device_remove_event_handler(device, ACPI_DEVICE_NOTIFY, acpi_fujitsu_bl_notify); Please move the function in a separate (no function changes intended) patch to keep this patch on point. > } > > /* ACPI device for hotkey handling */ > @@ -839,6 +852,11 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device) > if (ret) > goto err_free_fifo; > > + ret = acpi_device_install_event_handler(device, ACPI_DEVICE_NOTIFY, > + acpi_fujitsu_laptop_notify); > + if (ret) > + goto err_free_fifo; Here too, fujitsu_laptop_platform_remove() is necessary, goto + put it into the rollback path. Please go through your patches with these rollback corrections in mind, I'll skip looking through the rest of platform/x86 patches until you've done that. > + > return 0; > > err_free_fifo: > @@ -851,6 +869,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device) > { > struct fujitsu_laptop *priv = acpi_driver_data(device); > > + acpi_device_remove_event_handler(device, ACPI_DEVICE_NOTIFY, acpi_fujitsu_laptop_notify); > + > fujitsu_laptop_platform_remove(device); > > kfifo_free(&priv->fifo);
On 6/2/2023 3:30 PM, Ilpo Järvinen wrote: > On Thu, 1 Jun 2023, Michal Wilczynski wrote: > >> Currently logic for installing notifications from ACPI devices is >> implemented using notify callback in struct acpi_driver. Preparations >> are being made to replace acpi_driver with more generic struct >> platform_driver, which doesn't contain notify callback. Furthermore >> as of now handlers are being called indirectly through >> acpi_notify_device(), which decreases performance. >> >> Call acpi_device_install_event_handler() at the end of .add() callback. >> Call acpi_device_remove_event_handler() at the beginning of .remove() >> callback. Change arguments passed to the notify callback to match with >> what's required by acpi_device_install_event_handler(). >> >> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> >> --- >> drivers/platform/x86/fujitsu-laptop.c | 86 +++++++++++++++++---------- >> 1 file changed, 54 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c >> index 085e044e888e..001333edba9f 100644 >> --- a/drivers/platform/x86/fujitsu-laptop.c >> +++ b/drivers/platform/x86/fujitsu-laptop.c >> @@ -136,6 +136,8 @@ struct fujitsu_laptop { >> >> static struct acpi_device *fext; >> >> +static void acpi_fujitsu_laptop_notify(acpi_handle handle, u32 event, void *data); >> + >> /* Fujitsu ACPI interface function */ >> >> static int call_fext_func(struct acpi_device *device, >> @@ -382,6 +384,37 @@ static int fujitsu_backlight_register(struct acpi_device *device) >> return 0; >> } >> >> +static void acpi_fujitsu_bl_notify(acpi_handle handle, u32 event, void *data) >> +{ >> + struct acpi_device *device = data; >> + struct fujitsu_bl *priv; >> + int oldb, newb; >> + >> + priv = acpi_driver_data(device); >> + >> + if (event != ACPI_FUJITSU_NOTIFY_CODE) { >> + acpi_handle_info(device->handle, "unsupported event [0x%x]\n", >> + event); >> + sparse_keymap_report_event(priv->input, -1, 1, true); >> + return; >> + } >> + >> + oldb = priv->brightness_level; >> + get_lcd_level(device); >> + newb = priv->brightness_level; >> + >> + acpi_handle_debug(device->handle, >> + "brightness button event [%i -> %i]\n", oldb, newb); >> + >> + if (oldb == newb) >> + return; >> + >> + if (!disable_brightness_adjust) >> + set_lcd_level(device, newb); >> + >> + sparse_keymap_report_event(priv->input, oldb < newb, 1, true); >> +} >> + >> static int acpi_fujitsu_bl_add(struct acpi_device *device) >> { >> struct fujitsu_bl *priv; >> @@ -410,37 +443,17 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device) >> if (ret) >> return ret; >> >> - return fujitsu_backlight_register(device); >> -} >> + ret = fujitsu_backlight_register(device); >> + if (ret) >> + return ret; >> >> -/* Brightness notify */ >> + return acpi_device_install_event_handler(device, ACPI_DEVICE_NOTIFY, >> + acpi_fujitsu_bl_notify); >> +} >> >> -static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event) >> +static void acpi_fujitsu_bl_remove(struct acpi_device *device) >> { >> - struct fujitsu_bl *priv = acpi_driver_data(device); >> - int oldb, newb; >> - >> - if (event != ACPI_FUJITSU_NOTIFY_CODE) { >> - acpi_handle_info(device->handle, "unsupported event [0x%x]\n", >> - event); >> - sparse_keymap_report_event(priv->input, -1, 1, true); >> - return; >> - } >> - >> - oldb = priv->brightness_level; >> - get_lcd_level(device); >> - newb = priv->brightness_level; >> - >> - acpi_handle_debug(device->handle, >> - "brightness button event [%i -> %i]\n", oldb, newb); >> - >> - if (oldb == newb) >> - return; >> - >> - if (!disable_brightness_adjust) >> - set_lcd_level(device, newb); >> - >> - sparse_keymap_report_event(priv->input, oldb < newb, 1, true); >> + acpi_device_remove_event_handler(device, ACPI_DEVICE_NOTIFY, acpi_fujitsu_bl_notify); > Please move the function in a separate (no function changes intended) > patch to keep this patch on point. Sure I can do that, however this moving around make sense in context of this patch as it's necessary to compile with the new event installation method. > >> } >> >> /* ACPI device for hotkey handling */ >> @@ -839,6 +852,11 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device) >> if (ret) >> goto err_free_fifo; >> >> + ret = acpi_device_install_event_handler(device, ACPI_DEVICE_NOTIFY, >> + acpi_fujitsu_laptop_notify); >> + if (ret) >> + goto err_free_fifo; > Here too, fujitsu_laptop_platform_remove() is necessary, goto + put it > into the rollback path. > > Please go through your patches with these rollback corrections in mind, > I'll skip looking through the rest of platform/x86 patches until you've > done that. Will do that, Thank you for your time ! > >> + >> return 0; >> >> err_free_fifo: >> @@ -851,6 +869,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device) >> { >> struct fujitsu_laptop *priv = acpi_driver_data(device); >> >> + acpi_device_remove_event_handler(device, ACPI_DEVICE_NOTIFY, acpi_fujitsu_laptop_notify); >> + >> fujitsu_laptop_platform_remove(device); >> >> kfifo_free(&priv->fifo); >
On Fri, 2 Jun 2023, Wilczynski, Michal wrote: > On 6/2/2023 3:30 PM, Ilpo Järvinen wrote: > > On Thu, 1 Jun 2023, Michal Wilczynski wrote: > > > >> Currently logic for installing notifications from ACPI devices is > >> implemented using notify callback in struct acpi_driver. Preparations > >> are being made to replace acpi_driver with more generic struct > >> platform_driver, which doesn't contain notify callback. Furthermore > >> as of now handlers are being called indirectly through > >> acpi_notify_device(), which decreases performance. > >> > >> Call acpi_device_install_event_handler() at the end of .add() callback. > >> Call acpi_device_remove_event_handler() at the beginning of .remove() > >> callback. Change arguments passed to the notify callback to match with > >> what's required by acpi_device_install_event_handler(). > >> > >> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > >> --- > >> drivers/platform/x86/fujitsu-laptop.c | 86 +++++++++++++++++---------- > >> 1 file changed, 54 insertions(+), 32 deletions(-) > >> > >> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c > >> index 085e044e888e..001333edba9f 100644 > >> --- a/drivers/platform/x86/fujitsu-laptop.c > >> +++ b/drivers/platform/x86/fujitsu-laptop.c > >> @@ -136,6 +136,8 @@ struct fujitsu_laptop { > >> > >> static struct acpi_device *fext; > >> > >> +static void acpi_fujitsu_laptop_notify(acpi_handle handle, u32 event, void *data); > >> + > >> /* Fujitsu ACPI interface function */ > >> > >> static int call_fext_func(struct acpi_device *device, > >> @@ -382,6 +384,37 @@ static int fujitsu_backlight_register(struct acpi_device *device) > >> return 0; > >> } > >> > >> +static void acpi_fujitsu_bl_notify(acpi_handle handle, u32 event, void *data) > >> +{ > >> + struct acpi_device *device = data; > >> + struct fujitsu_bl *priv; > >> + int oldb, newb; > >> + > >> + priv = acpi_driver_data(device); > >> + > >> + if (event != ACPI_FUJITSU_NOTIFY_CODE) { > >> + acpi_handle_info(device->handle, "unsupported event [0x%x]\n", > >> + event); > >> + sparse_keymap_report_event(priv->input, -1, 1, true); > >> + return; > >> + } > >> + > >> + oldb = priv->brightness_level; > >> + get_lcd_level(device); > >> + newb = priv->brightness_level; > >> + > >> + acpi_handle_debug(device->handle, > >> + "brightness button event [%i -> %i]\n", oldb, newb); > >> + > >> + if (oldb == newb) > >> + return; > >> + > >> + if (!disable_brightness_adjust) > >> + set_lcd_level(device, newb); > >> + > >> + sparse_keymap_report_event(priv->input, oldb < newb, 1, true); > >> +} > >> + > >> static int acpi_fujitsu_bl_add(struct acpi_device *device) > >> { > >> struct fujitsu_bl *priv; > >> @@ -410,37 +443,17 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device) > >> if (ret) > >> return ret; > >> > >> - return fujitsu_backlight_register(device); > >> -} > >> + ret = fujitsu_backlight_register(device); > >> + if (ret) > >> + return ret; > >> > >> -/* Brightness notify */ > >> + return acpi_device_install_event_handler(device, ACPI_DEVICE_NOTIFY, > >> + acpi_fujitsu_bl_notify); > >> +} > >> > >> -static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event) > >> +static void acpi_fujitsu_bl_remove(struct acpi_device *device) > >> { > >> - struct fujitsu_bl *priv = acpi_driver_data(device); > >> - int oldb, newb; > >> - > >> - if (event != ACPI_FUJITSU_NOTIFY_CODE) { > >> - acpi_handle_info(device->handle, "unsupported event [0x%x]\n", > >> - event); > >> - sparse_keymap_report_event(priv->input, -1, 1, true); > >> - return; > >> - } > >> - > >> - oldb = priv->brightness_level; > >> - get_lcd_level(device); > >> - newb = priv->brightness_level; > >> - > >> - acpi_handle_debug(device->handle, > >> - "brightness button event [%i -> %i]\n", oldb, newb); > >> - > >> - if (oldb == newb) > >> - return; > >> - > >> - if (!disable_brightness_adjust) > >> - set_lcd_level(device, newb); > >> - > >> - sparse_keymap_report_event(priv->input, oldb < newb, 1, true); > >> + acpi_device_remove_event_handler(device, ACPI_DEVICE_NOTIFY, acpi_fujitsu_bl_notify); > > Please move the function in a separate (no function changes intended) > > patch to keep this patch on point. > > Sure I can do that, however this moving around make sense in context of this patch as > it's necessary to compile with the new event installation method. You move the function in one patch without changing anything else. And then in another change that is this notify change you make the necessary adjustments to the moved function. It makes both patches easier to review because one is a trivial copy and notify related changes are no longer mixed up with the copy.
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c index 085e044e888e..001333edba9f 100644 --- a/drivers/platform/x86/fujitsu-laptop.c +++ b/drivers/platform/x86/fujitsu-laptop.c @@ -136,6 +136,8 @@ struct fujitsu_laptop { static struct acpi_device *fext; +static void acpi_fujitsu_laptop_notify(acpi_handle handle, u32 event, void *data); + /* Fujitsu ACPI interface function */ static int call_fext_func(struct acpi_device *device, @@ -382,6 +384,37 @@ static int fujitsu_backlight_register(struct acpi_device *device) return 0; } +static void acpi_fujitsu_bl_notify(acpi_handle handle, u32 event, void *data) +{ + struct acpi_device *device = data; + struct fujitsu_bl *priv; + int oldb, newb; + + priv = acpi_driver_data(device); + + if (event != ACPI_FUJITSU_NOTIFY_CODE) { + acpi_handle_info(device->handle, "unsupported event [0x%x]\n", + event); + sparse_keymap_report_event(priv->input, -1, 1, true); + return; + } + + oldb = priv->brightness_level; + get_lcd_level(device); + newb = priv->brightness_level; + + acpi_handle_debug(device->handle, + "brightness button event [%i -> %i]\n", oldb, newb); + + if (oldb == newb) + return; + + if (!disable_brightness_adjust) + set_lcd_level(device, newb); + + sparse_keymap_report_event(priv->input, oldb < newb, 1, true); +} + static int acpi_fujitsu_bl_add(struct acpi_device *device) { struct fujitsu_bl *priv; @@ -410,37 +443,17 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device) if (ret) return ret; - return fujitsu_backlight_register(device); -} + ret = fujitsu_backlight_register(device); + if (ret) + return ret; -/* Brightness notify */ + return acpi_device_install_event_handler(device, ACPI_DEVICE_NOTIFY, + acpi_fujitsu_bl_notify); +} -static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event) +static void acpi_fujitsu_bl_remove(struct acpi_device *device) { - struct fujitsu_bl *priv = acpi_driver_data(device); - int oldb, newb; - - if (event != ACPI_FUJITSU_NOTIFY_CODE) { - acpi_handle_info(device->handle, "unsupported event [0x%x]\n", - event); - sparse_keymap_report_event(priv->input, -1, 1, true); - return; - } - - oldb = priv->brightness_level; - get_lcd_level(device); - newb = priv->brightness_level; - - acpi_handle_debug(device->handle, - "brightness button event [%i -> %i]\n", oldb, newb); - - if (oldb == newb) - return; - - if (!disable_brightness_adjust) - set_lcd_level(device, newb); - - sparse_keymap_report_event(priv->input, oldb < newb, 1, true); + acpi_device_remove_event_handler(device, ACPI_DEVICE_NOTIFY, acpi_fujitsu_bl_notify); } /* ACPI device for hotkey handling */ @@ -839,6 +852,11 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device) if (ret) goto err_free_fifo; + ret = acpi_device_install_event_handler(device, ACPI_DEVICE_NOTIFY, + acpi_fujitsu_laptop_notify); + if (ret) + goto err_free_fifo; + return 0; err_free_fifo: @@ -851,6 +869,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device) { struct fujitsu_laptop *priv = acpi_driver_data(device); + acpi_device_remove_event_handler(device, ACPI_DEVICE_NOTIFY, acpi_fujitsu_laptop_notify); + fujitsu_laptop_platform_remove(device); kfifo_free(&priv->fifo); @@ -889,13 +909,16 @@ static void acpi_fujitsu_laptop_release(struct acpi_device *device) } } -static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event) +static void acpi_fujitsu_laptop_notify(acpi_handle handle, u32 event, void *data) { - struct fujitsu_laptop *priv = acpi_driver_data(device); + struct acpi_device *device = data; + struct fujitsu_laptop *priv; unsigned long flags; int scancode, i = 0; unsigned int irb; + priv = acpi_driver_data(device); + if (event != ACPI_FUJITSU_NOTIFY_CODE) { acpi_handle_info(device->handle, "Unsupported event [0x%x]\n", event); @@ -947,7 +970,7 @@ static struct acpi_driver acpi_fujitsu_bl_driver = { .ids = fujitsu_bl_device_ids, .ops = { .add = acpi_fujitsu_bl_add, - .notify = acpi_fujitsu_bl_notify, + .remove = acpi_fujitsu_bl_remove, }, }; @@ -963,7 +986,6 @@ static struct acpi_driver acpi_fujitsu_laptop_driver = { .ops = { .add = acpi_fujitsu_laptop_add, .remove = acpi_fujitsu_laptop_remove, - .notify = acpi_fujitsu_laptop_notify, }, };
Currently logic for installing notifications from ACPI devices is implemented using notify callback in struct acpi_driver. Preparations are being made to replace acpi_driver with more generic struct platform_driver, which doesn't contain notify callback. Furthermore as of now handlers are being called indirectly through acpi_notify_device(), which decreases performance. Call acpi_device_install_event_handler() at the end of .add() callback. Call acpi_device_remove_event_handler() at the beginning of .remove() callback. Change arguments passed to the notify callback to match with what's required by acpi_device_install_event_handler(). Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> --- drivers/platform/x86/fujitsu-laptop.c | 86 +++++++++++++++++---------- 1 file changed, 54 insertions(+), 32 deletions(-)