[RFC/PATCH 0/5] Support children for legacy device properties

[RFC/PATCH 0/5] Support children for legacy device properties

[Dmitry Torokhov]

The generic device properties APIs are very helpful as they allow abstracting
away details of the platform (whether it is ACPI, device tree, or legacy board
file), so that individual driver does not need separate code paths to support
all variants. However there are drivers that currently can not use generic
device properties API as they need notion of children properties, for example
gpio_keys driver, that expects every button to be described as a sub-node of
main device.

This patch series introduces notion of sub-nodes for static properties and ties
it up with GPIO lookup tables so that they are usable with sub-nodes as well.

Thanks.

-- 
Dmitry

[RFC/PATCH 1/5] device property: split generic properties and property sets

[Dmitry Torokhov]

drivers/base/property.c became a bit of a kitchen sink, with code handling
generic device/fwnode properties intermingled with code handling static
properties (pset). Let's split them apart to not pollute each other.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/Makefile        |   4 +-
 drivers/base/property.c      | 525 ----------------------------------
 drivers/base/pset_property.c | 540 +++++++++++++++++++++++++++++++++++
 3 files changed, 542 insertions(+), 527 deletions(-)
 create mode 100644 drivers/base/pset_property.c

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 704f44295810..5f72cd5ab8ba 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -5,8 +5,8 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
-			   topology.o container.o property.o cacheinfo.o \
-			   devcon.o
+			   topology.o container.o cacheinfo.o \
+			   property.o pset_property.o devcon.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-y			+= power/
 obj-$(CONFIG_ISA_BUS_API)	+= isa.o
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 240ab5230ff6..d57df94e396c 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -18,236 +18,6 @@
 #include <linux/etherdevice.h>
 #include <linux/phy.h>
 
-struct property_set {
-	struct device *dev;
-	struct fwnode_handle fwnode;
-	const struct property_entry *properties;
-};
-
-static const struct fwnode_operations pset_fwnode_ops;
-
-static inline bool is_pset_node(const struct fwnode_handle *fwnode)
-{
-	return !IS_ERR_OR_NULL(fwnode) && fwnode->ops == &pset_fwnode_ops;
-}
-
-#define to_pset_node(__fwnode)						\
-	({								\
-		typeof(__fwnode) __to_pset_node_fwnode = __fwnode;	\
-									\
-		is_pset_node(__to_pset_node_fwnode) ?			\
-			container_of(__to_pset_node_fwnode,		\
-				     struct property_set, fwnode) :	\
-			NULL;						\
-	})
-
-static const struct property_entry *
-pset_prop_get(const struct property_set *pset, const char *name)
-{
-	const struct property_entry *prop;
-
-	if (!pset || !pset->properties)
-		return NULL;
-
-	for (prop = pset->properties; prop->name; prop++)
-		if (!strcmp(name, prop->name))
-			return prop;
-
-	return NULL;
-}
-
-static const void *property_get_pointer(const struct property_entry *prop)
-{
-	switch (prop->type) {
-	case DEV_PROP_U8:
-		if (prop->is_array)
-			return prop->pointer.u8_data;
-		return &prop->value.u8_data;
-	case DEV_PROP_U16:
-		if (prop->is_array)
-			return prop->pointer.u16_data;
-		return &prop->value.u16_data;
-	case DEV_PROP_U32:
-		if (prop->is_array)
-			return prop->pointer.u32_data;
-		return &prop->value.u32_data;
-	case DEV_PROP_U64:
-		if (prop->is_array)
-			return prop->pointer.u64_data;
-		return &prop->value.u64_data;
-	case DEV_PROP_STRING:
-		if (prop->is_array)
-			return prop->pointer.str;
-		return &prop->value.str;
-	default:
-		return NULL;
-	}
-}
-
-static void property_set_pointer(struct property_entry *prop, const void *pointer)
-{
-	switch (prop->type) {
-	case DEV_PROP_U8:
-		if (prop->is_array)
-			prop->pointer.u8_data = pointer;
-		else
-			prop->value.u8_data = *((u8 *)pointer);
-		break;
-	case DEV_PROP_U16:
-		if (prop->is_array)
-			prop->pointer.u16_data = pointer;
-		else
-			prop->value.u16_data = *((u16 *)pointer);
-		break;
-	case DEV_PROP_U32:
-		if (prop->is_array)
-			prop->pointer.u32_data = pointer;
-		else
-			prop->value.u32_data = *((u32 *)pointer);
-		break;
-	case DEV_PROP_U64:
-		if (prop->is_array)
-			prop->pointer.u64_data = pointer;
-		else
-			prop->value.u64_data = *((u64 *)pointer);
-		break;
-	case DEV_PROP_STRING:
-		if (prop->is_array)
-			prop->pointer.str = pointer;
-		else
-			prop->value.str = pointer;
-		break;
-	default:
-		break;
-	}
-}
-
-static const void *pset_prop_find(const struct property_set *pset,
-				  const char *propname, size_t length)
-{
-	const struct property_entry *prop;
-	const void *pointer;
-
-	prop = pset_prop_get(pset, propname);
-	if (!prop)
-		return ERR_PTR(-EINVAL);
-	pointer = property_get_pointer(prop);
-	if (!pointer)
-		return ERR_PTR(-ENODATA);
-	if (length > prop->length)
-		return ERR_PTR(-EOVERFLOW);
-	return pointer;
-}
-
-static int pset_prop_read_u8_array(const struct property_set *pset,
-				   const char *propname,
-				   u8 *values, size_t nval)
-{
-	const void *pointer;
-	size_t length = nval * sizeof(*values);
-
-	pointer = pset_prop_find(pset, propname, length);
-	if (IS_ERR(pointer))
-		return PTR_ERR(pointer);
-
-	memcpy(values, pointer, length);
-	return 0;
-}
-
-static int pset_prop_read_u16_array(const struct property_set *pset,
-				    const char *propname,
-				    u16 *values, size_t nval)
-{
-	const void *pointer;
-	size_t length = nval * sizeof(*values);
-
-	pointer = pset_prop_find(pset, propname, length);
-	if (IS_ERR(pointer))
-		return PTR_ERR(pointer);
-
-	memcpy(values, pointer, length);
-	return 0;
-}
-
-static int pset_prop_read_u32_array(const struct property_set *pset,
-				    const char *propname,
-				    u32 *values, size_t nval)
-{
-	const void *pointer;
-	size_t length = nval * sizeof(*values);
-
-	pointer = pset_prop_find(pset, propname, length);
-	if (IS_ERR(pointer))
-		return PTR_ERR(pointer);
-
-	memcpy(values, pointer, length);
-	return 0;
-}
-
-static int pset_prop_read_u64_array(const struct property_set *pset,
-				    const char *propname,
-				    u64 *values, size_t nval)
-{
-	const void *pointer;
-	size_t length = nval * sizeof(*values);
-
-	pointer = pset_prop_find(pset, propname, length);
-	if (IS_ERR(pointer))
-		return PTR_ERR(pointer);
-
-	memcpy(values, pointer, length);
-	return 0;
-}
-
-static int pset_prop_count_elems_of_size(const struct property_set *pset,
-					 const char *propname, size_t length)
-{
-	const struct property_entry *prop;
-
-	prop = pset_prop_get(pset, propname);
-	if (!prop)
-		return -EINVAL;
-
-	return prop->length / length;
-}
-
-static int pset_prop_read_string_array(const struct property_set *pset,
-				       const char *propname,
-				       const char **strings, size_t nval)
-{
-	const struct property_entry *prop;
-	const void *pointer;
-	size_t array_len, length;
-
-	/* Find out the array length. */
-	prop = pset_prop_get(pset, propname);
-	if (!prop)
-		return -EINVAL;
-
-	if (!prop->is_array)
-		/* The array length for a non-array string property is 1. */
-		array_len = 1;
-	else
-		/* Find the length of an array. */
-		array_len = pset_prop_count_elems_of_size(pset, propname,
-							  sizeof(const char *));
-
-	/* Return how many there are if strings is NULL. */
-	if (!strings)
-		return array_len;
-
-	array_len = min(nval, array_len);
-	length = array_len * sizeof(*strings);
-
-	pointer = pset_prop_find(pset, propname, length);
-	if (IS_ERR(pointer))
-		return PTR_ERR(pointer);
-
-	memcpy(strings, pointer, length);
-
-	return array_len;
-}
-
 struct fwnode_handle *dev_fwnode(struct device *dev)
 {
 	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
@@ -255,51 +25,6 @@ struct fwnode_handle *dev_fwnode(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_fwnode);
 
-static bool pset_fwnode_property_present(const struct fwnode_handle *fwnode,
-					 const char *propname)
-{
-	return !!pset_prop_get(to_pset_node(fwnode), propname);
-}
-
-static int pset_fwnode_read_int_array(const struct fwnode_handle *fwnode,
-				      const char *propname,
-				      unsigned int elem_size, void *val,
-				      size_t nval)
-{
-	const struct property_set *node = to_pset_node(fwnode);
-
-	if (!val)
-		return pset_prop_count_elems_of_size(node, propname, elem_size);
-
-	switch (elem_size) {
-	case sizeof(u8):
-		return pset_prop_read_u8_array(node, propname, val, nval);
-	case sizeof(u16):
-		return pset_prop_read_u16_array(node, propname, val, nval);
-	case sizeof(u32):
-		return pset_prop_read_u32_array(node, propname, val, nval);
-	case sizeof(u64):
-		return pset_prop_read_u64_array(node, propname, val, nval);
-	}
-
-	return -ENXIO;
-}
-
-static int
-pset_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
-				       const char *propname,
-				       const char **val, size_t nval)
-{
-	return pset_prop_read_string_array(to_pset_node(fwnode), propname,
-					   val, nval);
-}
-
-static const struct fwnode_operations pset_fwnode_ops = {
-	.property_present = pset_fwnode_property_present,
-	.property_read_int_array = pset_fwnode_read_int_array,
-	.property_read_string_array = pset_fwnode_property_read_string_array,
-};
-
 /**
  * device_property_present - check if a property of a device is present
  * @dev: Device whose property is being checked
@@ -759,256 +484,6 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
 
-static void property_entry_free_data(const struct property_entry *p)
-{
-	const void *pointer = property_get_pointer(p);
-	size_t i, nval;
-
-	if (p->is_array) {
-		if (p->type == DEV_PROP_STRING && p->pointer.str) {
-			nval = p->length / sizeof(const char *);
-			for (i = 0; i < nval; i++)
-				kfree(p->pointer.str[i]);
-		}
-		kfree(pointer);
-	} else if (p->type == DEV_PROP_STRING) {
-		kfree(p->value.str);
-	}
-	kfree(p->name);
-}
-
-static int property_copy_string_array(struct property_entry *dst,
-				      const struct property_entry *src)
-{
-	const char **d;
-	size_t nval = src->length / sizeof(*d);
-	int i;
-
-	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
-	if (!d)
-		return -ENOMEM;
-
-	for (i = 0; i < nval; i++) {
-		d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL);
-		if (!d[i] && src->pointer.str[i]) {
-			while (--i >= 0)
-				kfree(d[i]);
-			kfree(d);
-			return -ENOMEM;
-		}
-	}
-
-	dst->pointer.str = d;
-	return 0;
-}
-
-static int property_entry_copy_data(struct property_entry *dst,
-				    const struct property_entry *src)
-{
-	const void *pointer = property_get_pointer(src);
-	const void *new;
-	int error;
-
-	if (src->is_array) {
-		if (!src->length)
-			return -ENODATA;
-
-		if (src->type == DEV_PROP_STRING) {
-			error = property_copy_string_array(dst, src);
-			if (error)
-				return error;
-			new = dst->pointer.str;
-		} else {
-			new = kmemdup(pointer, src->length, GFP_KERNEL);
-			if (!new)
-				return -ENOMEM;
-		}
-	} else if (src->type == DEV_PROP_STRING) {
-		new = kstrdup(src->value.str, GFP_KERNEL);
-		if (!new && src->value.str)
-			return -ENOMEM;
-	} else {
-		new = pointer;
-	}
-
-	dst->length = src->length;
-	dst->is_array = src->is_array;
-	dst->type = src->type;
-
-	property_set_pointer(dst, new);
-
-	dst->name = kstrdup(src->name, GFP_KERNEL);
-	if (!dst->name)
-		goto out_free_data;
-
-	return 0;
-
-out_free_data:
-	property_entry_free_data(dst);
-	return -ENOMEM;
-}
-
-/**
- * property_entries_dup - duplicate array of properties
- * @properties: array of properties to copy
- *
- * This function creates a deep copy of the given NULL-terminated array
- * of property entries.
- */
-struct property_entry *
-property_entries_dup(const struct property_entry *properties)
-{
-	struct property_entry *p;
-	int i, n = 0;
-
-	while (properties[n].name)
-		n++;
-
-	p = kcalloc(n + 1, sizeof(*p), GFP_KERNEL);
-	if (!p)
-		return ERR_PTR(-ENOMEM);
-
-	for (i = 0; i < n; i++) {
-		int ret = property_entry_copy_data(&p[i], &properties[i]);
-		if (ret) {
-			while (--i >= 0)
-				property_entry_free_data(&p[i]);
-			kfree(p);
-			return ERR_PTR(ret);
-		}
-	}
-
-	return p;
-}
-EXPORT_SYMBOL_GPL(property_entries_dup);
-
-/**
- * property_entries_free - free previously allocated array of properties
- * @properties: array of properties to destroy
- *
- * This function frees given NULL-terminated array of property entries,
- * along with their data.
- */
-void property_entries_free(const struct property_entry *properties)
-{
-	const struct property_entry *p;
-
-	for (p = properties; p->name; p++)
-		property_entry_free_data(p);
-
-	kfree(properties);
-}
-EXPORT_SYMBOL_GPL(property_entries_free);
-
-/**
- * pset_free_set - releases memory allocated for copied property set
- * @pset: Property set to release
- *
- * Function takes previously copied property set and releases all the
- * memory allocated to it.
- */
-static void pset_free_set(struct property_set *pset)
-{
-	if (!pset)
-		return;
-
-	property_entries_free(pset->properties);
-	kfree(pset);
-}
-
-/**
- * pset_copy_set - copies property set
- * @pset: Property set to copy
- *
- * This function takes a deep copy of the given property set and returns
- * pointer to the copy. Call device_free_property_set() to free resources
- * allocated in this function.
- *
- * Return: Pointer to the new property set or error pointer.
- */
-static struct property_set *pset_copy_set(const struct property_set *pset)
-{
-	struct property_entry *properties;
-	struct property_set *p;
-
-	p = kzalloc(sizeof(*p), GFP_KERNEL);
-	if (!p)
-		return ERR_PTR(-ENOMEM);
-
-	properties = property_entries_dup(pset->properties);
-	if (IS_ERR(properties)) {
-		kfree(p);
-		return ERR_CAST(properties);
-	}
-
-	p->properties = properties;
-	return p;
-}
-
-/**
- * device_remove_properties - Remove properties from a device object.
- * @dev: Device whose properties to remove.
- *
- * The function removes properties previously associated to the device
- * secondary firmware node with device_add_properties(). Memory allocated
- * to the properties will also be released.
- */
-void device_remove_properties(struct device *dev)
-{
-	struct fwnode_handle *fwnode;
-	struct property_set *pset;
-
-	fwnode = dev_fwnode(dev);
-	if (!fwnode)
-		return;
-	/*
-	 * Pick either primary or secondary node depending which one holds
-	 * the pset. If there is no real firmware node (ACPI/DT) primary
-	 * will hold the pset.
-	 */
-	pset = to_pset_node(fwnode);
-	if (pset) {
-		set_primary_fwnode(dev, NULL);
-	} else {
-		pset = to_pset_node(fwnode->secondary);
-		if (pset && dev == pset->dev)
-			set_secondary_fwnode(dev, NULL);
-	}
-	if (pset && dev == pset->dev)
-		pset_free_set(pset);
-}
-EXPORT_SYMBOL_GPL(device_remove_properties);
-
-/**
- * device_add_properties - Add a collection of properties to a device object.
- * @dev: Device to add properties to.
- * @properties: Collection of properties to add.
- *
- * Associate a collection of device properties represented by @properties with
- * @dev as its secondary firmware node. The function takes a copy of
- * @properties.
- */
-int device_add_properties(struct device *dev,
-			  const struct property_entry *properties)
-{
-	struct property_set *p, pset;
-
-	if (!properties)
-		return -EINVAL;
-
-	pset.properties = properties;
-
-	p = pset_copy_set(&pset);
-	if (IS_ERR(p))
-		return PTR_ERR(p);
-
-	p->fwnode.ops = &pset_fwnode_ops;
-	set_secondary_fwnode(dev, &p->fwnode);
-	p->dev = dev;
-	return 0;
-}
-EXPORT_SYMBOL_GPL(device_add_properties);
-
 /**
  * fwnode_get_next_parent - Iterate to the node's parent
  * @fwnode: Firmware whose parent is retrieved
diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
new file mode 100644
index 000000000000..08ecc13080ae
--- /dev/null
+++ b/drivers/base/pset_property.c
@@ -0,0 +1,540 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Handling of device properties defined in legacy board files.
+ *
+ * Copyright (C) 2014, Intel Corporation
+ * Authors: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *          Mika Westerberg <mika.westerberg@linux.intel.com>
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+struct property_set {
+	struct device *dev;
+	struct fwnode_handle fwnode;
+	const struct property_entry *properties;
+};
+
+static const struct fwnode_operations pset_fwnode_ops;
+
+static inline bool is_pset_node(const struct fwnode_handle *fwnode)
+{
+	return !IS_ERR_OR_NULL(fwnode) && fwnode->ops == &pset_fwnode_ops;
+}
+
+#define to_pset_node(__fwnode)						\
+	({								\
+		typeof(__fwnode) __to_pset_node_fwnode = __fwnode;	\
+									\
+		is_pset_node(__to_pset_node_fwnode) ?			\
+			container_of(__to_pset_node_fwnode,		\
+				     struct property_set, fwnode) :	\
+			NULL;						\
+	})
+
+static const struct property_entry *
+pset_prop_get(const struct property_set *pset, const char *name)
+{
+	const struct property_entry *prop;
+
+	if (!pset || !pset->properties)
+		return NULL;
+
+	for (prop = pset->properties; prop->name; prop++)
+		if (!strcmp(name, prop->name))
+			return prop;
+
+	return NULL;
+}
+
+static const void *property_get_pointer(const struct property_entry *prop)
+{
+	switch (prop->type) {
+	case DEV_PROP_U8:
+		if (prop->is_array)
+			return prop->pointer.u8_data;
+		return &prop->value.u8_data;
+	case DEV_PROP_U16:
+		if (prop->is_array)
+			return prop->pointer.u16_data;
+		return &prop->value.u16_data;
+	case DEV_PROP_U32:
+		if (prop->is_array)
+			return prop->pointer.u32_data;
+		return &prop->value.u32_data;
+	case DEV_PROP_U64:
+		if (prop->is_array)
+			return prop->pointer.u64_data;
+		return &prop->value.u64_data;
+	case DEV_PROP_STRING:
+		if (prop->is_array)
+			return prop->pointer.str;
+		return &prop->value.str;
+	default:
+		return NULL;
+	}
+}
+
+static void property_set_pointer(struct property_entry *prop, const void *pointer)
+{
+	switch (prop->type) {
+	case DEV_PROP_U8:
+		if (prop->is_array)
+			prop->pointer.u8_data = pointer;
+		else
+			prop->value.u8_data = *((u8 *)pointer);
+		break;
+	case DEV_PROP_U16:
+		if (prop->is_array)
+			prop->pointer.u16_data = pointer;
+		else
+			prop->value.u16_data = *((u16 *)pointer);
+		break;
+	case DEV_PROP_U32:
+		if (prop->is_array)
+			prop->pointer.u32_data = pointer;
+		else
+			prop->value.u32_data = *((u32 *)pointer);
+		break;
+	case DEV_PROP_U64:
+		if (prop->is_array)
+			prop->pointer.u64_data = pointer;
+		else
+			prop->value.u64_data = *((u64 *)pointer);
+		break;
+	case DEV_PROP_STRING:
+		if (prop->is_array)
+			prop->pointer.str = pointer;
+		else
+			prop->value.str = pointer;
+		break;
+	default:
+		break;
+	}
+}
+
+static const void *pset_prop_find(const struct property_set *pset,
+				  const char *propname, size_t length)
+{
+	const struct property_entry *prop;
+	const void *pointer;
+
+	prop = pset_prop_get(pset, propname);
+	if (!prop)
+		return ERR_PTR(-EINVAL);
+	pointer = property_get_pointer(prop);
+	if (!pointer)
+		return ERR_PTR(-ENODATA);
+	if (length > prop->length)
+		return ERR_PTR(-EOVERFLOW);
+	return pointer;
+}
+
+static int pset_prop_read_u8_array(const struct property_set *pset,
+				   const char *propname,
+				   u8 *values, size_t nval)
+{
+	const void *pointer;
+	size_t length = nval * sizeof(*values);
+
+	pointer = pset_prop_find(pset, propname, length);
+	if (IS_ERR(pointer))
+		return PTR_ERR(pointer);
+
+	memcpy(values, pointer, length);
+	return 0;
+}
+
+static int pset_prop_read_u16_array(const struct property_set *pset,
+				    const char *propname,
+				    u16 *values, size_t nval)
+{
+	const void *pointer;
+	size_t length = nval * sizeof(*values);
+
+	pointer = pset_prop_find(pset, propname, length);
+	if (IS_ERR(pointer))
+		return PTR_ERR(pointer);
+
+	memcpy(values, pointer, length);
+	return 0;
+}
+
+static int pset_prop_read_u32_array(const struct property_set *pset,
+				    const char *propname,
+				    u32 *values, size_t nval)
+{
+	const void *pointer;
+	size_t length = nval * sizeof(*values);
+
+	pointer = pset_prop_find(pset, propname, length);
+	if (IS_ERR(pointer))
+		return PTR_ERR(pointer);
+
+	memcpy(values, pointer, length);
+	return 0;
+}
+
+static int pset_prop_read_u64_array(const struct property_set *pset,
+				    const char *propname,
+				    u64 *values, size_t nval)
+{
+	const void *pointer;
+	size_t length = nval * sizeof(*values);
+
+	pointer = pset_prop_find(pset, propname, length);
+	if (IS_ERR(pointer))
+		return PTR_ERR(pointer);
+
+	memcpy(values, pointer, length);
+	return 0;
+}
+
+static int pset_prop_count_elems_of_size(const struct property_set *pset,
+					 const char *propname, size_t length)
+{
+	const struct property_entry *prop;
+
+	prop = pset_prop_get(pset, propname);
+	if (!prop)
+		return -EINVAL;
+
+	return prop->length / length;
+}
+
+static int pset_prop_read_string_array(const struct property_set *pset,
+				       const char *propname,
+				       const char **strings, size_t nval)
+{
+	const struct property_entry *prop;
+	const void *pointer;
+	size_t array_len, length;
+
+	/* Find out the array length. */
+	prop = pset_prop_get(pset, propname);
+	if (!prop)
+		return -EINVAL;
+
+	if (!prop->is_array)
+		/* The array length for a non-array string property is 1. */
+		array_len = 1;
+	else
+		/* Find the length of an array. */
+		array_len = pset_prop_count_elems_of_size(pset, propname,
+							  sizeof(const char *));
+
+	/* Return how many there are if strings is NULL. */
+	if (!strings)
+		return array_len;
+
+	array_len = min(nval, array_len);
+	length = array_len * sizeof(*strings);
+
+	pointer = pset_prop_find(pset, propname, length);
+	if (IS_ERR(pointer))
+		return PTR_ERR(pointer);
+
+	memcpy(strings, pointer, length);
+
+	return array_len;
+}
+
+static bool pset_fwnode_property_present(const struct fwnode_handle *fwnode,
+					 const char *propname)
+{
+	return !!pset_prop_get(to_pset_node(fwnode), propname);
+}
+
+static int pset_fwnode_read_int_array(const struct fwnode_handle *fwnode,
+				      const char *propname,
+				      unsigned int elem_size, void *val,
+				      size_t nval)
+{
+	const struct property_set *node = to_pset_node(fwnode);
+
+	if (!val)
+		return pset_prop_count_elems_of_size(node, propname, elem_size);
+
+	switch (elem_size) {
+	case sizeof(u8):
+		return pset_prop_read_u8_array(node, propname, val, nval);
+	case sizeof(u16):
+		return pset_prop_read_u16_array(node, propname, val, nval);
+	case sizeof(u32):
+		return pset_prop_read_u32_array(node, propname, val, nval);
+	case sizeof(u64):
+		return pset_prop_read_u64_array(node, propname, val, nval);
+	}
+
+	return -ENXIO;
+}
+
+static int
+pset_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
+				       const char *propname,
+				       const char **val, size_t nval)
+{
+	return pset_prop_read_string_array(to_pset_node(fwnode), propname,
+					   val, nval);
+}
+
+static const struct fwnode_operations pset_fwnode_ops = {
+	.property_present = pset_fwnode_property_present,
+	.property_read_int_array = pset_fwnode_read_int_array,
+	.property_read_string_array = pset_fwnode_property_read_string_array,
+};
+
+static void property_entry_free_data(const struct property_entry *p)
+{
+	const void *pointer = property_get_pointer(p);
+	size_t i, nval;
+
+	if (p->is_array) {
+		if (p->type == DEV_PROP_STRING && p->pointer.str) {
+			nval = p->length / sizeof(const char *);
+			for (i = 0; i < nval; i++)
+				kfree(p->pointer.str[i]);
+		}
+		kfree(pointer);
+	} else if (p->type == DEV_PROP_STRING) {
+		kfree(p->value.str);
+	}
+	kfree(p->name);
+}
+
+static int property_copy_string_array(struct property_entry *dst,
+				      const struct property_entry *src)
+{
+	const char **d;
+	size_t nval = src->length / sizeof(*d);
+	int i;
+
+	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	for (i = 0; i < nval; i++) {
+		d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL);
+		if (!d[i] && src->pointer.str[i]) {
+			while (--i >= 0)
+				kfree(d[i]);
+			kfree(d);
+			return -ENOMEM;
+		}
+	}
+
+	dst->pointer.str = d;
+	return 0;
+}
+
+static int property_entry_copy_data(struct property_entry *dst,
+				    const struct property_entry *src)
+{
+	const void *pointer = property_get_pointer(src);
+	const void *new;
+	int error;
+
+	if (src->is_array) {
+		if (!src->length)
+			return -ENODATA;
+
+		if (src->type == DEV_PROP_STRING) {
+			error = property_copy_string_array(dst, src);
+			if (error)
+				return error;
+			new = dst->pointer.str;
+		} else {
+			new = kmemdup(pointer, src->length, GFP_KERNEL);
+			if (!new)
+				return -ENOMEM;
+		}
+	} else if (src->type == DEV_PROP_STRING) {
+		new = kstrdup(src->value.str, GFP_KERNEL);
+		if (!new && src->value.str)
+			return -ENOMEM;
+	} else {
+		new = pointer;
+	}
+
+	dst->length = src->length;
+	dst->is_array = src->is_array;
+	dst->type = src->type;
+
+	property_set_pointer(dst, new);
+
+	dst->name = kstrdup(src->name, GFP_KERNEL);
+	if (!dst->name)
+		goto out_free_data;
+
+	return 0;
+
+out_free_data:
+	property_entry_free_data(dst);
+	return -ENOMEM;
+}
+
+/**
+ * property_entries_dup - duplicate array of properties
+ * @properties: array of properties to copy
+ *
+ * This function creates a deep copy of the given NULL-terminated array
+ * of property entries.
+ */
+struct property_entry *
+property_entries_dup(const struct property_entry *properties)
+{
+	struct property_entry *p;
+	int i, n = 0;
+
+	while (properties[n].name)
+		n++;
+
+	p = kcalloc(n + 1, sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return ERR_PTR(-ENOMEM);
+
+	for (i = 0; i < n; i++) {
+		int ret = property_entry_copy_data(&p[i], &properties[i]);
+		if (ret) {
+			while (--i >= 0)
+				property_entry_free_data(&p[i]);
+			kfree(p);
+			return ERR_PTR(ret);
+		}
+	}
+
+	return p;
+}
+EXPORT_SYMBOL_GPL(property_entries_dup);
+
+/**
+ * property_entries_free - free previously allocated array of properties
+ * @properties: array of properties to destroy
+ *
+ * This function frees given NULL-terminated array of property entries,
+ * along with their data.
+ */
+void property_entries_free(const struct property_entry *properties)
+{
+	const struct property_entry *p;
+
+	for (p = properties; p->name; p++)
+		property_entry_free_data(p);
+
+	kfree(properties);
+}
+EXPORT_SYMBOL_GPL(property_entries_free);
+
+/**
+ * pset_free_set - releases memory allocated for copied property set
+ * @pset: Property set to release
+ *
+ * Function takes previously copied property set and releases all the
+ * memory allocated to it.
+ */
+static void pset_free_set(struct property_set *pset)
+{
+	if (!pset)
+		return;
+
+	property_entries_free(pset->properties);
+	kfree(pset);
+}
+
+/**
+ * pset_copy_set - copies property set
+ * @pset: Property set to copy
+ *
+ * This function takes a deep copy of the given property set and returns
+ * pointer to the copy. Call device_free_property_set() to free resources
+ * allocated in this function.
+ *
+ * Return: Pointer to the new property set or error pointer.
+ */
+static struct property_set *pset_copy_set(const struct property_set *pset)
+{
+	struct property_entry *properties;
+	struct property_set *p;
+
+	p = kzalloc(sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return ERR_PTR(-ENOMEM);
+
+	properties = property_entries_dup(pset->properties);
+	if (IS_ERR(properties)) {
+		kfree(p);
+		return ERR_CAST(properties);
+	}
+
+	p->properties = properties;
+	return p;
+}
+
+/**
+ * device_remove_properties - Remove properties from a device object.
+ * @dev: Device whose properties to remove.
+ *
+ * The function removes properties previously associated to the device
+ * secondary firmware node with device_add_properties(). Memory allocated
+ * to the properties will also be released.
+ */
+void device_remove_properties(struct device *dev)
+{
+	struct fwnode_handle *fwnode;
+	struct property_set *pset;
+
+	fwnode = dev_fwnode(dev);
+	if (!fwnode)
+		return;
+	/*
+	 * Pick either primary or secondary node depending which one holds
+	 * the pset. If there is no real firmware node (ACPI/DT) primary
+	 * will hold the pset.
+	 */
+	pset = to_pset_node(fwnode);
+	if (pset) {
+		set_primary_fwnode(dev, NULL);
+	} else {
+		pset = to_pset_node(fwnode->secondary);
+		if (pset && dev == pset->dev)
+			set_secondary_fwnode(dev, NULL);
+	}
+	if (pset && dev == pset->dev)
+		pset_free_set(pset);
+}
+EXPORT_SYMBOL_GPL(device_remove_properties);
+
+/**
+ * device_add_properties - Add a collection of properties to a device object.
+ * @dev: Device to add properties to.
+ * @properties: Collection of properties to add.
+ *
+ * Associate a collection of device properties represented by @properties with
+ * @dev as its secondary firmware node. The function takes a copy of
+ * @properties.
+ */
+int device_add_properties(struct device *dev,
+			  const struct property_entry *properties)
+{
+	struct property_set *p, pset;
+
+	if (!properties)
+		return -EINVAL;
+
+	pset.properties = properties;
+
+	p = pset_copy_set(&pset);
+	if (IS_ERR(p))
+		return PTR_ERR(p);
+
+	p->fwnode.ops = &pset_fwnode_ops;
+	set_secondary_fwnode(dev, &p->fwnode);
+	p->dev = dev;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_add_properties);
-- 
2.19.0.397.gdd90340f6a-goog

[RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Dmitry Torokhov]

Several drivers rely on having notion of sub-nodes when describing
hardware, let's allow static board-defined properties also have it.

The board files will then attach properties to devices in the following
fashion:

	device_add_properties(&board_platform_device.dev,
			      main_device_props);
	device_add_child_properties(&board_platform_device.dev,
				    dev_fwnode(&board_platform_device.dev),
				    child1_device_props);
	device_add_child_properties(&board_platform_device.dev,
				    dev_fwnode(&board_platform_device.dev),
				    child2_device_props);
	...
	platform_device_register(&board_platform_device.dev);

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/pset_property.c | 110 +++++++++++++++++++++++++++++++----
 include/linux/property.h     |   4 ++
 2 files changed, 102 insertions(+), 12 deletions(-)

diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
index 08ecc13080ae..63f2377aefe8 100644
--- a/drivers/base/pset_property.c
+++ b/drivers/base/pset_property.c
@@ -18,6 +18,11 @@ struct property_set {
 	struct device *dev;
 	struct fwnode_handle fwnode;
 	const struct property_entry *properties;
+
+	struct property_set *parent;
+	/* Entry in parent->children list */
+	struct list_head child_node;
+	struct list_head children;
 };
 
 static const struct fwnode_operations pset_fwnode_ops;
@@ -283,10 +288,47 @@ pset_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 					   val, nval);
 }
 
+struct fwnode_handle *pset_fwnode_get_parent(const struct fwnode_handle *fwnode)
+{
+	struct property_set *pset = to_pset_node(fwnode);
+
+	return pset ? &pset->parent->fwnode : NULL;
+}
+
+struct fwnode_handle *
+pset_fwnode_get_next_subnode(const struct fwnode_handle *fwnode,
+			     struct fwnode_handle *child)
+{
+	const struct property_set *pset = to_pset_node(fwnode);
+	struct property_set *first_child;
+	struct property_set *next;
+
+	if (!pset)
+		return NULL;
+
+	if (list_empty(&pset->children))
+		return NULL;
+
+	first_child = list_first_entry(&pset->children, struct property_set,
+				       child_node);
+
+	if (child) {
+		next = list_next_entry(to_pset_node(child), child_node);
+		if (next == first_child)
+			return NULL;
+	} else {
+		next = first_child;
+	}
+
+	return &next->fwnode;
+}
+
 static const struct fwnode_operations pset_fwnode_ops = {
 	.property_present = pset_fwnode_property_present,
 	.property_read_int_array = pset_fwnode_read_int_array,
 	.property_read_string_array = pset_fwnode_property_read_string_array,
+	.get_parent = pset_fwnode_get_parent,
+	.get_next_child_node = pset_fwnode_get_next_subnode,
 };
 
 static void property_entry_free_data(const struct property_entry *p)
@@ -439,24 +481,31 @@ EXPORT_SYMBOL_GPL(property_entries_free);
  */
 static void pset_free_set(struct property_set *pset)
 {
+	struct property_set *child, *next;
+
 	if (!pset)
 		return;
 
+	list_for_each_entry_safe(child, next, &pset->children, child_node) {
+		list_del(&child->child_node);
+		pset_free_set(child);
+	}
+
 	property_entries_free(pset->properties);
 	kfree(pset);
 }
 
 /**
- * pset_copy_set - copies property set
- * @pset: Property set to copy
+ * pset_create_set - creates property set.
+ * @src: property entries for the set.
  *
- * This function takes a deep copy of the given property set and returns
- * pointer to the copy. Call device_free_property_set() to free resources
- * allocated in this function.
+ * This function takes a deep copy of the given property entries and creates
+ * property set. Call pset_free_set() to free resources allocated in this
+ * function.
  *
  * Return: Pointer to the new property set or error pointer.
  */
-static struct property_set *pset_copy_set(const struct property_set *pset)
+static struct property_set *pset_create_set(const struct property_entry *src)
 {
 	struct property_entry *properties;
 	struct property_set *p;
@@ -465,7 +514,11 @@ static struct property_set *pset_copy_set(const struct property_set *pset)
 	if (!p)
 		return ERR_PTR(-ENOMEM);
 
-	properties = property_entries_dup(pset->properties);
+	INIT_LIST_HEAD(&p->child_node);
+	INIT_LIST_HEAD(&p->children);
+	p->fwnode.ops = &pset_fwnode_ops;
+
+	properties = property_entries_dup(src);
 	if (IS_ERR(properties)) {
 		kfree(p);
 		return ERR_CAST(properties);
@@ -521,20 +574,53 @@ EXPORT_SYMBOL_GPL(device_remove_properties);
 int device_add_properties(struct device *dev,
 			  const struct property_entry *properties)
 {
-	struct property_set *p, pset;
+	struct property_set *p;
 
 	if (!properties)
 		return -EINVAL;
 
-	pset.properties = properties;
-
-	p = pset_copy_set(&pset);
+	p = pset_create_set(properties);
 	if (IS_ERR(p))
 		return PTR_ERR(p);
 
-	p->fwnode.ops = &pset_fwnode_ops;
 	set_secondary_fwnode(dev, &p->fwnode);
 	p->dev = dev;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(device_add_properties);
+
+/**
+ * device_add_child_properties - Add a collection of properties to a device object.
+ * @dev: Device to add properties to.
+ * @properties: Collection of properties to add.
+ *
+ * Associate a collection of device properties represented by @properties as a
+ * child of given @parent firmware node.  The function takes a copy of
+ * @properties.
+ */
+struct fwnode_handle *
+device_add_child_properties(struct device *dev,
+			    struct fwnode_handle *parent,
+			    const struct property_entry *properties)
+{
+	struct property_set *p;
+	struct property_set *parent_pset;
+
+	if (!properties)
+		return ERR_PTR(-EINVAL);
+
+	parent_pset = to_pset_node(parent);
+	if (!parent_pset)
+		return ERR_PTR(-EINVAL);
+
+	p = pset_create_set(properties);
+	if (IS_ERR(p))
+		return ERR_CAST(p);
+
+	p->dev = dev;
+	p->parent = parent_pset;
+	list_add_tail(&p->child_node, &parent_pset->children);
+
+	return &p->fwnode;
+}
+EXPORT_SYMBOL_GPL(device_add_child_properties);
diff --git a/include/linux/property.h b/include/linux/property.h
index ac8a1ebc4c1b..bb1cf4f30770 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -275,6 +275,10 @@ void property_entries_free(const struct property_entry *properties);
 
 int device_add_properties(struct device *dev,
 			  const struct property_entry *properties);
+struct fwnode_handle *
+device_add_child_properties(struct device *dev,
+			    struct fwnode_handle *parent,
+			    const struct property_entry *properties);
 void device_remove_properties(struct device *dev);
 
 bool device_dma_supported(struct device *dev);
-- 
2.19.0.397.gdd90340f6a-goog

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Heikki Krogerus]

Hi,

On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote:
> Several drivers rely on having notion of sub-nodes when describing
> hardware, let's allow static board-defined properties also have it.
> 
> The board files will then attach properties to devices in the following
> fashion:
> 
> 	device_add_properties(&board_platform_device.dev,
> 			      main_device_props);
> 	device_add_child_properties(&board_platform_device.dev,
> 				    dev_fwnode(&board_platform_device.dev),
> 				    child1_device_props);
> 	device_add_child_properties(&board_platform_device.dev,
> 				    dev_fwnode(&board_platform_device.dev),
> 				    child2_device_props);
> 	...
> 	platform_device_register(&board_platform_device.dev);
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/base/pset_property.c | 110 +++++++++++++++++++++++++++++++----
>  include/linux/property.h     |   4 ++
>  2 files changed, 102 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
> index 08ecc13080ae..63f2377aefe8 100644
> --- a/drivers/base/pset_property.c
> +++ b/drivers/base/pset_property.c
> @@ -18,6 +18,11 @@ struct property_set {
>  	struct device *dev;
>  	struct fwnode_handle fwnode;
>  	const struct property_entry *properties;
> +
> +	struct property_set *parent;
> +	/* Entry in parent->children list */
> +	struct list_head child_node;
> +	struct list_head children;

Add

        const char *name;

and you can implement also pset_get_named_child_node().

>  };
>  
>  static const struct fwnode_operations pset_fwnode_ops;
> @@ -283,10 +288,47 @@ pset_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
>  					   val, nval);
>  }
>  
> +struct fwnode_handle *pset_fwnode_get_parent(const struct fwnode_handle *fwnode)
> +{
> +	struct property_set *pset = to_pset_node(fwnode);
> +
> +	return pset ? &pset->parent->fwnode : NULL;
> +}
> +
> +struct fwnode_handle *
> +pset_fwnode_get_next_subnode(const struct fwnode_handle *fwnode,
> +			     struct fwnode_handle *child)
> +{
> +	const struct property_set *pset = to_pset_node(fwnode);
> +	struct property_set *first_child;
> +	struct property_set *next;
> +
> +	if (!pset)
> +		return NULL;
> +
> +	if (list_empty(&pset->children))
> +		return NULL;
> +
> +	first_child = list_first_entry(&pset->children, struct property_set,
> +				       child_node);
> +
> +	if (child) {
> +		next = list_next_entry(to_pset_node(child), child_node);
> +		if (next == first_child)
> +			return NULL;
> +	} else {
> +		next = first_child;
> +	}
> +
> +	return &next->fwnode;
> +}
> +
>  static const struct fwnode_operations pset_fwnode_ops = {
>  	.property_present = pset_fwnode_property_present,
>  	.property_read_int_array = pset_fwnode_read_int_array,
>  	.property_read_string_array = pset_fwnode_property_read_string_array,
> +	.get_parent = pset_fwnode_get_parent,
> +	.get_next_child_node = pset_fwnode_get_next_subnode,
>  };
>  
>  static void property_entry_free_data(const struct property_entry *p)
> @@ -439,24 +481,31 @@ EXPORT_SYMBOL_GPL(property_entries_free);
>   */
>  static void pset_free_set(struct property_set *pset)
>  {
> +	struct property_set *child, *next;
> +
>  	if (!pset)
>  		return;
>  
> +	list_for_each_entry_safe(child, next, &pset->children, child_node) {
> +		list_del(&child->child_node);
> +		pset_free_set(child);
> +	}
> +
>  	property_entries_free(pset->properties);
>  	kfree(pset);
>  }
>  
>  /**
> - * pset_copy_set - copies property set
> - * @pset: Property set to copy
> + * pset_create_set - creates property set.
> + * @src: property entries for the set.
>   *
> - * This function takes a deep copy of the given property set and returns
> - * pointer to the copy. Call device_free_property_set() to free resources
> - * allocated in this function.
> + * This function takes a deep copy of the given property entries and creates
> + * property set. Call pset_free_set() to free resources allocated in this
> + * function.
>   *
>   * Return: Pointer to the new property set or error pointer.
>   */
> -static struct property_set *pset_copy_set(const struct property_set *pset)
> +static struct property_set *pset_create_set(const struct property_entry *src)
>  {
>  	struct property_entry *properties;
>  	struct property_set *p;
> @@ -465,7 +514,11 @@ static struct property_set *pset_copy_set(const struct property_set *pset)
>  	if (!p)
>  		return ERR_PTR(-ENOMEM);
>  
> -	properties = property_entries_dup(pset->properties);
> +	INIT_LIST_HEAD(&p->child_node);
> +	INIT_LIST_HEAD(&p->children);
> +	p->fwnode.ops = &pset_fwnode_ops;
> +
> +	properties = property_entries_dup(src);
>  	if (IS_ERR(properties)) {
>  		kfree(p);
>  		return ERR_CAST(properties);
> @@ -521,20 +574,53 @@ EXPORT_SYMBOL_GPL(device_remove_properties);
>  int device_add_properties(struct device *dev,
>  			  const struct property_entry *properties)
>  {
> -	struct property_set *p, pset;
> +	struct property_set *p;
>  
>  	if (!properties)
>  		return -EINVAL;
>  
> -	pset.properties = properties;
> -
> -	p = pset_copy_set(&pset);
> +	p = pset_create_set(properties);
>  	if (IS_ERR(p))
>  		return PTR_ERR(p);
>  
> -	p->fwnode.ops = &pset_fwnode_ops;
>  	set_secondary_fwnode(dev, &p->fwnode);
>  	p->dev = dev;
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(device_add_properties);
> +
> +/**
> + * device_add_child_properties - Add a collection of properties to a device object.
> + * @dev: Device to add properties to.

@parent: missing

> + * @properties: Collection of properties to add.
> + *
> + * Associate a collection of device properties represented by @properties as a
> + * child of given @parent firmware node.  The function takes a copy of
> + * @properties.
> + */
> +struct fwnode_handle *
> +device_add_child_properties(struct device *dev,

device_add_child_properties(struct device *dev, const char *child_name,

> +			    struct fwnode_handle *parent,
> +			    const struct property_entry *properties)
> +{
> +	struct property_set *p;
> +	struct property_set *parent_pset;
> +
> +	if (!properties)
> +		return ERR_PTR(-EINVAL);
> +
> +	parent_pset = to_pset_node(parent);
> +	if (!parent_pset)
> +		return ERR_PTR(-EINVAL);
> +
> +	p = pset_create_set(properties);
> +	if (IS_ERR(p))
> +		return ERR_CAST(p);
> +
> +	p->dev = dev;

        p->name = kstrdup_const(child_name, GFP_KERNEL);

You'll need to kfree_const(pset->name) in pset_free_set() of course.

> +	p->parent = parent_pset;
> +	list_add_tail(&p->child_node, &parent_pset->children);
> +
> +	return &p->fwnode;
> +}
> +EXPORT_SYMBOL_GPL(device_add_child_properties);


Br,

-- 
heikki

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Dmitry Torokhov]

Hi Heikki,

On Wed, Sep 19, 2018 at 06:10:26PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote:
> > Several drivers rely on having notion of sub-nodes when describing
> > hardware, let's allow static board-defined properties also have it.
> > 
> > The board files will then attach properties to devices in the following
> > fashion:
> > 
> > 	device_add_properties(&board_platform_device.dev,
> > 			      main_device_props);
> > 	device_add_child_properties(&board_platform_device.dev,
> > 				    dev_fwnode(&board_platform_device.dev),
> > 				    child1_device_props);
> > 	device_add_child_properties(&board_platform_device.dev,
> > 				    dev_fwnode(&board_platform_device.dev),
> > 				    child2_device_props);
> > 	...
> > 	platform_device_register(&board_platform_device.dev);
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/base/pset_property.c | 110 +++++++++++++++++++++++++++++++----
> >  include/linux/property.h     |   4 ++
> >  2 files changed, 102 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
> > index 08ecc13080ae..63f2377aefe8 100644
> > --- a/drivers/base/pset_property.c
> > +++ b/drivers/base/pset_property.c
> > @@ -18,6 +18,11 @@ struct property_set {
> >  	struct device *dev;
> >  	struct fwnode_handle fwnode;
> >  	const struct property_entry *properties;
> > +
> > +	struct property_set *parent;
> > +	/* Entry in parent->children list */
> > +	struct list_head child_node;
> > +	struct list_head children;
> 
> Add
> 
>         const char *name;
> 
> and you can implement also pset_get_named_child_node().

Or
	char name[];

to avoid separate allocation.

Alternatively, we can add it later when we need it, and add
device_add_named_child_properties().

I'll leave it up to Rafael to decide.

Thanks.

-- 
Dmitry

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Heikki Krogerus]

On Wed, Sep 19, 2018 at 10:13:26AM -0700, Dmitry Torokhov wrote:
> > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
> > > index 08ecc13080ae..63f2377aefe8 100644
> > > --- a/drivers/base/pset_property.c
> > > +++ b/drivers/base/pset_property.c
> > > @@ -18,6 +18,11 @@ struct property_set {
> > >  	struct device *dev;
> > >  	struct fwnode_handle fwnode;
> > >  	const struct property_entry *properties;
> > > +
> > > +	struct property_set *parent;
> > > +	/* Entry in parent->children list */
> > > +	struct list_head child_node;
> > > +	struct list_head children;
> > 
> > Add
> > 
> >         const char *name;
> > 
> > and you can implement also pset_get_named_child_node().
> 
> Or
> 	char name[];
> 
> to avoid separate allocation.

Let's not do that, especially if you are planning on exporting this
structure. If the name is coming from .rodata, there is no need to
allocate anything for the name. Check kstrdup_const().

> Alternatively, we can add it later when we need it, and add
> device_add_named_child_properties().
> 
> I'll leave it up to Rafael to decide.

Fair enough.


Thanks,

-- 
heikki

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Dmitry Torokhov]

On Thu, Sep 20, 2018 at 01:16:48PM +0300, Heikki Krogerus wrote:
> On Wed, Sep 19, 2018 at 10:13:26AM -0700, Dmitry Torokhov wrote:
> > > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
> > > > index 08ecc13080ae..63f2377aefe8 100644
> > > > --- a/drivers/base/pset_property.c
> > > > +++ b/drivers/base/pset_property.c
> > > > @@ -18,6 +18,11 @@ struct property_set {
> > > >  	struct device *dev;
> > > >  	struct fwnode_handle fwnode;
> > > >  	const struct property_entry *properties;
> > > > +
> > > > +	struct property_set *parent;
> > > > +	/* Entry in parent->children list */
> > > > +	struct list_head child_node;
> > > > +	struct list_head children;
> > > 
> > > Add
> > > 
> > >         const char *name;
> > > 
> > > and you can implement also pset_get_named_child_node().
> > 
> > Or
> > 	char name[];
> > 
> > to avoid separate allocation.
> 
> Let's not do that, especially if you are planning on exporting this
> structure.

Can you please elaborate why? Not using pointer saves us 4/8 bytes +
however much memory we need for bookkeeping for the extra chunk. Given
that majority of pset nodes are unnamed this seems wasteful.

> If the name is coming from .rodata, there is no need to
> allocate anything for the name. Check kstrdup_const().

The data is most likely coming as __initconst so we do need to copy it.

> 
> > Alternatively, we can add it later when we need it, and add
> > device_add_named_child_properties().
> > 
> > I'll leave it up to Rafael to decide.
> 
> Fair enough.
> 
> 
> Thanks,
> 
> -- 
> heikki

Thanks.

-- 
Dmitry

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Heikki Krogerus]

On Fri, Sep 21, 2018 at 04:33:36PM -0700, Dmitry Torokhov wrote:
> On Thu, Sep 20, 2018 at 01:16:48PM +0300, Heikki Krogerus wrote:
> > On Wed, Sep 19, 2018 at 10:13:26AM -0700, Dmitry Torokhov wrote:
> > > > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
> > > > > index 08ecc13080ae..63f2377aefe8 100644
> > > > > --- a/drivers/base/pset_property.c
> > > > > +++ b/drivers/base/pset_property.c
> > > > > @@ -18,6 +18,11 @@ struct property_set {
> > > > >  	struct device *dev;
> > > > >  	struct fwnode_handle fwnode;
> > > > >  	const struct property_entry *properties;
> > > > > +
> > > > > +	struct property_set *parent;
> > > > > +	/* Entry in parent->children list */
> > > > > +	struct list_head child_node;
> > > > > +	struct list_head children;
> > > > 
> > > > Add
> > > > 
> > > >         const char *name;
> > > > 
> > > > and you can implement also pset_get_named_child_node().
> > > 
> > > Or
> > > 	char name[];
> > > 
> > > to avoid separate allocation.
> > 
> > Let's not do that, especially if you are planning on exporting this
> > structure.
> 
> Can you please elaborate why? Not using pointer saves us 4/8 bytes +
> however much memory we need for bookkeeping for the extra chunk. Given
> that majority of pset nodes are unnamed this seems wasteful.
> 
> > If the name is coming from .rodata, there is no need to
> > allocate anything for the name. Check kstrdup_const().
> 
> The data is most likely coming as __initconst so we do need to copy it.

OK, I did not consider that. Yes, it makes sense to always copy.

Thanks,

-- 
heikki

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Heikki Krogerus]

Hi Dmitry,

On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote:
> +/**
> + * device_add_child_properties - Add a collection of properties to a device object.
> + * @dev: Device to add properties to.

In case you didn't notice my comment for this, you are missing @parent
here.

But why do you need both the parent and the dev?

> + * @properties: Collection of properties to add.
> + *
> + * Associate a collection of device properties represented by @properties as a
> + * child of given @parent firmware node.  The function takes a copy of
> + * @properties.
> + */
> +struct fwnode_handle *
> +device_add_child_properties(struct device *dev,
> +			    struct fwnode_handle *parent,
> +			    const struct property_entry *properties)
> +{
> +	struct property_set *p;
> +	struct property_set *parent_pset;
> +
> +	if (!properties)
> +		return ERR_PTR(-EINVAL);
> +
> +	parent_pset = to_pset_node(parent);

For this function, the parent will in practice have to be
dev_fwnode(dev), so I don't think you need @parent at all, no?

There is something wrong here..

> +	if (!parent_pset)
> +		return ERR_PTR(-EINVAL);
> +
> +	p = pset_create_set(properties);
> +	if (IS_ERR(p))
> +		return ERR_CAST(p);
> +
> +	p->dev = dev;

That looks wrong.

I'm guessing the assumption here is that the child nodes will never be
assigned to their own devices, but you can't do that. It will limit
the use of the child nodes to a very small number of cases, possibly
only to gpios.

I think that has to be fixed. It should not be a big deal. Just expect
the child nodes to be removed separately, and add ref counting to the
struct property_set handling.

> +	p->parent = parent_pset;
> +	list_add_tail(&p->child_node, &parent_pset->children);
> +
> +	return &p->fwnode;
> +}
> +EXPORT_SYMBOL_GPL(device_add_child_properties);

The child nodes will change the purpose of the build-in property
support. Originally the goal was just to support adding of build-in
device properties to real firmware nodes, but things have changed
quite a bit from that. These child nodes are purely tied to the
build-in device property support, so we should be talking about adding
pset type child nodes to pset type parent nodes in the API:
fwnode_pset_add_child_node(), or something like that.

I'm sorry to be a bit of pain in the butt with this. The build-in
property support is a hack, it always was. A useful hack, but hack
nevertheless. That means we need to be extra careful when expanding
it, like here.


Thanks,

-- 
heikki

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Linus Walleij]

On Thu, Sep 20, 2018 at 6:53 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:

> The child nodes will change the purpose of the build-in property
> support. Originally the goal was just to support adding of build-in
> device properties to real firmware nodes, but things have changed
> quite a bit from that. These child nodes are purely tied to the
> build-in device property support, so we should be talking about adding
> pset type child nodes to pset type parent nodes in the API:
> fwnode_pset_add_child_node(), or something like that.
>
> I'm sorry to be a bit of pain in the butt with this. The build-in
> property support is a hack, it always was. A useful hack, but hack
> nevertheless. That means we need to be extra careful when expanding
> it, like here.

I dunno how we got to here, what I tried to solve and Dmitry tries
to make more general is converting old boardfiles to use fwnode, because
I initially tried to just support gpio descriptors from board files.

If boardfiles is what you mean with "build-in property support" I don't
know, it predates both device tree and ACPI and any other hardware
descriptions on the ARM architecture, from the perspective of these
systems things are fine and the hardware description languages
are the novelty...

But I guess you know because you worked with OMAP :)

So there is something I don't understand here.

Yours,
Linus Walleij

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Heikki Krogerus]

