| Message ID | 20150730201431.GA5255@dtor-ws |
|---|---|
| State | Superseded |
| Headers | show |
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; > } > >
* Vignesh R <vigneshr@ti.com> [150731 04:00]: > 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. Good idea, yes the dedicated wake-up interrupts can be handled at the bus level to keep device drivers generic. One question below though.. > > @@ -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; Hmm why do we need the check for if (device_can_wakeup(&client->dev)))? Also wondering about the dev vs &client->dev usage here.. But I take you have checked that we end up calling the runtime PM calls of the client instead of the i2c bus controller :) Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tony, On Mon, Aug 03, 2015 at 03:21:21AM -0700, Tony Lindgren wrote: > * Vignesh R <vigneshr@ti.com> [150731 04:00]: > > 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. > > Good idea, yes the dedicated wake-up interrupts can be handled > at the bus level to keep device drivers generic. > > One question below though.. > > > > @@ -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; > > Hmm why do we need the check for if (device_can_wakeup(&client->dev)))? Because of the code in device_wakeup_attach_irq(): ws = dev->power.wakeup; if (!ws) { dev_err(dev, "forgot to call call device_init_wakeup?\n"); return -EINVAL; } > > Also wondering about the dev vs &client->dev usage here.. But I take > you have checked that we end up calling the runtime PM calls of the > client instead of the i2c bus controller :) dev *is* clent->dev in this context: struct i2c_client *client = i2c_verify_client(dev); Thanks!
* Dmitry Torokhov <dmitry.torokhov@gmail.com> [150803 13:05]: > On Mon, Aug 03, 2015 at 03:21:21AM -0700, Tony Lindgren wrote: > > > > Hmm why do we need the check for if (device_can_wakeup(&client->dev)))? > > Because of the code in device_wakeup_attach_irq(): > > ws = dev->power.wakeup; > if (!ws) { > dev_err(dev, "forgot to call call device_init_wakeup?\n"); > return -EINVAL; > } OK :) > > Also wondering about the dev vs &client->dev usage here.. But I take > > you have checked that we end up calling the runtime PM calls of the > > client instead of the i2c bus controller :) > > dev *is* clent->dev in this context: > > struct i2c_client *client = i2c_verify_client(dev); OK thanks for confirming that. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@ti.com> for > pixcir_i2c_ts driver. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> 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
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@ti.com> for > > pixcir_i2c_ts driver. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > 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.
> > 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.
> > > > @@ -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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
> > 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
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; }
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> --- drivers/i2c/i2c-core.c | 46 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 6 deletions(-)