Date:   Wed, 8 Nov 2017 04:44:09 +0100
From:   Alexandre Belloni <alexandre.belloni@...e-electrons.com>
To:     Baolin Wang <baolin.wang@...eadtrum.com>
Cc:     a.zummo@...ertech.it, robh+dt@...nel.org, mark.rutland@....com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-rtc@...r.kernel.org, broonie@...nel.org,
        baolin.wang@...aro.org
Subject: Re: [PATCH 2/2] rtc: sprd: Add Spreadtrum RTC driver

Hi,

On 07/11/2017 at 19:34:08 +0800, Baolin Wang wrote:
> This patch adds the Spreadtrum RTC driver, which embedded in the
> Spreadtrum SC27xx PMIC series.
> 
> Signed-off-by: Baolin Wang <baolin.wang@...eadtrum.com>
> ---
>  drivers/rtc/Kconfig    |    7 +
>  drivers/rtc/Makefile   |    1 +
>  drivers/rtc/rtc-sprd.c |  673 ++++++++++++++++++++++++++++++++++++++++++++++++

I would prefer that filename to be more specific. What if at some point
one of your SoC includes an RTC that is not handled by this driver. This
will become a naming nightmare.

>  3 files changed, 681 insertions(+)
>  create mode 100644 drivers/rtc/rtc-sprd.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e0e58f3..c3059fa 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1182,6 +1182,13 @@ config RTC_DRV_SPEAR
>  	 If you say Y here you will get support for the RTC found on
>  	 spear
>  
> +config RTC_DRV_SPRD
> +	tristate "Spreadtrum SC27xx RTC"
> +	depends on ARCH_SPRD || COMPILE_TEST

the commit message says this is a PMIC RTC, not a SoC RTC. I don't think
you need to depend on ARCH_SPRD. But you probably need to depend on the
MFD parent.

> +	help
> +	 If you say Y here you will get support for the RTC found on
> +	 Spreadtrum.

Be mor precise here.

