[rtc-linux] [PATCH 0/4] rtc: max77686: make max77686 rtc driver as IP driver

[rtc-linux] [PATCH 0/4] rtc: max77686: make max77686 rtc driver as IP driver

[Laxman Dewangan]

Based on discussion on patch series of MAX77620 when adding separate
driver for max77620 RTC, it is discussed to reuse the max77686 driver
for all CHips MAX77802, MAX77686 and MAX77620. For this, the rtc-max77686
need to make as IP driver independent of their MFD parent driver.

This series makes the rtc-max77686 as independent driver from its
parent. Required information is passed through the device parent
which is generic and does not depends on any max77686 specific
header ifnromation.

CC: Krzysztof Mazur <krzysiek@podlesie.net>
CC: Javier Martinez Canillas <javier@osg.samsung.com>

Laxman Dewangan (4):
  rtc: max77686: fix checkpatch error
  rtc: max77686: use rtc regmap to access RTC registers
  rtc: max77686: avoid reference of parent device info multiple palces
  rtc: max77686: move initialisation of rtc regmap, irq chip locally

 drivers/mfd/max77686.c               |  82 +---------
 drivers/rtc/Kconfig                  |   7 +-
 drivers/rtc/rtc-max77686.c           | 295 ++++++++++++++++++++++++++++-------
 include/linux/mfd/max77686-private.h |  16 --
 4 files changed, 244 insertions(+), 156 deletions(-)

-- 
2.1.4

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[PATCH 0/4] rtc: max77686: make max77686 rtc driver as IP driver

[Laxman Dewangan]

Based on discussion on patch series of MAX77620 when adding separate
driver for max77620 RTC, it is discussed to reuse the max77686 driver
for all CHips MAX77802, MAX77686 and MAX77620. For this, the rtc-max77686
need to make as IP driver independent of their MFD parent driver.

This series makes the rtc-max77686 as independent driver from its
parent. Required information is passed through the device parent
which is generic and does not depends on any max77686 specific
header ifnromation.

CC: Krzysztof Mazur <krzysiek@podlesie.net>
CC: Javier Martinez Canillas <javier@osg.samsung.com>

Laxman Dewangan (4):
  rtc: max77686: fix checkpatch error
  rtc: max77686: use rtc regmap to access RTC registers
  rtc: max77686: avoid reference of parent device info multiple palces
  rtc: max77686: move initialisation of rtc regmap, irq chip locally

 drivers/mfd/max77686.c               |  82 +---------
 drivers/rtc/Kconfig                  |   7 +-
 drivers/rtc/rtc-max77686.c           | 295 ++++++++++++++++++++++++++++-------
 include/linux/mfd/max77686-private.h |  16 --
 4 files changed, 244 insertions(+), 156 deletions(-)

-- 
2.1.4

[rtc-linux] [PATCH 1/4] rtc: max77686: fix checkpatch error

[Laxman Dewangan]

Fix following check patch error in rtc-max77686 driver:
- Alignment should match open parenthesis.
- braces {} should be used on all arms of this statement.
- Prefer using the BIT macro

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/rtc/rtc-max77686.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 188593d..253cf12 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -24,20 +24,20 @@
 
 /* RTC Control Register */
 #define BCD_EN_SHIFT			0
-#define BCD_EN_MASK			(1 << BCD_EN_SHIFT)
+#define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
 #define MODEL24_SHIFT			1
-#define MODEL24_MASK			(1 << MODEL24_SHIFT)
+#define MODEL24_MASK			BIT(MODEL24_SHIFT)
 /* RTC Update Register1 */
 #define RTC_UDR_SHIFT			0
-#define RTC_UDR_MASK			(1 << RTC_UDR_SHIFT)
+#define RTC_UDR_MASK			BIT(RTC_UDR_SHIFT)
 #define RTC_RBUDR_SHIFT			4
-#define RTC_RBUDR_MASK			(1 << RTC_RBUDR_SHIFT)
+#define RTC_RBUDR_MASK			BIT(RTC_RBUDR_SHIFT)
 /* RTC Hour register */
 #define HOUR_PM_SHIFT			6
-#define HOUR_PM_MASK			(1 << HOUR_PM_SHIFT)
+#define HOUR_PM_MASK			BIT(HOUR_PM_SHIFT)
 /* RTC Alarm Enable */
 #define ALARM_ENABLE_SHIFT		7
-#define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
+#define ALARM_ENABLE_MASK		BIT(ALARM_ENABLE_SHIFT)
 
 #define REG_RTC_NONE			0xdeadbeef
 
@@ -205,9 +205,9 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
 
 	tm->tm_sec = data[RTC_SEC] & mask;
 	tm->tm_min = data[RTC_MIN] & mask;
-	if (info->rtc_24hr_mode)
+	if (info->rtc_24hr_mode) {
 		tm->tm_hour = data[RTC_HOUR] & 0x1f;
-	else {
+	} else {
 		tm->tm_hour = data[RTC_HOUR] & 0x0f;
 		if (data[RTC_HOUR] & HOUR_PM_MASK)
 			tm->tm_hour += 12;
@@ -256,7 +256,7 @@ static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data,
 }
 
 static int max77686_rtc_update(struct max77686_rtc_info *info,
-	enum MAX77686_RTC_OP op)
+			       enum MAX77686_RTC_OP op)
 {
 	int ret;
 	unsigned int data;
@@ -547,7 +547,7 @@ out:
 }
 
 static int max77686_rtc_alarm_irq_enable(struct device *dev,
-					unsigned int enabled)
+					 unsigned int enabled)
 {
 	struct max77686_rtc_info *info = dev_get_drvdata(dev);
 	int ret;
@@ -612,7 +612,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 	int ret;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(struct max77686_rtc_info),
-				GFP_KERNEL);
+			    GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 
@@ -662,7 +662,8 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_threaded_irq(&pdev->dev, info->virq, NULL,
-				max77686_rtc_alarm_irq, 0, "rtc-alarm1", info);
+					max77686_rtc_alarm_irq, 0,
+					"rtc-alarm1", info);
 	if (ret < 0)
 		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
 			info->virq, ret);
-- 
2.1.4

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[PATCH 1/4] rtc: max77686: fix checkpatch error

[Laxman Dewangan]

Fix following check patch error in rtc-max77686 driver:
- Alignment should match open parenthesis.
- braces {} should be used on all arms of this statement.
- Prefer using the BIT macro

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/rtc/rtc-max77686.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 188593d..253cf12 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -24,20 +24,20 @@
 
 /* RTC Control Register */
 #define BCD_EN_SHIFT			0
-#define BCD_EN_MASK			(1 << BCD_EN_SHIFT)
+#define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
 #define MODEL24_SHIFT			1
-#define MODEL24_MASK			(1 << MODEL24_SHIFT)
+#define MODEL24_MASK			BIT(MODEL24_SHIFT)
 /* RTC Update Register1 */
 #define RTC_UDR_SHIFT			0
-#define RTC_UDR_MASK			(1 << RTC_UDR_SHIFT)
+#define RTC_UDR_MASK			BIT(RTC_UDR_SHIFT)
 #define RTC_RBUDR_SHIFT			4
-#define RTC_RBUDR_MASK			(1 << RTC_RBUDR_SHIFT)
+#define RTC_RBUDR_MASK			BIT(RTC_RBUDR_SHIFT)
 /* RTC Hour register */
 #define HOUR_PM_SHIFT			6
-#define HOUR_PM_MASK			(1 << HOUR_PM_SHIFT)
+#define HOUR_PM_MASK			BIT(HOUR_PM_SHIFT)
 /* RTC Alarm Enable */
 #define ALARM_ENABLE_SHIFT		7
-#define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
+#define ALARM_ENABLE_MASK		BIT(ALARM_ENABLE_SHIFT)
 
 #define REG_RTC_NONE			0xdeadbeef
 
@@ -205,9 +205,9 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
 
 	tm->tm_sec = data[RTC_SEC] & mask;
 	tm->tm_min = data[RTC_MIN] & mask;
-	if (info->rtc_24hr_mode)
+	if (info->rtc_24hr_mode) {
 		tm->tm_hour = data[RTC_HOUR] & 0x1f;
-	else {
+	} else {
 		tm->tm_hour = data[RTC_HOUR] & 0x0f;
 		if (data[RTC_HOUR] & HOUR_PM_MASK)
 			tm->tm_hour += 12;
@@ -256,7 +256,7 @@ static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data,
 }
 
 static int max77686_rtc_update(struct max77686_rtc_info *info,
-	enum MAX77686_RTC_OP op)
+			       enum MAX77686_RTC_OP op)
 {
 	int ret;
 	unsigned int data;
@@ -547,7 +547,7 @@ out:
 }
 
 static int max77686_rtc_alarm_irq_enable(struct device *dev,
-					unsigned int enabled)
+					 unsigned int enabled)
 {
 	struct max77686_rtc_info *info = dev_get_drvdata(dev);
 	int ret;
@@ -612,7 +612,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 	int ret;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(struct max77686_rtc_info),
-				GFP_KERNEL);
+			    GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 
@@ -662,7 +662,8 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_threaded_irq(&pdev->dev, info->virq, NULL,
-				max77686_rtc_alarm_irq, 0, "rtc-alarm1", info);
+					max77686_rtc_alarm_irq, 0,
+					"rtc-alarm1", info);
 	if (ret < 0)
 		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
 			info->virq, ret);
-- 
2.1.4

[rtc-linux] Re: [PATCH 1/4] rtc: max77686: fix checkpatch error

[Krzysztof Kozlowski]

On 02.02.2016 22:16, Laxman Dewangan wrote:
> Fix following check patch error in rtc-max77686 driver:
> - Alignment should match open parenthesis.
> - braces {} should be used on all arms of this statement.
> - Prefer using the BIT macro
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/rtc/rtc-max77686.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Tested on Trats2/max77686:
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>


Best regards,
Krzysztof

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/4] rtc: max77686: fix checkpatch error

[Krzysztof Kozlowski]

On 02.02.2016 22:16, Laxman Dewangan wrote:
> Fix following check patch error in rtc-max77686 driver:
> - Alignment should match open parenthesis.
> - braces {} should be used on all arms of this statement.
> - Prefer using the BIT macro
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/rtc/rtc-max77686.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Tested on Trats2/max77686:
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>


Best regards,
Krzysztof

[rtc-linux] Re: [PATCH 1/4] rtc: max77686: fix checkpatch error

[Javier Martinez Canillas]

Hello Laxman,

On 02/02/2016 10:16 AM, Laxman Dewangan wrote:
> Fix following check patch error in rtc-max77686 driver:
> - Alignment should match open parenthesis.
> - braces {} should be used on all arms of this statement.
> - Prefer using the BIT macro
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/4] rtc: max77686: fix checkpatch error

[Javier Martinez Canillas]

Hello Laxman,

On 02/02/2016 10:16 AM, Laxman Dewangan wrote:
> Fix following check patch error in rtc-max77686 driver:
> - Alignment should match open parenthesis.
> - braces {} should be used on all arms of this statement.
> - Prefer using the BIT macro
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[rtc-linux] [PATCH 2/4] rtc: max77686: use rtc regmap to access RTC registers

[Laxman Dewangan]

rtc_regmap should be used to access all RTC regsiters instead
of parent regmap regardless of what chip or property have it.

This makes the register access uniform and extendible for other
chips.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
CC: Javier Martinez Canillas <javier@osg.samsung.com>
---
 drivers/rtc/rtc-max77686.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 253cf12..11f74ed 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -370,7 +370,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 			goto out;
 		}
 
-		ret = regmap_read(info->max77686->regmap,
+		ret = regmap_read(info->max77686->rtc_regmap,
 				  map[REG_RTC_AE1], &val);
 		if (ret < 0) {
 			dev_err(info->dev,
@@ -426,7 +426,8 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
 			goto out;
 		}
 
-		ret = regmap_write(info->max77686->regmap, map[REG_RTC_AE1], 0);
+		ret = regmap_write(info->max77686->rtc_regmap,
+				   map[REG_RTC_AE1], 0);
 	} else {
 		ret = regmap_bulk_read(info->max77686->rtc_regmap,
 				       map[REG_ALARM1_SEC], data,
@@ -471,7 +472,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
 		goto out;
 
 	if (info->drv_data->alarm_enable_reg) {
-		ret = regmap_write(info->max77686->regmap, map[REG_RTC_AE1],
+		ret = regmap_write(info->max77686->rtc_regmap, map[REG_RTC_AE1],
 				   MAX77802_ALARM_ENABLE_VALUE);
 	} else {
 		ret = regmap_bulk_read(info->max77686->rtc_regmap,
-- 
2.1.4

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[PATCH 2/4] rtc: max77686: use rtc regmap to access RTC registers

[Laxman Dewangan]

rtc_regmap should be used to access all RTC regsiters instead
of parent regmap regardless of what chip or property have it.

This makes the register access uniform and extendible for other
chips.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
CC: Javier Martinez Canillas <javier@osg.samsung.com>
---
 drivers/rtc/rtc-max77686.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 253cf12..11f74ed 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -370,7 +370,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 			goto out;
 		}
 
-		ret = regmap_read(info->max77686->regmap,
+		ret = regmap_read(info->max77686->rtc_regmap,
 				  map[REG_RTC_AE1], &val);
 		if (ret < 0) {
 			dev_err(info->dev,
@@ -426,7 +426,8 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
 			goto out;
 		}
 
-		ret = regmap_write(info->max77686->regmap, map[REG_RTC_AE1], 0);
+		ret = regmap_write(info->max77686->rtc_regmap,
+				   map[REG_RTC_AE1], 0);
 	} else {
 		ret = regmap_bulk_read(info->max77686->rtc_regmap,
 				       map[REG_ALARM1_SEC], data,
@@ -471,7 +472,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
 		goto out;
 
 	if (info->drv_data->alarm_enable_reg) {
-		ret = regmap_write(info->max77686->regmap, map[REG_RTC_AE1],
+		ret = regmap_write(info->max77686->rtc_regmap, map[REG_RTC_AE1],
 				   MAX77802_ALARM_ENABLE_VALUE);
 	} else {
 		ret = regmap_bulk_read(info->max77686->rtc_regmap,
-- 
2.1.4

[rtc-linux] Re: [PATCH 2/4] rtc: max77686: use rtc regmap to access RTC registers

[Krzysztof Kozlowski]

On 02.02.2016 22:16, Laxman Dewangan wrote:
> rtc_regmap should be used to access all RTC regsiters instead
> of parent regmap regardless of what chip or property have it.
> 
> This makes the register access uniform and extendible for other
> chips.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>  drivers/rtc/rtc-max77686.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)


Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Tested on Trats2/max77686:
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>


Best regards,
Krzysztof


-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 2/4] rtc: max77686: use rtc regmap to access RTC registers

[Krzysztof Kozlowski]

On 02.02.2016 22:16, Laxman Dewangan wrote:
> rtc_regmap should be used to access all RTC regsiters instead
> of parent regmap regardless of what chip or property have it.
> 
> This makes the register access uniform and extendible for other
> chips.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>  drivers/rtc/rtc-max77686.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)


Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Tested on Trats2/max77686:
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>


Best regards,
Krzysztof

[rtc-linux] Re: [PATCH 2/4] rtc: max77686: use rtc regmap to access RTC registers

[Javier Martinez Canillas]

Hello Laxman

On 02/02/2016 10:16 AM, Laxman Dewangan wrote:
> rtc_regmap should be used to access all RTC regsiters instead

s/regsiters/registers

> of parent regmap regardless of what chip or property have it.
>
> This makes the register access uniform and extendible for other
> chips.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>   drivers/rtc/rtc-max77686.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>

Thanks for the patch, I indeed should had used rtc_regmap for consistency.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 2/4] rtc: max77686: use rtc regmap to access RTC registers

[Javier Martinez Canillas]

Hello Laxman

On 02/02/2016 10:16 AM, Laxman Dewangan wrote:
> rtc_regmap should be used to access all RTC regsiters instead

s/regsiters/registers

> of parent regmap regardless of what chip or property have it.
>
> This makes the register access uniform and extendible for other
> chips.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>   drivers/rtc/rtc-max77686.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>

Thanks for the patch, I indeed should had used rtc_regmap for consistency.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America

[rtc-linux] [PATCH 3/4] rtc: max77686: avoid reference of parent device info multiple palces

[Laxman Dewangan]

Get rid of referring parent device info for register access
all the places by making regmap as part of max77686 rtc
device info. This will also remove the need of storing parent
device info in max77686 rtc device info as this is no more required.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/rtc/rtc-max77686.c | 52 ++++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 29 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 11f74ed..ab1f2cd 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -74,12 +74,12 @@ struct max77686_rtc_driver_data {
 
 struct max77686_rtc_info {
 	struct device		*dev;
-	struct max77686_dev	*max77686;
 	struct i2c_client	*rtc;
 	struct rtc_device	*rtc_dev;
 	struct mutex		lock;
 
 	struct regmap		*regmap;
+	struct regmap		*rtc_regmap;
 
 	const struct max77686_rtc_driver_data *drv_data;
 
@@ -267,7 +267,7 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
 	else
 		data = 1 << RTC_RBUDR_SHIFT;
 
-	ret = regmap_update_bits(info->max77686->rtc_regmap,
+	ret = regmap_update_bits(info->rtc_regmap,
 				 info->drv_data->map[REG_RTC_UPDATE0],
 				 data, data);
 	if (ret < 0)
@@ -293,7 +293,7 @@ static int max77686_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	if (ret < 0)
 		goto out;
 
-	ret = regmap_bulk_read(info->max77686->rtc_regmap,
+	ret = regmap_bulk_read(info->rtc_regmap,
 			       info->drv_data->map[REG_RTC_SEC],
 			       data, ARRAY_SIZE(data));
 	if (ret < 0) {
@@ -322,7 +322,7 @@ static int max77686_rtc_set_time(struct device *dev, struct rtc_time *tm)
 
 	mutex_lock(&info->lock);
 
-	ret = regmap_bulk_write(info->max77686->rtc_regmap,
+	ret = regmap_bulk_write(info->rtc_regmap,
 				info->drv_data->map[REG_RTC_SEC],
 				data, ARRAY_SIZE(data));
 	if (ret < 0) {
@@ -351,8 +351,8 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	if (ret < 0)
 		goto out;
 
-	ret = regmap_bulk_read(info->max77686->rtc_regmap,
-			       map[REG_ALARM1_SEC], data, ARRAY_SIZE(data));
+	ret = regmap_bulk_read(info->rtc_regmap, map[REG_ALARM1_SEC],
+			       data, ARRAY_SIZE(data));
 	if (ret < 0) {
 		dev_err(info->dev, "Fail to read alarm reg(%d)\n", ret);
 		goto out;
@@ -370,8 +370,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 			goto out;
 		}
 
-		ret = regmap_read(info->max77686->rtc_regmap,
-				  map[REG_RTC_AE1], &val);
+		ret = regmap_read(info->rtc_regmap, map[REG_RTC_AE1], &val);
 		if (ret < 0) {
 			dev_err(info->dev,
 				"fail to read alarm enable(%d)\n", ret);
@@ -390,7 +389,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	}
 
 	alrm->pending = 0;
-	ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2, &val);
+	ret = regmap_read(info->regmap, MAX77686_REG_STATUS2, &val);
 	if (ret < 0) {
 		dev_err(info->dev, "Fail to read status2 reg(%d)\n", ret);
 		goto out;
@@ -426,12 +425,10 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
 			goto out;
 		}
 
-		ret = regmap_write(info->max77686->rtc_regmap,
-				   map[REG_RTC_AE1], 0);
+		ret = regmap_write(info->rtc_regmap, map[REG_RTC_AE1], 0);
 	} else {
-		ret = regmap_bulk_read(info->max77686->rtc_regmap,
-				       map[REG_ALARM1_SEC], data,
-				       ARRAY_SIZE(data));
+		ret = regmap_bulk_read(info->rtc_regmap, map[REG_ALARM1_SEC],
+				       data, ARRAY_SIZE(data));
 		if (ret < 0) {
 			dev_err(info->dev, "Fail to read alarm reg(%d)\n", ret);
 			goto out;
@@ -442,9 +439,8 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
 		for (i = 0; i < ARRAY_SIZE(data); i++)
 			data[i] &= ~ALARM_ENABLE_MASK;
 
-		ret = regmap_bulk_write(info->max77686->rtc_regmap,
-					map[REG_ALARM1_SEC], data,
-					ARRAY_SIZE(data));
+		ret = regmap_bulk_write(info->rtc_regmap, map[REG_ALARM1_SEC],
+					data, ARRAY_SIZE(data));
 	}
 
 	if (ret < 0) {
@@ -472,12 +468,11 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
 		goto out;
 
 	if (info->drv_data->alarm_enable_reg) {
-		ret = regmap_write(info->max77686->rtc_regmap, map[REG_RTC_AE1],
+		ret = regmap_write(info->rtc_regmap, map[REG_RTC_AE1],
 				   MAX77802_ALARM_ENABLE_VALUE);
 	} else {
-		ret = regmap_bulk_read(info->max77686->rtc_regmap,
-				       map[REG_ALARM1_SEC], data,
-				       ARRAY_SIZE(data));
+		ret = regmap_bulk_read(info->rtc_regmap, map[REG_ALARM1_SEC],
+				       data, ARRAY_SIZE(data));
 		if (ret < 0) {
 			dev_err(info->dev, "Fail to read alarm reg(%d)\n", ret);
 			goto out;
@@ -496,9 +491,8 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
 		if (data[RTC_DATE] & 0x1f)
 			data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
 
-		ret = regmap_bulk_write(info->max77686->rtc_regmap,
-					map[REG_ALARM1_SEC], data,
-					ARRAY_SIZE(data));
+		ret = regmap_bulk_write(info->rtc_regmap, map[REG_ALARM1_SEC],
+					data, ARRAY_SIZE(data));
 	}
 
 	if (ret < 0) {
@@ -527,7 +521,7 @@ static int max77686_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	if (ret < 0)
 		goto out;
 
-	ret = regmap_bulk_write(info->max77686->rtc_regmap,
+	ret = regmap_bulk_write(info->rtc_regmap,
 				info->drv_data->map[REG_ALARM1_SEC],
 				data, ARRAY_SIZE(data));
 
@@ -593,7 +587,7 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
 
 	info->rtc_24hr_mode = 1;
 
-	ret = regmap_bulk_write(info->max77686->rtc_regmap,
+	ret = regmap_bulk_write(info->rtc_regmap,
 				info->drv_data->map[REG_RTC_CONTROLM],
 				data, ARRAY_SIZE(data));
 	if (ret < 0) {
@@ -619,13 +613,13 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 
 	mutex_init(&info->lock);
 	info->dev = &pdev->dev;
-	info->max77686 = max77686;
 	info->rtc = max77686->rtc;
 	info->drv_data = (const struct max77686_rtc_driver_data *)
 		id->driver_data;
 
-	if (!info->drv_data->separate_i2c_addr)
-		info->max77686->rtc_regmap = info->max77686->regmap;
+	info->regmap = max77686->regmap;
+	info->rtc_regmap = (info->drv_data->separate_i2c_addr) ?
+			    max77686->rtc_regmap : info->regmap;
 
 	platform_set_drvdata(pdev, info);
 
-- 
2.1.4

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[PATCH 3/4] rtc: max77686: avoid reference of parent device info multiple palces

[Laxman Dewangan]

Get rid of referring parent device info for register access
all the places by making regmap as part of max77686 rtc
device info. This will also remove the need of storing parent
device info in max77686 rtc device info as this is no more required.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/rtc/rtc-max77686.c | 52 ++++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 29 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 11f74ed..ab1f2cd 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -74,12 +74,12 @@ struct max77686_rtc_driver_data {
 
 struct max77686_rtc_info {
 	struct device		*dev;
-	struct max77686_dev	*max77686;
 	struct i2c_client	*rtc;
 	struct rtc_device	*rtc_dev;
 	struct mutex		lock;
 
 	struct regmap		*regmap;
+	struct regmap		*rtc_regmap;
 
 	const struct max77686_rtc_driver_data *drv_data;
 
@@ -267,7 +267,7 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
 	else
 		data = 1 << RTC_RBUDR_SHIFT;
 
-	ret = regmap_update_bits(info->max77686->rtc_regmap,
+	ret = regmap_update_bits(info->rtc_regmap,
 				 info->drv_data->map[REG_RTC_UPDATE0],
 				 data, data);
 	if (ret < 0)
@@ -293,7 +293,7 @@ static int max77686_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	if (ret < 0)
 		goto out;
 
-	ret = regmap_bulk_read(info->max77686->rtc_regmap,
+	ret = regmap_bulk_read(info->rtc_regmap,
 			       info->drv_data->map[REG_RTC_SEC],
 			       data, ARRAY_SIZE(data));
 	if (ret < 0) {
@@ -322,7 +322,7 @@ static int max77686_rtc_set_time(struct device *dev, struct rtc_time *tm)
 
 	mutex_lock(&info->lock);
 
-	ret = regmap_bulk_write(info->max77686->rtc_regmap,
+	ret = regmap_bulk_write(info->rtc_regmap,
 				info->drv_data->map[REG_RTC_SEC],
 				data, ARRAY_SIZE(data));
 	if (ret < 0) {
@@ -351,8 +351,8 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	if (ret < 0)
 		goto out;
 
-	ret = regmap_bulk_read(info->max77686->rtc_regmap,
-			       map[REG_ALARM1_SEC], data, ARRAY_SIZE(data));
+	ret = regmap_bulk_read(info->rtc_regmap, map[REG_ALARM1_SEC],
+			       data, ARRAY_SIZE(data));
 	if (ret < 0) {
 		dev_err(info->dev, "Fail to read alarm reg(%d)\n", ret);
 		goto out;
@@ -370,8 +370,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 			goto out;
 		}
 
-		ret = regmap_read(info->max77686->rtc_regmap,
-				  map[REG_RTC_AE1], &val);
+		ret = regmap_read(info->rtc_regmap, map[REG_RTC_AE1], &val);
 		if (ret < 0) {
 			dev_err(info->dev,
 				"fail to read alarm enable(%d)\n", ret);
@@ -390,7 +389,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	}
 
 	alrm->pending = 0;
-	ret = regmap_read(info->max77686->regmap, MAX77686_REG_STATUS2, &val);
+	ret = regmap_read(info->regmap, MAX77686_REG_STATUS2, &val);
 	if (ret < 0) {
 		dev_err(info->dev, "Fail to read status2 reg(%d)\n", ret);
 		goto out;
@@ -426,12 +425,10 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
 			goto out;
 		}
 
-		ret = regmap_write(info->max77686->rtc_regmap,
-				   map[REG_RTC_AE1], 0);
+		ret = regmap_write(info->rtc_regmap, map[REG_RTC_AE1], 0);
 	} else {
-		ret = regmap_bulk_read(info->max77686->rtc_regmap,
-				       map[REG_ALARM1_SEC], data,
-				       ARRAY_SIZE(data));
+		ret = regmap_bulk_read(info->rtc_regmap, map[REG_ALARM1_SEC],
+				       data, ARRAY_SIZE(data));
 		if (ret < 0) {
 			dev_err(info->dev, "Fail to read alarm reg(%d)\n", ret);
 			goto out;
@@ -442,9 +439,8 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
 		for (i = 0; i < ARRAY_SIZE(data); i++)
 			data[i] &= ~ALARM_ENABLE_MASK;
 
-		ret = regmap_bulk_write(info->max77686->rtc_regmap,
-					map[REG_ALARM1_SEC], data,
-					ARRAY_SIZE(data));
+		ret = regmap_bulk_write(info->rtc_regmap, map[REG_ALARM1_SEC],
+					data, ARRAY_SIZE(data));
 	}
 
 	if (ret < 0) {
@@ -472,12 +468,11 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
 		goto out;
 
 	if (info->drv_data->alarm_enable_reg) {
-		ret = regmap_write(info->max77686->rtc_regmap, map[REG_RTC_AE1],
+		ret = regmap_write(info->rtc_regmap, map[REG_RTC_AE1],
 				   MAX77802_ALARM_ENABLE_VALUE);
 	} else {
-		ret = regmap_bulk_read(info->max77686->rtc_regmap,
-				       map[REG_ALARM1_SEC], data,
-				       ARRAY_SIZE(data));
+		ret = regmap_bulk_read(info->rtc_regmap, map[REG_ALARM1_SEC],
+				       data, ARRAY_SIZE(data));
 		if (ret < 0) {
 			dev_err(info->dev, "Fail to read alarm reg(%d)\n", ret);
 			goto out;
@@ -496,9 +491,8 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
 		if (data[RTC_DATE] & 0x1f)
 			data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
 
-		ret = regmap_bulk_write(info->max77686->rtc_regmap,
-					map[REG_ALARM1_SEC], data,
-					ARRAY_SIZE(data));
+		ret = regmap_bulk_write(info->rtc_regmap, map[REG_ALARM1_SEC],
+					data, ARRAY_SIZE(data));
 	}
 
 	if (ret < 0) {
@@ -527,7 +521,7 @@ static int max77686_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	if (ret < 0)
 		goto out;
 
-	ret = regmap_bulk_write(info->max77686->rtc_regmap,
+	ret = regmap_bulk_write(info->rtc_regmap,
 				info->drv_data->map[REG_ALARM1_SEC],
 				data, ARRAY_SIZE(data));
 
@@ -593,7 +587,7 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
 
 	info->rtc_24hr_mode = 1;
 
-	ret = regmap_bulk_write(info->max77686->rtc_regmap,
+	ret = regmap_bulk_write(info->rtc_regmap,
 				info->drv_data->map[REG_RTC_CONTROLM],
 				data, ARRAY_SIZE(data));
 	if (ret < 0) {
@@ -619,13 +613,13 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 
 	mutex_init(&info->lock);
 	info->dev = &pdev->dev;
-	info->max77686 = max77686;
 	info->rtc = max77686->rtc;
 	info->drv_data = (const struct max77686_rtc_driver_data *)
 		id->driver_data;
 
-	if (!info->drv_data->separate_i2c_addr)
-		info->max77686->rtc_regmap = info->max77686->regmap;
+	info->regmap = max77686->regmap;
+	info->rtc_regmap = (info->drv_data->separate_i2c_addr) ?
+			    max77686->rtc_regmap : info->regmap;
 
 	platform_set_drvdata(pdev, info);
 
-- 
2.1.4

[rtc-linux] Re: [PATCH 3/4] rtc: max77686: avoid reference of parent device info multiple palces

[Krzysztof Kozlowski]

On 02.02.2016 22:16, Laxman Dewangan wrote:
> Get rid of referring parent device info for register access
> all the places by making regmap as part of max77686 rtc
> device info. This will also remove the need of storing parent
> device info in max77686 rtc device info as this is no more required.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/rtc/rtc-max77686.c | 52 ++++++++++++++++++++--------------------------
>  1 file changed, 23 insertions(+), 29 deletions(-)

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Tested on Trats2/max77686:
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>


Best regards,
Krzysztof

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 3/4] rtc: max77686: avoid reference of parent device info multiple palces

[Krzysztof Kozlowski]

On 02.02.2016 22:16, Laxman Dewangan wrote:
> Get rid of referring parent device info for register access
> all the places by making regmap as part of max77686 rtc
> device info. This will also remove the need of storing parent
> device info in max77686 rtc device info as this is no more required.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/rtc/rtc-max77686.c | 52 ++++++++++++++++++++--------------------------
>  1 file changed, 23 insertions(+), 29 deletions(-)

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Tested on Trats2/max77686:
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>


Best regards,
Krzysztof

[rtc-linux] Re: [PATCH 3/4] rtc: max77686: avoid reference of parent device info multiple palces

[Javier Martinez Canillas]

Hello Laxman,

On 02/02/2016 10:16 AM, Laxman Dewangan wrote:
> Get rid of referring parent device info for register access
> all the places by making regmap as part of max77686 rtc
> device info. This will also remove the need of storing parent
> device info in max77686 rtc device info as this is no more required.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 3/4] rtc: max77686: avoid reference of parent device info multiple palces

[Javier Martinez Canillas]

Hello Laxman,

On 02/02/2016 10:16 AM, Laxman Dewangan wrote:
> Get rid of referring parent device info for register access
> all the places by making regmap as part of max77686 rtc
> device info. This will also remove the need of storing parent
> device info in max77686 rtc device info as this is no more required.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America

[rtc-linux] [PATCH 4/4] rtc: max77686: move initialisation of rtc regmap, irq chip locally

[Laxman Dewangan]

To make RTC block of MAX77686/MAX77802 as independent driver,
move the registeration of i2c device, regmap for register access
and irq_chip for interrupt support inside the RTC driver.
Removed the same initialisation from mfd driver.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
CC: Krzysztof Mazur <krzysiek@podlesie.net>
CC: Javier Martinez Canillas <javier@osg.samsung.com>
---
 drivers/mfd/max77686.c               |  82 +------------
 drivers/rtc/Kconfig                  |   7 +-
 drivers/rtc/rtc-max77686.c           | 225 ++++++++++++++++++++++++++++++++---
 include/linux/mfd/max77686-private.h |  16 ---
 4 files changed, 211 insertions(+), 119 deletions(-)

diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index d959ebb..8254201 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -116,11 +116,6 @@ static const struct regmap_config max77686_regmap_config = {
 	.val_bits = 8,
 };
 
-static const struct regmap_config max77686_rtc_regmap_config = {
-	.reg_bits = 8,
-	.val_bits = 8,
-};
-
 static const struct regmap_config max77802_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -156,25 +151,6 @@ static const struct regmap_irq_chip max77686_irq_chip = {
 	.num_irqs		= ARRAY_SIZE(max77686_irqs),
 };
 
-static const struct regmap_irq max77686_rtc_irqs[] = {
-	/* RTC interrupts */
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
-};
-
-static const struct regmap_irq_chip max77686_rtc_irq_chip = {
-	.name			= "max77686-rtc",
-	.status_base		= MAX77686_RTC_INT,
-	.mask_base		= MAX77686_RTC_INTM,
-	.num_regs		= 1,
-	.irqs			= max77686_rtc_irqs,
-	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
-};
-
 static const struct regmap_irq_chip max77802_irq_chip = {
 	.name			= "max77802-pmic",
 	.status_base		= MAX77802_REG_INT1,
@@ -184,15 +160,6 @@ static const struct regmap_irq_chip max77802_irq_chip = {
 	.num_irqs		= ARRAY_SIZE(max77686_irqs),
 };
 
-static const struct regmap_irq_chip max77802_rtc_irq_chip = {
-	.name			= "max77802-rtc",
-	.status_base		= MAX77802_RTC_INT,
-	.mask_base		= MAX77802_RTC_INTM,
-	.num_regs		= 1,
-	.irqs			= max77686_rtc_irqs, /* same masks as 77686 */
-	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
-};
-
 static const struct of_device_id max77686_pmic_dt_match[] = {
 	{
 		.compatible = "maxim,max77686",
@@ -214,8 +181,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 	int ret = 0;
 	const struct regmap_config *config;
 	const struct regmap_irq_chip *irq_chip;
-	const struct regmap_irq_chip *rtc_irq_chip;
-	struct regmap **rtc_regmap;
 	const struct mfd_cell *cells;
 	int n_devs;
 
@@ -242,15 +207,11 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 	if (max77686->type == TYPE_MAX77686) {
 		config = &max77686_regmap_config;
 		irq_chip = &max77686_irq_chip;
-		rtc_irq_chip = &max77686_rtc_irq_chip;
-		rtc_regmap = &max77686->rtc_regmap;
 		cells =  max77686_devs;
 		n_devs = ARRAY_SIZE(max77686_devs);
 	} else {
 		config = &max77802_regmap_config;
 		irq_chip = &max77802_irq_chip;
-		rtc_irq_chip = &max77802_rtc_irq_chip;
-		rtc_regmap = &max77686->regmap;
 		cells =  max77802_devs;
 		n_devs = ARRAY_SIZE(max77802_devs);
 	}
@@ -270,60 +231,25 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 		return -ENODEV;
 	}
 
-	if (max77686->type == TYPE_MAX77686) {
-		max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
-		if (!max77686->rtc) {
-			dev_err(max77686->dev,
-				"Failed to allocate I2C device for RTC\n");
-			return -ENODEV;
-		}
-		i2c_set_clientdata(max77686->rtc, max77686);
-
-		max77686->rtc_regmap =
-			devm_regmap_init_i2c(max77686->rtc,
-					     &max77686_rtc_regmap_config);
-		if (IS_ERR(max77686->rtc_regmap)) {
-			ret = PTR_ERR(max77686->rtc_regmap);
-			dev_err(max77686->dev,
-				"failed to allocate RTC regmap: %d\n",
-				ret);
-			goto err_unregister_i2c;
-		}
-	}
-
 	ret = regmap_add_irq_chip(max77686->regmap, max77686->irq,
 				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
 				  IRQF_SHARED, 0, irq_chip,
 				  &max77686->irq_data);
 	if (ret) {
 		dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", ret);
-		goto err_unregister_i2c;
-	}
-
-	ret = regmap_add_irq_chip(*rtc_regmap, max77686->irq,
-				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
-				  IRQF_SHARED, 0, rtc_irq_chip,
-				  &max77686->rtc_irq_data);
-	if (ret) {
-		dev_err(&i2c->dev, "failed to add RTC irq chip: %d\n", ret);
-		goto err_del_irqc;
+		return -ENODEV;
 	}
 
 	ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL);
 	if (ret < 0) {
 		dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
-		goto err_del_rtc_irqc;
+		goto err_del_irqc;
 	}
 
 	return 0;
 
-err_del_rtc_irqc:
-	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
 err_del_irqc:
 	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
-err_unregister_i2c:
-	if (max77686->type == TYPE_MAX77686)
-		i2c_unregister_device(max77686->rtc);
 
 	return ret;
 }
@@ -334,12 +260,8 @@ static int max77686_i2c_remove(struct i2c_client *i2c)
 
 	mfd_remove_devices(max77686->dev);
 
-	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
 	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
 
-	if (max77686->type == TYPE_MAX77686)
-		i2c_unregister_device(max77686->rtc);
-
 	return 0;
 }
 
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index ec6a253..bdac624 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -325,14 +325,13 @@ config RTC_DRV_MAX8997
 	  will be called rtc-max8997.
 
 config RTC_DRV_MAX77686
-	tristate "Maxim MAX77686"
-	depends on MFD_MAX77686
+	tristate "Maxim MAX77686/MAX77802"
 	help
 	  If you say yes here you will get support for the
-	  RTC of Maxim MAX77686 PMIC.
+	  RTC of Maxim MAX77686/MAX77802 PMIC.
 
 	  This driver can also be built as a module. If so, the module
-	  will be called rtc-max77686.
+	  will be called rtc-max77686/MAX77802.
 
 config RTC_DRV_RK808
 	tristate "Rockchip RK808 RTC"
diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index ab1f2cd..21a88e2 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -12,16 +12,100 @@
  *
  */
 
+#include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/rtc.h>
 #include <linux/delay.h>
 #include <linux/mutex.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/mfd/max77686-private.h>
 #include <linux/irqdomain.h>
 #include <linux/regmap.h>
 
+#define MAX77686_REG_STATUS2		0x07
+
+enum max77686_rtc_reg {
+	MAX77686_RTC_INT		= 0x00,
+	MAX77686_RTC_INTM		= 0x01,
+	MAX77686_RTC_CONTROLM		= 0x02,
+	MAX77686_RTC_CONTROL		= 0x03,
+	MAX77686_RTC_UPDATE0		= 0x04,
+	/* Reserved: 0x5 */
+	MAX77686_WTSR_SMPL_CNTL		= 0x06,
+	MAX77686_RTC_SEC		= 0x07,
+	MAX77686_RTC_MIN		= 0x08,
+	MAX77686_RTC_HOUR		= 0x09,
+	MAX77686_RTC_WEEKDAY		= 0x0A,
+	MAX77686_RTC_MONTH		= 0x0B,
+	MAX77686_RTC_YEAR		= 0x0C,
+	MAX77686_RTC_DATE		= 0x0D,
+	MAX77686_ALARM1_SEC		= 0x0E,
+	MAX77686_ALARM1_MIN		= 0x0F,
+	MAX77686_ALARM1_HOUR		= 0x10,
+	MAX77686_ALARM1_WEEKDAY		= 0x11,
+	MAX77686_ALARM1_MONTH		= 0x12,
+	MAX77686_ALARM1_YEAR		= 0x13,
+	MAX77686_ALARM1_DATE		= 0x14,
+	MAX77686_ALARM2_SEC		= 0x15,
+	MAX77686_ALARM2_MIN		= 0x16,
+	MAX77686_ALARM2_HOUR		= 0x17,
+	MAX77686_ALARM2_WEEKDAY		= 0x18,
+	MAX77686_ALARM2_MONTH		= 0x19,
+	MAX77686_ALARM2_YEAR		= 0x1A,
+	MAX77686_ALARM2_DATE		= 0x1B,
+};
+
+enum max77802_rtc_reg {
+	MAX77802_RTC_INT		= 0xC0,
+	MAX77802_RTC_INTM		= 0xC1,
+	MAX77802_RTC_CONTROLM		= 0xC2,
+	MAX77802_RTC_CONTROL		= 0xC3,
+	MAX77802_RTC_UPDATE0		= 0xC4,
+	MAX77802_RTC_UPDATE1		= 0xC5,
+	MAX77802_WTSR_SMPL_CNTL		= 0xC6,
+	MAX77802_RTC_SEC		= 0xC7,
+	MAX77802_RTC_MIN		= 0xC8,
+	MAX77802_RTC_HOUR		= 0xC9,
+	MAX77802_RTC_WEEKDAY		= 0xCA,
+	MAX77802_RTC_MONTH		= 0xCB,
+	MAX77802_RTC_YEAR		= 0xCC,
+	MAX77802_RTC_DATE		= 0xCD,
+	MAX77802_RTC_AE1		= 0xCE,
+	MAX77802_ALARM1_SEC		= 0xCF,
+	MAX77802_ALARM1_MIN		= 0xD0,
+	MAX77802_ALARM1_HOUR		= 0xD1,
+	MAX77802_ALARM1_WEEKDAY		= 0xD2,
+	MAX77802_ALARM1_MONTH		= 0xD3,
+	MAX77802_ALARM1_YEAR		= 0xD4,
+	MAX77802_ALARM1_DATE		= 0xD5,
+	MAX77802_RTC_AE2		= 0xD6,
+	MAX77802_ALARM2_SEC		= 0xD7,
+	MAX77802_ALARM2_MIN		= 0xD8,
+	MAX77802_ALARM2_HOUR		= 0xD9,
+	MAX77802_ALARM2_WEEKDAY		= 0xDA,
+	MAX77802_ALARM2_MONTH		= 0xDB,
+	MAX77802_ALARM2_YEAR		= 0xDC,
+	MAX77802_ALARM2_DATE		= 0xDD,
+
+	MAX77802_RTC_END		= 0xDF,
+};
+
+enum max77686_rtc_irq {
+	MAX77686_RTCIRQ_RTC60S = 0,
+	MAX77686_RTCIRQ_RTCA1,
+	MAX77686_RTCIRQ_RTCA2,
+	MAX77686_RTCIRQ_SMPL,
+	MAX77686_RTCIRQ_RTC1S,
+	MAX77686_RTCIRQ_WTSR,
+};
+
+#define MAX77686_RTCINT_RTC60S_MSK	BIT(0)
+#define MAX77686_RTCINT_RTCA1_MSK	BIT(1)
+#define MAX77686_RTCINT_RTCA2_MSK	BIT(2)
+#define MAX77686_RTCINT_SMPL_MSK	BIT(3)
+#define MAX77686_RTCINT_RTC1S_MSK	BIT(4)
+#define MAX77686_RTCINT_WTSR_MSK	BIT(5)
+
 /* RTC Control Register */
 #define BCD_EN_SHIFT			0
 #define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
@@ -68,8 +152,10 @@ struct max77686_rtc_driver_data {
 	const unsigned int	*map;
 	/* Has a separate alarm enable register? */
 	bool			alarm_enable_reg;
-	/* Has a separate I2C regmap for the RTC? */
-	bool			separate_i2c_addr;
+	/* I2C address for RTC block */
+	u8			separate_i2c_addr;
+	/* RTC IRQ CHIP for regmap */
+	const struct regmap_irq_chip *rtc_irq_chip;
 };
 
 struct max77686_rtc_info {
@@ -82,7 +168,9 @@ struct max77686_rtc_info {
 	struct regmap		*rtc_regmap;
 
 	const struct max77686_rtc_driver_data *drv_data;
+	struct regmap_irq_chip_data *rtc_irq_data;
 
+	int rtc_irq;
 	int virq;
 	int rtc_24hr_mode;
 };
@@ -153,12 +241,32 @@ static const unsigned int max77686_map[REG_RTC_END] = {
 	[REG_RTC_AE1]	     = REG_RTC_NONE,
 };
 
+static const struct regmap_irq max77686_rtc_irqs[] = {
+	/* RTC interrupts */
+	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
+	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
+	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
+	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
+	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
+	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
+};
+
+static const struct regmap_irq_chip max77686_rtc_irq_chip = {
+	.name		= "max77686-rtc",
+	.status_base	= MAX77686_RTC_INT,
+	.mask_base	= MAX77686_RTC_INTM,
+	.num_regs	= 1,
+	.irqs		= max77686_rtc_irqs,
+	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
+};
+
 static const struct max77686_rtc_driver_data max77686_drv_data = {
 	.delay = 16000,
 	.mask  = 0x7f,
 	.map   = max77686_map,
 	.alarm_enable_reg  = false,
-	.separate_i2c_addr = true,
+	.separate_i2c_addr = 0xC >> 1,
+	.rtc_irq_chip = &max77686_rtc_irq_chip,
 };
 
 static const unsigned int max77802_map[REG_RTC_END] = {
@@ -190,12 +298,22 @@ static const unsigned int max77802_map[REG_RTC_END] = {
 	[REG_RTC_AE1]	     = MAX77802_RTC_AE1,
 };
 
+static const struct regmap_irq_chip max77802_rtc_irq_chip = {
+	.name		= "max77802-rtc",
+	.status_base	= MAX77802_RTC_INT,
+	.mask_base	= MAX77802_RTC_INTM,
+	.num_regs	= 1,
+	.irqs		= max77686_rtc_irqs, /* same masks as 77686 */
+	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
+};
+
 static const struct max77686_rtc_driver_data max77802_drv_data = {
 	.delay = 200,
 	.mask  = 0xff,
 	.map   = max77802_map,
 	.alarm_enable_reg  = true,
-	.separate_i2c_addr = false,
+	.separate_i2c_addr = 0,
+	.rtc_irq_chip = &max77802_rtc_irq_chip,
 };
 
 static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
@@ -599,9 +717,65 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
 	return ret;
 }
 
+static const struct regmap_config max77686_rtc_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
+{
+	struct device *parent = info->dev->parent;
+	struct i2c_client *parent_i2c = to_i2c_client(parent);
+	int ret;
+
+	info->rtc_irq =  parent_i2c->irq;
+
+	info->regmap = dev_get_regmap(parent, NULL);
+	if (!info->regmap) {
+		dev_err(info->dev, "Failed to get rtc regmap\n");
+		return -ENODEV;
+	}
+
+	if (!info->drv_data->separate_i2c_addr) {
+		info->rtc = parent_i2c;
+		goto add_rtc_irq;
+	}
+
+	info->rtc = i2c_new_dummy(parent_i2c->adapter,
+				info->drv_data->separate_i2c_addr);
+	if (!info->rtc) {
+		dev_err(info->dev, "Failed to allocate I2C device for RTC\n");
+		return -ENODEV;
+	}
+	i2c_set_clientdata(info->rtc, info);
+
+	info->rtc_regmap = devm_regmap_init_i2c(info->rtc,
+						&max77686_rtc_regmap_config);
+	if (IS_ERR(info->rtc_regmap)) {
+		ret = PTR_ERR(info->rtc_regmap);
+		dev_err(info->dev, "failed to allocate RTC regmap: %d\n", ret);
+		goto err_unregister_i2c;
+	}
+
+add_rtc_irq:
+	ret = regmap_add_irq_chip(info->rtc_regmap, info->rtc_irq,
+				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
+				  IRQF_SHARED, 0, info->drv_data->rtc_irq_chip,
+				  &info->rtc_irq_data);
+	if (ret < 0) {
+		dev_err(info->dev, "failed to add RTC irq chip: %d\n", ret);
+		goto err_unregister_i2c;
+	}
+
+	return 0;
+
+err_unregister_i2c:
+	i2c_unregister_device(info->rtc);
+	return ret;
+}
+
 static int max77686_rtc_probe(struct platform_device *pdev)
 {
-	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
 	struct max77686_rtc_info *info;
 	const struct platform_device_id *id = platform_get_device_id(pdev);
 	int ret;
@@ -613,18 +787,16 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 
 	mutex_init(&info->lock);
 	info->dev = &pdev->dev;
-	info->rtc = max77686->rtc;
 	info->drv_data = (const struct max77686_rtc_driver_data *)
 		id->driver_data;
 
-	info->regmap = max77686->regmap;
-	info->rtc_regmap = (info->drv_data->separate_i2c_addr) ?
-			    max77686->rtc_regmap : info->regmap;
+	ret = max77686_init_rtc_regmap(info);
+	if (ret < 0)
+		return ret;
 
 	platform_set_drvdata(pdev, info);
 
 	ret = max77686_rtc_init_reg(info);
-
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to initialize RTC reg:%d\n", ret);
 		goto err_rtc;
@@ -643,13 +815,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 		goto err_rtc;
 	}
 
-	if (!max77686->rtc_irq_data) {
-		ret = -EINVAL;
-		dev_err(&pdev->dev, "No RTC regmap IRQ chip\n");
-		goto err_rtc;
-	}
-
-	info->virq = regmap_irq_get_virq(max77686->rtc_irq_data,
+	info->virq = regmap_irq_get_virq(info->rtc_irq_data,
 					 MAX77686_RTCIRQ_RTCA1);
 	if (!info->virq) {
 		ret = -ENXIO;
@@ -659,14 +825,34 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 	ret = devm_request_threaded_irq(&pdev->dev, info->virq, NULL,
 					max77686_rtc_alarm_irq, 0,
 					"rtc-alarm1", info);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
 			info->virq, ret);
+		goto err_rtc;
+	}
+
+	return 0;
 
 err_rtc:
+	if (info->drv_data->separate_i2c_addr)
+		i2c_unregister_device(info->rtc);
+	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
+
 	return ret;
 }
 
+static int max77686_rtc_remove(struct platform_device *pdev)
+{
+	struct max77686_rtc_info *info = platform_get_drvdata(pdev);
+
+	if (info->drv_data->separate_i2c_addr)
+		i2c_unregister_device(info->rtc);
+
+	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int max77686_rtc_suspend(struct device *dev)
 {
@@ -707,6 +893,7 @@ static struct platform_driver max77686_rtc_driver = {
 		.pm	= &max77686_rtc_pm_ops,
 	},
 	.probe		= max77686_rtc_probe,
+	.remove		= max77686_rtc_remove,
 	.id_table	= rtc_id,
 };
 
diff --git a/include/linux/mfd/max77686-private.h b/include/linux/mfd/max77686-private.h
index f504349..cc322ba 100644
--- a/include/linux/mfd/max77686-private.h
+++ b/include/linux/mfd/max77686-private.h
@@ -407,12 +407,6 @@ enum max77686_irq {
 	MAX77686_PMICIRQ_140C,
 	MAX77686_PMICIRQ_120C,
 
-	MAX77686_RTCIRQ_RTC60S = 0,
-	MAX77686_RTCIRQ_RTCA1,
-	MAX77686_RTCIRQ_RTCA2,
-	MAX77686_RTCIRQ_SMPL,
-	MAX77686_RTCIRQ_RTC1S,
-	MAX77686_RTCIRQ_WTSR,
 };
 
 #define MAX77686_INT1_PWRONF_MSK	BIT(0)
@@ -427,24 +421,14 @@ enum max77686_irq {
 #define MAX77686_INT2_140C_MSK		BIT(0)
 #define MAX77686_INT2_120C_MSK		BIT(1)
 
-#define MAX77686_RTCINT_RTC60S_MSK	BIT(0)
-#define MAX77686_RTCINT_RTCA1_MSK	BIT(1)
-#define MAX77686_RTCINT_RTCA2_MSK	BIT(2)
-#define MAX77686_RTCINT_SMPL_MSK	BIT(3)
-#define MAX77686_RTCINT_RTC1S_MSK	BIT(4)
-#define MAX77686_RTCINT_WTSR_MSK	BIT(5)
-
 struct max77686_dev {
 	struct device *dev;
 	struct i2c_client *i2c; /* 0xcc / PMIC, Battery Control, and FLASH */
-	struct i2c_client *rtc; /* slave addr 0x0c */
 
 	unsigned long type;
 
 	struct regmap *regmap;		/* regmap for mfd */
-	struct regmap *rtc_regmap;	/* regmap for rtc */
 	struct regmap_irq_chip_data *irq_data;
-	struct regmap_irq_chip_data *rtc_irq_data;
 
 	int irq;
 	struct mutex irqlock;
-- 
2.1.4

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[PATCH 4/4] rtc: max77686: move initialisation of rtc regmap, irq chip locally

[Laxman Dewangan]

To make RTC block of MAX77686/MAX77802 as independent driver,
move the registeration of i2c device, regmap for register access
and irq_chip for interrupt support inside the RTC driver.
Removed the same initialisation from mfd driver.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
CC: Krzysztof Mazur <krzysiek@podlesie.net>
CC: Javier Martinez Canillas <javier@osg.samsung.com>
---
 drivers/mfd/max77686.c               |  82 +------------
 drivers/rtc/Kconfig                  |   7 +-
 drivers/rtc/rtc-max77686.c           | 225 ++++++++++++++++++++++++++++++++---
 include/linux/mfd/max77686-private.h |  16 ---
 4 files changed, 211 insertions(+), 119 deletions(-)

diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index d959ebb..8254201 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -116,11 +116,6 @@ static const struct regmap_config max77686_regmap_config = {
 	.val_bits = 8,
 };
 
-static const struct regmap_config max77686_rtc_regmap_config = {
-	.reg_bits = 8,
-	.val_bits = 8,
-};
-
 static const struct regmap_config max77802_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -156,25 +151,6 @@ static const struct regmap_irq_chip max77686_irq_chip = {
 	.num_irqs		= ARRAY_SIZE(max77686_irqs),
 };
 
-static const struct regmap_irq max77686_rtc_irqs[] = {
-	/* RTC interrupts */
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
-	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
-};
-
-static const struct regmap_irq_chip max77686_rtc_irq_chip = {
-	.name			= "max77686-rtc",
-	.status_base		= MAX77686_RTC_INT,
-	.mask_base		= MAX77686_RTC_INTM,
-	.num_regs		= 1,
-	.irqs			= max77686_rtc_irqs,
-	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
-};
-
 static const struct regmap_irq_chip max77802_irq_chip = {
 	.name			= "max77802-pmic",
 	.status_base		= MAX77802_REG_INT1,
@@ -184,15 +160,6 @@ static const struct regmap_irq_chip max77802_irq_chip = {
 	.num_irqs		= ARRAY_SIZE(max77686_irqs),
 };
 
-static const struct regmap_irq_chip max77802_rtc_irq_chip = {
-	.name			= "max77802-rtc",
-	.status_base		= MAX77802_RTC_INT,
-	.mask_base		= MAX77802_RTC_INTM,
-	.num_regs		= 1,
-	.irqs			= max77686_rtc_irqs, /* same masks as 77686 */
-	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
-};
-
 static const struct of_device_id max77686_pmic_dt_match[] = {
 	{
 		.compatible = "maxim,max77686",
@@ -214,8 +181,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 	int ret = 0;
 	const struct regmap_config *config;
 	const struct regmap_irq_chip *irq_chip;
-	const struct regmap_irq_chip *rtc_irq_chip;
-	struct regmap **rtc_regmap;
 	const struct mfd_cell *cells;
 	int n_devs;
 
@@ -242,15 +207,11 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 	if (max77686->type == TYPE_MAX77686) {
 		config = &max77686_regmap_config;
 		irq_chip = &max77686_irq_chip;
-		rtc_irq_chip = &max77686_rtc_irq_chip;
-		rtc_regmap = &max77686->rtc_regmap;
 		cells =  max77686_devs;
 		n_devs = ARRAY_SIZE(max77686_devs);
 	} else {
 		config = &max77802_regmap_config;
 		irq_chip = &max77802_irq_chip;
-		rtc_irq_chip = &max77802_rtc_irq_chip;
-		rtc_regmap = &max77686->regmap;
 		cells =  max77802_devs;
 		n_devs = ARRAY_SIZE(max77802_devs);
 	}
@@ -270,60 +231,25 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 		return -ENODEV;
 	}
 
-	if (max77686->type == TYPE_MAX77686) {
-		max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
-		if (!max77686->rtc) {
-			dev_err(max77686->dev,
-				"Failed to allocate I2C device for RTC\n");
-			return -ENODEV;
-		}
-		i2c_set_clientdata(max77686->rtc, max77686);
-
-		max77686->rtc_regmap =
-			devm_regmap_init_i2c(max77686->rtc,
-					     &max77686_rtc_regmap_config);
-		if (IS_ERR(max77686->rtc_regmap)) {
-			ret = PTR_ERR(max77686->rtc_regmap);
-			dev_err(max77686->dev,
-				"failed to allocate RTC regmap: %d\n",
-				ret);
-			goto err_unregister_i2c;
-		}
-	}
-
 	ret = regmap_add_irq_chip(max77686->regmap, max77686->irq,
 				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
 				  IRQF_SHARED, 0, irq_chip,
 				  &max77686->irq_data);
 	if (ret) {
 		dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", ret);
-		goto err_unregister_i2c;
-	}
-
-	ret = regmap_add_irq_chip(*rtc_regmap, max77686->irq,
-				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
-				  IRQF_SHARED, 0, rtc_irq_chip,
-				  &max77686->rtc_irq_data);
-	if (ret) {
-		dev_err(&i2c->dev, "failed to add RTC irq chip: %d\n", ret);
-		goto err_del_irqc;
+		return -ENODEV;
 	}
 
 	ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL);
 	if (ret < 0) {
 		dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
-		goto err_del_rtc_irqc;
+		goto err_del_irqc;
 	}
 
 	return 0;
 
-err_del_rtc_irqc:
-	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
 err_del_irqc:
 	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
-err_unregister_i2c:
-	if (max77686->type == TYPE_MAX77686)
-		i2c_unregister_device(max77686->rtc);
 
 	return ret;
 }
@@ -334,12 +260,8 @@ static int max77686_i2c_remove(struct i2c_client *i2c)
 
 	mfd_remove_devices(max77686->dev);
 
-	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
 	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
 
-	if (max77686->type == TYPE_MAX77686)
-		i2c_unregister_device(max77686->rtc);
-
 	return 0;
 }
 
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index ec6a253..bdac624 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -325,14 +325,13 @@ config RTC_DRV_MAX8997
 	  will be called rtc-max8997.
 
 config RTC_DRV_MAX77686
-	tristate "Maxim MAX77686"
-	depends on MFD_MAX77686
+	tristate "Maxim MAX77686/MAX77802"
 	help
 	  If you say yes here you will get support for the
-	  RTC of Maxim MAX77686 PMIC.
+	  RTC of Maxim MAX77686/MAX77802 PMIC.
 
 	  This driver can also be built as a module. If so, the module
-	  will be called rtc-max77686.
+	  will be called rtc-max77686/MAX77802.
 
 config RTC_DRV_RK808
 	tristate "Rockchip RK808 RTC"
diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index ab1f2cd..21a88e2 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -12,16 +12,100 @@
  *
  */
 
+#include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/rtc.h>
 #include <linux/delay.h>
 #include <linux/mutex.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/mfd/max77686-private.h>
 #include <linux/irqdomain.h>
 #include <linux/regmap.h>
 
+#define MAX77686_REG_STATUS2		0x07
+
+enum max77686_rtc_reg {
+	MAX77686_RTC_INT		= 0x00,
+	MAX77686_RTC_INTM		= 0x01,
+	MAX77686_RTC_CONTROLM		= 0x02,
+	MAX77686_RTC_CONTROL		= 0x03,
+	MAX77686_RTC_UPDATE0		= 0x04,
+	/* Reserved: 0x5 */
+	MAX77686_WTSR_SMPL_CNTL		= 0x06,
+	MAX77686_RTC_SEC		= 0x07,
+	MAX77686_RTC_MIN		= 0x08,
+	MAX77686_RTC_HOUR		= 0x09,
+	MAX77686_RTC_WEEKDAY		= 0x0A,
+	MAX77686_RTC_MONTH		= 0x0B,
+	MAX77686_RTC_YEAR		= 0x0C,
+	MAX77686_RTC_DATE		= 0x0D,
+	MAX77686_ALARM1_SEC		= 0x0E,
+	MAX77686_ALARM1_MIN		= 0x0F,
+	MAX77686_ALARM1_HOUR		= 0x10,
+	MAX77686_ALARM1_WEEKDAY		= 0x11,
+	MAX77686_ALARM1_MONTH		= 0x12,
+	MAX77686_ALARM1_YEAR		= 0x13,
+	MAX77686_ALARM1_DATE		= 0x14,
+	MAX77686_ALARM2_SEC		= 0x15,
+	MAX77686_ALARM2_MIN		= 0x16,
+	MAX77686_ALARM2_HOUR		= 0x17,
+	MAX77686_ALARM2_WEEKDAY		= 0x18,
+	MAX77686_ALARM2_MONTH		= 0x19,
+	MAX77686_ALARM2_YEAR		= 0x1A,
+	MAX77686_ALARM2_DATE		= 0x1B,
+};
+
+enum max77802_rtc_reg {
+	MAX77802_RTC_INT		= 0xC0,
+	MAX77802_RTC_INTM		= 0xC1,
+	MAX77802_RTC_CONTROLM		= 0xC2,
+	MAX77802_RTC_CONTROL		= 0xC3,
+	MAX77802_RTC_UPDATE0		= 0xC4,
+	MAX77802_RTC_UPDATE1		= 0xC5,
+	MAX77802_WTSR_SMPL_CNTL		= 0xC6,
+	MAX77802_RTC_SEC		= 0xC7,
+	MAX77802_RTC_MIN		= 0xC8,
+	MAX77802_RTC_HOUR		= 0xC9,
+	MAX77802_RTC_WEEKDAY		= 0xCA,
+	MAX77802_RTC_MONTH		= 0xCB,
+	MAX77802_RTC_YEAR		= 0xCC,
+	MAX77802_RTC_DATE		= 0xCD,
+	MAX77802_RTC_AE1		= 0xCE,
+	MAX77802_ALARM1_SEC		= 0xCF,
+	MAX77802_ALARM1_MIN		= 0xD0,
+	MAX77802_ALARM1_HOUR		= 0xD1,
+	MAX77802_ALARM1_WEEKDAY		= 0xD2,
+	MAX77802_ALARM1_MONTH		= 0xD3,
+	MAX77802_ALARM1_YEAR		= 0xD4,
+	MAX77802_ALARM1_DATE		= 0xD5,
+	MAX77802_RTC_AE2		= 0xD6,
+	MAX77802_ALARM2_SEC		= 0xD7,
+	MAX77802_ALARM2_MIN		= 0xD8,
+	MAX77802_ALARM2_HOUR		= 0xD9,
+	MAX77802_ALARM2_WEEKDAY		= 0xDA,
+	MAX77802_ALARM2_MONTH		= 0xDB,
+	MAX77802_ALARM2_YEAR		= 0xDC,
+	MAX77802_ALARM2_DATE		= 0xDD,
+
+	MAX77802_RTC_END		= 0xDF,
+};
+
+enum max77686_rtc_irq {
+	MAX77686_RTCIRQ_RTC60S = 0,
+	MAX77686_RTCIRQ_RTCA1,
+	MAX77686_RTCIRQ_RTCA2,
+	MAX77686_RTCIRQ_SMPL,
+	MAX77686_RTCIRQ_RTC1S,
+	MAX77686_RTCIRQ_WTSR,
+};
+
+#define MAX77686_RTCINT_RTC60S_MSK	BIT(0)
+#define MAX77686_RTCINT_RTCA1_MSK	BIT(1)
+#define MAX77686_RTCINT_RTCA2_MSK	BIT(2)
+#define MAX77686_RTCINT_SMPL_MSK	BIT(3)
+#define MAX77686_RTCINT_RTC1S_MSK	BIT(4)
+#define MAX77686_RTCINT_WTSR_MSK	BIT(5)
+
 /* RTC Control Register */
 #define BCD_EN_SHIFT			0
 #define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
@@ -68,8 +152,10 @@ struct max77686_rtc_driver_data {
 	const unsigned int	*map;
 	/* Has a separate alarm enable register? */
 	bool			alarm_enable_reg;
-	/* Has a separate I2C regmap for the RTC? */
-	bool			separate_i2c_addr;
+	/* I2C address for RTC block */
+	u8			separate_i2c_addr;
+	/* RTC IRQ CHIP for regmap */
+	const struct regmap_irq_chip *rtc_irq_chip;
 };
 
 struct max77686_rtc_info {
@@ -82,7 +168,9 @@ struct max77686_rtc_info {
 	struct regmap		*rtc_regmap;
 
 	const struct max77686_rtc_driver_data *drv_data;
+	struct regmap_irq_chip_data *rtc_irq_data;
 
+	int rtc_irq;
 	int virq;
 	int rtc_24hr_mode;
 };
@@ -153,12 +241,32 @@ static const unsigned int max77686_map[REG_RTC_END] = {
 	[REG_RTC_AE1]	     = REG_RTC_NONE,
 };
 
+static const struct regmap_irq max77686_rtc_irqs[] = {
+	/* RTC interrupts */
+	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
+	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
+	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
+	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
+	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
+	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
+};
+
+static const struct regmap_irq_chip max77686_rtc_irq_chip = {
+	.name		= "max77686-rtc",
+	.status_base	= MAX77686_RTC_INT,
+	.mask_base	= MAX77686_RTC_INTM,
+	.num_regs	= 1,
+	.irqs		= max77686_rtc_irqs,
+	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
+};
+
 static const struct max77686_rtc_driver_data max77686_drv_data = {
 	.delay = 16000,
 	.mask  = 0x7f,
 	.map   = max77686_map,
 	.alarm_enable_reg  = false,
-	.separate_i2c_addr = true,
+	.separate_i2c_addr = 0xC >> 1,
+	.rtc_irq_chip = &max77686_rtc_irq_chip,
 };
 
 static const unsigned int max77802_map[REG_RTC_END] = {
@@ -190,12 +298,22 @@ static const unsigned int max77802_map[REG_RTC_END] = {
 	[REG_RTC_AE1]	     = MAX77802_RTC_AE1,
 };
 
+static const struct regmap_irq_chip max77802_rtc_irq_chip = {
+	.name		= "max77802-rtc",
+	.status_base	= MAX77802_RTC_INT,
+	.mask_base	= MAX77802_RTC_INTM,
+	.num_regs	= 1,
+	.irqs		= max77686_rtc_irqs, /* same masks as 77686 */
+	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
+};
+
 static const struct max77686_rtc_driver_data max77802_drv_data = {
 	.delay = 200,
 	.mask  = 0xff,
 	.map   = max77802_map,
 	.alarm_enable_reg  = true,
-	.separate_i2c_addr = false,
+	.separate_i2c_addr = 0,
+	.rtc_irq_chip = &max77802_rtc_irq_chip,
 };
 
 static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
@@ -599,9 +717,65 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
 	return ret;
 }
 
+static const struct regmap_config max77686_rtc_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
+{
+	struct device *parent = info->dev->parent;
+	struct i2c_client *parent_i2c = to_i2c_client(parent);
+	int ret;
+
+	info->rtc_irq =  parent_i2c->irq;
+
+	info->regmap = dev_get_regmap(parent, NULL);
+	if (!info->regmap) {
+		dev_err(info->dev, "Failed to get rtc regmap\n");
+		return -ENODEV;
+	}
+
+	if (!info->drv_data->separate_i2c_addr) {
+		info->rtc = parent_i2c;
+		goto add_rtc_irq;
+	}
+
+	info->rtc = i2c_new_dummy(parent_i2c->adapter,
+				info->drv_data->separate_i2c_addr);
+	if (!info->rtc) {
+		dev_err(info->dev, "Failed to allocate I2C device for RTC\n");
+		return -ENODEV;
+	}
+	i2c_set_clientdata(info->rtc, info);
+
+	info->rtc_regmap = devm_regmap_init_i2c(info->rtc,
+						&max77686_rtc_regmap_config);
+	if (IS_ERR(info->rtc_regmap)) {
+		ret = PTR_ERR(info->rtc_regmap);
+		dev_err(info->dev, "failed to allocate RTC regmap: %d\n", ret);
+		goto err_unregister_i2c;
+	}
+
+add_rtc_irq:
+	ret = regmap_add_irq_chip(info->rtc_regmap, info->rtc_irq,
+				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
+				  IRQF_SHARED, 0, info->drv_data->rtc_irq_chip,
+				  &info->rtc_irq_data);
+	if (ret < 0) {
+		dev_err(info->dev, "failed to add RTC irq chip: %d\n", ret);
+		goto err_unregister_i2c;
+	}
+
+	return 0;
+
+err_unregister_i2c:
+	i2c_unregister_device(info->rtc);
+	return ret;
+}
+
 static int max77686_rtc_probe(struct platform_device *pdev)
 {
-	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
 	struct max77686_rtc_info *info;
 	const struct platform_device_id *id = platform_get_device_id(pdev);
 	int ret;
@@ -613,18 +787,16 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 
 	mutex_init(&info->lock);
 	info->dev = &pdev->dev;
-	info->rtc = max77686->rtc;
 	info->drv_data = (const struct max77686_rtc_driver_data *)
 		id->driver_data;
 
-	info->regmap = max77686->regmap;
-	info->rtc_regmap = (info->drv_data->separate_i2c_addr) ?
-			    max77686->rtc_regmap : info->regmap;
+	ret = max77686_init_rtc_regmap(info);
+	if (ret < 0)
+		return ret;
 
 	platform_set_drvdata(pdev, info);
 
 	ret = max77686_rtc_init_reg(info);
-
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to initialize RTC reg:%d\n", ret);
 		goto err_rtc;
@@ -643,13 +815,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 		goto err_rtc;
 	}
 
-	if (!max77686->rtc_irq_data) {
-		ret = -EINVAL;
-		dev_err(&pdev->dev, "No RTC regmap IRQ chip\n");
-		goto err_rtc;
-	}
-
-	info->virq = regmap_irq_get_virq(max77686->rtc_irq_data,
+	info->virq = regmap_irq_get_virq(info->rtc_irq_data,
 					 MAX77686_RTCIRQ_RTCA1);
 	if (!info->virq) {
 		ret = -ENXIO;
@@ -659,14 +825,34 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 	ret = devm_request_threaded_irq(&pdev->dev, info->virq, NULL,
 					max77686_rtc_alarm_irq, 0,
 					"rtc-alarm1", info);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
 			info->virq, ret);
+		goto err_rtc;
+	}
+
+	return 0;
 
 err_rtc:
+	if (info->drv_data->separate_i2c_addr)
+		i2c_unregister_device(info->rtc);
+	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
+
 	return ret;
 }
 
+static int max77686_rtc_remove(struct platform_device *pdev)
+{
+	struct max77686_rtc_info *info = platform_get_drvdata(pdev);
+
+	if (info->drv_data->separate_i2c_addr)
+		i2c_unregister_device(info->rtc);
+
+	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int max77686_rtc_suspend(struct device *dev)
 {
@@ -707,6 +893,7 @@ static struct platform_driver max77686_rtc_driver = {
 		.pm	= &max77686_rtc_pm_ops,
 	},
 	.probe		= max77686_rtc_probe,
+	.remove		= max77686_rtc_remove,
 	.id_table	= rtc_id,
 };
 
diff --git a/include/linux/mfd/max77686-private.h b/include/linux/mfd/max77686-private.h
index f504349..cc322ba 100644
--- a/include/linux/mfd/max77686-private.h
+++ b/include/linux/mfd/max77686-private.h
@@ -407,12 +407,6 @@ enum max77686_irq {
 	MAX77686_PMICIRQ_140C,
 	MAX77686_PMICIRQ_120C,
 
-	MAX77686_RTCIRQ_RTC60S = 0,
-	MAX77686_RTCIRQ_RTCA1,
-	MAX77686_RTCIRQ_RTCA2,
-	MAX77686_RTCIRQ_SMPL,
-	MAX77686_RTCIRQ_RTC1S,
-	MAX77686_RTCIRQ_WTSR,
 };
 
 #define MAX77686_INT1_PWRONF_MSK	BIT(0)
@@ -427,24 +421,14 @@ enum max77686_irq {
 #define MAX77686_INT2_140C_MSK		BIT(0)
 #define MAX77686_INT2_120C_MSK		BIT(1)
 
-#define MAX77686_RTCINT_RTC60S_MSK	BIT(0)
-#define MAX77686_RTCINT_RTCA1_MSK	BIT(1)
-#define MAX77686_RTCINT_RTCA2_MSK	BIT(2)
-#define MAX77686_RTCINT_SMPL_MSK	BIT(3)
-#define MAX77686_RTCINT_RTC1S_MSK	BIT(4)
-#define MAX77686_RTCINT_WTSR_MSK	BIT(5)
-
 struct max77686_dev {
 	struct device *dev;
 	struct i2c_client *i2c; /* 0xcc / PMIC, Battery Control, and FLASH */
-	struct i2c_client *rtc; /* slave addr 0x0c */
 
 	unsigned long type;
 
 	struct regmap *regmap;		/* regmap for mfd */
-	struct regmap *rtc_regmap;	/* regmap for rtc */
 	struct regmap_irq_chip_data *irq_data;
-	struct regmap_irq_chip_data *rtc_irq_data;
 
 	int irq;
 	struct mutex irqlock;
-- 
2.1.4

[rtc-linux] Re: [PATCH 4/4] rtc: max77686: move initialisation of rtc regmap, irq chip locally

[Krzysztof Kozlowski]

On 02.02.2016 22:16, Laxman Dewangan wrote:
> To make RTC block of MAX77686/MAX77802 as independent driver,
> move the registeration of i2c device, regmap for register access

s/registeration/registration/

> and irq_chip for interrupt support inside the RTC driver.
> Removed the same initialisation from mfd driver.

s/mfd/MFD/

I would expect here also information answering to the "why" question.
Why are you doing all of this? Why we want RTC driver to be independent?

> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Mazur <krzysiek@podlesie.net>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>  drivers/mfd/max77686.c               |  82 +------------
>  drivers/rtc/Kconfig                  |   7 +-
>  drivers/rtc/rtc-max77686.c           | 225 ++++++++++++++++++++++++++++++++---
>  include/linux/mfd/max77686-private.h |  16 ---
>  4 files changed, 211 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
> index d959ebb..8254201 100644
> --- a/drivers/mfd/max77686.c
> +++ b/drivers/mfd/max77686.c
> @@ -116,11 +116,6 @@ static const struct regmap_config max77686_regmap_config = {
>  	.val_bits = 8,
>  };
>  
> -static const struct regmap_config max77686_rtc_regmap_config = {
> -	.reg_bits = 8,
> -	.val_bits = 8,
> -};
> -
>  static const struct regmap_config max77802_regmap_config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
> @@ -156,25 +151,6 @@ static const struct regmap_irq_chip max77686_irq_chip = {
>  	.num_irqs		= ARRAY_SIZE(max77686_irqs),
>  };
>  
> -static const struct regmap_irq max77686_rtc_irqs[] = {
> -	/* RTC interrupts */
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
> -};
> -
> -static const struct regmap_irq_chip max77686_rtc_irq_chip = {
> -	.name			= "max77686-rtc",
> -	.status_base		= MAX77686_RTC_INT,
> -	.mask_base		= MAX77686_RTC_INTM,
> -	.num_regs		= 1,
> -	.irqs			= max77686_rtc_irqs,
> -	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
> -};
> -
>  static const struct regmap_irq_chip max77802_irq_chip = {
>  	.name			= "max77802-pmic",
>  	.status_base		= MAX77802_REG_INT1,
> @@ -184,15 +160,6 @@ static const struct regmap_irq_chip max77802_irq_chip = {
>  	.num_irqs		= ARRAY_SIZE(max77686_irqs),
>  };
>  
> -static const struct regmap_irq_chip max77802_rtc_irq_chip = {
> -	.name			= "max77802-rtc",
> -	.status_base		= MAX77802_RTC_INT,
> -	.mask_base		= MAX77802_RTC_INTM,
> -	.num_regs		= 1,
> -	.irqs			= max77686_rtc_irqs, /* same masks as 77686 */
> -	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
> -};
> -
>  static const struct of_device_id max77686_pmic_dt_match[] = {
>  	{
>  		.compatible = "maxim,max77686",
> @@ -214,8 +181,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>  	int ret = 0;
>  	const struct regmap_config *config;
>  	const struct regmap_irq_chip *irq_chip;
> -	const struct regmap_irq_chip *rtc_irq_chip;
> -	struct regmap **rtc_regmap;
>  	const struct mfd_cell *cells;
>  	int n_devs;
>  
> @@ -242,15 +207,11 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>  	if (max77686->type == TYPE_MAX77686) {
>  		config = &max77686_regmap_config;
>  		irq_chip = &max77686_irq_chip;
> -		rtc_irq_chip = &max77686_rtc_irq_chip;
> -		rtc_regmap = &max77686->rtc_regmap;
>  		cells =  max77686_devs;
>  		n_devs = ARRAY_SIZE(max77686_devs);
>  	} else {
>  		config = &max77802_regmap_config;
>  		irq_chip = &max77802_irq_chip;
> -		rtc_irq_chip = &max77802_rtc_irq_chip;
> -		rtc_regmap = &max77686->regmap;
>  		cells =  max77802_devs;
>  		n_devs = ARRAY_SIZE(max77802_devs);
>  	}
> @@ -270,60 +231,25 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>  		return -ENODEV;
>  	}
>  
> -	if (max77686->type == TYPE_MAX77686) {
> -		max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
> -		if (!max77686->rtc) {
> -			dev_err(max77686->dev,
> -				"Failed to allocate I2C device for RTC\n");
> -			return -ENODEV;
> -		}
> -		i2c_set_clientdata(max77686->rtc, max77686);
> -
> -		max77686->rtc_regmap =
> -			devm_regmap_init_i2c(max77686->rtc,
> -					     &max77686_rtc_regmap_config);
> -		if (IS_ERR(max77686->rtc_regmap)) {
> -			ret = PTR_ERR(max77686->rtc_regmap);
> -			dev_err(max77686->dev,
> -				"failed to allocate RTC regmap: %d\n",
> -				ret);
> -			goto err_unregister_i2c;
> -		}
> -	}
> -
>  	ret = regmap_add_irq_chip(max77686->regmap, max77686->irq,
>  				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>  				  IRQF_SHARED, 0, irq_chip,
>  				  &max77686->irq_data);
>  	if (ret) {
>  		dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", ret);
> -		goto err_unregister_i2c;
> -	}
> -
> -	ret = regmap_add_irq_chip(*rtc_regmap, max77686->irq,
> -				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
> -				  IRQF_SHARED, 0, rtc_irq_chip,
> -				  &max77686->rtc_irq_data);
> -	if (ret) {
> -		dev_err(&i2c->dev, "failed to add RTC irq chip: %d\n", ret);
> -		goto err_del_irqc;
> +		return -ENODEV;

return ret;

>  	}
>  
>  	ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL);
>  	if (ret < 0) {
>  		dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
> -		goto err_del_rtc_irqc;
> +		goto err_del_irqc;
>  	}
>  
>  	return 0;
>  
> -err_del_rtc_irqc:
> -	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
>  err_del_irqc:
>  	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
> -err_unregister_i2c:
> -	if (max77686->type == TYPE_MAX77686)
> -		i2c_unregister_device(max77686->rtc);
>  
>  	return ret;
>  }
> @@ -334,12 +260,8 @@ static int max77686_i2c_remove(struct i2c_client *i2c)
>  
>  	mfd_remove_devices(max77686->dev);
>  
> -	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
>  	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
>  
> -	if (max77686->type == TYPE_MAX77686)
> -		i2c_unregister_device(max77686->rtc);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index ec6a253..bdac624 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -325,14 +325,13 @@ config RTC_DRV_MAX8997
>  	  will be called rtc-max8997.
>  
>  config RTC_DRV_MAX77686
> -	tristate "Maxim MAX77686"
> -	depends on MFD_MAX77686

It still depends on some parent. Instantiating this without main MFD
probably would cause errors around max77686_init_rtc_regmap().

> +	tristate "Maxim MAX77686/MAX77802"
>  	help
>  	  If you say yes here you will get support for the
> -	  RTC of Maxim MAX77686 PMIC.
> +	  RTC of Maxim MAX77686/MAX77802 PMIC.
>  
>  	  This driver can also be built as a module. If so, the module
> -	  will be called rtc-max77686.
> +	  will be called rtc-max77686/MAX77802.

The name of module does not change.

>  
>  config RTC_DRV_RK808
>  	tristate "Rockchip RK808 RTC"
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index ab1f2cd..21a88e2 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -12,16 +12,100 @@
>   *
>   */
>  
> +#include <linux/i2c.h>
>  #include <linux/slab.h>
>  #include <linux/rtc.h>
>  #include <linux/delay.h>
>  #include <linux/mutex.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> -#include <linux/mfd/max77686-private.h>

I think the include should remain. No need of duplicating here the
register addresses. Although we wnat the driver to be independent from
parent it is still coming from one parent or another.

>  #include <linux/irqdomain.h>
>  #include <linux/regmap.h>
>  
> +#define MAX77686_REG_STATUS2		0x07
> +
> +enum max77686_rtc_reg {
> +	MAX77686_RTC_INT		= 0x00,
> +	MAX77686_RTC_INTM		= 0x01,
> +	MAX77686_RTC_CONTROLM		= 0x02,
> +	MAX77686_RTC_CONTROL		= 0x03,
> +	MAX77686_RTC_UPDATE0		= 0x04,
> +	/* Reserved: 0x5 */
> +	MAX77686_WTSR_SMPL_CNTL		= 0x06,
> +	MAX77686_RTC_SEC		= 0x07,
> +	MAX77686_RTC_MIN		= 0x08,
> +	MAX77686_RTC_HOUR		= 0x09,
> +	MAX77686_RTC_WEEKDAY		= 0x0A,
> +	MAX77686_RTC_MONTH		= 0x0B,
> +	MAX77686_RTC_YEAR		= 0x0C,
> +	MAX77686_RTC_DATE		= 0x0D,
> +	MAX77686_ALARM1_SEC		= 0x0E,
> +	MAX77686_ALARM1_MIN		= 0x0F,
> +	MAX77686_ALARM1_HOUR		= 0x10,
> +	MAX77686_ALARM1_WEEKDAY		= 0x11,
> +	MAX77686_ALARM1_MONTH		= 0x12,
> +	MAX77686_ALARM1_YEAR		= 0x13,
> +	MAX77686_ALARM1_DATE		= 0x14,
> +	MAX77686_ALARM2_SEC		= 0x15,
> +	MAX77686_ALARM2_MIN		= 0x16,
> +	MAX77686_ALARM2_HOUR		= 0x17,
> +	MAX77686_ALARM2_WEEKDAY		= 0x18,
> +	MAX77686_ALARM2_MONTH		= 0x19,
> +	MAX77686_ALARM2_YEAR		= 0x1A,
> +	MAX77686_ALARM2_DATE		= 0x1B,
> +};
> +
> +enum max77802_rtc_reg {
> +	MAX77802_RTC_INT		= 0xC0,
> +	MAX77802_RTC_INTM		= 0xC1,
> +	MAX77802_RTC_CONTROLM		= 0xC2,
> +	MAX77802_RTC_CONTROL		= 0xC3,
> +	MAX77802_RTC_UPDATE0		= 0xC4,
> +	MAX77802_RTC_UPDATE1		= 0xC5,
> +	MAX77802_WTSR_SMPL_CNTL		= 0xC6,
> +	MAX77802_RTC_SEC		= 0xC7,
> +	MAX77802_RTC_MIN		= 0xC8,
> +	MAX77802_RTC_HOUR		= 0xC9,
> +	MAX77802_RTC_WEEKDAY		= 0xCA,
> +	MAX77802_RTC_MONTH		= 0xCB,
> +	MAX77802_RTC_YEAR		= 0xCC,
> +	MAX77802_RTC_DATE		= 0xCD,
> +	MAX77802_RTC_AE1		= 0xCE,
> +	MAX77802_ALARM1_SEC		= 0xCF,
> +	MAX77802_ALARM1_MIN		= 0xD0,
> +	MAX77802_ALARM1_HOUR		= 0xD1,
> +	MAX77802_ALARM1_WEEKDAY		= 0xD2,
> +	MAX77802_ALARM1_MONTH		= 0xD3,
> +	MAX77802_ALARM1_YEAR		= 0xD4,
> +	MAX77802_ALARM1_DATE		= 0xD5,
> +	MAX77802_RTC_AE2		= 0xD6,
> +	MAX77802_ALARM2_SEC		= 0xD7,
> +	MAX77802_ALARM2_MIN		= 0xD8,
> +	MAX77802_ALARM2_HOUR		= 0xD9,
> +	MAX77802_ALARM2_WEEKDAY		= 0xDA,
> +	MAX77802_ALARM2_MONTH		= 0xDB,
> +	MAX77802_ALARM2_YEAR		= 0xDC,
> +	MAX77802_ALARM2_DATE		= 0xDD,
> +
> +	MAX77802_RTC_END		= 0xDF,
> +};

All of these should come from existing max77686/max77802/max77620 headers.

> +
> +enum max77686_rtc_irq {
> +	MAX77686_RTCIRQ_RTC60S = 0,
> +	MAX77686_RTCIRQ_RTCA1,
> +	MAX77686_RTCIRQ_RTCA2,
> +	MAX77686_RTCIRQ_SMPL,
> +	MAX77686_RTCIRQ_RTC1S,
> +	MAX77686_RTCIRQ_WTSR,
> +};

ditto

> +
> +#define MAX77686_RTCINT_RTC60S_MSK	BIT(0)
> +#define MAX77686_RTCINT_RTCA1_MSK	BIT(1)
> +#define MAX77686_RTCINT_RTCA2_MSK	BIT(2)
> +#define MAX77686_RTCINT_SMPL_MSK	BIT(3)
> +#define MAX77686_RTCINT_RTC1S_MSK	BIT(4)
> +#define MAX77686_RTCINT_WTSR_MSK	BIT(5)

ditto

> +
>  /* RTC Control Register */
>  #define BCD_EN_SHIFT			0
>  #define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
> @@ -68,8 +152,10 @@ struct max77686_rtc_driver_data {
>  	const unsigned int	*map;
>  	/* Has a separate alarm enable register? */
>  	bool			alarm_enable_reg;
> -	/* Has a separate I2C regmap for the RTC? */
> -	bool			separate_i2c_addr;
> +	/* I2C address for RTC block */
> +	u8			separate_i2c_addr;
> +	/* RTC IRQ CHIP for regmap */
> +	const struct regmap_irq_chip *rtc_irq_chip;
>  };
>  
>  struct max77686_rtc_info {
> @@ -82,7 +168,9 @@ struct max77686_rtc_info {
>  	struct regmap		*rtc_regmap;
>  
>  	const struct max77686_rtc_driver_data *drv_data;
> +	struct regmap_irq_chip_data *rtc_irq_data;
>  
> +	int rtc_irq;
>  	int virq;
>  	int rtc_24hr_mode;
>  };
> @@ -153,12 +241,32 @@ static const unsigned int max77686_map[REG_RTC_END] = {
>  	[REG_RTC_AE1]	     = REG_RTC_NONE,
>  };
>  
> +static const struct regmap_irq max77686_rtc_irqs[] = {
> +	/* RTC interrupts */
> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
> +};
> +
> +static const struct regmap_irq_chip max77686_rtc_irq_chip = {
> +	.name		= "max77686-rtc",
> +	.status_base	= MAX77686_RTC_INT,
> +	.mask_base	= MAX77686_RTC_INTM,
> +	.num_regs	= 1,
> +	.irqs		= max77686_rtc_irqs,
> +	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
> +};
> +
>  static const struct max77686_rtc_driver_data max77686_drv_data = {
>  	.delay = 16000,
>  	.mask  = 0x7f,
>  	.map   = max77686_map,
>  	.alarm_enable_reg  = false,
> -	.separate_i2c_addr = true,
> +	.separate_i2c_addr = 0xC >> 1,

When the header will remain:
I2C_ADDR_RTC
(maybe it needs some prefix?)

> +	.rtc_irq_chip = &max77686_rtc_irq_chip,
>  };
>  
>  static const unsigned int max77802_map[REG_RTC_END] = {
> @@ -190,12 +298,22 @@ static const unsigned int max77802_map[REG_RTC_END] = {
>  	[REG_RTC_AE1]	     = MAX77802_RTC_AE1,
>  };
>  
> +static const struct regmap_irq_chip max77802_rtc_irq_chip = {
> +	.name		= "max77802-rtc",
> +	.status_base	= MAX77802_RTC_INT,
> +	.mask_base	= MAX77802_RTC_INTM,
> +	.num_regs	= 1,
> +	.irqs		= max77686_rtc_irqs, /* same masks as 77686 */
> +	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
> +};
> +
>  static const struct max77686_rtc_driver_data max77802_drv_data = {
>  	.delay = 200,
>  	.mask  = 0xff,
>  	.map   = max77802_map,
>  	.alarm_enable_reg  = true,
> -	.separate_i2c_addr = false,
> +	.separate_i2c_addr = 0,
> +	.rtc_irq_chip = &max77802_rtc_irq_chip,
>  };
>  
>  static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
> @@ -599,9 +717,65 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
>  	return ret;
>  }
>  
> +static const struct regmap_config max77686_rtc_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
> +{
> +	struct device *parent = info->dev->parent;
> +	struct i2c_client *parent_i2c = to_i2c_client(parent);
> +	int ret;
> +
> +	info->rtc_irq =  parent_i2c->irq;

Double space.

> +
> +	info->regmap = dev_get_regmap(parent, NULL);
> +	if (!info->regmap) {
> +		dev_err(info->dev, "Failed to get rtc regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!info->drv_data->separate_i2c_addr) {
> +		info->rtc = parent_i2c;

I think it should remain NULL in such case. You won't be using it
anyway... but in case if you use it then it will be an error anyway
(like in error path below).

> +		goto add_rtc_irq;
> +	}
> +
> +	info->rtc = i2c_new_dummy(parent_i2c->adapter,
> +				info->drv_data->separate_i2c_addr);

Please align the argument on new line.

> +	if (!info->rtc) {
> +		dev_err(info->dev, "Failed to allocate I2C device for RTC\n");
> +		return -ENODEV;
> +	}
> +	i2c_set_clientdata(info->rtc, info);

No need for this. There is no getter. Removal of this could be a
separate patch (before this one).

> +
> +	info->rtc_regmap = devm_regmap_init_i2c(info->rtc,
> +						&max77686_rtc_regmap_config);
> +	if (IS_ERR(info->rtc_regmap)) {
> +		ret = PTR_ERR(info->rtc_regmap);
> +		dev_err(info->dev, "failed to allocate RTC regmap: %d\n", ret);

While moving the messages can you unify the capital letter style (e.g.
all of error msg starting with capital).

> +		goto err_unregister_i2c;
> +	}
> +
> +add_rtc_irq:
> +	ret = regmap_add_irq_chip(info->rtc_regmap, info->rtc_irq,
> +				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
> +				  IRQF_SHARED, 0, info->drv_data->rtc_irq_chip,
> +				  &info->rtc_irq_data);
> +	if (ret < 0) {
> +		dev_err(info->dev, "failed to add RTC irq chip: %d\n", ret);
> +		goto err_unregister_i2c;
> +	}
> +
> +	return 0;
> +
> +err_unregister_i2c:
> +	i2c_unregister_device(info->rtc);

That would unregister the parent. You are missing "if (separate)".

> +	return ret;
> +}
> +
>  static int max77686_rtc_probe(struct platform_device *pdev)
>  {
> -	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
>  	struct max77686_rtc_info *info;
>  	const struct platform_device_id *id = platform_get_device_id(pdev);
>  	int ret;
> @@ -613,18 +787,16 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  
>  	mutex_init(&info->lock);
>  	info->dev = &pdev->dev;
> -	info->rtc = max77686->rtc;
>  	info->drv_data = (const struct max77686_rtc_driver_data *)
>  		id->driver_data;
>  
> -	info->regmap = max77686->regmap;
> -	info->rtc_regmap = (info->drv_data->separate_i2c_addr) ?
> -			    max77686->rtc_regmap : info->regmap;
> +	ret = max77686_init_rtc_regmap(info);
> +	if (ret < 0)
> +		return ret;
>  
>  	platform_set_drvdata(pdev, info);
>  
>  	ret = max77686_rtc_init_reg(info);
> -
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to initialize RTC reg:%d\n", ret);
>  		goto err_rtc;
> @@ -643,13 +815,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  		goto err_rtc;
>  	}
>  
> -	if (!max77686->rtc_irq_data) {
> -		ret = -EINVAL;
> -		dev_err(&pdev->dev, "No RTC regmap IRQ chip\n");
> -		goto err_rtc;
> -	}
> -
> -	info->virq = regmap_irq_get_virq(max77686->rtc_irq_data,
> +	info->virq = regmap_irq_get_virq(info->rtc_irq_data,
>  					 MAX77686_RTCIRQ_RTCA1);
>  	if (!info->virq) {
>  		ret = -ENXIO;
> @@ -659,14 +825,34 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  	ret = devm_request_threaded_irq(&pdev->dev, info->virq, NULL,
>  					max77686_rtc_alarm_irq, 0,
>  					"rtc-alarm1", info);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
>  			info->virq, ret);
> +		goto err_rtc;
> +	}
> +
> +	return 0;
>  
>  err_rtc:
> +	if (info->drv_data->separate_i2c_addr)
> +		i2c_unregister_device(info->rtc);
> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);

Cleanup in reverse-order of allocation. So first del irq_chip then
unregister i2c.

Best regards,
Krzysztof

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 4/4] rtc: max77686: move initialisation of rtc regmap, irq chip locally

[Krzysztof Kozlowski]

On 02.02.2016 22:16, Laxman Dewangan wrote:
> To make RTC block of MAX77686/MAX77802 as independent driver,
> move the registeration of i2c device, regmap for register access

s/registeration/registration/

> and irq_chip for interrupt support inside the RTC driver.
> Removed the same initialisation from mfd driver.

s/mfd/MFD/

I would expect here also information answering to the "why" question.
Why are you doing all of this? Why we want RTC driver to be independent?

> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Mazur <krzysiek@podlesie.net>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>  drivers/mfd/max77686.c               |  82 +------------
>  drivers/rtc/Kconfig                  |   7 +-
>  drivers/rtc/rtc-max77686.c           | 225 ++++++++++++++++++++++++++++++++---
>  include/linux/mfd/max77686-private.h |  16 ---
>  4 files changed, 211 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
> index d959ebb..8254201 100644
> --- a/drivers/mfd/max77686.c
> +++ b/drivers/mfd/max77686.c
> @@ -116,11 +116,6 @@ static const struct regmap_config max77686_regmap_config = {
>  	.val_bits = 8,
>  };
>  
> -static const struct regmap_config max77686_rtc_regmap_config = {
> -	.reg_bits = 8,
> -	.val_bits = 8,
> -};
> -
>  static const struct regmap_config max77802_regmap_config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
> @@ -156,25 +151,6 @@ static const struct regmap_irq_chip max77686_irq_chip = {
>  	.num_irqs		= ARRAY_SIZE(max77686_irqs),
>  };
>  
> -static const struct regmap_irq max77686_rtc_irqs[] = {
> -	/* RTC interrupts */
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
> -};
> -
> -static const struct regmap_irq_chip max77686_rtc_irq_chip = {
> -	.name			= "max77686-rtc",
> -	.status_base		= MAX77686_RTC_INT,
> -	.mask_base		= MAX77686_RTC_INTM,
> -	.num_regs		= 1,
> -	.irqs			= max77686_rtc_irqs,
> -	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
> -};
> -
>  static const struct regmap_irq_chip max77802_irq_chip = {
>  	.name			= "max77802-pmic",
>  	.status_base		= MAX77802_REG_INT1,
> @@ -184,15 +160,6 @@ static const struct regmap_irq_chip max77802_irq_chip = {
>  	.num_irqs		= ARRAY_SIZE(max77686_irqs),
>  };
>  
> -static const struct regmap_irq_chip max77802_rtc_irq_chip = {
> -	.name			= "max77802-rtc",
> -	.status_base		= MAX77802_RTC_INT,
> -	.mask_base		= MAX77802_RTC_INTM,
> -	.num_regs		= 1,
> -	.irqs			= max77686_rtc_irqs, /* same masks as 77686 */
> -	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
> -};
> -
>  static const struct of_device_id max77686_pmic_dt_match[] = {
>  	{
>  		.compatible = "maxim,max77686",
> @@ -214,8 +181,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>  	int ret = 0;
>  	const struct regmap_config *config;
>  	const struct regmap_irq_chip *irq_chip;
> -	const struct regmap_irq_chip *rtc_irq_chip;
> -	struct regmap **rtc_regmap;
>  	const struct mfd_cell *cells;
>  	int n_devs;
>  
> @@ -242,15 +207,11 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>  	if (max77686->type == TYPE_MAX77686) {
>  		config = &max77686_regmap_config;
>  		irq_chip = &max77686_irq_chip;
> -		rtc_irq_chip = &max77686_rtc_irq_chip;
> -		rtc_regmap = &max77686->rtc_regmap;
>  		cells =  max77686_devs;
>  		n_devs = ARRAY_SIZE(max77686_devs);
>  	} else {
>  		config = &max77802_regmap_config;
>  		irq_chip = &max77802_irq_chip;
> -		rtc_irq_chip = &max77802_rtc_irq_chip;
> -		rtc_regmap = &max77686->regmap;
>  		cells =  max77802_devs;
>  		n_devs = ARRAY_SIZE(max77802_devs);
>  	}
> @@ -270,60 +231,25 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>  		return -ENODEV;
>  	}
>  
> -	if (max77686->type == TYPE_MAX77686) {
> -		max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
> -		if (!max77686->rtc) {
> -			dev_err(max77686->dev,
> -				"Failed to allocate I2C device for RTC\n");
> -			return -ENODEV;
> -		}
> -		i2c_set_clientdata(max77686->rtc, max77686);
> -
> -		max77686->rtc_regmap =
> -			devm_regmap_init_i2c(max77686->rtc,
> -					     &max77686_rtc_regmap_config);
> -		if (IS_ERR(max77686->rtc_regmap)) {
> -			ret = PTR_ERR(max77686->rtc_regmap);
> -			dev_err(max77686->dev,
> -				"failed to allocate RTC regmap: %d\n",
> -				ret);
> -			goto err_unregister_i2c;
> -		}
> -	}
> -
>  	ret = regmap_add_irq_chip(max77686->regmap, max77686->irq,
>  				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>  				  IRQF_SHARED, 0, irq_chip,
>  				  &max77686->irq_data);
>  	if (ret) {
>  		dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", ret);
> -		goto err_unregister_i2c;
> -	}
> -
> -	ret = regmap_add_irq_chip(*rtc_regmap, max77686->irq,
> -				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
> -				  IRQF_SHARED, 0, rtc_irq_chip,
> -				  &max77686->rtc_irq_data);
> -	if (ret) {
> -		dev_err(&i2c->dev, "failed to add RTC irq chip: %d\n", ret);
> -		goto err_del_irqc;
> +		return -ENODEV;

return ret;

>  	}
>  
>  	ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL);
>  	if (ret < 0) {
>  		dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
> -		goto err_del_rtc_irqc;
> +		goto err_del_irqc;
>  	}
>  
>  	return 0;
>  
> -err_del_rtc_irqc:
> -	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
>  err_del_irqc:
>  	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
> -err_unregister_i2c:
> -	if (max77686->type == TYPE_MAX77686)
> -		i2c_unregister_device(max77686->rtc);
>  
>  	return ret;
>  }
> @@ -334,12 +260,8 @@ static int max77686_i2c_remove(struct i2c_client *i2c)
>  
>  	mfd_remove_devices(max77686->dev);
>  
> -	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
>  	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
>  
> -	if (max77686->type == TYPE_MAX77686)
> -		i2c_unregister_device(max77686->rtc);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index ec6a253..bdac624 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -325,14 +325,13 @@ config RTC_DRV_MAX8997
>  	  will be called rtc-max8997.
>  
>  config RTC_DRV_MAX77686
> -	tristate "Maxim MAX77686"
> -	depends on MFD_MAX77686

It still depends on some parent. Instantiating this without main MFD
probably would cause errors around max77686_init_rtc_regmap().

> +	tristate "Maxim MAX77686/MAX77802"
>  	help
>  	  If you say yes here you will get support for the
> -	  RTC of Maxim MAX77686 PMIC.
> +	  RTC of Maxim MAX77686/MAX77802 PMIC.
>  
>  	  This driver can also be built as a module. If so, the module
> -	  will be called rtc-max77686.
> +	  will be called rtc-max77686/MAX77802.

The name of module does not change.

>  
>  config RTC_DRV_RK808
>  	tristate "Rockchip RK808 RTC"
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index ab1f2cd..21a88e2 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -12,16 +12,100 @@
>   *
>   */
>  
> +#include <linux/i2c.h>
>  #include <linux/slab.h>
>  #include <linux/rtc.h>
>  #include <linux/delay.h>
>  #include <linux/mutex.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> -#include <linux/mfd/max77686-private.h>

I think the include should remain. No need of duplicating here the
register addresses. Although we wnat the driver to be independent from
parent it is still coming from one parent or another.

>  #include <linux/irqdomain.h>
>  #include <linux/regmap.h>
>  
> +#define MAX77686_REG_STATUS2		0x07
> +
> +enum max77686_rtc_reg {
> +	MAX77686_RTC_INT		= 0x00,
> +	MAX77686_RTC_INTM		= 0x01,
> +	MAX77686_RTC_CONTROLM		= 0x02,
> +	MAX77686_RTC_CONTROL		= 0x03,
> +	MAX77686_RTC_UPDATE0		= 0x04,
> +	/* Reserved: 0x5 */
> +	MAX77686_WTSR_SMPL_CNTL		= 0x06,
> +	MAX77686_RTC_SEC		= 0x07,
> +	MAX77686_RTC_MIN		= 0x08,
> +	MAX77686_RTC_HOUR		= 0x09,
> +	MAX77686_RTC_WEEKDAY		= 0x0A,
> +	MAX77686_RTC_MONTH		= 0x0B,
> +	MAX77686_RTC_YEAR		= 0x0C,
> +	MAX77686_RTC_DATE		= 0x0D,
> +	MAX77686_ALARM1_SEC		= 0x0E,
> +	MAX77686_ALARM1_MIN		= 0x0F,
> +	MAX77686_ALARM1_HOUR		= 0x10,
> +	MAX77686_ALARM1_WEEKDAY		= 0x11,
> +	MAX77686_ALARM1_MONTH		= 0x12,
> +	MAX77686_ALARM1_YEAR		= 0x13,
> +	MAX77686_ALARM1_DATE		= 0x14,
> +	MAX77686_ALARM2_SEC		= 0x15,
> +	MAX77686_ALARM2_MIN		= 0x16,
> +	MAX77686_ALARM2_HOUR		= 0x17,
> +	MAX77686_ALARM2_WEEKDAY		= 0x18,
> +	MAX77686_ALARM2_MONTH		= 0x19,
> +	MAX77686_ALARM2_YEAR		= 0x1A,
> +	MAX77686_ALARM2_DATE		= 0x1B,
> +};
> +
> +enum max77802_rtc_reg {
> +	MAX77802_RTC_INT		= 0xC0,
> +	MAX77802_RTC_INTM		= 0xC1,
> +	MAX77802_RTC_CONTROLM		= 0xC2,
> +	MAX77802_RTC_CONTROL		= 0xC3,
> +	MAX77802_RTC_UPDATE0		= 0xC4,
> +	MAX77802_RTC_UPDATE1		= 0xC5,
> +	MAX77802_WTSR_SMPL_CNTL		= 0xC6,
> +	MAX77802_RTC_SEC		= 0xC7,
> +	MAX77802_RTC_MIN		= 0xC8,
> +	MAX77802_RTC_HOUR		= 0xC9,
> +	MAX77802_RTC_WEEKDAY		= 0xCA,
> +	MAX77802_RTC_MONTH		= 0xCB,
> +	MAX77802_RTC_YEAR		= 0xCC,
> +	MAX77802_RTC_DATE		= 0xCD,
> +	MAX77802_RTC_AE1		= 0xCE,
> +	MAX77802_ALARM1_SEC		= 0xCF,
> +	MAX77802_ALARM1_MIN		= 0xD0,
> +	MAX77802_ALARM1_HOUR		= 0xD1,
> +	MAX77802_ALARM1_WEEKDAY		= 0xD2,
> +	MAX77802_ALARM1_MONTH		= 0xD3,
> +	MAX77802_ALARM1_YEAR		= 0xD4,
> +	MAX77802_ALARM1_DATE		= 0xD5,
> +	MAX77802_RTC_AE2		= 0xD6,
> +	MAX77802_ALARM2_SEC		= 0xD7,
> +	MAX77802_ALARM2_MIN		= 0xD8,
> +	MAX77802_ALARM2_HOUR		= 0xD9,
> +	MAX77802_ALARM2_WEEKDAY		= 0xDA,
> +	MAX77802_ALARM2_MONTH		= 0xDB,
> +	MAX77802_ALARM2_YEAR		= 0xDC,
> +	MAX77802_ALARM2_DATE		= 0xDD,
> +
> +	MAX77802_RTC_END		= 0xDF,
> +};

All of these should come from existing max77686/max77802/max77620 headers.

> +
> +enum max77686_rtc_irq {
> +	MAX77686_RTCIRQ_RTC60S = 0,
> +	MAX77686_RTCIRQ_RTCA1,
> +	MAX77686_RTCIRQ_RTCA2,
> +	MAX77686_RTCIRQ_SMPL,
> +	MAX77686_RTCIRQ_RTC1S,
> +	MAX77686_RTCIRQ_WTSR,
> +};

ditto

> +
> +#define MAX77686_RTCINT_RTC60S_MSK	BIT(0)
> +#define MAX77686_RTCINT_RTCA1_MSK	BIT(1)
> +#define MAX77686_RTCINT_RTCA2_MSK	BIT(2)
> +#define MAX77686_RTCINT_SMPL_MSK	BIT(3)
> +#define MAX77686_RTCINT_RTC1S_MSK	BIT(4)
> +#define MAX77686_RTCINT_WTSR_MSK	BIT(5)

ditto

> +
>  /* RTC Control Register */
>  #define BCD_EN_SHIFT			0
>  #define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
> @@ -68,8 +152,10 @@ struct max77686_rtc_driver_data {
>  	const unsigned int	*map;
>  	/* Has a separate alarm enable register? */
>  	bool			alarm_enable_reg;
> -	/* Has a separate I2C regmap for the RTC? */
> -	bool			separate_i2c_addr;
> +	/* I2C address for RTC block */
> +	u8			separate_i2c_addr;
> +	/* RTC IRQ CHIP for regmap */
> +	const struct regmap_irq_chip *rtc_irq_chip;
>  };
>  
>  struct max77686_rtc_info {
> @@ -82,7 +168,9 @@ struct max77686_rtc_info {
>  	struct regmap		*rtc_regmap;
>  
>  	const struct max77686_rtc_driver_data *drv_data;
> +	struct regmap_irq_chip_data *rtc_irq_data;
>  
> +	int rtc_irq;
>  	int virq;
>  	int rtc_24hr_mode;
>  };
> @@ -153,12 +241,32 @@ static const unsigned int max77686_map[REG_RTC_END] = {
>  	[REG_RTC_AE1]	     = REG_RTC_NONE,
>  };
>  
> +static const struct regmap_irq max77686_rtc_irqs[] = {
> +	/* RTC interrupts */
> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
> +};
> +
> +static const struct regmap_irq_chip max77686_rtc_irq_chip = {
> +	.name		= "max77686-rtc",
> +	.status_base	= MAX77686_RTC_INT,
> +	.mask_base	= MAX77686_RTC_INTM,
> +	.num_regs	= 1,
> +	.irqs		= max77686_rtc_irqs,
> +	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
> +};
> +
>  static const struct max77686_rtc_driver_data max77686_drv_data = {
>  	.delay = 16000,
>  	.mask  = 0x7f,
>  	.map   = max77686_map,
>  	.alarm_enable_reg  = false,
> -	.separate_i2c_addr = true,
> +	.separate_i2c_addr = 0xC >> 1,

When the header will remain:
I2C_ADDR_RTC
(maybe it needs some prefix?)

> +	.rtc_irq_chip = &max77686_rtc_irq_chip,
>  };
>  
>  static const unsigned int max77802_map[REG_RTC_END] = {
> @@ -190,12 +298,22 @@ static const unsigned int max77802_map[REG_RTC_END] = {
>  	[REG_RTC_AE1]	     = MAX77802_RTC_AE1,
>  };
>  
> +static const struct regmap_irq_chip max77802_rtc_irq_chip = {
> +	.name		= "max77802-rtc",
> +	.status_base	= MAX77802_RTC_INT,
> +	.mask_base	= MAX77802_RTC_INTM,
> +	.num_regs	= 1,
> +	.irqs		= max77686_rtc_irqs, /* same masks as 77686 */
> +	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
> +};
> +
>  static const struct max77686_rtc_driver_data max77802_drv_data = {
>  	.delay = 200,
>  	.mask  = 0xff,
>  	.map   = max77802_map,
>  	.alarm_enable_reg  = true,
> -	.separate_i2c_addr = false,
> +	.separate_i2c_addr = 0,
> +	.rtc_irq_chip = &max77802_rtc_irq_chip,
>  };
>  
>  static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
> @@ -599,9 +717,65 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
>  	return ret;
>  }
>  
> +static const struct regmap_config max77686_rtc_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
> +{
> +	struct device *parent = info->dev->parent;
> +	struct i2c_client *parent_i2c = to_i2c_client(parent);
> +	int ret;
> +
> +	info->rtc_irq =  parent_i2c->irq;

Double space.

> +
> +	info->regmap = dev_get_regmap(parent, NULL);
> +	if (!info->regmap) {
> +		dev_err(info->dev, "Failed to get rtc regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!info->drv_data->separate_i2c_addr) {
> +		info->rtc = parent_i2c;

I think it should remain NULL in such case. You won't be using it
anyway... but in case if you use it then it will be an error anyway
(like in error path below).

> +		goto add_rtc_irq;
> +	}
> +
> +	info->rtc = i2c_new_dummy(parent_i2c->adapter,
> +				info->drv_data->separate_i2c_addr);

Please align the argument on new line.

> +	if (!info->rtc) {
> +		dev_err(info->dev, "Failed to allocate I2C device for RTC\n");
> +		return -ENODEV;
> +	}
> +	i2c_set_clientdata(info->rtc, info);

No need for this. There is no getter. Removal of this could be a
separate patch (before this one).

> +
> +	info->rtc_regmap = devm_regmap_init_i2c(info->rtc,
> +						&max77686_rtc_regmap_config);
> +	if (IS_ERR(info->rtc_regmap)) {
> +		ret = PTR_ERR(info->rtc_regmap);
> +		dev_err(info->dev, "failed to allocate RTC regmap: %d\n", ret);

While moving the messages can you unify the capital letter style (e.g.
all of error msg starting with capital).

> +		goto err_unregister_i2c;
> +	}
> +
> +add_rtc_irq:
> +	ret = regmap_add_irq_chip(info->rtc_regmap, info->rtc_irq,
> +				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
> +				  IRQF_SHARED, 0, info->drv_data->rtc_irq_chip,
> +				  &info->rtc_irq_data);
> +	if (ret < 0) {
> +		dev_err(info->dev, "failed to add RTC irq chip: %d\n", ret);
> +		goto err_unregister_i2c;
> +	}
> +
> +	return 0;
> +
> +err_unregister_i2c:
> +	i2c_unregister_device(info->rtc);

That would unregister the parent. You are missing "if (separate)".

> +	return ret;
> +}
> +
>  static int max77686_rtc_probe(struct platform_device *pdev)
>  {
> -	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
>  	struct max77686_rtc_info *info;
>  	const struct platform_device_id *id = platform_get_device_id(pdev);
>  	int ret;
> @@ -613,18 +787,16 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  
>  	mutex_init(&info->lock);
>  	info->dev = &pdev->dev;
> -	info->rtc = max77686->rtc;
>  	info->drv_data = (const struct max77686_rtc_driver_data *)
>  		id->driver_data;
>  
> -	info->regmap = max77686->regmap;
> -	info->rtc_regmap = (info->drv_data->separate_i2c_addr) ?
> -			    max77686->rtc_regmap : info->regmap;
> +	ret = max77686_init_rtc_regmap(info);
> +	if (ret < 0)
> +		return ret;
>  
>  	platform_set_drvdata(pdev, info);
>  
>  	ret = max77686_rtc_init_reg(info);
> -
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to initialize RTC reg:%d\n", ret);
>  		goto err_rtc;
> @@ -643,13 +815,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  		goto err_rtc;
>  	}
>  
> -	if (!max77686->rtc_irq_data) {
> -		ret = -EINVAL;
> -		dev_err(&pdev->dev, "No RTC regmap IRQ chip\n");
> -		goto err_rtc;
> -	}
> -
> -	info->virq = regmap_irq_get_virq(max77686->rtc_irq_data,
> +	info->virq = regmap_irq_get_virq(info->rtc_irq_data,
>  					 MAX77686_RTCIRQ_RTCA1);
>  	if (!info->virq) {
>  		ret = -ENXIO;
> @@ -659,14 +825,34 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  	ret = devm_request_threaded_irq(&pdev->dev, info->virq, NULL,
>  					max77686_rtc_alarm_irq, 0,
>  					"rtc-alarm1", info);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
>  			info->virq, ret);
> +		goto err_rtc;
> +	}
> +
> +	return 0;
>  
>  err_rtc:
> +	if (info->drv_data->separate_i2c_addr)
> +		i2c_unregister_device(info->rtc);
> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);

Cleanup in reverse-order of allocation. So first del irq_chip then
unregister i2c.

Best regards,
Krzysztof

[rtc-linux] Re: [PATCH 4/4] rtc: max77686: move initialisation of rtc regmap, irq chip locally

[Javier Martinez Canillas]

Hello Laxman,

I'll comment on top of Krzysztof's answer since I agree with his remarks.

On 02/02/2016 10:17 PM, Krzysztof Kozlowski wrote:
> On 02.02.2016 22:16, Laxman Dewangan wrote:
>> To make RTC block of MAX77686/MAX77802 as independent driver,
>> move the registeration of i2c device, regmap for register access
>
> s/registeration/registration/
>
>> and irq_chip for interrupt support inside the RTC driver.
>> Removed the same initialisation from mfd driver.
>
> s/mfd/MFD/
>
> I would expect here also information answering to the "why" question.
> Why are you doing all of this? Why we want RTC driver to be independent?
>
>>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>> CC: Krzysztof Mazur <krzysiek@podlesie.net>

I guess you meant Krzysztof Kozlowski <k.kozlowski@samsung.com> here?

Unless I missed some previous discussion and Krzysztof Mazur was involved?

>> CC: Javier Martinez Canillas <javier@osg.samsung.com>
>> ---
>>   drivers/mfd/max77686.c               |  82 +------------
>>   drivers/rtc/Kconfig                  |   7 +-
>>   drivers/rtc/rtc-max77686.c           | 225 ++++++++++++++++++++++++++++++++---
>>   include/linux/mfd/max77686-private.h |  16 ---
>>   4 files changed, 211 insertions(+), 119 deletions(-)
>>
>> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
>> index d959ebb..8254201 100644
>> --- a/drivers/mfd/max77686.c
>> +++ b/drivers/mfd/max77686.c
>> @@ -116,11 +116,6 @@ static const struct regmap_config max77686_regmap_config = {
>>   	.val_bits = 8,
>>   };
>>
>> -static const struct regmap_config max77686_rtc_regmap_config = {
>> -	.reg_bits = 8,
>> -	.val_bits = 8,
>> -};
>> -
>>   static const struct regmap_config max77802_regmap_config = {
>>   	.reg_bits = 8,
>>   	.val_bits = 8,
>> @@ -156,25 +151,6 @@ static const struct regmap_irq_chip max77686_irq_chip = {
>>   	.num_irqs		= ARRAY_SIZE(max77686_irqs),
>>   };
>>
>> -static const struct regmap_irq max77686_rtc_irqs[] = {
>> -	/* RTC interrupts */
>> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
>> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
>> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
>> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
>> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
>> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
>> -};
>> -
>> -static const struct regmap_irq_chip max77686_rtc_irq_chip = {
>> -	.name			= "max77686-rtc",
>> -	.status_base		= MAX77686_RTC_INT,
>> -	.mask_base		= MAX77686_RTC_INTM,
>> -	.num_regs		= 1,
>> -	.irqs			= max77686_rtc_irqs,
>> -	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
>> -};
>> -
>>   static const struct regmap_irq_chip max77802_irq_chip = {
>>   	.name			= "max77802-pmic",
>>   	.status_base		= MAX77802_REG_INT1,
>> @@ -184,15 +160,6 @@ static const struct regmap_irq_chip max77802_irq_chip = {
>>   	.num_irqs		= ARRAY_SIZE(max77686_irqs),
>>   };
>>
>> -static const struct regmap_irq_chip max77802_rtc_irq_chip = {
>> -	.name			= "max77802-rtc",
>> -	.status_base		= MAX77802_RTC_INT,
>> -	.mask_base		= MAX77802_RTC_INTM,
>> -	.num_regs		= 1,
>> -	.irqs			= max77686_rtc_irqs, /* same masks as 77686 */
>> -	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
>> -};
>> -
>>   static const struct of_device_id max77686_pmic_dt_match[] = {
>>   	{
>>   		.compatible = "maxim,max77686",
>> @@ -214,8 +181,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>>   	int ret = 0;
>>   	const struct regmap_config *config;
>>   	const struct regmap_irq_chip *irq_chip;
>> -	const struct regmap_irq_chip *rtc_irq_chip;
>> -	struct regmap **rtc_regmap;
>>   	const struct mfd_cell *cells;
>>   	int n_devs;
>>
>> @@ -242,15 +207,11 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>>   	if (max77686->type == TYPE_MAX77686) {
>>   		config = &max77686_regmap_config;
>>   		irq_chip = &max77686_irq_chip;
>> -		rtc_irq_chip = &max77686_rtc_irq_chip;
>> -		rtc_regmap = &max77686->rtc_regmap;
>>   		cells =  max77686_devs;
>>   		n_devs = ARRAY_SIZE(max77686_devs);
>>   	} else {
>>   		config = &max77802_regmap_config;
>>   		irq_chip = &max77802_irq_chip;
>> -		rtc_irq_chip = &max77802_rtc_irq_chip;
>> -		rtc_regmap = &max77686->regmap;
>>   		cells =  max77802_devs;
>>   		n_devs = ARRAY_SIZE(max77802_devs);
>>   	}
>> @@ -270,60 +231,25 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>>   		return -ENODEV;
>>   	}
>>
>> -	if (max77686->type == TYPE_MAX77686) {
>> -		max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
>> -		if (!max77686->rtc) {
>> -			dev_err(max77686->dev,
>> -				"Failed to allocate I2C device for RTC\n");
>> -			return -ENODEV;
>> -		}
>> -		i2c_set_clientdata(max77686->rtc, max77686);
>> -
>> -		max77686->rtc_regmap =
>> -			devm_regmap_init_i2c(max77686->rtc,
>> -					     &max77686_rtc_regmap_config);
>> -		if (IS_ERR(max77686->rtc_regmap)) {
>> -			ret = PTR_ERR(max77686->rtc_regmap);
>> -			dev_err(max77686->dev,
>> -				"failed to allocate RTC regmap: %d\n",
>> -				ret);
>> -			goto err_unregister_i2c;
>> -		}
>> -	}
>> -
>>   	ret = regmap_add_irq_chip(max77686->regmap, max77686->irq,
>>   				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>>   				  IRQF_SHARED, 0, irq_chip,
>>   				  &max77686->irq_data);
>>   	if (ret) {
>>   		dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", ret);
>> -		goto err_unregister_i2c;
>> -	}
>> -
>> -	ret = regmap_add_irq_chip(*rtc_regmap, max77686->irq,
>> -				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>> -				  IRQF_SHARED, 0, rtc_irq_chip,
>> -				  &max77686->rtc_irq_data);
>> -	if (ret) {
>> -		dev_err(&i2c->dev, "failed to add RTC irq chip: %d\n", ret);
>> -		goto err_del_irqc;
>> +		return -ENODEV;
>
> return ret;
>
>>   	}
>>
>>   	ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL);
>>   	if (ret < 0) {
>>   		dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
>> -		goto err_del_rtc_irqc;
>> +		goto err_del_irqc;
>>   	}
>>
>>   	return 0;
>>
>> -err_del_rtc_irqc:
>> -	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
>>   err_del_irqc:
>>   	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
>> -err_unregister_i2c:
>> -	if (max77686->type == TYPE_MAX77686)
>> -		i2c_unregister_device(max77686->rtc);
>>
>>   	return ret;
>>   }
>> @@ -334,12 +260,8 @@ static int max77686_i2c_remove(struct i2c_client *i2c)
>>
>>   	mfd_remove_devices(max77686->dev);
>>
>> -	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
>>   	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
>>
>> -	if (max77686->type == TYPE_MAX77686)
>> -		i2c_unregister_device(max77686->rtc);
>> -
>>   	return 0;
>>   }
>>
>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>> index ec6a253..bdac624 100644
>> --- a/drivers/rtc/Kconfig
>> +++ b/drivers/rtc/Kconfig
>> @@ -325,14 +325,13 @@ config RTC_DRV_MAX8997
>>   	  will be called rtc-max8997.
>>
>>   config RTC_DRV_MAX77686
>> -	tristate "Maxim MAX77686"
>> -	depends on MFD_MAX77686
>
> It still depends on some parent. Instantiating this without main MFD
> probably would cause errors around max77686_init_rtc_regmap().
>

Indeed, if the RTC device can be registered by different MFD drivers,
then the driver should depend on X || Y

>> +	tristate "Maxim MAX77686/MAX77802"
>>   	help
>>   	  If you say yes here you will get support for the
>> -	  RTC of Maxim MAX77686 PMIC.
>> +	  RTC of Maxim MAX77686/MAX77802 PMIC.
>>
>>   	  This driver can also be built as a module. If so, the module
>> -	  will be called rtc-max77686.
>> +	  will be called rtc-max77686/MAX77802.
>
> The name of module does not change.
>
>>
>>   config RTC_DRV_RK808
>>   	tristate "Rockchip RK808 RTC"
>> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
>> index ab1f2cd..21a88e2 100644
>> --- a/drivers/rtc/rtc-max77686.c
>> +++ b/drivers/rtc/rtc-max77686.c
>> @@ -12,16 +12,100 @@
>>    *
>>    */
>>
>> +#include <linux/i2c.h>
>>   #include <linux/slab.h>
>>   #include <linux/rtc.h>
>>   #include <linux/delay.h>
>>   #include <linux/mutex.h>
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>> -#include <linux/mfd/max77686-private.h>
>
> I think the include should remain. No need of duplicating here the
> register addresses. Although we wnat the driver to be independent from
> parent it is still coming from one parent or another.
>
>>   #include <linux/irqdomain.h>
>>   #include <linux/regmap.h>
>>
>> +#define MAX77686_REG_STATUS2		0x07
>> +
>> +enum max77686_rtc_reg {
>> +	MAX77686_RTC_INT		= 0x00,
>> +	MAX77686_RTC_INTM		= 0x01,
>> +	MAX77686_RTC_CONTROLM		= 0x02,
>> +	MAX77686_RTC_CONTROL		= 0x03,
>> +	MAX77686_RTC_UPDATE0		= 0x04,
>> +	/* Reserved: 0x5 */
>> +	MAX77686_WTSR_SMPL_CNTL		= 0x06,
>> +	MAX77686_RTC_SEC		= 0x07,
>> +	MAX77686_RTC_MIN		= 0x08,
>> +	MAX77686_RTC_HOUR		= 0x09,
>> +	MAX77686_RTC_WEEKDAY		= 0x0A,
>> +	MAX77686_RTC_MONTH		= 0x0B,
>> +	MAX77686_RTC_YEAR		= 0x0C,
>> +	MAX77686_RTC_DATE		= 0x0D,
>> +	MAX77686_ALARM1_SEC		= 0x0E,
>> +	MAX77686_ALARM1_MIN		= 0x0F,
>> +	MAX77686_ALARM1_HOUR		= 0x10,
>> +	MAX77686_ALARM1_WEEKDAY		= 0x11,
>> +	MAX77686_ALARM1_MONTH		= 0x12,
>> +	MAX77686_ALARM1_YEAR		= 0x13,
>> +	MAX77686_ALARM1_DATE		= 0x14,
>> +	MAX77686_ALARM2_SEC		= 0x15,
>> +	MAX77686_ALARM2_MIN		= 0x16,
>> +	MAX77686_ALARM2_HOUR		= 0x17,
>> +	MAX77686_ALARM2_WEEKDAY		= 0x18,
>> +	MAX77686_ALARM2_MONTH		= 0x19,
>> +	MAX77686_ALARM2_YEAR		= 0x1A,
>> +	MAX77686_ALARM2_DATE		= 0x1B,
>> +};
>> +
>> +enum max77802_rtc_reg {
>> +	MAX77802_RTC_INT		= 0xC0,
>> +	MAX77802_RTC_INTM		= 0xC1,
>> +	MAX77802_RTC_CONTROLM		= 0xC2,
>> +	MAX77802_RTC_CONTROL		= 0xC3,
>> +	MAX77802_RTC_UPDATE0		= 0xC4,
>> +	MAX77802_RTC_UPDATE1		= 0xC5,
>> +	MAX77802_WTSR_SMPL_CNTL		= 0xC6,
>> +	MAX77802_RTC_SEC		= 0xC7,
>> +	MAX77802_RTC_MIN		= 0xC8,
>> +	MAX77802_RTC_HOUR		= 0xC9,
>> +	MAX77802_RTC_WEEKDAY		= 0xCA,
>> +	MAX77802_RTC_MONTH		= 0xCB,
>> +	MAX77802_RTC_YEAR		= 0xCC,
>> +	MAX77802_RTC_DATE		= 0xCD,
>> +	MAX77802_RTC_AE1		= 0xCE,
>> +	MAX77802_ALARM1_SEC		= 0xCF,
>> +	MAX77802_ALARM1_MIN		= 0xD0,
>> +	MAX77802_ALARM1_HOUR		= 0xD1,
>> +	MAX77802_ALARM1_WEEKDAY		= 0xD2,
>> +	MAX77802_ALARM1_MONTH		= 0xD3,
>> +	MAX77802_ALARM1_YEAR		= 0xD4,
>> +	MAX77802_ALARM1_DATE		= 0xD5,
>> +	MAX77802_RTC_AE2		= 0xD6,
>> +	MAX77802_ALARM2_SEC		= 0xD7,
>> +	MAX77802_ALARM2_MIN		= 0xD8,
>> +	MAX77802_ALARM2_HOUR		= 0xD9,
>> +	MAX77802_ALARM2_WEEKDAY		= 0xDA,
>> +	MAX77802_ALARM2_MONTH		= 0xDB,
>> +	MAX77802_ALARM2_YEAR		= 0xDC,
>> +	MAX77802_ALARM2_DATE		= 0xDD,
>> +
>> +	MAX77802_RTC_END		= 0xDF,
>> +};
>
> All of these should come from existing max77686/max77802/max77620 headers.
>
>> +
>> +enum max77686_rtc_irq {
>> +	MAX77686_RTCIRQ_RTC60S = 0,
>> +	MAX77686_RTCIRQ_RTCA1,
>> +	MAX77686_RTCIRQ_RTCA2,
>> +	MAX77686_RTCIRQ_SMPL,
>> +	MAX77686_RTCIRQ_RTC1S,
>> +	MAX77686_RTCIRQ_WTSR,
>> +};
>
> ditto
>
>> +
>> +#define MAX77686_RTCINT_RTC60S_MSK	BIT(0)
>> +#define MAX77686_RTCINT_RTCA1_MSK	BIT(1)
>> +#define MAX77686_RTCINT_RTCA2_MSK	BIT(2)
>> +#define MAX77686_RTCINT_SMPL_MSK	BIT(3)
>> +#define MAX77686_RTCINT_RTC1S_MSK	BIT(4)
>> +#define MAX77686_RTCINT_WTSR_MSK	BIT(5)
>
> ditto
>
>> +
>>   /* RTC Control Register */
>>   #define BCD_EN_SHIFT			0
>>   #define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
>> @@ -68,8 +152,10 @@ struct max77686_rtc_driver_data {
>>   	const unsigned int	*map;
>>   	/* Has a separate alarm enable register? */
>>   	bool			alarm_enable_reg;
>> -	/* Has a separate I2C regmap for the RTC? */
>> -	bool			separate_i2c_addr;
>> +	/* I2C address for RTC block */
>> +	u8			separate_i2c_addr;

The separate_i2c_addr field name made sense for a bool since it was
answering a question but now that is a u8, I believe that should be
called rtc_i2c_addr or something like that instead.

>> +	/* RTC IRQ CHIP for regmap */
>> +	const struct regmap_irq_chip *rtc_irq_chip;
>>   };
>>
>>   struct max77686_rtc_info {
>> @@ -82,7 +168,9 @@ struct max77686_rtc_info {
>>   	struct regmap		*rtc_regmap;
>>
>>   	const struct max77686_rtc_driver_data *drv_data;
>> +	struct regmap_irq_chip_data *rtc_irq_data;
>>
>> +	int rtc_irq;
>>   	int virq;
>>   	int rtc_24hr_mode;
>>   };
>> @@ -153,12 +241,32 @@ static const unsigned int max77686_map[REG_RTC_END] = {
>>   	[REG_RTC_AE1]	     = REG_RTC_NONE,
>>   };
>>
>> +static const struct regmap_irq max77686_rtc_irqs[] = {
>> +	/* RTC interrupts */
>> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
>> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
>> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
>> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
>> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
>> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
>> +};
>> +
>> +static const struct regmap_irq_chip max77686_rtc_irq_chip = {
>> +	.name		= "max77686-rtc",
>> +	.status_base	= MAX77686_RTC_INT,
>> +	.mask_base	= MAX77686_RTC_INTM,
>> +	.num_regs	= 1,
>> +	.irqs		= max77686_rtc_irqs,
>> +	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
>> +};
>> +
>>   static const struct max77686_rtc_driver_data max77686_drv_data = {
>>   	.delay = 16000,
>>   	.mask  = 0x7f,
>>   	.map   = max77686_map,
>>   	.alarm_enable_reg  = false,
>> -	.separate_i2c_addr = true,
>> +	.separate_i2c_addr = 0xC >> 1,
>
> When the header will remain:
> I2C_ADDR_RTC
> (maybe it needs some prefix?)
>
>> +	.rtc_irq_chip = &max77686_rtc_irq_chip,
>>   };
>>
>>   static const unsigned int max77802_map[REG_RTC_END] = {
>> @@ -190,12 +298,22 @@ static const unsigned int max77802_map[REG_RTC_END] = {
>>   	[REG_RTC_AE1]	     = MAX77802_RTC_AE1,
>>   };
>>
>> +static const struct regmap_irq_chip max77802_rtc_irq_chip = {
>> +	.name		= "max77802-rtc",
>> +	.status_base	= MAX77802_RTC_INT,
>> +	.mask_base	= MAX77802_RTC_INTM,
>> +	.num_regs	= 1,
>> +	.irqs		= max77686_rtc_irqs, /* same masks as 77686 */
>> +	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
>> +};
>> +
>>   static const struct max77686_rtc_driver_data max77802_drv_data = {
>>   	.delay = 200,
>>   	.mask  = 0xff,
>>   	.map   = max77802_map,
>>   	.alarm_enable_reg  = true,
>> -	.separate_i2c_addr = false,
>> +	.separate_i2c_addr = 0,

AFAIK 0 is a reserved address for I2C (general call address) so maybe
it would be better to use an invalid address to specify that the RTC
doesn't have a separate I2C address for the registers and check this?

>> +	.rtc_irq_chip = &max77802_rtc_irq_chip,
>>   };
>>
>>   static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
>> @@ -599,9 +717,65 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
>>   	return ret;
>>   }
>>
>> +static const struct regmap_config max77686_rtc_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +};
>> +
>> +static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
>> +{
>> +	struct device *parent = info->dev->parent;
>> +	struct i2c_client *parent_i2c = to_i2c_client(parent);
>> +	int ret;
>> +
>> +	info->rtc_irq =  parent_i2c->irq;
>
> Double space.
>
>> +
>> +	info->regmap = dev_get_regmap(parent, NULL);
>> +	if (!info->regmap) {
>> +		dev_err(info->dev, "Failed to get rtc regmap\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!info->drv_data->separate_i2c_addr) {
>> +		info->rtc = parent_i2c;
>
> I think it should remain NULL in such case. You won't be using it
> anyway... but in case if you use it then it will be an error anyway
> (like in error path below).
>
>> +		goto add_rtc_irq;
>> +	}
>> +
>> +	info->rtc = i2c_new_dummy(parent_i2c->adapter,
>> +				info->drv_data->separate_i2c_addr);
>
> Please align the argument on new line.
>
>> +	if (!info->rtc) {
>> +		dev_err(info->dev, "Failed to allocate I2C device for RTC\n");
>> +		return -ENODEV;
>> +	}
>> +	i2c_set_clientdata(info->rtc, info);
>
> No need for this. There is no getter. Removal of this could be a
> separate patch (before this one).
>
>> +
>> +	info->rtc_regmap = devm_regmap_init_i2c(info->rtc,
>> +						&max77686_rtc_regmap_config);
>> +	if (IS_ERR(info->rtc_regmap)) {
>> +		ret = PTR_ERR(info->rtc_regmap);
>> +		dev_err(info->dev, "failed to allocate RTC regmap: %d\n", ret);
>
> While moving the messages can you unify the capital letter style (e.g.
> all of error msg starting with capital).
>
>> +		goto err_unregister_i2c;
>> +	}
>> +
>> +add_rtc_irq:
>> +	ret = regmap_add_irq_chip(info->rtc_regmap, info->rtc_irq,
>> +				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>> +				  IRQF_SHARED, 0, info->drv_data->rtc_irq_chip,
>> +				  &info->rtc_irq_data);
>> +	if (ret < 0) {
>> +		dev_err(info->dev, "failed to add RTC irq chip: %d\n", ret);
>> +		goto err_unregister_i2c;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_unregister_i2c:
>> +	i2c_unregister_device(info->rtc);
>
> That would unregister the parent. You are missing "if (separate)".
>
>> +	return ret;
>> +}
>> +
>>   static int max77686_rtc_probe(struct platform_device *pdev)
>>   {
>> -	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
>>   	struct max77686_rtc_info *info;
>>   	const struct platform_device_id *id = platform_get_device_id(pdev);
>>   	int ret;
>> @@ -613,18 +787,16 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>
>>   	mutex_init(&info->lock);
>>   	info->dev = &pdev->dev;
>> -	info->rtc = max77686->rtc;
>>   	info->drv_data = (const struct max77686_rtc_driver_data *)
>>   		id->driver_data;
>>
>> -	info->regmap = max77686->regmap;
>> -	info->rtc_regmap = (info->drv_data->separate_i2c_addr) ?
>> -			    max77686->rtc_regmap : info->regmap;
>> +	ret = max77686_init_rtc_regmap(info);
>> +	if (ret < 0)
>> +		return ret;
>>
>>   	platform_set_drvdata(pdev, info);
>>
>>   	ret = max77686_rtc_init_reg(info);
>> -
>>   	if (ret < 0) {
>>   		dev_err(&pdev->dev, "Failed to initialize RTC reg:%d\n", ret);
>>   		goto err_rtc;
>> @@ -643,13 +815,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>   		goto err_rtc;
>>   	}
>>
>> -	if (!max77686->rtc_irq_data) {
>> -		ret = -EINVAL;
>> -		dev_err(&pdev->dev, "No RTC regmap IRQ chip\n");
>> -		goto err_rtc;
>> -	}
>> -
>> -	info->virq = regmap_irq_get_virq(max77686->rtc_irq_data,
>> +	info->virq = regmap_irq_get_virq(info->rtc_irq_data,
>>   					 MAX77686_RTCIRQ_RTCA1);
>>   	if (!info->virq) {
>>   		ret = -ENXIO;
>> @@ -659,14 +825,34 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>   	ret = devm_request_threaded_irq(&pdev->dev, info->virq, NULL,
>>   					max77686_rtc_alarm_irq, 0,
>>   					"rtc-alarm1", info);
>> -	if (ret < 0)
>> +	if (ret < 0) {
>>   		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
>>   			info->virq, ret);
>> +		goto err_rtc;
>> +	}
>> +
>> +	return 0;
>>
>>   err_rtc:
>> +	if (info->drv_data->separate_i2c_addr)
>> +		i2c_unregister_device(info->rtc);
>> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
>
> Cleanup in reverse-order of allocation. So first del irq_chip then
> unregister i2c.
>
> Best regards,
> Krzysztof
>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 4/4] rtc: max77686: move initialisation of rtc regmap, irq chip locally

[Javier Martinez Canillas]

Hello Laxman,

I'll comment on top of Krzysztof's answer since I agree with his remarks.

On 02/02/2016 10:17 PM, Krzysztof Kozlowski wrote:
> On 02.02.2016 22:16, Laxman Dewangan wrote:
>> To make RTC block of MAX77686/MAX77802 as independent driver,
>> move the registeration of i2c device, regmap for register access
>
> s/registeration/registration/
>
>> and irq_chip for interrupt support inside the RTC driver.
>> Removed the same initialisation from mfd driver.
>
> s/mfd/MFD/
>
> I would expect here also information answering to the "why" question.
> Why are you doing all of this? Why we want RTC driver to be independent?
>
>>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>> CC: Krzysztof Mazur <krzysiek@podlesie.net>

I guess you meant Krzysztof Kozlowski <k.kozlowski@samsung.com> here?

Unless I missed some previous discussion and Krzysztof Mazur was involved?

>> CC: Javier Martinez Canillas <javier@osg.samsung.com>
>> ---
>>   drivers/mfd/max77686.c               |  82 +------------
>>   drivers/rtc/Kconfig                  |   7 +-
>>   drivers/rtc/rtc-max77686.c           | 225 ++++++++++++++++++++++++++++++++---
>>   include/linux/mfd/max77686-private.h |  16 ---
>>   4 files changed, 211 insertions(+), 119 deletions(-)
>>
>> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
>> index d959ebb..8254201 100644
>> --- a/drivers/mfd/max77686.c
>> +++ b/drivers/mfd/max77686.c
>> @@ -116,11 +116,6 @@ static const struct regmap_config max77686_regmap_config = {
>>   	.val_bits = 8,
>>   };
>>
>> -static const struct regmap_config max77686_rtc_regmap_config = {
>> -	.reg_bits = 8,
>> -	.val_bits = 8,
>> -};
>> -
>>   static const struct regmap_config max77802_regmap_config = {
>>   	.reg_bits = 8,
>>   	.val_bits = 8,
>> @@ -156,25 +151,6 @@ static const struct regmap_irq_chip max77686_irq_chip = {
>>   	.num_irqs		= ARRAY_SIZE(max77686_irqs),
>>   };
>>
>> -static const struct regmap_irq max77686_rtc_irqs[] = {
>> -	/* RTC interrupts */
>> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
>> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
>> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
>> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
>> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
>> -	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
>> -};
>> -
>> -static const struct regmap_irq_chip max77686_rtc_irq_chip = {
>> -	.name			= "max77686-rtc",
>> -	.status_base		= MAX77686_RTC_INT,
>> -	.mask_base		= MAX77686_RTC_INTM,
>> -	.num_regs		= 1,
>> -	.irqs			= max77686_rtc_irqs,
>> -	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
>> -};
>> -
>>   static const struct regmap_irq_chip max77802_irq_chip = {
>>   	.name			= "max77802-pmic",
>>   	.status_base		= MAX77802_REG_INT1,
>> @@ -184,15 +160,6 @@ static const struct regmap_irq_chip max77802_irq_chip = {
>>   	.num_irqs		= ARRAY_SIZE(max77686_irqs),
>>   };
>>
>> -static const struct regmap_irq_chip max77802_rtc_irq_chip = {
>> -	.name			= "max77802-rtc",
>> -	.status_base		= MAX77802_RTC_INT,
>> -	.mask_base		= MAX77802_RTC_INTM,
>> -	.num_regs		= 1,
>> -	.irqs			= max77686_rtc_irqs, /* same masks as 77686 */
>> -	.num_irqs		= ARRAY_SIZE(max77686_rtc_irqs),
>> -};
>> -
>>   static const struct of_device_id max77686_pmic_dt_match[] = {
>>   	{
>>   		.compatible = "maxim,max77686",
>> @@ -214,8 +181,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>>   	int ret = 0;
>>   	const struct regmap_config *config;
>>   	const struct regmap_irq_chip *irq_chip;
>> -	const struct regmap_irq_chip *rtc_irq_chip;
>> -	struct regmap **rtc_regmap;
>>   	const struct mfd_cell *cells;
>>   	int n_devs;
>>
>> @@ -242,15 +207,11 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>>   	if (max77686->type == TYPE_MAX77686) {
>>   		config = &max77686_regmap_config;
>>   		irq_chip = &max77686_irq_chip;
>> -		rtc_irq_chip = &max77686_rtc_irq_chip;
>> -		rtc_regmap = &max77686->rtc_regmap;
>>   		cells =  max77686_devs;
>>   		n_devs = ARRAY_SIZE(max77686_devs);
>>   	} else {
>>   		config = &max77802_regmap_config;
>>   		irq_chip = &max77802_irq_chip;
>> -		rtc_irq_chip = &max77802_rtc_irq_chip;
>> -		rtc_regmap = &max77686->regmap;
>>   		cells =  max77802_devs;
>>   		n_devs = ARRAY_SIZE(max77802_devs);
>>   	}
>> @@ -270,60 +231,25 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
>>   		return -ENODEV;
>>   	}
>>
>> -	if (max77686->type == TYPE_MAX77686) {
>> -		max77686->rtc = i2c_new_dummy(i2c->adapter, I2C_ADDR_RTC);
>> -		if (!max77686->rtc) {
>> -			dev_err(max77686->dev,
>> -				"Failed to allocate I2C device for RTC\n");
>> -			return -ENODEV;
>> -		}
>> -		i2c_set_clientdata(max77686->rtc, max77686);
>> -
>> -		max77686->rtc_regmap =
>> -			devm_regmap_init_i2c(max77686->rtc,
>> -					     &max77686_rtc_regmap_config);
>> -		if (IS_ERR(max77686->rtc_regmap)) {
>> -			ret = PTR_ERR(max77686->rtc_regmap);
>> -			dev_err(max77686->dev,
>> -				"failed to allocate RTC regmap: %d\n",
>> -				ret);
>> -			goto err_unregister_i2c;
>> -		}
>> -	}
>> -
>>   	ret = regmap_add_irq_chip(max77686->regmap, max77686->irq,
>>   				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>>   				  IRQF_SHARED, 0, irq_chip,
>>   				  &max77686->irq_data);
>>   	if (ret) {
>>   		dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", ret);
>> -		goto err_unregister_i2c;
>> -	}
>> -
>> -	ret = regmap_add_irq_chip(*rtc_regmap, max77686->irq,
>> -				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>> -				  IRQF_SHARED, 0, rtc_irq_chip,
>> -				  &max77686->rtc_irq_data);
>> -	if (ret) {
>> -		dev_err(&i2c->dev, "failed to add RTC irq chip: %d\n", ret);
>> -		goto err_del_irqc;
>> +		return -ENODEV;
>
> return ret;
>
>>   	}
>>
>>   	ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL);
>>   	if (ret < 0) {
>>   		dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
>> -		goto err_del_rtc_irqc;
>> +		goto err_del_irqc;
>>   	}
>>
>>   	return 0;
>>
>> -err_del_rtc_irqc:
>> -	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
>>   err_del_irqc:
>>   	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
>> -err_unregister_i2c:
>> -	if (max77686->type == TYPE_MAX77686)
>> -		i2c_unregister_device(max77686->rtc);
>>
>>   	return ret;
>>   }
>> @@ -334,12 +260,8 @@ static int max77686_i2c_remove(struct i2c_client *i2c)
>>
>>   	mfd_remove_devices(max77686->dev);
>>
>> -	regmap_del_irq_chip(max77686->irq, max77686->rtc_irq_data);
>>   	regmap_del_irq_chip(max77686->irq, max77686->irq_data);
>>
>> -	if (max77686->type == TYPE_MAX77686)
>> -		i2c_unregister_device(max77686->rtc);
>> -
>>   	return 0;
>>   }
>>
>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>> index ec6a253..bdac624 100644
>> --- a/drivers/rtc/Kconfig
>> +++ b/drivers/rtc/Kconfig
>> @@ -325,14 +325,13 @@ config RTC_DRV_MAX8997
>>   	  will be called rtc-max8997.
>>
>>   config RTC_DRV_MAX77686
>> -	tristate "Maxim MAX77686"
>> -	depends on MFD_MAX77686
>
> It still depends on some parent. Instantiating this without main MFD
> probably would cause errors around max77686_init_rtc_regmap().
>

Indeed, if the RTC device can be registered by different MFD drivers,
then the driver should depend on X || Y

>> +	tristate "Maxim MAX77686/MAX77802"
>>   	help
>>   	  If you say yes here you will get support for the
>> -	  RTC of Maxim MAX77686 PMIC.
>> +	  RTC of Maxim MAX77686/MAX77802 PMIC.
>>
>>   	  This driver can also be built as a module. If so, the module
>> -	  will be called rtc-max77686.
>> +	  will be called rtc-max77686/MAX77802.
>
> The name of module does not change.
>
>>
>>   config RTC_DRV_RK808
>>   	tristate "Rockchip RK808 RTC"
>> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
>> index ab1f2cd..21a88e2 100644
>> --- a/drivers/rtc/rtc-max77686.c
>> +++ b/drivers/rtc/rtc-max77686.c
>> @@ -12,16 +12,100 @@
>>    *
>>    */
>>
>> +#include <linux/i2c.h>
>>   #include <linux/slab.h>
>>   #include <linux/rtc.h>
>>   #include <linux/delay.h>
>>   #include <linux/mutex.h>
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>> -#include <linux/mfd/max77686-private.h>
>
> I think the include should remain. No need of duplicating here the
> register addresses. Although we wnat the driver to be independent from
> parent it is still coming from one parent or another.
>
>>   #include <linux/irqdomain.h>
>>   #include <linux/regmap.h>
>>
>> +#define MAX77686_REG_STATUS2		0x07
>> +
>> +enum max77686_rtc_reg {
>> +	MAX77686_RTC_INT		= 0x00,
>> +	MAX77686_RTC_INTM		= 0x01,
>> +	MAX77686_RTC_CONTROLM		= 0x02,
>> +	MAX77686_RTC_CONTROL		= 0x03,
>> +	MAX77686_RTC_UPDATE0		= 0x04,
>> +	/* Reserved: 0x5 */
>> +	MAX77686_WTSR_SMPL_CNTL		= 0x06,
>> +	MAX77686_RTC_SEC		= 0x07,
>> +	MAX77686_RTC_MIN		= 0x08,
>> +	MAX77686_RTC_HOUR		= 0x09,
>> +	MAX77686_RTC_WEEKDAY		= 0x0A,
>> +	MAX77686_RTC_MONTH		= 0x0B,
>> +	MAX77686_RTC_YEAR		= 0x0C,
>> +	MAX77686_RTC_DATE		= 0x0D,
>> +	MAX77686_ALARM1_SEC		= 0x0E,
>> +	MAX77686_ALARM1_MIN		= 0x0F,
>> +	MAX77686_ALARM1_HOUR		= 0x10,
>> +	MAX77686_ALARM1_WEEKDAY		= 0x11,
>> +	MAX77686_ALARM1_MONTH		= 0x12,
>> +	MAX77686_ALARM1_YEAR		= 0x13,
>> +	MAX77686_ALARM1_DATE		= 0x14,
>> +	MAX77686_ALARM2_SEC		= 0x15,
>> +	MAX77686_ALARM2_MIN		= 0x16,
>> +	MAX77686_ALARM2_HOUR		= 0x17,
>> +	MAX77686_ALARM2_WEEKDAY		= 0x18,
>> +	MAX77686_ALARM2_MONTH		= 0x19,
>> +	MAX77686_ALARM2_YEAR		= 0x1A,
>> +	MAX77686_ALARM2_DATE		= 0x1B,
>> +};
>> +
>> +enum max77802_rtc_reg {
>> +	MAX77802_RTC_INT		= 0xC0,
>> +	MAX77802_RTC_INTM		= 0xC1,
>> +	MAX77802_RTC_CONTROLM		= 0xC2,
>> +	MAX77802_RTC_CONTROL		= 0xC3,
>> +	MAX77802_RTC_UPDATE0		= 0xC4,
>> +	MAX77802_RTC_UPDATE1		= 0xC5,
>> +	MAX77802_WTSR_SMPL_CNTL		= 0xC6,
>> +	MAX77802_RTC_SEC		= 0xC7,
>> +	MAX77802_RTC_MIN		= 0xC8,
>> +	MAX77802_RTC_HOUR		= 0xC9,
>> +	MAX77802_RTC_WEEKDAY		= 0xCA,
>> +	MAX77802_RTC_MONTH		= 0xCB,
>> +	MAX77802_RTC_YEAR		= 0xCC,
>> +	MAX77802_RTC_DATE		= 0xCD,
>> +	MAX77802_RTC_AE1		= 0xCE,
>> +	MAX77802_ALARM1_SEC		= 0xCF,
>> +	MAX77802_ALARM1_MIN		= 0xD0,
>> +	MAX77802_ALARM1_HOUR		= 0xD1,
>> +	MAX77802_ALARM1_WEEKDAY		= 0xD2,
>> +	MAX77802_ALARM1_MONTH		= 0xD3,
>> +	MAX77802_ALARM1_YEAR		= 0xD4,
>> +	MAX77802_ALARM1_DATE		= 0xD5,
>> +	MAX77802_RTC_AE2		= 0xD6,
>> +	MAX77802_ALARM2_SEC		= 0xD7,
>> +	MAX77802_ALARM2_MIN		= 0xD8,
>> +	MAX77802_ALARM2_HOUR		= 0xD9,
>> +	MAX77802_ALARM2_WEEKDAY		= 0xDA,
>> +	MAX77802_ALARM2_MONTH		= 0xDB,
>> +	MAX77802_ALARM2_YEAR		= 0xDC,
>> +	MAX77802_ALARM2_DATE		= 0xDD,
>> +
>> +	MAX77802_RTC_END		= 0xDF,
>> +};
>
> All of these should come from existing max77686/max77802/max77620 headers.
>
>> +
>> +enum max77686_rtc_irq {
>> +	MAX77686_RTCIRQ_RTC60S = 0,
>> +	MAX77686_RTCIRQ_RTCA1,
>> +	MAX77686_RTCIRQ_RTCA2,
>> +	MAX77686_RTCIRQ_SMPL,
>> +	MAX77686_RTCIRQ_RTC1S,
>> +	MAX77686_RTCIRQ_WTSR,
>> +};
>
> ditto
>
>> +
>> +#define MAX77686_RTCINT_RTC60S_MSK	BIT(0)
>> +#define MAX77686_RTCINT_RTCA1_MSK	BIT(1)
>> +#define MAX77686_RTCINT_RTCA2_MSK	BIT(2)
>> +#define MAX77686_RTCINT_SMPL_MSK	BIT(3)
>> +#define MAX77686_RTCINT_RTC1S_MSK	BIT(4)
>> +#define MAX77686_RTCINT_WTSR_MSK	BIT(5)
>
> ditto
>
>> +
>>   /* RTC Control Register */
>>   #define BCD_EN_SHIFT			0
>>   #define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
>> @@ -68,8 +152,10 @@ struct max77686_rtc_driver_data {
>>   	const unsigned int	*map;
>>   	/* Has a separate alarm enable register? */
>>   	bool			alarm_enable_reg;
>> -	/* Has a separate I2C regmap for the RTC? */
>> -	bool			separate_i2c_addr;
>> +	/* I2C address for RTC block */
>> +	u8			separate_i2c_addr;

The separate_i2c_addr field name made sense for a bool since it was
answering a question but now that is a u8, I believe that should be
called rtc_i2c_addr or something like that instead.

>> +	/* RTC IRQ CHIP for regmap */
>> +	const struct regmap_irq_chip *rtc_irq_chip;
>>   };
>>
>>   struct max77686_rtc_info {
>> @@ -82,7 +168,9 @@ struct max77686_rtc_info {
>>   	struct regmap		*rtc_regmap;
>>
>>   	const struct max77686_rtc_driver_data *drv_data;
>> +	struct regmap_irq_chip_data *rtc_irq_data;
>>
>> +	int rtc_irq;
>>   	int virq;
>>   	int rtc_24hr_mode;
>>   };
>> @@ -153,12 +241,32 @@ static const unsigned int max77686_map[REG_RTC_END] = {
>>   	[REG_RTC_AE1]	     = REG_RTC_NONE,
>>   };
>>
>> +static const struct regmap_irq max77686_rtc_irqs[] = {
>> +	/* RTC interrupts */
>> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC60S_MSK, },
>> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA1_MSK, },
>> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTCA2_MSK, },
>> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_SMPL_MSK, },
>> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_RTC1S_MSK, },
>> +	{ .reg_offset = 0, .mask = MAX77686_RTCINT_WTSR_MSK, },
>> +};
>> +
>> +static const struct regmap_irq_chip max77686_rtc_irq_chip = {
>> +	.name		= "max77686-rtc",
>> +	.status_base	= MAX77686_RTC_INT,
>> +	.mask_base	= MAX77686_RTC_INTM,
>> +	.num_regs	= 1,
>> +	.irqs		= max77686_rtc_irqs,
>> +	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
>> +};
>> +
>>   static const struct max77686_rtc_driver_data max77686_drv_data = {
>>   	.delay = 16000,
>>   	.mask  = 0x7f,
>>   	.map   = max77686_map,
>>   	.alarm_enable_reg  = false,
>> -	.separate_i2c_addr = true,
>> +	.separate_i2c_addr = 0xC >> 1,
>
> When the header will remain:
> I2C_ADDR_RTC
> (maybe it needs some prefix?)
>
>> +	.rtc_irq_chip = &max77686_rtc_irq_chip,
>>   };
>>
>>   static const unsigned int max77802_map[REG_RTC_END] = {
>> @@ -190,12 +298,22 @@ static const unsigned int max77802_map[REG_RTC_END] = {
>>   	[REG_RTC_AE1]	     = MAX77802_RTC_AE1,
>>   };
>>
>> +static const struct regmap_irq_chip max77802_rtc_irq_chip = {
>> +	.name		= "max77802-rtc",
>> +	.status_base	= MAX77802_RTC_INT,
>> +	.mask_base	= MAX77802_RTC_INTM,
>> +	.num_regs	= 1,
>> +	.irqs		= max77686_rtc_irqs, /* same masks as 77686 */
>> +	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
>> +};
>> +
>>   static const struct max77686_rtc_driver_data max77802_drv_data = {
>>   	.delay = 200,
>>   	.mask  = 0xff,
>>   	.map   = max77802_map,
>>   	.alarm_enable_reg  = true,
>> -	.separate_i2c_addr = false,
>> +	.separate_i2c_addr = 0,

AFAIK 0 is a reserved address for I2C (general call address) so maybe
it would be better to use an invalid address to specify that the RTC
doesn't have a separate I2C address for the registers and check this?

>> +	.rtc_irq_chip = &max77802_rtc_irq_chip,
>>   };
>>
>>   static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
>> @@ -599,9 +717,65 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
>>   	return ret;
>>   }
>>
>> +static const struct regmap_config max77686_rtc_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +};
>> +
>> +static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
>> +{
>> +	struct device *parent = info->dev->parent;
>> +	struct i2c_client *parent_i2c = to_i2c_client(parent);
>> +	int ret;
>> +
>> +	info->rtc_irq =  parent_i2c->irq;
>
> Double space.
>
>> +
>> +	info->regmap = dev_get_regmap(parent, NULL);
>> +	if (!info->regmap) {
>> +		dev_err(info->dev, "Failed to get rtc regmap\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!info->drv_data->separate_i2c_addr) {
>> +		info->rtc = parent_i2c;
>
> I think it should remain NULL in such case. You won't be using it
> anyway... but in case if you use it then it will be an error anyway
> (like in error path below).
>
>> +		goto add_rtc_irq;
>> +	}
>> +
>> +	info->rtc = i2c_new_dummy(parent_i2c->adapter,
>> +				info->drv_data->separate_i2c_addr);
>
> Please align the argument on new line.
>
>> +	if (!info->rtc) {
>> +		dev_err(info->dev, "Failed to allocate I2C device for RTC\n");
>> +		return -ENODEV;
>> +	}
>> +	i2c_set_clientdata(info->rtc, info);
>
> No need for this. There is no getter. Removal of this could be a
> separate patch (before this one).
>
>> +
>> +	info->rtc_regmap = devm_regmap_init_i2c(info->rtc,
>> +						&max77686_rtc_regmap_config);
>> +	if (IS_ERR(info->rtc_regmap)) {
>> +		ret = PTR_ERR(info->rtc_regmap);
>> +		dev_err(info->dev, "failed to allocate RTC regmap: %d\n", ret);
>
> While moving the messages can you unify the capital letter style (e.g.
> all of error msg starting with capital).
>
>> +		goto err_unregister_i2c;
>> +	}
>> +
>> +add_rtc_irq:
>> +	ret = regmap_add_irq_chip(info->rtc_regmap, info->rtc_irq,
>> +				  IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>> +				  IRQF_SHARED, 0, info->drv_data->rtc_irq_chip,
>> +				  &info->rtc_irq_data);
>> +	if (ret < 0) {
>> +		dev_err(info->dev, "failed to add RTC irq chip: %d\n", ret);
>> +		goto err_unregister_i2c;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_unregister_i2c:
>> +	i2c_unregister_device(info->rtc);
>
> That would unregister the parent. You are missing "if (separate)".
>
>> +	return ret;
>> +}
>> +
>>   static int max77686_rtc_probe(struct platform_device *pdev)
>>   {
>> -	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
>>   	struct max77686_rtc_info *info;
>>   	const struct platform_device_id *id = platform_get_device_id(pdev);
>>   	int ret;
>> @@ -613,18 +787,16 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>
>>   	mutex_init(&info->lock);
>>   	info->dev = &pdev->dev;
>> -	info->rtc = max77686->rtc;
>>   	info->drv_data = (const struct max77686_rtc_driver_data *)
>>   		id->driver_data;
>>
>> -	info->regmap = max77686->regmap;
>> -	info->rtc_regmap = (info->drv_data->separate_i2c_addr) ?
>> -			    max77686->rtc_regmap : info->regmap;
>> +	ret = max77686_init_rtc_regmap(info);
>> +	if (ret < 0)
>> +		return ret;
>>
>>   	platform_set_drvdata(pdev, info);
>>
>>   	ret = max77686_rtc_init_reg(info);
>> -
>>   	if (ret < 0) {
>>   		dev_err(&pdev->dev, "Failed to initialize RTC reg:%d\n", ret);
>>   		goto err_rtc;
>> @@ -643,13 +815,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>   		goto err_rtc;
>>   	}
>>
>> -	if (!max77686->rtc_irq_data) {
>> -		ret = -EINVAL;
>> -		dev_err(&pdev->dev, "No RTC regmap IRQ chip\n");
>> -		goto err_rtc;
>> -	}
>> -
>> -	info->virq = regmap_irq_get_virq(max77686->rtc_irq_data,
>> +	info->virq = regmap_irq_get_virq(info->rtc_irq_data,
>>   					 MAX77686_RTCIRQ_RTCA1);
>>   	if (!info->virq) {
>>   		ret = -ENXIO;
>> @@ -659,14 +825,34 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>   	ret = devm_request_threaded_irq(&pdev->dev, info->virq, NULL,
>>   					max77686_rtc_alarm_irq, 0,
>>   					"rtc-alarm1", info);
>> -	if (ret < 0)
>> +	if (ret < 0) {
>>   		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
>>   			info->virq, ret);
>> +		goto err_rtc;
>> +	}
>> +
>> +	return 0;
>>
>>   err_rtc:
>> +	if (info->drv_data->separate_i2c_addr)
>> +		i2c_unregister_device(info->rtc);
>> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
>
> Cleanup in reverse-order of allocation. So first del irq_chip then
> unregister i2c.
>
> Best regards,
> Krzysztof
>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America