[PATCH v2 0/5] rtc: s35390a: Allow use of output pin for interrupt signal 1 for wakealarm

[PATCH v2 0/5] rtc: s35390a: Allow use of output pin for interrupt signal 1 for wakealarm

[Markus Probst]

Add pinctrl to allow the use of output pin for interrupt signal 1
for wakealarm. This is needed for wakealarms to work on Synology NAS
devices.

I could only partially test the pinctrl patch. My testing system runs
ACPI, which makes it impossible for me to configure pinctrl there. I did
however verify that with missing pinctrl configuration in the devicetree,
the register were correctly set. So there should be no regressions.
Every other mode than ignore, disable and wakeup should also be
considered untested.

Also If I am not mistaken, wake alarms on these systems are currently broken:
(not tested, judged by looking at the devicetrees).

- arch/arm/boot/dts/marvell/armada-370-synology-ds213j.dts
- arch/arm/boot/dts/marvell/armada-xp-synology-ds414.dts
- arch/arm/boot/dts/marvell/kirkwood-synology.dtsi
- arch/arm/boot/dts/marvell/kirkwood-ds110jv10.dts
- arch/arm/boot/dts/marvell/kirkwood-ds111.dts
- arch/arm/boot/dts/marvell/kirkwood-ds112.dts
- arch/arm/boot/dts/marvell/kirkwood-ds210.dts
- arch/arm/boot/dts/marvell/kirkwood-ds212.dts
- arch/arm/boot/dts/marvell/kirkwood-ds212j.dts
- arch/arm/boot/dts/marvell/kirkwood-ds411.dts
- arch/arm/boot/dts/marvell/kirkwood-ds411j.dts
- arch/arm/boot/dts/marvell/kirkwood-ds411slim.dts
- arch/arm/boot/dts/marvell/kirkwood-rs212.dts
- arch/arm/boot/dts/marvell/kirkwood-rs411.dts

If thats the case it can be fixed by using this patch series and adding
the example in the devicetree to the s35390a devicetree.

If somebody still runs one of these systems, please test.

Thanks
- Markus Probst

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
Changes in v2:
- remove sii,wakealarm-output-pin property
- add pinctrl
- add fix to allow disabling of wake alarms
- add synology quirk
- Link to v1: https://patch.msgid.link/20260630-rtc_s35390a_int1-v1-0-1b2239e16be2@posteo.de

---
Markus Probst (5):
      dt-bindings: rtc: Add pinctrl for S35390A
      rtc: s35390a: Add missing newline to dev_err
      rtc: s35390a: Fix alarm not disabling
      rtc: s35390a: Add pinctrl
      rtc: s35390a: Add synology quirk

 .../devicetree/bindings/rtc/sii,s35390a.yaml       | 119 ++++++
 .../devicetree/bindings/rtc/trivial-rtc.yaml       |   3 -
 MAINTAINERS                                        |   1 +
 drivers/rtc/Kconfig                                |   3 +
 drivers/rtc/rtc-s35390a.c                          | 429 ++++++++++++++++++---
 include/dt-bindings/rtc/s35390a.h                  |  23 ++
 6 files changed, 519 insertions(+), 59 deletions(-)
---
base-commit: a8e429896436e8c2d288181f875f92af8204bc58
change-id: 20260630-rtc_s35390a_int1-556ccb308d3f

[PATCH v2 1/5] dt-bindings: rtc: Add pinctrl for S35390A

[Markus Probst]

Synology NAS devices use the output pin for interrupt signal 1 to wake up
the system.

Move devicetree bindings for sii,s35390a into its own file.
Add necessary properties to configure the individual pins via pinctrl,
which allows the interrupt signal 1 to be used for wakeup alarm.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 .../devicetree/bindings/rtc/sii,s35390a.yaml       | 119 +++++++++++++++++++++
 .../devicetree/bindings/rtc/trivial-rtc.yaml       |   3 -
 MAINTAINERS                                        |   1 +
 include/dt-bindings/rtc/s35390a.h                  |  23 ++++
 4 files changed, 143 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml b/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml
new file mode 100644
index 000000000000..78a049576d5e
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml
@@ -0,0 +1,119 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/sii,s35390a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: S-35390A 2-WIRE REAL-TIME CLOCK
+
+maintainers:
+  - Alexandre Belloni <alexandre.belloni@bootlin.com>
+
+description:
+  The S-35390A is a CMOS 2-wire real-time clock IC which operates with the
+  very low current consumption in the wide range of operation voltage.
+
+allOf:
+  - $ref: rtc.yaml#
+
+properties:
+  compatible:
+    const: sii,s35390a
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+    description: Supports up to 2 interrupt lines via the INT1 and INT2 pins.
+
+  wakeup-parent: true
+
+  wakeup-source: true
+
+required:
+  - compatible
+  - reg
+
+patternProperties:
+  "^pins":
+    type: object
+    patternProperties:
+      "-pins$":
+        type: object
+        properties:
+          pins:
+            $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/pins
+
+          sii,mode:
+            $ref: /schemas/types.yaml#/definitions/uint32
+            description: |
+              Modes for the specific pin:
+              - S35390A_MODE_IGNORE: Preserve the previous state.
+              - S35390A_MODE_DISABLE: Disable all interrupts.
+              - S35390A_MODE_WAKEUP: Used for waking up the system.
+              - S35390A_MODE_ALARM: Userspace defined alarm.
+              - S35390A_MODE_FREQ: Periodical at configurable frequency.
+              - S35390A_MODE_PMIN1: Minute periodical output with 50% duty.
+              - S35390A_MODE_PMIN2: Minute periodical output L for 7.81 ms.
+                                    Can only be used with pin INT1.
+              - S35390A_MODE_32768KHZ: Raw 32.768 kHz output.
+                                       Can only be used with pin INT1.
+            minimum: 0
+            maximum: 7
+
+          sii,frequency:
+            $ref: /schemas/types.yaml#/definitions/uint32
+            description: Frequency in Hz to be used with mode S35390A_MODE_FREQ.
+            minimum: 1
+            maximum: 31
+
+        required:
+          - sii,mode
+
+        if:
+          properties:
+            sii,mode:
+              const: 4
+        then:
+          required:
+            - sii,frequency
+        else:
+          properties:
+            sii,frequency: false
+
+        additionalProperties: false
+
+    additionalProperties: false
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/rtc/s35390a.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rtc: rtc@30 {
+            compatible = "sii,s35390a";
+            reg = <0x30>;
+            pinctrl-0 = <&rtc_pins_default>;
+            pinctrl-names = "default";
+
+            wakeup-parent = <&int>;
+
+            rtc_pins_default: pins {
+              int1-pins {
+                pins = S35390A_PIN_INT1;
+                sii,mode = <S35390A_MODE_WAKEUP>;
+              };
+              int2-pins {
+                pins = S35390A_PIN_INT2;
+                sii,mode = <S35390A_MODE_DISABLE>;
+              };
+            };
+        };
+    };
diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
index f4d0eed98a08..7b3f682ef4d5 100644
--- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
@@ -81,8 +81,6 @@ properties:
       - ricoh,rv5c386
       # I2C bus SERIAL INTERFACE REAL-TIME CLOCK IC
       - ricoh,rv5c387a
-      # 2-wire CMOS real-time clock
-      - sii,s35390a
       # ST SPEAr Real-time Clock
       - st,spear600-rtc
       # VIA/Wondermedia VT8500 Real-time Clock
@@ -105,5 +103,4 @@ required:
   - reg
 
 additionalProperties: false
-
 ...
diff --git a/MAINTAINERS b/MAINTAINERS
index 716acfc3d7c1..c85be1eefdb1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22696,6 +22696,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git
 F:	Documentation/admin-guide/rtc.rst
 F:	Documentation/devicetree/bindings/rtc/
 F:	drivers/rtc/
+F:	include/dt-bindings/rtc/
 F:	include/linux/rtc.h
 F:	include/linux/rtc/
 F:	include/uapi/linux/rtc.h
diff --git a/include/dt-bindings/rtc/s35390a.h b/include/dt-bindings/rtc/s35390a.h
new file mode 100644
index 000000000000..dcd0ca15f9e9
--- /dev/null
+++ b/include/dt-bindings/rtc/s35390a.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) */
+
+#ifndef __DT_BINDINGS_RTC_S35390A_H
+#define __DT_BINDINGS_RTC_S35390A_H
+
+#define S35390A_MODE_IGNORE		0x0
+#define S35390A_MODE_DISABLE		0x1
+#define S35390A_MODE_WAKEUP		0x2
+#define S35390A_MODE_ALARM		0x3
+#define S35390A_MODE_FREQ		0x4
+#define S35390A_MODE_PMIN1		0x5
+#define S35390A_MODE_PMIN2		0x6
+#define S35390A_MODE_32768KHZ		0x7
+
+#define S35390A_MODE_INT1_MAX		0x7
+#define S35390A_MODE_INT2_MAX		0x5
+
+#define S35390A_FREQ_MASK		0x1F
+
+#define S35390A_PIN_INT1		"int1"
+#define S35390A_PIN_INT2		"int2"
+
+#endif /* __DT_BINDINGS_RTC_S35390A_H */

-- 
2.54.0

[PATCH v2 2/5] rtc: s35390a: Add missing newline to dev_err

[Markus Probst]

Fixes: 3bd32722c827d ("rtc: s35390a: improve irq handling")
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 drivers/rtc/rtc-s35390a.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index b72eef4fb099..4cfe7034c516 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -494,7 +494,7 @@ static int s35390a_probe(struct i2c_client *client)
 		buf = 0;
 		err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
 		if (err < 0) {
-			dev_err(dev, "error disabling alarm");
+			dev_err(dev, "error disabling alarm\n");
 			return err;
 		}
 	} else {

-- 
2.54.0

[PATCH v2 3/5] rtc: s35390a: Fix alarm not disabling

[Markus Probst]

Implement alarm_irq_enable callback to disable alarm if requested.
On reenable restore last used alarm time.

Fixes: 542dd33a4925 ("drivers/rtc/rtc-s35390a.c: add wakealarm support for rtc-s35390A rtc chip")
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 drivers/rtc/rtc-s35390a.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 4cfe7034c516..fbf5471eb3d0 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -64,6 +64,7 @@ MODULE_DEVICE_TABLE(of, s35390a_of_match);
 
 struct s35390a {
 	struct i2c_client *client[8];
+	struct rtc_time tm_alarm;
 	int twentyfourhour;
 };
 
@@ -320,6 +321,9 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
 	err = s35390a_set_reg(s35390a, S35390A_CMD_INT2_REG1, buf,
 								sizeof(buf));
 
+	if (!err)
+		s35390a->tm_alarm = alm->time;
+
 	return err;
 }
 
@@ -379,6 +383,19 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
 	return 0;
 }
 
+static int s35390a_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+	struct s35390a *s35390a = dev_get_drvdata(dev);
+	struct rtc_wkalrm alm;
+
+	alm.enabled = enabled;
+
+	if (enabled)
+		alm.time = s35390a->tm_alarm;
+
+	return s35390a_rtc_set_alarm(dev, &alm);
+}
+
 static int s35390a_rtc_ioctl(struct device *dev, unsigned int cmd,
 			     unsigned long arg)
 {
@@ -410,11 +427,12 @@ static int s35390a_rtc_ioctl(struct device *dev, unsigned int cmd,
 }
 
 static const struct rtc_class_ops s35390a_rtc_ops = {
-	.read_time	= s35390a_rtc_read_time,
-	.set_time	= s35390a_rtc_set_time,
-	.set_alarm	= s35390a_rtc_set_alarm,
-	.read_alarm	= s35390a_rtc_read_alarm,
-	.ioctl          = s35390a_rtc_ioctl,
+	.read_time		= s35390a_rtc_read_time,
+	.set_time		= s35390a_rtc_set_time,
+	.set_alarm		= s35390a_rtc_set_alarm,
+	.read_alarm		= s35390a_rtc_read_alarm,
+	.alarm_irq_enable	= s35390a_rtc_alarm_irq_enable,
+	.ioctl			= s35390a_rtc_ioctl,
 };
 
 static int s35390a_nvmem_read(void *priv, unsigned int offset, void *val,

-- 
2.54.0

[PATCH v2 4/5] rtc: s35390a: Add pinctrl

[Markus Probst]

Allow configuration of other output modes than wake alarm, including:
- keeping the previous configured mode
- disabling output
- custom frequency
- minute periodical interrupt (1: 50% duty, 2: 7.81 ms)
- raw 32.768 kHz output

Allow use of interrupt signal 1.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 drivers/rtc/Kconfig       |   3 +
 drivers/rtc/rtc-s35390a.c | 403 +++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 349 insertions(+), 57 deletions(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 01def8231873..054ff134d84e 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -668,7 +668,10 @@ config RTC_DRV_RC5T619
 
 config RTC_DRV_S35390A
 	tristate "Seiko Instruments S-35390A"
+	depends on OF
+	depends on PINCTRL
 	select BITREVERSE
+	select GENERIC_PINCONF
 	help
 	  If you say yes here you will get support for the Seiko
 	  Instruments S-35390A.
diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index fbf5471eb3d0..fc0dbce0e8f4 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -12,11 +12,18 @@
 #include <linux/bcd.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <dt-bindings/rtc/s35390a.h>
+
+#define DRIVER_NAME "rtc-s35390a"
 
 #define S35390A_CMD_STATUS1	0
 #define S35390A_CMD_STATUS2	1
 #define S35390A_CMD_TIME1	2
 #define S35390A_CMD_TIME2	3
+#define S35390A_CMD_INT1_REG1	4
 #define S35390A_CMD_INT2_REG1	5
 #define S35390A_CMD_FREE_REG    7
 
@@ -36,19 +43,31 @@
 #define S35390A_FLAG_POC	BIT(0)
 #define S35390A_FLAG_BLD	BIT(1)
 #define S35390A_FLAG_INT2	BIT(2)
+#define S35390A_FLAG_INT1	BIT(3)
 #define S35390A_FLAG_24H	BIT(6)
 #define S35390A_FLAG_RESET	BIT(7)
 
 /* flag for STATUS2 */
 #define S35390A_FLAG_TEST	BIT(0)
 
+
+#define S35390A_INT_MODE_NOINTR	0x00
+
 /* INT2 pin output mode */
 #define S35390A_INT2_MODE_MASK		0x0E
-#define S35390A_INT2_MODE_NOINTR	0x00
 #define S35390A_INT2_MODE_ALARM		BIT(1) /* INT2AE */
 #define S35390A_INT2_MODE_PMIN_EDG	BIT(2) /* INT2ME */
 #define S35390A_INT2_MODE_FREQ		BIT(3) /* INT2FE */
-#define S35390A_INT2_MODE_PMIN		(BIT(3) | BIT(2)) /* INT2FE | INT2ME */
+#define S35390A_INT2_MODE_PMIN1		(BIT(3) | BIT(2)) /* INT2FE | INT2ME */
+
+/* INT1 pin output mode */
+#define S35390A_INT1_MODE_MASK		0xF0
+#define S35390A_INT1_MODE_ALARM		BIT(5) /* INT1AE */
+#define S35390A_INT1_MODE_PMIN_EDG	BIT(6) /* INT1ME */
+#define S35390A_INT1_MODE_FREQ		BIT(7) /* INT1FE */
+#define S35390A_INT1_MODE_PMIN1		(BIT(7) | BIT(6)) /* INT1FE | INT1ME */
+#define S35390A_INT1_MODE_PMIN2		(BIT(7) | BIT(6) | BIT(5)) /* INT1FE | INT1ME | INT1AE */
+#define S35390A_INT1_MODE_32768KHZ	BIT(4) /* 32kE */
 
 static const struct i2c_device_id s35390a_id[] = {
 	{ .name = "s35390a" },
@@ -64,8 +83,15 @@ MODULE_DEVICE_TABLE(of, s35390a_of_match);
 
 struct s35390a {
 	struct i2c_client *client[8];
-	struct rtc_time tm_alarm;
+	struct rtc_device *rtc;
+	struct pinctrl_dev *pinctrl;
+	struct rtc_wkalrm alarm;
 	int twentyfourhour;
+
+	struct mutex mode_lock;
+	bool mode_init;
+	int mode[2];
+	u8 freq[2];
 };
 
 static int s35390a_set_reg(struct s35390a *s35390a, int reg, u8  *buf, int len)
@@ -283,33 +309,33 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
 		alm->time.tm_min, alm->time.tm_hour, alm->time.tm_mday,
 		alm->time.tm_mon, alm->time.tm_year, alm->time.tm_wday);
 
-	/* disable interrupt (which deasserts the irq line) */
-	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
-	if (err < 0)
-		return err;
+	guard(mutex)(&s35390a->mode_lock);
 
-	/* clear pending interrupt (in STATUS1 only), if any */
-	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, &sts, sizeof(sts));
+	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
 	if (err < 0)
 		return err;
 
-	if (alm->enabled)
-		sts = S35390A_INT2_MODE_ALARM;
-	else
-		sts = S35390A_INT2_MODE_NOINTR;
+	/* disable interrupt (which deasserts the irq line) */
+	if (s35390a->mode[0] == S35390A_MODE_WAKEUP)
+		sts = (sts & ~S35390A_INT1_MODE_MASK) | S35390A_INT_MODE_NOINTR;
+
+	if (s35390a->mode[1] == S35390A_MODE_WAKEUP)
+		sts = (sts & ~S35390A_INT2_MODE_MASK) | S35390A_INT_MODE_NOINTR;
 
-	/* set interrupt mode*/
 	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
 	if (err < 0)
 		return err;
 
+	if (!alm->enabled)
+		goto end;
+
 	if (alm->time.tm_wday != -1)
 		buf[S35390A_ALRM_BYTE_WDAY] = bin2bcd(alm->time.tm_wday) | 0x80;
 	else
 		buf[S35390A_ALRM_BYTE_WDAY] = 0;
 
 	buf[S35390A_ALRM_BYTE_HOURS] = s35390a_hr2reg(s35390a,
-			alm->time.tm_hour) | 0x80;
+						      alm->time.tm_hour) | 0x80;
 	buf[S35390A_ALRM_BYTE_MINS] = bin2bcd(alm->time.tm_min) | 0x80;
 
 	if (alm->time.tm_hour >= 12)
@@ -318,13 +344,33 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
 	for (i = 0; i < 3; ++i)
 		buf[i] = bitrev8(buf[i]);
 
-	err = s35390a_set_reg(s35390a, S35390A_CMD_INT2_REG1, buf,
-								sizeof(buf));
+	/* set interrupt mode */
+	if (s35390a->mode[0] == S35390A_MODE_WAKEUP)
+		sts = (sts & ~S35390A_INT1_MODE_MASK) | S35390A_INT1_MODE_ALARM;
+
+	if (s35390a->mode[1] == S35390A_MODE_WAKEUP)
+		sts = (sts & ~S35390A_INT2_MODE_MASK) | S35390A_INT2_MODE_ALARM;
+
+	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
+	if (err < 0)
+		return err;
+
+	if (s35390a->mode[0] == S35390A_MODE_WAKEUP) {
+		err = s35390a_set_reg(s35390a, S35390A_CMD_INT1_REG1, buf, sizeof(buf));
+		if (err)
+			return err;
+	}
+
+	if (s35390a->mode[1] == S35390A_MODE_WAKEUP) {
+		err = s35390a_set_reg(s35390a, S35390A_CMD_INT2_REG1, buf, sizeof(buf));
+		if (err)
+			return err;
+	}
 
-	if (!err)
-		s35390a->tm_alarm = alm->time;
+end:
+	s35390a->alarm = *alm;
 
-	return err;
+	return 0;
 }
 
 static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
