| Message ID | 20231025111806.2416524-5-michal.wilczynski@intel.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | Replace acpi_driver with platform_driver | expand |
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(®ister_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
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(®ister_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; }
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(-)