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

Geert Uytterhoeven <[email protected]>
Newsgroups gmane.linux.ports.sh.devel
Message-ID <CAMuHMdXv3TPa=mCrso9P1Mbh8yCAzNusiX9ELF3mFCi4oY0igA@mail.gmail.com>
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.

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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.