2012-05-11 14:05:38

by Samuel Ortiz

[permalink] [raw]
Subject: Re: [RESEND PATCH v2 1/2] MFD: MAX77693: add MAX77693 MFD driver

Hi Choi,

On Tue, Mar 20, 2012 at 10:07:58AM +0900, Chanwoo Choi wrote:
> @@ -0,0 +1,224 @@
> +/*
> + * max77693.c - mfd core driver for the MAX 77693
> + *
> + * Copyright (C) 2011 Samsung Electronics
2012 ?

> +int max77693_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
> +{
> + struct max77693_dev *max77693 = i2c_get_clientdata(i2c);
> + int ret;
> +
> + mutex_lock(&max77693->iolock);
> + ret = i2c_smbus_read_byte_data(i2c, reg);
> + mutex_unlock(&max77693->iolock);
You don't need this locking as the i2c layer will do it for you.
Also, this definitely look like a good candidate for a regmap API conversion,
I'd appreciate if you could work on that.

> +static struct i2c_driver max77693_i2c_driver = {
> + .driver = {
> + .name = "max77693",
> + .owner = THIS_MODULE,
> + },
> + .probe = max77693_i2c_probe,
> + .remove = max77693_i2c_remove,
> + .id_table = max77693_i2c_id,
> +};
> +
> +static int __init max77693_i2c_init(void)
> +{
> + return i2c_add_driver(&max77693_i2c_driver);
> +}
> +/* init early so consumer devices can complete system boot */
> +subsys_initcall(max77693_i2c_init);
> +
> +static void __exit max77693_i2c_exit(void)
> +{
> + i2c_del_driver(&max77693_i2c_driver);
> +}
> +module_exit(max77693_i2c_exit);
You could use module_i2c_driver() here.

Cheers,
Samuel.

--
Intel Open Source Technology Centre
http://oss.intel.com/


2012-05-11 14:25:23

by Kyungmin Park

[permalink] [raw]
Subject: Re: [RESEND PATCH v2 1/2] MFD: MAX77693: add MAX77693 MFD driver

Hi Samuel,

On Fri, May 11, 2012 at 11:15 PM, Samuel Ortiz <[email protected]> wrote:
> Hi Choi,
>
> On Tue, Mar 20, 2012 at 10:07:58AM +0900, Chanwoo Choi wrote:
>> @@ -0,0 +1,224 @@
>> +/*
>> + * max77693.c - mfd core driver for the MAX 77693
>> + *
>> + * Copyright (C) 2011 Samsung Electronics
> 2012 ?
>
>> +int max77693_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
>> +{
>> + ? ? struct max77693_dev *max77693 = i2c_get_clientdata(i2c);
>> + ? ? int ret;
>> +
>> + ? ? mutex_lock(&max77693->iolock);
>> + ? ? ret = i2c_smbus_read_byte_data(i2c, reg);
>> + ? ? mutex_unlock(&max77693->iolock);
> You don't need this locking as the i2c layer will do it for you.
> Also, this definitely look like a good candidate for a regmap API conversion,
> I'd appreciate if you could work on that.

Right, it's already done. you can find the updated patches in your mail box.

[PATCH v2 0/3] mfd: MAX77686: Add initial support for MAXIM 77686 mfd chip

This patchset adds suppport for MAX77686 which is a multifunction
device including
regulator and rtc. It also contains drivers supporting rtc and regulator.
All drivers ard based on MAX8997 dirvers and use regmap to access to
the inner registers.
To manage IRQs occuered by max77686, it supports IRQ domain.

Thank you,
Kyungmin Park
>
>> +static struct i2c_driver max77693_i2c_driver = {
>> + ? ? .driver = {
>> + ? ? ? ? ? ? ? ?.name = "max77693",
>> + ? ? ? ? ? ? ? ?.owner = THIS_MODULE,
>> + ? ? },
>> + ? ? .probe = max77693_i2c_probe,
>> + ? ? .remove = max77693_i2c_remove,
>> + ? ? .id_table = max77693_i2c_id,
>> +};
>> +
>> +static int __init max77693_i2c_init(void)
>> +{
>> + ? ? return i2c_add_driver(&max77693_i2c_driver);
>> +}
>> +/* init early so consumer devices can complete system boot */
>> +subsys_initcall(max77693_i2c_init);
>> +
>> +static void __exit max77693_i2c_exit(void)
>> +{
>> + ? ? i2c_del_driver(&max77693_i2c_driver);
>> +}
>> +module_exit(max77693_i2c_exit);
> You could use module_i2c_driver() here.
>
> Cheers,
> Samuel.
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
> --
> 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/

2012-05-11 14:28:44

by Kyungmin Park

[permalink] [raw]
Subject: Re: [RESEND PATCH v2 1/2] MFD: MAX77693: add MAX77693 MFD driver

On Fri, May 11, 2012 at 11:25 PM, Kyungmin Park <[email protected]> wrote:
> Hi Samuel,
>
> On Fri, May 11, 2012 at 11:15 PM, Samuel Ortiz <[email protected]> wrote:
>> Hi Choi,
>>
>> On Tue, Mar 20, 2012 at 10:07:58AM +0900, Chanwoo Choi wrote:
>>> @@ -0,0 +1,224 @@
>>> +/*
>>> + * max77693.c - mfd core driver for the MAX 77693
>>> + *
>>> + * Copyright (C) 2011 Samsung Electronics
>> 2012 ?
>>
>>> +int max77693_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
>>> +{
>>> + ? ? struct max77693_dev *max77693 = i2c_get_clientdata(i2c);
>>> + ? ? int ret;
>>> +
>>> + ? ? mutex_lock(&max77693->iolock);
>>> + ? ? ret = i2c_smbus_read_byte_data(i2c, reg);
>>> + ? ? mutex_unlock(&max77693->iolock);
>> You don't need this locking as the i2c layer will do it for you.
>> Also, this definitely look like a good candidate for a regmap API conversion,
>> I'd appreciate if you could work on that.
>
> Right, it's already done. you can find the updated patches in your mail box.
>
> [PATCH v2 0/3] mfd: MAX77686: Add initial support for MAXIM 77686 mfd chip
>
> This patchset adds suppport for MAX77686 which is a multifunction
> device including
> regulator and rtc. It also contains drivers supporting rtc and regulator.
> All drivers ard based on MAX8997 dirvers and use regmap to access to
> the inner registers.
> To manage IRQs occuered by max77686, it supports IRQ domain.
>
Sorry, I'm confused. it's another PMIC IC. I'll check the max77693 also. :);
> Thank you,
> Kyungmin Park
>>
>>> +static struct i2c_driver max77693_i2c_driver = {
>>> + ? ? .driver = {
>>> + ? ? ? ? ? ? ? ?.name = "max77693",
>>> + ? ? ? ? ? ? ? ?.owner = THIS_MODULE,
>>> + ? ? },
>>> + ? ? .probe = max77693_i2c_probe,
>>> + ? ? .remove = max77693_i2c_remove,
>>> + ? ? .id_table = max77693_i2c_id,
>>> +};
>>> +
>>> +static int __init max77693_i2c_init(void)
>>> +{
>>> + ? ? return i2c_add_driver(&max77693_i2c_driver);
>>> +}
>>> +/* init early so consumer devices can complete system boot */
>>> +subsys_initcall(max77693_i2c_init);
>>> +
>>> +static void __exit max77693_i2c_exit(void)
>>> +{
>>> + ? ? i2c_del_driver(&max77693_i2c_driver);
>>> +}
>>> +module_exit(max77693_i2c_exit);
>> You could use module_i2c_driver() here.
>>
>> Cheers,
>> Samuel.
>>
>> --
>> Intel Open Source Technology Centre
>> http://oss.intel.com/
>> --
>> 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/

2012-05-13 10:07:08

by Mark Brown

[permalink] [raw]
Subject: Re: [RESEND PATCH v2 1/2] MFD: MAX77693: add MAX77693 MFD driver

On Fri, May 11, 2012 at 04:15:08PM +0200, Samuel Ortiz wrote:
> On Tue, Mar 20, 2012 at 10:07:58AM +0900, Chanwoo Choi wrote:

> > +static int __init max77693_i2c_init(void)
> > +{
> > + return i2c_add_driver(&max77693_i2c_driver);
> > +}
> > +/* init early so consumer devices can complete system boot */
> > +subsys_initcall(max77693_i2c_init);
> > +
> > +static void __exit max77693_i2c_exit(void)
> > +{
> > + i2c_del_driver(&max77693_i2c_driver);
> > +}
> > +module_exit(max77693_i2c_exit);

> You could use module_i2c_driver() here.

That doesn't work so well for PMICs yet - since cpufreq still doesn't
use struct device it can't use -EPROBE_DEFER which means that we need
the regulators to register before the cpufreq drivers and currently
we're doing that with the subsys_initcall() hack. Once cpufreq can
defer probes we should be able to stop doing this, it's the only blocker
at the minute.

2012-05-14 05:04:27

by Chanwoo Choi

[permalink] [raw]
Subject: Re: [RESEND PATCH v2 1/2] MFD: MAX77693: add MAX77693 MFD driver

Hi Samuel,

On 05/11/2012 11:15 PM, Samuel Ortiz wrote:

> Hi Choi,
>
> On Tue, Mar 20, 2012 at 10:07:58AM +0900, Chanwoo Choi wrote:
>> @@ -0,0 +1,224 @@
>> +/*
>> + * max77693.c - mfd core driver for the MAX 77693
>> + *
>> + * Copyright (C) 2011 Samsung Electronics
> 2012 ?

I fix it.

>
>> +int max77693_read_reg(struct i2c_client *i2c, u8 reg, u8 *dest)
>> +{
>> + struct max77693_dev *max77693 = i2c_get_clientdata(i2c);
>> + int ret;
>> +
>> + mutex_lock(&max77693->iolock);
>> + ret = i2c_smbus_read_byte_data(i2c, reg);
>> + mutex_unlock(&max77693->iolock);
> You don't need this locking as the i2c layer will do it for you.
> Also, this definitely look like a good candidate for a regmap API conversion,
> I'd appreciate if you could work on that.
>

OK, I will apply regmap for i2c of MAX77693 and post new patchset.

>> +static struct i2c_driver max77693_i2c_driver = {
>> + .driver = {
>> + .name = "max77693",
>> + .owner = THIS_MODULE,
>> + },
>> + .probe = max77693_i2c_probe,
>> + .remove = max77693_i2c_remove,
>> + .id_table = max77693_i2c_id,
>> +};
>> +
>> +static int __init max77693_i2c_init(void)
>> +{
>> + return i2c_add_driver(&max77693_i2c_driver);
>> +}
>> +/* init early so consumer devices can complete system boot */
>> +subsys_initcall(max77693_i2c_init);
>> +
>> +static void __exit max77693_i2c_exit(void)
>> +{
>> + i2c_del_driver(&max77693_i2c_driver);
>> +}
>> +module_exit(max77693_i2c_exit);
> You could use module_i2c_driver() here.


As Mark Brown said, I maintain it.

Thank you,

Best Regards,
Chanwoo Choi