2016-12-01 6:01 GMT+09:00 Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx>: > Hi, > > Sorry for the very late review! > > It seems mostly fine for me, two small comments: > > On 28/08/2016 at 23:55:18 +0900, Akinobu Mita wrote : >> diff --git a/drivers/rtc/rtc-r7301.c b/drivers/rtc/rtc-r7301.c >> new file mode 100644 >> index 0000000..b1be281 >> --- /dev/null >> +++ b/drivers/rtc/rtc-r7301.c >> @@ -0,0 +1,458 @@ >> +/* >> + * EPSON TOYOCOM RTC-7301SF/DG Driver >> + * >> + * Copyright (c) 2016 Akinobu Mita <akinobu.mita@xxxxxxxxx> >> + * >> + * Based on rtc-rp5c01.c >> + * >> + * Datasheet: http://www5.epsondevice.com/en/products/parallel/rtc7301sf.html >> + */ >> + >> +#include <linux/io.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/delay.h> >> +#include <linux/regmap.h> >> +#include <linux/platform_device.h> >> +#include <linux/rtc.h> >> + >> +#define DRV_NAME "rtc-r7301" >> + >> +enum { >> + RTC7301_1_SEC = 0x0, /* Bank 0 and Band 1 */ >> + RTC7301_10_SEC = 0x1, /* Bank 0 and Band 1 */ >> + RTC7301_AE = BIT(3), >> + RTC7301_1_MIN = 0x2, /* Bank 0 and Band 1 */ >> + RTC7301_10_MIN = 0x3, /* Bank 0 and Band 1 */ >> + RTC7301_1_HOUR = 0x4, /* Bank 0 and Band 1 */ >> + RTC7301_10_HOUR = 0x5, /* Bank 0 and Band 1 */ >> + RTC7301_DAY_OF_WEEK = 0x6, /* Bank 0 and Band 1 */ >> + RTC7301_1_DAY = 0x7, /* Bank 0 and Band 1 */ >> + RTC7301_10_DAY = 0x8, /* Bank 0 and Band 1 */ >> + RTC7301_1_MONTH = 0x9, /* Bank 0 */ >> + RTC7301_10_MONTH = 0xa, /* Bank 0 */ >> + RTC7301_1_YEAR = 0xb, /* Bank 0 */ >> + RTC7301_10_YEAR = 0xc, /* Bank 0 */ >> + RTC7301_100_YEAR = 0xd, /* Bank 0 */ >> + RTC7301_1000_YEAR = 0xe, /* Bank 0 */ >> + RTC7301_ALARM_CONTROL = 0xe, /* Bank 1 */ >> + RTC7301_ALARM_CONTROL_AIE = BIT(0), >> + RTC7301_ALARM_CONTROL_AF = BIT(1), >> + RTC7301_TIMER_CONTROL = 0xe, /* Bank 2 */ >> + RTC7301_ALARM_CONTROL_TIE = BIT(0), >> + RTC7301_ALARM_CONTROL_TF = BIT(1), >> + RTC7301_CONTROL = 0xf, /* All banks */ >> + RTC7301_CONTROL_BUSY = BIT(0), >> + RTC7301_CONTROL_STOP = BIT(1), >> + RTC7301_CONTROL_BANK_SEL_0 = BIT(2), >> + RTC7301_CONTROL_BANK_SEL_1 = BIT(3), >> +}; >> + > > Any particular reason why you use an enum instead of the usual #define? No particular reason. A part of this driver is based on rtc-rp5c01.c which declares the register addresses like this. > [...] > >> +static void rtc7301_init(struct rtc7301_priv *priv) >> +{ >> + unsigned long flags; >> + >> + spin_lock_irqsave(&priv->lock, flags); >> + >> + rtc7301_select_bank(priv, 1); >> + rtc7301_alarm_irq(priv, false); >> + > > If the RTC is battery backed, it may still run with the core power off > and maybe someone will actually expect the alarm to trigger at a later > time. You're right. I'll remove these lines. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
