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

[Frank Li]

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