Patchwork [1/4] rtc: stmp3xxx: add wdt-accessor function

login
register
mail settings
Submitter Wolfram Sang
Date Sept. 28, 2011, 12:29 p.m.
Message ID <1317212997-26668-2-git-send-email-w.sang@pengutronix.de>
Download mbox | patch
Permalink /patch/116780/
State New
Headers show

Comments

Wolfram Sang - Sept. 28, 2011, 12:29 p.m.
This RTC also includes a watchdog timer. Provide an accessor function
for it to be picked up by a watchdog driver. Also register the
platform_device here to get proper boot-time dependencies.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Alessandro Zummo <alessandro.zummo@towertech.it>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: rtc-linux@googlegroups.com
---

Andrew, Wim: Alessandro was not around in the last weeks, so I'd suggest this
patch goes via the watchdog-tree together with the rest of this series if it
passes the review of the watchdog-list? All changes here are in preparation for
the watchdog driver and do not affect the general RTC. Is that okay?

 drivers/rtc/rtc-stmp3xxx.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)
Andrew Morton - Sept. 28, 2011, 7:28 p.m.
On Wed, 28 Sep 2011 14:29:54 +0200
Wolfram Sang <w.sang@pengutronix.de> wrote:

> This RTC also includes a watchdog timer. Provide an accessor function
> for it to be picked up by a watchdog driver. Also register the
> platform_device here to get proper boot-time dependencies.
> 
> ...
>
> Andrew, Wim: Alessandro was not around in the last weeks, so I'd suggest this
> patch goes via the watchdog-tree together with the rest of this series if it
> passes the review of the watchdog-list? All changes here are in preparation for
> the watchdog driver and do not affect the general RTC. Is that okay?

OK by me.  Be careful about the Kconfig setup when doing
cross-subsystem dependencies like this.  Otherwise you get sad emails
from Randy when his build breaks.

>
> ...
>
>  struct stmp3xxx_rtc_data {
>  	struct rtc_device *rtc;
>  	void __iomem *io;
>  	int irq_alarm;
>  };
>  
> +#ifdef CONFIG_STMP3XXX_RTC_WATCHDOG

Should it be

#if defined(CONFIG_STMP3XXX_RTC_WATCHDOG) || defined(CONFIG_STMP3XXX_RTC_WATCHDOG_MODULE)

?

> +struct platform_device stmp3xxx_wdt_pdev = {

static?

> +	.name = "stmp3xxx_rtc_wdt",
> +	.id = -1,
> +};
> +
> +void stmp3xxx_wdt_setup(struct device *dev, u32 timeout)

I'd suggest documenting `timeout' at least.  Why would one want to set
it to zero and what effect does that have?  What are its units?

> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct stmp3xxx_rtc_data *rtc_data = platform_get_drvdata(pdev);
> +	void __iomem *base;
> +
> +	if (timeout) {
> +		writel(timeout, rtc_data->io + STMP3XXX_RTC_WATCHDOG);
> +		base = rtc_data->io + MXS_SET_ADDR;
> +	} else {
> +		base = rtc_data->io + MXS_CLR_ADDR;
> +	}
> +	writel(STMP3XXX_RTC_CTRL_WATCHDOGEN, base + STMP3XXX_RTC_CTRL);
> +	writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
> +		base + STMP3XXX_RTC_PERSISTENT1);
> +}
> +EXPORT_SYMBOL_GPL(stmp3xxx_wdt_setup);

The patch adds a global symbol but fails to declare that symbol in a
header file.  This is always wrong.

> +static void stmp3xxx_wdt_register(struct device *dev)
> +{
> +	stmp3xxx_wdt_pdev.dev.parent = dev;
> +	platform_device_register(&stmp3xxx_wdt_pdev);
> +}
> +#else
> +static void stmp3xxx_wdt_register(struct device *dev)
> +{
> +}
> +#endif /* CONFIG_STMP3XXX_RTC_WATCHDOG */
> +
>  static void stmp3xxx_wait_time(struct stmp3xxx_rtc_data *rtc_data)
>  {
>  	/*
> @@ -231,6 +274,7 @@ static int stmp3xxx_rtc_probe(struct platform_device *pdev)
>  		goto out_irq_alarm;
>  	}
>  
> +	stmp3xxx_wdt_register(&pdev->dev);
>  	return 0;
Wolfram Sang - Sept. 30, 2011, 5:26 p.m.
Hi Andrew,

> > Andrew, Wim: Alessandro was not around in the last weeks, so I'd suggest this
> > patch goes via the watchdog-tree together with the rest of this series if it
> > passes the review of the watchdog-list? All changes here are in preparation for
> > the watchdog driver and do not affect the general RTC. Is that okay?
> 
> OK by me.  Be careful about the Kconfig setup when doing
> cross-subsystem dependencies like this.  Otherwise you get sad emails
> from Randy when his build breaks.

Yes, the watchdog driver depends on the RTC.

> > +	.name = "stmp3xxx_rtc_wdt",
> > +	.id = -1,
> > +};
> > +
> > +void stmp3xxx_wdt_setup(struct device *dev, u32 timeout)
> 
> I'd suggest documenting `timeout' at least.  Why would one want to set
> it to zero and what effect does that have?  What are its units?

See below.

> The patch adds a global symbol but fails to declare that symbol in a
> header file.  This is always wrong.

I admit the aproach was quite wrong. I exported the function but wanted the
watchdog driver to be the only user (and so the parameters of the function were
quite specific to the watchdog driver). Thus, I kind of tried to hide its
interface for other users. I will try to come up with something better,
probably passing the function via the platform_device created at runtime. I
also played with sharing resources, yet that would require a top-level driver
managing the resources and two sub-level drivers (RTC and watchdog) which feels
unneccessary complex to me.

Thanks,

   Wolfram

Patch

diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c
index 7315068..379ceb6 100644
--- a/drivers/rtc/rtc-stmp3xxx.c
+++ b/drivers/rtc/rtc-stmp3xxx.c
@@ -27,6 +27,7 @@ 
 #include <linux/slab.h>
 
 #include <mach/common.h>
+#include <mach/mxs.h>
 
 #define STMP3XXX_RTC_CTRL			0x0
 #define STMP3XXX_RTC_CTRL_SET			0x4
@@ -34,6 +35,7 @@ 
 #define STMP3XXX_RTC_CTRL_ALARM_IRQ_EN		0x00000001
 #define STMP3XXX_RTC_CTRL_ONEMSEC_IRQ_EN	0x00000002
 #define STMP3XXX_RTC_CTRL_ALARM_IRQ		0x00000004
+#define STMP3XXX_RTC_CTRL_WATCHDOGEN		0x00000010
 
 #define STMP3XXX_RTC_STAT			0x10
 #define STMP3XXX_RTC_STAT_STALE_SHIFT		16
@@ -43,6 +45,8 @@ 
 
 #define STMP3XXX_RTC_ALARM			0x40
 
+#define STMP3XXX_RTC_WATCHDOG			0x50
+
 #define STMP3XXX_RTC_PERSISTENT0		0x60
 #define STMP3XXX_RTC_PERSISTENT0_SET		0x64
 #define STMP3XXX_RTC_PERSISTENT0_CLR		0x68
@@ -50,12 +54,51 @@ 
 #define STMP3XXX_RTC_PERSISTENT0_ALARM_EN	0x00000004
 #define STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE	0x00000080
 
+#define STMP3XXX_RTC_PERSISTENT1		0x70
+/* missing bitmask in headers */
+#define STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER	0x80000000
+
 struct stmp3xxx_rtc_data {
 	struct rtc_device *rtc;
 	void __iomem *io;
 	int irq_alarm;
 };
 
+#ifdef CONFIG_STMP3XXX_RTC_WATCHDOG
+struct platform_device stmp3xxx_wdt_pdev = {
+	.name = "stmp3xxx_rtc_wdt",
+	.id = -1,
+};
+
+void stmp3xxx_wdt_setup(struct device *dev, u32 timeout)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct stmp3xxx_rtc_data *rtc_data = platform_get_drvdata(pdev);
+	void __iomem *base;
+
+	if (timeout) {
+		writel(timeout, rtc_data->io + STMP3XXX_RTC_WATCHDOG);
+		base = rtc_data->io + MXS_SET_ADDR;
+	} else {
+		base = rtc_data->io + MXS_CLR_ADDR;
+	}
+	writel(STMP3XXX_RTC_CTRL_WATCHDOGEN, base + STMP3XXX_RTC_CTRL);
+	writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
+		base + STMP3XXX_RTC_PERSISTENT1);
+}
+EXPORT_SYMBOL_GPL(stmp3xxx_wdt_setup);
+
+static void stmp3xxx_wdt_register(struct device *dev)
+{
+	stmp3xxx_wdt_pdev.dev.parent = dev;
+	platform_device_register(&stmp3xxx_wdt_pdev);
+}
+#else
+static void stmp3xxx_wdt_register(struct device *dev)
+{
+}
+#endif /* CONFIG_STMP3XXX_RTC_WATCHDOG */
+
 static void stmp3xxx_wait_time(struct stmp3xxx_rtc_data *rtc_data)
 {
 	/*
@@ -231,6 +274,7 @@  static int stmp3xxx_rtc_probe(struct platform_device *pdev)
 		goto out_irq_alarm;
 	}
 
+	stmp3xxx_wdt_register(&pdev->dev);
 	return 0;
 
 out_irq_alarm: