[PATCH V5 1/6] of: move documentation of of_parse_phandle_with_args

[PATCH V5 1/6] of: move documentation of of_parse_phandle_with_args

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Commit bd69f73 "of: Create function for counting number of phandles in
a property" renamed of_parse_phandle_with_args(), and created a wrapper
function that implemented the original name. However, the documentation
of the original function was not moved, leaving it apparently documenting
the newly renamed function.

Move the documentation so that it is adjacent to the function it
documents.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
 drivers/of/base.c | 64 +++++++++++++++++++++++++++----------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5c54279..23e7073 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1104,38 +1104,6 @@ struct device_node *of_parse_phandle(const struct device_node *np,
 }
 EXPORT_SYMBOL(of_parse_phandle);
 
-/**
- * of_parse_phandle_with_args() - Find a node pointed by phandle in a list
- * @np:		pointer to a device tree node containing a list
- * @list_name:	property name that contains a list
- * @cells_name:	property name that specifies phandles' arguments count
- * @index:	index of a phandle to parse out
- * @out_args:	optional pointer to output arguments structure (will be filled)
- *
- * This function is useful to parse lists of phandles and their arguments.
- * Returns 0 on success and fills out_args, on error returns appropriate
- * errno value.
- *
- * Caller is responsible to call of_node_put() on the returned out_args->node
- * pointer.
- *
- * Example:
- *
- * phandle1: node1 {
- * 	#list-cells = <2>;
- * }
- *
- * phandle2: node2 {
- * 	#list-cells = <1>;
- * }
- *
- * node3 {
- * 	list = <&phandle1 1 2 &phandle2 3>;
- * }
- *
- * To get a device_node of the `node2' node you may call this:
- * of_parse_phandle_with_args(node3, "list", "#list-cells", 1, &args);
- */
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name, int index,
@@ -1238,6 +1206,38 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 	return rc;
 }
 
+/**
+ * of_parse_phandle_with_args() - Find a node pointed by phandle in a list
+ * @np:		pointer to a device tree node containing a list
+ * @list_name:	property name that contains a list
+ * @cells_name:	property name that specifies phandles' arguments count
+ * @index:	index of a phandle to parse out
+ * @out_args:	optional pointer to output arguments structure (will be filled)
+ *
+ * This function is useful to parse lists of phandles and their arguments.
+ * Returns 0 on success and fills out_args, on error returns appropriate
+ * errno value.
+ *
+ * Caller is responsible to call of_node_put() on the returned out_args->node
+ * pointer.
+ *
+ * Example:
+ *
+ * phandle1: node1 {
+ * 	#list-cells = <2>;
+ * }
+ *
+ * phandle2: node2 {
+ * 	#list-cells = <1>;
+ * }
+ *
+ * node3 {
+ * 	list = <&phandle1 1 2 &phandle2 3>;
+ * }
+ *
+ * To get a device_node of the `node2' node you may call this:
+ * of_parse_phandle_with_args(node3, "list", "#list-cells", 1, &args);
+ */
 int of_parse_phandle_with_args(const struct device_node *np, const char *list_name,
 				const char *cells_name, int index,
 				struct of_phandle_args *out_args)
-- 
1.8.1.5

[PATCH V5 2/6] of: move of_parse_phandle()

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Move of_parse_phandle() after __of_parse_phandle_with_args(), since a
future patch will call __of_parse_phandle_with_args() from
of_parse_phandle(). Moving the function avoids adding a prototype. Doing
the move separately highlights the code changes separately.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v5: New patch.
---
 drivers/of/base.c | 48 ++++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 23e7073..b9e77ec 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1080,30 +1080,6 @@ int of_property_count_strings(struct device_node *np, const char *propname)
 }
 EXPORT_SYMBOL_GPL(of_property_count_strings);
 
-/**
- * of_parse_phandle - Resolve a phandle property to a device_node pointer
- * @np: Pointer to device node holding phandle property
- * @phandle_name: Name of property holding a phandle value
- * @index: For properties holding a table of phandles, this is the index into
- *         the table
- *
- * Returns the device_node pointer with refcount incremented.  Use
- * of_node_put() on it when done.
- */
-struct device_node *of_parse_phandle(const struct device_node *np,
-				     const char *phandle_name, int index)
-{
-	const __be32 *phandle;
-	int size;
-
-	phandle = of_get_property(np, phandle_name, &size);
-	if ((!phandle) || (size < sizeof(*phandle) * (index + 1)))
-		return NULL;
-
-	return of_find_node_by_phandle(be32_to_cpup(phandle + index));
-}
-EXPORT_SYMBOL(of_parse_phandle);
-
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name, int index,
@@ -1207,6 +1183,30 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 }
 
 /**
+ * of_parse_phandle - Resolve a phandle property to a device_node pointer
+ * @np: Pointer to device node holding phandle property
+ * @phandle_name: Name of property holding a phandle value
+ * @index: For properties holding a table of phandles, this is the index into
+ *         the table
+ *
+ * Returns the device_node pointer with refcount incremented.  Use
+ * of_node_put() on it when done.
+ */
+struct device_node *of_parse_phandle(const struct device_node *np,
+				     const char *phandle_name, int index)
+{
+	const __be32 *phandle;
+	int size;
+
+	phandle = of_get_property(np, phandle_name, &size);
+	if ((!phandle) || (size < sizeof(*phandle) * (index + 1)))
+		return NULL;
+
+	return of_find_node_by_phandle(be32_to_cpup(phandle + index));
+}
+EXPORT_SYMBOL(of_parse_phandle);
+
+/**
  * of_parse_phandle_with_args() - Find a node pointed by phandle in a list
  * @np:		pointer to a device tree node containing a list
  * @list_name:	property name that contains a list
-- 
1.8.1.5

[PATCH V5 3/6] of: introduce of_parse_phandle_with_fixed_args

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This is identical to of_parse_phandle_with_args(), except that the
number of argument cells is fixed, rather than being parsed out of the
node referenced by each phandle.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
v3: s/cells_count/cell_count, add missing braces per coding style
---
 drivers/of/base.c  | 66 +++++++++++++++++++++++++++++++++++++++++++++++-------
 include/linux/of.h | 10 +++++++++
 2 files changed, 68 insertions(+), 8 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index b9e77ec..2f92522 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1082,7 +1082,8 @@ EXPORT_SYMBOL_GPL(of_property_count_strings);
 
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
-					const char *cells_name, int index,
+					const char *cells_name,
+					int cell_count, int index,
 					struct of_phandle_args *out_args)
 {
 	const __be32 *list, *list_end;
@@ -1118,11 +1119,17 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 					 np->full_name);
 				goto err;
 			}
-			if (of_property_read_u32(node, cells_name, &count)) {
-				pr_err("%s: could not get %s for %s\n",
-					 np->full_name, cells_name,
-					 node->full_name);
-				goto err;
+
+			if (cells_name) {
+				if (of_property_read_u32(node, cells_name,
+							 &count)) {
+					pr_err("%s: could not get %s for %s\n",
+						np->full_name, cells_name,
+						node->full_name);
+					goto err;
+				}
+			} else {
+				count = cell_count;
 			}
 
 			/*
@@ -1244,11 +1251,53 @@ int of_parse_phandle_with_args(const struct device_node *np, const char *list_na
 {
 	if (index < 0)
 		return -EINVAL;
-	return __of_parse_phandle_with_args(np, list_name, cells_name, index, out_args);
+	return __of_parse_phandle_with_args(np, list_name, cells_name, 0,
+					    index, out_args);
 }
 EXPORT_SYMBOL(of_parse_phandle_with_args);
 
 /**
+ * of_parse_phandle_with_fixed_args() - Find a node pointed by phandle in a list
+ * @np:		pointer to a device tree node containing a list
+ * @list_name:	property name that contains a list
+ * @cell_count: number of argument cells following the phandle
+ * @index:	index of a phandle to parse out
+ * @out_args:	optional pointer to output arguments structure (will be filled)
+ *
+ * This function is useful to parse lists of phandles and their arguments.
+ * Returns 0 on success and fills out_args, on error returns appropriate
+ * errno value.
+ *
+ * Caller is responsible to call of_node_put() on the returned out_args->node
+ * pointer.
+ *
+ * Example:
+ *
+ * phandle1: node1 {
+ * }
+ *
+ * phandle2: node2 {
+ * }
+ *
+ * node3 {
+ * 	list = <&phandle1 0 2 &phandle2 2 3>;
+ * }
+ *
+ * To get a device_node of the `node2' node you may call this:
+ * of_parse_phandle_with_fixed_args(node3, "list", 2, 1, &args);
+ */
+int of_parse_phandle_with_fixed_args(const struct device_node *np,
+				const char *list_name, int cell_count,
+				int index, struct of_phandle_args *out_args)
+{
+	if (index < 0)
+		return -EINVAL;
+	return __of_parse_phandle_with_args(np, list_name, NULL, cell_count,
+					   index, out_args);
+}
+EXPORT_SYMBOL(of_parse_phandle_with_fixed_args);
+
+/**
  * of_count_phandle_with_args() - Find the number of phandles references in a property
  * @np:		pointer to a device tree node containing a list
  * @list_name:	property name that contains a list
@@ -1266,7 +1315,8 @@ EXPORT_SYMBOL(of_parse_phandle_with_args);
 int of_count_phandle_with_args(const struct device_node *np, const char *list_name,
 				const char *cells_name)
 {
-	return __of_parse_phandle_with_args(np, list_name, cells_name, -1, NULL);
+	return __of_parse_phandle_with_args(np, list_name, cells_name, 0, -1,
+					    NULL);
 }
 EXPORT_SYMBOL(of_count_phandle_with_args);
 
diff --git a/include/linux/of.h b/include/linux/of.h
index 90a8811..87d0830 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -280,6 +280,9 @@ extern struct device_node *of_parse_phandle(const struct device_node *np,
 extern int of_parse_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name, int index,
 	struct of_phandle_args *out_args);
+extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
+	const char *list_name, int cells_count, int index,
+	struct of_phandle_args *out_args);
 extern int of_count_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name);
 
@@ -477,6 +480,13 @@ static inline int of_parse_phandle_with_args(struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline int of_parse_phandle_with_fixed_args(const struct device_node *np,
+	const char *list_name, int cells_count, int index,
+	struct of_phandle_args *out_args)
+{
+	return -ENOSYS;
+}
+
 static inline int of_count_phandle_with_args(struct device_node *np,
 					     const char *list_name,
 					     const char *cells_name)
-- 
1.8.1.5

[PATCH V5 4/6] of: call __of_parse_phandle_with_args from of_parse_phandle

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

The simplest case of __of_parse_phandle_with_args() implements
of_parse_phandle(), except that it doesn't return the node referenced by
the phandle. Modify it to do so, and then rewrite of_parse_phandle() to
call __of_parse_phandle_with_args() rather than open-coding the simple
case.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v5: New patch.
---
 drivers/of/base.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 2f92522..79ed82c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1202,14 +1202,16 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
 struct device_node *of_parse_phandle(const struct device_node *np,
 				     const char *phandle_name, int index)
 {
-	const __be32 *phandle;
-	int size;
+	struct of_phandle_args args;
 
-	phandle = of_get_property(np, phandle_name, &size);
-	if ((!phandle) || (size < sizeof(*phandle) * (index + 1)))
+	if (index < 0)
+		return NULL;
+
+	if (__of_parse_phandle_with_args(np, phandle_name, NULL, 0,
+					 index, &args))
 		return NULL;
 
-	return of_find_node_by_phandle(be32_to_cpup(phandle + index));
+	return args.np;
 }
 EXPORT_SYMBOL(of_parse_phandle);
 
-- 
1.8.1.5

[PATCH V5 5/6] gpio: clean up gpio-ranges documentation

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This change makes documentation of the the gpio-ranges property shorter
and more succinct, more consistent with the style of the rest of the
document, and not mention Linux-specifics such as the API
pinctrl_request_gpio(); DT binding documents should be OS independant
where at all possible. As part of this, the gpio-ranges property's format
is described in BNF form, in order to match the rest of the document.

This change also deprecates the #gpio-range-cells property. Such
properties are useful when one node references a second node, and that
second node dictates the format of the reference. However, that is not
the case here; the definition of gpio-ranges itself always dictates its
format entirely, and hence the value #gpio-range-cells must always be 3,
and hence there is no point requiring any referenced node to include
this property. The only remaining need for this property is to ensure
compatibility of DTs with older SW that was written to support the
previous version of the binding.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
v5:
* Re-ordered patches so all of patches first, then gpio patches after.
v4:
* Mention #gpio-range-cells as being deprecated, rather than removing all
  documentation of that property. This allows DTs to be written in a
  backwards-compatible way if desired, and also allows older DTs to be
  interpreted fully using the latest documentation.
v3:
* Mention BNF in commit description.
* Fixed typo.
* Dropped patch that removed the deprecated property from *.dts, since
  it's required to boot older kernels.

Patches 2-4 need to be applied in the same branch, since they all build
upon each-other.
---
 Documentation/devicetree/bindings/gpio/gpio.txt | 55 ++++++++++++++-----------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index d933af3..6cec6ff 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -75,23 +75,36 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
 		gpio-controller;
 	};
 
-2.1) gpio-controller and pinctrl subsystem
-------------------------------------------
+2.1) gpio- and pin-controller interaction
+-----------------------------------------
 
-gpio-controller on a SOC might be tightly coupled with the pinctrl
-subsystem, in the sense that the pins can be used by other functions
-together with optional gpio feature.
+Some or all of the GPIOs provided by a GPIO controller may be routed to pins
+on the package via a pin controller. This allows muxing those pins between
+GPIO and other functions.
 
-While the pin allocation is totally managed by the pin ctrl subsystem,
-gpio (under gpiolib) is still maintained by gpio drivers. It may happen
-that different pin ranges in a SoC is managed by different gpio drivers.
+It is useful to represent which GPIOs correspond to which pins on which pin
+controllers. The gpio-ranges property described below represents this, and
+contains information structures as follows:
 
-This makes it logical to let gpio drivers announce their pin ranges to
-the pin ctrl subsystem and call 'pinctrl_request_gpio' in order to
-request the corresponding pin before any gpio usage.
+	gpio-range-list ::= <single-gpio-range> [gpio-range-list]
+	single-gpio-range ::=
+			<pinctrl-phandle> <gpio-base> <pinctrl-base> <count>
+	gpio-phandle : phandle to pin controller node.
+	gpio-base : Base GPIO ID in the GPIO controller
+	pinctrl-base : Base pinctrl pin ID in the pin controller
+	count : The number of GPIOs/pins in this range
 
-For this, the gpio controller can use a pinctrl phandle and pins to
-announce the pinrange to the pin ctrl subsystem. For example,
+The "pin controller node" mentioned above must conform to the bindings
+described in ../pinctrl/pinctrl-bindings.txt.
+
+Previous versions of this binding required all pin controller nodes that
+were referenced by any gpio-ranges property to contain a property named
+#gpio-range-cells with value <3>. This requirement is now deprecated.
+However, that property may still exist in older device trees for
+compatibility reasons, and would still be required even in new device
+trees that need to be compatible with older software.
+
+Example:
 
 	qe_pio_e: gpio-controller@1460 {
 		#gpio-cells = <2>;
@@ -99,16 +112,8 @@ announce the pinrange to the pin ctrl subsystem. For example,
 		reg = <0x1460 0x18>;
 		gpio-controller;
 		gpio-ranges = <&pinctrl1 0 20 10>, <&pinctrl2 10 50 20>;
+	};
 
-    }
-
-where,
-   &pinctrl1 and &pinctrl2 is the phandle to the pinctrl DT node.
-
-   Next values specify the base pin and number of pins for the range
-   handled by 'qe_pio_e' gpio. In the given example from base pin 20 to
-   pin 29 under pinctrl1 with gpio offset 0 and pin 50 to pin 69 under
-   pinctrl2 with gpio offset 10 is handled by this gpio controller.
-
-The pinctrl node must have "#gpio-range-cells" property to show number of
-arguments to pass with phandle from gpio controllers node.
+Here, a single GPIO controller has GPIOs 0..9 routed to pin controller
+pinctrl1's pins 20..29, and GPIOs 10..19 routed to pin controller pinctrl2's
+pins 50..59.
-- 
1.8.1.5

[PATCH V5 6/6] gpio: implement gpio-ranges binding document fix

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Use the new of_parse_phandle_with_fixed_args() to implement the
corrected gpio-ranges DT property definition.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
 drivers/gpio/gpiolib-of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 665f953..48cda3c 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -194,8 +194,8 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip)
 		return;
 
 	for (;; index++) {
-		ret = of_parse_phandle_with_args(np, "gpio-ranges",
-				"#gpio-range-cells", index, &pinspec);
+		ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
+				index, &pinspec);
 		if (ret)
 			break;
 
-- 
1.8.1.5

Re: [PATCH V5 4/6] of: call __of_parse_phandle_with_args from of_parse_phandle

[Mark Rutland]

On Mon, Aug 12, 2013 at 06:36:30PM +0100, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> The simplest case of __of_parse_phandle_with_args() implements
> of_parse_phandle(), except that it doesn't return the node referenced by
> the phandle. Modify it to do so, and then rewrite of_parse_phandle() to
> call __of_parse_phandle_with_args() rather than open-coding the simple
> case.

That commit message doesn't seem to match the patch (which doesn't
modify __of_parse_phandle_with_args).

Rather, now that __of_parse_phandle_with_args can handle parsing with a
fixed number of argument cells, it's possible to write of_parse_phandle
in terms of it.

What's the overhead over the old of_parse_phandle? It looks like this is
going to do a lot of pointless work beyond what it already does --
parsing each prior entry in the list, and for each prior entry walking
the tree in of_find_node_by_phandle. Maybe we don't use long enough
phandle lists anywhere for that to be noticeable.

Thanks,
Mark.

> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v5: New patch.
> ---
>  drivers/of/base.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 2f92522..79ed82c 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1202,14 +1202,16 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
>  struct device_node *of_parse_phandle(const struct device_node *np,
>  				     const char *phandle_name, int index)
>  {
> -	const __be32 *phandle;
> -	int size;
> +	struct of_phandle_args args;
>  
> -	phandle = of_get_property(np, phandle_name, &size);
> -	if ((!phandle) || (size < sizeof(*phandle) * (index + 1)))
> +	if (index < 0)
> +		return NULL;
> +
> +	if (__of_parse_phandle_with_args(np, phandle_name, NULL, 0,
> +					 index, &args))
>  		return NULL;
>  
> -	return of_find_node_by_phandle(be32_to_cpup(phandle + index));
> +	return args.np;
>  }
>  EXPORT_SYMBOL(of_parse_phandle);
>  
> -- 
> 1.8.1.5
> 
> 

Re: [PATCH V5 4/6] of: call __of_parse_phandle_with_args from of_parse_phandle

[Stephen Warren]

On 08/13/2013 03:08 AM, Mark Rutland wrote:
> On Mon, Aug 12, 2013 at 06:36:30PM +0100, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> The simplest case of __of_parse_phandle_with_args() implements
>> of_parse_phandle(), except that it doesn't return the node referenced by
>> the phandle. Modify it to do so, and then rewrite of_parse_phandle() to
>> call __of_parse_phandle_with_args() rather than open-coding the simple
>> case.
> 
> That commit message doesn't seem to match the patch (which doesn't
> modify __of_parse_phandle_with_args).
> 
> Rather, now that __of_parse_phandle_with_args can handle parsing with a
> fixed number of argument cells, it's possible to write of_parse_phandle
> in terms of it.

True. I originally hadn't realized that __of_parse_phandle_with_args()
does already return the node and so started to add that feature, then
forgot to re-write the commit description. How about:

-----
of: call __of_parse_phandle_with_args from of_parse_phandle

The simplest case of __of_parse_phandle_with_args() now implements the
semantics of of_parse_phandle(). Rewrite of_parse_phandle() to call
__of_parse_phandle_with_args() rather than open-coding the simple case.
-----

> What's the overhead over the old of_parse_phandle? It looks like this is
> going to do a lot of pointless work beyond what it already does --
> parsing each prior entry in the list, and for each prior entry walking
> the tree in of_find_node_by_phandle. Maybe we don't use long enough
> phandle lists anywhere for that to be noticeable.

I think the overhead is pretty minimal. The main difference is that the
new code will loop over the property cell by cell rather than directly
jump into the required index. That's not likely to be much work for
typical properties. In particular, no extra DT property lookups are
performed, since of_parse_phandle() passes in cells_name=NULL,
cell_count=0, so the cells_name property is not looked up.

Besides, Grant told me to do this change:-)

Re: [PATCH V5 4/6] of: call __of_parse_phandle_with_args from of_parse_phandle

[Mark Rutland]

On Tue, Aug 13, 2013 at 04:54:02PM +0100, Stephen Warren wrote:
> On 08/13/2013 03:08 AM, Mark Rutland wrote:
> > On Mon, Aug 12, 2013 at 06:36:30PM +0100, Stephen Warren wrote:
> >> From: Stephen Warren <swarren@nvidia.com>
> >>
> >> The simplest case of __of_parse_phandle_with_args() implements
> >> of_parse_phandle(), except that it doesn't return the node referenced by
> >> the phandle. Modify it to do so, and then rewrite of_parse_phandle() to
> >> call __of_parse_phandle_with_args() rather than open-coding the simple
> >> case.
> > 
> > That commit message doesn't seem to match the patch (which doesn't
> > modify __of_parse_phandle_with_args).
> > 
> > Rather, now that __of_parse_phandle_with_args can handle parsing with a
> > fixed number of argument cells, it's possible to write of_parse_phandle
> > in terms of it.
> 
> True. I originally hadn't realized that __of_parse_phandle_with_args()
> does already return the node and so started to add that feature, then
> forgot to re-write the commit description. How about:
> 
> -----
> of: call __of_parse_phandle_with_args from of_parse_phandle
> 
> The simplest case of __of_parse_phandle_with_args() now implements the
> semantics of of_parse_phandle(). Rewrite of_parse_phandle() to call
> __of_parse_phandle_with_args() rather than open-coding the simple case.
> -----

Sounds good to me!

> 
> > What's the overhead over the old of_parse_phandle? It looks like this is
> > going to do a lot of pointless work beyond what it already does --
> > parsing each prior entry in the list, and for each prior entry walking
> > the tree in of_find_node_by_phandle. Maybe we don't use long enough
> > phandle lists anywhere for that to be noticeable.
> 
> I think the overhead is pretty minimal. The main difference is that the
> new code will loop over the property cell by cell rather than directly
> jump into the required index. That's not likely to be much work for
> typical properties. In particular, no extra DT property lookups are
> performed, since of_parse_phandle() passes in cells_name=NULL,
> cell_count=0, so the cells_name property is not looked up.

I thought even with your patch we still call of_find_node_by_phandle on
each (phandle) cell as we go over the property, before we hit the check
for cells_name?

Given that of_find_node_by_phandle does a pretty naive linear search of
the of_allnodes list, that could get significant, especially if all the
elements referred to in the property are near the end of the of_allnodes
list.

> 
> Besides, Grant told me to do this change:-)
> 

A Likely story... :)

Thanks,
Mark.

Re: [PATCH V5 4/6] of: call __of_parse_phandle_with_args from of_parse_phandle

[Stephen Warren]

On 08/13/2013 10:29 AM, Mark Rutland wrote:
> On Tue, Aug 13, 2013 at 04:54:02PM +0100, Stephen Warren wrote:
>> On 08/13/2013 03:08 AM, Mark Rutland wrote:
>>> On Mon, Aug 12, 2013 at 06:36:30PM +0100, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> The simplest case of __of_parse_phandle_with_args() implements
>>>> of_parse_phandle(), except that it doesn't return the node referenced by
>>>> the phandle. Modify it to do so, and then rewrite of_parse_phandle() to
>>>> call __of_parse_phandle_with_args() rather than open-coding the simple
>>>> case.
...
>>> What's the overhead over the old of_parse_phandle? It looks like this is
>>> going to do a lot of pointless work beyond what it already does --
>>> parsing each prior entry in the list, and for each prior entry walking
>>> the tree in of_find_node_by_phandle. Maybe we don't use long enough
>>> phandle lists anywhere for that to be noticeable.
>>
>> I think the overhead is pretty minimal. The main difference is that the
>> new code will loop over the property cell by cell rather than directly
>> jump into the required index. That's not likely to be much work for
>> typical properties. In particular, no extra DT property lookups are
>> performed, since of_parse_phandle() passes in cells_name=NULL,
>> cell_count=0, so the cells_name property is not looked up.
> 
> I thought even with your patch we still call of_find_node_by_phandle on
> each (phandle) cell as we go over the property, before we hit the check
> for cells_name?
> 
> Given that of_find_node_by_phandle does a pretty naive linear search of
> the of_allnodes list, that could get significant, especially if all the
> elements referred to in the property are near the end of the of_allnodes
> list.

Oh yes, that is true.

I suppose it'd be possible to make the call to of_find_node_by_phandle()
conditional; it'd be needed:

if (cells_name /* Need to get property from it */ ||
	cur_index == index /* Need to return it */)

Do you think it's worth making that change?

Re: [PATCH V5 4/6] of: call __of_parse_phandle_with_args from of_parse_phandle

[Mark Rutland]

On Tue, Aug 13, 2013 at 07:12:41PM +0100, Stephen Warren wrote:
> On 08/13/2013 10:29 AM, Mark Rutland wrote:
> > On Tue, Aug 13, 2013 at 04:54:02PM +0100, Stephen Warren wrote:
> >> On 08/13/2013 03:08 AM, Mark Rutland wrote:
> >>> On Mon, Aug 12, 2013 at 06:36:30PM +0100, Stephen Warren wrote:
> >>>> From: Stephen Warren <swarren@nvidia.com>
> >>>>
> >>>> The simplest case of __of_parse_phandle_with_args() implements
> >>>> of_parse_phandle(), except that it doesn't return the node referenced by
> >>>> the phandle. Modify it to do so, and then rewrite of_parse_phandle() to
> >>>> call __of_parse_phandle_with_args() rather than open-coding the simple
> >>>> case.
> ...
> >>> What's the overhead over the old of_parse_phandle? It looks like this is
> >>> going to do a lot of pointless work beyond what it already does --
> >>> parsing each prior entry in the list, and for each prior entry walking
> >>> the tree in of_find_node_by_phandle. Maybe we don't use long enough
> >>> phandle lists anywhere for that to be noticeable.
> >>
> >> I think the overhead is pretty minimal. The main difference is that the
> >> new code will loop over the property cell by cell rather than directly
> >> jump into the required index. That's not likely to be much work for
> >> typical properties. In particular, no extra DT property lookups are
> >> performed, since of_parse_phandle() passes in cells_name=NULL,
> >> cell_count=0, so the cells_name property is not looked up.
> > 
> > I thought even with your patch we still call of_find_node_by_phandle on
> > each (phandle) cell as we go over the property, before we hit the check
> > for cells_name?
> > 
> > Given that of_find_node_by_phandle does a pretty naive linear search of
> > the of_allnodes list, that could get significant, especially if all the
> > elements referred to in the property are near the end of the of_allnodes
> > list.
> 
> Oh yes, that is true.
> 
> I suppose it'd be possible to make the call to of_find_node_by_phandle()
> conditional; it'd be needed:
> 
> if (cells_name /* Need to get property from it */ ||
> 	cur_index == index /* Need to return it */)
> 
> Do you think it's worth making that change?
> 

To be honest, I'm not sure. From a quick grep, most of_parse_phandle()
instances are only grabbing element 0, and without digging I can only
see a couple that do more than that. There's probably not that big a hit
in those cases.

I'm happy either way, so feel free to stick my ack on with or without:

Acked-by: Mark Rutland <mark.rutland@arm.com> 

Thanks,
Mark.

Re: [PATCH V5 2/6] of: move of_parse_phandle()

[Mark Rutland]

On Mon, Aug 12, 2013 at 06:36:28PM +0100, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Move of_parse_phandle() after __of_parse_phandle_with_args(), since a
> future patch will call __of_parse_phandle_with_args() from
> of_parse_phandle(). Moving the function avoids adding a prototype. Doing
> the move separately highlights the code changes separately.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

> ---
> v5: New patch.
> ---
>  drivers/of/base.c | 48 ++++++++++++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 23e7073..b9e77ec 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1080,30 +1080,6 @@ int of_property_count_strings(struct device_node *np, const char *propname)
>  }
>  EXPORT_SYMBOL_GPL(of_property_count_strings);
>  
> -/**
> - * of_parse_phandle - Resolve a phandle property to a device_node pointer
> - * @np: Pointer to device node holding phandle property
> - * @phandle_name: Name of property holding a phandle value
> - * @index: For properties holding a table of phandles, this is the index into
> - *         the table
> - *
> - * Returns the device_node pointer with refcount incremented.  Use
> - * of_node_put() on it when done.
> - */
> -struct device_node *of_parse_phandle(const struct device_node *np,
> -				     const char *phandle_name, int index)
> -{
> -	const __be32 *phandle;
> -	int size;
> -
> -	phandle = of_get_property(np, phandle_name, &size);
> -	if ((!phandle) || (size < sizeof(*phandle) * (index + 1)))
> -		return NULL;
> -
> -	return of_find_node_by_phandle(be32_to_cpup(phandle + index));
> -}
> -EXPORT_SYMBOL(of_parse_phandle);
> -
>  static int __of_parse_phandle_with_args(const struct device_node *np,
>  					const char *list_name,
>  					const char *cells_name, int index,
> @@ -1207,6 +1183,30 @@ static int __of_parse_phandle_with_args(const struct device_node *np,
>  }
>  
>  /**
> + * of_parse_phandle - Resolve a phandle property to a device_node pointer
> + * @np: Pointer to device node holding phandle property
> + * @phandle_name: Name of property holding a phandle value
> + * @index: For properties holding a table of phandles, this is the index into
> + *         the table
> + *
> + * Returns the device_node pointer with refcount incremented.  Use
> + * of_node_put() on it when done.
> + */
> +struct device_node *of_parse_phandle(const struct device_node *np,
> +				     const char *phandle_name, int index)
> +{
> +	const __be32 *phandle;
> +	int size;
> +
> +	phandle = of_get_property(np, phandle_name, &size);
> +	if ((!phandle) || (size < sizeof(*phandle) * (index + 1)))
> +		return NULL;
> +
> +	return of_find_node_by_phandle(be32_to_cpup(phandle + index));
> +}
> +EXPORT_SYMBOL(of_parse_phandle);
> +
> +/**
>   * of_parse_phandle_with_args() - Find a node pointed by phandle in a list
>   * @np:		pointer to a device tree node containing a list
>   * @list_name:	property name that contains a list
> -- 
> 1.8.1.5
> 
>