This fixes a bug in the IRQ setup of the tqmx86 MFD / GPIO drivers and
adds support for our upcoming Elkhart Lake module TQMxE40M (as well as
future SoMs).
As patch 2 depends on patch 1, it would make sense to push the whole
series through the same tree.
Matthias Schiffer (3):
gpio: tqmx86: really make IRQ optional
mfd: tqmx86: clear GPIO IRQ resource when no IRQ is set
mfd: tqmx86: add support for TQMxE40M
drivers/gpio/gpio-tqmx86.c | 6 +++---
drivers/mfd/tqmx86.c | 13 ++++++-------
2 files changed, 9 insertions(+), 10 deletions(-)
--
2.17.1
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 <[email protected]>
---
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
All future TQMx86 SoMs will use a 24MHz LPC clock, so we can use that as
a default instead of listing each new module individually.
Signed-off-by: Matthias Schiffer <[email protected]>
---
drivers/mfd/tqmx86.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/mfd/tqmx86.c b/drivers/mfd/tqmx86.c
index 732013f40e4e..1d5cebc4e72b 100644
--- a/drivers/mfd/tqmx86.c
+++ b/drivers/mfd/tqmx86.c
@@ -36,6 +36,7 @@
#define TQMX86_REG_BOARD_ID_70EB 8
#define TQMX86_REG_BOARD_ID_80UC 9
#define TQMX86_REG_BOARD_ID_90UC 10
+#define TQMX86_REG_BOARD_ID_E40M 12
#define TQMX86_REG_BOARD_REV 0x21
#define TQMX86_REG_IO_EXT_INT 0x26
#define TQMX86_REG_IO_EXT_INT_NONE 0
@@ -130,6 +131,8 @@ static const char *tqmx86_board_id_to_name(u8 board_id)
return "TQMx80UC";
case TQMX86_REG_BOARD_ID_90UC:
return "TQMx90UC";
+ case TQMX86_REG_BOARD_ID_E40M:
+ return "TQMxE40M";
default:
return "Unknown";
}
@@ -138,12 +141,6 @@ static const char *tqmx86_board_id_to_name(u8 board_id)
static int tqmx86_board_id_to_clk_rate(u8 board_id)
{
switch (board_id) {
- case TQMX86_REG_BOARD_ID_50UC:
- case TQMX86_REG_BOARD_ID_60EB:
- case TQMX86_REG_BOARD_ID_70EB:
- case TQMX86_REG_BOARD_ID_80UC:
- case TQMX86_REG_BOARD_ID_90UC:
- return 24000;
case TQMX86_REG_BOARD_ID_E39M:
case TQMX86_REG_BOARD_ID_E39C:
case TQMX86_REG_BOARD_ID_E39x:
@@ -152,7 +149,7 @@ static int tqmx86_board_id_to_clk_rate(u8 board_id)
case TQMX86_REG_BOARD_ID_E38C:
return 33000;
default:
- return 0;
+ return 24000;
}
}
--
2.17.1
The driver was registering IRQ 0 when no IRQ was set. This leads to
warnings with newer kernels.
Clear the resource flags, so no resource is registered at all in this
case.
Signed-off-by: Matthias Schiffer <[email protected]>
---
drivers/mfd/tqmx86.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/mfd/tqmx86.c b/drivers/mfd/tqmx86.c
index ddddf08b6a4c..732013f40e4e 100644
--- a/drivers/mfd/tqmx86.c
+++ b/drivers/mfd/tqmx86.c
@@ -209,6 +209,8 @@ static int tqmx86_probe(struct platform_device *pdev)
/* Assumes the IRQ resource is first. */
tqmx_gpio_resources[0].start = gpio_irq;
+ } else {
+ tqmx_gpio_resources[0].flags = 0;
}
ocores_platfom_data.clock_khz = tqmx86_board_id_to_clk_rate(board_id);
--
2.17.1
On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer
<[email protected]> 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
On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer
<[email protected]> 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
On Wed, 2021-03-31 at 15:29 +0300, Andy Shevchenko wrote:
> On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer
> <[email protected]> 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;
>
>
On Wed, Mar 31, 2021 at 2:39 PM Matthias Schiffer
<[email protected]> wrote:
>
> The driver was registering IRQ 0 when no IRQ was set. This leads to
> warnings with newer kernels.
>
> Clear the resource flags, so no resource is registered at all in this
> case.
...
> /* Assumes the IRQ resource is first. */
> tqmx_gpio_resources[0].start = gpio_irq;
> + } else {
> + tqmx_gpio_resources[0].flags = 0;
Please set IORESOURCE_DISABLED flag in the initial structure instead.
> }
--
With Best Regards,
Andy Shevchenko
On Wed, Mar 31, 2021 at 2:38 PM Matthias Schiffer
<[email protected]> wrote:
>
> All future TQMx86 SoMs will use a 24MHz LPC clock, so we can use that as
> a default instead of listing each new module individually.
...
> case TQMX86_REG_BOARD_ID_90UC:
> return "TQMx90UC";
> + case TQMX86_REG_BOARD_ID_E40M:
> + return "TQMxE40M";
> default:
> return "Unknown";
> }
> @@ -138,12 +141,6 @@ static const char *tqmx86_board_id_to_name(u8 board_id)
> static int tqmx86_board_id_to_clk_rate(u8 board_id)
> {
> switch (board_id) {
> - case TQMX86_REG_BOARD_ID_50UC:
> - case TQMX86_REG_BOARD_ID_60EB:
> - case TQMX86_REG_BOARD_ID_70EB:
> - case TQMX86_REG_BOARD_ID_80UC:
> - case TQMX86_REG_BOARD_ID_90UC:
> - return 24000;
> case TQMX86_REG_BOARD_ID_E39M:
> case TQMX86_REG_BOARD_ID_E39C:
> case TQMX86_REG_BOARD_ID_E39x:
> @@ -152,7 +149,7 @@ static int tqmx86_board_id_to_clk_rate(u8 board_id)
> case TQMX86_REG_BOARD_ID_E38C:
> return 33000;
> default:
> - return 0;
> + return 24000;
AFAICS it will return 24 MHz for "Unknown" board. Is it okay to be so brave?
--
With Best Regards,
Andy Shevchenko
On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer
<[email protected]> wrote:
> On Wed, 2021-03-31 at 15:29 +0300, Andy Shevchenko wrote:
> > On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer
> > <[email protected]> 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
On Wed, 2021-03-31 at 15:35 +0300, Andy Shevchenko wrote:
> On Wed, Mar 31, 2021 at 2:39 PM Matthias Schiffer
> <[email protected]> wrote:
> >
> > The driver was registering IRQ 0 when no IRQ was set. This leads to
> > warnings with newer kernels.
> >
> > Clear the resource flags, so no resource is registered at all in this
> > case.
>
> ...
>
> > /* Assumes the IRQ resource is first. */
> > tqmx_gpio_resources[0].start = gpio_irq;
> > + } else {
> > + tqmx_gpio_resources[0].flags = 0;
>
> Please set IORESOURCE_DISABLED flag in the initial structure instead.
Is there any documentation for the correct usage of this flag? I think
I tried IORESOURCE_DISABLED originally, but it didn't have any effect
(platform_get_irq() ignored the flag and returned the resource
anyways). I might misremember though, I originally wrote the series
some time ago.
>
> > }
>
>
On Wed, 2021-03-31 at 15:37 +0300, Andy Shevchenko wrote:
> On Wed, Mar 31, 2021 at 2:38 PM Matthias Schiffer
> <[email protected]> wrote:
> >
> > All future TQMx86 SoMs will use a 24MHz LPC clock, so we can use that as
> > a default instead of listing each new module individually.
>
> ...
>
> > case TQMX86_REG_BOARD_ID_90UC:
> > return "TQMx90UC";
> > + case TQMX86_REG_BOARD_ID_E40M:
> > + return "TQMxE40M";
> > default:
> > return "Unknown";
> > }
> > @@ -138,12 +141,6 @@ static const char *tqmx86_board_id_to_name(u8 board_id)
> > static int tqmx86_board_id_to_clk_rate(u8 board_id)
> > {
> > switch (board_id) {
> > - case TQMX86_REG_BOARD_ID_50UC:
> > - case TQMX86_REG_BOARD_ID_60EB:
> > - case TQMX86_REG_BOARD_ID_70EB:
> > - case TQMX86_REG_BOARD_ID_80UC:
> > - case TQMX86_REG_BOARD_ID_90UC:
> > - return 24000;
> > case TQMX86_REG_BOARD_ID_E39M:
> > case TQMX86_REG_BOARD_ID_E39C:
> > case TQMX86_REG_BOARD_ID_E39x:
> > @@ -152,7 +149,7 @@ static int tqmx86_board_id_to_clk_rate(u8 board_id)
> > case TQMX86_REG_BOARD_ID_E38C:
> > return 33000;
> > default:
> > - return 0;
> > + return 24000;
>
> AFAICS it will return 24 MHz for "Unknown" board. Is it okay to be so brave?
As noted in the commit message, our hardware developers intend to use
24 MHz for all future x86 SoMs.
On Wed, 2021-03-31 at 15:39 +0300, Andy Shevchenko wrote:
> On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer
> <[email protected]> wrote:
> > On Wed, 2021-03-31 at 15:29 +0300, Andy Shevchenko wrote:
> > > On Wed, Mar 31, 2021 at 2:37 PM Matthias Schiffer
> > > <[email protected]> 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.
On Wed, Mar 31, 2021 at 4:36 PM Matthias Schiffer
<[email protected]> wrote:
>
> On Wed, 2021-03-31 at 15:39 +0300, Andy Shevchenko wrote:
> > On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer
> > <[email protected]> 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
On Wed, Mar 31, 2021 at 4:24 PM Matthias Schiffer
<[email protected]> wrote:
> On Wed, 2021-03-31 at 15:35 +0300, Andy Shevchenko wrote:
> > On Wed, Mar 31, 2021 at 2:39 PM Matthias Schiffer
> > <[email protected]> wrote:
...
> > > + tqmx_gpio_resources[0].flags = 0;
> >
> > Please set IORESOURCE_DISABLED flag in the initial structure instead.
>
> Is there any documentation for the correct usage of this flag? I think
> I tried IORESOURCE_DISABLED originally, but it didn't have any effect
> (platform_get_irq() ignored the flag and returned the resource
> anyways). I might misremember though, I originally wrote the series
> some time ago.
It seems this flag is used solely for ACPI / PNP resources...
Hmm... I dunno how platorm_get_irq*() should respect this flag. You
see, that in the ACPI branch it has been checked there.
--
With Best Regards,
Andy Shevchenko
On Wed, Mar 31, 2021 at 4:33 PM Matthias Schiffer
<[email protected]> wrote:
> On Wed, 2021-03-31 at 15:37 +0300, Andy Shevchenko wrote:
> > On Wed, Mar 31, 2021 at 2:38 PM Matthias Schiffer
> > <[email protected]> wrote:
...
> > > + return 24000;
> >
> > AFAICS it will return 24 MHz for "Unknown" board. Is it okay to be so brave?
>
> As noted in the commit message, our hardware developers intend to use
> 24 MHz for all future x86 SoMs.
What may go wrong in the future?.. (rhetorical question, obviously)
--
With Best Regards,
Andy Shevchenko
On Wed, 31 Mar 2021, Andy Shevchenko wrote:
> On Wed, Mar 31, 2021 at 4:33 PM Matthias Schiffer
> <[email protected]> wrote:
> > On Wed, 2021-03-31 at 15:37 +0300, Andy Shevchenko wrote:
> > > On Wed, Mar 31, 2021 at 2:38 PM Matthias Schiffer
> > > <[email protected]> wrote:
>
> ...
>
> > > > + return 24000;
> > >
> > > AFAICS it will return 24 MHz for "Unknown" board. Is it okay to be so brave?
> >
> > As noted in the commit message, our hardware developers intend to use
> > 24 MHz for all future x86 SoMs.
>
> What may go wrong in the future?.. (rhetorical question, obviously)
My preference would be to be explicit.
Rather than support boards implicitly i.e. by accident.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, 2021-04-01 at 09:04 +0100, Lee Jones wrote:
> On Wed, 31 Mar 2021, Andy Shevchenko wrote:
>
> > On Wed, Mar 31, 2021 at 4:33 PM Matthias Schiffer
> > <[email protected]> wrote:
> > > On Wed, 2021-03-31 at 15:37 +0300, Andy Shevchenko wrote:
> > > > On Wed, Mar 31, 2021 at 2:38 PM Matthias Schiffer
> > > > <[email protected]> wrote:
> >
> > ...
> >
> > > > > + return 24000;
> > > >
> > > > AFAICS it will return 24 MHz for "Unknown" board. Is it okay to be so brave?
> > >
> > > As noted in the commit message, our hardware developers intend to use
> > > 24 MHz for all future x86 SoMs.
> >
> > What may go wrong in the future?.. (rhetorical question, obviously)
>
> My preference would be to be explicit.
>
> Rather than support boards implicitly i.e. by accident.
>
How about logging a warning for unknown boards, but still returning
24 MHz?
On Wed, 2021-03-31 at 17:03 +0300, Andy Shevchenko wrote:
> On Wed, Mar 31, 2021 at 4:36 PM Matthias Schiffer
> <[email protected]> wrote:
> >
> > On Wed, 2021-03-31 at 15:39 +0300, Andy Shevchenko wrote:
> > > On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer
> > > <[email protected]> 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
<[email protected]> wrote:
> On Wed, 2021-03-31 at 17:03 +0300, Andy Shevchenko wrote:
> > On Wed, Mar 31, 2021 at 4:36 PM Matthias Schiffer
> > <[email protected]> wrote:
> > > On Wed, 2021-03-31 at 15:39 +0300, Andy Shevchenko wrote:
> > > > On Wed, Mar 31, 2021 at 3:37 PM Matthias Schiffer
> > > > <[email protected]> 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