Re: [PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Mar 18, 2021 at 1:12 PM Alexander Sverdlin
<alexander.sverdlin@xxxxxxxxx> wrote:
> On 18/03/2021 11:51, Andy Shevchenko wrote:
> > On Wednesday, March 17, 2021, Alexander A Sverdlin <alexander.sverdlin@xxxxxxxxx <mailto:alexander.sverdlin@xxxxxxxxx>> wrote:

...

> >     Existing (irq < 0) condition is always false because adev->irq has unsigned
> >     type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
> >     the mapping errors were silently ignored.
> >
> >     Seems that repairing this check would be backwards-incompatible and might
> >     break the probe() for the implementations without IRQ support. Therefore
> >     warn the user instead.

...

> >     -       if (irq < 0) {
> >     -               dev_err(&adev->dev, "invalid IRQ\n");
> >     -               return -ENODEV;
> >     -       }
> >     +       if (!irq)
> >     +               dev_warn(&adev->dev, "IRQ support disabled\n");
> >
> >
> >
> > I guess you need to preserve bailing out. Seems nobody hit this error path.
>
> Do you mean preserve "return -ENODEV;"?
> This never ever happened, because the "if" is "always false", irqs coming from irq[] cannot be
> negative.
> And there is another use-case actually: there are legal PL061 configurations without IRQs at all,
> which simply work even trying to instantiate irq chip, but as devm_gpiochip_add_data() doesn't
> fail with irq==0, this goes completely unnoticed and such a gpio bank works fine.
>
> The proper way would be not even try to instantiate any irq chip in such case.
> Let me know if I shall rework the patch this way.

Yes, please, rewrite it like
if (irq > 0) {
 ... instantiate an IRQ chip ...
} else {
 // nothing. No warning is needed (as it wasn't ever before), perhaps
just a debug message
}
--
With Best Regards,
Andy Shevchenko