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

Andrea della Porta
2025-01-09T15:53:06Z

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@suse.com> 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é