On 2018-02-23 14:37, Marc Zyngier wrote: > Hi Rasmus, > > On 23/02/18 12:16, Rasmus Villemoes wrote: >> On 2018-02-02 15:58, Marc Zyngier wrote: >>> Why 3? Reading the DT binding, this is indeed set to 3 without any >>> reason. I'd suggest this becomes 2, encoding the pin number and the >>> trigger information, as the leading 0 is quite useless. Yes, I know >>> other examples in the kernel are using this 0, and that was a >>> consequence of retrofitting the omitted interrupt controllers (back in >>> the days of the stupid gic_arch_extn...). Don't do that. >>> >> >> Hi Marc >> >> I'm about to send out a new revision of the ls-extirq patchset, and >> thanks to you pointing me to these patches, I've read the comments on >> the various revisions of this series and tried to take those into >> account. But the above confused me, because in response to my first RFC >> (https://patchwork.kernel.org/patch/10102643/) we have >> >> On 2017-12-08 17:09, Marc Zyngier wrote: >>> On 08/12/17 15:11, Alexander Stein wrote: >>>> Hi Rasmus, >>>> >>>>> + >>>>> +Required properties: >>>>> +- compatible: should be "fsl,ls1021a-extirq" >>>>> +- interrupt-controller: Identifies the node as an interrupt controller >>>>> +- #interrupt-cells: Use the same format as specified by GIC in >> arm,gic.txt. >>>> >>>> Do you really need 3 interrupt-cells here? As you've written below >> you don't >>>> support PPI anyway the 1st flag might be dropped then. So support >> just 2 cells: >>>> * IRQ number (IRQ0 - IRQ5) >>>> * IRQ flags >>> >>> The convention for irqchip stacked on top of a GIC is to keep the >>> interrupt specifier the same. It makes the maintenance if the DT much >>> easier, and doesn't hurt at all. >> >> Personally, I'd actually prefer the simpler interrupt specifiers without >> a redundant 0. Maybe I'm just missing some difference between this case >> and the ls-extirq one? > The difference is that you're adding a new irqchip to an existing DT, > and you get some possible breakage. Maybe you'd be happy with the > breakage, that's your call (and the maintainer's). OK. In the ls1021a case, I actually think "breaking" any existing users if and when they move to the new driver/irqchip is a good thing: the power-on-reset value is such that the lines have the polarity inverted. So there could be some board with a device with either a EDGE_FALLING or LEVEL_LOW interrupt connected to one of the external interrupt lines, which is described in DT by "lying" and using the opposite flag. Changing #interrupt-cells prevents such (ab)users from just changing interrupt-parent and calling it a day. In the QC case, it is > old brand new, so no harm in doing the right thing from day one> > It is in the end an implementation decision, and you could go either way. Doing the right thing sounds nice, so I'll go with that :) Thanks, Rasmus -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
