[PATCH v4 00/16]

[PATCH v4 00/16]

[Sakari Ailus]

Hello everyone,

I posted a previous RFC labelled set of ACPI graph support a while ago: 

<URL:http://www.spinics.net/lists/linux-acpi/msg69547.html> 

Since then, the matter of how the properties should be used as in ACPI
_DSD was discussed in Ksummit and LPC, and a document detailing the rules 
was written [1].

I've additionally posted v1, v2 and v3 which can be found here:

<URL:http://www.spinics.net/lists/linux-acpi/msg71661.html>
<URL:http://www.spinics.net/lists/linux-acpi/msg71809.html>
<URL:http://www.spinics.net/lists/linux-acpi/msg72171.html>

This set contains patches written by Mika Westerberg and by myself. The 
patchset brings support for graphs to ACPI. The functionality achieved by 
these patches is very similar to what the Device tree provides: the port
and the endpoint concept are being employed. The patches make use of the
_DSD property and data extensions to achieve this. The fwnode interface is
extended by graph functionality; this way graph information originating 
from both OF and ACPI may be accessed using the same interface, without 
being aware of the underlying firmware interface. 

The last patch of the set contains ASL documentation including an example.

The entire set may also be found here (on mediatree.git master, but it
also applies cleanly on linux-next):

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=acpi-graph> 

The resulting fwnode graph interface has been tested using V4L2 async with
fwnode matching and smiapp and omap3isp drivers, with appropriate changes 
to make use of the fwnode interface in drivers.

I'm additionally moving the firmware implementation specific code from
drivers/base/property.c to firmware specific locations. I'll post that
patchset soon. It'll be available here:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=acpi-graph-cleaned>

The V4L2 patches can be found here. The fwnode graph interface is used by 
the newly added V4L2 fwnode framework which replaces the V4L2 OF
framework, with equivalent functionality. 

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=v4l2-acpi>

changes since v3:

- Rebase on current PM tree including 4.11-rc1 merge --- there were a few
  conflicts.

changes since v2.2:

- Drop device_fwnode_handle() function in favour of moving the existing
  dev_fwnode() function from drivers/base/property.c to linux/property.h
  (now patch 12).

- Unmerge two unrelated patches accidentally merged between RFC v1 and
  PATCH v1 of the series. The patch adding device_fwnode_handle() was
  accidentally merged with "device property: Obtain device's fwnode
  independently of FW type".

- Remove redundant forward declaration of struct device in
  linux/property.h.

changes since v2:

- Include linux/property.h for property fwnode API in additional drivers
  (current patch 11).

changes since v1: 

- Fix a few checkpatch.pl warnings in Mika's patches (too long lines),

- remove the "endpoint" property specifying the endpoint id. The endpoint 
id is a software concept and the index in the endpoint array can be used
instead if needed. The changes are in patches "device property: Add 
support for fwnode endpoints" and "ACPI / DSD: Document references, 
ports and endpoints" and

- add patch "irqchip/gic: Add missing forward declaration for 
struct device" (patch 9) to fix compilation warning on arm64 caused 
by "of: No need to include linux/property.h, linux/fwnode.h is
sufficient" (now patch 10)

changes since RFC v1: 

- Rebased the set --- there were a few conflicts. 

- Fixed a bug in ACPI graph parsing. (Thanks to Mika!)

- Remove one layer (the "ports" node) of the _DSD hierarchical data 
structure. Change the documentation accordingly. Instead, rely on the 
presence of "port" and "endpoint" properties to identify port and 
endpoint nodes. 

- Add a reference the DSD property rule document [1].

-- 
Kind regards,
Sakari


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 01/16] ACPI / property: Add possiblity to retrieve parent firmware node

[Sakari Ailus]

From: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Sometimes it is useful to be able to navigate firmware node hierarchy
upwards toward parent nodes. ACPI device nodes are pretty much already
supported because ACPICA provides acpi_get_parent(). ACPI data nodes,
however, are all below the same parent ACPI device. Their hierarchy is
created by "linking" each other using references in the value field.

Add parent pointer to the parent data node while we create them so it is
easy to navigate the hierarchy backwards. We use this parent pointer in a
new function acpi_node_get_parent() that is able to extract parent of both
ACPI firmware node types.

Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/acpi/property.c | 72 ++++++++++++++++++++++++++++++++++++++-----------
 include/acpi/acpi_bus.h |  1 +
 include/linux/acpi.h    |  7 +++++
 3 files changed, 65 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 3afddcd..587c9d0 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -37,14 +37,16 @@ static const u8 ads_uuid[16] = {
 
 static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
 					   const union acpi_object *desc,
-					   struct acpi_device_data *data);
+					   struct acpi_device_data *data,
+					   struct fwnode_handle *parent);
 static bool acpi_extract_properties(const union acpi_object *desc,
 				    struct acpi_device_data *data);
 
 static bool acpi_nondev_subnode_extract(const union acpi_object *desc,
 					acpi_handle handle,
 					const union acpi_object *link,
-					struct list_head *list)
+					struct list_head *list,
+					struct fwnode_handle *parent)
 {
 	struct acpi_data_node *dn;
 	bool result;
@@ -55,6 +57,7 @@ static bool acpi_nondev_subnode_extract(const union acpi_object *desc,
 
 	dn->name = link->package.elements[0].string.pointer;
 	dn->fwnode.type = FWNODE_ACPI_DATA;
+	dn->parent = parent;
 	INIT_LIST_HEAD(&dn->data.subnodes);
 
 	result = acpi_extract_properties(desc, &dn->data);
@@ -71,9 +74,11 @@ static bool acpi_nondev_subnode_extract(const union acpi_object *desc,
 		 */
 		status = acpi_get_parent(handle, &scope);
 		if (ACPI_SUCCESS(status)
-		    && acpi_enumerate_nondev_subnodes(scope, desc, &dn->data))
+		    && acpi_enumerate_nondev_subnodes(scope, desc, &dn->data,
+						      &dn->fwnode))
 			result = true;
-	} else if (acpi_enumerate_nondev_subnodes(NULL, desc, &dn->data)) {
+	} else if (acpi_enumerate_nondev_subnodes(NULL, desc, &dn->data,
+						  &dn->fwnode)) {
 		result = true;
 	}
 
@@ -91,7 +96,8 @@ static bool acpi_nondev_subnode_extract(const union acpi_object *desc,
 
 static bool acpi_nondev_subnode_data_ok(acpi_handle handle,
 					const union acpi_object *link,
-					struct list_head *list)
+					struct list_head *list,
+					struct fwnode_handle *parent)
 {
 	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
 	acpi_status status;
@@ -101,7 +107,8 @@ static bool acpi_nondev_subnode_data_ok(acpi_handle handle,
 	if (ACPI_FAILURE(status))
 		return false;
 
-	if (acpi_nondev_subnode_extract(buf.pointer, handle, link, list))
+	if (acpi_nondev_subnode_extract(buf.pointer, handle, link, list,
+					parent))
 		return true;
 
 	ACPI_FREE(buf.pointer);
@@ -110,7 +117,8 @@ static bool acpi_nondev_subnode_data_ok(acpi_handle handle,
 
 static bool acpi_nondev_subnode_ok(acpi_handle scope,
 				   const union acpi_object *link,
-				   struct list_head *list)
+				   struct list_head *list,
+				   struct fwnode_handle *parent)
 {
 	acpi_handle handle;
 	acpi_status status;
@@ -123,12 +131,13 @@ static bool acpi_nondev_subnode_ok(acpi_handle scope,
 	if (ACPI_FAILURE(status))
 		return false;
 
-	return acpi_nondev_subnode_data_ok(handle, link, list);
+	return acpi_nondev_subnode_data_ok(handle, link, list, parent);
 }
 
 static int acpi_add_nondev_subnodes(acpi_handle scope,
 				    const union acpi_object *links,
-				    struct list_head *list)
+				    struct list_head *list,
+				    struct fwnode_handle *parent)
 {
 	bool ret = false;
 	int i;
@@ -150,15 +159,18 @@ static int acpi_add_nondev_subnodes(acpi_handle scope,
 		/* The second one may be a string, a reference or a package. */
 		switch (link->package.elements[1].type) {
 		case ACPI_TYPE_STRING:
-			result = acpi_nondev_subnode_ok(scope, link, list);
+			result = acpi_nondev_subnode_ok(scope, link, list,
+							 parent);
 			break;
 		case ACPI_TYPE_LOCAL_REFERENCE:
 			handle = link->package.elements[1].reference.handle;
-			result = acpi_nondev_subnode_data_ok(handle, link, list);
+			result = acpi_nondev_subnode_data_ok(handle, link, list,
+							     parent);
 			break;
 		case ACPI_TYPE_PACKAGE:
 			desc = &link->package.elements[1];
-			result = acpi_nondev_subnode_extract(desc, NULL, link, list);
+			result = acpi_nondev_subnode_extract(desc, NULL, link,
+							     list, parent);
 			break;
 		default:
 			result = false;
@@ -172,7 +184,8 @@ static int acpi_add_nondev_subnodes(acpi_handle scope,
 
 static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
 					   const union acpi_object *desc,
-					   struct acpi_device_data *data)
+					   struct acpi_device_data *data,
+					   struct fwnode_handle *parent)
 {
 	int i;
 
@@ -194,7 +207,8 @@ static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
 		if (memcmp(uuid->buffer.pointer, ads_uuid, sizeof(ads_uuid)))
 			continue;
 
-		return acpi_add_nondev_subnodes(scope, links, &data->subnodes);
+		return acpi_add_nondev_subnodes(scope, links, &data->subnodes,
+						parent);
 	}
 
 	return false;
@@ -345,7 +359,8 @@ void acpi_init_properties(struct acpi_device *adev)
 		if (acpi_of)
 			acpi_init_of_compatible(adev);
 	}
-	if (acpi_enumerate_nondev_subnodes(adev->handle, buf.pointer, &adev->data))
+	if (acpi_enumerate_nondev_subnodes(adev->handle, buf.pointer,
+					&adev->data, acpi_fwnode_handle(adev)))
 		adev->data.pointer = buf.pointer;
 
 	if (!adev->data.pointer) {
@@ -920,3 +935,30 @@ struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
 	}
 	return NULL;
 }
+
+/**
+ * acpi_node_get_parent - Return parent fwnode of this fwnode
+ * @fwnode: Firmware node whose parent to get
+ *
+ * Returns parent node of an ACPI device or data firmware node or %NULL if
+ * not available.
+ */
+struct fwnode_handle *acpi_node_get_parent(struct fwnode_handle *fwnode)
+{
+	if (is_acpi_data_node(fwnode)) {
+		/* All data nodes have parent pointer so just return that */
+		return to_acpi_data_node(fwnode)->parent;
+	} else if (is_acpi_device_node(fwnode)) {
+		acpi_handle handle, parent_handle;
+
+		handle = to_acpi_device_node(fwnode)->handle;
+		if (ACPI_SUCCESS(acpi_get_parent(handle, &parent_handle))) {
+			struct acpi_device *adev;
+
+			if (!acpi_bus_get_device(parent_handle, &adev))
+				return acpi_fwnode_handle(adev);
+		}
+	}
+
+	return NULL;
+}
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index ef0ae8a..49cca52 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -386,6 +386,7 @@ struct acpi_data_node {
 	const char *name;
 	acpi_handle handle;
 	struct fwnode_handle fwnode;
+	struct fwnode_handle *parent;
 	struct acpi_device_data data;
 	struct list_head sibling;
 	struct kobject kobj;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 673acda..a6f1b74 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1002,6 +1002,7 @@ int acpi_dev_prop_read(struct acpi_device *adev, const char *propname,
 
 struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
 					    struct fwnode_handle *subnode);
+struct fwnode_handle *acpi_node_get_parent(struct fwnode_handle *fwnode);
 
 struct acpi_probe_entry;
 typedef bool (*acpi_probe_entry_validate_subtbl)(struct acpi_subtable_header *,
@@ -1124,6 +1125,12 @@ static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
 	return NULL;
 }
 
+static inline struct fwnode_handle *
+acpi_node_get_parent(struct fwnode_handle *fwnode)
+{
+	return NULL;
+}
+
 #define ACPI_DECLARE_PROBE_ENTRY(table, name, table_id, subtable, valid, data, fn) \
 	static const void * __acpi_table_##name[]			\
 		__attribute__((unused))					\
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 02/16] device property: Add fwnode_get_parent()

[Sakari Ailus]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Now that ACPI has support for returning parent firmware node for both types
of nodes we can expose this to others as well. This adds a new function
fwnode_get_parent() that can be used for DT and ACPI nodes to retrieve the
parent firmware node.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/base/property.c  | 25 +++++++++++++++++++++++++
 include/linux/property.h |  2 ++
 2 files changed, 27 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index e67ec24..1023e50 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -958,6 +958,31 @@ int device_add_properties(struct device *dev,
 EXPORT_SYMBOL_GPL(device_add_properties);
 
 /**
+ * fwnode_get_parent - Return parent firwmare node
+ * @fwnode: Firmware whose parent is retrieved
+ *
+ * Return parent firmware node of the given node if possible or %NULL if no
+ * parent was available.
+ */
+struct fwnode_handle *fwnode_get_parent(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *parent = NULL;
+
+	if (is_of_node(fwnode)) {
+		struct device_node *node;
+
+		node = of_get_parent(to_of_node(fwnode));
+		if (node)
+			parent = &node->fwnode;
+	} else if (is_acpi_node(fwnode)) {
+		parent = acpi_node_get_parent(fwnode);
+	}
+
+	return parent;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_parent);
+
+/**
  * device_get_next_child_node - 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.
diff --git a/include/linux/property.h b/include/linux/property.h
index 64e3a9c..ab0a816 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -70,6 +70,8 @@ int fwnode_property_read_string(struct fwnode_handle *fwnode,
 int fwnode_property_match_string(struct fwnode_handle *fwnode,
 				 const char *propname, const char *string);
 
+struct fwnode_handle *fwnode_get_parent(struct fwnode_handle *fwnode);
+
 struct fwnode_handle *device_get_next_child_node(struct device *dev,
 						 struct fwnode_handle *child);
 
-- 
2.7.4

[PATCH v4 03/16] ACPI / property: Add fwnode_get_next_child_node()

[Sakari Ailus]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

The ACPI _DSD hierarchical data extension [1] makes it possible to have
hierarchies deeper than one level in similar way than DT allows. These
"subsubnodes" have not been accessible because device property
implementation only provides device_get_next_child_node() that is limited
to direct descendants of a device.

We need this ability in order support things like remote endpoints
currently supported in DT with of_graph_* APIs.

Modify acpi_get_next_subnode() to accept fwnode handle instead and update
callers accordingly. Also add a new function fwnode_get_next_child_node()
that works directly with fwnodes and modify device_get_next_child_node() to
call it directly. While there add a macro fwnode_for_each_child_node()
analogous to the current device_for_each_child_node() but it works with
fwnodes instead of devices.

[1] http://www.uefi.org/sites/default/files/resources/_DSD-hierarchical-data-extension-UUID-v1.pdf

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/property.c  | 27 +++++++++++++++++----------
 drivers/base/property.c  | 38 ++++++++++++++++++++++++++++++--------
 include/linux/acpi.h     |  8 ++++----
 include/linux/property.h |  6 ++++++
 4 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 587c9d0..8730ce7 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -880,21 +880,22 @@ int acpi_node_prop_read(struct fwnode_handle *fwnode,  const char *propname,
 }
 
 /**
- * acpi_get_next_subnode - Return the next child node handle for a device.
- * @dev: Device to find the next child node for.
+ * acpi_get_next_subnode - Return the next child node handle for a fwnode
+ * @fwnode: Firmware node 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 *acpi_get_next_subnode(struct fwnode_handle *fwnode,
 					    struct fwnode_handle *child)
 {
-	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct acpi_device *adev = to_acpi_device_node(fwnode);
 	struct list_head *head, *next;
 
-	if (!adev)
-		return NULL;
-
 	if (!child || child->type == FWNODE_ACPI) {
-		head = &adev->children;
+		if (adev)
+			head = &adev->children;
+		else
+			goto nondev;
+
 		if (list_empty(head))
 			goto nondev;
 
@@ -903,7 +904,6 @@ struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
 			next = adev->node.next;
 			if (next == head) {
 				child = NULL;
-				adev = ACPI_COMPANION(dev);
 				goto nondev;
 			}
 			adev = list_entry(next, struct acpi_device, node);
@@ -915,9 +915,16 @@ struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
 
  nondev:
 	if (!child || child->type == FWNODE_ACPI_DATA) {
+		struct acpi_data_node *data = to_acpi_data_node(fwnode);
 		struct acpi_data_node *dn;
 
-		head = &adev->data.subnodes;
+		if (adev)
+			head = &adev->data.subnodes;
+		else if (data)
+			head = &data->data.subnodes;
+		else
+			return NULL;
+
 		if (list_empty(head))
 			return NULL;
 
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 1023e50..55d994d 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -983,24 +983,46 @@ struct fwnode_handle *fwnode_get_parent(struct fwnode_handle *fwnode)
 EXPORT_SYMBOL_GPL(fwnode_get_parent);
 
 /**
- * device_get_next_child_node - 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.
+ * fwnode_get_next_child_node - Return the next child node handle for a node
+ * @fwnode: Firmware node to find the next child node for.
+ * @child: Handle to one of the node's child nodes or a %NULL handle.
  */
-struct fwnode_handle *device_get_next_child_node(struct device *dev,
+struct fwnode_handle *fwnode_get_next_child_node(struct fwnode_handle *fwnode,
 						 struct fwnode_handle *child)
 {
-	if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
+	if (is_of_node(fwnode)) {
 		struct device_node *node;
 
-		node = of_get_next_available_child(dev->of_node, to_of_node(child));
+		node = of_get_next_available_child(to_of_node(fwnode),
+						   to_of_node(child));
 		if (node)
 			return &node->fwnode;
-	} else if (IS_ENABLED(CONFIG_ACPI)) {
-		return acpi_get_next_subnode(dev, child);
+	} else if (is_acpi_node(fwnode)) {
+		return acpi_get_next_subnode(fwnode, child);
 	}
+
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(fwnode_get_next_child_node);
+
+/**
+ * device_get_next_child_node - 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 *device_get_next_child_node(struct device *dev,
+						 struct fwnode_handle *child)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct fwnode_handle *fwnode = NULL;
+
+	if (dev->of_node)
+		fwnode = &dev->of_node->fwnode;
+	else if (adev)
+		fwnode = acpi_fwnode_handle(adev);
+
+	return fwnode_get_next_child_node(fwnode, child);
+}
 EXPORT_SYMBOL_GPL(device_get_next_child_node);
 
 /**
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a6f1b74..8096b3a 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1000,8 +1000,8 @@ int acpi_node_prop_read(struct fwnode_handle *fwnode, const char *propname,
 int acpi_dev_prop_read(struct acpi_device *adev, const char *propname,
 		       enum dev_prop_type proptype, void *val, size_t nval);
 
-struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
-					    struct fwnode_handle *subnode);
+struct fwnode_handle *acpi_get_next_subnode(struct fwnode_handle *fwnode,
+					    struct fwnode_handle *child);
 struct fwnode_handle *acpi_node_get_parent(struct fwnode_handle *fwnode);
 
 struct acpi_probe_entry;
@@ -1119,8 +1119,8 @@ static inline int acpi_dev_prop_read(struct acpi_device *adev,
 	return -ENXIO;
 }
 
-static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
-						struct fwnode_handle *subnode)
+static inline struct fwnode_handle *
+acpi_get_next_subnode(struct fwnode_handle *fwnode, struct fwnode_handle *child)
 {
 	return NULL;
 }
diff --git a/include/linux/property.h b/include/linux/property.h
index ab0a816..f4786a86 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -71,6 +71,12 @@ int fwnode_property_match_string(struct fwnode_handle *fwnode,
 				 const char *propname, const char *string);
 
 struct fwnode_handle *fwnode_get_parent(struct fwnode_handle *fwnode);
+struct fwnode_handle *fwnode_get_next_child_node(struct fwnode_handle *fwnode,
+						 struct fwnode_handle *child);
+
+#define fwnode_for_each_child_node(fwnode, child)			\
+	for (child = fwnode_get_next_child_node(fwnode, NULL); child;	\
+	     child = fwnode_get_next_child_node(fwnode, child))
 
 struct fwnode_handle *device_get_next_child_node(struct device *dev,
 						 struct fwnode_handle *child);
-- 
2.7.4

[PATCH v4 04/16] device property: Add fwnode_get_named_child_node()

[Sakari Ailus]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Since now we have means to enumerate all children of any fwnode even in
ACPI we can implement fwnode_get_named_child_node(). This is similar than
device_get_named_child_node() with the exception that it can be called to
any fwnode handle. Make device_get_named_child_node() call directly this
new function.

This is useful in cases where we need to be able to find child nodes which
are not direct descendants of the parent device.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/base/property.c  | 22 +++++++++++++++++-----
 include/linux/property.h |  2 ++
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 55d994d..ed9ee94 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1026,20 +1026,20 @@ struct fwnode_handle *device_get_next_child_node(struct device *dev,
 EXPORT_SYMBOL_GPL(device_get_next_child_node);
 
 /**
- * device_get_named_child_node - Return first matching named child node handle
- * @dev: Device to find the named child node for.
+ * fwnode_get_named_child_node - Return first matching named child node handle
+ * @fwnode: Firmware node to find the named child node for.
  * @childname: String to match child node name against.
  */
-struct fwnode_handle *device_get_named_child_node(struct device *dev,
+struct fwnode_handle *fwnode_get_named_child_node(struct fwnode_handle *fwnode,
 						  const char *childname)
 {
 	struct fwnode_handle *child;
 
 	/*
-	 * Find first matching named child node of this device.
+	 * Find first matching named child node of this fwnode.
 	 * For ACPI this will be a data only sub-node.
 	 */
-	device_for_each_child_node(dev, child) {
+	fwnode_for_each_child_node(fwnode, child) {
 		if (is_of_node(child)) {
 			if (!of_node_cmp(to_of_node(child)->name, childname))
 				return child;
@@ -1051,6 +1051,18 @@ struct fwnode_handle *device_get_named_child_node(struct device *dev,
 
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(fwnode_get_named_child_node);
+
+/**
+ * device_get_named_child_node - Return first matching named child node handle
+ * @dev: Device to find the named child node for.
+ * @childname: String to match child node name against.
+ */
+struct fwnode_handle *device_get_named_child_node(struct device *dev,
+						  const char *childname)
+{
+	return fwnode_get_named_child_node(dev_fwnode(dev), childname);
+}
 EXPORT_SYMBOL_GPL(device_get_named_child_node);
 
 /**
diff --git a/include/linux/property.h b/include/linux/property.h
index f4786a86..514b195 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -85,6 +85,8 @@ struct fwnode_handle *device_get_next_child_node(struct device *dev,
 	for (child = device_get_next_child_node(dev, NULL); child;	\
 	     child = device_get_next_child_node(dev, child))
 
+struct fwnode_handle *fwnode_get_named_child_node(struct fwnode_handle *fwnode,
+						  const char *childname);
 struct fwnode_handle *device_get_named_child_node(struct device *dev,
 						  const char *childname);
 
-- 
2.7.4

[PATCH v4 05/16] ACPI / property: Add support for remote endpoints

[Sakari Ailus]

From: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

DT has had concept of remote endpoints for some time already. It makes
possible to reference another firmware node through a property called
remote-endpoint. This is already used by some subsystems like v4l2 for
parsing hardware properties related to camera.

This patch adds ACPI support for remote endpoints utilizing _DSD
hierarchical data extensions.

Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/acpi/property.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h    |  23 ++++++++
 2 files changed, 162 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 8730ce7..6e776de 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -969,3 +969,142 @@ struct fwnode_handle *acpi_node_get_parent(struct fwnode_handle *fwnode)
 
 	return NULL;
 }
+
+/**
+ * acpi_graph_get_next_endpoint - Get next endpoint ACPI firmware node
+ * @fwnode: Pointer to the parent firmware node
+ * @prev: Previous endpoint node or %NULL to get the first
+ *
+ * Looks up next endpoint ACPI firmware node below a given @fwnode. Returns
+ * %NULL if there is no next endpoint, ERR_PTR() in case of error. In case
+ * of success the next endpoint is returned.
+ */
+struct fwnode_handle *acpi_graph_get_next_endpoint(struct fwnode_handle *fwnode,
+						   struct fwnode_handle *prev)
+{
+	struct fwnode_handle *port = NULL;
+	struct fwnode_handle *endpoint;
+
+	if (!prev) {
+		do {
+			port = fwnode_get_next_child_node(fwnode, port);
+			/* Ports must have port property */
+			if (fwnode_property_present(port, "port"))
+				break;
+		} while (port);
+	} else {
+		port = fwnode_get_parent(prev);
+	}
+
+	if (!port)
+		return NULL;
+
+	endpoint = fwnode_get_next_child_node(port, prev);
+	while (!endpoint) {
+		port = fwnode_get_next_child_node(fwnode, port);
+		if (!port)
+			break;
+		if (fwnode_property_present(port, "port"))
+			endpoint = fwnode_get_next_child_node(port, NULL);
+	}
+
+	if (endpoint) {
+		/* Endpoints must have "endpoint" property */
+		if (!fwnode_property_present(endpoint, "endpoint"))
+			return ERR_PTR(-EPROTO);
+	}
+
+	return endpoint;
+}
+
+/**
+ * acpi_graph_get_child_at - Return a data extension child at given index
+ * @fwnode: parent node
+ * @at: index of the child node in the data extension package
+ *
+ * Find the child node at index of a data extension. Returns the child
+ * node on success, NULL otherwise.
+ */
+static struct fwnode_handle *acpi_graph_get_child_at(
+	struct fwnode_handle *fwnode, unsigned int at)
+{
+	struct fwnode_handle *child;
+	unsigned int i = 0;
+
+	fwnode_for_each_child_node(fwnode, child) {
+		if (i++ < at)
+			continue;
+
+		return child;
+	}
+
+	return NULL;
+}
+
+/**
+ * acpi_graph_get_remote_enpoint - Parses and returns remote end of an endpoint
+ * @fwnode: Endpoint firmware node pointing to a remote device
+ * @parent: Firmware node of remote port parent is filled here if not %NULL
+ * @port: Firmware node of remote port is filled here if not %NULL
+ * @endpoint: Firmware node of remote endpoint is filled here if not %NULL
+ *
+ * Function parses remote end of ACPI firmware remote endpoint and fills in
+ * fields requested by the caller. Returns %0 in case of success and
+ * negative errno otherwise.
+ */
+int acpi_graph_get_remote_endpoint(struct fwnode_handle *fwnode,
+				   struct fwnode_handle **parent,
+				   struct fwnode_handle **port,
+				   struct fwnode_handle **endpoint)
+{
+	unsigned int port_idx, endpoint_idx;
+	struct acpi_reference_args args;
+	int ret;
+
+	memset(&args, 0, sizeof(args));
+	ret = acpi_node_get_property_reference(fwnode, "remote-endpoint", 0,
+					       &args);
+	if (ret)
+		return ret;
+
+	/*
+	 * Always require two arguments with the reference: port and
+	 * endpoint indices.
+	 */
+	if (args.nargs != 2)
+		return -EPROTO;
+
+	fwnode = acpi_fwnode_handle(args.adev);
+	port_idx = args.args[0];
+	endpoint_idx = args.args[1];
+
+	if (parent)
+		*parent = fwnode;
+
+	if (!port && !endpoint)
+		return 0;
+
+	fwnode = acpi_graph_get_child_at(fwnode, port_idx);
+	if (!fwnode)
+		return -EPROTO;
+
+	if (!fwnode_property_present(fwnode, "port"))
+		return -EPROTO;
+
+	if (port)
+		*port = fwnode;
+
+	if (!endpoint)
+		return 0;
+
+	fwnode = acpi_graph_get_child_at(fwnode, endpoint_idx);
+	if (!fwnode)
+		return -EPROTO;
+
+	if (!fwnode_property_present(fwnode, "endpoint"))
+		return -EPROTO;
+
+	*endpoint = fwnode;
+
+	return 0;
+}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 8096b3a..9d920c0 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1004,6 +1004,13 @@ struct fwnode_handle *acpi_get_next_subnode(struct fwnode_handle *fwnode,
 					    struct fwnode_handle *child);
 struct fwnode_handle *acpi_node_get_parent(struct fwnode_handle *fwnode);
 
+struct fwnode_handle *acpi_graph_get_next_endpoint(struct fwnode_handle *fwnode,
+						   struct fwnode_handle *prev);
+int acpi_graph_get_remote_endpoint(struct fwnode_handle *fwnode,
+				   struct fwnode_handle **remote,
+				   struct fwnode_handle **port,
+				   struct fwnode_handle **endpoint);
+
 struct acpi_probe_entry;
 typedef bool (*acpi_probe_entry_validate_subtbl)(struct acpi_subtable_header *,
 						 struct acpi_probe_entry *);
@@ -1131,6 +1138,22 @@ acpi_node_get_parent(struct fwnode_handle *fwnode)
 	return NULL;
 }
 
+static inline struct fwnode_handle *
+acpi_graph_get_next_endpoint(struct fwnode_handle *fwnode,
+			     struct fwnode_handle *prev)
+{
+	return ERR_PTR(-ENXIO);
+}
+
+static inline int
+acpi_graph_get_remote_endpoint(struct fwnode_handle *fwnode,
+			       struct fwnode_handle **remote,
+			       struct fwnode_handle **port,
+			       struct fwnode_handle **endpoint)
+{
+	return -ENXIO;
+}
+
 #define ACPI_DECLARE_PROBE_ENTRY(table, name, table_id, subtable, valid, data, fn) \
 	static const void * __acpi_table_##name[]			\
 		__attribute__((unused))					\
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 06/16] device property: Add support for remote endpoints

[Sakari Ailus]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

This follows DT implementation of of_graph_* APIs but we call them
fwnode_graph_* instead. For DT nodes the existing of_graph_* implementation
will be used. For ACPI we use the new ACPI graph implementation instead.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/base/property.c  | 123 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |   9 ++++
 2 files changed, 132 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index ed9ee94..d827024 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_graph.h>
 #include <linux/property.h>
 #include <linux/etherdevice.h>
 #include <linux/phy.h>
@@ -1202,3 +1203,125 @@ void *device_get_mac_address(struct device *dev, char *addr, int alen)
 	return device_get_mac_addr(dev, "address", addr, alen);
 }
 EXPORT_SYMBOL(device_get_mac_address);
+
+/**
+ * device_graph_get_next_endpoint - Get next endpoint firmware node
+ * @fwnode: Pointer to the parent firmware node
+ * @prev: Previous endpoint node or %NULL to get the first
+ *
+ * Returns an endpoint firmware node pointer or %NULL if no more endpoints
+ * are available.
+ */
+struct fwnode_handle *
+fwnode_graph_get_next_endpoint(struct fwnode_handle *fwnode,
+			       struct fwnode_handle *prev)
+{
+	struct fwnode_handle *endpoint = NULL;
+
+	if (is_of_node(fwnode)) {
+		struct device_node *node;
+
+		node = of_graph_get_next_endpoint(to_of_node(fwnode),
+						  to_of_node(prev));
+
+		if (node)
+			endpoint = &node->fwnode;
+	} else if (is_acpi_node(fwnode)) {
+		endpoint = acpi_graph_get_next_endpoint(fwnode, prev);
+		if (IS_ERR(endpoint))
+			endpoint = NULL;
+	}
+
+	return endpoint;
+
+}
+EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
+
+/**
+ * fwnode_graph_get_remote_port_parent - Return fwnode of a remote device
+ * @fwnode: Endpoint firmware node pointing to the remote endpoint
+ *
+ * Extracts firmware node of a remote device the @fwnode points to.
+ */
+struct fwnode_handle *
+fwnode_graph_get_remote_port_parent(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *parent = NULL;
+
+	if (is_of_node(fwnode)) {
+		struct device_node *node;
+
+		node = of_graph_get_remote_port_parent(to_of_node(fwnode));
+		if (node)
+			parent = &node->fwnode;
+	} else if (is_acpi_node(fwnode)) {
+		int ret;
+
+		ret = acpi_graph_get_remote_endpoint(fwnode, &parent, NULL,
+						     NULL);
+		if (ret)
+			return NULL;
+	}
+
+	return parent;
+}
+EXPORT_SYMBOL_GPL(fwnode_graph_get_remote_port_parent);
+
+/**
+ * fwnode_graph_get_remote_port - Return fwnode of a remote port
+ * @fwnode: Endpoint firmware node pointing to the remote endpoint
+ *
+ * Extracts firmware node of a remote port the @fwnode points to.
+ */
+struct fwnode_handle *fwnode_graph_get_remote_port(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *port = NULL;
+
+	if (is_of_node(fwnode)) {
+		struct device_node *node;
+
+		node = of_graph_get_remote_port(to_of_node(fwnode));
+		if (node)
+			port = &node->fwnode;
+	} else if (is_acpi_node(fwnode)) {
+		int ret;
+
+		ret = acpi_graph_get_remote_endpoint(fwnode, NULL, &port, NULL);
+		if (ret)
+			return NULL;
+	}
+
+	return port;
+}
+EXPORT_SYMBOL_GPL(fwnode_graph_get_remote_port);
+
+/**
+ * fwnode_graph_get_remote_endpoint - Return fwnode of a remote endpoint
+ * @fwnode: Endpoint firmware node pointing to the remote endpoint
+ *
+ * Extracts firmware node of a remote endpoint the @fwnode points to.
+ */
+struct fwnode_handle *
+fwnode_graph_get_remote_endpoint(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *endpoint = NULL;
+
+	if (is_of_node(fwnode)) {
+		struct device_node *node;
+
+		node = of_parse_phandle(to_of_node(fwnode), "remote-endpoint",
+					0);
+		if (node)
+			endpoint = &node->fwnode;
+	} else if (is_acpi_node(fwnode)) {
+		int ret;
+
+		ret = acpi_graph_get_remote_endpoint(fwnode, NULL, NULL,
+						     &endpoint);
+		if (ret)
+			return NULL;
+	}
+
+	return endpoint;
+}
+EXPORT_SYMBOL_GPL(fwnode_graph_get_remote_endpoint);
diff --git a/include/linux/property.h b/include/linux/property.h
index 514b195..8d7809c 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -268,4 +268,13 @@ int device_get_phy_mode(struct device *dev);
 
 void *device_get_mac_address(struct device *dev, char *addr, int alen);
 
+struct fwnode_handle *fwnode_graph_get_next_endpoint(
+	struct fwnode_handle *fwnode, struct fwnode_handle *prev);
+struct fwnode_handle *fwnode_graph_get_remote_port_parent(
+	struct fwnode_handle *fwnode);
+struct fwnode_handle *fwnode_graph_get_remote_port(
+	struct fwnode_handle *fwnode);
+struct fwnode_handle *fwnode_graph_get_remote_endpoint(
+	struct fwnode_handle *fwnode);
+
 #endif /* _LINUX_PROPERTY_H_ */
-- 
2.7.4

[PATCH v4 07/16] device property: Add fwnode_handle_get()

[Sakari Ailus]

fwnode_handle_get() is used to obtain a reference to a fwnode_handle
container. In this case this is OF specific struct device_node.

This complements fwnode_handle_put() which is already implemented.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/base/property.c  | 11 +++++++++++
 include/linux/property.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index d827024..111049b 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1067,6 +1067,17 @@ struct fwnode_handle *device_get_named_child_node(struct device *dev,
 EXPORT_SYMBOL_GPL(device_get_named_child_node);
 
 /**
+ * fwnode_handle_get - Obtain a reference to a device node
+ * @fwnode: Pointer to the device node to obtain the reference to.
+ */
+void fwnode_handle_get(struct fwnode_handle *fwnode)
+{
+	if (is_of_node(fwnode))
+		of_node_get(to_of_node(fwnode));
+}
+EXPORT_SYMBOL_GPL(fwnode_handle_get);
+
+/**
  * fwnode_handle_put - Drop reference to a device node
  * @fwnode: Pointer to the device node to drop the reference to.
  *
diff --git a/include/linux/property.h b/include/linux/property.h
index 8d7809c..0ae7d20 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -90,6 +90,7 @@ struct fwnode_handle *fwnode_get_named_child_node(struct fwnode_handle *fwnode,
 struct fwnode_handle *device_get_named_child_node(struct device *dev,
 						  const char *childname);
 
+void fwnode_handle_get(struct fwnode_handle *fwnode);
 void fwnode_handle_put(struct fwnode_handle *fwnode);
 
 unsigned int device_get_child_node_count(struct device *dev);
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 08/16] of: Add of_fwnode_handle() to convert device nodes to fwnode_handle

[Sakari Ailus]

of_fwnode_handle() returns a struct fwnode_handle of the struct
device_node. This may be used on the fwnode property API.

Use a macro instead of a function in order to support const and non-const
arguments.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/linux/of.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index 21e6323..e5d4225f 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -159,6 +159,8 @@ static inline struct device_node *to_of_node(struct fwnode_handle *fwnode)
 		container_of(fwnode, struct device_node, fwnode) : NULL;
 }
 
+#define of_fwnode_handle(node) (&(node)->fwnode)
+
 static inline bool of_have_populated_dt(void)
 {
 	return of_root != NULL;
@@ -602,6 +604,8 @@ static inline struct device_node *of_find_node_with_property(
 	return NULL;
 }
 
+#define of_fwnode_handle(node) NULL
+
 static inline bool of_have_populated_dt(void)
 {
 	return false;
-- 
2.7.4

[PATCH v4 09/16] driver core: Arrange headers alphabetically

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/base/core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 684bda4..fde2c7c 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -13,20 +13,20 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/fwnode.h>
+#include <linux/genhd.h>
 #include <linux/init.h>
-#include <linux/module.h>
-#include <linux/slab.h>
-#include <linux/string.h>
+#include <linux/kallsyms.h>
 #include <linux/kdev_t.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/netdevice.h>
 #include <linux/notifier.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
-#include <linux/genhd.h>
-#include <linux/kallsyms.h>
-#include <linux/mutex.h>
 #include <linux/pm_runtime.h>
-#include <linux/netdevice.h>
 #include <linux/sched/signal.h>
+#include <linux/slab.h>
+#include <linux/string.h>
 #include <linux/sysfs.h>
 
 #include "base.h"
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 10/16] irqchip/gic: Add missing forward declaration for struct device

[Sakari Ailus]

A pointer to struct device is used in the header as a function argument
but there's no declaration for it. Add one.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/linux/irqchip/arm-gic.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index eafc965..b16afa1 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -100,6 +100,7 @@
 
 #include <linux/irqdomain.h>
 
+struct device;
 struct device_node;
 struct gic_chip_data;
 
-- 
2.7.4

[PATCH v4 11/16] of: No need to include linux/property.h, linux/fwnode.h is sufficient

[Sakari Ailus]

of.h requires a definition of struct fwnode_handle, and for that it
includes linux/property.h. struct fwnode_handle, however, is defined in
linux/fwnode.h. Include linux/fwnode.h directly.

A number of users were however depending on linux/property.h, thus fix
them by including that header directly as well.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/base/core.c                       | 1 +
 drivers/input/keyboard/gpio_keys.c        | 1 +
 drivers/input/misc/drv260x.c              | 1 +
 drivers/input/misc/gpio_decoder.c         | 1 +
 drivers/input/touchscreen/ad7879.c        | 1 +
 drivers/input/touchscreen/edt-ft5x06.c    | 1 +
 drivers/input/touchscreen/tsc200x-core.c  | 2 +-
 drivers/net/ethernet/faraday/ftgmac100.c  | 1 +
 drivers/net/ethernet/smsc/smc91x.c        | 1 +
 drivers/phy/phy-tusb1210.c                | 1 +
 drivers/power/supply/bq24735-charger.c    | 1 +
 drivers/usb/common/common.c               | 1 +
 drivers/usb/dwc2/debugfs.c                | 3 ++-
 drivers/usb/dwc2/params.c                 | 1 +
 drivers/usb/dwc2/pci.c                    | 3 ++-
 drivers/usb/dwc3/host.c                   | 1 +
 include/linux/of.h                        | 2 +-
 sound/soc/codecs/rt5514.c                 | 1 +
 sound/soc/codecs/ts3a227e.c               | 1 +
 sound/soc/mediatek/mt8173/mt8173-rt5650.c | 1 +
 sound/soc/rockchip/rk3399_gru_sound.c     | 1 +
 21 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index fde2c7c..92e7d80 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -24,6 +24,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/property.h>
 #include <linux/sched/signal.h>
 #include <linux/slab.h>
 #include <linux/string.h>
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index da3d362..a5509aa 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -20,6 +20,7 @@
 #include <linux/slab.h>
 #include <linux/sysctl.h>
 #include <linux/proc_fs.h>
+#include <linux/property.h>
 #include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/input.h>
diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
index fb089d3..a3972df 100644
--- a/drivers/input/misc/drv260x.c
+++ b/drivers/input/misc/drv260x.c
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
+#include <linux/property.h>
 #include <linux/regulator/consumer.h>
 
 #include <dt-bindings/input/ti-drv260x.h>
diff --git a/drivers/input/misc/gpio_decoder.c b/drivers/input/misc/gpio_decoder.c
index 1dca526..10bc109 100644
--- a/drivers/input/misc/gpio_decoder.c
+++ b/drivers/input/misc/gpio_decoder.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 
 struct gpio_decoder {
 	struct input_polled_dev *poll_dev;
diff --git a/drivers/input/touchscreen/ad7879.c b/drivers/input/touchscreen/ad7879.c
index e16a446..38ad365 100644
--- a/drivers/input/touchscreen/ad7879.c
+++ b/drivers/input/touchscreen/ad7879.c
@@ -34,6 +34,7 @@
 #include <linux/input/touchscreen.h>
 #include <linux/platform_data/ad7879.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include "ad7879.h"
 
 #define AD7879_REG_ZEROS		0
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 8cf8d8d..3beaea2 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -39,6 +39,7 @@
 #include <linux/input/mt.h>
 #include <linux/input/touchscreen.h>
 #include <linux/of_device.h>
+#include <linux/property.h>
 
 #define WORK_REGISTER_THRESHOLD		0x00
 #define WORK_REGISTER_REPORT_RATE	0x08
diff --git a/drivers/input/touchscreen/tsc200x-core.c b/drivers/input/touchscreen/tsc200x-core.c
index 88ea5e1..3964d67 100644
--- a/drivers/input/touchscreen/tsc200x-core.c
+++ b/drivers/input/touchscreen/tsc200x-core.c
@@ -25,10 +25,10 @@
 #include <linux/input/touchscreen.h>
 #include <linux/interrupt.h>
 #include <linux/delay.h>
-#include <linux/pm.h>
 #include <linux/of.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regmap.h>
+#include <linux/property.h>
 #include <linux/gpio/consumer.h>
 #include "tsc200x-core.h"
 
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 928b0df..f3eaa5f 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -30,6 +30,7 @@
 #include <linux/netdevice.h>
 #include <linux/phy.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <net/ip.h>
 #include <net/ncsi.h>
 
diff --git a/drivers/net/ethernet/smsc/smc91x.c b/drivers/net/ethernet/smsc/smc91x.c
index 65077c7..5352a08 100644
--- a/drivers/net/ethernet/smsc/smc91x.c
+++ b/drivers/net/ethernet/smsc/smc91x.c
@@ -75,6 +75,7 @@ static const char version[] =
 #include <linux/ioport.h>
 #include <linux/crc32.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/spinlock.h>
 #include <linux/ethtool.h>
 #include <linux/mii.h>
diff --git a/drivers/phy/phy-tusb1210.c b/drivers/phy/phy-tusb1210.c
index 4f6d5e7..f7dd21aa 100644
--- a/drivers/phy/phy-tusb1210.c
+++ b/drivers/phy/phy-tusb1210.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/ulpi/driver.h>
 #include <linux/gpio/consumer.h>
+#include <linux/property.h>
 
 #include "ulpi_phy.h"
 
diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
index eb01453..690bb4d 100644
--- a/drivers/power/supply/bq24735-charger.c
+++ b/drivers/power/supply/bq24735-charger.c
@@ -27,6 +27,7 @@
 #include <linux/of.h>
 #include <linux/gpio/consumer.h>
 #include <linux/power_supply.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 
 #include <linux/power/bq24735-charger.h>
diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index 5ef8da6..534a498 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -14,6 +14,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/property.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/of.h>
 #include <linux/usb/otg.h>
diff --git a/drivers/usb/dwc2/debugfs.c b/drivers/usb/dwc2/debugfs.c
index 794b959..10217cd 100644
--- a/drivers/usb/dwc2/debugfs.c
+++ b/drivers/usb/dwc2/debugfs.c
@@ -14,9 +14,10 @@
  * GNU General Public License for more details.
  */
 
-#include <linux/spinlock.h>
 #include <linux/debugfs.h>
+#include <linux/property.h>
 #include <linux/seq_file.h>
+#include <linux/spinlock.h>
 #include <linux/uaccess.h>
 
 #include "core.h"
diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 2990c34..0fe39ce 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -35,6 +35,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/property.h>
 
 #include "core.h"
 
diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c
index fdeb8c7..333a5cb 100644
--- a/drivers/usb/dwc2/pci.c
+++ b/drivers/usb/dwc2/pci.c
@@ -44,8 +44,9 @@
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
-#include <linux/slab.h>
 #include <linux/pci.h>
+#include <linux/property.h>
+#include <linux/slab.h>
 #include <linux/usb.h>
 
 #include <linux/usb/hcd.h>
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 76f0b0d..563b1df 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/platform_device.h>
+#include <linux/property.h>
 
 #include "core.h"
 
diff --git a/include/linux/of.h b/include/linux/of.h
index e5d4225f..d48e225 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -23,7 +23,7 @@
 #include <linux/spinlock.h>
 #include <linux/topology.h>
 #include <linux/notifier.h>
-#include <linux/property.h>
+#include <linux/fwnode.h>
 #include <linux/list.h>
 
 #include <asm/byteorder.h>
diff --git a/sound/soc/codecs/rt5514.c b/sound/soc/codecs/rt5514.c
index b281a46..0749219 100644
--- a/sound/soc/codecs/rt5514.c
+++ b/sound/soc/codecs/rt5514.c
@@ -15,6 +15,7 @@
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/pm.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/i2c.h>
 #include <linux/platform_device.h>
diff --git a/sound/soc/codecs/ts3a227e.c b/sound/soc/codecs/ts3a227e.c
index 4356843..e80b28b 100644
--- a/sound/soc/codecs/ts3a227e.c
+++ b/sound/soc/codecs/ts3a227e.c
@@ -14,6 +14,7 @@
 #include <linux/input.h>
 #include <linux/module.h>
 #include <linux/of_gpio.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 
 #include <sound/core.h>
diff --git a/sound/soc/mediatek/mt8173/mt8173-rt5650.c b/sound/soc/mediatek/mt8173/mt8173-rt5650.c
index ba65f41..433ae4f 100644
--- a/sound/soc/mediatek/mt8173/mt8173-rt5650.c
+++ b/sound/soc/mediatek/mt8173/mt8173-rt5650.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
+#include <linux/property.h>
 #include <sound/soc.h>
 #include <sound/jack.h>
 #include "../../codecs/rt5645.h"
diff --git a/sound/soc/rockchip/rk3399_gru_sound.c b/sound/soc/rockchip/rk3399_gru_sound.c
index 3475c61..9134b46 100644
--- a/sound/soc/rockchip/rk3399_gru_sound.c
+++ b/sound/soc/rockchip/rk3399_gru_sound.c
@@ -22,6 +22,7 @@
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
 #include <linux/delay.h>
+#include <linux/property.h>
 #include <linux/spi/spi.h>
 #include <linux/input.h>
 #include <sound/core.h>
-- 
2.7.4

[PATCH v4 12/16] device property: Move dev_fwnode() to linux/property.h

[Sakari Ailus]

The function to obtain a fwnode related to a struct device is useful for
drivers that use the fwnode property API: it allows not being aware of the
underlying firmware implementation.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/base/property.c  |  6 ------
 include/linux/property.h | 10 ++++++++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 111049b..860e160 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -183,12 +183,6 @@ static int pset_prop_read_string(struct property_set *pset,
 	return 0;
 }
 
-static inline struct fwnode_handle *dev_fwnode(struct device *dev)
-{
-	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
-		&dev->of_node->fwnode : dev->fwnode;
-}
-
 /**
  * device_property_present - check if a property of a device is present
  * @dev: Device whose property is being checked
diff --git a/include/linux/property.h b/include/linux/property.h
index 0ae7d20..0b61ea4 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -13,11 +13,11 @@
 #ifndef _LINUX_PROPERTY_H_
 #define _LINUX_PROPERTY_H_
 
+#include <linux/device.h>
 #include <linux/fwnode.h>
+#include <linux/of.h>
 #include <linux/types.h>
 
-struct device;
-
 enum dev_prop_type {
 	DEV_PROP_U8,
 	DEV_PROP_U16,
@@ -33,6 +33,12 @@ enum dev_dma_attr {
 	DEV_DMA_COHERENT,
 };
 
+static inline struct fwnode_handle *dev_fwnode(struct device *dev)
+{
+	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
+		&dev->of_node->fwnode : dev->fwnode;
+}
+
 bool device_property_present(struct device *dev, const char *propname);
 int device_property_read_u8_array(struct device *dev, const char *propname,
 				  u8 *val, size_t nval);
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 13/16] device property: Add support for fwnode endpoints

[Sakari Ailus]

Similar to OF endpoints, endpoint type nodes can be also supported on
ACPI. In order to make it possible for drivers to ignore the matter,
add a type for fwnode_endpoint and a function to parse them.

On ACPI, find the child node index instead of relying on the "endpoint"
property.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/base/property.c  | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/fwnode.h   |  6 ++++++
 include/linux/property.h |  3 +++
 3 files changed, 45 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 860e160..b8c1017 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1330,3 +1330,39 @@ fwnode_graph_get_remote_endpoint(struct fwnode_handle *fwnode)
 	return endpoint;
 }
 EXPORT_SYMBOL_GPL(fwnode_graph_get_remote_endpoint);
+
+/*
+ * fwnode_graph_parse_endpoint() - parse common endpoint node properties
+ * @node: pointer to endpoint device_node
+ * @endpoint: pointer to the fwnode endpoint data structure
+ *
+ * The caller should hold a reference to @node.
+ */
+int fwnode_graph_parse_endpoint(struct fwnode_handle *fwnode,
+				struct fwnode_endpoint *endpoint)
+{
+	struct fwnode_handle *port_fwnode = fwnode_get_parent(fwnode);
+
+	memset(endpoint, 0, sizeof(*endpoint));
+
+	endpoint->local_fwnode = fwnode;
+
+	if (is_acpi_node(port_fwnode)) {
+		struct fwnode_handle *iter;
+
+		fwnode_property_read_u32(port_fwnode, "port", &endpoint->port);
+
+		for (iter = fwnode_get_next_child_node(port_fwnode, NULL);
+		     iter != fwnode;
+		     iter = fwnode_get_next_child_node(port_fwnode, iter))
+			endpoint->id++;
+	} else {
+		fwnode_property_read_u32(port_fwnode, "reg", &endpoint->port);
+		fwnode_property_read_u32(fwnode, "reg", &endpoint->id);
+	}
+
+	fwnode_handle_put(port_fwnode);
+
+	return 0;
+}
+EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 8bd28ce..cb60f29 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -27,4 +27,10 @@ struct fwnode_handle {
 	struct fwnode_handle *secondary;
 };
 
+struct fwnode_endpoint {
+	unsigned int port;
+	unsigned int id;
+	const struct fwnode_handle *local_fwnode;
+};
+
 #endif
diff --git a/include/linux/property.h b/include/linux/property.h
index 0b61ea4..45ef00a 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -284,4 +284,7 @@ struct fwnode_handle *fwnode_graph_get_remote_port(
 struct fwnode_handle *fwnode_graph_get_remote_endpoint(
 	struct fwnode_handle *fwnode);
 
+int fwnode_graph_parse_endpoint(struct fwnode_handle *fwnode,
+				struct fwnode_endpoint *endpoint);
+
 #endif /* _LINUX_PROPERTY_H_ */
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 14/16] of: Add nop implementation of of_get_next_parent()

[Sakari Ailus]

To avoid #ifdefs where the function is used.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 include/linux/of.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index d48e225..c05fe25 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -586,6 +586,12 @@ static inline struct device_node *of_get_parent(const struct device_node *node)
 	return NULL;
 }
 
+static inline struct device_node *of_get_next_parent(
+	const struct device_node *node)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_get_next_child(
 	const struct device_node *node, struct device_node *prev)
 {
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 15/16] device property: Add fwnode_get_next_parent()

[Sakari Ailus]

In order to differentiate the functionality between dropping a reference
to the node (or not) for the benefit of OF, introduce
fwnode_get_next_parent().

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/base/property.c  | 29 +++++++++++++++++++++++++++++
 include/linux/property.h |  1 +
 2 files changed, 30 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index b8c1017..defeba9 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -953,6 +953,35 @@ int device_add_properties(struct device *dev,
 EXPORT_SYMBOL_GPL(device_add_properties);
 
 /**
+ * fwnode_get_next_parent - Iterate to the node's parent
+ * @fwnode: Firmware whose parent is retrieved
+ *
+ * This is like fwnode_get_parent() except that it drops the refcount
+ * on the passed node, making it suitable for iterating through a
+ * node's parents.
+ *
+ * Returns a node pointer with refcount incremented, use
+ * fwnode_handle_node() on it when done.
+ */
+struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *parent = NULL;
+
+	if (is_of_node(fwnode)) {
+		struct device_node *node;
+
+		node = of_get_next_parent(to_of_node(fwnode));
+		if (node)
+			parent = &node->fwnode;
+	} else if (is_acpi_node(fwnode)) {
+		parent = acpi_node_get_parent(fwnode);
+	}
+
+	return parent;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_next_parent);
+
+/**
  * fwnode_get_parent - Return parent firwmare node
  * @fwnode: Firmware whose parent is retrieved
  *
diff --git a/include/linux/property.h b/include/linux/property.h
index 45ef00a..19dd35e 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -77,6 +77,7 @@ int fwnode_property_match_string(struct fwnode_handle *fwnode,
 				 const char *propname, const char *string);
 
 struct fwnode_handle *fwnode_get_parent(struct fwnode_handle *fwnode);
+struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_child_node(struct fwnode_handle *fwnode,
 						 struct fwnode_handle *child);
 
-- 
2.7.4

[PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Sakari Ailus]

Document the use of references into the hierarchical data extension
structure, as well as the use of port and endpoint concepts that are very
similar to those in Devicetree.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 Documentation/acpi/dsd/graph.txt | 164 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 164 insertions(+)
 create mode 100644 Documentation/acpi/dsd/graph.txt

diff --git a/Documentation/acpi/dsd/graph.txt b/Documentation/acpi/dsd/graph.txt
new file mode 100644
index 0000000..6195936
--- /dev/null
+++ b/Documentation/acpi/dsd/graph.txt
@@ -0,0 +1,164 @@
+Graphs
+
+
+_DSD
+----
+
+_DSD (Device Specific Data) [7] is a predefined ACPI device
+configuration object that can be used to convey information on
+hardware features which are not specifically covered by the ACPI
+specification [1][6]. There are two _DSD extensions that are relevant
+for graphs: property [4] and hierarchical data extensions [5]. The
+property extension provides generic key-value pairs whereas the
+hierarchical data extension supports nodes with references to other
+nodes, forming a tree. The nodes in the tree may contain properties as
+defined by the property extension. The two extensions together provide
+a tree-like structure with zero or more properties (key-value pairs)
+in each node of the tree.
+
+The data structure may be accessed at runtime by using the device_*
+and fwnode_* functions defined in include/linux/fwnode.h .
+
+Fwnode represents a generic firmware node object. It is independent on
+the firmware type. In ACPI, fwnodes are _DSD hierarchical data
+extensions objects. A device's _DSD object is represented by an
+fwnode.
+
+The data structure may be referenced to elsewhere in the ACPI tables
+by using a hard reference to the device itself and an index to the
+hierarchical data extension array on each depth.
+
+
+Ports and endpoints
+-------------------
+
+The port and endpoint concepts are very similar to those in Devicetree
+[3]. The port represent represent interface in a device, and an
+endpoint represents a connection to that interface.
+
+All port nodes are located under the device's "_DSD" node in
+hierarchical data extension tree. The first package list entry of the
+hierarchical data extension related to each port node must begin with
+"port" string and must be followed by the number of the port.
+
+Further on, endpoints are located under the individual port nodes. The
+first hierarchical data extension package list entry of the endpoint
+nodes must begin with "endpoint" and must be followed by the number
+of the endpoint.
+
+Each port node contains a property extension key "port", the value of
+which is the number of the port node. The number of the endpoint node is
+the index of the endpoint node in the endpoint node array under the port
+node, starting from 0.
+
+The endpoint reference uses property extension with "remote-endpoint"
+property name followed by a reference in the same package. Such references
+consist of the name of the device, index of the port node and finally the
+index of the endpoint node. The port index is indeed index to the referred
+port array, not the number of the port that is related to numbering of
+actual hardware interfaces in the respective hardware. The endpoint nodes
+are always referred to using an index to the endpoint node array.
+Individual references thus appear as:
+
+    Package() { device, port_node_index, endpoint_node_index }
+
+The references to endpoints must be always done both ways, to the
+remote endpoint and back from the referred remote endpoint node.
+
+A simple example of this is show below:
+
+    Scope (\_SB.PCI0.I2C2)
+    {
+	Device (CAM0)
+	{
+	    Name (_DSD, Package () {
+		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		Package () {
+		    Package () { "compatible", Package () { "nokia,smia" } },
+		},
+		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+		Package () {
+		    Package () { "port0", "PRT0" },
+		}
+	    })
+	    Name (PRT0, Package() {
+		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		Package () {
+		    Package () { "port", 0 },
+		},
+		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+		Package () {
+		    Package () { "endpoint0", "EP0" },
+		}
+	    })
+	    Name (EP0, Package() {
+		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		Package () {
+		    Package () { "remote-endpoint", Package() { \_SB.PCI0.ISP, 0, 0 } },
+		}
+	    })
+	}
+    }
+
+    Scope (\_SB.PCI0)
+    {
+	Device (ISP)
+	{
+	    Name (_DSD, Package () {
+		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+		Package () {
+		    Package () { "port4", "PRT4" },
+		}
+	    })
+
+	    Name (PRT4, Package() {
+		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		Package () {
+		    Package () { "port", 4 }, /* CSI-2 port number */
+		},
+		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+		Package () {
+		    Package () { "endpoint0", "EP0" },
+		}
+	    })
+
+	    Name (EP0, Package() {
+		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+		Package () {
+		    Package () { "remote-endpoint", Package () { \_SB.PCI0.I2C2.CAM0, 0, 0 } },
+		}
+	    })
+	}
+    }
+
+Here, the port 0 of the "CAM0" device is connected to the port 4 of
+the "ISP" device. Note that the port index in the reference is still
+0, as it refers to an entry in the table and not the port node number
+itself.
+
+
+References
+----------
+
+[1] _DSD (Device Specific Data) Implementation Guide.
+    <URL:http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel-1_1.htm>,
+    referenced 2016-10-03.
+
+[2] Devicetree. <URL:http://www.devicetree.org>, referenced 2016-10-03.
+
+[3] Documentation/devicetree/bindings/graph.txt
+
+[4] Device Properties UUID For _DSD.
+    <URL:http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf>,
+    referenced 2016-10-04.
+
+[5] Hierarchical Data Extension UUID For _DSD.
+    <URL:http://www.uefi.org/sites/default/files/resources/_DSD-hierarchical-data-extension-UUID-v1.pdf>,
+    referenced 2016-10-04.
+
+[6] Advanced Configuration and Power Interface Specification.
+    <URL:http://www.uefi.org/sites/default/files/resources/ACPI_6_1.pdf>,
+    referenced 2016-10-04.
+
+[7] _DSD Device Properties Usage Rules.
+    Documentation/acpi/DSD-properties-rules.txt
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 00/16] ACPI graph support

[Sakari Ailus]

Hi Rafael,

On Mon, Mar 06, 2017 at 04:19:14PM +0200, Sakari Ailus wrote:
> This set contains patches written by Mika Westerberg and by myself. The 
> patchset brings support for graphs to ACPI. The functionality achieved by 
> these patches is very similar to what the Device tree provides: the port
> and the endpoint concept are being employed. The patches make use of the
> _DSD property and data extensions to achieve this. The fwnode interface is
> extended by graph functionality; this way graph information originating 
> from both OF and ACPI may be accessed using the same interface, without 
> being aware of the underlying firmware interface. 

I've tested the patches again on ARM (OF) and x86-64 (ACPI).

I have V4L2 patches depending on the ACPI graph patchset that I'd like to
have in during the same merge window. Would you either accept a pull
request, or could I have a branch on top of v4.11-rc1 containing these
patches only (for media tree)?

The patchset "Move firmware specific code to firmware specific locations"
depends on "Fwnode property API fixes for OF, pset". Would you prefer to
send the bugfixes as fixes to v4.11 or just put it all to v4.12?

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 00/16] ACPI graph support

[Rafael J. Wysocki]

On Tue, Mar 7, 2017 at 8:49 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi Rafael,
>
> On Mon, Mar 06, 2017 at 04:19:14PM +0200, Sakari Ailus wrote:
>> This set contains patches written by Mika Westerberg and by myself. The
>> patchset brings support for graphs to ACPI. The functionality achieved by
>> these patches is very similar to what the Device tree provides: the port
>> and the endpoint concept are being employed. The patches make use of the
>> _DSD property and data extensions to achieve this. The fwnode interface is
>> extended by graph functionality; this way graph information originating
>> from both OF and ACPI may be accessed using the same interface, without
>> being aware of the underlying firmware interface.
>
> I've tested the patches again on ARM (OF) and x86-64 (ACPI).

OK

> I have V4L2 patches depending on the ACPI graph patchset that I'd like to
> have in during the same merge window. Would you either accept a pull
> request, or could I have a branch on top of v4.11-rc1 containing these
> patches only (for media tree)?
>
> The patchset "Move firmware specific code to firmware specific locations"
> depends on "Fwnode property API fixes for OF, pset". Would you prefer to
> send the bugfixes as fixes to v4.11 or just put it all to v4.12?

I'll put it all into v4.12 if that's not a problem.

Also I don't really have the time to take care of that this week due
to other work.  I'll get to it next week, most likely.

Thanks,
Rafael

Re: [PATCH v4 00/16] ACPI graph support

[Sakari Ailus]

Hi Rafael,

On Tue, Mar 07, 2017 at 02:23:33PM +0100, Rafael J. Wysocki wrote:
> On Tue, Mar 7, 2017 at 8:49 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > Hi Rafael,
> >
> > On Mon, Mar 06, 2017 at 04:19:14PM +0200, Sakari Ailus wrote:
> >> This set contains patches written by Mika Westerberg and by myself. The
> >> patchset brings support for graphs to ACPI. The functionality achieved by
> >> these patches is very similar to what the Device tree provides: the port
> >> and the endpoint concept are being employed. The patches make use of the
> >> _DSD property and data extensions to achieve this. The fwnode interface is
> >> extended by graph functionality; this way graph information originating
> >> from both OF and ACPI may be accessed using the same interface, without
> >> being aware of the underlying firmware interface.
> >
> > I've tested the patches again on ARM (OF) and x86-64 (ACPI).
> 
> OK
> 
> > I have V4L2 patches depending on the ACPI graph patchset that I'd like to
> > have in during the same merge window. Would you either accept a pull
> > request, or could I have a branch on top of v4.11-rc1 containing these
> > patches only (for media tree)?
> >
> > The patchset "Move firmware specific code to firmware specific locations"
> > depends on "Fwnode property API fixes for OF, pset". Would you prefer to
> > send the bugfixes as fixes to v4.11 or just put it all to v4.12?
> 
> I'll put it all into v4.12 if that's not a problem.

That's fine for me but see below...

> 
> Also I don't really have the time to take care of that this week due
> to other work.  I'll get to it next week, most likely.

Ack.

Would you prefer me to send a pull request, or pick up the patches --- in
the latter case I'd like to request an immutable branch with these patches
only on v4.11-rc1 which I could use for basing my dependent V4L2 patches
that go to the media tree.

Thanks.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v4 00/16] ACPI graph support

[Rafael J. Wysocki]

On Tuesday, March 07, 2017 03:49:36 PM Sakari Ailus wrote:
> Hi Rafael,

Hi,

> On Tue, Mar 07, 2017 at 02:23:33PM +0100, Rafael J. Wysocki wrote:
> > On Tue, Mar 7, 2017 at 8:49 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > Hi Rafael,
> > >
> > > On Mon, Mar 06, 2017 at 04:19:14PM +0200, Sakari Ailus wrote:
> > >> This set contains patches written by Mika Westerberg and by myself. The
> > >> patchset brings support for graphs to ACPI. The functionality achieved by
> > >> these patches is very similar to what the Device tree provides: the port
> > >> and the endpoint concept are being employed. The patches make use of the
> > >> _DSD property and data extensions to achieve this. The fwnode interface is
> > >> extended by graph functionality; this way graph information originating
> > >> from both OF and ACPI may be accessed using the same interface, without
> > >> being aware of the underlying firmware interface.
> > >
> > > I've tested the patches again on ARM (OF) and x86-64 (ACPI).
> > 
> > OK
> > 
> > > I have V4L2 patches depending on the ACPI graph patchset that I'd like to
> > > have in during the same merge window. Would you either accept a pull
> > > request, or could I have a branch on top of v4.11-rc1 containing these
> > > patches only (for media tree)?
> > >
> > > The patchset "Move firmware specific code to firmware specific locations"
> > > depends on "Fwnode property API fixes for OF, pset". Would you prefer to
> > > send the bugfixes as fixes to v4.11 or just put it all to v4.12?
> > 
> > I'll put it all into v4.12 if that's not a problem.
> 
> That's fine for me but see below...
> 
> > 
> > Also I don't really have the time to take care of that this week due
> > to other work.  I'll get to it next week, most likely.
> 
> Ack.
> 
> Would you prefer me to send a pull request, or pick up the patches --- in
> the latter case I'd like to request an immutable branch with these patches
> only on v4.11-rc1 which I could use for basing my dependent V4L2 patches
> that go to the media tree.

So from this series I can queue up patches [1-7/16] no problem.

Patch [8/16] requires an ACK from Rob for me to take it.

Patch [9/16] is not even related to this series technically, so please send it
separately to Greg.

Patch [10/16] needs to be folded into the one that depends on it IMO.

I also have questions to patches [11-16/16], but I'll respond to those
separately later.

Thanks,
Rafael

Re: [PATCH v4 00/16] ACPI graph support

[Sakari Ailus]

Hi Rafael,

On Fri, Mar 10, 2017 at 12:05:04AM +0100, Rafael J. Wysocki wrote:
> On Tuesday, March 07, 2017 03:49:36 PM Sakari Ailus wrote:
> > Hi Rafael,
> 
> Hi,
> 
> > On Tue, Mar 07, 2017 at 02:23:33PM +0100, Rafael J. Wysocki wrote:
> > > On Tue, Mar 7, 2017 at 8:49 AM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > > Hi Rafael,
> > > >
> > > > On Mon, Mar 06, 2017 at 04:19:14PM +0200, Sakari Ailus wrote:
> > > >> This set contains patches written by Mika Westerberg and by myself. The
> > > >> patchset brings support for graphs to ACPI. The functionality achieved by
> > > >> these patches is very similar to what the Device tree provides: the port
> > > >> and the endpoint concept are being employed. The patches make use of the
> > > >> _DSD property and data extensions to achieve this. The fwnode interface is
> > > >> extended by graph functionality; this way graph information originating
> > > >> from both OF and ACPI may be accessed using the same interface, without
> > > >> being aware of the underlying firmware interface.
> > > >
> > > > I've tested the patches again on ARM (OF) and x86-64 (ACPI).
> > > 
> > > OK
> > > 
> > > > I have V4L2 patches depending on the ACPI graph patchset that I'd like to
> > > > have in during the same merge window. Would you either accept a pull
> > > > request, or could I have a branch on top of v4.11-rc1 containing these
> > > > patches only (for media tree)?
> > > >
> > > > The patchset "Move firmware specific code to firmware specific locations"
> > > > depends on "Fwnode property API fixes for OF, pset". Would you prefer to
> > > > send the bugfixes as fixes to v4.11 or just put it all to v4.12?
> > > 
> > > I'll put it all into v4.12 if that's not a problem.
> > 
> > That's fine for me but see below...
> > 
> > > 
> > > Also I don't really have the time to take care of that this week due
> > > to other work.  I'll get to it next week, most likely.
> > 
> > Ack.
> > 
> > Would you prefer me to send a pull request, or pick up the patches --- in
> > the latter case I'd like to request an immutable branch with these patches
> > only on v4.11-rc1 which I could use for basing my dependent V4L2 patches
> > that go to the media tree.
> 
> So from this series I can queue up patches [1-7/16] no problem.
> 
> Patch [8/16] requires an ACK from Rob for me to take it.
> 
> Patch [9/16] is not even related to this series technically, so please send it
> separately to Greg.

I'll drop that patch from the set in this case and submit another patch
later on --- for me it was a question of where do you add a header file when
you see a randomly ordered list with some entries there twice. ;-)

