Hi Marc,

On Thu, Jan 17, 2019 at 11:22:59AM +0000, Marc Zyngier wrote:

quoted

-static int qpnpint_irq_domain_map(struct irq_domain *d,
-				  unsigned int virq,
-				  irq_hw_number_t hwirq)
-{
-	struct spmi_pmic_arb *pmic_arb = d->host_data;
 
+static void qpnpint_irq_domain_map(struct spmi_pmic_arb *pmic_arb,
+				   struct irq_domain *domain, unsigned int virq,
+				   irq_hw_number_t hwirq)
+{
 	dev_dbg(&pmic_arb->spmic->dev, "virq = %u, hwirq = %lu\n", virq, hwirq);
 
-	irq_set_chip_and_handler(virq, &pmic_arb_irqchip, handle_level_irq);
-	irq_set_chip_data(virq, d->host_data);
-	irq_set_noprobe(virq);
+	irq_domain_set_info(domain, virq, hwirq, &pmic_arb_irqchip, pmic_arb,
+			    handle_level_irq, NULL, NULL);

I understand you haven't changed the existing semantic here by always
setting the handler to handle_level_irq. But is that guaranteed to
always be the case? See below.

quoted

+}
+
+static int qpnpint_irq_domain_alloc(struct irq_domain *domain,
+				    unsigned int virq, unsigned int nr_irqs,
+				    void *data)
+{
+	struct spmi_pmic_arb *pmic_arb = domain->host_data;
+	struct irq_fwspec *fwspec = data;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	int ret, i;
+
+	ret = qpnpint_irq_domain_translate(domain, fwspec, &hwirq, &type);

Here, you extract the trigger from DT.

quoted

+	if (ret)
+		return ret;
+
+	for (i = 0; i < nr_irqs; i++)
+		qpnpint_irq_domain_map(pmic_arb, domain, virq + i, hwirq + i);

Shouldn't you propagate it into the mapping function so that the handler
can be selected accordingly? Or does the interrupt controller convert
edge signals to level somehow?

qpnpint_irq_set_type() calls irq_set_handler_locked() to set the hander
to be either handle_edge_irq() or handle_level_irq(). So the handler is
initially setup incorrectly in some cases, but then setup correctly (via
__irq_set_trigger) when __setup_irq() is called by
request_threaded_irq().

It looks like that this will cause problems with shared IRQs to work as
expected.

I can rework this code and get this fixed.

Brian