[PATCH 0/3] RTC-S3C Bugfixes

[PATCH 0/3] RTC-S3C Bugfixes

[MyungJoo Ham]

1. Bugfix: incorrect debug messages
2. Bugfix: alarm entires not being reset
3. Bugfix (More like update?): allow to open rtc-s3c multiple times.

MyungJoo Ham (3):
  rtc: rtc-s3c: Correct debug messages.
  rtc: rtc-s3c: Disable alarm entries that are not chosen.
  rtc: rtc-s3c: allow multiple open / allow no-ioctl-open'ed rtc to
    have irq.

 drivers/rtc/rtc-s3c.c |   77 +++++++++++++++++-------------------------------
 1 files changed, 27 insertions(+), 50 deletions(-)

-- 
1.7.4.1

[PATCH 0/3] RTC-S3C Bugfixes

[MyungJoo Ham]

1. Bugfix: incorrect debug messages
2. Bugfix: alarm entires not being reset
3. Bugfix (More like update?): allow to open rtc-s3c multiple times.

MyungJoo Ham (3):
  rtc: rtc-s3c: Correct debug messages.
  rtc: rtc-s3c: Disable alarm entries that are not chosen.
  rtc: rtc-s3c: allow multiple open / allow no-ioctl-open'ed rtc to
    have irq.

 drivers/rtc/rtc-s3c.c |   77 +++++++++++++++++-------------------------------
 1 files changed, 27 insertions(+), 50 deletions(-)

-- 
1.7.4.1

[PATCH 1/3] rtc: rtc-s3c: Correct debug messages.

[MyungJoo Ham]

RTC-S3C used to print out debug messages incorrectly. This patch
corrects incorrect outputs. (undecoded bcd numbers, incorrectly decoded
register values) This patch affects the pr-debug messages only.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s3c.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 53c99b1..3c8dfa7 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -152,10 +152,6 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 		goto retry_get_time;
 	}
 
-	pr_debug("read time %04d.%02d.%02d %02d:%02d:%02d\n",
-		 1900 + rtc_tm->tm_year, rtc_tm->tm_mon, rtc_tm->tm_mday,
-		 rtc_tm->tm_hour, rtc_tm->tm_min, rtc_tm->tm_sec);
-
 	rtc_tm->tm_sec = bcd2bin(rtc_tm->tm_sec);
 	rtc_tm->tm_min = bcd2bin(rtc_tm->tm_min);
 	rtc_tm->tm_hour = bcd2bin(rtc_tm->tm_hour);
@@ -164,6 +160,11 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 	rtc_tm->tm_year = bcd2bin(rtc_tm->tm_year);
 
 	rtc_tm->tm_year += 100;
+
+	pr_debug("read time %04d.%02d.%02d %02d:%02d:%02d\n",
+		 1900 + rtc_tm->tm_year, rtc_tm->tm_mon, rtc_tm->tm_mday,
+		 rtc_tm->tm_hour, rtc_tm->tm_min, rtc_tm->tm_sec);
+
 	rtc_tm->tm_mon -= 1;
 
 	clk_disable(rtc_clk);
@@ -269,10 +270,9 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	clk_enable(rtc_clk);
 	pr_debug("s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
 		 alrm->enabled,
-		 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
+		 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
 		 tm->tm_hour, tm->tm_min, tm->tm_sec);
 
-
 	alrm_en = readb(base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
 	writeb(0x00, base + S3C2410_RTCALM);
 
@@ -359,6 +359,7 @@ static void s3c_rtc_release(struct device *dev)
 	free_irq(s3c_rtc_tickno, rtc_dev);
 }
 
+
 static const struct rtc_class_ops s3c_rtcops = {
 	.open		= s3c_rtc_open,
 	.release	= s3c_rtc_release,
-- 
1.7.4.1

[PATCH 1/3] rtc: rtc-s3c: Correct debug messages.

[MyungJoo Ham]

RTC-S3C used to print out debug messages incorrectly. This patch
corrects incorrect outputs. (undecoded bcd numbers, incorrectly decoded
register values) This patch affects the pr-debug messages only.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s3c.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 53c99b1..3c8dfa7 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -152,10 +152,6 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 		goto retry_get_time;
 	}
 
-	pr_debug("read time %04d.%02d.%02d %02d:%02d:%02d\n",
-		 1900 + rtc_tm->tm_year, rtc_tm->tm_mon, rtc_tm->tm_mday,
-		 rtc_tm->tm_hour, rtc_tm->tm_min, rtc_tm->tm_sec);
-
 	rtc_tm->tm_sec = bcd2bin(rtc_tm->tm_sec);
 	rtc_tm->tm_min = bcd2bin(rtc_tm->tm_min);
 	rtc_tm->tm_hour = bcd2bin(rtc_tm->tm_hour);
@@ -164,6 +160,11 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 	rtc_tm->tm_year = bcd2bin(rtc_tm->tm_year);
 
 	rtc_tm->tm_year += 100;
