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

[PATCH v3 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 function 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 v3:
- fix issues reported by Sashiko
  - fix use of uninitialized time data
  - remove dependence on `CONFIG_OF`
  - add missing `pinctrl_enable` call
  - fix wrong index used in `s35390a_pinconf_set`
  - fix interrupt not cleared
  - fix `device_set_wakeup_capable` called to late in probe
  - fix mode caching even on failure
  - fix failure on synology quirk mode update ignored
- only set irq in alarm_irq_enable callback
- fix alarm time not set if set_alarm is called with alarm disabled
- use pinmux instead of pinconf
- move patternProperties below properties in dt
- remove dt-bindings header
- add '#clock-cells' dt property
- merge 32768khz and user frequency mode into "clock"
- remove alarm mode
- refer to mode now as function in the code, to match phrasing in pinmux
- resolve checkpatches --strict warnings
- remove mode_init bool and instead set initial function to -1
- remove err_read probe variable and reuse err
- rebase onto v6.2-rc6
- Link to v2: https://patch.msgid.link/20260801-rtc_s35390a_int1-v2-0-f10c99ad1d6c@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       | 109 ++++++
 .../devicetree/bindings/rtc/trivial-rtc.yaml       |   3 -
 MAINTAINERS                                        |   1 +
 drivers/rtc/Kconfig                                |   1 +
 drivers/rtc/rtc-s35390a.c                          | 413 +++++++++++++++++----
 5 files changed, 461 insertions(+), 66 deletions(-)
---
base-commit: d7dd96eb916519208210bb4a0408fcf4f7fdce5d
change-id: 20260630-rtc_s35390a_int1-556ccb308d3f

[PATCH v3 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       | 109 +++++++++++++++++++++
 .../devicetree/bindings/rtc/trivial-rtc.yaml       |   3 -
 MAINTAINERS                                        |   1 +
 3 files changed, 110 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..0355f17f233a
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml
@@ -0,0 +1,109 @@
+# 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
+
+  "#clock-cells":
+    const: 1
+
+patternProperties:
+  "^pins":
+    type: object
+    patternProperties:
+      "-pins$":
+        type: object
+        properties:
+          pins:
+            $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/pins
+            items:
+              enum:
+                - int1
+                - int2
+
+          function:
+            $ref: /schemas/types.yaml#/definitions/string
+            description: |
+              Pin function:
+              - ignore:  Preserve the previous state.
+              - disable: Disable pin output.
+              - wakeup:  Output wakes up the system.
+              - clock:   Output clock pulse.
+              - pmin1:   Minute periodical output with 50% duty.
+              - pmin2:   Minute periodical output L for 7.81 ms.
+                         Can only be used with pin int1.
+            enum:
+              - ignore
+              - disable
+              - wakeup
+              - clock
+              - pmin1
+              - pmin2
+
+        required:
+          - function
+
+        additionalProperties: false
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        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 = "int1";
+                function = "wakeup";
+              };
+              int2-pins {
+                pins = "int2";
+                function = "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 5114e6db7307..4c8bc35cfe55 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22695,6 +22695,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

-- 
2.54.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The schema describes a hardware constraint for the 'pmin2' function but fails to enforce it programmatically in the YAML logic.
- [Medium] The patch makes an undocumented, unrelated modification to the MAINTAINERS file by adding a non-existent directory.
- [Medium] The pinctrl child node pattern `-pins$` requires the `function` property but omits the `pins` property from the `required` list.
--

--- Patch [1]: [PATCH v3 1/5] dt-bindings: rtc: Add pinctrl for S35390A ---
commit 7fb91877359a278935b0893edcef6e5ae7d9c6a2
Author: Markus Probst <markus.probst@posteo.de>

    dt-bindings: rtc: Add pinctrl for S35390A
    
    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.

> diff --git a/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml b/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml
> new file mode 100644
> index 0000000000000..0355f17f233a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml
[ ... ]
> +          function:
> +            $ref: /schemas/types.yaml#/definitions/string
> +            description: |
> +              Pin function:
> +              - ignore:  Preserve the previous state.
> +              - disable: Disable pin output.
> +              - wakeup:  Output wakes up the system.
> +              - clock:   Output clock pulse.
> +              - pmin1:   Minute periodical output with 50% duty.
> +              - pmin2:   Minute periodical output L for 7.81 ms.
> +                         Can only be used with pin int1.

[Severity: Medium]
Could this hardware constraint be enforced programmatically in the YAML schema?

As currently written, a device tree specifying function = "pmin2" for 
pins = "int2" would silently pass dt_binding_check validation despite violating 
the documented hardware constraints. Should this be enforced using conditional 
schema logic?

> +            enum:
> +              - ignore
> +              - disable
> +              - wakeup
> +              - clock
> +              - pmin1
> +              - pmin2
> +
> +        required:
> +          - function

[Severity: Medium]
Should the pins property also be required here?

If a developer creates a pinmux child node specifying a function but omits the 
mandatory pins identifier, the incomplete node will silently pass 
dt_binding_check validation but fail to configure the hardware correctly in 
the driver.

> +
> +        additionalProperties: false
> +
> +    additionalProperties: false
[ ... ]
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 15011f5752a99..46a19a0873e06 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22690,6 +22690,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/

[Severity: Medium]
Is this addition to the MAINTAINERS file intentional?

This introduces an undocumented and unrelated modification adding a directory 
that does not actually exist in the kernel tree, which may cause maintainer 
scripts to fail or behave unexpectedly.

>  F:	include/linux/rtc.h
>  F:	include/linux/rtc/
>  F:	include/uapi/linux/rtc.h

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-rtc_s35390a_int1-v3-0-e836faba3e25@posteo.de?part=1

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

[Markus Probst]

[-- Attachment #1: Type: text/plain, Size: 5130 bytes --]

On Wed, 2026-08-05 at 19:32 +0000, Markus Probst wrote:
> 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       | 109 +++++++++++++++++++++
>  .../devicetree/bindings/rtc/trivial-rtc.yaml       |   3 -
>  MAINTAINERS                                        |   1 +
>  3 files changed, 110 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..0355f17f233a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml
> @@ -0,0 +1,109 @@
> +# 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
> +
> +  "#clock-cells":
> +    const: 1
> +
> +patternProperties:
> +  "^pins":
> +    type: object
> +    patternProperties:
> +      "-pins$":
> +        type: object
> +        properties:
> +          pins:
> +            $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/pins
> +            items:
> +              enum:
> +                - int1
> +                - int2
> +
> +          function:
> +            $ref: /schemas/types.yaml#/definitions/string
> +            description: |
> +              Pin function:
> +              - ignore:  Preserve the previous state.
> +              - disable: Disable pin output.
> +              - wakeup:  Output wakes up the system.
> +              - clock:   Output clock pulse.
> +              - pmin1:   Minute periodical output with 50% duty.
> +              - pmin2:   Minute periodical output L for 7.81 ms.
> +                         Can only be used with pin int1.
> +            enum:
> +              - ignore
> +              - disable
> +              - wakeup
> +              - clock
> +              - pmin1
> +              - pmin2
> +
> +        required:
> +          - function
> +
> +        additionalProperties: false
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        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 = "int1";
> +                function = "wakeup";
> +              };
> +              int2-pins {
> +                pins = "int2";
> +                function = "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 5114e6db7307..4c8bc35cfe55 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22695,6 +22695,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
This hunk should be removed.

Thanks
- Markus Probst


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

[PATCH v3 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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Desynchronization between software state and hardware 24-hour mode after initialization causes time corruption.
- [High] Alarm hour formatting erroneously adds PM bit in 24-hour mode, causing PM alarms to permanently fail.
--

--- Patch [2]: [PATCH v3 2/5] rtc: s35390a: Add missing newline to dev_err ---
commit 60df926a63720bd3dbdd3b4991c3149af682d4b0
Author: Markus Probst <markus.probst@posteo.de>

rtc: s35390a: Add missing newline to dev_err

This commit fixes a missing newline character in the dev_err call
within s35390a_probe() when alarm disabling fails.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index b72eef4fb0997..4cfe7034c5165 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 {

While reviewing this driver, I noticed a couple of pre-existing issues.

[Severity: High]
This isn't a bug introduced by this patch, but looking at s35390a_init(),
is it possible for the software state to become desynchronized from the
hardware 24-hour mode?

drivers/rtc/rtc-s35390a.c:s35390a_init() {
    ...
    initialize:
	buf = S35390A_FLAG_RESET | S35390A_FLAG_24H;
	ret = s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, &buf, 1);
    ...
}

When this reset is triggered (for instance, via the RTC_VL_CLR ioctl or when
a brownout is detected), the hardware is explicitly switched to 24-hour mode
by setting S35390A_FLAG_24H.

However, s35390a->twentyfourhour is not updated to 1.

Does this mean subsequent calls to rtc_set_time() will incorrectly use the
12-hour formatting rules if twentyfourhour was previously 0, sending invalid
BCD formats to the chip and corrupting the timekeeping for PM hours?


[Severity: High]
This is a pre-existing issue, but I have a question about how the alarm hour
is formatted when the chip is operating in 24-hour mode:

drivers/rtc/rtc-s35390a.c:s35390a_rtc_set_alarm() {
    ...
	buf[S35390A_ALRM_BYTE_HOURS] = s35390a_hr2reg(s35390a,
			alm->time.tm_hour) | 0x80;
	buf[S35390A_ALRM_BYTE_MINS] = bin2bcd(alm->time.tm_min) | 0x80;

	if (alm->time.tm_hour >= 12)
		buf[S35390A_ALRM_BYTE_HOURS] |= 0x40;
    ...
}

If the chip is in 24-hour mode, s35390a_hr2reg() will already return the
standard 24-hour BCD value (e.g., 0x15 for 15:00).

Could the unconditional bitwise-OR with 0x40 (the PM bit) corrupt the alarm
time value in 24-hour mode, causing PM alarms to permanently fail because the
hardware comparator expects a standard 24-hour BCD value?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-rtc_s35390a_int1-v3-0-e836faba3e25@posteo.de?part=2

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

[Markus Probst]

Implement alarm_irq_enable callback.

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 | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 4cfe7034c516..575bb256eb25 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -270,6 +270,24 @@ static int s35390a_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	return 0;
 }
 
+static int s35390a_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+	struct s35390a *s35390a = dev_get_drvdata(dev);
+	u8 sts;
+	int err;
+
+	if (enabled)
+		sts = S35390A_INT2_MODE_ALARM;
+	else
+		sts = S35390A_INT2_MODE_NOINTR;
+
+	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
 static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -410,11 +428,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

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

[sashiko-bot]

> Implement alarm_irq_enable callback.
> 
> Fixes: 542dd33a4925 ("drivers/rtc/rtc-s35390a.c: add wakealarm support for rtc-s35390A rtc chip")
> 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/20260805-rtc_s35390a_int1-v3-0-e836faba3e25@posteo.de?part=3

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

[Markus Probst]

Allow configuration of pin functions, including:
- keeping the previous configured function
- disabling output
- waking up the system
- minute periodical interrupt (1: 50% duty, 2: 7.81 ms)

If no function has been configured, fallback to previous behaviour.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 drivers/rtc/Kconfig       |   1 +
 drivers/rtc/rtc-s35390a.c | 381 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 320 insertions(+), 62 deletions(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 01def8231873..4e0374129a4e 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -668,6 +668,7 @@ config RTC_DRV_RC5T619
 
 config RTC_DRV_S35390A
 	tristate "Seiko Instruments S-35390A"
+	depends on PINCTRL
 	select BITREVERSE
 	help
 	  If you say yes here you will get support for the Seiko
diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 575bb256eb25..d63bdeff8cdb 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -12,11 +12,16 @@
 #include <linux/bcd.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.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 +41,38 @@
 #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 */
+
+#define S35390A_FUNC_IGNORE		0x00
+#define S35390A_FUNC_DISABLE		0x01
+#define S35390A_FUNC_WAKEUP		0x02
+#define S35390A_FUNC_CLOCK		0x03
+#define S35390A_FUNC_PMIN1		0x04
+#define S35390A_FUNC_PMIN2		0x05
+
 
 static const struct i2c_device_id s35390a_id[] = {
 	{ .name = "s35390a" },
@@ -64,7 +88,11 @@ MODULE_DEVICE_TABLE(of, s35390a_of_match);
 
 struct s35390a {
 	struct i2c_client *client[8];
+	struct rtc_device *rtc;
 	int twentyfourhour;
+
+	struct mutex pinfunction_lock; /* lock preventing concurrent access of pin function */
+	int pinfunction[2];
 };
 
 static int s35390a_set_reg(struct s35390a *s35390a, int reg, u8  *buf, int len)
@@ -276,10 +304,25 @@ static int s35390a_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled
 	u8 sts;
 	int err;
 
-	if (enabled)
-		sts = S35390A_INT2_MODE_ALARM;
-	else
-		sts = S35390A_INT2_MODE_NOINTR;
+	guard(mutex)(&s35390a->pinfunction_lock);
+
+	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
+	if (err < 0)
+		return err;
+
+	if (enabled) {
+		if (s35390a->pinfunction[0] == S35390A_FUNC_WAKEUP)
+			sts = (sts & ~S35390A_INT1_MODE_MASK) | S35390A_INT1_MODE_ALARM;
+
+		if (s35390a->pinfunction[1] == S35390A_FUNC_WAKEUP)
+			sts = (sts & ~S35390A_INT2_MODE_MASK) | S35390A_INT2_MODE_ALARM;
+	} else {
+		if (s35390a->pinfunction[0] == S35390A_FUNC_WAKEUP)
+			sts = (sts & ~S35390A_INT1_MODE_MASK) | S35390A_INT_MODE_NOINTR;
+
+		if (s35390a->pinfunction[1] == S35390A_FUNC_WAKEUP)
+			sts = (sts & ~S35390A_INT2_MODE_MASK) | S35390A_INT_MODE_NOINTR;
+	}
 
 	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
 	if (err < 0)
@@ -292,7 +335,7 @@ static int s35390a_rtc_set_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 = 0;
+	u8 buf[3], sts = 0, tmp;
 	int err, i;
 
 	dev_dbg(&client->dev, "%s: alm is secs=%d, mins=%d, hours=%d mday=%d, "\
@@ -300,33 +343,35 @@ 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->pinfunction_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->pinfunction[0] == S35390A_FUNC_WAKEUP)
+		sts = (sts & ~S35390A_INT1_MODE_MASK) | S35390A_INT_MODE_NOINTR;
+
+	if (s35390a->pinfunction[1] == S35390A_FUNC_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;
 
+	/* clear pending interrupt (in STATUS1 only), if any */
+	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, &tmp, sizeof(tmp));
+	if (err < 0)
+		return err;
+
 	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)
@@ -335,10 +380,32 @@ 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));
+	if (alm->enabled) {
+		/* set interrupt mode */
+		if (s35390a->pinfunction[0] == S35390A_FUNC_WAKEUP)
+			sts = (sts & ~S35390A_INT1_MODE_MASK) | S35390A_INT1_MODE_ALARM;
+
+		if (s35390a->pinfunction[1] == S35390A_FUNC_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->pinfunction[0] == S35390A_FUNC_WAKEUP) {
+		err = s35390a_set_reg(s35390a, S35390A_CMD_INT1_REG1, buf, sizeof(buf));
+		if (err < 0)
+			return err;
+	}
+
+	if (s35390a->pinfunction[1] == S35390A_FUNC_WAKEUP) {
+		err = s35390a_set_reg(s35390a, S35390A_CMD_INT2_REG1, buf, sizeof(buf));
+		if (err < 0)
+			return err;
+	}
 
-	return err;
+	return 0;
 }
 
 static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
@@ -346,24 +413,32 @@ 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->pinfunction_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->pinfunction[1] == S35390A_FUNC_WAKEUP &&
+	    (sts & S35390A_INT2_MODE_MASK) == S35390A_INT2_MODE_ALARM) {
+		reg = S35390A_CMD_INT2_REG1;
+	} else if (s35390a->pinfunction[0] == S35390A_FUNC_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;
 		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;
 
@@ -372,7 +447,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)
@@ -453,13 +528,181 @@ static int s35390a_nvmem_write(void *priv, unsigned int offset, void *val,
 	return s35390a_set_reg(s35390a, S35390A_CMD_FREE_REG, val, bytes);
 }
 
+static const struct pinctrl_pin_desc s35390a_pins_desc[] = {
+	PINCTRL_PIN(0, "int1"),
+	PINCTRL_PIN(1, "int2"),
+};
+
+static const unsigned int int1_pins[] = { 0 };
+static const unsigned int int2_pins[] = { 1 };
+
+static const struct pingroup s35390a_pin_groups[] = {
+	PINCTRL_PINGROUP("int1_grp", int1_pins, ARRAY_SIZE(int1_pins)),
+	PINCTRL_PINGROUP("int2_grp", int2_pins, ARRAY_SIZE(int2_pins)),
+};
+
+static int s35390a_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(s35390a_pin_groups);
+}
+
+static const char *s35390a_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
+						  unsigned int group)
+{
+	return s35390a_pin_groups[group].name;
+}
+
+static int s35390a_pinctrl_get_group_pins(struct pinctrl_dev *pctldev, unsigned int selector,
+					  const unsigned int **pins, unsigned int *npins)
+{
+	*pins = s35390a_pin_groups[selector].pins;
+	*npins = s35390a_pin_groups[selector].npins;
+	return 0;
+}
+
+static const char * const all_groups[] = { "int1_grp", "int2_grp" };
+static const char * const int1_groups[] = { "int1_grp" };
+
+static const struct pinfunction s35390a_functions[] = {
+	[S35390A_FUNC_IGNORE] = PINCTRL_PINFUNCTION("ignore", all_groups, ARRAY_SIZE(all_groups)),
+	[S35390A_FUNC_DISABLE] = PINCTRL_PINFUNCTION("disable", all_groups, ARRAY_SIZE(all_groups)),
+	[S35390A_FUNC_WAKEUP] = PINCTRL_PINFUNCTION("wakeup", all_groups, ARRAY_SIZE(all_groups)),
+	[S35390A_FUNC_CLOCK] = PINCTRL_PINFUNCTION("clock", all_groups, ARRAY_SIZE(all_groups)),
+	[S35390A_FUNC_PMIN1] = PINCTRL_PINFUNCTION("pmin1", all_groups, ARRAY_SIZE(all_groups)),
+	[S35390A_FUNC_PMIN2] = PINCTRL_PINFUNCTION("pmin2", int1_groups, ARRAY_SIZE(int1_groups)),
+};
+
+static int s35390a_pinctrl_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(s35390a_functions);
+}
+
+static const char *s35390a_pinctrl_get_function_name(struct pinctrl_dev *pctldev,
+						     unsigned int selector)
+{
+	return s35390a_functions[selector].name;
+}
+
+static int s35390a_pinctrl_get_function_groups(struct pinctrl_dev *pctldev, unsigned int selector,
+					       const char * const **groups,
+					       unsigned int * const ngroups)
+{
+	*groups = s35390a_functions[selector].groups;
+	*ngroups = s35390a_functions[selector].ngroups;
+	return 0;
+}
+
+static int s35390a_pinctrl_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
+				   unsigned int group)
+{
+	int err;
+	u8 buf, status1, flag, mask;
+	bool update_irq = false;
+	struct s35390a *s35390a = pinctrl_dev_get_drvdata(pctldev);
+
+	mask = group == 0 ? S35390A_INT1_MODE_MASK : S35390A_INT2_MODE_MASK;
+
+	guard(mutex)(&s35390a->pinfunction_lock);
+
+	dev_dbg(&s35390a->client[0]->dev, "%s: function=%d group=%d\n",
+		__func__, function, group);
+
+	if (function == s35390a->pinfunction[group])
+		return 0;
+
+	if (function == S35390A_FUNC_IGNORE)
+		goto end;
+
+	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 (function) {
+	case S35390A_FUNC_DISABLE:
+	case S35390A_FUNC_CLOCK: /* not implemented */
+		buf = (buf & ~mask) | S35390A_INT_MODE_NOINTR;
+		break;
+	case S35390A_FUNC_WAKEUP:
+		flag = group == 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 = group == 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_FUNC_PMIN1:
+		flag = group == 0 ? S35390A_INT1_MODE_PMIN1 : S35390A_INT2_MODE_PMIN1;
+		buf = (buf & ~mask) | flag;
+		break;
+
+	/* INT1 only modes */
+	case S35390A_FUNC_PMIN2:
+		buf = (buf & ~mask) | S35390A_INT1_MODE_PMIN2;
+		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 (update_irq)
+		rtc_update_irq(s35390a->rtc, 1, RTC_AF);
+
+end:
+	s35390a->pinfunction[group] = function;
+
+	return 0;
+}
+
+static const struct pinctrl_ops s35390a_pinctrl_ops = {
+	.get_groups_count = s35390a_pinctrl_get_groups_count,
+	.get_group_name = s35390a_pinctrl_get_group_name,
+	.get_group_pins = s35390a_pinctrl_get_group_pins,
+};
+
+static const struct pinmux_ops s35390a_pinmux_ops = {
+	.get_functions_count = s35390a_pinctrl_get_functions_count,
+	.get_function_name = s35390a_pinctrl_get_function_name,
+	.get_function_groups = s35390a_pinctrl_get_function_groups,
+	.set_mux = s35390a_pinctrl_set_mux,
+	.strict = true,
+};
+
+static struct pinctrl_desc s35390a_pinctrl_desc = {
+	.name = DRIVER_NAME,
+	.pins = s35390a_pins_desc,
+	.npins = ARRAY_SIZE(s35390a_pins_desc),
+	.pctlops = &s35390a_pinctrl_ops,
+	.pmxops = &s35390a_pinmux_ops,
+	.owner = THIS_MODULE,
+};
+
 static int s35390a_probe(struct i2c_client *client)
 {
-	int err, err_read;
+	int err;
 	unsigned int i;
 	struct s35390a *s35390a;
 	struct rtc_device *rtc;
-	u8 buf, status1;
+	struct pinctrl_dev *pctl;
+	u8 status1;
 	struct device *dev = &client->dev;
 	struct nvmem_config nvmem_cfg = {
 		.name = "s35390a_nvram",
@@ -470,6 +713,7 @@ static int s35390a_probe(struct i2c_client *client)
 		.reg_read = s35390a_nvmem_read,
 		.reg_write = s35390a_nvmem_write,
 	};
+	int fallback[ARRAY_SIZE(s35390a_pin_groups)];
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
@@ -478,7 +722,11 @@ static int s35390a_probe(struct i2c_client *client)
 	if (!s35390a)
 		return -ENOMEM;
 
+	mutex_init(&s35390a->pinfunction_lock);
+	memset(s35390a->pinfunction, -1, sizeof(s35390a->pinfunction));
+
 	s35390a->client[0] = client;
+
 	i2c_set_clientdata(client, s35390a);
 
 	/* This chip uses multiple addresses, use dummy devices for them */
@@ -493,39 +741,16 @@ 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);
 
-	err_read = s35390a_read_status(s35390a, &status1);
-	if (err_read < 0) {
-		dev_err(dev, "error resetting chip\n");
-		return err_read;
-	}
-
-	if (status1 & S35390A_FLAG_24H)
-		s35390a->twentyfourhour = 1;
-	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;
@@ -533,8 +758,40 @@ static int s35390a_probe(struct i2c_client *client)
 	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);
