linux-kernel - Re: [PATCH] mfd: add MAX8907 core driver

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]

Date:	Thu, 26 Jul 2012 23:16:04 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Samuel Ortiz <sameo@...ux.intel.com>,
	Laxman Dewangan <ldewangan@...dia.com>,
	Liam Girdwood <lrg@...com>, linux-kernel@...r.kernel.org,
	devicetree-discuss@...ts.ozlabs.org,
	Gyungoh Yoo <jack.yoo@...im-ic.com>,
	Stephen Warren <swarren@...dia.com>
Subject: Re: [PATCH] mfd: add MAX8907 core driver

On Thu, Jul 26, 2012 at 04:07:12PM -0600, Stephen Warren wrote:
> On 07/26/2012 02:35 PM, Mark Brown wrote:
> > On Thu, Jul 26, 2012 at 01:40:30PM -0600, Stephen Warren wrote:

> >> +		if (irqd_is_wakeup_set(d)) {
> >> +			/* 1 -- disable, 0 -- enable */
> >> +			switch (irq_data->mask_reg) {

> > This loop we should just port over into the regmap code.

> I assume the best way of doing this is to add new functions
> regmap_irq_suspend()/regmap_irq_resume() (which would mask any enabled
> interrupts that were not wake enabled); that way, the regmap_irq code
> can loop over each register and just write it once. An alternative might
> be to implement struct irq_chip's .irq_suspend/.irq_resume ops, but that
> might worst-case end up with an I2C write per interrupt.

irq_suspend() and irq_resume() are only supposed to be called once per
irq_chip so there should be no concern with using them.  Even if they
weren't it's probably not that performance critical really.

> I see that the MAX8907 IRQ code does this in suspend:

> 	if (device_may_wakeup(chip->dev))
> 		enable_irq_wake(i2c->irq);
> 	else
> 		disable_irq(i2c->irq);

> and this in resume:

> 	if (device_may_wakeup(chip->dev))
> 		disable_irq_wake(i2c->irq);
> 	else
> 		enable_irq(i2c->irq);

> neither of which are done in regmap_irq, since it doesn't explicitly do
> anything for suspend/resume at the moment. Are those code blocks
> necessary? I see that regmap_irq_sync_unlock() is already calling
> irq_set_irq_wake(), which implies that suspend/resume may have already
> been completely taken care of?

Yes, it should already be taken care of.  What the calls here are doing
is mostly allowing userspace to explicitly override the wake state on a
per chip basis.  I'm not convinced it's terribly clever to implement
explicit wake support on an interrupt controller, it seems prone to
confusion.  We could do that though.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)