[PATCH v3 1/5] rtc: pcf2127: convert to devm_rtc_allocate_device

[PATCH v3 1/5] rtc: pcf2127: convert to devm_rtc_allocate_device

[Bruno Thomsen]

This allows further improvement of the driver.

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
v3: no change.
v2: no change.

 drivers/rtc/rtc-pcf2127.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 8632f58fed43..58eb96506e4b 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -237,11 +237,12 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 
 	dev_set_drvdata(dev, pcf2127);
 
-	pcf2127->rtc = devm_rtc_device_register(dev, name, &pcf2127_rtc_ops,
-						THIS_MODULE);
+	pcf2127->rtc = devm_rtc_allocate_device(dev);
 	if (IS_ERR(pcf2127->rtc))
 		return PTR_ERR(pcf2127->rtc);
 
+	pcf2127->rtc->ops = &pcf2127_rtc_ops;
+
 	if (has_nvmem) {
 		struct nvmem_config nvmem_cfg = {
 			.priv = pcf2127,
@@ -253,7 +254,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
 	}
 
-	return ret;
+	return rtc_register_device(pcf2127->rtc);
 }
 
 #ifdef CONFIG_OF
-- 
2.21.0

[PATCH v3 2/5] rtc: pcf2127: cleanup register and bit defines

[Bruno Thomsen]

Cleanup of defines to follow kernel coding style and increase code
readability by using same register and bit define style.

Change PCF2127_REG_RAM_{addr_MSB,wrt_cmd,rd_cmd} to upper case as
kernel coding guide section 12 'Macros, Enums and RTL' states
"Names of macros defining constants and labels in enums are capitalized".

Improve readability of RAM register comment by making whole sentences.

Remove parentheses from register defines as they are only used
for expressions and not constants.

As there are no clear style for name of registers and bits in the
kernel drivers, I suggest the following for at least this driver,
but hopefully also other RTC drivers.

Register name should follow this convention:
[chip]_REG_[reg name] 0xXX

Bit name should follow this convention, so it clearly states which
chip register it's part of:
[chip]_BIT_[reg name]_[bit name] BIT(X)

Additionally I suggest bit defines are always placed right below
its corresponding register define and using an extra tab indentation
for the BIT(X) part. This will visualt make it easy to see that bit
defines are part of the complete register definition.

Rename PCF2127_OSF to PCF2127_BIT_SC_OSF and move it right below
PCF2127_REG_SC. This will improve readability of bit checks as it's
easy to verify that it uses the correct register.

Move end of line comments above register defines as it's more like
a heading for 1 register define and up to 8 bit defines or a
collection of registers that are close related like timestamp
split across 6 registers.

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
v3: no change.
v2: updated commit message.

 drivers/rtc/rtc-pcf2127.c | 59 ++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 58eb96506e4b..cd8def79b379 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -19,26 +19,32 @@
 #include <linux/of.h>
 #include <linux/regmap.h>
 
-#define PCF2127_REG_CTRL1       (0x00)  /* Control Register 1 */
-#define PCF2127_REG_CTRL2       (0x01)  /* Control Register 2 */
-
-#define PCF2127_REG_CTRL3       (0x02)  /* Control Register 3 */
-#define PCF2127_REG_CTRL3_BLF		BIT(2)
-
-#define PCF2127_REG_SC          (0x03)  /* datetime */
-#define PCF2127_REG_MN          (0x04)
-#define PCF2127_REG_HR          (0x05)
-#define PCF2127_REG_DM          (0x06)
-#define PCF2127_REG_DW          (0x07)
-#define PCF2127_REG_MO          (0x08)
-#define PCF2127_REG_YR          (0x09)
-
-/* the pcf2127 has 512 bytes nvmem, pcf2129 doesn't */
-#define PCF2127_REG_RAM_addr_MSB       0x1a
-#define PCF2127_REG_RAM_wrt_cmd        0x1c
-#define PCF2127_REG_RAM_rd_cmd         0x1d
+/* Control register 1 */
+#define PCF2127_REG_CTRL1		0x00
+/* Control register 2 */
+#define PCF2127_REG_CTRL2		0x01
+/* Control register 3 */
+#define PCF2127_REG_CTRL3		0x02
+#define PCF2127_BIT_CTRL3_BLF			BIT(2)
+/* Time and date registers */
+#define PCF2127_REG_SC			0x03
+#define PCF2127_BIT_SC_OSF			BIT(7)
+#define PCF2127_REG_MN			0x04
+#define PCF2127_REG_HR			0x05
+#define PCF2127_REG_DM			0x06
+#define PCF2127_REG_DW			0x07
+#define PCF2127_REG_MO			0x08
+#define PCF2127_REG_YR			0x09
+/*
+ * RAM registers
+ * PCF2127 has 512 bytes general-purpose static RAM (SRAM) that is
+ * battery backed and can survive a power outage.
+ * PCF2129 doesn't have this feature.
+ */
+#define PCF2127_REG_RAM_ADDR_MSB	0x1A
+#define PCF2127_REG_RAM_WRT_CMD		0x1C
+#define PCF2127_REG_RAM_RD_CMD		0x1D
 
-#define PCF2127_OSF             BIT(7)  /* Oscillator Fail flag */
 
 struct pcf2127 {
 	struct rtc_device *rtc;
@@ -73,11 +79,12 @@ static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
 		return ret;
 	}
 
-	if (buf[PCF2127_REG_CTRL3] & PCF2127_REG_CTRL3_BLF)
+	if (buf[PCF2127_REG_CTRL3] & PCF2127_BIT_CTRL3_BLF)
 		dev_info(dev,
 			"low voltage detected, check/replace RTC battery.\n");
 
-	if (buf[PCF2127_REG_SC] & PCF2127_OSF) {
+	/* Clock integrity is not guaranteed when OSF flag is set. */
+	if (buf[PCF2127_REG_SC] & PCF2127_BIT_SC_OSF) {
 		/*
 		 * no need clear the flag here,
 		 * it will be cleared once the new date is saved
@@ -166,7 +173,7 @@ static int pcf2127_rtc_ioctl(struct device *dev,
 		if (ret)
 			return ret;
 
-		touser = touser & PCF2127_REG_CTRL3_BLF ? 1 : 0;
+		touser = touser & PCF2127_BIT_CTRL3_BLF ? 1 : 0;
 
 		if (copy_to_user((void __user *)arg, &touser, sizeof(int)))
 			return -EFAULT;
@@ -192,12 +199,12 @@ static int pcf2127_nvmem_read(void *priv, unsigned int offset,
 	int ret;
 	unsigned char offsetbuf[] = { offset >> 8, offset };
 
-	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_addr_MSB,
+	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_ADDR_MSB,
 				offsetbuf, 2);
 	if (ret)
 		return ret;
 
-	ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_RAM_rd_cmd,
+	ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_RAM_RD_CMD,
 			       val, bytes);
 
 	return ret ?: bytes;
@@ -210,12 +217,12 @@ static int pcf2127_nvmem_write(void *priv, unsigned int offset,
 	int ret;
 	unsigned char offsetbuf[] = { offset >> 8, offset };
 
-	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_addr_MSB,
+	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_ADDR_MSB,
 				offsetbuf, 2);
 	if (ret)
 		return ret;
 
-	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_wrt_cmd,
+	ret = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_RAM_WRT_CMD,
 				val, bytes);
 
 	return ret ?: bytes;
-- 
2.21.0

Re: [PATCH v3 2/5] rtc: pcf2127: cleanup register and bit defines

[Alexandre Belloni]

On 22/08/2019 15:19:33+0200, Bruno Thomsen wrote:
> Cleanup of defines to follow kernel coding style and increase code
> readability by using same register and bit define style.
> 
> Change PCF2127_REG_RAM_{addr_MSB,wrt_cmd,rd_cmd} to upper case as
> kernel coding guide section 12 'Macros, Enums and RTL' states
> "Names of macros defining constants and labels in enums are capitalized".
> 
> Improve readability of RAM register comment by making whole sentences.
> 
> Remove parentheses from register defines as they are only used
> for expressions and not constants.
> 
> As there are no clear style for name of registers and bits in the
> kernel drivers, I suggest the following for at least this driver,
> but hopefully also other RTC drivers.
> 
> Register name should follow this convention:
> [chip]_REG_[reg name] 0xXX
> 
> Bit name should follow this convention, so it clearly states which
> chip register it's part of:
> [chip]_BIT_[reg name]_[bit name] BIT(X)
> 
> Additionally I suggest bit defines are always placed right below
> its corresponding register define and using an extra tab indentation
> for the BIT(X) part. This will visualt make it easy to see that bit
> defines are part of the complete register definition.
> 
> Rename PCF2127_OSF to PCF2127_BIT_SC_OSF and move it right below
> PCF2127_REG_SC. This will improve readability of bit checks as it's
> easy to verify that it uses the correct register.
> 
> Move end of line comments above register defines as it's more like
> a heading for 1 register define and up to 8 bit defines or a
> collection of registers that are close related like timestamp
> split across 6 registers.
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
> ---
> v3: no change.
> v2: updated commit message.
> 
>  drivers/rtc/rtc-pcf2127.c | 59 ++++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 26 deletions(-)
> 
Applied, even if most of the churn is annoying. However, I agree
PCF2127_OSF should have been named differently.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v3 3/5] rtc: pcf2127: bugfix: read rtc disables watchdog

[Bruno Thomsen]

The previous fix listed bulk read of registers as root cause of
accendential disabling of watchdog, since the watchdog counter
register (WD_VAL) was zeroed.

Fixes: 3769a375ab83 rtc: pcf2127: bulk read only date and time registers.

Tested with the same PCF2127 chip as Sean reveled root cause
of WD_VAL register value zeroing was caused by reading CTRL2
register which is one of the watchdog feature control registers.

So the solution is to not read the first two control registers
(CTRL1 and CTRL2) in pcf2127_rtc_read_time as they are not
needed anyway. Size of local buf variable is kept to allow
easy usage of register defines to improve readability of code.

Debug trace line was updated after CTRL1 and CTRL2 are no longer
read from the chip. Also replaced magic numbers in buf access
with register defines.

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
v3: no change.
v2: new bugfix, not in v1.

 drivers/rtc/rtc-pcf2127.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index cd8def79b379..ee4921e4a47c 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -60,20 +60,14 @@ static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
 	unsigned char buf[10];
 	int ret;
-	int i;
 
-	for (i = 0; i <= PCF2127_REG_CTRL3; i++) {
-		ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL1 + i,
-				  (unsigned int *)(buf + i));
-		if (ret) {
-			dev_err(dev, "%s: read error\n", __func__);
-			return ret;
-		}
-	}
-
-	ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_SC,
-			       (buf + PCF2127_REG_SC),
-			       ARRAY_SIZE(buf) - PCF2127_REG_SC);
+	/*
+	 * Avoid reading CTRL2 register as it causes WD_VAL register
+	 * value to reset to 0 which means watchdog is stopped.
+	 */
+	ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_CTRL3,
+			       (buf + PCF2127_REG_CTRL3),
+			       ARRAY_SIZE(buf) - PCF2127_REG_CTRL3);
 	if (ret) {
 		dev_err(dev, "%s: read error\n", __func__);
 		return ret;
@@ -95,14 +89,12 @@ static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	}
 
 	dev_dbg(dev,
-		"%s: raw data is cr1=%02x, cr2=%02x, cr3=%02x, "
-		"sec=%02x, min=%02x, hr=%02x, "
+		"%s: raw data is cr3=%02x, sec=%02x, min=%02x, hr=%02x, "
 		"mday=%02x, wday=%02x, mon=%02x, year=%02x\n",
-		__func__,
-		buf[0], buf[1], buf[2],
-		buf[3], buf[4], buf[5],
-		buf[6], buf[7], buf[8], buf[9]);
-
+		__func__, buf[PCF2127_REG_CTRL3], buf[PCF2127_REG_SC],
+		buf[PCF2127_REG_MN], buf[PCF2127_REG_HR],
+		buf[PCF2127_REG_DM], buf[PCF2127_REG_DW],
+		buf[PCF2127_REG_MO], buf[PCF2127_REG_YR]);
 
 	tm->tm_sec = bcd2bin(buf[PCF2127_REG_SC] & 0x7F);
 	tm->tm_min = bcd2bin(buf[PCF2127_REG_MN] & 0x7F);
-- 
2.21.0

Re: [PATCH v3 3/5] rtc: pcf2127: bugfix: read rtc disables watchdog

[Alexandre Belloni]

On 22/08/2019 15:19:34+0200, Bruno Thomsen wrote:
> The previous fix listed bulk read of registers as root cause of
> accendential disabling of watchdog, since the watchdog counter
> register (WD_VAL) was zeroed.
> 
> Fixes: 3769a375ab83 rtc: pcf2127: bulk read only date and time registers.
> 
> Tested with the same PCF2127 chip as Sean reveled root cause
> of WD_VAL register value zeroing was caused by reading CTRL2
> register which is one of the watchdog feature control registers.
> 
> So the solution is to not read the first two control registers
> (CTRL1 and CTRL2) in pcf2127_rtc_read_time as they are not
> needed anyway. Size of local buf variable is kept to allow
> easy usage of register defines to improve readability of code.
> 
> Debug trace line was updated after CTRL1 and CTRL2 are no longer
> read from the chip. Also replaced magic numbers in buf access
> with register defines.
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
> ---
> v3: no change.
> v2: new bugfix, not in v1.
> 
>  drivers/rtc/rtc-pcf2127.c | 32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v3 4/5] rtc: pcf2127: add watchdog feature support

[Bruno Thomsen]

Add partial support for the watchdog functionality of
both PCF2127 and PCF2129 chips.

The programmable watchdog timer is currently using a fixed
clock source of 1Hz. This result in a selectable range of
1-255 seconds, which covers most embedded Linux use-cases.

Clock sources of 4096Hz, 64Hz and 1/60Hz is mostly useful
in MCU use-cases.

Countdown timer not available when using watchdog feature.

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
v3: removed 2 x dev_info() and 1 x dev_err() traces.
    lowered dev_info() to dbg_info() in pcf2127_wdt_set_timeout.
    removed unneeded ret variable in pcf2127_wdt_set_timeout.
v2: use new watchdog api, e.g. devm_watchdog_register_device.
    remove watchdog Kconfig option.
    update existing Kconfig option with additional information.

 drivers/rtc/Kconfig       |   7 ++-
 drivers/rtc/rtc-pcf2127.c | 118 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index e72f65b61176..a3bb58a08879 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -876,7 +876,12 @@ config RTC_DRV_PCF2127
 	depends on RTC_I2C_AND_SPI
 	help
 	  If you say yes here you get support for the NXP PCF2127/29 RTC
-	  chips.
+	  chips with integrated quartz crystal for industrial applications.
+	  Both chips also have watchdog timer and tamper switch detection
+	  features.
+
+	  PCF2127 has an additional feature of 512 bytes battery backed
+	  memory that's accessible using nvmem interface.
 
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-pcf2127.
diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index ee4921e4a47c..8d6eda455d81 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -5,6 +5,9 @@
  *
  * Author: Renaud Cerrato <r.cerrato@til-technologies.fr>
  *
+ * Watchdog and tamper functions
+ * Author: Bruno Thomsen <bruno.thomsen@gmail.com>
+ *
  * based on the other drivers in this same directory.
  *
  * Datasheet: http://cache.nxp.com/documents/data_sheet/PCF2127.pdf
@@ -18,6 +21,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/regmap.h>
+#include <linux/watchdog.h>
 
 /* Control register 1 */
 #define PCF2127_REG_CTRL1		0x00
@@ -35,6 +39,13 @@
 #define PCF2127_REG_DW			0x07
 #define PCF2127_REG_MO			0x08
 #define PCF2127_REG_YR			0x09
+/* Watchdog registers */
+#define PCF2127_REG_WD_CTL		0x10
+#define PCF2127_BIT_WD_CTL_TF0			BIT(0)
+#define PCF2127_BIT_WD_CTL_TF1			BIT(1)
+#define PCF2127_BIT_WD_CTL_CD0			BIT(6)
+#define PCF2127_BIT_WD_CTL_CD1			BIT(7)
+#define PCF2127_REG_WD_VAL		0x11
 /*
  * RAM registers
  * PCF2127 has 512 bytes general-purpose static RAM (SRAM) that is
@@ -45,9 +56,15 @@
 #define PCF2127_REG_RAM_WRT_CMD		0x1C
 #define PCF2127_REG_RAM_RD_CMD		0x1D
 
+/* Watchdog timer value constants */
+#define PCF2127_WD_VAL_STOP		0
+#define PCF2127_WD_VAL_MIN		2
+#define PCF2127_WD_VAL_MAX		255
+#define PCF2127_WD_VAL_DEFAULT		60
 
 struct pcf2127 {
 	struct rtc_device *rtc;
+	struct watchdog_device wdd;
 	struct regmap *regmap;
 };
 
@@ -220,6 +237,74 @@ static int pcf2127_nvmem_write(void *priv, unsigned int offset,
 	return ret ?: bytes;
 }
 
+/* watchdog driver */
+
+static int pcf2127_wdt_ping(struct watchdog_device *wdd)
+{
+	struct pcf2127 *pcf2127 = watchdog_get_drvdata(wdd);
+
+	return regmap_write(pcf2127->regmap, PCF2127_REG_WD_VAL, wdd->timeout);
+}
+
+/*
+ * Restart watchdog timer if feature is active.
+ *
+ * Note: Reading CTRL2 register causes watchdog to stop which is unfortunate,
+ * since register also contain control/status flags for other features.
+ * Always call this function after reading CTRL2 register.
+ */
+static int pcf2127_wdt_active_ping(struct watchdog_device *wdd)
+{
+	int ret = 0;
+
+	if (watchdog_active(wdd)) {
+		ret = pcf2127_wdt_ping(wdd);
+		if (ret)
+			dev_err(wdd->parent,
+				"%s: watchdog restart failed, ret=%d\n",
+				__func__, ret);
+	}
+
+	return ret;
+}
+
+static int pcf2127_wdt_start(struct watchdog_device *wdd)
+{
+	return pcf2127_wdt_ping(wdd);
+}
+
+static int pcf2127_wdt_stop(struct watchdog_device *wdd)
+{
+	struct pcf2127 *pcf2127 = watchdog_get_drvdata(wdd);
+
+	return regmap_write(pcf2127->regmap, PCF2127_REG_WD_VAL,
+			    PCF2127_WD_VAL_STOP);
+}
+
+static int pcf2127_wdt_set_timeout(struct watchdog_device *wdd,
+				   unsigned int new_timeout)
+{
+	dev_dbg(wdd->parent, "new watchdog timeout: %is (old: %is)\n",
+		new_timeout, wdd->timeout);
+
+	wdd->timeout = new_timeout;
+
+	return pcf2127_wdt_active_ping(wdd);
+}
+
+static const struct watchdog_info pcf2127_wdt_info = {
+	.identity = "NXP PCF2127/PCF2129 Watchdog",
+	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+};
+
+static const struct watchdog_ops pcf2127_watchdog_ops = {
+	.owner = THIS_MODULE,
+	.start = pcf2127_wdt_start,
+	.stop = pcf2127_wdt_stop,
+	.ping = pcf2127_wdt_ping,
+	.set_timeout = pcf2127_wdt_set_timeout,
+};
+
 static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 			const char *name, bool has_nvmem)
 {
@@ -242,6 +327,16 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 
 	pcf2127->rtc->ops = &pcf2127_rtc_ops;
 
+	pcf2127->wdd.parent = dev;
+	pcf2127->wdd.info = &pcf2127_wdt_info;
+	pcf2127->wdd.ops = &pcf2127_watchdog_ops;
+	pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
+	pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
+	pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
+	pcf2127->wdd.min_hw_heartbeat_ms = 500;
+
+	watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
+
 	if (has_nvmem) {
 		struct nvmem_config nvmem_cfg = {
 			.priv = pcf2127,
@@ -253,6 +348,29 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
 	}
 
+	/*
+	 * Watchdog timer enabled and reset pin /RST activated when timed out.
+	 * Select 1Hz clock source for watchdog timer.
+	 * Timer is not started until WD_VAL is loaded with a valid value.
+	 * Note: Countdown timer disabled and not available.
+	 */
+	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
+				 PCF2127_BIT_WD_CTL_CD1 |
+				 PCF2127_BIT_WD_CTL_CD0 |
+				 PCF2127_BIT_WD_CTL_TF1 |
+				 PCF2127_BIT_WD_CTL_TF0,
+				 PCF2127_BIT_WD_CTL_CD1 |
+				 PCF2127_BIT_WD_CTL_CD0 |
+				 PCF2127_BIT_WD_CTL_TF1);
+	if (ret) {
+		dev_err(dev, "%s: watchdog config (wd_ctl) failed\n", __func__);
+		return ret;
+	}
+
+	ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
+	if (ret)
+		return ret;
+
 	return rtc_register_device(pcf2127->rtc);
 }
 
-- 
2.21.0

Re: [PATCH v3 4/5] rtc: pcf2127: add watchdog feature support

[Guenter Roeck]

On Thu, Aug 22, 2019 at 03:19:35PM +0200, Bruno Thomsen wrote:
> Add partial support for the watchdog functionality of
> both PCF2127 and PCF2129 chips.
> 
> The programmable watchdog timer is currently using a fixed
> clock source of 1Hz. This result in a selectable range of
> 1-255 seconds, which covers most embedded Linux use-cases.
> 
> Clock sources of 4096Hz, 64Hz and 1/60Hz is mostly useful
> in MCU use-cases.
> 
> Countdown timer not available when using watchdog feature.
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

