Re: [PATCH v4 17/35] platform/x86/asus-wireless: Move handler installing logic to driver

From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Michal Wilczynski <michal.wilczynski@intel.com>
Cc: "Corentin Chary" <corentin.chary@gmail.com>,
	"João Paulo Rechi Vita" <jprvita@gmail.com>,
	"Hans de Goede" <hdegoede@redhat.com>,
	"Mark Gross" <markgross@kernel.org>,
	platform-driver-x86@vger.kernel.org,
	acpi4asus-user@lists.sourceforge.net, linux-acpi@vger.kernel.org,
	rafael@kernel.org
Subject: Re: [PATCH v4 17/35] platform/x86/asus-wireless: Move handler installing logic to driver
Date: Fri, 2 Jun 2023 16:09:51 +0300 (EEST)	[thread overview]
Message-ID: <de33f84-8774-9d8-5d6c-ea6a31343fab@linux.intel.com> (raw)
In-Reply-To: <20230601131739.300760-18-michal.wilczynski@intel.com>

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/asus-wireless.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c
> index abf01e00b799..6544e3419ae4 100644
> --- a/drivers/platform/x86/asus-wireless.c
> +++ b/drivers/platform/x86/asus-wireless.c
> @@ -108,19 +108,22 @@ static void led_state_set(struct led_classdev *led, enum led_brightness value)
>  	queue_work(data->wq, &data->led_work);
>  }
>  
> -static void asus_wireless_notify(struct acpi_device *adev, u32 event)
> +static void asus_wireless_notify(acpi_handle handle, u32 event, void *data)
>  {
> -	struct asus_wireless_data *data = acpi_driver_data(adev);
> +	struct asus_wireless_data *w_data;
> +	struct acpi_device *adev = data;
> +
> +	w_data = acpi_driver_data(adev);
>  
>  	dev_dbg(&adev->dev, "event=%#x\n", event);
>  	if (event != 0x88) {
>  		dev_notice(&adev->dev, "Unknown ASHS event: %#x\n", event);
>  		return;
>  	}
> -	input_report_key(data->idev, KEY_RFKILL, 1);
> -	input_sync(data->idev);
> -	input_report_key(data->idev, KEY_RFKILL, 0);
> -	input_sync(data->idev);
> +	input_report_key(w_data->idev, KEY_RFKILL, 1);
> +	input_sync(w_data->idev);
> +	input_report_key(w_data->idev, KEY_RFKILL, 0);
> +	input_sync(w_data->idev);
>  }
>  
>  static int asus_wireless_add(struct acpi_device *adev)
> @@ -169,16 +172,20 @@ static int asus_wireless_add(struct acpi_device *adev)
>  	data->led.max_brightness = 1;
>  	data->led.default_trigger = "rfkill-none";
>  	err = devm_led_classdev_register(&adev->dev, &data->led);
> -	if (err)
> +	if (err) {
>  		destroy_workqueue(data->wq);
> +		return err;
> +	}
>  
> -	return err;
> +	return acpi_device_install_event_handler(adev, ACPI_DEVICE_NOTIFY, asus_wireless_notify);

So if acpi_device_install_event_handler() returns error, should you 
rollback something here like the error branch above? If that's the case, 
use goto to rollback as you'll have two places calling destroy_workqueue() 
already.

-- 
 i.

>  }
>  
>  static void asus_wireless_remove(struct acpi_device *adev)
>  {
>  	struct asus_wireless_data *data = acpi_driver_data(adev);
>  
> +	acpi_device_remove_event_handler(adev, ACPI_DEVICE_NOTIFY, asus_wireless_notify);
> +
>  	if (data->wq) {
>  		devm_led_classdev_unregister(&adev->dev, &data->led);
>  		destroy_workqueue(data->wq);
> @@ -192,7 +199,6 @@ static struct acpi_driver asus_wireless_driver = {
>  	.ops = {
>  		.add = asus_wireless_add,
>  		.remove = asus_wireless_remove,
> -		.notify = asus_wireless_notify,
>  	},
>  };
>  module_acpi_driver(asus_wireless_driver);
> 