diff mbox series

[v2,4/5] qcom: spmi-gpio: add support for hierarchical IRQ chip

Message ID 20190107021145.6370-5-masneyb@onstation.org
State New
Headers show
Series qcom: spmi: add support for hierarchical IRQ chip | expand

Commit Message

Brian Masney Jan. 7, 2019, 2:11 a.m. UTC 
spmi-gpio did not have any irqchip support so consumers of this in
device tree would need to call gpio[d]_to_irq() in order to get the
proper IRQ on the underlying PMIC. IRQ chips in device tree should
be usable from the start without the consumer having to make an
additional call to get the proper IRQ on the parent. This patch adds
hierarchical IRQ chip support to the spmi-gpio code to correct this
issue.

Driver was tested using the volume buttons (via gpio-keys) on the LG
Nexus 5 (hammerhead) phone with the following two configurations.

volume-up {
	interrupts-extended = <&pm8941_gpios 2 IRQ_TYPE_EDGE_BOTH>;
	...
};

volume-up {
	gpios = <&pm8941_gpios 2 GPIO_ACTIVE_LOW>;
	...
};

Both configurations now show that spmi-gpio is the IRQ domain and that
the IRQ is setup in a hierarchy.

$ grep volume_up /proc/interrupts
 64:          0          0  spmi-gpio   1 Edge      volume_up

$ cat /sys/kernel/debug/irq/irqs/64
handler:  handle_edge_irq
device:   (null)
status:   0x00000403
            _IRQ_NOPROBE
istate:   0x00000000
ddepth:   0
wdepth:   0
dstate:   0x02400203
            IRQ_TYPE_EDGE_RISING
            IRQ_TYPE_EDGE_FALLING
            IRQD_ACTIVATED
            IRQD_IRQ_STARTED
node:     0
affinity: 0-3
effectiv:
domain:  :soc:spmi@fc4cf000:pm8941@0:gpios@c000
 hwirq:   0x1
 chip:    spmi-gpio
  flags:   0x4
             IRQCHIP_MASK_ON_SUSPEND
 parent:
    domain:  :soc:spmi@fc4cf000
     hwirq:   0xc100057
     chip:    pmic_arb
      flags:   0x4
                 IRQCHIP_MASK_ON_SUSPEND

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
Changes since v1:
- Use two cells for interrupts instead of four.
- Pin numbers in interrupts-extended are now one based instead of zero
  based so that they match the GPIO pin number.
- Drop unnecessary parenthesis in pmic_gpio_domain_translate
- Add missing of_node_put()
- Remove irq field from pmic_gpio_pad struct that is no longer
  necessary.

 drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 123 +++++++++++++++++++++--
 1 file changed, 113 insertions(+), 10 deletions(-)

Comments

