[PATCH v2 0/3] rtc: ds1307: Add support for Epson RX8901CE

[PATCH v2 0/3] rtc: ds1307: Add support for Epson RX8901CE

[Fredrik M Olsson]

Add basic support for the Epson RX8901CE RTC.
Datasheet: https://download.epsondevice.com/td/pdf/app/RX8901CE_en.pdf

Also includes a bug fix for an issue with reading the weekday from the
RTC which affects both the existing rx8130 and this rx8901 driver.

Signed-off-by: Fredrik M Olsson <fredrik.m.olsson@axis.com>
---
Changes in v2:
- Squashed the Voltage Low status patch into the driver patch
- Switch from dev_err to dev_dbg
- Implement Backup Switch Mode (BSM) ioctl instead of hard coding
  switching mode.
- Change the prototype for the do_trickle_setup function pointer in
  order to make it possible to propagate error codes from the added
  PWSW_CFG register read operation. This read operation is added so not
  to override previously set register fields.
- Link to v1: https://lore.kernel.org/r/20251219-ds1307-rx8901-add-v1-0-b13f346ebe93@axis.com

---
Fredrik M Olsson (3):
      dt-bindings: rtc: ds1307: Add epson,rx8901
      rtc: ds1307: Fix off-by-one issue with wday for rx8130
      rtc: ds1307: Add driver for Epson RX8901CE

 .../devicetree/bindings/rtc/rtc-ds1307.yaml        |   1 +
 drivers/rtc/rtc-ds1307.c                           | 218 ++++++++++++++++++++-
 2 files changed, 209 insertions(+), 10 deletions(-)
---
base-commit: 27fa82620cbaa89a7fc11ac3057701d598813e87
change-id: 20251126-ds1307-rx8901-add-a0fe173093e3

Best regards,
-- 
Fredrik M Olsson <fredrik.m.olsson@axis.com>

[PATCH v2 1/3] dt-bindings: rtc: ds1307: Add epson,rx8901

[Fredrik M Olsson]

Add compatible string epson,rx8901 for the Epson RX8901CE RTC.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@mail.toshiba>
Signed-off-by: Fredrik M Olsson <fredrik.m.olsson@axis.com>
---
 Documentation/devicetree/bindings/rtc/rtc-ds1307.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/rtc/rtc-ds1307.yaml b/Documentation/devicetree/bindings/rtc/rtc-ds1307.yaml
index 98d10e680144..9b2796804f07 100644
--- a/Documentation/devicetree/bindings/rtc/rtc-ds1307.yaml
+++ b/Documentation/devicetree/bindings/rtc/rtc-ds1307.yaml
@@ -31,6 +31,7 @@ properties:
           - epson,rx8025
           - isil,isl12057
           - epson,rx8130
+          - epson,rx8901
 
       - items:
           - enum:

-- 
2.43.0

[PATCH v2 2/3] rtc: ds1307: Fix off-by-one issue with wday for rx8130

[Fredrik M Olsson]

The RTC represent each weekday with a individual bit set in the WDAY
register, where the 0th bit represent the first day of the week and the
6th bit represents the last day of the week. For each passed day the
chip performs a rotary-left-shift by one to advance the weekday by one.

The tm_wday field represent weekdays by a value in the range of 0-6.

The fls() function return the bit index of the last bit set. To handle
when there are no bits set it will return 0, and if the 0th bit is set
it will return 1, and if the 1st bit is set it will return 2, and so on.

In order to make the result of the fls() function fall into the expected
range of 0-6 (instead of 1-7) this patch subtracts one from the result
(which matches how the value is written in ds1307_set_time()).

Fixes: 204756f016726 ("rtc: ds1307: Fix wday settings for rx8130")
Reviewed-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.x90@mail.toshiba>
Signed-off-by: Fredrik M Olsson <fredrik.m.olsson@axis.com>
---
 drivers/rtc/rtc-ds1307.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 7205c59ff729..bf42c250ea7d 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -308,7 +308,7 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 	t->tm_hour = bcd2bin(tmp);
 	/* rx8130 is bit position, not BCD */
 	if (ds1307->type == rx_8130)
