Re: [PATCH] mfd: add MAX8907 core driver

[Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>]

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.

Attachment: signature.asc
Description: Digital signature