[PATCH 0/4] of: dynamic: Changesets helpers & fixes

[PATCH 0/4] of: dynamic: Changesets helpers & fixes

[Pantelis Antoniou]

This patchset introduces changeset helpers which makes working with
changeset much easier and less error prone.

The first patch fixes a bug when using an update property changeset operation
but the property does not exist.

The second exports an internal method which is used for the third patch
which contains the bulk of the changes.

Finally the last patch adds a unittest for the changeset helpers.

Pantelis Antoniou (4):
  of: dynamic: changeset prop-update revert fix
  of: dynamic: Add __of_node_dupv()
  of: changesets: Introduce changeset helper methods
  of: unittest: changeset helpers

 drivers/of/dynamic.c  | 535 +++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/of/unittest.c |  54 +++++
 include/linux/of.h    | 143 ++++++++++++++
 3 files changed, 726 insertions(+), 6 deletions(-)

-- 
1.7.12

[PATCH 1/4] of: dynamic: changeset prop-update revert fix

[Pantelis Antoniou]

When reverting an update property changeset entry that created a
property the reverse operation is a remove property and not an update.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/dynamic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index c647bd1..4145b44 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -497,6 +497,11 @@ static void __of_changeset_entry_invert(struct of_changeset_entry *ce,
 	case OF_RECONFIG_UPDATE_PROPERTY:
 		rce->old_prop = ce->prop;
 		rce->prop = ce->old_prop;
+		/* update was used but original property did not exist */
+		if (!rce->prop) {
+			rce->action = OF_RECONFIG_REMOVE_PROPERTY;
+			rce->prop = ce->prop;
+		}
 		break;
 	}
 }
-- 
1.7.12

[PATCH 2/4] of: dynamic: Add __of_node_dupv()

[Pantelis Antoniou]

Add an __of_node_dupv() private method and make __of_node_dup() use it.
This is required for the subsequent changeset accessors which will
make use of it.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/dynamic.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 4145b44..e4dea94 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -394,8 +394,9 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
 }
 
 /**
- * __of_node_dup() - Duplicate or create an empty device node dynamically.
- * @fmt: Format string (plus vargs) for new full name of the device node
+ * __of_node_dupv() - Duplicate or create an empty device node dynamically.
+ * @fmt: Format string for new full name of the device node
+ * @vargs: va_list containing the arugments for the node full name
  *
  * Create an device tree node, either by duplicating an empty node or by allocating
  * an empty one suitable for further modification.  The node data are
@@ -403,17 +404,15 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
  * OF_DETACHED bits set. Returns the newly allocated node or NULL on out of
  * memory error.
  */
-struct device_node *__of_node_dup(const struct device_node *np, const char *fmt, ...)
+struct device_node *__of_node_dupv(const struct device_node *np,
+		const char *fmt, va_list vargs)
 {
-	va_list vargs;
 	struct device_node *node;
 
 	node = kzalloc(sizeof(*node), GFP_KERNEL);
 	if (!node)
 		return NULL;
-	va_start(vargs, fmt);
 	node->full_name = kvasprintf(GFP_KERNEL, fmt, vargs);
-	va_end(vargs);
 	if (!node->full_name) {
 		kfree(node);
 		return NULL;
@@ -445,6 +444,24 @@ struct device_node *__of_node_dup(const struct device_node *np, const char *fmt,
 	return NULL;
 }
 
+/**
+ * __of_node_dup() - Duplicate or create an empty device node dynamically.
+ * @fmt: Format string (plus vargs) for new full name of the device node
+ *
+ * See: __of_node_dupv()
+ */
+struct device_node *__of_node_dup(const struct device_node *np,
+		const char *fmt, ...)
+{
+	va_list vargs;
+	struct device_node *node;
+
+	va_start(vargs, fmt);
+	node = __of_node_dupv(np, fmt, vargs);
+	va_end(vargs);
+	return node;
+}
+
 static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
 {
 	of_node_put(ce->np);
-- 
1.7.12

[PATCH 3/4] of: changesets: Introduce changeset helper methods

[Pantelis Antoniou]

Changesets are very powerful, but the lack of a helper API
makes using them cumbersome. Introduce a simple copy based
API that makes things considerably easier.

To wit, adding a property using the raw API.

	struct property *prop;
	prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
	prop->name = kstrdup("compatible");
	prop->value = kstrdup("foo,bar");
	prop->length = strlen(prop->value) + 1;
	of_changeset_add_property(ocs, np, prop);

while using the helper API

	of_changeset_add_property_string(ocs, np, "compatible",
			"foo,bar");

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/dynamic.c | 501 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h   | 143 +++++++++++++++
 2 files changed, 644 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e4dea94..350b1cf 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -828,3 +828,504 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_changeset_action);
+
+/* changeset helpers */
+
+/**
+ * of_changeset_create_device_node - Create an empty device node
+ *
+ * @ocs:	changeset pointer
+ * @parent:	parent device node
+ * @fmt:	format string for the node's full_name
+ * @args:	argument list for the format string
+ *
+ * Create an empty device node, marking it as detached and allocated.
+ *
+ * Returns a device node on success, an error encoded pointer otherwise
+ */
+struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs)
+{
+	struct device_node *node;
+
+	node = __of_node_dupv(NULL, fmt, vargs);
+	if (!node)
+		return ERR_PTR(-ENOMEM);
+
+	node->parent = parent;
+	return node;
+}
+
+/**
+ * of_changeset_create_device_node - Create an empty device node
+ *
+ * @ocs:	changeset pointer
+ * @parent:	parent device node
+ * @fmt:	Format string for the node's full_name
+ * ...		Arguments
+ *
+ * Create an empty device node, marking it as detached and allocated.
+ *
+ * Returns a device node on success, an error encoded pointer otherwise
+ */
+struct device_node *of_changeset_create_device_node(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, ...)
+{
+	va_list vargs;
+	struct device_node *node;
+
+	va_start(vargs, fmt);
+	node = of_changeset_create_device_nodev(ocs, parent, fmt, vargs);
+	va_end(vargs);
+	return node;
+}
+
+static int __of_changeset_add_update_property_copy(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const void *value,
+		int length, bool update)
+{
+	struct property *prop;
+	char *new_name;
+	void *new_value;
+	int ret = -ENOMEM;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop)
+		goto out_no_prop;
+
+	new_name = kstrdup(name, GFP_KERNEL);
+	if (!new_name)
+		goto out_no_name;
+
+	/*
+	 * NOTE: There is no check for zero length value.
+	 * In case of a boolean property, this will allocate a value
+	 * of zero bytes. We do this to work around the use
+	 * of of_get_property() calls on boolean values.
+	 */
+	new_value = kmemdup(value, length, GFP_KERNEL);
+	if (!new_value)
+		goto out_no_value;
+
+	of_property_set_flag(prop, OF_DYNAMIC);
+
+	prop->name = new_name;
+	prop->value = new_value;
+	prop->length = length;
+
+	if (!update)
+		ret = of_changeset_add_property(ocs, np, prop);
+	else
+		ret = of_changeset_update_property(ocs, np, prop);
+
+	if (ret != 0)
+		goto out_no_add;
+
+	return 0;
+
+out_no_add:
+	kfree(prop->value);
+out_no_value:
+	kfree(prop->name);
+out_no_name:
+	kfree(prop);
+out_no_prop:
+	return ret;
+}
+
+static int __of_changeset_add_update_property_string(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *str,
+		bool update)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, str,
+			strlen(str) + 1, update);
+}
+
+static int __of_changeset_add_update_property_stringv(struct of_changeset *ocs,
+		struct device_node *np, const char *name,
+		const char *fmt, va_list vargs, bool update)
+{
+	char *str;
+	int ret;
+
+	str = kvasprintf(GFP_KERNEL, fmt, vargs);
+	if (!str)
+		return -ENOMEM;
+
+	ret = __of_changeset_add_update_property_string(ocs, np, name, str, update);
+
+	kfree(str);
+
+	return ret;
+}
+
+static int __of_changeset_add_update_property_string_list(
+		struct of_changeset *ocs, struct device_node *np, const char *name,
+		const char **strs, int count, bool update)
+{
+	int total = 0, i, ret;
+	char *value, *s;
+
+	for (i = 0; i < count; i++) {
+		/* check if  it's NULL */
+		if (!strs[i])
+			return -EINVAL;
+		total += strlen(strs[i]) + 1;
+	}
+
+	value = kmalloc(total, GFP_KERNEL);
+	if (!value)
+		return -ENOMEM;
+
+	for (i = 0, s = value; i < count; i++) {
+		/* no need to check for NULL, check above */
+		strcpy(s, strs[i]);
+		s += strlen(strs[i]) + 1;
+	}
+
+	ret = __of_changeset_add_update_property_copy(ocs, np, name, value,
+			total, update);
+
+	kfree(value);
+
+	return ret;
+}
+
+static int __of_changeset_add_update_property_u32(struct of_changeset *ocs,
+		struct device_node *np, const char *name, u32 val, bool update)
+{
+	/* in place */
+	val = cpu_to_be32(val);
+	return __of_changeset_add_update_property_copy(ocs, np, name, &val,
+			sizeof(val), update);
+}
+
+static int __of_changeset_add_update_property_bool(struct of_changeset *ocs,
+		struct device_node *np, const char *name, bool update)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, "", 0,
+			update);
+}
+
+/**
+ * of_changeset_add_property_copy - Create a new property copying name & value
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @value:	pointer to the value data
+ * @length:	length of the value in bytes
+ *
+ * Adds a property to the changeset by making copies of the name & value
+ * entries.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_copy(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const void *value,
+		int length)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, value,
+			length, false);
+}
+
+/**
+ * of_changeset_add_property_string - Create a new string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @str:	string property
+ *
+ * Adds a string property to the changeset by making copies of the name
+ * and the string value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_string(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *str)
+{
+	return __of_changeset_add_update_property_string(ocs, np, name, str,
+			false);
+}
+
+/**
+ * of_changeset_add_property_stringf - Create a new formatted string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @fmt:	format of string property
+ * ...		arguments of the format string
+ *
+ * Adds a string property to the changeset by making copies of the name
+ * and the formatted value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_stringf(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *fmt, ...)
+{
+	va_list vargs;
+	int ret;
+
+	va_start(vargs, fmt);
+	ret = __of_changeset_add_update_property_stringv(ocs, np, name, fmt,
+			vargs, false);
+	va_end(vargs);
+	return ret;
+}
+
+/**
+ * of_changeset_add_property_string_list - Create a new string list property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @strs:	pointer to the string list
+ * @count:	string count
+ *
+ * Adds a string list property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_string_list(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char **strs,
+		int count)
+{
+	return __of_changeset_add_update_property_string_list(ocs, np, name,
+			strs, count, false);
+}
+
+/**
+ * of_changeset_add_property_u32 - Create a new u32 property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @val:	value in host endian format
+ *
+ * Adds a u32 property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_u32(struct of_changeset *ocs,
+		struct device_node *np, const char *name, u32 val)
+{
+	return __of_changeset_add_update_property_u32(ocs, np, name,
+			val, false);
+}
+
+/**
+ * of_changeset_add_property_bool - Create a new u32 property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ *
+ * Adds a bool property to the changeset. Note that there is
+ * no option to set the value to false, since the property
+ * existing sets it to true.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_bool(struct of_changeset *ocs,
+		struct device_node *np, const char *name)
+{
+	return __of_changeset_add_update_property_bool(ocs, np, name, false);
+}
+
+/**
+ * of_changeset_update_property_copy - Update a property copying name & value
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @value:	pointer to the value data
+ * @length:	length of the value in bytes
+ *
+ * Update a property to the changeset by making copies of the name & value
+ * entries.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_copy(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const void *value,
+		int length)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, value,
+			length, true);
+}
+
+/**
+ * of_changeset_update_property_string - Create a new string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @str:	string property
+ *
+ * Updates a string property to the changeset by making copies of the name
+ * and the string value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_string(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *str)
+{
+	return __of_changeset_add_update_property_string(ocs, np, name, str,
+			true);
+}
+
+/**
+ * of_changeset_update_property_stringf - Update formatted string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @fmt:	format of string property
+ * ...		arguments of the format string
+ *
+ * Updates a string property to the changeset by making copies of the name
+ * and the formatted value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_stringf(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *fmt, ...)
+{
+	va_list vargs;
+	int ret;
+
+	va_start(vargs, fmt);
+	ret = __of_changeset_add_update_property_stringv(ocs, np, name, fmt,
+			vargs, true);
+	va_end(vargs);
+	return ret;
+}
+
+/**
+ * of_changeset_update_property_string_list - Update string list property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @strs:	pointer to the string list
+ * @count:	string count
+ *
+ * Updates a string list property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_string_list(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char **strs,
+		int count)
+{
+	return __of_changeset_add_update_property_string_list(ocs, np, name,
+			strs, count, true);
+}
+
+/**
+ * of_changeset_update_property_u32 - Update u32 property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @val:	value in host endian format
+ *
+ * Updates a u32 property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_u32(struct of_changeset *ocs,
+		struct device_node *np, const char *name, u32 val)
+{
+	return __of_changeset_add_update_property_u32(ocs, np, name,
+			val, true);
+}
+
+/**
+ * of_changeset_update_property_bool - Update a bool property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ *
+ * Updates a property to the changeset. Note that there is
+ * no option to set the value to false, since the property
+ * existing sets it to true.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_bool(struct of_changeset *ocs,
+		struct device_node *np, const char *name)
+{
+	return __of_changeset_add_update_property_bool(ocs, np, name, true);
+}
+
+static struct device_node *
+__of_changeset_node_move_one(struct of_changeset *ocs,
+		struct device_node *np, struct device_node *new_parent)
+{
+	struct device_node *np2;
+	const char *unitname;
+	int err;
+
+	err = of_changeset_detach_node(ocs, np);
+	if (err)
+		return ERR_PTR(err);
+
+	unitname = strrchr(np->full_name, '/');
+	if (!unitname)
+		unitname = np->full_name;
+
+	np2 = __of_node_dup(np, "%s/%s",
+			new_parent->full_name, unitname);
+	if (!np2)
+		return ERR_PTR(-ENOMEM);
+	np2->parent = new_parent;
+
+	err = of_changeset_attach_node(ocs, np2);
+	if (err)
+		return ERR_PTR(err);
+
+	return np2;
+}
+
+/**
+ * of_changeset_node_move_to - Moves a subtree to a new place in
+ *                             the tree
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer to be moved
+ * @to:		device node of the new parent
+ *
+ * Moves a subtree to a new place in the tree.
+ * Note that a move is a safe operation because the phandles
+ * remain valid.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_node_move(struct of_changeset *ocs,
+		struct device_node *np, struct device_node *new_parent)
+{
+	struct device_node *npc, *nppc;
+
+	/* move the root first */
+	nppc = __of_changeset_node_move_one(ocs, np, new_parent);
+	if (IS_ERR(nppc))
+		return PTR_ERR(nppc);
+
+	/* move the subtrees next */
+	for_each_child_of_node(np, npc) {
+		nppc = __of_changeset_node_move_one(ocs, npc, nppc);
+		if (IS_ERR(nppc)) {
+			of_node_put(npc);
+			return PTR_ERR(nppc);
+		}
+	}
+
+	return 0;
+}
diff --git a/include/linux/of.h b/include/linux/of.h
index 3175803..f015911 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1037,6 +1037,42 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
 {
 	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
 }
