2022-12-15 16:22:00

by Hugo Villeneuve

Subject: [PATCH v3 00/14] rtc: pcf2127: add PCF2131 driver

From: Hugo Villeneuve <[email protected]>

Hello,
this patch series adds the driver for the PCF2131 real-time clock.

This RTC is very similar in functionality to the PCF2127/29 with the
following differences:
-supports two new control registers at offsets 4 and 5
-supports a new reset register
-supports 4 tamper detection functions instead of 1
-has no nvmem (like the PCF2129)
-has two output interrupt pins instead of one
-has 1/100th seconds capabilities (not supported in this driver)
-pcf2127 has watchdog clock sources: 1/60, 1, 64 and 4096Hz
pcf2131 has watchdog clock sources: 1/64, 1/4, 4 and 64Hz
-watchdog value register cannot be read after being set

Most of the register addresses are very different, although they still
follow the same layout. For example, the time/date and tamper registers
have a different base address, but the offsets are all the same.
Consequently, the source code of the PCF2127 driver can be easily adapted
to support this new device.

Patches 1 to 6 modify the existing pcf2127 driver to make it more generic
and able to support multiple variants, like the PCF2131. This is done
mostly by using offsets instead of absolute hardcoded register addresses.

Patch 7 add actual support for the PCF2131.

Patch 8 configures all interrupt sources to go through the INT A pin.

Patch 9 changes the PWRMNG bits to be the same with the PCF2131 as they
are with the PCF2127/29 (different default values).

Patch 10 allow to confirm PCF2131 device presence by reading the reset
register fixed pattern.

Patch 11 adapt the time/date registers write sequence for PCF2131 (STOP and
CPR bits).

Patch 12 add support for generic watchdog timing configuration.

Patch 13 add a new flag to identify if device has read support for reading
watchdog register value.
Since the watchdog value register cannot be read on the PCF2131 after
being set, it seems that we cannot detect if watchdog timer was
started by bootloader. I am not sure what is the best way to handle
this situation, suggestions are welcomed.

Patch 14 add the dt-bindings for the PCF2131.

I have tested the driver using a PCF2131-ARD evaluation board connected to
an NXP imx8mp evaluation board:
- Time get/set ok;
- Alarms get/set ok
- Timestamp 1 to 4 ok
- IRQ alarm ok
- Watchdog ok
- Also tested successfully with "RTC Driver Test Example" from
Documentation/rtc.txt

I have also tested the driver on a custom PCF2129 adapter board connected to a
beaglebone black.

Thank you.

Link: [v1] https://patchwork.ozlabs.org/project/rtc-linux/patch/[email protected]/
Link: [v2] https://patchwork.ozlabs.org/project/rtc-linux/list/?series=285734

Changes for V3:
- Rebased for kernel v6.1

Changes for V2:
- In general, fix and improvements after I have tested on real hardware
- Fix alarm interrupt A/B mask setting for PCF2131:
PCF2131_BIT_INT_AIE must be cleared, not set, to enable interrupt.
- Remove low_reg validation: only check if TS interrupt flag is
defined, as low_reg is defined at address 0 for PCF2127/29.
- Change PWRMNG value for PCF2131: default is different than PCF2127/29.
- Adapt time/date registers write sequence for PCF2131 (STOP and CPR bits).
- Map all interrupt sources to INT A pin
- Read and validate PCF2131 device presence from RESET register
- Adapt watchdog configuration for PCF2131

Hugo Villeneuve (14):
rtc: pcf2127: add variant-specific configuration structure
rtc: pcf2127: adapt for time/date registers at any offset
rtc: pcf2127: adapt for alarm registers at any offset
rtc: pcf2127: adapt for WD registers at any offset
rtc: pcf2127: adapt for CLKOUT register at any offset
rtc: pcf2127: add support for multiple TS functions
rtc: pcf2127: add support for PCF2131 RTC
rtc: pcf2127: add support for PCF2131 interrupts on output INT_A
rtc: pcf2127: set PWRMNG value for PCF2131
rtc: pcf2127: read and validate PCF2131 device signature
rtc: pcf2127: adapt time/date registers write sequence for PCF2131
rtc: pcf2127: support generic watchdog timing configuration
rtc: pcf2127: add flag for watchdog register value read support
dt-bindings: rtc: pcf2127: add PCF2131

.../devicetree/bindings/rtc/nxp,pcf2127.yaml | 4 +-
drivers/rtc/Kconfig | 4 +-
drivers/rtc/rtc-pcf2127.c | 939 ++++++++++++++----
3 files changed, 752 insertions(+), 195 deletions(-)

--
2.30.2

2022-12-15 16:22:13

by Hugo Villeneuve

Subject: [PATCH v3 04/14] rtc: pcf2127: adapt for WD registers at any offset

From: Hugo Villeneuve <[email protected]>

This will simplify the implementation of new variants into this driver.

Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/rtc/rtc-pcf2127.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index db0cb784c0c9..5d8c06e32dce 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -114,6 +114,8 @@ struct pcf21xx_config {
unsigned int has_bit_wd_ctl_cd0:1;
u8 regs_td_base; /* Time/data base registers. */
u8 regs_alarm_base; /* Alarm function base registers. */
+ u8 reg_wd_ctl; /* Watchdog control register. */
+ u8 reg_wd_val; /* Watchdog value register. */
};

struct pcf2127 {
@@ -297,7 +299,7 @@ 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);
+ return regmap_write(pcf2127->regmap, pcf2127->cfg->reg_wd_val, wdd->timeout);
}

/*
@@ -331,7 +333,7 @@ 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,
+ return regmap_write(pcf2127->regmap, pcf2127->cfg->reg_wd_val,
PCF2127_WD_VAL_STOP);
}

@@ -380,7 +382,7 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
watchdog_set_drvdata(&pcf2127->wdd, pcf2127);

/* Test if watchdog timer is started by bootloader */
- ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
+ ret = regmap_read(pcf2127->regmap, pcf2127->cfg->reg_wd_val, &wdd_timeout);
if (ret)
return ret;

@@ -664,6 +666,8 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
.has_bit_wd_ctl_cd0 = 1,
.regs_td_base = PCF2127_REG_TIME_DATE_BASE,
.regs_alarm_base = PCF2127_REG_ALARM_BASE,
+ .reg_wd_ctl = PCF2127_REG_WD_CTL,
+ .reg_wd_val = PCF2127_REG_WD_VAL,
},
[PCF2129] = {
.max_register = 0x19,
@@ -671,6 +675,8 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
.has_bit_wd_ctl_cd0 = 0,
.regs_td_base = PCF2127_REG_TIME_DATE_BASE,
.regs_alarm_base = PCF2127_REG_ALARM_BASE,
+ .reg_wd_ctl = PCF2127_REG_WD_CTL,
+ .reg_wd_val = PCF2127_REG_WD_VAL,
},
};

