* [PATCH] i2c: allow specifying separate wakeup interrupt in device tree
@ 2015-07-30 20:14 Dmitry Torokhov
2015-07-31 10:57 ` Vignesh R
2015-08-09 15:22 ` Wolfram Sang
0 siblings, 2 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2015-07-30 20:14 UTC (permalink / raw)
To: Wolfram Sang
Cc: Mika Westerberg, Rafael J. Wysocki, Ulf Hansson, Vignesh R,
Tony Lindgren, Rob Herring, Mark Rutland,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Instead of having each i2c driver individually parse device tree data in
case it or platform supports separate wakeup interrupt, and handle
enabling and disabling wakeup interrupts in their power management
routines, let's have i2c core do that for us.
Platforms wishing to specify separate wakeup interrupt for the device
should use named interrupt syntax in their DTSes:
interrupt-parent = <&intc1>;
interrupts = <5 0>, <6 0>;
interrupt-names = "irq", "wakeup";
This patch is inspired by work done by Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org> for
pixcir_i2c_ts driver.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/i2c/i2c-core.c | 46 ++++++++++++++++++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e6d4935..19e7a17 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -47,6 +47,7 @@
#include <linux/rwsem.h>
#include <linux/pm_runtime.h>
#include <linux/pm_domain.h>
+#include <linux/pm_wakeirq.h>
#include <linux/acpi.h>
#include <linux/jump_label.h>
#include <asm/uaccess.h>
@@ -639,11 +640,13 @@ static int i2c_device_probe(struct device *dev)
if (!client->irq) {
int irq = -ENOENT;
- if (dev->of_node)
- irq = of_irq_get(dev->of_node, 0);
- else if (ACPI_COMPANION(dev))
+ if (dev->of_node) {
+ irq = of_irq_get_byname(dev->of_node, "irq");
+ if (irq == -EINVAL || irq == -ENODATA)
+ irq = of_irq_get(dev->of_node, 0);
+ } else if (ACPI_COMPANION(dev)) {
irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
-
+ }
if (irq == -EPROBE_DEFER)
return irq;
if (irq < 0)
@@ -659,20 +662,47 @@ static int i2c_device_probe(struct device *dev)
if (!device_can_wakeup(&client->dev))
device_init_wakeup(&client->dev,
client->flags & I2C_CLIENT_WAKE);
+
+ if (device_can_wakeup(&client->dev)) {
+ int wakeirq = -ENOENT;
+
+ if (dev->of_node) {
+ wakeirq = of_irq_get_byname(dev->of_node, "wakeup");
+ if (wakeirq == -EPROBE_DEFER)
+ return wakeirq;
+ }
+
+ if (wakeirq > 0 && wakeirq != client->irq)
+ status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
+ else if (client->irq > 0)
+ status = dev_pm_set_wake_irq(dev, wakeirq);
+ else
+ status = 0;
+
+ if (status)
+ dev_warn(&client->dev, "failed to set up wakeup irq");
+ }
+
dev_dbg(dev, "probe\n");
status = of_clk_set_defaults(dev->of_node, false);
if (status < 0)
- return status;
+ goto err_clear_wakeup_irq;
status = dev_pm_domain_attach(&client->dev, true);
if (status != -EPROBE_DEFER) {
status = driver->probe(client, i2c_match_id(driver->id_table,
client));
if (status)
- dev_pm_domain_detach(&client->dev, true);
+ goto err_detach_pm_domain;
}
+ return 0;
+
+err_detach_pm_domain:
+ dev_pm_domain_detach(&client->dev, true);
+err_clear_wakeup_irq:
+ dev_pm_clear_wake_irq(&client->dev);
return status;
}
@@ -692,6 +722,10 @@ static int i2c_device_remove(struct device *dev)
}
dev_pm_domain_detach(&client->dev, true);
+
+ dev_pm_clear_wake_irq(&client->dev);
+ device_init_wakeup(&client->dev, 0);
+
return status;
}
--
2.5.0.rc2.392.g76e840b
--
Dmitry
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree
2015-07-30 20:14 [PATCH] i2c: allow specifying separate wakeup interrupt in device tree Dmitry Torokhov
@ 2015-07-31 10:57 ` Vignesh R
[not found] ` <55BB54B1.80603-l0cyMroinI0@public.gmane.org>
2015-08-09 15:22 ` Wolfram Sang
1 sibling, 1 reply; 11+ messages in thread
From: Vignesh R @ 2015-07-31 10:57 UTC (permalink / raw)
To: Dmitry Torokhov, Wolfram Sang
Cc: Mika Westerberg, Rafael J. Wysocki, Ulf Hansson, Tony Lindgren,
Rob Herring, Mark Rutland, linux-i2c@vger.kernel.org,
linux-kernel@vger.kernel.org
On 07/31/2015 01:44 AM, Dmitry Torokhov wrote:
> Instead of having each i2c driver individually parse device tree data in
> case it or platform supports separate wakeup interrupt, and handle
> enabling and disabling wakeup interrupts in their power management
> routines, let's have i2c core do that for us.
>
> Platforms wishing to specify separate wakeup interrupt for the device
> should use named interrupt syntax in their DTSes:
>
> interrupt-parent = <&intc1>;
> interrupts = <5 0>, <6 0>;
> interrupt-names = "irq", "wakeup";
>
> This patch is inspired by work done by Vignesh R <vigneshr@ti.com> for
> pixcir_i2c_ts driver.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Tested this patch on am437x-gp-evm with pixcir_i2c_ts as the wakeup
source. I was able to wakeup from suspend both as built-in and as a
module (No changes were done to pixcir_i2c_ts driver).
Tested-by: Vignesh R <vigneshr@ti.com>
> ---
> drivers/i2c/i2c-core.c | 46 ++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index e6d4935..19e7a17 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -47,6 +47,7 @@
> #include <linux/rwsem.h>
> #include <linux/pm_runtime.h>
> #include <linux/pm_domain.h>
> +#include <linux/pm_wakeirq.h>
> #include <linux/acpi.h>
> #include <linux/jump_label.h>
> #include <asm/uaccess.h>
> @@ -639,11 +640,13 @@ static int i2c_device_probe(struct device *dev)
> if (!client->irq) {
> int irq = -ENOENT;
>
> - if (dev->of_node)
> - irq = of_irq_get(dev->of_node, 0);
> - else if (ACPI_COMPANION(dev))
> + if (dev->of_node) {
> + irq = of_irq_get_byname(dev->of_node, "irq");
> + if (irq == -EINVAL || irq == -ENODATA)
> + irq = of_irq_get(dev->of_node, 0);
> + } else if (ACPI_COMPANION(dev)) {
> irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
> -
> + }
> if (irq == -EPROBE_DEFER)
> return irq;
> if (irq < 0)
> @@ -659,20 +662,47 @@ static int i2c_device_probe(struct device *dev)
> if (!device_can_wakeup(&client->dev))
> device_init_wakeup(&client->dev,
> client->flags & I2C_CLIENT_WAKE);
> +
> + if (device_can_wakeup(&client->dev)) {
> + int wakeirq = -ENOENT;
> +
> + if (dev->of_node) {
> + wakeirq = of_irq_get_byname(dev->of_node, "wakeup");
> + if (wakeirq == -EPROBE_DEFER)
> + return wakeirq;
> + }
> +
> + if (wakeirq > 0 && wakeirq != client->irq)
> + status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
> + else if (client->irq > 0)
> + status = dev_pm_set_wake_irq(dev, wakeirq);
> + else
> + status = 0;
> +
> + if (status)
> + dev_warn(&client->dev, "failed to set up wakeup irq");
> + }
> +
> dev_dbg(dev, "probe\n");
>
> status = of_clk_set_defaults(dev->of_node, false);
> if (status < 0)
> - return status;
> + goto err_clear_wakeup_irq;
>
> status = dev_pm_domain_attach(&client->dev, true);
> if (status != -EPROBE_DEFER) {
> status = driver->probe(client, i2c_match_id(driver->id_table,
> client));
> if (status)
> - dev_pm_domain_detach(&client->dev, true);
> + goto err_detach_pm_domain;
> }
>
> + return 0;
> +
> +err_detach_pm_domain:
> + dev_pm_domain_detach(&client->dev, true);
> +err_clear_wakeup_irq:
> + dev_pm_clear_wake_irq(&client->dev);
> return status;
> }
>
> @@ -692,6 +722,10 @@ static int i2c_device_remove(struct device *dev)
> }
>
> dev_pm_domain_detach(&client->dev, true);
> +
> + dev_pm_clear_wake_irq(&client->dev);
> + device_init_wakeup(&client->dev, 0);
> +
> return status;
> }
>
>
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree
2015-07-30 20:14 [PATCH] i2c: allow specifying separate wakeup interrupt in device tree Dmitry Torokhov
2015-07-31 10:57 ` Vignesh R
@ 2015-08-09 15:22 ` Wolfram Sang
2015-08-10 5:59 ` Dmitry Torokhov
1 sibling, 1 reply; 11+ messages in thread
From: Wolfram Sang @ 2015-08-09 15:22 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mika Westerberg, Rafael J. Wysocki, Ulf Hansson, Vignesh R,
Tony Lindgren, Rob Herring, Mark Rutland,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
[-- Attachment #1: Type: text/plain, Size: 2973 bytes --]
On Thu, Jul 30, 2015 at 01:14:31PM -0700, Dmitry Torokhov wrote:
> Instead of having each i2c driver individually parse device tree data in
> case it or platform supports separate wakeup interrupt, and handle
> enabling and disabling wakeup interrupts in their power management
> routines, let's have i2c core do that for us.
>
> Platforms wishing to specify separate wakeup interrupt for the device
> should use named interrupt syntax in their DTSes:
>
> interrupt-parent = <&intc1>;
> interrupts = <5 0>, <6 0>;
> interrupt-names = "irq", "wakeup";
>
> This patch is inspired by work done by Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org> for
> pixcir_i2c_ts driver.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
I think it is a useful addition. Can someone add a paragraph describing
this handling on top of the new generic i2c binding docs?
http://patchwork.ozlabs.org/patch/505368/
> @@ -659,20 +662,47 @@ static int i2c_device_probe(struct device *dev)
> if (!device_can_wakeup(&client->dev))
> device_init_wakeup(&client->dev,
> client->flags & I2C_CLIENT_WAKE);
I was about to ask if we couldn't combine this and the later if-blocks
with an if-else combination. But now I stumble over the above block in
general: If the device cannot cause wake ups, then we might initialize
it as a wakeup-device depending on client->flags??
> +
> + if (device_can_wakeup(&client->dev)) {
> + int wakeirq = -ENOENT;
> +
> + if (dev->of_node) {
> + wakeirq = of_irq_get_byname(dev->of_node, "wakeup");
> + if (wakeirq == -EPROBE_DEFER)
> + return wakeirq;
> + }
> +
> + if (wakeirq > 0 && wakeirq != client->irq)
> + status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
> + else if (client->irq > 0)
> + status = dev_pm_set_wake_irq(dev, wakeirq);
> + else
> + status = 0;
> +
> + if (status)
> + dev_warn(&client->dev, "failed to set up wakeup irq");
> + }
> +
> dev_dbg(dev, "probe\n");
>
> status = of_clk_set_defaults(dev->of_node, false);
> if (status < 0)
> - return status;
> + goto err_clear_wakeup_irq;
>
> status = dev_pm_domain_attach(&client->dev, true);
> if (status != -EPROBE_DEFER) {
> status = driver->probe(client, i2c_match_id(driver->id_table,
> client));
> if (status)
> - dev_pm_domain_detach(&client->dev, true);
> + goto err_detach_pm_domain;
> }
>
> + return 0;
> +
> +err_detach_pm_domain:
> + dev_pm_domain_detach(&client->dev, true);
> +err_clear_wakeup_irq:
> + dev_pm_clear_wake_irq(&client->dev);
> return status;
> }
>
> @@ -692,6 +722,10 @@ static int i2c_device_remove(struct device *dev)
> }
>
> dev_pm_domain_detach(&client->dev, true);
> +
> + dev_pm_clear_wake_irq(&client->dev);
> + device_init_wakeup(&client->dev, 0);
> +
> return status;
> }
>
> --
> 2.5.0.rc2.392.g76e840b
>
>
> --
> Dmitry
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree
2015-08-09 15:22 ` Wolfram Sang
@ 2015-08-10 5:59 ` Dmitry Torokhov
2015-08-10 6:16 ` Wolfram Sang
0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2015-08-10 5:59 UTC (permalink / raw)
To: Wolfram Sang
Cc: Mika Westerberg, Rafael J. Wysocki, Ulf Hansson, Vignesh R,
Tony Lindgren, Rob Herring, Mark Rutland,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Sun, Aug 09, 2015 at 05:22:55PM +0200, Wolfram Sang wrote:
> On Thu, Jul 30, 2015 at 01:14:31PM -0700, Dmitry Torokhov wrote:
> > Instead of having each i2c driver individually parse device tree data in
> > case it or platform supports separate wakeup interrupt, and handle
> > enabling and disabling wakeup interrupts in their power management
> > routines, let's have i2c core do that for us.
> >
> > Platforms wishing to specify separate wakeup interrupt for the device
> > should use named interrupt syntax in their DTSes:
> >
> > interrupt-parent = <&intc1>;
> > interrupts = <5 0>, <6 0>;
> > interrupt-names = "irq", "wakeup";
> >
> > This patch is inspired by work done by Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org> for
> > pixcir_i2c_ts driver.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> I think it is a useful addition. Can someone add a paragraph describing
> this handling on top of the new generic i2c binding docs?
>
> http://patchwork.ozlabs.org/patch/505368/
Yes, I will.
>
> > @@ -659,20 +662,47 @@ static int i2c_device_probe(struct device *dev)
> > if (!device_can_wakeup(&client->dev))
> > device_init_wakeup(&client->dev,
> > client->flags & I2C_CLIENT_WAKE);
>
> I was about to ask if we couldn't combine this and the later if-blocks
> with an if-else combination. But now I stumble over the above block in
> general: If the device cannot cause wake ups, then we might initialize
> it as a wakeup-device depending on client->flags??
I believe it is done so that we do not try to re-add wakeup source after
unbinding/rebinding the device. With my patch we clearing wakeup flag on
unbind, so it is OK, but there is still error path where we might want
to reset the wakeup flag as well.
>
> > +
> > + if (device_can_wakeup(&client->dev)) {
> > + int wakeirq = -ENOENT;
> > +
> > + if (dev->of_node) {
> > + wakeirq = of_irq_get_byname(dev->of_node, "wakeup");
> > + if (wakeirq == -EPROBE_DEFER)
> > + return wakeirq;
> > + }
> > +
> > + if (wakeirq > 0 && wakeirq != client->irq)
> > + status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
> > + else if (client->irq > 0)
> > + status = dev_pm_set_wake_irq(dev, wakeirq);
> > + else
> > + status = 0;
> > +
> > + if (status)
> > + dev_warn(&client->dev, "failed to set up wakeup irq");
> > + }
> > +
> > dev_dbg(dev, "probe\n");
> >
> > status = of_clk_set_defaults(dev->of_node, false);
> > if (status < 0)
> > - return status;
> > + goto err_clear_wakeup_irq;
> >
> > status = dev_pm_domain_attach(&client->dev, true);
> > if (status != -EPROBE_DEFER) {
> > status = driver->probe(client, i2c_match_id(driver->id_table,
> > client));
> > if (status)
> > - dev_pm_domain_detach(&client->dev, true);
> > + goto err_detach_pm_domain;
> > }
> >
> > + return 0;
> > +
> > +err_detach_pm_domain:
> > + dev_pm_domain_detach(&client->dev, true);
> > +err_clear_wakeup_irq:
> > + dev_pm_clear_wake_irq(&client->dev);
> > return status;
> > }
> >
> > @@ -692,6 +722,10 @@ static int i2c_device_remove(struct device *dev)
> > }
> >
> > dev_pm_domain_detach(&client->dev, true);
> > +
> > + dev_pm_clear_wake_irq(&client->dev);
> > + device_init_wakeup(&client->dev, 0);
> > +
> > return status;
> > }
> >
> > --
> > 2.5.0.rc2.392.g76e840b
> >
> >
> > --
> > Dmitry
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree
2015-08-10 5:59 ` Dmitry Torokhov
@ 2015-08-10 6:16 ` Wolfram Sang
2015-08-19 17:43 ` Wolfram Sang
0 siblings, 1 reply; 11+ messages in thread
From: Wolfram Sang @ 2015-08-10 6:16 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mika Westerberg, Rafael J. Wysocki, Ulf Hansson, Vignesh R,
Tony Lindgren, Rob Herring, Mark Rutland, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]
> > I think it is a useful addition. Can someone add a paragraph describing
> > this handling on top of the new generic i2c binding docs?
> >
> > http://patchwork.ozlabs.org/patch/505368/
>
> Yes, I will.
Great, thanks!
>
> >
> > > @@ -659,20 +662,47 @@ static int i2c_device_probe(struct device *dev)
> > > if (!device_can_wakeup(&client->dev))
> > > device_init_wakeup(&client->dev,
> > > client->flags & I2C_CLIENT_WAKE);
> >
> > I was about to ask if we couldn't combine this and the later if-blocks
> > with an if-else combination. But now I stumble over the above block in
> > general: If the device cannot cause wake ups, then we might initialize
> > it as a wakeup-device depending on client->flags??
>
> I believe it is done so that we do not try to re-add wakeup source after
> unbinding/rebinding the device. With my patch we clearing wakeup flag on
> unbind, so it is OK, but there is still error path where we might want
> to reset the wakeup flag as well.
I was wondering if it wants to achieve that, why does it not
unconditionally use 0 instead of the WAKE flag.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree
2015-08-10 6:16 ` Wolfram Sang
@ 2015-08-19 17:43 ` Wolfram Sang
2015-08-19 17:51 ` Dmitry Torokhov
0 siblings, 1 reply; 11+ messages in thread
From: Wolfram Sang @ 2015-08-19 17:43 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mika Westerberg, Rafael J. Wysocki, Ulf Hansson, Vignesh R,
Tony Lindgren, Rob Herring, Mark Rutland,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
> > > > @@ -659,20 +662,47 @@ static int i2c_device_probe(struct device *dev)
> > > > if (!device_can_wakeup(&client->dev))
> > > > device_init_wakeup(&client->dev,
> > > > client->flags & I2C_CLIENT_WAKE);
> > >
> > > I was about to ask if we couldn't combine this and the later if-blocks
> > > with an if-else combination. But now I stumble over the above block in
> > > general: If the device cannot cause wake ups, then we might initialize
> > > it as a wakeup-device depending on client->flags??
> >
> > I believe it is done so that we do not try to re-add wakeup source after
> > unbinding/rebinding the device. With my patch we clearing wakeup flag on
> > unbind, so it is OK, but there is still error path where we might want
> > to reset the wakeup flag as well.
>
> I was wondering if it wants to achieve that, why does it not
> unconditionally use 0 instead of the WAKE flag.
When reviewing V2, I wasn't comfortable with just guessing what the old
code means. So, I did some digging and found:
https://lkml.org/lkml/2008/8/10/204
Quoting the interesting paragraph from David Brownell:
===
Better would be to preserve any existing settings:
if (!device_can_wakeup(&client->dev))
device_init_wakeup(...)
That way the userspace policy setting is preserved unless the
device itself gets removed ... instead of being clobbered by
the simple act of (re)probing a driver.
> > + device_init_wakeup(&client->dev, client->flags &
> > I2C_CLIENT_WAKE);
===
I have to admit that I am not familiar with device wakeup handling and
especially its userspace policies. Can you double check that your V2
meets the above intention?
Thanks,
Wolfram
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree
2015-08-19 17:43 ` Wolfram Sang
@ 2015-08-19 17:51 ` Dmitry Torokhov
2015-08-24 12:33 ` Wolfram Sang
0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2015-08-19 17:51 UTC (permalink / raw)
To: Wolfram Sang
Cc: Mika Westerberg, Rafael J. Wysocki, Ulf Hansson, Vignesh R,
Tony Lindgren, Rob Herring, Mark Rutland, Linux I2C, lkml
Hi Wolfram,
On Wed, Aug 19, 2015 at 10:43 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > > > @@ -659,20 +662,47 @@ static int i2c_device_probe(struct device *dev)
>> > > > if (!device_can_wakeup(&client->dev))
>> > > > device_init_wakeup(&client->dev,
>> > > > client->flags & I2C_CLIENT_WAKE);
>> > >
>> > > I was about to ask if we couldn't combine this and the later if-blocks
>> > > with an if-else combination. But now I stumble over the above block in
>> > > general: If the device cannot cause wake ups, then we might initialize
>> > > it as a wakeup-device depending on client->flags??
>> >
>> > I believe it is done so that we do not try to re-add wakeup source after
>> > unbinding/rebinding the device. With my patch we clearing wakeup flag on
>> > unbind, so it is OK, but there is still error path where we might want
>> > to reset the wakeup flag as well.
>>
>> I was wondering if it wants to achieve that, why does it not
>> unconditionally use 0 instead of the WAKE flag.
>
> When reviewing V2, I wasn't comfortable with just guessing what the old
> code means. So, I did some digging and found:
>
> https://lkml.org/lkml/2008/8/10/204
>
> Quoting the interesting paragraph from David Brownell:
>
> ===
>
> Better would be to preserve any existing settings:
>
> if (!device_can_wakeup(&client->dev))
> device_init_wakeup(...)
> That way the userspace policy setting is preserved unless the
> device itself gets removed ... instead of being clobbered by
> the simple act of (re)probing a driver.
>
>> > + device_init_wakeup(&client->dev, client->flags &
>> > I2C_CLIENT_WAKE);
>
> ===
>
> I have to admit that I am not familiar with device wakeup handling and
> especially its userspace policies. Can you double check that your V2
> meets the above intention?
No it does not; it explicitly resets the wakeup flag. Note that the
original code was not quite right in that regard either: it would
preserve wakeup flag set by userspace upon driver rebinding; but it
would re-arm the wakeup flag if it was disabled by userspace.
I believe that resetting the flag upon re-binding the driver is proper
behavior as the driver is responsible for setting up and handling
wakeups.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree
2015-08-19 17:51 ` Dmitry Torokhov
@ 2015-08-24 12:33 ` Wolfram Sang
0 siblings, 0 replies; 11+ messages in thread
From: Wolfram Sang @ 2015-08-24 12:33 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mika Westerberg, Rafael J. Wysocki, Ulf Hansson, Vignesh R,
Tony Lindgren, Rob Herring, Mark Rutland, Linux I2C, lkml
[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]
> > When reviewing V2, I wasn't comfortable with just guessing what the old
> > code means. So, I did some digging and found:
> >
> > https://lkml.org/lkml/2008/8/10/204
> >
> > Quoting the interesting paragraph from David Brownell:
> >
> > ===
> >
> > Better would be to preserve any existing settings:
> >
> > if (!device_can_wakeup(&client->dev))
> > device_init_wakeup(...)
> > That way the userspace policy setting is preserved unless the
> > device itself gets removed ... instead of being clobbered by
> > the simple act of (re)probing a driver.
> >
> >> > + device_init_wakeup(&client->dev, client->flags &
> >> > I2C_CLIENT_WAKE);
> >
> > ===
> >
> > I have to admit that I am not familiar with device wakeup handling and
> > especially its userspace policies. Can you double check that your V2
> > meets the above intention?
>
> No it does not; it explicitly resets the wakeup flag. Note that the
> original code was not quite right in that regard either: it would
> preserve wakeup flag set by userspace upon driver rebinding; but it
> would re-arm the wakeup flag if it was disabled by userspace.
>
> I believe that resetting the flag upon re-binding the driver is proper
> behavior as the driver is responsible for setting up and handling
> wakeups.
Okay, that meets my idea of how this should work. I rephrased the above
paragraph slightly and added it to the commit message of V2.
Thanks,
Wolfram
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-08-24 12:33 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-30 20:14 [PATCH] i2c: allow specifying separate wakeup interrupt in device tree Dmitry Torokhov
2015-07-31 10:57 ` Vignesh R
[not found] ` <55BB54B1.80603-l0cyMroinI0@public.gmane.org>
2015-08-03 10:21 ` Tony Lindgren
[not found] ` <20150803102121.GO16878-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
2015-08-03 20:02 ` Dmitry Torokhov
2015-08-05 13:33 ` Tony Lindgren
2015-08-09 15:22 ` Wolfram Sang
2015-08-10 5:59 ` Dmitry Torokhov
2015-08-10 6:16 ` Wolfram Sang
2015-08-19 17:43 ` Wolfram Sang
2015-08-19 17:51 ` Dmitry Torokhov
2015-08-24 12:33 ` Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).