Messages in this thread | | | From | Linus Walleij <> | Date | Thu, 28 Nov 2019 10:33:40 +0100 | Subject | Re: [PATCH v3 1/2] gpio: sch: Add edge event support |
| |
Hi Jan,
thanks for your patch!
On Wed, Nov 20, 2019 at 8:20 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com> > > Add the required infrastructure consisting of an irq_chip_generic with > its irq_chip_type callbacks to enable and report edge events of the pins > to the gpio core. The actual hook-up of the event interrupt will happen > separately. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Please resend after the merge window, some comments:
First I'm pretty sure this driver can select GPIOLIB_IRQCHIP and use infrastructure from the core to handle interrupts.
The fact that you register your own irq handler does not stop that. See for example the solution in drivers/gpio/gpio-mt7621.c where we set the ->parent_handler to NULL to let the driver handle the IRQs itself.
I will try to make this more explicit in the API as we work with this.
> struct sch_gpio { > struct gpio_chip chip; > spinlock_t lock; > unsigned short iobase; > unsigned short resume_base; > + int irq_base;
Why are you keeping this around in the state? Why not just a local variable?
> +static int sch_gpio_to_irq(struct gpio_chip *gpio, unsigned int offset) > +{ > + struct sch_gpio *sch = gpiochip_get_data(gpio); > + return sch->irq_base + offset; > +} (...) > + .to_irq = sch_gpio_to_irq, (...) > + irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, sch->chip.ngpio, > + NUMA_NO_NODE); > + if (irq_base < 0) > + return irq_base; > + sch->irq_base = irq_base; > + > + gc = devm_irq_alloc_generic_chip(&pdev->dev, "sch_gpio", 1, irq_base, > + NULL, handle_simple_irq); > + if (!gc) > + return -ENOMEM; (...) > + ret = devm_irq_setup_generic_chip(&pdev->dev, gc, > + IRQ_MSK(sch->chip.ngpio), > + 0, IRQ_NOREQUEST | IRQ_NOPROBE, 0); > + if (ret) > + return ret;
So I think you can avoid this complexity by jus doing what gpio-mt7621.c is doing, use devm_request_irq(), populate girq = &gc->irq; before registering the gpio_chip pass a handle_simple_irq and reuse core gpio irqchip infrastructure.
But I don't know everything so let's test and see!
Yours, Linus Walleij
| |