[PATCH v1 0/6] Replace acpi_driver with platform_driver

[PATCH v1 0/6] Replace acpi_driver with platform_driver

[Michal Wilczynski]

This patchset is a continuation of efforts from [1] aiming to replace
acpi_driver with platform_driver. To ease up review effort I'm sending
miniseries per driver, with a replacement patch + various improvements
that were noticed by me, or during internal review.

This mini-series takes care of acpi_video driver.

[1] - https://lore.kernel.org/linux-acpi/20231011083334.3987477-1-michal.wilczynski@intel.com/T/#t

Michal Wilczynski (6):
  ACPI: acpi_video: Remove unnecessary checks
  ACPI: acpi_video: Use yes_or_no helper instead of ternary operator
  ACPI: acpi_video: Remove unnecessary driver_data clear
  ACPI: acpi_video: Replace acpi_driver with platform_driver
  ACPI: acpi_video: Rename ACPI device instances from device to adev
  ACPI: acpi_video: Fix holes in acpi_video_bus

 drivers/acpi/acpi_video.c | 101 +++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 55 deletions(-)

-- 
2.41.0

[PATCH v1 1/6] ACPI: acpi_video: Remove unnecessary checks

[Michal Wilczynski]

Remove unnecessary checks for NULL for variables that can't be NULL at
the point they're checked for it.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
 drivers/acpi/acpi_video.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 0b7a01f38b65..c14b44f99e35 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1027,9 +1027,6 @@ static int acpi_video_bus_check(struct acpi_video_bus *video)
 	acpi_status status = -ENOENT;
 	struct pci_dev *dev;
 
-	if (!video)
-		return -EINVAL;
-
 	dev = acpi_get_pci_dev(video->device->handle);
 	if (!dev)
 		return -ENODEV;
@@ -2087,13 +2084,7 @@ static int acpi_video_bus_add(struct acpi_device *device)
 
 static void acpi_video_bus_remove(struct acpi_device *device)
 {
-	struct acpi_video_bus *video = NULL;
-
-
-	if (!device || !acpi_driver_data(device))
-		return;
-
-	video = acpi_driver_data(device);
+	struct acpi_video_bus *video = acpi_driver_data(device);
 
 	acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY,
 				       acpi_video_bus_notify);
-- 
2.41.0

[PATCH v1 2/6] ACPI: acpi_video: Use yes_or_no helper instead of ternary operator

[Michal Wilczynski]

Use string helper, instead of manually computing "yes" or "no" using
ternary operator.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
 drivers/acpi/acpi_video.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index c14b44f99e35..e17474949bbb 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -2032,9 +2032,9 @@ static int acpi_video_bus_add(struct acpi_device *device)
 
 	pr_info("%s [%s] (multi-head: %s  rom: %s  post: %s)\n",
 	       ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device),
-	       video->flags.multihead ? "yes" : "no",
-	       video->flags.rom ? "yes" : "no",
-	       video->flags.post ? "yes" : "no");
+	       str_yes_no(video->flags.multihead),
+	       str_yes_no(video->flags.rom),
+	       str_yes_no(video->flags.post));
 	mutex_lock(&video_list_lock);
 	list_add_tail(&video->entry, &video_bus_head);
 	mutex_unlock(&video_list_lock);
-- 
2.41.0

[PATCH v1 3/6] ACPI: acpi_video: Remove unnecessary driver_data clear

[Michal Wilczynski]

Commit
0d16710146a1 ("ACPI: bus: Set driver_data to NULL every time .add() fails")
introduced clearing driver_data every time probe fails, so it's not
necessary to clear it in the probe() callback.

Remove NULL assignment to driver_data in error path in
acpi_video_bus_add().

Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
 drivers/acpi/acpi_video.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index e17474949bbb..0c93b0ef0feb 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -2077,7 +2077,6 @@ static int acpi_video_bus_add(struct acpi_device *device)
 	kfree(video->attached_array);
 err_free_video:
 	kfree(video);
-	device->driver_data = NULL;
 
 	return error;
 }
-- 
2.41.0

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

[Michal Wilczynski]

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

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

[PATCH v1 5/6] ACPI: acpi_video: Rename ACPI device instances from device to adev

[Michal Wilczynski]

