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

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

[Hans de Goede]

From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

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-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
Changes in v2 (hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
-No changes (unmodified preparation patch)
---
 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 888fdbc..cc64675 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -397,8 +397,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
@@ -406,17 +407,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;
@@ -448,6 +447,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);
-- 
2.9.3

--
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 v2 2/2] of: changesets: Introduce changeset helper methods

[Hans de Goede]

From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

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>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
Changes in v2 (hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
-Address review comments from:
 https://www.spinics.net/lists/kernel/msg2252845.html
 -Simplify (and fix) __of_changeset_add_update_property_copy OOM handling
 -Remove (by manual inlining) these 2 static helpers:
  __of_changeset_add_update_property_u32
  __of_changeset_add_update_property_bool
 -Remove the following exported helper method:
  of_changeset_node_move_to
---
 drivers/of/dynamic.c | 428 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h   | 135 ++++++++++++++++
 2 files changed, 563 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index cc64675..d0b473b 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -830,3 +830,431 @@ 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;
+}
+EXPORT_SYMBOL_GPL(of_changeset_create_device_nodev);
+
+/**
+ * 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;
+}
+EXPORT_SYMBOL_GPL(of_changeset_create_device_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;
+	int ret = -ENOMEM;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop)
+		return ret;
+
+	prop->name = kstrdup(name, GFP_KERNEL);
+	if (!prop->name)
+		goto out;
+
+	/*
+	 * 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.
+	 */
+	prop->value = kmemdup(value, length, GFP_KERNEL);
+	if (!prop->value)
+		goto out;
+
+	of_property_set_flag(prop, OF_DYNAMIC);
+	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)
+		return 0;
+
+out:
+	kfree(prop->value);
+	kfree(prop->name);
+	kfree(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;
+}
+
+/**
+ * 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);
+}
+EXPORT_SYMBOL_GPL(of_changeset_add_property_copy);
+
+/**
+ * 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);
+}
+EXPORT_SYMBOL_GPL(of_changeset_add_property_string);
+
+/**
+ * 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;
+}
+EXPORT_SYMBOL_GPL(of_changeset_add_property_stringf);
+
+/**
+ * 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);
+}
+EXPORT_SYMBOL_GPL(of_changeset_add_property_string_list);
+
+/**
+ * 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)
+{
+	__be32 _val = cpu_to_be32(val);
+	return __of_changeset_add_update_property_copy(ocs, np, name, &_val,
+			sizeof(_val), false);
+}
+EXPORT_SYMBOL_GPL(of_changeset_add_property_u32);
+
+/**
+ * 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_copy(ocs, np, name, "", 0,
+			false);
+}
+EXPORT_SYMBOL_GPL(of_changeset_add_property_bool);
+
+/**
+ * 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);
+}
+EXPORT_SYMBOL_GPL(of_changeset_update_property_copy);
+
+/**
+ * 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);
+}
+EXPORT_SYMBOL_GPL(of_changeset_update_property_string);
+
+/**
+ * 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;
+}
+EXPORT_SYMBOL_GPL(of_changeset_update_property_stringf);
+
+/**
+ * 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);
+}
+EXPORT_SYMBOL_GPL(of_changeset_update_property_string_list);
+
+/**
+ * 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)
+{
+	__be32 _val = cpu_to_be32(val);
+	return __of_changeset_add_update_property_copy(ocs, np, name, &_val,
+			sizeof(_val), true);
+}
+EXPORT_SYMBOL_GPL(of_changeset_update_property_u32);
+
+/**
+ * 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_copy(ocs, np, name, "", 0,
+			true);
+}
+EXPORT_SYMBOL_GPL(of_changeset_update_property_bool);
diff --git a/include/linux/of.h b/include/linux/of.h
index 299aeb1..9c80457 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1227,6 +1227,40 @@ 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);
+
 #else /* CONFIG_OF_DYNAMIC */
 static inline int of_reconfig_notifier_register(struct notifier_block *nb)
 {
@@ -1246,6 +1280,107 @@ 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;
+}
+
 #endif /* CONFIG_OF_DYNAMIC */
 
 /* CONFIG_OF_RESOLVE api */