+
+struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs);
+__printf(3, 4) struct device_node *of_changeset_create_device_node(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, ...);
+int of_changeset_add_property_copy(struct of_changeset *ocs,
+	struct device_node *np, const char *name,
+	const void *value, int length);
+int of_changeset_add_property_string(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char *str);
+__printf(4, 5) int of_changeset_add_property_stringf(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *fmt, ...);
+int of_changeset_add_property_string_list(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char **strs, int count);
+int of_changeset_add_property_u32(struct of_changeset *ocs,
+	struct device_node *np, const char *name, u32 val);
+int of_changeset_add_property_bool(struct of_changeset *ocs,
+	struct device_node *np, const char *name);
+int of_changeset_update_property_copy(struct of_changeset *ocs,
+	struct device_node *np, const char *name,
+	const void *value, int length);
+int of_changeset_update_property_string(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char *str);
+__printf(4, 5) int of_changeset_update_property_stringf(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *fmt, ...);
+int of_changeset_update_property_string_list(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char **strs, int count);
+int of_changeset_update_property_u32(struct of_changeset *ocs,
+	struct device_node *np, const char *name, u32 val);
+int of_changeset_update_property_bool(struct of_changeset *ocs,
+	struct device_node *np, const char *name);
+int of_changeset_node_move(struct of_changeset *ocs,
+	struct device_node *np, struct device_node *new_parent);
+
 #else /* CONFIG_OF_DYNAMIC */
 static inline int of_reconfig_notifier_register(struct notifier_block *nb)
 {
@@ -1056,6 +1092,113 @@ static inline int of_reconfig_get_state_change(unsigned long action,
 {
 	return -EINVAL;
 }
+
+static inline int of_changeset_create_device_node(struct of_changeset *ocs,
+	struct device_node *parent, const char *fmt, ...)
+{
+	return -EINVAL;
+}
+
+int of_changeset_add_property_copy(struct of_changeset *ocs,
+	struct device_node *np, const char *name,
+	const void *value, int length)
+{
+	return -EINVAL;
+}
+
+int of_changeset_add_property_string(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char *str)
+{
+	return -EINVAL;
+}
+
+static inline struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline __printf(4, 5) struct device_node *
+	of_changeset_add_property_stringf(
+		struct of_changeset *ocs, struct device_node *np,
+		const char *name, const char *fmt, ...)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline int of_changeset_add_property_string_list(
+	struct of_changeset *ocs, struct device_node *np, const char *name,
+	const char **strs, int count)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_add_property_u32(struct of_changeset *ocs,
+	struct device_node *np, const char *name, u32 val)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_add_property_bool(struct of_changeset *ocs,
+	struct device_node *np, const char *name)
+{
+	return -EINVAL;
+}
+
+int of_changeset_update_property_copy(struct of_changeset *ocs,
+	struct device_node *np, const char *name,
+	const void *value, int length)
+{
+	return -EINVAL;
+}
+
+int of_changeset_update_property_string(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char *str)
+{
+	return -EINVAL;
+}
+
+static inline struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline __printf(4, 5) struct device_node *
+	of_changeset_update_property_stringf(
+		struct of_changeset *ocs, struct device_node *np,
+		const char *name, const char *fmt, ...)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline int of_changeset_update_property_string_list(
+	struct of_changeset *ocs, struct device_node *np, const char *name,
+	const char **strs, int count)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_update_property_u32(struct of_changeset *ocs,
+	struct device_node *np, const char *name, u32 val)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_update_property_bool(struct of_changeset *ocs,
+	struct device_node *np, const char *name)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_node_move(struct of_changeset *ocs,
+		struct device_node *np, struct device_node *new_parent)
+{
+	return -EINVAL;
+}
+
 #endif /* CONFIG_OF_DYNAMIC */
 
 /* CONFIG_OF_RESOLVE api */
-- 
1.7.12

[PATCH 3/4] of: changesets: Introduce changeset helper methods

[Pantelis Antoniou]

Changesets are very powerful, but the lack of a helper API
makes using them cumbersome. Introduce a simple copy based
API that makes things considerably easier.

To wit, adding a property using the raw API.

	struct property *prop;
	prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
	prop->name = kstrdup("compatible");
	prop->value = kstrdup("foo,bar");
	prop->length = strlen(prop->value) + 1;
	of_changeset_add_property(ocs, np, prop);

while using the helper API

	of_changeset_add_property_string(ocs, np, "compatible",
			"foo,bar");

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 drivers/of/dynamic.c | 501 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h   | 143 +++++++++++++++
 2 files changed, 644 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e4dea94..350b1cf 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -828,3 +828,504 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_changeset_action);
+
+/* changeset helpers */
+
+/**
+ * of_changeset_create_device_node - Create an empty device node
+ *
+ * @ocs:	changeset pointer
+ * @parent:	parent device node
+ * @fmt:	format string for the node's full_name
+ * @args:	argument list for the format string
+ *
+ * Create an empty device node, marking it as detached and allocated.
+ *
+ * Returns a device node on success, an error encoded pointer otherwise
+ */
+struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs)
+{
+	struct device_node *node;
+
+	node = __of_node_dupv(NULL, fmt, vargs);
+	if (!node)
+		return ERR_PTR(-ENOMEM);
+
+	node->parent = parent;
+	return node;
+}
+
+/**
+ * of_changeset_create_device_node - Create an empty device node
+ *
+ * @ocs:	changeset pointer
+ * @parent:	parent device node
+ * @fmt:	Format string for the node's full_name
+ * ...		Arguments
+ *
+ * Create an empty device node, marking it as detached and allocated.
+ *
+ * Returns a device node on success, an error encoded pointer otherwise
+ */
+struct device_node *of_changeset_create_device_node(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, ...)
+{
+	va_list vargs;
+	struct device_node *node;
+
+	va_start(vargs, fmt);
+	node = of_changeset_create_device_nodev(ocs, parent, fmt, vargs);
+	va_end(vargs);
+	return node;
+}
+
+static int __of_changeset_add_update_property_copy(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const void *value,
+		int length, bool update)
+{
+	struct property *prop;
+	char *new_name;
+	void *new_value;
+	int ret = -ENOMEM;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop)
+		goto out_no_prop;
+
+	new_name = kstrdup(name, GFP_KERNEL);
+	if (!new_name)
+		goto out_no_name;
+
+	/*
+	 * NOTE: There is no check for zero length value.
+	 * In case of a boolean property, this will allocate a value
+	 * of zero bytes. We do this to work around the use
+	 * of of_get_property() calls on boolean values.
+	 */
+	new_value = kmemdup(value, length, GFP_KERNEL);
+	if (!new_value)
+		goto out_no_value;
+
+	of_property_set_flag(prop, OF_DYNAMIC);
+
+	prop->name = new_name;
+	prop->value = new_value;
+	prop->length = length;
+
+	if (!update)
+		ret = of_changeset_add_property(ocs, np, prop);
+	else
+		ret = of_changeset_update_property(ocs, np, prop);
+
+	if (ret != 0)
+		goto out_no_add;
+
+	return 0;
+
+out_no_add:
+	kfree(prop->value);
+out_no_value:
+	kfree(prop->name);
+out_no_name:
+	kfree(prop);
+out_no_prop:
+	return ret;
+}
+
+static int __of_changeset_add_update_property_string(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *str,
+		bool update)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, str,
+			strlen(str) + 1, update);
+}
+
+static int __of_changeset_add_update_property_stringv(struct of_changeset *ocs,
+		struct device_node *np, const char *name,
+		const char *fmt, va_list vargs, bool update)
+{
+	char *str;
+	int ret;
+
+	str = kvasprintf(GFP_KERNEL, fmt, vargs);
+	if (!str)
+		return -ENOMEM;
+
+	ret = __of_changeset_add_update_property_string(ocs, np, name, str, update);
+
+	kfree(str);
+
+	return ret;
+}
+
+static int __of_changeset_add_update_property_string_list(
+		struct of_changeset *ocs, struct device_node *np, const char *name,
+		const char **strs, int count, bool update)
+{
+	int total = 0, i, ret;
+	char *value, *s;
+
+	for (i = 0; i < count; i++) {
+		/* check if  it's NULL */
+		if (!strs[i])
+			return -EINVAL;
+		total += strlen(strs[i]) + 1;
+	}
+
+	value = kmalloc(total, GFP_KERNEL);
+	if (!value)
+		return -ENOMEM;
+
+	for (i = 0, s = value; i < count; i++) {
+		/* no need to check for NULL, check above */
+		strcpy(s, strs[i]);
+		s += strlen(strs[i]) + 1;
+	}
+
+	ret = __of_changeset_add_update_property_copy(ocs, np, name, value,
+			total, update);
+
+	kfree(value);
+
+	return ret;
+}
+
+static int __of_changeset_add_update_property_u32(struct of_changeset *ocs,
+		struct device_node *np, const char *name, u32 val, bool update)
+{
+	/* in place */
+	val = cpu_to_be32(val);
+	return __of_changeset_add_update_property_copy(ocs, np, name, &val,
+			sizeof(val), update);
+}
+
+static int __of_changeset_add_update_property_bool(struct of_changeset *ocs,
+		struct device_node *np, const char *name, bool update)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, "", 0,
+			update);
+}
+
+/**
+ * of_changeset_add_property_copy - Create a new property copying name & value
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @value:	pointer to the value data
+ * @length:	length of the value in bytes
+ *
+ * Adds a property to the changeset by making copies of the name & value
+ * entries.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_copy(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const void *value,
+		int length)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, value,
+			length, false);
+}
+
+/**
+ * of_changeset_add_property_string - Create a new string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @str:	string property
+ *
+ * Adds a string property to the changeset by making copies of the name
+ * and the string value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_string(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *str)
+{
+	return __of_changeset_add_update_property_string(ocs, np, name, str,
+			false);
+}
+
+/**
+ * of_changeset_add_property_stringf - Create a new formatted string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @fmt:	format of string property
+ * ...		arguments of the format string
+ *
+ * Adds a string property to the changeset by making copies of the name
+ * and the formatted value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_stringf(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *fmt, ...)
+{
+	va_list vargs;
+	int ret;
+
+	va_start(vargs, fmt);
+	ret = __of_changeset_add_update_property_stringv(ocs, np, name, fmt,
+			vargs, false);
+	va_end(vargs);
+	return ret;
+}
+
+/**
+ * of_changeset_add_property_string_list - Create a new string list property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @strs:	pointer to the string list
+ * @count:	string count
+ *
+ * Adds a string list property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_string_list(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char **strs,
+		int count)
+{
+	return __of_changeset_add_update_property_string_list(ocs, np, name,
+			strs, count, false);
+}
+
+/**
+ * of_changeset_add_property_u32 - Create a new u32 property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @val:	value in host endian format
+ *
+ * Adds a u32 property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_u32(struct of_changeset *ocs,
+		struct device_node *np, const char *name, u32 val)
+{
+	return __of_changeset_add_update_property_u32(ocs, np, name,
+			val, false);
+}
+
+/**
+ * of_changeset_add_property_bool - Create a new u32 property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ *
+ * Adds a bool property to the changeset. Note that there is
+ * no option to set the value to false, since the property
+ * existing sets it to true.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_add_property_bool(struct of_changeset *ocs,
+		struct device_node *np, const char *name)
+{
+	return __of_changeset_add_update_property_bool(ocs, np, name, false);
+}
+
+/**
+ * of_changeset_update_property_copy - Update a property copying name & value
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @value:	pointer to the value data
+ * @length:	length of the value in bytes
+ *
+ * Update a property to the changeset by making copies of the name & value
+ * entries.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_copy(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const void *value,
+		int length)
+{
+	return __of_changeset_add_update_property_copy(ocs, np, name, value,
+			length, true);
+}
+
+/**
+ * of_changeset_update_property_string - Create a new string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @str:	string property
+ *
+ * Updates a string property to the changeset by making copies of the name
+ * and the string value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_string(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *str)
+{
+	return __of_changeset_add_update_property_string(ocs, np, name, str,
+			true);
+}
+
+/**
+ * of_changeset_update_property_stringf - Update formatted string property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @fmt:	format of string property
+ * ...		arguments of the format string
+ *
+ * Updates a string property to the changeset by making copies of the name
+ * and the formatted value.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_stringf(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *fmt, ...)
+{
+	va_list vargs;
+	int ret;
+
+	va_start(vargs, fmt);
+	ret = __of_changeset_add_update_property_stringv(ocs, np, name, fmt,
+			vargs, true);
+	va_end(vargs);
+	return ret;
+}
+
+/**
+ * of_changeset_update_property_string_list - Update string list property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @strs:	pointer to the string list
+ * @count:	string count
+ *
+ * Updates a string list property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_string_list(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char **strs,
+		int count)
+{
+	return __of_changeset_add_update_property_string_list(ocs, np, name,
+			strs, count, true);
+}
+
+/**
+ * of_changeset_update_property_u32 - Update u32 property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ * @val:	value in host endian format
+ *
+ * Updates a u32 property to the changeset.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_u32(struct of_changeset *ocs,
+		struct device_node *np, const char *name, u32 val)
+{
+	return __of_changeset_add_update_property_u32(ocs, np, name,
+			val, true);
+}
+
+/**
+ * of_changeset_update_property_bool - Update a bool property
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer
+ * @name:	name of the property
+ *
+ * Updates a property to the changeset. Note that there is
+ * no option to set the value to false, since the property
+ * existing sets it to true.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_update_property_bool(struct of_changeset *ocs,
+		struct device_node *np, const char *name)
+{
+	return __of_changeset_add_update_property_bool(ocs, np, name, true);
+}
+
+static struct device_node *
+__of_changeset_node_move_one(struct of_changeset *ocs,
+		struct device_node *np, struct device_node *new_parent)
+{
+	struct device_node *np2;
+	const char *unitname;
+	int err;
+
+	err = of_changeset_detach_node(ocs, np);
+	if (err)
+		return ERR_PTR(err);
+
+	unitname = strrchr(np->full_name, '/');
+	if (!unitname)
+		unitname = np->full_name;
+
+	np2 = __of_node_dup(np, "%s/%s",
+			new_parent->full_name, unitname);
+	if (!np2)
+		return ERR_PTR(-ENOMEM);
+	np2->parent = new_parent;
+
+	err = of_changeset_attach_node(ocs, np2);
+	if (err)
+		return ERR_PTR(err);
+
+	return np2;
+}
+
+/**
+ * of_changeset_node_move_to - Moves a subtree to a new place in
+ *                             the tree
+ *
+ * @ocs:	changeset pointer
+ * @np:		device node pointer to be moved
+ * @to:		device node of the new parent
+ *
+ * Moves a subtree to a new place in the tree.
+ * Note that a move is a safe operation because the phandles
+ * remain valid.
+ *
+ * Returns zero on success, a negative error value otherwise.
+ */
+int of_changeset_node_move(struct of_changeset *ocs,
+		struct device_node *np, struct device_node *new_parent)
+{
+	struct device_node *npc, *nppc;
+
+	/* move the root first */
+	nppc = __of_changeset_node_move_one(ocs, np, new_parent);
+	if (IS_ERR(nppc))
+		return PTR_ERR(nppc);
+
+	/* move the subtrees next */
+	for_each_child_of_node(np, npc) {
+		nppc = __of_changeset_node_move_one(ocs, npc, nppc);
+		if (IS_ERR(nppc)) {
+			of_node_put(npc);
+			return PTR_ERR(nppc);
+		}
+	}
+
+	return 0;
+}
diff --git a/include/linux/of.h b/include/linux/of.h
index 3175803..f015911 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1037,6 +1037,42 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
 {
 	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
 }
