Patchwork [v2] rtc: add initial support for mcp7941x parts

login
register
mail settings
Submitter David Anders
Date Sept. 25, 2011, 10:42 p.m.
Message ID <1316990533-14971-1-git-send-email-danders.dev@gmail.com>
Download mbox | patch
Permalink /patch/116330/
State New
Headers show

Comments

David Anders - Sept. 25, 2011, 10:42 p.m.
this patch adds initial support for the microchip mcp7941x series
of real time clocks. the mcp7941x series is generally compatible
with the ds1307 and ds1337 rtc devices from dallas semiconductor.
minor differences include a backup battery enable bit, and the
polarity of the oscillator enable bit.

Signed-off-by: David Anders <danders.dev@gmail.com>
---
 drivers/rtc/rtc-ds1307.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)
David Anders - Sept. 27, 2011, 6:28 p.m.
Andrew,

"The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/"

do you have your git tree mirrored somewhere while kernel.org is down?


Dave


On Sep 25, 5:42 pm, David Anders <danders....@gmail.com> wrote:
> this patch adds initial support for the microchip mcp7941x series
> of real time clocks. the mcp7941x series is generally compatible
> with the ds1307 and ds1337 rtc devices from dallas semiconductor.
> minor differences include a backup battery enable bit, and the
> polarity of the oscillator enable bit.
>
> Signed-off-by: David Anders <danders....@gmail.com>
> ---
>  drivers/rtc/rtc-ds1307.c |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index b2005b4..62b0763 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -34,6 +34,7 @@ enum ds_type {
>         ds_1388,
>         ds_3231,
>         m41t00,
> +       mcp7941x,
>         rx_8025,
>         // rs5c372 too?  different address...
>  };
> @@ -43,6 +44,7 @@ enum ds_type {
>  #define DS1307_REG_SECS                0x00    /* 00-59 */
>  #      define DS1307_BIT_CH            0x80
>  #      define DS1340_BIT_nEOSC         0x80
> +#      define MCP7941X_BIT_ST          0x80
>  #define DS1307_REG_MIN         0x01    /* 00-59 */
>  #define DS1307_REG_HOUR                0x02    /* 00-23, or 1-12{am,pm} */
>  #      define DS1307_BIT_12HR          0x40    /* in REG_HOUR */
> @@ -50,6 +52,7 @@ enum ds_type {
>  #      define DS1340_BIT_CENTURY_EN    0x80    /* in REG_HOUR */
>  #      define DS1340_BIT_CENTURY       0x40    /* in REG_HOUR */
>  #define DS1307_REG_WDAY                0x03    /* 01-07 */
> +#      define MCP7941X_BIT_VBATEN      0x08
>  #define DS1307_REG_MDAY                0x04    /* 01-31 */
>  #define DS1307_REG_MONTH       0x05    /* 01-12 */
>  #      define DS1337_BIT_CENTURY       0x80    /* in REG_MONTH */
> @@ -137,6 +140,8 @@ static const struct chip_desc chips[] = {
>  },
>  [m41t00] = {
>  },
> +[mcp7941x] = {
> +},
>  [rx_8025] = {
>  }, };
>
> @@ -149,6 +154,7 @@ static const struct i2c_device_id ds1307_id[] = {
>         { "ds1340", ds_1340 },
>         { "ds3231", ds_3231 },
>         { "m41t00", m41t00 },
> +       { "mcp7941x", mcp7941x },
>         { "pt7c4338", ds_1307 },
>         { "rx8025", rx_8025 },
>         { }
> @@ -365,6 +371,10 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>                 buf[DS1307_REG_HOUR] |= DS1340_BIT_CENTURY_EN
>                                 | DS1340_BIT_CENTURY;
>                 break;
> +       case mcp7941x:
> +               buf[DS1307_REG_SECS] |= MCP7941X_BIT_ST;
> +               buf[DS1307_REG_WDAY] |= MCP7941X_BIT_VBATEN;
> +               break;
>         default:
>                 break;
>         }
> @@ -809,6 +819,23 @@ read_rtc:
>                         dev_warn(&client->dev, "SET TIME!\n");
>                 }
>                 break;
> +       case mcp7941x:
> +               /* make sure that the backup battery is enabled */
> +               if (!(ds1307->regs[DS1307_REG_WDAY] & MCP7941X_BIT_VBATEN)) {
> +                       i2c_smbus_write_byte_data(client, DS1307_REG_WDAY,
> +                                       ds1307->regs[DS1307_REG_WDAY]
> +                                       | MCP7941X_BIT_VBATEN);
> +               }
> +
> +               /* clock halted?  turn it on, so clock can tick. */
> +               if (!(tmp & MCP7941X_BIT_ST)) {
> +                       i2c_smbus_write_byte_data(client, DS1307_REG_SECS,
> +                                       MCP7941X_BIT_ST);
> +                       dev_warn(&client->dev, "SET TIME!\n");
> +                       goto read_rtc;
> +               }
> +
> +               break;
>         case rx_8025:
>         case ds_1337:
>         case ds_1339:
> --
> 1.7.0.4
Wolfram Sang - Sept. 28, 2011, 5:27 p.m.
On Sun, Sep 25, 2011 at 05:42:13PM -0500, David Anders wrote:
> this patch adds initial support for the microchip mcp7941x series
> of real time clocks. the mcp7941x series is generally compatible
> with the ds1307 and ds1337 rtc devices from dallas semiconductor.
> minor differences include a backup battery enable bit, and the
> polarity of the oscillator enable bit.
> 
> Signed-off-by: David Anders <danders.dev@gmail.com>

