diff mbox

[V2,5/6] rtc: max77xxx: add RTC driver for Maxim MAX77xxx series RTC IP

Message ID 1452590273-16421-6-git-send-email-ldewangan@nvidia.com
State New
Headers show

Commit Message

Laxman Dewangan Jan. 12, 2016, 9:17 a.m. UTC
Maxim Semiconductor's PMIC MAX77620, MAX77686, MAX20024 have
same RTC IP on these PMICs.

Add generic MAX77xxxx series RTC driver which can be used as
RTC driver for these PMIC and avoids duplication of RTC driver
for each PMICs. Their MFD driver can be different here.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V1: 
- Rename the file to rtc-max77xxx.c and make the generic implementation.
- Direct regmap apis are used for the register access.
- Decouped from max77620 driver.
- Taken care of cleanup comments form V1 version.

 drivers/rtc/Kconfig        |  10 +
 drivers/rtc/Makefile       |   1 +
 drivers/rtc/rtc-max77xxx.c | 500 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 511 insertions(+)
 create mode 100644 drivers/rtc/rtc-max77xxx.c

Comments

Krzysztof Kozlowski Jan. 13, 2016, 12:06 a.m. UTC | #1
On 12.01.2016 18:17, Laxman Dewangan wrote:
> Maxim Semiconductor's PMIC MAX77620, MAX77686, MAX20024 have
> same RTC IP on these PMICs.
> 
> Add generic MAX77xxxx series RTC driver which can be used as
> RTC driver for these PMIC and avoids duplication of RTC driver
> for each PMICs. Their MFD driver can be different here.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
> Changes from V1: 
> - Rename the file to rtc-max77xxx.c and make the generic implementation.
> - Direct regmap apis are used for the register access.
> - Decouped from max77620 driver.
> - Taken care of cleanup comments form V1 version.
> 
>  drivers/rtc/Kconfig        |  10 +
>  drivers/rtc/Makefile       |   1 +
>  drivers/rtc/rtc-max77xxx.c | 500 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 511 insertions(+)
>  create mode 100644 drivers/rtc/rtc-max77xxx.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 376322f..4972dd5 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -315,6 +315,16 @@ config RTC_DRV_MAX8997
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-max8997.
>  
> +config RTC_DRV_MAX77XXX
> +	tristate "Maxim MAX77XXX series generic RTC driver"
> +	help
> +	  If you say yes here you will get support for the generic RTC driver
> +	  for Maxim Semiconductor MAX77XXX series of PMIC like MAX77620.
> +	  This also supports the RTC driver for Maxim PMIC MaX20024 which
> +	  is almost same as MAX77620.
> +	  This driver can also be built as a module. If so, the module
> +	  will be called rtc-max77xxx.
> +

That was not the consensus... You still added a new driver - but now
with different name.

That is useless duplication

Please work with existing code. Use existing maxim RTC drivers: either
max77686 or max77802.

There is no need for new one.

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laxman Dewangan Jan. 13, 2016, 4:07 a.m. UTC | #2
On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote:
> On 12.01.2016 18:17, Laxman Dewangan wrote:
>> Maxim Semiconductor's PMIC MAX77620, MAX77686, MAX20024 have
>> same RTC IP on these PMICs.
>>
>> Add generic MAX77xxxx series RTC driver which can be used as
>> RTC driver for these PMIC and avoids duplication of RTC driver
>> for each PMICs. Their MFD driver can be different here.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>> ---
>> Changes from V1:
>> - Rename the file to rtc-max77xxx.c and make the generic implementation.
>> - Direct regmap apis are used for the register access.
>> - Decouped from max77620 driver.
>> - Taken care of cleanup comments form V1 version.
>>
>>   drivers/rtc/Kconfig        |  10 +
>>   drivers/rtc/Makefile       |   1 +
>>   drivers/rtc/rtc-max77xxx.c | 500 +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 511 insertions(+)
>>   create mode 100644 drivers/rtc/rtc-max77xxx.c
>>
>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>> index 376322f..4972dd5 100644
>> --- a/drivers/rtc/Kconfig
>> +++ b/drivers/rtc/Kconfig
>> @@ -315,6 +315,16 @@ config RTC_DRV_MAX8997
>>   	  This driver can also be built as a module. If so, the module
>>   	  will be called rtc-max8997.
>>   
>> +config RTC_DRV_MAX77XXX
>> +	tristate "Maxim MAX77XXX series generic RTC driver"
>> +	help
>> +	  If you say yes here you will get support for the generic RTC driver
>> +	  for Maxim Semiconductor MAX77XXX series of PMIC like MAX77620.
>> +	  This also supports the RTC driver for Maxim PMIC MaX20024 which
>> +	  is almost same as MAX77620.
>> +	  This driver can also be built as a module. If so, the module
>> +	  will be called rtc-max77xxx.
>> +
> That was not the consensus... You still added a new driver - but now
> with different name.
>
> That is useless duplication
>
> Please work with existing code. Use existing maxim RTC drivers: either
> max77686 or max77802.
>
> There is no need for new one.

If we modify the existing one then that work will be outside of this 
series to make it independent.

However, the file name does not suggest common in older file. Also this 
will require mfd and rtc driver changes to decouple it.

Here is my approach:
- Let's have common driver in implementation and file name. This will be 
independent of the mfd driver on all sense.
- Once it is merged, move the max77686 and max77802 to use this driver, 
this will need the modification on the mfd driver, related defconfig 
file and if specific stuff needed in the rtc then addition of that.

Per your approach:
- Modify rtc max77686 and mfd driver max77686 to decouple and proper 
registration.
- Add support of max77620 on the max77686 driver if any specific is 
required.

That is also fine to me but still I am not comfortable with the config 
name and driver file name as this does not suggest the common.




--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laxman Dewangan Jan. 13, 2016, 4:25 a.m. UTC | #3
On Wednesday 13 January 2016 09:58 AM, Krzysztof Kozlowski wrote:
> On 13.01.2016 13:07, Laxman Dewangan wrote:
>> On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote:
>
>> That is also fine to me but still I am not comfortable with the config
>> name and driver file name as this does not suggest the common.
> The name does not matter. Really. We have a lot of drivers with a
> specific device-like name and supporting different devices. To point
> that your argument is invalid - your initial name of driver
> "rtc-max77620.c" supported totally different "names": the max77620 and
> max20024. It also wasn't suggesting something "common"...
In all config string, I have mentioned the MAX20024.


> With my approach we are not developing common think neither. We just
> want to extend/re-use existing max77686 (or max77802) driver for new
> devices. Just like everywhere else.

