| Message ID | 11a8323c249ae6ea7584402ab0fb74551b6a4b7d.1617189926.git.matthias.schiffer@ew.tq-group.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tqmx86: TQMxE40M support | expand |
On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> 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;
On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > On Wed, 2021-03-31 at 15:29 +0300, Andy Shevchenko wrote: > > On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer > > <matthias.schiffer@ew.tq-group.com> 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.
On Wed, Mar 31, 2021 at 4:36 PM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > > On Wed, 2021-03-31 at 15:39 +0300, Andy Shevchenko wrote: > > On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer > > <matthias.schiffer@ew.tq-group.com> 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.
On Wed, 2021-03-31 at 17:03 +0300, Andy Shevchenko wrote: > On Wed, Mar 31, 2021 at 4:36 PM Matthias Schiffer > <matthias.schiffer@ew.tq-group.com> wrote: > > > > On Wed, 2021-03-31 at 15:39 +0300, Andy Shevchenko wrote: > > > On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer > > > <matthias.schiffer@ew.tq-group.com> 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. > Hi Andy, what's the plan here? "driver core: platform: Make platform_get_irq_optional() optional" had to be reverted because it broke existing users of platform_get_irq_optional(). I'm not convinced that a slightly more convenient API is worth going through the trouble of fixing them all - I know we don't care much about out-of-tree modules, but subtly changing the behaviour of such a function doesn't seem like a good idea to me even if we review all in-tree users. Should I just rebase my patches with the existing ENXIO handing (and fix up the other issues that were noted), or do you intend to give the platform_get_irq_optional() revamp another try? Kind regards, Matthias
On Thu, Jun 24, 2021 at 4:44 PM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > On Wed, 2021-03-31 at 17:03 +0300, Andy Shevchenko wrote: > > On Wed, Mar 31, 2021 at 4:36 PM Matthias Schiffer > > <matthias.schiffer@ew.tq-group.com> wrote: > > > On Wed, 2021-03-31 at 15:39 +0300, Andy Shevchenko wrote: > > > > On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer > > > > <matthias.schiffer@ew.tq-group.com> 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. > > what's the plan here? "driver core: platform: Make > platform_get_irq_optional() optional" had to be reverted because it > broke existing users of platform_get_irq_optional(). I'm not convinced > that a slightly more convenient API is worth going through the trouble > of fixing them all - I know we don't care much about out-of-tree > modules, but subtly changing the behaviour of such a function doesn't > seem like a good idea to me even if we review all in-tree users. Why? The problem with this function is either naming or semantics. It should be fixed and that will require revisiting all current users anyway. > Should I just rebase my patches with the existing ENXIO handing (and > fix up the other issues that were noted), or do you intend to give the > platform_get_irq_optional() revamp another try? I do intend to give another try, but if you want to be independent of that, just make sure that in any new / revisited user of platform_get_irq_optional() the 0 is taken into consideration as an optional case. -- With Best Regards, Andy Shevchenko
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;
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 <matthias.schiffer@ew.tq-group.com> --- drivers/gpio/gpio-tqmx86.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)