On Thu, Jul 26, 2012 at 04:07:12PM -0600, Stephen Warren wrote:

On 07/26/2012 02:35 PM, Mark Brown wrote:

quoted

On Thu, Jul 26, 2012 at 01:40:30PM -0600, Stephen Warren wrote:

quoted

quoted

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

quoted

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.