[PATCH v2 0/8] rtc: enable adjustment of clock offset

[PATCH v2 0/8] rtc: enable adjustment of clock offset

[Joshua Clayton]

Alexandre,
this is the second version of apatch set to be able to adjust the
clock on a pcf2123, but with a big addition. I am also proposing
a couple of new rtc api functions and a new sysfs file.

Patches 1-5 are very much v2 patches that address All of the
concerns you had with v1. It is my hope to get them merged without
too much more trouble.

Patches 6-8 are attemping to add a new function to the rtc class
to adjust the clock rate. I hope I am going about this the right way,
But here goes.

A number of rtc devices, such as the NXP pcf2123 include a facility
to adjust the clock in order to compensate for temperature or a
crystal, capacitor, etc, that results in the rtc clock not running
at exactly 32.768 kHz.

This patchset adds kernel and sysfs hooks to access that ability.

One datasheet suggests it might be adjusted based on input from 
a temperature sensor. I could also potentially see it being set
as part of ntp calibration.

Data sheets I have seen refer to this as a clock offset, and measure it
in parts per million (ppm), however they often reference ppm to 2 digits
of precision, which makes integer ppm less than ideal. I use parts per
billion, which more than covers the precision needed and works nicely
within 32 bits

The name "offset" came from the pcf-2123 datasheet and is used by at
least some other data sheets. I would be happy to use a different term
if someone else comes up with something more concise.
    
Changes since v1:
- Use the BIT() macro for all register bits
- Remove unneeded range checks from read/write functions
- Merge patch 3 (replace magic numbers with defines) into patch 2
- Add a proper commit message to patch 5, now patch 4
- Fix a function alignment bug.
- Move OSC_HAS_STOPPED check into pcf2123_rtc_read_time, and get rid of
  pcf2123_time_valid()
- Drop patches refactoring pcf2123 sysfs.
- Add rtc interface and rtc sysfs file for clock offset

Joshua Clayton (8):
  rtc-pcf2123: define registers and bit macros
  rtc-pcf2123: clean up reads from the chip
  rtc-pcf2123: clean up writes to the rtc chip
  rtc-pcf2123: refactor chip reset into a function
  rtc-pcf2123: avoid resetting the clock if possible
  rtc: Add functions to set and read clock offset
  rtc: implement a sysfs interface for clock offset
  rtc-pcf2123: implement read_offset and set_offset

 drivers/rtc/interface.c   |  57 ++++++++++
 drivers/rtc/rtc-pcf2123.c | 271 +++++++++++++++++++++++++++++++++-------------
 drivers/rtc/rtc-sysfs.c   |  29 +++++
 include/linux/rtc.h       |   4 +
 4 files changed, 284 insertions(+), 77 deletions(-)

-- 
2.5.0

[PATCH v2 1/8] rtc-pcf2123: define registers and bit macros

[Joshua Clayton]

Add defines for all 16 registers in the pcf2123.
Add defines for useful bits from several registers
I've tried to document all the registers, and
as best as possible, all the special bits they employ

Use BIT() wherever possible in the bit definitions

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 drivers/rtc/rtc-pcf2123.c | 50 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index ea8a31c..be3ec83 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -48,6 +48,7 @@
 
 #define DRV_VERSION "0.6"
 
+/* REGISTERS */
 #define PCF2123_REG_CTRL1	(0x00)	/* Control Register 1 */
 #define PCF2123_REG_CTRL2	(0x01)	/* Control Register 2 */
 #define PCF2123_REG_SC		(0x02)	/* datetime */
@@ -57,10 +58,53 @@
 #define PCF2123_REG_DW		(0x06)
 #define PCF2123_REG_MO		(0x07)
 #define PCF2123_REG_YR		(0x08)
+#define PCF2123_REG_ALRM_MN	(0x09)	/* Alarm Registers */
+#define PCF2123_REG_ALRM_HR	(0x0a)
+#define PCF2123_REG_ALRM_DM	(0x0b)
+#define PCF2123_REG_ALRM_DW	(0x0c)
+#define PCF2123_REG_OFFSET	(0x0d)	/* Clock Rate Offset Register */
+#define PCF2123_REG_TMR_CLKOUT	(0x0e)	/* Timer Registers */
+#define PCF2123_REG_CTDWN_TMR	(0x0f)
+
+/* PCF2123_REG_CTRL1 BITS */
+#define CTRL1_CLEAR		(0)	/* Clear */
+#define CTRL1_CORR_INT		BIT(1)	/* Correction irq enable */
+#define CTRL1_12_HOUR		BIT(2)	/* 12 hour time */
+#define CTRL1_SW_RESET	(BIT(3) | BIT(4) | BIT(6))	/* Software reset */
+#define CTRL1_STOP		BIT(5)	/* Stop the clock */
+#define CTRL1_EXT_TEST		BIT(7)	/* External clock test mode */
+
+/* PCF2123_REG_CTRL2 BITS */
+#define CTRL2_TIE		BIT(0)	/* Countdown timer irq enable */
+#define CTRL2_AIE		BIT(1)	/* Alarm irq enable */
+#define CTRL2_TF		BIT(2)	/* Countdown timer flag */
+#define CTRL2_AF		BIT(3)	/* Alarm flag */
+#define CTRL2_TI_TP		BIT(4)	/* Irq pin generates pulse */
+#define CTRL2_MSF		BIT(5)	/* Minute or second irq flag */
+#define CTRL2_SI		BIT(6)	/* Second irq enable */
+#define CTRL2_MI		BIT(7)	/* Minute irq enable */
+
+/* PCF2123_REG_SC BITS */
+#define OSC_HAS_STOPPED		BIT(7)	/* Clock has been stopped */
+
+/* PCF2123_REG_ALRM_XX BITS */
+#define ALRM_ENABLE		BIT(7)	/* MN, HR, DM, or DW alarm enable */
+
+/* PCF2123_REG_TMR_CLKOUT BITS */
+#define CD_TMR_4096KHZ		(0)	/* 4096 KHz countdown timer */
+#define CD_TMR_64HZ		(1)	/* 64 Hz countdown timer */
+#define CD_TMR_1HZ		(2)	/* 1 Hz countdown timer */
+#define CD_TMR_60th_HZ		(3)	/* 60th Hz countdown timer */
+#define CD_TMR_TE		BIT(3)	/* Countdown timer enable */
+
+/* PCF2123_REG_OFFSET BITS */
+#define OFFSET_SIGN_BIT		BIT(6)	/* 2's complement sign bit */
+#define OFFSET_COARSE		BIT(7)	/* Coarse mode offset */
+
+/* READ/WRITE ADDRESS BITS */
+#define PCF2123_WRITE		BIT(4)
+#define PCF2123_READ		(BIT(4) | BIT(7))
 
-#define PCF2123_SUBADDR		(1 << 4)
-#define PCF2123_WRITE		((0 << 7) | PCF2123_SUBADDR)
-#define PCF2123_READ		((1 << 7) | PCF2123_SUBADDR)
 
 static struct spi_driver pcf2123_driver;
 
-- 
2.5.0

[PATCH v2 2/8] rtc-pcf2123: clean up reads from the chip

[Joshua Clayton]

Put read operations into a function.
This improves modularity and readability.

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 drivers/rtc/rtc-pcf2123.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index be3ec83..4c4b3fc 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -128,12 +128,23 @@ static inline void pcf2123_delay_trec(void)
 	ndelay(30);
 }
 
+static int pcf2123_read(struct device *dev, u8 reg, u8 *rxbuf, size_t size)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	int ret;
+
+	reg |= PCF2123_READ;
+	ret = spi_write_then_read(spi, &reg, 1, rxbuf, size);
+	pcf2123_delay_trec();
+
+	return ret;
+}
+
 static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,
 			    char *buffer)
 {
-	struct spi_device *spi = to_spi_device(dev);
 	struct pcf2123_sysfs_reg *r;
-	u8 txbuf[1], rxbuf[1];
+	u8 rxbuf[1];
 	unsigned long reg;
 	int ret;
 
@@ -143,11 +154,10 @@ static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,
 	if (ret)
 		return ret;
 
-	txbuf[0] = PCF2123_READ | reg;
-	ret = spi_write_then_read(spi, txbuf, 1, rxbuf, 1);
+	ret = pcf2123_read(dev, reg, rxbuf, 1);
 	if (ret < 0)
 		return -EIO;
-	pcf2123_delay_trec();
+
 	return sprintf(buffer, "0x%x\n", rxbuf[0]);
 }
 
@@ -182,16 +192,12 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,
 
 static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
-	struct spi_device *spi = to_spi_device(dev);
-	u8 txbuf[1], rxbuf[7];
+	u8 rxbuf[7];
 	int ret;
 
-	txbuf[0] = PCF2123_READ | PCF2123_REG_SC;
-	ret = spi_write_then_read(spi, txbuf, sizeof(txbuf),
-			rxbuf, sizeof(rxbuf));
+	ret = pcf2123_read(dev, PCF2123_REG_SC, rxbuf, sizeof(rxbuf));
 	if (ret < 0)
 		return ret;
-	pcf2123_delay_trec();
 
 	tm->tm_sec = bcd2bin(rxbuf[0] & 0x7F);
 	tm->tm_min = bcd2bin(rxbuf[1] & 0x7F);
@@ -297,16 +303,12 @@ static int pcf2123_probe(struct spi_device *spi)
 	pcf2123_delay_trec();
 
 	/* See if the counter was actually stopped */
-	txbuf[0] = PCF2123_READ | PCF2123_REG_CTRL1;
-	dev_dbg(&spi->dev, "checking for presence of RTC (0x%02X)\n",
-			txbuf[0]);
-	ret = spi_write_then_read(spi, txbuf, 1 * sizeof(u8),
-					rxbuf, 2 * sizeof(u8));
+	dev_dbg(&spi->dev, "checking for presence of RTC\n");
+	ret = pcf2123_read(&spi->dev, PCF2123_REG_CTRL1, rxbuf, sizeof(rxbuf));
 	dev_dbg(&spi->dev, "received data from RTC (0x%02X 0x%02X)\n",
 			rxbuf[0], rxbuf[1]);
 	if (ret < 0)
 		goto kfree_exit;