OK, fine to me if this is acceptable.
I will drop this rtc patch form this series and work in max77686 driver 
to modify first and once merged, use this config on my defconfig.

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Jan. 13, 2016, 4:28 a.m. UTC | #4
On 13.01.2016 13:07, Laxman Dewangan wrote:
> 
> On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote:
>> On 12.01.2016 18:17, Laxman Dewangan wrote:
>>> Maxim Semiconductor's PMIC MAX77620, MAX77686, MAX20024 have
>>> same RTC IP on these PMICs.
>>>
>>> Add generic MAX77xxxx series RTC driver which can be used as
>>> RTC driver for these PMIC and avoids duplication of RTC driver
>>> for each PMICs. Their MFD driver can be different here.
>>>
>>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>>> ---
>>> Changes from V1:
>>> - Rename the file to rtc-max77xxx.c and make the generic implementation.
>>> - Direct regmap apis are used for the register access.
>>> - Decouped from max77620 driver.
>>> - Taken care of cleanup comments form V1 version.
>>>
>>>   drivers/rtc/Kconfig        |  10 +
>>>   drivers/rtc/Makefile       |   1 +
>>>   drivers/rtc/rtc-max77xxx.c | 500
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 511 insertions(+)
>>>   create mode 100644 drivers/rtc/rtc-max77xxx.c
>>>
>>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>>> index 376322f..4972dd5 100644
>>> --- a/drivers/rtc/Kconfig
>>> +++ b/drivers/rtc/Kconfig
>>> @@ -315,6 +315,16 @@ config RTC_DRV_MAX8997
>>>         This driver can also be built as a module. If so, the module
>>>         will be called rtc-max8997.
>>>   +config RTC_DRV_MAX77XXX
>>> +    tristate "Maxim MAX77XXX series generic RTC driver"
>>> +    help
>>> +      If you say yes here you will get support for the generic RTC
>>> driver
>>> +      for Maxim Semiconductor MAX77XXX series of PMIC like MAX77620.
>>> +      This also supports the RTC driver for Maxim PMIC MaX20024 which
>>> +      is almost same as MAX77620.
>>> +      This driver can also be built as a module. If so, the module
>>> +      will be called rtc-max77xxx.
>>> +
>> That was not the consensus... You still added a new driver - but now
>> with different name.
>>
>> That is useless duplication
>>
>> Please work with existing code. Use existing maxim RTC drivers: either
>> max77686 or max77802.
>>
>> There is no need for new one.
> 
> If we modify the existing one then that work will be outside of this
> series to make it independent.

And that is the problem? The series evolve. The ultimate goal is to
support max77686, max77802, max77620 and max20024.

> 
> However, the file name does not suggest common in older file.

This is not a sensible argument. The name does not matter. But if really
needed we can rename it...

> Also this
> will require mfd and rtc driver changes to decouple it.

Yes, decouple everything! I like it! :) Make it robust, generic,
readable, fix bugs etc. :)

> 
> Here is my approach:
> - Let's have common driver in implementation and file name. This will be
> independent of the mfd driver on all sense.
> - Once it is merged, move the max77686 and max77802 to use this driver,
> this will need the modification on the mfd driver, related defconfig
> file and if specific stuff needed in the rtc then addition of that.

Nope, because *the second part won't happen*. Never. After merging you
will be happy and another duplicated stuff ends in the kernel.

Fix things before merging. Not after.

> 
> Per your approach:
> - Modify rtc max77686 and mfd driver max77686 to decouple and proper
> registration.
> - Add support of max77620 on the max77686 driver if any specific is
> required.

That is the way we usually extend the drivers for new devices.

> 
> That is also fine to me but still I am not comfortable with the config
> name and driver file name as this does not suggest the common.

The name does not matter. Really. We have a lot of drivers with a
specific device-like name and supporting different devices. To point
that your argument is invalid - your initial name of driver
"rtc-max77620.c" supported totally different "names": the max77620 and
max20024. It also wasn't suggesting something "common"...

With my approach we are not developing common think neither. We just
want to extend/re-use existing max77686 (or max77802) driver for new
devices. Just like everywhere else.

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laxman Dewangan Jan. 13, 2016, 2:59 p.m. UTC | #5
On Wednesday 13 January 2016 09:55 AM, Laxman Dewangan wrote:
>
> On Wednesday 13 January 2016 09:58 AM, Krzysztof Kozlowski wrote:
>> On 13.01.2016 13:07, Laxman Dewangan wrote:
>>> On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote:
>>
>>> That is also fine to me but still I am not comfortable with the config
>>> name and driver file name as this does not suggest the common.
>> The name does not matter. Really. We have a lot of drivers with a
>> specific device-like name and supporting different devices. To point
>> that your argument is invalid - your initial name of driver
>> "rtc-max77620.c" supported totally different "names": the max77620 and
>> max20024. It also wasn't suggesting something "common"...
> In all config string, I have mentioned the MAX20024.
>
>
>> With my approach we are not developing common think neither. We just
>> want to extend/re-use existing max77686 (or max77802) driver for new
>> devices. Just like everywhere else.
>
> OK, fine to me if this is acceptable.
> I will drop this rtc patch form this series and work in max77686 
> driver to modify first and once merged, use this config on my defconfig.
>

Here, MAX686 RTC driver needs two regmap handle, one for the rtc_regmap 
and other for STATUS2 register access.

         ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2, 
&val);
         if (ret < 0) {
                 dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
                                 __func__, __LINE__, ret);
                 goto out;
         }


We can not have two regmap handle on rtc driver as both regmap (pmic and 
rtc) registered with different i2c device.

Also this register should not be accessed by RTC driver if we want to 
decouple as this is very much MAX77686 register set.
Do we need this code?
static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm 
*alrm)
{
         struct max77686_rtc_info *info = dev_get_drvdata(dev);
         u8 data[RTC_NR_TIME];
         unsigned int val;
         int i, ret;

::::::::
        alrm->pending = 0;
         ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2, 
&val);
         if (ret < 0) {
                 dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
                                 __func__, __LINE__, ret);
                 goto out;
         }

         if (val & (1 << 4)) /* RTCA1 */
                 alrm->pending = 1;

}





