[PATCH 0/5] rtc: rv8803 patches

[PATCH 0/5] rtc: rv8803 patches

[Sascha Hauer]

This series has some patches for the rx8803 RTC handled by the rv8803
driver. These patches mostly improve the voltage loss handling in the
rx8803. While the rv8803 resets its registers on voltage loss the rx8803
leaves registers in an undefined state after voltage loss. The patches
are used here for a customer for quite a while, it's time to upstream
them.

Sascha

Ahmad Fatoum (5):
  rtc: rv8803: factor out existing register initialization to function
  rtc: rv8803: initialize registers on post-probe voltage loss
  rtc: rv8803: re-initialize all Epson RX8803 registers on voltage loss
  include/linux/bcd.h: provide bcd_is_valid() helper
  rtc: rv8803: invalidate date/time if alarm time is invalid

 drivers/rtc/rtc-rv8803.c | 134 ++++++++++++++++++++++++++++++++-------
 include/linux/bcd.h      |   4 ++
 2 files changed, 116 insertions(+), 22 deletions(-)

-- 
2.30.2

[PATCH 1/5] rtc: rv8803: factor out existing register initialization to function

[Sascha Hauer]

From: Ahmad Fatoum <a.fatoum@pengutronix.de>

The driver probe currently initializes some registers to non-POR
values. These values are not reinstated if the RTC experiences voltage
loss later on. Prepare for fixing this by factoring out the
initialization to a separate function.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/rtc/rtc-rv8803.c | 43 +++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c
index f69e0b1137cd0..c880f8d6c7423 100644
--- a/drivers/rtc/rtc-rv8803.c
+++ b/drivers/rtc/rtc-rv8803.c
@@ -64,6 +64,7 @@ struct rv8803_data {
 	struct rtc_device *rtc;
 	struct mutex flags_lock;
 	u8 ctrl;
+	u8 backup;
 	enum rv8803_type type;
 };
 
@@ -498,18 +499,32 @@ static int rx8900_trickle_charger_init(struct rv8803_data *rv8803)
 	if (err < 0)
 		return err;
 
-	flags = ~(RX8900_FLAG_VDETOFF | RX8900_FLAG_SWOFF) & (u8)err;
-
-	if (of_property_read_bool(node, "epson,vdet-disable"))
-		flags |= RX8900_FLAG_VDETOFF;
-
-	if (of_property_read_bool(node, "trickle-diode-disable"))
-		flags |= RX8900_FLAG_SWOFF;
+	flags = (u8)err;
+	flags &= ~(RX8900_FLAG_VDETOFF | RX8900_FLAG_SWOFF);
+	flags |= rv8803->backup;
 
 	return i2c_smbus_write_byte_data(rv8803->client, RX8900_BACKUP_CTRL,
 					 flags);
 }
 
+/* configure registers with values different than the Power-On reset defaults */
+static int rv8803_regs_configure(struct rv8803_data *rv8803)
+{
+	int err;
+
+	err = rv8803_write_reg(rv8803->client, RV8803_EXT, RV8803_EXT_WADA);
+	if (err)
+		return err;
+
+	err = rx8900_trickle_charger_init(rv8803);
+	if (err) {
+		dev_err(&rv8803->client->dev, "failed to init charger\n");
+		return err;
+	}
+
+	return 0;
+}
+
 static int rv8803_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -576,15 +591,15 @@ static int rv8803_probe(struct i2c_client *client,
 	if (!client->irq)
 		clear_bit(RTC_FEATURE_ALARM, rv8803->rtc->features);
 
-	err = rv8803_write_reg(rv8803->client, RV8803_EXT, RV8803_EXT_WADA);
-	if (err)
-		return err;
+	if (of_property_read_bool(client->dev.of_node, "epson,vdet-disable"))
+		rv8803->backup |= RX8900_FLAG_VDETOFF;
 
-	err = rx8900_trickle_charger_init(rv8803);
-	if (err) {
-		dev_err(&client->dev, "failed to init charger\n");
+	if (of_property_read_bool(client->dev.of_node, "trickle-diode-disable"))
+		rv8803->backup |= RX8900_FLAG_SWOFF;
+
+	err = rv8803_regs_configure(rv8803);
+	if (err)
 		return err;
-	}
 
 	rv8803->rtc->ops = &rv8803_rtc_ops;
 	rv8803->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
-- 
2.30.2

[PATCH 2/5] rtc: rv8803: initialize registers on post-probe voltage loss

[Sascha Hauer]

From: Ahmad Fatoum <a.fatoum@pengutronix.de>

The driver probe currently initializes some registers to non-POR
values. These values are not reinstated if the RTC experiences voltage
loss later on. Fix this.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/rtc/rtc-rv8803.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c
index c880f8d6c7423..21a6f1eddb092 100644
--- a/drivers/rtc/rtc-rv8803.c
+++ b/drivers/rtc/rtc-rv8803.c
@@ -137,6 +137,13 @@ static int rv8803_write_regs(const struct i2c_client *client,
 	return ret;
 }
 
+static int rv8803_regs_configure(struct rv8803_data *rv8803);
+
+static int rv8803_regs_reset(struct rv8803_data *rv8803)
+{
+	return rv8803_regs_configure(rv8803);
+}
+
 static irqreturn_t rv8803_handle_irq(int irq, void *dev_id)
 {
 	struct i2c_client *client = dev_id;
@@ -270,6 +277,12 @@ static int rv8803_set_time(struct device *dev, struct rtc_time *tm)
 		return flags;
 	}
 
+	if (flags & RV8803_FLAG_V2F) {
+		ret = rv8803_regs_reset(rv8803);
+		if (ret)
+			return ret;
+	}
+
 	ret = rv8803_write_reg(rv8803->client, RV8803_FLAG,
 			       flags & ~(RV8803_FLAG_V1F | RV8803_FLAG_V2F));
 
-- 
2.30.2

[PATCH 3/5] rtc: rv8803: re-initialize all Epson RX8803 registers on voltage loss

[Sascha Hauer]

From: Ahmad Fatoum <a.fatoum@pengutronix.de>

The reference manuals of both the RX8803 and RV8803 dictate that
"[On V2F/VLF = ] all registers must be initialized".

The RV-8803 application manual (rev. 1.6) further specifies that crossing
V_LOW2 threshold enables flag V2F and triggers a Power-On reset.
According to table 3.11 in the document, all control registers are
defined to sensible values.

However, The Epson RX-8803 doesn't offer the same guarantees.
It explicitly states:

  During the initial power-up, the TEST bit is reset to "0" and the VLF
  bit is set to "1".
  ∗ At this point, all other register values are _undefined_, so be sure to
  perform a reset before using the module.

Commit d3700b6b6479 ("rtc: rv8803: Stop the clock while setting the time")
also had this rationale:

  Indeed, all the registers must be initialized if the voltage has been
  lower than VLOW2 (triggering V2F), but not low enough to trigger a POR.

We should follow the advice and initialize all applicable registers.
We can group the registers into 3 groups:

A) Already correctly handled registers:
  * 0B-0Ch | Timer Counter  | unused and disabled by clearing TE in 0Dh
  * 0Dh    | Extension Reg  | already initialized in rv8803_regs_configure
  * 0Eh    | Flag Reg       | handled in IRQ handler, except for VLF, VDET
  * 0Eh    | VLF, VDET      | cleared in ->set_time
  * 10h    | 100th Seconds  | Already reset via RESET bit
  * 20-21h | Capture Buffer | holds timestamp unused by driver
  * 2Fh    | Event Control  | resets automatically

B) Registers that are hardware initialized on POR, but not on VLF:
  * 0Fh    | Control Reg
  * 2Ch    | OSC Offset

C) RAM that is undefined on voltage loss:
  * 00-06h | Date/Time
  * 07h    | RAM
  * 08-0Ah | Alarm

This means we should initialize after VLF the registers in group B
(RV8803_CTRL and RV8803_OSC_OFFSET).

Group C is all-zero after voltage loss on the RV-8803, but undefined on
the RX-8803. This is ok for Date/Time because ->get_time returns an
error code for as long as the voltage loss flag is active. It's cleared
on ->set_time however. Zeroing both RAM and alarm ensures a fixed value
is read afterwards.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/rtc/rtc-rv8803.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c
index 21a6f1eddb092..fe1247e771b98 100644
--- a/drivers/rtc/rtc-rv8803.c
+++ b/drivers/rtc/rtc-rv8803.c
@@ -9,6 +9,7 @@
 
 #include <linux/bcd.h>
 #include <linux/bitops.h>
+#include <linux/bitfield.h>
 #include <linux/log2.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -33,6 +34,7 @@
 #define RV8803_EXT			0x0D
 #define RV8803_FLAG			0x0E
 #define RV8803_CTRL			0x0F
