Re: [PATCH v4 3/7] pinctrl: extract pinctrl_generic_to_map() from pinctrl_generic_pins_function_dt_node_to_map()

[Conor Dooley]

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@xxxxxxx>
> ---
> 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
>

Attachment: signature.asc
Description: PGP signature