linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] driver: platform: Add support for GpioInt() ACPI to platform_get_irq()
@ 2019-02-07 18:59 egranata
  2019-02-07 19:02 ` Rafael J. Wysocki
  2019-02-11 19:01 ` [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq() egranata
  0 siblings, 2 replies; 21+ messages in thread
From: egranata @ 2019-02-07 18:59 UTC (permalink / raw)
  To: gregkh, rafael, enric.balletbo
  Cc: linux-kernel, gwendal, dtor, linux-acpi, briannorris, Enrico Granata

From: Enrico Granata <egranata@chromium.org>

ACPI 5 added support for GpioInt resources as a way to provide
information about interrupts mediated via a GPIO controller.

Several device buses (e.g. SPI, I2C) have support for retrieving
an IRQ specified via this type of resource, and providing it
directly to the driver as an IRQ number.
This is not currently done for the platform drivers, as platform_get_irq()
does not try to parse GpioInt() resources.

This commit adds that functionality.

Signed-off-by: Enrico Granata <egranata@chromium.org>
---
 drivers/base/platform.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 1c958eb33ef4d..c50c4f9033aef 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -127,7 +127,17 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
 	}
 
-	return r ? r->start : -ENXIO;
+	if (r)
+		return r->start;
+
+	/*
+	 * If no IRQ was found, try to parse ACPI GpioInt resources
+	 * as a last resort.
+	 */
+	if (has_acpi_companion(&dev->dev))
+		return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
+
+	return -ENXIO;
 #endif
 }
 EXPORT_SYMBOL_GPL(platform_get_irq);
-- 
2.20.1.611.gfbb209baf1-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH] driver: platform: Add support for GpioInt() ACPI to platform_get_irq()
  2019-02-07 18:59 [PATCH] driver: platform: Add support for GpioInt() ACPI to platform_get_irq() egranata
@ 2019-02-07 19:02 ` Rafael J. Wysocki
  2019-02-07 19:39   ` Andy Shevchenko
  2019-02-11 19:01 ` [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq() egranata
  1 sibling, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2019-02-07 19:02 UTC (permalink / raw)
  To: egranata
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Enric Balletbo i Serra,
	Linux Kernel Mailing List, gwendal, Dmitry Torokhov,
	ACPI Devel Maling List, Brian Norris, Enrico Granata,
	Mika Westerberg, Andy Shevchenko, Hans de Goede

+Mika Westerberg
+Andy Shevchenko
+Hans de Goede

On Thu, Feb 7, 2019 at 7:59 PM <egranata@google.com> wrote:
>
> From: Enrico Granata <egranata@chromium.org>
>
> ACPI 5 added support for GpioInt resources as a way to provide
> information about interrupts mediated via a GPIO controller.
>
> Several device buses (e.g. SPI, I2C) have support for retrieving
> an IRQ specified via this type of resource, and providing it
> directly to the driver as an IRQ number.
> This is not currently done for the platform drivers, as platform_get_irq()
> does not try to parse GpioInt() resources.
>
> This commit adds that functionality.
>
> Signed-off-by: Enrico Granata <egranata@chromium.org>
> ---
>  drivers/base/platform.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1c958eb33ef4d..c50c4f9033aef 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -127,7 +127,17 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>                 irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
>         }
>
> -       return r ? r->start : -ENXIO;
> +       if (r)
> +               return r->start;
> +
> +       /*
> +        * If no IRQ was found, try to parse ACPI GpioInt resources
> +        * as a last resort.
> +        */
> +       if (has_acpi_companion(&dev->dev))
> +               return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> +
> +       return -ENXIO;
>  #endif
>  }
>  EXPORT_SYMBOL_GPL(platform_get_irq);
> --
> 2.20.1.611.gfbb209baf1-goog
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] driver: platform: Add support for GpioInt() ACPI to platform_get_irq()
  2019-02-07 19:02 ` Rafael J. Wysocki
@ 2019-02-07 19:39   ` Andy Shevchenko
  2019-02-07 19:45     ` Andy Shevchenko
                       ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Andy Shevchenko @ 2019-02-07 19:39 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: egranata, Greg Kroah-Hartman, Enric Balletbo i Serra,
	Linux Kernel Mailing List, Gwendal Grignou, Dmitry Torokhov,
	ACPI Devel Maling List, Brian Norris, Enrico Granata,
	Mika Westerberg, Andy Shevchenko, Hans de Goede

On Thu, Feb 7, 2019 at 9:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> +Mika Westerberg
> +Andy Shevchenko
> +Hans de Goede

Thanks, Rafael.
My comments below.

> On Thu, Feb 7, 2019 at 7:59 PM <egranata@google.com> wrote:
> >
> > From: Enrico Granata <egranata@chromium.org>
> >
> > ACPI 5 added support for GpioInt resources as a way to provide
> > information about interrupts mediated via a GPIO controller.
> >
> > Several device buses (e.g. SPI, I2C) have support for retrieving
> > an IRQ specified via this type of resource, and providing it
> > directly to the driver as an IRQ number.

> > This is not currently done for the platform drivers, as platform_get_irq()
> > does not try to parse GpioInt() resources.

And why is this a problem?

> > This commit adds that functionality.

How that can override the configuration / BIOS flavour when driver
needs to register an interrupt out of GpioIo() resource?

P.S. Have you looked at drivers/platform/x86/i2c-multi-instantiate.c
and intel_cht_int33fe.c?
They are the only drivers which needs something like this for now and
I would rather say it's a bad BIOS decision to write a table like
that.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] driver: platform: Add support for GpioInt() ACPI to platform_get_irq()
  2019-02-07 19:39   ` Andy Shevchenko
@ 2019-02-07 19:45     ` Andy Shevchenko
  2019-02-07 19:55     ` Enrico Granata
       [not found]     ` <CAPR809vnpuh8nOjU3QMCh6YJUKmtX92+bnHSUKGiAXAp6NwCHQ@mail.gmail.com>
  2 siblings, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2019-02-07 19:45 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: egranata, Greg Kroah-Hartman, Enric Balletbo i Serra,
	Linux Kernel Mailing List, Gwendal Grignou, Dmitry Torokhov,
	ACPI Devel Maling List, Brian Norris, Enrico Granata,
	Mika Westerberg, Andy Shevchenko, Hans de Goede

On Thu, Feb 7, 2019 at 9:39 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Feb 7, 2019 at 9:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote:

> > > This commit adds that functionality.
>
> How that can override the configuration / BIOS flavour when driver
> needs to register an interrupt out of GpioIo() resource?

One more missed example,

Two variants of the IRQ resources in the table for the _same_ device
but on different platforms.

--Variant1--
Device(XYZ)
_CRS1() {
 Interrupt()
 GpioInt()
 Interrupt()
}

--Variant2--
Device(XYZ)
_CRS2() {
 GpioInt()
 Interrupt()
 Interrupt()
}

How to always get second resource?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] driver: platform: Add support for GpioInt() ACPI to platform_get_irq()
  2019-02-07 19:39   ` Andy Shevchenko
  2019-02-07 19:45     ` Andy Shevchenko
@ 2019-02-07 19:55     ` Enrico Granata
       [not found]     ` <CAPR809vnpuh8nOjU3QMCh6YJUKmtX92+bnHSUKGiAXAp6NwCHQ@mail.gmail.com>
  2 siblings, 0 replies; 21+ messages in thread
From: Enrico Granata @ 2019-02-07 19:55 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Enric Balletbo i Serra,
	Linux Kernel Mailing List, Gwendal Grignou, Dmitry Torokhov,
	ACPI Devel Maling List, Brian Norris, Enrico Granata,
	Mika Westerberg, Andy Shevchenko, Hans de Goede

Resending as plain text; comments inlined.

On Thu, Feb 7, 2019 at 11:39 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Feb 7, 2019 at 9:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > +Mika Westerberg
> > +Andy Shevchenko
> > +Hans de Goede
>
> Thanks, Rafael.
> My comments below.
>
> > On Thu, Feb 7, 2019 at 7:59 PM <egranata@google.com> wrote:
> > >
> > > From: Enrico Granata <egranata@chromium.org>
> > >
> > > ACPI 5 added support for GpioInt resources as a way to provide
> > > information about interrupts mediated via a GPIO controller.
> > >
> > > Several device buses (e.g. SPI, I2C) have support for retrieving
> > > an IRQ specified via this type of resource, and providing it
> > > directly to the driver as an IRQ number.
>
> > > This is not currently done for the platform drivers, as platform_get_irq()
> > > does not try to parse GpioInt() resources.
>
> And why is this a problem?

In ChromeOS, we have a driver (cros_ec_lpc) which can run either on
systems that directly expose the interrupt,
or systems where the interrupt goes through a GPIO controller. On the
former, firmware provides an Interrupt resource
and platform_get_irq() finds it. On the latter, firmware provides a
GpioInt resource and platform_get_irq does not
find it. We could work around this in the driver by probing both
paths, but since other subsystems seem to directly
look for GpioInt resources, it seemed to us to make more sense to
extend platform_get_irq() instead.

> > > This commit adds that functionality.
>
> How that can override the configuration / BIOS flavour when driver
> needs to register an interrupt out of GpioIo() resource?
>
> P.S. Have you looked at drivers/platform/x86/i2c-multi-instantiate.c
> and intel_cht_int33fe.c?

Those drivers seem to only ever expect a GpioInt(). Our case is one
where both are possible,
depending on the system we're running on. We are trying not to put
ad-hoc logic in the driver,
but if you think that doing so would be our best course of action,
please do let us know.

> They are the only drivers which needs something like this for now and
> I would rather say it's a bad BIOS decision to write a table like
> that.

Do you have any suggestions to write proper firmware tables to avoid this issue?

>
> --
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] driver: platform: Add support for GpioInt() ACPI to platform_get_irq()
       [not found]     ` <CAPR809vnpuh8nOjU3QMCh6YJUKmtX92+bnHSUKGiAXAp6NwCHQ@mail.gmail.com>
@ 2019-02-07 20:18       ` Andy Shevchenko
  2019-02-07 20:29         ` Dmitry Torokhov
  0 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2019-02-07 20:18 UTC (permalink / raw)
  To: Enrico Granata
  Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Enric Balletbo i Serra,
	Linux Kernel Mailing List, Gwendal Grignou, Dmitry Torokhov,
	ACPI Devel Maling List, Brian Norris, Enrico Granata,
	Mika Westerberg, Andy Shevchenko, Hans de Goede

On Thu, Feb 7, 2019 at 9:45 PM Enrico Granata <egranata@google.com> wrote:
> On Thu, Feb 7, 2019 at 11:39 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Thu, Feb 7, 2019 at 9:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote:

>> > > This is not currently done for the platform drivers, as platform_get_irq()
>> > > does not try to parse GpioInt() resources.
>>
>> And why is this a problem?

> In ChromeOS, we have a driver (cros_ec_lpc) which can run either on systems that directly expose the interrupt,
> or systems where the interrupt goes through a GPIO controller. On the former, firmware provides an Interrupt resource
> and platform_get_irq() finds it. On the latter, firmware provides a GpioInt resource and platform_get_irq does not
> find it. We could work around this in the driver by probing both paths, but since other subsystems seem to directly
> look for GpioInt resources, it seemed to us to make more sense to extend platform_get_irq() instead.

Looking briefly into the driver I found third scenario — no resource at all.
So, you already have a quirk for that. Now it's the question either
you go for global quirk (trying to find an IRQ via iterating over
GpioInt() resources like in this patch, but in the driver), or use DMI
table for more stricter rules.

Either way you choose, I don't see a necessity to put this to the
driver core for now since it would be the only (let's assume properly
written ACPI tables) driver needs such.

> Do you have a suggestion as to how to write ACPI tables to avoid the issue?

1. Allocate new ID and use it (perhaps not the best path).
2. Use GPE(s).

--
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] driver: platform: Add support for GpioInt() ACPI to platform_get_irq()
  2019-02-07 20:18       ` Andy Shevchenko
@ 2019-02-07 20:29         ` Dmitry Torokhov
  2019-02-11 10:30           ` Mika Westerberg
  0 siblings, 1 reply; 21+ messages in thread
From: Dmitry Torokhov @ 2019-02-07 20:29 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Enrico Granata, Rafael J. Wysocki, Greg Kroah-Hartman,
	Enric Balletbo i Serra, Linux Kernel Mailing List,
	Gwendal Grignou, ACPI Devel Maling List, Brian Norris,
	Enrico Granata, Mika Westerberg, Andy Shevchenko, Hans de Goede

On Thu, Feb 7, 2019 at 12:18 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Feb 7, 2019 at 9:45 PM Enrico Granata <egranata@google.com> wrote:
> > On Thu, Feb 7, 2019 at 11:39 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >> On Thu, Feb 7, 2019 at 9:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> >> > > This is not currently done for the platform drivers, as platform_get_irq()
> >> > > does not try to parse GpioInt() resources.
> >>
> >> And why is this a problem?
>
> > In ChromeOS, we have a driver (cros_ec_lpc) which can run either on systems that directly expose the interrupt,
> > or systems where the interrupt goes through a GPIO controller. On the former, firmware provides an Interrupt resource
> > and platform_get_irq() finds it. On the latter, firmware provides a GpioInt resource and platform_get_irq does not
> > find it. We could work around this in the driver by probing both paths, but since other subsystems seem to directly
> > look for GpioInt resources, it seemed to us to make more sense to extend platform_get_irq() instead.
>
> Looking briefly into the driver I found third scenario — no resource at all.
> So, you already have a quirk for that. Now it's the question either
> you go for global quirk (trying to find an IRQ via iterating over
> GpioInt() resources like in this patch, but in the driver), or use DMI
> table for more stricter rules.

