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

[Jia Wang]

On 2026-06-09 00:23 +0200, Linus Walleij wrote:
> 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!
>

Thanks for the review.
 
> (...)
> > +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...
>

Good catch. GPIOLIB is not actually needed here since the driver does not
provide any GPIO functionality. I'll drop the select in the next version.
 
> > +       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.)
>

Yes, that makes sense. I'll convert the lock sections to use scoped_guard()
throughout the driver in the next version.
 
> > +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.
> 

Agreed. I'll add a comment describing the routing model and the constraints
being resolved here.

> > +static bool ur_function_is_gpio(struct pinctrl_dev *pctldev,
> > +                               unsigned int selector)
> 
> Neat that you implement this!
>

Thanks.
 
> > +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?
> 

GENERIC_PINCONF is already selected via GENERIC_PINCTRL, so the dependency
is guaranteed. The ifdef is therefore unnecessary; I'll remove it in the
next version.

> Yours,
> Linus Walleij
> 

Best regards,
Jia Wang