Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <20231025111806.2416524-1-michal.wilczynski@intel.com> <20231025111806.2416524-5-michal.wilczynski@intel.com>
In-Reply-To: <20231025111806.2416524-5-michal.wilczynski@intel.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Wed, 29 Nov 2023 15:19:27 +0100
Message-ID: <CAJZ5v0h2G6i0qXvSyFJeC4zsML3yxGZvUt-nUPs2Z3iHEM845A@mail.gmail.com>
Subject: Re: [PATCH v1 4/6] ACPI: acpi_video: Replace acpi_driver with platform_driver
To:     Michal Wilczynski <michal.wilczynski@intel.com>,
        Hans de Goede <hdegoede@redhat.com>
Cc:     linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
        rafael.j.wysocki@intel.com, andriy.shevchenko@linux.intel.com,
        lenb@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

Hi Hans,

On Wed, Oct 25, 2023 at 2:35 PM Michal Wilczynski
<michal.wilczynski@intel.com> wrote:
>
> The acpi_video driver uses struct acpi_driver to register itself while it
> would be more logically consistent to use struct platform_driver for this
> purpose, because the corresponding platform device is present and the
> role of struct acpi_device is to amend the other bus types. ACPI devices
> are not meant to be used as proper representation of hardware entities,
> but to collect information on those hardware entities provided by the
> platform firmware.
>
> Use struct platform_driver for registering the acpi_video driver.
>
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>

Do you have any particular concerns regarding this change?  For
example, are there any setups that can break because of it?

> ---
>  drivers/acpi/acpi_video.c | 41 ++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 0c93b0ef0feb..5b9fb3374a2e 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -25,6 +25,7 @@
>  #include <linux/dmi.h>
>  #include <linux/suspend.h>
>  #include <linux/acpi.h>
> +#include <linux/platform_device.h>
>  #include <acpi/video.h>
>  #include <linux/uaccess.h>
>
> @@ -75,8 +76,8 @@ static int register_count;
>  static DEFINE_MUTEX(register_count_mutex);
>  static DEFINE_MUTEX(video_list_lock);
>  static LIST_HEAD(video_bus_head);
> -static int acpi_video_bus_add(struct acpi_device *device);
> -static void acpi_video_bus_remove(struct acpi_device *device);
> +static int acpi_video_bus_probe(struct platform_device *pdev);
> +static void acpi_video_bus_remove(struct platform_device *pdev);
>  static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data);
>
>  /*
> @@ -97,14 +98,13 @@ static const struct acpi_device_id video_device_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, video_device_ids);
>
> -static struct acpi_driver acpi_video_bus = {
> -       .name = "video",
> -       .class = ACPI_VIDEO_CLASS,
> -       .ids = video_device_ids,
> -       .ops = {
> -               .add = acpi_video_bus_add,
> -               .remove = acpi_video_bus_remove,
> -               },
> +static struct platform_driver acpi_video_bus = {
> +       .probe = acpi_video_bus_probe,
> +       .remove_new = acpi_video_bus_remove,
> +       .driver = {
> +               .name = "video",
> +               .acpi_match_table = video_device_ids,
> +       },
>  };
>
>  struct acpi_video_bus_flags {
> @@ -1525,8 +1525,8 @@ static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
>
>  static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data)
>  {
> -       struct acpi_device *device = data;
> -       struct acpi_video_bus *video = acpi_driver_data(device);
> +       struct acpi_video_bus *video = data;
> +       struct acpi_device *device = video->device;
>         struct input_dev *input;
>         int keycode = 0;
>
> @@ -1969,8 +1969,9 @@ static int acpi_video_bus_put_devices(struct acpi_video_bus *video)
>
>  static int instance;
>
> -static int acpi_video_bus_add(struct acpi_device *device)
> +static int acpi_video_bus_probe(struct platform_device *pdev)
>  {
> +       struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>         struct acpi_video_bus *video;
>         bool auto_detect;
>         int error;
> @@ -2010,7 +2011,7 @@ static int acpi_video_bus_add(struct acpi_device *device)
>         video->device = device;
>         strcpy(acpi_device_name(device), ACPI_VIDEO_BUS_NAME);
>         strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
> -       device->driver_data = video;
> +       platform_set_drvdata(pdev, video);
>
>         acpi_video_bus_find_cap(video);
>         error = acpi_video_bus_check(video);
> @@ -2059,7 +2060,7 @@ static int acpi_video_bus_add(struct acpi_device *device)
>                 goto err_del;
>
>         error = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY,
> -                                               acpi_video_bus_notify, device);
> +                                               acpi_video_bus_notify, video);
>         if (error)
>                 goto err_remove;
>
> @@ -2081,11 +2082,11 @@ static int acpi_video_bus_add(struct acpi_device *device)
>         return error;
>  }
>
> -static void acpi_video_bus_remove(struct acpi_device *device)
> +static void acpi_video_bus_remove(struct platform_device *pdev)
>  {
> -       struct acpi_video_bus *video = acpi_driver_data(device);
> +       struct acpi_video_bus *video = platform_get_drvdata(pdev);
>
> -       acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY,
> +       acpi_dev_remove_notify_handler(video->device, ACPI_DEVICE_NOTIFY,
>                                        acpi_video_bus_notify);
>
>         mutex_lock(&video_list_lock);
> @@ -2200,7 +2201,7 @@ int acpi_video_register(void)
>
>         dmi_check_system(video_dmi_table);
>
> -       ret = acpi_bus_register_driver(&acpi_video_bus);
> +       ret = platform_driver_register(&acpi_video_bus);
>         if (ret)
>                 goto leave;
>
> @@ -2220,7 +2221,7 @@ void acpi_video_unregister(void)
>  {
>         mutex_lock(&register_count_mutex);
>         if (register_count) {
> -               acpi_bus_unregister_driver(&acpi_video_bus);
> +               platform_driver_unregister(&acpi_video_bus);
>                 register_count = 0;
>                 may_report_brightness_keys = false;
>         }
> --
> 2.41.0
>
>