Re: [PATCH] Add TI-Nspire irqchip support

[Thomas Gleixner]

On Thu, 30 May 2013, Daniel Tang wrote:
 
> Hi,
> 
> This patch adds a driver for the interrupt controller found in the TI-Nspire calculator series.
> 
> Cheers,
> Daniel Tang

This is NOT a proper changelog.
 
> Signed-off-by: Daniel Tang <dt.tangr@xxxxxxxxx>

Also please read through this mail thread:

     https://lkml.org/lkml/2013/5/2/406

and rework your patches against:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/core

> +static void __iomem *irq_io_base;
> +static struct irq_domain *zevio_irq_domain;
> +
> +static void zevio_irq_ack(struct irq_data *irqd)
> +{
> +	void __iomem *base = irq_io_base;
> +
> +	if (irqd->hwirq < FIQ_START)
> +		base += IO_IRQ_BASE;
> +	else
> +		base += IO_FIQ_BASE;

This is horrible. If you redo this against the generic irq chip then
provide a separate base with the proper offsets to each chip. So you
can avoid this clumsy conditionals completely.

> +	readl(base + IO_RESET);
> +}
> +
> +static void zevio_irq_unmask(struct irq_data *irqd)
> +{
> +	void __iomem *base = irq_io_base;
> +	int irqnr = irqd->hwirq;
> +
> +	if (irqnr < FIQ_START) {
> +		base += IO_IRQ_BASE;
> +	} else {
> +		irqnr -= MAX_INTRS;
> +		base += IO_FIQ_BASE;
> +	}
> +
> +	writel((1<<irqnr), base + IO_ENABLE);

Replace with the generic function

> +}
> +
> +static void zevio_irq_mask(struct irq_data *irqd)
> +{
> +	void __iomem *base = irq_io_base;
> +	int irqnr = irqd->hwirq;
> +
> +	if (irqnr < FIQ_START) {
> +		base += IO_IRQ_BASE;
> +	} else {
> +		irqnr -= FIQ_START;
> +		base += IO_FIQ_BASE;
> +	}
> +
> +	writel((1<<irqnr), base + IO_DISABLE);

Replace with the generic function

> +static int process_base(void __iomem *base, struct pt_regs *regs)
> +{
> +	int irqnr;
> +
> +
> +	if (!readl(base + IO_STATUS))
> +		return 0;
> +
> +	irqnr = readl(base + IO_CURRENT);
> +	irqnr = irq_find_mapping(zevio_irq_domain, irqnr);
> +	handle_IRQ(irqnr, regs);
> +
> +	return 1;
> +}
> +
> +asmlinkage void __exception_irq_entry zevio_handle_irq(struct pt_regs *regs)
> +{
> +	while (process_base(irq_io_base + IO_FIQ_BASE, regs))
> +		;

Wheee. That's ugly as hell. Why don't you move the while loop into process_base() ?

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/