On 05/06/17 08:56, Icenowy Zheng wrote:

? 2017?6?5? GMT+08:00 ??3:53:50, Marc Zyngier [off-list ref] ??:

quoted

On 05/06/17 06:57, Chen-Yu Tsai wrote:

quoted

Hi Marc,

On Mon, May 22, 2017 at 10:25 PM, Chen-Yu Tsai [off-list ref] wrote:

quoted

On Mon, May 22, 2017 at 5:41 PM, Icenowy Zheng [off-list ref]

wrote:

quoted

quoted

quoted

? 2017?5?22? GMT+08:00 ??5:39:22, Marc Zyngier

[off-list ref] ??:

quoted

quoted

quoted

quoted

On 18/05/17 08:16, Icenowy Zheng wrote:

quoted

Add support for the newly imported compatible for the A64 R_INTC

in

quoted

quoted

quoted

quoted

quoted

irq-sunxi-nmi driver.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
Changes in v5:
- Fix A64 R_INTC compatible.

 drivers/irqchip/irq-sunxi-nmi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/irqchip/irq-sunxi-nmi.c

b/drivers/irqchip/irq-sunxi-nmi.c

quoted

index 668730c5cb66..5559c1d593bf 100644

--- a/drivers/irqchip/irq-sunxi-nmi.c
+++ b/drivers/irqchip/irq-sunxi-nmi.c

@@ -56,6 +56,12 @@ static struct sunxi_sc_nmi_reg_offs

sun9i_reg_offs

quoted

quoted

quoted

quoted

= {

quoted

     .enable = 0x04,
 };

+static struct sunxi_sc_nmi_reg_offs sun50i_reg_offs = {
+    .ctrl   = 0x0c,
+    .pend   = 0x10,
+    .enable = 0x40,
+};
+

Magic values? Even if no #define is provided, a pointer to the
corresponding documentation would be appreciated (assuming
documentation
exists).

No documents is available for A64 R_INTC.

No code either. In Allwinner's BSP, the interrupts for the PMICs go
through the (closed source) OpenRISC firmware, so there's no driver
for it in the kernel.

The registers line up with the old interrupt controller from the

A10,

quoted

quoted

but it seems only the NMI interrupt is wired up.

Is this OK? Or do you want Icenowy to respin a version with defines?

Ideally, I'd like to see some #defines, but given that the rest of the
file is already littered with hard-coded constants, you might as well
do
the whole thing in a subsequent patch that I would merge with these two
patches.

Personally I think the values are self-explained (the variable
name) and used only once in this structure.

Making defines doesn't make the code more clear.

That's where you're severely misguided. There is plenty of places in the
kernel where constants are used exactly once, and yet they have a
#define. And the reason for this is consistency. We don't deal with raw
values, we deal with named constants.

For you, this code is probably "write-only". Once it works, you'll never
go back to it. On the other hand, I get to maintain it and apply tree
wide changes as necessary. So if the code looks better *to me*, then
that's the way.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...