Re: [PATCH] pinctrl: at91: don't use the same irqchip with multiple gpiochips

2018-09-14 Thread Linus Walleij
On Thu, Sep 13, 2018 at 2:46 PM Ludovic Desroches
 wrote:

> Sharing the same irqchip with multiple gpiochips is not a good
> practice. For instance, when installing hooks, we change the state
> of the irqchip. The initial state of the irqchip for the second
> gpiochip to register is then disrupted.
>
> Signed-off-by: Ludovic Desroches 

Patch applied.

Yours,
Linus Walleij


[PATCH] pinctrl: at91: don't use the same irqchip with multiple gpiochips

2018-09-13 Thread Ludovic Desroches
Sharing the same irqchip with multiple gpiochips is not a good
practice. For instance, when installing hooks, we change the state
of the irqchip. The initial state of the irqchip for the second
gpiochip to register is then disrupted.

Signed-off-by: Ludovic Desroches 
---
Hi,

This patch fixes the issue encountered in linux-next because of
"gpiolib: override irq_enable/disable". As discussed [1], the gpiolib
can do some extra checks but sharing the same irqchip with multiple
gpiochips is seen as a bad practice.

[1] https://www.spinics.net/lists/arm-kernel/msg676097.html

 drivers/pinctrl/pinctrl-at91.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index cfd8239f2727..911ea0fe2a41 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -1574,16 +1574,6 @@ void at91_pinctrl_gpio_resume(void)
 #define gpio_irq_set_wake  NULL
 #endif /* CONFIG_PM */
 
-static struct irq_chip gpio_irqchip = {
-   .name   = "GPIO",
-   .irq_ack= gpio_irq_ack,
-   .irq_disable= gpio_irq_mask,
-   .irq_mask   = gpio_irq_mask,
-   .irq_unmask = gpio_irq_unmask,
-   /* .irq_set_type is set dynamically */
-   .irq_set_wake   = gpio_irq_set_wake,
-};
-
 static void gpio_irq_handler(struct irq_desc *desc)
 {
struct irq_chip *chip = irq_desc_get_chip(desc);
@@ -1624,12 +1614,22 @@ static int at91_gpio_of_irq_setup(struct 
platform_device *pdev,
struct gpio_chip*gpiochip_prev = NULL;
struct at91_gpio_chip   *prev = NULL;
struct irq_data *d = irq_get_irq_data(at91_gpio->pioc_virq);
+   struct irq_chip *gpio_irqchip;
int ret, i;
 
+   gpio_irqchip = devm_kzalloc(&pdev->dev, sizeof(*gpio_irqchip), 
GFP_KERNEL);
+   if (!gpio_irqchip)
+   return -ENOMEM;
+
at91_gpio->pioc_hwirq = irqd_to_hwirq(d);
 
-   /* Setup proper .irq_set_type function */
-   gpio_irqchip.irq_set_type = at91_gpio->ops->irq_type;
+   gpio_irqchip->name = "GPIO";
+   gpio_irqchip->irq_ack = gpio_irq_ack;
+   gpio_irqchip->irq_disable = gpio_irq_mask;
+   gpio_irqchip->irq_mask = gpio_irq_mask;
+   gpio_irqchip->irq_unmask = gpio_irq_unmask;
+   gpio_irqchip->irq_set_wake = gpio_irq_set_wake,
+   gpio_irqchip->irq_set_type = at91_gpio->ops->irq_type;
 
/* Disable irqs of this PIO controller */
writel_relaxed(~0, at91_gpio->regbase + PIO_IDR);
@@ -1640,7 +1640,7 @@ static int at91_gpio_of_irq_setup(struct platform_device 
*pdev,
 * interrupt.
 */
ret = gpiochip_irqchip_add(&at91_gpio->chip,
-  &gpio_irqchip,
+  gpio_irqchip,
   0,
   handle_edge_irq,
   IRQ_TYPE_NONE);
@@ -1658,7 +1658,7 @@ static int at91_gpio_of_irq_setup(struct platform_device 
*pdev,
if (!gpiochip_prev) {
/* Then register the chain on the parent IRQ */
gpiochip_set_chained_irqchip(&at91_gpio->chip,
-&gpio_irqchip,
+gpio_irqchip,
 at91_gpio->pioc_virq,
 gpio_irq_handler);
return 0;
-- 
2.12.2