Re: [PATCH 09/16] irqchip: Add RDA8810PL interrupt driver

[dropping the @rdamicro.com addresses, as they bounce...]

On Tue, 20 Nov 2018 03:19:58 +0000,
Manivannan Sadhasivam [off-list ref] wrote:

Hi Marc,

Thanks for the quick review!

On Mon, Nov 19, 2018 at 05:36:49PM +0000, Marc Zyngier wrote:

quoted

Manivannan,

On 19/11/2018 17:09, Manivannan Sadhasivam wrote:

quoted

quoted

+static int rda_intc_set_type(struct irq_data *data, unsigned int flow_type)
+{
+	if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))
+		irq_set_handler(data->irq, handle_edge_irq);
+	if (flow_type & (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW))
+		irq_set_handler(data->irq, handle_level_irq);

So you don't need to set anything in your interrupt controller for this
to switch between level and edge? That'd be a first...

Interrupt controller can only handle level triggered interrupts. Should
I just remove irq_set_type callback itself?

No, keep it, but return -EINVAL on anything that doesn't match what
the controller actually supports.

quoted

quoted

+
+	return 0;
+}
+
+struct irq_domain *rda_irq_domain;

static?

Ack.

quoted

quoted

+
+static void __exception_irq_entry rda_handle_irq(struct pt_regs *regs)
+{
+	u32 stat = readl(base + RDA_INTC_FINALSTATUS);
+	u32 hwirq;
+
+	while (stat) {
+		hwirq = __fls(stat);
+		handle_domain_irq(rda_irq_domain, hwirq, regs);
+		stat &= ~(1 << hwirq);
+	}
+}
+
+static struct irq_chip rda_irq_chip = {
+	.name		= "rda-intc",
+	.irq_ack	= rda_intc_mask_irq,

You're joking, right? What does it mean to implement both ack as mask
when you already have mask?

Right, but I just followed what other drivers were doing (irq-sa11x0). Will
remove it.

As usual, seeing something in another driver doesn't mean it is
right. Also, StrongARM is an interesting piece of history, and taking
inspiration from it is mostly a bad idea.

[...]

quoted

quoted

+static int __init rda8810_intc_init(struct device_node *node,
+				    struct device_node *parent)
+{
+	base = of_io_request_and_map(node, 0, "rda-intc");
+	if (!base)
+		return -ENXIO;
+	/*
+	 * Mask, and invalid all interrupt sources
+	 */
+	writel(RDA_IRQ_MASK_ALL, base + RDA_INTC_MASK_CLR);
+
+	rda_irq_domain = irq_domain_create_linear(&node->fwnode, RDA_NR_IRQS,
+						  &rda_irq_domain_ops, base);
+	WARN_ON(!rda_irq_domain);

Just WARN_ON(), and carry on? Please implement some error handling.

Sure. Which one would you recommend? Panic or returning -ENXIO?

Don't leak the IO space, return -ENOMEM.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.