[PATCH v2 0/2] of: Dynamic DT updates

[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

[PATCH v2 2/2] 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 | 251 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h   |  74 +++++++++++++++
 2 files changed, 325 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index e452171..afa8e31 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -796,3 +796,254 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
 	list_add_tail(&ce->node, &ocs->entries);
 	return 0;
 }
+
+/* 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;
+}
+
+/**
+ * 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)
+{
+	struct property *prop;
+	char *new_name;
+	void *new_value;
+	int ret;
+
+	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;
+
+	ret = of_changeset_add_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;
+}
+
+/**
+ * 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_property_copy(ocs, np, name, str,
+			strlen(str) + 1);
+}
+
+/**
+ * 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;
+	char *str;
+	int ret;
+
+	va_start(vargs, fmt);
+	str = kvasprintf(GFP_KERNEL, fmt, vargs);
+	va_end(vargs);
+
+	ret = of_changeset_add_property_string(ocs, np, name, str);
+
+	kfree(str);
+	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)
+{
+	int total, length, i, ret;
+	char *value, *s;
+
+	total = 0;
+	for (i = 0; i < count; i++) {
+		length = strlen(strs[i]);
+		total += length + 1;
+	}
+
+	value = kmalloc(total, GFP_KERNEL);
+	if (!value)
+		return -ENOMEM;
+
+	for (i = 0, s = value; i < count; i++) {
+		length = strlen(strs[i]);
+		memcpy(s, strs[i], length + 1);
+		s += length + 1;
+	}
+
+	ret = of_changeset_add_property_copy(ocs, np, name, value, total);
+
+	kfree(value);
+
+	return ret;
+}
+
+/**
+ * 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)
+{
+	/* in place */
+	val = cpu_to_be32(val);
+	return of_changeset_add_property_copy(ocs, np, name, &val, sizeof(val));
+}
+
+/**
+ * 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_property_copy(ocs, np, name, "", 0);
+}
diff --git a/include/linux/of.h b/include/linux/of.h
index b0856ed..1e6019a 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1037,6 +1037,27 @@ 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);
+
 #else /* CONFIG_OF_DYNAMIC */
 static inline int of_reconfig_notifier_register(struct notifier_block *nb)
 {
@@ -1056,6 +1077,59 @@ 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;
+}
 #endif /* CONFIG_OF_DYNAMIC */
 
 /* CONFIG_OF_RESOLVE api */
-- 
1.7.12

Re: [PATCH v2 0/2] of: Dynamic DT updates

[Rob Herring]

On Wed, Sep 16, 2015 at 11:11 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> 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"

Oh good, I'll ignore it until that is merged. ;) Seriously, this
doesn't really look dependent. It doesn't involve ABIs either, so it
is easier.

Rob

>
> 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
>

Re: [PATCH v2 0/2] of: Dynamic DT updates

[Pantelis Antoniou]

Hi Rob,

> On Sep 16, 2015, at 19:43 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Wed, Sep 16, 2015 at 11:11 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> 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"
> 
> Oh good, I'll ignore it until that is merged. ;) Seriously, this
> doesn't really look dependent. It doesn't involve ABIs either, so it
> is easier.
> 

Oops. Sorry this dependency was for the patches that they were dropped.
This series is stand-alone.

> Rob
> 
>> 

Regards

— Pantelis

>> 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
>> 

Re: [PATCH v2 2/2] of: changesets: Introduce changeset helper methods

[Rob Herring]

On 09/16/2015 11:11 AM, Pantelis Antoniou 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");

How about updating the unittest to use this.

> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  drivers/of/dynamic.c | 251 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h   |  74 +++++++++++++++
>  2 files changed, 325 insertions(+)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index e452171..afa8e31 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -796,3 +796,254 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
>  	list_add_tail(&ce->node, &ocs->entries);
>  	return 0;
>  }
> +
> +/* 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 here and on others?

> +
> +/**
> + * 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;
> +}
> +
> +/**
> + * 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)
> +{
> +	struct property *prop;
> +	char *new_name;
> +	void *new_value;

> +	int ret;
> +
> +	ret = -ENOMEM;

One line

> +
> +	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;
> +
> +	ret = of_changeset_add_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;
> +}
> +
> +/**
> + * 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_property_copy(ocs, np, name, str,
> +			strlen(str) + 1);
> +}
> +
> +/**
> + * 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;
> +	char *str;
> +	int ret;
> +
> +	va_start(vargs, fmt);
> +	str = kvasprintf(GFP_KERNEL, fmt, vargs);
> +	va_end(vargs);
> +
> +	ret = of_changeset_add_property_string(ocs, np, name, str);
> +
> +	kfree(str);
> +	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)
> +{
> +	int total, length, i, ret;
> +	char *value, *s;
> +
> +	total = 0;

initialize in declaration.

> +	for (i = 0; i < count; i++) {
> +		length = strlen(strs[i]);
> +		total += length + 1;

total += strlen(strs[i]) + 1;

What if strings are NULL? Error or skip? I vote for error.

> +	}
> +
> +	value = kmalloc(total, GFP_KERNEL);
> +	if (!value)
> +		return -ENOMEM;
> +
> +	for (i = 0, s = value; i < count; i++) {
> +		length = strlen(strs[i]);
> +		memcpy(s, strs[i], length + 1);

strcpy perhaps?

> +		s += length + 1;
> +	}
> +
> +	ret = of_changeset_add_property_copy(ocs, np, name, value, total);
> +
> +	kfree(value);
> +
> +	return ret;
> +}
> +
> +/**
> + * 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)
> +{
> +	/* in place */
> +	val = cpu_to_be32(val);
> +	return of_changeset_add_property_copy(ocs, np, name, &val, sizeof(val));
> +}
> +
> +/**
> + * 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_property_copy(ocs, np, name, "", 0);
> +}
> diff --git a/include/linux/of.h b/include/linux/of.h
> index b0856ed..1e6019a 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1037,6 +1037,27 @@ 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);
> +
>  #else /* CONFIG_OF_DYNAMIC */
>  static inline int of_reconfig_notifier_register(struct notifier_block *nb)
>  {
> @@ -1056,6 +1077,59 @@ 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;
> +}
>  #endif /* CONFIG_OF_DYNAMIC */
>  
>  /* CONFIG_OF_RESOLVE api */
> 

Re: [PATCH v2 2/2] of: changesets: Introduce changeset helper methods

[Pantelis Antoniou]

Hi Rob,

> On Sep 17, 2015, at 17:13 , Rob Herring <robh@kernel.org> wrote:
> 
> On 09/16/2015 11:11 AM, Pantelis Antoniou 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");
> 
> How about updating the unittest to use this.
> 

OK.

