2007-11-23 10:08:48

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 0/7] Blackfin On-Chip RTC driver updates

Mainly updates from Mike.


2007-11-23 10:09:03

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 2/7] Blackfin RTC driver: we pass in a (struct device*) to the irq handler, not a (struct platform_device*), so fix the irq handler

From: Mike Frysinger <[email protected]>

Signed-off-by: Mike Frysinger <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
drivers/rtc/rtc-bfin.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-bfin.c b/drivers/rtc/rtc-bfin.c
index 303ed6e..2a801f2 100644
--- a/drivers/rtc/rtc-bfin.c
+++ b/drivers/rtc/rtc-bfin.c
@@ -144,8 +144,8 @@ static void rtc_bfin_reset(struct bfin_rtc *rtc)

static irqreturn_t bfin_rtc_interrupt(int irq, void *dev_id)
{
- struct platform_device *pdev = to_platform_device(dev_id);
- struct bfin_rtc *rtc = platform_get_drvdata(pdev);
+ struct device *dev = dev_id;
+ struct bfin_rtc *rtc = dev_get_drvdata(dev);
unsigned long events = 0;
u16 rtc_istat;

--
1.5.3.4

2007-11-23 10:09:32

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 1/7] Blackfin RTC driver: the frequency function is in units of Hz, not units of seconds, so lock our driver down to 1 Hz

From: Mike Frysinger <[email protected]>

Signed-off-by: Mike Frysinger <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
drivers/rtc/rtc-bfin.c | 24 +++++++++++++++++-------
1 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/rtc/rtc-bfin.c b/drivers/rtc/rtc-bfin.c
index 1aa709d..303ed6e 100644
--- a/drivers/rtc/rtc-bfin.c
+++ b/drivers/rtc/rtc-bfin.c
@@ -72,7 +72,7 @@ struct bfin_rtc {
#define SEC_BITS_OFF 0

/* Some helper functions to convert between the common RTC notion of time
- * and the internal Blackfin notion that is stored in 32bits.
+ * and the internal Blackfin notion that is encoded in 32bits.
*/
static inline u32 rtc_time_to_bfin(unsigned long now)
{
@@ -112,6 +112,11 @@ static inline void rtc_bfin_to_tm(u32 rtc_bfin, struct rtc_time *tm)
* If anyone can point out the obvious solution here, i'm listening :). This
* shouldn't be an issue on an SMP or preempt system as this function should
* only be called with the rtc lock held.
+ *
+ * Other options:
+ * - disable PREN so the sync happens at 32.768kHZ ... but this changes the
+ * inc rate for all RTC registers from 1HZ to 32.768kHZ ...
+ * - use the write complete IRQ
*/
static void rtc_bfin_sync_pending(void)
{
@@ -356,12 +361,18 @@ static int bfin_rtc_proc(struct device *dev, struct seq_file *seq)
return 0;
}

+/**
+ * bfin_irq_set_freq - make sure hardware supports requested freq
+ * @dev: pointer to RTC device structure
+ * @freq: requested frequency rate
+ *
+ * The Blackfin RTC can only generate periodic events at 1 per
+ * second (1 Hz), so reject any attempt at changing it.
+ */
static int bfin_irq_set_freq(struct device *dev, int freq)
{
- struct bfin_rtc *rtc = dev_get_drvdata(dev);
stampit();
- rtc->rtc_dev->irq_freq = freq;
- return 0;
+ return -ENOTTY;
}

static struct rtc_class_ops bfin_rtc_ops = {
@@ -394,14 +405,13 @@ static int __devinit bfin_rtc_probe(struct platform_device *pdev)
ret = PTR_ERR(rtc->rtc_dev);
goto err;
}
- rtc->rtc_dev->irq_freq = 0;
- rtc->rtc_dev->max_user_freq = (2 << 16); /* stopwatch is an unsigned 16 bit reg */
+ rtc->rtc_dev->irq_freq = 1;

platform_set_drvdata(pdev, rtc);

return 0;

-err:
+ err:
kfree(rtc);
return ret;
}
--
1.5.3.4

2007-11-23 10:09:47

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 5/7] Blackfin RTC driver: read_alarm() checks the enabled field, not the pending field.

From: Mike Frysinger <[email protected]>

also, dont bother using memcpy since we can just do an assignment of the same structure.

Signed-off-by: Mike Frysinger <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
drivers/rtc/rtc-bfin.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-bfin.c b/drivers/rtc/rtc-bfin.c
index b14a9c4..a4ed2f8 100644
--- a/drivers/rtc/rtc-bfin.c
+++ b/drivers/rtc/rtc-bfin.c
@@ -326,7 +326,7 @@ static int bfin_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
struct bfin_rtc *rtc = dev_get_drvdata(dev);
dev_dbg_stamp(dev);
memcpy(&alrm->time, &rtc->rtc_alarm, sizeof(struct rtc_time));
- alrm->pending = !!(bfin_read_RTC_ICTL() & (RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY));
+ alrm->enabled = !!(bfin_read_RTC_ICTL() & (RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY));
return 0;
}

@@ -334,7 +334,7 @@ static int bfin_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
{
struct bfin_rtc *rtc = dev_get_drvdata(dev);
dev_dbg_stamp(dev);
- memcpy(&rtc->rtc_alarm, &alrm->time, sizeof(struct rtc_time));
+ rtc->rtc_alarm = alrm->time;
return 0;
}

--
1.5.3.4

2007-11-23 10:10:07

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 6/7] Blackfin RTC driver: shave off another memcpy() by using assignment.

