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

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH V2 0/5]  rtc: max77686: make max77686 rtc driver as IP driver
@ 2016-02-03  9:30 ` Laxman Dewangan
  0 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03  9:30 UTC (permalink / raw)
  To: lee.jones, alexandre.belloni, k.kozlowski, javier
  Cc: cw00.choi, linux-kernel, rtc-linux, 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 Kozlowski <k.kozlowski@samsung.com>
CC: Javier Martinez Canillas <javier@osg.samsung.com>

Changes from V1:
- Added reviewed/tested by tag which we got from V1.
- Remove changes from Kconfig.
- Maintain all register definition in max77686 private header and remove
  the movement to rtc driver.
- Taken care of all comments on V1 from Krzysztof and Javier.

Laxman Dewangan (5):
  rtc: max77686: fix checkpatch error
  rtc: max77686: use rtc regmap to access RTC registers
  rtc: max77686: avoid reference of parent device info multiple palces
  mfd: max77686: do not set i2c client data for rtc i2c client
  rtc: max77686: move initialisation of rtc regmap, irq chip locally

 drivers/mfd/max77686.c               |  86 +-------------
 drivers/rtc/rtc-max77686.c           | 212 ++++++++++++++++++++++++++---------
 include/linux/mfd/max77686-private.h |   3 -
 3 files changed, 160 insertions(+), 141 deletions(-)

-- 
2.1.4

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [rtc-linux] [PATCH V2 0/5]  rtc: max77686: make max77686 rtc driver as IP driver
@ 2016-02-03  9:30 ` Laxman Dewangan
  0 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03  9:30 UTC (permalink / raw)
  To: lee.jones, alexandre.belloni, k.kozlowski, javier
  Cc: cw00.choi, linux-kernel, rtc-linux, 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 Kozlowski <k.kozlowski@samsung.com>
CC: Javier Martinez Canillas <javier@osg.samsung.com>

Changes from V1:
- Added reviewed/tested by tag which we got from V1.
- Remove changes from Kconfig.
- Maintain all register definition in max77686 private header and remove
  the movement to rtc driver.
- Taken care of all comments on V1 from Krzysztof and Javier.

Laxman Dewangan (5):
  rtc: max77686: fix checkpatch error
  rtc: max77686: use rtc regmap to access RTC registers
  rtc: max77686: avoid reference of parent device info multiple palces
  mfd: max77686: do not set i2c client data for rtc i2c client
  rtc: max77686: move initialisation of rtc regmap, irq chip locally

 drivers/mfd/max77686.c               |  86 +-------------
 drivers/rtc/rtc-max77686.c           | 212 ++++++++++++++++++++++++++---------
 include/linux/mfd/max77686-private.h |   3 -
 3 files changed, 160 insertions(+), 141 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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH V2 1/5] rtc: max77686: fix checkpatch error
  2016-02-03  9:30 ` [rtc-linux] " Laxman Dewangan
@ 2016-02-03  9:30   ` Laxman Dewangan
  -1 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03  9:30 UTC (permalink / raw)
  To: lee.jones, alexandre.belloni, k.kozlowski, javier
  Cc: cw00.choi, linux-kernel, rtc-linux, 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>
CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
CC: Javier Martinez Canillas <javier@osg.samsung.com>
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

---
Changes from V1:
None, added reviewed/tested by.

 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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [rtc-linux] [PATCH V2 1/5] rtc: max77686: fix checkpatch error
@ 2016-02-03  9:30   ` Laxman Dewangan
  0 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03  9:30 UTC (permalink / raw)
  To: lee.jones, alexandre.belloni, k.kozlowski, javier
  Cc: cw00.choi, linux-kernel, rtc-linux, 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>
CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
CC: Javier Martinez Canillas <javier@osg.samsung.com>
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

---
Changes from V1:
None, added reviewed/tested by.

 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.

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH V2 2/5] rtc: max77686: use rtc regmap to access RTC registers
  2016-02-03  9:30 ` [rtc-linux] " Laxman Dewangan
@ 2016-02-03  9:30   ` Laxman Dewangan
  -1 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03  9:30 UTC (permalink / raw)
  To: lee.jones, alexandre.belloni, k.kozlowski, javier
  Cc: cw00.choi, linux-kernel, rtc-linux, Laxman Dewangan

rtc_regmap should be used to access all RTC registers 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>
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

---
Changes from V1:
None, added reviewed/tested by.

 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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [rtc-linux] [PATCH V2 2/5] rtc: max77686: use rtc regmap to access RTC registers
@ 2016-02-03  9:30   ` Laxman Dewangan
  0 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03  9:30 UTC (permalink / raw)
  To: lee.jones, alexandre.belloni, k.kozlowski, javier
  Cc: cw00.choi, linux-kernel, rtc-linux, Laxman Dewangan

rtc_regmap should be used to access all RTC registers 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>
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

---
Changes from V1:
None, added reviewed/tested by.

 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.

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH V2 3/5] rtc: max77686: avoid reference of parent device info multiple palces
  2016-02-03  9:30 ` [rtc-linux] " Laxman Dewangan
@ 2016-02-03  9:30   ` Laxman Dewangan
  -1 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03  9:30 UTC (permalink / raw)
  To: lee.jones, alexandre.belloni, k.kozlowski, javier
  Cc: cw00.choi, linux-kernel, rtc-linux, 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>
CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
CC: Javier Martinez Canillas <javier@osg.samsung.com>
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

---
Changes from V1:
None, added reviewed/tested by.

 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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [rtc-linux] [PATCH V2 3/5] rtc: max77686: avoid reference of parent device info multiple palces
@ 2016-02-03  9:30   ` Laxman Dewangan
  0 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03  9:30 UTC (permalink / raw)
  To: lee.jones, alexandre.belloni, k.kozlowski, javier
  Cc: cw00.choi, linux-kernel, rtc-linux, 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>
CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
CC: Javier Martinez Canillas <javier@osg.samsung.com>
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

---
Changes from V1:
None, added reviewed/tested by.

 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.

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH V2 3/5] rtc: max77686: avoid reference of parent device info multiple palces
  2016-02-03  9:30   ` [rtc-linux] " Laxman Dewangan
@ 2016-02-03 12:45     ` Javier Martinez Canillas
  -1 siblings, 0 replies; 26+ messages in thread
From: Javier Martinez Canillas @ 2016-02-03 12:45 UTC (permalink / raw)
  To: Laxman Dewangan, lee.jones, alexandre.belloni, k.kozlowski
  Cc: cw00.choi, linux-kernel, rtc-linux

Hello Laxman,

I noticed a typo in the subject line that missed before:

s/palces/places

On 02/03/2016 06:30 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>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
> Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
>

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [rtc-linux] Re: [PATCH V2 3/5] rtc: max77686: avoid reference of parent device info multiple palces
@ 2016-02-03 12:45     ` Javier Martinez Canillas
  0 siblings, 0 replies; 26+ messages in thread
From: Javier Martinez Canillas @ 2016-02-03 12:45 UTC (permalink / raw)
  To: Laxman Dewangan, lee.jones, alexandre.belloni, k.kozlowski
  Cc: cw00.choi, linux-kernel, rtc-linux

Hello Laxman,

I noticed a typo in the subject line that missed before:

s/palces/places

On 02/03/2016 06:30 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>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
> Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH V2 4/5] mfd: max77686: do not set i2c client data for rtc i2c client
  2016-02-03  9:30 ` [rtc-linux] " Laxman Dewangan
@ 2016-02-03  9:30   ` Laxman Dewangan
  -1 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03  9:30 UTC (permalink / raw)
  To: lee.jones, alexandre.belloni, k.kozlowski, javier
  Cc: cw00.choi, linux-kernel, rtc-linux, Laxman Dewangan

There is different RTC I2C address for RTC block in MAX77686.
Driver is creating dummy i2c client for this address to access
the register of this IP block.

As there is no need to get any data from this rtc i2c client,
it is not required to set client data for this i2c client.

Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
CC: Javier Martinez Canillas <javier@osg.samsung.com>

---
This is new in this series based on review comment from V1.

 drivers/mfd/max77686.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index d959ebb..bda6fd7 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -277,7 +277,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 				"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,
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [rtc-linux] [PATCH V2 4/5] mfd: max77686: do not set i2c client data for rtc i2c client
@ 2016-02-03  9:30   ` Laxman Dewangan
  0 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03  9:30 UTC (permalink / raw)
  To: lee.jones, alexandre.belloni, k.kozlowski, javier
  Cc: cw00.choi, linux-kernel, rtc-linux, Laxman Dewangan

There is different RTC I2C address for RTC block in MAX77686.
Driver is creating dummy i2c client for this address to access
the register of this IP block.

As there is no need to get any data from this rtc i2c client,
it is not required to set client data for this i2c client.

Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
CC: Javier Martinez Canillas <javier@osg.samsung.com>

---
This is new in this series based on review comment from V1.

 drivers/mfd/max77686.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index d959ebb..bda6fd7 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -277,7 +277,6 @@ static int max77686_i2c_probe(struct i2c_client *i2c,
 				"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,
-- 
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.

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH V2 4/5] mfd: max77686: do not set i2c client data for rtc i2c client
  2016-02-03  9:30   ` [rtc-linux] " Laxman Dewangan
@ 2016-02-03 10:19     ` Krzysztof Kozlowski
  -1 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2016-02-03 10:19 UTC (permalink / raw)
  To: Laxman Dewangan, lee.jones, alexandre.belloni, javier
  Cc: cw00.choi, linux-kernel, rtc-linux

On 03.02.2016 18:30, Laxman Dewangan wrote:
> There is different RTC I2C address for RTC block in MAX77686.
> Driver is creating dummy i2c client for this address to access
> the register of this IP block.
> 
> As there is no need to get any data from this rtc i2c client,
> it is not required to set client data for this i2c client.
> 
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> This is new in this series based on review comment from V1.
> 

You could trim a little bit the description - "stored pointer is not
retrieved anywhere, there is no call to i2c_get_clientdata()" might be
sufficient but anyway:

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

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [rtc-linux] Re: [PATCH V2 4/5] mfd: max77686: do not set i2c client data for rtc i2c client
@ 2016-02-03 10:19     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2016-02-03 10:19 UTC (permalink / raw)
  To: Laxman Dewangan, lee.jones, alexandre.belloni, javier
  Cc: cw00.choi, linux-kernel, rtc-linux

On 03.02.2016 18:30, Laxman Dewangan wrote:
> There is different RTC I2C address for RTC block in MAX77686.
> Driver is creating dummy i2c client for this address to access
> the register of this IP block.
> 
> As there is no need to get any data from this rtc i2c client,
> it is not required to set client data for this i2c client.
> 
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> This is new in this series based on review comment from V1.
> 

You could trim a little bit the description - "stored pointer is not
retrieved anywhere, there is no call to i2c_get_clientdata()" might be
sufficient but anyway:

Reviewed-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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH V2 4/5] mfd: max77686: do not set i2c client data for rtc i2c client
  2016-02-03  9:30   ` [rtc-linux] " Laxman Dewangan
@ 2016-02-03 12:47     ` Javier Martinez Canillas
  -1 siblings, 0 replies; 26+ messages in thread
From: Javier Martinez Canillas @ 2016-02-03 12:47 UTC (permalink / raw)
  To: Laxman Dewangan, lee.jones, alexandre.belloni, k.kozlowski
  Cc: cw00.choi, linux-kernel, rtc-linux

Hello Laxman,

On 02/03/2016 06:30 AM, Laxman Dewangan wrote:
> There is different RTC I2C address for RTC block in MAX77686.
> Driver is creating dummy i2c client for this address to access
> the register of this IP block.
>
> As there is no need to get any data from this rtc i2c client,
> it is not required to set client data for this i2c client.
>
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
>
> ---

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

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [rtc-linux] Re: [PATCH V2 4/5] mfd: max77686: do not set i2c client data for rtc i2c client
@ 2016-02-03 12:47     ` Javier Martinez Canillas
  0 siblings, 0 replies; 26+ messages in thread
From: Javier Martinez Canillas @ 2016-02-03 12:47 UTC (permalink / raw)
  To: Laxman Dewangan, lee.jones, alexandre.belloni, k.kozlowski
  Cc: cw00.choi, linux-kernel, rtc-linux

Hello Laxman,

On 02/03/2016 06:30 AM, Laxman Dewangan wrote:
> There is different RTC I2C address for RTC block in MAX77686.
> Driver is creating dummy i2c client for this address to access
> the register of this IP block.
>
> As there is no need to get any data from this rtc i2c client,
> it is not required to set client data for this i2c client.
>
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally
  2016-02-03  9:30 ` [rtc-linux] " Laxman Dewangan
@ 2016-02-03  9:30   ` Laxman Dewangan
  -1 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03  9:30 UTC (permalink / raw)
  To: lee.jones, alexandre.belloni, k.kozlowski, javier
  Cc: cw00.choi, linux-kernel, rtc-linux, Laxman Dewangan

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

Having this change will allow to reuse this driver for different
PMIC/devices from Maxim Semiconductor if they kept same RTC IP on
different PMIC. Some of examples as PMIC MAX77620, MAX20024 where
same RTC IP used and hence driver for these chips will use this
driver only for RTC support.

Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
CC: Javier Martinez Canillas <javier@osg.samsung.com>

---
Changes from V1:
- Remove changes from Kconfig.
- Maintain all register definition in max77686 private header and remove
  the movement to rtc driver.
- Taken care of all comments on V1 from Krzysztof and Javier.

 drivers/mfd/max77686.c               |  85 +--------------------
 drivers/rtc/rtc-max77686.c           | 142 ++++++++++++++++++++++++++++++-----
 include/linux/mfd/max77686-private.h |   3 -
 3 files changed, 127 insertions(+), 103 deletions(-)

diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index bda6fd7..98ecd13 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -35,8 +35,6 @@
 #include <linux/err.h>
 #include <linux/of.h>
 
-#define I2C_ADDR_RTC	(0x0C >> 1)
-
 static const struct mfd_cell max77686_devs[] = {
 	{ .name = "max77686-pmic", },
 	{ .name = "max77686-rtc", },
@@ -116,11 +114,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 +149,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 +158,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 +179,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 +205,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,59 +229,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;
-		}
-
-		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) {
+	if (ret < 0) {
 		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 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;
 }
@@ -333,12 +258,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/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index ab1f2cd..10984c4 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -12,6 +12,7 @@
  *
  */
 
+#include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/rtc.h>
 #include <linux/delay.h>
@@ -22,6 +23,9 @@
 #include <linux/irqdomain.h>
 #include <linux/regmap.h>
 
+#define MAX77686_I2C_ADDR_RTC		(0x0C >> 1)
+#define INVALID_I2C_ADDR		(-1)
+
 /* RTC Control Register */
 #define BCD_EN_SHIFT			0
 #define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
@@ -68,8 +72,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 */
+	int			rtc_i2c_addr;
+	/* RTC IRQ CHIP for regmap */
+	const struct regmap_irq_chip *rtc_irq_chip;
 };
 
 struct max77686_rtc_info {
@@ -82,7 +88,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 +161,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,
+	.rtc_i2c_addr = MAX77686_I2C_ADDR_RTC,
+	.rtc_irq_chip = &max77686_rtc_irq_chip,
 };
 
 static const unsigned int max77802_map[REG_RTC_END] = {
@@ -190,12 +218,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,
+	.rtc_i2c_addr = INVALID_I2C_ADDR,
+	.rtc_irq_chip = &max77802_rtc_irq_chip,
 };
 
 static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
@@ -599,9 +637,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->rtc_i2c_addr == INVALID_I2C_ADDR) {
+		info->rtc_regmap = info->regmap;
+		goto add_rtc_irq;
+	}
+
+	info->rtc = i2c_new_dummy(parent_i2c->adapter,
+				  info->drv_data->rtc_i2c_addr);
+	if (!info->rtc) {
+		dev_err(info->dev, "Failed to allocate I2C device for RTC\n");
+		return -ENODEV;
+	}
+
+	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:
+	if (info->rtc)
+		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 +707,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 +735,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 +745,33 @@ 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->rtc)
+		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);
+
+	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
+	if (info->rtc)
+		i2c_unregister_device(info->rtc);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int max77686_rtc_suspend(struct device *dev)
 {
@@ -707,6 +812,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..643dae7 100644
--- a/include/linux/mfd/max77686-private.h
+++ b/include/linux/mfd/max77686-private.h
@@ -437,14 +437,11 @@ enum max77686_irq {
 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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [rtc-linux] [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally
@ 2016-02-03  9:30   ` Laxman Dewangan
  0 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03  9:30 UTC (permalink / raw)
  To: lee.jones, alexandre.belloni, k.kozlowski, javier
  Cc: cw00.choi, linux-kernel, rtc-linux, Laxman Dewangan

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

Having this change will allow to reuse this driver for different
PMIC/devices from Maxim Semiconductor if they kept same RTC IP on
different PMIC. Some of examples as PMIC MAX77620, MAX20024 where
same RTC IP used and hence driver for these chips will use this
driver only for RTC support.

Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
CC: Javier Martinez Canillas <javier@osg.samsung.com>

---
Changes from V1:
- Remove changes from Kconfig.
- Maintain all register definition in max77686 private header and remove
  the movement to rtc driver.
- Taken care of all comments on V1 from Krzysztof and Javier.

 drivers/mfd/max77686.c               |  85 +--------------------
 drivers/rtc/rtc-max77686.c           | 142 ++++++++++++++++++++++++++++++-----
 include/linux/mfd/max77686-private.h |   3 -
 3 files changed, 127 insertions(+), 103 deletions(-)

diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index bda6fd7..98ecd13 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -35,8 +35,6 @@
 #include <linux/err.h>
 #include <linux/of.h>
 
-#define I2C_ADDR_RTC	(0x0C >> 1)
-
 static const struct mfd_cell max77686_devs[] = {
 	{ .name = "max77686-pmic", },
 	{ .name = "max77686-rtc", },
@@ -116,11 +114,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 +149,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 +158,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 +179,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 +205,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,59 +229,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;
-		}
-
-		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) {
+	if (ret < 0) {
 		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 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;
 }
@@ -333,12 +258,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/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index ab1f2cd..10984c4 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -12,6 +12,7 @@
  *
  */
 
+#include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/rtc.h>
 #include <linux/delay.h>
@@ -22,6 +23,9 @@
 #include <linux/irqdomain.h>
 #include <linux/regmap.h>
 
+#define MAX77686_I2C_ADDR_RTC		(0x0C >> 1)
+#define INVALID_I2C_ADDR		(-1)
+
 /* RTC Control Register */
 #define BCD_EN_SHIFT			0
 #define BCD_EN_MASK			BIT(BCD_EN_SHIFT)
@@ -68,8 +72,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 */
+	int			rtc_i2c_addr;
+	/* RTC IRQ CHIP for regmap */
+	const struct regmap_irq_chip *rtc_irq_chip;
 };
 
 struct max77686_rtc_info {
@@ -82,7 +88,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 +161,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,
+	.rtc_i2c_addr = MAX77686_I2C_ADDR_RTC,
+	.rtc_irq_chip = &max77686_rtc_irq_chip,
 };
 
 static const unsigned int max77802_map[REG_RTC_END] = {
@@ -190,12 +218,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,
+	.rtc_i2c_addr = INVALID_I2C_ADDR,
+	.rtc_irq_chip = &max77802_rtc_irq_chip,
 };
 
 static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
@@ -599,9 +637,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->rtc_i2c_addr == INVALID_I2C_ADDR) {
+		info->rtc_regmap = info->regmap;
+		goto add_rtc_irq;
+	}
+
+	info->rtc = i2c_new_dummy(parent_i2c->adapter,
+				  info->drv_data->rtc_i2c_addr);
+	if (!info->rtc) {
+		dev_err(info->dev, "Failed to allocate I2C device for RTC\n");
+		return -ENODEV;
+	}
+
+	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:
+	if (info->rtc)
+		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 +707,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 +735,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 +745,33 @@ 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->rtc)
+		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);
+
+	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
+	if (info->rtc)
+		i2c_unregister_device(info->rtc);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int max77686_rtc_suspend(struct device *dev)
 {
@@ -707,6 +812,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..643dae7 100644
--- a/include/linux/mfd/max77686-private.h
+++ b/include/linux/mfd/max77686-private.h
@@ -437,14 +437,11 @@ enum max77686_irq {
 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.

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally
  2016-02-03  9:30   ` [rtc-linux] " Laxman Dewangan
@ 2016-02-03 10:45     ` Krzysztof Kozlowski
  -1 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2016-02-03 10:45 UTC (permalink / raw)
  To: Laxman Dewangan, lee.jones, alexandre.belloni, javier
  Cc: cw00.choi, linux-kernel, rtc-linux

On 03.02.2016 18:30, Laxman Dewangan wrote:
> To make RTC block of MAX77686/MAX77802 as independent driver,
> move the registration of i2c device, regmap for register access
> and irq_chip for interrupt support inside the RTC driver.
> Removed the same initialisation from MFD driver.
> 
> Having this change will allow to reuse this driver for different
> PMIC/devices from Maxim Semiconductor if they kept same RTC IP on
> different PMIC. Some of examples as PMIC MAX77620, MAX20024 where
> same RTC IP used and hence driver for these chips will use this
> driver only for RTC support.
> 
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> Changes from V1:
> - Remove changes from Kconfig.
> - Maintain all register definition in max77686 private header and remove
>   the movement to rtc driver.
> - Taken care of all comments on V1 from Krzysztof and Javier.

Almost everything is good. You skipped one my comment, at the end:

(...)

> @@ -659,14 +745,33 @@ 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->rtc)
> +		i2c_unregister_device(info->rtc);
> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
> +

You should clean up in reverse order of allocation, so first
regmap_del_irq_chip then i2c_unregister_device. This
is a common pattern of cleaning up. Sometimes such order
is even necessary because of dependencies between
components... which is not a case here but still the
natural way is reversing the allocation code.

>  	return ret;
>  }
>  
> +static int max77686_rtc_remove(struct platform_device *pdev)
> +{
> +	struct max77686_rtc_info *info = platform_get_drvdata(pdev);
> +
> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
> +	if (info->rtc)
> +		i2c_unregister_device(info->rtc);

Here it is okay.

Anyway RTC works... but unbinding leads to oops:
echo max77686-rtc > /sys/bus/platform/drivers/max77686-rtc/unbind
[   80.505411] Unable to handle kernel paging request at virtual address ffffffff
[   80.511257] pgd = ec3a0000
[   80.513870] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
[   80.520130] Internal error: Oops: 37 [#1] PREEMPT SMP ARM
[   80.525498] Modules linked in:
[   80.528551] CPU: 2 PID: 1325 Comm: sleep Tainted: G        W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
[   80.538867] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   80.544947] task: eeb53440 ti: eeb12000 task.ti: eeb12000
[   80.550349] PC is at kmem_cache_alloc+0x7c/0x154
[   80.554933] LR is at kmem_cache_alloc+0x3c/0x154
[   80.559535] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a00f0013
[   80.559535] sp : eeb13e18  ip : 00000001  fp : bee6c4f4
[   80.570986] r10: 024080c0  r9 : eeb13ed0  r8 : 00033d72
[   80.576195] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
[   80.582704] r3 : 00000000  r2 : 00033d72  r1 : c070e2ac  r0 : 2ef40000
[   80.589217] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   80.596332] Control: 10c5387d  Table: 6c3a004a  DAC: 00000051
[   80.602061] Process sleep (pid: 1325, stack limit = 0xeeb12210)
[   80.607963] Stack: (0xeeb13e18 to 0xeeb14000)
[   80.612306] 3e00:                                                       c0858280 ee4ada00
[   80.620475] 3e20: c0863e80 eeb13f74 c000fa84 eeb13ed0 00000000 c00deda8 b6f4f000 00000011
[   80.628632] 3e40: 00000003 eeb13f74 00000001 c00e8ebc 386de9c0 00000000 00000001 00000001
[   80.636792] 3e60: eeb53440 00000041 eee89514 0000014f 00000054 0000053c eeb6c190 ee4f653c
[   80.644934] 3e80: 00013000 ee4f6000 00000001 00000000 00000054 024200ca 00000010 b6f4e000
[   80.653093] 3ea0: ee58e7a8 b6f3e000 00000011 00000003 eeb13f74 00000001 00000005 c000fa84
[   80.661253] 3ec0: eeb12000 00000000 bee6c4f4 c00eaa6c 00000000 c0018570 eea49700 ee58e160
[   80.669412] 3ee0: 00000000 00000000 c001921c 00000017 c0018388 b6f4f0dd c085cf34 eeb13fb0
[   80.677571] 3f00: b6f96b40 7f62b751 00000000 eeb13f10 eeb13f24 ef001b80 00000001 00000001
[   80.685730] 3f20: 00000003 ee5aac00 ee5aac24 00000020 00080000 c05a0390 00000000 c00f72ec
[   80.693889] 3f40: eebac000 00000000 eebac000 ffffff9c ffffff9c c000fa84 00000003 eebac000
[   80.702048] 3f60: ffffff9c c00dc29c eeb53440 00000003 ee5aac00 00000000 eea40000 00000004
[   80.710208] 3f80: 00000100 00000001 eea49700 00000008 00000000 bee6c534 00000005 c000fa84
[   80.718367] 3fa0: 00000000 c000f8c0 00000008 00000000 b6f4f0ce 00080000 b6f96958 00000000
[   80.726526] 3fc0: 00000008 00000000 bee6c534 00000005 00000001 b6f4f0ce 00000000 bee6c4f4
[   80.734685] 3fe0: b6f95ce0 bee6c4ac b6f6ba14 b6f7f24c 600f0010 b6f4f0ce ffffffff ffffffff
[   80.742854] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
[   80.751008] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
[   80.758731] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
[   80.766195] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
[   80.773838] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
[   80.781559] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
[   80.787694] ---[ end trace 9502799e3ea05a7e ]---
[   80.792503] Unable to handle kernel paging request at virtual address ffffffff
[   80.799457] pgd = ec39c000
[   80.802140] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
[   80.808377] Internal error: Oops: 37 [#2] PREEMPT SMP ARM
[   80.813756] Modules linked in:
[   80.816798] CPU: 2 PID: 628 Comm: sh Tainted: G      D W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
[   80.826776] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   80.832853] task: ec3050c0 ti: ee66e000 task.ti: ee66e000
[   80.838240] PC is at kmem_cache_alloc+0x7c/0x154
[   80.842836] LR is at kmem_cache_alloc+0x3c/0x154
[   80.847437] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a00f0013
[   80.847437] sp : ee66fe18  ip : 00000001  fp : beaba124
[   80.858893] r10: 024080c0  r9 : ee66fed0  r8 : 00033d72
[   80.864100] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
[   80.870610] r3 : 00000000  r2 : 00033d72  r1 : c070e2ac  r0 : 2ef40000
[   80.877120] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   80.884237] Control: 10c5387d  Table: 6c39c04a  DAC: 00000051
[   80.889966] Process sh (pid: 628, stack limit = 0xee66e210)
[   80.895522] Stack: (0xee66fe18 to 0xee670000)
[   80.899862] fe00:                                                       c0858280 ec264f80
[   80.908023] fe20: c0863e80 ee66ff74 c000fa84 ee66fed0 00000000 c00deda8 00000040 ee58e9a0
[   80.916182] fe40: 00000003 ee66ff74 00000001 c00e8ebc ec39efa8 eeb6cee0 69f907df ef00cc10
[   80.924341] fe60: c0850200 00000041 00000001 00000195 00000055 00000654 ec39edb8 ee464380
[   80.932500] fe80: ffffff05 c023bd4c 00000000 00000000 00000000 ee464380 ee58e9a0 c00c0904
[   80.940660] fea0: ee464380 ee58e9a0 ee464380 00000003 ee66ff74 00000001 00000005 c000fa84
[   80.948819] fec0: ee66e000 00000000 beaba124 c00eaa6c 00000800 c0018570 ec19fa08 00000000
[   80.956978] fee0: 00000000 00000000 00000000 00000817 c0018388 b6f95000 c085cf34 ee66ffb0
[   80.965137] ff00: 00000000 b6f974c0 00000000 ee66ff10 ee66ff24 ef001b80 00000001 00000001
[   80.973296] ff20: 00000003 ee59ba00 ee405e00 00000100 00080000 c05a0390 00000000 c00f72ec
[   80.981456] ff40: eebaf000 00000000 eebaf000 ffffff9c ffffff9c c000fa84 00000003 eebaf000
[   80.989615] ff60: ffffff9c c00dc29c beaba52c c00b0e8c 00000022 00000000 000b0000 00000004
[   80.997774] ff80: 00000100 00000001 ffffffff 000d6430 00000008 00000008 00000005 c000fa84
[   81.005933] ffa0: 00000000 c000f8c0 000d6430 00000008 beaba148 00080000 000001b6 000001b6
[   81.014092] ffc0: 000d6430 00000008 00000008 00000005 00000000 b6f0405c beaba408 beaba124
[   81.022252] ffe0: 00080000 beaba0c4 b6e3529c b6e8a9bc 200f0010 beaba148 6fffd861 6fffdc61
[   81.030416] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
[   81.038572] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
[   81.046296] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
[   81.053761] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
[   81.061401] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
[   81.069124] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
[   81.075245] ---[ end trace 9502799e3ea05a7f ]---
[   88.035087] Unable to handle kernel paging request at virtual address ffffffff
[   88.040971] pgd = ee604000
[   88.043617] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
[   88.049822] Internal error: Oops: 37 [#3] PREEMPT SMP ARM
[   88.055186] Modules linked in:
[   88.058245] CPU: 2 PID: 396 Comm: deviced Tainted: G      D W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
[   88.068641] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   88.074723] task: ec089c80 ti: ec180000 task.ti: ec180000
[   88.080132] PC is at kmem_cache_alloc+0x7c/0x154
[   88.084712] LR is at kmem_cache_alloc+0x3c/0x154
[   88.089313] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a0000013
[   88.089313] sp : ec181e18  ip : 00000001  fp : be9821e4
[   88.100763] r10: 024080c0  r9 : ec181ed0  r8 : 00033d82
[   88.105971] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
[   88.112482] r3 : 00000000  r2 : 00033d82  r1 : c070e2ac  r0 : 2ef40000
[   88.118995] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   88.126095] Control: 10c5387d  Table: 6e60404a  DAC: 00000051
[   88.131824] Process deviced (pid: 396, stack limit = 0xec180210)
[   88.137812] Stack: (0xec181e18 to 0xec182000)
[   88.142153] 1e00:                                                       c0858280 ee7bb900
[   88.150317] 1e20: c0863e80 ec181f74 c000fa84 ec181ed0 00000000 c00deda8 ec181e50 ec181e54
[   88.158475] 1e40: 00000016 ec181f74 00000001 c00e8ebc 00000000 00000000 00000000 00000000
[   88.166635] 1e60: ec181eb8 00000041 ffede000 00000127 00000055 0000049c ee605fd8 00002000
[   88.174793] 1e80: 00000011 ec181f80 00000044 00000000 00000011 c012ca68 00000000 eb17e260
[   88.182953] 1ea0: 00000051 00000011 ec181f80 00000016 ec181f74 00000001 00000005 c000fa84
[   88.191112] 1ec0: ec180000 00000000 be9821e4 c00eaa6c 00000800 c0018570 be981c48 ee51cd24
[   88.199272] 1ee0: ec089c80 00000008 ee51ccc0 0000081f c0018388 7f727c20 c085cfb4 ec181fb0
[   88.207430] 1f00: c792168f ffffffff 00000000 ec181f10 ec181f24 ef001b80 00000001 00000001
[   88.215590] 1f20: 00000016 ee5b1000 ee74d7c0 00000100 00000000 c05a0390 00000000 c00f72ec
[   88.223749] 1f40: eeba9000 00000000 eeba9000 ffffff9c ffffff9c c000fa84 00000016 eeba9000
[   88.231908] 1f60: ffffff9c c00dc29c ec180000 be9828a8 00000051 00000000 c0000000 00000004
[   88.240067] 1f80: 00000100 00000001 00000000 7f721c00 00000008 00000008 00000005 c000fa84
[   88.248226] 1fa0: 00000000 c000f8c0 7f721c00 00000008 be9822cc 00000000 000001b6 000001b6
[   88.256386] 1fc0: 7f721c00 00000008 00000008 00000005 7f6de5c0 be9822cc be9822cc be9821e4
[   88.264544] 1fe0: 00000000 be982180 b6efb4c0 b669aa14 80000010 be9822cc 00000000 00000000
[   88.272716] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
[   88.280868] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
[   88.288591] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
[   88.296055] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
[   88.303699] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
[   88.311419] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
[   88.317573] ---[ end trace 9502799e3ea05a80 ]---


Without patch 5 it is ok.

BR,
Krzysztof

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [rtc-linux] Re: [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally
@ 2016-02-03 10:45     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2016-02-03 10:45 UTC (permalink / raw)
  To: Laxman Dewangan, lee.jones, alexandre.belloni, javier
  Cc: cw00.choi, linux-kernel, rtc-linux

On 03.02.2016 18:30, Laxman Dewangan wrote:
> To make RTC block of MAX77686/MAX77802 as independent driver,
> move the registration of i2c device, regmap for register access
> and irq_chip for interrupt support inside the RTC driver.
> Removed the same initialisation from MFD driver.
> 
> Having this change will allow to reuse this driver for different
> PMIC/devices from Maxim Semiconductor if they kept same RTC IP on
> different PMIC. Some of examples as PMIC MAX77620, MAX20024 where
> same RTC IP used and hence driver for these chips will use this
> driver only for RTC support.
> 
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> Changes from V1:
> - Remove changes from Kconfig.
> - Maintain all register definition in max77686 private header and remove
>   the movement to rtc driver.
> - Taken care of all comments on V1 from Krzysztof and Javier.

Almost everything is good. You skipped one my comment, at the end:

(...)

> @@ -659,14 +745,33 @@ 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->rtc)
> +		i2c_unregister_device(info->rtc);
> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
> +

You should clean up in reverse order of allocation, so first
regmap_del_irq_chip then i2c_unregister_device. This
is a common pattern of cleaning up. Sometimes such order
is even necessary because of dependencies between
components... which is not a case here but still the
natural way is reversing the allocation code.

>  	return ret;
>  }
>  
> +static int max77686_rtc_remove(struct platform_device *pdev)
> +{
> +	struct max77686_rtc_info *info = platform_get_drvdata(pdev);
> +
> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
> +	if (info->rtc)
> +		i2c_unregister_device(info->rtc);

Here it is okay.

Anyway RTC works... but unbinding leads to oops:
echo max77686-rtc > /sys/bus/platform/drivers/max77686-rtc/unbind
[   80.505411] Unable to handle kernel paging request at virtual address ffffffff
[   80.511257] pgd = ec3a0000
[   80.513870] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
[   80.520130] Internal error: Oops: 37 [#1] PREEMPT SMP ARM
[   80.525498] Modules linked in:
[   80.528551] CPU: 2 PID: 1325 Comm: sleep Tainted: G        W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
[   80.538867] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   80.544947] task: eeb53440 ti: eeb12000 task.ti: eeb12000
[   80.550349] PC is at kmem_cache_alloc+0x7c/0x154
[   80.554933] LR is at kmem_cache_alloc+0x3c/0x154
[   80.559535] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a00f0013
[   80.559535] sp : eeb13e18  ip : 00000001  fp : bee6c4f4
[   80.570986] r10: 024080c0  r9 : eeb13ed0  r8 : 00033d72
[   80.576195] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
[   80.582704] r3 : 00000000  r2 : 00033d72  r1 : c070e2ac  r0 : 2ef40000
[   80.589217] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   80.596332] Control: 10c5387d  Table: 6c3a004a  DAC: 00000051
[   80.602061] Process sleep (pid: 1325, stack limit = 0xeeb12210)
[   80.607963] Stack: (0xeeb13e18 to 0xeeb14000)
[   80.612306] 3e00:                                                       c0858280 ee4ada00
[   80.620475] 3e20: c0863e80 eeb13f74 c000fa84 eeb13ed0 00000000 c00deda8 b6f4f000 00000011
[   80.628632] 3e40: 00000003 eeb13f74 00000001 c00e8ebc 386de9c0 00000000 00000001 00000001
[   80.636792] 3e60: eeb53440 00000041 eee89514 0000014f 00000054 0000053c eeb6c190 ee4f653c
[   80.644934] 3e80: 00013000 ee4f6000 00000001 00000000 00000054 024200ca 00000010 b6f4e000
[   80.653093] 3ea0: ee58e7a8 b6f3e000 00000011 00000003 eeb13f74 00000001 00000005 c000fa84
[   80.661253] 3ec0: eeb12000 00000000 bee6c4f4 c00eaa6c 00000000 c0018570 eea49700 ee58e160
[   80.669412] 3ee0: 00000000 00000000 c001921c 00000017 c0018388 b6f4f0dd c085cf34 eeb13fb0
[   80.677571] 3f00: b6f96b40 7f62b751 00000000 eeb13f10 eeb13f24 ef001b80 00000001 00000001
[   80.685730] 3f20: 00000003 ee5aac00 ee5aac24 00000020 00080000 c05a0390 00000000 c00f72ec
[   80.693889] 3f40: eebac000 00000000 eebac000 ffffff9c ffffff9c c000fa84 00000003 eebac000
[   80.702048] 3f60: ffffff9c c00dc29c eeb53440 00000003 ee5aac00 00000000 eea40000 00000004
[   80.710208] 3f80: 00000100 00000001 eea49700 00000008 00000000 bee6c534 00000005 c000fa84
[   80.718367] 3fa0: 00000000 c000f8c0 00000008 00000000 b6f4f0ce 00080000 b6f96958 00000000
[   80.726526] 3fc0: 00000008 00000000 bee6c534 00000005 00000001 b6f4f0ce 00000000 bee6c4f4
[   80.734685] 3fe0: b6f95ce0 bee6c4ac b6f6ba14 b6f7f24c 600f0010 b6f4f0ce ffffffff ffffffff
[   80.742854] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
[   80.751008] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
[   80.758731] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
[   80.766195] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
[   80.773838] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
[   80.781559] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
[   80.787694] ---[ end trace 9502799e3ea05a7e ]---
[   80.792503] Unable to handle kernel paging request at virtual address ffffffff
[   80.799457] pgd = ec39c000
[   80.802140] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
[   80.808377] Internal error: Oops: 37 [#2] PREEMPT SMP ARM
[   80.813756] Modules linked in:
[   80.816798] CPU: 2 PID: 628 Comm: sh Tainted: G      D W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
[   80.826776] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   80.832853] task: ec3050c0 ti: ee66e000 task.ti: ee66e000
[   80.838240] PC is at kmem_cache_alloc+0x7c/0x154
[   80.842836] LR is at kmem_cache_alloc+0x3c/0x154
[   80.847437] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a00f0013
[   80.847437] sp : ee66fe18  ip : 00000001  fp : beaba124
[   80.858893] r10: 024080c0  r9 : ee66fed0  r8 : 00033d72
[   80.864100] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
[   80.870610] r3 : 00000000  r2 : 00033d72  r1 : c070e2ac  r0 : 2ef40000
[   80.877120] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   80.884237] Control: 10c5387d  Table: 6c39c04a  DAC: 00000051
[   80.889966] Process sh (pid: 628, stack limit = 0xee66e210)
[   80.895522] Stack: (0xee66fe18 to 0xee670000)
[   80.899862] fe00:                                                       c0858280 ec264f80
[   80.908023] fe20: c0863e80 ee66ff74 c000fa84 ee66fed0 00000000 c00deda8 00000040 ee58e9a0
[   80.916182] fe40: 00000003 ee66ff74 00000001 c00e8ebc ec39efa8 eeb6cee0 69f907df ef00cc10
[   80.924341] fe60: c0850200 00000041 00000001 00000195 00000055 00000654 ec39edb8 ee464380
[   80.932500] fe80: ffffff05 c023bd4c 00000000 00000000 00000000 ee464380 ee58e9a0 c00c0904
[   80.940660] fea0: ee464380 ee58e9a0 ee464380 00000003 ee66ff74 00000001 00000005 c000fa84
[   80.948819] fec0: ee66e000 00000000 beaba124 c00eaa6c 00000800 c0018570 ec19fa08 00000000
[   80.956978] fee0: 00000000 00000000 00000000 00000817 c0018388 b6f95000 c085cf34 ee66ffb0
[   80.965137] ff00: 00000000 b6f974c0 00000000 ee66ff10 ee66ff24 ef001b80 00000001 00000001
[   80.973296] ff20: 00000003 ee59ba00 ee405e00 00000100 00080000 c05a0390 00000000 c00f72ec
[   80.981456] ff40: eebaf000 00000000 eebaf000 ffffff9c ffffff9c c000fa84 00000003 eebaf000
[   80.989615] ff60: ffffff9c c00dc29c beaba52c c00b0e8c 00000022 00000000 000b0000 00000004
[   80.997774] ff80: 00000100 00000001 ffffffff 000d6430 00000008 00000008 00000005 c000fa84
[   81.005933] ffa0: 00000000 c000f8c0 000d6430 00000008 beaba148 00080000 000001b6 000001b6
[   81.014092] ffc0: 000d6430 00000008 00000008 00000005 00000000 b6f0405c beaba408 beaba124
[   81.022252] ffe0: 00080000 beaba0c4 b6e3529c b6e8a9bc 200f0010 beaba148 6fffd861 6fffdc61
[   81.030416] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
[   81.038572] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
[   81.046296] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
[   81.053761] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
[   81.061401] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
[   81.069124] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
[   81.075245] ---[ end trace 9502799e3ea05a7f ]---
[   88.035087] Unable to handle kernel paging request at virtual address ffffffff
[   88.040971] pgd = ee604000
[   88.043617] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
[   88.049822] Internal error: Oops: 37 [#3] PREEMPT SMP ARM
[   88.055186] Modules linked in:
[   88.058245] CPU: 2 PID: 396 Comm: deviced Tainted: G      D W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
[   88.068641] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   88.074723] task: ec089c80 ti: ec180000 task.ti: ec180000
[   88.080132] PC is at kmem_cache_alloc+0x7c/0x154
[   88.084712] LR is at kmem_cache_alloc+0x3c/0x154
[   88.089313] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a0000013
[   88.089313] sp : ec181e18  ip : 00000001  fp : be9821e4
[   88.100763] r10: 024080c0  r9 : ec181ed0  r8 : 00033d82
[   88.105971] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
[   88.112482] r3 : 00000000  r2 : 00033d82  r1 : c070e2ac  r0 : 2ef40000
[   88.118995] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   88.126095] Control: 10c5387d  Table: 6e60404a  DAC: 00000051
[   88.131824] Process deviced (pid: 396, stack limit = 0xec180210)
[   88.137812] Stack: (0xec181e18 to 0xec182000)
[   88.142153] 1e00:                                                       c0858280 ee7bb900
[   88.150317] 1e20: c0863e80 ec181f74 c000fa84 ec181ed0 00000000 c00deda8 ec181e50 ec181e54
[   88.158475] 1e40: 00000016 ec181f74 00000001 c00e8ebc 00000000 00000000 00000000 00000000
[   88.166635] 1e60: ec181eb8 00000041 ffede000 00000127 00000055 0000049c ee605fd8 00002000
[   88.174793] 1e80: 00000011 ec181f80 00000044 00000000 00000011 c012ca68 00000000 eb17e260
[   88.182953] 1ea0: 00000051 00000011 ec181f80 00000016 ec181f74 00000001 00000005 c000fa84
[   88.191112] 1ec0: ec180000 00000000 be9821e4 c00eaa6c 00000800 c0018570 be981c48 ee51cd24
[   88.199272] 1ee0: ec089c80 00000008 ee51ccc0 0000081f c0018388 7f727c20 c085cfb4 ec181fb0
[   88.207430] 1f00: c792168f ffffffff 00000000 ec181f10 ec181f24 ef001b80 00000001 00000001
[   88.215590] 1f20: 00000016 ee5b1000 ee74d7c0 00000100 00000000 c05a0390 00000000 c00f72ec
[   88.223749] 1f40: eeba9000 00000000 eeba9000 ffffff9c ffffff9c c000fa84 00000016 eeba9000
[   88.231908] 1f60: ffffff9c c00dc29c ec180000 be9828a8 00000051 00000000 c0000000 00000004
[   88.240067] 1f80: 00000100 00000001 00000000 7f721c00 00000008 00000008 00000005 c000fa84
[   88.248226] 1fa0: 00000000 c000f8c0 7f721c00 00000008 be9822cc 00000000 000001b6 000001b6
[   88.256386] 1fc0: 7f721c00 00000008 00000008 00000005 7f6de5c0 be9822cc be9822cc be9821e4
[   88.264544] 1fe0: 00000000 be982180 b6efb4c0 b669aa14 80000010 be9822cc 00000000 00000000
[   88.272716] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
[   88.280868] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
[   88.288591] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
[   88.296055] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
[   88.303699] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
[   88.311419] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
[   88.317573] ---[ end trace 9502799e3ea05a80 ]---


Without patch 5 it is ok.

BR,
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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally
  2016-02-03 10:45     ` [rtc-linux] " Krzysztof Kozlowski
@ 2016-02-03 11:22       ` Krzysztof Kozlowski
  -1 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2016-02-03 11:22 UTC (permalink / raw)
  To: Laxman Dewangan, lee.jones, alexandre.belloni, javier
  Cc: cw00.choi, linux-kernel, rtc-linux

On 03.02.2016 19:45, Krzysztof Kozlowski wrote:
> On 03.02.2016 18:30, Laxman Dewangan wrote:
>> To make RTC block of MAX77686/MAX77802 as independent driver,
>> move the registration of i2c device, regmap for register access
>> and irq_chip for interrupt support inside the RTC driver.
>> Removed the same initialisation from MFD driver.
>>
>> Having this change will allow to reuse this driver for different
>> PMIC/devices from Maxim Semiconductor if they kept same RTC IP on
>> different PMIC. Some of examples as PMIC MAX77620, MAX20024 where
>> same RTC IP used and hence driver for these chips will use this
>> driver only for RTC support.
>>
>> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> CC: Javier Martinez Canillas <javier@osg.samsung.com>
>>
>> ---
>> Changes from V1:
>> - Remove changes from Kconfig.
>> - Maintain all register definition in max77686 private header and remove
>>   the movement to rtc driver.
>> - Taken care of all comments on V1 from Krzysztof and Javier.
> 
> Almost everything is good. You skipped one my comment, at the end:
> 
> (...)
> 
>> @@ -659,14 +745,33 @@ 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->rtc)
>> +		i2c_unregister_device(info->rtc);
>> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
>> +
> 
> You should clean up in reverse order of allocation, so first
> regmap_del_irq_chip then i2c_unregister_device. This
> is a common pattern of cleaning up. Sometimes such order
> is even necessary because of dependencies between
> components... which is not a case here but still the
> natural way is reversing the allocation code.
> 
>>  	return ret;
>>  }
>>  
>> +static int max77686_rtc_remove(struct platform_device *pdev)
>> +{
>> +	struct max77686_rtc_info *info = platform_get_drvdata(pdev);
>> +
>> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
>> +	if (info->rtc)
>> +		i2c_unregister_device(info->rtc);
> 
> Here it is okay.
> 
> Anyway RTC works... but unbinding leads to oops:
> echo max77686-rtc > /sys/bus/platform/drivers/max77686-rtc/unbind
> [   80.505411] Unable to handle kernel paging request at virtual address ffffffff
> [   80.511257] pgd = ec3a0000
> [   80.513870] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
> [   80.520130] Internal error: Oops: 37 [#1] PREEMPT SMP ARM
> [   80.525498] Modules linked in:
> [   80.528551] CPU: 2 PID: 1325 Comm: sleep Tainted: G        W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
> [   80.538867] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   80.544947] task: eeb53440 ti: eeb12000 task.ti: eeb12000
> [   80.550349] PC is at kmem_cache_alloc+0x7c/0x154
> [   80.554933] LR is at kmem_cache_alloc+0x3c/0x154
> [   80.559535] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a00f0013
> [   80.559535] sp : eeb13e18  ip : 00000001  fp : bee6c4f4
> [   80.570986] r10: 024080c0  r9 : eeb13ed0  r8 : 00033d72
> [   80.576195] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
> [   80.582704] r3 : 00000000  r2 : 00033d72  r1 : c070e2ac  r0 : 2ef40000
> [   80.589217] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [   80.596332] Control: 10c5387d  Table: 6c3a004a  DAC: 00000051
> [   80.602061] Process sleep (pid: 1325, stack limit = 0xeeb12210)
> [   80.607963] Stack: (0xeeb13e18 to 0xeeb14000)
> [   80.612306] 3e00:                                                       c0858280 ee4ada00
> [   80.620475] 3e20: c0863e80 eeb13f74 c000fa84 eeb13ed0 00000000 c00deda8 b6f4f000 00000011
> [   80.628632] 3e40: 00000003 eeb13f74 00000001 c00e8ebc 386de9c0 00000000 00000001 00000001
> [   80.636792] 3e60: eeb53440 00000041 eee89514 0000014f 00000054 0000053c eeb6c190 ee4f653c
> [   80.644934] 3e80: 00013000 ee4f6000 00000001 00000000 00000054 024200ca 00000010 b6f4e000
> [   80.653093] 3ea0: ee58e7a8 b6f3e000 00000011 00000003 eeb13f74 00000001 00000005 c000fa84
> [   80.661253] 3ec0: eeb12000 00000000 bee6c4f4 c00eaa6c 00000000 c0018570 eea49700 ee58e160
> [   80.669412] 3ee0: 00000000 00000000 c001921c 00000017 c0018388 b6f4f0dd c085cf34 eeb13fb0
> [   80.677571] 3f00: b6f96b40 7f62b751 00000000 eeb13f10 eeb13f24 ef001b80 00000001 00000001
> [   80.685730] 3f20: 00000003 ee5aac00 ee5aac24 00000020 00080000 c05a0390 00000000 c00f72ec
> [   80.693889] 3f40: eebac000 00000000 eebac000 ffffff9c ffffff9c c000fa84 00000003 eebac000
> [   80.702048] 3f60: ffffff9c c00dc29c eeb53440 00000003 ee5aac00 00000000 eea40000 00000004
> [   80.710208] 3f80: 00000100 00000001 eea49700 00000008 00000000 bee6c534 00000005 c000fa84
> [   80.718367] 3fa0: 00000000 c000f8c0 00000008 00000000 b6f4f0ce 00080000 b6f96958 00000000
> [   80.726526] 3fc0: 00000008 00000000 bee6c534 00000005 00000001 b6f4f0ce 00000000 bee6c4f4
> [   80.734685] 3fe0: b6f95ce0 bee6c4ac b6f6ba14 b6f7f24c 600f0010 b6f4f0ce ffffffff ffffffff
> [   80.742854] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
> [   80.751008] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
> [   80.758731] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
> [   80.766195] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
> [   80.773838] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
> [   80.781559] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
> [   80.787694] ---[ end trace 9502799e3ea05a7e ]---
> [   80.792503] Unable to handle kernel paging request at virtual address ffffffff
> [   80.799457] pgd = ec39c000
> [   80.802140] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
> [   80.808377] Internal error: Oops: 37 [#2] PREEMPT SMP ARM
> [   80.813756] Modules linked in:
> [   80.816798] CPU: 2 PID: 628 Comm: sh Tainted: G      D W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
> [   80.826776] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   80.832853] task: ec3050c0 ti: ee66e000 task.ti: ee66e000
> [   80.838240] PC is at kmem_cache_alloc+0x7c/0x154
> [   80.842836] LR is at kmem_cache_alloc+0x3c/0x154
> [   80.847437] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a00f0013
> [   80.847437] sp : ee66fe18  ip : 00000001  fp : beaba124
> [   80.858893] r10: 024080c0  r9 : ee66fed0  r8 : 00033d72
> [   80.864100] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
> [   80.870610] r3 : 00000000  r2 : 00033d72  r1 : c070e2ac  r0 : 2ef40000
> [   80.877120] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [   80.884237] Control: 10c5387d  Table: 6c39c04a  DAC: 00000051
> [   80.889966] Process sh (pid: 628, stack limit = 0xee66e210)
> [   80.895522] Stack: (0xee66fe18 to 0xee670000)
> [   80.899862] fe00:                                                       c0858280 ec264f80
> [   80.908023] fe20: c0863e80 ee66ff74 c000fa84 ee66fed0 00000000 c00deda8 00000040 ee58e9a0
> [   80.916182] fe40: 00000003 ee66ff74 00000001 c00e8ebc ec39efa8 eeb6cee0 69f907df ef00cc10
> [   80.924341] fe60: c0850200 00000041 00000001 00000195 00000055 00000654 ec39edb8 ee464380
> [   80.932500] fe80: ffffff05 c023bd4c 00000000 00000000 00000000 ee464380 ee58e9a0 c00c0904
> [   80.940660] fea0: ee464380 ee58e9a0 ee464380 00000003 ee66ff74 00000001 00000005 c000fa84
> [   80.948819] fec0: ee66e000 00000000 beaba124 c00eaa6c 00000800 c0018570 ec19fa08 00000000
> [   80.956978] fee0: 00000000 00000000 00000000 00000817 c0018388 b6f95000 c085cf34 ee66ffb0
> [   80.965137] ff00: 00000000 b6f974c0 00000000 ee66ff10 ee66ff24 ef001b80 00000001 00000001
> [   80.973296] ff20: 00000003 ee59ba00 ee405e00 00000100 00080000 c05a0390 00000000 c00f72ec
> [   80.981456] ff40: eebaf000 00000000 eebaf000 ffffff9c ffffff9c c000fa84 00000003 eebaf000
> [   80.989615] ff60: ffffff9c c00dc29c beaba52c c00b0e8c 00000022 00000000 000b0000 00000004
> [   80.997774] ff80: 00000100 00000001 ffffffff 000d6430 00000008 00000008 00000005 c000fa84
> [   81.005933] ffa0: 00000000 c000f8c0 000d6430 00000008 beaba148 00080000 000001b6 000001b6
> [   81.014092] ffc0: 000d6430 00000008 00000008 00000005 00000000 b6f0405c beaba408 beaba124
> [   81.022252] ffe0: 00080000 beaba0c4 b6e3529c b6e8a9bc 200f0010 beaba148 6fffd861 6fffdc61
> [   81.030416] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
> [   81.038572] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
> [   81.046296] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
> [   81.053761] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
> [   81.061401] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
> [   81.069124] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
> [   81.075245] ---[ end trace 9502799e3ea05a7f ]---
> [   88.035087] Unable to handle kernel paging request at virtual address ffffffff
> [   88.040971] pgd = ee604000
> [   88.043617] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
> [   88.049822] Internal error: Oops: 37 [#3] PREEMPT SMP ARM
> [   88.055186] Modules linked in:
> [   88.058245] CPU: 2 PID: 396 Comm: deviced Tainted: G      D W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
> [   88.068641] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   88.074723] task: ec089c80 ti: ec180000 task.ti: ec180000
> [   88.080132] PC is at kmem_cache_alloc+0x7c/0x154
> [   88.084712] LR is at kmem_cache_alloc+0x3c/0x154
> [   88.089313] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a0000013
> [   88.089313] sp : ec181e18  ip : 00000001  fp : be9821e4
> [   88.100763] r10: 024080c0  r9 : ec181ed0  r8 : 00033d82
> [   88.105971] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
> [   88.112482] r3 : 00000000  r2 : 00033d82  r1 : c070e2ac  r0 : 2ef40000
> [   88.118995] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [   88.126095] Control: 10c5387d  Table: 6e60404a  DAC: 00000051
> [   88.131824] Process deviced (pid: 396, stack limit = 0xec180210)
> [   88.137812] Stack: (0xec181e18 to 0xec182000)
> [   88.142153] 1e00:                                                       c0858280 ee7bb900
> [   88.150317] 1e20: c0863e80 ec181f74 c000fa84 ec181ed0 00000000 c00deda8 ec181e50 ec181e54
> [   88.158475] 1e40: 00000016 ec181f74 00000001 c00e8ebc 00000000 00000000 00000000 00000000
> [   88.166635] 1e60: ec181eb8 00000041 ffede000 00000127 00000055 0000049c ee605fd8 00002000
> [   88.174793] 1e80: 00000011 ec181f80 00000044 00000000 00000011 c012ca68 00000000 eb17e260
> [   88.182953] 1ea0: 00000051 00000011 ec181f80 00000016 ec181f74 00000001 00000005 c000fa84
> [   88.191112] 1ec0: ec180000 00000000 be9821e4 c00eaa6c 00000800 c0018570 be981c48 ee51cd24
> [   88.199272] 1ee0: ec089c80 00000008 ee51ccc0 0000081f c0018388 7f727c20 c085cfb4 ec181fb0
> [   88.207430] 1f00: c792168f ffffffff 00000000 ec181f10 ec181f24 ef001b80 00000001 00000001
> [   88.215590] 1f20: 00000016 ee5b1000 ee74d7c0 00000100 00000000 c05a0390 00000000 c00f72ec
> [   88.223749] 1f40: eeba9000 00000000 eeba9000 ffffff9c ffffff9c c000fa84 00000016 eeba9000
> [   88.231908] 1f60: ffffff9c c00dc29c ec180000 be9828a8 00000051 00000000 c0000000 00000004
> [   88.240067] 1f80: 00000100 00000001 00000000 7f721c00 00000008 00000008 00000005 c000fa84
> [   88.248226] 1fa0: 00000000 c000f8c0 7f721c00 00000008 be9822cc 00000000 000001b6 000001b6
> [   88.256386] 1fc0: 7f721c00 00000008 00000008 00000005 7f6de5c0 be9822cc be9822cc be9821e4
> [   88.264544] 1fe0: 00000000 be982180 b6efb4c0 b669aa14 80000010 be9822cc 00000000 00000000
> [   88.272716] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
> [   88.280868] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
> [   88.288591] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
> [   88.296055] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
> [   88.303699] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
> [   88.311419] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
> [   88.317573] ---[ end trace 9502799e3ea05a80 ]---

However removal of "remove" callback helps... which could be expected...
maybe it is not an error of the driver itself?

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [rtc-linux] Re: [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally
@ 2016-02-03 11:22       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2016-02-03 11:22 UTC (permalink / raw)
  To: Laxman Dewangan, lee.jones, alexandre.belloni, javier
  Cc: cw00.choi, linux-kernel, rtc-linux

On 03.02.2016 19:45, Krzysztof Kozlowski wrote:
> On 03.02.2016 18:30, Laxman Dewangan wrote:
>> To make RTC block of MAX77686/MAX77802 as independent driver,
>> move the registration of i2c device, regmap for register access
>> and irq_chip for interrupt support inside the RTC driver.
>> Removed the same initialisation from MFD driver.
>>
>> Having this change will allow to reuse this driver for different
>> PMIC/devices from Maxim Semiconductor if they kept same RTC IP on
>> different PMIC. Some of examples as PMIC MAX77620, MAX20024 where
>> same RTC IP used and hence driver for these chips will use this
>> driver only for RTC support.
>>
>> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> CC: Javier Martinez Canillas <javier@osg.samsung.com>
>>
>> ---
>> Changes from V1:
>> - Remove changes from Kconfig.
>> - Maintain all register definition in max77686 private header and remove
>>   the movement to rtc driver.
>> - Taken care of all comments on V1 from Krzysztof and Javier.
> 
> Almost everything is good. You skipped one my comment, at the end:
> 
> (...)
> 
>> @@ -659,14 +745,33 @@ 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->rtc)
>> +		i2c_unregister_device(info->rtc);
>> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
>> +
> 
> You should clean up in reverse order of allocation, so first
> regmap_del_irq_chip then i2c_unregister_device. This
> is a common pattern of cleaning up. Sometimes such order
> is even necessary because of dependencies between
> components... which is not a case here but still the
> natural way is reversing the allocation code.
> 
>>  	return ret;
>>  }
>>  
>> +static int max77686_rtc_remove(struct platform_device *pdev)
>> +{
>> +	struct max77686_rtc_info *info = platform_get_drvdata(pdev);
>> +
>> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
>> +	if (info->rtc)
>> +		i2c_unregister_device(info->rtc);
> 
> Here it is okay.
> 
> Anyway RTC works... but unbinding leads to oops:
> echo max77686-rtc > /sys/bus/platform/drivers/max77686-rtc/unbind
> [   80.505411] Unable to handle kernel paging request at virtual address ffffffff
> [   80.511257] pgd = ec3a0000
> [   80.513870] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
> [   80.520130] Internal error: Oops: 37 [#1] PREEMPT SMP ARM
> [   80.525498] Modules linked in:
> [   80.528551] CPU: 2 PID: 1325 Comm: sleep Tainted: G        W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
> [   80.538867] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   80.544947] task: eeb53440 ti: eeb12000 task.ti: eeb12000
> [   80.550349] PC is at kmem_cache_alloc+0x7c/0x154
> [   80.554933] LR is at kmem_cache_alloc+0x3c/0x154
> [   80.559535] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a00f0013
> [   80.559535] sp : eeb13e18  ip : 00000001  fp : bee6c4f4
> [   80.570986] r10: 024080c0  r9 : eeb13ed0  r8 : 00033d72
> [   80.576195] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
> [   80.582704] r3 : 00000000  r2 : 00033d72  r1 : c070e2ac  r0 : 2ef40000
> [   80.589217] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [   80.596332] Control: 10c5387d  Table: 6c3a004a  DAC: 00000051
> [   80.602061] Process sleep (pid: 1325, stack limit = 0xeeb12210)
> [   80.607963] Stack: (0xeeb13e18 to 0xeeb14000)
> [   80.612306] 3e00:                                                       c0858280 ee4ada00
> [   80.620475] 3e20: c0863e80 eeb13f74 c000fa84 eeb13ed0 00000000 c00deda8 b6f4f000 00000011
> [   80.628632] 3e40: 00000003 eeb13f74 00000001 c00e8ebc 386de9c0 00000000 00000001 00000001
> [   80.636792] 3e60: eeb53440 00000041 eee89514 0000014f 00000054 0000053c eeb6c190 ee4f653c
> [   80.644934] 3e80: 00013000 ee4f6000 00000001 00000000 00000054 024200ca 00000010 b6f4e000
> [   80.653093] 3ea0: ee58e7a8 b6f3e000 00000011 00000003 eeb13f74 00000001 00000005 c000fa84
> [   80.661253] 3ec0: eeb12000 00000000 bee6c4f4 c00eaa6c 00000000 c0018570 eea49700 ee58e160
> [   80.669412] 3ee0: 00000000 00000000 c001921c 00000017 c0018388 b6f4f0dd c085cf34 eeb13fb0
> [   80.677571] 3f00: b6f96b40 7f62b751 00000000 eeb13f10 eeb13f24 ef001b80 00000001 00000001
> [   80.685730] 3f20: 00000003 ee5aac00 ee5aac24 00000020 00080000 c05a0390 00000000 c00f72ec
> [   80.693889] 3f40: eebac000 00000000 eebac000 ffffff9c ffffff9c c000fa84 00000003 eebac000
> [   80.702048] 3f60: ffffff9c c00dc29c eeb53440 00000003 ee5aac00 00000000 eea40000 00000004
> [   80.710208] 3f80: 00000100 00000001 eea49700 00000008 00000000 bee6c534 00000005 c000fa84
> [   80.718367] 3fa0: 00000000 c000f8c0 00000008 00000000 b6f4f0ce 00080000 b6f96958 00000000
> [   80.726526] 3fc0: 00000008 00000000 bee6c534 00000005 00000001 b6f4f0ce 00000000 bee6c4f4
> [   80.734685] 3fe0: b6f95ce0 bee6c4ac b6f6ba14 b6f7f24c 600f0010 b6f4f0ce ffffffff ffffffff
> [   80.742854] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
> [   80.751008] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
> [   80.758731] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
> [   80.766195] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
> [   80.773838] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
> [   80.781559] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
> [   80.787694] ---[ end trace 9502799e3ea05a7e ]---
> [   80.792503] Unable to handle kernel paging request at virtual address ffffffff
> [   80.799457] pgd = ec39c000
> [   80.802140] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
> [   80.808377] Internal error: Oops: 37 [#2] PREEMPT SMP ARM
> [   80.813756] Modules linked in:
> [   80.816798] CPU: 2 PID: 628 Comm: sh Tainted: G      D W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
> [   80.826776] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   80.832853] task: ec3050c0 ti: ee66e000 task.ti: ee66e000
> [   80.838240] PC is at kmem_cache_alloc+0x7c/0x154
> [   80.842836] LR is at kmem_cache_alloc+0x3c/0x154
> [   80.847437] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a00f0013
> [   80.847437] sp : ee66fe18  ip : 00000001  fp : beaba124
> [   80.858893] r10: 024080c0  r9 : ee66fed0  r8 : 00033d72
> [   80.864100] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
> [   80.870610] r3 : 00000000  r2 : 00033d72  r1 : c070e2ac  r0 : 2ef40000
> [   80.877120] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [   80.884237] Control: 10c5387d  Table: 6c39c04a  DAC: 00000051
> [   80.889966] Process sh (pid: 628, stack limit = 0xee66e210)
> [   80.895522] Stack: (0xee66fe18 to 0xee670000)
> [   80.899862] fe00:                                                       c0858280 ec264f80
> [   80.908023] fe20: c0863e80 ee66ff74 c000fa84 ee66fed0 00000000 c00deda8 00000040 ee58e9a0
> [   80.916182] fe40: 00000003 ee66ff74 00000001 c00e8ebc ec39efa8 eeb6cee0 69f907df ef00cc10
> [   80.924341] fe60: c0850200 00000041 00000001 00000195 00000055 00000654 ec39edb8 ee464380
> [   80.932500] fe80: ffffff05 c023bd4c 00000000 00000000 00000000 ee464380 ee58e9a0 c00c0904
> [   80.940660] fea0: ee464380 ee58e9a0 ee464380 00000003 ee66ff74 00000001 00000005 c000fa84
> [   80.948819] fec0: ee66e000 00000000 beaba124 c00eaa6c 00000800 c0018570 ec19fa08 00000000
> [   80.956978] fee0: 00000000 00000000 00000000 00000817 c0018388 b6f95000 c085cf34 ee66ffb0
> [   80.965137] ff00: 00000000 b6f974c0 00000000 ee66ff10 ee66ff24 ef001b80 00000001 00000001
> [   80.973296] ff20: 00000003 ee59ba00 ee405e00 00000100 00080000 c05a0390 00000000 c00f72ec
> [   80.981456] ff40: eebaf000 00000000 eebaf000 ffffff9c ffffff9c c000fa84 00000003 eebaf000
> [   80.989615] ff60: ffffff9c c00dc29c beaba52c c00b0e8c 00000022 00000000 000b0000 00000004
> [   80.997774] ff80: 00000100 00000001 ffffffff 000d6430 00000008 00000008 00000005 c000fa84
> [   81.005933] ffa0: 00000000 c000f8c0 000d6430 00000008 beaba148 00080000 000001b6 000001b6
> [   81.014092] ffc0: 000d6430 00000008 00000008 00000005 00000000 b6f0405c beaba408 beaba124
> [   81.022252] ffe0: 00080000 beaba0c4 b6e3529c b6e8a9bc 200f0010 beaba148 6fffd861 6fffdc61
> [   81.030416] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
> [   81.038572] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
> [   81.046296] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
> [   81.053761] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
> [   81.061401] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
> [   81.069124] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
> [   81.075245] ---[ end trace 9502799e3ea05a7f ]---
> [   88.035087] Unable to handle kernel paging request at virtual address ffffffff
> [   88.040971] pgd = ee604000
> [   88.043617] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
> [   88.049822] Internal error: Oops: 37 [#3] PREEMPT SMP ARM
> [   88.055186] Modules linked in:
> [   88.058245] CPU: 2 PID: 396 Comm: deviced Tainted: G      D W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
> [   88.068641] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   88.074723] task: ec089c80 ti: ec180000 task.ti: ec180000
> [   88.080132] PC is at kmem_cache_alloc+0x7c/0x154
> [   88.084712] LR is at kmem_cache_alloc+0x3c/0x154
> [   88.089313] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a0000013
> [   88.089313] sp : ec181e18  ip : 00000001  fp : be9821e4
> [   88.100763] r10: 024080c0  r9 : ec181ed0  r8 : 00033d82
> [   88.105971] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
> [   88.112482] r3 : 00000000  r2 : 00033d82  r1 : c070e2ac  r0 : 2ef40000
> [   88.118995] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [   88.126095] Control: 10c5387d  Table: 6e60404a  DAC: 00000051
> [   88.131824] Process deviced (pid: 396, stack limit = 0xec180210)
> [   88.137812] Stack: (0xec181e18 to 0xec182000)
> [   88.142153] 1e00:                                                       c0858280 ee7bb900
> [   88.150317] 1e20: c0863e80 ec181f74 c000fa84 ec181ed0 00000000 c00deda8 ec181e50 ec181e54
> [   88.158475] 1e40: 00000016 ec181f74 00000001 c00e8ebc 00000000 00000000 00000000 00000000
> [   88.166635] 1e60: ec181eb8 00000041 ffede000 00000127 00000055 0000049c ee605fd8 00002000
> [   88.174793] 1e80: 00000011 ec181f80 00000044 00000000 00000011 c012ca68 00000000 eb17e260
> [   88.182953] 1ea0: 00000051 00000011 ec181f80 00000016 ec181f74 00000001 00000005 c000fa84
> [   88.191112] 1ec0: ec180000 00000000 be9821e4 c00eaa6c 00000800 c0018570 be981c48 ee51cd24
> [   88.199272] 1ee0: ec089c80 00000008 ee51ccc0 0000081f c0018388 7f727c20 c085cfb4 ec181fb0
> [   88.207430] 1f00: c792168f ffffffff 00000000 ec181f10 ec181f24 ef001b80 00000001 00000001
> [   88.215590] 1f20: 00000016 ee5b1000 ee74d7c0 00000100 00000000 c05a0390 00000000 c00f72ec
> [   88.223749] 1f40: eeba9000 00000000 eeba9000 ffffff9c ffffff9c c000fa84 00000016 eeba9000
> [   88.231908] 1f60: ffffff9c c00dc29c ec180000 be9828a8 00000051 00000000 c0000000 00000004
> [   88.240067] 1f80: 00000100 00000001 00000000 7f721c00 00000008 00000008 00000005 c000fa84
> [   88.248226] 1fa0: 00000000 c000f8c0 7f721c00 00000008 be9822cc 00000000 000001b6 000001b6
> [   88.256386] 1fc0: 7f721c00 00000008 00000008 00000005 7f6de5c0 be9822cc be9822cc be9821e4
> [   88.264544] 1fe0: 00000000 be982180 b6efb4c0 b669aa14 80000010 be9822cc 00000000 00000000
> [   88.272716] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
> [   88.280868] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
> [   88.288591] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
> [   88.296055] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
> [   88.303699] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
> [   88.311419] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
> [   88.317573] ---[ end trace 9502799e3ea05a80 ]---

However removal of "remove" callback helps... which could be expected...
maybe it is not an error of the driver itself?

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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally
  2016-02-03 11:22       ` [rtc-linux] " Krzysztof Kozlowski
@ 2016-02-03 13:13         ` Laxman Dewangan
  -1 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03 13:13 UTC (permalink / raw)
  To: Krzysztof Kozlowski, lee.jones, alexandre.belloni, javier
  Cc: cw00.choi, linux-kernel, rtc-linux


On Wednesday 03 February 2016 04:52 PM, Krzysztof Kozlowski wrote:
> On 03.02.2016 19:45, Krzysztof Kozlowski wrote:
>> On 03.02.2016 18:30, Laxman Dewangan wrote:
>>>   
>>>   err_rtc:
>>> +	if (info->rtc)
>>> +		i2c_unregister_device(info->rtc);
>>> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
>>> +
>> You should clean up in reverse order of allocation, so first
>> regmap_del_irq_chip then i2c_unregister_device. This
>> is a common pattern of cleaning up. Sometimes such order
>> is even necessary because of dependencies between
>> components... which is not a case here but still the
>> natural way is reversing the allocation code.

I made the change in other place (remove callback) but not this place. 
It is just missed by me. my bad..
Will do in next patch.

>> [   88.296055] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
>> [   88.303699] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
>> [   88.311419] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
>> [   88.317573] ---[ end trace 9502799e3ea05a80 ]---
> However removal of "remove" callback helps... which could be expected...
> maybe it is not an error of the driver itself?


OK, looked it and found that we are registering the chip_irq as normal 
and interrupt as devm_*.
So when we remove, we delete the regmap_irq_chip first and then free 
irq. That is creating issue.

It seems I can not use the devm_requested_irq_thread() and need to use 
requested_irq_thread()  for proper sequence of freeing it.

Otherwise we need to add the devm_regmap_add_irq_chip() first and then 
use it.

I am spinning the series to use the requested_thread_irq() only to avoid 
unbind issue.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [rtc-linux] Re: [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally
@ 2016-02-03 13:13         ` Laxman Dewangan
  0 siblings, 0 replies; 26+ messages in thread
From: Laxman Dewangan @ 2016-02-03 13:13 UTC (permalink / raw)
  To: Krzysztof Kozlowski, lee.jones, alexandre.belloni, javier
  Cc: cw00.choi, linux-kernel, rtc-linux


On Wednesday 03 February 2016 04:52 PM, Krzysztof Kozlowski wrote:
> On 03.02.2016 19:45, Krzysztof Kozlowski wrote:
>> On 03.02.2016 18:30, Laxman Dewangan wrote:
>>>   
>>>   err_rtc:
>>> +	if (info->rtc)
>>> +		i2c_unregister_device(info->rtc);
>>> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
>>> +
>> You should clean up in reverse order of allocation, so first
>> regmap_del_irq_chip then i2c_unregister_device. This
>> is a common pattern of cleaning up. Sometimes such order
>> is even necessary because of dependencies between
>> components... which is not a case here but still the
>> natural way is reversing the allocation code.

I made the change in other place (remove callback) but not this place. 
It is just missed by me. my bad..
Will do in next patch.

>> [   88.296055] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
>> [   88.303699] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
>> [   88.311419] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
>> [   88.317573] ---[ end trace 9502799e3ea05a80 ]---
> However removal of "remove" callback helps... which could be expected...
> maybe it is not an error of the driver itself?


OK, looked it and found that we are registering the chip_irq as normal 
and interrupt as devm_*.
So when we remove, we delete the regmap_irq_chip first and then free 
irq. That is creating issue.

It seems I can not use the devm_requested_irq_thread() and need to use 
requested_irq_thread()  for proper sequence of freeing it.

Otherwise we need to add the devm_regmap_add_irq_chip() first and then 
use it.

I am spinning the series to use the requested_thread_irq() only to avoid 
unbind issue.


-- 
-- 
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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally
  2016-02-03  9:30   ` [rtc-linux] " Laxman Dewangan
@ 2016-02-03 12:56     ` Javier Martinez Canillas
  -1 siblings, 0 replies; 26+ messages in thread
From: Javier Martinez Canillas @ 2016-02-03 12:56 UTC (permalink / raw)
  To: Laxman Dewangan, lee.jones, alexandre.belloni, k.kozlowski
  Cc: cw00.choi, linux-kernel, rtc-linux

Hello Laxman,

On 02/03/2016 06:30 AM, Laxman Dewangan wrote:
> To make RTC block of MAX77686/MAX77802 as independent driver,
> move the registration of i2c device, regmap for register access
> and irq_chip for interrupt support inside the RTC driver.
> Removed the same initialisation from MFD driver.
>
> Having this change will allow to reuse this driver for different
> PMIC/devices from Maxim Semiconductor if they kept same RTC IP on
> different PMIC. Some of examples as PMIC MAX77620, MAX20024 where
> same RTC IP used and hence driver for these chips will use this
> driver only for RTC support.
>
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
>

[snip]

>
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index ab1f2cd..10984c4 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -12,6 +12,7 @@
>    *
>    */
>
> +#include <linux/i2c.h>
>   #include <linux/slab.h>
>   #include <linux/rtc.h>
>   #include <linux/delay.h>
> @@ -22,6 +23,9 @@
>   #include <linux/irqdomain.h>
>   #include <linux/regmap.h>
>
> +#define MAX77686_I2C_ADDR_RTC		(0x0C >> 1)
> +#define INVALID_I2C_ADDR		(-1)
> +

Maybe call it MAX77686_INVALID_I2C_ADDR for consistency?

The patch looks good modulo the issues pointed out by Krzysztof, so after
fixing these feel free to add:

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

Since you are going to re-spin another version, I'll wait for that to test.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [rtc-linux] Re: [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally
@ 2016-02-03 12:56     ` Javier Martinez Canillas
  0 siblings, 0 replies; 26+ messages in thread
From: Javier Martinez Canillas @ 2016-02-03 12:56 UTC (permalink / raw)
  To: Laxman Dewangan, lee.jones, alexandre.belloni, k.kozlowski
  Cc: cw00.choi, linux-kernel, rtc-linux

Hello Laxman,

On 02/03/2016 06:30 AM, Laxman Dewangan wrote:
> To make RTC block of MAX77686/MAX77802 as independent driver,
> move the registration of i2c device, regmap for register access
> and irq_chip for interrupt support inside the RTC driver.
> Removed the same initialisation from MFD driver.
>
> Having this change will allow to reuse this driver for different
> PMIC/devices from Maxim Semiconductor if they kept same RTC IP on
> different PMIC. Some of examples as PMIC MAX77620, MAX20024 where
> same RTC IP used and hence driver for these chips will use this
> driver only for RTC support.
>
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> CC: Javier Martinez Canillas <javier@osg.samsung.com>
>

[snip]

>
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index ab1f2cd..10984c4 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -12,6 +12,7 @@
>    *
>    */
>
> +#include <linux/i2c.h>
>   #include <linux/slab.h>
>   #include <linux/rtc.h>
>   #include <linux/delay.h>
> @@ -22,6 +23,9 @@
>   #include <linux/irqdomain.h>
>   #include <linux/regmap.h>
>
> +#define MAX77686_I2C_ADDR_RTC		(0x0C >> 1)
> +#define INVALID_I2C_ADDR		(-1)
> +

Maybe call it MAX77686_INVALID_I2C_ADDR for consistency?

The patch looks good modulo the issues pointed out by Krzysztof, so after
fixing these feel free to add:

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

Since you are going to re-spin another version, I'll wait for that to test.

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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2016-02-03 13:24 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-03  9:30 [PATCH V2 0/5] rtc: max77686: make max77686 rtc driver as IP driver Laxman Dewangan
2016-02-03  9:30 ` [rtc-linux] " Laxman Dewangan
2016-02-03  9:30 ` [PATCH V2 1/5] rtc: max77686: fix checkpatch error Laxman Dewangan
2016-02-03  9:30   ` [rtc-linux] " Laxman Dewangan
2016-02-03  9:30 ` [PATCH V2 2/5] rtc: max77686: use rtc regmap to access RTC registers Laxman Dewangan
2016-02-03  9:30   ` [rtc-linux] " Laxman Dewangan
2016-02-03  9:30 ` [PATCH V2 3/5] rtc: max77686: avoid reference of parent device info multiple palces Laxman Dewangan
2016-02-03  9:30   ` [rtc-linux] " Laxman Dewangan
2016-02-03 12:45   ` Javier Martinez Canillas
2016-02-03 12:45     ` [rtc-linux] " Javier Martinez Canillas
2016-02-03  9:30 ` [PATCH V2 4/5] mfd: max77686: do not set i2c client data for rtc i2c client Laxman Dewangan
2016-02-03  9:30   ` [rtc-linux] " Laxman Dewangan
2016-02-03 10:19   ` Krzysztof Kozlowski
2016-02-03 10:19     ` [rtc-linux] " Krzysztof Kozlowski
2016-02-03 12:47   ` Javier Martinez Canillas
2016-02-03 12:47     ` [rtc-linux] " Javier Martinez Canillas
2016-02-03  9:30 ` [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally Laxman Dewangan
2016-02-03  9:30   ` [rtc-linux] " Laxman Dewangan
2016-02-03 10:45   ` Krzysztof Kozlowski
2016-02-03 10:45     ` [rtc-linux] " Krzysztof Kozlowski
2016-02-03 11:22     ` Krzysztof Kozlowski
2016-02-03 11:22       ` [rtc-linux] " Krzysztof Kozlowski
2016-02-03 13:13       ` Laxman Dewangan
2016-02-03 13:13         ` [rtc-linux] " Laxman Dewangan
2016-02-03 12:56   ` Javier Martinez Canillas
2016-02-03 12:56     ` [rtc-linux] " Javier Martinez Canillas

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.