Hi,
Thank you for the reviews.
Some comments below
On 08/31/2016 05:52 PM, Marc Zyngier wrote:
On 31/08/16 17:35, Zubair Lutfullah Kakakhel wrote:
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 <Zubair.Kakakhel@xxxxxxxxxx>
---
V2 -> V3
New patch. Cleans up driver structure
---
...
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);
Now that you dynamically allocate things, how are you handling failures?
+ if (!irqc)
+ return -ENOMEM;
+
+ xintc_irqc = irqc;
- intc_baseaddr = of_iomap(intc, 0);
- BUG_ON(!intc_baseaddr);
+ 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) {
@@ -150,43 +176,40 @@ static int __init xilinx_intc_of_init(struct device_node *intc,
return ret;
All the return paths should now take care of releasing the allocated
resources.
Thanks for pointing it out. I'll fix it in the next series.
}
- 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;
}
- 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);
irq_set_default_host(root_domain);
You haven't addressed the comment on get_irq() which could be static (at
least from solely looking at this file).
Apologies. In a rush to get a series out before heading home, it slipped my mind.
It is needed as it is used in arch/microblaze/kernel/irq.c
But I'll add a patch to rename it correctly. get_irq is far too generic outside arch code.
Thanks,
ZubairLK
Thanks,
M.