-	pcf2123_delay_trec();
 
 	if (!(rxbuf[0] & 0x20)) {
 		dev_err(&spi->dev, "chip not found\n");
-- 
2.5.0

[PATCH v2 3/8] rtc-pcf2123: clean up writes to the rtc chip

[Joshua Clayton]

Add new functions pcf2123_write(), and pcf2123_write_reg().
Use named defines for the values being written.

This improves modularity and readability, and reduces lines of code.

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 drivers/rtc/rtc-pcf2123.c | 67 ++++++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 35 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index 4c4b3fc..0f2ce45 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -140,6 +140,27 @@ static int pcf2123_read(struct device *dev, u8 reg, u8 *rxbuf, size_t size)
 	return ret;
 }
 
+static int pcf2123_write(struct device *dev, u8 *txbuf, size_t size)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	int ret;
+
+	txbuf[0] |= PCF2123_WRITE;
+	ret = spi_write(spi, txbuf, size);
+	pcf2123_delay_trec();
+
+	return ret;
+}
+
+static int pcf2123_write_reg(struct device *dev, u8 reg, u8 val)
+{
+	u8 txbuf[2];
+
+	txbuf[0] = reg;
+	txbuf[1] = val;
+	return pcf2123_write(dev, txbuf, sizeof(txbuf));
+}
+
 static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,
 			    char *buffer)
 {
@@ -163,9 +184,7 @@ static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,
 
 static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,
 			     const char *buffer, size_t count) {
-	struct spi_device *spi = to_spi_device(dev);
 	struct pcf2123_sysfs_reg *r;
-	u8 txbuf[2];
 	unsigned long reg;
 	unsigned long val;
 
@@ -181,12 +200,9 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,
 	if (ret)
 		return ret;
 
-	txbuf[0] = PCF2123_WRITE | reg;
-	txbuf[1] = val;
-	ret = spi_write(spi, txbuf, sizeof(txbuf));
+	pcf2123_write_reg(dev, reg, val);
 	if (ret < 0)
 		return -EIO;
-	pcf2123_delay_trec();
 	return count;
 }
 
@@ -220,7 +236,6 @@ static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)
 
 static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
-	struct spi_device *spi = to_spi_device(dev);
 	u8 txbuf[8];
 	int ret;
 
@@ -231,15 +246,12 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
 			tm->tm_mday, tm->tm_mon, tm->tm_year, tm->tm_wday);
 
 	/* Stop the counter first */
-	txbuf[0] = PCF2123_WRITE | PCF2123_REG_CTRL1;
-	txbuf[1] = 0x20;
-	ret = spi_write(spi, txbuf, 2);
+	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_STOP);
 	if (ret < 0)
 		return ret;
-	pcf2123_delay_trec();
 
 	/* Set the new time */
-	txbuf[0] = PCF2123_WRITE | PCF2123_REG_SC;
+	txbuf[0] = PCF2123_REG_SC;
 	txbuf[1] = bin2bcd(tm->tm_sec & 0x7F);
 	txbuf[2] = bin2bcd(tm->tm_min & 0x7F);
 	txbuf[3] = bin2bcd(tm->tm_hour & 0x3F);
@@ -248,18 +260,14 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	txbuf[6] = bin2bcd((tm->tm_mon + 1) & 0x1F); /* rtc mn 1-12 */
 	txbuf[7] = bin2bcd(tm->tm_year < 100 ? tm->tm_year : tm->tm_year - 100);
 
-	ret = spi_write(spi, txbuf, sizeof(txbuf));
+	ret = pcf2123_write(dev, txbuf, sizeof(txbuf));
 	if (ret < 0)
 		return ret;
-	pcf2123_delay_trec();
 
 	/* Start the counter */
-	txbuf[0] = PCF2123_WRITE | PCF2123_REG_CTRL1;
-	txbuf[1] = 0x00;
-	ret = spi_write(spi, txbuf, 2);
+	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_CLEAR);
 	if (ret < 0)
 		return ret;
-	pcf2123_delay_trec();
 
 	return 0;
 }
@@ -273,7 +281,7 @@ static int pcf2123_probe(struct spi_device *spi)
 {
 	struct rtc_device *rtc;
 	struct pcf2123_plat_data *pdata;
-	u8 txbuf[2], rxbuf[2];
+	u8  rxbuf[2];
 	int ret, i;
 
 	pdata = devm_kzalloc(&spi->dev, sizeof(struct pcf2123_plat_data),
@@ -283,24 +291,16 @@ static int pcf2123_probe(struct spi_device *spi)
 	spi->dev.platform_data = pdata;
 
 	/* Send a software reset command */
-	txbuf[0] = PCF2123_WRITE | PCF2123_REG_CTRL1;
-	txbuf[1] = 0x58;
-	dev_dbg(&spi->dev, "resetting RTC (0x%02X 0x%02X)\n",
-			txbuf[0], txbuf[1]);
-	ret = spi_write(spi, txbuf, 2 * sizeof(u8));
+	dev_dbg(&spi->dev, "resetting RTC\n");
+	ret = pcf2123_write_reg(&spi->dev, PCF2123_REG_CTRL1, CTRL1_SW_RESET);
 	if (ret < 0)
 		goto kfree_exit;
-	pcf2123_delay_trec();
 
 	/* Stop the counter */
-	txbuf[0] = PCF2123_WRITE | PCF2123_REG_CTRL1;
-	txbuf[1] = 0x20;
-	dev_dbg(&spi->dev, "stopping RTC (0x%02X 0x%02X)\n",
-			txbuf[0], txbuf[1]);
-	ret = spi_write(spi, txbuf, 2 * sizeof(u8));
+	dev_dbg(&spi->dev, "stopping RTC\n");
+	ret = pcf2123_write_reg(&spi->dev, PCF2123_REG_CTRL1, CTRL1_STOP);
 	if (ret < 0)
 		goto kfree_exit;
-	pcf2123_delay_trec();
 
 	/* See if the counter was actually stopped */
 	dev_dbg(&spi->dev, "checking for presence of RTC\n");
@@ -321,12 +321,9 @@ static int pcf2123_probe(struct spi_device *spi)
 			(spi->max_speed_hz + 500) / 1000);
 
 	/* Start the counter */
-	txbuf[0] = PCF2123_WRITE | PCF2123_REG_CTRL1;
-	txbuf[1] = 0x00;
-	ret = spi_write(spi, txbuf, sizeof(txbuf));
+	ret = pcf2123_write_reg(&spi->dev, PCF2123_REG_CTRL1, CTRL1_CLEAR);
 	if (ret < 0)
 		goto kfree_exit;
-	pcf2123_delay_trec();
 
 	/* Finalize the initialization */
 	rtc = devm_rtc_device_register(&spi->dev, pcf2123_driver.driver.name,
-- 
2.5.0

[PATCH v2 4/8] rtc-pcf2123: refactor chip reset into a function

[Joshua Clayton]

Refactor chip reset items into its own function, isolating it from
the rest of the device probe.
Subsequent commits will avoid calling this code.

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 drivers/rtc/rtc-pcf2123.c | 64 ++++++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index 0f2ce45..2d886d4 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -272,6 +272,40 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return 0;
 }
 
+static int pcf2123_reset(struct device *dev)
+{
+	int ret;
+	u8  rxbuf[2];
+
+	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_SW_RESET);
+	if (ret < 0)
+		return ret;
+
+	/* Stop the counter */
+	dev_dbg(dev, "stopping RTC\n");
+	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_STOP);
+	if (ret < 0)
+		return ret;
+
+	/* See if the counter was actually stopped */
+	dev_dbg(dev, "checking for presence of RTC\n");
+	ret = pcf2123_read(dev, PCF2123_REG_CTRL1, rxbuf, sizeof(rxbuf));
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(dev, "received data from RTC (0x%02X 0x%02X)\n",
+		rxbuf[0], rxbuf[1]);
+	if (!(rxbuf[0] & CTRL1_STOP))
+		return -ENODEV;
+
+	/* Start the counter */
+	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_CLEAR);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static const struct rtc_class_ops pcf2123_rtc_ops = {
 	.read_time	= pcf2123_rtc_read_time,
 	.set_time	= pcf2123_rtc_set_time,
@@ -281,7 +315,6 @@ static int pcf2123_probe(struct spi_device *spi)
 {
 	struct rtc_device *rtc;
 	struct pcf2123_plat_data *pdata;
-	u8  rxbuf[2];
 	int ret, i;
 
 	pdata = devm_kzalloc(&spi->dev, sizeof(struct pcf2123_plat_data),
@@ -290,29 +323,9 @@ static int pcf2123_probe(struct spi_device *spi)
 		return -ENOMEM;
 	spi->dev.platform_data = pdata;
 
-	/* Send a software reset command */
-	dev_dbg(&spi->dev, "resetting RTC\n");
-	ret = pcf2123_write_reg(&spi->dev, PCF2123_REG_CTRL1, CTRL1_SW_RESET);
-	if (ret < 0)
-		goto kfree_exit;
-
-	/* Stop the counter */
-	dev_dbg(&spi->dev, "stopping RTC\n");
-	ret = pcf2123_write_reg(&spi->dev, PCF2123_REG_CTRL1, CTRL1_STOP);
-	if (ret < 0)
-		goto kfree_exit;
-
-	/* See if the counter was actually stopped */
-	dev_dbg(&spi->dev, "checking for presence of RTC\n");
-	ret = pcf2123_read(&spi->dev, PCF2123_REG_CTRL1, rxbuf, sizeof(rxbuf));
-	dev_dbg(&spi->dev, "received data from RTC (0x%02X 0x%02X)\n",
-			rxbuf[0], rxbuf[1]);
-	if (ret < 0)
-		goto kfree_exit;
-
-	if (!(rxbuf[0] & 0x20)) {
+	ret = pcf2123_reset(&spi->dev);
+	if (ret < 0) {
 		dev_err(&spi->dev, "chip not found\n");
-		ret = -ENODEV;
 		goto kfree_exit;
 	}
 
@@ -320,11 +333,6 @@ static int pcf2123_probe(struct spi_device *spi)
 	dev_info(&spi->dev, "spiclk %u KHz.\n",
 			(spi->max_speed_hz + 500) / 1000);
 
-	/* Start the counter */
-	ret = pcf2123_write_reg(&spi->dev, PCF2123_REG_CTRL1, CTRL1_CLEAR);
-	if (ret < 0)
-		goto kfree_exit;
-
 	/* Finalize the initialization */
 	rtc = devm_rtc_device_register(&spi->dev, pcf2123_driver.driver.name,
 			&pcf2123_rtc_ops, THIS_MODULE);
-- 
2.5.0

[PATCH v2 5/8] rtc-pcf2123: avoid resetting the clock if possible

[Joshua Clayton]

pcf2123 data sheet recommends a software reset when the chip
is first powered on. This change avoids resetting the chip
every time the driver is loaded, which has some negative effects.

There are several registers including a clock rate adjustment that really
should survive a reload of the driver (or reboot).

In addition, stopping and restarting the clock to verify the chip is
there is not a good thing once the time is set.

According to the data sheet, the seconds register has a 1 in
the high bit when the voltage has gotten low. We check for this
condition, as well as whether the time retrieved from the chip is
valid. We reset the rtc only if the time is not reliable and valid.
This is sufficient for checking for the presence of the chip,
as either all zeros or all 0xff will result in an invalid time/date

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 drivers/rtc/rtc-pcf2123.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index 2d886d4..d9a44ad 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -215,6 +215,11 @@ static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	if (ret < 0)
 		return ret;
 
+	if (rxbuf[0] & OSC_HAS_STOPPED) {
+		dev_info(dev, "clock was stopped. Time is not valid\n");
+		return -EINVAL;
+	}
+
 	tm->tm_sec = bcd2bin(rxbuf[0] & 0x7F);
 	tm->tm_min = bcd2bin(rxbuf[1] & 0x7F);
 	tm->tm_hour = bcd2bin(rxbuf[2] & 0x3F); /* rtc hr 0-23 */
@@ -314,6 +319,7 @@ static const struct rtc_class_ops pcf2123_rtc_ops = {
 static int pcf2123_probe(struct spi_device *spi)
 {
 	struct rtc_device *rtc;
+	struct rtc_time tm;
 	struct pcf2123_plat_data *pdata;
 	int ret, i;
 
@@ -323,10 +329,13 @@ static int pcf2123_probe(struct spi_device *spi)
 		return -ENOMEM;
 	spi->dev.platform_data = pdata;
 
-	ret = pcf2123_reset(&spi->dev);
+	ret = pcf2123_rtc_read_time(&spi->dev, &tm);
 	if (ret < 0) {
-		dev_err(&spi->dev, "chip not found\n");
-		goto kfree_exit;
+		ret = pcf2123_reset(&spi->dev);
+		if (ret < 0) {
+			dev_err(&spi->dev, "chip not found\n");
+			goto kfree_exit;
+		}
 	}
 
 	dev_info(&spi->dev, "chip found, driver version " DRV_VERSION "\n");
-- 
2.5.0

[PATCH v2 6/8] rtc: Add functions to set and read clock offset

[Joshua Clayton]

A number of rtc devices, such as the NXP pcf2123 include a facility
to adjust the clock in order to compensate for temperature or a
crystal, capacitor, etc, that results in the rtc clock not running
at exactly 32.768 kHz.

Data sheets I have seen refer to this as a clock offset, and measure it
in parts per million (ppm), however they often reference ppm to 2 digits
of precision, which makes integer ppm less than ideal.

We use parts per billion, which more than covers the precision needed
and works nicely within 32 bits

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 drivers/rtc/interface.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/rtc.h     |  4 ++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 5836751..57cd928 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -939,4 +939,61 @@ void rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer *timer)
 	mutex_unlock(&rtc->ops_lock);
 }
 
+/**
+ * rtc_read_offset - Read the amount of rtc offset in parts per billion
+ * @ rtc: rtc device to be used
+ * @ offset: the offset in parts per billion
+ *
+ * see below for details.
+ *
+ * Kernel interface to read rtc clock offset
+ * Returns 0 on success, or a negative number on error.
+ * If the rtc offset is not setable (or not implemented), return 0 and put
+ * 0 in the offset value;
+ */
+int rtc_read_offset(struct rtc_device *rtc, long *offset)
+{
+	int ret = 0;
+
+	if (!rtc->ops)
+		return -ENODEV;
+
+	if (!rtc->ops->set_offset) {
+		offset = 0;
+		return 0;
+	}
+
+	mutex_lock(&rtc->ops_lock);
+	ret = rtc->ops->read_offset(rtc->dev.parent, offset);
+	mutex_unlock(&rtc->ops_lock);
+	return ret;
+}
 
+/**
+ * rtc_set_offset - Adjusts the duration of the average second
+ * @ rtc: rtc device to be used
+ * @ offset: the offset in parts per billion
+ *
+ * Some rtc's allow an adjustment to the average duration of a second
+ * to compensate for differences in the actual clock rate due to temperature,
+ * the crystal, capacitor, etc.
+ *
+ * Kernel interface to adjust an rtc clock offset.
+ * Return 0 on success, or a negative number on error.
+ * If the rtc offset is not setable (or not implemented), return -EINVAL
+ */
+int rtc_set_offset(struct rtc_device *rtc, long offset)
+{
+	int ret = 0;
+
+	if (!rtc->ops)
+		return -ENODEV;
+
+	if (!rtc->ops->set_offset)
+		return -EINVAL;
+
+	mutex_lock(&rtc->ops_lock);
+	ret = rtc->ops->set_offset(rtc->dev.parent, offset);
+	mutex_unlock(&rtc->ops_lock);
+	return ret;
+}
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 3359f04..b693ada 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -89,6 +89,8 @@ struct rtc_class_ops {
 	int (*set_mmss)(struct device *, unsigned long secs);
 	int (*read_callback)(struct device *, int data);
 	int (*alarm_irq_enable)(struct device *, unsigned int enabled);
+	int (*read_offset)(struct device *, long *offset);
+	int (*set_offset)(struct device *, long offset);
 };
 
 #define RTC_DEVICE_NAME_SIZE 20
@@ -208,6 +210,8 @@ void rtc_timer_init(struct rtc_timer *timer, void (*f)(void *p), void *data);
 int rtc_timer_start(struct rtc_device *rtc, struct rtc_timer *timer,
 		    ktime_t expires, ktime_t period);
 void rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer *timer);
+int rtc_read_offset(struct rtc_device *rtc, long *offset);
+int rtc_set_offset(struct rtc_device *rtc, long offset);
 void rtc_timer_do_work(struct work_struct *work);
 
 static inline bool is_leap_year(unsigned int year)
-- 
2.5.0

[PATCH v2 7/8] rtc: implement a sysfs interface for clock offset

[Joshua Clayton]

The file is called "offset", and may be set and read in decimal.
For rtcs that do not have read_offset or set_offset implemented,
read always returns zero and write will return -EINVAL.

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 drivers/rtc/interface.c |  2 +-
 drivers/rtc/rtc-sysfs.c | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 57cd928..c064eb9 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -959,7 +959,7 @@ int rtc_read_offset(struct rtc_device *rtc, long *offset)
 		return -ENODEV;
 
 	if (!rtc->ops->set_offset) {
-		offset = 0;
+		*offset = 0;
 		return 0;
 	}
 
diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
index 7273855..622a0dc 100644
--- a/drivers/rtc/rtc-sysfs.c
+++ b/drivers/rtc/rtc-sysfs.c
@@ -211,6 +211,34 @@ wakealarm_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(wakealarm);
 
+static ssize_t
+offset_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	ssize_t retval;
+	long offset;
+
+	retval = rtc_read_offset(to_rtc_device(dev), &offset);
+	if (retval == 0)
+		retval = sprintf(buf, "%ld\n", offset);
+
+	return retval;
+}
+
+static ssize_t
+offset_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t n)
+{
+	ssize_t retval;
+	long offset;
+
+	retval = kstrtol(buf, 10, &offset);
+	if (retval == 0)
+		retval = rtc_set_offset(to_rtc_device(dev), offset);
+
+	return (retval < 0) ? retval : n;
+}
+static DEVICE_ATTR_RW(offset);
+
 static struct attribute *rtc_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_date.attr,
@@ -219,6 +247,7 @@ static struct attribute *rtc_attrs[] = {
 	&dev_attr_max_user_freq.attr,
 	&dev_attr_hctosys.attr,
 	&dev_attr_wakealarm.attr,
+	&dev_attr_offset.attr,
 	NULL,
 };
 
-- 
2.5.0

Re: [PATCH v2 7/8] rtc: implement a sysfs interface for clock offset

[Alexandre Belloni]

Hi,

On 04/01/2016 at 10:31:25 -0800, Joshua Clayton wrote :
> The file is called "offset", and may be set and read in decimal.
> For rtcs that do not have read_offset or set_offset implemented,
> read always returns zero and write will return -EINVAL.
> 
Can you expand rtc_attr_is_visible() instead of having the offset file
always present?

Also, you need to expand Documentation/rtc.txt, section SYSFS INTERFACE.

I'm fine with having parts per billion as the unit and I hope we won't
ever need anything more precise :)

> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> ---
>  drivers/rtc/interface.c |  2 +-
>  drivers/rtc/rtc-sysfs.c | 29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 57cd928..c064eb9 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -959,7 +959,7 @@ int rtc_read_offset(struct rtc_device *rtc, long *offset)
>  		return -ENODEV;
>  
>  	if (!rtc->ops->set_offset) {
> -		offset = 0;
> +		*offset = 0;
>  		return 0;
>  	}

>  
> diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
> index 7273855..622a0dc 100644
> --- a/drivers/rtc/rtc-sysfs.c
> +++ b/drivers/rtc/rtc-sysfs.c
> @@ -211,6 +211,34 @@ wakealarm_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RW(wakealarm);
>  
> +static ssize_t
> +offset_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	ssize_t retval;
> +	long offset;
> +
> +	retval = rtc_read_offset(to_rtc_device(dev), &offset);
> +	if (retval == 0)
> +		retval = sprintf(buf, "%ld\n", offset);
> +
> +	return retval;
> +}
> +
> +static ssize_t
> +offset_store(struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t n)
> +{
> +	ssize_t retval;
> +	long offset;
> +
> +	retval = kstrtol(buf, 10, &offset);
> +	if (retval == 0)
> +		retval = rtc_set_offset(to_rtc_device(dev), offset);
> +
> +	return (retval < 0) ? retval : n;
> +}
> +static DEVICE_ATTR_RW(offset);
> +
>  static struct attribute *rtc_attrs[] = {
>  	&dev_attr_name.attr,
>  	&dev_attr_date.attr,
> @@ -219,6 +247,7 @@ static struct attribute *rtc_attrs[] = {
>  	&dev_attr_max_user_freq.attr,
>  	&dev_attr_hctosys.attr,
>  	&dev_attr_wakealarm.attr,
> +	&dev_attr_offset.attr,
>  	NULL,
>  };
>  

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v2 7/8] rtc: implement a sysfs interface for clock offset

