Message-ID: <CAMuHMdW9kfhr3d=yG8yfAuX+yBTR5vQr8XRi5ymbRyEVfWAjDg@mail.gmail.com>
Date:   Thu, 12 May 2022 09:05:51 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Phil Edworthy <phil.edworthy@...esas.com>,
        Biju Das <biju.das.jz@...renesas.com>,
        Prabhakar <prabhakar.csengg@...il.com>
Subject: Re: [PATCH v3 2/5] irqchip: Add RZ/G2L IA55 Interrupt Controller driver

Hi Prabhakar,

On Wed, May 11, 2022 at 8:32 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@...renesas.com> wrote:
> Add a driver for the Renesas RZ/G2L Interrupt Controller.
>
> This supports external pins being used as interrupts. It supports
> one line for NMI, 8 external pins and 32 GPIO pins (out of 123)
> to be used as IRQ lines.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>

LGTM ("I'm no irqchip expert"), so
Reviewed-by: Geert Uytterhoeven <geert+renesas@...der.be>

One suggestion for improvement below, which needs changes to the
irqchip core code, though.

> --- /dev/null
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c

> +static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
> +{
> +       struct irq_domain *irq_domain, *parent_domain;
> +       struct reset_control *resetn;
> +       struct rzg2l_irqc_priv *priv;
> +       struct clk *clk;
> +       struct clk *pclk;
> +       int ret;
> +
> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->base = of_iomap(node, 0);
> +       if (!priv->base) {
> +               ret = -ENXIO;
> +               goto free_priv;
> +       }
> +
> +       clk = of_clk_get_by_name(node, "clk");
> +       if (IS_ERR(clk)) {
> +               ret = IS_ERR(clk);
> +               goto iounmap_base;
> +       }
> +
> +       pclk = of_clk_get_by_name(node, "pclk");
> +       if (IS_ERR(pclk)) {
> +               ret = IS_ERR(pclk);
> +               goto iounmap_base;
> +       }
> +
> +       resetn = of_reset_control_get_exclusive_by_index(node, 0);
> +       if (IS_ERR(resetn)) {
> +               ret = IS_ERR(resetn);
> +               goto iounmap_base;
> +       }
> +
> +       parent_domain = irq_find_host(parent);
> +       if (!parent_domain) {
> +               pr_err("%pOF: cannot find parent domain\n", node);
> +               ret = -ENODEV;
> +               goto iounmap_base;
> +       }
> +
> +       ret = rzg2l_irqc_parse_map(priv, node);
> +       if (ret) {
> +               pr_err("%pOF: cannot parse interrupts: %d\n", node, ret);
> +               goto iounmap_base;
> +       }
> +
> +       ret = reset_control_deassert(resetn);
> +       if (ret) {
> +               pr_err("%pOF: failed to deassert resetn pin, %d\n", node, ret);
> +               goto iounmap_base;
> +       }
> +
> +       raw_spin_lock_init(&priv->lock);
> +
> +       ret = clk_prepare_enable(clk);
> +       if (ret)
> +               goto assert_reset;
> +
> +       ret = clk_prepare_enable(pclk);
> +       if (ret)
> +               goto disable_clk;

As the driver doesn't care about the properties of the clock, but
just needs to enable them, I think it would be worthwhile to replace
the explicit clock handling by Runtime PM.
But that needs access to the struct device pointer.
Of course that can be done later (see below).

> +
> +       irq_domain = irq_domain_add_hierarchy(parent_domain, 0, IRQC_NUM_IRQ,
> +                                             node, &rzg2l_irqc_domain_ops,
> +                                             priv);
> +       if (!irq_domain) {
> +               pr_err("%pOF: cannot initialize irq domain\n", node);
> +               ret = -ENOMEM;
> +               goto fail_irq_domain;
> +       }
> +
> +       return 0;
> +
> +fail_irq_domain:
> +       clk_disable_unprepare(pclk);
> +disable_clk:
> +       clk_disable_unprepare(clk);
> +assert_reset:
> +       reset_control_assert(resetn);
> +iounmap_base:
> +       iounmap(priv->base);
> +free_priv:
> +       kfree(priv);
> +       return ret;
> +}
> +
> +IRQCHIP_PLATFORM_DRIVER_BEGIN(rzg2l_irqc)
> +IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_init)
> +IRQCHIP_PLATFORM_DRIVER_END(rzg2l_irqc)

While platform_irqchip_probe() has access to the platform_device,
it does not pass that to the driver's initialization function, so
that function does not have access to the device framework.
Hence it can not be used for interrupt controllers that are part of a
Power Domain, as, unlike clocks, Power Domains can only be controlled
from Runtime PM, and thus require access to a struct device pointer.
I think it would be good to fix this sooner rather than later; there
are only a handful of users of this interface yet.

> +MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>");
> +MODULE_DESCRIPTION("Renesas RZ/G2L IRQC Driver");
> +MODULE_LICENSE("GPL");

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

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

Powered by blists - more mailing lists