[PATCH V2 1/6] dt: add property iteration helpers

[PATCH V2 1/6] dt: add property iteration helpers

[Stephen Warren]

This patch adds macros of_property_for_each_u32() and
of_property_for_each_string(), which iterate over an array of values
within a device-tree property. Usage is for example:

struct property *prop;
const __be32 *p;
u32 u;
of_property_for_each_u32(np, "propname", prop, p, u)
	printk("U32 value: %x\n", u);

struct property *prop;
const char *s;
of_property_for_each_string(np, "propname", prop, s)
	printk("String value: %s\n", s);

Based on work by Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
---
v2: Simplified the implementation per suggestion by Rob Herring.
---
 include/linux/of_iter_prop.h |  101 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 101 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/of_iter_prop.h

diff --git a/include/linux/of_iter_prop.h b/include/linux/of_iter_prop.h
new file mode 100644
index 0000000..e3df23c
--- /dev/null
+++ b/include/linux/of_iter_prop.h
@@ -0,0 +1,101 @@
+/*
+ * Copyright (c) 2011-2012 NVIDIA CORPORATION. All rights reserved.
+ *
+ * Iterate over properties that store arrays.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __OF_ITER_PROP_H__
+#define __OF_ITER_PROP_H__
+
+#include <linux/of.h>
+
+#ifdef CONFIG_OF
+static inline const __be32 *of_prop_next_u32(struct property *prop,
+					     const __be32 *cur, u32 *pu)
+{
+	const void *curv = cur;
+
+	if (!prop)
+		return NULL;
+
+	if (!cur) {
+		curv = prop->value;
+		goto out_val;
+	}
+
+	curv += sizeof(*cur);
+	if (curv >= prop->value + prop->length)
+		return NULL;
+
+out_val:
+	*pu = be32_to_cpup(curv);
+	return curv;
+}
+
+/*
+ * struct property *prop;
+ * const __be32 *p;
+ * u32 u;
+ *
+ * of_property_for_each_u32(np, "propname", prop, p, u)
+ *         printk("U32 value: %x\n", u);
+ */
+#define of_property_for_each_u32(np, propname, prop, p, u)	\
+	for (prop = of_find_property(np, propname, NULL),	\
+		p = of_prop_next_u32(prop, NULL, &u);		\
+		p;						\
+		p = of_prop_next_u32(prop, p, &u))
+
+static inline const char *of_prop_next_string(struct property *prop,
+					      const char *cur)
+{
+	const void *curv = cur;
+
+	if (!prop)
+		return NULL;
+
+	if (!cur)
+		return prop->value;
+
+	curv += strlen(cur) + 1;
+	if (curv >= prop->value + prop->length)
+		return NULL;
+
+	return curv;
+}
+
+/*
+ * struct property *prop;
+ * const char *s;
+ *
+ * of_property_for_each_string(np, "propname", prop, s)
+ *         printk("String value: %s\n", s);
+ */
+#define of_property_for_each_string(np, propname, prop, s)	\
+	for (prop = of_find_property(np, propname, NULL),	\
+		s = of_prop_next_string(prop, NULL);		\
+		s;						\
+		s = of_prop_next_string(prop, s))
+
+#else
+
+#define of_property_for_each_u32(np, propname, prop, p, u) \
+	while (0)
+
+#define of_property_for_each_string(np, propname, prop, s) \
+	while (0)
+
+#endif /* CONFIG_OF */
+
+#endif /* __OF_ITER_PROP_H__ */
-- 
1.7.0.4

[PATCH V2 1/6] dt: add property iteration helpers

[Stephen Warren]

This patch adds macros of_property_for_each_u32() and
of_property_for_each_string(), which iterate over an array of values
within a device-tree property. Usage is for example:

struct property *prop;
const __be32 *p;
u32 u;
of_property_for_each_u32(np, "propname", prop, p, u)
	printk("U32 value: %x\n", u);

struct property *prop;
const char *s;
of_property_for_each_string(np, "propname", prop, s)
	printk("String value: %s\n", s);

Based on work by Rob Herring <robherring2@gmail.com>

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
v2: Simplified the implementation per suggestion by Rob Herring.
---
 include/linux/of_iter_prop.h |  101 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 101 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/of_iter_prop.h

diff --git a/include/linux/of_iter_prop.h b/include/linux/of_iter_prop.h
new file mode 100644
index 0000000..e3df23c
--- /dev/null
+++ b/include/linux/of_iter_prop.h
@@ -0,0 +1,101 @@
+/*
+ * Copyright (c) 2011-2012 NVIDIA CORPORATION. All rights reserved.
+ *
+ * Iterate over properties that store arrays.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __OF_ITER_PROP_H__
+#define __OF_ITER_PROP_H__
+
+#include <linux/of.h>
+
+#ifdef CONFIG_OF
+static inline const __be32 *of_prop_next_u32(struct property *prop,
+					     const __be32 *cur, u32 *pu)
+{
+	const void *curv = cur;
+
+	if (!prop)
+		return NULL;
+
+	if (!cur) {
+		curv = prop->value;
+		goto out_val;
+	}
+
+	curv += sizeof(*cur);
+	if (curv >= prop->value + prop->length)
+		return NULL;
+
+out_val:
+	*pu = be32_to_cpup(curv);
+	return curv;
+}
+
+/*
+ * struct property *prop;
+ * const __be32 *p;
+ * u32 u;
+ *
+ * of_property_for_each_u32(np, "propname", prop, p, u)
+ *         printk("U32 value: %x\n", u);
+ */
+#define of_property_for_each_u32(np, propname, prop, p, u)	\
+	for (prop = of_find_property(np, propname, NULL),	\
+		p = of_prop_next_u32(prop, NULL, &u);		\
+		p;						\
+		p = of_prop_next_u32(prop, p, &u))
+
+static inline const char *of_prop_next_string(struct property *prop,
+					      const char *cur)
+{
+	const void *curv = cur;
+
+	if (!prop)
+		return NULL;
+
+	if (!cur)
+		return prop->value;
+
+	curv += strlen(cur) + 1;
+	if (curv >= prop->value + prop->length)
+		return NULL;
+
+	return curv;
+}
+
+/*
+ * struct property *prop;
+ * const char *s;
+ *
+ * of_property_for_each_string(np, "propname", prop, s)
+ *         printk("String value: %s\n", s);
+ */
+#define of_property_for_each_string(np, propname, prop, s)	\
+	for (prop = of_find_property(np, propname, NULL),	\
+		s = of_prop_next_string(prop, NULL);		\
+		s;						\
+		s = of_prop_next_string(prop, s))
+
+#else
+
+#define of_property_for_each_u32(np, propname, prop, p, u) \
+	while (0)
+
+#define of_property_for_each_string(np, propname, prop, s) \
+	while (0)
+
+#endif /* CONFIG_OF */
+
+#endif /* __OF_ITER_PROP_H__ */
-- 
1.7.0.4

[PATCH V2 2/6] dt: pinctrl: Document device tree binding

[Stephen Warren]

The core pin controller bindings define:
* The fact that pin controllers expose pin configurations as nodes in
  device tree.
* That the bindings for those pin configuration nodes is defined by the
  individual pin controller drivers.
* A standardized set of properties for client devices to define numbered
  or named pin configuration states, each referring to some number of the
  afore-mentioned pin configuration nodes.
* That the bindings for the client devices determines the set of numbered
  or named states that must exist.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
Acked-by: Shawn Guo <shawn.guo@linaro.org>
Acked-by: Tony Lindgren <tony@atomide.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
v2: Fix a couple of grammar-os per Randy Dunlap. Add example of a client
device that references no pin configuration nodes per Dong Aisheng.
---
 .../bindings/pinctrl/pinctrl-bindings.txt          |  128 ++++++++++++++++++++
 1 files changed, 128 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
new file mode 100644
index 0000000..c95ea82
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -0,0 +1,128 @@
+== Introduction ==
+
+Hardware modules that control pin multiplexing or configuration parameters
+such as pull-up/down, tri-state, drive-strength etc are designated as pin
+controllers. Each pin controller must be represented as a node in device tree,
+just like any other hardware module.
+
+Hardware modules whose signals are affected by pin configuration are
+designated client devices. Again, each client device must be represented as a
+node in device tree, just like any other hardware module.
+
+For a client device to operate correctly, certain pin controllers must
+set up certain specific pin configurations. Some client devices need a
+single static pin configuration, e.g. set up during initialization. Others
+need to reconfigure pins at run-time, for example to tri-state pins when the
+device is inactive. Hence, each client device can define a set of named
+states. The number and names of those states is defined by the client device's
+own binding.
+
+The common pinctrl bindings defined in this file provide an infrastructure
+for client device device tree nodes to map those state names to the pin
+configuration used by those states.
+
+Note that pin controllers themselves may also be client devices of themselves.
+For example, a pin controller may set up its own "active" state when the
+driver loads. This would allow representing a board's static pin configuration
+in a single place, rather than splitting it across multiple client device
+nodes. The decision to do this or not somewhat rests with the author of
+individual board device tree files, and any requirements imposed by the
+bindings for the individual client devices in use by that board, i.e. whether
+they require certain specific named states for dynamic pin configuration.
+
+== Pinctrl client devices ==
+
+For each client device individually, every pin state is assigned an integer
+ID. These numbers start at 0, and are contiguous. For each state ID, a unique
+property exists to define the pin configuration. Each state may also be
+assigned a name. When names are used, another property exists to map from
+those names to the integer IDs.
+
+Each client device's own binding determines the set of states the must be
+defined in its device tree node, and whether to define the set of state
+IDs that must be provided, or whether to define the set of state names that
+must be provided.
+
+Required properties:
+pinctrl-0:	List of phandles, each pointing at a pin configuration
+		node. These referenced pin configuration nodes must be child
+		nodes of the pin controller that they configure. Multiple
+		entries may exist in this list so that multiple pin
+		controllers may be configured, or so that a state may be built
+		from multiple nodes for a single pin controller, each
+		contributing part of the overall configuration. See the next
+		section of this document for details of the format of these
+		pin configuration nodes.
+
+		In some cases, it may be useful to define a state, but for it
+		to be empty. This may be required when a common IP block is
+		used in an SoC either without a pin controller, or where the
+		pin controller does not affect the HW module in question. If
+		the binding for that IP block requires certain pin states to
+		exist, they must still be defined, but may be left empty.
+
+Optional properties:
+pinctrl-1:	List of phandles, each pointing at a pin configuration
+		node within a pin controller.
+...
+pinctrl-n:	List of phandles, each pointing at a pin configuration
+		node within a pin controller.
+pinctrl-names:	The list of names to assign states. List entry 0 defines the
+		name for integer state ID 0, list entry 1 for state ID 1, and
+		so on.
+
+For example:
+
+	/* For a client device requiring named states */
+	device {
+		pinctrl-names = "active", "idle";
+		pinctrl-0 = <&state_0_node_a>;
+		pinctrl-1 = <&state_1_node_a &state_1_node_b>;
+	};
+
+	/* For the same device if using state IDs */
+	device {
+		pinctrl-0 = <&state_0_node_a>;
+		pinctrl-1 = <&state_1_node_a &state_1_node_b>;
+	};
+
+	/*
+	 * For an IP block whose binding supports pin configuration,
+	 * but in use on an SoC that doesn't have any pin control hardware
+	 */
+	device {
+		pinctrl-names = "active", "idle";
+		pinctrl-0 = <>;
+		pinctrl-1 = <>;
+	};
+
+== Pin controller devices ==
+
+Pin controller devices should contain the pin configuration nodes that client
+devices reference.
+
+For example:
+
+	pincontroller {
+		... /* Standard DT properties for the device itself elided */
+
+		state_0_node_a {
+			...
+		};
+		state_1_node_a {
+			...
+		};
+		state_1_node_b {
+			...
+		};
+	}
+
+The contents of each of those pin configuration child nodes is defined
+entirely by the binding for the individual pin controller device. There
+exists no common standard for this content.
+
+The pin configuration nodes need not be direct children of the pin controller
+device; they may be grandchildren, for example. Whether this is legal, and
+whether there is any interaction between the child and intermediate parent
+nodes, is again defined entirely by the binding for the individual pin
+controller device.
-- 
1.7.0.4

Re: [PATCH V2 2/6] dt: pinctrl: Document device tree binding

[Simon Glass]

On Tue, Mar 20, 2012 at 10:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> The core pin controller bindings define:
> * The fact that pin controllers expose pin configurations as nodes in
>  device tree.
> * That the bindings for those pin configuration nodes is defined by the
>  individual pin controller drivers.
> * A standardized set of properties for client devices to define numbered
>  or named pin configuration states, each referring to some number of the
>  afore-mentioned pin configuration nodes.
> * That the bindings for the client devices determines the set of numbered
>  or named states that must exist.
>
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> Acked-by: Shawn Guo <shawn.guo@linaro.org>
> Acked-by: Tony Lindgren <tony@atomide.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Acked-by: Simon Glass <sjg@chromium.org>

> ---
> v2: Fix a couple of grammar-os per Randy Dunlap. Add example of a client
> device that references no pin configuration nodes per Dong Aisheng.
> ---
>  .../bindings/pinctrl/pinctrl-bindings.txt          |  128 ++++++++++++++++++++
>  1 files changed, 128 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> new file mode 100644
> index 0000000..c95ea82
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> @@ -0,0 +1,128 @@
> +== Introduction ==
> +
> +Hardware modules that control pin multiplexing or configuration parameters
> +such as pull-up/down, tri-state, drive-strength etc are designated as pin
> +controllers. Each pin controller must be represented as a node in device tree,
> +just like any other hardware module.
> +
> +Hardware modules whose signals are affected by pin configuration are
> +designated client devices. Again, each client device must be represented as a
> +node in device tree, just like any other hardware module.
> +
> +For a client device to operate correctly, certain pin controllers must
> +set up certain specific pin configurations. Some client devices need a
> +single static pin configuration, e.g. set up during initialization. Others
> +need to reconfigure pins at run-time, for example to tri-state pins when the
> +device is inactive. Hence, each client device can define a set of named
> +states. The number and names of those states is defined by the client device's
> +own binding.
> +
> +The common pinctrl bindings defined in this file provide an infrastructure
> +for client device device tree nodes to map those state names to the pin
> +configuration used by those states.
> +
> +Note that pin controllers themselves may also be client devices of themselves.
> +For example, a pin controller may set up its own "active" state when the
> +driver loads. This would allow representing a board's static pin configuration
> +in a single place, rather than splitting it across multiple client device
> +nodes. The decision to do this or not somewhat rests with the author of
> +individual board device tree files, and any requirements imposed by the
> +bindings for the individual client devices in use by that board, i.e. whether
> +they require certain specific named states for dynamic pin configuration.
> +
> +== Pinctrl client devices ==
> +
> +For each client device individually, every pin state is assigned an integer
> +ID. These numbers start at 0, and are contiguous. For each state ID, a unique
> +property exists to define the pin configuration. Each state may also be
> +assigned a name. When names are used, another property exists to map from
> +those names to the integer IDs.
> +
> +Each client device's own binding determines the set of states the must be
> +defined in its device tree node, and whether to define the set of state
> +IDs that must be provided, or whether to define the set of state names that
> +must be provided.
> +
> +Required properties:
> +pinctrl-0:     List of phandles, each pointing at a pin configuration
> +               node. These referenced pin configuration nodes must be child
> +               nodes of the pin controller that they configure. Multiple
> +               entries may exist in this list so that multiple pin
> +               controllers may be configured, or so that a state may be built
> +               from multiple nodes for a single pin controller, each
> +               contributing part of the overall configuration. See the next
> +               section of this document for details of the format of these
> +               pin configuration nodes.
> +
> +               In some cases, it may be useful to define a state, but for it
> +               to be empty. This may be required when a common IP block is
> +               used in an SoC either without a pin controller, or where the
> +               pin controller does not affect the HW module in question. If
> +               the binding for that IP block requires certain pin states to
> +               exist, they must still be defined, but may be left empty.
> +
> +Optional properties:
> +pinctrl-1:     List of phandles, each pointing at a pin configuration
> +               node within a pin controller.
> +...
> +pinctrl-n:     List of phandles, each pointing at a pin configuration
> +               node within a pin controller.
> +pinctrl-names: The list of names to assign states. List entry 0 defines the
> +               name for integer state ID 0, list entry 1 for state ID 1, and
> +               so on.
> +
> +For example:
> +
> +       /* For a client device requiring named states */
> +       device {
> +               pinctrl-names = "active", "idle";
> +               pinctrl-0 = <&state_0_node_a>;
> +               pinctrl-1 = <&state_1_node_a &state_1_node_b>;
> +       };
> +
> +       /* For the same device if using state IDs */
> +       device {
> +               pinctrl-0 = <&state_0_node_a>;
> +               pinctrl-1 = <&state_1_node_a &state_1_node_b>;
> +       };
> +
> +       /*
> +        * For an IP block whose binding supports pin configuration,
> +        * but in use on an SoC that doesn't have any pin control hardware
> +        */
> +       device {
> +               pinctrl-names = "active", "idle";
> +               pinctrl-0 = <>;
> +               pinctrl-1 = <>;
> +       };
> +
> +== Pin controller devices ==
> +
> +Pin controller devices should contain the pin configuration nodes that client
> +devices reference.
> +
> +For example:
> +
> +       pincontroller {
> +               ... /* Standard DT properties for the device itself elided */
> +
> +               state_0_node_a {
> +                       ...
> +               };
> +               state_1_node_a {
> +                       ...
> +               };
> +               state_1_node_b {
> +                       ...
> +               };
> +       }
> +
> +The contents of each of those pin configuration child nodes is defined
> +entirely by the binding for the individual pin controller device. There
> +exists no common standard for this content.
> +
> +The pin configuration nodes need not be direct children of the pin controller
> +device; they may be grandchildren, for example. Whether this is legal, and
> +whether there is any interaction between the child and intermediate parent
> +nodes, is again defined entirely by the binding for the individual pin
> +controller device.
> --
> 1.7.0.4
>

Re: [PATCH V2 2/6] dt: pinctrl: Document device tree binding

[Dong Aisheng]

On Wed, Mar 21, 2012 at 01:44:35AM +0800, Stephen Warren wrote:
> The core pin controller bindings define:
> * The fact that pin controllers expose pin configurations as nodes in
>   device tree.
> * That the bindings for those pin configuration nodes is defined by the
>   individual pin controller drivers.
> * A standardized set of properties for client devices to define numbered
>   or named pin configuration states, each referring to some number of the
>   afore-mentioned pin configuration nodes.
> * That the bindings for the client devices determines the set of numbered
>   or named states that must exist.
> 
> Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> Acked-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Acked-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> v2: Fix a couple of grammar-os per Randy Dunlap. Add example of a client
> device that references no pin configuration nodes per Dong Aisheng.
> ---
Acked-by: Dong Aisheng <dong.aisheng-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Regards
Dong Aisheng

Re: [PATCH V2 2/6] dt: pinctrl: Document device tree binding

[Dong Aisheng]

On Wed, Mar 21, 2012 at 01:44:35AM +0800, Stephen Warren wrote:
> The core pin controller bindings define:
> * The fact that pin controllers expose pin configurations as nodes in
>   device tree.
> * That the bindings for those pin configuration nodes is defined by the
>   individual pin controller drivers.
> * A standardized set of properties for client devices to define numbered
>   or named pin configuration states, each referring to some number of the
>   afore-mentioned pin configuration nodes.
> * That the bindings for the client devices determines the set of numbered
>   or named states that must exist.
> 
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> Acked-by: Shawn Guo <shawn.guo@linaro.org>
> Acked-by: Tony Lindgren <tony@atomide.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> v2: Fix a couple of grammar-os per Randy Dunlap. Add example of a client
> device that references no pin configuration nodes per Dong Aisheng.
> ---
Acked-by: Dong Aisheng <dong.aisheng@linaro.org>

Regards
Dong Aisheng

[PATCH V2 3/6] pinctrl: core device tree mapping table parsing support

[Stephen Warren]

During pinctrl_get(), if the client device has a device tree node, look
for the common pinctrl properties there. If found, parse the referenced
device tree nodes, with the help of the pinctrl drivers, and generate
mapping table entries from them.

During pinctrl_put(), free any results of device tree parsing.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
v2: Place most code into new file devicetree.c
---
 drivers/pinctrl/Makefile        |    1 +
 drivers/pinctrl/core.c          |   72 ++++++++---
 drivers/pinctrl/core.h          |    8 ++
 drivers/pinctrl/devicetree.c    |  265 +++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/devicetree.h    |   35 +++++
 include/linux/pinctrl/pinctrl.h |    7 +
 6 files changed, 371 insertions(+), 17 deletions(-)
 create mode 100644 drivers/pinctrl/devicetree.c
 create mode 100644 drivers/pinctrl/devicetree.h

diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 6d4150b..049c9fb 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_PINCTRL)	+= -DDEBUG
 obj-$(CONFIG_PINCTRL)		+= core.o
 obj-$(CONFIG_PINMUX)		+= pinmux.o
 obj-$(CONFIG_PINCONF)		+= pinconf.o
+obj-$(CONFIG_OF)		+= devicetree.o
 obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
 obj-$(CONFIG_PINCTRL_PXA3xx)	+= pinctrl-pxa3xx.o
 obj-$(CONFIG_PINCTRL_MMP2)	+= pinctrl-mmp2.o
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index df6296c..832f71d 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -26,6 +26,7 @@
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/machine.h>
 #include "core.h"
+#include "devicetree.h"
 #include "pinmux.h"
 #include "pinconf.h"
 
@@ -45,7 +46,7 @@ struct pinctrl_maps {
 DEFINE_MUTEX(pinctrl_mutex);
 
 /* Global list of pin control devices (struct pinctrl_dev) */
-static LIST_HEAD(pinctrldev_list);
+LIST_HEAD(pinctrldev_list);
 
 /* List of pin controller handles (struct pinctrl) */
 static LIST_HEAD(pinctrl_list);
@@ -579,6 +580,13 @@ static struct pinctrl *create_pinctrl(struct device *dev)
 	}
 	p->dev = dev;
 	INIT_LIST_HEAD(&p->states);
