[PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Krzysztof Kozlowski]

Remove the 'rtc' prefix from some of the fields in struct
s5m_rtc_reg_config because it is obvious - this is a RTC driver. No
functional changes.

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

---

Changes since v1:
1. None.
---
 drivers/rtc/rtc-s5m.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index 0d68a85dd429..85649861a6b0 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -55,9 +55,9 @@ struct s5m_rtc_reg_config {
 	 * will enable update of time or alarm register. Then it will be
 	 * auto-cleared after successful update.
 	 */
-	unsigned int rtc_udr_update;
-	/* Mask for UDR field in 'rtc_udr_update' register */
-	unsigned int rtc_udr_mask;
+	unsigned int udr_update;
+	/* Mask for UDR field in 'udr_update' register */
+	unsigned int udr_mask;
 };
 
 /* Register map for S5M8763 and S5M8767 */
@@ -67,8 +67,8 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
 	.ctrl			= S5M_ALARM1_CONF,
 	.alarm0			= S5M_ALARM0_SEC,
 	.alarm1			= S5M_ALARM1_SEC,
-	.rtc_udr_update		= S5M_RTC_UDR_CON,
-	.rtc_udr_mask		= S5M_RTC_UDR_MASK,
+	.udr_update		= S5M_RTC_UDR_CON,
+	.udr_mask		= S5M_RTC_UDR_MASK,
 };
 
 /*
@@ -81,8 +81,8 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
 	.ctrl			= S2MPS_RTC_CTRL,
 	.alarm0			= S2MPS_ALARM0_SEC,
 	.alarm1			= S2MPS_ALARM1_SEC,
-	.rtc_udr_update		= S2MPS_RTC_UDR_CON,
-	.rtc_udr_mask		= S2MPS_RTC_WUDR_MASK,
+	.udr_update		= S2MPS_RTC_UDR_CON,
+	.udr_mask		= S2MPS_RTC_WUDR_MASK,
 };
 
 struct s5m_rtc_info {
@@ -166,9 +166,8 @@ static inline int s5m8767_wait_for_udr_update(struct s5m_rtc_info *info)
 	unsigned int data;
 
 	do {
-		ret = regmap_read(info->regmap, info->regs->rtc_udr_update,
-				&data);
-	} while (--retry && (data & info->regs->rtc_udr_mask) && !ret);
+		ret = regmap_read(info->regmap, info->regs->udr_update, &data);
+	} while (--retry && (data & info->regs->udr_mask) && !ret);
 
 	if (!retry)
 		dev_err(info->dev, "waiting for UDR update, reached max number of retries\n");
@@ -214,7 +213,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
 	int ret;
 	unsigned int data;
 
-	ret = regmap_read(info->regmap, info->regs->rtc_udr_update, &data);
+	ret = regmap_read(info->regmap, info->regs->udr_update, &data);
 	if (ret < 0) {
 		dev_err(info->dev, "failed to read update reg(%d)\n", ret);
 		return ret;
@@ -223,21 +222,20 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
 	switch (info->device_type) {
 	case S5M8763X:
 	case S5M8767X:
-		data |= info->regs->rtc_udr_mask | S5M_RTC_TIME_EN_MASK;
+		data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
 	case S2MPS15X:
 		/* As per UM, for write time register, set WUDR bit to high */
 		data |= S2MPS15_RTC_WUDR_MASK;
 		break;
 	case S2MPS14X:
 	case S2MPS13X:
-		data |= info->regs->rtc_udr_mask;
+		data |= info->regs->udr_mask;
 		break;
 	default:
 		return -EINVAL;
 	}
 
-
-	ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
+	ret = regmap_write(info->regmap, info->regs->udr_update, data);
 	if (ret < 0) {
 		dev_err(info->dev, "failed to write update reg(%d)\n", ret);
 		return ret;
@@ -253,14 +251,14 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
 	int ret;
 	unsigned int data;
 
-	ret = regmap_read(info->regmap, info->regs->rtc_udr_update, &data);
+	ret = regmap_read(info->regmap, info->regs->udr_update, &data);
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to read update reg(%d)\n",
 			__func__, ret);
 		return ret;
 	}
 
-	data |= info->regs->rtc_udr_mask;
+	data |= info->regs->udr_mask;
 	switch (info->device_type) {
 	case S5M8763X:
 	case S5M8767X:
@@ -268,7 +266,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
 		break;
 	case S2MPS15X:
 		/* As per UM, for write alarm, set A_UDR(bit[4]) to high
-		 * rtc_udr_mask above sets bit[4]
+		 * udr_mask above sets bit[4]
 		 */
 		break;
 	case S2MPS14X:
@@ -281,7 +279,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
 		return -EINVAL;
 	}
 
-	ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
+	ret = regmap_write(info->regmap, info->regs->udr_update, data);
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to write update reg(%d)\n",
 			__func__, ret);
@@ -292,7 +290,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
 
 	/* On S2MPS13 the AUDR is not auto-cleared */
 	if (info->device_type == S2MPS13X)
-		regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
+		regmap_update_bits(info->regmap, info->regs->udr_update,
 				   S2MPS13_RTC_AUDR_MASK, 0);
 
 	return ret;
@@ -339,7 +337,7 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
 			info->device_type == S2MPS13X) {
 		ret = regmap_update_bits(info->regmap,
-				info->regs->rtc_udr_update,
+				info->regs->udr_update,
 				S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
 		if (ret) {
 			dev_err(dev,
-- 
1.9.1

[rtc-linux] [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Krzysztof Kozlowski]

Remove the 'rtc' prefix from some of the fields in struct
s5m_rtc_reg_config because it is obvious - this is a RTC driver. No
functional changes.

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

---

Changes since v1:
1. None.
---
 drivers/rtc/rtc-s5m.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index 0d68a85dd429..85649861a6b0 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -55,9 +55,9 @@ struct s5m_rtc_reg_config {
 	 * will enable update of time or alarm register. Then it will be
 	 * auto-cleared after successful update.
 	 */
-	unsigned int rtc_udr_update;
-	/* Mask for UDR field in 'rtc_udr_update' register */
-	unsigned int rtc_udr_mask;
+	unsigned int udr_update;
+	/* Mask for UDR field in 'udr_update' register */
+	unsigned int udr_mask;
 };
 
 /* Register map for S5M8763 and S5M8767 */
@@ -67,8 +67,8 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
 	.ctrl			= S5M_ALARM1_CONF,
 	.alarm0			= S5M_ALARM0_SEC,
 	.alarm1			= S5M_ALARM1_SEC,
-	.rtc_udr_update		= S5M_RTC_UDR_CON,
-	.rtc_udr_mask		= S5M_RTC_UDR_MASK,
+	.udr_update		= S5M_RTC_UDR_CON,
+	.udr_mask		= S5M_RTC_UDR_MASK,
 };
 
 /*
@@ -81,8 +81,8 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
 	.ctrl			= S2MPS_RTC_CTRL,
 	.alarm0			= S2MPS_ALARM0_SEC,
 	.alarm1			= S2MPS_ALARM1_SEC,
-	.rtc_udr_update		= S2MPS_RTC_UDR_CON,
-	.rtc_udr_mask		= S2MPS_RTC_WUDR_MASK,
+	.udr_update		= S2MPS_RTC_UDR_CON,
+	.udr_mask		= S2MPS_RTC_WUDR_MASK,
 };
 
 struct s5m_rtc_info {
@@ -166,9 +166,8 @@ static inline int s5m8767_wait_for_udr_update(struct s5m_rtc_info *info)
 	unsigned int data;
 
 	do {
-		ret = regmap_read(info->regmap, info->regs->rtc_udr_update,
-				&data);
-	} while (--retry && (data & info->regs->rtc_udr_mask) && !ret);
+		ret = regmap_read(info->regmap, info->regs->udr_update, &data);
+	} while (--retry && (data & info->regs->udr_mask) && !ret);
 
 	if (!retry)
 		dev_err(info->dev, "waiting for UDR update, reached max number of retries\n");
@@ -214,7 +213,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
 	int ret;
 	unsigned int data;
 
-	ret = regmap_read(info->regmap, info->regs->rtc_udr_update, &data);
+	ret = regmap_read(info->regmap, info->regs->udr_update, &data);
 	if (ret < 0) {
 		dev_err(info->dev, "failed to read update reg(%d)\n", ret);
 		return ret;
@@ -223,21 +222,20 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
 	switch (info->device_type) {
 	case S5M8763X:
 	case S5M8767X:
-		data |= info->regs->rtc_udr_mask | S5M_RTC_TIME_EN_MASK;
+		data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
 	case S2MPS15X:
 		/* As per UM, for write time register, set WUDR bit to high */
 		data |= S2MPS15_RTC_WUDR_MASK;
 		break;
 	case S2MPS14X:
 	case S2MPS13X:
-		data |= info->regs->rtc_udr_mask;
+		data |= info->regs->udr_mask;
 		break;
 	default:
 		return -EINVAL;
 	}
 
-
-	ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
+	ret = regmap_write(info->regmap, info->regs->udr_update, data);
 	if (ret < 0) {
 		dev_err(info->dev, "failed to write update reg(%d)\n", ret);
 		return ret;
@@ -253,14 +251,14 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
 	int ret;
 	unsigned int data;
 
-	ret = regmap_read(info->regmap, info->regs->rtc_udr_update, &data);
+	ret = regmap_read(info->regmap, info->regs->udr_update, &data);
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to read update reg(%d)\n",
 			__func__, ret);
 		return ret;
 	}
 
-	data |= info->regs->rtc_udr_mask;
+	data |= info->regs->udr_mask;
 	switch (info->device_type) {
 	case S5M8763X:
 	case S5M8767X:
@@ -268,7 +266,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
 		break;
 	case S2MPS15X:
 		/* As per UM, for write alarm, set A_UDR(bit[4]) to high
-		 * rtc_udr_mask above sets bit[4]
+		 * udr_mask above sets bit[4]
 		 */
 		break;
 	case S2MPS14X:
@@ -281,7 +279,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
 		return -EINVAL;
 	}
 
-	ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
+	ret = regmap_write(info->regmap, info->regs->udr_update, data);
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to write update reg(%d)\n",
 			__func__, ret);
@@ -292,7 +290,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
 
 	/* On S2MPS13 the AUDR is not auto-cleared */
 	if (info->device_type == S2MPS13X)
-		regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
+		regmap_update_bits(info->regmap, info->regs->udr_update,
 				   S2MPS13_RTC_AUDR_MASK, 0);
 
 	return ret;
@@ -339,7 +337,7 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
 			info->device_type == S2MPS13X) {
 		ret = regmap_update_bits(info->regmap,
-				info->regs->rtc_udr_update,
+				info->regs->udr_update,
 				S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
 		if (ret) {
 			dev_err(dev,
-- 
1.9.1

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

[PATCH v2 2/3] rtc: s5m: Add separate field for storing auto-cleared mask in register config

[Krzysztof Kozlowski]

Some devices from S2M/S5M family use different register update masks for
different operations (alarm and register update). Now the driver uses
common register configuration and a lot of exceptions per device in code.

Before eliminating the exceptions and using specific register
configuration for given device, make the auto-cleared mask a separate
field. This is merely a refactoring.

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

---

Changes since v1:
1. None.
---
 drivers/rtc/rtc-s5m.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index 85649861a6b0..559db8f72117 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -56,6 +56,8 @@ struct s5m_rtc_reg_config {
 	 * auto-cleared after successful update.
 	 */
 	unsigned int udr_update;
+	/* Auto-cleared mask in UDR field for writing time and alarm */
+	unsigned int autoclear_udr_mask;
 	/* Mask for UDR field in 'udr_update' register */
 	unsigned int udr_mask;
 };
@@ -68,6 +70,7 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
 	.alarm0			= S5M_ALARM0_SEC,
 	.alarm1			= S5M_ALARM1_SEC,
 	.udr_update		= S5M_RTC_UDR_CON,
+	.autoclear_udr_mask	= S5M_RTC_UDR_MASK,
 	.udr_mask		= S5M_RTC_UDR_MASK,
 };
 
@@ -82,6 +85,7 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
 	.alarm0			= S2MPS_ALARM0_SEC,
 	.alarm1			= S2MPS_ALARM1_SEC,
 	.udr_update		= S2MPS_RTC_UDR_CON,
+	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
 	.udr_mask		= S2MPS_RTC_WUDR_MASK,
 };
 
@@ -167,7 +171,7 @@ static inline int s5m8767_wait_for_udr_update(struct s5m_rtc_info *info)
 
 	do {
 		ret = regmap_read(info->regmap, info->regs->udr_update, &data);
-	} while (--retry && (data & info->regs->udr_mask) && !ret);
+	} while (--retry && (data & info->regs->autoclear_udr_mask) && !ret);
 
 	if (!retry)
 		dev_err(info->dev, "waiting for UDR update, reached max number of retries\n");
-- 
1.9.1

[rtc-linux] [PATCH v2 2/3] rtc: s5m: Add separate field for storing auto-cleared mask in register config

[Krzysztof Kozlowski]

Some devices from S2M/S5M family use different register update masks for
different operations (alarm and register update). Now the driver uses
common register configuration and a lot of exceptions per device in code.

Before eliminating the exceptions and using specific register
configuration for given device, make the auto-cleared mask a separate
field. This is merely a refactoring.

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

---

Changes since v1:
1. None.
---
 drivers/rtc/rtc-s5m.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index 85649861a6b0..559db8f72117 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -56,6 +56,8 @@ struct s5m_rtc_reg_config {
 	 * auto-cleared after successful update.
 	 */
 	unsigned int udr_update;
+	/* Auto-cleared mask in UDR field for writing time and alarm */
+	unsigned int autoclear_udr_mask;
 	/* Mask for UDR field in 'udr_update' register */
 	unsigned int udr_mask;
 };
@@ -68,6 +70,7 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
 	.alarm0			= S5M_ALARM0_SEC,
 	.alarm1			= S5M_ALARM1_SEC,
 	.udr_update		= S5M_RTC_UDR_CON,
+	.autoclear_udr_mask	= S5M_RTC_UDR_MASK,
 	.udr_mask		= S5M_RTC_UDR_MASK,
 };
 
@@ -82,6 +85,7 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
 	.alarm0			= S2MPS_ALARM0_SEC,
 	.alarm1			= S2MPS_ALARM1_SEC,
 	.udr_update		= S2MPS_RTC_UDR_CON,
+	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
 	.udr_mask		= S2MPS_RTC_WUDR_MASK,
 };
 
@@ -167,7 +171,7 @@ static inline int s5m8767_wait_for_udr_update(struct s5m_rtc_info *info)
 
 	do {
 		ret = regmap_read(info->regmap, info->regs->udr_update, &data);
-	} while (--retry && (data & info->regs->udr_mask) && !ret);
+	} while (--retry && (data & info->regs->autoclear_udr_mask) && !ret);
 
 	if (!retry)
 		dev_err(info->dev, "waiting for UDR update, reached max number of retries\n");
-- 
1.9.1

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

[PATCH v2 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions

[Krzysztof Kozlowski]

Before updating time and alarm the driver must set appropriate mask in
UDR register. For that purpose the driver uses common register
configuration and a lot of exceptions per device in the code. The
exceptions are not obvious, for example except the change in the logic
sometimes the fields are swapped (WUDR and AUDR between S2MPS14 and
S2MPS15). This leads to quite complicated code.

Try to make it more obvious by:
1. Documenting the UDR masks for devices and operations.
2. Adding fields in register configuration structure for each operation
   (read time, write time and alarm).
3. Splitting the configuration per S2MPS13, S2MPS14 and S2MPS15 thus
   removing exceptions for them.

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

---

Tested on S2MPS11 (Odroid XU4) and S2MPS13. Testing on S2MPS15 would be
appreciated!

Changes since v1:
1. Fix value used in S2MPS15_RTC_AUDR_MASK, pointed by Yadwinder Singh
   Brar.
---
 drivers/rtc/rtc-s5m.c           | 110 +++++++++++++++++++++++++++-------------
 include/linux/mfd/samsung/rtc.h |   2 +
 2 files changed, 77 insertions(+), 35 deletions(-)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index 559db8f72117..7407d7394bb4 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -38,7 +38,22 @@
  */
 #define UDR_READ_RETRY_CNT	5
 
-/* Registers used by the driver which are different between chipsets. */
+/*
+ * Registers used by the driver which are different between chipsets.
+ *
+ * Operations like read time and write alarm/time require updating
+ * specific fields in UDR register. These fields usually are auto-cleared
+ * (with some exceptions).
+ *
+ * Table of operations per device:
+ *
+ * Device     | Write time | Read time | Write alarm
+ * =================================================
+ * S5M8767    | UDR + TIME |           | UDR
+ * S2MPS11/14 | WUDR       | RUDR      | WUDR + RUDR
+ * S2MPS13    | WUDR       | RUDR      | WUDR + AUDR
+ * S2MPS15    | WUDR       | RUDR      | AUDR
+ */
 struct s5m_rtc_reg_config {
 	/* Number of registers used for setting time/alarm0/alarm1 */
 	unsigned int regs_count;
@@ -58,8 +73,13 @@ struct s5m_rtc_reg_config {
 	unsigned int udr_update;
 	/* Auto-cleared mask in UDR field for writing time and alarm */
 	unsigned int autoclear_udr_mask;
-	/* Mask for UDR field in 'udr_update' register */
-	unsigned int udr_mask;
+	/*
+	 * Masks in UDR field for time and alarm operations.
+	 * The read time mask can be 0. Rest should not.
+	 */
+	unsigned int read_time_udr_mask;
+	unsigned int write_time_udr_mask;
+	unsigned int write_alarm_udr_mask;
 };
 
 /* Register map for S5M8763 and S5M8767 */
@@ -71,14 +91,44 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
 	.alarm1			= S5M_ALARM1_SEC,
 	.udr_update		= S5M_RTC_UDR_CON,
 	.autoclear_udr_mask	= S5M_RTC_UDR_MASK,
-	.udr_mask		= S5M_RTC_UDR_MASK,
+	.read_time_udr_mask	= 0, /* Not needed */
+	.write_time_udr_mask	= S5M_RTC_UDR_MASK | S5M_RTC_TIME_EN_MASK,
+	.write_alarm_udr_mask	= S5M_RTC_UDR_MASK,
+};
+
+/* Register map for S2MPS13 */
+static const struct s5m_rtc_reg_config s2mps13_rtc_regs = {
+	.regs_count		= 7,
+	.time			= S2MPS_RTC_SEC,
+	.ctrl			= S2MPS_RTC_CTRL,
+	.alarm0			= S2MPS_ALARM0_SEC,
+	.alarm1			= S2MPS_ALARM1_SEC,
+	.udr_update		= S2MPS_RTC_UDR_CON,
+	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
+	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
+	.write_time_udr_mask	= S2MPS_RTC_WUDR_MASK,
+	.write_alarm_udr_mask	= S2MPS_RTC_WUDR_MASK | S2MPS13_RTC_AUDR_MASK,
+};
+
+/* Register map for S2MPS11/14 */
+static const struct s5m_rtc_reg_config s2mps14_rtc_regs = {
+	.regs_count		= 7,
+	.time			= S2MPS_RTC_SEC,
+	.ctrl			= S2MPS_RTC_CTRL,
+	.alarm0			= S2MPS_ALARM0_SEC,
+	.alarm1			= S2MPS_ALARM1_SEC,
+	.udr_update		= S2MPS_RTC_UDR_CON,
+	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
+	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
+	.write_time_udr_mask	= S2MPS_RTC_WUDR_MASK,
+	.write_alarm_udr_mask	= S2MPS_RTC_WUDR_MASK | S2MPS_RTC_RUDR_MASK,
 };
 
 /*
- * Register map for S2MPS14.
- * It may be also suitable for S2MPS11 but this was not tested.
+ * Register map for S2MPS15 - in comparison to S2MPS14 the WUDR and AUDR bits
+ * are swapped.
  */
-static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
+static const struct s5m_rtc_reg_config s2mps15_rtc_regs = {
 	.regs_count		= 7,
 	.time			= S2MPS_RTC_SEC,
 	.ctrl			= S2MPS_RTC_CTRL,
@@ -86,7 +136,9 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
 	.alarm1			= S2MPS_ALARM1_SEC,
 	.udr_update		= S2MPS_RTC_UDR_CON,
 	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
-	.udr_mask		= S2MPS_RTC_WUDR_MASK,
+	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
+	.write_time_udr_mask	= S2MPS15_RTC_WUDR_MASK,
+	.write_alarm_udr_mask	= S2MPS15_RTC_AUDR_MASK,
 };
 
 struct s5m_rtc_info {
@@ -223,21 +275,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
 		return ret;
 	}
 
-	switch (info->device_type) {
-	case S5M8763X:
-	case S5M8767X:
-		data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
-	case S2MPS15X:
-		/* As per UM, for write time register, set WUDR bit to high */
-		data |= S2MPS15_RTC_WUDR_MASK;
-		break;
-	case S2MPS14X:
-	case S2MPS13X:
-		data |= info->regs->udr_mask;
-		break;
-	default:
-		return -EINVAL;
-	}
+	data |= info->regs->write_time_udr_mask;
 
 	ret = regmap_write(info->regmap, info->regs->udr_update, data);
 	if (ret < 0) {
@@ -262,22 +300,16 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
 		return ret;
 	}
 
-	data |= info->regs->udr_mask;
+	data |= info->regs->write_alarm_udr_mask;
 	switch (info->device_type) {
 	case S5M8763X:
 	case S5M8767X:
 		data &= ~S5M_RTC_TIME_EN_MASK;
 		break;
 	case S2MPS15X:
-		/* As per UM, for write alarm, set A_UDR(bit[4]) to high
-		 * udr_mask above sets bit[4]
-		 */
-		break;
 	case S2MPS14X:
-		data |= S2MPS_RTC_RUDR_MASK;
-		break;
 	case S2MPS13X:
-		data |= S2MPS13_RTC_AUDR_MASK;
+		/* No exceptions needed */
 		break;
 	default:
 		return -EINVAL;
@@ -338,11 +370,11 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	u8 data[info->regs->regs_count];
 	int ret;
 
-	if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
-			info->device_type == S2MPS13X) {
+	if (info->regs->read_time_udr_mask) {
 		ret = regmap_update_bits(info->regmap,
 				info->regs->udr_update,
-				S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
+				info->regs->read_time_udr_mask,
+				info->regs->read_time_udr_mask);
 		if (ret) {
 			dev_err(dev,
 				"Failed to prepare registers for time reading: %d\n",
@@ -709,10 +741,18 @@ static int s5m_rtc_probe(struct platform_device *pdev)
 
 	switch (platform_get_device_id(pdev)->driver_data) {
 	case S2MPS15X:
+		regmap_cfg = &s2mps14_rtc_regmap_config;
+		info->regs = &s2mps15_rtc_regs;
+		alarm_irq = S2MPS14_IRQ_RTCA0;
+		break;
 	case S2MPS14X:
+		regmap_cfg = &s2mps14_rtc_regmap_config;
+		info->regs = &s2mps14_rtc_regs;
+		alarm_irq = S2MPS14_IRQ_RTCA0;
+		break;
 	case S2MPS13X:
 		regmap_cfg = &s2mps14_rtc_regmap_config;
-		info->regs = &s2mps_rtc_regs;
+		info->regs = &s2mps13_rtc_regs;
 		alarm_irq = S2MPS14_IRQ_RTCA0;
 		break;
 	case S5M8763X:
diff --git a/include/linux/mfd/samsung/rtc.h b/include/linux/mfd/samsung/rtc.h
index a65e4655d470..48c3c5be7eb1 100644
--- a/include/linux/mfd/samsung/rtc.h
+++ b/include/linux/mfd/samsung/rtc.h
@@ -105,6 +105,8 @@ enum s2mps_rtc_reg {
 #define S5M_RTC_UDR_MASK	(1 << S5M_RTC_UDR_SHIFT)
 #define S2MPS_RTC_WUDR_SHIFT	4
 #define S2MPS_RTC_WUDR_MASK	(1 << S2MPS_RTC_WUDR_SHIFT)
+#define S2MPS15_RTC_AUDR_SHIFT	4
+#define S2MPS15_RTC_AUDR_MASK	(1 << S2MPS15_RTC_AUDR_SHIFT)
 #define S2MPS13_RTC_AUDR_SHIFT	1
 #define S2MPS13_RTC_AUDR_MASK	(1 << S2MPS13_RTC_AUDR_SHIFT)
 #define S2MPS15_RTC_WUDR_SHIFT	1
-- 
1.9.1

[rtc-linux] [PATCH v2 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions

[Krzysztof Kozlowski]

Before updating time and alarm the driver must set appropriate mask in
UDR register. For that purpose the driver uses common register
configuration and a lot of exceptions per device in the code. The
exceptions are not obvious, for example except the change in the logic
sometimes the fields are swapped (WUDR and AUDR between S2MPS14 and
S2MPS15). This leads to quite complicated code.

Try to make it more obvious by:
1. Documenting the UDR masks for devices and operations.
2. Adding fields in register configuration structure for each operation
   (read time, write time and alarm).
3. Splitting the configuration per S2MPS13, S2MPS14 and S2MPS15 thus
   removing exceptions for them.

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

---

Tested on S2MPS11 (Odroid XU4) and S2MPS13. Testing on S2MPS15 would be
appreciated!

Changes since v1:
1. Fix value used in S2MPS15_RTC_AUDR_MASK, pointed by Yadwinder Singh
   Brar.
---
 drivers/rtc/rtc-s5m.c           | 110 +++++++++++++++++++++++++++-------------
 include/linux/mfd/samsung/rtc.h |   2 +
 2 files changed, 77 insertions(+), 35 deletions(-)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index 559db8f72117..7407d7394bb4 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -38,7 +38,22 @@
  */
 #define UDR_READ_RETRY_CNT	5
 
-/* Registers used by the driver which are different between chipsets. */
+/*
+ * Registers used by the driver which are different between chipsets.
+ *
+ * Operations like read time and write alarm/time require updating
+ * specific fields in UDR register. These fields usually are auto-cleared
+ * (with some exceptions).
+ *
+ * Table of operations per device:
+ *
+ * Device     | Write time | Read time | Write alarm
+ * =================================================
+ * S5M8767    | UDR + TIME |           | UDR
+ * S2MPS11/14 | WUDR       | RUDR      | WUDR + RUDR
+ * S2MPS13    | WUDR       | RUDR      | WUDR + AUDR
+ * S2MPS15    | WUDR       | RUDR      | AUDR
+ */
 struct s5m_rtc_reg_config {
 	/* Number of registers used for setting time/alarm0/alarm1 */
 	unsigned int regs_count;
@@ -58,8 +73,13 @@ struct s5m_rtc_reg_config {
 	unsigned int udr_update;
 	/* Auto-cleared mask in UDR field for writing time and alarm */
 	unsigned int autoclear_udr_mask;
-	/* Mask for UDR field in 'udr_update' register */
-	unsigned int udr_mask;
+	/*
+	 * Masks in UDR field for time and alarm operations.
+	 * The read time mask can be 0. Rest should not.
+	 */
+	unsigned int read_time_udr_mask;
+	unsigned int write_time_udr_mask;
+	unsigned int write_alarm_udr_mask;
 };
 
 /* Register map for S5M8763 and S5M8767 */
@@ -71,14 +91,44 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
 	.alarm1			= S5M_ALARM1_SEC,
 	.udr_update		= S5M_RTC_UDR_CON,
 	.autoclear_udr_mask	= S5M_RTC_UDR_MASK,
-	.udr_mask		= S5M_RTC_UDR_MASK,
+	.read_time_udr_mask	= 0, /* Not needed */
+	.write_time_udr_mask	= S5M_RTC_UDR_MASK | S5M_RTC_TIME_EN_MASK,
+	.write_alarm_udr_mask	= S5M_RTC_UDR_MASK,
+};
+
+/* Register map for S2MPS13 */
+static const struct s5m_rtc_reg_config s2mps13_rtc_regs = {
+	.regs_count		= 7,
+	.time			= S2MPS_RTC_SEC,
+	.ctrl			= S2MPS_RTC_CTRL,
+	.alarm0			= S2MPS_ALARM0_SEC,
+	.alarm1			= S2MPS_ALARM1_SEC,
+	.udr_update		= S2MPS_RTC_UDR_CON,
+	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
+	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
+	.write_time_udr_mask	= S2MPS_RTC_WUDR_MASK,
+	.write_alarm_udr_mask	= S2MPS_RTC_WUDR_MASK | S2MPS13_RTC_AUDR_MASK,
+};
+
+/* Register map for S2MPS11/14 */
+static const struct s5m_rtc_reg_config s2mps14_rtc_regs = {
+	.regs_count		= 7,
+	.time			= S2MPS_RTC_SEC,
+	.ctrl			= S2MPS_RTC_CTRL,
+	.alarm0			= S2MPS_ALARM0_SEC,
+	.alarm1			= S2MPS_ALARM1_SEC,
+	.udr_update		= S2MPS_RTC_UDR_CON,
+	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
+	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
+	.write_time_udr_mask	= S2MPS_RTC_WUDR_MASK,
+	.write_alarm_udr_mask	= S2MPS_RTC_WUDR_MASK | S2MPS_RTC_RUDR_MASK,
 };
 
 /*
- * Register map for S2MPS14.
- * It may be also suitable for S2MPS11 but this was not tested.
+ * Register map for S2MPS15 - in comparison to S2MPS14 the WUDR and AUDR bits
+ * are swapped.
  */
-static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
+static const struct s5m_rtc_reg_config s2mps15_rtc_regs = {
 	.regs_count		= 7,
 	.time			= S2MPS_RTC_SEC,
 	.ctrl			= S2MPS_RTC_CTRL,
@@ -86,7 +136,9 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
 	.alarm1			= S2MPS_ALARM1_SEC,
 	.udr_update		= S2MPS_RTC_UDR_CON,
 	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
-	.udr_mask		= S2MPS_RTC_WUDR_MASK,
+	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
+	.write_time_udr_mask	= S2MPS15_RTC_WUDR_MASK,
+	.write_alarm_udr_mask	= S2MPS15_RTC_AUDR_MASK,
 };
 
 struct s5m_rtc_info {
@@ -223,21 +275,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
 		return ret;
 	}
 
-	switch (info->device_type) {
-	case S5M8763X:
-	case S5M8767X:
-		data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
-	case S2MPS15X:
-		/* As per UM, for write time register, set WUDR bit to high */
-		data |= S2MPS15_RTC_WUDR_MASK;
-		break;
-	case S2MPS14X:
-	case S2MPS13X:
-		data |= info->regs->udr_mask;
-		break;
-	default:
-		return -EINVAL;
-	}
+	data |= info->regs->write_time_udr_mask;
 
 	ret = regmap_write(info->regmap, info->regs->udr_update, data);
 	if (ret < 0) {
@@ -262,22 +300,16 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
 		return ret;
 	}
 
-	data |= info->regs->udr_mask;
+	data |= info->regs->write_alarm_udr_mask;
 	switch (info->device_type) {
 	case S5M8763X:
 	case S5M8767X:
 		data &= ~S5M_RTC_TIME_EN_MASK;
 		break;
 	case S2MPS15X:
-		/* As per UM, for write alarm, set A_UDR(bit[4]) to high
-		 * udr_mask above sets bit[4]
-		 */
-		break;
 	case S2MPS14X:
-		data |= S2MPS_RTC_RUDR_MASK;
-		break;
 	case S2MPS13X:
-		data |= S2MPS13_RTC_AUDR_MASK;
+		/* No exceptions needed */
 		break;
 	default:
 		return -EINVAL;
@@ -338,11 +370,11 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	u8 data[info->regs->regs_count];
 	int ret;
 
-	if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
-			info->device_type == S2MPS13X) {
+	if (info->regs->read_time_udr_mask) {
 		ret = regmap_update_bits(info->regmap,
 				info->regs->udr_update,
-				S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
+				info->regs->read_time_udr_mask,
+				info->regs->read_time_udr_mask);
 		if (ret) {
 			dev_err(dev,
 				"Failed to prepare registers for time reading: %d\n",
@@ -709,10 +741,18 @@ static int s5m_rtc_probe(struct platform_device *pdev)
 
 	switch (platform_get_device_id(pdev)->driver_data) {
 	case S2MPS15X:
+		regmap_cfg = &s2mps14_rtc_regmap_config;
+		info->regs = &s2mps15_rtc_regs;
+		alarm_irq = S2MPS14_IRQ_RTCA0;
+		break;
 	case S2MPS14X:
+		regmap_cfg = &s2mps14_rtc_regmap_config;
+		info->regs = &s2mps14_rtc_regs;
+		alarm_irq = S2MPS14_IRQ_RTCA0;
+		break;
 	case S2MPS13X:
 		regmap_cfg = &s2mps14_rtc_regmap_config;
-		info->regs = &s2mps_rtc_regs;
+		info->regs = &s2mps13_rtc_regs;
 		alarm_irq = S2MPS14_IRQ_RTCA0;
 		break;
 	case S5M8763X:
diff --git a/include/linux/mfd/samsung/rtc.h b/include/linux/mfd/samsung/rtc.h
index a65e4655d470..48c3c5be7eb1 100644
--- a/include/linux/mfd/samsung/rtc.h
+++ b/include/linux/mfd/samsung/rtc.h
@@ -105,6 +105,8 @@ enum s2mps_rtc_reg {
 #define S5M_RTC_UDR_MASK	(1 << S5M_RTC_UDR_SHIFT)
 #define S2MPS_RTC_WUDR_SHIFT	4
 #define S2MPS_RTC_WUDR_MASK	(1 << S2MPS_RTC_WUDR_SHIFT)
+#define S2MPS15_RTC_AUDR_SHIFT	4
+#define S2MPS15_RTC_AUDR_MASK	(1 << S2MPS15_RTC_AUDR_SHIFT)
 #define S2MPS13_RTC_AUDR_SHIFT	1
 #define S2MPS13_RTC_AUDR_MASK	(1 << S2MPS13_RTC_AUDR_SHIFT)
 #define S2MPS15_RTC_WUDR_SHIFT	1
-- 
1.9.1

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

Re: [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Alim Akhtar]

Hi Krzysztof,

On 12/30/2015 10:17 AM, Krzysztof Kozlowski wrote:
> Remove the 'rtc' prefix from some of the fields in struct
> s5m_rtc_reg_config because it is obvious - this is a RTC driver. No
> functional changes.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>
Looks good.
Tested for s2mps15 pmic on exynos7-espresso board. Works well as before.
Fill free to add
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Tested-by: Alim Akhtar <alim.akhtar@samsung.com>

> ---
>
> Changes since v1:
> 1. None.
> ---
>   drivers/rtc/rtc-s5m.c | 40 +++++++++++++++++++---------------------
>   1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index 0d68a85dd429..85649861a6b0 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -55,9 +55,9 @@ struct s5m_rtc_reg_config {
>   	 * will enable update of time or alarm register. Then it will be
>   	 * auto-cleared after successful update.
>   	 */
> -	unsigned int rtc_udr_update;
> -	/* Mask for UDR field in 'rtc_udr_update' register */
> -	unsigned int rtc_udr_mask;
> +	unsigned int udr_update;
> +	/* Mask for UDR field in 'udr_update' register */
> +	unsigned int udr_mask;
>   };
>
>   /* Register map for S5M8763 and S5M8767 */
> @@ -67,8 +67,8 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
>   	.ctrl			= S5M_ALARM1_CONF,
>   	.alarm0			= S5M_ALARM0_SEC,
>   	.alarm1			= S5M_ALARM1_SEC,
> -	.rtc_udr_update		= S5M_RTC_UDR_CON,
> -	.rtc_udr_mask		= S5M_RTC_UDR_MASK,
> +	.udr_update		= S5M_RTC_UDR_CON,
> +	.udr_mask		= S5M_RTC_UDR_MASK,
>   };
>
>   /*
> @@ -81,8 +81,8 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
>   	.ctrl			= S2MPS_RTC_CTRL,
>   	.alarm0			= S2MPS_ALARM0_SEC,
>   	.alarm1			= S2MPS_ALARM1_SEC,
> -	.rtc_udr_update		= S2MPS_RTC_UDR_CON,
> -	.rtc_udr_mask		= S2MPS_RTC_WUDR_MASK,
> +	.udr_update		= S2MPS_RTC_UDR_CON,
> +	.udr_mask		= S2MPS_RTC_WUDR_MASK,
>   };
>
>   struct s5m_rtc_info {
> @@ -166,9 +166,8 @@ static inline int s5m8767_wait_for_udr_update(struct s5m_rtc_info *info)
>   	unsigned int data;
>
>   	do {
> -		ret = regmap_read(info->regmap, info->regs->rtc_udr_update,
> -				&data);
> -	} while (--retry && (data & info->regs->rtc_udr_mask) && !ret);
> +		ret = regmap_read(info->regmap, info->regs->udr_update, &data);
> +	} while (--retry && (data & info->regs->udr_mask) && !ret);
>
>   	if (!retry)
>   		dev_err(info->dev, "waiting for UDR update, reached max number of retries\n");
> @@ -214,7 +213,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
>   	int ret;
>   	unsigned int data;
>
> -	ret = regmap_read(info->regmap, info->regs->rtc_udr_update, &data);
> +	ret = regmap_read(info->regmap, info->regs->udr_update, &data);
>   	if (ret < 0) {
>   		dev_err(info->dev, "failed to read update reg(%d)\n", ret);
>   		return ret;
> @@ -223,21 +222,20 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
>   	switch (info->device_type) {
>   	case S5M8763X:
>   	case S5M8767X:
> -		data |= info->regs->rtc_udr_mask | S5M_RTC_TIME_EN_MASK;
> +		data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
>   	case S2MPS15X:
>   		/* As per UM, for write time register, set WUDR bit to high */
>   		data |= S2MPS15_RTC_WUDR_MASK;
>   		break;
>   	case S2MPS14X:
>   	case S2MPS13X:
> -		data |= info->regs->rtc_udr_mask;
> +		data |= info->regs->udr_mask;
>   		break;
>   	default:
>   		return -EINVAL;
>   	}
>
> -
> -	ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
> +	ret = regmap_write(info->regmap, info->regs->udr_update, data);
>   	if (ret < 0) {
>   		dev_err(info->dev, "failed to write update reg(%d)\n", ret);
>   		return ret;
> @@ -253,14 +251,14 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>   	int ret;
>   	unsigned int data;
>
> -	ret = regmap_read(info->regmap, info->regs->rtc_udr_update, &data);
> +	ret = regmap_read(info->regmap, info->regs->udr_update, &data);
>   	if (ret < 0) {
>   		dev_err(info->dev, "%s: fail to read update reg(%d)\n",
>   			__func__, ret);
>   		return ret;
>   	}
>
> -	data |= info->regs->rtc_udr_mask;
> +	data |= info->regs->udr_mask;
>   	switch (info->device_type) {
>   	case S5M8763X:
>   	case S5M8767X:
> @@ -268,7 +266,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>   		break;
>   	case S2MPS15X:
>   		/* As per UM, for write alarm, set A_UDR(bit[4]) to high
> -		 * rtc_udr_mask above sets bit[4]
> +		 * udr_mask above sets bit[4]
>   		 */
>   		break;
>   	case S2MPS14X:
> @@ -281,7 +279,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>   		return -EINVAL;
>   	}
>
> -	ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
> +	ret = regmap_write(info->regmap, info->regs->udr_update, data);
>   	if (ret < 0) {
>   		dev_err(info->dev, "%s: fail to write update reg(%d)\n",
>   			__func__, ret);
> @@ -292,7 +290,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>
>   	/* On S2MPS13 the AUDR is not auto-cleared */
>   	if (info->device_type == S2MPS13X)
> -		regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
> +		regmap_update_bits(info->regmap, info->regs->udr_update,
>   				   S2MPS13_RTC_AUDR_MASK, 0);
>
>   	return ret;
> @@ -339,7 +337,7 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm)
>   	if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
>   			info->device_type == S2MPS13X) {
>   		ret = regmap_update_bits(info->regmap,
> -				info->regs->rtc_udr_update,
> +				info->regs->udr_update,
>   				S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
>   		if (ret) {
>   			dev_err(dev,
>

[rtc-linux] Re: [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Alim Akhtar]

Hi Krzysztof,

On 12/30/2015 10:17 AM, Krzysztof Kozlowski wrote:
> Remove the 'rtc' prefix from some of the fields in struct
> s5m_rtc_reg_config because it is obvious - this is a RTC driver. No
> functional changes.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>
Looks good.
Tested for s2mps15 pmic on exynos7-espresso board. Works well as before.
Fill free to add
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Tested-by: Alim Akhtar <alim.akhtar@samsung.com>

> ---
>
> Changes since v1:
> 1. None.
> ---
>   drivers/rtc/rtc-s5m.c | 40 +++++++++++++++++++---------------------
>   1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index 0d68a85dd429..85649861a6b0 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -55,9 +55,9 @@ struct s5m_rtc_reg_config {
>   	 * will enable update of time or alarm register. Then it will be
>   	 * auto-cleared after successful update.
>   	 */
> -	unsigned int rtc_udr_update;
> -	/* Mask for UDR field in 'rtc_udr_update' register */
> -	unsigned int rtc_udr_mask;
> +	unsigned int udr_update;
> +	/* Mask for UDR field in 'udr_update' register */
> +	unsigned int udr_mask;
>   };
>
>   /* Register map for S5M8763 and S5M8767 */
> @@ -67,8 +67,8 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
>   	.ctrl			= S5M_ALARM1_CONF,
>   	.alarm0			= S5M_ALARM0_SEC,
>   	.alarm1			= S5M_ALARM1_SEC,
> -	.rtc_udr_update		= S5M_RTC_UDR_CON,
> -	.rtc_udr_mask		= S5M_RTC_UDR_MASK,
> +	.udr_update		= S5M_RTC_UDR_CON,
> +	.udr_mask		= S5M_RTC_UDR_MASK,
>   };
>
>   /*
> @@ -81,8 +81,8 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
>   	.ctrl			= S2MPS_RTC_CTRL,
>   	.alarm0			= S2MPS_ALARM0_SEC,
>   	.alarm1			= S2MPS_ALARM1_SEC,
> -	.rtc_udr_update		= S2MPS_RTC_UDR_CON,
> -	.rtc_udr_mask		= S2MPS_RTC_WUDR_MASK,
> +	.udr_update		= S2MPS_RTC_UDR_CON,
> +	.udr_mask		= S2MPS_RTC_WUDR_MASK,
>   };
>
>   struct s5m_rtc_info {
> @@ -166,9 +166,8 @@ static inline int s5m8767_wait_for_udr_update(struct s5m_rtc_info *info)
>   	unsigned int data;
>
>   	do {
> -		ret = regmap_read(info->regmap, info->regs->rtc_udr_update,
> -				&data);
> -	} while (--retry && (data & info->regs->rtc_udr_mask) && !ret);
> +		ret = regmap_read(info->regmap, info->regs->udr_update, &data);
> +	} while (--retry && (data & info->regs->udr_mask) && !ret);
>
>   	if (!retry)
>   		dev_err(info->dev, "waiting for UDR update, reached max number of retries\n");
> @@ -214,7 +213,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
>   	int ret;
>   	unsigned int data;
>
> -	ret = regmap_read(info->regmap, info->regs->rtc_udr_update, &data);
> +	ret = regmap_read(info->regmap, info->regs->udr_update, &data);
>   	if (ret < 0) {
>   		dev_err(info->dev, "failed to read update reg(%d)\n", ret);
>   		return ret;
> @@ -223,21 +222,20 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
>   	switch (info->device_type) {
>   	case S5M8763X:
>   	case S5M8767X:
> -		data |= info->regs->rtc_udr_mask | S5M_RTC_TIME_EN_MASK;
> +		data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
>   	case S2MPS15X:
>   		/* As per UM, for write time register, set WUDR bit to high */
>   		data |= S2MPS15_RTC_WUDR_MASK;
>   		break;
>   	case S2MPS14X:
>   	case S2MPS13X:
> -		data |= info->regs->rtc_udr_mask;
> +		data |= info->regs->udr_mask;
>   		break;
>   	default:
>   		return -EINVAL;
>   	}
>
> -
> -	ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
> +	ret = regmap_write(info->regmap, info->regs->udr_update, data);
>   	if (ret < 0) {
>   		dev_err(info->dev, "failed to write update reg(%d)\n", ret);
>   		return ret;
> @@ -253,14 +251,14 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>   	int ret;
>   	unsigned int data;
>
> -	ret = regmap_read(info->regmap, info->regs->rtc_udr_update, &data);
> +	ret = regmap_read(info->regmap, info->regs->udr_update, &data);
>   	if (ret < 0) {
>   		dev_err(info->dev, "%s: fail to read update reg(%d)\n",
>   			__func__, ret);
>   		return ret;
>   	}
>
> -	data |= info->regs->rtc_udr_mask;
> +	data |= info->regs->udr_mask;
>   	switch (info->device_type) {
>   	case S5M8763X:
>   	case S5M8767X:
> @@ -268,7 +266,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>   		break;
>   	case S2MPS15X:
>   		/* As per UM, for write alarm, set A_UDR(bit[4]) to high
> -		 * rtc_udr_mask above sets bit[4]
> +		 * udr_mask above sets bit[4]
>   		 */
>   		break;
>   	case S2MPS14X:
> @@ -281,7 +279,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>   		return -EINVAL;
>   	}
>
> -	ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
> +	ret = regmap_write(info->regmap, info->regs->udr_update, data);
>   	if (ret < 0) {
>   		dev_err(info->dev, "%s: fail to write update reg(%d)\n",
>   			__func__, ret);
> @@ -292,7 +290,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>
>   	/* On S2MPS13 the AUDR is not auto-cleared */
>   	if (info->device_type == S2MPS13X)
> -		regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
> +		regmap_update_bits(info->regmap, info->regs->udr_update,
>   				   S2MPS13_RTC_AUDR_MASK, 0);
>
>   	return ret;
> @@ -339,7 +337,7 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm)
>   	if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
>   			info->device_type == S2MPS13X) {
>   		ret = regmap_update_bits(info->regmap,
> -				info->regs->rtc_udr_update,
> +				info->regs->udr_update,
>   				S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
>   		if (ret) {
>   			dev_err(dev,
>

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

Re: [PATCH v2 2/3] rtc: s5m: Add separate field for storing auto-cleared mask in register config

[Alim Akhtar]

Hi Krzysztof,


On 12/30/2015 10:17 AM, Krzysztof Kozlowski wrote:
> Some devices from S2M/S5M family use different register update masks for
> different operations (alarm and register update). Now the driver uses
> common register configuration and a lot of exceptions per device in code.
>
> Before eliminating the exceptions and using specific register
> configuration for given device, make the auto-cleared mask a separate
> field. This is merely a refactoring.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>

Tested for s2mps15 pmic on exynos7-espresso board. Works well.
Fill free to add
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Tested-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>
> Changes since v1:
> 1. None.
> ---
>   drivers/rtc/rtc-s5m.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index 85649861a6b0..559db8f72117 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -56,6 +56,8 @@ struct s5m_rtc_reg_config {
>   	 * auto-cleared after successful update.
>   	 */
>   	unsigned int udr_update;
> +	/* Auto-cleared mask in UDR field for writing time and alarm */
> +	unsigned int autoclear_udr_mask;
>   	/* Mask for UDR field in 'udr_update' register */
>   	unsigned int udr_mask;
>   };
> @@ -68,6 +70,7 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
>   	.alarm0			= S5M_ALARM0_SEC,
>   	.alarm1			= S5M_ALARM1_SEC,
>   	.udr_update		= S5M_RTC_UDR_CON,
> +	.autoclear_udr_mask	= S5M_RTC_UDR_MASK,
>   	.udr_mask		= S5M_RTC_UDR_MASK,
>   };
>
> @@ -82,6 +85,7 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
>   	.alarm0			= S2MPS_ALARM0_SEC,
>   	.alarm1			= S2MPS_ALARM1_SEC,
>   	.udr_update		= S2MPS_RTC_UDR_CON,
> +	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
>   	.udr_mask		= S2MPS_RTC_WUDR_MASK,
>   };
>
> @@ -167,7 +171,7 @@ static inline int s5m8767_wait_for_udr_update(struct s5m_rtc_info *info)
>
>   	do {
>   		ret = regmap_read(info->regmap, info->regs->udr_update, &data);
> -	} while (--retry && (data & info->regs->udr_mask) && !ret);
> +	} while (--retry && (data & info->regs->autoclear_udr_mask) && !ret);
>
>   	if (!retry)
>   		dev_err(info->dev, "waiting for UDR update, reached max number of retries\n");
>

[rtc-linux] Re: [PATCH v2 2/3] rtc: s5m: Add separate field for storing auto-cleared mask in register config

[Alim Akhtar]

Hi Krzysztof,


On 12/30/2015 10:17 AM, Krzysztof Kozlowski wrote:
> Some devices from S2M/S5M family use different register update masks for
> different operations (alarm and register update). Now the driver uses
> common register configuration and a lot of exceptions per device in code.
>
> Before eliminating the exceptions and using specific register
> configuration for given device, make the auto-cleared mask a separate
> field. This is merely a refactoring.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>

Tested for s2mps15 pmic on exynos7-espresso board. Works well.
Fill free to add
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Tested-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>
> Changes since v1:
> 1. None.
> ---
>   drivers/rtc/rtc-s5m.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index 85649861a6b0..559db8f72117 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -56,6 +56,8 @@ struct s5m_rtc_reg_config {
>   	 * auto-cleared after successful update.
>   	 */
>   	unsigned int udr_update;
> +	/* Auto-cleared mask in UDR field for writing time and alarm */
> +	unsigned int autoclear_udr_mask;
>   	/* Mask for UDR field in 'udr_update' register */
>   	unsigned int udr_mask;
>   };
> @@ -68,6 +70,7 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
>   	.alarm0			= S5M_ALARM0_SEC,
>   	.alarm1			= S5M_ALARM1_SEC,
>   	.udr_update		= S5M_RTC_UDR_CON,
> +	.autoclear_udr_mask	= S5M_RTC_UDR_MASK,
>   	.udr_mask		= S5M_RTC_UDR_MASK,
>   };
>
> @@ -82,6 +85,7 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
>   	.alarm0			= S2MPS_ALARM0_SEC,
>   	.alarm1			= S2MPS_ALARM1_SEC,
>   	.udr_update		= S2MPS_RTC_UDR_CON,
> +	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
>   	.udr_mask		= S2MPS_RTC_WUDR_MASK,
>   };
>
> @@ -167,7 +171,7 @@ static inline int s5m8767_wait_for_udr_update(struct s5m_rtc_info *info)
>
>   	do {
>   		ret = regmap_read(info->regmap, info->regs->udr_update, &data);
> -	} while (--retry && (data & info->regs->udr_mask) && !ret);
> +	} while (--retry && (data & info->regs->autoclear_udr_mask) && !ret);
>
>   	if (!retry)
>   		dev_err(info->dev, "waiting for UDR update, reached max number of retries\n");
>

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

Re: [PATCH v2 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions

[Alim Akhtar]

Hi Krzysztof,

On 12/30/2015 10:17 AM, Krzysztof Kozlowski wrote:
> Before updating time and alarm the driver must set appropriate mask in
> UDR register. For that purpose the driver uses common register
> configuration and a lot of exceptions per device in the code. The
> exceptions are not obvious, for example except the change in the logic
> sometimes the fields are swapped (WUDR and AUDR between S2MPS14 and
> S2MPS15). This leads to quite complicated code.
>
> Try to make it more obvious by:
> 1. Documenting the UDR masks for devices and operations.
> 2. Adding fields in register configuration structure for each operation
>     (read time, write time and alarm).
> 3. Splitting the configuration per S2MPS13, S2MPS14 and S2MPS15 thus
>     removing exceptions for them.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>
Tested for s2mps15 pmic on exynos7-espresso board. rtc and timer works 
as expected.
Fill free to add
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Tested-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>
> Tested on S2MPS11 (Odroid XU4) and S2MPS13. Testing on S2MPS15 would be
> appreciated!
>
> Changes since v1:
> 1. Fix value used in S2MPS15_RTC_AUDR_MASK, pointed by Yadwinder Singh
>     Brar.
> ---
>   drivers/rtc/rtc-s5m.c           | 110 +++++++++++++++++++++++++++-------------
>   include/linux/mfd/samsung/rtc.h |   2 +
>   2 files changed, 77 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index 559db8f72117..7407d7394bb4 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -38,7 +38,22 @@
>    */
>   #define UDR_READ_RETRY_CNT	5
>
> -/* Registers used by the driver which are different between chipsets. */
> +/*
> + * Registers used by the driver which are different between chipsets.
> + *
> + * Operations like read time and write alarm/time require updating
> + * specific fields in UDR register. These fields usually are auto-cleared
> + * (with some exceptions).
> + *
> + * Table of operations per device:
> + *
> + * Device     | Write time | Read time | Write alarm
> + * =================================================
> + * S5M8767    | UDR + TIME |           | UDR
> + * S2MPS11/14 | WUDR       | RUDR      | WUDR + RUDR
> + * S2MPS13    | WUDR       | RUDR      | WUDR + AUDR
> + * S2MPS15    | WUDR       | RUDR      | AUDR
> + */
>   struct s5m_rtc_reg_config {
>   	/* Number of registers used for setting time/alarm0/alarm1 */
>   	unsigned int regs_count;
> @@ -58,8 +73,13 @@ struct s5m_rtc_reg_config {
>   	unsigned int udr_update;
>   	/* Auto-cleared mask in UDR field for writing time and alarm */
>   	unsigned int autoclear_udr_mask;
> -	/* Mask for UDR field in 'udr_update' register */
> -	unsigned int udr_mask;
> +	/*
> +	 * Masks in UDR field for time and alarm operations.
> +	 * The read time mask can be 0. Rest should not.
> +	 */
> +	unsigned int read_time_udr_mask;
> +	unsigned int write_time_udr_mask;
> +	unsigned int write_alarm_udr_mask;
>   };
>
>   /* Register map for S5M8763 and S5M8767 */
> @@ -71,14 +91,44 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
>   	.alarm1			= S5M_ALARM1_SEC,
>   	.udr_update		= S5M_RTC_UDR_CON,
>   	.autoclear_udr_mask	= S5M_RTC_UDR_MASK,
> -	.udr_mask		= S5M_RTC_UDR_MASK,
> +	.read_time_udr_mask	= 0, /* Not needed */
> +	.write_time_udr_mask	= S5M_RTC_UDR_MASK | S5M_RTC_TIME_EN_MASK,
> +	.write_alarm_udr_mask	= S5M_RTC_UDR_MASK,
> +};
> +
> +/* Register map for S2MPS13 */
> +static const struct s5m_rtc_reg_config s2mps13_rtc_regs = {
> +	.regs_count		= 7,
> +	.time			= S2MPS_RTC_SEC,
> +	.ctrl			= S2MPS_RTC_CTRL,
> +	.alarm0			= S2MPS_ALARM0_SEC,
> +	.alarm1			= S2MPS_ALARM1_SEC,
> +	.udr_update		= S2MPS_RTC_UDR_CON,
> +	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
> +	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
> +	.write_time_udr_mask	= S2MPS_RTC_WUDR_MASK,
> +	.write_alarm_udr_mask	= S2MPS_RTC_WUDR_MASK | S2MPS13_RTC_AUDR_MASK,
> +};
> +
> +/* Register map for S2MPS11/14 */
> +static const struct s5m_rtc_reg_config s2mps14_rtc_regs = {
> +	.regs_count		= 7,
> +	.time			= S2MPS_RTC_SEC,
> +	.ctrl			= S2MPS_RTC_CTRL,
> +	.alarm0			= S2MPS_ALARM0_SEC,
> +	.alarm1			= S2MPS_ALARM1_SEC,
> +	.udr_update		= S2MPS_RTC_UDR_CON,
> +	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
> +	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
> +	.write_time_udr_mask	= S2MPS_RTC_WUDR_MASK,
> +	.write_alarm_udr_mask	= S2MPS_RTC_WUDR_MASK | S2MPS_RTC_RUDR_MASK,
>   };
>
>   /*
> - * Register map for S2MPS14.
> - * It may be also suitable for S2MPS11 but this was not tested.
> + * Register map for S2MPS15 - in comparison to S2MPS14 the WUDR and AUDR bits
> + * are swapped.
>    */
> -static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
> +static const struct s5m_rtc_reg_config s2mps15_rtc_regs = {
>   	.regs_count		= 7,
>   	.time			= S2MPS_RTC_SEC,
>   	.ctrl			= S2MPS_RTC_CTRL,
> @@ -86,7 +136,9 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
>   	.alarm1			= S2MPS_ALARM1_SEC,
>   	.udr_update		= S2MPS_RTC_UDR_CON,
>   	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
> -	.udr_mask		= S2MPS_RTC_WUDR_MASK,
> +	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
> +	.write_time_udr_mask	= S2MPS15_RTC_WUDR_MASK,
> +	.write_alarm_udr_mask	= S2MPS15_RTC_AUDR_MASK,
>   };
>
>   struct s5m_rtc_info {
> @@ -223,21 +275,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
>   		return ret;
>   	}
>
> -	switch (info->device_type) {
> -	case S5M8763X:
> -	case S5M8767X:
> -		data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
> -	case S2MPS15X:
> -		/* As per UM, for write time register, set WUDR bit to high */
> -		data |= S2MPS15_RTC_WUDR_MASK;
> -		break;
> -	case S2MPS14X:
> -	case S2MPS13X:
> -		data |= info->regs->udr_mask;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +	data |= info->regs->write_time_udr_mask;
>
>   	ret = regmap_write(info->regmap, info->regs->udr_update, data);
>   	if (ret < 0) {
> @@ -262,22 +300,16 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>   		return ret;
>   	}
>
> -	data |= info->regs->udr_mask;
> +	data |= info->regs->write_alarm_udr_mask;
>   	switch (info->device_type) {
>   	case S5M8763X:
>   	case S5M8767X:
>   		data &= ~S5M_RTC_TIME_EN_MASK;
>   		break;
>   	case S2MPS15X:
> -		/* As per UM, for write alarm, set A_UDR(bit[4]) to high
> -		 * udr_mask above sets bit[4]
> -		 */
> -		break;
>   	case S2MPS14X:
> -		data |= S2MPS_RTC_RUDR_MASK;
> -		break;
>   	case S2MPS13X:
> -		data |= S2MPS13_RTC_AUDR_MASK;
> +		/* No exceptions needed */
>   		break;
>   	default:
>   		return -EINVAL;
> @@ -338,11 +370,11 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm)
>   	u8 data[info->regs->regs_count];
>   	int ret;
>
> -	if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
> -			info->device_type == S2MPS13X) {
> +	if (info->regs->read_time_udr_mask) {
>   		ret = regmap_update_bits(info->regmap,
>   				info->regs->udr_update,
> -				S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
> +				info->regs->read_time_udr_mask,
> +				info->regs->read_time_udr_mask);
>   		if (ret) {
>   			dev_err(dev,
>   				"Failed to prepare registers for time reading: %d\n",
> @@ -709,10 +741,18 @@ static int s5m_rtc_probe(struct platform_device *pdev)
>
>   	switch (platform_get_device_id(pdev)->driver_data) {
>   	case S2MPS15X:
> +		regmap_cfg = &s2mps14_rtc_regmap_config;
> +		info->regs = &s2mps15_rtc_regs;
> +		alarm_irq = S2MPS14_IRQ_RTCA0;
> +		break;
>   	case S2MPS14X:
> +		regmap_cfg = &s2mps14_rtc_regmap_config;
> +		info->regs = &s2mps14_rtc_regs;
> +		alarm_irq = S2MPS14_IRQ_RTCA0;
> +		break;
>   	case S2MPS13X:
>   		regmap_cfg = &s2mps14_rtc_regmap_config;
> -		info->regs = &s2mps_rtc_regs;
> +		info->regs = &s2mps13_rtc_regs;
>   		alarm_irq = S2MPS14_IRQ_RTCA0;
>   		break;
>   	case S5M8763X:
> diff --git a/include/linux/mfd/samsung/rtc.h b/include/linux/mfd/samsung/rtc.h
> index a65e4655d470..48c3c5be7eb1 100644
> --- a/include/linux/mfd/samsung/rtc.h
> +++ b/include/linux/mfd/samsung/rtc.h
> @@ -105,6 +105,8 @@ enum s2mps_rtc_reg {
>   #define S5M_RTC_UDR_MASK	(1 << S5M_RTC_UDR_SHIFT)
>   #define S2MPS_RTC_WUDR_SHIFT	4
>   #define S2MPS_RTC_WUDR_MASK	(1 << S2MPS_RTC_WUDR_SHIFT)
> +#define S2MPS15_RTC_AUDR_SHIFT	4
> +#define S2MPS15_RTC_AUDR_MASK	(1 << S2MPS15_RTC_AUDR_SHIFT)
>   #define S2MPS13_RTC_AUDR_SHIFT	1
>   #define S2MPS13_RTC_AUDR_MASK	(1 << S2MPS13_RTC_AUDR_SHIFT)
>   #define S2MPS15_RTC_WUDR_SHIFT	1
>

[rtc-linux] Re: [PATCH v2 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions

[Alim Akhtar]

Hi Krzysztof,

On 12/30/2015 10:17 AM, Krzysztof Kozlowski wrote:
> Before updating time and alarm the driver must set appropriate mask in
> UDR register. For that purpose the driver uses common register
> configuration and a lot of exceptions per device in the code. The
> exceptions are not obvious, for example except the change in the logic
> sometimes the fields are swapped (WUDR and AUDR between S2MPS14 and
> S2MPS15). This leads to quite complicated code.
>
> Try to make it more obvious by:
> 1. Documenting the UDR masks for devices and operations.
> 2. Adding fields in register configuration structure for each operation
>     (read time, write time and alarm).
> 3. Splitting the configuration per S2MPS13, S2MPS14 and S2MPS15 thus
>     removing exceptions for them.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>
Tested for s2mps15 pmic on exynos7-espresso board. rtc and timer works 
as expected.
Fill free to add
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Tested-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>
> Tested on S2MPS11 (Odroid XU4) and S2MPS13. Testing on S2MPS15 would be
> appreciated!
>
> Changes since v1:
> 1. Fix value used in S2MPS15_RTC_AUDR_MASK, pointed by Yadwinder Singh
>     Brar.
> ---
>   drivers/rtc/rtc-s5m.c           | 110 +++++++++++++++++++++++++++-------------
>   include/linux/mfd/samsung/rtc.h |   2 +
>   2 files changed, 77 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index 559db8f72117..7407d7394bb4 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -38,7 +38,22 @@
>    */
>   #define UDR_READ_RETRY_CNT	5
>
> -/* Registers used by the driver which are different between chipsets. */
> +/*
> + * Registers used by the driver which are different between chipsets.
> + *
> + * Operations like read time and write alarm/time require updating
> + * specific fields in UDR register. These fields usually are auto-cleared
> + * (with some exceptions).
> + *
> + * Table of operations per device:
> + *
> + * Device     | Write time | Read time | Write alarm
> + * =================================================
> + * S5M8767    | UDR + TIME |           | UDR
> + * S2MPS11/14 | WUDR       | RUDR      | WUDR + RUDR
> + * S2MPS13    | WUDR       | RUDR      | WUDR + AUDR
> + * S2MPS15    | WUDR       | RUDR      | AUDR
> + */
>   struct s5m_rtc_reg_config {
>   	/* Number of registers used for setting time/alarm0/alarm1 */
>   	unsigned int regs_count;
> @@ -58,8 +73,13 @@ struct s5m_rtc_reg_config {
>   	unsigned int udr_update;
>   	/* Auto-cleared mask in UDR field for writing time and alarm */
>   	unsigned int autoclear_udr_mask;
> -	/* Mask for UDR field in 'udr_update' register */
> -	unsigned int udr_mask;
> +	/*
> +	 * Masks in UDR field for time and alarm operations.
> +	 * The read time mask can be 0. Rest should not.
> +	 */
> +	unsigned int read_time_udr_mask;
> +	unsigned int write_time_udr_mask;
> +	unsigned int write_alarm_udr_mask;
>   };
>
>   /* Register map for S5M8763 and S5M8767 */
> @@ -71,14 +91,44 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
>   	.alarm1			= S5M_ALARM1_SEC,
>   	.udr_update		= S5M_RTC_UDR_CON,
>   	.autoclear_udr_mask	= S5M_RTC_UDR_MASK,
> -	.udr_mask		= S5M_RTC_UDR_MASK,
> +	.read_time_udr_mask	= 0, /* Not needed */
> +	.write_time_udr_mask	= S5M_RTC_UDR_MASK | S5M_RTC_TIME_EN_MASK,
> +	.write_alarm_udr_mask	= S5M_RTC_UDR_MASK,
> +};
> +
> +/* Register map for S2MPS13 */
> +static const struct s5m_rtc_reg_config s2mps13_rtc_regs = {
> +	.regs_count		= 7,
> +	.time			= S2MPS_RTC_SEC,
> +	.ctrl			= S2MPS_RTC_CTRL,
> +	.alarm0			= S2MPS_ALARM0_SEC,
> +	.alarm1			= S2MPS_ALARM1_SEC,
> +	.udr_update		= S2MPS_RTC_UDR_CON,
> +	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
> +	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
> +	.write_time_udr_mask	= S2MPS_RTC_WUDR_MASK,
> +	.write_alarm_udr_mask	= S2MPS_RTC_WUDR_MASK | S2MPS13_RTC_AUDR_MASK,
> +};
> +
> +/* Register map for S2MPS11/14 */
> +static const struct s5m_rtc_reg_config s2mps14_rtc_regs = {
> +	.regs_count		= 7,
> +	.time			= S2MPS_RTC_SEC,
> +	.ctrl			= S2MPS_RTC_CTRL,
> +	.alarm0			= S2MPS_ALARM0_SEC,
> +	.alarm1			= S2MPS_ALARM1_SEC,
> +	.udr_update		= S2MPS_RTC_UDR_CON,
> +	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
> +	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
> +	.write_time_udr_mask	= S2MPS_RTC_WUDR_MASK,
> +	.write_alarm_udr_mask	= S2MPS_RTC_WUDR_MASK | S2MPS_RTC_RUDR_MASK,
>   };
>
>   /*
> - * Register map for S2MPS14.
> - * It may be also suitable for S2MPS11 but this was not tested.
> + * Register map for S2MPS15 - in comparison to S2MPS14 the WUDR and AUDR bits
> + * are swapped.
>    */
> -static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
> +static const struct s5m_rtc_reg_config s2mps15_rtc_regs = {
>   	.regs_count		= 7,
>   	.time			= S2MPS_RTC_SEC,
>   	.ctrl			= S2MPS_RTC_CTRL,
> @@ -86,7 +136,9 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
>   	.alarm1			= S2MPS_ALARM1_SEC,
>   	.udr_update		= S2MPS_RTC_UDR_CON,
>   	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
> -	.udr_mask		= S2MPS_RTC_WUDR_MASK,
> +	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
> +	.write_time_udr_mask	= S2MPS15_RTC_WUDR_MASK,
> +	.write_alarm_udr_mask	= S2MPS15_RTC_AUDR_MASK,
>   };
>
>   struct s5m_rtc_info {
> @@ -223,21 +275,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
>   		return ret;
>   	}
>
> -	switch (info->device_type) {
> -	case S5M8763X:
> -	case S5M8767X:
> -		data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
> -	case S2MPS15X:
> -		/* As per UM, for write time register, set WUDR bit to high */
> -		data |= S2MPS15_RTC_WUDR_MASK;
> -		break;
> -	case S2MPS14X:
> -	case S2MPS13X:
> -		data |= info->regs->udr_mask;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +	data |= info->regs->write_time_udr_mask;
>
>   	ret = regmap_write(info->regmap, info->regs->udr_update, data);
>   	if (ret < 0) {
> @@ -262,22 +300,16 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>   		return ret;
>   	}
>
> -	data |= info->regs->udr_mask;
> +	data |= info->regs->write_alarm_udr_mask;
>   	switch (info->device_type) {
>   	case S5M8763X:
>   	case S5M8767X:
>   		data &= ~S5M_RTC_TIME_EN_MASK;
>   		break;
>   	case S2MPS15X:
> -		/* As per UM, for write alarm, set A_UDR(bit[4]) to high
> -		 * udr_mask above sets bit[4]
> -		 */
> -		break;
>   	case S2MPS14X:
> -		data |= S2MPS_RTC_RUDR_MASK;
> -		break;
>   	case S2MPS13X:
> -		data |= S2MPS13_RTC_AUDR_MASK;
> +		/* No exceptions needed */
>   		break;
>   	default:
>   		return -EINVAL;
> @@ -338,11 +370,11 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm)
>   	u8 data[info->regs->regs_count];
>   	int ret;
>
> -	if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
> -			info->device_type == S2MPS13X) {
> +	if (info->regs->read_time_udr_mask) {
>   		ret = regmap_update_bits(info->regmap,
>   				info->regs->udr_update,
> -				S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
> +				info->regs->read_time_udr_mask,
> +				info->regs->read_time_udr_mask);
>   		if (ret) {
>   			dev_err(dev,
>   				"Failed to prepare registers for time reading: %d\n",
> @@ -709,10 +741,18 @@ static int s5m_rtc_probe(struct platform_device *pdev)
>
>   	switch (platform_get_device_id(pdev)->driver_data) {
>   	case S2MPS15X:
> +		regmap_cfg = &s2mps14_rtc_regmap_config;
> +		info->regs = &s2mps15_rtc_regs;
> +		alarm_irq = S2MPS14_IRQ_RTCA0;
> +		break;
>   	case S2MPS14X:
> +		regmap_cfg = &s2mps14_rtc_regmap_config;
> +		info->regs = &s2mps14_rtc_regs;
> +		alarm_irq = S2MPS14_IRQ_RTCA0;
> +		break;
>   	case S2MPS13X:
>   		regmap_cfg = &s2mps14_rtc_regmap_config;
> -		info->regs = &s2mps_rtc_regs;
> +		info->regs = &s2mps13_rtc_regs;
>   		alarm_irq = S2MPS14_IRQ_RTCA0;
>   		break;
>   	case S5M8763X:
> diff --git a/include/linux/mfd/samsung/rtc.h b/include/linux/mfd/samsung/rtc.h
> index a65e4655d470..48c3c5be7eb1 100644
> --- a/include/linux/mfd/samsung/rtc.h
> +++ b/include/linux/mfd/samsung/rtc.h
> @@ -105,6 +105,8 @@ enum s2mps_rtc_reg {
>   #define S5M_RTC_UDR_MASK	(1 << S5M_RTC_UDR_SHIFT)
>   #define S2MPS_RTC_WUDR_SHIFT	4
>   #define S2MPS_RTC_WUDR_MASK	(1 << S2MPS_RTC_WUDR_SHIFT)
> +#define S2MPS15_RTC_AUDR_SHIFT	4
> +#define S2MPS15_RTC_AUDR_MASK	(1 << S2MPS15_RTC_AUDR_SHIFT)
>   #define S2MPS13_RTC_AUDR_SHIFT	1
>   #define S2MPS13_RTC_AUDR_MASK	(1 << S2MPS13_RTC_AUDR_SHIFT)
>   #define S2MPS15_RTC_WUDR_SHIFT	1
>

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

Re: [rtc-linux] Re: [PATCH v2 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions

[Krzysztof Kozlowski]

2015-12-30 19:14 GMT+09:00 Alim Akhtar <alim.akhtar@samsung.com>:
>
> Hi Krzysztof,
>
> On 12/30/2015 10:17 AM, Krzysztof Kozlowski wrote:
>>
>> Before updating time and alarm the driver must set appropriate mask in
>> UDR register. For that purpose the driver uses common register
>> configuration and a lot of exceptions per device in the code. The
>> exceptions are not obvious, for example except the change in the logic
>> sometimes the fields are swapped (WUDR and AUDR between S2MPS14 and
>> S2MPS15). This leads to quite complicated code.
>>
>> Try to make it more obvious by:
>> 1. Documenting the UDR masks for devices and operations.
>> 2. Adding fields in register configuration structure for each operation
>>     (read time, write time and alarm).
>> 3. Splitting the configuration per S2MPS13, S2MPS14 and S2MPS15 thus
>>     removing exceptions for them.
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>
> Tested for s2mps15 pmic on exynos7-espresso board. rtc and timer works as expected.
> Fill free to add
> Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> Tested-by: Alim Akhtar <alim.akhtar@samsung.com>
>

Thank you, I really appreciate this!

Best regards,
Krzysztof

Re: [rtc-linux] Re: [PATCH v2 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions

[Krzysztof Kozlowski]

2015-12-30 19:14 GMT+09:00 Alim Akhtar <alim.akhtar@samsung.com>:
>
> Hi Krzysztof,
>
> On 12/30/2015 10:17 AM, Krzysztof Kozlowski wrote:
>>
>> Before updating time and alarm the driver must set appropriate mask in
>> UDR register. For that purpose the driver uses common register
>> configuration and a lot of exceptions per device in the code. The
>> exceptions are not obvious, for example except the change in the logic
>> sometimes the fields are swapped (WUDR and AUDR between S2MPS14 and
>> S2MPS15). This leads to quite complicated code.
>>
>> Try to make it more obvious by:
>> 1. Documenting the UDR masks for devices and operations.
>> 2. Adding fields in register configuration structure for each operation
>>     (read time, write time and alarm).
>> 3. Splitting the configuration per S2MPS13, S2MPS14 and S2MPS15 thus
>>     removing exceptions for them.
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>
> Tested for s2mps15 pmic on exynos7-espresso board. rtc and timer works as expected.
> Fill free to add
> Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> Tested-by: Alim Akhtar <alim.akhtar@samsung.com>
>

Thank you, I really appreciate this!

Best regards,
Krzysztof

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

Re: [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Alexandre Belloni]

Hi,

On 30/12/2015 at 13:47:25 +0900, Krzysztof Kozlowski wrote :
> Remove the 'rtc' prefix from some of the fields in struct
> s5m_rtc_reg_config because it is obvious - this is a RTC driver. No
> functional changes.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 

I was going to apply those patches but they don't apply cleanly on
rtc-next. Did I miss something?

> ---
> 
> Changes since v1:
> 1. None.
> ---
>  drivers/rtc/rtc-s5m.c | 40 +++++++++++++++++++---------------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index 0d68a85dd429..85649861a6b0 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -55,9 +55,9 @@ struct s5m_rtc_reg_config {
>  	 * will enable update of time or alarm register. Then it will be
>  	 * auto-cleared after successful update.
>  	 */
> -	unsigned int rtc_udr_update;
> -	/* Mask for UDR field in 'rtc_udr_update' register */
> -	unsigned int rtc_udr_mask;
> +	unsigned int udr_update;
> +	/* Mask for UDR field in 'udr_update' register */
> +	unsigned int udr_mask;
>  };
>  
>  /* Register map for S5M8763 and S5M8767 */
> @@ -67,8 +67,8 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
>  	.ctrl			= S5M_ALARM1_CONF,
>  	.alarm0			= S5M_ALARM0_SEC,
>  	.alarm1			= S5M_ALARM1_SEC,
> -	.rtc_udr_update		= S5M_RTC_UDR_CON,
> -	.rtc_udr_mask		= S5M_RTC_UDR_MASK,
> +	.udr_update		= S5M_RTC_UDR_CON,
> +	.udr_mask		= S5M_RTC_UDR_MASK,
>  };
>  
>  /*
> @@ -81,8 +81,8 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
>  	.ctrl			= S2MPS_RTC_CTRL,
>  	.alarm0			= S2MPS_ALARM0_SEC,
>  	.alarm1			= S2MPS_ALARM1_SEC,
> -	.rtc_udr_update		= S2MPS_RTC_UDR_CON,
> -	.rtc_udr_mask		= S2MPS_RTC_WUDR_MASK,
> +	.udr_update		= S2MPS_RTC_UDR_CON,
> +	.udr_mask		= S2MPS_RTC_WUDR_MASK,
>  };
>  
>  struct s5m_rtc_info {
> @@ -166,9 +166,8 @@ static inline int s5m8767_wait_for_udr_update(struct s5m_rtc_info *info)
>  	unsigned int data;
>  
>  	do {
> -		ret = regmap_read(info->regmap, info->regs->rtc_udr_update,
> -				&data);
> -	} while (--retry && (data & info->regs->rtc_udr_mask) && !ret);
> +		ret = regmap_read(info->regmap, info->regs->udr_update, &data);
> +	} while (--retry && (data & info->regs->udr_mask) && !ret);
>  
>  	if (!retry)
>  		dev_err(info->dev, "waiting for UDR update, reached max number of retries\n");
> @@ -214,7 +213,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
>  	int ret;
>  	unsigned int data;
>  
> -	ret = regmap_read(info->regmap, info->regs->rtc_udr_update, &data);
> +	ret = regmap_read(info->regmap, info->regs->udr_update, &data);
>  	if (ret < 0) {
>  		dev_err(info->dev, "failed to read update reg(%d)\n", ret);
>  		return ret;
> @@ -223,21 +222,20 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
>  	switch (info->device_type) {
>  	case S5M8763X:
>  	case S5M8767X:
> -		data |= info->regs->rtc_udr_mask | S5M_RTC_TIME_EN_MASK;
> +		data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
>  	case S2MPS15X:
>  		/* As per UM, for write time register, set WUDR bit to high */
>  		data |= S2MPS15_RTC_WUDR_MASK;
>  		break;
>  	case S2MPS14X:
>  	case S2MPS13X:
> -		data |= info->regs->rtc_udr_mask;
> +		data |= info->regs->udr_mask;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> -
> -	ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
> +	ret = regmap_write(info->regmap, info->regs->udr_update, data);
>  	if (ret < 0) {
>  		dev_err(info->dev, "failed to write update reg(%d)\n", ret);
>  		return ret;
> @@ -253,14 +251,14 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>  	int ret;
>  	unsigned int data;
>  
> -	ret = regmap_read(info->regmap, info->regs->rtc_udr_update, &data);
> +	ret = regmap_read(info->regmap, info->regs->udr_update, &data);
>  	if (ret < 0) {
>  		dev_err(info->dev, "%s: fail to read update reg(%d)\n",
>  			__func__, ret);
>  		return ret;
>  	}
>  
> -	data |= info->regs->rtc_udr_mask;
> +	data |= info->regs->udr_mask;
>  	switch (info->device_type) {
>  	case S5M8763X:
>  	case S5M8767X:
> @@ -268,7 +266,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>  		break;
>  	case S2MPS15X:
>  		/* As per UM, for write alarm, set A_UDR(bit[4]) to high
> -		 * rtc_udr_mask above sets bit[4]
> +		 * udr_mask above sets bit[4]
>  		 */
>  		break;
>  	case S2MPS14X:
> @@ -281,7 +279,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>  		return -EINVAL;
>  	}
>  
> -	ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
> +	ret = regmap_write(info->regmap, info->regs->udr_update, data);
>  	if (ret < 0) {
>  		dev_err(info->dev, "%s: fail to write update reg(%d)\n",
>  			__func__, ret);
> @@ -292,7 +290,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>  
>  	/* On S2MPS13 the AUDR is not auto-cleared */
>  	if (info->device_type == S2MPS13X)
> -		regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
> +		regmap_update_bits(info->regmap, info->regs->udr_update,
>  				   S2MPS13_RTC_AUDR_MASK, 0);
>  
>  	return ret;
> @@ -339,7 +337,7 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  	if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
>  			info->device_type == S2MPS13X) {
>  		ret = regmap_update_bits(info->regmap,
> -				info->regs->rtc_udr_update,
> +				info->regs->udr_update,
>  				S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
>  		if (ret) {
>  			dev_err(dev,
> -- 
> 1.9.1
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[rtc-linux] Re: [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Alexandre Belloni]

Hi,

On 30/12/2015 at 13:47:25 +0900, Krzysztof Kozlowski wrote :
> Remove the 'rtc' prefix from some of the fields in struct
> s5m_rtc_reg_config because it is obvious - this is a RTC driver. No
> functional changes.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 

I was going to apply those patches but they don't apply cleanly on
rtc-next. Did I miss something?

> ---
> 
> Changes since v1:
> 1. None.
> ---
>  drivers/rtc/rtc-s5m.c | 40 +++++++++++++++++++---------------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index 0d68a85dd429..85649861a6b0 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -55,9 +55,9 @@ struct s5m_rtc_reg_config {
>  	 * will enable update of time or alarm register. Then it will be
>  	 * auto-cleared after successful update.
>  	 */
> -	unsigned int rtc_udr_update;
> -	/* Mask for UDR field in 'rtc_udr_update' register */
> -	unsigned int rtc_udr_mask;
> +	unsigned int udr_update;
> +	/* Mask for UDR field in 'udr_update' register */
> +	unsigned int udr_mask;
>  };
>  
>  /* Register map for S5M8763 and S5M8767 */
> @@ -67,8 +67,8 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
>  	.ctrl			= S5M_ALARM1_CONF,
>  	.alarm0			= S5M_ALARM0_SEC,
>  	.alarm1			= S5M_ALARM1_SEC,
> -	.rtc_udr_update		= S5M_RTC_UDR_CON,
> -	.rtc_udr_mask		= S5M_RTC_UDR_MASK,
> +	.udr_update		= S5M_RTC_UDR_CON,
> +	.udr_mask		= S5M_RTC_UDR_MASK,
>  };
>  
>  /*
> @@ -81,8 +81,8 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
>  	.ctrl			= S2MPS_RTC_CTRL,
>  	.alarm0			= S2MPS_ALARM0_SEC,
>  	.alarm1			= S2MPS_ALARM1_SEC,
> -	.rtc_udr_update		= S2MPS_RTC_UDR_CON,
> -	.rtc_udr_mask		= S2MPS_RTC_WUDR_MASK,
> +	.udr_update		= S2MPS_RTC_UDR_CON,
> +	.udr_mask		= S2MPS_RTC_WUDR_MASK,
>  };
>  
>  struct s5m_rtc_info {
> @@ -166,9 +166,8 @@ static inline int s5m8767_wait_for_udr_update(struct s5m_rtc_info *info)
>  	unsigned int data;
>  
>  	do {
> -		ret = regmap_read(info->regmap, info->regs->rtc_udr_update,
> -				&data);
> -	} while (--retry && (data & info->regs->rtc_udr_mask) && !ret);
> +		ret = regmap_read(info->regmap, info->regs->udr_update, &data);
> +	} while (--retry && (data & info->regs->udr_mask) && !ret);
>  
>  	if (!retry)
>  		dev_err(info->dev, "waiting for UDR update, reached max number of retries\n");
> @@ -214,7 +213,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
>  	int ret;
>  	unsigned int data;
>  
> -	ret = regmap_read(info->regmap, info->regs->rtc_udr_update, &data);
> +	ret = regmap_read(info->regmap, info->regs->udr_update, &data);
>  	if (ret < 0) {
>  		dev_err(info->dev, "failed to read update reg(%d)\n", ret);
>  		return ret;
> @@ -223,21 +222,20 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
>  	switch (info->device_type) {
>  	case S5M8763X:
>  	case S5M8767X:
> -		data |= info->regs->rtc_udr_mask | S5M_RTC_TIME_EN_MASK;
> +		data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
>  	case S2MPS15X:
>  		/* As per UM, for write time register, set WUDR bit to high */
>  		data |= S2MPS15_RTC_WUDR_MASK;
>  		break;
>  	case S2MPS14X:
>  	case S2MPS13X:
> -		data |= info->regs->rtc_udr_mask;
> +		data |= info->regs->udr_mask;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> -
> -	ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
> +	ret = regmap_write(info->regmap, info->regs->udr_update, data);
>  	if (ret < 0) {
>  		dev_err(info->dev, "failed to write update reg(%d)\n", ret);
>  		return ret;
> @@ -253,14 +251,14 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>  	int ret;
>  	unsigned int data;
>  
> -	ret = regmap_read(info->regmap, info->regs->rtc_udr_update, &data);
> +	ret = regmap_read(info->regmap, info->regs->udr_update, &data);
>  	if (ret < 0) {
>  		dev_err(info->dev, "%s: fail to read update reg(%d)\n",
>  			__func__, ret);
>  		return ret;
>  	}
>  
> -	data |= info->regs->rtc_udr_mask;
> +	data |= info->regs->udr_mask;
>  	switch (info->device_type) {
>  	case S5M8763X:
>  	case S5M8767X:
> @@ -268,7 +266,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>  		break;
>  	case S2MPS15X:
>  		/* As per UM, for write alarm, set A_UDR(bit[4]) to high
> -		 * rtc_udr_mask above sets bit[4]
> +		 * udr_mask above sets bit[4]
>  		 */
>  		break;
>  	case S2MPS14X:
> @@ -281,7 +279,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>  		return -EINVAL;
>  	}
>  
> -	ret = regmap_write(info->regmap, info->regs->rtc_udr_update, data);
> +	ret = regmap_write(info->regmap, info->regs->udr_update, data);
>  	if (ret < 0) {
>  		dev_err(info->dev, "%s: fail to write update reg(%d)\n",
>  			__func__, ret);
> @@ -292,7 +290,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>  
>  	/* On S2MPS13 the AUDR is not auto-cleared */
>  	if (info->device_type == S2MPS13X)
> -		regmap_update_bits(info->regmap, info->regs->rtc_udr_update,
> +		regmap_update_bits(info->regmap, info->regs->udr_update,
>  				   S2MPS13_RTC_AUDR_MASK, 0);
>  
>  	return ret;
> @@ -339,7 +337,7 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  	if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
>  			info->device_type == S2MPS13X) {
>  		ret = regmap_update_bits(info->regmap,
> -				info->regs->rtc_udr_update,
> +				info->regs->udr_update,
>  				S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
>  		if (ret) {
>  			dev_err(dev,
> -- 
> 1.9.1
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Krzysztof Kozlowski]

On 05.01.2016 02:08, Alexandre Belloni wrote:
> Hi,
> 
> On 30/12/2015 at 13:47:25 +0900, Krzysztof Kozlowski wrote :
>> Remove the 'rtc' prefix from some of the fields in struct
>> s5m_rtc_reg_config because it is obvious - this is a RTC driver. No
>> functional changes.
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>
> 
> I was going to apply those patches but they don't apply cleanly on
> rtc-next. Did I miss something?
> 

Probably that's my fault because I rebased it on linux-next. I will
rebase the patches on rtc-next and resend.

Best regards,
Krzysztof

[rtc-linux] Re: [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Krzysztof Kozlowski]

On 05.01.2016 02:08, Alexandre Belloni wrote:
> Hi,
> 
> On 30/12/2015 at 13:47:25 +0900, Krzysztof Kozlowski wrote :
>> Remove the 'rtc' prefix from some of the fields in struct
>> s5m_rtc_reg_config because it is obvious - this is a RTC driver. No
>> functional changes.
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>
> 
> I was going to apply those patches but they don't apply cleanly on
> rtc-next. Did I miss something?
> 

Probably that's my fault because I rebased it on linux-next. I will
rebase the patches on rtc-next and resend.

Best regards,
Krzysztof

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

Re: [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Krzysztof Kozlowski]

On 05.01.2016 08:53, Krzysztof Kozlowski wrote:
> On 05.01.2016 02:08, Alexandre Belloni wrote:
>> Hi,
>>
>> On 30/12/2015 at 13:47:25 +0900, Krzysztof Kozlowski wrote :
>>> Remove the 'rtc' prefix from some of the fields in struct
>>> s5m_rtc_reg_config because it is obvious - this is a RTC driver. No
>>> functional changes.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>
>>
>> I was going to apply those patches but they don't apply cleanly on
>> rtc-next. Did I miss something?
>>
> 
> Probably that's my fault because I rebased it on linux-next. I will
> rebase the patches on rtc-next and resend.

Ah, no need for rebasing. I forgot about dependency - the support for
S2MPS15 was added recently through Lee Jones' tree. Fortunately Lee put
S2MPS15 patches in a separate branch and tag: "ib-mfd-regulator-rtc-4.5"

This means that to apply the patches this is needed first:
https://git.kernel.org/cgit/linux/kernel/git/lee/mfd.git/log/?h=ib-mfd-regulator-rtc-4.5

Best regards,
Krzysztof

[rtc-linux] Re: [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Krzysztof Kozlowski]

On 05.01.2016 08:53, Krzysztof Kozlowski wrote:
> On 05.01.2016 02:08, Alexandre Belloni wrote:
>> Hi,
>>
>> On 30/12/2015 at 13:47:25 +0900, Krzysztof Kozlowski wrote :
>>> Remove the 'rtc' prefix from some of the fields in struct
>>> s5m_rtc_reg_config because it is obvious - this is a RTC driver. No
>>> functional changes.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>>
>>
>> I was going to apply those patches but they don't apply cleanly on
>> rtc-next. Did I miss something?
>>
> 
> Probably that's my fault because I rebased it on linux-next. I will
> rebase the patches on rtc-next and resend.

Ah, no need for rebasing. I forgot about dependency - the support for
S2MPS15 was added recently through Lee Jones' tree. Fortunately Lee put
S2MPS15 patches in a separate branch and tag: "ib-mfd-regulator-rtc-4.5"

This means that to apply the patches this is needed first:
https://git.kernel.org/cgit/linux/kernel/git/lee/mfd.git/log/?h=ib-mfd-regulator-rtc-4.5

Best regards,
Krzysztof



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

Re: [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Alexandre Belloni]

On 05/01/2016 at 09:04:39 +0900, Krzysztof Kozlowski wrote :
> > 
> > Probably that's my fault because I rebased it on linux-next. I will
> > rebase the patches on rtc-next and resend.
> 
> Ah, no need for rebasing. I forgot about dependency - the support for
> S2MPS15 was added recently through Lee Jones' tree. Fortunately Lee put
> S2MPS15 patches in a separate branch and tag: "ib-mfd-regulator-rtc-4.5"
> 
> This means that to apply the patches this is needed first:
> https://git.kernel.org/cgit/linux/kernel/git/lee/mfd.git/log/?h=ib-mfd-regulator-rtc-4.5
> 

Ah, true, that's what I forgot, thanks.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[rtc-linux] Re: [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Alexandre Belloni]

On 05/01/2016 at 09:04:39 +0900, Krzysztof Kozlowski wrote :
> > 
> > Probably that's my fault because I rebased it on linux-next. I will
> > rebase the patches on rtc-next and resend.
> 
> Ah, no need for rebasing. I forgot about dependency - the support for
> S2MPS15 was added recently through Lee Jones' tree. Fortunately Lee put
> S2MPS15 patches in a separate branch and tag: "ib-mfd-regulator-rtc-4.5"
> 
> This means that to apply the patches this is needed first:
> https://git.kernel.org/cgit/linux/kernel/git/lee/mfd.git/log/?h=ib-mfd-regulator-rtc-4.5
> 

Ah, true, that's what I forgot, thanks.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH v2 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions

[Lee Jones]

On Wed, 30 Dec 2015, Krzysztof Kozlowski wrote:

> Before updating time and alarm the driver must set appropriate mask in
> UDR register. For that purpose the driver uses common register
> configuration and a lot of exceptions per device in the code. The
> exceptions are not obvious, for example except the change in the logic
> sometimes the fields are swapped (WUDR and AUDR between S2MPS14 and
> S2MPS15). This leads to quite complicated code.
> 
> Try to make it more obvious by:
> 1. Documenting the UDR masks for devices and operations.
> 2. Adding fields in register configuration structure for each operation
>    (read time, write time and alarm).
> 3. Splitting the configuration per S2MPS13, S2MPS14 and S2MPS15 thus
>    removing exceptions for them.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> ---
> 
> Tested on S2MPS11 (Odroid XU4) and S2MPS13. Testing on S2MPS15 would be
> appreciated!
> 
> Changes since v1:
> 1. Fix value used in S2MPS15_RTC_AUDR_MASK, pointed by Yadwinder Singh
>    Brar.
> ---
>  drivers/rtc/rtc-s5m.c           | 110 +++++++++++++++++++++++++++-------------
>  include/linux/mfd/samsung/rtc.h |   2 +

Acked-by: Lee Jones <lee.jones@linaro.org>

>  2 files changed, 77 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index 559db8f72117..7407d7394bb4 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -38,7 +38,22 @@
>   */
>  #define UDR_READ_RETRY_CNT	5
>  
> -/* Registers used by the driver which are different between chipsets. */
> +/*
> + * Registers used by the driver which are different between chipsets.
> + *
> + * Operations like read time and write alarm/time require updating
> + * specific fields in UDR register. These fields usually are auto-cleared
> + * (with some exceptions).
> + *
> + * Table of operations per device:
> + *
> + * Device     | Write time | Read time | Write alarm
> + * =================================================
> + * S5M8767    | UDR + TIME |           | UDR
> + * S2MPS11/14 | WUDR       | RUDR      | WUDR + RUDR
> + * S2MPS13    | WUDR       | RUDR      | WUDR + AUDR
> + * S2MPS15    | WUDR       | RUDR      | AUDR
> + */
>  struct s5m_rtc_reg_config {
>  	/* Number of registers used for setting time/alarm0/alarm1 */
>  	unsigned int regs_count;
> @@ -58,8 +73,13 @@ struct s5m_rtc_reg_config {
>  	unsigned int udr_update;
>  	/* Auto-cleared mask in UDR field for writing time and alarm */
>  	unsigned int autoclear_udr_mask;
> -	/* Mask for UDR field in 'udr_update' register */
> -	unsigned int udr_mask;
> +	/*
> +	 * Masks in UDR field for time and alarm operations.
> +	 * The read time mask can be 0. Rest should not.
> +	 */
> +	unsigned int read_time_udr_mask;
> +	unsigned int write_time_udr_mask;
> +	unsigned int write_alarm_udr_mask;
>  };
>  
>  /* Register map for S5M8763 and S5M8767 */
> @@ -71,14 +91,44 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
>  	.alarm1			= S5M_ALARM1_SEC,
>  	.udr_update		= S5M_RTC_UDR_CON,
>  	.autoclear_udr_mask	= S5M_RTC_UDR_MASK,
> -	.udr_mask		= S5M_RTC_UDR_MASK,
> +	.read_time_udr_mask	= 0, /* Not needed */
> +	.write_time_udr_mask	= S5M_RTC_UDR_MASK | S5M_RTC_TIME_EN_MASK,
> +	.write_alarm_udr_mask	= S5M_RTC_UDR_MASK,
> +};
> +
> +/* Register map for S2MPS13 */
> +static const struct s5m_rtc_reg_config s2mps13_rtc_regs = {
> +	.regs_count		= 7,
> +	.time			= S2MPS_RTC_SEC,
> +	.ctrl			= S2MPS_RTC_CTRL,
> +	.alarm0			= S2MPS_ALARM0_SEC,
> +	.alarm1			= S2MPS_ALARM1_SEC,
> +	.udr_update		= S2MPS_RTC_UDR_CON,
> +	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
> +	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
> +	.write_time_udr_mask	= S2MPS_RTC_WUDR_MASK,
> +	.write_alarm_udr_mask	= S2MPS_RTC_WUDR_MASK | S2MPS13_RTC_AUDR_MASK,
> +};
> +
> +/* Register map for S2MPS11/14 */
> +static const struct s5m_rtc_reg_config s2mps14_rtc_regs = {
> +	.regs_count		= 7,
> +	.time			= S2MPS_RTC_SEC,
> +	.ctrl			= S2MPS_RTC_CTRL,
> +	.alarm0			= S2MPS_ALARM0_SEC,
> +	.alarm1			= S2MPS_ALARM1_SEC,
> +	.udr_update		= S2MPS_RTC_UDR_CON,
> +	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
> +	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
> +	.write_time_udr_mask	= S2MPS_RTC_WUDR_MASK,
> +	.write_alarm_udr_mask	= S2MPS_RTC_WUDR_MASK | S2MPS_RTC_RUDR_MASK,
>  };
>  
>  /*
> - * Register map for S2MPS14.
> - * It may be also suitable for S2MPS11 but this was not tested.
> + * Register map for S2MPS15 - in comparison to S2MPS14 the WUDR and AUDR bits
> + * are swapped.
>   */
> -static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
> +static const struct s5m_rtc_reg_config s2mps15_rtc_regs = {
>  	.regs_count		= 7,
>  	.time			= S2MPS_RTC_SEC,
>  	.ctrl			= S2MPS_RTC_CTRL,
> @@ -86,7 +136,9 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
>  	.alarm1			= S2MPS_ALARM1_SEC,
>  	.udr_update		= S2MPS_RTC_UDR_CON,
>  	.autoclear_udr_mask	= S2MPS_RTC_WUDR_MASK,
> -	.udr_mask		= S2MPS_RTC_WUDR_MASK,
> +	.read_time_udr_mask	= S2MPS_RTC_RUDR_MASK,
> +	.write_time_udr_mask	= S2MPS15_RTC_WUDR_MASK,
> +	.write_alarm_udr_mask	= S2MPS15_RTC_AUDR_MASK,
>  };
>  
>  struct s5m_rtc_info {
> @@ -223,21 +275,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
>  		return ret;
>  	}
>  
> -	switch (info->device_type) {
> -	case S5M8763X:
> -	case S5M8767X:
> -		data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
> -	case S2MPS15X:
> -		/* As per UM, for write time register, set WUDR bit to high */
> -		data |= S2MPS15_RTC_WUDR_MASK;
> -		break;
> -	case S2MPS14X:
> -	case S2MPS13X:
> -		data |= info->regs->udr_mask;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +	data |= info->regs->write_time_udr_mask;
>  
>  	ret = regmap_write(info->regmap, info->regs->udr_update, data);
>  	if (ret < 0) {
> @@ -262,22 +300,16 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>  		return ret;
>  	}
>  
> -	data |= info->regs->udr_mask;
> +	data |= info->regs->write_alarm_udr_mask;
>  	switch (info->device_type) {
>  	case S5M8763X:
>  	case S5M8767X:
>  		data &= ~S5M_RTC_TIME_EN_MASK;
>  		break;
>  	case S2MPS15X:
> -		/* As per UM, for write alarm, set A_UDR(bit[4]) to high
> -		 * udr_mask above sets bit[4]
> -		 */
> -		break;
>  	case S2MPS14X:
> -		data |= S2MPS_RTC_RUDR_MASK;
> -		break;
>  	case S2MPS13X:
> -		data |= S2MPS13_RTC_AUDR_MASK;
> +		/* No exceptions needed */
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -338,11 +370,11 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  	u8 data[info->regs->regs_count];
>  	int ret;
>  
> -	if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
> -			info->device_type == S2MPS13X) {
> +	if (info->regs->read_time_udr_mask) {
>  		ret = regmap_update_bits(info->regmap,
>  				info->regs->udr_update,
> -				S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
> +				info->regs->read_time_udr_mask,
> +				info->regs->read_time_udr_mask);
>  		if (ret) {
>  			dev_err(dev,
>  				"Failed to prepare registers for time reading: %d\n",
> @@ -709,10 +741,18 @@ static int s5m_rtc_probe(struct platform_device *pdev)
>  
>  	switch (platform_get_device_id(pdev)->driver_data) {
>  	case S2MPS15X:
> +		regmap_cfg = &s2mps14_rtc_regmap_config;
> +		info->regs = &s2mps15_rtc_regs;
> +		alarm_irq = S2MPS14_IRQ_RTCA0;
> +		break;
>  	case S2MPS14X:
> +		regmap_cfg = &s2mps14_rtc_regmap_config;
> +		info->regs = &s2mps14_rtc_regs;
> +		alarm_irq = S2MPS14_IRQ_RTCA0;
> +		break;
>  	case S2MPS13X:
>  		regmap_cfg = &s2mps14_rtc_regmap_config;
> -		info->regs = &s2mps_rtc_regs;
> +		info->regs = &s2mps13_rtc_regs;
>  		alarm_irq = S2MPS14_IRQ_RTCA0;
>  		break;
>  	case S5M8763X:
> diff --git a/include/linux/mfd/samsung/rtc.h b/include/linux/mfd/samsung/rtc.h
> index a65e4655d470..48c3c5be7eb1 100644
> --- a/include/linux/mfd/samsung/rtc.h
> +++ b/include/linux/mfd/samsung/rtc.h
> @@ -105,6 +105,8 @@ enum s2mps_rtc_reg {
>  #define S5M_RTC_UDR_MASK	(1 << S5M_RTC_UDR_SHIFT)
>  #define S2MPS_RTC_WUDR_SHIFT	4
>  #define S2MPS_RTC_WUDR_MASK	(1 << S2MPS_RTC_WUDR_SHIFT)
> +#define S2MPS15_RTC_AUDR_SHIFT	4
> +#define S2MPS15_RTC_AUDR_MASK	(1 << S2MPS15_RTC_AUDR_SHIFT)
>  #define S2MPS13_RTC_AUDR_SHIFT	1
>  #define S2MPS13_RTC_AUDR_MASK	(1 << S2MPS13_RTC_AUDR_SHIFT)
>  #define S2MPS15_RTC_WUDR_SHIFT	1

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[rtc-linux] Re: [PATCH v2 3/3] rtc: s5m: Make register configuration per S2MPS device to remove exceptions

[Lee Jones]

On Wed, 30 Dec 2015, Krzysztof Kozlowski wrote:

> Before updating time and alarm the driver must set appropriate mask in
> UDR register. For that purpose the driver uses common register
> configuration and a lot of exceptions per device in the code. The
> exceptions are not obvious, for example except the change in the logic
> sometimes the fields are swapped (WUDR and AUDR between S2MPS14 and
> S2MPS15). This leads to quite complicated code.
>=20
> Try to make it more obvious by:
> 1. Documenting the UDR masks for devices and operations.
> 2. Adding fields in register configuration structure for each operation
>    (read time, write time and alarm).
> 3. Splitting the configuration per S2MPS13, S2MPS14 and S2MPS15 thus
>    removing exceptions for them.
>=20
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>=20
> ---
>=20
> Tested on S2MPS11 (Odroid XU4) and S2MPS13. Testing on S2MPS15 would be
> appreciated!
>=20
> Changes since v1:
> 1. Fix value used in S2MPS15_RTC_AUDR_MASK, pointed by Yadwinder Singh
>    Brar.
> ---
>  drivers/rtc/rtc-s5m.c           | 110 +++++++++++++++++++++++++++-------=
------
>  include/linux/mfd/samsung/rtc.h |   2 +

Acked-by: Lee Jones <lee.jones@linaro.org>

>  2 files changed, 77 insertions(+), 35 deletions(-)
>=20
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index 559db8f72117..7407d7394bb4 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -38,7 +38,22 @@
>   */
>  #define UDR_READ_RETRY_CNT	5
> =20
> -/* Registers used by the driver which are different between chipsets. */
> +/*
> + * Registers used by the driver which are different between chipsets.
> + *
> + * Operations like read time and write alarm/time require updating
> + * specific fields in UDR register. These fields usually are auto-cleare=
d
> + * (with some exceptions).
> + *
> + * Table of operations per device:
> + *
> + * Device     | Write time | Read time | Write alarm
> + * =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D
> + * S5M8767    | UDR + TIME |           | UDR
> + * S2MPS11/14 | WUDR       | RUDR      | WUDR + RUDR
> + * S2MPS13    | WUDR       | RUDR      | WUDR + AUDR
> + * S2MPS15    | WUDR       | RUDR      | AUDR
> + */
>  struct s5m_rtc_reg_config {
>  	/* Number of registers used for setting time/alarm0/alarm1 */
>  	unsigned int regs_count;
> @@ -58,8 +73,13 @@ struct s5m_rtc_reg_config {
>  	unsigned int udr_update;
>  	/* Auto-cleared mask in UDR field for writing time and alarm */
>  	unsigned int autoclear_udr_mask;
> -	/* Mask for UDR field in 'udr_update' register */
> -	unsigned int udr_mask;
> +	/*
> +	 * Masks in UDR field for time and alarm operations.
> +	 * The read time mask can be 0. Rest should not.
> +	 */
> +	unsigned int read_time_udr_mask;
> +	unsigned int write_time_udr_mask;
> +	unsigned int write_alarm_udr_mask;
>  };
> =20
>  /* Register map for S5M8763 and S5M8767 */
> @@ -71,14 +91,44 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs =
=3D {
>  	.alarm1			=3D S5M_ALARM1_SEC,
>  	.udr_update		=3D S5M_RTC_UDR_CON,
>  	.autoclear_udr_mask	=3D S5M_RTC_UDR_MASK,
> -	.udr_mask		=3D S5M_RTC_UDR_MASK,
> +	.read_time_udr_mask	=3D 0, /* Not needed */
> +	.write_time_udr_mask	=3D S5M_RTC_UDR_MASK | S5M_RTC_TIME_EN_MASK,
> +	.write_alarm_udr_mask	=3D S5M_RTC_UDR_MASK,
> +};
> +
> +/* Register map for S2MPS13 */
> +static const struct s5m_rtc_reg_config s2mps13_rtc_regs =3D {
> +	.regs_count		=3D 7,
> +	.time			=3D S2MPS_RTC_SEC,
> +	.ctrl			=3D S2MPS_RTC_CTRL,
> +	.alarm0			=3D S2MPS_ALARM0_SEC,
> +	.alarm1			=3D S2MPS_ALARM1_SEC,
> +	.udr_update		=3D S2MPS_RTC_UDR_CON,
> +	.autoclear_udr_mask	=3D S2MPS_RTC_WUDR_MASK,
> +	.read_time_udr_mask	=3D S2MPS_RTC_RUDR_MASK,
> +	.write_time_udr_mask	=3D S2MPS_RTC_WUDR_MASK,
> +	.write_alarm_udr_mask	=3D S2MPS_RTC_WUDR_MASK | S2MPS13_RTC_AUDR_MASK,
> +};
> +
> +/* Register map for S2MPS11/14 */
> +static const struct s5m_rtc_reg_config s2mps14_rtc_regs =3D {
> +	.regs_count		=3D 7,
> +	.time			=3D S2MPS_RTC_SEC,
> +	.ctrl			=3D S2MPS_RTC_CTRL,
> +	.alarm0			=3D S2MPS_ALARM0_SEC,
> +	.alarm1			=3D S2MPS_ALARM1_SEC,
> +	.udr_update		=3D S2MPS_RTC_UDR_CON,
> +	.autoclear_udr_mask	=3D S2MPS_RTC_WUDR_MASK,
> +	.read_time_udr_mask	=3D S2MPS_RTC_RUDR_MASK,
> +	.write_time_udr_mask	=3D S2MPS_RTC_WUDR_MASK,
> +	.write_alarm_udr_mask	=3D S2MPS_RTC_WUDR_MASK | S2MPS_RTC_RUDR_MASK,
>  };
> =20
>  /*
> - * Register map for S2MPS14.
> - * It may be also suitable for S2MPS11 but this was not tested.
> + * Register map for S2MPS15 - in comparison to S2MPS14 the WUDR and AUDR=
 bits
> + * are swapped.
>   */
> -static const struct s5m_rtc_reg_config s2mps_rtc_regs =3D {
> +static const struct s5m_rtc_reg_config s2mps15_rtc_regs =3D {
>  	.regs_count		=3D 7,
>  	.time			=3D S2MPS_RTC_SEC,
>  	.ctrl			=3D S2MPS_RTC_CTRL,
> @@ -86,7 +136,9 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs =
=3D {
>  	.alarm1			=3D S2MPS_ALARM1_SEC,
>  	.udr_update		=3D S2MPS_RTC_UDR_CON,
>  	.autoclear_udr_mask	=3D S2MPS_RTC_WUDR_MASK,
> -	.udr_mask		=3D S2MPS_RTC_WUDR_MASK,
> +	.read_time_udr_mask	=3D S2MPS_RTC_RUDR_MASK,
> +	.write_time_udr_mask	=3D S2MPS15_RTC_WUDR_MASK,
> +	.write_alarm_udr_mask	=3D S2MPS15_RTC_AUDR_MASK,
>  };
> =20
>  struct s5m_rtc_info {
> @@ -223,21 +275,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5=
m_rtc_info *info)
>  		return ret;
>  	}
> =20
> -	switch (info->device_type) {
> -	case S5M8763X:
> -	case S5M8767X:
> -		data |=3D info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
> -	case S2MPS15X:
> -		/* As per UM, for write time register, set WUDR bit to high */
> -		data |=3D S2MPS15_RTC_WUDR_MASK;
> -		break;
> -	case S2MPS14X:
> -	case S2MPS13X:
> -		data |=3D info->regs->udr_mask;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +	data |=3D info->regs->write_time_udr_mask;
> =20
>  	ret =3D regmap_write(info->regmap, info->regs->udr_update, data);
>  	if (ret < 0) {
> @@ -262,22 +300,16 @@ static inline int s5m8767_rtc_set_alarm_reg(struct =
s5m_rtc_info *info)
>  		return ret;
>  	}
> =20
> -	data |=3D info->regs->udr_mask;
> +	data |=3D info->regs->write_alarm_udr_mask;
>  	switch (info->device_type) {
>  	case S5M8763X:
>  	case S5M8767X:
>  		data &=3D ~S5M_RTC_TIME_EN_MASK;
>  		break;
>  	case S2MPS15X:
> -		/* As per UM, for write alarm, set A_UDR(bit[4]) to high
> -		 * udr_mask above sets bit[4]
> -		 */
> -		break;
>  	case S2MPS14X:
> -		data |=3D S2MPS_RTC_RUDR_MASK;
> -		break;
>  	case S2MPS13X:
> -		data |=3D S2MPS13_RTC_AUDR_MASK;
> +		/* No exceptions needed */
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -338,11 +370,11 @@ static int s5m_rtc_read_time(struct device *dev, st=
ruct rtc_time *tm)
>  	u8 data[info->regs->regs_count];
>  	int ret;
> =20
> -	if (info->device_type =3D=3D S2MPS15X || info->device_type =3D=3D S2MPS=
14X ||
> -			info->device_type =3D=3D S2MPS13X) {
> +	if (info->regs->read_time_udr_mask) {
>  		ret =3D regmap_update_bits(info->regmap,
>  				info->regs->udr_update,
> -				S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
> +				info->regs->read_time_udr_mask,
> +				info->regs->read_time_udr_mask);
>  		if (ret) {
>  			dev_err(dev,
>  				"Failed to prepare registers for time reading: %d\n",
> @@ -709,10 +741,18 @@ static int s5m_rtc_probe(struct platform_device *pd=
ev)
> =20
>  	switch (platform_get_device_id(pdev)->driver_data) {
>  	case S2MPS15X:
> +		regmap_cfg =3D &s2mps14_rtc_regmap_config;
> +		info->regs =3D &s2mps15_rtc_regs;
> +		alarm_irq =3D S2MPS14_IRQ_RTCA0;
> +		break;
>  	case S2MPS14X:
> +		regmap_cfg =3D &s2mps14_rtc_regmap_config;
> +		info->regs =3D &s2mps14_rtc_regs;
> +		alarm_irq =3D S2MPS14_IRQ_RTCA0;
> +		break;
>  	case S2MPS13X:
>  		regmap_cfg =3D &s2mps14_rtc_regmap_config;
> -		info->regs =3D &s2mps_rtc_regs;
> +		info->regs =3D &s2mps13_rtc_regs;
>  		alarm_irq =3D S2MPS14_IRQ_RTCA0;
>  		break;
>  	case S5M8763X:
> diff --git a/include/linux/mfd/samsung/rtc.h b/include/linux/mfd/samsung/=
rtc.h
> index a65e4655d470..48c3c5be7eb1 100644
> --- a/include/linux/mfd/samsung/rtc.h
> +++ b/include/linux/mfd/samsung/rtc.h
> @@ -105,6 +105,8 @@ enum s2mps_rtc_reg {
>  #define S5M_RTC_UDR_MASK	(1 << S5M_RTC_UDR_SHIFT)
>  #define S2MPS_RTC_WUDR_SHIFT	4
>  #define S2MPS_RTC_WUDR_MASK	(1 << S2MPS_RTC_WUDR_SHIFT)
> +#define S2MPS15_RTC_AUDR_SHIFT	4
> +#define S2MPS15_RTC_AUDR_MASK	(1 << S2MPS15_RTC_AUDR_SHIFT)
>  #define S2MPS13_RTC_AUDR_SHIFT	1
>  #define S2MPS13_RTC_AUDR_MASK	(1 << S2MPS13_RTC_AUDR_SHIFT)
>  #define S2MPS15_RTC_WUDR_SHIFT	1

--=20
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org =E2=94=82 Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

--=20
--=20
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.
---=20
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 e=
mail to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Alexandre Belloni]

On 05/01/2016 at 09:04:39 +0900, Krzysztof Kozlowski wrote :
> On 05.01.2016 08:53, Krzysztof Kozlowski wrote:
> > On 05.01.2016 02:08, Alexandre Belloni wrote:
> >> Hi,
> >>
> >> On 30/12/2015 at 13:47:25 +0900, Krzysztof Kozlowski wrote :
> >>> Remove the 'rtc' prefix from some of the fields in struct
> >>> s5m_rtc_reg_config because it is obvious - this is a RTC driver. No
> >>> functional changes.
> >>>
> >>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >>>
> >>
> >> I was going to apply those patches but they don't apply cleanly on
> >> rtc-next. Did I miss something?
> >>
> > 
> > Probably that's my fault because I rebased it on linux-next. I will
> > rebase the patches on rtc-next and resend.
> 
> Ah, no need for rebasing. I forgot about dependency - the support for
> S2MPS15 was added recently through Lee Jones' tree. Fortunately Lee put
> S2MPS15 patches in a separate branch and tag: "ib-mfd-regulator-rtc-4.5"
> 
> This means that to apply the patches this is needed first:
> https://git.kernel.org/cgit/linux/kernel/git/lee/mfd.git/log/?h=ib-mfd-regulator-rtc-4.5
> 

All applied now, thanks

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[rtc-linux] Re: [PATCH v2 1/3] rtc: s5m: Cleanup by removing useless 'rtc' prefix from fields

[Alexandre Belloni]

On 05/01/2016 at 09:04:39 +0900, Krzysztof Kozlowski wrote :
> On 05.01.2016 08:53, Krzysztof Kozlowski wrote:
> > On 05.01.2016 02:08, Alexandre Belloni wrote:
> >> Hi,
> >>
> >> On 30/12/2015 at 13:47:25 +0900, Krzysztof Kozlowski wrote :
> >>> Remove the 'rtc' prefix from some of the fields in struct
> >>> s5m_rtc_reg_config because it is obvious - this is a RTC driver. No
> >>> functional changes.
> >>>
> >>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >>>
> >>
> >> I was going to apply those patches but they don't apply cleanly on
> >> rtc-next. Did I miss something?
> >>
> > 
> > Probably that's my fault because I rebased it on linux-next. I will
> > rebase the patches on rtc-next and resend.
> 
> Ah, no need for rebasing. I forgot about dependency - the support for
> S2MPS15 was added recently through Lee Jones' tree. Fortunately Lee put
> S2MPS15 patches in a separate branch and tag: "ib-mfd-regulator-rtc-4.5"
> 
> This means that to apply the patches this is needed first:
> https://git.kernel.org/cgit/linux/kernel/git/lee/mfd.git/log/?h=ib-mfd-regulator-rtc-4.5
> 

All applied now, thanks

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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