* [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq
2022-08-30 23:15 [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq Raul E Rangel
@ 2022-08-30 23:15 ` Raul E Rangel
2022-08-31 18:01 ` Rafael J. Wysocki
2022-08-30 23:15 ` [PATCH 2/8] HID: i2c-hid: " Raul E Rangel
` (7 subsequent siblings)
8 siblings, 1 reply; 31+ messages in thread
From: Raul E Rangel @ 2022-08-30 23:15 UTC (permalink / raw)
To: linux-acpi, linux-input
Cc: hdegoede, mario.limonciello, timvp, rafael, Raul E Rangel,
Dmitry Torokhov, jingle.wu, linux-kernel
The Elan I2C touchpad driver is currently manually managing the wake
IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
and instead relies on the PM subsystem. This is done by calling
dev_pm_set_wake_irq.
i2c_device_probe already calls dev_pm_set_wake_irq when using device
tree, so it's only required when using ACPI. The net result is that this
change should be a no-op. i2c_device_remove also already calls
dev_pm_clear_wake_irq, so we don't need to do that in this driver.
I tested this on an ACPI system where the touchpad doesn't have _PRW
defined. I verified I can still wake the system and that the wake source
was the touchpad IRQ GPIO.
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
drivers/input/mouse/elan_i2c_core.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index e1758d5ffe4218..7d997d2b56436b 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -33,6 +33,7 @@
#include <linux/jiffies.h>
#include <linux/completion.h>
#include <linux/of.h>
+#include <linux/pm_wakeirq.h>
#include <linux/property.h>
#include <linux/regulator/consumer.h>
#include <asm/unaligned.h>
@@ -86,8 +87,6 @@ struct elan_tp_data {
u16 fw_page_size;
u32 fw_signature_address;
- bool irq_wake;
-
u8 min_baseline;
u8 max_baseline;
bool baseline_ready;
@@ -1337,8 +1336,10 @@ static int elan_probe(struct i2c_client *client,
* Systems using device tree should set up wakeup via DTS,
* the rest will configure device as wakeup source by default.
*/
- if (!dev->of_node)
+ if (!dev->of_node) {
device_init_wakeup(dev, true);
+ dev_pm_set_wake_irq(dev, client->irq);
+ }
return 0;
}
@@ -1362,8 +1363,6 @@ static int __maybe_unused elan_suspend(struct device *dev)
if (device_may_wakeup(dev)) {
ret = elan_sleep(data);
- /* Enable wake from IRQ */
- data->irq_wake = (enable_irq_wake(client->irq) == 0);
} else {
ret = elan_set_power(data, false);
if (ret)
@@ -1394,9 +1393,6 @@ static int __maybe_unused elan_resume(struct device *dev)
dev_err(dev, "error %d enabling regulator\n", error);
goto err;
}
- } else if (data->irq_wake) {
- disable_irq_wake(client->irq);
- data->irq_wake = false;
}
error = elan_set_power(data, true);
--
2.37.2.672.g94769d06f0-goog
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq
2022-08-30 23:15 ` [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage " Raul E Rangel
@ 2022-08-31 18:01 ` Rafael J. Wysocki
2022-08-31 18:13 ` Raul Rangel
2022-08-31 19:12 ` Dmitry Torokhov
0 siblings, 2 replies; 31+ messages in thread
From: Rafael J. Wysocki @ 2022-08-31 18:01 UTC (permalink / raw)
To: Raul E Rangel
Cc: ACPI Devel Maling List, linux-input, Hans de Goede,
Mario Limonciello, timvp, Rafael J. Wysocki, Dmitry Torokhov,
jingle.wu, Linux Kernel Mailing List, Tony Lindgren
On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
>
> The Elan I2C touchpad driver is currently manually managing the wake
> IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> and instead relies on the PM subsystem. This is done by calling
> dev_pm_set_wake_irq.
>
> i2c_device_probe already calls dev_pm_set_wake_irq when using device
> tree, so it's only required when using ACPI. The net result is that this
> change should be a no-op. i2c_device_remove also already calls
> dev_pm_clear_wake_irq, so we don't need to do that in this driver.
>
> I tested this on an ACPI system where the touchpad doesn't have _PRW
> defined. I verified I can still wake the system and that the wake source
> was the touchpad IRQ GPIO.
>
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
I like this a lot, but the assumption in the wakeirq code is that the
IRQ in question will be dedicated for signaling wakeup. Does it hold
here?
> ---
>
> drivers/input/mouse/elan_i2c_core.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index e1758d5ffe4218..7d997d2b56436b 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -33,6 +33,7 @@
> #include <linux/jiffies.h>
> #include <linux/completion.h>
> #include <linux/of.h>
> +#include <linux/pm_wakeirq.h>
> #include <linux/property.h>
> #include <linux/regulator/consumer.h>
> #include <asm/unaligned.h>
> @@ -86,8 +87,6 @@ struct elan_tp_data {
> u16 fw_page_size;
> u32 fw_signature_address;
>
> - bool irq_wake;
> -
> u8 min_baseline;
> u8 max_baseline;
> bool baseline_ready;
> @@ -1337,8 +1336,10 @@ static int elan_probe(struct i2c_client *client,
> * Systems using device tree should set up wakeup via DTS,
> * the rest will configure device as wakeup source by default.
> */
> - if (!dev->of_node)
> + if (!dev->of_node) {
> device_init_wakeup(dev, true);
> + dev_pm_set_wake_irq(dev, client->irq);
> + }
>
> return 0;
> }
> @@ -1362,8 +1363,6 @@ static int __maybe_unused elan_suspend(struct device *dev)
>
> if (device_may_wakeup(dev)) {
> ret = elan_sleep(data);
> - /* Enable wake from IRQ */
> - data->irq_wake = (enable_irq_wake(client->irq) == 0);
> } else {
> ret = elan_set_power(data, false);
> if (ret)
> @@ -1394,9 +1393,6 @@ static int __maybe_unused elan_resume(struct device *dev)
> dev_err(dev, "error %d enabling regulator\n", error);
> goto err;
> }
> - } else if (data->irq_wake) {
> - disable_irq_wake(client->irq);
> - data->irq_wake = false;
> }
>
> error = elan_set_power(data, true);
> --
> 2.37.2.672.g94769d06f0-goog
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq
2022-08-31 18:01 ` Rafael J. Wysocki
@ 2022-08-31 18:13 ` Raul Rangel
2022-08-31 18:42 ` Rafael J. Wysocki
2022-08-31 19:12 ` Dmitry Torokhov
1 sibling, 1 reply; 31+ messages in thread
From: Raul Rangel @ 2022-08-31 18:13 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: ACPI Devel Maling List, linux-input, Hans de Goede,
Mario Limonciello, Tim Van Patten, Dmitry Torokhov, jingle.wu,
Linux Kernel Mailing List, Tony Lindgren
On Wed, Aug 31, 2022 at 12:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> >
> > The Elan I2C touchpad driver is currently manually managing the wake
> > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > and instead relies on the PM subsystem. This is done by calling
> > dev_pm_set_wake_irq.
> >
> > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > tree, so it's only required when using ACPI. The net result is that this
> > change should be a no-op. i2c_device_remove also already calls
> > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> >
> > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > defined. I verified I can still wake the system and that the wake source
> > was the touchpad IRQ GPIO.
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
>
> I like this a lot, but the assumption in the wakeirq code is that the
> IRQ in question will be dedicated for signaling wakeup. Does it hold
> here?
The wakeirq code defines two methods: `dev_pm_set_wake_irq` and
`dev_pm_set_dedicated_wake_irq`.
The latter is used when you have a dedicated wakeup signal. In this
driver it's currently assumed
that the IRQ and the wake IRQ are the same, so I used `dev_pm_set_wake_irq`.
This change in theory also fixes a bug where you define a dedicated
wake irq in DT, but
then the driver enables the `client->irq` as a wake source. In
practice this doesn't happen
since the elan touchpads only have a single IRQ line.
>
> > ---
> >
> > drivers/input/mouse/elan_i2c_core.c | 12 ++++--------
> > 1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> > index e1758d5ffe4218..7d997d2b56436b 100644
> > --- a/drivers/input/mouse/elan_i2c_core.c
> > +++ b/drivers/input/mouse/elan_i2c_core.c
> > @@ -33,6 +33,7 @@
> > #include <linux/jiffies.h>
> > #include <linux/completion.h>
> > #include <linux/of.h>
> > +#include <linux/pm_wakeirq.h>
> > #include <linux/property.h>
> > #include <linux/regulator/consumer.h>
> > #include <asm/unaligned.h>
> > @@ -86,8 +87,6 @@ struct elan_tp_data {
> > u16 fw_page_size;
> > u32 fw_signature_address;
> >
> > - bool irq_wake;
> > -
> > u8 min_baseline;
> > u8 max_baseline;
> > bool baseline_ready;
> > @@ -1337,8 +1336,10 @@ static int elan_probe(struct i2c_client *client,
> > * Systems using device tree should set up wakeup via DTS,
> > * the rest will configure device as wakeup source by default.
> > */
> > - if (!dev->of_node)
> > + if (!dev->of_node) {
> > device_init_wakeup(dev, true);
> > + dev_pm_set_wake_irq(dev, client->irq);
> > + }
> >
> > return 0;
> > }
> > @@ -1362,8 +1363,6 @@ static int __maybe_unused elan_suspend(struct device *dev)
> >
> > if (device_may_wakeup(dev)) {
> > ret = elan_sleep(data);
> > - /* Enable wake from IRQ */
> > - data->irq_wake = (enable_irq_wake(client->irq) == 0);
> > } else {
> > ret = elan_set_power(data, false);
> > if (ret)
> > @@ -1394,9 +1393,6 @@ static int __maybe_unused elan_resume(struct device *dev)
> > dev_err(dev, "error %d enabling regulator\n", error);
> > goto err;
> > }
> > - } else if (data->irq_wake) {
> > - disable_irq_wake(client->irq);
> > - data->irq_wake = false;
> > }
> >
> > error = elan_set_power(data, true);
> > --
> > 2.37.2.672.g94769d06f0-goog
> >
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq
2022-08-31 18:13 ` Raul Rangel
@ 2022-08-31 18:42 ` Rafael J. Wysocki
2022-09-01 6:57 ` Tony Lindgren
0 siblings, 1 reply; 31+ messages in thread
From: Rafael J. Wysocki @ 2022-08-31 18:42 UTC (permalink / raw)
To: Raul Rangel
Cc: Rafael J. Wysocki, ACPI Devel Maling List, linux-input,
Hans de Goede, Mario Limonciello, Tim Van Patten,
Dmitry Torokhov, jingle.wu, Linux Kernel Mailing List,
Tony Lindgren
On Wed, Aug 31, 2022 at 8:14 PM Raul Rangel <rrangel@chromium.org> wrote:
>
> On Wed, Aug 31, 2022 at 12:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > >
> > > The Elan I2C touchpad driver is currently manually managing the wake
> > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > and instead relies on the PM subsystem. This is done by calling
> > > dev_pm_set_wake_irq.
> > >
> > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > tree, so it's only required when using ACPI. The net result is that this
> > > change should be a no-op. i2c_device_remove also already calls
> > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > >
> > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > defined. I verified I can still wake the system and that the wake source
> > > was the touchpad IRQ GPIO.
> > >
> > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> >
>
>
> > I like this a lot, but the assumption in the wakeirq code is that the
> > IRQ in question will be dedicated for signaling wakeup. Does it hold
> > here?
>
> The wakeirq code defines two methods: `dev_pm_set_wake_irq` and
> `dev_pm_set_dedicated_wake_irq`.
> The latter is used when you have a dedicated wakeup signal. In this
> driver it's currently assumed
> that the IRQ and the wake IRQ are the same, so I used `dev_pm_set_wake_irq`.
>
> This change in theory also fixes a bug where you define a dedicated
> wake irq in DT, but
> then the driver enables the `client->irq` as a wake source. In
> practice this doesn't happen
> since the elan touchpads only have a single IRQ line.
OK, thanks!
Please feel free to add
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
to the patch.
> >
> > > ---
> > >
> > > drivers/input/mouse/elan_i2c_core.c | 12 ++++--------
> > > 1 file changed, 4 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> > > index e1758d5ffe4218..7d997d2b56436b 100644
> > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > @@ -33,6 +33,7 @@
> > > #include <linux/jiffies.h>
> > > #include <linux/completion.h>
> > > #include <linux/of.h>
> > > +#include <linux/pm_wakeirq.h>
> > > #include <linux/property.h>
> > > #include <linux/regulator/consumer.h>
> > > #include <asm/unaligned.h>
> > > @@ -86,8 +87,6 @@ struct elan_tp_data {
> > > u16 fw_page_size;
> > > u32 fw_signature_address;
> > >
> > > - bool irq_wake;
> > > -
> > > u8 min_baseline;
> > > u8 max_baseline;
> > > bool baseline_ready;
> > > @@ -1337,8 +1336,10 @@ static int elan_probe(struct i2c_client *client,
> > > * Systems using device tree should set up wakeup via DTS,
> > > * the rest will configure device as wakeup source by default.
> > > */
> > > - if (!dev->of_node)
> > > + if (!dev->of_node) {
> > > device_init_wakeup(dev, true);
> > > + dev_pm_set_wake_irq(dev, client->irq);
> > > + }
> > >
> > > return 0;
> > > }
> > > @@ -1362,8 +1363,6 @@ static int __maybe_unused elan_suspend(struct device *dev)
> > >
> > > if (device_may_wakeup(dev)) {
> > > ret = elan_sleep(data);
> > > - /* Enable wake from IRQ */
> > > - data->irq_wake = (enable_irq_wake(client->irq) == 0);
> > > } else {
> > > ret = elan_set_power(data, false);
> > > if (ret)
> > > @@ -1394,9 +1393,6 @@ static int __maybe_unused elan_resume(struct device *dev)
> > > dev_err(dev, "error %d enabling regulator\n", error);
> > > goto err;
> > > }
> > > - } else if (data->irq_wake) {
> > > - disable_irq_wake(client->irq);
> > > - data->irq_wake = false;
> > > }
> > >
> > > error = elan_set_power(data, true);
> > > --
> > > 2.37.2.672.g94769d06f0-goog
> > >
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq
2022-08-31 18:42 ` Rafael J. Wysocki
@ 2022-09-01 6:57 ` Tony Lindgren
0 siblings, 0 replies; 31+ messages in thread
From: Tony Lindgren @ 2022-09-01 6:57 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Raul Rangel, ACPI Devel Maling List, linux-input, Hans de Goede,
Mario Limonciello, Tim Van Patten, Dmitry Torokhov, jingle.wu,
Linux Kernel Mailing List
* Rafael J. Wysocki <rafael@kernel.org> [220831 18:35]:
> On Wed, Aug 31, 2022 at 8:14 PM Raul Rangel <rrangel@chromium.org> wrote:
> >
> > On Wed, Aug 31, 2022 at 12:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > > >
> > > > The Elan I2C touchpad driver is currently manually managing the wake
> > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > > and instead relies on the PM subsystem. This is done by calling
> > > > dev_pm_set_wake_irq.
> > > >
> > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > > tree, so it's only required when using ACPI. The net result is that this
> > > > change should be a no-op. i2c_device_remove also already calls
> > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > > >
> > > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > > defined. I verified I can still wake the system and that the wake source
> > > > was the touchpad IRQ GPIO.
> > > >
> > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > >
> >
> >
> > > I like this a lot, but the assumption in the wakeirq code is that the
> > > IRQ in question will be dedicated for signaling wakeup. Does it hold
> > > here?
> >
> > The wakeirq code defines two methods: `dev_pm_set_wake_irq` and
> > `dev_pm_set_dedicated_wake_irq`.
> > The latter is used when you have a dedicated wakeup signal. In this
> > driver it's currently assumed
> > that the IRQ and the wake IRQ are the same, so I used `dev_pm_set_wake_irq`.
> >
> > This change in theory also fixes a bug where you define a dedicated
> > wake irq in DT, but
> > then the driver enables the `client->irq` as a wake source. In
> > practice this doesn't happen
> > since the elan touchpads only have a single IRQ line.
>
> OK, thanks!
>
> Please feel free to add
>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> to the patch.
Looks good to me too:
Reviewed-by: Tony Lindgren <tony@atomide.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq
2022-08-31 18:01 ` Rafael J. Wysocki
2022-08-31 18:13 ` Raul Rangel
@ 2022-08-31 19:12 ` Dmitry Torokhov
2022-08-31 19:16 ` Dmitry Torokhov
1 sibling, 1 reply; 31+ messages in thread
From: Dmitry Torokhov @ 2022-08-31 19:12 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Raul E Rangel, ACPI Devel Maling List, linux-input,
Hans de Goede, Mario Limonciello, timvp, jingle.wu,
Linux Kernel Mailing List, Tony Lindgren
On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:
> On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> >
> > The Elan I2C touchpad driver is currently manually managing the wake
> > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > and instead relies on the PM subsystem. This is done by calling
> > dev_pm_set_wake_irq.
> >
> > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > tree, so it's only required when using ACPI. The net result is that this
> > change should be a no-op. i2c_device_remove also already calls
> > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> >
> > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > defined. I verified I can still wake the system and that the wake source
> > was the touchpad IRQ GPIO.
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
>
> I like this a lot [...]
I also like this a lot, but this assumes that firmware has correct
settings for the interrupt... Unfortunately it is not always the case
and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
ect) do not mark interrupt as wakeup:
src/mainboard/google/glados/variants/chell/overridetree.cb
chip drivers/i2c/generic
register "hid" = ""ELAN0000""
register "desc" = ""ELAN Touchpad""
register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
register "wake" = "GPE0_DW0_05"
device i2c 15 on end
I assume it should have been ACPI_IRQ_WAKE_LEVEL_LOW for the interrupt
to be marked as wakeup.
(we do correctly mark GPE as wakeup).
So we need to do something about older devices....
--
Dmitry
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq
2022-08-31 19:12 ` Dmitry Torokhov
@ 2022-08-31 19:16 ` Dmitry Torokhov
2022-09-01 2:17 ` Raul Rangel
0 siblings, 1 reply; 31+ messages in thread
From: Dmitry Torokhov @ 2022-08-31 19:16 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Raul E Rangel, ACPI Devel Maling List, linux-input,
Hans de Goede, Mario Limonciello, timvp, jingle.wu,
Linux Kernel Mailing List, Tony Lindgren
On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote:
> On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > >
> > > The Elan I2C touchpad driver is currently manually managing the wake
> > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > and instead relies on the PM subsystem. This is done by calling
> > > dev_pm_set_wake_irq.
> > >
> > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > tree, so it's only required when using ACPI. The net result is that this
> > > change should be a no-op. i2c_device_remove also already calls
> > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > >
> > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > defined. I verified I can still wake the system and that the wake source
> > > was the touchpad IRQ GPIO.
> > >
> > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> >
> > I like this a lot [...]
>
> I also like this a lot, but this assumes that firmware has correct
> settings for the interrupt... Unfortunately it is not always the case
> and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
> ect) do not mark interrupt as wakeup:
>
> src/mainboard/google/glados/variants/chell/overridetree.cb
>
> chip drivers/i2c/generic
> register "hid" = ""ELAN0000""
> register "desc" = ""ELAN Touchpad""
> register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
> register "wake" = "GPE0_DW0_05"
> device i2c 15 on end
>
> I assume it should have been ACPI_IRQ_WAKE_LEVEL_LOW for the interrupt
> to be marked as wakeup.
>
> (we do correctly mark GPE as wakeup).
>
> So we need to do something about older devices....
After re-reading the patch I believe this comment is more applicable to
the followup patch to elan_i2c, not this one, which is fine on its own.
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq
2022-08-31 19:16 ` Dmitry Torokhov
@ 2022-09-01 2:17 ` Raul Rangel
2022-09-03 5:06 ` Dmitry Torokhov
0 siblings, 1 reply; 31+ messages in thread
From: Raul Rangel @ 2022-09-01 2:17 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rafael J. Wysocki, ACPI Devel Maling List, linux-input,
Hans de Goede, Mario Limonciello, Tim Van Patten, jingle.wu,
Linux Kernel Mailing List, Tony Lindgren
On Wed, Aug 31, 2022 at 1:16 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote:
> > On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > > >
> > > > The Elan I2C touchpad driver is currently manually managing the wake
> > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > > and instead relies on the PM subsystem. This is done by calling
> > > > dev_pm_set_wake_irq.
> > > >
> > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > > tree, so it's only required when using ACPI. The net result is that this
> > > > change should be a no-op. i2c_device_remove also already calls
> > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > > >
> > > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > > defined. I verified I can still wake the system and that the wake source
> > > > was the touchpad IRQ GPIO.
> > > >
> > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > >
> > > I like this a lot [...]
> >
> > I also like this a lot, but this assumes that firmware has correct
> > settings for the interrupt... Unfortunately it is not always the case
> > and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
> > ect) do not mark interrupt as wakeup:
> >
> > src/mainboard/google/glados/variants/chell/overridetree.cb
> >
> > chip drivers/i2c/generic
> > register "hid" = ""ELAN0000""
> > register "desc" = ""ELAN Touchpad""
> > register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
> > register "wake" = "GPE0_DW0_05"
> > device i2c 15 on end
> >
So the above entry specifies the `wake` register. This generates an
ACPI _PRW resource. The patch series will actually fix devices like
this. Today without this patch series we get two wake events for a
device. The ACPI wake GPE specified by the _PRW resource, and the
erroneous GPIO wake event. But you bring up a good point.
I wrote a quick and dirty script (https://0paste.com/391849) to parse
the coreboot device tree entries. Open source firmware is great isn't
it? ;)
$ find src/mainboard/google/ -iname '*.cb' | xargs awk -f touch.awk --
src/mainboard/google/eve/devicetree.cb
1
chip drivers/i2c/hid
register "generic.hid" = ""ACPI0C50""
register "generic.desc" = ""Touchpad""
register "generic.irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
register "hid_desc_reg_offset" = "0x1"
device i2c 49 on end
end
src/mainboard/google/eve/devicetree.cb
1
chip drivers/i2c/generic
register "hid" = ""GOOG0008""
register "desc" = ""Touchpad EC Interface""
device i2c 1e on end
end
src/mainboard/google/drallion/variants/drallion/devicetree.cb
1
chip drivers/i2c/generic
register "hid" = ""ELAN0000""
register "desc" = ""ELAN Touchpad""
register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
register "probed" = "1"
device i2c 2c on end
end
src/mainboard/google/drallion/variants/drallion/devicetree.cb
1
chip drivers/i2c/generic
register "hid" = ""ELAN0000""
register "desc" = ""ELAN Touchpad""
register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
register "probed" = "1"
device i2c 15 on end
end
src/mainboard/google/sarien/variants/arcada/devicetree.cb
1
chip drivers/i2c/generic
register "hid" = ""ELAN0000""
register "desc" = ""ELAN Touchpad""
register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
register "probed" = "1"
device i2c 2c on end
end
src/mainboard/google/sarien/variants/arcada/devicetree.cb
1
chip drivers/i2c/hid
register "generic.hid" = ""PNP0C50""
register "generic.desc" = ""Cirque Touchpad""
register "generic.irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
register "generic.probed" = "1"
register "hid_desc_reg_offset" = "0x20"
device i2c 2a on end
end
src/mainboard/google/sarien/variants/sarien/devicetree.cb
1
chip drivers/i2c/generic
register "hid" = ""ELAN0000""
register "desc" = ""ELAN Touchpad""
register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
register "probed" = "1"
device i2c 2c on end
end
Total Touchpad: 202
Total Wake: 195
Out of all the touchpads defined on ChromeOS it looks like only 4
devices are missing a wake declaration. I omitted touchpanels because
ChromeOS doesn't use those as a wake source. chromeos_laptop.c already
defines some devices with i2c board_info and it sets the
`I2C_CLIENT_WAKE` flag. I'm not sure if this is actually working as
expected. `i2c_device_probe` requires a `wakeup` irq to be present in
the device tree if the `I2C_CLIENT_WAKE` flag is set, but I'm assuming
the device tree was missing wake attributes.
Anyway, patches 6, and 7 are the ones that drop the legacy behavior. I
can figure out how to add the above boards to chromeos_laptop.c and
get the wake attribute plumbed, or I can add something directly to the
elan_i2c_core, etc so others can add overrides for their boards there.
I'll also send out CLs to fix the device tree configs (not that we
would run a FW qual just for this change).
> > I assume it should have been ACPI_IRQ_WAKE_LEVEL_LOW for the interrupt
> > to be marked as wakeup.
> >
> > (we do correctly mark GPE as wakeup).
> >
> > So we need to do something about older devices....
>
> After re-reading the patch I believe this comment is more applicable to
> the followup patch to elan_i2c, not this one, which is fine on its own.
>
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq
2022-09-01 2:17 ` Raul Rangel
@ 2022-09-03 5:06 ` Dmitry Torokhov
2022-09-06 17:18 ` Raul Rangel
0 siblings, 1 reply; 31+ messages in thread
From: Dmitry Torokhov @ 2022-09-03 5:06 UTC (permalink / raw)
To: Raul Rangel
Cc: Rafael J. Wysocki, ACPI Devel Maling List, linux-input,
Hans de Goede, Mario Limonciello, Tim Van Patten, jingle.wu,
Linux Kernel Mailing List, Tony Lindgren
On Wed, Aug 31, 2022 at 08:17:23PM -0600, Raul Rangel wrote:
> On Wed, Aug 31, 2022 at 1:16 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote:
> > > On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:
> > > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > > > >
> > > > > The Elan I2C touchpad driver is currently manually managing the wake
> > > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > > > and instead relies on the PM subsystem. This is done by calling
> > > > > dev_pm_set_wake_irq.
> > > > >
> > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > > > tree, so it's only required when using ACPI. The net result is that this
> > > > > change should be a no-op. i2c_device_remove also already calls
> > > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > > > >
> > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > > > defined. I verified I can still wake the system and that the wake source
> > > > > was the touchpad IRQ GPIO.
> > > > >
> > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > >
> > > > I like this a lot [...]
> > >
>
> > > I also like this a lot, but this assumes that firmware has correct
> > > settings for the interrupt... Unfortunately it is not always the case
> > > and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
> > > ect) do not mark interrupt as wakeup:
> > >
> > > src/mainboard/google/glados/variants/chell/overridetree.cb
> > >
> > > chip drivers/i2c/generic
> > > register "hid" = ""ELAN0000""
> > > register "desc" = ""ELAN Touchpad""
> > > register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
> > > register "wake" = "GPE0_DW0_05"
> > > device i2c 15 on end
> > >
>
> So the above entry specifies the `wake` register. This generates an
> ACPI _PRW resource. The patch series will actually fix devices like
> this. Today without this patch series we get two wake events for a
> device. The ACPI wake GPE specified by the _PRW resource, and the
> erroneous GPIO wake event. But you bring up a good point.
Does this mean that the example that we currently have in coreboot
documentation (Documentation/acpi/devicetree.md) is not correct:
device pci 15.0 on
chip drivers/i2c/generic
register "hid" = ""ELAN0000""
register "desc" = ""ELAN Touchpad""
register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A21_IRQ)"
register "wake" = "GPE0_DW0_21"
device i2c 15 on end
end
end # I2C #0
Doesn't in say that we have both GpioIrq and GPE wakeup methods defined
for the same device?
>
> I wrote a quick and dirty script (https://0paste.com/391849) to parse
> the coreboot device tree entries. Open source firmware is great isn't
> it? ;)
>
> $ find src/mainboard/google/ -iname '*.cb' | xargs awk -f touch.awk --
> src/mainboard/google/eve/devicetree.cb
...
> src/mainboard/google/sarien/variants/sarien/devicetree.cb
> 1
> chip drivers/i2c/generic
> register "hid" = ""ELAN0000""
> register "desc" = ""ELAN Touchpad""
> register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
> register "probed" = "1"
> device i2c 2c on end
> end
> Total Touchpad: 202
> Total Wake: 195
>
> Out of all the touchpads defined on ChromeOS it looks like only 4
> devices are missing a wake declaration. I omitted touchpanels because
> ChromeOS doesn't use those as a wake source. chromeos_laptop.c already
> defines some devices with i2c board_info and it sets the
> `I2C_CLIENT_WAKE` flag. I'm not sure if this is actually working as
> expected. `i2c_device_probe` requires a `wakeup` irq to be present in
> the device tree if the `I2C_CLIENT_WAKE` flag is set, but I'm assuming
No it does not. If there is no wakeup IRQ defined of_irq_get_byname()
will return an error and we'll take the "else if (client->irq > 0)"
branch and will set up client->irq as the wakeup irq.
> the device tree was missing wake attributes.
>
> Anyway, patches 6, and 7 are the ones that drop the legacy behavior. I
> can figure out how to add the above boards to chromeos_laptop.c and
> get the wake attribute plumbed, or I can add something directly to the
> elan_i2c_core, etc so others can add overrides for their boards there.
> I'll also send out CLs to fix the device tree configs (not that we
> would run a FW qual just for this change).
My preference is to limit board-specific hacks in drivers if we can, so
adding missing properties to chromeos_laptop.c would be my preference.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq
2022-09-03 5:06 ` Dmitry Torokhov
@ 2022-09-06 17:18 ` Raul Rangel
2022-09-06 18:40 ` Dmitry Torokhov
0 siblings, 1 reply; 31+ messages in thread
From: Raul Rangel @ 2022-09-06 17:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rafael J. Wysocki, ACPI Devel Maling List, linux-input,
Hans de Goede, Mario Limonciello, Tim Van Patten, jingle.wu,
Linux Kernel Mailing List, Tony Lindgren
On Fri, Sep 2, 2022 at 11:07 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Wed, Aug 31, 2022 at 08:17:23PM -0600, Raul Rangel wrote:
> > On Wed, Aug 31, 2022 at 1:16 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote:
> > > > On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:
> > > > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > > > > >
> > > > > > The Elan I2C touchpad driver is currently manually managing the wake
> > > > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > > > > and instead relies on the PM subsystem. This is done by calling
> > > > > > dev_pm_set_wake_irq.
> > > > > >
> > > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > > > > tree, so it's only required when using ACPI. The net result is that this
> > > > > > change should be a no-op. i2c_device_remove also already calls
> > > > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > > > > >
> > > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > > > > defined. I verified I can still wake the system and that the wake source
> > > > > > was the touchpad IRQ GPIO.
> > > > > >
> > > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > > >
> > > > > I like this a lot [...]
> > > >
> >
> > > > I also like this a lot, but this assumes that firmware has correct
> > > > settings for the interrupt... Unfortunately it is not always the case
> > > > and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
> > > > ect) do not mark interrupt as wakeup:
> > > >
> > > > src/mainboard/google/glados/variants/chell/overridetree.cb
> > > >
> > > > chip drivers/i2c/generic
> > > > register "hid" = ""ELAN0000""
> > > > register "desc" = ""ELAN Touchpad""
> > > > register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
> > > > register "wake" = "GPE0_DW0_05"
> > > > device i2c 15 on end
> > > >
> >
> > So the above entry specifies the `wake` register. This generates an
> > ACPI _PRW resource. The patch series will actually fix devices like
> > this. Today without this patch series we get two wake events for a
> > device. The ACPI wake GPE specified by the _PRW resource, and the
> > erroneous GPIO wake event. But you bring up a good point.
>
> Does this mean that the example that we currently have in coreboot
> documentation (Documentation/acpi/devicetree.md) is not correct:
>
> device pci 15.0 on
> chip drivers/i2c/generic
> register "hid" = ""ELAN0000""
> register "desc" = ""ELAN Touchpad""
> register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A21_IRQ)"
> register "wake" = "GPE0_DW0_21"
> device i2c 15 on end
> end
> end # I2C #0
>
> Doesn't in say that we have both GpioIrq and GPE wakeup methods defined
> for the same device?
Hrmm, yeah that is wrong and will cause duplicate wake events for the
device. I'll push a CL to clean up the documentation.
>
> >
> > I wrote a quick and dirty script (https://0paste.com/391849) to parse
> > the coreboot device tree entries. Open source firmware is great isn't
> > it? ;)
> >
> > $ find src/mainboard/google/ -iname '*.cb' | xargs awk -f touch.awk --
> > src/mainboard/google/eve/devicetree.cb
>
> ...
>
> > src/mainboard/google/sarien/variants/sarien/devicetree.cb
> > 1
> > chip drivers/i2c/generic
> > register "hid" = ""ELAN0000""
> > register "desc" = ""ELAN Touchpad""
> > register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
> > register "probed" = "1"
> > device i2c 2c on end
> > end
> > Total Touchpad: 202
> > Total Wake: 195
> >
> > Out of all the touchpads defined on ChromeOS it looks like only 4
> > devices are missing a wake declaration. I omitted touchpanels because
> > ChromeOS doesn't use those as a wake source. chromeos_laptop.c already
> > defines some devices with i2c board_info and it sets the
> > `I2C_CLIENT_WAKE` flag. I'm not sure if this is actually working as
> > expected. `i2c_device_probe` requires a `wakeup` irq to be present in
> > the device tree if the `I2C_CLIENT_WAKE` flag is set, but I'm assuming
>
> No it does not. If there is no wakeup IRQ defined of_irq_get_byname()
> will return an error and we'll take the "else if (client->irq > 0)"
> branch and will set up client->irq as the wakeup irq.
>
> > the device tree was missing wake attributes.
Oh thanks for pointing that out. I might refactor patch #4 to just set
the `I2C_CLIENT_WAKE` flag when `acpi_wake_capable` is true.
>
> >
> > Anyway, patches 6, and 7 are the ones that drop the legacy behavior. I
> > can figure out how to add the above boards to chromeos_laptop.c and
> > get the wake attribute plumbed, or I can add something directly to the
> > elan_i2c_core, etc so others can add overrides for their boards there.
> > I'll also send out CLs to fix the device tree configs (not that we
> > would run a FW qual just for this change).
>
> My preference is to limit board-specific hacks in drivers if we can, so
> adding missing properties to chromeos_laptop.c would be my preference.
How should we handle non chromeos boards?
>
> Thanks.
>
> --
> Dmitry
Thanks!
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq
2022-09-06 17:18 ` Raul Rangel
@ 2022-09-06 18:40 ` Dmitry Torokhov
0 siblings, 0 replies; 31+ messages in thread
From: Dmitry Torokhov @ 2022-09-06 18:40 UTC (permalink / raw)
To: Raul Rangel
Cc: Rafael J. Wysocki, ACPI Devel Maling List, linux-input,
Hans de Goede, Mario Limonciello, Tim Van Patten, jingle.wu,
Linux Kernel Mailing List, Tony Lindgren
On Tue, Sep 06, 2022 at 11:18:49AM -0600, Raul Rangel wrote:
> On Fri, Sep 2, 2022 at 11:07 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Wed, Aug 31, 2022 at 08:17:23PM -0600, Raul Rangel wrote:
> > > On Wed, Aug 31, 2022 at 1:16 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote:
> > > > > On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > > > > > >
> > > > > > > The Elan I2C touchpad driver is currently manually managing the wake
> > > > > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > > > > > and instead relies on the PM subsystem. This is done by calling
> > > > > > > dev_pm_set_wake_irq.
> > > > > > >
> > > > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > > > > > tree, so it's only required when using ACPI. The net result is that this
> > > > > > > change should be a no-op. i2c_device_remove also already calls
> > > > > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > > > > > >
> > > > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > > > > > defined. I verified I can still wake the system and that the wake source
> > > > > > > was the touchpad IRQ GPIO.
> > > > > > >
> > > > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > > > >
> > > > > > I like this a lot [...]
> > > > >
> > >
> > > > > I also like this a lot, but this assumes that firmware has correct
> > > > > settings for the interrupt... Unfortunately it is not always the case
> > > > > and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
> > > > > ect) do not mark interrupt as wakeup:
> > > > >
> > > > > src/mainboard/google/glados/variants/chell/overridetree.cb
> > > > >
> > > > > chip drivers/i2c/generic
> > > > > register "hid" = ""ELAN0000""
> > > > > register "desc" = ""ELAN Touchpad""
> > > > > register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
> > > > > register "wake" = "GPE0_DW0_05"
> > > > > device i2c 15 on end
> > > > >
> > >
> > > So the above entry specifies the `wake` register. This generates an
> > > ACPI _PRW resource. The patch series will actually fix devices like
> > > this. Today without this patch series we get two wake events for a
> > > device. The ACPI wake GPE specified by the _PRW resource, and the
> > > erroneous GPIO wake event. But you bring up a good point.
> >
>
>
> > Does this mean that the example that we currently have in coreboot
> > documentation (Documentation/acpi/devicetree.md) is not correct:
> >
> > device pci 15.0 on
> > chip drivers/i2c/generic
> > register "hid" = ""ELAN0000""
> > register "desc" = ""ELAN Touchpad""
> > register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A21_IRQ)"
> > register "wake" = "GPE0_DW0_21"
> > device i2c 15 on end
> > end
> > end # I2C #0
> >
> > Doesn't in say that we have both GpioIrq and GPE wakeup methods defined
> > for the same device?
>
> Hrmm, yeah that is wrong and will cause duplicate wake events for the
> device. I'll push a CL to clean up the documentation.
Thanks. I think we also need to clean up our ADL boards (and likely
more).
>
> >
> > >
> > > I wrote a quick and dirty script (https://0paste.com/391849) to parse
> > > the coreboot device tree entries. Open source firmware is great isn't
> > > it? ;)
> > >
> > > $ find src/mainboard/google/ -iname '*.cb' | xargs awk -f touch.awk --
> > > src/mainboard/google/eve/devicetree.cb
> >
> > ...
> >
> > > src/mainboard/google/sarien/variants/sarien/devicetree.cb
> > > 1
> > > chip drivers/i2c/generic
> > > register "hid" = ""ELAN0000""
> > > register "desc" = ""ELAN Touchpad""
> > > register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
> > > register "probed" = "1"
> > > device i2c 2c on end
> > > end
> > > Total Touchpad: 202
> > > Total Wake: 195
> > >
> > > Out of all the touchpads defined on ChromeOS it looks like only 4
> > > devices are missing a wake declaration. I omitted touchpanels because
> > > ChromeOS doesn't use those as a wake source. chromeos_laptop.c already
> > > defines some devices with i2c board_info and it sets the
> > > `I2C_CLIENT_WAKE` flag. I'm not sure if this is actually working as
> > > expected. `i2c_device_probe` requires a `wakeup` irq to be present in
> > > the device tree if the `I2C_CLIENT_WAKE` flag is set, but I'm assuming
> >
> > No it does not. If there is no wakeup IRQ defined of_irq_get_byname()
> > will return an error and we'll take the "else if (client->irq > 0)"
> > branch and will set up client->irq as the wakeup irq.
> >
> > > the device tree was missing wake attributes.
>
> Oh thanks for pointing that out. I might refactor patch #4 to just set
> the `I2C_CLIENT_WAKE` flag when `acpi_wake_capable` is true.
>
> >
> > >
> > > Anyway, patches 6, and 7 are the ones that drop the legacy behavior. I
> > > can figure out how to add the above boards to chromeos_laptop.c and
> > > get the wake attribute plumbed, or I can add something directly to the
> > > elan_i2c_core, etc so others can add overrides for their boards there.
> > > I'll also send out CLs to fix the device tree configs (not that we
> > > would run a FW qual just for this change).
> >
> > My preference is to limit board-specific hacks in drivers if we can, so
> > adding missing properties to chromeos_laptop.c would be my preference.
>
> How should we handle non chromeos boards?
My preference would be to shove something like chromeos_laptop into
drivers/platform/x86... Something like
drivers/platform/x86/x86-android-tablets.c
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 2/8] HID: i2c-hid: Use PM subsystem to manage wake irq
2022-08-30 23:15 [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq Raul E Rangel
2022-08-30 23:15 ` [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage " Raul E Rangel
@ 2022-08-30 23:15 ` Raul E Rangel
2022-08-30 23:15 ` [PATCH 3/8] gpiolib: acpi: Add wake_capable parameter to acpi_dev_gpio_irq_get_by Raul E Rangel
` (6 subsequent siblings)
8 siblings, 0 replies; 31+ messages in thread
From: Raul E Rangel @ 2022-08-30 23:15 UTC (permalink / raw)
To: linux-acpi, linux-input
Cc: hdegoede, mario.limonciello, timvp, rafael, Raul E Rangel,
Angela Czubak, Bartosz Szczepanek, Benjamin Tissoires,
Dmitry Torokhov, Jiri Kosina, Yang Li, linux-kernel
The I2C hid driver is currently manually managing the wake
IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
and instead relies on the PM subsystem. This is done by calling
dev_pm_set_wake_irq.
i2c_device_probe already calls dev_pm_set_wake_irq when using device
tree, so it's only required when using ACPI. The net result is that this
change should be a no-op. i2c_device_remove also already calls
dev_pm_clear_wake_irq, so we don't need to do that in this driver.
I tested this on an ACPI system that has a HID touchscreen and verified
the IRQ was armed for wake on suspend.
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 33 +++++++++++-------------------
1 file changed, 12 insertions(+), 21 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index baa169fadd6632..0b7a1a8b3e9a33 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -26,6 +26,7 @@
#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/pm.h>
+#include <linux/pm_wakeirq.h>
#include <linux/device.h>
#include <linux/wait.h>
#include <linux/err.h>
@@ -116,7 +117,6 @@ struct i2c_hid {
wait_queue_head_t wait; /* For waiting the interrupt */
- bool irq_wake_enabled;
struct mutex reset_lock;
struct i2chid_ops *ops;
@@ -1036,6 +1036,15 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
if (ret < 0)
goto err_powered;
+ /*
+ * Systems using device tree should set up wakeup via DTS,
+ * the rest will configure device as wakeup source by default.
+ */
+ if (!client->dev.of_node) {
+ device_init_wakeup(&client->dev, true);
+ dev_pm_set_wake_irq(&client->dev, client->irq);
+ }
+
hid = hid_allocate_device();
if (IS_ERR(hid)) {
ret = PTR_ERR(hid);
@@ -1119,7 +1128,6 @@ static int i2c_hid_core_suspend(struct device *dev)
struct i2c_hid *ihid = i2c_get_clientdata(client);
struct hid_device *hid = ihid->hid;
int ret;
- int wake_status;
ret = hid_driver_suspend(hid, PMSG_SUSPEND);
if (ret < 0)
@@ -1130,16 +1138,8 @@ static int i2c_hid_core_suspend(struct device *dev)
disable_irq(client->irq);
- if (device_may_wakeup(&client->dev)) {
- wake_status = enable_irq_wake(client->irq);
- if (!wake_status)
- ihid->irq_wake_enabled = true;
- else
- hid_warn(hid, "Failed to enable irq wake: %d\n",
- wake_status);
- } else {
+ if (!device_may_wakeup(&client->dev))
i2c_hid_core_power_down(ihid);
- }
return 0;
}
@@ -1150,18 +1150,9 @@ static int i2c_hid_core_resume(struct device *dev)
struct i2c_client *client = to_i2c_client(dev);
struct i2c_hid *ihid = i2c_get_clientdata(client);
struct hid_device *hid = ihid->hid;
- int wake_status;
- if (!device_may_wakeup(&client->dev)) {
+ if (!device_may_wakeup(&client->dev))
i2c_hid_core_power_up(ihid);
- } else if (ihid->irq_wake_enabled) {
- wake_status = disable_irq_wake(client->irq);
- if (!wake_status)
- ihid->irq_wake_enabled = false;
- else
- hid_warn(hid, "Failed to disable irq wake: %d\n",
- wake_status);
- }
enable_irq(client->irq);
--
2.37.2.672.g94769d06f0-goog
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 3/8] gpiolib: acpi: Add wake_capable parameter to acpi_dev_gpio_irq_get_by
2022-08-30 23:15 [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq Raul E Rangel
2022-08-30 23:15 ` [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage " Raul E Rangel
2022-08-30 23:15 ` [PATCH 2/8] HID: i2c-hid: " Raul E Rangel
@ 2022-08-30 23:15 ` Raul E Rangel
2022-08-31 4:58 ` kernel test robot
2022-08-30 23:15 ` [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq Raul E Rangel
` (5 subsequent siblings)
8 siblings, 1 reply; 31+ messages in thread
From: Raul E Rangel @ 2022-08-30 23:15 UTC (permalink / raw)
To: linux-acpi, linux-input
Cc: hdegoede, mario.limonciello, timvp, rafael, Raul E Rangel,
Andy Shevchenko, Bartosz Golaszewski, Len Brown, Linus Walleij,
Mika Westerberg, linux-gpio, linux-kernel
The ACPI spec defines the SharedAndWake and ExclusiveAndWake share type
keywords. This is an indication that the GPIO IRQ can also be used as a
wake source. This change exposes the wake_capable bit so drivers can
correctly enable wake functionality instead of making an assumption.
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
drivers/gpio/gpio-pca953x.c | 3 ++-
drivers/gpio/gpiolib-acpi.c | 11 ++++++++++-
drivers/gpio/gpiolib-acpi.h | 2 ++
include/linux/acpi.h | 14 +++++++++++---
4 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index ecd7d169470b06..df02c3eb34a294 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -130,7 +130,8 @@ static int pca953x_acpi_get_irq(struct device *dev)
if (ret)
dev_warn(dev, "can't add GPIO ACPI mapping\n");
- ret = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(dev), "irq-gpios", 0);
+ ret = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(dev), "irq-gpios", 0,
+ NULL);
if (ret < 0)
return ret;
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 9be1376f9a627f..5cda2fcf7f43df 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -741,6 +741,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
lookup->info.pin_config = agpio->pin_config;
lookup->info.debounce = agpio->debounce_timeout;
lookup->info.gpioint = gpioint;
+ lookup->info.wake_capable = agpio->wake_capable;
/*
* Polarity and triggering are only specified for GpioInt
@@ -991,6 +992,7 @@ struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
* @adev: pointer to a ACPI device to get IRQ from
* @name: optional name of GpioInt resource
* @index: index of GpioInt resource (starting from %0)
+ * @wake_capable: Set to 1 if the IRQ is wake capable
*
* If the device has one or more GpioInt resources, this function can be
* used to translate from the GPIO offset in the resource to the Linux IRQ
@@ -1002,9 +1004,13 @@ struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
* The function takes optional @name parameter. If the resource has a property
* name, then only those will be taken into account.
*
+ * The GPIO is considered wake capable if GpioInt specifies SharedAndWake or
+ * ExclusiveAndWake.
+ *
* Return: Linux IRQ number (> %0) on success, negative errno on failure.
*/
-int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int index)
+int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name,
+ int index, int *wake_capable)
{
int idx, i;
unsigned int irq_flags;
@@ -1061,6 +1067,9 @@ int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int ind
dev_dbg(&adev->dev, "IRQ %d already in use\n", irq);
}
+ if (wake_capable)
+ *wake_capable = info.wake_capable;
+
return irq;
}
diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h
index e476558d947136..1ac6816839dbce 100644
--- a/drivers/gpio/gpiolib-acpi.h
+++ b/drivers/gpio/gpiolib-acpi.h
@@ -18,6 +18,7 @@ struct acpi_device;
* @pin_config: pin bias as provided by ACPI
* @polarity: interrupt polarity as provided by ACPI
* @triggering: triggering type as provided by ACPI
+ * @wake_capable: wake capability as provided by ACPI
* @debounce: debounce timeout as provided by ACPI
* @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping
*/
@@ -28,6 +29,7 @@ struct acpi_gpio_info {
int pin_config;
int polarity;
int triggering;
+ bool wake_capable;
unsigned int debounce;
unsigned int quirks;
};
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6f64b2f3dc5479..7ee946758c5bcc 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1202,7 +1202,8 @@ bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
struct acpi_resource_gpio **agpio);
bool acpi_gpio_get_io_resource(struct acpi_resource *ares,
struct acpi_resource_gpio **agpio);
-int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int index);
+int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name,
+ int index, int *wake_capable);
#else
static inline bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
struct acpi_resource_gpio **agpio)
@@ -1215,7 +1216,8 @@ static inline bool acpi_gpio_get_io_resource(struct acpi_resource *ares,
return false;
}
static inline int acpi_dev_gpio_irq_get_by(struct acpi_device *adev,
- const char *name, int index)
+ const char *name, int index,
+ int *wake_capable)
{
return -ENXIO;
}
@@ -1223,7 +1225,13 @@ static inline int acpi_dev_gpio_irq_get_by(struct acpi_device *adev,
static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
{
- return acpi_dev_gpio_irq_get_by(adev, NULL, index);
+ return acpi_dev_gpio_irq_get_by(adev, NULL, index, NULL);
+}
+
+static inline int acpi_dev_gpio_irq_get_wake(struct acpi_device *adev,
+ int index, int *wake_capable)
+{
+ return acpi_dev_gpio_irq_get_by(adev, NULL, index, wake_capable);
}
/* Device properties */
--
2.37.2.672.g94769d06f0-goog
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 3/8] gpiolib: acpi: Add wake_capable parameter to acpi_dev_gpio_irq_get_by
2022-08-30 23:15 ` [PATCH 3/8] gpiolib: acpi: Add wake_capable parameter to acpi_dev_gpio_irq_get_by Raul E Rangel
@ 2022-08-31 4:58 ` kernel test robot
0 siblings, 0 replies; 31+ messages in thread
From: kernel test robot @ 2022-08-31 4:58 UTC (permalink / raw)
To: Raul E Rangel, linux-acpi, linux-input
Cc: kbuild-all, hdegoede, mario.limonciello, timvp, rafael,
Raul E Rangel, Andy Shevchenko, Bartosz Golaszewski, Len Brown,
Linus Walleij, Mika Westerberg, linux-gpio, linux-kernel
Hi Raul,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on hid/for-next]
[also build test ERROR on brgl/gpio/for-next rafael-pm/linux-next linus/master v6.0-rc3 next-20220830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Raul-E-Rangel/acpi-i2c-Use-SharedAndWake-and-ExclusiveAndWake-to-enable-wake-irq/20220831-071916
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20220831/202208311235.pxWOoHPE-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/131a07025a591f4ca6d7540f1055bc03f8cf64af
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Raul-E-Rangel/acpi-i2c-Use-SharedAndWake-and-ExclusiveAndWake-to-enable-wake-irq/20220831-071916
git checkout 131a07025a591f4ca6d7540f1055bc03f8cf64af
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash drivers/net/ethernet/mellanox/mlxbf_gige/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c: In function 'mlxbf_gige_probe':
>> drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c:343:19: error: too few arguments to function 'acpi_dev_gpio_irq_get_by'
343 | phy_irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(&pdev->dev), "phy-gpios", 0);
| ^~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c:8:
include/linux/acpi.h:1212:19: note: declared here
1212 | static inline int acpi_dev_gpio_irq_get_by(struct acpi_device *adev,
| ^~~~~~~~~~~~~~~~~~~~~~~~
vim +/acpi_dev_gpio_irq_get_by +343 drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
f92e1869d74e1a David Thompson 2021-06-24 272
f92e1869d74e1a David Thompson 2021-06-24 273 static int mlxbf_gige_probe(struct platform_device *pdev)
f92e1869d74e1a David Thompson 2021-06-24 274 {
f92e1869d74e1a David Thompson 2021-06-24 275 struct phy_device *phydev;
f92e1869d74e1a David Thompson 2021-06-24 276 struct net_device *netdev;
f92e1869d74e1a David Thompson 2021-06-24 277 struct mlxbf_gige *priv;
f92e1869d74e1a David Thompson 2021-06-24 278 void __iomem *llu_base;
f92e1869d74e1a David Thompson 2021-06-24 279 void __iomem *plu_base;
f92e1869d74e1a David Thompson 2021-06-24 280 void __iomem *base;
6c2a6ddca76327 Asmaa Mnebhi 2021-10-15 281 int addr, phy_irq;
f92e1869d74e1a David Thompson 2021-06-24 282 u64 control;
f92e1869d74e1a David Thompson 2021-06-24 283 int err;
f92e1869d74e1a David Thompson 2021-06-24 284
464a57281f29af Cai Huoqing 2021-08-31 285 base = devm_platform_ioremap_resource(pdev, MLXBF_GIGE_RES_MAC);
f92e1869d74e1a David Thompson 2021-06-24 286 if (IS_ERR(base))
f92e1869d74e1a David Thompson 2021-06-24 287 return PTR_ERR(base);
f92e1869d74e1a David Thompson 2021-06-24 288
464a57281f29af Cai Huoqing 2021-08-31 289 llu_base = devm_platform_ioremap_resource(pdev, MLXBF_GIGE_RES_LLU);
f92e1869d74e1a David Thompson 2021-06-24 290 if (IS_ERR(llu_base))
f92e1869d74e1a David Thompson 2021-06-24 291 return PTR_ERR(llu_base);
f92e1869d74e1a David Thompson 2021-06-24 292
464a57281f29af Cai Huoqing 2021-08-31 293 plu_base = devm_platform_ioremap_resource(pdev, MLXBF_GIGE_RES_PLU);
f92e1869d74e1a David Thompson 2021-06-24 294 if (IS_ERR(plu_base))
f92e1869d74e1a David Thompson 2021-06-24 295 return PTR_ERR(plu_base);
f92e1869d74e1a David Thompson 2021-06-24 296
f92e1869d74e1a David Thompson 2021-06-24 297 /* Perform general init of GigE block */
f92e1869d74e1a David Thompson 2021-06-24 298 control = readq(base + MLXBF_GIGE_CONTROL);
f92e1869d74e1a David Thompson 2021-06-24 299 control |= MLXBF_GIGE_CONTROL_PORT_EN;
f92e1869d74e1a David Thompson 2021-06-24 300 writeq(control, base + MLXBF_GIGE_CONTROL);
f92e1869d74e1a David Thompson 2021-06-24 301
f92e1869d74e1a David Thompson 2021-06-24 302 netdev = devm_alloc_etherdev(&pdev->dev, sizeof(*priv));
f92e1869d74e1a David Thompson 2021-06-24 303 if (!netdev)
f92e1869d74e1a David Thompson 2021-06-24 304 return -ENOMEM;
f92e1869d74e1a David Thompson 2021-06-24 305
f92e1869d74e1a David Thompson 2021-06-24 306 SET_NETDEV_DEV(netdev, &pdev->dev);
f92e1869d74e1a David Thompson 2021-06-24 307 netdev->netdev_ops = &mlxbf_gige_netdev_ops;
f92e1869d74e1a David Thompson 2021-06-24 308 netdev->ethtool_ops = &mlxbf_gige_ethtool_ops;
f92e1869d74e1a David Thompson 2021-06-24 309 priv = netdev_priv(netdev);
f92e1869d74e1a David Thompson 2021-06-24 310 priv->netdev = netdev;
f92e1869d74e1a David Thompson 2021-06-24 311
f92e1869d74e1a David Thompson 2021-06-24 312 platform_set_drvdata(pdev, priv);
f92e1869d74e1a David Thompson 2021-06-24 313 priv->dev = &pdev->dev;
f92e1869d74e1a David Thompson 2021-06-24 314 priv->pdev = pdev;
f92e1869d74e1a David Thompson 2021-06-24 315
f92e1869d74e1a David Thompson 2021-06-24 316 spin_lock_init(&priv->lock);
f92e1869d74e1a David Thompson 2021-06-24 317
f92e1869d74e1a David Thompson 2021-06-24 318 /* Attach MDIO device */
f92e1869d74e1a David Thompson 2021-06-24 319 err = mlxbf_gige_mdio_probe(pdev, priv);
f92e1869d74e1a David Thompson 2021-06-24 320 if (err)
f92e1869d74e1a David Thompson 2021-06-24 321 return err;
f92e1869d74e1a David Thompson 2021-06-24 322
f92e1869d74e1a David Thompson 2021-06-24 323 priv->base = base;
f92e1869d74e1a David Thompson 2021-06-24 324 priv->llu_base = llu_base;
f92e1869d74e1a David Thompson 2021-06-24 325 priv->plu_base = plu_base;
f92e1869d74e1a David Thompson 2021-06-24 326
f92e1869d74e1a David Thompson 2021-06-24 327 priv->rx_q_entries = MLXBF_GIGE_DEFAULT_RXQ_SZ;
f92e1869d74e1a David Thompson 2021-06-24 328 priv->tx_q_entries = MLXBF_GIGE_DEFAULT_TXQ_SZ;
f92e1869d74e1a David Thompson 2021-06-24 329
f92e1869d74e1a David Thompson 2021-06-24 330 /* Write initial MAC address to hardware */
f92e1869d74e1a David Thompson 2021-06-24 331 mlxbf_gige_initial_mac(priv);
f92e1869d74e1a David Thompson 2021-06-24 332
f92e1869d74e1a David Thompson 2021-06-24 333 err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
f92e1869d74e1a David Thompson 2021-06-24 334 if (err) {
f92e1869d74e1a David Thompson 2021-06-24 335 dev_err(&pdev->dev, "DMA configuration failed: 0x%x\n", err);
f92e1869d74e1a David Thompson 2021-06-24 336 goto out;
f92e1869d74e1a David Thompson 2021-06-24 337 }
f92e1869d74e1a David Thompson 2021-06-24 338
f92e1869d74e1a David Thompson 2021-06-24 339 priv->error_irq = platform_get_irq(pdev, MLXBF_GIGE_ERROR_INTR_IDX);
f92e1869d74e1a David Thompson 2021-06-24 340 priv->rx_irq = platform_get_irq(pdev, MLXBF_GIGE_RECEIVE_PKT_INTR_IDX);
f92e1869d74e1a David Thompson 2021-06-24 341 priv->llu_plu_irq = platform_get_irq(pdev, MLXBF_GIGE_LLU_PLU_INTR_IDX);
f92e1869d74e1a David Thompson 2021-06-24 342
6c2a6ddca76327 Asmaa Mnebhi 2021-10-15 @343 phy_irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(&pdev->dev), "phy-gpios", 0);
6c2a6ddca76327 Asmaa Mnebhi 2021-10-15 344 if (phy_irq < 0) {
6c2a6ddca76327 Asmaa Mnebhi 2021-10-15 345 dev_err(&pdev->dev, "Error getting PHY irq. Use polling instead");
6c2a6ddca76327 Asmaa Mnebhi 2021-10-15 346 phy_irq = PHY_POLL;
6c2a6ddca76327 Asmaa Mnebhi 2021-10-15 347 }
6c2a6ddca76327 Asmaa Mnebhi 2021-10-15 348
f92e1869d74e1a David Thompson 2021-06-24 349 phydev = phy_find_first(priv->mdiobus);
f92e1869d74e1a David Thompson 2021-06-24 350 if (!phydev) {
f92e1869d74e1a David Thompson 2021-06-24 351 err = -ENODEV;
f92e1869d74e1a David Thompson 2021-06-24 352 goto out;
f92e1869d74e1a David Thompson 2021-06-24 353 }
f92e1869d74e1a David Thompson 2021-06-24 354
f92e1869d74e1a David Thompson 2021-06-24 355 addr = phydev->mdio.addr;
6c2a6ddca76327 Asmaa Mnebhi 2021-10-15 356 priv->mdiobus->irq[addr] = phy_irq;
6c2a6ddca76327 Asmaa Mnebhi 2021-10-15 357 phydev->irq = phy_irq;
f92e1869d74e1a David Thompson 2021-06-24 358
f92e1869d74e1a David Thompson 2021-06-24 359 err = phy_connect_direct(netdev, phydev,
f92e1869d74e1a David Thompson 2021-06-24 360 mlxbf_gige_adjust_link,
f92e1869d74e1a David Thompson 2021-06-24 361 PHY_INTERFACE_MODE_GMII);
f92e1869d74e1a David Thompson 2021-06-24 362 if (err) {
f92e1869d74e1a David Thompson 2021-06-24 363 dev_err(&pdev->dev, "Could not attach to PHY\n");
f92e1869d74e1a David Thompson 2021-06-24 364 goto out;
f92e1869d74e1a David Thompson 2021-06-24 365 }
f92e1869d74e1a David Thompson 2021-06-24 366
f92e1869d74e1a David Thompson 2021-06-24 367 /* MAC only supports 1000T full duplex mode */
f92e1869d74e1a David Thompson 2021-06-24 368 phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
f92e1869d74e1a David Thompson 2021-06-24 369 phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Full_BIT);
f92e1869d74e1a David Thompson 2021-06-24 370 phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
f92e1869d74e1a David Thompson 2021-06-24 371 phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
f92e1869d74e1a David Thompson 2021-06-24 372 phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
f92e1869d74e1a David Thompson 2021-06-24 373
f92e1869d74e1a David Thompson 2021-06-24 374 /* Only symmetric pause with flow control enabled is supported so no
f92e1869d74e1a David Thompson 2021-06-24 375 * need to negotiate pause.
f92e1869d74e1a David Thompson 2021-06-24 376 */
f92e1869d74e1a David Thompson 2021-06-24 377 linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->advertising);
f92e1869d74e1a David Thompson 2021-06-24 378 linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->advertising);
f92e1869d74e1a David Thompson 2021-06-24 379
f92e1869d74e1a David Thompson 2021-06-24 380 /* Display information about attached PHY device */
f92e1869d74e1a David Thompson 2021-06-24 381 phy_attached_info(phydev);
f92e1869d74e1a David Thompson 2021-06-24 382
f92e1869d74e1a David Thompson 2021-06-24 383 err = register_netdev(netdev);
f92e1869d74e1a David Thompson 2021-06-24 384 if (err) {
f92e1869d74e1a David Thompson 2021-06-24 385 dev_err(&pdev->dev, "Failed to register netdev\n");
f92e1869d74e1a David Thompson 2021-06-24 386 phy_disconnect(phydev);
f92e1869d74e1a David Thompson 2021-06-24 387 goto out;
f92e1869d74e1a David Thompson 2021-06-24 388 }
f92e1869d74e1a David Thompson 2021-06-24 389
f92e1869d74e1a David Thompson 2021-06-24 390 return 0;
f92e1869d74e1a David Thompson 2021-06-24 391
f92e1869d74e1a David Thompson 2021-06-24 392 out:
f92e1869d74e1a David Thompson 2021-06-24 393 mlxbf_gige_mdio_remove(priv);
f92e1869d74e1a David Thompson 2021-06-24 394 return err;
f92e1869d74e1a David Thompson 2021-06-24 395 }
f92e1869d74e1a David Thompson 2021-06-24 396
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq
2022-08-30 23:15 [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq Raul E Rangel
` (2 preceding siblings ...)
2022-08-30 23:15 ` [PATCH 3/8] gpiolib: acpi: Add wake_capable parameter to acpi_dev_gpio_irq_get_by Raul E Rangel
@ 2022-08-30 23:15 ` Raul E Rangel
2022-09-07 1:00 ` Dmitry Torokhov
2022-08-30 23:15 ` [PATCH 5/8] HID: i2c-hid: acpi: Stop setting wakeup_capable Raul E Rangel
` (4 subsequent siblings)
8 siblings, 1 reply; 31+ messages in thread
From: Raul E Rangel @ 2022-08-30 23:15 UTC (permalink / raw)
To: linux-acpi, linux-input
Cc: hdegoede, mario.limonciello, timvp, rafael, Raul E Rangel,
Mika Westerberg, Wolfram Sang, linux-i2c, linux-kernel
Device tree already has a mechanism to pass the wake_irq. It does this
by looking for the wakeup-source property and setting the
I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
ACPI GpioInt wake flag to determine if the interrupt can be used to wake
the system. Previously the i2c drivers had to make assumptions and
blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
If there is a device with an Active Low interrupt and the device gets
powered off while suspending, the interrupt line will go low since it's
no longer powered and wake the system. For this reason we should respect
the board designers wishes and honor the wake bit defined on the
GpioInt.
This change does not cover the ACPI Interrupt or IRQ resources.
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
drivers/i2c/i2c-core-acpi.c | 8 ++++++--
drivers/i2c/i2c-core-base.c | 17 +++++++++++------
drivers/i2c/i2c-core.h | 4 ++--
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index c762a879c4cc6b..cfe82a6ba3ef28 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
/**
* i2c_acpi_get_irq - get device IRQ number from ACPI
* @client: Pointer to the I2C client device
+ * @wake_capable: Set to 1 if the IRQ is wake capable
*
* Find the IRQ number used by a specific client device.
*
* Return: The IRQ number or an error code.
*/
-int i2c_acpi_get_irq(struct i2c_client *client)
+int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
{
struct acpi_device *adev = ACPI_COMPANION(&client->dev);
struct list_head resource_list;
@@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
INIT_LIST_HEAD(&resource_list);
+ if (wake_capable)
+ *wake_capable = 0;
+
ret = acpi_dev_get_resources(adev, &resource_list,
i2c_acpi_add_resource, &irq);
if (ret < 0)
@@ -204,7 +208,7 @@ int i2c_acpi_get_irq(struct i2c_client *client)
acpi_dev_free_resource_list(&resource_list);
if (irq == -ENOENT)
- irq = acpi_dev_gpio_irq_get(adev, 0);
+ irq = acpi_dev_gpio_irq_get_wake(adev, 0, wake_capable);
return irq;
}
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 91007558bcb260..88f4ef76235f4e 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -468,6 +468,7 @@ static int i2c_device_probe(struct device *dev)
struct i2c_client *client = i2c_verify_client(dev);
struct i2c_driver *driver;
int status;
+ int acpi_wake_capable = 0;
if (!client)
return 0;
@@ -487,7 +488,7 @@ static int i2c_device_probe(struct device *dev)
if (irq == -EINVAL || irq == -ENODATA)
irq = of_irq_get(dev->of_node, 0);
} else if (ACPI_COMPANION(dev)) {
- irq = i2c_acpi_get_irq(client);
+ irq = i2c_acpi_get_irq(client, &acpi_wake_capable);
}
if (irq == -EPROBE_DEFER) {
status = irq;
@@ -513,13 +514,17 @@ static int i2c_device_probe(struct device *dev)
goto put_sync_adapter;
}
- if (client->flags & I2C_CLIENT_WAKE) {
+ if (client->flags & I2C_CLIENT_WAKE || acpi_wake_capable) {
int wakeirq;
- wakeirq = of_irq_get_byname(dev->of_node, "wakeup");
- if (wakeirq == -EPROBE_DEFER) {
- status = wakeirq;
- goto put_sync_adapter;
+ if (acpi_wake_capable) {
+ wakeirq = client->irq;
+ } else {
+ wakeirq = of_irq_get_byname(dev->of_node, "wakeup");
+ if (wakeirq == -EPROBE_DEFER) {
+ status = wakeirq;
+ goto put_sync_adapter;
+ }
}
device_init_wakeup(&client->dev, true);
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 87e2c914f1c57b..8e336638a0cd2e 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -61,11 +61,11 @@ static inline int __i2c_check_suspended(struct i2c_adapter *adap)
#ifdef CONFIG_ACPI
void i2c_acpi_register_devices(struct i2c_adapter *adap);
-int i2c_acpi_get_irq(struct i2c_client *client);
+int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable);
#else /* CONFIG_ACPI */
static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { }
-static inline int i2c_acpi_get_irq(struct i2c_client *client)
+static inline int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
{
return 0;
}
--
2.37.2.672.g94769d06f0-goog
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq
2022-08-30 23:15 ` [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq Raul E Rangel
@ 2022-09-07 1:00 ` Dmitry Torokhov
2022-09-07 2:00 ` Raul Rangel
0 siblings, 1 reply; 31+ messages in thread
From: Dmitry Torokhov @ 2022-09-07 1:00 UTC (permalink / raw)
To: Raul E Rangel
Cc: linux-acpi, linux-input, hdegoede, mario.limonciello, timvp,
rafael, Mika Westerberg, Wolfram Sang, linux-i2c, linux-kernel
On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
> Device tree already has a mechanism to pass the wake_irq. It does this
> by looking for the wakeup-source property and setting the
> I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
> ACPI GpioInt wake flag to determine if the interrupt can be used to wake
> the system. Previously the i2c drivers had to make assumptions and
> blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
> If there is a device with an Active Low interrupt and the device gets
> powered off while suspending, the interrupt line will go low since it's
> no longer powered and wake the system. For this reason we should respect
> the board designers wishes and honor the wake bit defined on the
> GpioInt.
>
> This change does not cover the ACPI Interrupt or IRQ resources.
>
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
>
> drivers/i2c/i2c-core-acpi.c | 8 ++++++--
> drivers/i2c/i2c-core-base.c | 17 +++++++++++------
> drivers/i2c/i2c-core.h | 4 ++--
> 3 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index c762a879c4cc6b..cfe82a6ba3ef28 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
> /**
> * i2c_acpi_get_irq - get device IRQ number from ACPI
> * @client: Pointer to the I2C client device
> + * @wake_capable: Set to 1 if the IRQ is wake capable
> *
> * Find the IRQ number used by a specific client device.
> *
> * Return: The IRQ number or an error code.
> */
> -int i2c_acpi_get_irq(struct i2c_client *client)
> +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
> {
> struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> struct list_head resource_list;
> @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
>
> INIT_LIST_HEAD(&resource_list);
>
> + if (wake_capable)
> + *wake_capable = 0;
> +
> ret = acpi_dev_get_resources(adev, &resource_list,
> i2c_acpi_add_resource, &irq);
You also need to handle "Interrupt(..., ...AndWake)" case here. I would
look into maybe defining
#define IORESOURCE_IRQ_WAKECAPABLE (1<<6)
in include/linux/ioport.h and plumbing it through from ACPI layer.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq
2022-09-07 1:00 ` Dmitry Torokhov
@ 2022-09-07 2:00 ` Raul Rangel
2022-09-07 2:04 ` Dmitry Torokhov
2022-09-07 8:12 ` Hans de Goede
0 siblings, 2 replies; 31+ messages in thread
From: Raul Rangel @ 2022-09-07 2:00 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux ACPI, linux-input, Hans de Goede, Limonciello, Mario,
Tim Van Patten, Rafael J. Wysocki, Mika Westerberg, Wolfram Sang,
open list:I2C SUBSYSTEM HOST DRIVERS, linux-kernel
On Tue, Sep 6, 2022 at 7:00 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
> > Device tree already has a mechanism to pass the wake_irq. It does this
> > by looking for the wakeup-source property and setting the
> > I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
> > ACPI GpioInt wake flag to determine if the interrupt can be used to wake
> > the system. Previously the i2c drivers had to make assumptions and
> > blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
> > If there is a device with an Active Low interrupt and the device gets
> > powered off while suspending, the interrupt line will go low since it's
> > no longer powered and wake the system. For this reason we should respect
> > the board designers wishes and honor the wake bit defined on the
> > GpioInt.
> >
> > This change does not cover the ACPI Interrupt or IRQ resources.
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > ---
> >
> > drivers/i2c/i2c-core-acpi.c | 8 ++++++--
> > drivers/i2c/i2c-core-base.c | 17 +++++++++++------
> > drivers/i2c/i2c-core.h | 4 ++--
> > 3 files changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> > index c762a879c4cc6b..cfe82a6ba3ef28 100644
> > --- a/drivers/i2c/i2c-core-acpi.c
> > +++ b/drivers/i2c/i2c-core-acpi.c
> > @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
> > /**
> > * i2c_acpi_get_irq - get device IRQ number from ACPI
> > * @client: Pointer to the I2C client device
> > + * @wake_capable: Set to 1 if the IRQ is wake capable
> > *
> > * Find the IRQ number used by a specific client device.
> > *
> > * Return: The IRQ number or an error code.
> > */
> > -int i2c_acpi_get_irq(struct i2c_client *client)
> > +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
> > {
> > struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> > struct list_head resource_list;
> > @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
> >
> > INIT_LIST_HEAD(&resource_list);
> >
> > + if (wake_capable)
> > + *wake_capable = 0;
> > +
> > ret = acpi_dev_get_resources(adev, &resource_list,
> > i2c_acpi_add_resource, &irq);
>
> You also need to handle "Interrupt(..., ...AndWake)" case here. I would
> look into maybe defining
>
> #define IORESOURCE_IRQ_WAKECAPABLE (1<<6)
>
> in include/linux/ioport.h and plumbing it through from ACPI layer.
>
> Thanks.
AFAIK the Intel (Not 100% certain) and AMD IO-APIC's can't actually
wake a system from suspend/suspend-to-idle. It requires either a GPE
or GPIO controller to wake the system. This is the reason I haven't
pushed patches to handle the Interrupt/IRQ resource. Can anyone
confirm?
Thanks
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq
2022-09-07 2:00 ` Raul Rangel
@ 2022-09-07 2:04 ` Dmitry Torokhov
2022-09-07 8:12 ` Hans de Goede
1 sibling, 0 replies; 31+ messages in thread
From: Dmitry Torokhov @ 2022-09-07 2:04 UTC (permalink / raw)
To: Raul Rangel
Cc: Linux ACPI, linux-input, Hans de Goede, Limonciello, Mario,
Tim Van Patten, Rafael J. Wysocki, Mika Westerberg, Wolfram Sang,
open list:I2C SUBSYSTEM HOST DRIVERS, linux-kernel
On Tue, Sep 06, 2022 at 08:00:00PM -0600, Raul Rangel wrote:
> On Tue, Sep 6, 2022 at 7:00 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
> > > Device tree already has a mechanism to pass the wake_irq. It does this
> > > by looking for the wakeup-source property and setting the
> > > I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
> > > ACPI GpioInt wake flag to determine if the interrupt can be used to wake
> > > the system. Previously the i2c drivers had to make assumptions and
> > > blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
> > > If there is a device with an Active Low interrupt and the device gets
> > > powered off while suspending, the interrupt line will go low since it's
> > > no longer powered and wake the system. For this reason we should respect
> > > the board designers wishes and honor the wake bit defined on the
> > > GpioInt.
> > >
> > > This change does not cover the ACPI Interrupt or IRQ resources.
> > >
> > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > ---
> > >
> > > drivers/i2c/i2c-core-acpi.c | 8 ++++++--
> > > drivers/i2c/i2c-core-base.c | 17 +++++++++++------
> > > drivers/i2c/i2c-core.h | 4 ++--
> > > 3 files changed, 19 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> > > index c762a879c4cc6b..cfe82a6ba3ef28 100644
> > > --- a/drivers/i2c/i2c-core-acpi.c
> > > +++ b/drivers/i2c/i2c-core-acpi.c
> > > @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
> > > /**
> > > * i2c_acpi_get_irq - get device IRQ number from ACPI
> > > * @client: Pointer to the I2C client device
> > > + * @wake_capable: Set to 1 if the IRQ is wake capable
> > > *
> > > * Find the IRQ number used by a specific client device.
> > > *
> > > * Return: The IRQ number or an error code.
> > > */
> > > -int i2c_acpi_get_irq(struct i2c_client *client)
> > > +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
> > > {
> > > struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> > > struct list_head resource_list;
> > > @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
> > >
> > > INIT_LIST_HEAD(&resource_list);
> > >
> > > + if (wake_capable)
> > > + *wake_capable = 0;
> > > +
> > > ret = acpi_dev_get_resources(adev, &resource_list,
> > > i2c_acpi_add_resource, &irq);
> >
>
>
> > You also need to handle "Interrupt(..., ...AndWake)" case here. I would
> > look into maybe defining
> >
> > #define IORESOURCE_IRQ_WAKECAPABLE (1<<6)
> >
> > in include/linux/ioport.h and plumbing it through from ACPI layer.
> >
> > Thanks.
>
> AFAIK the Intel (Not 100% certain) and AMD IO-APIC's can't actually
> wake a system from suspend/suspend-to-idle. It requires either a GPE
> or GPIO controller to wake the system. This is the reason I haven't
> pushed patches to handle the Interrupt/IRQ resource. Can anyone
> confirm?
I've heard there are ARM ACPI systems...
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq
2022-09-07 2:00 ` Raul Rangel
2022-09-07 2:04 ` Dmitry Torokhov
@ 2022-09-07 8:12 ` Hans de Goede
2022-09-08 14:40 ` Raul Rangel
1 sibling, 1 reply; 31+ messages in thread
From: Hans de Goede @ 2022-09-07 8:12 UTC (permalink / raw)
To: Raul Rangel, Dmitry Torokhov
Cc: Linux ACPI, linux-input, Limonciello, Mario, Tim Van Patten,
Rafael J. Wysocki, Mika Westerberg, Wolfram Sang,
open list:I2C SUBSYSTEM HOST DRIVERS, linux-kernel
Hi,
On 9/7/22 04:00, Raul Rangel wrote:
> On Tue, Sep 6, 2022 at 7:00 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>>
>> On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
>>> Device tree already has a mechanism to pass the wake_irq. It does this
>>> by looking for the wakeup-source property and setting the
>>> I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
>>> ACPI GpioInt wake flag to determine if the interrupt can be used to wake
>>> the system. Previously the i2c drivers had to make assumptions and
>>> blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
>>> If there is a device with an Active Low interrupt and the device gets
>>> powered off while suspending, the interrupt line will go low since it's
>>> no longer powered and wake the system. For this reason we should respect
>>> the board designers wishes and honor the wake bit defined on the
>>> GpioInt.
>>>
>>> This change does not cover the ACPI Interrupt or IRQ resources.
>>>
>>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
>>> ---
>>>
>>> drivers/i2c/i2c-core-acpi.c | 8 ++++++--
>>> drivers/i2c/i2c-core-base.c | 17 +++++++++++------
>>> drivers/i2c/i2c-core.h | 4 ++--
>>> 3 files changed, 19 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
>>> index c762a879c4cc6b..cfe82a6ba3ef28 100644
>>> --- a/drivers/i2c/i2c-core-acpi.c
>>> +++ b/drivers/i2c/i2c-core-acpi.c
>>> @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
>>> /**
>>> * i2c_acpi_get_irq - get device IRQ number from ACPI
>>> * @client: Pointer to the I2C client device
>>> + * @wake_capable: Set to 1 if the IRQ is wake capable
>>> *
>>> * Find the IRQ number used by a specific client device.
>>> *
>>> * Return: The IRQ number or an error code.
>>> */
>>> -int i2c_acpi_get_irq(struct i2c_client *client)
>>> +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
>>> {
>>> struct acpi_device *adev = ACPI_COMPANION(&client->dev);
>>> struct list_head resource_list;
>>> @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
>>>
>>> INIT_LIST_HEAD(&resource_list);
>>>
>>> + if (wake_capable)
>>> + *wake_capable = 0;
>>> +
>>> ret = acpi_dev_get_resources(adev, &resource_list,
>>> i2c_acpi_add_resource, &irq);
>>
>
>
>> You also need to handle "Interrupt(..., ...AndWake)" case here. I would
>> look into maybe defining
>>
>> #define IORESOURCE_IRQ_WAKECAPABLE (1<<6)
>>
>> in include/linux/ioport.h and plumbing it through from ACPI layer.
>>
>> Thanks.
>
> AFAIK the Intel (Not 100% certain) and AMD IO-APIC's can't actually
> wake a system from suspend/suspend-to-idle.
That may be true for S3 suspend (it sounds about right) there
certainly is no way to "arm for wakeup" on the APIC, but with
s2idle all IRQs which are not explicitly disabled by the OS
still function normally so there any IRQ can be a wakeup
source (AFAIK).
And even with S3 suspend I think some IRQs can act as wakeup,
but that is configured by the BIOS then and not something which
linux can enable/disable. E.g IIRC the parent IRQ of the GPIO
controllers on x86 is an APIC IRQ ...
Regards,
Hans
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq
2022-09-07 8:12 ` Hans de Goede
@ 2022-09-08 14:40 ` Raul Rangel
2022-09-08 15:23 ` Rafael J. Wysocki
0 siblings, 1 reply; 31+ messages in thread
From: Raul Rangel @ 2022-09-08 14:40 UTC (permalink / raw)
To: Hans de Goede
Cc: Dmitry Torokhov, Linux ACPI, linux-input, Limonciello, Mario,
Tim Van Patten, Rafael J. Wysocki, Mika Westerberg, Wolfram Sang,
open list:I2C SUBSYSTEM HOST DRIVERS, linux-kernel
On Wed, Sep 7, 2022 at 2:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 9/7/22 04:00, Raul Rangel wrote:
> > On Tue, Sep 6, 2022 at 7:00 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >>
> >> On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
> >>> Device tree already has a mechanism to pass the wake_irq. It does this
> >>> by looking for the wakeup-source property and setting the
> >>> I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
> >>> ACPI GpioInt wake flag to determine if the interrupt can be used to wake
> >>> the system. Previously the i2c drivers had to make assumptions and
> >>> blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
> >>> If there is a device with an Active Low interrupt and the device gets
> >>> powered off while suspending, the interrupt line will go low since it's
> >>> no longer powered and wake the system. For this reason we should respect
> >>> the board designers wishes and honor the wake bit defined on the
> >>> GpioInt.
> >>>
> >>> This change does not cover the ACPI Interrupt or IRQ resources.
> >>>
> >>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> >>> ---
> >>>
> >>> drivers/i2c/i2c-core-acpi.c | 8 ++++++--
> >>> drivers/i2c/i2c-core-base.c | 17 +++++++++++------
> >>> drivers/i2c/i2c-core.h | 4 ++--
> >>> 3 files changed, 19 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> >>> index c762a879c4cc6b..cfe82a6ba3ef28 100644
> >>> --- a/drivers/i2c/i2c-core-acpi.c
> >>> +++ b/drivers/i2c/i2c-core-acpi.c
> >>> @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
> >>> /**
> >>> * i2c_acpi_get_irq - get device IRQ number from ACPI
> >>> * @client: Pointer to the I2C client device
> >>> + * @wake_capable: Set to 1 if the IRQ is wake capable
> >>> *
> >>> * Find the IRQ number used by a specific client device.
> >>> *
> >>> * Return: The IRQ number or an error code.
> >>> */
> >>> -int i2c_acpi_get_irq(struct i2c_client *client)
> >>> +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
> >>> {
> >>> struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> >>> struct list_head resource_list;
> >>> @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
> >>>
> >>> INIT_LIST_HEAD(&resource_list);
> >>>
> >>> + if (wake_capable)
> >>> + *wake_capable = 0;
> >>> +
> >>> ret = acpi_dev_get_resources(adev, &resource_list,
> >>> i2c_acpi_add_resource, &irq);
> >>
> >
> >
> >> You also need to handle "Interrupt(..., ...AndWake)" case here. I would
> >> look into maybe defining
> >>
> >> #define IORESOURCE_IRQ_WAKECAPABLE (1<<6)
> >>
> >> in include/linux/ioport.h and plumbing it through from ACPI layer.
> >>
> >> Thanks.
> >
> > AFAIK the Intel (Not 100% certain) and AMD IO-APIC's can't actually
> > wake a system from suspend/suspend-to-idle.
>
> That may be true for S3 suspend (it sounds about right) there
> certainly is no way to "arm for wakeup" on the APIC, but with
> s2idle all IRQs which are not explicitly disabled by the OS
> still function normally so there any IRQ can be a wakeup
> source (AFAIK).
>
> And even with S3 suspend I think some IRQs can act as wakeup,
> but that is configured by the BIOS then and not something which
> linux can enable/disable. E.g IIRC the parent IRQ of the GPIO
> controllers on x86 is an APIC IRQ ...
>
> Regards,
>
> Hans
>
SGTM. I wanted to make sure there was interest before I invested the
time in adding the functionality. Hopefully I can push up a new patch
set tomorrow.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq
2022-09-08 14:40 ` Raul Rangel
@ 2022-09-08 15:23 ` Rafael J. Wysocki
2022-09-09 18:47 ` Raul Rangel
0 siblings, 1 reply; 31+ messages in thread
From: Rafael J. Wysocki @ 2022-09-08 15:23 UTC (permalink / raw)
To: Raul Rangel
Cc: Hans de Goede, Dmitry Torokhov, Linux ACPI, linux-input,
Limonciello, Mario, Tim Van Patten, Rafael J. Wysocki,
Mika Westerberg, Wolfram Sang,
open list:I2C SUBSYSTEM HOST DRIVERS, linux-kernel
On Thu, Sep 8, 2022 at 4:40 PM Raul Rangel <rrangel@chromium.org> wrote:
>
> On Wed, Sep 7, 2022 at 2:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi,
> >
> > On 9/7/22 04:00, Raul Rangel wrote:
> > > On Tue, Sep 6, 2022 at 7:00 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > >>
> > >> On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
> > >>> Device tree already has a mechanism to pass the wake_irq. It does this
> > >>> by looking for the wakeup-source property and setting the
> > >>> I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
> > >>> ACPI GpioInt wake flag to determine if the interrupt can be used to wake
> > >>> the system. Previously the i2c drivers had to make assumptions and
> > >>> blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
> > >>> If there is a device with an Active Low interrupt and the device gets
> > >>> powered off while suspending, the interrupt line will go low since it's
> > >>> no longer powered and wake the system. For this reason we should respect
> > >>> the board designers wishes and honor the wake bit defined on the
> > >>> GpioInt.
> > >>>
> > >>> This change does not cover the ACPI Interrupt or IRQ resources.
> > >>>
> > >>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > >>> ---
> > >>>
> > >>> drivers/i2c/i2c-core-acpi.c | 8 ++++++--
> > >>> drivers/i2c/i2c-core-base.c | 17 +++++++++++------
> > >>> drivers/i2c/i2c-core.h | 4 ++--
> > >>> 3 files changed, 19 insertions(+), 10 deletions(-)
> > >>>
> > >>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> > >>> index c762a879c4cc6b..cfe82a6ba3ef28 100644
> > >>> --- a/drivers/i2c/i2c-core-acpi.c
> > >>> +++ b/drivers/i2c/i2c-core-acpi.c
> > >>> @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
> > >>> /**
> > >>> * i2c_acpi_get_irq - get device IRQ number from ACPI
> > >>> * @client: Pointer to the I2C client device
> > >>> + * @wake_capable: Set to 1 if the IRQ is wake capable
> > >>> *
> > >>> * Find the IRQ number used by a specific client device.
> > >>> *
> > >>> * Return: The IRQ number or an error code.
> > >>> */
> > >>> -int i2c_acpi_get_irq(struct i2c_client *client)
> > >>> +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
> > >>> {
> > >>> struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> > >>> struct list_head resource_list;
> > >>> @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
> > >>>
> > >>> INIT_LIST_HEAD(&resource_list);
> > >>>
> > >>> + if (wake_capable)
> > >>> + *wake_capable = 0;
> > >>> +
> > >>> ret = acpi_dev_get_resources(adev, &resource_list,
> > >>> i2c_acpi_add_resource, &irq);
> > >>
> > >
> > >
> > >> You also need to handle "Interrupt(..., ...AndWake)" case here. I would
> > >> look into maybe defining
> > >>
> > >> #define IORESOURCE_IRQ_WAKECAPABLE (1<<6)
> > >>
> > >> in include/linux/ioport.h and plumbing it through from ACPI layer.
> > >>
> > >> Thanks.
> > >
> > > AFAIK the Intel (Not 100% certain) and AMD IO-APIC's can't actually
> > > wake a system from suspend/suspend-to-idle.
> >
> > That may be true for S3 suspend (it sounds about right) there
> > certainly is no way to "arm for wakeup" on the APIC, but with
> > s2idle all IRQs which are not explicitly disabled by the OS
> > still function normally so there any IRQ can be a wakeup
> > source (AFAIK).
That's true.
Moreover, even for S3 there are transitions into it and there may be
wakeup interrupts taking place during those transitions. Those may be
any IRQs too.
> > And even with S3 suspend I think some IRQs can act as wakeup,
> > but that is configured by the BIOS then and not something which
> > linux can enable/disable. E.g IIRC the parent IRQ of the GPIO
> > controllers on x86 is an APIC IRQ ...
It's more about how the system is wired up AFAICS. Basically, in
order to wake up the system from S3, the given IRQ needs to be
physically attached to an input that will trigger the platform wakeup
logic while in S3.
> >
>
> SGTM. I wanted to make sure there was interest before I invested the
> time in adding the functionality. Hopefully I can push up a new patch
> set tomorrow.
Sounds good. :-)
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq
2022-09-08 15:23 ` Rafael J. Wysocki
@ 2022-09-09 18:47 ` Raul Rangel
2022-09-10 1:25 ` Dmitry Torokhov
0 siblings, 1 reply; 31+ messages in thread
From: Raul Rangel @ 2022-09-09 18:47 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Hans de Goede, Dmitry Torokhov, Linux ACPI, linux-input,
Limonciello, Mario, Tim Van Patten, Mika Westerberg,
Wolfram Sang, open list:I2C SUBSYSTEM HOST DRIVERS, linux-kernel
It looks like `i2c_acpi_get_irq` and `platform_get_irq_optional` are
doing pretty much the same thing. Can we replace `i2c_acpi_get_irq`
and switch over to `platform_get_irq_optional`? Is it possible to get
a `platform_device` from an `i2c_client`?
On Thu, Sep 8, 2022 at 9:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Sep 8, 2022 at 4:40 PM Raul Rangel <rrangel@chromium.org> wrote:
> >
> > On Wed, Sep 7, 2022 at 2:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > On 9/7/22 04:00, Raul Rangel wrote:
> > > > On Tue, Sep 6, 2022 at 7:00 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > >>
> > > >> On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
> > > >>> Device tree already has a mechanism to pass the wake_irq. It does this
> > > >>> by looking for the wakeup-source property and setting the
> > > >>> I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
> > > >>> ACPI GpioInt wake flag to determine if the interrupt can be used to wake
> > > >>> the system. Previously the i2c drivers had to make assumptions and
> > > >>> blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
> > > >>> If there is a device with an Active Low interrupt and the device gets
> > > >>> powered off while suspending, the interrupt line will go low since it's
> > > >>> no longer powered and wake the system. For this reason we should respect
> > > >>> the board designers wishes and honor the wake bit defined on the
> > > >>> GpioInt.
> > > >>>
> > > >>> This change does not cover the ACPI Interrupt or IRQ resources.
> > > >>>
> > > >>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > >>> ---
> > > >>>
> > > >>> drivers/i2c/i2c-core-acpi.c | 8 ++++++--
> > > >>> drivers/i2c/i2c-core-base.c | 17 +++++++++++------
> > > >>> drivers/i2c/i2c-core.h | 4 ++--
> > > >>> 3 files changed, 19 insertions(+), 10 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> > > >>> index c762a879c4cc6b..cfe82a6ba3ef28 100644
> > > >>> --- a/drivers/i2c/i2c-core-acpi.c
> > > >>> +++ b/drivers/i2c/i2c-core-acpi.c
> > > >>> @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
> > > >>> /**
> > > >>> * i2c_acpi_get_irq - get device IRQ number from ACPI
> > > >>> * @client: Pointer to the I2C client device
> > > >>> + * @wake_capable: Set to 1 if the IRQ is wake capable
> > > >>> *
> > > >>> * Find the IRQ number used by a specific client device.
> > > >>> *
> > > >>> * Return: The IRQ number or an error code.
> > > >>> */
> > > >>> -int i2c_acpi_get_irq(struct i2c_client *client)
> > > >>> +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
> > > >>> {
> > > >>> struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> > > >>> struct list_head resource_list;
> > > >>> @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
> > > >>>
> > > >>> INIT_LIST_HEAD(&resource_list);
> > > >>>
> > > >>> + if (wake_capable)
> > > >>> + *wake_capable = 0;
> > > >>> +
> > > >>> ret = acpi_dev_get_resources(adev, &resource_list,
> > > >>> i2c_acpi_add_resource, &irq);
> > > >>
> > > >
> > > >
> > > >> You also need to handle "Interrupt(..., ...AndWake)" case here. I would
> > > >> look into maybe defining
> > > >>
> > > >> #define IORESOURCE_IRQ_WAKECAPABLE (1<<6)
> > > >>
> > > >> in include/linux/ioport.h and plumbing it through from ACPI layer.
> > > >>
> > > >> Thanks.
> > > >
> > > > AFAIK the Intel (Not 100% certain) and AMD IO-APIC's can't actually
> > > > wake a system from suspend/suspend-to-idle.
> > >
> > > That may be true for S3 suspend (it sounds about right) there
> > > certainly is no way to "arm for wakeup" on the APIC, but with
> > > s2idle all IRQs which are not explicitly disabled by the OS
> > > still function normally so there any IRQ can be a wakeup
> > > source (AFAIK).
>
> That's true.
>
> Moreover, even for S3 there are transitions into it and there may be
> wakeup interrupts taking place during those transitions. Those may be
> any IRQs too.
>
> > > And even with S3 suspend I think some IRQs can act as wakeup,
> > > but that is configured by the BIOS then and not something which
> > > linux can enable/disable. E.g IIRC the parent IRQ of the GPIO
> > > controllers on x86 is an APIC IRQ ...
>
> It's more about how the system is wired up AFAICS. Basically, in
> order to wake up the system from S3, the given IRQ needs to be
> physically attached to an input that will trigger the platform wakeup
> logic while in S3.
>
> > >
> >
> > SGTM. I wanted to make sure there was interest before I invested the
> > time in adding the functionality. Hopefully I can push up a new patch
> > set tomorrow.
>
> Sounds good. :-)
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq
2022-09-09 18:47 ` Raul Rangel
@ 2022-09-10 1:25 ` Dmitry Torokhov
0 siblings, 0 replies; 31+ messages in thread
From: Dmitry Torokhov @ 2022-09-10 1:25 UTC (permalink / raw)
To: Raul Rangel
Cc: Rafael J. Wysocki, Hans de Goede, Linux ACPI, linux-input,
Limonciello, Mario, Tim Van Patten, Mika Westerberg,
Wolfram Sang, open list:I2C SUBSYSTEM HOST DRIVERS, linux-kernel
On Fri, Sep 09, 2022 at 12:47:11PM -0600, Raul Rangel wrote:
> It looks like `i2c_acpi_get_irq` and `platform_get_irq_optional` are
> doing pretty much the same thing. Can we replace `i2c_acpi_get_irq`
> and switch over to `platform_get_irq_optional`? Is it possible to get
> a `platform_device` from an `i2c_client`?
No, they are completely different objects.
struct device
/ | \
platform_device i2c_client spi_device ...
Also, please no top-posting on kernel mailing lists.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 5/8] HID: i2c-hid: acpi: Stop setting wakeup_capable
2022-08-30 23:15 [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq Raul E Rangel
` (3 preceding siblings ...)
2022-08-30 23:15 ` [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq Raul E Rangel
@ 2022-08-30 23:15 ` Raul E Rangel
2022-08-30 23:15 ` [PATCH 6/8] Input: elan_i2c - Don't set wake_irq when using ACPI Raul E Rangel
` (3 subsequent siblings)
8 siblings, 0 replies; 31+ messages in thread
From: Raul E Rangel @ 2022-08-30 23:15 UTC (permalink / raw)
To: linux-acpi, linux-input
Cc: hdegoede, mario.limonciello, timvp, rafael, Raul E Rangel,
Alistair Francis, Benjamin Tissoires, Jiri Kosina, Rob Herring,
linux-kernel
This is now handled by the i2c-core driver.
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
index b96ae15e0ad917..375c77c3db74d9 100644
--- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
+++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
@@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
acpi_device_fix_up_power(adev);
- if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
- device_set_wakeup_capable(dev, true);
- device_set_wakeup_enable(dev, false);
- }
-
return i2c_hid_core_probe(client, &ihid_acpi->ops,
hid_descriptor_address, 0);
}
--
2.37.2.672.g94769d06f0-goog
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 6/8] Input: elan_i2c - Don't set wake_irq when using ACPI
2022-08-30 23:15 [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq Raul E Rangel
` (4 preceding siblings ...)
2022-08-30 23:15 ` [PATCH 5/8] HID: i2c-hid: acpi: Stop setting wakeup_capable Raul E Rangel
@ 2022-08-30 23:15 ` Raul E Rangel
2022-08-30 23:15 ` [PATCH 7/8] HID: i2c-hid: " Raul E Rangel
` (2 subsequent siblings)
8 siblings, 0 replies; 31+ messages in thread
From: Raul E Rangel @ 2022-08-30 23:15 UTC (permalink / raw)
To: linux-acpi, linux-input
Cc: hdegoede, mario.limonciello, timvp, rafael, Raul E Rangel,
Dmitry Torokhov, jingle.wu, linux-kernel
The i2c-core will now handle setting the wake_irq for ACPI systems.
I didn't delete the whole block since this also covers systems that
don't use ACPI or DT, but I'm honestly not sure if that's a valid
config.
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
drivers/input/mouse/elan_i2c_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 7d997d2b56436b..d434c8ff8c4ca2 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -1333,10 +1333,10 @@ static int elan_probe(struct i2c_client *client,
}
/*
- * Systems using device tree should set up wakeup via DTS,
+ * Systems using device tree should set up wakeup via DTS or ACPI,
* the rest will configure device as wakeup source by default.
*/
- if (!dev->of_node) {
+ if (!dev->of_node && !has_acpi_companion(dev)) {
device_init_wakeup(dev, true);
dev_pm_set_wake_irq(dev, client->irq);
}
--
2.37.2.672.g94769d06f0-goog
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 7/8] HID: i2c-hid: Don't set wake_irq when using ACPI
2022-08-30 23:15 [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq Raul E Rangel
` (5 preceding siblings ...)
2022-08-30 23:15 ` [PATCH 6/8] Input: elan_i2c - Don't set wake_irq when using ACPI Raul E Rangel
@ 2022-08-30 23:15 ` Raul E Rangel
2022-08-30 23:15 ` [PATCH 8/8] ACPI: PM: Take wake IRQ into consideration when entering suspend-to-idle Raul E Rangel
2022-08-31 11:52 ` [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq Andy Shevchenko
8 siblings, 0 replies; 31+ messages in thread
From: Raul E Rangel @ 2022-08-30 23:15 UTC (permalink / raw)
To: linux-acpi, linux-input
Cc: hdegoede, mario.limonciello, timvp, rafael, Raul E Rangel,
Alistair Francis, Angela Czubak, Benjamin Tissoires,
Dmitry Torokhov, Jiri Kosina, Matthias Kaehlcke, linux-kernel
The i2c-core will now handle setting the wake_irq for ACPI systems.
I didn't delete the whole block since this also covers systems that
don't use ACPI or DT, but I'm honestly not sure if that's a valid
config.
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 0b7a1a8b3e9a33..630e8dcda1100d 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1037,10 +1037,10 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
goto err_powered;
/*
- * Systems using device tree should set up wakeup via DTS,
+ * Systems using device tree should set up wakeup via DTS or ACPI,
* the rest will configure device as wakeup source by default.
*/
- if (!client->dev.of_node) {
+ if (!client->dev.of_node && !has_acpi_companion(&client->dev)) {
device_init_wakeup(&client->dev, true);
dev_pm_set_wake_irq(&client->dev, client->irq);
}
--
2.37.2.672.g94769d06f0-goog
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 8/8] ACPI: PM: Take wake IRQ into consideration when entering suspend-to-idle
2022-08-30 23:15 [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq Raul E Rangel
` (6 preceding siblings ...)
2022-08-30 23:15 ` [PATCH 7/8] HID: i2c-hid: " Raul E Rangel
@ 2022-08-30 23:15 ` Raul E Rangel
2022-08-31 11:52 ` [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq Andy Shevchenko
8 siblings, 0 replies; 31+ messages in thread
From: Raul E Rangel @ 2022-08-30 23:15 UTC (permalink / raw)
To: linux-acpi, linux-input
Cc: hdegoede, mario.limonciello, timvp, rafael, Raul E Rangel,
Len Brown, linux-kernel
This change adds support for ACPI devices that use ExclusiveAndWake or
SharedAndWake in their _CRS GpioInt definition (instead of using _PRW),
and also provide power resources. Previously the ACPI subsystem had no
idea if the device had a wake capable interrupt armed. This resulted
in the ACPI device PM system placing the device into D3Cold, and thus
cutting power to the device. With this change we will now query the
_S0W method to figure out the appropriate wake capable D-state.
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
drivers/acpi/device_pm.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 9dce1245689ca2..6bc81f525d5160 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -681,8 +681,23 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
d_min = ret;
wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
&& adev->wakeup.sleep_state >= target_state;
- } else {
- wakeup = adev->wakeup.flags.valid;
+ } else if (acpi_device_can_wakeup(adev)) {
+ /* ACPI GPE from specified by _PRW. */
+ wakeup = true;
+ } else if (device_may_wakeup(dev) && dev->power.wakeirq) {
+ /*
+ * The ACPI subsystem doesn't manage the wake bit for IRQs
+ * defined with ExclusiveAndWake and SharedAndWake. Instead we
+ * expect them to be managed via the PM subsystem. Drivers
+ * should call dev_pm_set_wake_irq to register an IRQ as a wake
+ * source.
+ *
+ * If a device has a wake IRQ attached we need to check the
+ * _S0W method to get the correct wake D-state. Otherwise we
+ * end up putting the device into D3Cold which will more than
+ * likely disable wake functionality.
+ */
+ wakeup = true;
}
/*
--
2.37.2.672.g94769d06f0-goog
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq
2022-08-30 23:15 [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq Raul E Rangel
` (7 preceding siblings ...)
2022-08-30 23:15 ` [PATCH 8/8] ACPI: PM: Take wake IRQ into consideration when entering suspend-to-idle Raul E Rangel
@ 2022-08-31 11:52 ` Andy Shevchenko
2022-08-31 14:37 ` Raul Rangel
8 siblings, 1 reply; 31+ messages in thread
From: Andy Shevchenko @ 2022-08-31 11:52 UTC (permalink / raw)
To: Raul E Rangel
Cc: linux-acpi, linux-input, hdegoede, mario.limonciello, timvp,
rafael, Alistair Francis, Angela Czubak, Bartosz Golaszewski,
Bartosz Szczepanek, Benjamin Tissoires, Dmitry Torokhov,
Jiri Kosina, Len Brown, Linus Walleij, Matthias Kaehlcke,
Mika Westerberg, Rob Herring, Wolfram Sang, Yang Li, jingle.wu,
linux-gpio, linux-i2c, linux-kernel
On Tue, Aug 30, 2022 at 05:15:33PM -0600, Raul E Rangel wrote:
> Today, i2c drivers are making the assumption that their IRQs can also
> be used as wake IRQs. This isn't always the case and it can lead to
> spurious wakes. This has recently started to affect AMD Chromebooks.
> With the introduction of
> d62bd5ce12d7 ("pinctrl: amd: Implement irq_set_wake"), the AMD GPIO
> controller gained the capability to set the wake bit on each GPIO. The
> ACPI specification defines two ways to inform the system if a device is
> wake capable:
> 1) The _PRW object defines the GPE that can be used to wake the system.
> 2) Setting ExclusiveAndWake or SharedAndWake in the _CRS GpioInt.
>
> Currently only the first method is supported. The i2c drivers don't have
> any indication that the IRQ is wake capable, so they guess. This causes
> spurious interrupts, for example:
> * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
> `_PRW` or `ExclusiveAndWake` so that means the device can't wake the
> system.
> * The IRQ line is active level low for this device and is pulled up by
> the power resource defined in `_PR0`/`_PR3`.
> * The i2c driver will (incorrectly) arm the GPIO for wake by calling
> `enable_irq_wake` as part of its suspend hook.
> * ACPI will power down the device since it doesn't have a wake GPE
> associated with it.
> * When the device is powered down, the IRQ line will drop, and it will
> trigger a wake event.
>
> See the following debug log:
> [ 42.335804] PM: Suspending system (s2idle)
> [ 42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
> [ 42.467736] power-0416 __acpi_power_off : Power resource [PR00] turned off
> [ 42.467739] device_pm-0280 device_set_power : Device [H05D] transitioned to D3cold
> [ 42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
> [ 42.535293] PM: Wakeup unrelated to ACPI SCI
> [ 42.535294] PM: resume from suspend-to-idle
>
> In order to fix this, we need to take into account the wake capable bit
> defined on the GpioInt. This is accomplished by:
> * Migrating some of the i2c drivers over to using the PM subsystem to
> manage the wake IRQ. max8925-i2c, elants_i2c, and raydium_i2c_ts still
> need to be migrated, I can do that depending on the feedback to this
> patch series.
> * Expose the wake_capable bit from the ACPI GpioInt resource to the
> i2c core.
> * Use the wake_capable bit in the i2c core to call
> `dev_pm_set_wake_irq`. This reuses the existing device tree flow.
> * Make the i2c drivers stop calling `dev_pm_set_wake_irq` since it's now
> handled by the i2c core.
> * Make the ACPI device PM system aware of the wake_irq. This is
> necessary so the device doesn't incorrectly get powered down when a
> wake_irq is enabled.
>
> I've tested this code with various combinations of having _PRW,
> ExclusiveAndWake and power resources all defined or not defined, but it
> would be great if others could test this out on their hardware.
I have got only cover letter and a single patch (#3). What's going on?
Note: I'm also reviewer of I²C DesignWare driver, you really have to
fix your tools / submission process and try again. No review for this
series.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq
2022-08-31 11:52 ` [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq Andy Shevchenko
@ 2022-08-31 14:37 ` Raul Rangel
2022-08-31 15:18 ` Hans de Goede
0 siblings, 1 reply; 31+ messages in thread
From: Raul Rangel @ 2022-08-31 14:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linux ACPI, linux-input, Hans de Goede, Limonciello, Mario,
Tim Van Patten, Rafael J. Wysocki, Alistair Francis,
Angela Czubak, Bartosz Golaszewski, Bartosz Szczepanek,
Benjamin Tissoires, Dmitry Torokhov, Jiri Kosina, Len Brown,
Linus Walleij, Matthias Kaehlcke, Mika Westerberg, Rob Herring,
Wolfram Sang, Yang Li, jingle.wu, open list:GPIO SUBSYSTEM,
open list:I2C SUBSYSTEM HOST DRIVERS, linux-kernel
Interesting... The patch series is here:
https://patchwork.kernel.org/project/linux-input/cover/20220830231541.1135813-1-rrangel@chromium.org/
I'll look into why you only got added to 2 of the emails.
On Wed, Aug 31, 2022 at 5:52 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Aug 30, 2022 at 05:15:33PM -0600, Raul E Rangel wrote:
> > Today, i2c drivers are making the assumption that their IRQs can also
> > be used as wake IRQs. This isn't always the case and it can lead to
> > spurious wakes. This has recently started to affect AMD Chromebooks.
> > With the introduction of
> > d62bd5ce12d7 ("pinctrl: amd: Implement irq_set_wake"), the AMD GPIO
> > controller gained the capability to set the wake bit on each GPIO. The
> > ACPI specification defines two ways to inform the system if a device is
> > wake capable:
> > 1) The _PRW object defines the GPE that can be used to wake the system.
> > 2) Setting ExclusiveAndWake or SharedAndWake in the _CRS GpioInt.
> >
> > Currently only the first method is supported. The i2c drivers don't have
> > any indication that the IRQ is wake capable, so they guess. This causes
> > spurious interrupts, for example:
> > * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
> > `_PRW` or `ExclusiveAndWake` so that means the device can't wake the
> > system.
> > * The IRQ line is active level low for this device and is pulled up by
> > the power resource defined in `_PR0`/`_PR3`.
> > * The i2c driver will (incorrectly) arm the GPIO for wake by calling
> > `enable_irq_wake` as part of its suspend hook.
> > * ACPI will power down the device since it doesn't have a wake GPE
> > associated with it.
> > * When the device is powered down, the IRQ line will drop, and it will
> > trigger a wake event.
> >
> > See the following debug log:
> > [ 42.335804] PM: Suspending system (s2idle)
> > [ 42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
> > [ 42.467736] power-0416 __acpi_power_off : Power resource [PR00] turned off
> > [ 42.467739] device_pm-0280 device_set_power : Device [H05D] transitioned to D3cold
> > [ 42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
> > [ 42.535293] PM: Wakeup unrelated to ACPI SCI
> > [ 42.535294] PM: resume from suspend-to-idle
> >
> > In order to fix this, we need to take into account the wake capable bit
> > defined on the GpioInt. This is accomplished by:
> > * Migrating some of the i2c drivers over to using the PM subsystem to
> > manage the wake IRQ. max8925-i2c, elants_i2c, and raydium_i2c_ts still
> > need to be migrated, I can do that depending on the feedback to this
> > patch series.
> > * Expose the wake_capable bit from the ACPI GpioInt resource to the
> > i2c core.
> > * Use the wake_capable bit in the i2c core to call
> > `dev_pm_set_wake_irq`. This reuses the existing device tree flow.
> > * Make the i2c drivers stop calling `dev_pm_set_wake_irq` since it's now
> > handled by the i2c core.
> > * Make the ACPI device PM system aware of the wake_irq. This is
> > necessary so the device doesn't incorrectly get powered down when a
> > wake_irq is enabled.
> >
> > I've tested this code with various combinations of having _PRW,
> > ExclusiveAndWake and power resources all defined or not defined, but it
> > would be great if others could test this out on their hardware.
>
> I have got only cover letter and a single patch (#3). What's going on?
>
> Note: I'm also reviewer of I涎 DesignWare driver, you really have to
> fix your tools / submission process and try again. No review for this
> series.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq
2022-08-31 14:37 ` Raul Rangel
@ 2022-08-31 15:18 ` Hans de Goede
0 siblings, 0 replies; 31+ messages in thread
From: Hans de Goede @ 2022-08-31 15:18 UTC (permalink / raw)
To: Raul Rangel, Andy Shevchenko
Cc: Linux ACPI, linux-input, Limonciello, Mario, Tim Van Patten,
Rafael J. Wysocki, Alistair Francis, Angela Czubak,
Bartosz Golaszewski, Bartosz Szczepanek, Benjamin Tissoires,
Dmitry Torokhov, Jiri Kosina, Len Brown, Linus Walleij,
Matthias Kaehlcke, Mika Westerberg, Rob Herring, Wolfram Sang,
Yang Li, jingle.wu, open list:GPIO SUBSYSTEM,
open list:I2C SUBSYSTEM HOST DRIVERS, linux-kernel
Hi,
On 8/31/22 16:37, Raul Rangel wrote:
> Interesting... The patch series is here:
> https://patchwork.kernel.org/project/linux-input/cover/20220830231541.1135813-1-rrangel@chromium.org/
>
> I'll look into why you only got added to 2 of the emails.
FWIW I also received the full series without problems.
I'll try to reply to this soon-ish, but I have a bit of
a patch backlog to process and I'm trying to process
the backlog in FIFO order and this is one of the last
series in the backlog ...
Regards,
Hans
>
> On Wed, Aug 31, 2022 at 5:52 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> On Tue, Aug 30, 2022 at 05:15:33PM -0600, Raul E Rangel wrote:
>>> Today, i2c drivers are making the assumption that their IRQs can also
>>> be used as wake IRQs. This isn't always the case and it can lead to
>>> spurious wakes. This has recently started to affect AMD Chromebooks.
>>> With the introduction of
>>> d62bd5ce12d7 ("pinctrl: amd: Implement irq_set_wake"), the AMD GPIO
>>> controller gained the capability to set the wake bit on each GPIO. The
>>> ACPI specification defines two ways to inform the system if a device is
>>> wake capable:
>>> 1) The _PRW object defines the GPE that can be used to wake the system.
>>> 2) Setting ExclusiveAndWake or SharedAndWake in the _CRS GpioInt.
>>>
>>> Currently only the first method is supported. The i2c drivers don't have
>>> any indication that the IRQ is wake capable, so they guess. This causes
>>> spurious interrupts, for example:
>>> * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
>>> `_PRW` or `ExclusiveAndWake` so that means the device can't wake the
>>> system.
>>> * The IRQ line is active level low for this device and is pulled up by
>>> the power resource defined in `_PR0`/`_PR3`.
>>> * The i2c driver will (incorrectly) arm the GPIO for wake by calling
>>> `enable_irq_wake` as part of its suspend hook.
>>> * ACPI will power down the device since it doesn't have a wake GPE
>>> associated with it.
>>> * When the device is powered down, the IRQ line will drop, and it will
>>> trigger a wake event.
>>>
>>> See the following debug log:
>>> [ 42.335804] PM: Suspending system (s2idle)
>>> [ 42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
>>> [ 42.467736] power-0416 __acpi_power_off : Power resource [PR00] turned off
>>> [ 42.467739] device_pm-0280 device_set_power : Device [H05D] transitioned to D3cold
>>> [ 42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
>>> [ 42.535293] PM: Wakeup unrelated to ACPI SCI
>>> [ 42.535294] PM: resume from suspend-to-idle
>>>
>>> In order to fix this, we need to take into account the wake capable bit
>>> defined on the GpioInt. This is accomplished by:
>>> * Migrating some of the i2c drivers over to using the PM subsystem to
>>> manage the wake IRQ. max8925-i2c, elants_i2c, and raydium_i2c_ts still
>>> need to be migrated, I can do that depending on the feedback to this
>>> patch series.
>>> * Expose the wake_capable bit from the ACPI GpioInt resource to the
>>> i2c core.
>>> * Use the wake_capable bit in the i2c core to call
>>> `dev_pm_set_wake_irq`. This reuses the existing device tree flow.
>>> * Make the i2c drivers stop calling `dev_pm_set_wake_irq` since it's now
>>> handled by the i2c core.
>>> * Make the ACPI device PM system aware of the wake_irq. This is
>>> necessary so the device doesn't incorrectly get powered down when a
>>> wake_irq is enabled.
>>>
>>> I've tested this code with various combinations of having _PRW,
>>> ExclusiveAndWake and power resources all defined or not defined, but it
>>> would be great if others could test this out on their hardware.
>>
>> I have got only cover letter and a single patch (#3). What's going on?
>>
>> Note: I'm also reviewer of I涎 DesignWare driver, you really have to
>> fix your tools / submission process and try again. No review for this
>> series.
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>>
>>
>
^ permalink raw reply [flat|nested] 31+ messages in thread