> 
> Patch [10/16] needs to be folded into the one that depends on it IMO.

Will do.

> 
> I also have questions to patches [11-16/16], but I'll respond to those
> separately later.

Ack.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v4 10/16] irqchip/gic: Add missing forward declaration for struct device

[Rafael J. Wysocki]

On Monday, March 06, 2017 04:19:24 PM Sakari Ailus wrote:
> A pointer to struct device is used in the header as a function argument
> but there's no declaration for it. Add one.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/linux/irqchip/arm-gic.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
> index eafc965..b16afa1 100644
> --- a/include/linux/irqchip/arm-gic.h
> +++ b/include/linux/irqchip/arm-gic.h
> @@ -100,6 +100,7 @@
>  
>  #include <linux/irqdomain.h>
>  
> +struct device;
>  struct device_node;
>  struct gic_chip_data;

That should be folded into the patch that really depends on it and then it
would require an ACK from the maintainer of arm-gic.h.

Thanks,
Rafael

Re: [PATCH v4 11/16] of: No need to include linux/property.h, linux/fwnode.h is sufficient

[Rafael J. Wysocki]

On Monday, March 06, 2017 04:19:25 PM Sakari Ailus wrote:
> of.h requires a definition of struct fwnode_handle, and for that it
> includes linux/property.h. struct fwnode_handle, however, is defined in
> linux/fwnode.h. Include linux/fwnode.h directly.
> 
> A number of users were however depending on linux/property.h, thus fix
> them by including that header directly as well.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

How exactly is this related to the rest of the series?  Do any other patches in
the series depend on it?

Thanks,
Rafael


> ---
>  drivers/base/core.c                       | 1 +
>  drivers/input/keyboard/gpio_keys.c        | 1 +
>  drivers/input/misc/drv260x.c              | 1 +
>  drivers/input/misc/gpio_decoder.c         | 1 +
>  drivers/input/touchscreen/ad7879.c        | 1 +
>  drivers/input/touchscreen/edt-ft5x06.c    | 1 +
>  drivers/input/touchscreen/tsc200x-core.c  | 2 +-
>  drivers/net/ethernet/faraday/ftgmac100.c  | 1 +
>  drivers/net/ethernet/smsc/smc91x.c        | 1 +
>  drivers/phy/phy-tusb1210.c                | 1 +
>  drivers/power/supply/bq24735-charger.c    | 1 +
>  drivers/usb/common/common.c               | 1 +
>  drivers/usb/dwc2/debugfs.c                | 3 ++-
>  drivers/usb/dwc2/params.c                 | 1 +
>  drivers/usb/dwc2/pci.c                    | 3 ++-
>  drivers/usb/dwc3/host.c                   | 1 +
>  include/linux/of.h                        | 2 +-
>  sound/soc/codecs/rt5514.c                 | 1 +
>  sound/soc/codecs/ts3a227e.c               | 1 +
>  sound/soc/mediatek/mt8173/mt8173-rt5650.c | 1 +
>  sound/soc/rockchip/rk3399_gru_sound.c     | 1 +
>  21 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index fde2c7c..92e7d80 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -24,6 +24,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/property.h>
>  #include <linux/sched/signal.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index da3d362..a5509aa 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -20,6 +20,7 @@
>  #include <linux/slab.h>
>  #include <linux/sysctl.h>
>  #include <linux/proc_fs.h>
> +#include <linux/property.h>
>  #include <linux/delay.h>
>  #include <linux/platform_device.h>
>  #include <linux/input.h>
> diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
> index fb089d3..a3972df 100644
> --- a/drivers/input/misc/drv260x.c
> +++ b/drivers/input/misc/drv260x.c
> @@ -22,6 +22,7 @@
>  #include <linux/slab.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/property.h>
>  #include <linux/regulator/consumer.h>
>  
>  #include <dt-bindings/input/ti-drv260x.h>
> diff --git a/drivers/input/misc/gpio_decoder.c b/drivers/input/misc/gpio_decoder.c
> index 1dca526..10bc109 100644
> --- a/drivers/input/misc/gpio_decoder.c
> +++ b/drivers/input/misc/gpio_decoder.c
> @@ -22,6 +22,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  
>  struct gpio_decoder {
>  	struct input_polled_dev *poll_dev;
> diff --git a/drivers/input/touchscreen/ad7879.c b/drivers/input/touchscreen/ad7879.c
> index e16a446..38ad365 100644
> --- a/drivers/input/touchscreen/ad7879.c
> +++ b/drivers/input/touchscreen/ad7879.c
> @@ -34,6 +34,7 @@
>  #include <linux/input/touchscreen.h>
>  #include <linux/platform_data/ad7879.h>
>  #include <linux/module.h>
> +#include <linux/property.h>
>  #include "ad7879.h"
>  
>  #define AD7879_REG_ZEROS		0
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index 8cf8d8d..3beaea2 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -39,6 +39,7 @@
>  #include <linux/input/mt.h>
>  #include <linux/input/touchscreen.h>
>  #include <linux/of_device.h>
> +#include <linux/property.h>
>  
>  #define WORK_REGISTER_THRESHOLD		0x00
>  #define WORK_REGISTER_REPORT_RATE	0x08
> diff --git a/drivers/input/touchscreen/tsc200x-core.c b/drivers/input/touchscreen/tsc200x-core.c
> index 88ea5e1..3964d67 100644
> --- a/drivers/input/touchscreen/tsc200x-core.c
> +++ b/drivers/input/touchscreen/tsc200x-core.c
> @@ -25,10 +25,10 @@
>  #include <linux/input/touchscreen.h>
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
> -#include <linux/pm.h>
>  #include <linux/of.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/regmap.h>
> +#include <linux/property.h>
>  #include <linux/gpio/consumer.h>
>  #include "tsc200x-core.h"
>  
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 928b0df..f3eaa5f 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -30,6 +30,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <net/ip.h>
>  #include <net/ncsi.h>
>  
> diff --git a/drivers/net/ethernet/smsc/smc91x.c b/drivers/net/ethernet/smsc/smc91x.c
> index 65077c7..5352a08 100644
> --- a/drivers/net/ethernet/smsc/smc91x.c
> +++ b/drivers/net/ethernet/smsc/smc91x.c
> @@ -75,6 +75,7 @@ static const char version[] =
>  #include <linux/ioport.h>
>  #include <linux/crc32.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/spinlock.h>
>  #include <linux/ethtool.h>
>  #include <linux/mii.h>
> diff --git a/drivers/phy/phy-tusb1210.c b/drivers/phy/phy-tusb1210.c
> index 4f6d5e7..f7dd21aa 100644
> --- a/drivers/phy/phy-tusb1210.c
> +++ b/drivers/phy/phy-tusb1210.c
> @@ -12,6 +12,7 @@
>  #include <linux/module.h>
>  #include <linux/ulpi/driver.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/property.h>
>  
>  #include "ulpi_phy.h"
>  
> diff --git a/drivers/power/supply/bq24735-charger.c b/drivers/power/supply/bq24735-charger.c
> index eb01453..690bb4d 100644
> --- a/drivers/power/supply/bq24735-charger.c
> +++ b/drivers/power/supply/bq24735-charger.c
> @@ -27,6 +27,7 @@
>  #include <linux/of.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/power_supply.h>
> +#include <linux/property.h>
>  #include <linux/slab.h>
>  
>  #include <linux/power/bq24735-charger.h>
> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> index 5ef8da6..534a498 100644
> --- a/drivers/usb/common/common.c
> +++ b/drivers/usb/common/common.c
> @@ -14,6 +14,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/property.h>
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/of.h>
>  #include <linux/usb/otg.h>
> diff --git a/drivers/usb/dwc2/debugfs.c b/drivers/usb/dwc2/debugfs.c
> index 794b959..10217cd 100644
> --- a/drivers/usb/dwc2/debugfs.c
> +++ b/drivers/usb/dwc2/debugfs.c
> @@ -14,9 +14,10 @@
>   * GNU General Public License for more details.
>   */
>  
> -#include <linux/spinlock.h>
>  #include <linux/debugfs.h>
> +#include <linux/property.h>
>  #include <linux/seq_file.h>
> +#include <linux/spinlock.h>
>  #include <linux/uaccess.h>
>  
>  #include "core.h"
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 2990c34..0fe39ce 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -35,6 +35,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/property.h>
>  
>  #include "core.h"
>  
> diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c
> index fdeb8c7..333a5cb 100644
> --- a/drivers/usb/dwc2/pci.c
> +++ b/drivers/usb/dwc2/pci.c
> @@ -44,8 +44,9 @@
>  #include <linux/spinlock.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> -#include <linux/slab.h>
>  #include <linux/pci.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
>  #include <linux/usb.h>
>  
>  #include <linux/usb/hcd.h>
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index 76f0b0d..563b1df 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -16,6 +16,7 @@
>   */
>  
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  
>  #include "core.h"
>  
> diff --git a/include/linux/of.h b/include/linux/of.h
> index e5d4225f..d48e225 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -23,7 +23,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/topology.h>
>  #include <linux/notifier.h>
> -#include <linux/property.h>
> +#include <linux/fwnode.h>
>  #include <linux/list.h>
>  
>  #include <asm/byteorder.h>
> diff --git a/sound/soc/codecs/rt5514.c b/sound/soc/codecs/rt5514.c
> index b281a46..0749219 100644
> --- a/sound/soc/codecs/rt5514.c
> +++ b/sound/soc/codecs/rt5514.c
> @@ -15,6 +15,7 @@
>  #include <linux/init.h>
>  #include <linux/delay.h>
>  #include <linux/pm.h>
> +#include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/i2c.h>
>  #include <linux/platform_device.h>
> diff --git a/sound/soc/codecs/ts3a227e.c b/sound/soc/codecs/ts3a227e.c
> index 4356843..e80b28b 100644
> --- a/sound/soc/codecs/ts3a227e.c
> +++ b/sound/soc/codecs/ts3a227e.c
> @@ -14,6 +14,7 @@
>  #include <linux/input.h>
>  #include <linux/module.h>
>  #include <linux/of_gpio.h>
> +#include <linux/property.h>
>  #include <linux/regmap.h>
>  
>  #include <sound/core.h>
> diff --git a/sound/soc/mediatek/mt8173/mt8173-rt5650.c b/sound/soc/mediatek/mt8173/mt8173-rt5650.c
> index ba65f41..433ae4f 100644
> --- a/sound/soc/mediatek/mt8173/mt8173-rt5650.c
> +++ b/sound/soc/mediatek/mt8173/mt8173-rt5650.c
> @@ -17,6 +17,7 @@
>  #include <linux/module.h>
>  #include <linux/gpio.h>
>  #include <linux/of_gpio.h>
> +#include <linux/property.h>
>  #include <sound/soc.h>
>  #include <sound/jack.h>
>  #include "../../codecs/rt5645.h"
> diff --git a/sound/soc/rockchip/rk3399_gru_sound.c b/sound/soc/rockchip/rk3399_gru_sound.c
> index 3475c61..9134b46 100644
> --- a/sound/soc/rockchip/rk3399_gru_sound.c
> +++ b/sound/soc/rockchip/rk3399_gru_sound.c
> @@ -22,6 +22,7 @@
>  #include <linux/gpio.h>
>  #include <linux/of_gpio.h>
>  #include <linux/delay.h>
> +#include <linux/property.h>
>  #include <linux/spi/spi.h>
>  #include <linux/input.h>
>  #include <sound/core.h>
> 

Re: [PATCH v4 12/16] device property: Move dev_fwnode() to linux/property.h

[Rafael J. Wysocki]

On Monday, March 06, 2017 04:19:26 PM Sakari Ailus wrote:
> The function to obtain a fwnode related to a struct device is useful for
> drivers that use the fwnode property API: it allows not being aware of the
> underlying firmware implementation.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/base/property.c  |  6 ------
>  include/linux/property.h | 10 ++++++++--
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 111049b..860e160 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -183,12 +183,6 @@ static int pset_prop_read_string(struct property_set *pset,
>  	return 0;
>  }
>  
> -static inline struct fwnode_handle *dev_fwnode(struct device *dev)
> -{
> -	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
> -		&dev->of_node->fwnode : dev->fwnode;
> -}

What about making it non-inline instead?

> -
>  /**
>   * device_property_present - check if a property of a device is present
>   * @dev: Device whose property is being checked
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 0ae7d20..0b61ea4 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -13,11 +13,11 @@
>  #ifndef _LINUX_PROPERTY_H_
>  #define _LINUX_PROPERTY_H_
>  
> +#include <linux/device.h>
>  #include <linux/fwnode.h>
> +#include <linux/of.h>
>  #include <linux/types.h>

You would not need to add the above includes here then.

Thanks,
Rafael

Re: [PATCH v4 13/16] device property: Add support for fwnode endpoints

[Rafael J. Wysocki]

On Monday, March 06, 2017 04:19:27 PM Sakari Ailus wrote:
> Similar to OF endpoints, endpoint type nodes can be also supported on
> ACPI. In order to make it possible for drivers to ignore the matter,
> add a type for fwnode_endpoint and a function to parse them.
> 
> On ACPI, find the child node index instead of relying on the "endpoint"
> property.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/base/property.c  | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/fwnode.h   |  6 ++++++
>  include/linux/property.h |  3 +++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 860e160..b8c1017 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1330,3 +1330,39 @@ fwnode_graph_get_remote_endpoint(struct fwnode_handle *fwnode)
>  	return endpoint;
>  }
>  EXPORT_SYMBOL_GPL(fwnode_graph_get_remote_endpoint);
> +
> +/*

That should be /** I gather?

> + * fwnode_graph_parse_endpoint() - parse common endpoint node properties

The parens at the end of the function name above are not necessary.

> + * @node: pointer to endpoint device_node
> + * @endpoint: pointer to the fwnode endpoint data structure
> + *
> + * The caller should hold a reference to @node.

A few words about what the function does, please.

> + */
> +int fwnode_graph_parse_endpoint(struct fwnode_handle *fwnode,
> +				struct fwnode_endpoint *endpoint)
> +{
> +	struct fwnode_handle *port_fwnode = fwnode_get_parent(fwnode);
> +
> +	memset(endpoint, 0, sizeof(*endpoint));
> +
> +	endpoint->local_fwnode = fwnode;
> +
> +	if (is_acpi_node(port_fwnode)) {
> +		struct fwnode_handle *iter;
> +
> +		fwnode_property_read_u32(port_fwnode, "port", &endpoint->port);
> +
> +		for (iter = fwnode_get_next_child_node(port_fwnode, NULL);
> +		     iter != fwnode;
> +		     iter = fwnode_get_next_child_node(port_fwnode, iter))
> +			endpoint->id++;
> +	} else {
> +		fwnode_property_read_u32(port_fwnode, "reg", &endpoint->port);
> +		fwnode_property_read_u32(fwnode, "reg", &endpoint->id);
> +	}
> +
> +	fwnode_handle_put(port_fwnode);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 8bd28ce..cb60f29 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -27,4 +27,10 @@ struct fwnode_handle {
>  	struct fwnode_handle *secondary;
>  };
>  
> +struct fwnode_endpoint {
> +	unsigned int port;
> +	unsigned int id;
> +	const struct fwnode_handle *local_fwnode;
> +};

Any kerneldoc?

> +
>  #endif
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 0b61ea4..45ef00a 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -284,4 +284,7 @@ struct fwnode_handle *fwnode_graph_get_remote_port(
>  struct fwnode_handle *fwnode_graph_get_remote_endpoint(
>  	struct fwnode_handle *fwnode);
>  
> +int fwnode_graph_parse_endpoint(struct fwnode_handle *fwnode,
> +				struct fwnode_endpoint *endpoint);
> +
>  #endif /* _LINUX_PROPERTY_H_ */
> 

Thanks,
Rafael

Re: [PATCH v4 14/16] of: Add nop implementation of of_get_next_parent()

[Rafael J. Wysocki]

On Monday, March 06, 2017 04:19:28 PM Sakari Ailus wrote:
> To avoid #ifdefs where the function is used.

A bit more explanation?

About where this happens (or will happen) to be used with CONFIG_OF unset?

Plus, that would require an ACK from Rob.

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/linux/of.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index d48e225..c05fe25 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -586,6 +586,12 @@ static inline struct device_node *of_get_parent(const struct device_node *node)
>  	return NULL;
>  }
>  
> +static inline struct device_node *of_get_next_parent(
> +	const struct device_node *node)
> +{
> +	return NULL;
> +}
> +
>  static inline struct device_node *of_get_next_child(
>  	const struct device_node *node, struct device_node *prev)
>  {
> 

Thanks,
Rafael

Re: [PATCH v4 15/16] device property: Add fwnode_get_next_parent()

[Rafael J. Wysocki]

On Monday, March 06, 2017 04:19:29 PM Sakari Ailus wrote:
> In order to differentiate the functionality between dropping a reference
> to the node (or not) for the benefit of OF, introduce
> fwnode_get_next_parent().
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/base/property.c  | 29 +++++++++++++++++++++++++++++
>  include/linux/property.h |  1 +
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index b8c1017..defeba9 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -953,6 +953,35 @@ int device_add_properties(struct device *dev,
>  EXPORT_SYMBOL_GPL(device_add_properties);
>  
>  /**
> + * fwnode_get_next_parent - Iterate to the node's parent
> + * @fwnode: Firmware whose parent is retrieved
> + *
> + * This is like fwnode_get_parent() except that it drops the refcount
> + * on the passed node, making it suitable for iterating through a
> + * node's parents.
> + *
> + * Returns a node pointer with refcount incremented, use
> + * fwnode_handle_node() on it when done.
> + */
> +struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode)
> +{
> +	struct fwnode_handle *parent = NULL;
> +
> +	if (is_of_node(fwnode)) {
> +		struct device_node *node;
> +
> +		node = of_get_next_parent(to_of_node(fwnode));

So I would fold the [14/15] into this one and then it should be clear why that
change was useful.

> +		if (node)
> +			parent = &node->fwnode;
> +	} else if (is_acpi_node(fwnode)) {
> +		parent = acpi_node_get_parent(fwnode);
> +	}
> +
> +	return parent;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_get_next_parent);
> +
> +/**
>   * fwnode_get_parent - Return parent firwmare node
>   * @fwnode: Firmware whose parent is retrieved
>   *
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 45ef00a..19dd35e 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -77,6 +77,7 @@ int fwnode_property_match_string(struct fwnode_handle *fwnode,
>  				 const char *propname, const char *string);
>  
>  struct fwnode_handle *fwnode_get_parent(struct fwnode_handle *fwnode);
> +struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode);
>  struct fwnode_handle *fwnode_get_next_child_node(struct fwnode_handle *fwnode,
>  						 struct fwnode_handle *child);
>  
> 

Thanks,
Rafael

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Rafael J. Wysocki]

On Monday, March 06, 2017 04:19:30 PM Sakari Ailus wrote:
> Document the use of references into the hierarchical data extension
> structure, as well as the use of port and endpoint concepts that are very
> similar to those in Devicetree.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/acpi/dsd/graph.txt | 164 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 164 insertions(+)
>  create mode 100644 Documentation/acpi/dsd/graph.txt
> 
> diff --git a/Documentation/acpi/dsd/graph.txt b/Documentation/acpi/dsd/graph.txt
> new file mode 100644
> index 0000000..6195936
> --- /dev/null
> +++ b/Documentation/acpi/dsd/graph.txt
> @@ -0,0 +1,164 @@
> +Graphs
> +
> +
> +_DSD
> +----
> +
> +_DSD (Device Specific Data) [7] is a predefined ACPI device
> +configuration object that can be used to convey information on
> +hardware features which are not specifically covered by the ACPI
> +specification [1][6]. There are two _DSD extensions that are relevant
> +for graphs: property [4] and hierarchical data extensions [5]. The
> +property extension provides generic key-value pairs whereas the
> +hierarchical data extension supports nodes with references to other
> +nodes, forming a tree. The nodes in the tree may contain properties as
> +defined by the property extension. The two extensions together provide
> +a tree-like structure with zero or more properties (key-value pairs)
> +in each node of the tree.
> +
> +The data structure may be accessed at runtime by using the device_*
> +and fwnode_* functions defined in include/linux/fwnode.h .
> +
> +Fwnode represents a generic firmware node object. It is independent on
> +the firmware type. In ACPI, fwnodes are _DSD hierarchical data
> +extensions objects. A device's _DSD object is represented by an
> +fwnode.
> +
> +The data structure may be referenced to elsewhere in the ACPI tables
> +by using a hard reference to the device itself and an index to the
> +hierarchical data extension array on each depth.
> +
> +
> +Ports and endpoints
> +-------------------
> +
> +The port and endpoint concepts are very similar to those in Devicetree
> +[3]. The port represent represent interface in a device, and an

s/represent represent/represents/

Plus I would say "a port" and "an interface".

> +endpoint represents a connection to that interface.
> +
> +All port nodes are located under the device's "_DSD" node in
> +hierarchical data extension tree. The first package list entry of the
> +hierarchical data extension related to each port node must begin with
> +"port" string and must be followed by the number of the port.

So "port" should be the key, right?

> +
> +Further on, endpoints are located under the individual port nodes. The
> +first hierarchical data extension package list entry of the endpoint
> +nodes must begin with "endpoint" and must be followed by the number
> +of the endpoint.
> +
> +Each port node contains a property extension key "port", the value of
> +which is the number of the port node. The number of the endpoint node is
> +the index of the endpoint node in the endpoint node array under the port
> +node, starting from 0.
> +
> +The endpoint reference uses property extension with "remote-endpoint"
> +property name followed by a reference in the same package. Such references
> +consist of the name of the device, index of the port node and finally the
> +index of the endpoint node. The port index is indeed index to the referred
> +port array, not the number of the port that is related to numbering of
> +actual hardware interfaces in the respective hardware. The endpoint nodes
> +are always referred to using an index to the endpoint node array.
> +Individual references thus appear as:
> +
> +    Package() { device, port_node_index, endpoint_node_index }
> +
> +The references to endpoints must be always done both ways, to the
> +remote endpoint and back from the referred remote endpoint node.
> +
> +A simple example of this is show below:
> +
> +    Scope (\_SB.PCI0.I2C2)
> +    {
> +	Device (CAM0)
> +	{
> +	    Name (_DSD, Package () {
> +		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> +		Package () {
> +		    Package () { "compatible", Package () { "nokia,smia" } },
> +		},
> +		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> +		Package () {
> +		    Package () { "port0", "PRT0" },
> +		}
> +	    })
> +	    Name (PRT0, Package() {
> +		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> +		Package () {
> +		    Package () { "port", 0 },
> +		},
> +		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> +		Package () {
> +		    Package () { "endpoint0", "EP0" },
> +		}
> +	    })
> +	    Name (EP0, Package() {
> +		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> +		Package () {
> +		    Package () { "remote-endpoint", Package() { \_SB.PCI0.ISP, 0, 0 } },
> +		}
> +	    })
> +	}
> +    }
> +
> +    Scope (\_SB.PCI0)
> +    {
> +	Device (ISP)
> +	{
> +	    Name (_DSD, Package () {
> +		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> +		Package () {
> +		    Package () { "port4", "PRT4" },
> +		}
> +	    })
> +
> +	    Name (PRT4, Package() {
> +		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> +		Package () {
> +		    Package () { "port", 4 }, /* CSI-2 port number */
> +		},
> +		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> +		Package () {
> +		    Package () { "endpoint0", "EP0" },
> +		}
> +	    })
> +
> +	    Name (EP0, Package() {
> +		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> +		Package () {
> +		    Package () { "remote-endpoint", Package () { \_SB.PCI0.I2C2.CAM0, 0, 0 } },

Why indices and not the keys?  Something like

	Package () { "remote-endpoint", Package () { \_SB.PCI0.I2C2.CAM0, "port0", "endpoint0" } },

should work too and would be more flexible I suppose and less prone to errors.

> +		}
> +	    })
> +	}
> +    }
> +
> +Here, the port 0 of the "CAM0" device is connected to the port 4 of
> +the "ISP" device. Note that the port index in the reference is still
> +0, as it refers to an entry in the table and not the port node number
> +itself.

