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

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

quoted

+		if (!irqd_irq_disabled(d) && (value & irq_data->offs)) {

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.

quoted

+static void max8907_irq_enable(struct irq_data *data) +{ +	/*
Everything happens in max8907_irq_sync_unlock */ +}

quoted

+static void max8907_irq_disable(struct irq_data *data) +{ +	/*
Everything happens in max8907_irq_sync_unlock */ +}

The fact that these functions are empty is the second part of the
above suspicous check for disabled IRQs.  We're just completely
ignoring the caller here.  What would idiomatically happen is that
we'd update a variable here then write it out in the unmask.

If these functions really should be empty then they should be
omitted.

quoted

+static int max8907_irq_set_wake(struct irq_data *data, unsigned
int on) +{ +	/* Everything happens in max8907_irq_sync_unlock */ 
+ +	return 0; +}

Again, this doesn't look clever at all.

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?