[PATCH 0/9] rtc-2123: access the clock offset feature

[PATCH 0/9] rtc-2123: access the clock offset feature

[Joshua Clayton]

Greetings,
This series was prompted by a need to adjust the clock rate of the rtc
The existing code performs a soft reset during probe, which wipes out
several registers including the offset register, which performs adjustments
to the clock rate.

The first several patches are cleanup, with patch 5 and 6 avoiding the reset,
and patch 9 adding a nice sysfs interface to the clock offset.

I know that this is not the only rtc to provide a programmable clock offset
I wonder if this interface would make a good addition to the rtc api?

The rtc chips I have seen list their clock adjustments in parts per million.
I went with parts per billion, since the ppm listed was listed with a
fractional component.

Joshua Clayton (9):
  rtc-pcf2123: Document all registers and useful bits
  rtc-pcf2123: clean up reads from the chip
  rtc-pcf2123: clean up writes to the rtc chip
  rtc-pcf2123: replace magic numbers with defines
  rtc-pcf2123: put the chip reset into a function
  rtc-pcf2123: avoid resetting the clock if possible
  rtc-pcf2123: allow sysfs to accept hexidecimal
  rtc-pcf2123: use sysfs groups
  rtc-pcf2123: adjust the clock rate via sysfs

 drivers/rtc/rtc-pcf2123.c | 391 ++++++++++++++++++++++++++++++----------------
 1 file changed, 257 insertions(+), 134 deletions(-)

-- 
2.5.0

[PATCH 1/9] rtc-pcf2123: Document all registers and useful bits

[Joshua Clayton]

Document all 16 registers in the pcf2123, as well as
useful bit masks from the Control1 and seconds registers

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

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index d1953bb..7756210 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -47,6 +47,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 */
@@ -56,7 +57,27 @@
 #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)
+#define PCF2123_REG_MAX		(PCF2123_REG_CTDWN_TMR)
+
+/* PCF2123_REG_CTRL1 BITS */
+#define CTRL1_CLEAR		(0x00)	/* Clear */
+#define CTRL1_CORRECTION_INT	(0x02)	/* Correction Interrupt */
+#define CTRL1_12_HOUR		(0x04)	/* 12 hour time */
+#define CTRL1_STOP		(0x20)	/* Stop the clock */
+#define CTRL1_SW_RESET		(0x58)	/* Software reset */
+#define CTRL1_EXT_TEST		(0x80)	/* External Clock Test mode */
+
+/* PCF2123_REG_SC BITS */
+#define OSC_HAS_STOPPED		(0x80)	/* Clock has been stopped */
+
+/* READ/WRITE ADDRESS BITS */
 #define PCF2123_SUBADDR		(1 << 4)
 #define PCF2123_WRITE		((0 << 7) | PCF2123_SUBADDR)
 #define PCF2123_READ		((1 << 7) | PCF2123_SUBADDR)
-- 
2.5.0

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

[Joshua Clayton]

put read operations into a function.
This makes the starting register more prominent and hides the delay.

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

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index 7756210..648cb74 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -104,12 +104,26 @@ 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;
+
+	if (reg > PCF2123_REG_MAX)
+		return -EFAULT;
+
+	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;
 
@@ -119,11 +133,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]);
 }
 
@@ -158,16 +171,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);
@@ -279,16 +288,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 3/9] rtc-pcf2123: clean up writes to the rtc chip

[Joshua Clayton]

A new function pcf2123_write() hides the delay and the spi device,
while pcf2123_write_reg() further simplifies the most common case:
writing a single register.

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

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index 648cb74..3acb2c8 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -119,6 +119,30 @@ 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;
+
+	if (txbuf[0] > PCF2123_REG_MAX)
+		return -EFAULT;
+
+	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)
 {
@@ -142,9 +166,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;
 
@@ -160,12 +182,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;
 }
 
@@ -205,7 +224,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;
 
@@ -216,15 +234,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, 0x20);
 	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);
@@ -233,18 +248,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, 0x00);
 	if (ret < 0)
 		return ret;
-	pcf2123_delay_trec();
 
 	return 0;
 }
@@ -258,7 +269,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),
@@ -268,24 +279,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, 0x58);
 	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, 0x20);
 	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");
@@ -306,12 +309,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, 0x00);
 	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 4/9] rtc-pcf2123: replace magic numbers with defines

[Joshua Clayton]

All of the magic numbers pertain to register CTRL1.
document all the values in CTRL1 with defines from the datasheet

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

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index 3acb2c8..257ce7d 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -234,7 +234,7 @@ 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 */
-	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, 0x20);
+	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_STOP);
 	if (ret < 0)
 		return ret;
 
@@ -253,7 +253,7 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		return ret;
 
 	/* Start the counter */
-	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, 0x00);
+	ret = pcf2123_write_reg(dev, PCF2123_REG_CTRL1, CTRL1_CLEAR);
 	if (ret < 0)
 		return ret;
 
@@ -280,13 +280,13 @@ static int pcf2123_probe(struct spi_device *spi)
 
 	/* Send a software reset command */
 	dev_dbg(&spi->dev, "resetting RTC\n");
-	ret = pcf2123_write_reg(&spi->dev, PCF2123_REG_CTRL1, 0x58);
+	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, 0x20);
+	ret = pcf2123_write_reg(&spi->dev, PCF2123_REG_CTRL1, CTRL1_STOP);
 	if (ret < 0)
 		goto kfree_exit;
 
@@ -298,7 +298,7 @@ static int pcf2123_probe(struct spi_device *spi)
 	if (ret < 0)
 		goto kfree_exit;
 
-	if (!(rxbuf[0] & 0x20)) {
+	if (!(rxbuf[0] & CTRL1_STOP)) {
 		dev_err(&spi->dev, "chip not found\n");
 		ret = -ENODEV;
 		goto kfree_exit;
@@ -309,7 +309,7 @@ static int pcf2123_probe(struct spi_device *spi)
 			(spi->max_speed_hz + 500) / 1000);
 
 	/* Start the counter */
-	ret = pcf2123_write_reg(&spi->dev, PCF2123_REG_CTRL1, 0x00);
+	ret = pcf2123_write_reg(&spi->dev, PCF2123_REG_CTRL1, CTRL1_CLEAR);
 	if (ret < 0)
 		goto kfree_exit;
 
-- 
2.5.0

[PATCH 5/9] rtc-pcf2123: put the chip reset into a function

[Joshua Clayton]

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 257ce7d..d3c1447 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -260,6 +260,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,
@@ -269,7 +303,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),
@@ -278,29 +311,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] & CTRL1_STOP)) {
+	ret = pcf2123_reset(&spi->dev);
+	if (ret < 0) {
 		dev_err(&spi->dev, "chip not found\n");
-		ret = -ENODEV;
 		goto kfree_exit;
 	}
 
@@ -308,11 +321,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 6/9] 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 | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index d3c1447..4964d5c 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -260,6 +260,33 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return 0;
 }
 
