[RFC v1 0/4] generic pinmux dt_node_to_map implementation

[RFC v1 0/4] generic pinmux dt_node_to_map implementation

[Conor Dooley]

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

Hey Linus,

Whipped this up last week, at to a first glance it appears to work,
although the spacemit platform I've used to implement this has very
limited in-tree use of pinctrl so it is hard to be sure.

What I don't love though is how similar the functions
pinctrl_generic_pins_function_dt_node_to_map() and
pinctrl_generic_pinmux_dt_node_to_map() are - essentially identical
other than which function they in turn call.

Basically, I wanna know if you think that that is acceptable, or if
you'd rather see something that's totally generic between the two
and figures out which foo_dt_subnode_to_map() function to call*,
or convert pinctrl_generic_pins_function_dt_node_to_map() and
pinctrl_generic_pinmux_dt_node_to_map() to wrappers that pass a function
pointer of the appropriate foo_dt_subnode_to_map() to a shared node
parsing function, or some third option that I have not considered.

If you try to apply this, it's on top of Frank's mux series. It's also
here if any of the spacemit-ters want to look at what I did to their
driver:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/log/?h=spacemit-pinctrl

None of the patches have real commit messages due to this being RFC :)

Cheers,
Conor.

* pinmux/pins + functions/groups + functions I think are the only
  really valid combinations here, so it wouldn't be too difficult to
  arrange that logic.

CC: Linus Walleij <linusw@kernel.org>
CC: Yixun Lan <dlan@kernel.org>
CC: Conor Dooley <conor.dooley@microchip.com>
CC: Troy Mitchell <troy.mitchell@linux.spacemit.com>
CC: linux-gpio@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-riscv@lists.infradead.org
CC: spacemit@lists.linux.dev

Conor Dooley (4):
  pinctrl: generic: change signature of pinctrl_generic_to_map() to pass
    void data
  pinctrl: add new generic groups/function creation function for pinmux
  pinctrl: spacemit: delete check_power()
  pinctrl: spacemit: move over to generic pinmux dt_node_to_map
    implementation

 drivers/pinctrl/pinconf.h             |  20 +++-
 drivers/pinctrl/pinctrl-generic.c     | 139 +++++++++++++++++++++++++-
 drivers/pinctrl/spacemit/pinctrl-k1.c |  70 +++----------
 3 files changed, 164 insertions(+), 65 deletions(-)

-- 
2.53.0

[RFC v1 1/4] pinctrl: generic: change signature of pinctrl_generic_to_map() to pass void data

[Conor Dooley]

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

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pinctrl/pinconf.h         | 6 ++----
 drivers/pinctrl/pinctrl-generic.c | 5 ++---
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
index 83f2d00c732e1..5d99fef27657f 100644
--- a/drivers/pinctrl/pinconf.h
+++ b/drivers/pinctrl/pinconf.h
@@ -171,8 +171,7 @@ int pinctrl_generic_to_map(struct pinctrl_dev *pctldev, struct device_node *pare
 			   struct device_node *np, struct pinctrl_map **maps,
 			   unsigned int *num_maps, unsigned int *num_reserved_maps,
 			   const char **group_name, unsigned int ngroups,
-			   const char **functions, unsigned int *pins,
-			   unsigned int npins);
+			   void *data, unsigned int *pins, unsigned int npins);
 #else
 static inline int
 pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
@@ -188,8 +187,7 @@ pinctrl_generic_to_map(struct pinctrl_dev *pctldev, struct device_node *parent,
 		       struct device_node *np, struct pinctrl_map **maps,
 		       unsigned int *num_maps, unsigned int *num_reserved_maps,
 		       const char **group_name, unsigned int ngroups,
-		       const char **functions, unsigned int *pins,
-		       unsigned int npins)
+		       void *data, unsigned int *pins, unsigned int npins)
 {
 	return -ENOTSUPP;
 }
diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c
index 76670aef62da4..6a13fd4eea65b 100644
--- a/drivers/pinctrl/pinctrl-generic.c
+++ b/drivers/pinctrl/pinctrl-generic.c
@@ -21,8 +21,7 @@ int pinctrl_generic_to_map(struct pinctrl_dev *pctldev, struct device_node *pare
 			   struct device_node *np, struct pinctrl_map **maps,
 			   unsigned int *num_maps, unsigned int *num_reserved_maps,
 			   const char **group_names, unsigned int ngroups,
-			   const char **functions, unsigned int *pins,
-			   unsigned int npins)
+			   void *data, unsigned int *pins, unsigned int npins)
 {
 	struct device *dev = pctldev->dev;
 	unsigned int num_configs;
@@ -45,7 +44,7 @@ int pinctrl_generic_to_map(struct pinctrl_dev *pctldev, struct device_node *pare
 	if (ret < 0)
 		return ret;
 
-	ret = pinctrl_generic_add_group(pctldev, group_name, pins, npins, functions);
+	ret = pinctrl_generic_add_group(pctldev, group_name, pins, npins, data);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "failed to add group %s: %d\n",
 				     group_name, ret);
-- 
2.53.0

[RFC v1 2/4] pinctrl: add new generic groups/function creation function for pinmux

[Conor Dooley]

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

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pinctrl/pinconf.h         |  14 ++++
 drivers/pinctrl/pinctrl-generic.c | 134 ++++++++++++++++++++++++++++++
 2 files changed, 148 insertions(+)

diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
index 5d99fef27657f..30c42d81e8829 100644
--- a/drivers/pinctrl/pinconf.h
+++ b/drivers/pinctrl/pinconf.h
@@ -167,6 +167,11 @@ int pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
 						 struct pinctrl_map **maps,
 						 unsigned int *num_maps);
 