[Joshua Clayton]

On Sun, 31 Jan 2016 12:41:15 +0100
Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:

> Hi,
> 
> On 04/01/2016 at 10:31:25 -0800, Joshua Clayton wrote :
> > The file is called "offset", and may be set and read in decimal.
> > For rtcs that do not have read_offset or set_offset implemented,
> > read always returns zero and write will return -EINVAL.
> > 
> Can you expand rtc_attr_is_visible() instead of having the offset file
> always present?

Yes, Absolutely.
I wanted to do something like this, but didn't know is_visible
existed... and no wonder. Its in attribute_group

> 
> Also, you need to expand Documentation/rtc.txt, section SYSFS
> INTERFACE.
> 
OK. Will do.

> I'm fine with having parts per billion as the unit and I hope we won't
> ever need anything more precise :)
> 
It is orders of magnitude more than needed at present
...and no will ever need more than 640k of memory... right?

Re: [PATCH v2 7/8] rtc: implement a sysfs interface for clock offset

[Alexandre Belloni]

On 01/02/2016 at 12:56:48 -0800, Joshua Clayton wrote :
> > I'm fine with having parts per billion as the unit and I hope we won't
> > ever need anything more precise :)
> > 
> It is orders of magnitude more than needed at present

Yeah, until we get an RTC with an offset precision that has 4 decimals in ppm.

> ...and no will ever need more than 640k of memory... right?

Right :)

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 1/3] rtc: Add functions to set and read rtc offset

[Joshua Clayton]

A number of rtc devices, such as the NXP pcf2123 include a facility
to adjust the clock in order to compensate for temperature or a
crystal, capacitor, etc, that results in the rtc clock not running
at exactly 32.768 kHz.

Data sheets I have seen refer to this as a clock offset, and measure it
in parts per million, however they often reference ppm to 2 digits of
precision, which makes integer ppm less than ideal.

We use parts per billion, which more than covers the precision needed
and works nicely within 32 bits

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
Changes since v2:
- patches 1 - 5 from v2 were merged, so 6, 7 , and 8 become 1, 2, 3
- Added documentation of the sysfs interface in Documenbtation/rtc.txt
- Added code so that the offset attrbute does not appear on rtcs that
  do not support it.

 drivers/rtc/interface.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/rtc.h     |  4 ++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 5836751..c064eb9 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -939,4 +939,61 @@ void rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer *timer)
 	mutex_unlock(&rtc->ops_lock);
 }
 
+/**
+ * rtc_read_offset - Read the amount of rtc offset in parts per billion
+ * @ rtc: rtc device to be used
+ * @ offset: the offset in parts per billion
+ *
+ * see below for details.
+ *
+ * Kernel interface to read rtc clock offset
+ * Returns 0 on success, or a negative number on error.
+ * If the rtc offset is not setable (or not implemented), return 0 and put
+ * 0 in the offset value;
+ */
+int rtc_read_offset(struct rtc_device *rtc, long *offset)
+{
+	int ret = 0;
+
+	if (!rtc->ops)
+		return -ENODEV;
+
+	if (!rtc->ops->set_offset) {
+		*offset = 0;
+		return 0;
+	}
+
+	mutex_lock(&rtc->ops_lock);
+	ret = rtc->ops->read_offset(rtc->dev.parent, offset);
+	mutex_unlock(&rtc->ops_lock);
+	return ret;
+}
 
+/**
+ * rtc_set_offset - Adjusts the duration of the average second
+ * @ rtc: rtc device to be used
+ * @ offset: the offset in parts per billion
+ *
+ * Some rtc's allow an adjustment to the average duration of a second
+ * to compensate for differences in the actual clock rate due to temperature,
+ * the crystal, capacitor, etc.
+ *
+ * Kernel interface to adjust an rtc clock offset.
+ * Return 0 on success, or a negative number on error.
+ * If the rtc offset is not setable (or not implemented), return -EINVAL
+ */
+int rtc_set_offset(struct rtc_device *rtc, long offset)
+{
+	int ret = 0;
+
+	if (!rtc->ops)
+		return -ENODEV;
+
+	if (!rtc->ops->set_offset)
+		return -EINVAL;
+
+	mutex_lock(&rtc->ops_lock);
+	ret = rtc->ops->set_offset(rtc->dev.parent, offset);
+	mutex_unlock(&rtc->ops_lock);
+	return ret;
+}
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 3359f04..b693ada 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -89,6 +89,8 @@ struct rtc_class_ops {
 	int (*set_mmss)(struct device *, unsigned long secs);
 	int (*read_callback)(struct device *, int data);
 	int (*alarm_irq_enable)(struct device *, unsigned int enabled);
+	int (*read_offset)(struct device *, long *offset);
+	int (*set_offset)(struct device *, long offset);
 };
 
 #define RTC_DEVICE_NAME_SIZE 20
@@ -208,6 +210,8 @@ void rtc_timer_init(struct rtc_timer *timer, void (*f)(void *p), void *data);
 int rtc_timer_start(struct rtc_device *rtc, struct rtc_timer *timer,
 		    ktime_t expires, ktime_t period);
 void rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer *timer);
+int rtc_read_offset(struct rtc_device *rtc, long *offset);
+int rtc_set_offset(struct rtc_device *rtc, long offset);
 void rtc_timer_do_work(struct work_struct *work);
 
 static inline bool is_leap_year(unsigned int year)
-- 
2.5.0

Re: [PATCH v3 1/3] rtc: Add functions to set and read rtc offset

[Alexandre Belloni]

Hi,

On 03/02/2016 at 09:16:42 -0800, Joshua Clayton wrote :
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 5836751..c064eb9 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -939,4 +939,61 @@ void rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer *timer)
>  	mutex_unlock(&rtc->ops_lock);
>  }
>  
> +/**
> + * rtc_read_offset - Read the amount of rtc offset in parts per billion
> + * @ rtc: rtc device to be used
> + * @ offset: the offset in parts per billion
> + *
> + * see below for details.
> + *
> + * Kernel interface to read rtc clock offset
> + * Returns 0 on success, or a negative number on error.
> + * If the rtc offset is not setable (or not implemented), return 0 and put
> + * 0 in the offset value;
> + */
> +int rtc_read_offset(struct rtc_device *rtc, long *offset)
> +{
> +	int ret = 0;
> +
> +	if (!rtc->ops)
> +		return -ENODEV;
> +
> +	if (!rtc->ops->set_offset) {
> +		*offset = 0;
> +		return 0;
> +	}
> +

I should have been clearer but this is not necessary anymore since the
sysfs interface will not always be present but you
should probably test rtc->ops->read_offset instead.

> +	mutex_lock(&rtc->ops_lock);
> +	ret = rtc->ops->read_offset(rtc->dev.parent, offset);
> +	mutex_unlock(&rtc->ops_lock);
> +	return ret;
> +}
>  
> +/**
> + * rtc_set_offset - Adjusts the duration of the average second
> + * @ rtc: rtc device to be used
> + * @ offset: the offset in parts per billion
> + *
> + * Some rtc's allow an adjustment to the average duration of a second
> + * to compensate for differences in the actual clock rate due to temperature,
> + * the crystal, capacitor, etc.
> + *
> + * Kernel interface to adjust an rtc clock offset.
> + * Return 0 on success, or a negative number on error.
> + * If the rtc offset is not setable (or not implemented), return -EINVAL
> + */
> +int rtc_set_offset(struct rtc_device *rtc, long offset)
> +{
> +	int ret = 0;
> +
> +	if (!rtc->ops)
> +		return -ENODEV;
> +
> +	if (!rtc->ops->set_offset)
> +		return -EINVAL;
> +
> +	mutex_lock(&rtc->ops_lock);
> +	ret = rtc->ops->set_offset(rtc->dev.parent, offset);
> +	mutex_unlock(&rtc->ops_lock);
> +	return ret;
> +}
> diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> index 3359f04..b693ada 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -89,6 +89,8 @@ struct rtc_class_ops {
>  	int (*set_mmss)(struct device *, unsigned long secs);
>  	int (*read_callback)(struct device *, int data);
>  	int (*alarm_irq_enable)(struct device *, unsigned int enabled);
> +	int (*read_offset)(struct device *, long *offset);
> +	int (*set_offset)(struct device *, long offset);
>  };
>  
>  #define RTC_DEVICE_NAME_SIZE 20
> @@ -208,6 +210,8 @@ void rtc_timer_init(struct rtc_timer *timer, void (*f)(void *p), void *data);
>  int rtc_timer_start(struct rtc_device *rtc, struct rtc_timer *timer,
>  		    ktime_t expires, ktime_t period);
>  void rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer *timer);
> +int rtc_read_offset(struct rtc_device *rtc, long *offset);
> +int rtc_set_offset(struct rtc_device *rtc, long offset);
>  void rtc_timer_do_work(struct work_struct *work);
>  
>  static inline bool is_leap_year(unsigned int year)
> -- 
> 2.5.0
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v3 1/3] rtc: Add functions to set and read rtc offset

[Joshua Clayton]

On Thu, 4 Feb 2016 23:07:54 +0100
Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:

