Re: [rtc-linux] Re: invalid default values in RTC chip
From: Jüri Reitel
Date: Thu Oct 23 2008 - 03:44:29 EST
David Brownell wrote:
On Thursday 16 October 2008, Alessandro Zummo wrote:
On Tue, 14 Oct 2008 12:00:21 +0300
Jüri Reitel <juri.reitel@xxxxxxxxxxxxx> wrote:
My question relates to RTC driver linux/drivers/rtc/rtc-ds1307.c for
device m41t00. Driver probe function will fail if some of the chip's
registers contain invalid date time values i.e. if month register is 32
or minutes is 61.
This stuff?
tmp = ds1307->regs[DS1307_REG_SECS];
tmp = BCD2BIN(tmp & 0x7f);
if (tmp > 60)
goto exit_bad;
tmp = BCD2BIN(ds1307->regs[DS1307_REG_MIN] & 0x7f);
if (tmp > 60)
goto exit_bad;
tmp = BCD2BIN(ds1307->regs[DS1307_REG_MDAY] & 0x3f);
if (tmp == 0 || tmp > 31)
goto exit_bad;
tmp = BCD2BIN(ds1307->regs[DS1307_REG_MONTH] & 0x1f);
if (tmp == 0 || tmp > 12)
goto exit_bad;
Is this correct behavior? Probe function's purpose is
to check if the device is as was assumed (this time RTC and it is). The
chip's values are incorrect but the chip works, even the m41t00 chip
manual states that after initial powerup (RTC battery power applied)
internal registers will contain random data.
There are two solutions first is driver patch and another is i2c-dev and
i2cset tool to use from user space during bootup. Whitch one should be used?
Given we transitioned to the new i2c model, which mandates
a platform device to be declared,
Actually "i2c_board_info", not a "platform_device" ...
there's no more the need
to check for the device in the probing function.
this should be fixed in the driver.
Right. The code I snipped above dates from the old
drivers/i2c/chips/ds1337.c code, as I recall, which
was trying to defend against some random non-RTC chip
sitting at the relevant address. As pretty much all
legacy I2C drivers needed to do.
With "new style" I2C drivers, no guessing is needed
and such defenses are superfluos. It's fair to notice
that no chip is actually present ... but if there's
a chip there, it should be assumed to match the
declaration Linux was given.
however, a driver may still return a failure when reading
the time if there are inappropriate values in the registers.
In this driver, ds1307_get_time() already has a
return rtc_valid_tm(t);
But in the just-merged alarm support, ds1307_read_alarm()
doesn't do that. Actually that function should use a name
like ds1337_read_alarm(), since the 1307 has no alarm!
the solution is to patch the driver for detection and then
use a recent hwclock on bootup to write the time if required.
Exactly. Feel free to submit a patch removing those
probe() checks, and fixing the code returning the alarm
setting too.
I included patch that just removes the RTC time registers check, when i
compiled the module and booted up with invalid values in RTC then system
automatically set system time and RTC time to 1.1.2000 (this is nice
because i dont have to use date and hwclock manually)
i checked the rtc_valid_tm function and saw that it also checks for the
year value among others but in ds1307_read_alarm (now this function is
renamed to ds1337_read_alarm) year is set to -1 with some other fields
so rtc_valid_tm function can not be used in ds1337_read_alarm function.
- Dave
This patch removes rtc date time values check in probe function
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 162330b..2e87620 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -298,7 +298,7 @@ static int ds1307_set_time(struct device *dev,
struct rtc_time *t)
return 0;
}
-static int ds1307_read_alarm(struct device *dev, struct rtc_wkalrm *t)
+static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t)
{
struct i2c_client *client = to_i2c_client(dev);
struct ds1307 *ds1307 = i2c_get_clientdata(client);
@@ -353,7 +353,7 @@ static int ds1307_read_alarm(struct device *dev,
struct rtc_wkalrm *t)
return 0;
}
-static int ds1307_set_alarm(struct device *dev, struct rtc_wkalrm *t)
+static int ds1337_set_alarm(struct device *dev, struct rtc_wkalrm *t)
{
struct i2c_client *client = to_i2c_client(dev);
struct ds1307 *ds1307 = i2c_get_clientdata(client);
@@ -475,8 +475,8 @@ static int ds1307_ioctl(struct device *dev, unsigned
int cmd, unsigned long arg)
static const struct rtc_class_ops ds13xx_rtc_ops = {
.read_time = ds1307_get_time,
.set_time = ds1307_set_time,
- .read_alarm = ds1307_read_alarm,
- .set_alarm = ds1307_set_alarm,
+ .read_alarm = ds1337_read_alarm,
+ .set_alarm = ds1337_set_alarm,
.ioctl = ds1307_ioctl,
};
@@ -707,22 +707,6 @@ read_rtc:
break;
}
- tmp = ds1307->regs[DS1307_REG_SECS];
- tmp = bcd2bin(tmp & 0x7f);
- if (tmp > 60)
- goto exit_bad;
- tmp = bcd2bin(ds1307->regs[DS1307_REG_MIN] & 0x7f);
- if (tmp > 60)
- goto exit_bad;
-
- tmp = bcd2bin(ds1307->regs[DS1307_REG_MDAY] & 0x3f);
- if (tmp == 0 || tmp > 31)
- goto exit_bad;
-
- tmp = bcd2bin(ds1307->regs[DS1307_REG_MONTH] & 0x1f);
- if (tmp == 0 || tmp > 12)
- goto exit_bad;
-
tmp = ds1307->regs[DS1307_REG_HOUR];
switch (ds1307->type) {
case ds_1340:
@@ -779,13 +763,6 @@ read_rtc:
return 0;
-exit_bad:
- dev_dbg(&client->dev, "%s: %02x %02x %02x %02x %02x %02x %02x\n",
- "bogus register",
- ds1307->regs[0], ds1307->regs[1],
- ds1307->regs[2], ds1307->regs[3],
- ds1307->regs[4], ds1307->regs[5],
- ds1307->regs[6]);
exit_irq:
if (ds1307->rtc)
rtc_device_unregister(ds1307->rtc);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/