Re: [PATCH v5 2/2] mfd: arizona: Convert GPIO IRQ handling to descriptors

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Thu, Jun 11, 2026 at 09:20:00AM +0100, Lee Jones wrote:
> On Wed, 27 May 2026, Linus Walleij wrote:
> > -			   gpio_get_value_cansleep(arizona->pdata.irq_gpio)) {
> > +			   gpiod_get_value_cansleep(arizona->irq_gpiod)) {
> 
> [Severity: High]
> Does this incorrectly check the logical value of the GPIO instead of the raw
> physical state?
> 
> The legacy gpio_get_value_cansleep() returned the raw physical state of the
> pin. The new gpiod_get_value_cansleep() returns the logical state, which
> incorporates the ACTIVE_LOW flag from the devicetree or software node.
> 
> Because the IRQ trigger flags describe physical voltage transitions, comparing
> these physical flags against a logical value might be incorrect. For example,
> if a user defines irq-gpios with GPIO_ACTIVE_LOW for a falling edge interrupt,
> a physically low line becomes logically 1, causing the polling condition
> !gpiod_get_value_cansleep() to evaluate to 0, which fails to poll when the
> interrupt is active.
> 
> Conversely, when the line is idle, it evaluates to 1, causing the IRQ handler
> to poll infinitely.
> 
> Should this use gpiod_get_raw_value_cansleep() to preserve the original
> physical semantics?

Hmm.. yeah that is a good spot we should probably use the
raw_value version here as we presumably can't trust that the
device trees specify the ACTIVE_LOW/HIGH flags correctly.

> [Severity: High]
> This is a pre-existing issue, but can this loop forever if the GPIO read
> returns a negative error code?
> 
> gpiod_get_value_cansleep() (and its legacy predecessor) can return a negative
> error code (e.g., -EIO) if reading the GPIO state fails over the bus. Since
> negative values are non-zero, this evaluates to true for IRQF_TRIGGER_RISING,
> setting poll = true and causing an infinite loop in the IRQ handler thread if
> the read continues to fail.
> 
> Conversely, for IRQF_TRIGGER_FALLING, !(-EIO) evaluates to false, which
> immediately aborts the poll.
> 
> Could the return value be stored in an integer and explicitly checked for
> negative errors before evaluating it as a boolean?

This is also fair, although given this feature is only really
supporting some quite old things that have been doing this for
ages I would be inclined to not stress too much about this.

Thanks,
Charles