From: Mike Frysinger <[email protected]>

Signed-off-by: Mike Frysinger <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
drivers/rtc/rtc-bfin.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/rtc/rtc-bfin.c b/drivers/rtc/rtc-bfin.c
index a4ed2f8..69810a2 100644
--- a/drivers/rtc/rtc-bfin.c
+++ b/drivers/rtc/rtc-bfin.c
@@ -325,7 +325,7 @@ static int bfin_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
{
struct bfin_rtc *rtc = dev_get_drvdata(dev);
dev_dbg_stamp(dev);
- memcpy(&alrm->time, &rtc->rtc_alarm, sizeof(struct rtc_time));
+ alrm->time = rtc->rtc_alarm;
alrm->enabled = !!(bfin_read_RTC_ICTL() & (RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY));
return 0;
}
--
1.5.3.4

2007-11-23 10:10:37

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 3/7] Blackfin RTC driver: cleanup proc handler (we dont need RTC reg dump now that we have MMR filesystem in sysfs)

From: Mike Frysinger <[email protected]>

Signed-off-by: Mike Frysinger <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
drivers/rtc/rtc-bfin.c | 26 ++++++++++++--------------
1 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-bfin.c b/drivers/rtc/rtc-bfin.c
index 2a801f2..343f284 100644
--- a/drivers/rtc/rtc-bfin.c
+++ b/drivers/rtc/rtc-bfin.c
@@ -45,7 +45,7 @@

#include <asm/blackfin.h>

-#define stamp(fmt, args...) pr_debug("%s:%i: " fmt "\n", __FUNCTION__, __LINE__, ## args)
+#define stamp(fmt, args...) pr_debug("%s:%i: " fmt "\n", __func__, __LINE__, ## args)
#define stampit() stamp("here i am")

struct bfin_rtc {
@@ -343,22 +343,20 @@ static int bfin_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)

static int bfin_rtc_proc(struct device *dev, struct seq_file *seq)
{
-#define yesno(x) (x ? "yes" : "no")
+#define yesno(x) ((x) ? "yes" : "no")
u16 ictl = bfin_read_RTC_ICTL();
stampit();
- seq_printf(seq, "alarm_IRQ\t: %s\n", yesno(ictl & RTC_ISTAT_ALARM));
- seq_printf(seq, "wkalarm_IRQ\t: %s\n", yesno(ictl & RTC_ISTAT_ALARM_DAY));
- seq_printf(seq, "seconds_IRQ\t: %s\n", yesno(ictl & RTC_ISTAT_SEC));
- seq_printf(seq, "periodic_IRQ\t: %s\n", yesno(ictl & RTC_ISTAT_STOPWATCH));
-#ifdef DEBUG
- seq_printf(seq, "RTC_STAT\t: 0x%08X\n", bfin_read_RTC_STAT());
- seq_printf(seq, "RTC_ICTL\t: 0x%04X\n", bfin_read_RTC_ICTL());
- seq_printf(seq, "RTC_ISTAT\t: 0x%04X\n", bfin_read_RTC_ISTAT());
- seq_printf(seq, "RTC_SWCNT\t: 0x%04X\n", bfin_read_RTC_SWCNT());
- seq_printf(seq, "RTC_ALARM\t: 0x%08X\n", bfin_read_RTC_ALARM());
- seq_printf(seq, "RTC_PREN\t: 0x%04X\n", bfin_read_RTC_PREN());
-#endif
+ seq_printf(seq,
+ "alarm_IRQ\t: %s\n"
+ "wkalarm_IRQ\t: %s\n"
+ "seconds_IRQ\t: %s\n"
+ "periodic_IRQ\t: %s\n",
+ yesno(ictl & RTC_ISTAT_ALARM),
+ yesno(ictl & RTC_ISTAT_ALARM_DAY),
+ yesno(ictl & RTC_ISTAT_SEC),
+ yesno(ictl & RTC_ISTAT_STOPWATCH));
return 0;
+#undef yesno
}

/**
--
1.5.3.4

2007-11-23 10:10:54

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 7/7] Blackfin RTC driver: convert sync wait to use the irq write complete notice

From: Mike Frysinger <[email protected]>

- thus clearing out the need for spin locks
- add a small optimization for reading of the rtc field

Signed-off-by: Mike Frysinger <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
drivers/rtc/rtc-bfin.c | 249 ++++++++++++++++++++++++++----------------------
1 files changed, 136 insertions(+), 113 deletions(-)

diff --git a/drivers/rtc/rtc-bfin.c b/drivers/rtc/rtc-bfin.c
index 69810a2..d90ba86 100644
--- a/drivers/rtc/rtc-bfin.c
+++ b/drivers/rtc/rtc-bfin.c
@@ -1,6 +1,6 @@
/*
* Blackfin On-Chip Real Time Clock Driver
- * Supports BF53[123]/BF53[467]/BF54[2489]
+ * Supports BF52[257]/BF53[123]/BF53[467]/BF54[24789]
*
* Copyright 2004-2007 Analog Devices Inc.
*
@@ -32,16 +32,16 @@
* writes to clear status registers complete immediately.
*/

-#include <linux/module.h>
-#include <linux/kernel.h>
#include <linux/bcd.h>
-#include <linux/rtc.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
#include <linux/platform_device.h>
+#include <linux/rtc.h>
#include <linux/seq_file.h>
-#include <linux/interrupt.h>
-#include <linux/spinlock.h>
-#include <linux/delay.h>

#include <asm/blackfin.h>

