| Message ID | 20210208085954.30050-3-nikita.shubin@maquefel.me |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,1/7] gpio: ep93xx: fix BUG_ON port F usage | expand |
Hello Andy. On Monday, 8 February 2021 16:20:17 MSK Andy Shevchenko wrote: >On Mon, Feb 8, 2021 at 11:00 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote: >> Fixes the following warnings which results in interrupts disabled on >> port B/F: >> >> gpio gpiochip1: (B): detected irqchip that is shared with multiple >> gpiochips: please fix the driver. gpio gpiochip5: (F): detected >> irqchip that is shared with multiple gpiochips: please fix the >> driver. >> >> - added separate irqchip for each interrupt capable gpiochip >> - provided unique names for each irqchip > >... > >> +static void ep93xx_init_irq_chip(struct device *dev, struct irq_chip >> *ic, const char *label) +{ >> >> + ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s", >> label); >Is the label being NULL okay? The label is taken from ep93xx_gpio_banks[], so unless we explicitly pass zero to ep93xx_init_irq_chip(), we are ok. > >> + ic->irq_ack = ep93xx_gpio_irq_ack; >> + ic->irq_mask_ack = ep93xx_gpio_irq_mask_ack; >> + ic->irq_mask = ep93xx_gpio_irq_mask; >> + ic->irq_unmask = ep93xx_gpio_irq_unmask; >> + ic->irq_set_type = ep93xx_gpio_irq_type; >> +} > >... > >> - girq->chip = &ep93xx_gpio_irq_chip; > >I don't see where you remove that static structure. Good catch - thank you very much, also i noticed that i forgot to switch IRQ chip in irq_set_chip_and_handler() for port F.
On Tue, Feb 9, 2021 at 2:35 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > On Monday, 8 February 2021 16:20:17 MSK Andy Shevchenko wrote: > >On Mon, Feb 8, 2021 at 11:00 AM Nikita Shubin > <nikita.shubin@maquefel.me> wrote: ... > >> +static void ep93xx_init_irq_chip(struct device *dev, struct irq_chip > >> *ic, const char *label) +{ > >> > >> + ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s", > >> label); > >Is the label being NULL okay? > > The label is taken from ep93xx_gpio_banks[], so unless we explicitly > pass zero to ep93xx_init_irq_chip(), we are ok. Maybe I was unclear, let me rephrase: Is the *resulting* label being NULL okay? > >> + ic->irq_ack = ep93xx_gpio_irq_ack; > >> + ic->irq_mask_ack = ep93xx_gpio_irq_mask_ack; > >> + ic->irq_mask = ep93xx_gpio_irq_mask; > >> + ic->irq_unmask = ep93xx_gpio_irq_unmask; > >> + ic->irq_set_type = ep93xx_gpio_irq_type; > >> +} -- With Best Regards, Andy Shevchenko
On Tuesday, 9 February 2021 15:46:19 MSK Andy Shevchenko wrote: >On Tue, Feb 9, 2021 at 2:35 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: >> On Monday, 8 February 2021 16:20:17 MSK Andy Shevchenko wrote: >> >On Mon, Feb 8, 2021 at 11:00 AM Nikita Shubin >> >> <nikita.shubin@maquefel.me> wrote: >... > >> >> +static void ep93xx_init_irq_chip(struct device *dev, struct >> >> irq_chip >> >> *ic, const char *label) +{ >> >> >> >> + ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s", >> >> label); >> > >> >Is the label being NULL okay? You mean ENOMEM should be honored ? I think you are right about it. >> >> The label is taken from ep93xx_gpio_banks[], so unless we explicitly >> pass zero to ep93xx_init_irq_chip(), we are ok. > >Maybe I was unclear, let me rephrase: Is the *resulting* label being >NULL okay? >> >> + ic->irq_ack = ep93xx_gpio_irq_ack; >> >> + ic->irq_mask_ack = ep93xx_gpio_irq_mask_ack; >> >> + ic->irq_mask = ep93xx_gpio_irq_mask; >> >> + ic->irq_unmask = ep93xx_gpio_irq_unmask; >> >> + ic->irq_set_type = ep93xx_gpio_irq_type; >> >> +}
On Tue, Feb 9, 2021 at 2:54 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > On Tuesday, 9 February 2021 15:46:19 MSK Andy Shevchenko wrote: > >On Tue, Feb 9, 2021 at 2:35 PM Nikita Shubin > <nikita.shubin@maquefel.me> wrote: > >> On Monday, 8 February 2021 16:20:17 MSK Andy Shevchenko wrote: > >> >On Mon, Feb 8, 2021 at 11:00 AM Nikita Shubin > >> <nikita.shubin@maquefel.me> wrote: ... > >> >> + ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s", > >> >> label); > >> > > >> >Is the label being NULL okay? > > You mean ENOMEM should be honored ? I think you are right about it. Depending on what is the answer to the question below. If NULL label is okay (and here is just optional) then simply comment it in the code, otherwise check is missed. > >> The label is taken from ep93xx_gpio_banks[], so unless we explicitly > >> pass zero to ep93xx_init_irq_chip(), we are ok. > > > >Maybe I was unclear, let me rephrase: Is the *resulting* label being > >NULL okay? -- With Best Regards, Andy Shevchenko
diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c index 64d6c2b4282e..3d8eb8769470 100644 --- a/drivers/gpio/gpio-ep93xx.c +++ b/drivers/gpio/gpio-ep93xx.c @@ -38,6 +38,7 @@ #define EP93XX_GPIO_F_IRQ_BASE 80 struct ep93xx_gpio_irq_chip { + struct irq_chip ic; u8 irq_offset; u8 int_unmasked; u8 int_enabled; @@ -331,6 +332,16 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc, unsigned offset) return EP93XX_GPIO_F_IRQ_BASE + offset; } +static void ep93xx_init_irq_chip(struct device *dev, struct irq_chip *ic, const char *label) +{ + ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s", label); + ic->irq_ack = ep93xx_gpio_irq_ack; + ic->irq_mask_ack = ep93xx_gpio_irq_mask_ack; + ic->irq_mask = ep93xx_gpio_irq_mask; + ic->irq_unmask = ep93xx_gpio_irq_unmask; + ic->irq_set_type = ep93xx_gpio_irq_type; +} + static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc, struct platform_device *pdev, struct ep93xx_gpio *epg, @@ -352,6 +363,8 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc, girq = &gc->irq; if (bank->has_irq || bank->has_hierarchical_irq) { + struct irq_chip *ic; + gc->set_config = ep93xx_gpio_set_config; egc->eic = devm_kcalloc(dev, 1, sizeof(*egc->eic), @@ -359,7 +372,9 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc, if (!egc->eic) return -ENOMEM; egc->eic->irq_offset = bank->irq; - girq->chip = &ep93xx_gpio_irq_chip; + ic = &egc->eic->ic; + ep93xx_init_irq_chip(dev, ic, bank->label); + girq->chip = ic; } if (bank->has_irq) {
Fixes the following warnings which results in interrupts disabled on port B/F: gpio gpiochip1: (B): detected irqchip that is shared with multiple gpiochips: please fix the driver. gpio gpiochip5: (F): detected irqchip that is shared with multiple gpiochips: please fix the driver. - added separate irqchip for each interrupt capable gpiochip - provided unique names for each irqchip Fixes: d2b091961510 ("gpio: ep93xx: Pass irqchip when adding gpiochip") Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> --- v4->v5: - generate IRQ chip's names dynamicaly from label --- drivers/gpio/gpio-ep93xx.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)