Messages in this thread | | | Date | Mon, 5 Dec 2016 23:16:55 +0100 | From | Emil Bartczak <> | Subject | Re: [PATCH 3/4] rtc: mcp795: fix month write resetting date to 1. |
| |
Hi,
On Mon, Dec 05, 2016 at 04:24:10PM +0100, Alexandre Belloni wrote: > On 05/12/2016 at 14:11:52 +0100, Emil Bartczak wrote : > > According to Microchip errata some combinations of date and month > > values may result in the date being reset to 1, even if the date > > is also written with the month (for example 31-07 or 31-08). > > As a workaround avoid writing date and month values within the same > > Write command. Instead, terminate the Write command after loading > > the date and begin a new command to write the month. In addition, > > disable the oscillator before loading the new values. This is done > > by ensuring both the ST and EXTOSC bits are cleared and waiting for > > the OSCON bit to clear. > > --- > > drivers/rtc/rtc-mcp795.c | 103 +++++++++++++++++++++++++++++++++++++---------- > > 1 file changed, 82 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/rtc/rtc-mcp795.c b/drivers/rtc/rtc-mcp795.c > > index d15072c..c9ad46c 100644 > > --- a/drivers/rtc/rtc-mcp795.c > > +++ b/drivers/rtc/rtc-mcp795.c > > @@ -21,25 +21,34 @@ > > #include <linux/spi/spi.h> > > #include <linux/rtc.h> > > #include <linux/of.h> > > +#include <linux/delay.h> > > > > /* MCP795 Instructions, see datasheet table 3-1 */ > > -#define MCP795_EEREAD 0x03 > > -#define MCP795_EEWRITE 0x02 > > -#define MCP795_EEWRDI 0x04 > > -#define MCP795_EEWREN 0x06 > > -#define MCP795_SRREAD 0x05 > > -#define MCP795_SRWRITE 0x01 > > -#define MCP795_READ 0x13 > > -#define MCP795_WRITE 0x12 > > -#define MCP795_UNLOCK 0x14 > > -#define MCP795_IDWRITE 0x32 > > -#define MCP795_IDREAD 0x33 > > -#define MCP795_CLRWDT 0x44 > > -#define MCP795_CLRRAM 0x54 > > - > > -#define MCP795_ST_BIT 0x80 > > -#define MCP795_24_BIT 0x40 > > -#define MCP795_LP_BIT 0x20 > > +#define MCP795_EEREAD 0x03 > > +#define MCP795_EEWRITE 0x02 > > +#define MCP795_EEWRDI 0x04 > > +#define MCP795_EEWREN 0x06 > > +#define MCP795_SRREAD 0x05 > > +#define MCP795_SRWRITE 0x01 > > +#define MCP795_READ 0x13 > > +#define MCP795_WRITE 0x12 > > +#define MCP795_UNLOCK 0x14 > > +#define MCP795_IDWRITE 0x32 > > +#define MCP795_IDREAD 0x33 > > +#define MCP795_CLRWDT 0x44 > > +#define MCP795_CLRRAM 0x54 > > + > > Please don't reindent, they are a separate block anyway. Ok,
> > > +/* MCP795 RTCC registers, see datasheet table 4-1 */ > > +#define MCP795_REG_SECONDS 0x01 > > +#define MCP795_REG_DAY 0x04 > > +#define MCP795_REG_MONTH 0x06 > > +#define MCP795_REG_CONTROL 0x08 > > + > > +#define MCP795_ST_BIT 0x80 > > +#define MCP795_24_BIT 0x40 > > +#define MCP795_LP_BIT 0x20 > > +#define MCP795_EXTOSC_BIT 0x08 > > +#define MCP795_OSCON_BIT 0x20 > > Please use BIT() and that is valid for the first patch too). Ok, I will fix it in the next patchset.
> > > > > static int mcp795_rtcc_read(struct device *dev, u8 addr, u8 *buf, u8 count) > > { > > @@ -94,14 +103,51 @@ static int mcp795_rtcc_set_bits(struct device *dev, u8 addr, u8 mask, u8 state) > > return ret; > > } > > > > +static int mcp795_stop_oscillator(struct device *dev) > > +{ > > + int retries = 5; > > + int ret; > > + u8 data; > > + > > + ret = mcp795_rtcc_set_bits(dev, MCP795_REG_SECONDS, MCP795_ST_BIT, 0); > > + if (ret) > > + return ret; > > + ret = mcp795_rtcc_set_bits(dev, MCP795_REG_CONTROL, MCP795_EXTOSC_BIT, 0); > > + if (ret) > > + return ret; > > + do { > > + usleep_range(700, 800); > > + ret = mcp795_rtcc_read(dev, MCP795_REG_DAY, > > + &data, sizeof(data)); > > + if (ret) > > + break; > > + if (!(data & MCP795_OSCON_BIT)) > > + break; > > + > > + } while (--retries); > > + > > + return !retries ? -EIO : ret; > > +} > > + > > +static int mcp795_start_oscillator(struct device *dev) > > +{ > > + return mcp795_rtcc_set_bits(dev, MCP795_REG_SECONDS, > > + MCP795_ST_BIT, MCP795_ST_BIT); > > You probably want to restore EXTOSC to its previous value here. I came to the conclusion that it is better to remove clearing of bit EXTOSC in function mcp795_stop_oscillator. Because regarding datasheet, after power up the RTC chip (or reset) that bit has value 0. What do you think?
> > > -- > Alexandre Belloni, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
Emil,
| |