@@ -50,7 +50,7 @@
struct bfin_rtc {
struct rtc_device *rtc_dev;
struct rtc_time rtc_alarm;
- spinlock_t lock;
+ u16 rtc_wrote_regs;
};

/* Bit values for the ISTAT / ICTL registers */
@@ -96,7 +96,10 @@ static inline void rtc_bfin_to_tm(u32 rtc_bfin, struct rtc_time *tm)
rtc_time_to_tm(rtc_bfin_to_time(rtc_bfin), tm);
}

-/* Wait for the previous write to a RTC register to complete.
+/**
+ * bfin_rtc_sync_pending - make sure pending writes have complete
+ *
+ * Wait for the previous write to a RTC register to complete.
* Unfortunately, we can't sleep here as that introduces a race condition when
* turning on interrupt events. Consider this:
* - process sets alarm
@@ -117,64 +120,102 @@ static inline void rtc_bfin_to_tm(u32 rtc_bfin, struct rtc_time *tm)
* inc rate for all RTC registers from 1HZ to 32.768kHZ ...
* - use the write complete IRQ
*/
-static void rtc_bfin_sync_pending(void)
+/*
+static void bfin_rtc_sync_pending_polled(void)
{
- while (!(bfin_read_RTC_ISTAT() & RTC_ISTAT_WRITE_COMPLETE)) {
+ while (!(bfin_read_RTC_ISTAT() & RTC_ISTAT_WRITE_COMPLETE))
if (!(bfin_read_RTC_ISTAT() & RTC_ISTAT_WRITE_PENDING))
break;
- }
bfin_write_RTC_ISTAT(RTC_ISTAT_WRITE_COMPLETE);
}
+*/
+static DECLARE_COMPLETION(bfin_write_complete);
+static void bfin_rtc_sync_pending(struct device *dev)
+{
+ dev_dbg_stamp(dev);
+ while (bfin_read_RTC_ISTAT() & RTC_ISTAT_WRITE_PENDING)
+ wait_for_completion_timeout(&bfin_write_complete, HZ * 5);
+ dev_dbg_stamp(dev);
+}

-static void rtc_bfin_reset(struct device *dev)
+/**
+ * bfin_rtc_reset - set RTC to sane/known state
+ *
+ * Initialize the RTC. Enable pre-scaler to scale RTC clock
+ * to 1Hz and clear interrupt/status registers.
+ */
+static void bfin_rtc_reset(struct device *dev)
{
struct bfin_rtc *rtc = dev_get_drvdata(dev);
- /* Initialize the RTC. Enable pre-scaler to scale RTC clock
- * to 1Hz and clear interrupt/status registers. */
- spin_lock_irq(&rtc->lock);
- rtc_bfin_sync_pending();
+ dev_dbg_stamp(dev);
+ bfin_rtc_sync_pending(dev);
bfin_write_RTC_PREN(0x1);
- bfin_write_RTC_ICTL(0);
+ bfin_write_RTC_ICTL(RTC_ISTAT_WRITE_COMPLETE);
bfin_write_RTC_SWCNT(0);
bfin_write_RTC_ALARM(0);
bfin_write_RTC_ISTAT(0xFFFF);
- spin_unlock_irq(&rtc->lock);
+ rtc->rtc_wrote_regs = 0;
}

