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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150810055936.GB26735@dtor-ws>
Date:	Sun, 9 Aug 2015 22:59:36 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Wolfram Sang <wsa@...-dreams.de>
Cc:	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	Vignesh R <vigneshr@...com>, Tony Lindgren <tony@...mide.com>,
	Rob Herring <robh+dt@...nel.org>,
	Mark Rutland <mark.rutland@....com>, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i2c: allow specifying separate wakeup interrupt in
 device tree

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@...com> for
> > pixcir_i2c_ts driver.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/