Stephen Boyd Jan. 7, 2019, 9:55 p.m. UTC | #1 
Quoting Brian Masney (2019-01-06 18:11:44)
> @@ -762,11 +763,14 @@ static int pmic_gpio_of_xlate(struct gpio_chip *chip,
>  static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
>  {
>         struct pmic_gpio_state *state = gpiochip_get_data(chip);
> -       struct pmic_gpio_pad *pad;
> +       struct irq_fwspec fwspec;
>  
> -       pad = state->ctrl->desc->pins[pin].drv_data;
> +       fwspec.fwnode = state->fwnode;
> +       fwspec.param_count = 2;
> +       fwspec.param[0] = pin + 1;

There seems to be PMIC_GPIO_PHYSICAL_OFFSET for these purposes.

> +       fwspec.param[1] = IRQ_TYPE_NONE;
>  
> -       return pad->irq;
> +       return irq_create_fwspec_mapping(&fwspec);
>  }
>  
>  static void pmic_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> @@ -936,6 +940,86 @@ static int pmic_gpio_populate(struct pmic_gpio_state *state,
>         return 0;
>  }
>  
> +static struct irq_chip pmic_gpio_irq_chip = {
> +       .name = "spmi-gpio",
> +       .irq_ack = irq_chip_ack_parent,
> +       .irq_mask = irq_chip_mask_parent,
> +       .irq_unmask = irq_chip_unmask_parent,
> +       .irq_set_type = irq_chip_set_type_parent,
> +       .irq_set_wake = irq_chip_set_wake_parent,
> +       .flags = IRQCHIP_MASK_ON_SUSPEND,
> +};
> +
> +static int pmic_gpio_irq_activate(struct irq_domain *domain,
> +                                 struct irq_data *data, bool reserve)
> +{
> +       struct pmic_gpio_state *state = domain->host_data;
> +
> +       return gpiochip_lock_as_irq(&state->chip, data->hwirq);
> +}
> +
> +static void pmic_gpio_irq_deactivate(struct irq_domain *domain,
> +                                    struct irq_data *data)
> +{
> +       struct pmic_gpio_state *state = domain->host_data;
> +
> +       gpiochip_unlock_as_irq(&state->chip, data->hwirq);
> +}

I meant these sorts of wrappers that could be used by other drivers
possibly, if all they need to store in their domain->host_data is the
gpiochip structure (they can probably indirect through the gpiochip
again to get their driver specific structure).

int gpiochip_irq_domain_activate(struct irq_domain *domain,
				 struct irq_data *data, bool reserve)
{
	struct gpiochip *chip = domain->host_data;

	return gpiochip_lock_as_irq(chip, data->hwirq);
}

void gpiochip_irq_domain_deactivate(struct irq_domain *domain,
				    struct irq_data *data)
{
	struct gpiochip *chip = domain->host_data;

	return gpiochip_unlock_as_irq(chip, data->hwirq);
}

> +
> +static int pmic_gpio_domain_translate(struct irq_domain *domain,
> +                                     struct irq_fwspec *fwspec,
> +                                     unsigned long *hwirq,
> +                                     unsigned int *type)
> +{
> +       struct pmic_gpio_state *state = domain->host_data;
> +
> +       if (fwspec->param_count != 2 || fwspec->param[0] >= state->chip.ngpio)
> +               return -EINVAL;
> +
> +       *hwirq = fwspec->param[0] - 1;

There seems to be PMIC_GPIO_PHYSICAL_OFFSET for these purposes.

> +       *type = fwspec->param[1];
> +
> +       return 0;
> +}
> +
> +static int pmic_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +                                 unsigned int nr_irqs, void *data)
> +{
> +       struct pmic_gpio_state *state = domain->host_data;
> +       struct irq_fwspec *fwspec = data;
> +       struct irq_fwspec parent_fwspec;
> +       irq_hw_number_t hwirq;
> +       unsigned int type;
> +       int ret, i;
> +
> +       ret = pmic_gpio_domain_translate(domain, fwspec, &hwirq, &type);
> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; i < nr_irqs; i++)
> +               irq_domain_set_info(domain, virq + i, hwirq + i,
> +                                   &pmic_gpio_irq_chip, state,
> +                                   handle_level_irq, NULL, NULL);
> +
> +       parent_fwspec.fwnode = domain->parent->fwnode;
> +       parent_fwspec.param_count = 4;
> +       parent_fwspec.param[0] = 0;
> +       parent_fwspec.param[1] = fwspec->param[0] + 0xc0 - 1;

Is the -1 there twice, once in pmic_gpio_domain_translate() and then
here?

> +       parent_fwspec.param[2] = 0;
> +       parent_fwspec.param[3] = fwspec->param[1];
> +
> +       return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> +                                           &parent_fwspec);
> +}
> +
Brian Masney Jan. 8, 2019, 10:35 a.m. UTC | #2 
On Mon, Jan 07, 2019 at 01:55:42PM -0800, Stephen Boyd wrote:
> > +       ret = pmic_gpio_domain_translate(domain, fwspec, &hwirq, &type);
> > +       if (ret)
> > +               return ret;
> > +
> > +       for (i = 0; i < nr_irqs; i++)
> > +               irq_domain_set_info(domain, virq + i, hwirq + i,
> > +                                   &pmic_gpio_irq_chip, state,
> > +                                   handle_level_irq, NULL, NULL);
> > +
> > +       parent_fwspec.fwnode = domain->parent->fwnode;
> > +       parent_fwspec.param_count = 4;
> > +       parent_fwspec.param[0] = 0;
> > +       parent_fwspec.param[1] = fwspec->param[0] + 0xc0 - 1;
> 
> Is the -1 there twice, once in pmic_gpio_domain_translate() and then
> here?

