Driver for Real Time Clock chip ST M41T65

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
[email protected]

drivers/rtc/rtc-m41t80.c +0 -0

index a3e0880..4d9b1a9 100644

drivers/rtc/rtc-m41t80.c +19 -8

 #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 | M41T80_FEATURE_SQ },
+       { "m41st84", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
+       { "m41st85", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
+       { "m41st87", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
       { }
 };
 MODULE_DEVICE_TABLE(i2c, m41t80_id);

@@ -385,8 +388,12 @@ static ssize_t m41t80_sysfs_show_sqwfreq(struct device *dev,
                               struct device_attribute *attr, char *buf)
 {
       struct i2c_client *client = to_i2c_client(dev);
+       struct m41t80_data *clientdata = i2c_get_clientdata(client);
       int val;
 
+       if (!(clientdata->features & M41T80_FEATURE_SQ))
+               return -EINVAL;
+
       val = i2c_smbus_read_byte_data(client, M41T80_REG_SQW);
       if (val < 0)
               return -EIO;

@@ -407,9 +414,13 @@ static ssize_t m41t80_sysfs_set_sqwfreq(struct device *dev,
                               const char *buf, size_t count)
 {
       struct i2c_client *client = to_i2c_client(dev);
+       struct m41t80_data *clientdata = i2c_get_clientdata(client);
       int almon, sqw;
       int val = simple_strtoul(buf, NULL, 0);
 
+       if (!(clientdata->features & M41T80_FEATURE_SQ))
+               return -EINVAL;
+
       if (val) {
               if (!is_power_of_2(val))
                       return -EINVAL;

@@ -498,6 +509,8 @@ static void wdt_ping(void)
                       .buf    = i2c_data,
               },
       };
+       struct m41t80_data *clientdata = i2c_get_clientdata(save_client);
+
       i2c_data[0] = 0x09;             /* watchdog register */
 
       if (wdt_margin > 31)

drivers/rtc/rtc-m41t80.c +1 -1

+
       i2c_transfer(save_client->adapter, msgs1, 1);
 }
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 6 lines · Steven A. Falco

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?

Quoted 5 lines · Will Newton

Quoted 4 lines · Steven A. Falco

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.

Quoted 2 lines

Quoted 2 lines

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?

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 <[email protected]>

drivers/rtc/rtc-m41t80.c +33 -10

index a3e0880..7eb4e05 100644
@@ -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 | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
+       { "m41st84", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
+       { "m41st85", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
+       { "m41st87", M41T80_FEATURE_HT | M41T80_FEATURE_BL | M41T80_FEATURE_SQ },
       { }
 };
 MODULE_DEVICE_TABLE(i2c, m41t80_id);

@@ -385,8 +391,12 @@ static ssize_t m41t80_sysfs_show_sqwfreq(struct device *dev,
                               struct device_attribute *attr, char *buf)
 {
       struct i2c_client *client = to_i2c_client(dev);
+       struct m41t80_data *clientdata = i2c_get_clientdata(client);
       int val;
 
+       if (!(clientdata->features & M41T80_FEATURE_SQ))
+               return -EINVAL;
+
       val = i2c_smbus_read_byte_data(client, M41T80_REG_SQW);
       if (val < 0)
               return -EIO;

@@ -407,9 +417,13 @@ static ssize_t m41t80_sysfs_set_sqwfreq(struct device *dev,
                               const char *buf, size_t count)
 {
       struct i2c_client *client = to_i2c_client(dev);
+       struct m41t80_data *clientdata = i2c_get_clientdata(client);
       int almon, sqw;
       int val = simple_strtoul(buf, NULL, 0);
 
+       if (!(clientdata->features & M41T80_FEATURE_SQ))
+               return -EINVAL;
+
       if (val) {
               if (!is_power_of_2(val))
                       return -EINVAL;

@@ -498,6 +512,8 @@ static void wdt_ping(void)
                       .buf    = i2c_data,
               },
       };
+       struct m41t80_data *clientdata = i2c_get_clientdata(save_client);
+
       i2c_data[0] = 0x09;             /* watchdog register */
 
       if (wdt_margin > 31)

@@ -508,6 +524,13 @@ static void wdt_ping(void)
                */
               i2c_data[1] = wdt_margin<<2 | 0x82;
 
+       /*
+        * M41T65 has three bits for watchdog resolution.  Don't set bit 7, as
+        * that would be an invalid resolution.
+        */
+       if (clientdata->features & M41T80_FEATURE_WD)
+               i2c_data[1] &= ~M41T80_WATCHDOG_RB2;
+
       i2c_transfer(save_client->adapter, msgs1, 1);
 }
 

Quoted 5 lines

Quoted 5 lines · Steven A. Falco

Quoted 4 lines

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.

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

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?

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?

Quoted 7 lines · On Tue, 26 Aug 2008 15:00:03 +0100 "Will Newton" <[email protected]> wrote:

Quoted 7 lines

Quoted 7 lines · Steven A. Falco

Quoted 6 lines

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?

Quoted 5 lines

Quoted 5 lines

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?

Given the small amount of changes, I would use the existing driver without renaming it.

Quoted 3 lines

Given the small amount of changes, I would use the existing driver without renaming it.

Sounds good. Please let me know if there are other changes needed to make this patch acceptable for inclusion into the mainline.

Add support for M41T65 Real Time Clock chip.

Signed-off-by Steven A. Falco <[email protected]>

---

Ok - here is a respin with the Kconfig modded to indicate support for the M41T65.

Quoted 2 lines

Ok - here is a respin with the Kconfig modded to indicate support for the M41T65.

Thanks, looks fine for me.

Add support for M41T65 Real Time Clock chip

Signed-off-by Steven A. Falco <[email protected]>

---

Fair enough. Here is a sorted version.

Add support for M41T65 Real Time Clock chip

Signed-off-by Steven A. Falco <[email protected]>

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?