The STMicroelectronics M41T11 is an RTC chip which is similar in most
ways to the Dallas/Maxim DS1307 or DS1337, except that it supports
software calibration:
http://www.st.com/stonline/books/pdf/docs/6103.pdf
I want to use this calibration feature in an embedded device I'm
working on at TANDBERG Telecom AS. The basic idea is that when the
device is connected to the network and an accurate reference clock is
available, a userland process will periodically measure the divergence
between the reference clock and the RTC, set the RTC to the correct
time and step the calibration register in the correct direction to
compensate for the observed drift.
The purpose of this is to reduce the extent to which the RTC will
drift when the device is switched off. Obviously it won't be perfect,
as the temperature will be lower when the device is off than when it
is being calibrated, but the crystal's accuracy curve is reasonably
flat across the relevant temperature range.
Since there is no way to distinguish the M41T11 from its many cousins,
I've forked a new rtc-m41t11 driver from rtc-ds1307, stripped some
irrelevant code, cleaned up the rest, and added support for the
calibration register.
In order to access this from userland, I've added two ioctls,
(RTC_SPEED_UP and RTC_SPEED_DOWN) with corresponding functions in
drivers/rtc/interface.c (rtc_speed_up() and rtc_speed_down()) and
corresponding slots in rtc_class_ops (speed_up and speed_down).
These ioctls etc. take no parameters: speed_up will increase the speed
of the RTC by some unspecified small amount and slow_down will
decrease it by some unspecified small amount. The application must
simply measure, tweak, wait, measure, tweak, wait etc. until the RTC
is within epsilon of the reference - where epsilon is some number that
takes into account the precision of the RTC and of the reference
clock, system call overhead, etc. Since the RTC usually has a 1s
precision, epsilon is likely to be ?1s in practice.
My hope is that this interface is simple enough and general enough to
be usable by other RTC chips that may also support calibration, and
that both design and code are of sufficiently quality to make it into
the mainline at some point.
Patch follows my sig. Comments of all kinds are welcome.
DES
--
Dag-Erling Sm?rgrav
Senior Software Developer
Linpro AS - http://www.linpro.no
Dag-Erling Sm?rgrav schrieb:
> The STMicroelectronics M41T11 is an RTC chip which is similar in most
> ways to the Dallas/Maxim DS1307 or DS1337, except that it supports
> software calibration:
>
> http://www.st.com/stonline/books/pdf/docs/6103.pdf
>
> I want to use this calibration feature [...]
> [...]
> In order to access this from userland, I've added two ioctls,
> (RTC_SPEED_UP and RTC_SPEED_DOWN) with corresponding functions in
> drivers/rtc/interface.c (rtc_speed_up() and rtc_speed_down()) and
> corresponding slots in rtc_class_ops (speed_up and speed_down).
It looks odd to me to do only differential up and down adjustments.
I would prefer read_calibration_register and write_calibration_register
access and let the userspace decide how much it wants to
increment/decrement the register.
Or - do you adjust your date also after you changed your battery
with +1567days,7h,34m,12sec instead of telling it's 11th Sept. 2007, 4:33pm ?
Regards,
Clemens Koller
__________________________________
R&D Imaging Devices
Anagramm GmbH
Rupert-Mayer-Stra?e 45/1
Linhof Werksgel?nde
D-81379 M?nchen
Tel.089-741518-50
Fax 089-741518-19
http://www.anagramm-technology.com
Clemens Koller <[email protected]> writes:
> It looks odd to me to do only differential up and down adjustments.
> I would prefer read_calibration_register and write_calibration_register
> access and let the userspace decide how much it wants to
> increment/decrement the register.
Without knowing exacly which chip is present, there is no way for the
userland calibration tool to know how big a difference a calibration
step makes. I don't see the value in telling the caller that the
current calibration value is N when it actually has no idea what N
means, what the allowable range is, if the scale is linear,
logarithmic, exponential, discontinuous...
The M41T11, for instance, has a calibration register that runs from
-31 to +31, with negative values are multiples of ~2.034 ppm while
positive values are multiples of ~4.068 ppm.
Therefore, I prefer to leave it to the driver author to decide what a
reasonable step in either direction is. The only consequence for the
userland calibration tool is that the RTC may take longer to converge
on hardware that has smaller calibration steps.
> Or - do you adjust your date also after you changed your battery
> with +1567days,7h,34m,12sec instead of telling it's 11th Sept. 2007, 4:33pm ?
No, the calibration process should start by setting the RTC to the
same time as the reference clock once it becomes available, then
waiting for a sufficient amount of time (at least three or four hours
to begin with, then progressively longer as the divergence decreases)
before measuring and adjusting.
Alternatively, if you don't want to set the RTC, you measure the
delta, wait sufficiently long, measure the delta again, and compare
the two deltas (this method is actually exactly the same as the one
outlined above, except that above the initial delta is 0)
DES
--
Dag-Erling Sm?rgrav
Senior Software Developer
Linpro AS - http://www.linpro.no
On Tue, Sep 11, 2007 at 03:48:53PM +0200, Dag-Erling Sm?rgrav wrote:
> The STMicroelectronics M41T11 is an RTC chip which is similar in most
> ways to the Dallas/Maxim DS1307 or DS1337, except that it supports
> software calibration:
>
> http://www.st.com/stonline/books/pdf/docs/6103.pdf
>
> I want to use this calibration feature in an embedded device I'm
> working on at TANDBERG Telecom AS. The basic idea is that when the
> device is connected to the network and an accurate reference clock is
> available, a userland process will periodically measure the divergence
> between the reference clock and the RTC, set the RTC to the correct
> time and step the calibration register in the correct direction to
> compensate for the observed drift.
>
> The purpose of this is to reduce the extent to which the RTC will
> drift when the device is switched off. Obviously it won't be perfect,
> as the temperature will be lower when the device is off than when it
> is being calibrated, but the crystal's accuracy curve is reasonably
> flat across the relevant temperature range.
>
> Since there is no way to distinguish the M41T11 from its many cousins,
> I've forked a new rtc-m41t11 driver from rtc-ds1307, stripped some
> irrelevant code, cleaned up the rest, and added support for the
> calibration register.
>
> In order to access this from userland, I've added two ioctls,
> (RTC_SPEED_UP and RTC_SPEED_DOWN) with corresponding functions in
> drivers/rtc/interface.c (rtc_speed_up() and rtc_speed_down()) and
> corresponding slots in rtc_class_ops (speed_up and speed_down).
I wonder if you can plug into the ntp code somehow to do this
automatically?
> These ioctls etc. take no parameters: speed_up will increase the speed
> of the RTC by some unspecified small amount and slow_down will
> decrease it by some unspecified small amount. The application must
> simply measure, tweak, wait, measure, tweak, wait etc. until the RTC
> is within epsilon of the reference - where epsilon is some number that
> takes into account the precision of the RTC and of the reference
> clock, system call overhead, etc. Since the RTC usually has a 1s
> precision, epsilon is likely to be ?1s in practice.
Sounds like a simplified version of ntp to me.
>
> My hope is that this interface is simple enough and general enough to
> be usable by other RTC chips that may also support calibration, and
> that both design and code are of sufficiently quality to make it into
> the mainline at some point.
>
> Patch follows my sig. Comments of all kinds are welcome.
Why not use NTP?
>
> DES
> --
> Dag-Erling Sm?rgrav
> Senior Software Developer
> Linpro AS - http://www.linpro.no
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index ff9e35c..07e5182 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -206,6 +206,15 @@ config RTC_DRV_PCF8583
> This driver can also be built as a module. If so, the module
> will be called rtc-pcf8583.
>
> +config RTC_DRV_M41T11
> + tristate "ST M41T11"
> + depends on RTC_CLASS && I2C
> + help
> + If you say yes here you get support for the ST M41T11 RTC chip.
> +
> + This driver can also be built as a module. If so, the module
> + will be called rtc-m41t11.
> +
> config RTC_DRV_M41T80
> tristate "ST M41T80 series RTC"
> help
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index d3a33aa..468aa0c 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_RTC_DRV_DS1672) += rtc-ds1672.o
> obj-$(CONFIG_RTC_DRV_DS1742) += rtc-ds1742.o
> obj-$(CONFIG_RTC_DRV_EP93XX) += rtc-ep93xx.o
> obj-$(CONFIG_RTC_DRV_ISL1208) += rtc-isl1208.o
> +obj-$(CONFIG_RTC_DRV_M41T11) += rtc-m41t11.o
> obj-$(CONFIG_RTC_DRV_M41T80) += rtc-m41t80.o
> obj-$(CONFIG_RTC_DRV_M48T59) += rtc-m48t59.o
> obj-$(CONFIG_RTC_DRV_M48T86) += rtc-m48t86.o
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index ad66c6e..189afb4 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -272,3 +272,43 @@ int rtc_irq_set_freq(struct rtc_device *rtc, struct rtc_task *task, int freq)
> return err;
> }
> EXPORT_SYMBOL_GPL(rtc_irq_set_freq);
Is it necessary to change the rtc interface? Can't you detect within
your m48t59 driver that you are getting consistent time adjustments up
or down from whats in the rtc and infer it needs speeding up or slowing
down?
Something like :
within the *_get_time function compute time delta, if 0<delta<Magic#
then slow down; else if -Magic#<delta<0 then speed up... (some magic
based on update interval and device spec)
> +
> +int rtc_speed_up(struct rtc_device *rtc)
> +{
> + int err = 0;
> +
> + err = mutex_lock_interruptible(&rtc->ops_lock);
> + if (err)
> + return -EBUSY;
> +
> + if (!rtc->ops)
> + err = -ENODEV;
> + else if (!rtc->ops->speed_up)
> + err = -EINVAL;
> + else
> + err = rtc->ops->speed_up(rtc->dev.parent);
> +
> + mutex_unlock(&rtc->ops_lock);
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(rtc_speed_up);
> +
> +int rtc_slow_down(struct rtc_device *rtc)
> +{
> + int err = 0;
> +
> + err = mutex_lock_interruptible(&rtc->ops_lock);
> + if (err)
> + return -EBUSY;
> +
> + if (!rtc->ops)
> + err = -ENODEV;
> + else if (!rtc->ops->slow_down)
> + err = -EINVAL;
> + else
> + err = rtc->ops->slow_down(rtc->dev.parent);
> +
> + mutex_unlock(&rtc->ops_lock);
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(rtc_slow_down);
> diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
> index 005fff3..d35f907 100644
> --- a/drivers/rtc/rtc-dev.c
> +++ b/drivers/rtc/rtc-dev.c
> @@ -223,6 +223,8 @@ static int rtc_dev_ioctl(struct inode *inode, struct file *file,
> switch (cmd) {
> case RTC_EPOCH_SET:
> case RTC_SET_TIME:
> + case RTC_SPEED_UP:
> + case RTC_SLOW_DOWN:
> if (!capable(CAP_SYS_TIME))
> return -EACCES;
> break;
> @@ -395,6 +397,15 @@ static int rtc_dev_ioctl(struct inode *inode, struct file *file,
> case RTC_UIE_ON:
> return set_uie(rtc);
> #endif
> +
> + case RTC_SPEED_UP:
> + err = rtc_speed_up(rtc);
> + break;
> +
> + case RTC_SLOW_DOWN:
> + err = rtc_speed_up(rtc);
> + break;
> +
> default:
> err = -ENOTTY;
> break;
> diff --git a/drivers/rtc/rtc-m41t11.c b/drivers/rtc/rtc-m41t11.c
> new file mode 100644
> index 0000000..2d7128f
> --- /dev/null
> +++ b/drivers/rtc/rtc-m41t11.c
> @@ -0,0 +1,319 @@
> +/*
> + * rtc-m41t11.c - RTC driver for some ST M41T11, derived from rtc-ds1307.c
> + *
> + * Copyright (C) 2005 James Chapman (ds1337 core)
> + * Copyright (C) 2006 David Brownell
> + * Copyright (C) 2007 TANDBERG Telecom AS
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/string.h>
> +#include <linux/rtc.h>
> +#include <linux/bcd.h>
> +
> +static unsigned short normal_i2c[] = { 0x68, I2C_CLIENT_END };
> +
> +I2C_CLIENT_INSMOD;
> +
> +#define M41T11_REG_SECS 0x00 /* 00-59 */
> +# define M41T11_BIT_ST 0x80
> +# define M41T11_MASK_SECS 0x7f
> +#define M41T11_REG_MIN 0x01 /* 00-59 */
> +# define M41T11_MASK_MIN 0x7f
> +#define M41T11_REG_HOUR 0x02 /* 00-23 */
> +# define M41T11_BIT_CEB 0x80
> +# define M41T11_BIT_CB 0x40
> +# define M41T11_MASK_HOUR 0x3f
> +#define M41T11_REG_WDAY 0x03 /* 01-07 */
> +# define M41T11_MASK_WDAY 0x7
> +#define M41T11_REG_MDAY 0x04 /* 01-31 */
> +# define M41T11_MASK_MDAY 0x3f
> +#define M41T11_REG_MONTH 0x05 /* 01-12 */
> +# define M41T11_MASK_MONTH 0x1f
> +#define M41T11_REG_YEAR 0x06 /* 00-99 */
> +# define M41T11_MASK_YEAR 0xff
> +#define M41T11_REG_CONTROL 0x07
> +# define M41T11_BIT_OUT 0x80
> +# define M41T11_BIT_FT 0x40
> +# define M41T11_BIT_S 0x20
> +# define M41T11_MASK_CAL 0x1f
> +
> +struct m41t11 {
> + struct i2c_client i2c;
> + struct rtc_device *rtc;
> +};
> +
> +static int m41t11_read(struct m41t11 *m41t11, u8 *data, int len)
> +{
> + struct i2c_msg msg[2];
> + int result;
> +
> + /* write address register */
> + msg[0].addr = m41t11->i2c.addr;
> + msg[0].flags = 0;
> + msg[0].len = 1;
> + msg[0].buf = data;
> +
> + /* read data */
> + msg[1].addr = m41t11->i2c.addr;
> + msg[1].flags = I2C_M_RD;
> + msg[1].len = len;
> + msg[1].buf = data + 1;
> +
> + result = i2c_transfer(m41t11->i2c.adapter, msg, 2);
> + if (result != 2) {
> + dev_err(&m41t11->i2c.dev, "read error %d\n", result);
> + return result;
> + }
> + return 0;
> +}
> +
> +static int m41t11_write(struct m41t11 *m41t11, u8 *data, int len)
> +{
> + struct i2c_msg msg[1];
> + int result;
> +
> + /* write address register and data */
> + msg[0].addr = m41t11->i2c.addr;
> + msg[0].flags = 0;
> + msg[0].len = 1 + len;
> + msg[0].buf = data;
> +
> + result = i2c_transfer(m41t11->i2c.adapter, msg, 1);
> + if (result != 1) {
> + dev_err(&m41t11->i2c.dev, "write error %d\n", result);
> + return result;
> + }
> + return 0;
> +}
> +
> +static int m41t11_get_time(struct device *dev, struct rtc_time *t)
> +{
> + struct m41t11 *m41t11 = dev_get_drvdata(dev);
> + u8 regs[8];
> +
> + /* read the RTC registers all at once */
> + regs[0] = 0;
> + if (m41t11_read(m41t11, regs, 7) != 0)
> + return -EIO;
> +
> + dev_info(dev, "read: %02x %02x %02x %02x %02x %02x %02x\n",
> + regs[1], regs[2], regs[3], regs[4], regs[5], regs[6], regs[7]);
> +
> + t->tm_sec = BCD2BIN(regs[1+M41T11_REG_SECS] & M41T11_MASK_SECS);
> + t->tm_min = BCD2BIN(regs[1+M41T11_REG_MIN] & M41T11_MASK_MIN);
> + t->tm_hour = BCD2BIN(regs[1+M41T11_REG_HOUR] & M41T11_MASK_HOUR);
> + t->tm_wday = BCD2BIN(regs[1+M41T11_REG_WDAY] & M41T11_MASK_WDAY) - 1;
> + t->tm_mday = BCD2BIN(regs[1+M41T11_REG_MDAY] & M41T11_MASK_MDAY);
> + t->tm_mon = BCD2BIN(regs[1+M41T11_REG_MONTH] & M41T11_MASK_MONTH) - 1;
> +
> + t->tm_year = BCD2BIN(regs[1+M41T11_REG_YEAR]);
> + /* assume 20xx if century bit is set, 19xx otherwise */
> + if (regs[1+M41T11_REG_HOUR] & M41T11_BIT_CB)
> + t->tm_year += 100;
> +
> + dev_info(dev, "read secs=%d, mins=%d, "
> + "hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
> + t->tm_sec, t->tm_min, t->tm_hour, t->tm_mday,
> + t->tm_mon, t->tm_year, t->tm_wday);
> +
> + return 0;
> +}
> +
> +static int m41t11_set_time(struct device *dev, struct rtc_time *t)
> +{
> + struct m41t11 *m41t11 = dev_get_drvdata(dev);
> + u8 regs[8], val;
> +
> + dev_info(dev, "write secs=%d, mins=%d, "
> + "hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
> + t->tm_sec, t->tm_min, t->tm_hour, t->tm_mday,
> + t->tm_mon, t->tm_year, t->tm_wday);
> +
> + regs[0] = 0;
> + regs[1+M41T11_REG_SECS] = BIN2BCD(t->tm_sec);
> + regs[1+M41T11_REG_MIN] = BIN2BCD(t->tm_min);
> + regs[1+M41T11_REG_HOUR] = BIN2BCD(t->tm_hour);
> + regs[1+M41T11_REG_WDAY] = BIN2BCD(t->tm_wday + 1);
> + regs[1+M41T11_REG_MDAY] = BIN2BCD(t->tm_mday);
> + regs[1+M41T11_REG_MONTH] = BIN2BCD(t->tm_mon + 1);
> +
> + val = t->tm_year % 100;
> + regs[1+M41T11_REG_YEAR] = BIN2BCD(val);
> + regs[1+M41T11_REG_HOUR] |= M41T11_BIT_CEB;
> + if ((t->tm_year / 100) % 2)
> + regs[1+M41T11_REG_HOUR] |= M41T11_BIT_CB;
> +
> + dev_info(dev, "write %02x %02x %02x %02x %02x %02x %02x\n",
> + regs[1], regs[2], regs[3], regs[4], regs[5], regs[6], regs[7]);
> +
> + if (m41t11_write(m41t11, regs, 7) != 0)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int m41t11_speed_up(struct device *dev)
> +{
> + struct m41t11 *m41t11 = dev_get_drvdata(dev);
> + u8 regs[2];
> +
> + regs[0] = M41T11_REG_CONTROL;
> + if (m41t11_read(m41t11, regs, 1) != 0)
> + return -EIO;
> +
> + /* XXX */
> +
> + return -EIO;
> +}
> +
> +static int m41t11_slow_down(struct device *dev)
> +{
> + struct m41t11 *m41t11 = dev_get_drvdata(dev);
> + u8 regs[2];
> +
> + regs[0] = M41T11_REG_CONTROL;
> + if (m41t11_read(m41t11, regs, 1) != 0)
> + return -EIO;
> +
> + /* XXX */
> +
> + return -EIO;
> +}
> +
> +static const struct rtc_class_ops ds13xx_rtc_ops = {
> + .read_time = m41t11_get_time,
> + .set_time = m41t11_set_time,
> + .speed_up = m41t11_speed_up,
> + .slow_down = m41t11_slow_down,
> +};
> +
> +static struct i2c_driver m41t11_driver;
> +
> +static int __devinit
> +m41t11_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> + struct m41t11 *m41t11;
> + u8 regs[8], val;
> + int err = -ENODEV;
> +
> + if (!(m41t11 = kzalloc(sizeof(*m41t11), GFP_KERNEL))) {
> + err = -ENOMEM;
> + goto exit;
> + }
> +
> + strlcpy(m41t11->i2c.name, "m41t11", I2C_NAME_SIZE);
> + m41t11->i2c.addr = address;
> + m41t11->i2c.adapter = adapter;
> + m41t11->i2c.driver = &m41t11_driver;
> + m41t11->i2c.flags = 0;
> + i2c_set_clientdata(&m41t11->i2c, m41t11);
> +
> +read_rtc:
> + /* read RTC registers */
> + regs[0] = 0;
> + if (m41t11_read(m41t11, regs, 7) != 0) {
> + err = -EIO;
> + goto exit_free;
> + }
> +
> + /* minimal sanity checking */
> + if (regs[1+M41T11_REG_SECS] & M41T11_BIT_ST) {
> + /* oscillator was stopped */
> + dev_warn(&m41t11->i2c.dev, "oscillator started; SET TIME!\n");
> + regs[0] = M41T11_REG_SECS;
> + regs[1] = 0;
> + m41t11_write(m41t11, regs, 1);
> + goto read_rtc;
> + }
> + val = BCD2BIN(regs[1+M41T11_REG_SECS] & M41T11_MASK_SECS);
> + if (val > 59)
> + goto exit_free;
> + val = BCD2BIN(regs[1+M41T11_REG_MIN] & M41T11_MASK_MIN);
> + if (val > 59)
> + goto exit_free;
> + val = BCD2BIN(regs[1+M41T11_REG_HOUR] & M41T11_MASK_HOUR);
> + if (val > 23)
> + goto exit_free;
> + val = BCD2BIN(regs[1+M41T11_REG_MDAY] & M41T11_MASK_MDAY);
> + if (val < 1 || val > 31)
> + goto exit_free;
> + val = BCD2BIN(regs[1+M41T11_REG_MONTH] & M41T11_MASK_MONTH);
> + if (val < 1 || val > 12)
> + goto exit_free;
> +
> + /* Tell the I2C layer a new client has arrived */
> + if ((err = i2c_attach_client(&m41t11->i2c)) != 0)
> + goto exit_free;
> +
> + m41t11->rtc = rtc_device_register(m41t11->i2c.name,
> + &m41t11->i2c.dev,
> + &ds13xx_rtc_ops,
> + THIS_MODULE);
> + if (IS_ERR(m41t11->rtc)) {
> + err = PTR_ERR(m41t11->rtc);
> + dev_err(&m41t11->i2c.dev,
> + "unable to register the class device\n");
> + goto exit_detach;
> + }
> +
> + return 0;
> +
> +exit_detach:
> + i2c_detach_client(&m41t11->i2c);
> +exit_free:
> + kfree(m41t11);
> +exit:
> + return err;
> +}
> +
> +static int __devinit
> +m41t11_attach_adapter(struct i2c_adapter *adapter)
> +{
> + if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
> + return 0;
> + return i2c_probe(adapter, &addr_data, m41t11_detect);
> +}
> +
> +static int __devexit m41t11_detach_client(struct i2c_client *client)
> +{
> + int err;
> + struct m41t11 *m41t11 = i2c_get_clientdata(client);
> +
> + rtc_device_unregister(m41t11->rtc);
> + if ((err = i2c_detach_client(client)))
> + return err;
> + kfree(m41t11);
> + return 0;
> +}
> +
> +static struct i2c_driver m41t11_driver = {
> + .driver = {
> + .name = "m41t11",
> + .owner = THIS_MODULE,
> + },
> + .attach_adapter = m41t11_attach_adapter,
> + .detach_client = __devexit_p(m41t11_detach_client),
> +};
> +
> +static int __init m41t11_init(void)
> +{
> + return i2c_add_driver(&m41t11_driver);
> +}
> +module_init(m41t11_init);
> +
> +static void __exit m41t11_exit(void)
> +{
> + i2c_del_driver(&m41t11_driver);
> +}
> +module_exit(m41t11_exit);
> +
> +MODULE_DESCRIPTION("RTC driver for ST M41T11");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> index 6d5e4a4..53e1980 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -91,6 +91,9 @@ struct rtc_pll_info {
> #define RTC_PLL_GET _IOR('p', 0x11, struct rtc_pll_info) /* Get PLL correction */
> #define RTC_PLL_SET _IOW('p', 0x12, struct rtc_pll_info) /* Set PLL correction */
>
> +#define RTC_SPEED_UP _IO('p', 0x13) /* speed up a notch */
> +#define RTC_SLOW_DOWN _IO('p', 0x14) /* slow down a notch */
> +
> /* interrupt flags */
> #define RTC_IRQF 0x80 /* any of the following is active */
> #define RTC_PF 0x40
> @@ -128,6 +131,8 @@ struct rtc_class_ops {
> int (*irq_set_state)(struct device *, int enabled);
> int (*irq_set_freq)(struct device *, int freq);
> int (*read_callback)(struct device *, int data);
> + int (*speed_up)(struct device *);
> + int (*slow_down)(struct device *);
> };
>
> #define RTC_DEVICE_NAME_SIZE 20
> @@ -196,6 +201,8 @@ extern int rtc_irq_set_state(struct rtc_device *rtc,
> struct rtc_task *task, int enabled);
> extern int rtc_irq_set_freq(struct rtc_device *rtc,
> struct rtc_task *task, int freq);
> +extern int rtc_speed_up(struct rtc_device *rtc);
> +extern int rtc_slow_down(struct rtc_device *rtc);
>
> typedef struct rtc_task {
> void (*func)(void *private_data);
Dag-Erling Sm?rgrav schrieb:
> Clemens Koller <[email protected]> writes:
>> It looks odd to me to do only differential up and down adjustments.
>> I would prefer read_calibration_register and write_calibration_register
>> access and let the userspace decide how much it wants to
>> increment/decrement the register.
>
> Without knowing exacly which chip is present, there is no way for the
> userland calibration tool to know how big a difference a calibration
> step makes.
I am not talking about the calibration algorithm and it's quality.
I am talking about _how_ the calibration register is addressed from
userspace. It's a simple register, some bits at address 7 and I would
expect to read/modify/write registers to do all the things you want
to do. Register access in userspace doesn't put any limitation
to applications.
Having only incs and decs without getting the actual value back seems
to be an absolutely unnecessary limitation here.
You cannot get the current value back to see if it's i.e. in saturation in
a way that it doesn't make sense to inc/decrement it further or in bigger steps
or reset it to zero...
Regards,
Clemens Koller
__________________________________
R&D Imaging Devices
Anagramm GmbH
Rupert-Mayer-Stra?e 45/1
Linhof Werksgel?nde
D-81379 M?nchen
Tel.089-741518-50
Fax 089-741518-19
http://www.anagramm-technology.com
Mark Gross <[email protected]> writes:
> Why not use NTP?
Because NTP is a completely different beast. NTP is used to keep a
software clock synchronized with a remote source. This software clock
is usually implemented as a software PLL on top of a high-resolution
hardware clock, with the remote clock serving as frequency reference.
What I need to do, however, is calibrate a low-resolution hardware
clock using a trusted reference (which could very well be a software
clock maintained by NTP).
DES
--
Dag-Erling Sm?rgrav
Senior Software Developer
Linpro AS - http://www.linpro.no
Clemens Koller <[email protected]> writes:
> Dag-Erling Sm?rgrav <[email protected]> writes:
> > Without knowing exacly which chip is present, there is no way for the
> > userland calibration tool to know how big a difference a calibration
> > step makes.
> I am not talking about the calibration algorithm and it's quality.
Neither am I.
> I am talking about _how_ the calibration register is addressed from
> userspace. It's a simple register, some bits at address 7 and I would
> expect to read/modify/write registers to do all the things you want
> to do. Register access in userspace doesn't put any limitation
> to applications.
It requires the application to know the hardware intimately.
Calibration of the M41T11 is implemented using the lower 6 bits of
register 7; this is not necessarily the case for other existing or
future chips.
Let's say I normalize this to [-128;127]; an application that tried to
speed up the clock would waste several hours increasing the
calibration value from 0 to 1, 2, 3 before seeing an effect after
increasing it to 4. And how do I normalize the assymetric range of
the M41T11?
> Having only incs and decs without getting the actual value back seems
> to be an absolutely unnecessary limitation here.
> You cannot get the current value back to see if it's i.e. in saturation in
> a way that it doesn't make sense to inc/decrement it further or in bigger steps
> or reset it to zero...
The driver will return EINVAL if you try to increment or decrement the
calibration register beyond its limits.
DES
--
Dag-Erling Sm?rgrav
Senior Software Developer
Linpro AS - http://www.linpro.no
Mark Gross <[email protected]> writes:
> Is it necessary to change the rtc interface? Can't you detect within
> your m48t59 driver that you are getting consistent time adjustments up
> or down from whats in the rtc and infer it needs speeding up or slowing
> down?
[I didn't notice this when I first replied]
Because of the RTC's low resolution and of how the calibration is
implemented in the chip, you need to sample the clock at intervals of
several hours in order to determine the direction of the required
adjustment. Repeatedly setting the RTC to the correct time will
actually prevent correct calibration.
DES
--
Dag-Erling Sm?rgrav
Senior Software Developer
Linpro AS - http://www.linpro.no
Dag-Erling Sm?rgrav schrieb:
> Clemens Koller <[email protected]> writes:
>> I am talking about _how_ the calibration register is addressed from
>> userspace. It's a simple register, some bits at address 7 and I would
>> expect to read/modify/write registers to do all the things you want
>> to do. Register access in userspace doesn't put any limitation
>> to applications.
>
> It requires the application to know the hardware intimately.
That's right... there is no need to put that into the kernel and
hide this trivial functionality from userspace.
> Calibration of the M41T11 is implemented using the lower 6 bits of
> register 7; this is not necessarily the case for other existing or
> future chips.
I've read the datasheet.
Your driver is specific for the M41T11 chip as mentioned in your
first mail, isn't it? If any future driver comes up with 8 bits
you wouldn't need to change a generic interface to read/modify/write
these 8 bits, right?
> Let's say I normalize this to [-128;127];
Why do any normalization in the driver? That's what userspace can do
in any way it might be necessary to do.
>> Having only incs and decs without getting the actual value back seems
>> to be an absolutely unnecessary limitation here.
>> You cannot get the current value back to see if it's i.e. in saturation in
>> a way that it doesn't make sense to inc/decrement it further or in bigger steps
>> or reset it to zero...
>
> The driver will return EINVAL if you try to increment or decrement the
> calibration register beyond its limits.
That behaviour seems also odd to me...
I can increment/decrement by an unknown number and then I get an EINVAL.
And I cannot reset it to some default value easily.
I still don't see any reason to implement relative changes to an otherwise
unknown value if it's possible to give absolute values to work with.
Well, that's my opinion, my five cents... I don't want to get into a lenghtly
discussion... I just used common sense how a interface to a register might
look like and your way of a relative manipulation just looks very uncommon
to me, having seen lot's of drivers.
Best regards,
Clemens Koller
__________________________________
R&D Imaging Devices
Anagramm GmbH
Rupert-Mayer-Stra?e 45/1
Linhof Werksgel?nde
D-81379 M?nchen
Tel.089-741518-50
Fax 089-741518-19
http://www.anagramm-technology.com
Dag-Erling Sm?rgrav <[email protected]> writes:
> + case RTC_SPEED_UP:
> + err = rtc_speed_up(rtc);
> + break;
> +
> + case RTC_SLOW_DOWN:
> + err = rtc_speed_up(rtc);
> + break;
This doesn't look quite right. rtc_slow_down, surely?
--
Arne.
Arne Georg Gleditsch <[email protected]> writes:
> Dag-Erling Sm?rgrav <[email protected]> writes:
> > + case RTC_SPEED_UP:
> > + err = rtc_speed_up(rtc);
> > + break;
> > +
> > + case RTC_SLOW_DOWN:
> > + err = rtc_speed_up(rtc);
> > + break;
> This doesn't look quite right. rtc_slow_down, surely?
Uh, yes. ENOCOFFEE.
DES
--
Dag-Erling Sm?rgrav
Senior Software Developer
Linpro AS - http://www.linpro.no
On Tue 2007-09-11 18:04:06, Dag-Erling Sm?rgrav wrote:
> Clemens Koller <[email protected]> writes:
> > Dag-Erling Sm?rgrav <[email protected]> writes:
> > > Without knowing exacly which chip is present, there is no way for the
> > > userland calibration tool to know how big a difference a calibration
> > > step makes.
> > I am not talking about the calibration algorithm and it's quality.
>
> Neither am I.
>
> > I am talking about _how_ the calibration register is addressed from
> > userspace. It's a simple register, some bits at address 7 and I would
> > expect to read/modify/write registers to do all the things you want
> > to do. Register access in userspace doesn't put any limitation
> > to applications.
>
> It requires the application to know the hardware intimately.
>
> Calibration of the M41T11 is implemented using the lower 6 bits of
> register 7; this is not necessarily the case for other existing or
> future chips.
The driver should know the hardware.
> Let's say I normalize this to [-128;127]; an application that tried to
> speed up the clock would waste several hours increasing the
> calibration value from 0 to 1, 2, 3 before seeing an effect after
> increasing it to 4. And how do I normalize the assymetric range of
> the M41T11?
So you normalize it to -32;32 range, and tell application (using
ioctl) that range is -32;32? Or you just -EINVAL if it goes out of
range?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html