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

From: Heiko Stübner <[email-masked]>
Date: Fri, 23 Jan 2026 22:52:37 +0100

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 <[email-masked]> 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?

Heiko