> ---
> v3: removed 2 x dev_info() and 1 x dev_err() traces.
>     lowered dev_info() to dbg_info() in pcf2127_wdt_set_timeout.
>     removed unneeded ret variable in pcf2127_wdt_set_timeout.
> v2: use new watchdog api, e.g. devm_watchdog_register_device.
>     remove watchdog Kconfig option.
>     update existing Kconfig option with additional information.
> 
>  drivers/rtc/Kconfig       |   7 ++-
>  drivers/rtc/rtc-pcf2127.c | 118 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 124 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e72f65b61176..a3bb58a08879 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -876,7 +876,12 @@ config RTC_DRV_PCF2127
>  	depends on RTC_I2C_AND_SPI
>  	help
>  	  If you say yes here you get support for the NXP PCF2127/29 RTC
> -	  chips.
> +	  chips with integrated quartz crystal for industrial applications.
> +	  Both chips also have watchdog timer and tamper switch detection
> +	  features.
> +
> +	  PCF2127 has an additional feature of 512 bytes battery backed
> +	  memory that's accessible using nvmem interface.
>  
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-pcf2127.
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index ee4921e4a47c..8d6eda455d81 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -5,6 +5,9 @@
>   *
>   * Author: Renaud Cerrato <r.cerrato@til-technologies.fr>
>   *
> + * Watchdog and tamper functions
> + * Author: Bruno Thomsen <bruno.thomsen@gmail.com>
> + *
>   * based on the other drivers in this same directory.
>   *
>   * Datasheet: http://cache.nxp.com/documents/data_sheet/PCF2127.pdf
> @@ -18,6 +21,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/regmap.h>
> +#include <linux/watchdog.h>
>  
>  /* Control register 1 */
>  #define PCF2127_REG_CTRL1		0x00
> @@ -35,6 +39,13 @@
>  #define PCF2127_REG_DW			0x07
>  #define PCF2127_REG_MO			0x08
>  #define PCF2127_REG_YR			0x09
> +/* Watchdog registers */
> +#define PCF2127_REG_WD_CTL		0x10
> +#define PCF2127_BIT_WD_CTL_TF0			BIT(0)
> +#define PCF2127_BIT_WD_CTL_TF1			BIT(1)
> +#define PCF2127_BIT_WD_CTL_CD0			BIT(6)
> +#define PCF2127_BIT_WD_CTL_CD1			BIT(7)
> +#define PCF2127_REG_WD_VAL		0x11
>  /*
>   * RAM registers
>   * PCF2127 has 512 bytes general-purpose static RAM (SRAM) that is
> @@ -45,9 +56,15 @@
>  #define PCF2127_REG_RAM_WRT_CMD		0x1C
>  #define PCF2127_REG_RAM_RD_CMD		0x1D
>  
> +/* Watchdog timer value constants */
> +#define PCF2127_WD_VAL_STOP		0
> +#define PCF2127_WD_VAL_MIN		2
> +#define PCF2127_WD_VAL_MAX		255
> +#define PCF2127_WD_VAL_DEFAULT		60
>  
>  struct pcf2127 {
>  	struct rtc_device *rtc;
> +	struct watchdog_device wdd;
>  	struct regmap *regmap;
>  };
>  
> @@ -220,6 +237,74 @@ static int pcf2127_nvmem_write(void *priv, unsigned int offset,
>  	return ret ?: bytes;
>  }
>  
> +/* watchdog driver */
> +
> +static int pcf2127_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct pcf2127 *pcf2127 = watchdog_get_drvdata(wdd);
> +
> +	return regmap_write(pcf2127->regmap, PCF2127_REG_WD_VAL, wdd->timeout);
> +}
> +
> +/*
> + * Restart watchdog timer if feature is active.
> + *
> + * Note: Reading CTRL2 register causes watchdog to stop which is unfortunate,
> + * since register also contain control/status flags for other features.
> + * Always call this function after reading CTRL2 register.
> + */
> +static int pcf2127_wdt_active_ping(struct watchdog_device *wdd)
> +{
> +	int ret = 0;
> +
> +	if (watchdog_active(wdd)) {
> +		ret = pcf2127_wdt_ping(wdd);
> +		if (ret)
> +			dev_err(wdd->parent,
> +				"%s: watchdog restart failed, ret=%d\n",
> +				__func__, ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static int pcf2127_wdt_start(struct watchdog_device *wdd)
> +{
> +	return pcf2127_wdt_ping(wdd);
> +}
> +
> +static int pcf2127_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct pcf2127 *pcf2127 = watchdog_get_drvdata(wdd);
> +
> +	return regmap_write(pcf2127->regmap, PCF2127_REG_WD_VAL,
> +			    PCF2127_WD_VAL_STOP);
> +}
> +
> +static int pcf2127_wdt_set_timeout(struct watchdog_device *wdd,
> +				   unsigned int new_timeout)
> +{
> +	dev_dbg(wdd->parent, "new watchdog timeout: %is (old: %is)\n",
> +		new_timeout, wdd->timeout);
> +
> +	wdd->timeout = new_timeout;
> +
> +	return pcf2127_wdt_active_ping(wdd);
> +}
> +
> +static const struct watchdog_info pcf2127_wdt_info = {
> +	.identity = "NXP PCF2127/PCF2129 Watchdog",
> +	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +};
> +
> +static const struct watchdog_ops pcf2127_watchdog_ops = {
> +	.owner = THIS_MODULE,
> +	.start = pcf2127_wdt_start,
> +	.stop = pcf2127_wdt_stop,
> +	.ping = pcf2127_wdt_ping,
> +	.set_timeout = pcf2127_wdt_set_timeout,
> +};
> +
>  static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  			const char *name, bool has_nvmem)
>  {
> @@ -242,6 +327,16 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  
>  	pcf2127->rtc->ops = &pcf2127_rtc_ops;
>  
> +	pcf2127->wdd.parent = dev;
> +	pcf2127->wdd.info = &pcf2127_wdt_info;
> +	pcf2127->wdd.ops = &pcf2127_watchdog_ops;
> +	pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
> +	pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
> +	pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
> +	pcf2127->wdd.min_hw_heartbeat_ms = 500;
> +
> +	watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
> +
>  	if (has_nvmem) {
>  		struct nvmem_config nvmem_cfg = {
>  			.priv = pcf2127,
> @@ -253,6 +348,29 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  		ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
>  	}
>  
> +	/*
> +	 * Watchdog timer enabled and reset pin /RST activated when timed out.
> +	 * Select 1Hz clock source for watchdog timer.
> +	 * Timer is not started until WD_VAL is loaded with a valid value.
> +	 * Note: Countdown timer disabled and not available.
> +	 */
> +	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
> +				 PCF2127_BIT_WD_CTL_CD1 |
> +				 PCF2127_BIT_WD_CTL_CD0 |
> +				 PCF2127_BIT_WD_CTL_TF1 |
> +				 PCF2127_BIT_WD_CTL_TF0,
> +				 PCF2127_BIT_WD_CTL_CD1 |
> +				 PCF2127_BIT_WD_CTL_CD0 |
> +				 PCF2127_BIT_WD_CTL_TF1);
> +	if (ret) {
> +		dev_err(dev, "%s: watchdog config (wd_ctl) failed\n", __func__);
> +		return ret;
> +	}
> +
> +	ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
> +	if (ret)
> +		return ret;
> +
>  	return rtc_register_device(pcf2127->rtc);
>  }
>  
> -- 
> 2.21.0
> 

Re: [PATCH v3 4/5] rtc: pcf2127: add watchdog feature support

[Alexandre Belloni]

On 22/08/2019 15:19:35+0200, Bruno Thomsen wrote:
> Add partial support for the watchdog functionality of
> both PCF2127 and PCF2129 chips.
> 
> The programmable watchdog timer is currently using a fixed
> clock source of 1Hz. This result in a selectable range of
> 1-255 seconds, which covers most embedded Linux use-cases.
> 
> Clock sources of 4096Hz, 64Hz and 1/60Hz is mostly useful
> in MCU use-cases.
> 
> Countdown timer not available when using watchdog feature.
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
> ---
> v3: removed 2 x dev_info() and 1 x dev_err() traces.
>     lowered dev_info() to dbg_info() in pcf2127_wdt_set_timeout.
>     removed unneeded ret variable in pcf2127_wdt_set_timeout.
> v2: use new watchdog api, e.g. devm_watchdog_register_device.
>     remove watchdog Kconfig option.
>     update existing Kconfig option with additional information.
> 
>  drivers/rtc/Kconfig       |   7 ++-
>  drivers/rtc/rtc-pcf2127.c | 118 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 124 insertions(+), 1 deletion(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 4/5] rtc: pcf2127: add watchdog feature support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

Hello,

On Thu, Aug 22, 2019 at 03:19:35PM +0200, Bruno Thomsen wrote:
> Add partial support for the watchdog functionality of
> both PCF2127 and PCF2129 chips.

I have a board here with a pcf2127 that has the #RST pin
not connected.

The problem this creates is: The bootloader arms the SoC's watchdog and
jumps into Linux. The pcf2127 driver happens to load first, so watchdog0
is provided by the RTC (but non-functional). Systemd is configured to
feed the watchdog, but happens to feed the wrong one, so the machine
resets shortly after it is up :-|

So I wonder if we need a dt property that tells the driver if the RST
line is connected or not.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 4/5] rtc: pcf2127: add watchdog feature support

[Alexandre Belloni]

Hi,

On 16/07/2020 16:47:05+0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Aug 22, 2019 at 03:19:35PM +0200, Bruno Thomsen wrote:
> > Add partial support for the watchdog functionality of
> > both PCF2127 and PCF2129 chips.
> 
> I have a board here with a pcf2127 that has the #RST pin
> not connected.
> 
> The problem this creates is: The bootloader arms the SoC's watchdog and
> jumps into Linux. The pcf2127 driver happens to load first, so watchdog0
> is provided by the RTC (but non-functional). Systemd is configured to
> feed the watchdog, but happens to feed the wrong one, so the machine
> resets shortly after it is up :-|
> 
> So I wonder if we need a dt property that tells the driver if the RST
> line is connected or not.
> 

I guess the current solution is to set WatchdogDevice to point to a link
that is updated by udev thus ensuring it points to the correct watchdog
device regardless of the probe order.

This would be similar to the /dev/rtc symlink, pointing to the systohc
RTC by default (even if I don't really like that heuristic).

What you suggest is somewhat okay but doesn't really solve the issue if
both watchdogs are functional and systemd still doesn't pick the one
that is armed by the bootloader.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 4/5] rtc: pcf2127: add watchdog feature support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1805 bytes --]

Hi Alexandre,

On Thu, Jul 16, 2020 at 08:18:16PM +0200, Alexandre Belloni wrote:
> On 16/07/2020 16:47:05+0200, Uwe Kleine-König wrote:
> > On Thu, Aug 22, 2019 at 03:19:35PM +0200, Bruno Thomsen wrote:
> > > Add partial support for the watchdog functionality of
> > > both PCF2127 and PCF2129 chips.
> > 
> > I have a board here with a pcf2127 that has the #RST pin
> > not connected.
> > 
> > The problem this creates is: The bootloader arms the SoC's watchdog and
> > jumps into Linux. The pcf2127 driver happens to load first, so watchdog0
> > is provided by the RTC (but non-functional). Systemd is configured to
> > feed the watchdog, but happens to feed the wrong one, so the machine
> > resets shortly after it is up :-|
> > 
> > So I wonder if we need a dt property that tells the driver if the RST
> > line is connected or not.
> 
> I guess the current solution is to set WatchdogDevice to point to a link
> that is updated by udev thus ensuring it points to the correct watchdog
> device regardless of the probe order.
> 
> This would be similar to the /dev/rtc symlink, pointing to the systohc
> RTC by default (even if I don't really like that heuristic).
> 
> What you suggest is somewhat okay but doesn't really solve the issue if
> both watchdogs are functional and systemd still doesn't pick the one
> that is armed by the bootloader.

Yes, my suggestion doesn't solve the problem "Oh, there are two
watchdogs, which should I feed?". But IMHO in this case there shouldn't
be a watchdog device provided at all by an RTC that can be a watchdog
but isn't wired up correctly.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v3 5/5] rtc: pcf2127: add tamper detection support

[Bruno Thomsen]

Add support for integrated tamper detection function in both PCF2127 and
PCF2129 chips. This patch implements the feature by adding an additional
timestamp0 file to sysfs device path. This file contains seconds since
epoch, if an event occurred, or is empty, if none occurred.
Interface should match ISL1208 and RV3028 RTC drivers.

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
v3: no change.
v2: call pcf2127_wdt_active_ping after CTRL2 register read.
    add dev_dbg() trace in timestamp0_show().
    minor regmap dev_err() text update in pcf2127_probe().

 drivers/rtc/rtc-pcf2127.c | 160 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 160 insertions(+)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 8d6eda455d81..3ec87d320766 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -25,11 +25,18 @@
 
 /* Control register 1 */
 #define PCF2127_REG_CTRL1		0x00
+#define PCF2127_BIT_CTRL1_TSF1			BIT(4)
 /* Control register 2 */
 #define PCF2127_REG_CTRL2		0x01
+#define PCF2127_BIT_CTRL2_TSIE			BIT(2)
+#define PCF2127_BIT_CTRL2_TSF2			BIT(5)
 /* Control register 3 */
 #define PCF2127_REG_CTRL3		0x02
+#define PCF2127_BIT_CTRL3_BLIE			BIT(0)
+#define PCF2127_BIT_CTRL3_BIE			BIT(1)
 #define PCF2127_BIT_CTRL3_BLF			BIT(2)
+#define PCF2127_BIT_CTRL3_BF			BIT(3)
+#define PCF2127_BIT_CTRL3_BTSE			BIT(4)
 /* Time and date registers */
 #define PCF2127_REG_SC			0x03
 #define PCF2127_BIT_SC_OSF			BIT(7)
@@ -46,6 +53,16 @@
 #define PCF2127_BIT_WD_CTL_CD0			BIT(6)
 #define PCF2127_BIT_WD_CTL_CD1			BIT(7)
 #define PCF2127_REG_WD_VAL		0x11
+/* Tamper timestamp registers */
+#define PCF2127_REG_TS_CTRL		0x12
+#define PCF2127_BIT_TS_CTRL_TSOFF		BIT(6)
+#define PCF2127_BIT_TS_CTRL_TSM			BIT(7)
+#define PCF2127_REG_TS_SC		0x13
+#define PCF2127_REG_TS_MN		0x14
+#define PCF2127_REG_TS_HR		0x15
+#define PCF2127_REG_TS_DM		0x16
+#define PCF2127_REG_TS_MO		0x17
+#define PCF2127_REG_TS_YR		0x18
 /*
  * RAM registers
  * PCF2127 has 512 bytes general-purpose static RAM (SRAM) that is
@@ -305,6 +322,97 @@ static const struct watchdog_ops pcf2127_watchdog_ops = {
 	.set_timeout = pcf2127_wdt_set_timeout,
 };
 
+/* sysfs interface */
+
+static ssize_t timestamp0_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct pcf2127 *pcf2127 = dev_get_drvdata(dev->parent);
+	int ret;
+
+	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
+				 PCF2127_BIT_CTRL1_TSF1, 0);
+	if (ret) {
+		dev_err(dev, "%s: update ctrl1 ret=%d\n", __func__, ret);
+		return ret;
+	}
+
+	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
+				 PCF2127_BIT_CTRL2_TSF2, 0);
+	if (ret) {
+		dev_err(dev, "%s: update ctrl2 ret=%d\n", __func__, ret);
+		return ret;
+	}
+
+	ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
+	if (ret)
+		return ret;
+
+	return count;
+};
+
+static ssize_t timestamp0_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct pcf2127 *pcf2127 = dev_get_drvdata(dev->parent);
+	struct rtc_time tm;
+	int ret;
+	unsigned char data[25];
+
+	ret = regmap_bulk_read(pcf2127->regmap, PCF2127_REG_CTRL1, data,
+			       sizeof(data));
+	if (ret) {
+		dev_err(dev, "%s: read error ret=%d\n", __func__, ret);
+		return ret;
+	}
+
+	dev_dbg(dev,
+		"%s: raw data is cr1=%02x, cr2=%02x, cr3=%02x, ts_sc=%02x, "
+		"ts_mn=%02x, ts_hr=%02x, ts_dm=%02x, ts_mo=%02x, ts_yr=%02x\n",
+		__func__, data[PCF2127_REG_CTRL1], data[PCF2127_REG_CTRL2],
+		data[PCF2127_REG_CTRL3], data[PCF2127_REG_TS_SC],
+		data[PCF2127_REG_TS_MN], data[PCF2127_REG_TS_HR],
+		data[PCF2127_REG_TS_DM], data[PCF2127_REG_TS_MO],
+		data[PCF2127_REG_TS_YR]);
+
+	ret = pcf2127_wdt_active_ping(&pcf2127->wdd);
+	if (ret)
+		return ret;
+
+	if (!(data[PCF2127_REG_CTRL1] & PCF2127_BIT_CTRL1_TSF1) &&
+	    !(data[PCF2127_REG_CTRL2] & PCF2127_BIT_CTRL2_TSF2))
+		return 0;
+
+	tm.tm_sec = bcd2bin(data[PCF2127_REG_TS_SC] & 0x7F);
+	tm.tm_min = bcd2bin(data[PCF2127_REG_TS_MN] & 0x7F);
+	tm.tm_hour = bcd2bin(data[PCF2127_REG_TS_HR] & 0x3F);
+	tm.tm_mday = bcd2bin(data[PCF2127_REG_TS_DM] & 0x3F);
+	/* TS_MO register (month) value range: 1-12 */
+	tm.tm_mon = bcd2bin(data[PCF2127_REG_TS_MO] & 0x1F) - 1;
+	tm.tm_year = bcd2bin(data[PCF2127_REG_TS_YR]);
+	if (tm.tm_year < 70)
+		tm.tm_year += 100; /* assume we are in 1970...2069 */
+
+	ret = rtc_valid_tm(&tm);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "%llu\n",
+		       (unsigned long long)rtc_tm_to_time64(&tm));
+};
+
+static DEVICE_ATTR_RW(timestamp0);
+
+static struct attribute *pcf2127_attrs[] = {
+	&dev_attr_timestamp0.attr,
+	NULL
+};
+
+static const struct attribute_group pcf2127_attr_group = {
+	.attrs	= pcf2127_attrs,
+};
+
 static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 			const char *name, bool has_nvmem)
 {
@@ -371,6 +479,58 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 	if (ret)
 		return ret;
 
+	/*
+	 * Disable battery low/switch-over timestamp and interrupts.
+	 * Clear battery interrupt flags which can block new trigger events.
+	 * Note: This is the default chip behaviour but added to ensure
+	 * correct tamper timestamp and interrupt function.
+	 */
+	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL3,
+				 PCF2127_BIT_CTRL3_BTSE |
+				 PCF2127_BIT_CTRL3_BF |
+				 PCF2127_BIT_CTRL3_BIE |
+				 PCF2127_BIT_CTRL3_BLIE, 0);
+	if (ret) {
+		dev_err(dev, "%s: interrupt config (ctrl3) failed\n",
+			__func__);
+		return ret;
+	}
+
+	/*
+	 * Enable timestamp function and store timestamp of first trigger
+	 * event until TSF1 and TFS2 interrupt flags are cleared.
+	 */
+	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_TS_CTRL,
+				 PCF2127_BIT_TS_CTRL_TSOFF |
+				 PCF2127_BIT_TS_CTRL_TSM,
+				 PCF2127_BIT_TS_CTRL_TSM);
+	if (ret) {
+		dev_err(dev, "%s: tamper detection config (ts_ctrl) failed\n",
+			__func__);
+		return ret;
+	}
+
+	/*
+	 * Enable interrupt generation when TSF1 or TSF2 timestamp flags
+	 * are set. Interrupt signal is an open-drain output and can be
+	 * left floating if unused.
+	 */
+	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
+				 PCF2127_BIT_CTRL2_TSIE,
+				 PCF2127_BIT_CTRL2_TSIE);
+	if (ret) {
+		dev_err(dev, "%s: tamper detection config (ctrl2) failed\n",
+			__func__);
+		return ret;
+	}
+
+	ret = rtc_add_group(pcf2127->rtc, &pcf2127_attr_group);
+	if (ret) {
+		dev_err(dev, "%s: tamper sysfs registering failed\n",
+			__func__);
+		return ret;
+	}
+
 	return rtc_register_device(pcf2127->rtc);
 }
 
-- 
2.21.0

Re: [PATCH v3 5/5] rtc: pcf2127: add tamper detection support

[Alexandre Belloni]

On 22/08/2019 15:19:36+0200, Bruno Thomsen wrote:
> Add support for integrated tamper detection function in both PCF2127 and
> PCF2129 chips. This patch implements the feature by adding an additional
> timestamp0 file to sysfs device path. This file contains seconds since
> epoch, if an event occurred, or is empty, if none occurred.
> Interface should match ISL1208 and RV3028 RTC drivers.
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
> ---
> v3: no change.
> v2: call pcf2127_wdt_active_ping after CTRL2 register read.
>     add dev_dbg() trace in timestamp0_show().
>     minor regmap dev_err() text update in pcf2127_probe().
> 
>  drivers/rtc/rtc-pcf2127.c | 160 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 160 insertions(+)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 1/5] rtc: pcf2127: convert to devm_rtc_allocate_device

[Alexandre Belloni]

On 22/08/2019 15:19:32+0200, Bruno Thomsen wrote:
> This allows further improvement of the driver.
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
> ---
> v3: no change.
> v2: no change.
> 
>  drivers/rtc/rtc-pcf2127.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com