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

[Alexander Sverdlin <alexander.sverdlin@nokia.com>]

Hello Andy,

On 18/03/2021 11:51, Andy Shevchenko wrote:
> 
> 
> On Wednesday, March 17, 2021, Alexander A Sverdlin <alexander.sverdlin@nokia.com <mailto:alexander.sverdlin@nokia.com>> wrote:
> 
>     From: Alexander Sverdlin <alexander.sverdlin@nokia.com <mailto:alexander.sverdlin@nokia.com>>
> 
>     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.
> 
>     Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com <mailto:alexander.sverdlin@nokia.com>>
>     ---
>      drivers/gpio/gpio-pl061.c | 6 ++----
>      1 file changed, 2 insertions(+), 4 deletions(-)
> 
>     diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
>     index 5df7782..3439120 100644
>     --- a/drivers/gpio/gpio-pl061.c
>     +++ b/drivers/gpio/gpio-pl061.c
>     @@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
> 
>             writeb(0, pl061->base + GPIOIE); /* disable irqs */
>             irq = adev->irq[0];
>     -       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.

-- 
Best regards,
Alexander Sverdlin.