[PATCH] rtc: ds1307: Fix relying on reset value for weekday

[PATCH] rtc: ds1307: Fix relying on reset value for weekday

[Keerthy]

The reset value of weekday is 0x1. This is wrong since
the reset values of the day/month/year make up to Jan 1 2001.
When computed weekday comes out to be Monday. On a scale
of 1-7(Sunday - Saturday) it should be 0x2. So we should not
be relying on the reset value.

Hence compute the wday using the current date/month/year values.
Check if reset wday is any different from the computed wday,
If different then set the wday which we computed using
date/month/year values.

Document Referred:
http://ww1.microchip.com/downloads/en/DeviceDoc/20002266F.pdf

Fixes: 1d1945d261a2af "drivers/rtc/rtc-ds1307.c: add alarm support for mcp7941x chips"
Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 821d9c0..d9045cc 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -602,6 +602,8 @@ static const struct rtc_class_ops ds13xx_rtc_ops = {
  * Alarm support for mcp794xx devices.
  */
 
+#define MCP794XX_REG_WEEKDAY		0x3
+#define MCP794XX_REG_WEEKDAY_WDAY_MASK	0x7
 #define MCP794XX_REG_CONTROL		0x07
 #	define MCP794XX_BIT_ALM0_EN	0x10
 #	define MCP794XX_BIT_ALM1_EN	0x20
@@ -1231,13 +1233,16 @@ static int ds1307_probe(struct i2c_client *client,
 {
 	struct ds1307		*ds1307;
 	int			err = -ENODEV;
-	int			tmp;
+	int			tmp, wday;
 	struct chip_desc	*chip = &chips[id->driver_data];
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
 	bool			want_irq = false;
 	bool			ds1307_can_wakeup_device = false;
 	unsigned char		*buf;
 	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct rtc_time		tm;
+	unsigned long		timestamp;
+
 	irq_handler_t	irq_handler = ds1307_irq;
 
 	static const int	bbsqi_bitpos[] = {
@@ -1526,6 +1531,27 @@ read_rtc:
 				bin2bcd(tmp));
 	}
 
+	/*
+	 * Some IPs have weekday reset value = 0x1 which might not correct
+	 * hence compute the wday using the current date/month/year values
+	 */
+	ds1307_get_time(&client->dev, &tm);
+	wday = tm.tm_wday;
+	timestamp = rtc_tm_to_time64(&tm);
+	rtc_time64_to_tm(timestamp, &tm);
+
+	/*
+	 * Check if reset wday is different from the computed wday
+	 * If different then set the wday which we computed using
+	 * timestamp
+	 */
+	if (wday != tm.tm_wday) {
+		wday = i2c_smbus_read_byte_data(client, MCP794XX_REG_WEEKDAY);
+		wday = wday & ~MCP794XX_REG_WEEKDAY_WDAY_MASK;
+		wday = wday | (tm.tm_wday + 1);
+		i2c_smbus_write_byte_data(client, MCP794XX_REG_WEEKDAY, wday);
+	}
+
 	if (want_irq) {
 		device_set_wakeup_capable(&client->dev, true);
 		set_bit(HAS_ALARM, &ds1307->flags);
-- 
1.9.1

Re: [PATCH] rtc: ds1307: Fix relying on reset value for weekday

[Alexandre Belloni]

Hi,

On 01/06/2016 at 16:19:07 +0530, Keerthy wrote :
> The reset value of weekday is 0x1. This is wrong since
> the reset values of the day/month/year make up to Jan 1 2001.
> When computed weekday comes out to be Monday. On a scale
> of 1-7(Sunday - Saturday) it should be 0x2. So we should not
> be relying on the reset value.
> 

Hum, what are the chances that the reset value is actually the correct
date/time?
Won't that be corrected after the first call to set_time? Until then,
the date is not correct so, do we care anyway?

Maybe I'm missing something here.

> Hence compute the wday using the current date/month/year values.
> Check if reset wday is any different from the computed wday,
> If different then set the wday which we computed using
> date/month/year values.
> 
> Document Referred:
> http://ww1.microchip.com/downloads/en/DeviceDoc/20002266F.pdf
> 
> Fixes: 1d1945d261a2af "drivers/rtc/rtc-ds1307.c: add alarm support for mcp7941x chips"
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 821d9c0..d9045cc 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -602,6 +602,8 @@ static const struct rtc_class_ops ds13xx_rtc_ops = {
>   * Alarm support for mcp794xx devices.
>   */
>  
> +#define MCP794XX_REG_WEEKDAY		0x3
> +#define MCP794XX_REG_WEEKDAY_WDAY_MASK	0x7
>  #define MCP794XX_REG_CONTROL		0x07
>  #	define MCP794XX_BIT_ALM0_EN	0x10
>  #	define MCP794XX_BIT_ALM1_EN	0x20
> @@ -1231,13 +1233,16 @@ static int ds1307_probe(struct i2c_client *client,
>  {
>  	struct ds1307		*ds1307;
>  	int			err = -ENODEV;
> -	int			tmp;
> +	int			tmp, wday;
>  	struct chip_desc	*chip = &chips[id->driver_data];
>  	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
>  	bool			want_irq = false;
>  	bool			ds1307_can_wakeup_device = false;
>  	unsigned char		*buf;
>  	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
> +	struct rtc_time		tm;
> +	unsigned long		timestamp;
> +
>  	irq_handler_t	irq_handler = ds1307_irq;
>  
>  	static const int	bbsqi_bitpos[] = {
> @@ -1526,6 +1531,27 @@ read_rtc:
>  				bin2bcd(tmp));
>  	}
>  
> +	/*
> +	 * Some IPs have weekday reset value = 0x1 which might not correct
> +	 * hence compute the wday using the current date/month/year values
> +	 */
> +	ds1307_get_time(&client->dev, &tm);
> +	wday = tm.tm_wday;
> +	timestamp = rtc_tm_to_time64(&tm);
> +	rtc_time64_to_tm(timestamp, &tm);
> +
> +	/*
> +	 * Check if reset wday is different from the computed wday
> +	 * If different then set the wday which we computed using
> +	 * timestamp
> +	 */
> +	if (wday != tm.tm_wday) {
> +		wday = i2c_smbus_read_byte_data(client, MCP794XX_REG_WEEKDAY);
> +		wday = wday & ~MCP794XX_REG_WEEKDAY_WDAY_MASK;
> +		wday = wday | (tm.tm_wday + 1);
> +		i2c_smbus_write_byte_data(client, MCP794XX_REG_WEEKDAY, wday);
> +	}
> +
>  	if (want_irq) {
>  		device_set_wakeup_capable(&client->dev, true);
>  		set_bit(HAS_ALARM, &ds1307->flags);
> -- 
> 1.9.1

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

Re: [PATCH] rtc: ds1307: Fix relying on reset value for weekday

[Keerthy]

On Wednesday 01 June 2016 05:48 PM, Alexandre Belloni wrote:
> Hi,
>
> On 01/06/2016 at 16:19:07 +0530, Keerthy wrote :
>> The reset value of weekday is 0x1. This is wrong since
>> the reset values of the day/month/year make up to Jan 1 2001.
>> When computed weekday comes out to be Monday. On a scale
>> of 1-7(Sunday - Saturday) it should be 0x2. So we should not
>> be relying on the reset value.
>>
>
> Hum, what are the chances that the reset value is actually the correct
> date/time?
> Won't that be corrected after the first call to set_time? Until then,
> the date is not correct so, do we care anyway?

Yes if an alarm is programmed without set_time.

ex: rtcwake -d /dev/rtc0 -s 5

Even the basic rtctest under tools/testing/selftests/timers/rtctest.c fails
as the wday is wrong and no alarm fires.

Instead of relying on some one to call set_time before programming alarm 
its better to fix the wday to reflect the current day right?


>
> Maybe I'm missing something here.
>
>> Hence compute the wday using the current date/month/year values.
>> Check if reset wday is any different from the computed wday,
>> If different then set the wday which we computed using
>> date/month/year values.
>>
>> Document Referred:
>> http://ww1.microchip.com/downloads/en/DeviceDoc/20002266F.pdf
>>
>> Fixes: 1d1945d261a2af "drivers/rtc/rtc-ds1307.c: add alarm support for mcp7941x chips"
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>   drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
>> index 821d9c0..d9045cc 100644
>> --- a/drivers/rtc/rtc-ds1307.c
>> +++ b/drivers/rtc/rtc-ds1307.c
>> @@ -602,6 +602,8 @@ static const struct rtc_class_ops ds13xx_rtc_ops = {
>>    * Alarm support for mcp794xx devices.
>>    */
>>
>> +#define MCP794XX_REG_WEEKDAY		0x3
>> +#define MCP794XX_REG_WEEKDAY_WDAY_MASK	0x7
>>   #define MCP794XX_REG_CONTROL		0x07
>>   #	define MCP794XX_BIT_ALM0_EN	0x10
>>   #	define MCP794XX_BIT_ALM1_EN	0x20
>> @@ -1231,13 +1233,16 @@ static int ds1307_probe(struct i2c_client *client,
>>   {
>>   	struct ds1307		*ds1307;
>>   	int			err = -ENODEV;
>> -	int			tmp;
>> +	int			tmp, wday;
>>   	struct chip_desc	*chip = &chips[id->driver_data];
>>   	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
>>   	bool			want_irq = false;
>>   	bool			ds1307_can_wakeup_device = false;
>>   	unsigned char		*buf;
>>   	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
>> +	struct rtc_time		tm;
>> +	unsigned long		timestamp;
>> +
>>   	irq_handler_t	irq_handler = ds1307_irq;
>>
>>   	static const int	bbsqi_bitpos[] = {
>> @@ -1526,6 +1531,27 @@ read_rtc:
>>   				bin2bcd(tmp));
>>   	}
>>
>> +	/*
>> +	 * Some IPs have weekday reset value = 0x1 which might not correct
>> +	 * hence compute the wday using the current date/month/year values
>> +	 */
>> +	ds1307_get_time(&client->dev, &tm);
>> +	wday = tm.tm_wday;
>> +	timestamp = rtc_tm_to_time64(&tm);
>> +	rtc_time64_to_tm(timestamp, &tm);
>> +
>> +	/*
>> +	 * Check if reset wday is different from the computed wday
>> +	 * If different then set the wday which we computed using
>> +	 * timestamp
>> +	 */
>> +	if (wday != tm.tm_wday) {
>> +		wday = i2c_smbus_read_byte_data(client, MCP794XX_REG_WEEKDAY);
>> +		wday = wday & ~MCP794XX_REG_WEEKDAY_WDAY_MASK;
>> +		wday = wday | (tm.tm_wday + 1);
>> +		i2c_smbus_write_byte_data(client, MCP794XX_REG_WEEKDAY, wday);
>> +	}
>> +
>>   	if (want_irq) {
>>   		device_set_wakeup_capable(&client->dev, true);
>>   		set_bit(HAS_ALARM, &ds1307->flags);
>> --
>> 1.9.1
>

Re: [PATCH] rtc: ds1307: Fix relying on reset value for weekday

[Keerthy]

Hi Alexandre,

On Wednesday 01 June 2016 06:06 PM, Keerthy wrote:
>
>
> On Wednesday 01 June 2016 05:48 PM, Alexandre Belloni wrote:
>> Hi,
>>
>> On 01/06/2016 at 16:19:07 +0530, Keerthy wrote :
>>> The reset value of weekday is 0x1. This is wrong since
>>> the reset values of the day/month/year make up to Jan 1 2001.
>>> When computed weekday comes out to be Monday. On a scale
>>> of 1-7(Sunday - Saturday) it should be 0x2. So we should not
>>> be relying on the reset value.
>>>
>>
>> Hum, what are the chances that the reset value is actually the correct
>> date/time?
>> Won't that be corrected after the first call to set_time? Until then,
>> the date is not correct so, do we care anyway?
>
> Yes if an alarm is programmed without set_time.
>
> ex: rtcwake -d /dev/rtc0 -s 5
>
> Even the basic rtctest under tools/testing/selftests/timers/rtctest.c fails
> as the wday is wrong and no alarm fires.
>
> Instead of relying on some one to call set_time before programming alarm
> its better to fix the wday to reflect the current day right?
>
>
>>
>> Maybe I'm missing something here.

Let me know if you feel this is a valid fix.

Regards,
Keerthy
>>
>>> Hence compute the wday using the current date/month/year values.
>>> Check if reset wday is any different from the computed wday,
>>> If different then set the wday which we computed using
>>> date/month/year values.
>>>
>>> Document Referred:
>>> http://ww1.microchip.com/downloads/en/DeviceDoc/20002266F.pdf
>>>
>>> Fixes: 1d1945d261a2af "drivers/rtc/rtc-ds1307.c: add alarm support
>>> for mcp7941x chips"
>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>> ---
>>>   drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++++++++++++-
>>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
>>> index 821d9c0..d9045cc 100644
>>> --- a/drivers/rtc/rtc-ds1307.c
>>> +++ b/drivers/rtc/rtc-ds1307.c
>>> @@ -602,6 +602,8 @@ static const struct rtc_class_ops ds13xx_rtc_ops = {
>>>    * Alarm support for mcp794xx devices.
>>>    */
>>>
>>> +#define MCP794XX_REG_WEEKDAY        0x3
>>> +#define MCP794XX_REG_WEEKDAY_WDAY_MASK    0x7
>>>   #define MCP794XX_REG_CONTROL        0x07
>>>   #    define MCP794XX_BIT_ALM0_EN    0x10
>>>   #    define MCP794XX_BIT_ALM1_EN    0x20
>>> @@ -1231,13 +1233,16 @@ static int ds1307_probe(struct i2c_client
>>> *client,
>>>   {
>>>       struct ds1307        *ds1307;
>>>       int            err = -ENODEV;
>>> -    int            tmp;
>>> +    int            tmp, wday;
>>>       struct chip_desc    *chip = &chips[id->driver_data];
>>>       struct i2c_adapter    *adapter =
>>> to_i2c_adapter(client->dev.parent);
>>>       bool            want_irq = false;
>>>       bool            ds1307_can_wakeup_device = false;
>>>       unsigned char        *buf;
>>>       struct ds1307_platform_data *pdata =
>>> dev_get_platdata(&client->dev);
>>> +    struct rtc_time        tm;
>>> +    unsigned long        timestamp;
>>> +
>>>       irq_handler_t    irq_handler = ds1307_irq;
>>>
>>>       static const int    bbsqi_bitpos[] = {
>>> @@ -1526,6 +1531,27 @@ read_rtc:
>>>                   bin2bcd(tmp));
>>>       }
>>>
>>> +    /*
>>> +     * Some IPs have weekday reset value = 0x1 which might not correct
>>> +     * hence compute the wday using the current date/month/year values
>>> +     */
>>> +    ds1307_get_time(&client->dev, &tm);
>>> +    wday = tm.tm_wday;
>>> +    timestamp = rtc_tm_to_time64(&tm);
>>> +    rtc_time64_to_tm(timestamp, &tm);
>>> +
>>> +    /*
>>> +     * Check if reset wday is different from the computed wday
>>> +     * If different then set the wday which we computed using
>>> +     * timestamp
>>> +     */
>>> +    if (wday != tm.tm_wday) {
>>> +        wday = i2c_smbus_read_byte_data(client, MCP794XX_REG_WEEKDAY);
>>> +        wday = wday & ~MCP794XX_REG_WEEKDAY_WDAY_MASK;
>>> +        wday = wday | (tm.tm_wday + 1);
>>> +        i2c_smbus_write_byte_data(client, MCP794XX_REG_WEEKDAY, wday);
>>> +    }
>>> +
>>>       if (want_irq) {
>>>           device_set_wakeup_capable(&client->dev, true);
>>>           set_bit(HAS_ALARM, &ds1307->flags);
>>> --
>>> 1.9.1
>>

Re: [PATCH] rtc: ds1307: Fix relying on reset value for weekday

[Alexandre Belloni]

On 07/06/2016 at 14:59:05 +0530, Keerthy wrote :
> Hi Alexandre,
> 
> On Wednesday 01 June 2016 06:06 PM, Keerthy wrote:
> > 
> > 
> > On Wednesday 01 June 2016 05:48 PM, Alexandre Belloni wrote:
> > > Hi,
> > > 
> > > On 01/06/2016 at 16:19:07 +0530, Keerthy wrote :
> > > > The reset value of weekday is 0x1. This is wrong since
> > > > the reset values of the day/month/year make up to Jan 1 2001.
> > > > When computed weekday comes out to be Monday. On a scale
> > > > of 1-7(Sunday - Saturday) it should be 0x2. So we should not
> > > > be relying on the reset value.
> > > > 
> > > 
> > > Hum, what are the chances that the reset value is actually the correct
> > > date/time?
> > > Won't that be corrected after the first call to set_time? Until then,
> > > the date is not correct so, do we care anyway?
> > 
> > Yes if an alarm is programmed without set_time.
> > 
> > ex: rtcwake -d /dev/rtc0 -s 5
> > 
> > Even the basic rtctest under tools/testing/selftests/timers/rtctest.c fails
> > as the wday is wrong and no alarm fires.
> > 
> > Instead of relying on some one to call set_time before programming alarm
> > its better to fix the wday to reflect the current day right?
> > 
> > 
> > > 
> > > Maybe I'm missing something here.
> 
> Let me know if you feel this is a valid fix.
> 

Well, this is probably a valid fix and I'll take it as is if I don't
find the time to make something more generic.

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

Re: [PATCH] rtc: ds1307: Fix relying on reset value for weekday

[Keerthy]

On Tuesday 14 June 2016 08:09 PM, Alexandre Belloni wrote:
> On 07/06/2016 at 14:59:05 +0530, Keerthy wrote :
>> Hi Alexandre,
>>
>> On Wednesday 01 June 2016 06:06 PM, Keerthy wrote:
>>>
>>>
>>> On Wednesday 01 June 2016 05:48 PM, Alexandre Belloni wrote:
>>>> Hi,
>>>>
>>>> On 01/06/2016 at 16:19:07 +0530, Keerthy wrote :
>>>>> The reset value of weekday is 0x1. This is wrong since
>>>>> the reset values of the day/month/year make up to Jan 1 2001.
>>>>> When computed weekday comes out to be Monday. On a scale
>>>>> of 1-7(Sunday - Saturday) it should be 0x2. So we should not
>>>>> be relying on the reset value.
>>>>>
>>>>
>>>> Hum, what are the chances that the reset value is actually the correct
>>>> date/time?
>>>> Won't that be corrected after the first call to set_time? Until then,
>>>> the date is not correct so, do we care anyway?
>>>
>>> Yes if an alarm is programmed without set_time.
>>>
>>> ex: rtcwake -d /dev/rtc0 -s 5
>>>
>>> Even the basic rtctest under tools/testing/selftests/timers/rtctest.c fails
>>> as the wday is wrong and no alarm fires.
>>>
>>> Instead of relying on some one to call set_time before programming alarm
>>> its better to fix the wday to reflect the current day right?
>>>
>>>
>>>>
>>>> Maybe I'm missing something here.
>>
>> Let me know if you feel this is a valid fix.
>>
>
> Well, this is probably a valid fix and I'll take it as is if I don't
> find the time to make something more generic.

Okay Thanks.

>

Re: [PATCH] rtc: ds1307: Fix relying on reset value for weekday

[Alexandre Belloni]

On 01/06/2016 at 16:19:07 +0530, Keerthy wrote :
> The reset value of weekday is 0x1. This is wrong since
> the reset values of the day/month/year make up to Jan 1 2001.
> When computed weekday comes out to be Monday. On a scale
> of 1-7(Sunday - Saturday) it should be 0x2. So we should not
> be relying on the reset value.
> 
> Hence compute the wday using the current date/month/year values.
> Check if reset wday is any different from the computed wday,
> If different then set the wday which we computed using
> date/month/year values.
> 
> Document Referred:
> http://ww1.microchip.com/downloads/en/DeviceDoc/20002266F.pdf
> 
> Fixes: 1d1945d261a2af "drivers/rtc/rtc-ds1307.c: add alarm support for mcp7941x chips"
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
Applied, thanks.

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