-- 
2.9.3

--
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 v2 2/2] of: changesets: Introduce changeset helper methods

[Frank Rowand]

On 11/04/16 07:42, Hans de Goede wrote:
> From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> 
> 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>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v2 (hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> -Address review comments from:
>  https://www.spinics.net/lists/kernel/msg2252845.html

That points to the May 9 version 1 patches from Pantelis (as expected),
but containing 4, not 2, patches.  Patch 1/4 was applied.  Patch 4/4
seems to have disappeared?

Pantelis then sent a version 2 set of the patches on May 16.

Your version is a modification of the May 9 patches (as would be expected
of a version 2).  It is confusing to have two different version 2 patch
sets.  I don't have any brilliant ideas on how this patch set could have
been named differently to avoid that confusion.

The point of this little side-track is simply to note the existence of two
different version 2 series so I won't be confused when I revisit this
thread in the future.

>  -Simplify (and fix) __of_changeset_add_update_property_copy OOM handling
>  -Remove (by manual inlining) these 2 static helpers:
>   __of_changeset_add_update_property_u32
>   __of_changeset_add_update_property_bool
>  -Remove the following exported helper method:
>   of_changeset_node_move_to

Not all comments were addressed.

There are some other changes made that are not noted in the changelog.

I am still reading through the patches. I will reply again either with
a reviewed-by or specific comments when I finish.

-Frank

> ---
>  drivers/of/dynamic.c | 428 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h   | 135 ++++++++++++++++
>  2 files changed, 563 insertions(+)

< snip >
--
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 v2 2/2] of: changesets: Introduce changeset helper methods

[Hans de Goede]

Hi,

On 13-11-16 03:15, Frank Rowand wrote:
> On 11/04/16 07:42, Hans de Goede wrote:
>> From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>
>> 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>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> Changes in v2 (hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
>> -Address review comments from:
>>  https://www.spinics.net/lists/kernel/msg2252845.html
>
> That points to the May 9 version 1 patches from Pantelis (as expected),
> but containing 4, not 2, patches.  Patch 1/4 was applied.  Patch 4/4
> seems to have disappeared?

Ah, I cherry picked this into my tree a long time ago, hoping
Pantelis himself would upstream them, so I only cherry picked
the bare minimum and I did not pick-up the unit tests.

> Pantelis then sent a version 2 set of the patches on May 16.

Hmm, I never found this in google.

> Your version is a modification of the May 9 patches (as would be expected
> of a version 2).  It is confusing to have two different version 2 patch
> sets.  I don't have any brilliant ideas on how this patch set could have
> been named differently to avoid that confusion.

Actually my patches were cherry picked from Pantelis' beaglebone
capemanager tree, which I hope contained the last version.

> The point of this little side-track is simply to note the existence of two
> different version 2 series so I won't be confused when I revisit this
> thread in the future.

Ack, sorry about that.

Regards,

Hans


>
>>  -Simplify (and fix) __of_changeset_add_update_property_copy OOM handling
>>  -Remove (by manual inlining) these 2 static helpers:
>>   __of_changeset_add_update_property_u32
>>   __of_changeset_add_update_property_bool
>>  -Remove the following exported helper method:
>>   of_changeset_node_move_to
>
> Not all comments were addressed.
>
> There are some other changes made that are not noted in the changelog.
>
> I am still reading through the patches. I will reply again either with
> a reviewed-by or specific comments when I finish.
>
> -Frank
>
>> ---
>>  drivers/of/dynamic.c | 428 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/of.h   | 135 ++++++++++++++++
>>  2 files changed, 563 insertions(+)
>
> < snip >
>
--
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 v2 2/2] of: changesets: Introduce changeset helper methods

[Frank Rowand]

Hi Hans, Pantelis,