Since transformation from ACPI driver to platform driver there are
two devices on which the driver operates - ACPI device and platform
device.  For the sake of reader this calls for the distinction in
their naming, to avoid confusion. Rename relevant ACPI devices to adev,
as corresponding platform devices are called pdev.

Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
 drivers/acpi/acpi_video.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 5b9fb3374a2e..bfc7f51a527d 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1526,7 +1526,7 @@ 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_video_bus *video = data;
-	struct acpi_device *device = video->device;
+	struct acpi_device *adev = video->device;
 	struct input_dev *input;
 	int keycode = 0;
 
@@ -1559,12 +1559,12 @@ static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data)
 		break;
 
 	default:
-		acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
+		acpi_handle_debug(adev->handle, "Unsupported event [0x%x]\n",
 				  event);
 		break;
 	}
 
-	if (acpi_notifier_call_chain(device, event, 0))
+	if (acpi_notifier_call_chain(adev, event, 0))
 		/* Something vetoed the keypress. */
 		keycode = 0;
 
@@ -1971,16 +1971,16 @@ static int instance;
 
 static int acpi_video_bus_probe(struct platform_device *pdev)
 {
-	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
 	struct acpi_video_bus *video;
 	bool auto_detect;
 	int error;
 	acpi_status status;
 
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE,
-				acpi_dev_parent(device)->handle, 1,
-				acpi_video_bus_match, NULL,
-				device, NULL);
+				     acpi_dev_parent(adev)->handle, 1,
+				     acpi_video_bus_match, NULL,
+				     adev, NULL);
 	if (status == AE_ALREADY_EXISTS) {
 		pr_info(FW_BUG
 			"Duplicate ACPI video bus devices for the"
@@ -1996,21 +1996,21 @@ static int acpi_video_bus_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	/* a hack to fix the duplicate name "VID" problem on T61 */
-	if (!strcmp(device->pnp.bus_id, "VID")) {
+	if (!strcmp(adev->pnp.bus_id, "VID")) {
 		if (instance)
-			device->pnp.bus_id[3] = '0' + instance;
+			adev->pnp.bus_id[3] = '0' + instance;
 		instance++;
 	}
 	/* a hack to fix the duplicate name "VGA" problem on Pa 3553 */
-	if (!strcmp(device->pnp.bus_id, "VGA")) {
+	if (!strcmp(adev->pnp.bus_id, "VGA")) {
 		if (instance)
-			device->pnp.bus_id[3] = '0' + instance;
+			adev->pnp.bus_id[3] = '0' + instance;
 		instance++;
 	}
 
-	video->device = device;
-	strcpy(acpi_device_name(device), ACPI_VIDEO_BUS_NAME);
-	strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
+	video->device = adev;
+	strcpy(acpi_device_name(adev), ACPI_VIDEO_BUS_NAME);
+	strcpy(acpi_device_class(adev), ACPI_VIDEO_CLASS);
 	platform_set_drvdata(pdev, video);
 
 	acpi_video_bus_find_cap(video);
@@ -2021,7 +2021,7 @@ static int acpi_video_bus_probe(struct platform_device *pdev)
 	mutex_init(&video->device_list_lock);
 	INIT_LIST_HEAD(&video->video_device_list);
 
-	error = acpi_video_bus_get_devices(video, device);
+	error = acpi_video_bus_get_devices(video, adev);
 	if (error)
 		goto err_put_video;
 
@@ -2029,10 +2029,10 @@ static int acpi_video_bus_probe(struct platform_device *pdev)
 	 * HP ZBook Fury 16 G10 requires ACPI video's child devices have _PS0
 	 * evaluated to have functional panel brightness control.
 	 */
-	acpi_device_fix_up_power_extended(device);
+	acpi_device_fix_up_power_extended(adev);
 
 	pr_info("%s [%s] (multi-head: %s  rom: %s  post: %s)\n",
-	       ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device),
+	       ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(adev),
 	       str_yes_no(video->flags.multihead),
 	       str_yes_no(video->flags.rom),
 	       str_yes_no(video->flags.post));
@@ -2059,7 +2059,7 @@ static int acpi_video_bus_probe(struct platform_device *pdev)
 	if (error)
 		goto err_del;
 
-	error = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY,
+	error = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY,
 						acpi_video_bus_notify, video);
 	if (error)
 		goto err_remove;
-- 
2.41.0

[PATCH v1 6/6] ACPI: acpi_video: Fix holes in acpi_video_bus

[Michal Wilczynski]

As identified by 'pahole' utility there are holes in acpi_video_bus
struct. Rearrange elements to get rid of the holes. Put elements
biggest in size first, and one-byte elements later.

Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
 drivers/acpi/acpi_video.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index bfc7f51a527d..1e10d5b748b2 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -155,19 +155,19 @@ struct acpi_video_enumerated_device {
 
 struct acpi_video_bus {
 	struct acpi_device *device;
+	struct acpi_video_enumerated_device *attached_array;
+	struct list_head video_device_list;
+	struct mutex device_list_lock;	/* protects video_device_list */
+	struct list_head entry;
+	struct input_dev *input;
+	struct notifier_block pm_nb;
 	bool backlight_registered;
 	u8 dos_setting;
-	struct acpi_video_enumerated_device *attached_array;
 	u8 attached_count;
 	u8 child_count;
 	struct acpi_video_bus_cap cap;
 	struct acpi_video_bus_flags flags;
-	struct list_head video_device_list;
-	struct mutex device_list_lock;	/* protects video_device_list */
-	struct list_head entry;
-	struct input_dev *input;
 	char phys[32];	/* for input device */
-	struct notifier_block pm_nb;
 };
 
 struct acpi_video_device_flags {
-- 
2.41.0

Re: [PATCH v1 0/6] Replace acpi_driver with platform_driver

[Rafael J. Wysocki]

On Wed, Oct 25, 2023 at 2:34 PM Michal Wilczynski
<michal.wilczynski@intel.com> wrote:
>
> This patchset is a continuation of efforts from [1] aiming to replace
> acpi_driver with platform_driver. To ease up review effort I'm sending
> miniseries per driver, with a replacement patch + various improvements
> that were noticed by me, or during internal review.
>
> This mini-series takes care of acpi_video driver.
>
> [1] - https://lore.kernel.org/linux-acpi/20231011083334.3987477-1-michal.wilczynski@intel.com/T/#t
>
> Michal Wilczynski (6):
>   ACPI: acpi_video: Remove unnecessary checks
>   ACPI: acpi_video: Use yes_or_no helper instead of ternary operator
>   ACPI: acpi_video: Remove unnecessary driver_data clear
>   ACPI: acpi_video: Replace acpi_driver with platform_driver
>   ACPI: acpi_video: Rename ACPI device instances from device to adev
>   ACPI: acpi_video: Fix holes in acpi_video_bus
>
>  drivers/acpi/acpi_video.c | 101 +++++++++++++++++---------------------
>  1 file changed, 46 insertions(+), 55 deletions(-)
>
> --

Because this is not going to get into 6.7 anyway, I'm deferring the
review of it until 6.7-rc1 is out.

Thanks!

Re: [PATCH v1 3/6] ACPI: acpi_video: Remove unnecessary driver_data clear

[Andy Shevchenko]

On Wed, Oct 25, 2023 at 02:18:03PM +0300, Michal Wilczynski wrote:
> Commit
> 0d16710146a1 ("ACPI: bus: Set driver_data to NULL every time .add() fails")

It's fine to wrap it here, it's just a restriction in the tag block that one
tag == one line. Also I believe the form "The commit" sounds more English :-)
as reader knows exact commit we are talking about.

> introduced clearing driver_data every time probe fails, so it's not
> necessary to clear it in the probe() callback.

> Remove NULL assignment to driver_data in error path in
> acpi_video_bus_add().

This can be one line.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 6/6] ACPI: acpi_video: Fix holes in acpi_video_bus

[Andy Shevchenko]

On Wed, Oct 25, 2023 at 02:18:06PM +0300, Michal Wilczynski wrote:
> As identified by 'pahole' utility there are holes in acpi_video_bus
> struct. Rearrange elements to get rid of the holes. Put elements
> biggest in size first, and one-byte elements later.

How does this affect the code generation (e.g., measured by bloat-o-meter)?

-- 
With Best Regards,
Andy Shevchenko

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

[Rafael J. Wysocki]

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

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

[Hans de Goede]

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