[PATCH v1 0/3] ACPI: video: Convert ACPI video driver to a platform one

[PATCH v1 0/3] ACPI: video: Convert ACPI video driver to a platform one

[Rafael J. Wysocki]

Hi All,

This is a continuation of

https://lore.kernel.org/linux-acpi/2339822.iZASKD2KPV@rafael.j.wysocki/

specifically concerning the ACPI video (backlight) driver which is kind of
a special case because ACPI backlight device objects it binds to are ACPI
companions of PCI devices (GPUs).

The general rationale is the same as for the series linked above.

While binding drivers directly to struct acpi_device objects allows
basic functionality to be provided, at least in the majority of cases,
there are some problems with it, related to general consistency, sysfs
layout, power management operation ordering, and code cleanliness.

First of all, struct acpi_device objects represent firmware entities
rather than hardware and in many cases they provide auxiliary information
on devices enumerated independently (like PCI devices or CPUs).  It is
therefore generally questionable to assign resources to them or create
class devices and similar under them because they don't provide
functionality associated with those entities by themselves (for example,
they don't generate wakeup or input events).

As a general rule, a struct acpi_device can only be a parent of another
struct acpi_device.  If that's not the case, the location of the child
device in the device hierarchy is at least confusing and it may not be
straightforward to identify the piece of hardware corresponding to that
device.

Using system suspend and resume callbacks directly with struct acpi_device
objects is questionable either because it may cause ordering problems to
happen.  Namely, struct acpi_device objects are registered before any
devices corresponded to by them and they land on the PM list before all
of those devices.  Consequently, the execution ordering of their PM
callbacks may be different from what is generally expected.  Moreover,
dependencies returned by _DEP objects don't generally affect struct
acpi_device objects themselves, only the "physical" device objects
associated with them, which potentially is one more source of inconsistency.

All of the above means that binding drivers to struct acpi_device "devices"
should generally be avoided and so this series converts the ACPI video
driver to a platform one.

Patches [1-2/3] are preparatory and patch [3/3] is the driver conversion one.

Thanks!

[PATCH v1 1/3] ACPI: scan: Register platform devices for backlight device objects

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

ACPI device objects associated with backlight interfaces are special
because they are ACPI companions of PCI devices (GPUs), but the
interfaces exposed by them resemble platform device one.

Currently, the ACPI video driver binds to them with the help of a
special "synthetic" device ID regardless of the pairing with the PCI
devices, but since it is generally better to use platform drivers for
handling such interfaces, the plan is to convert that drviver into a
platform one.

However, for this purpose, platform devices corresponding to the
ACPI backlight device objects need to be registered, so update
acpi_bus_attach() to apply the default ACPI enumeration to them
and modify acpi_create_platform_device() to avoid bailing out early
if a "physical" device is already attached to a backlight ACPI device
object.

In addition, update acpi_companion_match() to return a valid struct
acpi_device pointer if the ACPI companion of the given device is a
backlight ACPI device object, which will facilitate driver matching
for platform devices corresponding to those objects.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/acpi_platform.c |    5 +++--
 drivers/acpi/bus.c           |    3 +++
 drivers/acpi/scan.c          |    3 ++-
 3 files changed, 8 insertions(+), 3 deletions(-)

--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -120,7 +120,7 @@ struct platform_device *acpi_create_plat
 	int count;
 
 	/* If the ACPI node already has a physical device attached, skip it. */
-	if (adev->physical_node_count)
+	if (adev->physical_node_count && !adev->pnp.type.backlight)
 		return NULL;
 
 	match = acpi_match_acpi_device(forbidden_id_list, adev);
@@ -138,7 +138,8 @@ struct platform_device *acpi_create_plat
 	}
 
 	INIT_LIST_HEAD(&resource_list);