+
+	pr_debug("read time %04d.%02d.%02d %02d:%02d:%02d\n",
+		 1900 + rtc_tm->tm_year, rtc_tm->tm_mon, rtc_tm->tm_mday,
+		 rtc_tm->tm_hour, rtc_tm->tm_min, rtc_tm->tm_sec);
+
 	rtc_tm->tm_mon -= 1;
 
 	clk_disable(rtc_clk);
@@ -269,10 +270,9 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	clk_enable(rtc_clk);
 	pr_debug("s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
 		 alrm->enabled,
-		 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
+		 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
 		 tm->tm_hour, tm->tm_min, tm->tm_sec);
 
-
 	alrm_en = readb(base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
 	writeb(0x00, base + S3C2410_RTCALM);
 
@@ -359,6 +359,7 @@ static void s3c_rtc_release(struct device *dev)
 	free_irq(s3c_rtc_tickno, rtc_dev);
 }
 
+
 static const struct rtc_class_ops s3c_rtcops = {
 	.open		= s3c_rtc_open,
 	.release	= s3c_rtc_release,
-- 
1.7.4.1

[PATCH 2/3] rtc: rtc-s3c: Disable alarm entries that are not chosen.

[MyungJoo Ham]

When rtc_setalarm is called, the entries that are not chosen (entries
without valid time values) should be disabled. However, in the previous
rtc-s3c driver, they are not explicitly disabled (did not changed). This
patch allows to disable such entries even if they were previously used.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s3c.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 3c8dfa7..be85ea5 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -273,7 +273,8 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 		 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
 		 tm->tm_hour, tm->tm_min, tm->tm_sec);
 
