[PATCH 0/9] qcom: ssbi-gpio: add support for hierarchical IRQ chip

[PATCH 0/9] qcom: ssbi-gpio: add support for hierarchical IRQ chip

[Brian Masney]

This patch series adds hierarchical IRQ chip support to ssbi-gpio so
that device tree consumers can request an IRQ directly from the GPIO
block rather than having to request an IRQ from the underlying PMIC.

For more background information, see the email thread with Linus
Walleij's excellent description of the problem at
https://www.spinics.net/lists/linux-gpio/msg34655.html.

This change was not tested on any actual hardware, however the same
change was made to spmi-gpio and tested on a LG Nexus 5 (hammerhead)
phone. That patch series is available at
https://lore.kernel.org/lkml/20190119204252.18370-1-masneyb@onstation.org/
and is queued for the next merge window.

Brian Masney (9):
  pinctrl: qcom: ssbi-gpio: hardcode IRQ counts
  genirq: introduce irq_domain_translate_twocell
  mfd: pm8xxx: convert to v2 irq interfaces to support hierarchical IRQ
    chips
  mfd: pm8xxx: disassociate old virq if hwirq mapping already exists
  qcom: ssbi-gpio: add support for hierarchical IRQ chip
  arm: dts: qcom: apq8064: add interrupt controller properties
  arm: dts: qcom: msm8660: add interrupt controller properties
  arm: dts: qcom: mdm9615: add interrupt controller properties
  mfd: pm8xxx: revert "disassociate old virq if hwirq mapping already
    exists"

 arch/arm/boot/dts/qcom-apq8064.dtsi      |  46 +------
 arch/arm/boot/dts/qcom-mdm9615.dtsi      |   9 +-
 arch/arm/boot/dts/qcom-msm8660.dtsi      |  47 +------
 drivers/mfd/qcom-pm8xxx.c                |  86 +++++++-----
 drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c | 161 +++++++++++++++++++----
 include/linux/irqdomain.h                |   5 +
 kernel/irq/irqdomain.c                   |  21 +++
 7 files changed, 218 insertions(+), 157 deletions(-)

-- 
2.17.2

[PATCH 1/9] pinctrl: qcom: ssbi-gpio: hardcode IRQ counts

[Brian Masney]

The probing of this driver calls platform_irq_count, which will
setup all of the IRQs that are configured in device tree. In
preparation for converting this driver to be a hierarchical IRQ
chip, hardcode the IRQ count based on the hardware type so that all
the IRQs are not configured immediately and are configured on an
as-needed basis later in the boot process. This change will also
allow for the removal of the interrupts property later in this
patch series once the hierarchical IRQ chip support is in.

This patch also removes the generic qcom,ssbi-gpio OF match since we
don't know the number of pins. All of the existing upstream bindings
already include the more-specific binding.

This change was not tested on any actual hardware, however the same
change was made to spmi-gpio and tested on a LG Nexus 5 (hammerhead)
phone.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c b/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
index ded7d765af2e..9a9e9cb80cc5 100644
--- a/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
@@ -665,12 +665,11 @@ static int pm8xxx_pin_populate(struct pm8xxx_gpio *pctrl,
 }
 
 static const struct of_device_id pm8xxx_gpio_of_match[] = {
-	{ .compatible = "qcom,pm8018-gpio" },
-	{ .compatible = "qcom,pm8038-gpio" },
-	{ .compatible = "qcom,pm8058-gpio" },
-	{ .compatible = "qcom,pm8917-gpio" },
-	{ .compatible = "qcom,pm8921-gpio" },
-	{ .compatible = "qcom,ssbi-gpio" },
+	{ .compatible = "qcom,pm8018-gpio", .data = (void *) 6 },
+	{ .compatible = "qcom,pm8038-gpio", .data = (void *) 12 },
+	{ .compatible = "qcom,pm8058-gpio", .data = (void *) 44 },
+	{ .compatible = "qcom,pm8917-gpio", .data = (void *) 38 },
+	{ .compatible = "qcom,pm8921-gpio", .data = (void *) 44 },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, pm8xxx_gpio_of_match);
@@ -687,13 +686,7 @@ static int pm8xxx_gpio_probe(struct platform_device *pdev)
 	if (!pctrl)
 		return -ENOMEM;
 
-	pctrl->dev = &pdev->dev;
-	npins = platform_irq_count(pdev);
-	if (!npins)
-		return -EINVAL;
-	if (npins < 0)
-		return npins;
-	pctrl->npins = npins;
+	npins = (uintptr_t) device_get_match_data(&pdev->dev);
 
 	pctrl->regmap = dev_get_regmap(pdev->dev.parent, NULL);
 	if (!pctrl->regmap) {
-- 
2.17.2

Re: [PATCH 1/9] pinctrl: qcom: ssbi-gpio: hardcode IRQ counts

[Linus Walleij]

Hi Brian!

On Fri, Jan 25, 2019 at 5:23 PM Brian Masney <masneyb@onstation.org> wrote:

> The probing of this driver calls platform_irq_count, which will
> setup all of the IRQs that are configured in device tree. In
> preparation for converting this driver to be a hierarchical IRQ
> chip, hardcode the IRQ count based on the hardware type so that all
> the IRQs are not configured immediately and are configured on an
> as-needed basis later in the boot process. This change will also
> allow for the removal of the interrupts property later in this
> patch series once the hierarchical IRQ chip support is in.
>
> This patch also removes the generic qcom,ssbi-gpio OF match since we
> don't know the number of pins. All of the existing upstream bindings
> already include the more-specific binding.
>
> This change was not tested on any actual hardware, however the same
> change was made to spmi-gpio and tested on a LG Nexus 5 (hammerhead)
> phone.
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>

I found a bug here:

> -       pctrl->dev = &pdev->dev;

Do not delete this line. Subsequent code makes heavy use
of pctrl->dev and crashes.

After fixing this I get other crashes :D but those are from
other patches so I try to locate those problems too.

Yours,
Linus Walleij

Re: [PATCH 1/9] pinctrl: qcom: ssbi-gpio: hardcode IRQ counts

[Linus Walleij]

Hi Brian!

I found a second bug in this patch:

On Fri, Jan 25, 2019 at 5:23 PM Brian Masney <masneyb@onstation.org> wrote:

> -       npins = platform_irq_count(pdev);
(...)
> -       pctrl->npins = npins;

Do not lose this assignment of pctrl->npins.

pctrl->npins is used further down in the code and the gpiochip
does not probe because of zero pins. You could
get rid of the local variable npins and just use pctrl->npins
everywhere though.

Yours,
Linus Walleij

[PATCH 2/9] genirq: introduce irq_domain_translate_twocell

[Brian Masney]

Add a new function irq_domain_translate_twocell() that is to be used as
the translate function in struct irq_domain_ops for v2 IRQ interfaces.
This is based on irq_domain_xlate_twocell().

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 include/linux/irqdomain.h |  5 +++++
 kernel/irq/irqdomain.c    | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 35965f41d7be..fcefe0c7263f 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -419,6 +419,11 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d, struct device_node *ctrlr,
 			const u32 *intspec, unsigned int intsize,
 			irq_hw_number_t *out_hwirq, unsigned int *out_type);
 
+int irq_domain_translate_twocell(struct irq_domain *d,
+				 struct irq_fwspec *fwspec,
+				 unsigned long *out_hwirq,
+				 unsigned int *out_type);
+
 /* IPI functions */
 int irq_reserve_ipi(struct irq_domain *domain, const struct cpumask *dest);
 int irq_destroy_ipi(unsigned int irq, const struct cpumask *dest);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 8b0be4bd6565..15c2a291aa3c 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -968,6 +968,27 @@ const struct irq_domain_ops irq_domain_simple_ops = {
 };
 EXPORT_SYMBOL_GPL(irq_domain_simple_ops);
 
