On 27/11/17 12:28, Greentime Hu wrote:

Consider writing (1 << data->hwirq) as BIT(data->hwirq).

+}
+
+static void ativic32_mask_irq(struct irq_data *data)
+{
+	unsigned long int_mask2 = __nds32__mfsr(NDS32_SR_INT_MASK2);
+	__nds32__mtsr_dsb(int_mask2 & (~(1 << data->hwirq)), NDS32_SR_INT_MASK2);

Same here.

+}
+
+static void ativic32_mask_ack_irq(struct irq_data *data)
+{
+	unsigned long int_mask2 = __nds32__mfsr(NDS32_SR_INT_MASK2);
+	__nds32__mtsr_dsb(int_mask2 & (~(1 << data->hwirq)), NDS32_SR_INT_MASK2);
+	__nds32__mtsr_dsb((1 << data->hwirq), NDS32_SR_INT_PEND2);

This is effectively MASK+ACK, so you're better off just writing it as
such. And since there is no advantage in your implementation in having
MASK_ACK over MASK+ACK, I suggest you remove this function completely,
and rely on the core code which will call them in sequence.

+
+}
+
+static void ativic32_unmask_irq(struct irq_data *data)
+{
+	unsigned long int_mask2 = __nds32__mfsr(NDS32_SR_INT_MASK2);
+	__nds32__mtsr_dsb(int_mask2 | (1 << data->hwirq), NDS32_SR_INT_MASK2);

Same BIT() here.

+}
+
+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,
+};
+
+static unsigned int __initdata nivic_map[6] = { 6, 2, 10, 16, 24, 32 };
+
+static struct irq_domain *root_domain;
+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))

And here.

+		irq_set_chip_and_handler(virq, &ativic32_chip, handle_edge_irq);
+	else
+		irq_set_chip_and_handler(virq, &ativic32_chip, handle_level_irq);

Since you do not express the trigger in DT, you need to tell the core
about it by calling irqd_set_trigger_type() with the right setting.

+
+	return 0;
+}
+
+static struct irq_domain_ops ativic32_ops = {
+	.map = ativic32_irq_domain_map,
+	.xlate = irq_domain_xlate_onecell
+};
+
+static int get_intr_src(void)

I'm not sure "int" is the best return type here. I suspect something
unsigned would be preferable, or even the irq_hw_number_t type.

+{
+	return ((__nds32__mfsr(NDS32_SR_ITYPE)&ITYPE_mskVECTOR) >> ITYPE_offVECTOR)

Spaces around '&'.

+		- NDS32_VECTOR_offINTERRUPT;
+}
+
+asmlinkage void asm_do_IRQ(struct pt_regs *regs)
+{
+	int hwirq = get_intr_src();

irq_hw_number_t.

+	handle_domain_irq(root_domain, hwirq, regs);
+}
+
+int __init ativic32_init_irq(struct device_node *node, struct device_node *parent)
+{
+	unsigned long int_vec_base, nivic;
+
+	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 atcivic32 for this cpu.\n");
+
+	nivic = (int_vec_base & IVB_mskNIVIC) >> IVB_offNIVIC;
+	if (nivic >= (sizeof nivic_map / sizeof nivic_map[0]))

This should be:
	if (nivic >= ARRAY_SIZE(NIVIC_MAP))

+		panic("The number of input for ativic32 is not supported.\n");
+
+	nivic = nivic_map[nivic];

I'd rather you use another variable (nr_ints?).

+
+	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);
+
+	return 0;
+}
+IRQCHIP_DECLARE(ativic32, "andestech,ativic32", ativic32_init_irq);

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html