Re: [PATCH v3 04/21] pinctrl: starfive: Add StarFive JHB100 sys0 controller driver

Hi, linus

Thanks for the review.

> On Thu, Jul 30, 2026 at 12:58 PM Changhuang Liang
> <[email protected]> wrote:
> 
> > > If a pin controller back-end is used, the GPIO controller or
> > > hardware description needs to provide "GPIO ranges" mapping the GPIO
> > > line offsets to pin numbers on the pin controller so they can
> > > properly cross-reference each other."
> >
> > I tried this change, but it doesn't work. In my new version, the GPIO
> > direction is set via the `struct pinmux_ops .gpio_set_direction`
> > callback, which is executed after `mutex_lock(&pctldev->mutex);`. I
> > need to configure some pinconf settings while setting the GPIO direction
> inside `.gpio_set_direction`, for example:
> >
> >     config = pinconf_to_config_packed(PIN_CONFIG_INPUT_ENABLE, 0);
> >     ret = pinctrl_gpio_set_config(gc, gpio, config);
> >
> > However, `pinctrl_gpio_set_config` will again acquire the lock with
> > `mutex_lock(&pctldev->mutex);`.
> >
> > So this approach may no longer work?
> 
> I can't see all your code so I don't know exactly why this happens, but
> nominally you implement the GPIO helpers:
> 
> struct pinmux_ops {
> (...)
>         int (*gpio_request_enable) (struct pinctrl_dev *pctldev,
>                                     struct pinctrl_gpio_range
> *range,
>                                     unsigned int offset);
>         void (*gpio_disable_free) (struct pinctrl_dev *pctldev,
>                                    struct pinctrl_gpio_range *range,
>                                    unsigned int offset);
>         int (*gpio_set_direction) (struct pinctrl_dev *pctldev,
>                                    struct pinctrl_gpio_range *range,
>                                    unsigned int offset,
>                                    bool input);
> 
> Then on the GPIO side:
> 
> static int my_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> {
>         return pinctrl_gpio_direction_input(chip, offset); }
> 
> static int my_gpio_direction_output(struct gpio_chip *chip, unsigned int
> offset,
>                                        int value) {
>         int ret;
> 
>         ret = my_gpio_set(chip, offset, value);
>         if (ret)
>                 return ret;
> 
>         return pinctrl_gpio_direction_output(chip, offset); }
> 
> static const struct gpio_chip my_gpio_chip = {
>         .direction_input = my_gpio_direction_input,
>         .direction_output = my_gpio_direction_output,
>         .set_config = gpiochip_generic_config, };
> 
> And these will call into the pin controller backend for you, so you do not need
> to set this yourself?

Copy from v4: -----------:

static int jhb100_gpio_direction_input(struct gpio_chip *gc,
				       unsigned int gpio)
{
	struct jhb100_gpio_bank *bank = jhb100_gc_to_bank(gc);
	struct jhb100_pinctrl *sfp = gpiochip_get_data(gc);
	const struct jhb100_pinctrl_domain_info *info = sfp->info;
	const struct config_reg_layout_desc *crl_desc;
	unsigned int pin = jhb100_gpio_to_pin(gc, gpio);
	unsigned int offset = 4 * bank->id;
	void __iomem *reg_gpio_oen;
	u32 doen = 0;

	crl_desc = get_crl_desc_by_pin(sfp, pin);
	if (!crl_desc) {
		dev_err(sfp->dev, "pin %d can't not found reg layout descriptor\n",
			pin);
		return -EINVAL;
	}

	jhb100_padcfg_rmw(sfp, pin,
			  RL_DESC_GENMASK(crl_desc, PAD_CFG_INPUT_ENABLE) |
			  RL_DESC_GENMASK(crl_desc, PAD_CFG_SCHMITT_TRIGGER_SELECT),
			  RL_DESC_GENMASK(crl_desc, PAD_CFG_INPUT_ENABLE) |
			  RL_DESC_GENMASK(crl_desc, PAD_CFG_SCHMITT_TRIGGER_SELECT));

	reg_gpio_oen = sfp->base + info->regs->output_en + offset;

	guard(raw_spinlock_irqsave)(&sfp->lock);
	doen = readl_relaxed(reg_gpio_oen) | BIT(gpio);
	writel_relaxed(doen, reg_gpio_oen);

	return 0;
}

static int jhb100_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio)
{
	struct jhb100_gpio_bank *bank = jhb100_gc_to_bank(gc);
	struct jhb100_pinctrl *sfp = gpiochip_get_data(gc);
	const struct jhb100_pinctrl_domain_info *info = sfp->info;
	const struct config_reg_layout_desc *crl_desc;
	unsigned int pin = jhb100_gpio_to_pin(gc, gpio);
	unsigned int offset = 4 * bank->id;
	void __iomem *reg_gpio_oen;
	u32 doen = 0;

	crl_desc = get_crl_desc_by_pin(sfp, pin);
	if (!crl_desc) {
		dev_err(sfp->dev, "pin %d can't not found reg layout descriptor\n",
			pin);
		return -EINVAL;
	}

	jhb100_padcfg_rmw(sfp, pin,
			  RL_DESC_GENMASK(crl_desc, PAD_CFG_INPUT_ENABLE) |
			  RL_DESC_GENMASK(crl_desc, PAD_CFG_SCHMITT_TRIGGER_SELECT) |
			  RL_DESC_GENMASK(crl_desc, PAD_CFG_PULL_DOWN) |
			  RL_DESC_GENMASK(crl_desc, PAD_CFG_PULL_UP),
			  0);

	reg_gpio_oen = sfp->base + info->regs->output_en + offset;

	guard(raw_spinlock_irqsave)(&sfp->lock);
	doen = readl_relaxed(reg_gpio_oen) & ~BIT(gpio);
	writel_relaxed(doen, reg_gpio_oen);

	return 0;
}