...

> @@ -365,6 +371,10 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>  		buf[DS1307_REG_HOUR] |= DS1340_BIT_CENTURY_EN
>  				| DS1340_BIT_CENTURY;
>  		break;
> +	case mcp7941x:
> +		buf[DS1307_REG_SECS] |= MCP7941X_BIT_ST;
> +		buf[DS1307_REG_WDAY] |= MCP7941X_BIT_VBATEN;
> +		break;

Okay, now I got it :) Maybe a comment like "keep clock & battery going" would
have been nice.

>  	default:
>  		break;
>  	}
> @@ -809,6 +819,23 @@ read_rtc:
>  			dev_warn(&client->dev, "SET TIME!\n");
>  		}
>  		break;
> +	case mcp7941x:
> +		/* make sure that the backup battery is enabled */
> +		if (!(ds1307->regs[DS1307_REG_WDAY] & MCP7941X_BIT_VBATEN)) {
> +			i2c_smbus_write_byte_data(client, DS1307_REG_WDAY,
> +					ds1307->regs[DS1307_REG_WDAY]
> +					| MCP7941X_BIT_VBATEN);
> +		}
> +
> +		/* clock halted?  turn it on, so clock can tick. */
> +		if (!(tmp & MCP7941X_BIT_ST)) {
> +			i2c_smbus_write_byte_data(client, DS1307_REG_SECS,
> +					MCP7941X_BIT_ST);
> +			dev_warn(&client->dev, "SET TIME!\n");
> +			goto read_rtc;
> +		}

Here I was wondering why the register on the chip was changed but not the local
cache. Well, this is not coherently handled throughout the driver and probably
needs a more general cleanup.

Thus, for this patch:

Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>
David Anders - Sept. 28, 2011, 6:20 p.m.
Wolfram,

On Wed, Sep 28, 2011 at 12:27 PM, Wolfram Sang <w.sang@pengutronix.de>wrote:

