Re: [PATCH v2 6/6] pintrl: meson: add support for GPIO interrupts

[Jerome Brunet <jbrunet@xxxxxxxxxxxx>]

On Wed, 2017-05-17 at 22:36 +0200, Heiner Kallweit wrote:
> Am 17.05.2017 um 01:16 schrieb Jerome Brunet:
> > On Tue, 2017-05-16 at 20:31 +0200, Heiner Kallweit wrote:

[snip]

> > 
> > Maybe you missed it in our previous discussion with Linus but, no you can't
> > create mapping in gpio_to_irq. This callback is fast and creating mappings
> > might
> > sleep because of the irq_domain mutex
> > 
> > https://patchwork.ozlabs.org/patch/684208/
> > 
> 
> My comment was misunderstandable. What I meant was that a driver doesn't have
> to
> use gpio_to_irq to request an interrupt, this can also be done via DT.
> 

If you don't provide gpio_to_irq, you won't support the drivers which only
request a gpio through DT (because they need to read the pin state) and later
request an irq from it (to get an event and avoid polling)

That's a fairly common use-case. IMHO, this callback should be implemented by
the gpio controller, when it is an interrupt controller.

[snip]

> > > I think you are onto something. As I told you previously, the problem was
> > > to
> > create the mappings in pinctrl w/o allocating the parent. I struggled with
> > that
> > back in November and I had no time to really get back to it since.
> > 
> > Creating domains in each gpio controller, w/ all the mapping created at
> > startup,
> >   stacked to the domain provided by the driver/irqchip would solve the
> > problem.
> > 
> > We would just have to call find_mapping in gpio_to_irq, which is fine and
> > allocate the parent irq in the request_ressource callback.
> > 
> 
> In request_resource we don't know the irq type yet (and therefore whether we
> need
> one or wo parent interrupts). IMHO we can't get completely rid of allocating
> parents in set_type.

Yes you can. The HW does *not* support IRQ_BOTH on an irq line. You are trying
to come up with a SW workaround to get this feature. W/O this workaround, there
no reason to allocate a parent irq in set_type.
At this point, no one has come up with a clean solution to add this feature to
the meson-gpio-irq device.
Maybe evolutions in the irq framework will allow this cleanly in the future, I
don't think this is the case right now. 

[snip]
> 
> > > And all I had to do is implementing one irq_chip and changing one line in
> > > the
> > > existing
> > > pinctrl driver.
> > > Whilst I have to admit that e.g. calling request_irq for a parent irq from
> > > the
> > > irq_set_type
> > > callback may be something people find ugly and hacky.
> > > 
> > 
> > The fact is that the HW does not directly support IRQ_EDGE_BOTH. Yes, it
> > would
> > be nice to find a way to support it anyway. There two possibilities for it:
> > * Change the change the edge type depending on the next expected edge: Marc
> > already stated that he is firmly against that. It is indeed not very robust
> > * Allocate and deallocate additional parent irq to get secondary irq in
> > set_type: This indeed looks hacky, I'd like to get the view of irq
> > maintainers
> > on this.
> > 
> 
> Support for IRQ_TYPE_EDGE_BOTH I'd consider a mandatory feature. It's needed
> e.g. by the SD card detection (drivers/mmc/slot-gpio.c).
> 
"You'd consider" maybe, but the fact is that it is not mandatory. Having irq
based card detect (or buttons, or jack insert, or whatever) is probably more
elegant, but the polled version also works well. Elegant and mandatory are not
the same things



--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html