Re: [PATCH] mfd: add MAX8907 core driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

On Thu, Jul 26, 2012 at 03:14:21PM -0600, Stephen Warren wrote:
> On 07/26/2012 02:35 PM, Mark Brown wrote:

> > This looks very suspicious...  why do we need to call 
> > irqd_irq_disabled() here?

> I believe the status register reflects the unmasked status, it's just
> the interrupt signal that's affected by the mask.

This is totally normal, the standard pattern here is to and the status
register with the mask register before parsing.

> So the idea here was that the IRQ core is already maintaining state
> which describes which IRQs are enabled/disabled and wake/not. Rather
> than have irq_enable/irq_disable/set_wake do nothing but save the same
> state to irq_chip-specific structures, I removed the body of those
> functions and instead just call irqd_irq_disabled() etc. wherever I
> would have accessed the "local" state. Is that not a legitimate design
> then?

It seems very smelly, if you were supposed to do this then you wouldn't
have to be defining the empty operations.  There's also the counter
argument that it means you've got to go through every interrupt and work
out the state before syncing rather than just being able to blast the
register states in but that's fairly weak.

I think it's a totally sensible idea to reuse the state in the core, but
I do think it's a bad idea to dance around the core's back like this
with the empty operations.

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