Hi,

Thanks for the review.
Comments inline.

On 09/01/2016 06:15 PM, Marc Zyngier wrote:

On 01/09/16 17:50, Zubair Lutfullah Kakakhel wrote:

quoted

The drivers read/write function handling is a bit quirky.
And the irqmask is passed directly to the handler.

Add a new irqchip struct to pass to the handler and
cleanup read/write handling.

Signed-off-by: Zubair Lutfullah Kakakhel <redacted>

---
V3 -> V4
Better error handling for kzalloc
Erroring out if the axi intc is probed twice as that isn't
supported

...

quoted

 static int __init xilinx_intc_of_init(struct device_node *intc,
 					     struct device_node *parent)
 {
-	u32 nr_irq, intr_mask;
+	u32 nr_irq;
 	int ret;
+	struct xintc_irq_chip *irqc;
+
+	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
+	if (!irqc)
+		return -ENOMEM;

-	intc_baseaddr = of_iomap(intc, 0);
-	BUG_ON(!intc_baseaddr);
+	if (xintc_irqc) {
+		pr_err("%s: Multiple instances of axi_intc aren't supported\n");
+		ret = -EINVAL;
+		goto err_alloc;

How about testing the variable first and error-ing early, rather than
performing the allocation and undoing it later?

Sure. Thanks

quoted

+	} else {
+		xintc_irqc = irqc;
+	}
+
+	irqc->base = of_iomap(intc, 0);
+	BUG_ON(!irqc->base);

 	ret = of_property_read_u32(intc, "xlnx,num-intr-inputs", &nr_irq);
 	if (ret < 0) {
 		pr_err("%s: unable to read xlnx,num-intr-inputs\n", __func__);
-		return ret;
+		goto err_alloc;
 	}

-	ret = of_property_read_u32(intc, "xlnx,kind-of-intr", &intr_mask);
+	ret = of_property_read_u32(intc, "xlnx,kind-of-intr", &irqc->intr_mask);
 	if (ret < 0) {
 		pr_err("%s: unable to read xlnx,kind-of-intr\n", __func__);
-		return ret;
+		goto err_alloc;
 	}

-	if (intr_mask >> nr_irq)
+	if (irqc->intr_mask >> nr_irq)
 		pr_warn("%s: mismatch in kind-of-intr param\n", __func__);

 	pr_info("%s: num_irq=%d, edge=0x%x\n",
-		intc->full_name, nr_irq, intr_mask);
+		intc->full_name, nr_irq, irqc->intr_mask);

-	write_fn = intc_write32;
-	read_fn = intc_read32;
+	irqc->read = intc_read32;
+	irqc->write = intc_write32;

 	/*
 	 * Disable all external interrupts until they are
 	 * explicity requested.
 	 */
-	write_fn(0, intc_baseaddr + IER);
+	xintc_write(irqc, IER, 0);

 	/* Acknowledge any pending interrupts just in case. */
-	write_fn(0xffffffff, intc_baseaddr + IAR);
+	xintc_write(irqc, IAR, 0xffffffff);

 	/* Turn on the Master Enable. */
-	write_fn(MER_HIE | MER_ME, intc_baseaddr + MER);
-	if (!(read_fn(intc_baseaddr + MER) & (MER_HIE | MER_ME))) {
-		write_fn = intc_write32_be;
-		read_fn = intc_read32_be;
-		write_fn(MER_HIE | MER_ME, intc_baseaddr + MER);
+	xintc_write(irqc, MER, MER_HIE | MER_ME);
+	if (!(xintc_read(irqc, MER) & (MER_HIE | MER_ME))) {
+		irqc->read = intc_read32_be;
+		irqc->write = intc_write32_be;
+		xintc_write(irqc, MER, MER_HIE | MER_ME);
 	}

-	/* Yeah, okay, casting the intr_mask to a void* is butt-ugly, but I'm
-	 * lazy and Michal can clean it up to something nicer when he tests
-	 * and commits this patch.  ~~gcl */
 	root_domain = irq_domain_add_linear(intc, nr_irq, &xintc_irq_domain_ops,
-							(void *)intr_mask);
+					    irqc);

What if the domain allocation fails? You've now configured the HW for
something you can't use. What are the side effects? Hint: handle
everything that can fail first, and only then poke the HW.

Thanks for pointing it out. I'll add an error check.

quoted

 	irq_set_default_host(root_domain);

 	return 0;
+
+err_alloc:
+	xintc_irqc = NULL;
+	kfree(irqc);
+	return ret;
+
 }

 IRQCHIP_DECLARE(xilinx_intc, "xlnx,xps-intc-1.00.a", xilinx_intc_of_init);

Instead of posting 3 versions in 3 days, please take the time to
correctly address the comments, and review your own code before
re-posting it. Rushing to get it merged for 4.9 is really not the best
approach.

Apologies for the spam. Combination of some free time this week + window of opportunity.
To be fair, AFAIK, this driver has lived in arch/microblaze without receiving a full thorough
review by any irqchip maintainer.

Hence the various missing bits. e.g. the root_domain error check didn't exist before.
And I didn't see it.

Regards,
ZubairLK

Thanks,

	M.