> +
>  config RTC_DRV_PCF50633
>  	depends on MFD_PCF50633
>  	tristate "NXP PCF50633 RTC"
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 7230014..ec5cb75 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -147,6 +147,7 @@ obj-$(CONFIG_RTC_DRV_SH)	+= rtc-sh.o
>  obj-$(CONFIG_RTC_DRV_SIRFSOC)	+= rtc-sirfsoc.o
>  obj-$(CONFIG_RTC_DRV_SNVS)	+= rtc-snvs.o
>  obj-$(CONFIG_RTC_DRV_SPEAR)	+= rtc-spear.o
> +obj-$(CONFIG_RTC_DRV_SPRD)	+= rtc-sprd.o
>  obj-$(CONFIG_RTC_DRV_STARFIRE)	+= rtc-starfire.o
>  obj-$(CONFIG_RTC_DRV_STK17TA8)	+= rtc-stk17ta8.o
>  obj-$(CONFIG_RTC_DRV_STM32) 	+= rtc-stm32.o
> diff --git a/drivers/rtc/rtc-sprd.c b/drivers/rtc/rtc-sprd.c
> new file mode 100644
> index 0000000..6b2477f
> --- /dev/null
> +++ b/drivers/rtc/rtc-sprd.c
> @@ -0,0 +1,673 @@
> +/*
> + * Copyright (C) 2017 Spreadtrum Communications Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/rtc.h>
> +
> +#define SPRD_RTC_SEC_CNT_VALUE		0x0
> +#define SPRD_RTC_MIN_CNT_VALUE		0x4
> +#define SPRD_RTC_HOUR_CNT_VALUE		0x8
> +#define SPRD_RTC_DAY_CNT_VALUE		0xc
> +#define SPRD_RTC_SEC_CNT_UPD		0x10
> +#define SPRD_RTC_MIN_CNT_UPD		0x14
> +#define SPRD_RTC_HOUR_CNT_UPD		0x18
> +#define SPRD_RTC_DAY_CNT_UPD		0x1c
> +#define SPRD_RTC_SEC_ALM_UPD		0x20
> +#define SPRD_RTC_MIN_ALM_UPD		0x24
> +#define SPRD_RTC_HOUR_ALM_UPD		0x28
> +#define SPRD_RTC_DAY_ALM_UPD		0x2c
> +#define SPRD_RTC_INT_EN			0x30
> +#define SPRD_RTC_INT_RAW_STS		0x34
> +#define SPRD_RTC_INT_CLR		0x38
> +#define SPRD_RTC_INT_MASK_STS		0x3C
> +#define SPRD_RTC_SEC_ALM_VALUE		0x40
> +#define SPRD_RTC_MIN_ALM_VALUE		0x44
> +#define SPRD_RTC_HOUR_ALM_VALUE		0x48
> +#define SPRD_RTC_DAY_ALM_VALUE		0x4c
> +#define SPRD_RTC_SPG_VALUE		0x50
> +#define SPRD_RTC_SPG_UPD		0x54
> +#define SPRD_RTC_SEC_AUXALM_UPD		0x60
> +#define SPRD_RTC_MIN_AUXALM_UPD		0x64
> +#define SPRD_RTC_HOUR_AUXALM_UPD	0x68
> +#define SPRD_RTC_DAY_AUXALM_UPD		0x6c
> +
> +/* BIT & MASK definition for SPRD_RTC_INT_* registers */
> +#define SPRD_RTC_SEC_EN			BIT(0)
> +#define SPRD_RTC_MIN_EN			BIT(1)
> +#define SPRD_RTC_HOUR_EN		BIT(2)
> +#define SPRD_RTC_DAY_EN			BIT(3)
> +#define SPRD_RTC_ALARM_EN		BIT(4)
> +#define SPRD_RTC_HRS_FORMAT_EN		BIT(5)
> +#define SPRD_RTC_AUXALM_EN		BIT(6)
> +#define SPRD_RTC_SPG_UPD_EN		BIT(7)
> +#define SPRD_RTC_SEC_UPD_EN		BIT(8)
> +#define SPRD_RTC_MIN_UPD_EN		BIT(9)
> +#define SPRD_RTC_HOUR_UPD_EN		BIT(10)
> +#define SPRD_RTC_DAY_UPD_EN		BIT(11)
> +#define SPRD_RTC_ALMSEC_UPD_EN		BIT(12)
> +#define SPRD_RTC_ALMMIN_UPD_EN		BIT(13)
> +#define SPRD_RTC_ALMHOUR_UPD_EN		BIT(14)
> +#define SPRD_RTC_ALMDAY_UPD_EN		BIT(15)
> +#define SPRD_RTC_INT_MASK		GENMASK(15, 0)
> +
> +#define SPRD_RTC_TIME_INT_MASK				\
> +	(SPRD_RTC_SEC_UPD_EN | SPRD_RTC_MIN_UPD_EN |	\
> +	 SPRD_RTC_HOUR_UPD_EN | SPRD_RTC_DAY_UPD_EN)
> +
> +#define SPRD_RTC_ALMTIME_INT_MASK				\
> +	(SPRD_RTC_ALMSEC_UPD_EN | SPRD_RTC_ALMMIN_UPD_EN |	\
> +	 SPRD_RTC_ALMHOUR_UPD_EN | SPRD_RTC_ALMDAY_UPD_EN)
> +
> +#define SPRD_RTC_ALM_INT_MASK			\
> +	(SPRD_RTC_SEC_EN | SPRD_RTC_MIN_EN |	\
> +	 SPRD_RTC_HOUR_EN | SPRD_RTC_DAY_EN |	\
> +	 SPRD_RTC_ALARM_EN | SPRD_RTC_AUXALM_EN)
> +
> +/* second/minute/hour/day values mask definition */
> +#define SPRD_RTC_SEC_MASK		GENMASK(5, 0)
> +#define SPRD_RTC_MIN_MASK		GENMASK(5, 0)
> +#define SPRD_RTC_HOUR_MASK		GENMASK(4, 0)
> +#define SPRD_RTC_DAY_MASK		GENMASK(15, 0)
> +
> +/* alarm lock definition for SPRD_RTC_SPG_UPD register */
> +#define SPRD_RTC_ALMLOCK_MASK		GENMASK(7, 0)
> +#define SPRD_RTC_ALM_UNLOCK		0xa5
> +#define SPRD_RTC_ALM_LOCK		(~SPRD_RTC_ALM_UNLOCK & SPRD_RTC_ALMLOCK_MASK)
> +
> +/* SPG values definition for SPRD_RTC_SPG_UPD register */
> +#define SPRD_RTC_POWEROFF_ALM_FLAG	BIT(8)
> +#define SPRD_RTC_POWER_RESET_FLAG	BIT(9)
> +
> +/* timeout of synchronizing time and alarm registers (us) */
> +#define SPRD_RTC_POLL_TIMEOUT		200000
> +#define SPRD_RTC_POLL_DELAY_US		20000
> +#define SPRD_RTC_START_YEAR		2017
> +
> +struct sprd_rtc {
> +	struct rtc_device	*rtc;
> +	struct regmap		*regmap;
> +	struct device		*dev;
> +	u32			base;
> +	int			irq;
> +};
> +
> +/*
> + * The Spreadtrum RTC controller has 3 groups registers, including time, normal
> + * alarm and auxiliary alarm. The time group registers are used to set RTC time,
> + * the normal alarm registers are used to set normal alarm, and the auxiliary
> + * alarm registers are used to set auxiliary alarm. Both alarm event and
> + * auxiliary alarm event can wake up system from deep sleep, but only alarm
> + * event can power up system from power down status.
> + */
> +enum sprd_rtc_reg_types {
> +	SPRD_RTC_TIME,
> +	SPRD_RTC_ALARM,
> +	SPRD_RTC_AUX_ALARM,
> +};
> +
> +static int sprd_rtc_read(struct sprd_rtc *rtc, u32 reg, u32 *val)
> +{
> +	return regmap_read(rtc->regmap, rtc->base + reg, val);
> +}
> +
> +static int sprd_rtc_write(struct sprd_rtc *rtc, u32 reg, u32 val)
> +{
> +	return regmap_write(rtc->regmap, rtc->base + reg, val);
> +}
> +
> +static int sprd_rtc_update(struct sprd_rtc *rtc, u32 reg, u32 mask, u32 val)
> +{
> +	return regmap_update_bits(rtc->regmap, rtc->base + reg, mask, val);
> +}
> +
> +static int sprd_rtc_read_poll(struct sprd_rtc *rtc, u32 reg, u32 mask)
> +{
> +	u32 val;
> +
> +	return regmap_read_poll_timeout(rtc->regmap, rtc->base + reg, val,
> +					((val & mask) == mask),
> +					SPRD_RTC_POLL_DELAY_US,
> +					SPRD_RTC_POLL_TIMEOUT);
> +}
> +

