From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754678AbcFGJaY (ORCPT ); Tue, 7 Jun 2016 05:30:24 -0400 Received: from bear.ext.ti.com ([198.47.19.11]:50809 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754643AbcFGJaU (ORCPT ); Tue, 7 Jun 2016 05:30:20 -0400 Subject: Re: [PATCH] rtc: ds1307: Fix relying on reset value for weekday To: Alexandre Belloni , Keerthy References: <1464778147-22075-1-git-send-email-j-keerthy@ti.com> <20160601121802.GF4249@piout.net> <574ED6D1.70800@ti.com> CC: , , , From: Keerthy Message-ID: <575693E1.2090302@ti.com> Date: Tue, 7 Jun 2016 14:59:05 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <574ED6D1.70800@ti.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> --- >>> 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 >>