regressions in linux-next?

Commit Message

Javier Martinez Canillas April 23, 2014, 2:50 p.m. UTC

On Wed, Apr 23, 2014 at 4:38 PM, Linus Walleij <[e-mail]> wrote:
> On Wed, Apr 23, 2014 at 3:29 PM, Nishanth Menon <[e-mail]> wrote:
>> On 04/23/2014 08:01 AM, Linus Walleij wrote:
>
>>> What about this:
>>>
>>> if (chip->irq_default_type != IRQ_TYPE_NONE)
>>>     irq_set_irq_type(irq, chip->irq_default_type);
>>>
>>> This way you can pass IRQ_TYPE_NONE and nothing happens in
>>> the mapping.
>>
>> What if these drivers depend on IRQ_TYPE_NONE to do something for the
>> GPIO pins?
>
> Yeah :-(
>
>> would you expect these drivers to pass IRQ_TYPE_DEFAULT?
>
> Actually that sounds like a good idea. Maybe we can go over the few
> drivers that pass IRQ_TYPE_NONE and see what they actually want.
> There are not *too* many users of this call yet.
>
>> OR I wonder
>> if we could pass some flag like -1 for platforms that dont care?
>
> The flags parameter to gpiochip_irqchip_add() is unsigned...
> Switching to IRQ_TYPE_DEFAULT for drivers that want this is likely
> the sane thing to do.
>
> Yours,
> Linus Walleij

I prefer to have an explicit way to tell gpiolib whether it has to set
a default IRQ type or not than relying on passing magic numbers like
-1.

What do you think about the following patch? Although I agree that if
we can use IRQ_TYPE_NONE as you propose then tht is a much better and
efficient solution that this patch.

Best regards,
Javier

From 8720c6798f86b71d8b11c57ecb7bae86a4ee656b Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <[e-mail]>
Date: Wed, 23 Apr 2014 16:45:05 +0200
Subject: [RFC] gpio: don't set IRQ default type unconditionally

Creating a mapping for a GPIO pin into the Linux virtual IRQ
space does not necessarily mean that the GPIO is going to be
used as an interrupt line, it only mean that it could be used.

But current gpiolib irqchip .map function handler calls to
irq_set_irq_type() for each GPIO pin in the bank. Some drivers
assume that this function is called either because a driver
has explicitly requested an IRQ with request_irq() or that a
DT device node has defined a GPIO controller phandle as its
"interrupt-parent" and setups the chip accordingly.

Others drivers have different semantics and expects that a
irq_set_irq_type() would setup the hardware to be some default state.

So is better to allow each driver to choose whether they want
to set a default IRQ type on the mapping function or not.

Signed-off-by: Javier Martinez Canillas <[e-mail]>
---
 drivers/gpio/gpio-omap.c    | 1 +
 drivers/gpio/gpiolib.c      | 3 ++-
 include/linux/gpio/driver.h | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

Comments

Linus Walleij April 23, 2014, 2:52 p.m. UTC | #1

On Wed, Apr 23, 2014 at 4:50 PM, Javier Martinez Canillas
<[e-mail]> wrote:

> What do you think about the following patch? Although I agree that if
> we can use IRQ_TYPE_NONE as you propose then tht is a much better and
> efficient solution that this patch.

Let's try IRQ_TYPE_NONE first and if that fails we go back to this.

I've pushed the current patch to linux-next.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 8cc9e91..2e23858 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1101,6 +1101,7 @@  static int omap_gpio_chip_init(struct gpio_bank *bank)
  gpio += bank->width;
  }
  bank->chip.ngpio = bank->width;
+ bank->chip.set_irq_default = false;

  ret = gpiochip_add(&bank->chip);
  if (ret) {
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c12fe9d..f12aea3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1402,7 +1402,8 @@  static int gpiochip_irq_map(struct irq_domain
*d, unsigned int irq,
 #else
  irq_set_noprobe(irq);
 #endif
- irq_set_irq_type(irq, chip->irq_default_type);
+ if (chip->set_irq_default)
+ irq_set_irq_type(irq, chip->irq_default_type);

  return 0;
 }
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 573e4f3..168c93e 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -113,6 +113,7 @@  struct gpio_chip {
  unsigned int irq_base;
  irq_flow_handler_t irq_handler;
  unsigned int irq_default_type;
+ bool set_irq_default;
 #endif

 #if defined(CONFIG_OF_GPIO)