Skip to content

Commit 7c8b81f

Browse files
committed
Merge branch 'acpi-driver'
Merge updates of drivers handling devices defined in the ACPI specification and other generic devices with ACPI interfaces for 6.20-rc1/7.0-rc1: - Add a piece of documentation explaining why binding drivers directly to ACPI device objects is not a good idea in general and why it is desirable to convert drivers doing so into proper platform drivers that use struct platform_driver for device binding (Rafael Wysocki) - Convert multiple "core ACPI" drivers, including the NFIT ACPI device driver, the generic ACPI button drivers, the generic ACPI thermal zone driver, the ACPI hardware event device (HED) driver, the ACPI EC driver, the ACPI SMBUS HC driver, the ACPI Smart Battery Subsystem (SBS) driver, and the ACPI backlight (video) driver to proper platform drivers that use struct platform_driver for device binding (Rafael Wysocki) - Use acpi_get_local_u64_address() in the ACPI backlight (video) driver to evaluate _ADR instead of evaluating that object directly (Andy Shevchenko) * acpi-driver: (25 commits) ACPI: video: simplify code with acpi_get_local_u64_address() ACPI: scan: Clean up after recent changes ACPI: scan: Use acpi_setup_gpe_for_wake() for buttons ACPI: PM: Let acpi_dev_pm_attach() skip devices without ACPI PM ACPI: Documentation: driver-api: Disapprove of using ACPI drivers ACPI: video: Convert the driver to a platform one ACPI: video: Adjust event notification routine ACPI: scan: Register platform devices for backlight device objects ACPI: SBS: Convert the driver to a platform one ACPI: SMBUS HC: Convert the driver to a platform one ACPI: EC: Convert the driver to a platform one ACPI: EC: Register a platform device for ECDT EC ACPI: HED: Convert the driver to a platform one ACPI: thermal: Rework system suspend and resume handling ACPI: thermal: Convert the driver to a platform one ACPI: thermal: Adjust event notification routine ACPI: scan: Register platform devices for thermal zones ACPI: scan: Do not mark button ACPI devices as wakeup-capable ACPI: scan: Do not bind ACPI drivers to fixed event buttons ACPI: tiny-power-button: Convert the driver to a platform one ...
2 parents 4322612 + 5315c0d commit 7c8b81f

15 files changed

Lines changed: 380 additions & 338 deletions

File tree

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
.. SPDX-License-Identifier: GPL-2.0
2+
.. include:: <isonum.txt>
3+
4+
=========================================
5+
Why using ACPI drivers is not a good idea
6+
=========================================
7+
8+
:Copyright: |copy| 2026, Intel Corporation
9+
10+
:Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
11+
12+
Even though binding drivers directly to struct acpi_device objects, also
13+
referred to as "ACPI device nodes", allows basic functionality to be provided
14+
at least in some cases, there are problems with it, related to general
15+
consistency, sysfs layout, power management operation ordering, and code
16+
cleanliness.
17+
18+
First of all, ACPI device nodes represent firmware entities rather than
19+
hardware and in many cases they provide auxiliary information on devices
20+
enumerated independently (like PCI devices or CPUs). It is therefore generally
21+
questionable to assign resources to them because the entities represented by
22+
them do not decode addresses in the memory or I/O address spaces and do not
23+
generate interrupts or similar (all of that is done by hardware).
24+
25+
Second, as a general rule, a struct acpi_device can only be a parent of another
26+
struct acpi_device. If that is not the case, the location of the child device
27+
in the device hierarchy is at least confusing and it may not be straightforward
28+
to identify the piece of hardware providing functionality represented by it.
29+
However, binding a driver directly to an ACPI device node may cause that to
30+
happen if the given driver registers input devices or wakeup sources under it,
31+
for example.
32+
33+
Next, using system suspend and resume callbacks directly on ACPI device nodes
34+
is also questionable because it may cause ordering problems to appear. Namely,
35+
ACPI device nodes are registered before enumerating hardware corresponding to
36+
them and they land on the PM list in front of the majority of other device
37+
objects. Consequently, the execution ordering of their PM callbacks may be
38+
different from what is generally expected. Also, in general, dependencies
39+
returned by _DEP objects do not affect ACPI device nodes themselves, but the
40+
"physical" devices associated with them, which potentially is one more source
41+
of inconsistency related to treating ACPI device nodes as "real" device
42+
representation.
43+
44+
All of the above means that binding drivers to ACPI device nodes should
45+
generally be avoided and so struct acpi_driver objects should not be used.
46+
47+
Moreover, a device ID is necessary to bind a driver directly to an ACPI device
48+
node, but device IDs are not generally associated with all of them. Some of
49+
them contain alternative information allowing the corresponding pieces of
50+
hardware to be identified, for example represeted by an _ADR object return
51+
value, and device IDs are not used in those cases. In consequence, confusingly
52+
enough, binding an ACPI driver to an ACPI device node may even be impossible.
53+
54+
When that happens, the piece of hardware corresponding to the given ACPI device
55+
node is represented by another device object, like a struct pci_dev, and the
56+
ACPI device node is the "ACPI companion" of that device, accessible through its
57+
fwnode pointer used by the ACPI_COMPANION() macro. The ACPI companion holds
58+
additional information on the device configuration and possibly some "recipes"
59+
on device manipulation in the form of AML (ACPI Machine Language) bytecode
60+
provided by the platform firmware. Thus the role of the ACPI device node is
61+
similar to the role of a struct device_node on a system where Device Tree is
62+
used for platform description.
63+
64+
For consistency, this approach has been extended to the cases in which ACPI
65+
device IDs are used. Namely, in those cases, an additional device object is
66+
created to represent the piece of hardware corresponding to a given ACPI device
67+
node. By default, it is a platform device, but it may also be a PNP device, a
68+
CPU device, or another type of device, depending on what the given piece of
69+
hardware actually is. There are even cases in which multiple devices are
70+
"backed" or "accompanied" by one ACPI device node (e.g. ACPI device nodes
71+
corresponding to GPUs that may provide firmware interfaces for backlight
72+
brightness control in addition to GPU configuration information).
73+
74+
This means that it really should never be necessary to bind a driver directly to
75+
an ACPI device node because there is a "proper" device object representing the
76+
corresponding piece of hardware that can be bound to by a "proper" driver using
77+
the given ACPI device node as the device's ACPI companion. Thus, in principle,
78+
there is no reason to use ACPI drivers and if they all were replaced with other
79+
driver types (for example, platform drivers), some code could be dropped and
80+
some complexity would go away.

Documentation/driver-api/acpi/index.rst

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,4 @@ ACPI Support
77

88
linuxized-acpica
99
scan_handlers
10+
acpi-drivers

drivers/acpi/acpi_platform.c

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -114,13 +114,11 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
114114
struct platform_device *pdev = NULL;
115115
struct platform_device_info pdevinfo;
116116
const struct acpi_device_id *match;
117-
struct resource_entry *rentry;
118-
struct list_head resource_list;
119117
struct resource *resources = NULL;
120-
int count;
118+
int count = 0;
121119

122120
/* If the ACPI node already has a physical device attached, skip it. */
123-
if (adev->physical_node_count)
121+
if (adev->physical_node_count && !adev->pnp.type.backlight)
124122
return NULL;
125123

126124
match = acpi_match_acpi_device(forbidden_id_list, adev);
@@ -137,22 +135,28 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
137135
}
138136
}
139137

