[PATCH 0/8] rtc: max77686: Extend driver and add max77802 support

[PATCH 0/8] rtc: max77686: Extend driver and add max77802 support

[Javier Martinez Canillas]

Hello,

On a recent disussion [0] with Krzysztof Kozlowski and Laxman Dewangan,
we came to the conclusion that the max77686 and max77802 RTC are almost
the same with only a few differences so there shouldn't be two separate
drivers and is better to extend max77686 driver and delete rtc-max77802.

By making the driver more generic, other RTC IP blocks from Maxim PMICs
could be supported as well like the max77620.

Patches #1 is just a trivial cleanup.

Patch #2 allows to support RTCs that need a shorter delay when updating
the RTC.

Patch #3 adds a driver data structure to avoid hard-coding parameters
specific to a certain RTC such as the needed delay and RTC register mask.

Patch #4 changes the driver to use a mapping table instead of using the
max77686 registers offsets directly to allow supporting RTC with other
registers addresses and layout.

Patch #5 Adds support for max77802 to max77686 RTC driver and patch #6
removes the old driver since is not needed anymore.

Finally patch #7 and patch #8 removes the Kconfig symbol from defconfigs.

I've tested this patch-set on an Exynos5800 Peach Pi Chromebook that has
a max77802 PMIC and the RTC was working correctly but I don't have a
machine with max77686 so I will really appreaciate if someone can test
that no regressions were introduced.

One thing that I'm not sure is how to handle bisectability, in patch #5
there will be two drivers that matches "rtc-max77802". So I don't know
if I should use a different platform_device_id name and change in the
same patch that max77802 is removed or if this is not a big deal so the
patches could stay as is.

I believe all patches should go through the RTC tree with proper acks or
wait until the RTC patches land to pick the defconfig changes.

[0]: http://www.spinics.net/lists/devicetree/msg110348.html


Javier Martinez Canillas (8):
  rtc: max77686: Use ARRAY_SIZE() instead of current array length
  rtc: max77686: Use usleep_range() instead of msleep()
  rtc: max77686: Use a driver data struct instead hard-coded values
  rtc: max77686: Add an indirection level to access RTC registers
  rtc: max77686: Add max77802 support
  rtc: Remove Maxim 77802 driver
  ARM: exynos_defconfig: Remove MAX77802 RTC Kconfig symbol
  ARM: multi_v7_defconfig: Remove MAX77802 RTC Kconfig symbol

 arch/arm/configs/exynos_defconfig   |   1 -
 arch/arm/configs/multi_v7_defconfig |   1 -
 drivers/rtc/Kconfig                 |  10 -
 drivers/rtc/Makefile                |   1 -
 drivers/rtc/rtc-max77686.c          | 276 +++++++++++++++-----
 drivers/rtc/rtc-max77802.c          | 502 ------------------------------------
 6 files changed, 213 insertions(+), 578 deletions(-)
 delete mode 100644 drivers/rtc/rtc-max77802.c

-- 
2.5.0

[PATCH 1/8] rtc: max77686: Use ARRAY_SIZE() instead of current array length

[Javier Martinez Canillas]

It is better to use the ARRAY_SIZE() macro instead of the array length
to avoid bugs if the array is later changed and the length not updated.

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

 drivers/rtc/rtc-max77686.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 7184a0eda793..9599be257db4 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -406,7 +406,8 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
 
 	info->rtc_24hr_mode = 1;
 
-	ret = regmap_bulk_write(info->max77686->rtc_regmap, MAX77686_RTC_CONTROLM, data, 2);
+	ret = regmap_bulk_write(info->max77686->rtc_regmap,
+				MAX77686_RTC_CONTROLM, data, ARRAY_SIZE(data));
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to write controlm reg(%d)\n",
 				__func__, ret);
-- 
2.5.0

[PATCH 2/8] rtc: max77686: Use usleep_range() instead of msleep()

[Javier Martinez Canillas]

Documentation/timers/timers-howto.txt suggest to use usleep_range()
instead of msleep() for small msec (1ms - 20ms) since msleep() will
often sleep for 20ms for any value in that range.

This is fine in this case since 16ms is the _minimum_ delay required
by max77686 for an RTC update but by using usleep_range() instead of
msleep(), the driver can support other RTC IP blocks with a shorter
minium delay (i.e: in the range of usecs insted of msecs).

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

 drivers/rtc/rtc-max77686.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 9599be257db4..ae4d61e7ce4b 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -41,7 +41,7 @@
 #define ALARM_ENABLE_SHIFT		7
 #define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
 
-#define MAX77686_RTC_UPDATE_DELAY	16
+#define MAX77686_RTC_UPDATE_DELAY	16000
 
 enum {
 	RTC_SEC = 0,
@@ -130,7 +130,8 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
 				__func__, ret, data);
 	else {
 		/* Minimum 16ms delay required before RTC update. */
-		msleep(MAX77686_RTC_UPDATE_DELAY);
+		usleep_range(MAX77686_RTC_UPDATE_DELAY,
+			     MAX77686_RTC_UPDATE_DELAY * 2);
 	}
 
 	return ret;
-- 
2.5.0

[PATCH 3/8] rtc: max77686: Use a driver data struct instead hard-coded values

[Javier Martinez Canillas]

The driver has some hard-coded values such as the minimum delay needed
before a RTC update or the mask used for the sec/min/hour/etc registers.

Use a data structure that contains these values and pass as driver data
using the platform device ID table for each device.

This allows to make the driver's ops callbacks more generic so other RTC
that are similar but don't have the same values can also be supported.

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

 drivers/rtc/rtc-max77686.c | 47 +++++++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index ae4d61e7ce4b..441d163dcbeb 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -41,8 +41,6 @@
 #define ALARM_ENABLE_SHIFT		7
 #define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
 
-#define MAX77686_RTC_UPDATE_DELAY	16000
-
 enum {
 	RTC_SEC = 0,
 	RTC_MIN,
@@ -54,6 +52,11 @@ enum {
 	RTC_NR_TIME
 };
 
+struct rtc_driver_data {
+	unsigned long		delay;
+	int			mask;
+};
+
 struct max77686_rtc_info {
 	struct device		*dev;
 	struct max77686_dev	*max77686;
@@ -63,6 +66,8 @@ struct max77686_rtc_info {
 
 	struct regmap		*regmap;
 
+	struct rtc_driver_data	*drv_data;
+
 	int virq;
 	int rtc_24hr_mode;
 };
@@ -72,12 +77,19 @@ enum MAX77686_RTC_OP {
 	MAX77686_RTC_READ,
 };
 
+static const struct rtc_driver_data max77686_drv_data = {
+	.delay = 1600,
+	.mask  = 0x7f,
+};
+
 static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
-				   int rtc_24hr_mode)
+				    struct max77686_rtc_info *info)
 {
-	tm->tm_sec = data[RTC_SEC] & 0x7f;
-	tm->tm_min = data[RTC_MIN] & 0x7f;
-	if (rtc_24hr_mode)
+	int mask = info->drv_data->mask;
+
+	tm->tm_sec = data[RTC_SEC] & mask;
+	tm->tm_min = data[RTC_MIN] & mask;
+	if (info->rtc_24hr_mode)
 		tm->tm_hour = data[RTC_HOUR] & 0x1f;
 	else {
 		tm->tm_hour = data[RTC_HOUR] & 0x0f;
@@ -86,10 +98,10 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
 	}
 
 	/* Only a single bit is set in data[], so fls() would be equivalent */
-	tm->tm_wday = ffs(data[RTC_WEEKDAY] & 0x7f) - 1;
+	tm->tm_wday = ffs(data[RTC_WEEKDAY] & mask) - 1;
 	tm->tm_mday = data[RTC_DATE] & 0x1f;
 	tm->tm_mon = (data[RTC_MONTH] & 0x0f) - 1;
-	tm->tm_year = (data[RTC_YEAR] & 0x7f) + 100;
+	tm->tm_year = (data[RTC_YEAR] & mask) + 100;
 	tm->tm_yday = 0;
 	tm->tm_isdst = 0;
 }
@@ -117,6 +129,7 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
 {
 	int ret;
 	unsigned int data;
+	unsigned long delay = info->drv_data->delay;
 
 	if (op == MAX77686_RTC_WRITE)
 		data = 1 << RTC_UDR_SHIFT;
@@ -129,9 +142,8 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
 		dev_err(info->dev, "%s: fail to write update reg(ret=%d, data=0x%x)\n",
 				__func__, ret, data);
 	else {
-		/* Minimum 16ms delay required before RTC update. */
-		usleep_range(MAX77686_RTC_UPDATE_DELAY,
-			     MAX77686_RTC_UPDATE_DELAY * 2);
+		/* Minimum delay required before RTC update. */
+		usleep_range(delay, delay * 2);
 	}
 
 	return ret;
@@ -156,7 +168,7 @@ static int max77686_rtc_read_time(struct device *dev, struct rtc_time *tm)
 		goto out;
 	}
 
-	max77686_rtc_data_to_tm(data, tm, info->rtc_24hr_mode);
+	max77686_rtc_data_to_tm(data, tm, info);
 
 	ret = rtc_valid_tm(tm);
 
@@ -213,7 +225,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 		goto out;
 	}
 
-	max77686_rtc_data_to_tm(data, &alrm->time, info->rtc_24hr_mode);
+	max77686_rtc_data_to_tm(data, &alrm->time, info);
 
 	alrm->enabled = 0;
 	for (i = 0; i < RTC_NR_TIME; i++) {
@@ -260,7 +272,7 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
 		goto out;
 	}
 
-	max77686_rtc_data_to_tm(data, &tm, info->rtc_24hr_mode);
+	max77686_rtc_data_to_tm(data, &tm, info);
 
 	for (i = 0; i < RTC_NR_TIME; i++)
 		data[i] &= ~ALARM_ENABLE_MASK;
@@ -299,7 +311,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
 		goto out;
 	}
 
-	max77686_rtc_data_to_tm(data, &tm, info->rtc_24hr_mode);
+	max77686_rtc_data_to_tm(data, &tm, info);
 
 	data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
 	data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
@@ -307,7 +319,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
 	data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
 	if (data[RTC_MONTH] & 0xf)
 		data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
-	if (data[RTC_YEAR] & 0x7f)
+	if (data[RTC_YEAR] & info->drv_data->mask)
 		data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
 	if (data[RTC_DATE] & 0x1f)
 		data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
@@ -436,6 +448,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 	info->dev = &pdev->dev;
 	info->max77686 = max77686;
 	info->rtc = max77686->rtc;
+	info->drv_data = (struct rtc_driver_data *)pdev->id_entry->driver_data;
 
 	platform_set_drvdata(pdev, info);
 
@@ -510,7 +523,7 @@ static SIMPLE_DEV_PM_OPS(max77686_rtc_pm_ops,
 			 max77686_rtc_suspend, max77686_rtc_resume);
 
 static const struct platform_device_id rtc_id[] = {
-	{ "max77686-rtc", 0 },
+	{ "max77686-rtc", .driver_data = (kernel_ulong_t)&max77686_drv_data, },
 	{},
 };
 MODULE_DEVICE_TABLE(platform, rtc_id);
-- 
2.5.0

[PATCH 4/8] rtc: max77686: Add an indirection level to access RTC registers

[Javier Martinez Canillas]

The max77686 driver is generic enough that can be used for other
Maxim RTC IP blocks but these might not have the same registers
layout so instead of accessing the registers directly, add a map
to translate offsets to the real registers addresses for each IP.

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

 drivers/rtc/rtc-max77686.c | 75 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 65 insertions(+), 10 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 441d163dcbeb..7316e41820c7 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -41,6 +41,8 @@
 #define ALARM_ENABLE_SHIFT		7
 #define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
 
+#define REG_RTC_NONE			0xdeadbeef
+
 enum {
 	RTC_SEC = 0,
 	RTC_MIN,
@@ -55,6 +57,7 @@ enum {
 struct rtc_driver_data {
 	unsigned long		delay;
 	int			mask;
+	const unsigned int	*map;
 };
 
 struct max77686_rtc_info {
@@ -77,9 +80,53 @@ enum MAX77686_RTC_OP {
 	MAX77686_RTC_READ,
 };
 
+/* These are not registers but just offsets that are mapped to addresses */
+enum rtc_reg {
+	REG_RTC_CONTROLM = 0,
+	REG_RTC_CONTROL,
+	REG_RTC_UPDATE0,
+	REG_RTC_UPDATE1,
+	REG_WTSR_SMPL_CNTL,
+	REG_RTC_SEC,
+	REG_RTC_MIN,
+	REG_RTC_HOUR,
+	REG_RTC_WEEKDAY,
+	REG_RTC_MONTH,
+	REG_RTC_YEAR,
+	REG_RTC_DATE,
+	REG_ALARM1_SEC,
+	REG_ALARM1_MIN,
+	REG_ALARM1_HOUR,
+	REG_ALARM1_WEEKDAY,
+	REG_ALARM1_MONTH,
+	REG_ALARM1_YEAR,
+	REG_ALARM1_DATE,
+	REG_ALARM2_SEC,
+	REG_ALARM2_MIN,
+	REG_ALARM2_HOUR,
+	REG_ALARM2_WEEKDAY,
+	REG_ALARM2_MONTH,
+	REG_ALARM2_YEAR,
+	REG_ALARM2_DATE,
+	REG_RTC_END,
+};
+
+static const unsigned int max77686_map[REG_RTC_END] = {
+	MAX77686_RTC_CONTROLM, MAX77686_RTC_CONTROL, MAX77686_RTC_UPDATE0,
+	REG_RTC_NONE, MAX77686_WTSR_SMPL_CNTL, MAX77686_RTC_SEC,
+	MAX77686_RTC_MIN, MAX77686_RTC_HOUR, MAX77686_RTC_WEEKDAY,
+	MAX77686_RTC_MONTH, MAX77686_RTC_YEAR, MAX77686_RTC_DATE,
+	MAX77686_ALARM1_SEC, MAX77686_ALARM1_MIN, MAX77686_ALARM1_HOUR,
+	MAX77686_ALARM1_WEEKDAY, MAX77686_ALARM1_MONTH, MAX77686_ALARM1_YEAR,
+	MAX77686_ALARM1_DATE, MAX77686_ALARM2_SEC, MAX77686_ALARM2_MIN,
+	MAX77686_ALARM2_HOUR, MAX77686_ALARM2_WEEKDAY, MAX77686_ALARM2_MONTH,
+	MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE,
+};
+
 static const struct rtc_driver_data max77686_drv_data = {
 	.delay = 1600,
 	.mask  = 0x7f,
+	.map   = max77686_map,
 };
 
 static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
@@ -137,7 +184,8 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
 		data = 1 << RTC_RBUDR_SHIFT;
 
 	ret = regmap_update_bits(info->max77686->rtc_regmap,
-				 MAX77686_RTC_UPDATE0, data, data);
+				 info->drv_data->map[REG_RTC_UPDATE0],
+				 data, data);
 	if (ret < 0)
 		dev_err(info->dev, "%s: fail to write update reg(ret=%d, data=0x%x)\n",
 				__func__, ret, data);
@@ -162,7 +210,8 @@ static int max77686_rtc_read_time(struct device *dev, struct rtc_time *tm)
 		goto out;
 
 	ret = regmap_bulk_read(info->max77686->rtc_regmap,
-				MAX77686_RTC_SEC, data, RTC_NR_TIME);
+			       info->drv_data->map[REG_RTC_SEC],
+			       data, RTC_NR_TIME);
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to read time reg(%d)\n", __func__,	ret);
 		goto out;
@@ -190,7 +239,8 @@ static int max77686_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	mutex_lock(&info->lock);
 
 	ret = regmap_bulk_write(info->max77686->rtc_regmap,
-				 MAX77686_RTC_SEC, data, RTC_NR_TIME);
+				info->drv_data->map[REG_RTC_SEC],
+				data, RTC_NR_TIME);
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to write time reg(%d)\n", __func__,
 				ret);
