[PATCH 1/7] rtc: jz4740: set range

[PATCH 1/7] rtc: jz4740: set range

[Alexandre Belloni]

RTC_SEC is a 32-bit seconds counter.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-jz4740.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
index d0a891777f44..079469627bd7 100644
--- a/drivers/rtc/rtc-jz4740.c
+++ b/drivers/rtc/rtc-jz4740.c
@@ -348,10 +348,18 @@ static int jz4740_rtc_probe(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, 1);
 
-	rtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
-					&jz4740_rtc_ops, THIS_MODULE);
+	rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
 	if (IS_ERR(rtc->rtc)) {
 		ret = PTR_ERR(rtc->rtc);
+		dev_err(&pdev->dev, "Failed to allocate rtc device: %d\n", ret);
+		return ret;
+	}
+
+	rtc->rtc->ops = &jz4740_rtc_ops;
+	rtc->rtc->range_max = U32_MAX;
+
+	ret = rtc_register_device(rtc->rtc);
+	if (ret) {
 		dev_err(&pdev->dev, "Failed to register rtc device: %d\n", ret);
 		return ret;
 	}
-- 
2.20.1

[PATCH 2/7] rtc: jz4740: switch to rtc_time64_to_tm/rtc_tm_to_time64

[Alexandre Belloni]

Call the 64bit versions of rtc_tm time conversion now that the range is
enforced by the core.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-jz4740.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
index 079469627bd7..15b6832f3931 100644
--- a/drivers/rtc/rtc-jz4740.c
+++ b/drivers/rtc/rtc-jz4740.c
@@ -171,7 +171,7 @@ static int jz4740_rtc_read_time(struct device *dev, struct rtc_time *time)
 	if (timeout == 0)
 		return -EIO;
 
-	rtc_time_to_tm(secs, time);
+	rtc_time64_to_tm(secs, time);
 
 	return 0;
 }
@@ -196,7 +196,7 @@ static int jz4740_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	alrm->enabled = !!(ctrl & JZ_RTC_CTRL_AE);
 	alrm->pending = !!(ctrl & JZ_RTC_CTRL_AF);
 
-	rtc_time_to_tm(secs, &alrm->time);
+	rtc_time64_to_tm(secs, &alrm->time);
 
 	return rtc_valid_tm(&alrm->time);
 }
@@ -205,9 +205,7 @@ static int jz4740_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
 	int ret;
 	struct jz4740_rtc *rtc = dev_get_drvdata(dev);
-	unsigned long secs;
-
-	rtc_tm_to_time(&alrm->time, &secs);
+	unsigned long secs = rtc_tm_to_time64(&alrm->time);
 
 	ret = jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC_ALARM, secs);
 	if (!ret)
-- 
2.20.1

Re: [PATCH 2/7] rtc: jz4740: switch to rtc_time64_to_tm/rtc_tm_to_time64

[Mathieu Malaterre]

On Thu, Apr 18, 2019 at 12:00 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> Call the 64bit versions of rtc_tm time conversion now that the range is
> enforced by the core.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/rtc/rtc-jz4740.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
> index 079469627bd7..15b6832f3931 100644
> --- a/drivers/rtc/rtc-jz4740.c
> +++ b/drivers/rtc/rtc-jz4740.c
> @@ -171,7 +171,7 @@ static int jz4740_rtc_read_time(struct device *dev, struct rtc_time *time)
>         if (timeout == 0)
>                 return -EIO;
>
> -       rtc_time_to_tm(secs, time);
> +       rtc_time64_to_tm(secs, time);
>
>         return 0;
>  }
> @@ -196,7 +196,7 @@ static int jz4740_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>         alrm->enabled = !!(ctrl & JZ_RTC_CTRL_AE);
>         alrm->pending = !!(ctrl & JZ_RTC_CTRL_AF);
>
> -       rtc_time_to_tm(secs, &alrm->time);
> +       rtc_time64_to_tm(secs, &alrm->time);
>
>         return rtc_valid_tm(&alrm->time);
>  }
> @@ -205,9 +205,7 @@ static int jz4740_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  {
>         int ret;
>         struct jz4740_rtc *rtc = dev_get_drvdata(dev);
> -       unsigned long secs;
> -
> -       rtc_tm_to_time(&alrm->time, &secs);
> +       unsigned long secs = rtc_tm_to_time64(&alrm->time);

nitpick: all other declarations are done with uint32_t, so to make it
clear (maybe) with something like:

uint32_t secs = lower_32_bits(rtc_tm_to_time64(&alrm->time));

Technically I would have stored the full time64_t here and put a
WARN_ON(secs > U32_MAX) but I am not sure what other driver are
supposed to do in this case.

>
>         ret = jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC_ALARM, secs);
>         if (!ret)
> --
> 2.20.1
>

Re: [PATCH 2/7] rtc: jz4740: switch to rtc_time64_to_tm/rtc_tm_to_time64

[Alexandre Belloni]

On 26/04/2019 16:29:36+0200, Mathieu Malaterre wrote:
> On Thu, Apr 18, 2019 at 12:00 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > Call the 64bit versions of rtc_tm time conversion now that the range is
> > enforced by the core.
> >
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > ---
> >  drivers/rtc/rtc-jz4740.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
> > index 079469627bd7..15b6832f3931 100644
> > --- a/drivers/rtc/rtc-jz4740.c
> > +++ b/drivers/rtc/rtc-jz4740.c
> > @@ -171,7 +171,7 @@ static int jz4740_rtc_read_time(struct device *dev, struct rtc_time *time)
> >         if (timeout == 0)
> >                 return -EIO;
> >
> > -       rtc_time_to_tm(secs, time);
> > +       rtc_time64_to_tm(secs, time);
> >
> >         return 0;
> >  }
> > @@ -196,7 +196,7 @@ static int jz4740_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> >         alrm->enabled = !!(ctrl & JZ_RTC_CTRL_AE);
> >         alrm->pending = !!(ctrl & JZ_RTC_CTRL_AF);
> >
> > -       rtc_time_to_tm(secs, &alrm->time);
> > +       rtc_time64_to_tm(secs, &alrm->time);
> >
> >         return rtc_valid_tm(&alrm->time);
> >  }
> > @@ -205,9 +205,7 @@ static int jz4740_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> >  {
> >         int ret;
> >         struct jz4740_rtc *rtc = dev_get_drvdata(dev);
> > -       unsigned long secs;
> > -
> > -       rtc_tm_to_time(&alrm->time, &secs);
> > +       unsigned long secs = rtc_tm_to_time64(&alrm->time);
> 
> nitpick: all other declarations are done with uint32_t, so to make it
> clear (maybe) with something like:
> 
> uint32_t secs = lower_32_bits(rtc_tm_to_time64(&alrm->time));
> 

Ok, I'll change that.

> Technically I would have stored the full time64_t here and put a
> WARN_ON(secs > U32_MAX) but I am not sure what other driver are
> supposed to do in this case.
> 

Because range_max is set properly, the core will ensure that all the
times passed to the driver fit within the range.

Thanks for the review!

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 3/7] rtc: jz4740: remove useless check

[Alexandre Belloni]

rtc_time64_to_tm always returns a valid tm, it is not necessary to validate
it.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-jz4740.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
index 15b6832f3931..a7e1a382cac5 100644
--- a/drivers/rtc/rtc-jz4740.c
+++ b/drivers/rtc/rtc-jz4740.c
@@ -198,7 +198,7 @@ static int jz4740_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 
 	rtc_time64_to_tm(secs, &alrm->time);
 
-	return rtc_valid_tm(&alrm->time);
+	return 0;
 }
 
 static int jz4740_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
-- 
2.20.1

[PATCH 4/7] rtc: jz4740: use .set_time

[Alexandre Belloni]

Use .set_time instead of the deprecated .set_mmss.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-jz4740.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
index a7e1a382cac5..91ccb45bb886 100644
--- a/drivers/rtc/rtc-jz4740.c
+++ b/drivers/rtc/rtc-jz4740.c
@@ -176,11 +176,11 @@ static int jz4740_rtc_read_time(struct device *dev, struct rtc_time *time)
 	return 0;
 }
 
-static int jz4740_rtc_set_mmss(struct device *dev, unsigned long secs)
+static int jz4740_rtc_set_time(struct device *dev, struct rtc_time *time)
 {
 	struct jz4740_rtc *rtc = dev_get_drvdata(dev);
 
-	return jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC, secs);
+	return jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC, rtc_tm_to_time64(time));
 }
 
 static int jz4740_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
@@ -223,7 +223,7 @@ static int jz4740_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
 
 static const struct rtc_class_ops jz4740_rtc_ops = {
 	.read_time	= jz4740_rtc_read_time,
-	.set_mmss	= jz4740_rtc_set_mmss,
+	.set_time	= jz4740_rtc_set_time,
 	.read_alarm	= jz4740_rtc_read_alarm,
 	.set_alarm	= jz4740_rtc_set_alarm,
 	.alarm_irq_enable = jz4740_rtc_alarm_irq_enable,
-- 
2.20.1

[PATCH 5/7] rtc: jz4740: use dev_pm_set_wake_irq() to simplify code

[Alexandre Belloni]

Use dev_pm_set_wake_irq() to set the RTC as a wakeup source for suspend.
This allows to remove the whole dev_pm_ops structure.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-jz4740.c | 37 +++++++------------------------------
 1 file changed, 7 insertions(+), 30 deletions(-)

diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
index 91ccb45bb886..fa15ad9fbe13 100644
--- a/drivers/rtc/rtc-jz4740.c
+++ b/drivers/rtc/rtc-jz4740.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/reboot.h>
 #include <linux/rtc.h>
 #include <linux/slab.h>
@@ -346,6 +347,12 @@ static int jz4740_rtc_probe(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, 1);
 
+	ret = dev_pm_set_wake_irq(&pdev->dev, rtc->irq);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to set wake irq: %d\n", ret);
+		return ret;
+	}
+
 	rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
 	if (IS_ERR(rtc->rtc)) {
 		ret = PTR_ERR(rtc->rtc);
@@ -403,35 +410,6 @@ static int jz4740_rtc_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int jz4740_rtc_suspend(struct device *dev)
-{
-	struct jz4740_rtc *rtc = dev_get_drvdata(dev);
-
-	if (device_may_wakeup(dev))
-		enable_irq_wake(rtc->irq);
-	return 0;
-}
-
-static int jz4740_rtc_resume(struct device *dev)
-{
-	struct jz4740_rtc *rtc = dev_get_drvdata(dev);
-
-	if (device_may_wakeup(dev))
-		disable_irq_wake(rtc->irq);
-	return 0;
-}
-
-static const struct dev_pm_ops jz4740_pm_ops = {
-	.suspend = jz4740_rtc_suspend,
-	.resume  = jz4740_rtc_resume,
-};
-#define JZ4740_RTC_PM_OPS (&jz4740_pm_ops)
-
-#else
-#define JZ4740_RTC_PM_OPS NULL
-#endif  /* CONFIG_PM */
-
 static const struct platform_device_id jz4740_rtc_ids[] = {
 	{ "jz4740-rtc", ID_JZ4740 },
 	{ "jz4780-rtc", ID_JZ4780 },
@@ -443,7 +421,6 @@ static struct platform_driver jz4740_rtc_driver = {
 	.probe	 = jz4740_rtc_probe,
 	.driver	 = {
 		.name  = "jz4740-rtc",
-		.pm    = JZ4740_RTC_PM_OPS,
 		.of_match_table = of_match_ptr(jz4740_rtc_of_match),
 	},
 	.id_table = jz4740_rtc_ids,
-- 
2.20.1

[PATCH 6/7] rtc: jz4740: rework invalid time detection

[Alexandre Belloni]

The scratchpad register is used to detect an invalid time when power to the
RTC has been lost. Instead of deleting that precious information and set
the time to the UNIX epoch, forward it to userspace.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-jz4740.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
index fa15ad9fbe13..c9e16d857fa8 100644
--- a/drivers/rtc/rtc-jz4740.c
+++ b/drivers/rtc/rtc-jz4740.c
@@ -157,6 +157,9 @@ static int jz4740_rtc_read_time(struct device *dev, struct rtc_time *time)
 	uint32_t secs, secs2;
 	int timeout = 5;
 
+	if (jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD) != 0x12345678)
+		return -EINVAL;
+
 	/* If the seconds register is read while it is updated, it can contain a
 	 * bogus value. This can be avoided by making sure that two consecutive
 	 * reads have the same value.
@@ -180,8 +183,13 @@ static int jz4740_rtc_read_time(struct device *dev, struct rtc_time *time)
 static int jz4740_rtc_set_time(struct device *dev, struct rtc_time *time)
 {
 	struct jz4740_rtc *rtc = dev_get_drvdata(dev);
+	int ret;
+
+	ret = jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC, rtc_tm_to_time64(time));
+	if (ret)
+		return ret;
 
-	return jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC, rtc_tm_to_time64(time));
+	return jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SCRATCHPAD, 0x12345678);
 }
 
 static int jz4740_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
@@ -308,7 +316,6 @@ static int jz4740_rtc_probe(struct platform_device *pdev)
 {
 	int ret;
 	struct jz4740_rtc *rtc;
-	uint32_t scratchpad;
 	struct resource *mem;
 	const struct platform_device_id *id = platform_get_device_id(pdev);
 	const struct of_device_id *of_id = of_match_device(
@@ -376,16 +383,6 @@ static int jz4740_rtc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	scratchpad = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD);
-	if (scratchpad != 0x12345678) {
-		ret = jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SCRATCHPAD, 0x12345678);
-		ret = jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC, 0);
-		if (ret) {
-			dev_err(&pdev->dev, "Could not write to RTC registers\n");
-			return ret;
-		}
-	}
-
 	if (np && of_device_is_system_power_controller(np)) {
 		if (!pm_power_off) {
 			/* Default: 60ms */
-- 
2.20.1

[PATCH 7/7] rtc: jz4740: convert to SPDX identifier

[Alexandre Belloni]

Use SPDX-License-Identifier instead of a verbose license text.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-jz4740.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
index c9e16d857fa8..0e95282df525 100644
--- a/drivers/rtc/rtc-jz4740.c
+++ b/drivers/rtc/rtc-jz4740.c
@@ -1,17 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0+
 /*
  *  Copyright (C) 2009-2010, Lars-Peter Clausen <lars@metafoo.de>
  *  Copyright (C) 2010, Paul Cercueil <paul@crapouillou.net>
  *	 JZ4740 SoC RTC driver
- *
- *  This program is free software; you can redistribute it and/or modify it
- *  under  the terms of  the GNU General Public License as published by the
- *  Free Software Foundation;  either version 2 of the License, or (at your
- *  option) any later version.
- *
- *  You should have received a copy of the GNU General Public License along
- *  with this program; if not, write to the Free Software Foundation, Inc.,
- *  675 Mass Ave, Cambridge, MA 02139, USA.
- *
  */
 
 #include <linux/clk.h>
-- 
2.20.1

Re: [PATCH 1/7] rtc: jz4740: set range

[Mathieu Malaterre]

On Thu, Apr 18, 2019 at 12:01 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> RTC_SEC is a 32-bit seconds counter.

For the entire series:

Tested-by: Mathieu Malaterre <malat@debian.org>

Arch is MIPS Creator CI20

>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>  drivers/rtc/rtc-jz4740.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
> index d0a891777f44..079469627bd7 100644
> --- a/drivers/rtc/rtc-jz4740.c
> +++ b/drivers/rtc/rtc-jz4740.c
> @@ -348,10 +348,18 @@ static int jz4740_rtc_probe(struct platform_device *pdev)
>
>         device_init_wakeup(&pdev->dev, 1);
>
> -       rtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> -                                       &jz4740_rtc_ops, THIS_MODULE);
> +       rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
>         if (IS_ERR(rtc->rtc)) {
>                 ret = PTR_ERR(rtc->rtc);
> +               dev_err(&pdev->dev, "Failed to allocate rtc device: %d\n", ret);
> +               return ret;
> +       }
> +
> +       rtc->rtc->ops = &jz4740_rtc_ops;
> +       rtc->rtc->range_max = U32_MAX;
> +
> +       ret = rtc_register_device(rtc->rtc);
> +       if (ret) {
>                 dev_err(&pdev->dev, "Failed to register rtc device: %d\n", ret);
>                 return ret;
>         }
> --
> 2.20.1
>