> On Sun, Sep 25, 2011 at 05:42:13PM -0500, David Anders wrote:
> > this patch adds initial support for the microchip mcp7941x series
> > of real time clocks. the mcp7941x series is generally compatible
> > with the ds1307 and ds1337 rtc devices from dallas semiconductor.
> > minor differences include a backup battery enable bit, and the
> > polarity of the oscillator enable bit.
> >
> > Signed-off-by: David Anders <danders.dev@gmail.com>
>
> ...
>
> > @@ -365,6 +371,10 @@ static int ds1307_set_time(struct device *dev,
> struct rtc_time *t)
> >               buf[DS1307_REG_HOUR] |= DS1340_BIT_CENTURY_EN
> >                               | DS1340_BIT_CENTURY;
> >               break;
> > +     case mcp7941x:
> > +             buf[DS1307_REG_SECS] |= MCP7941X_BIT_ST;
> > +             buf[DS1307_REG_WDAY] |= MCP7941X_BIT_VBATEN;
> > +             break;
>
> Okay, now I got it :) Maybe a comment like "keep clock & battery going"
> would
> have been nice.
>
>
this is just for initial support, i have some follow up work to post. i'll
add a comment.


> >       default:
> >               break;
> >       }
> > @@ -809,6 +819,23 @@ read_rtc:
> >                       dev_warn(&client->dev, "SET TIME!\n");
> >               }
> >               break;
> > +     case mcp7941x:
> > +             /* make sure that the backup battery is enabled */
> > +             if (!(ds1307->regs[DS1307_REG_WDAY] & MCP7941X_BIT_VBATEN))
> {
> > +                     i2c_smbus_write_byte_data(client, DS1307_REG_WDAY,
> > +                                     ds1307->regs[DS1307_REG_WDAY]
> > +                                     | MCP7941X_BIT_VBATEN);
> > +             }
> > +
> > +             /* clock halted?  turn it on, so clock can tick. */
> > +             if (!(tmp & MCP7941X_BIT_ST)) {
> > +                     i2c_smbus_write_byte_data(client, DS1307_REG_SECS,
> > +                                     MCP7941X_BIT_ST);
> > +                     dev_warn(&client->dev, "SET TIME!\n");
> > +                     goto read_rtc;
> > +             }
>
> Here I was wondering why the register on the chip was changed but not the
> local
> cache. Well, this is not coherently handled throughout the driver and
> probably
> needs a more general cleanup.
>
>

yea i noticed some inconsistencies and want to try and follow the current
line of development as close as possible.

i'll be happy to submit follow up patches as the ds1307 code evolves.


> Thus, for this patch:
>
> Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>


thanks for the review. akpm has already merged the patch into linux-next.

Dave
Wolfram Sang - Sept. 28, 2011, 6:34 p.m.
Hi Dave,

(please also disable HTML messages)

>      Okay, now I got it :) Maybe a comment like "keep clock & battery going"
>      would
>      have been nice.
> 
>    this is just for initial support, i have some follow up work to post. i'll
>    add a comment.

Cool, thanks.

>    i'll be happy to submit follow up patches as the ds1307 code evolves.

I noticed some potential cleanups as well. Maybe I'll create a ds1307-branch
tomorrow...

>    thanks for the review. akpm has already merged the patch into linux-next.

Yup, but he usually collects tags nontheless.

Regards,

   Wolfram
David Anders - Sept. 28, 2011, 6:46 p.m.
Wolfram,

On Sep 28, 1:34 pm, Wolfram Sang <w.s...@pengutronix.de> wrote:
> Hi Dave,
>
> (please also disable HTML messages)
>
> >      Okay, now I got it :) Maybe a comment like "keep clock & battery going"
> >      would
> >      have been nice.
>
> >    this is just for initial support, i have some follow up work to post. i'll
> >    add a comment.
>
> Cool, thanks.
>
> >    i'll be happy to submit follow up patches as the ds1307 code evolves.
>
> I noticed some potential cleanups as well. Maybe I'll create a ds1307-branch
> tomorrow...
>

i ran checkpatch on rtc-ds1307.c and noticed a few warnings...

> >    thanks for the review. akpm has already merged the patch into linux-next.
>
> Yup, but he usually collects tags nontheless.
>

right, just wanted to make sure you were aware that it had be merged.

> Regards,
>
>    Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 |http://www.pengutronix.de/ |
>
>  signature.asc
> < 1KViewDownload

thanks
Dave
Wolfram Sang - Sept. 28, 2011, 7:36 p.m.
Dave,

> > I noticed some potential cleanups as well. Maybe I'll create a ds1307-branch
> > tomorrow...

Well, don't say tomorrow when you can do it today :)

git://git.pengutronix.de/git/wsa/linux-2.6.git ds1307

contains your patch and two things I noticed. Would be great if you could test
those. I can only compile test. Will send the patches to the list, too.

> i ran checkpatch on rtc-ds1307.c and noticed a few warnings...

I think the bigger problem is that the driver is messy in some places and might
need refactoring. Check the huge probe function, for example.

Thanks,

   Wolfram

Patch

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index b2005b4..62b0763 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -34,6 +34,7 @@  enum ds_type {
 	ds_1388,
 	ds_3231,
 	m41t00,
+	mcp7941x,
 	rx_8025,
 	// rs5c372 too?  different address...
 };
@@ -43,6 +44,7 @@  enum ds_type {
 #define DS1307_REG_SECS		0x00	/* 00-59 */
 #	define DS1307_BIT_CH		0x80
 #	define DS1340_BIT_nEOSC		0x80
+#	define MCP7941X_BIT_ST		0x80
 #define DS1307_REG_MIN		0x01	/* 00-59 */
 #define DS1307_REG_HOUR		0x02	/* 00-23, or 1-12{am,pm} */
 #	define DS1307_BIT_12HR		0x40	/* in REG_HOUR */
@@ -50,6 +52,7 @@  enum ds_type {
 #	define DS1340_BIT_CENTURY_EN	0x80	/* in REG_HOUR */
 #	define DS1340_BIT_CENTURY	0x40	/* in REG_HOUR */
 #define DS1307_REG_WDAY		0x03	/* 01-07 */
+#	define MCP7941X_BIT_VBATEN	0x08
 #define DS1307_REG_MDAY		0x04	/* 01-31 */
 #define DS1307_REG_MONTH	0x05	/* 01-12 */
 #	define DS1337_BIT_CENTURY	0x80	/* in REG_MONTH */
@@ -137,6 +140,8 @@  static const struct chip_desc chips[] = {
 },
 [m41t00] = {
 },
+[mcp7941x] = {
+},
 [rx_8025] = {
 }, };
 
@@ -149,6 +154,7 @@  static const struct i2c_device_id ds1307_id[] = {
 	{ "ds1340", ds_1340 },
 	{ "ds3231", ds_3231 },
 	{ "m41t00", m41t00 },
+	{ "mcp7941x", mcp7941x },
 	{ "pt7c4338", ds_1307 },
 	{ "rx8025", rx_8025 },
 	{ }
@@ -365,6 +371,10 @@  static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 		buf[DS1307_REG_HOUR] |= DS1340_BIT_CENTURY_EN
 				| DS1340_BIT_CENTURY;
 		break;
+	case mcp7941x:
+		buf[DS1307_REG_SECS] |= MCP7941X_BIT_ST;
+		buf[DS1307_REG_WDAY] |= MCP7941X_BIT_VBATEN;
+		break;
 	default:
 		break;
 	}
@@ -809,6 +819,23 @@  read_rtc:
 			dev_warn(&client->dev, "SET TIME!\n");
 		}
 		break;
+	case mcp7941x:
+		/* make sure that the backup battery is enabled */
+		if (!(ds1307->regs[DS1307_REG_WDAY] & MCP7941X_BIT_VBATEN)) {
+			i2c_smbus_write_byte_data(client, DS1307_REG_WDAY,
+					ds1307->regs[DS1307_REG_WDAY]
+					| MCP7941X_BIT_VBATEN);
+		}
+
+		/* clock halted?  turn it on, so clock can tick. */
+		if (!(tmp & MCP7941X_BIT_ST)) {
+			i2c_smbus_write_byte_data(client, DS1307_REG_SECS,
+					MCP7941X_BIT_ST);
+			dev_warn(&client->dev, "SET TIME!\n");
+			goto read_rtc;
+		}
+
+		break;
 	case rx_8025:
 	case ds_1337:
 	case ds_1339: