Re: [PATCH v3 5/6] irqchip/renesas-rzv2h: Add CA55 software interrupt support

[Lad, Prabhakar]

Hi Thomas,

Thank you for the review.

On Tue, Feb 24, 2026 at 7:34 AM Thomas Gleixner <tglx@xxxxxxxxxx> wrote:
>
> On Mon, Feb 09 2026 at 10:41, Prabhakar wrote:
> >  static int rzv2h_icu_set_type(struct irq_data *d, unsigned int type)
> >  {
> > +     unsigned int gic_type = IRQ_TYPE_LEVEL_HIGH;
> >       unsigned int hw_irq = irqd_to_hwirq(d);
> >       int ret;
> >
> > @@ -445,6 +475,11 @@ static int rzv2h_icu_set_type(struct irq_data *d, unsigned int type)
> >               /* TINT */
> >               ret = rzv2h_tint_set_type(d, type);
> >               break;
> > +     case ICU_CA55_INT_START ... ICU_CA55_INT_LAST:
> > +             /* CA55 Software Interrupts have EDGE_RISING type */
> > +             gic_type = IRQ_TYPE_EDGE_RISING;
>
> So this unconditionally selects EDGE_RISING independent of the type
> provided by the caller. Interesting choice and compatible with the rest
> of the code - _not_.
>
Ok, I will update it to use the type which has been passed.

> > +
> > +static int rzv2h_icu_setup_irqs(struct platform_device *pdev,
> > +                             struct irq_domain *irq_domain)
>
> I told you before that you have 100 characters. Get rid of these line breaks.
>
Ok.

> > +{
> > +     bool irq_inject = IS_ENABLED(CONFIG_GENERIC_IRQ_INJECTION);
> > +     static const char * const rzv2h_swint_names[] = {
> > +             "int-ca55-0", "int-ca55-1",
> > +             "int-ca55-2", "int-ca55-3",
> > +     };
> > +     static const u8 swint_idx[] = { 0, 1, 2, 3 };
> > +     struct device *dev = &pdev->dev;
> > +     struct irq_fwspec fwspec;
> > +     unsigned int virq;
> > +     unsigned int i;
>
> Coalesce same types into a single line. See Documentation/....
>
Ok.

> > +     int ret;
> > +
> > +     for (i = 0; i < ICU_CA55_INT_COUNT && irq_inject; i++) {
> > +             fwspec.fwnode = irq_domain->fwnode;
> > +             fwspec.param_count = 2;
> > +             fwspec.param[0] = ICU_CA55_INT_START + i;
> > +             fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> > +
> > +             virq = irq_create_fwspec_mapping(&fwspec);
> > +             if (!virq)
> > +                     return dev_err_probe(dev, -EINVAL, "failed to create IRQ mapping for %s\n",
> > +                                          rzv2h_swint_names[i]);
>
> This lacks curly brackets on the if(). See Documentation/.....
>
Ok.

Cheers,
Prabhakar