> On 13.11.2017, at 10:35, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: > > On Sun, Nov 12 2017 at 5:49:39 pm GMT, <kernel@xxxxxxxxxxxxxxxx> wrote: >>> On 12.11.2017, at 16:41, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: >>>> + >>>> + can0: mcp2517fd@0 { >>>> + reg = <0>; >>>> + compatible = "microchip,mcp2517fd"; >>>> + pinctrl-names = "default"; >>>> + pinctrl-0 = <&can0_pins>; >>>> + spi-max-frequency = <12500000>; >>>> + interrupt-parent = <&gpio>; >>>> + interrupts = <16 0x2>; >>> >>> This indicates a falling edge. No wonder the kernel is confused (I >>> don't know why this isn't enforced the first time though, probably an >>> issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should >>> allow you to make some progress. >> >> Thanks for the clarification - with that change it works! >> >> For a better understanding: >> Isn’t the interrupt type to use more of a driver decision than a >> HW implementation detail that needs to get defined in the device tree? > > Absolutely *not*. The signalling of an interrupt is completely HW > dependent, and the driver *must* use abide by the HW rule. That's > because edge and level interrupts signal entirely different things: > > - An edge interrupt signals a an event. Something has happened, and many > of these events can be signalled independently without the CPU doing > anything. > > - A level interrupt indicates a change of state, and this state persist > until the CPU has services the interrupt. > > That's the difference between receiving a SMS each time you pay > something your bank card, and receiving a phone call from your bank > because your account is overdrawn. So for all practical purposes any typical active low interrupt line connected to a GPIO should be configured as TRIGGER_LOW in the device tree. Under this assumption: There are actually quite a few drivers that are having a /INT line, but are configured in the DT bindings as 2 - hence TRIGGER_FALLING. Two of those are: * Documentation/devicetree/bindings/net/can/microchip,mcp251x.txt * Documentation/devicetree/bindings/net/microchip,enc28j60.txt The enc28j60 does use flags = 0, which seems could be ok, as it is not requesting some specific kind of irq, while the other is using flags = IRQF_ONESHOT | IRQF_TRIGGER_FALLING, which is incorrect. That was one of the reasons why I was adding this subsequent question for clarification. So I wonder how one would go about correcting those devices... > >> In my case I probably could write some more code that would allow edge >> interrupts to work (without race-conditions on spi transfers - probably >> by using spi_async to reenable interrupts on the HW device), but it >> would not be as straight-forward and a bit more complex. > > And more or less wrong, given that the spec sheet calls out "active low" > interrupts. It would have been a workaround which would not have been accepted anyway. >> Summary: Essentially the driver has to match the interrupt type - >> otherwise it will fail (on second initialization). > > And even that is not quite right. The driver should fail straight > away. on the first request. I don't have the HW to track it down, but > I'd appreciate it if you could have a look and enlighten me. When I have finished this driver I am working on right now, then I may look into it in detail. But if I remember the bits and pieces I have been looking at before asking this question: I remember that there is some code in irq_create_fwspec_mapping that allows for first time initialization. The best I can do is: * in __setup_irq: * the Trigger-type flags are set to “defaults” if none are set * otherwise the flags are used without matching them against what is configured in the device tree. * so there is no validation happening in case the types disagree. Something like this patch would trigger an error on first try (unless flags = 0 or flags = “correct”): diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index ea41820ab12e..c2dd1b4b879a 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1136,8 +1136,15 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) * If the trigger type is not specified by the caller, * then use the default for this interrupt. */ - if (!(new->flags & IRQF_TRIGGER_MASK)) - new->flags |= irqd_get_trigger_type(&desc->irq_data); + flags = irqd_get_trigger_type(&desc->irq_data); + if (!(new->flags & IRQF_TRIGGER_MASK)) { + new->flags |= flags; + } else if ((new->flags & IRQF_TRIGGER_MASK) != flags) { + pr_err("Requested trigger type %i does not match default %lu\n", + new->flags & IRQF_TRIGGER_MASK, + flags); + return -EINVAL; + } /* * Check whether the interrupt nests into another interrupt Patch is against 4.9.57 - please ignore the whitespace issue as it is a simple console cut/paste. It does not trigger anything else on my system, but there may be other use-cases that would fail because of this. Here some of the debugging output (see the irq_flags module parameter) root@raspcm:~# rmmod mcp2517fd; dmesg -c; modprobe mcp2517fd irq_flags=2; dmesg -c; grep mcp2517fd /proc/interrupts rmmod: ERROR: Module mcp2517fd is not currently loaded [ 36.329522] genirq: Requested trigger type 2 does not match default 8 [ 36.329556] mcp2517fd spi0.0: failed to acquire irq 176 - -22 [ 36.329611] mcp2517fd: probe of spi0.0 failed with error -22 root@raspcm:~# rmmod mcp2517fd; dmesg -c; modprobe mcp2517fd irq_flags=0; dmesg -c; grep mcp2517fd /proc/interrupts 176: 0 pinctrl-bcm2835 16 Level mcp2517fd root@raspcm:~# rmmod mcp2517fd; dmesg -c; modprobe mcp2517fd irq_flags=8; dmesg -c; grep mcp2517fd /proc/interrupts 176: 0 pinctrl-bcm2835 16 Level mcp2517fd Hope this analysis helps... Martin -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html