>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> drivers/of/dynamic.c | 251 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/of.h   |  74 +++++++++++++++
>> 2 files changed, 325 insertions(+)
>> 
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index e452171..afa8e31 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -796,3 +796,254 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
>> 	list_add_tail(&ce->node, &ocs->entries);
>> 	return 0;
>> }
>> +
>> +/* 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 here and on others?
> 

Err, none of the others of_changeset* methods are exported right now.
Up to now it was all internal API; should I go ahead and put EXPORT_SYMBOL_GPL
to all the others as well?

>> +
>> +/**
>> + * 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;
>> +}
>> +
>> +/**
>> + * 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)
>> +{
>> +	struct property *prop;
>> +	char *new_name;
>> +	void *new_value;
> 
>> +	int ret;
>> +
>> +	ret = -ENOMEM;
> 
> One line
> 

OK

>> +
>> +	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;
>> +
>> +	ret = of_changeset_add_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;
>> +}
>> +
>> +/**
>> + * 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_property_copy(ocs, np, name, str,
>> +			strlen(str) + 1);
>> +}
>> +
>> +/**
>> + * 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;
>> +	char *str;
>> +	int ret;
>> +
>> +	va_start(vargs, fmt);
>> +	str = kvasprintf(GFP_KERNEL, fmt, vargs);
>> +	va_end(vargs);
>> +
>> +	ret = of_changeset_add_property_string(ocs, np, name, str);
>> +
>> +	kfree(str);
>> +	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)
>> +{
>> +	int total, length, i, ret;
>> +	char *value, *s;
>> +
>> +	total = 0;
> 
> initialize in declaration.
> 
OK

>> +	for (i = 0; i < count; i++) {
>> +		length = strlen(strs[i]);
>> +		total += length + 1;
> 
> total += strlen(strs[i]) + 1;
> 
> What if strings are NULL? Error or skip? I vote for error.
> 

Yeah.

>> +	}
>> +
>> +	value = kmalloc(total, GFP_KERNEL);
>> +	if (!value)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0, s = value; i < count; i++) {
>> +		length = strlen(strs[i]);
>> +		memcpy(s, strs[i], length + 1);
> 
> strcpy perhaps?
> 

Yes

>> +		s += length + 1;
>> +	}
>> +
>> +	ret = of_changeset_add_property_copy(ocs, np, name, value, total);
>> +
>> +	kfree(value);
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * 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)
>> +{
>> +	/* in place */
>> +	val = cpu_to_be32(val);
>> +	return of_changeset_add_property_copy(ocs, np, name, &val, sizeof(val));
>> +}
>> +
>> +/**
>> + * 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_property_copy(ocs, np, name, "", 0);
>> +}
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index b0856ed..1e6019a 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -1037,6 +1037,27 @@ 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);
>> +
>> #else /* CONFIG_OF_DYNAMIC */
>> static inline int of_reconfig_notifier_register(struct notifier_block *nb)
>> {
>> @@ -1056,6 +1077,59 @@ 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;
>> +}
>> #endif /* CONFIG_OF_DYNAMIC */
>> 
>> /* CONFIG_OF_RESOLVE api */

OK, updated patchset over the weekend.

Regards

— Pantelis

Re: [PATCH v2 2/2] of: changesets: Introduce changeset helper methods

[Rob Herring]

On Fri, Sep 18, 2015 at 4:15 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On Sep 17, 2015, at 17:13 , Rob Herring <robh@kernel.org> wrote:
>>
>> On 09/16/2015 11:11 AM, Pantelis Antoniou 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.

[...]

>>> +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 here and on others?
>>
>
> Err, none of the others of_changeset* methods are exported right now.
> Up to now it was all internal API; should I go ahead and put EXPORT_SYMBOL_GPL
> to all the others as well?

Okay, NM then.

Rob

Re: [PATCH v2 2/2] of: changesets: Introduce changeset helper methods

[Geert Uytterhoeven]

Hi Pantelis,

On Wed, Sep 16, 2015 at 6:11 PM, 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");

What about removing properties?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] of: changesets: Introduce changeset helper methods

[Pantelis Antoniou]

Hi Geert,

> On Sep 21, 2015, at 15:35 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> Hi Pantelis,
> 
> On Wed, Sep 16, 2015 at 6:11 PM, 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");
> 
> What about removing properties?
> 

Once upon a time there was that capability. It was removed after we didn’t have
a good use for them yet. Do you have any? I’d be happy to re-add it :)


> Gr{oetje,eeting}s,
> 
>                        Geert
> 

Regards

— Pantelis

> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds

Re: [PATCH v2 2/2] of: changesets: Introduce changeset helper methods

[Geert Uytterhoeven]

Hi Pantelis,

On Mon, Sep 21, 2015 at 2:36 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
>> On Sep 21, 2015, at 15:35 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Wed, Sep 16, 2015 at 6:11 PM, 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");
>>
>> What about removing properties?
>
> Once upon a time there was that capability. It was removed after we didn’t have
> a good use for them yet. Do you have any? I’d be happy to re-add it :)

Aliases?

If an overlay removes e.g. a serial port, it should remove its alias, too.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] of: changesets: Introduce changeset helper methods

[Pantelis Antoniou]

Hi Geert,

> On Sep 21, 2015, at 15:47 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> Hi Pantelis,
> 
> On Mon, Sep 21, 2015 at 2:36 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>>> On Sep 21, 2015, at 15:35 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Wed, Sep 16, 2015 at 6:11 PM, 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");
>>> 
>>> What about removing properties?
>> 
>> Once upon a time there was that capability. It was removed after we didn’t have
>> a good use for them yet. Do you have any? I’d be happy to re-add it :)
> 
> Aliases?
> 
> If an overlay removes e.g. a serial port, it should remove its alias, too.
> 

Well, that case is handled. Addition of a property results in removal of a property when
the overlay is reverted.

This is an accessor API that makes things simpler than what’s internally used at
overlays.

We’re talking about removal of a property/node as part of application.

> Gr{oetje,eeting}s,
> 
>                        Geert
> 

Regards

— Pantelis

> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds

Re: [PATCH v2 2/2] of: changesets: Introduce changeset helper methods

[Geert Uytterhoeven]

Hi Pantelis,

On Mon, Sep 21, 2015 at 2:49 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
>> On Sep 21, 2015, at 15:47 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Mon, Sep 21, 2015 at 2:36 PM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>>> On Sep 21, 2015, at 15:35 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Sep 16, 2015 at 6:11 PM, 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");
>>>>
>>>> What about removing properties?
>>>
>>> Once upon a time there was that capability. It was removed after we didn’t have
>>> a good use for them yet. Do you have any? I’d be happy to re-add it :)
>>
>> Aliases?
>>
>> If an overlay removes e.g. a serial port, it should remove its alias, too.
>
> Well, that case is handled. Addition of a property results in removal of a property when
> the overlay is reverted.

Actually what I meant is the other way around: _adding_ the overlay would
_remove_ the alias.

I have a board with an SDHI connector, and an expansion connector.
SDHI and serial on the expansion connector share the same pins.
By default, SDHI is enabled in the DTS.

To add a serial port to the expansion connector, I disable the SDHI device,
add the alias, and add the serial device.
(dtsi in http://www.spinics.net/lists/devicetree/msg79438.html)

Now imagine doing the opposite: having the serial device enabled by default.
Then the overlay should disable the serial device, remove the alias, and add
the SDHI device.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] of: changesets: Introduce changeset helper methods

[Pantelis Antoniou]

Hi Geert,

> On Sep 21, 2015, at 16:07 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> Hi Pantelis,
> 
> On Mon, Sep 21, 2015 at 2:49 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>>> On Sep 21, 2015, at 15:47 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Mon, Sep 21, 2015 at 2:36 PM, Pantelis Antoniou
>>> <pantelis.antoniou@konsulko.com> wrote:
>>>>> On Sep 21, 2015, at 15:35 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Wed, Sep 16, 2015 at 6:11 PM, 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");
>>>>> 
>>>>> What about removing properties?
>>>> 
>>>> Once upon a time there was that capability. It was removed after we didn’t have
>>>> a good use for them yet. Do you have any? I’d be happy to re-add it :)
>>> 
>>> Aliases?
>>> 
>>> If an overlay removes e.g. a serial port, it should remove its alias, too.
>> 
>> Well, that case is handled. Addition of a property results in removal of a property when
>> the overlay is reverted.
> 
> Actually what I meant is the other way around: _adding_ the overlay would
> _remove_ the alias.
> 
> I have a board with an SDHI connector, and an expansion connector.
> SDHI and serial on the expansion connector share the same pins.
> By default, SDHI is enabled in the DTS.
> 
> To add a serial port to the expansion connector, I disable the SDHI device,
> add the alias, and add the serial device.
> (dtsi in http://www.spinics.net/lists/devicetree/msg79438.html)
> 
> Now imagine doing the opposite: having the serial device enabled by default.
> Then the overlay should disable the serial device, remove the alias, and add
> the SDHI device.
> 

Excellent; this is the use-case I was looking for :)

> Gr{oetje,eeting}s,
> 
>                        Geert
> 

Regards

— Pantelis

> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds