RE: [PATCH 2/4] irqchip: add RZ/{T2H,N2H} Interrupt Controller (ICU) driver

[Cosmin-Gabriel Tanislav]

> -----Original Message-----
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Sent: Saturday, November 22, 2025 5:56 PM
> To: Cosmin-Gabriel Tanislav <cosmin-gabriel.tanislav.xa@xxxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Geert Uytterhoeven
> <geert+renesas@xxxxxxxxx>; magnus.damm <magnus.damm@xxxxxxxxx>; Cosmin-Gabriel Tanislav <cosmin-
> gabriel.tanislav.xa@xxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-renesas-soc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/4] irqchip: add RZ/{T2H,N2H} Interrupt Controller (ICU) driver
>
> On Fri, Nov 21 2025 at 13:14, Cosmin Tanislav wrote:
> > +static inline int rzt2h_icu_irq_to_offset(struct irq_data *d, void __iomem **base,
> > +                                     unsigned int *offset)
> > +{
> > +   struct rzt2h_icu_priv *priv = irq_data_to_priv(d);
> > +   unsigned int hwirq = irqd_to_hwirq(d);
> > +
> > +   if (RZT2H_ICU_IRQ_IN_RANGE(hwirq, IRQ_NS)) {
> > +           *offset = hwirq - RZT2H_ICU_IRQ_NS_START;
> > +           *base = priv->base_ns;
> > +   } else if (RZT2H_ICU_IRQ_IN_RANGE(hwirq, IRQ_S) ||
> > +              /* SEI follows safety IRQs in registers and in IRQ numbers. */
> > +              RZT2H_ICU_IRQ_IN_RANGE(hwirq, SEI)) {
>
> This nested commend in the condition is really unreadable.
>

Would this read better in your opinion?

        /*
         * Safety IRQs and SEI use a separate register space from the non-safety IRQs.
         * SEI interrupt number follows immediately after the safety IRQs.
         */
        if (RZT2H_ICU_IRQ_IN_RANGE(hwirq, IRQ_NS)) {
                *offset = hwirq - RZT2H_ICU_IRQ_NS_START;
                *base = priv->base_ns;
        } else if (RZT2H_ICU_IRQ_IN_RANGE(hwirq, IRQ_S) ||
                   RZT2H_ICU_IRQ_IN_RANGE(hwirq, SEI)) {
                *offset = hwirq - RZT2H_ICU_IRQ_S_START;
                *base = priv->base_s;
        } else {
                return -EINVAL;
        }

One more thing, for the above cases where the same macro is used twice
in a condition, is it okay to keep it split across two lines to align
them with each other, or do you want them on a single line up to 100
columns?

> > +           *offset = hwirq - RZT2H_ICU_IRQ_S_START;
> > +           *base = priv->base_s;
> > +   } else {
> > +           return -EINVAL;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +static int rzt2h_icu_irq_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +   struct rzt2h_icu_priv *priv = irq_data_to_priv(d);
> > +   unsigned int parent_type;
> > +   unsigned int offset;
>
> Combine same data types into one line please.
>

Ack.

> > +   void __iomem *base;
> > +   u32 val, md;
> > +   int ret;
>
>
> > +   guard(raw_spinlock)(&priv->lock);
> > +   val = readl_relaxed(base + RZT2H_ICU_PORTNF_MD);
> > +   val &= ~RZT2H_ICU_PORTNF_MDi_MASK(offset);
> > +   val |= RZT2H_ICU_PORTNF_MDi_PREP(offset, md);
> > +   writel_relaxed(val, base + RZT2H_ICU_PORTNF_MD);
> > +
>
> This looks wrong. guard() holds the lock across the set_parent()
> call. If you really need that then this needs a comment explaining the
> why. Otherwise use scoped_guard().
>

Ack.

> > +   return irq_chip_set_type_parent(d, parent_type);
> > +}
> > +static const struct irq_chip rzt2h_icu_chip = {
> > +   .name = "rzt2h-icu",
> > +   .irq_mask = irq_chip_mask_parent,
> > +   .irq_unmask = irq_chip_unmask_parent,
> > +   .irq_eoi = irq_chip_eoi_parent,
> > +   .irq_set_type = rzt2h_icu_set_type,
> > +   .irq_set_wake = irq_chip_set_wake_parent,
> > +   .irq_set_affinity = irq_chip_set_affinity_parent,
> > +   .irq_retrigger = irq_chip_retrigger_hierarchy,
> > +   .irq_get_irqchip_state = irq_chip_get_parent_state,
> > +   .irq_set_irqchip_state = irq_chip_set_parent_state,
> > +   .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SET_TYPE_MASKED |
> > +            IRQCHIP_SKIP_SET_WAKE,
>
> https://www.kernel.org/doc/html/latest%25
> 2Fprocess%2Fmaintainer-tip.html%23struct-declarations-and-initializers&data=05%7C02%7Ccosmin-
> gabriel.tanislav.xa%40renesas.com%7C377460d2de2a4899c85f08de29df9604%7C53d82571da1947e49cb4625a166a4a2a
> %7C0%7C0%7C638994237471295714%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiO
> iJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=K%2FjQlj7d2xbeyIrYLSmDoI90CNwyndz%2B0nkU
> j%2Bu6fn0%3D&reserved=0
>
> And please read and follow the rest of the documentation too.
>

Ack.

> > +};
> > +
> > +static int rzt2h_icu_alloc(struct irq_domain *domain, unsigned int virq,
> > +                      unsigned int nr_irqs, void *arg)
> > +{
> > +   struct rzt2h_icu_priv *priv = domain->host_data;
> > +   irq_hw_number_t hwirq;
> > +   unsigned int type;
> > +   int ret;
> > +
> > +   ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type);
> > +   if (ret)
> > +           return ret;
> > +
> > +   ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &rzt2h_icu_chip,
> > +                                       NULL);
>
> Get rid of the line breaks all over the place. You have 100 characters.
>