+/**
+ * bfin_rtc_interrupt - handle interrupt from RTC
+ *
+ * Since we handle all RTC events here, we have to make sure the requested
+ * interrupt is enabled (in RTC_ICTL) as the event status register (RTC_ISTAT)
+ * always gets updated regardless of the interrupt being enabled. So when one
+ * even we care about (e.g. stopwatch) goes off, we don't want to turn around
+ * and say that other events have happened as well (e.g. second). We do not
+ * have to worry about pending writes to the RTC_ICTL register as interrupts
+ * only fire if they are enabled in the RTC_ICTL register.
+ */
static irqreturn_t bfin_rtc_interrupt(int irq, void *dev_id)
{
struct device *dev = dev_id;
struct bfin_rtc *rtc = dev_get_drvdata(dev);
unsigned long events = 0;
- u16 rtc_istat;
+ bool write_complete = false;
+ u16 rtc_istat, rtc_ictl;

dev_dbg_stamp(dev);

- spin_lock_irq(&rtc->lock);
-
rtc_istat = bfin_read_RTC_ISTAT();
+ rtc_ictl = bfin_read_RTC_ICTL();

- if (rtc_istat & (RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY)) {
- bfin_write_RTC_ISTAT(RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY);
- events |= RTC_AF | RTC_IRQF;
+ if (rtc_istat & RTC_ISTAT_WRITE_COMPLETE) {
+ bfin_write_RTC_ISTAT(RTC_ISTAT_WRITE_COMPLETE);
+ write_complete = true;
+ complete(&bfin_write_complete);
}

- if (rtc_istat & RTC_ISTAT_STOPWATCH) {
- bfin_write_RTC_ISTAT(RTC_ISTAT_STOPWATCH);
- events |= RTC_PF | RTC_IRQF;
- bfin_write_RTC_SWCNT(rtc->rtc_dev->irq_freq);
+ if (rtc_ictl & (RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY)) {
+ if (rtc_istat & (RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY)) {
+ bfin_write_RTC_ISTAT(RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY);
+ events |= RTC_AF | RTC_IRQF;
+ }
}

- if (rtc_istat & RTC_ISTAT_SEC) {
- bfin_write_RTC_ISTAT(RTC_ISTAT_SEC);
- events |= RTC_UF | RTC_IRQF;
+ if (rtc_ictl & RTC_ISTAT_STOPWATCH) {
+ if (rtc_istat & RTC_ISTAT_STOPWATCH) {
+ bfin_write_RTC_ISTAT(RTC_ISTAT_STOPWATCH);
+ events |= RTC_PF | RTC_IRQF;
+ bfin_write_RTC_SWCNT(rtc->rtc_dev->irq_freq);
+ }
}

- rtc_update_irq(rtc->rtc_dev, 1, events);
+ if (rtc_ictl & RTC_ISTAT_SEC) {
+ if (rtc_istat & RTC_ISTAT_SEC) {
+ bfin_write_RTC_ISTAT(RTC_ISTAT_SEC);
+ events |= RTC_UF | RTC_IRQF;
+ }
+ }

- spin_unlock_irq(&rtc->lock);
+ if (events)
+ rtc_update_irq(rtc->rtc_dev, 1, events);

- return IRQ_HANDLED;
+ if (write_complete || events)
+ return IRQ_HANDLED;
+ else
+ return IRQ_NONE;
}

static int bfin_rtc_open(struct device *dev)
@@ -183,13 +224,9 @@ static int bfin_rtc_open(struct device *dev)

dev_dbg_stamp(dev);

- ret = request_irq(IRQ_RTC, bfin_rtc_interrupt, IRQF_DISABLED, "rtc-bfin", dev);
- if (unlikely(ret)) {
- dev_err(dev, "request RTC IRQ failed with %d\n", ret);
- return ret;
- }
-
- rtc_bfin_reset(dev);
+ ret = request_irq(IRQ_RTC, bfin_rtc_interrupt, IRQF_SHARED, to_platform_device(dev)->name, dev);
+ if (!ret)
+ bfin_rtc_reset(dev);

return ret;
}
@@ -197,93 +234,70 @@ static int bfin_rtc_open(struct device *dev)
static void bfin_rtc_release(struct device *dev)
{
dev_dbg_stamp(dev);
- rtc_bfin_reset(dev);
+ bfin_rtc_reset(dev);
free_irq(IRQ_RTC, dev);
}

+static void bfin_rtc_int_set(struct bfin_rtc *rtc, u16 rtc_int)
+{
+ bfin_write_RTC_ISTAT(rtc_int);
+ bfin_write_RTC_ICTL(bfin_read_RTC_ICTL() | rtc_int);
+}
+static void bfin_rtc_int_clear(struct bfin_rtc *rtc, u16 rtc_int)
+{
+ bfin_write_RTC_ICTL(bfin_read_RTC_ICTL() & rtc_int);
+}
+static void bfin_rtc_int_set_alarm(struct bfin_rtc *rtc)
+{
+ /* Blackfin has different bits for whether the alarm is
+ * more than 24 hours away.
+ */
+ bfin_rtc_int_set(rtc, (rtc->rtc_alarm.tm_yday == -1 ? RTC_ISTAT_ALARM : RTC_ISTAT_ALARM_DAY));
+}
static int bfin_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
{
struct bfin_rtc *rtc = dev_get_drvdata(dev);
+ int ret = 0;

dev_dbg_stamp(dev);

+ bfin_rtc_sync_pending(dev);
+
switch (cmd) {
case RTC_PIE_ON:
dev_dbg_stamp(dev);
- spin_lock_irq(&rtc->lock);
- rtc_bfin_sync_pending();
- bfin_write_RTC_ISTAT(RTC_ISTAT_STOPWATCH);
+ bfin_rtc_int_set(rtc, RTC_ISTAT_STOPWATCH);
bfin_write_RTC_SWCNT(rtc->rtc_dev->irq_freq);
- bfin_write_RTC_ICTL(bfin_read_RTC_ICTL() | RTC_ISTAT_STOPWATCH);
- spin_unlock_irq(&rtc->lock);
- return 0;
+ break;
case RTC_PIE_OFF:
dev_dbg_stamp(dev);
- spin_lock_irq(&rtc->lock);
- rtc_bfin_sync_pending();
- bfin_write_RTC_SWCNT(0);
- bfin_write_RTC_ICTL(bfin_read_RTC_ICTL() & ~RTC_ISTAT_STOPWATCH);
- spin_unlock_irq(&rtc->lock);
- return 0;
+ bfin_rtc_int_clear(rtc, ~RTC_ISTAT_STOPWATCH);
+ break;

case RTC_UIE_ON:
dev_dbg_stamp(dev);
- spin_lock_irq(&rtc->lock);
- rtc_bfin_sync_pending();
- bfin_write_RTC_ISTAT(RTC_ISTAT_SEC);
- bfin_write_RTC_ICTL(bfin_read_RTC_ICTL() | RTC_ISTAT_SEC);
- spin_unlock_irq(&rtc->lock);
- return 0;
+ bfin_rtc_int_set(rtc, RTC_ISTAT_SEC);
+ break;
case RTC_UIE_OFF:
dev_dbg_stamp(dev);
- spin_lock_irq(&rtc->lock);
- rtc_bfin_sync_pending();
- bfin_write_RTC_ICTL(bfin_read_RTC_ICTL() & ~RTC_ISTAT_SEC);
- spin_unlock_irq(&rtc->lock);
- return 0;
-
- case RTC_AIE_ON: {
- unsigned long rtc_alarm;
- u16 which_alarm;
- int ret = 0;
+ bfin_rtc_int_clear(rtc, ~RTC_ISTAT_SEC);
+ break;

+ case RTC_AIE_ON:
dev_dbg_stamp(dev);
-
- spin_lock_irq(&rtc->lock);
-
- rtc_bfin_sync_pending();
- if (rtc->rtc_alarm.tm_yday == -1) {
- struct rtc_time now;
- rtc_bfin_to_tm(bfin_read_RTC_STAT(), &now);
- now.tm_sec = rtc->rtc_alarm.tm_sec;
- now.tm_min = rtc->rtc_alarm.tm_min;
- now.tm_hour = rtc->rtc_alarm.tm_hour;
- ret = rtc_tm_to_time(&now, &rtc_alarm);
- which_alarm = RTC_ISTAT_ALARM;
- } else {
- ret = rtc_tm_to_time(&rtc->rtc_alarm, &rtc_alarm);
- which_alarm = RTC_ISTAT_ALARM_DAY;
- }
- if (ret == 0) {
- bfin_write_RTC_ISTAT(which_alarm);
- bfin_write_RTC_ALARM(rtc_time_to_bfin(rtc_alarm));
- bfin_write_RTC_ICTL(bfin_read_RTC_ICTL() | which_alarm);
- }
-
- spin_unlock_irq(&rtc->lock);
-
- return ret;
- }
+ bfin_rtc_int_set_alarm(rtc);
+ break;
case RTC_AIE_OFF:
dev_dbg_stamp(dev);
- spin_lock_irq(&rtc->lock);
- rtc_bfin_sync_pending();
- bfin_write_RTC_ICTL(bfin_read_RTC_ICTL() & ~(RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY));
- spin_unlock_irq(&rtc->lock);
- return 0;
+ bfin_rtc_int_clear(rtc, ~(RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY));
+ break;
+
+ default:
+ dev_dbg_stamp(dev);
+ ret = -ENOIOCTLCMD;
}

- return -ENOIOCTLCMD;
+ return ret;
}

static int bfin_rtc_read_time(struct device *dev, struct rtc_time *tm)
@@ -292,10 +306,10 @@ static int bfin_rtc_read_time(struct device *dev, struct rtc_time *tm)

dev_dbg_stamp(dev);

- spin_lock_irq(&rtc->lock);
- rtc_bfin_sync_pending();
+ if (rtc->rtc_wrote_regs & 0x1)
+ bfin_rtc_sync_pending(dev);
+
rtc_bfin_to_tm(bfin_read_RTC_STAT(), tm);
- spin_unlock_irq(&rtc->lock);

return 0;
}
@@ -308,16 +322,14 @@ static int bfin_rtc_set_time(struct device *dev, struct rtc_time *tm)

dev_dbg_stamp(dev);

- spin_lock_irq(&rtc->lock);
-
ret = rtc_tm_to_time(tm, &now);
if (ret == 0) {
- rtc_bfin_sync_pending();
+ if (rtc->rtc_wrote_regs & 0x1)
+ bfin_rtc_sync_pending(dev);
bfin_write_RTC_STAT(rtc_time_to_bfin(now));
+ rtc->rtc_wrote_regs = 0x1;
}

- spin_unlock_irq(&rtc->lock);
-
return ret;
}