@@ -209,6 +259,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	struct max77686_rtc_info *info = dev_get_drvdata(dev);
 	u8 data[RTC_NR_TIME];
 	unsigned int val;
+	const unsigned int *map = info->drv_data->map;
 	int i, ret;
 
 	mutex_lock(&info->lock);
@@ -218,7 +269,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 		goto out;
 
 	ret = regmap_bulk_read(info->max77686->rtc_regmap,
-				 MAX77686_ALARM1_SEC, data, RTC_NR_TIME);
+			       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
 	if (ret < 0) {
 		dev_err(info->dev, "%s:%d fail to read alarm reg(%d)\n",
 				__func__, __LINE__, ret);
@@ -256,6 +307,7 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
 	u8 data[RTC_NR_TIME];
 	int ret, i;
 	struct rtc_time tm;
+	const unsigned int *map = info->drv_data->map;
 
 	if (!mutex_is_locked(&info->lock))
 		dev_warn(info->dev, "%s: should have mutex locked\n", __func__);
@@ -265,7 +317,7 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
 		goto out;
 
 	ret = regmap_bulk_read(info->max77686->rtc_regmap,
-				 MAX77686_ALARM1_SEC, data, RTC_NR_TIME);
+			       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
 				__func__, ret);
@@ -278,7 +330,7 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
 		data[i] &= ~ALARM_ENABLE_MASK;
 
 	ret = regmap_bulk_write(info->max77686->rtc_regmap,
-				 MAX77686_ALARM1_SEC, data, RTC_NR_TIME);
+				map[REG_ALARM1_SEC], data, RTC_NR_TIME);
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
 				__func__, ret);
@@ -295,6 +347,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
 	u8 data[RTC_NR_TIME];
 	int ret;
 	struct rtc_time tm;
+	const unsigned int *map = info->drv_data->map;
 
 	if (!mutex_is_locked(&info->lock))
 		dev_warn(info->dev, "%s: should have mutex locked\n", __func__);
@@ -304,7 +357,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
 		goto out;
 
 	ret = regmap_bulk_read(info->max77686->rtc_regmap,
-				 MAX77686_ALARM1_SEC, data, RTC_NR_TIME);
+			       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
 				__func__, ret);
@@ -325,7 +378,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
 		data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
 
 	ret = regmap_bulk_write(info->max77686->rtc_regmap,
-				 MAX77686_ALARM1_SEC, data, RTC_NR_TIME);
+				map[REG_ALARM1_SEC], data, RTC_NR_TIME);
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
 				__func__, ret);
@@ -354,7 +407,8 @@ static int max77686_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 		goto out;
 
 	ret = regmap_bulk_write(info->max77686->rtc_regmap,
-				 MAX77686_ALARM1_SEC, data, RTC_NR_TIME);
+				info->drv_data->map[REG_ALARM1_SEC],
+				data, RTC_NR_TIME);
 
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
@@ -420,7 +474,8 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
 	info->rtc_24hr_mode = 1;
 
 	ret = regmap_bulk_write(info->max77686->rtc_regmap,
-				MAX77686_RTC_CONTROLM, data, ARRAY_SIZE(data));
+				info->drv_data->map[REG_RTC_CONTROLM],
+				data, ARRAY_SIZE(data));
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to write controlm reg(%d)\n",
 				__func__, ret);
-- 
2.5.0

[PATCH 5/8] rtc: max77686: Add max77802 support

[Javier Martinez Canillas]

The MAX77686 and MAX77802 RTC IP blocks are very similar with only
these differences:

0) The RTC registers layout and addresses are different.

1) The MAX77686 use 1 bit of the sec/min/hour/etc registers as the
   alarm enable while MAX77802 has a separate register for that.

2) The MAX77686 RTCYEAR register valid values range is 0..99 while
   for MAX77802 is 0..199.

3) The MAX77686 has a separate I2C address for the RTC registers
   while the MAX77802 uses the same I2C address as the PMIC regs.

5) They minium delay before a RTC update (16ms vs 200 usecs).

There are separate drivers for MAX77686 and MAX77802 RTC IP blocks
but the differences are not that big so the driver can be extended
to support both instead of duplicating a lot of code in 2 drivers.

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

 drivers/rtc/rtc-max77686.c | 176 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 128 insertions(+), 48 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 7316e41820c7..7a144e7ecd27 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -1,5 +1,5 @@
 /*
- * RTC driver for Maxim MAX77686
+ * RTC driver for Maxim MAX77686 and MAX77802
  *
  * Copyright (C) 2012 Samsung Electronics Co.Ltd
  *
@@ -43,6 +43,13 @@
 
 #define REG_RTC_NONE			0xdeadbeef
 
+/*
+ * MAX77802 has separate register (RTCAE1) for alarm enable instead
+ * using 1 bit from registers RTC{SEC,MIN,HOUR,DAY,MONTH,YEAR,DATE}
+ * as in done in MAX77686.
+ */
+#define ALARM_ENABLE_VALUE		0x77
+
 enum {
 	RTC_SEC = 0,
 	RTC_MIN,
@@ -58,6 +65,10 @@ struct rtc_driver_data {
 	unsigned long		delay;
 	int			mask;
 	const unsigned int	*map;
+	/* Has a separate alarm enable register? */
+	bool			rtcae;
+	/* Has a separate I2C regmap for the RTC? */
+	bool			rtcrm;
 };
 
 struct max77686_rtc_info {
@@ -108,6 +119,8 @@ enum rtc_reg {
 	REG_ALARM2_MONTH,
 	REG_ALARM2_YEAR,
 	REG_ALARM2_DATE,
+	REG_RTC_AE1,
+	REG_RTC_AE2,
 	REG_RTC_END,
 };
 
@@ -120,13 +133,36 @@ static const unsigned int max77686_map[REG_RTC_END] = {
 	MAX77686_ALARM1_WEEKDAY, MAX77686_ALARM1_MONTH, MAX77686_ALARM1_YEAR,
 	MAX77686_ALARM1_DATE, MAX77686_ALARM2_SEC, MAX77686_ALARM2_MIN,
 	MAX77686_ALARM2_HOUR, MAX77686_ALARM2_WEEKDAY, MAX77686_ALARM2_MONTH,
-	MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE,
+	MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE, REG_RTC_NONE, REG_RTC_NONE,
 };
 
 static const struct rtc_driver_data max77686_drv_data = {
 	.delay = 1600,
 	.mask  = 0x7f,
 	.map   = max77686_map,
+	.rtcae = false,
+	.rtcrm = true,
+};
+
+static const unsigned int max77802_map[REG_RTC_END] = {
+	MAX77802_RTC_CONTROLM, MAX77802_RTC_CONTROL, MAX77802_RTC_UPDATE0,
+	REG_RTC_NONE, MAX77802_WTSR_SMPL_CNTL, MAX77802_RTC_SEC,
+	MAX77802_RTC_MIN, MAX77802_RTC_HOUR, MAX77802_RTC_WEEKDAY,
+	MAX77802_RTC_MONTH, MAX77802_RTC_YEAR, MAX77802_RTC_DATE,
+	MAX77802_ALARM1_SEC, MAX77802_ALARM1_MIN, MAX77802_ALARM1_HOUR,
+	MAX77686_ALARM1_WEEKDAY, MAX77802_ALARM1_MONTH, MAX77802_ALARM1_YEAR,
+	MAX77802_ALARM1_DATE, MAX77802_ALARM1_SEC, MAX77802_ALARM1_MIN,
+	MAX77802_ALARM1_HOUR, MAX77802_ALARM1_WEEKDAY, MAX77802_ALARM1_MONTH,
+	MAX77802_ALARM1_YEAR, MAX77802_ALARM1_DATE, MAX77802_RTC_AE1,
+	MAX77802_RTC_AE2,
+};
+
+static const struct rtc_driver_data max77802_drv_data = {
+	.delay = 200,
+	.mask  = 0xff,
+	.map   = max77802_map,
+	.rtcae = true,
+	.rtcrm = false,
 };
 
 static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
@@ -148,12 +184,20 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
 	tm->tm_wday = ffs(data[RTC_WEEKDAY] & mask) - 1;
 	tm->tm_mday = data[RTC_DATE] & 0x1f;
 	tm->tm_mon = (data[RTC_MONTH] & 0x0f) - 1;
-	tm->tm_year = (data[RTC_YEAR] & mask) + 100;
+	tm->tm_year = data[RTC_YEAR] & mask;
 	tm->tm_yday = 0;
 	tm->tm_isdst = 0;
+
+	/*
+	 * MAX77686 uses 1 bit from sec/min/hour/etc RTC registers and the
+	 * year values are just 0..99 so add 100 to support up to 2099.
+	 */
+	if (!info->drv_data->rtcae)
+		tm->tm_year += 100;
 }
 
-static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data)
+static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data,
+				   struct max77686_rtc_info *info)
 {
 	data[RTC_SEC] = tm->tm_sec;
 	data[RTC_MIN] = tm->tm_min;
@@ -161,13 +205,19 @@ static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data)
 	data[RTC_WEEKDAY] = 1 << tm->tm_wday;
 	data[RTC_DATE] = tm->tm_mday;
 	data[RTC_MONTH] = tm->tm_mon + 1;
-	data[RTC_YEAR] = tm->tm_year > 100 ? (tm->tm_year - 100) : 0;
 
-	if (tm->tm_year < 100) {
-		pr_warn("RTC cannot handle the year %d.  Assume it's 2000.\n",
-			1900 + tm->tm_year);
-		return -EINVAL;
+	if (!info->drv_data->rtcae) {
+		data[RTC_YEAR] = tm->tm_year > 100 ? (tm->tm_year - 100) : 0;
+
+		if (tm->tm_year < 100) {
+			pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
+				1900 + tm->tm_year);
+			return -EINVAL;
+		}
+	} else {
+		data[RTC_YEAR] = tm->tm_year;
 	}
+
 	return 0;
 }
 
@@ -232,7 +282,7 @@ static int max77686_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	u8 data[RTC_NR_TIME];
 	int ret;
 
-	ret = max77686_rtc_tm_to_data(tm, data);
+	ret = max77686_rtc_tm_to_data(tm, data, info);
 	if (ret < 0)
 		return ret;
 
@@ -279,11 +329,24 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	max77686_rtc_data_to_tm(data, &alrm->time, info);
 
 	alrm->enabled = 0;
-	for (i = 0; i < RTC_NR_TIME; i++) {
-		if (data[i] & ALARM_ENABLE_MASK) {
-			alrm->enabled = 1;
-			break;
+
+	if (!info->drv_data->rtcae) {
+		for (i = 0; i < RTC_NR_TIME; i++) {
+			if (data[i] & ALARM_ENABLE_MASK) {
+				alrm->enabled = 1;
+				break;
+			}
 		}
+	} else {
+		ret = regmap_read(info->max77686->regmap,
+				  map[REG_RTC_AE1], &val);
+		if (ret < 0) {
+			dev_err(info->dev, "%s:%d fail to read alarm enable(%d)\n",
+				__func__, __LINE__, ret);
+			goto out;
+		}
+		if (val)
+			alrm->enabled = 1;
 	}
 
 	alrm->pending = 0;
@@ -316,21 +379,27 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
 	if (ret < 0)
 		goto out;
 
-	ret = regmap_bulk_read(info->max77686->rtc_regmap,
-			       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
-	if (ret < 0) {
-		dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
+	if (!info->drv_data->rtcae) {
+		ret = regmap_bulk_read(info->max77686->rtc_regmap,
+				       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
+		if (ret < 0) {
+			dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
 				__func__, ret);
-		goto out;
-	}
+			goto out;
+		}
 
-	max77686_rtc_data_to_tm(data, &tm, info);
+		max77686_rtc_data_to_tm(data, &tm, info);
 
-	for (i = 0; i < RTC_NR_TIME; i++)
-		data[i] &= ~ALARM_ENABLE_MASK;
+		for (i = 0; i < RTC_NR_TIME; i++)
+			data[i] &= ~ALARM_ENABLE_MASK;
+
+		ret = regmap_bulk_write(info->max77686->rtc_regmap,
+					map[REG_ALARM1_SEC], data, RTC_NR_TIME);
+	} else {
+		ret = regmap_write(info->max77686->regmap,
+				   map[REG_RTC_AE1], 0);
+	}
 
-	ret = regmap_bulk_write(info->max77686->rtc_regmap,
-				map[REG_ALARM1_SEC], data, RTC_NR_TIME);
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
 				__func__, ret);
@@ -356,29 +425,35 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
 	if (ret < 0)
 		goto out;
 
-	ret = regmap_bulk_read(info->max77686->rtc_regmap,
-			       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
-	if (ret < 0) {
-		dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
+	if (!info->drv_data->rtcae) {
+		ret = regmap_bulk_read(info->max77686->rtc_regmap,
+				       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
+		if (ret < 0) {
+			dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
 				__func__, ret);
-		goto out;
-	}
-
-	max77686_rtc_data_to_tm(data, &tm, info);
+			goto out;
+		}
 
-	data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
-	data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
-	data[RTC_HOUR] |= (1 << ALARM_ENABLE_SHIFT);
-	data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
-	if (data[RTC_MONTH] & 0xf)
-		data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
-	if (data[RTC_YEAR] & info->drv_data->mask)
-		data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
-	if (data[RTC_DATE] & 0x1f)
-		data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
+		max77686_rtc_data_to_tm(data, &tm, info);
+
+		data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
+		data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
+		data[RTC_HOUR] |= (1 << ALARM_ENABLE_SHIFT);
+		data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
+		if (data[RTC_MONTH] & 0xf)
+			data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
+		if (data[RTC_YEAR] & info->drv_data->mask)
+			data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
+		if (data[RTC_DATE] & 0x1f)
+			data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
+
+		ret = regmap_bulk_write(info->max77686->rtc_regmap,
+					map[REG_ALARM1_SEC], data, RTC_NR_TIME);
+	} else {
+		ret = regmap_write(info->max77686->regmap,
+				   map[REG_RTC_AE1], ALARM_ENABLE_VALUE);
+	}
 
-	ret = regmap_bulk_write(info->max77686->rtc_regmap,
-				map[REG_ALARM1_SEC], data, RTC_NR_TIME);
 	if (ret < 0) {
 		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
 				__func__, ret);
@@ -396,7 +471,7 @@ static int max77686_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	u8 data[RTC_NR_TIME];
 	int ret;
 
-	ret = max77686_rtc_tm_to_data(&alrm->time, data);
+	ret = max77686_rtc_tm_to_data(&alrm->time, data, info);
 	if (ret < 0)
 		return ret;
 
@@ -490,6 +565,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 {
 	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
 	struct max77686_rtc_info *info;
+	const struct platform_device_id *id = pdev->id_entry;
 	int ret;
 
 	dev_info(&pdev->dev, "%s\n", __func__);
@@ -503,7 +579,10 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 	info->dev = &pdev->dev;
 	info->max77686 = max77686;
 	info->rtc = max77686->rtc;
-	info->drv_data = (struct rtc_driver_data *)pdev->id_entry->driver_data;
+	info->drv_data = (struct rtc_driver_data *)id->driver_data;
+
+	if (!info->drv_data->rtcrm)
+		info->max77686->rtc_regmap = info->max77686->regmap;
 
 	platform_set_drvdata(pdev, info);
 
@@ -516,7 +595,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, 1);
 
-	info->rtc_dev = devm_rtc_device_register(&pdev->dev, "max77686-rtc",
+	info->rtc_dev = devm_rtc_device_register(&pdev->dev, id->name,
 					&max77686_rtc_ops, THIS_MODULE);
 
 	if (IS_ERR(info->rtc_dev)) {
@@ -579,6 +658,7 @@ static SIMPLE_DEV_PM_OPS(max77686_rtc_pm_ops,
 
 static const struct platform_device_id rtc_id[] = {
 	{ "max77686-rtc", .driver_data = (kernel_ulong_t)&max77686_drv_data, },
+	{ "max77802-rtc", .driver_data = (kernel_ulong_t)&max77802_drv_data, },
 	{},
 };
 MODULE_DEVICE_TABLE(platform, rtc_id);
-- 
2.5.0

[PATCH 6/8] rtc: Remove Maxim 77802 driver

[Javier Martinez Canillas]

The max77686 RTC driver now supports the max77802 RTC as
well so there's no need to have a separate driver anymore.

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

 drivers/rtc/Kconfig        |  10 -
 drivers/rtc/Makefile       |   1 -
 drivers/rtc/rtc-max77802.c | 502 ---------------------------------------------
 3 files changed, 513 deletions(-)
 delete mode 100644 drivers/rtc/rtc-max77802.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 376322f71fd5..ef456d3cf265 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -335,16 +335,6 @@ config RTC_DRV_RK808
 	  This driver can also be built as a module. If so, the module
 	  will be called rk808-rtc.
 
-config RTC_DRV_MAX77802
-	tristate "Maxim 77802 RTC"
-	depends on MFD_MAX77686
-	help
-	  If you say yes here you will get support for the
-	  RTC of Maxim MAX77802 PMIC.
-
-	  This driver can also be built as a module. If so, the module
-	  will be called rtc-max77802.
-
 config RTC_DRV_RS5C372
 	tristate "Ricoh R2025S/D, RS5C372A/B, RV5C386, RV5C387A"
 	help
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 62d61b26ca7e..ed4519efa3ca 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -86,7 +86,6 @@ obj-$(CONFIG_RTC_DRV_M48T86)	+= rtc-m48t86.o
 obj-$(CONFIG_RTC_DRV_MAX6900)	+= rtc-max6900.o
 obj-$(CONFIG_RTC_DRV_MAX6902)	+= rtc-max6902.o
 obj-$(CONFIG_RTC_DRV_MAX77686)	+= rtc-max77686.o
-obj-$(CONFIG_RTC_DRV_MAX77802)	+= rtc-max77802.o
 obj-$(CONFIG_RTC_DRV_MAX8907)	+= rtc-max8907.o
 obj-$(CONFIG_RTC_DRV_MAX8925)	+= rtc-max8925.o
 obj-$(CONFIG_RTC_DRV_MAX8997)	+= rtc-max8997.o
diff --git a/drivers/rtc/rtc-max77802.c b/drivers/rtc/rtc-max77802.c
deleted file mode 100644
index 82ffcc5a5345..000000000000
--- a/drivers/rtc/rtc-max77802.c
+++ /dev/null
@@ -1,502 +0,0 @@
-/*
- * RTC driver for Maxim MAX77802
- *
- * Copyright (C) 2013 Google, Inc
- *
- * Copyright (C) 2012 Samsung Electronics Co.Ltd
- *
- *  based on rtc-max8997.c
- *
- *  This program is free software; you can redistribute  it and/or modify it
- *  under  the terms of  the GNU General  Public License as published by the
- *  Free Software Foundation;  either version 2 of the  License, or (at your
- *  option) any later version.
- *
- */
-
-#include <linux/slab.h>
-#include <linux/rtc.h>
-#include <linux/delay.h>
-#include <linux/mutex.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/mfd/max77686-private.h>
-#include <linux/irqdomain.h>
-#include <linux/regmap.h>
-
-/* RTC Control Register */
-#define BCD_EN_SHIFT			0
-#define BCD_EN_MASK			(1 << BCD_EN_SHIFT)
-#define MODEL24_SHIFT			1
-#define MODEL24_MASK			(1 << MODEL24_SHIFT)
-/* RTC Update Register1 */
-#define RTC_UDR_SHIFT			0
-#define RTC_UDR_MASK			(1 << RTC_UDR_SHIFT)
-#define RTC_RBUDR_SHIFT			4
-#define RTC_RBUDR_MASK			(1 << RTC_RBUDR_SHIFT)
-/* RTC Hour register */
-#define HOUR_PM_SHIFT			6
-#define HOUR_PM_MASK			(1 << HOUR_PM_SHIFT)
-/* RTC Alarm Enable */
-#define ALARM_ENABLE_SHIFT		7
-#define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
-
-/* For the RTCAE1 register, we write this value to enable the alarm */
-#define ALARM_ENABLE_VALUE		0x77
-
-#define MAX77802_RTC_UPDATE_DELAY_US	200
-
-enum {
-	RTC_SEC = 0,
-	RTC_MIN,
-	RTC_HOUR,
-	RTC_WEEKDAY,
-	RTC_MONTH,
-	RTC_YEAR,
-	RTC_DATE,
-	RTC_NR_TIME
-};
-
-struct max77802_rtc_info {
-	struct device		*dev;
-	struct max77686_dev	*max77802;
-	struct i2c_client	*rtc;
-	struct rtc_device	*rtc_dev;
-	struct mutex		lock;
-
-	struct regmap		*regmap;
-
-	int virq;
-	int rtc_24hr_mode;
-};
-
-enum MAX77802_RTC_OP {
-	MAX77802_RTC_WRITE,
-	MAX77802_RTC_READ,
-};
-
-static void max77802_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
-				   int rtc_24hr_mode)
-{
-	tm->tm_sec = data[RTC_SEC] & 0xff;
-	tm->tm_min = data[RTC_MIN] & 0xff;
-	if (rtc_24hr_mode)
-		tm->tm_hour = data[RTC_HOUR] & 0x1f;
-	else {
-		tm->tm_hour = data[RTC_HOUR] & 0x0f;
-		if (data[RTC_HOUR] & HOUR_PM_MASK)
-			tm->tm_hour += 12;
-	}
-
-	/* Only a single bit is set in data[], so fls() would be equivalent */
-	tm->tm_wday = ffs(data[RTC_WEEKDAY] & 0xff) - 1;
-	tm->tm_mday = data[RTC_DATE] & 0x1f;
-	tm->tm_mon = (data[RTC_MONTH] & 0x0f) - 1;
-
-	tm->tm_year = data[RTC_YEAR] & 0xff;
-	tm->tm_yday = 0;
-	tm->tm_isdst = 0;
-}
-
-static int max77802_rtc_tm_to_data(struct rtc_time *tm, u8 *data)
-{
-	data[RTC_SEC] = tm->tm_sec;
-	data[RTC_MIN] = tm->tm_min;
-	data[RTC_HOUR] = tm->tm_hour;
-	data[RTC_WEEKDAY] = 1 << tm->tm_wday;
-	data[RTC_DATE] = tm->tm_mday;
-	data[RTC_MONTH] = tm->tm_mon + 1;
-	data[RTC_YEAR] = tm->tm_year;
-
-	return 0;
-}
-
-static int max77802_rtc_update(struct max77802_rtc_info *info,
-	enum MAX77802_RTC_OP op)
-{
-	int ret;
-	unsigned int data;
-
-	if (op == MAX77802_RTC_WRITE)
-		data = 1 << RTC_UDR_SHIFT;
-	else
-		data = 1 << RTC_RBUDR_SHIFT;
-
-	ret = regmap_update_bits(info->max77802->regmap,
-				 MAX77802_RTC_UPDATE0, data, data);
-	if (ret < 0)
-		dev_err(info->dev, "%s: fail to write update reg(ret=%d, data=0x%x)\n",
-				__func__, ret, data);
-	else {
-		/* Minimum delay required before RTC update. */
-		usleep_range(MAX77802_RTC_UPDATE_DELAY_US,
-			     MAX77802_RTC_UPDATE_DELAY_US * 2);
-	}
-
-	return ret;
-}
-
-static int max77802_rtc_read_time(struct device *dev, struct rtc_time *tm)
-{
-	struct max77802_rtc_info *info = dev_get_drvdata(dev);
-	u8 data[RTC_NR_TIME];
-	int ret;
-
-	mutex_lock(&info->lock);
-
-	ret = max77802_rtc_update(info, MAX77802_RTC_READ);
-	if (ret < 0)
-		goto out;
-
-	ret = regmap_bulk_read(info->max77802->regmap,
-				MAX77802_RTC_SEC, data, RTC_NR_TIME);
-	if (ret < 0) {
-		dev_err(info->dev, "%s: fail to read time reg(%d)\n", __func__,
-			ret);
-		goto out;
-	}
-
-	max77802_rtc_data_to_tm(data, tm, info->rtc_24hr_mode);
-
-	ret = rtc_valid_tm(tm);
-
-out:
-	mutex_unlock(&info->lock);
-	return ret;
-}
-
-static int max77802_rtc_set_time(struct device *dev, struct rtc_time *tm)
-{
-	struct max77802_rtc_info *info = dev_get_drvdata(dev);
-	u8 data[RTC_NR_TIME];
-	int ret;
-
-	ret = max77802_rtc_tm_to_data(tm, data);
-	if (ret < 0)
-		return ret;
-
-	mutex_lock(&info->lock);
-
-	ret = regmap_bulk_write(info->max77802->regmap,
-				 MAX77802_RTC_SEC, data, RTC_NR_TIME);
-	if (ret < 0) {
-		dev_err(info->dev, "%s: fail to write time reg(%d)\n", __func__,
-			ret);
-		goto out;
-	}
-
-	ret = max77802_rtc_update(info, MAX77802_RTC_WRITE);
-
-out:
-	mutex_unlock(&info->lock);
-	return ret;
-}
-
-static int max77802_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
-{
-	struct max77802_rtc_info *info = dev_get_drvdata(dev);
-	u8 data[RTC_NR_TIME];
-	unsigned int val;
-	int ret;
-
-	mutex_lock(&info->lock);
-
-	ret = max77802_rtc_update(info, MAX77802_RTC_READ);
-	if (ret < 0)
-		goto out;
-
-	ret = regmap_bulk_read(info->max77802->regmap,
-				 MAX77802_ALARM1_SEC, data, RTC_NR_TIME);
-	if (ret < 0) {
-		dev_err(info->dev, "%s:%d fail to read alarm reg(%d)\n",
-				__func__, __LINE__, ret);
-		goto out;
-	}
-
-	max77802_rtc_data_to_tm(data, &alrm->time, info->rtc_24hr_mode);
-
-	alrm->enabled = 0;
-	ret = regmap_read(info->max77802->regmap,
-			  MAX77802_RTC_AE1, &val);
-	if (ret < 0) {
-		dev_err(info->dev, "%s:%d fail to read alarm enable(%d)\n",
-			__func__, __LINE__, ret);
-		goto out;
-	}
-	if (val)
-		alrm->enabled = 1;
-
-	alrm->pending = 0;
-	ret = regmap_read(info->max77802->regmap, MAX77802_REG_STATUS2, &val);
-	if (ret < 0) {
-		dev_err(info->dev, "%s:%d fail to read status2 reg(%d)\n",
-				__func__, __LINE__, ret);
-		goto out;
-	}
-
-	if (val & (1 << 2)) /* RTCA1 */
-		alrm->pending = 1;
-
-out:
-	mutex_unlock(&info->lock);
-	return 0;
-}
-
-static int max77802_rtc_stop_alarm(struct max77802_rtc_info *info)
-{
-	int ret;
-
-	if (!mutex_is_locked(&info->lock))
-		dev_warn(info->dev, "%s: should have mutex locked\n", __func__);
-
-	ret = max77802_rtc_update(info, MAX77802_RTC_READ);
-	if (ret < 0)
-		goto out;
-
-	ret = regmap_write(info->max77802->regmap,
-			   MAX77802_RTC_AE1, 0);
-	if (ret < 0) {
-		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
-			__func__, ret);
-		goto out;
-	}
-
-	ret = max77802_rtc_update(info, MAX77802_RTC_WRITE);
-out:
-	return ret;
-}
-
-static int max77802_rtc_start_alarm(struct max77802_rtc_info *info)
-{
-	int ret;
-
-	if (!mutex_is_locked(&info->lock))
-		dev_warn(info->dev, "%s: should have mutex locked\n",
-			 __func__);
-
-	ret = max77802_rtc_update(info, MAX77802_RTC_READ);
-	if (ret < 0)
-		goto out;
-
-	ret = regmap_write(info->max77802->regmap,
-				   MAX77802_RTC_AE1,
-				   ALARM_ENABLE_VALUE);
-
-	if (ret < 0) {
-		dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
-				__func__, ret);
-		goto out;
-	}
-
-	ret = max77802_rtc_update(info, MAX77802_RTC_WRITE);
-out:
-	return ret;
-}
-
-static int max77802_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
-{
-	struct max77802_rtc_info *info = dev_get_drvdata(dev);
-	u8 data[RTC_NR_TIME];
-	int ret;
-
-	ret = max77802_rtc_tm_to_data(&alrm->time, data);
-	if (ret < 0)
-		return ret;
-
-	mutex_lock(&info->lock);
-
-	ret = max77802_rtc_stop_alarm(info);
-	if (ret < 0)
-		goto out;
-
-	ret = regmap_bulk_write(info->max77802->regmap,
-				 MAX77802_ALARM1_SEC, data, RTC_NR_TIME);
-
-	if (ret < 0) {
-		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
-				__func__, ret);
-		goto out;
-	}
-
-	ret = max77802_rtc_update(info, MAX77802_RTC_WRITE);
-	if (ret < 0)
-		goto out;
-
-	if (alrm->enabled)
-		ret = max77802_rtc_start_alarm(info);
-out:
-	mutex_unlock(&info->lock);
-	return ret;
-}
-
-static int max77802_rtc_alarm_irq_enable(struct device *dev,
-					 unsigned int enabled)
-{
-	struct max77802_rtc_info *info = dev_get_drvdata(dev);
-	int ret;
-
-	mutex_lock(&info->lock);
-	if (enabled)
-		ret = max77802_rtc_start_alarm(info);
-	else
-		ret = max77802_rtc_stop_alarm(info);
-	mutex_unlock(&info->lock);
-
-	return ret;
-}
-
-static irqreturn_t max77802_rtc_alarm_irq(int irq, void *data)
-{
-	struct max77802_rtc_info *info = data;
-
-	dev_dbg(info->dev, "%s:irq(%d)\n", __func__, irq);
-
-	rtc_update_irq(info->rtc_dev, 1, RTC_IRQF | RTC_AF);
-
-	return IRQ_HANDLED;
-}
-
-static const struct rtc_class_ops max77802_rtc_ops = {
-	.read_time = max77802_rtc_read_time,
-	.set_time = max77802_rtc_set_time,
-	.read_alarm = max77802_rtc_read_alarm,
-	.set_alarm = max77802_rtc_set_alarm,
-	.alarm_irq_enable = max77802_rtc_alarm_irq_enable,
-};
-
-static int max77802_rtc_init_reg(struct max77802_rtc_info *info)
-{
-	u8 data[2];
-	int ret;
-
-	max77802_rtc_update(info, MAX77802_RTC_READ);
-
-	/* Set RTC control register : Binary mode, 24hour mdoe */
-	data[0] = (1 << BCD_EN_SHIFT) | (1 << MODEL24_SHIFT);
-	data[1] = (0 << BCD_EN_SHIFT) | (1 << MODEL24_SHIFT);
-
-	info->rtc_24hr_mode = 1;
-
-	ret = regmap_bulk_write(info->max77802->regmap,
-				MAX77802_RTC_CONTROLM, data, ARRAY_SIZE(data));
-	if (ret < 0) {
-		dev_err(info->dev, "%s: fail to write controlm reg(%d)\n",
-				__func__, ret);
-		return ret;
-	}
-
-	ret = max77802_rtc_update(info, MAX77802_RTC_WRITE);
-	return ret;
-}
-
-static int max77802_rtc_probe(struct platform_device *pdev)
-{
-	struct max77686_dev *max77802 = dev_get_drvdata(pdev->dev.parent);
-	struct max77802_rtc_info *info;
-	int ret;
-
-	dev_dbg(&pdev->dev, "%s\n", __func__);
-
-	info = devm_kzalloc(&pdev->dev, sizeof(struct max77802_rtc_info),
-			    GFP_KERNEL);
-	if (!info)
-		return -ENOMEM;
-
-	mutex_init(&info->lock);
-	info->dev = &pdev->dev;
-	info->max77802 = max77802;
-	info->rtc = max77802->i2c;
-
-	platform_set_drvdata(pdev, info);
-
-	ret = max77802_rtc_init_reg(info);
-
-	if (ret < 0) {
-		dev_err(&pdev->dev, "Failed to initialize RTC reg:%d\n", ret);
-		return ret;
-	}
-
-	device_init_wakeup(&pdev->dev, 1);
-
-	info->rtc_dev = devm_rtc_device_register(&pdev->dev, "max77802-rtc",
-						 &max77802_rtc_ops, THIS_MODULE);
-
-	if (IS_ERR(info->rtc_dev)) {
-		ret = PTR_ERR(info->rtc_dev);
-		dev_err(&pdev->dev, "Failed to register RTC device: %d\n", ret);
-		if (ret == 0)
-			ret = -EINVAL;
-		return ret;
-	}
-
-	if (!max77802->rtc_irq_data) {
-		dev_err(&pdev->dev, "No RTC regmap IRQ chip\n");
-		return -EINVAL;
-	}
-
-	info->virq = regmap_irq_get_virq(max77802->rtc_irq_data,
-					 MAX77686_RTCIRQ_RTCA1);
-
-	if (info->virq <= 0) {
-		dev_err(&pdev->dev, "Failed to get virtual IRQ %d\n",
-			MAX77686_RTCIRQ_RTCA1);
-		return -EINVAL;
-	}
-
-	ret = devm_request_threaded_irq(&pdev->dev, info->virq, NULL,
-					max77802_rtc_alarm_irq, 0, "rtc-alarm1",
-					info);
-	if (ret < 0)
-		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
-			info->virq, ret);
-
-	return ret;
-}
-
-#ifdef CONFIG_PM_SLEEP
-static int max77802_rtc_suspend(struct device *dev)
-{
-	if (device_may_wakeup(dev)) {
-		struct max77802_rtc_info *info = dev_get_drvdata(dev);
-
-		return enable_irq_wake(info->virq);
-	}
-
-	return 0;
-}
-
-static int max77802_rtc_resume(struct device *dev)
-{
-	if (device_may_wakeup(dev)) {
-		struct max77802_rtc_info *info = dev_get_drvdata(dev);
-
-		return disable_irq_wake(info->virq);
-	}
-
-	return 0;
-}
-#endif
-
-static SIMPLE_DEV_PM_OPS(max77802_rtc_pm_ops,
-			 max77802_rtc_suspend, max77802_rtc_resume);
-
-static const struct platform_device_id rtc_id[] = {
-	{ "max77802-rtc", 0 },
-	{},
-};
-MODULE_DEVICE_TABLE(platform, rtc_id);
-
-static struct platform_driver max77802_rtc_driver = {
-	.driver		= {
-		.name	= "max77802-rtc",
-		.pm	= &max77802_rtc_pm_ops,
-	},
-	.probe		= max77802_rtc_probe,
-	.id_table	= rtc_id,
-};
-
-module_platform_driver(max77802_rtc_driver);
-
-MODULE_DESCRIPTION("Maxim MAX77802 RTC driver");
-MODULE_AUTHOR("Simon Glass <sjg@chromium.org>");
-MODULE_LICENSE("GPL");
-- 
2.5.0

[PATCH 7/8] ARM: exynos_defconfig: Remove MAX77802 RTC Kconfig symbol

[Javier Martinez Canillas]

The driver has been removed so the Kconfig symbol is not valid anymore.

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

 arch/arm/configs/exynos_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/configs/exynos_defconfig b/arch/arm/configs/exynos_defconfig
index 24dcd2bb1215..cdbe6dbc6e75 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -193,7 +193,6 @@ CONFIG_LEDS_TRIGGER_HEARTBEAT=y
 CONFIG_RTC_CLASS=y
 CONFIG_RTC_DRV_MAX8997=y
 CONFIG_RTC_DRV_MAX77686=y
-CONFIG_RTC_DRV_MAX77802=y
 CONFIG_RTC_DRV_S5M=y
 CONFIG_RTC_DRV_S3C=y
 CONFIG_DMADEVICES=y
-- 
2.5.0

[PATCH 8/8] ARM: multi_v7_defconfig: Remove MAX77802 RTC Kconfig symbol

[Javier Martinez Canillas]

The driver has been removed so the Kconfig symbol is not valid anymore.

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


---

 arch/arm/configs/multi_v7_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 314f6be2dca2..bdb42c09332c 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -664,7 +664,6 @@ CONFIG_RTC_DRV_MAX8907=y
 CONFIG_RTC_DRV_MAX8997=m
 CONFIG_RTC_DRV_MAX77686=y
 CONFIG_RTC_DRV_RK808=m
-CONFIG_RTC_DRV_MAX77802=m
 CONFIG_RTC_DRV_RS5C372=m
 CONFIG_RTC_DRV_PALMAS=y
 CONFIG_RTC_DRV_ST_LPC=y
-- 
2.5.0

Re: [PATCH 0/8] rtc: max77686: Extend driver and add max77802 support

[Alexandre Belloni]

Hi,

On 20/01/2016 at 14:14:40 -0300, Javier Martinez Canillas wrote :
> One thing that I'm not sure is how to handle bisectability, in patch #5
> there will be two drivers that matches "rtc-max77802". So I don't know
> if I should use a different platform_device_id name and change in the
> same patch that max77802 is removed or if this is not a big deal so the
> patches could stay as is.
> 

I'm fine with those patches as is.

> I believe all patches should go through the RTC tree with proper acks or
> wait until the RTC patches land to pick the defconfig changes.
> 

I think Olof would prefer the last patches to go through arm-soc.

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

Re: [PATCH 0/8] rtc: max77686: Extend driver and add max77802 support

[Krzysztof Kozlowski]

On 21.01.2016 09:30, Alexandre Belloni wrote:
>> I believe all patches should go through the RTC tree with proper acks or
>> wait until the RTC patches land to pick the defconfig changes.
>>
> 
> I think Olof would prefer the last patches to go through arm-soc.

That would be preferred but merging them before the 5/8 would cause a
loss of functionality on these defconfigs making it non-bisectable
approach. I think it would be good to preserve bisectability in that
matter so either:
1. a tag from RTC on top of which these patches would be applied in arm-soc,
2. take them to RTC tree with our acks.

Best regards,
Krzysztof

Re: [PATCH 1/8] rtc: max77686: Use ARRAY_SIZE() instead of current array length

[Krzysztof Kozlowski]

On 21.01.2016 02:14, Javier Martinez Canillas wrote:
> It is better to use the ARRAY_SIZE() macro instead of the array length
> to avoid bugs if the array is later changed and the length not updated.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/rtc/rtc-max77686.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

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

Best regards,
Krzysztof

Re: [PATCH 2/8] rtc: max77686: Use usleep_range() instead of msleep()

[Krzysztof Kozlowski]

On 21.01.2016 02:14, Javier Martinez Canillas wrote:
> Documentation/timers/timers-howto.txt suggest to use usleep_range()
> instead of msleep() for small msec (1ms - 20ms) since msleep() will
> often sleep for 20ms for any value in that range.
> 
> This is fine in this case since 16ms is the _minimum_ delay required
> by max77686 for an RTC update but by using usleep_range() instead of
> msleep(), the driver can support other RTC IP blocks with a shorter
> minium delay (i.e: in the range of usecs insted of msecs).

s/minium/minimum/
s/insted/instead/

> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/rtc/rtc-max77686.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

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

Best regards,
Krzysztof

Re: [PATCH 3/8] rtc: max77686: Use a driver data struct instead hard-coded values

[Krzysztof Kozlowski]

On 21.01.2016 02:14, Javier Martinez Canillas wrote:
> The driver has some hard-coded values such as the minimum delay needed
> before a RTC update or the mask used for the sec/min/hour/etc registers.
> 
> Use a data structure that contains these values and pass as driver data
> using the platform device ID table for each device.
> 
> This allows to make the driver's ops callbacks more generic so other RTC
> that are similar but don't have the same values can also be supported.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/rtc/rtc-max77686.c | 47 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index ae4d61e7ce4b..441d163dcbeb 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -41,8 +41,6 @@
>  #define ALARM_ENABLE_SHIFT		7
>  #define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
>  
> -#define MAX77686_RTC_UPDATE_DELAY	16000
> -
>  enum {
>  	RTC_SEC = 0,
>  	RTC_MIN,
> @@ -54,6 +52,11 @@ enum {
>  	RTC_NR_TIME
>  };
>  
> +struct rtc_driver_data {

1. This is only local structure so maybe: max77686_rtc_driver_data?

2. Can you put here a comment explaining the purpose of this delay
(quite generic name) and used units?

> +	unsigned long		delay;

A comment here - what 'mask'? Generic name so explanation would be welcomed.

> +	int			mask;

I think this should be u8 to avoid early promotions to int. It is used
directly with other u8 values. At the end effect of operation is
promoted to int anyway but using only u8 for operations looks more
consistent to me.

> +};
> +
>  struct max77686_rtc_info {
>  	struct device		*dev;
>  	struct max77686_dev	*max77686;
> @@ -63,6 +66,8 @@ struct max77686_rtc_info {
>  
>  	struct regmap		*regmap;
>  
> +	struct rtc_driver_data	*drv_data;

That may be pointer to const data. Actually not "may be" but
even "should be" because you allocate later really a const data and you
are discarding const-ness with cast.

> +
>  	int virq;
>  	int rtc_24hr_mode;
>  };
> @@ -72,12 +77,19 @@ enum MAX77686_RTC_OP {
>  	MAX77686_RTC_READ,
>  };
>  
> +static const struct rtc_driver_data max77686_drv_data = {
> +	.delay = 1600,
> +	.mask  = 0x7f,
> +};
> +
>  static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
> -				   int rtc_24hr_mode)
> +				    struct max77686_rtc_info *info)
>  {
> -	tm->tm_sec = data[RTC_SEC] & 0x7f;
> -	tm->tm_min = data[RTC_MIN] & 0x7f;
> -	if (rtc_24hr_mode)
> +	int mask = info->drv_data->mask;
> +
> +	tm->tm_sec = data[RTC_SEC] & mask;
> +	tm->tm_min = data[RTC_MIN] & mask;
> +	if (info->rtc_24hr_mode)
>  		tm->tm_hour = data[RTC_HOUR] & 0x1f;
>  	else {
>  		tm->tm_hour = data[RTC_HOUR] & 0x0f;
> @@ -86,10 +98,10 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
>  	}
>  
>  	/* Only a single bit is set in data[], so fls() would be equivalent */
> -	tm->tm_wday = ffs(data[RTC_WEEKDAY] & 0x7f) - 1;
> +	tm->tm_wday = ffs(data[RTC_WEEKDAY] & mask) - 1;
>  	tm->tm_mday = data[RTC_DATE] & 0x1f;
>  	tm->tm_mon = (data[RTC_MONTH] & 0x0f) - 1;
> -	tm->tm_year = (data[RTC_YEAR] & 0x7f) + 100;
> +	tm->tm_year = (data[RTC_YEAR] & mask) + 100;
>  	tm->tm_yday = 0;
>  	tm->tm_isdst = 0;
>  }
> @@ -117,6 +129,7 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
>  {
>  	int ret;
>  	unsigned int data;
> +	unsigned long delay = info->drv_data->delay;
>  
>  	if (op == MAX77686_RTC_WRITE)
>  		data = 1 << RTC_UDR_SHIFT;
> @@ -129,9 +142,8 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
>  		dev_err(info->dev, "%s: fail to write update reg(ret=%d, data=0x%x)\n",
>  				__func__, ret, data);
>  	else {
> -		/* Minimum 16ms delay required before RTC update. */
> -		usleep_range(MAX77686_RTC_UPDATE_DELAY,
> -			     MAX77686_RTC_UPDATE_DELAY * 2);
> +		/* Minimum delay required before RTC update. */
> +		usleep_range(delay, delay * 2);
>  	}
>  
>  	return ret;
> @@ -156,7 +168,7 @@ static int max77686_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  		goto out;
>  	}
>  
> -	max77686_rtc_data_to_tm(data, tm, info->rtc_24hr_mode);
> +	max77686_rtc_data_to_tm(data, tm, info);
>  
>  	ret = rtc_valid_tm(tm);
>  
> @@ -213,7 +225,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  		goto out;
>  	}
>  
> -	max77686_rtc_data_to_tm(data, &alrm->time, info->rtc_24hr_mode);
> +	max77686_rtc_data_to_tm(data, &alrm->time, info);
>  
>  	alrm->enabled = 0;
>  	for (i = 0; i < RTC_NR_TIME; i++) {
> @@ -260,7 +272,7 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
>  		goto out;
>  	}
>  
> -	max77686_rtc_data_to_tm(data, &tm, info->rtc_24hr_mode);
> +	max77686_rtc_data_to_tm(data, &tm, info);
>  
>  	for (i = 0; i < RTC_NR_TIME; i++)
>  		data[i] &= ~ALARM_ENABLE_MASK;
> @@ -299,7 +311,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
>  		goto out;
>  	}
>  
> -	max77686_rtc_data_to_tm(data, &tm, info->rtc_24hr_mode);
> +	max77686_rtc_data_to_tm(data, &tm, info);
>  
>  	data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
>  	data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
> @@ -307,7 +319,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
>  	data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
>  	if (data[RTC_MONTH] & 0xf)
>  		data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
> -	if (data[RTC_YEAR] & 0x7f)
> +	if (data[RTC_YEAR] & info->drv_data->mask)
>  		data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
>  	if (data[RTC_DATE] & 0x1f)
>  		data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
> @@ -436,6 +448,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  	info->dev = &pdev->dev;
>  	info->max77686 = max77686;
>  	info->rtc = max77686->rtc;
> +	info->drv_data = (struct rtc_driver_data *)pdev->id_entry->driver_data;