-	alrm_en = readb(base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
+	/* Disable entires that are not chosen by alrm */
+	alrm_en = S3C2410_RTCALM_ALMEN;
 	writeb(0x00, base + S3C2410_RTCALM);
 
 	if (tm->tm_sec < 60 && tm->tm_sec >= 0) {
-- 
1.7.4.1

[PATCH 2/3] rtc: rtc-s3c: Disable alarm entries that are not chosen.

[MyungJoo Ham]

When rtc_setalarm is called, the entries that are not chosen (entries
without valid time values) should be disabled. However, in the previous
rtc-s3c driver, they are not explicitly disabled (did not changed). This
patch allows to disable such entries even if they were previously used.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s3c.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 3c8dfa7..be85ea5 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -273,7 +273,8 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 		 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
 		 tm->tm_hour, tm->tm_min, tm->tm_sec);
 
-	alrm_en = readb(base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
+	/* Disable entires that are not chosen by alrm */
+	alrm_en = S3C2410_RTCALM_ALMEN;
 	writeb(0x00, base + S3C2410_RTCALM);
 
 	if (tm->tm_sec < 60 && tm->tm_sec >= 0) {
-- 
1.7.4.1

[PATCH 3/3] rtc: rtc-s3c: allow multiple open / allow no-ioctl-open'ed rtc to have irq.

[MyungJoo Ham]

The previous rtc-s3c had two issues related with its IRQ.
1. Users cannot open rtc multiple times because an open operation calls
request_irq on the same IRQ. (e.g., two user processes wants to open and
read RTC time from rtc-s3c at the same time)
2. If alarm is set and no one has the rtc opened with filesystem
(either the alarm is set by kernel/boot-loader or user set an alarm
and closed rtc dev file), the pending bit is not cleared and no further
interrupt is invoked. When the alarm is used by the system itself such
as a resume from suspend-to-RAM or other Low-power modes/idle, this is a
critical issue.

This patch mitigates these issue by calling request_irq at probe and
free_irq at remove.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s3c.c |   63 ++++++++++++++----------------------------------
 1 files changed, 19 insertions(+), 44 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index be85ea5..8d2807f 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -320,50 +320,7 @@ static int s3c_rtc_proc(struct device *dev, struct seq_file *seq)
 	return 0;
 }
 
-static int s3c_rtc_open(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct rtc_device *rtc_dev = platform_get_drvdata(pdev);
-	int ret;
-
-	ret = request_irq(s3c_rtc_alarmno, s3c_rtc_alarmirq,
-			  IRQF_DISABLED,  "s3c2410-rtc alarm", rtc_dev);
-
-	if (ret) {
-		dev_err(dev, "IRQ%d error %d\n", s3c_rtc_alarmno, ret);
-		return ret;
-	}
-
-	ret = request_irq(s3c_rtc_tickno, s3c_rtc_tickirq,
-			  IRQF_DISABLED,  "s3c2410-rtc tick", rtc_dev);
-
-	if (ret) {
-		dev_err(dev, "IRQ%d error %d\n", s3c_rtc_tickno, ret);
-		goto tick_err;
-	}
-
-	return ret;
-
- tick_err:
-	free_irq(s3c_rtc_alarmno, rtc_dev);
-	return ret;
-}
-
-static void s3c_rtc_release(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct rtc_device *rtc_dev = platform_get_drvdata(pdev);
-
-	/* do not clear AIE here, it may be needed for wake */
-
-	free_irq(s3c_rtc_alarmno, rtc_dev);
-	free_irq(s3c_rtc_tickno, rtc_dev);
-}
-
-
 static const struct rtc_class_ops s3c_rtcops = {
-	.open		= s3c_rtc_open,
-	.release	= s3c_rtc_release,
 	.read_time	= s3c_rtc_gettime,
 	.set_time	= s3c_rtc_settime,
 	.read_alarm	= s3c_rtc_getalarm,
@@ -427,6 +384,9 @@ static int __devexit s3c_rtc_remove(struct platform_device *dev)
 {
 	struct rtc_device *rtc = platform_get_drvdata(dev);
 
+	free_irq(s3c_rtc_alarmno, rtc);
+	free_irq(s3c_rtc_tickno, rtc);
+
 	platform_set_drvdata(dev, NULL);
 	rtc_device_unregister(rtc);
 
@@ -553,8 +513,23 @@ static int __devinit s3c_rtc_probe(struct platform_device *pdev)
 
 	clk_disable(rtc_clk);
 
-	return 0;
+	ret = request_irq(s3c_rtc_alarmno, s3c_rtc_alarmirq,
+			  IRQF_DISABLED,  "s3c2410-rtc alarm", rtc);
 
+	if (ret) {
+		dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_alarmno, ret);
+		return ret;
+	}
+
+	ret = request_irq(s3c_rtc_tickno, s3c_rtc_tickirq,
+			  IRQF_DISABLED,  "s3c2410-rtc tick", rtc);
+
+	if (ret) {
+		dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_tickno, ret);
+		free_irq(s3c_rtc_alarmno, rtc);
+	}
+
+	return ret;
  err_nortc:
 	s3c_rtc_enable(pdev, 0);
 	clk_disable(rtc_clk);
-- 
1.7.4.1

[PATCH 3/3] rtc: rtc-s3c: allow multiple open / allow no-ioctl-open'ed rtc to have irq.

[MyungJoo Ham]

The previous rtc-s3c had two issues related with its IRQ.
1. Users cannot open rtc multiple times because an open operation calls
request_irq on the same IRQ. (e.g., two user processes wants to open and
read RTC time from rtc-s3c at the same time)
2. If alarm is set and no one has the rtc opened with filesystem
(either the alarm is set by kernel/boot-loader or user set an alarm
and closed rtc dev file), the pending bit is not cleared and no further
interrupt is invoked. When the alarm is used by the system itself such
as a resume from suspend-to-RAM or other Low-power modes/idle, this is a
critical issue.

This patch mitigates these issue by calling request_irq at probe and
free_irq at remove.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s3c.c |   63 ++++++++++++++----------------------------------
 1 files changed, 19 insertions(+), 44 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index be85ea5..8d2807f 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -320,50 +320,7 @@ static int s3c_rtc_proc(struct device *dev, struct seq_file *seq)
 	return 0;
 }
 
-static int s3c_rtc_open(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct rtc_device *rtc_dev = platform_get_drvdata(pdev);
-	int ret;
-
-	ret = request_irq(s3c_rtc_alarmno, s3c_rtc_alarmirq,
-			  IRQF_DISABLED,  "s3c2410-rtc alarm", rtc_dev);
-
-	if (ret) {
-		dev_err(dev, "IRQ%d error %d\n", s3c_rtc_alarmno, ret);
-		return ret;
-	}
-
-	ret = request_irq(s3c_rtc_tickno, s3c_rtc_tickirq,
-			  IRQF_DISABLED,  "s3c2410-rtc tick", rtc_dev);
-
-	if (ret) {
-		dev_err(dev, "IRQ%d error %d\n", s3c_rtc_tickno, ret);
-		goto tick_err;
-	}
-
-	return ret;
-
- tick_err:
-	free_irq(s3c_rtc_alarmno, rtc_dev);
-	return ret;
-}
-
-static void s3c_rtc_release(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct rtc_device *rtc_dev = platform_get_drvdata(pdev);
-
-	/* do not clear AIE here, it may be needed for wake */
-
-	free_irq(s3c_rtc_alarmno, rtc_dev);
-	free_irq(s3c_rtc_tickno, rtc_dev);
-}
-
-
 static const struct rtc_class_ops s3c_rtcops = {
-	.open		= s3c_rtc_open,
-	.release	= s3c_rtc_release,
 	.read_time	= s3c_rtc_gettime,
 	.set_time	= s3c_rtc_settime,
 	.read_alarm	= s3c_rtc_getalarm,
@@ -427,6 +384,9 @@ static int __devexit s3c_rtc_remove(struct platform_device *dev)
 {
 	struct rtc_device *rtc = platform_get_drvdata(dev);
 
+	free_irq(s3c_rtc_alarmno, rtc);
+	free_irq(s3c_rtc_tickno, rtc);
+
 	platform_set_drvdata(dev, NULL);
 	rtc_device_unregister(rtc);
 
@@ -553,8 +513,23 @@ static int __devinit s3c_rtc_probe(struct platform_device *pdev)
 
 	clk_disable(rtc_clk);
 
-	return 0;
+	ret = request_irq(s3c_rtc_alarmno, s3c_rtc_alarmirq,
+			  IRQF_DISABLED,  "s3c2410-rtc alarm", rtc);
 
+	if (ret) {
+		dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_alarmno, ret);
+		return ret;
+	}
+
+	ret = request_irq(s3c_rtc_tickno, s3c_rtc_tickirq,
+			  IRQF_DISABLED,  "s3c2410-rtc tick", rtc);
+
+	if (ret) {
+		dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_tickno, ret);
+		free_irq(s3c_rtc_alarmno, rtc);
+	}
+
+	return ret;
  err_nortc:
 	s3c_rtc_enable(pdev, 0);
 	clk_disable(rtc_clk);
-- 
1.7.4.1

RE: [rtc-linux] [PATCH 2/3] rtc: rtc-s3c: Disable alarm entries that are not chosen.

[Changhwan Youn]

MyungJoo Ham wrote:
> When rtc_setalarm is called, the entries that are not chosen (entries
> without valid time values) should be disabled. However, in the previous
> rtc-s3c driver, they are not explicitly disabled (did not changed). This
> patch allows to disable such entries even if they were previously used.

This has already been implemented by current code.
The original code you modified only leaves alarm enable bit.
You may misunderstand the code.

Regards,
Changhwan
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/rtc/rtc-s3c.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
> index 3c8dfa7..be85ea5 100644
> --- a/drivers/rtc/rtc-s3c.c
> +++ b/drivers/rtc/rtc-s3c.c
> @@ -273,7 +273,8 @@ static int s3c_rtc_setalarm(struct device *dev, struct
> rtc_wkalrm *alrm)
>  		 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
>  		 tm->tm_hour, tm->tm_min, tm->tm_sec);
> 
> -	alrm_en = readb(base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
> +	/* Disable entires that are not chosen by alrm */
> +	alrm_en = S3C2410_RTCALM_ALMEN;
>  	writeb(0x00, base + S3C2410_RTCALM);
> 
>  	if (tm->tm_sec < 60 && tm->tm_sec >= 0) {
> --
> 1.7.4.1
> 
> --
> You received this message because you are subscribed to "rtc-linux".
> Membership options at http://groups.google.com/group/rtc-linux .
> Please read http://groups.google.com/group/rtc-linux/web/checklist
> before submitting a driver.

[rtc-linux] [PATCH 2/3] rtc: rtc-s3c: Disable alarm entries that are not chosen.

[Changhwan Youn]

MyungJoo Ham wrote:
> When rtc_setalarm is called, the entries that are not chosen (entries
> without valid time values) should be disabled. However, in the previous
> rtc-s3c driver, they are not explicitly disabled (did not changed). This
> patch allows to disable such entries even if they were previously used.

This has already been implemented by current code.
The original code you modified only leaves alarm enable bit.
You may misunderstand the code.

Regards,
Changhwan
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/rtc/rtc-s3c.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
> index 3c8dfa7..be85ea5 100644
> --- a/drivers/rtc/rtc-s3c.c
> +++ b/drivers/rtc/rtc-s3c.c
> @@ -273,7 +273,8 @@ static int s3c_rtc_setalarm(struct device *dev, struct
> rtc_wkalrm *alrm)
>  		 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
>  		 tm->tm_hour, tm->tm_min, tm->tm_sec);
> 
> -	alrm_en = readb(base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
> +	/* Disable entires that are not chosen by alrm */
> +	alrm_en = S3C2410_RTCALM_ALMEN;
>  	writeb(0x00, base + S3C2410_RTCALM);
> 
>  	if (tm->tm_sec < 60 && tm->tm_sec >= 0) {
> --
> 1.7.4.1
> 
> --
> You received this message because you are subscribed to "rtc-linux".
> Membership options at http://groups.google.com/group/rtc-linux .
> Please read http://groups.google.com/group/rtc-linux/web/checklist
> before submitting a driver.

RE: [PATCH 3/3] rtc: rtc-s3c: allow multiple open / allow no-ioctl-open'ed rtc to have irq.

[Changhwan Youn]

AbwBwAGUAbgAgAC8AIAB
	hAGwAbABvAHcAIABuAG8ALQBpAG8AYwB0AGwALQBvAHAAZQBuACcAZQBkACAAcgB0AGMAIAB0AG8AIABoAGEAdgBlACAAaQByAHEALgA=
x-cr-puzzleid: {71A23705-5400-4E21-9C23-2B1E96E33A68}

MyungJoo Ham wrote:
> The previous rtc-s3c had two issues related with its IRQ.
> 1. Users cannot open rtc multiple times because an open operation calls
> request_irq on the same IRQ. (e.g., two user processes wants to open and
> read RTC time from rtc-s3c at the same time)
> 2. If alarm is set and no one has the rtc opened with filesystem
> (either the alarm is set by kernel/boot-loader or user set an alarm
> and closed rtc dev file), the pending bit is not cleared and no further
> interrupt is invoked. When the alarm is used by the system itself such
> as a resume from suspend-to-RAM or other Low-power modes/idle, this is a
> critical issue.
> 
> This patch mitigates these issue by calling request_irq at probe and
> free_irq at remove.
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/rtc/rtc-s3c.c |   63
++++++++++++++-------------------------------
> ---
>  1 files changed, 19 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
> index be85ea5..8d2807f 100644
> --- a/drivers/rtc/rtc-s3c.c
> +++ b/drivers/rtc/rtc-s3c.c
> @@ -320,50 +320,7 @@ static int s3c_rtc_proc(struct device *dev, struct
> seq_file *seq)
>  	return 0;
>  }
> 
> -static int s3c_rtc_open(struct device *dev)
> -{
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct rtc_device *rtc_dev = platform_get_drvdata(pdev);
> -	int ret;
> -
> -	ret = request_irq(s3c_rtc_alarmno, s3c_rtc_alarmirq,
> -			  IRQF_DISABLED,  "s3c2410-rtc alarm", rtc_dev);
> -
> -	if (ret) {
> -		dev_err(dev, "IRQ%d error %d\n", s3c_rtc_alarmno, ret);
> -		return ret;
> -	}
> -
> -	ret = request_irq(s3c_rtc_tickno, s3c_rtc_tickirq,
> -			  IRQF_DISABLED,  "s3c2410-rtc tick", rtc_dev);
> -
> -	if (ret) {
> -		dev_err(dev, "IRQ%d error %d\n", s3c_rtc_tickno, ret);
> -		goto tick_err;
> -	}
> -
> -	return ret;
> -
> - tick_err:
> -	free_irq(s3c_rtc_alarmno, rtc_dev);
> -	return ret;
> -}
> -
> -static void s3c_rtc_release(struct device *dev)
> -{
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct rtc_device *rtc_dev = platform_get_drvdata(pdev);
> -
> -	/* do not clear AIE here, it may be needed for wake */
> -
> -	free_irq(s3c_rtc_alarmno, rtc_dev);
> -	free_irq(s3c_rtc_tickno, rtc_dev);
> -}
> -
> -
>  static const struct rtc_class_ops s3c_rtcops = {
> -	.open		= s3c_rtc_open,
> -	.release	= s3c_rtc_release,
>  	.read_time	= s3c_rtc_gettime,
>  	.set_time	= s3c_rtc_settime,
>  	.read_alarm	= s3c_rtc_getalarm,
> @@ -427,6 +384,9 @@ static int __devexit s3c_rtc_remove(struct
> platform_device *dev)
>  {
>  	struct rtc_device *rtc = platform_get_drvdata(dev);
> 
> +	free_irq(s3c_rtc_alarmno, rtc);
> +	free_irq(s3c_rtc_tickno, rtc);
> +
>  	platform_set_drvdata(dev, NULL);
>  	rtc_device_unregister(rtc);
> 
> @@ -553,8 +513,23 @@ static int __devinit s3c_rtc_probe(struct
> platform_device *pdev)
> 
>  	clk_disable(rtc_clk);
> 
> -	return 0;
> +	ret = request_irq(s3c_rtc_alarmno, s3c_rtc_alarmirq,
> +			  IRQF_DISABLED,  "s3c2410-rtc alarm", rtc);
> 
> +	if (ret) {
> +		dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_alarmno,
> ret);
> +		return ret;
> +	}
> +
> +	ret = request_irq(s3c_rtc_tickno, s3c_rtc_tickirq,
> +			  IRQF_DISABLED,  "s3c2410-rtc tick", rtc);
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_tickno,
ret);
> +		free_irq(s3c_rtc_alarmno, rtc);
> +	}
> +
> +	return ret;

You need proper error handlings.

Regards,
Changhwan

>   err_nortc:
>  	s3c_rtc_enable(pdev, 0);
>  	clk_disable(rtc_clk);
> --
> 1.7.4.1
> 
> --
> You received this message because you are subscribed to "rtc-linux".
> Membership options at http://groups.google.com/group/rtc-linux .
> Please read http://groups.google.com/group/rtc-linux/web/checklist
> before submitting a driver.

[PATCH 3/3] rtc: rtc-s3c: allow multiple open / allow no-ioctl-open'ed rtc to have irq.

[Changhwan Youn]

AbwBwAGUAbgAgAC8AIAB
	hAGwAbABvAHcAIABuAG8ALQBpAG8AYwB0AGwALQBvAHAAZQBuACcAZQBkACAAcgB0AGMAIAB0AG8AIABoAGEAdgBlACAAaQByAHEALgA=
x-cr-puzzleid: {71A23705-5400-4E21-9C23-2B1E96E33A68}

MyungJoo Ham wrote:
> The previous rtc-s3c had two issues related with its IRQ.
> 1. Users cannot open rtc multiple times because an open operation calls
> request_irq on the same IRQ. (e.g., two user processes wants to open and
> read RTC time from rtc-s3c at the same time)
> 2. If alarm is set and no one has the rtc opened with filesystem
> (either the alarm is set by kernel/boot-loader or user set an alarm
> and closed rtc dev file), the pending bit is not cleared and no further
> interrupt is invoked. When the alarm is used by the system itself such
> as a resume from suspend-to-RAM or other Low-power modes/idle, this is a
> critical issue.
> 
> This patch mitigates these issue by calling request_irq at probe and
> free_irq at remove.
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/rtc/rtc-s3c.c |   63
++++++++++++++-------------------------------
> ---
>  1 files changed, 19 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
> index be85ea5..8d2807f 100644
> --- a/drivers/rtc/rtc-s3c.c
> +++ b/drivers/rtc/rtc-s3c.c
> @@ -320,50 +320,7 @@ static int s3c_rtc_proc(struct device *dev, struct
> seq_file *seq)
>  	return 0;
>  }
> 
> -static int s3c_rtc_open(struct device *dev)
> -{
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct rtc_device *rtc_dev = platform_get_drvdata(pdev);
> -	int ret;
> -
> -	ret = request_irq(s3c_rtc_alarmno, s3c_rtc_alarmirq,
> -			  IRQF_DISABLED,  "s3c2410-rtc alarm", rtc_dev);
> -
> -	if (ret) {
> -		dev_err(dev, "IRQ%d error %d\n", s3c_rtc_alarmno, ret);
> -		return ret;
> -	}
> -
> -	ret = request_irq(s3c_rtc_tickno, s3c_rtc_tickirq,
> -			  IRQF_DISABLED,  "s3c2410-rtc tick", rtc_dev);
> -
> -	if (ret) {
> -		dev_err(dev, "IRQ%d error %d\n", s3c_rtc_tickno, ret);
> -		goto tick_err;
> -	}
> -
> -	return ret;
> -
> - tick_err:
> -	free_irq(s3c_rtc_alarmno, rtc_dev);
> -	return ret;
> -}
> -
> -static void s3c_rtc_release(struct device *dev)
> -{
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct rtc_device *rtc_dev = platform_get_drvdata(pdev);
> -
> -	/* do not clear AIE here, it may be needed for wake */
> -
> -	free_irq(s3c_rtc_alarmno, rtc_dev);
> -	free_irq(s3c_rtc_tickno, rtc_dev);
> -}
> -
> -
>  static const struct rtc_class_ops s3c_rtcops = {
> -	.open		= s3c_rtc_open,
> -	.release	= s3c_rtc_release,
>  	.read_time	= s3c_rtc_gettime,
>  	.set_time	= s3c_rtc_settime,
>  	.read_alarm	= s3c_rtc_getalarm,
> @@ -427,6 +384,9 @@ static int __devexit s3c_rtc_remove(struct
> platform_device *dev)
>  {
>  	struct rtc_device *rtc = platform_get_drvdata(dev);
> 
> +	free_irq(s3c_rtc_alarmno, rtc);
> +	free_irq(s3c_rtc_tickno, rtc);
> +
>  	platform_set_drvdata(dev, NULL);
>  	rtc_device_unregister(rtc);
> 
> @@ -553,8 +513,23 @@ static int __devinit s3c_rtc_probe(struct
> platform_device *pdev)
> 
>  	clk_disable(rtc_clk);
> 
> -	return 0;
> +	ret = request_irq(s3c_rtc_alarmno, s3c_rtc_alarmirq,
> +			  IRQF_DISABLED,  "s3c2410-rtc alarm", rtc);
> 
> +	if (ret) {
> +		dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_alarmno,
> ret);
> +		return ret;
> +	}
> +
> +	ret = request_irq(s3c_rtc_tickno, s3c_rtc_tickirq,
> +			  IRQF_DISABLED,  "s3c2410-rtc tick", rtc);
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_tickno,
ret);
> +		free_irq(s3c_rtc_alarmno, rtc);
> +	}
> +
> +	return ret;