@@ -332,24 +378,35 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct s35390a *s35390a = i2c_get_clientdata(client);
 	u8 buf[3], sts;
-	int i, err;
+	int i, err, reg;
+
+	guard(mutex)(&s35390a->mode_lock);
 
 	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
 	if (err < 0)
 		return err;
 
-	if ((sts & S35390A_INT2_MODE_MASK) != S35390A_INT2_MODE_ALARM) {
+	if (s35390a->mode[1] == S35390A_MODE_WAKEUP
+		&& (sts & S35390A_INT2_MODE_MASK) == S35390A_INT2_MODE_ALARM) {
+
+		reg = S35390A_CMD_INT2_REG1;
+	} else if (s35390a->mode[0] == S35390A_MODE_WAKEUP
+		&& (sts & S35390A_INT1_MODE_MASK) == S35390A_INT1_MODE_ALARM) {
+
+		reg = S35390A_CMD_INT1_REG1;
+	} else {
 		/*
 		 * When the alarm isn't enabled, the register to configure
 		 * the alarm time isn't accessible.
 		 */
 		alm->enabled = 0;
+		alm->time = s35390a->alarm.time;
 		return 0;
-	} else {
-		alm->enabled = 1;
 	}
 
-	err = s35390a_get_reg(s35390a, S35390A_CMD_INT2_REG1, buf, sizeof(buf));
+	alm->enabled = 1;
+
+	err = s35390a_get_reg(s35390a, reg, buf, sizeof(buf));
 	if (err < 0)
 		return err;
 
@@ -358,7 +415,7 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
 		buf[i] = bitrev8(buf[i]);
 
 	/*
-	 * B0 of the three matching registers is an enable flag. Iff it is set
+	 * B0 of the three matching registers is an enable flag. If it is set
 	 * the configured value is used for matching.
 	 */
 	if (buf[S35390A_ALRM_BYTE_WDAY] & 0x80)
@@ -388,10 +445,10 @@ static int s35390a_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled
 	struct s35390a *s35390a = dev_get_drvdata(dev);
 	struct rtc_wkalrm alm;
 
-	alm.enabled = enabled;
-
 	if (enabled)
-		alm.time = s35390a->tm_alarm;
+		alm = s35390a->alarm;
+
+	alm.enabled = enabled;
 
 	return s35390a_rtc_set_alarm(dev, &alm);
 }
@@ -452,13 +509,241 @@ static int s35390a_nvmem_write(void *priv, unsigned int offset, void *val,
 	return s35390a_set_reg(s35390a, S35390A_CMD_FREE_REG, val, bytes);
 }
 
+static int s35390a_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return 0;
+}
+
+static const char *s35390a_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
+						  unsigned int group)
+{
+	return NULL;
+}
+
+static const struct pinctrl_pin_desc s35390a_pins_desc[] = {
+	PINCTRL_PIN(0, "int1"),
+	PINCTRL_PIN(1, "int2"),
+};
+
+static const struct pinctrl_ops s35390a_pinctrl_ops = {
+	.get_groups_count = s35390a_pinctrl_get_groups_count,
+	.get_group_name = s35390a_pinctrl_get_group_name,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
+	.dt_free_map = pinconf_generic_dt_free_map,
+};
+
+#define PIN_CONFIG_MODE		(PIN_CONFIG_END + 1)
+#define PIN_CONFIG_FREQ		(PIN_CONFIG_END + 2)
+
+static const struct pinconf_generic_params s35390a_pinconf_params[] = {
+	{"sii,mode", PIN_CONFIG_MODE},
+	{"sii,frequency", PIN_CONFIG_FREQ},
+};
+
+static int s35390a_set_freq(struct s35390a *s35390a, int pin, int freq)
+{
+	int err, reg;
+	u8 buf;
+
+	reg = pin == 0 ? S35390A_CMD_INT1_REG1 : S35390A_CMD_INT2_REG1;
+
+	buf = bitrev8(freq);
+
+	err = s35390a_set_reg(s35390a, reg, &buf, 1);
+	if (err < 0)
+		return err;
+	return 0;
+}
+
+static int s35390a_update_mode(struct s35390a *s35390a, int pin, int mode, int freq)
+{
+	int err;
+	u8 buf, status1, flag, mask;
+	bool update_irq = false;
+
+	mask = pin == 0 ? S35390A_INT1_MODE_MASK : S35390A_INT2_MODE_MASK;
+
+	s35390a->mode_init = true;
+
+	guard(mutex)(&s35390a->mode_lock);
+
+	dev_dbg(&s35390a->client[0]->dev, "%s: pin=%d mode=%d\n",
+			__func__, pin, mode);
+
+	if (mode == S35390A_MODE_FREQ) {
+		if (freq & ~S35390A_FREQ_MASK) {
+			dev_err(&s35390a->client[0]->dev, "Unsupported frequency %u\n", freq);
+			return -EINVAL;
+		}
+	} else
+		freq = 0;
+
+	if (mode == s35390a->mode[pin] && freq == s35390a->freq[pin])
+		return 0;
+
+	s35390a->mode[pin] = mode;
+	s35390a->freq[pin] = freq;
+
+	if (mode == S35390A_MODE_IGNORE)
+		return 0;
+
+	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
+	if (err < 0) {
+		dev_err(&s35390a->client[0]->dev, "error reading status\n");
+		return err;
+	}
+
+	switch (mode) {
+	case S35390A_MODE_DISABLE:
+	case S35390A_MODE_ALARM: /* not implemented */
+		buf = (buf & ~mask) | S35390A_INT_MODE_NOINTR;
+		break;
+	case S35390A_MODE_WAKEUP:
+		flag = pin == 0 ? S35390A_INT1_MODE_ALARM : S35390A_INT2_MODE_ALARM;
+		if ((buf & mask) != flag) {
+			buf = (buf & ~mask) | S35390A_INT_MODE_NOINTR;
+			break;
+		}
+
+		err = s35390a_read_status(s35390a, &status1);
+		if (err < 0) {
+			dev_err(&s35390a->client[0]->dev, "error reading status\n");
+			return err;
+		}
+
+		flag = pin == 0 ? S35390A_FLAG_INT1 : S35390A_FLAG_INT2;
+
+		/* disable alarm */
+		if (status1 & flag) {
+			buf = (buf & ~mask) | S35390A_INT_MODE_NOINTR;
+			update_irq = true;
+		}
+
+		break;
+	case S35390A_MODE_FREQ:
+		flag = pin == 0 ? S35390A_INT1_MODE_FREQ : S35390A_INT2_MODE_FREQ;
+		buf = (buf & ~mask) | flag;
+		break;
+	case S35390A_MODE_PMIN1:
+		flag = pin == 0 ? S35390A_INT1_MODE_PMIN1 : S35390A_INT2_MODE_PMIN1;
+		buf = (buf & ~mask) | flag;
+		break;
+
+	/* INT1 only modes */
+	case S35390A_MODE_PMIN2:
+		buf = (buf & ~mask) | S35390A_INT1_MODE_PMIN2;
+		break;
+	case S35390A_MODE_32768KHZ:
+		buf = (buf & ~mask) | S35390A_INT1_MODE_32768KHZ;
+		break;
+	}
+
+	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
+	if (err < 0) {
+		dev_err(&s35390a->client[0]->dev, "error setting interrupts\n");
+		return err;
+	}
+
+	if (freq) {
+		err = s35390a_set_freq(s35390a, pin, freq);
+		if (err < 0) {
+			dev_err(&s35390a->client[0]->dev, "error setting frequency\n");
+			return err;
+		}
+	}
+
+	if (update_irq)
+		rtc_update_irq(s35390a->rtc, 1, RTC_AF);
+
+	return 0;
+}
+
+static int s35390a_pinconf_get(struct pinctrl_dev *pctldev,
+			       unsigned int pin, unsigned long *config)
+{
+	struct s35390a *s35390a = pinctrl_dev_get_drvdata(pctldev);
+	unsigned int param = pinconf_to_config_param(*config);
+	u16 arg;
+
+	switch (param) {
+	case PIN_CONFIG_MODE:
+		arg = s35390a->mode[pin];
+		break;
+	case PIN_CONFIG_FREQ:
+		arg = s35390a->freq[pin];
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return 0;
+}
+
+static int s35390a_pinconf_set(struct pinctrl_dev *pctldev,
+			       unsigned int pin, unsigned long *configs,
+			       unsigned int num_configs)
+{
+	struct s35390a *s35390a = pinctrl_dev_get_drvdata(pctldev);
+	unsigned int param;
+	u32 param_val;
+	int i, mode, freq = 0;
+
+	mode = pin == 0 ? S35390A_MODE_IGNORE : S35390A_MODE_WAKEUP;
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[0]);
+		param_val = pinconf_to_config_argument(configs[0]);
+
+		switch (param) {
+		case PIN_CONFIG_MODE:
+			if (param_val <= (pin == 0 ? S35390A_MODE_INT1_MAX : S35390A_MODE_INT2_MAX))
+				mode = param_val;
+			else {
+				dev_err(&s35390a->client[0]->dev, "Unsupported pin mode %u\n",
+					param_val);
+				return -EINVAL;
+			}
+			break;
+		case PIN_CONFIG_FREQ:
+			freq = param_val;
+			break;
+		default:
+			dev_err(&s35390a->client[0]->dev, "Property %u not supported\n",
+				param);
+			return -EOPNOTSUPP;
+		}
+	}
+
+	return s35390a_update_mode(s35390a, pin, mode, freq);
+}
+
+
+static const struct pinconf_ops s35390a_pinconf_ops = {
+	.is_generic = true,
+	.pin_config_get = s35390a_pinconf_get,
+	.pin_config_set = s35390a_pinconf_set,
+};
+
+static struct pinctrl_desc s35390a_pinctrl_desc = {
+	.name = DRIVER_NAME,
+	.pins = s35390a_pins_desc,
+	.npins = ARRAY_SIZE(s35390a_pins_desc),
+	.pctlops = &s35390a_pinctrl_ops,
+	.confops = &s35390a_pinconf_ops,
+	.custom_params = s35390a_pinconf_params,
+	.num_custom_params = ARRAY_SIZE(s35390a_pinconf_params),
+	.owner = THIS_MODULE,
+};
+
 static int s35390a_probe(struct i2c_client *client)
 {
 	int err, err_read;
 	unsigned int i;
 	struct s35390a *s35390a;
 	struct rtc_device *rtc;
-	u8 buf, status1;
+	u8 status1;
 	struct device *dev = &client->dev;
 	struct nvmem_config nvmem_cfg = {
 		.name = "s35390a_nvram",
@@ -477,7 +762,10 @@ static int s35390a_probe(struct i2c_client *client)
 	if (!s35390a)
 		return -ENOMEM;
 
+	mutex_init(&s35390a->mode_lock);
+
 	s35390a->client[0] = client;
+
 	i2c_set_clientdata(client, s35390a);
 
 	/* This chip uses multiple addresses, use dummy devices for them */
@@ -492,10 +780,37 @@ static int s35390a_probe(struct i2c_client *client)
 		}
 	}
 