+	INIT_LIST_HEAD(&p->dt_maps);
+
+	ret = pinctrl_dt_to_map(p);
+	if (ret < 0) {
+		kfree(p);
+		return ERR_PTR(ret);
+	}
 
 	devname = dev_name(dev);
 
@@ -662,6 +670,8 @@ static void pinctrl_put_locked(struct pinctrl *p, bool inlist)
 		kfree(state);
 	}
 
+	pinctrl_dt_free_maps(p);
+
 	if (inlist)
 		list_del(&p->node);
 	kfree(p);
@@ -787,15 +797,8 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
 }
 EXPORT_SYMBOL_GPL(pinctrl_select_state);
 
-/**
- * pinctrl_register_mappings() - register a set of pin controller mappings
- * @maps: the pincontrol mappings table to register. This should probably be
- *	marked with __initdata so it can be discarded after boot. This
- *	function will perform a shallow copy for the mapping entries.
- * @num_maps: the number of maps in the mapping table
- */
-int pinctrl_register_mappings(struct pinctrl_map const *maps,
-			      unsigned num_maps)
+int pinctrl_register_map(struct pinctrl_map const *maps, unsigned num_maps,
+			 bool dup, bool locked)
 {
 	int i, ret;
 	struct pinctrl_maps *maps_node;
@@ -851,20 +854,52 @@ int pinctrl_register_mappings(struct pinctrl_map const *maps,
 	}
 
 	maps_node->num_maps = num_maps;
-	maps_node->maps = kmemdup(maps, sizeof(*maps) * num_maps, GFP_KERNEL);
-	if (!maps_node->maps) {
-		pr_err("failed to duplicate mapping table\n");
-		kfree(maps_node);
-		return -ENOMEM;
+	if (dup) {
+		maps_node->maps = kmemdup(maps, sizeof(*maps) * num_maps,
+					  GFP_KERNEL);
+		if (!maps_node->maps) {
+			pr_err("failed to duplicate mapping table\n");
+			kfree(maps_node);
+			return -ENOMEM;
+		}
+	} else {
+		maps_node->maps = maps;
 	}
 
-	mutex_lock(&pinctrl_mutex);
+	if (!locked)
+		mutex_lock(&pinctrl_mutex);
 	list_add_tail(&maps_node->node, &pinctrl_maps);
-	mutex_unlock(&pinctrl_mutex);
+	if (!locked)
+		mutex_unlock(&pinctrl_mutex);
 
 	return 0;
 }
 
+/**
+ * pinctrl_register_mappings() - register a set of pin controller mappings
+ * @maps: the pincontrol mappings table to register. This should probably be
+ *	marked with __initdata so it can be discarded after boot. This
+ *	function will perform a shallow copy for the mapping entries.
+ * @num_maps: the number of maps in the mapping table
+ */
+int pinctrl_register_mappings(struct pinctrl_map const *maps,
+			      unsigned num_maps)
+{
+	return pinctrl_register_map(maps, num_maps, true, false);
+}
+
+void pinctrl_unregister_map(struct pinctrl_map const *map)
+{
+	struct pinctrl_maps *maps_node;
+
+	list_for_each_entry(maps_node, &pinctrl_maps, node) {
+		if (maps_node->maps == map) {
+			list_del(&maps_node->node);
+			return;
+		}
+	}
+}
+
 #ifdef CONFIG_DEBUG_FS
 
 static int pinctrl_pins_show(struct seq_file *s, void *what)
@@ -1231,6 +1266,9 @@ static int pinctrl_check_ops(struct pinctrl_dev *pctldev)
 	    !ops->get_group_pins)
 		return -EINVAL;
 
+	if (ops->dt_node_to_map && !ops->dt_free_map)
+		return -EINVAL;
+
 	return 0;
 }
 
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 17ecf65..8fc8314 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -52,12 +52,15 @@ struct pinctrl_dev {
  * @dev: the device using this pin control handle
  * @states: a list of states for this device
  * @state: the current state
+ * @dt_maps: the mapping table chunks dynamically parsed from device tree for
+ *	this device, if any
  */
 struct pinctrl {
 	struct list_head node;
 	struct device *dev;
 	struct list_head states;
 	struct pinctrl_state *state;
+	struct list_head dt_maps;
 };
 
 /**
@@ -153,4 +156,9 @@ static inline struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev,
 	return radix_tree_lookup(&pctldev->pin_desc_tree, pin);
 }
 
+int pinctrl_register_map(struct pinctrl_map const *maps, unsigned num_maps,
+			 bool dup, bool locked);
+void pinctrl_unregister_map(struct pinctrl_map const *map);
+
 extern struct mutex pinctrl_mutex;
+extern struct list_head pinctrldev_list;
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
new file mode 100644
index 0000000..fe70b23
--- /dev/null
+++ b/drivers/pinctrl/devicetree.c
@@ -0,0 +1,265 @@
+/*
+ * Device tree integration for the pin control subsystem
+ *
+ * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/slab.h>
+
+#include "core.h"
+#include "devicetree.h"
+
+/**
+ * struct pinctrl_dt_map - mapping table chunk parsed from device tree
+ * @node: list node for struct pinctrl's @dt_maps field
+ * @pctldev: the pin controller that allocated this struct, and will free it
+ * @maps: the mapping table entries
+ */
+struct pinctrl_dt_map {
+	struct list_head node;
+	struct pinctrl_dev *pctldev;
+	struct pinctrl_map *map;
+	unsigned num_maps;
+};
+
+static void dt_free_map(struct pinctrl_dev *pctldev,
+		     struct pinctrl_map *map, unsigned num_maps)
+{
+	if (pctldev) {
+		struct pinctrl_ops *ops = pctldev->desc->pctlops;
+		ops->dt_free_map(pctldev, map, num_maps);
+	} else {
+		kfree(map);
+	}
+}
+
+void pinctrl_dt_free_maps(struct pinctrl *p)
+{
+	struct pinctrl_dt_map *dt_map, *n1;
+
+	list_for_each_entry_safe(dt_map, n1, &p->dt_maps, node) {
+		pinctrl_unregister_map(dt_map->map);
+		list_del(&dt_map->node);
+		dt_free_map(dt_map->pctldev, dt_map->map,
+			    dt_map->num_maps);
+		kfree(dt_map);
+	}
+
+	of_node_put(p->dev->of_node);
+}
+
+static int dt_remember_or_free_map(struct pinctrl *p, const char *statename,
+				   struct pinctrl_dev *pctldev,
+				   struct pinctrl_map *map, unsigned num_maps)
+{
+	int i;
+	struct pinctrl_dt_map *dt_map;
+
+	/* Initialize common mapping table entry fields */
+	for (i = 0; i < num_maps; i++) {
+		map[i].dev_name = dev_name(p->dev);
+		map[i].name = statename;
+		if (pctldev)
+			map[i].ctrl_dev_name = dev_name(pctldev->dev);
+	}
+
+	/* Remember the converted mapping table entries */
+	dt_map = kzalloc(sizeof(*dt_map), GFP_KERNEL);
+	if (!dt_map) {
+		dev_err(p->dev, "failed to alloc struct pinctrl_dt_map\n");
+		dt_free_map(pctldev, map, num_maps);
+		return -ENOMEM;
+	}
+
+	dt_map->pctldev = pctldev;
+	dt_map->map = map;
+	dt_map->num_maps = num_maps;
+	list_add_tail(&dt_map->node, &p->dt_maps);
+
+	return 0;
+}
+
+static struct pinctrl_dev *find_pinctrl_by_of_node(struct device_node *np)
+{
+	struct pinctrl_dev *pctldev;
+
+	list_for_each_entry(pctldev, &pinctrldev_list, node)
+		if (pctldev->dev->of_node == np)
+			return pctldev;
+
+	return NULL;
+}
+
+static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
+				struct device_node *np_config)
+{
+	struct device_node *np_pctldev;
+	struct pinctrl_dev *pctldev;
+	struct pinctrl_ops *ops;
+	int ret;
+	struct pinctrl_map *map;
+	unsigned num_maps;
+
+	/* Find the pin controller containing np_config */
+	np_pctldev = of_node_get(np_config);
+	for (;;) {
+		np_pctldev = of_get_next_parent(np_pctldev);
+		if (!np_pctldev || of_node_is_root(np_pctldev)) {
+			dev_err(p->dev, "could not find pctldev for node %s\n",
+				np_config->full_name);
+			of_node_put(np_pctldev);
+			/* FIXME: This should trigger deferrered probe */
+			return -ENODEV;
+		}
+		pctldev = find_pinctrl_by_of_node(np_pctldev);
+		if (pctldev)
+			break;
+	}
+	of_node_put(np_pctldev);
+
+	/*
+	 * Call pinctrl driver to parse device tree node, and
+	 * generate mapping table entries
+	 */
+	ops = pctldev->desc->pctlops;
+	if (!ops->dt_node_to_map) {
+		dev_err(p->dev, "pctldev %s doesn't support DT\n",
+			dev_name(pctldev->dev));
+		return -ENODEV;
+	}
+	ret = ops->dt_node_to_map(pctldev, np_config, &map, &num_maps);
+	if (ret < 0)
+		return ret;
+
+	/* Stash the mapping table chunk away for later use */
+	ret = dt_remember_or_free_map(p, statename, pctldev,
+					map, num_maps);
+	if (ret < 0)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int dt_remember_dummy_state(struct pinctrl *p, const char *statename)
+{
+	struct pinctrl_map *map;
+	int ret;
+
+	map = kzalloc(sizeof(*map), GFP_KERNEL);
+	if (!map) {
+		dev_err(p->dev, "failed to alloc struct pinctrl_map\n");
+		return -ENOMEM;
+	}
+
+	map->type = PIN_MAP_TYPE_DUMMY_STATE;
+
+	ret = dt_remember_or_free_map(p, statename, NULL, map, 1);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+int pinctrl_dt_to_map(struct pinctrl *p)
+{
+	struct device_node *np = p->dev->of_node;
+	int state, ret;
+	char *propname;
+	struct property *prop;
+	const char *statename;
+	const __be32 *list;
+	int size, config;
+	phandle phandle;
+	struct device_node *np_config;
+	struct pinctrl_dt_map *dt_map;
+
+	/* We may store pointers to property names within the node */
+	of_node_get(np);
+
+	/* For each defined state ID */
+	for (state = 0; ; state++) {
+		/* Retrieve the pinctrl-* property */
+		propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
+		prop = of_find_property(np, propname, &size);
+		kfree(propname);
+		if (!prop)
+			break;
+		list = prop->value;
+		size /= sizeof(*list);
+
+		/* Determine whether pinctrl-names property names the state */
+		ret = of_property_read_string_index(np, "pinctrl-names",
+						    state, &statename);
+		/*
+		 * If not, statename is just the integer state ID. But rather
+		 * than dynamically allocate it and have to free it later,
+		 * just point part way into the property name for the string.
+		 */
+		if (ret < 0) {
+			/* strlen("pinctrl-") == 8 */
+			if (strlen(prop->name) < 8) {
+				dev_err(p->dev, "prop %s inconsistent length\n",
+					prop->name);
+				ret = -EINVAL;
+				goto err;
+			}
+			statename = prop->name + 8;
+		}
+
+		/* For every referenced pin configuration node in it */
+		for (config = 0; config < size; config++) {
+			phandle = be32_to_cpup(list++);
+
+			/* Look up the pin configuration node */
+			np_config = of_find_node_by_phandle(phandle);
+			if (!np_config) {
+				dev_err(p->dev,
+					"prop %s index %i invalid phandle\n",
+					prop->name, config);
+				ret = -EINVAL;
+				goto err;
+			}
+
+			/* Parse the node */
+			ret = dt_to_map_one_config(p, statename, np_config);
+			of_node_put(np_config);
+			if (ret < 0)
+				goto err;
+		}
+
+		/* No entries in DT? Generate a dummy state table entry */
+		if (!size) {
+			ret = dt_remember_dummy_state(p, statename);
+			if (ret < 0)
+				goto err;
+		}
+	}
+
+	list_for_each_entry(dt_map, &p->dt_maps, node) {
+		ret = pinctrl_register_map(dt_map->map, dt_map->num_maps,
+					   false, true);
+		if (ret < 0)
+			goto err;
+	}
+
+	return 0;
+
+err:
+	pinctrl_dt_free_maps(p);
+	return ret;
+}
diff --git a/drivers/pinctrl/devicetree.h b/drivers/pinctrl/devicetree.h
new file mode 100644
index 0000000..760bc49
--- /dev/null
+++ b/drivers/pinctrl/devicetree.h
@@ -0,0 +1,35 @@
+/*
+ * Internal interface to pinctrl device tree integration
+ *
+ * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifdef CONFIG_OF
+
+void pinctrl_dt_free_maps(struct pinctrl *p);
+int pinctrl_dt_to_map(struct pinctrl *p);
+
+#else
+
+static inline int pinctrl_dt_to_map(struct pinctrl *p)
+{
+	return 0;
+}
+
+static inline void pinctrl_dt_free_maps(struct pinctrl *p)
+{
+}
+
+#endif
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 4e9f078..aa92cde 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -21,9 +21,11 @@
 
 struct device;
 struct pinctrl_dev;
+struct pinctrl_map;
 struct pinmux_ops;
 struct pinconf_ops;
 struct gpio_chip;
+struct device_node;
 
 /**
  * struct pinctrl_pin_desc - boards/machines provide information on their
@@ -83,6 +85,11 @@ struct pinctrl_ops {
 			       unsigned *num_pins);
 	void (*pin_dbg_show) (struct pinctrl_dev *pctldev, struct seq_file *s,
 			  unsigned offset);
+	int (*dt_node_to_map) (struct pinctrl_dev *pctldev,
+			       struct device_node *np_config,
+			       struct pinctrl_map **map, unsigned *num_maps);
+	void (*dt_free_map) (struct pinctrl_dev *pctldev,
+			     struct pinctrl_map *map, unsigned num_maps);
 };
 
 /**
-- 
1.7.0.4

Re: [PATCH V2 3/6] pinctrl: core device tree mapping table parsing support

[Dong Aisheng]

On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:
> During pinctrl_get(), if the client device has a device tree node, look
> for the common pinctrl properties there. If found, parse the referenced
> device tree nodes, with the help of the pinctrl drivers, and generate
> mapping table entries from them.
> 
> During pinctrl_put(), free any results of device tree parsing.
> 
> Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> ---
> v2: Place most code into new file devicetree.c
Much clearer.:)

> + * struct pinctrl_dt_map - mapping table chunk parsed from device tree
> + * @node: list node for struct pinctrl's @dt_maps field
> + * @pctldev: the pin controller that allocated this struct, and will free it
> + * @maps: the mapping table entries
> + */
> +struct pinctrl_dt_map {
> +	struct list_head node;
> +	struct pinctrl_dev *pctldev;
> +	struct pinctrl_map *map;
> +	unsigned num_maps;
> +};
> +
> +static void dt_free_map(struct pinctrl_dev *pctldev,
> +		     struct pinctrl_map *map, unsigned num_maps)
> +{
> +	if (pctldev) {
> +		struct pinctrl_ops *ops = pctldev->desc->pctlops;
> +		ops->dt_free_map(pctldev, map, num_maps);
> +	} else {
I remember for hog on functions the pctldev becomes pinctrl devices itself,
so in which case pctldev can be NULL?

> +static struct pinctrl_dev *find_pinctrl_by_of_node(struct device_node *np)
> +{
> +	struct pinctrl_dev *pctldev;
> +
> +	list_for_each_entry(pctldev, &pinctrldev_list, node)
> +		if (pctldev->dev->of_node == np)
> +			return pctldev;
> +
> +	return NULL;
> +}
> +
> +static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
> +				struct device_node *np_config)
> +{
> +	struct device_node *np_pctldev;
> +	struct pinctrl_dev *pctldev;
> +	struct pinctrl_ops *ops;
> +	int ret;
> +	struct pinctrl_map *map;
> +	unsigned num_maps;
> +
> +	/* Find the pin controller containing np_config */
> +	np_pctldev = of_node_get(np_config);
It seems the np_config node is already got when call of_find_node_by_phandle.
So do we still need this line?

> +int pinctrl_dt_to_map(struct pinctrl *p)
> +{
> +	struct device_node *np = p->dev->of_node;
> +	int state, ret;
> +	char *propname;
> +	struct property *prop;
> +	const char *statename;
> +	const __be32 *list;
> +	int size, config;
> +	phandle phandle;
> +	struct device_node *np_config;
> +	struct pinctrl_dt_map *dt_map;
> +
Add NULL np checking?

> +	/* We may store pointers to property names within the node */
> +	of_node_get(np);
> +
> +	/* For each defined state ID */
> +	for (state = 0; ; state++) {
> +		/* Retrieve the pinctrl-* property */
> +		propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
> +		prop = of_find_property(np, propname, &size);
> +		kfree(propname);
> +		if (!prop)
> +			break;
> +		list = prop->value;
> +		size /= sizeof(*list);
> +
> +		/* Determine whether pinctrl-names property names the state */
> +		ret = of_property_read_string_index(np, "pinctrl-names",
> +						    state, &statename);
> +		/*
> +		 * If not, statename is just the integer state ID. But rather
> +		 * than dynamically allocate it and have to free it later,
> +		 * just point part way into the property name for the string.
> +		 */
> +		if (ret < 0) {
> +			/* strlen("pinctrl-") == 8 */
> +			if (strlen(prop->name) < 8) {
Do we really need this extra checking?
It seems the prop->name is the "pinctrl-%d" you searched above, so the
strlen(prop->name) must not < 8, right?

> +				dev_err(p->dev, "prop %s inconsistent length\n",
> +					prop->name);
> +				ret = -EINVAL;
> +				goto err;
> +			}
> +			statename = prop->name + 8;
From this code, it seems actually we provide user the option by chance to define
state as pinctrl-syspend which is out of our binding doc.

> +		}
> +
> +		/* For every referenced pin configuration node in it */
> +		for (config = 0; config < size; config++) {
> +			phandle = be32_to_cpup(list++);
> +
> +			/* Look up the pin configuration node */
> +			np_config = of_find_node_by_phandle(phandle);
One option is using of_parse_phandle, then we do not need calculate
the phandle offset by ourselves.
Like:
np_config = of_parse_phandle(propname , config);

> +			if (!np_config) {
> +				dev_err(p->dev,
> +					"prop %s index %i invalid phandle\n",
> +					prop->name, config);
> +				ret = -EINVAL;
> +				goto err;
> +			}
> +
> +			/* Parse the node */
> +			ret = dt_to_map_one_config(p, statename, np_config);
> +			of_node_put(np_config);
> +			if (ret < 0)
> +				goto err;
> +		}
> +
> +		/* No entries in DT? Generate a dummy state table entry */
> +		if (!size) {
> +			ret = dt_remember_dummy_state(p, statename);
> +			if (ret < 0)
> +				goto err;
> +		}
> +	}
> +
> +	list_for_each_entry(dt_map, &p->dt_maps, node) {
> +		ret = pinctrl_register_map(dt_map->map, dt_map->num_maps,
> +					   false, true);
> +		if (ret < 0)
> +			goto err;
> +	}
What's main purpose of differing the map registration and introduce a
intermediate pinctrl_dt_map(dt_remember_or_free_map)?
What about directly register maps once it's parsed?

Regards
Dong Aisheng

Re: [PATCH V2 3/6] pinctrl: core device tree mapping table parsing support

[Dong Aisheng]

On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:
> During pinctrl_get(), if the client device has a device tree node, look
> for the common pinctrl properties there. If found, parse the referenced
> device tree nodes, with the help of the pinctrl drivers, and generate
> mapping table entries from them.
> 
> During pinctrl_put(), free any results of device tree parsing.
> 
> Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> ---
> v2: Place most code into new file devicetree.c
Much clearer.:)

> + * struct pinctrl_dt_map - mapping table chunk parsed from device tree
> + * @node: list node for struct pinctrl's @dt_maps field
> + * @pctldev: the pin controller that allocated this struct, and will free it
> + * @maps: the mapping table entries
> + */
> +struct pinctrl_dt_map {
> +	struct list_head node;
> +	struct pinctrl_dev *pctldev;
> +	struct pinctrl_map *map;
> +	unsigned num_maps;
> +};
> +
> +static void dt_free_map(struct pinctrl_dev *pctldev,
> +		     struct pinctrl_map *map, unsigned num_maps)
> +{
> +	if (pctldev) {
> +		struct pinctrl_ops *ops = pctldev->desc->pctlops;
> +		ops->dt_free_map(pctldev, map, num_maps);
> +	} else {
I remember for hog on functions the pctldev becomes pinctrl devices itself,
so in which case pctldev can be NULL?

> +static struct pinctrl_dev *find_pinctrl_by_of_node(struct device_node *np)
> +{
> +	struct pinctrl_dev *pctldev;
> +
> +	list_for_each_entry(pctldev, &pinctrldev_list, node)
> +		if (pctldev->dev->of_node == np)
> +			return pctldev;
> +
> +	return NULL;
> +}
> +
> +static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
> +				struct device_node *np_config)
> +{
> +	struct device_node *np_pctldev;
> +	struct pinctrl_dev *pctldev;
> +	struct pinctrl_ops *ops;
> +	int ret;
> +	struct pinctrl_map *map;
> +	unsigned num_maps;
> +
> +	/* Find the pin controller containing np_config */
> +	np_pctldev = of_node_get(np_config);
It seems the np_config node is already got when call of_find_node_by_phandle.
So do we still need this line?

> +int pinctrl_dt_to_map(struct pinctrl *p)
> +{
> +	struct device_node *np = p->dev->of_node;
> +	int state, ret;
> +	char *propname;
> +	struct property *prop;
> +	const char *statename;
> +	const __be32 *list;
> +	int size, config;
> +	phandle phandle;
> +	struct device_node *np_config;
> +	struct pinctrl_dt_map *dt_map;
> +
Add NULL np checking?

> +	/* We may store pointers to property names within the node */
> +	of_node_get(np);
> +
> +	/* For each defined state ID */
> +	for (state = 0; ; state++) {
> +		/* Retrieve the pinctrl-* property */
> +		propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
> +		prop = of_find_property(np, propname, &size);
> +		kfree(propname);
> +		if (!prop)
> +			break;
> +		list = prop->value;
> +		size /= sizeof(*list);
> +
> +		/* Determine whether pinctrl-names property names the state */
> +		ret = of_property_read_string_index(np, "pinctrl-names",
> +						    state, &statename);
> +		/*
> +		 * If not, statename is just the integer state ID. But rather
> +		 * than dynamically allocate it and have to free it later,
> +		 * just point part way into the property name for the string.
> +		 */
> +		if (ret < 0) {
> +			/* strlen("pinctrl-") == 8 */
> +			if (strlen(prop->name) < 8) {
Do we really need this extra checking?
It seems the prop->name is the "pinctrl-%d" you searched above, so the
strlen(prop->name) must not < 8, right?

> +				dev_err(p->dev, "prop %s inconsistent length\n",
> +					prop->name);
> +				ret = -EINVAL;
> +				goto err;
> +			}
> +			statename = prop->name + 8;
>From this code, it seems actually we provide user the option by chance to define
state as pinctrl-syspend which is out of our binding doc.

> +		}
> +
> +		/* For every referenced pin configuration node in it */
> +		for (config = 0; config < size; config++) {
> +			phandle = be32_to_cpup(list++);
> +
> +			/* Look up the pin configuration node */
> +			np_config = of_find_node_by_phandle(phandle);
One option is using of_parse_phandle, then we do not need calculate
the phandle offset by ourselves.
Like:
np_config = of_parse_phandle(propname , config);

> +			if (!np_config) {
> +				dev_err(p->dev,
> +					"prop %s index %i invalid phandle\n",
> +					prop->name, config);
> +				ret = -EINVAL;
> +				goto err;
> +			}
> +
> +			/* Parse the node */
> +			ret = dt_to_map_one_config(p, statename, np_config);
> +			of_node_put(np_config);
> +			if (ret < 0)
> +				goto err;
> +		}
> +
> +		/* No entries in DT? Generate a dummy state table entry */
> +		if (!size) {
> +			ret = dt_remember_dummy_state(p, statename);
> +			if (ret < 0)
> +				goto err;
> +		}
> +	}
> +
> +	list_for_each_entry(dt_map, &p->dt_maps, node) {
> +		ret = pinctrl_register_map(dt_map->map, dt_map->num_maps,
> +					   false, true);
> +		if (ret < 0)
> +			goto err;
> +	}
What's main purpose of differing the map registration and introduce a
intermediate pinctrl_dt_map(dt_remember_or_free_map)?
What about directly register maps once it's parsed?

Regards
Dong Aisheng

Re: [PATCH V2 3/6] pinctrl: core device tree mapping table parsing support

[Dong Aisheng]

On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:
> During pinctrl_get(), if the client device has a device tree node, look
> for the common pinctrl properties there. If found, parse the referenced
> device tree nodes, with the help of the pinctrl drivers, and generate
> mapping table entries from them.
> 
> During pinctrl_put(), free any results of device tree parsing.
> 
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
> v2: Place most code into new file devicetree.c
Much clearer.:)

> + * struct pinctrl_dt_map - mapping table chunk parsed from device tree
> + * @node: list node for struct pinctrl's @dt_maps field
> + * @pctldev: the pin controller that allocated this struct, and will free it
> + * @maps: the mapping table entries
> + */
> +struct pinctrl_dt_map {
> +	struct list_head node;
> +	struct pinctrl_dev *pctldev;
> +	struct pinctrl_map *map;
> +	unsigned num_maps;
> +};
> +
> +static void dt_free_map(struct pinctrl_dev *pctldev,
> +		     struct pinctrl_map *map, unsigned num_maps)
> +{
> +	if (pctldev) {
> +		struct pinctrl_ops *ops = pctldev->desc->pctlops;
> +		ops->dt_free_map(pctldev, map, num_maps);
> +	} else {
I remember for hog on functions the pctldev becomes pinctrl devices itself,
so in which case pctldev can be NULL?

> +static struct pinctrl_dev *find_pinctrl_by_of_node(struct device_node *np)
> +{
> +	struct pinctrl_dev *pctldev;
> +
> +	list_for_each_entry(pctldev, &pinctrldev_list, node)
> +		if (pctldev->dev->of_node == np)
> +			return pctldev;
> +
> +	return NULL;
> +}
> +
> +static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
> +				struct device_node *np_config)
> +{
> +	struct device_node *np_pctldev;
> +	struct pinctrl_dev *pctldev;
> +	struct pinctrl_ops *ops;
> +	int ret;
> +	struct pinctrl_map *map;
> +	unsigned num_maps;
> +
> +	/* Find the pin controller containing np_config */
> +	np_pctldev = of_node_get(np_config);
It seems the np_config node is already got when call of_find_node_by_phandle.
So do we still need this line?

> +int pinctrl_dt_to_map(struct pinctrl *p)
> +{
> +	struct device_node *np = p->dev->of_node;
> +	int state, ret;
> +	char *propname;
> +	struct property *prop;
> +	const char *statename;
> +	const __be32 *list;
> +	int size, config;
> +	phandle phandle;
> +	struct device_node *np_config;
> +	struct pinctrl_dt_map *dt_map;
> +
Add NULL np checking?

> +	/* We may store pointers to property names within the node */
> +	of_node_get(np);
> +
> +	/* For each defined state ID */
> +	for (state = 0; ; state++) {
> +		/* Retrieve the pinctrl-* property */
> +		propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
> +		prop = of_find_property(np, propname, &size);
> +		kfree(propname);
> +		if (!prop)
> +			break;
> +		list = prop->value;
> +		size /= sizeof(*list);
> +
> +		/* Determine whether pinctrl-names property names the state */
> +		ret = of_property_read_string_index(np, "pinctrl-names",
> +						    state, &statename);
> +		/*
> +		 * If not, statename is just the integer state ID. But rather
> +		 * than dynamically allocate it and have to free it later,
> +		 * just point part way into the property name for the string.
> +		 */
> +		if (ret < 0) {
> +			/* strlen("pinctrl-") == 8 */
> +			if (strlen(prop->name) < 8) {
Do we really need this extra checking?
It seems the prop->name is the "pinctrl-%d" you searched above, so the
strlen(prop->name) must not < 8, right?

> +				dev_err(p->dev, "prop %s inconsistent length\n",
> +					prop->name);
> +				ret = -EINVAL;
> +				goto err;
> +			}
> +			statename = prop->name + 8;
>From this code, it seems actually we provide user the option by chance to define
state as pinctrl-syspend which is out of our binding doc.

> +		}
> +
> +		/* For every referenced pin configuration node in it */
> +		for (config = 0; config < size; config++) {
> +			phandle = be32_to_cpup(list++);
> +
> +			/* Look up the pin configuration node */
> +			np_config = of_find_node_by_phandle(phandle);
One option is using of_parse_phandle, then we do not need calculate
the phandle offset by ourselves.
Like:
np_config = of_parse_phandle(propname , config);

> +			if (!np_config) {
> +				dev_err(p->dev,
> +					"prop %s index %i invalid phandle\n",
> +					prop->name, config);
> +				ret = -EINVAL;
> +				goto err;
> +			}
> +
> +			/* Parse the node */
> +			ret = dt_to_map_one_config(p, statename, np_config);
> +			of_node_put(np_config);
> +			if (ret < 0)
> +				goto err;
> +		}
> +
> +		/* No entries in DT? Generate a dummy state table entry */
> +		if (!size) {
> +			ret = dt_remember_dummy_state(p, statename);
> +			if (ret < 0)
> +				goto err;
> +		}
> +	}
> +
> +	list_for_each_entry(dt_map, &p->dt_maps, node) {
> +		ret = pinctrl_register_map(dt_map->map, dt_map->num_maps,
> +					   false, true);
> +		if (ret < 0)
> +			goto err;
> +	}
What's main purpose of differing the map registration and introduce a
intermediate pinctrl_dt_map(dt_remember_or_free_map)?
What about directly register maps once it's parsed?

Regards
Dong Aisheng

Re: [PATCH V2 3/6] pinctrl: core device tree mapping table parsing support

[Stephen Warren]

On 03/21/2012 01:31 AM, Dong Aisheng wrote:
> On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:
>> During pinctrl_get(), if the client device has a device tree node, look
>> for the common pinctrl properties there. If found, parse the referenced
>> device tree nodes, with the help of the pinctrl drivers, and generate
>> mapping table entries from them.
>>
>> During pinctrl_put(), free any results of device tree parsing.

>> +static void dt_free_map(struct pinctrl_dev *pctldev,
>> +		     struct pinctrl_map *map, unsigned num_maps)
>> +{
>> +	if (pctldev) {
>> +		struct pinctrl_ops *ops = pctldev->desc->pctlops;
>> +		ops->dt_free_map(pctldev, map, num_maps);
>> +	} else {
>
> I remember for hog on functions the pctldev becomes pinctrl devices itself,
> so in which case pctldev can be NULL?

PIN_MAP_TYPE_DUMMY_STATE has no pctldev.

>> +static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
>> +				struct device_node *np_config)
>> +{
>> +	struct device_node *np_pctldev;
>> +	struct pinctrl_dev *pctldev;
>> +	struct pinctrl_ops *ops;
>> +	int ret;
>> +	struct pinctrl_map *map;
>> +	unsigned num_maps;
>> +
>> +	/* Find the pin controller containing np_config */
>> +	np_pctldev = of_node_get(np_config);
>
> It seems the np_config node is already got when call of_find_node_by_phandle.
> So do we still need this line?

Right below that code, we traverse up the tree using
of_get_next_parent(). Internally, this calls of_node_put() on the node
pointer that's passed in. Hence, we need an extra get() to match this.

>> +int pinctrl_dt_to_map(struct pinctrl *p)
>> +{
>> +	struct device_node *np = p->dev->of_node;
>> +	int state, ret;
>> +	char *propname;
>> +	struct property *prop;
>> +	const char *statename;
>> +	const __be32 *list;
>> +	int size, config;
>> +	phandle phandle;
>> +	struct device_node *np_config;
>> +	struct pinctrl_dt_map *dt_map;
>
> Add NULL np checking?

Oops yes. I though I had that somewhere, but evidently not...

>> +	/* We may store pointers to property names within the node */
>> +	of_node_get(np);
>> +
>> +	/* For each defined state ID */
>> +	for (state = 0; ; state++) {
>> +		/* Retrieve the pinctrl-* property */
>> +		propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
>> +		prop = of_find_property(np, propname, &size);
...
>> +			/* strlen("pinctrl-") == 8 */
>> +			if (strlen(prop->name) < 8) {
>
> Do we really need this extra checking?
> It seems the prop->name is the "pinctrl-%d" you searched above, so the
> strlen(prop->name) must not < 8, right?

Assuming of_find_property works correctly, I guess that's true. We can
remove that if check.

>> +				dev_err(p->dev, "prop %s inconsistent length\n",
>> +					prop->name);
>> +				ret = -EINVAL;
>> +				goto err;
>> +			}
>> +			statename = prop->name + 8;
>
> From this code, it seems actually we provide user the option by chance to define
> state as pinctrl-syspend which is out of our binding doc.

The user can place a property with name "pinctrl-suspend" into the DT.
However, since we only look for properties named pinctrl-%d, then the
code will never read/use it, just like any other unexpected property.

>> +		}
>> +
>> +		/* For every referenced pin configuration node in it */
>> +		for (config = 0; config < size; config++) {
>> +			phandle = be32_to_cpup(list++);
>> +
>> +			/* Look up the pin configuration node */
>> +			np_config = of_find_node_by_phandle(phandle);
>
> One option is using of_parse_phandle, then we do not need calculate
> the phandle offset by ourselves.
> Like:
> np_config = of_parse_phandle(propname , config);

Yes, that's a good idea. I'll try that.

>> +			if (!np_config) {
>> +				dev_err(p->dev,
>> +					"prop %s index %i invalid phandle\n",
>> +					prop->name, config);
>> +				ret = -EINVAL;
>> +				goto err;
>> +			}
>> +
>> +			/* Parse the node */
>> +			ret = dt_to_map_one_config(p, statename, np_config);
>> +			of_node_put(np_config);
>> +			if (ret < 0)
>> +				goto err;
>> +		}
>> +
>> +		/* No entries in DT? Generate a dummy state table entry */
>> +		if (!size) {
>> +			ret = dt_remember_dummy_state(p, statename);
>> +			if (ret < 0)
>> +				goto err;
>> +		}
>> +	}
>> +
>> +	list_for_each_entry(dt_map, &p->dt_maps, node) {
>> +		ret = pinctrl_register_map(dt_map->map, dt_map->num_maps,
>> +					   false, true);
>> +		if (ret < 0)
>> +			goto err;
>> +	}
>
> What's main purpose of differing the map registration and introduce a
> intermediate pinctrl_dt_map(dt_remember_or_free_map)?
> What about directly register maps once it's parsed?

s/differing/deferring/ I assume.

IIRC, this was mainly to simplify error handling; by deferring it, you
don't have to unregister everything when undoing a failed parse.
However, I guess that pinctrl_dt_free_maps() already cleans up
everything anyway, so we couuld just register everything as soon as its
parsed. I'll think a little more about this and switch to doing that if
will work.

Re: [PATCH V2 3/6] pinctrl: core device tree mapping table parsing support

[Stephen Warren]

On 03/21/2012 01:31 AM, Dong Aisheng wrote:
> On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:
>> During pinctrl_get(), if the client device has a device tree node, look
>> for the common pinctrl properties there. If found, parse the referenced
>> device tree nodes, with the help of the pinctrl drivers, and generate
>> mapping table entries from them.
>>
>> During pinctrl_put(), free any results of device tree parsing.

>> +static void dt_free_map(struct pinctrl_dev *pctldev,
>> +		     struct pinctrl_map *map, unsigned num_maps)
>> +{
>> +	if (pctldev) {
>> +		struct pinctrl_ops *ops = pctldev->desc->pctlops;
>> +		ops->dt_free_map(pctldev, map, num_maps);
>> +	} else {
>
> I remember for hog on functions the pctldev becomes pinctrl devices itself,
> so in which case pctldev can be NULL?

PIN_MAP_TYPE_DUMMY_STATE has no pctldev.

>> +static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
>> +				struct device_node *np_config)
>> +{
>> +	struct device_node *np_pctldev;
>> +	struct pinctrl_dev *pctldev;
>> +	struct pinctrl_ops *ops;
>> +	int ret;
>> +	struct pinctrl_map *map;
>> +	unsigned num_maps;
>> +
>> +	/* Find the pin controller containing np_config */
>> +	np_pctldev = of_node_get(np_config);
>
> It seems the np_config node is already got when call of_find_node_by_phandle.
> So do we still need this line?

Right below that code, we traverse up the tree using
of_get_next_parent(). Internally, this calls of_node_put() on the node
pointer that's passed in. Hence, we need an extra get() to match this.

>> +int pinctrl_dt_to_map(struct pinctrl *p)
>> +{
>> +	struct device_node *np = p->dev->of_node;
>> +	int state, ret;
>> +	char *propname;
>> +	struct property *prop;
>> +	const char *statename;
>> +	const __be32 *list;
>> +	int size, config;
>> +	phandle phandle;
>> +	struct device_node *np_config;
>> +	struct pinctrl_dt_map *dt_map;
>
> Add NULL np checking?

Oops yes. I though I had that somewhere, but evidently not...

>> +	/* We may store pointers to property names within the node */
>> +	of_node_get(np);
>> +
>> +	/* For each defined state ID */
>> +	for (state = 0; ; state++) {
>> +		/* Retrieve the pinctrl-* property */
>> +		propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
>> +		prop = of_find_property(np, propname, &size);
...
>> +			/* strlen("pinctrl-") == 8 */
>> +			if (strlen(prop->name) < 8) {
>
> Do we really need this extra checking?
> It seems the prop->name is the "pinctrl-%d" you searched above, so the
> strlen(prop->name) must not < 8, right?

Assuming of_find_property works correctly, I guess that's true. We can
remove that if check.

>> +				dev_err(p->dev, "prop %s inconsistent length\n",
>> +					prop->name);
>> +				ret = -EINVAL;
>> +				goto err;
>> +			}
>> +			statename = prop->name + 8;
>
> From this code, it seems actually we provide user the option by chance to define
> state as pinctrl-syspend which is out of our binding doc.

The user can place a property with name "pinctrl-suspend" into the DT.
However, since we only look for properties named pinctrl-%d, then the
code will never read/use it, just like any other unexpected property.

>> +		}
>> +
>> +		/* For every referenced pin configuration node in it */
>> +		for (config = 0; config < size; config++) {
>> +			phandle = be32_to_cpup(list++);
>> +
>> +			/* Look up the pin configuration node */
>> +			np_config = of_find_node_by_phandle(phandle);
>
> One option is using of_parse_phandle, then we do not need calculate
> the phandle offset by ourselves.
> Like:
> np_config = of_parse_phandle(propname , config);

Yes, that's a good idea. I'll try that.

>> +			if (!np_config) {
>> +				dev_err(p->dev,
>> +					"prop %s index %i invalid phandle\n",
>> +					prop->name, config);
>> +				ret = -EINVAL;
>> +				goto err;
>> +			}
>> +
>> +			/* Parse the node */
>> +			ret = dt_to_map_one_config(p, statename, np_config);
>> +			of_node_put(np_config);
>> +			if (ret < 0)
>> +				goto err;
>> +		}
>> +
>> +		/* No entries in DT? Generate a dummy state table entry */
>> +		if (!size) {
>> +			ret = dt_remember_dummy_state(p, statename);
>> +			if (ret < 0)
>> +				goto err;
>> +		}
>> +	}
>> +
>> +	list_for_each_entry(dt_map, &p->dt_maps, node) {
>> +		ret = pinctrl_register_map(dt_map->map, dt_map->num_maps,
>> +					   false, true);
>> +		if (ret < 0)
>> +			goto err;
>> +	}
>
> What's main purpose of differing the map registration and introduce a
> intermediate pinctrl_dt_map(dt_remember_or_free_map)?
> What about directly register maps once it's parsed?

s/differing/deferring/ I assume.

IIRC, this was mainly to simplify error handling; by deferring it, you
don't have to unregister everything when undoing a failed parse.
However, I guess that pinctrl_dt_free_maps() already cleans up
everything anyway, so we couuld just register everything as soon as its
parsed. I'll think a little more about this and switch to doing that if
will work.

Re: [PATCH V2 3/6] pinctrl: core device tree mapping table parsing support

[Stephen Warren]

On 03/21/2012 09:48 AM, Stephen Warren wrote:
> On 03/21/2012 01:31 AM, Dong Aisheng wrote:
>> On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:
...
>>> +int pinctrl_dt_to_map(struct pinctrl *p)
>>> +{
>>> +	struct device_node *np = p->dev->of_node;
>>> +	int state, ret;
>>> +	char *propname;
>>> +	struct property *prop;
>>> +	const char *statename;
>>> +	const __be32 *list;
>>> +	int size, config;
>>> +	phandle phandle;
>>> +	struct device_node *np_config;
>>> +	struct pinctrl_dt_map *dt_map;
>>
>> Add NULL np checking?
> 
> Oops yes. I though I had that somewhere, but evidently not...

It turns out this isn't needed; of_node_get() and of_find_property()
both handle a NULL np just fine. Still, I suppose this might not always
be true for arbitrary code that's in pinctrl_dt_to_map(), so perhaps we
should add this anyway.

>>> +		}
>>> +
>>> +		/* For every referenced pin configuration node in it */
>>> +		for (config = 0; config < size; config++) {
>>> +			phandle = be32_to_cpup(list++);
>>> +
>>> +			/* Look up the pin configuration node */
>>> +			np_config = of_find_node_by_phandle(phandle);
>>
>> One option is using of_parse_phandle, then we do not need calculate
>> the phandle offset by ourselves.
>> Like:
>> np_config = of_parse_phandle(propname , config);
> 
> Yes, that's a good idea. I'll try that.

I looked at this more, and the existing code is a lot more efficient;
of_parse_phandle() internally calls of_find_property() each time, which
this pinctrl code has already done. I'd rather just leave this as it is.
Are you OK with that?

Re: [PATCH V2 3/6] pinctrl: core device tree mapping table parsing support

[Stephen Warren]

On 03/21/2012 09:48 AM, Stephen Warren wrote:
> On 03/21/2012 01:31 AM, Dong Aisheng wrote:
>> On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:
...
>>> +int pinctrl_dt_to_map(struct pinctrl *p)
>>> +{
>>> +	struct device_node *np = p->dev->of_node;
>>> +	int state, ret;
>>> +	char *propname;
>>> +	struct property *prop;
>>> +	const char *statename;
>>> +	const __be32 *list;
>>> +	int size, config;
>>> +	phandle phandle;
>>> +	struct device_node *np_config;
>>> +	struct pinctrl_dt_map *dt_map;
>>
>> Add NULL np checking?
> 
> Oops yes. I though I had that somewhere, but evidently not...

It turns out this isn't needed; of_node_get() and of_find_property()
both handle a NULL np just fine. Still, I suppose this might not always
be true for arbitrary code that's in pinctrl_dt_to_map(), so perhaps we
should add this anyway.

>>> +		}
>>> +
>>> +		/* For every referenced pin configuration node in it */
>>> +		for (config = 0; config < size; config++) {
>>> +			phandle = be32_to_cpup(list++);
>>> +
>>> +			/* Look up the pin configuration node */
>>> +			np_config = of_find_node_by_phandle(phandle);
>>
>> One option is using of_parse_phandle, then we do not need calculate
>> the phandle offset by ourselves.
>> Like:
>> np_config = of_parse_phandle(propname , config);
> 
> Yes, that's a good idea. I'll try that.

I looked at this more, and the existing code is a lot more efficient;
of_parse_phandle() internally calls of_find_property() each time, which
this pinctrl code has already done. I'd rather just leave this as it is.
Are you OK with that?

Re: [PATCH V2 3/6] pinctrl: core device tree mapping table parsing support

[Dong Aisheng]

On Thu, Mar 22, 2012 at 01:25:25AM +0800, Stephen Warren wrote:
> On 03/21/2012 09:48 AM, Stephen Warren wrote:
> > On 03/21/2012 01:31 AM, Dong Aisheng wrote:
> >> On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:
> ...
> >>> +int pinctrl_dt_to_map(struct pinctrl *p)
> >>> +{
> >>> +	struct device_node *np = p->dev->of_node;
> >>> +	int state, ret;
> >>> +	char *propname;
> >>> +	struct property *prop;
> >>> +	const char *statename;
> >>> +	const __be32 *list;
> >>> +	int size, config;
> >>> +	phandle phandle;
> >>> +	struct device_node *np_config;
> >>> +	struct pinctrl_dt_map *dt_map;
> >>
> >> Add NULL np checking?
> > 
> > Oops yes. I though I had that somewhere, but evidently not...
> 
> It turns out this isn't needed; of_node_get() and of_find_property()
> both handle a NULL np just fine. Still, I suppose this might not always
> be true for arbitrary code that's in pinctrl_dt_to_map(), so perhaps we
> should add this anyway.
> 
Yes, and it's meaningless to keep going forward if np is NULL.

> >>> +		}
> >>> +
> >>> +		/* For every referenced pin configuration node in it */
> >>> +		for (config = 0; config < size; config++) {
> >>> +			phandle = be32_to_cpup(list++);
> >>> +
> >>> +			/* Look up the pin configuration node */
> >>> +			np_config = of_find_node_by_phandle(phandle);
> >>
> >> One option is using of_parse_phandle, then we do not need calculate
> >> the phandle offset by ourselves.
> >> Like:
> >> np_config = of_parse_phandle(propname , config);
> > 
> > Yes, that's a good idea. I'll try that.
> 
> I looked at this more, and the existing code is a lot more efficient;
> of_parse_phandle() internally calls of_find_property() each time, which
> this pinctrl code has already done. I'd rather just leave this as it is.
> Are you OK with that?
> 
Yes, i'm ok with it.

Regards
Dong Aisheng

Re: [PATCH V2 3/6] pinctrl: core device tree mapping table parsing support

[Dong Aisheng]

On Thu, Mar 22, 2012 at 01:25:25AM +0800, Stephen Warren wrote:
> On 03/21/2012 09:48 AM, Stephen Warren wrote:
> > On 03/21/2012 01:31 AM, Dong Aisheng wrote:
> >> On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:
> ...
> >>> +int pinctrl_dt_to_map(struct pinctrl *p)
> >>> +{
> >>> +	struct device_node *np = p->dev->of_node;
> >>> +	int state, ret;
> >>> +	char *propname;
> >>> +	struct property *prop;
> >>> +	const char *statename;
> >>> +	const __be32 *list;
> >>> +	int size, config;
> >>> +	phandle phandle;
> >>> +	struct device_node *np_config;
> >>> +	struct pinctrl_dt_map *dt_map;
> >>
> >> Add NULL np checking?
> > 
> > Oops yes. I though I had that somewhere, but evidently not...
> 
> It turns out this isn't needed; of_node_get() and of_find_property()
> both handle a NULL np just fine. Still, I suppose this might not always
> be true for arbitrary code that's in pinctrl_dt_to_map(), so perhaps we
> should add this anyway.
> 
Yes, and it's meaningless to keep going forward if np is NULL.

> >>> +		}
> >>> +
> >>> +		/* For every referenced pin configuration node in it */
> >>> +		for (config = 0; config < size; config++) {
> >>> +			phandle = be32_to_cpup(list++);
> >>> +
> >>> +			/* Look up the pin configuration node */
> >>> +			np_config = of_find_node_by_phandle(phandle);
> >>
> >> One option is using of_parse_phandle, then we do not need calculate
> >> the phandle offset by ourselves.
> >> Like:
> >> np_config = of_parse_phandle(propname , config);
> > 
> > Yes, that's a good idea. I'll try that.
> 
> I looked at this more, and the existing code is a lot more efficient;
> of_parse_phandle() internally calls of_find_property() each time, which
> this pinctrl code has already done. I'd rather just leave this as it is.
> Are you OK with that?
> 
Yes, i'm ok with it.

Regards
Dong Aisheng

Re: [PATCH V2 3/6] pinctrl: core device tree mapping table parsing support

[Dong Aisheng]

On Wed, Mar 21, 2012 at 11:48:20PM +0800, Stephen Warren wrote:
> On 03/21/2012 01:31 AM, Dong Aisheng wrote:
> > On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:
> >> During pinctrl_get(), if the client device has a device tree node, look
> >> for the common pinctrl properties there. If found, parse the referenced
> >> device tree nodes, with the help of the pinctrl drivers, and generate
> >> mapping table entries from them.
> >>
> >> During pinctrl_put(), free any results of device tree parsing.
> 
> >> +static void dt_free_map(struct pinctrl_dev *pctldev,
> >> +		     struct pinctrl_map *map, unsigned num_maps)
> >> +{
> >> +	if (pctldev) {
> >> +		struct pinctrl_ops *ops = pctldev->desc->pctlops;
> >> +		ops->dt_free_map(pctldev, map, num_maps);
> >> +	} else {
> >
> > I remember for hog on functions the pctldev becomes pinctrl devices itself,
> > so in which case pctldev can be NULL?
> 
> PIN_MAP_TYPE_DUMMY_STATE has no pctldev.
> 
Oh, get it now.
Maybe we could a line of comment in the generating dummy state code telling about
this.

> >> +static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
> >> +				struct device_node *np_config)
> >> +{
> >> +	struct device_node *np_pctldev;
> >> +	struct pinctrl_dev *pctldev;
> >> +	struct pinctrl_ops *ops;
> >> +	int ret;
> >> +	struct pinctrl_map *map;
> >> +	unsigned num_maps;
> >> +
> >> +	/* Find the pin controller containing np_config */
> >> +	np_pctldev = of_node_get(np_config);
> >
> > It seems the np_config node is already got when call of_find_node_by_phandle.
> > So do we still need this line?
> 
> Right below that code, we traverse up the tree using
> of_get_next_parent(). Internally, this calls of_node_put() on the node
> pointer that's passed in. Hence, we need an extra get() to match this.
> 
Yes, it's true.
So it's reasonable here.

> >> +int pinctrl_dt_to_map(struct pinctrl *p)
> >> +{
> >> +	struct device_node *np = p->dev->of_node;
> >> +	int state, ret;
> >> +	char *propname;
> >> +	struct property *prop;
> >> +	const char *statename;
> >> +	const __be32 *list;
> >> +	int size, config;
> >> +	phandle phandle;
> >> +	struct device_node *np_config;
> >> +	struct pinctrl_dt_map *dt_map;
> >
> > Add NULL np checking?
> 
> Oops yes. I though I had that somewhere, but evidently not...
> 
> >> +	/* We may store pointers to property names within the node */
> >> +	of_node_get(np);
> >> +
> >> +	/* For each defined state ID */
> >> +	for (state = 0; ; state++) {
> >> +		/* Retrieve the pinctrl-* property */
> >> +		propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
> >> +		prop = of_find_property(np, propname, &size);
> ...
> >> +			/* strlen("pinctrl-") == 8 */
> >> +			if (strlen(prop->name) < 8) {
> >
> > Do we really need this extra checking?
> > It seems the prop->name is the "pinctrl-%d" you searched above, so the
> > strlen(prop->name) must not < 8, right?
> 
> Assuming of_find_property works correctly, I guess that's true. We can
> remove that if check.
> 
> >> +				dev_err(p->dev, "prop %s inconsistent length\n",
> >> +					prop->name);
> >> +				ret = -EINVAL;
> >> +				goto err;
> >> +			}
> >> +			statename = prop->name + 8;
> >
> > From this code, it seems actually we provide user the option by chance to define
> > state as pinctrl-syspend which is out of our binding doc.
> 
> The user can place a property with name "pinctrl-suspend" into the DT.
> However, since we only look for properties named pinctrl-%d, then the
> code will never read/use it, just like any other unexpected property.
> 
Yes, you're correct.
I misunderstood that.

> >> +		}
> >> +
> >> +		/* For every referenced pin configuration node in it */
> >> +		for (config = 0; config < size; config++) {
> >> +			phandle = be32_to_cpup(list++);
> >> +
> >> +			/* Look up the pin configuration node */
> >> +			np_config = of_find_node_by_phandle(phandle);
> >
> > One option is using of_parse_phandle, then we do not need calculate
> > the phandle offset by ourselves.
> > Like:
> > np_config = of_parse_phandle(propname , config);
> 
> Yes, that's a good idea. I'll try that.
> 
> >> +			if (!np_config) {
> >> +				dev_err(p->dev,
> >> +					"prop %s index %i invalid phandle\n",
> >> +					prop->name, config);
> >> +				ret = -EINVAL;
> >> +				goto err;
> >> +			}
> >> +
> >> +			/* Parse the node */
> >> +			ret = dt_to_map_one_config(p, statename, np_config);
> >> +			of_node_put(np_config);
> >> +			if (ret < 0)
> >> +				goto err;
> >> +		}
> >> +
> >> +		/* No entries in DT? Generate a dummy state table entry */
> >> +		if (!size) {
> >> +			ret = dt_remember_dummy_state(p, statename);
> >> +			if (ret < 0)
> >> +				goto err;
> >> +		}
> >> +	}
> >> +
> >> +	list_for_each_entry(dt_map, &p->dt_maps, node) {
> >> +		ret = pinctrl_register_map(dt_map->map, dt_map->num_maps,
> >> +					   false, true);
> >> +		if (ret < 0)
> >> +			goto err;
> >> +	}
> >
> > What's main purpose of differing the map registration and introduce a
> > intermediate pinctrl_dt_map(dt_remember_or_free_map)?
> > What about directly register maps once it's parsed?
> 
> s/differing/deferring/ I assume.
> 
> IIRC, this was mainly to simplify error handling; by deferring it, you
> don't have to unregister everything when undoing a failed parse.
> However, I guess that pinctrl_dt_free_maps() already cleans up
> everything anyway, so we couuld just register everything as soon as its
> parsed. I'll think a little more about this and switch to doing that if
> will work.
> 
Yes, since it introduces extra complexities which i'm not sure is worth
enough, we can double check it.

Regards
Dong Aisheng

Re: [PATCH V2 3/6] pinctrl: core device tree mapping table parsing support

[Dong Aisheng]

On Wed, Mar 21, 2012 at 11:48:20PM +0800, Stephen Warren wrote:
> On 03/21/2012 01:31 AM, Dong Aisheng wrote:
> > On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:
> >> During pinctrl_get(), if the client device has a device tree node, look
> >> for the common pinctrl properties there. If found, parse the referenced
> >> device tree nodes, with the help of the pinctrl drivers, and generate
> >> mapping table entries from them.
> >>
> >> During pinctrl_put(), free any results of device tree parsing.
> 
> >> +static void dt_free_map(struct pinctrl_dev *pctldev,
> >> +		     struct pinctrl_map *map, unsigned num_maps)
> >> +{
> >> +	if (pctldev) {
> >> +		struct pinctrl_ops *ops = pctldev->desc->pctlops;
> >> +		ops->dt_free_map(pctldev, map, num_maps);
> >> +	} else {
> >
> > I remember for hog on functions the pctldev becomes pinctrl devices itself,
> > so in which case pctldev can be NULL?
> 
> PIN_MAP_TYPE_DUMMY_STATE has no pctldev.
> 
Oh, get it now.
Maybe we could a line of comment in the generating dummy state code telling about
this.

> >> +static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
> >> +				struct device_node *np_config)
> >> +{
> >> +	struct device_node *np_pctldev;
> >> +	struct pinctrl_dev *pctldev;
> >> +	struct pinctrl_ops *ops;
> >> +	int ret;
> >> +	struct pinctrl_map *map;
> >> +	unsigned num_maps;
> >> +
> >> +	/* Find the pin controller containing np_config */
> >> +	np_pctldev = of_node_get(np_config);
> >
> > It seems the np_config node is already got when call of_find_node_by_phandle.
> > So do we still need this line?
> 
> Right below that code, we traverse up the tree using
> of_get_next_parent(). Internally, this calls of_node_put() on the node
> pointer that's passed in. Hence, we need an extra get() to match this.
> 
Yes, it's true.
So it's reasonable here.

> >> +int pinctrl_dt_to_map(struct pinctrl *p)
> >> +{
> >> +	struct device_node *np = p->dev->of_node;
> >> +	int state, ret;
> >> +	char *propname;
> >> +	struct property *prop;
> >> +	const char *statename;
> >> +	const __be32 *list;
> >> +	int size, config;
> >> +	phandle phandle;
> >> +	struct device_node *np_config;
> >> +	struct pinctrl_dt_map *dt_map;
> >
> > Add NULL np checking?
> 
> Oops yes. I though I had that somewhere, but evidently not...
> 
> >> +	/* We may store pointers to property names within the node */
> >> +	of_node_get(np);
> >> +
> >> +	/* For each defined state ID */
> >> +	for (state = 0; ; state++) {
> >> +		/* Retrieve the pinctrl-* property */
> >> +		propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
> >> +		prop = of_find_property(np, propname, &size);
> ...
> >> +			/* strlen("pinctrl-") == 8 */
> >> +			if (strlen(prop->name) < 8) {
> >
> > Do we really need this extra checking?
> > It seems the prop->name is the "pinctrl-%d" you searched above, so the
> > strlen(prop->name) must not < 8, right?
> 
> Assuming of_find_property works correctly, I guess that's true. We can
> remove that if check.
> 
> >> +				dev_err(p->dev, "prop %s inconsistent length\n",
> >> +					prop->name);
> >> +				ret = -EINVAL;
> >> +				goto err;
> >> +			}
> >> +			statename = prop->name + 8;
> >
> > From this code, it seems actually we provide user the option by chance to define
> > state as pinctrl-syspend which is out of our binding doc.
> 
> The user can place a property with name "pinctrl-suspend" into the DT.
> However, since we only look for properties named pinctrl-%d, then the
> code will never read/use it, just like any other unexpected property.
> 
Yes, you're correct.
I misunderstood that.

> >> +		}
> >> +
> >> +		/* For every referenced pin configuration node in it */
> >> +		for (config = 0; config < size; config++) {
> >> +			phandle = be32_to_cpup(list++);
> >> +
> >> +			/* Look up the pin configuration node */
> >> +			np_config = of_find_node_by_phandle(phandle);
> >
> > One option is using of_parse_phandle, then we do not need calculate
> > the phandle offset by ourselves.
> > Like:
> > np_config = of_parse_phandle(propname , config);
> 
> Yes, that's a good idea. I'll try that.
> 
> >> +			if (!np_config) {
> >> +				dev_err(p->dev,
> >> +					"prop %s index %i invalid phandle\n",
> >> +					prop->name, config);
> >> +				ret = -EINVAL;
> >> +				goto err;
> >> +			}
> >> +
> >> +			/* Parse the node */
> >> +			ret = dt_to_map_one_config(p, statename, np_config);
> >> +			of_node_put(np_config);
> >> +			if (ret < 0)
> >> +				goto err;
> >> +		}
> >> +
> >> +		/* No entries in DT? Generate a dummy state table entry */
> >> +		if (!size) {
> >> +			ret = dt_remember_dummy_state(p, statename);
> >> +			if (ret < 0)
> >> +				goto err;
> >> +		}
> >> +	}
> >> +
> >> +	list_for_each_entry(dt_map, &p->dt_maps, node) {
> >> +		ret = pinctrl_register_map(dt_map->map, dt_map->num_maps,
> >> +					   false, true);
> >> +		if (ret < 0)
> >> +			goto err;
> >> +	}
> >
> > What's main purpose of differing the map registration and introduce a
> > intermediate pinctrl_dt_map(dt_remember_or_free_map)?
> > What about directly register maps once it's parsed?
> 
> s/differing/deferring/ I assume.
> 
> IIRC, this was mainly to simplify error handling; by deferring it, you
> don't have to unregister everything when undoing a failed parse.
> However, I guess that pinctrl_dt_free_maps() already cleans up
> everything anyway, so we couuld just register everything as soon as its
> parsed. I'll think a little more about this and switch to doing that if
> will work.
> 
Yes, since it introduces extra complexities which i'm not sure is worth
enough, we can double check it.

Regards
Dong Aisheng

[PATCH V2 4/6] dt: Move Tegra20 pin mux binding into new pinctrl directory

[Stephen Warren]

This places the file in the new location for all pin controller bindings.

Also, rename the file using the full compatible value for easier
avoidance of conflicts between multiple bindings.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
 .../bindings/pinctrl/nvidia,tegra20-pinmux.txt     |    5 +++++
 .../devicetree/bindings/pinmux/pinmux_nvidia.txt   |    5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra20-pinmux.txt
 delete mode 100644 Documentation/devicetree/bindings/pinmux/pinmux_nvidia.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra20-pinmux.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra20-pinmux.txt
new file mode 100644
index 0000000..36f82db
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra20-pinmux.txt
@@ -0,0 +1,5 @@
+NVIDIA Tegra 2 pinmux controller
+
+Required properties:
+- compatible : "nvidia,tegra20-pinmux"
+
diff --git a/Documentation/devicetree/bindings/pinmux/pinmux_nvidia.txt b/Documentation/devicetree/bindings/pinmux/pinmux_nvidia.txt
deleted file mode 100644
index 36f82db..0000000
--- a/Documentation/devicetree/bindings/pinmux/pinmux_nvidia.txt
+++ /dev/null
@@ -1,5 +0,0 @@
-NVIDIA Tegra 2 pinmux controller
-
-Required properties:
-- compatible : "nvidia,tegra20-pinmux"
-
-- 
1.7.0.4

[PATCH V2 5/6] dt: Document Tegra20/30 pinctrl binding

[Stephen Warren]

Define a new binding for the Tegra pin controller, which is capable of
defining all aspects of desired pin multiplexing and pin configuration.
This is all based on the new common pinctrl bindings.

Add Tegra30 binding based on Tegra20 binding.

Add some basic stuff that was missing before:
* How many and what reg property entries must be provided.
* An example.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
 .../bindings/pinctrl/nvidia,tegra20-pinmux.txt     |  131 +++++++++++++++++++-
 .../bindings/pinctrl/nvidia,tegra30-pinmux.txt     |  132 ++++++++++++++++++++
 2 files changed, 261 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra30-pinmux.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra20-pinmux.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra20-pinmux.txt
index 36f82db..7d6602b 100644
--- a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra20-pinmux.txt
+++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra20-pinmux.txt
@@ -1,5 +1,132 @@
-NVIDIA Tegra 2 pinmux controller
+NVIDIA Tegra20 pinmux controller
 
 Required properties:
-- compatible : "nvidia,tegra20-pinmux"
+- compatible: "nvidia,tegra20-pinmux"
+- reg: Should contain the register physical address and length for each of
+  the tri-state, mux, pull-up/down, and pad control register sets.
 
+Please refer to pinctrl-bindings.txt in this directory for details of the
+common pinctrl bindings used by client devices, including the meaning of the
+phrase "pin configuration node".
+
+Tegra's pin configuration nodes act as a container for an abitrary number of
+subnodes. Each of these subnodes represents some desired configuration for a
+pin, a group, or a list of pins or groups. This configuration can include the
+mux function to select on those pin(s)/group(s), and various pin configuration
+parameters, such as pull-up, tristate, drive strength, etc.
+
+The name of each subnode is not important; all subnodes should be enumerated
+and processed purely based on their content.
+
+Each subnode only affects those parameters that are explicitly listed. In
+other words, a subnode that lists a mux function but no pin configuration
+parameters implies no information about any pin configuration parameters.
+Similarly, a pin subnode that describes a pullup parameter implies no
+information about e.g. the mux function or tristate parameter. For this
+reason, even seemingly boolean values are actually tristates in this binding:
+unspecified, off, or on. Unspecified is represented as an absent property,
+and off/on are represented as integer values 0 and 1.
+
+Required subnode-properties:
+- nvidia,pins : An array of strings. Each string contains the name of a pin or
+    group. Valid values for these names are listed below.
+
+Optional subnode-properties:
+- nvidia,function: A string containing the name of the function to mux to the
+  pin or group. Valid values for function names are listed below. See the Tegra
+  TRM to determine which are valid for each pin or group.
+- nvidia,pull: Integer, representing the pull-down/up to apply to the pin.
+    0: none, 1: down, 2: up.
+- nvidia,tristate: Integer.
+    0: drive, 1: tristate.
+- nvidia,high-speed-mode: Integer. Enable high speed mode the pins.
+    0: no, 1: yes.
+- nvidia,schmitt: Integer. Enables Schmitt Trigger on the input.
+    0: no, 1: yes.
+- nvidia,low-power-mode: Integer. Valid values 0-3. 0 is least power, 3 is
+    most power. Controls the drive power or current. See "Low Power Mode"
+    or "LPMD1" and "LPMD0" in the Tegra TRM.
+- nvidia,pull-down-strength: Integer. Controls drive strength. 0 is weakest.
+    The range of valid values depends on the pingroup. See "CAL_DRVDN" in the
+    Tegra TRM.
+- nvidia,pull-up-strength: Integer. Controls drive strength. 0 is weakest.
+    The range of valid values depends on the pingroup. See "CAL_DRVUP" in the
+    Tegra TRM.
+- nvidia,slew-rate-rising: Integer. Controls rising signal slew rate. 0 is
+    fastest. The range of valid values depends on the pingroup. See
+    "DRVDN_SLWR" in the Tegra TRM.
+- nvidia,slew-rate-falling: Integer. Controls falling signal slew rate. 0 is
+    fastest. The range of valid values depends on the pingroup. See
+    "DRVUP_SLWF" in the Tegra TRM.
+
+Note that many of these properties are only valid for certain specific pins
+or groups. See the Tegra TRM and various pinmux spreadsheets for complete
+details regarding which groups support which functionality. The Linux pinctrl
+driver may also be a useful reference, since it consolidates, disambiguates,
+and corrects data from all those sources.
+
+Valid values for pin and group names are:
+
+  mux groups:
+
+    These all support nvidia,function, nvidia,tristate, and many support
+    nvidia,pull.
+
+    ata, atb, atc, atd, ate, cdev1, cdev2, crtp, csus, dap1, dap2, dap3, dap4,
+    ddc, dta, dtb, dtc, dtd, dte, dtf, gma, gmb, gmc, gmd, gme, gpu, gpu7,
+    gpv, hdint, i2cp, irrx, irtx, kbca, kbcb, kbcc, kbcd, kbce, kbcf, lcsn,
+    ld0, ld1, ld2, ld3, ld4, ld5, ld6, ld7, ld8, ld9, ld10, ld11, ld12, ld13,
+    ld14, ld15, ld16, ld17, ldc, ldi, lhp0, lhp1, lhp2, lhs, lm0, lm1, lpp,
+    lpw0, lpw1, lpw2, lsc0, lsc1, lsck, lsda, lsdi, lspi, lvp0, lvp1, lvs,
+    owc, pmc, pta, rm, sdb, sdc, sdd, sdio1, slxa, slxc, slxd, slxk, spdi,
+    spdo, spia, spib, spic, spid, spie, spif, spig, spih, uaa, uab, uac, uad,
+    uca, ucb, uda.
+
+  tristate groups:
+
+    These only support nvidia,pull.
+
+    ck32, ddrc, pmca, pmcb, pmcc, pmcd, pmce, xm2c, xm2d, ls, lc, ld17_0,
+    ld19_18, ld21_20, ld23_22.
+
+  drive groups:
+
+    With some exceptions, these support nvidia,high-speed-mode,
+    nvidia,schmitt, nvidia,low-power-mode, nvidia,pull-down-strength,
+    nvidia,pull-up-strength, nvidia,slew_rate-rising, nvidia,slew_rate-falling.
+
+    drive_ao1, drive_ao2, drive_at1, drive_at2, drive_cdev1, drive_cdev2,
+    drive_csus, drive_dap1, drive_dap2, drive_dap3, drive_dap4, drive_dbg,
+    drive_lcd1, drive_lcd2, drive_sdmmc2, drive_sdmmc3, drive_spi, drive_uaa,
+    drive_uab, drive_uart2, drive_uart3, drive_vi1, drive_vi2, drive_xm2a,
+    drive_xm2c, drive_xm2d, drive_xm2clk, drive_sdio1, drive_crt, drive_ddc,
+    drive_gma, drive_gmb, drive_gmc, drive_gmd, drive_gme, drive_owr,
+    drive_uda.
+
+Example:
+
+	pinctrl@70000000 {
+		compatible = "nvidia,tegra20-pinmux";
+		reg = < 0x70000014 0x10    /* Tri-state registers */
+			0x70000080 0x20    /* Mux registers */
+			0x700000a0 0x14    /* Pull-up/down registers */
+			0x70000868 0xa8 >; /* Pad control registers */
+	};
+
+Example board file extract:
+
+	pinctrl@70000000 {
+		sdio4_default {
+			atb {
+				nvidia,pins = "atb", "gma", "gme";
+				nvidia,function = "sdio4";
+				nvidia,pull = <0>;
+				nvidia,tristate = <0>;
+			};
+		};
+	};
+
+	sdhci@c8000600 {
+		pinctrl-names = "default";
+		pinctrl-0 = <&sdio4_default>;
+	};
diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra30-pinmux.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra30-pinmux.txt
new file mode 100644
index 0000000..c275b70
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra30-pinmux.txt
@@ -0,0 +1,132 @@
+NVIDIA Tegra30 pinmux controller
+
+The Tegra30 pinctrl binding is very similar to the Tegra20 pinctrl binding,
+as described in nvidia,tegra20-pinmux.txt. In fact, this document assumes
+that binding as a baseline, and only documents the differences between the
+two bindings.
+
+Required properties:
+- compatible: "nvidia,tegra30-pinmux"
+- reg: Should contain the register physical address and length for each of
+  the pad control and mux registers.
+
+Tegra30 adds the following optional properties for pin configuration subnodes:
+- nvidia,enable-input: Integer. Enable the pin's input path. 0: no, 1: yes.
+- nvidia,open-drain: Integer. Enable open drain mode. 0: no, 1: yes.
+- nvidia,lock: Integer. Lock the pin configuration against further changes
+    until reset. 0: no, 1: yes.
+- nvidia,io-reset: Integer. Reset the IO path. 0: no, 1: yes.
+
+As with Tegra20, see the Tegra TRM for complete details regarding which groups
+support which functionality.
+
+Valid values for pin and group names are:
+
+  per-pin mux groups:
+
+    These all support nvidia,function, nvidia,tristate, nvidia,pull,
+    nvidia,enable-input, nvidia,lock. Some support nvidia,open-drain,
+    nvidia,io-reset.
+
+    clk_32k_out_pa0, uart3_cts_n_pa1, dap2_fs_pa2, dap2_sclk_pa3,
+    dap2_din_pa4, dap2_dout_pa5, sdmmc3_clk_pa6, sdmmc3_cmd_pa7, gmi_a17_pb0,
+    gmi_a18_pb1, lcd_pwr0_pb2, lcd_pclk_pb3, sdmmc3_dat3_pb4, sdmmc3_dat2_pb5,
+    sdmmc3_dat1_pb6, sdmmc3_dat0_pb7, uart3_rts_n_pc0, lcd_pwr1_pc1,
+    uart2_txd_pc2, uart2_rxd_pc3, gen1_i2c_scl_pc4, gen1_i2c_sda_pc5,
+    lcd_pwr2_pc6, gmi_wp_n_pc7, sdmmc3_dat5_pd0, sdmmc3_dat4_pd1, lcd_dc1_pd2,
+    sdmmc3_dat6_pd3, sdmmc3_dat7_pd4, vi_d1_pd5, vi_vsync_pd6, vi_hsync_pd7,
+    lcd_d0_pe0, lcd_d1_pe1, lcd_d2_pe2, lcd_d3_pe3, lcd_d4_pe4, lcd_d5_pe5,
+    lcd_d6_pe6, lcd_d7_pe7, lcd_d8_pf0, lcd_d9_pf1, lcd_d10_pf2, lcd_d11_pf3,
+    lcd_d12_pf4, lcd_d13_pf5, lcd_d14_pf6, lcd_d15_pf7, gmi_ad0_pg0,
+    gmi_ad1_pg1, gmi_ad2_pg2, gmi_ad3_pg3, gmi_ad4_pg4, gmi_ad5_pg5,
+    gmi_ad6_pg6, gmi_ad7_pg7, gmi_ad8_ph0, gmi_ad9_ph1, gmi_ad10_ph2,
+    gmi_ad11_ph3, gmi_ad12_ph4, gmi_ad13_ph5, gmi_ad14_ph6, gmi_ad15_ph7,
+    gmi_wr_n_pi0, gmi_oe_n_pi1, gmi_dqs_pi2, gmi_cs6_n_pi3, gmi_rst_n_pi4,
+    gmi_iordy_pi5, gmi_cs7_n_pi6, gmi_wait_pi7, gmi_cs0_n_pj0, lcd_de_pj1,
+    gmi_cs1_n_pj2, lcd_hsync_pj3, lcd_vsync_pj4, uart2_cts_n_pj5,
+    uart2_rts_n_pj6, gmi_a16_pj7, gmi_adv_n_pk0, gmi_clk_pk1, gmi_cs4_n_pk2,
+    gmi_cs2_n_pk3, gmi_cs3_n_pk4, spdif_out_pk5, spdif_in_pk6, gmi_a19_pk7,
+    vi_d2_pl0, vi_d3_pl1, vi_d4_pl2, vi_d5_pl3, vi_d6_pl4, vi_d7_pl5,
+    vi_d8_pl6, vi_d9_pl7, lcd_d16_pm0, lcd_d17_pm1, lcd_d18_pm2, lcd_d19_pm3,
+    lcd_d20_pm4, lcd_d21_pm5, lcd_d22_pm6, lcd_d23_pm7, dap1_fs_pn0,
+    dap1_din_pn1, dap1_dout_pn2, dap1_sclk_pn3, lcd_cs0_n_pn4, lcd_sdout_pn5,
+    lcd_dc0_pn6, hdmi_int_pn7, ulpi_data7_po0, ulpi_data0_po1, ulpi_data1_po2,
+    ulpi_data2_po3, ulpi_data3_po4, ulpi_data4_po5, ulpi_data5_po6,
+    ulpi_data6_po7, dap3_fs_pp0, dap3_din_pp1, dap3_dout_pp2, dap3_sclk_pp3,
+    dap4_fs_pp4, dap4_din_pp5, dap4_dout_pp6, dap4_sclk_pp7, kb_col0_pq0,
+    kb_col1_pq1, kb_col2_pq2, kb_col3_pq3, kb_col4_pq4, kb_col5_pq5,
+    kb_col6_pq6, kb_col7_pq7, kb_row0_pr0, kb_row1_pr1, kb_row2_pr2,
+    kb_row3_pr3, kb_row4_pr4, kb_row5_pr5, kb_row6_pr6, kb_row7_pr7,
+    kb_row8_ps0, kb_row9_ps1, kb_row10_ps2, kb_row11_ps3, kb_row12_ps4,
+    kb_row13_ps5, kb_row14_ps6, kb_row15_ps7, vi_pclk_pt0, vi_mclk_pt1,
+    vi_d10_pt2, vi_d11_pt3, vi_d0_pt4, gen2_i2c_scl_pt5, gen2_i2c_sda_pt6,
+    sdmmc4_cmd_pt7, pu0, pu1, pu2, pu3, pu4, pu5, pu6, jtag_rtck_pu7, pv0,
+    pv1, pv2, pv3, ddc_scl_pv4, ddc_sda_pv5, crt_hsync_pv6, crt_vsync_pv7,
+    lcd_cs1_n_pw0, lcd_m1_pw1, spi2_cs1_n_pw2, spi2_cs2_n_pw3, clk1_out_pw4,
+    clk2_out_pw5, uart3_txd_pw6, uart3_rxd_pw7, spi2_mosi_px0, spi2_miso_px1,
+    spi2_sck_px2, spi2_cs0_n_px3, spi1_mosi_px4, spi1_sck_px5, spi1_cs0_n_px6,
+    spi1_miso_px7, ulpi_clk_py0, ulpi_dir_py1, ulpi_nxt_py2, ulpi_stp_py3,
+    sdmmc1_dat3_py4, sdmmc1_dat2_py5, sdmmc1_dat1_py6, sdmmc1_dat0_py7,
+    sdmmc1_clk_pz0, sdmmc1_cmd_pz1, lcd_sdin_pz2, lcd_wr_n_pz3, lcd_sck_pz4,
+    sys_clk_req_pz5, pwr_i2c_scl_pz6, pwr_i2c_sda_pz7, sdmmc4_dat0_paa0,
+    sdmmc4_dat1_paa1, sdmmc4_dat2_paa2, sdmmc4_dat3_paa3, sdmmc4_dat4_paa4,
+    sdmmc4_dat5_paa5, sdmmc4_dat6_paa6, sdmmc4_dat7_paa7, pbb0,
+    cam_i2c_scl_pbb1, cam_i2c_sda_pbb2, pbb3, pbb4, pbb5, pbb6, pbb7,
+    cam_mclk_pcc0, pcc1, pcc2, sdmmc4_rst_n_pcc3, sdmmc4_clk_pcc4,
+    clk2_req_pcc5, pex_l2_rst_n_pcc6, pex_l2_clkreq_n_pcc7,
+    pex_l0_prsnt_n_pdd0, pex_l0_rst_n_pdd1, pex_l0_clkreq_n_pdd2,
+    pex_wake_n_pdd3, pex_l1_prsnt_n_pdd4, pex_l1_rst_n_pdd5,
+    pex_l1_clkreq_n_pdd6, pex_l2_prsnt_n_pdd7, clk3_out_pee0, clk3_req_pee1,
+    clk1_req_pee2, hdmi_cec_pee3, clk_32k_in, core_pwr_req, cpu_pwr_req, owr,
+    pwr_int_n.
+
+  drive groups:
+
+    These all support nvidia,pull-down-strength, nvidia,pull-up-strength,
+    nvidia,slew_rate-rising, nvidia,slew_rate-falling. Most but not all
+    support nvidia,high-speed-mode, nvidia,schmitt, nvidia,low-power-mode.
+
+    ao1, ao2, at1, at2, at3, at4, at5, cdev1, cdev2, cec, crt, csus, dap1,
+    dap2, dap3, dap4, dbg, ddc, dev3, gma, gmb, gmc, gmd, gme, gmf, gmg,
+    gmh, gpv, lcd1, lcd2, owr, sdio1, sdio2, sdio3, spi, uaa, uab, uart2,
+    uart3, uda, vi1.
+
+Example:
+
+	pinctrl@70000000 {
+		compatible = "nvidia,tegra30-pinmux";
+		reg = < 0x70000868 0xd0     /* Pad control registers */
+			0x70003000 0x3e0 >; /* Mux registers */
+	};
+
+Example board file extract:
+
+	pinctrl@70000000 {
+		sdmmc4_default: pinmux {
+			sdmmc4_clk_pcc4 {
+				nvidia,pins =	"sdmmc4_clk_pcc4",
+						"sdmmc4_rst_n_pcc3";
+				nvidia,function = "sdmmc4";
+				nvidia,pull = <0>;
+				nvidia,tristate = <0>;
+			};
+			sdmmc4_dat0_paa0 {
+				nvidia,pins =	"sdmmc4_dat0_paa0",
+						"sdmmc4_dat1_paa1",
+						"sdmmc4_dat2_paa2",
+						"sdmmc4_dat3_paa3",
+						"sdmmc4_dat4_paa4",
+						"sdmmc4_dat5_paa5",
+						"sdmmc4_dat6_paa6",
+						"sdmmc4_dat7_paa7";
+				nvidia,function = "sdmmc4";
+				nvidia,pull = <2>;
+				nvidia,tristate = <0>;
+			};
+		};
+	};
+
+	sdhci@78000400 {
+		pinctrl-names = "default";
+		pinctrl-0 = <&sdmmc4_default>;
+	};
-- 
1.7.0.4

Re: [PATCH V2 5/6] dt: Document Tegra20/30 pinctrl binding

[Dong Aisheng]

On Wed, Mar 21, 2012 at 01:44:38AM +0800, Stephen Warren wrote:
> Define a new binding for the Tegra pin controller, which is capable of
> defining all aspects of desired pin multiplexing and pin configuration.
> This is all based on the new common pinctrl bindings.
> 
> Add Tegra30 binding based on Tegra20 binding.
> 
> Add some basic stuff that was missing before:
> * How many and what reg property entries must be provided.
> * An example.
> 
> Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> ---
........
> +Example board file extract:
> +
> +	pinctrl@70000000 {
> +		sdio4_default {
> +			atb {
> +				nvidia,pins = "atb", "gma", "gme";
> +				nvidia,function = "sdio4";
> +				nvidia,pull = <0>;
> +				nvidia,tristate = <0>;
> +			};
> +		};
> +	};
> +
> +	sdhci@c8000600 {
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&sdio4_default>;
A typo error? sdio4_default is not a phandle.

> +Example board file extract:
> +
> +	pinctrl@70000000 {
> +		sdmmc4_default: pinmux {
> +			sdmmc4_clk_pcc4 {
> +				nvidia,pins =	"sdmmc4_clk_pcc4",
> +						"sdmmc4_rst_n_pcc3";
> +				nvidia,function = "sdmmc4";
> +				nvidia,pull = <0>;
> +				nvidia,tristate = <0>;
> +			};
> +			sdmmc4_dat0_paa0 {
> +				nvidia,pins =	"sdmmc4_dat0_paa0",
> +						"sdmmc4_dat1_paa1",
> +						"sdmmc4_dat2_paa2",
> +						"sdmmc4_dat3_paa3",
> +						"sdmmc4_dat4_paa4",
> +						"sdmmc4_dat5_paa5",
> +						"sdmmc4_dat6_paa6",
> +						"sdmmc4_dat7_paa7";
> +				nvidia,function = "sdmmc4";
> +				nvidia,pull = <2>;
> +				nvidia,tristate = <0>;
It seems it does not support per pin config for tegra30 and we have to
separate them in different nodes with same group config value, right?

Regards
Dong Aisheng

Re: [PATCH V2 5/6] dt: Document Tegra20/30 pinctrl binding

[Dong Aisheng]

On Wed, Mar 21, 2012 at 01:44:38AM +0800, Stephen Warren wrote:
> Define a new binding for the Tegra pin controller, which is capable of
> defining all aspects of desired pin multiplexing and pin configuration.
> This is all based on the new common pinctrl bindings.
> 
> Add Tegra30 binding based on Tegra20 binding.
> 
> Add some basic stuff that was missing before:
> * How many and what reg property entries must be provided.
> * An example.
> 
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
........
> +Example board file extract:
> +
> +	pinctrl@70000000 {
> +		sdio4_default {
> +			atb {
> +				nvidia,pins = "atb", "gma", "gme";
> +				nvidia,function = "sdio4";
> +				nvidia,pull = <0>;
> +				nvidia,tristate = <0>;
> +			};
> +		};
> +	};
> +
> +	sdhci@c8000600 {
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&sdio4_default>;
A typo error? sdio4_default is not a phandle.

> +Example board file extract:
> +
> +	pinctrl@70000000 {
> +		sdmmc4_default: pinmux {
> +			sdmmc4_clk_pcc4 {
> +				nvidia,pins =	"sdmmc4_clk_pcc4",
> +						"sdmmc4_rst_n_pcc3";
> +				nvidia,function = "sdmmc4";
> +				nvidia,pull = <0>;
> +				nvidia,tristate = <0>;
> +			};
> +			sdmmc4_dat0_paa0 {
> +				nvidia,pins =	"sdmmc4_dat0_paa0",
> +						"sdmmc4_dat1_paa1",
> +						"sdmmc4_dat2_paa2",
> +						"sdmmc4_dat3_paa3",
> +						"sdmmc4_dat4_paa4",
> +						"sdmmc4_dat5_paa5",
> +						"sdmmc4_dat6_paa6",
> +						"sdmmc4_dat7_paa7";
> +				nvidia,function = "sdmmc4";
> +				nvidia,pull = <2>;
> +				nvidia,tristate = <0>;
It seems it does not support per pin config for tegra30 and we have to
separate them in different nodes with same group config value, right?

Regards
Dong Aisheng

Re: [PATCH V2 5/6] dt: Document Tegra20/30 pinctrl binding

[Stephen Warren]

On 03/21/2012 03:19 AM, Dong Aisheng wrote:
> On Wed, Mar 21, 2012 at 01:44:38AM +0800, Stephen Warren wrote:
>> Define a new binding for the Tegra pin controller, which is capable of
>> defining all aspects of desired pin multiplexing and pin configuration.
>> This is all based on the new common pinctrl bindings.
>>
>> Add Tegra30 binding based on Tegra20 binding.
>>
>> Add some basic stuff that was missing before:
>> * How many and what reg property entries must be provided.
>> * An example.
>>
>> Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
>> ---
> ........
>> +Example board file extract:
>> +
>> +	pinctrl@70000000 {
>> +		sdio4_default {
>> +			atb {
>> +				nvidia,pins = "atb", "gma", "gme";
>> +				nvidia,function = "sdio4";
>> +				nvidia,pull = <0>;
>> +				nvidia,tristate = <0>;
>> +			};
>> +		};
>> +	};
>> +
>> +	sdhci@c8000600 {
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&sdio4_default>;
>
> A typo error? sdio4_default is not a phandle.

Yes.

>> +Example board file extract:
>> +
>> +	pinctrl@70000000 {
>> +		sdmmc4_default: pinmux {
>> +			sdmmc4_clk_pcc4 {
>> +				nvidia,pins =	"sdmmc4_clk_pcc4",
>> +						"sdmmc4_rst_n_pcc3";
>> +				nvidia,function = "sdmmc4";
>> +				nvidia,pull = <0>;
>> +				nvidia,tristate = <0>;
>> +			};
>> +			sdmmc4_dat0_paa0 {
>> +				nvidia,pins =	"sdmmc4_dat0_paa0",
>> +						"sdmmc4_dat1_paa1",
>> +						"sdmmc4_dat2_paa2",
>> +						"sdmmc4_dat3_paa3",
>> +						"sdmmc4_dat4_paa4",
>> +						"sdmmc4_dat5_paa5",
>> +						"sdmmc4_dat6_paa6",
>> +						"sdmmc4_dat7_paa7";
>> +				nvidia,function = "sdmmc4";
>> +				nvidia,pull = <2>;
>> +				nvidia,tristate = <0>;
>
> It seems it does not support per pin config for tegra30 and we have to
> separate them in different nodes with same group config value, right?

Sorry, I don't understand the question.

On Tegra30, pin mux selection and some pin configuration parameters are
per-pin. Other pin configuration parameters are per-group. The pinctrl
driver defines groups for:

* Each pin (for per-pin properties)
* Each group that has non-per-pin configuration parameters.

You can use either/both sets of groups in the .dts file (i.e. in the
nvidia,pins property).

You'd never end up with a single node that mixes the "per-pin" group
names and "non-per-pin" group names, simply because there are no
properties that can be applied to groups of both types in hardware.

Re: [PATCH V2 5/6] dt: Document Tegra20/30 pinctrl binding

[Stephen Warren]

On 03/21/2012 03:19 AM, Dong Aisheng wrote:
> On Wed, Mar 21, 2012 at 01:44:38AM +0800, Stephen Warren wrote:
>> Define a new binding for the Tegra pin controller, which is capable of
>> defining all aspects of desired pin multiplexing and pin configuration.
>> This is all based on the new common pinctrl bindings.
>>
>> Add Tegra30 binding based on Tegra20 binding.
>>
>> Add some basic stuff that was missing before:
>> * How many and what reg property entries must be provided.
>> * An example.
>>
>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>> ---
> ........
>> +Example board file extract:
>> +
>> +	pinctrl@70000000 {
>> +		sdio4_default {
>> +			atb {
>> +				nvidia,pins = "atb", "gma", "gme";
>> +				nvidia,function = "sdio4";
>> +				nvidia,pull = <0>;
>> +				nvidia,tristate = <0>;
>> +			};
>> +		};
>> +	};
>> +
>> +	sdhci@c8000600 {
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&sdio4_default>;
>
> A typo error? sdio4_default is not a phandle.

Yes.

>> +Example board file extract:
>> +
>> +	pinctrl@70000000 {
>> +		sdmmc4_default: pinmux {
>> +			sdmmc4_clk_pcc4 {
>> +				nvidia,pins =	"sdmmc4_clk_pcc4",
>> +						"sdmmc4_rst_n_pcc3";
>> +				nvidia,function = "sdmmc4";
>> +				nvidia,pull = <0>;
>> +				nvidia,tristate = <0>;
>> +			};
>> +			sdmmc4_dat0_paa0 {
>> +				nvidia,pins =	"sdmmc4_dat0_paa0",
>> +						"sdmmc4_dat1_paa1",
>> +						"sdmmc4_dat2_paa2",
>> +						"sdmmc4_dat3_paa3",
>> +						"sdmmc4_dat4_paa4",
>> +						"sdmmc4_dat5_paa5",
>> +						"sdmmc4_dat6_paa6",
>> +						"sdmmc4_dat7_paa7";
>> +				nvidia,function = "sdmmc4";
>> +				nvidia,pull = <2>;
>> +				nvidia,tristate = <0>;
>
> It seems it does not support per pin config for tegra30 and we have to
> separate them in different nodes with same group config value, right?

Sorry, I don't understand the question.

On Tegra30, pin mux selection and some pin configuration parameters are
per-pin. Other pin configuration parameters are per-group. The pinctrl
driver defines groups for:

* Each pin (for per-pin properties)
* Each group that has non-per-pin configuration parameters.

You can use either/both sets of groups in the .dts file (i.e. in the
nvidia,pins property).

You'd never end up with a single node that mixes the "per-pin" group
names and "non-per-pin" group names, simply because there are no
properties that can be applied to groups of both types in hardware.

Re: [PATCH V2 5/6] dt: Document Tegra20/30 pinctrl binding

[Dong Aisheng]

On Wed, Mar 21, 2012 at 11:57:14PM +0800, Stephen Warren wrote:
> On 03/21/2012 03:19 AM, Dong Aisheng wrote:
> > On Wed, Mar 21, 2012 at 01:44:38AM +0800, Stephen Warren wrote:
> >> Define a new binding for the Tegra pin controller, which is capable of
> >> defining all aspects of desired pin multiplexing and pin configuration.
> >> This is all based on the new common pinctrl bindings.
> >>
> >> Add Tegra30 binding based on Tegra20 binding.
> >>
> >> Add some basic stuff that was missing before:
> >> * How many and what reg property entries must be provided.
> >> * An example.
> >>
> >> Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> >> ---
> > ........
> >> +Example board file extract:
> >> +
> >> +	pinctrl@70000000 {
> >> +		sdio4_default {
> >> +			atb {
> >> +				nvidia,pins = "atb", "gma", "gme";
> >> +				nvidia,function = "sdio4";
> >> +				nvidia,pull = <0>;
> >> +				nvidia,tristate = <0>;
> >> +			};
> >> +		};
> >> +	};
> >> +
> >> +	sdhci@c8000600 {
> >> +		pinctrl-names = "default";
> >> +		pinctrl-0 = <&sdio4_default>;
> >
> > A typo error? sdio4_default is not a phandle.
> 
> Yes.
> 
> >> +Example board file extract:
> >> +
> >> +	pinctrl@70000000 {
> >> +		sdmmc4_default: pinmux {
> >> +			sdmmc4_clk_pcc4 {
> >> +				nvidia,pins =	"sdmmc4_clk_pcc4",
> >> +						"sdmmc4_rst_n_pcc3";
> >> +				nvidia,function = "sdmmc4";
> >> +				nvidia,pull = <0>;
> >> +				nvidia,tristate = <0>;
> >> +			};
> >> +			sdmmc4_dat0_paa0 {
> >> +				nvidia,pins =	"sdmmc4_dat0_paa0",
> >> +						"sdmmc4_dat1_paa1",
> >> +						"sdmmc4_dat2_paa2",
> >> +						"sdmmc4_dat3_paa3",
> >> +						"sdmmc4_dat4_paa4",
> >> +						"sdmmc4_dat5_paa5",
> >> +						"sdmmc4_dat6_paa6",
> >> +						"sdmmc4_dat7_paa7";
> >> +				nvidia,function = "sdmmc4";
> >> +				nvidia,pull = <2>;
> >> +				nvidia,tristate = <0>;
> >
> > It seems it does not support per pin config for tegra30 and we have to
> > separate them in different nodes with same group config value, right?
> 
> Sorry, I don't understand the question.
> 
I meant for tegra, the config(not mux) in one pinctrl node functions
on all entities in nvidia,pins, IOW, all pin or group must have the
same config.
So we can not set them differently in one node.

For example, considering if  sdmmc4_dat0_paa0 is nvidia,pull <0>
while sdmmc4_dat1_paa1 is nvidia,pull <1>.
Then we need to separate them in different pinctrl nodes, right?

I was thinking about whether we can use one pinctrl node to cover
all different group settings, however that may involve more complexity.
Anyway, it's driver specific part, i'm ok with what Tegra are doing.
It looks good to me right now.

> On Tegra30, pin mux selection and some pin configuration parameters are
> per-pin. Other pin configuration parameters are per-group. The pinctrl
> driver defines groups for:
> 
> * Each pin (for per-pin properties)
> * Each group that has non-per-pin configuration parameters.
> 
> You can use either/both sets of groups in the .dts file (i.e. in the
> nvidia,pins property).
> 
> You'd never end up with a single node that mixes the "per-pin" group
> names and "non-per-pin" group names, simply because there are no
> properties that can be applied to groups of both types in hardware.
> 

Regards
Dong Aisheng

Re: [PATCH V2 5/6] dt: Document Tegra20/30 pinctrl binding

[Dong Aisheng]

On Wed, Mar 21, 2012 at 11:57:14PM +0800, Stephen Warren wrote:
> On 03/21/2012 03:19 AM, Dong Aisheng wrote:
> > On Wed, Mar 21, 2012 at 01:44:38AM +0800, Stephen Warren wrote:
> >> Define a new binding for the Tegra pin controller, which is capable of
> >> defining all aspects of desired pin multiplexing and pin configuration.
> >> This is all based on the new common pinctrl bindings.
> >>
> >> Add Tegra30 binding based on Tegra20 binding.
> >>
> >> Add some basic stuff that was missing before:
> >> * How many and what reg property entries must be provided.
> >> * An example.
> >>
> >> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> >> ---
> > ........
> >> +Example board file extract:
> >> +
> >> +	pinctrl@70000000 {
> >> +		sdio4_default {
> >> +			atb {
> >> +				nvidia,pins = "atb", "gma", "gme";
> >> +				nvidia,function = "sdio4";
> >> +				nvidia,pull = <0>;
> >> +				nvidia,tristate = <0>;
> >> +			};
> >> +		};
> >> +	};
> >> +
> >> +	sdhci@c8000600 {
> >> +		pinctrl-names = "default";
> >> +		pinctrl-0 = <&sdio4_default>;
> >
> > A typo error? sdio4_default is not a phandle.
> 
> Yes.
> 
> >> +Example board file extract:
> >> +
> >> +	pinctrl@70000000 {
> >> +		sdmmc4_default: pinmux {
> >> +			sdmmc4_clk_pcc4 {
> >> +				nvidia,pins =	"sdmmc4_clk_pcc4",
> >> +						"sdmmc4_rst_n_pcc3";
> >> +				nvidia,function = "sdmmc4";
> >> +				nvidia,pull = <0>;
> >> +				nvidia,tristate = <0>;
> >> +			};
> >> +			sdmmc4_dat0_paa0 {
> >> +				nvidia,pins =	"sdmmc4_dat0_paa0",
> >> +						"sdmmc4_dat1_paa1",
> >> +						"sdmmc4_dat2_paa2",
> >> +						"sdmmc4_dat3_paa3",
> >> +						"sdmmc4_dat4_paa4",
> >> +						"sdmmc4_dat5_paa5",
> >> +						"sdmmc4_dat6_paa6",
> >> +						"sdmmc4_dat7_paa7";
> >> +				nvidia,function = "sdmmc4";
> >> +				nvidia,pull = <2>;
> >> +				nvidia,tristate = <0>;
> >
> > It seems it does not support per pin config for tegra30 and we have to
> > separate them in different nodes with same group config value, right?
> 
> Sorry, I don't understand the question.
> 
I meant for tegra, the config(not mux) in one pinctrl node functions
on all entities in nvidia,pins, IOW, all pin or group must have the
same config.
So we can not set them differently in one node.

For example, considering if  sdmmc4_dat0_paa0 is nvidia,pull <0>
while sdmmc4_dat1_paa1 is nvidia,pull <1>.
Then we need to separate them in different pinctrl nodes, right?

I was thinking about whether we can use one pinctrl node to cover
all different group settings, however that may involve more complexity.
Anyway, it's driver specific part, i'm ok with what Tegra are doing.
It looks good to me right now.

> On Tegra30, pin mux selection and some pin configuration parameters are
> per-pin. Other pin configuration parameters are per-group. The pinctrl
> driver defines groups for:
> 
> * Each pin (for per-pin properties)
> * Each group that has non-per-pin configuration parameters.
> 
> You can use either/both sets of groups in the .dts file (i.e. in the
> nvidia,pins property).
> 
> You'd never end up with a single node that mixes the "per-pin" group
> names and "non-per-pin" group names, simply because there are no
> properties that can be applied to groups of both types in hardware.
> 

Regards
Dong Aisheng

Re: [PATCH V2 5/6] dt: Document Tegra20/30 pinctrl binding

[Stephen Warren]

On 03/21/2012 10:00 PM, Dong Aisheng wrote:
> On Wed, Mar 21, 2012 at 11:57:14PM +0800, Stephen Warren wrote:
>> On 03/21/2012 03:19 AM, Dong Aisheng wrote:
>>> On Wed, Mar 21, 2012 at 01:44:38AM +0800, Stephen Warren wrote:
>>>> Define a new binding for the Tegra pin controller, which is capable of
>>>> defining all aspects of desired pin multiplexing and pin configuration.
>>>> This is all based on the new common pinctrl bindings.
>>>>
>>>> Add Tegra30 binding based on Tegra20 binding.
...
>>>> +Example board file extract:
>>>> +
>>>> +	pinctrl@70000000 {
>>>> +		sdmmc4_default: pinmux {
>>>> +			sdmmc4_clk_pcc4 {
>>>> +				nvidia,pins =	"sdmmc4_clk_pcc4",
>>>> +						"sdmmc4_rst_n_pcc3";
>>>> +				nvidia,function = "sdmmc4";
>>>> +				nvidia,pull = <0>;
>>>> +				nvidia,tristate = <0>;
>>>> +			};
>>>> +			sdmmc4_dat0_paa0 {
>>>> +				nvidia,pins =	"sdmmc4_dat0_paa0",
>>>> +						"sdmmc4_dat1_paa1",
>>>> +						"sdmmc4_dat2_paa2",
>>>> +						"sdmmc4_dat3_paa3",
>>>> +						"sdmmc4_dat4_paa4",
>>>> +						"sdmmc4_dat5_paa5",
>>>> +						"sdmmc4_dat6_paa6",
>>>> +						"sdmmc4_dat7_paa7";
>>>> +				nvidia,function = "sdmmc4";
>>>> +				nvidia,pull = <2>;
>>>> +				nvidia,tristate = <0>;
>>>
>>> It seems it does not support per pin config for tegra30 and we have to
>>> separate them in different nodes with same group config value, right?
>>
>> Sorry, I don't understand the question.
>
> I meant for tegra, the config(not mux) in one pinctrl node functions
> on all entities in nvidia,pins, IOW, all pin or group must have the
> same config.
> So we can not set them differently in one node.
> 
> For example, considering if  sdmmc4_dat0_paa0 is nvidia,pull <0>
> while sdmmc4_dat1_paa1 is nvidia,pull <1>.
> Then we need to separate them in different pinctrl nodes, right?

Yes, that's true.

However, note that you can have more than one node affecting a
particular pin or group if you want. For example, you could have one set
of nodes that sets the mux, another set of nodes that sets
pullup/down/none, another set that sets tristate/driven. That way,
within each set of nodes, you get to group together all pins with
similar properties. This is all because we take the list of affected
pins/groups from the nvidia,pins property, rather than e.g. the node
name, so it's possible to list the same pin/group in multiple nodes. An
example from the Tegra Harmony board file:

There's one node for each mux function that's used, specifying which
pins/groups it's used on:

    ata {
            pins = "ata";
            function = "ide";
    };
    atb {
            nvidia,pins = "atb", "gma", "gme";
            nvidia,function = "sdio4";
    };
    ... many other nodes for other functions

There's also one node for each combination of pull and tristate that's
used, specifying which pins/groups it's used on:

    conf_ata {
            nvidia,pins = "ata", "atb", "atc", "atd", "ate",
                    "cdev1", "dap1", "dtb", "gma", "gmb",
                    "gmc", "gmd", "gme", "gpu7", "gpv",
                    "i2cp", "pta", "rm", "slxa", "slxk",
                    "spia", "spib";
            nvidia,pull = <0>;
            nvidia,tristate = <0>;
    };
    ... many other nodes for other pull/tristate combinations

I ran a script to group the various pins into nodes in different ways,
and this particular combination seemed to generate the smallest device
tree source file for Harmony.

Re: [PATCH V2 5/6] dt: Document Tegra20/30 pinctrl binding

[Stephen Warren]

On 03/21/2012 10:00 PM, Dong Aisheng wrote:
> On Wed, Mar 21, 2012 at 11:57:14PM +0800, Stephen Warren wrote:
>> On 03/21/2012 03:19 AM, Dong Aisheng wrote:
>>> On Wed, Mar 21, 2012 at 01:44:38AM +0800, Stephen Warren wrote:
>>>> Define a new binding for the Tegra pin controller, which is capable of
>>>> defining all aspects of desired pin multiplexing and pin configuration.
>>>> This is all based on the new common pinctrl bindings.
>>>>
>>>> Add Tegra30 binding based on Tegra20 binding.
...
>>>> +Example board file extract:
>>>> +
>>>> +	pinctrl@70000000 {
>>>> +		sdmmc4_default: pinmux {
>>>> +			sdmmc4_clk_pcc4 {
>>>> +				nvidia,pins =	"sdmmc4_clk_pcc4",
>>>> +						"sdmmc4_rst_n_pcc3";
>>>> +				nvidia,function = "sdmmc4";
>>>> +				nvidia,pull = <0>;
>>>> +				nvidia,tristate = <0>;
>>>> +			};
>>>> +			sdmmc4_dat0_paa0 {
>>>> +				nvidia,pins =	"sdmmc4_dat0_paa0",
>>>> +						"sdmmc4_dat1_paa1",
>>>> +						"sdmmc4_dat2_paa2",
>>>> +						"sdmmc4_dat3_paa3",
>>>> +						"sdmmc4_dat4_paa4",
>>>> +						"sdmmc4_dat5_paa5",
>>>> +						"sdmmc4_dat6_paa6",
>>>> +						"sdmmc4_dat7_paa7";
>>>> +				nvidia,function = "sdmmc4";
>>>> +				nvidia,pull = <2>;
>>>> +				nvidia,tristate = <0>;
>>>
>>> It seems it does not support per pin config for tegra30 and we have to
>>> separate them in different nodes with same group config value, right?
>>
>> Sorry, I don't understand the question.
>
> I meant for tegra, the config(not mux) in one pinctrl node functions
> on all entities in nvidia,pins, IOW, all pin or group must have the
> same config.
> So we can not set them differently in one node.
> 
> For example, considering if  sdmmc4_dat0_paa0 is nvidia,pull <0>
> while sdmmc4_dat1_paa1 is nvidia,pull <1>.
> Then we need to separate them in different pinctrl nodes, right?

Yes, that's true.

However, note that you can have more than one node affecting a
particular pin or group if you want. For example, you could have one set
of nodes that sets the mux, another set of nodes that sets
pullup/down/none, another set that sets tristate/driven. That way,
within each set of nodes, you get to group together all pins with
similar properties. This is all because we take the list of affected
pins/groups from the nvidia,pins property, rather than e.g. the node
name, so it's possible to list the same pin/group in multiple nodes. An
example from the Tegra Harmony board file:

There's one node for each mux function that's used, specifying which
pins/groups it's used on:

    ata {
            pins = "ata";
            function = "ide";
    };
    atb {
            nvidia,pins = "atb", "gma", "gme";
            nvidia,function = "sdio4";
    };
    ... many other nodes for other functions

There's also one node for each combination of pull and tristate that's
used, specifying which pins/groups it's used on:

    conf_ata {
            nvidia,pins = "ata", "atb", "atc", "atd", "ate",
                    "cdev1", "dap1", "dtb", "gma", "gmb",
                    "gmc", "gmd", "gme", "gpu7", "gpv",
                    "i2cp", "pta", "rm", "slxa", "slxk",
                    "spia", "spib";
            nvidia,pull = <0>;
            nvidia,tristate = <0>;
    };
    ... many other nodes for other pull/tristate combinations

I ran a script to group the various pins into nodes in different ways,
and this particular combination seemed to generate the smallest device
tree source file for Harmony.

Re: [PATCH V2 5/6] dt: Document Tegra20/30 pinctrl binding

[Dong Aisheng]

On Thu, Mar 22, 2012 at 11:45:31PM +0800, Stephen Warren wrote:
> On 03/21/2012 10:00 PM, Dong Aisheng wrote:
> > On Wed, Mar 21, 2012 at 11:57:14PM +0800, Stephen Warren wrote:
> >> On 03/21/2012 03:19 AM, Dong Aisheng wrote:
> >>> On Wed, Mar 21, 2012 at 01:44:38AM +0800, Stephen Warren wrote:
> >>>> Define a new binding for the Tegra pin controller, which is capable of
> >>>> defining all aspects of desired pin multiplexing and pin configuration.
> >>>> This is all based on the new common pinctrl bindings.
> >>>>
> >>>> Add Tegra30 binding based on Tegra20 binding.
> ...
> >>>> +Example board file extract:
> >>>> +
> >>>> +	pinctrl@70000000 {
> >>>> +		sdmmc4_default: pinmux {
> >>>> +			sdmmc4_clk_pcc4 {
> >>>> +				nvidia,pins =	"sdmmc4_clk_pcc4",
> >>>> +						"sdmmc4_rst_n_pcc3";
> >>>> +				nvidia,function = "sdmmc4";
> >>>> +				nvidia,pull = <0>;
> >>>> +				nvidia,tristate = <0>;
> >>>> +			};
> >>>> +			sdmmc4_dat0_paa0 {
> >>>> +				nvidia,pins =	"sdmmc4_dat0_paa0",
> >>>> +						"sdmmc4_dat1_paa1",
> >>>> +						"sdmmc4_dat2_paa2",
> >>>> +						"sdmmc4_dat3_paa3",
> >>>> +						"sdmmc4_dat4_paa4",
> >>>> +						"sdmmc4_dat5_paa5",
> >>>> +						"sdmmc4_dat6_paa6",
> >>>> +						"sdmmc4_dat7_paa7";
> >>>> +				nvidia,function = "sdmmc4";
> >>>> +				nvidia,pull = <2>;
> >>>> +				nvidia,tristate = <0>;
> >>>
> >>> It seems it does not support per pin config for tegra30 and we have to
> >>> separate them in different nodes with same group config value, right?
> >>
> >> Sorry, I don't understand the question.
> >
> > I meant for tegra, the config(not mux) in one pinctrl node functions
> > on all entities in nvidia,pins, IOW, all pin or group must have the
> > same config.
> > So we can not set them differently in one node.
> > 
> > For example, considering if  sdmmc4_dat0_paa0 is nvidia,pull <0>
> > while sdmmc4_dat1_paa1 is nvidia,pull <1>.
> > Then we need to separate them in different pinctrl nodes, right?
> 
> Yes, that's true.
> 
> However, note that you can have more than one node affecting a
> particular pin or group if you want. For example, you could have one set
> of nodes that sets the mux, another set of nodes that sets
> pullup/down/none, another set that sets tristate/driven. That way,
> within each set of nodes, you get to group together all pins with
> similar properties. This is all because we take the list of affected
> pins/groups from the nvidia,pins property, rather than e.g. the node
> name, so it's possible to list the same pin/group in multiple nodes. An
> example from the Tegra Harmony board file:
> 
> There's one node for each mux function that's used, specifying which
> pins/groups it's used on:
> 
>     ata {
>             pins = "ata";
>             function = "ide";
>     };
>     atb {
>             nvidia,pins = "atb", "gma", "gme";
>             nvidia,function = "sdio4";
>     };
>     ... many other nodes for other functions
> 
> There's also one node for each combination of pull and tristate that's
> used, specifying which pins/groups it's used on:
> 
>     conf_ata {
>             nvidia,pins = "ata", "atb", "atc", "atd", "ate",
>                     "cdev1", "dap1", "dtb", "gma", "gmb",
>                     "gmc", "gmd", "gme", "gpu7", "gpv",
>                     "i2cp", "pta", "rm", "slxa", "slxk",
>                     "spia", "spib";
>             nvidia,pull = <0>;
>             nvidia,tristate = <0>;
>     };
>     ... many other nodes for other pull/tristate combinations
> 
> I ran a script to group the various pins into nodes in different ways,
> and this particular combination seemed to generate the smallest device
> tree source file for Harmony.
> 

Hmm, either is ok to me. this way may save some properties and nodes.
But i still prefer to do pin mux and config at the same place as you did
in the binding doc like:
sdmmc4_clk_pcc4 {
	nvidia,pins =	"sdmmc4_clk_pcc4",
			"sdmmc4_rst_n_pcc3";
	nvidia,function = "sdmmc4";
	nvidia,pull = <0>;
	nvidia,tristate = <0>;
};
It's very easy to read and control.
Anyway, this is question how to write dts rather than the patch itself.
This patch is ok to me.

Regards
Dong Aisheng

Re: [PATCH V2 5/6] dt: Document Tegra20/30 pinctrl binding

[Dong Aisheng]

On Thu, Mar 22, 2012 at 11:45:31PM +0800, Stephen Warren wrote:
> On 03/21/2012 10:00 PM, Dong Aisheng wrote:
> > On Wed, Mar 21, 2012 at 11:57:14PM +0800, Stephen Warren wrote:
> >> On 03/21/2012 03:19 AM, Dong Aisheng wrote:
> >>> On Wed, Mar 21, 2012 at 01:44:38AM +0800, Stephen Warren wrote:
> >>>> Define a new binding for the Tegra pin controller, which is capable of
> >>>> defining all aspects of desired pin multiplexing and pin configuration.
> >>>> This is all based on the new common pinctrl bindings.
> >>>>
> >>>> Add Tegra30 binding based on Tegra20 binding.
> ...
> >>>> +Example board file extract:
> >>>> +
> >>>> +	pinctrl@70000000 {
> >>>> +		sdmmc4_default: pinmux {
> >>>> +			sdmmc4_clk_pcc4 {
> >>>> +				nvidia,pins =	"sdmmc4_clk_pcc4",
> >>>> +						"sdmmc4_rst_n_pcc3";
> >>>> +				nvidia,function = "sdmmc4";
> >>>> +				nvidia,pull = <0>;
> >>>> +				nvidia,tristate = <0>;
> >>>> +			};
> >>>> +			sdmmc4_dat0_paa0 {
> >>>> +				nvidia,pins =	"sdmmc4_dat0_paa0",
> >>>> +						"sdmmc4_dat1_paa1",
> >>>> +						"sdmmc4_dat2_paa2",
> >>>> +						"sdmmc4_dat3_paa3",
> >>>> +						"sdmmc4_dat4_paa4",
> >>>> +						"sdmmc4_dat5_paa5",
> >>>> +						"sdmmc4_dat6_paa6",
> >>>> +						"sdmmc4_dat7_paa7";
> >>>> +				nvidia,function = "sdmmc4";
> >>>> +				nvidia,pull = <2>;
> >>>> +				nvidia,tristate = <0>;
> >>>
> >>> It seems it does not support per pin config for tegra30 and we have to
> >>> separate them in different nodes with same group config value, right?
> >>
> >> Sorry, I don't understand the question.
> >
> > I meant for tegra, the config(not mux) in one pinctrl node functions
> > on all entities in nvidia,pins, IOW, all pin or group must have the
> > same config.
> > So we can not set them differently in one node.
> > 
> > For example, considering if  sdmmc4_dat0_paa0 is nvidia,pull <0>
> > while sdmmc4_dat1_paa1 is nvidia,pull <1>.
> > Then we need to separate them in different pinctrl nodes, right?
> 
> Yes, that's true.
> 
> However, note that you can have more than one node affecting a
> particular pin or group if you want. For example, you could have one set
> of nodes that sets the mux, another set of nodes that sets
> pullup/down/none, another set that sets tristate/driven. That way,
> within each set of nodes, you get to group together all pins with
> similar properties. This is all because we take the list of affected
> pins/groups from the nvidia,pins property, rather than e.g. the node
> name, so it's possible to list the same pin/group in multiple nodes. An
> example from the Tegra Harmony board file:
> 
> There's one node for each mux function that's used, specifying which
> pins/groups it's used on:
> 
>     ata {
>             pins = "ata";
>             function = "ide";
>     };
>     atb {
>             nvidia,pins = "atb", "gma", "gme";
>             nvidia,function = "sdio4";
>     };
>     ... many other nodes for other functions
> 
> There's also one node for each combination of pull and tristate that's
> used, specifying which pins/groups it's used on:
> 
>     conf_ata {
>             nvidia,pins = "ata", "atb", "atc", "atd", "ate",
>                     "cdev1", "dap1", "dtb", "gma", "gmb",
>                     "gmc", "gmd", "gme", "gpu7", "gpv",
>                     "i2cp", "pta", "rm", "slxa", "slxk",
>                     "spia", "spib";
>             nvidia,pull = <0>;
>             nvidia,tristate = <0>;
>     };
>     ... many other nodes for other pull/tristate combinations
> 
> I ran a script to group the various pins into nodes in different ways,
> and this particular combination seemed to generate the smallest device
> tree source file for Harmony.
> 

Hmm, either is ok to me. this way may save some properties and nodes.
But i still prefer to do pin mux and config at the same place as you did
in the binding doc like:
sdmmc4_clk_pcc4 {
	nvidia,pins =	"sdmmc4_clk_pcc4",
			"sdmmc4_rst_n_pcc3";
	nvidia,function = "sdmmc4";
	nvidia,pull = <0>;
	nvidia,tristate = <0>;
};
It's very easy to read and control.
Anyway, this is question how to write dts rather than the patch itself.
This patch is ok to me.

Regards
Dong Aisheng

[PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Stephen Warren]

Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
complete specification of the desired pinmux configuration using device
tree.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
v2: Rebase on of_property_for_each_string() API changes.
---
 drivers/pinctrl/pinctrl-tegra.c |  181 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 181 insertions(+), 0 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-tegra.c b/drivers/pinctrl/pinctrl-tegra.c
index 9b32968..304788f 100644
--- a/drivers/pinctrl/pinctrl-tegra.c
+++ b/drivers/pinctrl/pinctrl-tegra.c
@@ -23,9 +23,12 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/of_iter_prop.h>
+#include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 #include <linux/pinctrl/pinconf.h>
+#include <linux/slab.h>
 
 #include <mach/pinconf-tegra.h>
 
@@ -98,11 +101,189 @@ static void tegra_pinctrl_pin_dbg_show(struct pinctrl_dev *pctldev,
 	seq_printf(s, " " DRIVER_NAME);
 }
 
+static int add_map(struct pinctrl_map **map, unsigned *num_maps)
+{
+	unsigned old_num = *num_maps;
+	unsigned new_num = old_num + 1;
+	struct pinctrl_map *new_map;
+
+	new_map = krealloc(*map, sizeof(*new_map) * new_num, GFP_KERNEL);
+	if (!new_map)
+		return -ENOMEM;
+
+	memset(new_map + old_num, 0, sizeof(*new_map));
+
+	*map = new_map;
+	*num_maps = new_num;
+
+	return 0;
+}
+
+static int add_map_mux(struct pinctrl_map **map, unsigned *num_maps,
+		       const char *group, const char *function)
+{
+	unsigned i = *num_maps;
+	int ret;
+
+	ret = add_map(map, num_maps);
+	if (ret < 0)
+		return ret;
+
+	(*map)[i].type = PIN_MAP_TYPE_MUX_GROUP;
+	(*map)[i].data.mux.group = group;
+	(*map)[i].data.mux.function = function;
+
+	return 0;
+}
+
+static int add_map_configs(struct pinctrl_map **map, unsigned *num_maps,
+			   const char *group, unsigned long *configs,
+			   unsigned num_configs)
+{
+	unsigned i = *num_maps;
+	unsigned long *dup_configs;
+	int ret;
+
+	dup_configs = kmemdup(configs, num_configs * sizeof(*dup_configs),
+			      GFP_KERNEL);
+	if (!dup_configs)
+		return -ENOMEM;
+
+	ret = add_map(map, num_maps);
+	if (ret < 0)
+		return ret;
+
+	(*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
+	(*map)[i].data.configs.group_or_pin = group;
+	(*map)[i].data.configs.configs = dup_configs;
+	(*map)[i].data.configs.num_configs = num_configs;
+
+	return 0;
+}
+
+static int add_config(unsigned long **configs, unsigned *num_configs,
+		      unsigned long config)
+{
+	unsigned old_num = *num_configs;
+	unsigned new_num = old_num + 1;
+	unsigned long *new_configs;
+
+	new_configs = krealloc(*configs, sizeof(*new_configs) * new_num,
+			       GFP_KERNEL);
+	if (!new_configs)
+		return -ENOMEM;
+
+	new_configs[old_num] = config;
+
+	*configs = new_configs;
+	*num_configs = new_num;
+
+	return 0;
+}
+
+void tegra_pinctrl_dt_free_map(struct pinctrl_dev *pctldev,
+			       struct pinctrl_map *map, unsigned num_maps)
+{
+	int i;
+
+	for (i = 0; i < num_maps; i++)
+		if (map[i].type == PIN_MAP_TYPE_CONFIGS_GROUP)
+			kfree(map[i].data.configs.configs);
+
+	kfree(map);
+}
+
+static const struct cfg_param {
+	const char *property;
+	enum tegra_pinconf_param param;
+} cfg_params[] = {
+	{"nvidia,pull",			TEGRA_PINCONF_PARAM_PULL},
+	{"nvidia,tristate",		TEGRA_PINCONF_PARAM_TRISTATE},
+	{"nvidia,enable-input",		TEGRA_PINCONF_PARAM_ENABLE_INPUT},
+	{"nvidia,open-drain",		TEGRA_PINCONF_PARAM_OPEN_DRAIN},
+	{"nvidia,lock",			TEGRA_PINCONF_PARAM_LOCK},
+	{"nvidia,io-reset",		TEGRA_PINCONF_PARAM_IORESET},
+	{"nvidia,high-speed-mode",	TEGRA_PINCONF_PARAM_HIGH_SPEED_MODE},
+	{"nvidia,schmitt",		TEGRA_PINCONF_PARAM_SCHMITT},
+	{"nvidia,low-power-mode",	TEGRA_PINCONF_PARAM_LOW_POWER_MODE},
+	{"nvidia,pull-down-strength",	TEGRA_PINCONF_PARAM_DRIVE_DOWN_STRENGTH},
+	{"nvidia,pull-up-strength",	TEGRA_PINCONF_PARAM_DRIVE_UP_STRENGTH},
+	{"nvidia,slew-rate-falling",	TEGRA_PINCONF_PARAM_SLEW_RATE_FALLING},
+	{"nvidia,slew-rate-rising",	TEGRA_PINCONF_PARAM_SLEW_RATE_RISING},
+};
+
+int tegra_pinctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
+				 struct device_node *np_config,
+				 struct pinctrl_map **map, unsigned *num_maps)
+{
+	struct device_node *np;
+	unsigned long config;
+	const char *function;
+	unsigned long *configs = NULL;
+	unsigned num_configs = 0;
+	u32 val;
+	int ret, i;
+	struct property *prop;
+	const char *group;
+
+	*map = NULL;
+	*num_maps = 0;
+
+	for_each_child_of_node(np_config, np) {
+		ret = of_property_read_string(np, "nvidia,function", &function);
+		if (ret < 0)
+			function = NULL;
+
+		for (i = 0; i < ARRAY_SIZE(cfg_params); i++) {
+			ret = of_property_read_u32(np, cfg_params[i].property,
+						   &val);
+			if (!ret) {
+				config = TEGRA_PINCONF_PACK(
+						cfg_params[i].param, val);
+				ret = add_config(&configs, &num_configs,
+						 config);
+				if (ret < 0)
+					goto error;
+			}
+		}
+
+		of_property_for_each_string(np, "nvidia,pins", prop, group) {
+			if (function) {
+				ret = add_map_mux(map, num_maps, group,
+						  function);
+				if (ret < 0)
+					goto error;
+			}
+
+			if (num_configs) {
+				ret = add_map_configs(map, num_maps, group,
+						      configs, num_configs);
+				if (ret < 0)
+					goto error;
+			}
+		}
+
+		kfree(configs);
+		configs = NULL;
+		num_configs = 0;
+	}
+
+	return 0;
+
+error:
+	kfree(configs);
+	tegra_pinctrl_dt_free_map(pctldev, *map, *num_maps);
+
+	return ret;
+}
+
 static struct pinctrl_ops tegra_pinctrl_ops = {
 	.list_groups = tegra_pinctrl_list_groups,
 	.get_group_name = tegra_pinctrl_get_group_name,
 	.get_group_pins = tegra_pinctrl_get_group_pins,
 	.pin_dbg_show = tegra_pinctrl_pin_dbg_show,
+	.dt_node_to_map = tegra_pinctrl_dt_node_to_map,
+	.dt_free_map = tegra_pinctrl_dt_free_map,
 };
 
 static int tegra_pinctrl_list_funcs(struct pinctrl_dev *pctldev,
-- 
1.7.0.4

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Dong Aisheng]

On Wed, Mar 21, 2012 at 01:44:39AM +0800, Stephen Warren wrote:
> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
> complete specification of the desired pinmux configuration using device
> tree.
> 
> Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> ---
> v2: Rebase on of_property_for_each_string() API changes.
> ---
Nice code and a good example to people.

A small suggestion below:
> +static int add_map_configs(struct pinctrl_map **map, unsigned *num_maps,
> +			   const char *group, unsigned long *configs,
> +			   unsigned num_configs)
> +{
> +	unsigned i = *num_maps;
> +	unsigned long *dup_configs;
> +	int ret;
> +
> +	dup_configs = kmemdup(configs, num_configs * sizeof(*dup_configs),
> +			      GFP_KERNEL);
> +	if (!dup_configs)
> +		return -ENOMEM;
> +
> +	ret = add_map(map, num_maps);
> +	if (ret < 0)
> +		return ret;
> +
> +	(*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
It still does not support PIN_MAP_TYPE_CONFIGS_PIN, right?

> +	for_each_child_of_node(np_config, np) {
> +		ret = of_property_read_string(np, "nvidia,function", &function);
> +		if (ret < 0)
> +			function = NULL;
> +
> +		for (i = 0; i < ARRAY_SIZE(cfg_params); i++) {
> +			ret = of_property_read_u32(np, cfg_params[i].property,
> +						   &val);
> +			if (!ret) {
> +				config = TEGRA_PINCONF_PACK(
> +						cfg_params[i].param, val);
> +				ret = add_config(&configs, &num_configs,
> +						 config);
> +				if (ret < 0)
> +					goto error;
> +			}
> +		}
> +
> +		of_property_for_each_string(np, "nvidia,pins", prop, group) {
If we calculate out the strings count and allocate corresponding size array, we may not
need to keep krealloc the maps and configs array size for each entry.
And this may be a little higher efficient.

Regards
Dong Aisheng

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Dong Aisheng]

On Wed, Mar 21, 2012 at 01:44:39AM +0800, Stephen Warren wrote:
> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
> complete specification of the desired pinmux configuration using device
> tree.
> 
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
> v2: Rebase on of_property_for_each_string() API changes.
> ---
Nice code and a good example to people.

A small suggestion below:
> +static int add_map_configs(struct pinctrl_map **map, unsigned *num_maps,
> +			   const char *group, unsigned long *configs,
> +			   unsigned num_configs)
> +{
> +	unsigned i = *num_maps;
> +	unsigned long *dup_configs;
> +	int ret;
> +
> +	dup_configs = kmemdup(configs, num_configs * sizeof(*dup_configs),
> +			      GFP_KERNEL);
> +	if (!dup_configs)
> +		return -ENOMEM;
> +
> +	ret = add_map(map, num_maps);
> +	if (ret < 0)
> +		return ret;
> +
> +	(*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
It still does not support PIN_MAP_TYPE_CONFIGS_PIN, right?

> +	for_each_child_of_node(np_config, np) {
> +		ret = of_property_read_string(np, "nvidia,function", &function);
> +		if (ret < 0)
> +			function = NULL;
> +
> +		for (i = 0; i < ARRAY_SIZE(cfg_params); i++) {
> +			ret = of_property_read_u32(np, cfg_params[i].property,
> +						   &val);
> +			if (!ret) {
> +				config = TEGRA_PINCONF_PACK(
> +						cfg_params[i].param, val);
> +				ret = add_config(&configs, &num_configs,
> +						 config);
> +				if (ret < 0)
> +					goto error;
> +			}
> +		}
> +
> +		of_property_for_each_string(np, "nvidia,pins", prop, group) {
If we calculate out the strings count and allocate corresponding size array, we may not
need to keep krealloc the maps and configs array size for each entry.
And this may be a little higher efficient.

Regards
Dong Aisheng

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Stephen Warren]

On 03/21/2012 03:35 AM, Dong Aisheng wrote:
> On Wed, Mar 21, 2012 at 01:44:39AM +0800, Stephen Warren wrote:
>> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
>> complete specification of the desired pinmux configuration using device
>> tree.
>>
>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>> ---
>> v2: Rebase on of_property_for_each_string() API changes.
>> ---
> Nice code and a good example to people.
> 
> A small suggestion below:
>> +static int add_map_configs(struct pinctrl_map **map, unsigned *num_maps,
>> +			   const char *group, unsigned long *configs,
>> +			   unsigned num_configs)
>> +{
>> +	unsigned i = *num_maps;
>> +	unsigned long *dup_configs;
>> +	int ret;
>> +
>> +	dup_configs = kmemdup(configs, num_configs * sizeof(*dup_configs),
>> +			      GFP_KERNEL);
>> +	if (!dup_configs)
>> +		return -ENOMEM;
>> +
>> +	ret = add_map(map, num_maps);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	(*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
>
> It still does not support PIN_MAP_TYPE_CONFIGS_PIN, right?

Yes.

This is mainly due to a pinctrl core limitation. The core only supports
muxing on groups, so even though the Tegra30 HW supports muxing per pin,
the driver must define a group for each pin. Given that, it's simplest
just to do all the pin config on those same groups.

If/when the pinctrl core supports muxing per pin, we can take advantage
of this within the Tegra pinctrl driver without affecting the binding at
all.

>> +	for_each_child_of_node(np_config, np) {
>> +		ret = of_property_read_string(np, "nvidia,function", &function);
>> +		if (ret < 0)
>> +			function = NULL;
>> +
>> +		for (i = 0; i < ARRAY_SIZE(cfg_params); i++) {
>> +			ret = of_property_read_u32(np, cfg_params[i].property,
>> +						   &val);
>> +			if (!ret) {
>> +				config = TEGRA_PINCONF_PACK(
>> +						cfg_params[i].param, val);
>> +				ret = add_config(&configs, &num_configs,
>> +						 config);
>> +				if (ret < 0)
>> +					goto error;
>> +			}
>> +		}
>> +
>> +		of_property_for_each_string(np, "nvidia,pins", prop, group) {
>
> If we calculate out the strings count and allocate corresponding size array, we may not
> need to keep krealloc the maps and configs array size for each entry.
> And this may be a little higher efficient.

That's true. However, it'd require the code to loop once to determine
how many properties are present and how many entries there are in the
pin list. Then, loop again to actually construct the mapping table
array. This is all added complexity that doesn't affect correctness. I'd
rather get the simple code going first, and then refine it later if
there turns out to be a performance issue.

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Stephen Warren]

On 03/21/2012 03:35 AM, Dong Aisheng wrote:
> On Wed, Mar 21, 2012 at 01:44:39AM +0800, Stephen Warren wrote:
>> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
>> complete specification of the desired pinmux configuration using device
>> tree.
>>
>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>> ---
>> v2: Rebase on of_property_for_each_string() API changes.
>> ---
> Nice code and a good example to people.
> 
> A small suggestion below:
>> +static int add_map_configs(struct pinctrl_map **map, unsigned *num_maps,
>> +			   const char *group, unsigned long *configs,
>> +			   unsigned num_configs)
>> +{
>> +	unsigned i = *num_maps;
>> +	unsigned long *dup_configs;
>> +	int ret;
>> +
>> +	dup_configs = kmemdup(configs, num_configs * sizeof(*dup_configs),
>> +			      GFP_KERNEL);
>> +	if (!dup_configs)
>> +		return -ENOMEM;
>> +
>> +	ret = add_map(map, num_maps);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	(*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
>
> It still does not support PIN_MAP_TYPE_CONFIGS_PIN, right?

Yes.

This is mainly due to a pinctrl core limitation. The core only supports
muxing on groups, so even though the Tegra30 HW supports muxing per pin,
the driver must define a group for each pin. Given that, it's simplest
just to do all the pin config on those same groups.

If/when the pinctrl core supports muxing per pin, we can take advantage
of this within the Tegra pinctrl driver without affecting the binding at
all.

>> +	for_each_child_of_node(np_config, np) {
>> +		ret = of_property_read_string(np, "nvidia,function", &function);
>> +		if (ret < 0)
>> +			function = NULL;
>> +
>> +		for (i = 0; i < ARRAY_SIZE(cfg_params); i++) {
>> +			ret = of_property_read_u32(np, cfg_params[i].property,
>> +						   &val);
>> +			if (!ret) {
>> +				config = TEGRA_PINCONF_PACK(
>> +						cfg_params[i].param, val);
>> +				ret = add_config(&configs, &num_configs,
>> +						 config);
>> +				if (ret < 0)
>> +					goto error;
>> +			}
>> +		}
>> +
>> +		of_property_for_each_string(np, "nvidia,pins", prop, group) {
>
> If we calculate out the strings count and allocate corresponding size array, we may not
> need to keep krealloc the maps and configs array size for each entry.
> And this may be a little higher efficient.

That's true. However, it'd require the code to loop once to determine
how many properties are present and how many entries there are in the
pin list. Then, loop again to actually construct the mapping table
array. This is all added complexity that doesn't affect correctness. I'd
rather get the simple code going first, and then refine it later if
there turns out to be a performance issue.

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Dong Aisheng]

On Thu, Mar 22, 2012 at 12:07:27AM +0800, Stephen Warren wrote:
> On 03/21/2012 03:35 AM, Dong Aisheng wrote:
> > On Wed, Mar 21, 2012 at 01:44:39AM +0800, Stephen Warren wrote:
> >> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
> >> complete specification of the desired pinmux configuration using device
> >> tree.
> >>
> >> Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> >> ---
> >> v2: Rebase on of_property_for_each_string() API changes.
> >> ---
> > Nice code and a good example to people.
> > 
> > A small suggestion below:
> >> +static int add_map_configs(struct pinctrl_map **map, unsigned *num_maps,
> >> +			   const char *group, unsigned long *configs,
> >> +			   unsigned num_configs)
> >> +{
> >> +	unsigned i = *num_maps;
> >> +	unsigned long *dup_configs;
> >> +	int ret;
> >> +
> >> +	dup_configs = kmemdup(configs, num_configs * sizeof(*dup_configs),
> >> +			      GFP_KERNEL);
> >> +	if (!dup_configs)
> >> +		return -ENOMEM;
> >> +
> >> +	ret = add_map(map, num_maps);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	(*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
> >
> > It still does not support PIN_MAP_TYPE_CONFIGS_PIN, right?
> 
> Yes.
> 
> This is mainly due to a pinctrl core limitation. The core only supports
> muxing on groups, so even though the Tegra30 HW supports muxing per pin,
> the driver must define a group for each pin. Given that, it's simplest
> just to do all the pin config on those same groups.
> 
> If/when the pinctrl core supports muxing per pin, we can take advantage
> of this within the Tegra pinctrl driver without affecting the binding at
> all.
> 
Yes, reasonable.

> >> +	for_each_child_of_node(np_config, np) {
> >> +		ret = of_property_read_string(np, "nvidia,function", &function);
> >> +		if (ret < 0)
> >> +			function = NULL;
> >> +
> >> +		for (i = 0; i < ARRAY_SIZE(cfg_params); i++) {
> >> +			ret = of_property_read_u32(np, cfg_params[i].property,
> >> +						   &val);
> >> +			if (!ret) {
> >> +				config = TEGRA_PINCONF_PACK(
> >> +						cfg_params[i].param, val);
> >> +				ret = add_config(&configs, &num_configs,
> >> +						 config);
> >> +				if (ret < 0)
> >> +					goto error;
> >> +			}
> >> +		}
> >> +
> >> +		of_property_for_each_string(np, "nvidia,pins", prop, group) {
> >
> > If we calculate out the strings count and allocate corresponding size array, we may not
> > need to keep krealloc the maps and configs array size for each entry.
> > And this may be a little higher efficient.
> 
> That's true. However, it'd require the code to loop once to determine
> how many properties are present and how many entries there are in the
> pin list. Then, loop again to actually construct the mapping table
> array. This is all added complexity that doesn't affect correctness. I'd
> rather get the simple code going first, and then refine it later if
> there turns out to be a performance issue.
> 
Can we use of_property_count_strings?

Regards
Dong Aisheng

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Dong Aisheng]

On Thu, Mar 22, 2012 at 12:07:27AM +0800, Stephen Warren wrote:
> On 03/21/2012 03:35 AM, Dong Aisheng wrote:
> > On Wed, Mar 21, 2012 at 01:44:39AM +0800, Stephen Warren wrote:
> >> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
> >> complete specification of the desired pinmux configuration using device
> >> tree.
> >>
> >> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> >> ---
> >> v2: Rebase on of_property_for_each_string() API changes.
> >> ---
> > Nice code and a good example to people.
> > 
> > A small suggestion below:
> >> +static int add_map_configs(struct pinctrl_map **map, unsigned *num_maps,
> >> +			   const char *group, unsigned long *configs,
> >> +			   unsigned num_configs)
> >> +{
> >> +	unsigned i = *num_maps;
> >> +	unsigned long *dup_configs;
> >> +	int ret;
> >> +
> >> +	dup_configs = kmemdup(configs, num_configs * sizeof(*dup_configs),
> >> +			      GFP_KERNEL);
> >> +	if (!dup_configs)
> >> +		return -ENOMEM;
> >> +
> >> +	ret = add_map(map, num_maps);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	(*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
> >
> > It still does not support PIN_MAP_TYPE_CONFIGS_PIN, right?
> 
> Yes.
> 
> This is mainly due to a pinctrl core limitation. The core only supports
> muxing on groups, so even though the Tegra30 HW supports muxing per pin,
> the driver must define a group for each pin. Given that, it's simplest
> just to do all the pin config on those same groups.
> 
> If/when the pinctrl core supports muxing per pin, we can take advantage
> of this within the Tegra pinctrl driver without affecting the binding at
> all.
> 
Yes, reasonable.

> >> +	for_each_child_of_node(np_config, np) {
> >> +		ret = of_property_read_string(np, "nvidia,function", &function);
> >> +		if (ret < 0)
> >> +			function = NULL;
> >> +
> >> +		for (i = 0; i < ARRAY_SIZE(cfg_params); i++) {
> >> +			ret = of_property_read_u32(np, cfg_params[i].property,
> >> +						   &val);
> >> +			if (!ret) {
> >> +				config = TEGRA_PINCONF_PACK(
> >> +						cfg_params[i].param, val);
> >> +				ret = add_config(&configs, &num_configs,
> >> +						 config);
> >> +				if (ret < 0)
> >> +					goto error;
> >> +			}
> >> +		}
> >> +
> >> +		of_property_for_each_string(np, "nvidia,pins", prop, group) {
> >
> > If we calculate out the strings count and allocate corresponding size array, we may not
> > need to keep krealloc the maps and configs array size for each entry.
> > And this may be a little higher efficient.
> 
> That's true. However, it'd require the code to loop once to determine
> how many properties are present and how many entries there are in the
> pin list. Then, loop again to actually construct the mapping table
> array. This is all added complexity that doesn't affect correctness. I'd
> rather get the simple code going first, and then refine it later if
> there turns out to be a performance issue.
> 
Can we use of_property_count_strings?

Regards
Dong Aisheng

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Stephen Warren]

On 03/21/2012 10:07 PM, Dong Aisheng wrote:
> On Thu, Mar 22, 2012 at 12:07:27AM +0800, Stephen Warren wrote:
>> On 03/21/2012 03:35 AM, Dong Aisheng wrote:
>>> On Wed, Mar 21, 2012 at 01:44:39AM +0800, Stephen Warren wrote:
>>>> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
>>>> complete specification of the desired pinmux configuration using device
>>>> tree.
...
>>>> +	for_each_child_of_node(np_config, np) {
>>>> +		ret = of_property_read_string(np, "nvidia,function", &function);
>>>> +		if (ret < 0)
>>>> +			function = NULL;
>>>> +
>>>> +		for (i = 0; i < ARRAY_SIZE(cfg_params); i++) {
>>>> +			ret = of_property_read_u32(np, cfg_params[i].property,
>>>> +						   &val);
>>>> +			if (!ret) {
>>>> +				config = TEGRA_PINCONF_PACK(
>>>> +						cfg_params[i].param, val);
>>>> +				ret = add_config(&configs, &num_configs,
>>>> +						 config);
>>>> +				if (ret < 0)
>>>> +					goto error;
>>>> +			}
>>>> +		}
>>>> +
>>>> +		of_property_for_each_string(np, "nvidia,pins", prop, group) {
>>>
>>> If we calculate out the strings count and allocate corresponding size array, we may not
>>> need to keep krealloc the maps and configs array size for each entry.
>>> And this may be a little higher efficient.
>>
>> That's true. However, it'd require the code to loop once to determine
>> how many properties are present and how many entries there are in the
>> pin list. Then, loop again to actually construct the mapping table
>> array. This is all added complexity that doesn't affect correctness. I'd
>> rather get the simple code going first, and then refine it later if
>> there turns out to be a performance issue.
>>
> Can we use of_property_count_strings?

It'd be possible to avoid some of the reallocs this way. We could
realloc once per node rather than once per (node, pin). It does make the
code a bit more complex though, since you have to reserve space up-front
during the one realloc and so have to store separate num_maps and
num_maps_allocated. I'll see how bad it gets, and maybe include it in v3
if it isn't horrible.

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Stephen Warren]

On 03/21/2012 10:07 PM, Dong Aisheng wrote:
> On Thu, Mar 22, 2012 at 12:07:27AM +0800, Stephen Warren wrote:
>> On 03/21/2012 03:35 AM, Dong Aisheng wrote:
>>> On Wed, Mar 21, 2012 at 01:44:39AM +0800, Stephen Warren wrote:
>>>> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
>>>> complete specification of the desired pinmux configuration using device
>>>> tree.
...
>>>> +	for_each_child_of_node(np_config, np) {
>>>> +		ret = of_property_read_string(np, "nvidia,function", &function);
>>>> +		if (ret < 0)
>>>> +			function = NULL;
>>>> +
>>>> +		for (i = 0; i < ARRAY_SIZE(cfg_params); i++) {
>>>> +			ret = of_property_read_u32(np, cfg_params[i].property,
>>>> +						   &val);
>>>> +			if (!ret) {
>>>> +				config = TEGRA_PINCONF_PACK(
>>>> +						cfg_params[i].param, val);
>>>> +				ret = add_config(&configs, &num_configs,
>>>> +						 config);
>>>> +				if (ret < 0)
>>>> +					goto error;
>>>> +			}
>>>> +		}
>>>> +
>>>> +		of_property_for_each_string(np, "nvidia,pins", prop, group) {
>>>
>>> If we calculate out the strings count and allocate corresponding size array, we may not
>>> need to keep krealloc the maps and configs array size for each entry.
>>> And this may be a little higher efficient.
>>
>> That's true. However, it'd require the code to loop once to determine
>> how many properties are present and how many entries there are in the
>> pin list. Then, loop again to actually construct the mapping table
>> array. This is all added complexity that doesn't affect correctness. I'd
>> rather get the simple code going first, and then refine it later if
>> there turns out to be a performance issue.
>>
> Can we use of_property_count_strings?

It'd be possible to avoid some of the reallocs this way. We could
realloc once per node rather than once per (node, pin). It does make the
code a bit more complex though, since you have to reserve space up-front
during the one realloc and so have to store separate num_maps and
num_maps_allocated. I'll see how bad it gets, and maybe include it in v3
if it isn't horrible.

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Linus Walleij]

On Tue, Mar 20, 2012 at 6:44 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
> complete specification of the desired pinmux configuration using device
> tree.
>
> Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> ---
> v2: Rebase on of_property_for_each_string() API changes.

Stephen is this eligible for merge? Or does it need changes on top
of the v4 core patch?

Yours,
Linus Walleij

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Linus Walleij]

On Tue, Mar 20, 2012 at 6:44 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
> complete specification of the desired pinmux configuration using device
> tree.
>
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
> v2: Rebase on of_property_for_each_string() API changes.

Stephen is this eligible for merge? Or does it need changes on top
of the v4 core patch?

Yours,
Linus Walleij

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Stephen Warren]

On 04/01/2012 11:29 AM, Linus Walleij wrote:
> On Tue, Mar 20, 2012 at 6:44 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
>> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
>> complete specification of the desired pinmux configuration using device
>> tree.
>>
>> Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
>> ---
>> v2: Rebase on of_property_for_each_string() API changes.
> 
> Stephen is this eligible for merge? Or does it need changes on top
> of the v4 core patch?

I believe if you take everything from v3, except replace the one patch
that I posted a v4 for, and you'll be set. I don't believe the v4
changes in the intermediate patch required and changes to any later
patches. Let me know if you want me to repost the series, thanks!

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Stephen Warren]

On 04/01/2012 11:29 AM, Linus Walleij wrote:
> On Tue, Mar 20, 2012 at 6:44 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
>> complete specification of the desired pinmux configuration using device
>> tree.
>>
>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>> ---
>> v2: Rebase on of_property_for_each_string() API changes.
> 
> Stephen is this eligible for merge? Or does it need changes on top
> of the v4 core patch?

I believe if you take everything from v3, except replace the one patch
that I posted a v4 for, and you'll be set. I don't believe the v4
changes in the intermediate patch required and changes to any later
patches. Let me know if you want me to repost the series, thanks!

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Linus Walleij]

On Mon, Apr 2, 2012 at 5:34 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 04/01/2012 11:29 AM, Linus Walleij wrote:
>> On Tue, Mar 20, 2012 at 6:44 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>
>>> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
>>> complete specification of the desired pinmux configuration using device
>>> tree.
>>>
>>> Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
>>> ---
>>> v2: Rebase on of_property_for_each_string() API changes.
>>
>> Stephen is this eligible for merge? Or does it need changes on top
>> of the v4 core patch?
>
> I believe if you take everything from v3, except replace the one patch
> that I posted a v4 for, and you'll be set. I don't believe the v4
> changes in the intermediate patch required and changes to any later
> patches. Let me know if you want me to repost the series, thanks!

Hm. I went in and applied Viresh's group enumeration refactoring
patch so I suspect tehy need some rebasing. Care to rebase to
the branch in -next tomorrow or so and I'll apply?

Yours,
Linus Walleij

Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Linus Walleij]

On Mon, Apr 2, 2012 at 5:34 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/01/2012 11:29 AM, Linus Walleij wrote:
>> On Tue, Mar 20, 2012 at 6:44 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>
>>> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
>>> complete specification of the desired pinmux configuration using device
>>> tree.
>>>
>>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>>> ---
>>> v2: Rebase on of_property_for_each_string() API changes.
>>
>> Stephen is this eligible for merge? Or does it need changes on top
>> of the v4 core patch?
>
> I believe if you take everything from v3, except replace the one patch
> that I posted a v4 for, and you'll be set. I don't believe the v4
> changes in the intermediate patch required and changes to any later
> patches. Let me know if you want me to repost the series, thanks!

Hm. I went in and applied Viresh's group enumeration refactoring
patch so I suspect tehy need some rebasing. Care to rebase to
the branch in -next tomorrow or so and I'll apply?

Yours,
Linus Walleij

Re: [PATCH V2 1/6] dt: add property iteration helpers

[Rob Herring]

On 03/20/2012 12:44 PM, Stephen Warren wrote:
> This patch adds macros of_property_for_each_u32() and
> of_property_for_each_string(), which iterate over an array of values
> within a device-tree property. Usage is for example:
> 
> struct property *prop;
> const __be32 *p;
> u32 u;
> of_property_for_each_u32(np, "propname", prop, p, u)
> 	printk("U32 value: %x\n", u);
> 
> struct property *prop;
> const char *s;
> of_property_for_each_string(np, "propname", prop, s)
> 	printk("String value: %s\n", s);
> 
> Based on work by Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Signed-off-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> ---
> v2: Simplified the implementation per suggestion by Rob Herring.
> ---
>  include/linux/of_iter_prop.h |  101 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 101 insertions(+), 0 deletions(-)
>  create mode 100644 include/linux/of_iter_prop.h

I don't think this makes sense to be a new header as it's use is pretty
much the same as other property functions. I would un-inline the helper
functions to of/base.c and move the defines to of.h.

Rob

> diff --git a/include/linux/of_iter_prop.h b/include/linux/of_iter_prop.h
> new file mode 100644
> index 0000000..e3df23c
> --- /dev/null
> +++ b/include/linux/of_iter_prop.h
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright (c) 2011-2012 NVIDIA CORPORATION. All rights reserved.
> + *
> + * Iterate over properties that store arrays.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#ifndef __OF_ITER_PROP_H__
> +#define __OF_ITER_PROP_H__
> +
> +#include <linux/of.h>
> +
> +#ifdef CONFIG_OF
> +static inline const __be32 *of_prop_next_u32(struct property *prop,
> +					     const __be32 *cur, u32 *pu)
> +{
> +	const void *curv = cur;
> +
> +	if (!prop)
> +		return NULL;
> +
> +	if (!cur) {
> +		curv = prop->value;
> +		goto out_val;
> +	}
> +
> +	curv += sizeof(*cur);
> +	if (curv >= prop->value + prop->length)
> +		return NULL;
> +
> +out_val:
> +	*pu = be32_to_cpup(curv);
> +	return curv;
> +}
> +
> +/*
> + * struct property *prop;
> + * const __be32 *p;
> + * u32 u;
> + *
> + * of_property_for_each_u32(np, "propname", prop, p, u)
> + *         printk("U32 value: %x\n", u);
> + */
> +#define of_property_for_each_u32(np, propname, prop, p, u)	\
> +	for (prop = of_find_property(np, propname, NULL),	\
> +		p = of_prop_next_u32(prop, NULL, &u);		\
> +		p;						\
> +		p = of_prop_next_u32(prop, p, &u))
> +
> +static inline const char *of_prop_next_string(struct property *prop,
> +					      const char *cur)
> +{
> +	const void *curv = cur;
> +
> +	if (!prop)
> +		return NULL;
> +
> +	if (!cur)
> +		return prop->value;
> +
> +	curv += strlen(cur) + 1;
> +	if (curv >= prop->value + prop->length)
> +		return NULL;
> +
> +	return curv;
> +}
> +
> +/*
> + * struct property *prop;
> + * const char *s;
> + *
> + * of_property_for_each_string(np, "propname", prop, s)
> + *         printk("String value: %s\n", s);
> + */
> +#define of_property_for_each_string(np, propname, prop, s)	\
> +	for (prop = of_find_property(np, propname, NULL),	\
> +		s = of_prop_next_string(prop, NULL);		\
> +		s;						\
> +		s = of_prop_next_string(prop, s))
> +
> +#else
> +
> +#define of_property_for_each_u32(np, propname, prop, p, u) \
> +	while (0)
> +
> +#define of_property_for_each_string(np, propname, prop, s) \
> +	while (0)
> +
> +#endif /* CONFIG_OF */
> +
> +#endif /* __OF_ITER_PROP_H__ */

Re: [PATCH V2 1/6] dt: add property iteration helpers

[Rob Herring]

On 03/20/2012 12:44 PM, Stephen Warren wrote:
> This patch adds macros of_property_for_each_u32() and
> of_property_for_each_string(), which iterate over an array of values
> within a device-tree property. Usage is for example:
> 
> struct property *prop;
> const __be32 *p;
> u32 u;
> of_property_for_each_u32(np, "propname", prop, p, u)
> 	printk("U32 value: %x\n", u);
> 
> struct property *prop;
> const char *s;
> of_property_for_each_string(np, "propname", prop, s)
> 	printk("String value: %s\n", s);
> 
> Based on work by Rob Herring <robherring2@gmail.com>
> 
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
> v2: Simplified the implementation per suggestion by Rob Herring.
> ---
>  include/linux/of_iter_prop.h |  101 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 101 insertions(+), 0 deletions(-)
>  create mode 100644 include/linux/of_iter_prop.h

I don't think this makes sense to be a new header as it's use is pretty
much the same as other property functions. I would un-inline the helper
functions to of/base.c and move the defines to of.h.

Rob

> diff --git a/include/linux/of_iter_prop.h b/include/linux/of_iter_prop.h
> new file mode 100644
> index 0000000..e3df23c
> --- /dev/null
> +++ b/include/linux/of_iter_prop.h
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright (c) 2011-2012 NVIDIA CORPORATION. All rights reserved.
> + *
> + * Iterate over properties that store arrays.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#ifndef __OF_ITER_PROP_H__
> +#define __OF_ITER_PROP_H__
> +
> +#include <linux/of.h>
> +
> +#ifdef CONFIG_OF
> +static inline const __be32 *of_prop_next_u32(struct property *prop,
> +					     const __be32 *cur, u32 *pu)
> +{
> +	const void *curv = cur;
> +
> +	if (!prop)
> +		return NULL;
> +
> +	if (!cur) {
> +		curv = prop->value;
> +		goto out_val;
> +	}
> +
> +	curv += sizeof(*cur);
> +	if (curv >= prop->value + prop->length)
> +		return NULL;
> +
> +out_val:
> +	*pu = be32_to_cpup(curv);
> +	return curv;
> +}
> +
> +/*
> + * struct property *prop;
> + * const __be32 *p;
> + * u32 u;
> + *
> + * of_property_for_each_u32(np, "propname", prop, p, u)
> + *         printk("U32 value: %x\n", u);
> + */
> +#define of_property_for_each_u32(np, propname, prop, p, u)	\
> +	for (prop = of_find_property(np, propname, NULL),	\
> +		p = of_prop_next_u32(prop, NULL, &u);		\
> +		p;						\
> +		p = of_prop_next_u32(prop, p, &u))
> +
> +static inline const char *of_prop_next_string(struct property *prop,
> +					      const char *cur)
> +{
> +	const void *curv = cur;
> +
> +	if (!prop)
> +		return NULL;
> +
> +	if (!cur)
> +		return prop->value;
> +
> +	curv += strlen(cur) + 1;
> +	if (curv >= prop->value + prop->length)
> +		return NULL;
> +
> +	return curv;
> +}
> +
> +/*
> + * struct property *prop;
> + * const char *s;
> + *
> + * of_property_for_each_string(np, "propname", prop, s)
> + *         printk("String value: %s\n", s);
> + */
> +#define of_property_for_each_string(np, propname, prop, s)	\
> +	for (prop = of_find_property(np, propname, NULL),	\
> +		s = of_prop_next_string(prop, NULL);		\
> +		s;						\
> +		s = of_prop_next_string(prop, s))
> +
> +#else
> +
> +#define of_property_for_each_u32(np, propname, prop, p, u) \
> +	while (0)
> +
> +#define of_property_for_each_string(np, propname, prop, s) \
> +	while (0)
> +
> +#endif /* CONFIG_OF */
> +
> +#endif /* __OF_ITER_PROP_H__ */