You need proper error handlings.

Regards,
Changhwan

>   err_nortc:
>  	s3c_rtc_enable(pdev, 0);
>  	clk_disable(rtc_clk);
> --
> 1.7.4.1
> 
> --
> You received this message because you are subscribed to "rtc-linux".
> Membership options at http://groups.google.com/group/rtc-linux .
> Please read http://groups.google.com/group/rtc-linux/web/checklist
> before submitting a driver.

Re: [rtc-linux] [PATCH 2/3] rtc: rtc-s3c: Disable alarm entries that are not chosen.

[MyungJoo Ham]

On Mon, Jul 4, 2011 at 11:39 AM, Changhwan Youn <chaos.youn@samsung.com> wrote:
> MyungJoo Ham wrote:
>> When rtc_setalarm is called, the entries that are not chosen (entries
>> without valid time values) should be disabled. However, in the previous
>> rtc-s3c driver, they are not explicitly disabled (did not changed). This
>> patch allows to disable such entries even if they were previously used.
>
> This has already been implemented by current code.
> The original code you modified only leaves alarm enable bit.
> You may misunderstand the code.

Ah.. sorry. I misunderstood when I was writing the commit message.

It is not about the enable bits of each time entry. It is about the enable bit.

However, as rtc->ops->set_alarm is not called when alarm->enabled is
false, the patch becomes meaningless.