@@ -772,7 +778,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
* as T. Bits labeled as T must always be written with
* logic 0.
*/
- ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
+ ret = regmap_update_bits(pcf2127->regmap, pcf2127->cfg->reg_wd_ctl,
PCF2127_BIT_WD_CTL_CD1 |
PCF2127_BIT_WD_CTL_CD0 |
PCF2127_BIT_WD_CTL_TF1 |
--
2.30.2

2022-12-15 16:23:53

by Hugo Villeneuve

Subject: [PATCH v3 02/14] rtc: pcf2127: adapt for time/date registers at any offset

From: Hugo Villeneuve <[email protected]>

This will simplify the implementation of new variants into this driver.

Some variants (PCF2131) have a 100th seconds register. This register is
currently not supported in this driver.

Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/rtc/rtc-pcf2127.c | 68 ++++++++++++++++++++++-----------------
1 file changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index b9a5d47a439f..fb0caacaabee 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -44,14 +44,17 @@
#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_REG_TIME_DATE_BASE 0x03
+/* Time and date registers offsets (starting from base register) */
+#define PCF2127_OFFSET_TD_SC 0
+#define PCF2127_OFFSET_TD_MN 1
+#define PCF2127_OFFSET_TD_HR 2
+#define PCF2127_OFFSET_TD_DM 3
+#define PCF2127_OFFSET_TD_DW 4
+#define PCF2127_OFFSET_TD_MO 5
+#define PCF2127_OFFSET_TD_YR 6
+/* Time and date registers bits */
#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
/* Alarm registers */
#define PCF2127_REG_ALARM_SC 0x0A
#define PCF2127_REG_ALARM_MN 0x0B
@@ -106,6 +109,7 @@ struct pcf21xx_config {
int max_register;
unsigned int has_nvmem:1;
unsigned int has_bit_wd_ctl_cd0:1;
+ u8 regs_td_base; /* Time/data base registers. */
};

struct pcf2127 {
@@ -125,27 +129,31 @@ struct pcf2127 {
static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
{
struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
- unsigned char buf[10];
+ unsigned char buf[7];
+ unsigned int ctrl3;
int ret;

/*
* 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__);
+ ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &ctrl3);
+ if (ret)
return ret;
- }

- if (buf[PCF2127_REG_CTRL3] & PCF2127_BIT_CTRL3_BLF)
+ if (ctrl3 & PCF2127_BIT_CTRL3_BLF)
dev_info(dev,
"low voltage detected, check/replace RTC battery.\n");

+ ret = regmap_bulk_read(pcf2127->regmap, pcf2127->cfg->regs_td_base,
+ buf, sizeof(buf));
+ if (ret) {
+ dev_err(dev, "%s: read error\n", __func__);
+ return ret;
+ }
+
/* Clock integrity is not guaranteed when OSF flag is set. */
- if (buf[PCF2127_REG_SC] & PCF2127_BIT_SC_OSF) {
+ if (buf[PCF2127_OFFSET_TD_SC] & PCF2127_BIT_SC_OSF) {
/*
* no need clear the flag here,
* it will be cleared once the new date is saved
@@ -158,18 +166,18 @@ static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
dev_dbg(dev,
"%s: raw data is cr3=%02x, sec=%02x, min=%02x, hr=%02x, "
"mday=%02x, wday=%02x, mon=%02x, year=%02x\n",
- __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);
- tm->tm_hour = bcd2bin(buf[PCF2127_REG_HR] & 0x3F); /* rtc hr 0-23 */
- tm->tm_mday = bcd2bin(buf[PCF2127_REG_DM] & 0x3F);
- tm->tm_wday = buf[PCF2127_REG_DW] & 0x07;
- tm->tm_mon = bcd2bin(buf[PCF2127_REG_MO] & 0x1F) - 1; /* rtc mn 1-12 */
- tm->tm_year = bcd2bin(buf[PCF2127_REG_YR]);
+ __func__, ctrl3, buf[PCF2127_OFFSET_TD_SC],
+ buf[PCF2127_OFFSET_TD_MN], buf[PCF2127_OFFSET_TD_HR],
+ buf[PCF2127_OFFSET_TD_DM], buf[PCF2127_OFFSET_TD_DW],
+ buf[PCF2127_OFFSET_TD_MO], buf[PCF2127_OFFSET_TD_YR]);
+
+ tm->tm_sec = bcd2bin(buf[PCF2127_OFFSET_TD_SC] & 0x7F);
+ tm->tm_min = bcd2bin(buf[PCF2127_OFFSET_TD_MN] & 0x7F);
+ tm->tm_hour = bcd2bin(buf[PCF2127_OFFSET_TD_HR] & 0x3F); /* rtc hr 0-23 */
+ tm->tm_mday = bcd2bin(buf[PCF2127_OFFSET_TD_DM] & 0x3F);
+ tm->tm_wday = buf[PCF2127_OFFSET_TD_DW] & 0x07;
+ tm->tm_mon = bcd2bin(buf[PCF2127_OFFSET_TD_MO] & 0x1F) - 1; /* rtc mn 1-12 */
+ tm->tm_year = bcd2bin(buf[PCF2127_OFFSET_TD_YR]);
tm->tm_year += 100;

dev_dbg(dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
@@ -207,7 +215,7 @@ static int pcf2127_rtc_set_time(struct device *dev, struct rtc_time *tm)
buf[i++] = bin2bcd(tm->tm_year - 100);

/* write register's data */
- err = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_SC, buf, i);
+ err = regmap_bulk_write(pcf2127->regmap, pcf2127->cfg->regs_td_base, buf, i);
if (err) {
dev_err(dev,
"%s: err=%d", __func__, err);
@@ -650,11 +658,13 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
.max_register = 0x1d,
.has_nvmem = 1,
.has_bit_wd_ctl_cd0 = 1,
+ .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
},
[PCF2129] = {
.max_register = 0x19,
.has_nvmem = 0,
.has_bit_wd_ctl_cd0 = 0,
+ .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
},
};

--
2.30.2

2022-12-19 10:13:05

by Bruno Thomsen

Subject: Re: [PATCH v3 02/14] rtc: pcf2127: adapt for time/date registers at any offset

Den tor. 15. dec. 2022 kl. 16.20 skrev Hugo Villeneuve <[email protected]>:
>
> From: Hugo Villeneuve <[email protected]>
>
> This will simplify the implementation of new variants into this driver.
>
> Some variants (PCF2131) have a 100th seconds register. This register is
> currently not supported in this driver.
>
> Signed-off-by: Hugo Villeneuve <[email protected]>
> ---
> drivers/rtc/rtc-pcf2127.c | 68 ++++++++++++++++++++++-----------------
> 1 file changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index b9a5d47a439f..fb0caacaabee 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -44,14 +44,17 @@
> #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_REG_TIME_DATE_BASE 0x03
> +/* Time and date registers offsets (starting from base register) */
> +#define PCF2127_OFFSET_TD_SC 0
> +#define PCF2127_OFFSET_TD_MN 1
> +#define PCF2127_OFFSET_TD_HR 2
> +#define PCF2127_OFFSET_TD_DM 3
> +#define PCF2127_OFFSET_TD_DW 4
> +#define PCF2127_OFFSET_TD_MO 5
> +#define PCF2127_OFFSET_TD_YR 6
> +/* Time and date registers bits */
> #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
> /* Alarm registers */
> #define PCF2127_REG_ALARM_SC 0x0A
> #define PCF2127_REG_ALARM_MN 0x0B
> @@ -106,6 +109,7 @@ struct pcf21xx_config {
> int max_register;
> unsigned int has_nvmem:1;
> unsigned int has_bit_wd_ctl_cd0:1;
> + u8 regs_td_base; /* Time/data base registers. */

There is only one base register, so no need to add s.
I think td is an odd short, so maybe u8 reg_time_base.

/Bruno

> };
>
> struct pcf2127 {
> @@ -125,27 +129,31 @@ struct pcf2127 {
> static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
> {
> struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> - unsigned char buf[10];
> + unsigned char buf[7];
> + unsigned int ctrl3;
> int ret;
>
> /*
> * 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__);
> + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &ctrl3);
> + if (ret)
> return ret;
> - }
>
> - if (buf[PCF2127_REG_CTRL3] & PCF2127_BIT_CTRL3_BLF)
> + if (ctrl3 & PCF2127_BIT_CTRL3_BLF)
> dev_info(dev,
> "low voltage detected, check/replace RTC battery.\n");
>
> + ret = regmap_bulk_read(pcf2127->regmap, pcf2127->cfg->regs_td_base,
> + buf, sizeof(buf));
> + if (ret) {
> + dev_err(dev, "%s: read error\n", __func__);
> + return ret;
> + }
> +
> /* Clock integrity is not guaranteed when OSF flag is set. */
> - if (buf[PCF2127_REG_SC] & PCF2127_BIT_SC_OSF) {
> + if (buf[PCF2127_OFFSET_TD_SC] & PCF2127_BIT_SC_OSF) {
> /*
> * no need clear the flag here,
> * it will be cleared once the new date is saved
> @@ -158,18 +166,18 @@ static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
> dev_dbg(dev,
> "%s: raw data is cr3=%02x, sec=%02x, min=%02x, hr=%02x, "
> "mday=%02x, wday=%02x, mon=%02x, year=%02x\n",
> - __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);
> - tm->tm_hour = bcd2bin(buf[PCF2127_REG_HR] & 0x3F); /* rtc hr 0-23 */
> - tm->tm_mday = bcd2bin(buf[PCF2127_REG_DM] & 0x3F);
> - tm->tm_wday = buf[PCF2127_REG_DW] & 0x07;
> - tm->tm_mon = bcd2bin(buf[PCF2127_REG_MO] & 0x1F) - 1; /* rtc mn 1-12 */
> - tm->tm_year = bcd2bin(buf[PCF2127_REG_YR]);
> + __func__, ctrl3, buf[PCF2127_OFFSET_TD_SC],
> + buf[PCF2127_OFFSET_TD_MN], buf[PCF2127_OFFSET_TD_HR],
> + buf[PCF2127_OFFSET_TD_DM], buf[PCF2127_OFFSET_TD_DW],
> + buf[PCF2127_OFFSET_TD_MO], buf[PCF2127_OFFSET_TD_YR]);
> +
> + tm->tm_sec = bcd2bin(buf[PCF2127_OFFSET_TD_SC] & 0x7F);
> + tm->tm_min = bcd2bin(buf[PCF2127_OFFSET_TD_MN] & 0x7F);
> + tm->tm_hour = bcd2bin(buf[PCF2127_OFFSET_TD_HR] & 0x3F); /* rtc hr 0-23 */
> + tm->tm_mday = bcd2bin(buf[PCF2127_OFFSET_TD_DM] & 0x3F);
> + tm->tm_wday = buf[PCF2127_OFFSET_TD_DW] & 0x07;
> + tm->tm_mon = bcd2bin(buf[PCF2127_OFFSET_TD_MO] & 0x1F) - 1; /* rtc mn 1-12 */
> + tm->tm_year = bcd2bin(buf[PCF2127_OFFSET_TD_YR]);
> tm->tm_year += 100;
>
> dev_dbg(dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
> @@ -207,7 +215,7 @@ static int pcf2127_rtc_set_time(struct device *dev, struct rtc_time *tm)
> buf[i++] = bin2bcd(tm->tm_year - 100);
>
> /* write register's data */
> - err = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_SC, buf, i);
> + err = regmap_bulk_write(pcf2127->regmap, pcf2127->cfg->regs_td_base, buf, i);
> if (err) {
> dev_err(dev,
> "%s: err=%d", __func__, err);
> @@ -650,11 +658,13 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> .max_register = 0x1d,
> .has_nvmem = 1,
> .has_bit_wd_ctl_cd0 = 1,
> + .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> },
> [PCF2129] = {
> .max_register = 0x19,
> .has_nvmem = 0,
> .has_bit_wd_ctl_cd0 = 0,
> + .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> },
> };
>
> --
> 2.30.2
>

2022-12-19 16:57:37

by Hugo Villeneuve

Subject: Re: [PATCH v3 02/14] rtc: pcf2127: adapt for time/date registers at any offset

On Mon, 19 Dec 2022 10:34:08 +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 will simplify the implementation of new variants into this driver.
> >
> > Some variants (PCF2131) have a 100th seconds register. This register is
> > currently not supported in this driver.
> >
> > Signed-off-by: Hugo Villeneuve <[email protected]>
> > ---
> > drivers/rtc/rtc-pcf2127.c | 68 ++++++++++++++++++++++-----------------
> > 1 file changed, 39 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > index b9a5d47a439f..fb0caacaabee 100644
> > --- a/drivers/rtc/rtc-pcf2127.c
> > +++ b/drivers/rtc/rtc-pcf2127.c
> > @@ -44,14 +44,17 @@
> > #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_REG_TIME_DATE_BASE 0x03
> > +/* Time and date registers offsets (starting from base register) */
> > +#define PCF2127_OFFSET_TD_SC 0
> > +#define PCF2127_OFFSET_TD_MN 1
> > +#define PCF2127_OFFSET_TD_HR 2
> > +#define PCF2127_OFFSET_TD_DM 3
> > +#define PCF2127_OFFSET_TD_DW 4
> > +#define PCF2127_OFFSET_TD_MO 5
> > +#define PCF2127_OFFSET_TD_YR 6
> > +/* Time and date registers bits */
> > #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
> > /* Alarm registers */
> > #define PCF2127_REG_ALARM_SC 0x0A
> > #define PCF2127_REG_ALARM_MN 0x0B
> > @@ -106,6 +109,7 @@ struct pcf21xx_config {
> > int max_register;
> > unsigned int has_nvmem:1;
> > unsigned int has_bit_wd_ctl_cd0:1;
> > + u8 regs_td_base; /* Time/data base registers. */
>
> There is only one base register, so no need to add s.
> I think td is an odd short, so maybe u8 reg_time_base.
>
> /Bruno

Hi,
it makes sense, I will fix it in V4.

I will also fix the comment "Time/data" -> "Time/date".

Hugo.


> > };
> >
> > struct pcf2127 {
> > @@ -125,27 +129,31 @@ struct pcf2127 {
> > static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > {
> > struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> > - unsigned char buf[10];
> > + unsigned char buf[7];
> > + unsigned int ctrl3;
> > int ret;
> >
> > /*
> > * 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__);
> > + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &ctrl3);
> > + if (ret)
> > return ret;
> > - }
> >
> > - if (buf[PCF2127_REG_CTRL3] & PCF2127_BIT_CTRL3_BLF)
> > + if (ctrl3 & PCF2127_BIT_CTRL3_BLF)
> > dev_info(dev,
> > "low voltage detected, check/replace RTC battery.\n");
> >
> > + ret = regmap_bulk_read(pcf2127->regmap, pcf2127->cfg->regs_td_base,
> > + buf, sizeof(buf));
> > + if (ret) {
> > + dev_err(dev, "%s: read error\n", __func__);
> > + return ret;
> > + }
> > +
> > /* Clock integrity is not guaranteed when OSF flag is set. */
> > - if (buf[PCF2127_REG_SC] & PCF2127_BIT_SC_OSF) {
> > + if (buf[PCF2127_OFFSET_TD_SC] & PCF2127_BIT_SC_OSF) {
> > /*
> > * no need clear the flag here,
> > * it will be cleared once the new date is saved
> > @@ -158,18 +166,18 @@ static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > dev_dbg(dev,
> > "%s: raw data is cr3=%02x, sec=%02x, min=%02x, hr=%02x, "
> > "mday=%02x, wday=%02x, mon=%02x, year=%02x\n",
> > - __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);
> > - tm->tm_hour = bcd2bin(buf[PCF2127_REG_HR] & 0x3F); /* rtc hr 0-23 */
> > - tm->tm_mday = bcd2bin(buf[PCF2127_REG_DM] & 0x3F);
> > - tm->tm_wday = buf[PCF2127_REG_DW] & 0x07;
> > - tm->tm_mon = bcd2bin(buf[PCF2127_REG_MO] & 0x1F) - 1; /* rtc mn 1-12 */
> > - tm->tm_year = bcd2bin(buf[PCF2127_REG_YR]);
> > + __func__, ctrl3, buf[PCF2127_OFFSET_TD_SC],
> > + buf[PCF2127_OFFSET_TD_MN], buf[PCF2127_OFFSET_TD_HR],
> > + buf[PCF2127_OFFSET_TD_DM], buf[PCF2127_OFFSET_TD_DW],
> > + buf[PCF2127_OFFSET_TD_MO], buf[PCF2127_OFFSET_TD_YR]);
> > +
> > + tm->tm_sec = bcd2bin(buf[PCF2127_OFFSET_TD_SC] & 0x7F);
> > + tm->tm_min = bcd2bin(buf[PCF2127_OFFSET_TD_MN] & 0x7F);
> > + tm->tm_hour = bcd2bin(buf[PCF2127_OFFSET_TD_HR] & 0x3F); /* rtc hr 0-23 */
> > + tm->tm_mday = bcd2bin(buf[PCF2127_OFFSET_TD_DM] & 0x3F);
> > + tm->tm_wday = buf[PCF2127_OFFSET_TD_DW] & 0x07;
> > + tm->tm_mon = bcd2bin(buf[PCF2127_OFFSET_TD_MO] & 0x1F) - 1; /* rtc mn 1-12 */
> > + tm->tm_year = bcd2bin(buf[PCF2127_OFFSET_TD_YR]);
> > tm->tm_year += 100;
> >
> > dev_dbg(dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
> > @@ -207,7 +215,7 @@ static int pcf2127_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > buf[i++] = bin2bcd(tm->tm_year - 100);
> >
> > /* write register's data */
> > - err = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_SC, buf, i);
> > + err = regmap_bulk_write(pcf2127->regmap, pcf2127->cfg->regs_td_base, buf, i);
> > if (err) {
> > dev_err(dev,
> > "%s: err=%d", __func__, err);
> > @@ -650,11 +658,13 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> > .max_register = 0x1d,
> > .has_nvmem = 1,
> > .has_bit_wd_ctl_cd0 = 1,
> > + .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> > },
> > [PCF2129] = {
> > .max_register = 0x19,
> > .has_nvmem = 0,
> > .has_bit_wd_ctl_cd0 = 0,
> > + .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> > },
> > };
> >
> > --
> > 2.30.2
> >
>


--
Hugo Villeneuve <[email protected]>

2023-01-07 17:07:24

by Bruno Thomsen

Subject: Re: [PATCH v3 02/14] rtc: pcf2127: adapt for time/date registers at any offset

Den man. 19. dec. 2022 kl. 10.34 skrev Bruno Thomsen <[email protected]>:
>
> Den tor. 15. dec. 2022 kl. 16.20 skrev Hugo Villeneuve <[email protected]>:
> >
> > From: Hugo Villeneuve <[email protected]>
> >
> > This will simplify the implementation of new variants into this driver.
> >
> > Some variants (PCF2131) have a 100th seconds register. This register is
> > currently not supported in this driver.
> >
> > Signed-off-by: Hugo Villeneuve <[email protected]>
> > ---
> > drivers/rtc/rtc-pcf2127.c | 68 ++++++++++++++++++++++-----------------
> > 1 file changed, 39 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > index b9a5d47a439f..fb0caacaabee 100644
> > --- a/drivers/rtc/rtc-pcf2127.c
> > +++ b/drivers/rtc/rtc-pcf2127.c
> > @@ -44,14 +44,17 @@
> > #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_REG_TIME_DATE_BASE 0x03
> > +/* Time and date registers offsets (starting from base register) */
> > +#define PCF2127_OFFSET_TD_SC 0

Maybe do the same change for register defines as the pc721xx_config
struct parameter. This will also match what you did in patch 3 with
alarm registers.

- PCF2127_REG_TIME_DATE_BASE
+ PCF2127_REG_TIME_BASE

- PCF2127_OFFSET_TD_SC
+ PCF2127_OFFSET_TIME_SC

etc.

> > +#define PCF2127_OFFSET_TD_MN 1
> > +#define PCF2127_OFFSET_TD_HR 2
> > +#define PCF2127_OFFSET_TD_DM 3
> > +#define PCF2127_OFFSET_TD_DW 4
> > +#define PCF2127_OFFSET_TD_MO 5
> > +#define PCF2127_OFFSET_TD_YR 6
> > +/* Time and date registers bits */
> > #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
> > /* Alarm registers */
> > #define PCF2127_REG_ALARM_SC 0x0A
> > #define PCF2127_REG_ALARM_MN 0x0B
> > @@ -106,6 +109,7 @@ struct pcf21xx_config {
> > int max_register;
> > unsigned int has_nvmem:1;
> > unsigned int has_bit_wd_ctl_cd0:1;
> > + u8 regs_td_base; /* Time/data base registers. */
>
> There is only one base register, so no need to add s.
> I think td is an odd short, so maybe u8 reg_time_base.
>
> /Bruno
>
> > };
> >
> > struct pcf2127 {
> > @@ -125,27 +129,31 @@ struct pcf2127 {
> > static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > {
> > struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> > - unsigned char buf[10];
> > + unsigned char buf[7];
> > + unsigned int ctrl3;
> > int ret;
> >
> > /*
> > * 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__);
> > + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &ctrl3);
> > + if (ret)
> > return ret;
> > - }
> >
> > - if (buf[PCF2127_REG_CTRL3] & PCF2127_BIT_CTRL3_BLF)
> > + if (ctrl3 & PCF2127_BIT_CTRL3_BLF)
> > dev_info(dev,
> > "low voltage detected, check/replace RTC battery.\n");
> >
> > + ret = regmap_bulk_read(pcf2127->regmap, pcf2127->cfg->regs_td_base,
> > + buf, sizeof(buf));
> > + if (ret) {
> > + dev_err(dev, "%s: read error\n", __func__);
> > + return ret;
> > + }
> > +
> > /* Clock integrity is not guaranteed when OSF flag is set. */
> > - if (buf[PCF2127_REG_SC] & PCF2127_BIT_SC_OSF) {
> > + if (buf[PCF2127_OFFSET_TD_SC] & PCF2127_BIT_SC_OSF) {
> > /*
> > * no need clear the flag here,
> > * it will be cleared once the new date is saved
> > @@ -158,18 +166,18 @@ static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > dev_dbg(dev,
> > "%s: raw data is cr3=%02x, sec=%02x, min=%02x, hr=%02x, "
> > "mday=%02x, wday=%02x, mon=%02x, year=%02x\n",
> > - __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);
> > - tm->tm_hour = bcd2bin(buf[PCF2127_REG_HR] & 0x3F); /* rtc hr 0-23 */
> > - tm->tm_mday = bcd2bin(buf[PCF2127_REG_DM] & 0x3F);
> > - tm->tm_wday = buf[PCF2127_REG_DW] & 0x07;
> > - tm->tm_mon = bcd2bin(buf[PCF2127_REG_MO] & 0x1F) - 1; /* rtc mn 1-12 */
> > - tm->tm_year = bcd2bin(buf[PCF2127_REG_YR]);
> > + __func__, ctrl3, buf[PCF2127_OFFSET_TD_SC],
> > + buf[PCF2127_OFFSET_TD_MN], buf[PCF2127_OFFSET_TD_HR],
> > + buf[PCF2127_OFFSET_TD_DM], buf[PCF2127_OFFSET_TD_DW],
> > + buf[PCF2127_OFFSET_TD_MO], buf[PCF2127_OFFSET_TD_YR]);
> > +
> > + tm->tm_sec = bcd2bin(buf[PCF2127_OFFSET_TD_SC] & 0x7F);
> > + tm->tm_min = bcd2bin(buf[PCF2127_OFFSET_TD_MN] & 0x7F);
> > + tm->tm_hour = bcd2bin(buf[PCF2127_OFFSET_TD_HR] & 0x3F); /* rtc hr 0-23 */
> > + tm->tm_mday = bcd2bin(buf[PCF2127_OFFSET_TD_DM] & 0x3F);
> > + tm->tm_wday = buf[PCF2127_OFFSET_TD_DW] & 0x07;
> > + tm->tm_mon = bcd2bin(buf[PCF2127_OFFSET_TD_MO] & 0x1F) - 1; /* rtc mn 1-12 */
> > + tm->tm_year = bcd2bin(buf[PCF2127_OFFSET_TD_YR]);
> > tm->tm_year += 100;
> >
> > dev_dbg(dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
> > @@ -207,7 +215,7 @@ static int pcf2127_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > buf[i++] = bin2bcd(tm->tm_year - 100);
> >
> > /* write register's data */
> > - err = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_SC, buf, i);
> > + err = regmap_bulk_write(pcf2127->regmap, pcf2127->cfg->regs_td_base, buf, i);
> > if (err) {
> > dev_err(dev,
> > "%s: err=%d", __func__, err);
> > @@ -650,11 +658,13 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> > .max_register = 0x1d,
> > .has_nvmem = 1,
> > .has_bit_wd_ctl_cd0 = 1,
> > + .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> > },
> > [PCF2129] = {
> > .max_register = 0x19,
> > .has_nvmem = 0,
> > .has_bit_wd_ctl_cd0 = 0,
> > + .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> > },
> > };
> >
> > --
> > 2.30.2
> >

2023-01-07 17:17:03

by Bruno Thomsen

Subject: Re: [PATCH v3 04/14] rtc: pcf2127: adapt for WD registers at any offset

Den tor. 15. dec. 2022 kl. 16.19 skrev Hugo Villeneuve <[email protected]>:
>
> From: Hugo Villeneuve <[email protected]>
>
> This will simplify the implementation of new variants into this driver.
>
> Signed-off-by: Hugo Villeneuve <[email protected]>

Reviewed-by: Bruno Thomsen <[email protected]>
Tested-by: Bruno Thomsen <[email protected]>

> ---
> drivers/rtc/rtc-pcf2127.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index db0cb784c0c9..5d8c06e32dce 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -114,6 +114,8 @@ struct pcf21xx_config {
> unsigned int has_bit_wd_ctl_cd0:1;
> u8 regs_td_base; /* Time/data base registers. */
> u8 regs_alarm_base; /* Alarm function base registers. */
> + u8 reg_wd_ctl; /* Watchdog control register. */
> + u8 reg_wd_val; /* Watchdog value register. */
> };
>
> struct pcf2127 {
> @@ -297,7 +299,7 @@ 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);
> + return regmap_write(pcf2127->regmap, pcf2127->cfg->reg_wd_val, wdd->timeout);
> }
>
> /*
> @@ -331,7 +333,7 @@ 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,
> + return regmap_write(pcf2127->regmap, pcf2127->cfg->reg_wd_val,
> PCF2127_WD_VAL_STOP);
> }
>
> @@ -380,7 +382,7 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
> watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
>
> /* Test if watchdog timer is started by bootloader */
> - ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
> + ret = regmap_read(pcf2127->regmap, pcf2127->cfg->reg_wd_val, &wdd_timeout);
> if (ret)
> return ret;
>
> @@ -664,6 +666,8 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> .has_bit_wd_ctl_cd0 = 1,
> .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> .regs_alarm_base = PCF2127_REG_ALARM_BASE,
> + .reg_wd_ctl = PCF2127_REG_WD_CTL,
> + .reg_wd_val = PCF2127_REG_WD_VAL,
> },
> [PCF2129] = {
> .max_register = 0x19,
> @@ -671,6 +675,8 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> .has_bit_wd_ctl_cd0 = 0,
> .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> .regs_alarm_base = PCF2127_REG_ALARM_BASE,
> + .reg_wd_ctl = PCF2127_REG_WD_CTL,
> + .reg_wd_val = PCF2127_REG_WD_VAL,
> },
> };
>
> @@ -772,7 +778,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> * as T. Bits labeled as T must always be written with
> * logic 0.
> */
> - ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
> + ret = regmap_update_bits(pcf2127->regmap, pcf2127->cfg->reg_wd_ctl,
> PCF2127_BIT_WD_CTL_CD1 |
> PCF2127_BIT_WD_CTL_CD0 |
> PCF2127_BIT_WD_CTL_TF1 |
> --
> 2.30.2
>

2023-01-20 20:07:10

by Alexandre Belloni

Subject: Re: [PATCH v3 02/14] rtc: pcf2127: adapt for time/date registers at any offset

On 15/12/2022 10:02:03-0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> This will simplify the implementation of new variants into this driver.
>
> Some variants (PCF2131) have a 100th seconds register. This register is
> currently not supported in this driver.
>
> Signed-off-by: Hugo Villeneuve <[email protected]>
> ---
> drivers/rtc/rtc-pcf2127.c | 68 ++++++++++++++++++++++-----------------
> 1 file changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index b9a5d47a439f..fb0caacaabee 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -44,14 +44,17 @@
> #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_REG_TIME_DATE_BASE 0x03
> +/* Time and date registers offsets (starting from base register) */
> +#define PCF2127_OFFSET_TD_SC 0
> +#define PCF2127_OFFSET_TD_MN 1
> +#define PCF2127_OFFSET_TD_HR 2
> +#define PCF2127_OFFSET_TD_DM 3
> +#define PCF2127_OFFSET_TD_DW 4
> +#define PCF2127_OFFSET_TD_MO 5
> +#define PCF2127_OFFSET_TD_YR 6

Same comment as for the alarms, I would simply remove the defines as
they don't really carry any useful information.

> +/* Time and date registers bits */
> #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
> /* Alarm registers */
> #define PCF2127_REG_ALARM_SC 0x0A
> #define PCF2127_REG_ALARM_MN 0x0B
> @@ -106,6 +109,7 @@ struct pcf21xx_config {
> int max_register;
> unsigned int has_nvmem:1;
> unsigned int has_bit_wd_ctl_cd0:1;
> + u8 regs_td_base; /* Time/data base registers. */
> };
>
> struct pcf2127 {
> @@ -125,27 +129,31 @@ struct pcf2127 {
> static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
> {
> struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> - unsigned char buf[10];
> + unsigned char buf[7];
> + unsigned int ctrl3;
> int ret;
>
> /*
> * 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__);
> + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &ctrl3);
> + if (ret)
> return ret;
> - }
>
> - if (buf[PCF2127_REG_CTRL3] & PCF2127_BIT_CTRL3_BLF)
> + if (ctrl3 & PCF2127_BIT_CTRL3_BLF)
> dev_info(dev,
> "low voltage detected, check/replace RTC battery.\n");
>
> + ret = regmap_bulk_read(pcf2127->regmap, pcf2127->cfg->regs_td_base,
> + buf, sizeof(buf));
> + if (ret) {
> + dev_err(dev, "%s: read error\n", __func__);
> + return ret;
> + }
> +
> /* Clock integrity is not guaranteed when OSF flag is set. */
> - if (buf[PCF2127_REG_SC] & PCF2127_BIT_SC_OSF) {
> + if (buf[PCF2127_OFFSET_TD_SC] & PCF2127_BIT_SC_OSF) {
> /*
> * no need clear the flag here,
> * it will be cleared once the new date is saved
> @@ -158,18 +166,18 @@ static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
> dev_dbg(dev,
> "%s: raw data is cr3=%02x, sec=%02x, min=%02x, hr=%02x, "
> "mday=%02x, wday=%02x, mon=%02x, year=%02x\n",
> - __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);
> - tm->tm_hour = bcd2bin(buf[PCF2127_REG_HR] & 0x3F); /* rtc hr 0-23 */
> - tm->tm_mday = bcd2bin(buf[PCF2127_REG_DM] & 0x3F);
> - tm->tm_wday = buf[PCF2127_REG_DW] & 0x07;
> - tm->tm_mon = bcd2bin(buf[PCF2127_REG_MO] & 0x1F) - 1; /* rtc mn 1-12 */
> - tm->tm_year = bcd2bin(buf[PCF2127_REG_YR]);
> + __func__, ctrl3, buf[PCF2127_OFFSET_TD_SC],
> + buf[PCF2127_OFFSET_TD_MN], buf[PCF2127_OFFSET_TD_HR],
> + buf[PCF2127_OFFSET_TD_DM], buf[PCF2127_OFFSET_TD_DW],
> + buf[PCF2127_OFFSET_TD_MO], buf[PCF2127_OFFSET_TD_YR]);
> +
> + tm->tm_sec = bcd2bin(buf[PCF2127_OFFSET_TD_SC] & 0x7F);
> + tm->tm_min = bcd2bin(buf[PCF2127_OFFSET_TD_MN] & 0x7F);
> + tm->tm_hour = bcd2bin(buf[PCF2127_OFFSET_TD_HR] & 0x3F); /* rtc hr 0-23 */

You can drop the comment

> + tm->tm_mday = bcd2bin(buf[PCF2127_OFFSET_TD_DM] & 0x3F);
> + tm->tm_wday = buf[PCF2127_OFFSET_TD_DW] & 0x07;
> + tm->tm_mon = bcd2bin(buf[PCF2127_OFFSET_TD_MO] & 0x1F) - 1; /* rtc mn 1-12 */

This comment too.

> + tm->tm_year = bcd2bin(buf[PCF2127_OFFSET_TD_YR]);
> tm->tm_year += 100;
>
> dev_dbg(dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
> @@ -207,7 +215,7 @@ static int pcf2127_rtc_set_time(struct device *dev, struct rtc_time *tm)
> buf[i++] = bin2bcd(tm->tm_year - 100);
>
> /* write register's data */
> - err = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_SC, buf, i);
> + err = regmap_bulk_write(pcf2127->regmap, pcf2127->cfg->regs_td_base, buf, i);
> if (err) {
> dev_err(dev,
> "%s: err=%d", __func__, err);
> @@ -650,11 +658,13 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> .max_register = 0x1d,
> .has_nvmem = 1,
> .has_bit_wd_ctl_cd0 = 1,
> + .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> },
> [PCF2129] = {
> .max_register = 0x19,
> .has_nvmem = 0,
> .has_bit_wd_ctl_cd0 = 0,
> + .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> },
> };
>
> --
> 2.30.2
>

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

2023-01-20 20:38:41

by Alexandre Belloni

Subject: Re: [PATCH v3 00/14] rtc: pcf2127: add PCF2131 driver

Hello,

I know I've been holding off on the review of this series for a while
and I'm sorry for that.

One of the main issue that is remaining is that the driver ends up being
53% bigger and generaly less efficient for no added functionality for
the existing RTCs.

I know performance is not a concern however, having more code in the
set/read time and irq paths means that it is more difficult to set an
get the time precisely.

I guess I'll take it as a merged driver but I took a different decision
for other RTCs.

On 15/12/2022 10:02:01-0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> Hello,
> this patch series adds the driver for the PCF2131 real-time clock.
>
> This RTC is very similar in functionality to the PCF2127/29 with the
> following differences:
> -supports two new control registers at offsets 4 and 5
> -supports a new reset register
> -supports 4 tamper detection functions instead of 1
> -has no nvmem (like the PCF2129)
> -has two output interrupt pins instead of one
> -has 1/100th seconds capabilities (not supported in this driver)
> -pcf2127 has watchdog clock sources: 1/60, 1, 64 and 4096Hz
> pcf2131 has watchdog clock sources: 1/64, 1/4, 4 and 64Hz
> -watchdog value register cannot be read after being set
>
> Most of the register addresses are very different, although they still
> follow the same layout. For example, the time/date and tamper registers
> have a different base address, but the offsets are all the same.
> Consequently, the source code of the PCF2127 driver can be easily adapted
> to support this new device.
>
> Patches 1 to 6 modify the existing pcf2127 driver to make it more generic
> and able to support multiple variants, like the PCF2131. This is done
> mostly by using offsets instead of absolute hardcoded register addresses.
>
> Patch 7 add actual support for the PCF2131.
>
> Patch 8 configures all interrupt sources to go through the INT A pin.
>
> Patch 9 changes the PWRMNG bits to be the same with the PCF2131 as they
> are with the PCF2127/29 (different default values).
>
> Patch 10 allow to confirm PCF2131 device presence by reading the reset
> register fixed pattern.
>
> Patch 11 adapt the time/date registers write sequence for PCF2131 (STOP and
> CPR bits).
>
> Patch 12 add support for generic watchdog timing configuration.
>
> Patch 13 add a new flag to identify if device has read support for reading
> watchdog register value.
> Since the watchdog value register cannot be read on the PCF2131 after
> being set, it seems that we cannot detect if watchdog timer was
> started by bootloader. I am not sure what is the best way to handle
> this situation, suggestions are welcomed.
>
> Patch 14 add the dt-bindings for the PCF2131.
>
> I have tested the driver using a PCF2131-ARD evaluation board connected to
> an NXP imx8mp evaluation board:
> - Time get/set ok;
> - Alarms get/set ok
> - Timestamp 1 to 4 ok
> - IRQ alarm ok
> - Watchdog ok
> - Also tested successfully with "RTC Driver Test Example" from
> Documentation/rtc.txt
>
> I have also tested the driver on a custom PCF2129 adapter board connected to a
> beaglebone black.
>
> Thank you.
>
> Link: [v1] https://patchwork.ozlabs.org/project/rtc-linux/patch/[email protected]/
> Link: [v2] https://patchwork.ozlabs.org/project/rtc-linux/list/?series=285734
>
> Changes for V3:
> - Rebased for kernel v6.1
>
> Changes for V2:
> - In general, fix and improvements after I have tested on real hardware
> - Fix alarm interrupt A/B mask setting for PCF2131:
> PCF2131_BIT_INT_AIE must be cleared, not set, to enable interrupt.
> - Remove low_reg validation: only check if TS interrupt flag is
> defined, as low_reg is defined at address 0 for PCF2127/29.
> - Change PWRMNG value for PCF2131: default is different than PCF2127/29.
> - Adapt time/date registers write sequence for PCF2131 (STOP and CPR bits).
> - Map all interrupt sources to INT A pin
> - Read and validate PCF2131 device presence from RESET register
> - Adapt watchdog configuration for PCF2131
>
> Hugo Villeneuve (14):
> rtc: pcf2127: add variant-specific configuration structure
> rtc: pcf2127: adapt for time/date registers at any offset
> rtc: pcf2127: adapt for alarm registers at any offset
> rtc: pcf2127: adapt for WD registers at any offset
> rtc: pcf2127: adapt for CLKOUT register at any offset
> rtc: pcf2127: add support for multiple TS functions
> rtc: pcf2127: add support for PCF2131 RTC
> rtc: pcf2127: add support for PCF2131 interrupts on output INT_A
> rtc: pcf2127: set PWRMNG value for PCF2131
> rtc: pcf2127: read and validate PCF2131 device signature
> rtc: pcf2127: adapt time/date registers write sequence for PCF2131
> rtc: pcf2127: support generic watchdog timing configuration
> rtc: pcf2127: add flag for watchdog register value read support
> dt-bindings: rtc: pcf2127: add PCF2131
>
> .../devicetree/bindings/rtc/nxp,pcf2127.yaml | 4 +-
> drivers/rtc/Kconfig | 4 +-
> drivers/rtc/rtc-pcf2127.c | 939 ++++++++++++++----
> 3 files changed, 752 insertions(+), 195 deletions(-)
>
> --
> 2.30.2
>

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

2023-01-23 15:51:19

by Hugo Villeneuve

Subject: Re: [PATCH v3 00/14] rtc: pcf2127: add PCF2131 driver

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

> Hello,
>
> I know I've been holding off on the review of this series for a while
> and I'm sorry for that.
>
> One of the main issue that is remaining is that the driver ends up being
> 53% bigger and generaly less efficient for no added functionality for
> the existing RTCs.

Hi Alexandre,
that is why before submitting my driver I sent an RFC on the RTC mailing list to ask what was the better approach for developping this driver (separate or merged into pcf2127), but I didn't got an answer from any of the maintainers.

> I know performance is not a concern however, having more code in the
> set/read time and irq paths means that it is more difficult to set an
> get the time precisely.

Just some ideas about that...

Looking at pcf2127_rtc_read_time(), we now do a separate read operation of CTRL3 to check for a low voltage condition. And we only display a message if the battery is low (no abort of read time). Looking at pcf8523 driver for example, this check is done only when responding to an ioctl. Could we do the same in our driver?

Another scheme I say is in rtc-ab-b5ze-s3 driver where this detection is done at startup and using the BLF interrupt flag...

> I guess I'll take it as a merged driver but I took a different decision
> for other RTCs.

I'll address all the comments/issues Bruno and you found and submit a V4 soon then.

Thank you, Hugo.


> On 15/12/2022 10:02:01-0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <[email protected]>
> >
> > Hello,
> > this patch series adds the driver for the PCF2131 real-time clock.
> >
> > This RTC is very similar in functionality to the PCF2127/29 with the
> > following differences:
> > -supports two new control registers at offsets 4 and 5
> > -supports a new reset register
> > -supports 4 tamper detection functions instead of 1
> > -has no nvmem (like the PCF2129)
> > -has two output interrupt pins instead of one
> > -has 1/100th seconds capabilities (not supported in this driver)
> > -pcf2127 has watchdog clock sources: 1/60, 1, 64 and 4096Hz
> > pcf2131 has watchdog clock sources: 1/64, 1/4, 4 and 64Hz
> > -watchdog value register cannot be read after being set
> >
> > Most of the register addresses are very different, although they still
> > follow the same layout. For example, the time/date and tamper registers
> > have a different base address, but the offsets are all the same.
> > Consequently, the source code of the PCF2127 driver can be easily adapted
> > to support this new device.
> >
> > Patches 1 to 6 modify the existing pcf2127 driver to make it more generic
> > and able to support multiple variants, like the PCF2131. This is done
> > mostly by using offsets instead of absolute hardcoded register addresses.
> >
> > Patch 7 add actual support for the PCF2131.
> >
> > Patch 8 configures all interrupt sources to go through the INT A pin.
> >
> > Patch 9 changes the PWRMNG bits to be the same with the PCF2131 as they
> > are with the PCF2127/29 (different default values).
> >
> > Patch 10 allow to confirm PCF2131 device presence by reading the reset
> > register fixed pattern.
> >
> > Patch 11 adapt the time/date registers write sequence for PCF2131 (STOP and
> > CPR bits).
> >
> > Patch 12 add support for generic watchdog timing configuration.
> >
> > Patch 13 add a new flag to identify if device has read support for reading
> > watchdog register value.
> > Since the watchdog value register cannot be read on the PCF2131 after
> > being set, it seems that we cannot detect if watchdog timer was
> > started by bootloader. I am not sure what is the best way to handle
> > this situation, suggestions are welcomed.
> >
> > Patch 14 add the dt-bindings for the PCF2131.
> >
> > I have tested the driver using a PCF2131-ARD evaluation board connected to
> > an NXP imx8mp evaluation board:
> > - Time get/set ok;
> > - Alarms get/set ok
> > - Timestamp 1 to 4 ok
> > - IRQ alarm ok
> > - Watchdog ok
> > - Also tested successfully with "RTC Driver Test Example" from
> > Documentation/rtc.txt
> >
> > I have also tested the driver on a custom PCF2129 adapter board connected to a
> > beaglebone black.
> >
> > Thank you.
> >
> > Link: [v1] https://patchwork.ozlabs.org/project/rtc-linux/patch/[email protected]/
> > Link: [v2] https://patchwork.ozlabs.org/project/rtc-linux/list/?series=285734
> >
> > Changes for V3:
> > - Rebased for kernel v6.1
> >
> > Changes for V2:
> > - In general, fix and improvements after I have tested on real hardware
> > - Fix alarm interrupt A/B mask setting for PCF2131:
> > PCF2131_BIT_INT_AIE must be cleared, not set, to enable interrupt.
> > - Remove low_reg validation: only check if TS interrupt flag is
> > defined, as low_reg is defined at address 0 for PCF2127/29.
> > - Change PWRMNG value for PCF2131: default is different than PCF2127/29.
> > - Adapt time/date registers write sequence for PCF2131 (STOP and CPR bits).
> > - Map all interrupt sources to INT A pin
> > - Read and validate PCF2131 device presence from RESET register
> > - Adapt watchdog configuration for PCF2131
> >
> > Hugo Villeneuve (14):
> > rtc: pcf2127: add variant-specific configuration structure
> > rtc: pcf2127: adapt for time/date registers at any offset
> > rtc: pcf2127: adapt for alarm registers at any offset
> > rtc: pcf2127: adapt for WD registers at any offset
> > rtc: pcf2127: adapt for CLKOUT register at any offset
> > rtc: pcf2127: add support for multiple TS functions
> > rtc: pcf2127: add support for PCF2131 RTC
> > rtc: pcf2127: add support for PCF2131 interrupts on output INT_A
> > rtc: pcf2127: set PWRMNG value for PCF2131
> > rtc: pcf2127: read and validate PCF2131 device signature
> > rtc: pcf2127: adapt time/date registers write sequence for PCF2131
> > rtc: pcf2127: support generic watchdog timing configuration
> > rtc: pcf2127: add flag for watchdog register value read support
> > dt-bindings: rtc: pcf2127: add PCF2131
> >
> > .../devicetree/bindings/rtc/nxp,pcf2127.yaml | 4 +-
> > drivers/rtc/Kconfig | 4 +-
> > drivers/rtc/rtc-pcf2127.c | 939 ++++++++++++++----
> > 3 files changed, 752 insertions(+), 195 deletions(-)
> >
> > --
> > 2.30.2
> >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>


--
Hugo Villeneuve <[email protected]>

2023-01-23 15:55:06

by Hugo Villeneuve

Subject: Re: [PATCH v3 02/14] rtc: pcf2127: adapt for time/date registers at any offset

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

> On 15/12/2022 10:02:03-0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <[email protected]>
> >
> > This will simplify the implementation of new variants into this driver.
> >
> > Some variants (PCF2131) have a 100th seconds register. This register is
> > currently not supported in this driver.
> >
> > Signed-off-by: Hugo Villeneuve <[email protected]>
> > ---
> > drivers/rtc/rtc-pcf2127.c | 68 ++++++++++++++++++++++-----------------
> > 1 file changed, 39 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > index b9a5d47a439f..fb0caacaabee 100644
> > --- a/drivers/rtc/rtc-pcf2127.c
> > +++ b/drivers/rtc/rtc-pcf2127.c
> > @@ -44,14 +44,17 @@
> > #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_REG_TIME_DATE_BASE 0x03
> > +/* Time and date registers offsets (starting from base register) */
> > +#define PCF2127_OFFSET_TD_SC 0
> > +#define PCF2127_OFFSET_TD_MN 1
> > +#define PCF2127_OFFSET_TD_HR 2
> > +#define PCF2127_OFFSET_TD_DM 3
> > +#define PCF2127_OFFSET_TD_DW 4
> > +#define PCF2127_OFFSET_TD_MO 5
> > +#define PCF2127_OFFSET_TD_YR 6
>
> Same comment as for the alarms, I would simply remove the defines as
> they don't really carry any useful information.

Note that if I remove them, the patch for pcf2127_rtc_read_time() would look like this:

/* Clock integrity is not guaranteed when OSF flag is set. */
- if (buf[PCF2127_REG_SC] & PCF2127_BIT_SC_OSF) {
+ if (buf[0] & PCF2127_BIT_SC_OSF) {
...
- __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);
- tm->tm_hour = bcd2bin(buf[PCF2127_REG_HR] & 0x3F); /* rtc hr 0-23 */
- tm->tm_mday = bcd2bin(buf[PCF2127_REG_DM] & 0x3F);
- tm->tm_wday = buf[PCF2127_REG_DW] & 0x07;
- tm->tm_mon = bcd2bin(buf[PCF2127_REG_MO] & 0x1F) - 1; /* rtc mn 1-12 */
- tm->tm_year = bcd2bin(buf[PCF2127_REG_YR]);
+ __func__, ctrl3, buf[0],
+ buf[1], buf[2],
+ buf[3], buf[4],
+ buf[5], buf[PCF2127_OFFSET_TD_YR]);
+
+ tm->tm_sec = bcd2bin(buf[0] & 0x7F);
+ tm->tm_min = bcd2bin(buf[1] & 0x7F);
+ tm->tm_hour = bcd2bin(buf[2] & 0x3F); /* rtc hr 0-23 */
+ tm->tm_mday = bcd2bin(buf[3] & 0x3F);
+ tm->tm_wday = buf[4] & 0x07;
+ tm->tm_mon = bcd2bin(buf[5] & 0x1F) - 1; /* rtc mn 1-12 */
+ tm->tm_year = bcd2bin(buf[6]);

Do you still want to remove the defines then?


> > +/* Time and date registers bits */
> > #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
> > /* Alarm registers */
> > #define PCF2127_REG_ALARM_SC 0x0A
> > #define PCF2127_REG_ALARM_MN 0x0B
> > @@ -106,6 +109,7 @@ struct pcf21xx_config {
> > int max_register;
> > unsigned int has_nvmem:1;
> > unsigned int has_bit_wd_ctl_cd0:1;
> > + u8 regs_td_base; /* Time/data base registers. */
> > };
> >
> > struct pcf2127 {
> > @@ -125,27 +129,31 @@ struct pcf2127 {
> > static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > {
> > struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> > - unsigned char buf[10];
> > + unsigned char buf[7];
> > + unsigned int ctrl3;
> > int ret;
> >
> > /*
> > * 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__);
> > + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &ctrl3);
> > + if (ret)
> > return ret;
> > - }
> >
> > - if (buf[PCF2127_REG_CTRL3] & PCF2127_BIT_CTRL3_BLF)
> > + if (ctrl3 & PCF2127_BIT_CTRL3_BLF)
> > dev_info(dev,
> > "low voltage detected, check/replace RTC battery.\n");
> >
> > + ret = regmap_bulk_read(pcf2127->regmap, pcf2127->cfg->regs_td_base,
> > + buf, sizeof(buf));
> > + if (ret) {
> > + dev_err(dev, "%s: read error\n", __func__);
> > + return ret;
> > + }
> > +
> > /* Clock integrity is not guaranteed when OSF flag is set. */
> > - if (buf[PCF2127_REG_SC] & PCF2127_BIT_SC_OSF) {
> > + if (buf[PCF2127_OFFSET_TD_SC] & PCF2127_BIT_SC_OSF) {
> > /*
> > * no need clear the flag here,
> > * it will be cleared once the new date is saved
> > @@ -158,18 +166,18 @@ static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > dev_dbg(dev,
> > "%s: raw data is cr3=%02x, sec=%02x, min=%02x, hr=%02x, "
> > "mday=%02x, wday=%02x, mon=%02x, year=%02x\n",
> > - __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);
> > - tm->tm_hour = bcd2bin(buf[PCF2127_REG_HR] & 0x3F); /* rtc hr 0-23 */
> > - tm->tm_mday = bcd2bin(buf[PCF2127_REG_DM] & 0x3F);
> > - tm->tm_wday = buf[PCF2127_REG_DW] & 0x07;
> > - tm->tm_mon = bcd2bin(buf[PCF2127_REG_MO] & 0x1F) - 1; /* rtc mn 1-12 */
> > - tm->tm_year = bcd2bin(buf[PCF2127_REG_YR]);
> > + __func__, ctrl3, buf[PCF2127_OFFSET_TD_SC],
> > + buf[PCF2127_OFFSET_TD_MN], buf[PCF2127_OFFSET_TD_HR],
> > + buf[PCF2127_OFFSET_TD_DM], buf[PCF2127_OFFSET_TD_DW],
> > + buf[PCF2127_OFFSET_TD_MO], buf[PCF2127_OFFSET_TD_YR]);
> > +
> > + tm->tm_sec = bcd2bin(buf[PCF2127_OFFSET_TD_SC] & 0x7F);
> > + tm->tm_min = bcd2bin(buf[PCF2127_OFFSET_TD_MN] & 0x7F);
> > + tm->tm_hour = bcd2bin(buf[PCF2127_OFFSET_TD_HR] & 0x3F); /* rtc hr 0-23 */
>
> You can drop the comment

Done

>
> > + tm->tm_mday = bcd2bin(buf[PCF2127_OFFSET_TD_DM] & 0x3F);
> > + tm->tm_wday = buf[PCF2127_OFFSET_TD_DW] & 0x07;
> > + tm->tm_mon = bcd2bin(buf[PCF2127_OFFSET_TD_MO] & 0x1F) - 1; /* rtc mn 1-12 */
>
> This comment too.

Done

>
> > + tm->tm_year = bcd2bin(buf[PCF2127_OFFSET_TD_YR]);
> > tm->tm_year += 100;
> >
> > dev_dbg(dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
> > @@ -207,7 +215,7 @@ static int pcf2127_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > buf[i++] = bin2bcd(tm->tm_year - 100);
> >
> > /* write register's data */
> > - err = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_SC, buf, i);
> > + err = regmap_bulk_write(pcf2127->regmap, pcf2127->cfg->regs_td_base, buf, i);
> > if (err) {
> > dev_err(dev,
> > "%s: err=%d", __func__, err);
> > @@ -650,11 +658,13 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> > .max_register = 0x1d,
> > .has_nvmem = 1,
> > .has_bit_wd_ctl_cd0 = 1,
> > + .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> > },
> > [PCF2129] = {
> > .max_register = 0x19,
> > .has_nvmem = 0,
> > .has_bit_wd_ctl_cd0 = 0,
> > + .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> > },
> > };
> >
> > --
> > 2.30.2
> >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>


--
Hugo Villeneuve <[email protected]>

2023-06-21 14:34:34

by Hugo Villeneuve

Subject: Re: [PATCH v3 00/14] rtc: pcf2127: add PCF2131 driver

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

> Hello,
>
> I know I've been holding off on the review of this series for a while
> and I'm sorry for that.
>
> One of the main issue that is remaining is that the driver ends up being
> 53% bigger and generaly less efficient for no added functionality for
> the existing RTCs.
>
> I know performance is not a concern however, having more code in the
> set/read time and irq paths means that it is more difficult to set an
> get the time precisely.

Hi Alexandre,
one way to keep rtc_read_time() as efficient as before, and even more
efficient by reading 7 instead of 10 registers, would be to drop reading
the CTRL3 register, which is only used to detect and display an info
message for the low battery condition. This low battery check could be
moved to an ioctl call, like it is done in the PCF8523 driver.

Hugo.


> I guess I'll take it as a merged driver but I took a different decision
> for other RTCs.
>
> On 15/12/2022 10:02:01-0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <[email protected]>
> >
> > Hello,
> > this patch series adds the driver for the PCF2131 real-time clock.
> >
> > This RTC is very similar in functionality to the PCF2127/29 with the
> > following differences:
> > -supports two new control registers at offsets 4 and 5
> > -supports a new reset register
> > -supports 4 tamper detection functions instead of 1
> > -has no nvmem (like the PCF2129)
> > -has two output interrupt pins instead of one
> > -has 1/100th seconds capabilities (not supported in this driver)
> > -pcf2127 has watchdog clock sources: 1/60, 1, 64 and 4096Hz
> > pcf2131 has watchdog clock sources: 1/64, 1/4, 4 and 64Hz
> > -watchdog value register cannot be read after being set
> >
> > Most of the register addresses are very different, although they still
> > follow the same layout. For example, the time/date and tamper registers
> > have a different base address, but the offsets are all the same.
> > Consequently, the source code of the PCF2127 driver can be easily adapted
> > to support this new device.
> >
> > Patches 1 to 6 modify the existing pcf2127 driver to make it more generic
> > and able to support multiple variants, like the PCF2131. This is done
> > mostly by using offsets instead of absolute hardcoded register addresses.
> >
> > Patch 7 add actual support for the PCF2131.
> >
> > Patch 8 configures all interrupt sources to go through the INT A pin.
> >
> > Patch 9 changes the PWRMNG bits to be the same with the PCF2131 as they
> > are with the PCF2127/29 (different default values).
> >
> > Patch 10 allow to confirm PCF2131 device presence by reading the reset
> > register fixed pattern.
> >
> > Patch 11 adapt the time/date registers write sequence for PCF2131 (STOP and
> > CPR bits).
> >
> > Patch 12 add support for generic watchdog timing configuration.
> >
> > Patch 13 add a new flag to identify if device has read support for reading
> > watchdog register value.
> > Since the watchdog value register cannot be read on the PCF2131 after
> > being set, it seems that we cannot detect if watchdog timer was
> > started by bootloader. I am not sure what is the best way to handle
> > this situation, suggestions are welcomed.
> >
> > Patch 14 add the dt-bindings for the PCF2131.
> >
> > I have tested the driver using a PCF2131-ARD evaluation board connected to
> > an NXP imx8mp evaluation board:
> > - Time get/set ok;
> > - Alarms get/set ok
> > - Timestamp 1 to 4 ok
> > - IRQ alarm ok
> > - Watchdog ok
> > - Also tested successfully with "RTC Driver Test Example" from
> > Documentation/rtc.txt
> >
> > I have also tested the driver on a custom PCF2129 adapter board connected to a
> > beaglebone black.
> >
> > Thank you.
> >
> > Link: [v1] https://patchwork.ozlabs.org/project/rtc-linux/patch/[email protected]/
> > Link: [v2] https://patchwork.ozlabs.org/project/rtc-linux/list/?series=285734
> >
> > Changes for V3:
> > - Rebased for kernel v6.1
> >
> > Changes for V2:
> > - In general, fix and improvements after I have tested on real hardware
> > - Fix alarm interrupt A/B mask setting for PCF2131:
> > PCF2131_BIT_INT_AIE must be cleared, not set, to enable interrupt.
> > - Remove low_reg validation: only check if TS interrupt flag is
> > defined, as low_reg is defined at address 0 for PCF2127/29.
> > - Change PWRMNG value for PCF2131: default is different than PCF2127/29.
> > - Adapt time/date registers write sequence for PCF2131 (STOP and CPR bits).
> > - Map all interrupt sources to INT A pin
> > - Read and validate PCF2131 device presence from RESET register
> > - Adapt watchdog configuration for PCF2131
> >
> > Hugo Villeneuve (14):
> > rtc: pcf2127: add variant-specific configuration structure
> > rtc: pcf2127: adapt for time/date registers at any offset
> > rtc: pcf2127: adapt for alarm registers at any offset
> > rtc: pcf2127: adapt for WD registers at any offset
> > rtc: pcf2127: adapt for CLKOUT register at any offset
> > rtc: pcf2127: add support for multiple TS functions
> > rtc: pcf2127: add support for PCF2131 RTC
> > rtc: pcf2127: add support for PCF2131 interrupts on output INT_A
> > rtc: pcf2127: set PWRMNG value for PCF2131
> > rtc: pcf2127: read and validate PCF2131 device signature
> > rtc: pcf2127: adapt time/date registers write sequence for PCF2131
> > rtc: pcf2127: support generic watchdog timing configuration
> > rtc: pcf2127: add flag for watchdog register value read support
> > dt-bindings: rtc: pcf2127: add PCF2131
> >
> > .../devicetree/bindings/rtc/nxp,pcf2127.yaml | 4 +-
> > drivers/rtc/Kconfig | 4 +-
> > drivers/rtc/rtc-pcf2127.c | 939 ++++++++++++++----
> > 3 files changed, 752 insertions(+), 195 deletions(-)
> >
> > --
> > 2.30.2
> >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>

2023-06-21 17:16:19

by Hugo Villeneuve

Subject: Re: [PATCH v3 00/14] rtc: pcf2127: add PCF2131 driver

On Wed, 21 Jun 2023 10:14:29 -0400
Hugo Villeneuve <[email protected]> wrote:

> On Fri, 20 Jan 2023 20:05:07 +0100
> Alexandre Belloni <[email protected]> wrote:
>
> > Hello,
> >
> > I know I've been holding off on the review of this series for a while
> > and I'm sorry for that.
> >
> > One of the main issue that is remaining is that the driver ends up being
> > 53% bigger and generaly less efficient for no added functionality for
> > the existing RTCs.
> >
> > I know performance is not a concern however, having more code in the
> > set/read time and irq paths means that it is more difficult to set an
> > get the time precisely.
>
> Hi Alexandre,
> one way to keep rtc_read_time() as efficient as before, and even more
> efficient by reading 7 instead of 10 registers, would be to drop reading
> the CTRL3 register, which is only used to detect and display an info
> message for the low battery condition. This low battery check could be
> moved to an ioctl call, like it is done in the PCF8523 driver.
>
> Hugo.

Hi,
in fact it is already part of the ioctl, so it is even simpler...

Hugo.


>
>
> > I guess I'll take it as a merged driver but I took a different decision
> > for other RTCs.
> >
> > On 15/12/2022 10:02:01-0500, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <[email protected]>
> > >
> > > Hello,
> > > this patch series adds the driver for the PCF2131 real-time clock.
> > >
> > > This RTC is very similar in functionality to the PCF2127/29 with the
> > > following differences:
> > > -supports two new control registers at offsets 4 and 5
> > > -supports a new reset register
> > > -supports 4 tamper detection functions instead of 1
> > > -has no nvmem (like the PCF2129)
> > > -has two output interrupt pins instead of one
> > > -has 1/100th seconds capabilities (not supported in this driver)
> > > -pcf2127 has watchdog clock sources: 1/60, 1, 64 and 4096Hz
> > > pcf2131 has watchdog clock sources: 1/64, 1/4, 4 and 64Hz
> > > -watchdog value register cannot be read after being set
> > >
> > > Most of the register addresses are very different, although they still
> > > follow the same layout. For example, the time/date and tamper registers
> > > have a different base address, but the offsets are all the same.
> > > Consequently, the source code of the PCF2127 driver can be easily adapted
> > > to support this new device.
> > >
> > > Patches 1 to 6 modify the existing pcf2127 driver to make it more generic
> > > and able to support multiple variants, like the PCF2131. This is done
> > > mostly by using offsets instead of absolute hardcoded register addresses.
> > >
> > > Patch 7 add actual support for the PCF2131.
> > >
> > > Patch 8 configures all interrupt sources to go through the INT A pin.
> > >
> > > Patch 9 changes the PWRMNG bits to be the same with the PCF2131 as they
> > > are with the PCF2127/29 (different default values).
> > >
> > > Patch 10 allow to confirm PCF2131 device presence by reading the reset
> > > register fixed pattern.
> > >
> > > Patch 11 adapt the time/date registers write sequence for PCF2131 (STOP and
> > > CPR bits).
> > >
> > > Patch 12 add support for generic watchdog timing configuration.
> > >
> > > Patch 13 add a new flag to identify if device has read support for reading
> > > watchdog register value.
> > > Since the watchdog value register cannot be read on the PCF2131 after
> > > being set, it seems that we cannot detect if watchdog timer was
> > > started by bootloader. I am not sure what is the best way to handle
> > > this situation, suggestions are welcomed.
> > >
> > > Patch 14 add the dt-bindings for the PCF2131.
> > >
> > > I have tested the driver using a PCF2131-ARD evaluation board connected to
> > > an NXP imx8mp evaluation board:
> > > - Time get/set ok;
> > > - Alarms get/set ok
> > > - Timestamp 1 to 4 ok
> > > - IRQ alarm ok
> > > - Watchdog ok
> > > - Also tested successfully with "RTC Driver Test Example" from
> > > Documentation/rtc.txt
> > >
> > > I have also tested the driver on a custom PCF2129 adapter board connected to a
> > > beaglebone black.
> > >
> > > Thank you.
> > >
> > > Link: [v1] https://patchwork.ozlabs.org/project/rtc-linux/patch/[email protected]/
> > > Link: [v2] https://patchwork.ozlabs.org/project/rtc-linux/list/?series=285734
> > >
> > > Changes for V3:
> > > - Rebased for kernel v6.1
> > >
> > > Changes for V2:
> > > - In general, fix and improvements after I have tested on real hardware
> > > - Fix alarm interrupt A/B mask setting for PCF2131:
> > > PCF2131_BIT_INT_AIE must be cleared, not set, to enable interrupt.
> > > - Remove low_reg validation: only check if TS interrupt flag is
> > > defined, as low_reg is defined at address 0 for PCF2127/29.
> > > - Change PWRMNG value for PCF2131: default is different than PCF2127/29.
> > > - Adapt time/date registers write sequence for PCF2131 (STOP and CPR bits).
> > > - Map all interrupt sources to INT A pin
> > > - Read and validate PCF2131 device presence from RESET register
> > > - Adapt watchdog configuration for PCF2131
> > >
> > > Hugo Villeneuve (14):
> > > rtc: pcf2127: add variant-specific configuration structure
> > > rtc: pcf2127: adapt for time/date registers at any offset
> > > rtc: pcf2127: adapt for alarm registers at any offset
> > > rtc: pcf2127: adapt for WD registers at any offset
> > > rtc: pcf2127: adapt for CLKOUT register at any offset
> > > rtc: pcf2127: add support for multiple TS functions
> > > rtc: pcf2127: add support for PCF2131 RTC
> > > rtc: pcf2127: add support for PCF2131 interrupts on output INT_A
> > > rtc: pcf2127: set PWRMNG value for PCF2131
> > > rtc: pcf2127: read and validate PCF2131 device signature
> > > rtc: pcf2127: adapt time/date registers write sequence for PCF2131
> > > rtc: pcf2127: support generic watchdog timing configuration
> > > rtc: pcf2127: add flag for watchdog register value read support
> > > dt-bindings: rtc: pcf2127: add PCF2131
> > >
> > > .../devicetree/bindings/rtc/nxp,pcf2127.yaml | 4 +-
> > > drivers/rtc/Kconfig | 4 +-
> > > drivers/rtc/rtc-pcf2127.c | 939 ++++++++++++++----
> > > 3 files changed, 752 insertions(+), 195 deletions(-)
> > >
> > > --
> > > 2.30.2
> > >
> >
> > --
> > Alexandre Belloni, co-owner and COO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> >

2023-06-21 18:52:05

by Alexandre Belloni

Subject: Re: [PATCH v3 00/14] rtc: pcf2127: add PCF2131 driver

On 21/06/2023 12:59:45-0400, Hugo Villeneuve wrote:
> On Wed, 21 Jun 2023 10:14:29 -0400
> Hugo Villeneuve <[email protected]> wrote:
>
> > On Fri, 20 Jan 2023 20:05:07 +0100
> > Alexandre Belloni <[email protected]> wrote:
> >
> > > Hello,
> > >
> > > I know I've been holding off on the review of this series for a while
> > > and I'm sorry for that.
> > >
> > > One of the main issue that is remaining is that the driver ends up being
> > > 53% bigger and generaly less efficient for no added functionality for
> > > the existing RTCs.
> > >
> > > I know performance is not a concern however, having more code in the
> > > set/read time and irq paths means that it is more difficult to set an
> > > get the time precisely.
> >
> > Hi Alexandre,
> > one way to keep rtc_read_time() as efficient as before, and even more
> > efficient by reading 7 instead of 10 registers, would be to drop reading
> > the CTRL3 register, which is only used to detect and display an info
> > message for the low battery condition. This low battery check could be
> > moved to an ioctl call, like it is done in the PCF8523 driver.
> >
> > Hugo.
>
> Hi,
> in fact it is already part of the ioctl, so it is even simpler...
>

Yes, the dev_info can be removed.

> Hugo.
>
>
> >
> >
> > > I guess I'll take it as a merged driver but I took a different decision
> > > for other RTCs.
> > >
> > > On 15/12/2022 10:02:01-0500, Hugo Villeneuve wrote:
> > > > From: Hugo Villeneuve <[email protected]>
> > > >
> > > > Hello,
> > > > this patch series adds the driver for the PCF2131 real-time clock.
> > > >
> > > > This RTC is very similar in functionality to the PCF2127/29 with the
> > > > following differences:
> > > > -supports two new control registers at offsets 4 and 5
> > > > -supports a new reset register
> > > > -supports 4 tamper detection functions instead of 1
> > > > -has no nvmem (like the PCF2129)
> > > > -has two output interrupt pins instead of one
> > > > -has 1/100th seconds capabilities (not supported in this driver)
> > > > -pcf2127 has watchdog clock sources: 1/60, 1, 64 and 4096Hz
> > > > pcf2131 has watchdog clock sources: 1/64, 1/4, 4 and 64Hz
> > > > -watchdog value register cannot be read after being set
> > > >
> > > > Most of the register addresses are very different, although they still
> > > > follow the same layout. For example, the time/date and tamper registers
> > > > have a different base address, but the offsets are all the same.
> > > > Consequently, the source code of the PCF2127 driver can be easily adapted
> > > > to support this new device.
> > > >
> > > > Patches 1 to 6 modify the existing pcf2127 driver to make it more generic
> > > > and able to support multiple variants, like the PCF2131. This is done
> > > > mostly by using offsets instead of absolute hardcoded register addresses.
> > > >
> > > > Patch 7 add actual support for the PCF2131.
> > > >
> > > > Patch 8 configures all interrupt sources to go through the INT A pin.
> > > >
> > > > Patch 9 changes the PWRMNG bits to be the same with the PCF2131 as they
> > > > are with the PCF2127/29 (different default values).
> > > >
> > > > Patch 10 allow to confirm PCF2131 device presence by reading the reset
> > > > register fixed pattern.
> > > >
> > > > Patch 11 adapt the time/date registers write sequence for PCF2131 (STOP and
> > > > CPR bits).
> > > >
> > > > Patch 12 add support for generic watchdog timing configuration.
> > > >
> > > > Patch 13 add a new flag to identify if device has read support for reading
> > > > watchdog register value.
> > > > Since the watchdog value register cannot be read on the PCF2131 after
> > > > being set, it seems that we cannot detect if watchdog timer was
> > > > started by bootloader. I am not sure what is the best way to handle
> > > > this situation, suggestions are welcomed.
> > > >
> > > > Patch 14 add the dt-bindings for the PCF2131.
> > > >
> > > > I have tested the driver using a PCF2131-ARD evaluation board connected to
> > > > an NXP imx8mp evaluation board:
> > > > - Time get/set ok;
> > > > - Alarms get/set ok
> > > > - Timestamp 1 to 4 ok
> > > > - IRQ alarm ok
> > > > - Watchdog ok
> > > > - Also tested successfully with "RTC Driver Test Example" from
> > > > Documentation/rtc.txt
> > > >
> > > > I have also tested the driver on a custom PCF2129 adapter board connected to a
> > > > beaglebone black.
> > > >
> > > > Thank you.
> > > >
> > > > Link: [v1] https://patchwork.ozlabs.org/project/rtc-linux/patch/[email protected]/
> > > > Link: [v2] https://patchwork.ozlabs.org/project/rtc-linux/list/?series=285734
> > > >
> > > > Changes for V3:
> > > > - Rebased for kernel v6.1
> > > >
> > > > Changes for V2:
> > > > - In general, fix and improvements after I have tested on real hardware
> > > > - Fix alarm interrupt A/B mask setting for PCF2131:
> > > > PCF2131_BIT_INT_AIE must be cleared, not set, to enable interrupt.
> > > > - Remove low_reg validation: only check if TS interrupt flag is
> > > > defined, as low_reg is defined at address 0 for PCF2127/29.
> > > > - Change PWRMNG value for PCF2131: default is different than PCF2127/29.
> > > > - Adapt time/date registers write sequence for PCF2131 (STOP and CPR bits).
> > > > - Map all interrupt sources to INT A pin
> > > > - Read and validate PCF2131 device presence from RESET register
> > > > - Adapt watchdog configuration for PCF2131
> > > >
> > > > Hugo Villeneuve (14):
> > > > rtc: pcf2127: add variant-specific configuration structure
> > > > rtc: pcf2127: adapt for time/date registers at any offset
> > > > rtc: pcf2127: adapt for alarm registers at any offset
> > > > rtc: pcf2127: adapt for WD registers at any offset
> > > > rtc: pcf2127: adapt for CLKOUT register at any offset
> > > > rtc: pcf2127: add support for multiple TS functions
> > > > rtc: pcf2127: add support for PCF2131 RTC
> > > > rtc: pcf2127: add support for PCF2131 interrupts on output INT_A
> > > > rtc: pcf2127: set PWRMNG value for PCF2131
> > > > rtc: pcf2127: read and validate PCF2131 device signature
> > > > rtc: pcf2127: adapt time/date registers write sequence for PCF2131
> > > > rtc: pcf2127: support generic watchdog timing configuration
> > > > rtc: pcf2127: add flag for watchdog register value read support
> > > > dt-bindings: rtc: pcf2127: add PCF2131
> > > >
> > > > .../devicetree/bindings/rtc/nxp,pcf2127.yaml | 4 +-
> > > > drivers/rtc/Kconfig | 4 +-
> > > > drivers/rtc/rtc-pcf2127.c | 939 ++++++++++++++----
> > > > 3 files changed, 752 insertions(+), 195 deletions(-)
> > > >
> > > > --
> > > > 2.30.2
> > > >
> > >
> > > --
> > > Alexandre Belloni, co-owner and COO, Bootlin
> > > Embedded Linux and Kernel engineering
> > > https://bootlin.com
> > >

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

2023-06-21 18:56:14

by Hugo Villeneuve

Subject: Re: [PATCH v3 00/14] rtc: pcf2127: add PCF2131 driver

On Wed, 21 Jun 2023 20:14:41 +0200
Alexandre Belloni <[email protected]> wrote:

> On 21/06/2023 12:59:45-0400, Hugo Villeneuve wrote:
> > On Wed, 21 Jun 2023 10:14:29 -0400
> > Hugo Villeneuve <[email protected]> wrote:
> >
> > > On Fri, 20 Jan 2023 20:05:07 +0100
> > > Alexandre Belloni <[email protected]> wrote:
> > >
> > > > Hello,
> > > >
> > > > I know I've been holding off on the review of this series for a while
> > > > and I'm sorry for that.
> > > >
> > > > One of the main issue that is remaining is that the driver ends up being
> > > > 53% bigger and generaly less efficient for no added functionality for
> > > > the existing RTCs.
> > > >
> > > > I know performance is not a concern however, having more code in the
> > > > set/read time and irq paths means that it is more difficult to set an
> > > > get the time precisely.
> > >
> > > Hi Alexandre,
> > > one way to keep rtc_read_time() as efficient as before, and even more
> > > efficient by reading 7 instead of 10 registers, would be to drop reading
> > > the CTRL3 register, which is only used to detect and display an info
> > > message for the low battery condition. This low battery check could be
> > > moved to an ioctl call, like it is done in the PCF8523 driver.
> > >
> > > Hugo.
> >
> > Hi,
> > in fact it is already part of the ioctl, so it is even simpler...
> >
>
> Yes, the dev_info can be removed.

Hi,
great, I will integrate that patch to improve rtc_read_time()
performance, and resubmit V4 soon with the requested changes mentioned
during V3 review.

Thank you, Hugo.


> > > > I guess I'll take it as a merged driver but I took a different decision
> > > > for other RTCs.
> > > >
> > > > On 15/12/2022 10:02:01-0500, Hugo Villeneuve wrote:
> > > > > From: Hugo Villeneuve <[email protected]>
> > > > >
> > > > > Hello,
> > > > > this patch series adds the driver for the PCF2131 real-time clock.
> > > > >
> > > > > This RTC is very similar in functionality to the PCF2127/29 with the
> > > > > following differences:
> > > > > -supports two new control registers at offsets 4 and 5
> > > > > -supports a new reset register
> > > > > -supports 4 tamper detection functions instead of 1
> > > > > -has no nvmem (like the PCF2129)
> > > > > -has two output interrupt pins instead of one
> > > > > -has 1/100th seconds capabilities (not supported in this driver)
> > > > > -pcf2127 has watchdog clock sources: 1/60, 1, 64 and 4096Hz
> > > > > pcf2131 has watchdog clock sources: 1/64, 1/4, 4 and 64Hz
> > > > > -watchdog value register cannot be read after being set
> > > > >
> > > > > Most of the register addresses are very different, although they still
> > > > > follow the same layout. For example, the time/date and tamper registers
> > > > > have a different base address, but the offsets are all the same.
> > > > > Consequently, the source code of the PCF2127 driver can be easily adapted
> > > > > to support this new device.
> > > > >
> > > > > Patches 1 to 6 modify the existing pcf2127 driver to make it more generic
> > > > > and able to support multiple variants, like the PCF2131. This is done
> > > > > mostly by using offsets instead of absolute hardcoded register addresses.
> > > > >
> > > > > Patch 7 add actual support for the PCF2131.
> > > > >
> > > > > Patch 8 configures all interrupt sources to go through the INT A pin.
> > > > >
> > > > > Patch 9 changes the PWRMNG bits to be the same with the PCF2131 as they
> > > > > are with the PCF2127/29 (different default values).
> > > > >
> > > > > Patch 10 allow to confirm PCF2131 device presence by reading the reset
> > > > > register fixed pattern.
> > > > >
> > > > > Patch 11 adapt the time/date registers write sequence for PCF2131 (STOP and
> > > > > CPR bits).
> > > > >
> > > > > Patch 12 add support for generic watchdog timing configuration.
> > > > >
> > > > > Patch 13 add a new flag to identify if device has read support for reading
> > > > > watchdog register value.
> > > > > Since the watchdog value register cannot be read on the PCF2131 after
> > > > > being set, it seems that we cannot detect if watchdog timer was
> > > > > started by bootloader. I am not sure what is the best way to handle
> > > > > this situation, suggestions are welcomed.
> > > > >
> > > > > Patch 14 add the dt-bindings for the PCF2131.
> > > > >
> > > > > I have tested the driver using a PCF2131-ARD evaluation board connected to
> > > > > an NXP imx8mp evaluation board:
> > > > > - Time get/set ok;
> > > > > - Alarms get/set ok
> > > > > - Timestamp 1 to 4 ok
> > > > > - IRQ alarm ok
> > > > > - Watchdog ok
> > > > > - Also tested successfully with "RTC Driver Test Example" from
> > > > > Documentation/rtc.txt
> > > > >
> > > > > I have also tested the driver on a custom PCF2129 adapter board connected to a
> > > > > beaglebone black.
> > > > >
> > > > > Thank you.
> > > > >
> > > > > Link: [v1] https://patchwork.ozlabs.org/project/rtc-linux/patch/[email protected]/
> > > > > Link: [v2] https://patchwork.ozlabs.org/project/rtc-linux/list/?series=285734
> > > > >
> > > > > Changes for V3:
> > > > > - Rebased for kernel v6.1
> > > > >
> > > > > Changes for V2:
> > > > > - In general, fix and improvements after I have tested on real hardware
> > > > > - Fix alarm interrupt A/B mask setting for PCF2131:
> > > > > PCF2131_BIT_INT_AIE must be cleared, not set, to enable interrupt.
> > > > > - Remove low_reg validation: only check if TS interrupt flag is
> > > > > defined, as low_reg is defined at address 0 for PCF2127/29.
> > > > > - Change PWRMNG value for PCF2131: default is different than PCF2127/29.
> > > > > - Adapt time/date registers write sequence for PCF2131 (STOP and CPR bits).
> > > > > - Map all interrupt sources to INT A pin
> > > > > - Read and validate PCF2131 device presence from RESET register
> > > > > - Adapt watchdog configuration for PCF2131
> > > > >
> > > > > Hugo Villeneuve (14):
> > > > > rtc: pcf2127: add variant-specific configuration structure
> > > > > rtc: pcf2127: adapt for time/date registers at any offset
> > > > > rtc: pcf2127: adapt for alarm registers at any offset
> > > > > rtc: pcf2127: adapt for WD registers at any offset
> > > > > rtc: pcf2127: adapt for CLKOUT register at any offset
> > > > > rtc: pcf2127: add support for multiple TS functions
> > > > > rtc: pcf2127: add support for PCF2131 RTC
> > > > > rtc: pcf2127: add support for PCF2131 interrupts on output INT_A
> > > > > rtc: pcf2127: set PWRMNG value for PCF2131
> > > > > rtc: pcf2127: read and validate PCF2131 device signature
> > > > > rtc: pcf2127: adapt time/date registers write sequence for PCF2131
> > > > > rtc: pcf2127: support generic watchdog timing configuration
> > > > > rtc: pcf2127: add flag for watchdog register value read support
> > > > > dt-bindings: rtc: pcf2127: add PCF2131
> > > > >
> > > > > .../devicetree/bindings/rtc/nxp,pcf2127.yaml | 4 +-
> > > > > drivers/rtc/Kconfig | 4 +-
> > > > > drivers/rtc/rtc-pcf2127.c | 939 ++++++++++++++----
> > > > > 3 files changed, 752 insertions(+), 195 deletions(-)
> > > > >
> > > > > --
> > > > > 2.30.2
> > > > >
> > > >
> > > > --
> > > > Alexandre Belloni, co-owner and COO, Bootlin
> > > > Embedded Linux and Kernel engineering
> > > > https://bootlin.com
> > > >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>

2023-06-21 18:56:57

by Alexandre Belloni

Subject: Re: [PATCH v3 02/14] rtc: pcf2127: adapt for time/date registers at any offset

On 23/01/2023 10:54:57-0500, Hugo Villeneuve wrote:
> On Fri, 20 Jan 2023 19:47:53 +0100
> Alexandre Belloni <[email protected]> wrote:
>
> > On 15/12/2022 10:02:03-0500, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <[email protected]>
> > >
> > > This will simplify the implementation of new variants into this driver.
> > >
> > > Some variants (PCF2131) have a 100th seconds register. This register is
> > > currently not supported in this driver.
> > >
> > > Signed-off-by: Hugo Villeneuve <[email protected]>
> > > ---
> > > drivers/rtc/rtc-pcf2127.c | 68 ++++++++++++++++++++++-----------------
> > > 1 file changed, 39 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > > index b9a5d47a439f..fb0caacaabee 100644
> > > --- a/drivers/rtc/rtc-pcf2127.c
> > > +++ b/drivers/rtc/rtc-pcf2127.c
> > > @@ -44,14 +44,17 @@
> > > #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_REG_TIME_DATE_BASE 0x03
> > > +/* Time and date registers offsets (starting from base register) */
> > > +#define PCF2127_OFFSET_TD_SC 0
> > > +#define PCF2127_OFFSET_TD_MN 1
> > > +#define PCF2127_OFFSET_TD_HR 2
> > > +#define PCF2127_OFFSET_TD_DM 3
> > > +#define PCF2127_OFFSET_TD_DW 4
> > > +#define PCF2127_OFFSET_TD_MO 5
> > > +#define PCF2127_OFFSET_TD_YR 6
> >
> > Same comment as for the alarms, I would simply remove the defines as
> > they don't really carry any useful information.
>
> Note that if I remove them, the patch for pcf2127_rtc_read_time() would look like this:
>
> /* Clock integrity is not guaranteed when OSF flag is set. */
> - if (buf[PCF2127_REG_SC] & PCF2127_BIT_SC_OSF) {
> + if (buf[0] & PCF2127_BIT_SC_OSF) {
> ...
> - __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);
> - tm->tm_hour = bcd2bin(buf[PCF2127_REG_HR] & 0x3F); /* rtc hr 0-23 */
> - tm->tm_mday = bcd2bin(buf[PCF2127_REG_DM] & 0x3F);
> - tm->tm_wday = buf[PCF2127_REG_DW] & 0x07;
> - tm->tm_mon = bcd2bin(buf[PCF2127_REG_MO] & 0x1F) - 1; /* rtc mn 1-12 */
> - tm->tm_year = bcd2bin(buf[PCF2127_REG_YR]);
> + __func__, ctrl3, buf[0],
> + buf[1], buf[2],
> + buf[3], buf[4],
> + buf[5], buf[PCF2127_OFFSET_TD_YR]);
> +
> + tm->tm_sec = bcd2bin(buf[0] & 0x7F);
> + tm->tm_min = bcd2bin(buf[1] & 0x7F);
> + tm->tm_hour = bcd2bin(buf[2] & 0x3F); /* rtc hr 0-23 */
> + tm->tm_mday = bcd2bin(buf[3] & 0x3F);
> + tm->tm_wday = buf[4] & 0x07;
> + tm->tm_mon = bcd2bin(buf[5] & 0x1F) - 1; /* rtc mn 1-12 */
> + tm->tm_year = bcd2bin(buf[6]);
>
> Do you still want to remove the defines then?

This is fine, yes.

>
>
> > > +/* Time and date registers bits */
> > > #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
> > > /* Alarm registers */
> > > #define PCF2127_REG_ALARM_SC 0x0A
> > > #define PCF2127_REG_ALARM_MN 0x0B
> > > @@ -106,6 +109,7 @@ struct pcf21xx_config {
> > > int max_register;
> > > unsigned int has_nvmem:1;
> > > unsigned int has_bit_wd_ctl_cd0:1;
> > > + u8 regs_td_base; /* Time/data base registers. */
> > > };
> > >
> > > struct pcf2127 {
> > > @@ -125,27 +129,31 @@ struct pcf2127 {
> > > static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > > {
> > > struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
> > > - unsigned char buf[10];
> > > + unsigned char buf[7];
> > > + unsigned int ctrl3;
> > > int ret;
> > >
> > > /*
> > > * 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__);
> > > + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &ctrl3);
> > > + if (ret)
> > > return ret;
> > > - }
> > >
> > > - if (buf[PCF2127_REG_CTRL3] & PCF2127_BIT_CTRL3_BLF)
> > > + if (ctrl3 & PCF2127_BIT_CTRL3_BLF)
> > > dev_info(dev,
> > > "low voltage detected, check/replace RTC battery.\n");
> > >
> > > + ret = regmap_bulk_read(pcf2127->regmap, pcf2127->cfg->regs_td_base,
> > > + buf, sizeof(buf));
> > > + if (ret) {
> > > + dev_err(dev, "%s: read error\n", __func__);
> > > + return ret;
> > > + }
> > > +
> > > /* Clock integrity is not guaranteed when OSF flag is set. */
> > > - if (buf[PCF2127_REG_SC] & PCF2127_BIT_SC_OSF) {
> > > + if (buf[PCF2127_OFFSET_TD_SC] & PCF2127_BIT_SC_OSF) {
> > > /*
> > > * no need clear the flag here,
> > > * it will be cleared once the new date is saved
> > > @@ -158,18 +166,18 @@ static int pcf2127_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > > dev_dbg(dev,
> > > "%s: raw data is cr3=%02x, sec=%02x, min=%02x, hr=%02x, "
> > > "mday=%02x, wday=%02x, mon=%02x, year=%02x\n",
> > > - __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);
> > > - tm->tm_hour = bcd2bin(buf[PCF2127_REG_HR] & 0x3F); /* rtc hr 0-23 */
> > > - tm->tm_mday = bcd2bin(buf[PCF2127_REG_DM] & 0x3F);
> > > - tm->tm_wday = buf[PCF2127_REG_DW] & 0x07;
> > > - tm->tm_mon = bcd2bin(buf[PCF2127_REG_MO] & 0x1F) - 1; /* rtc mn 1-12 */
> > > - tm->tm_year = bcd2bin(buf[PCF2127_REG_YR]);
> > > + __func__, ctrl3, buf[PCF2127_OFFSET_TD_SC],
> > > + buf[PCF2127_OFFSET_TD_MN], buf[PCF2127_OFFSET_TD_HR],
> > > + buf[PCF2127_OFFSET_TD_DM], buf[PCF2127_OFFSET_TD_DW],
> > > + buf[PCF2127_OFFSET_TD_MO], buf[PCF2127_OFFSET_TD_YR]);
> > > +
> > > + tm->tm_sec = bcd2bin(buf[PCF2127_OFFSET_TD_SC] & 0x7F);
> > > + tm->tm_min = bcd2bin(buf[PCF2127_OFFSET_TD_MN] & 0x7F);
> > > + tm->tm_hour = bcd2bin(buf[PCF2127_OFFSET_TD_HR] & 0x3F); /* rtc hr 0-23 */
> >
> > You can drop the comment
>
> Done
>
> >
> > > + tm->tm_mday = bcd2bin(buf[PCF2127_OFFSET_TD_DM] & 0x3F);
> > > + tm->tm_wday = buf[PCF2127_OFFSET_TD_DW] & 0x07;
> > > + tm->tm_mon = bcd2bin(buf[PCF2127_OFFSET_TD_MO] & 0x1F) - 1; /* rtc mn 1-12 */
> >
> > This comment too.
>
> Done
>
> >
> > > + tm->tm_year = bcd2bin(buf[PCF2127_OFFSET_TD_YR]);
> > > tm->tm_year += 100;
> > >
> > > dev_dbg(dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
> > > @@ -207,7 +215,7 @@ static int pcf2127_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > > buf[i++] = bin2bcd(tm->tm_year - 100);
> > >
> > > /* write register's data */
> > > - err = regmap_bulk_write(pcf2127->regmap, PCF2127_REG_SC, buf, i);
> > > + err = regmap_bulk_write(pcf2127->regmap, pcf2127->cfg->regs_td_base, buf, i);
> > > if (err) {
> > > dev_err(dev,
> > > "%s: err=%d", __func__, err);
> > > @@ -650,11 +658,13 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> > > .max_register = 0x1d,
> > > .has_nvmem = 1,
> > > .has_bit_wd_ctl_cd0 = 1,
> > > + .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> > > },
> > > [PCF2129] = {
> > > .max_register = 0x19,
> > > .has_nvmem = 0,
> > > .has_bit_wd_ctl_cd0 = 0,
> > > + .regs_td_base = PCF2127_REG_TIME_DATE_BASE,
> > > },
> > > };
> > >
> > > --
> > > 2.30.2
> > >
> >
> > --
> > Alexandre Belloni, co-owner and COO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> >
>
>
> --
> Hugo Villeneuve <[email protected]>

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

2023-07-05 13:54:23

by Hugo Villeneuve

Subject: Re: [PATCH v3 00/14] rtc: pcf2127: add PCF2131 driver

On Wed, 21 Jun 2023 14:28:52 -0400
Hugo Villeneuve <[email protected]> wrote:

> On Wed, 21 Jun 2023 20:14:41 +0200
> Alexandre Belloni <[email protected]> wrote:
>
> > On 21/06/2023 12:59:45-0400, Hugo Villeneuve wrote:
> > > On Wed, 21 Jun 2023 10:14:29 -0400
> > > Hugo Villeneuve <[email protected]> wrote:
> > >
> > > > On Fri, 20 Jan 2023 20:05:07 +0100
> > > > Alexandre Belloni <[email protected]> wrote:
> > > >
> > > > > Hello,
> > > > >
> > > > > I know I've been holding off on the review of this series for a while
> > > > > and I'm sorry for that.
> > > > >
> > > > > One of the main issue that is remaining is that the driver ends up being
> > > > > 53% bigger and generaly less efficient for no added functionality for
> > > > > the existing RTCs.
> > > > >
> > > > > I know performance is not a concern however, having more code in the
> > > > > set/read time and irq paths means that it is more difficult to set an
> > > > > get the time precisely.
> > > >
> > > > Hi Alexandre,
> > > > one way to keep rtc_read_time() as efficient as before, and even more
> > > > efficient by reading 7 instead of 10 registers, would be to drop reading
> > > > the CTRL3 register, which is only used to detect and display an info
> > > > message for the low battery condition. This low battery check could be
> > > > moved to an ioctl call, like it is done in the PCF8523 driver.
> > > >
> > > > Hugo.
> > >
> > > Hi,
> > > in fact it is already part of the ioctl, so it is even simpler...
> > >
> >
> > Yes, the dev_info can be removed.
>
> Hi,
> great, I will integrate that patch to improve rtc_read_time()
> performance, and resubmit V4 soon with the requested changes mentioned
> during V3 review.
>
> Thank you, Hugo.

Hi Alexandre,
I submitted V4 a few days ago, please let me know if everything is
in order and all comments properly addressed.

If all is good, any chance we can have that integrated into v6.5?

Thank you, Hugo.


> > > > > I guess I'll take it as a merged driver but I took a different decision
> > > > > for other RTCs.
> > > > >
> > > > > On 15/12/2022 10:02:01-0500, Hugo Villeneuve wrote:
> > > > > > From: Hugo Villeneuve <[email protected]>
> > > > > >
> > > > > > Hello,
> > > > > > this patch series adds the driver for the PCF2131 real-time clock.
> > > > > >
> > > > > > This RTC is very similar in functionality to the PCF2127/29 with the
> > > > > > following differences:
> > > > > > -supports two new control registers at offsets 4 and 5
> > > > > > -supports a new reset register
> > > > > > -supports 4 tamper detection functions instead of 1
> > > > > > -has no nvmem (like the PCF2129)
> > > > > > -has two output interrupt pins instead of one
> > > > > > -has 1/100th seconds capabilities (not supported in this driver)
> > > > > > -pcf2127 has watchdog clock sources: 1/60, 1, 64 and 4096Hz
> > > > > > pcf2131 has watchdog clock sources: 1/64, 1/4, 4 and 64Hz
> > > > > > -watchdog value register cannot be read after being set
> > > > > >
> > > > > > Most of the register addresses are very different, although they still
> > > > > > follow the same layout. For example, the time/date and tamper registers
> > > > > > have a different base address, but the offsets are all the same.
> > > > > > Consequently, the source code of the PCF2127 driver can be easily adapted
> > > > > > to support this new device.
> > > > > >
> > > > > > Patches 1 to 6 modify the existing pcf2127 driver to make it more generic
> > > > > > and able to support multiple variants, like the PCF2131. This is done
> > > > > > mostly by using offsets instead of absolute hardcoded register addresses.
> > > > > >
> > > > > > Patch 7 add actual support for the PCF2131.
> > > > > >
> > > > > > Patch 8 configures all interrupt sources to go through the INT A pin.
> > > > > >
> > > > > > Patch 9 changes the PWRMNG bits to be the same with the PCF2131 as they
> > > > > > are with the PCF2127/29 (different default values).
> > > > > >
> > > > > > Patch 10 allow to confirm PCF2131 device presence by reading the reset
> > > > > > register fixed pattern.
> > > > > >
> > > > > > Patch 11 adapt the time/date registers write sequence for PCF2131 (STOP and
> > > > > > CPR bits).
> > > > > >
> > > > > > Patch 12 add support for generic watchdog timing configuration.
> > > > > >
> > > > > > Patch 13 add a new flag to identify if device has read support for reading
> > > > > > watchdog register value.
> > > > > > Since the watchdog value register cannot be read on the PCF2131 after
> > > > > > being set, it seems that we cannot detect if watchdog timer was
> > > > > > started by bootloader. I am not sure what is the best way to handle
> > > > > > this situation, suggestions are welcomed.
> > > > > >
> > > > > > Patch 14 add the dt-bindings for the PCF2131.
> > > > > >
> > > > > > I have tested the driver using a PCF2131-ARD evaluation board connected to
> > > > > > an NXP imx8mp evaluation board:
> > > > > > - Time get/set ok;
> > > > > > - Alarms get/set ok
> > > > > > - Timestamp 1 to 4 ok
> > > > > > - IRQ alarm ok
> > > > > > - Watchdog ok
> > > > > > - Also tested successfully with "RTC Driver Test Example" from
> > > > > > Documentation/rtc.txt
> > > > > >
> > > > > > I have also tested the driver on a custom PCF2129 adapter board connected to a
> > > > > > beaglebone black.
> > > > > >
> > > > > > Thank you.
> > > > > >
> > > > > > Link: [v1] https://patchwork.ozlabs.org/project/rtc-linux/patch/[email protected]/
> > > > > > Link: [v2] https://patchwork.ozlabs.org/project/rtc-linux/list/?series=285734
> > > > > >
> > > > > > Changes for V3:
> > > > > > - Rebased for kernel v6.1
> > > > > >
> > > > > > Changes for V2:
> > > > > > - In general, fix and improvements after I have tested on real hardware
> > > > > > - Fix alarm interrupt A/B mask setting for PCF2131:
> > > > > > PCF2131_BIT_INT_AIE must be cleared, not set, to enable interrupt.
> > > > > > - Remove low_reg validation: only check if TS interrupt flag is
> > > > > > defined, as low_reg is defined at address 0 for PCF2127/29.
> > > > > > - Change PWRMNG value for PCF2131: default is different than PCF2127/29.
> > > > > > - Adapt time/date registers write sequence for PCF2131 (STOP and CPR bits).
> > > > > > - Map all interrupt sources to INT A pin
> > > > > > - Read and validate PCF2131 device presence from RESET register
> > > > > > - Adapt watchdog configuration for PCF2131
> > > > > >
> > > > > > Hugo Villeneuve (14):
> > > > > > rtc: pcf2127: add variant-specific configuration structure
> > > > > > rtc: pcf2127: adapt for time/date registers at any offset
> > > > > > rtc: pcf2127: adapt for alarm registers at any offset
> > > > > > rtc: pcf2127: adapt for WD registers at any offset
> > > > > > rtc: pcf2127: adapt for CLKOUT register at any offset
> > > > > > rtc: pcf2127: add support for multiple TS functions
> > > > > > rtc: pcf2127: add support for PCF2131 RTC
> > > > > > rtc: pcf2127: add support for PCF2131 interrupts on output INT_A
> > > > > > rtc: pcf2127: set PWRMNG value for PCF2131
> > > > > > rtc: pcf2127: read and validate PCF2131 device signature
> > > > > > rtc: pcf2127: adapt time/date registers write sequence for PCF2131
> > > > > > rtc: pcf2127: support generic watchdog timing configuration
> > > > > > rtc: pcf2127: add flag for watchdog register value read support
> > > > > > dt-bindings: rtc: pcf2127: add PCF2131
> > > > > >
> > > > > > .../devicetree/bindings/rtc/nxp,pcf2127.yaml | 4 +-
> > > > > > drivers/rtc/Kconfig | 4 +-
> > > > > > drivers/rtc/rtc-pcf2127.c | 939 ++++++++++++++----
> > > > > > 3 files changed, 752 insertions(+), 195 deletions(-)
> > > > > >
> > > > > > --
> > > > > > 2.30.2
> > > > > >
> > > > >
> > > > > --
> > > > > Alexandre Belloni, co-owner and COO, Bootlin
> > > > > Embedded Linux and Kernel engineering
> > > > > https://bootlin.com
> > > > >
> >
> > --
> > Alexandre Belloni, co-owner and COO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> >

2023-07-07 15:14:04

by Alexandre Belloni

Subject: Re: [PATCH v3 00/14] rtc: pcf2127: add PCF2131 driver

Hello,

On 05/07/2023 09:40:12-0400, Hugo Villeneuve wrote:
> On Wed, 21 Jun 2023 14:28:52 -0400
> Hugo Villeneuve <[email protected]> wrote:
>
> > On Wed, 21 Jun 2023 20:14:41 +0200
> > Alexandre Belloni <[email protected]> wrote:
> >
> > > On 21/06/2023 12:59:45-0400, Hugo Villeneuve wrote:
> > > > On Wed, 21 Jun 2023 10:14:29 -0400
> > > > Hugo Villeneuve <[email protected]> wrote:
> > > >
> > > > > On Fri, 20 Jan 2023 20:05:07 +0100
> > > > > Alexandre Belloni <[email protected]> wrote:
> > > > >
> > > > > > Hello,
> > > > > >
> > > > > > I know I've been holding off on the review of this series for a while
> > > > > > and I'm sorry for that.
> > > > > >
> > > > > > One of the main issue that is remaining is that the driver ends up being
> > > > > > 53% bigger and generaly less efficient for no added functionality for
> > > > > > the existing RTCs.
> > > > > >
> > > > > > I know performance is not a concern however, having more code in the
> > > > > > set/read time and irq paths means that it is more difficult to set an
> > > > > > get the time precisely.
> > > > >
> > > > > Hi Alexandre,
> > > > > one way to keep rtc_read_time() as efficient as before, and even more
> > > > > efficient by reading 7 instead of 10 registers, would be to drop reading
> > > > > the CTRL3 register, which is only used to detect and display an info
> > > > > message for the low battery condition. This low battery check could be
> > > > > moved to an ioctl call, like it is done in the PCF8523 driver.
> > > > >
> > > > > Hugo.
> > > >
> > > > Hi,
> > > > in fact it is already part of the ioctl, so it is even simpler...
> > > >
> > >
> > > Yes, the dev_info can be removed.
> >
> > Hi,
> > great, I will integrate that patch to improve rtc_read_time()
> > performance, and resubmit V4 soon with the requested changes mentioned
> > during V3 review.
> >
> > Thank you, Hugo.
>
> Hi Alexandre,
> I submitted V4 a few days ago, please let me know if everything is
> in order and all comments properly addressed.
>
> If all is good, any chance we can have that integrated into v6.5?
>

I've seen v4, I'll review more this week end but it looks good. However,
I have to wait for 6.5-rc1 to apply it. This means it won't be in before v6.6.

> Thank you, Hugo.
>
>
> > > > > > I guess I'll take it as a merged driver but I took a different decision
> > > > > > for other RTCs.
> > > > > >
> > > > > > On 15/12/2022 10:02:01-0500, Hugo Villeneuve wrote:
> > > > > > > From: Hugo Villeneuve <[email protected]>
> > > > > > >
> > > > > > > Hello,
> > > > > > > this patch series adds the driver for the PCF2131 real-time clock.
> > > > > > >
> > > > > > > This RTC is very similar in functionality to the PCF2127/29 with the
> > > > > > > following differences:
> > > > > > > -supports two new control registers at offsets 4 and 5
> > > > > > > -supports a new reset register
> > > > > > > -supports 4 tamper detection functions instead of 1
> > > > > > > -has no nvmem (like the PCF2129)
> > > > > > > -has two output interrupt pins instead of one
> > > > > > > -has 1/100th seconds capabilities (not supported in this driver)
> > > > > > > -pcf2127 has watchdog clock sources: 1/60, 1, 64 and 4096Hz
> > > > > > > pcf2131 has watchdog clock sources: 1/64, 1/4, 4 and 64Hz
> > > > > > > -watchdog value register cannot be read after being set
> > > > > > >
> > > > > > > Most of the register addresses are very different, although they still
> > > > > > > follow the same layout. For example, the time/date and tamper registers
> > > > > > > have a different base address, but the offsets are all the same.
> > > > > > > Consequently, the source code of the PCF2127 driver can be easily adapted
> > > > > > > to support this new device.
> > > > > > >
> > > > > > > Patches 1 to 6 modify the existing pcf2127 driver to make it more generic
> > > > > > > and able to support multiple variants, like the PCF2131. This is done
> > > > > > > mostly by using offsets instead of absolute hardcoded register addresses.
> > > > > > >
> > > > > > > Patch 7 add actual support for the PCF2131.
> > > > > > >
> > > > > > > Patch 8 configures all interrupt sources to go through the INT A pin.
> > > > > > >
> > > > > > > Patch 9 changes the PWRMNG bits to be the same with the PCF2131 as they
> > > > > > > are with the PCF2127/29 (different default values).
> > > > > > >
> > > > > > > Patch 10 allow to confirm PCF2131 device presence by reading the reset
> > > > > > > register fixed pattern.
> > > > > > >
> > > > > > > Patch 11 adapt the time/date registers write sequence for PCF2131 (STOP and
> > > > > > > CPR bits).
> > > > > > >
> > > > > > > Patch 12 add support for generic watchdog timing configuration.
> > > > > > >
> > > > > > > Patch 13 add a new flag to identify if device has read support for reading
> > > > > > > watchdog register value.
> > > > > > > Since the watchdog value register cannot be read on the PCF2131 after
> > > > > > > being set, it seems that we cannot detect if watchdog timer was
> > > > > > > started by bootloader. I am not sure what is the best way to handle
> > > > > > > this situation, suggestions are welcomed.
> > > > > > >
> > > > > > > Patch 14 add the dt-bindings for the PCF2131.
> > > > > > >
> > > > > > > I have tested the driver using a PCF2131-ARD evaluation board connected to
> > > > > > > an NXP imx8mp evaluation board:
> > > > > > > - Time get/set ok;
> > > > > > > - Alarms get/set ok
> > > > > > > - Timestamp 1 to 4 ok
> > > > > > > - IRQ alarm ok
> > > > > > > - Watchdog ok
> > > > > > > - Also tested successfully with "RTC Driver Test Example" from
> > > > > > > Documentation/rtc.txt
> > > > > > >
> > > > > > > I have also tested the driver on a custom PCF2129 adapter board connected to a
> > > > > > > beaglebone black.
> > > > > > >
> > > > > > > Thank you.
> > > > > > >
> > > > > > > Link: [v1] https://patchwork.ozlabs.org/project/rtc-linux/patch/[email protected]/
> > > > > > > Link: [v2] https://patchwork.ozlabs.org/project/rtc-linux/list/?series=285734
> > > > > > >
> > > > > > > Changes for V3:
> > > > > > > - Rebased for kernel v6.1
> > > > > > >
> > > > > > > Changes for V2:
> > > > > > > - In general, fix and improvements after I have tested on real hardware
> > > > > > > - Fix alarm interrupt A/B mask setting for PCF2131:
> > > > > > > PCF2131_BIT_INT_AIE must be cleared, not set, to enable interrupt.
> > > > > > > - Remove low_reg validation: only check if TS interrupt flag is
> > > > > > > defined, as low_reg is defined at address 0 for PCF2127/29.
> > > > > > > - Change PWRMNG value for PCF2131: default is different than PCF2127/29.
> > > > > > > - Adapt time/date registers write sequence for PCF2131 (STOP and CPR bits).
> > > > > > > - Map all interrupt sources to INT A pin
> > > > > > > - Read and validate PCF2131 device presence from RESET register
> > > > > > > - Adapt watchdog configuration for PCF2131
> > > > > > >
> > > > > > > Hugo Villeneuve (14):
> > > > > > > rtc: pcf2127: add variant-specific configuration structure
> > > > > > > rtc: pcf2127: adapt for time/date registers at any offset
> > > > > > > rtc: pcf2127: adapt for alarm registers at any offset
> > > > > > > rtc: pcf2127: adapt for WD registers at any offset
> > > > > > > rtc: pcf2127: adapt for CLKOUT register at any offset
> > > > > > > rtc: pcf2127: add support for multiple TS functions
> > > > > > > rtc: pcf2127: add support for PCF2131 RTC
> > > > > > > rtc: pcf2127: add support for PCF2131 interrupts on output INT_A
> > > > > > > rtc: pcf2127: set PWRMNG value for PCF2131
> > > > > > > rtc: pcf2127: read and validate PCF2131 device signature
> > > > > > > rtc: pcf2127: adapt time/date registers write sequence for PCF2131
> > > > > > > rtc: pcf2127: support generic watchdog timing configuration
> > > > > > > rtc: pcf2127: add flag for watchdog register value read support
> > > > > > > dt-bindings: rtc: pcf2127: add PCF2131
> > > > > > >
> > > > > > > .../devicetree/bindings/rtc/nxp,pcf2127.yaml | 4 +-
> > > > > > > drivers/rtc/Kconfig | 4 +-
> > > > > > > drivers/rtc/rtc-pcf2127.c | 939 ++++++++++++++----
> > > > > > > 3 files changed, 752 insertions(+), 195 deletions(-)
> > > > > > >
> > > > > > > --
> > > > > > > 2.30.2
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > Alexandre Belloni, co-owner and COO, Bootlin
> > > > > > Embedded Linux and Kernel engineering
> > > > > > https://bootlin.com
> > > > > >
> > >
> > > --
> > > Alexandre Belloni, co-owner and COO, Bootlin
> > > Embedded Linux and Kernel engineering
> > > https://bootlin.com
> > >

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