[PATCH 0/5] of: Resolver and dynamic updates

[PATCH 0/5] of: Resolver and dynamic updates

[Pantelis Antoniou]

The following patch-series implements a number of fixes to the
dynamic DT handling of the kernel, and provides infrastructure that
the upcoming Device Tree Overlay patchset will use.

Pantelis Antoniou (5):
  of: resolver: Switch to new local fixups format.
  of: testcases: Update with new local fixups format
  of: Only call notifiers when node is attached
  of: Add old prop argument on OF_RECONFIG_UPDATE_PROPERTY
  of: of_reconfig_get_state_change() of notifier helper.

 drivers/of/base.c                      |   9 +-
 drivers/of/dynamic.c                   | 119 ++++++++++++++++++--
 drivers/of/resolver.c                  | 191 ++++++++++++++++++++++++++++-----
 drivers/of/selftest.c                  |   2 +-
 drivers/of/testcase-data/testcases.dts |  61 ++++++-----
 include/linux/of.h                     |  21 ++--
 6 files changed, 334 insertions(+), 69 deletions(-)

-- 
1.7.12

[PATCH 1/5] of: resolver: Switch to new local fixups format.

[Pantelis Antoniou]

The original resolver format is way too cryptic, switch
to using a tree based format that gets rid of repetitions,
is more compact and readable.

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

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index aed7959..0224348 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -111,7 +111,8 @@ static void __of_adjust_tree_phandles(struct device_node *node,
 		__of_adjust_tree_phandles(child, phandle_delta);
 }
 
-static int __of_adjust_phandle_ref(struct device_node *node, struct property *rprop, int value, bool is_delta)
+static int __of_adjust_phandle_ref(struct device_node *node,
+		struct property *rprop, int value)
 {
 	phandle phandle;
 	struct device_node *refnode;
@@ -181,7 +182,7 @@ static int __of_adjust_phandle_ref(struct device_node *node, struct property *rp
 			goto err_fail;
 		}
 
-		phandle = is_delta ? be32_to_cpup(sprop->value + offset) + value : value;
+		phandle = value;
 		*(__be32 *)(sprop->value + offset) = cpu_to_be32(phandle);
 	}
 
@@ -190,42 +191,166 @@ err_fail:
 	return err;
 }
 
+/* compare nodes taking into account that 'name' strips out the @ part */
+static int __of_node_name_cmp(const struct device_node *dn1,
+		const struct device_node *dn2)
+{
+	const char *n1;
+	const char *n2;
+
+	n1 = strrchr(dn1->full_name, '/');
+	if (n1 == NULL)
+		n1 = "/";
+
+	n2 = strrchr(dn2->full_name, '/');
+	if (n2 == NULL)
+		n2 = "/";
+
+	return of_node_cmp(n1, n2);
+}
+
 /*
- * Adjust the local phandle references by the given phandle delta.
- * Assumes the existances of a __local_fixups__ node at the root
+ * Verify that the local phandle references are correct.
+ * Assumes the existence of a __local_fixups__ node at the root
  * of the tree. Does not take any devtree locks so make sure you
  * call this on a tree which is at the detached state.
  */
-static int __of_adjust_tree_phandle_references(struct device_node *node,
-		int phandle_delta)
+static int __of_verify_tree_phandle_references(struct device_node *node,
+		struct device_node *target)
 {
-	struct device_node *child;
-	struct property *rprop;
-	int err;
+	struct device_node *child, *childtarget;
+	struct property *rprop, *sprop;
+	int err, i, count;
+	unsigned int off;
 
-	/* locate the symbols & fixups nodes on resolve */
-	for_each_child_of_node(node, child)
-		if (of_node_cmp(child->name, "__local_fixups__") == 0)
-			break;
-
-	/* no local fixups */
-	if (!child)
+	if (node == NULL)
 		return 0;
 
-	/* find the local fixups property */
-	for_each_property_of_node(child, rprop) {
+	for_each_property_of_node(node, rprop) {
+
 		/* skip properties added automatically */
-		if (of_prop_cmp(rprop->name, "name") == 0)
+		if (of_prop_cmp(rprop->name, "name") == 0 ||
+		    of_prop_cmp(rprop->name, "phandle") == 0 ||
+		    of_prop_cmp(rprop->name, "linux,phandle") == 0)
 			continue;
 
-		err = __of_adjust_phandle_ref(node, rprop, phandle_delta, true);
-		if (err)
+		if ((rprop->length % 4) != 0 || rprop->length == 0) {
+			pr_err("%s: Illegal property (size) '%s' @%s\n",
+					__func__, rprop->name, node->full_name);
+			return -EINVAL;
+		}
+		count = rprop->length / sizeof(__be32);
+
+		/* now find the target property */
+		for_each_property_of_node(target, sprop) {
+			if (of_prop_cmp(sprop->name, rprop->name) == 0)
+				break;
+		}
+
+		if (sprop == NULL) {
+			pr_err("%s: Could not find target property '%s' @%s\n",
+					__func__, rprop->name, node->full_name);
+			return -EINVAL;
+		}
+
+		for (i = 0; i < count; i++) {
+			off = be32_to_cpu(((__be32 *)rprop->value)[i]);
+			/* make sure the offset doesn't overstep (even wrap) */
+			if (off >= sprop->length ||
+					(off + 4) > sprop->length) {
+				pr_err("%s: Illegal property '%s' @%s\n",
+						__func__, rprop->name,
+						node->full_name);
+				return -EINVAL;
+			}
+		}
+	}
+
+	for_each_child_of_node(node, child) {
+
+		for_each_child_of_node(target, childtarget)
+			if (__of_node_name_cmp(child, childtarget) == 0)
+				break;
+
+		if (!childtarget) {
+			pr_err("%s: Could not find target child '%s' @%s\n",
+					__func__, child->name, node->full_name);
+			return -EINVAL;
+		}
+
+		err = __of_verify_tree_phandle_references(child, childtarget);
+		if (err != 0)
 			return err;
 	}
 
 	return 0;
 }
 
+/*
+ * Adjust the local phandle references by the given phandle delta.
+ * Assumes the existances of a __local_fixups__ node at the root.
+ * Assumes that __of_verify_tree_phandle_references has been called.
+ * Does not take any devtree locks so make sure you call this on a tree
+ * which is at the detached state.
+ */
+static void __of_adjust_tree_phandle_references(struct device_node *node,
+		struct device_node *target, int phandle_delta)
+{
+	struct device_node *child, *childtarget;
+	struct property *rprop, *sprop;
+	int i, count;
+	unsigned int off;
+	phandle phandle;
+
+	if (node == NULL)
+		return;
+
+	for_each_property_of_node(node, rprop) {
+
+		/* skip properties added automatically */
+		if (of_prop_cmp(rprop->name, "name") == 0 ||
+		    of_prop_cmp(rprop->name, "phandle") == 0 ||
+		    of_prop_cmp(rprop->name, "linux,phandle") == 0)
+			continue;
+
+		BUG_ON((rprop->length % 4) != 0 || rprop->length == 0);
+
+		count = rprop->length / sizeof(__be32);
+
+		/* now find the target property */
+		for_each_property_of_node(target, sprop) {
+			if (of_prop_cmp(sprop->name, rprop->name) == 0)
+				break;
+		}
+
+		BUG_ON(sprop == NULL);
+
+		for (i = 0; i < count; i++) {
+			off = be32_to_cpu(((__be32 *)rprop->value)[i]);
+			/* make sure the offset doesn't overstep (even wrap) */
+			BUG_ON(off >= sprop->length ||
+					(off + 4) > sprop->length);
+
+			/* adjust */
+			phandle = be32_to_cpu(*(__be32 *)(sprop->value + off));
+			phandle += phandle_delta;
+			*(__be32 *)(sprop->value + off) = cpu_to_be32(phandle);
+		}
+	}
+
+	for_each_child_of_node(node, child) {
+
+		for_each_child_of_node(target, childtarget)
+			if (__of_node_name_cmp(child, childtarget) == 0)
+				break;
+
+		BUG_ON(childtarget == NULL);
+
+		__of_adjust_tree_phandle_references(child, childtarget,
+				phandle_delta);
+	}
+}
+
 /**
  * of_resolve	- Resolve the given node against the live tree.
  *
@@ -241,7 +366,7 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
  */
 int of_resolve_phandles(struct device_node *resolve)
 {
-	struct device_node *child, *refnode;
+	struct device_node *child, *childroot, *refnode;
 	struct device_node *root_sym, *resolve_sym, *resolve_fix;
 	struct property *rprop;
 	const char *refpath;
@@ -255,9 +380,23 @@ int of_resolve_phandles(struct device_node *resolve)
 	/* first we need to adjust the phandles */
 	phandle_delta = of_get_tree_max_phandle() + 1;
 	__of_adjust_tree_phandles(resolve, phandle_delta);
-	err = __of_adjust_tree_phandle_references(resolve, phandle_delta);
-	if (err != 0)
-		return err;
+
+	/* locate the local fixups */
+	childroot = NULL;
+	for_each_child_of_node(resolve, childroot)
+		if (of_node_cmp(childroot->name, "__local_fixups__") == 0)
+			break;
+
+	if (childroot != NULL) {
+		/* resolve root is guaranteed to be the '/' */
+		err = __of_verify_tree_phandle_references(childroot,
+				resolve);
+		if (err != 0)
+			return err;
+
+		__of_adjust_tree_phandle_references(childroot,
+				resolve, phandle_delta);
+	}
 
 	root_sym = NULL;
 	resolve_sym = NULL;
@@ -322,7 +461,7 @@ int of_resolve_phandles(struct device_node *resolve)
 		pr_debug("%s: %s phandle is 0x%08x\n",
 				__func__, rprop->name, phandle);
 
-		err = __of_adjust_phandle_ref(resolve, rprop, phandle, false);
+		err = __of_adjust_phandle_ref(resolve, rprop, phandle);
 		if (err)
 			break;
 	}
-- 
1.7.12

[PATCH 2/5] of: testcases: Update with new local fixups format

[Pantelis Antoniou]

Update the selftests to using the new (and more readable) local
fixups format.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/testcase-data/testcases.dts | 61 +++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/drivers/of/testcase-data/testcases.dts b/drivers/of/testcase-data/testcases.dts
index 6994e15..b6bc41b 100644
--- a/drivers/of/testcase-data/testcases.dts
+++ b/drivers/of/testcase-data/testcases.dts
@@ -23,28 +23,41 @@
  * this a kernel-internal data format.
  */
 / { __local_fixups__ {
-	fixup = "/testcase-data/testcase-device2:interrupt-parent:0",
-		"/testcase-data/testcase-device1:interrupt-parent:0",
-		"/testcase-data/interrupts/interrupts-extended0:interrupts-extended:60",
-		"/testcase-data/interrupts/interrupts-extended0:interrupts-extended:52",
-		"/testcase-data/interrupts/interrupts-extended0:interrupts-extended:44",
-		"/testcase-data/interrupts/interrupts-extended0:interrupts-extended:36",
-		"/testcase-data/interrupts/interrupts-extended0:interrupts-extended:24",
-		"/testcase-data/interrupts/interrupts-extended0:interrupts-extended:8",
-		"/testcase-data/interrupts/interrupts-extended0:interrupts-extended:0",
-		"/testcase-data/interrupts/interrupts1:interrupt-parent:0",
-		"/testcase-data/interrupts/interrupts0:interrupt-parent:0",
-		"/testcase-data/interrupts/intmap1:interrupt-map:12",
-		"/testcase-data/interrupts/intmap0:interrupt-map:52",
-		"/testcase-data/interrupts/intmap0:interrupt-map:36",
-		"/testcase-data/interrupts/intmap0:interrupt-map:16",
-		"/testcase-data/interrupts/intmap0:interrupt-map:4",
-		"/testcase-data/phandle-tests/consumer-a:phandle-list-bad-args:12",
-		"/testcase-data/phandle-tests/consumer-a:phandle-list-bad-args:0",
-		"/testcase-data/phandle-tests/consumer-a:phandle-list:56",
-		"/testcase-data/phandle-tests/consumer-a:phandle-list:52",
-		"/testcase-data/phandle-tests/consumer-a:phandle-list:40",
-		"/testcase-data/phandle-tests/consumer-a:phandle-list:24",
-		"/testcase-data/phandle-tests/consumer-a:phandle-list:8",
-		"/testcase-data/phandle-tests/consumer-a:phandle-list:0";
+	testcase-data {
+		phandle-tests {
+			consumer-a {
+				phandle-list = <0x00000000 0x00000008
+						0x00000018 0x00000028
+						0x00000034 0x00000038>;
+				phandle-list-bad-args = <0x00000000 0x0000000c>;
+			};
+		};
+		interrupts {
+			intmap0 {
+				interrupt-map = <0x00000004 0x00000010
+						 0x00000024 0x00000034>;
+			};
+			intmap1 {
+				interrupt-map = <0x0000000c>;
+			};
+			interrupts0 {
+				interrupt-parent = <0x00000000>;
+			};
+			interrupts1 {
+				interrupt-parent = <0x00000000>;
+			};
+			interrupts-extended0 {
+				interrupts-extended = <0x00000000 0x00000008
+						       0x00000018 0x00000024
+						       0x0000002c 0x00000034
+						       0x0000003c>;
+			};
+		};
+		testcase-device1 {
+			interrupt-parent = <0x00000000>;
+		};
+		testcase-device2 {
+			interrupt-parent = <0x00000000>;
+		};
+	};
 }; };
-- 
1.7.12

[PATCH 3/5] of: Only call notifiers when node is attached

[Pantelis Antoniou]

Make sure we call notifier only when the node is attached.
When a detatched tree is being constructed we do not want the
notifiers to fire at all.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/base.c    | 9 ++++++---
 drivers/of/dynamic.c | 5 +----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 2305dc0..a79d4ee 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1695,7 +1695,8 @@ int of_add_property(struct device_node *np, struct property *prop)
 
 	mutex_unlock(&of_mutex);
 
-	if (!rc)
+	/* only call notifiers if the node is attached and no error occurred */
+	if (of_node_is_attached(np) && !rc)
 		of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop, NULL);
 
 	return rc;
@@ -1754,7 +1755,8 @@ int of_remove_property(struct device_node *np, struct property *prop)
 
 	mutex_unlock(&of_mutex);
 
-	if (!rc)
+	/* only call notifiers if the node is attached and no error occurred */
+	if (of_node_is_attached(np) && !rc)
 		of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop, NULL);
 
 	return rc;
@@ -1830,7 +1832,8 @@ int of_update_property(struct device_node *np, struct property *newprop)
 
 	mutex_unlock(&of_mutex);
 
-	if (!rc)
+	/* only call notifiers if the node is attached and no error occurred */
+	if (of_node_is_attached(np) && !rc)
 		of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop, oldprop);
 
 	return rc;
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index f297891..8e8b614 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -90,10 +90,6 @@ int of_property_notify(int action, struct device_node *np,
 {
 	struct of_prop_reconfig pr;
 
-	/* only call notifiers if the node is attached */
-	if (!of_node_is_attached(np))
-		return 0;
-
 	pr.dn = np;
 	pr.prop = prop;
 	pr.old_prop = oldprop;
@@ -138,6 +134,7 @@ int of_attach_node(struct device_node *np)
 	__of_attach_node_sysfs(np);
 	mutex_unlock(&of_mutex);
 
+	/* node is guaranteed to be attached at this point */
 	of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
 
 	return 0;
-- 
1.7.12

Re: [PATCH 3/5] of: Only call notifiers when node is attached

[Grant Likely]

On Tue, 28 Oct 2014 22:33:51 +0200
, Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
 wrote:
> Make sure we call notifier only when the node is attached.
> When a detatched tree is being constructed we do not want the
> notifiers to fire at all.

The description does not match what the patch does. The patch moves the
test into of_{add,remove,update}_property() and out of
of_property_notify() itself. That leaves one other caller of
of_property_notify(); __of_changeset_entry_notify(). The effect of this
patch is that applying a changeset will cause notifiers to be fired for
each property modified in a changeset. The comment says nothing about
the change in behaviour and it sounds like it is a bug fix when it
doesn't actually change the behaviour at all for the
of_{add,remove,update}_property() paths.

This needs a better changelog. It needs to describe what the effects of
the patch are and why the change is being made. When someone is
bisecting a problem and they land on this change, the changelog needs to
give them a good idea about what is going on and why.

g.

> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/of/base.c    | 9 ++++++---
>  drivers/of/dynamic.c | 5 +----
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 2305dc0..a79d4ee 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1695,7 +1695,8 @@ int of_add_property(struct device_node *np, struct property *prop)
>  
>  	mutex_unlock(&of_mutex);
>  
> -	if (!rc)
> +	/* only call notifiers if the node is attached and no error occurred */
> +	if (of_node_is_attached(np) && !rc)
>  		of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop, NULL);
>  
>  	return rc;
> @@ -1754,7 +1755,8 @@ int of_remove_property(struct device_node *np, struct property *prop)
>  
>  	mutex_unlock(&of_mutex);
>  
> -	if (!rc)
> +	/* only call notifiers if the node is attached and no error occurred */
> +	if (of_node_is_attached(np) && !rc)
>  		of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop, NULL);
>  
>  	return rc;
> @@ -1830,7 +1832,8 @@ int of_update_property(struct device_node *np, struct property *newprop)
>  
>  	mutex_unlock(&of_mutex);
>  
> -	if (!rc)
> +	/* only call notifiers if the node is attached and no error occurred */
> +	if (of_node_is_attached(np) && !rc)
>  		of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop, oldprop);
>  
>  	return rc;
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index f297891..8e8b614 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -90,10 +90,6 @@ int of_property_notify(int action, struct device_node *np,
>  {
>  	struct of_prop_reconfig pr;
>  
> -	/* only call notifiers if the node is attached */
> -	if (!of_node_is_attached(np))
> -		return 0;
> -
>  	pr.dn = np;
>  	pr.prop = prop;
>  	pr.old_prop = oldprop;
> @@ -138,6 +134,7 @@ int of_attach_node(struct device_node *np)
>  	__of_attach_node_sysfs(np);
>  	mutex_unlock(&of_mutex);
>  
> +	/* node is guaranteed to be attached at this point */
>  	of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
>  
>  	return 0;
> -- 
> 1.7.12
> 

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

Re: [PATCH 3/5] of: Only call notifiers when node is attached

[Pantelis Antoniou]

Hi Grant,

> On Nov 5, 2014, at 23:39 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> On Tue, 28 Oct 2014 22:33:51 +0200
> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> wrote:
>> Make sure we call notifier only when the node is attached.
>> When a detatched tree is being constructed we do not want the
>> notifiers to fire at all.
> 
> The description does not match what the patch does. The patch moves the
> test into of_{add,remove,update}_property() and out of
> of_property_notify() itself. That leaves one other caller of
> of_property_notify(); __of_changeset_entry_notify(). The effect of this
> patch is that applying a changeset will cause notifiers to be fired for
> each property modified in a changeset. The comment says nothing about
> the change in behaviour and it sounds like it is a bug fix when it
> doesn't actually change the behaviour at all for the
> of_{add,remove,update}_property() paths.
> 
> This needs a better changelog. It needs to describe what the effects of
> the patch are and why the change is being made. When someone is
> bisecting a problem and they land on this change, the changelog needs to
> give them a good idea about what is going on and why.
> 

Valid points. In fact I performed some tests and with this reverted things
still work.

The rationale behind this is for when nodes/properties are removed in the overlay,
but since we don’t support this for now, we never hit the case where it was
needed.

Please remove from the patch series, I’ll revisit this when I add the removal
functionality.

> g.
> 

Regards

— Pantelis

>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> ---
>> drivers/of/base.c    | 9 ++++++---
>> drivers/of/dynamic.c | 5 +----
>> 2 files changed, 7 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 2305dc0..a79d4ee 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -1695,7 +1695,8 @@ int of_add_property(struct device_node *np, struct property *prop)
>> 
>> 	mutex_unlock(&of_mutex);
>> 
>> -	if (!rc)
>> +	/* only call notifiers if the node is attached and no error occurred */
>> +	if (of_node_is_attached(np) && !rc)
>> 		of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop, NULL);
>> 
>> 	return rc;
>> @@ -1754,7 +1755,8 @@ int of_remove_property(struct device_node *np, struct property *prop)
>> 
>> 	mutex_unlock(&of_mutex);
>> 
>> -	if (!rc)
>> +	/* only call notifiers if the node is attached and no error occurred */
>> +	if (of_node_is_attached(np) && !rc)
>> 		of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop, NULL);
>> 
>> 	return rc;
>> @@ -1830,7 +1832,8 @@ int of_update_property(struct device_node *np, struct property *newprop)
>> 
>> 	mutex_unlock(&of_mutex);
>> 
>> -	if (!rc)
>> +	/* only call notifiers if the node is attached and no error occurred */
>> +	if (of_node_is_attached(np) && !rc)
>> 		of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop, oldprop);
>> 
>> 	return rc;
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index f297891..8e8b614 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -90,10 +90,6 @@ int of_property_notify(int action, struct device_node *np,
>> {
>> 	struct of_prop_reconfig pr;
>> 
>> -	/* only call notifiers if the node is attached */
>> -	if (!of_node_is_attached(np))
>> -		return 0;
>> -
>> 	pr.dn = np;
>> 	pr.prop = prop;
>> 	pr.old_prop = oldprop;
>> @@ -138,6 +134,7 @@ int of_attach_node(struct device_node *np)
>> 	__of_attach_node_sysfs(np);
>> 	mutex_unlock(&of_mutex);
>> 
>> +	/* node is guaranteed to be attached at this point */
>> 	of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
>> 
>> 	return 0;
>> -- 
>> 1.7.12
>> 
> 

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

Re: [PATCH 3/5] of: Only call notifiers when node is attached

[Grant Likely]

On Thu, Nov 6, 2014 at 12:33 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Grant,
>
>> On Nov 5, 2014, at 23:39 , Grant Likely <grant.likely@secretlab.ca> wrote:
>>
>> On Tue, 28 Oct 2014 22:33:51 +0200
>> , Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> wrote:
>>> Make sure we call notifier only when the node is attached.
>>> When a detatched tree is being constructed we do not want the
>>> notifiers to fire at all.
>>
>> The description does not match what the patch does. The patch moves the
>> test into of_{add,remove,update}_property() and out of
>> of_property_notify() itself. That leaves one other caller of
>> of_property_notify(); __of_changeset_entry_notify(). The effect of this
>> patch is that applying a changeset will cause notifiers to be fired for
>> each property modified in a changeset. The comment says nothing about
>> the change in behaviour and it sounds like it is a bug fix when it
>> doesn't actually change the behaviour at all for the
>> of_{add,remove,update}_property() paths.
>>
>> This needs a better changelog. It needs to describe what the effects of
>> the patch are and why the change is being made. When someone is
>> bisecting a problem and they land on this change, the changelog needs to
>> give them a good idea about what is going on and why.
>>
>
> Valid points. In fact I performed some tests and with this reverted things
> still work.
>
> The rationale behind this is for when nodes/properties are removed in the overlay,
> but since we don't support this for now, we never hit the case where it was
> needed.
>
> Please remove from the patch series, I'll revisit this when I add the removal
> functionality.

Cool, thanks.

g.

[PATCH 4/5] of: Add old prop argument on OF_RECONFIG_UPDATE_PROPERTY

[Pantelis Antoniou]

The notifier now includes the old_prop argument when updating
properties, propagate this API to changeset internals while
also retaining the old behaviour of retrieving the old_property
when NULL is passed.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 drivers/of/dynamic.c  | 18 ++++++++++++++----
 drivers/of/selftest.c |  2 +-
 include/linux/of.h    | 20 +++++++++++++-------
 3 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 8e8b614..9eb8528 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -627,6 +627,7 @@ int of_changeset_revert(struct of_changeset *ocs)
  * @action:	action to perform
  * @np:		Pointer to device node
  * @prop:	Pointer to property
+ * @old_prop:	Pointer to old property (if updating)
  *
  * On action being one of:
  * + OF_RECONFIG_ATTACH_NODE
@@ -637,10 +638,21 @@ int of_changeset_revert(struct of_changeset *ocs)
  * Returns 0 on success, a negative error value in case of an error.
  */
 int of_changeset_action(struct of_changeset *ocs, unsigned long action,
-		struct device_node *np, struct property *prop)
+		struct device_node *np, struct property *prop,
+		struct property *oldprop)
 {
 	struct of_changeset_entry *ce;
 
+	/* in case someone passed NULL as old_prop, find it */
+	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop && !oldprop) {
+		oldprop = of_find_property(np, prop->name, NULL);
+		if (!oldprop) {
+			pr_err("%s: Failed to find old_prop for \"%s/%s\"\n",
+					__func__, np->full_name, prop->name);
+			return -EINVAL;
+		}
+	}
+
 	ce = kzalloc(sizeof(*ce), GFP_KERNEL);
 	if (!ce) {
 		pr_err("%s: Failed to allocate\n", __func__);
@@ -650,9 +662,7 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
 	ce->action = action;
 	ce->np = of_node_get(np);
 	ce->prop = prop;
-
-	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop)
-		ce->old_prop = of_find_property(np, prop->name, NULL);
+	ce->old_prop = oldprop;
 
 	/* add it to the list */
 	list_add_tail(&ce->node, &ocs->entries);
diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
index 7800127..16102fd 100644
--- a/drivers/of/selftest.c
+++ b/drivers/of/selftest.c
@@ -427,7 +427,7 @@ static void __init of_selftest_changeset(void)
 	selftest(!of_changeset_detach_node(&chgset, nremove), "fail remove node\n");
 	selftest(!of_changeset_attach_node(&chgset, n21), "fail attach n21\n");
 	selftest(!of_changeset_add_property(&chgset, parent, ppadd), "fail add prop\n");
-	selftest(!of_changeset_update_property(&chgset, parent, ppupdate), "fail update prop\n");
+	selftest(!of_changeset_update_property(&chgset, parent, ppupdate, NULL), "fail update prop\n");
 	selftest(!of_changeset_remove_property(&chgset, parent, ppremove), "fail remove prop\n");
 	mutex_lock(&of_mutex);
 	selftest(!of_changeset_apply(&chgset), "apply failed\n");
diff --git a/include/linux/of.h b/include/linux/of.h
index 6545e7a..71d25aa 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -830,36 +830,42 @@ extern int of_changeset_apply(struct of_changeset *ocs);
 extern int of_changeset_revert(struct of_changeset *ocs);
 extern int of_changeset_action(struct of_changeset *ocs,
 		unsigned long action, struct device_node *np,
-		struct property *prop);
+		struct property *prop, struct property *old_prop);
 
 static inline int of_changeset_attach_node(struct of_changeset *ocs,
 		struct device_node *np)
 {
-	return of_changeset_action(ocs, OF_RECONFIG_ATTACH_NODE, np, NULL);
+	return of_changeset_action(ocs, OF_RECONFIG_ATTACH_NODE, np, NULL,
+			NULL);
 }
 
 static inline int of_changeset_detach_node(struct of_changeset *ocs,
 		struct device_node *np)
 {
-	return of_changeset_action(ocs, OF_RECONFIG_DETACH_NODE, np, NULL);
+	return of_changeset_action(ocs, OF_RECONFIG_DETACH_NODE, np, NULL,
+			NULL);
 }
 
 static inline int of_changeset_add_property(struct of_changeset *ocs,
 		struct device_node *np, struct property *prop)
 {
-	return of_changeset_action(ocs, OF_RECONFIG_ADD_PROPERTY, np, prop);
+	return of_changeset_action(ocs, OF_RECONFIG_ADD_PROPERTY, np, prop,
+			NULL);
 }
 
 static inline int of_changeset_remove_property(struct of_changeset *ocs,
 		struct device_node *np, struct property *prop)
 {
-	return of_changeset_action(ocs, OF_RECONFIG_REMOVE_PROPERTY, np, prop);
+	return of_changeset_action(ocs, OF_RECONFIG_REMOVE_PROPERTY, np, prop,
+			NULL);
 }
 
 static inline int of_changeset_update_property(struct of_changeset *ocs,
-		struct device_node *np, struct property *prop)
+		struct device_node *np, struct property *prop,
+		struct property *old_prop)
 {
-	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
+	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop,
+			old_prop);
 }
 #endif
 
