Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] pinctrl: nsp: Set irq handler based on trig type
To:     Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>,
        bcm-kernel-feedback-list@broadcom.com, linus.walleij@linaro.org,
        linux-gpio@vger.kernel.org, sbranden@broadcom.com
Cc:     linux-kernel@vger.kernel.org
References: <20200703011830.15655-1-mark.tomlinson@alliedtelesis.co.nz>
From:   Ray Jui <ray.jui@broadcom.com>
Message-ID: <900a32bf-9c8c-eafd-fb04-08b4eeac4428@broadcom.com>
Date:   Mon, 6 Jul 2020 11:07:12 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.9.0
MIME-Version: 1.0
In-Reply-To: <20200703011830.15655-1-mark.tomlinson@alliedtelesis.co.nz>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Mark,

On 7/2/2020 6:18 PM, Mark Tomlinson wrote:
> Rather than always using handle_simple_irq() as the gpio_irq_chip
> handler, set a more appropriate handler based on the IRQ trigger type
> requested. This is important for level triggered interrupts which need
> to be masked during handling. Also, fix the interrupt acknowledge so
> that it clears only one interrupt instead of all interrupts which are
> currently active. Finally there is no need to clear the interrupt during
> the interrupt handler, since the edge-triggered handler will do that for
> us.
> 
> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
> ---
> Changes in v2:
> - Don't perform unnecessary acks.
> 
>  drivers/pinctrl/bcm/pinctrl-nsp-gpio.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
> index bed0124388c0..a00a42a61a90 100644
> --- a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
> +++ b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
> @@ -154,15 +154,9 @@ static irqreturn_t nsp_gpio_irq_handler(int irq, void *data)
>  		level &= readl(chip->base + NSP_GPIO_INT_MASK);
>  		int_bits = level | event;
>  
> -		for_each_set_bit(bit, &int_bits, gc->ngpio) {
> -			/*
> -			 * Clear the interrupt before invoking the
> -			 * handler, so we do not leave any window
> -			 */
> -			writel(BIT(bit), chip->base + NSP_GPIO_EVENT);
> +		for_each_set_bit(bit, &int_bits, gc->ngpio)
>  			generic_handle_irq(
>  				irq_linear_revmap(gc->irq.domain, bit));
> -		}
>  	}
>  
>  	return  int_bits ? IRQ_HANDLED : IRQ_NONE;
> @@ -178,7 +172,7 @@ static void nsp_gpio_irq_ack(struct irq_data *d)
>  
>  	trigger_type = irq_get_trigger_type(d->irq);
>  	if (trigger_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> -		nsp_set_bit(chip, REG, NSP_GPIO_EVENT, gpio, val);
> +		writel(val, chip->base + NSP_GPIO_EVENT);
>  }
>  
>  /*
> @@ -262,6 +256,12 @@ static int nsp_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  
>  	nsp_set_bit(chip, REG, NSP_GPIO_EVENT_INT_POLARITY, gpio, falling);
>  	nsp_set_bit(chip, REG, NSP_GPIO_INT_POLARITY, gpio, level_low);
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		irq_set_handler_locked(d, handle_edge_irq);
> +	else
> +		irq_set_handler_locked(d, handle_level_irq);
> +
>  	raw_spin_unlock_irqrestore(&chip->lock, flags);
>  
>  	dev_dbg(chip->dev, "gpio:%u level_low:%s falling:%s\n", gpio,
> @@ -691,7 +691,7 @@ static int nsp_gpio_probe(struct platform_device *pdev)
>  		girq->num_parents = 0;
>  		girq->parents = NULL;
>  		girq->default_type = IRQ_TYPE_NONE;
> -		girq->handler = handle_simple_irq;
> +		girq->handler = handle_bad_irq;
>  	}
>  
>  	ret = devm_gpiochip_add_data(dev, gc, chip);
> 

This change looks good to me. Thanks!

Reviewed-by: Ray Jui <ray.jui@broadcom.com>