> Hi,
> 
> On 03/02/2016 at 09:16:42 -0800, Joshua Clayton wrote :
> > diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> > index 5836751..c064eb9 100644
> > --- a/drivers/rtc/interface.c
> > +++ b/drivers/rtc/interface.c
> > @@ -939,4 +939,61 @@ void rtc_timer_cancel(struct rtc_device *rtc,
> > struct rtc_timer *timer) mutex_unlock(&rtc->ops_lock);
> >  }
> >  
> > +/**
> > + * rtc_read_offset - Read the amount of rtc offset in parts per
> > billion
> > + * @ rtc: rtc device to be used
> > + * @ offset: the offset in parts per billion
> > + *
> > + * see below for details.
> > + *
> > + * Kernel interface to read rtc clock offset
> > + * Returns 0 on success, or a negative number on error.
> > + * If the rtc offset is not setable (or not implemented), return 0
> > and put
> > + * 0 in the offset value;
> > + */
> > +int rtc_read_offset(struct rtc_device *rtc, long *offset)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!rtc->ops)
> > +		return -ENODEV;
> > +
> > +	if (!rtc->ops->set_offset) {
> > +		*offset = 0;
> > +		return 0;
> > +	}
> > +
> 
> I should have been clearer but this is not necessary anymore since the
> sysfs interface will not always be present but you
> should probably test rtc->ops->read_offset instead.
>
I left it because the kernel API is still there even if the sysfs
file is not...
but yeah, you are right. I'll fix the check, and the formatting
in the other patch.

> > +	mutex_lock(&rtc->ops_lock);
> > +	ret = rtc->ops->read_offset(rtc->dev.parent, offset);
> > +	mutex_unlock(&rtc->ops_lock);
> > +	return ret;
> > +}
> >  
> > +/**
> > + * rtc_set_offset - Adjusts the duration of the average second
> > + * @ rtc: rtc device to be used
> > + * @ offset: the offset in parts per billion
> > + *
> > + * Some rtc's allow an adjustment to the average duration of a
> > second
> > + * to compensate for differences in the actual clock rate due to
> > temperature,
> > + * the crystal, capacitor, etc.
> > + *
> > + * Kernel interface to adjust an rtc clock offset.
> > + * Return 0 on success, or a negative number on error.
> > + * If the rtc offset is not setable (or not implemented), return
> > -EINVAL
> > + */
> > +int rtc_set_offset(struct rtc_device *rtc, long offset)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!rtc->ops)
> > +		return -ENODEV;
> > +
> > +	if (!rtc->ops->set_offset)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&rtc->ops_lock);
> > +	ret = rtc->ops->set_offset(rtc->dev.parent, offset);
> > +	mutex_unlock(&rtc->ops_lock);
> > +	return ret;
> > +}
> > diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> > index 3359f04..b693ada 100644
> > --- a/include/linux/rtc.h
> > +++ b/include/linux/rtc.h
> > @@ -89,6 +89,8 @@ struct rtc_class_ops {
> >  	int (*set_mmss)(struct device *, unsigned long secs);
> >  	int (*read_callback)(struct device *, int data);
> >  	int (*alarm_irq_enable)(struct device *, unsigned int
> > enabled);
> > +	int (*read_offset)(struct device *, long *offset);
> > +	int (*set_offset)(struct device *, long offset);
> >  };
> >  
> >  #define RTC_DEVICE_NAME_SIZE 20
> > @@ -208,6 +210,8 @@ void rtc_timer_init(struct rtc_timer *timer,
> > void (*f)(void *p), void *data); int rtc_timer_start(struct
> > rtc_device *rtc, struct rtc_timer *timer, ktime_t expires, ktime_t
> > period); void rtc_timer_cancel(struct rtc_device *rtc, struct
> > rtc_timer *timer); +int rtc_read_offset(struct rtc_device *rtc,
> > long *offset); +int rtc_set_offset(struct rtc_device *rtc, long
> > offset); void rtc_timer_do_work(struct work_struct *work);
> >  
> >  static inline bool is_leap_year(unsigned int year)
> > -- 
> > 2.5.0
> > 
> 

Re: [PATCH v3 1/3] rtc: Add functions to set and read rtc offset

[Alexandre Belloni]

On 04/02/2016 at 15:32:42 -0800, Joshua Clayton wrote :
> > > +int rtc_read_offset(struct rtc_device *rtc, long *offset)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (!rtc->ops)
> > > +		return -ENODEV;
> > > +
> > > +	if (!rtc->ops->set_offset) {
> > > +		*offset = 0;
> > > +		return 0;
> > > +	}
> > > +
> > 
> > I should have been clearer but this is not necessary anymore since the
> > sysfs interface will not always be present but you
> > should probably test rtc->ops->read_offset instead.
> >
> I left it because the kernel API is still there even if the sysfs
> file is not...
> but yeah, you are right. I'll fix the check, and the formatting
> in the other patch.
> 

Yeah but from inside the kernel, I feel that it is more useful to know
that the value doesn't exist instead of having 0.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v4 1/3] rtc: Add functions to set and read rtc offset

[Joshua Clayton]

A number of rtc devices, such as the NXP pcf2123 include a facility
to adjust the clock in order to compensate for temperature or a
crystal, capacitor, etc, that results in the rtc clock not running
at exactly 32.768 kHz.

Data sheets I have seen refer to this as a clock offset, and measure it
in parts per million, however they often reference ppm to 2 digits of
precision, which makes integer ppm less than ideal.

We use parts per billion, which more than covers the precision needed
and works nicely within 32 bits

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 drivers/rtc/interface.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/rtc.h     |  4 ++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 5836751..9ef5f6f 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -939,4 +939,58 @@ void rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer *timer)
 	mutex_unlock(&rtc->ops_lock);
 }
 
+/**
+ * rtc_read_offset - Read the amount of rtc offset in parts per billion
+ * @ rtc: rtc device to be used
+ * @ offset: the offset in parts per billion
+ *
+ * see below for details.
+ *
+ * Kernel interface to read rtc clock offset
+ * Returns 0 on success, or a negative number on error.
+ * If read_offset() is not implemented for the rtc, return -EINVAL
+ */
+int rtc_read_offset(struct rtc_device *rtc, long *offset)
+{
+	int ret;
+
+	if (!rtc->ops)
+		return -ENODEV;
+
+	if (!rtc->ops->read_offset)
+		return -EINVAL;
+
+	mutex_lock(&rtc->ops_lock);
+	ret = rtc->ops->read_offset(rtc->dev.parent, offset);
+	mutex_unlock(&rtc->ops_lock);
+	return ret;
+}
 
+/**
+ * rtc_set_offset - Adjusts the duration of the average second
+ * @ rtc: rtc device to be used
+ * @ offset: the offset in parts per billion
+ *
+ * Some rtc's allow an adjustment to the average duration of a second
+ * to compensate for differences in the actual clock rate due to temperature,
+ * the crystal, capacitor, etc.
+ *
+ * Kernel interface to adjust an rtc clock offset.
+ * Return 0 on success, or a negative number on error.
+ * If the rtc offset is not setable (or not implemented), return -EINVAL
+ */
+int rtc_set_offset(struct rtc_device *rtc, long offset)
+{
+	int ret;
+
+	if (!rtc->ops)
+		return -ENODEV;
+
+	if (!rtc->ops->set_offset)
+		return -EINVAL;
+
+	mutex_lock(&rtc->ops_lock);
+	ret = rtc->ops->set_offset(rtc->dev.parent, offset);
+	mutex_unlock(&rtc->ops_lock);
+	return ret;
+}
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 3359f04..b693ada 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -89,6 +89,8 @@ struct rtc_class_ops {
 	int (*set_mmss)(struct device *, unsigned long secs);
 	int (*read_callback)(struct device *, int data);
 	int (*alarm_irq_enable)(struct device *, unsigned int enabled);
+	int (*read_offset)(struct device *, long *offset);
+	int (*set_offset)(struct device *, long offset);
 };
 
 #define RTC_DEVICE_NAME_SIZE 20
@@ -208,6 +210,8 @@ void rtc_timer_init(struct rtc_timer *timer, void (*f)(void *p), void *data);
 int rtc_timer_start(struct rtc_device *rtc, struct rtc_timer *timer,
 		    ktime_t expires, ktime_t period);
 void rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer *timer);
+int rtc_read_offset(struct rtc_device *rtc, long *offset);
+int rtc_set_offset(struct rtc_device *rtc, long offset);
 void rtc_timer_do_work(struct work_struct *work);
 
 static inline bool is_leap_year(unsigned int year)
-- 
2.5.0

[PATCH v4 2/3] rtc: implement a sysfs interface for clock offset

[Joshua Clayton]

clock offset may be set and read in decimal parts per billion
attribute is /sys/class/rtc/rtcN/offset
The attribute is only visible for rtcs that have set_offset implemented.

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 Documentation/rtc.txt   |  6 ++++++
 drivers/rtc/rtc-sysfs.c | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/Documentation/rtc.txt b/Documentation/rtc.txt
index 8446f1e..ddc3660 100644
--- a/Documentation/rtc.txt
+++ b/Documentation/rtc.txt
@@ -157,6 +157,12 @@ wakealarm:	 The time at which the clock will generate a system wakeup
 		 the epoch by default, or if there's a leading +, seconds in the
 		 future, or if there is a leading +=, seconds ahead of the current
 		 alarm.
+offset:		 The amount which the rtc clock has been adjusted in firmware.
+		 Visible only if the driver supports clock offset adjustment.
+		 The unit is parts per billion, i.e. The number of clock ticks
+		 which are added to or removed from the rtc's base clock per
+		 billion ticks. A positive value makes a day pass more slowly,
+		 longer, and a negative value makes a day pass more quickly.
 
 IOCTL INTERFACE
 ---------------
diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
index 463e286..63b9fb1 100644
--- a/drivers/rtc/rtc-sysfs.c
+++ b/drivers/rtc/rtc-sysfs.c
@@ -218,6 +218,34 @@ wakealarm_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(wakealarm);
 
+static ssize_t
+offset_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	ssize_t retval;
+	long offset;
+
+	retval = rtc_read_offset(to_rtc_device(dev), &offset);
+	if (retval == 0)
+		retval = sprintf(buf, "%ld\n", offset);
+
+	return retval;
+}
+
+static ssize_t
+offset_store(struct device *dev, struct device_attribute *attr,
+	     const char *buf, size_t n)
+{
+	ssize_t retval;
+	long offset;
+
+	retval = kstrtol(buf, 10, &offset);
+	if (retval == 0)
+		retval = rtc_set_offset(to_rtc_device(dev), offset);
+
+	return (retval < 0) ? retval : n;
+}
+static DEVICE_ATTR_RW(offset);
+
 static struct attribute *rtc_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_date.attr,