+	s35390a->rtc = rtc;
+
+	device_set_wakeup_capable(dev, 1);
+
+	err = devm_pinctrl_register_and_init(dev, &s35390a_pinctrl_desc, s35390a, &pctl);
+	if (err)
+		return dev_err_probe(dev, err, "pinctrl register failed\n");
+
+	err = pinctrl_enable(pctl);
+	if (err)
+		return dev_err_probe(dev, err, "pinctrl enable failed\n");
+
+	/* If no pinmux function is defined in DT, fallback to previous behaviour */
+	fallback[0] = S35390A_FUNC_IGNORE;
+	fallback[1] = S35390A_FUNC_WAKEUP;
+
+	for (i = 0; i < ARRAY_SIZE(s35390a_pin_groups); i++) {
+		if (s35390a->pinfunction[i] == -1) {
+			err = s35390a_pinctrl_set_mux(pctl, fallback[i], i);
+			if (err)
+				return err;
+		}
+	}
+
+	err = s35390a_read_status(s35390a, &status1);
+	if (err < 0) {
+		dev_err(dev, "error resetting chip\n");
+		return err;
+	}
+
+	if (status1 & S35390A_FLAG_24H)
+		s35390a->twentyfourhour = 1;
+	else
+		s35390a->twentyfourhour = 0;
 
 	nvmem_cfg.priv = s35390a;
 	err = devm_rtc_nvmem_register(rtc, &nvmem_cfg);