@@ -326,6 +338,7 @@ static int bfin_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
struct bfin_rtc *rtc = dev_get_drvdata(dev);
dev_dbg_stamp(dev);
alrm->time = rtc->rtc_alarm;
+ bfin_rtc_sync_pending(dev);
alrm->enabled = !!(bfin_read_RTC_ICTL() & (RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY));
return 0;
}
@@ -333,8 +346,20 @@ static int bfin_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
static int bfin_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
{
struct bfin_rtc *rtc = dev_get_drvdata(dev);
+ unsigned long rtc_alarm;
+
dev_dbg_stamp(dev);
+
+ if (rtc_tm_to_time(&alrm->time, &rtc_alarm))
+ return -EINVAL;
+
rtc->rtc_alarm = alrm->time;
+
+ bfin_rtc_sync_pending(dev);
+ bfin_write_RTC_ALARM(rtc_time_to_bfin(rtc_alarm));
+ if (alrm->enabled)
+ bfin_rtc_int_set_alarm(rtc);
+
return 0;
}

@@ -393,8 +418,6 @@ static int __devinit bfin_rtc_probe(struct platform_device *pdev)
if (unlikely(!rtc))
return -ENOMEM;

- spin_lock_init(&rtc->lock);
-
rtc->rtc_dev = rtc_device_register(pdev->name, &pdev->dev, &bfin_rtc_ops, THIS_MODULE);
if (unlikely(IS_ERR(rtc))) {
ret = PTR_ERR(rtc->rtc_dev);
--
1.5.3.4

2007-11-23 10:11:22

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 4/7] Blackfin RTC driver: use dev_dbg() rather than pr_stamp()

From: Mike Frysinger <[email protected]>

Signed-off-by: Mike Frysinger <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
drivers/rtc/rtc-bfin.c | 48 ++++++++++++++++++++++--------------------------
1 files changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/rtc/rtc-bfin.c b/drivers/rtc/rtc-bfin.c
index 343f284..b14a9c4 100644
--- a/drivers/rtc/rtc-bfin.c
+++ b/drivers/rtc/rtc-bfin.c
@@ -45,8 +45,7 @@

#include <asm/blackfin.h>

-#define stamp(fmt, args...) pr_debug("%s:%i: " fmt "\n", __func__, __LINE__, ## args)
-#define stampit() stamp("here i am")
+#define dev_dbg_stamp(dev) dev_dbg(dev, "%s:%i: here i am\n", __func__, __LINE__)

