| Message ID | 2876529.n6mMd1HPca@tool |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] rtc: rs5c372: support alarms up to 1 week | expand |
Hello, On 23/11/2020 11:38:44+0100, Daniel González Cabanelas wrote: > 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 <dgcbueu@gmail.com> > --- > drivers/rtc/rtc-rs5c372.c | 48 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 42 insertions(+), 6 deletions(-) > > diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c > index 3bd6eaa0d..94b778c6e 100644 > --- a/drivers/rtc/rtc-rs5c372.c > +++ b/drivers/rtc/rtc-rs5c372.c > @@ -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. > + 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; 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. > + 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); Please avoid adding an error message. It will not be read anyway. > return -EINVAL; Maybe it is a good opportunity to change to -ERANGE? > + } > > /* 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.
Hi Alexandre, El mar, 12 ene 2021 a las 23:08, Alexandre Belloni (<alexandre.belloni@bootlin.com>) escribió: > > Hello, > > On 23/11/2020 11:38:44+0100, Daniel González Cabanelas wrote: > > 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 <dgcbueu@gmail.com> > > --- > > drivers/rtc/rtc-rs5c372.c | 48 ++++++++++++++++++++++++++++++++++----- > > 1 file changed, 42 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c > > index 3bd6eaa0d..94b778c6e 100644 > > --- a/drivers/rtc/rtc-rs5c372.c > > +++ b/drivers/rtc/rtc-rs5c372.c > > @@ -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. > Ok > > + 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; > > 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------------- > I'm currently working on a way for the drivers to ask the core to ensure > wday is correct. > > > + 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); > > Please avoid adding an error message. It will not be read anyway. > > > return -EINVAL; > > Maybe it is a good opportunity to change to -ERANGE? > Ok > > + } > > > > /* 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. 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 > > > -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c index 3bd6eaa0d..94b778c6e 100644 --- a/drivers/rtc/rtc-rs5c372.c +++ b/drivers/rtc/rtc-rs5c372.c @@ -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);
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 <dgcbueu@gmail.com> --- drivers/rtc/rtc-rs5c372.c | 48 ++++++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 6 deletions(-)