+	err = s35390a_disable_test_mode(s35390a);
+	if (err < 0) {
+		dev_err(dev, "error disabling test mode\n");
+		return err;
+	}
+
 	rtc = devm_rtc_allocate_device(dev);
 	if (IS_ERR(rtc))
 		return PTR_ERR(rtc);
 
+	rtc->ops = &s35390a_rtc_ops;
+	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
+	rtc->range_max = RTC_TIMESTAMP_END_2099;
+
+	set_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->features);
+	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);
+
+	s35390a->rtc = rtc;
+
+	err = devm_pinctrl_register_and_init(dev, &s35390a_pinctrl_desc, s35390a,
+					     &s35390a->pinctrl);
+	if (err)
+		return err;
+
+	/* If no pinctrl config is defined in DT, fallback to previous behaviour */
+	if (!s35390a->mode_init) {
+		err = s35390a_update_mode(s35390a, 1, S35390A_MODE_WAKEUP, 0);
+		if (err)
+			return err;
+	}
+
 	err_read = s35390a_read_status(s35390a, &status1);
 	if (err_read < 0) {
 		dev_err(dev, "error resetting chip\n");
@@ -507,34 +822,8 @@ static int s35390a_probe(struct i2c_client *client)
 	else
 		s35390a->twentyfourhour = 0;
 
-	if (status1 & S35390A_FLAG_INT2) {
-		/* disable alarm (and maybe test mode) */
-		buf = 0;
-		err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
-		if (err < 0) {
-			dev_err(dev, "error disabling alarm\n");
-			return err;
-		}
-	} else {
-		err = s35390a_disable_test_mode(s35390a);
-		if (err < 0) {
-			dev_err(dev, "error disabling test mode\n");
-			return err;
-		}
-	}
-
 	device_set_wakeup_capable(dev, 1);
 
-	rtc->ops = &s35390a_rtc_ops;
-	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
-	rtc->range_max = RTC_TIMESTAMP_END_2099;
-
-	set_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->features);
-	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);
-
-	if (status1 & S35390A_FLAG_INT2)
-		rtc_update_irq(rtc, 1, RTC_AF);
-
 	nvmem_cfg.priv = s35390a;
 	err = devm_rtc_nvmem_register(rtc, &nvmem_cfg);
 	if (err)
@@ -545,7 +834,7 @@ static int s35390a_probe(struct i2c_client *client)
 
 static struct i2c_driver s35390a_driver = {
 	.driver		= {
-		.name	= "rtc-s35390a",
+		.name	= DRIVER_NAME,
 		.of_match_table = of_match_ptr(s35390a_of_match),
 	},
 	.probe		= s35390a_probe,

-- 
2.54.0

[PATCH v2 5/5] rtc: s35390a: Add synology quirk

[Markus Probst]

Synology NAS devices use interrupt signal 1 for wakeup alarms. On ACPI
it is not possible to configure pinctrl.

Use interrupt signal 1 for wakeup if no pinctrl state has been
configured in devicetree and dmi sys vendor matches "Synology Inc.".

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 drivers/rtc/rtc-s35390a.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index fc0dbce0e8f4..f20b8a5a72de 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2007 Byron Bradley
  */
 
+#include <linux/dmi.h>
 #include <linux/module.h>
 #include <linux/rtc.h>
 #include <linux/i2c.h>
@@ -806,7 +807,16 @@ static int s35390a_probe(struct i2c_client *client)
 
 	/* If no pinctrl config is defined in DT, fallback to previous behaviour */
 	if (!s35390a->mode_init) {
-		err = s35390a_update_mode(s35390a, 1, S35390A_MODE_WAKEUP, 0);
+		i = 1;
+		/*
+		 * Synology uses interrupt signal 1 for wakeup. Since pinctrl cannot be
+		 * configured on ACPI, we check against the dmi sys vendor.
+		 */
+		if (dmi_match(DMI_SYS_VENDOR, "Synology Inc.")) {
+			i = 0;
+			s35390a_update_mode(s35390a, 1, S35390A_MODE_DISABLE, 0);
+		}
+		err = s35390a_update_mode(s35390a, i, S35390A_MODE_WAKEUP, 0);
 		if (err)
 			return err;
 	}

-- 
2.54.0

Re: [PATCH v2 2/5] rtc: s35390a: Add missing newline to dev_err

[sashiko-bot]

> Fixes: 3bd32722c827d ("rtc: s35390a: improve irq handling")
> Signed-off-by: Markus Probst <markus.probst@posteo.de>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-rtc_s35390a_int1-v2-0-f10c99ad1d6c@posteo.de?part=2

Re: [PATCH v2 1/5] dt-bindings: rtc: Add pinctrl for S35390A

[sashiko-bot]

> Synology NAS devices use the output pin for interrupt signal 1 to wake up
> the system.
> 
> Move devicetree bindings for sii,s35390a into its own file.
> Add necessary properties to configure the individual pins via pinctrl,
> which allows the interrupt signal 1 to be used for wakeup alarm.
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-rtc_s35390a_int1-v2-0-f10c99ad1d6c@posteo.de?part=1

Re: [PATCH v2 5/5] rtc: s35390a: Add synology quirk

[sashiko-bot]

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

New issues:
- [High] The return value of s35390a_update_mode() is ignored when disabling INT2 for Synology devices.

Pre-existing issues:
- [High] Software mode cache is eagerly updated before hardware is successfully modified in s35390a_update_mode().
--

--- Patch [5]: [PATCH v2 5/5] rtc: s35390a: Add synology quirk ---
commit 707121acedc466ad23fc787561fac16b4efa8e1f
Author: Markus Probst <markus.probst@posteo.de>

rtc: s35390a: Add synology quirk
    
Synology NAS devices use interrupt signal 1 for wakeup alarms. On ACPI
it is not possible to configure pinctrl. Use interrupt signal 1 for
wakeup if no pinctrl state has been configured in devicetree and dmi
sys vendor matches "Synology Inc.".

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index fc0dbce0e8f4a..f20b8a5a72dea 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c

[ ... ]

> @@ -806,7 +807,16 @@ static int s35390a_probe(struct i2c_client *client)
>  
>  	/* If no pinctrl config is defined in DT, fallback to previous behaviour */
>  	if (!s35390a->mode_init) {
> -		err = s35390a_update_mode(s35390a, 1, S35390A_MODE_WAKEUP, 0);
> +		i = 1;
> +		/*
> +		 * Synology uses interrupt signal 1 for wakeup. Since pinctrl cannot be
> +		 * configured on ACPI, we check against the dmi sys vendor.
> +		 */
> +		if (dmi_match(DMI_SYS_VENDOR, "Synology Inc.")) {
> +			i = 0;
> +			s35390a_update_mode(s35390a, 1, S35390A_MODE_DISABLE, 0);

[Severity: High]
Does this code ignore the return value of s35390a_update_mode() when
disabling INT2 for Synology devices?

If the I2C transaction fails during driver probe, such as due to a bus
error, the hardware configuration failure is silently ignored. This could
leave INT2 active when it should be disabled.

This is a pre-existing issue, but updating the software mode cache before
the hardware is successfully modified in s35390a_update_mode() makes
ignoring the return value more problematic:

drivers/rtc/rtc-s35390a.c:s35390a_update_mode() {
    ...
	if (mode == s35390a->mode[pin] && freq == s35390a->freq[pin])
		return 0;

	s35390a->mode[pin] = mode;
	s35390a->freq[pin] = freq;

	if (mode == S35390A_MODE_IGNORE)
		return 0;

	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
    ...
}

Because the internal software cache retains the new target mode even if the
I2C communication fails later in the function, subsequent requests to set
the same mode will silently return success without communicating with the
hardware.

Since the error is ignored in s35390a_probe(), if the initial disable
fails, the software cache becomes permanently out of sync with the
hardware, making any subsequent attempt to disable INT2 silently fail.

> +		}
> +		err = s35390a_update_mode(s35390a, i, S35390A_MODE_WAKEUP, 0);
>  		if (err)
>  			return err;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-rtc_s35390a_int1-v2-0-f10c99ad1d6c@posteo.de?part=5

Re: [PATCH v2 3/5] rtc: s35390a: Fix alarm not disabling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Uninitialized stack memory usage in `s35390a_rtc_alarm_irq_enable` corrupts the RTC hardware state and cached alarm time.
--

--- Patch [3]: [PATCH v2 3/5] rtc: s35390a: Fix alarm not disabling ---
commit 14759f80e44e19699052dca75586c20731eb4d5d
Author: Markus Probst <markus.probst@posteo.de>

rtc: s35390a: Fix alarm not disabling

This commit implements the alarm_irq_enable callback to disable the alarm if
requested. It also restores the last used alarm time when the alarm is
re-enabled.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index 4cfe7034c5165..fbf5471eb3d0b 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c

[ ... ]

> @@ -379,6 +383,19 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  	return 0;
>  }
>  
> +static int s35390a_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct s35390a *s35390a = dev_get_drvdata(dev);
> +	struct rtc_wkalrm alm;
> +
> +	alm.enabled = enabled;
> +
> +	if (enabled)
> +		alm.time = s35390a->tm_alarm;
> +
> +	return s35390a_rtc_set_alarm(dev, &alm);
> +}

[Severity: High]
When s35390a_rtc_alarm_irq_enable() is called with enabled == 0, the alm.time
field remains uninitialized on the stack.

Since alm is passed to s35390a_rtc_set_alarm(), won't the uninitialized
alm.time be read by the dev_dbg() call at the start of that function?

Additionally, does this uninitialized memory overwrite the cached alarm time?
Looking at the earlier changes to s35390a_rtc_set_alarm():

s35390a_rtc_set_alarm()
    ...
    if (!err)
        s35390a->tm_alarm = alm->time;

It appears the uninitialized stack memory can be assigned to s35390a->tm_alarm,
which would permanently corrupt the cache. When the alarm is subsequently
re-enabled, this corrupted time would then be restored and written to the
hardware.

Could this be resolved by zero-initializing the alm structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-rtc_s35390a_int1-v2-0-f10c99ad1d6c@posteo.de?part=3

Re: [PATCH v2 4/5] rtc: s35390a: Add pinctrl

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Missing `pinctrl_enable()` call after `devm_pinctrl_register_and_init()`.
- [High] Adding `depends on OF` breaks the driver on ACPI-based x86 systems.
- [High] Hardcoded array index `configs[0]` inside loop in `s35390a_pinconf_set` ignores multiple configurations.
- [High] Information leak due to uninitialized stack variable in `s35390a_rtc_alarm_irq_enable()`.
- [High] Hardware pending interrupt flag is no longer cleared, breaking subsequent alarm wakeups.
- [High] Alarm interrupts notify the PM core before the device is configured as wakeup capable, causing lost wake events.
- [Medium] Software state caching updates before hardware programming succeeds, causing permanent desynchronization on I2C failure.
--

