From: tglx@linutronix.de (Thomas Gleixner)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v3 7/9] irqchip/irq-mxs.c: add asm9260 support
Date: Wed, 8 Oct 2014 09:45:25 +0200 (CEST) [thread overview]
Message-ID: <alpine.DEB.2.11.1410080919510.4292@nanos> (raw)
In-Reply-To: <1412672123-16694-8-git-send-email-linux@rempel-privat.de>
On Tue, 7 Oct 2014, Oleksij Rempel wrote:
> --- /dev/null
> +++ b/drivers/irqchip/alphascale,asm9260-icoll.h
Eew. We really can do without the comma in the filename.
> +struct icoll_priv {
> + void __iomem *vector;
> + void __iomem *levelack;
> + void __iomem *ctrl;
> + void __iomem *stat;
> + void __iomem *intr;
> + /* number of interrupts per register */
> + int intr_reg_num;
So why on earth can't you find a name for this variable which reflects
its meaning? intr_per_reg would be pretty clear, but intr_reg_num is
just confusing as hell.
> + void __iomem *clear;
> +};
> +
> +struct icoll_priv icoll_priv;
Why is this global?
> static struct irq_domain *icoll_domain;
>
> +static u32 icoll_intr_bitshift(struct irq_data *d, u32 bit)
> +{
> + int n = icoll_priv.intr_reg_num - 1;
Newline between declaration and code.
> + return bit << ((d->hwirq & n) << n);
I really had to twist my brain around this. A comment would be helpful
for the casual reader/reviewer.
> +}
> +
> +static void __iomem *icoll_intr_reg(struct irq_data *d)
> +{
> + int n = icoll_priv.intr_reg_num;
> + return icoll_priv.intr + ((d->hwirq / n) * 0x10);
See above. And aside of that, this should be implemented with a shift
to avoid a divide in the hot path.
> +}
> +
> static void icoll_ack_irq(struct irq_data *d)
> {
> /*
> @@ -51,19 +88,25 @@ static void icoll_ack_irq(struct irq_data *d)
> * BV_ICOLL_LEVELACK_IRQLEVELACK__LEVEL0 unconditionally.
> */
> __raw_writel(BV_ICOLL_LEVELACK_IRQLEVELACK__LEVEL0,
> - icoll_base + HW_ICOLL_LEVELACK);
> + icoll_priv.levelack);
The icoll_priv conversion of that code wants to be a separate
patch. First change existing code then add new functionality.
> static void icoll_unmask_irq(struct irq_data *d)
> {
> - __raw_writel(BM_ICOLL_INTERRUPTn_ENABLE,
> - icoll_base + HW_ICOLL_INTERRUPTn_SET(d->hwirq));
> + if (!IS_ERR(icoll_priv.clear)) {
This is a horrible construct. Why isn't it sufficient to do
if (icoll_priv.clear)
and initialize it to NULL for the IMX case?
> + icoll_priv.clear = ERR_PTR(-ENODEV);
Thanks,
tglx
next prev parent reply other threads:[~2014-10-08 7:45 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-21 18:41 [PATCH v2 0/8] initial suport for Alpscale ASM9260 Oleksij Rempel
2014-09-21 18:41 ` [PATCH v2 1/8] ARM: add mach-asm9260 Oleksij Rempel
2014-09-22 15:08 ` Arnd Bergmann
2014-09-23 9:00 ` Oleksij Rempel
2014-09-23 10:19 ` Arnd Bergmann
2014-09-24 8:00 ` Oleksij Rempel
2014-09-24 9:43 ` Russell King - ARM Linux
2014-09-24 9:56 ` Oleksij Rempel
2014-09-24 10:25 ` Russell King - ARM Linux
2014-09-24 10:33 ` Arnd Bergmann
2014-09-24 11:30 ` Oleksij Rempel
2014-09-21 18:41 ` [PATCH v2 2/8] add include/debug/asm9260.S Oleksij Rempel
2014-09-21 18:45 ` Oleksij Rempel
2014-09-21 18:45 ` [PATCH v2 3/8] add alphascale,asm9260.h binding Oleksij Rempel
2014-09-24 10:15 ` Mark Rutland
2014-09-21 18:45 ` [PATCH v2 4/8] ARM: dts: add DT for Alphascale ASM9260 SoC Oleksij Rempel
2014-09-22 15:14 ` Arnd Bergmann
2014-09-24 10:11 ` Mark Rutland
2014-09-21 18:45 ` [PATCH v2 5/8] clk: add clk-asm9260 driver Oleksij Rempel
2014-09-21 18:45 ` [PATCH v2 6/8] clocksource: add asm9260_timer driver Oleksij Rempel
2014-09-21 18:45 ` [PATCH v2 7/8] irqchip: add irq-asm9260 driver Oleksij Rempel
2014-09-22 15:22 ` Arnd Bergmann
2014-09-21 18:45 ` [PATCH v2 8/8] tty/serial: add asm9260-serial driver Oleksij Rempel
2014-09-22 15:26 ` Arnd Bergmann
2014-09-22 16:04 ` Oleksij Rempel
2014-09-24 9:24 ` Oleksij Rempel
2014-09-24 10:20 ` Arnd Bergmann
2014-09-23 11:32 ` [PATCH v2 0/8] initial suport for Alpscale ASM9260 Arnd Bergmann
2014-09-24 10:13 ` Mark Rutland
2014-10-07 8:55 ` [PATCH v3 0/9] initial suport for Alphascale ASM9260 Oleksij Rempel
2014-10-07 8:55 ` [PATCH v3 1/9] ARM: add mach-asm9260 Oleksij Rempel
2014-10-07 8:55 ` [PATCH v3 2/9] arm: add lolevel debug support for asm9260 Oleksij Rempel
2014-10-07 8:55 ` [PATCH v3 3/9] ARM: dts: add DT for Alphascale ASM9260 SoC Oleksij Rempel
2014-10-07 8:55 ` [PATCH v3 4/9] ARM: add alphascale,acc.txt bindings documentation Oleksij Rempel
2014-10-07 8:55 ` [PATCH v3 5/9] ARM: clk: add clk-asm9260 driver Oleksij Rempel
2014-10-07 8:55 ` [PATCH v3 6/9] clocksource: add asm9260_timer driver Oleksij Rempel
2014-10-07 8:55 ` [PATCH v3 7/9] irqchip/irq-mxs.c: add asm9260 support Oleksij Rempel
2014-10-08 7:45 ` Thomas Gleixner [this message]
2014-10-08 12:11 ` [PATCH v4 1/2] irqchip: mxs: prepare driver for HW with different offsets Oleksij Rempel
2014-10-08 12:11 ` [PATCH v4 2/2] irqchip: mxs: add Alpascale ASM9260 support Oleksij Rempel
2014-10-07 8:55 ` [PATCH v3 8/9] tty/serial/mxs-auart.c: add initial Alphascale " Oleksij Rempel
2014-10-07 8:55 ` [PATCH v3 9/9] add Alphascale to vendor-prefixes.txt Oleksij Rempel
2014-10-10 5:38 ` [PATCH v3 0/9] initial suport for Alphascale ASM9260 Oleksij Rempel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.11.1410080919510.4292@nanos \
--to=tglx@linutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).