Hi,
Thanks for the review.
On 12/07/2016 08:08 PM, Alexandre Belloni wrote:
Hi,
It seems mostly fine.
On 02/12/2016 at 15:09:56 +0100, Amelie Delaunay wrote :
This patch adds support for the STM32 RTC.
Signed-off-by: Amelie Delaunay <amelie.delaunay@xxxxxx>
---
drivers/rtc/Kconfig | 10 +
drivers/rtc/Makefile | 1 +
drivers/rtc/rtc-stm32.c | 777 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 788 insertions(+)
create mode 100644 drivers/rtc/rtc-stm32.c
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index e859d14..dd8b218 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1706,6 +1706,16 @@ config RTC_DRV_PIC32
This driver can also be built as a module. If so, the module
will be called rtc-pic32
+config RTC_DRV_STM32
+ tristate "STM32 On-Chip RTC"
+ depends on ARCH_STM32
Can you add COMPILE_TEST? Looking at it, nothing seemed to be
architecture specific and this nicely increases compile test coverage.
It should also probably select REGMAP_MMIO.
Ok.
diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
new file mode 100644
index 0000000..9e710ff
--- /dev/null
+++ b/drivers/rtc/rtc-stm32.c
@@ -0,0 +1,777 @@
+/*
+ * Copyright (C) Amelie Delaunay 2015
+ * Author: Amelie Delaunay <adelaunay.stm32@xxxxxxxxx>
This differs from your SoB. I don't really care but it seems odd.
Yes, I've already changed it in my upcoming V2!
+ * License terms: GNU General Public License (GPL), version 2
+ */
+
+#include <linux/bcd.h>
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+#include <linux/spinlock.h>
+
I have the feeling that some of those headers are not necessary maybe
some cleanup should be done.
Ok I'll have a look.
+static struct regmap *dbp;
+
+struct stm32_rtc {
+ struct rtc_device *rtc_dev;
+ void __iomem *base;
+ struct clk *pclk;
+ struct clk *ck_rtc;
+ unsigned int clksrc;
+ spinlock_t lock; /* Protects registers accesses */
That comment makes checkpatch happy but is not super useful :) Anyway...
Lack of inspiration :)
+static irqreturn_t stm32_rtc_alarm_irq(int irq, void *dev_id)
+{
...can you make that one a threaded IRQ? If that's the case, just take
the rtc_device mutex here and remove the spinlock. All the other
function are already protected.
Ok I'll study this point.
+static int stm32_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+ struct stm32_rtc *rtc = dev_get_drvdata(dev);
+ struct rtc_time *tm = &alrm->time;
+ unsigned int alrmar, cr, isr;
+ unsigned long irqflags;
+
+ spin_lock_irqsave(&rtc->lock, irqflags);
+
+ alrmar = stm32_rtc_readl(rtc, STM32_RTC_ALRMAR);
+ cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
+ isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
+
+ spin_unlock_irqrestore(&rtc->lock, irqflags);
+
+ if (alrmar & STM32_RTC_ALRMXR_DATE_MASK) {
+ /*
+ * Date/day don't care in Alarm comparison so alarm triggers
I guess you meant "doesn't matter" (that is also valid for the other
usages of "don't care".
+ * every day
+ */
+ tm->tm_mday = -1;
+ tm->tm_wday = -1;
+ } else {
+ if (alrmar & STM32_RTC_ALRMXR_WDSEL) {
+ /* Alarm is set to a day of week */
+ tm->tm_mday = -1;
+ tm->tm_wday = (alrmar & STM32_RTC_ALRMXR_WDAY) >>
+ STM32_RTC_ALRMXR_WDAY_SHIFT;
+ tm->tm_wday %= 7;
+ } else {
+ /* Alarm is set to a day of month */
+ tm->tm_wday = -1;
+ tm->tm_mday = (alrmar & STM32_RTC_ALRMXR_DATE) >>
+ STM32_RTC_ALRMXR_DATE_SHIFT;
+ }
+ }
+
+ if (alrmar & STM32_RTC_ALRMXR_HOUR_MASK) {
+ /* Hours don't care in Alarm comparison */
+ tm->tm_hour = -1;
+ } else {
+ tm->tm_hour = (alrmar & STM32_RTC_ALRMXR_HOUR) >>
+ STM32_RTC_ALRMXR_HOUR_SHIFT;
+ if (alrmar & STM32_RTC_ALRMXR_PM)
+ tm->tm_hour += 12;
+ }
+
+ if (alrmar & STM32_RTC_ALRMXR_MIN_MASK) {
+ /* Minutes don't care in Alarm comparison */
+ tm->tm_min = -1;
+ } else {
+ tm->tm_min = (alrmar & STM32_RTC_ALRMXR_MIN) >>
+ STM32_RTC_ALRMXR_MIN_SHIFT;
+ }
+
+ if (alrmar & STM32_RTC_ALRMXR_SEC_MASK) {
+ /* Seconds don't care in Alarm comparison */
+ tm->tm_sec = -1;
+ } else {
+ tm->tm_sec = (alrmar & STM32_RTC_ALRMXR_SEC) >>
+ STM32_RTC_ALRMXR_SEC_SHIFT;
+ }
+
I'm not sure those multiple cases (including STM32_RTC_ALRMXR_WDSEL) are
useful because the core will always give you valid tm_sec, tm_min,
tm_hour and tm_mday (it is actually checked up to four times!) so you
should always end up in the same configuration.
If you think some code other than Linux may set an alarm (e.g. the
bootloader) then you may keep them in read_alarm but at least you can
remove them from set_alarm.
Ok I'll clean this part.
+static int stm32_rtc_probe(struct platform_device *pdev)
+{
+ struct stm32_rtc *rtc;
+ struct resource *res;
+ int ret;
+
+ rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
+ if (!rtc)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ rtc->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(rtc->base))
+ return PTR_ERR(rtc->base);
+
+ dbp = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "st,syscfg");
+ if (IS_ERR(dbp)) {
+ dev_err(&pdev->dev, "no st,syscfg\n");
+ return PTR_ERR(dbp);
+ }
+
+ spin_lock_init(&rtc->lock);
+
+ rtc->ck_rtc = devm_clk_get(&pdev->dev, "ck_rtc");
+ if (IS_ERR(rtc->ck_rtc)) {
+ dev_err(&pdev->dev, "no ck_rtc clock");
+ return PTR_ERR(rtc->ck_rtc);
+ }
+
+ ret = clk_prepare_enable(rtc->ck_rtc);
+ if (ret)
+ return ret;
+
+ if (dbp)
+ regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, PWR_CR_DBP);
+
+ ret = stm32_rtc_init(pdev, rtc);
+ if (ret)
+ goto err;
+
Isn't that RTC backuped in some way, do you really need to reinit it
each time the system reboots?
Indeed, RTC is backuped. I need to reinit it only if RTC parent clock
has changed.
Best Regards,
Amelie
--
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
