* [PATCH] irqchip/gic: Use GIC_SPI symbolic constant
@ 2017-07-16 17:40 Mason
2017-07-16 20:13 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Mason @ 2017-07-16 17:40 UTC (permalink / raw)
To: linux-arm-kernel
Use GIC_SPI explicitly instead of an implicit 0.
Signed-off-by: Mason <slash.tmp@free.fr>
---
FWIW, 'make C=2' flagged a few lines:
$ make C=2 drivers/irqchip/irq-gic.o
CHECK drivers/irqchip/irq-gic.c
drivers/irqchip/irq-gic.c:1079:44: warning: incorrect type in assignment (different address spaces)
drivers/irqchip/irq-gic.c:1079:44: expected void [noderef] <asn:3>*[noderef] <asn:2>*percpu_base
drivers/irqchip/irq-gic.c:1079:44: got void [noderef] <asn:2>*[noderef] <asn:3>*<noident>
drivers/irqchip/irq-gic.c:1080:43: warning: incorrect type in assignment (different address spaces)
drivers/irqchip/irq-gic.c:1080:43: expected void [noderef] <asn:3>*[noderef] <asn:2>*percpu_base
drivers/irqchip/irq-gic.c:1080:43: got void [noderef] <asn:2>*[noderef] <asn:3>*<noident>
drivers/irqchip/irq-gic.c:1091:26: warning: incorrect type in initializer (different address spaces)
drivers/irqchip/irq-gic.c:1091:26: expected void const [noderef] <asn:3>*__vpp_verify
drivers/irqchip/irq-gic.c:1091:26: got void [noderef] <asn:3>*[noderef] <asn:2>*<noident>
drivers/irqchip/irq-gic.c:1091:71: warning: incorrect type in assignment (different address spaces)
drivers/irqchip/irq-gic.c:1091:71: expected void [noderef] <asn:3>*<noident>
drivers/irqchip/irq-gic.c:1091:71: got void [noderef] <asn:2>*
drivers/irqchip/irq-gic.c:1093:26: warning: incorrect type in initializer (different address spaces)
drivers/irqchip/irq-gic.c:1093:26: expected void const [noderef] <asn:3>*__vpp_verify
drivers/irqchip/irq-gic.c:1093:26: got void [noderef] <asn:3>*[noderef] <asn:2>*<noident>
drivers/irqchip/irq-gic.c:1093:70: warning: incorrect type in assignment (different address spaces)
drivers/irqchip/irq-gic.c:1093:70: expected void [noderef] <asn:3>*<noident>
drivers/irqchip/irq-gic.c:1093:70: got void [noderef] <asn:2>*
drivers/irqchip/irq-gic.c:1167:43: warning: incorrect type in argument 1 (different address spaces)
drivers/irqchip/irq-gic.c:1167:43: expected void [noderef] <asn:3>*__pdata
drivers/irqchip/irq-gic.c:1167:43: got void [noderef] <asn:3>*[noderef] <asn:2>*percpu_base
drivers/irqchip/irq-gic.c:1168:42: warning: incorrect type in argument 1 (different address spaces)
drivers/irqchip/irq-gic.c:1168:42: expected void [noderef] <asn:3>*__pdata
drivers/irqchip/irq-gic.c:1168:42: got void [noderef] <asn:3>*[noderef] <asn:2>*percpu_base
---
drivers/irqchip/irq-gic.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 1b1df4f770bd..ae414f5223b6 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -42,6 +42,8 @@
#include <linux/irqchip/chained_irq.h>
#include <linux/irqchip/arm-gic.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
#include <asm/cputype.h>
#include <asm/irq.h>
#include <asm/exception.h>
@@ -990,7 +992,7 @@ static int gic_irq_domain_translate(struct irq_domain *d,
* For SPIs, we need to add 16 more to get the GIC irq
* ID number
*/
- if (!fwspec->param[0])
+ if (fwspec->param[0] == GIC_SPI)
*hwirq += 16;
*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
--
2.8.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] irqchip/gic: Use GIC_SPI symbolic constant
2017-07-16 17:40 [PATCH] irqchip/gic: Use GIC_SPI symbolic constant Mason
@ 2017-07-16 20:13 ` Marc Zyngier
2017-07-16 20:50 ` Mason
0 siblings, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2017-07-16 20:13 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Jul 16 2017 at 7:40:06 pm BST, Mason <slash.tmp@free.fr> wrote:
> Use GIC_SPI explicitly instead of an implicit 0.
What bug is this fixing? What benefit does this bring?
>
> Signed-off-by: Mason <slash.tmp@free.fr>
> ---
> FWIW, 'make C=2' flagged a few lines:
>
> $ make C=2 drivers/irqchip/irq-gic.o
> CHECK drivers/irqchip/irq-gic.c
> drivers/irqchip/irq-gic.c:1079:44: warning: incorrect type in assignment (different address spaces)
> drivers/irqchip/irq-gic.c:1079:44: expected void [noderef] <asn:3>*[noderef] <asn:2>*percpu_base
> drivers/irqchip/irq-gic.c:1079:44: got void [noderef] <asn:2>*[noderef] <asn:3>*<noident>
How is that related to this patch?
M.
--
Jazz is not dead. It just smells funny.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] irqchip/gic: Use GIC_SPI symbolic constant
2017-07-16 20:13 ` Marc Zyngier
@ 2017-07-16 20:50 ` Mason
2017-07-16 21:08 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Mason @ 2017-07-16 20:50 UTC (permalink / raw)
To: linux-arm-kernel
On 16/07/2017 22:13, Marc Zyngier wrote:
> Mason wrote:
>
>> Use GIC_SPI explicitly instead of an implicit 0.
>
> What bug is this fixing? What benefit does this bring?
The patch aims to replace an (implicit) literal constant
with the corresponding symbolic macro. IMO, it makes the
intent somewhat clearer, and, more importantly, grepping
for said symbol now returns the respective file/line.
(It took me a while to find the line.)
Are you saying that changing the code at this point is
not worth the trouble?
>> FWIW, 'make C=2' flagged a few lines:
>>
>> $ make C=2 drivers/irqchip/irq-gic.o
>> CHECK drivers/irqchip/irq-gic.c
>> drivers/irqchip/irq-gic.c:1079:44: warning: incorrect type in assignment (different address spaces)
>> drivers/irqchip/irq-gic.c:1079:44: expected void [noderef] <asn:3>*[noderef] <asn:2>*percpu_base
>> drivers/irqchip/irq-gic.c:1079:44: got void [noderef] <asn:2>*[noderef] <asn:3>*<noident>
>
> How is that related to this patch?
I tested the patch with 'make C=2' and reported the
output FWIW. Are you saying I should have written
a separate message, or not bothered altogether?
Regards.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] irqchip/gic: Use GIC_SPI symbolic constant
2017-07-16 20:50 ` Mason
@ 2017-07-16 21:08 ` Marc Zyngier
2017-07-16 21:29 ` Mason
2017-07-17 8:07 ` Thomas Petazzoni
0 siblings, 2 replies; 6+ messages in thread
From: Marc Zyngier @ 2017-07-16 21:08 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Jul 16 2017 at 10:50:17 pm BST, Mason <slash.tmp@free.fr> wrote:
> On 16/07/2017 22:13, Marc Zyngier wrote:
>
>> Mason wrote:
>>
>>> Use GIC_SPI explicitly instead of an implicit 0.
>>
>> What bug is this fixing? What benefit does this bring?
>
> The patch aims to replace an (implicit) literal constant
> with the corresponding symbolic macro. IMO, it makes the
> intent somewhat clearer, and, more importantly, grepping
> for said symbol now returns the respective file/line.
> (It took me a while to find the line.)
>
> Are you saying that changing the code at this point is
> not worth the trouble?
You're assuming that this GIC_SPI macro has anything to do with the GIC
driver. It doesn't. That's just a convenience macro for people writing
DT, and definitely not something I'd ever want to rely on in the Linux
driver. The binding defines the raw value, and not this macro.
So to sum it up, thank you, but no thank out.
>
>>> FWIW, 'make C=2' flagged a few lines:
>>>
>>> $ make C=2 drivers/irqchip/irq-gic.o
>>> CHECK drivers/irqchip/irq-gic.c
>>> drivers/irqchip/irq-gic.c:1079:44: warning: incorrect type in
>>> assignment (different address spaces)
>>> drivers/irqchip/irq-gic.c:1079:44: expected void [noderef]
>>> <asn:3>*[noderef] <asn:2>*percpu_base
>>> drivers/irqchip/irq-gic.c:1079:44: got void [noderef]
>>> <asn:2>*[noderef] <asn:3>*<noident>
>>
>> How is that related to this patch?
>
> I tested the patch with 'make C=2' and reported the
> output FWIW. Are you saying I should have written
> a separate message, or not bothered altogether?
An even better outcome would be analysing the issue and coming up with a
patch if there is something to fix. On its own, dumping these warnings
in the middle of something unrelated is a best at waste of bandwidth.
M.
--
Jazz is not dead. It just smells funny.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] irqchip/gic: Use GIC_SPI symbolic constant
2017-07-16 21:08 ` Marc Zyngier
@ 2017-07-16 21:29 ` Mason
2017-07-17 8:07 ` Thomas Petazzoni
1 sibling, 0 replies; 6+ messages in thread
From: Mason @ 2017-07-16 21:29 UTC (permalink / raw)
To: linux-arm-kernel
On 16/07/2017 23:08, Marc Zyngier wrote:
> Mason wrote:
>
>> On 16/07/2017 22:13, Marc Zyngier wrote:
>>
>>> Mason wrote:
>>>
>>>> Use GIC_SPI explicitly instead of an implicit 0.
>>>
>>> What bug is this fixing? What benefit does this bring?
>>
>> The patch aims to replace an (implicit) literal constant
>> with the corresponding symbolic macro. IMO, it makes the
>> intent somewhat clearer, and, more importantly, grepping
>> for said symbol now returns the respective file/line.
>> (It took me a while to find the line.)
>>
>> Are you saying that changing the code at this point is
>> not worth the trouble?
>
> You're assuming that this GIC_SPI macro has anything to do with the GIC
> driver. It doesn't. That's just a convenience macro for people writing
> DT, and definitely not something I'd ever want to rely on in the Linux
> driver. The binding defines the raw value, and not this macro.
AFAIU, you're referring to
Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
"The 1st cell is the interrupt type;
0 for SPI interrupts, 1 for PPI interrupts."
Doesn't it make sense to have symbolic constants for
the 0 and 1 above? In linux/irqchip/arm-gic.h ?
IIUC, headers in include/dt-bindings are just convenience
macros, and are not to be included from driver code?
Regards.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] irqchip/gic: Use GIC_SPI symbolic constant
2017-07-16 21:08 ` Marc Zyngier
2017-07-16 21:29 ` Mason
@ 2017-07-17 8:07 ` Thomas Petazzoni
1 sibling, 0 replies; 6+ messages in thread
From: Thomas Petazzoni @ 2017-07-17 8:07 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Sun, 16 Jul 2017 22:08:01 +0100, Marc Zyngier wrote:
> > Are you saying that changing the code at this point is
> > not worth the trouble?
>
> You're assuming that this GIC_SPI macro has anything to do with the GIC
> driver. It doesn't. That's just a convenience macro for people writing
> DT, and definitely not something I'd ever want to rely on in the Linux
> driver. The binding defines the raw value, and not this macro.
>
> So to sum it up, thank you, but no thank out.
FWIW, a few drivers are already using GIC_SPI:
irq-mvebu-gicp.c: fwspec.param[0] = GIC_SPI;
irq-mvebu-odmi.c: fwspec.param[0] = GIC_SPI;
irq-tegra.c: if (fwspec->param[0] != GIC_SPI)
irq-vf610-mscm-ir.c: parent_fwspec.param[0] = GIC_SPI;
For pretty much exactly the situation being patched by Mason.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-17 8:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-16 17:40 [PATCH] irqchip/gic: Use GIC_SPI symbolic constant Mason
2017-07-16 20:13 ` Marc Zyngier
2017-07-16 20:50 ` Mason
2017-07-16 21:08 ` Marc Zyngier
2017-07-16 21:29 ` Mason
2017-07-17 8:07 ` Thomas Petazzoni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox