ST M41T00 I2C RTC chip driver

This patch adds support for the ST M41T00 RTC chip.

You will likely notice that it implements a PPC-specific interface (/dev/rtc->drivers/char/genrtc.h->include/asm-ppc/rtc.h->this file). This was necessary to support a subset of ppc platforms that need to hook up the rtc support at runtime. If I implemented /dev/rtc directly or interfaced to genrtc.c directly, those platforms couldn't use this driver. Eventually, I hope to work on more uniform rtc support across all the processor architectures.

Also, on ppc at least, the hw clock can be set from a timer interrupt if STA_UNSYNC is not set (e.g., ntpd is running). To handle this, a tasklet is used to set the clock if in_interrupt() is true.

I'd appreciate an comments or to have it pushed into the kernel.org tree if its acceptable.

Thanks,

Mark

Signed-off-by Mark A. Greer <[email protected]>

--

This patch adds support for the ST M41T00 RTC chip.

On ix86 machines, it is appropriate to read the RTC clock
several times in a row until nothing changes. This protects
against getting bad readings when some values wrap (like
seconds). You can't stop the clock when you read it
or you will lose time. I don't see anything like this in your code. Also, when setting the clock it is necessary to stop the clock so its settings don't
change while you are writing a new time. I also don't
see anything like this in your code either.

Also ix86 machines have a spin-lock, rtc_lock, so that other procedures, even interrupts can access its registers. I see you using a semaphore that can't be used in interrupt context.

Notice: the RTC clock is used for high-precision timing via interrupt in some ix86 drivers. If you modify the RTC code on all platforms as you propose, you cannot "keep" the RTC all to your self. Its interrupt must be available to drivers and access to the hardware needs to be locked with the existing spin-lock.

I note that on 2.6.9 on somebody broke it so that, the interrupt is used for something only ONCE during startup and ONCE during shut-down.

CPU0

  0:    6225448    IO-APIC-edge  timer
  1:      14002    IO-APIC-edge  i8042
  8:          1    IO-APIC-edge  rtc    <--- bingo
  9:          0   IO-APIC-level  acpi

[snipped...]

I don't have a clue why anybody would grab that interrupt. Fortunately, the interrupt-grabbing code can be loaded as a module and then unloaded so that other drivers can use that device and its interrupt. Any changes to the RTC code need to consider these things.

Hi Mark,

Review

Quoted 4 lines

(quoted diff) +2 -0

+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-m41t00.

It'll actually be called m41t00, according to the Makefile.

Review

Quoted 7 lines

(quoted diff) +3 -0

+struct m41t00_data {
+	struct i2c_client client;
+};

You don't have to do that. Including the i2c_client stucture in the data structure is a trick which let us get both allocated with a single kmalloc (and freed with a single kfree) while still respecting the arch-dependent alignment requirements. If you have no private data to carry around, you can do the kmalloc on the i2c_client structure directly, and have client->data point to NULL (which it actually already does thanks to memset). This will save some code in both the detection and the detach functions.

However, if you know that, in a future update of this driver, you *will* have to store client-private data, then I guess you can keep it this way.

Review

Quoted 3 lines

(quoted diff) +1 -0

+	i2c_detach_client(client);

This one supposedly can fail.

Review

Quoted 5 lines

(quoted diff) +1 -0

+	.name		= "M41T00",

No caps in name please (will be used in sysfs).

Quoted 9 lines

(quoted diff) +5 -0

+static int __devinit
+m41t00_init(void)
+{
+	return i2c_add_driver(&m41t00_driver);
+}

Review

Quoted 8 lines

(quoted diff) +6 -0

+static void __devexit
+m41t00_exit(void)
+{
+	i2c_del_driver(&m41t00_driver);
+	return;
+}

Should be __init and __exit, respectively, unless I am mistaken. And the last return is usless.

I'm also suspicious about the other __devexit and __devinit you used. No other i2c chip drivers has them.

<snip>

Here is a replacement patch that should address Jean Delvare and Dick Johnson's issues. Please let me know if there are any more issues.

Thanks,

Mark

Signed-off-by Mark A. Greer <[email protected]>

From http://archives.andrew.net.au/lm-sensors/msg29319.html:

Quoted 7 lines · Mark A. Greer

<snip>

Here is a replacement patch that should address Jean Delvare and Dick Johnson's issues. Please let me know if there are any more issues.

Thanks,

Quoted 4 lines

Mark

Signed-off-by Mark A. Greer <[email protected]>

I haven't heard of any more problems so can I get this patch applied?

Can you resend it with a proper Changelog description in the top of the email and the signed-off-by line? thanks,

greg k-h

Quoted 7 lines · Greg KH

Can you resend it with a proper Changelog description in the top of the email and the signed-off-by line? thanks,

greg k-h

Certainly.
--

This patch adds support for the ST M41T00 I2C RTC chip.

This rtc chip has no mechanism to freeze it's registers while being read; however, it will delay updating the external values of the registers for 250ms after a register is read. To ensure that a sane time value is read, the driver verifies that the same registers values were read twice before returning.

Also, when setting the rtc from an interrupt handler, a tasklet is used to provide the context required by the i2c core code.

Please apply.

Thank you for your comments, Jean.

Quoted 11 lines · Jean Delvare

Hi Mark,

drivers/i2c/chips/Kconfig line 381

Quoted 3 lines

drivers/i2c/chips/Kconfig +2 -0

+      This driver can also be built as a module.  If so, the module
+      will be called i2c-m41t00.

It'll actually be called m41t00, according to the Makefile.

Good catch.

Quoted 17 lines

drivers/i2c/chips/m41t00.c line 35

Quoted 6 lines

drivers/i2c/chips/m41t00.c new file +3 -0

+struct m41t00_data {
+    struct i2c_client client;
+};

You don't have to do that. Including the i2c_client stucture in the data structure is a trick which let us get both allocated with a single kmalloc (and freed with a single kfree) while still respecting the arch-dependent alignment requirements. If you have no private data to carry around, you can do the kmalloc on the i2c_client structure directly, and have client->data point to NULL (which it actually already does thanks to memset). This will save some code in both the detection and the detach functions.

Quoted 3 lines

However, if you know that, in a future update of this driver, you *will* have to store client-private data, then I guess you can keep it this way.

Its gone.

Quoted 6 lines

Quoted 2 lines

+ i2c_detach_client(client);

This one supposedly can fail.

Okay, I'll check.

Quoted 8 lines

Quoted 4 lines

+ .name = "M41T00",

No caps in name please (will be used in sysfs).

Done.

Quoted 9 lines

Quoted 9 lines

drivers/i2c/chips/m41t00.c new file +5 -0

+static int __devinit
+m41t00_init(void)
+{
+    return i2c_add_driver(&m41t00_driver);
+}

Quoted 12 lines

drivers/i2c/chips/m41t00.c line 217

Quoted 7 lines

drivers/i2c/chips/m41t00.c new file +6 -0

+static void __devexit
+m41t00_exit(void)
+{
+    i2c_del_driver(&m41t00_driver);
+    return;
+}

Should be __init and __exit, respectively, unless I am mistaken. And the last return is usless.

I thought the __devxxx ones were the best ones to use but I don't know for sure. I'll change them to __init/__exit.

Quoted 5 lines

I'm also suspicious about the other __devexit and __devinit you used. No other i2c chip drivers has them.

Done.

Here is a replacement patch that should address Jean Delvare and Dick Johnson's issues. Please let me know if there are any more issues.

Thanks,

Mark