-- 
1.7.12

Re: [PATCH 4/5] of: Add old prop argument on OF_RECONFIG_UPDATE_PROPERTY

[Grant Likely]

On Tue, 28 Oct 2014 22:33:52 +0200
, Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
 wrote:
> The notifier now includes the old_prop argument when updating
> properties, propagate this API to changeset internals while
> also retaining the old behaviour of retrieving the old_property
> when NULL is passed.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

I'm still fuzzy on the need for this patch. Is this just an optimization
so that the property doesn't get looked up twice? Or is there a reason
when the oldprop really needs to be passed in explicitly?

g.

> ---
>  drivers/of/dynamic.c  | 18 ++++++++++++++----
>  drivers/of/selftest.c |  2 +-
>  include/linux/of.h    | 20 +++++++++++++-------
>  3 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 8e8b614..9eb8528 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -627,6 +627,7 @@ int of_changeset_revert(struct of_changeset *ocs)
>   * @action:	action to perform
>   * @np:		Pointer to device node
>   * @prop:	Pointer to property
> + * @old_prop:	Pointer to old property (if updating)
>   *
>   * On action being one of:
>   * + OF_RECONFIG_ATTACH_NODE
> @@ -637,10 +638,21 @@ int of_changeset_revert(struct of_changeset *ocs)
>   * Returns 0 on success, a negative error value in case of an error.
>   */
>  int of_changeset_action(struct of_changeset *ocs, unsigned long action,
> -		struct device_node *np, struct property *prop)
> +		struct device_node *np, struct property *prop,
> +		struct property *oldprop)
>  {
>  	struct of_changeset_entry *ce;
>  
> +	/* in case someone passed NULL as old_prop, find it */
> +	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop && !oldprop) {
> +		oldprop = of_find_property(np, prop->name, NULL);
> +		if (!oldprop) {
> +			pr_err("%s: Failed to find old_prop for \"%s/%s\"\n",
> +					__func__, np->full_name, prop->name);
> +			return -EINVAL;
> +		}
> +	}
> +
>  	ce = kzalloc(sizeof(*ce), GFP_KERNEL);
>  	if (!ce) {
>  		pr_err("%s: Failed to allocate\n", __func__);
> @@ -650,9 +662,7 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
>  	ce->action = action;
>  	ce->np = of_node_get(np);
>  	ce->prop = prop;
> -
> -	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop)
> -		ce->old_prop = of_find_property(np, prop->name, NULL);
> +	ce->old_prop = oldprop;
>  
>  	/* add it to the list */
>  	list_add_tail(&ce->node, &ocs->entries);
> diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
> index 7800127..16102fd 100644
> --- a/drivers/of/selftest.c
> +++ b/drivers/of/selftest.c
> @@ -427,7 +427,7 @@ static void __init of_selftest_changeset(void)
>  	selftest(!of_changeset_detach_node(&chgset, nremove), "fail remove node\n");
>  	selftest(!of_changeset_attach_node(&chgset, n21), "fail attach n21\n");
>  	selftest(!of_changeset_add_property(&chgset, parent, ppadd), "fail add prop\n");
> -	selftest(!of_changeset_update_property(&chgset, parent, ppupdate), "fail update prop\n");
> +	selftest(!of_changeset_update_property(&chgset, parent, ppupdate, NULL), "fail update prop\n");
>  	selftest(!of_changeset_remove_property(&chgset, parent, ppremove), "fail remove prop\n");
>  	mutex_lock(&of_mutex);
>  	selftest(!of_changeset_apply(&chgset), "apply failed\n");
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 6545e7a..71d25aa 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -830,36 +830,42 @@ extern int of_changeset_apply(struct of_changeset *ocs);
>  extern int of_changeset_revert(struct of_changeset *ocs);
>  extern int of_changeset_action(struct of_changeset *ocs,
>  		unsigned long action, struct device_node *np,
> -		struct property *prop);
> +		struct property *prop, struct property *old_prop);
>  
>  static inline int of_changeset_attach_node(struct of_changeset *ocs,
>  		struct device_node *np)
>  {
> -	return of_changeset_action(ocs, OF_RECONFIG_ATTACH_NODE, np, NULL);
> +	return of_changeset_action(ocs, OF_RECONFIG_ATTACH_NODE, np, NULL,
> +			NULL);
>  }
>  
>  static inline int of_changeset_detach_node(struct of_changeset *ocs,
>  		struct device_node *np)
>  {
> -	return of_changeset_action(ocs, OF_RECONFIG_DETACH_NODE, np, NULL);
> +	return of_changeset_action(ocs, OF_RECONFIG_DETACH_NODE, np, NULL,
> +			NULL);
>  }
>  
>  static inline int of_changeset_add_property(struct of_changeset *ocs,
>  		struct device_node *np, struct property *prop)
>  {
> -	return of_changeset_action(ocs, OF_RECONFIG_ADD_PROPERTY, np, prop);
> +	return of_changeset_action(ocs, OF_RECONFIG_ADD_PROPERTY, np, prop,
> +			NULL);
>  }
>  
>  static inline int of_changeset_remove_property(struct of_changeset *ocs,
>  		struct device_node *np, struct property *prop)
>  {
> -	return of_changeset_action(ocs, OF_RECONFIG_REMOVE_PROPERTY, np, prop);
> +	return of_changeset_action(ocs, OF_RECONFIG_REMOVE_PROPERTY, np, prop,
> +			NULL);
>  }
>  
>  static inline int of_changeset_update_property(struct of_changeset *ocs,
> -		struct device_node *np, struct property *prop)
> +		struct device_node *np, struct property *prop,
> +		struct property *old_prop)
>  {
> -	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
> +	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop,
> +			old_prop);
>  }
>  #endif
>  
> -- 
> 1.7.12
> 

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

