Re: [PATCH] mfd: arizona: Check if AOD interrupts are pending before dispatching

[Lee Jones <lee.jones@linaro.org>]

On Thu, 14 Apr 2016, Richard Fitzgerald wrote:

> Previously the arizona_irq_thread implementation would call
> handle_nested_irqs() to handle AOD interrupts without checking if any
> were actually pending. The kernel will see these as spurious IRQs and
> will eventually disable the IRQ.
> 
> This patch ensures we only launch the nested handler if there are AOD
> interrupts pending in the codec.
> 
> Signed-off-by: Simon Trimmer <simont@opensource.wolfsonmicro.com>
> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-irq.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
> index edeb495..edaf592 100644
> --- a/drivers/mfd/arizona-irq.c
> +++ b/drivers/mfd/arizona-irq.c
> @@ -97,7 +97,7 @@ static irqreturn_t arizona_irq_thread(int irq, void *data)
>  {
>  	struct arizona *arizona = data;
>  	bool poll;
> -	unsigned int val;
> +	unsigned int val, nest_irq;
>  	int ret;
>  
>  	ret = pm_runtime_get_sync(arizona->dev);
> @@ -109,8 +109,23 @@ static irqreturn_t arizona_irq_thread(int irq, void *data)
>  	do {
>  		poll = false;
>  
> -		if (arizona->aod_irq_chip)
> -			handle_nested_irq(irq_find_mapping(arizona->virq, 0));
> +		if (arizona->aod_irq_chip) {
> +			/*
> +			 * Check the AOD status register to determine whether
> +			 * the nested IRQ handler should be called.
> +			 */
> +			ret = regmap_read(arizona->regmap,
> +					  ARIZONA_AOD_IRQ1,
> +					  &val);

Nit: Pop this on the line above.

> +			if (ret == 0 && val != 0) {
> +				nest_irq = irq_find_mapping(arizona->virq, 0);
> +				handle_nested_irq(nest_irq);
> +			} else if (ret != 0) {
> +				dev_err(arizona->dev,

It's only a valid 'error' if you're return an error condition.
AFAICT, you're not doing that, so use dev_warn() instead.

> +					"Failed to read AOD IRQ1 %d\n",
> +					ret);

Nit: Pop this on the line above.

> +			}
> +		}

I think the whole thing would be better written like:

		if (arizona->aod_irq_chip) {
			/*
			 * Check the AOD status register to determine whether
			 * the nested IRQ handler should be called.
			 */
			ret = regmap_read(arizona->regmap,
					  ARIZONA_AOD_IRQ1, &val);
			if (ret)
				dev_warn(arizona->dev,
					"Failed to read AOD IRQ1 %d\n",	ret);
			else if (val)
				handle_nested_irq(
					irq_find_mapping(arizona->virq, 0));
		}

>  
>  		/*
>  		 * Check if one of the main interrupts is asserted and only

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog