Re: [PATCH v3] irqchip: Add support for ARMv7-M's NVIC

[Thomas Gleixner <tglx@linutronix.de>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1687 bytes --]

On Wed, 17 Apr 2013, Uwe Kleine-König wrote:
> +struct nvic_bank_data {
> +	/*
> +	 * For irq i base holds nvic_base + 4 * i / 32. So you can access the
> +	 * right ISER register (i.e ISER[i / 32]) by just taking base + ISER.
> +	 * Ditto for ICER.
> +	 */
> +	void __iomem *base;
> +};

What's the point of a struct with a single member? Why not having an
array of base pointers ?

> +static struct nvic_chip_data {
> +	struct irq_domain *domain;
> +	struct nvic_bank_data bdata[NVIC_MAX_BANKS];
> +} nvic_chip_data;
> +
> +asmlinkage void __exception_irq_entry
> +nvic_do_IRQ(irq_hw_number_t hwirq, struct pt_regs *regs)
> +{
> +	unsigned int irq = irq_linear_revmap(nvic_chip_data.domain, hwirq);
> +
> +	handle_IRQ(irq, regs);
> +}
> +
> +static inline void __iomem *nvic_bank_base(struct irq_data *d)
> +{
> +	struct nvic_bank_data *bank_data = irq_data_get_irq_chip_data(d);
> +	return bank_data->base;
> +}
> +
> +static void nvic_mask_irq(struct irq_data *d)
> +{
> +	u32 mask = 1 << (d->hwirq % 32);
> +
> +	writel_relaxed(mask, nvic_bank_base(d) + NVIC_ICER);
> +}
> +
> +static void nvic_unmask_irq(struct irq_data *d)
> +{
> +	u32 mask = 1 << (d->hwirq % 32);
> +
> +	writel_relaxed(mask, nvic_bank_base(d) + NVIC_ISER);
> +}

How is that different from what the generic irq chip implementation
does? The only difference is that mask is generated by d->hwirq and
not by d->irq. And due to the fact, that you use a full linear mapping
between hwirq and virq the generic code simply works.

Even if it would not work, it would be trivial to extend the generic
chip with that functionality instead of hacking another slightly
different copy of the same thing.

Thanks,

	tglx