struct bfin_rtc {
struct rtc_device *rtc_dev;
@@ -120,7 +119,6 @@ static inline void rtc_bfin_to_tm(u32 rtc_bfin, struct rtc_time *tm)
*/
static void rtc_bfin_sync_pending(void)
{
- stampit();
while (!(bfin_read_RTC_ISTAT() & RTC_ISTAT_WRITE_COMPLETE)) {
if (!(bfin_read_RTC_ISTAT() & RTC_ISTAT_WRITE_PENDING))
break;
@@ -128,8 +126,9 @@ static void rtc_bfin_sync_pending(void)
bfin_write_RTC_ISTAT(RTC_ISTAT_WRITE_COMPLETE);
}

-static void rtc_bfin_reset(struct bfin_rtc *rtc)
+static void rtc_bfin_reset(struct device *dev)
{
+ struct bfin_rtc *rtc = dev_get_drvdata(dev);
/* Initialize the RTC. Enable pre-scaler to scale RTC clock
* to 1Hz and clear interrupt/status registers. */
spin_lock_irq(&rtc->lock);
@@ -149,7 +148,7 @@ static irqreturn_t bfin_rtc_interrupt(int irq, void *dev_id)
unsigned long events = 0;
u16 rtc_istat;

- stampit();
+ dev_dbg_stamp(dev);

spin_lock_irq(&rtc->lock);

@@ -180,10 +179,9 @@ static irqreturn_t bfin_rtc_interrupt(int irq, void *dev_id)

static int bfin_rtc_open(struct device *dev)
{
- struct bfin_rtc *rtc = dev_get_drvdata(dev);
int ret;

- stampit();
+ dev_dbg_stamp(dev);

ret = request_irq(IRQ_RTC, bfin_rtc_interrupt, IRQF_DISABLED, "rtc-bfin", dev);
if (unlikely(ret)) {
@@ -191,16 +189,15 @@ static int bfin_rtc_open(struct device *dev)
return ret;
}

- rtc_bfin_reset(rtc);
+ rtc_bfin_reset(dev);

return ret;
}

static void bfin_rtc_release(struct device *dev)
{
- struct bfin_rtc *rtc = dev_get_drvdata(dev);
- stampit();
- rtc_bfin_reset(rtc);
+ dev_dbg_stamp(dev);
+ rtc_bfin_reset(dev);
free_irq(IRQ_RTC, dev);
}

@@ -208,11 +205,11 @@ static int bfin_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long ar
{
struct bfin_rtc *rtc = dev_get_drvdata(dev);

- stampit();
+ dev_dbg_stamp(dev);

switch (cmd) {
case RTC_PIE_ON:
- stampit();
+ dev_dbg_stamp(dev);
spin_lock_irq(&rtc->lock);
rtc_bfin_sync_pending();
bfin_write_RTC_ISTAT(RTC_ISTAT_STOPWATCH);
@@ -221,7 +218,7 @@ static int bfin_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long ar
spin_unlock_irq(&rtc->lock);
return 0;
case RTC_PIE_OFF:
- stampit();
+ dev_dbg_stamp(dev);
spin_lock_irq(&rtc->lock);
rtc_bfin_sync_pending();
bfin_write_RTC_SWCNT(0);
@@ -230,7 +227,7 @@ static int bfin_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long ar
return 0;

case RTC_UIE_ON:
- stampit();
+ dev_dbg_stamp(dev);
spin_lock_irq(&rtc->lock);
rtc_bfin_sync_pending();
bfin_write_RTC_ISTAT(RTC_ISTAT_SEC);
@@ -238,7 +235,7 @@ static int bfin_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long ar
spin_unlock_irq(&rtc->lock);
return 0;
case RTC_UIE_OFF:
- stampit();
+ dev_dbg_stamp(dev);
spin_lock_irq(&rtc->lock);
rtc_bfin_sync_pending();
bfin_write_RTC_ICTL(bfin_read_RTC_ICTL() & ~RTC_ISTAT_SEC);
@@ -250,7 +247,7 @@ static int bfin_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long ar
u16 which_alarm;
int ret = 0;

- stampit();
+ dev_dbg_stamp(dev);

spin_lock_irq(&rtc->lock);

@@ -278,7 +275,7 @@ static int bfin_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long ar
return ret;
}
case RTC_AIE_OFF:
- stampit();
+ dev_dbg_stamp(dev);
spin_lock_irq(&rtc->lock);
rtc_bfin_sync_pending();
bfin_write_RTC_ICTL(bfin_read_RTC_ICTL() & ~(RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY));
@@ -293,7 +290,7 @@ static int bfin_rtc_read_time(struct device *dev, struct rtc_time *tm)
{
struct bfin_rtc *rtc = dev_get_drvdata(dev);

- stampit();
+ dev_dbg_stamp(dev);

spin_lock_irq(&rtc->lock);
rtc_bfin_sync_pending();
@@ -309,7 +306,7 @@ static int bfin_rtc_set_time(struct device *dev, struct rtc_time *tm)
int ret;
unsigned long now;

- stampit();
+ dev_dbg_stamp(dev);

spin_lock_irq(&rtc->lock);

@@ -327,7 +324,7 @@ static int bfin_rtc_set_time(struct device *dev, struct rtc_time *tm)
static int bfin_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
{
struct bfin_rtc *rtc = dev_get_drvdata(dev);
- stampit();
+ dev_dbg_stamp(dev);
memcpy(&alrm->time, &rtc->rtc_alarm, sizeof(struct rtc_time));
alrm->pending = !!(bfin_read_RTC_ICTL() & (RTC_ISTAT_ALARM | RTC_ISTAT_ALARM_DAY));
return 0;
@@ -336,7 +333,7 @@ static int bfin_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
static int bfin_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
{
struct bfin_rtc *rtc = dev_get_drvdata(dev);
- stampit();
+ dev_dbg_stamp(dev);
memcpy(&rtc->rtc_alarm, &alrm->time, sizeof(struct rtc_time));
return 0;
}
@@ -345,7 +342,7 @@ static int bfin_rtc_proc(struct device *dev, struct seq_file *seq)
{
#define yesno(x) ((x) ? "yes" : "no")
u16 ictl = bfin_read_RTC_ICTL();
- stampit();
+ dev_dbg_stamp(dev);
seq_printf(seq,
"alarm_IRQ\t: %s\n"
"wkalarm_IRQ\t: %s\n"
@@ -369,7 +366,7 @@ static int bfin_rtc_proc(struct device *dev, struct seq_file *seq)
*/
static int bfin_irq_set_freq(struct device *dev, int freq)
{
- stampit();
+ dev_dbg_stamp(dev);
return -ENOTTY;
}

@@ -390,7 +387,7 @@ static int __devinit bfin_rtc_probe(struct platform_device *pdev)
struct bfin_rtc *rtc;
int ret = 0;

- stampit();
+ dev_dbg_stamp(&pdev->dev);

rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
if (unlikely(!rtc))
@@ -436,7 +433,6 @@ static struct platform_driver bfin_rtc_driver = {

static int __init bfin_rtc_init(void)
{
- stampit();
return platform_driver_register(&bfin_rtc_driver);
}

--
1.5.3.4

2007-12-04 16:27:08

by Alessandro Zummo

[permalink] [raw]
Subject: Re: [PATCH 1/7] Blackfin RTC driver: the frequency function is in units of Hz, not units of seconds, so lock our driver down to 1 Hz

On Fri, 23 Nov 2007 18:08:26 +0800
Bryan Wu <[email protected]> wrote:

Hi, I fannly got some time to review the patches. Seems ok, the
only question is...

> static int bfin_irq_set_freq(struct device *dev, int freq)
> {
> - struct bfin_rtc *rtc = dev_get_drvdata(dev);
> stampit();
> - rtc->rtc_dev->irq_freq = freq;
> - return 0;
> + return -ENOTTY;
> }

.. why ENOTTY here?

--

Best regards,

Alessandro Zummo,
Tower Technologies - Torino, Italy

http://www.towertech.it

2007-12-04 16:29:21

by Mike Frysinger

[permalink] [raw]
Subject: Re: [rtc-linux] Re: [PATCH 1/7] Blackfin RTC driver: the frequency function is in units of Hz, not units of seconds, so lock our driver down to 1 Hz

On Dec 4, 2007 11:26 AM, Alessandro Zummo <[email protected]> wrote:
> Bryan Wu <[email protected]> wrote:
>
> Hi, I fannly got some time to review the patches. Seems ok, the
> only question is...
>
> > static int bfin_irq_set_freq(struct device *dev, int freq)
> > {
> > - struct bfin_rtc *rtc = dev_get_drvdata(dev);
> > stampit();
> > - rtc->rtc_dev->irq_freq = freq;
> > - return 0;
> > + return -ENOTTY;
> > }
>
> .. why ENOTTY here?

blah, hit "reply" in previous e-mail ...

that's what the documentation says to do as does every other rtc driver ?
-mike

2007-12-04 16:42:34

by Alessandro Zummo

[permalink] [raw]
Subject: Re: [rtc-linux] Re: [PATCH 1/7] Blackfin RTC driver: the frequency function is in units of Hz, not units of seconds, so lock our driver down to 1 Hz

On Tue, 4 Dec 2007 11:29:11 -0500
"Mike Frysinger" <[email protected]> wrote:

>
> On Dec 4, 2007 11:26 AM, Alessandro Zummo <[email protected]> wrote:
> > Bryan Wu <[email protected]> wrote:
> >
> > Hi, I fannly got some time to review the patches. Seems ok, the
> > only question is...
> >
> > > static int bfin_irq_set_freq(struct device *dev, int freq)
> > > {
> > > - struct bfin_rtc *rtc = dev_get_drvdata(dev);
> > > stampit();
> > > - rtc->rtc_dev->irq_freq = freq;
> > > - return 0;
> > > + return -ENOTTY;
> > > }
> >
> > .. why ENOTTY here?
>
> blah, hit "reply" in previous e-mail ...
>
> that's what the documentation says to do as does every other rtc driver ?
> -mike

it should be EINVAL. at least it is wat rtc-cmos does when the value
is invalid.

--

Best regards,

Alessandro Zummo,
Tower Technologies - Torino, Italy

http://www.towertech.it

2007-12-04 16:48:14

by Mike Frysinger

[permalink] [raw]
Subject: Re: [rtc-linux] Re: [PATCH 1/7] Blackfin RTC driver: the frequency function is in units of Hz, not units of seconds, so lock our driver down to 1 Hz

On Dec 4, 2007 11:41 AM, Alessandro Zummo <[email protected]> wrote:
> On Tue, 4 Dec 2007 11:29:11 -0500 "Mike Frysinger" <[email protected]> wrote:
> > On Dec 4, 2007 11:26 AM, Alessandro Zummo <[email protected]> wrote:
> > > Bryan Wu <[email protected]> wrote:
> > >
> > > Hi, I fannly got some time to review the patches. Seems ok, the
> > > only question is...
> > >
> > > > static int bfin_irq_set_freq(struct device *dev, int freq)
> > > > {
> > > > - struct bfin_rtc *rtc = dev_get_drvdata(dev);
> > > > stampit();
> > > > - rtc->rtc_dev->irq_freq = freq;
> > > > - return 0;
> > > > + return -ENOTTY;
> > > > }
> > >
> > > .. why ENOTTY here?
> >
> > that's what the documentation says to do as does every other rtc driver ?
>
> it should be EINVAL. at least it is wat rtc-cmos does when the value
> is invalid.

the return of ENOTTY is to say "changing of freq isnt supported", not
that the value is invalid. but i can get the same behavior by
deleting the function as the rtc-dev layer will take care of returning
ENOTTY.

so the behavior is for the RTC_IRQP_SET ioctl:
- return ENOTTY if you cannot change freq
- the rtc-dev layer will do this for you if you do not set irq_set_freq
- return EINVAL if the requested freq is not within the capabilities
of the hardware

sound about right ?
-mike

2007-12-04 16:50:21

by Alessandro Zummo

[permalink] [raw]
Subject: Re: [rtc-linux] Re: [PATCH 1/7] Blackfin RTC driver: the frequency function is in units of Hz, not units of seconds, so lock our driver down to 1 Hz

On Tue, 4 Dec 2007 11:47:56 -0500
"Mike Frysinger" <[email protected]> wrote:

>
> the return of ENOTTY is to say "changing of freq isnt supported", not
> that the value is invalid. but i can get the same behavior by
> deleting the function as the rtc-dev layer will take care of returning
> ENOTTY.
>
> so the behavior is for the RTC_IRQP_SET ioctl:
> - return ENOTTY if you cannot change freq
> - the rtc-dev layer will do this for you if you do not set irq_set_freq
> - return EINVAL if the requested freq is not within the capabilities
> of the hardware
>
> sound about right ?

yes, it is. I agree about deleting it.


--

Best regards,

Alessandro Zummo,
Tower Technologies - Torino, Italy

http://www.towertech.it

2007-12-04 16:57:12

by Mike Frysinger

[permalink] [raw]
Subject: Re: [rtc-linux] Re: [PATCH 1/7] Blackfin RTC driver: the frequency function is in units of Hz, not units of seconds, so lock our driver down to 1 Hz

On Dec 4, 2007 11:49 AM, Alessandro Zummo <[email protected]> wrote:
> On Tue, 4 Dec 2007 11:47:56 -0500
> "Mike Frysinger" <[email protected]> wrote:
> > the return of ENOTTY is to say "changing of freq isnt supported", not
> > that the value is invalid. but i can get the same behavior by
> > deleting the function as the rtc-dev layer will take care of returning
> > ENOTTY.
> >
> > so the behavior is for the RTC_IRQP_SET ioctl:
> > - return ENOTTY if you cannot change freq
> > - the rtc-dev layer will do this for you if you do not set irq_set_freq
> > - return EINVAL if the requested freq is not within the capabilities
> > of the hardware
> >
> > sound about right ?
>
> yes, it is. I agree about deleting it.

so, merge this patch and i'll follow up with another to delete the
func and update the documentation ?
-mike

2007-12-04 17:00:26

by Alessandro Zummo

[permalink] [raw]
Subject: Re: [rtc-linux] Re: [PATCH 1/7] Blackfin RTC driver: the frequency function is in units of Hz, not units of seconds, so lock our driver down to 1 Hz

On Tue, 4 Dec 2007 11:56:54 -0500
"Mike Frysinger" <[email protected]> wrote:

> On Dec 4, 2007 11:49 AM, Alessandro Zummo <[email protected]> wrote:
> > On Tue, 4 Dec 2007 11:47:56 -0500
> > "Mike Frysinger" <[email protected]> wrote:
> > > the return of ENOTTY is to say "changing of freq isnt supported", not
> > > that the value is invalid. but i can get the same behavior by
> > > deleting the function as the rtc-dev layer will take care of returning
> > > ENOTTY.
> > >
> > > so the behavior is for the RTC_IRQP_SET ioctl:
> > > - return ENOTTY if you cannot change freq
> > > - the rtc-dev layer will do this for you if you do not set irq_set_freq
> > > - return EINVAL if the requested freq is not within the capabilities
> > > of the hardware
> > >
> > > sound about right ?
> >
> > yes, it is. I agree about deleting it.
>
> so, merge this patch and i'll follow up with another to delete the
> func and update the documentation ?

ok. You have my

Signed-off-by: Alessandro Zummo <[email protected]>

on the whole patch set.

--

Best regards,

Alessandro Zummo,
Tower Technologies - Torino, Italy

http://www.towertech.it

2007-12-04 17:15:56

by Mike Frysinger

[permalink] [raw]
Subject: Re: [rtc-linux] Re: [PATCH 1/7] Blackfin RTC driver: the frequency function is in units of Hz, not units of seconds, so lock our driver down to 1 Hz

On Dec 4, 2007 11:41 AM, Alessandro Zummo <[email protected]> wrote:
> it should be EINVAL. at least it is wat rtc-cmos does when the value
> is invalid.

in the example rtc test code (Documentation/rtc.txt), would it be
useful to handle EINVAL ? if the freq isnt supported, just say
"hardware does not support frequency %i" and continue on to try the
next freq ? the current code just does:
perror("RTC_IRQP_SET ioctl");
exit(errno);
-mike