Hi Linus,

On Fri, Sep 21, 2018 at 08:36:53AM -0700, Linus Walleij wrote:
> On Thu, Sep 20, 2018 at 6:53 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> 
> > The child nodes will change the purpose of the build-in property
> > support. Originally the goal was just to support adding of build-in
> > device properties to real firmware nodes, but things have changed
> > quite a bit from that. These child nodes are purely tied to the
> > build-in device property support, so we should be talking about adding
> > pset type child nodes to pset type parent nodes in the API:
> > fwnode_pset_add_child_node(), or something like that.
> >
> > I'm sorry to be a bit of pain in the butt with this. The build-in
> > property support is a hack, it always was. A useful hack, but hack
> > nevertheless. That means we need to be extra careful when expanding
> > it, like here.
> 
> I dunno how we got to here, what I tried to solve and Dmitry tries
> to make more general is converting old boardfiles to use fwnode, because
> I initially tried to just support gpio descriptors from board files.

I understand that, but what I'm trying to say is that the solution for
the child nodes is not generic enough. I'll try to explain below.

> If boardfiles is what you mean with "build-in property support" I don't
> know, it predates both device tree and ACPI and any other hardware
> descriptions on the ARM architecture, from the perspective of these
> systems things are fine and the hardware description languages
> are the novelty...

Rafael talked about "build-in properties" at one point, and I just
started using that expression after that, but it's probable not the
best term to describe this thing.

But please note that we are not talking about only static information
with these property sets. They can be populated dynamically as well,
so this kind of extensions must consider and support that. On top of
board files we need to consider things like glue and probing drivers
and even buses in some cases.

> But I guess you know because you worked with OMAP :)
> 
> So there is something I don't understand here.

Sorry for the poor choice of words on my behalf. I guess I'm not
explaining my point properly. Let me try again.

So I'm seeing a case that this solution does not seem to be able to
support, but ultimately it will need to do so: with USB ports we are
going to need to be able to assign a device to the child nodes that
represent those ports.

To be honest, normally I would not care about that, and I would simply
wait for this to go into mainline, and then propose a modification to
it so it can also support that other case.

However, this time I feel that we should not do that. The solution for
the child nodes should be implemented so that it can support all the
known cases from the beginning. The reason for that is because I fear
that we'll end up having a pile of ad-hoc style solutions, each
solving an individual problem, and a whole lot of duplicated stuff for
these property sets. In the end we will have spaghetti with meatballs
that nobody is able fully understand nor handle. And I really see a
strong possibility for that to happen here.


Thanks,

-- 
heikki

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Dmitry Torokhov]

Hi Heikki,

On Thu, Sep 20, 2018 at 04:53:48PM +0300, Heikki Krogerus wrote:
> Hi Dmitry,
> 
> On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote:
> > +/**
> > + * device_add_child_properties - Add a collection of properties to a device object.
> > + * @dev: Device to add properties to.
> 
> In case you didn't notice my comment for this, you are missing @parent
> here.
> 
> But why do you need both the parent and the dev?

I could go by parent only and fetch dev from parent.

> 
> > + * @properties: Collection of properties to add.
> > + *
> > + * Associate a collection of device properties represented by @properties as a
> > + * child of given @parent firmware node.  The function takes a copy of
> > + * @properties.
> > + */
> > +struct fwnode_handle *
> > +device_add_child_properties(struct device *dev,
> > +			    struct fwnode_handle *parent,
> > +			    const struct property_entry *properties)
> > +{
> > +	struct property_set *p;
> > +	struct property_set *parent_pset;
> > +
> > +	if (!properties)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	parent_pset = to_pset_node(parent);
> 
> For this function, the parent will in practice have to be
> dev_fwnode(dev), so I don't think you need @parent at all, no?
> 
> There is something wrong here..

Yes, I expect majority of the calls will use dev_fwnode(dev) as parent,
but nobody stops you from doing:

	device_add_properties(dev, props);
	c1 = device_add_child_properties(dev, dev_fwnode(dev), cp1);
	c2 = device_add_child_properties(dev, c1, cp2);
	c3 = device_add_child_properties(dev, c2, cp3);
	...

> 
> > +	if (!parent_pset)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	p = pset_create_set(properties);
> > +	if (IS_ERR(p))
> > +		return ERR_CAST(p);
> > +
> > +	p->dev = dev;
> 
> That looks wrong.
> 
> I'm guessing the assumption here is that the child nodes will never be
> assigned to their own devices, but you can't do that. It will limit
> the use of the child nodes to a very small number of cases, possibly
> only to gpios.

If I need to assign a node to a device I'll use device_add_properties()
API. device_add_child_properties() is for nodes living "below" the
device.

All nodes (the primary/secondary and children) would point to the owning
device, just for convenience.

> 
> I think that has to be fixed. It should not be a big deal. Just expect
> the child nodes to be removed separately, and add ref counting to the
> struct property_set handling.

Why do we need to remove them separately and what do we need refcounting
for?

> 
> > +	p->parent = parent_pset;
> > +	list_add_tail(&p->child_node, &parent_pset->children);
> > +
> > +	return &p->fwnode;
> > +}
> > +EXPORT_SYMBOL_GPL(device_add_child_properties);
> 
> The child nodes will change the purpose of the build-in property
> support. Originally the goal was just to support adding of build-in
> device properties to real firmware nodes, but things have changed
> quite a bit from that. These child nodes are purely tied to the
> build-in device property support, so we should be talking about adding
> pset type child nodes to pset type parent nodes in the API:
> fwnode_pset_add_child_node(), or something like that.

