rtc: rs5c372: support alarms up to 1 week

PATCH In review Daniel González Cabanelas 2 participants 3 messages Nov 23, 2020 – Mar 8, 2021 Real time clock (RTC)

lore

Share & tools

lore

Daniel González Cabanelas

author

2020-11-23 10:40 UTC #0 lore raw

PATCH 1/2 rtc: rs5c372: support alarms up to 1 week

The Ricoh R2221x, R2223x, RS5C372, RV5C387A chips can handle 1 week alarms.

Read the "wday" alarm register and convert it to a date to support up 1 week in our driver.

Signed-off-by Daniel González Cabanelas <[email protected]>

---
 drivers/rtc/rtc-rs5c372.c | 48 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 6 deletions(-)

drivers/rtc/rtc-rs5c372.c +42 -6

index 3bd6eaa0d..94b778c6e 100644
@@ -393,7 +393,9 @@ static int rs5c_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client	*client = to_i2c_client(dev);
 	struct rs5c372		*rs5c = i2c_get_clientdata(client);
-	int			status;
+	int			status, wday_offs;
+	struct rtc_time 	rtc;
+	unsigned long 		alarm_secs;
 
 	status = rs5c_get_regs(rs5c);
 	if (status < 0)

@@ -403,6 +405,30 @@ static int rs5c_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 	t->time.tm_sec = 0;
 	t->time.tm_min = bcd2bin(rs5c->regs[RS5C_REG_ALARM_A_MIN] & 0x7f);
 	t->time.tm_hour = rs5c_reg2hr(rs5c, rs5c->regs[RS5C_REG_ALARM_A_HOURS]);
+	t->time.tm_wday = ffs(rs5c->regs[RS5C_REG_ALARM_A_WDAY] & 0x7f) - 1;
+
+	/* determine the day, month and year based on alarm wday, taking as a
+	 * reference the current time from the rtc
+	 */
+	status = rs5c372_rtc_read_time(dev, &rtc);
+	if (status < 0)
+		return status;
+
+	wday_offs = t->time.tm_wday - rtc.tm_wday;
+	alarm_secs = mktime64(rtc.tm_year + 1900,
+			      rtc.tm_mon + 1,
+			      rtc.tm_mday + wday_offs,
+			      t->time.tm_hour,
+			      t->time.tm_min,
+			      t->time.tm_sec);
+
+	if (wday_offs < 0 || (wday_offs == 0 &&
+			      (t->time.tm_hour < rtc.tm_hour ||
+			       (t->time.tm_hour == rtc.tm_hour &&
+				t->time.tm_min <= rtc.tm_min))))
+		alarm_secs += 7 * 86400;
+
+	rtc_time64_to_tm(alarm_secs, &t->time);
 
 	/* ... and status */
 	t->enabled = !!(rs5c->regs[RS5C_REG_CTRL1] & RS5C_CTRL1_AALE);

@@ -417,12 +443,20 @@ static int rs5c_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	struct rs5c372		*rs5c = i2c_get_clientdata(client);
 	int			status, addr, i;
 	unsigned char		buf[3];
+	struct rtc_time 	rtc_tm;
+	unsigned long 		rtc_secs, alarm_secs;
 
-	/* only handle up to 24 hours in the future, like RTC_ALM_SET */
-	if (t->time.tm_mday != -1
-			|| t->time.tm_mon != -1
-			|| t->time.tm_year != -1)
+	/* chip only can handle alarms up to one week in the future*/
+	status = rs5c372_rtc_read_time(dev, &rtc_tm);
+	if (status)
+		return status;
+	rtc_secs = rtc_tm_to_time64(&rtc_tm);
+	alarm_secs = rtc_tm_to_time64(&t->time);
+	if (alarm_secs >= rtc_secs + 7 * 86400) {
+		dev_err(dev, "%s: alarm maximum is one week in the future (%d)\n",
+			__func__, status);
 		return -EINVAL;
+	}
 
 	/* REVISIT: round up tm_sec */

@@ -443,7 +477,9 @@ static int rs5c_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	/* set alarm */
 	buf[0] = bin2bcd(t->time.tm_min);
 	buf[1] = rs5c_hr2reg(rs5c, t->time.tm_hour);