static int jhb100_gpio_set_direction(struct pinctrl_dev *pctldev,
				     struct pinctrl_gpio_range *range,
				     unsigned int pin,
				     bool input)
{
	struct jhb100_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
	unsigned int id = pin / JHB100_NR_GPIOS_PER_BANK;
	unsigned int gpio = pin % JHB100_NR_GPIOS_PER_BANK;

	if (input)
		return jhb100_gpio_direction_input(&sfp->banks[id].chip.gc, gpio);

	return jhb100_gpio_direction_output(&sfp->banks[id].chip.gc, gpio);
}

static const struct pinmux_ops jhb100_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,
	.set_mux		= jhb100_set_mux,
	.gpio_request_enable	= jhb100_gpio_request_enable,
	.gpio_set_direction	= jhb100_gpio_set_direction,
};

Yes, I sent the new version as above. However, I still kept jhb100_gpio_to_pin() in jhb100_gpio_direction_input(). 
The first time I wanted to replace this part:

-----------------------------------------------------------------------------
unsigned int pin = jhb100_gpio_to_pin(gc, gpio);

jhb100_padcfg_rmw(sfp, pin,
          RL_DESC_GENMASK(crl_desc, PAD_CFG_INPUT_ENABLE) |
          RL_DESC_GENMASK(crl_desc, PAD_CFG_SCHMITT_TRIGGER_SELECT),
          RL_DESC_GENMASK(crl_desc, PAD_CFG_INPUT_ENABLE) |
          RL_DESC_GENMASK(crl_desc, PAD_CFG_SCHMITT_TRIGGER_SELECT));
-----------------------------------------------------------------------------
with
------------------------------------------------------------------------------
config = pinconf_to_config_packed(PIN_CONFIG_INPUT_ENABLE, 1);
ret = pinctrl_gpio_set_config(gc, gpio, config);

config = pinconf_to_config_packed(PAD_CFG_SCHMITT_TRIGGER_SELECT, 1);
ret = pinctrl_gpio_set_config(gc, gpio, config);
-------------------------------------------------------------------------------

However, during testing, I found a recursive lock issue:

pinctrl_gpio_direction()
  mutex_lock(&pctldev->mutex);
  -> pinmux_gpio_direction()
   -> ops->gpio_set_direction
	-> jhb100_gpio_set_direction()
     -> jhb100_gpio_direction_input()
	  -> pinctrl_gpio_set_config()
       -> pinctrl_get_device_gpio_range()
        -> pinctrl_match_gpio_range()
         -> mutex_lock(&pctldev->mutex);  ====>  Executes mutex_lock(&pctldev->mutex); again
  mutex_unlock(&pctldev->mutex);
-------------------------------------

So in v4, I temporarily kept jhb100_gpio_to_pin().

Best Regards,
Changhuang