[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[linux@arm.linux.org.uk (Russell King - ARM Linux)]

On Fri, Aug 05, 2011 at 08:43:20AM -0700, Stephen Warren wrote:
> Russell King - ARM Linux wrote at Friday, August 05, 2011 3:40 AM:
> > On Thu, Aug 04, 2011 at 05:00:17PM -0600, Stephen Warren wrote:
> > > In http://www.spinics.net/lists/linux-tegra/msg01731.html, Mark Brown
> > > pointed out that it was a little silly forcing every board or driver
> > > to gpio_request() a GPIO that is later converted to an IRQ, and passed
> > > to request_irq. The first patch in this series instead makes the core
> > > IRQ code perform these calls when appropriate, to avoid duplicating it
> > > everywhere.
> > 
> > Trying to go from IRQ to GPIO is not a good idea - most of the
> > IRQ <-> GPIO macros we have today are just plain broken.  Many of them
> > just add or subtract a constant, which means non-GPIO IRQs have an
> > apparant GPIO number too.  Couple this with the fact that all positive
> > GPIO numbers are valid, and this is a recipe for wrong GPIOs getting
> > used and GPIOs being requested for non-GPIO IRQs.
> > 
> > I think this was also discussed in the past, and the conclusion was that
> > IRQs should be kept separate from GPIOs.  Maybe views have changed since
> > then...
> > 
> > However, if we do want to do this, then it would be much better to provide
> > a new API for requesting GPIO IRQs, eg:
> > 
> > gpio_request_irq()
> > 
> > which would wrap around request_threaded_irq(), takes a GPIO number,
> > does the GPIO->IRQ conversion internally, and whatever GPIO setup is
> > required.  Something like this:
> 
> With that approach, drivers need to explicitly know whether they're
> passed a GPIO or an IRQ, and do something different, or they need to
> choose to only accept a GPIO or IRQ.

You completely missed the biggest reason why your approach is broken.

+               gpio = irq_to_gpio(irq);
+               if (gpio_is_valid(gpio))

Let's look at the code:

#define ARCH_NR_GPIOS           256

static inline bool gpio_is_valid(int number)
{
        return number >= 0 && number < ARCH_NR_GPIOS;
}

Now, let's take AT91:

#define irq_to_gpio(irq)  (irq)

This doesn't define ARCH_NR_GPIOS, so it gets the default 256.  Now lets
take a random selection of the AT91 interrupt numbers:

#define AT91RM9200_ID_US3       9       /* USART 3 */
#define AT91RM9200_ID_MCI       10      /* Multimedia Card Interface */
#define AT91RM9200_ID_UDP       11      /* USB Device Port */
#define AT91RM9200_ID_TWI       12      /* Two-Wire Interface */
#define AT91RM9200_ID_SPI       13      /* Serial Peripheral Interface */
#define AT91RM9200_ID_SSC0      14      /* Serial Synchronous Controller 0 */
#define AT91RM9200_ID_SSC1      15      /* Serial Synchronous Controller 1 */

None of these are GPIOs.  Yet gpio_is_valid(irq_to_gpio(AT91RM9200_ID_TWI))
is true.

That's the problem - it's undefined whether gpio_is_valid(irq_to_gpio(irq))
returns true or false for any particular interrupt.  There's no multiplexing
through gpiolib for the IRQ-to-GPIO mapping either, so it doesn't work for
off-SoC GPIOs.

So, you can't reliably go from interrupt numbers to GPIO numbers - it's
just not supported.  So to throw this into the IRQ layer is just going to
end up breaking a hell of a lot of platforms.

Now, stack on top of that a discussion@the Linaro Connect conference
this week where we discussed getting rid of IRQ numbers entirely, and
our desire to kill off irq_to_gpio() and I think it makes this approach
a non-starter.

> So it seems like, as was mentioned elsewhere in this thread, the upshot of
> this conversation is that interrupt chip drivers should do this internally,
> both to avoid requiring a general irq_to_gpio function, and because calling
> gpio_direction_input for GPIOs-used-as-IRQs isn't appropriate for all
> hardware.

That would be more appropriate, because the IRQ chip stuff at least knows
if there's a GPIO associated with it.

There's still the unanswered question whether we even want the IRQ layer
to do this kind of stuff, and the previous decision on that I believe was
in the negative.  So I think Thomas needs to respond to that point first.