--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Jan. 14, 2016, 12:50 a.m. UTC | #6
On 13.01.2016 23:59, Laxman Dewangan wrote:
> 
> On Wednesday 13 January 2016 09:55 AM, Laxman Dewangan wrote:
>>
>> On Wednesday 13 January 2016 09:58 AM, Krzysztof Kozlowski wrote:
>>> On 13.01.2016 13:07, Laxman Dewangan wrote:
>>>> On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote:
>>>
>>>> That is also fine to me but still I am not comfortable with the config
>>>> name and driver file name as this does not suggest the common.
>>> The name does not matter. Really. We have a lot of drivers with a
>>> specific device-like name and supporting different devices. To point
>>> that your argument is invalid - your initial name of driver
>>> "rtc-max77620.c" supported totally different "names": the max77620 and
>>> max20024. It also wasn't suggesting something "common"...
>> In all config string, I have mentioned the MAX20024.
>>
>>
>>> With my approach we are not developing common think neither. We just
>>> want to extend/re-use existing max77686 (or max77802) driver for new
>>> devices. Just like everywhere else.
>>
>> OK, fine to me if this is acceptable.
>> I will drop this rtc patch form this series and work in max77686
>> driver to modify first and once merged, use this config on my defconfig.
>>
> 
> Here, MAX686 RTC driver needs two regmap handle, one for the rtc_regmap
> and other for STATUS2 register access.
> 
>         ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
> &val);
>         if (ret < 0) {
>                 dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
>                                 __func__, __LINE__, ret);
>                 goto out;
>         }
> 
> 
> We can not have two regmap handle on rtc driver as both regmap (pmic and
> rtc) registered with different i2c device.
> 
> Also this register should not be accessed by RTC driver if we want to
> decouple as this is very much MAX77686 register set.
> Do we need this code?
> static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm
> *alrm)
> {
>         struct max77686_rtc_info *info = dev_get_drvdata(dev);
>         u8 data[RTC_NR_TIME];
>         unsigned int val;
>         int i, ret;
> 
> ::::::::
>        alrm->pending = 0;
>         ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
> &val);
>         if (ret < 0) {
>                 dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
>                                 __func__, __LINE__, ret);
>                 goto out;
>         }
> 
>         if (val & (1 << 4)) /* RTCA1 */
>                 alrm->pending = 1;
> 
> }

Most of the drivers set the 'pending' field when reading alarm. Your
original driver did not.