-	if (adev->device_type == ACPI_BUS_TYPE_DEVICE) {
+	if (adev->device_type == ACPI_BUS_TYPE_DEVICE &&
+	    !adev->pnp.type.backlight) {
 		count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
 		if (count < 0)
 			return NULL;
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -761,6 +761,9 @@ const struct acpi_device *acpi_companion
 	if (list_empty(&adev->pnp.ids))
 		return NULL;
 
+	if (adev->pnp.type.backlight)
+		return adev;
+
 	return acpi_primary_dev_companion(adev, dev);
 }
 
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2320,7 +2320,8 @@ static int acpi_bus_attach(struct acpi_d
 	if (ret < 0)
 		return 0;
 
-	if (device->pnp.type.platform_id || device->flags.enumeration_by_parent)
+	if (device->pnp.type.platform_id || device->pnp.type.backlight ||
+	    device->flags.enumeration_by_parent)
 		acpi_default_enumeration(device);
 	else
 		acpi_device_set_enumerated(device);

[PATCH v1 2/3] ACPI: video: Adjust event notification routine

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Adjust acpi_video_bus_notify() to cast its "data" argument to a struct
acpi_video_bus pointer istead of a struct acpi_device one, which allows
the use of acpi_driver_data() to be limited and will facilitate
subsequent changes.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/acpi_video.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1540,14 +1540,11 @@ static int acpi_video_bus_stop_devices(s
 
 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;
 
-	if (!video || !video->input)
-		return;
-
 	input = video->input;
 
 	switch (event) {
@@ -2076,7 +2073,7 @@ static int acpi_video_bus_add(struct acp
 		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;
 

[PATCH v1 3/3] ACPI: video: Convert the driver to a platform one

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

While binding drivers directly to struct acpi_device objects allows
basic functionality to be provided, at least in the majority of cases,
there are some problems with it, related to general consistency, sysfs
layout, power management operation ordering, and code cleanliness.

Overall, it is better to bind drivers to platform devices than to their
ACPI companions, so convert the ACPI video driver to a platform one.

While this is not expected to alter functionality, it changes sysfs
layout and so it will be visible to user space.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/acpi_video.c |   47 ++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -21,6 +21,7 @@
 #include <linux/sort.h>
 #include <linux/pci.h>
 #include <linux/pci_ids.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/dmi.h>
 #include <linux/suspend.h>
@@ -76,8 +77,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);
 
 /*
@@ -98,14 +99,13 @@ static const struct acpi_device_id video
 };
 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 = acpi_video_bus_remove,
+	.driver = {
+		.name = "acpi-video",
+		.acpi_match_table = video_device_ids,
+ 	},
 };
 
 struct acpi_video_bus_flags {
@@ -1888,7 +1888,8 @@ static void acpi_video_dev_add_notify_ha
 		device->flags.notify = 1;
 }
 
-static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video)
+static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video,
+					     struct platform_device *pdev)
 {
 	struct input_dev *input;
 	struct acpi_video_device *dev;
@@ -1911,7 +1912,7 @@ static int acpi_video_bus_add_notify_han
 	input->phys = video->phys;
 	input->id.bustype = BUS_HOST;
 	input->id.product = 0x06;
-	input->dev.parent = &video->device->dev;
+	input->dev.parent = &pdev->dev;
 	input->evbit[0] = BIT(EV_KEY);
 	set_bit(KEY_SWITCHVIDEOMODE, input->keybit);
 	set_bit(KEY_VIDEO_NEXT, input->keybit);
@@ -1983,8 +1984,9 @@ static int acpi_video_bus_put_devices(st
 
 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;
@@ -2021,6 +2023,8 @@ static int acpi_video_bus_add(struct acp
 		instance++;
 	}
 
+	platform_set_drvdata(pdev, video);
+
 	video->device = device;
 	strscpy(acpi_device_name(device), ACPI_VIDEO_BUS_NAME);
 	strscpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
@@ -2068,7 +2072,7 @@ static int acpi_video_bus_add(struct acp
 	    !auto_detect)
 		acpi_video_bus_register_backlight(video);
 
-	error = acpi_video_bus_add_notify_handler(video);
+	error = acpi_video_bus_add_notify_handler(video, pdev);
 	if (error)
 		goto err_del;
 
@@ -2096,15 +2100,10 @@ err_free_video:
 	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 = NULL;
-
-
-	if (!device || !acpi_driver_data(device))
-		return;
-
-	video = acpi_driver_data(device);
+	struct acpi_video_bus *video = platform_get_drvdata(pdev);
+	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
 
 	acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY,
 				       acpi_video_bus_notify);
@@ -2167,7 +2166,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;
 
@@ -2187,7 +2186,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;
 	}