gpio: clean up gpio-ranges documentation

PATCH Superseded Stephen Warren 5 participants 15 messages Jul 15, 2013 – Jul 23, 2013

Subsystems (3)

Device tree bindings GPIO Device tree

lore

Share & tools

lore

Stephen Warren

author

2013-07-15 18:40 UTC #0 lore raw

PATCH v2 1/5 gpio: clean up gpio-ranges documentation

From: Stephen Warren <[email protected]>

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.

This change also removes any mention of 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.

Signed-off-by Stephen Warren <[email protected]>

---
Patches 2-6 need to be applied in the same branch, since they all build upon each-other.

---
 Documentation/devicetree/bindings/gpio/gpio.txt | 48 ++++++++++++-------------
 1 file changed, 23 insertions(+), 25 deletions(-)

Documentation/devicetree/bindings/gpio/gpio.txt +23 ~~-25~~

index d933af3..b262378 100644
@@ -75,23 +75,29 @@ 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 strucutres 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.
+
+Example:
 
 	qe_pio_e: gpio-controller at 1460 {
 		#gpio-cells = <2>;

@@ -99,16 +105,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.

Signature · 1 line

1.8.1.5

Stephen Warren

author

2013-07-15 18:40 UTC #1 lore raw

PATCH v2 2/5 of: move documentation of of_parse_phandle_with_args

From: Stephen Warren <[email protected]>

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 <[email protected]>

---
 drivers/of/base.c | 64 +++++++++++++++++++++++++++----------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

drivers/of/base.c +32 ~~-32~~

index 5c54279..23e7073 100644
@@ -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)

Signature · 1 line

1.8.1.5

Stephen Warren

author

2013-07-15 18:40 UTC #2 lore raw

PATCH v2 3/5 of: introduce of_parse_phandle_with_fixed_args

In reply to Stephen Warren

From: Stephen Warren <[email protected]>

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 <[email protected]>

---
 drivers/of/base.c  | 67 ++++++++++++++++++++++++++++++++++++++++++++++--------
 include/linux/of.h | 10 ++++++++
 2 files changed, 68 insertions(+), 9 deletions(-)

drivers/of/base.c +58 -9

index 23e7073..ad799d9 100644
@@ -1106,7 +1106,8 @@ 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,
+					const char *cells_name,
+					int cells_count, int index,
 					struct of_phandle_args *out_args)
 {
 	const __be32 *list, *list_end;

@@ -1142,12 +1143,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 = cells_count;
 
 			/*
 			 * Make sure that the arguments actually fit in the

@@ -1244,11 +1250,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
+ * @cells_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 cells_count,
+				int index, struct of_phandle_args *out_args)
+{
+	if (index < 0)
+		return -EINVAL;
+	return __of_parse_phandle_with_args(np, list_name, NULL, cells_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 +1314,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);

include/linux/of.h +10 -0

index 1fd08ca..0c457f5 100644
@@ -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);

@@ -488,6 +491,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)

Signature · 1 line

1.8.1.5

Sergei Shtylyov

2013-07-15 18:59 UTC #3 lore raw

PATCH v2 3/5 of: introduce of_parse_phandle_with_fixed_args

Quoted 17 lines · Stephen Warren

From: Stephen Warren <[email protected]>
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 <[email protected]>
---
  drivers/of/base.c  | 67 ++++++++++++++++++++++++++++++++++++++++++++++--------
  include/linux/of.h | 10 ++++++++
  2 files changed, 68 insertions(+), 9 deletions(-)
drivers/of/base.c +0 -0
index 23e7073..ad799d9 100644

drivers/of/base.c EXPORT_SYMBOL(of_parse_phandle); · line 1110

Quoted 6 lines

drivers/of/base.c +2 -1

@@ -1106,7 +1106,8 @@ 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,
+					const char *cells_name,
+					int cells_count, int index,

The correct grammar would be 'cell_name' and 'cell_count' I think.

Quoted 14 lines

drivers/of/base.c +1 -6

  					struct of_phandle_args *out_args)
  {
  	const __be32 *list, *list_end;

@@ -1142,12 +1143,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;
-			}
+

drivers/of/base.c static int __of_parse_phandle_with_args(const struct device_node *np, · line 1156

Quoted 10 lines

drivers/of/base.c +10 -0

+			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 = cells_count;

According to Documentation/CodingStyle chapter 3, both arms of the
*if* statment should have {} if oe arm has it.

WBR, Sergei

Stephen Warren

author

2013-07-15 23:06 UTC #4 lore raw

PATCH v2 3/5 of: introduce of_parse_phandle_with_fixed_args

Quoted 14 lines · Sergei Shtylyov

Quoted 12 lines · Stephen Warren
From: Stephen Warren <[email protected]>
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.
drivers/of/base.c +0 -0
index 23e7073..ad799d9 100644
@@ -1106,7 +1106,8 @@ EXPORT_SYMBOL(of_parse_phandle);
 

Quoted 7 lines

drivers/of/base.c EXPORT_SYMBOL(of_parse_phandle); · line 1110
Quoted 5 lines
drivers/of/base.c +2 -1
  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 cells_count, int index,
The correct grammar would be 'cell_name' and 'cell_count' I think.

I guess I can see the argument for cell_count, but "cells_name" is named based on the DT property named "gpio-cells", so "cells" seems correct here (and is a pre-existing issue anyway).

I'll fix up cell_count and the brace issue locally, but hold off reposting.

Stephen Warren

author

2013-07-15 18:40 UTC #5 lore raw

PATCH v2 4/5 gpio: implement gpio-ranges binding document fix

In reply to Stephen Warren

From: Stephen Warren <[email protected]>

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

Signed-off-by Stephen Warren <[email protected]>

---
 drivers/gpio/gpiolib-of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/gpio/gpiolib-of.c +2 -2

index 665f953..48cda3c 100644
@@ -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;

Signature · 1 line

1.8.1.5

Stephen Warren

author

2013-07-15 18:40 UTC #6 lore raw

PATCH v2 5/5 ARM: remove #gpio-ranges-cells property

In reply to Stephen Warren

From: Stephen Warren <[email protected]>

This property is no longer required by the GPIO binding. Remove it.

Signed-off-by Stephen Warren <[email protected]>

Acked-by Viresh Kumar <[email protected]>

---
This should presumably be applied along with the previous changes

v2: Squash spear and mxc changes into a single patch; there's no need to keep vf610.dtsi separate now that this series will go into v3.12 or later, and that file is part of v3.11-rc1.

---
 arch/arm/boot/dts/spear1310.dtsi | 1 -
 arch/arm/boot/dts/spear1340.dtsi | 1 -
 arch/arm/boot/dts/spear310.dtsi  | 1 -
 arch/arm/boot/dts/spear320.dtsi  | 2 --
 arch/arm/boot/dts/vf610.dtsi     | 1 -
 5 files changed, 6 deletions(-)

arch/arm/boot/dts/spear1310.dtsi +0 -1

index 122ae94..90e99b5 100644
@@ -89,7 +89,6 @@
 		pinmux: pinmux at e0700000 {
 			compatible = "st,spear1310-pinmux";
 			reg = <0xe0700000 0x1000>;
-			#gpio-range-cells = <3>;
 		};
 
 		apb {

arch/arm/boot/dts/spear1340.dtsi +0 -1

index 54d128d..5003013 100644
@@ -63,7 +63,6 @@
 		pinmux: pinmux at e0700000 {
 			compatible = "st,spear1340-pinmux";
 			reg = <0xe0700000 0x1000>;
-			#gpio-range-cells = <3>;
 		};
 
 		pwm: pwm at e0180000 {

arch/arm/boot/dts/spear310.dtsi +0 -1

index 9537208..1e1318e 100644
@@ -25,7 +25,6 @@
 		pinmux: pinmux at b4000000 {
 			compatible = "st,spear310-pinmux";
 			reg = <0xb4000000 0x1000>;
-			#gpio-range-cells = <3>;
 		};
 
 		fsmc: flash at 44000000 {

arch/arm/boot/dts/spear320.dtsi +0 -2

index ffea342..f630d0c 100644
@@ -1,4 +1,3 @@
-/*
  * DTS file for SPEAr320 SoC
  *
  * Copyright 2012 Viresh Kumar <[email protected]>

@@ -24,7 +23,6 @@
 		pinmux: pinmux at b3000000 {
 			compatible = "st,spear320-pinmux";
 			reg = <0xb3000000 0x1000>;
-			#gpio-range-cells = <3>;
 		};
 
 		clcd at 90000000 {

arch/arm/boot/dts/vf610.dtsi +0 -1

index e1eb7da..8577780 100644
@@ -162,7 +162,6 @@
 			iomuxc: iomuxc at 40048000 {
 				compatible = "fsl,vf610-iomuxc";
 				reg = <0x40048000 0x1000>;
-				#gpio-range-cells = <3>;
 
 				/* functions and groups pins */

Signature · 1 line

1.8.1.5

Rob Herring

2013-07-15 19:34 UTC #7 lore raw

PATCH v2 5/5 ARM: remove #gpio-ranges-cells property

Quoted 3 lines · Stephen Warren

From: Stephen Warren <[email protected]>
This property is no longer required by the GPIO binding. Remove it.

Won't this break compatibility with older kernel? It is one thing to deprecate, but removal is another. If the relevant maintainers don't care, then I guess it is fine.

Quoted 4 lines

Signed-off-by Stephen Warren <[email protected]>
Acked-by Viresh Kumar <[email protected]>
---
This should presumably be applied along with the previous changes

The rest looks fine to me. I can take this series, but would actually prefer that this patch go separately to arm-soc.

Rob

Quoted 78 lines

v2: Squash spear and mxc changes into a single patch; there's no need to keep vf610.dtsi separate now that this series will go into v3.12 or later, and that file is part of v3.11-rc1.

---
 arch/arm/boot/dts/spear1310.dtsi | 1 -
 arch/arm/boot/dts/spear1340.dtsi | 1 -
 arch/arm/boot/dts/spear310.dtsi  | 1 -
 arch/arm/boot/dts/spear320.dtsi  | 2 --
 arch/arm/boot/dts/vf610.dtsi     | 1 -
 5 files changed, 6 deletions(-)

arch/arm/boot/dts/spear1310.dtsi +0 -1

index 122ae94..90e99b5 100644
@@ -89,7 +89,6 @@
 		pinmux: pinmux at e0700000 {
 			compatible = "st,spear1310-pinmux";
 			reg = <0xe0700000 0x1000>;
-			#gpio-range-cells = <3>;
 		};
 
 		apb {

arch/arm/boot/dts/spear1340.dtsi +0 -1

index 54d128d..5003013 100644
@@ -63,7 +63,6 @@
 		pinmux: pinmux at e0700000 {
 			compatible = "st,spear1340-pinmux";
 			reg = <0xe0700000 0x1000>;
-			#gpio-range-cells = <3>;
 		};
 
 		pwm: pwm at e0180000 {

arch/arm/boot/dts/spear310.dtsi +0 -1

index 9537208..1e1318e 100644
@@ -25,7 +25,6 @@
 		pinmux: pinmux at b4000000 {
 			compatible = "st,spear310-pinmux";
 			reg = <0xb4000000 0x1000>;
-			#gpio-range-cells = <3>;
 		};
 
 		fsmc: flash at 44000000 {

arch/arm/boot/dts/spear320.dtsi +0 -2

index ffea342..f630d0c 100644
@@ -1,4 +1,3 @@
-/*
  * DTS file for SPEAr320 SoC
  *
  * Copyright 2012 Viresh Kumar <[email protected]>

@@ -24,7 +23,6 @@
 		pinmux: pinmux at b3000000 {
 			compatible = "st,spear320-pinmux";
 			reg = <0xb3000000 0x1000>;
-			#gpio-range-cells = <3>;
 		};
 
 		clcd at 90000000 {

arch/arm/boot/dts/vf610.dtsi +0 -1

index e1eb7da..8577780 100644
@@ -162,7 +162,6 @@
 			iomuxc: iomuxc at 40048000 {
 				compatible = "fsl,vf610-iomuxc";
 				reg = <0x40048000 0x1000>;
-				#gpio-range-cells = <3>;
 
 				/* functions and groups pins */

Stephen Warren

author

2013-07-15 23:02 UTC #8 lore raw

PATCH v2 5/5 ARM: remove #gpio-ranges-cells property

Quoted 8 lines · Rob Herring

Quoted 3 lines · Stephen Warren
From: Stephen Warren <[email protected]>
This property is no longer required by the GPIO binding. Remove it.
Won't this break compatibility with older kernel? It is one thing to deprecate, but removal is another. If the relevant maintainers don't care, then I guess it is fine.

Yes.

I had originally hoped this could sneak in late for 3.11, but I suppose it's too late now. vf610.dtsi is a new file in 3.11 so has no legacy to protect.

Admittedly, the #gpio-cells property was added into the SPEAr files in 3.10.

Stephen Warren

author

2013-07-16 23:30 UTC #9 lore raw

PATCH v2 5/5 ARM: remove #gpio-ranges-cells property

Quoted 10 lines · Stephen Warren

Quoted 8 lines · Rob Herring
Quoted 3 lines · Stephen Warren
From: Stephen Warren <[email protected]>
This property is no longer required by the GPIO binding. Remove it.
Won't this break compatibility with older kernel? It is one thing to deprecate, but removal is another. If the relevant maintainers don't care, then I guess it is fine.

Quoted 7 lines

Yes.
I had originally hoped this could sneak in late for 3.11, but I suppose it's too late now. vf610.dtsi is a new file in 3.11 so has no legacy to protect.
Admittedly, the #gpio-cells property was added into the SPEAr files in 3.10.

One more thought here:

I know DT bindings are supposed to evolve so that a new kernel will support arbitrary old DTs. I'll call that backwards-compatibility for the DT parsing code.

However, this situation is the reverse; this patch would prevent a new DT running on an older kernel. I'll call that forwards-compatibility. I'm not sure if the intent is to support this or not? It's certainly the first I explicitly thought about compatibility in this direction...

Rob Herring

2013-07-17 01:50 UTC #10 lore raw

PATCH v2 5/5 ARM: remove #gpio-ranges-cells property

Quoted 17 lines · Stephen Warren

Quoted 16 lines · Stephen Warren
Quoted 8 lines · Rob Herring
Quoted 3 lines · Stephen Warren
From: Stephen Warren <[email protected]>
This property is no longer required by the GPIO binding. Remove it.
Won't this break compatibility with older kernel? It is one thing to deprecate, but removal is another. If the relevant maintainers don't care, then I guess it is fine.
Yes.
I had originally hoped this could sneak in late for 3.11, but I suppose it's too late now. vf610.dtsi is a new file in 3.11 so has no legacy to protect.

Quoted 7 lines

Quoted 1 line
Admittedly, the #gpio-cells property was added into the SPEAr files in 3.10.
One more thought here:
I know DT bindings are supposed to evolve so that a new kernel will support arbitrary old DTs. I'll call that backwards-compatibility for the DT parsing code.

That is the more common case.

Quoted 4 lines

However, this situation is the reverse; this patch would prevent a new DT running on an older kernel. I'll call that forwards-compatibility. I'm not sure if the intent is to support this or not? It's certainly the first I explicitly thought about compatibility in this direction...

So you would be okay if your computer stopped booting a kernel after a BIOS update? It's the same deal. It's both forwards and backwards compatibility that is needed.

Rob

Stephen Warren

author

2013-07-17 02:58 UTC #11 lore raw

PATCH v2 5/5 ARM: remove #gpio-ranges-cells property

Quoted 28 lines · Rob Herring

Quoted 17 lines · Stephen Warren
Quoted 16 lines · Stephen Warren
Quoted 8 lines · Rob Herring
Quoted 3 lines · Stephen Warren
From: Stephen Warren <[email protected]>
This property is no longer required by the GPIO binding. Remove it.
Won't this break compatibility with older kernel? It is one thing to deprecate, but removal is another. If the relevant maintainers don't care, then I guess it is fine.
Yes.
I had originally hoped this could sneak in late for 3.11, but I suppose it's too late now. vf610.dtsi is a new file in 3.11 so has no legacy to protect.
Quoted 7 lines
Quoted 1 line
Admittedly, the #gpio-cells property was added into the SPEAr files in 3.10.
One more thought here:
I know DT bindings are supposed to evolve so that a new kernel will support arbitrary old DTs. I'll call that backwards-compatibility for the DT parsing code.
That is the more common case.

Quoted 8 lines

Quoted 4 lines
However, this situation is the reverse; this patch would prevent a new DT running on an older kernel. I'll call that forwards-compatibility. I'm not sure if the intent is to support this or not? It's certainly the first I explicitly thought about compatibility in this direction...
So you would be okay if your computer stopped booting a kernel after a BIOS update? It's the same deal. It's both forwards and backwards compatibility that is needed.

I would strongly hope the BIOS/bootloader/... would have nothing to do with the DT content. There's a reason that Grant asserted early on that DTBs shouldn't be part of the BIOS/bootloader, but rather stored separately, so the DTB could be updated without updating firmware, just like the kernel. And I see no real problem with a new DTB requiring a new kernel or even vice-versa to be honest.

Laurent Pinchart

2013-07-18 01:35 UTC #12 lore raw

PATCH v2 5/5 ARM: remove #gpio-ranges-cells property

In reply to Stephen Warren

Hi Stephen,

Quoted 8 lines · On Monday 15 July 2013 12:40:29 Stephen Warren wrote:

From: Stephen Warren <[email protected]>
This property is no longer required by the GPIO binding. Remove it.
Signed-off-by Stephen Warren <[email protected]>
Acked-by Viresh Kumar <[email protected]>
---
This should presumably be applied along with the previous changes

The mach-shmobile tree includes patches for v3.12 that will add the #gpio- ranges-cells to several .dtsi files. I can get that fixed locally (most probably through a rebase), could you please ping me when this series will be accepted for upstream merge ?

Quoted 75 lines

v2: Squash spear and mxc changes into a single patch; there's no need to keep vf610.dtsi separate now that this series will go into v3.12 or later, and that file is part of v3.11-rc1.

---
 arch/arm/boot/dts/spear1310.dtsi | 1 -
 arch/arm/boot/dts/spear1340.dtsi | 1 -
 arch/arm/boot/dts/spear310.dtsi  | 1 -
 arch/arm/boot/dts/spear320.dtsi  | 2 --
 arch/arm/boot/dts/vf610.dtsi     | 1 -
 5 files changed, 6 deletions(-)

(quoted diff) +0 -0

b/arch/arm/boot/dts/spear1310.dtsi index 122ae94..90e99b5 100644

arch/arm/boot/dts/spear1310.dtsi +0 -1

@@ -89,7 +89,6 @@
 		pinmux: pinmux at e0700000 {
 			compatible = "st,spear1310-pinmux";
 			reg = <0xe0700000 0x1000>;
-			#gpio-range-cells = <3>;
 		};
 
 		apb {

(quoted diff) +0 -0

b/arch/arm/boot/dts/spear1340.dtsi index 54d128d..5003013 100644

arch/arm/boot/dts/spear1340.dtsi +0 -1

@@ -63,7 +63,6 @@
 		pinmux: pinmux at e0700000 {
 			compatible = "st,spear1340-pinmux";
 			reg = <0xe0700000 0x1000>;
-			#gpio-range-cells = <3>;
 		};
 
 		pwm: pwm at e0180000 {

(quoted diff) +0 -0

b/arch/arm/boot/dts/spear310.dtsi index 9537208..1e1318e 100644

arch/arm/boot/dts/spear310.dtsi +0 -1

@@ -25,7 +25,6 @@
 		pinmux: pinmux at b4000000 {
 			compatible = "st,spear310-pinmux";
 			reg = <0xb4000000 0x1000>;
-			#gpio-range-cells = <3>;
 		};
 
 		fsmc: flash at 44000000 {

(quoted diff) +0 -0

b/arch/arm/boot/dts/spear320.dtsi index ffea342..f630d0c 100644

arch/arm/boot/dts/spear320.dtsi +0 -2

@@ -1,4 +1,3 @@
-/*
  * DTS file for SPEAr320 SoC
  *
  * Copyright 2012 Viresh Kumar <[email protected]>

@@ -24,7 +23,6 @@
 		pinmux: pinmux at b3000000 {
 			compatible = "st,spear320-pinmux";
 			reg = <0xb3000000 0x1000>;
-			#gpio-range-cells = <3>;
 		};
 
 		clcd at 90000000 {

arch/arm/boot/dts/vf610.dtsi +0 -1

index e1eb7da..8577780 100644
@@ -162,7 +162,6 @@
 			iomuxc: iomuxc at 40048000 {
 				compatible = "fsl,vf610-iomuxc";
 				reg = <0x40048000 0x1000>;
-				#gpio-range-cells = <3>;
 
 				/* functions and groups pins */

Signature · 3 lines

Regards,

Laurent Pinchart

Linus Walleij

2013-07-22 22:31 UTC #13 lore raw

PATCH v2 1/5 gpio: clean up gpio-ranges documentation

In reply to Stephen Warren

Quoted 5 lines · Stephen Warren

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.

This is all good.

Quoted 9 lines

This change also removes any mention of 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.
Signed-off-by Stephen Warren <[email protected]>

Can this mention the newly introduced Backus-Naur Form description of the ranges.

Quoted 4 lines

---
Patches 2-6 need to be applied in the same branch, since they all build upon each-other.
---

Still waiting for the OF maintainers to ACK so can't apply this. :-(

Documentation/devicetree/bindings/gpio/gpio.txt Example of two SOC GPIO banks defined as gpio-controller nodes: · line 87

Quoted 3 lines

Documentation/devicetree/bindings/gpio/gpio.txt +3 -0

+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 strucutres as follows:

speling of strucutres

Should you mention that this is given in BNF?
Or is that implicit for all bindings?

Documentation/devicetree/bindings/gpio/gpio.txt Example of two SOC GPIO banks defined as gpio-controller nodes: · line 95

Quoted 11 lines

Documentation/devicetree/bindings/gpio/gpio.txt +7 -3

-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

Yours,
Linus Walleij

Stephen Warren

author

2013-07-23 16:14 UTC #14 lore raw

PATCH v2 1/5 gpio: clean up gpio-ranges documentation

Quoted 16 lines · Linus Walleij

Quoted 13 lines · Stephen Warren
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.
This change also removes any mention of 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.

Quoted 8 lines

Documentation/devicetree/bindings/gpio/gpio.txt Example of two SOC GPIO banks defined as gpio-controller nodes: · line 87
Quoted 3 lines
Documentation/devicetree/bindings/gpio/gpio.txt +3 -0
+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 strucutres as follows:
speling of strucutres
Should you mention that this is given in BNF?
Or is that implicit for all bindings?

The rest of the document already has a couple of other sections written that way, so explicitly mentioning BNF seems like a logically unrelated patch to fix a separate issue in the document. I didn't actually check whether the syntax used here is strictly BNF either:-) Either way though, I think it's easy enough to read the BNF without having to explicitly know it's BNF or anything in-particular, so I'd err on the side of not bothering to mention that myself...

I'll fix the other issues you mentioned locally, and wait for an ack for drivers/of before reposting.