[v1,4/6] ACPI: acpi_video: Replace acpi_driver with platform_driver

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>
---
 drivers/acpi/acpi_video.c | 41 ++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

On Wed, Oct 25, 2023 at 02:18:04PM +0300, Michal Wilczynski 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.

...

>  #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>

Despite this being unsorted I would squeeze to the most sorted part of it,
i.e.  with the given context the new inclusion is good to have after dmi.h
(but in full context it might be even better spot).

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
>
>

Hi,

On 11/29/23 15:19, Rafael J. Wysocki wrote:
> 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?

I have just given this a quick test spin and on most hw
it actually causes the apci_video driver to not bind
anymore *at all* which will cause a bunch of brokenness
all over the place.

The problem is that the physical-node for which the 
/sys/bus/acpi/devices/LNXVIDEO:00 fwnode / acpi-companion node 
is the companion normally is the GPU, which is a PCI
device so no /sys/bus/platform/devices/LNXVIDEO:00
device is instantiated for the new "video" platform driver
to bind to.

While I appreciate the efforts being done to clean up
the ACPI subsystem I must also say that this makes me
question how well all these convert to platform driver
patches are tested ?

Almost all modern x86 hw has a /sys/bus/acpi/devices/LNXVIDEO:00
device, so this can be tested almost everywhere and this should
have been caught during testing by a test as simple as:

1. "ls /sys/bus/platform/devices/LNXVIDEO:00" and notice this
does not exist and/or:
2. "ls /sys/bus/platform/drivers/video/" and notice it has not
bound to anything where before this change the acpi_video
module would have bound to /sys/bus/acpi/devices/LNXVIDEO:00

Also the "Video Bus" input/evdev device is now gone
from "sudo evtest" which is a third quick way to see this
now all no longer works.

One possible way to solve this is to treat LNXVIDEO devices
specially and always create a platform_device for them.

This will also require some changes to the modalias
and driver-matching code because normally acpi:xxxx
modaliases are only used / matched when the platform_device
is the first physical node, where as I think
the platform_device will end up being the second physical
node now.

One last remark, assuming we find a way to solve this,
then IMHO the .name field in the driver:

>> +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,
>> +       },
>>  };

MUST not be just "video" platform devices <-> drivers  also get
matched by dev_name() so if anyone now creates a platform_device
named "video" then the platform_bus will now bind this driver
to it. "acpi_video", matching the .c filename (but not the module
name for historical reasons) would be better IMHO.

Regards,

Hans