@@ -546,7 +803,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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver initiates I2C communication with the hardware before honoring the required Power-On Circuit (POC) delay, violating the initialization sequence.
- [High] The pinctrl driver cannot parse device tree pin configuration nodes because it is missing the `dt_node_to_map` callback.
- [High] Reading the `STATUS1` register to check or clear pending alarms inadvertently clears the alarm flags for both pins, causing lost wakeup events.
- [Medium] The driver introduces a hard dependency on `CONFIG_PINCTRL`, breaking existing kernel configurations.
--

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

Allow configuration of pin functions, including keeping previous config,
disabling output, waking up, and minute periodical interrupt. Fallback to
previous behavior if not configured.

> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 01def82318731..4e0374129a4ed 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -668,6 +668,7 @@ config RTC_DRV_RC5T619
>  
>  config RTC_DRV_S35390A
>  	tristate "Seiko Instruments S-35390A"
> +	depends on PINCTRL

[Severity: Medium]
Since the kernel pinctrl subsystem provides dummy stubs when CONFIG_PINCTRL is
disabled, should this be a hard dependency? This will prevent building the
driver on platforms without pinctrl support, breaking existing configurations.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index 575bb256eb258..d63bdeff8cdbc 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c
[ ... ]
> @@ -343,33 +343,35 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
[ ... ]
> +	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
> +	if (err < 0)
> +		return err;
> +
> +	/* clear pending interrupt (in STATUS1 only), if any */
> +	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, &tmp, sizeof(tmp));
> +	if (err < 0)
> +		return err;