-		t->tm_wday = fls(regs[DS1307_REG_WDAY] & 0x7f);
+		t->tm_wday = fls(regs[DS1307_REG_WDAY] & 0x7f) - 1;
 	else
 		t->tm_wday = bcd2bin(regs[DS1307_REG_WDAY] & 0x07) - 1;
 	t->tm_mday = bcd2bin(regs[DS1307_REG_MDAY] & 0x3f);

-- 
2.43.0

Re: [PATCH v2 2/3] rtc: ds1307: Fix off-by-one issue with wday for rx8130

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Undefined Behavior via out-of-bounds shift in ds1307_set_time()
- [High] Hardware configuration corruption in rx8130_set_alarm()
--

commit 3f83bdbd74c118b742ad3d44c6daf3ae8e8ffcbe
Author: Fredrik M Olsson <fredrik.m.olsson@axis.com>

rtc: ds1307: Fix off-by-one issue with wday for rx8130

The RTC represent each weekday with a individual bit set in the WDAY
register, where the 0th bit represent the first day of the week and the
6th bit represents the last day of the week.

> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 7205c59ff7294..bf42c250ea7d1 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -308,7 +308,7 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
>  	t->tm_hour = bcd2bin(tmp);
>  	/* rx8130 is bit position, not BCD */
>  	if (ds1307->type == rx_8130)
> -		t->tm_wday = fls(regs[DS1307_REG_WDAY] & 0x7f);
> +		t->tm_wday = fls(regs[DS1307_REG_WDAY] & 0x7f) - 1;

[Severity: High]
This isn't a bug introduced by this patch, but does this introduce a new
non-malicious trigger for a pre-existing undefined behavior?

The kernel's rtc_valid_tm() omits validation of tm_wday. A malicious userspace
program with CAP_SYS_TIME could already pass a negative value to RTC_SET_TIME,
triggering an out-of-bounds shift in ds1307_set_time():

drivers/rtc/rtc-ds1307.c:ds1307_set_time() {
    ...
	if (ds1307->type == rx_8130)
		regs[DS1307_REG_WDAY] = 1 << t->tm_wday;
    ...
}

With this patch, if the hardware WDAY register is zeroed, fls(0) returns 0,
making t->tm_wday equal to -1. A benign read-modify-write cycle by standard
tools (like hwclock) will now naturally trigger the undefined behavior.

>  	else
>  		t->tm_wday = bcd2bin(regs[DS1307_REG_WDAY] & 0x07) - 1;
>  	t->tm_mday = bcd2bin(regs[DS1307_REG_MDAY] & 0x3f);

[Severity: High]
This is a pre-existing issue, but does rx8130_set_alarm() corrupt the RTC's
hardware configuration?

drivers/rtc/rtc-ds1307.c:rx8130_set_alarm() {
    ...
	/* Read control registers. */
	ret = regmap_bulk_read(ds1307->regmap, RX8130_REG_EXTENSION, ctl,
			       sizeof(ctl));
	if (ret < 0)
		return ret;

	ctl[0] &= RX8130_REG_EXTENSION_WADA;
	ctl[1] &= ~RX8130_REG_FLAG_AF;
	ctl[2] &= ~RX8130_REG_CONTROL0_AIE;
    ...
}

Because RX8130_REG_EXTENSION_WADA is defined as BIT(3), this operation masks
out and clears all other configuration bits in the register (such as timer
enables and frequency selection), while preserving the WADA bit if it was set
instead of clearing it.

Does this break other features every time an alarm is set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-ds1307-rx8901-add-v2-0-e069ea32e1db@axis.com?part=2

[PATCH v2 3/3] rtc: ds1307: Add driver for Epson RX8901CE

[Fredrik M Olsson]

Adds support for:
- Reading and writing time to/from the RTC.
- Changing Backup Switch Mode (BSM) to DISABLED/DIRECT/LEVEL using the
  RTC_PARAM_SET ioctl.
- Optionally enabling battery charging.
- Reading the battery voltage low status using the RTC_VL_READ ioctl
  (only supported in LEVEL BSM), which also reports invalid time
  information if the VLF flag is set.

Signed-off-by: Fredrik M Olsson <fredrik.m.olsson@axis.com>
---
 drivers/rtc/rtc-ds1307.c | 216 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 207 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index bf42c250ea7d..23dd104aa2be 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/bcd.h>
+#include <linux/bitfield.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/kstrtox.h>
@@ -48,6 +49,7 @@ enum ds_type {
 	mcp794xx,
 	rx_8025,
 	rx_8130,
+	rx_8901,
 	last_ds_type /* always last */
 	/* rs5c372 too?  different address... */
 };
@@ -129,6 +131,18 @@ enum ds_type {
 #define RX8130_REG_CONTROL1_INIEN	BIT(4)
 #define RX8130_REG_CONTROL1_CHGEN	BIT(5)
 
+#define RX8901_REG_INTF			0x0e
+#define RX8901_REG_INTF_VLF		BIT(1)
+#define RX8901_REG_PWSW_CFG		0x37
+#define RX8901_REG_PWSW_CFG_SWSEL	GENMASK(3, 2)
+#define RX8901_REG_PWSW_CFG_VBATLDETEN	BIT(4)
+#define RX8901_REG_PWSW_CFG_INIEN	BIT(6)
+#define RX8901_REG_PWSW_CFG_CHGEN	BIT(7)
+#define RX8901_REG_BUF_INTF		0x46
+#define RX8901_REG_BUF_INTF_VBATLF	BIT(3)
+#define RX8901_SWSEL_PRIMARY_BACKUP	0x1
+#define RX8901_SWSEL_PRIMARY		0x2
+
 #define MCP794XX_REG_CONTROL		0x07
 #	define MCP794XX_BIT_ALM0_EN	0x10
 #	define MCP794XX_BIT_ALM1_EN	0x20
@@ -192,8 +206,8 @@ struct chip_desc {
 	irq_handler_t		irq_handler;
 	const struct rtc_class_ops *rtc_ops;
 	u16			trickle_charger_reg;
-	u8			(*do_trickle_setup)(struct ds1307 *, u32,
-						    bool);
+	int			(*do_trickle_setup)(struct ds1307 *ds1307, u32 ohms,
+						    bool diode);
 	/* Does the RTC require trickle-resistor-ohms to select the value of
 	 * the resistor between Vcc and Vbackup?
 	 */
@@ -216,6 +230,7 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 
 	if (ds1307->type == rx_8130) {
 		unsigned int regflag;
+
 		ret = regmap_read(ds1307->regmap, RX8130_REG_FLAG, &regflag);
 		if (ret) {
 			dev_err(dev, "%s error %d\n", "read", ret);
@@ -226,6 +241,19 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 			dev_warn_once(dev, "oscillator failed, set time!\n");
 			return -EINVAL;
 		}
+	} else if (ds1307->type == rx_8901) {
+		unsigned int regflag;
+
+		ret = regmap_read(ds1307->regmap, RX8901_REG_INTF, &regflag);
+		if (ret) {
+			dev_dbg(dev, "%s error %d\n", "read", ret);
+			return ret;
+		}
+
+		if (regflag & RX8901_REG_INTF_VLF) {
+			dev_warn_once(dev, "oscillator failed, set time!\n");
+			return -EINVAL;
+		}
 	}
 
 	/* read the RTC date and time registers all at once */
@@ -307,7 +335,7 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 	tmp = regs[DS1307_REG_HOUR] & 0x3f;
 	t->tm_hour = bcd2bin(tmp);
 	/* rx8130 is bit position, not BCD */
-	if (ds1307->type == rx_8130)
+	if (ds1307->type == rx_8130 || ds1307->type == rx_8901)
 		t->tm_wday = fls(regs[DS1307_REG_WDAY] & 0x7f) - 1;
 	else
 		t->tm_wday = bcd2bin(regs[DS1307_REG_WDAY] & 0x07) - 1;
@@ -358,7 +386,7 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 	regs[DS1307_REG_MIN] = bin2bcd(t->tm_min);
 	regs[DS1307_REG_HOUR] = bin2bcd(t->tm_hour);
 	/* rx8130 is bit position, not BCD */
-	if (ds1307->type == rx_8130)
+	if (ds1307->type == rx_8130 || ds1307->type == rx_8901)
 		regs[DS1307_REG_WDAY] = 1 << t->tm_wday;
 	else
 		regs[DS1307_REG_WDAY] = bin2bcd(t->tm_wday + 1);
@@ -422,11 +450,132 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 			dev_err(dev, "%s error %d\n", "write", result);
 			return result;
 		}
+	} else if (ds1307->type == rx_8901) {
+		/*
+		 * clear Voltage Loss Flag as data is available now (writing 1
+		 * to the other bits in the INTF register has no effect)
+		 */
+		result = regmap_write(ds1307->regmap, RX8901_REG_INTF,
+				      0xff ^ RX8901_REG_INTF_VLF);
+		if (result) {
+			dev_dbg(dev, "%s error %d\n", "write", result);
+			return result;
+		}
 	}
 
 	return 0;
 }
 
