Re: [RFC PATCH v2 14/30] drivers/irqchip: Add SH7751 Internal INTC drivers.

On Tue, 19 Sep 2023 20:50:14 +0900,
Geert Uytterhoeven wrote:
> 
> Hi Sato-san,
> 
> On Wed, Sep 13, 2023 at 11:24 AM Yoshinori Sato
> <[email protected]> wrote:
> > Signed-off-by: Yoshinori Sato <[email protected]>
> 
> Thanks for your patch!
> 
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -679,4 +679,13 @@ config SUNPLUS_SP7021_INTC
> >           chained controller, routing all interrupt source in P-Chip to
> >           the primary controller on C-Chip.
> >
> > +config RENESAS_SH7751_INTC
> > +        bool "Renesas SH7751 Interrupt Controller"
> > +       depends on SH_DEVICE_TREE
> 
> "|| COMPILE_TEST"?
> 
> > +       select IRQ_DOMAIN
> > +       select IRQ_DOMAIN_HIERARCHY
> > +       help
> > +         Support for the Renesas SH7751 On-chip interrupt controller.
> > +         And external interrupt encoder for some targets.
> 
> Inconsistent indentation
> 
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-renesas-sh7751.c
> 
> > +/* INTEVT to IPR mapping */
> > +static const struct iprmap {
> > +       int intevt;
> 
> irq, as you're storing the irq number not the event number?
> 
> > +       int off;
> > +       int bit;
> 
> All unsigned int ...
> 
> > +} iprmaps[] = {
> > +#define IPRDEF(e, o, b) { .intevt = evt2irq(e), .off = o, .bit = b }
> > +       IPRDEF(0x240, IPRD, IPR_B12),   /* IRL0 */
> > +       IPRDEF(0x2a0, IPRD, IPR_B8),    /* IRL1 */
> > +       IPRDEF(0x300, IPRD, IPR_B4),    /* IRL2 */
> > +       IPRDEF(0x360, IPRD, IPR_B0),    /* IRL3 */
> > +       IPRDEF(0x400, IPRA, IPR_B12),   /* TMU0 */
> > +       IPRDEF(0x420, IPRA, IPR_B8),    /* TMU1 */
> > +       IPRDEF(0x440, IPRA, IPR_B4),    /* TMU2 TNUI */
> > +       IPRDEF(0x460, IPRA, IPR_B4),    /* TMU2 TICPI */
> > +       IPRDEF(0x480, IPRA, IPR_B0),    /* RTC ATI */
> > +       IPRDEF(0x4a0, IPRA, IPR_B0),    /* RTC PRI */
> > +       IPRDEF(0x4c0, IPRA, IPR_B0),    /* RTC CUI */
> > +       IPRDEF(0x4e0, IPRB, IPR_B4),    /* SCI ERI */
> > +       IPRDEF(0x500, IPRB, IPR_B4),    /* SCI RXI */
> > +       IPRDEF(0x520, IPRB, IPR_B4),    /* SCI TXI */
> > +       IPRDEF(0x540, IPRB, IPR_B4),    /* SCI TEI */
> > +       IPRDEF(0x560, IPRB, IPR_B12),   /* WDT */
> > +       IPRDEF(0x580, IPRB, IPR_B8),    /* REF RCMI */
> > +       IPRDEF(0x5a0, IPRB, IPR_B4),    /* REF ROVI */
> > +       IPRDEF(0x600, IPRC, IPR_B0),    /* H-UDI */
> > +       IPRDEF(0x620, IPRC, IPR_B12),   /* GPIO */
> > +       IPRDEF(0x640, IPRC, IPR_B8),    /* DMAC DMTE0 */
> > +       IPRDEF(0x660, IPRC, IPR_B8),    /* DMAC DMTE1 */
> > +       IPRDEF(0x680, IPRC, IPR_B8),    /* DMAC DMTE2 */
> > +       IPRDEF(0x6a0, IPRC, IPR_B8),    /* DMAC DMTE3 */
> > +       IPRDEF(0x6c0, IPRC, IPR_B8),    /* DMAC DMAE */
> > +       IPRDEF(0x700, IPRC, IPR_B4),    /* SCIF ERI */
> > +       IPRDEF(0x720, IPRC, IPR_B4),    /* SCIF RXI */
> > +       IPRDEF(0x740, IPRC, IPR_B4),    /* SCIF BRI */
> > +       IPRDEF(0x760, IPRC, IPR_B4),    /* SCIF TXI */
> > +       IPRDEF(0x780, IPRC, IPR_B8),    /* DMAC DMTE4 */
> > +       IPRDEF(0x7a0, IPRC, IPR_B8),    /* DMAC DMTE5 */
> > +       IPRDEF(0x7c0, IPRC, IPR_B8),    /* DMAC DMTE6 */
> > +       IPRDEF(0x7e0, IPRC, IPR_B8),    /* DMAC DMTE7 */
> > +       IPRDEF(0xa00, INTPRI00, IPR_B0),        /* PCIC PCISERR */
> > +       IPRDEF(0xa20, INTPRI00, IPR_B4),        /* PCIC PCIDMA3 */
> > +       IPRDEF(0xa40, INTPRI00, IPR_B4),        /* PCIC PCIDMA2 */
> > +       IPRDEF(0xa60, INTPRI00, IPR_B4),        /* PCIC PCIDMA1 */
> > +       IPRDEF(0xa80, INTPRI00, IPR_B4),        /* PCIC PCIDMA0 */
> > +       IPRDEF(0xaa0, INTPRI00, IPR_B4),        /* PCIC PCIPWON */
> > +       IPRDEF(0xac0, INTPRI00, IPR_B4),        /* PCIC PCIPWDWN */
> > +       IPRDEF(0xae0, INTPRI00, IPR_B4),        /* PCIC PCIERR */
> > +       IPRDEF(0xb00, INTPRI00, IPR_B8),        /* TMU3 */
> > +       IPRDEF(0xb80, INTPRI00, IPR_B12),       /* TMU4 */
> 
> Probably the same or a very similar interrupt controller is present
> on other SoCs? Then the comments don't make much sense, as the actual
> interrupt mapping is specified in the DTS anyway.

This interrupt controller design is quite old, so there doesn't seem to be
any SoC with a similar design.
Since the SH interrupt controllers have almost the same design,
I think this driver can be used for other devices besides the SH7751.
I think a good way to write IPR mapping is to use dts.

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Yosinori Sato