Re: [PATCH] rtc: add mxc driver for i.MX53

From: Alexandre Belloni
Date: Wed Nov 29 2017 - 17:11:34 EST

Next message: Kees Cook: "Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE"
Previous message: Lukasz Majewski: "Re: [PATCH v2 6/6] ARM: ep93xx: ts72xx: Add support for BK3 board - ts72xx derivative"
In reply to: linux-kernel-dev: "[PATCH] rtc: add mxc driver for i.MX53"
Next in thread: Philippe Ombredanne: "Re: [PATCH] rtc: add mxc driver for i.MX53"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

A really quick review:

On 28/11/2017 at 08:39:27 +0100, linux-kernel-dev@xxxxxxxxxxxx wrote:
> From: Patrick Bruenn <p.bruenn@xxxxxxxxxxxx>
>
> 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 <p.bruenn@xxxxxxxxxxxx>
>
> ---
>
> 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 <a.zummo@xxxxxxxxxxxx>
> To: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: linux-rtc@xxxxxxxxxxxxxxx (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
> Cc: devicetree@xxxxxxxxxxxxxxx (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: linux-kernel@xxxxxxxxxxxxxxx (open list)
> Cc: Fabio Estevam <fabio.estevam@xxxxxxx>
> Cc: Juergen Borleis <jbe@xxxxxxxxxxxxxx>
> Cc: Noel Vellemans <Noel.Vellemans@xxxxxxxxxxxxx>
> Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
> Cc: Sascha Hauer <kernel@xxxxxxxxxxxxxx> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
> Cc: Russell King <linux@xxxxxxxxxxxxxxx> (maintainer:ARM PORT)
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx (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 <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/rtc.h>
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/uaccess.h>
> +#include <linux/io.h>

This should be in alphabetical order

> +//#include <linux/mxc_srtc.h>

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

Next message: Kees Cook: "Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE"
Previous message: Lukasz Majewski: "Re: [PATCH v2 6/6] ARM: ep93xx: ts72xx: Add support for BK3 board - ts72xx derivative"
In reply to: linux-kernel-dev: "[PATCH] rtc: add mxc driver for i.MX53"
Next in thread: Philippe Ombredanne: "Re: [PATCH] rtc: add mxc driver for i.MX53"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]