+#ifdef CONFIG_RTC_INTF_DEV
+static int rx8901_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
+{
+	struct ds1307 *ds1307 = dev_get_drvdata(dev);
+	unsigned int regflag, tmp = 0;
+	int ret = 0;
+
+	switch (cmd) {
+	case RTC_VL_READ:
+		ret = regmap_read(ds1307->regmap, RX8901_REG_INTF, &regflag);
+		if (ret)
+			return ret;
+
+		if (regflag & RX8901_REG_INTF_VLF)
+			tmp |= RTC_VL_DATA_INVALID;
+
+		ret = regmap_read(ds1307->regmap, RX8901_REG_BUF_INTF, &regflag);
+		if (ret)
+			return ret;
+
+		if (regflag & RX8901_REG_BUF_INTF_VBATLF)
+			tmp |= RTC_VL_BACKUP_LOW;
+
+		return put_user(tmp, (unsigned int __user *)arg);
+	default:
+		return -ENOIOCTLCMD;
+	}
+	return ret;
+}
+
+static int rx8901_param_get(struct device *dev, struct rtc_param *param)
+{
+	struct ds1307 *ds1307 = dev_get_drvdata(dev);
+	unsigned int regflag;
+	int ret;
+
+	switch (param->param) {
+	case RTC_PARAM_BACKUP_SWITCH_MODE:
+		ret = regmap_read(ds1307->regmap, RX8901_REG_PWSW_CFG, &regflag);
+		if (ret)
+			return ret;
+
+		if (regflag & RX8901_REG_PWSW_CFG_INIEN) {
+			param->uvalue = RTC_BSM_LEVEL;
+		} else {
+			unsigned int swsel = FIELD_GET(RX8901_REG_PWSW_CFG_SWSEL, regflag);
+
+			if (swsel == RX8901_SWSEL_PRIMARY_BACKUP)
+				param->uvalue = RTC_BSM_DIRECT;
+			else
+				param->uvalue = RTC_BSM_DISABLED;
+		}
+
+		break;
+
+	default:
+		return -EINVAL;
+	}
+	return ret;
+}
+
+static int rx8901_param_set(struct device *dev, struct rtc_param *param)
+{
+	struct ds1307 *ds1307 = dev_get_drvdata(dev);
+	unsigned int regmask;
+	unsigned int regval;
+	int ret;
+
+	switch (param->param) {
+	case RTC_PARAM_BACKUP_SWITCH_MODE:
+
+		switch (param->uvalue) {
+		case RTC_BSM_DISABLED:
+			/* Only main power supply is used */
+			regmask = RX8901_REG_PWSW_CFG_INIEN |
+				  RX8901_REG_PWSW_CFG_SWSEL;
+			regval = FIELD_PREP(RX8901_REG_PWSW_CFG_SWSEL,
+					    RX8901_SWSEL_PRIMARY) |
+				 FIELD_PREP(RX8901_REG_PWSW_CFG_INIEN, 0);
+			break;
+		case RTC_BSM_DIRECT:
+			/* Main and battery power supply is put in parallel (default) */
+			regmask = RX8901_REG_PWSW_CFG_INIEN |
+				  RX8901_REG_PWSW_CFG_SWSEL;
+			regval = FIELD_PREP(RX8901_REG_PWSW_CFG_SWSEL,
+					    RX8901_SWSEL_PRIMARY_BACKUP) |
+				 FIELD_PREP(RX8901_REG_PWSW_CFG_INIEN, 0);
+			break;
+		case RTC_BSM_LEVEL:
+			/* Enable auto power switching between main and backup power supply */
+			regmask = RX8901_REG_PWSW_CFG_INIEN;
+			regval = FIELD_PREP(RX8901_REG_PWSW_CFG_INIEN, 1);
+			break;
+		default:
+			return -EINVAL;
+		}
+		ret = regmap_update_bits(ds1307->regmap, RX8901_REG_PWSW_CFG, regmask, regval);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+#else
+#define rx8901_ioctl NULL
+#define rx8901_param_get NULL
+#define rx8901_param_set NULL
+#endif
+
 static int ds1337_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct ds1307		*ds1307 = dev_get_drvdata(dev);
@@ -533,7 +682,7 @@ static int ds1307_alarm_irq_enable(struct device *dev, unsigned int enabled)
 				  enabled ? DS1337_BIT_A1IE : 0);
 }
 
-static u8 do_trickle_setup_ds1339(struct ds1307 *ds1307, u32 ohms, bool diode)
+static int do_trickle_setup_ds1339(struct ds1307 *ds1307, u32 ohms, bool diode)
 {
 	u8 setup = (diode) ? DS1307_TRICKLE_CHARGER_DIODE :
 		DS1307_TRICKLE_CHARGER_NO_DIODE;
@@ -558,7 +707,7 @@ static u8 do_trickle_setup_ds1339(struct ds1307 *ds1307, u32 ohms, bool diode)
 	return setup;
 }
 
-static u8 do_trickle_setup_rx8130(struct ds1307 *ds1307, u32 ohms, bool diode)
+static int do_trickle_setup_rx8130(struct ds1307 *ds1307, u32 ohms, bool diode)
 {
 	/* make sure that the backup battery is enabled */
 	u8 setup = RX8130_REG_CONTROL1_INIEN;
@@ -568,6 +717,26 @@ static u8 do_trickle_setup_rx8130(struct ds1307 *ds1307, u32 ohms, bool diode)
 	return setup;
 }
 
+static int do_trickle_setup_rx8901(struct ds1307 *ds1307, u32 ohms __always_unused, bool diode)
+{
+	int ret;
+	unsigned int setup;
+
+	ret = regmap_read(ds1307->regmap, RX8901_REG_PWSW_CFG, &setup);
+	if (ret) {
+		dev_err(ds1307->dev, "Failed to read PWSW_CFG register\n");
+		return ret;
+	}
+
+	/* Enable low battery voltage detection */
+	setup |= RX8901_REG_PWSW_CFG_VBATLDETEN;
+
+	if (diode)
+		setup |= RX8901_REG_PWSW_CFG_CHGEN;
+
+	return setup;
+}
+
 static irqreturn_t rx8130_irq(int irq, void *dev_id)
 {
 	struct ds1307           *ds1307 = dev_id;
@@ -960,6 +1129,14 @@ static const struct rtc_class_ops rx8130_rtc_ops = {
 	.alarm_irq_enable = rx8130_alarm_irq_enable,
 };
 
+static const struct rtc_class_ops rx8901_rtc_ops = {
+	.read_time      = ds1307_get_time,
+	.set_time       = ds1307_set_time,
+	.ioctl          = rx8901_ioctl,
+	.param_get      = rx8901_param_get,
+	.param_set      = rx8901_param_set,
+};
+
 static const struct rtc_class_ops mcp794xx_rtc_ops = {
 	.read_time      = ds1307_get_time,
 	.set_time       = ds1307_set_time,
@@ -1040,6 +1217,12 @@ static const struct chip_desc chips[last_ds_type] = {
 		.trickle_charger_reg = RX8130_REG_CONTROL1,
 		.do_trickle_setup = &do_trickle_setup_rx8130,
 	},
+	[rx_8901] = {
+		.offset		= 0x0,
+		.rtc_ops = &rx8901_rtc_ops,
+		.trickle_charger_reg = RX8901_REG_PWSW_CFG,
+		.do_trickle_setup = &do_trickle_setup_rx8901,
+	},
 	[m41t0] = {
 		.rtc_ops	= &m41txx_rtc_ops,
 	},
@@ -1081,6 +1264,7 @@ static const struct i2c_device_id ds1307_id[] = {
 	{ "rx8025", rx_8025 },
 	{ "isl12057", ds_1337 },
 	{ "rx8130", rx_8130 },
+	{ "rx8901", rx_8901 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ds1307_id);
@@ -1158,6 +1342,10 @@ static const struct of_device_id ds1307_of_match[] = {
 		.compatible = "epson,rx8130",
 		.data = (void *)rx_8130
 	},
+	{
+		.compatible = "epson,rx8901",
+		.data = (void *)rx_8901
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ds1307_of_match);
@@ -1292,7 +1480,7 @@ static int ds1307_nvram_write(void *priv, unsigned int offset, void *val,
 
 /*----------------------------------------------------------------------*/
 
-static u8 ds1307_trickle_init(struct ds1307 *ds1307,
+static int ds1307_trickle_init(struct ds1307 *ds1307,
 			      const struct chip_desc *chip)
 {
 	u32 ohms, chargeable;
@@ -1745,7 +1933,7 @@ static int ds1307_probe(struct i2c_client *client)
 	bool			ds1307_can_wakeup_device = false;
 	unsigned char		regs[8];
 	struct ds1307_platform_data *pdata = dev_get_platdata(&client->dev);
-	u8			trickle_charger_setup = 0;
+	int			trickle_charger_setup = 0;
 
 	ds1307 = devm_kzalloc(&client->dev, sizeof(struct ds1307), GFP_KERNEL);
 	if (!ds1307)
@@ -1781,12 +1969,15 @@ static int ds1307_probe(struct i2c_client *client)
 	else if (pdata->trickle_charger_setup)
 		trickle_charger_setup = pdata->trickle_charger_setup;
 
+	if (trickle_charger_setup < 0)
+		return trickle_charger_setup;
+
 	if (trickle_charger_setup && chip->trickle_charger_reg) {
 		dev_dbg(ds1307->dev,
 			"writing trickle charger info 0x%x to 0x%x\n",
 			trickle_charger_setup, chip->trickle_charger_reg);
 		regmap_write(ds1307->regmap, chip->trickle_charger_reg,
-			     trickle_charger_setup);
+			     (u8)trickle_charger_setup);
 	}
 
 /*
@@ -1990,6 +2181,13 @@ static int ds1307_probe(struct i2c_client *client)
 		}
 	}
 
+	switch (ds1307->type) {
+	case rx_8901:
+		set_bit(RTC_FEATURE_BACKUP_SWITCH_MODE, ds1307->rtc->features);
+		break;
+	default:
+	}
+
 	ds1307->rtc->ops = chip->rtc_ops ?: &ds13xx_rtc_ops;
 	err = ds1307_add_frequency_test(ds1307);
 	if (err)

-- 
2.43.0

Re: [PATCH v2 3/3] rtc: ds1307: Add driver for Epson RX8901CE

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Trickle charging is not explicitly disabled when requested, posing a battery safety hazard.
--

commit 1b11fec487efda4e55a4152eb696205b964ae2cd
Author: Fredrik M Olsson <fredrik.m.olsson@axis.com>

rtc: ds1307: Add driver for Epson RX8901CE

This commit adds I2C RTC driver support for the Epson RX8901CE. It includes
support for time reading/writing, Backup Switch Mode (BSM) configuration,
trickle charging, and battery voltage low status detection.

> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index bf42c250ea7d1..23dd104aa2be7 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c

[ ... ]

> @@ -568,6 +717,26 @@ static u8 do_trickle_setup_rx8130(struct ds1307 *ds1307, u32 ohms, bool diode)
>  	return setup;
>  }
>  
> +static int do_trickle_setup_rx8901(struct ds1307 *ds1307, u32 ohms __always_unused, bool diode)
> +{
> +	int ret;
> +	unsigned int setup;
> +
> +	ret = regmap_read(ds1307->regmap, RX8901_REG_PWSW_CFG, &setup);
> +	if (ret) {
> +		dev_err(ds1307->dev, "Failed to read PWSW_CFG register\n");
> +		return ret;
> +	}
> +
> +	/* Enable low battery voltage detection */
> +	setup |= RX8901_REG_PWSW_CFG_VBATLDETEN;
> +
> +	if (diode)
> +		setup |= RX8901_REG_PWSW_CFG_CHGEN;

[Severity: High]
If diode is false, does this code need to explicitly clear the
RX8901_REG_PWSW_CFG_CHGEN bit?

Because RX8901_REG_PWSW_CFG is a battery-backed register, a previously
set charge enable bit (for example, from a bootloader or a prior warm
reboot) might remain set even if trickle charging is explicitly disabled
by the system configuration.

Could this lead to a physical safety hazard by unintentionally charging a
primary (non-rechargeable) battery if the bit is not explicitly cleared
using something like setup &= ~RX8901_REG_PWSW_CFG_CHGEN?

> +
> +	return setup;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-ds1307-rx8901-add-v2-0-e069ea32e1db@axis.com?part=3