Re: [PATCH] gpio: rockchip: mark the GPIO controller as sleeping

[Robin Murphy <robin.murphy@arm.com>]

On 2026-01-23 9:52 pm, Heiko Stübner wrote:
> Am Freitag, 23. Januar 2026, 21:57:50 Mitteleuropäische Normalzeit schrieb Robin Murphy:
>> On 2026-01-23 7:27 pm, Bartosz Golaszewski wrote:
>>> On Fri, Jan 23, 2026 at 2:27 PM Robin Murphy <robin.murphy@arm.com> wrote:
>>>>
>>>>>>
>>>>>> It's not a big issue for the hdmirx driver specifically, but I wonder
>>>>>> how many more (less often tested) rockchip drivers use GPIOs from their
>>>>>> IRQ handler.
>>>>
>>>> Yeah, seems this finally reached my distro kernel and now the kernel log
>>>> on one of my boards is totally flooded from gpio_ir_recv_irq()
>>>> (legitimately) calling gpio_get_value()... that's not really OK :/
>>>>
>>>
>>> This has always been a sleeping driver. The driver does not know the
>>> firmware configuration it'll be passed and - as I explained above -
>>> depending on the lookup flags, we may call .direction_output() and
>>> descend into pinctrl which uses mutexes. Ideally, we'd make
>>> GPIO-facing pinctrl operations not sleeping but this is a long-time
>>> project and quite complex. Telling the GPIO core that it cannot sleep
>>> is simply incorrect - even if it worked for this particular use-case -
>>> and has an impact on paths we're choosing.
>>>
>>> Can the GPIO reading in the gpio-ir-recv driver be done from a
>>> high-priority workqueue by any chance? Or can we make it a threaded
>>> interrupt?
>>
>> rockchip_gpio_get() is essentially nothing but a readl(), please explain
>> how that could sleep? Saying that countless in-tree and out-of-tree
>> arbitrary GPIO consumer drivers should pointlessly refactor just to
>> avoid the GPIO core spewing spurious WARN()s is not reasonable.
>>
>> I appreciate there are cases where the warning most definitely *is*
>> relevant, which is why I picked up this discussion rather than proposing
>> a revert, even though the documentation says:
>>
>>    * @can_sleep: flag must be set iff get()/set() methods sleep, as they
>>
>> where since neither rockchip_gpio_get() nor rockchip_gpio_set()
>> themselves sleep, apparently this flag must *not* be set. It's
>> irrelevant that a higher-level gpiod_set_value() invocation might end up
>> calling .set_direction before it gets as far as calling .set - that's
>> not the gpio_chip's fault, and gpiolib knows exactly what it's doing.
>>
>> What I'm getting at is that getting, and even (directly) setting a
>> GPIO's value can reasonably be expected to be more common and applicable
>> in a wider range of circumstances than changing its configuration, so
>> the former should not be unfairly penalised because of the latter. This
>> case is clearly distinct from external GPIO expanders on buses that
>> fundamentally can't guarantee fast memory-mapped access at all, so
>> trying to conflate it under the same flag doesn't fit, and that needs
>> fixing ASAP, before the reverts *do* start piling in. Maybe that just
>> means some new dir_can_sleep or more abstract dir_needs_pinctrl flag
>> might suffice, maybe it's something more involved; I don't really know,
>> and I don't have the bandwidth to take this on myself.
> 
> the sadest part here is, that the dive from gpio to pinctrl is sort of
> a remant of the past. Originally it was meant to "automatically" set
> the gpio pinmux, but nowadays we (at least try to) have pinctrl entries
> doing that separately.
> 
> But of course that's hard to enforce now, because we can't really know
> where gpios without pinctrl might hide, and of course newer kernels
> need to still run with old DTBs.
> 
> But...
> 
> rockchip_pmx_gpio_set_direction()'s only function is to set the GPIO
> pinmux - it does not handle the actual the actual direction.
> 
> Can't we move the pinctrl_gpio_direction_input/_output() call just over
> to the request callback of the gpiochip?

In fact, after an hour or so chasing through the code, is that not just
pretty much this? (Not even compile tested as I'd rather go to bed now...)

Cheers,
Robin.

----->8-----
diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 47174eb3ba76..118edd57c252 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -164,12 +164,6 @@ static int rockchip_gpio_set_direction(struct gpio_chip *chip,
  	unsigned long flags;
  	u32 data = input ? 0 : 1;
  
-
-	if (input)
-		pinctrl_gpio_direction_input(chip, offset);
-	else
-		pinctrl_gpio_direction_output(chip, offset);
-
  	raw_spin_lock_irqsave(&bank->slock, flags);
  	rockchip_gpio_writel_bit(bank, offset, data, bank->gpio_regs->port_ddr);
  	raw_spin_unlock_irqrestore(&bank->slock, flags);
diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index e44ef262beec..2fc67aeafdb3 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -3545,10 +3545,9 @@ static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
  	return 0;
  }
  
-static int rockchip_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
-					   struct pinctrl_gpio_range *range,
-					   unsigned offset,
-					   bool input)
+static int rockchip_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
+					    struct pinctrl_gpio_range *range,
+					    unsigned int offset)
  {
  	struct rockchip_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
  	struct rockchip_pin_bank *bank;
@@ -3562,7 +3561,7 @@ static const struct pinmux_ops rockchip_pmx_ops = {
  	.get_function_name	= rockchip_pmx_get_func_name,
  	.get_function_groups	= rockchip_pmx_get_groups,
  	.set_mux		= rockchip_pmx_set,
-	.gpio_set_direction	= rockchip_pmx_gpio_set_direction,
+	.gpio_request_enable	= rockchip_pmx_gpio_request_enable,
  };
  
  /*