The max77802 does exactly the same (BTW, these should be merged as
well... I'll add this to the TODO list) so I think this is necessary.

BR,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Jan. 14, 2016, 1:20 a.m. UTC | #7
On 14.01.2016 09:50, Krzysztof Kozlowski wrote:
> On 13.01.2016 23:59, Laxman Dewangan wrote:
>>
>> On Wednesday 13 January 2016 09:55 AM, Laxman Dewangan wrote:
>>>
>>> On Wednesday 13 January 2016 09:58 AM, Krzysztof Kozlowski wrote:
>>>> On 13.01.2016 13:07, Laxman Dewangan wrote:
>>>>> On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote:
>>>>
>>>>> That is also fine to me but still I am not comfortable with the config
>>>>> name and driver file name as this does not suggest the common.
>>>> The name does not matter. Really. We have a lot of drivers with a
>>>> specific device-like name and supporting different devices. To point
>>>> that your argument is invalid - your initial name of driver
>>>> "rtc-max77620.c" supported totally different "names": the max77620 and
>>>> max20024. It also wasn't suggesting something "common"...
>>> In all config string, I have mentioned the MAX20024.
>>>
>>>
>>>> With my approach we are not developing common think neither. We just
>>>> want to extend/re-use existing max77686 (or max77802) driver for new
>>>> devices. Just like everywhere else.
>>>
>>> OK, fine to me if this is acceptable.
>>> I will drop this rtc patch form this series and work in max77686
>>> driver to modify first and once merged, use this config on my defconfig.
>>>
>>
>> Here, MAX686 RTC driver needs two regmap handle, one for the rtc_regmap
>> and other for STATUS2 register access.
>>
>>         ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
>> &val);
>>         if (ret < 0) {
>>                 dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
>>                                 __func__, __LINE__, ret);
>>                 goto out;
>>         }
>>
>>
>> We can not have two regmap handle on rtc driver as both regmap (pmic and
>> rtc) registered with different i2c device.
>>
>> Also this register should not be accessed by RTC driver if we want to
>> decouple as this is very much MAX77686 register set.
>> Do we need this code?
>> static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm
>> *alrm)
>> {
>>         struct max77686_rtc_info *info = dev_get_drvdata(dev);
>>         u8 data[RTC_NR_TIME];
>>         unsigned int val;
>>         int i, ret;
>>
>> ::::::::
>>        alrm->pending = 0;
>>         ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
>> &val);
>>         if (ret < 0) {
>>                 dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
>>                                 __func__, __LINE__, ret);
>>                 goto out;
>>         }
>>
>>         if (val & (1 << 4)) /* RTCA1 */
>>                 alrm->pending = 1;
>>
>> }
> 
> Most of the drivers set the 'pending' field when reading alarm. Your
> original driver did not.
> 
> The max77802 does exactly the same (BTW, these should be merged as
> well... I'll add this to the TODO list) so I think this is necessary.

How about merging max77802 to max77686 first? The only differences I
found are:
1. It uses main MFD/PMIC regmap.
   This can be solved as part of decoupling code. The driver will get
MFD's regmap and set up its own (only on max77686). The max77802 will
only use parent's regmap.

2. It has different register address.
   We need a register-layout/configuration structure. The logic is the
same except few differences (e.g. presence of MAX77802_RTC_AE1).

It may be easier to merge them now, before adding support for max77620?
I could handle this probably next week or in the following week
(assuming someone would test max77802 because I don't have the hardware).

Anyway I think we should develop these RTC patches having this in mind:
merge all of them.

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laxman Dewangan Jan. 14, 2016, 11:50 a.m. UTC | #8
On Thursday 14 January 2016 06:50 AM, Krzysztof Kozlowski wrote:
> On 14.01.2016 09:50, Krzysztof Kozlowski wrote:
>> On 13.01.2016 23:59, Laxman Dewangan wrote:
>>> On Wednesday 13 January 2016 09:55 AM, Laxman Dewangan wrote:
>>>> On Wednesday 13 January 2016 09:58 AM, Krzysztof Kozlowski wrote:
>>>>> On 13.01.2016 13:07, Laxman Dewangan wrote:
>>>>>> On Wednesday 13 January 2016 05:36 AM, Krzysztof Kozlowski wrote:
>>>>>> That is also fine to me but still I am not comfortable with the config
>>>>>> name and driver file name as this does not suggest the common.
>>>>> The name does not matter. Really. We have a lot of drivers with a
>>>>> specific device-like name and supporting different devices. To point
>>>>> that your argument is invalid - your initial name of driver
>>>>> "rtc-max77620.c" supported totally different "names": the max77620 and
>>>>> max20024. It also wasn't suggesting something "common"...
>>>> In all config string, I have mentioned the MAX20024.
>>>>
>>>>
>>>>> With my approach we are not developing common think neither. We just
>>>>> want to extend/re-use existing max77686 (or max77802) driver for new
>>>>> devices. Just like everywhere else.
>>>> OK, fine to me if this is acceptable.
>>>> I will drop this rtc patch form this series and work in max77686
>>>> driver to modify first and once merged, use this config on my defconfig.
>>>>
>>> Here, MAX686 RTC driver needs two regmap handle, one for the rtc_regmap
>>> and other for STATUS2 register access.
>>>
>>>          ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
>>> &val);
>>>          if (ret < 0) {
>>>                  dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
>>>                                  __func__, __LINE__, ret);
>>>                  goto out;
>>>          }
>>>
>>>
>>> We can not have two regmap handle on rtc driver as both regmap (pmic and
>>> rtc) registered with different i2c device.
>>>
>>> Also this register should not be accessed by RTC driver if we want to
>>> decouple as this is very much MAX77686 register set.
>>> Do we need this code?
>>> static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm
>>> *alrm)
>>> {
>>>          struct max77686_rtc_info *info = dev_get_drvdata(dev);
>>>          u8 data[RTC_NR_TIME];
>>>          unsigned int val;
>>>          int i, ret;
>>>
>>> ::::::::
>>>         alrm->pending = 0;
>>>          ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2,
>>> &val);
>>>          if (ret < 0) {
>>>                  dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
>>>                                  __func__, __LINE__, ret);
>>>                  goto out;
>>>          }
>>>
>>>          if (val & (1 << 4)) /* RTCA1 */
>>>                  alrm->pending = 1;
>>>
>>> }
>> Most of the drivers set the 'pending' field when reading alarm. Your
>> original driver did not.
>>
>> The max77802 does exactly the same (BTW, these should be merged as
>> well... I'll add this to the TODO list) so I think this is necessary.
> How about merging max77802 to max77686 first? The only differences I
> found are:
> 1. It uses main MFD/PMIC regmap.
>     This can be solved as part of decoupling code. The driver will get
> MFD's regmap and set up its own (only on max77686). The max77802 will
> only use parent's regmap.
>
> 2. It has different register address.
>     We need a register-layout/configuration structure. The logic is the
> same except few differences (e.g. presence of MAX77802_RTC_AE1).
>
> It may be easier to merge them now, before adding support for max77620?
> I could handle this probably next week or in the following week
> (assuming someone would test max77802 because I don't have the hardware).
>
> Anyway I think we should develop these RTC patches having this in mind:
> merge all of them.
>

I think we can do the merging of max77802 and max77686 at second step.

At first, lets decouple the rtc-max77686.c with its mfd driver. This 
will give better picture how will it be done.
Once this is there then max77620 can use this and in parallel, can be 
work for max77802 to use the same driver.


Let's first conclude and get accpepted for max77686 and max77620 as both 
of us have related HW to verify the changes.


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Jan. 14, 2016, 2:20 p.m. UTC | #9
On Thu, Jan 14, 2016 at 10:20:56AM +0900, Krzysztof Kozlowski wrote:

> 2. It has different register address.
>    We need a register-layout/configuration structure. The logic is the
> same except few differences (e.g. presence of MAX77802_RTC_AE1).

Depending on what the differences are either just some variables in the
driver data that get set on probe with the different registers (if it's
just the register) or using regmap_field (if things get shifted as well,
or if it happens to make more sense) should work.
Javier Martinez Canillas Jan. 15, 2016, 1:50 a.m. UTC | #10
Hello,

On Thu, Jan 14, 2016 at 8:50 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> On Thursday 14 January 2016 06:50 AM, Krzysztof Kozlowski wrote:
>> On 14.01.2016 09:50, Krzysztof Kozlowski wrote:

[snip]

>>>
>>> The max77802 does exactly the same (BTW, these should be merged as
>>> well... I'll add this to the TODO list) so I think this is necessary.
>>
>> How about merging max77802 to max77686 first? The only differences I
>> found are:
>> 1. It uses main MFD/PMIC regmap.
>>     This can be solved as part of decoupling code. The driver will get
>> MFD's regmap and set up its own (only on max77686). The max77802 will
>> only use parent's regmap.
>>
>> 2. It has different register address.
>>     We need a register-layout/configuration structure. The logic is the
>> same except few differences (e.g. presence of MAX77802_RTC_AE1).
>>
>> It may be easier to merge them now, before adding support for max77620?

Agreed.

When I originally posted the max77802 support, I had separate MFD,
RTC, regulator and clock drivers and the feedback (IIRC) was that the
MFD and clock blocks were too similar to the max77686 so I extended
those drivers instead of adding new ones. But that the RTC and
regulator blocks were different so a separate driver was justified...
but it's true that are not that different and rtc-max77686 could be
extended and rtc-max77802 removed.

In fact, the ChromiumOS vendor tree has a single RTC driver for both
max77686 and max77802:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/rtc/rtc-max77xxx.c

>> I could handle this probably next week or in the following week
>> (assuming someone would test max77802 because I don't have the hardware).
>>

I could also work on this next week if you want. After all I feel
guilty for the code duplication :-)

>> Anyway I think we should develop these RTC patches having this in mind:
>> merge all of them.
>>
>
> I think we can do the merging of max77802 and max77686 at second step.
>
> At first, lets decouple the rtc-max77686.c with its mfd driver. This will
> give better picture how will it be done.

I don't quite understand what you mean by decoupling here, the
max77686 MFD driver today is not highly coupled to their cells devices
drivers since it supports both max77802 and max77686 already. Yes,
some changes will be needed but I think those should be small.

> Once this is there then max77620 can use this and in parallel, can be work
> for max77802 to use the same driver.
>
>
> Let's first conclude and get accpepted for max77686 and max77620 as both of
> us have related HW to verify the changes.
>

I agree with Krzysztof that merging first before extending makes more
sense but I don't have a strong opinion on this and can be done as a
followup as well.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Jan. 18, 2016, 4:23 a.m. UTC | #11
On 14.01.2016 23:20, Mark Brown wrote:
> On Thu, Jan 14, 2016 at 10:20:56AM +0900, Krzysztof Kozlowski wrote:
> 
>> 2. It has different register address.
>>    We need a register-layout/configuration structure. The logic is the
>> same except few differences (e.g. presence of MAX77802_RTC_AE1).
> 
> Depending on what the differences are either just some variables in the
> driver data that get set on probe with the different registers (if it's
> just the register) or using regmap_field (if things get shifted as well,
> or if it happens to make more sense) should work.

Thanks for the hints!

The addresses for registers are indeed shifted. It is non-linear because
apart of different offset at beginning, a new field appears. Anyway it
would be good for me (or other person doing this work) to get more
knowledge of what regmap API provides.

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Jan. 18, 2016, 4:27 a.m. UTC | #12
On 15.01.2016 10:50, Javier Martinez Canillas wrote:
> Hello,
> 
> On Thu, Jan 14, 2016 at 8:50 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>> On Thursday 14 January 2016 06:50 AM, Krzysztof Kozlowski wrote:
>>> On 14.01.2016 09:50, Krzysztof Kozlowski wrote:
> 
> [snip]
> 
>>>>
>>>> The max77802 does exactly the same (BTW, these should be merged as
>>>> well... I'll add this to the TODO list) so I think this is necessary.
>>>
>>> How about merging max77802 to max77686 first? The only differences I
>>> found are:
>>> 1. It uses main MFD/PMIC regmap.
>>>     This can be solved as part of decoupling code. The driver will get
>>> MFD's regmap and set up its own (only on max77686). The max77802 will
>>> only use parent's regmap.
>>>
>>> 2. It has different register address.
>>>     We need a register-layout/configuration structure. The logic is the
>>> same except few differences (e.g. presence of MAX77802_RTC_AE1).
>>>
>>> It may be easier to merge them now, before adding support for max77620?
> 
> Agreed.
> 
> When I originally posted the max77802 support, I had separate MFD,
> RTC, regulator and clock drivers and the feedback (IIRC) was that the
> MFD and clock blocks were too similar to the max77686 so I extended
> those drivers instead of adding new ones. But that the RTC and
> regulator blocks were different so a separate driver was justified...
> but it's true that are not that different and rtc-max77686 could be
> extended and rtc-max77802 removed.

Great!

> 
> In fact, the ChromiumOS vendor tree has a single RTC driver for both
> max77686 and max77802:
> 
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/rtc/rtc-max77xxx.c
> 
>>> I could handle this probably next week or in the following week
>>> (assuming someone would test max77802 because I don't have the hardware).
>>>
> 
> I could also work on this next week if you want. After all I feel
> guilty for the code duplication :-)

So feel free to take that job from me. :) I will happily do the testing
and provide complains (I mean, comments).

> 
>>> Anyway I think we should develop these RTC patches having this in mind:
>>> merge all of them.
>>>
>>
>> I think we can do the merging of max77802 and max77686 at second step.
>>
>> At first, lets decouple the rtc-max77686.c with its mfd driver. This will
>> give better picture how will it be done.
> 
> I don't quite understand what you mean by decoupling here, the
> max77686 MFD driver today is not highly coupled to their cells devices
> drivers since it supports both max77802 and max77686 already. Yes,
> some changes will be needed but I think those should be small.

The decoupling needed is to move RTC-related stuff (i2c_new_dummy and
regmap) entirely to RTC driver. This work is independent of which driver
will be merged to rtc-max77xxx first.

> 
>> Once this is there then max77620 can use this and in parallel, can be work
>> for max77802 to use the same driver.
>>
>>
>> Let's first conclude and get accpepted for max77686 and max77620 as both of
>> us have related HW to verify the changes.
>>
> 
> I agree with Krzysztof that merging first before extending makes more
> sense but I don't have a strong opinion on this and can be done as a
> followup as well.

AFAIR, Javier have max77802 on Chromebook, right?

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laxman Dewangan Jan. 18, 2016, 12:01 p.m. UTC | #13
On Monday 18 January 2016 09:57 AM, Krzysztof Kozlowski wrote:
> On 15.01.2016 10:50, Javier Martinez Canillas wrote:
>> Hello,
>>
>> On Thu, Jan 14, 2016 at 8:50 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>> On Thursday 14 January 2016 06:50 AM, Krzysztof Kozlowski wrote:
>>>> On 14.01.2016 09:50, Krzysztof Kozlowski wrote:
>> [snip]
>>
>>>>> The max77802 does exactly the same (BTW, these should be merged as
>>>>> well... I'll add this to the TODO list) so I think this is necessary.
>>>> How about merging max77802 to max77686 first? The only differences I
>>>> found are:
>>>> 1. It uses main MFD/PMIC regmap.
>>>>      This can be solved as part of decoupling code. The driver will get
>>>> MFD's regmap and set up its own (only on max77686). The max77802 will
>>>> only use parent's regmap.
>>>>
>>>> 2. It has different register address.
>>>>      We need a register-layout/configuration structure. The logic is the
>>>> same except few differences (e.g. presence of MAX77802_RTC_AE1).
>>>>
>>>> It may be easier to merge them now, before adding support for max77620?
>> Agreed.
>>
>> When I originally posted the max77802 support, I had separate MFD,
>> RTC, regulator and clock drivers and the feedback (IIRC) was that the
>> MFD and clock blocks were too similar to the max77686 so I extended
>> those drivers instead of adding new ones. But that the RTC and
>> regulator blocks were different so a separate driver was justified...
>> but it's true that are not that different and rtc-max77686 could be
>> extended and rtc-max77802 removed.
> Great!
>
>> In fact, the ChromiumOS vendor tree has a single RTC driver for both
>> max77686 and max77802:
>>
>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/rtc/rtc-max77xxx.c
>>
>>>> I could handle this probably next week or in the following week
>>>> (assuming someone would test max77802 because I don't have the hardware).
>>>>
>> I could also work on this next week if you want. After all I feel
>> guilty for the code duplication :-)
> So feel free to take that job from me. :) I will happily do the testing
> and provide complains (I mean, comments).
>

Hi Javier,
If you can provide the patch for moving MAX77802 to MAX77686 then I can 
create the other change to move the regmap and i2c dummy client for rtc 
of max77686 to rtc driver from core on top of your change. This will  
completely decouple the rtc driver from core and so will be easy to use 
for other chip MAX77620.

Thanks,
Laxman

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Javier Martinez Canillas Jan. 18, 2016, 12:52 p.m. UTC | #14
Hello Laxman and Krzysztof,

On Mon, Jan 18, 2016 at 9:01 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>
> On Monday 18 January 2016 09:57 AM, Krzysztof Kozlowski wrote:
>>
>> On 15.01.2016 10:50, Javier Martinez Canillas wrote:
>>>
>>> Hello,
>>>
>>> On Thu, Jan 14, 2016 at 8:50 AM, Laxman Dewangan <ldewangan@nvidia.com>
>>> wrote:
>>>>
>>>> On Thursday 14 January 2016 06:50 AM, Krzysztof Kozlowski wrote:
>>>>>
>>>>> On 14.01.2016 09:50, Krzysztof Kozlowski wrote:
>>>
>>> [snip]
>>>
>>>>>> The max77802 does exactly the same (BTW, these should be merged as
>>>>>> well... I'll add this to the TODO list) so I think this is necessary.
>>>>>
>>>>> How about merging max77802 to max77686 first? The only differences I
>>>>> found are:
>>>>> 1. It uses main MFD/PMIC regmap.
>>>>>      This can be solved as part of decoupling code. The driver will get
>>>>> MFD's regmap and set up its own (only on max77686). The max77802 will
>>>>> only use parent's regmap.
>>>>>
>>>>> 2. It has different register address.
>>>>>      We need a register-layout/configuration structure. The logic is
>>>>> the
>>>>> same except few differences (e.g. presence of MAX77802_RTC_AE1).
>>>>>
>>>>> It may be easier to merge them now, before adding support for max77620?
>>>
>>> Agreed.
>>>
>>> When I originally posted the max77802 support, I had separate MFD,
>>> RTC, regulator and clock drivers and the feedback (IIRC) was that the
>>> MFD and clock blocks were too similar to the max77686 so I extended
>>> those drivers instead of adding new ones. But that the RTC and
>>> regulator blocks were different so a separate driver was justified...
>>> but it's true that are not that different and rtc-max77686 could be
>>> extended and rtc-max77802 removed.
>>
>> Great!
>>
>>> In fact, the ChromiumOS vendor tree has a single RTC driver for both
>>> max77686 and max77802:
>>>
>>>
>>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/rtc/rtc-max77xxx.c
>>>
>>>>> I could handle this probably next week or in the following week
>>>>> (assuming someone would test max77802 because I don't have the
>>>>> hardware).
>>>>>
>>> I could also work on this next week if you want. After all I feel
>>> guilty for the code duplication :-)
>>
>> So feel free to take that job from me. :) I will happily do the testing
>> and provide complains (I mean, comments).
>>
>
> Hi Javier,
> If you can provide the patch for moving MAX77802 to MAX77686 then I can
> create the other change to move the regmap and i2c dummy client for rtc of
> max77686 to rtc driver from core on top of your change. This will
> completely decouple the rtc driver from core and so will be easy to use for
> other chip MAX77620.
>

Ok, sounds like a plan. I'll just do the change to extend rtc-max77686
to support the max77802 rtc then and let the other change to you.

I'll work on this today once I finish other task that I'm currently working on.

> Thanks,
> Laxman
>

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 376322f..4972dd5 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -315,6 +315,16 @@  config RTC_DRV_MAX8997
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-max8997.
 
+config RTC_DRV_MAX77XXX
+	tristate "Maxim MAX77XXX series generic RTC driver"
+	help
+	  If you say yes here you will get support for the generic RTC driver
+	  for Maxim Semiconductor MAX77XXX series of PMIC like MAX77620.
+	  This also supports the RTC driver for Maxim PMIC MaX20024 which
+	  is almost same as MAX77620.
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-max77xxx.
+
 config RTC_DRV_MAX77686
 	tristate "Maxim MAX77686"
 	depends on MFD_MAX77686
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 62d61b2..e5aba30 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -85,6 +85,7 @@  obj-$(CONFIG_RTC_DRV_M48T59)	+= rtc-m48t59.o
 obj-$(CONFIG_RTC_DRV_M48T86)	+= rtc-m48t86.o
 obj-$(CONFIG_RTC_DRV_MAX6900)	+= rtc-max6900.o
 obj-$(CONFIG_RTC_DRV_MAX6902)	+= rtc-max6902.o
+obj-$(CONFIG_RTC_DRV_MAX77XXX)	+= rtc-max77xxx.o
 obj-$(CONFIG_RTC_DRV_MAX77686)	+= rtc-max77686.o
 obj-$(CONFIG_RTC_DRV_MAX77802)	+= rtc-max77802.o
 obj-$(CONFIG_RTC_DRV_MAX8907)	+= rtc-max8907.o
diff --git a/drivers/rtc/rtc-max77xxx.c b/drivers/rtc/rtc-max77xxx.c
new file mode 100644
index 0000000..c5ada63
--- /dev/null
+++ b/drivers/rtc/rtc-max77xxx.c
@@ -0,0 +1,500 @@ 
+/*
+ * Max77xxx(MAX77620, MAX77686 etc) RTC driver
+ *
+ * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+
+/* RTC Registers */
+#define MAX77XXX_REG_RTCINT			0x00
+#define MAX77XXX_REG_RTCINTM			0x01
+#define MAX77XXX_REG_RTCCNTLM			0x02
+#define MAX77XXX_REG_RTCCNTL			0x03
+#define MAX77XXX_REG_RTCUPDATE0			0x04
+#define MAX77XXX_REG_RTCUPDATE1			0x05
+#define MAX77XXX_REG_RTCSMPL			0x06
+#define MAX77XXX_REG_RTCSEC			0x07
+#define MAX77XXX_REG_RTCMIN			0x08
+#define MAX77XXX_REG_RTCHOUR			0x09
+#define MAX77XXX_REG_RTCDOW			0x0A
+#define MAX77XXX_REG_RTCMONTH			0x0B
+#define MAX77XXX_REG_RTCYEAR			0x0C
+#define MAX77XXX_REG_RTCDOM			0x0D
+#define MAX77XXX_REG_RTCSECA1			0x0E
+#define MAX77XXX_REG_RTCMINA1			0x0F
+#define MAX77XXX_REG_RTCHOURA1			0x10
+#define MAX77XXX_REG_RTCDOWA1			0x11
+#define MAX77XXX_REG_RTCMONTHA1			0x12
+#define MAX77XXX_REG_RTCYEARA1			0x13
+#define MAX77XXX_REG_RTCDOMA1			0x14
+#define MAX77XXX_REG_RTCSECA2			0x15
+#define MAX77XXX_REG_RTCMINA2			0x16
+#define MAX77XXX_REG_RTCHOURA2			0x17
+#define MAX77XXX_REG_RTCDOWA2			0x18
+#define MAX77XXX_REG_RTCMONTHA2			0x19
+#define MAX77XXX_REG_RTCYEARA2			0x1A
+#define MAX77XXX_REG_RTCDOMA2			0x1B
+
+#define MAX77XXX_RTC60S_MASK			BIT(0)
+#define MAX77XXX_RTCA1_MASK			BIT(1)
+#define MAX77XXX_RTCA2_MASK			BIT(2)
+#define MAX77XXX_RTC_SMPL_MASK			BIT(3)
+#define MAX77XXX_RTC_RTC1S_MASK			BIT(4)
+#define MAX77XXX_RTC_ALL_IRQ_MASK		0x1F
+
+#define MAX77XXX_BCDM_MASK			BIT(0)
+#define MAX77XXX_HRMODEM_MASK			BIT(1)
+
+#define WB_UPDATE_MASK				BIT(0)
+#define FLAG_AUTO_CLEAR_MASK			BIT(1)
+#define FREEZE_SEC_MASK				BIT(2)
+#define RTC_WAKE_MASK				BIT(3)
+#define RB_UPDATE_MASK				BIT(4)
+
+#define MAX77XXX_UDF_MASK			BIT(0)
+#define MAX77XXX_RBUDF_MASK			BIT(1)
+
+#define SEC_MASK				0x7F
+#define MIN_MASK				0x7F
+#define HOUR_MASK				0x3F
+#define WEEKDAY_MASK				0x7F
+#define MONTH_MASK				0x1F
+#define YEAR_MASK				0xFF
+#define MONTHDAY_MASK				0x3F
+
+#define ALARM_EN_MASK				0x80
+#define ALARM_EN_SHIFT				7
+
+#define RTC_YEAR_BASE				100
+#define RTC_YEAR_MAX				99
+
+#define ONOFF_WK_ALARM1_MASK			BIT(2)
+
+enum {
+	RTC_SEC,
+	RTC_MIN,
+	RTC_HOUR,
+	RTC_WEEKDAY,
+	RTC_MONTH,
+	RTC_YEAR,
+	RTC_MONTHDAY,
+	RTC_NR
+};
+
+struct max77xxx_rtc_info {
+	struct rtc_device *rtc;
+	struct device *dev;
+	struct regmap *rmap;
+	struct mutex io_lock;
+	int irq;
+	u8 irq_mask;
+};
+
+static int max77xxx_rtc_update_buffer(struct max77xxx_rtc_info *rinfo,
+		bool write)
+{
+	u8 val = FLAG_AUTO_CLEAR_MASK | RTC_WAKE_MASK;
+	int ret;
+
+	if (write)
+		val |= WB_UPDATE_MASK;
+	else
+		val |= RB_UPDATE_MASK;
+
+	ret = regmap_write(rinfo->rmap, MAX77XXX_REG_RTCUPDATE0, val);
+	if (ret < 0) {
+		dev_err(rinfo->dev, "Reg RTCUPDATE0 write failed: %d\n", ret);
+		return ret;
+	}
+
+	/* Must wait 16ms for buffer update */
+	usleep_range(16000, 17000);
+
+	return 0;
+}
+
+static int max77xxx_rtc_write(struct max77xxx_rtc_info *rinfo, u8 addr,
+			void *vals, u32 len)
+{
+	int ret;
+	int i;
+	u8 *src = vals;
+
+	mutex_lock(&rinfo->io_lock);
+	for (i = 0; i < len; ++i) {
+		ret = regmap_write(rinfo->rmap, addr + i, *src++);
+		if (ret < 0) {
+			dev_err(rinfo->dev, "Reg 0x%02x write failed: %d\n",
+				addr + i, ret);
+			goto out;
+		}
+	}
+	ret = max77xxx_rtc_update_buffer(rinfo, true);
+out:
+	mutex_unlock(&rinfo->io_lock);
+
+	return ret;
+}
+
+static int max77xxx_rtc_read(struct max77xxx_rtc_info *rinfo, u8 addr,
+			void *vals, u32 len, bool update_buffer)
+{
+	int ret;
+
+	mutex_lock(&rinfo->io_lock);
+	if (update_buffer) {
+		ret = max77xxx_rtc_update_buffer(rinfo, false);
+		if (ret < 0)
+			goto out;
+	}
+
+	ret = regmap_bulk_read(rinfo->rmap, addr, vals, len);
+	if (ret < 0)
+		dev_err(rinfo->dev, "Reg 0x%02x read failed: %d\n", addr, ret);
+out:
+	mutex_unlock(&rinfo->io_lock);
+
+	return ret;
+}
+
+static int max77xxx_rtc_reg_to_tm(struct max77xxx_rtc_info *rinfo, u8 *buf,
+			 struct rtc_time *tm)
+{
+	int wday = buf[RTC_WEEKDAY] & WEEKDAY_MASK;
+
+	if (!wday) {
+		dev_err(rinfo->dev, "Invalid day of week, %d\n", wday);
+		return -EINVAL;
+	}
+
+	tm->tm_sec = (int)(buf[RTC_SEC] & SEC_MASK);
+	tm->tm_min = (int)(buf[RTC_MIN] & MIN_MASK);
+	tm->tm_hour = (int)(buf[RTC_HOUR] & HOUR_MASK);
+	tm->tm_mday = (int)(buf[RTC_MONTHDAY] & MONTHDAY_MASK);
+	tm->tm_mon = (int)(buf[RTC_MONTH] & MONTH_MASK) - 1;
+	tm->tm_year = (int)(buf[RTC_YEAR] & YEAR_MASK) + RTC_YEAR_BASE;
+	tm->tm_wday = ffs(wday) - 1;
+
+	return 0;
+}
+
+static int max77xxx_rtc_tm_to_reg(struct max77xxx_rtc_info *rinfo, u8 *buf,
+			 struct rtc_time *tm, int alarm)
+{
+	u8 alarm_mask = alarm ? ALARM_EN_MASK : 0;
+
+	if ((tm->tm_year < RTC_YEAR_BASE) ||
+			(tm->tm_year > (RTC_YEAR_BASE + RTC_YEAR_MAX))) {
+		dev_err(rinfo->dev, "Invalid year, %d\n", tm->tm_year);
+		return -EINVAL;
+	}
+
+	buf[RTC_SEC] = tm->tm_sec | alarm_mask;
+	buf[RTC_MIN] = tm->tm_min | alarm_mask;
+	buf[RTC_HOUR] = tm->tm_hour | alarm_mask;
+	buf[RTC_MONTHDAY] = tm->tm_mday | alarm_mask;
+	buf[RTC_MONTH] = (tm->tm_mon + 1) | alarm_mask;
+	buf[RTC_YEAR] = (tm->tm_year - RTC_YEAR_BASE) | alarm_mask;
+
+	/* The wday is configured only when disabled alarm. */
+	buf[RTC_WEEKDAY] = (alarm) ? 0x01 : (1 << tm->tm_wday);
+
+	return 0;
+}
+
+static int max77xxx_rtc_irq_mask(struct max77xxx_rtc_info *rinfo, u8 irq)
+{
+	u8 irq_mask = rinfo->irq_mask | irq;
+	int ret;
+
+	ret = max77xxx_rtc_write(rinfo, MAX77XXX_REG_RTCINTM, &irq_mask, 1);
+	if (ret < 0)
+		return ret;
+	rinfo->irq_mask = irq_mask;
+
+	return ret;
+}
+
+static int max77xxx_rtc_irq_unmask(struct max77xxx_rtc_info *rinfo, u8 irq)
+{
+	u8 irq_mask = rinfo->irq_mask & ~irq;
+	int ret;
+
+	ret = max77xxx_rtc_write(rinfo, MAX77XXX_REG_RTCINTM, &irq_mask, 1);
+	if (ret < 0)
+		return ret;
+	rinfo->irq_mask = irq_mask;
+
+	return ret;
+}
+
+static int max77xxx_rtc_do_irq(struct max77xxx_rtc_info *rinfo)
+{
+	unsigned int irq_status;
+	int ret;
+
+	ret = regmap_read(rinfo->rmap, MAX77XXX_REG_RTCINT, &irq_status);
+	if (ret < 0) {
+		dev_err(rinfo->dev, "RTCINT read failed: %d\n", ret);
+		return ret;
+	}
+
+	if (!(rinfo->irq_mask & MAX77XXX_RTCA1_MASK) &&
+			(irq_status & MAX77XXX_RTCA1_MASK))
+		rtc_update_irq(rinfo->rtc, 1, RTC_IRQF | RTC_AF);
+
+	if (!(rinfo->irq_mask & MAX77XXX_RTC_RTC1S_MASK) &&
+			(irq_status & MAX77XXX_RTC_RTC1S_MASK))
+		rtc_update_irq(rinfo->rtc, 1, RTC_IRQF | RTC_UF);
+
+	return ret;
+}
+
+static irqreturn_t max77xxx_rtc_irq(int irq, void *data)
+{
+	struct max77xxx_rtc_info *rinfo = (struct max77xxx_rtc_info *)data;
+
+	max77xxx_rtc_do_irq(rinfo);
+
+	return IRQ_HANDLED;
+}
+
+static int max77xxx_rtc_alarm_irq_enable(struct device *dev,
+					 unsigned int enabled)
+{
+	struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+	int ret;
+
+	if (rinfo->irq < 0)
+		return -ENXIO;
+
+	/* Handle pending interrupt */
+	ret = max77xxx_rtc_do_irq(rinfo);
+	if (ret < 0)
+		return ret;
+
+	/* Configure alarm interrupt */
+	if (enabled)
+		ret = max77xxx_rtc_irq_unmask(rinfo, MAX77XXX_RTCA1_MASK);
+	else
+		ret = max77xxx_rtc_irq_mask(rinfo, MAX77XXX_RTCA1_MASK);
+
+	return ret;
+}
+
+static int max77xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+	u8 buf[RTC_NR];
+	int ret;
+
+	ret = max77xxx_rtc_read(rinfo, MAX77XXX_REG_RTCSEC, buf, RTC_NR, true);
+	if (ret < 0)
+		return ret;
+
+	return max77xxx_rtc_reg_to_tm(rinfo, buf, tm);
+}
+
+static int max77xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+	u8 buf[RTC_NR];
+	int ret;
+
+	ret = max77xxx_rtc_tm_to_reg(rinfo, buf, tm, 0);
+	if (ret < 0)
+		return ret;
+
+	return max77xxx_rtc_write(rinfo, MAX77XXX_REG_RTCSEC, buf, RTC_NR);
+}
+
+static int max77xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+	u8 buf[RTC_NR];
+	int ret;
+
+	ret = max77xxx_rtc_read(rinfo, MAX77XXX_REG_RTCSECA1, buf, RTC_NR, 1);
+	if (ret < 0)
+		return ret;
+
+	buf[RTC_YEAR] &= ~ALARM_EN_MASK;
+	ret = max77xxx_rtc_reg_to_tm(rinfo, buf, &alrm->time);
+	if (ret < 0)
+		return ret;
+
+	alrm->enabled = (rinfo->irq_mask & MAX77XXX_RTCA1_MASK) ? 0 : 1;
+
+	return 0;
+}
+
+static int max77xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+	u8 buf[RTC_NR];
+	int ret;
+
+	ret = max77xxx_rtc_tm_to_reg(rinfo, buf, &alrm->time, 1);
+	if (ret < 0)
+		return ret;
+
+	ret = max77xxx_rtc_write(rinfo, MAX77XXX_REG_RTCSECA1, buf, RTC_NR);
+	if (ret < 0)
+		return ret;
+
+	ret = max77xxx_rtc_alarm_irq_enable(dev, alrm->enabled);
+	if (ret < 0)
+		return ret;
+
+	return ret;
+}
+
+static const struct rtc_class_ops max77xxx_rtc_ops = {
+	.read_time = max77xxx_rtc_read_time,
+	.set_time = max77xxx_rtc_set_time,
+	.read_alarm = max77xxx_rtc_read_alarm,
+	.set_alarm = max77xxx_rtc_set_alarm,
+	.alarm_irq_enable = max77xxx_rtc_alarm_irq_enable,
+};
+
+static int max77xxx_rtc_preinit(struct max77xxx_rtc_info *rinfo)
+{
+	u8 val;
+	int ret;
+
+	/* Mask all interrupts */
+	rinfo->irq_mask = 0xFF;
+	ret = max77xxx_rtc_write(rinfo, MAX77XXX_REG_RTCINTM,
+			&rinfo->irq_mask, 1);
+	if (ret < 0)
+		return ret;
+
+	max77xxx_rtc_read(rinfo, MAX77XXX_REG_RTCINT, &val, 1, false);
+
+	/* Configure Binary mode and 24hour mode */
+	val = MAX77XXX_HRMODEM_MASK;
+	return max77xxx_rtc_write(rinfo, MAX77XXX_REG_RTCCNTL, &val, 1);
+}
+
+static int max77xxx_rtc_probe(struct platform_device *pdev)
+{
+	static struct max77xxx_rtc_info *rinfo;
+	int ret;
+
+	rinfo = devm_kzalloc(&pdev->dev, sizeof(*rinfo), GFP_KERNEL);
+	if (!rinfo)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, rinfo);
+	rinfo->dev = &pdev->dev;
+	mutex_init(&rinfo->io_lock);
+	rinfo->rmap = dev_get_regmap(pdev->dev.parent, "rtc-slave");
+	if (!rinfo->rmap) {
+		dev_err(&pdev->dev, "Regmap for RTC device not found\n");
+		return -ENODEV;
+	}
+
+	ret = max77xxx_rtc_preinit(rinfo);
+	if (ret < 0)
+		goto fail_preinit;
+
+	device_init_wakeup(&pdev->dev, 1);
+
+	rinfo->rtc = devm_rtc_device_register(&pdev->dev, "max77xxx-rtc",
+				&max77xxx_rtc_ops, THIS_MODULE);
+	if (IS_ERR(rinfo->rtc)) {
+		ret = PTR_ERR(rinfo->rtc);
+		dev_err(&pdev->dev, "RTC registration failed: %d\n", ret);
+		goto fail_preinit;
+	}
+
+	rinfo->irq = platform_get_irq(pdev, 0);
+	ret = devm_request_threaded_irq(&pdev->dev, rinfo->irq, NULL,
+			max77xxx_rtc_irq, IRQF_ONESHOT, "max77xxx-rtc", rinfo);
+	if (ret < 0) {
+		dev_err(rinfo->dev, "Failed to request irq %d\n", rinfo->irq);
+		goto fail_preinit;
+	}
+
+	enable_irq_wake(rinfo->irq);
+
+	return 0;
+
+fail_preinit:
+	mutex_destroy(&rinfo->io_lock);
+
+	return ret;
+}
+
+static int max77xxx_rtc_remove(struct platform_device *pdev)
+{
+	struct max77xxx_rtc_info *rinfo = dev_get_drvdata(&pdev->dev);
+
+	mutex_destroy(&rinfo->io_lock);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int max77xxx_rtc_suspend(struct device *dev)
+{
+	struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(rinfo->irq);
+
+	return 0;
+}
+
+static int max77xxx_rtc_resume(struct device *dev)
+{
+	struct max77xxx_rtc_info *rinfo = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(rinfo->irq);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops max77xxx_rtc_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(max77xxx_rtc_suspend, max77xxx_rtc_resume)
+};
+
+static const struct platform_device_id max77xxx_rtc_devtype[] = {
+	{ .name = "max77xxx-rtc", },
+	{ .name = "max77620-rtc", },
+	{ .name = "max20024-rtc", },
+};
+
+static struct platform_driver max77xxx_rtc_driver = {
+	.probe = max77xxx_rtc_probe,
+	.remove = max77xxx_rtc_remove,
+	.id_table = max77xxx_rtc_devtype,
+	.driver = {
+			.name = "max77xxx-rtc",
+			.pm = &max77xxx_rtc_pm_ops,
+	},
+};
+
+module_platform_driver(max77xxx_rtc_driver);
+
+MODULE_DESCRIPTION("max77xxx RTC driver");
+MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
+MODULE_ALIAS("platform:max77xxx-rtc");
+MODULE_LICENSE("GPL v2");