Thanks,
Rafael

Re: [PATCH v4 12/16] device property: Move dev_fwnode() to linux/property.h

[Sakari Ailus]

Hi Rafael,

On 03/13/17 23:49, Rafael J. Wysocki wrote:
> On Monday, March 06, 2017 04:19:26 PM Sakari Ailus wrote:
>> The function to obtain a fwnode related to a struct device is useful for
>> drivers that use the fwnode property API: it allows not being aware of the
>> underlying firmware implementation.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> ---
>>  drivers/base/property.c  |  6 ------
>>  include/linux/property.h | 10 ++++++++--
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>> index 111049b..860e160 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -183,12 +183,6 @@ static int pset_prop_read_string(struct property_set *pset,
>>  	return 0;
>>  }
>>  
>> -static inline struct fwnode_handle *dev_fwnode(struct device *dev)
>> -{
>> -	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
>> -		&dev->of_node->fwnode : dev->fwnode;
>> -}
> 
> What about making it non-inline instead?
> 
>> -
>>  /**
>>   * device_property_present - check if a property of a device is present
>>   * @dev: Device whose property is being checked
>> diff --git a/include/linux/property.h b/include/linux/property.h
>> index 0ae7d20..0b61ea4 100644
>> --- a/include/linux/property.h
>> +++ b/include/linux/property.h
>> @@ -13,11 +13,11 @@
>>  #ifndef _LINUX_PROPERTY_H_
>>  #define _LINUX_PROPERTY_H_
>>  
>> +#include <linux/device.h>
>>  #include <linux/fwnode.h>
>> +#include <linux/of.h>
>>  #include <linux/types.h>
> 
> You would not need to add the above includes here then.

Makes sense. I'll change it.

-- 
Regards,

Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 13/16] device property: Add support for fwnode endpoints

[Sakari Ailus]

Hi Rafael,

On 03/13/17 23:52, Rafael J. Wysocki wrote:
> On Monday, March 06, 2017 04:19:27 PM Sakari Ailus wrote:
>> Similar to OF endpoints, endpoint type nodes can be also supported on
>> ACPI. In order to make it possible for drivers to ignore the matter,
>> add a type for fwnode_endpoint and a function to parse them.
>>
>> On ACPI, find the child node index instead of relying on the "endpoint"
>> property.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>  drivers/base/property.c  | 36 ++++++++++++++++++++++++++++++++++++
>>  include/linux/fwnode.h   |  6 ++++++
>>  include/linux/property.h |  3 +++
>>  3 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>> index 860e160..b8c1017 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -1330,3 +1330,39 @@ fwnode_graph_get_remote_endpoint(struct fwnode_handle *fwnode)
>>  	return endpoint;
>>  }
>>  EXPORT_SYMBOL_GPL(fwnode_graph_get_remote_endpoint);
>> +
>> +/*
> 
> That should be /** I gather?

Indeed.

> 
>> + * fwnode_graph_parse_endpoint() - parse common endpoint node properties
> 
> The parens at the end of the function name above are not necessary.

I'll drop them.

> 
>> + * @node: pointer to endpoint device_node
>> + * @endpoint: pointer to the fwnode endpoint data structure
>> + *
>> + * The caller should hold a reference to @node.
> 
> A few words about what the function does, please.

I'll add that. Also "node" should be "fwnode", I think I changed the
argument name but not the documentation. "device_node" should be
"fwnode_handle", too.

> 
>> + */
>> +int fwnode_graph_parse_endpoint(struct fwnode_handle *fwnode,
>> +				struct fwnode_endpoint *endpoint)
>> +{
>> +	struct fwnode_handle *port_fwnode = fwnode_get_parent(fwnode);
>> +
>> +	memset(endpoint, 0, sizeof(*endpoint));
>> +
>> +	endpoint->local_fwnode = fwnode;
>> +
>> +	if (is_acpi_node(port_fwnode)) {
>> +		struct fwnode_handle *iter;
>> +
>> +		fwnode_property_read_u32(port_fwnode, "port", &endpoint->port);
>> +
>> +		for (iter = fwnode_get_next_child_node(port_fwnode, NULL);
>> +		     iter != fwnode;
>> +		     iter = fwnode_get_next_child_node(port_fwnode, iter))
>> +			endpoint->id++;
>> +	} else {
>> +		fwnode_property_read_u32(port_fwnode, "reg", &endpoint->port);
>> +		fwnode_property_read_u32(fwnode, "reg", &endpoint->id);
>> +	}
>> +
>> +	fwnode_handle_put(port_fwnode);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
>> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
>> index 8bd28ce..cb60f29 100644
>> --- a/include/linux/fwnode.h
>> +++ b/include/linux/fwnode.h
>> @@ -27,4 +27,10 @@ struct fwnode_handle {
>>  	struct fwnode_handle *secondary;
>>  };
>>  
>> +struct fwnode_endpoint {
>> +	unsigned int port;
>> +	unsigned int id;
>> +	const struct fwnode_handle *local_fwnode;
>> +};
> 
> Any kerneldoc?

There was none in the original so I didn't end up adding any here
either. I'll do that now.

> 
>> +
>>  #endif
>> diff --git a/include/linux/property.h b/include/linux/property.h
>> index 0b61ea4..45ef00a 100644
>> --- a/include/linux/property.h
>> +++ b/include/linux/property.h
>> @@ -284,4 +284,7 @@ struct fwnode_handle *fwnode_graph_get_remote_port(
>>  struct fwnode_handle *fwnode_graph_get_remote_endpoint(
>>  	struct fwnode_handle *fwnode);
>>  
>> +int fwnode_graph_parse_endpoint(struct fwnode_handle *fwnode,
>> +				struct fwnode_endpoint *endpoint);
>> +
>>  #endif /* _LINUX_PROPERTY_H_ */
>>
> 
> Thanks,
> Rafael
> 


-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 15/16] device property: Add fwnode_get_next_parent()

[Sakari Ailus]

On 03/13/17 23:58, Rafael J. Wysocki wrote:
> On Monday, March 06, 2017 04:19:29 PM Sakari Ailus wrote:
>> In order to differentiate the functionality between dropping a reference
>> to the node (or not) for the benefit of OF, introduce
>> fwnode_get_next_parent().
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>  drivers/base/property.c  | 29 +++++++++++++++++++++++++++++
>>  include/linux/property.h |  1 +
>>  2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>> index b8c1017..defeba9 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -953,6 +953,35 @@ int device_add_properties(struct device *dev,
>>  EXPORT_SYMBOL_GPL(device_add_properties);
>>  
>>  /**
>> + * fwnode_get_next_parent - Iterate to the node's parent
>> + * @fwnode: Firmware whose parent is retrieved
>> + *
>> + * This is like fwnode_get_parent() except that it drops the refcount
>> + * on the passed node, making it suitable for iterating through a
>> + * node's parents.
>> + *
>> + * Returns a node pointer with refcount incremented, use
>> + * fwnode_handle_node() on it when done.
>> + */
>> +struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode)
>> +{
>> +	struct fwnode_handle *parent = NULL;
>> +
>> +	if (is_of_node(fwnode)) {
>> +		struct device_node *node;
>> +
>> +		node = of_get_next_parent(to_of_node(fwnode));
> 
> So I would fold the [14/15] into this one and then it should be clear why that
> change was useful.

Ok, I'll fold the two patches.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Sakari Ailus]

Hi Rafael,

On 03/14/17 00:08, Rafael J. Wysocki wrote:
> On Monday, March 06, 2017 04:19:30 PM Sakari Ailus wrote:
>> Document the use of references into the hierarchical data extension
>> structure, as well as the use of port and endpoint concepts that are very
>> similar to those in Devicetree.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>  Documentation/acpi/dsd/graph.txt | 164 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 164 insertions(+)
>>  create mode 100644 Documentation/acpi/dsd/graph.txt
>>
>> diff --git a/Documentation/acpi/dsd/graph.txt b/Documentation/acpi/dsd/graph.txt
>> new file mode 100644
>> index 0000000..6195936
>> --- /dev/null
>> +++ b/Documentation/acpi/dsd/graph.txt
>> @@ -0,0 +1,164 @@
>> +Graphs
>> +
>> +
>> +_DSD
>> +----
>> +
>> +_DSD (Device Specific Data) [7] is a predefined ACPI device
>> +configuration object that can be used to convey information on
>> +hardware features which are not specifically covered by the ACPI
>> +specification [1][6]. There are two _DSD extensions that are relevant
>> +for graphs: property [4] and hierarchical data extensions [5]. The
>> +property extension provides generic key-value pairs whereas the
>> +hierarchical data extension supports nodes with references to other
>> +nodes, forming a tree. The nodes in the tree may contain properties as
>> +defined by the property extension. The two extensions together provide
>> +a tree-like structure with zero or more properties (key-value pairs)
>> +in each node of the tree.
>> +
>> +The data structure may be accessed at runtime by using the device_*
>> +and fwnode_* functions defined in include/linux/fwnode.h .
>> +
>> +Fwnode represents a generic firmware node object. It is independent on
>> +the firmware type. In ACPI, fwnodes are _DSD hierarchical data
>> +extensions objects. A device's _DSD object is represented by an
>> +fwnode.
>> +
>> +The data structure may be referenced to elsewhere in the ACPI tables
>> +by using a hard reference to the device itself and an index to the
>> +hierarchical data extension array on each depth.
>> +
>> +
>> +Ports and endpoints
>> +-------------------
>> +
>> +The port and endpoint concepts are very similar to those in Devicetree
>> +[3]. The port represent represent interface in a device, and an
> 
> s/represent represent/represents/
> 
> Plus I would say "a port" and "an interface".

Fixed.

> 
>> +endpoint represents a connection to that interface.
>> +
>> +All port nodes are located under the device's "_DSD" node in
>> +hierarchical data extension tree. The first package list entry of the
>> +hierarchical data extension related to each port node must begin with
>> +"port" string and must be followed by the number of the port.
> 
> So "port" should be the key, right?

How about this instead:

All port nodes are located under the device's "_DSD" node in the
hierarchical data extension tree. The property extension related to
each port node must contain the key "port" and an integer value which
is the number of the port.

> 
>> +
>> +Further on, endpoints are located under the individual port nodes. The
>> +first hierarchical data extension package list entry of the endpoint
>> +nodes must begin with "endpoint" and must be followed by the number
>> +of the endpoint.
>> +
>> +Each port node contains a property extension key "port", the value of
>> +which is the number of the port node. The number of the endpoint node is
>> +the index of the endpoint node in the endpoint node array under the port
>> +node, starting from 0.
>> +
>> +The endpoint reference uses property extension with "remote-endpoint"
>> +property name followed by a reference in the same package. Such references
>> +consist of the name of the device, index of the port node and finally the
>> +index of the endpoint node. The port index is indeed index to the referred
>> +port array, not the number of the port that is related to numbering of
>> +actual hardware interfaces in the respective hardware. The endpoint nodes
>> +are always referred to using an index to the endpoint node array.
>> +Individual references thus appear as:
>> +
>> +    Package() { device, port_node_index, endpoint_node_index }
>> +
>> +The references to endpoints must be always done both ways, to the
>> +remote endpoint and back from the referred remote endpoint node.
>> +
>> +A simple example of this is show below:
>> +
>> +    Scope (\_SB.PCI0.I2C2)
>> +    {
>> +	Device (CAM0)
>> +	{
>> +	    Name (_DSD, Package () {
>> +		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> +		Package () {
>> +		    Package () { "compatible", Package () { "nokia,smia" } },
>> +		},
>> +		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>> +		Package () {
>> +		    Package () { "port0", "PRT0" },
>> +		}
>> +	    })
>> +	    Name (PRT0, Package() {
>> +		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> +		Package () {
>> +		    Package () { "port", 0 },
>> +		},
>> +		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>> +		Package () {
>> +		    Package () { "endpoint0", "EP0" },
>> +		}
>> +	    })
>> +	    Name (EP0, Package() {
>> +		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> +		Package () {
>> +		    Package () { "remote-endpoint", Package() { \_SB.PCI0.ISP, 0, 0 } },
>> +		}
>> +	    })
>> +	}
>> +    }
>> +
>> +    Scope (\_SB.PCI0)
>> +    {
>> +	Device (ISP)
>> +	{
>> +	    Name (_DSD, Package () {
>> +		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>> +		Package () {
>> +		    Package () { "port4", "PRT4" },
>> +		}
>> +	    })
>> +
>> +	    Name (PRT4, Package() {
>> +		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> +		Package () {
>> +		    Package () { "port", 4 }, /* CSI-2 port number */
>> +		},
>> +		ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
>> +		Package () {
>> +		    Package () { "endpoint0", "EP0" },
>> +		}
>> +	    })
>> +
>> +	    Name (EP0, Package() {
>> +		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> +		Package () {
>> +		    Package () { "remote-endpoint", Package () { \_SB.PCI0.I2C2.CAM0, 0, 0 } },
> 
> Why indices and not the keys?  Something like
> 
> 	Package () { "remote-endpoint", Package () { \_SB.PCI0.I2C2.CAM0, "port0", "endpoint0" } },
> 
> should work too and would be more flexible I suppose and less prone to errors.

I agree. I'm definitely for using human readable strings instead of
indices here. I'll change the implementation correspondingly.

> 
>> +		}
>> +	    })
>> +	}
>> +    }
>> +
>> +Here, the port 0 of the "CAM0" device is connected to the port 4 of
>> +the "ISP" device. Note that the port index in the reference is still
>> +0, as it refers to an entry in the table and not the port node number
>> +itself.
> 
> Thanks,
> Rafael
> 


-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Sakari Ailus]

On 03/14/17 10:08, Sakari Ailus wrote:
> How about this instead:
> 
> All port nodes are located under the device's "_DSD" node in the
> hierarchical data extension tree. The property extension related to
> each port node must contain the key "port" and an integer value which
> is the number of the port.

So with matching strings instead of indices, this will change, too...

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Rafael J. Wysocki]

On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> On 03/14/17 10:08, Sakari Ailus wrote:
>> How about this instead:
>>
>> All port nodes are located under the device's "_DSD" node in the
>> hierarchical data extension tree. The property extension related to
>> each port node must contain the key "port" and an integer value which
>> is the number of the port.
>
> So with matching strings instead of indices, this will change, too...

It doesn't have to AFAICS, but the number is just redundant IMO.  You
only need a boolean property saying "this is a port", so you know that
you should expect a list of endpoints in that object.

Thanks,
Rafael

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Sakari Ailus]

Hi Rafael,

Rafael J. Wysocki wrote:
> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> On 03/14/17 10:08, Sakari Ailus wrote:
>>> How about this instead:
>>>
>>> All port nodes are located under the device's "_DSD" node in the
>>> hierarchical data extension tree. The property extension related to
>>> each port node must contain the key "port" and an integer value which
>>> is the number of the port.
>>
>> So with matching strings instead of indices, this will change, too...
>
> It doesn't have to AFAICS, but the number is just redundant IMO.  You
> only need a boolean property saying "this is a port", so you know that
> you should expect a list of endpoints in that object.