No, no DMI rules please, they are more pain than anything.

>
> Either way you choose, I don't see a necessity to put this to the
> driver core for now since it would be the only (let's assume properly
> written ACPI tables) driver needs such.

This is simply kicking the can down the road.

>
> > Do you have a suggestion as to how to write ACPI tables to avoid the issue?
>
> 1. Allocate new ID and use it (perhaps not the best path).
> 2. Use GPE(s).
>

Or just solve the issue of intermixing Interrupt() with GpioInt(). We
have similar issue with i2c and spi, but we sidestep that there as we
only parse the first interrupt and do not give option of fetching 2nd,
3rd, etc. Maybe we should only GpioInt parsing for the first interrupt
in platform_get_irq() as well for the first iteration and then see if
we need to improve it if we see devices with multiple interrupts.

Thanks,
Dmitry

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] driver: platform: Add support for GpioInt() ACPI to platform_get_irq()
  2019-02-07 20:29         ` Dmitry Torokhov
@ 2019-02-11 10:30           ` Mika Westerberg
  2019-02-11 15:42             ` Hans de Goede
  0 siblings, 1 reply; 21+ messages in thread
From: Mika Westerberg @ 2019-02-11 10:30 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Andy Shevchenko, Enrico Granata, Rafael J. Wysocki,
	Greg Kroah-Hartman, Enric Balletbo i Serra,
	Linux Kernel Mailing List, Gwendal Grignou,
	ACPI Devel Maling List, Brian Norris, Enrico Granata,
	Andy Shevchenko, Hans de Goede

Hi,

On Thu, Feb 07, 2019 at 12:29:17PM -0800, Dmitry Torokhov wrote:
> >
> > > Do you have a suggestion as to how to write ACPI tables to avoid the issue?
> >
> > 1. Allocate new ID and use it (perhaps not the best path).
> > 2. Use GPE(s).
> >
> 
> Or just solve the issue of intermixing Interrupt() with GpioInt(). We
> have similar issue with i2c and spi, but we sidestep that there as we
> only parse the first interrupt and do not give option of fetching 2nd,
> 3rd, etc. Maybe we should only GpioInt parsing for the first interrupt
> in platform_get_irq() as well for the first iteration and then see if
> we need to improve it if we see devices with multiple interrupts.

I think it should be fine to intermix them or do what you suggest and
start supporting index 0 for now and then maybe extend it in the future
to cover more.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] driver: platform: Add support for GpioInt() ACPI to platform_get_irq()
  2019-02-11 10:30           ` Mika Westerberg
@ 2019-02-11 15:42             ` Hans de Goede
  0 siblings, 0 replies; 21+ messages in thread
From: Hans de Goede @ 2019-02-11 15:42 UTC (permalink / raw)
  To: Mika Westerberg, Dmitry Torokhov
  Cc: Andy Shevchenko, Enrico Granata, Rafael J. Wysocki,
	Greg Kroah-Hartman, Enric Balletbo i Serra,
	Linux Kernel Mailing List, Gwendal Grignou,
	ACPI Devel Maling List, Brian Norris, Enrico Granata,
	Andy Shevchenko

Hi,

On 11-02-19 11:30, Mika Westerberg wrote:
> Hi,
> 
> On Thu, Feb 07, 2019 at 12:29:17PM -0800, Dmitry Torokhov wrote:
>>>
>>>> Do you have a suggestion as to how to write ACPI tables to avoid the issue?
>>>
>>> 1. Allocate new ID and use it (perhaps not the best path).
>>> 2. Use GPE(s).
>>>
>>
>> Or just solve the issue of intermixing Interrupt() with GpioInt(). We
>> have similar issue with i2c and spi, but we sidestep that there as we
>> only parse the first interrupt and do not give option of fetching 2nd,
>> 3rd, etc. Maybe we should only GpioInt parsing for the first interrupt
>> in platform_get_irq() as well for the first iteration and then see if
>> we need to improve it if we see devices with multiple interrupts.
> 
> I think it should be fine to intermix them or do what you suggest and
> start supporting index 0 for now and then maybe extend it in the future
> to cover more.

I think only support fallback to GpioInt for index 0 for now is probably
the best solution. A device could have both Interrupt and GpioInt resources,
as soon as that is the case then the meaning of index becomes ambiguous.

We are already seeing something similar with mixed use of GpioInt + Gpio
resources on some devices, where we need the GpioInt for the IRQ and
the Gpio resource to toggle something else and the ACPI tables on
different devices have them in a different order, see:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=4d1f7a6eabd45639d9de22a8a004f3c208d13c1a

I suspect that on Windows device drivers specifically specify if they want a
Gpio or a GpioInt; or in this case if they want an Interrupt or a GpioInt.

Regards,

Hans

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq()
  2019-02-07 18:59 [PATCH] driver: platform: Add support for GpioInt() ACPI to platform_get_irq() egranata
  2019-02-07 19:02 ` Rafael J. Wysocki
