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

[Conor Dooley <conor@kernel.org>]

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