+static bool pcf2123_time_valid(struct device *dev)
+{
+	u8 seconds;
+	struct rtc_time tm;
+	int ret;
+
+	ret = pcf2123_read(dev, PCF2123_REG_SC, &seconds, 1);
+	if (ret < 0)
+		return false;
+
+	if (seconds & OSC_HAS_STOPPED) {
+		dev_info(dev, "clock was stopped. seconds: 0x%02X\n", seconds);
+		return false;
+	}
+
+	ret = pcf2123_rtc_read_time(dev, &tm);
+	if (ret < 0)
+		return false;
+
+	if (rtc_valid_tm(&tm) < 0) {
+		dev_err(dev, "retrieved date/time is not valid.\n");
+		return false;
+	}
+
+	return true;
+}
+
 static int pcf2123_reset(struct device *dev)
 {
 	int ret;
@@ -311,10 +338,12 @@ static int pcf2123_probe(struct spi_device *spi)
 		return -ENOMEM;
 	spi->dev.platform_data = pdata;
 
-	ret = pcf2123_reset(&spi->dev);
-	if (ret < 0) {
-		dev_err(&spi->dev, "chip not found\n");
-		goto kfree_exit;
+	if (!pcf2123_time_valid(&spi->dev)) {
+		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 7/9] rtc-pcf2123: allow sysfs to accept hexidecimal

[Joshua Clayton]

pcf2123 registers store their values in bcd.
sysfs sensibly displays them in hexidecimal.
Up to now, the sysfs store functions only accept base 10, which
makes no sense.

Add support for hexidecimal without removing base10 support.

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

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index 4964d5c..6701e6d 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -178,7 +178,7 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,
 	if (ret)
 		return ret;
 
-	ret = kstrtoul(buffer, 10, &val);
+	ret = kstrtoul(buffer, 0, &val);
 	if (ret)
 		return ret;
 
-- 
2.5.0

[PATCH 8/9] rtc-pcf2123: use sysfs groups

[Joshua Clayton]

Switch from manually creating all the sysfs attributes to
defining them mostly in the canonical way.
We are adding them to an spi driver, so we must still add and remove
the group manually, but everythig else is done by The Book(tm) .

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

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index 6701e6d..d494638 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -84,16 +84,6 @@
 
 static struct spi_driver pcf2123_driver;
 
-struct pcf2123_sysfs_reg {
-	struct device_attribute attr;
-	char name[2];
-};
-
-struct pcf2123_plat_data {
-	struct rtc_device *rtc;
-	struct pcf2123_sysfs_reg regs[16];
-};
-
 /*
  * Causes a 30 nanosecond delay to ensure that the PCF2123 chip select
  * is released properly after an SPI write.  This function should be
@@ -146,17 +136,11 @@ static int pcf2123_write_reg(struct device *dev, u8 reg, u8 val)
 static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,
 			    char *buffer)
 {
-	struct pcf2123_sysfs_reg *r;
 	u8 rxbuf[1];
 	unsigned long reg;
 	int ret;
 
-	r = container_of(attr, struct pcf2123_sysfs_reg, attr);
-
-	ret = kstrtoul(r->name, 16, &reg);
-	if (ret)
-		return ret;
-
+	reg = hex_to_bin(attr->attr.name[0]);
 	ret = pcf2123_read(dev, reg, rxbuf, 1);
 	if (ret < 0)
 		return -EIO;
@@ -166,25 +150,19 @@ 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 pcf2123_sysfs_reg *r;
 	unsigned long reg;
 	unsigned long val;
-
 	int ret;
 
-	r = container_of(attr, struct pcf2123_sysfs_reg, attr);
-
-	ret = kstrtoul(r->name, 16, &reg);
-	if (ret)
-		return ret;
-
 	ret = kstrtoul(buffer, 0, &val);
 	if (ret)
 		return ret;
 
+	reg = hex_to_bin(attr->attr.name[0]);
 	pcf2123_write_reg(dev, reg, val);
 	if (ret < 0)
 		return -EIO;
+
 	return count;
 }
 
@@ -326,17 +304,56 @@ static const struct rtc_class_ops pcf2123_rtc_ops = {
 	.set_time	= pcf2123_rtc_set_time,
 };
 
+static DEVICE_ATTR(0, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(1, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(2, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(3, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(4, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(5, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(6, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(7, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(8, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(9, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(a, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(b, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(c, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(d, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(e, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(f, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+
+static struct attribute *pcf2123_attrs[] = {
+	&dev_attr_0.attr,
+	&dev_attr_1.attr,
+	&dev_attr_2.attr,
+	&dev_attr_3.attr,
+	&dev_attr_4.attr,
+	&dev_attr_5.attr,
+	&dev_attr_6.attr,
+	&dev_attr_7.attr,
+	&dev_attr_8.attr,
+	&dev_attr_9.attr,
+	&dev_attr_a.attr,
+	&dev_attr_b.attr,
+	&dev_attr_c.attr,
+	&dev_attr_d.attr,
+	&dev_attr_e.attr,
+	&dev_attr_f.attr,
+	NULL
+};
+
+static const struct attribute_group pcf2123_group = {
+	.attrs = pcf2123_attrs,
+};
+
+static const struct attribute_group *pcf2123_groups[] = {
+	&pcf2123_group,
+	NULL
+};
+
 static int pcf2123_probe(struct spi_device *spi)
 {
 	struct rtc_device *rtc;
-	struct pcf2123_plat_data *pdata;
-	int ret, i;
-
-	pdata = devm_kzalloc(&spi->dev, sizeof(struct pcf2123_plat_data),
-				GFP_KERNEL);
-	if (!pdata)
-		return -ENOMEM;
-	spi->dev.platform_data = pdata;
+	int ret;
 
 	if (!pcf2123_time_valid(&spi->dev)) {
 		ret = pcf2123_reset(&spi->dev);
@@ -360,29 +377,13 @@ static int pcf2123_probe(struct spi_device *spi)
 		goto kfree_exit;
 	}
 
-	pdata->rtc = rtc;
-
-	for (i = 0; i < 16; i++) {
-		sysfs_attr_init(&pdata->regs[i].attr.attr);
-		sprintf(pdata->regs[i].name, "%1x", i);
-		pdata->regs[i].attr.attr.mode = S_IRUGO | S_IWUSR;
-		pdata->regs[i].attr.attr.name = pdata->regs[i].name;
-		pdata->regs[i].attr.show = pcf2123_show;
-		pdata->regs[i].attr.store = pcf2123_store;
-		ret = device_create_file(&spi->dev, &pdata->regs[i].attr);
-		if (ret) {
-			dev_err(&spi->dev, "Unable to create sysfs %s\n",
-				pdata->regs[i].name);
-			goto sysfs_exit;
-		}
-	}
+	ret = sysfs_create_groups(&spi->dev.kobj, pcf2123_groups);
+	if (ret)
+		goto sysfs_exit;
 
 	return 0;
-
 sysfs_exit:
-	for (i--; i >= 0; i--)
-		device_remove_file(&spi->dev, &pdata->regs[i].attr);
-
+	sysfs_remove_groups(&spi->dev.kobj, pcf2123_groups);
 kfree_exit:
 	spi->dev.platform_data = NULL;
 	return ret;
@@ -390,16 +391,7 @@ kfree_exit:
 
 static int pcf2123_remove(struct spi_device *spi)
 {
-	struct pcf2123_plat_data *pdata = dev_get_platdata(&spi->dev);
-	int i;
-
-	if (pdata) {
-		for (i = 0; i < 16; i++)
-			if (pdata->regs[i].name[0])
-				device_remove_file(&spi->dev,
-						   &pdata->regs[i].attr);
-	}
-
+	sysfs_remove_groups(&spi->dev.kobj, pcf2123_groups);
 	return 0;
 }
 
-- 
2.5.0

[PATCH 9/9] rtc-pcf2123: adjust the clock rate via sysfs

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

The adjustment is calculated in parts per billion. The data sheet
uses parts per million (as do some others), but with 2 digits of
precision. Since floating point is forbidden, parts per billion is
a better fit.

Add a pair of sysfs files to seetand retrieve the offset in ppm.

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

diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index d494638..6d70860 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -77,11 +77,17 @@
 /* PCF2123_REG_SC BITS */
 #define OSC_HAS_STOPPED		(0x80)	/* Clock has been stopped */
 
+/* PCF2123_REG_OFFSET BITS */
+#define OFFSET_COARSE		(0x80)	/* Coarse Mode Offset */
+
 /* READ/WRITE ADDRESS BITS */
 #define PCF2123_SUBADDR		(1 << 4)
 #define PCF2123_WRITE		((0 << 7) | PCF2123_SUBADDR)
 #define PCF2123_READ		((1 << 7) | PCF2123_SUBADDR)
 
+/* offset granularity in parts per billion in fine mode */
+#define OFFSET_STEP		(2170)
+
 static struct spi_driver pcf2123_driver;
 
 /*
@@ -166,6 +172,65 @@ static ssize_t pcf2123_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static ssize_t pcf2123_adjust_show(struct device *dev,
+				   struct device_attribute *attr, char *buffer)
+{
+	ssize_t ret;
+	s8 reg;
+
+	ret = pcf2123_read(dev, PCF2123_REG_OFFSET, &reg, 1);
+	if (ret < 0)
+		return -EIO;
+
+	if (reg & OFFSET_COARSE) {
+		reg <<= 1;
+	} else {
+		reg &= ~OFFSET_COARSE;
+		reg |= (reg & 0x40) << 1; /* sign extend */
+	}
+
+	return sprintf(buffer, "%ld\n", ((long)reg) * OFFSET_STEP);
+}
+
+static ssize_t pcf2123_adjust_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buffer, size_t count)
+{
+	ssize_t ret;
+	long val;
+	s8 reg;
+
+	ret = kstrtol(buffer, 10, &val);
+	if (ret)
+		return ret;
+
+	if (val > OFFSET_STEP * 127)
+		reg = 127;
+	else if (val < OFFSET_STEP * -128)
+		reg = -128;
+	else
+		reg = (s8)((val + (OFFSET_STEP >> 1)) / OFFSET_STEP);
+
+/*
+ * Each even value of the fine adjust overlaps with a value of the coarse
+ * adjustment, and since the coarse adjsutment will spread the adjustments
+ * over both hours, we use coarse for all even values, as well as values
+ * that are beyond the range of fine adjustment
+ */
+	if (reg <= 63 && reg >= -64 && reg & 1) {
+		reg &= ~OFFSET_COARSE;
+	} else {
+		reg >>= 1;
+		reg |= OFFSET_COARSE;
+	}
+
+	pcf2123_write_reg(dev, PCF2123_REG_OFFSET, reg);
+	if (ret < 0)
+		return -EIO;
+
+	return count;
+}
+
 static int pcf2123_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	u8 rxbuf[7];
