| Message ID | 1316990533-14971-1-git-send-email-danders.dev@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
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
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>
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
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
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
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
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:
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(-)