+/**
+ * irq_domain_translate_twocell() - Generic translate for direct two cell
+ * bindings
+ *
+ * Device Tree IRQ specifier translation function which works with two cell
+ * bindings where the cell values map directly to the hwirq number
+ * and linux irq flags.
+ */
+int irq_domain_translate_twocell(struct irq_domain *d,
+				 struct irq_fwspec *fwspec,
+				 unsigned long *out_hwirq,
+				 unsigned int *out_type)
+{
+	if (WARN_ON(fwspec->param_count < 2))
+		return -EINVAL;
+	*out_hwirq = fwspec->param[0];
+	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(irq_domain_translate_twocell);
+
 int irq_domain_alloc_descs(int virq, unsigned int cnt, irq_hw_number_t hwirq,
 			   int node, const struct irq_affinity_desc *affinity)
 {
-- 
2.17.2

Re: [PATCH 2/9] genirq: introduce irq_domain_translate_twocell

[Marc Zyngier]

Hi Brian,

On 25/01/2019 16:22, Brian Masney wrote:
> Add a new function irq_domain_translate_twocell() that is to be used as
> the translate function in struct irq_domain_ops for v2 IRQ interfaces.
> This is based on irq_domain_xlate_twocell().
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  include/linux/irqdomain.h |  5 +++++
>  kernel/irq/irqdomain.c    | 21 +++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 35965f41d7be..fcefe0c7263f 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -419,6 +419,11 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d, struct device_node *ctrlr,
>  			const u32 *intspec, unsigned int intsize,
>  			irq_hw_number_t *out_hwirq, unsigned int *out_type);
>  
> +int irq_domain_translate_twocell(struct irq_domain *d,
> +				 struct irq_fwspec *fwspec,
> +				 unsigned long *out_hwirq,
> +				 unsigned int *out_type);
> +
>  /* IPI functions */
>  int irq_reserve_ipi(struct irq_domain *domain, const struct cpumask *dest);
>  int irq_destroy_ipi(unsigned int irq, const struct cpumask *dest);
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 8b0be4bd6565..15c2a291aa3c 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -968,6 +968,27 @@ const struct irq_domain_ops irq_domain_simple_ops = {
>  };
>  EXPORT_SYMBOL_GPL(irq_domain_simple_ops);
>  
> +/**
> + * irq_domain_translate_twocell() - Generic translate for direct two cell
> + * bindings
> + *
> + * Device Tree IRQ specifier translation function which works with two cell
> + * bindings where the cell values map directly to the hwirq number
> + * and linux irq flags.
> + */
> +int irq_domain_translate_twocell(struct irq_domain *d,
> +				 struct irq_fwspec *fwspec,
> +				 unsigned long *out_hwirq,
> +				 unsigned int *out_type)
> +{
> +	if (WARN_ON(fwspec->param_count < 2))
> +		return -EINVAL;
> +	*out_hwirq = fwspec->param[0];
> +	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(irq_domain_translate_twocell);
> +
>  int irq_domain_alloc_descs(int virq, unsigned int cnt, irq_hw_number_t hwirq,
>  			   int node, const struct irq_affinity_desc *affinity)
>  {
> 

So why not bite the bullet and rewrite irq_domain_xlate_twocell() in
terms of irq_domain_translate_twocell()? Something like this (untested):

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 3366d11c3e02..76d5bc11eea6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -729,16 +729,18 @@ static int irq_domain_translate(struct irq_domain *d,
 	return 0;
 }
 
-static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
+static void of_phandle_args_to_fwspec(struct device_node *np,
+				      const u32 *args,
+				      unsigned int count,
 				      struct irq_fwspec *fwspec)
 {
 	int i;
 
-	fwspec->fwnode = irq_data->np ? &irq_data->np->fwnode : NULL;
-	fwspec->param_count = irq_data->args_count;
+	fwspec->fwnode = np ? &np->fwnode : NULL;
+	fwspec->param_count = count;
 
-	for (i = 0; i < irq_data->args_count; i++)
-		fwspec->param[i] = irq_data->args[i];
+	for (i = 0; i < count; i++)
+		fwspec->param[i] = args[i];
 }
 
 unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
@@ -836,7 +838,8 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
 {
 	struct irq_fwspec fwspec;
 
-	of_phandle_args_to_fwspec(irq_data, &fwspec);
+	of_phandle_args_to_fwspec(irq_data->np, irq_data->args,
+				  irq_data->args_count, &fwspec);
 	return irq_create_fwspec_mapping(&fwspec);
 }
 EXPORT_SYMBOL_GPL(irq_create_of_mapping);
@@ -928,11 +931,10 @@ int irq_domain_xlate_twocell(struct irq_domain *d, struct device_node *ctrlr,
 			const u32 *intspec, unsigned int intsize,
 			irq_hw_number_t *out_hwirq, unsigned int *out_type)
 {
-	if (WARN_ON(intsize < 2))
-		return -EINVAL;
-	*out_hwirq = intspec[0];
-	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
-	return 0;
+	struct irq_fwspec fwspec;
+
+	of_phandle_args_to_fwspec(ctrlr, intspec, intsize, &fwspec);
+	return irq_domain_translate_twocell(d, &fwspec, out_irq, out_type);
 }
 EXPORT_SYMBOL_GPL(irq_domain_xlate_twocell);
 
I find this more palatable, as it shows the direction of travel for the API.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 3/9] mfd: pm8xxx: convert to v2 irq interfaces to support hierarchical IRQ chips

[Brian Masney]

Convert the PM8XXX IRQ code to use the version 2 IRQ interface in order
to support hierarchical IRQ chips. This is necessary so that ssbi-gpio
can be setup as a hierarchical IRQ chip with PM8xxx as the parent. IRQ
chips in device tree should be usable from the start without the
having to make an additional call to gpio[d]_to_irq() to get the proper
IRQ on the parent.

The IRQ handler was hardcoded as handle_level_irq and this patch
properly sets the handler to either handle_edge_irq or handle_level_irq
depending on the IRQ type.

pm8821_irq_domain_ops and pm8821_irq_domain_map are removed by this
patch since the irq_chip is now contained in the pm_irq_data struct, and
that allows us to use a common IRQ mapping function.

This change was not tested on any actual hardware, however the same
change was made to spmi-gpio and tested on a LG Nexus 5 (hammerhead)
phone.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/mfd/qcom-pm8xxx.c | 86 +++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 36 deletions(-)

diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
index e6e8d81c15fd..a976890c4019 100644
--- a/drivers/mfd/qcom-pm8xxx.c
+++ b/drivers/mfd/qcom-pm8xxx.c
@@ -70,20 +70,20 @@
 #define PM8XXX_NR_IRQS		256
 #define PM8821_NR_IRQS		112
 
+struct pm_irq_data {
+	int num_irqs;
+	struct irq_chip *irq_chip;
+	void (*irq_handler)(struct irq_desc *desc);
+};
+
 struct pm_irq_chip {
 	struct regmap		*regmap;
 	spinlock_t		pm_irq_lock;
 	struct irq_domain	*irqdomain;
-	unsigned int		num_irqs;
 	unsigned int		num_blocks;
 	unsigned int		num_masters;
 	u8			config[0];
-};
-
-struct pm_irq_data {
-	int num_irqs;
-	const struct irq_domain_ops  *irq_domain_ops;
-	void (*irq_handler)(struct irq_desc *desc);
+	const struct pm_irq_data *pm_irq_data;
 };
 
 static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
@@ -303,6 +303,7 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type)
 {
 	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
 	unsigned int pmirq = irqd_to_hwirq(d);
+	irq_flow_handler_t flow_handler;
 	int irq_bit;
 	u8 block, config;
 
@@ -316,6 +317,8 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type)
 			chip->config[pmirq] &= ~PM_IRQF_MASK_RE;
 		if (flow_type & IRQF_TRIGGER_FALLING)
 			chip->config[pmirq] &= ~PM_IRQF_MASK_FE;
+
+		flow_handler = handle_edge_irq;
 	} else {
 		chip->config[pmirq] |= PM_IRQF_LVL_SEL;
 
@@ -323,8 +326,12 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type)
 			chip->config[pmirq] &= ~PM_IRQF_MASK_RE;
 		else
 			chip->config[pmirq] &= ~PM_IRQF_MASK_FE;