OK, I can change device_add_child_properties() to
fwnode_pset_add_child_node() if Rafael would prefer this name.

Thanks.

-- 
Dmitry

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Heikki Krogerus]

Hi Dmitry,

On Fri, Sep 21, 2018 at 04:31:19PM -0700, Dmitry Torokhov wrote:
> > > +	if (!parent_pset)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	p = pset_create_set(properties);
> > > +	if (IS_ERR(p))
> > > +		return ERR_CAST(p);
> > > +
> > > +	p->dev = dev;
> > 
> > That looks wrong.
> > 
> > I'm guessing the assumption here is that the child nodes will never be
> > assigned to their own devices, but you can't do that. It will limit
> > the use of the child nodes to a very small number of cases, possibly
> > only to gpios.
> 
> If I need to assign a node to a device I'll use device_add_properties()
> API. device_add_child_properties() is for nodes living "below" the
> device.

device_add_properties() is not available to us before we have the
actual struct device meant for the properties. If the child device is
populated outside of the "boardfiles" then we have to be able to link
it to the child node afterwards.

But I took a closer look at the code and realized that you are not
using that p->dev with the child nodes for anything, so I may be wrong
about this. You are not actually linking the child nodes to that
device here.

> All nodes (the primary/secondary and children) would point to the owning
> device, just for convenience.

Since you don't use that for anything, then drop that line. It's only
confusing.

And besides, since we will have separate child devices for those child
nodes, there is a small change that somebody needs to call
device_remove_properties(child_dev). At least then that operation
becomes safe, as it's a nop with the child pset nodes.

> > I think that has to be fixed. It should not be a big deal. Just expect
> > the child nodes to be removed separately, and add ref counting to the
> > struct property_set handling.
> 
> Why do we need to remove them separately and what do we need refcounting
> for?

If you could remove the nodes independently, then obviously the parent
can not be removed before the children. The ref count would be there
to prevent that.

Though my original comment is not valid, I still think that the child
nodes should be created and destroyed separately. Actually, I don't
think we should be talking about child nodes in the API at all. Check
below..

> > > +	p->parent = parent_pset;
> > > +	list_add_tail(&p->child_node, &parent_pset->children);
> > > +
> > > +	return &p->fwnode;
> > > +}
> > > +EXPORT_SYMBOL_GPL(device_add_child_properties);
> > 
> > The child nodes will change the purpose of the build-in property
> > support. Originally the goal was just to support adding of build-in
> > device properties to real firmware nodes, but things have changed
> > quite a bit from that. These child nodes are purely tied to the
> > build-in device property support, so we should be talking about adding
> > pset type child nodes to pset type parent nodes in the API:
> > fwnode_pset_add_child_node(), or something like that.
> 
> OK, I can change device_add_child_properties() to
> fwnode_pset_add_child_node() if Rafael would prefer this name.

So not even that. We should have API like this:

struct fwnode_handle *
fwnode_pset_create(struct fwnode_handle *parent, const char *name,
                   const struct property_entry *props);

void fwnode_pset_destroy(struct fwnode_handle *fwnode);

int device_add_properties(struct device *dev,
                          const struct fwnode_handle *pset_node);

void device_remove_properties(struct device *dev);

So basically we separate creation of the nodes from linking them to the
devices completely. That would give this API much better structure. It
would scale better, for example, it would then be possible to add
child nodes from different locations if needed.


Thanks,

-- 
heikki

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Dmitry Torokhov]

On Mon, Sep 24, 2018 at 04:20:50PM +0300, Heikki Krogerus wrote:
> Hi Dmitry,
> 
> On Fri, Sep 21, 2018 at 04:31:19PM -0700, Dmitry Torokhov wrote:
> > > > +	if (!parent_pset)
> > > > +		return ERR_PTR(-EINVAL);
> > > > +
> > > > +	p = pset_create_set(properties);
> > > > +	if (IS_ERR(p))
> > > > +		return ERR_CAST(p);
> > > > +
> > > > +	p->dev = dev;
> > > 
> > > That looks wrong.
> > > 
> > > I'm guessing the assumption here is that the child nodes will never be
> > > assigned to their own devices, but you can't do that. It will limit
> > > the use of the child nodes to a very small number of cases, possibly
> > > only to gpios.
> > 
> > If I need to assign a node to a device I'll use device_add_properties()
> > API. device_add_child_properties() is for nodes living "below" the
> > device.
> 
> device_add_properties() is not available to us before we have the
> actual struct device meant for the properties. If the child device is
> populated outside of the "boardfiles" then we have to be able to link
> it to the child node afterwards.

I think we are talking about totally different use cases and that is why
we are having hard time coming to a mutually agreeable solution. Could
you please describe in more detail what you would like to achieve,
and preferably show how it is described now with DT and/or ACPI, so that
I have a better frame of reference.

Thanks.

-- 
Dmitry

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Heikki Krogerus]

On Mon, Sep 24, 2018 at 11:45:43AM -0700, Dmitry Torokhov wrote:
> I think we are talking about totally different use cases and that is why
> we are having hard time coming to a mutually agreeable solution. Could
> you please describe in more detail what you would like to achieve,
> and preferably show how it is described now with DT and/or ACPI, so that
> I have a better frame of reference.

Yes, of course. Sorry.

USB ports are devices that usually the USB controller drivers register
(or actually the USB core code). They are represented in both ACPI and
DT as child nodes of the controller device node. The USB connector OF
node is defined in file
Documentation/devicetree/bindings/connector/usb-connector.txt

In short, the controller drivers will request handle to a child node
that represents a port, and only after that register the actual port
device.

The drivers I'm looking at currently are the USB Type-C port
controller drivers and the port manager (in Greg's usb-next or
linux-next):

        drivers/usb/typec/tcpm/tcpci.c
        drivers/usb/typec/tcpm/fusb302.c
        drivers/usb/typec/tcpm/tcpm.c

The goal is simply to get rid of the platform data as usual, and
ideally so that we don't need any extra code in order to support the
"legacy" platforms.


Thanks,

-- 
heikki

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Dmitry Torokhov]

Hi Heikki,

On Tue, Sep 25, 2018 at 03:19:27PM +0300, Heikki Krogerus wrote:
> On Mon, Sep 24, 2018 at 11:45:43AM -0700, Dmitry Torokhov wrote:
> > I think we are talking about totally different use cases and that is why
> > we are having hard time coming to a mutually agreeable solution. Could
> > you please describe in more detail what you would like to achieve,
> > and preferably show how it is described now with DT and/or ACPI, so that
> > I have a better frame of reference.
> 
> Yes, of course. Sorry.
> 
> USB ports are devices that usually the USB controller drivers register
> (or actually the USB core code). They are represented in both ACPI and
> DT as child nodes of the controller device node. The USB connector OF
> node is defined in file
> Documentation/devicetree/bindings/connector/usb-connector.txt
> 
> In short, the controller drivers will request handle to a child node
> that represents a port, and only after that register the actual port
> device.
> 
> The drivers I'm looking at currently are the USB Type-C port
> controller drivers and the port manager (in Greg's usb-next or
> linux-next):
> 
>         drivers/usb/typec/tcpm/tcpci.c
>         drivers/usb/typec/tcpm/fusb302.c
>         drivers/usb/typec/tcpm/tcpm.c
> 
> The goal is simply to get rid of the platform data as usual, and
> ideally so that we don't need any extra code in order to support the
> "legacy" platforms.

Are these actually used on any of the "legacy" platforms? I fetched
linux-next today, but I do not actually see anything in
drivers/usb/typec touching platform data...

In the context of the connector, even before we descend to child nodes
details, how do you propose implementing references between fwnodes?
Especially since the other node (in case you complementing existing
topology) may be ACPI or DT instance?

I want to say that "You aren't gonna need it" for what you are asking
here and we can actually split it apart if and when we actually want to
separate creation of pset-backed device nodes and instantiating
corresponding device structures.

Thanks.

-- 
Dmitry

Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards

[Heikki Krogerus]

Hi Dmitry,

Sorry for the late reply. I decided to give this a try myself, and
I've now prepared something (not for the gpio stuff!). I'll do a quick
internal review round, and send it to you guys as RFC after that..

On Fri, Oct 05, 2018 at 02:47:34PM -0700, Dmitry Torokhov wrote:
> Hi Heikki,
> 
> On Tue, Sep 25, 2018 at 03:19:27PM +0300, Heikki Krogerus wrote:
> > On Mon, Sep 24, 2018 at 11:45:43AM -0700, Dmitry Torokhov wrote:
> > > I think we are talking about totally different use cases and that is why
> > > we are having hard time coming to a mutually agreeable solution. Could
> > > you please describe in more detail what you would like to achieve,
> > > and preferably show how it is described now with DT and/or ACPI, so that
> > > I have a better frame of reference.
> > 
> > Yes, of course. Sorry.
> > 
> > USB ports are devices that usually the USB controller drivers register
> > (or actually the USB core code). They are represented in both ACPI and
> > DT as child nodes of the controller device node. The USB connector OF
> > node is defined in file
> > Documentation/devicetree/bindings/connector/usb-connector.txt
> > 
> > In short, the controller drivers will request handle to a child node
> > that represents a port, and only after that register the actual port
> > device.
> > 
> > The drivers I'm looking at currently are the USB Type-C port
> > controller drivers and the port manager (in Greg's usb-next or
> > linux-next):
> > 
> >         drivers/usb/typec/tcpm/tcpci.c
> >         drivers/usb/typec/tcpm/fusb302.c
> >         drivers/usb/typec/tcpm/tcpm.c
> > 
> > The goal is simply to get rid of the platform data as usual, and
> > ideally so that we don't need any extra code in order to support the
> > "legacy" platforms.
> 
> Are these actually used on any of the "legacy" platforms? I fetched
> linux-next today, but I do not actually see anything in
> drivers/usb/typec touching platform data...

It's not touching any platform data, and I would like to keep it that
way :-). The device for fusb302 is created for example in
drivers/platform/x86/intel_cht_int33fe.c.

The fusb302.c driver already tries to find that child node named
"connector" (and so do the other port drivers). I'm trying to supply
the driver that node here somehow so I can avoid quirks.

> In the context of the connector, even before we descend to child nodes
> details, how do you propose implementing references between fwnodes?
> Especially since the other node (in case you complementing existing
> topology) may be ACPI or DT instance?

The different fwnode types (ACPI, OF, etc.) must live in parallel,
i.e. you can not have something like ACPI parent node with
property_set children if that's what you mean. The "secondary" member
will link the different types of fwnodes together, but they will live
their separate lives. I'm not planning on changing that.

I'm just looking for a smooth way of describing the devices in
software when the firmware is not supplying the hardware description,
just like you. What I really need is separation of the fwnode
registration from device registration in those cases. For that I now
have a proposal, and I believe my proposal will work for you as a
baseline as well.


Thanks,

-- 
heikki

[RFC/PATCH 3/5] device property: export property_set structure

[Dmitry Torokhov]

Future gpiolib will need to handle property sets properly, and this we
need to export property_set, is_pset_node() and to_pset_node().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/pset_property.c | 31 ++-----------------------------
 include/linux/property.h     | 29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
index 63f2377aefe8..7118528816a4 100644
--- a/drivers/base/pset_property.c
+++ b/drivers/base/pset_property.c
@@ -14,34 +14,6 @@
 #include <linux/property.h>
 #include <linux/slab.h>
 