[Severity: High]
Since reading STATUS1 acts as a clear-on-read for both INT1 and INT2 interrupt
flags, won't this unconditional read wipe out any pending interrupts for the
other pin?

[ ... ]
> @@ -453,13 +528,181 @@ static int s35390a_nvmem_write(void *priv, unsigned int offset, void *val,
[ ... ]
> +	case S35390A_FUNC_WAKEUP:
> +		flag = group == 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);

[Severity: High]
Similarly, if s35390a_pinctrl_set_mux() is called twice during initialization
(once for each pin), won't the first call to s35390a_read_status() clear the
flags for both pins, causing the second call to lose any pending wakeup event?

[ ... ]
> +static const struct pinctrl_ops s35390a_pinctrl_ops = {
> +	.get_groups_count = s35390a_pinctrl_get_groups_count,
> +	.get_group_name = s35390a_pinctrl_get_group_name,
> +	.get_group_pins = s35390a_pinctrl_get_group_pins,
> +};

[Severity: High]
Is this struct missing the dt_node_to_map callback (e.g.,
pinconf_generic_dt_node_to_map_all)? Without it, the pinctrl subsystem will fail
with -ENODEV during pinctrl_dt_to_map(), which ignores any custom pin
multiplexing requested via device tree.

[ ... ]
> @@ -493,39 +741,16 @@ 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;
> +	}
[ ... ]
> +	err = devm_pinctrl_register_and_init(dev, &s35390a_pinctrl_desc, s35390a, &pctl);
> +	if (err)
> +		return dev_err_probe(dev, err, "pinctrl register failed\n");
> +
> +	err = pinctrl_enable(pctl);
> +	if (err)
> +		return dev_err_probe(dev, err, "pinctrl enable failed\n");
[ ... ]
> +	err = s35390a_read_status(s35390a, &status1);
> +	if (err < 0) {
> +		dev_err(dev, "error resetting chip\n");
> +		return err;
> +	}

