[RFC] Driver for Real Time Clock chip ST M41T65

[RFC] Driver for Real Time Clock chip ST M41T65

[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?

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

Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Will Newton]

On Mon, Aug 25, 2008 at 6:27 PM, Steven A. Falco <sfalco@harris.com> wrote:
> 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)

Perhaps these feature flags should be documented with a comment?

>  #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)
> @@ -508,6 +521,12 @@ static void wdt_ping(void)
>                 */
>                i2c_data[1] = wdt_margin<<2 | 0x82;
>
> +       /* M41T65 has three bits for resolution.  Don't set bit 7, as that would
> +        * be an invalid resolution.
> +        */

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.
 */

> +       if (clientdata->features & M41T80_FEATURE_WD)
> +               i2c_data[1] &= ~0x80;

Could this magic constant be moved to a #define?

> +
>        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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Steven A. Falco]

Will Newton wrote:
> On Mon, Aug 25, 2008 at 6:27 PM, Steven A. Falco <sfalco@harris.com> wrote:
>> 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?

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 | 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);
 }
 

Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Will Newton]

On Tue, Aug 26, 2008 at 2:55 PM, Steven A. Falco <sfalco@harris.com> wrote:
> Will Newton wrote:
>> On Mon, Aug 25, 2008 at 6:27 PM, Steven A. Falco <sfalco@harris.com> wrote:
>>> 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?
>
> 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?

I've ccd the rtc-linux list.

> 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 | 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);
>  }
>
>

Re: [rtc-linux] Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Alessandro Zummo]

On Tue, 26 Aug 2008 15:00:03 +0100
"Will Newton" <will.newton@gmail.com> wrote:


> >> On Mon, Aug 25, 2008 at 6:27 PM, Steven A. Falco <sfalco@harris.com> wrote:
> >>> 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?
> >
> > 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.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: [rtc-linux] Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Steven A. Falco]

Alessandro Zummo wrote:
> On Tue, 26 Aug 2008 15:00:03 +0100
> "Will Newton" <will.newton@gmail.com> wrote:
> 
> 
>>>> On Mon, Aug 25, 2008 at 6:27 PM, Steven A. Falco <sfalco@harris.com> wrote:
>>>>> 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?
>>> 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.
> 

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

	Steve

Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Maciej W. Rozycki]

On Tue, 26 Aug 2008, Steven A. Falco wrote:

> 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?

 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

Re: [rtc-linux] Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Atsushi Nemoto]

On Tue, 26 Aug 2008 10:39:22 -0400, "Steven A. Falco" <sfalco@harris.com> wrote:
> >  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.

It would be better adding the chip name to the Kconfig help text as well.

---
Atsushi Nemoto

Re: [rtc-linux] Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Steven A. Falco]

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: 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);
 }
 
-- 
1.5.5.1

Re: [rtc-linux] Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Atsushi Nemoto]

On Wed, 27 Aug 2008 09:32:56 -0400, "Steven A. Falco" <sfalco@harris.com> wrote:
> 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.

Thanks, looks fine for me.

Acked-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

Re: [rtc-linux] Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Maciej W. Rozycki]

On Wed, 27 Aug 2008, Atsushi Nemoto wrote:

> > Ok - here is a respin with the Kconfig modded to indicate support for
> > the M41T65.
> 
> Thanks, looks fine for me.

 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

Re: [rtc-linux] Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Steven A. Falco]

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 */
+#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);
 }
 
-- 
1.5.5.1

Re: [rtc-linux] Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Maciej W. Rozycki]

On Wed, 27 Aug 2008, Steven A. Falco wrote:

> Add support for M41T65 Real Time Clock chip
> 
> Signed-off-by: Steven A. Falco <sfalco@harris.com>
> ---
> 
> Fair enough.  Here is a sorted version.

 You've had a good idea to spell out M41T65 in full -- it makes it more 
prominent.  Thanks.

  Maciej

Re: [rtc-linux] Re: [RFC] Driver for Real Time Clock chip ST M41T65

[Alessandro Zummo]

On Wed, 27 Aug 2008 13:22:05 -0400
"Steven A. Falco" <sfalco@harris.com> wrote:

> Add support for M41T65 Real Time Clock chip
> 
> Signed-off-by: Steven A. Falco <sfalco@harris.com>


 Acked-by: Alessandro Zummo <a.zummo@towertech.it>

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it