--- Patch [4]: [PATCH v2 4/5] rtc: s35390a: Add pinctrl ---
commit 58cfab3a8ceba177a47650a06a107e0c784e6fff
Author: Markus Probst <markus.probst@posteo.de>
Subject: rtc: s35390a: Add pinctrl

Allow configuration of other output modes than wake alarm, including:
keeping the previous configured mode, disabling output, custom frequency,
minute periodical interrupt, and raw 32.768 kHz output.

> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -668,7 +668,10 @@ config RTC_DRV_RC5T619
>  
>  config RTC_DRV_S35390A
>  	tristate "Seiko Instruments S-35390A"
> +	depends on OF

[Severity: High]
Does adding the strict OF dependency here break the driver for ACPI-based x86
systems? 

The driver explicitly supports DMI quirks for such systems, and this might
prevent compilation for previously supported non-OF hardware like Synology NAS
platforms.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c
[ ... ]
> @@ -283,33 +309,33 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  		alm->time.tm_min, alm->time.tm_hour, alm->time.tm_mday,
>  		alm->time.tm_mon, alm->time.tm_year, alm->time.tm_wday);
>  
> -	/* disable interrupt (which deasserts the irq line) */
> -	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
> -	if (err < 0)
> -		return err;
> +	guard(mutex)(&s35390a->mode_lock);
>  
> -	/* clear pending interrupt (in STATUS1 only), if any */
> -	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, &sts, sizeof(sts));
> +	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
>  	if (err < 0)
>  		return err;

[Severity: High]
Is it intentional to remove the read of S35390A_CMD_STATUS1 here?

By removing this read, the RTC chip's internal pending interrupt flag might 
never be cleared when a new alarm is set. Can this cause the physical WAKEUP 
pin to remain asserted, preventing the host from detecting subsequent alarm 
edges?

[ ... ]
> @@ -388,10 +445,10 @@ static int s35390a_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled
>  	struct s35390a *s35390a = dev_get_drvdata(dev);
>  	struct rtc_wkalrm alm;
>  
> -	alm.enabled = enabled;
> -
>  	if (enabled)
> -		alm.time = s35390a->tm_alarm;
> +		alm = s35390a->alarm;
> +
> +	alm.enabled = enabled;
>  
>  	return s35390a_rtc_set_alarm(dev, &alm);
>  }

[Severity: High]
If enabled is 0, is the stack-allocated alm struct left uninitialized except
for the enabled field?

If RTC_AIE_OFF is called, this uninitialized memory is passed to
s35390a_rtc_set_alarm() where it is copied into the permanent device state
s35390a->alarm. A subsequent RTC_ALM_READ could then copy this uninitialized
kernel stack memory to userspace via s35390a_rtc_read_alarm(). Does this
create an information leak?

[ ... ]
> +static int s35390a_update_mode(struct s35390a *s35390a, int pin, int mode, int freq)
> +{
[ ... ]
> +	if (mode == s35390a->mode[pin] && freq == s35390a->freq[pin])
> +		return 0;
> +
> +	s35390a->mode[pin] = mode;
> +	s35390a->freq[pin] = freq;
> +
> +	if (mode == S35390A_MODE_IGNORE)
> +		return 0;
> +
> +	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
> +	if (err < 0) {
> +		dev_err(&s35390a->client[0]->dev, "error reading status\n");
> +		return err;
> +	}

[Severity: Medium]
Is it safe to update the software state (s35390a->mode and freq) before
confirming the hardware configuration via I2C succeeds?

If s35390a_get_reg() or a later I2C operation fails, the driver state will
incorrectly reflect the new mode. Future attempts to set this same mode would
then be skipped by the check above, permanently desynchronizing the software
state from the hardware until a different mode is applied. Should the state
update happen at the end of the function after hardware success?

[ ... ]
> +static int s35390a_pinconf_set(struct pinctrl_dev *pctldev,
> +			       unsigned int pin, unsigned long *configs,
> +			       unsigned int num_configs)
> +{
[ ... ]
> +	for (i = 0; i < num_configs; i++) {
> +		param = pinconf_to_config_param(configs[0]);
> +		param_val = pinconf_to_config_argument(configs[0]);

[Severity: High]
Does this loop intend to use configs[i] instead of configs[0]?

As written, when multiple pin configurations are defined in the device tree,
the loop will repeatedly process the first configuration while ignoring all
subsequent properties.

[ ... ]
> @@ -477,7 +762,10 @@ static int s35390a_probe(struct i2c_client *client)
[ ... ]
> +	err = devm_pinctrl_register_and_init(dev, &s35390a_pinctrl_desc, s35390a,
> +					     &s35390a->pinctrl);
> +	if (err)
> +		return err;

[Severity: High]
Is a call to pinctrl_enable() missing here?

After devm_pinctrl_register_and_init(), the pinctrl device remains in the
initialization state. Without pinctrl_enable(), it may never be fully
registered or exposed to the pinctrl core, rendering the feature
non-functional.

> +
> +	/* If no pinctrl config is defined in DT, fallback to previous behaviour */
> +	if (!s35390a->mode_init) {
> +		err = s35390a_update_mode(s35390a, 1, S35390A_MODE_WAKEUP, 0);
> +		if (err)
> +			return err;
> +	}
> +
>  	err_read = s35390a_read_status(s35390a, &status1);
>  	if (err_read < 0) {
>  		dev_err(dev, "error resetting chip\n");
> @@ -507,34 +822,8 @@ static int s35390a_probe(struct i2c_client *client)
>  	else
>  		s35390a->twentyfourhour = 0;
>  
>  	device_set_wakeup_capable(dev, 1);

[Severity: High]
Can s35390a_update_mode() trigger rtc_update_irq() prematurely in the probe
sequence?

If an alarm fired while the system was off, s35390a_update_mode() calls
rtc_update_irq(), which relies on pm_stay_awake(). However, since
device_set_wakeup_capable(dev, 1) hasn't been called yet, the PM core may
drop the wakeup notification. Additionally, the queued IRQ work might read
the time before s35390a->twentyfourhour is initialized.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-rtc_s35390a_int1-v2-0-f10c99ad1d6c@posteo.de?part=4