login
Search
I will be using a Real Time Clock chip, ST M41T65, on a new embedded system. The chip is quite similar to the M41T8x family, which already has driver rtc-m41t80.c. Would it be more acceptible to generalize rtc-m41t80.c (perhaps renaming it to rtc-m41txx.c) or would it be better to create a new rtc-m41t6x.c? The main differences I see between the M41T65 and M41T80 are that: 1) The M41T65 watchdog timer has three bits controlling resolution (versus two for the M41T80). 2) There is no register 0x13 for controlling square-wave output. Here is the patch I am currently using (on some prototype hardware), for reference. I've introduced two new feature bits to cover the above differences. I have not yet done any renaming, nor have I modified any of the defconfigs to account for a name change. Steve Falco sfalco@harris.com diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c index a3e0880..4d9b1a9 100644 --- a/drivers/rtc/rtc-m41t80.c +++ b/drivers/rtc/rtc-m41t80.c @@ -58,18 +58,21 @@ #define M41T80_FEATURE_HT (1 << 0) #define M41T80_FEATURE_BL (1 << 1) +#define M41T80_FEATURE_SQ (1 << 2) +#define M41T80_FEATURE_WD (1 << 3) #define DRV_VERSION "0.05" static const struct i2c_device_id m41t80_id[] = { - { "m41t80", 0 }, - { "m41t81", M41T80_FEATURE_HT }, - { "m41t81s", M41T80_FEATURE_HT | M41T80_FEATURE_BL }, - { "m41t82", M41T80_FEATURE_HT | M41T80_FEATURE_BL }, - { "m41t83", M41T80_FEATURE_HT | M41T80_FEATURE_BL }, - { "m41st84", M41T80_FEATURE_HT | M41T80_FEATURE_BL }, - { "m41st85", M41T80_FEATURE_HT | M41T80_FEATURE_BL }, - { "m41st87", M41T80_FEATURE_HT | M41T80_FEATURE_BL }, + { "m41t65", M41T80_FEATURE_HT | M41T80_FEATURE_WD }, + { "m41t80", M41T80_FEATURE_SQ }, + { "m41t81", M41T80_FEATURE_HT | M41T80_FEATURE_SQ}, + { "m41t81s", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ }, + { "m41t82", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ }, + { "m41t83", M41T80_FEATURE_HT | M41T80_FEATURE_BL | ...
I believe the kernel comment style would be: /* * M41T65 has three bits for resolution. Don't set bit 7, as that would * be an invalid resolution. --
Thanks for the feedback. Here is a respin which addresses your comments.
I'd also like to know if I should submit a new driver or mod the existing
one. If I mod the existing one, should it be renamed, should I update the
Kconfig, existing defconfigs, etc?
Signed-off-by: Steven A. Falco <sfalco@harris.com>
diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index a3e0880..7eb4e05 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -55,21 +55,27 @@
#define M41T80_ALHOUR_HT (1 << 6) /* HT: Halt Update Bit */
#define M41T80_FLAGS_AF (1 << 6) /* AF: Alarm Flag Bit */
#define M41T80_FLAGS_BATT_LOW (1 << 4) /* BL: Battery Low Bit */
+#define M41T80_WATCHDOG_RB2 (1 << 7) /* RB: Watchdog resolution */
+#define M41T80_WATCHDOG_RB1 (1 << 1) /* RB: Watchdog resolution */
+#define M41T80_WATCHDOG_RB0 (1 << 0) /* RB: Watchdog resolution */
-#define M41T80_FEATURE_HT (1 << 0)
-#define M41T80_FEATURE_BL (1 << 1)
+#define M41T80_FEATURE_HT (1 << 0) /* Halt feature */
+#define M41T80_FEATURE_BL (1 << 1) /* Battery low indicator */
+#define M41T80_FEATURE_SQ (1 << 2) /* Squarewave feature */
+#define M41T80_FEATURE_WD (1 << 3) /* Extra watchdog resolution */
#define DRV_VERSION "0.05"
static const struct i2c_device_id m41t80_id[] = {
- { "m41t80", 0 },
- { "m41t81", M41T80_FEATURE_HT },
- { "m41t81s", M41T80_FEATURE_HT | M41T80_FEATURE_BL },
- { "m41t82", M41T80_FEATURE_HT | M41T80_FEATURE_BL },
- { "m41t83", M41T80_FEATURE_HT | M41T80_FEATURE_BL },
- { "m41st84", M41T80_FEATURE_HT | M41T80_FEATURE_BL },
- { "m41st85", M41T80_FEATURE_HT | M41T80_FEATURE_BL },
- { "m41st87", M41T80_FEATURE_HT | M41T80_FEATURE_BL },
+ { "m41t65", M41T80_FEATURE_HT | M41T80_FEATURE_WD },
+ { "m41t80", M41T80_FEATURE_SQ },
+ { "m41t81", M41T80_FEATURE_HT | M41T80_FEATURE_SQ},
+ { "m41t81s", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
+ { "m41t82", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
+ { "m41t83", M41T80_FEATURE_HT ...On Tue, 26 Aug 2008 15:00:03 +0100 Given the small amount of changes, I would use the existing driver without renaming it. -- Best regards, Alessandro Zummo, Tower Technologies - Torino, Italy http://www.towertech.it --
Sounds good. Please let me know if there are other changes needed to make this patch acceptable for inclusion into the mainline. Steve --
It would be better adding the chip name to the Kconfig help text as well. --- Atsushi Nemoto --
Add support for M41T65 Real Time Clock chip. Signed-off-by: Steven A. Falco <sfalco@harris.com> --- Ok - here is a respin with the Kconfig modded to indicate support for the M41T65. drivers/rtc/Kconfig | 12 ++++++------ drivers/rtc/rtc-m41t80.c | 43 +++++++++++++++++++++++++++++++++---------- 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index 4949dc4..b3da827 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -220,12 +220,12 @@ config RTC_DRV_PCF8583 will be called rtc-pcf8583. config RTC_DRV_M41T80 - tristate "ST M41T80/81/82/83/84/85/87" + tristate "ST M41T80/81/82/83/84/85/87/65" help - If you say Y here you will get support for the - ST M41T80 RTC chips series. Currently following chips are - supported: M41T80, M41T81, M41T82, M41T83, M41ST84, M41ST85 - and M41ST87. + If you say Y here you will get support for the ST M41T80 + and M41T60 RTC chips series. Currently, the following chips are + supported: M41T80, M41T81, M41T82, M41T83, M41ST84, M41ST85, + M41ST87, and M41T65. This driver can also be built as a module. If so, the module will be called rtc-m41t80. @@ -235,7 +235,7 @@ config RTC_DRV_M41T80_WDT depends on RTC_DRV_M41T80 help If you say Y here you will get support for the - watchdog timer in ST M41T80 RTC chips series. + watchdog timer in the ST M41T80 and M41T60 RTC chips series. config RTC_DRV_TWL92330 boolean "TI TWL92330/Menelaus" diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c index a3e0880..7eb4e05 100644 --- a/drivers/rtc/rtc-m41t80.c +++ b/drivers/rtc/rtc-m41t80.c @@ -55,21 +55,27 @@ #define M41T80_ALHOUR_HT (1 << 6) /* HT: Halt Update Bit */ #define M41T80_FLAGS_AF (1 << 6) /* AF: Alarm Flag Bit */ #define M41T80_FLAGS_BATT_LOW (1 << 4) /* BL: Battery Low Bit */ +#define M41T80_WATCHDOG_RB2 (1 << 7) /* RB: Watchdog resolution */ +#define M41T80_WATCHDOG_RB1 (1 << 1) /* RB: ...
Thanks, looks fine for me. Acked-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp> --
Not for me though -- the names should be sorted alphabetically as this is how the reader will look for the right one (assuming they do not get fed up and resort to `grep'). There is virtually no chance to notice this one among the plethora of chip variations supported. Maciej --
Add support for M41T65 Real Time Clock chip Signed-off-by: Steven A. Falco <sfalco@harris.com> --- Fair enough. Here is a sorted version. drivers/rtc/Kconfig | 14 +++++++------- drivers/rtc/rtc-m41t80.c | 43 +++++++++++++++++++++++++++++++++---------- 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index 4949dc4..c7586bc 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -220,22 +220,22 @@ config RTC_DRV_PCF8583 will be called rtc-pcf8583. config RTC_DRV_M41T80 - tristate "ST M41T80/81/82/83/84/85/87" + tristate "ST M41T65/M41T80/81/82/83/84/85/87" help - If you say Y here you will get support for the - ST M41T80 RTC chips series. Currently following chips are - supported: M41T80, M41T81, M41T82, M41T83, M41ST84, M41ST85 - and M41ST87. + If you say Y here you will get support for the ST M41T60 + and M41T80 RTC chips series. Currently, the following chips are + supported: M41T65, M41T80, M41T81, M41T82, M41T83, M41ST84, + M41ST85, and M41ST87. This driver can also be built as a module. If so, the module will be called rtc-m41t80. config RTC_DRV_M41T80_WDT - bool "ST M41T80 series RTC watchdog timer" + bool "ST M41T65/M41T80 series RTC watchdog timer" depends on RTC_DRV_M41T80 help If you say Y here you will get support for the - watchdog timer in ST M41T80 RTC chips series. + watchdog timer in the ST M41T60 and M41T80 RTC chips series. config RTC_DRV_TWL92330 boolean "TI TWL92330/Menelaus" diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c index a3e0880..7eb4e05 100644 --- a/drivers/rtc/rtc-m41t80.c +++ b/drivers/rtc/rtc-m41t80.c @@ -55,21 +55,27 @@ #define M41T80_ALHOUR_HT (1 << 6) /* HT: Halt Update Bit */ #define M41T80_FLAGS_AF (1 << 6) /* AF: Alarm Flag Bit */ #define M41T80_FLAGS_BATT_LOW (1 << 4) /* BL: Battery Low Bit */ +#define M41T80_WATCHDOG_RB2 (1 << 7) /* RB: Watchdog resolution ...
You've had a good idea to spell out M41T65 in full -- it makes it more prominent. Thanks. Maciej --
On Wed, 27 Aug 2008 13:22:05 -0400 Acked-by: Alessandro Zummo <a.zummo@towertech.it> -- Best regards, Alessandro Zummo, Tower Technologies - Torino, Italy http://www.towertech.it --
Keeping the code base common is good for maintenance. Therefore for a minor differences like yours there is no doubt you better just add them to the existing driver. For larger variations -- it depends, i.e. it's better to keep the common bits shared, unless the maintenance cost of such an approach exceeds that of the other one. Frequently it just boils down to doing it right which may not necessarily be easy. ;) Maciej --