[PATCH] i2c: allow specifying separate wakeup interrupt in device tree

[PATCH] i2c: allow specifying separate wakeup interrupt in device tree

[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 <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(-)

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

Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree

[Vignesh R]

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

Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree

[Tony Lindgren]

* 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

Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree

[Dmitry Torokhov]

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

Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree

[Tony Lindgren]

* 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

Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2921 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@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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree

[Wolfram Sang]

[-- 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 --]

Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree

[Wolfram Sang]

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

Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree

[Dmitry Torokhov]

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

Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree

[Wolfram Sang]

[-- 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 --]