Hi Everyone,
This has been in the works for some time, but the official document it is
based on was not quite ready before the last week. It now is available at
http://www.uefi.org/sites/default/files/resources/_DSD-hierarchical-data-extension-UUID-v1.pdf
The issue at hand is that we need to be able to support hierarchical device
properties in ACPI, like when the set of properties of an entity called "Fred"
may include a subset called "dog" containing the properties of the Fred's dog
rather than those of Fred himself. And it may make sense to have the same
property, like "hair color", for both Fred and the dog, but with different
values.
We (I, Darren and Dave at least) have explored many possible ways to deal with
that in ACPI, but the majority of them turned out to be unattractive for various
reasons. Our first take was to use ACPI device objects to make the "child"
property sets available via _DSD, but that approach is generally incompatible
with the PnP Manager in Windows following the notion that all device objects
in ACPI tables are supposed to represent real devices. It can still be made
work by adding _STA that returns 0 to those "property-only" device objects,
but that leads to complications in other places and is error prone (if the _STA
is forgotten, for example). Moreover, it adds quite a bit of overhead even in
Linux (an ACPICA representation, struct acpi_device, driver core mechanics etc)
for things that are only supposed to represent sets of device properties. And,
in addition to that, we'd need to figure out how to give those things arbitrary
names in a consistent way. All of that caused us to drop the approach based on
device objects and look for other options.
One of those was to nest the "child" property sets within _DSD packages, but it
follows from experience that this is error prone too (firmware people tend to have
problems with getting deeply nested packages right in ASL) and we wanted to be
able to visually distinguish those sets as separate entities in ASL code. That
led us to the directory concept defined by the document mentioned above.
The idea is that _DSD may return a package containing the properties of the
device it belongs to along with a directory of objects that need to be evaluated
in order to obtain the "child" property sets of it. That directory needs to be
present in a separate section of the _DSD package (after the new UUID defined in
the above document) and is a list of two-element sub-packages (entries) where
the first element is the name of the entry (the name of the "child" property set
represented by it) and the second element is a "pointer" (essentially, a path
or "namestring" in the ACPI terminology) to the control method or a static
data package that needs to be evaluated to obtain the entry's data. The data
returned by it is then interpreted in the same way as a _DSD return package,
so it may also contain properties and a directory of its own "child" property
sets.
As an example, consider the following ASL from an experimental MinnowBoard
firmware:
Device (LEDS)
{
Name (_HID, "PRP0001")
Name (_CRS, ResourceTemplate () {
GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
"\\_SB.PCI0.LPC", 0, ResourceConsumer) {10}
GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
"\\_SB.PCI0.LPC", 0, ResourceConsumer) {11}
})
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () {"compatible", Package () {"gpio-leds"}},
},
// Data extension
ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
Package () {
Package () {"heartbeat", "LEDH"},
Package () {"mmc-activity", "LEDM"},
}
})
Name (LEDH, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () {"label", "Heartbeat"},
Package () {"gpios", Package () {^LEDS, 0, 0, 0}},
Package () {"linux,default-trigger", "heartbeat"},
Package () {"linux,default-state", "off"},
Package () {"linux,retain-state-suspended", 1},
}
})
Name (LEDM, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () {"label", Package () {"MMC0 Activity"}},
Package () {"gpios", Package () {^LEDS, 1, 0, 0}},
Package () {"linux,default-trigger", Package () {"mmc0"}},
Package () {"linux,default-state", "off"},
Package () {"linux,retain-state-suspended", 1},
}
})
}
where each LED in a GPIO LED array is represented by a "child" property set of
the master device object LEDS. There are two "child" sets called "heartbeat"
and "mmc-activity" whose data come from the LEDH and LEDM static data
packages under LEDS, respectively.
The patch series introduces an infrastructure allowing "child" property
sets like the above to be accessed via the generic device properties API.
It represents those property sets as structures extending struct fwnode_handle
with the new type FWNODE_ACPI_DATA and reworks the ACPI property handling
code to do the right thing if an fwnode_handle of that type is passed to it
(please refer to the patch changelogs for details).
Please note that this new mechanism is not a replacement for anything. It
simply adds general support for representing hierarchical properties of
devices in a new way, but things that worked previously should still work.
Also please let me know if you have objections agaist this approach or
suggestions on improving the code.
Although the 4.3 merge window has not officially opened yet, I'm not regarding
this as 4.3 material unless someone wants it in 4.3 really badly, in which
case please let me know ASAP as well.
Thanks,
Rafael
From: Rafael J. Wysocki <[email protected]>
Move the extraction of _DSD properties from acpi_init_properties()
to a separate routine called acpi_extract_properties() to make the
subsequent changes more straightforward.
Signed-off-by: Rafael J. Wysocki <[email protected]>
Tested-by: Mika Westerberg <[email protected]>
---
drivers/acpi/property.c | 69 +++++++++++++++++++++++++-----------------------
1 file changed, 37 insertions(+), 32 deletions(-)
Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -100,34 +100,13 @@ static void acpi_init_of_compatible(stru
adev->flags.of_compatible_ok = 1;
}
-void acpi_init_properties(struct acpi_device *adev)
+static bool acpi_extract_properties(const union acpi_object *desc,
+ struct acpi_device_data *data)
{
- struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
- bool acpi_of = false;
- struct acpi_hardware_id *hwid;
- const union acpi_object *desc;
- acpi_status status;
int i;
- /*
- * Check if ACPI_DT_NAMESPACE_HID is present and inthat case we fill in
- * Device Tree compatible properties for this device.
- */
- list_for_each_entry(hwid, &adev->pnp.ids, list) {
- if (!strcmp(hwid->id, ACPI_DT_NAMESPACE_HID)) {
- acpi_of = true;
- break;
- }
- }
-
- status = acpi_evaluate_object_typed(adev->handle, "_DSD", NULL, &buf,
- ACPI_TYPE_PACKAGE);
- if (ACPI_FAILURE(status))
- goto out;
-
- desc = buf.pointer;
if (desc->package.count % 2)
- goto fail;
+ return false;
/* Look for the device properties UUID. */
for (i = 0; i < desc->package.count; i += 2) {
@@ -154,18 +133,44 @@ void acpi_init_properties(struct acpi_de
if (!acpi_properties_format_valid(properties))
break;
- adev->data.pointer = buf.pointer;
- adev->data.properties = properties;
+ data->properties = properties;
+ return true;
+ }
- if (acpi_of)
- acpi_init_of_compatible(adev);
+ return false;
+}
- goto out;
+void acpi_init_properties(struct acpi_device *adev)
+{
+ struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
+ struct acpi_hardware_id *hwid;
+ acpi_status status;
+ bool acpi_of = false;
+
+ /*
+ * Check if ACPI_DT_NAMESPACE_HID is present and inthat case we fill in
+ * Device Tree compatible properties for this device.
+ */
+ list_for_each_entry(hwid, &adev->pnp.ids, list) {
+ if (!strcmp(hwid->id, ACPI_DT_NAMESPACE_HID)) {
+ acpi_of = true;
+ break;
+ }
}
- fail:
- dev_dbg(&adev->dev, "Returned _DSD data is not valid, skipping\n");
- ACPI_FREE(buf.pointer);
+ status = acpi_evaluate_object_typed(adev->handle, "_DSD", NULL, &buf,
+ ACPI_TYPE_PACKAGE);
+ if (ACPI_FAILURE(status))
+ goto out;
+
+ if (acpi_extract_properties(buf.pointer, &adev->data)) {
+ adev->data.pointer = buf.pointer;
+ if (acpi_of)
+ acpi_init_of_compatible(adev);
+ } else {
+ acpi_handle_debug(adev->handle, "Invalid _DSD data, skipping\n");
+ ACPI_FREE(buf.pointer);
+ }
out:
if (acpi_of && !adev->flags.of_compatible_ok)
From: Rafael J. Wysocki <[email protected]>
In some cases, the information expressed via device properties is
hierarchical by nature. For example, the properties of a composite
device consisting of multiple semi-dependent components may need
to be represented in the form of a tree of property data sets
corresponding to specific components of the device.
Unfortunately, using ACPI device objects for this purpose turns out
to be problematic, mostly due to the assumption made by some operating
systems (that platform firmware generally needs to work with) that
each device object in the ACPI namespace represents a device requiring
a separate driver. That assumption leads to complications which
reportedly are impractically difficult to overcome and a different
approach is needed for the sake of interoperability.
The approach implemented here is based on extending _DSD via pointers
(links) to additional ACPI objects returning data packages formatted
in accordance with the _DSD formatting rules defined by Section 6.2.5
of ACPI 6. Those additional objects are referred to as data-only
subnodes of the device object containing the _DSD pointing to them.
The links to them need to be located in a separate section of the
_DSD data package following UUID dbb8e3e6-5886-4ba6-8795-1319f52a966b
referred to as the Hierarchical Data Extension UUID as defined in [1].
Each of them is represented by a package of two strings. The first
string in that package (the key) is regarded as the name of the
data-only subnode pointed to by the link. The second string in it
(the target) is expected to hold the ACPI namespace path (possibly
utilizing the usual ACPI namespace search rules) of an ACPI object
evaluating to a data package extending the _DSD.
The device properties initialization code follows those links,
creates a struct acpi_data_node object for each of them to store
the data returned by the ACPI object pointed to by it and processes
those data recursively (which may lead to the creation of more
struct acpi_data_node objects if the returned data package contains
the Hierarchical Data Extension UUID section with more links in it).
All of the struct acpi_data_node objects are present until the the
ACPI device object containing the _DSD with links to them is deleted
and they are deleted along with that object.
[1]: http://www.uefi.org/sites/default/files/resources/_DSD-hierarchical-data-extension-UUID-v1.pdf
Signed-off-by: Rafael J. Wysocki <[email protected]>
Tested-by: Mika Westerberg <[email protected]>
---
drivers/acpi/property.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++-
include/acpi/acpi_bus.h | 9 +++
include/linux/fwnode.h | 1
3 files changed, 142 insertions(+), 1 deletion(-)
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -343,6 +343,7 @@ struct acpi_device_data {
const union acpi_object *pointer;
const union acpi_object *properties;
const union acpi_object *of_compatible;
+ struct list_head subnodes;
};
struct acpi_gpio_mapping;
@@ -378,6 +379,14 @@ struct acpi_device {
void (*remove)(struct acpi_device *);
};
+/* Non-device subnode */
+struct acpi_data_node {
+ const char *name;
+ struct fwnode_handle fwnode;
+ struct acpi_device_data data;
+ struct list_head sibling;
+};
+
static inline bool acpi_check_dma(struct acpi_device *adev, bool *coherent)
{
bool ret = false;
Index: linux-pm/include/linux/fwnode.h
===================================================================
--- linux-pm.orig/include/linux/fwnode.h
+++ linux-pm/include/linux/fwnode.h
@@ -16,6 +16,7 @@ enum fwnode_type {
FWNODE_INVALID = 0,
FWNODE_OF,
FWNODE_ACPI,
+ FWNODE_ACPI_DATA,
FWNODE_PDATA,
};
Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -24,6 +24,115 @@ static const u8 prp_uuid[16] = {
0x14, 0xd8, 0xff, 0xda, 0xba, 0x6e, 0x8c, 0x4d,
0x8a, 0x91, 0xbc, 0x9b, 0xbf, 0x4a, 0xa3, 0x01
};
+/* ACPI _DSD data subnodes UUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
+static const u8 ads_uuid[16] = {
+ 0xe6, 0xe3, 0xb8, 0xdb, 0x86, 0x58, 0xa6, 0x4b,
+ 0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b
+};
+
+static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
+ const union acpi_object *desc,
+ struct acpi_device_data *data);
+static bool acpi_extract_properties(const union acpi_object *desc,
+ struct acpi_device_data *data);
+
+static bool acpi_nondev_subnode_ok(acpi_handle scope,
+ const union acpi_object *link,
+ struct list_head *list)
+{
+ struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
+ struct acpi_data_node *dn;
+ acpi_handle handle;
+ acpi_status status;
+
+ dn = kzalloc(sizeof(*dn), GFP_KERNEL);
+ if (!dn)
+ return false;
+
+ dn->name = link->package.elements[0].string.pointer;
+ dn->fwnode.type = FWNODE_ACPI_DATA;
+ INIT_LIST_HEAD(&dn->data.subnodes);
+
+ status = acpi_get_handle(scope, link->package.elements[1].string.pointer,
+ &handle);
+ if (ACPI_FAILURE(status))
+ goto fail;
+
+ status = acpi_evaluate_object_typed(handle, NULL, NULL, &buf,
+ ACPI_TYPE_PACKAGE);
+ if (ACPI_FAILURE(status))
+ goto fail;
+
+ if (acpi_extract_properties(buf.pointer, &dn->data))
+ dn->data.pointer = buf.pointer;
+
+ if (acpi_enumerate_nondev_subnodes(scope, buf.pointer, &dn->data))
+ dn->data.pointer = buf.pointer;
+
+ if (dn->data.pointer) {
+ list_add_tail(&dn->sibling, list);
+ return true;
+ }
+
+ acpi_handle_debug(handle, "Invalid properties/subnodes data, skipping\n");
+
+ fail:
+ ACPI_FREE(buf.pointer);
+ kfree(dn);
+ return false;
+}
+
+static int acpi_add_nondev_subnodes(acpi_handle scope,
+ const union acpi_object *links,
+ struct list_head *list)
+{
+ bool ret = false;
+ int i;
+
+ for (i = 0; i < links->package.count; i++) {
+ const union acpi_object *link;
+
+ link = &links->package.elements[i];
+ /* Only two elements allowed, both must be strings. */
+ if (link->package.count == 2
+ && link->package.elements[0].type == ACPI_TYPE_STRING
+ && link->package.elements[1].type == ACPI_TYPE_STRING
+ && acpi_nondev_subnode_ok(scope, link, list))
+ ret = true;
+ }
+
+ return ret;
+}
+
+static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
+ const union acpi_object *desc,
+ struct acpi_device_data *data)
+{
+ int i;
+
+ /* Look for the ACPI data subnodes UUID. */
+ for (i = 0; i < desc->package.count; i += 2) {
+ const union acpi_object *uuid, *links;
+
+ uuid = &desc->package.elements[i];
+ links = &desc->package.elements[i + 1];
+
+ /*
+ * The first element must be a UUID and the second one must be
+ * a package.
+ */
+ if (uuid->type != ACPI_TYPE_BUFFER || uuid->buffer.length != 16
+ || links->type != ACPI_TYPE_PACKAGE)
+ break;
+
+ if (memcmp(uuid->buffer.pointer, ads_uuid, sizeof(ads_uuid)))
+ continue;
+
+ return acpi_add_nondev_subnodes(scope, links, &data->subnodes);
+ }
+
+ return false;
+}
static bool acpi_property_value_ok(const union acpi_object *value)
{
@@ -147,6 +256,8 @@ void acpi_init_properties(struct acpi_de
acpi_status status;
bool acpi_of = false;
+ INIT_LIST_HEAD(&adev->data.subnodes);
+
/*
* Check if ACPI_DT_NAMESPACE_HID is present and inthat case we fill in
* Device Tree compatible properties for this device.
@@ -167,7 +278,11 @@ void acpi_init_properties(struct acpi_de
adev->data.pointer = buf.pointer;
if (acpi_of)
acpi_init_of_compatible(adev);
- } else {
+ }
+ if (acpi_enumerate_nondev_subnodes(adev->handle, buf.pointer, &adev->data))
+ adev->data.pointer = buf.pointer;
+
+ if (!adev->data.pointer) {
acpi_handle_debug(adev->handle, "Invalid _DSD data, skipping\n");
ACPI_FREE(buf.pointer);
}
@@ -178,8 +293,24 @@ void acpi_init_properties(struct acpi_de
ACPI_DT_NAMESPACE_HID " requires 'compatible' property\n");
}
+static void acpi_destroy_nondev_subnodes(struct list_head *list)
+{
+ struct acpi_data_node *dn, *next;
+
+ if (list_empty(list))
+ return;
+
+ list_for_each_entry_safe_reverse(dn, next, list, sibling) {
+ acpi_destroy_nondev_subnodes(&dn->data.subnodes);
+ list_del(&dn->sibling);
+ ACPI_FREE((void *)dn->data.pointer);
+ kfree(dn);
+ }
+}
+
void acpi_free_properties(struct acpi_device *adev)
{
+ acpi_destroy_nondev_subnodes(&adev->data.subnodes);
ACPI_FREE((void *)adev->data.pointer);
adev->data.of_compatible = NULL;
adev->data.pointer = NULL;
From: Rafael J. Wysocki <[email protected]>
Add infrastructure needed to expose data-only subnodes of ACPI
device objects introduced previously via sysfs.
Each data-only subnode is represented as a sysfs directory under
the directory corresponding to its parent object (a device or a
data-only subnode). Each of them has a "path" attribute (containing
the full ACPI namespace path to the object the subnode data come from)
at this time.
Signed-off-by: Rafael J. Wysocki <[email protected]>
Tested-by: Mika Westerberg <[email protected]>
---
drivers/acpi/device_sysfs.c | 120 +++++++++++++++++++++++++++++++++++++++-----
drivers/acpi/property.c | 8 +-
include/acpi/acpi_bus.h | 3 +
3 files changed, 116 insertions(+), 15 deletions(-)
Index: linux-pm/drivers/acpi/device_sysfs.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_sysfs.c
+++ linux-pm/drivers/acpi/device_sysfs.c
@@ -26,6 +26,106 @@
#include "internal.h"
+static ssize_t acpi_object_path(acpi_handle handle, char *buf)
+{
+ struct acpi_buffer path = {ACPI_ALLOCATE_BUFFER, NULL};
+ int result;
+
+ result = acpi_get_name(handle, ACPI_FULL_PATHNAME, &path);
+ if (result)
+ return result;
+
+ result = sprintf(buf, "%s\n", (char*)path.pointer);
+ kfree(path.pointer);
+ return result;
+}
+
+struct acpi_data_node_attr {
+ struct attribute attr;
+ ssize_t (*show)(struct acpi_data_node *, char *);
+ ssize_t (*store)(struct acpi_data_node *, const char *, size_t count);
+};
+
+#define DATA_NODE_ATTR(_name) \
+ static struct acpi_data_node_attr data_node_##_name = \
+ __ATTR(_name, 0444, data_node_show_##_name, NULL)
+
+static ssize_t data_node_show_path(struct acpi_data_node *dn, char *buf)
+{
+ return acpi_object_path(dn->handle, buf);
+}
+
+DATA_NODE_ATTR(path);
+
+static struct attribute *acpi_data_node_default_attrs[] = {
+ &data_node_path.attr,
+ NULL
+};
+
+#define to_data_node(k) container_of(k, struct acpi_data_node, kobj)
+#define to_attr(a) container_of(a, struct acpi_data_node_attr, attr)
+
+static ssize_t acpi_data_node_attr_show(struct kobject *kobj,
+ struct attribute *attr, char *buf)
+{
+ struct acpi_data_node *dn = to_data_node(kobj);
+ struct acpi_data_node_attr *dn_attr = to_attr(attr);
+
+ return dn_attr->show ? dn_attr->show(dn, buf) : -ENXIO;
+}
+
+static const struct sysfs_ops acpi_data_node_sysfs_ops = {
+ .show = acpi_data_node_attr_show,
+};
+
+static void acpi_data_node_release(struct kobject *kobj)
+{
+ struct acpi_data_node *dn = to_data_node(kobj);
+ complete(&dn->kobj_done);
+}
+
+static struct kobj_type acpi_data_node_ktype = {
+ .sysfs_ops = &acpi_data_node_sysfs_ops,
+ .default_attrs = acpi_data_node_default_attrs,
+ .release = acpi_data_node_release,
+};
+
+static void acpi_expose_nondev_subnodes(struct kobject *kobj,
+ struct acpi_device_data *data)
+{
+ struct list_head *list = &data->subnodes;
+ struct acpi_data_node *dn;
+
+ if (list_empty(list))
+ return;
+
+ list_for_each_entry(dn, list, sibling) {
+ int ret;
+
+ init_completion(&dn->kobj_done);
+ ret = kobject_init_and_add(&dn->kobj, &acpi_data_node_ktype,
+ kobj, dn->name);
+ if (ret)
+ acpi_handle_err(dn->handle, "Failed to expose (%d)\n", ret);
+ else
+ acpi_expose_nondev_subnodes(&dn->kobj, &dn->data);
+ }
+}
+
+static void acpi_hide_nondev_subnodes(struct acpi_device_data *data)
+{
+ struct list_head *list = &data->subnodes;
+ struct acpi_data_node *dn;
+
+ if (list_empty(list))
+ return;
+
+ list_for_each_entry_reverse(dn, list, sibling) {
+ acpi_hide_nondev_subnodes(&dn->data);
+ kobject_put(&dn->kobj);
+ }
+}
+
/**
* create_pnp_modalias - Create hid/cid(s) string for modalias and uevent
* @acpi_dev: ACPI device object.
@@ -323,20 +423,12 @@ static ssize_t acpi_device_adr_show(stru
}
static DEVICE_ATTR(adr, 0444, acpi_device_adr_show, NULL);
-static ssize_t
-acpi_device_path_show(struct device *dev, struct device_attribute *attr, char *buf) {
+static ssize_t acpi_device_path_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
struct acpi_device *acpi_dev = to_acpi_device(dev);
- struct acpi_buffer path = {ACPI_ALLOCATE_BUFFER, NULL};
- int result;
-
- result = acpi_get_name(acpi_dev->handle, ACPI_FULL_PATHNAME, &path);
- if (result)
- goto end;
- result = sprintf(buf, "%s\n", (char*)path.pointer);
- kfree(path.pointer);
-end:
- return result;
+ return acpi_object_path(acpi_dev->handle, buf);
}
static DEVICE_ATTR(path, 0444, acpi_device_path_show, NULL);
@@ -475,6 +567,8 @@ int acpi_device_setup_files(struct acpi_
&dev_attr_real_power_state);
}
+ acpi_expose_nondev_subnodes(&dev->dev.kobj, &dev->data);
+
end:
return result;
}
@@ -485,6 +579,8 @@ end:
*/
void acpi_device_remove_files(struct acpi_device *dev)
{
+ acpi_hide_nondev_subnodes(&dev->data);
+
if (dev->flags.power_manageable) {
device_remove_file(&dev->dev, &dev_attr_power_state);
if (dev->power.flags.power_resources)
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -382,9 +382,12 @@ struct acpi_device {
/* Non-device subnode */
struct acpi_data_node {
const char *name;
+ acpi_handle handle;
struct fwnode_handle fwnode;
struct acpi_device_data data;
struct list_head sibling;
+ struct kobject kobj;
+ struct completion kobj_done;
};
static inline bool acpi_check_dma(struct acpi_device *adev, bool *coherent)
Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -64,12 +64,13 @@ static bool acpi_nondev_subnode_ok(acpi_
goto fail;
if (acpi_extract_properties(buf.pointer, &dn->data))
- dn->data.pointer = buf.pointer;
+ dn->handle = handle;
if (acpi_enumerate_nondev_subnodes(scope, buf.pointer, &dn->data))
- dn->data.pointer = buf.pointer;
+ dn->handle = handle;
- if (dn->data.pointer) {
+ if (dn->handle) {
+ dn->data.pointer = buf.pointer;
list_add_tail(&dn->sibling, list);
return true;
}
@@ -302,6 +303,7 @@ static void acpi_destroy_nondev_subnodes
list_for_each_entry_safe_reverse(dn, next, list, sibling) {
acpi_destroy_nondev_subnodes(&dn->data.subnodes);
+ wait_for_completion(&dn->kobj_done);
list_del(&dn->sibling);
ACPI_FREE((void *)dn->data.pointer);
kfree(dn);
From: Rafael J. Wysocki <[email protected]>
Modify is_acpi_node() to return "true" for ACPI data-only subnodes as
well as for ACPI device objects and change the name of to_acpi_node()
to to_acpi_device_node() so it is clear that it covers ACPI device
objects only. Accordingly, introduce to_acpi_data_node() to cover
data-only subnodes in an analogous way.
With that, make the fwnode_property_* family of functions work with
ACPI data-only subnodes introduced previously.
Signed-off-by: Rafael J. Wysocki <[email protected]>
Tested-by: Mika Westerberg <[email protected]>
---
drivers/acpi/property.c | 138 ++++++++++++++++++++++++++++++++++++------------
drivers/base/property.c | 16 ++---
drivers/gpio/gpiolib.c | 6 +-
include/acpi/acpi_bus.h | 21 ++++++-
include/linux/acpi.h | 53 +++++++++++++-----
5 files changed, 173 insertions(+), 61 deletions(-)
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -425,15 +425,32 @@ static inline bool acpi_check_dma(struct
static inline bool is_acpi_node(struct fwnode_handle *fwnode)
{
+ return fwnode && (fwnode->type == FWNODE_ACPI
+ || fwnode->type == FWNODE_ACPI_DATA);
+}
+
+static inline bool is_acpi_device_node(struct fwnode_handle *fwnode)
+{
return fwnode && fwnode->type == FWNODE_ACPI;
}
-static inline struct acpi_device *to_acpi_node(struct fwnode_handle *fwnode)
+static inline struct acpi_device *to_acpi_device_node(struct fwnode_handle *fwnode)
{
- return is_acpi_node(fwnode) ?
+ return is_acpi_device_node(fwnode) ?
container_of(fwnode, struct acpi_device, fwnode) : NULL;
}
+static inline bool is_acpi_data_node(struct fwnode_handle *fwnode)
+{
+ return fwnode && fwnode->type == FWNODE_ACPI_DATA;
+}
+
+static inline struct acpi_data_node *to_acpi_data_node(struct fwnode_handle *fwnode)
+{
+ return is_acpi_data_node(fwnode) ?
+ container_of(fwnode, struct acpi_data_node, fwnode) : NULL;
+}
+
static inline struct fwnode_handle *acpi_fwnode_handle(struct acpi_device *adev)
{
return &adev->fwnode;
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -49,7 +49,7 @@ static inline acpi_handle acpi_device_ha
return adev ? adev->handle : NULL;
}
-#define ACPI_COMPANION(dev) to_acpi_node((dev)->fwnode)
+#define ACPI_COMPANION(dev) to_acpi_device_node((dev)->fwnode)
#define ACPI_COMPANION_SET(dev, adev) set_primary_fwnode(dev, (adev) ? \
acpi_fwnode_handle(adev) : NULL)
#define ACPI_HANDLE(dev) acpi_device_handle(ACPI_COMPANION(dev))
@@ -69,7 +69,7 @@ static inline acpi_handle acpi_device_ha
static inline bool has_acpi_companion(struct device *dev)
{
- return is_acpi_node(dev->fwnode);
+ return is_acpi_device_node(dev->fwnode);
}
static inline void acpi_preset_companion(struct device *dev,
@@ -461,7 +461,22 @@ static inline bool is_acpi_node(struct f
return false;
}
-static inline struct acpi_device *to_acpi_node(struct fwnode_handle *fwnode)
+static inline bool is_acpi_device_node(struct fwnode_handle *fwnode)
+{
+ return false;
+}
+
+static inline struct acpi_device *to_acpi_device_node(struct fwnode_handle *fwnode)
+{
+ return NULL;
+}
+
+static inline bool is_acpi_data_node(struct fwnode_handle *fwnode)
+{
+ return false;
+}
+
+static inline struct acpi_data_node *to_acpi_data_node(struct fwnode_handle *fwnode)
{
return NULL;
}
@@ -743,17 +758,16 @@ struct acpi_reference_args {
#ifdef CONFIG_ACPI
int acpi_dev_get_property(struct acpi_device *adev, const char *name,
acpi_object_type type, const union acpi_object **obj);
-int acpi_dev_get_property_array(struct acpi_device *adev, const char *name,
- acpi_object_type type,
- const union acpi_object **obj);
int acpi_dev_get_property_reference(struct acpi_device *adev,
const char *name, size_t index,
struct acpi_reference_args *args);
-int acpi_dev_prop_get(struct acpi_device *adev, const char *propname,
- void **valptr);
+int acpi_node_prop_get(struct fwnode_handle *fwnode, const char *propname,
+ void **valptr);
int acpi_dev_prop_read_single(struct acpi_device *adev, const char *propname,
enum dev_prop_type proptype, void *val);
+int acpi_node_prop_read(struct fwnode_handle *fwnode, const char *propname,
+ enum dev_prop_type proptype, void *val, size_t nval);
int acpi_dev_prop_read(struct acpi_device *adev, const char *propname,
enum dev_prop_type proptype, void *val, size_t nval);
@@ -766,13 +780,7 @@ static inline int acpi_dev_get_property(
{
return -ENXIO;
}
-static inline int acpi_dev_get_property_array(struct acpi_device *adev,
- const char *name,
- acpi_object_type type,
- const union acpi_object **obj)
-{
- return -ENXIO;
-}
+
static inline int acpi_dev_get_property_reference(struct acpi_device *adev,
const char *name, const char *cells_name,
size_t index, struct acpi_reference_args *args)
@@ -780,6 +788,13 @@ static inline int acpi_dev_get_property_
return -ENXIO;
}
+static inline int acpi_node_prop_get(struct fwnode_handle *fwnode,
+ const char *propname,
+ void **valptr)
+{
+ return -ENXIO;
+}
+
static inline int acpi_dev_prop_get(struct acpi_device *adev,
const char *propname,
void **valptr)
@@ -794,6 +809,14 @@ static inline int acpi_dev_prop_read_sin
{
return -ENXIO;
}
+
+static inline int acpi_node_prop_read(struct fwnode_handle *fwnode,
+ const char *propname,
+ enum dev_prop_type proptype,
+ void *val, size_t nval)
+{
+ return -ENXIO;
+}
static inline int acpi_dev_prop_read(struct acpi_device *adev,
const char *propname,
Index: linux-pm/drivers/gpio/gpiolib.c
===================================================================
--- linux-pm.orig/drivers/gpio/gpiolib.c
+++ linux-pm/drivers/gpio/gpiolib.c
@@ -2056,11 +2056,11 @@ struct gpio_desc *fwnode_get_named_gpiod
&flags);
if (!IS_ERR(desc))
active_low = flags & OF_GPIO_ACTIVE_LOW;
- } else if (is_acpi_node(fwnode)) {
+ } else if (is_acpi_device_node(fwnode)) {
struct acpi_gpio_info info;
- desc = acpi_get_gpiod_by_index(to_acpi_node(fwnode), propname, 0,
- &info);
+ desc = acpi_get_gpiod_by_index(to_acpi_device_node(fwnode),
+ propname, 0, &info);
if (!IS_ERR(desc))
active_low = info.active_low;
}
Index: linux-pm/drivers/base/property.c
===================================================================
--- linux-pm.orig/drivers/base/property.c
+++ linux-pm/drivers/base/property.c
@@ -132,7 +132,7 @@ bool fwnode_property_present(struct fwno
if (is_of_node(fwnode))
return of_property_read_bool(to_of_node(fwnode), propname);
else if (is_acpi_node(fwnode))
- return !acpi_dev_prop_get(to_acpi_node(fwnode), propname, NULL);
+ return !acpi_node_prop_get(fwnode, propname, NULL);
return !!pset_prop_get(to_pset(fwnode), propname);
}
@@ -290,8 +290,8 @@ EXPORT_SYMBOL_GPL(device_property_read_s
_ret_ = OF_DEV_PROP_READ_ARRAY(to_of_node(_fwnode_), _propname_, \
_type_, _val_, _nval_); \
else if (is_acpi_node(_fwnode_)) \
- _ret_ = acpi_dev_prop_read(to_acpi_node(_fwnode_), _propname_, \
- _proptype_, _val_, _nval_); \
+ _ret_ = acpi_node_prop_read(_fwnode_, _propname_, _proptype_, \
+ _val_, _nval_); \
else \
_ret_ = pset_prop_read_array(to_pset(_fwnode_), _propname_, \
_proptype_, _val_, _nval_); \
@@ -430,8 +430,8 @@ int fwnode_property_read_string_array(st
propname, val, nval) :
of_property_count_strings(to_of_node(fwnode), propname);
else if (is_acpi_node(fwnode))
- return acpi_dev_prop_read(to_acpi_node(fwnode), propname,
- DEV_PROP_STRING, val, nval);
+ return acpi_node_prop_read(fwnode, propname, DEV_PROP_STRING,
+ val, nval);
return pset_prop_read_array(to_pset(fwnode), propname,
DEV_PROP_STRING, val, nval);
@@ -459,8 +459,8 @@ int fwnode_property_read_string(struct f
if (is_of_node(fwnode))
return of_property_read_string(to_of_node(fwnode), propname, val);
else if (is_acpi_node(fwnode))
- return acpi_dev_prop_read(to_acpi_node(fwnode), propname,
- DEV_PROP_STRING, val, 1);
+ return acpi_node_prop_read(fwnode, propname, DEV_PROP_STRING,
+ val, 1);
return -ENXIO;
}
@@ -483,7 +483,7 @@ struct fwnode_handle *device_get_next_ch
} else if (IS_ENABLED(CONFIG_ACPI)) {
struct acpi_device *node;
- node = acpi_get_next_child(dev, to_acpi_node(child));
+ node = acpi_get_next_child(dev, to_acpi_device_node(child));
if (node)
return acpi_fwnode_handle(node);
}
Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -19,6 +19,11 @@
#include "internal.h"
+static int acpi_data_get_property_array(struct acpi_device_data *data,
+ const char *name,
+ acpi_object_type type,
+ const union acpi_object **obj);
+
/* ACPI _DSD device properties UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
static const u8 prp_uuid[16] = {
0x14, 0xd8, 0xff, 0xda, 0xba, 0x6e, 0x8c, 0x4d,
@@ -191,8 +196,8 @@ static void acpi_init_of_compatible(stru
const union acpi_object *of_compatible;
int ret;
- ret = acpi_dev_get_property_array(adev, "compatible", ACPI_TYPE_STRING,
- &of_compatible);
+ ret = acpi_data_get_property_array(&adev->data, "compatible",
+ ACPI_TYPE_STRING, &of_compatible);
if (ret) {
ret = acpi_dev_get_property(adev, "compatible",
ACPI_TYPE_STRING, &of_compatible);
@@ -320,8 +325,8 @@ void acpi_free_properties(struct acpi_de
}
/**
- * acpi_dev_get_property - return an ACPI property with given name
- * @adev: ACPI device to get property
+ * acpi_data_get_property - return an ACPI property with given name
+ * @data: ACPI device deta object to get the property from
* @name: Name of the property
* @type: Expected property type
* @obj: Location to store the property value (if not %NULL)
@@ -330,26 +335,27 @@ void acpi_free_properties(struct acpi_de
* object at the location pointed to by @obj if found.
*
* Callers must not attempt to free the returned objects. These objects will be
- * freed by the ACPI core automatically during the removal of @adev.
+ * freed by the ACPI core automatically during the removal of @data.
*
* Return: %0 if property with @name has been found (success),
* %-EINVAL if the arguments are invalid,
* %-ENODATA if the property doesn't exist,
* %-EPROTO if the property value type doesn't match @type.
*/
-int acpi_dev_get_property(struct acpi_device *adev, const char *name,
- acpi_object_type type, const union acpi_object **obj)
+static int acpi_data_get_property(struct acpi_device_data *data,
+ const char *name, acpi_object_type type,
+ const union acpi_object **obj)
{
const union acpi_object *properties;
int i;
- if (!adev || !name)
+ if (!data || !name)
return -EINVAL;
- if (!adev->data.pointer || !adev->data.properties)
+ if (!data->pointer || !data->properties)
return -ENODATA;
- properties = adev->data.properties;
+ properties = data->properties;
for (i = 0; i < properties->package.count; i++) {
const union acpi_object *propname, *propvalue;
const union acpi_object *property;
@@ -370,11 +376,50 @@ int acpi_dev_get_property(struct acpi_de
}
return -ENODATA;
}
+
+/**
+ * acpi_dev_get_property - return an ACPI property with given name.
+ * @adev: ACPI device to get the property from.
+ * @name: Name of the property.
+ * @type: Expected property type.
+ * @obj: Location to store the property value (if not %NULL).
+ */
+int acpi_dev_get_property(struct acpi_device *adev, const char *name,
+ acpi_object_type type, const union acpi_object **obj)
+{
+ return adev ? acpi_data_get_property(&adev->data, name, type, obj) : -EINVAL;
+}
EXPORT_SYMBOL_GPL(acpi_dev_get_property);
+static struct acpi_device_data *acpi_device_data_of_node(struct fwnode_handle *fwnode)
+{
+ if (fwnode->type == FWNODE_ACPI) {
+ struct acpi_device *adev = to_acpi_device_node(fwnode);
+ return &adev->data;
+ } else if (fwnode->type == FWNODE_ACPI_DATA) {
+ struct acpi_data_node *dn = to_acpi_data_node(fwnode);
+ return &dn->data;
+ }
+ return NULL;
+}
+
/**
- * acpi_dev_get_property_array - return an ACPI array property with given name
- * @adev: ACPI device to get property
+ * acpi_node_prop_get - return an ACPI property with given name.
+ * @fwnode: Firmware node to get the property from.
+ * @propname: Name of the property.
+ * @valptr: Location to store a pointer to the property value (if not %NULL).
+ */
+int acpi_node_prop_get(struct fwnode_handle *fwnode, const char *propname,
+ void **valptr)
+{
+ return acpi_data_get_property(acpi_device_data_of_node(fwnode),
+ propname, ACPI_TYPE_ANY,
+ (const union acpi_object **)valptr);
+}
+
+/**
+ * acpi_data_get_property_array - return an ACPI array property with given name
+ * @adev: ACPI data object to get the property from
* @name: Name of the property
* @type: Expected type of array elements
* @obj: Location to store a pointer to the property value (if not NULL)
@@ -383,7 +428,7 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_property)
* ACPI object at the location pointed to by @obj if found.
*
* Callers must not attempt to free the returned objects. Those objects will be
- * freed by the ACPI core automatically during the removal of @adev.
+ * freed by the ACPI core automatically during the removal of @data.
*
* Return: %0 if array property (package) with @name has been found (success),
* %-EINVAL if the arguments are invalid,
@@ -391,14 +436,15 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_property)
* %-EPROTO if the property is not a package or the type of its elements
* doesn't match @type.
*/
-int acpi_dev_get_property_array(struct acpi_device *adev, const char *name,
- acpi_object_type type,
- const union acpi_object **obj)
+static int acpi_data_get_property_array(struct acpi_device_data *data,
+ const char *name,
+ acpi_object_type type,
+ const union acpi_object **obj)
{
const union acpi_object *prop;
int ret, i;
- ret = acpi_dev_get_property(adev, name, ACPI_TYPE_PACKAGE, &prop);
+ ret = acpi_data_get_property(data, name, ACPI_TYPE_PACKAGE, &prop);
if (ret)
return ret;
@@ -413,7 +459,6 @@ int acpi_dev_get_property_array(struct a
return 0;
}
-EXPORT_SYMBOL_GPL(acpi_dev_get_property_array);
/**
* acpi_dev_get_property_reference - returns handle to the referenced object
@@ -518,15 +563,9 @@ int acpi_dev_get_property_reference(stru
}
EXPORT_SYMBOL_GPL(acpi_dev_get_property_reference);
-int acpi_dev_prop_get(struct acpi_device *adev, const char *propname,
- void **valptr)
-{
- return acpi_dev_get_property(adev, propname, ACPI_TYPE_ANY,
- (const union acpi_object **)valptr);
-}
-
-int acpi_dev_prop_read_single(struct acpi_device *adev, const char *propname,
- enum dev_prop_type proptype, void *val)
+static int acpi_data_prop_read_single(struct acpi_device_data *data,
+ const char *propname,
+ enum dev_prop_type proptype, void *val)
{
const union acpi_object *obj;
int ret;
@@ -535,7 +574,7 @@ int acpi_dev_prop_read_single(struct acp
return -EINVAL;
if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64) {
- ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_INTEGER, &obj);
+ ret = acpi_data_get_property(data, propname, ACPI_TYPE_INTEGER, &obj);
if (ret)
return ret;
@@ -560,7 +599,7 @@ int acpi_dev_prop_read_single(struct acp
break;
}
} else if (proptype == DEV_PROP_STRING) {
- ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_STRING, &obj);
+ ret = acpi_data_get_property(data, propname, ACPI_TYPE_STRING, &obj);
if (ret)
return ret;
@@ -571,6 +610,12 @@ int acpi_dev_prop_read_single(struct acp
return ret;
}
+int acpi_dev_prop_read_single(struct acpi_device *adev, const char *propname,
+ enum dev_prop_type proptype, void *val)
+{
+ return adev ? acpi_data_prop_read_single(&adev->data, propname, proptype, val) : -EINVAL;
+}
+
static int acpi_copy_property_array_u8(const union acpi_object *items, u8 *val,
size_t nval)
{
@@ -647,20 +692,22 @@ static int acpi_copy_property_array_stri
return 0;
}
-int acpi_dev_prop_read(struct acpi_device *adev, const char *propname,
- enum dev_prop_type proptype, void *val, size_t nval)
+static int acpi_data_prop_read(struct acpi_device_data *data,
+ const char *propname,
+ enum dev_prop_type proptype,
+ void *val, size_t nval)
{
const union acpi_object *obj;
const union acpi_object *items;
int ret;
if (val && nval == 1) {
- ret = acpi_dev_prop_read_single(adev, propname, proptype, val);
+ ret = acpi_data_prop_read_single(data, propname, proptype, val);
if (!ret)
return ret;
}
- ret = acpi_dev_get_property_array(adev, propname, ACPI_TYPE_ANY, &obj);
+ ret = acpi_data_get_property_array(data, propname, ACPI_TYPE_ANY, &obj);
if (ret)
return ret;
@@ -695,3 +742,28 @@ int acpi_dev_prop_read(struct acpi_devic
}
return ret;
}
+
+int acpi_dev_prop_read(struct acpi_device *adev, const char *propname,
+ enum dev_prop_type proptype, void *val, size_t nval)
+{
+ return adev ? acpi_data_prop_read(&adev->data, propname, proptype, val, nval) : -EINVAL;
+}
+
+/**
+ * acpi_node_prop_read - retrieve the value of an ACPI property with given name.
+ * @fwnode: Firmware node to get the property from.
+ * @propname: Name of the property.
+ * @proptype: Expected property type.
+ * @val: Location to store the property value (if not %NULL).
+ * @nval: Size of the array pointed to by @val.
+ *
+ * If @val is %NULL, return the number of array elements comprising the value
+ * of the property. Otherwise, read at most @nval values to the array at the
+ * location pointed to by @val.
+ */
+int acpi_node_prop_read(struct fwnode_handle *fwnode, const char *propname,
+ enum dev_prop_type proptype, void *val, size_t nval)
+{
+ return acpi_data_prop_read(acpi_device_data_of_node(fwnode),
+ propname, proptype, val, nval);
+}
From: Rafael J. Wysocki <[email protected]>
Split acpi_get_gpiod_by_index() into three smaller routines to
allow the subsequent change of the generic firmware node properties
code to be more strarightforward.
Signed-off-by: Rafael J. Wysocki <[email protected]>
Tested-by: Mika Westerberg <[email protected]>
---
drivers/gpio/gpiolib-acpi.c | 113 +++++++++++++++++++++++++-------------------
1 file changed, 65 insertions(+), 48 deletions(-)
Index: linux-pm/drivers/gpio/gpiolib-acpi.c
===================================================================
--- linux-pm.orig/drivers/gpio/gpiolib-acpi.c
+++ linux-pm/drivers/gpio/gpiolib-acpi.c
@@ -389,6 +389,8 @@ struct acpi_gpio_lookup {
struct acpi_gpio_info info;
int index;
int pin_index;
+ bool active_low;
+ struct acpi_device *adev;
struct gpio_desc *desc;
int n;
};
@@ -425,6 +427,59 @@ static int acpi_find_gpio(struct acpi_re
return 1;
}
+static int acpi_gpio_resource_lookup(struct acpi_gpio_lookup *lookup,
+ struct acpi_gpio_info *info)
+{
+ struct list_head res_list;
+ int ret;
+
+ INIT_LIST_HEAD(&res_list);
+
+ ret = acpi_dev_get_resources(lookup->adev, &res_list, acpi_find_gpio,
+ lookup);
+ if (ret < 0)
+ return ret;
+
+ acpi_dev_free_resource_list(&res_list);
+
+ if (!lookup->desc)
+ return -ENOENT;
+
+ if (info) {
+ *info = lookup->info;
+ if (lookup->active_low)
+ info->active_low = lookup->active_low;
+ }
+ return 0;
+}
+
+static int acpi_gpio_property_lookup(struct acpi_device *adev,
+ const char *propname, int index,
+ struct acpi_gpio_lookup *lookup)
+{
+ struct acpi_reference_args args;
+ int ret;
+
+ memset(&args, 0, sizeof(args));
+ ret = acpi_dev_get_property_reference(adev, propname, index, &args);
+ if (ret && !acpi_get_driver_gpio_data(adev, propname, index, &args))
+ return ret;
+
+ /*
+ * The property was found and resolved, so need to lookup the GPIO based
+ * on returned args.
+ */
+ lookup->adev = args.adev;
+ if (args.nargs >= 2) {
+ lookup->index = args.args[0];
+ lookup->pin_index = args.args[1];
+ /* 3rd argument, if present is used to specify active_low. */
+ if (args.nargs >= 3)
+ lookup->active_low = !!args.args[2];
+ }
+ return 0;
+}
+
/**
* acpi_get_gpiod_by_index() - get a GPIO descriptor from device resources
* @adev: pointer to a ACPI device to get GPIO from
@@ -452,8 +507,6 @@ struct gpio_desc *acpi_get_gpiod_by_inde
struct acpi_gpio_info *info)
{
struct acpi_gpio_lookup lookup;
- struct list_head resource_list;
- bool active_low = false;
int ret;
if (!adev)
@@ -463,58 +516,22 @@ struct gpio_desc *acpi_get_gpiod_by_inde
lookup.index = index;
if (propname) {
- struct acpi_reference_args args;
-
dev_dbg(&adev->dev, "GPIO: looking up %s\n", propname);
- memset(&args, 0, sizeof(args));
- ret = acpi_dev_get_property_reference(adev, propname,
- index, &args);
- if (ret) {
- bool found = acpi_get_driver_gpio_data(adev, propname,
- index, &args);
- if (!found)
- return ERR_PTR(ret);
- }
-
- /*
- * The property was found and resolved so need to
- * lookup the GPIO based on returned args instead.
- */
- adev = args.adev;
- if (args.nargs >= 2) {
- lookup.index = args.args[0];
- lookup.pin_index = args.args[1];
- /*
- * 3rd argument, if present is used to
- * specify active_low.
- */
- if (args.nargs >= 3)
- active_low = !!args.args[2];
- }
-
- dev_dbg(&adev->dev, "GPIO: _DSD returned %s %zd %llu %llu %llu\n",
- dev_name(&adev->dev), args.nargs,
- args.args[0], args.args[1], args.args[2]);
+ ret = acpi_gpio_property_lookup(adev, propname, index, &lookup);
+ if (ret)
+ return ERR_PTR(ret);
+
+ dev_dbg(&adev->dev, "GPIO: _DSD returned %s %d %d %u\n",
+ dev_name(&lookup.adev->dev), lookup.index,
+ lookup.pin_index, lookup.active_low);
} else {
dev_dbg(&adev->dev, "GPIO: looking up %d in _CRS\n", index);
+ lookup.adev = adev;
}
- INIT_LIST_HEAD(&resource_list);
- ret = acpi_dev_get_resources(adev, &resource_list, acpi_find_gpio,
- &lookup);
- if (ret < 0)
- return ERR_PTR(ret);
-
- acpi_dev_free_resource_list(&resource_list);
-
- if (lookup.desc && info) {
- *info = lookup.info;
- if (active_low)
- info->active_low = active_low;
- }
-
- return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
+ ret = acpi_gpio_resource_lookup(&lookup, info);
+ return ret ? ERR_PTR(ret) : lookup.desc;
}
/**
From: Rafael J. Wysocki <[email protected]>
Make device_get_next_child_node() work with ACPI data-only subnodes
introduced previously.
Namely, replace acpi_get_next_child() with acpi_get_next_subnode()
that can handle (and return) child device objects as well as child
data-only subnodes of the given device and modify the ACPI part
of the GPIO subsystem to handle data-only subnodes returned by it.
To that end, introduce acpi_node_get_gpiod() taking a struct
fwnode_handle pointer as the first argument. That argument may
point to an ACPI device object as well as to a data-only subnode
and the function should do the right thing (ie. look for the matching
GPIO descriptor correctly) in either case.
Next, modify fwnode_get_named_gpiod() to use acpi_node_get_gpiod()
instead of acpi_get_gpiod_by_index() which automatically causes
devm_get_gpiod_from_child() to work with ACPI data-only subnodes
that may be returned by device_get_next_child_node() which in turn
is required by the users of that function (the gpio_keys_polled
and gpio-leds drivers).
Signed-off-by: Rafael J. Wysocki <[email protected]>
Tested-by: Mika Westerberg <[email protected]>
---
drivers/acpi/property.c | 88 ++++++++++++++++++++++++++++++++++++++++----
drivers/acpi/scan.c | 20 ----------
drivers/base/property.c | 6 ---
drivers/gpio/gpiolib-acpi.c | 58 ++++++++++++++++++++++++++---
drivers/gpio/gpiolib.c | 5 +-
drivers/gpio/gpiolib.h | 10 ++++-
include/linux/acpi.h | 17 ++++----
7 files changed, 153 insertions(+), 51 deletions(-)
Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -461,9 +461,9 @@ static int acpi_data_get_property_array(
}
/**
- * acpi_dev_get_property_reference - returns handle to the referenced object
- * @adev: ACPI device to get property
- * @name: Name of the property
+ * acpi_data_get_property_reference - returns handle to the referenced object
+ * @data: ACPI device data object containing the property
+ * @propname: Name of the property
* @index: Index of the reference to return
* @args: Location to store the returned reference with optional arguments
*
@@ -477,16 +477,16 @@ static int acpi_data_get_property_array(
*
* Return: %0 on success, negative error code on failure.
*/
-int acpi_dev_get_property_reference(struct acpi_device *adev,
- const char *name, size_t index,
- struct acpi_reference_args *args)
+static int acpi_data_get_property_reference(struct acpi_device_data *data,
+ const char *propname, size_t index,
+ struct acpi_reference_args *args)
{
const union acpi_object *element, *end;
const union acpi_object *obj;
struct acpi_device *device;
int ret, idx = 0;
- ret = acpi_dev_get_property(adev, name, ACPI_TYPE_ANY, &obj);
+ ret = acpi_data_get_property(data, propname, ACPI_TYPE_ANY, &obj);
if (ret)
return ret;
@@ -561,7 +561,23 @@ int acpi_dev_get_property_reference(stru
return -EPROTO;
}
-EXPORT_SYMBOL_GPL(acpi_dev_get_property_reference);
+
+/**
+ * acpi_node_get_property_reference - get a handle to the referenced object.
+ * @fwnode: Firmware node to get the property from.
+ * @propname: Name of the property.
+ * @index: Index of the reference to return.
+ * @args: Location to store the returned reference with optional arguments.
+ */
+int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
+ const char *name, size_t index,
+ struct acpi_reference_args *args)
+{
+ struct acpi_device_data *data = acpi_device_data_of_node(fwnode);
+
+ return data ? acpi_data_get_property_reference(data, name, index, args) : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(acpi_node_get_property_reference);
static int acpi_data_prop_read_single(struct acpi_device_data *data,
const char *propname,
@@ -767,3 +783,59 @@ int acpi_node_prop_read(struct fwnode_ha
return acpi_data_prop_read(acpi_device_data_of_node(fwnode),
propname, proptype, val, nval);
}
+
+/**
+ * acpi_get_next_subnode - Return the next child node handle for a device.
+ * @dev: Device to find the next child node for.
+ * @child: Handle to one of the device's child nodes or a null handle.
+ */
+struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
+ struct fwnode_handle *child)
+{
+ struct acpi_device *adev = ACPI_COMPANION(dev);
+ struct list_head *head, *next;
+
+ if (!adev)
+ return NULL;
+
+ if (!child || child->type == FWNODE_ACPI) {
+ head = &adev->children;
+ if (list_empty(head))
+ goto nondev;
+
+ if (child) {
+ adev = to_acpi_device_node(child);
+ next = adev->node.next;
+ if (next == head) {
+ child = NULL;
+ goto nondev;
+ }
+ adev = list_entry(next, struct acpi_device, node);
+ } else {
+ adev = list_first_entry(head, struct acpi_device, node);
+ }
+ return acpi_fwnode_handle(adev);
+ }
+
+ nondev:
+ if (!child || child->type == FWNODE_ACPI_DATA) {
+ struct acpi_data_node *dn;
+
+ head = &adev->data.subnodes;
+ if (list_empty(head))
+ return NULL;
+
+ if (child) {
+ dn = to_acpi_data_node(child);
+ next = dn->sibling.next;
+ if (next == head)
+ return NULL;
+
+ dn = list_entry(next, struct acpi_data_node, sibling);
+ } else {
+ dn = list_first_entry(head, struct acpi_data_node, sibling);
+ }
+ return &dn->fwnode;
+ }
+ return NULL;
+}
Index: linux-pm/drivers/base/property.c
===================================================================
--- linux-pm.orig/drivers/base/property.c
+++ linux-pm/drivers/base/property.c
@@ -481,11 +481,7 @@ struct fwnode_handle *device_get_next_ch
if (node)
return &node->fwnode;
} else if (IS_ENABLED(CONFIG_ACPI)) {
- struct acpi_device *node;
-
- node = acpi_get_next_child(dev, to_acpi_device_node(child));
- if (node)
- return acpi_fwnode_handle(node);
+ return acpi_get_next_subnode(dev, child);
}
return NULL;
}
Index: linux-pm/include/linux/acpi.h
===================================================================
--- linux-pm.orig/include/linux/acpi.h
+++ linux-pm/include/linux/acpi.h
@@ -758,9 +758,9 @@ struct acpi_reference_args {
#ifdef CONFIG_ACPI
int acpi_dev_get_property(struct acpi_device *adev, const char *name,
acpi_object_type type, const union acpi_object **obj);
-int acpi_dev_get_property_reference(struct acpi_device *adev,
- const char *name, size_t index,
- struct acpi_reference_args *args);
+int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
+ const char *name, size_t index,
+ struct acpi_reference_args *args);
int acpi_node_prop_get(struct fwnode_handle *fwnode, const char *propname,
void **valptr);
@@ -771,8 +771,8 @@ int acpi_node_prop_read(struct fwnode_ha
int acpi_dev_prop_read(struct acpi_device *adev, const char *propname,
enum dev_prop_type proptype, void *val, size_t nval);
-struct acpi_device *acpi_get_next_child(struct device *dev,
- struct acpi_device *child);
+struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
+ struct fwnode_handle *subnode);
#else
static inline int acpi_dev_get_property(struct acpi_device *adev,
const char *name, acpi_object_type type,
@@ -781,7 +781,7 @@ static inline int acpi_dev_get_property(
return -ENXIO;
}
-static inline int acpi_dev_get_property_reference(struct acpi_device *adev,
+static inline int acpi_node_get_property_reference(struct fwnode_handle *fwnode,
const char *name, const char *cells_name,
size_t index, struct acpi_reference_args *args)
{
@@ -826,12 +826,11 @@ static inline int acpi_dev_prop_read(str
return -ENXIO;
}
-static inline struct acpi_device *acpi_get_next_child(struct device *dev,
- struct acpi_device *child)
+static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
+ struct fwnode_handle *subnode)
{
return NULL;
}
-
#endif
#endif /*_LINUX_ACPI_H*/
Index: linux-pm/drivers/gpio/gpiolib.c
===================================================================
--- linux-pm.orig/drivers/gpio/gpiolib.c
+++ linux-pm/drivers/gpio/gpiolib.c
@@ -2056,11 +2056,10 @@ struct gpio_desc *fwnode_get_named_gpiod
&flags);
if (!IS_ERR(desc))
active_low = flags & OF_GPIO_ACTIVE_LOW;
- } else if (is_acpi_device_node(fwnode)) {
+ } else if (is_acpi_node(fwnode)) {
struct acpi_gpio_info info;
- desc = acpi_get_gpiod_by_index(to_acpi_device_node(fwnode),
- propname, 0, &info);
+ desc = acpi_node_get_gpiod(fwnode, propname, 0, &info);
if (!IS_ERR(desc))
active_low = info.active_low;
}
Index: linux-pm/drivers/gpio/gpiolib-acpi.c
===================================================================
--- linux-pm.orig/drivers/gpio/gpiolib-acpi.c
+++ linux-pm/drivers/gpio/gpiolib-acpi.c
@@ -453,7 +453,7 @@ static int acpi_gpio_resource_lookup(str
return 0;
}
-static int acpi_gpio_property_lookup(struct acpi_device *adev,
+static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
const char *propname, int index,
struct acpi_gpio_lookup *lookup)
{
@@ -461,10 +461,16 @@ static int acpi_gpio_property_lookup(str
int ret;
memset(&args, 0, sizeof(args));
- ret = acpi_dev_get_property_reference(adev, propname, index, &args);
- if (ret && !acpi_get_driver_gpio_data(adev, propname, index, &args))
- return ret;
+ ret = acpi_node_get_property_reference(fwnode, propname, index, &args);
+ if (ret) {
+ struct acpi_device *adev = to_acpi_device_node(fwnode);
+ if (!adev)
+ return ret;
+
+ if (!acpi_get_driver_gpio_data(adev, propname, index, &args))
+ return ret;
+ }
/*
* The property was found and resolved, so need to lookup the GPIO based
* on returned args.
@@ -518,7 +524,8 @@ struct gpio_desc *acpi_get_gpiod_by_inde
if (propname) {
dev_dbg(&adev->dev, "GPIO: looking up %s\n", propname);
- ret = acpi_gpio_property_lookup(adev, propname, index, &lookup);
+ ret = acpi_gpio_property_lookup(acpi_fwnode_handle(adev),
+ propname, index, &lookup);
if (ret)
return ERR_PTR(ret);
@@ -532,6 +539,47 @@ struct gpio_desc *acpi_get_gpiod_by_inde
ret = acpi_gpio_resource_lookup(&lookup, info);
return ret ? ERR_PTR(ret) : lookup.desc;
+}
+
+/**
+ * acpi_node_get_gpiod() - get a GPIO descriptor from ACPI resources
+ * @fwnode: pointer to an ACPI firmware node to get the GPIO information from
+ * @propname: Property name of the GPIO
+ * @index: index of GpioIo/GpioInt resource (starting from %0)
+ * @info: info pointer to fill in (optional)
+ *
+ * If @fwnode is an ACPI device object, call %acpi_get_gpiod_by_index() for it.
+ * Otherwise (ie. it is a data-only non-device object), use the property-based
+ * GPIO lookup to get to the GPIO resource with the relevant information and use
+ * that to obtain the GPIO descriptor to return.
+ */
+struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
+ const char *propname, int index,
+ struct acpi_gpio_info *info)
+{
+ struct acpi_gpio_lookup lookup;
+ struct acpi_device *adev;
+ int ret;
+
+ adev = to_acpi_device_node(fwnode);
+ if (adev)
+ return acpi_get_gpiod_by_index(adev, propname, index, info);
+
+ if (!is_acpi_data_node(fwnode))
+ return ERR_PTR(-ENODEV);
+
+ if (!propname)
+ return ERR_PTR(-EINVAL);
+
+ memset(&lookup, 0, sizeof(lookup));
+ lookup.index = index;
+
+ ret = acpi_gpio_property_lookup(fwnode, propname, index, &lookup);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = acpi_gpio_resource_lookup(&lookup, info);
+ return ret ? ERR_PTR(ret) : lookup.desc;
}
/**
Index: linux-pm/drivers/gpio/gpiolib.h
===================================================================
--- linux-pm.orig/drivers/gpio/gpiolib.h
+++ linux-pm/drivers/gpio/gpiolib.h
@@ -42,6 +42,9 @@ void acpi_gpiochip_free_interrupts(struc
struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
const char *propname, int index,
struct acpi_gpio_info *info);
+struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
+ const char *propname, int index,
+ struct acpi_gpio_info *info);
int acpi_gpio_count(struct device *dev, const char *con_id);
#else
@@ -60,7 +63,12 @@ acpi_get_gpiod_by_index(struct acpi_devi
{
return ERR_PTR(-ENOSYS);
}
-
+static inline struct gpio_desc *
+acpi_node_get_gpiod(struct fwnode_handle *fwnode, const char *propname,
+ int index, struct acpi_gpio_info *info)
+{
+ return ERR_PTR(-ENXIO);
+}
static inline int acpi_gpio_count(struct device *dev, const char *con_id)
{
return -ENODEV;
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -695,26 +695,6 @@ int acpi_device_add(struct acpi_device *
return result;
}
-struct acpi_device *acpi_get_next_child(struct device *dev,
- struct acpi_device *child)
-{
- struct acpi_device *adev = ACPI_COMPANION(dev);
- struct list_head *head, *next;
-
- if (!adev)
- return NULL;
-
- head = &adev->children;
- if (list_empty(head))
- return NULL;
-
- if (!child)
- return list_first_entry(head, struct acpi_device, node);
-
- next = child->node.next;
- return next == head ? NULL : list_entry(next, struct acpi_device, node);
-}
-
/* --------------------------------------------------------------------------
Device Enumeration
-------------------------------------------------------------------------- */
On Thu Aug 27 04:34, Rafael J. Wysocki wrote:
Hi Rafael,
> The issue at hand is that we need to be able to support hierarchical device
> properties in ACPI, like when the set of properties of an entity called "Fred"
> may include a subset called "dog" containing the properties of the Fred's dog
> rather than those of Fred himself. And it may make sense to have the same
> property, like "hair color", for both Fred and the dog, but with different
> values.
OK. I have a couple of questions.
> We (I, Darren and Dave at least) have explored many possible ways to deal with
> that in ACPI, but the majority of them turned out to be unattractive for various
> reasons. Our first take was to use ACPI device objects to make the "child"
> property sets available via _DSD, but that approach is generally incompatible
> with the PnP Manager in Windows following the notion that all device objects
> in ACPI tables are supposed to represent real devices. It can still be made
> work by adding _STA that returns 0 to those "property-only" device objects,
> but that leads to complications in other places and is error prone (if the _STA
> is forgotten, for example). Moreover, it adds quite a bit of overhead even in
> Linux (an ACPICA representation, struct acpi_device, driver core mechanics etc)
> for things that are only supposed to represent sets of device properties. And,
> in addition to that, we'd need to figure out how to give those things arbitrary
> names in a consistent way. All of that caused us to drop the approach based on
> device objects and look for other options.
What's the overhead/effect on Windows for an ACPI object without a _HID (_ADR
only)? That seems like a case where the OS shouldn't be expecting to load
another driver for the ACPI object and the _ADR gives each node a unique name
(even if it is an unfriendly integer)
> The idea is that _DSD may return a package containing the properties of the
> device it belongs to along with a directory of objects that need to be evaluated
> in order to obtain the "child" property sets of it. That directory needs to be
> present in a separate section of the _DSD package (after the new UUID defined in
> the above document) and is a list of two-element sub-packages (entries) where
> the first element is the name of the entry (the name of the "child" property set
> represented by it) and the second element is a "pointer" (essentially, a path
> or "namestring" in the ACPI terminology) to the control method or a static
> data package that needs to be evaluated to obtain the entry's data. The data
> returned by it is then interpreted in the same way as a _DSD return package,
> so it may also contain properties and a directory of its own "child" property
> sets.
Do you expect there to be cases where using an ACPI device object is more
desirable than hierarchical properties? Or is it just impossible given the PNP
manager in Windows?
The best example I can think of is perhaps a multi function device where each
sub-function really does look like a separate device and you probably want to
reference that sub-device, as a device, in other ASL code.
Stating the above more generally, by taking this approach you loose the ability
to reference these child nodes as a device object. In this LED example, I
think it would be nice to set the "trigger" for the led by adding a reference
to the LED from another device (such as a NIC).
Device (NIC0)
{
...
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () {"activity-led", LEDS.LEDM },
Package () {"link-led", LEDS.LEDH },
},
}
}
I'm not sure that's even supported in devicetree or LEDs are the best example
of this, but the pattern seems generally useful.
Without a device you're also forced to use a "label" property instead of a
_STR.
> As an example, consider the following ASL from an experimental MinnowBoard
> firmware:
>
> Device (LEDS)
> {
> Name (_HID, "PRP0001")
>
> Name (_CRS, ResourceTemplate () {
> GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {10}
> GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {11}
> })
>
> Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"compatible", Package () {"gpio-leds"}},
> },
> // Data extension
> ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> Package () {
> Package () {"heartbeat", "LEDH"},
> Package () {"mmc-activity", "LEDM"},
I guess LEDH and LEDM have to be strings here. It would be nice if the
compiler could verify the path resolves. I suppose it's more incentive to keep
these in the same scope.
> Name (LEDH, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"label", "Heartbeat"},
> Package () {"gpios", Package () {^LEDS, 0, 0, 0}},
> Package () {"linux,default-trigger", "heartbeat"},
> Package () {"linux,default-state", "off"},
> Package () {"linux,retain-state-suspended", 1},
> }
> })
>
> Name (LEDM, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"label", Package () {"MMC0 Activity"}},
> Package () {"gpios", Package () {^LEDS, 1, 0, 0}},
> Package () {"linux,default-trigger", Package () {"mmc0"}},
> Package () {"linux,default-state", "off"},
> Package () {"linux,retain-state-suspended", 1},
> }
> })
> }
I suspect you've thought of all of this. Thanks in advance for any
explanations.
--Dustin
Hi,
On Fri, Aug 28, 2015 at 2:10 AM, Dustin Byford
<[email protected]> wrote:
> On Thu Aug 27 04:34, Rafael J. Wysocki wrote:
>
> Hi Rafael,
>
>> The issue at hand is that we need to be able to support hierarchical device
>> properties in ACPI, like when the set of properties of an entity called "Fred"
>> may include a subset called "dog" containing the properties of the Fred's dog
>> rather than those of Fred himself. And it may make sense to have the same
>> property, like "hair color", for both Fred and the dog, but with different
>> values.
>
> OK. I have a couple of questions.
>
>> We (I, Darren and Dave at least) have explored many possible ways to deal with
>> that in ACPI, but the majority of them turned out to be unattractive for various
>> reasons. Our first take was to use ACPI device objects to make the "child"
>> property sets available via _DSD, but that approach is generally incompatible
>> with the PnP Manager in Windows following the notion that all device objects
>> in ACPI tables are supposed to represent real devices. It can still be made
>> work by adding _STA that returns 0 to those "property-only" device objects,
>> but that leads to complications in other places and is error prone (if the _STA
>> is forgotten, for example). Moreover, it adds quite a bit of overhead even in
>> Linux (an ACPICA representation, struct acpi_device, driver core mechanics etc)
>> for things that are only supposed to represent sets of device properties. And,
>> in addition to that, we'd need to figure out how to give those things arbitrary
>> names in a consistent way. All of that caused us to drop the approach based on
>> device objects and look for other options.
>
> What's the overhead/effect on Windows for an ACPI object without a _HID (_ADR
> only)? That seems like a case where the OS shouldn't be expecting to load
> another driver for the ACPI object and the _ADR gives each node a unique name
> (even if it is an unfriendly integer)
I'm not sure about the driver thing in Windows in the case of _ADR
devices to be honest. That said, by the spec you're only supposed to
use _ADR with things listed in Table 6-168 (page 278) as of ACPI 6.
So that already is a stretch to use _ADR for something not listed
there.
The fact that this is an integer rather than a name is slightly
problematic too, because it does not fit into some use cases we
already have drivers for. In some cases the "child" entities are
referred to by names, because things have been developed with DT in
mind. We generally would like to avoid having to provide a separate
ACPI code path at least in some of those cases if possible.
As for the overhead, if something is a device object, it already has a
scope associated with it in the ACPI namespace which adds complexity.
Further, we create a struct acpi_device for it, which is based on
struct device and registered with the driver core etc. All of those
things are not small objects so they take up quite a bit of memory.
Not to mention sysfs interfaces created for them and so on. They also
are taken into account by things like acpi_walk_namespace() and
generally cause those operations to be slightly slower. The are taken
into account by the system suspend/resume code too.
>> The idea is that _DSD may return a package containing the properties of the
>> device it belongs to along with a directory of objects that need to be evaluated
>> in order to obtain the "child" property sets of it. That directory needs to be
>> present in a separate section of the _DSD package (after the new UUID defined in
>> the above document) and is a list of two-element sub-packages (entries) where
>> the first element is the name of the entry (the name of the "child" property set
>> represented by it) and the second element is a "pointer" (essentially, a path
>> or "namestring" in the ACPI terminology) to the control method or a static
>> data package that needs to be evaluated to obtain the entry's data. The data
>> returned by it is then interpreted in the same way as a _DSD return package,
>> so it may also contain properties and a directory of its own "child" property
>> sets.
>
> Do you expect there to be cases where using an ACPI device object is more
> desirable than hierarchical properties? Or is it just impossible given the PNP
> manager in Windows?
>
> The best example I can think of is perhaps a multi function device where each
> sub-function really does look like a separate device and you probably want to
> reference that sub-device, as a device, in other ASL code.
In those cases you may need to use device objects, but you can still
do that just fine. It is just more heavy-weight and may be
problematic for other OSes at least in principle, so by doing that you
may end up with Linux-specific firmware.
The new mechanism introduced here is for the situations in which you
don't have to do that and by using it you can avoid having to worry
about things like the Windows PnP manager entirely.
> Stating the above more generally, by taking this approach you loose the ability
> to reference these child nodes as a device object. In this LED example, I
> think it would be nice to set the "trigger" for the led by adding a reference
> to the LED from another device (such as a NIC).
>
> Device (NIC0)
> {
> ...
> Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"activity-led", LEDS.LEDM },
> Package () {"link-led", LEDS.LEDH },
> },
> }
> }
>
> I'm not sure that's even supported in devicetree or LEDs are the best example
> of this, but the pattern seems generally useful.
I don't see why you can't do that (except that the paths need to be
represented as strings and they are relative to the current scope by
default, so here they need to be absolute or use the "one level up"
operator).
> Without a device you're also forced to use a "label" property instead of a _STR.
Well, you are.
Why exactly does that matter?
>> As an example, consider the following ASL from an experimental MinnowBoard
>> firmware:
>>
>> Device (LEDS)
>> {
>> Name (_HID, "PRP0001")
>>
>> Name (_CRS, ResourceTemplate () {
>> GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
>> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {10}
>> GpioIo (Exclusive, PullDown, 0, 0, IoRestrictionOutputOnly,
>> "\\_SB.PCI0.LPC", 0, ResourceConsumer) {11}
>> })
>>
>> Name (_DSD, Package () {
>> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> Package () {
>> Package () {"compatible", Package () {"gpio-leds"}},
>> },
>> // Data extension
>> ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>> Package () {
>> Package () {"heartbeat", "LEDH"},
>> Package () {"mmc-activity", "LEDM"},
>
> I guess LEDH and LEDM have to be strings here.
Yes, they have to be strings.
We attempted to use references for that, but those are replaced with
the target objects in some cases when the package is being created.
> It would be nice if the compiler could verify the path resolves.
That only is possible at run time when the namespace has been created
(think about stuff coming from different SSDTs, for example).
> I suppose it's more incentive to keep these in the same scope.
That is recommended even. :-)
>> Name (LEDH, Package () {
>> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> Package () {
>> Package () {"label", "Heartbeat"},
>> Package () {"gpios", Package () {^LEDS, 0, 0, 0}},
>> Package () {"linux,default-trigger", "heartbeat"},
>> Package () {"linux,default-state", "off"},
>> Package () {"linux,retain-state-suspended", 1},
>> }
>> })
>>
>> Name (LEDM, Package () {
>> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> Package () {
>> Package () {"label", Package () {"MMC0 Activity"}},
>> Package () {"gpios", Package () {^LEDS, 1, 0, 0}},
>> Package () {"linux,default-trigger", Package () {"mmc0"}},
>> Package () {"linux,default-state", "off"},
>> Package () {"linux,retain-state-suspended", 1},
>> }
>> })
>> }
>
> I suspect you've thought of all of this. Thanks in advance for any explanations.
No problem at all. Please let me know if the above is sufficient.
Thanks,
Rafael