No, it's not redundant. It's the number of the physical port in the 
device --- this is how the driver gets to know where the connection has 
been made.

-- 
Regards,

Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Rafael J. Wysocki]

On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
<sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> Hi Rafael,
>
> Rafael J. Wysocki wrote:
>>
>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
>> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>
>>> On 03/14/17 10:08, Sakari Ailus wrote:
>>>>
>>>> How about this instead:
>>>>
>>>> All port nodes are located under the device's "_DSD" node in the
>>>> hierarchical data extension tree. The property extension related to
>>>> each port node must contain the key "port" and an integer value which
>>>> is the number of the port.
>>>
>>>
>>> So with matching strings instead of indices, this will change, too...
>>
>>
>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
>> only need a boolean property saying "this is a port", so you know that
>> you should expect a list of endpoints in that object.
>
>
> No, it's not redundant. It's the number of the physical port in the device
> --- this is how the driver gets to know where the connection has been made.

OK, but what exactly do you mean by "physical port"?

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Sakari Ailus]

Rafael J. Wysocki wrote:
> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> Hi Rafael,
>>
>> Rafael J. Wysocki wrote:
>>>
>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
>>> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>>
>>>> On 03/14/17 10:08, Sakari Ailus wrote:
>>>>>
>>>>> How about this instead:
>>>>>
>>>>> All port nodes are located under the device's "_DSD" node in the
>>>>> hierarchical data extension tree. The property extension related to
>>>>> each port node must contain the key "port" and an integer value which
>>>>> is the number of the port.
>>>>
>>>>
>>>> So with matching strings instead of indices, this will change, too...
>>>
>>>
>>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
>>> only need a boolean property saying "this is a port", so you know that
>>> you should expect a list of endpoints in that object.
>>
>>
>> No, it's not redundant. It's the number of the physical port in the device
>> --- this is how the driver gets to know where the connection has been made.
>
> OK, but what exactly do you mean by "physical port"?

The device (or an IP block) has physical interfaces to the world 
outside. There could be just one, but there may be more. For an ISP, 
there could be e.g. four CSI-2 receivers to each of which you could 
connect a camera sensor. So for an ISP device, that number tells which 
of the receivers a given sensor is connected to.

The mapping between this number and what the hardware datasheet refers 
to needs to be documented per device.

-- 
Regards,

Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Rafael J. Wysocki]

On Tue, Mar 14, 2017 at 10:16 PM, Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> Rafael J. Wysocki wrote:
>>
>> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
>> <sakari.ailus@linux.intel.com> wrote:
>>>
>>> Hi Rafael,
>>>
>>> Rafael J. Wysocki wrote:
>>>>
>>>>
>>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
>>>> <sakari.ailus@linux.intel.com> wrote:
>>>>>
>>>>>
>>>>> On 03/14/17 10:08, Sakari Ailus wrote:
>>>>>>
>>>>>>
>>>>>> How about this instead:
>>>>>>
>>>>>> All port nodes are located under the device's "_DSD" node in the
>>>>>> hierarchical data extension tree. The property extension related to
>>>>>> each port node must contain the key "port" and an integer value which
>>>>>> is the number of the port.
>>>>>
>>>>>
>>>>>
>>>>> So with matching strings instead of indices, this will change, too...
>>>>
>>>>
>>>>
>>>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
>>>> only need a boolean property saying "this is a port", so you know that
>>>> you should expect a list of endpoints in that object.
>>>
>>>
>>>
>>> No, it's not redundant. It's the number of the physical port in the
>>> device
>>> --- this is how the driver gets to know where the connection has been
>>> made.
>>
>>
>> OK, but what exactly do you mean by "physical port"?
>
>
> The device (or an IP block) has physical interfaces to the world outside.
> There could be just one, but there may be more. For an ISP, there could be
> e.g. four CSI-2 receivers to each of which you could connect a camera
> sensor. So for an ISP device, that number tells which of the receivers a
> given sensor is connected to.
>
> The mapping between this number and what the hardware datasheet refers to
> needs to be documented per device.

OK, so the number actually is an arbitrary piece of data associated
with the key "port" and the interpretation of that piece of data
depends on whoever asks for that value.

IOW, the core doesn't care.

With all due respect to whoever invented this on the DT side, this is
just bad design to me, because it causes the "port" property to serve
two different purposes at the same time.  First, it tells the core
that this object is a port.  Second, it is expected to provide a piece
of data of unspecified interpretation to somebody.  Which means that
the "port" property is both general and device-specific at the same
time and the sanity of that is quite questionable IMO.

Thanks,
Rafael

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Sakari Ailus]

Rafael J. Wysocki wrote:
> On Tue, Mar 14, 2017 at 10:16 PM, Sakari Ailus
> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> Rafael J. Wysocki wrote:
>>>
>>> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
>>> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> Rafael J. Wysocki wrote:
>>>>>
>>>>>
>>>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
>>>>> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>>>>
>>>>>>
>>>>>> On 03/14/17 10:08, Sakari Ailus wrote:
>>>>>>>
>>>>>>>
>>>>>>> How about this instead:
>>>>>>>
>>>>>>> All port nodes are located under the device's "_DSD" node in the
>>>>>>> hierarchical data extension tree. The property extension related to
>>>>>>> each port node must contain the key "port" and an integer value which
>>>>>>> is the number of the port.
>>>>>>
>>>>>>
>>>>>>
>>>>>> So with matching strings instead of indices, this will change, too...
>>>>>
>>>>>
>>>>>
>>>>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
>>>>> only need a boolean property saying "this is a port", so you know that
>>>>> you should expect a list of endpoints in that object.
>>>>
>>>>
>>>>
>>>> No, it's not redundant. It's the number of the physical port in the
>>>> device
>>>> --- this is how the driver gets to know where the connection has been
>>>> made.
>>>
>>>
>>> OK, but what exactly do you mean by "physical port"?
>>
>>
>> The device (or an IP block) has physical interfaces to the world outside.
>> There could be just one, but there may be more. For an ISP, there could be
>> e.g. four CSI-2 receivers to each of which you could connect a camera
>> sensor. So for an ISP device, that number tells which of the receivers a
>> given sensor is connected to.
>>
>> The mapping between this number and what the hardware datasheet refers to
>> needs to be documented per device.
>
> OK, so the number actually is an arbitrary piece of data associated
> with the key "port" and the interpretation of that piece of data
> depends on whoever asks for that value.
>
> IOW, the core doesn't care.
>
> With all due respect to whoever invented this on the DT side, this is
> just bad design to me, because it causes the "port" property to serve
> two different purposes at the same time.  First, it tells the core
> that this object is a port.  Second, it is expected to provide a piece
> of data of unspecified interpretation to somebody.  Which means that
> the "port" property is both general and device-specific at the same
> time and the sanity of that is quite questionable IMO.

DT uses a node called either "port" or "ports" to store the port nodes. 
The reg property tells the number of the port (see 
Documentation/devicetree/bindings/graph.txt). I'm no DT expert, but my 
understanding is that the node namespace is different from the property 
namespace.

If you're concerned of possible double meanings, it's entirely possible 
to put the port nodes under hierarchical data extension named e.g. 
"ports", and document that this is what the node must be called (single 
port node could be just called "port"). This way, it should be much more 
difficult to interpret a non-port node as a port node --- roughly 
equivalent of the DT ports node.

The drawback with this change is that the size of the data structure in 
ASL (and AML) will grow.

-- 
Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Rafael J. Wysocki]

On Tue, Mar 14, 2017 at 11:53 PM, Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> Rafael J. Wysocki wrote:
>>
>> On Tue, Mar 14, 2017 at 10:16 PM, Sakari Ailus
>> <sakari.ailus@linux.intel.com> wrote:
>>>
>>> Rafael J. Wysocki wrote:
>>>>
>>>>
>>>> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
>>>> <sakari.ailus@linux.intel.com> wrote:
>>>>>
>>>>>
>>>>> Hi Rafael,
>>>>>
>>>>> Rafael J. Wysocki wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
>>>>>> <sakari.ailus@linux.intel.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 03/14/17 10:08, Sakari Ailus wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> How about this instead:
>>>>>>>>
>>>>>>>> All port nodes are located under the device's "_DSD" node in the
>>>>>>>> hierarchical data extension tree. The property extension related to
>>>>>>>> each port node must contain the key "port" and an integer value
>>>>>>>> which
>>>>>>>> is the number of the port.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> So with matching strings instead of indices, this will change, too...
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
>>>>>> only need a boolean property saying "this is a port", so you know that
>>>>>> you should expect a list of endpoints in that object.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> No, it's not redundant. It's the number of the physical port in the
>>>>> device
>>>>> --- this is how the driver gets to know where the connection has been
>>>>> made.
>>>>
>>>>
>>>>
>>>> OK, but what exactly do you mean by "physical port"?
>>>
>>>
>>>
>>> The device (or an IP block) has physical interfaces to the world outside.
>>> There could be just one, but there may be more. For an ISP, there could
>>> be
>>> e.g. four CSI-2 receivers to each of which you could connect a camera
>>> sensor. So for an ISP device, that number tells which of the receivers a
>>> given sensor is connected to.
>>>
>>> The mapping between this number and what the hardware datasheet refers to
>>> needs to be documented per device.
>>
>>
>> OK, so the number actually is an arbitrary piece of data associated
>> with the key "port" and the interpretation of that piece of data
>> depends on whoever asks for that value.
>>
>> IOW, the core doesn't care.
>>
>> With all due respect to whoever invented this on the DT side, this is
>> just bad design to me, because it causes the "port" property to serve
>> two different purposes at the same time.  First, it tells the core
>> that this object is a port.  Second, it is expected to provide a piece
>> of data of unspecified interpretation to somebody.  Which means that
>> the "port" property is both general and device-specific at the same
>> time and the sanity of that is quite questionable IMO.
>
>
> DT uses a node called either "port" or "ports" to store the port nodes. The
> reg property tells the number of the port (see
> Documentation/devicetree/bindings/graph.txt). I'm no DT expert, but my
> understanding is that the node namespace is different from the property
> namespace.

So on the DT side it actually looks OK to me.

And the <reg> value is referred to as the port-endpoint identifier, so
I guess this is used for referring to the port/endpoint instead of an
index or the key value somewhere?

> If you're concerned of possible double meanings, it's entirely possible to
> put the port nodes under hierarchical data extension named e.g. "ports", and
> document that this is what the node must be called (single port node could
> be just called "port"). This way, it should be much more difficult to
> interpret a non-port node as a port node --- roughly equivalent of the DT
> ports node.
>
> The drawback with this change is that the size of the data structure in ASL
> (and AML) will grow.

The "ports" thing would only be useful if we had the other properties
to put in there.

So I guess we can specify that the "port" property value is the
identifier of the port and then we will use this in the
"remote-endpoint" property on the other end instead of an index.

And analogously for the "enpoint" property value.

Thanks,
Rafael

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Sakari Ailus]

On Wed, Mar 15, 2017 at 12:13:45AM +0100, Rafael J. Wysocki wrote:
> On Tue, Mar 14, 2017 at 11:53 PM, Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > Rafael J. Wysocki wrote:
> >>
> >> On Tue, Mar 14, 2017 at 10:16 PM, Sakari Ailus
> >> <sakari.ailus@linux.intel.com> wrote:
> >>>
> >>> Rafael J. Wysocki wrote:
> >>>>
> >>>>
> >>>> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
> >>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>>
> >>>>>
> >>>>> Hi Rafael,
> >>>>>
> >>>>> Rafael J. Wysocki wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
> >>>>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On 03/14/17 10:08, Sakari Ailus wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> How about this instead:
> >>>>>>>>
> >>>>>>>> All port nodes are located under the device's "_DSD" node in the
> >>>>>>>> hierarchical data extension tree. The property extension related to
> >>>>>>>> each port node must contain the key "port" and an integer value
> >>>>>>>> which
> >>>>>>>> is the number of the port.
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> So with matching strings instead of indices, this will change, too...
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
> >>>>>> only need a boolean property saying "this is a port", so you know that
> >>>>>> you should expect a list of endpoints in that object.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> No, it's not redundant. It's the number of the physical port in the
> >>>>> device
> >>>>> --- this is how the driver gets to know where the connection has been
> >>>>> made.
> >>>>
> >>>>
> >>>>
> >>>> OK, but what exactly do you mean by "physical port"?
> >>>
> >>>
> >>>
> >>> The device (or an IP block) has physical interfaces to the world outside.
> >>> There could be just one, but there may be more. For an ISP, there could
> >>> be
> >>> e.g. four CSI-2 receivers to each of which you could connect a camera
> >>> sensor. So for an ISP device, that number tells which of the receivers a
> >>> given sensor is connected to.
> >>>
> >>> The mapping between this number and what the hardware datasheet refers to
> >>> needs to be documented per device.
> >>
> >>
> >> OK, so the number actually is an arbitrary piece of data associated
> >> with the key "port" and the interpretation of that piece of data
> >> depends on whoever asks for that value.
> >>
> >> IOW, the core doesn't care.
> >>
> >> With all due respect to whoever invented this on the DT side, this is
> >> just bad design to me, because it causes the "port" property to serve
> >> two different purposes at the same time.  First, it tells the core
> >> that this object is a port.  Second, it is expected to provide a piece
> >> of data of unspecified interpretation to somebody.  Which means that
> >> the "port" property is both general and device-specific at the same
> >> time and the sanity of that is quite questionable IMO.
> >
> >
> > DT uses a node called either "port" or "ports" to store the port nodes. The
> > reg property tells the number of the port (see
> > Documentation/devicetree/bindings/graph.txt). I'm no DT expert, but my
> > understanding is that the node namespace is different from the property
> > namespace.
> 
> So on the DT side it actually looks OK to me.
> 
> And the <reg> value is referred to as the port-endpoint identifier, so
> I guess this is used for referring to the port/endpoint instead of an
> index or the key value somewhere?

The remote-endpoint uses phandles; they're a mechanism in DT to refer to
different nodes in the tree (DT does not differentiate between devices and
non-device nodes). There's a relatively good example here:

	arch/arm/boot/dts/omap3-n9.dts

> 
> > If you're concerned of possible double meanings, it's entirely possible to
> > put the port nodes under hierarchical data extension named e.g. "ports", and
> > document that this is what the node must be called (single port node could
> > be just called "port"). This way, it should be much more difficult to
> > interpret a non-port node as a port node --- roughly equivalent of the DT
> > ports node.
> >
> > The drawback with this change is that the size of the data structure in ASL
> > (and AML) will grow.
> 
> The "ports" thing would only be useful if we had the other properties
> to put in there.
> 
> So I guess we can specify that the "port" property value is the
> identifier of the port and then we will use this in the
> "remote-endpoint" property on the other end instead of an index.

Makes sense.

> 
> And analogously for the "enpoint" property value.

The endpoint hierarchical data extension node name? There is no endpoint
property being used by this version of the set anymore; I removed it as it
was redundant. However a human readable endpoint node name can be chosen
which that'd be quite practical to identify the endpoint.

Then the remote-endpoint properties would be:

	Package () { device, port (integer), endpoint-node-name (string) }

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Sakari Ailus]

On 03/15/17 10:23, Sakari Ailus wrote:
> On Wed, Mar 15, 2017 at 12:13:45AM +0100, Rafael J. Wysocki wrote:
>> On Tue, Mar 14, 2017 at 11:53 PM, Sakari Ailus
>> <sakari.ailus@linux.intel.com> wrote:
>>> Rafael J. Wysocki wrote:
>>>>
>>>> On Tue, Mar 14, 2017 at 10:16 PM, Sakari Ailus
>>>> <sakari.ailus@linux.intel.com> wrote:
>>>>>
>>>>> Rafael J. Wysocki wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
>>>>>> <sakari.ailus@linux.intel.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hi Rafael,
>>>>>>>
>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
>>>>>>>> <sakari.ailus@linux.intel.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 03/14/17 10:08, Sakari Ailus wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> How about this instead:
>>>>>>>>>>
>>>>>>>>>> All port nodes are located under the device's "_DSD" node in the
>>>>>>>>>> hierarchical data extension tree. The property extension related to
>>>>>>>>>> each port node must contain the key "port" and an integer value
>>>>>>>>>> which
>>>>>>>>>> is the number of the port.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> So with matching strings instead of indices, this will change, too...
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
>>>>>>>> only need a boolean property saying "this is a port", so you know that
>>>>>>>> you should expect a list of endpoints in that object.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> No, it's not redundant. It's the number of the physical port in the
>>>>>>> device
>>>>>>> --- this is how the driver gets to know where the connection has been
>>>>>>> made.
>>>>>>
>>>>>>
>>>>>>
>>>>>> OK, but what exactly do you mean by "physical port"?
>>>>>
>>>>>
>>>>>
>>>>> The device (or an IP block) has physical interfaces to the world outside.
>>>>> There could be just one, but there may be more. For an ISP, there could
>>>>> be
>>>>> e.g. four CSI-2 receivers to each of which you could connect a camera
>>>>> sensor. So for an ISP device, that number tells which of the receivers a
>>>>> given sensor is connected to.
>>>>>
>>>>> The mapping between this number and what the hardware datasheet refers to
>>>>> needs to be documented per device.
>>>>
>>>>
>>>> OK, so the number actually is an arbitrary piece of data associated
>>>> with the key "port" and the interpretation of that piece of data
>>>> depends on whoever asks for that value.
>>>>
>>>> IOW, the core doesn't care.
>>>>
>>>> With all due respect to whoever invented this on the DT side, this is
>>>> just bad design to me, because it causes the "port" property to serve
>>>> two different purposes at the same time.  First, it tells the core
>>>> that this object is a port.  Second, it is expected to provide a piece
>>>> of data of unspecified interpretation to somebody.  Which means that
>>>> the "port" property is both general and device-specific at the same
>>>> time and the sanity of that is quite questionable IMO.
>>>
>>>
>>> DT uses a node called either "port" or "ports" to store the port nodes. The
>>> reg property tells the number of the port (see
>>> Documentation/devicetree/bindings/graph.txt). I'm no DT expert, but my
>>> understanding is that the node namespace is different from the property
>>> namespace.
>>
>> So on the DT side it actually looks OK to me.
>>
>> And the <reg> value is referred to as the port-endpoint identifier, so
>> I guess this is used for referring to the port/endpoint instead of an
>> index or the key value somewhere?
> 
> The remote-endpoint uses phandles; they're a mechanism in DT to refer to
> different nodes in the tree (DT does not differentiate between devices and
> non-device nodes). There's a relatively good example here:
> 
> 	arch/arm/boot/dts/omap3-n9.dts
> 
>>
>>> If you're concerned of possible double meanings, it's entirely possible to
>>> put the port nodes under hierarchical data extension named e.g. "ports", and
>>> document that this is what the node must be called (single port node could
>>> be just called "port"). This way, it should be much more difficult to
>>> interpret a non-port node as a port node --- roughly equivalent of the DT
>>> ports node.
>>>
>>> The drawback with this change is that the size of the data structure in ASL
>>> (and AML) will grow.
>>
>> The "ports" thing would only be useful if we had the other properties
>> to put in there.
>>
>> So I guess we can specify that the "port" property value is the
>> identifier of the port and then we will use this in the
>> "remote-endpoint" property on the other end instead of an index.
> 
> Makes sense.
> 
>>
>> And analogously for the "enpoint" property value.
> 
> The endpoint hierarchical data extension node name? There is no endpoint
> property being used by this version of the set anymore; I removed it as it
> was redundant. However a human readable endpoint node name can be chosen
> which that'd be quite practical to identify the endpoint.
> 
> Then the remote-endpoint properties would be:
> 
> 	Package () { device, port (integer), endpoint-node-name (string) }
> 

Oh, well... the device is present there already, so the endpoint
reference would well use an index pretty much equally well. Most of the
time it'll be zero anyway. I.e.

	Package() { device, port number (integer), endpoint id (integer }

But switching from the port index to the port number is a tangible
improvement.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Rafael J. Wysocki]

On Wednesday, March 15, 2017 11:33:35 AM Sakari Ailus wrote:
> On 03/15/17 10:23, Sakari Ailus wrote:
> > On Wed, Mar 15, 2017 at 12:13:45AM +0100, Rafael J. Wysocki wrote:
> >> On Tue, Mar 14, 2017 at 11:53 PM, Sakari Ailus
> >> <sakari.ailus@linux.intel.com> wrote:
> >>> Rafael J. Wysocki wrote:
> >>>>
> >>>> On Tue, Mar 14, 2017 at 10:16 PM, Sakari Ailus
> >>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>>
> >>>>> Rafael J. Wysocki wrote:
> >>>>>>
> >>>>>>
> >>>>>> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
> >>>>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> Hi Rafael,
> >>>>>>>
> >>>>>>> Rafael J. Wysocki wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
> >>>>>>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On 03/14/17 10:08, Sakari Ailus wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> How about this instead:
> >>>>>>>>>>
> >>>>>>>>>> All port nodes are located under the device's "_DSD" node in the
> >>>>>>>>>> hierarchical data extension tree. The property extension related to
> >>>>>>>>>> each port node must contain the key "port" and an integer value
> >>>>>>>>>> which
> >>>>>>>>>> is the number of the port.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> So with matching strings instead of indices, this will change, too...
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
> >>>>>>>> only need a boolean property saying "this is a port", so you know that
> >>>>>>>> you should expect a list of endpoints in that object.
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> No, it's not redundant. It's the number of the physical port in the
> >>>>>>> device
> >>>>>>> --- this is how the driver gets to know where the connection has been
> >>>>>>> made.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> OK, but what exactly do you mean by "physical port"?
> >>>>>
> >>>>>
> >>>>>
> >>>>> The device (or an IP block) has physical interfaces to the world outside.
> >>>>> There could be just one, but there may be more. For an ISP, there could
> >>>>> be
> >>>>> e.g. four CSI-2 receivers to each of which you could connect a camera
> >>>>> sensor. So for an ISP device, that number tells which of the receivers a
> >>>>> given sensor is connected to.
> >>>>>
> >>>>> The mapping between this number and what the hardware datasheet refers to
> >>>>> needs to be documented per device.
> >>>>
> >>>>
> >>>> OK, so the number actually is an arbitrary piece of data associated
> >>>> with the key "port" and the interpretation of that piece of data
> >>>> depends on whoever asks for that value.
> >>>>
> >>>> IOW, the core doesn't care.
> >>>>
> >>>> With all due respect to whoever invented this on the DT side, this is
> >>>> just bad design to me, because it causes the "port" property to serve
> >>>> two different purposes at the same time.  First, it tells the core
> >>>> that this object is a port.  Second, it is expected to provide a piece
> >>>> of data of unspecified interpretation to somebody.  Which means that
> >>>> the "port" property is both general and device-specific at the same
> >>>> time and the sanity of that is quite questionable IMO.
> >>>
> >>>
> >>> DT uses a node called either "port" or "ports" to store the port nodes. The
> >>> reg property tells the number of the port (see
> >>> Documentation/devicetree/bindings/graph.txt). I'm no DT expert, but my
> >>> understanding is that the node namespace is different from the property
> >>> namespace.
> >>
> >> So on the DT side it actually looks OK to me.
> >>
> >> And the <reg> value is referred to as the port-endpoint identifier, so
> >> I guess this is used for referring to the port/endpoint instead of an
> >> index or the key value somewhere?
> > 
> > The remote-endpoint uses phandles; they're a mechanism in DT to refer to
> > different nodes in the tree (DT does not differentiate between devices and
> > non-device nodes). There's a relatively good example here:
> > 
> > 	arch/arm/boot/dts/omap3-n9.dts

Yes, I figured that out. :-)

Still, the <rev> property value is used for something somewhere in the code I gather.

> >>
> >>> If you're concerned of possible double meanings, it's entirely possible to
> >>> put the port nodes under hierarchical data extension named e.g. "ports", and
> >>> document that this is what the node must be called (single port node could
> >>> be just called "port"). This way, it should be much more difficult to
> >>> interpret a non-port node as a port node --- roughly equivalent of the DT
> >>> ports node.
> >>>
> >>> The drawback with this change is that the size of the data structure in ASL
> >>> (and AML) will grow.
> >>
> >> The "ports" thing would only be useful if we had the other properties
> >> to put in there.
> >>
> >> So I guess we can specify that the "port" property value is the
> >> identifier of the port and then we will use this in the
> >> "remote-endpoint" property on the other end instead of an index.
> > 
> > Makes sense.
> > 
> >>
> >> And analogously for the "enpoint" property value.
> > 
> > The endpoint hierarchical data extension node name? There is no endpoint
> > property being used by this version of the set anymore; I removed it as it
> > was redundant. However a human readable endpoint node name can be chosen
> > which that'd be quite practical to identify the endpoint.
> > 
> > Then the remote-endpoint properties would be:
> > 
> > 	Package () { device, port (integer), endpoint-node-name (string) }
> > 
> 
> Oh, well... the device is present there already, so the endpoint
> reference would well use an index pretty much equally well. Most of the
> time it'll be zero anyway. I.e.
> 
> 	Package() { device, port number (integer), endpoint id (integer }

Yes, I would do that, and actually not using the index for the endpoint too.

Let ports and endpoints be symmetrical in that respect, that is a port is
required to have a { "port", id } property and an endpoind is required to
have a { "endpoint", id } property and let the ids be used in
"remote-endpoint" properties as per the above.

Then, in each case, the id would be whatever the value of the <rev> property
on the DT side would be.

> But switching from the port index to the port number is a tangible
> improvement.

Well, to me, using indices should not even be taken into consideration as
a viable approach in similar cases.

I know that using indices is a common practice in the ACPI land, but IMO that's
not a good one.

Thanks,
Rafael

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Sakari Ailus]

On 03/15/17 13:28, Rafael J. Wysocki wrote:
> On Wednesday, March 15, 2017 11:33:35 AM Sakari Ailus wrote:
>> On 03/15/17 10:23, Sakari Ailus wrote:
>>> On Wed, Mar 15, 2017 at 12:13:45AM +0100, Rafael J. Wysocki wrote:
>>>> On Tue, Mar 14, 2017 at 11:53 PM, Sakari Ailus
>>>> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>>> Rafael J. Wysocki wrote:
>>>>>>
>>>>>> On Tue, Mar 14, 2017 at 10:16 PM, Sakari Ailus
>>>>>> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>>>>>
>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
>>>>>>>> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Rafael,
>>>>>>>>>
>>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
>>>>>>>>>> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 03/14/17 10:08, Sakari Ailus wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> How about this instead:
>>>>>>>>>>>>
>>>>>>>>>>>> All port nodes are located under the device's "_DSD" node in the
>>>>>>>>>>>> hierarchical data extension tree. The property extension related to
>>>>>>>>>>>> each port node must contain the key "port" and an integer value
>>>>>>>>>>>> which
>>>>>>>>>>>> is the number of the port.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So with matching strings instead of indices, this will change, too...
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
>>>>>>>>>> only need a boolean property saying "this is a port", so you know that
>>>>>>>>>> you should expect a list of endpoints in that object.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> No, it's not redundant. It's the number of the physical port in the
>>>>>>>>> device
>>>>>>>>> --- this is how the driver gets to know where the connection has been
>>>>>>>>> made.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> OK, but what exactly do you mean by "physical port"?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The device (or an IP block) has physical interfaces to the world outside.
>>>>>>> There could be just one, but there may be more. For an ISP, there could
>>>>>>> be
>>>>>>> e.g. four CSI-2 receivers to each of which you could connect a camera
>>>>>>> sensor. So for an ISP device, that number tells which of the receivers a
>>>>>>> given sensor is connected to.
>>>>>>>
>>>>>>> The mapping between this number and what the hardware datasheet refers to
>>>>>>> needs to be documented per device.
>>>>>>
>>>>>>
>>>>>> OK, so the number actually is an arbitrary piece of data associated
>>>>>> with the key "port" and the interpretation of that piece of data
>>>>>> depends on whoever asks for that value.
>>>>>>
>>>>>> IOW, the core doesn't care.
>>>>>>
>>>>>> With all due respect to whoever invented this on the DT side, this is
>>>>>> just bad design to me, because it causes the "port" property to serve
>>>>>> two different purposes at the same time.  First, it tells the core
>>>>>> that this object is a port.  Second, it is expected to provide a piece
>>>>>> of data of unspecified interpretation to somebody.  Which means that
>>>>>> the "port" property is both general and device-specific at the same
>>>>>> time and the sanity of that is quite questionable IMO.
>>>>>
>>>>>
>>>>> DT uses a node called either "port" or "ports" to store the port nodes. The
>>>>> reg property tells the number of the port (see
>>>>> Documentation/devicetree/bindings/graph.txt). I'm no DT expert, but my
>>>>> understanding is that the node namespace is different from the property
>>>>> namespace.
>>>>
>>>> So on the DT side it actually looks OK to me.
>>>>
>>>> And the <reg> value is referred to as the port-endpoint identifier, so
>>>> I guess this is used for referring to the port/endpoint instead of an
>>>> index or the key value somewhere?
>>>
>>> The remote-endpoint uses phandles; they're a mechanism in DT to refer to
>>> different nodes in the tree (DT does not differentiate between devices and
>>> non-device nodes). There's a relatively good example here:
>>>
>>> 	arch/arm/boot/dts/omap3-n9.dts
> 
> Yes, I figured that out. :-)
> 
> Still, the <rev> property value is used for something somewhere in the code I gather.
> 
>>>>
>>>>> If you're concerned of possible double meanings, it's entirely possible to
>>>>> put the port nodes under hierarchical data extension named e.g. "ports", and
>>>>> document that this is what the node must be called (single port node could
>>>>> be just called "port"). This way, it should be much more difficult to
>>>>> interpret a non-port node as a port node --- roughly equivalent of the DT
>>>>> ports node.
>>>>>
>>>>> The drawback with this change is that the size of the data structure in ASL
>>>>> (and AML) will grow.
>>>>
>>>> The "ports" thing would only be useful if we had the other properties
>>>> to put in there.
>>>>
>>>> So I guess we can specify that the "port" property value is the
>>>> identifier of the port and then we will use this in the
>>>> "remote-endpoint" property on the other end instead of an index.
>>>
>>> Makes sense.
>>>
>>>>
>>>> And analogously for the "enpoint" property value.
>>>
>>> The endpoint hierarchical data extension node name? There is no endpoint
>>> property being used by this version of the set anymore; I removed it as it
>>> was redundant. However a human readable endpoint node name can be chosen
>>> which that'd be quite practical to identify the endpoint.
>>>
>>> Then the remote-endpoint properties would be:
>>>
>>> 	Package () { device, port (integer), endpoint-node-name (string) }
>>>
>>
>> Oh, well... the device is present there already, so the endpoint
>> reference would well use an index pretty much equally well. Most of the
>> time it'll be zero anyway. I.e.
>>
>> 	Package() { device, port number (integer), endpoint id (integer }
> 
> Yes, I would do that, and actually not using the index for the endpoint too.
> 
> Let ports and endpoints be symmetrical in that respect, that is a port is
> required to have a { "port", id } property and an endpoind is required to
> have a { "endpoint", id } property and let the ids be used in
> "remote-endpoint" properties as per the above.
> 
> Then, in each case, the id would be whatever the value of the <rev> property
> on the DT side would be.

DT graphs only have port numbers, the endpoints are not referred to by
IDs --- just phandles. The closest equivalent we have in ACPI is a
device reference as far as I can tell, and this is why the port number
and some identifier for the endpoint is required.

Adding a numeric ID for the endpoint is somewhat artificial. Most would
just have zero there as there are commonly only a single endpoint in a
port. And if there were more than one, one of the most sensible
approaches to number them would be a monotonically incrementing number
from zero --- which is the same than the index.

So my view is that adding an endpoint property simply adds no value.

-- 
Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Rafael J. Wysocki]

On Wednesday, March 15, 2017 01:45:48 PM Sakari Ailus wrote:
> On 03/15/17 13:28, Rafael J. Wysocki wrote:
> > On Wednesday, March 15, 2017 11:33:35 AM Sakari Ailus wrote:
> >> On 03/15/17 10:23, Sakari Ailus wrote:
> >>> On Wed, Mar 15, 2017 at 12:13:45AM +0100, Rafael J. Wysocki wrote:
> >>>> On Tue, Mar 14, 2017 at 11:53 PM, Sakari Ailus
> >>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>> Rafael J. Wysocki wrote:
> >>>>>>
> >>>>>> On Tue, Mar 14, 2017 at 10:16 PM, Sakari Ailus
> >>>>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>>>>
> >>>>>>> Rafael J. Wysocki wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
> >>>>>>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Hi Rafael,
> >>>>>>>>>
> >>>>>>>>> Rafael J. Wysocki wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
> >>>>>>>>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On 03/14/17 10:08, Sakari Ailus wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> How about this instead:
> >>>>>>>>>>>>
> >>>>>>>>>>>> All port nodes are located under the device's "_DSD" node in the
> >>>>>>>>>>>> hierarchical data extension tree. The property extension related to
> >>>>>>>>>>>> each port node must contain the key "port" and an integer value
> >>>>>>>>>>>> which
> >>>>>>>>>>>> is the number of the port.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> So with matching strings instead of indices, this will change, too...
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
> >>>>>>>>>> only need a boolean property saying "this is a port", so you know that
> >>>>>>>>>> you should expect a list of endpoints in that object.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> No, it's not redundant. It's the number of the physical port in the
> >>>>>>>>> device
> >>>>>>>>> --- this is how the driver gets to know where the connection has been
> >>>>>>>>> made.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> OK, but what exactly do you mean by "physical port"?
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> The device (or an IP block) has physical interfaces to the world outside.
> >>>>>>> There could be just one, but there may be more. For an ISP, there could
> >>>>>>> be
> >>>>>>> e.g. four CSI-2 receivers to each of which you could connect a camera
> >>>>>>> sensor. So for an ISP device, that number tells which of the receivers a
> >>>>>>> given sensor is connected to.
> >>>>>>>
> >>>>>>> The mapping between this number and what the hardware datasheet refers to
> >>>>>>> needs to be documented per device.
> >>>>>>
> >>>>>>
> >>>>>> OK, so the number actually is an arbitrary piece of data associated
> >>>>>> with the key "port" and the interpretation of that piece of data
> >>>>>> depends on whoever asks for that value.
> >>>>>>
> >>>>>> IOW, the core doesn't care.
> >>>>>>
> >>>>>> With all due respect to whoever invented this on the DT side, this is
> >>>>>> just bad design to me, because it causes the "port" property to serve
> >>>>>> two different purposes at the same time.  First, it tells the core
> >>>>>> that this object is a port.  Second, it is expected to provide a piece
> >>>>>> of data of unspecified interpretation to somebody.  Which means that
> >>>>>> the "port" property is both general and device-specific at the same
> >>>>>> time and the sanity of that is quite questionable IMO.
> >>>>>
> >>>>>
> >>>>> DT uses a node called either "port" or "ports" to store the port nodes. The
> >>>>> reg property tells the number of the port (see
> >>>>> Documentation/devicetree/bindings/graph.txt). I'm no DT expert, but my
> >>>>> understanding is that the node namespace is different from the property
> >>>>> namespace.
> >>>>
> >>>> So on the DT side it actually looks OK to me.
> >>>>
> >>>> And the <reg> value is referred to as the port-endpoint identifier, so
> >>>> I guess this is used for referring to the port/endpoint instead of an
> >>>> index or the key value somewhere?
> >>>
> >>> The remote-endpoint uses phandles; they're a mechanism in DT to refer to
> >>> different nodes in the tree (DT does not differentiate between devices and
> >>> non-device nodes). There's a relatively good example here:
> >>>
> >>> 	arch/arm/boot/dts/omap3-n9.dts
> > 
> > Yes, I figured that out. :-)
> > 
> > Still, the <rev> property value is used for something somewhere in the code I gather.
> > 
> >>>>
> >>>>> If you're concerned of possible double meanings, it's entirely possible to
> >>>>> put the port nodes under hierarchical data extension named e.g. "ports", and
> >>>>> document that this is what the node must be called (single port node could
> >>>>> be just called "port"). This way, it should be much more difficult to
> >>>>> interpret a non-port node as a port node --- roughly equivalent of the DT
> >>>>> ports node.
> >>>>>
> >>>>> The drawback with this change is that the size of the data structure in ASL
> >>>>> (and AML) will grow.
> >>>>
> >>>> The "ports" thing would only be useful if we had the other properties
> >>>> to put in there.
> >>>>
> >>>> So I guess we can specify that the "port" property value is the
> >>>> identifier of the port and then we will use this in the
> >>>> "remote-endpoint" property on the other end instead of an index.
> >>>
> >>> Makes sense.
> >>>
> >>>>
> >>>> And analogously for the "enpoint" property value.
> >>>
> >>> The endpoint hierarchical data extension node name? There is no endpoint
> >>> property being used by this version of the set anymore; I removed it as it
> >>> was redundant. However a human readable endpoint node name can be chosen
> >>> which that'd be quite practical to identify the endpoint.
> >>>
> >>> Then the remote-endpoint properties would be:
> >>>
> >>> 	Package () { device, port (integer), endpoint-node-name (string) }
> >>>
> >>
> >> Oh, well... the device is present there already, so the endpoint
> >> reference would well use an index pretty much equally well. Most of the
> >> time it'll be zero anyway. I.e.
> >>
> >> 	Package() { device, port number (integer), endpoint id (integer }
> > 
> > Yes, I would do that, and actually not using the index for the endpoint too.
> > 
> > Let ports and endpoints be symmetrical in that respect, that is a port is
> > required to have a { "port", id } property and an endpoind is required to
> > have a { "endpoint", id } property and let the ids be used in
> > "remote-endpoint" properties as per the above.
> > 
> > Then, in each case, the id would be whatever the value of the <rev> property
> > on the DT side would be.
> 
> DT graphs only have port numbers, the endpoints are not referred to by
> IDs --- just phandles. The closest equivalent we have in ACPI is a
> device reference as far as I can tell, and this is why the port number
> and some identifier for the endpoint is required.
> 
> Adding a numeric ID for the endpoint is somewhat artificial. Most would
> just have zero there as there are commonly only a single endpoint in a
> port. And if there were more than one, one of the most sensible
> approaches to number them would be a monotonically incrementing number
> from zero --- which is the same than the index.
> 
> So my view is that adding an endpoint property simply adds no value.

But endpoints can have a <rev> property in DT.  What is it used for then?

Thanks,
Rafael

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Sakari Ailus]

On 03/15/17 13:53, Rafael J. Wysocki wrote:
> On Wednesday, March 15, 2017 01:45:48 PM Sakari Ailus wrote:
>> On 03/15/17 13:28, Rafael J. Wysocki wrote:
>>> On Wednesday, March 15, 2017 11:33:35 AM Sakari Ailus wrote:
>>>> On 03/15/17 10:23, Sakari Ailus wrote:
>>>>> On Wed, Mar 15, 2017 at 12:13:45AM +0100, Rafael J. Wysocki wrote:
>>>>>> On Tue, Mar 14, 2017 at 11:53 PM, Sakari Ailus
>>>>>> <sakari.ailus@linux.intel.com> wrote:
>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>
>>>>>>>> On Tue, Mar 14, 2017 at 10:16 PM, Sakari Ailus
>>>>>>>> <sakari.ailus@linux.intel.com> wrote:
>>>>>>>>>
>>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
>>>>>>>>>> <sakari.ailus@linux.intel.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Hi Rafael,
>>>>>>>>>>>
>>>>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
>>>>>>>>>>>> <sakari.ailus@linux.intel.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 03/14/17 10:08, Sakari Ailus wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> How about this instead:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> All port nodes are located under the device's "_DSD" node in the
>>>>>>>>>>>>>> hierarchical data extension tree. The property extension related to
>>>>>>>>>>>>>> each port node must contain the key "port" and an integer value
>>>>>>>>>>>>>> which
>>>>>>>>>>>>>> is the number of the port.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> So with matching strings instead of indices, this will change, too...
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
>>>>>>>>>>>> only need a boolean property saying "this is a port", so you know that
>>>>>>>>>>>> you should expect a list of endpoints in that object.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> No, it's not redundant. It's the number of the physical port in the
>>>>>>>>>>> device
>>>>>>>>>>> --- this is how the driver gets to know where the connection has been
>>>>>>>>>>> made.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> OK, but what exactly do you mean by "physical port"?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The device (or an IP block) has physical interfaces to the world outside.
>>>>>>>>> There could be just one, but there may be more. For an ISP, there could
>>>>>>>>> be
>>>>>>>>> e.g. four CSI-2 receivers to each of which you could connect a camera
>>>>>>>>> sensor. So for an ISP device, that number tells which of the receivers a
>>>>>>>>> given sensor is connected to.
>>>>>>>>>
>>>>>>>>> The mapping between this number and what the hardware datasheet refers to
>>>>>>>>> needs to be documented per device.
>>>>>>>>
>>>>>>>>
>>>>>>>> OK, so the number actually is an arbitrary piece of data associated
>>>>>>>> with the key "port" and the interpretation of that piece of data
>>>>>>>> depends on whoever asks for that value.
>>>>>>>>
>>>>>>>> IOW, the core doesn't care.
>>>>>>>>
>>>>>>>> With all due respect to whoever invented this on the DT side, this is
>>>>>>>> just bad design to me, because it causes the "port" property to serve
>>>>>>>> two different purposes at the same time.  First, it tells the core
>>>>>>>> that this object is a port.  Second, it is expected to provide a piece
>>>>>>>> of data of unspecified interpretation to somebody.  Which means that
>>>>>>>> the "port" property is both general and device-specific at the same
>>>>>>>> time and the sanity of that is quite questionable IMO.
>>>>>>>
>>>>>>>
>>>>>>> DT uses a node called either "port" or "ports" to store the port nodes. The
>>>>>>> reg property tells the number of the port (see
>>>>>>> Documentation/devicetree/bindings/graph.txt). I'm no DT expert, but my
>>>>>>> understanding is that the node namespace is different from the property
>>>>>>> namespace.
>>>>>>
>>>>>> So on the DT side it actually looks OK to me.
>>>>>>
>>>>>> And the <reg> value is referred to as the port-endpoint identifier, so
>>>>>> I guess this is used for referring to the port/endpoint instead of an
>>>>>> index or the key value somewhere?
>>>>>
>>>>> The remote-endpoint uses phandles; they're a mechanism in DT to refer to
>>>>> different nodes in the tree (DT does not differentiate between devices and
>>>>> non-device nodes). There's a relatively good example here:
>>>>>
>>>>> 	arch/arm/boot/dts/omap3-n9.dts
>>>
>>> Yes, I figured that out. :-)
>>>
>>> Still, the <rev> property value is used for something somewhere in the code I gather.
>>>
>>>>>>
>>>>>>> If you're concerned of possible double meanings, it's entirely possible to
>>>>>>> put the port nodes under hierarchical data extension named e.g. "ports", and
>>>>>>> document that this is what the node must be called (single port node could
>>>>>>> be just called "port"). This way, it should be much more difficult to
>>>>>>> interpret a non-port node as a port node --- roughly equivalent of the DT
>>>>>>> ports node.
>>>>>>>
>>>>>>> The drawback with this change is that the size of the data structure in ASL
>>>>>>> (and AML) will grow.
>>>>>>
>>>>>> The "ports" thing would only be useful if we had the other properties
>>>>>> to put in there.
>>>>>>
>>>>>> So I guess we can specify that the "port" property value is the
>>>>>> identifier of the port and then we will use this in the
>>>>>> "remote-endpoint" property on the other end instead of an index.
>>>>>
>>>>> Makes sense.
>>>>>
>>>>>>
>>>>>> And analogously for the "enpoint" property value.
>>>>>
>>>>> The endpoint hierarchical data extension node name? There is no endpoint
>>>>> property being used by this version of the set anymore; I removed it as it
>>>>> was redundant. However a human readable endpoint node name can be chosen
>>>>> which that'd be quite practical to identify the endpoint.
>>>>>
>>>>> Then the remote-endpoint properties would be:
>>>>>
>>>>> 	Package () { device, port (integer), endpoint-node-name (string) }
>>>>>
>>>>
>>>> Oh, well... the device is present there already, so the endpoint
>>>> reference would well use an index pretty much equally well. Most of the
>>>> time it'll be zero anyway. I.e.
>>>>
>>>> 	Package() { device, port number (integer), endpoint id (integer }
>>>
>>> Yes, I would do that, and actually not using the index for the endpoint too.
>>>
>>> Let ports and endpoints be symmetrical in that respect, that is a port is
>>> required to have a { "port", id } property and an endpoind is required to
>>> have a { "endpoint", id } property and let the ids be used in
>>> "remote-endpoint" properties as per the above.
>>>
>>> Then, in each case, the id would be whatever the value of the <rev> property
>>> on the DT side would be.
>>
>> DT graphs only have port numbers, the endpoints are not referred to by
>> IDs --- just phandles. The closest equivalent we have in ACPI is a
>> device reference as far as I can tell, and this is why the port number
>> and some identifier for the endpoint is required.
>>
>> Adding a numeric ID for the endpoint is somewhat artificial. Most would
>> just have zero there as there are commonly only a single endpoint in a
>> port. And if there were more than one, one of the most sensible
>> approaches to number them would be a monotonically incrementing number
>> from zero --- which is the same than the index.
>>
>> So my view is that adding an endpoint property simply adds no value.
> 
> But endpoints can have a <rev> property in DT.  What is it used for then?

Ah, right. Indeed there's a reg property for endpoints as well --- I
forgot that because you can omit the property if its value is zero,
which is mostly the case for endpoints. The number after @ is used at
least to give a node a unique name. The custom is that the number is the
same than the value of the reg property.

The reg property of an endpoint node is stored in struct of_endpoint.id
when the endpoint is parsed for the port number and the endpoint ID but
I've never seen the endpoint ID being used. It's an identifier for
software to refer to in the remote-endpoint reference elsewhere, and
thus the array index could be used equally well IMHO.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 11/16] of: No need to include linux/property.h, linux/fwnode.h is sufficient

