Re: [PATCH 6/9] pinctrl: ultrarisc: Add UltraRISC DP1000 pinctrl driver

[Jia Wang <wangjia@ultrarisc.com>]

On 2026-05-25 11:10 +0100, Conor Dooley wrote:
> On Mon, May 25, 2026 at 11:28:51AM +0200, Linus Walleij wrote:
> > On Fri, May 15, 2026 at 3:18 AM Jia Wang via B4 Relay
> > <devnull+wangjia.ultrarisc.com@kernel.org> wrote:
> 
> > > +static int ur_legacy_parse_prop(struct pinctrl_dev *pctldev,
> > > +                               struct device_node *np,
> > > +                               const char *propname,
> > > +                               struct ur_legacy_prop_data *prop)
> > > +static const char *ur_legacy_get_function_name(const struct ur_pinctrl_match_data *match_data,
> > > +                                              u32 mode)
> > > +static int ur_legacy_conf_to_configs(struct pinctrl_dev *pctldev, u32 conf,
> > > +                                    unsigned long **configs,
> > > +                                    unsigned int *num_configs)
> > > +static int ur_legacy_add_pinconf_maps(struct pinctrl_dev *pctldev,
> > > +                                     struct pinctrl_map **map,
> > > +                                     unsigned int *reserved_maps,
> > > +                                     unsigned int *num_maps,
> > > +                                     const struct ur_legacy_prop_data *prop)
> > > +static int ur_legacy_dt_node_to_map(struct pinctrl_dev *pctldev,
> > > +                                   struct device_node *np,
> > > +                                   struct pinctrl_map **map,
> > > +                                   unsigned int *num_maps)
> > 
> > What's up with all this legacy stuff?
> > 
> > What is this a legacy of?
> > 
> > I thought this was a *new* driver so how can it be "legacy"?
> 
> Jia already agreed to drop this stuff :)
> 
> > 
> > > +static int ur_generic_dt_node_to_map(struct pinctrl_dev *pctldev,
> > > +                                    struct device_node *np_config,
> > > +                                    struct pinctrl_map **map,
> > > +                                    unsigned int *num_maps)
> > > +{
> > > +       return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps,
> > > +                                             PIN_MAP_TYPE_INVALID);
> > > +}
> > 
> > Hm I think Conor has new helpers for this so you don't need to wrap
> > it like this.
> 
> Yeah, although "more importantly" the new helpers mean that drivers
> shouldn't need to do things like calling these home-rolled functions in
> probe, just so that they can try to use pinconf_generic_dt_node_to_map():
> | static int ur_add_pin_groups(struct ur_pinctrl *pctldata)
> | {
> | 	for (u32 i = 0; i < pctldata->match_data->npins; i++) {
> | 		int ret;
> | 
> | 		pctldata->group_names[i] = pctldata->match_data->pins[i].name;
> | 		pctldata->group_pins[i] = pctldata->match_data->pins[i].number;
> | 
> | 		ret = pinctrl_generic_add_group(pctldata->pctl_dev, pctldata->group_names[i],
> | 						&pctldata->group_pins[i], 1, NULL);
> | 		if (ret < 0)
> | 			return dev_err_probe(pctldata->dev, ret,
> | 					     "failed to add pin group %s\n",
> | 					     pctldata->group_names[i]);
> | 	}
> | 
> | 	return 0;
> | }
> | 
> | static int ur_add_functions(struct ur_pinctrl *pctldata)
> | {
> | 	for (u32 i = 0; i < pctldata->match_data->num_functions; i++) {
> | 		const struct ur_function_desc *desc = &pctldata->match_data->functions[i];
> | 		struct pinfunction func = desc->gpio ?
> | 			PINCTRL_GPIO_PINFUNCTION(desc->name, pctldata->group_names,
> | 						 pctldata->match_data->npins) :
> | 			PINCTRL_PINFUNCTION(desc->name, pctldata->group_names,
> | 					    pctldata->match_data->npins);
> | 		int ret;
> | 
> | 		ret = pinmux_generic_add_pinfunction(pctldata->pctl_dev, &func, (void *)desc);
> | 		if (ret < 0)
> | 			return dev_err_probe(pctldata->dev, ret,
> | 					     "failed to add function %s\n",
> | 					     desc->name);
> | 	}
> | 
> | 	return 0;
> | }
> (If I had more time, I would probably go looking to see if there are
> more candidates for conversion)
> 
> Jia, the helper in question is pinctrl_generic_pins_functions_dt_node_to_map().
>

I found pinctrl_generic_pins_function_dt_node_to_map() in mainline, but
there is no pinctrl_generic_pins_functions_dt_node_to_map() as mentioned.
Did you mean the singular version, and is that the one I should use?

> Cheers,
> Conor.
> 
> Also worth pointing out that this driver has match data but only one
> supported device:
> +static const struct of_device_id ur_pinctrl_of_match[] = {
> +	{ .compatible = "ultrarisc,dp1000-pinctrl", .data = &ur_dp1000_match_data, },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ur_pinctrl_of_match);
> 

Ack, I'll remove the match data since there's only one compatible for now.

Best regards,
Jia Wang