linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/5] device property: Introducing software nodes
@ 2018-10-12 11:39 Heikki Krogerus
  2018-10-12 11:39 ` [RFC PATCH 1/5] drivers core: Prepare support for multiple platform notifications Heikki Krogerus
                   ` (7 more replies)
  0 siblings, 8 replies; 18+ messages in thread
From: Heikki Krogerus @ 2018-10-12 11:39 UTC (permalink / raw)
  To: Dmitry Torokhov, Linus Walleij, Rafael J. Wysocki,
	Andy Shevchenko, Mika Westerberg
  Cc: linux-kernel, linux-acpi

Hi guys,

To continue the discussion started by Dmitry [1], this is my proposal
that I mentioned in my last mail. In short, the idea is that instead
of trying to extend the support for the currently used struct
property_set, I'm proposing that we introduce a completely new,
independent type of fwnode, and replace the struct property_set with
it. I'm calling the type "software node" here.

The reason for a complete separation of the software nodes from the
generic property handling code is the need to be able to create the
nodes independently from the devices that they are bind to.

The way this works is that every node that is created will have a
kobject registered. That will take care the ref counting for us, and
also allow us to for example display the properties in sysfs.

There are a few more details in patch 3/5 about the software nodes in
the commit message.

[1] https://lkml.org/lkml/2018/9/17/1067

--
heikki


Heikki Krogerus (5):
  drivers core: Prepare support for multiple platform notifications
  ACPI / glue: Add acpi_platform_notify() function
  drivers: base: Introducing software nodes to the firmware node
    framework
  device property: Move device_add_properties() to swnode.c
  device property: Remove struct property_set

 .../ABI/testing/sysfs-devices-software_node   |  27 +
 drivers/acpi/bus.c                            |   1 -
 drivers/acpi/glue.c                           |  21 +-
 drivers/acpi/internal.h                       |   1 -
 drivers/base/Makefile                         |   2 +-
 drivers/base/core.c                           |  32 +-
 drivers/base/property.c                       | 529 +-----------
 drivers/base/swnode.c                         | 812 ++++++++++++++++++
 include/linux/acpi.h                          |  10 +
 include/linux/property.h                      |  12 +
 10 files changed, 929 insertions(+), 518 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-software_node
 create mode 100644 drivers/base/swnode.c

-- 
2.19.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [RFC PATCH 1/5] drivers core: Prepare support for multiple platform notifications
  2018-10-12 11:39 [RFC PATCH 0/5] device property: Introducing software nodes Heikki Krogerus
@ 2018-10-12 11:39 ` Heikki Krogerus
  2018-10-12 11:39 ` [RFC PATCH 2/5] ACPI / glue: Add acpi_platform_notify() function Heikki Krogerus
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Heikki Krogerus @ 2018-10-12 11:39 UTC (permalink / raw)
  To: Dmitry Torokhov, Linus Walleij, Rafael J. Wysocki,
	Andy Shevchenko, Mika Westerberg
  Cc: linux-kernel, linux-acpi

Since it should be possible to support several hardware
description models at the same time (at least in theory),
for example ACPI and devicetree on a running system, the
platform notifications need to be handled differently.

For now a single "platform_notify" callback function was
used to notify the underlying base system which is in charge
of the hardware description when a new device entry was
added to the system, but that callback is available to only
a single base system at the time. This will add a function
device_platform_notify() and replace all direct
platform_notify() calls with it.

device_platform_notify() will first simply call the
platform_notify() callback, so this commit has no functional
affect, however, the idea is that individual base systems
will put their direct notification calls there instead of
using the platform_notify function pointer.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/core.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 04bbcd779e11..e42d6e7dd368 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -728,6 +728,16 @@ static inline int device_is_not_partition(struct device *dev)
 }
 #endif
 
+static int
+device_platform_notify(struct device *dev, enum kobject_action action)
+{
+	if (platform_notify && action == KOBJ_ADD)
+		platform_notify(dev);
+	else if (platform_notify_remove && action == KOBJ_REMOVE)
+		platform_notify_remove(dev);
+	return 0;
+}
+
 /**
  * dev_driver_string - Return a device's driver name, if at all possible
  * @dev: struct device to get the name of
@@ -1883,8 +1893,9 @@ int device_add(struct device *dev)
 	}
 
 	/* notify platform of device entry */
-	if (platform_notify)
-		platform_notify(dev);
+	error = device_platform_notify(dev, KOBJ_ADD);
+	if (error)
+		goto platform_error;
 
 	error = device_create_file(dev, &dev_attr_uevent);
 	if (error)
@@ -1960,6 +1971,8 @@ int device_add(struct device *dev)
  SymlinkError:
 	device_remove_file(dev, &dev_attr_uevent);
  attrError:
+	device_platform_notify(dev, KOBJ_REMOVE);
+platform_error:
 	kobject_uevent(&dev->kobj, KOBJ_REMOVE);
 	glue_dir = get_glue_dir(dev);
 	kobject_del(&dev->kobj);
@@ -2083,8 +2096,8 @@ void device_del(struct device *dev)
 	/* Notify the platform of the removal, in case they
 	 * need to do anything...
 	 */
-	if (platform_notify_remove)
-		platform_notify_remove(dev);
+	device_platform_notify(dev, KOBJ_REMOVE);
+
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_REMOVED_DEVICE, dev);
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC PATCH 2/5] ACPI / glue: Add acpi_platform_notify() function
  2018-10-12 11:39 [RFC PATCH 0/5] device property: Introducing software nodes Heikki Krogerus
  2018-10-12 11:39 ` [RFC PATCH 1/5] drivers core: Prepare support for multiple platform notifications Heikki Krogerus
@ 2018-10-12 11:39 ` Heikki Krogerus
  2018-10-12 11:39 ` [RFC PATCH 3/5] drivers: base: Introducing software nodes to the firmware node framework Heikki Krogerus
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Heikki Krogerus @ 2018-10-12 11:39 UTC (permalink / raw)
  To: Dmitry Torokhov, Linus Walleij, Rafael J. Wysocki,
	Andy Shevchenko, Mika Westerberg
  Cc: linux-kernel, linux-acpi

Instead of relying on the "platform_notify" callback hook,
introducing separate notification function
acpi_platform_notify() and calling that directly from
drivers core when device entries are added and removed.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/bus.c      |  1 -
 drivers/acpi/glue.c     | 21 +++++++++++++--------
 drivers/acpi/internal.h |  1 -
 drivers/base/core.c     |  7 +++++++
 include/linux/acpi.h    | 10 ++++++++++
 5 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index d2e29a19890d..500729913b35 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1249,7 +1249,6 @@ static int __init acpi_init(void)
 		acpi_kobj = NULL;
 	}
 
-	init_acpi_device_notify();
 	result = acpi_bus_init();
 	if (result) {
 		disable_acpi();
diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 12ba2bee8789..edd10b3c7ec8 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -296,7 +296,7 @@ int acpi_unbind_one(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(acpi_unbind_one);
 
-static int acpi_platform_notify(struct device *dev)
+static int acpi_device_notify(struct device *dev)
 {
 	struct acpi_bus_type *type = acpi_get_bus_type(dev);
 	struct acpi_device *adev;
@@ -343,7 +343,7 @@ static int acpi_platform_notify(struct device *dev)
 	return ret;
 }
 
-static int acpi_platform_notify_remove(struct device *dev)
+static int acpi_device_notify_remove(struct device *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(dev);
 	struct acpi_bus_type *type;
@@ -361,12 +361,17 @@ static int acpi_platform_notify_remove(struct device *dev)
 	return 0;
 }
 
-void __init init_acpi_device_notify(void)
+int acpi_platform_notify(struct device *dev, enum kobject_action action)
 {
-	if (platform_notify || platform_notify_remove) {
-		printk(KERN_ERR PREFIX "Can't use platform_notify\n");
-		return;
+	switch (action) {
+	case KOBJ_ADD:
+		acpi_device_notify(dev);
+		break;
+	case KOBJ_REMOVE:
+		acpi_device_notify_remove(dev);
+		break;
+	default:
+		break;
 	}
-	platform_notify = acpi_platform_notify;
-	platform_notify_remove = acpi_platform_notify_remove;
+	return 0;
 }
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 530a3f675490..83a7dfb7d1cf 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -23,7 +23,6 @@
 int early_acpi_osi_init(void);
 int acpi_osi_init(void);
 acpi_status acpi_os_initialize1(void);
-void init_acpi_device_notify(void);
 int acpi_scan_init(void);
 void acpi_pci_root_init(void);
 void acpi_pci_link_init(void);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index e42d6e7dd368..d0b5b9d6f19e 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -8,6 +8,7 @@
  * Copyright (c) 2006 Novell, Inc.
  */
 
+#include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/fwnode.h>
@@ -731,6 +732,12 @@ static inline int device_is_not_partition(struct device *dev)
 static int
 device_platform_notify(struct device *dev, enum kobject_action action)
 {
+	int ret;
+
+	ret = acpi_platform_notify(dev, action);
+	if (ret)
+		return ret;
+
 	if (platform_notify && action == KOBJ_ADD)
 		platform_notify(dev);
 	else if (platform_notify_remove && action == KOBJ_REMOVE)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index ed80f147bd50..4ba2e2d24676 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1313,4 +1313,14 @@ static inline int find_acpi_cpu_cache_topology(unsigned int cpu, int level)
 }
 #endif
 
+#ifdef CONFIG_ACPI
+extern int acpi_platform_notify(struct device *dev, enum kobject_action action);
+#else
+static inline int
+acpi_platform_notify(struct device *dev, enum kobject_action action)
+{
+	return 0;
+}
+#endif
+
 #endif	/*_LINUX_ACPI_H*/
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC PATCH 3/5] drivers: base: Introducing software nodes to the firmware node framework
  2018-10-12 11:39 [RFC PATCH 0/5] device property: Introducing software nodes Heikki Krogerus
  2018-10-12 11:39 ` [RFC PATCH 1/5] drivers core: Prepare support for multiple platform notifications Heikki Krogerus
  2018-10-12 11:39 ` [RFC PATCH 2/5] ACPI / glue: Add acpi_platform_notify() function Heikki Krogerus
@ 2018-10-12 11:39 ` Heikki Krogerus
  2018-10-16 12:57   ` Andy Shevchenko
  2018-10-12 11:39 ` [RFC PATCH 4/5] device property: Move device_add_properties() to swnode.c Heikki Krogerus
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Heikki Krogerus @ 2018-10-12 11:39 UTC (permalink / raw)
  To: Dmitry Torokhov, Linus Walleij, Rafael J. Wysocki,
	Andy Shevchenko, Mika Westerberg
  Cc: linux-kernel, linux-acpi

