Re: [rtc-linux] Re: invalid default values in RTC chip

[David Brownell]

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.

- Dave
--
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/