+
+		flow_handler = handle_level_irq;
 	}
 
+	irq_set_handler_locked(d, flow_handler);
+
 	config = chip->config[pmirq] | PM_IRQF_CLR;
 	return pm8xxx_config_irq(chip, block, config);
 }
@@ -375,21 +382,45 @@ static struct irq_chip pm8xxx_irq_chip = {
 	.flags		= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
 };
 
-static int pm8xxx_irq_domain_map(struct irq_domain *d, unsigned int irq,
-				   irq_hw_number_t hwirq)
+static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
+				  struct irq_domain *domain, unsigned int irq,
+				  irq_hw_number_t hwirq, unsigned int type)
 {
-	struct pm_irq_chip *chip = d->host_data;
+	irq_flow_handler_t handler;
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		handler = handle_edge_irq;
+	else
+		handler = handle_level_irq;
 
-	irq_set_chip_and_handler(irq, &pm8xxx_irq_chip, handle_level_irq);
-	irq_set_chip_data(irq, chip);
+	irq_domain_set_info(domain, irq, hwirq, chip->pm_irq_data->irq_chip,
+			    chip, handler, NULL, NULL);
 	irq_set_noprobe(irq);
+}
+
+static int pm8xxx_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				   unsigned int nr_irqs, void *data)
+{
+	struct pm_irq_chip *chip = domain->host_data;
+	struct irq_fwspec *fwspec = data;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	int ret, i;
+
+	ret = irq_domain_translate_twocell(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < nr_irqs; i++)
+		pm8xxx_irq_domain_map(chip, domain, virq + i, hwirq + i, type);
 
 	return 0;
 }
 
 static const struct irq_domain_ops pm8xxx_irq_domain_ops = {
-	.xlate = irq_domain_xlate_twocell,
-	.map = pm8xxx_irq_domain_map,
+	.alloc = pm8xxx_irq_domain_alloc,
+	.free = irq_domain_free_irqs_common,
+	.translate = irq_domain_translate_twocell,
 };
 
 static void pm8821_irq_mask_ack(struct irq_data *d)
@@ -473,23 +504,6 @@ static struct irq_chip pm8821_irq_chip = {
 	.flags		= IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
 };
 
-static int pm8821_irq_domain_map(struct irq_domain *d, unsigned int irq,
-				   irq_hw_number_t hwirq)
-{
-	struct pm_irq_chip *chip = d->host_data;
-
-	irq_set_chip_and_handler(irq, &pm8821_irq_chip, handle_level_irq);
-	irq_set_chip_data(irq, chip);
-	irq_set_noprobe(irq);
-
-	return 0;
-}
-
-static const struct irq_domain_ops pm8821_irq_domain_ops = {
-	.xlate = irq_domain_xlate_twocell,
-	.map = pm8821_irq_domain_map,
-};
-
 static const struct regmap_config ssbi_regmap_config = {
 	.reg_bits = 16,
 	.val_bits = 8,
@@ -501,13 +515,13 @@ static const struct regmap_config ssbi_regmap_config = {
 
 static const struct pm_irq_data pm8xxx_data = {
 	.num_irqs = PM8XXX_NR_IRQS,
-	.irq_domain_ops = &pm8xxx_irq_domain_ops,
+	.irq_chip = &pm8xxx_irq_chip,
 	.irq_handler = pm8xxx_irq_handler,
 };
 
 static const struct pm_irq_data pm8821_data = {
 	.num_irqs = PM8821_NR_IRQS,
-	.irq_domain_ops = &pm8821_irq_domain_ops,
+	.irq_chip = &pm8821_irq_chip,
 	.irq_handler = pm8821_irq_handler,
 };
 
@@ -571,14 +585,14 @@ static int pm8xxx_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, chip);
 	chip->regmap = regmap;
-	chip->num_irqs = data->num_irqs;
-	chip->num_blocks = DIV_ROUND_UP(chip->num_irqs, 8);
+	chip->num_blocks = DIV_ROUND_UP(data->num_irqs, 8);
 	chip->num_masters = DIV_ROUND_UP(chip->num_blocks, 8);
+	chip->pm_irq_data = data;
 	spin_lock_init(&chip->pm_irq_lock);
 
 	chip->irqdomain = irq_domain_add_linear(pdev->dev.of_node,
 						data->num_irqs,
-						data->irq_domain_ops,
+						&pm8xxx_irq_domain_ops,
 						chip);
 	if (!chip->irqdomain)
 		return -ENODEV;
-- 
2.17.2

Re: [PATCH 3/9] mfd: pm8xxx: convert to v2 irq interfaces to support hierarchical IRQ chips

[Lee Jones]

On Fri, 25 Jan 2019, Brian Masney wrote:

> Convert the PM8XXX IRQ code to use the version 2 IRQ interface in order
> to support hierarchical IRQ chips. This is necessary so that ssbi-gpio
> can be setup as a hierarchical IRQ chip with PM8xxx as the parent. IRQ
> chips in device tree should be usable from the start without the
> having to make an additional call to gpio[d]_to_irq() to get the proper
> IRQ on the parent.
> 
> The IRQ handler was hardcoded as handle_level_irq and this patch
> properly sets the handler to either handle_edge_irq or handle_level_irq
> depending on the IRQ type.
> 
> pm8821_irq_domain_ops and pm8821_irq_domain_map are removed by this
> patch since the irq_chip is now contained in the pm_irq_data struct, and
> that allows us to use a common IRQ mapping function.
> 
> This change was not tested on any actual hardware, however the same
> change was made to spmi-gpio and tested on a LG Nexus 5 (hammerhead)
> phone.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/mfd/qcom-pm8xxx.c | 86 +++++++++++++++++++++++----------------
>  1 file changed, 50 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> index e6e8d81c15fd..a976890c4019 100644
> --- a/drivers/mfd/qcom-pm8xxx.c
> +++ b/drivers/mfd/qcom-pm8xxx.c
> @@ -70,20 +70,20 @@
>  #define PM8XXX_NR_IRQS		256
>  #define PM8821_NR_IRQS		112
>  
> +struct pm_irq_data {
> +	int num_irqs;
> +	struct irq_chip *irq_chip;
> +	void (*irq_handler)(struct irq_desc *desc);
> +};
> +
>  struct pm_irq_chip {
>  	struct regmap		*regmap;
>  	spinlock_t		pm_irq_lock;
>  	struct irq_domain	*irqdomain;
> -	unsigned int		num_irqs;
>  	unsigned int		num_blocks;
>  	unsigned int		num_masters;
>  	u8			config[0];
> -};
> -
> -struct pm_irq_data {
> -	int num_irqs;
> -	const struct irq_domain_ops  *irq_domain_ops;
> -	void (*irq_handler)(struct irq_desc *desc);
> +	const struct pm_irq_data *pm_irq_data;
>  };
>  
>  static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
> @@ -303,6 +303,7 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type)
>  {
>  	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>  	unsigned int pmirq = irqd_to_hwirq(d);
> +	irq_flow_handler_t flow_handler;
>  	int irq_bit;
>  	u8 block, config;
>  
> @@ -316,6 +317,8 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type)
>  			chip->config[pmirq] &= ~PM_IRQF_MASK_RE;
>  		if (flow_type & IRQF_TRIGGER_FALLING)
>  			chip->config[pmirq] &= ~PM_IRQF_MASK_FE;
> +
> +		flow_handler = handle_edge_irq;
>  	} else {
>  		chip->config[pmirq] |= PM_IRQF_LVL_SEL;
>  
> @@ -323,8 +326,12 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type)
>  			chip->config[pmirq] &= ~PM_IRQF_MASK_RE;
>  		else
>  			chip->config[pmirq] &= ~PM_IRQF_MASK_FE;
> +
> +		flow_handler = handle_level_irq;
>  	}
>  
> +	irq_set_handler_locked(d, flow_handler);
> +

Why don't you save yourself 3 lines of code and a variable and just
call irq_set_handler_locked() where you set flow_handler?

Apart from that nit, the code looks good to me.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/9] mfd: pm8xxx: convert to v2 irq interfaces to support hierarchical IRQ chips

[Brian Masney]

On Wed, Jan 30, 2019 at 01:27:39PM +0000, Lee Jones wrote:
> > @@ -303,6 +303,7 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type)
> >  {
> >  	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> >  	unsigned int pmirq = irqd_to_hwirq(d);
> > +	irq_flow_handler_t flow_handler;
> >  	int irq_bit;
> >  	u8 block, config;
> >  
> > @@ -316,6 +317,8 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type)
> >  			chip->config[pmirq] &= ~PM_IRQF_MASK_RE;
> >  		if (flow_type & IRQF_TRIGGER_FALLING)
> >  			chip->config[pmirq] &= ~PM_IRQF_MASK_FE;
> > +
> > +		flow_handler = handle_edge_irq;
> >  	} else {
> >  		chip->config[pmirq] |= PM_IRQF_LVL_SEL;
> >  
> > @@ -323,8 +326,12 @@ static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type)
> >  			chip->config[pmirq] &= ~PM_IRQF_MASK_RE;
> >  		else
> >  			chip->config[pmirq] &= ~PM_IRQF_MASK_FE;
> > +
> > +		flow_handler = handle_level_irq;
> >  	}
> >  
> > +	irq_set_handler_locked(d, flow_handler);
> > +
> 
> Why don't you save yourself 3 lines of code and a variable and just
> call irq_set_handler_locked() where you set flow_handler?
> 
> Apart from that nit, the code looks good to me.

OK.... I'll fix this up in v2.

I'll let this ssbi-gpio series sit on the list for a few weeks before I
send out v2. I ordered a Sony Xperia Z on eBay but it won't be here
until the beginning of March. I'd like to have this series tested on
actual hardware before this series is merged just to be sure it works
properly. If someone else has time to test it sooner, then please do.

Brian

Re: [PATCH 3/9] mfd: pm8xxx: convert to v2 irq interfaces to support hierarchical IRQ chips

[Linus Walleij]

Hi Brian!

Thanks for this patch!

On Fri, Jan 25, 2019 at 5:23 PM Brian Masney <masneyb@onstation.org> wrote:

> The IRQ handler was hardcoded as handle_level_irq and this patch
> properly sets the handler to either handle_edge_irq or handle_level_irq
> depending on the IRQ type.

Done like so:

> +               flow_handler = handle_edge_irq;

This creates a problem and makes my kernel crash, because
handle_edge_irq() *unconditionally* calls chip->irq_ack(),
and since this chip does not implement .irq_ack(), it crashes.

I tried to just add a void .irq_ack(). That instead made the
kernel hang.

If I just assign handle_level_irq() as before (just replaced
all assignments of handle_edge_irq with handle_level_irq)
it boots nicely, AND the interrupts are working fine on the
ADC calibration path, and further readings of the ADC
generates new IRQs, as expected!

The handle_edge_irq and .irq_ack() call path is for hardware
that require an explicit ACK (such as writing a bit in some
register) when handling edge IRQs. This is necessary in some
hardware because of the transient nature of edge IRQs.
This is not necessary on some hardware that will simply
clear the flag when something is reading the status register.
I think this is the case with PM8xxx and you can safely use
handle_level_irq with all IRQs. It looks counterintuitive and
maybe the function handle_edge_irq() has a stupid name.
Just drop this handle_edge_irq() business for now.

This was tested on the APQ8060 DragonBoard.

However this happens:

[    3.007727] mmci-pl18x 12180000.sdcc: ignoring dependency for
device, assuming no driver
[    3.007970] mmci-pl18x 12200000.sdcc: ignoring dependency for
device, assuming no driver
[    3.014984] mpu3050-i2c 1-0068: ignoring dependency for device,
assuming no driver
[    3.023543] cm3605 cm3605: ignoring dependency for device, assuming no driver
[    3.030532] ak8975 1-000c: ignoring dependency for device, assuming no driver
[    3.037729] bmp280 1-0077: ignoring dependency for device, assuming no driver
[    3.044789] reg-fixed-voltage regulators:xc622a331mrg: ignoring
dependency for device, assuming no driver
[    3.052485] smsc911x 1b800000.ethernet-ebi2: ignoring dependency
for device, assuming no driver

But I think that is because these all use ssbi-gpio interrupts and
these are now handled properly so I will try to fix the device
tree and send a separate patch that you can include.

Yours,
Linus Walleij

Re: [PATCH 3/9] mfd: pm8xxx: convert to v2 irq interfaces to support hierarchical IRQ chips

[Linus Walleij]

Hi Brian!

I found one more bug in this patch, still not the last bug but I'm still
digging around:

On Fri, Jan 25, 2019 at 5:23 PM Brian Masney <masneyb@onstation.org> wrote:

> +struct pm_irq_data {
> +       int num_irqs;
> +       struct irq_chip *irq_chip;
> +       void (*irq_handler)(struct irq_desc *desc);
> +};
> +
>  struct pm_irq_chip {
>         struct regmap           *regmap;
>         spinlock_t              pm_irq_lock;
>         struct irq_domain       *irqdomain;
> -       unsigned int            num_irqs;
>         unsigned int            num_blocks;
>         unsigned int            num_masters;
>         u8                      config[0];
> -};
> -
> -struct pm_irq_data {
> -       int num_irqs;
> -       const struct irq_domain_ops  *irq_domain_ops;
> -       void (*irq_handler)(struct irq_desc *desc);
> +       const struct pm_irq_data *pm_irq_data;
>  };

This doesn't work: the config[0] must be the tail element
of the struct since we allocate dynamically the trailing
config[] bytes.

As it looks now, the *pm_irq_data gets overwritten by
the configs and it crashes.

Yours,
Linus Walleij

Re: [PATCH 3/9] mfd: pm8xxx: convert to v2 irq interfaces to support hierarchical IRQ chips

[Brian Masney]

Hi Linus,

On Wed, Feb 06, 2019 at 02:07:52PM +0100, Linus Walleij wrote:
> > +struct pm_irq_data {
> > +       int num_irqs;
> > +       struct irq_chip *irq_chip;
> > +       void (*irq_handler)(struct irq_desc *desc);
> > +};
> > +
> >  struct pm_irq_chip {
> >         struct regmap           *regmap;
> >         spinlock_t              pm_irq_lock;
> >         struct irq_domain       *irqdomain;
> > -       unsigned int            num_irqs;
> >         unsigned int            num_blocks;
> >         unsigned int            num_masters;
> >         u8                      config[0];
> > -};
> > -
> > -struct pm_irq_data {
> > -       int num_irqs;
> > -       const struct irq_domain_ops  *irq_domain_ops;
> > -       void (*irq_handler)(struct irq_desc *desc);
> > +       const struct pm_irq_data *pm_irq_data;
> >  };
> 
> This doesn't work: the config[0] must be the tail element
> of the struct since we allocate dynamically the trailing
> config[] bytes.
> 
> As it looks now, the *pm_irq_data gets overwritten by
> the configs and it crashes.

Thank you for testing all of this on actual hardware. You can either
send out the little issues like this that need corrected and I'll
collect everything up, and send out a V2 once you are done with testing.
Or, you can just take my patches, incorporate the fixes, and add me
with a Co-developed-by tag to the relevant patches. Whatever is easier
for you. I assume that the latter approach may be easier since you're
already making the changes in your tree for testing.

Brian

Re: [PATCH 3/9] mfd: pm8xxx: convert to v2 irq interfaces to support hierarchical IRQ chips

[Linus Walleij]

On Wed, Feb 6, 2019 at 3:10 PM Brian Masney <masneyb@onstation.org> wrote:

> Thank you for testing all of this on actual hardware. You can either
> send out the little issues like this that need corrected and I'll
> collect everything up, and send out a V2 once you are done with testing.
> Or, you can just take my patches, incorporate the fixes, and add me
> with a Co-developed-by tag to the relevant patches. Whatever is easier
> for you. I assume that the latter approach may be easier since you're
> already making the changes in your tree for testing.

Well there are some remarks from Marc you need to address in v2
as well so you better keep the master patch set for now.

I can copy/paste the diff I have though, but there is some bug
I still need to dig into...

Yours,
Linus Walleij

[PATCH 4/9] mfd: pm8xxx: disassociate old virq if hwirq mapping already exists

[Brian Masney]

Check to see if the hwirq is already associated with another virq on
this IRQ domain. If so, then disassociate it before associating the
hwirq with the new virq.

This is a temporary hack that is needed in order to not break git
bisect for existing boards. The next patch in this series converts
ssbi-gpio to be a hierarchical IRQ chip, then there are several patches
to update all of the device tree files, and finally this patch will be
reverted within the same patch series.

IRQs for ssbi-gpio are all initially setup without an IRQ hierarchy
this driver is probed due to the interrupts property in device tree.
Once ssbi-gpio is converted to be a hierarchical IRQ chip in the next
patch, existing users of gpio[d]_to_irq() will call pmic_gpio_to_irq(),
and that will use the new IRQ chip code in ssbi-gpio that sets up the
IRQ in an IRQ hierarchy. The hwirq is now associated with two Linux
virqs and interrupts will not work as expected. This patch corrects
that issue.

This change was not tested on any actual hardware, however the same
change was made to spmi-pmic-arb.c and tested on a LG Nexus 5
(hammerhead) phone.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/mfd/qcom-pm8xxx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
index a976890c4019..97b931465601 100644
--- a/drivers/mfd/qcom-pm8xxx.c
+++ b/drivers/mfd/qcom-pm8xxx.c
@@ -387,6 +387,11 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
 				  irq_hw_number_t hwirq, unsigned int type)
 {
 	irq_flow_handler_t handler;
+	unsigned int old_virq;
+
+	old_virq = irq_find_mapping(domain, hwirq);
+	if (old_virq)
+		irq_domain_disassociate(domain, old_virq);
 
 	if (type & IRQ_TYPE_EDGE_BOTH)
 		handler = handle_edge_irq;
-- 
2.17.2

Re: [PATCH 4/9] mfd: pm8xxx: disassociate old virq if hwirq mapping already exists

[Lee Jones]

Thomas,

On Fri, 25 Jan 2019, Brian Masney wrote:

> Check to see if the hwirq is already associated with another virq on
> this IRQ domain. If so, then disassociate it before associating the
> hwirq with the new virq.
> 
> This is a temporary hack that is needed in order to not break git
> bisect for existing boards. The next patch in this series converts
> ssbi-gpio to be a hierarchical IRQ chip, then there are several patches
> to update all of the device tree files, and finally this patch will be
> reverted within the same patch series.
> 
> IRQs for ssbi-gpio are all initially setup without an IRQ hierarchy
> this driver is probed due to the interrupts property in device tree.
> Once ssbi-gpio is converted to be a hierarchical IRQ chip in the next
> patch, existing users of gpio[d]_to_irq() will call pmic_gpio_to_irq(),
> and that will use the new IRQ chip code in ssbi-gpio that sets up the
> IRQ in an IRQ hierarchy. The hwirq is now associated with two Linux
> virqs and interrupts will not work as expected. This patch corrects
> that issue.

I guess that's okay.  Would quite like a second opinion though.

> This change was not tested on any actual hardware, however the same
> change was made to spmi-pmic-arb.c and tested on a LG Nexus 5
> (hammerhead) phone.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/mfd/qcom-pm8xxx.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> index a976890c4019..97b931465601 100644
> --- a/drivers/mfd/qcom-pm8xxx.c
> +++ b/drivers/mfd/qcom-pm8xxx.c
> @@ -387,6 +387,11 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
>  				  irq_hw_number_t hwirq, unsigned int type)
>  {
>  	irq_flow_handler_t handler;
> +	unsigned int old_virq;
> +
> +	old_virq = irq_find_mapping(domain, hwirq);
> +	if (old_virq)
> +		irq_domain_disassociate(domain, old_virq);
>  
>  	if (type & IRQ_TYPE_EDGE_BOTH)
>  		handler = handle_edge_irq;

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 5/9] qcom: ssbi-gpio: add support for hierarchical IRQ chip

[Brian Masney]

ssbi-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 ssbi-gpio code to correct this
issue.

The constant PM8XXX_GPIO_PHYSICAL_OFFSET is introduced to replace the
hardcoded '1' that previously existed in two places in this driver to
improve code readability.

This change was not tested on any actual hardware, however the same
change was made to spmi-gpio and tested on a LG Nexus 5 (hammerhead)
phone.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c | 142 +++++++++++++++++++++--
 1 file changed, 130 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c b/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
index 9a9e9cb80cc5..c6b606b225a9 100644
--- a/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
@@ -55,6 +55,8 @@
 
 #define PM8XXX_MAX_GPIOS               44
 
+#define PM8XXX_GPIO_PHYSICAL_OFFSET	1
+
 /* custom pinconf parameters */
 #define PM8XXX_QCOM_DRIVE_STRENGH      (PIN_CONFIG_END + 1)
 #define PM8XXX_QCOM_PULL_UP_STRENGTH   (PIN_CONFIG_END + 2)
@@ -99,6 +101,9 @@ struct pm8xxx_gpio {
 
 	struct pinctrl_desc desc;
 	unsigned npins;
+
+	struct fwnode_handle *fwnode;
+	struct irq_domain *domain;
 };
 
 static const struct pinconf_generic_params pm8xxx_gpio_bindings[] = {
@@ -499,11 +504,12 @@ static int pm8xxx_gpio_get(struct gpio_chip *chip, unsigned offset)
 
 	if (pin->mode == PM8XXX_GPIO_MODE_OUTPUT) {
 		ret = pin->output_value;
-	} else {
+	} else if (pin->irq >= 0) {
 		ret = irq_get_irqchip_state(pin->irq, IRQCHIP_STATE_LINE_LEVEL, &state);
 		if (!ret)
 			ret = !!state;
-	}
+	} else
+		ret = -EINVAL;
 
 	return ret;
 }
