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@xxxxxx>
> > ---
> >   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