| Message ID | 20220511183210.5248-5-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Renesas RZ/G2L IRQC support | expand |
Hi Prabhakar, On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Supported GPIO IRQs by the chip is not always equal to the number of GPIO > pins. For example on Renesas RZ/G2L SoC where it has GPIO0-122 pins but at > a give point a maximum of only 32 GPIO pins can be used as IRQ lines in > the IRQC domain. > > This patch adds ngirq member to struct gpio_irq_chip and passes this as a > size to irq_domain_create_hierarchy()/irq_domain_create_simple() if it is > being set in the driver otherwise fallbacks to using ngpio. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Thanks for your patch! > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1221,7 +1221,7 @@ static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc) > gc->irq.domain = irq_domain_create_hierarchy( > gc->irq.parent_domain, > 0, > - gc->ngpio, > + gc->irq.ngirq ?: gc->ngpio, > gc->irq.fwnode, > &gc->irq.child_irq_domain_ops, > gc); > @@ -1574,7 +1574,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, > } else { > /* Some drivers provide custom irqdomain ops */ > gc->irq.domain = irq_domain_create_simple(fwnode, > - gc->ngpio, > + gc->irq.ngirq ?: gc->ngpio, > gc->irq.first, > gc->irq.domain_ops ?: &gpiochip_domain_ops, > gc); OK. gpiochip_irqchip_remove() does: /* Remove all IRQ mappings and delete the domain */ if (gc->irq.domain) { unsigned int irq; for (offset = 0; offset < gc->ngpio; offset++) { if (!gpiochip_irqchip_irq_valid(gc, offset)) Hence it relies on gc->irq.valid_mask, which I think is OK in general. continue; irq = irq_find_mapping(gc->irq.domain, offset); irq_dispose_mapping(irq); } irq_domain_remove(gc->irq.domain); } > --- a/include/linux/gpio/driver.h > +++ b/include/linux/gpio/driver.h > @@ -51,6 +51,14 @@ struct gpio_irq_chip { > */ > const struct irq_domain_ops *domain_ops; > > + /** > + * @ngirq: > + * > + * The number of GPIO IRQ's handled by this IRQ domain; usually is > + * equal to ngpio. "If not set, ngpio will be used." > + */ > + u16 ngirq; > + > #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY > /** > * @fwnode: Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Supported GPIO IRQs by the chip is not always equal to the number of GPIO > pins. For example on Renesas RZ/G2L SoC where it has GPIO0-122 pins but at > a give point a maximum of only 32 GPIO pins can be used as IRQ lines in > the IRQC domain. > > This patch adds ngirq member to struct gpio_irq_chip and passes this as a > size to irq_domain_create_hierarchy()/irq_domain_create_simple() if it is > being set in the driver otherwise fallbacks to using ngpio. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> As Geert says, I think you can just use .valid_mask for this, what do you say? Yours, Linus Walleij
Hi Geert, Thank you for the review. On Thu, May 12, 2022 at 8:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Supported GPIO IRQs by the chip is not always equal to the number of GPIO > > pins. For example on Renesas RZ/G2L SoC where it has GPIO0-122 pins but at > > a give point a maximum of only 32 GPIO pins can be used as IRQ lines in > > the IRQC domain. > > > > This patch adds ngirq member to struct gpio_irq_chip and passes this as a > > size to irq_domain_create_hierarchy()/irq_domain_create_simple() if it is > > being set in the driver otherwise fallbacks to using ngpio. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -1221,7 +1221,7 @@ static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc) > > gc->irq.domain = irq_domain_create_hierarchy( > > gc->irq.parent_domain, > > 0, > > - gc->ngpio, > > + gc->irq.ngirq ?: gc->ngpio, > > gc->irq.fwnode, > > &gc->irq.child_irq_domain_ops, > > gc); > > @@ -1574,7 +1574,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, > > } else { > > /* Some drivers provide custom irqdomain ops */ > > gc->irq.domain = irq_domain_create_simple(fwnode, > > - gc->ngpio, > > + gc->irq.ngirq ?: gc->ngpio, > > gc->irq.first, > > gc->irq.domain_ops ?: &gpiochip_domain_ops, > > gc); > > OK. > > gpiochip_irqchip_remove() does: > > /* Remove all IRQ mappings and delete the domain */ > if (gc->irq.domain) { > unsigned int irq; > > for (offset = 0; offset < gc->ngpio; offset++) { > if (!gpiochip_irqchip_irq_valid(gc, offset)) > > Hence it relies on gc->irq.valid_mask, which I think is OK in general. > Agreed. > continue; > > irq = irq_find_mapping(gc->irq.domain, offset); > irq_dispose_mapping(irq); > } > > irq_domain_remove(gc->irq.domain); > > } > > > --- a/include/linux/gpio/driver.h > > +++ b/include/linux/gpio/driver.h > > @@ -51,6 +51,14 @@ struct gpio_irq_chip { > > */ > > const struct irq_domain_ops *domain_ops; > > > > + /** > > + * @ngirq: > > + * > > + * The number of GPIO IRQ's handled by this IRQ domain; usually is > > + * equal to ngpio. > > "If not set, ngpio will be used." > sure will update the comment. Cheers, Prabhakar > > + */ > > + u16 ngirq; > > + > > #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY > > /** > > * @fwnode: > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Linus, Thank you for the review. On Fri, May 13, 2022 at 9:47 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > Supported GPIO IRQs by the chip is not always equal to the number of GPIO > > pins. For example on Renesas RZ/G2L SoC where it has GPIO0-122 pins but at > > a give point a maximum of only 32 GPIO pins can be used as IRQ lines in > > the IRQC domain. > > > > This patch adds ngirq member to struct gpio_irq_chip and passes this as a > > size to irq_domain_create_hierarchy()/irq_domain_create_simple() if it is > > being set in the driver otherwise fallbacks to using ngpio. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > As Geert says, I think you can just use .valid_mask for this, > what do you say? > I don't think Geert is suggesting that. The .valid_mask option is one time setting but what I need is something dynamic i.e. out of 392 GPIO pins any 32 can be used as an interrupt pin. Also with this patch we also save on memory [0]. [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/kernel/irq/irqdomain.c?h=next-20220518#n153 Cheers, Prabhakar
On Wed, May 18, 2022 at 8:36 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > > As Geert says, I think you can just use .valid_mask for this, > > what do you say? > > > I don't think Geert is suggesting that. The .valid_mask option is one > time setting but what I need is something dynamic i.e. out of 392 GPIO > pins any 32 can be used as an interrupt pin. So why can't this just be determined from the compatible? This does not sound like a configuration option at all but something related to the IP block per se, and then you know that if it has a certain compatible then it has this property. Yours, Linus Walleij
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index d36c4a965efc..a98877f939ea 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1221,7 +1221,7 @@ static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc) gc->irq.domain = irq_domain_create_hierarchy( gc->irq.parent_domain, 0, - gc->ngpio, + gc->irq.ngirq ?: gc->ngpio, gc->irq.fwnode, &gc->irq.child_irq_domain_ops, gc); @@ -1574,7 +1574,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, } else { /* Some drivers provide custom irqdomain ops */ gc->irq.domain = irq_domain_create_simple(fwnode, - gc->ngpio, + gc->irq.ngirq ?: gc->ngpio, gc->irq.first, gc->irq.domain_ops ?: &gpiochip_domain_ops, gc); diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 874aabd270c9..ed6df186907d 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -51,6 +51,14 @@ struct gpio_irq_chip { */ const struct irq_domain_ops *domain_ops; + /** + * @ngirq: + * + * The number of GPIO IRQ's handled by this IRQ domain; usually is + * equal to ngpio. + */ + u16 ngirq; + #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY /** * @fwnode:
Supported GPIO IRQs by the chip is not always equal to the number of GPIO pins. For example on Renesas RZ/G2L SoC where it has GPIO0-122 pins but at a give point a maximum of only 32 GPIO pins can be used as IRQ lines in the IRQC domain. This patch adds ngirq member to struct gpio_irq_chip and passes this as a size to irq_domain_create_hierarchy()/irq_domain_create_simple() if it is being set in the driver otherwise fallbacks to using ngpio. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- drivers/gpio/gpiolib.c | 4 ++-- include/linux/gpio/driver.h | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-)