Re: [PATCH 4/5] of: Add old prop argument on OF_RECONFIG_UPDATE_PROPERTY

[Pantelis Antoniou]

Hi Grant,

> On Nov 5, 2014, at 22:01 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> On Tue, 28 Oct 2014 22:33:52 +0200
> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> wrote:
>> The notifier now includes the old_prop argument when updating
>> properties, propagate this API to changeset internals while
>> also retaining the old behaviour of retrieving the old_property
>> when NULL is passed.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> 
> I'm still fuzzy on the need for this patch. Is this just an optimization
> so that the property doesn't get looked up twice? Or is there a reason
> when the oldprop really needs to be passed in explicitly?
> 

In the case of overlay’s applying a property change the old property has
been already retrieved. Passing it as an argument saves us a traversal of the
property list.

On the original series were removal was supported, the old property was required
since you can’t find the old property anymore on the node (it was on another list). 

When we go back to revisit the removal case, that API is useful.

> g.
> 

Regards

— Pantelis

>> ---
>> drivers/of/dynamic.c  | 18 ++++++++++++++----
>> drivers/of/selftest.c |  2 +-
>> include/linux/of.h    | 20 +++++++++++++-------
>> 3 files changed, 28 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index 8e8b614..9eb8528 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -627,6 +627,7 @@ int of_changeset_revert(struct of_changeset *ocs)
>>  * @action:	action to perform
>>  * @np:		Pointer to device node
>>  * @prop:	Pointer to property
>> + * @old_prop:	Pointer to old property (if updating)
>>  *
>>  * On action being one of:
>>  * + OF_RECONFIG_ATTACH_NODE
>> @@ -637,10 +638,21 @@ int of_changeset_revert(struct of_changeset *ocs)
>>  * Returns 0 on success, a negative error value in case of an error.
>>  */
>> int of_changeset_action(struct of_changeset *ocs, unsigned long action,
>> -		struct device_node *np, struct property *prop)
>> +		struct device_node *np, struct property *prop,
>> +		struct property *oldprop)
>> {
>> 	struct of_changeset_entry *ce;
>> 
>> +	/* in case someone passed NULL as old_prop, find it */
>> +	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop && !oldprop) {
>> +		oldprop = of_find_property(np, prop->name, NULL);
>> +		if (!oldprop) {
>> +			pr_err("%s: Failed to find old_prop for \"%s/%s\"\n",
>> +					__func__, np->full_name, prop->name);
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> 	ce = kzalloc(sizeof(*ce), GFP_KERNEL);
>> 	if (!ce) {
>> 		pr_err("%s: Failed to allocate\n", __func__);
>> @@ -650,9 +662,7 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
>> 	ce->action = action;
>> 	ce->np = of_node_get(np);
>> 	ce->prop = prop;
>> -
>> -	if (action == OF_RECONFIG_UPDATE_PROPERTY && prop)
>> -		ce->old_prop = of_find_property(np, prop->name, NULL);
>> +	ce->old_prop = oldprop;
>> 
>> 	/* add it to the list */
>> 	list_add_tail(&ce->node, &ocs->entries);
>> diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
>> index 7800127..16102fd 100644
>> --- a/drivers/of/selftest.c
>> +++ b/drivers/of/selftest.c
>> @@ -427,7 +427,7 @@ static void __init of_selftest_changeset(void)
>> 	selftest(!of_changeset_detach_node(&chgset, nremove), "fail remove node\n");
>> 	selftest(!of_changeset_attach_node(&chgset, n21), "fail attach n21\n");
>> 	selftest(!of_changeset_add_property(&chgset, parent, ppadd), "fail add prop\n");
>> -	selftest(!of_changeset_update_property(&chgset, parent, ppupdate), "fail update prop\n");
>> +	selftest(!of_changeset_update_property(&chgset, parent, ppupdate, NULL), "fail update prop\n");
>> 	selftest(!of_changeset_remove_property(&chgset, parent, ppremove), "fail remove prop\n");
>> 	mutex_lock(&of_mutex);
>> 	selftest(!of_changeset_apply(&chgset), "apply failed\n");
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index 6545e7a..71d25aa 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -830,36 +830,42 @@ extern int of_changeset_apply(struct of_changeset *ocs);
>> extern int of_changeset_revert(struct of_changeset *ocs);
>> extern int of_changeset_action(struct of_changeset *ocs,
>> 		unsigned long action, struct device_node *np,
>> -		struct property *prop);
>> +		struct property *prop, struct property *old_prop);
>> 
>> static inline int of_changeset_attach_node(struct of_changeset *ocs,
>> 		struct device_node *np)
>> {
>> -	return of_changeset_action(ocs, OF_RECONFIG_ATTACH_NODE, np, NULL);
>> +	return of_changeset_action(ocs, OF_RECONFIG_ATTACH_NODE, np, NULL,
>> +			NULL);
>> }
>> 
>> static inline int of_changeset_detach_node(struct of_changeset *ocs,
>> 		struct device_node *np)
>> {
>> -	return of_changeset_action(ocs, OF_RECONFIG_DETACH_NODE, np, NULL);
>> +	return of_changeset_action(ocs, OF_RECONFIG_DETACH_NODE, np, NULL,
>> +			NULL);
>> }
>> 
>> static inline int of_changeset_add_property(struct of_changeset *ocs,
>> 		struct device_node *np, struct property *prop)
>> {
>> -	return of_changeset_action(ocs, OF_RECONFIG_ADD_PROPERTY, np, prop);
>> +	return of_changeset_action(ocs, OF_RECONFIG_ADD_PROPERTY, np, prop,
>> +			NULL);
>> }
>> 
>> static inline int of_changeset_remove_property(struct of_changeset *ocs,
>> 		struct device_node *np, struct property *prop)
>> {
>> -	return of_changeset_action(ocs, OF_RECONFIG_REMOVE_PROPERTY, np, prop);
>> +	return of_changeset_action(ocs, OF_RECONFIG_REMOVE_PROPERTY, np, prop,
>> +			NULL);
>> }
>> 
>> static inline int of_changeset_update_property(struct of_changeset *ocs,
>> -		struct device_node *np, struct property *prop)
>> +		struct device_node *np, struct property *prop,
>> +		struct property *old_prop)
>> {
>> -	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>> +	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop,
>> +			old_prop);
>> }
>> #endif
>> 
>> -- 
>> 1.7.12
>> 
> 

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

