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