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@xxxxxxxxx>
> >> ---
> >> 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.
--
i.