On 11/12/16 18:15, Frank Rowand wrote:
> On 11/04/16 07:42, Hans de Goede wrote:
>> From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>
>> 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>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>> Changes in v2 (hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
>> -Address review comments from:
>>  https://www.spinics.net/lists/kernel/msg2252845.html
> 
> That points to the May 9 version 1 patches from Pantelis (as expected),
> but containing 4, not 2, patches.  Patch 1/4 was applied.  Patch 4/4
> seems to have disappeared?
> 
> Pantelis then sent a version 2 set of the patches on May 16.
> 
> Your version is a modification of the May 9 patches (as would be expected
> of a version 2).  It is confusing to have two different version 2 patch
> sets.  I don't have any brilliant ideas on how this patch set could have
> been named differently to avoid that confusion.
> 
> The point of this little side-track is simply to note the existence of two
> different version 2 series so I won't be confused when I revisit this
> thread in the future.
> 
>>  -Simplify (and fix) __of_changeset_add_update_property_copy OOM handling
>>  -Remove (by manual inlining) these 2 static helpers:
>>   __of_changeset_add_update_property_u32
>>   __of_changeset_add_update_property_bool
>>  -Remove the following exported helper method:
>>   of_changeset_node_move_to
> 
> Not all comments were addressed.
> 
> There are some other changes made that are not noted in the changelog.
> 
> I am still reading through the patches. I will reply again either with
> a reviewed-by or specific comments when I finish.

Replying here for the entire patchset (there was no patch 0 to reply to).

After reading through the patches, my reply is meta instead of specific
comments about the code.

There are very few users of change sets in tree.  I do not see the need to
add these helpers until such users are likely to appear.

I would expect change sets to be _mostly_ used internally by the device tree
overlay framework, not directly by drivers.  If change sets are an attractive
technology for drivers, I want to approach that usage very carefully to avoid
inappropriate use, which could be very difficult to reign in after the fact.

Even if helpers should be added, this seems to be an overly complex approach.
If the need for these helpers becomes apparent I can provide review comments
with the specifics about how it appears to be overly complex.

Can you please  provide some more insights into the needs driving the desire
to have change set helpers and the expected use cases of them?  Please put
your architect's hat on when replying to this question.

-Frank

> 
> -Frank
> 
>> ---
>>  drivers/of/dynamic.c | 428 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/of.h   | 135 ++++++++++++++++
>>  2 files changed, 563 insertions(+)
> 
> < snip >
> 

--
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 v2 2/2] of: changesets: Introduce changeset helper methods

[Hans de Goede]

Hi,

On 14-11-16 08:34, Frank Rowand wrote:
> Hi Hans, Pantelis,
>
> On 11/12/16 18:15, Frank Rowand wrote:
>> On 11/04/16 07:42, Hans de Goede wrote:
>>> From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>>
>>> 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>
>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>> Changes in v2 (hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
>>> -Address review comments from:
>>>  https://www.spinics.net/lists/kernel/msg2252845.html
>>
>> That points to the May 9 version 1 patches from Pantelis (as expected),
>> but containing 4, not 2, patches.  Patch 1/4 was applied.  Patch 4/4
>> seems to have disappeared?
>>
>> Pantelis then sent a version 2 set of the patches on May 16.
>>
>> Your version is a modification of the May 9 patches (as would be expected
>> of a version 2).  It is confusing to have two different version 2 patch
>> sets.  I don't have any brilliant ideas on how this patch set could have
>> been named differently to avoid that confusion.
>>
>> The point of this little side-track is simply to note the existence of two
>> different version 2 series so I won't be confused when I revisit this
>> thread in the future.
>>
>>>  -Simplify (and fix) __of_changeset_add_update_property_copy OOM handling
>>>  -Remove (by manual inlining) these 2 static helpers:
>>>   __of_changeset_add_update_property_u32
>>>   __of_changeset_add_update_property_bool
>>>  -Remove the following exported helper method:
>>>   of_changeset_node_move_to
>>
>> Not all comments were addressed.
>>
>> There are some other changes made that are not noted in the changelog.
>>
>> I am still reading through the patches. I will reply again either with
>> a reviewed-by or specific comments when I finish.
>
> Replying here for the entire patchset (there was no patch 0 to reply to).
>
> After reading through the patches, my reply is meta instead of specific
> comments about the code.
>
> There are very few users of change sets in tree.  I do not see the need to
> add these helpers until such users are likely to appear.
>
> I would expect change sets to be _mostly_ used internally by the device tree
> overlay framework, not directly by drivers.  If change sets are an attractive
> technology for drivers, I want to approach that usage very carefully to avoid
> inappropriate use, which could be very difficult to reign in after the fact.
>
> Even if helpers should be added, this seems to be an overly complex approach.
> If the need for these helpers becomes apparent I can provide review comments
> with the specifics about how it appears to be overly complex.
>
> Can you please  provide some more insights into the needs driving the desire
> to have change set helpers and the expected use cases of them?  Please put
> your architect's hat on when replying to this question.

My use case for this is discussed in this thread:
https://www.spinics.net/lists/arm-kernel/msg536111.html

With the dt-bindings for the hardware-manager I want to add here:
https://www.spinics.net/lists/arm-kernel/msg536109.html

Note that there is a lot of discussion in this thread whether or
not this belongs in the kernel. I strongly believe though that
some functionality like this will be needed in the kernel for
ARM+dt devices going forward, just like there is plenty of x86
code which adjusts itself to specific hardware, because whether
we like it or not hardware (revisions) will always have quirks.

Regards,

Hans
--
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 v2 2/2] of: changesets: Introduce changeset helper methods

[Frank Rowand]

On 11/14/16 03:04, Hans de Goede wrote:
> Hi,
> 
> On 14-11-16 08:34, Frank Rowand wrote:
>> Hi Hans, Pantelis,
>>
>> On 11/12/16 18:15, Frank Rowand wrote:
>>> On 11/04/16 07:42, Hans de Goede wrote:
>>>> From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>>>
>>>> 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>
>>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>> Changes in v2 (hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
>>>> -Address review comments from:
>>>>  https://www.spinics.net/lists/kernel/msg2252845.html
>>>
>>> That points to the May 9 version 1 patches from Pantelis (as expected),
>>> but containing 4, not 2, patches.  Patch 1/4 was applied.  Patch 4/4
>>> seems to have disappeared?
>>>
>>> Pantelis then sent a version 2 set of the patches on May 16.
>>>
>>> Your version is a modification of the May 9 patches (as would be expected
>>> of a version 2).  It is confusing to have two different version 2 patch
>>> sets.  I don't have any brilliant ideas on how this patch set could have
>>> been named differently to avoid that confusion.
>>>
>>> The point of this little side-track is simply to note the existence of two
>>> different version 2 series so I won't be confused when I revisit this
>>> thread in the future.
>>>
>>>>  -Simplify (and fix) __of_changeset_add_update_property_copy OOM handling
>>>>  -Remove (by manual inlining) these 2 static helpers:
>>>>   __of_changeset_add_update_property_u32
>>>>   __of_changeset_add_update_property_bool
>>>>  -Remove the following exported helper method:
>>>>   of_changeset_node_move_to
>>>
>>> Not all comments were addressed.
>>>
>>> There are some other changes made that are not noted in the changelog.
>>>
>>> I am still reading through the patches. I will reply again either with
>>> a reviewed-by or specific comments when I finish.
>>
>> Replying here for the entire patchset (there was no patch 0 to reply to).
>>
>> After reading through the patches, my reply is meta instead of specific
>> comments about the code.
>>
>> There are very few users of change sets in tree.  I do not see the need to
>> add these helpers until such users are likely to appear.
>>
>> I would expect change sets to be _mostly_ used internally by the device tree
>> overlay framework, not directly by drivers.  If change sets are an attractive
>> technology for drivers, I want to approach that usage very carefully to avoid
>> inappropriate use, which could be very difficult to reign in after the fact.
>>
>> Even if helpers should be added, this seems to be an overly complex approach.
>> If the need for these helpers becomes apparent I can provide review comments
>> with the specifics about how it appears to be overly complex.
>>
>> Can you please  provide some more insights into the needs driving the desire
>> to have change set helpers and the expected use cases of them?  Please put
>> your architect's hat on when replying to this question.
> 
> My use case for this is discussed in this thread:
> https://www.spinics.net/lists/arm-kernel/msg536111.html
> 
> With the dt-bindings for the hardware-manager I want to add here:
> https://www.spinics.net/lists/arm-kernel/msg536109.html
> 
> Note that there is a lot of discussion in this thread whether or
> not this belongs in the kernel. I strongly believe though that
> some functionality like this will be needed in the kernel for
> ARM+dt devices going forward, just like there is plenty of x86
> code which adjusts itself to specific hardware, because whether
> we like it or not hardware (revisions) will always have quirks.

Thanks! That context should have been provided with the patches.

The use case discussion is important and I am paying a lot of
attention to that discussion and many other discussions about
dynamic device trees.  I don't think it makes sense to apply the
change set helper patches yet, given the unsettled state of the
various dynamic device tree discussions.

-Frank
--
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 v2 2/2] of: changesets: Introduce changeset helper methods

