Refactor pinctrl_generic_pins_function_dt_subnode_to_map() by separating DT
parsing logic from map creation. Introduce a new helper
pinctrl_generic_to_map() to handle mapping to kernel data structures, while
keeping DT property parsing in the subnode function.

Improve code structure and enables easier reuse for platforms using
different DT properties (e.g. pinmux) without modifying the
dt_node_to_map-style callback API. Avoid unnecessary coupling to
pinctrl_generic_pins_function_dt_node_to_map(), which provides
functionality not needed when the phandle target is unambiguous.

Maximize code reuse and provide a cleaner extension point for future
pinctrl drivers.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change in v4
- new patch
---
 drivers/pinctrl/pinconf.h         | 18 ++++++++
 drivers/pinctrl/pinctrl-generic.c | 91 ++++++++++++++++++++++++---------------
 2 files changed, 74 insertions(+), 35 deletions(-)

diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
index 2880adef476e68950ffdd540ea42cdee6a16ec27..ffdabddb9660324ed8886a2e8dcacff7e1c6c529 100644
--- a/drivers/pinctrl/pinconf.h
+++ b/drivers/pinctrl/pinconf.h
@@ -166,6 +166,13 @@ 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);
+
+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,
+		       const char **group_name, unsigned int ngroups,
+		       const char **functions, unsigned int *pins);
 #else
 static inline int
 pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
@@ -175,4 +182,15 @@ pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
 {
 	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,
+		       unsigned int *num_maps, unsigned int *num_reserved_maps,
+		       const char **group_name, unsigned int ngroups,
+		       const char **functions, unsigned int *pins,
+		       void *function_data)
+{
+	return -ENOTSUPP;
+}
 #endif
diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c
index efb39c6a670331775855efdc8566102b5c6202ef..20a216ae63e91b69985ea4cfcd0b57103c6ca950 100644
--- a/drivers/pinctrl/pinctrl-generic.c
+++ b/drivers/pinctrl/pinctrl-generic.c
@@ -17,29 +17,18 @@
 #include "pinctrl-utils.h"
 #include "pinmux.h"
 
-static int pinctrl_generic_pins_function_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)
+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,
+		       const char **group_names, unsigned int ngroups,
+		       const char **functions, unsigned int *pins)
 {
 	struct device *dev = pctldev->dev;
-	const char **functions;
+	int npins, ret, reserve = 1;
+	unsigned int num_configs;
 	const char *group_name;
 	unsigned long *configs;
-	unsigned int num_configs, pin, *pins;
-	int npins, ret, reserve = 1;
-
-	npins = of_property_count_u32_elems(np, "pins");
-
-	if (npins < 1) {
-		dev_err(dev, "invalid pinctrl group %pOFn.%pOFn %d\n",
-			parent, np, npins);
-		return npins;
-	}
 
 	group_name = devm_kasprintf(dev, GFP_KERNEL, "%pOFn.%pOFn", parent, np);
 	if (!group_name)
@@ -51,22 +40,6 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
 	if (!pins)
 		return -ENOMEM;
 
-	functions = devm_kcalloc(dev, npins, sizeof(*functions), GFP_KERNEL);
-	if (!functions)
-		return -ENOMEM;
-
-	for (int i = 0; i < npins; i++) {
-		ret = of_property_read_u32_index(np, "pins", i, &pin);
-		if (ret)
-			return ret;
-
-		pins[i] = pin;
-
-		ret = of_property_read_string(np, "function", &functions[i]);
-		if (ret)
-			return ret;
-	}
-
 	ret = pinctrl_utils_reserve_map(pctldev, maps, num_reserved_maps, num_maps, reserve);
 	if (ret)
 		return ret;
@@ -103,6 +76,54 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
 	return 0;
 };
 
+static int
+pinctrl_generic_pins_function_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 pin, *pins;
+	const char **functions;
+	int npins, ret;
+
+	npins = of_property_count_u32_elems(np, "pins");
+
+	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;
+
+	functions = devm_kcalloc(dev, npins, sizeof(*functions), GFP_KERNEL);
+	if (!functions)
+		return -ENOMEM;
+
+	for (int i = 0; i < npins; i++) {
+		ret = of_property_read_u32_index(np, "pins", i, &pin);
+		if (ret)
+			return ret;
+
+		pins[i] = pin;
+
+		ret = of_property_read_string(np, "function", &functions[i]);
+		if (ret)
+			return ret;
+	}
+
+	return pinctrl_generic_to_map(pctldev, parent, np, maps, num_maps,
+				      num_reserved_maps, group_names, ngroups,
+				      functions, pins);
+}
+
 /*
  * For platforms that do not define groups or functions in the driver, but
  * instead use the devicetree to describe them. This function will, unlike

-- 
2.43.0

On Wed, Mar 25, 2026 at 07:04:12PM -0400, Frank Li wrote:
> Refactor pinctrl_generic_pins_function_dt_subnode_to_map() by separating DT
> parsing logic from map creation. Introduce a new helper
> pinctrl_generic_to_map() to handle mapping to kernel data structures, while
> keeping DT property parsing in the subnode function.
> 
> Improve code structure and enables easier reuse for platforms using
> different DT properties (e.g. pinmux) without modifying the
> dt_node_to_map-style callback API. Avoid unnecessary coupling to
> pinctrl_generic_pins_function_dt_node_to_map(), which provides
> functionality not needed when the phandle target is unambiguous.
> 
> Maximize code reuse and provide a cleaner extension point for future
> pinctrl drivers.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> change in v4
> - new patch
> ---
>  drivers/pinctrl/pinconf.h         | 18 ++++++++
>  drivers/pinctrl/pinctrl-generic.c | 91 ++++++++++++++++++++++++---------------
>  2 files changed, 74 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
> index 2880adef476e68950ffdd540ea42cdee6a16ec27..ffdabddb9660324ed8886a2e8dcacff7e1c6c529 100644
> --- a/drivers/pinctrl/pinconf.h
> +++ b/drivers/pinctrl/pinconf.h
> @@ -166,6 +166,13 @@ 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);
> +
> +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,
> +		       const char **group_name, unsigned int ngroups,
> +		       const char **functions, unsigned int *pins);
>  #else
>  static inline int
>  pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
> @@ -175,4 +182,15 @@ pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
>  {
>  	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,
> +		       unsigned int *num_maps, unsigned int *num_reserved_maps,
> +		       const char **group_name, unsigned int ngroups,
> +		       const char **functions, unsigned int *pins,
> +		       void *function_data)
> +{
> +	return -ENOTSUPP;
> +}
>  #endif
> diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c
> index efb39c6a670331775855efdc8566102b5c6202ef..20a216ae63e91b69985ea4cfcd0b57103c6ca950 100644
> --- a/drivers/pinctrl/pinctrl-generic.c
> +++ b/drivers/pinctrl/pinctrl-generic.c
> @@ -17,29 +17,18 @@
>  #include "pinctrl-utils.h"
>  #include "pinmux.h"
>  
> -static int pinctrl_generic_pins_function_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)
> +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,
> +		       const char **group_names, unsigned int ngroups,
> +		       const char **functions, unsigned int *pins)

npins needs to be an argument to this function also, otherwise
pinctrl_generic_add_group() uses it uninitialised...

>  {
>  	struct device *dev = pctldev->dev;
> -	const char **functions;
> +	int npins, ret, reserve = 1;

...because you're declaring it here when it's something set by the dt
parsing code in pinctrl_generic_pins_function_dt_subnode_to_map()...

> +	unsigned int num_configs;
>  	const char *group_name;
>  	unsigned long *configs;
> -	unsigned int num_configs, pin, *pins;
> -	int npins, ret, reserve = 1;
> -
> -	npins = of_property_count_u32_elems(np, "pins");
> -
> -	if (npins < 1) {
> -		dev_err(dev, "invalid pinctrl group %pOFn.%pOFn %d\n",
> -			parent, np, npins);
> -		return npins;
> -	}
>  
>  	group_name = devm_kasprintf(dev, GFP_KERNEL, "%pOFn.%pOFn", parent, np);
>  	if (!group_name)
> @@ -51,22 +40,6 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
>  	if (!pins)
>  		return -ENOMEM;
>  
> -	functions = devm_kcalloc(dev, npins, sizeof(*functions), GFP_KERNEL);
> -	if (!functions)
> -		return -ENOMEM;
> -
> -	for (int i = 0; i < npins; i++) {
> -		ret = of_property_read_u32_index(np, "pins", i, &pin);
> -		if (ret)
> -			return ret;
> -
> -		pins[i] = pin;
> -
> -		ret = of_property_read_string(np, "function", &functions[i]);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	ret = pinctrl_utils_reserve_map(pctldev, maps, num_reserved_maps, num_maps, reserve);
>  	if (ret)
>  		return ret;
> @@ -103,6 +76,54 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
>  	return 0;
>  };
>  
> +static int
> +pinctrl_generic_pins_function_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 pin, *pins;
> +	const char **functions;
> +	int npins, ret;
> +
> +	npins = of_property_count_u32_elems(np, "pins");

...down here.

> +
> +	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;
> +
> +	functions = devm_kcalloc(dev, npins, sizeof(*functions), GFP_KERNEL);
> +	if (!functions)
> +		return -ENOMEM;
> +
> +	for (int i = 0; i < npins; i++) {
> +		ret = of_property_read_u32_index(np, "pins", i, &pin);
> +		if (ret)
> +			return ret;
> +
> +		pins[i] = pin;
> +
> +		ret = of_property_read_string(np, "function", &functions[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return pinctrl_generic_to_map(pctldev, parent, np, maps, num_maps,
> +				      num_reserved_maps, group_names, ngroups,
> +				      functions, pins);
> +}
> +
>  /*
>   * For platforms that do not define groups or functions in the driver, but
>   * instead use the devicetree to describe them. This function will, unlike
> 
> -- 
> 2.43.0
> 

On Fri, Mar 27, 2026 at 12:09:32AM +0000, Conor Dooley wrote:
> On Wed, Mar 25, 2026 at 07:04:12PM -0400, Frank Li wrote:
> > Refactor pinctrl_generic_pins_function_dt_subnode_to_map() by separating DT
> > parsing logic from map creation. Introduce a new helper
> > pinctrl_generic_to_map() to handle mapping to kernel data structures, while
> > keeping DT property parsing in the subnode function.
> >
> > Improve code structure and enables easier reuse for platforms using
> > different DT properties (e.g. pinmux) without modifying the
> > dt_node_to_map-style callback API. Avoid unnecessary coupling to
> > pinctrl_generic_pins_function_dt_node_to_map(), which provides
> > functionality not needed when the phandle target is unambiguous.
> >
> > Maximize code reuse and provide a cleaner extension point for future
> > pinctrl drivers.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > change in v4
> > - new patch
> > ---
> >  drivers/pinctrl/pinconf.h         | 18 ++++++++
> >  drivers/pinctrl/pinctrl-generic.c | 91 ++++++++++++++++++++++++---------------
> >  2 files changed, 74 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
> > index 2880adef476e68950ffdd540ea42cdee6a16ec27..ffdabddb9660324ed8886a2e8dcacff7e1c6c529 100644
> > --- a/drivers/pinctrl/pinconf.h
> > +++ b/drivers/pinctrl/pinconf.h
> > @@ -166,6 +166,13 @@ 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);
> > +
> > +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,
> > +		       const char **group_name, unsigned int ngroups,
> > +		       const char **functions, unsigned int *pins);
> >  #else
> >  static inline int
> >  pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
> > @@ -175,4 +182,15 @@ pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
> >  {
> >  	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,
> > +		       unsigned int *num_maps, unsigned int *num_reserved_maps,
> > +		       const char **group_name, unsigned int ngroups,
> > +		       const char **functions, unsigned int *pins,
> > +		       void *function_data)
> > +{
> > +	return -ENOTSUPP;
> > +}
> >  #endif
> > diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c
> > index efb39c6a670331775855efdc8566102b5c6202ef..20a216ae63e91b69985ea4cfcd0b57103c6ca950 100644
> > --- a/drivers/pinctrl/pinctrl-generic.c
> > +++ b/drivers/pinctrl/pinctrl-generic.c
> > @@ -17,29 +17,18 @@
> >  #include "pinctrl-utils.h"
> >  #include "pinmux.h"
> >
> > -static int pinctrl_generic_pins_function_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)
> > +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,
> > +		       const char **group_names, unsigned int ngroups,
> > +		       const char **functions, unsigned int *pins)
>
> npins needs to be an argument to this function also, otherwise
> pinctrl_generic_add_group() uses it uninitialised...

Is this one the root cause of then broken? I am not sure why compiler have
not report waring for it.

Frank
> >

On Fri, Mar 27, 2026 at 12:54:42PM -0400, Frank Li wrote:
> On Fri, Mar 27, 2026 at 12:09:32AM +0000, Conor Dooley wrote:
> > On Wed, Mar 25, 2026 at 07:04:12PM -0400, Frank Li wrote:
> > > Refactor pinctrl_generic_pins_function_dt_subnode_to_map() by separating DT
> > > parsing logic from map creation. Introduce a new helper
> > > pinctrl_generic_to_map() to handle mapping to kernel data structures, while
> > > keeping DT property parsing in the subnode function.
> > >
> > > Improve code structure and enables easier reuse for platforms using
> > > different DT properties (e.g. pinmux) without modifying the
> > > dt_node_to_map-style callback API. Avoid unnecessary coupling to
> > > pinctrl_generic_pins_function_dt_node_to_map(), which provides
> > > functionality not needed when the phandle target is unambiguous.
> > >
> > > Maximize code reuse and provide a cleaner extension point for future
> > > pinctrl drivers.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > > change in v4
> > > - new patch
> > > ---
> > >  drivers/pinctrl/pinconf.h         | 18 ++++++++
> > >  drivers/pinctrl/pinctrl-generic.c | 91 ++++++++++++++++++++++++---------------
> > >  2 files changed, 74 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
> > > index 2880adef476e68950ffdd540ea42cdee6a16ec27..ffdabddb9660324ed8886a2e8dcacff7e1c6c529 100644
> > > --- a/drivers/pinctrl/pinconf.h
> > > +++ b/drivers/pinctrl/pinconf.h
> > > @@ -166,6 +166,13 @@ 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);
> > > +
> > > +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,
> > > +		       const char **group_name, unsigned int ngroups,
> > > +		       const char **functions, unsigned int *pins);
> > >  #else
> > >  static inline int
> > >  pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
> > > @@ -175,4 +182,15 @@ pinctrl_generic_pins_function_dt_node_to_map(struct pinctrl_dev *pctldev,
> > >  {
> > >  	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,
> > > +		       unsigned int *num_maps, unsigned int *num_reserved_maps,
> > > +		       const char **group_name, unsigned int ngroups,
> > > +		       const char **functions, unsigned int *pins,
> > > +		       void *function_data)
> > > +{
> > > +	return -ENOTSUPP;
> > > +}
> > >  #endif
> > > diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c
> > > index efb39c6a670331775855efdc8566102b5c6202ef..20a216ae63e91b69985ea4cfcd0b57103c6ca950 100644
> > > --- a/drivers/pinctrl/pinctrl-generic.c
> > > +++ b/drivers/pinctrl/pinctrl-generic.c
> > > @@ -17,29 +17,18 @@
> > >  #include "pinctrl-utils.h"
> > >  #include "pinmux.h"
> > >
> > > -static int pinctrl_generic_pins_function_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)
> > > +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,
> > > +		       const char **group_names, unsigned int ngroups,
> > > +		       const char **functions, unsigned int *pins)
> >
> > npins needs to be an argument to this function also, otherwise
> > pinctrl_generic_add_group() uses it uninitialised...
> 
> Is this one the root cause of then broken?

No, this is not the cause of the breakage. I can't believe I still have
to say that. Go read the code and you'll see why that allocation thing
is problematic.

> I am not sure why compiler have not report waring for it.

It did, that's how I found it. Used uninitialised warnings are normally
from clang, so your toolchain might not have seen it. clangd integration
with my editor is how I saw it.

On Wed, Mar 25, 2026 at 07:04:12PM -0400, Frank Li wrote:

> diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c
> index efb39c6a670331775855efdc8566102b5c6202ef..20a216ae63e91b69985ea4cfcd0b57103c6ca950 100644
> --- a/drivers/pinctrl/pinctrl-generic.c
> +++ b/drivers/pinctrl/pinctrl-generic.c
> @@ -17,29 +17,18 @@
>  #include "pinctrl-utils.h"
>  #include "pinmux.h"
>  
> -static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *pctldev,

> +int
> +pinctrl_generic_to_map(struct pinctrl_dev *pctldev, struct device_node *parent,

Can you drop this stylistic change please? The 

> +		       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)
>  {
>  	struct device *dev = pctldev->dev;
> -	const char **functions;
> +	int npins, ret, reserve = 1;
> +	unsigned int num_configs;
>  	const char *group_name;
>  	unsigned long *configs;
> -	unsigned int num_configs, pin, *pins;
> -	int npins, ret, reserve = 1;
> -
> -	npins = of_property_count_u32_elems(np, "pins");
> -
> -	if (npins < 1) {
> -		dev_err(dev, "invalid pinctrl group %pOFn.%pOFn %d\n",
> -			parent, np, npins);
> -		return npins;
> -	}
>  
>  	group_name = devm_kasprintf(dev, GFP_KERNEL, "%pOFn.%pOFn", parent, np);
>  	if (!group_name)
> @@ -51,22 +40,6 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
>  	if (!pins)
>  		return -ENOMEM;

This looks suspect. You've left the pins allocation behind:
	pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
	if (!pins)
		return -ENOMEM;
but pinctrl_generic_pins_function_dt_subnode_to_map() has already
populated this array before calling the function.

Also, this should probably be
Suggested-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

>  
> -	functions = devm_kcalloc(dev, npins, sizeof(*functions), GFP_KERNEL);
> -	if (!functions)
> -		return -ENOMEM;
> -
> -	for (int i = 0; i < npins; i++) {
> -		ret = of_property_read_u32_index(np, "pins", i, &pin);
> -		if (ret)
> -			return ret;
> -
> -		pins[i] = pin;
> -
> -		ret = of_property_read_string(np, "function", &functions[i]);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	ret = pinctrl_utils_reserve_map(pctldev, maps, num_reserved_maps, num_maps, reserve);
>  	if (ret)
>  		return ret;
> @@ -103,6 +76,54 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
>  	return 0;
>  };
>  
> +static int
> +pinctrl_generic_pins_function_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 pin, *pins;
> +	const char **functions;
> +	int npins, ret;
> +
> +	npins = of_property_count_u32_elems(np, "pins");
> +
> +	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;
> +
> +	functions = devm_kcalloc(dev, npins, sizeof(*functions), GFP_KERNEL);
> +	if (!functions)
> +		return -ENOMEM;
> +
> +	for (int i = 0; i < npins; i++) {
> +		ret = of_property_read_u32_index(np, "pins", i, &pin);
> +		if (ret)
> +			return ret;
> +
> +		pins[i] = pin;
> +
> +		ret = of_property_read_string(np, "function", &functions[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return pinctrl_generic_to_map(pctldev, parent, np, maps, num_maps,
> +				      num_reserved_maps, group_names, ngroups,
> +				      functions, pins);
> +}
> +
>  /*
>   * For platforms that do not define groups or functions in the driver, but
>   * instead use the devicetree to describe them. This function will, unlike
> 
> -- 
> 2.43.0
> 

On Thu, Mar 26, 2026 at 06:52:12PM +0000, Conor Dooley wrote:
> On Wed, Mar 25, 2026 at 07:04:12PM -0400, Frank Li wrote:
> 
> > diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c
> > index efb39c6a670331775855efdc8566102b5c6202ef..20a216ae63e91b69985ea4cfcd0b57103c6ca950 100644
> > --- a/drivers/pinctrl/pinctrl-generic.c
> > +++ b/drivers/pinctrl/pinctrl-generic.c
> > @@ -17,29 +17,18 @@
> >  #include "pinctrl-utils.h"
> >  #include "pinmux.h"
> >  
> > -static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *pctldev,
> 
> > +int
> > +pinctrl_generic_to_map(struct pinctrl_dev *pctldev, struct device_node *parent,
> 
> Can you drop this stylistic change please? The 

Whoops, cut myself off. To be clear, what I am asking for is to keep the
"int" etc on the same line as the function name. This function is new,
but you did it for the existing function too and the comparison is here.

> 
> > +		       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)
> >  {
> >  	struct device *dev = pctldev->dev;
> > -	const char **functions;
> > +	int npins, ret, reserve = 1;
> > +	unsigned int num_configs;
> >  	const char *group_name;
> >  	unsigned long *configs;
> > -	unsigned int num_configs, pin, *pins;
> > -	int npins, ret, reserve = 1;
> > -
> > -	npins = of_property_count_u32_elems(np, "pins");
> > -
> > -	if (npins < 1) {
> > -		dev_err(dev, "invalid pinctrl group %pOFn.%pOFn %d\n",
> > -			parent, np, npins);
> > -		return npins;
> > -	}
> >  
> >  	group_name = devm_kasprintf(dev, GFP_KERNEL, "%pOFn.%pOFn", parent, np);
> >  	if (!group_name)
> > @@ -51,22 +40,6 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
> >  	if (!pins)
> >  		return -ENOMEM;
> 
> This looks suspect. You've left the pins allocation behind:
> 	pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
> 	if (!pins)
> 		return -ENOMEM;
> but pinctrl_generic_pins_function_dt_subnode_to_map() has already
> populated this array before calling the function.
> 
> Also, this should probably be
> Suggested-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Cheers,
> Conor.
> 
> >  
> > -	functions = devm_kcalloc(dev, npins, sizeof(*functions), GFP_KERNEL);
> > -	if (!functions)
> > -		return -ENOMEM;
> > -
> > -	for (int i = 0; i < npins; i++) {
> > -		ret = of_property_read_u32_index(np, "pins", i, &pin);
> > -		if (ret)
> > -			return ret;
> > -
> > -		pins[i] = pin;
> > -
> > -		ret = of_property_read_string(np, "function", &functions[i]);
> > -		if (ret)
> > -			return ret;
> > -	}
> > -
> >  	ret = pinctrl_utils_reserve_map(pctldev, maps, num_reserved_maps, num_maps, reserve);
> >  	if (ret)
> >  		return ret;
> > @@ -103,6 +76,54 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
> >  	return 0;
> >  };
> >  
> > +static int
> > +pinctrl_generic_pins_function_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 pin, *pins;
> > +	const char **functions;
> > +	int npins, ret;
> > +
> > +	npins = of_property_count_u32_elems(np, "pins");
> > +
> > +	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;
> > +
> > +	functions = devm_kcalloc(dev, npins, sizeof(*functions), GFP_KERNEL);
> > +	if (!functions)
> > +		return -ENOMEM;
> > +
> > +	for (int i = 0; i < npins; i++) {
> > +		ret = of_property_read_u32_index(np, "pins", i, &pin);
> > +		if (ret)
> > +			return ret;
> > +
> > +		pins[i] = pin;
> > +
> > +		ret = of_property_read_string(np, "function", &functions[i]);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return pinctrl_generic_to_map(pctldev, parent, np, maps, num_maps,
> > +				      num_reserved_maps, group_names, ngroups,
> > +				      functions, pins);
> > +}
> > +
> >  /*
> >   * For platforms that do not define groups or functions in the driver, but
> >   * instead use the devicetree to describe them. This function will, unlike
> > 
> > -- 
> > 2.43.0
> > 

On Thu, Mar 26, 2026 at 06:55:01PM +0000, Conor Dooley wrote:
> On Thu, Mar 26, 2026 at 06:52:12PM +0000, Conor Dooley wrote:
> > On Wed, Mar 25, 2026 at 07:04:12PM -0400, Frank Li wrote:
> >
> > > diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c
> > > index efb39c6a670331775855efdc8566102b5c6202ef..20a216ae63e91b69985ea4cfcd0b57103c6ca950 100644
> > > --- a/drivers/pinctrl/pinctrl-generic.c
> > > +++ b/drivers/pinctrl/pinctrl-generic.c
> > > @@ -17,29 +17,18 @@
> > >  #include "pinctrl-utils.h"
> > >  #include "pinmux.h"
> > >
> > > -static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *pctldev,
> >
> > > +int
> > > +pinctrl_generic_to_map(struct pinctrl_dev *pctldev, struct device_node *parent,
> >
> > Can you drop this stylistic change please? The
>
> Whoops, cut myself off. To be clear, what I am asking for is to keep the
> "int" etc on the same line as the function name. This function is new,
> but you did it for the existing function too and the comparison is here.
>
> >
> > > +		       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)
> > >  {
> > >  	struct device *dev = pctldev->dev;
> > > -	const char **functions;
> > > +	int npins, ret, reserve = 1;
> > > +	unsigned int num_configs;
> > >  	const char *group_name;
> > >  	unsigned long *configs;
> > > -	unsigned int num_configs, pin, *pins;
> > > -	int npins, ret, reserve = 1;
> > > -
> > > -	npins = of_property_count_u32_elems(np, "pins");
> > > -
> > > -	if (npins < 1) {
> > > -		dev_err(dev, "invalid pinctrl group %pOFn.%pOFn %d\n",
> > > -			parent, np, npins);
> > > -		return npins;
> > > -	}
> > >
> > >  	group_name = devm_kasprintf(dev, GFP_KERNEL, "%pOFn.%pOFn", parent, np);
> > >  	if (!group_name)
> > > @@ -51,22 +40,6 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
> > >  	if (!pins)
> > >  		return -ENOMEM;
> >
> > This looks suspect. You've left the pins allocation behind:
> > 	pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
> > 	if (!pins)
> > 		return -ENOMEM;
> > but pinctrl_generic_pins_function_dt_subnode_to_map() has already
> > populated this array before calling the function.

what's means?

pinctrl_generic_pins_function_dt_subnode_to_map()
{
	pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
	...
	pinctrl_generic_to_map();
}

pinctrl_generic_pins_function_dt_subnode_to_map() have not use this array.

Frank
> >
> > Also, this should probably be
> > Suggested-by: Conor Dooley <conor.dooley@microchip.com>
> >
> > Cheers,
> > Conor.
> >
> > >
> > > -	functions = devm_kcalloc(dev, npins, sizeof(*functions), GFP_KERNEL);
> > > -	if (!functions)
> > > -		return -ENOMEM;
> > > -
> > > -	for (int i = 0; i < npins; i++) {
> > > -		ret = of_property_read_u32_index(np, "pins", i, &pin);
> > > -		if (ret)
> > > -			return ret;
> > > -
> > > -		pins[i] = pin;
> > > -
> > > -		ret = of_property_read_string(np, "function", &functions[i]);
> > > -		if (ret)
> > > -			return ret;
> > > -	}
> > > -
> > >  	ret = pinctrl_utils_reserve_map(pctldev, maps, num_reserved_maps, num_maps, reserve);
> > >  	if (ret)
> > >  		return ret;
> > > @@ -103,6 +76,54 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
> > >  	return 0;
> > >  };
> > >
> > > +static int
> > > +pinctrl_generic_pins_function_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 pin, *pins;
> > > +	const char **functions;
> > > +	int npins, ret;
> > > +
> > > +	npins = of_property_count_u32_elems(np, "pins");
> > > +
> > > +	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;
> > > +
> > > +	functions = devm_kcalloc(dev, npins, sizeof(*functions), GFP_KERNEL);
> > > +	if (!functions)
> > > +		return -ENOMEM;
> > > +
> > > +	for (int i = 0; i < npins; i++) {
> > > +		ret = of_property_read_u32_index(np, "pins", i, &pin);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		pins[i] = pin;
> > > +
> > > +		ret = of_property_read_string(np, "function", &functions[i]);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return pinctrl_generic_to_map(pctldev, parent, np, maps, num_maps,
> > > +				      num_reserved_maps, group_names, ngroups,
> > > +				      functions, pins);
> > > +}
> > > +
> > >  /*
> > >   * For platforms that do not define groups or functions in the driver, but
> > >   * instead use the devicetree to describe them. This function will, unlike
> > >
> > > --
> > > 2.43.0
> > >
>
>

On Thu, Mar 26, 2026 at 03:47:06PM -0400, Frank Li wrote:
> On Thu, Mar 26, 2026 at 06:55:01PM +0000, Conor Dooley wrote:
> > On Thu, Mar 26, 2026 at 06:52:12PM +0000, Conor Dooley wrote:
> > > On Wed, Mar 25, 2026 at 07:04:12PM -0400, Frank Li wrote:
> > >
> > > > diff --git a/drivers/pinctrl/pinctrl-generic.c b/drivers/pinctrl/pinctrl-generic.c
> > > > index efb39c6a670331775855efdc8566102b5c6202ef..20a216ae63e91b69985ea4cfcd0b57103c6ca950 100644
> > > > --- a/drivers/pinctrl/pinctrl-generic.c
> > > > +++ b/drivers/pinctrl/pinctrl-generic.c
> > > > @@ -17,29 +17,18 @@
> > > >  #include "pinctrl-utils.h"
> > > >  #include "pinmux.h"
> > > >
> > > > -static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *pctldev,
> > >
> > > > +int
> > > > +pinctrl_generic_to_map(struct pinctrl_dev *pctldev, struct device_node *parent,
> > >
> > > Can you drop this stylistic change please? The
> >
> > Whoops, cut myself off. To be clear, what I am asking for is to keep the
> > "int" etc on the same line as the function name. This function is new,
> > but you did it for the existing function too and the comparison is here.
> >
> > >
> > > > +		       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)
> > > >  {
> > > >  	struct device *dev = pctldev->dev;
> > > > -	const char **functions;
> > > > +	int npins, ret, reserve = 1;
> > > > +	unsigned int num_configs;
> > > >  	const char *group_name;
> > > >  	unsigned long *configs;
> > > > -	unsigned int num_configs, pin, *pins;
> > > > -	int npins, ret, reserve = 1;
> > > > -
> > > > -	npins = of_property_count_u32_elems(np, "pins");
> > > > -
> > > > -	if (npins < 1) {
> > > > -		dev_err(dev, "invalid pinctrl group %pOFn.%pOFn %d\n",
> > > > -			parent, np, npins);
> > > > -		return npins;
> > > > -	}
> > > >
> > > >  	group_name = devm_kasprintf(dev, GFP_KERNEL, "%pOFn.%pOFn", parent, np);
> > > >  	if (!group_name)
> > > > @@ -51,22 +40,6 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
> > > >  	if (!pins)
> > > >  		return -ENOMEM;
> > >
> > > This looks suspect. You've left the pins allocation behind:
> > > 	pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
> > > 	if (!pins)
> > > 		return -ENOMEM;
> > > but pinctrl_generic_pins_function_dt_subnode_to_map() has already
> > > populated this array before calling the function.
> 
> what's means?

It means you broke my driver by not removing this allocation from
pinctrl_generic_to_map().

> 
> pinctrl_generic_pins_function_dt_subnode_to_map()
> {
> 	pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
> 	...
> 	pinctrl_generic_to_map();
> }
> 
> pinctrl_generic_pins_function_dt_subnode_to_map() have not use this array.

I have no idea what this statement means.

> 
> Frank
> > >
> > > Also, this should probably be
> > > Suggested-by: Conor Dooley <conor.dooley@microchip.com>
> > >
> > > Cheers,
> > > Conor.
> > >
> > > >
> > > > -	functions = devm_kcalloc(dev, npins, sizeof(*functions), GFP_KERNEL);
> > > > -	if (!functions)
> > > > -		return -ENOMEM;
> > > > -
> > > > -	for (int i = 0; i < npins; i++) {
> > > > -		ret = of_property_read_u32_index(np, "pins", i, &pin);
> > > > -		if (ret)
> > > > -			return ret;
> > > > -
> > > > -		pins[i] = pin;
> > > > -
> > > > -		ret = of_property_read_string(np, "function", &functions[i]);
> > > > -		if (ret)
> > > > -			return ret;
> > > > -	}
> > > > -
> > > >  	ret = pinctrl_utils_reserve_map(pctldev, maps, num_reserved_maps, num_maps, reserve);
> > > >  	if (ret)
> > > >  		return ret;
> > > > @@ -103,6 +76,54 @@ static int pinctrl_generic_pins_function_dt_subnode_to_map(struct pinctrl_dev *p
> > > >  	return 0;
> > > >  };
> > > >
> > > > +static int
> > > > +pinctrl_generic_pins_function_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 pin, *pins;
> > > > +	const char **functions;
> > > > +	int npins, ret;
> > > > +
> > > > +	npins = of_property_count_u32_elems(np, "pins");
> > > > +
> > > > +	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;
> > > > +
> > > > +	functions = devm_kcalloc(dev, npins, sizeof(*functions), GFP_KERNEL);
> > > > +	if (!functions)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	for (int i = 0; i < npins; i++) {
> > > > +		ret = of_property_read_u32_index(np, "pins", i, &pin);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		pins[i] = pin;
> > > > +
> > > > +		ret = of_property_read_string(np, "function", &functions[i]);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	return pinctrl_generic_to_map(pctldev, parent, np, maps, num_maps,
> > > > +				      num_reserved_maps, group_names, ngroups,
> > > > +				      functions, pins);
> > > > +}
> > > > +
> > > >  /*
> > > >   * For platforms that do not define groups or functions in the driver, but
> > > >   * instead use the devicetree to describe them. This function will, unlike
> > > >
> > > > --
> > > > 2.43.0
> > > >
> >
> >
> 
>