| Message ID | 20220830171332.1.Id022caf53d01112188308520915798f08a33cd3e@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq | expand |
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 >
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 > >
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 > > >
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....
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.
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
* 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>
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.
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!
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.
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);
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(-)