[Rob Herring]

On Mon, Nov 14, 2016 at 12:44 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 11/14/16 03:04, Hans de Goede wrote:
>> Hi,
>>
>> On 14-11-16 08:34, Frank Rowand wrote:
>>> Hi Hans, Pantelis,
>>>
>>> On 11/12/16 18:15, Frank Rowand wrote:
>>>> On 11/04/16 07:42, Hans de Goede wrote:
>>>>> From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>>>>
>>>>> 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>
>>>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>> ---
>>>>> Changes in v2 (hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
>>>>> -Address review comments from:
>>>>>  https://www.spinics.net/lists/kernel/msg2252845.html
>>>>
>>>> That points to the May 9 version 1 patches from Pantelis (as expected),
>>>> but containing 4, not 2, patches.  Patch 1/4 was applied.  Patch 4/4
>>>> seems to have disappeared?
>>>>
>>>> Pantelis then sent a version 2 set of the patches on May 16.
>>>>
>>>> Your version is a modification of the May 9 patches (as would be expected
>>>> of a version 2).  It is confusing to have two different version 2 patch
>>>> sets.  I don't have any brilliant ideas on how this patch set could have
>>>> been named differently to avoid that confusion.
>>>>
>>>> The point of this little side-track is simply to note the existence of two
>>>> different version 2 series so I won't be confused when I revisit this
>>>> thread in the future.
>>>>
>>>>>  -Simplify (and fix) __of_changeset_add_update_property_copy OOM handling
>>>>>  -Remove (by manual inlining) these 2 static helpers:
>>>>>   __of_changeset_add_update_property_u32
>>>>>   __of_changeset_add_update_property_bool
>>>>>  -Remove the following exported helper method:
>>>>>   of_changeset_node_move_to
>>>>
>>>> Not all comments were addressed.
>>>>
>>>> There are some other changes made that are not noted in the changelog.
>>>>
>>>> I am still reading through the patches. I will reply again either with
>>>> a reviewed-by or specific comments when I finish.
>>>
>>> Replying here for the entire patchset (there was no patch 0 to reply to).
>>>
>>> After reading through the patches, my reply is meta instead of specific
>>> comments about the code.
>>>
>>> There are very few users of change sets in tree.  I do not see the need to
>>> add these helpers until such users are likely to appear.
>>>
>>> I would expect change sets to be _mostly_ used internally by the device tree
>>> overlay framework, not directly by drivers.  If change sets are an attractive
>>> technology for drivers, I want to approach that usage very carefully to avoid
>>> inappropriate use, which could be very difficult to reign in after the fact.
>>>
>>> Even if helpers should be added, this seems to be an overly complex approach.
>>> If the need for these helpers becomes apparent I can provide review comments
>>> with the specifics about how it appears to be overly complex.
>>>
>>> Can you please  provide some more insights into the needs driving the desire
>>> to have change set helpers and the expected use cases of them?  Please put
>>> your architect's hat on when replying to this question.
>>
>> My use case for this is discussed in this thread:
>> https://www.spinics.net/lists/arm-kernel/msg536111.html
>>
>> With the dt-bindings for the hardware-manager I want to add here:
>> https://www.spinics.net/lists/arm-kernel/msg536109.html
>>
>> Note that there is a lot of discussion in this thread whether or
>> not this belongs in the kernel. I strongly believe though that
>> some functionality like this will be needed in the kernel for
>> ARM+dt devices going forward, just like there is plenty of x86
>> code which adjusts itself to specific hardware, because whether
>> we like it or not hardware (revisions) will always have quirks.
>
> Thanks! That context should have been provided with the patches.
>
> The use case discussion is important and I am paying a lot of
> attention to that discussion and many other discussions about
> dynamic device trees.  I don't think it makes sense to apply the
> change set helper patches yet, given the unsettled state of the
> various dynamic device tree discussions.

These helpers are useful and easier to use than the existing API
independent of any issues to sort out with how we use overlays. So I
plan to take them whether there's a user right away or not.

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 v2 2/2] of: changesets: Introduce changeset helper methods

