Re: [PATCH v5 08/10] misc: rp1: RaspberryPi RP1 misc driver

Hi Andrea,

On Thu, 9 Jan 2025 15:13:42 +0100
Andrea della Porta [off-list ref] wrote:

Hi Rob,

On 15:08 Mon 16 Dec     , Andrea della Porta wrote:

quoted

Hi Rob,

On 16:48 Tue 10 Dec     , Rob Herring wrote:  

quoted

On Mon, Dec 02, 2024 at 12:19:32PM +0100, Andrea della Porta wrote:  

quoted

The RaspberryPi RP1 is a PCI multi function device containing
peripherals ranging from Ethernet to USB controller, I2C, SPI
and others.  

...

quoted

quoted

quoted

+#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?

Best regards,
Hervé