Re: [PATCH v3 2/2] pinctrl: ultrarisc: Add UltraRISC DP1000 pinctrl driver

[Linus Walleij]

Hi Jia,

thanks for your patch!

On Mon, Jun 8, 2026 at 9:50 AM Jia Wang via B4 Relay
<devnull+wangjia.ultrarisc.com@xxxxxxxxxx> wrote:

> From: Jia Wang <wangjia@xxxxxxxxxxxxx>
>
> Add support for the pin controller on the UltraRISC DP1000 SoC.
>
> The controller provides mux selection for pins in ports A, B, C, D, and
> LPC. Ports A-D default to GPIO and support peripheral muxing. LPC pins
> can be switched to eSPI, but are not available as GPIOs. Basic pin
> configuration controls such as drive strength, pull-up, and pull-down
> are also supported.
>
> Signed-off-by: Jia Wang <wangjia@xxxxxxxxxxxxx>

Overall this looks very good, some things to fix up below but nothing
major!

(...)
> +config PINCTRL_ULTRARISC
> +       tristate
> +       depends on OF
> +       depends on ARCH_ULTRARISC || COMPILE_TEST
> +       select GENERIC_PINCTRL
> +       select PINMUX
> +       select GPIOLIB

Why GPIOLIB? You don't implement any GPIO chips...

> +       raw_spin_lock_irqsave(&pctrl->lock, flags);
> +       val = readl_relaxed(reg);
> +       val = (val & ~mask) | field_prep(mask, conf);
> +       writel_relaxed(val, reg);
> +       raw_spin_unlock_irqrestore(&pctrl->lock, flags);

Have you thought about using a scoped guard for this lock?
It will make the code easier to read.
(Applies everywhere.)

> +static int ur_find_group_route(struct ur_pinctrl *pctrl,
> +                              const char *function,
> +                              u64 group_mask,
> +                              const struct ur_func_route **route_out)
> +{
> +       const struct ur_func_route *match = NULL;
> +
> +       for (u32 i = 0; i < pctrl->data->num_routes; i++) {
> +               const struct ur_func_route *route = &pctrl->data->routes[i];
> +
> +               if (strcmp(route->function, function))
> +                       continue;
> +
> +               if ((route->valid_pins & group_mask) != group_mask)
> +                       continue;
> +
> +               if (match) {
> +                       dev_err(pctrl->dev,
> +                               "ambiguous route for function %s group_mask=%#llx\n",
> +                               function, (unsigned long long)group_mask);
> +                       return -EINVAL;
> +               }
> +
> +               match = route;
> +       }
> +
> +       if (match) {
> +               *route_out = match;
> +               return 0;
> +       }
> +
> +       return -EINVAL;
> +}

This routing function needs some kind of comment before it explaining
what is going on and what constraints you are trying to resolve with this.

> +static bool ur_function_is_gpio(struct pinctrl_dev *pctldev,
> +                               unsigned int selector)

Neat that you implement this!

> +static const struct pinctrl_ops ur_pinctrl_ops = {
> +       .get_groups_count = pinctrl_generic_get_group_count,
> +       .get_group_name = pinctrl_generic_get_group_name,
> +       .get_group_pins = pinctrl_generic_get_group_pins,
> +       .dt_node_to_map = pinctrl_generic_pins_function_dt_node_to_map,
> +       .dt_free_map = pinconf_generic_dt_free_map,
> +};

Good use of generic helpers!

> +static const struct pinmux_ops ur_pinmux_ops = {
> +       .get_functions_count = pinmux_generic_get_function_count,
> +       .get_function_name = pinmux_generic_get_function_name,
> +       .get_function_groups = pinmux_generic_get_function_groups,
> +       .function_is_gpio = ur_function_is_gpio,
> +       .set_mux = ur_set_mux,
> +       .gpio_request_enable = ur_gpio_request_enable,
> +       .strict = true,
> +};

Here too.

> +static const struct pinconf_ops ur_pinconf_ops = {
> +       .pin_config_get = ur_pin_config_get,
> +       .pin_config_set = ur_pin_config_set,
> +       .pin_config_group_get = ur_pin_config_group_get,
> +       .pin_config_group_set = ur_pin_config_group_set,
> +#ifdef CONFIG_GENERIC_PINCONF
> +       .is_generic = true,
> +       .pin_config_config_dbg_show = pinconf_generic_dump_config,
> +#endif

Why ifdef:ed? Just select it in your Kconfig and rely on it?

Yours,
Linus Walleij