-	buf[2] = 0x7f;	/* any/all days */
+	/* each bit is the day of the week, 0x7f means all days */
+	buf[2] = (t->time.tm_wday >= 0 && t->time.tm_wday < 7) ?
+		  BIT(t->time.tm_wday) : 0x7f;
 
 	for (i = 0; i < sizeof(buf); i++) {
 		addr = RS5C_ADDR(RS5C_REG_ALARM_A_MIN + i);

Signature · 1 line

2.29.2

Alexandre Belloni

maintainer

2021-01-12 22:08 UTC #1 lore raw

Hello,

Quoted 16 lines · Daniel González Cabanelas

The Ricoh R2221x, R2223x, RS5C372, RV5C387A chips can handle 1 week alarms.
Read the "wday" alarm register and convert it to a date to support up 1 week in our driver.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
 drivers/rtc/rtc-rs5c372.c | 48 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 6 deletions(-)
drivers/rtc/rtc-rs5c372.c +0 -0
index 3bd6eaa0d..94b778c6e 100644

drivers/rtc/rtc-rs5c372.c static int rs5c_read_alarm(struct device *dev, struct rtc_wkalrm *t) · line 397

Quoted 6 lines

drivers/rtc/rtc-rs5c372.c +2 -1

@@ -393,7 +393,9 @@ static int rs5c_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct i2c_client	*client = to_i2c_client(dev);
 	struct rs5c372		*rs5c = i2c_get_clientdata(client);
-	int			status;
+	int			status, wday_offs;
+	struct rtc_time 	rtc;

You have a few spaces before tabs, please fix them.

Quoted 8 lines

drivers/rtc/rtc-rs5c372.c +1 -0

+	unsigned long 		alarm_secs;
 
 	status = rs5c_get_regs(rs5c);
 	if (status < 0)

@@ -403,6 +405,30 @@ static int rs5c_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 	t->time.tm_sec = 0;
 	t->time.tm_min = bcd2bin(rs5c->regs[RS5C_REG_ALARM_A_MIN] & 0x7f);
 	t->time.tm_hour = rs5c_reg2hr(rs5c, rs5c->regs[RS5C_REG_ALARM_A_HOURS]);

drivers/rtc/rtc-rs5c372.c static int rs5c_read_alarm(struct device *dev, struct rtc_wkalrm *t) · line 417

Quoted 10 lines

drivers/rtc/rtc-rs5c372.c +10 -0

+	t->time.tm_wday = ffs(rs5c->regs[RS5C_REG_ALARM_A_WDAY] & 0x7f) - 1;
+
+	/* determine the day, month and year based on alarm wday, taking as a
+	 * reference the current time from the rtc
+	 */
+	status = rs5c372_rtc_read_time(dev, &rtc);
+	if (status < 0)
+		return status;
+
+	wday_offs = t->time.tm_wday - rtc.tm_wday;

Note that you can not rely on tm_wday being set correctly. The core will not (currently) enforce that and most tools jut pass a bogus value or 0. So you need to calculate wday in rs5c372_rtc_set_time.
I'm currently working on a way for the drivers to ask the core to ensure wday is correct.

Quoted 27 lines

drivers/rtc/rtc-rs5c372.c +16 -3

+	alarm_secs = mktime64(rtc.tm_year + 1900,
+			      rtc.tm_mon + 1,
+			      rtc.tm_mday + wday_offs,
+			      t->time.tm_hour,
+			      t->time.tm_min,
+			      t->time.tm_sec);
+
+	if (wday_offs < 0 || (wday_offs == 0 &&
+			      (t->time.tm_hour < rtc.tm_hour ||
+			       (t->time.tm_hour == rtc.tm_hour &&
+				t->time.tm_min <= rtc.tm_min))))
+		alarm_secs += 7 * 86400;
+
+	rtc_time64_to_tm(alarm_secs, &t->time);
 
 	/* ... and status */
 	t->enabled = !!(rs5c->regs[RS5C_REG_CTRL1] & RS5C_CTRL1_AALE);

@@ -417,12 +443,20 @@ static int rs5c_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	struct rs5c372		*rs5c = i2c_get_clientdata(client);
 	int			status, addr, i;
 	unsigned char		buf[3];
+	struct rtc_time 	rtc_tm;
+	unsigned long 		rtc_secs, alarm_secs;
 
-	/* only handle up to 24 hours in the future, like RTC_ALM_SET */
-	if (t->time.tm_mday != -1
-			|| t->time.tm_mon != -1

drivers/rtc/rtc-rs5c372.c static int rs5c_set_alarm(struct device *dev, struct rtc_wkalrm *t) · line 457

Quoted 10 lines

drivers/rtc/rtc-rs5c372.c +9 -1

-			|| t->time.tm_year != -1)
+	/* chip only can handle alarms up to one week in the future*/
+	status = rs5c372_rtc_read_time(dev, &rtc_tm);
+	if (status)
+		return status;
+	rtc_secs = rtc_tm_to_time64(&rtc_tm);
+	alarm_secs = rtc_tm_to_time64(&t->time);
+	if (alarm_secs >= rtc_secs + 7 * 86400) {
+		dev_err(dev, "%s: alarm maximum is one week in the future (%d)\n",
+			__func__, status);

Please avoid adding an error message. It will not be read anyway.

drivers/rtc/rtc-rs5c372.c static int rs5c_set_alarm(struct device *dev, struct rtc_wkalrm *t) · line 458

Quoted 1 line

drivers/rtc/rtc-rs5c372.c +0 -0
 		return -EINVAL;

Maybe it is a good opportunity to change to -ERANGE?

drivers/rtc/rtc-rs5c372.c static int rs5c_set_alarm(struct device *dev, struct rtc_wkalrm *t) · line 482

Quoted 12 lines

drivers/rtc/rtc-rs5c372.c +4 -1

+	}
 
 	/* REVISIT: round up tm_sec */

@@ -443,7 +477,9 @@ static int rs5c_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	/* set alarm */
 	buf[0] = bin2bcd(t->time.tm_min);
 	buf[1] = rs5c_hr2reg(rs5c, t->time.tm_hour);
-	buf[2] = 0x7f;	/* any/all days */
+	/* each bit is the day of the week, 0x7f means all days */
+	buf[2] = (t->time.tm_wday >= 0 && t->time.tm_wday < 7) ?
+		  BIT(t->time.tm_wday) : 0x7f;

Here, you also need to calculate buf[2] from t->time.tm_mday instead of relying on t->time.tm_wday.

Signature · 3 lines

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

Daniel González Cabanelas

author

2021-03-08 10:59 UTC #2 lore raw

Hi Alexandre,

Quoted 15 lines · Alexandre Belloni

Hello,
Quoted 11 lines · Daniel González Cabanelas
The Ricoh R2221x, R2223x, RS5C372, RV5C387A chips can handle 1 week alarms.
Read the "wday" alarm register and convert it to a date to support up 1 week in our driver.
Signed-off-by Daniel González Cabanelas <[email protected]>
---
 drivers/rtc/rtc-rs5c372.c | 48 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 6 deletions(-)

Quoted 14 lines

drivers/rtc/rtc-rs5c372.c static int rs5c_read_alarm(struct device *dev, struct rtc_wkalrm *t) · line 397

Quoted 11 lines

drivers/rtc/rtc-rs5c372.c +2 -1

index 3bd6eaa0d..94b778c6e 100644
@@ -393,7 +393,9 @@ static int rs5c_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
      struct i2c_client       *client = to_i2c_client(dev);
      struct rs5c372          *rs5c = i2c_get_clientdata(client);
-     int                     status;
+     int                     status, wday_offs;
+     struct rtc_time         rtc;

You have a few spaces before tabs, please fix them.

Quoted 14 lines

drivers/rtc/rtc-rs5c372.c +7 -0

+     unsigned long           alarm_secs;
 
      status = rs5c_get_regs(rs5c);
      if (status < 0)

@@ -403,6 +405,30 @@ static int rs5c_read_alarm(struct device *dev, struct rtc_wkalrm *t)
      t->time.tm_sec = 0;
      t->time.tm_min = bcd2bin(rs5c->regs[RS5C_REG_ALARM_A_MIN] & 0x7f);
      t->time.tm_hour = rs5c_reg2hr(rs5c, rs5c->regs[RS5C_REG_ALARM_A_HOURS]);
+     t->time.tm_wday = ffs(rs5c->regs[RS5C_REG_ALARM_A_WDAY] & 0x7f) - 1;
+
+     /* determine the day, month and year based on alarm wday, taking as a
+      * reference the current time from the rtc
+      */
+     status = rs5c372_rtc_read_time(dev, &rtc);

Quoted 8 lines

drivers/rtc/rtc-rs5c372.c static int rs5c_read_alarm(struct device *dev, struct rtc_wkalrm *t) · line 417
Quoted 4 lines
drivers/rtc/rtc-rs5c372.c +4 -0
+     if (status < 0)
+             return status;
+
+     wday_offs = t->time.tm_wday - rtc.tm_wday;
Note that you can not rely on tm_wday being set correctly. The core will not (currently) enforce that and most tools jut pass a bogus value or 0. So you need to calculate wday in rs5c372_rtc_set_time.

will this code be enough for the set_time function? -------------snip-------------
int wday;
struct rtc_time calc_tm;

/* wday calculate */
rtc_time64_to_tm(rtc_tm_to_time64(tm), &calc_tm);
wday = calc_tm.tm_wday;

buf[3] = bin2bcd(wday);
-------------snip-------------

Quoted 43 lines

I'm currently working on a way for the drivers to ask the core to ensure wday is correct.

Quoted 27 lines

drivers/rtc/rtc-rs5c372.c +16 -3

+     alarm_secs = mktime64(rtc.tm_year + 1900,
+                           rtc.tm_mon + 1,
+                           rtc.tm_mday + wday_offs,
+                           t->time.tm_hour,
+                           t->time.tm_min,
+                           t->time.tm_sec);
+
+     if (wday_offs < 0 || (wday_offs == 0 &&
+                           (t->time.tm_hour < rtc.tm_hour ||
+                            (t->time.tm_hour == rtc.tm_hour &&
+                             t->time.tm_min <= rtc.tm_min))))
+             alarm_secs += 7 * 86400;
+
+     rtc_time64_to_tm(alarm_secs, &t->time);
 
      /* ... and status */
      t->enabled = !!(rs5c->regs[RS5C_REG_CTRL1] & RS5C_CTRL1_AALE);

@@ -417,12 +443,20 @@ static int rs5c_set_alarm(struct device *dev, struct rtc_wkalrm *t)
      struct rs5c372          *rs5c = i2c_get_clientdata(client);
      int                     status, addr, i;
      unsigned char           buf[3];
+     struct rtc_time         rtc_tm;
+     unsigned long           rtc_secs, alarm_secs;
 
-     /* only handle up to 24 hours in the future, like RTC_ALM_SET */
-     if (t->time.tm_mday != -1
-                     || t->time.tm_mon != -1

drivers/rtc/rtc-rs5c372.c static int rs5c_set_alarm(struct device *dev, struct rtc_wkalrm *t) · line 457

Quoted 10 lines

drivers/rtc/rtc-rs5c372.c +9 -1

-                     || t->time.tm_year != -1)
+     /* chip only can handle alarms up to one week in the future*/
+     status = rs5c372_rtc_read_time(dev, &rtc_tm);
+     if (status)
+             return status;
+     rtc_secs = rtc_tm_to_time64(&rtc_tm);
+     alarm_secs = rtc_tm_to_time64(&t->time);
+     if (alarm_secs >= rtc_secs + 7 * 86400) {
+             dev_err(dev, "%s: alarm maximum is one week in the future (%d)\n",
+                     __func__, status);

Please avoid adding an error message. It will not be read anyway.

Quoted 4 lines

drivers/rtc/rtc-rs5c372.c static int rs5c_set_alarm(struct device *dev, struct rtc_wkalrm *t) · line 458
Quoted 1 line
drivers/rtc/rtc-rs5c372.c +0 -0
              return -EINVAL;
Maybe it is a good opportunity to change to -ERANGE?

Quoted 4 lines

Quoted 4 lines
drivers/rtc/rtc-rs5c372.c +1 -0
+     }
 
      /* REVISIT: round up tm_sec */
 

Quoted 11 lines

drivers/rtc/rtc-rs5c372.c static int rs5c_set_alarm(struct device *dev, struct rtc_wkalrm *t) · line 482
Quoted 8 lines
drivers/rtc/rtc-rs5c372.c +3 -1
@@ -443,7 +477,9 @@ static int rs5c_set_alarm(struct device *dev, struct rtc_wkalrm *t)
      /* set alarm */
      buf[0] = bin2bcd(t->time.tm_min);
      buf[1] = rs5c_hr2reg(rs5c, t->time.tm_hour);
-     buf[2] = 0x7f;  /* any/all days */
+     /* each bit is the day of the week, 0x7f means all days */
+     buf[2] = (t->time.tm_wday >= 0 && t->time.tm_wday < 7) ?
+               BIT(t->time.tm_wday) : 0x7f;
Here, you also need to calculate buf[2] from t->time.tm_mday instead of relying on t->time.tm_wday.

I can't se how to calculate the wday using t->time.tm_mday. Again can I use this?:
-------------snip------------- rtc_time64_to_tm(rtc_tm_to_time64(tm), &calc_tm);
wday = calc_tm.tm_wday;
-------------snip-------------

Regards
Daniel

Quoted 6 lines

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