Re: [PATCH] MFD : add MAX77686 mfd driver

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

On Mon, Apr 30, 2012 at 05:53:42PM +0900, Jonghwa Lee wrote:

> +	if (irq_src & MAX77686_IRQSRC_RTC) {
> +#ifdef CONFIG_RTC_DRV_MAX77686
> +		ret = max77686_read_reg(max77686->rtc, MAX77686_RTC_INT,
> +					 &irq_reg[RTC_INT]);
> +#else
> +		ret = -ENODEV;
> +#endif

Why is this in an ifdef?  It's not really idiomatic to do this and if
the interrupt is never requested presumably everything should be fine.

> +int max77686_irq_resume(struct max77686_dev *max77686)
> +{
> +	if (max77686->irq && max77686->irq_base)
> +		max77686_irq_thread(max77686->irq_base, max77686);
> +	return 0;
> +}

Why is this needed?  I'd expect the parent IRQ controller to notice if
the device is asserting an interrupt when it resumes.

> +int max77686_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
> +{
> +	struct max77686_dev *max77686 = i2c_get_clientdata(i2c);
> +	int ret;
> +
> +	mutex_lock(&max77686->iolock);
> +	ret = i2c_smbus_read_byte_data(i2c, reg);
> +	mutex_unlock(&max77686->iolock);
> +	if (ret < 0)
> +		return ret;

It would be much better to use regmap for the register I/O since this is
a PMIC and at least the regulator framework (possibly others soon) are
starting to abstract things out using it.  It also gets you things like
the debugfs dumps of the register map and so on easily.

> +	} else
> +		dev_info(max77686->dev, "device found\n");

This isn't relly adding much - can you log a chip revision or anything?

> +#ifdef CONFIG_RTC_DRV_MAX77686
> +	max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
> +	i2c_set_clientdata(max77686->rtc, max77686);
> +#endif

Again, it's very odd that this is conditional.