Message-ID: <20260203-craftsman-battered-3491ff68f462@spud>
Date: Tue,  3 Feb 2026 16:17:07 +0000
From: Conor Dooley <conor@...nel.org>
To: linusw@...nel.org
Cc: conor@...nel.org,
	Conor Dooley <conor.dooley@...rochip.com>,
	Xianwei Zhao <xianwei.zhao@...ogic.com>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Kevin Hilman <khilman@...libre.com>,
	Jerome Brunet <jbrunet@...libre.com>,
	Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
	linux-amlogic@...ts.infradead.org,
	linux-gpio@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: [RFC] pinctrl: pinconf-generic: move ..dt_node_to_map_pinmux() to amlogic-am4 driver

From: Conor Dooley <conor.dooley@...rochip.com>

pinconf_generic_dt_node_to_map_pinmux() is not actually a generic
function, and really belongs in the amlogic-am4 driver. There are three
reasons why.

First, and least, of the reasons is that this function behaves
differently to the other dt_node_to_map functions in a way that is not
obvious from a first glance. This difference stems for the devicetree
properties that the function is intended for use with, and how they are
typically used. The other generic dt_node_to_map functions support
platforms where the pins, groups and functions are described statically
in the driver and require a function that will produce a mapping from dt
nodes to these pre-established descriptions. No other code in the driver
is require to be executed at runtime.
pinconf_generic_dt_node_to_map_pinmux() on the other hand is intended for
use with the pinmux property, where groups and functions are determined
entirely from the devicetree. As a result, there are no statically
defined groups and functions in the driver for this function to perform
a mapping to. Other drivers that use the pinmux property (e.g. the k1)
their dt_node_to_map function creates the groups and functions as the
devicetree is parsed. Instead of that,
pinconf_generic_dt_node_to_map_pinmux() requires that the devicetree is
parsed twice, once by it and once at probe, so that the driver
dynamically creates the groups and functions before the dt_node_to_map
callback is executed. I don't believe this double parsing requirement is
how developers would expect this to work and is not necessary given
there are drivers that do not have this behaviour.

Secondly and thirdly, the function bakes in some assumptions that only
really match the amlogic platform about how the devicetree is constructed.
These, to me, are problematic for something that claims to be generic.

The other dt_node_to_map implementations accept a being called for
either a node containing pin configuration properties or a node
containing child nodes that each contain the configuration properties.
IOW, they support the following two devicetree configurations:

| cfg {
| 	label: group {
| 		pinmux = <asjhdasjhlajskd>;
| 		config-item1;
| 	};
| };

| label: cfg {
| 	group1 {
| 		pinmux = <dsjhlfka>;
| 		config-item2;
| 	};
| 	group2 {
| 		pinmux = <lsdjhaf>;
| 		config-item1;
| 	};
| };

pinconf_generic_dt_node_to_map_pinmux() only supports the latter.

The other assumption about devicetree configuration that the function
makes is that the labeled node's parent is a "function node". The amlogic
driver uses these "function nodes" to create the functions at probe
time, and pinconf_generic_dt_node_to_map_pinmux() finds the parent of
the node it is operating on's name as part of the mapping. IOW, it
requires that the devicetree look like:

| pinctrl@bla {
|
| 	func-foo {
| 		label: group-default {
| 			pinmuxes = <lskdf>;
| 		};
| 	};
| };

and couldn't be used if the nodes containing the pinmux and
configuration properties are children of the pinctrl node itself:

| pinctrl@bla {
|
| 	label: group-default {
| 		pinmuxes = <lskdf>;
| 	};
| };

These final two reasons are mainly why I believe this is not suitable as
a generic function, and should be moved into the driver that is the sole
user and originator of the "generic" function.

Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
---
CC: Xianwei Zhao <xianwei.zhao@...ogic.com>
CC: Linus Walleij <linusw@...nel.org>
CC: Neil Armstrong <neil.armstrong@...aro.org>
CC: Kevin Hilman <khilman@...libre.com>
CC: Jerome Brunet <jbrunet@...libre.com>
CC: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
CC: linux-amlogic@...ts.infradead.org
CC: linux-gpio@...r.kernel.org
CC: linux-arm-kernel@...ts.infradead.org
CC: linux-kernel@...r.kernel.org
 drivers/pinctrl/meson/pinctrl-amlogic-a4.c | 71 +++++++++++++++++++++-
 drivers/pinctrl/pinconf-generic.c          | 69 ---------------------
 include/linux/pinctrl/pinconf-generic.h    |  5 --
 3 files changed, 70 insertions(+), 75 deletions(-)

diff --git a/drivers/pinctrl/meson/pinctrl-amlogic-a4.c b/drivers/pinctrl/meson/pinctrl-amlogic-a4.c
index d9e3a8d5932a..67c96e4661f5 100644
--- a/drivers/pinctrl/meson/pinctrl-amlogic-a4.c
+++ b/drivers/pinctrl/meson/pinctrl-amlogic-a4.c
@@ -24,6 +24,7 @@
 #include <dt-bindings/pinctrl/amlogic,pinctrl.h>
 
 #include "../core.h"
+#include "../pinctrl-utils.h"
 #include "../pinconf.h"
 
 #define gpio_chip_to_bank(chip) \
@@ -672,11 +673,79 @@ static void aml_pin_dbg_show(struct pinctrl_dev *pcdev, struct seq_file *s,
 	seq_printf(s, " %s", dev_name(pcdev->dev));
 }
 
+static int aml_dt_node_to_map_pinmux(struct pinctrl_dev *pctldev,
+				     struct device_node *np,
+				     struct pinctrl_map **map,
+				     unsigned int *num_maps)
+{
+	struct device *dev = pctldev->dev;
+	struct device_node *pnode;
+	unsigned long *configs = NULL;
+	unsigned int num_configs = 0;
+	struct property *prop;
+	unsigned int reserved_maps;
+	int reserve;
+	int ret;
+
+	prop = of_find_property(np, "pinmux", NULL);
+	if (!prop) {
+		dev_info(dev, "Missing pinmux property\n");
+		return -ENOENT;
+	}
+
+	pnode = of_get_parent(np);
+	if (!pnode) {
+		dev_info(dev, "Missing function node\n");
+		return -EINVAL;
+	}
+
+	reserved_maps = 0;
+	*map = NULL;
+	*num_maps = 0;
+
+	ret = pinconf_generic_parse_dt_config(np, pctldev, &configs,
+					      &num_configs);
+	if (ret < 0) {
+		dev_err(dev, "%pOF: could not parse node property\n", np);
+		return ret;
+	}
+
+	reserve = 1;
+	if (num_configs)
+		reserve++;
+
+	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps,
+					num_maps, reserve);
+	if (ret < 0)
+		goto exit;
+
+	ret = pinctrl_utils_add_map_mux(pctldev, map,
+					&reserved_maps, num_maps, np->name,
+					pnode->name);
+	if (ret < 0)
+		goto exit;
+
+	if (num_configs) {
+		ret = pinctrl_utils_add_map_configs(pctldev, map, &reserved_maps,
+						    num_maps, np->name, configs,
+						    num_configs, PIN_MAP_TYPE_CONFIGS_GROUP);
+		if (ret < 0)
+			goto exit;
+	}
+
+exit:
+	kfree(configs);
+	if (ret)
+		pinctrl_utils_free_map(pctldev, *map, *num_maps);
+
+	return ret;
+}
+
 static const struct pinctrl_ops aml_pctrl_ops = {
 	.get_groups_count	= aml_get_groups_count,
 	.get_group_name		= aml_get_group_name,
 	.get_group_pins		= aml_get_group_pins,
-	.dt_node_to_map		= pinconf_generic_dt_node_to_map_pinmux,
+	.dt_node_to_map		= aml_dt_node_to_map_pinmux,
 	.dt_free_map		= pinconf_generic_dt_free_map,
 	.pin_dbg_show		= aml_pin_dbg_show,
 };
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index d182ec84e2df..30475da0fd10 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -424,75 +424,6 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 }
 EXPORT_SYMBOL_GPL(pinconf_generic_parse_dt_config);
 
-int pinconf_generic_dt_node_to_map_pinmux(struct pinctrl_dev *pctldev,
-					  struct device_node *np,
-					  struct pinctrl_map **map,
-					  unsigned int *num_maps)
-{
-	struct device *dev = pctldev->dev;
-	struct device_node *pnode;
-	unsigned long *configs = NULL;
-	unsigned int num_configs = 0;
-	struct property *prop;
-	unsigned int reserved_maps;
-	int reserve;
-	int ret;
-
-	prop = of_find_property(np, "pinmux", NULL);
-	if (!prop) {
-		dev_info(dev, "Missing pinmux property\n");
-		return -ENOENT;
-	}
-
-	pnode = of_get_parent(np);
-	if (!pnode) {
-		dev_info(dev, "Missing function node\n");
-		return -EINVAL;
-	}
-
-	reserved_maps = 0;
-	*map = NULL;
-	*num_maps = 0;
-
-	ret = pinconf_generic_parse_dt_config(np, pctldev, &configs,
-					      &num_configs);
-	if (ret < 0) {
-		dev_err(dev, "%pOF: could not parse node property\n", np);
-		return ret;
-	}
-
-	reserve = 1;
-	if (num_configs)
-		reserve++;
-
-	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps,
-					num_maps, reserve);
-	if (ret < 0)
-		goto exit;
-
-	ret = pinctrl_utils_add_map_mux(pctldev, map,
-					&reserved_maps, num_maps, np->name,
-					pnode->name);
-	if (ret < 0)
-		goto exit;
-
-	if (num_configs) {
-		ret = pinctrl_utils_add_map_configs(pctldev, map, &reserved_maps,
-						    num_maps, np->name, configs,
-						    num_configs, PIN_MAP_TYPE_CONFIGS_GROUP);
-		if (ret < 0)
-			goto exit;
-	}
-
-exit:
-	kfree(configs);
-	if (ret)
-		pinctrl_utils_free_map(pctldev, *map, *num_maps);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(pinconf_generic_dt_node_to_map_pinmux);
-
 int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 		struct device_node *np, struct pinctrl_map **map,
 		unsigned int *reserved_maps, unsigned int *num_maps,
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 1be4032071c2..89277808ea61 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -250,9 +250,4 @@ static inline int pinconf_generic_dt_node_to_map_all(struct pinctrl_dev *pctldev
 	return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps,
 			PIN_MAP_TYPE_INVALID);
 }
-
-int pinconf_generic_dt_node_to_map_pinmux(struct pinctrl_dev *pctldev,
-					  struct device_node *np,
-					  struct pinctrl_map **map,
-					  unsigned int *num_maps);
 #endif /* __LINUX_PINCTRL_PINCONF_GENERIC_H */
-- 
2.51.0

Powered by blists - more mailing lists