Re: [PATCH 1/3] gpio: tqmx86: really make IRQ optional
On Wed, Mar 31, 2021 at 4:36 PM Matthias Schiffer wrote: > > On Wed, 2021-03-31 at 15:39 +0300, Andy Shevchenko wrote: > > On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer > > wrote: > > > On Wed, 2021-03-31 at 15:29 +0300, Andy Shevchenko wrote: ... > > > I don't understand which part of the code is dead now. I assume the > > > `return irq` case is still useful for unexpected errors, or things like > > > EPROBE_DEFER? I'm not sure if EPROBE_DEFER is relevant for this driver, > > > but just ignoring the error code completely doesn't seem right to me. > > > > platform_get_irq() AFAIK won't ever return such a code. > > So, basically your conditional is always false. > > > > I would like to see the code path which makes my comment wrong. > > > > EPROBE_DEFER appears a few times in platform_get_irq_optional() > (drivers/base/platform.c), but it's possible that this is only relevant > for OF-based platforms and not x86. Ah, okay, that's something I haven't paid attention to. So the root cause of the your case is platform_get_irq_optional|() return code. I'm wondering why it can't return 0 instead of absent IRQ? Perhaps you need to fix it instead of lurking into each caller. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/3] gpio: tqmx86: really make IRQ optional
On Wed, 2021-03-31 at 15:39 +0300, Andy Shevchenko wrote: > On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer > wrote: > > On Wed, 2021-03-31 at 15:29 +0300, Andy Shevchenko wrote: > > > On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer > > > wrote: > > ... > > > > > - irq = platform_get_irq(pdev, 0); > > > > - if (irq < 0) > > > > + irq = platform_get_irq_optional(pdev, 0); > > > > + if (irq < 0 && irq != -ENXIO) > > > > return irq; > > > > > > This is a dead code now. I suggest you to do the opposite, i.e. > > > if (irq < 0) > > > irq = 0; > > > > I don't understand which part of the code is dead now. I assume the > > `return irq` case is still useful for unexpected errors, or things like > > EPROBE_DEFER? I'm not sure if EPROBE_DEFER is relevant for this driver, > > but just ignoring the error code completely doesn't seem right to me. > > platform_get_irq() AFAIK won't ever return such a code. > So, basically your conditional is always false. > > I would like to see the code path which makes my comment wrong. > EPROBE_DEFER appears a few times in platform_get_irq_optional() (drivers/base/platform.c), but it's possible that this is only relevant for OF-based platforms and not x86.
Re: (EXT) Re: [PATCH 1/3] gpio: tqmx86: really make IRQ optional
On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer wrote: > On Wed, 2021-03-31 at 15:29 +0300, Andy Shevchenko wrote: > > On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer > > wrote: ... > > > - irq = platform_get_irq(pdev, 0); > > > - if (irq < 0) > > > + irq = platform_get_irq_optional(pdev, 0); > > > + if (irq < 0 && irq != -ENXIO) > > > return irq; > > > > This is a dead code now. I suggest you to do the opposite, i.e. > > if (irq < 0) > > irq = 0; > > I don't understand which part of the code is dead now. I assume the > `return irq` case is still useful for unexpected errors, or things like > EPROBE_DEFER? I'm not sure if EPROBE_DEFER is relevant for this driver, > but just ignoring the error code completely doesn't seem right to me. platform_get_irq() AFAIK won't ever return such a code. So, basically your conditional is always false. I would like to see the code path which makes my comment wrong. -- With Best Regards, Andy Shevchenko
Re: (EXT) Re: [PATCH 1/3] gpio: tqmx86: really make IRQ optional
On Wed, 2021-03-31 at 15:29 +0300, Andy Shevchenko wrote:
> On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer
> wrote:
> >
> > The tqmx86 MFD driver was passing IRQ 0 for "no IRQ" in the past. This
> > causes warnings with newer kernels.
> >
> > Prepare the gpio-tqmx86 driver for the fixed MFD driver by handling a
> > missing IRQ properly.
>
> ...
>
> > - irq = platform_get_irq(pdev, 0);
> > - if (irq < 0)
> > + irq = platform_get_irq_optional(pdev, 0);
> > + if (irq < 0 && irq != -ENXIO)
> > return irq;
>
> This is a dead code now. I suggest you to do the opposite, i.e.
> if (irq < 0)
> irq = 0;
I don't understand which part of the code is dead now. I assume the
`return irq` case is still useful for unexpected errors, or things like
EPROBE_DEFER? I'm not sure if EPROBE_DEFER is relevant for this driver,
but just ignoring the error code completely doesn't seem right to me.
>
> In such a case below change is not required.
>
> ...
>
> > - if (irq) {
> > + if (irq > 0) {
> > struct irq_chip *irq_chip = &gpio->irq_chip;
> > u8 irq_status;
>
>
Re: [PATCH 1/3] gpio: tqmx86: really make IRQ optional
On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer wrote: > > The tqmx86 MFD driver was passing IRQ 0 for "no IRQ" in the past. This > causes warnings with newer kernels. > > Prepare the gpio-tqmx86 driver for the fixed MFD driver by handling a > missing IRQ properly. Also you missed a Fixes tag. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/3] gpio: tqmx86: really make IRQ optional
On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer
wrote:
>
> The tqmx86 MFD driver was passing IRQ 0 for "no IRQ" in the past. This
> causes warnings with newer kernels.
>
> Prepare the gpio-tqmx86 driver for the fixed MFD driver by handling a
> missing IRQ properly.
...
> - irq = platform_get_irq(pdev, 0);
> - if (irq < 0)
> + irq = platform_get_irq_optional(pdev, 0);
> + if (irq < 0 && irq != -ENXIO)
> return irq;
This is a dead code now. I suggest you to do the opposite, i.e.
if (irq < 0)
irq = 0;
In such a case below change is not required.
...
> - if (irq) {
> + if (irq > 0) {
> struct irq_chip *irq_chip = &gpio->irq_chip;
> u8 irq_status;
--
With Best Regards,
Andy Shevchenko
[PATCH 1/3] gpio: tqmx86: really make IRQ optional
The tqmx86 MFD driver was passing IRQ 0 for "no IRQ" in the past. This
causes warnings with newer kernels.
Prepare the gpio-tqmx86 driver for the fixed MFD driver by handling a
missing IRQ properly.
Signed-off-by: Matthias Schiffer
---
drivers/gpio/gpio-tqmx86.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 5022e0ad0fae..0f5d17f343f1 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -238,8 +238,8 @@ static int tqmx86_gpio_probe(struct platform_device *pdev)
struct resource *res;
int ret, irq;
- irq = platform_get_irq(pdev, 0);
- if (irq < 0)
+ irq = platform_get_irq_optional(pdev, 0);
+ if (irq < 0 && irq != -ENXIO)
return irq;
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
@@ -278,7 +278,7 @@ static int tqmx86_gpio_probe(struct platform_device *pdev)
pm_runtime_enable(&pdev->dev);
- if (irq) {
+ if (irq > 0) {
struct irq_chip *irq_chip = &gpio->irq_chip;
u8 irq_status;
--
2.17.1