That should be 'hwirq + c0' to improve the code readability.

Brian
diff mbox series

Patch

diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index d910fa6c49fe..66b24fa70b3a 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -12,6 +12,7 @@ 
  */
 
 #include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -137,7 +138,6 @@  enum pmic_gpio_func_index {
 /**
  * struct pmic_gpio_pad - keep current GPIO settings
  * @base: Address base in SPMI device.
- * @irq: IRQ number which this GPIO generate.
  * @is_enabled: Set to false when GPIO should be put in high Z state.
  * @out_value: Cached pin output value
  * @have_buffer: Set to true if GPIO output could be configured in push-pull,
@@ -157,7 +157,6 @@  enum pmic_gpio_func_index {
  */
 struct pmic_gpio_pad {
 	u16		base;
-	int		irq;
 	bool		is_enabled;
 	bool		out_value;
 	bool		have_buffer;
@@ -180,6 +179,8 @@  struct pmic_gpio_state {
 	struct regmap	*map;
 	struct pinctrl_dev *ctrl;
 	struct gpio_chip chip;
+	struct fwnode_handle *fwnode;
+	struct irq_domain *domain;
 };
 
 static const struct pinconf_generic_params pmic_gpio_bindings[] = {
@@ -762,11 +763,14 @@  static int pmic_gpio_of_xlate(struct gpio_chip *chip,
 static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
 {
 	struct pmic_gpio_state *state = gpiochip_get_data(chip);
-	struct pmic_gpio_pad *pad;
+	struct irq_fwspec fwspec;
 
-	pad = state->ctrl->desc->pins[pin].drv_data;
+	fwspec.fwnode = state->fwnode;
+	fwspec.param_count = 2;
+	fwspec.param[0] = pin + 1;
+	fwspec.param[1] = IRQ_TYPE_NONE;
 
-	return pad->irq;
+	return irq_create_fwspec_mapping(&fwspec);
 }
 
 static void pmic_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
@@ -936,6 +940,86 @@  static int pmic_gpio_populate(struct pmic_gpio_state *state,
 	return 0;
 }
 
+static struct irq_chip pmic_gpio_irq_chip = {
+	.name = "spmi-gpio",
+	.irq_ack = irq_chip_ack_parent,
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_set_type = irq_chip_set_type_parent,
+	.irq_set_wake = irq_chip_set_wake_parent,
+	.flags = IRQCHIP_MASK_ON_SUSPEND,
+};
+
+static int pmic_gpio_irq_activate(struct irq_domain *domain,
+				  struct irq_data *data, bool reserve)
+{
+	struct pmic_gpio_state *state = domain->host_data;
+
+	return gpiochip_lock_as_irq(&state->chip, data->hwirq);
+}
+
+static void pmic_gpio_irq_deactivate(struct irq_domain *domain,
+				     struct irq_data *data)
+{
+	struct pmic_gpio_state *state = domain->host_data;
+
+	gpiochip_unlock_as_irq(&state->chip, data->hwirq);
+}
+
+static int pmic_gpio_domain_translate(struct irq_domain *domain,
+				      struct irq_fwspec *fwspec,
+				      unsigned long *hwirq,
+				      unsigned int *type)
+{
+	struct pmic_gpio_state *state = domain->host_data;
+
+	if (fwspec->param_count != 2 || fwspec->param[0] >= state->chip.ngpio)
+		return -EINVAL;
+
+	*hwirq = fwspec->param[0] - 1;
+	*type = fwspec->param[1];
+
+	return 0;
+}
+
+static int pmic_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				  unsigned int nr_irqs, void *data)
+{
+	struct pmic_gpio_state *state = domain->host_data;
+	struct irq_fwspec *fwspec = data;
+	struct irq_fwspec parent_fwspec;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	int ret, i;
+
+	ret = pmic_gpio_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_info(domain, virq + i, hwirq + i,
+				    &pmic_gpio_irq_chip, state,
+				    handle_level_irq, NULL, NULL);
+
+	parent_fwspec.fwnode = domain->parent->fwnode;
+	parent_fwspec.param_count = 4;
+	parent_fwspec.param[0] = 0;
+	parent_fwspec.param[1] = fwspec->param[0] + 0xc0 - 1;
+	parent_fwspec.param[2] = 0;
+	parent_fwspec.param[3] = fwspec->param[1];
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+					    &parent_fwspec);
+}
+
+static const struct irq_domain_ops pmic_gpio_domain_ops = {
+	.activate = pmic_gpio_irq_activate,
+	.alloc = pmic_gpio_domain_alloc,
+	.deactivate = pmic_gpio_irq_deactivate,
+	.free = irq_domain_free_irqs_common,
+	.translate = pmic_gpio_domain_translate,
+};
+
 /* data contains the number of GPIOs */
 static const struct of_device_id pmic_gpio_of_match[] = {
 	{ .compatible = "qcom,pm8916-gpio", .data = (void *) 4 },
@@ -953,6 +1037,8 @@  MODULE_DEVICE_TABLE(of, pmic_gpio_of_match);
 static int pmic_gpio_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *of_id;
+	struct irq_domain *parent_domain;
+	struct device_node *parent_node;
 	struct device *dev = &pdev->dev;
 	struct pinctrl_pin_desc *pindesc;
 	struct pinctrl_desc *pctrldesc;
@@ -1013,10 +1099,6 @@  static int pmic_gpio_probe(struct platform_device *pdev)
 		pindesc->number = i;
 		pindesc->name = pmic_gpio_groups[i];
 
-		pad->irq = platform_get_irq(pdev, i);
-		if (pad->irq < 0)
-			return pad->irq;
-
 		pad->base = reg + i * PMIC_GPIO_ADDRESS_RANGE;
 
 		ret = pmic_gpio_populate(state, pad);
@@ -1036,10 +1118,28 @@  static int pmic_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(state->ctrl))
 		return PTR_ERR(state->ctrl);
 
+	parent_node = of_irq_find_parent(state->dev->of_node);
+	if (!parent_node)
+		return -ENXIO;
+
+	parent_domain = irq_find_host(parent_node);
+	of_node_put(parent_node);
+	if (!parent_domain)
+		return -ENXIO;
+
+	state->fwnode = of_node_to_fwnode(state->dev->of_node);
+	state->domain = irq_domain_create_hierarchy(parent_domain, 0,
+						    state->chip.ngpio,
+						    state->fwnode,
+						    &pmic_gpio_domain_ops,
+						    state);
+	if (!state->domain)
+		return -ENODEV;
+
 	ret = gpiochip_add_data(&state->chip, state);
 	if (ret) {
 		dev_err(state->dev, "can't add gpio chip\n");
-		return ret;
+		goto err_chip_add_data;
 	}
 
 	/*
@@ -1065,6 +1165,8 @@  static int pmic_gpio_probe(struct platform_device *pdev)
 
 err_range:
 	gpiochip_remove(&state->chip);
+err_chip_add_data:
+	irq_domain_remove(state->domain);
 	return ret;
 }
 
@@ -1073,6 +1175,7 @@  static int pmic_gpio_remove(struct platform_device *pdev)
 	struct pmic_gpio_state *state = platform_get_drvdata(pdev);
 
 	gpiochip_remove(&state->chip);
+	irq_domain_remove(state->domain);
 	return 0;
 }