That cast dropping constness is bad.

Best regards,
Krzysztof

>  
>  	platform_set_drvdata(pdev, info);
>  
> @@ -510,7 +523,7 @@ static SIMPLE_DEV_PM_OPS(max77686_rtc_pm_ops,
>  			 max77686_rtc_suspend, max77686_rtc_resume);
>  
>  static const struct platform_device_id rtc_id[] = {
> -	{ "max77686-rtc", 0 },
> +	{ "max77686-rtc", .driver_data = (kernel_ulong_t)&max77686_drv_data, },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(platform, rtc_id);
> 

Re: [PATCH 0/8] rtc: max77686: Extend driver and add max77802 support

[Krzysztof Kozlowski]

On 21.01.2016 02:14, Javier Martinez Canillas wrote:
> Hello,
> 
> On a recent disussion [0] with Krzysztof Kozlowski and Laxman Dewangan,
> we came to the conclusion that the max77686 and max77802 RTC are almost
> the same with only a few differences so there shouldn't be two separate
> drivers and is better to extend max77686 driver and delete rtc-max77802.
> 
> By making the driver more generic, other RTC IP blocks from Maxim PMICs
> could be supported as well like the max77620.
> 
> Patches #1 is just a trivial cleanup.
> 
> Patch #2 allows to support RTCs that need a shorter delay when updating
> the RTC.
> 
> Patch #3 adds a driver data structure to avoid hard-coding parameters
> specific to a certain RTC such as the needed delay and RTC register mask.
> 
> Patch #4 changes the driver to use a mapping table instead of using the
> max77686 registers offsets directly to allow supporting RTC with other
> registers addresses and layout.
> 
> Patch #5 Adds support for max77802 to max77686 RTC driver and patch #6
> removes the old driver since is not needed anymore.
> 
> Finally patch #7 and patch #8 removes the Kconfig symbol from defconfigs.
> 
> I've tested this patch-set on an Exynos5800 Peach Pi Chromebook that has
> a max77802 PMIC and the RTC was working correctly but I don't have a
> machine with max77686 so I will really appreaciate if someone can test
> that no regressions were introduced.
> 

Thanks for the submission. I like the approach. I'll start reviewing the
code and testing it... or maybe I'll wait with testing for v2 because I
already sent some comments. :)

Anyway I will provide later tested-by on max77686.

Best regards,
Krzysztof

Re: [PATCH 4/8] rtc: max77686: Add an indirection level to access RTC registers

[Krzysztof Kozlowski]

On 21.01.2016 02:14, Javier Martinez Canillas wrote:
> The max77686 driver is generic enough that can be used for other
> Maxim RTC IP blocks but these might not have the same registers
> layout so instead of accessing the registers directly, add a map
> to translate offsets to the real registers addresses for each IP.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/rtc/rtc-max77686.c | 75 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 65 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index 441d163dcbeb..7316e41820c7 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -41,6 +41,8 @@
>  #define ALARM_ENABLE_SHIFT		7
>  #define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
>  
> +#define REG_RTC_NONE			0xdeadbeef
> +
>  enum {
>  	RTC_SEC = 0,
>  	RTC_MIN,
> @@ -55,6 +57,7 @@ enum {
>  struct rtc_driver_data {
>  	unsigned long		delay;
>  	int			mask;
> +	const unsigned int	*map;
>  };
>  
>  struct max77686_rtc_info {
> @@ -77,9 +80,53 @@ enum MAX77686_RTC_OP {
>  	MAX77686_RTC_READ,
>  };
>  
> +/* These are not registers but just offsets that are mapped to addresses */
> +enum rtc_reg {

enum max77686_rtc_reg_offset?

> +	REG_RTC_CONTROLM = 0,
> +	REG_RTC_CONTROL,
> +	REG_RTC_UPDATE0,
> +	REG_RTC_UPDATE1,
> +	REG_WTSR_SMPL_CNTL,
> +	REG_RTC_SEC,
> +	REG_RTC_MIN,
> +	REG_RTC_HOUR,
> +	REG_RTC_WEEKDAY,
> +	REG_RTC_MONTH,
> +	REG_RTC_YEAR,
> +	REG_RTC_DATE,
> +	REG_ALARM1_SEC,
> +	REG_ALARM1_MIN,
> +	REG_ALARM1_HOUR,
> +	REG_ALARM1_WEEKDAY,
> +	REG_ALARM1_MONTH,
> +	REG_ALARM1_YEAR,
> +	REG_ALARM1_DATE,
> +	REG_ALARM2_SEC,
> +	REG_ALARM2_MIN,
> +	REG_ALARM2_HOUR,
> +	REG_ALARM2_WEEKDAY,
> +	REG_ALARM2_MONTH,
> +	REG_ALARM2_YEAR,
> +	REG_ALARM2_DATE,
> +	REG_RTC_END,
> +};
> +

A short comment what is mapped into what would be appreciated.

> +static const unsigned int max77686_map[REG_RTC_END] = {
> +	MAX77686_RTC_CONTROLM, MAX77686_RTC_CONTROL, MAX77686_RTC_UPDATE0,
> +	REG_RTC_NONE, MAX77686_WTSR_SMPL_CNTL, MAX77686_RTC_SEC,
> +	MAX77686_RTC_MIN, MAX77686_RTC_HOUR, MAX77686_RTC_WEEKDAY,
> +	MAX77686_RTC_MONTH, MAX77686_RTC_YEAR, MAX77686_RTC_DATE,
> +	MAX77686_ALARM1_SEC, MAX77686_ALARM1_MIN, MAX77686_ALARM1_HOUR,
> +	MAX77686_ALARM1_WEEKDAY, MAX77686_ALARM1_MONTH, MAX77686_ALARM1_YEAR,
> +	MAX77686_ALARM1_DATE, MAX77686_ALARM2_SEC, MAX77686_ALARM2_MIN,
> +	MAX77686_ALARM2_HOUR, MAX77686_ALARM2_WEEKDAY, MAX77686_ALARM2_MONTH,
> +	MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE,
> +};

It is difficult to check for mistakes here. I would prefer direct mapping:
	[REG_RTC_CONTROLM] = MAX77686_RTC_CONTROLM,
	....


Rest looks good but I did not check the correctness of mapping above.

BR,
Krzysztof

Re: [PATCH 5/8] rtc: max77686: Add max77802 support

[Krzysztof Kozlowski]

On 21.01.2016 02:14, Javier Martinez Canillas wrote:
> The MAX77686 and MAX77802 RTC IP blocks are very similar with only
> these differences:
> 
> 0) The RTC registers layout and addresses are different.
> 
> 1) The MAX77686 use 1 bit of the sec/min/hour/etc registers as the
>    alarm enable while MAX77802 has a separate register for that.
> 
> 2) The MAX77686 RTCYEAR register valid values range is 0..99 while
>    for MAX77802 is 0..199.
> 
> 3) The MAX77686 has a separate I2C address for the RTC registers
>    while the MAX77802 uses the same I2C address as the PMIC regs.
> 
> 5) They minium delay before a RTC update (16ms vs 200 usecs).
> 
> There are separate drivers for MAX77686 and MAX77802 RTC IP blocks
> but the differences are not that big so the driver can be extended
> to support both instead of duplicating a lot of code in 2 drivers.
> 
> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/rtc/rtc-max77686.c | 176 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 128 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index 7316e41820c7..7a144e7ecd27 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -1,5 +1,5 @@
>  /*
> - * RTC driver for Maxim MAX77686
> + * RTC driver for Maxim MAX77686 and MAX77802
>   *
>   * Copyright (C) 2012 Samsung Electronics Co.Ltd
>   *
> @@ -43,6 +43,13 @@
>  
>  #define REG_RTC_NONE			0xdeadbeef
>  
> +/*
> + * MAX77802 has separate register (RTCAE1) for alarm enable instead
> + * using 1 bit from registers RTC{SEC,MIN,HOUR,DAY,MONTH,YEAR,DATE}
> + * as in done in MAX77686.
> + */
> +#define ALARM_ENABLE_VALUE		0x77

MAX77802_ALARM_ENABLE_VALUE
(it is specific to 77802, right?)