@ 2019-02-11 19:01 ` egranata
  2019-02-11 19:25   ` Dmitry Torokhov
                     ` (6 more replies)
  1 sibling, 7 replies; 21+ messages in thread
From: egranata @ 2019-02-11 19:01 UTC (permalink / raw)
  To: hdegoede, mika.westerberg, dtor, andy.shevchenko, rafael, gregkh,
	enric.balletbo, linux-kernel, gwendal, linux-acpi, briannorris,
	andriy.shevchenko
  Cc: egranata, Enrico Granata

From: Enrico Granata <egranata@chromium.org>

ACPI 5 added support for GpioInt resources as a way to provide
information about interrupts mediated via a GPIO controller.

Several device buses (e.g. SPI, I2C) have support for retrieving
an IRQ specified via this type of resource, and providing it
directly to the driver as an IRQ number.

This is not currently done for the platform drivers, as platform_get_irq()
does not try to parse GpioInt() resources. This requires drivers to
either have to support only one possible IRQ resource, or to have code
in place to try both as a failsafe.

While there is a possibility of ambiguity for devices that exposes
multiple IRQs, it is easy and feasible to support the common case
of devices that only expose one IRQ which would be of either type
depending on the underlying system's architecture.

This commit adds support for parsing a GpioInt resource in order
to fulfill a request for the index 0 IRQ for a platform device.

Signed-off-by: Enrico Granata <egranata@chromium.org>
---
Changes in v2:
 - only support IRQ index 0

 drivers/base/platform.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 1c958eb33ef4d..0d3611cd1b3bc 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -127,7 +127,20 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
 	}
 
-	return r ? r->start : -ENXIO;
+	if (r)
+		return r->start;
+
+	/*
+	 * For the index 0 interrupt, allow falling back to GpioInt
+	 * resources. While a device could have both Interrupt and GpioInt
+	 * resources, making this fallback ambiguous, in many common cases
+	 * the device will only expose one IRQ, and this fallback
+	 * allows a common code path across either kind of resource.
+	 */
+	if (num == 0 && has_acpi_companion(&dev->dev))
+		return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
+
+	return -ENXIO;
 #endif
 }
 EXPORT_SYMBOL_GPL(platform_get_irq);
-- 
2.20.1.791.gb4d0f1c61a-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq()
  2019-02-11 19:01 ` [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq() egranata
@ 2019-02-11 19:25   ` Dmitry Torokhov
  2019-02-12  7:29   ` Hans de Goede
                     ` (5 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Dmitry Torokhov @ 2019-02-11 19:25 UTC (permalink / raw)
  To: egranata
  Cc: Hans de Goede, Mika Westerberg, andy.shevchenko,
	Rafael J. Wysocki, Greg Kroah-Hartman, enric.balletbo, lkml,
	gwendal, ACPI Devel Maling List, briannorris, andriy.shevchenko,
	Enrico Granata

On Mon, Feb 11, 2019 at 11:01 AM <egranata@chromium.org> wrote:
>
> From: Enrico Granata <egranata@chromium.org>
>
> ACPI 5 added support for GpioInt resources as a way to provide
> information about interrupts mediated via a GPIO controller.
>
> Several device buses (e.g. SPI, I2C) have support for retrieving
> an IRQ specified via this type of resource, and providing it
> directly to the driver as an IRQ number.
>
> This is not currently done for the platform drivers, as platform_get_irq()
> does not try to parse GpioInt() resources. This requires drivers to
> either have to support only one possible IRQ resource, or to have code
> in place to try both as a failsafe.
>
> While there is a possibility of ambiguity for devices that exposes
> multiple IRQs, it is easy and feasible to support the common case
> of devices that only expose one IRQ which would be of either type
> depending on the underlying system's architecture.
>
> This commit adds support for parsing a GpioInt resource in order
> to fulfill a request for the index 0 IRQ for a platform device.
>
> Signed-off-by: Enrico Granata <egranata@chromium.org>

Yeah, this looks good to me.

Reviewed-by: Dmitry Torokhov <dtor@chromium.org>

> ---
> Changes in v2:
>  - only support IRQ index 0
>
>  drivers/base/platform.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1c958eb33ef4d..0d3611cd1b3bc 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -127,7 +127,20 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>                 irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
>         }
>
> -       return r ? r->start : -ENXIO;
> +       if (r)
> +               return r->start;
> +
> +       /*
> +        * For the index 0 interrupt, allow falling back to GpioInt
> +        * resources. While a device could have both Interrupt and GpioInt
> +        * resources, making this fallback ambiguous, in many common cases
> +        * the device will only expose one IRQ, and this fallback
> +        * allows a common code path across either kind of resource.
> +        */
> +       if (num == 0 && has_acpi_companion(&dev->dev))
> +               return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> +
> +       return -ENXIO;
>  #endif
>  }
>  EXPORT_SYMBOL_GPL(platform_get_irq);
> --
> 2.20.1.791.gb4d0f1c61a-goog
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq()
  2019-02-11 19:01 ` [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq() egranata
  2019-02-11 19:25   ` Dmitry Torokhov
@ 2019-02-12  7:29   ` Hans de Goede
  2019-02-12  9:08   ` Mika Westerberg
                     ` (4 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Hans de Goede @ 2019-02-12  7:29 UTC (permalink / raw)
  To: egranata, mika.westerberg, dtor, andy.shevchenko, rafael, gregkh,
	enric.balletbo, linux-kernel, gwendal, linux-acpi, briannorris,
	andriy.shevchenko
  Cc: egranata

Hi,

On 11-02-19 20:01, egranata@chromium.org wrote:
> From: Enrico Granata <egranata@chromium.org>
> 
> ACPI 5 added support for GpioInt resources as a way to provide
> information about interrupts mediated via a GPIO controller.
> 
> Several device buses (e.g. SPI, I2C) have support for retrieving
> an IRQ specified via this type of resource, and providing it
> directly to the driver as an IRQ number.
> 
> This is not currently done for the platform drivers, as platform_get_irq()
> does not try to parse GpioInt() resources. This requires drivers to
> either have to support only one possible IRQ resource, or to have code
> in place to try both as a failsafe.
> 
> While there is a possibility of ambiguity for devices that exposes
> multiple IRQs, it is easy and feasible to support the common case
> of devices that only expose one IRQ which would be of either type
> depending on the underlying system's architecture.
> 
> This commit adds support for parsing a GpioInt resource in order
> to fulfill a request for the index 0 IRQ for a platform device.
> 
> Signed-off-by: Enrico Granata <egranata@chromium.org>

Looks good to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
> Changes in v2:
>   - only support IRQ index 0
> 
>   drivers/base/platform.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1c958eb33ef4d..0d3611cd1b3bc 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -127,7 +127,20 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>   		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
>   	}
>   
> -	return r ? r->start : -ENXIO;
> +	if (r)
> +		return r->start;
> +
> +	/*
> +	 * For the index 0 interrupt, allow falling back to GpioInt
> +	 * resources. While a device could have both Interrupt and GpioInt
> +	 * resources, making this fallback ambiguous, in many common cases
> +	 * the device will only expose one IRQ, and this fallback
> +	 * allows a common code path across either kind of resource.
> +	 */
> +	if (num == 0 && has_acpi_companion(&dev->dev))
> +		return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> +
> +	return -ENXIO;
>   #endif
>   }
>   EXPORT_SYMBOL_GPL(platform_get_irq);
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq()
  2019-02-11 19:01 ` [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq() egranata
  2019-02-11 19:25   ` Dmitry Torokhov
  2019-02-12  7:29   ` Hans de Goede
@ 2019-02-12  9:08   ` Mika Westerberg
  2019-02-12  9:18   ` Rafael J. Wysocki
                     ` (3 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Mika Westerberg @ 2019-02-12  9:08 UTC (permalink / raw)
  To: egranata
  Cc: hdegoede, dtor, andy.shevchenko, rafael, gregkh, enric.balletbo,
	linux-kernel, gwendal, linux-acpi, briannorris,
	andriy.shevchenko, egranata

On Mon, Feb 11, 2019 at 11:01:12AM -0800, egranata@chromium.org wrote:
> From: Enrico Granata <egranata@chromium.org>
> 
> ACPI 5 added support for GpioInt resources as a way to provide
> information about interrupts mediated via a GPIO controller.
> 
> Several device buses (e.g. SPI, I2C) have support for retrieving
> an IRQ specified via this type of resource, and providing it
> directly to the driver as an IRQ number.
> 
> This is not currently done for the platform drivers, as platform_get_irq()
> does not try to parse GpioInt() resources. This requires drivers to
> either have to support only one possible IRQ resource, or to have code
> in place to try both as a failsafe.
> 
> While there is a possibility of ambiguity for devices that exposes
> multiple IRQs, it is easy and feasible to support the common case
> of devices that only expose one IRQ which would be of either type
> depending on the underlying system's architecture.
> 
> This commit adds support for parsing a GpioInt resource in order
> to fulfill a request for the index 0 IRQ for a platform device.
> 
> Signed-off-by: Enrico Granata <egranata@chromium.org>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq()
  2019-02-11 19:01 ` [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq() egranata
                     ` (2 preceding siblings ...)
  2019-02-12  9:08   ` Mika Westerberg
@ 2019-02-12  9:18   ` Rafael J. Wysocki
  2019-02-12 12:46   ` Andy Shevchenko
                     ` (2 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Rafael J. Wysocki @ 2019-02-12  9:18 UTC (permalink / raw)
  To: Enrico Granata
  Cc: Hans de Goede, Mika Westerberg, Dmitry Torokhov, Andy Shevchenko,
	Rafael J. Wysocki, Greg Kroah-Hartman, Enric Balletbo i Serra,
	Linux Kernel Mailing List, Gwendal Grignou,
	ACPI Devel Maling List, Brian Norris, Andy Shevchenko,
	Enrico Granata

On Mon, Feb 11, 2019 at 8:01 PM <egranata@chromium.org> wrote:
>
> From: Enrico Granata <egranata@chromium.org>
>
> ACPI 5 added support for GpioInt resources as a way to provide
> information about interrupts mediated via a GPIO controller.
>
> Several device buses (e.g. SPI, I2C) have support for retrieving
> an IRQ specified via this type of resource, and providing it
> directly to the driver as an IRQ number.
>
> This is not currently done for the platform drivers, as platform_get_irq()
> does not try to parse GpioInt() resources. This requires drivers to
> either have to support only one possible IRQ resource, or to have code
> in place to try both as a failsafe.
>
> While there is a possibility of ambiguity for devices that exposes
> multiple IRQs, it is easy and feasible to support the common case
> of devices that only expose one IRQ which would be of either type
> depending on the underlying system's architecture.
>
> This commit adds support for parsing a GpioInt resource in order
> to fulfill a request for the index 0 IRQ for a platform device.
>
> Signed-off-by: Enrico Granata <egranata@chromium.org>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
> Changes in v2:
>  - only support IRQ index 0
>
>  drivers/base/platform.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1c958eb33ef4d..0d3611cd1b3bc 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -127,7 +127,20 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>                 irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
>         }
>
> -       return r ? r->start : -ENXIO;
> +       if (r)
> +               return r->start;
> +
> +       /*
> +        * For the index 0 interrupt, allow falling back to GpioInt
> +        * resources. While a device could have both Interrupt and GpioInt
> +        * resources, making this fallback ambiguous, in many common cases
> +        * the device will only expose one IRQ, and this fallback
> +        * allows a common code path across either kind of resource.
> +        */
> +       if (num == 0 && has_acpi_companion(&dev->dev))
> +               return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> +
> +       return -ENXIO;
>  #endif
>  }
>  EXPORT_SYMBOL_GPL(platform_get_irq);
> --
> 2.20.1.791.gb4d0f1c61a-goog
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq()
  2019-02-11 19:01 ` [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq() egranata
                     ` (3 preceding siblings ...)
  2019-02-12  9:18   ` Rafael J. Wysocki
@ 2019-02-12 12:46   ` Andy Shevchenko
  2019-02-20 18:05   ` Brian Norris
  2019-02-24 19:34   ` [PATCH v2] " Rafael J. Wysocki
  6 siblings, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2019-02-12 12:46 UTC (permalink / raw)
  To: egranata
  Cc: hdegoede, mika.westerberg, dtor, rafael, gregkh, enric.balletbo,
	linux-kernel, gwendal, linux-acpi, briannorris, egranata

On Mon, Feb 11, 2019 at 11:01:12AM -0800, egranata@chromium.org wrote:
> From: Enrico Granata <egranata@chromium.org>
> 
> ACPI 5 added support for GpioInt resources as a way to provide
> information about interrupts mediated via a GPIO controller.
> 
> Several device buses (e.g. SPI, I2C) have support for retrieving
> an IRQ specified via this type of resource, and providing it
> directly to the driver as an IRQ number.
> 
> This is not currently done for the platform drivers, as platform_get_irq()
> does not try to parse GpioInt() resources. This requires drivers to
> either have to support only one possible IRQ resource, or to have code
> in place to try both as a failsafe.
> 
> While there is a possibility of ambiguity for devices that exposes
> multiple IRQs, it is easy and feasible to support the common case
> of devices that only expose one IRQ which would be of either type
> depending on the underlying system's architecture.
> 
> This commit adds support for parsing a GpioInt resource in order
> to fulfill a request for the index 0 IRQ for a platform device.
> 

Yes, let's go this way as a compromise to get major of the cases working.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Enrico Granata <egranata@chromium.org>
> ---
> Changes in v2:
>  - only support IRQ index 0
> 
>  drivers/base/platform.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1c958eb33ef4d..0d3611cd1b3bc 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -127,7 +127,20 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>  		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
>  	}
>  
> -	return r ? r->start : -ENXIO;
> +	if (r)
> +		return r->start;
> +
> +	/*
> +	 * For the index 0 interrupt, allow falling back to GpioInt
> +	 * resources. While a device could have both Interrupt and GpioInt
> +	 * resources, making this fallback ambiguous, in many common cases
> +	 * the device will only expose one IRQ, and this fallback
> +	 * allows a common code path across either kind of resource.
> +	 */
> +	if (num == 0 && has_acpi_companion(&dev->dev))
> +		return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> +
> +	return -ENXIO;
>  #endif
>  }
>  EXPORT_SYMBOL_GPL(platform_get_irq);
> -- 
> 2.20.1.791.gb4d0f1c61a-goog
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq()
  2019-02-11 19:01 ` [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq() egranata
                     ` (4 preceding siblings ...)
  2019-02-12 12:46   ` Andy Shevchenko
@ 2019-02-20 18:05   ` Brian Norris
  2019-02-21 18:58     ` Enrico Granata
  2019-02-21 19:34     ` [PATCH v3] " egranata
  2019-02-24 19:34   ` [PATCH v2] " Rafael J. Wysocki
  6 siblings, 2 replies; 21+ messages in thread
From: Brian Norris @ 2019-02-20 18:05 UTC (permalink / raw)
  To: egranata
  Cc: hdegoede, mika.westerberg, dtor, andy.shevchenko, rafael, gregkh,
	enric.balletbo, linux-kernel, gwendal, linux-acpi,
	andriy.shevchenko, egranata

Hi,

On Mon, Feb 11, 2019 at 11:01:12AM -0800, egranata@chromium.org wrote:
> From: Enrico Granata <egranata@chromium.org>
> 
> ACPI 5 added support for GpioInt resources as a way to provide
> information about interrupts mediated via a GPIO controller.
> 
> Several device buses (e.g. SPI, I2C) have support for retrieving
> an IRQ specified via this type of resource, and providing it
> directly to the driver as an IRQ number.
> 
> This is not currently done for the platform drivers, as platform_get_irq()
> does not try to parse GpioInt() resources. This requires drivers to
> either have to support only one possible IRQ resource, or to have code
> in place to try both as a failsafe.
> 
> While there is a possibility of ambiguity for devices that exposes
> multiple IRQs, it is easy and feasible to support the common case
> of devices that only expose one IRQ which would be of either type
> depending on the underlying system's architecture.
> 
> This commit adds support for parsing a GpioInt resource in order
> to fulfill a request for the index 0 IRQ for a platform device.
> 
> Signed-off-by: Enrico Granata <egranata@chromium.org>
> ---
> Changes in v2:
>  - only support IRQ index 0
> 
>  drivers/base/platform.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1c958eb33ef4d..0d3611cd1b3bc 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -127,7 +127,20 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>  		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
>  	}
>  
> -	return r ? r->start : -ENXIO;
> +	if (r)
> +		return r->start;
> +
> +	/*
> +	 * For the index 0 interrupt, allow falling back to GpioInt
> +	 * resources. While a device could have both Interrupt and GpioInt
> +	 * resources, making this fallback ambiguous, in many common cases
> +	 * the device will only expose one IRQ, and this fallback
> +	 * allows a common code path across either kind of resource.
> +	 */
> +	if (num == 0 && has_acpi_companion(&dev->dev))
> +		return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);

For ACPI devices, this changes the return code for a missing interrupt
0 from ENXIO to ENOENT, because acpi_dev_gpio_irq_get() uses ENOENT
instead of ENXIO. While ENXIO isn't exactly documented as the *specific*
error code for a missing interrupt in platform_get_irq(), there are
definitely drivers out there that are looking specifically for ENXIO
(grepping the tree finds several Rockchip platform drivers and a few
ethernet drivers at a minimum). And it also incidentally broke some
usage of the very driver you were trying to support
(drivers/platform/chrome/cros_ec_lpc.c).

I suspect a good strategy here would be to check
acpi_dev_gpio_irq_get()'s return codes here with something like:

	if (ret > 0 || ret == -EPROBE_DEFER)
		return ret;
	return -ENXIO;

Although, the gpiolib functions embedded in there also can return EIO,
so maybe something like this is better?

	if (ret == -ENOENT || ret == 0)
		return -ENXIO;
	return ret;

I'm kinda unsure what to do with error codes besides PROBE_DEFER or
"missing", since most users don't really have it in their mind that
platform_get_irq() can fail with EIO or similar.

Brian

> +
> +	return -ENXIO;
>  #endif
>  }
>  EXPORT_SYMBOL_GPL(platform_get_irq);
> -- 
> 2.20.1.791.gb4d0f1c61a-goog
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq()
  2019-02-20 18:05   ` Brian Norris
@ 2019-02-21 18:58     ` Enrico Granata
  2019-02-21 19:34     ` [PATCH v3] " egranata
  1 sibling, 0 replies; 21+ messages in thread
From: Enrico Granata @ 2019-02-21 18:58 UTC (permalink / raw)
  To: Brian Norris
  Cc: Enrico Granata, Hans de Goede, Mika Westerberg, Dmitry Torokhov,
	Andy Shevchenko, Rafael J. Wysocki, Greg Kroah-Hartman,
	Enric Balletbo i Serra, Linux Kernel Mailing List,
	Gwendal Grignou, ACPI Devel Maling List, Andy Shevchenko

Thanks for catching that and sorry for the delayed response, I was on vacation.

I think your analysis makes sense. I would personally lean towards the
former suggestion (keeping the change localized to the return value of
platform_get_irq() which is the function that apparently has an
informal contract about returning -ENXIO specifically).

I am happy to post a PATCH v3 to that effect if this seems amenable.

Thanks

Enrico Granata | egranata@google.com | ChromeOS | MTV1600


On Wed, Feb 20, 2019 at 10:05 AM Brian Norris <briannorris@chromium.org> wrote:
>
> Hi,
>
> On Mon, Feb 11, 2019 at 11:01:12AM -0800, egranata@chromium.org wrote:
> > From: Enrico Granata <egranata@chromium.org>
> >
> > ACPI 5 added support for GpioInt resources as a way to provide
> > information about interrupts mediated via a GPIO controller.
> >
> > Several device buses (e.g. SPI, I2C) have support for retrieving
> > an IRQ specified via this type of resource, and providing it
> > directly to the driver as an IRQ number.
> >
> > This is not currently done for the platform drivers, as platform_get_irq()
> > does not try to parse GpioInt() resources. This requires drivers to
> > either have to support only one possible IRQ resource, or to have code
> > in place to try both as a failsafe.
> >
> > While there is a possibility of ambiguity for devices that exposes
> > multiple IRQs, it is easy and feasible to support the common case
> > of devices that only expose one IRQ which would be of either type
> > depending on the underlying system's architecture.
> >
> > This commit adds support for parsing a GpioInt resource in order
> > to fulfill a request for the index 0 IRQ for a platform device.
> >
> > Signed-off-by: Enrico Granata <egranata@chromium.org>
> > ---
> > Changes in v2:
> >  - only support IRQ index 0
> >
> >  drivers/base/platform.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 1c958eb33ef4d..0d3611cd1b3bc 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -127,7 +127,20 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
> >               irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
> >       }
> >
> > -     return r ? r->start : -ENXIO;
> > +     if (r)
> > +             return r->start;
> > +
> > +     /*
> > +      * For the index 0 interrupt, allow falling back to GpioInt
> > +      * resources. While a device could have both Interrupt and GpioInt
> > +      * resources, making this fallback ambiguous, in many common cases
> > +      * the device will only expose one IRQ, and this fallback
> > +      * allows a common code path across either kind of resource.
> > +      */
> > +     if (num == 0 && has_acpi_companion(&dev->dev))
> > +             return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
>
> For ACPI devices, this changes the return code for a missing interrupt
> 0 from ENXIO to ENOENT, because acpi_dev_gpio_irq_get() uses ENOENT
> instead of ENXIO. While ENXIO isn't exactly documented as the *specific*
> error code for a missing interrupt in platform_get_irq(), there are
> definitely drivers out there that are looking specifically for ENXIO
> (grepping the tree finds several Rockchip platform drivers and a few
> ethernet drivers at a minimum). And it also incidentally broke some
> usage of the very driver you were trying to support
> (drivers/platform/chrome/cros_ec_lpc.c).
>
> I suspect a good strategy here would be to check
> acpi_dev_gpio_irq_get()'s return codes here with something like:
>
>         if (ret > 0 || ret == -EPROBE_DEFER)
>                 return ret;
>         return -ENXIO;
>
> Although, the gpiolib functions embedded in there also can return EIO,
> so maybe something like this is better?
>
>         if (ret == -ENOENT || ret == 0)
>                 return -ENXIO;
>         return ret;
>
> I'm kinda unsure what to do with error codes besides PROBE_DEFER or
> "missing", since most users don't really have it in their mind that
> platform_get_irq() can fail with EIO or similar.
>
> Brian
>
> > +
> > +     return -ENXIO;
> >  #endif
> >  }
> >  EXPORT_SYMBOL_GPL(platform_get_irq);
> > --
> > 2.20.1.791.gb4d0f1c61a-goog
> >

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v3] driver: platform: Support parsing GpioInt 0 in platform_get_irq()
  2019-02-20 18:05   ` Brian Norris
  2019-02-21 18:58     ` Enrico Granata
