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

[Keerthy <a0393675@ti.com>]

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
>