On 08/11/17 05:55, Greentime Hu wrote:

From: Greentime Hu <redacted>

Please add a commit message, indicating what this does, and potentially
a pointer to some documentation (if publicly available).

If it is not configurable, what is the point of having each interrupt
carry a trigger type in DT?

+	return 0;
+}
+
+static struct irq_chip ativic32_chip = {
+	.name = "ativic32",
+	.irq_ack = ativic32_ack_irq,
+	.irq_mask = ativic32_mask_irq,
+	.irq_mask_ack = ativic32_mask_ack_irq,
+	.irq_unmask = ativic32_unmask_irq,
+	.irq_set_type = ativic32_set_type,
+};
+
+static unsigned int __initdata nivic_map[6] = { 6, 2, 10, 16, 24, 32 };
+
+struct irq_domain *root_domain;

Why isn't this static? Is there anything accessing it from outside of
this driver?

+static int ativic32_irq_domain_map(struct irq_domain *id, unsigned int virq,
+				  irq_hw_number_t hw)
+{
+
+	unsigned long int_trigger_type;
+	int_trigger_type = __nds32__mfsr(NDS32_SR_INT_TRIGGER);
+	if (int_trigger_type & (1 << hw))
+		irq_set_chip_and_handler(virq, &ativic32_chip, handle_edge_irq);
+	else
+		irq_set_chip_and_handler(virq, &ativic32_chip, handle_level_irq);
+
+	return 0;
+}
+
+static struct irq_domain_ops ativic32_ops = {
+	.map = ativic32_irq_domain_map,
+	.xlate = irq_domain_xlate_onecell

Huh... Your DT binding insist on two cells, and yet...

+};
+
+static int get_intr_src(void)
+{
+	return ((__nds32__mfsr(NDS32_SR_ITYPE)&ITYPE_mskVECTOR) >> ITYPE_offVECTOR)
+		- NDS32_VECTOR_offINTERRUPT;
+}
+
+asmlinkage void asm_do_IRQ(struct pt_regs *regs)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+	int virq;
+	int irq = get_intr_src();
+
+	/*
+	 * Some hardware gives randomly wrong interrupts.  Rather
+	 * than crashing, do something sensible.
+	 */
+	if (unlikely(irq >= NR_IRQS)) {
+		pr_emerg( "IRQ exceeds NR_IRQS\n");
+		BUG();

Given the above comment, I find this hilarious!

+	}
+
+	irq_enter();
+	virq = irq_find_mapping(root_domain, irq);
+	generic_handle_irq(virq);
+	irq_exit();
+	set_irq_regs(old_regs);

Why can't you use handle_domain_irq() directly instead of what looks
like a copy of it (this is where the comment comes from...)?

+
+}
+
+int __init ativic32_init_irq(struct device_node *node, struct device_node *parent)
+{
+	unsigned long int_vec_base, nivic, i;
+
+	if (WARN(parent, "non-root ativic32 are not supported"))
+		return -EINVAL;
+
+	int_vec_base = __nds32__mfsr(NDS32_SR_IVB);
+
+	if (((int_vec_base & IVB_mskIVIC_VER) >> IVB_offIVIC_VER) == 0) {
+		panic("Unable to use NOINTC option to boot on this cpu\n");
+	}
+
+	nivic = (int_vec_base & IVB_mskNIVIC) >> IVB_offNIVIC;
+	if (nivic >= (sizeof nivic_map / sizeof nivic_map[0])) {
+		panic
+		    ("The number of input for IVIC Controller is not supported on this cpu\n");

Irk. Please leave this on a single line... Or better yet, get rid of it
(see below).

+	}
+	nivic = nivic_map[nivic];

If you have multiple configurations, I'd rather they live in DT,
specially if the IP is easily configurable.

+
+	root_domain = irq_domain_add_linear(node, nivic,
+			&ativic32_ops, NULL);
+
+	if (!root_domain)
+		panic("%s: unable to create IRQ domain\n", node->full_name);
+
+	for(i = 0; i < nivic; i++)
+		irq_create_mapping(root_domain, i);

You shouldn't need this, as the core DT code will populate the interrupt
on demand.

+
+	return 0;
+
+}
+IRQCHIP_DECLARE(ativic32, "andestech,ativic32", ativic32_init_irq);

Thanks,

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