@@ -226,6 +254,7 @@ static struct attribute *rtc_attrs[] = {
 	&dev_attr_max_user_freq.attr,
 	&dev_attr_hctosys.attr,
 	&dev_attr_wakealarm.attr,
+	&dev_attr_offset.attr,
 	NULL,
 };
 
@@ -249,9 +278,13 @@ static umode_t rtc_attr_is_visible(struct kobject *kobj,
 	struct rtc_device *rtc = to_rtc_device(dev);
 	umode_t mode = attr->mode;
 
-	if (attr == &dev_attr_wakealarm.attr)
+	if (attr == &dev_attr_wakealarm.attr) {
 		if (!rtc_does_wakealarm(rtc))
 			mode = 0;
+	} else if (attr == &dev_attr_offset.attr) {
+		if (!rtc->ops->set_offset)
+			mode = 0;
+	}
 
 	return mode;
 }
-- 
2.5.0

[PATCH v4 3/3] rtc-pcf2123: implement read_offset and set_offset

[Joshua Clayton]

pcf2123 has an offset register, which can be used to make minor
adjustments to the clock rate to compensate for temperature or
a crystal that is not exactly right.

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 drivers/rtc/rtc-pcf2123.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index d9a44ad..da27738 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -100,6 +100,7 @@
 /* PCF2123_REG_OFFSET BITS */
 #define OFFSET_SIGN_BIT		BIT(6)	/* 2's complement sign bit */
 #define OFFSET_COARSE		BIT(7)	/* Coarse mode offset */
+#define OFFSET_STEP		(2170)	/* Offset step in parts per billion */
 
 /* READ/WRITE ADDRESS BITS */
 #define PCF2123_WRITE		BIT(4)
@@ -206,6 +207,59 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static int pcf2123_read_offset(struct device *dev, long *offset)
+{
+	int ret;
+	s8 reg;
+
+	ret = pcf2123_read(dev, PCF2123_REG_OFFSET, &reg, 1);
+	if (ret < 0)
+		return ret;
+
+	if (reg & OFFSET_COARSE)
+		reg <<= 1; /* multiply by 2 and sign extend */
+	else
+		reg |= (reg & OFFSET_SIGN_BIT) << 1; /* sign extend only */
+
+	*offset = ((long)reg) * OFFSET_STEP;
+
+	return 0;
+}
+
+/*
+ * The offset register is a 7 bit signed value with a coarse bit in bit 7.
+ * The main difference between the two is normal offset adjusts the first
+ * second of n minutes every other hour, with 61, 62 and 63 being shoved
+ * into the 60th minute.
+ * The coarse adjustment does the same, but every hour.
+ * the two overlap, with every even normal offset value corresponding
+ * to a coarse offset. Based on this algorithm, it seems that despite the
+ * name, coarse offset is a better fit for overlapping values.
+ */
+static int pcf2123_set_offset(struct device *dev, long offset)
+{
+	s8 reg;
+
+	if (offset > OFFSET_STEP * 127)
+		reg = 127;
+	else if (offset < OFFSET_STEP * -128)
+		reg = -128;
+	else
+		reg = (s8)((offset + (OFFSET_STEP >> 1)) / OFFSET_STEP);
+
+	/* choose fine offset only for odd values in the normal range */
+	if (reg & 1 && reg <= 63 && reg >= -64) {
+		/* Normal offset. Clear the coarse bit */
+		reg &= ~OFFSET_COARSE;
+	} else {
+		/* Coarse offset. Divide by 2 and set the coarse bit */
+		reg >>= 1;
+		reg |= OFFSET_COARSE;
+	}
+
+	return pcf2123_write_reg(dev, PCF2123_REG_OFFSET, reg);
+}
+
 static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	u8 rxbuf[7];
@@ -314,6 +368,9 @@ static int pcf2123_reset(struct device *dev)
 static const struct rtc_class_ops pcf2123_rtc_ops = {
 	.read_time	= pcf2123_rtc_read_time,
 	.set_time	= pcf2123_rtc_set_time,
+	.read_offset	= pcf2123_read_offset,
+	.set_offset	= pcf2123_set_offset,
+
 };
 
 static int pcf2123_probe(struct spi_device *spi)
-- 
2.5.0

Re: [PATCH v4 1/3] rtc: Add functions to set and read rtc offset

[Joshua Clayton]

On Fri,  5 Feb 2016 12:41:11 -0800
Joshua Clayton <stillcompiling@gmail.com> wrote:

Alexandre,
in case you didn't see, I made the change you requested.
The rtc_read_offset function checks for a read_offset function
and returns -EINVAL if not there, instead of an offset of 0.
I just didn't put together a cover letter this time around.
 
> A number of rtc devices, such as the NXP pcf2123 include a facility
> to adjust the clock in order to compensate for temperature or a
> crystal, capacitor, etc, that results in the rtc clock not running
> at exactly 32.768 kHz.
> 
> Data sheets I have seen refer to this as a clock offset, and measure
> it in parts per million, however they often reference ppm to 2 digits
> of precision, which makes integer ppm less than ideal.
> 
> We use parts per billion, which more than covers the precision needed
> and works nicely within 32 bits
> 
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> ---
>  drivers/rtc/interface.c | 54
> +++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/rtc.h     |  4 ++++ 2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 5836751..9ef5f6f 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -939,4 +939,58 @@ void
> rtc_t4e40602412d61b7ef4162621a1a15be32126baeeimer_cancel(struct
> rtc_device *rtc, struct rtc_timer *timer)
> mutex_unlock(&rtc->ops_lock); } 
> +/**
> + * rtc_read_offset - Read the amount of rtc offset in parts per
> billion
> + * @ rtc: rtc device to be used
> + * @ offset: the offset in parts per billion
> + *
> + * see below for details.
> + *
> + * Kernel interface to read rtc clock offset
> + * Returns 0 on success, or a negative number on error.
> + * If read_offset() is not implemented for the rtc, return -EINVAL
> + */
> +int rtc_read_offset(struct rtc_device *rtc, long *offset)
> +{
> +	int ret;
> +
> +	if (!rtc->ops)
> +		return -ENODEV;
> +
> +	if (!rtc->ops->read_offset)
> +		return -EINVAL;
> +
> +	mutex_lock(&rtc->ops_lock);
> +	ret = rtc->ops->read_offset(rtc->dev.parent, offset);
> +	mutex_unlock(&rtc->ops_lock);
> +	return ret;
> +}
>  
> +/**
> + * rtc_set_offset - Adjusts the duration of the average second
> + * @ rtc: rtc device to be used
> + * @ offset: the offset in parts per billion
> + *
> + * Some rtc's allow an adjustment to the average duration of a second
> + * to compensate for differences in the actual clock rate due to
> temperature,
> + * the crystal, capacitor, etc.
> + *
> + * Kernel interface to adjust an rtc clock offset.
> + * Return 0 on success, or a negative number on error.
> + * If the rtc offset is not setable (or not implemented), return
> -EINVAL
> + */
> +int rtc_set_offset(struct rtc_device *rtc, long offset)
> +{
> +	int ret;
> +
> +	if (!rtc->ops)
> +		return -ENODEV;
> +
> +	if (!rtc->ops->set_offset)
> +		return -EINVAL;
> +
> +	mutex_lock(&rtc->ops_lock);
> +	ret = rtc->ops->set_offset(rtc->dev.parent, offset);
> +	mutex_unlock(&rtc->ops_lock);
> +	return ret;
> +}
> diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> index 3359f04..b693ada 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -89,6 +89,8 @@ struct rtc_class_ops {
>  	int (*set_mmss)(struct device *, unsigned long secs);
>  	int (*read_callback)(struct device *, int data);
>  	int (*alarm_irq_enable)(struct device *, unsigned int
> enabled);
> +	int (*read_offset)(struct device *, long *offset);
> +	int (*set_offset)(struct device *, long offset);
>  };
>  
>  #define RTC_DEVICE_NAME_SIZE 20
> @@ -208,6 +210,8 @@ void rtc_timer_init(struct rtc_timer *timer, void
> (*f)(void *p), void *data); int rtc_timer_start(struct rtc_device
> *rtc, struct rtc_timer *timer, ktime_t expires, ktime_t period);
>  void rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer
> *timer); +int rtc_read_offset(struct rtc_device *rtc, long *offset);
> +int rtc_set_offset(struct rtc_device *rtc, long offset);
>  void rtc_timer_do_work(struct work_struct *work);
>  
>  static inline bool is_leap_year(unsigned int year)

Thanks,

Joshua

Re: [PATCH v4 1/3] rtc: Add functions to set and read rtc offset

[Alexandre Belloni]

Hi,

On 23/02/2016 at 10:47:13 -0800, Joshua Clayton wrote :
> On Fri,  5 Feb 2016 12:41:11 -0800
> Joshua Clayton <stillcompiling@gmail.com> wrote:
> 
> Alexandre,
> in case you didn't see, I made the change you requested.
> The rtc_read_offset function checks for a read_offset function
> and returns -EINVAL if not there, instead of an offset of 0.
> I just didn't put together a cover letter this time around.
>  

Sure, I was planning to review them tonight anyway.

All applied, thanks!

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 2/3] rtc: implement a sysfs interface for clock offset

[Joshua Clayton]

clock offset may be set and read in decimal parts per billion
attribute is /sys/class/rtc/rtcN/offset
The attribute is only visible for rtcs that have set_offset implemented.

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 Documentation/rtc.txt   |  6 ++++++
 drivers/rtc/rtc-sysfs.c | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/Documentation/rtc.txt b/Documentation/rtc.txt
index 8446f1e..ddc3660 100644
--- a/Documentation/rtc.txt
+++ b/Documentation/rtc.txt
@@ -157,6 +157,12 @@ wakealarm:	 The time at which the clock will generate a system wakeup
 		 the epoch by default, or if there's a leading +, seconds in the
 		 future, or if there is a leading +=, seconds ahead of the current
 		 alarm.
+offset:		 The amount which the rtc clock has been adjusted in firmware.
+		 Visible only if the driver supports clock offset adjustment.
+		 The unit is parts per billion, i.e. The number of clock ticks
+		 which are added to or removed from the rtc's base clock per
+		 billion ticks. A positive value makes a day pass more slowly,
+		 longer, and a negative value makes a day pass more quickly.
 
 IOCTL INTERFACE
 ---------------
diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
index 463e286..f8d7f8d 100644
--- a/drivers/rtc/rtc-sysfs.c
+++ b/drivers/rtc/rtc-sysfs.c
@@ -218,6 +218,34 @@ wakealarm_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(wakealarm);
 
+static ssize_t
+offset_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	ssize_t retval;
+	long offset;
+
+	retval = rtc_read_offset(to_rtc_device(dev), &offset);
+	if (retval == 0)
+		retval = sprintf(buf, "%ld\n", offset);
+
+	return retval;
+}
+
+static ssize_t
+offset_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t n)
+{
+	ssize_t retval;
+	long offset;
+
+	retval = kstrtol(buf, 10, &offset);
+	if (retval == 0)
+		retval = rtc_set_offset(to_rtc_device(dev), offset);
+
+	return (retval < 0) ? retval : n;
+}
+static DEVICE_ATTR_RW(offset);
+
 static struct attribute *rtc_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_date.attr,
@@ -226,6 +254,7 @@ static struct attribute *rtc_attrs[] = {
 	&dev_attr_max_user_freq.attr,
 	&dev_attr_hctosys.attr,
 	&dev_attr_wakealarm.attr,
+	&dev_attr_offset.attr,
 	NULL,
 };
 
@@ -249,9 +278,13 @@ static umode_t rtc_attr_is_visible(struct kobject *kobj,
 	struct rtc_device *rtc = to_rtc_device(dev);
 	umode_t mode = attr->mode;
 
-	if (attr == &dev_attr_wakealarm.attr)
+	if (attr == &dev_attr_wakealarm.attr) {
 		if (!rtc_does_wakealarm(rtc))
 			mode = 0;
+	} else if (attr == &dev_attr_offset.attr) {
+		if (!rtc->ops->set_offset)
+			mode = 0;
+	}
 
 	return mode;
 }
-- 
2.5.0

Re: [PATCH v3 2/3] rtc: implement a sysfs interface for clock offset

[Alexandre Belloni]

A really small nitpick below

On 03/02/2016 at 09:16:43 -0800, Joshua Clayton wrote :
> clock offset may be set and read in decimal parts per billion
> attribute is /sys/class/rtc/rtcN/offset
> The attribute is only visible for rtcs that have set_offset implemented.
> 
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> ---
>  Documentation/rtc.txt   |  6 ++++++
>  drivers/rtc/rtc-sysfs.c | 35 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/rtc.txt b/Documentation/rtc.txt
> index 8446f1e..ddc3660 100644
> --- a/Documentation/rtc.txt
> +++ b/Documentation/rtc.txt
> @@ -157,6 +157,12 @@ wakealarm:	 The time at which the clock will generate a system wakeup
>  		 the epoch by default, or if there's a leading +, seconds in the
>  		 future, or if there is a leading +=, seconds ahead of the current
>  		 alarm.
> +offset:		 The amount which the rtc clock has been adjusted in firmware.
> +		 Visible only if the driver supports clock offset adjustment.
> +		 The unit is parts per billion, i.e. The number of clock ticks
> +		 which are added to or removed from the rtc's base clock per
> +		 billion ticks. A positive value makes a day pass more slowly,
> +		 longer, and a negative value makes a day pass more quickly.
>  
>  IOCTL INTERFACE
>  ---------------
> diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
> index 463e286..f8d7f8d 100644
> --- a/drivers/rtc/rtc-sysfs.c
> +++ b/drivers/rtc/rtc-sysfs.c
> @@ -218,6 +218,34 @@ wakealarm_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RW(wakealarm);
>  
> +static ssize_t
> +offset_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	ssize_t retval;
> +	long offset;
> +
> +	retval = rtc_read_offset(to_rtc_device(dev), &offset);
> +	if (retval == 0)
> +		retval = sprintf(buf, "%ld\n", offset);
> +
> +	return retval;
> +}
> +
> +static ssize_t
> +offset_store(struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t n)

This is not properly aligned.

> +{
> +	ssize_t retval;
> +	long offset;
> +
> +	retval = kstrtol(buf, 10, &offset);
> +	if (retval == 0)
> +		retval = rtc_set_offset(to_rtc_device(dev), offset);
> +
> +	return (retval < 0) ? retval : n;
> +}
> +static DEVICE_ATTR_RW(offset);
> +
>  static struct attribute *rtc_attrs[] = {
>  	&dev_attr_name.attr,
>  	&dev_attr_date.attr,
> @@ -226,6 +254,7 @@ static struct attribute *rtc_attrs[] = {
>  	&dev_attr_max_user_freq.attr,
>  	&dev_attr_hctosys.attr,
>  	&dev_attr_wakealarm.attr,
> +	&dev_attr_offset.attr,
>  	NULL,
>  };
>  
> @@ -249,9 +278,13 @@ static umode_t rtc_attr_is_visible(struct kobject *kobj,
>  	struct rtc_device *rtc = to_rtc_device(dev);
>  	umode_t mode = attr->mode;
>  
> -	if (attr == &dev_attr_wakealarm.attr)
> +	if (attr == &dev_attr_wakealarm.attr) {
>  		if (!rtc_does_wakealarm(rtc))
>  			mode = 0;
> +	} else if (attr == &dev_attr_offset.attr) {
> +		if (!rtc->ops->set_offset)
> +			mode = 0;
> +	}
>  
>  	return mode;
>  }
> -- 
> 2.5.0
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v3 3/3] rtc-pcf2123: implement read_offset and set_offset

[Joshua Clayton]

pcf2123 has an offset register, which can be used to make minor
adjustments to the clock rate to compensate for temperature or
a crystal that is not exactly right.

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 drivers/rtc/rtc-pcf2123.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index d9a44ad..da27738 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -100,6 +100,7 @@
 /* PCF2123_REG_OFFSET BITS */
 #define OFFSET_SIGN_BIT		BIT(6)	/* 2's complement sign bit */
 #define OFFSET_COARSE		BIT(7)	/* Coarse mode offset */
+#define OFFSET_STEP		(2170)	/* Offset step in parts per billion */
 
 /* READ/WRITE ADDRESS BITS */
 #define PCF2123_WRITE		BIT(4)
@@ -206,6 +207,59 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static int pcf2123_read_offset(struct device *dev, long *offset)
+{
+	int ret;
+	s8 reg;
+
+	ret = pcf2123_read(dev, PCF2123_REG_OFFSET, &reg, 1);
+	if (ret < 0)
+		return ret;
+
+	if (reg & OFFSET_COARSE)
+		reg <<= 1; /* multiply by 2 and sign extend */
+	else
+		reg |= (reg & OFFSET_SIGN_BIT) << 1; /* sign extend only */
+
+	*offset = ((long)reg) * OFFSET_STEP;
+
+	return 0;
+}
+
+/*
+ * The offset register is a 7 bit signed value with a coarse bit in bit 7.
+ * The main difference between the two is normal offset adjusts the first
+ * second of n minutes every other hour, with 61, 62 and 63 being shoved
+ * into the 60th minute.
+ * The coarse adjustment does the same, but every hour.
+ * the two overlap, with every even normal offset value corresponding
+ * to a coarse offset. Based on this algorithm, it seems that despite the
+ * name, coarse offset is a better fit for overlapping values.
+ */
+static int pcf2123_set_offset(struct device *dev, long offset)
+{
+	s8 reg;
+
+	if (offset > OFFSET_STEP * 127)
+		reg = 127;
+	else if (offset < OFFSET_STEP * -128)
+		reg = -128;
+	else
+		reg = (s8)((offset + (OFFSET_STEP >> 1)) / OFFSET_STEP);
+
+	/* choose fine offset only for odd values in the normal range */
+	if (reg & 1 && reg <= 63 && reg >= -64) {
+		/* Normal offset. Clear the coarse bit */
+		reg &= ~OFFSET_COARSE;
+	} else {
+		/* Coarse offset. Divide by 2 and set the coarse bit */
+		reg >>= 1;
+		reg |= OFFSET_COARSE;
+	}
+
+	return pcf2123_write_reg(dev, PCF2123_REG_OFFSET, reg);
+}
+
 static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	u8 rxbuf[7];
@@ -314,6 +368,9 @@ static int pcf2123_reset(struct device *dev)
 static const struct rtc_class_ops pcf2123_rtc_ops = {
 	.read_time	= pcf2123_rtc_read_time,
 	.set_time	= pcf2123_rtc_set_time,
+	.read_offset	= pcf2123_read_offset,
+	.set_offset	= pcf2123_set_offset,
+
 };
 
 static int pcf2123_probe(struct spi_device *spi)
-- 
2.5.0

[PATCH v2 8/8] rtc-pcf2123: implement read_offset and set_offset

[Joshua Clayton]

pcf2123 has an offset register, which can be used to make minor
adjustments to the clock rate to compensate for temperature or
a crystal that is not exactly right.

Expose the offset register to sysfs

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
---
 drivers/rtc/rtc-pcf2123.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index d9a44ad..da27738 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -100,6 +100,7 @@
 /* PCF2123_REG_OFFSET BITS */
 #define OFFSET_SIGN_BIT		BIT(6)	/* 2's complement sign bit */
 #define OFFSET_COARSE		BIT(7)	/* Coarse mode offset */
+#define OFFSET_STEP		(2170)	/* Offset step in parts per billion */
 
 /* READ/WRITE ADDRESS BITS */
 #define PCF2123_WRITE		BIT(4)
@@ -206,6 +207,59 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static int pcf2123_read_offset(struct device *dev, long *offset)
+{
+	int ret;
+	s8 reg;
+
+	ret = pcf2123_read(dev, PCF2123_REG_OFFSET, &reg, 1);
+	if (ret < 0)
+		return ret;
+
+	if (reg & OFFSET_COARSE)
+		reg <<= 1; /* multiply by 2 and sign extend */
+	else
+		reg |= (reg & OFFSET_SIGN_BIT) << 1; /* sign extend only */
+
+	*offset = ((long)reg) * OFFSET_STEP;
+
+	return 0;
+}
+
+/*
+ * The offset register is a 7 bit signed value with a coarse bit in bit 7.
+ * The main difference between the two is normal offset adjusts the first
+ * second of n minutes every other hour, with 61, 62 and 63 being shoved
+ * into the 60th minute.
+ * The coarse adjustment does the same, but every hour.
+ * the two overlap, with every even normal offset value corresponding
+ * to a coarse offset. Based on this algorithm, it seems that despite the
+ * name, coarse offset is a better fit for overlapping values.
+ */
+static int pcf2123_set_offset(struct device *dev, long offset)
+{
+	s8 reg;
+
+	if (offset > OFFSET_STEP * 127)
+		reg = 127;
+	else if (offset < OFFSET_STEP * -128)
+		reg = -128;
+	else
+		reg = (s8)((offset + (OFFSET_STEP >> 1)) / OFFSET_STEP);
+
+	/* choose fine offset only for odd values in the normal range */
+	if (reg & 1 && reg <= 63 && reg >= -64) {
+		/* Normal offset. Clear the coarse bit */
+		reg &= ~OFFSET_COARSE;
+	} else {
+		/* Coarse offset. Divide by 2 and set the coarse bit */
+		reg >>= 1;
+		reg |= OFFSET_COARSE;
+	}
+
+	return pcf2123_write_reg(dev, PCF2123_REG_OFFSET, reg);
+}
+
 static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	u8 rxbuf[7];
@@ -314,6 +368,9 @@ static int pcf2123_reset(struct device *dev)
 static const struct rtc_class_ops pcf2123_rtc_ops = {
 	.read_time	= pcf2123_rtc_read_time,
 	.set_time	= pcf2123_rtc_set_time,
+	.read_offset	= pcf2123_read_offset,
+	.set_offset	= pcf2123_set_offset,
+
 };
 
 static int pcf2123_probe(struct spi_device *spi)
-- 
2.5.0

Re: [PATCH v2 0/8] rtc: enable adjustment of clock offset

[Joshua Clayton]

Ping?

On Monday, January 04, 2016 10:31:18 AM Joshua Clayton wrote:
> Alexandre,
> this is the second version of apatch set to be able to adjust the
> clock on a pcf2123, but with a big addition. I am also proposing
> a couple of new rtc api functions and a new sysfs file.
> 
> Patches 1-5 are very much v2 patches that address All of the
> concerns you had with v1. It is my hope to get them merged without
> too much more trouble.
> 
The rtc _needs_ patches 1-5 in order to use the clock offset
adjustment at all (current upstream code resets it to zero during
device probe, so it doesn't survive a reboot)
Should I split out patches 1-5 to make them easier to review?

> Patches 6-8 are attemping to add a new function to the rtc class
> to adjust the clock rate. I hope I am going about this the right way,
> But here goes.
> 
I am hoping to have some discussion about 6, 7 and 8.
Particularly whether parts per billion is the right unit (I think it is)
and whether "offset" is the best name for the attribute (not as sure).

> A number of rtc devices, such as the NXP pcf2123 include a facility
> to adjust the clock in order to compensate for temperature or a
> crystal, capacitor, etc, that results in the rtc clock not running
> at exactly 32.768 kHz.
> 
> This patchset adds kernel and sysfs hooks to access that ability.
> 
> One datasheet suggests it might be adjusted based on input from 
> a temperature sensor. I could also potentially see it being set
> as part of ntp calibration.
> 
> Data sheets I have seen refer to this as a clock offset, and measure it
> in parts per million (ppm), however they often reference ppm to 2 digits
> of precision, which makes integer ppm less than ideal. I use parts per
> billion, which more than covers the precision needed and works nicely
> within 32 bits
> 
> The name "offset" came from the pcf-2123 datasheet and is used by at
> least some other data sheets. I would be happy to use a different term
> if someone else comes up with something more concise.
>     
> Changes since v1:
> - Use the BIT() macro for all register bits
> - Remove unneeded range checks from read/write functions
> - Merge patch 3 (replace magic numbers with defines) into patch 2
> - Add a proper commit message to patch 5, now patch 4
> - Fix a function alignment bug.
> - Move OSC_HAS_STOPPED check into pcf2123_rtc_read_time, and get rid of
>   pcf2123_time_valid()
> - Drop patches refactoring pcf2123 sysfs.
> - Add rtc interface and rtc sysfs file for clock offset
> 
> Joshua Clayton (8):
>   rtc-pcf2123: define registers and bit macros
>   rtc-pcf2123: clean up reads from the chip
>   rtc-pcf2123: clean up writes to the rtc chip
>   rtc-pcf2123: refactor chip reset into a function
>   rtc-pcf2123: avoid resetting the clock if possible
>   rtc: Add functions to set and read clock offset
>   rtc: implement a sysfs interface for clock offset
>   rtc-pcf2123: implement read_offset and set_offset
> 
>  drivers/rtc/interface.c   |  57 ++++++++++
>  drivers/rtc/rtc-pcf2123.c | 271 +++++++++++++++++++++++++++++++++-------------
>  drivers/rtc/rtc-sysfs.c   |  29 +++++
>  include/linux/rtc.h       |   4 +
>  4 files changed, 284 insertions(+), 77 deletions(-)
> 
> 
Thanks again,
Joshua Clayton

Re: [PATCH v2 0/8] rtc: enable adjustment of clock offset

[Alexandre Belloni]

On 26/01/2016 at 06:56:40 -0800, Joshua Clayton wrote :
> Ping?
> 
> On Monday, January 04, 2016 10:31:18 AM Joshua Clayton wrote:
> > Alexandre,
> > this is the second version of apatch set to be able to adjust the
> > clock on a pcf2123, but with a big addition. I am also proposing
> > a couple of new rtc api functions and a new sysfs file.
> > 
> > Patches 1-5 are very much v2 patches that address All of the
> > concerns you had with v1. It is my hope to get them merged without
> > too much more trouble.
> > 
> The rtc _needs_ patches 1-5 in order to use the clock offset
> adjustment at all (current upstream code resets it to zero during
> device probe, so it doesn't survive a reboot)
> Should I split out patches 1-5 to make them easier to review?
> 

The merge widows just closed so I've started to collect a few patches
since monday. This is a big series and I didn't have time to review
everything yet.

> > Patches 6-8 are attemping to add a new function to the rtc class
> > to adjust the clock rate. I hope I am going about this the right way,
> > But here goes.
> > 
> I am hoping to have some discussion about 6, 7 and 8.
> Particularly whether parts per billion is the right unit (I think it is)
> and whether "offset" is the best name for the attribute (not as sure).
> 

Yeah, My plan was to take 1 to 5 and leave out 6-8 for discussion, I'm
not yet sure what the correct api should look like.

> > A number of rtc devices, such as the NXP pcf2123 include a facility
> > to adjust the clock in order to compensate for temperature or a
> > crystal, capacitor, etc, that results in the rtc clock not running
> > at exactly 32.768 kHz.
> > 
> > This patchset adds kernel and sysfs hooks to access that ability.
> > 
> > One datasheet suggests it might be adjusted based on input from 
> > a temperature sensor. I could also potentially see it being set
> > as part of ntp calibration.
> > 
> > Data sheets I have seen refer to this as a clock offset, and measure it
> > in parts per million (ppm), however they often reference ppm to 2 digits
> > of precision, which makes integer ppm less than ideal. I use parts per
> > billion, which more than covers the precision needed and works nicely
> > within 32 bits
> > 
> > The name "offset" came from the pcf-2123 datasheet and is used by at
> > least some other data sheets. I would be happy to use a different term
> > if someone else comes up with something more concise.
> >     
> > Changes since v1:
> > - Use the BIT() macro for all register bits
> > - Remove unneeded range checks from read/write functions
> > - Merge patch 3 (replace magic numbers with defines) into patch 2
> > - Add a proper commit message to patch 5, now patch 4
> > - Fix a function alignment bug.
> > - Move OSC_HAS_STOPPED check into pcf2123_rtc_read_time, and get rid of
> >   pcf2123_time_valid()
> > - Drop patches refactoring pcf2123 sysfs.
> > - Add rtc interface and rtc sysfs file for clock offset
> > 
> > Joshua Clayton (8):
> >   rtc-pcf2123: define registers and bit macros
> >   rtc-pcf2123: clean up reads from the chip
> >   rtc-pcf2123: clean up writes to the rtc chip
> >   rtc-pcf2123: refactor chip reset into a function
> >   rtc-pcf2123: avoid resetting the clock if possible
> >   rtc: Add functions to set and read clock offset
> >   rtc: implement a sysfs interface for clock offset
> >   rtc-pcf2123: implement read_offset and set_offset
> > 
> >  drivers/rtc/interface.c   |  57 ++++++++++
> >  drivers/rtc/rtc-pcf2123.c | 271 +++++++++++++++++++++++++++++++++-------------
> >  drivers/rtc/rtc-sysfs.c   |  29 +++++
> >  include/linux/rtc.h       |   4 +
> >  4 files changed, 284 insertions(+), 77 deletions(-)
> > 
> > 
> Thanks again,
> Joshua Clayton

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v2 0/8] rtc: enable adjustment of clock offset

[Alexandre Belloni]

Hi,

On 26/01/2016 at 17:00:31 +0100, Alexandre Belloni wrote :
> On 26/01/2016 at 06:56:40 -0800, Joshua Clayton wrote :
> > Ping?
> > 
> > On Monday, January 04, 2016 10:31:18 AM Joshua Clayton wrote:
> > > Alexandre,
> > > this is the second version of apatch set to be able to adjust the
> > > clock on a pcf2123, but with a big addition. I am also proposing
> > > a couple of new rtc api functions and a new sysfs file.
> > > 
> > > Patches 1-5 are very much v2 patches that address All of the
> > > concerns you had with v1. It is my hope to get them merged without
> > > too much more trouble.
> > > 
> > The rtc _needs_ patches 1-5 in order to use the clock offset
> > adjustment at all (current upstream code resets it to zero during
> > device probe, so it doesn't survive a reboot)
> > Should I split out patches 1-5 to make them easier to review?
> > 
> 
> The merge widows just closed so I've started to collect a few patches
> since monday. This is a big series and I didn't have time to review
> everything yet.
> 


Patches 1-5 are applied and pushed to rtc-next.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com