Re: [PATCH 4/5] of: Add old prop argument on OF_RECONFIG_UPDATE_PROPERTY

[Grant Likely]

On Wed, Nov 5, 2014 at 8:08 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Grant,
>
>> On Nov 5, 2014, at 22:01 , Grant Likely <grant.likely@secretlab.ca> wrote:
>>
>> On Tue, 28 Oct 2014 22:33:52 +0200
>> , Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> wrote:
>>> The notifier now includes the old_prop argument when updating
>>> properties, propagate this API to changeset internals while
>>> also retaining the old behaviour of retrieving the old_property
>>> when NULL is passed.
>>>
>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>>
>> I'm still fuzzy on the need for this patch. Is this just an optimization
>> so that the property doesn't get looked up twice? Or is there a reason
>> when the oldprop really needs to be passed in explicitly?
>>
>
> In the case of overlay's applying a property change the old property has
> been already retrieved. Passing it as an argument saves us a traversal of the
> property list.
>
> On the original series were removal was supported, the old property was required
> since you can't find the old property anymore on the node (it was on another list).
>
> When we go back to revisit the removal case, that API is useful.

Since we're not doing removal anymore, lets drop this patch.

g.

Re: [PATCH 4/5] of: Add old prop argument on OF_RECONFIG_UPDATE_PROPERTY

[Pantelis Antoniou]

Hi Grant,

> On Nov 6, 2014, at 14:46 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> On Wed, Nov 5, 2014 at 8:08 PM, Pantelis Antoniou
> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>> Hi Grant,
>> 
>>> On Nov 5, 2014, at 22:01 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>>> 
>>> On Tue, 28 Oct 2014 22:33:52 +0200
>>> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>> wrote:
>>>> The notifier now includes the old_prop argument when updating
>>>> properties, propagate this API to changeset internals while
>>>> also retaining the old behaviour of retrieving the old_property
>>>> when NULL is passed.
>>>> 
>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>> 
>>> I'm still fuzzy on the need for this patch. Is this just an optimization
>>> so that the property doesn't get looked up twice? Or is there a reason
>>> when the oldprop really needs to be passed in explicitly?
>>> 
>> 
>> In the case of overlay's applying a property change the old property has
>> been already retrieved. Passing it as an argument saves us a traversal of the
>> property list.
>> 
>> On the original series were removal was supported, the old property was required
>> since you can't find the old property anymore on the node (it was on another list).
>> 
>> When we go back to revisit the removal case, that API is useful.
> 
> Since we're not doing removal anymore, lets drop this patch.
> 

Fine, I’ll have to rework the overlay patches however.

> g.

Regards

— Pantelis

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

[PATCH 5/5] of: of_reconfig_get_state_change() of notifier helper.

[Pantelis Antoniou]

Introduce of_reconfig_get_state_change() which allows an of notifier
to query about device state changes.

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

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 9eb8528..303a59b 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -85,6 +85,102 @@ int of_reconfig_notify(unsigned long action, void *p)
 	return notifier_to_errno(rc);
 }
 