[Frank Rowand]

On 11/14/16 14:16, Rob Herring wrote:
> On Mon, Nov 14, 2016 at 12:44 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 11/14/16 03:04, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 14-11-16 08:34, Frank Rowand wrote:
>>>> Hi Hans, Pantelis,
>>>>
>>>> On 11/12/16 18:15, Frank Rowand wrote:
>>>>> On 11/04/16 07:42, Hans de Goede wrote:
>>>>>> From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>>>>>
>>>>>> 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>
>>>>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>>> ---
>>>>>> Changes in v2 (hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
>>>>>> -Address review comments from:
>>>>>>  https://www.spinics.net/lists/kernel/msg2252845.html
>>>>>
>>>>> That points to the May 9 version 1 patches from Pantelis (as expected),
>>>>> but containing 4, not 2, patches.  Patch 1/4 was applied.  Patch 4/4
>>>>> seems to have disappeared?
>>>>>
>>>>> Pantelis then sent a version 2 set of the patches on May 16.
>>>>>
>>>>> Your version is a modification of the May 9 patches (as would be expected
>>>>> of a version 2).  It is confusing to have two different version 2 patch
>>>>> sets.  I don't have any brilliant ideas on how this patch set could have
>>>>> been named differently to avoid that confusion.
>>>>>
>>>>> The point of this little side-track is simply to note the existence of two
>>>>> different version 2 series so I won't be confused when I revisit this
>>>>> thread in the future.
>>>>>
>>>>>>  -Simplify (and fix) __of_changeset_add_update_property_copy OOM handling
>>>>>>  -Remove (by manual inlining) these 2 static helpers:
>>>>>>   __of_changeset_add_update_property_u32
>>>>>>   __of_changeset_add_update_property_bool
>>>>>>  -Remove the following exported helper method:
>>>>>>   of_changeset_node_move_to
>>>>>
>>>>> Not all comments were addressed.
>>>>>
>>>>> There are some other changes made that are not noted in the changelog.
>>>>>
>>>>> I am still reading through the patches. I will reply again either with
>>>>> a reviewed-by or specific comments when I finish.
>>>>
>>>> Replying here for the entire patchset (there was no patch 0 to reply to).
>>>>
>>>> After reading through the patches, my reply is meta instead of specific
>>>> comments about the code.
>>>>
>>>> There are very few users of change sets in tree.  I do not see the need to
>>>> add these helpers until such users are likely to appear.
>>>>
>>>> I would expect change sets to be _mostly_ used internally by the device tree
>>>> overlay framework, not directly by drivers.  If change sets are an attractive
>>>> technology for drivers, I want to approach that usage very carefully to avoid
>>>> inappropriate use, which could be very difficult to reign in after the fact.
>>>>
>>>> Even if helpers should be added, this seems to be an overly complex approach.
>>>> If the need for these helpers becomes apparent I can provide review comments
>>>> with the specifics about how it appears to be overly complex.
>>>>
>>>> Can you please  provide some more insights into the needs driving the desire
>>>> to have change set helpers and the expected use cases of them?  Please put
>>>> your architect's hat on when replying to this question.
>>>
>>> My use case for this is discussed in this thread:
>>> https://www.spinics.net/lists/arm-kernel/msg536111.html
>>>
>>> With the dt-bindings for the hardware-manager I want to add here:
>>> https://www.spinics.net/lists/arm-kernel/msg536109.html
>>>
>>> Note that there is a lot of discussion in this thread whether or
>>> not this belongs in the kernel. I strongly believe though that
>>> some functionality like this will be needed in the kernel for
>>> ARM+dt devices going forward, just like there is plenty of x86
>>> code which adjusts itself to specific hardware, because whether
>>> we like it or not hardware (revisions) will always have quirks.
>>
>> Thanks! That context should have been provided with the patches.
>>
>> The use case discussion is important and I am paying a lot of
>> attention to that discussion and many other discussions about
>> dynamic device trees.  I don't think it makes sense to apply the
>> change set helper patches yet, given the unsettled state of the
>> various dynamic device tree discussions.
> 
> These helpers are useful and easier to use than the existing API
> independent of any issues to sort out with how we use overlays. So I
> plan to take them whether there's a user right away or not.
> 
> Rob

OK, expect a more detailed review from me this week.

-Frank
--
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 v2 2/2] of: changesets: Introduce changeset helper methods

[Frank Rowand]

Rob, Hans, Pantelis,

On 11/14/16 17:56, Frank Rowand wrote:
> On 11/14/16 14:16, Rob Herring wrote:
>> On Mon, Nov 14, 2016 at 12:44 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

< snip >

>>
>> These helpers are useful and easier to use than the existing API
>> independent of any issues to sort out with how we use overlays. So I
>> plan to take them whether there's a user right away or not.
>>
>> Rob
> 
> OK, expect a more detailed review from me this week.
> 
> -Frank

Part of my issues with these patches is related to using a format
string and the variables required by that format string as arguments
to several of the proposed helper functions.  That construct is driven
by the helper functions calling  __of_node_dup() which has that same
pattern of arguments.  Blindly accepting a format string as an argument
to populate a buffer is not good from a security or robustness standpoint.

The only callers of __of_node_dup() are one site in drivers/of/overlay.c
and three sites in drivers/of/unittest.c.

I would like to see if I can find a good alternate to the format
string approach in __of_node_dup(), which would remove that issue
in the helper functions.

I do not expect Hans to fix the existing __of_node_dup(), I am
willing to do that myself.

Rob, are you in a hurry to accept the helper functions or are you
willing to give me some time to resolve the __of_node_dup() issue
and come up with a new version of the helper function patches?

Caveat, I have a hard deadline late Monday Nov 21 so I can't
start on this until Nov 22.  Then that is Thanksgiving week and
I have some other work commitments that will demand much of my
time the next two weeks.  I can commit to starting Nov 22, then
making it my top priority starting Dec 12.

-Frank
--
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 v2 0/2] of: Dynamic DT updates

[Pantelis Antoniou]

The two patches add a new API for using changeset which
makes things considerably easier.

This patchset applies against Linus's tree as of today and
is dependent on the previous patchset I've send out earlier.
"of: overlay: kobject & sysfs'ation"

Changes since v1:
* Dropped the indirect and target root patches until we
figure out what to do with the DT connector.

Pantelis Antoniou (2):
  of: dynamic: Add __of_node_dupv()
  of: changesets: Introduce changeset helper methods

 drivers/of/dynamic.c | 280 +++++++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/of.h   |  74 ++++++++++++++
 2 files changed, 348 insertions(+), 6 deletions(-)

-- 
1.7.12

[PATCH v2 1/2] 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 53826b8..e452171 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