@@ -320,6 +385,8 @@ static DEVICE_ATTR(c, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
 static DEVICE_ATTR(d, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
 static DEVICE_ATTR(e, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
 static DEVICE_ATTR(f, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
+static DEVICE_ATTR(adjust, S_IRUGO | S_IWUSR, pcf2123_adjust_show,
+		   pcf2123_adjust_store);
 
 static struct attribute *pcf2123_attrs[] = {
 	&dev_attr_0.attr,
@@ -338,6 +405,7 @@ static struct attribute *pcf2123_attrs[] = {
 	&dev_attr_d.attr,
 	&dev_attr_e.attr,
 	&dev_attr_f.attr,
+	&dev_attr_adjust.attr,
 	NULL
 };
 
-- 
2.5.0

Re: [PATCH 0/9] rtc-2123: access the clock offset feature

[Joshua Clayton]

On Wednesday, November 04, 2015 07:36:31 AM Joshua Clayton wrote:
> Greetings,
> This series was prompted by a need to adjust the clock rate of the rtc
> The existing code performs a soft reset during probe, which wipes out
> several registers including the offset register, which performs adjustments
> to the clock rate.
> 
> The first several patches are cleanup, with patch 5 and 6 avoiding the reset,
> and patch 9 adding a nice sysfs interface to the clock offset.
> 
> I know that this is not the only rtc to provide a programmable clock offset
> I wonder if this interface would make a good addition to the rtc api?
> 
> The rtc chips I have seen list their clock adjustments in parts per million.
> I went with parts per billion, since the ppm listed was listed with a
> fractional component.
> 
> Joshua Clayton (9):
>   rtc-pcf2123: Document all registers and useful bits
>   rtc-pcf2123: clean up reads from the chip
>   rtc-pcf2123: clean up writes to the rtc chip
>   rtc-pcf2123: replace magic numbers with defines
>   rtc-pcf2123: put the chip reset into a function
>   rtc-pcf2123: avoid resetting the clock if possible
>   rtc-pcf2123: allow sysfs to accept hexidecimal
>   rtc-pcf2123: use sysfs groups
>   rtc-pcf2123: adjust the clock rate via sysfs
> 
>  drivers/rtc/rtc-pcf2123.c | 391 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 257 insertions(+), 134 deletions(-)
> 
> 
Any comments on this series?
I realize now that I submitted it during the merge window, so it may have been overlooked.

-- 
~Joshua Clayton

Re: [PATCH 0/9] rtc-2123: access the clock offset feature

[Alexandre Belloni]

On 17/11/2015 at 07:30:48 -0800, Joshua Clayton wrote :
> On Wednesday, November 04, 2015 07:36:31 AM Joshua Clayton wrote:
> > Greetings,
> > This series was prompted by a need to adjust the clock rate of the rtc
> > The existing code performs a soft reset during probe, which wipes out
> > several registers including the offset register, which performs adjustments
> > to the clock rate.
> > 
> > The first several patches are cleanup, with patch 5 and 6 avoiding the reset,
> > and patch 9 adding a nice sysfs interface to the clock offset.
> > 
> > I know that this is not the only rtc to provide a programmable clock offset
> > I wonder if this interface would make a good addition to the rtc api?
> > 
> > The rtc chips I have seen list their clock adjustments in parts per million.
> > I went with parts per billion, since the ppm listed was listed with a
> > fractional component.
> > 
> > Joshua Clayton (9):
> >   rtc-pcf2123: Document all registers and useful bits
> >   rtc-pcf2123: clean up reads from the chip
> >   rtc-pcf2123: clean up writes to the rtc chip
> >   rtc-pcf2123: replace magic numbers with defines
> >   rtc-pcf2123: put the chip reset into a function
> >   rtc-pcf2123: avoid resetting the clock if possible
> >   rtc-pcf2123: allow sysfs to accept hexidecimal
> >   rtc-pcf2123: use sysfs groups
> >   rtc-pcf2123: adjust the clock rate via sysfs
> > 
> >  drivers/rtc/rtc-pcf2123.c | 391 ++++++++++++++++++++++++++++++----------------
> >  1 file changed, 257 insertions(+), 134 deletions(-)
> > 
> > 
> Any comments on this series?
> I realize now that I submitted it during the merge window, so it may have been overlooked.
> 

I will have a few comments but I didn't review everything thoroughly
yet. As you mentioned, you submitted during the merge window so this was
not going to be in 4.4 anyway.

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

Re: [PATCH 9/9] rtc-pcf2123: adjust the clock rate via sysfs

[Joshua Clayton]

On Wednesday, November 04, 2015 07:36:40 AM Joshua Clayton wrote:
> 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.
> 
> The adjustment is calculated in parts per billion. The data sheet
> uses parts per million (as do some others), but with 2 digits of
> precision. Since floating point is forbidden, parts per billion is
> a better fit.
> 
> Add a pair of sysfs files to seetand retrieve the offset in ppm.
Bah.
I noticed a couple of typos in the last line of the commit message.
Will get rid of "in ppm" since it is not in ppm.
s/seetand/set and/

-- 
~Joshua Clayton

Re: [PATCH 8/9] rtc-pcf2123: use sysfs groups

[Joshua Clayton]

On Wednesday, November 04, 2015 07:36:39 AM Joshua Clayton wrote:
> Switch from manually creating all the sysfs attributes to
> defining them mostly in the canonical way.
> We are adding them to an spi driver, so we must still add and remove
> the group manually, but everythig else is done by The Book(tm) .
s/everythig/everything/

-- 
~Joshua Clayton

Re: [PATCH 0/9] rtc-2123: access the clock offset feature

[Joshua Clayton]

On Tuesday, November 17, 2015 05:25:30 PM Alexandre Belloni wrote:
> On 17/11/2015 at 07:30:48 -0800, Joshua Clayton wrote :
> > On Wednesday, November 04, 2015 07:36:31 AM Joshua Clayton wrote:
> > Any comments on this series?
> > I realize now that I submitted it during the merge window, so it may have been overlooked.
> > 
> 
> I will have a few comments but I didn't review everything thoroughly
> yet. As you mentioned, you submitted during the merge window so this was
> not going to be in 4.4 anyway.
> 
> 
Thanks!
I can relax since I know it is on your radar.
I have responded my self to a couple of fat fingers in the commit messages.
I am also in the process of testing the actual effects of the feature.

One thing I'm not sure I have adequately described (and didn't really
understand until I started testing) is that a positive value in the adjust
register increases the average value of a second, so it makes the clock
slower, while a negative adjustment makes it faster.

This was counterintuitive to me.
Not having deeply examined other rtc's with similar capabilities, I can't
say whether this is the norm.

-- 
~Joshua Clayton

Re: [PATCH 1/9] rtc-pcf2123: Document all registers and useful bits

[Alexandre Belloni]

Hi,

On 04/11/2015 at 07:36:32 -0800, Joshua Clayton wrote :
> Document all 16 registers in the pcf2123, as well as
> useful bit masks from the Control1 and seconds registers
> 
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> ---
>  drivers/rtc/rtc-pcf2123.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
> index d1953bb..7756210 100644
> --- a/drivers/rtc/rtc-pcf2123.c
> +++ b/drivers/rtc/rtc-pcf2123.c
> @@ -47,6 +47,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 */
> @@ -56,7 +57,27 @@
>  #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)
> +#define PCF2123_REG_MAX		(PCF2123_REG_CTDWN_TMR)
> +
> +/* PCF2123_REG_CTRL1 BITS */
> +#define CTRL1_CLEAR		(0x00)	/* Clear */
> +#define CTRL1_CORRECTION_INT	(0x02)	/* Correction Interrupt */
> +#define CTRL1_12_HOUR		(0x04)	/* 12 hour time */
> +#define CTRL1_STOP		(0x20)	/* Stop the clock */
> +#define CTRL1_SW_RESET		(0x58)	/* Software reset */
> +#define CTRL1_EXT_TEST		(0x80)	/* External Clock Test mode */
> +
> +/* PCF2123_REG_SC BITS */
> +#define OSC_HAS_STOPPED		(0x80)	/* Clock has been stopped */
> +

Can you use the BIT() macro?


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

Re: [PATCH 3/9] rtc-pcf2123: clean up writes to the rtc chip

[Alexandre Belloni]

On 04/11/2015 at 07:36:34 -0800, Joshua Clayton wrote :
> +static int pcf2123_write(struct device *dev, u8 *txbuf, size_t size)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	int ret;
> +
> +	if (txbuf[0] > PCF2123_REG_MAX)
> +		return -EFAULT;
> +

Is that test really necessary? From what I understand the driver always
controls which register is written.

> +	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)
>  {
> @@ -142,9 +166,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;
>  
> @@ -160,12 +182,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;
>  }
>  
> @@ -205,7 +224,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;
>  
> @@ -216,15 +234,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, 0x20);
>  	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);
> @@ -233,18 +248,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, 0x00);
>  	if (ret < 0)
>  		return ret;
> -	pcf2123_delay_trec();
>  
>  	return 0;
>  }
> @@ -258,7 +269,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),
> @@ -268,24 +279,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, 0x58);
>  	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, 0x20);
>  	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");
> @@ -306,12 +309,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, 0x00);
>  	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
> 

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

Re: [PATCH 5/9] rtc-pcf2123: put the chip reset into a function

[Alexandre Belloni]

On 04/11/2015 at 07:36:36 -0800, Joshua Clayton wrote :

A commit message, even when simple is mandatory.

> 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 257ce7d..d3c1447 100644
> --- a/drivers/rtc/rtc-pcf2123.c
> +++ b/drivers/rtc/rtc-pcf2123.c
> @@ -260,6 +260,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]);

This is not properly aligned (use checkpatch --strict)


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

Re: [PATCH 6/9] rtc-pcf2123: avoid resetting the clock if possible

[Alexandre Belloni]

On 04/11/2015 at 07:36:37 -0800, Joshua Clayton wrote :
> +	ret = pcf2123_rtc_read_time(dev, &tm);
> +	if (ret < 0)
> +		return false;
> +
> +	if (rtc_valid_tm(&tm) < 0) {
> +		dev_err(dev, "retrieved date/time is not valid.\n");
> +		return false;
> +	}
> +

I would remove that test as basically, the date/time will only be valid
when OSC_HAS_STOPPED is not set.

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

Re: [PATCH 8/9] rtc-pcf2123: use sysfs groups

[Alexandre Belloni]

Hi,

This is okay but I'm not actually sure about the usefulness of that
interface. The driver is exactly here to abstract access to the
registers. Limit it to registers that are not used by the driver? Also,
It would probably better live in debugfs rather than in sysfs.

On 04/11/2015 at 07:36:39 -0800, Joshua Clayton wrote :
> Switch from manually creating all the sysfs attributes to
> defining them mostly in the canonical way.
> We are adding them to an spi driver, so we must still add and remove
> the group manually, but everythig else is done by The Book(tm) .
> 
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> ---
>  drivers/rtc/rtc-pcf2123.c | 118 +++++++++++++++++++++-------------------------
>  1 file changed, 55 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
> index 6701e6d..d494638 100644
> --- a/drivers/rtc/rtc-pcf2123.c
> +++ b/drivers/rtc/rtc-pcf2123.c
> @@ -84,16 +84,6 @@
>  
>  static struct spi_driver pcf2123_driver;
>  
> -struct pcf2123_sysfs_reg {
> -	struct device_attribute attr;
> -	char name[2];
> -};
> -
> -struct pcf2123_plat_data {
> -	struct rtc_device *rtc;
> -	struct pcf2123_sysfs_reg regs[16];
> -};
> -
>  /*
>   * Causes a 30 nanosecond delay to ensure that the PCF2123 chip select
>   * is released properly after an SPI write.  This function should be
> @@ -146,17 +136,11 @@ static int pcf2123_write_reg(struct device *dev, u8 reg, u8 val)
>  static ssize_t pcf2123_show(struct device *dev, struct device_attribute *attr,
>  			    char *buffer)
>  {
> -	struct pcf2123_sysfs_reg *r;
>  	u8 rxbuf[1];
>  	unsigned long reg;
>  	int ret;
>  
> -	r = container_of(attr, struct pcf2123_sysfs_reg, attr);
> -
> -	ret = kstrtoul(r->name, 16, &reg);
> -	if (ret)
> -		return ret;
> -
> +	reg = hex_to_bin(attr->attr.name[0]);
>  	ret = pcf2123_read(dev, reg, rxbuf, 1);
>  	if (ret < 0)
>  		return -EIO;
> @@ -166,25 +150,19 @@ 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 pcf2123_sysfs_reg *r;
>  	unsigned long reg;
>  	unsigned long val;
> -
>  	int ret;
>  
> -	r = container_of(attr, struct pcf2123_sysfs_reg, attr);
> -
> -	ret = kstrtoul(r->name, 16, &reg);
> -	if (ret)
> -		return ret;
> -
>  	ret = kstrtoul(buffer, 0, &val);
>  	if (ret)
>  		return ret;
>  
> +	reg = hex_to_bin(attr->attr.name[0]);
>  	pcf2123_write_reg(dev, reg, val);
>  	if (ret < 0)
>  		return -EIO;
> +
>  	return count;
>  }
>  
> @@ -326,17 +304,56 @@ static const struct rtc_class_ops pcf2123_rtc_ops = {
>  	.set_time	= pcf2123_rtc_set_time,
>  };
>  
> +static DEVICE_ATTR(0, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(1, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(2, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(3, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(4, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(5, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(6, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(7, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(8, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(9, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(a, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(b, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(c, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(d, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(e, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +static DEVICE_ATTR(f, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> +
> +static struct attribute *pcf2123_attrs[] = {
> +	&dev_attr_0.attr,
> +	&dev_attr_1.attr,
> +	&dev_attr_2.attr,
> +	&dev_attr_3.attr,
> +	&dev_attr_4.attr,
> +	&dev_attr_5.attr,
> +	&dev_attr_6.attr,
> +	&dev_attr_7.attr,
> +	&dev_attr_8.attr,
> +	&dev_attr_9.attr,
> +	&dev_attr_a.attr,
> +	&dev_attr_b.attr,
> +	&dev_attr_c.attr,
> +	&dev_attr_d.attr,
> +	&dev_attr_e.attr,
> +	&dev_attr_f.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group pcf2123_group = {
> +	.attrs = pcf2123_attrs,
> +};
> +
> +static const struct attribute_group *pcf2123_groups[] = {
> +	&pcf2123_group,
> +	NULL
> +};
> +
>  static int pcf2123_probe(struct spi_device *spi)
>  {
>  	struct rtc_device *rtc;
> -	struct pcf2123_plat_data *pdata;
> -	int ret, i;
> -
> -	pdata = devm_kzalloc(&spi->dev, sizeof(struct pcf2123_plat_data),
> -				GFP_KERNEL);
> -	if (!pdata)
> -		return -ENOMEM;
> -	spi->dev.platform_data = pdata;
> +	int ret;
>  
>  	if (!pcf2123_time_valid(&spi->dev)) {
>  		ret = pcf2123_reset(&spi->dev);
> @@ -360,29 +377,13 @@ static int pcf2123_probe(struct spi_device *spi)
>  		goto kfree_exit;
>  	}
>  
> -	pdata->rtc = rtc;
> -
> -	for (i = 0; i < 16; i++) {
> -		sysfs_attr_init(&pdata->regs[i].attr.attr);
> -		sprintf(pdata->regs[i].name, "%1x", i);
> -		pdata->regs[i].attr.attr.mode = S_IRUGO | S_IWUSR;
> -		pdata->regs[i].attr.attr.name = pdata->regs[i].name;
> -		pdata->regs[i].attr.show = pcf2123_show;
> -		pdata->regs[i].attr.store = pcf2123_store;
> -		ret = device_create_file(&spi->dev, &pdata->regs[i].attr);
> -		if (ret) {
> -			dev_err(&spi->dev, "Unable to create sysfs %s\n",
> -				pdata->regs[i].name);
> -			goto sysfs_exit;
> -		}
> -	}
> +	ret = sysfs_create_groups(&spi->dev.kobj, pcf2123_groups);
> +	if (ret)
> +		goto sysfs_exit;
>  
>  	return 0;
> -
>  sysfs_exit:
> -	for (i--; i >= 0; i--)
> -		device_remove_file(&spi->dev, &pdata->regs[i].attr);
> -
> +	sysfs_remove_groups(&spi->dev.kobj, pcf2123_groups);
>  kfree_exit:
>  	spi->dev.platform_data = NULL;
>  	return ret;
> @@ -390,16 +391,7 @@ kfree_exit:
>  
>  static int pcf2123_remove(struct spi_device *spi)
>  {
> -	struct pcf2123_plat_data *pdata = dev_get_platdata(&spi->dev);
> -	int i;
> -
> -	if (pdata) {
> -		for (i = 0; i < 16; i++)
> -			if (pdata->regs[i].name[0])
> -				device_remove_file(&spi->dev,
> -						   &pdata->regs[i].attr);
> -	}
> -
> +	sysfs_remove_groups(&spi->dev.kobj, pcf2123_groups);
>  	return 0;
>  }
>  
> -- 
> 2.5.0
> 

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

Re: [PATCH 1/9] rtc-pcf2123: Document all registers and useful bits

[Joshua Clayton]

On Tue, 24 Nov 2015 22:51:36 +0100
Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:

> Hi,
> 
> On 04/11/2015 at 07:36:32 -0800, Joshua Clayton wrote :
> > Document all 16 registers in the pcf2123, as well as
> > useful bit masks from the Control1 and seconds registers
> > 
> > Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> > ---
> >  drivers/rtc/rtc-pcf2123.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
> > index d1953bb..7756210 100644
> > --- a/drivers/rtc/rtc-pcf2123.c
> > +++ b/drivers/rtc/rtc-pcf2123.c
> > @@ -47,6 +47,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 */ @@ -56,7 +57,27 @@
> >  #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) +#define
> > PCF2123_REG_MAX		(PCF2123_REG_CTDWN_TMR) +
> > +/* PCF2123_REG_CTRL1 BITS */
> > +#define CTRL1_CLEAR		(0x00)	/* Clear */
> > +#define CTRL1_CORRECTION_INT	(0x02)	/* Correction
> > Interrupt */ +#define CTRL1_12_HOUR		(0x04)	/*
> > 12 hour time */ +#define CTRL1_STOP		(0x20)	/*
> > Stop the clock */ +#define CTRL1_SW_RESET
> > (0x58)	/* Software reset */ +#define
> > CTRL1_EXT_TEST		(0x80)	/* External Clock Test
> > mode */ + +/* PCF2123_REG_SC BITS */
> > +#define OSC_HAS_STOPPED		(0x80)	/* Clock has
> > been stopped */ +
> 
> Can you use the BIT() macro?
> 
> 

Sure. I Will change that in v2

Re: [PATCH 3/9] rtc-pcf2123: clean up writes to the rtc chip

[Joshua Clayton]

On Tue, 24 Nov 2015 23:16:26 +0100
Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:

> On 04/11/2015 at 07:36:34 -0800, Joshua Clayton wrote :
> > +static int pcf2123_write(struct device *dev, u8 *txbuf, size_t
> > size) +{
> > +	struct spi_device *spi = to_spi_device(dev);
> > +	int ret;
> > +
> > +	if (txbuf[0] > PCF2123_REG_MAX)
> > +		return -EFAULT;
> > +
> 
> Is that test really necessary? From what I understand the driver
> always controls which register is written.
> 

In the larger context of the driver, you are correct, there is 
no way for an out of range request unless someone were to add new code.
I can remove it. 

Re: [PATCH 5/9] rtc-pcf2123: put the chip reset into a function

[Joshua Clayton]

On Wed, 25 Nov 2015 00:17:36 +0100
Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:

> On 04/11/2015 at 07:36:36 -0800, Joshua Clayton wrote :
> 
> A commit message, even when simple is mandatory.
> 
Sorry about that. I know better. will fix for v2
 
> > +	/* 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]);
> 
> This is not properly aligned (use checkpatch --strict)
> 
> 
Will fix.

Re: [PATCH 6/9] rtc-pcf2123: avoid resetting the clock if possible

[Joshua Clayton]

On Wed, 25 Nov 2015 00:25:12 +0100
Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:

> On 04/11/2015 at 07:36:37 -0800, Joshua Clayton wrote :
> > +	ret = pcf2123_rtc_read_time(dev, &tm);
> > +	if (ret < 0)
> > +		return false;
> > +
> > +	if (rtc_valid_tm(&tm) < 0) {
> > +		dev_err(dev, "retrieved date/time is not
> > valid.\n");
> > +		return false;
> > +	}
> > +
> 
> I would remove that test as basically, the date/time will only be
> valid when OSC_HAS_STOPPED is not set.
> 
OSC_HAS_STOPPED really only protects us in case everything else
looks good, but we've lost power.
There are other reasons to check for a valid time.
Specifically, if there is no communication with the device, the spi
operation will succeed and return all zeros or all ones.
Since either of these results in an invalid time, it is a nice way
to probe whether we really have a pcf2123 compatible device.

I don't think I should remove this test, but I can add a comment

Re: [PATCH 8/9] rtc-pcf2123: use sysfs groups

[Joshua Clayton]

On Wed, 25 Nov 2015 00:31:01 +0100
Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:

> Hi,
> 
> This is okay but I'm not actually sure about the usefulness of that
> interface. The driver is exactly here to abstract access to the
> registers. Limit it to registers that are not used by the driver?
> Also, It would probably better live in debugfs rather than in sysfs.
> 

I agree completely... can I do that? Can I just move them to debugfs or
remove them?
I ask because these sysfs register files are in mainline already, and
I know removing user facing kernel features is usually frowned on.

> On 04/11/2015 at 07:36:39 -0800, Joshua Clayton wrote :
> > Switch from manually creating all the sysfs attributes to
> > defining them mostly in the canonical way.
> > We are adding them to an spi driver, so we must still add and remove
> > the group manually, but everythig else is done by The Book(tm) .
> > 
> > Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> > ---
> >  drivers/rtc/rtc-pcf2123.c | 118
> > +++++++++++++++++++++------------------------- 1 file changed, 55
> > insertions(+), 63 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
> > index 6701e6d..d494638 100644
> > --- a/drivers/rtc/rtc-pcf2123.c
> > +++ b/drivers/rtc/rtc-pcf2123.c
> > @@ -84,16 +84,6 @@
> >  
> >  static struct spi_driver pcf2123_driver;
> >  
> > -struct pcf2123_sysfs_reg {
> > -	struct device_attribute attr;
> > -	char name[2];
> > -};
> > -
> > -struct pcf2123_plat_data {
> > -	struct rtc_device *rtc;
> > -	struct pcf2123_sysfs_reg regs[16];
> > -};
> > -
> >  /*
> >   * Causes a 30 nanosecond delay to ensure that the PCF2123 chip
> > select
> >   * is released properly after an SPI write.  This function should
> > be @@ -146,17 +136,11 @@ static int pcf2123_write_reg(struct device
> > *dev, u8 reg, u8 val) static ssize_t pcf2123_show(struct device
> > *dev, struct device_attribute *attr, char *buffer)
> >  {
> > -	struct pcf2123_sysfs_reg *r;
> >  	u8 rxbuf[1];
> >  	unsigned long reg;
> >  	int ret;
> >  
> > -	r = container_of(attr, struct pcf2123_sysfs_reg, attr);
> > -
> > -	ret = kstrtoul(r->name, 16, &reg);
> > -	if (ret)
> > -		return ret;
> > -
> > +	reg = hex_to_bin(attr->attr.name[0]);
> >  	ret = pcf2123_read(dev, reg, rxbuf, 1);
> >  	if (ret < 0)
> >  		return -EIO;
> > @@ -166,25 +150,19 @@ 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 pcf2123_sysfs_reg *r;
> >  	unsigned long reg;
> >  	unsigned long val;
> > -
> >  	int ret;
> >  
> > -	r = container_of(attr, struct pcf2123_sysfs_reg, attr);
> > -
> > -	ret = kstrtoul(r->name, 16, &reg);
> > -	if (ret)
> > -		return ret;
> > -
> >  	ret = kstrtoul(buffer, 0, &val);
> >  	if (ret)
> >  		return ret;
> >  
> > +	reg = hex_to_bin(attr->attr.name[0]);
> >  	pcf2123_write_reg(dev, reg, val);
> >  	if (ret < 0)
> >  		return -EIO;
> > +
> >  	return count;
> >  }
> >  
> > @@ -326,17 +304,56 @@ static const struct rtc_class_ops
> > pcf2123_rtc_ops = { .set_time	= pcf2123_rtc_set_time,
> >  };
> >  
> > +static DEVICE_ATTR(0, S_IRUGO | S_IWUSR, pcf2123_show,
> > pcf2123_store); +static DEVICE_ATTR(1, S_IRUGO | S_IWUSR,
> > pcf2123_show, pcf2123_store); +static DEVICE_ATTR(2, S_IRUGO |
> > S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(3,
> > S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static
> > DEVICE_ATTR(4, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> > +static DEVICE_ATTR(5, S_IRUGO | S_IWUSR, pcf2123_show,
> > pcf2123_store); +static DEVICE_ATTR(6, S_IRUGO | S_IWUSR,
> > pcf2123_show, pcf2123_store); +static DEVICE_ATTR(7, S_IRUGO |
> > S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(8,
> > S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static
> > DEVICE_ATTR(9, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> > +static DEVICE_ATTR(a, S_IRUGO | S_IWUSR, pcf2123_show,
> > pcf2123_store); +static DEVICE_ATTR(b, S_IRUGO | S_IWUSR,
> > pcf2123_show, pcf2123_store); +static DEVICE_ATTR(c, S_IRUGO |
> > S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(d,
> > S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static
> > DEVICE_ATTR(e, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> > +static DEVICE_ATTR(f, S_IRUGO | S_IWUSR, pcf2123_show,
> > pcf2123_store); + +static struct attribute *pcf2123_attrs[] = {
> > +	&dev_attr_0.attr,
> > +	&dev_attr_1.attr,
> > +	&dev_attr_2.attr,
> > +	&dev_attr_3.attr,
> > +	&dev_attr_4.attr,
> > +	&dev_attr_5.attr,
> > +	&dev_attr_6.attr,
> > +	&dev_attr_7.attr,
> > +	&dev_attr_8.attr,
> > +	&dev_attr_9.attr,
> > +	&dev_attr_a.attr,
> > +	&dev_attr_b.attr,
> > +	&dev_attr_c.attr,
> > +	&dev_attr_d.attr,
> > +	&dev_attr_e.attr,
> > +	&dev_attr_f.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group pcf2123_group = {
> > +	.attrs = pcf2123_attrs,
> > +};
> > +
> > +static const struct attribute_group *pcf2123_groups[] = {
> > +	&pcf2123_group,
> > +	NULL
> > +};
> > +
> >  static int pcf2123_probe(struct spi_device *spi)
> >  {
> >  	struct rtc_device *rtc;
> > -	struct pcf2123_plat_data *pdata;
> > -	int ret, i;
> > -
> > -	pdata = devm_kzalloc(&spi->dev, sizeof(struct
> > pcf2123_plat_data),
> > -				GFP_KERNEL);
> > -	if (!pdata)
> > -		return -ENOMEM;
> > -	spi->dev.platform_data = pdata;
> > +	int ret;
> >  
> >  	if (!pcf2123_time_valid(&spi->dev)) {
> >  		ret = pcf2123_reset(&spi->dev);
> > @@ -360,29 +377,13 @@ static int pcf2123_probe(struct spi_device
> > *spi) goto kfree_exit;
> >  	}
> >  
> > -	pdata->rtc = rtc;
> > -
> > -	for (i = 0; i < 16; i++) {
> > -		sysfs_attr_init(&pdata->regs[i].attr.attr);
> > -		sprintf(pdata->regs[i].name, "%1x", i);
> > -		pdata->regs[i].attr.attr.mode = S_IRUGO | S_IWUSR;
> > -		pdata->regs[i].attr.attr.name =
> > pdata->regs[i].name;
> > -		pdata->regs[i].attr.show = pcf2123_show;
> > -		pdata->regs[i].attr.store = pcf2123_store;
> > -		ret = device_create_file(&spi->dev,
> > &pdata->regs[i].attr);
> > -		if (ret) {
> > -			dev_err(&spi->dev, "Unable to create sysfs
> > %s\n",
> > -				pdata->regs[i].name);
> > -			goto sysfs_exit;
> > -		}
> > -	}
> > +	ret = sysfs_create_groups(&spi->dev.kobj, pcf2123_groups);
> > +	if (ret)
> > +		goto sysfs_exit;
> >  
> >  	return 0;
> > -
> >  sysfs_exit:
> > -	for (i--; i >= 0; i--)
> > -		device_remove_file(&spi->dev,
> > &pdata->regs[i].attr); -
> > +	sysfs_remove_groups(&spi->dev.kobj, pcf2123_groups);
> >  kfree_exit:
> >  	spi->dev.platform_data = NULL;
> >  	return ret;
> > @@ -390,16 +391,7 @@ kfree_exit:
> >  
> >  static int pcf2123_remove(struct spi_device *spi)
> >  {
> > -	struct pcf2123_plat_data *pdata =
> > dev_get_platdata(&spi->dev);
> > -	int i;
> > -
> > -	if (pdata) {
> > -		for (i = 0; i < 16; i++)
> > -			if (pdata->regs[i].name[0])
> > -				device_remove_file(&spi->dev,
> > -
> > &pdata->regs[i].attr);
> > -	}
> > -
> > +	sysfs_remove_groups(&spi->dev.kobj, pcf2123_groups);
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.5.0
> > 
> 

Re: [PATCH 8/9] rtc-pcf2123: use sysfs groups

[Alexandre Belloni]

On 01/12/2015 at 12:28:11 -0800, Joshua Clayton wrote :
>
> On Wed, 25 Nov 2015 00:31:01 +0100
> Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:
> 
> > Hi,
> > 
> > This is okay but I'm not actually sure about the usefulness of that
> > interface. The driver is exactly here to abstract access to the
> > registers. Limit it to registers that are not used by the driver?
> > Also, It would probably better live in debugfs rather than in sysfs.
> > 
> 
> I agree completely... can I do that? Can I just move them to debugfs or
> remove them?
> I ask because these sysfs register files are in mainline already, and
> I know removing user facing kernel features is usually frowned on.
> 

My guess is that they haven't been documented anyway. If that is the
case, I wouldn't mind removing them. My thinking is that using them in a
production system is totally insane.

> > On 04/11/2015 at 07:36:39 -0800, Joshua Clayton wrote :
> > > Switch from manually creating all the sysfs attributes to
> > > defining them mostly in the canonical way.
> > > We are adding them to an spi driver, so we must still add and remove
> > > the group manually, but everythig else is done by The Book(tm) .
> > > 
> > > Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> > > ---
> > >  drivers/rtc/rtc-pcf2123.c | 118
> > > +++++++++++++++++++++------------------------- 1 file changed, 55
> > > insertions(+), 63 deletions(-)
> > > 
> > > diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
> > > index 6701e6d..d494638 100644
> > > --- a/drivers/rtc/rtc-pcf2123.c
> > > +++ b/drivers/rtc/rtc-pcf2123.c
> > > @@ -84,16 +84,6 @@
> > >  
> > >  static struct spi_driver pcf2123_driver;
> > >  
> > > -struct pcf2123_sysfs_reg {
> > > -	struct device_attribute attr;
> > > -	char name[2];
> > > -};
> > > -
> > > -struct pcf2123_plat_data {
> > > -	struct rtc_device *rtc;
> > > -	struct pcf2123_sysfs_reg regs[16];
> > > -};
> > > -
> > >  /*
> > >   * Causes a 30 nanosecond delay to ensure that the PCF2123 chip
> > > select
> > >   * is released properly after an SPI write.  This function should
> > > be @@ -146,17 +136,11 @@ static int pcf2123_write_reg(struct device
> > > *dev, u8 reg, u8 val) static ssize_t pcf2123_show(struct device
> > > *dev, struct device_attribute *attr, char *buffer)
> > >  {
> > > -	struct pcf2123_sysfs_reg *r;
> > >  	u8 rxbuf[1];
> > >  	unsigned long reg;
> > >  	int ret;
> > >  
> > > -	r = container_of(attr, struct pcf2123_sysfs_reg, attr);
> > > -
> > > -	ret = kstrtoul(r->name, 16, &reg);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > +	reg = hex_to_bin(attr->attr.name[0]);
> > >  	ret = pcf2123_read(dev, reg, rxbuf, 1);
> > >  	if (ret < 0)
> > >  		return -EIO;
> > > @@ -166,25 +150,19 @@ 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 pcf2123_sysfs_reg *r;
> > >  	unsigned long reg;
> > >  	unsigned long val;
> > > -
> > >  	int ret;
> > >  
> > > -	r = container_of(attr, struct pcf2123_sysfs_reg, attr);
> > > -
> > > -	ret = kstrtoul(r->name, 16, &reg);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	ret = kstrtoul(buffer, 0, &val);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	reg = hex_to_bin(attr->attr.name[0]);
> > >  	pcf2123_write_reg(dev, reg, val);
> > >  	if (ret < 0)
> > >  		return -EIO;
> > > +
> > >  	return count;
> > >  }
> > >  
> > > @@ -326,17 +304,56 @@ static const struct rtc_class_ops
> > > pcf2123_rtc_ops = { .set_time	= pcf2123_rtc_set_time,
> > >  };
> > >  
> > > +static DEVICE_ATTR(0, S_IRUGO | S_IWUSR, pcf2123_show,
> > > pcf2123_store); +static DEVICE_ATTR(1, S_IRUGO | S_IWUSR,
> > > pcf2123_show, pcf2123_store); +static DEVICE_ATTR(2, S_IRUGO |
> > > S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(3,
> > > S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static
> > > DEVICE_ATTR(4, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> > > +static DEVICE_ATTR(5, S_IRUGO | S_IWUSR, pcf2123_show,
> > > pcf2123_store); +static DEVICE_ATTR(6, S_IRUGO | S_IWUSR,
> > > pcf2123_show, pcf2123_store); +static DEVICE_ATTR(7, S_IRUGO |
> > > S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(8,
> > > S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static
> > > DEVICE_ATTR(9, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> > > +static DEVICE_ATTR(a, S_IRUGO | S_IWUSR, pcf2123_show,
> > > pcf2123_store); +static DEVICE_ATTR(b, S_IRUGO | S_IWUSR,
> > > pcf2123_show, pcf2123_store); +static DEVICE_ATTR(c, S_IRUGO |
> > > S_IWUSR, pcf2123_show, pcf2123_store); +static DEVICE_ATTR(d,
> > > S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store); +static
> > > DEVICE_ATTR(e, S_IRUGO | S_IWUSR, pcf2123_show, pcf2123_store);
> > > +static DEVICE_ATTR(f, S_IRUGO | S_IWUSR, pcf2123_show,
> > > pcf2123_store); + +static struct attribute *pcf2123_attrs[] = {
> > > +	&dev_attr_0.attr,
> > > +	&dev_attr_1.attr,
> > > +	&dev_attr_2.attr,
> > > +	&dev_attr_3.attr,
> > > +	&dev_attr_4.attr,
> > > +	&dev_attr_5.attr,
> > > +	&dev_attr_6.attr,
> > > +	&dev_attr_7.attr,
> > > +	&dev_attr_8.attr,
> > > +	&dev_attr_9.attr,
> > > +	&dev_attr_a.attr,
> > > +	&dev_attr_b.attr,
> > > +	&dev_attr_c.attr,
> > > +	&dev_attr_d.attr,
> > > +	&dev_attr_e.attr,
> > > +	&dev_attr_f.attr,
> > > +	NULL
> > > +};
> > > +
> > > +static const struct attribute_group pcf2123_group = {
> > > +	.attrs = pcf2123_attrs,
> > > +};
> > > +
> > > +static const struct attribute_group *pcf2123_groups[] = {
> > > +	&pcf2123_group,
> > > +	NULL
> > > +};
> > > +
> > >  static int pcf2123_probe(struct spi_device *spi)
> > >  {
> > >  	struct rtc_device *rtc;
> > > -	struct pcf2123_plat_data *pdata;
> > > -	int ret, i;
> > > -
> > > -	pdata = devm_kzalloc(&spi->dev, sizeof(struct
> > > pcf2123_plat_data),
> > > -				GFP_KERNEL);
> > > -	if (!pdata)
> > > -		return -ENOMEM;
> > > -	spi->dev.platform_data = pdata;
> > > +	int ret;
> > >  
> > >  	if (!pcf2123_time_valid(&spi->dev)) {
> > >  		ret = pcf2123_reset(&spi->dev);
> > > @@ -360,29 +377,13 @@ static int pcf2123_probe(struct spi_device
> > > *spi) goto kfree_exit;
> > >  	}
> > >  
> > > -	pdata->rtc = rtc;
> > > -
> > > -	for (i = 0; i < 16; i++) {
> > > -		sysfs_attr_init(&pdata->regs[i].attr.attr);
> > > -		sprintf(pdata->regs[i].name, "%1x", i);
> > > -		pdata->regs[i].attr.attr.mode = S_IRUGO | S_IWUSR;
> > > -		pdata->regs[i].attr.attr.name =
> > > pdata->regs[i].name;
> > > -		pdata->regs[i].attr.show = pcf2123_show;
> > > -		pdata->regs[i].attr.store = pcf2123_store;
> > > -		ret = device_create_file(&spi->dev,
> > > &pdata->regs[i].attr);
> > > -		if (ret) {
> > > -			dev_err(&spi->dev, "Unable to create sysfs
> > > %s\n",
> > > -				pdata->regs[i].name);
> > > -			goto sysfs_exit;
> > > -		}
> > > -	}
> > > +	ret = sysfs_create_groups(&spi->dev.kobj, pcf2123_groups);
> > > +	if (ret)
> > > +		goto sysfs_exit;
> > >  
> > >  	return 0;
> > > -
> > >  sysfs_exit:
> > > -	for (i--; i >= 0; i--)
> > > -		device_remove_file(&spi->dev,
> > > &pdata->regs[i].attr); -
> > > +	sysfs_remove_groups(&spi->dev.kobj, pcf2123_groups);
> > >  kfree_exit:
> > >  	spi->dev.platform_data = NULL;
> > >  	return ret;
> > > @@ -390,16 +391,7 @@ kfree_exit:
> > >  
> > >  static int pcf2123_remove(struct spi_device *spi)
> > >  {
> > > -	struct pcf2123_plat_data *pdata =
> > > dev_get_platdata(&spi->dev);
> > > -	int i;
> > > -
> > > -	if (pdata) {
> > > -		for (i = 0; i < 16; i++)
> > > -			if (pdata->regs[i].name[0])
> > > -				device_remove_file(&spi->dev,
> > > -
> > > &pdata->regs[i].attr);
> > > -	}
> > > -
> > > +	sysfs_remove_groups(&spi->dev.kobj, pcf2123_groups);
> > >  	return 0;
> > >  }
> > >  
> > > -- 
> > > 2.5.0
> > > 
> > 
> 

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

Re: [PATCH 6/9] rtc-pcf2123: avoid resetting the clock if possible

[Alexandre Belloni]

On 01/12/2015 at 12:23:51 -0800, Joshua Clayton wrote :
> On Wed, 25 Nov 2015 00:25:12 +0100
> Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:
> 
> > On 04/11/2015 at 07:36:37 -0800, Joshua Clayton wrote :
> > > +	ret = pcf2123_rtc_read_time(dev, &tm);
> > > +	if (ret < 0)
> > > +		return false;
> > > +
> > > +	if (rtc_valid_tm(&tm) < 0) {
> > > +		dev_err(dev, "retrieved date/time is not
> > > valid.\n");
> > > +		return false;
> > > +	}
> > > +
> > 
> > I would remove that test as basically, the date/time will only be
> > valid when OSC_HAS_STOPPED is not set.
> > 
> OSC_HAS_STOPPED really only protects us in case everything else
> looks good, but we've lost power.
> There are other reasons to check for a valid time.
> Specifically, if there is no communication with the device, the spi
> operation will succeed and return all zeros or all ones.
> Since either of these results in an invalid time, it is a nice way
> to probe whether we really have a pcf2123 compatible device.
> 
> I don't think I should remove this test, but I can add a comment

OK but then pcf2123_rtc_read_time actually returns rtc_valid_tm(tm) ;)

The proper course of action is probably to do the OSC_HAS_STOPPED check
in pcf2123_rtc_read_time then you don't even need pcf2123_time_valid().

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