linux-kernel - Re: [PATCH] Add TI-Nspire irqchip support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 30 May 2013 16:53:31 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Daniel Tang <dt.tangr@...il.com>
cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Add TI-Nspire irqchip support

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@...il.com>

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/