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

[Conor Dooley <conor@kernel.org>]

[-- 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 --]