+#define RV8803_OSC_OFFSET		0x2C
 
 #define RV8803_EXT_WADA			BIT(6)
 
@@ -49,12 +51,15 @@
 #define RV8803_CTRL_TIE			BIT(4)
 #define RV8803_CTRL_UIE			BIT(5)
 
+#define RX8803_CTRL_CSEL		GENMASK(7, 6)
+
 #define RX8900_BACKUP_CTRL		0x18
 #define RX8900_FLAG_SWOFF		BIT(2)
 #define RX8900_FLAG_VDETOFF		BIT(3)
 
 enum rv8803_type {
 	rv_8803,
+	rx_8803,
 	rx_8804,
 	rx_8900
 };
@@ -137,10 +142,41 @@ static int rv8803_write_regs(const struct i2c_client *client,
 	return ret;
 }
 
+static int rv8803_regs_init(struct rv8803_data *rv8803)
+{
+	int ret;
+
+	ret = rv8803_write_reg(rv8803->client, RV8803_OSC_OFFSET, 0x00);
+	if (ret)
+		return ret;
+
+	ret = rv8803_write_reg(rv8803->client, RV8803_CTRL,
+			       FIELD_PREP(RX8803_CTRL_CSEL, 1)); /* 2s */
+	if (ret)
+		return ret;
+
+	ret = rv8803_write_regs(rv8803->client, RV8803_ALARM_MIN, 3,
+				(u8[]){ 0, 0, 0 });
+	if (ret)
+		return ret;
+
+	return rv8803_write_reg(rv8803->client, RV8803_RAM, 0x00);
+}
+
 static int rv8803_regs_configure(struct rv8803_data *rv8803);
 
 static int rv8803_regs_reset(struct rv8803_data *rv8803)
 {
+	/*
+	 * The RV-8803 resets all registers to POR defaults after voltage-loss,
+	 * the Epson RTCs don't, so we manually reset the remainder here.
+	 */
+	if (rv8803->type == rx_8803 || rv8803->type == rx_8900) {
+		int ret = rv8803_regs_init(rv8803);
+		if (ret)
+			return ret;
+	}
+
 	return rv8803_regs_configure(rv8803);
 }
 
