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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5910 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]