Re: [PATCH] MFD : add MAX77686 mfd driver

[jonghwa3 . lee]

Hi Mark,

You point out many things. ;-) Thanks for your concern.
I want to explain one by one.

You mentioned 5 things in your e-mail.

1. I agreed that option is unnecessary, so i decided to remove it.
	(#ifdef CONFIG_RTC_DRV_MAX77686 )

2. Those steps is needed for wake-up interrupt occurred by 77686 irq.
   When system has woken up from Suspend-to-RAM state by 77686 irq,
	then it never call irq handler.

3. I accept your opinion and will apply it.

4. This will work when I2C interface is changed according to board
	revision with board configuration.

5. This is for some board that doesn't use RTC. (e.g. SMDK board)


Thanks,

On 2012-05-02 01:58, Mark Brown wrote:

> 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.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/