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] pinctrl: nsp: Set irq handler based on trig type
To:     Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>,
        rjui@broadcom.com, sbranden@broadcom.com,
        bcm-kernel-feedback-list@broadcom.com, linus.walleij@linaro.org,
        linux-gpio@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org
References: <20200630204704.17736-1-mark.tomlinson@alliedtelesis.co.nz>
From:   Ray Jui <ray.jui@broadcom.com>
Message-ID: <bc6c62f7-1ae5-4f7d-43ba-efae057e8cb7@broadcom.com>
Date:   Tue, 30 Jun 2020 15:26:57 -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: <20200630204704.17736-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 6/30/2020 1:47 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, always acknowledge the interrupt
> regardless of whether it is edge or level triggered.
> 
> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
> ---
>  drivers/pinctrl/bcm/pinctrl-nsp-gpio.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
> index bed0124388c0..349fb384113e 100644
> --- a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
> +++ b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
> @@ -174,11 +174,8 @@ static void nsp_gpio_irq_ack(struct irq_data *d)
>  	struct nsp_gpio *chip = gpiochip_get_data(gc);
>  	unsigned gpio = d->hwirq;
>  	u32 val = BIT(gpio);
> -	u32 trigger_type;
>  
> -	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);
> +	nsp_set_bit(chip, REG, NSP_GPIO_EVENT, gpio, val);


I have a question here. I assume writing a bit to this register will
result in clearing that bit, is that true?

Based on the driver, the 'nsp_gpio_irq_handler' seems to rely on
'NSP_GPIO_EVENT' register to figure out which GPIO the interrupt is for.
And if so, and if this is cleared here that is invoked before the actual
IRQ handler, how does this work?

I could be missing something here, so please help to explain it in more
details here.

Thanks,

Ray