Hi Herve, On 15:50 Thu 09 Jan , Herve Codina wrote: > Hi Andrea, > > On Thu, 9 Jan 2025 15:13:42 +0100 > Andrea della Porta <andrea.porta@xxxxxxxx> wrote: > > > Hi Rob, > > > > On 15:08 Mon 16 Dec , Andrea della Porta wrote: > > > Hi Rob, > > > > > > On 16:48 Tue 10 Dec , Rob Herring wrote: > > > > On Mon, Dec 02, 2024 at 12:19:32PM +0100, Andrea della Porta wrote: > > > > > The RaspberryPi RP1 is a PCI multi function device containing > > > > > peripherals ranging from Ethernet to USB controller, I2C, SPI > > > > > and others. > > > > ... > > > > > > > +#define RP1_INT_ADC_FIFO 52 > > > > > +#define RP1_INT_PCIE_OUT 53 > > > > > +#define RP1_INT_SPI6 54 > > > > > +#define RP1_INT_SPI7 55 > > > > > +#define RP1_INT_SPI8 56 > > > > > +#define RP1_INT_SYSCFG 58 > > > > > +#define RP1_INT_CLOCKS_DEFAULT 59 > > > > > +#define RP1_INT_VBUSCTRL 60 > > > > > +#define RP1_INT_PROC_MISC 57 > > > > > > > > Why all these defines which will never be used because they come from > > > > DT? > > > > > > > > > > Right, those defines where originally designed to be included from dts, but > > > previous discussion deemed interrupt numbers to be hardcoded instead of being > > > specified as mnemonics. In the driver source code I just use RP1_INT_END as the > > > number of interrupts but I thought that the specific interrupt numbers should > > > be documented in some way or another. Since no one is currently referencing > > > those defines, would it be better to just turn those in a multiline comment > > > just to describe them in a more compact form? > > > > So, here's a couple of proposals about the interrupt defines: > > > > - since they were banned from devicetree, and are not used anywhere in the code, > > turn them into a (admittedly long) multiline comment, so they are still at > > least documented > > > > - since they were banned from devicetree, and are not use anywhere in the code, > > just drop them, we don't currently need them after all > > > > Not sure what's the best way here, anyone can advise? > > Maybe in the #interrupt-cells description in the device-tree binding? > > In your patch 4, you describe this interrupt controller and you have: > '#interrupt-cells': > const: 2 > description: > Specifies respectively the interrupt number and flags as defined > in include/dt-bindings/interrupt-controller/irq.h. > > In this description, why not add the supported interrupt number values? > description: | > Specifies respectively the interrupt number and flags as defined > in include/dt-bindings/interrupt-controller/irq.h. > The supported values for the interrupt number are: > - IO BANK0: 0 > - IO BANK1: 1 > ... > > Or something similar. > > This kind of description is already available. For instance: > https://elixir.bootlin.com/linux/v6.13-rc1/source/Documentation/devicetree/bindings/dma/fsl,imx-sdma.yaml#L64 > > Does it make sense? Seems fine to me, if there's no concern from anyone I will procede like that. Thanks for the suggestion. Andrea > > Best regards, > Hervé
