On Thu, Jul 26, 2012 at 01:40:30PM -0600, Stephen Warren wrote: > +struct max8907_irq_data { > + int reg; > + int mask_reg; > + int offs; /* bit offset in mask register */ > + bool is_rtc; > +};
This (and all the code in here) looks very much like regmap-irq (or one of the pre-regmap drivers I wrote which were factored out to there)... why can't we use regmap_irq? Looking at the code it looks like a very similar pattern to the arizona chips where you've got two IRQ domains in the chip which can be handled with a single virtual IRQ to do the demux. We could factor that out too easily enough, I might just do that... > + if (!irqd_irq_disabled(d) && (value & irq_data->offs)) { This looks very suspicious... why do we need to call irqd_irq_disabled() here? > + regmap_write(chip->regmap_gen, MAX8907_REG_CHG_IRQ1_MASK, irq_chg[0]); > + regmap_write(chip->regmap_gen, MAX8907_REG_CHG_IRQ2_MASK, irq_chg[1]); > + regmap_write(chip->regmap_gen, MAX8907_REG_ON_OFF_IRQ1_MASK, > + irq_on[0]); > + regmap_write(chip->regmap_gen, MAX8907_REG_ON_OFF_IRQ2_MASK, > + irq_on[1]); > + regmap_write(chip->regmap_rtc, MAX8907_REG_RTC_IRQ_MASK, irq_rtc); If you have the cache enabled regmap_update_bits() is your friend here, it'll suppress duplicate I/O. > +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. > + 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. > +static const struct regmap_config max8907_regmap_gen_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .volatile_reg = max8907_gen_is_volatile_reg, > + .writeable_reg = max8907_gen_is_writeable_reg, > + .max_register = MAX8907_REG_LDO20VOUT, > + .cache_type = REGCACHE_RBTREE, > +}; Your IRQ registers appear to be clear on read which means you should have a precious_reg callback too otherwise someone looking at the register map in debugfs can ack interrupts.
signature.asc
Description: Digital signature