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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> 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(-)
>
On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
<[email protected]> 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 <[email protected]>
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
On Tue, Oct 28, 2014 at 4:29 PM, Linus Walleij <[email protected]> wrote:
> On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
> <[email protected]> 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 <[email protected]>
>
> 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
On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
<[email protected]> 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 <[email protected]>
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
On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
<[email protected]> 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 <[email protected]>
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
On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
<[email protected]> 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 <[email protected]>
Really nice. Will just apply this with the other stuff in the revised version.
Yours,
Linus Walleij
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
> <[email protected]> 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
On Thu, Oct 30, 2014 at 8:48 PM, Antoine Tenart
<[email protected]> 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