@@ -631,7 +667,7 @@ static int rv8803_probe(struct i2c_client *client,
 static const struct i2c_device_id rv8803_id[] = {
 	{ "rv8803", rv_8803 },
 	{ "rv8804", rx_8804 },
-	{ "rx8803", rv_8803 },
+	{ "rx8803", rx_8803 },
 	{ "rx8900", rx_8900 },
 	{ }
 };
@@ -644,7 +680,7 @@ static const __maybe_unused struct of_device_id rv8803_of_match[] = {
 	},
 	{
 		.compatible = "epson,rx8803",
-		.data = (void *)rv_8803
+		.data = (void *)rx_8803
 	},
 	{
 		.compatible = "epson,rx8804",
-- 
2.30.2

[PATCH 4/5] include/linux/bcd.h: provide bcd_is_valid() helper

[Sascha Hauer]

From: Ahmad Fatoum <a.fatoum@pengutronix.de>

bcd2bin(0x0A) happily returns 10, despite this being an invalid BCD
value. RTC drivers converting possibly corrupted BCD timestamps might
want to validate their input before calling bcd2bin().

Provide a macro to do so. Unlike bcd2bin and bin2bcd, out-of-line
versions are not implemented. Should the macro experience enough use,
this can be retrofitted.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 include/linux/bcd.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/bcd.h b/include/linux/bcd.h
index 118bea36d7d49..abbc8149178e6 100644
--- a/include/linux/bcd.h
+++ b/include/linux/bcd.h
@@ -14,8 +14,12 @@
 		const_bin2bcd(x) :			\
 		_bin2bcd(x))
 
+#define bcd_is_valid(x)					\
+		const_bcd_is_valid(x)
+
 #define const_bcd2bin(x)	(((x) & 0x0f) + ((x) >> 4) * 10)
 #define const_bin2bcd(x)	((((x) / 10) << 4) + (x) % 10)
+#define const_bcd_is_valid(x)	(((x) & 0x0f) < 10 && ((x) >> 4) < 10)
 
 unsigned _bcd2bin(unsigned char val) __attribute_const__;
 unsigned char _bin2bcd(unsigned val) __attribute_const__;
-- 
2.30.2

[PATCH 5/5] rtc: rv8803: invalidate date/time if alarm time is invalid

[Sascha Hauer]

From: Ahmad Fatoum <a.fatoum@pengutronix.de>

RTC core never calls rv8803_set_alarm with an invalid alarm time,
so if an invalid alarm time > 0 is set, external factors must have
corrupted the RTC's alarm time and possibly other registers.

Play it safe by marking the date/time invalid, so all registers are
reinitialized on a ->set_time.

This may cause existing setups to lose time if they so far set only
date/time, but ignored that the alarm registers had an invalid date
value, e.g.:

  rtc rtc0: invalid alarm value: 2020-3-27 7:82:0

These systems will have their ->get_time return -EINVAL till
->set_time initializes the alarm value (and sets a new time).

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/rtc/rtc-rv8803.c | 46 +++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c
index fe1247e771b98..036c449cf1c20 100644
--- a/drivers/rtc/rtc-rv8803.c
+++ b/drivers/rtc/rtc-rv8803.c
@@ -70,6 +70,7 @@ struct rv8803_data {
 	struct mutex flags_lock;
 	u8 ctrl;
 	u8 backup;
+	u8 alarm_invalid:1;
 	enum rv8803_type type;
 };
 
@@ -165,13 +166,13 @@ static int rv8803_regs_init(struct rv8803_data *rv8803)
 
 static int rv8803_regs_configure(struct rv8803_data *rv8803);
 
-static int rv8803_regs_reset(struct rv8803_data *rv8803)
+static int rv8803_regs_reset(struct rv8803_data *rv8803, bool full)
 {
 	/*
 	 * The RV-8803 resets all registers to POR defaults after voltage-loss,
 	 * the Epson RTCs don't, so we manually reset the remainder here.
 	 */
-	if (rv8803->type == rx_8803 || rv8803->type == rx_8900) {
+	if (full || rv8803->type == rx_8803 || rv8803->type == rx_8900) {
 		int ret = rv8803_regs_init(rv8803);
 		if (ret)
 			return ret;
@@ -238,6 +239,11 @@ static int rv8803_get_time(struct device *dev, struct rtc_time *tm)
 	u8 *date = date1;
 	int ret, flags;
 
+	if (rv8803->alarm_invalid) {
+		dev_warn(dev, "Corruption detected, data may be invalid.\n");
+		return -EINVAL;
+	}
+
 	flags = rv8803_read_reg(rv8803->client, RV8803_FLAG);
 	if (flags < 0)
 		return flags;
@@ -313,10 +319,16 @@ static int rv8803_set_time(struct device *dev, struct rtc_time *tm)
 		return flags;
 	}
 
-	if (flags & RV8803_FLAG_V2F) {
-		ret = rv8803_regs_reset(rv8803);
+	if ((flags & RV8803_FLAG_V2F) || rv8803->alarm_invalid) {
+		/*
+		 * If we sense corruption in the alarm registers, but see no
+		 * voltage loss flag, we can't rely on other registers having
+		 * sensible values. Reset them fully.
+		 */
+		ret = rv8803_regs_reset(rv8803, rv8803->alarm_invalid);
 		if (ret)
 			return ret;
+		rv8803->alarm_invalid = false;
 	}
 
 	ret = rv8803_write_reg(rv8803->client, RV8803_FLAG,
@@ -342,13 +354,27 @@ static int rv8803_get_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	if (flags < 0)
 		return flags;
 
-	alrm->time.tm_sec  = 0;
-	alrm->time.tm_min  = bcd2bin(alarmvals[0] & 0x7f);
-	alrm->time.tm_hour = bcd2bin(alarmvals[1] & 0x3f);
-	alrm->time.tm_mday = bcd2bin(alarmvals[2] & 0x3f);
+	alarmvals[0] &= 0x7f;
+	alarmvals[1] &= 0x3f;
+	alarmvals[2] &= 0x3f;
+
+	if (bcd_is_valid(alarmvals[0]) && bcd_is_valid(alarmvals[1]) &&
+	    bcd_is_valid(alarmvals[2])) {
+		alrm->time.tm_sec  = 0;
+		alrm->time.tm_min  = bcd2bin(alarmvals[0]);
+		alrm->time.tm_hour = bcd2bin(alarmvals[1]);
+		alrm->time.tm_mday = bcd2bin(alarmvals[2]);
 
-	alrm->enabled = !!(rv8803->ctrl & RV8803_CTRL_AIE);
-	alrm->pending = (flags & RV8803_FLAG_AF) && alrm->enabled;
+		alrm->enabled = !!(rv8803->ctrl & RV8803_CTRL_AIE);
+		alrm->pending = (flags & RV8803_FLAG_AF) && alrm->enabled;
+	}
+
+	if ((unsigned)alrm->time.tm_mday > 31 ||
+	    (unsigned)alrm->time.tm_hour >= 24 ||
+	    (unsigned)alrm->time.tm_min >= 60) {
+		rv8803->alarm_invalid = true;
+		return -EINVAL;
+	}
 
 	return 0;
 }
-- 
2.30.2

Re: [PATCH 0/5] rtc: rv8803 patches

[Sascha Hauer]

Alessandro, Alexandre,

Any input to this one?

Thanks,
 Sascha

On Tue, Apr 26, 2022 at 09:10:51AM +0200, Sascha Hauer wrote:
> This series has some patches for the rx8803 RTC handled by the rv8803
> driver. These patches mostly improve the voltage loss handling in the
> rx8803. While the rv8803 resets its registers on voltage loss the rx8803
> leaves registers in an undefined state after voltage loss. The patches
> are used here for a customer for quite a while, it's time to upstream
> them.
> 
> Sascha
> 
> Ahmad Fatoum (5):
>   rtc: rv8803: factor out existing register initialization to function
>   rtc: rv8803: initialize registers on post-probe voltage loss
>   rtc: rv8803: re-initialize all Epson RX8803 registers on voltage loss
>   include/linux/bcd.h: provide bcd_is_valid() helper
>   rtc: rv8803: invalidate date/time if alarm time is invalid
> 
>  drivers/rtc/rtc-rv8803.c | 134 ++++++++++++++++++++++++++++++++-------
>  include/linux/bcd.h      |   4 ++
>  2 files changed, 116 insertions(+), 22 deletions(-)
> 
> -- 
> 2.30.2
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 5/5] rtc: rv8803: invalidate date/time if alarm time is invalid

[Alexandre Belloni]

Hello Sascha,

Sorry for the very late review. I'm mostly fine with the whole series.

On 26/04/2022 09:10:56+0200, Sascha Hauer wrote:
> From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> RTC core never calls rv8803_set_alarm with an invalid alarm time,
> so if an invalid alarm time > 0 is set, external factors must have
> corrupted the RTC's alarm time and possibly other registers.
> 
> Play it safe by marking the date/time invalid, so all registers are
> reinitialized on a ->set_time.
> 
> This may cause existing setups to lose time if they so far set only
> date/time, but ignored that the alarm registers had an invalid date
> value, e.g.:
> 
>   rtc rtc0: invalid alarm value: 2020-3-27 7:82:0
> 
> These systems will have their ->get_time return -EINVAL till
> ->set_time initializes the alarm value (and sets a new time).
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/rtc/rtc-rv8803.c | 46 +++++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c
> index fe1247e771b98..036c449cf1c20 100644
> --- a/drivers/rtc/rtc-rv8803.c
> +++ b/drivers/rtc/rtc-rv8803.c
> @@ -70,6 +70,7 @@ struct rv8803_data {
>  	struct mutex flags_lock;
>  	u8 ctrl;
>  	u8 backup;
> +	u8 alarm_invalid:1;
>  	enum rv8803_type type;
>  };
>  
> @@ -165,13 +166,13 @@ static int rv8803_regs_init(struct rv8803_data *rv8803)
>  
>  static int rv8803_regs_configure(struct rv8803_data *rv8803);
>  
> -static int rv8803_regs_reset(struct rv8803_data *rv8803)
> +static int rv8803_regs_reset(struct rv8803_data *rv8803, bool full)
>  {
>  	/*
>  	 * The RV-8803 resets all registers to POR defaults after voltage-loss,
>  	 * the Epson RTCs don't, so we manually reset the remainder here.
>  	 */
> -	if (rv8803->type == rx_8803 || rv8803->type == rx_8900) {
> +	if (full || rv8803->type == rx_8803 || rv8803->type == rx_8900) {
>  		int ret = rv8803_regs_init(rv8803);
>  		if (ret)
>  			return ret;
> @@ -238,6 +239,11 @@ static int rv8803_get_time(struct device *dev, struct rtc_time *tm)
>  	u8 *date = date1;
>  	int ret, flags;
>  
> +	if (rv8803->alarm_invalid) {
> +		dev_warn(dev, "Corruption detected, data may be invalid.\n");
> +		return -EINVAL;
> +	}
> +
>  	flags = rv8803_read_reg(rv8803->client, RV8803_FLAG);
>  	if (flags < 0)
>  		return flags;
> @@ -313,10 +319,16 @@ static int rv8803_set_time(struct device *dev, struct rtc_time *tm)
>  		return flags;
>  	}
>  
> -	if (flags & RV8803_FLAG_V2F) {
> -		ret = rv8803_regs_reset(rv8803);
> +	if ((flags & RV8803_FLAG_V2F) || rv8803->alarm_invalid) {
> +		/*
> +		 * If we sense corruption in the alarm registers, but see no
> +		 * voltage loss flag, we can't rely on other registers having
> +		 * sensible values. Reset them fully.
> +		 */
> +		ret = rv8803_regs_reset(rv8803, rv8803->alarm_invalid);
>  		if (ret)
>  			return ret;
> +		rv8803->alarm_invalid = false;
>  	}
>  
>  	ret = rv8803_write_reg(rv8803->client, RV8803_FLAG,
> @@ -342,13 +354,27 @@ static int rv8803_get_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	if (flags < 0)
>  		return flags;
>  
> -	alrm->time.tm_sec  = 0;
> -	alrm->time.tm_min  = bcd2bin(alarmvals[0] & 0x7f);
> -	alrm->time.tm_hour = bcd2bin(alarmvals[1] & 0x3f);
> -	alrm->time.tm_mday = bcd2bin(alarmvals[2] & 0x3f);
> +	alarmvals[0] &= 0x7f;
> +	alarmvals[1] &= 0x3f;
> +	alarmvals[2] &= 0x3f;
> +
> +	if (bcd_is_valid(alarmvals[0]) && bcd_is_valid(alarmvals[1]) &&
> +	    bcd_is_valid(alarmvals[2])) {
> +		alrm->time.tm_sec  = 0;
> +		alrm->time.tm_min  = bcd2bin(alarmvals[0]);
> +		alrm->time.tm_hour = bcd2bin(alarmvals[1]);
> +		alrm->time.tm_mday = bcd2bin(alarmvals[2]);
>  
> -	alrm->enabled = !!(rv8803->ctrl & RV8803_CTRL_AIE);
> -	alrm->pending = (flags & RV8803_FLAG_AF) && alrm->enabled;
> +		alrm->enabled = !!(rv8803->ctrl & RV8803_CTRL_AIE);
> +		alrm->pending = (flags & RV8803_FLAG_AF) && alrm->enabled;
> +	}
> +
> +	if ((unsigned)alrm->time.tm_mday > 31 ||

You'd need to use (unsigned int) here, else I'll get follow up patches
doing the conversion. My only issue is actually this check as it would
be better to use rtc_valid_tm that checks tm_month but obviously, your
rtc_tm is missing information at this time and this would require to
replicate what is done in __rtc_read_alarm. I don't have a great
solution to that though

> +	    (unsigned)alrm->time.tm_hour >= 24 ||
> +	    (unsigned)alrm->time.tm_min >= 60) {
> +		rv8803->alarm_invalid = true;
> +		return -EINVAL;
> +	}


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

Re: (subset) [PATCH 0/5] rtc: rv8803 patches

[Alexandre Belloni]

On Tue, 26 Apr 2022 09:10:51 +0200, Sascha Hauer wrote:
> This series has some patches for the rx8803 RTC handled by the rv8803
> driver. These patches mostly improve the voltage loss handling in the
> rx8803. While the rv8803 resets its registers on voltage loss the rx8803
> leaves registers in an undefined state after voltage loss. The patches
> are used here for a customer for quite a while, it's time to upstream
> them.
> 
> [...]

Applied, thanks!

[1/5] rtc: rv8803: factor out existing register initialization to function
      commit: 924e4892b167eb8adbcb2399d2b26f7667ff266d
[2/5] rtc: rv8803: initialize registers on post-probe voltage loss
      commit: 7f1a1106537217edbdc68781e95b356df8463559
[3/5] rtc: rv8803: re-initialize all Epson RX8803 registers on voltage loss
      commit: 084dc7c0072a7f93a947824d2b69883ed66a657f

Best regards,

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