Re: [PATCH v3 02/14] irqchip: irq-pic32-evic: Add support for PIC32 interrupt controller

[Thomas Gleixner]

On Thu, 7 Jan 2016, Joshua Henderson wrote:
> +struct pic_reg {
> +	u32 val; /* value register*/

Just a nit. If you want to document your data structure, then please use
KernelDoc. These tail comments are horrible.

> +	u32 clr; /* clear register */
> +	u32 set; /* set register */
> +	u32 inv; /* inv register */
> +} __packed;
> +
> +struct evic {
> +	struct pic_reg intcon;
> +	struct pic_reg priss;
> +	struct pic_reg intstat;
> +	struct pic_reg iptmr;
> +	struct pic_reg ifs[6];
> +	u32 reserved1[8];
> +	struct pic_reg iec[6];
> +	u32 reserved2[8];
> +	struct pic_reg ipc[48];
> +	u32 reserved3[64];
> +	u32 off[191];

It would be way simpler to parse if you structured it like a table

+	struct pic_reg	intcon;
+	struct pic_reg	priss;
+	struct pic_reg	intstat;
+	struct pic_reg	iptmr;
+	struct pic_reg	ifs[6];
+	u32		reserved1[8];
+	struct pic_reg	iec[6];
+	u32		reserved2[8];
+	struct pic_reg	ipc[48];
+	u32		reserved3[64];
+	u32		off[191];

> +} __packed;
> +
> +static int get_ext_irq_index(irq_hw_number_t hw);
> +static void evic_set_ext_irq_polarity(int ext_irq, u32 type);

If you move the functions right here, then you don't need the forward
declarations.

> +/* mask off an interrupt */
> +static inline void mask_pic32_irq(struct irq_data *irqd)
> +{
> +	u32 reg, mask;
> +	unsigned int hwirq = irqd_to_hwirq(irqd);
> +
> +	BIT_REG_MASK(hwirq, reg, mask);
> +	writel(mask, &evic_base->iec[reg].clr);
> +}
> +
> +/* unmask an interrupt */
> +static inline void unmask_pic32_irq(struct irq_data *irqd)
> +{
> +	u32 reg, mask;
> +	unsigned int hwirq = irqd_to_hwirq(irqd);
> +
> +	BIT_REG_MASK(hwirq, reg, mask);
> +	writel(mask, &evic_base->iec[reg].set);
> +}
> +
> +/* acknowledge an interrupt */
> +static void ack_pic32_irq(struct irq_data *irqd)
> +{
> +	u32 reg, mask;
> +	unsigned int hwirq = irqd_to_hwirq(irqd);
> +
> +	BIT_REG_MASK(hwirq, reg, mask);
> +	writel(mask, &evic_base->ifs[reg].clr);

So you invented an open coded variant of the generic irq chip. Just with the
difference that the generic chip caches the mask and the register offsets ....

> +}
> +
> +static int set_type_pic32_irq(struct irq_data *data, unsigned int flow_type)
> +{
> +	int index;
> +
> +	switch (flow_type) {
> +
> +	case IRQ_TYPE_EDGE_RISING:
> +	case IRQ_TYPE_EDGE_FALLING:
> +		irq_set_handler_locked(data, handle_edge_irq);
> +		break;
> +
> +	case IRQ_TYPE_LEVEL_HIGH:
> +	case IRQ_TYPE_LEVEL_LOW:
> +		irq_set_handler_locked(data, handle_fasteoi_irq);
> +		break;
> +
> +	default:
> +		pr_err("Invalid interrupt type !\n");
> +		return -EINVAL;
> +	}
> +
> +	/* set polarity for external interrupts only */
> +	index = get_ext_irq_index(data->hwirq);
> +	if (index >= 0)
> +		evic_set_ext_irq_polarity(index, flow_type);

So for the non external interrupts you set a different handler and be
done. How is that supposed to work? They switch magically from one mode to the
other?

> +static void pic32_bind_evic_interrupt(int irq, int set)
> +{
> +	writel(set, &evic_base->off[irq]);
> +}
> +
> +int pic32_get_c0_compare_int(void)
> +{
> +	int virq;
> +
> +	virq = irq_create_mapping(evic_irq_domain, CORE_TIMER_INTERRUPT);
> +	irq_set_irq_type(virq, IRQ_TYPE_EDGE_RISING);
> +	return virq;

Why isn't that information retrieved via device tree?

> +}
> +
> +static struct irq_chip pic32_irq_chip = {
> +	.name = "PIC32-EVIC",
> +	.irq_ack = ack_pic32_irq,
> +	.irq_mask = mask_pic32_irq,
> +	.irq_unmask = unmask_pic32_irq,
> +	.irq_eoi = ack_pic32_irq,
> +	.irq_set_type = set_type_pic32_irq,

Again, this want's to be in tabular form, if at all.

> +};
> +
> +static void evic_set_irq_priority(int irq, int priority)
> +{
> +	u32 reg, shift;
> +
> +	reg = irq / 4;
> +	shift = (irq % 4) * 8;
> +
> +	/* set priority */
> +	writel(INT_MASK << shift, &evic_base->ipc[reg].clr);
> +	writel(priority << shift, &evic_base->ipc[reg].set);
> +}
> +
> +static void evic_set_ext_irq_polarity(int ext_irq, u32 type)
> +{
> +	if (WARN_ON(ext_irq >= NR_EXT_IRQS))
> +		return;

That WARN_ON is really helpful because you already made sure not to call it
for non EXT irqs.

> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		writel(1 << ext_irq, &evic_base->intcon.set);
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		writel(1 << ext_irq, &evic_base->intcon.clr);
> +		break;
> +	default:
> +		pr_err("Invalid external interrupt polarity !\n");
> +	}
> +}
> +
> +static int get_ext_irq_index(irq_hw_number_t hw)
> +{
> +	switch (hw) {
> +	case EXTERNAL_INTERRUPT_0:
> +		return 0;
> +	case EXTERNAL_INTERRUPT_1:
> +		return 1;
> +	case EXTERNAL_INTERRUPT_2:
> +		return 2;
> +	case EXTERNAL_INTERRUPT_3:
> +		return 3;
> +	case EXTERNAL_INTERRUPT_4:
> +		return 4;
> +	default:
> +		return -1;
> +	}
> +}

Why don't you use a seperate irq chip for the ext irqs? You can do that with
the generic chip as well.

    irq_alloc_domain_generic_chips(domain, 32, 2, ....)

And then you assign the alternate chip (2) to your ext irqs and have the set
type function only for the alternate chip.

Thanks,

	tglx