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.
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Wed, May 02, 2012 at 02:01:16PM +0900, [email protected] wrote:
> Hi Mark,
>
> You point out many things. ;-) Thanks for your concern.
> I want to explain one by one.
Don't top post! http://daringfireball.net/2007/07/on_top
> 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.
Why? Are you sure you aren't doing something like requesting the wrong
trigger type for the interrupt?
> 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)
Since you top posted I'm really not sure which comments you're referring
to with these...