@@ -533,16 +539,39 @@ static int pm8xxx_gpio_of_xlate(struct gpio_chip *chip,
 	if (flags)
 		*flags = gpio_desc->args[1];
 
-	return gpio_desc->args[0] - 1;
+	return gpio_desc->args[0] - PM8XXX_GPIO_PHYSICAL_OFFSET;
 }
 
 
 static int pm8xxx_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	struct pm8xxx_gpio *pctrl = gpiochip_get_data(chip);
+	struct pm8xxx_pin_data *pin = pctrl->desc.pins[offset].drv_data;
+	struct irq_fwspec fwspec;
+	int ret;
+
+	fwspec.fwnode = pctrl->fwnode;
+	fwspec.param_count = 2;
+	fwspec.param[0] = offset + PM8XXX_GPIO_PHYSICAL_OFFSET;
+	fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
+
+	ret = irq_create_fwspec_mapping(&fwspec);
+
+	/*
+	 * Cache the IRQ since pm8xxx_gpio_get() needs this to get determine the
+	 * line level.
+	 */
+	pin->irq = ret;
+
+	return ret;
+}
+
+static void pm8xxx_gpio_free(struct gpio_chip *chip, unsigned int offset)
 {
 	struct pm8xxx_gpio *pctrl = gpiochip_get_data(chip);
 	struct pm8xxx_pin_data *pin = pctrl->desc.pins[offset].drv_data;
 
-	return pin->irq;
+	pin->irq = -1;
 }
 
 #ifdef CONFIG_DEBUG_FS
@@ -571,7 +600,7 @@ static void pm8xxx_gpio_dbg_show_one(struct seq_file *s,
 		"no", "high", "medium", "low"
 	};
 
