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.
>
>> }
>>
>> /* 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);
>
