2022-12-15 16:08:53

by Hugo Villeneuve

[permalink] [raw]
Subject: [PATCH v3 07/14] rtc: pcf2127: add support for PCF2131 RTC

From: Hugo Villeneuve <[email protected]>

This RTC is very similar in functionality to the PCF2127/29.

Basically it:
-supports two new control registers at offsets 4 and 5
-supports a new reset register (not implemented in this driver)
-supports 4 tamper detection functions instead of 1
-has no nvmem (like the PCF2129)
-has two output interrupt pins

Because of that, most of the register addresses are very different,
although they still follow the same layout. For example, the tamper
registers have a different base address, but the offsets are all the same.

Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/rtc/Kconfig | 4 +-
drivers/rtc/rtc-pcf2127.c | 234 ++++++++++++++++++++++++++++++++++----
2 files changed, 215 insertions(+), 23 deletions(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 2bb640d1521d..3d4043ce0057 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -910,9 +910,9 @@ config RTC_DRV_PCF2127
select REGMAP_SPI if SPI_MASTER
select WATCHDOG_CORE if WATCHDOG
help
- If you say yes here you get support for the NXP PCF2127/29 RTC
+ If you say yes here you get support for the NXP PCF2127/29/31 RTC
chips with integrated quartz crystal for industrial applications.
- Both chips also have watchdog timer and tamper switch detection
+ These chips also have watchdog timer and tamper switch detection
features.

PCF2127 has an additional feature of 512 bytes battery backed
diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 3265878edc48..4148e135f935 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -1,16 +1,26 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * An I2C and SPI driver for the NXP PCF2127/29 RTC
+ * An I2C and SPI driver for the NXP PCF2127/29/31 RTC
* Copyright 2013 Til-Technologies
+ * Copyright 2021 DimOnOff
*
* Author: Renaud Cerrato <[email protected]>
*
* Watchdog and tamper functions
* Author: Bruno Thomsen <[email protected]>
*
+ * PCF2131 support
+ * Author: Hugo Villeneuve <[email protected]>
+ *
* based on the other drivers in this same directory.
*
- * Datasheet: https://www.nxp.com/docs/en/data-sheet/PCF2127.pdf
+ * Datasheets: https://www.nxp.com/docs/en/data-sheet/PCF2127.pdf
+ * https://www.nxp.com/docs/en/data-sheet/PCF2131DS.pdf
+ */
+
+/*
+ * The following features are not yet implemented for the PCF2131:
+ * - support for 1/100th seconds
*/

#include <linux/i2c.h>
@@ -43,8 +53,30 @@
#define PCF2127_BIT_CTRL3_BLF BIT(2)
#define PCF2127_BIT_CTRL3_BF BIT(3)
#define PCF2127_BIT_CTRL3_BTSE BIT(4)
+/* Control register 4 */
+#define PCF2131_REG_CTRL4 0x03
+#define PCF2131_BIT_CTRL4_TSF4 BIT(4)
+#define PCF2131_BIT_CTRL4_TSF3 BIT(5)
+#define PCF2131_BIT_CTRL4_TSF2 BIT(6)
+#define PCF2131_BIT_CTRL4_TSF1 BIT(7)
+/* Control register 5 */
+#define PCF2131_REG_CTRL5 0x04
+#define PCF2131_BIT_CTRL5_TSIE4 BIT(4)
+#define PCF2131_BIT_CTRL5_TSIE3 BIT(5)
+#define PCF2131_BIT_CTRL5_TSIE2 BIT(6)
+#define PCF2131_BIT_CTRL5_TSIE1 BIT(7)
+/* Software reset register */
+#define PCF2131_REG_SR_RESET 0x05
+#define PCF2131_SR_RESET_READ_PATTERN 0b00100100 /* Fixed pattern. */
+#define PCF2131_SR_RESET_RESET_CMD 0x2C /* SR is bit 3. */
/* Time and date registers */
#define PCF2127_REG_TIME_DATE_BASE 0x03
+#define PCF2131_REG_TIME_DATE_BASE 0x07 /* Register 0x06 is 100th seconds,
+ * but we do not support it. By
+ * using offset 0x07, we can be
+ * compatible with existing
+ * time/date functions.
+ */
/* Time and date registers offsets (starting from base register) */
#define PCF2127_OFFSET_TD_SC 0
#define PCF2127_OFFSET_TD_MN 1
@@ -57,6 +89,7 @@
#define PCF2127_BIT_SC_OSF BIT(7)
/* Alarm registers */
#define PCF2127_REG_ALARM_BASE 0x0A
+#define PCF2131_REG_ALARM_BASE 0x0E
/* Alarm registers offsets (starting from base register) */
#define PCF2127_OFFSET_ALARM_SC 0
#define PCF2127_OFFSET_ALARM_MN 1
@@ -67,16 +100,26 @@
#define PCF2127_BIT_ALARM_AE BIT(7)
/* CLKOUT control register */
#define PCF2127_REG_CLKOUT 0x0f
+#define PCF2131_REG_CLKOUT 0x13
#define PCF2127_BIT_CLKOUT_OTPR BIT(5)
/* Watchdog registers */
#define PCF2127_REG_WD_CTL 0x10
+#define PCF2131_REG_WD_CTL 0x35
#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
+#define PCF2131_REG_WD_VAL 0x36
/* Tamper timestamp1 registers */
#define PCF2127_REG_TS1_BASE 0x12
+#define PCF2131_REG_TS1_BASE 0x14
+/* Tamper timestamp2 registers */
+#define PCF2131_REG_TS2_BASE 0x1B
+/* Tamper timestamp3 registers */
+#define PCF2131_REG_TS3_BASE 0x22
+/* Tamper timestamp4 registers */
+#define PCF2131_REG_TS4_BASE 0x29
/* Tamper timestamp registers common offsets (starting from base register) */
#define PCF2127_OFFSET_TS_CTL 0
#define PCF2127_OFFSET_TS_SC 1
@@ -92,11 +135,22 @@
* 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.
+ * PCF2129/31 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
+/* Interrupt mask registers */
+#define PCF2131_REG_INT_A_MASK1 0x31
+#define PCF2131_REG_INT_A_MASK2 0x32
+#define PCF2131_REG_INT_B_MASK1 0x33
+#define PCF2131_REG_INT_B_MASK2 0x34
+#define PCF2131_BIT_INT_BLIE BIT(0)
+#define PCF2131_BIT_INT_BIE BIT(1)
+#define PCF2131_BIT_INT_AIE BIT(2)
+#define PCF2131_BIT_INT_WD_CD BIT(3)
+#define PCF2131_BIT_INT_SI BIT(4)
+#define PCF2131_BIT_INT_MI BIT(5)

/* Watchdog timer value constants */
#define PCF2127_WD_VAL_STOP 0
@@ -110,6 +164,14 @@
PCF2127_BIT_CTRL2_AF | \
PCF2127_BIT_CTRL2_WDTF | \
PCF2127_BIT_CTRL2_TSF2)
+#define PCF2131_CTRL2_IRQ_MASK ( \
+ PCF2127_BIT_CTRL2_AF | \
+ PCF2127_BIT_CTRL2_WDTF)
+#define PCF2131_CTRL4_IRQ_MASK ( \
+ PCF2131_BIT_CTRL4_TSF4 | \
+ PCF2131_BIT_CTRL4_TSF3 | \
+ PCF2131_BIT_CTRL4_TSF2 | \
+ PCF2131_BIT_CTRL4_TSF1)