+
+struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs);
+__printf(3, 4) struct device_node *of_changeset_create_device_node(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, ...);
+int of_changeset_add_property_copy(struct of_changeset *ocs,
+	struct device_node *np, const char *name,
+	const void *value, int length);
+int of_changeset_add_property_string(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char *str);
+__printf(4, 5) int of_changeset_add_property_stringf(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *fmt, ...);
+int of_changeset_add_property_string_list(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char **strs, int count);
+int of_changeset_add_property_u32(struct of_changeset *ocs,
+	struct device_node *np, const char *name, u32 val);
+int of_changeset_add_property_bool(struct of_changeset *ocs,
+	struct device_node *np, const char *name);
+int of_changeset_update_property_copy(struct of_changeset *ocs,
+	struct device_node *np, const char *name,
+	const void *value, int length);
+int of_changeset_update_property_string(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char *str);
+__printf(4, 5) int of_changeset_update_property_stringf(struct of_changeset *ocs,
+		struct device_node *np, const char *name, const char *fmt, ...);
+int of_changeset_update_property_string_list(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char **strs, int count);
+int of_changeset_update_property_u32(struct of_changeset *ocs,
+	struct device_node *np, const char *name, u32 val);
+int of_changeset_update_property_bool(struct of_changeset *ocs,
+	struct device_node *np, const char *name);
+int of_changeset_node_move(struct of_changeset *ocs,
+	struct device_node *np, struct device_node *new_parent);
+
 #else /* CONFIG_OF_DYNAMIC */
 static inline int of_reconfig_notifier_register(struct notifier_block *nb)
 {
@@ -1056,6 +1092,113 @@ static inline int of_reconfig_get_state_change(unsigned long action,
 {
 	return -EINVAL;
 }
+
+static inline int of_changeset_create_device_node(struct of_changeset *ocs,
+	struct device_node *parent, const char *fmt, ...)
+{
+	return -EINVAL;
+}
+
+int of_changeset_add_property_copy(struct of_changeset *ocs,
+	struct device_node *np, const char *name,
+	const void *value, int length)
+{
+	return -EINVAL;
+}
+
+int of_changeset_add_property_string(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char *str)
+{
+	return -EINVAL;
+}
+
+static inline struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline __printf(4, 5) struct device_node *
+	of_changeset_add_property_stringf(
+		struct of_changeset *ocs, struct device_node *np,
+		const char *name, const char *fmt, ...)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline int of_changeset_add_property_string_list(
+	struct of_changeset *ocs, struct device_node *np, const char *name,
+	const char **strs, int count)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_add_property_u32(struct of_changeset *ocs,
+	struct device_node *np, const char *name, u32 val)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_add_property_bool(struct of_changeset *ocs,
+	struct device_node *np, const char *name)
+{
+	return -EINVAL;
+}
+
+int of_changeset_update_property_copy(struct of_changeset *ocs,
+	struct device_node *np, const char *name,
+	const void *value, int length)
+{
+	return -EINVAL;
+}
+
+int of_changeset_update_property_string(struct of_changeset *ocs,
+	struct device_node *np, const char *name, const char *str)
+{
+	return -EINVAL;
+}
+
+static inline struct device_node *of_changeset_create_device_nodev(
+	struct of_changeset *ocs, struct device_node *parent,
+	const char *fmt, va_list vargs)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline __printf(4, 5) struct device_node *
+	of_changeset_update_property_stringf(
+		struct of_changeset *ocs, struct device_node *np,
+		const char *name, const char *fmt, ...)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline int of_changeset_update_property_string_list(
+	struct of_changeset *ocs, struct device_node *np, const char *name,
+	const char **strs, int count)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_update_property_u32(struct of_changeset *ocs,
+	struct device_node *np, const char *name, u32 val)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_update_property_bool(struct of_changeset *ocs,
+	struct device_node *np, const char *name)
+{
+	return -EINVAL;
+}
+
+static inline int of_changeset_node_move(struct of_changeset *ocs,
+		struct device_node *np, struct device_node *new_parent)
+{
+	return -EINVAL;
+}
+
 #endif /* CONFIG_OF_DYNAMIC */
 
 /* CONFIG_OF_RESOLVE api */
-- 
1.7.12

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

[PATCH 4/4] of: unittest: changeset helpers

[Pantelis Antoniou]

Add a unitest specific for the new changeset helpers.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/unittest.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e986e6e..ff6939b 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -543,6 +543,59 @@ static void __init of_unittest_changeset(void)
 #endif
 }
 
+static void __init of_unittest_changeset_helper(void)
+{
+#ifdef CONFIG_OF_DYNAMIC
+	struct device_node *n1, *n2, *n21, *parent, *np;
+	struct of_changeset chgset;
+
+	of_changeset_init(&chgset);
+
+	parent = of_find_node_by_path("/testcase-data/changeset");
+
+	unittest(parent, "testcase setup failure\n");
+	n1 = of_changeset_create_device_node(&chgset,
+			parent, "/testcase-data/changeset/n1");
+	unittest(n1, "testcase setup failure\n");
+	n2 = of_changeset_create_device_node(&chgset,
+			parent, "/testcase-data/changeset/n2");
+	unittest(n2, "testcase setup failure\n");
+	n21 = of_changeset_create_device_node(&chgset, n2, "%s/%s",
+			"/testcase-data/changeset/n2", "n21");
+	unittest(n21, "testcase setup failure\n");
+
+	unittest(!of_changeset_add_property_string(&chgset, parent,
+				"prop-add", "foo"), "fail add prop\n");
+
+	unittest(!of_changeset_attach_node(&chgset, n1), "fail n1 attach\n");
+	unittest(!of_changeset_attach_node(&chgset, n2), "fail n2 attach\n");
+	unittest(!of_changeset_attach_node(&chgset, n21), "fail n21 attach\n");
+
+	unittest(!of_changeset_apply(&chgset), "apply failed\n");
+
+	/* Make sure node names are constructed correctly */
+	np = of_find_node_by_path("/testcase-data/changeset/n1");
+	unittest(np, "'%s' not added\n", n1->full_name);
+	of_node_put(np);
+
+	/* Make sure node names are constructed correctly */
+	np = of_find_node_by_path("/testcase-data/changeset/n2");
+	unittest(np, "'%s' not added\n", n2->full_name);
+	of_node_put(np);
+
+	np = of_find_node_by_path("/testcase-data/changeset/n2/n21");
+	unittest(np, "'%s' not added\n", n21->full_name);
+	of_node_put(np);
+
+	unittest(!of_changeset_revert(&chgset), "revert failed\n");
+
+	of_changeset_destroy(&chgset);
+
+	of_node_put(parent);
+#endif
+}
+
+
 static void __init of_unittest_parse_interrupts(void)
 {
 	struct device_node *np;
@@ -1969,6 +2022,7 @@ static int __init of_unittest(void)
 	of_unittest_property_string();
 	of_unittest_property_copy();
 	of_unittest_changeset();
+	of_unittest_changeset_helper();
 	of_unittest_parse_interrupts();
 	of_unittest_parse_interrupts_extended();
 	of_unittest_match_node();
-- 
1.7.12

Re: [PATCH 3/4] of: changesets: Introduce changeset helper methods

[Rob Herring]

On Mon, May 9, 2016 at 8:20 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Changesets are very powerful, but the lack of a helper API
> makes using them cumbersome. Introduce a simple copy based
> API that makes things considerably easier.
>
> To wit, adding a property using the raw API.
>
>         struct property *prop;
>         prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
>         prop->name = kstrdup("compatible");
>         prop->value = kstrdup("foo,bar");
>         prop->length = strlen(prop->value) + 1;
>         of_changeset_add_property(ocs, np, prop);
>
> while using the helper API
>
>         of_changeset_add_property_string(ocs, np, "compatible",
>                         "foo,bar");

Seems useful. Do we not have any users that can be converted?

[...]

> +static int __of_changeset_add_update_property_copy(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, const void *value,
> +               int length, bool update)
> +{
> +       struct property *prop;
> +       char *new_name;
> +       void *new_value;
> +       int ret = -ENOMEM;
> +
> +       prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> +       if (!prop)
> +               goto out_no_prop;

Just return here.

> +
> +       new_name = kstrdup(name, GFP_KERNEL);
> +       if (!new_name)
> +               goto out_no_name;
> +
> +       /*
> +        * NOTE: There is no check for zero length value.
> +        * In case of a boolean property, this will allocate a value
> +        * of zero bytes. We do this to work around the use
> +        * of of_get_property() calls on boolean values.
> +        */
> +       new_value = kmemdup(value, length, GFP_KERNEL);
> +       if (!new_value)
> +               goto out_no_value;
> +
> +       of_property_set_flag(prop, OF_DYNAMIC);
> +
> +       prop->name = new_name;
> +       prop->value = new_value;
> +       prop->length = length;
> +
> +       if (!update)
> +               ret = of_changeset_add_property(ocs, np, prop);
> +       else
> +               ret = of_changeset_update_property(ocs, np, prop);
> +
> +       if (ret != 0)

if (!ret)
  return 0;


> +               goto out_no_add;
> +
> +       return 0;
> +
> +out_no_add:

... and remove all this.

> +       kfree(prop->value);
> +out_no_value:
> +       kfree(prop->name);
> +out_no_name:
> +       kfree(prop);
> +out_no_prop:
> +       return ret;
> +}
> +
> +static int __of_changeset_add_update_property_string(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, const char *str,
> +               bool update)
> +{
> +       return __of_changeset_add_update_property_copy(ocs, np, name, str,
> +                       strlen(str) + 1, update);
> +}
> +
> +static int __of_changeset_add_update_property_stringv(struct of_changeset *ocs,
> +               struct device_node *np, const char *name,
> +               const char *fmt, va_list vargs, bool update)
> +{
> +       char *str;
> +       int ret;
> +
> +       str = kvasprintf(GFP_KERNEL, fmt, vargs);
> +       if (!str)
> +               return -ENOMEM;
> +
> +       ret = __of_changeset_add_update_property_string(ocs, np, name, str, update);
> +
> +       kfree(str);
> +
> +       return ret;
> +}
> +
> +static int __of_changeset_add_update_property_string_list(
> +               struct of_changeset *ocs, struct device_node *np, const char *name,
> +               const char **strs, int count, bool update)
> +{
> +       int total = 0, i, ret;
> +       char *value, *s;
> +
> +       for (i = 0; i < count; i++) {
> +               /* check if  it's NULL */
> +               if (!strs[i])
> +                       return -EINVAL;
> +               total += strlen(strs[i]) + 1;
> +       }
> +
> +       value = kmalloc(total, GFP_KERNEL);
> +       if (!value)
> +               return -ENOMEM;
> +
> +       for (i = 0, s = value; i < count; i++) {
> +               /* no need to check for NULL, check above */
> +               strcpy(s, strs[i]);
> +               s += strlen(strs[i]) + 1;
> +       }
> +
> +       ret = __of_changeset_add_update_property_copy(ocs, np, name, value,
> +                       total, update);
> +
> +       kfree(value);
> +
> +       return ret;
> +}
> +
> +static int __of_changeset_add_update_property_u32(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, u32 val, bool update)
> +{
> +       /* in place */
> +       val = cpu_to_be32(val);

Kill this function and move this to the 2 callers.

> +       return __of_changeset_add_update_property_copy(ocs, np, name, &val,
> +                       sizeof(val), update);
> +}
> +
> +static int __of_changeset_add_update_property_bool(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, bool update)
> +{

I think this intermediate function could be killed off too.

> +       return __of_changeset_add_update_property_copy(ocs, np, name, "", 0,
> +                       update);
> +}

[...]

> +static struct device_node *
> +__of_changeset_node_move_one(struct of_changeset *ocs,
> +               struct device_node *np, struct device_node *new_parent)
> +{
> +       struct device_node *np2;
> +       const char *unitname;
> +       int err;
> +
> +       err = of_changeset_detach_node(ocs, np);
> +       if (err)
> +               return ERR_PTR(err);
> +
> +       unitname = strrchr(np->full_name, '/');
> +       if (!unitname)
> +               unitname = np->full_name;
> +
> +       np2 = __of_node_dup(np, "%s/%s",
> +                       new_parent->full_name, unitname);
> +       if (!np2)
> +               return ERR_PTR(-ENOMEM);
> +       np2->parent = new_parent;
> +
> +       err = of_changeset_attach_node(ocs, np2);
> +       if (err)
> +               return ERR_PTR(err);
> +
> +       return np2;
> +}
> +
> +/**
> + * of_changeset_node_move_to - Moves a subtree to a new place in
> + *                             the tree
> + *
> + * @ocs:       changeset pointer
> + * @np:                device node pointer to be moved
> + * @to:                device node of the new parent
> + *
> + * Moves a subtree to a new place in the tree.
> + * Note that a move is a safe operation because the phandles
> + * remain valid.

I'm having a hard time imagining a usecase for this one. I would drop
until we have a user in tree.

> + *
> + * Returns zero on success, a negative error value otherwise.
> + */
> +int of_changeset_node_move(struct of_changeset *ocs,
> +               struct device_node *np, struct device_node *new_parent)
> +{
> +       struct device_node *npc, *nppc;
> +
> +       /* move the root first */
> +       nppc = __of_changeset_node_move_one(ocs, np, new_parent);
> +       if (IS_ERR(nppc))
> +               return PTR_ERR(nppc);
> +
> +       /* move the subtrees next */
> +       for_each_child_of_node(np, npc) {
> +               nppc = __of_changeset_node_move_one(ocs, npc, nppc);
> +               if (IS_ERR(nppc)) {
> +                       of_node_put(npc);
> +                       return PTR_ERR(nppc);
> +               }
> +       }
> +
> +       return 0;
> +}
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 3175803..f015911 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1037,6 +1037,42 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>  {
>         return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>  }
> +
> +struct device_node *of_changeset_create_device_nodev(
> +       struct of_changeset *ocs, struct device_node *parent,
> +       const char *fmt, va_list vargs);
> +__printf(3, 4) struct device_node *of_changeset_create_device_node(
> +       struct of_changeset *ocs, struct device_node *parent,
> +       const char *fmt, ...);
> +int of_changeset_add_property_copy(struct of_changeset *ocs,
> +       struct device_node *np, const char *name,
> +       const void *value, int length);
> +int of_changeset_add_property_string(struct of_changeset *ocs,
> +       struct device_node *np, const char *name, const char *str);
> +__printf(4, 5) int of_changeset_add_property_stringf(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, const char *fmt, ...);
> +int of_changeset_add_property_string_list(struct of_changeset *ocs,
> +       struct device_node *np, const char *name, const char **strs, int count);
> +int of_changeset_add_property_u32(struct of_changeset *ocs,
> +       struct device_node *np, const char *name, u32 val);
> +int of_changeset_add_property_bool(struct of_changeset *ocs,
> +       struct device_node *np, const char *name);
> +int of_changeset_update_property_copy(struct of_changeset *ocs,
> +       struct device_node *np, const char *name,
> +       const void *value, int length);
> +int of_changeset_update_property_string(struct of_changeset *ocs,
> +       struct device_node *np, const char *name, const char *str);
> +__printf(4, 5) int of_changeset_update_property_stringf(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, const char *fmt, ...);
> +int of_changeset_update_property_string_list(struct of_changeset *ocs,
> +       struct device_node *np, const char *name, const char **strs, int count);
> +int of_changeset_update_property_u32(struct of_changeset *ocs,
> +       struct device_node *np, const char *name, u32 val);
> +int of_changeset_update_property_bool(struct of_changeset *ocs,
> +       struct device_node *np, const char *name);
> +int of_changeset_node_move(struct of_changeset *ocs,
> +       struct device_node *np, struct device_node *new_parent);

A bunch of these can be static inline wrappers.

You need EXPORT_SYMBOL_GPL on these as well.

Rob

Re: [PATCH 3/4] of: changesets: Introduce changeset helper methods

[Rob Herring]

On Mon, May 9, 2016 at 8:20 AM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> Changesets are very powerful, but the lack of a helper API
> makes using them cumbersome. Introduce a simple copy based
> API that makes things considerably easier.
>
> To wit, adding a property using the raw API.
>
>         struct property *prop;
>         prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
>         prop->name = kstrdup("compatible");
>         prop->value = kstrdup("foo,bar");
>         prop->length = strlen(prop->value) + 1;
>         of_changeset_add_property(ocs, np, prop);
>
> while using the helper API
>
>         of_changeset_add_property_string(ocs, np, "compatible",
>                         "foo,bar");

Seems useful. Do we not have any users that can be converted?

[...]

> +static int __of_changeset_add_update_property_copy(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, const void *value,
> +               int length, bool update)
> +{
> +       struct property *prop;
> +       char *new_name;
> +       void *new_value;
> +       int ret = -ENOMEM;
> +
> +       prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> +       if (!prop)
> +               goto out_no_prop;

Just return here.

> +
> +       new_name = kstrdup(name, GFP_KERNEL);
> +       if (!new_name)
> +               goto out_no_name;
> +
> +       /*
> +        * NOTE: There is no check for zero length value.
> +        * In case of a boolean property, this will allocate a value
> +        * of zero bytes. We do this to work around the use
> +        * of of_get_property() calls on boolean values.
> +        */
> +       new_value = kmemdup(value, length, GFP_KERNEL);
> +       if (!new_value)
> +               goto out_no_value;
> +
> +       of_property_set_flag(prop, OF_DYNAMIC);
> +
> +       prop->name = new_name;
> +       prop->value = new_value;
> +       prop->length = length;
> +
> +       if (!update)
> +               ret = of_changeset_add_property(ocs, np, prop);
> +       else
> +               ret = of_changeset_update_property(ocs, np, prop);
> +
> +       if (ret != 0)

if (!ret)
  return 0;


> +               goto out_no_add;
> +
> +       return 0;
> +
> +out_no_add:

... and remove all this.

> +       kfree(prop->value);
> +out_no_value:
> +       kfree(prop->name);
> +out_no_name:
> +       kfree(prop);
> +out_no_prop:
> +       return ret;
> +}
> +
> +static int __of_changeset_add_update_property_string(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, const char *str,
> +               bool update)
> +{
> +       return __of_changeset_add_update_property_copy(ocs, np, name, str,
> +                       strlen(str) + 1, update);
> +}
> +
> +static int __of_changeset_add_update_property_stringv(struct of_changeset *ocs,
> +               struct device_node *np, const char *name,
> +               const char *fmt, va_list vargs, bool update)
> +{
> +       char *str;
> +       int ret;
> +
> +       str = kvasprintf(GFP_KERNEL, fmt, vargs);
> +       if (!str)
> +               return -ENOMEM;
> +
> +       ret = __of_changeset_add_update_property_string(ocs, np, name, str, update);
> +
> +       kfree(str);
> +
> +       return ret;
> +}
> +
> +static int __of_changeset_add_update_property_string_list(
> +               struct of_changeset *ocs, struct device_node *np, const char *name,
> +               const char **strs, int count, bool update)
> +{
> +       int total = 0, i, ret;
> +       char *value, *s;
> +
> +       for (i = 0; i < count; i++) {
> +               /* check if  it's NULL */
> +               if (!strs[i])
> +                       return -EINVAL;
> +               total += strlen(strs[i]) + 1;
> +       }
> +
> +       value = kmalloc(total, GFP_KERNEL);
> +       if (!value)
> +               return -ENOMEM;
> +
> +       for (i = 0, s = value; i < count; i++) {
> +               /* no need to check for NULL, check above */
> +               strcpy(s, strs[i]);
> +               s += strlen(strs[i]) + 1;
> +       }
> +
> +       ret = __of_changeset_add_update_property_copy(ocs, np, name, value,
> +                       total, update);
> +
> +       kfree(value);
> +
> +       return ret;
> +}
> +
> +static int __of_changeset_add_update_property_u32(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, u32 val, bool update)
> +{
> +       /* in place */
> +       val = cpu_to_be32(val);

Kill this function and move this to the 2 callers.

> +       return __of_changeset_add_update_property_copy(ocs, np, name, &val,
> +                       sizeof(val), update);
> +}
> +
> +static int __of_changeset_add_update_property_bool(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, bool update)
> +{

I think this intermediate function could be killed off too.

> +       return __of_changeset_add_update_property_copy(ocs, np, name, "", 0,
> +                       update);
> +}

[...]

> +static struct device_node *
> +__of_changeset_node_move_one(struct of_changeset *ocs,
> +               struct device_node *np, struct device_node *new_parent)
> +{
> +       struct device_node *np2;
> +       const char *unitname;
> +       int err;
> +
> +       err = of_changeset_detach_node(ocs, np);
> +       if (err)
> +               return ERR_PTR(err);
> +
> +       unitname = strrchr(np->full_name, '/');
> +       if (!unitname)
> +               unitname = np->full_name;
> +
> +       np2 = __of_node_dup(np, "%s/%s",
> +                       new_parent->full_name, unitname);
> +       if (!np2)
> +               return ERR_PTR(-ENOMEM);
> +       np2->parent = new_parent;
> +
> +       err = of_changeset_attach_node(ocs, np2);
> +       if (err)
> +               return ERR_PTR(err);
> +
> +       return np2;
> +}
> +
> +/**
> + * of_changeset_node_move_to - Moves a subtree to a new place in
> + *                             the tree
> + *
> + * @ocs:       changeset pointer
> + * @np:                device node pointer to be moved
> + * @to:                device node of the new parent
> + *
> + * Moves a subtree to a new place in the tree.
> + * Note that a move is a safe operation because the phandles
> + * remain valid.

I'm having a hard time imagining a usecase for this one. I would drop
until we have a user in tree.

> + *
> + * Returns zero on success, a negative error value otherwise.
> + */
> +int of_changeset_node_move(struct of_changeset *ocs,
> +               struct device_node *np, struct device_node *new_parent)
> +{
> +       struct device_node *npc, *nppc;
> +
> +       /* move the root first */
> +       nppc = __of_changeset_node_move_one(ocs, np, new_parent);
> +       if (IS_ERR(nppc))
> +               return PTR_ERR(nppc);
> +
> +       /* move the subtrees next */
> +       for_each_child_of_node(np, npc) {
> +               nppc = __of_changeset_node_move_one(ocs, npc, nppc);
> +               if (IS_ERR(nppc)) {
> +                       of_node_put(npc);
> +                       return PTR_ERR(nppc);
> +               }
> +       }
> +
> +       return 0;
> +}
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 3175803..f015911 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1037,6 +1037,42 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>  {
>         return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>  }
> +
> +struct device_node *of_changeset_create_device_nodev(
> +       struct of_changeset *ocs, struct device_node *parent,
> +       const char *fmt, va_list vargs);
> +__printf(3, 4) struct device_node *of_changeset_create_device_node(
> +       struct of_changeset *ocs, struct device_node *parent,
> +       const char *fmt, ...);
> +int of_changeset_add_property_copy(struct of_changeset *ocs,
> +       struct device_node *np, const char *name,
> +       const void *value, int length);
> +int of_changeset_add_property_string(struct of_changeset *ocs,
> +       struct device_node *np, const char *name, const char *str);
> +__printf(4, 5) int of_changeset_add_property_stringf(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, const char *fmt, ...);
> +int of_changeset_add_property_string_list(struct of_changeset *ocs,
> +       struct device_node *np, const char *name, const char **strs, int count);
> +int of_changeset_add_property_u32(struct of_changeset *ocs,
> +       struct device_node *np, const char *name, u32 val);
> +int of_changeset_add_property_bool(struct of_changeset *ocs,
> +       struct device_node *np, const char *name);
> +int of_changeset_update_property_copy(struct of_changeset *ocs,
> +       struct device_node *np, const char *name,
> +       const void *value, int length);
> +int of_changeset_update_property_string(struct of_changeset *ocs,
> +       struct device_node *np, const char *name, const char *str);
> +__printf(4, 5) int of_changeset_update_property_stringf(struct of_changeset *ocs,
> +               struct device_node *np, const char *name, const char *fmt, ...);
> +int of_changeset_update_property_string_list(struct of_changeset *ocs,
> +       struct device_node *np, const char *name, const char **strs, int count);
> +int of_changeset_update_property_u32(struct of_changeset *ocs,
> +       struct device_node *np, const char *name, u32 val);
> +int of_changeset_update_property_bool(struct of_changeset *ocs,
> +       struct device_node *np, const char *name);
> +int of_changeset_node_move(struct of_changeset *ocs,
> +       struct device_node *np, struct device_node *new_parent);

A bunch of these can be static inline wrappers.

You need EXPORT_SYMBOL_GPL on these as well.

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

Re: [PATCH 3/4] of: changesets: Introduce changeset helper methods

[Pantelis Antoniou]

Hi Rob,

> On May 9, 2016, at 17:27 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Mon, May 9, 2016 at 8:20 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Changesets are very powerful, but the lack of a helper API
>> makes using them cumbersome. Introduce a simple copy based
>> API that makes things considerably easier.
>> 
>> To wit, adding a property using the raw API.
>> 
>>        struct property *prop;
>>        prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
>>        prop->name = kstrdup("compatible");
>>        prop->value = kstrdup("foo,bar");
>>        prop->length = strlen(prop->value) + 1;
>>        of_changeset_add_property(ocs, np, prop);
>> 
>> while using the helper API
>> 
>>        of_changeset_add_property_string(ocs, np, "compatible",
>>                        "foo,bar");
> 
> Seems useful. Do we not have any users that can be converted?
> 

There is a user for this, which will be shortly posted.


> [...]
> 
>> +static int __of_changeset_add_update_property_copy(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name, const void *value,
>> +               int length, bool update)
>> +{
>> +       struct property *prop;
>> +       char *new_name;
>> +       void *new_value;
>> +       int ret = -ENOMEM;
>> +
>> +       prop = kzalloc(sizeof(*prop), GFP_KERNEL);
>> +       if (!prop)
>> +               goto out_no_prop;
> 
> Just return here.
> 

OK

>> +
>> +       new_name = kstrdup(name, GFP_KERNEL);
>> +       if (!new_name)
>> +               goto out_no_name;
>> +
>> +       /*
>> +        * NOTE: There is no check for zero length value.
>> +        * In case of a boolean property, this will allocate a value
>> +        * of zero bytes. We do this to work around the use
>> +        * of of_get_property() calls on boolean values.
>> +        */
>> +       new_value = kmemdup(value, length, GFP_KERNEL);
>> +       if (!new_value)
>> +               goto out_no_value;
>> +
>> +       of_property_set_flag(prop, OF_DYNAMIC);
>> +
>> +       prop->name = new_name;
>> +       prop->value = new_value;
>> +       prop->length = length;
>> +
>> +       if (!update)
>> +               ret = of_changeset_add_property(ocs, np, prop);
>> +       else
>> +               ret = of_changeset_update_property(ocs, np, prop);
>> +
>> +       if (ret != 0)
> 
> if (!ret)
>  return 0;
> 
> 

>> +               goto out_no_add;
>> +
>> +       return 0;
>> +
>> +out_no_add:
> 
> ... and remove all this.
> 

Err, there’s an exit path here from kmemdup (goto err_no_value).
We’ll be leaking memory on error.

>> +       kfree(prop->value);
>> +out_no_value:
>> +       kfree(prop->name);
>> +out_no_name:
>> +       kfree(prop);
>> +out_no_prop:
>> +       return ret;
>> +}
>> +
>> +static int __of_changeset_add_update_property_string(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name, const char *str,
>> +               bool update)
>> +{
>> +       return __of_changeset_add_update_property_copy(ocs, np, name, str,
>> +                       strlen(str) + 1, update);
>> +}
>> +
>> +static int __of_changeset_add_update_property_stringv(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name,
>> +               const char *fmt, va_list vargs, bool update)
>> +{
>> +       char *str;
>> +       int ret;
>> +
>> +       str = kvasprintf(GFP_KERNEL, fmt, vargs);
>> +       if (!str)
>> +               return -ENOMEM;
>> +
>> +       ret = __of_changeset_add_update_property_string(ocs, np, name, str, update);
>> +
>> +       kfree(str);
>> +
>> +       return ret;
>> +}
>> +
>> +static int __of_changeset_add_update_property_string_list(
>> +               struct of_changeset *ocs, struct device_node *np, const char *name,
>> +               const char **strs, int count, bool update)
>> +{
>> +       int total = 0, i, ret;
>> +       char *value, *s;
>> +
>> +       for (i = 0; i < count; i++) {
>> +               /* check if  it's NULL */
>> +               if (!strs[i])
>> +                       return -EINVAL;
>> +               total += strlen(strs[i]) + 1;
>> +       }
>> +
>> +       value = kmalloc(total, GFP_KERNEL);
>> +       if (!value)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0, s = value; i < count; i++) {
>> +               /* no need to check for NULL, check above */
>> +               strcpy(s, strs[i]);
>> +               s += strlen(strs[i]) + 1;
>> +       }
>> +
>> +       ret = __of_changeset_add_update_property_copy(ocs, np, name, value,
>> +                       total, update);
>> +
>> +       kfree(value);
>> +
>> +       return ret;
>> +}
>> +
>> +static int __of_changeset_add_update_property_u32(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name, u32 val, bool update)
>> +{
>> +       /* in place */
>> +       val = cpu_to_be32(val);
> 
> Kill this function and move this to the 2 callers.
> 
>> +       return __of_changeset_add_update_property_copy(ocs, np, name, &val,
>> +                       sizeof(val), update);
>> +}
>> +
>> +static int __of_changeset_add_update_property_bool(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name, bool update)
>> +{
> 
> I think this intermediate function could be killed off too.
> 
>> +       return __of_changeset_add_update_property_copy(ocs, np, name, "", 0,
>> +                       update);
>> +}
> 
> [...]
> 
>> +static struct device_node *
>> +__of_changeset_node_move_one(struct of_changeset *ocs,
>> +               struct device_node *np, struct device_node *new_parent)
>> +{
>> +       struct device_node *np2;
>> +       const char *unitname;
>> +       int err;
>> +
>> +       err = of_changeset_detach_node(ocs, np);
>> +       if (err)
>> +               return ERR_PTR(err);
>> +
>> +       unitname = strrchr(np->full_name, '/');
>> +       if (!unitname)
>> +               unitname = np->full_name;
>> +
>> +       np2 = __of_node_dup(np, "%s/%s",
>> +                       new_parent->full_name, unitname);
>> +       if (!np2)
>> +               return ERR_PTR(-ENOMEM);
>> +       np2->parent = new_parent;
>> +
>> +       err = of_changeset_attach_node(ocs, np2);
>> +       if (err)
>> +               return ERR_PTR(err);
>> +
>> +       return np2;
>> +}
>> +
>> +/**
>> + * of_changeset_node_move_to - Moves a subtree to a new place in
>> + *                             the tree
>> + *
>> + * @ocs:       changeset pointer
>> + * @np:                device node pointer to be moved
>> + * @to:                device node of the new parent
>> + *
>> + * Moves a subtree to a new place in the tree.
>> + * Note that a move is a safe operation because the phandles
>> + * remain valid.
> 
> I'm having a hard time imagining a usecase for this one. I would drop
> until we have a user in tree.
> 

There’s a use case - it will be apparent with the next patches.
 
>> + *
>> + * Returns zero on success, a negative error value otherwise.
>> + */
>> +int of_changeset_node_move(struct of_changeset *ocs,
>> +               struct device_node *np, struct device_node *new_parent)
>> +{
>> +       struct device_node *npc, *nppc;
>> +
>> +       /* move the root first */
>> +       nppc = __of_changeset_node_move_one(ocs, np, new_parent);
>> +       if (IS_ERR(nppc))
>> +               return PTR_ERR(nppc);
>> +
>> +       /* move the subtrees next */
>> +       for_each_child_of_node(np, npc) {
>> +               nppc = __of_changeset_node_move_one(ocs, npc, nppc);
>> +               if (IS_ERR(nppc)) {
>> +                       of_node_put(npc);
>> +                       return PTR_ERR(nppc);
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index 3175803..f015911 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -1037,6 +1037,42 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>> {
>>        return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>> }
>> +
>> +struct device_node *of_changeset_create_device_nodev(
>> +       struct of_changeset *ocs, struct device_node *parent,
>> +       const char *fmt, va_list vargs);
>> +__printf(3, 4) struct device_node *of_changeset_create_device_node(
>> +       struct of_changeset *ocs, struct device_node *parent,
>> +       const char *fmt, ...);
>> +int of_changeset_add_property_copy(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name,
>> +       const void *value, int length);
>> +int of_changeset_add_property_string(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name, const char *str);
>> +__printf(4, 5) int of_changeset_add_property_stringf(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name, const char *fmt, ...);
>> +int of_changeset_add_property_string_list(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name, const char **strs, int count);
>> +int of_changeset_add_property_u32(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name, u32 val);
>> +int of_changeset_add_property_bool(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name);
>> +int of_changeset_update_property_copy(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name,
>> +       const void *value, int length);
>> +int of_changeset_update_property_string(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name, const char *str);
>> +__printf(4, 5) int of_changeset_update_property_stringf(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name, const char *fmt, ...);
>> +int of_changeset_update_property_string_list(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name, const char **strs, int count);
>> +int of_changeset_update_property_u32(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name, u32 val);
>> +int of_changeset_update_property_bool(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name);
>> +int of_changeset_node_move(struct of_changeset *ocs,
>> +       struct device_node *np, struct device_node *new_parent);
> 
> A bunch of these can be static inline wrappers.
> 
> You need EXPORT_SYMBOL_GPL on these as well.
> 

I will add the exports.

> Rob

Regards

— Pantelis

Re: [PATCH 3/4] of: changesets: Introduce changeset helper methods

[Pantelis Antoniou]

Hi Rob,

> On May 9, 2016, at 17:27 , Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> On Mon, May 9, 2016 at 8:20 AM, Pantelis Antoniou
> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>> Changesets are very powerful, but the lack of a helper API
>> makes using them cumbersome. Introduce a simple copy based
>> API that makes things considerably easier.
>> 
>> To wit, adding a property using the raw API.
>> 
>>        struct property *prop;
>>        prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
>>        prop->name = kstrdup("compatible");
>>        prop->value = kstrdup("foo,bar");
>>        prop->length = strlen(prop->value) + 1;
>>        of_changeset_add_property(ocs, np, prop);
>> 
>> while using the helper API
>> 
>>        of_changeset_add_property_string(ocs, np, "compatible",
>>                        "foo,bar");
> 
> Seems useful. Do we not have any users that can be converted?
> 

There is a user for this, which will be shortly posted.


> [...]
> 
>> +static int __of_changeset_add_update_property_copy(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name, const void *value,
>> +               int length, bool update)
>> +{
>> +       struct property *prop;
>> +       char *new_name;
>> +       void *new_value;
>> +       int ret = -ENOMEM;
>> +
>> +       prop = kzalloc(sizeof(*prop), GFP_KERNEL);
>> +       if (!prop)
>> +               goto out_no_prop;
> 
> Just return here.
> 

OK

>> +
>> +       new_name = kstrdup(name, GFP_KERNEL);
>> +       if (!new_name)
>> +               goto out_no_name;
>> +
>> +       /*
>> +        * NOTE: There is no check for zero length value.
>> +        * In case of a boolean property, this will allocate a value
>> +        * of zero bytes. We do this to work around the use
>> +        * of of_get_property() calls on boolean values.
>> +        */
>> +       new_value = kmemdup(value, length, GFP_KERNEL);
>> +       if (!new_value)
>> +               goto out_no_value;
>> +
>> +       of_property_set_flag(prop, OF_DYNAMIC);
>> +
>> +       prop->name = new_name;
>> +       prop->value = new_value;
>> +       prop->length = length;
>> +
>> +       if (!update)
>> +               ret = of_changeset_add_property(ocs, np, prop);
>> +       else
>> +               ret = of_changeset_update_property(ocs, np, prop);
>> +
>> +       if (ret != 0)
> 
> if (!ret)
>  return 0;
> 
> 

>> +               goto out_no_add;
>> +
>> +       return 0;
>> +
>> +out_no_add:
> 
> ... and remove all this.
> 

Err, there’s an exit path here from kmemdup (goto err_no_value).
We’ll be leaking memory on error.

>> +       kfree(prop->value);
>> +out_no_value:
>> +       kfree(prop->name);
>> +out_no_name:
>> +       kfree(prop);
>> +out_no_prop:
>> +       return ret;
>> +}
>> +
>> +static int __of_changeset_add_update_property_string(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name, const char *str,
>> +               bool update)
>> +{
>> +       return __of_changeset_add_update_property_copy(ocs, np, name, str,
>> +                       strlen(str) + 1, update);
>> +}
>> +
>> +static int __of_changeset_add_update_property_stringv(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name,
>> +               const char *fmt, va_list vargs, bool update)
>> +{
>> +       char *str;
>> +       int ret;
>> +
>> +       str = kvasprintf(GFP_KERNEL, fmt, vargs);
>> +       if (!str)
>> +               return -ENOMEM;
>> +
>> +       ret = __of_changeset_add_update_property_string(ocs, np, name, str, update);
>> +
>> +       kfree(str);
>> +
>> +       return ret;
>> +}
>> +
>> +static int __of_changeset_add_update_property_string_list(
>> +               struct of_changeset *ocs, struct device_node *np, const char *name,
>> +               const char **strs, int count, bool update)
>> +{
>> +       int total = 0, i, ret;
>> +       char *value, *s;
>> +
>> +       for (i = 0; i < count; i++) {
>> +               /* check if  it's NULL */
>> +               if (!strs[i])
>> +                       return -EINVAL;
>> +               total += strlen(strs[i]) + 1;
>> +       }
>> +
>> +       value = kmalloc(total, GFP_KERNEL);
>> +       if (!value)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0, s = value; i < count; i++) {
>> +               /* no need to check for NULL, check above */
>> +               strcpy(s, strs[i]);
>> +               s += strlen(strs[i]) + 1;
>> +       }
>> +
>> +       ret = __of_changeset_add_update_property_copy(ocs, np, name, value,
>> +                       total, update);
>> +
>> +       kfree(value);
>> +
>> +       return ret;
>> +}
>> +
>> +static int __of_changeset_add_update_property_u32(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name, u32 val, bool update)
>> +{
>> +       /* in place */
>> +       val = cpu_to_be32(val);
> 
> Kill this function and move this to the 2 callers.
> 
>> +       return __of_changeset_add_update_property_copy(ocs, np, name, &val,
>> +                       sizeof(val), update);
>> +}
>> +
>> +static int __of_changeset_add_update_property_bool(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name, bool update)
>> +{
> 
> I think this intermediate function could be killed off too.
> 
>> +       return __of_changeset_add_update_property_copy(ocs, np, name, "", 0,
>> +                       update);
>> +}
> 
> [...]
> 
>> +static struct device_node *
>> +__of_changeset_node_move_one(struct of_changeset *ocs,
>> +               struct device_node *np, struct device_node *new_parent)
>> +{
>> +       struct device_node *np2;
>> +       const char *unitname;
>> +       int err;
>> +
>> +       err = of_changeset_detach_node(ocs, np);
>> +       if (err)
>> +               return ERR_PTR(err);
>> +
>> +       unitname = strrchr(np->full_name, '/');
>> +       if (!unitname)
>> +               unitname = np->full_name;
>> +
>> +       np2 = __of_node_dup(np, "%s/%s",
>> +                       new_parent->full_name, unitname);
>> +       if (!np2)
>> +               return ERR_PTR(-ENOMEM);
>> +       np2->parent = new_parent;
>> +
>> +       err = of_changeset_attach_node(ocs, np2);
>> +       if (err)
>> +               return ERR_PTR(err);
>> +
>> +       return np2;
>> +}
>> +
>> +/**
>> + * of_changeset_node_move_to - Moves a subtree to a new place in
>> + *                             the tree
>> + *
>> + * @ocs:       changeset pointer
>> + * @np:                device node pointer to be moved
>> + * @to:                device node of the new parent
>> + *
>> + * Moves a subtree to a new place in the tree.
>> + * Note that a move is a safe operation because the phandles
>> + * remain valid.
> 
> I'm having a hard time imagining a usecase for this one. I would drop
> until we have a user in tree.
> 

There’s a use case - it will be apparent with the next patches.
 
>> + *
>> + * Returns zero on success, a negative error value otherwise.
>> + */
>> +int of_changeset_node_move(struct of_changeset *ocs,
>> +               struct device_node *np, struct device_node *new_parent)
>> +{
>> +       struct device_node *npc, *nppc;
>> +
>> +       /* move the root first */
>> +       nppc = __of_changeset_node_move_one(ocs, np, new_parent);
>> +       if (IS_ERR(nppc))
>> +               return PTR_ERR(nppc);
>> +
>> +       /* move the subtrees next */
>> +       for_each_child_of_node(np, npc) {
>> +               nppc = __of_changeset_node_move_one(ocs, npc, nppc);
>> +               if (IS_ERR(nppc)) {
>> +                       of_node_put(npc);
>> +                       return PTR_ERR(nppc);
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index 3175803..f015911 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -1037,6 +1037,42 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>> {
>>        return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>> }
>> +
>> +struct device_node *of_changeset_create_device_nodev(
>> +       struct of_changeset *ocs, struct device_node *parent,
>> +       const char *fmt, va_list vargs);
>> +__printf(3, 4) struct device_node *of_changeset_create_device_node(
>> +       struct of_changeset *ocs, struct device_node *parent,
>> +       const char *fmt, ...);
>> +int of_changeset_add_property_copy(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name,
>> +       const void *value, int length);
>> +int of_changeset_add_property_string(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name, const char *str);
>> +__printf(4, 5) int of_changeset_add_property_stringf(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name, const char *fmt, ...);
>> +int of_changeset_add_property_string_list(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name, const char **strs, int count);
>> +int of_changeset_add_property_u32(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name, u32 val);
>> +int of_changeset_add_property_bool(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name);
>> +int of_changeset_update_property_copy(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name,
>> +       const void *value, int length);
>> +int of_changeset_update_property_string(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name, const char *str);
>> +__printf(4, 5) int of_changeset_update_property_stringf(struct of_changeset *ocs,
>> +               struct device_node *np, const char *name, const char *fmt, ...);
>> +int of_changeset_update_property_string_list(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name, const char **strs, int count);
>> +int of_changeset_update_property_u32(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name, u32 val);
>> +int of_changeset_update_property_bool(struct of_changeset *ocs,
>> +       struct device_node *np, const char *name);
>> +int of_changeset_node_move(struct of_changeset *ocs,
>> +       struct device_node *np, struct device_node *new_parent);
> 
> A bunch of these can be static inline wrappers.
> 
> You need EXPORT_SYMBOL_GPL on these as well.
> 

I will add the exports.

> Rob

Regards

— Pantelis

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

Re: [PATCH 3/4] of: changesets: Introduce changeset helper methods

[Rob Herring]

On Mon, May 9, 2016 at 9:34 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On May 9, 2016, at 17:27 , Rob Herring <robherring2@gmail.com> wrote:
>>
>> On Mon, May 9, 2016 at 8:20 AM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> Changesets are very powerful, but the lack of a helper API
>>> makes using them cumbersome. Introduce a simple copy based
>>> API that makes things considerably easier.

[...]

>>> +       /*
>>> +        * NOTE: There is no check for zero length value.
>>> +        * In case of a boolean property, this will allocate a value
>>> +        * of zero bytes. We do this to work around the use
>>> +        * of of_get_property() calls on boolean values.
>>> +        */
>>> +       new_value = kmemdup(value, length, GFP_KERNEL);
>>> +       if (!new_value)
>>> +               goto out_no_value;
>>> +
>>> +       of_property_set_flag(prop, OF_DYNAMIC);
>>> +
>>> +       prop->name = new_name;
>>> +       prop->value = new_value;
>>> +       prop->length = length;
>>> +
>>> +       if (!update)
>>> +               ret = of_changeset_add_property(ocs, np, prop);
>>> +       else
>>> +               ret = of_changeset_update_property(ocs, np, prop);
>>> +
>>> +       if (ret != 0)
>>
>> if (!ret)
>>  return 0;
>>
>>
>
>>> +               goto out_no_add;
>>> +
>>> +       return 0;
>>> +
>>> +out_no_add:
>>
>> ... and remove all this.
>>
>
> Err, there’s an exit path here from kmemdup (goto err_no_value).
> We’ll be leaking memory on error.

No you won't. "This" is the hunk above it. The error handling would
still be here:

>>> +       kfree(prop->value);
>>> +out_no_value:
>>> +       kfree(prop->name);
>>> +out_no_name:
>>> +       kfree(prop);
>>> +out_no_prop:
>>> +       return ret;
>>> +}

Re: [PATCH 1/4] of: dynamic: changeset prop-update revert fix

[Rob Herring]

On Mon, May 09, 2016 at 04:20:42PM +0300, Pantelis Antoniou wrote:
> When reverting an update property changeset entry that created a
> property the reverse operation is a remove property and not an update.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  drivers/of/dynamic.c | 5 +++++
>  1 file changed, 5 insertions(+)

I've applied this one.

Rob

Re: [PATCH 1/4] of: dynamic: changeset prop-update revert fix

[Rob Herring]

On Mon, May 09, 2016 at 04:20:42PM +0300, Pantelis Antoniou wrote:
> When reverting an update property changeset entry that created a
> property the reverse operation is a remove property and not an update.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/of/dynamic.c | 5 +++++
>  1 file changed, 5 insertions(+)

I've applied this one.

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