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
