From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754243AbcLLKUe (ORCPT ); Mon, 12 Dec 2016 05:20:34 -0500 Received: from mx07-00178001.pphosted.com ([62.209.51.94]:34089 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752708AbcLLKUc (ORCPT ); Mon, 12 Dec 2016 05:20:32 -0500 Subject: Re: [PATCH 3/8] rtc: add STM32 RTC driver References: To: "alexandre.belloni@free-electrons.com" CC: "a.zummo@towertech.it" , "robh+dt@kernel.org" , "mark.rutland@arm.com" , "mcoquelin.stm32@gmail.com" , Alexandre TORGUE , "linux@armlinux.org.uk" , "rtc-linux@googlegroups.com" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Gabriel FERNANDEZ From: Amelie DELAUNAY X-Forwarded-Message-Id: Message-ID: Date: Mon, 12 Dec 2016 11:19:36 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.75.127.47] X-ClientProxiedBy: SFHDAG3NODE3.st.com (10.75.127.9) To SFHDAG5NODE2.st.com (10.75.127.14) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-12-12_05:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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 > > 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + > > 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