Re: [PATCH 4/8] rtc: max77686: Add an indirection level to access RTC registers
From: Javier Martinez Canillas
Date: Thu Jan 21 2016 - 09:57:14 EST
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@xxxxxxxxxxxxxxx>
---
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