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

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Nov 24 2025 at 12:50, Cosmin-Gabriel Tanislav wrote:
>> -----Original Message-----
>> From: Thomas Gleixner <tglx@linutronix.de>
>> Sent: Saturday, November 22, 2025 5:56 PM
>> To: Cosmin-Gabriel Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>; Rob Herring <robh@kernel.org>;
>> Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Geert Uytterhoeven
>> <geert+renesas@glider.be>; magnus.damm <magnus.damm@gmail.com>; Cosmin-Gabriel Tanislav <cosmin-
>> gabriel.tanislav.xa@renesas.com>
>> Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-renesas-soc@vger.kernel.org
>> Subject: Re: [PATCH 2/4] irqchip: add RZ/{T2H,N2H} Interrupt Controller (ICU) driver

Can you please fix your mail-client not to copy the whole header into
the reply?

>> 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;
>         }

Yes. Way better.

> 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?

Usually single line, but in this case it might be more readable. Up to you.

>> > +   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.

That's what I was missing. Now it makes sense.

Thanks,

        tglx