[PATCH 0/4] pinctrl: add helpers for group based drivers

[PATCH 0/4] pinctrl: add helpers for group based drivers

[Antoine Tenart]

Linus, Sebastian,

As discussed earlier this year[1], this series introduce helpers for group based
pinctrl drivers:
- of_pinctrl_utils_read_function(): reads the function name of a
  specified node, and gets the number of groups it should be
  applied to.
- of_pinctrl_for_each_function_group(): navigates through the groups of
  a specified node, reading at each iteration the name of the current
  group.

A generic function to parse nodes for group based drivers is also added, and
then used in the Berlin pinctrl driver:
- pinconf_generic_function_groups_dt_node_to_map()

[1] https://lkml.org/lkml/2014/5/17/38


Antoine Tenart (4):
  Documentation: bindings: pinctrl: document the generic groups property
  pinctrl: add helpers for group based drivers
  pinctrl: add a generic way to map node to map for group based drivers
  pinctrl: berlin: use the generic node to map function

 .../bindings/pinctrl/pinctrl-bindings.txt          |  1 +
 drivers/pinctrl/berlin/Kconfig                     |  1 +
 drivers/pinctrl/berlin/berlin.c                    | 53 +---------------------
 drivers/pinctrl/pinconf-generic.c                  | 36 +++++++++++++++
 drivers/pinctrl/pinctrl-utils.c                    | 26 +++++++++++
 drivers/pinctrl/pinctrl-utils.h                    |  9 ++++
 include/linux/pinctrl/pinconf-generic.h            |  3 ++
 7 files changed, 78 insertions(+), 51 deletions(-)

-- 
1.9.1

[PATCH 1/4] Documentation: bindings: pinctrl: document the generic groups property

[Antoine Tenart]

Group-based drivers use a groups property to define on which groups a
mux function is applied to. Document it.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index fa40a177164c..7a1adc08b366 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -141,6 +141,7 @@ Supported generic properties are:
 pins			- the list of pins that properties in the node
 			  apply to
 function		- the mux function to select
+groups			- a list of groups a mux function is applied to
 bias-disable		- disable any pin bias
 bias-high-impedance	- high impedance mode ("third-state", "floating")
 bias-bus-hold		- latch weakly
-- 
1.9.1

[PATCH 2/4] pinctrl: add helpers for group based drivers

[Antoine Tenart]

Since the group based drivers have their dt properties documented in the
generic pinctrl documentation, add generic helpers to avoid duplicating
code and to be sure new drivers won't use specific bindings for a known
purpose.

This patch add two functions to help group based drivers map their
nodes:
- of_pinctrl_utils_read_function(): reads the function name of a
  specified node, and gets the number of groups it should be
  applied to.
- of_pinctrl_for_each_function_group(): navigates through the groups of
  a specified node, reading at each iteration the name of the current
  group.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/pinctrl/pinctrl-utils.c | 26 ++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-utils.h |  9 +++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-utils.c b/drivers/pinctrl/pinctrl-utils.c
index d77693f2cc1b..0ce44ff70197 100644
--- a/drivers/pinctrl/pinctrl-utils.c
+++ b/drivers/pinctrl/pinctrl-utils.c
@@ -140,3 +140,29 @@ void pinctrl_utils_dt_free_map(struct pinctrl_dev *pctldev,
 	kfree(map);
 }
 EXPORT_SYMBOL_GPL(pinctrl_utils_dt_free_map);
+
+#ifdef CONFIG_OF
+int of_pinctrl_utils_read_function(struct pinctrl_dev *pctldev,
+		struct device_node *node, const char **function_name,
+		int *ngroups)
+{
+	int ret;
+
+	ret = of_property_read_string(node, "function", function_name);
+	if (ret) {
+		dev_err(pctldev->dev, "missing function property in node %s\n",
+			node->name);
+		return -EINVAL;
+	}
+
+	*ngroups = of_property_count_strings(node, "groups");
+	if (ngroups <= 0) {
+		dev_err(pctldev->dev, "missing groups property in node %s\n",
+			node->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_pinctrl_utils_read_function);
+#endif
diff --git a/drivers/pinctrl/pinctrl-utils.h b/drivers/pinctrl/pinctrl-utils.h
index d0ffe1ce200f..d768dfe5daee 100644
--- a/drivers/pinctrl/pinctrl-utils.h
+++ b/drivers/pinctrl/pinctrl-utils.h
@@ -40,4 +40,13 @@ int pinctrl_utils_add_config(struct pinctrl_dev *pctldev,
 void pinctrl_utils_dt_free_map(struct pinctrl_dev *pctldev,
 		struct pinctrl_map *map, unsigned num_maps);
 
+#ifdef CONFIG_OF
+int of_pinctrl_utils_read_function(struct pinctrl_dev *pctrldev,
+		struct device_node *node, const char **function_name,
+		int *ngroups);
+
+#define of_pinctrl_for_each_function_group(node, prop, group_name)	\
+		of_property_for_each_string(node, "groups", prop, group_name)
+#endif
+
 #endif /* __PINCTRL_UTILS_H__ */
-- 
1.9.1

[PATCH 3/4] pinctrl: add a generic way to map node to map for group based drivers

[Antoine Tenart]

This patch add a generic function to use a standard callback to
.dt_node_to_map for group based pinctrl drivers.

It parses nodes of the form:

	foo_pmux: foo-pmux {
		function = "foo";
		groups = "g0", "g1", "g2";
	}

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/pinctrl/pinconf-generic.c       | 36 +++++++++++++++++++++++++++++++++
 include/linux/pinctrl/pinconf-generic.h |  3 +++
 2 files changed, 39 insertions(+)

diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index 29ff77f90fcb..456902150226 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -336,4 +336,40 @@ int pinconf_generic_dt_node_to_map(struct pinctrl_dev *pctldev,
 }
 EXPORT_SYMBOL_GPL(pinconf_generic_dt_node_to_map);
 
+int pinconf_generic_function_groups_dt_node_to_map(struct pinctrl_dev *pctldev,
+		struct device_node *node, struct pinctrl_map **map,
+		unsigned *num_maps)
+{
+	struct property *prop;
+	unsigned reserved_maps = 0;
+	const char *function_name, *group_name;
+	int ngroups, ret;
+
+	*map = NULL;
+	*num_maps = 0;
+
+	ret = of_pinctrl_utils_read_function(pctldev, node, &function_name,
+					     &ngroups);
+	if (ret)
+		return ret;
+
+	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps, num_maps,
+					ngroups);
+	if (ret)
+		return ret;
+
+	of_pinctrl_for_each_function_group(node, prop, group_name) {
+		ret = pinctrl_utils_add_map_mux(pctldev, map, &reserved_maps,
+						num_maps, group_name,
+						function_name);
+		if (ret) {
+			dev_err(pctldev->dev, "cannot add map: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinconf_generic_function_groups_dt_node_to_map);
+
 #endif
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index a15f10727eb8..acda4b89596d 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -157,6 +157,9 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 int pinconf_generic_dt_node_to_map(struct pinctrl_dev *pctldev,
 		struct device_node *np_config, struct pinctrl_map **map,
 		unsigned *num_maps, enum pinctrl_map_type type);
+int pinconf_generic_function_groups_dt_node_to_map(struct pinctrl_dev *pctldev,
+		struct device_node *node, struct pinctrl_map **map,
+		unsigned *num_maps);
 
 static inline int pinconf_generic_dt_node_to_map_group(
 		struct pinctrl_dev *pctldev, struct device_node *np_config,
-- 
1.9.1

[PATCH 4/4] pinctrl: berlin: use the generic node to map function

[Antoine Tenart]

A generic node to map function has been added into the pinctrl
framework. It is provieded by GENERIC_PINCONF. Use it in the Berlin
pinctrl driver as it fits the needs.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/pinctrl/berlin/Kconfig  |  1 +
 drivers/pinctrl/berlin/berlin.c | 53 ++---------------------------------------
 2 files changed, 3 insertions(+), 51 deletions(-)

diff --git a/drivers/pinctrl/berlin/Kconfig b/drivers/pinctrl/berlin/Kconfig
index b18322bc7bf9..b38c0abf1790 100644
--- a/drivers/pinctrl/berlin/Kconfig
+++ b/drivers/pinctrl/berlin/Kconfig
@@ -2,6 +2,7 @@ if ARCH_BERLIN
 
 config PINCTRL_BERLIN
 	bool
+	select GENERIC_PINCONF
 	select PINMUX
 	select REGMAP_MMIO
 
diff --git a/drivers/pinctrl/berlin/berlin.c b/drivers/pinctrl/berlin/berlin.c
index 86db2235ab00..da98efae8d8f 100644
--- a/drivers/pinctrl/berlin/berlin.c
+++ b/drivers/pinctrl/berlin/berlin.c
@@ -15,6 +15,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 #include <linux/platform_device.h>
@@ -49,56 +50,6 @@ static const char *berlin_pinctrl_get_group_name(struct pinctrl_dev *pctrl_dev,
 	return pctrl->desc->groups[group].name;
 }
 
-static int berlin_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev,
-					 struct device_node *node,
-					 struct pinctrl_map **map,
-					 unsigned *num_maps)
-{
-	struct berlin_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
-	struct property *prop;
-	const char *function_name, *group_name;
-	unsigned reserved_maps = 0;
-	int ret, ngroups;
-
-	*map = NULL;
-	*num_maps = 0;
-
-	ret = of_property_read_string(node, "function", &function_name);
-	if (ret) {
-		dev_err(pctrl->dev,
-			"missing function property in node %s\n",
-			node->name);
-		return -EINVAL;
-	}
-
-	ngroups = of_property_count_strings(node, "groups");
-	if (ngroups < 0) {
-		dev_err(pctrl->dev,
-			"missing groups property in node %s\n",
-			node->name);
-		return -EINVAL;
-	}
-
-	ret = pinctrl_utils_reserve_map(pctrl_dev, map, &reserved_maps,
-					num_maps, ngroups);
-	if (ret) {
-		dev_err(pctrl->dev, "can't reserve map: %d\n", ret);
-		return ret;
-	}
-
-	of_property_for_each_string(node, "groups", prop, group_name) {
-		ret = pinctrl_utils_add_map_mux(pctrl_dev, map, &reserved_maps,
-						num_maps, group_name,
-						function_name);
-		if (ret) {
-			dev_err(pctrl->dev, "can't add map: %d\n", ret);
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
 static void berlin_pinctrl_dt_free_map(struct pinctrl_dev *pctrl_dev,
 				       struct pinctrl_map *map,
 				       unsigned nmaps)
@@ -121,7 +72,7 @@ static void berlin_pinctrl_dt_free_map(struct pinctrl_dev *pctrl_dev,
 static const struct pinctrl_ops berlin_pinctrl_ops = {
 	.get_groups_count	= &berlin_pinctrl_get_group_count,
 	.get_group_name		= &berlin_pinctrl_get_group_name,
-	.dt_node_to_map		= &berlin_pinctrl_dt_node_to_map,
+	.dt_node_to_map		= &pinconf_generic_function_groups_dt_node_to_map,
 	.dt_free_map		= &berlin_pinctrl_dt_free_map,
 };
 
-- 
1.9.1

[PATCH 4/4] pinctrl: berlin: use the generic node to map function

[Antoine Tenart]

A generic node to map function has been added into the pinctrl
framework. It is provieded by GENERIC_PINCONF. Use it in the Berlin
pinctrl driver as it fits the needs.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/pinctrl/berlin/Kconfig  |  1 +
 drivers/pinctrl/berlin/berlin.c | 53 ++---------------------------------------
 2 files changed, 3 insertions(+), 51 deletions(-)

diff --git a/drivers/pinctrl/berlin/Kconfig b/drivers/pinctrl/berlin/Kconfig
index b18322bc7bf9..b38c0abf1790 100644
--- a/drivers/pinctrl/berlin/Kconfig
+++ b/drivers/pinctrl/berlin/Kconfig
@@ -2,6 +2,7 @@ if ARCH_BERLIN
 
 config PINCTRL_BERLIN
 	bool
+	select GENERIC_PINCONF
 	select PINMUX
 	select REGMAP_MMIO
 
diff --git a/drivers/pinctrl/berlin/berlin.c b/drivers/pinctrl/berlin/berlin.c
index 86db2235ab00..da98efae8d8f 100644
--- a/drivers/pinctrl/berlin/berlin.c
+++ b/drivers/pinctrl/berlin/berlin.c
@@ -15,6 +15,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 #include <linux/platform_device.h>
@@ -49,56 +50,6 @@ static const char *berlin_pinctrl_get_group_name(struct pinctrl_dev *pctrl_dev,
 	return pctrl->desc->groups[group].name;
 }
 
-static int berlin_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev,
-					 struct device_node *node,
-					 struct pinctrl_map **map,
-					 unsigned *num_maps)
-{
-	struct berlin_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
-	struct property *prop;
-	const char *function_name, *group_name;
-	unsigned reserved_maps = 0;
-	int ret, ngroups;
-
-	*map = NULL;
-	*num_maps = 0;
-
-	ret = of_property_read_string(node, "function", &function_name);
-	if (ret) {
-		dev_err(pctrl->dev,
-			"missing function property in node %s\n",
-			node->name);
-		return -EINVAL;
-	}
-
-	ngroups = of_property_count_strings(node, "groups");
-	if (ngroups < 0) {
-		dev_err(pctrl->dev,
-			"missing groups property in node %s\n",
-			node->name);
-		return -EINVAL;
-	}
-
-	ret = pinctrl_utils_reserve_map(pctrl_dev, map, &reserved_maps,
-					num_maps, ngroups);
-	if (ret) {
-		dev_err(pctrl->dev, "can't reserve map: %d\n", ret);
-		return ret;
-	}
-
-	of_property_for_each_string(node, "groups", prop, group_name) {
-		ret = pinctrl_utils_add_map_mux(pctrl_dev, map, &reserved_maps,
-						num_maps, group_name,
-						function_name);
-		if (ret) {
-			dev_err(pctrl->dev, "can't add map: %d\n", ret);
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
 static void berlin_pinctrl_dt_free_map(struct pinctrl_dev *pctrl_dev,
 				       struct pinctrl_map *map,
 				       unsigned nmaps)
@@ -121,7 +72,7 @@ static void berlin_pinctrl_dt_free_map(struct pinctrl_dev *pctrl_dev,
 static const struct pinctrl_ops berlin_pinctrl_ops = {
 	.get_groups_count	= &berlin_pinctrl_get_group_count,
 	.get_group_name		= &berlin_pinctrl_get_group_name,
-	.dt_node_to_map		= &berlin_pinctrl_dt_node_to_map,
+	.dt_node_to_map		= &pinconf_generic_function_groups_dt_node_to_map,
 	.dt_free_map		= &berlin_pinctrl_dt_free_map,
 };
 
-- 
1.9.1

Re: [PATCH 0/4] pinctrl: add helpers for group based drivers

[Sebastian Hesselbarth]

On 20.10.2014 10:04, Antoine Tenart wrote:
> Linus, Sebastian,
>
> As discussed earlier this year[1], this series introduce helpers for group based
> pinctrl drivers:
> - of_pinctrl_utils_read_function(): reads the function name of a
>    specified node, and gets the number of groups it should be
>    applied to.
> - of_pinctrl_for_each_function_group(): navigates through the groups of
>    a specified node, reading at each iteration the name of the current
>    group.
>
> A generic function to parse nodes for group based drivers is also added, and
> then used in the Berlin pinctrl driver:
> - pinconf_generic_function_groups_dt_node_to_map()
>
> [1] https://lkml.org/lkml/2014/5/17/38

Antoine,

thanks for looking into this!

 From Berlin POV,

Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

> Antoine Tenart (4):
>    Documentation: bindings: pinctrl: document the generic groups property
>    pinctrl: add helpers for group based drivers
>    pinctrl: add a generic way to map node to map for group based drivers
>    pinctrl: berlin: use the generic node to map function
>
>   .../bindings/pinctrl/pinctrl-bindings.txt          |  1 +
>   drivers/pinctrl/berlin/Kconfig                     |  1 +
>   drivers/pinctrl/berlin/berlin.c                    | 53 +---------------------
>   drivers/pinctrl/pinconf-generic.c                  | 36 +++++++++++++++
>   drivers/pinctrl/pinctrl-utils.c                    | 26 +++++++++++
>   drivers/pinctrl/pinctrl-utils.h                    |  9 ++++
>   include/linux/pinctrl/pinconf-generic.h            |  3 ++
>   7 files changed, 78 insertions(+), 51 deletions(-)
>

Re: [PATCH 1/4] Documentation: bindings: pinctrl: document the generic groups property

[Linus Walleij]

On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> Group-based drivers use a groups property to define on which groups a
> mux function is applied to. Document it.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

This is already documented but under another heading.

This section was to be for pin config, we now have a separate
section for pin muxing.

Yours,
Linus Walleij

Re: [PATCH 1/4] Documentation: bindings: pinctrl: document the generic groups property

[Linus Walleij]

On Tue, Oct 28, 2014 at 4:29 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
> <antoine.tenart@free-electrons.com> wrote:
>
>> Group-based drivers use a groups property to define on which groups a
>> mux function is applied to. Document it.
>>
>> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
>
> This is already documented but under another heading.
>
> This section was to be for pin config, we now have a separate
> section for pin muxing.

Bah rather, look in the upstream kernel:

== Generic pin multiplexing node content ==

pin multiplexing nodes:

function                - the mux function to select
groups                  - the list of groups to select with this function

Example:

state_0_node_a {
        function = "uart0";
        groups = "u0rxtx", "u0rtscts";
};
state_1_node_a {
        function = "spi0";
        groups = "spi0pins";
};

== Generic pin configuration node content ==

(...)

Supported generic properties are:

pins                    - the list of pins that properties in the node
                          apply to (either this or "group" has to be
                          specified)
group                   - the group to apply the properties to, if the driver
                          supports configuration of whole groups rather than
                          individual pins (either this or "pins" has to be
                          specified)


So now the first one is explicitly for mux and the latter on explicitly for
configs.

Maybe we have to have "pins" for the mux too, for drivers that do
per-pin function assignment, but it's clear that configs are per-pin
or per-group atleast, while muxing is done in nodes that contain
a "function" element.

Yours,
Linus Walleij

Re: [PATCH 2/4] pinctrl: add helpers for group based drivers

[Linus Walleij]

On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> Since the group based drivers have their dt properties documented in the
> generic pinctrl documentation, add generic helpers to avoid duplicating
> code and to be sure new drivers won't use specific bindings for a known
> purpose.
>
> This patch add two functions to help group based drivers map their
> nodes:
> - of_pinctrl_utils_read_function(): reads the function name of a
>   specified node, and gets the number of groups it should be
>   applied to.
> - of_pinctrl_for_each_function_group(): navigates through the groups of
>   a specified node, reading at each iteration the name of the current
>   group.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

GOOD IDEA!

Let's work from this and change a few drivers.

Some comments:

> +#ifdef CONFIG_OF
> +int of_pinctrl_utils_read_function(struct pinctrl_dev *pctldev,
> +               struct device_node *node, const char **function_name,
> +               int *ngroups)
> +{
> +       int ret;
> +
> +       ret = of_property_read_string(node, "function", function_name);
> +       if (ret) {
> +               dev_err(pctldev->dev, "missing function property in node %s\n",
> +                       node->name);
> +               return -EINVAL;
> +       }
> +
> +       *ngroups = of_property_count_strings(node, "groups");
> +       if (ngroups <= 0) {
> +               dev_err(pctldev->dev, "missing groups property in node %s\n",
> +                       node->name);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_pinctrl_utils_read_function);
> +#endif

Isn't it nasty to print dev_err() here if you maybe just want
to check if this node has a "function" element and proceed to
see if it's a pin config group in case it hasn't?

Or should we add of_pinctrl_utils_node_is_mux() and
of_pinctrl_utils_node_is_config() to check this?

> +#define of_pinctrl_for_each_function_group(node, prop, group_name)     \
> +               of_property_for_each_string(node, "groups", prop, group_name)

I *really* like this macro.

Yours,
Linus Walleij

Re: [PATCH 3/4] pinctrl: add a generic way to map node to map for group based drivers

[Linus Walleij]

On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> This patch add a generic function to use a standard callback to
> .dt_node_to_map for group based pinctrl drivers.
>
> It parses nodes of the form:
>
>         foo_pmux: foo-pmux {
>                 function = "foo";
>                 groups = "g0", "g1", "g2";
>         }
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Really nice! :)

> +int pinconf_generic_function_groups_dt_node_to_map(struct pinctrl_dev *pctldev,
> +               struct device_node *node, struct pinctrl_map **map,
> +               unsigned *num_maps)
> +{
> +       struct property *prop;
> +       unsigned reserved_maps = 0;
> +       const char *function_name, *group_name;
> +       int ngroups, ret;
> +
> +       *map = NULL;
> +       *num_maps = 0;
> +
> +       ret = of_pinctrl_utils_read_function(pctldev, node, &function_name,
> +                                            &ngroups);
> +       if (ret)
> +               return ret;

So maybe you want this not to print errors as I said in the first patch.

Or add a function of_pinctrl_utils_is_mux()?

> +       ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps, num_maps,
> +                                       ngroups);
> +       if (ret)
> +               return ret;
> +
> +       of_pinctrl_for_each_function_group(node, prop, group_name) {
> +               ret = pinctrl_utils_add_map_mux(pctldev, map, &reserved_maps,
> +                                               num_maps, group_name,
> +                                               function_name);
> +               if (ret) {
> +                       dev_err(pctldev->dev, "cannot add map: %d\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(pinconf_generic_function_groups_dt_node_to_map);

The rest looks really good.

Yours,
Linus Walleij

[PATCH 4/4] pinctrl: berlin: use the generic node to map function

[Linus Walleij]

On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> A generic node to map function has been added into the pinctrl
> framework. It is provieded by GENERIC_PINCONF. Use it in the Berlin
> pinctrl driver as it fits the needs.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Really nice. Will just apply this with the other stuff in the revised version.

Yours,
Linus Walleij

Re: [PATCH 4/4] pinctrl: berlin: use the generic node to map function

[Linus Walleij]

On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> A generic node to map function has been added into the pinctrl
> framework. It is provieded by GENERIC_PINCONF. Use it in the Berlin
> pinctrl driver as it fits the needs.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Really nice. Will just apply this with the other stuff in the revised version.

Yours,
Linus Walleij

Re: [PATCH 2/4] pinctrl: add helpers for group based drivers

[Antoine Tenart]

Hi Linus,

On Tue, Oct 28, 2014 at 04:38:27PM +0100, Linus Walleij wrote:
> On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
> <antoine.tenart@free-electrons.com> wrote:
> 
> > +#ifdef CONFIG_OF
> > +int of_pinctrl_utils_read_function(struct pinctrl_dev *pctldev,
> > +               struct device_node *node, const char **function_name,
> > +               int *ngroups)
> > +{
> > +       int ret;
> > +
> > +       ret = of_property_read_string(node, "function", function_name);
> > +       if (ret) {
> > +               dev_err(pctldev->dev, "missing function property in node %s\n",
> > +                       node->name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       *ngroups = of_property_count_strings(node, "groups");
> > +       if (ngroups <= 0) {
> > +               dev_err(pctldev->dev, "missing groups property in node %s\n",
> > +                       node->name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_pinctrl_utils_read_function);
> > +#endif
> 
> Isn't it nasty to print dev_err() here if you maybe just want
> to check if this node has a "function" element and proceed to
> see if it's a pin config group in case it hasn't?
> 
> Or should we add of_pinctrl_utils_node_is_mux() and
> of_pinctrl_utils_node_is_config() to check this?

We could have an additional pin_name parameter and return an error if both
function_name *and* pin_name are NULL. That could also allow to have nodes with
both a function and a pin, if that make any sense.

Maybe the of_pinctrl_utils_node_is_mux/config() solution is more
appropriate to avoid having a confusing function. Plus we would have a
dedicated of_pinctrl_utils_read_pins() function.

I think I prefer the second solution because it could avoid some
confusion and the logic would stay in the pinctrl core functions. 

I'll cook up a new version if the of_pinctrl_utils_node_is_mux/config()
is okay with you.

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 2/4] pinctrl: add helpers for group based drivers

[Linus Walleij]

On Thu, Oct 30, 2014 at 8:48 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> Maybe the of_pinctrl_utils_node_is_mux/config() solution is more
> appropriate to avoid having a confusing function. Plus we would have a
> dedicated of_pinctrl_utils_read_pins() function.
>
> I think I prefer the second solution because it could avoid some
> confusion and the logic would stay in the pinctrl core functions.
>
> I'll cook up a new version if the of_pinctrl_utils_node_is_mux/config()
> is okay with you.

I'm following your idea and I agree. Let's see how that
looks.

Yours,
Linus Walleij