[Severity: High]
Does moving the s35390a_read_status() call to later in the probe sequence violate
the Power-On Circuit delay?

The s35390a_disable_test_mode() and pinctrl_enable() calls both trigger I2C
communication. If the hardware is still in the 0.5-second POC delay period,
communicating with it before checking the POC flag in s35390a_read_status()
might lead to I2C bus hangs or silent failures on cold boot.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-rtc_s35390a_int1-v3-0-e836faba3e25@posteo.de?part=4

[PATCH v3 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 pinmux function 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 | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index d63bdeff8cdb..2537bd8fe3e8 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>
@@ -771,8 +772,18 @@ static int s35390a_probe(struct i2c_client *client)
 		return dev_err_probe(dev, err, "pinctrl enable failed\n");
 
 	/* If no pinmux function is defined in DT, fallback to previous behaviour */
-	fallback[0] = S35390A_FUNC_IGNORE;
-	fallback[1] = S35390A_FUNC_WAKEUP;
+
+	if (dmi_match(DMI_SYS_VENDOR, "Synology Inc.")) {
+		/*
+		 * Synology uses interrupt signal 1 for wakeup. Since pinctrl cannot be
+		 * configured on ACPI, we check against the dmi sys vendor.
+		 */
+		fallback[0] = S35390A_FUNC_WAKEUP;
+		fallback[1] = S35390A_FUNC_DISABLE;
+	} else {
+		fallback[0] = S35390A_FUNC_IGNORE;
+		fallback[1] = S35390A_FUNC_WAKEUP;
+	}
 
 	for (i = 0; i < ARRAY_SIZE(s35390a_pin_groups); i++) {
 		if (s35390a->pinfunction[i] == -1) {

-- 
2.54.0

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

[sashiko-bot]

> 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 pinmux function has been
> configured in devicetree and dmi sys vendor matches "Synology Inc.".
> 
> 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/20260805-rtc_s35390a_int1-v3-0-e836faba3e25@posteo.de?part=5