[2/4] gpio: allow customizing hierarchical IRQ chips
diff mbox series

Message ID 20190708110138.24657-3-masneyb@onstation.org
State New
Headers show
Series
  • gpio: hierarchical IRQ improvements
Related show

Commit Message

Brian Masney July 8, 2019, 11:01 a.m. UTC
Now that the GPIO core has support for hierarchical IRQ chips, let's add
support for three new callbacks in struct gpio_irq_chip:

populate_parent_fwspec:
    This optional callback populates the struct irq_fwspec for the
    parent's IRQ domain. If this is not specified, then
    gpiochip_populate_parent_fwspec_twocell will be used. A four-cell
    variant named &gpiochip_populate_parent_fwspec_twocell is also
    available.

child_pin_to_irq:
    This optional callback is used to translate the child's GPIO pin
    number to an IRQ number for the GPIO to_irq() callback. If this is
    not specified, then a default callback will be provided that
    returns the pin number.

child_irq_domain_ops:
    The IRQ domain operations that will be used for this GPIO IRQ
    chip. If no operations are provided, then default callbacks will
    be populated to setup the IRQ hierarchy. Some drivers need to
    supply their own translate function.

These will be initially used by Qualcomm's spmi-gpio and ssbi-gpio.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
Note: checkpatch doesn't like that child_irq_domain_ops is not const.

 drivers/gpio/gpiolib.c      | 52 +++++++++++++++++++++++++++----------
 include/linux/gpio/driver.h | 35 +++++++++++++++++++++++++
 2 files changed, 73 insertions(+), 14 deletions(-)

Comments

Linus Walleij July 28, 2019, 10:49 p.m. UTC | #1
On Mon, Jul 8, 2019 at 1:01 PM Brian Masney <masneyb@onstation.org> wrote:

> Now that the GPIO core has support for hierarchical IRQ chips, let's add
> support for three new callbacks in struct gpio_irq_chip:
>
> populate_parent_fwspec:
>     This optional callback populates the struct irq_fwspec for the
>     parent's IRQ domain. If this is not specified, then
>     gpiochip_populate_parent_fwspec_twocell will be used. A four-cell
>     variant named &gpiochip_populate_parent_fwspec_twocell is also
>     available.
>
> child_pin_to_irq:
>     This optional callback is used to translate the child's GPIO pin
>     number to an IRQ number for the GPIO to_irq() callback. If this is
>     not specified, then a default callback will be provided that
>     returns the pin number.
>
> child_irq_domain_ops:
>     The IRQ domain operations that will be used for this GPIO IRQ
>     chip. If no operations are provided, then default callbacks will
>     be populated to setup the IRQ hierarchy. Some drivers need to
>     supply their own translate function.
>
> These will be initially used by Qualcomm's spmi-gpio and ssbi-gpio.
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>

This is overall looking very appetizing!

I want to apply this on top of my patch and respin it
with some of Masahiro's comments as well and then let's
try to just apply all of this.

> Note: checkpatch doesn't like that child_irq_domain_ops is not const.

Hm? I suspect some janitor will find the problem and patch it for us.

> +static void gpiochip_add_default_irq_domain_ops(struct irq_domain_ops *ops)
> +{
> +       if (!ops->activate)
> +               ops->activate = gpiochip_irq_domain_activate;
> +
> +       if (!ops->deactivate)
> +               ops->deactivate = gpiochip_irq_domain_deactivate;
> +
> +       if (!ops->translate)
> +               ops->translate = gpiochip_hierarchy_irq_domain_translate;
> +
> +       if (!ops->alloc)
> +               ops->alloc = gpiochip_hierarchy_irq_domain_alloc;
> +
> +       if (!ops->free)
> +               ops->free = irq_domain_free_irqs_common;
> +}

I'm fine with translate(), this seems to be what Lina needs too.

But do we really need to make them all optional? activate() and
deactivate() will require the driver to lock the irq etc which is hairy.

Yours,
Linus Walleij
Brian Masney July 29, 2019, 12:11 a.m. UTC | #2
On Mon, Jul 29, 2019 at 12:49:55AM +0200, Linus Walleij wrote:
> On Mon, Jul 8, 2019 at 1:01 PM Brian Masney <masneyb@onstation.org> wrote:
> > +static void gpiochip_add_default_irq_domain_ops(struct irq_domain_ops *ops)
> > +{
> > +       if (!ops->activate)
> > +               ops->activate = gpiochip_irq_domain_activate;
> > +
> > +       if (!ops->deactivate)
> > +               ops->deactivate = gpiochip_irq_domain_deactivate;
> > +
> > +       if (!ops->translate)
> > +               ops->translate = gpiochip_hierarchy_irq_domain_translate;
> > +
> > +       if (!ops->alloc)
> > +               ops->alloc = gpiochip_hierarchy_irq_domain_alloc;
> > +
> > +       if (!ops->free)
> > +               ops->free = irq_domain_free_irqs_common;
> > +}
> 
> I'm fine with translate(), this seems to be what Lina needs too.
> 
> But do we really need to make them all optional? activate() and
> deactivate() will require the driver to lock the irq etc which is hairy.

I can't think of a reason right now that we'd need to override the
others. Since the structure was there, I made all of them optional to
try to future proof this a little bit.

It probably would be better to only make translate optional at this
point. If needed, someone can submit a patch for one or more of the
others with their use case.

Brian

Patch
diff mbox series

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 06c9cf714c99..5423242deb81 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1778,7 +1778,7 @@  static void gpiochip_set_hierarchical_irqchip(struct gpio_chip *gc,
 
 			fwspec.fwnode = gc->irq.fwnode;
 			/* This is the hwirq for the GPIO line side of things */
-			fwspec.param[0] = i;
+			fwspec.param[0] = girq->child_pin_to_irq(gc, i);
 			/* Just pick something */
 			fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
 			fwspec.param_count = 2;
@@ -1841,7 +1841,7 @@  static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
 
 	chip_info(gc, "called %s\n", __func__);
 
-	ret = gpiochip_hierarchy_irq_domain_translate(d, fwspec, &hwirq, &type);
+	ret = gc->irq.child_irq_domain_ops.translate(d, fwspec, &hwirq, &type);
 	if (ret)
 		return ret;
 
@@ -1882,10 +1882,9 @@  static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
 		 * all together up the chain.
 		 */
 		parent_fwspec.fwnode = d->parent->fwnode;
-		parent_fwspec.param_count = 2;
-		parent_fwspec.param[0] = parent_hwirq;
 		/* This parent only handles asserted level IRQs */
-		parent_fwspec.param[1] = parent_type;
+		girq->populate_parent_fwspec(gc, &parent_fwspec, parent_hwirq,
+					     parent_type);
 		chip_info(gc, "alloc_irqs_parent for %d parent hwirq %d\n",
 			  irq + i, parent_hwirq);
 		ret = irq_domain_alloc_irqs_parent(d, irq + i, 1,
@@ -1899,13 +1898,29 @@  static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
 	return 0;
 }
 
-static const struct irq_domain_ops gpiochip_hierarchy_domain_ops = {
-	.activate = gpiochip_irq_domain_activate,
-	.deactivate = gpiochip_irq_domain_deactivate,
-	.translate = gpiochip_hierarchy_irq_domain_translate,
-	.alloc = gpiochip_hierarchy_irq_domain_alloc,
-	.free = irq_domain_free_irqs_common,
-};
+static unsigned int gpiochip_child_pin_to_irq_noop(struct gpio_chip *chip,
+						   unsigned int pin)
+{
+	return pin;
+}
+
+static void gpiochip_add_default_irq_domain_ops(struct irq_domain_ops *ops)
+{
+	if (!ops->activate)
+		ops->activate = gpiochip_irq_domain_activate;
+
+	if (!ops->deactivate)
+		ops->deactivate = gpiochip_irq_domain_deactivate;
+
+	if (!ops->translate)
+		ops->translate = gpiochip_hierarchy_irq_domain_translate;
+
+	if (!ops->alloc)
+		ops->alloc = gpiochip_hierarchy_irq_domain_alloc;
+
+	if (!ops->free)
+		ops->free = irq_domain_free_irqs_common;
+}
 
 static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
 {
@@ -1921,12 +1936,21 @@  static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
 		return -EINVAL;
 	}
 
+	if (!gc->irq.child_pin_to_irq)
+		gc->irq.child_pin_to_irq = gpiochip_child_pin_to_irq_noop;
+
+	if (!gc->irq.populate_parent_fwspec)
+		gc->irq.populate_parent_fwspec =
+			gpiochip_populate_parent_fwspec_twocell;
+
+	gpiochip_add_default_irq_domain_ops(&gc->irq.child_irq_domain_ops);
+
 	gc->irq.domain = irq_domain_create_hierarchy(
 		gc->irq.parent_domain,
 		IRQ_DOMAIN_FLAG_HIERARCHY,
 		gc->ngpio,
 		gc->irq.fwnode,
-		&gpiochip_hierarchy_domain_ops,
+		&gc->irq.child_irq_domain_ops,
 		gc);
 
 	if (!gc->irq.domain) {
@@ -2106,7 +2130,7 @@  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
 
 		spec.fwnode = domain->fwnode;
 		spec.param_count = 2;
-		spec.param[0] = offset;
+		spec.param[0] = chip->irq.child_pin_to_irq(chip, offset);
 		spec.param[1] = IRQ_TYPE_NONE;
 
 		return irq_create_fwspec_mapping(&spec);
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 6b6bca20c8f9..fb9c126dd8f0 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -93,6 +93,41 @@  struct gpio_irq_chip {
 				     unsigned int child_type,
 				     unsigned int *parent_hwirq,
 				     unsigned int *parent_type);
+
+	/**
+	 * @populate_parent_fwspec:
+	 *
+	 * This optional callback populates the &struct irq_fwspec for the
+	 * parent's IRQ domain. If this is not specified, then
+	 * &gpiochip_populate_parent_fwspec_twocell will be used. A four-cell
+	 * variant named &gpiochip_populate_parent_fwspec_twocell is also
+	 * available.
+	 */
+	void (*populate_parent_fwspec)(struct gpio_chip *chip,
+				       struct irq_fwspec *fwspec,
+				       unsigned int parent_hwirq,
+				       unsigned int parent_type);
+
+	/**
+	 * @child_pin_to_irq:
+	 *
+	 * This optional callback is used to translate the child's GPIO pin
+	 * number to an IRQ number for the GPIO to_irq() callback. If this is
+	 * not specified, then a default callback will be provided that
+	 * returns the pin number.
+	 */
+	unsigned int (*child_pin_to_irq)(struct gpio_chip *chip,
+					 unsigned int pin);
+
+	/**
+	 * @child_irq_domain_ops:
+	 *
+	 * The IRQ domain operations that will be used for this GPIO IRQ
+	 * chip. If no operations are provided, then default callbacks will
+	 * be populated to setup the IRQ hierarchy. Some drivers need to
+	 * supply their own translate function.
+	 */
+	struct irq_domain_ops child_irq_domain_ops;
 #endif
 
 	/**