struct pcf21xx_ts_config {
u8 regs_base; /* Base register to read timestamp values. */
@@ -370,7 +432,7 @@ static int pcf2127_wdt_set_timeout(struct watchdog_device *wdd,
}

static const struct watchdog_info pcf2127_wdt_info = {
- .identity = "NXP PCF2127/PCF2129 Watchdog",
+ .identity = "NXP PCF2127/29/31 Watchdog",
.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
};

@@ -555,30 +617,64 @@ static void pcf2127_rtc_ts_snapshot(struct device *dev, int ts_id)
static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
{
struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
- unsigned int ctrl1, ctrl2;
+ unsigned int ctrl2;
int ret = 0;

- ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL1, &ctrl1);
- if (ret)
- return IRQ_NONE;
-
ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
if (ret)
return IRQ_NONE;

- if (!(ctrl1 & PCF2127_CTRL1_IRQ_MASK || ctrl2 & PCF2127_CTRL2_IRQ_MASK))
- return IRQ_NONE;
+ if (pcf2127->cfg->ts_count == 1) {
+ /* PCF2127/29 */
+ unsigned int ctrl1;
+
+ ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL1, &ctrl1);
+ if (ret)
+ return IRQ_NONE;
+
+ if (!(ctrl1 & PCF2127_CTRL1_IRQ_MASK || ctrl2 & PCF2127_CTRL2_IRQ_MASK))
+ return IRQ_NONE;

- if (ctrl1 & PCF2127_BIT_CTRL1_TSF1 || ctrl2 & PCF2127_BIT_CTRL2_TSF2)
- pcf2127_rtc_ts_snapshot(dev, 0);
+ if (ctrl1 & PCF2127_BIT_CTRL1_TSF1 || ctrl2 & PCF2127_BIT_CTRL2_TSF2)
+ pcf2127_rtc_ts_snapshot(dev, 0);

- if (ctrl1 & PCF2127_CTRL1_IRQ_MASK)
- regmap_write(pcf2127->regmap, PCF2127_REG_CTRL1,
- ctrl1 & ~PCF2127_CTRL1_IRQ_MASK);
+ if (ctrl1 & PCF2127_CTRL1_IRQ_MASK)
+ regmap_write(pcf2127->regmap, PCF2127_REG_CTRL1,
+ ctrl1 & ~PCF2127_CTRL1_IRQ_MASK);
+
+ if (ctrl2 & PCF2127_CTRL2_IRQ_MASK)
+ regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
+ ctrl2 & ~PCF2127_CTRL2_IRQ_MASK);
+ } else {
+ /* PCF2131. */
+ unsigned int ctrl4;
+
+ ret = regmap_read(pcf2127->regmap, PCF2131_REG_CTRL4, &ctrl4);
+ if (ret)
+ return IRQ_NONE;

- if (ctrl2 & PCF2127_CTRL2_IRQ_MASK)
- regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
- ctrl2 & ~PCF2127_CTRL2_IRQ_MASK);
+ if (!(ctrl4 & PCF2131_CTRL4_IRQ_MASK || ctrl2 & PCF2131_CTRL2_IRQ_MASK))
+ return IRQ_NONE;
+
+ if (ctrl4 & PCF2131_CTRL4_IRQ_MASK) {
+ int i;
+ int tsf_bit = PCF2131_BIT_CTRL4_TSF1; /* Start at bit 7. */
+
+ for (i = 0; i < pcf2127->cfg->ts_count; i++) {
+ if (ctrl4 & tsf_bit)
+ pcf2127_rtc_ts_snapshot(dev, i);
+
+ tsf_bit = tsf_bit >> 1;
+ }
+
+ regmap_write(pcf2127->regmap, PCF2131_REG_CTRL4,
+ ctrl4 & ~PCF2131_CTRL4_IRQ_MASK);
+ }
+
+ if (ctrl2 & PCF2131_CTRL2_IRQ_MASK)
+ regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
+ ctrl2 & ~PCF2131_CTRL2_IRQ_MASK);
+ }

if (ctrl2 & PCF2127_BIT_CTRL2_AF)
rtc_update_irq(pcf2127->rtc, 1, RTC_IRQF | RTC_AF);
@@ -650,6 +746,27 @@ static ssize_t timestamp0_store(struct device *dev,
return timestamp_store(dev, attr, buf, count, 0);
};

+static ssize_t timestamp1_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ return timestamp_store(dev, attr, buf, count, 1);
+};
+
+static ssize_t timestamp2_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ return timestamp_store(dev, attr, buf, count, 2);
+};
+
+static ssize_t timestamp3_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ return timestamp_store(dev, attr, buf, count, 3);
+};
+
static ssize_t timestamp_show(struct device *dev,
struct device_attribute *attr, char *buf,
int ts_id)
@@ -714,16 +831,46 @@ static ssize_t timestamp0_show(struct device *dev,
return timestamp_show(dev, attr, buf, 0);
};

+static ssize_t timestamp1_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return timestamp_show(dev, attr, buf, 1);
+};
+
+static ssize_t timestamp2_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return timestamp_show(dev, attr, buf, 2);
+};
+
+static ssize_t timestamp3_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return timestamp_show(dev, attr, buf, 3);
+};
+
static DEVICE_ATTR_RW(timestamp0);
+static DEVICE_ATTR_RW(timestamp1);
+static DEVICE_ATTR_RW(timestamp2);
+static DEVICE_ATTR_RW(timestamp3);

static struct attribute *pcf2127_attrs[] = {
&dev_attr_timestamp0.attr,
NULL
};

+static struct attribute *pcf2131_attrs[] = {
+ &dev_attr_timestamp0.attr,
+ &dev_attr_timestamp1.attr,
+ &dev_attr_timestamp2.attr,
+ &dev_attr_timestamp3.attr,
+ NULL
+};
+
enum pcf21xx_type {
PCF2127,
PCF2129,
+ PCF2131,
PCF21XX_LAST_ID
};

@@ -774,6 +921,48 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
.attrs = pcf2127_attrs,
},
},
+ [PCF2131] = {
+ .max_register = 0x36,
+ .has_nvmem = 0,
+ .has_bit_wd_ctl_cd0 = 0,
+ .regs_td_base = PCF2131_REG_TIME_DATE_BASE,
+ .regs_alarm_base = PCF2131_REG_ALARM_BASE,
+ .reg_wd_ctl = PCF2131_REG_WD_CTL,
+ .reg_wd_val = PCF2131_REG_WD_VAL,
+ .reg_clkout = PCF2131_REG_CLKOUT,
+ .ts_count = 4,
+ .ts[0] = {
+ .regs_base = PCF2131_REG_TS1_BASE,
+ .low_reg = PCF2131_REG_CTRL4,
+ .low_bit = PCF2131_BIT_CTRL4_TSF1,
+ .ie_reg = PCF2131_REG_CTRL5,
+ .ie_bit = PCF2131_BIT_CTRL5_TSIE1,
+ },
+ .ts[1] = {
+ .regs_base = PCF2131_REG_TS2_BASE,
+ .low_reg = PCF2131_REG_CTRL4,
+ .low_bit = PCF2131_BIT_CTRL4_TSF2,
+ .ie_reg = PCF2131_REG_CTRL5,
+ .ie_bit = PCF2131_BIT_CTRL5_TSIE2,
+ },
+ .ts[2] = {
+ .regs_base = PCF2131_REG_TS3_BASE,
+ .low_reg = PCF2131_REG_CTRL4,
+ .low_bit = PCF2131_BIT_CTRL4_TSF3,
+ .ie_reg = PCF2131_REG_CTRL5,
+ .ie_bit = PCF2131_BIT_CTRL5_TSIE3,
+ },
+ .ts[3] = {
+ .regs_base = PCF2131_REG_TS4_BASE,
+ .low_reg = PCF2131_REG_CTRL4,
+ .low_bit = PCF2131_BIT_CTRL4_TSF4,
+ .ie_reg = PCF2131_REG_CTRL5,
+ .ie_bit = PCF2131_BIT_CTRL5_TSIE4,
+ },
+ .attribute_group = {
+ .attrs = pcf2131_attrs,
+ },
+ },
};

