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

[Lee Jones <lee@kernel.org>]

Couple of Sashiko issues I'd like you to comment on please buddy.

On Wed, 27 May 2026, Linus Walleij wrote:

> Convert the arizona polling GPIO handling to use a GPIO descriptor
> instead of passing a global GPIO number as platform data.
> 
> This mechanism is not used in the kernel, but let's preserve
> the mechanism to be nice.
> 
> Users can define "irq-gpios" in the devicetree or software node
> for the Arizona chip to provide the GPIO line corresponding to
> the IRQ.
> 
> Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
>  drivers/mfd/arizona-irq.c         | 50 +++++++++++++++++++++------------------
>  include/linux/mfd/arizona/core.h  |  2 ++
>  include/linux/mfd/arizona/pdata.h |  5 ----
>  3 files changed, 29 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
> index 544016d420fe..cf8e42662585 100644
> --- a/drivers/mfd/arizona-irq.c
> +++ b/drivers/mfd/arizona-irq.c
> @@ -136,21 +136,19 @@ static irqreturn_t arizona_irq_thread(int irq, void *data)
>  			dev_err(arizona->dev,
>  				"Failed to read main IRQ status: %d\n", ret);
>  		}
> -#ifdef CONFIG_GPIOLIB_LEGACY
>  		/*
>  		 * Poll the IRQ pin status to see if we're really done
>  		 * if the interrupt controller can't do it for us.
>  		 */
> -		if (!arizona->pdata.irq_gpio) {
> +		if (!arizona->irq_gpiod) {
>  			break;
>  		} else if (arizona->pdata.irq_flags & IRQF_TRIGGER_RISING &&
> -			   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?



[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?


>  			poll = true;
>  		} else if (arizona->pdata.irq_flags & IRQF_TRIGGER_FALLING &&
> -			   !gpio_get_value_cansleep(arizona->pdata.irq_gpio)) {
> +			   !gpiod_get_value_cansleep(arizona->irq_gpiod)) {
>  			poll = true;
>  		}
> -#endif
>  	} while (poll);
>  
>  	pm_runtime_put_autosuspend(arizona->dev);
> @@ -210,6 +208,7 @@ int arizona_irq_init(struct arizona *arizona)
>  	const struct regmap_irq_chip *aod, *irq;
>  	struct irq_data *irq_data;
>  	unsigned int virq;
> +	int gpio_irq;
>  
>  	arizona->ctrlif_error = true;
>  
> @@ -350,27 +349,32 @@ int arizona_irq_init(struct arizona *arizona)
>  		goto err_map_main_irq;
>  	}
>  
> -#ifdef CONFIG_GPIOLIB_LEGACY
> -	/* Used to emulate edge trigger and to work around broken pinmux */
> -	if (arizona->pdata.irq_gpio) {
> -		if (gpio_to_irq(arizona->pdata.irq_gpio) != arizona->irq) {
> -			dev_warn(arizona->dev, "IRQ %d is not GPIO %d (%d)\n",
> -				 arizona->irq, arizona->pdata.irq_gpio,
> -				 gpio_to_irq(arizona->pdata.irq_gpio));
> -			arizona->irq = gpio_to_irq(arizona->pdata.irq_gpio);
> +	/*
> +	 * Used to emulate edge trigger and to work around broken pinmux
> +	 * define "irq-gpios" in device tree or software node.
> +	 */
> +	arizona->irq_gpiod = devm_gpiod_get_optional(arizona->dev, "irq",
> +						     GPIOD_IN);
> +	if (IS_ERR(arizona->irq_gpiod)) {
> +		ret = dev_err_probe(arizona->dev,
> +				    PTR_ERR(arizona->irq_gpiod),
> +				    "Error getting IRQ GPIO\n");
> +		goto err_main_irq;
> +	}
> +	if (arizona->irq_gpiod) {
> +		gpio_irq = gpiod_to_irq(arizona->irq_gpiod);
> +		if (gpio_irq < 0) {
> +			ret = dev_err_probe(arizona->dev, gpio_irq,
> +					    "GPIO IRQ line is invalid\n");
> +			goto err_main_irq;
>  		}
> -
> -		ret = devm_gpio_request_one(arizona->dev,
> -					    arizona->pdata.irq_gpio,
> -					    GPIOF_IN, "arizona IRQ");
> -		if (ret != 0) {
> -			dev_err(arizona->dev,
> -				"Failed to request IRQ GPIO %d:: %d\n",
> -				arizona->pdata.irq_gpio, ret);
> -			arizona->pdata.irq_gpio = 0;
> +		if (gpio_irq != arizona->irq) {
> +			dev_warn(arizona->dev, "IRQ %d does not match GPIO's IRQ %d\n",
> +				 arizona->irq, gpio_irq);
> +			arizona->irq = gpio_irq;
>  		}
> +		gpiod_set_consumer_name(arizona->irq_gpiod, "arizona IRQ");
>  	}
> -#endif
>  
>  	ret = request_threaded_irq(arizona->irq, NULL, arizona_irq_thread,
>  				   flags, "arizona", arizona);
> diff --git a/include/linux/mfd/arizona/core.h b/include/linux/mfd/arizona/core.h
> index 6d6f96b2b29f..7129651c9af6 100644
> --- a/include/linux/mfd/arizona/core.h
> +++ b/include/linux/mfd/arizona/core.h
> @@ -11,6 +11,7 @@
>  #define _WM_ARIZONA_CORE_H
>  
>  #include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/interrupt.h>
>  #include <linux/notifier.h>
>  #include <linux/regmap.h>
> @@ -136,6 +137,7 @@ struct arizona {
>  	struct irq_domain *virq;
>  	struct regmap_irq_chip_data *aod_irq_chip;
>  	struct regmap_irq_chip_data *irq_chip;
> +	struct gpio_desc *irq_gpiod;
>  
>  	bool hpdet_clamp;
>  	unsigned int hp_ena;
> diff --git a/include/linux/mfd/arizona/pdata.h b/include/linux/mfd/arizona/pdata.h
> index d465dcd8c90a..2f980d51cdfa 100644
> --- a/include/linux/mfd/arizona/pdata.h
> +++ b/include/linux/mfd/arizona/pdata.h
> @@ -178,11 +178,6 @@ struct arizona_pdata {
>  	/** Haptic actuator type */
>  	unsigned int hap_act;
>  
> -#ifdef CONFIG_GPIOLIB_LEGACY
> -	/** GPIO for primary IRQ (used for edge triggered emulation) */
> -	int irq_gpio;
> -#endif
> -
>  	/** General purpose switch control */
>  	unsigned int gpsw;
>  };
> 
> -- 
> 2.54.0
> 

-- 
Lee Jones