Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] pinctrl: nsp: Set irq handler based on trig type
@ 2020-06-30 20:47 Mark Tomlinson
  2020-06-30 22:26 ` Ray Jui
  0 siblings, 1 reply; 3+ messages in thread
From: Mark Tomlinson @ 2020-06-30 20:47 UTC (permalink / raw)
  To: rjui, sbranden, bcm-kernel-feedback-list, linus.walleij, linux-gpio
  Cc: linux-kernel, Mark Tomlinson

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);
 }
 
 /*
@@ -262,6 +259,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 +694,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);
-- 
2.27.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] pinctrl: nsp: Set irq handler based on trig type
  2020-06-30 20:47 [PATCH] pinctrl: nsp: Set irq handler based on trig type Mark Tomlinson
@ 2020-06-30 22:26 ` Ray Jui
  2020-07-01 20:14   ` Mark Tomlinson
  0 siblings, 1 reply; 3+ messages in thread
From: Ray Jui @ 2020-06-30 22:26 UTC (permalink / raw)
  To: Mark Tomlinson, rjui, sbranden, bcm-kernel-feedback-list,
	linus.walleij, linux-gpio
  Cc: linux-kernel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] pinctrl: nsp: Set irq handler based on trig type
  2020-06-30 22:26 ` Ray Jui
@ 2020-07-01 20:14   ` Mark Tomlinson
  0 siblings, 0 replies; 3+ messages in thread
From: Mark Tomlinson @ 2020-07-01 20:14 UTC (permalink / raw)
  To: ray.jui, bcm-kernel-feedback-list, linus.walleij, linux-gpio,
	sbranden, rjui
  Cc: linux-kernel

On Tue, 2020-06-30 at 15:26 -0700, Ray Jui wrote:
> -	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?

It seems that this change masked another issue I was having. However,
the original code is still wrong as using nsp_set_bit() will do a
read/modify/write and clear all events regardless of what gpio is.

I have found another issue, where I'm getting many more edge-triggered
interrupts than I think I should be, so I'll sort that and send a v2 of
this patch.


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-30 20:47 [PATCH] pinctrl: nsp: Set irq handler based on trig type Mark Tomlinson
2020-06-30 22:26 ` Ray Jui
2020-07-01 20:14   ` Mark Tomlinson

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org
	public-inbox-index linux-gpio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git