[Sakari Ailus]

On Mon, Mar 13, 2017 at 10:46:32PM +0100, Rafael J. Wysocki wrote:
> On Monday, March 06, 2017 04:19:25 PM Sakari Ailus wrote:
> > of.h requires a definition of struct fwnode_handle, and for that it
> > includes linux/property.h. struct fwnode_handle, however, is defined in
> > linux/fwnode.h. Include linux/fwnode.h directly.
> > 
> > A number of users were however depending on linux/property.h, thus fix
> > them by including that header directly as well.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> How exactly is this related to the rest of the series?  Do any other patches in
> the series depend on it?

It did; keeping dev_fwnode() in property.c instead of moving it to the
header removes the need for the patch. I'll drop it.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Rafael J. Wysocki]

On Wed, Mar 15, 2017 at 1:26 PM, Sakari Ailus
<sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> On 03/15/17 13:53, Rafael J. Wysocki wrote:
>> On Wednesday, March 15, 2017 01:45:48 PM Sakari Ailus wrote:
>>> On 03/15/17 13:28, Rafael J. Wysocki wrote:
>>>> On Wednesday, March 15, 2017 11:33:35 AM Sakari Ailus wrote:
>>>>> On 03/15/17 10:23, Sakari Ailus wrote:
>>>>>> On Wed, Mar 15, 2017 at 12:13:45AM +0100, Rafael J. Wysocki wrote:
>>>>>>> On Tue, Mar 14, 2017 at 11:53 PM, Sakari Ailus
>>>>>>> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Mar 14, 2017 at 10:16 PM, Sakari Ailus
>>>>>>>>> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>>>>>>>>
>>>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
>>>>>>>>>>> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Rafael,
>>>>>>>>>>>>
>>>>>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
>>>>>>>>>>>>> <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 03/14/17 10:08, Sakari Ailus wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> How about this instead:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> All port nodes are located under the device's "_DSD" node in the
>>>>>>>>>>>>>>> hierarchical data extension tree. The property extension related to
>>>>>>>>>>>>>>> each port node must contain the key "port" and an integer value
>>>>>>>>>>>>>>> which
>>>>>>>>>>>>>>> is the number of the port.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So with matching strings instead of indices, this will change, too...
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
>>>>>>>>>>>>> only need a boolean property saying "this is a port", so you know that
>>>>>>>>>>>>> you should expect a list of endpoints in that object.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> No, it's not redundant. It's the number of the physical port in the
>>>>>>>>>>>> device
>>>>>>>>>>>> --- this is how the driver gets to know where the connection has been
>>>>>>>>>>>> made.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> OK, but what exactly do you mean by "physical port"?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The device (or an IP block) has physical interfaces to the world outside.
>>>>>>>>>> There could be just one, but there may be more. For an ISP, there could
>>>>>>>>>> be
>>>>>>>>>> e.g. four CSI-2 receivers to each of which you could connect a camera
>>>>>>>>>> sensor. So for an ISP device, that number tells which of the receivers a
>>>>>>>>>> given sensor is connected to.
>>>>>>>>>>
>>>>>>>>>> The mapping between this number and what the hardware datasheet refers to
>>>>>>>>>> needs to be documented per device.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> OK, so the number actually is an arbitrary piece of data associated
>>>>>>>>> with the key "port" and the interpretation of that piece of data
>>>>>>>>> depends on whoever asks for that value.
>>>>>>>>>
>>>>>>>>> IOW, the core doesn't care.
>>>>>>>>>
>>>>>>>>> With all due respect to whoever invented this on the DT side, this is
>>>>>>>>> just bad design to me, because it causes the "port" property to serve
>>>>>>>>> two different purposes at the same time.  First, it tells the core
>>>>>>>>> that this object is a port.  Second, it is expected to provide a piece
>>>>>>>>> of data of unspecified interpretation to somebody.  Which means that
>>>>>>>>> the "port" property is both general and device-specific at the same
>>>>>>>>> time and the sanity of that is quite questionable IMO.
>>>>>>>>
>>>>>>>>
>>>>>>>> DT uses a node called either "port" or "ports" to store the port nodes. The
>>>>>>>> reg property tells the number of the port (see
>>>>>>>> Documentation/devicetree/bindings/graph.txt). I'm no DT expert, but my
>>>>>>>> understanding is that the node namespace is different from the property
>>>>>>>> namespace.
>>>>>>>
>>>>>>> So on the DT side it actually looks OK to me.
>>>>>>>
>>>>>>> And the <reg> value is referred to as the port-endpoint identifier, so
>>>>>>> I guess this is used for referring to the port/endpoint instead of an
>>>>>>> index or the key value somewhere?
>>>>>>
>>>>>> The remote-endpoint uses phandles; they're a mechanism in DT to refer to
>>>>>> different nodes in the tree (DT does not differentiate between devices and
>>>>>> non-device nodes). There's a relatively good example here:
>>>>>>
>>>>>>   arch/arm/boot/dts/omap3-n9.dts
>>>>
>>>> Yes, I figured that out. :-)
>>>>
>>>> Still, the <rev> property value is used for something somewhere in the code I gather.
>>>>
>>>>>>>
>>>>>>>> If you're concerned of possible double meanings, it's entirely possible to
>>>>>>>> put the port nodes under hierarchical data extension named e.g. "ports", and
>>>>>>>> document that this is what the node must be called (single port node could
>>>>>>>> be just called "port"). This way, it should be much more difficult to
>>>>>>>> interpret a non-port node as a port node --- roughly equivalent of the DT
>>>>>>>> ports node.
>>>>>>>>
>>>>>>>> The drawback with this change is that the size of the data structure in ASL
>>>>>>>> (and AML) will grow.
>>>>>>>
>>>>>>> The "ports" thing would only be useful if we had the other properties
>>>>>>> to put in there.
>>>>>>>
>>>>>>> So I guess we can specify that the "port" property value is the
>>>>>>> identifier of the port and then we will use this in the
>>>>>>> "remote-endpoint" property on the other end instead of an index.
>>>>>>
>>>>>> Makes sense.
>>>>>>
>>>>>>>
>>>>>>> And analogously for the "enpoint" property value.
>>>>>>
>>>>>> The endpoint hierarchical data extension node name? There is no endpoint
>>>>>> property being used by this version of the set anymore; I removed it as it
>>>>>> was redundant. However a human readable endpoint node name can be chosen
>>>>>> which that'd be quite practical to identify the endpoint.
>>>>>>
>>>>>> Then the remote-endpoint properties would be:
>>>>>>
>>>>>>   Package () { device, port (integer), endpoint-node-name (string) }
>>>>>>
>>>>>
>>>>> Oh, well... the device is present there already, so the endpoint
>>>>> reference would well use an index pretty much equally well. Most of the
>>>>> time it'll be zero anyway. I.e.
>>>>>
>>>>>    Package() { device, port number (integer), endpoint id (integer }
>>>>
>>>> Yes, I would do that, and actually not using the index for the endpoint too.
>>>>
>>>> Let ports and endpoints be symmetrical in that respect, that is a port is
>>>> required to have a { "port", id } property and an endpoind is required to
>>>> have a { "endpoint", id } property and let the ids be used in
>>>> "remote-endpoint" properties as per the above.
>>>>
>>>> Then, in each case, the id would be whatever the value of the <rev> property
>>>> on the DT side would be.
>>>
>>> DT graphs only have port numbers, the endpoints are not referred to by
>>> IDs --- just phandles. The closest equivalent we have in ACPI is a
>>> device reference as far as I can tell, and this is why the port number
>>> and some identifier for the endpoint is required.
>>>
>>> Adding a numeric ID for the endpoint is somewhat artificial. Most would
>>> just have zero there as there are commonly only a single endpoint in a
>>> port. And if there were more than one, one of the most sensible
>>> approaches to number them would be a monotonically incrementing number
>>> from zero --- which is the same than the index.
>>>
>>> So my view is that adding an endpoint property simply adds no value.
>>
>> But endpoints can have a <rev> property in DT.  What is it used for then?
>
> Ah, right. Indeed there's a reg property for endpoints as well --- I
> forgot that because you can omit the property if its value is zero,
> which is mostly the case for endpoints. The number after @ is used at
> least to give a node a unique name. The custom is that the number is the
> same than the value of the reg property.
>
> The reg property of an endpoint node is stored in struct of_endpoint.id
> when the endpoint is parsed for the port number and the endpoint ID but
> I've never seen the endpoint ID being used. It's an identifier for
> software to refer to in the remote-endpoint reference elsewhere, and
> thus the array index could be used equally well IMHO.

No, the array index is error prone, that's my point.

Say, two people work on ASL at different times.  One of them sets
everything properly and then the other one removes one of the
endpoints (say, the first one) such that the indexes change, but
doesn't update the remote-endpoint properties somewhere else (eg. by
mistake or lack of experience etc).  Mess ensues.

If you use an identifier instead (the key or a separate one, like the
"endpoint" property value), this particular problem cannot happen.

So, please don't design things to rely on array indexes.

Now, you can use the key (endpoint name) for that too, but having
different conventions for endpoints and ports would be confusing,
wouldn't it?

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 16/16] ACPI / DSD: Document references, ports and endpoints

[Sakari Ailus]

On Wed, Mar 15, 2017 at 03:21:45PM +0100, Rafael J. Wysocki wrote:
> On Wed, Mar 15, 2017 at 1:26 PM, Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > On 03/15/17 13:53, Rafael J. Wysocki wrote:
> >> On Wednesday, March 15, 2017 01:45:48 PM Sakari Ailus wrote:
> >>> On 03/15/17 13:28, Rafael J. Wysocki wrote:
> >>>> On Wednesday, March 15, 2017 11:33:35 AM Sakari Ailus wrote:
> >>>>> On 03/15/17 10:23, Sakari Ailus wrote:
> >>>>>> On Wed, Mar 15, 2017 at 12:13:45AM +0100, Rafael J. Wysocki wrote:
> >>>>>>> On Tue, Mar 14, 2017 at 11:53 PM, Sakari Ailus
> >>>>>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>>>>> Rafael J. Wysocki wrote:
> >>>>>>>>>
> >>>>>>>>> On Tue, Mar 14, 2017 at 10:16 PM, Sakari Ailus
> >>>>>>>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Rafael J. Wysocki wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
> >>>>>>>>>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> Hi Rafael,
> >>>>>>>>>>>>
> >>>>>>>>>>>> Rafael J. Wysocki wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
> >>>>>>>>>>>>> <sakari.ailus@linux.intel.com> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On 03/14/17 10:08, Sakari Ailus wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> How about this instead:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> All port nodes are located under the device's "_DSD" node in the
> >>>>>>>>>>>>>>> hierarchical data extension tree. The property extension related to
> >>>>>>>>>>>>>>> each port node must contain the key "port" and an integer value
> >>>>>>>>>>>>>>> which
> >>>>>>>>>>>>>>> is the number of the port.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> So with matching strings instead of indices, this will change, too...
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> It doesn't have to AFAICS, but the number is just redundant IMO.  You
> >>>>>>>>>>>>> only need a boolean property saying "this is a port", so you know that
> >>>>>>>>>>>>> you should expect a list of endpoints in that object.
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> No, it's not redundant. It's the number of the physical port in the
> >>>>>>>>>>>> device
> >>>>>>>>>>>> --- this is how the driver gets to know where the connection has been
> >>>>>>>>>>>> made.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> OK, but what exactly do you mean by "physical port"?
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> The device (or an IP block) has physical interfaces to the world outside.
> >>>>>>>>>> There could be just one, but there may be more. For an ISP, there could
> >>>>>>>>>> be
> >>>>>>>>>> e.g. four CSI-2 receivers to each of which you could connect a camera
> >>>>>>>>>> sensor. So for an ISP device, that number tells which of the receivers a
> >>>>>>>>>> given sensor is connected to.
> >>>>>>>>>>
> >>>>>>>>>> The mapping between this number and what the hardware datasheet refers to
> >>>>>>>>>> needs to be documented per device.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> OK, so the number actually is an arbitrary piece of data associated
> >>>>>>>>> with the key "port" and the interpretation of that piece of data
> >>>>>>>>> depends on whoever asks for that value.
> >>>>>>>>>
> >>>>>>>>> IOW, the core doesn't care.
> >>>>>>>>>
> >>>>>>>>> With all due respect to whoever invented this on the DT side, this is
> >>>>>>>>> just bad design to me, because it causes the "port" property to serve
> >>>>>>>>> two different purposes at the same time.  First, it tells the core
> >>>>>>>>> that this object is a port.  Second, it is expected to provide a piece
> >>>>>>>>> of data of unspecified interpretation to somebody.  Which means that
> >>>>>>>>> the "port" property is both general and device-specific at the same
> >>>>>>>>> time and the sanity of that is quite questionable IMO.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> DT uses a node called either "port" or "ports" to store the port nodes. The
> >>>>>>>> reg property tells the number of the port (see
> >>>>>>>> Documentation/devicetree/bindings/graph.txt). I'm no DT expert, but my
> >>>>>>>> understanding is that the node namespace is different from the property
> >>>>>>>> namespace.
> >>>>>>>
> >>>>>>> So on the DT side it actually looks OK to me.
> >>>>>>>
> >>>>>>> And the <reg> value is referred to as the port-endpoint identifier, so
> >>>>>>> I guess this is used for referring to the port/endpoint instead of an
> >>>>>>> index or the key value somewhere?
> >>>>>>
> >>>>>> The remote-endpoint uses phandles; they're a mechanism in DT to refer to
> >>>>>> different nodes in the tree (DT does not differentiate between devices and
> >>>>>> non-device nodes). There's a relatively good example here:
> >>>>>>
> >>>>>>   arch/arm/boot/dts/omap3-n9.dts
> >>>>
> >>>> Yes, I figured that out. :-)
> >>>>
> >>>> Still, the <rev> property value is used for something somewhere in the code I gather.
> >>>>
> >>>>>>>
> >>>>>>>> If you're concerned of possible double meanings, it's entirely possible to
> >>>>>>>> put the port nodes under hierarchical data extension named e.g. "ports", and
> >>>>>>>> document that this is what the node must be called (single port node could
> >>>>>>>> be just called "port"). This way, it should be much more difficult to
> >>>>>>>> interpret a non-port node as a port node --- roughly equivalent of the DT
> >>>>>>>> ports node.
> >>>>>>>>
> >>>>>>>> The drawback with this change is that the size of the data structure in ASL
> >>>>>>>> (and AML) will grow.
> >>>>>>>
> >>>>>>> The "ports" thing would only be useful if we had the other properties
> >>>>>>> to put in there.
> >>>>>>>
> >>>>>>> So I guess we can specify that the "port" property value is the
> >>>>>>> identifier of the port and then we will use this in the
> >>>>>>> "remote-endpoint" property on the other end instead of an index.
> >>>>>>
> >>>>>> Makes sense.
> >>>>>>
> >>>>>>>
> >>>>>>> And analogously for the "enpoint" property value.
> >>>>>>
> >>>>>> The endpoint hierarchical data extension node name? There is no endpoint
> >>>>>> property being used by this version of the set anymore; I removed it as it
> >>>>>> was redundant. However a human readable endpoint node name can be chosen
> >>>>>> which that'd be quite practical to identify the endpoint.
> >>>>>>
> >>>>>> Then the remote-endpoint properties would be:
> >>>>>>
> >>>>>>   Package () { device, port (integer), endpoint-node-name (string) }
> >>>>>>
> >>>>>
> >>>>> Oh, well... the device is present there already, so the endpoint
> >>>>> reference would well use an index pretty much equally well. Most of the
> >>>>> time it'll be zero anyway. I.e.
> >>>>>
> >>>>>    Package() { device, port number (integer), endpoint id (integer }
> >>>>
> >>>> Yes, I would do that, and actually not using the index for the endpoint too.
> >>>>
> >>>> Let ports and endpoints be symmetrical in that respect, that is a port is
> >>>> required to have a { "port", id } property and an endpoind is required to
> >>>> have a { "endpoint", id } property and let the ids be used in
> >>>> "remote-endpoint" properties as per the above.
> >>>>
> >>>> Then, in each case, the id would be whatever the value of the <rev> property
> >>>> on the DT side would be.
> >>>
> >>> DT graphs only have port numbers, the endpoints are not referred to by
> >>> IDs --- just phandles. The closest equivalent we have in ACPI is a
> >>> device reference as far as I can tell, and this is why the port number
> >>> and some identifier for the endpoint is required.
> >>>
> >>> Adding a numeric ID for the endpoint is somewhat artificial. Most would
> >>> just have zero there as there are commonly only a single endpoint in a
> >>> port. And if there were more than one, one of the most sensible
> >>> approaches to number them would be a monotonically incrementing number
> >>> from zero --- which is the same than the index.
> >>>
> >>> So my view is that adding an endpoint property simply adds no value.
> >>
> >> But endpoints can have a <rev> property in DT.  What is it used for then?
> >
> > Ah, right. Indeed there's a reg property for endpoints as well --- I
> > forgot that because you can omit the property if its value is zero,
> > which is mostly the case for endpoints. The number after @ is used at
> > least to give a node a unique name. The custom is that the number is the
> > same than the value of the reg property.
> >
> > The reg property of an endpoint node is stored in struct of_endpoint.id
> > when the endpoint is parsed for the port number and the endpoint ID but
> > I've never seen the endpoint ID being used. It's an identifier for
> > software to refer to in the remote-endpoint reference elsewhere, and
> > thus the array index could be used equally well IMHO.
> 
> No, the array index is error prone, that's my point.
> 
> Say, two people work on ASL at different times.  One of them sets
> everything properly and then the other one removes one of the
> endpoints (say, the first one) such that the indexes change, but
> doesn't update the remote-endpoint properties somewhere else (eg. by
> mistake or lack of experience etc).  Mess ensues.
> 
> If you use an identifier instead (the key or a separate one, like the
> "endpoint" property value), this particular problem cannot happen.
> 
> So, please don't design things to rely on array indexes.
> 
> Now, you can use the key (endpoint name) for that too, but having
> different conventions for endpoints and ports would be confusing,
> wouldn't it?

Fair points, I admit. I'll change the implementation accordingly.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v4 14/16] of: Add nop implementation of of_get_next_parent()

[Sakari Ailus]

Hi Rafael,

On Mon, Mar 13, 2017 at 10:55:01PM +0100, Rafael J. Wysocki wrote:
> On Monday, March 06, 2017 04:19:28 PM Sakari Ailus wrote:
> > To avoid #ifdefs where the function is used.
> 
> A bit more explanation?
> 
> About where this happens (or will happen) to be used with CONFIG_OF unset?

v5.1 has:

of_get_next_parent() was only defined if CONFIG_OF was set. Using the
function thus requires #ifdef CONFIG_OF ... #endif in all cases. Avoid
having to use pre-processor macros around the function by providing a nop
implementation of of_get_next_parent().

> 
> Plus, that would require an ACK from Rob.

I checked with Rob (cc'd) that he's fine with the patch and that it goes
through your tree.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk