All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] rtc: ds1307: improve weekday handling
@ 2017-08-28 18:37 Heiner Kallweit
  2017-08-28 19:15 ` Alexandre Belloni
  0 siblings, 1 reply; 5+ messages in thread
From: Heiner Kallweit @ 2017-08-28 18:37 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: linux-rtc

mcp794xx as one chip supported by this driver needs the weekday for
alarm matching. RTC core ignores the weekday so we can't rely on
the values we receive in member tm_wday of struct rtc_time.
Therefore calculate the weekday from date/time when setting the
time and setting the alarm time for mcp794xx.

When having this in place we don't have to check the weekday
at each driver load.
After a chip reset date/time and weekday may be out of sync but
in this case date/time need to be set anyway.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/rtc/rtc-ds1307.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 9d680d36..83b8c997 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -437,6 +437,18 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 	return rtc_valid_tm(t);
 }
 
+/*
+ * Certain chips need the weekday for alarm matching and tm->t_wday
+ * may be not or not properly populated
+ */
+static int ds1307_get_weekday(struct rtc_time *tm)
+{
+	time64_t secs64 = rtc_tm_to_time64(tm);
+	int days = div_s64(secs64, 24 * 60 * 60);
+
+	return (days + 4) % 7 + 1;
+}
+
 static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 {
 	struct ds1307	*ds1307 = dev_get_drvdata(dev);
@@ -465,7 +477,7 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 	regs[DS1307_REG_SECS] = bin2bcd(t->tm_sec);
 	regs[DS1307_REG_MIN] = bin2bcd(t->tm_min);
 	regs[DS1307_REG_HOUR] = bin2bcd(t->tm_hour);
-	regs[DS1307_REG_WDAY] = bin2bcd(t->tm_wday + 1);
+	regs[DS1307_REG_WDAY] = ds1307_get_weekday(t);
 	regs[DS1307_REG_MDAY] = bin2bcd(t->tm_mday);
 	regs[DS1307_REG_MONTH] = bin2bcd(t->tm_mon + 1);
 
@@ -902,7 +914,7 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	regs[3] = bin2bcd(t->time.tm_sec);
 	regs[4] = bin2bcd(t->time.tm_min);
 	regs[5] = bin2bcd(t->time.tm_hour);
-	regs[6] = bin2bcd(t->time.tm_wday + 1);
+	regs[6] = ds1307_get_weekday(&t->time);
 	regs[7] = bin2bcd(t->time.tm_mday);
 	regs[8] = bin2bcd(t->time.tm_mon + 1);
 
@@ -1355,14 +1367,12 @@ static int ds1307_probe(struct i2c_client *client,
 {
 	struct ds1307		*ds1307;
 	int			err = -ENODEV;
-	int			tmp, wday;
+	int			tmp;
 	const struct chip_desc	*chip;
 	bool			want_irq;
 	bool			ds1307_can_wakeup_device = false;
 	unsigned char		regs[8];
 	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
-	struct rtc_time		tm;
-	unsigned long		timestamp;
 	u8			trickle_charger_setup = 0;
 
 	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
@@ -1642,25 +1652,6 @@ static int ds1307_probe(struct i2c_client *client,
 			     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(ds1307->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)
-		regmap_update_bits(ds1307->regmap, MCP794XX_REG_WEEKDAY,
-				   MCP794XX_REG_WEEKDAY_WDAY_MASK,
-				   tm.tm_wday + 1);
-
 	if (want_irq || ds1307_can_wakeup_device) {
 		device_set_wakeup_capable(ds1307->dev, true);
 		set_bit(HAS_ALARM, &ds1307->flags);
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] rtc: ds1307: improve weekday handling
  2017-08-28 18:37 [PATCH] rtc: ds1307: improve weekday handling Heiner Kallweit
@ 2017-08-28 19:15 ` Alexandre Belloni
  2017-08-28 19:58   ` Heiner Kallweit
  0 siblings, 1 reply; 5+ messages in thread
From: Alexandre Belloni @ 2017-08-28 19:15 UTC (permalink / raw)
  To: Heiner Kallweit; +Cc: linux-rtc

On 28/08/2017 at 20:37:21 +0200, Heiner Kallweit wrote:
> mcp794xx as one chip supported by this driver needs the weekday for
> alarm matching. RTC core ignores the weekday so we can't rely on
> the values we receive in member tm_wday of struct rtc_time.
> Therefore calculate the weekday from date/time when setting the
> time and setting the alarm time for mcp794xx.
> 
> When having this in place we don't have to check the weekday
> at each driver load.
> After a chip reset date/time and weekday may be out of sync but
> in this case date/time need to be set anyway.
> 

Nope, the core issue is that you can actually set an alarm in the future
without setting the time beforehand so this as to be fixed in the probe
(this was the issue as reported at the time of the fix).

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/rtc/rtc-ds1307.c | 39 +++++++++++++++------------------------
>  1 file changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 9d680d36..83b8c997 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -437,6 +437,18 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
>  	return rtc_valid_tm(t);
>  }
>  
> +/*
> + * Certain chips need the weekday for alarm matching and tm->t_wday
> + * may be not or not properly populated
> + */
> +static int ds1307_get_weekday(struct rtc_time *tm)
> +{
> +	time64_t secs64 = rtc_tm_to_time64(tm);
> +	int days = div_s64(secs64, 24 * 60 * 60);
> +
> +	return (days + 4) % 7 + 1;
> +}
> +
>  static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>  {
>  	struct ds1307	*ds1307 = dev_get_drvdata(dev);
> @@ -465,7 +477,7 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>  	regs[DS1307_REG_SECS] = bin2bcd(t->tm_sec);
>  	regs[DS1307_REG_MIN] = bin2bcd(t->tm_min);
>  	regs[DS1307_REG_HOUR] = bin2bcd(t->tm_hour);
> -	regs[DS1307_REG_WDAY] = bin2bcd(t->tm_wday + 1);
> +	regs[DS1307_REG_WDAY] = ds1307_get_weekday(t);
>  	regs[DS1307_REG_MDAY] = bin2bcd(t->tm_mday);
>  	regs[DS1307_REG_MONTH] = bin2bcd(t->tm_mon + 1);
>  
> @@ -902,7 +914,7 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>  	regs[3] = bin2bcd(t->time.tm_sec);
>  	regs[4] = bin2bcd(t->time.tm_min);
>  	regs[5] = bin2bcd(t->time.tm_hour);
> -	regs[6] = bin2bcd(t->time.tm_wday + 1);
> +	regs[6] = ds1307_get_weekday(&t->time);
>  	regs[7] = bin2bcd(t->time.tm_mday);
>  	regs[8] = bin2bcd(t->time.tm_mon + 1);
>  
> @@ -1355,14 +1367,12 @@ static int ds1307_probe(struct i2c_client *client,
>  {
>  	struct ds1307		*ds1307;
>  	int			err = -ENODEV;
> -	int			tmp, wday;
> +	int			tmp;
>  	const struct chip_desc	*chip;
>  	bool			want_irq;
>  	bool			ds1307_can_wakeup_device = false;
>  	unsigned char		regs[8];
>  	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
> -	struct rtc_time		tm;
> -	unsigned long		timestamp;
>  	u8			trickle_charger_setup = 0;
>  
>  	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
> @@ -1642,25 +1652,6 @@ static int ds1307_probe(struct i2c_client *client,
>  			     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(ds1307->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)
> -		regmap_update_bits(ds1307->regmap, MCP794XX_REG_WEEKDAY,
> -				   MCP794XX_REG_WEEKDAY_WDAY_MASK,
> -				   tm.tm_wday + 1);
> -
>  	if (want_irq || ds1307_can_wakeup_device) {
>  		device_set_wakeup_capable(ds1307->dev, true);
>  		set_bit(HAS_ALARM, &ds1307->flags);
> -- 
> 2.14.1
> 

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rtc: ds1307: improve weekday handling
  2017-08-28 19:15 ` Alexandre Belloni
@ 2017-08-28 19:58   ` Heiner Kallweit
  2017-08-28 20:12     ` Heiner Kallweit
  2017-08-28 20:25     ` Alexandre Belloni
  0 siblings, 2 replies; 5+ messages in thread
From: Heiner Kallweit @ 2017-08-28 19:58 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: linux-rtc

Am 28.08.2017 um 21:15 schrieb Alexandre Belloni:
> On 28/08/2017 at 20:37:21 +0200, Heiner Kallweit wrote:
>> mcp794xx as one chip supported by this driver needs the weekday for
>> alarm matching. RTC core ignores the weekday so we can't rely on
>> the values we receive in member tm_wday of struct rtc_time.
>> Therefore calculate the weekday from date/time when setting the
>> time and setting the alarm time for mcp794xx.
>>
>> When having this in place we don't have to check the weekday
>> at each driver load.
>> After a chip reset date/time and weekday may be out of sync but
>> in this case date/time need to be set anyway.
>>
> 
> Nope, the core issue is that you can actually set an alarm in the future
> without setting the time beforehand so this as to be fixed in the probe
> (this was the issue as reported at the time of the fix).
> 
Ah, found the original thread where this was discussed.
Still I don't really understand the problem. mcp794xx matches based on
secs, min, hour, wday, mday, month. When I use "rtcwake -s 5" I expect
the alarm to trigger 5 secs from now. How can this ever work if the
RTC has random date/time (wday being valid or not)?

I'd tend to say that if we know the RTC time is incorrect we shouldn't
allow setting an alarm.
At least I wouldn't dare to program a bomb explosion time if I stand in
front of it and know that the bomb's clock has a random value ;)

>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/rtc/rtc-ds1307.c | 39 +++++++++++++++------------------------
>>  1 file changed, 15 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
>> index 9d680d36..83b8c997 100644
>> --- a/drivers/rtc/rtc-ds1307.c
>> +++ b/drivers/rtc/rtc-ds1307.c
>> @@ -437,6 +437,18 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
>>  	return rtc_valid_tm(t);
>>  }
>>  
>> +/*
>> + * Certain chips need the weekday for alarm matching and tm->t_wday
>> + * may be not or not properly populated
>> + */
>> +static int ds1307_get_weekday(struct rtc_time *tm)
>> +{
>> +	time64_t secs64 = rtc_tm_to_time64(tm);
>> +	int days = div_s64(secs64, 24 * 60 * 60);
>> +
>> +	return (days + 4) % 7 + 1;
>> +}
>> +
>>  static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>>  {
>>  	struct ds1307	*ds1307 = dev_get_drvdata(dev);
>> @@ -465,7 +477,7 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>>  	regs[DS1307_REG_SECS] = bin2bcd(t->tm_sec);
>>  	regs[DS1307_REG_MIN] = bin2bcd(t->tm_min);
>>  	regs[DS1307_REG_HOUR] = bin2bcd(t->tm_hour);
>> -	regs[DS1307_REG_WDAY] = bin2bcd(t->tm_wday + 1);
>> +	regs[DS1307_REG_WDAY] = ds1307_get_weekday(t);
>>  	regs[DS1307_REG_MDAY] = bin2bcd(t->tm_mday);
>>  	regs[DS1307_REG_MONTH] = bin2bcd(t->tm_mon + 1);
>>  
>> @@ -902,7 +914,7 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>>  	regs[3] = bin2bcd(t->time.tm_sec);
>>  	regs[4] = bin2bcd(t->time.tm_min);
>>  	regs[5] = bin2bcd(t->time.tm_hour);
>> -	regs[6] = bin2bcd(t->time.tm_wday + 1);
>> +	regs[6] = ds1307_get_weekday(&t->time);
>>  	regs[7] = bin2bcd(t->time.tm_mday);
>>  	regs[8] = bin2bcd(t->time.tm_mon + 1);
>>  
>> @@ -1355,14 +1367,12 @@ static int ds1307_probe(struct i2c_client *client,
>>  {
>>  	struct ds1307		*ds1307;
>>  	int			err = -ENODEV;
>> -	int			tmp, wday;
>> +	int			tmp;
>>  	const struct chip_desc	*chip;
>>  	bool			want_irq;
>>  	bool			ds1307_can_wakeup_device = false;
>>  	unsigned char		regs[8];
>>  	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
>> -	struct rtc_time		tm;
>> -	unsigned long		timestamp;
>>  	u8			trickle_charger_setup = 0;
>>  
>>  	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
>> @@ -1642,25 +1652,6 @@ static int ds1307_probe(struct i2c_client *client,
>>  			     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(ds1307->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)
>> -		regmap_update_bits(ds1307->regmap, MCP794XX_REG_WEEKDAY,
>> -				   MCP794XX_REG_WEEKDAY_WDAY_MASK,
>> -				   tm.tm_wday + 1);
>> -
>>  	if (want_irq || ds1307_can_wakeup_device) {
>>  		device_set_wakeup_capable(ds1307->dev, true);
>>  		set_bit(HAS_ALARM, &ds1307->flags);
>> -- 
>> 2.14.1
>>
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rtc: ds1307: improve weekday handling
  2017-08-28 19:58   ` Heiner Kallweit
@ 2017-08-28 20:12     ` Heiner Kallweit
  2017-08-28 20:25     ` Alexandre Belloni
  1 sibling, 0 replies; 5+ messages in thread
From: Heiner Kallweit @ 2017-08-28 20:12 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: linux-rtc

Am 28.08.2017 um 21:58 schrieb Heiner Kallweit:
> Am 28.08.2017 um 21:15 schrieb Alexandre Belloni:
>> On 28/08/2017 at 20:37:21 +0200, Heiner Kallweit wrote:
>>> mcp794xx as one chip supported by this driver needs the weekday for
>>> alarm matching. RTC core ignores the weekday so we can't rely on
>>> the values we receive in member tm_wday of struct rtc_time.
>>> Therefore calculate the weekday from date/time when setting the
>>> time and setting the alarm time for mcp794xx.
>>>
>>> When having this in place we don't have to check the weekday
>>> at each driver load.
>>> After a chip reset date/time and weekday may be out of sync but
>>> in this case date/time need to be set anyway.
>>>
>>
>> Nope, the core issue is that you can actually set an alarm in the future
>> without setting the time beforehand so this as to be fixed in the probe
>> (this was the issue as reported at the time of the fix).
>>
> Ah, found the original thread where this was discussed.
> Still I don't really understand the problem. mcp794xx matches based on
> secs, min, hour, wday, mday, month. When I use "rtcwake -s 5" I expect
> the alarm to trigger 5 secs from now. How can this ever work if the
> RTC has random date/time (wday being valid or not)?
> 
OK, if "rtcwake -s 5" sets the alarm relative to the rtc time, not the
sys time then it would work. But allowing to use the rtc when it has
a random date / time still seems to be a weird use case to me.

> I'd tend to say that if we know the RTC time is incorrect we shouldn't
> allow setting an alarm.
> At least I wouldn't dare to program a bomb explosion time if I stand in
> front of it and know that the bomb's clock has a random value ;)
> 
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/rtc/rtc-ds1307.c | 39 +++++++++++++++------------------------
>>>  1 file changed, 15 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
>>> index 9d680d36..83b8c997 100644
>>> --- a/drivers/rtc/rtc-ds1307.c
>>> +++ b/drivers/rtc/rtc-ds1307.c
>>> @@ -437,6 +437,18 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
>>>  	return rtc_valid_tm(t);
>>>  }
>>>  
>>> +/*
>>> + * Certain chips need the weekday for alarm matching and tm->t_wday
>>> + * may be not or not properly populated
>>> + */
>>> +static int ds1307_get_weekday(struct rtc_time *tm)
>>> +{
>>> +	time64_t secs64 = rtc_tm_to_time64(tm);
>>> +	int days = div_s64(secs64, 24 * 60 * 60);
>>> +
>>> +	return (days + 4) % 7 + 1;
>>> +}
>>> +
>>>  static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>>>  {
>>>  	struct ds1307	*ds1307 = dev_get_drvdata(dev);
>>> @@ -465,7 +477,7 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>>>  	regs[DS1307_REG_SECS] = bin2bcd(t->tm_sec);
>>>  	regs[DS1307_REG_MIN] = bin2bcd(t->tm_min);
>>>  	regs[DS1307_REG_HOUR] = bin2bcd(t->tm_hour);
>>> -	regs[DS1307_REG_WDAY] = bin2bcd(t->tm_wday + 1);
>>> +	regs[DS1307_REG_WDAY] = ds1307_get_weekday(t);
>>>  	regs[DS1307_REG_MDAY] = bin2bcd(t->tm_mday);
>>>  	regs[DS1307_REG_MONTH] = bin2bcd(t->tm_mon + 1);
>>>  
>>> @@ -902,7 +914,7 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>>>  	regs[3] = bin2bcd(t->time.tm_sec);
>>>  	regs[4] = bin2bcd(t->time.tm_min);
>>>  	regs[5] = bin2bcd(t->time.tm_hour);
>>> -	regs[6] = bin2bcd(t->time.tm_wday + 1);
>>> +	regs[6] = ds1307_get_weekday(&t->time);
>>>  	regs[7] = bin2bcd(t->time.tm_mday);
>>>  	regs[8] = bin2bcd(t->time.tm_mon + 1);
>>>  
>>> @@ -1355,14 +1367,12 @@ static int ds1307_probe(struct i2c_client *client,
>>>  {
>>>  	struct ds1307		*ds1307;
>>>  	int			err = -ENODEV;
>>> -	int			tmp, wday;
>>> +	int			tmp;
>>>  	const struct chip_desc	*chip;
>>>  	bool			want_irq;
>>>  	bool			ds1307_can_wakeup_device = false;
>>>  	unsigned char		regs[8];
>>>  	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
>>> -	struct rtc_time		tm;
>>> -	unsigned long		timestamp;
>>>  	u8			trickle_charger_setup = 0;
>>>  
>>>  	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
>>> @@ -1642,25 +1652,6 @@ static int ds1307_probe(struct i2c_client *client,
>>>  			     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(ds1307->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)
>>> -		regmap_update_bits(ds1307->regmap, MCP794XX_REG_WEEKDAY,
>>> -				   MCP794XX_REG_WEEKDAY_WDAY_MASK,
>>> -				   tm.tm_wday + 1);
>>> -
>>>  	if (want_irq || ds1307_can_wakeup_device) {
>>>  		device_set_wakeup_capable(ds1307->dev, true);
>>>  		set_bit(HAS_ALARM, &ds1307->flags);
>>> -- 
>>> 2.14.1
>>>
>>
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rtc: ds1307: improve weekday handling
  2017-08-28 19:58   ` Heiner Kallweit
  2017-08-28 20:12     ` Heiner Kallweit
@ 2017-08-28 20:25     ` Alexandre Belloni
  1 sibling, 0 replies; 5+ messages in thread
From: Alexandre Belloni @ 2017-08-28 20:25 UTC (permalink / raw)
  To: Heiner Kallweit; +Cc: linux-rtc

On 28/08/2017 at 21:58:23 +0200, Heiner Kallweit wrote:
> Am 28.08.2017 um 21:15 schrieb Alexandre Belloni:
> > On 28/08/2017 at 20:37:21 +0200, Heiner Kallweit wrote:
> >> mcp794xx as one chip supported by this driver needs the weekday for
> >> alarm matching. RTC core ignores the weekday so we can't rely on
> >> the values we receive in member tm_wday of struct rtc_time.
> >> Therefore calculate the weekday from date/time when setting the
> >> time and setting the alarm time for mcp794xx.
> >>
> >> When having this in place we don't have to check the weekday
> >> at each driver load.
> >> After a chip reset date/time and weekday may be out of sync but
> >> in this case date/time need to be set anyway.
> >>
> > 
> > Nope, the core issue is that you can actually set an alarm in the future
> > without setting the time beforehand so this as to be fixed in the probe
> > (this was the issue as reported at the time of the fix).
> > 
> Ah, found the original thread where this was discussed.
> Still I don't really understand the problem. mcp794xx matches based on
> secs, min, hour, wday, mday, month. When I use "rtcwake -s 5" I expect
> the alarm to trigger 5 secs from now. How can this ever work if the
> RTC has random date/time (wday being valid or not)?
> 

wakealarm_store() reads the rtc time, adds the relative offset and sets
the alarm.

> I'd tend to say that if we know the RTC time is incorrect we shouldn't
> allow setting an alarm.

Unfortunately, this would break some platforms. Note that this is
already handled that way by some RTC drivers because they return an
error when it is known that the time is invalid.

I've started to work on that but it is far from finished.

> At least I wouldn't dare to program a bomb explosion time if I stand in
> front of it and know that the bomb's clock has a random value ;)
> 

A timer will work as long as the clock is ticking, it doesn't matter
when it is started ;)


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-08-28 20:26 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-28 18:37 [PATCH] rtc: ds1307: improve weekday handling Heiner Kallweit
2017-08-28 19:15 ` Alexandre Belloni
2017-08-28 19:58   ` Heiner Kallweit
2017-08-28 20:12     ` Heiner Kallweit
2017-08-28 20:25     ` Alexandre Belloni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.