-	seq_printf(s, " gpio%-2d:", offset + 1);
+	seq_printf(s, " gpio%-2d:", offset + PM8XXX_GPIO_PHYSICAL_OFFSET);
 	if (pin->disable) {
 		seq_puts(s, " ---");
 	} else {
@@ -603,6 +632,7 @@ static void pm8xxx_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 #endif
 
 static const struct gpio_chip pm8xxx_gpio_template = {
+	.free = pm8xxx_gpio_free,
 	.direction_input = pm8xxx_gpio_direction_input,
 	.direction_output = pm8xxx_gpio_direction_output,
 	.get = pm8xxx_gpio_get,
@@ -664,6 +694,75 @@ static int pm8xxx_pin_populate(struct pm8xxx_gpio *pctrl,
 	return 0;
 }
 
+static struct irq_chip pm8xxx_irq_chip = {
+	.name = "ssbi-gpio",
+	.irq_mask = irq_chip_mask_parent,
+	.irq_ack = irq_chip_ack_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_set_type = irq_chip_set_type_parent,
+	.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
+};
+
+static int pm8xxx_domain_translate(struct irq_domain *domain,
+				   struct irq_fwspec *fwspec,
+				   unsigned long *hwirq,
+				   unsigned int *type)
+{
+	struct pm8xxx_gpio *pctrl = container_of(domain->host_data,
+						 struct pm8xxx_gpio, chip);
+
+	if (fwspec->param_count != 2 || fwspec->param[0] >= pctrl->chip.ngpio)
+		return -EINVAL;
+
+	*hwirq = fwspec->param[0] - PM8XXX_GPIO_PHYSICAL_OFFSET;
+	*type = fwspec->param[1];
+
+	return 0;
+}
+
+static int pm8xxx_domain_alloc(struct irq_domain *domain, unsigned int virq,
+			       unsigned int nr_irqs, void *data)
+{
+	struct pm8xxx_gpio *pctrl = container_of(domain->host_data,
+						 struct pm8xxx_gpio, chip);
+	struct irq_fwspec *fwspec = data;
+	struct irq_fwspec parent_fwspec;
+	irq_flow_handler_t handler;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	int ret, i;
+
+	ret = pm8xxx_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		handler = handle_edge_irq;
+	else
+		handler = handle_level_irq;
+
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_info(domain, virq + i, hwirq + i,
+				    &pm8xxx_irq_chip, pctrl, handler,
+				    NULL, NULL);
+
+	parent_fwspec.fwnode = domain->parent->fwnode;
+	parent_fwspec.param_count = 2;
+	parent_fwspec.param[0] = hwirq + 0xc0;
+	parent_fwspec.param[1] = fwspec->param[1];
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+					    &parent_fwspec);
+}
+
+static const struct irq_domain_ops pm8xxx_domain_ops = {
+	.activate = gpiochip_irq_domain_activate,
+	.alloc = pm8xxx_domain_alloc,
+	.deactivate = gpiochip_irq_domain_deactivate,
+	.free = irq_domain_free_irqs_common,
+	.translate = pm8xxx_domain_translate,
+};
+
 static const struct of_device_id pm8xxx_gpio_of_match[] = {
 	{ .compatible = "qcom,pm8018-gpio", .data = (void *) 6 },
 	{ .compatible = "qcom,pm8038-gpio", .data = (void *) 12 },
@@ -677,6 +776,8 @@ MODULE_DEVICE_TABLE(of, pm8xxx_gpio_of_match);
 static int pm8xxx_gpio_probe(struct platform_device *pdev)
 {
 	struct pm8xxx_pin_data *pin_data;
+	struct irq_domain *parent_domain;
+	struct device_node *parent_node;
 	struct pinctrl_pin_desc *pins;
 	struct pm8xxx_gpio *pctrl;
 	int ret;
@@ -713,12 +814,7 @@ static int pm8xxx_gpio_probe(struct platform_device *pdev)
 
 	for (i = 0; i < pctrl->desc.npins; i++) {
 		pin_data[i].reg = SSBI_REG_ADDR_GPIO(i);
-		pin_data[i].irq = platform_get_irq(pdev, i);
-		if (pin_data[i].irq < 0) {
-			dev_err(&pdev->dev,
-				"missing interrupts for pin %d\n", i);
-			return pin_data[i].irq;
-		}
+		pin_data[i].irq = -1;
 
 		ret = pm8xxx_pin_populate(pctrl, &pin_data[i]);
 		if (ret)
@@ -749,10 +845,29 @@ static int pm8xxx_gpio_probe(struct platform_device *pdev)
 	pctrl->chip.of_gpio_n_cells = 2;
 	pctrl->chip.label = dev_name(pctrl->dev);
 	pctrl->chip.ngpio = pctrl->npins;
+
+	parent_node = of_irq_find_parent(pctrl->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;
+
+	pctrl->fwnode = of_node_to_fwnode(pctrl->dev->of_node);
+	pctrl->domain = irq_domain_create_hierarchy(parent_domain, 0,
+						    pctrl->chip.ngpio,
+						    pctrl->fwnode,
+						    &pm8xxx_domain_ops,
+						    &pctrl->chip);
+	if (!pctrl->domain)
+		return -ENODEV;
+
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
 		dev_err(&pdev->dev, "failed register gpiochip\n");
-		return ret;
+		goto err_chip_add_data;
 	}
 
 	/*
@@ -782,6 +897,8 @@ static int pm8xxx_gpio_probe(struct platform_device *pdev)
 
 unregister_gpiochip:
 	gpiochip_remove(&pctrl->chip);
+err_chip_add_data:
+	irq_domain_remove(pctrl->domain);
 
 	return ret;
 }
@@ -791,6 +908,7 @@ static int pm8xxx_gpio_remove(struct platform_device *pdev)
 	struct pm8xxx_gpio *pctrl = platform_get_drvdata(pdev);
 
 	gpiochip_remove(&pctrl->chip);
+	irq_domain_remove(pctrl->domain);
 
 	return 0;
 }
-- 
2.17.2

[PATCH 6/9] arm: dts: qcom: apq8064: add interrupt controller properties

[Brian Masney]

Add interrupt controller properties now that ssbi-gpio is a proper
hierarchical IRQ chip. The interrupts property is no longer needed so
remove it.

This change was not tested on any hardware but the same change was
tested on qcom-pm8941.dtsi with spmi-gpio using a LG Nexus 5
(hammerhead) phone with no issues.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 arch/arm/boot/dts/qcom-apq8064.dtsi | 46 ++---------------------------
 1 file changed, 2 insertions(+), 44 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
index 48c3cf427610..4744fe757cf4 100644
--- a/arch/arm/boot/dts/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
@@ -705,50 +705,8 @@
 					compatible = "qcom,pm8921-gpio",
 						     "qcom,ssbi-gpio";
 					reg = <0x150>;
-					interrupts = <192 IRQ_TYPE_NONE>,
-						     <193 IRQ_TYPE_NONE>,
-						     <194 IRQ_TYPE_NONE>,
-						     <195 IRQ_TYPE_NONE>,
-						     <196 IRQ_TYPE_NONE>,
-						     <197 IRQ_TYPE_NONE>,
-						     <198 IRQ_TYPE_NONE>,
-						     <199 IRQ_TYPE_NONE>,
-						     <200 IRQ_TYPE_NONE>,
-						     <201 IRQ_TYPE_NONE>,
-						     <202 IRQ_TYPE_NONE>,
-						     <203 IRQ_TYPE_NONE>,
-						     <204 IRQ_TYPE_NONE>,
-						     <205 IRQ_TYPE_NONE>,
-						     <206 IRQ_TYPE_NONE>,
-						     <207 IRQ_TYPE_NONE>,
-						     <208 IRQ_TYPE_NONE>,
-						     <209 IRQ_TYPE_NONE>,
-						     <210 IRQ_TYPE_NONE>,
-						     <211 IRQ_TYPE_NONE>,
-						     <212 IRQ_TYPE_NONE>,
-						     <213 IRQ_TYPE_NONE>,
-						     <214 IRQ_TYPE_NONE>,
-						     <215 IRQ_TYPE_NONE>,
-						     <216 IRQ_TYPE_NONE>,
-						     <217 IRQ_TYPE_NONE>,
-						     <218 IRQ_TYPE_NONE>,
-						     <219 IRQ_TYPE_NONE>,
-						     <220 IRQ_TYPE_NONE>,
-						     <221 IRQ_TYPE_NONE>,
-						     <222 IRQ_TYPE_NONE>,
-						     <223 IRQ_TYPE_NONE>,
-						     <224 IRQ_TYPE_NONE>,
-						     <225 IRQ_TYPE_NONE>,
-						     <226 IRQ_TYPE_NONE>,
-						     <227 IRQ_TYPE_NONE>,
-						     <228 IRQ_TYPE_NONE>,
-						     <229 IRQ_TYPE_NONE>,
-						     <230 IRQ_TYPE_NONE>,
-						     <231 IRQ_TYPE_NONE>,
-						     <232 IRQ_TYPE_NONE>,
-						     <233 IRQ_TYPE_NONE>,
-						     <234 IRQ_TYPE_NONE>,
-						     <235 IRQ_TYPE_NONE>;
+					interrupt-controller;
+					#interrupt-cells = <2>;
 					gpio-controller;
 					#gpio-cells = <2>;
 
-- 
2.17.2

[PATCH 7/9] arm: dts: qcom: msm8660: add interrupt controller properties

[Brian Masney]

Add interrupt controller properties now that ssbi-gpio is a proper
hierarchical IRQ chip. The interrupts property is no longer needed so
remove it.

This change was not tested on any hardware but the same change was
tested on qcom-pm8941.dtsi with spmi-gpio using a LG Nexus 5
(hammerhead) phone with no issues.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 arch/arm/boot/dts/qcom-msm8660.dtsi | 47 ++---------------------------
 1 file changed, 2 insertions(+), 45 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-msm8660.dtsi b/arch/arm/boot/dts/qcom-msm8660.dtsi
index e5da87036dbb..0cf6bb0f869d 100644
--- a/arch/arm/boot/dts/qcom-msm8660.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8660.dtsi
@@ -287,51 +287,8 @@
 					compatible = "qcom,pm8058-gpio",
 						     "qcom,ssbi-gpio";
 					reg = <0x150>;
-					interrupt-parent = <&pm8058>;
-					interrupts = <192 IRQ_TYPE_NONE>,
-						     <193 IRQ_TYPE_NONE>,
-						     <194 IRQ_TYPE_NONE>,
-						     <195 IRQ_TYPE_NONE>,
-						     <196 IRQ_TYPE_NONE>,
-						     <197 IRQ_TYPE_NONE>,
-						     <198 IRQ_TYPE_NONE>,
-						     <199 IRQ_TYPE_NONE>,
-						     <200 IRQ_TYPE_NONE>,
-						     <201 IRQ_TYPE_NONE>,
-						     <202 IRQ_TYPE_NONE>,
-						     <203 IRQ_TYPE_NONE>,
-						     <204 IRQ_TYPE_NONE>,
-						     <205 IRQ_TYPE_NONE>,
-						     <206 IRQ_TYPE_NONE>,
-						     <207 IRQ_TYPE_NONE>,
-						     <208 IRQ_TYPE_NONE>,
-						     <209 IRQ_TYPE_NONE>,
-						     <210 IRQ_TYPE_NONE>,
-						     <211 IRQ_TYPE_NONE>,
-						     <212 IRQ_TYPE_NONE>,
-						     <213 IRQ_TYPE_NONE>,
-						     <214 IRQ_TYPE_NONE>,
-						     <215 IRQ_TYPE_NONE>,
-						     <216 IRQ_TYPE_NONE>,
-						     <217 IRQ_TYPE_NONE>,
-						     <218 IRQ_TYPE_NONE>,
-						     <219 IRQ_TYPE_NONE>,
-						     <220 IRQ_TYPE_NONE>,
-						     <221 IRQ_TYPE_NONE>,
-						     <222 IRQ_TYPE_NONE>,
-						     <223 IRQ_TYPE_NONE>,
-						     <224 IRQ_TYPE_NONE>,
-						     <225 IRQ_TYPE_NONE>,
-						     <226 IRQ_TYPE_NONE>,
-						     <227 IRQ_TYPE_NONE>,
-						     <228 IRQ_TYPE_NONE>,
-						     <229 IRQ_TYPE_NONE>,
-						     <230 IRQ_TYPE_NONE>,
-						     <231 IRQ_TYPE_NONE>,
-						     <232 IRQ_TYPE_NONE>,
-						     <233 IRQ_TYPE_NONE>,
-						     <234 IRQ_TYPE_NONE>,
-						     <235 IRQ_TYPE_NONE>;
+					interrupt-controller;
+					#interrupt-cells = <2>;
 					gpio-controller;
 					#gpio-cells = <2>;
 
-- 
2.17.2

[PATCH 8/9] arm: dts: qcom: mdm9615: add interrupt controller properties

[Brian Masney]

Add interrupt controller properties now that ssbi-gpio is a proper
hierarchical IRQ chip. The interrupts property is no longer needed so
remove it.

Note that the IRQs started at 24 instead of 192 like all of the other
PMICs. This is the same IRQs as the MPP for this board. qcom-pm8xxx.c
doesn't set the shared IRQs so this is highly likely to be a copy and
paste error.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 arch/arm/boot/dts/qcom-mdm9615.dtsi | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-mdm9615.dtsi b/arch/arm/boot/dts/qcom-mdm9615.dtsi
index c852b69229c9..0ed6fc3e873c 100644
--- a/arch/arm/boot/dts/qcom-mdm9615.dtsi
+++ b/arch/arm/boot/dts/qcom-mdm9615.dtsi
@@ -323,13 +323,8 @@
 
 				pmicgpio: gpio@150 {
 					compatible = "qcom,pm8018-gpio", "qcom,ssbi-gpio";
-					interrupt-parent = <&pmicintc>;
-					interrupts = <24 IRQ_TYPE_NONE>,
-						     <25 IRQ_TYPE_NONE>,
-						     <26 IRQ_TYPE_NONE>,
-						     <27 IRQ_TYPE_NONE>,
-						     <28 IRQ_TYPE_NONE>,
-						     <29 IRQ_TYPE_NONE>;
+					interrupt-controller;
+					#interrupt-cells = <2>;
 					gpio-controller;
 					#gpio-cells = <2>;
 				};
-- 
2.17.2

[PATCH 9/9] mfd: pm8xxx: revert "disassociate old virq if hwirq mapping already exists"

[Brian Masney]

Now that ssbi-gpio is a proper hierarchical IRQ chip, and all in-tree
users of device tree have been updated, we can now drop the hack that
was introduced to disassociate the old Linux virq if a hwirq mapping
already exists. That patch was introduced to not break git bisect for
any existing boards.

This change was not tested on any actual hardware, however the same
change was made to spmi-pmic-arb.c and tested on a LG Nexus 5
(hammerhead) phone.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/mfd/qcom-pm8xxx.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
index 97b931465601..a976890c4019 100644
--- a/drivers/mfd/qcom-pm8xxx.c
+++ b/drivers/mfd/qcom-pm8xxx.c
@@ -387,11 +387,6 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
 				  irq_hw_number_t hwirq, unsigned int type)
 {
 	irq_flow_handler_t handler;
-	unsigned int old_virq;
-
-	old_virq = irq_find_mapping(domain, hwirq);
-	if (old_virq)
-		irq_domain_disassociate(domain, old_virq);
 
 	if (type & IRQ_TYPE_EDGE_BOTH)
 		handler = handle_edge_irq;
-- 
2.17.2

Re: [PATCH 0/9] qcom: ssbi-gpio: add support for hierarchical IRQ chip

[Linus Walleij]

On Fri, Jan 25, 2019 at 5:23 PM Brian Masney <masneyb@onstation.org> wrote:

> This patch series adds hierarchical IRQ chip support to ssbi-gpio so
> that device tree consumers can request an IRQ directly from the GPIO
> block rather than having to request an IRQ from the underlying PMIC.

I had to apply the patch I just sent out:
"genirq: introduce irq_chip_mask_ack_parent()"
and then the following diff to make things work. But after that:
Tested-by: Linus Walleij <linus.walleij@linaro.org>
It works like a charm on the APQ8060 DragonBoard!

I would recommend you just fold the below into your series
and add the extra patch for genirq to your patch stack.

From 14569f7901cfe84221ed775ba63b09b32277de62 Mon Sep 17 00:00:00 2001
From: Linus Walleij <linus.walleij@linaro.org>
Date: Wed, 6 Feb 2019 16:19:32 +0100
Subject: [PATCH] qcom hierarchy: fixup hacks

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mfd/qcom-pm8xxx.c                | 19 ++++---------------
 drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c |  8 ++++----
 2 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
index a976890c4019..2ee3b075cd0e 100644
--- a/drivers/mfd/qcom-pm8xxx.c
+++ b/drivers/mfd/qcom-pm8xxx.c
@@ -82,8 +82,9 @@ struct pm_irq_chip {
  struct irq_domain *irqdomain;
  unsigned int num_blocks;
  unsigned int num_masters;
- u8 config[0];
  const struct pm_irq_data *pm_irq_data;
+ /* MUST BE AT THE END OF THIS STRUCT */
+ u8 config[0];
 };

 static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
@@ -303,7 +304,6 @@ static int pm8xxx_irq_set_type(struct irq_data *d,
unsigned int flow_type)
 {
  struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
  unsigned int pmirq = irqd_to_hwirq(d);
- irq_flow_handler_t flow_handler;
  int irq_bit;
  u8 block, config;

@@ -317,8 +317,6 @@ static int pm8xxx_irq_set_type(struct irq_data *d,
unsigned int flow_type)
  chip->config[pmirq] &= ~PM_IRQF_MASK_RE;
  if (flow_type & IRQF_TRIGGER_FALLING)
  chip->config[pmirq] &= ~PM_IRQF_MASK_FE;
-
- flow_handler = handle_edge_irq;
  } else {
  chip->config[pmirq] |= PM_IRQF_LVL_SEL;

@@ -326,11 +324,9 @@ static int pm8xxx_irq_set_type(struct irq_data
*d, unsigned int flow_type)
  chip->config[pmirq] &= ~PM_IRQF_MASK_RE;
  else
  chip->config[pmirq] &= ~PM_IRQF_MASK_FE;
-
- flow_handler = handle_level_irq;
  }

- irq_set_handler_locked(d, flow_handler);
+ irq_set_handler_locked(d, handle_level_irq);

  config = chip->config[pmirq] | PM_IRQF_CLR;
  return pm8xxx_config_irq(chip, block, config);
@@ -386,15 +382,8 @@ static void pm8xxx_irq_domain_map(struct pm_irq_chip *chip,
    struct irq_domain *domain, unsigned int irq,
    irq_hw_number_t hwirq, unsigned int type)
 {
- irq_flow_handler_t handler;
-
- if (type & IRQ_TYPE_EDGE_BOTH)
- handler = handle_edge_irq;
- else
- handler = handle_level_irq;
-
  irq_domain_set_info(domain, irq, hwirq, chip->pm_irq_data->irq_chip,
-     chip, handler, NULL, NULL);
+     chip, handle_level_irq, NULL, NULL);
  irq_set_noprobe(irq);
 }

diff --git a/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
b/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
index d4847f42f52f..e2509f26be4c 100644
--- a/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
@@ -696,8 +696,7 @@ static int pm8xxx_pin_populate(struct pm8xxx_gpio *pctrl,

 static struct irq_chip pm8xxx_irq_chip = {
  .name = "ssbi-gpio",
- .irq_mask = irq_chip_mask_parent,
- .irq_ack = irq_chip_ack_parent,
+ .irq_mask_ack = irq_chip_mask_ack_parent,
  .irq_unmask = irq_chip_unmask_parent,
  .irq_set_type = irq_chip_set_type_parent,
  .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
@@ -781,13 +780,14 @@ static int pm8xxx_gpio_probe(struct platform_device *pdev)
  struct pinctrl_pin_desc *pins;
  struct pm8xxx_gpio *pctrl;
  int ret;
- int i, npins;
+ int i;

  pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
  if (!pctrl)
  return -ENOMEM;

- npins = (uintptr_t) device_get_match_data(&pdev->dev);
+ pctrl->dev = &pdev->dev;
+ pctrl->npins = (uintptr_t) device_get_match_data(&pdev->dev);

  pctrl->regmap = dev_get_regmap(pdev->dev.parent, NULL);
  if (!pctrl->regmap) {
-- 
2.20.1


Yours,
Linus Walleij