Messages in this thread | | | Subject | Re: [PATCH 3/8] rtc: add STM32 RTC driver | From | Amelie DELAUNAY <> | Date | Mon, 12 Dec 2016 11:19:36 +0100 |
| |
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@st.com> >> --- >> 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@gmail.com> > > 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
| |