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_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. >> +static void max8907_irq_enable(struct irq_data *data) +{ + /* >> Everything happens in max8907_irq_sync_unlock */ +} > >> +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. > >> +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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/