Ack.

> > +   if (ret)
> > +           return ret;
> > +
> > +   return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
> > +                                       &priv->fwspec[hwirq]);
> > +}
>
>
> > +static int rzt2h_icu_init(struct platform_device *pdev,
> > +                     struct device_node *parent)
> > +{
> > +   struct irq_domain *irq_domain, *parent_domain;
> > +   struct device_node *node = pdev->dev.of_node;
> > +   struct device *dev = &pdev->dev;
> > +   struct rzt2h_icu_priv *priv;
> > +   int ret;
> > +
> > +   parent_domain = irq_find_host(parent);
> > +   if (!parent_domain)
> > +           return dev_err_probe(dev, -ENODEV, "cannot find parent domain\n");
> > +
> > +   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +   if (!priv)
> > +           return -ENOMEM;
> > +
> > +   platform_set_drvdata(pdev, priv);
> > +
> > +   priv->base_ns = devm_of_iomap(dev, dev->of_node, 0, NULL);
> > +   if (IS_ERR(priv->base_ns))
> > +           return PTR_ERR(priv->base_ns);
> > +
> > +   priv->base_s = devm_of_iomap(dev, dev->of_node, 1, NULL);
> > +   if (IS_ERR(priv->base_s))
> > +           return PTR_ERR(priv->base_s);
> > +
> > +   ret = rzt2h_icu_parse_interrupts(priv, node);
> > +   if (ret)
> > +           return dev_err_probe(dev, ret,
> > +                                "cannot parse interrupts: %d\n", ret);
> > +
> > +   ret = devm_pm_runtime_enable(dev);
> > +   if (ret)
> > +           return dev_err_probe(dev, ret,
> > +                                "devm_pm_runtime_enable failed: %d\n", ret);
> > +
> > +   ret = pm_runtime_resume_and_get(dev);
> > +   if (ret)
> > +           return dev_err_probe(dev, ret,
> > +                                "pm_runtime_resume_and_get failed: %d\n", ret);
> > +
> > +   raw_spin_lock_init(&priv->lock);
> > +
> > +   irq_domain = irq_domain_create_hierarchy(parent_domain, 0, RZT2H_ICU_NUM_IRQ,
> > +                                            dev_fwnode(dev),
> > +                                            &rzt2h_icu_domain_ops, priv);
> > +   if (!irq_domain) {
> > +           pm_runtime_put(dev);
> > +           return -ENOMEM;
> > +   }
>
> The mix of 'return $ERR' and 'return dev_err_probe()' is confusing at best.
>

For ENOMEM, dev_err_probe() doesn't really print anything. ENOMEM is
what other drivers seem to use for a NULL irq_domain_create_hierarchy()
result.

Do you want me to use a different error code and switch to
dev_err_probe(), or keep ENOMEM and switch to dev_err_probe() for
uniformity anyway?

What should be done for the devm_kzalloc() failure case at the top of
rzt2h_icu_init()?

> Thanks,
>
>         tglx