+/*
+ * of_reconfig_get_state_change()	- Returns new state of device
+ * @action	- action of the of notifier
+ * @arg		- argument of the of notifier
+ *
+ * Returns the new state of a device based on the notifier used.
+ * Returns 0 on device going from enabled to disabled, 1 on device
+ * going from disabled to enabled and -1 on no change.
+ */
+int of_reconfig_get_state_change(unsigned long action, void *arg)
+{
+	struct device_node *dn;
+	struct property *prop, *old_prop;
+	struct of_prop_reconfig *pr;
+	int is_status, status_state, old_status_state, prev_state, new_state;
+
+	/* figure out if a device should be created or destroyed */
+	dn = NULL;
+	prop = old_prop = NULL;
+	switch (action) {
+	case OF_RECONFIG_ATTACH_NODE:
+	case OF_RECONFIG_DETACH_NODE:
+		dn = arg;
+		prop = of_find_property(dn, "status", NULL);
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+	case OF_RECONFIG_REMOVE_PROPERTY:
+		pr = arg;
+		dn = pr->dn;
+		prop = pr->prop;
+		break;
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		pr = arg;
+		dn = pr->dn;
+		prop = pr->prop;
+		old_prop = pr->old_prop;
+		break;
+	default:
+		return -1;	/* don't care */
+	}
+
+	is_status = 0;
+	status_state = -1;
+	old_status_state = -1;
+	prev_state = -1;
+	new_state = -1;
+
+	if (prop && !strcmp(prop->name, "status")) {
+		is_status = 1;
+		status_state = !strcmp(prop->value, "okay") ||
+			       !strcmp(prop->value, "ok");
+		if (old_prop)
+			old_status_state = !strcmp(old_prop->value, "okay") ||
+					   !strcmp(old_prop->value, "ok");
+	}
+
+	switch (action) {
+	case OF_RECONFIG_ATTACH_NODE:
+		prev_state = 0;
+		/* -1 & 0 status either missing or okay */
+		new_state = status_state != 0;
+		break;
+	case OF_RECONFIG_DETACH_NODE:
+		/* -1 & 0 status either missing or okay */
+		prev_state = status_state != 0;
+		new_state = 0;
+		break;
+	case OF_RECONFIG_ADD_PROPERTY:
+		if (is_status) {
+			/* no status property -> enabled (legacy) */
+			prev_state = 1;
+			new_state = status_state;
+		}
+		break;
+	case OF_RECONFIG_REMOVE_PROPERTY:
+		if (is_status) {
+			prev_state = status_state;
+			/* no status property -> enabled (legacy) */
+			new_state = 1;
+		}
+		break;
+	case OF_RECONFIG_UPDATE_PROPERTY:
+		if (is_status) {
+			prev_state = old_status_state != 0;
+			new_state = status_state != 0;
+		}
+		break;
+	}
+
+	if (prev_state == new_state)
+		return -1;
+
+	return new_state;
+}
+EXPORT_SYMBOL_GPL(of_reconfig_get_state_change);
+
 int of_property_notify(int action, struct device_node *np,
 		       struct property *prop, struct property *oldprop)
 {
diff --git a/include/linux/of.h b/include/linux/of.h
index 71d25aa..9ff1ec5 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -328,6 +328,7 @@ struct of_prop_reconfig {
 extern int of_reconfig_notifier_register(struct notifier_block *);
 extern int of_reconfig_notifier_unregister(struct notifier_block *);
 extern int of_reconfig_notify(unsigned long, void *);
+extern int of_reconfig_get_state_change(unsigned long action, void *arg);
 
 extern int of_attach_node(struct device_node *);
 extern int of_detach_node(struct device_node *);
-- 
1.7.12

Re: [PATCH 5/5] of: of_reconfig_get_state_change() of notifier helper.

[Grant Likely]

On Tue, 28 Oct 2014 22:33:53 +0200
, Pantelis Antoniou <pantelis.antoniou@konsulko.com>
 wrote:
> Introduce of_reconfig_get_state_change() which allows an of notifier
> to query about device state changes.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

Applied, thanks.

g.

> ---
>  drivers/of/dynamic.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h   |  1 +
>  2 files changed, 97 insertions(+)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 9eb8528..303a59b 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -85,6 +85,102 @@ int of_reconfig_notify(unsigned long action, void *p)
>  	return notifier_to_errno(rc);
>  }
>  
> +/*
> + * of_reconfig_get_state_change()	- Returns new state of device
> + * @action	- action of the of notifier
> + * @arg		- argument of the of notifier
> + *
> + * Returns the new state of a device based on the notifier used.
> + * Returns 0 on device going from enabled to disabled, 1 on device
> + * going from disabled to enabled and -1 on no change.
> + */
> +int of_reconfig_get_state_change(unsigned long action, void *arg)
> +{
> +	struct device_node *dn;
> +	struct property *prop, *old_prop;
> +	struct of_prop_reconfig *pr;
> +	int is_status, status_state, old_status_state, prev_state, new_state;
> +
> +	/* figure out if a device should be created or destroyed */
> +	dn = NULL;
> +	prop = old_prop = NULL;
> +	switch (action) {
> +	case OF_RECONFIG_ATTACH_NODE:
> +	case OF_RECONFIG_DETACH_NODE:
> +		dn = arg;
> +		prop = of_find_property(dn, "status", NULL);
> +		break;
> +	case OF_RECONFIG_ADD_PROPERTY:
> +	case OF_RECONFIG_REMOVE_PROPERTY:
> +		pr = arg;
> +		dn = pr->dn;
> +		prop = pr->prop;
> +		break;
> +	case OF_RECONFIG_UPDATE_PROPERTY:
> +		pr = arg;
> +		dn = pr->dn;
> +		prop = pr->prop;
> +		old_prop = pr->old_prop;
> +		break;
> +	default:
> +		return -1;	/* don't care */
> +	}
> +
> +	is_status = 0;
> +	status_state = -1;
> +	old_status_state = -1;
> +	prev_state = -1;
> +	new_state = -1;
> +
> +	if (prop && !strcmp(prop->name, "status")) {
> +		is_status = 1;
> +		status_state = !strcmp(prop->value, "okay") ||
> +			       !strcmp(prop->value, "ok");
> +		if (old_prop)
> +			old_status_state = !strcmp(old_prop->value, "okay") ||
> +					   !strcmp(old_prop->value, "ok");
> +	}
> +
> +	switch (action) {
> +	case OF_RECONFIG_ATTACH_NODE:
> +		prev_state = 0;
> +		/* -1 & 0 status either missing or okay */
> +		new_state = status_state != 0;
> +		break;
> +	case OF_RECONFIG_DETACH_NODE:
> +		/* -1 & 0 status either missing or okay */
> +		prev_state = status_state != 0;
> +		new_state = 0;
> +		break;
> +	case OF_RECONFIG_ADD_PROPERTY:
> +		if (is_status) {
> +			/* no status property -> enabled (legacy) */
> +			prev_state = 1;
> +			new_state = status_state;
> +		}
> +		break;
> +	case OF_RECONFIG_REMOVE_PROPERTY:
> +		if (is_status) {
> +			prev_state = status_state;
> +			/* no status property -> enabled (legacy) */
> +			new_state = 1;
> +		}
> +		break;
> +	case OF_RECONFIG_UPDATE_PROPERTY:
> +		if (is_status) {
> +			prev_state = old_status_state != 0;
> +			new_state = status_state != 0;
> +		}
> +		break;
> +	}
> +
> +	if (prev_state == new_state)
> +		return -1;
> +
> +	return new_state;
> +}
> +EXPORT_SYMBOL_GPL(of_reconfig_get_state_change);
> +
>  int of_property_notify(int action, struct device_node *np,
>  		       struct property *prop, struct property *oldprop)
>  {
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 71d25aa..9ff1ec5 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -328,6 +328,7 @@ struct of_prop_reconfig {
>  extern int of_reconfig_notifier_register(struct notifier_block *);
>  extern int of_reconfig_notifier_unregister(struct notifier_block *);
>  extern int of_reconfig_notify(unsigned long, void *);
> +extern int of_reconfig_get_state_change(unsigned long action, void *arg);
>  
>  extern int of_attach_node(struct device_node *);
>  extern int of_detach_node(struct device_node *);
> -- 
> 1.7.12
>