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

Frank Li
2026-03-26T19:47:18Z

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