All those functions are only wrappers around regmap, they are useless,
please use regmap_* directly.

> +static int sprd_rtc_clear_alarm_ints(struct sprd_rtc *rtc)
> +{
> +	return sprd_rtc_write(rtc, SPRD_RTC_INT_CLR, SPRD_RTC_ALM_INT_MASK);
> +}
> +
> +static int sprd_rtc_disable_ints(struct sprd_rtc *rtc)
> +{
> +	int ret;
> +
> +	ret = sprd_rtc_update(rtc, SPRD_RTC_INT_EN, SPRD_RTC_INT_MASK, 0);
> +	if (ret)
> +		return ret;
> +
> +	return sprd_rtc_write(rtc, SPRD_RTC_INT_CLR, SPRD_RTC_INT_MASK);
> +}
> +
> +static int sprd_rtc_lock_alarm(struct sprd_rtc *rtc)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = sprd_rtc_read(rtc, SPRD_RTC_SPG_VALUE, &val);
> +	if (ret)
> +		return ret;
> +
> +	val &= ~(SPRD_RTC_ALMLOCK_MASK | SPRD_RTC_POWEROFF_ALM_FLAG);
> +	val |= SPRD_RTC_ALM_LOCK;
> +	return sprd_rtc_write(rtc, SPRD_RTC_SPG_UPD, val);
> +}
> +
> +static int sprd_rtc_unlock_alarm(struct sprd_rtc *rtc)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = sprd_rtc_read(rtc, SPRD_RTC_SPG_VALUE, &val);
> +	if (ret)
> +		return ret;
> +
> +	val &= ~(SPRD_RTC_ALMLOCK_MASK | SPRD_RTC_POWEROFF_ALM_FLAG);
> +	val |= SPRD_RTC_ALM_UNLOCK | SPRD_RTC_POWEROFF_ALM_FLAG;
> +	return sprd_rtc_write(rtc, SPRD_RTC_SPG_UPD, val);
> +}
> +
> +static int sprd_rtc_get_secs(struct sprd_rtc *rtc, enum sprd_rtc_reg_types type,
> +			     time64_t *secs)
> +{
> +	u32 sec_reg, min_reg, hour_reg, day_reg;
> +	u32 val, sec, min, hour, day;
> +	int ret;
> +
> +	switch (type) {
> +	case SPRD_RTC_TIME:
> +		sec_reg = SPRD_RTC_SEC_CNT_VALUE;
> +		min_reg = SPRD_RTC_MIN_CNT_VALUE;
> +		hour_reg = SPRD_RTC_HOUR_CNT_VALUE;
> +		day_reg = SPRD_RTC_DAY_CNT_VALUE;
> +		break;
> +	case SPRD_RTC_ALARM:
> +		sec_reg = SPRD_RTC_SEC_ALM_VALUE;
> +		min_reg = SPRD_RTC_MIN_ALM_VALUE;
> +		hour_reg = SPRD_RTC_HOUR_ALM_VALUE;
> +		day_reg = SPRD_RTC_DAY_ALM_VALUE;
> +		break;
> +	case SPRD_RTC_AUX_ALARM:
> +		sec_reg = SPRD_RTC_SEC_AUXALM_UPD;
> +		min_reg = SPRD_RTC_MIN_AUXALM_UPD;
> +		hour_reg = SPRD_RTC_HOUR_AUXALM_UPD;
> +		day_reg = SPRD_RTC_DAY_AUXALM_UPD;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = sprd_rtc_read(rtc, sec_reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	sec = val & SPRD_RTC_SEC_MASK;
> +
> +	ret = sprd_rtc_read(rtc, min_reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	min = val & SPRD_RTC_MIN_MASK;
> +
> +	ret = sprd_rtc_read(rtc, hour_reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	hour = val & SPRD_RTC_HOUR_MASK;
> +
> +	ret = sprd_rtc_read(rtc, day_reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	day = val & SPRD_RTC_DAY_MASK;
> +	*secs = (((time64_t)(day * 24) + hour) * 60 + min) * 60 + sec;
> +	return 0;
> +}
> +
> +static int sprd_rtc_set_secs(struct sprd_rtc *rtc, enum sprd_rtc_reg_types type,
> +			     time64_t secs)
> +{
> +	u32 sec_reg, min_reg, hour_reg, day_reg, sts_mask;
> +	u32 sec, min, hour, day;
> +	int ret, rem;
> +
> +	/* convert seconds to RTC time format */
> +	day = div_s64_rem(secs, 86400, &rem);
> +	hour = rem / 3600;
> +	rem -= hour * 3600;
> +	min = rem / 60;
> +	sec = rem - min * 60;
> +
> +	switch (type) {
> +	case SPRD_RTC_TIME:
> +		sec_reg = SPRD_RTC_SEC_CNT_UPD;
> +		min_reg = SPRD_RTC_MIN_CNT_UPD;
> +		hour_reg = SPRD_RTC_HOUR_CNT_UPD;
> +		day_reg = SPRD_RTC_DAY_CNT_UPD;
> +		sts_mask = SPRD_RTC_TIME_INT_MASK;
> +		break;
> +	case SPRD_RTC_ALARM:
> +		sec_reg = SPRD_RTC_SEC_ALM_UPD;
> +		min_reg = SPRD_RTC_MIN_ALM_UPD;
> +		hour_reg = SPRD_RTC_HOUR_ALM_UPD;
> +		day_reg = SPRD_RTC_DAY_ALM_UPD;
> +		sts_mask = SPRD_RTC_ALMTIME_INT_MASK;
> +		break;
> +	case SPRD_RTC_AUX_ALARM:
> +		sec_reg = SPRD_RTC_SEC_AUXALM_UPD;
> +		min_reg = SPRD_RTC_MIN_AUXALM_UPD;
> +		hour_reg = SPRD_RTC_HOUR_AUXALM_UPD;
> +		day_reg = SPRD_RTC_DAY_AUXALM_UPD;
> +		sts_mask = 0;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = sprd_rtc_write(rtc, sec_reg, sec);
> +	if (ret)
> +		return ret;
> +
> +	ret = sprd_rtc_write(rtc, min_reg, min);
> +	if (ret)
> +		return ret;
> +
> +	ret = sprd_rtc_write(rtc, hour_reg, hour);
> +	if (ret)
> +		return ret;
> +
> +	ret = sprd_rtc_write(rtc, day_reg, day);
> +	if (ret)
> +		return ret;
> +
> +	if (type == SPRD_RTC_AUX_ALARM)
> +		return 0;
> +
> +	/*
> +	 * Since the time and normal alarm registers are put in always-power-on
> +	 * region supplied by VDDRTC, then these registers changing time will
> +	 * be very long, about 125ms. Thus here we should wait until all
> +	 * values are updated successfully.
> +	 */
> +	ret = sprd_rtc_read_poll(rtc, SPRD_RTC_INT_RAW_STS, sts_mask);
> +	if (ret < 0) {
> +		dev_err(rtc->dev, "set time/alarm values timeout\n");
> +		return ret;
> +	}
> +
> +	return sprd_rtc_write(rtc, SPRD_RTC_INT_CLR, sts_mask);
> +}
> +
> +static int sprd_rtc_read_aux_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct sprd_rtc *rtc = dev_get_drvdata(dev);
> +	time64_t secs;
> +	u32 val;
> +	int ret;
> +
> +	ret = sprd_rtc_get_secs(rtc, SPRD_RTC_AUX_ALARM, &secs);
> +	if (ret)
> +		return ret;
> +
> +	rtc_time64_to_tm(secs, &alrm->time);
> +
> +	ret = sprd_rtc_read(rtc, SPRD_RTC_INT_EN, &val);
> +	if (ret)
> +		return ret;
> +
> +	alrm->enabled = !!(val & SPRD_RTC_AUXALM_EN);
> +
> +	ret = sprd_rtc_read(rtc, SPRD_RTC_INT_RAW_STS, &val);
> +	if (ret)
> +		return ret;
> +
> +	alrm->pending = !!(val & SPRD_RTC_AUXALM_EN);
> +	return 0;
> +}
> +
> +static int sprd_rtc_set_aux_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct sprd_rtc *rtc = dev_get_drvdata(dev);
> +	int ret;
> +
> +	/* clear the auxiliary alarm interrupt status */
> +	ret = sprd_rtc_write(rtc, SPRD_RTC_INT_CLR, SPRD_RTC_AUXALM_EN);
> +	if (ret)
> +		return ret;
> +
> +	if (alrm->enabled) {
> +		time64_t secs = rtc_tm_to_time64(&alrm->time);
> +
> +		/* enable the auxiliary alarm interrupt */
> +		ret = sprd_rtc_update(rtc, SPRD_RTC_INT_EN, SPRD_RTC_AUXALM_EN,
> +				      SPRD_RTC_AUXALM_EN);
> +		if (ret)
> +			return ret;
> +
> +		ret = sprd_rtc_set_secs(rtc, SPRD_RTC_AUX_ALARM, secs);
> +		if (ret) {
> +			sprd_rtc_update(rtc, SPRD_RTC_INT_EN,
> +					SPRD_RTC_AUXALM_EN, 0);
> +		}
> +	} else {
> +		/* disable the auxiliary alarm interrupt */
> +		ret = sprd_rtc_update(rtc, SPRD_RTC_INT_EN,
> +				      SPRD_RTC_AUXALM_EN, 0);

This is not setting the alarm registers properly. What if the alarm is
enabled at a later time by sprd_rtc_alarm_irq_enable()?

> +	}
> +
> +	return ret;
> +}
> +
> +static int sprd_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct sprd_rtc *rtc = dev_get_drvdata(dev);
> +	time64_t secs;
> +	int ret;
> +
> +	ret = sprd_rtc_get_secs(rtc, SPRD_RTC_TIME, &secs);
> +	if (ret)
> +		return ret;
> +
> +	rtc_time64_to_tm(secs, tm);
> +	return rtc_valid_tm(tm);
> +}
> +
> +static int sprd_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct sprd_rtc *rtc = dev_get_drvdata(dev);
> +	time64_t secs = rtc_tm_to_time64(tm);
> +
> +	return sprd_rtc_set_secs(rtc, SPRD_RTC_TIME, secs);
> +}
> +
> +static int sprd_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct sprd_rtc *rtc = dev_get_drvdata(dev);
> +	time64_t secs;
> +	int ret;
> +	u32 val;
> +
> +	/*
> +	 * If aie_timer is enabled, we should get the normal alarm time.
> +	 * Otherwise we should get auxiliary alarm time.
> +	 */
> +	if (rtc->rtc && rtc->rtc->aie_timer.enabled == 0)
> +		return sprd_rtc_read_aux_alarm(dev, alrm);
> +
> +	ret = sprd_rtc_get_secs(rtc, SPRD_RTC_ALARM, &secs);
> +	if (ret)
> +		return ret;
> +
> +	rtc_time64_to_tm(secs, &alrm->time);
> +
> +	ret = sprd_rtc_read(rtc, SPRD_RTC_INT_EN, &val);
> +	if (ret)
> +		return ret;
> +
> +	alrm->enabled = !!(val & SPRD_RTC_ALARM_EN);
> +
> +	ret = sprd_rtc_read(rtc, SPRD_RTC_INT_RAW_STS, &val);
> +	if (ret)
> +		return ret;
> +
> +	alrm->pending = !!(val & SPRD_RTC_ALARM_EN);
> +	return 0;
> +}
> +
> +static int sprd_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct sprd_rtc *rtc = dev_get_drvdata(dev);
> +	struct rtc_time aie_time =
> +		rtc_ktime_to_tm(rtc->rtc->aie_timer.node.expires);
> +	int ret;
> +
> +	/*
> +	 * We have 2 groups alarms: normal alarm and auxiliary alarm. Since
> +	 * both normal alarm event and auxiliary alarm event can wake up system
> +	 * from deep sleep, but only alarm event can power up system from power
> +	 * down status. Moreover we do not need to poll about 125ms when
> +	 * updating auxiliary alarm registers. Thus we usually set auxiliary
> +	 * alarm when wake up system from deep sleep, and for other scenarios,
> +	 * we should set normal alarm with polling status.
> +	 *
> +	 * So here we check if the alarm time is set by aie_timer, if yes, we
> +	 * should set normal alarm, if not, we should set auxiliary alarm which
> +	 * means it is just a wake event.
> +	 */
> +	if (!rtc->rtc->aie_timer.enabled || rtc_tm_sub(&aie_time, &alrm->time))
> +		return sprd_rtc_set_aux_alarm(dev, alrm);
> +
> +	/* clear the alarm interrupt status firstly */
> +	ret = sprd_rtc_write(rtc, SPRD_RTC_INT_CLR, SPRD_RTC_ALARM_EN);
> +	if (ret)
> +		return ret;
> +
> +	if (alrm->enabled) {
> +		time64_t secs = rtc_tm_to_time64(&alrm->time);
> +
> +		/* enable the alarm interrupt */
> +		ret = sprd_rtc_update(rtc, SPRD_RTC_INT_EN, SPRD_RTC_ALARM_EN,
> +				      SPRD_RTC_ALARM_EN);
> +		if (ret)
> +			return ret;
> +
> +		ret = sprd_rtc_set_secs(rtc, SPRD_RTC_ALARM, secs);
> +		if (ret)
> +			goto err;
> +
> +		/* unlock the alarm to enable the alarm function. */
> +		ret = sprd_rtc_unlock_alarm(rtc);
> +		if (ret)
> +			goto err;
> +	} else {

ditto.

> +		/* disable the alarm interrupt */
> +		ret = sprd_rtc_update(rtc, SPRD_RTC_INT_EN,
> +				      SPRD_RTC_ALARM_EN, 0);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Lock the alarm function in case fake alarm event will power
> +		 * up systems.
> +		 */
> +		ret = sprd_rtc_lock_alarm(rtc);
> +	}
> +
> +	return ret;
> +
> +err:
> +	sprd_rtc_update(rtc, SPRD_RTC_INT_EN, SPRD_RTC_ALARM_EN, 0);
> +	return ret;
> +}
> +
> +static int sprd_rtc_set_mmss(struct device *dev, time64_t secs)
> +{
> +	struct sprd_rtc *rtc = dev_get_drvdata(dev);
> +
> +	return sprd_rtc_set_secs(rtc, SPRD_RTC_TIME, secs);
> +}

Because set_time is defined, this function will never be called, please
remove it.

> +
> +static int sprd_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct sprd_rtc *rtc = dev_get_drvdata(dev);
> +
> +	if (enabled) {
> +		return sprd_rtc_update(rtc, SPRD_RTC_INT_EN,
> +				       SPRD_RTC_ALARM_EN | SPRD_RTC_AUXALM_EN,
> +				       SPRD_RTC_ALARM_EN | SPRD_RTC_AUXALM_EN);
> +	} else {
> +		return sprd_rtc_update(rtc, SPRD_RTC_INT_EN,
> +				       SPRD_RTC_ALARM_EN | SPRD_RTC_AUXALM_EN,
> +				       0);
> +	}
> +}
> +
> +static const struct rtc_class_ops sprd_rtc_ops = {
> +	.read_time = sprd_rtc_read_time,
> +	.set_time = sprd_rtc_set_time,
> +	.read_alarm = sprd_rtc_read_alarm,
> +	.set_alarm = sprd_rtc_set_alarm,
> +	.set_mmss64 = sprd_rtc_set_mmss,
> +	.alarm_irq_enable = sprd_rtc_alarm_irq_enable,
> +};
> +
> +static irqreturn_t sprd_rtc_handler(int irq, void *dev_id)
> +{
> +	struct sprd_rtc *rtc = dev_id;
> +	int ret;
> +
> +	ret = sprd_rtc_clear_alarm_ints(rtc);
> +	if (ret)
> +		return IRQ_RETVAL(ret);
> +
> +	rtc_update_irq(rtc->rtc, 1, RTC_AF | RTC_IRQF);
> +	return IRQ_HANDLED;
> +}
> +
> +static int sprd_rtc_init_time(struct sprd_rtc *rtc)
> +{
> +	time64_t secs = mktime64(SPRD_RTC_START_YEAR, 1, 1, 0, 0, 0);
> +
> +	dev_dbg(rtc->dev, "RTC power down and reset time.\n");
> +	return sprd_rtc_set_secs(rtc, SPRD_RTC_TIME, secs);
> +}
> +
> +static int sprd_rtc_check_power_down(struct sprd_rtc *rtc)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = sprd_rtc_read(rtc, SPRD_RTC_SPG_VALUE, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val & SPRD_RTC_POWER_RESET_FLAG)
> +		return 0;
> +
> +	ret = sprd_rtc_init_time(rtc);

Don't do that. This is a valuable information. If you know the time is
invalid, return -EINVAL in read_time(). What your are doing here is
confusing the system by making it believe your fake date is the correct
time.

> +	if (ret < 0) {
> +		dev_err(rtc->dev, "failed to initialize RTC time\n");
> +		return ret;
> +	}
> +
> +	val |= SPRD_RTC_POWER_RESET_FLAG;
> +	ret = sprd_rtc_write(rtc, SPRD_RTC_SPG_UPD, val);
> +	if (ret)
> +		return ret;
> +
> +	/* wait until the SPG value is updated successfully */
> +	ret = sprd_rtc_read_poll(rtc, SPRD_RTC_INT_RAW_STS,
> +				 SPRD_RTC_SPG_UPD_EN);
> +	if (ret < 0) {
> +		dev_err(rtc->dev, "failed to update SPG value\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sprd_rtc_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct sprd_rtc *rtc;
> +	int ret;
> +
> +	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> +	if (!rtc)
> +		return -ENOMEM;
> +
> +	rtc->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!rtc->regmap)
> +		return -ENODEV;
> +
> +	ret = of_property_read_u32(node, "reg", &rtc->base);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to get RTC base address\n");
> +		return ret;
> +	}
> +
> +	rtc->irq = platform_get_irq(pdev, 0);
> +	if (rtc->irq < 0) {
> +		dev_err(&pdev->dev, "failed to get RTC irq number\n");
> +		return rtc->irq;
> +	}
> +
> +	rtc->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, rtc);
> +
> +	/* Clear all RTC interrupts and disable all RTC interrupts */
> +	ret = sprd_rtc_disable_ints(rtc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to disable RTC interrupts\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Check if RTC has been powered down, if so, we should reset RTC
> +	 * time to avoid uncertain values in RTC registers.
> +	 */
> +	ret = sprd_rtc_check_power_down(rtc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to check RTC power\n");
> +		return ret;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> +					sprd_rtc_handler,
> +					IRQF_ONESHOT | IRQF_EARLY_RESUME,
> +					pdev->name, rtc);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to request RTC irq\n");
> +		return ret;
> +	}
> +
> +	rtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> +					    &sprd_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(rtc->rtc))
> +		return PTR_ERR(rtc->rtc);
> +
> +	device_init_wakeup(&pdev->dev, 1);
> +	return 0;
> +}
> +
> +static int sprd_rtc_remove(struct platform_device *pdev)
> +{
> +	device_init_wakeup(&pdev->dev, 0);
> +	return 0;
> +}
> +
> +static const struct of_device_id sprd_rtc_of_match[] = {
> +	{ .compatible = "sprd,sc27xx-rtc", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, sprd_rtc_of_match);
> +
> +static struct platform_driver sprd_rtc_driver = {
> +	.driver = {
> +		.name = "sprd-rtc",
> +		.of_match_table = sprd_rtc_of_match,
> +	},
> +	.probe	= sprd_rtc_probe,
> +	.remove = sprd_rtc_remove,
> +};
> +module_platform_driver(sprd_rtc_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Spreadtrum RTC Device Driver");
> +MODULE_AUTHOR("Baolin Wang <baolin.wang@...eadtrum.com>");
> -- 
> 1.7.9.5
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Powered by blists - more mailing lists