/*
@@ -922,7 +1111,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
* Watchdog timer enabled and reset pin /RST activated when timed out.
* Select 1Hz clock source for watchdog timer.
* Note: Countdown timer disabled and not available.
- * For pca2129, pcf2129, only bit[7] is for Symbol WD_CD
+ * For pca2129, pcf2129 and pcf2131, only bit[7] is for Symbol WD_CD
* of register watchdg_tim_ctl. The bit[6] is labeled
* as T. Bits labeled as T must always be written with
* logic 0.
@@ -982,6 +1171,7 @@ static const struct of_device_id pcf2127_of_match[] = {
{ .compatible = "nxp,pcf2127", .data = &pcf21xx_cfg[PCF2127] },
{ .compatible = "nxp,pcf2129", .data = &pcf21xx_cfg[PCF2129] },
{ .compatible = "nxp,pca2129", .data = &pcf21xx_cfg[PCF2129] },
+ { .compatible = "nxp,pcf2131", .data = &pcf21xx_cfg[PCF2131] },
{}
};
MODULE_DEVICE_TABLE(of, pcf2127_of_match);
@@ -1069,6 +1259,7 @@ static const struct i2c_device_id pcf2127_i2c_id[] = {
{ "pcf2127", PCF2127 },
{ "pcf2129", PCF2129 },
{ "pca2129", PCF2129 },
+ { "pcf2131", PCF2131 },
{ }
};
MODULE_DEVICE_TABLE(i2c, pcf2127_i2c_id);
@@ -1190,6 +1381,7 @@ static const struct spi_device_id pcf2127_spi_id[] = {
{ "pcf2127", PCF2127 },
{ "pcf2129", PCF2129 },
{ "pca2129", PCF2129 },
+ { "pcf2131", PCF2131 },
{ }
};
MODULE_DEVICE_TABLE(spi, pcf2127_spi_id);
@@ -1254,5 +1446,5 @@ static void __exit pcf2127_exit(void)
module_exit(pcf2127_exit)

MODULE_AUTHOR("Renaud Cerrato <[email protected]>");
-MODULE_DESCRIPTION("NXP PCF2127/29 RTC driver");
+MODULE_DESCRIPTION("NXP PCF2127/29/31 RTC driver");
MODULE_LICENSE("GPL v2");
--
2.30.2


2023-01-07 18:32:32

by Bruno Thomsen

[permalink] [raw]
Subject: Re: [PATCH v3 07/14] rtc: pcf2127: add support for PCF2131 RTC

Den tor. 15. dec. 2022 kl. 16.20 skrev Hugo Villeneuve <[email protected]>:
>
> From: Hugo Villeneuve <[email protected]>
>
> This RTC is very similar in functionality to the PCF2127/29.
>
> Basically it:
> -supports two new control registers at offsets 4 and 5
> -supports a new reset register (not implemented in this driver)
> -supports 4 tamper detection functions instead of 1
> -has no nvmem (like the PCF2129)
> -has two output interrupt pins
>
> Because of that, most of the register addresses are very different,
> although they still follow the same layout. For example, the tamper
> registers have a different base address, but the offsets are all the same.
>
> Signed-off-by: Hugo Villeneuve <[email protected]>
> ---
> drivers/rtc/Kconfig | 4 +-
> drivers/rtc/rtc-pcf2127.c | 234 ++++++++++++++++++++++++++++++++++----
> 2 files changed, 215 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 2bb640d1521d..3d4043ce0057 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -910,9 +910,9 @@ config RTC_DRV_PCF2127
> select REGMAP_SPI if SPI_MASTER
> select WATCHDOG_CORE if WATCHDOG
> help
> - If you say yes here you get support for the NXP PCF2127/29 RTC
> + If you say yes here you get support for the NXP PCF2127/29/31 RTC
> chips with integrated quartz crystal for industrial applications.
> - Both chips also have watchdog timer and tamper switch detection
> + These chips also have watchdog timer and tamper switch detection
> features.
>
> PCF2127 has an additional feature of 512 bytes battery backed
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 3265878edc48..4148e135f935 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -1,16 +1,26 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> - * An I2C and SPI driver for the NXP PCF2127/29 RTC
> + * An I2C and SPI driver for the NXP PCF2127/29/31 RTC
> * Copyright 2013 Til-Technologies
> + * Copyright 2021 DimOnOff
> *
> * Author: Renaud Cerrato <[email protected]>
> *
> * Watchdog and tamper functions
> * Author: Bruno Thomsen <[email protected]>
> *
> + * PCF2131 support
> + * Author: Hugo Villeneuve <[email protected]>
> + *
> * based on the other drivers in this same directory.
> *
> - * Datasheet: https://www.nxp.com/docs/en/data-sheet/PCF2127.pdf
> + * Datasheets: https://www.nxp.com/docs/en/data-sheet/PCF2127.pdf
> + * https://www.nxp.com/docs/en/data-sheet/PCF2131DS.pdf
> + */
> +
> +/*
> + * The following features are not yet implemented for the PCF2131:
> + * - support for 1/100th seconds
> */
>
> #include <linux/i2c.h>
> @@ -43,8 +53,30 @@
> #define PCF2127_BIT_CTRL3_BLF BIT(2)
> #define PCF2127_BIT_CTRL3_BF BIT(3)
> #define PCF2127_BIT_CTRL3_BTSE BIT(4)
> +/* Control register 4 */
> +#define PCF2131_REG_CTRL4 0x03
> +#define PCF2131_BIT_CTRL4_TSF4 BIT(4)
> +#define PCF2131_BIT_CTRL4_TSF3 BIT(5)
> +#define PCF2131_BIT_CTRL4_TSF2 BIT(6)
> +#define PCF2131_BIT_CTRL4_TSF1 BIT(7)
> +/* Control register 5 */
> +#define PCF2131_REG_CTRL5 0x04
> +#define PCF2131_BIT_CTRL5_TSIE4 BIT(4)
> +#define PCF2131_BIT_CTRL5_TSIE3 BIT(5)
> +#define PCF2131_BIT_CTRL5_TSIE2 BIT(6)
> +#define PCF2131_BIT_CTRL5_TSIE1 BIT(7)
> +/* Software reset register */
> +#define PCF2131_REG_SR_RESET 0x05
> +#define PCF2131_SR_RESET_READ_PATTERN 0b00100100 /* Fixed pattern. */

Replace 0b00100100 with (BIT(2) | BIT(5))

> +#define PCF2131_SR_RESET_RESET_CMD 0x2C /* SR is bit 3. */

Replace 0x2C with (BIT(2) | BIT(3) | BIT(5)) or
(PCF2131_SR_RESET_READ_PATTERN | BIT(3))
and then drop the comment.

> /* Time and date registers */
> #define PCF2127_REG_TIME_DATE_BASE 0x03
> +#define PCF2131_REG_TIME_DATE_BASE 0x07 /* Register 0x06 is 100th seconds,
> + * but we do not support it. By
> + * using offset 0x07, we can be
> + * compatible with existing
> + * time/date functions.
> + */

I think you should add this rather big comment above
#define PCF2131_REG_TIME_DATE_BASE 0x07

and rename the define to PCF2131_REG_TIME_BASE.

> /* Time and date registers offsets (starting from base register) */
> #define PCF2127_OFFSET_TD_SC 0
> #define PCF2127_OFFSET_TD_MN 1
> @@ -57,6 +89,7 @@
> #define PCF2127_BIT_SC_OSF BIT(7)
> /* Alarm registers */
> #define PCF2127_REG_ALARM_BASE 0x0A
> +#define PCF2131_REG_ALARM_BASE 0x0E
> /* Alarm registers offsets (starting from base register) */
> #define PCF2127_OFFSET_ALARM_SC 0
> #define PCF2127_OFFSET_ALARM_MN 1
> @@ -67,16 +100,26 @@
> #define PCF2127_BIT_ALARM_AE BIT(7)
> /* CLKOUT control register */
> #define PCF2127_REG_CLKOUT 0x0f
> +#define PCF2131_REG_CLKOUT 0x13
> #define PCF2127_BIT_CLKOUT_OTPR BIT(5)
> /* Watchdog registers */
> #define PCF2127_REG_WD_CTL 0x10
> +#define PCF2131_REG_WD_CTL 0x35
> #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
> +#define PCF2131_REG_WD_VAL 0x36
> /* Tamper timestamp1 registers */
> #define PCF2127_REG_TS1_BASE 0x12
> +#define PCF2131_REG_TS1_BASE 0x14
> +/* Tamper timestamp2 registers */
> +#define PCF2131_REG_TS2_BASE 0x1B
> +/* Tamper timestamp3 registers */
> +#define PCF2131_REG_TS3_BASE 0x22
> +/* Tamper timestamp4 registers */
> +#define PCF2131_REG_TS4_BASE 0x29
> /* Tamper timestamp registers common offsets (starting from base register) */
> #define PCF2127_OFFSET_TS_CTL 0
> #define PCF2127_OFFSET_TS_SC 1
> @@ -92,11 +135,22 @@
> * 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.
> + * PCF2129/31 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
> +/* Interrupt mask registers */
> +#define PCF2131_REG_INT_A_MASK1 0x31
> +#define PCF2131_REG_INT_A_MASK2 0x32
> +#define PCF2131_REG_INT_B_MASK1 0x33
> +#define PCF2131_REG_INT_B_MASK2 0x34
> +#define PCF2131_BIT_INT_BLIE BIT(0)
> +#define PCF2131_BIT_INT_BIE BIT(1)
> +#define PCF2131_BIT_INT_AIE BIT(2)
> +#define PCF2131_BIT_INT_WD_CD BIT(3)
> +#define PCF2131_BIT_INT_SI BIT(4)
> +#define PCF2131_BIT_INT_MI BIT(5)
>
> /* Watchdog timer value constants */
> #define PCF2127_WD_VAL_STOP 0
> @@ -110,6 +164,14 @@
> PCF2127_BIT_CTRL2_AF | \
> PCF2127_BIT_CTRL2_WDTF | \
> PCF2127_BIT_CTRL2_TSF2)
> +#define PCF2131_CTRL2_IRQ_MASK ( \
> + PCF2127_BIT_CTRL2_AF | \
> + PCF2127_BIT_CTRL2_WDTF)
> +#define PCF2131_CTRL4_IRQ_MASK ( \
> + PCF2131_BIT_CTRL4_TSF4 | \
> + PCF2131_BIT_CTRL4_TSF3 | \
> + PCF2131_BIT_CTRL4_TSF2 | \
> + PCF2131_BIT_CTRL4_TSF1)
>
> struct pcf21xx_ts_config {
> u8 regs_base; /* Base register to read timestamp values. */
> @@ -370,7 +432,7 @@ static int pcf2127_wdt_set_timeout(struct watchdog_device *wdd,
> }
>
> static const struct watchdog_info pcf2127_wdt_info = {
> - .identity = "NXP PCF2127/PCF2129 Watchdog",
> + .identity = "NXP PCF2127/29/31 Watchdog",
> .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> };
>
> @@ -555,30 +617,64 @@ static void pcf2127_rtc_ts_snapshot(struct device *dev, int ts_id)
> static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
> {
> struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> - unsigned int ctrl1, ctrl2;
> + unsigned int ctrl2;
> int ret = 0;
>
> - ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL1, &ctrl1);
> - if (ret)
> - return IRQ_NONE;
> -
> ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
> if (ret)
> return IRQ_NONE;
>
> - if (!(ctrl1 & PCF2127_CTRL1_IRQ_MASK || ctrl2 & PCF2127_CTRL2_IRQ_MASK))
> - return IRQ_NONE;
> + if (pcf2127->cfg->ts_count == 1) {
> + /* PCF2127/29 */
> + unsigned int ctrl1;
> +
> + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL1, &ctrl1);
> + if (ret)
> + return IRQ_NONE;
> +
> + if (!(ctrl1 & PCF2127_CTRL1_IRQ_MASK || ctrl2 & PCF2127_CTRL2_IRQ_MASK))
> + return IRQ_NONE;
>
> - if (ctrl1 & PCF2127_BIT_CTRL1_TSF1 || ctrl2 & PCF2127_BIT_CTRL2_TSF2)
> - pcf2127_rtc_ts_snapshot(dev, 0);
> + if (ctrl1 & PCF2127_BIT_CTRL1_TSF1 || ctrl2 & PCF2127_BIT_CTRL2_TSF2)
> + pcf2127_rtc_ts_snapshot(dev, 0);
>
> - if (ctrl1 & PCF2127_CTRL1_IRQ_MASK)
> - regmap_write(pcf2127->regmap, PCF2127_REG_CTRL1,
> - ctrl1 & ~PCF2127_CTRL1_IRQ_MASK);
> + if (ctrl1 & PCF2127_CTRL1_IRQ_MASK)
> + regmap_write(pcf2127->regmap, PCF2127_REG_CTRL1,
> + ctrl1 & ~PCF2127_CTRL1_IRQ_MASK);
> +
> + if (ctrl2 & PCF2127_CTRL2_IRQ_MASK)
> + regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
> + ctrl2 & ~PCF2127_CTRL2_IRQ_MASK);
> + } else {
> + /* PCF2131. */
> + unsigned int ctrl4;
> +
> + ret = regmap_read(pcf2127->regmap, PCF2131_REG_CTRL4, &ctrl4);
> + if (ret)
> + return IRQ_NONE;
>
> - if (ctrl2 & PCF2127_CTRL2_IRQ_MASK)
> - regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
> - ctrl2 & ~PCF2127_CTRL2_IRQ_MASK);
> + if (!(ctrl4 & PCF2131_CTRL4_IRQ_MASK || ctrl2 & PCF2131_CTRL2_IRQ_MASK))
> + return IRQ_NONE;
> +
> + if (ctrl4 & PCF2131_CTRL4_IRQ_MASK) {
> + int i;
> + int tsf_bit = PCF2131_BIT_CTRL4_TSF1; /* Start at bit 7. */
> +
> + for (i = 0; i < pcf2127->cfg->ts_count; i++) {
> + if (ctrl4 & tsf_bit)
> + pcf2127_rtc_ts_snapshot(dev, i);
> +
> + tsf_bit = tsf_bit >> 1;
> + }
> +
> + regmap_write(pcf2127->regmap, PCF2131_REG_CTRL4,
> + ctrl4 & ~PCF2131_CTRL4_IRQ_MASK);
> + }
> +
> + if (ctrl2 & PCF2131_CTRL2_IRQ_MASK)
> + regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
> + ctrl2 & ~PCF2131_CTRL2_IRQ_MASK);
> + }
>
> if (ctrl2 & PCF2127_BIT_CTRL2_AF)
> rtc_update_irq(pcf2127->rtc, 1, RTC_IRQF | RTC_AF);
> @@ -650,6 +746,27 @@ static ssize_t timestamp0_store(struct device *dev,
> return timestamp_store(dev, attr, buf, count, 0);
> };
>
> +static ssize_t timestamp1_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + return timestamp_store(dev, attr, buf, count, 1);
> +};
> +
> +static ssize_t timestamp2_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + return timestamp_store(dev, attr, buf, count, 2);
> +};
> +
> +static ssize_t timestamp3_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + return timestamp_store(dev, attr, buf, count, 3);
> +};
> +
> static ssize_t timestamp_show(struct device *dev,
> struct device_attribute *attr, char *buf,
> int ts_id)
> @@ -714,16 +831,46 @@ static ssize_t timestamp0_show(struct device *dev,
> return timestamp_show(dev, attr, buf, 0);
> };
>
> +static ssize_t timestamp1_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return timestamp_show(dev, attr, buf, 1);
> +};
> +
> +static ssize_t timestamp2_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return timestamp_show(dev, attr, buf, 2);
> +};
> +
> +static ssize_t timestamp3_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return timestamp_show(dev, attr, buf, 3);
> +};
> +
> static DEVICE_ATTR_RW(timestamp0);
> +static DEVICE_ATTR_RW(timestamp1);
> +static DEVICE_ATTR_RW(timestamp2);
> +static DEVICE_ATTR_RW(timestamp3);
>
> static struct attribute *pcf2127_attrs[] = {
> &dev_attr_timestamp0.attr,
> NULL
> };
>
> +static struct attribute *pcf2131_attrs[] = {
> + &dev_attr_timestamp0.attr,
> + &dev_attr_timestamp1.attr,
> + &dev_attr_timestamp2.attr,
> + &dev_attr_timestamp3.attr,
> + NULL
> +};
> +
> enum pcf21xx_type {
> PCF2127,
> PCF2129,
> + PCF2131,
> PCF21XX_LAST_ID
> };
>
> @@ -774,6 +921,48 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> .attrs = pcf2127_attrs,
> },
> },
> + [PCF2131] = {
> + .max_register = 0x36,
> + .has_nvmem = 0,
> + .has_bit_wd_ctl_cd0 = 0,
> + .regs_td_base = PCF2131_REG_TIME_DATE_BASE,
> + .regs_alarm_base = PCF2131_REG_ALARM_BASE,
> + .reg_wd_ctl = PCF2131_REG_WD_CTL,
> + .reg_wd_val = PCF2131_REG_WD_VAL,
> + .reg_clkout = PCF2131_REG_CLKOUT,
> + .ts_count = 4,
> + .ts[0] = {
> + .regs_base = PCF2131_REG_TS1_BASE,
> + .low_reg = PCF2131_REG_CTRL4,
> + .low_bit = PCF2131_BIT_CTRL4_TSF1,

Set .inter_bit to 0 as it's used/read in timestamp_store() to
control flow in if-condition.

> + .ie_reg = PCF2131_REG_CTRL5,
> + .ie_bit = PCF2131_BIT_CTRL5_TSIE1,
> + },
> + .ts[1] = {
> + .regs_base = PCF2131_REG_TS2_BASE,
> + .low_reg = PCF2131_REG_CTRL4,
> + .low_bit = PCF2131_BIT_CTRL4_TSF2,
> + .ie_reg = PCF2131_REG_CTRL5,
> + .ie_bit = PCF2131_BIT_CTRL5_TSIE2,
> + },
> + .ts[2] = {
> + .regs_base = PCF2131_REG_TS3_BASE,
> + .low_reg = PCF2131_REG_CTRL4,
> + .low_bit = PCF2131_BIT_CTRL4_TSF3,
> + .ie_reg = PCF2131_REG_CTRL5,
> + .ie_bit = PCF2131_BIT_CTRL5_TSIE3,
> + },
> + .ts[3] = {
> + .regs_base = PCF2131_REG_TS4_BASE,
> + .low_reg = PCF2131_REG_CTRL4,
> + .low_bit = PCF2131_BIT_CTRL4_TSF4,
> + .ie_reg = PCF2131_REG_CTRL5,
> + .ie_bit = PCF2131_BIT_CTRL5_TSIE4,
> + },
> + .attribute_group = {
> + .attrs = pcf2131_attrs,
> + },
> + },
> };
>
> /*
> @@ -922,7 +1111,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> * Watchdog timer enabled and reset pin /RST activated when timed out.
> * Select 1Hz clock source for watchdog timer.
> * Note: Countdown timer disabled and not available.
> - * For pca2129, pcf2129, only bit[7] is for Symbol WD_CD
> + * For pca2129, pcf2129 and pcf2131, only bit[7] is for Symbol WD_CD
> * of register watchdg_tim_ctl. The bit[6] is labeled
> * as T. Bits labeled as T must always be written with
> * logic 0.
> @@ -982,6 +1171,7 @@ static const struct of_device_id pcf2127_of_match[] = {
> { .compatible = "nxp,pcf2127", .data = &pcf21xx_cfg[PCF2127] },
> { .compatible = "nxp,pcf2129", .data = &pcf21xx_cfg[PCF2129] },
> { .compatible = "nxp,pca2129", .data = &pcf21xx_cfg[PCF2129] },
> + { .compatible = "nxp,pcf2131", .data = &pcf21xx_cfg[PCF2131] },
> {}
> };
> MODULE_DEVICE_TABLE(of, pcf2127_of_match);
> @@ -1069,6 +1259,7 @@ static const struct i2c_device_id pcf2127_i2c_id[] = {
> { "pcf2127", PCF2127 },
> { "pcf2129", PCF2129 },
> { "pca2129", PCF2129 },
> + { "pcf2131", PCF2131 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, pcf2127_i2c_id);
> @@ -1190,6 +1381,7 @@ static const struct spi_device_id pcf2127_spi_id[] = {
> { "pcf2127", PCF2127 },
> { "pcf2129", PCF2129 },
> { "pca2129", PCF2129 },
> + { "pcf2131", PCF2131 },
> { }
> };
> MODULE_DEVICE_TABLE(spi, pcf2127_spi_id);
> @@ -1254,5 +1446,5 @@ static void __exit pcf2127_exit(void)
> module_exit(pcf2127_exit)
>
> MODULE_AUTHOR("Renaud Cerrato <[email protected]>");
> -MODULE_DESCRIPTION("NXP PCF2127/29 RTC driver");
> +MODULE_DESCRIPTION("NXP PCF2127/29/31 RTC driver");
> MODULE_LICENSE("GPL v2");
> --
> 2.30.2
>

2023-01-20 19:21:27

by Alexandre Belloni

[permalink] [raw]
Subject: Re: [PATCH v3 07/14] rtc: pcf2127: add support for PCF2131 RTC

On 15/12/2022 10:02:08-0500, Hugo Villeneuve wrote:
> PCF2127 has an additional feature of 512 bytes battery backed
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 3265878edc48..4148e135f935 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -1,16 +1,26 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> - * An I2C and SPI driver for the NXP PCF2127/29 RTC
> + * An I2C and SPI driver for the NXP PCF2127/29/31 RTC
> * Copyright 2013 Til-Technologies
> + * Copyright 2021 DimOnOff

For the record, I don't really like that because git will be the
authoritative source for the copyright. This will only end up being
outdated info (as it is already)

> *
> * Author: Renaud Cerrato <[email protected]>
> *
> * Watchdog and tamper functions
> * Author: Bruno Thomsen <[email protected]>
> *
> + * PCF2131 support
> + * Author: Hugo Villeneuve <[email protected]>
> + *
> * based on the other drivers in this same directory.
> *
> - * Datasheet: https://www.nxp.com/docs/en/data-sheet/PCF2127.pdf
> + * Datasheets: https://www.nxp.com/docs/en/data-sheet/PCF2127.pdf
> + * https://www.nxp.com/docs/en/data-sheet/PCF2131DS.pdf
> + */
> +
> +/*
> + * The following features are not yet implemented for the PCF2131:
> + * - support for 1/100th seconds

This will never be added so I would remove that comment

> */
>
> #include <linux/i2c.h>
> @@ -43,8 +53,30 @@
> #define PCF2127_BIT_CTRL3_BLF BIT(2)
> #define PCF2127_BIT_CTRL3_BF BIT(3)
> #define PCF2127_BIT_CTRL3_BTSE BIT(4)
> +/* Control register 4 */
> +#define PCF2131_REG_CTRL4 0x03
> +#define PCF2131_BIT_CTRL4_TSF4 BIT(4)
> +#define PCF2131_BIT_CTRL4_TSF3 BIT(5)
> +#define PCF2131_BIT_CTRL4_TSF2 BIT(6)
> +#define PCF2131_BIT_CTRL4_TSF1 BIT(7)
> +/* Control register 5 */
> +#define PCF2131_REG_CTRL5 0x04
> +#define PCF2131_BIT_CTRL5_TSIE4 BIT(4)
> +#define PCF2131_BIT_CTRL5_TSIE3 BIT(5)
> +#define PCF2131_BIT_CTRL5_TSIE2 BIT(6)
> +#define PCF2131_BIT_CTRL5_TSIE1 BIT(7)
> +/* Software reset register */
> +#define PCF2131_REG_SR_RESET 0x05
> +#define PCF2131_SR_RESET_READ_PATTERN 0b00100100 /* Fixed pattern. */
> +#define PCF2131_SR_RESET_RESET_CMD 0x2C /* SR is bit 3. */
> /* Time and date registers */
> #define PCF2127_REG_TIME_DATE_BASE 0x03
> +#define PCF2131_REG_TIME_DATE_BASE 0x07 /* Register 0x06 is 100th seconds,
> + * but we do not support it. By
> + * using offset 0x07, we can be
> + * compatible with existing
> + * time/date functions.
> + */

Because we will never support 100th of seconds, this comment is not
useful

> /* Time and date registers offsets (starting from base register) */
> #define PCF2127_OFFSET_TD_SC 0
> #define PCF2127_OFFSET_TD_MN 1
> @@ -57,6 +89,7 @@
> #define PCF2127_BIT_SC_OSF BIT(7)
> /* Alarm registers */
> #define PCF2127_REG_ALARM_BASE 0x0A
> +#define PCF2131_REG_ALARM_BASE 0x0E

I'd keep the defines ordered by address, so you could move all the
PCF2131 defines after the PCF2127 ones

> /* Alarm registers offsets (starting from base register) */
> #define PCF2127_OFFSET_ALARM_SC 0
> #define PCF2127_OFFSET_ALARM_MN 1
> @@ -67,16 +100,26 @@
> #define PCF2127_BIT_ALARM_AE BIT(7)
> /* CLKOUT control register */
> #define PCF2127_REG_CLKOUT 0x0f
> +#define PCF2131_REG_CLKOUT 0x13
> #define PCF2127_BIT_CLKOUT_OTPR BIT(5)
> /* Watchdog registers */
> #define PCF2127_REG_WD_CTL 0x10
> +#define PCF2131_REG_WD_CTL 0x35
> #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
> +#define PCF2131_REG_WD_VAL 0x36
> /* Tamper timestamp1 registers */
> #define PCF2127_REG_TS1_BASE 0x12
> +#define PCF2131_REG_TS1_BASE 0x14
> +/* Tamper timestamp2 registers */
> +#define PCF2131_REG_TS2_BASE 0x1B
> +/* Tamper timestamp3 registers */
> +#define PCF2131_REG_TS3_BASE 0x22
> +/* Tamper timestamp4 registers */
> +#define PCF2131_REG_TS4_BASE 0x29
> /* Tamper timestamp registers common offsets (starting from base register) */
> #define PCF2127_OFFSET_TS_CTL 0
> #define PCF2127_OFFSET_TS_SC 1
> @@ -92,11 +135,22 @@
> * 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.
> + * PCF2129/31 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
> +/* Interrupt mask registers */
> +#define PCF2131_REG_INT_A_MASK1 0x31
> +#define PCF2131_REG_INT_A_MASK2 0x32
> +#define PCF2131_REG_INT_B_MASK1 0x33
> +#define PCF2131_REG_INT_B_MASK2 0x34
> +#define PCF2131_BIT_INT_BLIE BIT(0)
> +#define PCF2131_BIT_INT_BIE BIT(1)
> +#define PCF2131_BIT_INT_AIE BIT(2)
> +#define PCF2131_BIT_INT_WD_CD BIT(3)
> +#define PCF2131_BIT_INT_SI BIT(4)
> +#define PCF2131_BIT_INT_MI BIT(5)
>
> /* Watchdog timer value constants */
> #define PCF2127_WD_VAL_STOP 0
> @@ -110,6 +164,14 @@
> PCF2127_BIT_CTRL2_AF | \
> PCF2127_BIT_CTRL2_WDTF | \
> PCF2127_BIT_CTRL2_TSF2)
> +#define PCF2131_CTRL2_IRQ_MASK ( \
> + PCF2127_BIT_CTRL2_AF | \
> + PCF2127_BIT_CTRL2_WDTF)
> +#define PCF2131_CTRL4_IRQ_MASK ( \
> + PCF2131_BIT_CTRL4_TSF4 | \
> + PCF2131_BIT_CTRL4_TSF3 | \
> + PCF2131_BIT_CTRL4_TSF2 | \
> + PCF2131_BIT_CTRL4_TSF1)
>
> struct pcf21xx_ts_config {
> u8 regs_base; /* Base register to read timestamp values. */
> @@ -370,7 +432,7 @@ static int pcf2127_wdt_set_timeout(struct watchdog_device *wdd,
> }
>
> static const struct watchdog_info pcf2127_wdt_info = {
> - .identity = "NXP PCF2127/PCF2129 Watchdog",
> + .identity = "NXP PCF2127/29/31 Watchdog",

This change may break userspace tools as this is exposed to userspace


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

2023-01-23 17:27:22

by Hugo Villeneuve

[permalink] [raw]
Subject: Re: [PATCH v3 07/14] rtc: pcf2127: add support for PCF2131 RTC

On Fri, 20 Jan 2023 19:57:07 +0100
Alexandre Belloni <[email protected]> wrote:

> On 15/12/2022 10:02:08-0500, Hugo Villeneuve wrote:
> > PCF2127 has an additional feature of 512 bytes battery backed
> > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > index 3265878edc48..4148e135f935 100644
> > --- a/drivers/rtc/rtc-pcf2127.c
> > +++ b/drivers/rtc/rtc-pcf2127.c
> > @@ -1,16 +1,26 @@
> > // SPDX-License-Identifier: GPL-2.0-only
> > /*
> > - * An I2C and SPI driver for the NXP PCF2127/29 RTC
> > + * An I2C and SPI driver for the NXP PCF2127/29/31 RTC
> > * Copyright 2013 Til-Technologies
> > + * Copyright 2021 DimOnOff
>
> For the record, I don't really like that because git will be the
> authoritative source for the copyright. This will only end up being
> outdated info (as it is already)

Removed.

>
> > *
> > * Author: Renaud Cerrato <[email protected]>
> > *
> > * Watchdog and tamper functions
> > * Author: Bruno Thomsen <[email protected]>
> > *
> > + * PCF2131 support
> > + * Author: Hugo Villeneuve <[email protected]>
> > + *
> > * based on the other drivers in this same directory.
> > *
> > - * Datasheet: https://www.nxp.com/docs/en/data-sheet/PCF2127.pdf
> > + * Datasheets: https://www.nxp.com/docs/en/data-sheet/PCF2127.pdf
> > + * https://www.nxp.com/docs/en/data-sheet/PCF2131DS.pdf
> > + */
> > +
> > +/*
> > + * The following features are not yet implemented for the PCF2131:
> > + * - support for 1/100th seconds
>
> This will never be added so I would remove that comment

Done.

>
> > */
> >
> > #include <linux/i2c.h>
> > @@ -43,8 +53,30 @@
> > #define PCF2127_BIT_CTRL3_BLF BIT(2)
> > #define PCF2127_BIT_CTRL3_BF BIT(3)
> > #define PCF2127_BIT_CTRL3_BTSE BIT(4)
> > +/* Control register 4 */
> > +#define PCF2131_REG_CTRL4 0x03
> > +#define PCF2131_BIT_CTRL4_TSF4 BIT(4)
> > +#define PCF2131_BIT_CTRL4_TSF3 BIT(5)
> > +#define PCF2131_BIT_CTRL4_TSF2 BIT(6)
> > +#define PCF2131_BIT_CTRL4_TSF1 BIT(7)
> > +/* Control register 5 */
> > +#define PCF2131_REG_CTRL5 0x04
> > +#define PCF2131_BIT_CTRL5_TSIE4 BIT(4)
> > +#define PCF2131_BIT_CTRL5_TSIE3 BIT(5)
> > +#define PCF2131_BIT_CTRL5_TSIE2 BIT(6)
> > +#define PCF2131_BIT_CTRL5_TSIE1 BIT(7)
> > +/* Software reset register */
> > +#define PCF2131_REG_SR_RESET 0x05
> > +#define PCF2131_SR_RESET_READ_PATTERN 0b00100100 /* Fixed pattern. */
> > +#define PCF2131_SR_RESET_RESET_CMD 0x2C /* SR is bit 3. */
> > /* Time and date registers */
> > #define PCF2127_REG_TIME_DATE_BASE 0x03
> > +#define PCF2131_REG_TIME_DATE_BASE 0x07 /* Register 0x06 is 100th seconds,
> > + * but we do not support it. By
> > + * using offset 0x07, we can be
> > + * compatible with existing
> > + * time/date functions.
> > + */
>
> Because we will never support 100th of seconds, this comment is not
> useful

Removed.

>
> > /* Time and date registers offsets (starting from base register) */
> > #define PCF2127_OFFSET_TD_SC 0
> > #define PCF2127_OFFSET_TD_MN 1
> > @@ -57,6 +89,7 @@
> > #define PCF2127_BIT_SC_OSF BIT(7)
> > /* Alarm registers */
> > #define PCF2127_REG_ALARM_BASE 0x0A
> > +#define PCF2131_REG_ALARM_BASE 0x0E
>
> I'd keep the defines ordered by address, so you could move all the
> PCF2131 defines after the PCF2127 ones

Done.


>
> > /* Alarm registers offsets (starting from base register) */
> > #define PCF2127_OFFSET_ALARM_SC 0
> > #define PCF2127_OFFSET_ALARM_MN 1
> > @@ -67,16 +100,26 @@
> > #define PCF2127_BIT_ALARM_AE BIT(7)
> > /* CLKOUT control register */
> > #define PCF2127_REG_CLKOUT 0x0f
> > +#define PCF2131_REG_CLKOUT 0x13
> > #define PCF2127_BIT_CLKOUT_OTPR BIT(5)
> > /* Watchdog registers */
> > #define PCF2127_REG_WD_CTL 0x10
> > +#define PCF2131_REG_WD_CTL 0x35
> > #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
> > +#define PCF2131_REG_WD_VAL 0x36
> > /* Tamper timestamp1 registers */
> > #define PCF2127_REG_TS1_BASE 0x12
> > +#define PCF2131_REG_TS1_BASE 0x14
> > +/* Tamper timestamp2 registers */
> > +#define PCF2131_REG_TS2_BASE 0x1B
> > +/* Tamper timestamp3 registers */
> > +#define PCF2131_REG_TS3_BASE 0x22
> > +/* Tamper timestamp4 registers */
> > +#define PCF2131_REG_TS4_BASE 0x29
> > /* Tamper timestamp registers common offsets (starting from base register) */
> > #define PCF2127_OFFSET_TS_CTL 0
> > #define PCF2127_OFFSET_TS_SC 1
> > @@ -92,11 +135,22 @@
> > * 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.
> > + * PCF2129/31 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
> > +/* Interrupt mask registers */
> > +#define PCF2131_REG_INT_A_MASK1 0x31
> > +#define PCF2131_REG_INT_A_MASK2 0x32
> > +#define PCF2131_REG_INT_B_MASK1 0x33
> > +#define PCF2131_REG_INT_B_MASK2 0x34
> > +#define PCF2131_BIT_INT_BLIE BIT(0)
> > +#define PCF2131_BIT_INT_BIE BIT(1)
> > +#define PCF2131_BIT_INT_AIE BIT(2)
> > +#define PCF2131_BIT_INT_WD_CD BIT(3)
> > +#define PCF2131_BIT_INT_SI BIT(4)
> > +#define PCF2131_BIT_INT_MI BIT(5)
> >
> > /* Watchdog timer value constants */
> > #define PCF2127_WD_VAL_STOP 0
> > @@ -110,6 +164,14 @@
> > PCF2127_BIT_CTRL2_AF | \
> > PCF2127_BIT_CTRL2_WDTF | \
> > PCF2127_BIT_CTRL2_TSF2)
> > +#define PCF2131_CTRL2_IRQ_MASK ( \
> > + PCF2127_BIT_CTRL2_AF | \
> > + PCF2127_BIT_CTRL2_WDTF)
> > +#define PCF2131_CTRL4_IRQ_MASK ( \
> > + PCF2131_BIT_CTRL4_TSF4 | \
> > + PCF2131_BIT_CTRL4_TSF3 | \
> > + PCF2131_BIT_CTRL4_TSF2 | \
> > + PCF2131_BIT_CTRL4_TSF1)
> >
> > struct pcf21xx_ts_config {
> > u8 regs_base; /* Base register to read timestamp values. */
> > @@ -370,7 +432,7 @@ static int pcf2127_wdt_set_timeout(struct watchdog_device *wdd,
> > }
> >
> > static const struct watchdog_info pcf2127_wdt_info = {
> > - .identity = "NXP PCF2127/PCF2129 Watchdog",
> > + .identity = "NXP PCF2127/29/31 Watchdog",
>
> This change may break userspace tools as this is exposed to userspace

Ok, removed.


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

2023-01-23 19:07:11

by Hugo Villeneuve

[permalink] [raw]
Subject: Re: [PATCH v3 07/14] rtc: pcf2127: add support for PCF2131 RTC

On Sat, 7 Jan 2023 19:15:00 +0100
Bruno Thomsen <[email protected]> wrote:

> Den tor. 15. dec. 2022 kl. 16.20 skrev Hugo Villeneuve <[email protected]>:
> >
> > From: Hugo Villeneuve <[email protected]>
> >
> > This RTC is very similar in functionality to the PCF2127/29.
> >
> > Basically it:
> > -supports two new control registers at offsets 4 and 5
> > -supports a new reset register (not implemented in this driver)
> > -supports 4 tamper detection functions instead of 1
> > -has no nvmem (like the PCF2129)
> > -has two output interrupt pins
> >
> > Because of that, most of the register addresses are very different,
> > although they still follow the same layout. For example, the tamper
> > registers have a different base address, but the offsets are all the same.
> >
> > Signed-off-by: Hugo Villeneuve <[email protected]>
> > ---
> > drivers/rtc/Kconfig | 4 +-
> > drivers/rtc/rtc-pcf2127.c | 234 ++++++++++++++++++++++++++++++++++----
> > 2 files changed, 215 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > index 2bb640d1521d..3d4043ce0057 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -910,9 +910,9 @@ config RTC_DRV_PCF2127
> > select REGMAP_SPI if SPI_MASTER
> > select WATCHDOG_CORE if WATCHDOG
> > help
> > - If you say yes here you get support for the NXP PCF2127/29 RTC
> > + If you say yes here you get support for the NXP PCF2127/29/31 RTC
> > chips with integrated quartz crystal for industrial applications.
> > - Both chips also have watchdog timer and tamper switch detection
> > + These chips also have watchdog timer and tamper switch detection
> > features.
> >
> > PCF2127 has an additional feature of 512 bytes battery backed
> > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > index 3265878edc48..4148e135f935 100644
> > --- a/drivers/rtc/rtc-pcf2127.c
> > +++ b/drivers/rtc/rtc-pcf2127.c
> > @@ -1,16 +1,26 @@
> > // SPDX-License-Identifier: GPL-2.0-only
> > /*
> > - * An I2C and SPI driver for the NXP PCF2127/29 RTC
> > + * An I2C and SPI driver for the NXP PCF2127/29/31 RTC
> > * Copyright 2013 Til-Technologies
> > + * Copyright 2021 DimOnOff
> > *
> > * Author: Renaud Cerrato <[email protected]>
> > *
> > * Watchdog and tamper functions
> > * Author: Bruno Thomsen <[email protected]>
> > *
> > + * PCF2131 support
> > + * Author: Hugo Villeneuve <[email protected]>
> > + *
> > * based on the other drivers in this same directory.
> > *
> > - * Datasheet: https://www.nxp.com/docs/en/data-sheet/PCF2127.pdf
> > + * Datasheets: https://www.nxp.com/docs/en/data-sheet/PCF2127.pdf
> > + * https://www.nxp.com/docs/en/data-sheet/PCF2131DS.pdf
> > + */
> > +
> > +/*
> > + * The following features are not yet implemented for the PCF2131:
> > + * - support for 1/100th seconds
> > */
> >
> > #include <linux/i2c.h>
> > @@ -43,8 +53,30 @@
> > #define PCF2127_BIT_CTRL3_BLF BIT(2)
> > #define PCF2127_BIT_CTRL3_BF BIT(3)
> > #define PCF2127_BIT_CTRL3_BTSE BIT(4)
> > +/* Control register 4 */
> > +#define PCF2131_REG_CTRL4 0x03
> > +#define PCF2131_BIT_CTRL4_TSF4 BIT(4)
> > +#define PCF2131_BIT_CTRL4_TSF3 BIT(5)
> > +#define PCF2131_BIT_CTRL4_TSF2 BIT(6)
> > +#define PCF2131_BIT_CTRL4_TSF1 BIT(7)
> > +/* Control register 5 */
> > +#define PCF2131_REG_CTRL5 0x04
> > +#define PCF2131_BIT_CTRL5_TSIE4 BIT(4)
> > +#define PCF2131_BIT_CTRL5_TSIE3 BIT(5)
> > +#define PCF2131_BIT_CTRL5_TSIE2 BIT(6)
> > +#define PCF2131_BIT_CTRL5_TSIE1 BIT(7)
> > +/* Software reset register */
> > +#define PCF2131_REG_SR_RESET 0x05
> > +#define PCF2131_SR_RESET_READ_PATTERN 0b00100100 /* Fixed pattern. */
>
> Replace 0b00100100 with (BIT(2) | BIT(5))

Removed this define as the reset signature is no longer verified.


> > +#define PCF2131_SR_RESET_RESET_CMD 0x2C /* SR is bit 3. */
>
> Replace 0x2C with (BIT(2) | BIT(3) | BIT(5)) or
> (PCF2131_SR_RESET_READ_PATTERN | BIT(3))
> and then drop the comment.

Done.


>
> > /* Time and date registers */
> > #define PCF2127_REG_TIME_DATE_BASE 0x03
> > +#define PCF2131_REG_TIME_DATE_BASE 0x07 /* Register 0x06 is 100th seconds,
> > + * but we do not support it. By
> > + * using offset 0x07, we can be
> > + * compatible with existing
> > + * time/date functions.
> > + */
>
> I think you should add this rather big comment above
> #define PCF2131_REG_TIME_DATE_BASE 0x07

Removed comment as suggested by Alexandre.

>
> and rename the define to PCF2131_REG_TIME_BASE.

Done.


>
> > /* Time and date registers offsets (starting from base register) */
> > #define PCF2127_OFFSET_TD_SC 0
> > #define PCF2127_OFFSET_TD_MN 1
> > @@ -57,6 +89,7 @@
> > #define PCF2127_BIT_SC_OSF BIT(7)
> > /* Alarm registers */
> > #define PCF2127_REG_ALARM_BASE 0x0A
> > +#define PCF2131_REG_ALARM_BASE 0x0E
> > /* Alarm registers offsets (starting from base register) */
> > #define PCF2127_OFFSET_ALARM_SC 0
> > #define PCF2127_OFFSET_ALARM_MN 1
> > @@ -67,16 +100,26 @@
> > #define PCF2127_BIT_ALARM_AE BIT(7)
> > /* CLKOUT control register */
> > #define PCF2127_REG_CLKOUT 0x0f
> > +#define PCF2131_REG_CLKOUT 0x13
> > #define PCF2127_BIT_CLKOUT_OTPR BIT(5)
> > /* Watchdog registers */
> > #define PCF2127_REG_WD_CTL 0x10
> > +#define PCF2131_REG_WD_CTL 0x35
> > #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
> > +#define PCF2131_REG_WD_VAL 0x36
> > /* Tamper timestamp1 registers */
> > #define PCF2127_REG_TS1_BASE 0x12
> > +#define PCF2131_REG_TS1_BASE 0x14
> > +/* Tamper timestamp2 registers */
> > +#define PCF2131_REG_TS2_BASE 0x1B
> > +/* Tamper timestamp3 registers */
> > +#define PCF2131_REG_TS3_BASE 0x22
> > +/* Tamper timestamp4 registers */
> > +#define PCF2131_REG_TS4_BASE 0x29
> > /* Tamper timestamp registers common offsets (starting from base register) */
> > #define PCF2127_OFFSET_TS_CTL 0
> > #define PCF2127_OFFSET_TS_SC 1
> > @@ -92,11 +135,22 @@
> > * 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.
> > + * PCF2129/31 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
> > +/* Interrupt mask registers */
> > +#define PCF2131_REG_INT_A_MASK1 0x31
> > +#define PCF2131_REG_INT_A_MASK2 0x32
> > +#define PCF2131_REG_INT_B_MASK1 0x33
> > +#define PCF2131_REG_INT_B_MASK2 0x34
> > +#define PCF2131_BIT_INT_BLIE BIT(0)
> > +#define PCF2131_BIT_INT_BIE BIT(1)
> > +#define PCF2131_BIT_INT_AIE BIT(2)
> > +#define PCF2131_BIT_INT_WD_CD BIT(3)
> > +#define PCF2131_BIT_INT_SI BIT(4)
> > +#define PCF2131_BIT_INT_MI BIT(5)
> >
> > /* Watchdog timer value constants */
> > #define PCF2127_WD_VAL_STOP 0
> > @@ -110,6 +164,14 @@
> > PCF2127_BIT_CTRL2_AF | \
> > PCF2127_BIT_CTRL2_WDTF | \
> > PCF2127_BIT_CTRL2_TSF2)
> > +#define PCF2131_CTRL2_IRQ_MASK ( \
> > + PCF2127_BIT_CTRL2_AF | \
> > + PCF2127_BIT_CTRL2_WDTF)
> > +#define PCF2131_CTRL4_IRQ_MASK ( \
> > + PCF2131_BIT_CTRL4_TSF4 | \
> > + PCF2131_BIT_CTRL4_TSF3 | \
> > + PCF2131_BIT_CTRL4_TSF2 | \
> > + PCF2131_BIT_CTRL4_TSF1)
> >
> > struct pcf21xx_ts_config {
> > u8 regs_base; /* Base register to read timestamp values. */
> > @@ -370,7 +432,7 @@ static int pcf2127_wdt_set_timeout(struct watchdog_device *wdd,
> > }
> >
> > static const struct watchdog_info pcf2127_wdt_info = {
> > - .identity = "NXP PCF2127/PCF2129 Watchdog",
> > + .identity = "NXP PCF2127/29/31 Watchdog",
> > .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> > };
> >
> > @@ -555,30 +617,64 @@ static void pcf2127_rtc_ts_snapshot(struct device *dev, int ts_id)
> > static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
> > {
> > struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> > - unsigned int ctrl1, ctrl2;
> > + unsigned int ctrl2;
> > int ret = 0;
> >
> > - ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL1, &ctrl1);
> > - if (ret)
> > - return IRQ_NONE;
> > -
> > ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
> > if (ret)
> > return IRQ_NONE;
> >
> > - if (!(ctrl1 & PCF2127_CTRL1_IRQ_MASK || ctrl2 & PCF2127_CTRL2_IRQ_MASK))
> > - return IRQ_NONE;
> > + if (pcf2127->cfg->ts_count == 1) {
> > + /* PCF2127/29 */
> > + unsigned int ctrl1;
> > +
> > + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL1, &ctrl1);
> > + if (ret)
> > + return IRQ_NONE;
> > +
> > + if (!(ctrl1 & PCF2127_CTRL1_IRQ_MASK || ctrl2 & PCF2127_CTRL2_IRQ_MASK))
> > + return IRQ_NONE;
> >
> > - if (ctrl1 & PCF2127_BIT_CTRL1_TSF1 || ctrl2 & PCF2127_BIT_CTRL2_TSF2)
> > - pcf2127_rtc_ts_snapshot(dev, 0);
> > + if (ctrl1 & PCF2127_BIT_CTRL1_TSF1 || ctrl2 & PCF2127_BIT_CTRL2_TSF2)
> > + pcf2127_rtc_ts_snapshot(dev, 0);
> >
> > - if (ctrl1 & PCF2127_CTRL1_IRQ_MASK)
> > - regmap_write(pcf2127->regmap, PCF2127_REG_CTRL1,
> > - ctrl1 & ~PCF2127_CTRL1_IRQ_MASK);
> > + if (ctrl1 & PCF2127_CTRL1_IRQ_MASK)
> > + regmap_write(pcf2127->regmap, PCF2127_REG_CTRL1,
> > + ctrl1 & ~PCF2127_CTRL1_IRQ_MASK);
> > +
> > + if (ctrl2 & PCF2127_CTRL2_IRQ_MASK)
> > + regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
> > + ctrl2 & ~PCF2127_CTRL2_IRQ_MASK);
> > + } else {
> > + /* PCF2131. */
> > + unsigned int ctrl4;
> > +
> > + ret = regmap_read(pcf2127->regmap, PCF2131_REG_CTRL4, &ctrl4);
> > + if (ret)
> > + return IRQ_NONE;
> >
> > - if (ctrl2 & PCF2127_CTRL2_IRQ_MASK)
> > - regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
> > - ctrl2 & ~PCF2127_CTRL2_IRQ_MASK);
> > + if (!(ctrl4 & PCF2131_CTRL4_IRQ_MASK || ctrl2 & PCF2131_CTRL2_IRQ_MASK))
> > + return IRQ_NONE;
> > +
> > + if (ctrl4 & PCF2131_CTRL4_IRQ_MASK) {
> > + int i;
> > + int tsf_bit = PCF2131_BIT_CTRL4_TSF1; /* Start at bit 7. */
> > +
> > + for (i = 0; i < pcf2127->cfg->ts_count; i++) {
> > + if (ctrl4 & tsf_bit)
> > + pcf2127_rtc_ts_snapshot(dev, i);
> > +
> > + tsf_bit = tsf_bit >> 1;
> > + }
> > +
> > + regmap_write(pcf2127->regmap, PCF2131_REG_CTRL4,
> > + ctrl4 & ~PCF2131_CTRL4_IRQ_MASK);
> > + }
> > +
> > + if (ctrl2 & PCF2131_CTRL2_IRQ_MASK)
> > + regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
> > + ctrl2 & ~PCF2131_CTRL2_IRQ_MASK);
> > + }
> >
> > if (ctrl2 & PCF2127_BIT_CTRL2_AF)
> > rtc_update_irq(pcf2127->rtc, 1, RTC_IRQF | RTC_AF);
> > @@ -650,6 +746,27 @@ static ssize_t timestamp0_store(struct device *dev,
> > return timestamp_store(dev, attr, buf, count, 0);
> > };
> >
> > +static ssize_t timestamp1_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + return timestamp_store(dev, attr, buf, count, 1);
> > +};
> > +
> > +static ssize_t timestamp2_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + return timestamp_store(dev, attr, buf, count, 2);
> > +};
> > +
> > +static ssize_t timestamp3_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + return timestamp_store(dev, attr, buf, count, 3);
> > +};
> > +
> > static ssize_t timestamp_show(struct device *dev,
> > struct device_attribute *attr, char *buf,
> > int ts_id)
> > @@ -714,16 +831,46 @@ static ssize_t timestamp0_show(struct device *dev,
> > return timestamp_show(dev, attr, buf, 0);
> > };
> >
> > +static ssize_t timestamp1_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + return timestamp_show(dev, attr, buf, 1);
> > +};
> > +
> > +static ssize_t timestamp2_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + return timestamp_show(dev, attr, buf, 2);
> > +};
> > +
> > +static ssize_t timestamp3_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + return timestamp_show(dev, attr, buf, 3);
> > +};
> > +
> > static DEVICE_ATTR_RW(timestamp0);
> > +static DEVICE_ATTR_RW(timestamp1);
> > +static DEVICE_ATTR_RW(timestamp2);
> > +static DEVICE_ATTR_RW(timestamp3);
> >
> > static struct attribute *pcf2127_attrs[] = {
> > &dev_attr_timestamp0.attr,
> > NULL
> > };
> >
> > +static struct attribute *pcf2131_attrs[] = {
> > + &dev_attr_timestamp0.attr,
> > + &dev_attr_timestamp1.attr,
> > + &dev_attr_timestamp2.attr,
> > + &dev_attr_timestamp3.attr,
> > + NULL
> > +};
> > +
> > enum pcf21xx_type {
> > PCF2127,
> > PCF2129,
> > + PCF2131,
> > PCF21XX_LAST_ID
> > };
> >
> > @@ -774,6 +921,48 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> > .attrs = pcf2127_attrs,
> > },
> > },
> > + [PCF2131] = {
> > + .max_register = 0x36,
> > + .has_nvmem = 0,
> > + .has_bit_wd_ctl_cd0 = 0,
> > + .regs_td_base = PCF2131_REG_TIME_DATE_BASE,
> > + .regs_alarm_base = PCF2131_REG_ALARM_BASE,
> > + .reg_wd_ctl = PCF2131_REG_WD_CTL,
> > + .reg_wd_val = PCF2131_REG_WD_VAL,
> > + .reg_clkout = PCF2131_REG_CLKOUT,
> > + .ts_count = 4,
> > + .ts[0] = {
> > + .regs_base = PCF2131_REG_TS1_BASE,
> > + .low_reg = PCF2131_REG_CTRL4,
> > + .low_bit = PCF2131_BIT_CTRL4_TSF1,
>
> Set .inter_bit to 0 as it's used/read in timestamp_store() to
> control flow in if-condition.

Done for all 4 ts[] structures.


>
> > + .ie_reg = PCF2131_REG_CTRL5,
> > + .ie_bit = PCF2131_BIT_CTRL5_TSIE1,
> > + },
> > + .ts[1] = {
> > + .regs_base = PCF2131_REG_TS2_BASE,
> > + .low_reg = PCF2131_REG_CTRL4,
> > + .low_bit = PCF2131_BIT_CTRL4_TSF2,
> > + .ie_reg = PCF2131_REG_CTRL5,
> > + .ie_bit = PCF2131_BIT_CTRL5_TSIE2,
> > + },
> > + .ts[2] = {
> > + .regs_base = PCF2131_REG_TS3_BASE,
> > + .low_reg = PCF2131_REG_CTRL4,
> > + .low_bit = PCF2131_BIT_CTRL4_TSF3,
> > + .ie_reg = PCF2131_REG_CTRL5,
> > + .ie_bit = PCF2131_BIT_CTRL5_TSIE3,
> > + },
> > + .ts[3] = {
> > + .regs_base = PCF2131_REG_TS4_BASE,
> > + .low_reg = PCF2131_REG_CTRL4,
> > + .low_bit = PCF2131_BIT_CTRL4_TSF4,
> > + .ie_reg = PCF2131_REG_CTRL5,
> > + .ie_bit = PCF2131_BIT_CTRL5_TSIE4,
> > + },
> > + .attribute_group = {
> > + .attrs = pcf2131_attrs,
> > + },
> > + },
> > };
> >
> > /*
> > @@ -922,7 +1111,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> > * Watchdog timer enabled and reset pin /RST activated when timed out.
> > * Select 1Hz clock source for watchdog timer.
> > * Note: Countdown timer disabled and not available.
> > - * For pca2129, pcf2129, only bit[7] is for Symbol WD_CD
> > + * For pca2129, pcf2129 and pcf2131, only bit[7] is for Symbol WD_CD
> > * of register watchdg_tim_ctl. The bit[6] is labeled
> > * as T. Bits labeled as T must always be written with
> > * logic 0.
> > @@ -982,6 +1171,7 @@ static const struct of_device_id pcf2127_of_match[] = {
> > { .compatible = "nxp,pcf2127", .data = &pcf21xx_cfg[PCF2127] },
> > { .compatible = "nxp,pcf2129", .data = &pcf21xx_cfg[PCF2129] },
> > { .compatible = "nxp,pca2129", .data = &pcf21xx_cfg[PCF2129] },
> > + { .compatible = "nxp,pcf2131", .data = &pcf21xx_cfg[PCF2131] },
> > {}
> > };
> > MODULE_DEVICE_TABLE(of, pcf2127_of_match);
> > @@ -1069,6 +1259,7 @@ static const struct i2c_device_id pcf2127_i2c_id[] = {
> > { "pcf2127", PCF2127 },
> > { "pcf2129", PCF2129 },
> > { "pca2129", PCF2129 },
> > + { "pcf2131", PCF2131 },
> > { }
> > };
> > MODULE_DEVICE_TABLE(i2c, pcf2127_i2c_id);
> > @@ -1190,6 +1381,7 @@ static const struct spi_device_id pcf2127_spi_id[] = {
> > { "pcf2127", PCF2127 },
> > { "pcf2129", PCF2129 },
> > { "pca2129", PCF2129 },
> > + { "pcf2131", PCF2131 },
> > { }
> > };
> > MODULE_DEVICE_TABLE(spi, pcf2127_spi_id);
> > @@ -1254,5 +1446,5 @@ static void __exit pcf2127_exit(void)
> > module_exit(pcf2127_exit)
> >
> > MODULE_AUTHOR("Renaud Cerrato <[email protected]>");
> > -MODULE_DESCRIPTION("NXP PCF2127/29 RTC driver");
> > +MODULE_DESCRIPTION("NXP PCF2127/29/31 RTC driver");
> > MODULE_LICENSE("GPL v2");
> > --
> > 2.30.2
> >
>


--
Hugo Villeneuve <[email protected]>