From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexandre Belloni Subject: Re: [PATCH] rtc: add support for EPSON TOYOCOM RTC-7301SF/DG Date: Wed, 30 Nov 2016 22:01:52 +0100 Message-ID: <20161130210152.hiyfa3srismgvtj5@piout.net> References: <1472396118-13424-1-git-send-email-akinobu.mita@gmail.com> Reply-To: rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: Sender: rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Content-Disposition: inline In-Reply-To: <1472396118-13424-1-git-send-email-akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Akinobu Mita Cc: rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Alessandro Zummo List-Id: devicetree@vger.kernel.org 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 > + * > + * Based on rtc-rp5c01.c > + * > + * Datasheet: http://www5.epsondevice.com/en/products/parallel/rtc7301sf.html > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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? [...] > +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. I don't mind much as you are probably the only user anyway. -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. --- You received this message because you are subscribed to the Google Groups "rtc-linux" group. To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit https://groups.google.com/d/optout.