140-
INIT_LIST_HEAD(&resource_list);
141-
count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
142-
if (count < 0)
143-
return NULL;
144-
if (count > 0) {
145-
resources = kcalloc(count, sizeof(*resources), GFP_KERNEL);
146-
if (!resources) {
138+
if (adev->device_type == ACPI_BUS_TYPE_DEVICE && !adev->pnp.type.backlight) {
139+
LIST_HEAD(resource_list);
140+
141+
count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
142+
if (count < 0)
143+
return ERR_PTR(-ENODATA);
144+
145+
if (count > 0) {
146+
struct resource_entry *rentry;
147+
148+
resources = kcalloc(count, sizeof(*resources), GFP_KERNEL);
149+
if (!resources) {
150+
acpi_dev_free_resource_list(&resource_list);
151+
return ERR_PTR(-ENOMEM);
152+
}
153+
count = 0;
154+
list_for_each_entry(rentry, &resource_list, node)
155+
acpi_platform_fill_resource(adev, rentry->res,
156+
&resources[count++]);
157+
147158
acpi_dev_free_resource_list(&resource_list);
148-
return ERR_PTR(-ENOMEM);
149159
}
150-
count = 0;
151-
list_for_each_entry(rentry, &resource_list, node)
152-
acpi_platform_fill_resource(adev, rentry->res,
153-
&resources[count++]);
154-
155-
acpi_dev_free_resource_list(&resource_list);
156160
}
157161

158162
memset(&pdevinfo, 0, sizeof(pdevinfo));

drivers/acpi/acpi_video.c

Lines changed: 28 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <linux/sort.h>
2222
#include <linux/pci.h>
2323
#include <linux/pci_ids.h>
24+
#include <linux/platform_device.h>
2425
#include <linux/slab.h>
2526
#include <linux/dmi.h>
2627
#include <linux/suspend.h>
@@ -76,8 +77,8 @@ static int register_count;
7677
static DEFINE_MUTEX(register_count_mutex);
7778
static DEFINE_MUTEX(video_list_lock);
7879
static LIST_HEAD(video_bus_head);
79-
static int acpi_video_bus_add(struct acpi_device *device);
80-
static void acpi_video_bus_remove(struct acpi_device *device);
80+
static int acpi_video_bus_probe(struct platform_device *pdev);
81+
static void acpi_video_bus_remove(struct platform_device *pdev);
8182
static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data);
8283

8384
/*
@@ -98,14 +99,13 @@ static const struct acpi_device_id video_device_ids[] = {
9899
};
99100
MODULE_DEVICE_TABLE(acpi, video_device_ids);
100101

101-
static struct acpi_driver acpi_video_bus = {
102-
.name = "video",
103-
.class = ACPI_VIDEO_CLASS,
104-
.ids = video_device_ids,
105-
.ops = {
106-
.add = acpi_video_bus_add,
107-
.remove = acpi_video_bus_remove,
108-
},
102+
static struct platform_driver acpi_video_bus = {
103+
.probe = acpi_video_bus_probe,
104+
.remove = acpi_video_bus_remove,
105+
.driver = {
106+
.name = "acpi-video",
107+
.acpi_match_table = video_device_ids,
108+
},
109109
};
110110

111111
struct acpi_video_bus_flags {
@@ -1134,13 +1134,11 @@ static int acpi_video_bus_get_one_device(struct acpi_device *device, void *arg)
11341134
struct acpi_video_bus *video = arg;
11351135
struct acpi_video_device_attrib *attribute;
11361136
struct acpi_video_device *data;
1137-
unsigned long long device_id;
1138-
acpi_status status;
11391137
int device_type;
1138+
u64 device_id;
11401139

1141-
status = acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
11421140
/* Skip devices without _ADR instead of failing. */
1143-
if (ACPI_FAILURE(status))
1141+
if (acpi_get_local_u64_address(device->handle, &device_id))
11441142
goto exit;
11451143

11461144
data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
@@ -1540,14 +1538,11 @@ static int acpi_video_bus_stop_devices(struct acpi_video_bus *video)
15401538

15411539
static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data)
15421540
{
1543-
struct acpi_device *device = data;
1544-
struct acpi_video_bus *video = acpi_driver_data(device);
1541+
struct acpi_video_bus *video = data;
1542+
struct acpi_device *device = video->device;
15451543
struct input_dev *input;
15461544
int keycode = 0;
15471545

1548-
if (!video || !video->input)
1549-
return;
1550-
15511546
input = video->input;
15521547

15531548
switch (event) {
@@ -1891,7 +1886,8 @@ static void acpi_video_dev_add_notify_handler(struct acpi_video_device *device)
18911886
device->flags.notify = 1;
18921887
}
18931888

1894-
static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video)
1889+
static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video,
1890+
struct platform_device *pdev)
18951891
{
18961892
struct input_dev *input;
18971893
struct acpi_video_device *dev;
@@ -1914,7 +1910,7 @@ static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video)
19141910
input->phys = video->phys;
19151911
input->id.bustype = BUS_HOST;
19161912
input->id.product = 0x06;
1917-
input->dev.parent = &video->device->dev;
1913+
input->dev.parent = &pdev->dev;
19181914
input->evbit[0] = BIT(EV_KEY);
19191915
set_bit(KEY_SWITCHVIDEOMODE, input->keybit);
19201916
set_bit(KEY_VIDEO_NEXT, input->keybit);
@@ -1986,8 +1982,9 @@ static int acpi_video_bus_put_devices(struct acpi_video_bus *video)
19861982

19871983
static int instance;
19881984

1989-
static int acpi_video_bus_add(struct acpi_device *device)
1985+
static int acpi_video_bus_probe(struct platform_device *pdev)
19901986
{
1987+
struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
19911988
struct acpi_video_bus *video;
19921989
bool auto_detect;
19931990
int error;
@@ -2024,6 +2021,8 @@ static int acpi_video_bus_add(struct acpi_device *device)
20242021
instance++;
20252022
}
20262023

2024+
platform_set_drvdata(pdev, video);
2025+
20272026
video->device = device;
20282027
strscpy(acpi_device_name(device), ACPI_VIDEO_BUS_NAME);
20292028
strscpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
@@ -2071,12 +2070,12 @@ static int acpi_video_bus_add(struct acpi_device *device)
20712070
!auto_detect)
20722071
acpi_video_bus_register_backlight(video);
20732072

2074-
error = acpi_video_bus_add_notify_handler(video);
2073+
error = acpi_video_bus_add_notify_handler(video, pdev);
20752074
if (error)
20762075
goto err_del;
20772076

20782077
error = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY,
2079-
acpi_video_bus_notify, device);
2078+
acpi_video_bus_notify, video);
20802079
if (error)
20812080
goto err_remove;
20822081

@@ -2099,15 +2098,10 @@ static int acpi_video_bus_add(struct acpi_device *device)
20992098
return error;
21002099
}
21012100

2102-
static void acpi_video_bus_remove(struct acpi_device *device)
2101+
static void acpi_video_bus_remove(struct platform_device *pdev)
21032102
{
2104-
struct acpi_video_bus *video = NULL;
2105-
2106-
2107-
if (!device || !acpi_driver_data(device))
2108-
return;
2109-
2110-
video = acpi_driver_data(device);
2103+
struct acpi_video_bus *video = platform_get_drvdata(pdev);
2104+
struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
21112105

21122106
acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY,
21132107
acpi_video_bus_notify);
@@ -2170,7 +2164,7 @@ int acpi_video_register(void)
21702164

21712165
dmi_check_system(video_dmi_table);
21722166

2173-
ret = acpi_bus_register_driver(&acpi_video_bus);
2167+
ret = platform_driver_register(&acpi_video_bus);
21742168
if (ret)
21752169
goto leave;
21762170

@@ -2190,7 +2184,7 @@ void acpi_video_unregister(void)
21902184
{
21912185
mutex_lock(&register_count_mutex);
21922186
if (register_count) {
2193-
acpi_bus_unregister_driver(&acpi_video_bus);
2187+
platform_driver_unregister(&acpi_video_bus);
21942188
register_count = 0;
21952189
may_report_brightness_keys = false;
21962190
}

drivers/acpi/bus.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -818,6 +818,9 @@ const struct acpi_device *acpi_companion_match(const struct device *dev)
818818
if (list_empty(&adev->pnp.ids))
819819
return NULL;
820820

821+
if (adev->pnp.type.backlight)
822+
return adev;
823+
821824
return acpi_primary_dev_companion(adev, dev);
822825
}
823826

0 commit comments

Comments
 (0)