[PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device

The original sa1100_rtc_open/sa1100_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will enable/disable the clock, and
register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only enable clock and register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

Signed-off-by: Chao Xie <redacted>
---
 drivers/rtc/rtc-sa1100.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-sa1100.c b/drivers/rtc/rtc-sa1100.c
index 50a5c4a..e933327 100644
--- a/drivers/rtc/rtc-sa1100.c
+++ b/drivers/rtc/rtc-sa1100.c

@@ -218,8 +218,6 @@ static int sa1100_rtc_proc(struct device *dev, struct seq_file *seq)
 }
 
 static const struct rtc_class_ops sa1100_rtc_ops = {
-	.open = sa1100_rtc_open,
-	.release = sa1100_rtc_release,
 	.read_time = sa1100_rtc_read_time,
 	.set_time = sa1100_rtc_set_time,
 	.read_alarm = sa1100_rtc_read_alarm,

@@ -253,6 +251,10 @@ static int sa1100_rtc_probe(struct platform_device *pdev)
 	spin_lock_init(&info->lock);
 	platform_set_drvdata(pdev, info);
 
+	ret = sa1100_rtc_open(&pdev->dev);
+	if (ret)
+		goto err_open;
+
 	/*
 	 * According to the manual we should be able to let RTTR be zero
 	 * and then a default diviser for a 32.768KHz clock is used.

@@ -305,7 +307,9 @@ static int sa1100_rtc_probe(struct platform_device *pdev)
 
 	return 0;
 err_dev:
+	sa1100_rtc_release(&pdev->dev);
 	platform_set_drvdata(pdev, NULL);
+err_open:
 	clk_put(info->clk);
 err_clk:
 	kfree(info);

@@ -318,6 +322,7 @@ static int sa1100_rtc_remove(struct platform_device *pdev)
 
 	if (info) {
 		rtc_device_unregister(info->rtc);
+		sa1100_rtc_release(&pdev->dev);
 		clk_put(info->clk);
 		platform_set_drvdata(pdev, NULL);
 		kfree(info);

-- 
1.7.4.1

The original calculation does not take care of week-of-month and
day-of-week. It will make rdar/rdcr not matched when set the
alarm.

Signed-off-by: Chao Xie <redacted>
---
 drivers/rtc/rtc-pxa.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/rtc/rtc-pxa.c b/drivers/rtc/rtc-pxa.c
index f771b2e..22ea4f5 100644
--- a/drivers/rtc/rtc-pxa.c
+++ b/drivers/rtc/rtc-pxa.c

@@ -62,6 +62,10 @@
 #define RYxR_MONTH_S	5
 #define RYxR_MONTH_MASK	(0xf << RYxR_MONTH_S)
 #define RYxR_DAY_MASK	0x1f
+#define RDxR_WOM_S	20
+#define RDxR_WOM_MASK	(0x7 << RDxR_WOM_S)
+#define RDxR_DOW_S	17
+#define RDxR_DOW_MASK	(0x7 << RDxR_DOW_S)
 #define RDxR_HOUR_S	12
 #define RDxR_HOUR_MASK	(0x1f << RDxR_HOUR_S)
 #define RDxR_MIN_S	6

@@ -100,7 +104,10 @@ static u32 ryxr_calc(struct rtc_time *tm)
 
 static u32 rdxr_calc(struct rtc_time *tm)
 {
-	return (tm->tm_hour << RDxR_HOUR_S) | (tm->tm_min << RDxR_MIN_S)
+	return ((((tm->tm_mday + 6) / 7) << RDxR_WOM_S) & RDxR_WOM_MASK)
+		| (((tm->tm_wday + 1) << RDxR_DOW_S) & RDxR_DOW_MASK)
+		| (tm->tm_hour << RDxR_HOUR_S)
+		| (tm->tm_min << RDxR_MIN_S)
 		| tm->tm_sec;
 }
 

@@ -109,6 +116,7 @@ static void tm_calc(u32 rycr, u32 rdcr, struct rtc_time *tm)
 	tm->tm_year = ((rycr & RYxR_YEAR_MASK) >> RYxR_YEAR_S) - 1900;
 	tm->tm_mon = (((rycr & RYxR_MONTH_MASK) >> RYxR_MONTH_S)) - 1;
 	tm->tm_mday = (rycr & RYxR_DAY_MASK);
+	tm->tm_wday = ((rdcr & RDxR_DOW_MASK) >> RDxR_DOW_S) - 1;
 	tm->tm_hour = (rdcr & RDxR_HOUR_MASK) >> RDxR_HOUR_S;
 	tm->tm_min = (rdcr & RDxR_MIN_MASK) >> RDxR_MIN_S;
 	tm->tm_sec = rdcr & RDxR_SEC_MASK;

-- 
1.7.4.1

the pxa95x rtc need access PBSR register before write to
RTTR, RCNR, RDCR, and RYCR registers.

Signed-off-by: Chao Xie <redacted>
---
 drivers/rtc/rtc-pxa.c |   97 +++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 85 insertions(+), 12 deletions(-)

diff --git a/drivers/rtc/rtc-pxa.c b/drivers/rtc/rtc-pxa.c
index 22ea4f5..29646af 100644
--- a/drivers/rtc/rtc-pxa.c
+++ b/drivers/rtc/rtc-pxa.c

@@ -29,6 +29,7 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/delay.h>
 
 #include <mach/hardware.h>
 

@@ -81,14 +82,28 @@
 #define RTCPICR		0x34
 #define PIAR		0x38
 
+#define PSBR_RTC	0x00
+
 #define rtc_readl(pxa_rtc, reg)	\
 	__raw_readl((pxa_rtc)->base + (reg))
 #define rtc_writel(pxa_rtc, reg, value)	\
 	__raw_writel((value), (pxa_rtc)->base + (reg))
+#define rtc_readl_psbr(pxa_rtc, reg)	\
+	__raw_readl((pxa_rtc)->base_psbr + (reg))
+#define rtc_writel_psbr(pxa_rtc, reg, value)	\
+	__raw_writel((value), (pxa_rtc)->base_psbr + (reg))
+
+enum {
+	RTC_PXA27X,
+	RTC_PXA95X,
+};
 
 struct pxa_rtc {
 	struct resource	*ress;
+	struct resource *ress_psbr;
+	unsigned int		id;
 	void __iomem		*base;
+	void __iomem		*base_psbr;
 	int			irq_1Hz;
 	int			irq_Alrm;
 	struct rtc_device	*rtc;

@@ -250,9 +265,26 @@ static int pxa_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct pxa_rtc *pxa_rtc = dev_get_drvdata(dev);
 
+	/*
+	 * sequence to wirte pxa rtc register RCNR RDCR RYCR is
+	 * 1. set PSBR[RWE] bit, take 2x32-khz to complete
+	 * 2. write to RTC register,take 2x32-khz to complete
+	 * 3. clear PSBR[RWE] bit,take 2x32-khz to complete
+	 */
+	if (pxa_rtc->id == RTC_PXA95X) {
+		rtc_writel_psbr(pxa_rtc, PSBR_RTC, 0x01);
+		udelay(100);
+	}
+
 	rtc_writel(pxa_rtc, RYCR, ryxr_calc(tm));
 	rtc_writel(pxa_rtc, RDCR, rdxr_calc(tm));
 
+	if (pxa_rtc->id == RTC_PXA95X) {
+		udelay(100);
+		rtc_writel_psbr(pxa_rtc, PSBR_RTC, 0x00);
+		udelay(100);
+	}
+
 	return 0;
 }
 

@@ -318,6 +350,20 @@ static const struct rtc_class_ops pxa_rtc_ops = {
 	.proc = pxa_rtc_proc,
 };
 
+static struct of_device_id pxa_rtc_dt_ids[] = {
+	{ .compatible = "marvell,pxa-rtc", .data = (void *)RTC_PXA27X },
+	{ .compatible = "marvell,pxa95x-rtc", .data = (void *)RTC_PXA95X },
+	{}
+};
+MODULE_DEVICE_TABLE(of, pxa_rtc_dt_ids);
+
+static const struct platform_device_id pxa_rtc_id_table[] = {
+	{ "pxa-rtc", RTC_PXA27X },
+	{ "pxa95x-rtc", RTC_PXA95X },
+        { },
+};
+MODULE_DEVICE_TABLE(platform, pxa_rtc_id_table);
+
 static int __init pxa_rtc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;

@@ -332,13 +378,34 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
 	spin_lock_init(&pxa_rtc->lock);
 	platform_set_drvdata(pdev, pxa_rtc);
 
+	if (pdev->dev.of_node) {
+		const struct of_device_id *of_id =
+				of_match_device(pxa_rtc_dt_ids, &pdev->dev);
+
+		pxa_rtc->id = (unsigned int)(of_id->data);
+	} else {
+		const struct platform_device_id *id =
+				platform_get_device_id(pdev);
+
+		pxa_rtc->id = id->driver_data;
+	}
+
 	ret = -ENXIO;
 	pxa_rtc->ress = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!pxa_rtc->ress) {
-		dev_err(dev, "No I/O memory resource defined\n");
+		dev_err(dev, "No I/O memory resource(id=0) defined\n");
 		goto err_ress;
 	}
 
+	if (pxa_rtc->id == RTC_PXA95X) {
+		pxa_rtc->ress_psbr =
+			platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (!pxa_rtc->ress_psbr) {
+			dev_err(dev, "No I/O memory resource(id=1) defined\n");
+			goto err_ress;
+		}
+	}
+
 	pxa_rtc->irq_1Hz = platform_get_irq(pdev, 0);
 	if (pxa_rtc->irq_1Hz < 0) {
 		dev_err(dev, "No 1Hz IRQ resource defined\n");

@@ -355,7 +422,17 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
 				resource_size(pxa_rtc->ress));
 	if (!pxa_rtc->base) {
 		dev_err(&pdev->dev, "Unable to map pxa RTC I/O memory\n");
-		goto err_map;
+		goto err_map_base;
+	}
+
+	if (pxa_rtc->id == RTC_PXA95X) {
+		pxa_rtc->base_psbr = ioremap(pxa_rtc->ress_psbr->start,
+					resource_size(pxa_rtc->ress_psbr));
+		if (!pxa_rtc->base_psbr) {
+			dev_err(&pdev->dev,
+				"Unable to map pxa RTC PSBR I/O memory\n");
+			goto err_map_base_psbr;
+		}
 	}
 
 	/*

@@ -384,9 +461,12 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
 	return 0;
 
 err_rtc_reg:
+	if (pxa_rtc->id == RTC_PXA95X)
+		 iounmap(pxa_rtc->base_psbr);
+err_map_base_psbr:
 	 iounmap(pxa_rtc->base);
+err_map_base:
 err_ress:
-err_map:
 	kfree(pxa_rtc);
 	return ret;
 }

@@ -406,14 +486,6 @@ static int __exit pxa_rtc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_OF
-static struct of_device_id pxa_rtc_dt_ids[] = {
-	{ .compatible = "marvell,pxa-rtc" },
-	{}
-};
-MODULE_DEVICE_TABLE(of, pxa_rtc_dt_ids);
-#endif
-
 #ifdef CONFIG_PM
 static int pxa_rtc_suspend(struct device *dev)
 {

@@ -448,11 +520,12 @@ static struct platform_driver pxa_rtc_driver = {
 		.pm	= &pxa_rtc_pm_ops,
 #endif
 	},
+	.id_table	= pxa_rtc_id_table,
 };
 
 static int __init pxa_rtc_init(void)
 {
-	if (cpu_is_pxa27x() || cpu_is_pxa3xx())
+	if (cpu_is_pxa27x() || cpu_is_pxa3xx() || cpu_is_pxa95x())
 		return platform_driver_probe(&pxa_rtc_driver, pxa_rtc_probe);
 
 	return -ENODEV;

-- 
1.7.4.1

The original pxa_rtc_open/pxa_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

Signed-off-by: Chao Xie <redacted>
---
 drivers/rtc/rtc-pxa.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-pxa.c b/drivers/rtc/rtc-pxa.c
index 29646af..19abeb8 100644
--- a/drivers/rtc/rtc-pxa.c
+++ b/drivers/rtc/rtc-pxa.c

@@ -340,8 +340,6 @@ static int pxa_rtc_proc(struct device *dev, struct seq_file *seq)
 }
 
 static const struct rtc_class_ops pxa_rtc_ops = {
-	.open = pxa_rtc_open,
-	.release = pxa_rtc_release,
 	.read_time = pxa_rtc_read_time,
 	.set_time = pxa_rtc_set_time,
 	.read_alarm = pxa_rtc_read_alarm,

@@ -435,6 +433,11 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = pxa_rtc_open(&pdev->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to request irqs for rtc\n");
+		goto err_open;
+	}
 	/*
 	 * If the clock divider is uninitialized then reset it to the
 	 * default value to get the 1Hz clock.

@@ -461,6 +464,8 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
 	return 0;
 
 err_rtc_reg:
+	pxa_rtc_release(&pdev->dev);
+err_open:
 	if (pxa_rtc->id == RTC_PXA95X)
 		 iounmap(pxa_rtc->base_psbr);
 err_map_base_psbr:

@@ -477,6 +482,8 @@ static int __exit pxa_rtc_remove(struct platform_device *pdev)
 
 	rtc_device_unregister(pxa_rtc->rtc);
 
+	pxa_rtc_release(&pdev->dev);
+
 	spin_lock_irq(&pxa_rtc->lock);
 	iounmap(pxa_rtc->base);
 	spin_unlock_irq(&pxa_rtc->lock);

-- 
1.7.4.1

On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:

The original sa1100_rtc_open/sa1100_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will enable/disable the clock, and
register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only enable clock and register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

NAK.  I don't think you properly understand what's going on here if you
think moving the entire open and release functions into the probe and
remove functions is the right thing to do.

On Wed, Nov 28, 2012 at 09:21:10PM -0500, Chao Xie wrote:

The original pxa_rtc_open/pxa_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

Again, this is wrong.

On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
[off-list ref] wrote:

On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:

quoted

The original sa1100_rtc_open/sa1100_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will enable/disable the clock, and
register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only enable clock and register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

NAK.  I don't think you properly understand what's going on here if you
think moving the entire open and release functions into the probe and
remove functions is the right thing to do.

Since PXA27x & PXA3xx supports dual rtc device at the same time,
user could choose use either of rtc at run time. Then clk & irq are setup
in open().

Chao,
So you shouldn't remove them into probe().

On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
[off-list ref] wrote:

On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
[off-list ref] wrote:

quoted

On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:

quoted

The original sa1100_rtc_open/sa1100_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will enable/disable the clock, and
register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only enable clock and register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

NAK.  I don't think you properly understand what's going on here if you
think moving the entire open and release functions into the probe and
remove functions is the right thing to do.

Since PXA27x & PXA3xx supports dual rtc device at the same time,
user could choose use either of rtc at run time. Then clk & irq are setup
in open().

Chao,
So you shouldn't remove them into probe().

How can user to choose one RTC?
The user API, for example "date" will open the device, get the time
and then close the device.
WHen set a alarm, it is the same routine. open->set->close.
The open routine will enable clock and register irq, close will
disable the clock and unregister irq. It means that if only open is
invoked, rtc begins to work, and after close is invoked rtc stops
working. It means that the user can not get correct time and can not
get right alarm.

On Mon, Dec 3, 2012 at 9:39 AM, Chao Xie [off-list ref] wrote:

On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
[off-list ref] wrote:

quoted

On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
[off-list ref] wrote:

quoted

On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:

quoted

The original sa1100_rtc_open/sa1100_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will enable/disable the clock, and
register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only enable clock and register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

NAK.  I don't think you properly understand what's going on here if you
think moving the entire open and release functions into the probe and
remove functions is the right thing to do.

Since PXA27x & PXA3xx supports dual rtc device at the same time,
user could choose use either of rtc at run time. Then clk & irq are setup
in open().

Chao,
So you shouldn't remove them into probe().

How can user to choose one RTC?
The user API, for example "date" will open the device, get the time
and then close the device.
WHen set a alarm, it is the same routine. open->set->close.
The open routine will enable clock and register irq, close will
disable the clock and unregister irq. It means that if only open is
invoked, rtc begins to work, and after close is invoked rtc stops
working. It means that the user can not get correct time and can not
get right alarm.

hi
I want to correct what i said. For the irq register/unregister i think
can be done at open/release. But for clock enable/disable, i do not
think so. If clock is disabled, as i think RTC will not work. User API
still use open->get_time->close for "date" command. It means that RTC
will not return correct date to user.

CCing Andrew as he is the maintainer of the rtc subsystem

On Wed, Nov 28, 2012 at 9:21 PM, Chao Xie [off-list ref] wrote:

On Mon, Dec 3, 2012 at 9:39 AM, Chao Xie [off-list ref] wrote:

On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
[off-list ref] wrote:

quoted

On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
[off-list ref] wrote:

quoted

On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:

quoted

The original sa1100_rtc_open/sa1100_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will enable/disable the clock, and
register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only enable clock and register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

NAK.  I don't think you properly understand what's going on here if you
think moving the entire open and release functions into the probe and
remove functions is the right thing to do.

Since PXA27x & PXA3xx supports dual rtc device at the same time,
user could choose use either of rtc at run time. Then clk & irq are setup
in open().

Chao,
So you shouldn't remove them into probe().

How can user to choose one RTC?

/dev/rtc0 & /dev/rtc1.

The user can choose the rtc by iteself. Is it right?

The user API, for example "date" will open the device, get the time
and then close the device.

"date" command is always using /dev/rtc. It's the alias of rtc0 or rtc1.

WHen set a alarm, it is the same routine. open->set->close.
The open routine will enable clock and register irq, close will
disable the clock and unregister irq. It means that if only open is
invoked, rtc begins to work, and after close is invoked rtc stops
working. It means that the user can not get correct time and can not
get right alarm.

On Mon, Dec 3, 2012 at 10:53 AM, Chao Xie [off-list ref] wrote:

On Mon, Dec 3, 2012 at 9:39 AM, Chao Xie [off-list ref] wrote:

quoted

On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
[off-list ref] wrote:

quoted

On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
[off-list ref] wrote:

quoted

On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:

quoted

The original sa1100_rtc_open/sa1100_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will enable/disable the clock, and
register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only enable clock and register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

NAK.  I don't think you properly understand what's going on here if you
think moving the entire open and release functions into the probe and
remove functions is the right thing to do.

Since PXA27x & PXA3xx supports dual rtc device at the same time,
user could choose use either of rtc at run time. Then clk & irq are setup
in open().

Chao,
So you shouldn't remove them into probe().

How can user to choose one RTC?
The user API, for example "date" will open the device, get the time
and then close the device.
WHen set a alarm, it is the same routine. open->set->close.
The open routine will enable clock and register irq, close will
disable the clock and unregister irq. It means that if only open is
invoked, rtc begins to work, and after close is invoked rtc stops
working. It means that the user can not get correct time and can not
get right alarm.

hi
I want to correct what i said. For the irq register/unregister i think
can be done at open/release. But for clock enable/disable, i do not
think so. If clock is disabled, as i think RTC will not work. User API
still use open->get_time->close for "date" command. It means that RTC
will not return correct date to user.

I didn't get your point. Do you mean that RTC can't work without your patch?

On Mon, Dec 3, 2012 at 1:35 PM, Haojian Zhuang [off-list ref] wrote:

On Mon, Dec 3, 2012 at 10:53 AM, Chao Xie [off-list ref] wrote:

quoted

On Mon, Dec 3, 2012 at 9:39 AM, Chao Xie [off-list ref] wrote:

quoted

On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
[off-list ref] wrote:

quoted

On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
[off-list ref] wrote:

quoted

On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:

quoted

The original sa1100_rtc_open/sa1100_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will enable/disable the clock, and
register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only enable clock and register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

NAK.  I don't think you properly understand what's going on here if you
think moving the entire open and release functions into the probe and
remove functions is the right thing to do.

Since PXA27x & PXA3xx supports dual rtc device at the same time,
user could choose use either of rtc at run time. Then clk & irq are setup
in open().

Chao,
So you shouldn't remove them into probe().

How can user to choose one RTC?
The user API, for example "date" will open the device, get the time
and then close the device.
WHen set a alarm, it is the same routine. open->set->close.
The open routine will enable clock and register irq, close will
disable the clock and unregister irq. It means that if only open is
invoked, rtc begins to work, and after close is invoked rtc stops
working. It means that the user can not get correct time and can not
get right alarm.

hi
I want to correct what i said. For the irq register/unregister i think
can be done at open/release. But for clock enable/disable, i do not
think so. If clock is disabled, as i think RTC will not work. User API
still use open->get_time->close for "date" command. It means that RTC
will not return correct date to user.

I didn't get your point. Do you mean that RTC can't work without your patch?

hi
open/release will done two things
1. enable/disable clock
2. register/unregister the irq
i checked the rtc dev code. register/unregister irq is prefered to be
done at open/release routine, and the users will keep rtc to be opened
if he want to make use of the interrupts.
the only problemis the clock. It should be on always.
If user want to get the rtc value, or set alarm. it will  follow
open->ioctrl->release. After release the clock is offed, and i do not
think rtc will has its value updated if the clock is offed.

On Mon, Dec 03, 2012 at 10:53:07AM +0800, Chao Xie wrote:

I want to correct what i said. For the irq register/unregister i think
can be done at open/release. But for clock enable/disable, i do not
think so. If clock is disabled, as i think RTC will not work. User API
still use open->get_time->close for "date" command. It means that RTC
will not return correct date to user.

"I think" is not good enough for patches like this.  Please test it.

On SA11x0 and PXA platforms, the clock for the sa1100-rtc is a dummy
clock; it has no effect.  For MMP, I don't have access to the TRMs so
that's something you're going to have to find out.

On Mon, Dec 3, 2012 at 5:48 PM, Russell King - ARM Linux
[off-list ref] wrote:

On Mon, Dec 03, 2012 at 10:53:07AM +0800, Chao Xie wrote:

quoted

I want to correct what i said. For the irq register/unregister i think
can be done at open/release. But for clock enable/disable, i do not
think so. If clock is disabled, as i think RTC will not work. User API
still use open->get_time->close for "date" command. It means that RTC
will not return correct date to user.

"I think" is not good enough for patches like this.  Please test it.

On SA11x0 and PXA platforms, the clock for the sa1100-rtc is a dummy
clock; it has no effect.  For MMP, I don't have access to the TRMs so
that's something you're going to have to find out.

I tested at pxa910 which uses rtc-sa1100 as driver.
the test procedure is simple
open->ioctl(RTC_RD_TIME)->close
With the original code, the rtc will not update, i always get the same value

remove clock disable/enable in release/open, and enable/disable clock
at probe/remove.
the rtc can update, and i can read the correct rtc value.

On Tuesday, December 4, 2012, Chao Xie wrote:

On Mon, Dec 3, 2012 at 5:48 PM, Russell King - ARM Linux
[off-list ref] wrote:

quoted

On Mon, Dec 03, 2012 at 10:53:07AM +0800, Chao Xie wrote:

quoted

I want to correct what i said. For the irq register/unregister i think
can be done at open/release. But for clock enable/disable, i do not
think so. If clock is disabled, as i think RTC will not work. User API
still use open->get_time->close for "date" command. It means that RTC
will not return correct date to user.

"I think" is not good enough for patches like this.  Please test it.

On SA11x0 and PXA platforms, the clock for the sa1100-rtc is a dummy
clock; it has no effect.  For MMP, I don't have access to the TRMs so
that's something you're going to have to find out.

I tested at pxa910 which uses rtc-sa1100 as driver.
the test procedure is simple
open->ioctl(RTC_RD_TIME)->close
With the original code, the rtc will not update, i always get the same value

remove clock disable/enable in release/open, and enable/disable clock
at probe/remove.
the rtc can update, and i can read the correct rtc value.

Yes, clock shouldn't be controlled in open/close. Otherwise, RTC can't
work since
clock is already gated. I didn't find the issue since there're two RTC
enabled in my
board. The default one impacts the test.

I think that you need send the patch that only move clock operation
into probe().

On Thu, Nov 29, 2012 at 10:21 AM, Chao Xie [off-list ref] wrote:

Don't use cpu_is_xxx() any more. If there's some different thing, we
should parse it
from DTS.

                return platform_driver_probe(&pxa_rtc_driver, pxa_rtc_probe);

        return -ENODEV;
--
1.7.4.1