Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1203653 > unrolled thread

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

Started byWolfram Sang <wsa@the-dreams.de>
First post2015-08-09 17:30 +0200
Last post2015-08-19 20:00 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] i2c: allow specifying separate wakeup interrupt in  device tree Wolfram Sang <wsa@the-dreams.de> - 2015-08-09 17:30 +0200
    Re: [PATCH] i2c: allow specifying separate wakeup interrupt in  device tree Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2015-08-10 08:00 +0200
      Re: [PATCH] i2c: allow specifying separate wakeup interrupt in  device tree Wolfram Sang <wsa@the-dreams.de> - 2015-08-10 08:20 +0200
        Re: [PATCH] i2c: allow specifying separate wakeup interrupt in  device tree Wolfram Sang <wsa@the-dreams.de> - 2015-08-19 19:50 +0200
          Re: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2015-08-19 20:00 +0200

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

FromWolfram Sang <wsa@the-dreams.de>
Date2015-08-09 17:30 +0200
SubjectRe: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree
Message-ID<pVAHU-1zK-3@gated-at.bofh.it>

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

#1203773

FromDmitry Torokhov <dmitry.torokhov@gmail.com>
Date2015-08-10 08:00 +0200
Message-ID<pVOhQ-535-5@gated-at.bofh.it>
In reply to#1203653
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1203786

FromWolfram Sang <wsa@the-dreams.de>
Date2015-08-10 08:20 +0200
Message-ID<pVOBc-5JE-31@gated-at.bofh.it>
In reply to#1203773

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1209966

FromWolfram Sang <wsa@the-dreams.de>
Date2015-08-19 19:50 +0200
Message-ID<pZfER-4uL-5@gated-at.bofh.it>
In reply to#1203786
> > > > @@ -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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

FromDmitry Torokhov <dmitry.torokhov@gmail.com>
Date2015-08-19 20:00 +0200
SubjectRe: [PATCH] i2c: allow specifying separate wakeup interrupt in device tree
Message-ID<pZfOx-4G1-1@gated-at.bofh.it>
In reply to#1209966
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web