Add Dallas DS1390 RTC chip
Subsystems (2)
PATCH v2 1/1 Add Dallas DS1390 RTC chip
v2 of this patch with some code tidying as per previous comments.
This patch adds support for the Dallas DS1390 SPI RTC chip.
--- drivers/rtc/Kconfig | 14 +++ drivers/rtc/Makefile | 1 + drivers/rtc/rtc-ds1390.c | 258 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 273 insertions(+), 0 deletions(-)
drivers/rtc/Kconfig +14 -0
@@ -292,6 +292,20 @@ config RTC_DRV_DS1305 This driver can also be built as a module. If so, the module will be called rtc-ds1305. +config RTC_DRV_DS1390 + tristate "Dallas/Maxim DS1390" + help + If you say yes here you get support for the DS1390 and probably + other chips connected with SPI. + + The first seven registers on these chips hold an RTC, and other + registers may add features such as NVRAM, a trickle charger for + the RTC/NVRAM backup power, and alarms. This driver may not + expose all those available chip features. + + This driver can also be built as a module. If so, the module + will be called rtc-ds1390. + config RTC_DRV_MAX6902 tristate "Maxim MAX6902" help
drivers/rtc/Makefile +1 -0
@@ -27,6 +27,7 @@ obj-$(CONFIG_RTC_DRV_DS1302) += rtc-ds1302.o obj-$(CONFIG_RTC_DRV_DS1305) += rtc-ds1305.o obj-$(CONFIG_RTC_DRV_DS1307) += rtc-ds1307.o obj-$(CONFIG_RTC_DRV_DS1374) += rtc-ds1374.o +obj-$(CONFIG_RTC_DRV_DS1390) += rtc-ds1390.o obj-$(CONFIG_RTC_DRV_DS1511) += rtc-ds1511.o obj-$(CONFIG_RTC_DRV_DS1553) += rtc-ds1553.o obj-$(CONFIG_RTC_DRV_DS1672) += rtc-ds1672.o
drivers/rtc/rtc-ds1390.c new file +258 -0
258 lines not on this page · raw message
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
drivers/rtc/rtc-ds1390.c line 48
Quoted 7 lines · On Mon, 20 Oct 2008 09:41:18 +0100 Mark Jackson…
drivers/rtc/rtc-ds1390.c new file +7
-0+struct ds1390 { +struct rtc_device *rtc; +u8 buf[9 ];/* cmd + 8 registers */ +u8 tx_buf[2 ]; +u8 rx_buf[2 ]; +}; +
don't we still have too many buffers?
the use of bin2bcd an bcd2bin (lowercase) would also be appreciated.
Signature · 13 lines
Best regards, Alessandro Zummo, Tower Technologies - Torino, Italy http://www.towertech.it ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/
Quoted 12 lines · Alessandro Zummo
drivers/rtc/rtc-ds1390.c line 48
Quoted 7 lines · On Mon, 20 Oct 2008 09:41:18 +0100 Mark Jackson <[email protected]> wrote:
drivers/rtc/rtc-ds1390.c new file +7
-0+struct ds1390 { +struct rtc_device *rtc; +u8 buf[9 ];/* cmd + 8 registers */ +u8 tx_buf[2 ]; +u8 rx_buf[2 ]; +}; +don't we still have too many buffers?
Ah yes ... ooops !!
Quoted 3 lines
the use of bin2bcd an bcd2bin (lowercase) would also be appreciated.
Will do.
Mark
Quoted 5 lines · Mark Jackson
+config
RTC_DRV_DS1390
+ tristate "Dallas/Maxim DS1390"
+ help
+ If you say yes here you get support for the DS1390 and probably + other chips connected with SPI.
According to the datasheet at
http://www.maxim-ic.com/quick_view2.cfm/qv_pk/4359
the DS1390, DS1391, DS1392, DS1393, and DS1394 chips are essentially all the same except that the control register for the '91 and '92 has different bits.
So it looks like this driver should already support three chips, if the spi_board_info has the right spi->mode bits set: SPI_MODE_* and SPI_3WIRE. And if there were platform_data defined to hold the initial control register setting, two more ... configuring that trickle charger would already justify having platform_data too.
Please strike the "probably" and list at least those three chips.
Quoted 4 lines
+ The first seven registers on these chips hold an RTC, and other + registers may add features such as NVRAM, a trickle charger for + the RTC/NVRAM backup power, and alarms. This driver may not + expose all those available chip features.
The "these chips" language bothers me. This looks like cut'n'paste from the rtc-ds1307 language, but you don't actually describe what chips the driver handles.
I'd rather see the description say which other chips it expects to handle, and have the description match. All of these have alarms and a trickle charger for backup power, but none have NVRAM.
Quoted 1 line
+/*
drivers/rtc/rtc-ds1390.c
It's a bit of a nit, but the convention is
/* * rtc-ds1390.c -- driver for DS1390 and related SPI RTCs * * Copyright (C) ... etc
Maybe with a separate "written by Mark Jackson" next to the copyright.
Quoted 9 lines
+ *
+ * Copyright (C) 2008 Mercury IMC Ltd
+ *
+ * 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.
+ *
+ * Driver for DS1390 spi RTC
+ *
And to keep changelogs out of source code; that's what GIT is for!
Quoted 6 lines
+ * Changelog:
+ *
+ * 04-Apr-2008: Mark Jackson <[email protected]>
+ * - Initial version based on rtc-max6902
+ * - No alarm support though !!
+ */
My personal taste is to have the copyright comment cover ONLY the copyright (and licensing) issues, and have a separate comment describing open issues with the code. In this case, that would be no support for the alarm, any other aspect of the control register, or (unless it was set up by pre-OS code) the trickle charger ...
drivers/rtc/rtc-ds1390.c line 131
Quoted 4 lines
drivers/rtc/rtc-ds1390.c new file +4
-0+static int ds1390_get_datetime (struct device *dev,struct rtc_time *dt) +{ + ... + return 0;
Especially since you don't currently have logic to handle the initialization from first power up (clearing OSF etc), it'd be good to "return rtc_valid_tm(dt);".
Quoted 5 lines
+ status =
spi_sync(spi, &message);
+ if (status == 0)
+ return message.status;
+
+ return 0;
Nowadays that can just be "return spi_sync(...)".
Although ... you can make this code be a bunch simpler by using spi_write_then_read().
Quoted 4 lines
+static struct
spi_driverds1390_driver= {
+ .driver = {
+ .name = "rtc-ds1390",
+ .bus =&spi_bus_type,
Rule of thumb: always let the bus code set driver->bus, never set it yourself.
Quoted 5 lines
+ .owner =
THIS_MODULE,
+ },
+ .probe =ds1390_probe,
+ .remove =__devexit_p(ds1390_remove),
+};
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/