Please never mind this patch.

>
> Regards,
> Changhwan
>>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/rtc/rtc-s3c.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
>> index 3c8dfa7..be85ea5 100644
>> --- a/drivers/rtc/rtc-s3c.c
>> +++ b/drivers/rtc/rtc-s3c.c
>> @@ -273,7 +273,8 @@ static int s3c_rtc_setalarm(struct device *dev, struct
>> rtc_wkalrm *alrm)
>>                1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
>>                tm->tm_hour, tm->tm_min, tm->tm_sec);
>>
>> -     alrm_en = readb(base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
>> +     /* Disable entires that are not chosen by alrm */
>> +     alrm_en = S3C2410_RTCALM_ALMEN;
>>       writeb(0x00, base + S3C2410_RTCALM);
>>
>>       if (tm->tm_sec < 60 && tm->tm_sec >= 0) {
>> --
>> 1.7.4.1
>>
>> --
>> You received this message because you are subscribed to "rtc-linux".
>> Membership options at http://groups.google.com/group/rtc-linux .
>> Please read http://groups.google.com/group/rtc-linux/web/checklist
>> before submitting a driver.
>
>



-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

[rtc-linux] [PATCH 2/3] rtc: rtc-s3c: Disable alarm entries that are not chosen.

[MyungJoo Ham]

On Mon, Jul 4, 2011 at 11:39 AM, Changhwan Youn <chaos.youn@samsung.com> wrote:
> MyungJoo Ham wrote:
>> When rtc_setalarm is called, the entries that are not chosen (entries
>> without valid time values) should be disabled. However, in the previous
>> rtc-s3c driver, they are not explicitly disabled (did not changed). This
>> patch allows to disable such entries even if they were previously used.
>
> This has already been implemented by current code.
> The original code you modified only leaves alarm enable bit.
> You may misunderstand the code.

Ah.. sorry. I misunderstood when I was writing the commit message.

It is not about the enable bits of each time entry. It is about the enable bit.

However, as rtc->ops->set_alarm is not called when alarm->enabled is
false, the patch becomes meaningless.

Please never mind this patch.

>
> Regards,
> Changhwan
>>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> ?drivers/rtc/rtc-s3c.c | ? ?3 ++-
>> ?1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
>> index 3c8dfa7..be85ea5 100644
>> --- a/drivers/rtc/rtc-s3c.c
>> +++ b/drivers/rtc/rtc-s3c.c
>> @@ -273,7 +273,8 @@ static int s3c_rtc_setalarm(struct device *dev, struct
>> rtc_wkalrm *alrm)
>> ? ? ? ? ? ? ? ?1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
>> ? ? ? ? ? ? ? ?tm->tm_hour, tm->tm_min, tm->tm_sec);
>>
>> - ? ? alrm_en = readb(base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
>> + ? ? /* Disable entires that are not chosen by alrm */
>> + ? ? alrm_en = S3C2410_RTCALM_ALMEN;
>> ? ? ? writeb(0x00, base + S3C2410_RTCALM);
>>
>> ? ? ? if (tm->tm_sec < 60 && tm->tm_sec >= 0) {
>> --
>> 1.7.4.1
>>
>> --
>> You received this message because you are subscribed to "rtc-linux".
>> Membership options at http://groups.google.com/group/rtc-linux .
>> Please read http://groups.google.com/group/rtc-linux/web/checklist
>> before submitting a driver.
>
>



-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

Re: [PATCH 3/3] rtc: rtc-s3c: allow multiple open / allow no-ioctl-open'ed rtc to have irq.

[MyungJoo Ham]

On Mon, Jul 4, 2011 at 12:40 PM, Changhwan Youn <chaos.youn@samsung.com> wrote:
> AbwBwAGUAbgAgAC8AIAB
>        hAGwAbABvAHcAIABuAG8ALQBpAG8AYwB0AGwALQBvAHAAZQBuACcAZQBkACAAcgB0AGMAIAB0AG8AIABoAGEAdgBlACAAaQByAHEALgA=
> x-cr-puzzleid: {71A23705-5400-4E21-9C23-2B1E96E33A68}
>
> MyungJoo Ham wrote:
>> The previous rtc-s3c had two issues related with its IRQ.
>> 1. Users cannot open rtc multiple times because an open operation calls
>> request_irq on the same IRQ. (e.g., two user processes wants to open and
>> read RTC time from rtc-s3c at the same time)
>> 2. If alarm is set and no one has the rtc opened with filesystem
>> (either the alarm is set by kernel/boot-loader or user set an alarm
>> and closed rtc dev file), the pending bit is not cleared and no further
>> interrupt is invoked. When the alarm is used by the system itself such
>> as a resume from suspend-to-RAM or other Low-power modes/idle, this is a
>> critical issue.
>>
>> This patch mitigates these issue by calling request_irq at probe and
>> free_irq at remove.
>>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
[]
>>
>> -     return 0;
>> +     ret = request_irq(s3c_rtc_alarmno, s3c_rtc_alarmirq,
>> +                       IRQF_DISABLED,  "s3c2410-rtc alarm", rtc);
>>
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_alarmno,
>> ret);
>> +             return ret;
>> +     }
>> +
>> +     ret = request_irq(s3c_rtc_tickno, s3c_rtc_tickirq,
>> +                       IRQF_DISABLED,  "s3c2410-rtc tick", rtc);
>> +
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_tickno,
> ret);
>> +             free_irq(s3c_rtc_alarmno, rtc);
>> +     }
>> +
>> +     return ret;
>
> You need proper error handlings.
>
> Regards,
> Changhwan

Ah.. ok, I'll let them go through the error handling code like other errors do.

Thanks.


- MyungJoo

>
>>   err_nortc:
>>       s3c_rtc_enable(pdev, 0);
>>       clk_disable(rtc_clk);
>> --
>> 1.7.4.1
>>
>> --
>> You received this message because you are subscribed to "rtc-linux".
>> Membership options at http://groups.google.com/group/rtc-linux .
>> Please read http://groups.google.com/group/rtc-linux/web/checklist
>> before submitting a driver.
>
>



-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

[PATCH 3/3] rtc: rtc-s3c: allow multiple open / allow no-ioctl-open'ed rtc to have irq.

[MyungJoo Ham]

On Mon, Jul 4, 2011 at 12:40 PM, Changhwan Youn <chaos.youn@samsung.com> wrote:
> AbwBwAGUAbgAgAC8AIAB
> ? ? ? ?hAGwAbABvAHcAIABuAG8ALQBpAG8AYwB0AGwALQBvAHAAZQBuACcAZQBkACAAcgB0AGMAIAB0AG8AIABoAGEAdgBlACAAaQByAHEALgA=
> x-cr-puzzleid: {71A23705-5400-4E21-9C23-2B1E96E33A68}
>
> MyungJoo Ham wrote:
>> The previous rtc-s3c had two issues related with its IRQ.
>> 1. Users cannot open rtc multiple times because an open operation calls
>> request_irq on the same IRQ. (e.g., two user processes wants to open and
>> read RTC time from rtc-s3c at the same time)
>> 2. If alarm is set and no one has the rtc opened with filesystem
>> (either the alarm is set by kernel/boot-loader or user set an alarm
>> and closed rtc dev file), the pending bit is not cleared and no further
>> interrupt is invoked. When the alarm is used by the system itself such
>> as a resume from suspend-to-RAM or other Low-power modes/idle, this is a
>> critical issue.
>>
>> This patch mitigates these issue by calling request_irq at probe and
>> free_irq at remove.
>>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
[]
>>
>> - ? ? return 0;
>> + ? ? ret = request_irq(s3c_rtc_alarmno, s3c_rtc_alarmirq,
>> + ? ? ? ? ? ? ? ? ? ? ? IRQF_DISABLED, ?"s3c2410-rtc alarm", rtc);
>>
>> + ? ? if (ret) {
>> + ? ? ? ? ? ? dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_alarmno,
>> ret);
>> + ? ? ? ? ? ? return ret;
>> + ? ? }
>> +
>> + ? ? ret = request_irq(s3c_rtc_tickno, s3c_rtc_tickirq,
>> + ? ? ? ? ? ? ? ? ? ? ? IRQF_DISABLED, ?"s3c2410-rtc tick", rtc);
>> +
>> + ? ? if (ret) {
>> + ? ? ? ? ? ? dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_tickno,
> ret);
>> + ? ? ? ? ? ? free_irq(s3c_rtc_alarmno, rtc);
>> + ? ? }
>> +
>> + ? ? return ret;
>
> You need proper error handlings.
>
> Regards,
> Changhwan

Ah.. ok, I'll let them go through the error handling code like other errors do.

Thanks.


- MyungJoo

>
>> ? err_nortc:
>> ? ? ? s3c_rtc_enable(pdev, 0);
>> ? ? ? clk_disable(rtc_clk);
>> --
>> 1.7.4.1
>>
>> --
>> You received this message because you are subscribed to "rtc-linux".
>> Membership options at http://groups.google.com/group/rtc-linux .
>> Please read http://groups.google.com/group/rtc-linux/web/checklist
>> before submitting a driver.
>
>



-- 
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858