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:

quoted

On Thu, Aug 04, 2011 at 05:00:17PM -0600, Stephen Warren wrote:

quoted

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.