@ 2019-02-21 19:34     ` egranata
  2019-02-22  9:03       ` Rafael J. Wysocki
  1 sibling, 1 reply; 21+ messages in thread
From: egranata @ 2019-02-21 19:34 UTC (permalink / raw)
  To: gwendal, briannorris, hdegoede, mika.westerberg, dtor,
	andy.shevchenko, rafael, gregkh, enric.balletbo, linux-kernel,
	linux-acpi, andriy.shevchenko
  Cc: egranata, Enrico Granata

From: Enrico Granata <egranata@chromium.org>

ACPI 5 added support for GpioInt resources as a way to provide
information about interrupts mediated via a GPIO controller.

Several device buses (e.g. SPI, I2C) have support for retrieving
an IRQ specified via this type of resource, and providing it
directly to the driver as an IRQ number.

This is not currently done for the platform drivers, as platform_get_irq()
does not try to parse GpioInt() resources. This requires drivers to
either have to support only one possible IRQ resource, or to have code
in place to try both as a failsafe.

While there is a possibility of ambiguity for devices that exposes
multiple IRQs, it is easy and feasible to support the common case
of devices that only expose one IRQ which would be of either type
depending on the underlying system's architecture.

This commit adds support for parsing a GpioInt resource in order
to fulfill a request for the index 0 IRQ for a platform device.

Signed-off-by: Enrico Granata <egranata@chromium.org>
---
Changes in v3:
 - ensured that -ENOENT return from acpi_dev_gpio_irq_get is not propagated
   upwards, as some drivers expect platform_get_irq to return either a valid
   IRQ or -ENXIO and will break otherwise

 drivers/base/platform.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 1c958eb33ef4d..afd8b916303e4 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -127,7 +127,24 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
 	}
 
-	return r ? r->start : -ENXIO;
+	if (r)
+		return r->start;
+
+	/*
+	 * For the index 0 interrupt, allow falling back to GpioInt
+	 * resources. While a device could have both Interrupt and GpioInt
+	 * resources, making this fallback ambiguous, in many common cases
+	 * the device will only expose one IRQ, and this fallback
+	 * allows a common code path across either kind of resource.
+	 */
+	if (num == 0 && has_acpi_companion(&dev->dev)) {
+		int ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
+
+		if (ret > 0 || ret == -EPROBE_DEFER)
+			return ret;
+	}
+
+	return -ENXIO;
 #endif
 }
 EXPORT_SYMBOL_GPL(platform_get_irq);
-- 
2.21.0.rc0.258.g878e2cd30e-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] driver: platform: Support parsing GpioInt 0 in platform_get_irq()
  2019-02-21 19:34     ` [PATCH v3] " egranata
@ 2019-02-22  9:03       ` Rafael J. Wysocki
  2019-02-22 17:06         ` Brian Norris
  0 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2019-02-22  9:03 UTC (permalink / raw)
  To: Enrico Granata
  Cc: Gwendal Grignou, Brian Norris, Hans de Goede, Mika Westerberg,
	Dmitry Torokhov, Andy Shevchenko, Rafael J. Wysocki,
	Greg Kroah-Hartman, Enric Balletbo i Serra,
	Linux Kernel Mailing List, ACPI Devel Maling List,
	Andy Shevchenko, Enrico Granata

On Thu, Feb 21, 2019 at 8:34 PM <egranata@chromium.org> wrote:
>
> From: Enrico Granata <egranata@chromium.org>
>
> ACPI 5 added support for GpioInt resources as a way to provide
> information about interrupts mediated via a GPIO controller.
>
> Several device buses (e.g. SPI, I2C) have support for retrieving
> an IRQ specified via this type of resource, and providing it
> directly to the driver as an IRQ number.
>
> This is not currently done for the platform drivers, as platform_get_irq()
> does not try to parse GpioInt() resources. This requires drivers to
> either have to support only one possible IRQ resource, or to have code
> in place to try both as a failsafe.
>
> While there is a possibility of ambiguity for devices that exposes
> multiple IRQs, it is easy and feasible to support the common case
> of devices that only expose one IRQ which would be of either type
> depending on the underlying system's architecture.
>
> This commit adds support for parsing a GpioInt resource in order
> to fulfill a request for the index 0 IRQ for a platform device.
>
> Signed-off-by: Enrico Granata <egranata@chromium.org>
> ---
> Changes in v3:
>  - ensured that -ENOENT return from acpi_dev_gpio_irq_get is not propagated
>    upwards, as some drivers expect platform_get_irq to return either a valid
>    IRQ or -ENXIO and will break otherwise
>
>  drivers/base/platform.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1c958eb33ef4d..afd8b916303e4 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -127,7 +127,24 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>                 irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
>         }
>
> -       return r ? r->start : -ENXIO;
> +       if (r)
> +               return r->start;
> +
> +       /*
> +        * For the index 0 interrupt, allow falling back to GpioInt
> +        * resources. While a device could have both Interrupt and GpioInt
> +        * resources, making this fallback ambiguous, in many common cases
> +        * the device will only expose one IRQ, and this fallback
> +        * allows a common code path across either kind of resource.
> +        */
> +       if (num == 0 && has_acpi_companion(&dev->dev)) {
> +               int ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> +
> +               if (ret > 0 || ret == -EPROBE_DEFER)

Can't 0 be a valid GPIO IRQ?

> +                       return ret;
> +       }
> +
> +       return -ENXIO;
>  #endif
>  }
>  EXPORT_SYMBOL_GPL(platform_get_irq);
> --

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] driver: platform: Support parsing GpioInt 0 in platform_get_irq()
  2019-02-22  9:03       ` Rafael J. Wysocki
@ 2019-02-22 17:06         ` Brian Norris
  0 siblings, 0 replies; 21+ messages in thread
From: Brian Norris @ 2019-02-22 17:06 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Enrico Granata, Gwendal Grignou, Hans de Goede, Mika Westerberg,
	Dmitry Torokhov, Andy Shevchenko, Greg Kroah-Hartman,
	Enric Balletbo i Serra, Linux Kernel Mailing List,
	ACPI Devel Maling List, Andy Shevchenko, Enrico Granata

On Fri, Feb 22, 2019 at 1:03 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Feb 21, 2019 at 8:34 PM <egranata@chromium.org> wrote:
> >
> > From: Enrico Granata <egranata@chromium.org>
> >
> > ACPI 5 added support for GpioInt resources as a way to provide
> > information about interrupts mediated via a GPIO controller.
> >
> > Several device buses (e.g. SPI, I2C) have support for retrieving
> > an IRQ specified via this type of resource, and providing it
> > directly to the driver as an IRQ number.
> >
> > This is not currently done for the platform drivers, as platform_get_irq()
> > does not try to parse GpioInt() resources. This requires drivers to
> > either have to support only one possible IRQ resource, or to have code
> > in place to try both as a failsafe.
> >
> > While there is a possibility of ambiguity for devices that exposes
> > multiple IRQs, it is easy and feasible to support the common case
> > of devices that only expose one IRQ which would be of either type
> > depending on the underlying system's architecture.
> >
> > This commit adds support for parsing a GpioInt resource in order
> > to fulfill a request for the index 0 IRQ for a platform device.
> >
> > Signed-off-by: Enrico Granata <egranata@chromium.org>
> > ---
> > Changes in v3:
> >  - ensured that -ENOENT return from acpi_dev_gpio_irq_get is not propagated
> >    upwards, as some drivers expect platform_get_irq to return either a valid
> >    IRQ or -ENXIO and will break otherwise

I hope there are no other lurking ways in which this might break things...

Reviewed-by: Brian Norris <briannorris@chromium.org>

> >  drivers/base/platform.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 1c958eb33ef4d..afd8b916303e4 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -127,7 +127,24 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
> >                 irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
> >         }
> >
> > -       return r ? r->start : -ENXIO;
> > +       if (r)
> > +               return r->start;
> > +
> > +       /*
> > +        * For the index 0 interrupt, allow falling back to GpioInt
> > +        * resources. While a device could have both Interrupt and GpioInt
> > +        * resources, making this fallback ambiguous, in many common cases
> > +        * the device will only expose one IRQ, and this fallback
> > +        * allows a common code path across either kind of resource.
> > +        */
> > +       if (num == 0 && has_acpi_companion(&dev->dev)) {
> > +               int ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> > +
> > +               if (ret > 0 || ret == -EPROBE_DEFER)
>
> Can't 0 be a valid GPIO IRQ?

acpi_dev_gpio_irq_get() claims:

 * Return: Linux IRQ number (> %0) on success, negative errno on failure.

Should I trust the documentation? It seems like yes, I should:

int gpiod_to_irq(const struct gpio_desc *desc)
{
...
                /* Zero means NO_IRQ */
                if (!retirq)
                        return -ENXIO;


Brian

> > +                       return ret;
> > +       }
> > +
> > +       return -ENXIO;
> >  #endif
> >  }
> >  EXPORT_SYMBOL_GPL(platform_get_irq);
> > --

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq()
  2019-02-11 19:01 ` [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq() egranata
                     ` (5 preceding siblings ...)
  2019-02-20 18:05   ` Brian Norris
@ 2019-02-24 19:34   ` Rafael J. Wysocki
  6 siblings, 0 replies; 21+ messages in thread
From: Rafael J. Wysocki @ 2019-02-24 19:34 UTC (permalink / raw)
  To: Enrico Granata
  Cc: Hans de Goede, Mika Westerberg, Dmitry Torokhov, Andy Shevchenko,
	Rafael J. Wysocki, Greg Kroah-Hartman, Enric Balletbo i Serra,
	Linux Kernel Mailing List, Gwendal Grignou,
	ACPI Devel Maling List, Brian Norris, Andy Shevchenko,
	Enrico Granata

On Mon, Feb 11, 2019 at 8:01 PM <egranata@chromium.org> wrote:
>
> From: Enrico Granata <egranata@chromium.org>
>
> ACPI 5 added support for GpioInt resources as a way to provide
> information about interrupts mediated via a GPIO controller.
>
> Several device buses (e.g. SPI, I2C) have support for retrieving
> an IRQ specified via this type of resource, and providing it
> directly to the driver as an IRQ number.
>
> This is not currently done for the platform drivers, as platform_get_irq()
> does not try to parse GpioInt() resources. This requires drivers to
> either have to support only one possible IRQ resource, or to have code
> in place to try both as a failsafe.
>
> While there is a possibility of ambiguity for devices that exposes
> multiple IRQs, it is easy and feasible to support the common case
> of devices that only expose one IRQ which would be of either type
> depending on the underlying system's architecture.
>
> This commit adds support for parsing a GpioInt resource in order
> to fulfill a request for the index 0 IRQ for a platform device.
>
> Signed-off-by: Enrico Granata <egranata@chromium.org>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
> Changes in v2:
>  - only support IRQ index 0
>
>  drivers/base/platform.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1c958eb33ef4d..0d3611cd1b3bc 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -127,7 +127,20 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
>                 irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
>         }
>
> -       return r ? r->start : -ENXIO;
> +       if (r)
> +               return r->start;
> +
> +       /*
> +        * For the index 0 interrupt, allow falling back to GpioInt
> +        * resources. While a device could have both Interrupt and GpioInt
> +        * resources, making this fallback ambiguous, in many common cases
> +        * the device will only expose one IRQ, and this fallback
> +        * allows a common code path across either kind of resource.
> +        */
> +       if (num == 0 && has_acpi_companion(&dev->dev))
> +               return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
> +
> +       return -ENXIO;
>  #endif
>  }
>  EXPORT_SYMBOL_GPL(platform_get_irq);
> --
> 2.20.1.791.gb4d0f1c61a-goog
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2019-02-24 19:35 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-07 18:59 [PATCH] driver: platform: Add support for GpioInt() ACPI to platform_get_irq() egranata
2019-02-07 19:02 ` Rafael J. Wysocki
2019-02-07 19:39   ` Andy Shevchenko
2019-02-07 19:45     ` Andy Shevchenko
2019-02-07 19:55     ` Enrico Granata
     [not found]     ` <CAPR809vnpuh8nOjU3QMCh6YJUKmtX92+bnHSUKGiAXAp6NwCHQ@mail.gmail.com>
2019-02-07 20:18       ` Andy Shevchenko
2019-02-07 20:29         ` Dmitry Torokhov
2019-02-11 10:30           ` Mika Westerberg
2019-02-11 15:42             ` Hans de Goede
2019-02-11 19:01 ` [PATCH v2] driver: platform: Support parsing GpioInt 0 in platform_get_irq() egranata
2019-02-11 19:25   ` Dmitry Torokhov
2019-02-12  7:29   ` Hans de Goede
2019-02-12  9:08   ` Mika Westerberg
2019-02-12  9:18   ` Rafael J. Wysocki
2019-02-12 12:46   ` Andy Shevchenko
2019-02-20 18:05   ` Brian Norris
2019-02-21 18:58     ` Enrico Granata
2019-02-21 19:34     ` [PATCH v3] " egranata
2019-02-22  9:03       ` Rafael J. Wysocki
2019-02-22 17:06         ` Brian Norris
2019-02-24 19:34   ` [PATCH v2] " Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).