Messages in this thread |  | | Date | Wed, 16 Aug 2017 17:12:18 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH v7 04/15] irqchip: RISC-V Local Interrupt Controller Driver |
| |
On Mon, 31 Jul 2017, Palmer Dabbelt wrote: > +static void riscv_software_interrupt(void) > +{ > +#ifdef CONFIG_SMP > + irqreturn_t ret; > + > + ret = handle_ipi(); > + > + WARN_ON(ret == IRQ_NONE);
WARN_ON(handle_ipi() == IRQ_NONE);
perhaps?
> +#else > + /* > + * We currently only use software interrupts to pass inter-processor > + * interrupts, so if a non-SMP system gets a software interrupt then we > + * don't know what to do. > + */ > + pr_warning("Software Interrupt without CONFIG_SMP\n"); > +#endif > +}
> +static void riscv_irq_enable(struct irq_data *d) > +{ > + struct riscv_irq_data *data = irq_data_get_irq_chip_data(d); > + > + /* > + * It's only possible to write SIE on the current hart. This jumps > + * over to the target hart if it's not the current one. It's invalid > + * to write SIE on a hart that's not currently running. > + */ > + if (data->hart == smp_processor_id()) > + riscv_irq_unmask(d); > + else if (cpu_online(data->hart)) > + smp_call_function_single(data->hart, > + riscv_irq_enable_helper, > + d, > + true); > + else > + WARN_ON_ONCE(1);
If you write a small helper:
static void riscv_remote_ctrl(unsigned int cpu, void (*fn)(void *d), struct irq_data *data) { smp_call_function_single(cpu, cb, data, true); }
Then both riscv_irq_enable() and riscv_irq_disable() become readable functions.
if (data->hart == smp_processor_id()) riscv_irq_unmask(d); else if (cpu_online(data->hart)) riscv_remote_ctrl(data->hart, riscv_irq_enable_helper, d); else WARN_ON_ONCE(1);
Hmm?
> +static int riscv_intc_init(struct device_node *node, struct device_node *parent) > +{ > + int hart; > + struct riscv_irq_data *data; > + > + if (parent) > + return 0; > + > + hart = riscv_of_processor_hart(node->parent); > + if (hart < 0) > + return -EIO; > + > + data = &per_cpu(riscv_irq_data, hart); > + snprintf(data->name, sizeof(data->name), "riscv,cpu_intc,%d", hart); > + data->hart = hart; > + data->chip.name = data->name; > + data->chip.irq_mask = riscv_irq_mask; > + data->chip.irq_unmask = riscv_irq_unmask; > + data->chip.irq_enable = riscv_irq_enable; > + data->chip.irq_disable = riscv_irq_disable; > + data->domain = irq_domain_add_linear( > + node, > + 8*sizeof(uintptr_t), > + &riscv_irqdomain_ops, > + data);
This is really horrible to read. What's wrong with using the full 80 chars?
data->domain = irq_domain_add_linear(node, 8 * sizeof(uintptr_t), &riscv_irqdomain_ops, data);
> + if (!data->domain) > + goto error_add_linear; > + pr_info("%s: %d local interrupts mapped\n", > + data->name, 8*(int)sizeof(uintptr_t));
Can we please make that '8 * sizeof()' a constant and use it in both places? Which makes the pr_info also fit into a single line.
> + return 0; > + > +error_add_linear: > + pr_warning("%s: unable to add IRQ domain\n", > + data->name);
Single line please. Enough room.
> + return -(ENXIO);
No braces.
Thanks,
tglx
|  |