| Message ID | 20200717141924.57887-1-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: max732x: Use irqchip template | expand |
Hi Linus, On Fri, 17 Jul 2020 at 17:19, Linus Walleij <linus.walleij@linaro.org> wrote: > > This makes the driver use the irqchip template to assign > properties to the gpio_irq_chip instead of using the > explicit calls to gpiochip_irqchip_add_nested() and > gpiochip_set_nested_irqchip(). The irqchip is instead > added while adding the gpiochip. > > Cc: Sam Protsenko <semen.protsenko@linaro.org> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- I can test it on my MAX7325 board with BeagleBone Black in a day or two. Do you want me to verify your patch on top of linux-mainline or linux-next? Also, is there any specific stuff you want me to look at, or making sure there are no regressions w.r.t. IRQ from the chip is enough? Thanks! > drivers/gpio/gpio-max732x.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpio/gpio-max732x.c b/drivers/gpio/gpio-max732x.c > index 63472f308857..347415344a20 100644 > --- a/drivers/gpio/gpio-max732x.c > +++ b/drivers/gpio/gpio-max732x.c > @@ -503,6 +503,8 @@ static int max732x_irq_setup(struct max732x_chip *chip, > > if (((pdata && pdata->irq_base) || client->irq) > && has_irq != INT_NONE) { > + struct gpio_irq_chip *girq; > + > if (pdata) > irq_base = pdata->irq_base; > chip->irq_features = has_irq; > @@ -517,19 +519,17 @@ static int max732x_irq_setup(struct max732x_chip *chip, > client->irq); > return ret; > } > - ret = gpiochip_irqchip_add_nested(&chip->gpio_chip, > - &max732x_irq_chip, > - irq_base, > - handle_simple_irq, > - IRQ_TYPE_NONE); > - if (ret) { > - dev_err(&client->dev, > - "could not connect irqchip to gpiochip\n"); > - return ret; > - } > - gpiochip_set_nested_irqchip(&chip->gpio_chip, > - &max732x_irq_chip, > - client->irq); > + > + girq = &chip->gpio_chip.irq; > + girq->chip = &max732x_irq_chip; > + /* This will let us handle the parent IRQ in the driver */ > + girq->parent_handler = NULL; > + girq->num_parents = 0; > + girq->parents = NULL; > + girq->default_type = IRQ_TYPE_NONE; > + girq->handler = handle_simple_irq; > + girq->threaded = true; > + girq->first = irq_base; /* FIXME: get rid of this */ > } > > return 0; > -- > 2.26.2 >
On Sun, Jul 19, 2020 at 6:34 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > On Fri, 17 Jul 2020 at 17:19, Linus Walleij <linus.walleij@linaro.org> wrote: > > > > This makes the driver use the irqchip template to assign > > properties to the gpio_irq_chip instead of using the > > explicit calls to gpiochip_irqchip_add_nested() and > > gpiochip_set_nested_irqchip(). The irqchip is instead > > added while adding the gpiochip. > > > > Cc: Sam Protsenko <semen.protsenko@linaro.org> > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > I can test it on my MAX7325 board with BeagleBone Black in a day or > two. Thanks! No hurry. > Do you want me to verify your patch on top of linux-mainline or > linux-next? Either should work. > Also, is there any specific stuff you want me to look at, > or making sure there are no regressions w.r.t. IRQ from the chip is > enough? Just that really, that the IRQs happen as before. Yours, Linus Walleij
Hi Linus, On Tue, 21 Jul 2020 at 12:54, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Sun, Jul 19, 2020 at 6:34 PM Sam Protsenko > <semen.protsenko@linaro.org> wrote: > > On Fri, 17 Jul 2020 at 17:19, Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > > This makes the driver use the irqchip template to assign > > > properties to the gpio_irq_chip instead of using the > > > explicit calls to gpiochip_irqchip_add_nested() and > > > gpiochip_set_nested_irqchip(). The irqchip is instead > > > added while adding the gpiochip. > > > > > > Cc: Sam Protsenko <semen.protsenko@linaro.org> > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > > I can test it on my MAX7325 board with BeagleBone Black in a day or > > two. > > Thanks! No hurry. > > > Do you want me to verify your patch on top of linux-mainline or > > linux-next? > > Either should work. > > > Also, is there any specific stuff you want me to look at, > > or making sure there are no regressions w.r.t. IRQ from the chip is > > enough? > > Just that really, that the IRQs happen as before. > Just tested it on my MAX7325 board [1], by adding gpio-keys and gpio-leds to BBB dts [2]. Alas, the patch seems to be breaking IRQs. Before the patch, I can see gpio-buttons appear in /proc/interrupts and dmesg is clear of errors. After applying the patch, no gpio-keys appear in /proc/interrupts and dmesg is reporting errors like this: irq: no irq domain found for max7325@68 ! This is probably because the patch is setting gpio_chip structure fields after devm_gpiochip_add_data() was executed. Next hacky change fixes it: <<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>> @@ -695,11 +695,11 @@ static int max732x_probe(struct i2c_client *client, return ret; } - ret = devm_gpiochip_add_data(&client->dev, &chip->gpio_chip, chip); + ret = max732x_irq_setup(chip, id); if (ret) return ret; - ret = max732x_irq_setup(chip, id); + ret = devm_gpiochip_add_data(&client->dev, &chip->gpio_chip, chip); if (ret) return ret; <<<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>> I didn't check if it's a correct fix, so the diff above is only to check the idea. Anyway, if you send v2 I can retest it, as my setup is ready. Thanks! [1] https://github.com/joe-skb7/max7325-pcb [2] https://github.com/joe-skb7/linux-mainline-bbb-max732x/commits/max732_bbb_test > Yours, > Linus Walleij
On Fri, Jul 24, 2020 at 1:50 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > On Tue, 21 Jul 2020 at 12:54, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Sun, Jul 19, 2020 at 6:34 PM Sam Protsenko > > <semen.protsenko@linaro.org> wrote: ... > > Just that really, that the IRQs happen as before. > Just tested it on my MAX7325 board [1], by adding gpio-keys and > gpio-leds to BBB dts [2]. Alas, the patch seems to be breaking IRQs. > Before the patch, I can see gpio-buttons appear in /proc/interrupts > and dmesg is clear of errors. After applying the patch, no gpio-keys > appear in /proc/interrupts and dmesg is reporting errors like this: > > irq: no irq domain found for max7325@68 ! > > This is probably because the patch is setting gpio_chip structure > fields after devm_gpiochip_add_data() was executed. ... > I didn't check if it's a correct fix, so the diff above is only to > check the idea. I checked pca953x and there the same trick has been applied. In my setup that driver works. I think it's the right way to go. -- With Best Regards, Andy Shevchenko
diff --git a/drivers/gpio/gpio-max732x.c b/drivers/gpio/gpio-max732x.c index 63472f308857..347415344a20 100644 --- a/drivers/gpio/gpio-max732x.c +++ b/drivers/gpio/gpio-max732x.c @@ -503,6 +503,8 @@ static int max732x_irq_setup(struct max732x_chip *chip, if (((pdata && pdata->irq_base) || client->irq) && has_irq != INT_NONE) { + struct gpio_irq_chip *girq; + if (pdata) irq_base = pdata->irq_base; chip->irq_features = has_irq; @@ -517,19 +519,17 @@ static int max732x_irq_setup(struct max732x_chip *chip, client->irq); return ret; } - ret = gpiochip_irqchip_add_nested(&chip->gpio_chip, - &max732x_irq_chip, - irq_base, - handle_simple_irq, - IRQ_TYPE_NONE); - if (ret) { - dev_err(&client->dev, - "could not connect irqchip to gpiochip\n"); - return ret; - } - gpiochip_set_nested_irqchip(&chip->gpio_chip, - &max732x_irq_chip, - client->irq); + + girq = &chip->gpio_chip.irq; + girq->chip = &max732x_irq_chip; + /* This will let us handle the parent IRQ in the driver */ + girq->parent_handler = NULL; + girq->num_parents = 0; + girq->parents = NULL; + girq->default_type = IRQ_TYPE_NONE; + girq->handler = handle_simple_irq; + girq->threaded = true; + girq->first = irq_base; /* FIXME: get rid of this */ } return 0;
This makes the driver use the irqchip template to assign properties to the gpio_irq_chip instead of using the explicit calls to gpiochip_irqchip_add_nested() and gpiochip_set_nested_irqchip(). The irqchip is instead added while adding the gpiochip. Cc: Sam Protsenko <semen.protsenko@linaro.org> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/gpio/gpio-max732x.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) -- 2.26.2