Software node is a new struct fwnode_handle type that can be
used to describe devices in kernel (software). It is meant
to complement fwnodes representing real firmware nodes when
they are incomplete (for example missing device properties)
and to supply the primary fwnode when the firmware lacks
hardware description for a device completely.

The software node type is really meant to replace the
currently used "property_set" struct fwnode_handle type. The
handling of struct property_set is glued to the generic
device property handling code, and it is not possible to
create a struct property_set independently from a device
that it is bind to. struct property_set is only created when
device properties are added to already initialized struct
device, and control of it is only possible from the generic
property handling code.

Software nodes are instead designed to be created
independently from the device entries (struct device). It
makes them much more flexible, as then the device meant to
be bind to the node can be created at a later time, and from
another location. It is also possible to bind multiple
devices to a single software node if needed.

The software node implementation also includes support for
node hierarchy, which was the main motivation for this
commit. The node hierarchy was something that was requested
for the struct property_set, but it did not seem reasonable
to try to extend the property_set support for that purpose.
struct property_set was really meant only for device
property handling like the name suggests.

Support for struct property_set is not yet removed in this
commit, but it will be in the following one.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 .../ABI/testing/sysfs-devices-software_node   |  27 +
 drivers/base/Makefile                         |   2 +-
 drivers/base/core.c                           |   4 +
 drivers/base/swnode.c                         | 628 ++++++++++++++++++
 include/linux/property.h                      |  12 +
 5 files changed, 672 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-software_node
 create mode 100644 drivers/base/swnode.c

diff --git a/Documentation/ABI/testing/sysfs-devices-software_node b/Documentation/ABI/testing/sysfs-devices-software_node
new file mode 100644
index 000000000000..3b0b24ae39e7
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-software_node
@@ -0,0 +1,27 @@
+What:		/sys/devices/.../software_node/
+Date:		January 2019
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		This directory contains the details about the device that are
+		assigned in kernel (i.e. software), as opposed to the
+		firmware_node directory which contains the details that are
+		assigned for the device in firmware. The main attributes in the
+		directory will show the properties the device has, and the
+		relationship it has to some of the other devices.
+
+What:		/sys/devices/.../software_node/properties/
+Date:		January 2019
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		The /sys/devices/.../software_node/properties directory contains
+		device properties that are assigned in kernel for the device.
+		These properties may be additional properties on top of the ones
+		that the firmware has assigned for the device. The device
+		properties that the firmware supplies for the device are not
+		visible in this directory.
+
+		Every property that has been assigned in kernel for the device
+		will have its own attribute file under this directory, and those
+		files will be named with the names of the properties. Reading
+		one of these files will return the value or values the property
+		has.
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 704f44295810..157452080f3d 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -6,7 +6,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.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
+			   devcon.o swnode.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-y			+= power/
 obj-$(CONFIG_ISA_BUS_API)	+= isa.o
diff --git a/drivers/base/core.c b/drivers/base/core.c
index d0b5b9d6f19e..6a8a12d32562 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -738,6 +738,10 @@ device_platform_notify(struct device *dev, enum kobject_action action)
 	if (ret)
 		return ret;
 
+	ret = software_node_notify(dev, action);
+	if (ret)
+		return ret;
+
 	if (platform_notify && action == KOBJ_ADD)
 		platform_notify(dev);
 	else if (platform_notify_remove && action == KOBJ_REMOVE)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
new file mode 100644
index 000000000000..77d5645d2fcf
--- /dev/null
+++ b/drivers/base/swnode.c
@@ -0,0 +1,628 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Software nodes for the firmware node framework.
+ *
+ * Copyright (C) 2018, Intel Corporation
+ * Authors: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+struct software_node {
+	int id;
+	struct kobject kobj;
+	struct fwnode_handle fwnode;
+
+	/* hierarchy */
+	struct ida child_ids;
+	struct list_head entry;
+	struct list_head children;
+	struct software_node *parent;
+
+	/* properties */
+	struct kobj_attribute *property_attrs;
+	struct attribute_group property_group;
+	const struct property_entry *properties;
+};
+
+static DEFINE_IDA(swnode_root_ids);
+static struct kset *swnode_kset;
+
+#define kobj_to_swnode(_kobj_) container_of(_kobj_, struct software_node, kobj)
+
+static const struct fwnode_operations software_node_ops;
+
+bool is_software_node(const struct fwnode_handle *fwnode)
+{
+	return !IS_ERR_OR_NULL(fwnode) && fwnode->ops == &software_node_ops;
+}
+
+#define to_software_node(__fwnode)					\
+	({								\
+		typeof(__fwnode) __to_software_node_fwnode = __fwnode;	\
+									\
+		is_software_node(__to_software_node_fwnode) ?		\
+			container_of(__to_software_node_fwnode,		\
+				     struct software_node, fwnode) :	\
+			NULL;						\
+	})
+
+/* -------------------------------------------------------------------------- */
+/* property_entry processing */
+
+static const struct property_entry *
+property_entry_get(const struct property_entry *prop, const char *name)
+{
+	if (!prop)
+		return NULL;
+
+	for (; 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 const void *property_entry_find(const struct property_entry *props,
+				       const char *propname, size_t length)
+{
+	const struct property_entry *prop;
+	const void *pointer;
+
+	prop = property_entry_get(props, 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 property_entry_read_u8_array(const struct property_entry *props,
+					const char *propname,
+					u8 *values, size_t nval)
+{
+	const void *pointer;
+	size_t length = nval * sizeof(*values);
+
+	pointer = property_entry_find(props, propname, length);
+	if (IS_ERR(pointer))
+		return PTR_ERR(pointer);
+
+	memcpy(values, pointer, length);
+	return 0;
+}
+
+static int property_entry_read_u16_array(const struct property_entry *props,
+					 const char *propname,
+					 u16 *values, size_t nval)
+{
+	const void *pointer;
+	size_t length = nval * sizeof(*values);
+
+	pointer = property_entry_find(props, propname, length);
+	if (IS_ERR(pointer))
+		return PTR_ERR(pointer);
+
+	memcpy(values, pointer, length);
+	return 0;
+}
+
+static int property_entry_read_u32_array(const struct property_entry *props,
+					 const char *propname,
+					 u32 *values, size_t nval)
+{
+	const void *pointer;
+	size_t length = nval * sizeof(*values);
+
+	pointer = property_entry_find(props, propname, length);
+	if (IS_ERR(pointer))
+		return PTR_ERR(pointer);
+
+	memcpy(values, pointer, length);
+	return 0;
+}
+
+static int property_entry_read_u64_array(const struct property_entry *props,
+					 const char *propname,
+					 u64 *values, size_t nval)
+{
+	const void *pointer;
+	size_t length = nval * sizeof(*values);
+
+	pointer = property_entry_find(props, propname, length);
+	if (IS_ERR(pointer))
+		return PTR_ERR(pointer);
+
+	memcpy(values, pointer, length);
+	return 0;
+}
+
+static int
+property_entry_count_elems_of_size(const struct property_entry *props,
+				   const char *propname, size_t length)
+{
+	const struct property_entry *prop;
+
+	prop = property_entry_get(props, propname);
+	if (!prop)
+		return -EINVAL;
+
+	return prop->length / length;
+}
+
+static int property_entry_read_int_array(const struct property_entry *props,
+					 const char *name,
+					 unsigned int elem_size, void *val,
+					 size_t nval)
+{
+	if (!val)
+		return property_entry_count_elems_of_size(props, name,
+							  elem_size);
+	switch (elem_size) {
+	case sizeof(u8):
+		return property_entry_read_u8_array(props, name, val, nval);
+	case sizeof(u16):
+		return property_entry_read_u16_array(props, name, val, nval);
+	case sizeof(u32):
+		return property_entry_read_u32_array(props, name, val, nval);
+	case sizeof(u64):
+		return property_entry_read_u64_array(props, name, val, nval);
+	}
+
+	return -ENXIO;
+}
+
+static int property_entry_read_string_array(const struct property_entry *props,
+					    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 = property_entry_get(props, 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 = property_entry_count_elems_of_size(props, 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 = property_entry_find(props, propname, length);
+	if (IS_ERR(pointer))
+		return PTR_ERR(pointer);
+
+	memcpy(strings, pointer, length);
+
+	return array_len;
+}
+
+/* -------------------------------------------------------------------------- */
+/* fwnode operations */
+
+static struct fwnode_handle *software_node_get(struct fwnode_handle *fwnode)
+{
+	struct software_node *swnode = to_software_node(fwnode);
+
+	kobject_get(&swnode->kobj);
+
+	return &swnode->fwnode;
+}
+
+static void software_node_put(struct fwnode_handle *fwnode)
+{
+	struct software_node *swnode = to_software_node(fwnode);
+
+	kobject_put(&swnode->kobj);
+}
+
+static bool software_node_property_present(const struct fwnode_handle *fwnode,
+					   const char *propname)
+{
+	return !!property_entry_get(to_software_node(fwnode)->properties,
+				    propname);
+}
+
+static int software_node_read_int_array(const struct fwnode_handle *fwnode,
+					const char *propname,
+					unsigned int elem_size, void *val,
+					size_t nval)
+{
+	struct software_node *swnode = to_software_node(fwnode);
+
+	return property_entry_read_int_array(swnode->properties, propname,
+					     elem_size, val, nval);
+}
+
+static int software_node_read_string_array(const struct fwnode_handle *fwnode,
+					   const char *propname,
+					   const char **val, size_t nval)
+{
+	struct software_node *swnode = to_software_node(fwnode);
+
+	return property_entry_read_string_array(swnode->properties, propname,
+						val, nval);
+}
+
+struct fwnode_handle *
+software_node_get_parent(const struct fwnode_handle *fwnode)
+{
+	struct software_node *swnode = to_software_node(fwnode);
+
+	return swnode->parent ? &swnode->parent->fwnode : NULL;
+}
+
+struct fwnode_handle *
+software_node_get_next_child(const struct fwnode_handle *fwnode,
+			     struct fwnode_handle *child)
+{
+	struct software_node *p = to_software_node(fwnode);
+	struct software_node *c = to_software_node(child);
+
+	if (list_empty(&p->children) ||
+	    (c && list_is_last(&c->entry, &p->children)))
+		return NULL;
+
+	if (c)
+		c = list_next_entry(c, entry);
+	else
+		c = list_first_entry(&p->children, struct software_node, entry);
+	return &c->fwnode;
+}
+
+struct fwnode_handle *
+software_node_get_named_child(const struct fwnode_handle *fwnode,
+			      const char *name)
+{
+	struct software_node *swnode = to_software_node(fwnode);
+	const struct property_entry *prop;
+	struct software_node *child;
+
+	list_for_each_entry(child, &swnode->children, entry) {
+		prop = property_entry_get(child->properties, "name");
+		if (!prop)
+			continue;
+
+		if (!strcmp(name, prop->value.str))
+			return software_node_get(&child->fwnode);
+	}
+
+	return NULL;
+}
+
+static const struct fwnode_operations software_node_ops = {
+	.get = software_node_get,
+	.put = software_node_put,
+	.property_present = software_node_property_present,
+	.property_read_int_array = software_node_read_int_array,
+	.property_read_string_array = software_node_read_string_array,
+	.get_parent = software_node_get_parent,
+	.get_next_child_node = software_node_get_next_child,
+	.get_named_child_node = software_node_get_named_child,
+};
+
+/* -------------------------------------------------------------------------- */
+
+static ssize_t property_array_show(const struct property_entry *prop, char *buf)
+{
+	int ret = 0;
+	int i;
+
+	switch (prop->type) {
+	case DEV_PROP_U8:
+		for (i = 0; i < prop->length / sizeof(u8); i++)
+			ret += sprintf(buf + ret, "%u\n",
+				       prop->pointer.u8_data[i]);
+		break;
+	case DEV_PROP_U16:
+		for (i = 0; i < prop->length / sizeof(u16); i++)
+			ret += sprintf(buf + ret, "0x%x\n",
+				       prop->pointer.u16_data[i]);
+		break;
+	case DEV_PROP_U32:
+		for (i = 0; i < prop->length / sizeof(u32); i++)
+			ret += sprintf(buf + ret, "0x%x\n",
+				       prop->pointer.u8_data[i]);
+		break;
+	case DEV_PROP_U64:
+		for (i = 0; i < prop->length / sizeof(u64); i++)
+			ret += sprintf(buf + ret, "0x%llx\n",
+				       prop->pointer.u64_data[i]);
+		break;
+	case DEV_PROP_STRING:
+		for (i = 0; i < prop->length / sizeof(const char *); i++)
+			ret += sprintf(buf + ret, "%s\n", prop->pointer.str[i]);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static ssize_t software_node_property_show(struct kobject *kobj,
+					   struct kobj_attribute *attr,
+					   char *buf)
+{
+	struct software_node *swnode = kobj_to_swnode(kobj);
+	const struct property_entry *prop;
+
+	for (prop = swnode->properties; prop->name; prop++)
+		if (prop->name == attr->attr.name)
+			break;
+
+	if (prop->is_array)
+		return property_array_show(prop, buf);
+
+	/* boolean property */
+	if (!prop->length)
+		return sprintf(buf, "1\n");
+
+	switch (prop->type) {
+	case DEV_PROP_U8:
+		return sprintf(buf, "%u\n", prop->value.u8_data);
+	case DEV_PROP_U16:
+		return sprintf(buf, "0x%x\n", prop->value.u16_data);
+	case DEV_PROP_U32:
+		return sprintf(buf, "0x%x\n", prop->value.u32_data);
+	case DEV_PROP_U64:
+		return sprintf(buf, "0x%llx\n", prop->value.u64_data);
+	case DEV_PROP_STRING:
+		return sprintf(buf, "%s\n", prop->value.str);
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int
+software_node_register_properties(struct software_node *swnode,
+				  const struct property_entry *properties)
+{
+	struct kobj_attribute *property_attrs;
+	struct attribute **group_attrs;
+	struct property_entry *props;
+	int i, n = 0;
+
+	if (!properties)
+		return 0;
+
+	while (properties[n].name)
+		n++;
+
+	property_attrs = kcalloc(n, sizeof(*property_attrs), GFP_KERNEL);
+	if (!property_attrs)
+		return -ENOMEM;
+
+	group_attrs = kcalloc(n + 1, sizeof(*group_attrs), GFP_KERNEL);
+	if (!group_attrs) {
+		kfree(property_attrs);
+		return -ENOMEM;
+	}
+
+	props = property_entries_dup(properties);
+	if (IS_ERR(props)) {
+		kfree(property_attrs);
+		kfree(group_attrs);
+		return PTR_ERR(props);
+	}
+
+	for (i = 0; i < n; i++) {
+		sysfs_attr_init(&property_attrs[i].attr);
+		property_attrs[i].attr.mode = 0444;
+		property_attrs[i].attr.name = props[i].name;
+		property_attrs[i].show = software_node_property_show;
+		group_attrs[i] = &property_attrs[i].attr;
+	}
+
+	swnode->property_group.name = "properties";
+	swnode->property_group.attrs = group_attrs;
+	swnode->property_attrs = property_attrs;
+	swnode->properties = props;
+
+	return sysfs_create_group(&swnode->kobj, &swnode->property_group);
+}
+
+static void software_node_release(struct kobject *kobj)
+{
+	struct software_node *swnode = kobj_to_swnode(kobj);
+
+	if (swnode->parent) {
+		ida_simple_remove(&swnode->parent->child_ids, swnode->id);
+		list_del(&swnode->entry);
+	} else {
+		ida_simple_remove(&swnode_root_ids, swnode->id);
+	}
+
+	ida_destroy(&swnode->child_ids);
+	property_entries_free(swnode->properties);
+	kfree(swnode->property_group.attrs);
+	kfree(swnode->property_attrs);
+	kfree(swnode);
+}
+
+static struct kobj_type software_node_type = {
+	.release = software_node_release,
+	.sysfs_ops = &kobj_sysfs_ops,
+};
+
+#define NODE_NAME_MAXSIZE	11
+
+struct fwnode_handle *
+fwnode_create_software_node(const struct property_entry *properties,
+			    const struct fwnode_handle *parent)
+{
+	char node_name[NODE_NAME_MAXSIZE];
+	struct software_node *p = NULL;
+	struct software_node *swnode;
+	int ret;
+
+	if (parent) {
+		if (IS_ERR(parent))
+			return ERR_CAST(parent);
+		if (!is_software_node(parent))
+			return ERR_PTR(-EINVAL);
+		p = to_software_node(parent);
+	}
+
+	swnode = kzalloc(sizeof(*swnode), GFP_KERNEL);
+	if (!swnode)
+		return ERR_PTR(-ENOMEM);
+
+	swnode->id = ida_simple_get(p ? &p->child_ids : &swnode_root_ids, 0, 0,
+				    GFP_KERNEL);
+	if (swnode->id < 0) {
+		kfree(swnode);
+		return ERR_PTR(swnode->id);
+	}
+
+	sprintf(node_name, "node%d", swnode->id);
+
+	swnode->kobj.kset = swnode_kset;
+	swnode->fwnode.ops = &software_node_ops;
+
+	ida_init(&swnode->child_ids);
+	INIT_LIST_HEAD(&swnode->entry);
+	INIT_LIST_HEAD(&swnode->children);
+	swnode->parent = p;
+
+	if (p)
+		list_add_tail(&swnode->entry, &p->children);
+
+	ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
+				   p ? &p->kobj : NULL, node_name);
+	if (ret) {
+		kobject_put(&swnode->kobj);
+		return ERR_PTR(ret);
+	}
+
+	ret = software_node_register_properties(swnode, properties);
+	if (ret) {
+		kobject_put(&swnode->kobj);
+		return ERR_PTR(ret);
+	}
+
+	kobject_uevent(&swnode->kobj, KOBJ_ADD);
+	return &swnode->fwnode;
+}
+EXPORT_SYMBOL_GPL(fwnode_create_software_node);
+
+void fwnode_remove_software_node(struct fwnode_handle *fwnode)
+{
+	struct software_node *swnode = to_software_node(fwnode);
+
+	if (!swnode)
+		return;
+
+	if (swnode->properties)
+		sysfs_remove_group(&swnode->kobj, &swnode->property_group);
+
+	kobject_put(&swnode->kobj);
+}
+EXPORT_SYMBOL_GPL(fwnode_remove_software_node);
+
+int software_node_notify(struct device *dev, unsigned long action)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	struct software_node *swnode;
+	int ret;
+
+	if (!fwnode)
+		return 0;
+
+	if (!is_software_node(fwnode))
+		fwnode = fwnode->secondary;
+	if (!is_software_node(fwnode))
+		return 0;
+
+	swnode = to_software_node(fwnode);
+
+	switch (action) {
+	case KOBJ_ADD:
+		ret = sysfs_create_link(&dev->kobj, &swnode->kobj,
+					"software_node");
+		if (ret)
+			break;
+
+		ret = sysfs_create_link(&swnode->kobj, &dev->kobj,
+					dev_name(dev));
+		if (ret) {
+			sysfs_remove_link(&dev->kobj, "software_node");
+			break;
+		}
+		kobject_get(&swnode->kobj);
+		break;
+	case KOBJ_REMOVE:
+		sysfs_remove_link(&swnode->kobj, dev_name(dev));
+		sysfs_remove_link(&dev->kobj, "software_node");
+		kobject_put(&swnode->kobj);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int __init software_node_init(void)
+{
+	swnode_kset = kset_create_and_add("software_nodes", NULL, kernel_kobj);
+	if (!swnode_kset)
+		return -ENOMEM;
+	return 0;
+}
+postcore_initcall(software_node_init);
+
+static void __exit software_node_exit(void)
+{
+	ida_destroy(&swnode_root_ids);
+	kset_unregister(swnode_kset);
+}
+__exitcall(software_node_exit);
diff --git a/include/linux/property.h b/include/linux/property.h
index ac8a1ebc4c1b..3789ec755fb6 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -311,4 +311,16 @@ fwnode_graph_get_remote_node(const struct fwnode_handle *fwnode, u32 port,
 int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 				struct fwnode_endpoint *endpoint);
 
+/* -------------------------------------------------------------------------- */
+/* Software fwnode support - when HW description is incomplete or missing */
+
+bool is_software_node(const struct fwnode_handle *fwnode);
+
+int software_node_notify(struct device *dev, unsigned long action);
+
+struct fwnode_handle *
+fwnode_create_software_node(const struct property_entry *properties,
+			    const struct fwnode_handle *parent);
+void fwnode_remove_software_node(struct fwnode_handle *fwnode);
+
 #endif /* _LINUX_PROPERTY_H_ */
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC PATCH 4/5] device property: Move device_add_properties() to swnode.c
  2018-10-12 11:39 [RFC PATCH 0/5] device property: Introducing software nodes Heikki Krogerus
                   ` (2 preceding siblings ...)
  2018-10-12 11:39 ` [RFC PATCH 3/5] drivers: base: Introducing software nodes to the firmware node framework Heikki Krogerus
@ 2018-10-12 11:39 ` Heikki Krogerus
  2018-10-12 11:39 ` [RFC PATCH 5/5] device property: Remove struct property_set Heikki Krogerus
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Heikki Krogerus @ 2018-10-12 11:39 UTC (permalink / raw)
  To: Dmitry Torokhov, Linus Walleij, Rafael J. Wysocki,
	Andy Shevchenko, Mika Westerberg
  Cc: linux-kernel, linux-acpi

Concentrating struct property_entry processing to
drivers/base/swnode.c

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/property.c | 179 --------------------------------------
 drivers/base/swnode.c   | 184 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 184 insertions(+), 179 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 240ab5230ff6..e20642759c67 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -84,44 +84,6 @@ static const void *property_get_pointer(const struct property_entry *prop)
 	}
 }
 
-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)
 {
@@ -759,147 +721,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
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 77d5645d2fcf..ea120e216a31 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -66,6 +66,45 @@ property_entry_get(const struct property_entry *prop, const char *name)
 	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 *property_get_pointer(const struct property_entry *prop)
 {
 	switch (prop->type) {
@@ -243,6 +282,151 @@ static int property_entry_read_string_array(const struct property_entry *props,
 	return array_len;
 }
 
+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;
+	int ret;
+
+	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++) {
+		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;
+
+	if (!properties)
+		return;
+
+	for (p = properties; p->name; p++)
+		property_entry_free_data(p);
+
+	kfree(properties);
+}
+EXPORT_SYMBOL_GPL(property_entries_free);
+
 /* -------------------------------------------------------------------------- */
 /* fwnode operations */
 
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [RFC PATCH 5/5] device property: Remove struct property_set
  2018-10-12 11:39 [RFC PATCH 0/5] device property: Introducing software nodes Heikki Krogerus
                   ` (3 preceding siblings ...)
  2018-10-12 11:39 ` [RFC PATCH 4/5] device property: Move device_add_properties() to swnode.c Heikki Krogerus
@ 2018-10-12 11:39 ` Heikki Krogerus
  2018-10-16  7:35 ` [RFC PATCH 0/5] device property: Introducing software nodes Linus Walleij
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Heikki Krogerus @ 2018-10-12 11:39 UTC (permalink / raw)
  To: Dmitry Torokhov, Linus Walleij, Rafael J. Wysocki,
	Andy Shevchenko, Mika Westerberg
  Cc: linux-kernel, linux-acpi

Replacing struct property_set with the software nodes that
were just introduced.

The API and functionality for adding properties to devices
remains the same, however, the goal is to convert the
drivers to use the API for software nodes when the device
has no real firmware node, and use the old API only when
"extra" build-in properties are needed.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/property.c | 350 +++-------------------------------------
 1 file changed, 26 insertions(+), 324 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index e20642759c67..0a543dd99582 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -18,198 +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 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 ?
@@ -217,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
@@ -722,114 +485,53 @@ int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
 EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
 
 /**
- * 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
+ * device_add_properties - Add a collection of properties to a device object.
+ * @dev: Device to add properties to.
+ * @properties: Collection of properties to add.
  *
- * 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.
+ * Associate a collection of device properties represented by @properties with
+ * @dev. The function takes a copy of @properties.
  *
- * Return: Pointer to the new property set or error pointer.
+ * WARNING: The callers should not use this function if it is known that there
+ * is no real firmware node associated with @dev! In that case the callers
+ * should create a software node and assign it to @dev directly.
  */
-static struct property_set *pset_copy_set(const struct property_set *pset)
+int device_add_properties(struct device *dev,
+			  const struct property_entry *properties)
 {
-	struct property_entry *properties;
-	struct property_set *p;
-
-	p = kzalloc(sizeof(*p), GFP_KERNEL);
-	if (!p)
-		return ERR_PTR(-ENOMEM);
+	struct fwnode_handle *fwnode;
 
-	properties = property_entries_dup(pset->properties);
-	if (IS_ERR(properties)) {
-		kfree(p);
-		return ERR_CAST(properties);
-	}
+	fwnode = fwnode_create_software_node(properties, NULL);
+	if (IS_ERR(fwnode))
+		return PTR_ERR(fwnode);
 
-	p->properties = properties;
-	return p;
+	set_secondary_fwnode(dev, fwnode);
+	return 0;
 }
+EXPORT_SYMBOL_GPL(device_add_properties);
 
 /**
  * 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.
+ * 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;
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
 
-	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 (fwnode && !is_software_node(fwnode))
+		fwnode = fwnode->secondary;
+
+	if (is_software_node(fwnode)) {
+		set_secondary_fwnode(dev, NULL);
+		fwnode_remove_software_node(fwnode);
 	}
-	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
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/5] device property: Introducing software nodes
  2018-10-12 11:39 [RFC PATCH 0/5] device property: Introducing software nodes Heikki Krogerus
                   ` (4 preceding siblings ...)
  2018-10-12 11:39 ` [RFC PATCH 5/5] device property: Remove struct property_set Heikki Krogerus
@ 2018-10-16  7:35 ` Linus Walleij
  2018-10-16  7:36   ` Rafael J. Wysocki
  2018-10-16 14:35 ` Andy Shevchenko
  2018-10-16 17:32 ` Dmitry Torokhov
  7 siblings, 1 reply; 18+ messages in thread
From: Linus Walleij @ 2018-10-16  7:35 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Dmitry Torokhov, Rafael J. Wysocki, Andy Shevchenko,
	Mika Westerberg, linux-kernel, ACPI Devel Maling List

On Fri, Oct 12, 2018 at 1:39 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:

> To continue the discussion started by Dmitry [1], this is my proposal
> that I mentioned in my last mail. In short, the idea is that instead
> of trying to extend the support for the currently used struct
> property_set, I'm proposing that we introduce a completely new,
> independent type of fwnode, and replace the struct property_set with
> it. I'm calling the type "software node" here.

I'm a big fan of this approach.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
for all patches.

I don't know who can finally review and merge this though,
I guess Rafael?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/5] device property: Introducing software nodes
  2018-10-16  7:35 ` [RFC PATCH 0/5] device property: Introducing software nodes Linus Walleij
@ 2018-10-16  7:36   ` Rafael J. Wysocki
  2018-10-16  8:40     ` Heikki Krogerus
  0 siblings, 1 reply; 18+ messages in thread
From: Rafael J. Wysocki @ 2018-10-16  7:36 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Heikki Krogerus, Dmitry Torokhov, Rafael J. Wysocki,
	Andy Shevchenko, Mika Westerberg, Linux Kernel Mailing List,
	ACPI Devel Maling List

On Tue, Oct 16, 2018 at 9:35 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Oct 12, 2018 at 1:39 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
>
> > To continue the discussion started by Dmitry [1], this is my proposal
> > that I mentioned in my last mail. In short, the idea is that instead
> > of trying to extend the support for the currently used struct
> > property_set, I'm proposing that we introduce a completely new,
> > independent type of fwnode, and replace the struct property_set with
> > it. I'm calling the type "software node" here.
>
> I'm a big fan of this approach.
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> for all patches.
>
> I don't know who can finally review and merge this though,
> I guess Rafael?

Yes, that would be me. :-)

I no one speaks up against them, I'll pick them up.

Cheers,
Rafael

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/5] device property: Introducing software nodes
  2018-10-16  7:36   ` Rafael J. Wysocki
@ 2018-10-16  8:40     ` Heikki Krogerus
  2018-10-16  8:44       ` Rafael J. Wysocki
  0 siblings, 1 reply; 18+ messages in thread
From: Heikki Krogerus @ 2018-10-16  8:40 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linus Walleij, Dmitry Torokhov, Rafael J. Wysocki,
	Andy Shevchenko, Mika Westerberg, Linux Kernel Mailing List,
	ACPI Devel Maling List

On Tue, Oct 16, 2018 at 09:36:33AM +0200, Rafael J. Wysocki wrote:
> On Tue, Oct 16, 2018 at 9:35 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > On Fri, Oct 12, 2018 at 1:39 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> >
> > > To continue the discussion started by Dmitry [1], this is my proposal
> > > that I mentioned in my last mail. In short, the idea is that instead
> > > of trying to extend the support for the currently used struct
> > > property_set, I'm proposing that we introduce a completely new,
> > > independent type of fwnode, and replace the struct property_set with
> > > it. I'm calling the type "software node" here.
> >
> > I'm a big fan of this approach.
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > for all patches.
> >
> > I don't know who can finally review and merge this though,
> > I guess Rafael?
> 
> Yes, that would be me. :-)
> 
> I no one speaks up against them, I'll pick them up.

Let me send a final version of these.

I need to add one more patch to the series where I remove an extra
device_remove_properties() call from platform_device_del().

It's unnecessary in any case as device_del() calls
device_remove_properties() for every device, but as the properties are
removed there before the device is removed, we're unable to deduct
the final ref count in the "remove" platform notification since our
node is no longer bind to the device.


Thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/5] device property: Introducing software nodes
  2018-10-16  8:40     ` Heikki Krogerus
@ 2018-10-16  8:44       ` Rafael J. Wysocki
  0 siblings, 0 replies; 18+ messages in thread
From: Rafael J. Wysocki @ 2018-10-16  8:44 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Rafael J. Wysocki, Linus Walleij, Dmitry Torokhov,
	Rafael J. Wysocki, Andy Shevchenko, Mika Westerberg,
	Linux Kernel Mailing List, ACPI Devel Maling List

On Tue, Oct 16, 2018 at 10:40 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Tue, Oct 16, 2018 at 09:36:33AM +0200, Rafael J. Wysocki wrote:
> > On Tue, Oct 16, 2018 at 9:35 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > >
> > > On Fri, Oct 12, 2018 at 1:39 PM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > > To continue the discussion started by Dmitry [1], this is my proposal
> > > > that I mentioned in my last mail. In short, the idea is that instead
> > > > of trying to extend the support for the currently used struct
> > > > property_set, I'm proposing that we introduce a completely new,
> > > > independent type of fwnode, and replace the struct property_set with
> > > > it. I'm calling the type "software node" here.
> > >
> > > I'm a big fan of this approach.
> > > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > > for all patches.
> > >
> > > I don't know who can finally review and merge this though,
> > > I guess Rafael?
> >
> > Yes, that would be me. :-)
> >
> > I no one speaks up against them, I'll pick them up.
>
> Let me send a final version of these.
>
> I need to add one more patch to the series where I remove an extra
> device_remove_properties() call from platform_device_del().
>
> It's unnecessary in any case as device_del() calls
> device_remove_properties() for every device, but as the properties are
> removed there before the device is removed, we're unable to deduct
> the final ref count in the "remove" platform notification since our
> node is no longer bind to the device.

OK, I'll wait for an update, then.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 3/5] drivers: base: Introducing software nodes to the firmware node framework
  2018-10-12 11:39 ` [RFC PATCH 3/5] drivers: base: Introducing software nodes to the firmware node framework Heikki Krogerus
@ 2018-10-16 12:57   ` Andy Shevchenko
  2018-10-16 14:53     ` Heikki Krogerus
  0 siblings, 1 reply; 18+ messages in thread
From: Andy Shevchenko @ 2018-10-16 12:57 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Dmitry Torokhov, Linus Walleij, Rafael J. Wysocki,
	Mika Westerberg, linux-kernel, linux-acpi

On Fri, Oct 12, 2018 at 02:39:32PM +0300, Heikki Krogerus wrote:
> Software node is a new struct fwnode_handle type that can be
> used to describe devices in kernel (software). It is meant
> to complement fwnodes representing real firmware nodes when
> they are incomplete (for example missing device properties)
> and to supply the primary fwnode when the firmware lacks
> hardware description for a device completely.
> 
> The software node type is really meant to replace the
> currently used "property_set" struct fwnode_handle type. The
> handling of struct property_set is glued to the generic
> device property handling code, and it is not possible to
> create a struct property_set independently from a device
> that it is bind to. struct property_set is only created when
> device properties are added to already initialized struct
> device, and control of it is only possible from the generic
> property handling code.
> 
> Software nodes are instead designed to be created
> independently from the device entries (struct device). It
> makes them much more flexible, as then the device meant to
> be bind to the node can be created at a later time, and from
> another location. It is also possible to bind multiple
> devices to a single software node if needed.
> 
> The software node implementation also includes support for
> node hierarchy, which was the main motivation for this
> commit. The node hierarchy was something that was requested
> for the struct property_set, but it did not seem reasonable
> to try to extend the property_set support for that purpose.
> struct property_set was really meant only for device
> property handling like the name suggests.
> 
> Support for struct property_set is not yet removed in this
> commit, but it will be in the following one.

> +static int property_entry_read_string_array(const struct property_entry *props,
> +					    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 = property_entry_get(props, 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 = property_entry_count_elems_of_size(props, propname,
> +							  sizeof(const char *));

I understand where it comes from, but here we may use positive condition.

> +
> +	/* 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 = property_entry_find(props, propname, length);
> +	if (IS_ERR(pointer))
> +		return PTR_ERR(pointer);
> +
> +	memcpy(strings, pointer, length);
> +
> +	return array_len;
> +}

> +struct fwnode_handle *
> +software_node_get_next_child(const struct fwnode_handle *fwnode,
> +			     struct fwnode_handle *child)
> +{
> +	struct software_node *p = to_software_node(fwnode);
> +	struct software_node *c = to_software_node(child);
> +
> +	if (list_empty(&p->children) ||
> +	    (c && list_is_last(&c->entry, &p->children)))
> +		return NULL;
> +
> +	if (c)
> +		c = list_next_entry(c, entry);
> +	else
> +		c = list_first_entry(&p->children, struct software_node, entry);
> +	return &c->fwnode;
> +}

> +static ssize_t software_node_property_show(struct kobject *kobj,
> +					   struct kobj_attribute *attr,
> +					   char *buf)
> +{
> +	struct software_node *swnode = kobj_to_swnode(kobj);
> +	const struct property_entry *prop;
> +
> +	for (prop = swnode->properties; prop->name; prop++)
> +		if (prop->name == attr->attr.name)
> +			break;
> +
> +	if (prop->is_array)
> +		return property_array_show(prop, buf);
> +
> +	/* boolean property */
> +	if (!prop->length)
> +		return sprintf(buf, "1\n");
> +
> +	switch (prop->type) {

> +	case DEV_PROP_U8:
> +		return sprintf(buf, "%u\n", prop->value.u8_data);

I would expect same base for all numbers.

> +	case DEV_PROP_U16:
> +		return sprintf(buf, "0x%x\n", prop->value.u16_data);
> +	case DEV_PROP_U32:
> +		return sprintf(buf, "0x%x\n", prop->value.u32_data);
> +	case DEV_PROP_U64:
> +		return sprintf(buf, "0x%llx\n", prop->value.u64_data);
> +	case DEV_PROP_STRING:
> +		return sprintf(buf, "%s\n", prop->value.str);
> +	default:
> +		break;
> +	}

I just realize that we might need to export type of the node as well.
How can we distinguish string "251" from a number?

> +
> +	return -EINVAL;
> +}

> +}

> +#define NODE_NAME_MAXSIZE	11

sizeof(int) = 4 (32 bits), so, 32 * 3 / 10 ~= 10. On top are "node" and '\0'.
Thus, I would rather put 16 here. Or limit the maximum for ida_simple_get().

> +struct fwnode_handle *
> +fwnode_create_software_node(const struct property_entry *properties,
> +			    const struct fwnode_handle *parent)
> +{
> +	char node_name[NODE_NAME_MAXSIZE];
> +	struct software_node *p = NULL;
> +	struct software_node *swnode;
> +	int ret;
> +
> +	if (parent) {
> +		if (IS_ERR(parent))
> +			return ERR_CAST(parent);
> +		if (!is_software_node(parent))
> +			return ERR_PTR(-EINVAL);
> +		p = to_software_node(parent);
> +	}
> +
> +	swnode = kzalloc(sizeof(*swnode), GFP_KERNEL);
> +	if (!swnode)
> +		return ERR_PTR(-ENOMEM);
> +

> +	swnode->id = ida_simple_get(p ? &p->child_ids : &swnode_root_ids, 0, 0,
> +				    GFP_KERNEL);

> +	if (swnode->id < 0) {
> +		kfree(swnode);
> +		return ERR_PTR(swnode->id);
> +	}
> +
> +	sprintf(node_name, "node%d", swnode->id);
> +
> +	swnode->kobj.kset = swnode_kset;
> +	swnode->fwnode.ops = &software_node_ops;
> +
> +	ida_init(&swnode->child_ids);
> +	INIT_LIST_HEAD(&swnode->entry);
> +	INIT_LIST_HEAD(&swnode->children);
> +	swnode->parent = p;
> +
> +	if (p)
> +		list_add_tail(&swnode->entry, &p->children);
> +
> +	ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
> +				   p ? &p->kobj : NULL, node_name);
> +	if (ret) {
> +		kobject_put(&swnode->kobj);
> +		return ERR_PTR(ret);
> +	}
> +
> +	ret = software_node_register_properties(swnode, properties);
> +	if (ret) {
> +		kobject_put(&swnode->kobj);
> +		return ERR_PTR(ret);
> +	}
> +
> +	kobject_uevent(&swnode->kobj, KOBJ_ADD);
> +	return &swnode->fwnode;
> +}


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/5] device property: Introducing software nodes
  2018-10-12 11:39 [RFC PATCH 0/5] device property: Introducing software nodes Heikki Krogerus
                   ` (5 preceding siblings ...)
  2018-10-16  7:35 ` [RFC PATCH 0/5] device property: Introducing software nodes Linus Walleij
@ 2018-10-16 14:35 ` Andy Shevchenko
  2018-10-16 14:46   ` Heikki Krogerus
  2018-10-17 14:53   ` Heikki Krogerus
  2018-10-16 17:32 ` Dmitry Torokhov
  7 siblings, 2 replies; 18+ messages in thread
From: Andy Shevchenko @ 2018-10-16 14:35 UTC (permalink / raw)
  To: Krogerus, Heikki
  Cc: Dmitry Torokhov, Linus Walleij, Rafael J. Wysocki,
	Andy Shevchenko, Mika Westerberg, Linux Kernel Mailing List,
	ACPI Devel Maling List

On Fri, Oct 12, 2018 at 2:41 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi guys,
>
> To continue the discussion started by Dmitry [1], this is my proposal
> that I mentioned in my last mail. In short, the idea is that instead
> of trying to extend the support for the currently used struct
> property_set, I'm proposing that we introduce a completely new,
> independent type of fwnode, and replace the struct property_set with
> it. I'm calling the type "software node" here.
>
> The reason for a complete separation of the software nodes from the
> generic property handling code is the need to be able to create the
> nodes independently from the devices that they are bind to.
>
> The way this works is that every node that is created will have a
> kobject registered. That will take care the ref counting for us, and
> also allow us to for example display the properties in sysfs.
>
> There are a few more details in patch 3/5 about the software nodes in
> the commit message.
>
> [1] https://lkml.org/lkml/2018/9/17/1067

In private discussion I brought a concern that we exposed properties
as a part of ABI, but at the same time we have not strict rules which
might lead to ambiguous reading, e.g. there is no type exported and
thus no possibility to tell what kind of property it is.

Examples:
1. 0x1 and 0x1 — are they of the same type?
2. 0x1 — is it an array or single value?
3. 0x12345678 — is it string or hex?
4. 25 — is it hex or decimal?

Until these will not be solved, better to not to expose properties to userspace.

>
> --
> heikki
>
>
> Heikki Krogerus (5):
>   drivers core: Prepare support for multiple platform notifications
>   ACPI / glue: Add acpi_platform_notify() function
>   drivers: base: Introducing software nodes to the firmware node
>     framework
>   device property: Move device_add_properties() to swnode.c
>   device property: Remove struct property_set
>
>  .../ABI/testing/sysfs-devices-software_node   |  27 +
>  drivers/acpi/bus.c                            |   1 -
>  drivers/acpi/glue.c                           |  21 +-
>  drivers/acpi/internal.h                       |   1 -
>  drivers/base/Makefile                         |   2 +-
>  drivers/base/core.c                           |  32 +-
>  drivers/base/property.c                       | 529 +-----------
>  drivers/base/swnode.c                         | 812 ++++++++++++++++++
>  include/linux/acpi.h                          |  10 +
>  include/linux/property.h                      |  12 +
>  10 files changed, 929 insertions(+), 518 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-devices-software_node
>  create mode 100644 drivers/base/swnode.c
>
> --
> 2.19.1
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/5] device property: Introducing software nodes
  2018-10-16 14:35 ` Andy Shevchenko
@ 2018-10-16 14:46   ` Heikki Krogerus
  2018-10-17 14:53   ` Heikki Krogerus
  1 sibling, 0 replies; 18+ messages in thread
From: Heikki Krogerus @ 2018-10-16 14:46 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Dmitry Torokhov, Linus Walleij, Rafael J. Wysocki,
	Andy Shevchenko, Mika Westerberg, Linux Kernel Mailing List,
	ACPI Devel Maling List

On Tue, Oct 16, 2018 at 05:35:50PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 12, 2018 at 2:41 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi guys,
> >
> > To continue the discussion started by Dmitry [1], this is my proposal
> > that I mentioned in my last mail. In short, the idea is that instead
> > of trying to extend the support for the currently used struct
> > property_set, I'm proposing that we introduce a completely new,
> > independent type of fwnode, and replace the struct property_set with
> > it. I'm calling the type "software node" here.
> >
> > The reason for a complete separation of the software nodes from the
> > generic property handling code is the need to be able to create the
> > nodes independently from the devices that they are bind to.
> >
> > The way this works is that every node that is created will have a
> > kobject registered. That will take care the ref counting for us, and
> > also allow us to for example display the properties in sysfs.
> >
> > There are a few more details in patch 3/5 about the software nodes in
> > the commit message.
> >
> > [1] https://lkml.org/lkml/2018/9/17/1067
> 
> In private discussion I brought a concern that we exposed properties
> as a part of ABI, but at the same time we have not strict rules which
> might lead to ambiguous reading, e.g. there is no type exported and
> thus no possibility to tell what kind of property it is.
> 
> Examples:
> 1. 0x1 and 0x1 ??? are they of the same type?
> 2. 0x1 ??? is it an array or single value?
> 3. 0x12345678 ??? is it string or hex?
> 4. 25 ??? is it hex or decimal?
> 
> Until these will not be solved, better to not to expose properties to userspace.

I agree. I'll drop that part from my final version.


Thanks Andy,

-- 
heikki

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 3/5] drivers: base: Introducing software nodes to the firmware node framework
  2018-10-16 12:57   ` Andy Shevchenko
@ 2018-10-16 14:53     ` Heikki Krogerus
  0 siblings, 0 replies; 18+ messages in thread
From: Heikki Krogerus @ 2018-10-16 14:53 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Dmitry Torokhov, Linus Walleij, Rafael J. Wysocki,
	Mika Westerberg, linux-kernel, linux-acpi

On Tue, Oct 16, 2018 at 03:57:43PM +0300, Andy Shevchenko wrote:
> > +static int property_entry_read_string_array(const struct property_entry *props,
> > +					    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 = property_entry_get(props, 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 = property_entry_count_elems_of_size(props, propname,
> > +							  sizeof(const char *));
> 
> I understand where it comes from, but here we may use positive condition.

OK.

> > +#define NODE_NAME_MAXSIZE	11
> 
> sizeof(int) = 4 (32 bits), so, 32 * 3 / 10 ~= 10. On top are "node" and '\0'.
> Thus, I would rather put 16 here. Or limit the maximum for ida_simple_get().

Let's make it 16.


thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/5] device property: Introducing software nodes
  2018-10-12 11:39 [RFC PATCH 0/5] device property: Introducing software nodes Heikki Krogerus
                   ` (6 preceding siblings ...)
  2018-10-16 14:35 ` Andy Shevchenko
@ 2018-10-16 17:32 ` Dmitry Torokhov
  2018-10-17 13:36   ` Heikki Krogerus
  7 siblings, 1 reply; 18+ messages in thread
From: Dmitry Torokhov @ 2018-10-16 17:32 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Linus Walleij, Rafael J. Wysocki, Andy Shevchenko,
	Mika Westerberg, linux-kernel, linux-acpi

Hi Heikki,

On Fri, Oct 12, 2018 at 02:39:29PM +0300, Heikki Krogerus wrote:
> Hi guys,
> 
> To continue the discussion started by Dmitry [1], this is my proposal
> that I mentioned in my last mail. In short, the idea is that instead
> of trying to extend the support for the currently used struct
> property_set, I'm proposing that we introduce a completely new,
> independent type of fwnode, and replace the struct property_set with
> it. I'm calling the type "software node" here.
> 
> The reason for a complete separation of the software nodes from the
> generic property handling code is the need to be able to create the
> nodes independently from the devices that they are bind to.

It would be great it you would provide an example of creating these
sowftware property sets separately from devices. How do you tie device
and its properties if they are not created together? For OF we have
compatibles and phandles for references, ACPI has HIDs and CIDs and
notion of references as well. What do we use here, especially when
software node is created in one subsystem (let's say
drivers/platform/x86), but device is created somewhere else?

Another issue that is not clear to me: looking at the USB connector it
seems you want to have references to fwnodes. How do you resolve them
when there are nodes of different class. I.e. how do you express
software fwnode referencing ACPI or DT node when you are supplementing
ACPI or DT description of a system with these custom/secondary nodes?
What about the other direction? I.e. can I have a DT system with USB
connector and augment USB set up with static nodes? Not only
basic/scalar properties, but links as well?

As I said, having and example of using this new code to achieve your
goal with regard to USB connector would be awesome and clear a lot of my
questions.

Thanks!

-- 
Dmitry

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/5] device property: Introducing software nodes
  2018-10-16 17:32 ` Dmitry Torokhov
@ 2018-10-17 13:36   ` Heikki Krogerus
  2018-10-18  1:06     ` Dmitry Torokhov
  0 siblings, 1 reply; 18+ messages in thread
From: Heikki Krogerus @ 2018-10-17 13:36 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Linus Walleij, Rafael J. Wysocki, Andy Shevchenko,
	Mika Westerberg, linux-kernel, linux-acpi

[-- Attachment #1: Type: text/plain, Size: 6551 bytes --]

On Tue, Oct 16, 2018 at 10:32:44AM -0700, Dmitry Torokhov wrote:
> Hi Heikki,
> 
> On Fri, Oct 12, 2018 at 02:39:29PM +0300, Heikki Krogerus wrote:
> > Hi guys,
> > 
> > To continue the discussion started by Dmitry [1], this is my proposal
> > that I mentioned in my last mail. In short, the idea is that instead
> > of trying to extend the support for the currently used struct
> > property_set, I'm proposing that we introduce a completely new,
> > independent type of fwnode, and replace the struct property_set with
> > it. I'm calling the type "software node" here.
> > 
> > The reason for a complete separation of the software nodes from the
> > generic property handling code is the need to be able to create the
> > nodes independently from the devices that they are bind to.
> 
> It would be great it you would provide an example of creating these
> sowftware property sets separately from devices. How do you tie device
> and its properties if they are not created together?

The properties are bind to the software nodes and not the devices. The
software nodes can then be bind to the devices when they are created.
This is actually exactly the same behaviour that we had with the
property_sets, the only difference being that we can bind the software
node to the device at a later stage.

> For OF we have compatibles and phandles for references, ACPI has
> HIDs and CIDs and notion of references as well. What do we use here,
> especially when software node is created in one subsystem (let's say
> drivers/platform/x86), but device is created somewhere else?

Reference usually means a handle to a node that is outside of the
direct child-parent relationship (hierarchy) for the caller device
node. That I do not support at this point. We support the node
hierarchy which allows us to "refer" the child and parent nodes, and
that is all that we need at this stage.

Support for references is in my plans. I will need that later. But
we'll do that as the next step.

> Another issue that is not clear to me: looking at the USB connector it
> seems you want to have references to fwnodes.

If by references you mean here access to the nodes outside of the
hierarchy, then you've misunderstood. I do not expect that with the
USB connectors.

> How do you resolve them when there are nodes of different class.
> I.e. how do you express software fwnode referencing ACPI or DT node
> when you are supplementing ACPI or DT description of a system with
> these custom/secondary nodes?

So references are not supported as I said, but I don't know if we can
or even want to support references to different types of fwnodes. I'm
not even sure we would ever need to refer an other type of fwnode from
software node. I mean, we should always be able to place a secondary
software node to both fwnodes.

In any case, this topic is outside the scope of this series.

And in case this was not clear, with the hierarchy, different types of
fwnode nodes are not supported. It means that software node can only
have software node parent and children.

> What about the other direction? I.e. can I have a DT system with USB
> connector and augment USB set up with static nodes? Not only
> basic/scalar properties, but links as well?
> 
> As I said, having and example of using this new code to achieve your
> goal with regard to USB connector would be awesome and clear a lot of my
> questions.

OK. I used the attached code to test these on Intel Cherry Trail
board. I'm creating two software nodes there: one for the FUSB302
controller, and one (a child of the FUSB302 node) for the connector.

The fusb302 node is assigned to the i2c client that is registered in
that driver, but the child node is left waiting. The child node has a
property called "name" with value "connector".

The fusb302 driver already requests a handle to the child node named
"connector" in its probe function which it assigns to the port device
that it registers. With the attached patch it will get the child node
also on CherryTrail boards, no changes to the Type-C drivers needed.

Here is the file listing that we see in sysfs (node3 is for FUSB302):

        % find /sys/kernel/software_nodes/ | grep -v properties
        ...
        /sys/kernel/software_nodes/node3
        /sys/kernel/software_nodes/node3/node0
        /sys/kernel/software_nodes/node3/node0/port0
        /sys/kernel/software_nodes/node3/i2c-fusb302
        ...

The node for the FUSB302 controller:

        % ls -l /sys/kernel/software_nodes/node3/
        total 0
        lrwxrwxrwx    1 root     root             0 Oct 17 12:12 i2c-fusb302 -> ../../../devices/pci0000:00/808622C1:00/i2c-0/i2c-fusb302
        drwxr-xr-x    3 root     root             0 Oct 17 12:03 node0
        drwxr-xr-x    2 root     root             0 Oct 17 12:12 properties

The node for the connector (child of node3):

        % ls -l /sys/kernel/software_nodes/node3/node0/
        total 0
        lrwxrwxrwx    1 root     root             0 Oct 17 12:12 port0 -> ../../../../devices/pci0000:00/808622C1:00/i2c-0/i2c-fusb302/typec/port0
        drwxr-xr-x    2 root     root             0 Oct 17 12:12 properties

And this is what the actual connector device directory looks like
(look at the "software_node" symlink:

        % ls -l /sys/class/typec/port0/
        total 0
        -rw-r--r--    1 root     root          4096 Oct 17 12:16 data_role
        lrwxrwxrwx    1 root     root             0 Oct 17 12:16 device -> ../../../i2c-fusb302
        -rw-r--r--    1 root     root          4096 Oct 17 12:16 port_type
        drwxr-xr-x    2 root     root             0 Oct 17 12:16 power
        -r--r--r--    1 root     root          4096 Oct 17 12:16 power_operation_mode
        -rw-r--r--    1 root     root          4096 Oct 17 12:16 power_role
        -rw-r--r--    1 root     root          4096 Oct 17 12:16 preferred_role
        lrwxrwxrwx    1 root     root             0 Oct 17 12:16 software_node -> ../../../../../../../kernel/software_nodes/node3/node0
        lrwxrwxrwx    1 root     root             0 Oct 17 12:16 subsystem -> ../../../../../../../class/typec
        -r--r--r--    1 root     root          4096 Oct 17 12:16 supported_accessory_modes
        -rw-r--r--    1 root     root          4096 Oct 17 12:03 uevent
        -r--r--r--    1 root     root          4096 Oct 17 12:16 usb_power_delivery_revision
        -r--r--r--    1 root     root          4096 Oct 17 12:16 usb_typec_revision
        -rw-r--r--    1 root     root          4096 Oct 17 12:16 vconn_source


thanks,

-- 
heikki

[-- Attachment #2: intel_cht_int33fe.diff --]
[-- Type: text/plain, Size: 2612 bytes --]

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 7787c6ed9671..108ed001acf8 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -30,6 +30,8 @@ struct cht_int33fe_data {
 	struct i2c_client *max17047;
 	struct i2c_client *fusb302;
 	struct i2c_client *pi3usb30532;
+	struct fwnode_handle *port_node;
+	struct fwnode_handle *fusb302_node;
 	/* Contain a list-head must be per device */
 	struct device_connection connections[5];
 };
@@ -85,6 +87,12 @@ static const struct property_entry fusb302_props[] = {
 	{ }
 };
 
+static const struct property_entry usb_connector_props[] = {
+	PROPERTY_ENTRY_STRING("name", "connector"),
+	PROPERTY_ENTRY_STRING("data-role", "dual"),
+	{ }
+};
+
 static int cht_int33fe_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -150,6 +158,19 @@ static int cht_int33fe_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
+	/* Node for the FUSB302 controller */
+	data->fusb302_node = fwnode_create_software_node(fusb302_props, NULL);
+	if (IS_ERR(data->fusb302_node))
+		return PTR_ERR(data->fusb302_node);
+
+	/* Node for the connector (FUSB302 is the parent) */
+	data->port_node = fwnode_create_software_node(usb_connector_props,
+						      data->fusb302_node);
+	if (IS_ERR(data->port_node)) {
+		fwnode_remove_software_node(data->fusb302_node);
+		return PTR_ERR(data->port_node);
+	}
+
 	/* Work around BIOS bug, see comment on cht_int33fe_find_max17047 */
 	max17047 = cht_int33fe_find_max17047();
 	if (max17047) {
@@ -189,7 +210,7 @@ static int cht_int33fe_probe(struct i2c_client *client)
 	memset(&board_info, 0, sizeof(board_info));
 	strlcpy(board_info.type, "typec_fusb302", I2C_NAME_SIZE);
 	board_info.dev_name = "fusb302";
-	board_info.properties = fusb302_props;
+	board_info.fwnode = data->fusb302_node;
 	board_info.irq = fusb302_irq;
 
 	data->fusb302 = i2c_acpi_new_device(dev, 2, &board_info);
@@ -216,6 +237,8 @@ static int cht_int33fe_probe(struct i2c_client *client)
 		i2c_unregister_device(data->max17047);
 
 	device_connections_remove(data->connections);
+	fwnode_remove_software_node(data->port_node);
+	fwnode_remove_software_node(data->fusb302_node);
 
 	return -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
 }
@@ -230,6 +253,8 @@ static int cht_int33fe_remove(struct i2c_client *i2c)
 		i2c_unregister_device(data->max17047);
 
 	device_connections_remove(data->connections);
+	fwnode_remove_software_node(data->port_node);
+	fwnode_remove_software_node(data->fusb302_node);
 
 	return 0;
 }

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/5] device property: Introducing software nodes
  2018-10-16 14:35 ` Andy Shevchenko
  2018-10-16 14:46   ` Heikki Krogerus
@ 2018-10-17 14:53   ` Heikki Krogerus
  1 sibling, 0 replies; 18+ messages in thread
From: Heikki Krogerus @ 2018-10-17 14:53 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Dmitry Torokhov, Linus Walleij, Rafael J. Wysocki,
	Andy Shevchenko, Mika Westerberg, Linux Kernel Mailing List,
	ACPI Devel Maling List

On Tue, Oct 16, 2018 at 05:35:50PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 12, 2018 at 2:41 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi guys,
> >
> > To continue the discussion started by Dmitry [1], this is my proposal
> > that I mentioned in my last mail. In short, the idea is that instead
> > of trying to extend the support for the currently used struct
> > property_set, I'm proposing that we introduce a completely new,
> > independent type of fwnode, and replace the struct property_set with
> > it. I'm calling the type "software node" here.
> >
> > The reason for a complete separation of the software nodes from the
> > generic property handling code is the need to be able to create the
> > nodes independently from the devices that they are bind to.
> >
> > The way this works is that every node that is created will have a
> > kobject registered. That will take care the ref counting for us, and
> > also allow us to for example display the properties in sysfs.
> >
> > There are a few more details in patch 3/5 about the software nodes in
> > the commit message.
> >
> > [1] https://lkml.org/lkml/2018/9/17/1067
> 
> In private discussion I brought a concern that we exposed properties
> as a part of ABI, but at the same time we have not strict rules which
> might lead to ambiguous reading, e.g. there is no type exported and
> thus no possibility to tell what kind of property it is.
> 
> Examples:
> 1. 0x1 and 0x1 ??? are they of the same type?
> 2. 0x1 ??? is it an array or single value?
> 3. 0x12345678 ??? is it string or hex?
> 4. 25 ??? is it hex or decimal?

This is mostly a note to self, but also to let everybody know:

After thinking about this a bit more I realised that the user space
has to know what the property it is reading contains. An array of
integers can actually be a string in reality, just like a string may
contain an integer value(s). String or string array could describe a
data structure, or even supply the values for one. In reality the type
of the property, or the fact that it's an array or not, do not help at
all to determine the content of the property.

So the user space has to know what a property returns if it wants
to use it, and once the user space knows that, the user space will
also know the type and other details about the property, including
knowing is it an array or not.

Based on that, I'm against any kind of grouping or naming of the
properties, was it based on the type of the property or is it an array
or not, or supplying any details about the properties in any way to
the user space. That would only complicate the life for the user
space, as the grouping or naming, or supplying the details about the
properties in any way, does not provide any information that the user
space does not already have. The details about the properties would
just be a sort of a useless noise for the user space that just has to
be filtered out.

Therefore I'm going to continue to propose that we expose the
properties exactly the way I'm doing now: we'll have the "properties"
directory that contains an attribute file for every property named
with the names of the properties, and nothing else.

The output formats we can still debate about, but Andy had already
good proposals for that.

I'm still not planning to include the property exposure at the first
stage. Well add it after the initial support is in.


Thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/5] device property: Introducing software nodes
  2018-10-17 13:36   ` Heikki Krogerus
@ 2018-10-18  1:06     ` Dmitry Torokhov
  0 siblings, 0 replies; 18+ messages in thread
From: Dmitry Torokhov @ 2018-10-18  1:06 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Linus Walleij, Rafael J. Wysocki, Andy Shevchenko,
	Mika Westerberg, linux-kernel, linux-acpi

Hi Heikki,

On Wed, Oct 17, 2018 at 04:36:12PM +0300, Heikki Krogerus wrote:
> @@ -189,7 +210,7 @@ static int cht_int33fe_probe(struct i2c_client *client)
>  	memset(&board_info, 0, sizeof(board_info));
>  	strlcpy(board_info.type, "typec_fusb302", I2C_NAME_SIZE);
>  	board_info.dev_name = "fusb302";
> -	board_info.properties = fusb302_props;
> +	board_info.fwnode = data->fusb302_node;
>  	board_info.irq = fusb302_irq;
>  
>  	data->fusb302 = i2c_acpi_new_device(dev, 2, &board_info);

OK, this totally explains it. I forgot how we can supply the properties
in board info for I2C/SPI devices and they would get attached to a
device later, and this obviously does not work if one wants to attach a
sub-tree.

Thank you for providing this example.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2018-10-18  1:06 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-12 11:39 [RFC PATCH 0/5] device property: Introducing software nodes Heikki Krogerus
2018-10-12 11:39 ` [RFC PATCH 1/5] drivers core: Prepare support for multiple platform notifications Heikki Krogerus
2018-10-12 11:39 ` [RFC PATCH 2/5] ACPI / glue: Add acpi_platform_notify() function Heikki Krogerus
2018-10-12 11:39 ` [RFC PATCH 3/5] drivers: base: Introducing software nodes to the firmware node framework Heikki Krogerus
2018-10-16 12:57   ` Andy Shevchenko
2018-10-16 14:53     ` Heikki Krogerus
2018-10-12 11:39 ` [RFC PATCH 4/5] device property: Move device_add_properties() to swnode.c Heikki Krogerus
2018-10-12 11:39 ` [RFC PATCH 5/5] device property: Remove struct property_set Heikki Krogerus
2018-10-16  7:35 ` [RFC PATCH 0/5] device property: Introducing software nodes Linus Walleij
2018-10-16  7:36   ` Rafael J. Wysocki
2018-10-16  8:40     ` Heikki Krogerus
2018-10-16  8:44       ` Rafael J. Wysocki
2018-10-16 14:35 ` Andy Shevchenko
2018-10-16 14:46   ` Heikki Krogerus
2018-10-17 14:53   ` Heikki Krogerus
2018-10-16 17:32 ` Dmitry Torokhov
2018-10-17 13:36   ` Heikki Krogerus
2018-10-18  1:06     ` Dmitry Torokhov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).