> +
>  enum {
>  	RTC_SEC = 0,
>  	RTC_MIN,
> @@ -58,6 +65,10 @@ struct rtc_driver_data {
>  	unsigned long		delay;
>  	int			mask;
>  	const unsigned int	*map;
> +	/* Has a separate alarm enable register? */
> +	bool			rtcae;
> +	/* Has a separate I2C regmap for the RTC? */
> +	bool			rtcrm;

Both members are a tongue twisters. :)

'rtcae' you are mostly using in an inverted way (!rtcae) so how about:
'alarm_enable_bit'?

'rtcrm' - 'separate_i2c_addr'?

By the way, I was thinking that you would do decoupling of i2c and
regmap here. It is not required (more useful for Laxman's patch) but it
might by a part of these series.

>  };
>  
>  struct max77686_rtc_info {
> @@ -108,6 +119,8 @@ enum rtc_reg {
>  	REG_ALARM2_MONTH,
>  	REG_ALARM2_YEAR,
>  	REG_ALARM2_DATE,
> +	REG_RTC_AE1,
> +	REG_RTC_AE2,
>  	REG_RTC_END,
>  };
>  
> @@ -120,13 +133,36 @@ static const unsigned int max77686_map[REG_RTC_END] = {
>  	MAX77686_ALARM1_WEEKDAY, MAX77686_ALARM1_MONTH, MAX77686_ALARM1_YEAR,
>  	MAX77686_ALARM1_DATE, MAX77686_ALARM2_SEC, MAX77686_ALARM2_MIN,
>  	MAX77686_ALARM2_HOUR, MAX77686_ALARM2_WEEKDAY, MAX77686_ALARM2_MONTH,
> -	MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE,
> +	MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE, REG_RTC_NONE, REG_RTC_NONE,
>  };
>  
>  static const struct rtc_driver_data max77686_drv_data = {
>  	.delay = 1600,
>  	.mask  = 0x7f,
>  	.map   = max77686_map,
> +	.rtcae = false,
> +	.rtcrm = true,
> +};
> +
> +static const unsigned int max77802_map[REG_RTC_END] = {
> +	MAX77802_RTC_CONTROLM, MAX77802_RTC_CONTROL, MAX77802_RTC_UPDATE0,
> +	REG_RTC_NONE, MAX77802_WTSR_SMPL_CNTL, MAX77802_RTC_SEC,
> +	MAX77802_RTC_MIN, MAX77802_RTC_HOUR, MAX77802_RTC_WEEKDAY,
> +	MAX77802_RTC_MONTH, MAX77802_RTC_YEAR, MAX77802_RTC_DATE,
> +	MAX77802_ALARM1_SEC, MAX77802_ALARM1_MIN, MAX77802_ALARM1_HOUR,
> +	MAX77686_ALARM1_WEEKDAY, MAX77802_ALARM1_MONTH, MAX77802_ALARM1_YEAR,
> +	MAX77802_ALARM1_DATE, MAX77802_ALARM1_SEC, MAX77802_ALARM1_MIN,
> +	MAX77802_ALARM1_HOUR, MAX77802_ALARM1_WEEKDAY, MAX77802_ALARM1_MONTH,
> +	MAX77802_ALARM1_YEAR, MAX77802_ALARM1_DATE, MAX77802_RTC_AE1,
> +	MAX77802_RTC_AE2,
> +};
> +
> +static const struct rtc_driver_data max77802_drv_data = {
> +	.delay = 200,
> +	.mask  = 0xff,
> +	.map   = max77802_map,
> +	.rtcae = true,
> +	.rtcrm = false,
>  };
>  
>  static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
> @@ -148,12 +184,20 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
>  	tm->tm_wday = ffs(data[RTC_WEEKDAY] & mask) - 1;
>  	tm->tm_mday = data[RTC_DATE] & 0x1f;
>  	tm->tm_mon = (data[RTC_MONTH] & 0x0f) - 1;
> -	tm->tm_year = (data[RTC_YEAR] & mask) + 100;
> +	tm->tm_year = data[RTC_YEAR] & mask;
>  	tm->tm_yday = 0;
>  	tm->tm_isdst = 0;
> +
> +	/*
> +	 * MAX77686 uses 1 bit from sec/min/hour/etc RTC registers and the
> +	 * year values are just 0..99 so add 100 to support up to 2099.
> +	 */
> +	if (!info->drv_data->rtcae)
> +		tm->tm_year += 100;
>  }
>  
> -static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data)
> +static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data,
> +				   struct max77686_rtc_info *info)
>  {
>  	data[RTC_SEC] = tm->tm_sec;
>  	data[RTC_MIN] = tm->tm_min;
> @@ -161,13 +205,19 @@ static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data)
>  	data[RTC_WEEKDAY] = 1 << tm->tm_wday;
>  	data[RTC_DATE] = tm->tm_mday;
>  	data[RTC_MONTH] = tm->tm_mon + 1;
> -	data[RTC_YEAR] = tm->tm_year > 100 ? (tm->tm_year - 100) : 0;
>  
> -	if (tm->tm_year < 100) {
> -		pr_warn("RTC cannot handle the year %d.  Assume it's 2000.\n",
> -			1900 + tm->tm_year);
> -		return -EINVAL;
> +	if (!info->drv_data->rtcae) {
> +		data[RTC_YEAR] = tm->tm_year > 100 ? (tm->tm_year - 100) : 0;
> +
> +		if (tm->tm_year < 100) {
> +			pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
> +				1900 + tm->tm_year);

Maybe in a separate patch use dev_warn()? It wasn't possible before
because you need 'info' argument but it is possible.

> +			return -EINVAL;
> +		}
> +	} else {
> +		data[RTC_YEAR] = tm->tm_year;
>  	}
> +
>  	return 0;
>  }
>  
> @@ -232,7 +282,7 @@ static int max77686_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	u8 data[RTC_NR_TIME];
>  	int ret;
>  
> -	ret = max77686_rtc_tm_to_data(tm, data);
> +	ret = max77686_rtc_tm_to_data(tm, data, info);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -279,11 +329,24 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	max77686_rtc_data_to_tm(data, &alrm->time, info);
>  
>  	alrm->enabled = 0;
> -	for (i = 0; i < RTC_NR_TIME; i++) {
> -		if (data[i] & ALARM_ENABLE_MASK) {
> -			alrm->enabled = 1;
> -			break;
> +
> +	if (!info->drv_data->rtcae) {
> +		for (i = 0; i < RTC_NR_TIME; i++) {
> +			if (data[i] & ALARM_ENABLE_MASK) {
> +				alrm->enabled = 1;
> +				break;
> +			}
>  		}
> +	} else {
> +		ret = regmap_read(info->max77686->regmap,
> +				  map[REG_RTC_AE1], &val);
> +		if (ret < 0) {
> +			dev_err(info->dev, "%s:%d fail to read alarm enable(%d)\n",
> +				__func__, __LINE__, ret);

I don't like the func/LINE. I know that driver is using them already but
I think it is better not to add new usages of it.

> +			goto out;

Actually I think there is a bug here already. The function will always
return '0'. Instead the 'out' label should return 'ret'. Can you fix it
in separate patch (with reported-by :) )?

> +		}
> +		if (val)
> +			alrm->enabled = 1;
>  	}
>  
>  	alrm->pending = 0;
> @@ -316,21 +379,27 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
>  	if (ret < 0)
>  		goto out;
>  
> -	ret = regmap_bulk_read(info->max77686->rtc_regmap,
> -			       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> -	if (ret < 0) {
> -		dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
> +	if (!info->drv_data->rtcae) {
> +		ret = regmap_bulk_read(info->max77686->rtc_regmap,
> +				       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> +		if (ret < 0) {
> +			dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
>  				__func__, ret);
> -		goto out;
> -	}
> +			goto out;
> +		}
>  
> -	max77686_rtc_data_to_tm(data, &tm, info);
> +		max77686_rtc_data_to_tm(data, &tm, info);
>  
> -	for (i = 0; i < RTC_NR_TIME; i++)
> -		data[i] &= ~ALARM_ENABLE_MASK;
> +		for (i = 0; i < RTC_NR_TIME; i++)
> +			data[i] &= ~ALARM_ENABLE_MASK;
> +
> +		ret = regmap_bulk_write(info->max77686->rtc_regmap,
> +					map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> +	} else {
> +		ret = regmap_write(info->max77686->regmap,
> +				   map[REG_RTC_AE1], 0);
> +	}
>  
> -	ret = regmap_bulk_write(info->max77686->rtc_regmap,
> -				map[REG_ALARM1_SEC], data, RTC_NR_TIME);
>  	if (ret < 0) {
>  		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
>  				__func__, ret);
> @@ -356,29 +425,35 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
>  	if (ret < 0)
>  		goto out;
>  
> -	ret = regmap_bulk_read(info->max77686->rtc_regmap,
> -			       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> -	if (ret < 0) {
> -		dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
> +	if (!info->drv_data->rtcae) {
> +		ret = regmap_bulk_read(info->max77686->rtc_regmap,
> +				       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> +		if (ret < 0) {
> +			dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
>  				__func__, ret);
> -		goto out;
> -	}
> -
> -	max77686_rtc_data_to_tm(data, &tm, info);
> +			goto out;
> +		}
>  
> -	data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
> -	data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
> -	data[RTC_HOUR] |= (1 << ALARM_ENABLE_SHIFT);
> -	data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
> -	if (data[RTC_MONTH] & 0xf)
> -		data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
> -	if (data[RTC_YEAR] & info->drv_data->mask)
> -		data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
> -	if (data[RTC_DATE] & 0x1f)
> -		data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
> +		max77686_rtc_data_to_tm(data, &tm, info);
> +
> +		data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
> +		data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
> +		data[RTC_HOUR] |= (1 << ALARM_ENABLE_SHIFT);
> +		data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
> +		if (data[RTC_MONTH] & 0xf)
> +			data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
> +		if (data[RTC_YEAR] & info->drv_data->mask)
> +			data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
> +		if (data[RTC_DATE] & 0x1f)
> +			data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
> +
> +		ret = regmap_bulk_write(info->max77686->rtc_regmap,
> +					map[REG_ALARM1_SEC], data, RTC_NR_TIME);
> +	} else {
> +		ret = regmap_write(info->max77686->regmap,
> +				   map[REG_RTC_AE1], ALARM_ENABLE_VALUE);
> +	}
>  
> -	ret = regmap_bulk_write(info->max77686->rtc_regmap,
> -				map[REG_ALARM1_SEC], data, RTC_NR_TIME);
>  	if (ret < 0) {
>  		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
>  				__func__, ret);
> @@ -396,7 +471,7 @@ static int max77686_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	u8 data[RTC_NR_TIME];
>  	int ret;
>  
> -	ret = max77686_rtc_tm_to_data(&alrm->time, data);
> +	ret = max77686_rtc_tm_to_data(&alrm->time, data, info);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -490,6 +565,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  {
>  	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
>  	struct max77686_rtc_info *info;
> +	const struct platform_device_id *id = pdev->id_entry;
>  	int ret;
>  
>  	dev_info(&pdev->dev, "%s\n", __func__);
> @@ -503,7 +579,10 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  	info->dev = &pdev->dev;
>  	info->max77686 = max77686;
>  	info->rtc = max77686->rtc;
> -	info->drv_data = (struct rtc_driver_data *)pdev->id_entry->driver_data;

Comment for previous patch: use platform_get_device_id(pdev).

Best regards,
Krzysztof

Re: [PATCH 6/8] rtc: Remove Maxim 77802 driver

[Krzysztof Kozlowski]

On 21.01.2016 02:14, Javier Martinez Canillas wrote:
> The max77686 RTC driver now supports the max77802 RTC as
> well so there's no need to have a separate driver anymore.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/rtc/Kconfig        |  10 -
>  drivers/rtc/Makefile       |   1 -
>  drivers/rtc/rtc-max77802.c | 502 ---------------------------------------------
>  3 files changed, 513 deletions(-)
>  delete mode 100644 drivers/rtc/rtc-max77802.c
> 

Oh yes, I like removals so much! :)

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

Best regards,
Krzysztof

Re: [PATCH 7/8] ARM: exynos_defconfig: Remove MAX77802 RTC Kconfig symbol

[Krzysztof Kozlowski]

On 21.01.2016 02:14, Javier Martinez Canillas wrote:
> The driver has been removed so the Kconfig symbol is not valid anymore.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  arch/arm/configs/exynos_defconfig | 1 -
>  1 file changed, 1 deletion(-)

The patch should go after merging rtc-max77802 into rtc-max77686 to
preserve bisectability on defconfig.

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

Best regards,
Krzysztof

Re: [PATCH 8/8] ARM: multi_v7_defconfig: Remove MAX77802 RTC Kconfig symbol

[Krzysztof Kozlowski]

On 21.01.2016 02:14, Javier Martinez Canillas wrote:
> The driver has been removed so the Kconfig symbol is not valid anymore.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> 
> ---
> 
>  arch/arm/configs/multi_v7_defconfig | 1 -
>  1 file changed, 1 deletion(-)

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

Best regards,
Krzysztof

Re: [PATCH 0/8] rtc: max77686: Extend driver and add max77802 support

[Javier Martinez Canillas]

Hello,

On 01/20/2016 09:34 PM, Krzysztof Kozlowski wrote:
> On 21.01.2016 09:30, Alexandre Belloni wrote:
>>> I believe all patches should go through the RTC tree with proper acks or
>>> wait until the RTC patches land to pick the defconfig changes.
>>>
>>
>> I think Olof would prefer the last patches to go through arm-soc.
>
> That would be preferred but merging them before the 5/8 would cause a
> loss of functionality on these defconfigs making it non-bisectable

Exactly, that's why I suggested merging all through RTC with proper acks.

> approach. I think it would be good to preserve bisectability in that
> matter so either:
> 1. a tag from RTC on top of which these patches would be applied in arm-soc,

I didn't suggest that option because I thought it would be a lot of burden
for such a trivial change.

> 2. take them to RTC tree with our acks.
>

Or another option is to not pick the defconfig changes and I can repost
those again once the RTC changes land into mainline.

But of course if up to you to decide since I'm not a maintainer :)
  
> Best regards,
> Krzysztof
>

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

Re: [PATCH 2/8] rtc: max77686: Use usleep_range() instead of msleep()

[Javier Martinez Canillas]

Hello Krzysztof,

On 01/20/2016 09:37 PM, Krzysztof Kozlowski wrote:
> On 21.01.2016 02:14, Javier Martinez Canillas wrote:
>> Documentation/timers/timers-howto.txt suggest to use usleep_range()
>> instead of msleep() for small msec (1ms - 20ms) since msleep() will
>> often sleep for 20ms for any value in that range.
>>
>> This is fine in this case since 16ms is the _minimum_ delay required
>> by max77686 for an RTC update but by using usleep_range() instead of
>> msleep(), the driver can support other RTC IP blocks with a shorter
>> minium delay (i.e: in the range of usecs insted of msecs).
>
> s/minium/minimum/
> s/insted/instead/
>

Fixed.

>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> ---
>>
>>   drivers/rtc/rtc-max77686.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>

Thanks a lot for your review.
  
> Best regards,
> Krzysztof
>

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

Re: [PATCH 3/8] rtc: max77686: Use a driver data struct instead hard-coded values

[Javier Martinez Canillas]

Hello Krzysztof,

Thanks a lot for your review.

On 01/20/2016 09:45 PM, Krzysztof Kozlowski wrote:
> On 21.01.2016 02:14, Javier Martinez Canillas wrote:
>> The driver has some hard-coded values such as the minimum delay needed
>> before a RTC update or the mask used for the sec/min/hour/etc registers.
>>
>> Use a data structure that contains these values and pass as driver data
>> using the platform device ID table for each device.
>>
>> This allows to make the driver's ops callbacks more generic so other RTC
>> that are similar but don't have the same values can also be supported.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> ---
>>
>>   drivers/rtc/rtc-max77686.c | 47 +++++++++++++++++++++++++++++-----------------
>>   1 file changed, 30 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
>> index ae4d61e7ce4b..441d163dcbeb 100644
>> --- a/drivers/rtc/rtc-max77686.c
>> +++ b/drivers/rtc/rtc-max77686.c
>> @@ -41,8 +41,6 @@
>>   #define ALARM_ENABLE_SHIFT		7
>>   #define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
>>
>> -#define MAX77686_RTC_UPDATE_DELAY	16000
>> -
>>   enum {
>>   	RTC_SEC = 0,
>>   	RTC_MIN,
>> @@ -54,6 +52,11 @@ enum {
>>   	RTC_NR_TIME
>>   };
>>
>> +struct rtc_driver_data {
>
> 1. This is only local structure so maybe: max77686_rtc_driver_data?
>

Agreed.

> 2. Can you put here a comment explaining the purpose of this delay
> (quite generic name) and used units?
>
>> +	unsigned long		delay;
>
> A comment here - what 'mask'? Generic name so explanation would be welcomed.
>
>> +	int			mask;
>
> I think this should be u8 to avoid early promotions to int. It is used
> directly with other u8 values. At the end effect of operation is
> promoted to int anyway but using only u8 for operations looks more
> consistent to me.
>

Ok, I added comments for these two fields and used u8 for mask instead.
  
>> +};
>> +
>>   struct max77686_rtc_info {
>>   	struct device		*dev;
>>   	struct max77686_dev	*max77686;
>> @@ -63,6 +66,8 @@ struct max77686_rtc_info {
>>
>>   	struct regmap		*regmap;
>>
>> +	struct rtc_driver_data	*drv_data;
>
> That may be pointer to const data. Actually not "may be" but
> even "should be" because you allocate later really a const data and you
> are discarding const-ness with cast.
>

Agreed, I missed that before.
  
>> +
>>   	int virq;
>>   	int rtc_24hr_mode;
>>   };
>> @@ -72,12 +77,19 @@ enum MAX77686_RTC_OP {
>>   	MAX77686_RTC_READ,
>>   };
>>
>> +static const struct rtc_driver_data max77686_drv_data = {
>> +	.delay = 1600,
>> +	.mask  = 0x7f,
>> +};
>> +
>>   static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
>> -				   int rtc_24hr_mode)
>> +				    struct max77686_rtc_info *info)
>>   {
>> -	tm->tm_sec = data[RTC_SEC] & 0x7f;
>> -	tm->tm_min = data[RTC_MIN] & 0x7f;
>> -	if (rtc_24hr_mode)
>> +	int mask = info->drv_data->mask;
>> +
>> +	tm->tm_sec = data[RTC_SEC] & mask;
>> +	tm->tm_min = data[RTC_MIN] & mask;
>> +	if (info->rtc_24hr_mode)
>>   		tm->tm_hour = data[RTC_HOUR] & 0x1f;
>>   	else {
>>   		tm->tm_hour = data[RTC_HOUR] & 0x0f;
>> @@ -86,10 +98,10 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
>>   	}
>>
>>   	/* Only a single bit is set in data[], so fls() would be equivalent */
>> -	tm->tm_wday = ffs(data[RTC_WEEKDAY] & 0x7f) - 1;
>> +	tm->tm_wday = ffs(data[RTC_WEEKDAY] & mask) - 1;
>>   	tm->tm_mday = data[RTC_DATE] & 0x1f;
>>   	tm->tm_mon = (data[RTC_MONTH] & 0x0f) - 1;
>> -	tm->tm_year = (data[RTC_YEAR] & 0x7f) + 100;
>> +	tm->tm_year = (data[RTC_YEAR] & mask) + 100;
>>   	tm->tm_yday = 0;
>>   	tm->tm_isdst = 0;
>>   }
>> @@ -117,6 +129,7 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
>>   {
>>   	int ret;
>>   	unsigned int data;
>> +	unsigned long delay = info->drv_data->delay;
>>
>>   	if (op == MAX77686_RTC_WRITE)
>>   		data = 1 << RTC_UDR_SHIFT;
>> @@ -129,9 +142,8 @@ static int max77686_rtc_update(struct max77686_rtc_info *info,
>>   		dev_err(info->dev, "%s: fail to write update reg(ret=%d, data=0x%x)\n",
>>   				__func__, ret, data);
>>   	else {
>> -		/* Minimum 16ms delay required before RTC update. */
>> -		usleep_range(MAX77686_RTC_UPDATE_DELAY,
>> -			     MAX77686_RTC_UPDATE_DELAY * 2);
>> +		/* Minimum delay required before RTC update. */
>> +		usleep_range(delay, delay * 2);
>>   	}
>>
>>   	return ret;
>> @@ -156,7 +168,7 @@ static int max77686_rtc_read_time(struct device *dev, struct rtc_time *tm)
>>   		goto out;
>>   	}
>>
>> -	max77686_rtc_data_to_tm(data, tm, info->rtc_24hr_mode);
>> +	max77686_rtc_data_to_tm(data, tm, info);
>>
>>   	ret = rtc_valid_tm(tm);
>>
>> @@ -213,7 +225,7 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>>   		goto out;
>>   	}
>>
>> -	max77686_rtc_data_to_tm(data, &alrm->time, info->rtc_24hr_mode);
>> +	max77686_rtc_data_to_tm(data, &alrm->time, info);
>>
>>   	alrm->enabled = 0;
>>   	for (i = 0; i < RTC_NR_TIME; i++) {
>> @@ -260,7 +272,7 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
>>   		goto out;
>>   	}
>>
>> -	max77686_rtc_data_to_tm(data, &tm, info->rtc_24hr_mode);
>> +	max77686_rtc_data_to_tm(data, &tm, info);
>>
>>   	for (i = 0; i < RTC_NR_TIME; i++)
>>   		data[i] &= ~ALARM_ENABLE_MASK;
>> @@ -299,7 +311,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
>>   		goto out;
>>   	}
>>
>> -	max77686_rtc_data_to_tm(data, &tm, info->rtc_24hr_mode);
>> +	max77686_rtc_data_to_tm(data, &tm, info);
>>
>>   	data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
>>   	data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
>> @@ -307,7 +319,7 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
>>   	data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
>>   	if (data[RTC_MONTH] & 0xf)
>>   		data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
>> -	if (data[RTC_YEAR] & 0x7f)
>> +	if (data[RTC_YEAR] & info->drv_data->mask)
>>   		data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
>>   	if (data[RTC_DATE] & 0x1f)
>>   		data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
>> @@ -436,6 +448,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>   	info->dev = &pdev->dev;
>>   	info->max77686 = max77686;
>>   	info->rtc = max77686->rtc;
>> +	info->drv_data = (struct rtc_driver_data *)pdev->id_entry->driver_data;
>
> That cast dropping constness is bad.
>

Agreed, fixed.
  
> Best regards,
> Krzysztof
>

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

Re: [PATCH 4/8] rtc: max77686: Add an indirection level to access RTC registers

[Javier Martinez Canillas]

Hello Krzysztof,

Thanks a lot for your review.

On 01/20/2016 10:05 PM, Krzysztof Kozlowski wrote:
> On 21.01.2016 02:14, Javier Martinez Canillas wrote:
>> The max77686 driver is generic enough that can be used for other
>> Maxim RTC IP blocks but these might not have the same registers
>> layout so instead of accessing the registers directly, add a map
>> to translate offsets to the real registers addresses for each IP.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> ---
>>
>>   drivers/rtc/rtc-max77686.c | 75 +++++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 65 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
>> index 441d163dcbeb..7316e41820c7 100644
>> --- a/drivers/rtc/rtc-max77686.c
>> +++ b/drivers/rtc/rtc-max77686.c
>> @@ -41,6 +41,8 @@
>>   #define ALARM_ENABLE_SHIFT		7
>>   #define ALARM_ENABLE_MASK		(1 << ALARM_ENABLE_SHIFT)
>>
>> +#define REG_RTC_NONE			0xdeadbeef
>> +
>>   enum {
>>   	RTC_SEC = 0,
>>   	RTC_MIN,
>> @@ -55,6 +57,7 @@ enum {
>>   struct rtc_driver_data {
>>   	unsigned long		delay;
>>   	int			mask;
>> +	const unsigned int	*map;
>>   };
>>
>>   struct max77686_rtc_info {
>> @@ -77,9 +80,53 @@ enum MAX77686_RTC_OP {
>>   	MAX77686_RTC_READ,
>>   };
>>
>> +/* These are not registers but just offsets that are mapped to addresses */
>> +enum rtc_reg {
>
> enum max77686_rtc_reg_offset?
>

Agreed.
  
>> +	REG_RTC_CONTROLM = 0,
>> +	REG_RTC_CONTROL,
>> +	REG_RTC_UPDATE0,
>> +	REG_RTC_UPDATE1,
>> +	REG_WTSR_SMPL_CNTL,
>> +	REG_RTC_SEC,
>> +	REG_RTC_MIN,
>> +	REG_RTC_HOUR,
>> +	REG_RTC_WEEKDAY,
>> +	REG_RTC_MONTH,
>> +	REG_RTC_YEAR,
>> +	REG_RTC_DATE,
>> +	REG_ALARM1_SEC,
>> +	REG_ALARM1_MIN,
>> +	REG_ALARM1_HOUR,
>> +	REG_ALARM1_WEEKDAY,
>> +	REG_ALARM1_MONTH,
>> +	REG_ALARM1_YEAR,
>> +	REG_ALARM1_DATE,
>> +	REG_ALARM2_SEC,
>> +	REG_ALARM2_MIN,
>> +	REG_ALARM2_HOUR,
>> +	REG_ALARM2_WEEKDAY,
>> +	REG_ALARM2_MONTH,
>> +	REG_ALARM2_YEAR,
>> +	REG_ALARM2_DATE,
>> +	REG_RTC_END,
>> +};
>> +
>
> A short comment what is mapped into what would be appreciated.
>

Ok, added such comment for v2.
  
>> +static const unsigned int max77686_map[REG_RTC_END] = {
>> +	MAX77686_RTC_CONTROLM, MAX77686_RTC_CONTROL, MAX77686_RTC_UPDATE0,
>> +	REG_RTC_NONE, MAX77686_WTSR_SMPL_CNTL, MAX77686_RTC_SEC,
>> +	MAX77686_RTC_MIN, MAX77686_RTC_HOUR, MAX77686_RTC_WEEKDAY,
>> +	MAX77686_RTC_MONTH, MAX77686_RTC_YEAR, MAX77686_RTC_DATE,
>> +	MAX77686_ALARM1_SEC, MAX77686_ALARM1_MIN, MAX77686_ALARM1_HOUR,
>> +	MAX77686_ALARM1_WEEKDAY, MAX77686_ALARM1_MONTH, MAX77686_ALARM1_YEAR,
>> +	MAX77686_ALARM1_DATE, MAX77686_ALARM2_SEC, MAX77686_ALARM2_MIN,
>> +	MAX77686_ALARM2_HOUR, MAX77686_ALARM2_WEEKDAY, MAX77686_ALARM2_MONTH,
>> +	MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE,
>> +};
>
> It is difficult to check for mistakes here. I would prefer direct mapping:
> 	[REG_RTC_CONTROLM] = MAX77686_RTC_CONTROLM,
> 	....
>

Right, that's a very good idea.
  
>
> Rest looks good but I did not check the correctness of mapping above.
>

Great, thanks.
  
> BR,
> Krzysztof
>

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

Re: [PATCH 5/8] rtc: max77686: Add max77802 support

[Javier Martinez Canillas]

Hello Krzysztof,

Thanks a lot for your review.

On 01/20/2016 10:56 PM, Krzysztof Kozlowski wrote:
> On 21.01.2016 02:14, Javier Martinez Canillas wrote:
>> The MAX77686 and MAX77802 RTC IP blocks are very similar with only
>> these differences:
>>
>> 0) The RTC registers layout and addresses are different.
>>
>> 1) The MAX77686 use 1 bit of the sec/min/hour/etc registers as the
>>     alarm enable while MAX77802 has a separate register for that.
>>
>> 2) The MAX77686 RTCYEAR register valid values range is 0..99 while
>>     for MAX77802 is 0..199.
>>
>> 3) The MAX77686 has a separate I2C address for the RTC registers
>>     while the MAX77802 uses the same I2C address as the PMIC regs.
>>
>> 5) They minium delay before a RTC update (16ms vs 200 usecs).
>>
>> There are separate drivers for MAX77686 and MAX77802 RTC IP blocks
>> but the differences are not that big so the driver can be extended
>> to support both instead of duplicating a lot of code in 2 drivers.
>>
>> Suggested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> ---
>>
>>   drivers/rtc/rtc-max77686.c | 176 ++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 128 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
>> index 7316e41820c7..7a144e7ecd27 100644
>> --- a/drivers/rtc/rtc-max77686.c
>> +++ b/drivers/rtc/rtc-max77686.c
>> @@ -1,5 +1,5 @@
>>   /*
>> - * RTC driver for Maxim MAX77686
>> + * RTC driver for Maxim MAX77686 and MAX77802
>>    *
>>    * Copyright (C) 2012 Samsung Electronics Co.Ltd
>>    *
>> @@ -43,6 +43,13 @@
>>
>>   #define REG_RTC_NONE			0xdeadbeef
>>
>> +/*
>> + * MAX77802 has separate register (RTCAE1) for alarm enable instead
>> + * using 1 bit from registers RTC{SEC,MIN,HOUR,DAY,MONTH,YEAR,DATE}
>> + * as in done in MAX77686.
>> + */
>> +#define ALARM_ENABLE_VALUE		0x77
>
> MAX77802_ALARM_ENABLE_VALUE
> (it is specific to 77802, right?)
>

Correct, although I guess other RTC in Maxim PMICs will need the same
value but we can stick to the convention of adding the prefix for the
first IP that needs it and the following can just use the same (as is
the case for max77686 macros used by max77802 code).

So yes, I'm adding a MAX77802 prefix to this.
  
>> +
>>   enum {
>>   	RTC_SEC = 0,
>>   	RTC_MIN,
>> @@ -58,6 +65,10 @@ struct rtc_driver_data {
>>   	unsigned long		delay;
>>   	int			mask;
>>   	const unsigned int	*map;
>> +	/* Has a separate alarm enable register? */
>> +	bool			rtcae;
>> +	/* Has a separate I2C regmap for the RTC? */
>> +	bool			rtcrm;
>
> Both members are a tongue twisters. :)
>

They are indeed!
  
> 'rtcae' you are mostly using in an inverted way (!rtcae) so how about:
> 'alarm_enable_bit'?
>

I'll better use alarm_enable_reg since is about having a separate alarm
enable register and not a bit (in fact max77686 is the one that uses 1
bit of each sec/min/hour/etc registers to encode the alarm enable info).

> 'rtcrm' - 'separate_i2c_addr'?
>

Agreed.
  
> By the way, I was thinking that you would do decoupling of i2c and
> regmap here. It is not required (more useful for Laxman's patch) but it
> might by a part of these series.
>

I prefer to stick with our plan with Laxman that I should merge max77686
and max77802 drivers and he would do the I2C decoupling on top of that.

I could do the I2C decoupling in this series but that would mean touching
the MFD driver and if possible I try to avoid cross-subsystems series.

>>   };
>>
>>   struct max77686_rtc_info {
>> @@ -108,6 +119,8 @@ enum rtc_reg {
>>   	REG_ALARM2_MONTH,
>>   	REG_ALARM2_YEAR,
>>   	REG_ALARM2_DATE,
>> +	REG_RTC_AE1,
>> +	REG_RTC_AE2,
>>   	REG_RTC_END,
>>   };
>>
>> @@ -120,13 +133,36 @@ static const unsigned int max77686_map[REG_RTC_END] = {
>>   	MAX77686_ALARM1_WEEKDAY, MAX77686_ALARM1_MONTH, MAX77686_ALARM1_YEAR,
>>   	MAX77686_ALARM1_DATE, MAX77686_ALARM2_SEC, MAX77686_ALARM2_MIN,
>>   	MAX77686_ALARM2_HOUR, MAX77686_ALARM2_WEEKDAY, MAX77686_ALARM2_MONTH,
>> -	MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE,
>> +	MAX77686_ALARM2_YEAR, MAX77686_ALARM2_DATE, REG_RTC_NONE, REG_RTC_NONE,
>>   };
>>
>>   static const struct rtc_driver_data max77686_drv_data = {
>>   	.delay = 1600,
>>   	.mask  = 0x7f,
>>   	.map   = max77686_map,
>> +	.rtcae = false,
>> +	.rtcrm = true,
>> +};
>> +
>> +static const unsigned int max77802_map[REG_RTC_END] = {
>> +	MAX77802_RTC_CONTROLM, MAX77802_RTC_CONTROL, MAX77802_RTC_UPDATE0,
>> +	REG_RTC_NONE, MAX77802_WTSR_SMPL_CNTL, MAX77802_RTC_SEC,
>> +	MAX77802_RTC_MIN, MAX77802_RTC_HOUR, MAX77802_RTC_WEEKDAY,
>> +	MAX77802_RTC_MONTH, MAX77802_RTC_YEAR, MAX77802_RTC_DATE,
>> +	MAX77802_ALARM1_SEC, MAX77802_ALARM1_MIN, MAX77802_ALARM1_HOUR,
>> +	MAX77686_ALARM1_WEEKDAY, MAX77802_ALARM1_MONTH, MAX77802_ALARM1_YEAR,
>> +	MAX77802_ALARM1_DATE, MAX77802_ALARM1_SEC, MAX77802_ALARM1_MIN,
>> +	MAX77802_ALARM1_HOUR, MAX77802_ALARM1_WEEKDAY, MAX77802_ALARM1_MONTH,
>> +	MAX77802_ALARM1_YEAR, MAX77802_ALARM1_DATE, MAX77802_RTC_AE1,
>> +	MAX77802_RTC_AE2,
>> +};
>> +
>> +static const struct rtc_driver_data max77802_drv_data = {
>> +	.delay = 200,
>> +	.mask  = 0xff,
>> +	.map   = max77802_map,
>> +	.rtcae = true,
>> +	.rtcrm = false,
>>   };
>>
>>   static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
>> @@ -148,12 +184,20 @@ static void max77686_rtc_data_to_tm(u8 *data, struct rtc_time *tm,
>>   	tm->tm_wday = ffs(data[RTC_WEEKDAY] & mask) - 1;
>>   	tm->tm_mday = data[RTC_DATE] & 0x1f;
>>   	tm->tm_mon = (data[RTC_MONTH] & 0x0f) - 1;
>> -	tm->tm_year = (data[RTC_YEAR] & mask) + 100;
>> +	tm->tm_year = data[RTC_YEAR] & mask;
>>   	tm->tm_yday = 0;
>>   	tm->tm_isdst = 0;
>> +
>> +	/*
>> +	 * MAX77686 uses 1 bit from sec/min/hour/etc RTC registers and the
>> +	 * year values are just 0..99 so add 100 to support up to 2099.
>> +	 */
>> +	if (!info->drv_data->rtcae)
>> +		tm->tm_year += 100;
>>   }
>>
>> -static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data)
>> +static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data,
>> +				   struct max77686_rtc_info *info)
>>   {
>>   	data[RTC_SEC] = tm->tm_sec;
>>   	data[RTC_MIN] = tm->tm_min;
>> @@ -161,13 +205,19 @@ static int max77686_rtc_tm_to_data(struct rtc_time *tm, u8 *data)
>>   	data[RTC_WEEKDAY] = 1 << tm->tm_wday;
>>   	data[RTC_DATE] = tm->tm_mday;
>>   	data[RTC_MONTH] = tm->tm_mon + 1;
>> -	data[RTC_YEAR] = tm->tm_year > 100 ? (tm->tm_year - 100) : 0;
>>
>> -	if (tm->tm_year < 100) {
>> -		pr_warn("RTC cannot handle the year %d.  Assume it's 2000.\n",
>> -			1900 + tm->tm_year);
>> -		return -EINVAL;
>> +	if (!info->drv_data->rtcae) {
>> +		data[RTC_YEAR] = tm->tm_year > 100 ? (tm->tm_year - 100) : 0;
>> +
>> +		if (tm->tm_year < 100) {
>> +			pr_warn("RTC can't handle year %d. Assume it's 2000.\n",
>> +				1900 + tm->tm_year);
>
> Maybe in a separate patch use dev_warn()? It wasn't possible before
> because you need 'info' argument but it is possible.
>

Agreed, I'll add as another patch after this one in v2.
  
>> +			return -EINVAL;
>> +		}
>> +	} else {
>> +		data[RTC_YEAR] = tm->tm_year;
>>   	}
>> +
>>   	return 0;
>>   }
>>
>> @@ -232,7 +282,7 @@ static int max77686_rtc_set_time(struct device *dev, struct rtc_time *tm)
>>   	u8 data[RTC_NR_TIME];
>>   	int ret;
>>
>> -	ret = max77686_rtc_tm_to_data(tm, data);
>> +	ret = max77686_rtc_tm_to_data(tm, data, info);
>>   	if (ret < 0)
>>   		return ret;
>>
>> @@ -279,11 +329,24 @@ static int max77686_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>>   	max77686_rtc_data_to_tm(data, &alrm->time, info);
>>
>>   	alrm->enabled = 0;
>> -	for (i = 0; i < RTC_NR_TIME; i++) {
>> -		if (data[i] & ALARM_ENABLE_MASK) {
>> -			alrm->enabled = 1;
>> -			break;
>> +
>> +	if (!info->drv_data->rtcae) {
>> +		for (i = 0; i < RTC_NR_TIME; i++) {
>> +			if (data[i] & ALARM_ENABLE_MASK) {
>> +				alrm->enabled = 1;
>> +				break;
>> +			}
>>   		}
>> +	} else {
>> +		ret = regmap_read(info->max77686->regmap,
>> +				  map[REG_RTC_AE1], &val);
>> +		if (ret < 0) {
>> +			dev_err(info->dev, "%s:%d fail to read alarm enable(%d)\n",
>> +				__func__, __LINE__, ret);
>
> I don't like the func/LINE. I know that driver is using them already but
> I think it is better not to add new usages of it.
>

Fair enough, I'll remove it.
  
>> +			goto out;
>
> Actually I think there is a bug here already. The function will always
> return '0'. Instead the 'out' label should return 'ret'. Can you fix it
> in separate patch (with reported-by :) )?
>

Nice catch, I'll add a bugfix as another patch at the start of the series.
  
>> +		}
>> +		if (val)
>> +			alrm->enabled = 1;
>>   	}
>>
>>   	alrm->pending = 0;
>> @@ -316,21 +379,27 @@ static int max77686_rtc_stop_alarm(struct max77686_rtc_info *info)
>>   	if (ret < 0)
>>   		goto out;
>>
>> -	ret = regmap_bulk_read(info->max77686->rtc_regmap,
>> -			       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
>> -	if (ret < 0) {
>> -		dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
>> +	if (!info->drv_data->rtcae) {
>> +		ret = regmap_bulk_read(info->max77686->rtc_regmap,
>> +				       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
>> +		if (ret < 0) {
>> +			dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
>>   				__func__, ret);
>> -		goto out;
>> -	}
>> +			goto out;
>> +		}
>>
>> -	max77686_rtc_data_to_tm(data, &tm, info);
>> +		max77686_rtc_data_to_tm(data, &tm, info);
>>
>> -	for (i = 0; i < RTC_NR_TIME; i++)
>> -		data[i] &= ~ALARM_ENABLE_MASK;
>> +		for (i = 0; i < RTC_NR_TIME; i++)
>> +			data[i] &= ~ALARM_ENABLE_MASK;
>> +
>> +		ret = regmap_bulk_write(info->max77686->rtc_regmap,
>> +					map[REG_ALARM1_SEC], data, RTC_NR_TIME);
>> +	} else {
>> +		ret = regmap_write(info->max77686->regmap,
>> +				   map[REG_RTC_AE1], 0);
>> +	}
>>
>> -	ret = regmap_bulk_write(info->max77686->rtc_regmap,
>> -				map[REG_ALARM1_SEC], data, RTC_NR_TIME);
>>   	if (ret < 0) {
>>   		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
>>   				__func__, ret);
>> @@ -356,29 +425,35 @@ static int max77686_rtc_start_alarm(struct max77686_rtc_info *info)
>>   	if (ret < 0)
>>   		goto out;
>>
>> -	ret = regmap_bulk_read(info->max77686->rtc_regmap,
>> -			       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
>> -	if (ret < 0) {
>> -		dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
>> +	if (!info->drv_data->rtcae) {
>> +		ret = regmap_bulk_read(info->max77686->rtc_regmap,
>> +				       map[REG_ALARM1_SEC], data, RTC_NR_TIME);
>> +		if (ret < 0) {
>> +			dev_err(info->dev, "%s: fail to read alarm reg(%d)\n",
>>   				__func__, ret);
>> -		goto out;
>> -	}
>> -
>> -	max77686_rtc_data_to_tm(data, &tm, info);
>> +			goto out;
>> +		}
>>
>> -	data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
>> -	data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
>> -	data[RTC_HOUR] |= (1 << ALARM_ENABLE_SHIFT);
>> -	data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
>> -	if (data[RTC_MONTH] & 0xf)
>> -		data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
>> -	if (data[RTC_YEAR] & info->drv_data->mask)
>> -		data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
>> -	if (data[RTC_DATE] & 0x1f)
>> -		data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
>> +		max77686_rtc_data_to_tm(data, &tm, info);
>> +
>> +		data[RTC_SEC] |= (1 << ALARM_ENABLE_SHIFT);
>> +		data[RTC_MIN] |= (1 << ALARM_ENABLE_SHIFT);
>> +		data[RTC_HOUR] |= (1 << ALARM_ENABLE_SHIFT);
>> +		data[RTC_WEEKDAY] &= ~ALARM_ENABLE_MASK;
>> +		if (data[RTC_MONTH] & 0xf)
>> +			data[RTC_MONTH] |= (1 << ALARM_ENABLE_SHIFT);
>> +		if (data[RTC_YEAR] & info->drv_data->mask)
>> +			data[RTC_YEAR] |= (1 << ALARM_ENABLE_SHIFT);
>> +		if (data[RTC_DATE] & 0x1f)
>> +			data[RTC_DATE] |= (1 << ALARM_ENABLE_SHIFT);
>> +
>> +		ret = regmap_bulk_write(info->max77686->rtc_regmap,
>> +					map[REG_ALARM1_SEC], data, RTC_NR_TIME);
>> +	} else {
>> +		ret = regmap_write(info->max77686->regmap,
>> +				   map[REG_RTC_AE1], ALARM_ENABLE_VALUE);
>> +	}
>>
>> -	ret = regmap_bulk_write(info->max77686->rtc_regmap,
>> -				map[REG_ALARM1_SEC], data, RTC_NR_TIME);
>>   	if (ret < 0) {
>>   		dev_err(info->dev, "%s: fail to write alarm reg(%d)\n",
>>   				__func__, ret);
>> @@ -396,7 +471,7 @@ static int max77686_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>>   	u8 data[RTC_NR_TIME];
>>   	int ret;
>>
>> -	ret = max77686_rtc_tm_to_data(&alrm->time, data);
>> +	ret = max77686_rtc_tm_to_data(&alrm->time, data, info);
>>   	if (ret < 0)
>>   		return ret;
>>
>> @@ -490,6 +565,7 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>   {
>>   	struct max77686_dev *max77686 = dev_get_drvdata(pdev->dev.parent);
>>   	struct max77686_rtc_info *info;
>> +	const struct platform_device_id *id = pdev->id_entry;
>>   	int ret;
>>
>>   	dev_info(&pdev->dev, "%s\n", __func__);
>> @@ -503,7 +579,10 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>   	info->dev = &pdev->dev;
>>   	info->max77686 = max77686;
>>   	info->rtc = max77686->rtc;
>> -	info->drv_data = (struct rtc_driver_data *)pdev->id_entry->driver_data;
>
> Comment for previous patch: use platform_get_device_id(pdev).
>

Ok.
  
> Best regards,
> Krzysztof
>

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

Re: [PATCH 6/8] rtc: Remove Maxim 77802 driver

[Javier Martinez Canillas]

Hello Krzysztof,

On 01/20/2016 10:57 PM, Krzysztof Kozlowski wrote:
> On 21.01.2016 02:14, Javier Martinez Canillas wrote:
>> The max77686 RTC driver now supports the max77802 RTC as
>> well so there's no need to have a separate driver anymore.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> ---
>>
>>   drivers/rtc/Kconfig        |  10 -
>>   drivers/rtc/Makefile       |   1 -
>>   drivers/rtc/rtc-max77802.c | 502 ---------------------------------------------
>>   3 files changed, 513 deletions(-)
>>   delete mode 100644 drivers/rtc/rtc-max77802.c
>>
>
> Oh yes, I like removals so much! :)
>

I knew you would like it :)
  
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>

Thanks.
  
> Best regards,
> Krzysztof
>
>

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

Re: [PATCH 7/8] ARM: exynos_defconfig: Remove MAX77802 RTC Kconfig symbol

[Javier Martinez Canillas]

Hello Krzysztof,

On 01/20/2016 10:58 PM, Krzysztof Kozlowski wrote:
> On 21.01.2016 02:14, Javier Martinez Canillas wrote:
>> The driver has been removed so the Kconfig symbol is not valid anymore.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> ---
>>
>>   arch/arm/configs/exynos_defconfig | 1 -
>>   1 file changed, 1 deletion(-)
>
> The patch should go after merging rtc-max77802 into rtc-max77686 to
> preserve bisectability on defconfig.
>

That's correct, same for 8/8.
  
> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>

Thanks.
  
> Best regards,
> Krzysztof
>

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

Re: [PATCH 0/8] rtc: max77686: Extend driver and add max77802 support

[Javier Martinez Canillas]

Hello Krzysztof,

On 01/20/2016 09:48 PM, Krzysztof Kozlowski wrote:
> On 21.01.2016 02:14, Javier Martinez Canillas wrote:
>> Hello,
>>
>> On a recent disussion [0] with Krzysztof Kozlowski and Laxman Dewangan,
>> we came to the conclusion that the max77686 and max77802 RTC are almost
>> the same with only a few differences so there shouldn't be two separate
>> drivers and is better to extend max77686 driver and delete rtc-max77802.
>>
>> By making the driver more generic, other RTC IP blocks from Maxim PMICs
>> could be supported as well like the max77620.
>>
>> Patches #1 is just a trivial cleanup.
>>
>> Patch #2 allows to support RTCs that need a shorter delay when updating
>> the RTC.
>>
>> Patch #3 adds a driver data structure to avoid hard-coding parameters
>> specific to a certain RTC such as the needed delay and RTC register mask.
>>
>> Patch #4 changes the driver to use a mapping table instead of using the
>> max77686 registers offsets directly to allow supporting RTC with other
>> registers addresses and layout.
>>
>> Patch #5 Adds support for max77802 to max77686 RTC driver and patch #6
>> removes the old driver since is not needed anymore.
>>
>> Finally patch #7 and patch #8 removes the Kconfig symbol from defconfigs.
>>
>> I've tested this patch-set on an Exynos5800 Peach Pi Chromebook that has
>> a max77802 PMIC and the RTC was working correctly but I don't have a
>> machine with max77686 so I will really appreaciate if someone can test
>> that no regressions were introduced.
>>
>
> Thanks for the submission. I like the approach. I'll start reviewing the
> code and testing it... or maybe I'll wait with testing for v2 because I
> already sent some comments. :)
>
> Anyway I will provide later tested-by on max77686.
>

Thanks a lot for your detailed review. I'll post v2 shortly so you can
just test that.
  
> Best regards,
> Krzysztof
>

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