[RFC] pinctrl: pinconf-generic: move ..dt_node_to_map_pinmux() to amlogic-am4 driver

[RFC] pinctrl: pinconf-generic: move ..dt_node_to_map_pinmux() to amlogic-am4 driver

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.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@microchip.com>
---
CC: Xianwei Zhao <xianwei.zhao@amlogic.com>
CC: Linus Walleij <linusw@kernel.org>
CC: Neil Armstrong <neil.armstrong@linaro.org>
CC: Kevin Hilman <khilman@baylibre.com>
CC: Jerome Brunet <jbrunet@baylibre.com>
CC: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
CC: linux-amlogic@lists.infradead.org
CC: linux-gpio@vger.kernel.org
CC: linux-arm-kernel@lists.infradead.org
CC: linux-kernel@vger.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

Re: [RFC] pinctrl: pinconf-generic: move ..dt_node_to_map_pinmux() to amlogic-am4 driver

[Linus Walleij]

On Tue, Feb 3, 2026 at 5:17 PM Conor Dooley <conor@kernel.org> wrote:

> From: Conor Dooley <conor.dooley@microchip.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.
(...)
> 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.

This alone is a good reason to apply the patch.

I have a strong urge to apply this already for v7.0 despite its RFC state.

Anyone against?

Yours,
Linus Walleij

Re: [RFC] pinctrl: pinconf-generic: move ..dt_node_to_map_pinmux() to amlogic-am4 driver

[Andy Shevchenko]

On Wed, Feb 04, 2026 at 12:34:36AM +0100, Linus Walleij wrote:
> On Tue, Feb 3, 2026 at 5:17 PM Conor Dooley <conor@kernel.org> wrote:

> > 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.
> (...)
> > 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.
> 
> This alone is a good reason to apply the patch.
> 
> I have a strong urge to apply this already for v7.0 despite its RFC state.
> 
> Anyone against?

Quite the opposite! I fully support unloading pin control core from OF-centric
code. Note, please, remove extra '.' (dot) in the Subject.o
Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] pinctrl: pinconf-generic: move ..dt_node_to_map_pinmux() to amlogic-am4 driver

[Conor Dooley]

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

On Wed, Feb 04, 2026 at 09:05:34AM +0100, Andy Shevchenko wrote:
> On Wed, Feb 04, 2026 at 12:34:36AM +0100, Linus Walleij wrote:
> > On Tue, Feb 3, 2026 at 5:17 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > > 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.
> > (...)
> > > 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.
> > 
> > This alone is a good reason to apply the patch.
> > 
> > I have a strong urge to apply this already for v7.0 despite its RFC state.
> > 
> > Anyone against?

I forgot to say that I had made it rfc cos of where in the cycle we are
and the fact that I didn't test it. I've got no objection though if
that's what you want to do.

> Quite the opposite! I fully support unloading pin control core from OF-centric
> code.

> Note, please, remove extra '.' (dot) in the Subject.o

fwiw, the .. was intentional cos I was truncating the pinconf_generic
from the function since the subject was really long, not referring to
a member of an ops struct.

> Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com>

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

Re: [RFC] pinctrl: pinconf-generic: move ..dt_node_to_map_pinmux() to amlogic-am4 driver

[Andy Shevchenko]

On Wed, Feb 04, 2026 at 02:15:10PM +0000, Conor Dooley wrote:
> On Wed, Feb 04, 2026 at 09:05:34AM +0100, Andy Shevchenko wrote:
> > On Wed, Feb 04, 2026 at 12:34:36AM +0100, Linus Walleij wrote:
> > > On Tue, Feb 3, 2026 at 5:17 PM Conor Dooley <conor@kernel.org> wrote:

...

> > Note, please, remove extra '.' (dot) in the Subject.o
> 
> fwiw, the .. was intentional cos I was truncating the pinconf_generic
> from the function since the subject was really long, not referring to
> a member of an ops struct.

Yes, and that's how we refer to the callbacks — with a single dot and parentheses:

	.my_cool_cb()

Alternatively

	->my_cool_cb()

but it one character longer and TBH it slightly less readable (I personally
used the latter and then switched to the former in the recent years).

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] pinctrl: pinconf-generic: move ..dt_node_to_map_pinmux() to amlogic-am4 driver

[Andy Shevchenko]

On Wed, Feb 04, 2026 at 04:22:47PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 04, 2026 at 02:15:10PM +0000, Conor Dooley wrote:
> > On Wed, Feb 04, 2026 at 09:05:34AM +0100, Andy Shevchenko wrote:
> > > On Wed, Feb 04, 2026 at 12:34:36AM +0100, Linus Walleij wrote:
> > > > On Tue, Feb 3, 2026 at 5:17 PM Conor Dooley <conor@kernel.org> wrote:

...

> > > Note, please, remove extra '.' (dot) in the Subject.o
> > 
> > fwiw, the .. was intentional cos I was truncating the pinconf_generic
> > from the function since the subject was really long, not referring to
> > a member of an ops struct.
> 
> Yes, and that's how we refer to the callbacks — with a single dot and parentheses:
> 
> 	.my_cool_cb()
> 
> Alternatively
> 
> 	->my_cool_cb()
> 
> but it one character longer and TBH it slightly less readable (I personally
> used the latter and then switched to the former in the recent years).

Hmm... My memory tricked me, it seems I switched to ->cb() notation, at least
there are patches with that from October last year. Whatever, choose one and
use it :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] pinctrl: pinconf-generic: move ..dt_node_to_map_pinmux() to amlogic-am4 driver

[Conor Dooley]

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

On Wed, Feb 04, 2026 at 04:26:29PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 04, 2026 at 04:22:47PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 04, 2026 at 02:15:10PM +0000, Conor Dooley wrote:
> > > On Wed, Feb 04, 2026 at 09:05:34AM +0100, Andy Shevchenko wrote:
> > > > On Wed, Feb 04, 2026 at 12:34:36AM +0100, Linus Walleij wrote:
> > > > > On Tue, Feb 3, 2026 at 5:17 PM Conor Dooley <conor@kernel.org> wrote:
> 
> ...
> 
> > > > Note, please, remove extra '.' (dot) in the Subject.o
> > > 
> > > fwiw, the .. was intentional cos I was truncating the pinconf_generic
> > > from the function since the subject was really long, not referring to
> > > a member of an ops struct.
> > 
> > Yes, and that's how we refer to the callbacks — with a single dot and parentheses:
> > 
> > 	.my_cool_cb()
> > 
> > Alternatively
> > 
> > 	->my_cool_cb()
> > 
> > but it one character longer and TBH it slightly less readable (I personally
> > used the latter and then switched to the former in the recent years).
> 
> Hmm... My memory tricked me, it seems I switched to ->cb() notation, at least
> there are patches with that from October last year. Whatever, choose one and
> use it :-)

I think you missed my point, I was /not/ trying to refer to an ops struct
member. For those I follow the first of the two notations you listed.

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

Re: [RFC] pinctrl: pinconf-generic: move ..dt_node_to_map_pinmux() to amlogic-am4 driver

[Andy Shevchenko]

On Wed, Feb 04, 2026 at 03:50:02PM +0000, Conor Dooley wrote:
> On Wed, Feb 04, 2026 at 04:26:29PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 04, 2026 at 04:22:47PM +0200, Andy Shevchenko wrote:
> > > On Wed, Feb 04, 2026 at 02:15:10PM +0000, Conor Dooley wrote:
> > > > On Wed, Feb 04, 2026 at 09:05:34AM +0100, Andy Shevchenko wrote:
> > > > > On Wed, Feb 04, 2026 at 12:34:36AM +0100, Linus Walleij wrote:
> > > > > > On Tue, Feb 3, 2026 at 5:17 PM Conor Dooley <conor@kernel.org> wrote:

...

> > > > > Note, please, remove extra '.' (dot) in the Subject.o
> > > > 
> > > > fwiw, the .. was intentional cos I was truncating the pinconf_generic
> > > > from the function since the subject was really long, not referring to
> > > > a member of an ops struct.
> > > 
> > > Yes, and that's how we refer to the callbacks — with a single dot and parentheses:
> > > 
> > > 	.my_cool_cb()
> > > 
> > > Alternatively
> > > 
> > > 	->my_cool_cb()
> > > 
> > > but it one character longer and TBH it slightly less readable (I personally
> > > used the latter and then switched to the former in the recent years).
> > 
> > Hmm... My memory tricked me, it seems I switched to ->cb() notation, at least
> > there are patches with that from October last year. Whatever, choose one and
> > use it :-)
> 
> I think you missed my point, I was /not/ trying to refer to an ops struct
> member. For those I follow the first of the two notations you listed.

Indeed, for that cases I usually use and underscore (however it might
in some cases be ambiguous):

  pinctrl: pinconf-generic: move _dt_node_to_map_pinmux() to amlogic-am4 driver

I think it's possible to drop the 'pinctrl:' prefix, the pinconf-generic is
unique enough and had been used in the past a couple of times (yes, I know that
the convention is to use subsystem prefix).

Also word 'driver' can be dropped.

TL;DR: My point that double dots is confusing and non-standard way to refer to
something which is cut. If not full, ideally it can use triple dots followed by
underscore

  pinconf-generic: move ..._dt_node_to_map_pinmux() to amlogic-am4 driver


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] pinctrl: pinconf-generic: move ..dt_node_to_map_pinmux() to amlogic-am4 driver

[Conor Dooley]

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

On Wed, Feb 04, 2026 at 06:16:58PM +0200, Andy Shevchenko wrote:
> 
> TL;DR: My point that double dots is confusing and non-standard way to refer to
> something which is cut. If not full, ideally it can use triple dots followed by
> underscore
> 
>   pinconf-generic: move ..._dt_node_to_map_pinmux() to amlogic-am4 driver

This looks good to me. Linus, want a resend or will you adapt it
yourself?

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

Re: [RFC] pinctrl: pinconf-generic: move ..dt_node_to_map_pinmux() to amlogic-am4 driver

[Linus Walleij]

On Tue, Feb 3, 2026 at 5:17 PM Conor Dooley <conor@kernel.org> wrote:


> From: Conor Dooley <conor.dooley@microchip.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.

Patch subject tweaked and applied!

Yours,
Linus Walleij