-struct property_set {
-	struct device *dev;
-	struct fwnode_handle fwnode;
-	const struct property_entry *properties;
-
-	struct property_set *parent;
-	/* Entry in parent->children list */
-	struct list_head child_node;
-	struct list_head children;
-};
-
-static const struct fwnode_operations pset_fwnode_ops;
-
-static inline bool is_pset_node(const struct fwnode_handle *fwnode)
-{
-	return !IS_ERR_OR_NULL(fwnode) && fwnode->ops == &pset_fwnode_ops;
-}
-
-#define to_pset_node(__fwnode)						\
-	({								\
-		typeof(__fwnode) __to_pset_node_fwnode = __fwnode;	\
-									\
-		is_pset_node(__to_pset_node_fwnode) ?			\
-			container_of(__to_pset_node_fwnode,		\
-				     struct property_set, fwnode) :	\
-			NULL;						\
-	})
-
 static const struct property_entry *
 pset_prop_get(const struct property_set *pset, const char *name)
 {
@@ -323,13 +295,14 @@ pset_fwnode_get_next_subnode(const struct fwnode_handle *fwnode,
 	return &next->fwnode;
 }
 
-static const struct fwnode_operations pset_fwnode_ops = {
+const struct fwnode_operations pset_fwnode_ops = {
 	.property_present = pset_fwnode_property_present,
 	.property_read_int_array = pset_fwnode_read_int_array,
 	.property_read_string_array = pset_fwnode_property_read_string_array,
 	.get_parent = pset_fwnode_get_parent,
 	.get_next_child_node = pset_fwnode_get_next_subnode,
 };
+EXPORT_SYMBOL_GPL(pset_fwnode_ops);
 
 static void property_entry_free_data(const struct property_entry *p)
 {
diff --git a/include/linux/property.h b/include/linux/property.h
index bb1cf4f30770..957f75d10cf9 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -13,6 +13,7 @@
 #ifndef _LINUX_PROPERTY_H_
 #define _LINUX_PROPERTY_H_
 
+#include <linux/err.h>
 #include <linux/fwnode.h>
 #include <linux/types.h>
 
@@ -281,6 +282,34 @@ device_add_child_properties(struct device *dev,
 			    const struct property_entry *properties);
 void device_remove_properties(struct device *dev);
 
+struct property_set {
+	struct device *dev;
+	struct fwnode_handle fwnode;
+	const struct property_entry *properties;
+
+	struct property_set *parent;
+	/* Entry in parent->children list */
+	struct list_head child_node;
+	struct list_head children;
+};
+
+extern const struct fwnode_operations pset_fwnode_ops;
+
+static inline bool is_pset_node(const struct fwnode_handle *fwnode)
+{
+	return !IS_ERR_OR_NULL(fwnode) && fwnode->ops == &pset_fwnode_ops;
+}
+
+#define to_pset_node(__fwnode)						\
+	({								\
+		typeof(__fwnode) __to_pset_node_fwnode = __fwnode;	\
+									\
+		is_pset_node(__to_pset_node_fwnode) ?			\
+			container_of(__to_pset_node_fwnode,		\
+				     struct property_set, fwnode) :	\
+			NULL;						\
+	})
+
 bool device_dma_supported(struct device *dev);
 
 enum dev_dma_attr device_get_dma_attr(struct device *dev);
-- 
2.19.0.397.gdd90340f6a-goog

[RFC/PATCH 4/5] gpiolib: add support for fetching descriptors from static properties

[Dmitry Torokhov]

Now that static device properties understand notion of child nodes, let's
teach gpiolib to tie such children and machine GPIO descriptor tables.
We will continue using a single table for entire device, but instead of
using connection ID as a lookup key in the GPIO descriptor table directly,
we will perform additional translation: fwnode_get_named_gpiod() when
dealing with property_set-backed fwnodes will try parsing string property
with name matching connection ID and use result of the lookup as the key in
the table:

static const struct property_entry dev_child1_props[] __initconst = {
	...
	PROPERTY_ENTRY_STRING("gpios",		"child-1-gpios"),
	{ }
};

static struct gpiod_lookup_table dev_gpiod_table = {
	.dev_id = "some-device",
	.table = {
		...
		GPIO_LOOKUP_IDX("B", 1, "child-1-gpios", 1, GPIO_ACTIVE_LOW),
		...
	},
};

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/gpio/gpiolib.c | 109 +++++++++++++++++++++++++++++------------
 1 file changed, 79 insertions(+), 30 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e11a3bb03820..f0e51d34a1c1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3959,39 +3959,44 @@ struct gpio_desc *gpiod_get_from_of_node(struct device_node *node,
 }
 EXPORT_SYMBOL(gpiod_get_from_of_node);
 
-/**
- * fwnode_get_named_gpiod - obtain a GPIO from firmware node
- * @fwnode:	handle of the firmware node
- * @propname:	name of the firmware property representing the GPIO
- * @index:	index of the GPIO to obtain for the consumer
- * @dflags:	GPIO initialization flags
- * @label:	label to attach to the requested GPIO
- *
- * This function can be used for drivers that get their configuration
- * from opaque firmware.
- *
- * The function properly finds the corresponding GPIO using whatever is the
- * underlying firmware interface and then makes sure that the GPIO
- * descriptor is requested before it is returned to the caller.
- *
- * Returns:
- * On successful request the GPIO pin is configured in accordance with
- * provided @dflags.
- *
- * In case of error an ERR_PTR() is returned.
- */
-struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
-					 const char *propname, int index,
-					 enum gpiod_flags dflags,
-					 const char *label)
+static struct gpio_desc *pset_node_get_gpiod(struct fwnode_handle *fwnode,
+					     const char *propname, int index,
+					     enum gpio_lookup_flags *flags)
 {
-	struct gpio_desc *desc = ERR_PTR(-ENODEV);
-	unsigned long lflags = 0;
-	int ret;
+	struct property_set *pset;
+	const char *con_id;
 
-	if (!fwnode)
+	pset = to_pset_node(fwnode);
+	if (!pset)
 		return ERR_PTR(-EINVAL);
 
+	if (fwnode_property_read_string(fwnode, propname, &con_id)) {
+		/*
+		 * We could not find string mapping property name to
+		 * entry in gpio lookup table. Let's see if we are
+		 * dealing with firmware node corresponding to the
+		 * device (and not a child node): for such nodes we can
+		 * try doing lookup directly with property name.
+		 */
+		if (pset->parent)
+			return ERR_PTR(-ENOENT);
+
+		con_id = propname;
+	}
+
+	return gpiod_find(pset->dev, con_id, index, flags);
+}
+
+static struct gpio_desc *__fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
+						  const char *propname,
+						  int index,
+						  enum gpiod_flags dflags,
+						  const char *label)
+{
+	struct gpio_desc *desc = ERR_PTR(-ENODEV);
+	enum gpio_lookup_flags lflags = 0;
+	int ret;
+
 	if (is_of_node(fwnode)) {
 		desc = gpiod_get_from_of_node(to_of_node(fwnode),
 					      propname, index,
@@ -4009,9 +4014,12 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 
 		if (info.polarity == GPIO_ACTIVE_LOW)
 			lflags |= GPIO_ACTIVE_LOW;
+	} else if (is_pset_node(fwnode)) {
+		desc = pset_node_get_gpiod(fwnode, propname, index, &lflags);
+		if (IS_ERR(desc))
+			return desc;
 	}
 
-	/* Currently only ACPI takes this path */
 	ret = gpiod_request(desc, label);
 	if (ret)
 		return ERR_PTR(ret);
@@ -4024,6 +4032,47 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 
 	return desc;
 }
+
+/**
+ * fwnode_get_named_gpiod - obtain a GPIO from firmware node
+ * @fwnode:	handle of the firmware node
+ * @propname:	name of the firmware property representing the GPIO
+ * @index:	index of the GPIO to obtain for the consumer
+ * @dflags:	GPIO initialization flags
+ * @label:	label to attach to the requested GPIO
+ *
+ * This function can be used for drivers that get their configuration
+ * from opaque firmware.
+ *
+ * The function properly finds the corresponding GPIO using whatever is the
+ * underlying firmware interface and then makes sure that the GPIO
+ * descriptor is requested before it is returned to the caller.
+ *
+ * Returns:
+ * On successful request the GPIO pin is configured in accordance with
+ * provided @dflags.
+ *
+ * In case of error an ERR_PTR() is returned.
+ */
+struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
+					 const char *propname, int index,
+					 enum gpiod_flags dflags,
+					 const char *label)
+{
+	struct gpio_desc *desc;
+
+	if (!fwnode)
+		return ERR_PTR(-EINVAL);
+
+	desc = __fwnode_get_named_gpiod(fwnode, propname, index, dflags, label);
+	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT &&
+	    !IS_ERR_OR_NULL(fwnode->secondary)) {
+		desc = __fwnode_get_named_gpiod(fwnode->secondary,
+						propname, index, dflags, label);
+	}
+
+	return desc;
+}
 EXPORT_SYMBOL_GPL(fwnode_get_named_gpiod);
 
 /**
-- 
2.19.0.397.gdd90340f6a-goog

Re: [RFC/PATCH 4/5] gpiolib: add support for fetching descriptors from static properties

[Mika Westerberg]

Hi,

On Mon, Sep 17, 2018 at 11:16:02AM -0700, Dmitry Torokhov wrote:
> Now that static device properties understand notion of child nodes, let's
> teach gpiolib to tie such children and machine GPIO descriptor tables.
> We will continue using a single table for entire device, but instead of
> using connection ID as a lookup key in the GPIO descriptor table directly,
> we will perform additional translation: fwnode_get_named_gpiod() when
> dealing with property_set-backed fwnodes will try parsing string property
> with name matching connection ID and use result of the lookup as the key in
> the table:
> 
> static const struct property_entry dev_child1_props[] __initconst = {
> 	...
> 	PROPERTY_ENTRY_STRING("gpios",		"child-1-gpios"),
> 	{ }
> };
> 
> static struct gpiod_lookup_table dev_gpiod_table = {
> 	.dev_id = "some-device",
> 	.table = {
> 		...
> 		GPIO_LOOKUP_IDX("B", 1, "child-1-gpios", 1, GPIO_ACTIVE_LOW),
> 		...
> 	},
> };

I wonder if instead of passing and parsing strings (and hoping there are
no typos) we could get the compiler to help us bit more?

Something like below:

static const struct property_entry dev_child1_props[] __initconst = {
 	...
 	PROPERTY_ENTRY_STRING("gpios","child-1-gpios"),
 	{ }
};
 
static struct gpiod_lookup_table dev_gpiod_table = {
 	.dev_id = "some-device",
 	.table = {
 		...
 		GPIO_LOOKUP_IDX("B", 1, dev_child1_props, SIZEOF(dev_child1_props),
				1, GPIO_ACTIVE_LOW),
 		...
 	},
};

Re: [RFC/PATCH 4/5] gpiolib: add support for fetching descriptors from static properties

[Dmitry Torokhov]

Hi Mika,

On Tue, Sep 18, 2018 at 12:02:19PM +0300, Mika Westerberg wrote:
> Hi,
> 
> On Mon, Sep 17, 2018 at 11:16:02AM -0700, Dmitry Torokhov wrote:
> > Now that static device properties understand notion of child nodes, let's
> > teach gpiolib to tie such children and machine GPIO descriptor tables.
> > We will continue using a single table for entire device, but instead of
> > using connection ID as a lookup key in the GPIO descriptor table directly,
> > we will perform additional translation: fwnode_get_named_gpiod() when
> > dealing with property_set-backed fwnodes will try parsing string property
> > with name matching connection ID and use result of the lookup as the key in
> > the table:
> > 
> > static const struct property_entry dev_child1_props[] __initconst = {
> > 	...
> > 	PROPERTY_ENTRY_STRING("gpios",		"child-1-gpios"),
> > 	{ }
> > };
> > 
> > static struct gpiod_lookup_table dev_gpiod_table = {
> > 	.dev_id = "some-device",
> > 	.table = {
> > 		...
> > 		GPIO_LOOKUP_IDX("B", 1, "child-1-gpios", 1, GPIO_ACTIVE_LOW),
> > 		...
> > 	},
> > };
> 
> I wonder if instead of passing and parsing strings (and hoping there are
> no typos) we could get the compiler to help us bit more?
> 
> Something like below:
> 
> static const struct property_entry dev_child1_props[] __initconst = {
>  	...
>  	PROPERTY_ENTRY_STRING("gpios","child-1-gpios"),
>  	{ }
> };
>  
> static struct gpiod_lookup_table dev_gpiod_table = {
>  	.dev_id = "some-device",
>  	.table = {
>  		...
>  		GPIO_LOOKUP_IDX("B", 1, dev_child1_props, SIZEOF(dev_child1_props),
> 				1, GPIO_ACTIVE_LOW),
>  		...
>  	},
> };

I am not sure how that would work, as there are multiple properties in
that child array, so we can't simply take the first entry or assume that
all entries describe GPIOs. Here is the fuller example:

static const struct property_entry simone_key_enter_props[] __initconst = {
	PROPERTY_ENTRY_U32("linux,code",	KEY_ENTER),
	PROPERTY_ENTRY_STRING("label",		"enter"),
	PROPERTY_ENTRY_STRING("gpios",		"enter-gpios"),
	{ }
};

static const struct property_entry simone_key_up_props[] __initconst = {
	PROPERTY_ENTRY_U32("linux,code",	KEY_UP),
	PROPERTY_ENTRY_STRING("label",		"up"),
	PROPERTY_ENTRY_STRING("gpios",		"up-gpios"),
	{ }
};

static const struct property_entry simone_key_up_props[] __initconst = {
	PROPERTY_ENTRY_U32("linux,code",	KEY_LEFT),
	PROPERTY_ENTRY_STRING("label",		"left"),
	PROPERTY_ENTRY_STRING("gpios",		"left-gpios"),
	{ }
};

static const struct property_entry simone_key_props[] __initconst = {
	/* There are no properties at device level on this device */
	{ }
};