+int pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
+					  struct device_node *np,
+					  struct pinctrl_map **maps,
+					  unsigned int *num_maps);
+
 int pinctrl_generic_to_map(struct pinctrl_dev *pctldev, struct device_node *parent,
 			   struct device_node *np, struct pinctrl_map **maps,
 			   unsigned int *num_maps, unsigned int *num_reserved_maps,
@@ -182,6 +187,15 @@ pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
 	return -ENOTSUPP;
 }
 
+static inline int
+pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
+				      struct device_node *np,
+				      struct pinctrl_map **maps,
+				      unsigned int *num_maps)
+{
+	return -ENOTSUPP;
+}
+
 static inline int
 pinctrl_generic_to_map(struct pinctrl_dev *pctldev, struct device_node *parent,
 		       struct device_node *np, struct pinctrl_map **maps,
diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c
index 6a13fd4eea65b..69df211f6b964 100644
--- a/drivers/pinctrl/pinctrl-generic.c
+++ b/drivers/pinctrl/pinctrl-generic.c
@@ -202,3 +202,137 @@ int pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pinctrl_generic_pins_function_dt_node_to_map);
+
+
+static int pinctrl_generic_pinmux_dt_subnode_to_map(struct pinctrl_dev *pctldev,
+						    struct device_node *parent,
+						    struct device_node *np,
+						    struct pinctrl_map **maps,
+						    unsigned int *num_maps,
+						    unsigned int *num_reserved_maps,
+						    const char **group_names,
+						    unsigned int ngroups)
+{
+	struct device *dev = pctldev->dev;
+	unsigned int *pins, *muxes;
+	int npins, ret;
+
+	npins = of_property_count_u32_elems(np, "pinmux");
+
+	if (npins < 1) {
+		dev_err(dev, "invalid pinctrl group %pOFn.%pOFn %d\n",
+			parent, np, npins);
+		return npins;
+	}
+
+	pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	muxes = devm_kcalloc(dev, npins, sizeof(*muxes), GFP_KERNEL);
+	if (!muxes)
+		return -ENOMEM;
+
+	for (int i = 0; i < npins; i++) {
+		unsigned int pinmux;
+
+		ret = of_property_read_u32_index(np, "pinmux", i, &pinmux);
+		if (ret)
+			return ret;
+
+		pins[i] = pinmux >> 16;
+		muxes[i] = pinmux & GENMASK(15, 0);
+	}
+
+	return pinctrl_generic_to_map(pctldev, parent, np, maps, num_maps,
+				      num_reserved_maps, group_names, ngroups,
+				      muxes, pins, npins);
+}
+
+/*
+ * For platforms that do not define groups or functions in the driver, but
+ * instead use the devicetree to describe them. This function will, unlike
+ * pinconf_generic_dt_node_to_map() etc which rely on driver defined groups
+ * and functions, create them in addition to parsing pinconf properties and
+ * adding mappings.
+ *
+ * It assumes that the upper 16 bits of the pinmux items contain the pin
+ * and the lower 16 the mux setting.
+ */
+int pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
+					  struct device_node *np,
+					  struct pinctrl_map **maps,
+					  unsigned int *num_maps)
+{
+	struct device *dev = pctldev->dev;
+	struct device_node *child_np;
+	const char **group_names;
+	unsigned int num_reserved_maps = 0;
+	int ngroups = 0;
+	int ret;
+
+	*maps = NULL;
+	*num_maps = 0;
+
+	/*
+	 * Check if this is actually the pinmux node, or a parent containing
+	 * multiple pinmux nodes.
+	 */
+	if (!of_property_present(np, "pinmux"))
+		goto parent;
+
+	group_names = devm_kcalloc(dev, 1, sizeof(*group_names), GFP_KERNEL);
+	if (!group_names)
+		return -ENOMEM;
+
+	ret = pinctrl_generic_pinmux_dt_subnode_to_map(pctldev, np, np,
+						       maps, num_maps,
+						       &num_reserved_maps,
+						       group_names,
+						       ngroups);
+	if (ret) {
+		pinctrl_utils_free_map(pctldev, *maps, *num_maps);
+		return dev_err_probe(dev, ret, "error figuring out mappings for %s\n", np->name);
+	}
+
+	ret = pinmux_generic_add_function(pctldev, np->name, group_names, 1, NULL);
+	if (ret < 0) {
+		pinctrl_utils_free_map(pctldev, *maps, *num_maps);
+		return dev_err_probe(dev, ret, "error adding function %s\n", np->name);
+	}
+
+	return 0;
+
+parent:
+	for_each_available_child_of_node(np, child_np)
+		ngroups += 1;
+
+	group_names = devm_kcalloc(dev, ngroups, sizeof(*group_names), GFP_KERNEL);
+	if (!group_names)
+		return -ENOMEM;
+
+	ngroups = 0;
+	for_each_available_child_of_node_scoped(np, child_np) {
+		ret = pinctrl_generic_pinmux_dt_subnode_to_map(pctldev, np, child_np,
+							       maps, num_maps,
+							       &num_reserved_maps,
+							       group_names,
+							       ngroups);
+		if (ret) {
+			pinctrl_utils_free_map(pctldev, *maps, *num_maps);
+			return dev_err_probe(dev, ret, "error figuring out mappings for %s\n",
+					     np->name);
+		}
+
+		ngroups++;
+	}
+
+	ret = pinmux_generic_add_function(pctldev, np->name, group_names, ngroups, NULL);
+	if (ret < 0) {
+		pinctrl_utils_free_map(pctldev, *maps, *num_maps);
+		return dev_err_probe(dev, ret, "error adding function %s\n", np->name);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_generic_pinmux_dt_node_to_map);
-- 
2.53.0

[RFC v1 3/4] pinctrl: spacemit: delete check_power()

[Conor Dooley]

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

AFAICT, this is pointless because generate_config performs this check
too.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pinctrl/spacemit/pinctrl-k1.c | 37 ---------------------------
 1 file changed, 37 deletions(-)

diff --git a/drivers/pinctrl/spacemit/pinctrl-k1.c b/drivers/pinctrl/spacemit/pinctrl-k1.c
index 62cab6f6cd0a8..41d8a34bc386b 100644
--- a/drivers/pinctrl/spacemit/pinctrl-k1.c
+++ b/drivers/pinctrl/spacemit/pinctrl-k1.c
@@ -409,38 +409,6 @@ static inline u32 spacemit_get_drive_strength_mA(enum spacemit_pin_io_type type,
 	}
 }
 
-static int spacemit_pctrl_check_power(struct pinctrl_dev *pctldev,
-				      struct device_node *dn,
-				      struct spacemit_pin_mux_config *pinmuxs,
-				      int num_pins, const char *grpname)
-{
-	struct spacemit_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
-	struct device *dev = pctrl->dev;
-	enum spacemit_pin_io_type type;
-	u32 power = 0, i;
-
-	of_property_read_u32(dn, "power-source", &power);
-
-	for (i = 0; i < num_pins; i++) {
-		type = spacemit_to_pin_io_type(pinmuxs[i].pin);
-
-		if (type != IO_TYPE_EXTERNAL)
-			continue;
-
-		switch (power) {
-		case PIN_POWER_STATE_1V8:
-		case PIN_POWER_STATE_3V3:
-			break;
-		default:
-			dev_err(dev, "group %s has unsupported power\n",
-				grpname);
-			return -ENOTSUPP;
-		}
-	}
-
-	return 0;
-}
-
 static void spacemit_set_io_pwr_domain(struct spacemit_pinctrl *pctrl,
 				      const struct spacemit_pin *spin,
 				      const enum spacemit_pin_io_type type)
@@ -548,11 +516,6 @@ static int spacemit_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
 				return dev_err_probe(dev, -ENODEV, "failed to get pin %d\n", pins[i]);
 		}
 
-		ret = spacemit_pctrl_check_power(pctldev, child, pinmuxs,
-						 npins, grpname);
-		if (ret < 0)
-			return ret;
-
 		map[nmaps].type = PIN_MAP_TYPE_MUX_GROUP;
 		map[nmaps].data.mux.function = np->name;
 		map[nmaps].data.mux.group = grpname;
-- 
2.53.0

[RFC v1 4/4] pinctrl: spacemit: move over to generic pinmux dt_node_to_map implementation

[Conor Dooley]

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

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pinctrl/spacemit/pinctrl-k1.c | 33 ++++++++++-----------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/pinctrl/spacemit/pinctrl-k1.c b/drivers/pinctrl/spacemit/pinctrl-k1.c
index 41d8a34bc386b..c90b6ccfce2ea 100644
--- a/drivers/pinctrl/spacemit/pinctrl-k1.c
+++ b/drivers/pinctrl/spacemit/pinctrl-k1.c
@@ -114,11 +114,6 @@ struct spacemit_pinctrl_data {
 	const struct spacemit_pinctrl_dconf	*dconf;
 };
 
-struct spacemit_pin_mux_config {
-	const struct spacemit_pin	*pin;
-	u32				config;
-};
-
 /* map pin id to pinctrl register offset, refer MFPR definition */
 static unsigned int spacemit_k1_pin_to_offset(unsigned int pin)
 {
@@ -474,7 +469,6 @@ static int spacemit_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
 	ngroups = 0;
 	guard(mutex)(&pctrl->mutex);
 	for_each_available_child_of_node_scoped(np, child) {
-		struct spacemit_pin_mux_config *pinmuxs;
 		unsigned int config, *pins;
 		int i, npins;
 
@@ -497,10 +491,6 @@ static int spacemit_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		if (!pins)
 			return -ENOMEM;
 
-		pinmuxs = devm_kcalloc(dev, npins, sizeof(*pinmuxs), GFP_KERNEL);
-		if (!pinmuxs)
-			return -ENOMEM;
-
 		for (i = 0; i < npins; i++) {
 			ret = of_property_read_u32_index(child, "pinmux",
 							 i, &config);
@@ -509,11 +499,6 @@ static int spacemit_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
 				return -EINVAL;
 
 			pins[i] = spacemit_dt_get_pin(config);
-			pinmuxs[i].config = config;
-			pinmuxs[i].pin = spacemit_get_pin(pctrl, pins[i]);
-
-			if (!pinmuxs[i].pin)
-				return dev_err_probe(dev, -ENODEV, "failed to get pin %d\n", pins[i]);
 		}
 
 		map[nmaps].type = PIN_MAP_TYPE_MUX_GROUP;
@@ -522,7 +507,7 @@ static int spacemit_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
 		nmaps += 1;
 
 		ret = pinctrl_generic_add_group(pctldev, grpname,
-						pins, npins, pinmuxs);
+						pins, npins, &config);
 		if (ret < 0)
 			return dev_err_probe(dev, ret, "failed to add group %s: %d\n", grpname, ret);
 
@@ -559,7 +544,7 @@ static const struct pinctrl_ops spacemit_pctrl_ops = {
 	.get_group_name		= pinctrl_generic_get_group_name,
 	.get_group_pins		= pinctrl_generic_get_group_pins,
 	.pin_dbg_show		= spacemit_pctrl_dbg_show,
-	.dt_node_to_map		= spacemit_pctrl_dt_node_to_map,
+	.dt_node_to_map		= pinctrl_generic_pinmux_dt_node_to_map,
 	.dt_free_map		= pinctrl_utils_free_map,
 };
 
@@ -568,8 +553,8 @@ static int spacemit_pmx_set_mux(struct pinctrl_dev *pctldev,
 {
 	struct spacemit_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	const struct group_desc *group;
-	const struct spacemit_pin_mux_config *configs;
 	unsigned int i, mux;
+	unsigned int *configs;
 	void __iomem *reg;
 
 	group = pinctrl_generic_get_group(pctldev, gsel);
@@ -579,11 +564,17 @@ static int spacemit_pmx_set_mux(struct pinctrl_dev *pctldev,
 	configs = group->data;
 
 	for (i = 0; i < group->grp.npins; i++) {
-		const struct spacemit_pin *spin = configs[i].pin;
-		u32 value = configs[i].config;
+		const struct spacemit_pin *spin;
+		u32 value = configs[i];
+
+		spin = spacemit_get_pin(pctrl, group->grp.pins[i]);
+		if (!spin) {
+			dev_err(pctrl->dev, "Invalid pin %u\n", group->grp.pins[i]);
+			return -EINVAL;
+		}
 
 		reg = spacemit_pin_to_reg(pctrl, spin->pin);
-		mux = spacemit_dt_get_pin_mux(value);
+		mux = value;
 
 		guard(raw_spinlock_irqsave)(&pctrl->lock);
 		value = readl_relaxed(reg) & ~PAD_MUX;
-- 
2.53.0

Re: [RFC v1 0/4] generic pinmux dt_node_to_map implementation

[Linus Walleij]

On Wed, May 6, 2026 at 11:58 AM Conor Dooley <conor@kernel.org> wrote:

> Whipped this up last week, at to a first glance it appears to work,
> although the spacemit platform I've used to implement this has very
> limited in-tree use of pinctrl so it is hard to be sure.

I like it, if it wasn't RFC I would merge it.

> What I don't love though is how similar the functions
> pinctrl_generic_pins_function_dt_node_to_map() and
> pinctrl_generic_pinmux_dt_node_to_map() are - essentially identical
> other than which function they in turn call.

Hm we can maybe think of something more descriptive
to the first one?

I think the new function is very much to the point. That's what
it does. pinctrl_generic_pins_function_dt_node_to_map() could
perhaps be names something that make it evident what is
special about it. Not that I have a good idea.

> Basically, I wanna know if you think that that is acceptable,

Looks Good To Me (TM) no-one else is helping out with pin
control core work so I'm happy for everything I get.

Yours,
Linus Walleij

Re: [RFC v1 0/4] generic pinmux dt_node_to_map implementation

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1641 bytes --]

On Mon, May 11, 2026 at 10:23:16PM +0200, Linus Walleij wrote:
> On Wed, May 6, 2026 at 11:58 AM Conor Dooley <conor@kernel.org> wrote:
> 
> > Whipped this up last week, at to a first glance it appears to work,
> > although the spacemit platform I've used to implement this has very
> > limited in-tree use of pinctrl so it is hard to be sure.
> 
> I like it, if it wasn't RFC I would merge it.

Half the reason that it is RFC is that I knew dlan wanted to take a look
but told me they weren't available, so I said I'd send it on the list in
the interim.

> > What I don't love though is how similar the functions
> > pinctrl_generic_pins_function_dt_node_to_map() and
> > pinctrl_generic_pinmux_dt_node_to_map() are - essentially identical
> > other than which function they in turn call.
> 
> Hm we can maybe think of something more descriptive
> to the first one?

I think the name is actually okay, it was the similarity of the code
that I don't like. There's a fair bit of duplication.

> I think the new function is very much to the point. That's what
> it does. pinctrl_generic_pins_function_dt_node_to_map() could
> perhaps be names something that make it evident what is
> special about it. Not that I have a good idea.
> 
> > Basically, I wanna know if you think that that is acceptable,
> 
> Looks Good To Me (TM) no-one else is helping out with pin
> control core work so I'm happy for everything I get.

Right, well I'll go clean it up I suppose. I might send a rfc v2 with an
extra patch that tries to get rid of some of the code duplication and
you can tell me which version you prefer?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC v1 0/4] generic pinmux dt_node_to_map implementation

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 9547 bytes --]

On Thu, May 14, 2026 at 07:57:46PM +0100, Conor Dooley wrote:
> On Mon, May 11, 2026 at 10:23:16PM +0200, Linus Walleij wrote:
> > On Wed, May 6, 2026 at 11:58 AM Conor Dooley <conor@kernel.org> wrote:
> > 
> > > Whipped this up last week, at to a first glance it appears to work,
> > > although the spacemit platform I've used to implement this has very
> > > limited in-tree use of pinctrl so it is hard to be sure.
> > 
> > I like it, if it wasn't RFC I would merge it.
> 
> Half the reason that it is RFC is that I knew dlan wanted to take a look
> but told me they weren't available, so I said I'd send it on the list in
> the interim.
> 
> > > What I don't love though is how similar the functions
> > > pinctrl_generic_pins_function_dt_node_to_map() and
> > > pinctrl_generic_pinmux_dt_node_to_map() are - essentially identical
> > > other than which function they in turn call.
> > 
> > Hm we can maybe think of something more descriptive
> > to the first one?
> 
> I think the name is actually okay, it was the similarity of the code
> that I don't like. There's a fair bit of duplication.
> 
> > I think the new function is very much to the point. That's what
> > it does. pinctrl_generic_pins_function_dt_node_to_map() could
> > perhaps be names something that make it evident what is
> > special about it. Not that I have a good idea.
> > 
> > > Basically, I wanna know if you think that that is acceptable,
> > 
> > Looks Good To Me (TM) no-one else is helping out with pin
> > control core work so I'm happy for everything I get.
> 
> Right, well I'll go clean it up I suppose. I might send a rfc v2 with an
> extra patch that tries to get rid of some of the code duplication and
> you can tell me which version you prefer?

Actually, no need for rfc v2, can just paste here:

diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c
index d1365acfd1f8c..9759b0186bcc2 100644
--- a/drivers/pinctrl/pinctrl-generic.c
+++ b/drivers/pinctrl/pinctrl-generic.c
@@ -119,92 +119,6 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
 				      functions, pins, npins);
 }
 
-/*
- * For platforms that do not define groups or functions in the driver, but
- * instead use the devicetree to describe them. This function will, unlike
- * pinconf_generic_dt_node_to_map() etc which rely on driver defined groups
- * and functions, create them in addition to parsing pinconf properties and
- * adding mappings.
- */
-int pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
-						 struct device_node *np,
-						 struct pinctrl_map **maps,
-						 unsigned int *num_maps)
-{
-	struct device *dev = pctldev->dev;
-	struct device_node *child_np;
-	const char **group_names;
-	unsigned int num_reserved_maps = 0;
-	int ngroups = 0;
-	int ret;
-
-	*maps = NULL;
-	*num_maps = 0;
-
-	/*
-	 * Check if this is actually the pins node, or a parent containing
-	 * multiple pins nodes.
-	 */
-	if (!of_property_present(np, "pins"))
-		goto parent;
-
-	group_names = devm_kcalloc(dev, 1, sizeof(*group_names), GFP_KERNEL);
-	if (!group_names)
-		return -ENOMEM;
-
-	ret = pinctrl_generic_pins_function_dt_subnode_to_map(pctldev, np, np,
-							      maps, num_maps,
-							      &num_reserved_maps,
-							      group_names,
-							      ngroups);
-	if (ret) {
-		pinctrl_utils_free_map(pctldev, *maps, *num_maps);
-		return dev_err_probe(dev, ret, "error figuring out mappings for %s\n", np->name);
-	}
-
-	ret = pinmux_generic_add_function(pctldev, np->name, group_names, 1, NULL);
-	if (ret < 0) {
-		pinctrl_utils_free_map(pctldev, *maps, *num_maps);
-		return dev_err_probe(dev, ret, "error adding function %s\n", np->name);
-	}
-
-	return 0;
-
-parent:
-	for_each_available_child_of_node(np, child_np)
-		ngroups += 1;
-
-	group_names = devm_kcalloc(dev, ngroups, sizeof(*group_names), GFP_KERNEL);
-	if (!group_names)
-		return -ENOMEM;
-
-	ngroups = 0;
-	for_each_available_child_of_node_scoped(np, child_np) {
-		ret = pinctrl_generic_pins_function_dt_subnode_to_map(pctldev, np, child_np,
-								      maps, num_maps,
-								      &num_reserved_maps,
-								      group_names,
-								      ngroups);
-		if (ret) {
-			pinctrl_utils_free_map(pctldev, *maps, *num_maps);
-			return dev_err_probe(dev, ret, "error figuring out mappings for %s\n",
-					     np->name);
-		}
-
-		ngroups++;
-	}
-
-	ret = pinmux_generic_add_function(pctldev, np->name, group_names, ngroups, NULL);
-	if (ret < 0) {
-		pinctrl_utils_free_map(pctldev, *maps, *num_maps);
-		return dev_err_probe(dev, ret, "error adding function %s\n", np->name);
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(pinctrl_generic_pins_function_dt_node_to_map);
-
-
 static int pinctrl_generic_pinmux_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 						    struct device_node *parent,
 						    struct device_node *np,
@@ -250,20 +164,19 @@ static int pinctrl_generic_pinmux_dt_subnode_to_map(struct pinctrl_dev *pctldev,
 				      muxes, pins, npins);
 }
 
-/*
- * For platforms that do not define groups or functions in the driver, but
- * instead use the devicetree to describe them. This function will, unlike
- * pinconf_generic_dt_node_to_map() etc which rely on driver defined groups
- * and functions, create them in addition to parsing pinconf properties and
- * adding mappings.
- *
- * It assumes that the upper 16 bits of the pinmux items contain the pin
- * and the lower 16 the mux setting.
- */
-int pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
+static int pinctrl_generic_dt_node_to_map(struct pinctrl_dev *pctldev,
 					  struct device_node *np,
 					  struct pinctrl_map **maps,
-					  unsigned int *num_maps)
+					  unsigned int *num_maps,
+					  int (dt_subnode_to_map)(
+						  struct pinctrl_dev *,
+						  struct device_node *,
+						  struct device_node *,
+						  struct pinctrl_map **,
+						  unsigned int *,
+						  unsigned int *,
+						  const char **,
+						  unsigned int))
 {
 	struct device *dev = pctldev->dev;
 	struct device_node *child_np;
@@ -276,21 +189,18 @@ int pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
 	*num_maps = 0;
 
 	/*
-	 * Check if this is actually the pinmux node, or a parent containing
-	 * multiple pinmux nodes.
+	 * Check if this is actually the pins node, or a parent containing
+	 * multiple pins nodes.
 	 */
-	if (!of_property_present(np, "pinmux"))
+	if (!of_property_present(np, "pins"))
 		goto parent;
 
 	group_names = devm_kcalloc(dev, 1, sizeof(*group_names), GFP_KERNEL);
 	if (!group_names)
 		return -ENOMEM;
 
-	ret = pinctrl_generic_pinmux_dt_subnode_to_map(pctldev, np, np,
-						       maps, num_maps,
-						       &num_reserved_maps,
-						       group_names,
-						       ngroups);
+	ret = dt_subnode_to_map(pctldev, np, np, maps, num_maps,
+				&num_reserved_maps, group_names, ngroups);
 	if (ret) {
 		pinctrl_utils_free_map(pctldev, *maps, *num_maps);
 		return dev_err_probe(dev, ret, "error figuring out mappings for %s\n", np->name);
@@ -314,11 +224,8 @@ int pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
 
 	ngroups = 0;
 	for_each_available_child_of_node_scoped(np, child_np) {
-		ret = pinctrl_generic_pinmux_dt_subnode_to_map(pctldev, np, child_np,
-							       maps, num_maps,
-							       &num_reserved_maps,
-							       group_names,
-							       ngroups);
+		ret = dt_subnode_to_map(pctldev, np, child_np, maps, num_maps,
+					&num_reserved_maps, group_names, ngroups);
 		if (ret) {
 			pinctrl_utils_free_map(pctldev, *maps, *num_maps);
 			return dev_err_probe(dev, ret, "error figuring out mappings for %s\n",
@@ -336,4 +243,40 @@ int pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
 
 	return 0;
 }
+
+/*
+ * For platforms that do not define groups or functions in the driver, but
+ * instead use the devicetree to describe them. This function will, unlike
+ * pinconf_generic_dt_node_to_map() etc which rely on driver defined groups
+ * and functions, create them in addition to parsing pinconf properties and
+ * adding mappings.
+ */
+int pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
+						 struct device_node *np,
+						 struct pinctrl_map **maps,
+						 unsigned int *num_maps)
+{
+	return pinctrl_generic_dt_node_to_map(pctldev, np, maps, num_maps,
+					      &pinctrl_generic_pins_function_dt_subnode_to_map);
+}
+EXPORT_SYMBOL_GPL(pinctrl_generic_pins_function_dt_node_to_map);
+
+/*
+ * For platforms that do not define groups or functions in the driver, but
+ * instead use the devicetree to describe them. This function will, unlike
+ * pinconf_generic_dt_node_to_map() etc which rely on driver defined groups
+ * and functions, create them in addition to parsing pinconf properties and
+ * adding mappings.
+ *
+ * It assumes that the upper 16 bits of the pinmux items contain the pin
+ * and the lower 16 the mux setting.
+ */
+int pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
+					  struct device_node *np,
+					  struct pinctrl_map **maps,
+					  unsigned int *num_maps)
+{
+	return pinctrl_generic_dt_node_to_map(pctldev, np, maps, num_maps,
+					      &pinctrl_generic_pinmux_dt_subnode_to_map);
+};
 EXPORT_SYMBOL_GPL(pinctrl_generic_pinmux_dt_node_to_map);


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC v1 0/4] generic pinmux dt_node_to_map implementation

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 10281 bytes --]

On Thu, May 14, 2026 at 08:40:26PM +0100, Conor Dooley wrote:
> On Thu, May 14, 2026 at 07:57:46PM +0100, Conor Dooley wrote:
> > On Mon, May 11, 2026 at 10:23:16PM +0200, Linus Walleij wrote:
> > > On Wed, May 6, 2026 at 11:58 AM Conor Dooley <conor@kernel.org> wrote:
> > > 
> > > > Whipped this up last week, at to a first glance it appears to work,
> > > > although the spacemit platform I've used to implement this has very
> > > > limited in-tree use of pinctrl so it is hard to be sure.
> > > 
> > > I like it, if it wasn't RFC I would merge it.
> > 
> > Half the reason that it is RFC is that I knew dlan wanted to take a look
> > but told me they weren't available, so I said I'd send it on the list in
> > the interim.
> > 
> > > > What I don't love though is how similar the functions
> > > > pinctrl_generic_pins_function_dt_node_to_map() and
> > > > pinctrl_generic_pinmux_dt_node_to_map() are - essentially identical
> > > > other than which function they in turn call.
> > > 
> > > Hm we can maybe think of something more descriptive
> > > to the first one?
> > 
> > I think the name is actually okay, it was the similarity of the code
> > that I don't like. There's a fair bit of duplication.
> > 
> > > I think the new function is very much to the point. That's what
> > > it does. pinctrl_generic_pins_function_dt_node_to_map() could
> > > perhaps be names something that make it evident what is
> > > special about it. Not that I have a good idea.
> > > 
> > > > Basically, I wanna know if you think that that is acceptable,
> > > 
> > > Looks Good To Me (TM) no-one else is helping out with pin
> > > control core work so I'm happy for everything I get.
> > 
> > Right, well I'll go clean it up I suppose. I might send a rfc v2 with an
> > extra patch that tries to get rid of some of the code duplication and
> > you can tell me which version you prefer?
> 
> Actually, no need for rfc v2, can just paste here:
> 
> diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c
> index d1365acfd1f8c..9759b0186bcc2 100644
> --- a/drivers/pinctrl/pinctrl-generic.c
> +++ b/drivers/pinctrl/pinctrl-generic.c
> @@ -119,92 +119,6 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
>  				      functions, pins, npins);
>  }
>  
> -/*
> - * For platforms that do not define groups or functions in the driver, but
> - * instead use the devicetree to describe them. This function will, unlike
> - * pinconf_generic_dt_node_to_map() etc which rely on driver defined groups
> - * and functions, create them in addition to parsing pinconf properties and
> - * adding mappings.
> - */
> -int pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
> -						 struct device_node *np,
> -						 struct pinctrl_map **maps,
> -						 unsigned int *num_maps)
> -{
> -	struct device *dev = pctldev->dev;
> -	struct device_node *child_np;
> -	const char **group_names;
> -	unsigned int num_reserved_maps = 0;
> -	int ngroups = 0;
> -	int ret;
> -
> -	*maps = NULL;
> -	*num_maps = 0;
> -
> -	/*
> -	 * Check if this is actually the pins node, or a parent containing
> -	 * multiple pins nodes.
> -	 */
> -	if (!of_property_present(np, "pins"))
> -		goto parent;
> -
> -	group_names = devm_kcalloc(dev, 1, sizeof(*group_names), GFP_KERNEL);
> -	if (!group_names)
> -		return -ENOMEM;
> -
> -	ret = pinctrl_generic_pins_function_dt_subnode_to_map(pctldev, np, np,
> -							      maps, num_maps,
> -							      &num_reserved_maps,
> -							      group_names,
> -							      ngroups);
> -	if (ret) {
> -		pinctrl_utils_free_map(pctldev, *maps, *num_maps);
> -		return dev_err_probe(dev, ret, "error figuring out mappings for %s\n", np->name);
> -	}
> -
> -	ret = pinmux_generic_add_function(pctldev, np->name, group_names, 1, NULL);
> -	if (ret < 0) {
> -		pinctrl_utils_free_map(pctldev, *maps, *num_maps);
> -		return dev_err_probe(dev, ret, "error adding function %s\n", np->name);
> -	}
> -
> -	return 0;
> -
> -parent:
> -	for_each_available_child_of_node(np, child_np)
> -		ngroups += 1;
> -
> -	group_names = devm_kcalloc(dev, ngroups, sizeof(*group_names), GFP_KERNEL);
> -	if (!group_names)
> -		return -ENOMEM;
> -
> -	ngroups = 0;
> -	for_each_available_child_of_node_scoped(np, child_np) {
> -		ret = pinctrl_generic_pins_function_dt_subnode_to_map(pctldev, np, child_np,
> -								      maps, num_maps,
> -								      &num_reserved_maps,
> -								      group_names,
> -								      ngroups);
> -		if (ret) {
> -			pinctrl_utils_free_map(pctldev, *maps, *num_maps);
> -			return dev_err_probe(dev, ret, "error figuring out mappings for %s\n",
> -					     np->name);
> -		}
> -
> -		ngroups++;
> -	}
> -
> -	ret = pinmux_generic_add_function(pctldev, np->name, group_names, ngroups, NULL);
> -	if (ret < 0) {
> -		pinctrl_utils_free_map(pctldev, *maps, *num_maps);
> -		return dev_err_probe(dev, ret, "error adding function %s\n", np->name);
> -	}
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(pinctrl_generic_pins_function_dt_node_to_map);
> -
> -
>  static int pinctrl_generic_pinmux_dt_subnode_to_map(struct pinctrl_dev *pctldev,
>  						    struct device_node *parent,
>  						    struct device_node *np,
> @@ -250,20 +164,19 @@ static int pinctrl_generic_pinmux_dt_subnode_to_map(struct pinctrl_dev *pctldev,
>  				      muxes, pins, npins);
>  }
>  
> -/*
> - * For platforms that do not define groups or functions in the driver, but
> - * instead use the devicetree to describe them. This function will, unlike
> - * pinconf_generic_dt_node_to_map() etc which rely on driver defined groups
> - * and functions, create them in addition to parsing pinconf properties and
> - * adding mappings.
> - *
> - * It assumes that the upper 16 bits of the pinmux items contain the pin
> - * and the lower 16 the mux setting.
> - */
> -int pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
> +static int pinctrl_generic_dt_node_to_map(struct pinctrl_dev *pctldev,

This function name conflicts with another helper in the pinctrl
core, so I would have to rename it, but that's not needed for proof of
concept of course.

>  					  struct device_node *np,
>  					  struct pinctrl_map **maps,
> -					  unsigned int *num_maps)
> +					  unsigned int *num_maps,
> +					  int (dt_subnode_to_map)(
> +						  struct pinctrl_dev *,
> +						  struct device_node *,
> +						  struct device_node *,
> +						  struct pinctrl_map **,
> +						  unsigned int *,
> +						  unsigned int *,
> +						  const char **,
> +						  unsigned int))
>  {
>  	struct device *dev = pctldev->dev;
>  	struct device_node *child_np;
> @@ -276,21 +189,18 @@ int pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
>  	*num_maps = 0;
>  
>  	/*
> -	 * Check if this is actually the pinmux node, or a parent containing
> -	 * multiple pinmux nodes.
> +	 * Check if this is actually the pins node, or a parent containing
> +	 * multiple pins nodes.
>  	 */
> -	if (!of_property_present(np, "pinmux"))
> +	if (!of_property_present(np, "pins"))
>  		goto parent;
>  
>  	group_names = devm_kcalloc(dev, 1, sizeof(*group_names), GFP_KERNEL);
>  	if (!group_names)
>  		return -ENOMEM;
>  
> -	ret = pinctrl_generic_pinmux_dt_subnode_to_map(pctldev, np, np,
> -						       maps, num_maps,
> -						       &num_reserved_maps,
> -						       group_names,
> -						       ngroups);
> +	ret = dt_subnode_to_map(pctldev, np, np, maps, num_maps,
> +				&num_reserved_maps, group_names, ngroups);
>  	if (ret) {
>  		pinctrl_utils_free_map(pctldev, *maps, *num_maps);
>  		return dev_err_probe(dev, ret, "error figuring out mappings for %s\n", np->name);
> @@ -314,11 +224,8 @@ int pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
>  
>  	ngroups = 0;
>  	for_each_available_child_of_node_scoped(np, child_np) {
> -		ret = pinctrl_generic_pinmux_dt_subnode_to_map(pctldev, np, child_np,
> -							       maps, num_maps,
> -							       &num_reserved_maps,
> -							       group_names,
> -							       ngroups);
> +		ret = dt_subnode_to_map(pctldev, np, child_np, maps, num_maps,
> +					&num_reserved_maps, group_names, ngroups);
>  		if (ret) {
>  			pinctrl_utils_free_map(pctldev, *maps, *num_maps);
>  			return dev_err_probe(dev, ret, "error figuring out mappings for %s\n",
> @@ -336,4 +243,40 @@ int pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
>  
>  	return 0;
>  }
> +
> +/*
> + * For platforms that do not define groups or functions in the driver, but
> + * instead use the devicetree to describe them. This function will, unlike
> + * pinconf_generic_dt_node_to_map() etc which rely on driver defined groups
> + * and functions, create them in addition to parsing pinconf properties and
> + * adding mappings.
> + */
> +int pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
> +						 struct device_node *np,
> +						 struct pinctrl_map **maps,
> +						 unsigned int *num_maps)
> +{
> +	return pinctrl_generic_dt_node_to_map(pctldev, np, maps, num_maps,
> +					      &pinctrl_generic_pins_function_dt_subnode_to_map);
> +}
> +EXPORT_SYMBOL_GPL(pinctrl_generic_pins_function_dt_node_to_map);
> +
> +/*
> + * For platforms that do not define groups or functions in the driver, but
> + * instead use the devicetree to describe them. This function will, unlike
> + * pinconf_generic_dt_node_to_map() etc which rely on driver defined groups
> + * and functions, create them in addition to parsing pinconf properties and
> + * adding mappings.
> + *
> + * It assumes that the upper 16 bits of the pinmux items contain the pin
> + * and the lower 16 the mux setting.
> + */
> +int pinctrl_generic_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
> +					  struct device_node *np,
> +					  struct pinctrl_map **maps,
> +					  unsigned int *num_maps)
> +{
> +	return pinctrl_generic_dt_node_to_map(pctldev, np, maps, num_maps,
> +					      &pinctrl_generic_pinmux_dt_subnode_to_map);
> +};
>  EXPORT_SYMBOL_GPL(pinctrl_generic_pinmux_dt_node_to_map);
> 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]