i2c: allow specifying separate wakeup interrupt in device tree

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 <[email protected]> for pixcir_i2c_ts driver.

Signed-off-by Dmitry Torokhov <[email protected]>

---
 drivers/i2c/i2c-core.c | 46 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 6 deletions(-)

drivers/i2c/i2c-core.c +40 -6

index e6d4935..19e7a17 100644
@@ -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;
 }
 

Quoted 4 lines · Dmitry Torokhov

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.

Quoted 30 lines

drivers/i2c/i2c-core.c +14 -4

@@ -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;

drivers/i2c/i2c-core.c static int i2c_device_probe(struct device *dev) · line 680

Quoted 8 lines

drivers/i2c/i2c-core.c +8 -0

+		}
+
+		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;

Quoted 5 lines · Vignesh R

Quoted 4 lines · Dmitry Torokhov

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..

Quoted 32 lines

Quoted 32 lines

drivers/i2c/i2c-core.c +16 -4

@@ -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;
+		}
+

Quoted 6 lines

drivers/i2c/i2c-core.c static int i2c_device_probe(struct device *dev) · line 680

Quoted 6 lines

drivers/i2c/i2c-core.c +6 -0

+		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 :)

Quoted 2 lines · Tony Lindgren

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;
	}

Quoted 3 lines

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);

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 <[email protected]> for pixcir_i2c_ts driver.

Signed-off-by Dmitry Torokhov <[email protected]>

drivers/i2c/i2c-core.c +33 -2

+
+	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;
 }
 

Signature · 5 lines

2.5.0.rc2.392.g76e840b


-- 
Dmitry

Quoted 15 lines · Dmitry Torokhov

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 <[email protected]> for pixcir_i2c_ts driver.

Quoted 1 line

Signed-off-by Dmitry Torokhov <[email protected]>

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/

drivers/i2c/i2c-core.c static int i2c_device_probe(struct device *dev) · line 664

Quoted 5 lines

drivers/i2c/i2c-core.c +0 -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);

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??

Quoted 58 lines

drivers/i2c/i2c-core.c +33 -2

+
+	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;
 }
 

Quoted 6 lines

Signature · 5 lines

2.5.0.rc2.392.g76e840b


-- 
Dmitry

Quoted 5 lines

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.

Quoted 11 lines

drivers/i2c/i2c-core.c static int i2c_device_probe(struct device *dev) · line 664

Quoted 6 lines

drivers/i2c/i2c-core.c +0 -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);

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.

Quoted 11 lines

Quoted 10 lines

drivers/i2c/i2c-core.c static int i2c_device_probe(struct device *dev) · line 664

Quoted 5 lines

drivers/i2c/i2c-core.c +0 -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);

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??

Quoted 4 lines

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.

Quoted 2 lines

(quoted diff) +1 -0

+	device_init_wakeup(&client->dev, client->flags &

I2C_CLIENT_WAKE);

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

drivers/i2c/i2c-core.c static int i2c_device_probe(struct device *dev) · line 664

Quoted 5 lines

drivers/i2c/i2c-core.c +0 -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);

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??

Quoted 7 lines

Quoted 4 lines

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.

Quoted 2 lines

Quoted 2 lines

+ 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?

Quoted 18 lines

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.

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

+ 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.