static struct gpiod_lookup_table simone_keys_gpiod_table = {
	.dev_id = "gpio-keys",
	.table = {
		/* Use local offsets on gpiochip/port "B" */
		GPIO_LOOKUP_IDX("B", 0, "enter-gpios", 0, GPIO_ACTIVE_LOW),
		GPIO_LOOKUP_IDX("B", 1, "up-gpios", 1, GPIO_ACTIVE_LOW),
		GPIO_LOOKUP_IDX("B", 2, "left-gpios", 2, GPIO_ACTIVE_LOW),
	},
};

static struct platform_device simone_keys_device = {
	.name = "gpio-keys",
	.id = -1,
};

static void __init simone_init_machine(void)
{
	...
	gpiod_add_lookup_table(&simone_keys_gpiod_table);
	device_add_properties(&simone_keys_device.dev,
			      simone_keys_device_props);
	device_add_child_properties(&simone_keys_device.dev,
				    dev_fwnode(&simone_keys_device.dev),
				    simone_key_enter_props);
	device_add_child_properties(&simone_keys_device.dev,
				    dev_fwnode(&simone_keys_device.dev),
				    simone_key_up_props);
	device_add_child_properties(&simone_keys_device.dev,
				    dev_fwnode(&simone_keys_device.dev),
				    simone_key_left_props);
	platform_device_register(&simone_keys_device);
	...
}

Thanks.

-- 
Dmitry

Re: [RFC/PATCH 4/5] gpiolib: add support for fetching descriptors from static properties

[Mika Westerberg]

On Tue, Sep 18, 2018 at 10:04:18AM -0700, Dmitry Torokhov wrote:
> I am not sure how that would work, as there are multiple properties in
> that child array, so we can't simply take the first entry or assume that
> all entries describe GPIOs. Here is the fuller example:
> 
> static const struct property_entry simone_key_enter_props[] __initconst = {
> 	PROPERTY_ENTRY_U32("linux,code",	KEY_ENTER),
> 	PROPERTY_ENTRY_STRING("label",		"enter"),
> 	PROPERTY_ENTRY_STRING("gpios",		"enter-gpios"),
> 	{ }
> };
> 
> static const struct property_entry simone_key_up_props[] __initconst = {
> 	PROPERTY_ENTRY_U32("linux,code",	KEY_UP),
> 	PROPERTY_ENTRY_STRING("label",		"up"),
> 	PROPERTY_ENTRY_STRING("gpios",		"up-gpios"),
> 	{ }
> };
> 
> static const struct property_entry simone_key_up_props[] __initconst = {
> 	PROPERTY_ENTRY_U32("linux,code",	KEY_LEFT),
> 	PROPERTY_ENTRY_STRING("label",		"left"),
> 	PROPERTY_ENTRY_STRING("gpios",		"left-gpios"),
> 	{ }
> };
> 
> static const struct property_entry simone_key_props[] __initconst = {
> 	/* There are no properties at device level on this device */
> 	{ }
> };
> 
> static struct gpiod_lookup_table simone_keys_gpiod_table = {
> 	.dev_id = "gpio-keys",
> 	.table = {
> 		/* Use local offsets on gpiochip/port "B" */
> 		GPIO_LOOKUP_IDX("B", 0, "enter-gpios", 0, GPIO_ACTIVE_LOW),
> 		GPIO_LOOKUP_IDX("B", 1, "up-gpios", 1, GPIO_ACTIVE_LOW),
> 		GPIO_LOOKUP_IDX("B", 2, "left-gpios", 2, GPIO_ACTIVE_LOW),
> 	},
> };
> 
> static struct platform_device simone_keys_device = {
> 	.name = "gpio-keys",
> 	.id = -1,
> };
> 
> static void __init simone_init_machine(void)
> {
> 	...
> 	gpiod_add_lookup_table(&simone_keys_gpiod_table);
> 	device_add_properties(&simone_keys_device.dev,
> 			      simone_keys_device_props);
> 	device_add_child_properties(&simone_keys_device.dev,
> 				    dev_fwnode(&simone_keys_device.dev),
> 				    simone_key_enter_props);
> 	device_add_child_properties(&simone_keys_device.dev,
> 				    dev_fwnode(&simone_keys_device.dev),
> 				    simone_key_up_props);
> 	device_add_child_properties(&simone_keys_device.dev,
> 				    dev_fwnode(&simone_keys_device.dev),
> 				    simone_key_left_props);
> 	platform_device_register(&simone_keys_device);
> 	...
> }

Thanks for clarifying. I missed this last part where you feed the
properties to the device.

So looks fine by me :)

[RFC/PATCH 5/5] RFC: ARM: simone: Hacked in keys

[Dmitry Torokhov]

From: Linus Walleij <linus.walleij@linaro.org>

This serves as an illustration of how to use the gpio-keys
in board files using static device properties and machine GPIO descriptor
tables.

It is a hack for the joystick connector on the entirely
boardfile-based SIM.ONE. It will probably not be applied.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 arch/arm/mach-ep93xx/simone.c | 60 +++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/arch/arm/mach-ep93xx/simone.c b/arch/arm/mach-ep93xx/simone.c
index 41aa57581356..b998d6772158 100644
--- a/arch/arm/mach-ep93xx/simone.c
+++ b/arch/arm/mach-ep93xx/simone.c
@@ -24,7 +24,11 @@
 #include <linux/spi/mmc_spi.h>
 #include <linux/platform_data/video-ep93xx.h>
 #include <linux/platform_data/spi-ep93xx.h>
+#include <linux/gpio/machine.h>
 #include <linux/gpio.h>
+#include <linux/input.h>
+#include <linux/gpio_keys.h>
+#include <linux/property.h>
 
 #include <mach/hardware.h>
 #include <mach/gpio-ep93xx.h>
@@ -34,6 +38,47 @@
 
 #include "soc.h"
 
+static const struct property_entry simone_key_enter_props[] __initconst = {
+	PROPERTY_ENTRY_U32("linux,code",	KEY_ENTER),
+	PROPERTY_ENTRY_STRING("label",		"enter"),
+	PROPERTY_ENTRY_STRING("gpios",		"enter-gpios"),
+	{ }
+};
+
+static const struct property_entry simone_key_up_props[] __initconst = {
+	PROPERTY_ENTRY_U32("linux,code",	KEY_UP),
+	PROPERTY_ENTRY_STRING("label",		"up"),
+	PROPERTY_ENTRY_STRING("gpios",		"up-gpios"),
+	{ }
+};
+
+static const struct property_entry simone_key_up_props[] __initconst = {
+	PROPERTY_ENTRY_U32("linux,code",	KEY_LEFT),
+	PROPERTY_ENTRY_STRING("label",		"left"),
+	PROPERTY_ENTRY_STRING("gpios",		"left-gpios"),
+	{ }
+};
+
+static const struct property_entry simone_key_props[] __initconst = {
+	/* There are no properties at device level on this device */
+	{ }
+};
+
+static struct gpiod_lookup_table simone_keys_gpiod_table = {
+	.dev_id = "gpio-keys",
+	.table = {
+		/* Use local offsets on gpiochip/port "B" */
+		GPIO_LOOKUP_IDX("B", 0, "enter-gpios", 0, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("B", 1, "up-gpios", 1, GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP_IDX("B", 2, "left-gpios", 2, GPIO_ACTIVE_LOW),
+	},
+};
+
+static struct platform_device simone_keys_device = {
+	.name = "gpio-keys",
+	.id = -1,
+};
+
 static struct ep93xx_eth_data __initdata simone_eth_data = {
 	.phy_id		= 1,
 };
@@ -107,6 +152,21 @@ static void __init simone_init_machine(void)
 			    ARRAY_SIZE(simone_i2c_board_info));
 	ep93xx_register_spi(&simone_spi_info, simone_spi_devices,
 			    ARRAY_SIZE(simone_spi_devices));
+
+	gpiod_add_lookup_table(&simone_keys_gpiod_table);
+	device_add_properties(&simone_keys_device.dev,
+			      simone_keys_device_props);
+	device_add_child_properties(&simone_keys_device.dev,
+				    dev_fwnode(&simone_keys_device.dev),
+				    simone_key_enter_props);
+	device_add_child_properties(&simone_keys_device.dev,
+				    dev_fwnode(&simone_keys_device.dev),
+				    simone_key_up_props);
+	device_add_child_properties(&simone_keys_device.dev,
+				    dev_fwnode(&simone_keys_device.dev),
+				    simone_key_left_props);
+	platform_device_register(&simone_keys_device);
+
 	simone_register_audio();
 }
 
-- 
2.19.0.397.gdd90340f6a-goog

Re: [RFC/PATCH 0/5] Support children for legacy device properties

[Andy Shevchenko]

On Mon, Sep 17, 2018 at 9:18 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> The generic device properties APIs are very helpful as they allow abstracting
> away details of the platform (whether it is ACPI, device tree, or legacy board
> file), so that individual driver does not need separate code paths to support
> all variants. However there are drivers that currently can not use generic
> device properties API as they need notion of children properties, for example
> gpio_keys driver, that expects every button to be described as a sub-node of
> main device.
>
> This patch series introduces notion of sub-nodes for static properties and ties
> it up with GPIO lookup tables so that they are usable with sub-nodes as well.

I like the idea.
Though, I Cc'ed this to Mika and Sakari who might be interested to see
and, perhaps, comment on it.

>
> Thanks.
>
> --
> Dmitry
>


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC/PATCH 0/5] Support children for legacy device properties

[Rafael J. Wysocki]

On 9/17/2018 8:15 PM, Dmitry Torokhov wrote:
> The generic device properties APIs are very helpful as they allow abstracting
> away details of the platform (whether it is ACPI, device tree, or legacy board
> file), so that individual driver does not need separate code paths to support
> all variants. However there are drivers that currently can not use generic
> device properties API as they need notion of children properties, for example
> gpio_keys driver, that expects every button to be described as a sub-node of
> main device.
>
> This patch series introduces notion of sub-nodes for static properties and ties
> it up with GPIO lookup tables so that they are usable with sub-nodes as well.

I don't have any objections against this series.

Thanks,
Rafael

Re: [RFC/PATCH 0/5] Support children for legacy device properties

[Linus Walleij]

On Mon, Sep 17, 2018 at 11:16 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> The generic device properties APIs are very helpful as they allow abstracting
> away details of the platform (whether it is ACPI, device tree, or legacy board
> file), so that individual driver does not need separate code paths to support
> all variants. However there are drivers that currently can not use generic
> device properties API as they need notion of children properties, for example
> gpio_keys driver, that expects every button to be described as a sub-node of
> main device.
>
> This patch series introduces notion of sub-nodes for static properties and ties
> it up with GPIO lookup tables so that they are usable with sub-nodes as well.

This is the patch series I would have written, had I been smart enough.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
for the series.

I can't test the SIM.ONE board with this until next week but the approach
is definately what we want, not just for legacy boards, but also for any
other non-discoverable hardware we currently poke into
drivers/platform or arch/x86/platform etc.

Yours,
Linus Walleij

Yours,
Linus Walleij