rtc-palmas: correct for bcd year

rtc-palmas: correct for bcd year

[Mark Salyzyn]

Replace bcd2bin and bin2bcd with one that maps years 1970 to 2129
in a pattern that works with the underlying hardware.

The only transition that does not work correctly for this rtc clock
is the transition from 2099 to 2100, it proceeds to 2000. The rtc
clock retains and transitions the year correctly in all other
circumstances.

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
---
 drivers/rtc/rtc-palmas.c | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-palmas.c b/drivers/rtc/rtc-palmas.c
index 7ea2c47..3e9663d 100644
--- a/drivers/rtc/rtc-palmas.c
+++ b/drivers/rtc/rtc-palmas.c
@@ -45,6 +45,42 @@ struct palmas_rtc {
 /* Total number of RTC registers needed to set time*/
 #define PALMAS_NUM_TIME_REGS	(PALMAS_YEARS_REG - PALMAS_SECONDS_REG + 1)
 
+/*
+ * Special bin2bcd mapping to deal with bcd storage of year.
+ *
+ *   0-69                -> 0xD0
+ *  70-99  (1970 - 1999) -> 0xD0 - 0xF9 (correctly rolls to 0x00)
+ * 100-199 (2000 - 2099) -> 0x00 - 0x99 (does not roll to 0xA0 :-( )
+ * 200-229 (2100 - 2129) -> 0xA0 - 0xC9 (really for completeness)
+ * 230-                  -> 0xC9
+ *
+ * Confirmed: the only transition that does not work correctly for this rtc
+ * clock is the transition from 2099 to 2100, it proceeds to 2000. We will
+ * accept this issue since the clock retains and transitions the year correctly
+ * in all other conditions.
+ */
+static unsigned char year_bin2bcd(int val)
+{
+	if (val < 70)
+		return 0xD0;
+	if (val < 100)
+		return bin2bcd(val - 20) | 0x80; /* KISS leverage of bin2bcd */
+	if (val >= 230)
+		return 0xC9;
+	if (val >= 200)
+		return bin2bcd(val - 180) | 0x80;
+	return bin2bcd(val - 100);
+}
+
+static int year_bcd2bin(unsigned char val)
+{
+	if (val >= 0xD0)
+		return bcd2bin(val & 0x7F) + 20;
+	if (val >= 0xA0)
+		return bcd2bin(val & 0x7F) + 180;
+	return bcd2bin(val) + 100;
+}
+
 static int palmas_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	unsigned char rtc_data[PALMAS_NUM_TIME_REGS];
@@ -71,7 +107,7 @@ static int palmas_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	tm->tm_hour = bcd2bin(rtc_data[2]);
 	tm->tm_mday = bcd2bin(rtc_data[3]);
 	tm->tm_mon = bcd2bin(rtc_data[4]) - 1;
-	tm->tm_year = bcd2bin(rtc_data[5]) + 100;
+	tm->tm_year = year_bcd2bin(rtc_data[5]);
 
 	return ret;
 }
@@ -87,7 +123,7 @@ static int palmas_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	rtc_data[2] = bin2bcd(tm->tm_hour);
 	rtc_data[3] = bin2bcd(tm->tm_mday);
 	rtc_data[4] = bin2bcd(tm->tm_mon + 1);
-	rtc_data[5] = bin2bcd(tm->tm_year - 100);
+	rtc_data[5] = year_bin2bcd(tm->tm_year);
 
 	/* Stop RTC while updating the RTC time registers */
 	ret = palmas_update_bits(palmas, PALMAS_RTC_BASE, PALMAS_RTC_CTRL_REG,
@@ -142,7 +178,7 @@ static int palmas_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
 	alm->time.tm_hour = bcd2bin(alarm_data[2]);
 	alm->time.tm_mday = bcd2bin(alarm_data[3]);
 	alm->time.tm_mon = bcd2bin(alarm_data[4]) - 1;
-	alm->time.tm_year = bcd2bin(alarm_data[5]) + 100;
+	alm->time.tm_year = year_bcd2bin(alarm_data[5]);
 
 	ret = palmas_read(palmas, PALMAS_RTC_BASE, PALMAS_RTC_INTERRUPTS_REG,
 			&int_val);
@@ -173,7 +209,7 @@ static int palmas_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
 	alarm_data[2] = bin2bcd(alm->time.tm_hour);
 	alarm_data[3] = bin2bcd(alm->time.tm_mday);
 	alarm_data[4] = bin2bcd(alm->time.tm_mon + 1);
-	alarm_data[5] = bin2bcd(alm->time.tm_year - 100);
+	alarm_data[5] = year_bin2bcd(alm->time.tm_year);
 
 	ret = palmas_bulk_write(palmas, PALMAS_RTC_BASE,
 		PALMAS_ALARM_SECONDS_REG, alarm_data, PALMAS_NUM_TIME_REGS);
-- 
2.6.0.rc2.230.g3dd15c0

Re: rtc-palmas: correct for bcd year

[Alexandre Belloni]

Hi,

On 30/12/2015 at 12:51:45 -0800, Mark Salyzyn wrote :
> Replace bcd2bin and bin2bcd with one that maps years 1970 to 2129
> in a pattern that works with the underlying hardware.
> 
> The only transition that does not work correctly for this rtc clock
> is the transition from 2099 to 2100, it proceeds to 2000. The rtc
> clock retains and transitions the year correctly in all other
> circumstances.
> 

If that transition doesn't work, it is not useful to try to support
dates after 2099. Also, I'm concerned about the leap year handling in
the other case. What is done right now is probably the best however, I
couldn't find the datasheet to confirm.

> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> ---
>  drivers/rtc/rtc-palmas.c | 44 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-palmas.c b/drivers/rtc/rtc-palmas.c
> index 7ea2c47..3e9663d 100644
> --- a/drivers/rtc/rtc-palmas.c
> +++ b/drivers/rtc/rtc-palmas.c
> @@ -45,6 +45,42 @@ struct palmas_rtc {
>  /* Total number of RTC registers needed to set time*/
>  #define PALMAS_NUM_TIME_REGS	(PALMAS_YEARS_REG - PALMAS_SECONDS_REG + 1)
>  
> +/*
> + * Special bin2bcd mapping to deal with bcd storage of year.
> + *
> + *   0-69                -> 0xD0
> + *  70-99  (1970 - 1999) -> 0xD0 - 0xF9 (correctly rolls to 0x00)
> + * 100-199 (2000 - 2099) -> 0x00 - 0x99 (does not roll to 0xA0 :-( )
> + * 200-229 (2100 - 2129) -> 0xA0 - 0xC9 (really for completeness)
> + * 230-                  -> 0xC9
> + *
> + * Confirmed: the only transition that does not work correctly for this rtc
> + * clock is the transition from 2099 to 2100, it proceeds to 2000. We will
> + * accept this issue since the clock retains and transitions the year correctly
> + * in all other conditions.
> + */
> +static unsigned char year_bin2bcd(int val)
> +{
> +	if (val < 70)
> +		return 0xD0;
> +	if (val < 100)
> +		return bin2bcd(val - 20) | 0x80; /* KISS leverage of bin2bcd */
> +	if (val >= 230)
> +		return 0xC9;
> +	if (val >= 200)
> +		return bin2bcd(val - 180) | 0x80;
> +	return bin2bcd(val - 100);
> +}
> +
> +static int year_bcd2bin(unsigned char val)
> +{
> +	if (val >= 0xD0)
> +		return bcd2bin(val & 0x7F) + 20;
> +	if (val >= 0xA0)
> +		return bcd2bin(val & 0x7F) + 180;
> +	return bcd2bin(val) + 100;
> +}
> +
>  static int palmas_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  {
>  	unsigned char rtc_data[PALMAS_NUM_TIME_REGS];
> @@ -71,7 +107,7 @@ static int palmas_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  	tm->tm_hour = bcd2bin(rtc_data[2]);
>  	tm->tm_mday = bcd2bin(rtc_data[3]);
>  	tm->tm_mon = bcd2bin(rtc_data[4]) - 1;
> -	tm->tm_year = bcd2bin(rtc_data[5]) + 100;
> +	tm->tm_year = year_bcd2bin(rtc_data[5]);
>  
>  	return ret;
>  }
> @@ -87,7 +123,7 @@ static int palmas_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	rtc_data[2] = bin2bcd(tm->tm_hour);
>  	rtc_data[3] = bin2bcd(tm->tm_mday);
>  	rtc_data[4] = bin2bcd(tm->tm_mon + 1);
> -	rtc_data[5] = bin2bcd(tm->tm_year - 100);
> +	rtc_data[5] = year_bin2bcd(tm->tm_year);
>  
>  	/* Stop RTC while updating the RTC time registers */
>  	ret = palmas_update_bits(palmas, PALMAS_RTC_BASE, PALMAS_RTC_CTRL_REG,
> @@ -142,7 +178,7 @@ static int palmas_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  	alm->time.tm_hour = bcd2bin(alarm_data[2]);
>  	alm->time.tm_mday = bcd2bin(alarm_data[3]);
>  	alm->time.tm_mon = bcd2bin(alarm_data[4]) - 1;
> -	alm->time.tm_year = bcd2bin(alarm_data[5]) + 100;
> +	alm->time.tm_year = year_bcd2bin(alarm_data[5]);
>  
>  	ret = palmas_read(palmas, PALMAS_RTC_BASE, PALMAS_RTC_INTERRUPTS_REG,
>  			&int_val);
> @@ -173,7 +209,7 @@ static int palmas_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  	alarm_data[2] = bin2bcd(alm->time.tm_hour);
>  	alarm_data[3] = bin2bcd(alm->time.tm_mday);
>  	alarm_data[4] = bin2bcd(alm->time.tm_mon + 1);
> -	alarm_data[5] = bin2bcd(alm->time.tm_year - 100);
> +	alarm_data[5] = year_bin2bcd(alm->time.tm_year);
>  
>  	ret = palmas_bulk_write(palmas, PALMAS_RTC_BASE,
>  		PALMAS_ALARM_SECONDS_REG, alarm_data, PALMAS_NUM_TIME_REGS);
> -- 
> 2.6.0.rc2.230.g3dd15c0
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: rtc-palmas: correct for bcd year

[Mark Salyzyn]

On 01/04/2016 08:18 AM, Alexandre Belloni wrote:
> Hi,
>
> On 30/12/2015 at 12:51:45 -0800, Mark Salyzyn wrote :
>> Replace bcd2bin and bin2bcd with one that maps years 1970 to 2129
>> in a pattern that works with the underlying hardware.
>>
>> The only transition that does not work correctly for this rtc clock
>> is the transition from 2099 to 2100, it proceeds to 2000. The rtc
>> clock retains and transitions the year correctly in all other
>> circumstances.
>>
> If that transition doesn't work, it is not useful to try to support
> dates after 2099. Also, I'm concerned about the leap year handling in
> the other case. What is done right now is probably the best however, I
> couldn't find the datasheet to confirm.
>

As it stands today, if I set the date to 1970, it returns 2066, so this 
is a leap(sic) forward for this one rtc clock.

The advantages of supporting 2099+ for being able to set those years and 
not return garbage like in the 1970 case. The failure to roll over from 
2099-2100 is but a millisecond of failure for an additional 1/3 of 
century of well being, and support code is minor, albeit flawed. Without 
these additional four lines, if something sets the year to 2100, they 
will get a garbage back in return, a small price to pay IMHO.

There are dozens and dozens of other bcd-based rtc clocks that could 
gain from this example (and may not have this issue with 2099 rollover), 
so maybe this year code should be in the library?

I will remove 2099+ support if you continue to consider this rollover 
issue egregious given my rationalization.

Sincerely -- Mark Salyzyn

Re: rtc-palmas: correct for bcd year

[Alexandre Belloni]

On 04/01/2016 at 08:45:54 -0800, Mark Salyzyn wrote :
> On 01/04/2016 08:18 AM, Alexandre Belloni wrote:
> >Hi,
> >
> >On 30/12/2015 at 12:51:45 -0800, Mark Salyzyn wrote :
> >>Replace bcd2bin and bin2bcd with one that maps years 1970 to 2129
> >>in a pattern that works with the underlying hardware.
> >>
> >>The only transition that does not work correctly for this rtc clock
> >>is the transition from 2099 to 2100, it proceeds to 2000. The rtc
> >>clock retains and transitions the year correctly in all other
> >>circumstances.
> >>
> >If that transition doesn't work, it is not useful to try to support
> >dates after 2099. Also, I'm concerned about the leap year handling in
> >the other case. What is done right now is probably the best however, I
> >couldn't find the datasheet to confirm.
> >
> 
> As it stands today, if I set the date to 1970, it returns 2066, so this is a
> leap(sic) forward for this one rtc clock.
> 

I don't think we should care about dates in the past. However, it is
probably useful to return -EINVAL when trying to set a date outside of
its range.

> The advantages of supporting 2099+ for being able to set those years and not
> return garbage like in the 1970 case. The failure to roll over from
> 2099-2100 is but a millisecond of failure for an additional 1/3 of century
> of well being, and support code is minor, albeit flawed. Without these
> additional four lines, if something sets the year to 2100, they will get a
> garbage back in return, a small price to pay IMHO.
> 

If it doesn't go properly from 2099 to 2100 then userspace has no way of
knowing that this happened. Also, what happens between the 28th of
February the first of March when setting 0xA0? My guess is that you
actually have two month of failure.


> There are dozens and dozens of other bcd-based rtc clocks that could gain
> from this example (and may not have this issue with 2099 rollover), so maybe
> this year code should be in the library?

All the ones I know about will fail in 2100. Either because they will not
rollover to 2100 or because they will think 2100 is a leap year.

> 
> I will remove 2099+ support if you continue to consider this rollover issue
> egregious given my rationalization.
> 

I'd say that the proper course of action is to refuse to set dates
before 2000 and after 2100. See
http://patchwork.ozlabs.org/patch/541037/


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: rtc-palmas: correct for bcd year

[Mark Salyzyn]

On 01/04/2016 04:00 PM, Alexandre Belloni wrote:
> I'd say that the proper course of action is to refuse to set dates 
> before 2000 and after 2100. See http://patchwork.ozlabs.org/patch/541037/ 
Got it.

We have an issue though, Android (or rather any embedded) devices must 
continue to function when date is manually set to any value between 1970 
and 2037. The issue here is a fresh device with a recently charged 
battery will _start_ at 1970 until ntp or cell time/date/locale is set 
and the device must continue to function in this vacuum. A device reboot 
should not result in the other calendar values being reset, should the 
year be wrong, as this will result in a bad user experience.

We will have to use a different patch on Android than upstream if dates 
before 2000 are deprecated.

All other factors (rollover, leap) can be corrected by frameworks and 
runtime since the rtc is generally secondary (ie: first rtc driver was 
in 1979, created a daemon to correct the flaws in the hardware clock 
using a cron job)

Sincerely -- Mark Salyzyn

Re: rtc-palmas: correct for bcd year

[Alexandre Belloni]

On 05/01/2016 at 08:08:50 -0800, Mark Salyzyn wrote :
> On 01/04/2016 04:00 PM, Alexandre Belloni wrote:
> > I'd say that the proper course of action is to refuse to set dates
> > before 2000 and after 2100. See
> > http://patchwork.ozlabs.org/patch/541037/
> Got it.
> 
> We have an issue though, Android (or rather any embedded) devices must
> continue to function when date is manually set to any value between 1970 and
> 2037. The issue here is a fresh device with a recently charged battery will
> _start_ at 1970 until ntp or cell time/date/locale is set and the device
> must continue to function in this vacuum. A device reboot should not result
> in the other calendar values being reset, should the year be wrong, as this
> will result in a bad user experience.
> 
> We will have to use a different patch on Android than upstream if dates
> before 2000 are deprecated.
> 
> All other factors (rollover, leap) can be corrected by frameworks and
> runtime since the rtc is generally secondary (ie: first rtc driver was in
> 1979, created a daemon to correct the flaws in the hardware clock using a
> cron job)
> 

Well, I was still thinking about that issue but while handling properly
a gap in the RTC continuity is impossible, it is actually easy to handle
an offset in the RTC by using mktime and then offseting the resulting
time_t. Of course you'll then need to handle leap years and the likes
but you said that was not an issue for you.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com