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

[Conor Dooley]

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

Attachment: signature.asc
Description: PGP signature