From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752454AbdK2WLa (ORCPT ); Wed, 29 Nov 2017 17:11:30 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:47843 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752069AbdK2WL1 (ORCPT ); Wed, 29 Nov 2017 17:11:27 -0500 Date: Wed, 29 Nov 2017 23:11:14 +0100 From: Alexandre Belloni To: linux-kernel-dev@beckhoff.com Cc: Alessandro Zummo , Patrick Bruenn , Rob Herring , Mark Rutland , "open list:REAL TIME CLOCK (RTC) SUBSYSTEM" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list , Fabio Estevam , Juergen Borleis , Noel Vellemans , Shawn Guo , Sascha Hauer , Russell King , "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" Subject: Re: [PATCH] rtc: add mxc driver for i.MX53 Message-ID: <20171129221114.GV21126@piout.net> References: <20171128073927.12035-1-linux-kernel-dev@beckhoff.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171128073927.12035-1-linux-kernel-dev@beckhoff.com> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, A really quick review: On 28/11/2017 at 08:39:27 +0100, linux-kernel-dev@beckhoff.com wrote: > From: Patrick Bruenn > > Neither rtc-imxdi nor rtc-mxc are compatible with i.MX53. > Add a modernized version of mxc_v2 from here: > http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/rtc/rtc-mxc_v2.c?h=imx_2.6.35_11.09.01 > > Changes to that version: > - updated to v4.15-rc1 > - removed ioctl() > - removed proc() > - removed manual(redundant) enable_irq flag > I'd say this doesn't add much information as this link is probably bound to die at some point (especially seeing the NXP/Qualcomm/Broadcom story). > Signed-off-by: Patrick Bruenn > > --- > > Open issues: > - driver naming, should it be merged with rtc-mxc.c ? This seems different enough to be a separate driver. > - document DT binding "fsl,imx53-rtc" accordingly Yes, this has to be documented in a previous patch > - Should unused defines be removed or kept for someone else to be > useful? I don't care much, either are fine but the less lines, the easier the reviews. > - Is the use of __raw_readl/writel() correct? Should it be replaced with > readl/writel()? The __raw version will not work if someone is running a BE kernel so they should be moved to the _relaxed version or the usual readl/writel. > - suspend/resume() seems different to existing rtc-mxc.c, should I apply > the pattern from rtc-mxc.c? If you mean using SIMPLE_DEV_PM_OPS, yes, you should do that. > - On Shawns tree imx53.dtsi has been reverted already[1][2]. Should I split > the imx53.dtsi change into a separate patch based on his tree? Or can > we still stop the full revert and just remove the imx25-rtc compatible? > I am not in a hurry, so we could just wait until the revert landed in > Linus tree. Whatever you think is best. > I'm not taking DT changes through the RTC tree so it should be in a separate patch that will go through Shawn's tree You should also use checkpatch --strict, most of the warnings are correct. > [1] https://www.spinics.net/lists/arm-kernel/msg617113.html > [2] commit ee76f7729babd2700afd6f3874449d8084dd85ea > > To: Alessandro Zummo > To: Alexandre Belloni > Cc: Rob Herring > Cc: Mark Rutland (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Cc: linux-rtc@vger.kernel.org (open list:REAL TIME CLOCK (RTC) SUBSYSTEM) > Cc: devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Cc: linux-kernel@vger.kernel.org (open list) > Cc: Fabio Estevam > Cc: Juergen Borleis > Cc: Noel Vellemans > Cc: Shawn Guo > Cc: Sascha Hauer (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE) > Cc: Russell King (maintainer:ARM PORT) > Cc: linux-arm-kernel@lists.infradead.org (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE) > --- > arch/arm/boot/dts/imx53.dtsi | 2 +- > drivers/rtc/Makefile | 1 + > drivers/rtc/rtc-mxc_v2.c | 531 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 533 insertions(+), 1 deletion(-) > create mode 100644 drivers/rtc/rtc-mxc_v2.c > > diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi > index 589a67c5f796..3d1a55e11ea8 100644 > --- a/arch/arm/boot/dts/imx53.dtsi > +++ b/arch/arm/boot/dts/imx53.dtsi > @@ -434,7 +434,7 @@ > }; > > srtc: srtc@53fa4000 { > - compatible = "fsl,imx53-rtc", "fsl,imx25-rtc"; > + compatible = "fsl,imx53-rtc"; > reg = <0x53fa4000 0x4000>; > interrupts = <24>; > interrupt-parent = <&tzic>; > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile > index f2f50c11dc38..fb3dc458c185 100644 > --- a/drivers/rtc/Makefile > +++ b/drivers/rtc/Makefile > @@ -106,6 +106,7 @@ obj-$(CONFIG_RTC_DRV_MT6397) += rtc-mt6397.o > obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o > obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o > obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o > +obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc_v2.o This definitively needs a different Kconfig symbol. > obj-$(CONFIG_RTC_DRV_NUC900) += rtc-nuc900.o > obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o > obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o > diff --git a/drivers/rtc/rtc-mxc_v2.c b/drivers/rtc/rtc-mxc_v2.c > new file mode 100644 > index 000000000000..5049b521b38e > --- /dev/null > +++ b/drivers/rtc/rtc-mxc_v2.c > @@ -0,0 +1,531 @@ I think SPDX identifier will soon be required on new files, please add one. > +/* > + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights Reserved. > + */ > + > +/* > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > +/* > + * Implementation based on rtc-ds1553.c > + */ > + > +/*! > + * @defgroup RTC Real Time Clock (RTC) Driver for i.MX53 > + */ > +/*! > + * @file rtc-mxc_v2.c > + * @brief Real Time Clock interface > + * > + * This file contains Real Time Clock interface for Linux. > + * > + * @ingroup RTC > + */ > + Is that comment really useful? > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include This should be in alphabetical order > +//#include This is not useful > +#define RTC_READ_TIME_47BIT _IOR('p', 0x20, unsigned long long) > +/* blocks until LPSCMR is set, returns difference */ > +#define RTC_WAIT_TIME_SET _IOR('p', 0x21, int64_t) > + Those ioctls are not used. > +#define SRTC_LPSCLR_LLPSC_LSH 17 /* start bit for LSB time value */ > + > +#define SRTC_LPPDR_INIT 0x41736166 /* init for glitch detect */ > + > +#define SRTC_LPCR_SWR_LP (1 << 0) /* lp software reset */ > +#define SRTC_LPCR_EN_LP (1 << 3) /* lp enable */ > +#define SRTC_LPCR_WAE (1 << 4) /* lp wakeup alarm enable */ > +#define SRTC_LPCR_SAE (1 << 5) /* lp security alarm enable */ > +#define SRTC_LPCR_SI (1 << 6) /* lp security interrupt enable */ > +#define SRTC_LPCR_ALP (1 << 7) /* lp alarm flag */ > +#define SRTC_LPCR_LTC (1 << 8) /* lp lock time counter */ > +#define SRTC_LPCR_LMC (1 << 9) /* lp lock monotonic counter */ > +#define SRTC_LPCR_SV (1 << 10) /* lp security violation */ > +#define SRTC_LPCR_NSA (1 << 11) /* lp non secure access */ > +#define SRTC_LPCR_NVEIE (1 << 12) /* lp non valid state exit int en */ > +#define SRTC_LPCR_IEIE (1 << 13) /* lp init state exit int enable */ > +#define SRTC_LPCR_NVE (1 << 14) /* lp non valid state exit bit */ > +#define SRTC_LPCR_IE (1 << 15) /* lp init state exit bit */ > + > +#define SRTC_LPCR_ALL_INT_EN (SRTC_LPCR_WAE | SRTC_LPCR_SAE | \ > + SRTC_LPCR_SI | SRTC_LPCR_ALP | \ > + SRTC_LPCR_NVEIE | SRTC_LPCR_IEIE) > + > +#define SRTC_LPSR_TRI (1 << 0) /* lp time read invalidate */ > +#define SRTC_LPSR_PGD (1 << 1) /* lp power supply glitc detected */ > +#define SRTC_LPSR_CTD (1 << 2) /* lp clock tampering detected */ > +#define SRTC_LPSR_ALP (1 << 3) /* lp alarm flag */ > +#define SRTC_LPSR_MR (1 << 4) /* lp monotonic counter rollover */ > +#define SRTC_LPSR_TR (1 << 5) /* lp time rollover */ > +#define SRTC_LPSR_EAD (1 << 6) /* lp external alarm detected */ > +#define SRTC_LPSR_IT0 (1 << 7) /* lp IIM throttle */ > +#define SRTC_LPSR_IT1 (1 << 8) > +#define SRTC_LPSR_IT2 (1 << 9) > +#define SRTC_LPSR_SM0 (1 << 10) /* lp security mode */ > +#define SRTC_LPSR_SM1 (1 << 11) > +#define SRTC_LPSR_STATE_LP0 (1 << 12) /* lp state */ > +#define SRTC_LPSR_STATE_LP1 (1 << 13) > +#define SRTC_LPSR_NVES (1 << 14) /* lp non-valid state exit status */ > +#define SRTC_LPSR_IES (1 << 15) /* lp init state exit status */ > + > +#define MAX_PIE_NUM 15 > +#define MAX_PIE_FREQ 32768 > +#define MIN_PIE_FREQ 1 > + > +#define SRTC_PI0 (1 << 0) > +#define SRTC_PI1 (1 << 1) > +#define SRTC_PI2 (1 << 2) > +#define SRTC_PI3 (1 << 3) > +#define SRTC_PI4 (1 << 4) > +#define SRTC_PI5 (1 << 5) > +#define SRTC_PI6 (1 << 6) > +#define SRTC_PI7 (1 << 7) > +#define SRTC_PI8 (1 << 8) > +#define SRTC_PI9 (1 << 9) > +#define SRTC_PI10 (1 << 10) > +#define SRTC_PI11 (1 << 11) > +#define SRTC_PI12 (1 << 12) > +#define SRTC_PI13 (1 << 13) > +#define SRTC_PI14 (1 << 14) > +#define SRTC_PI15 (1 << 15) > + > +#define PIT_ALL_ON (SRTC_PI1 | SRTC_PI2 | SRTC_PI3 | \ > + SRTC_PI4 | SRTC_PI5 | SRTC_PI6 | SRTC_PI7 | \ > + SRTC_PI8 | SRTC_PI9 | SRTC_PI10 | SRTC_PI11 | \ > + SRTC_PI12 | SRTC_PI13 | SRTC_PI14 | SRTC_PI15) > + > +#define SRTC_SWR_HP (1 << 0) /* hp software reset */ > +#define SRTC_EN_HP (1 << 3) /* hp enable */ > +#define SRTC_TS (1 << 4) /* time synchronize hp with lp */ > + > +#define SRTC_IE_AHP (1 << 16) /* Alarm HP Interrupt Enable bit */ > +#define SRTC_IE_WDHP (1 << 18) /* Write Done HP Interrupt Enable bit */ > +#define SRTC_IE_WDLP (1 << 19) /* Write Done LP Interrupt Enable bit */ > + > +#define SRTC_ISR_AHP (1 << 16) /* interrupt status: alarm hp */ > +#define SRTC_ISR_WDHP (1 << 18) /* interrupt status: write done hp */ > +#define SRTC_ISR_WDLP (1 << 19) /* interrupt status: write done lp */ > +#define SRTC_ISR_WPHP (1 << 20) /* interrupt status: write pending hp */ > +#define SRTC_ISR_WPLP (1 << 21) /* interrupt status: write pending lp */ > + > +#define SRTC_LPSCMR 0x00 /* LP Secure Counter MSB Reg */ > +#define SRTC_LPSCLR 0x04 /* LP Secure Counter LSB Reg */ > +#define SRTC_LPSAR 0x08 /* LP Secure Alarm Reg */ > +#define SRTC_LPSMCR 0x0C /* LP Secure Monotonic Counter Reg */ > +#define SRTC_LPCR 0x10 /* LP Control Reg */ > +#define SRTC_LPSR 0x14 /* LP Status Reg */ > +#define SRTC_LPPDR 0x18 /* LP Power Supply Glitch Detector Reg */ > +#define SRTC_LPGR 0x1C /* LP General Purpose Reg */ > +#define SRTC_HPCMR 0x20 /* HP Counter MSB Reg */ > +#define SRTC_HPCLR 0x24 /* HP Counter LSB Reg */ > +#define SRTC_HPAMR 0x28 /* HP Alarm MSB Reg */ > +#define SRTC_HPALR 0x2C /* HP Alarm LSB Reg */ > +#define SRTC_HPCR 0x30 /* HP Control Reg */ > +#define SRTC_HPISR 0x34 /* HP Interrupt Status Reg */ > +#define SRTC_HPIENR 0x38 /* HP Interrupt Enable Reg */ > + > +#define SRTC_SECMODE_MASK 0x3 /* the mask of SRTC security mode */ > +#define SRTC_SECMODE_LOW 0x0 /* Low Security */ > +#define SRTC_SECMODE_MED 0x1 /* Medium Security */ > +#define SRTC_SECMODE_HIGH 0x2 /* High Security */ > +#define SRTC_SECMODE_RESERVED 0x3 /* Reserved */ > + > +struct rtc_drv_data { > + struct rtc_device *rtc; > + void __iomem *ioaddr; > + int irq; > + struct clk *clk; > +}; > + > +static DEFINE_SPINLOCK(rtc_lock); > + Shouldn't that lock be part of rtc_drv_data? > +/*! > + * This function does write synchronization for writes to the lp srtc block. > + * To take care of the asynchronous CKIL clock, all writes from the IP domain > + * will be synchronized to the CKIL domain. > + */ > +static inline void rtc_write_sync_lp(void __iomem *ioaddr) > +{ > + unsigned int i, count; > + /* Wait for 3 CKIL cycles */ > + for (i = 0; i < 3; i++) { > + count = readl(ioaddr + SRTC_LPSCLR); > + while ((readl(ioaddr + SRTC_LPSCLR)) == count) > + ; > + } > +} > + > +/*! > + * This function updates the RTC alarm registers and then clears all the > + * interrupt status bits. > + * > + * @param alrm the new alarm value to be updated in the RTC > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int rtc_update_alarm(struct device *dev, struct rtc_time *alrm) > +{ > + struct rtc_drv_data *pdata = dev_get_drvdata(dev); > + void __iomem *ioaddr = pdata->ioaddr; > + struct rtc_time alarm_tm, now_tm; > + unsigned long now, time; > + int ret; > + > + now = __raw_readl(ioaddr + SRTC_LPSCMR); > + rtc_time_to_tm(now, &now_tm); > + > + alarm_tm.tm_year = now_tm.tm_year; > + alarm_tm.tm_mon = now_tm.tm_mon; > + alarm_tm.tm_mday = now_tm.tm_mday; > + > + alarm_tm.tm_hour = alrm->tm_hour; > + alarm_tm.tm_min = alrm->tm_min; > + alarm_tm.tm_sec = alrm->tm_sec; > + > + rtc_tm_to_time(&now_tm, &now); > + rtc_tm_to_time(&alarm_tm, &time); > + > + if (time < now) { > + time += 60 * 60 * 24; > + rtc_time_to_tm(time, &alarm_tm); > + } > + ret = rtc_tm_to_time(&alarm_tm, &time); > + > + __raw_writel(time, ioaddr + SRTC_LPSAR); > + > + /* clear alarm interrupt status bit */ > + __raw_writel(SRTC_LPSR_ALP, ioaddr + SRTC_LPSR); > + > + return ret; > +} > + > +/*! > + * This function is the RTC interrupt service routine. > + * > + * @param irq RTC IRQ number > + * @param dev_id device ID which is not used > + * > + * @return IRQ_HANDLED as defined in the include/linux/interrupt.h file. > + */ > +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id) > +{ > + struct platform_device *pdev = dev_id; > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + void __iomem *ioaddr = pdata->ioaddr; > + u32 lp_status, lp_cr; > + u32 events = 0; > + > + lp_status = __raw_readl(ioaddr + SRTC_LPSR); > + lp_cr = __raw_readl(ioaddr + SRTC_LPCR); > + > + /* update irq data & counter */ > + if (lp_status & SRTC_LPSR_ALP) { > + if (lp_cr & SRTC_LPCR_ALP) > + events |= (RTC_AF | RTC_IRQF); > + > + /* disable further lp alarm interrupts */ > + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE); > + } > + > + /* Update interrupt enables */ > + __raw_writel(lp_cr, ioaddr + SRTC_LPCR); > + > + /* clear interrupt status */ > + __raw_writel(lp_status, ioaddr + SRTC_LPSR); > + > + rtc_write_sync_lp(ioaddr); > + rtc_update_irq(pdata->rtc, 1, events); > + return IRQ_HANDLED; > +} > + > +/*! > + * This function reads the current RTC time into tm in Gregorian date. > + * > + * @param tm contains the RTC time value upon return > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct rtc_drv_data *pdata = dev_get_drvdata(dev); > + time_t now = readl(pdata->ioaddr + SRTC_LPSCMR); > + > + rtc_time_to_tm(now, tm); > + return rtc_valid_tm(tm); > +} > + > +/*! > + * This function sets the internal RTC time based on tm in Gregorian date. > + * > + * @param tm the time value to be set in the RTC > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int mxc_rtc_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct rtc_drv_data *pdata = dev_get_drvdata(dev); > + time64_t time = rtc_tm_to_time64(tm); > + > + if (time > UINT_MAX) { > + dev_warn(dev, "time_t has overflow\n"); > + } > + > + writel(time, pdata->ioaddr + SRTC_LPSCMR); > + rtc_write_sync_lp(pdata->ioaddr); > + return 0; > +} > + > +/*! > + * This function reads the current alarm value into the passed in \b alrm > + * argument. It updates the \b alrm's pending field value based on the whether > + * an alarm interrupt occurs or not. > + * > + * @param alrm contains the RTC alarm value upon return > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int mxc_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) > +{ > + struct rtc_drv_data *pdata = dev_get_drvdata(dev); > + void __iomem *ioaddr = pdata->ioaddr; > + > + rtc_time_to_tm(__raw_readl(ioaddr + SRTC_LPSAR), &alrm->time); > + alrm->pending = > + ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_ALP) != 0) ? 1 : 0; > + > + return 0; > +} > + > +static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enable) > +{ > + struct rtc_drv_data *pdata = dev_get_drvdata(dev); > + unsigned long lock_flags = 0; > + u32 lp_cr; > + > + spin_lock_irqsave(&rtc_lock, lock_flags); > + lp_cr = __raw_readl(pdata->ioaddr + SRTC_LPCR); > + > + if (enable) > + lp_cr |= (SRTC_LPCR_ALP | SRTC_LPCR_WAE); > + else > + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE); > + > + __raw_writel(lp_cr, pdata->ioaddr + SRTC_LPCR); > + spin_unlock_irqrestore(&rtc_lock, lock_flags); > + return 0; > +} > + > +/*! > + * This function sets the RTC alarm based on passed in alrm. > + * > + * @param alrm the alarm value to be set in the RTC > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > +{ > + struct rtc_drv_data *pdata = dev_get_drvdata(dev); > + void __iomem *ioaddr = pdata->ioaddr; > + int ret; > + > + ret = rtc_update_alarm(dev, &alrm->time); > + if (!ret) > + mxc_rtc_alarm_irq_enable(dev, alrm->enabled); > + > + rtc_write_sync_lp(ioaddr); > + return ret; > +} > + > +/*! > + * The RTC driver structure > + */ > +static const struct rtc_class_ops mxc_rtc_ops = { > + .read_time = mxc_rtc_read_time, > + .set_time = mxc_rtc_set_time, > + .read_alarm = mxc_rtc_read_alarm, > + .set_alarm = mxc_rtc_set_alarm, > + .alarm_irq_enable = mxc_rtc_alarm_irq_enable, > +}; > + > +/*! MXC RTC Power management control */ This comment is probably wrong ;) > +static int mxc_rtc_probe(struct platform_device *pdev) > +{ > + struct timespec tv; > + struct resource *res; > + struct rtc_drv_data *pdata = NULL; > + void __iomem *ioaddr; > + int ret = 0; > + > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; > + > + pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(pdata->ioaddr)) > + return PTR_ERR(pdata->ioaddr); > + > + ioaddr = pdata->ioaddr; > + > + pdata->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(pdata->clk)) { > + dev_err(&pdev->dev, "unable to get rtc clock!\n"); > + return PTR_ERR(pdata->clk); > + } > + ret = clk_prepare_enable(pdata->clk); > + if (ret) > + return ret; > + > + /* Configure and enable the RTC */ > + pdata->irq = platform_get_irq(pdev, 0); > + if (pdata->irq >= 0 > + && devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt, > + IRQF_SHARED, pdev->name, pdev) < 0) { > + dev_warn(&pdev->dev, "interrupt not available.\n"); > + pdata->irq = -1; > + } > + > + if (pdata->irq >= 0) > + device_init_wakeup(&pdev->dev, 1); > + > + /* initialize glitch detect */ > + __raw_writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR); > + udelay(100); > + > + /* clear lp interrupt status */ > + __raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); > + udelay(100); > + > + /* move out of init state */ > + __raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR); > + > + udelay(100); > + > + while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_IES) == 0) > + ; > + > + /* move out of non-valid state */ > + __raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA | > + SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR); > + > + udelay(100); > + > + while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_NVES) == 0) > + ; > + > + __raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); > + udelay(100); > + > + platform_set_drvdata(pdev, pdata); > + pdata->rtc = > + devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops, > + THIS_MODULE); > + if (IS_ERR(pdata->rtc)) { > + ret = PTR_ERR(pdata->rtc); > + goto exit_put_clk; > + } > + > + tv.tv_nsec = 0; > + tv.tv_sec = __raw_readl(ioaddr + SRTC_LPSCMR); > + > + /* By default, devices should wakeup if they can */ > + /* So srtc is set as "should wakeup" as it can */ That comment is not formatted correctly. > + device_init_wakeup(&pdev->dev, 1); > + > + return ret; For clarity, this has to return 0. > + > +exit_put_clk: > + clk_disable_unprepare(pdata->clk); > + return ret; > +} > + > +static int __exit mxc_rtc_remove(struct platform_device *pdev) > +{ > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(pdata->clk); > + return 0; > +} > + > +/*! > + * This function is called to save the system time delta relative to > + * the MXC RTC when enterring a low power state. This time delta is > + * then used on resume to adjust the system time to account for time > + * loss while suspended. > + * > + * @param pdev not used > + * @param state Power state to enter. > + * > + * @return The function always returns 0. > + */ > +static int mxc_rtc_suspend(struct platform_device *pdev, pm_message_t state) > +{ > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + > + if (device_may_wakeup(&pdev->dev)) > + enable_irq_wake(pdata->irq); > + > + return 0; > +} > + > +/*! > + * This function is called to correct the system time based on the > + * current MXC RTC time relative to the time delta saved during > + * suspend. > + * > + * @param pdev not used > + * > + * @return The function always returns 0. > + */ > +static int mxc_rtc_resume(struct platform_device *pdev) > +{ > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + > + if (device_may_wakeup(&pdev->dev)) > + disable_irq_wake(pdata->irq); > + > + return 0; > +} > + > +static const struct of_device_id mxc_ids[] = { > + {.compatible = "fsl,imx53-rtc",}, > + {} > +}; > + > +/*! > + * Contains pointers to the power management callback functions. > + */ > +static struct platform_driver mxc_rtc_driver = { > + .driver = { > + .name = "mxc_rtc_v8", Isn't it weird to have v8 here when the file is named v2? > + .of_match_table = mxc_ids, > + }, > + .probe = mxc_rtc_probe, > + .remove = mxc_rtc_remove, > + .suspend = mxc_rtc_suspend, > + .resume = mxc_rtc_resume, > +}; > + > +module_platform_driver(mxc_rtc_driver); > + > +MODULE_AUTHOR("Freescale Semiconductor, Inc."); > +MODULE_DESCRIPTION("Realtime Clock Driver (RTC)"); > +MODULE_LICENSE("GPL"); > -- > 2.11.0 > > -- Alexandre Belloni, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com