* 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ 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; 23+ 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] 23+ messages in thread