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

[PATCH v5 0/6] 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 v5:
- fix issues reported by Sashiko
  - fix time corruption on setting 24-hour mode
  - fix setting 24-hour even if uninitialized
- Link to v4: https://patch.msgid.link/20260815-rtc_s35390a_int1-v4-0-3133309c365f@posteo.de

Changes in v4:
- fix issues reported by Sashiko
  - require "pins" property in dt
  - remove maintainers entry for include/dt-bindings/rtc (which only
    existed in previous iterations)
  - fix multiple reads to status1, which clears interrupt flags
  - fix not waiting for Power-On Circuit check
  - fix missing pinctrl dt callbacks
- enforce minimum length of "pins" property in dt
- fix pmin2 on int2 affecting int1
- force use of 24-hour mode
- Link to v3: https://patch.msgid.link/20260805-rtc_s35390a_int1-v3-0-e836faba3e25@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 (6):
      dt-bindings: rtc: Add pinctrl for S35390A
      rtc: s35390a: Add missing newline to dev_err
      rtc: s35390a: Fix alarm not disabling
      rtc: s35390a: force 24-hour mode
      rtc: s35390a: Add pinctrl
      rtc: s35390a: Add synology quirk

 .../devicetree/bindings/rtc/sii,s35390a.yaml       | 111 +++++
 .../devicetree/bindings/rtc/trivial-rtc.yaml       |   3 -
 drivers/rtc/Kconfig                                |   2 +
 drivers/rtc/rtc-s35390a.c                          | 478 ++++++++++++++++-----
 4 files changed, 488 insertions(+), 106 deletions(-)
---
base-commit: 237a1c39e8dfd3e1c6f1f023eea37a48ec04cc63
change-id: 20260630-rtc_s35390a_int1-556ccb308d3f

[PATCH v5 1/6] 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       | 111 +++++++++++++++++++++
 .../devicetree/bindings/rtc/trivial-rtc.yaml       |   3 -
 2 files changed, 111 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..d9da3abbbd51
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml
@@ -0,0 +1,111 @@
+# 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
+            minItems: 1
+            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:
+          - pins
+          - 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
-
 ...

-- 
2.54.0

[PATCH v5 2/6] 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 v5 3/6] 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

[PATCH v5 4/6] rtc: s35390a: force 24-hour mode

[Markus Probst]

Instead of trying to remember the current 12/24-hour mode, which could
result in time corruption if out of sync, we force the use of the 24-hour
mode. Set to 24-hour mode if in 12-hour mode on startup and assume
24-hour mode. The 24-hour mode is already forced on reset.

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

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 575bb256eb25..b34a3f7e8476 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -64,7 +64,6 @@ MODULE_DEVICE_TABLE(of, s35390a_of_match);
 
 struct s35390a {
 	struct i2c_client *client[8];
-	int twentyfourhour;
 };
 
 static int s35390a_set_reg(struct s35390a *s35390a, int reg, u8  *buf, int len)
@@ -181,31 +180,6 @@ static int s35390a_disable_test_mode(struct s35390a *s35390a)
 	return s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, buf, sizeof(buf));
 }
 
-static char s35390a_hr2reg(struct s35390a *s35390a, int hour)
-{
-	if (s35390a->twentyfourhour)
-		return bin2bcd(hour);
-
-	if (hour < 12)
-		return bin2bcd(hour);
-
-	return 0x40 | bin2bcd(hour - 12);
-}
-
-static int s35390a_reg2hr(struct s35390a *s35390a, char reg)
-{
-	unsigned hour;
-
-	if (s35390a->twentyfourhour)
-		return bcd2bin(reg & 0x3f);
-
-	hour = bcd2bin(reg & 0x3f);
-	if (reg & 0x40)
-		hour += 12;
-
-	return hour;
-}
-
 static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -225,7 +199,7 @@ static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	buf[S35390A_BYTE_MONTH] = bin2bcd(tm->tm_mon + 1);
 	buf[S35390A_BYTE_DAY] = bin2bcd(tm->tm_mday);
 	buf[S35390A_BYTE_WDAY] = bin2bcd(tm->tm_wday);
-	buf[S35390A_BYTE_HOURS] = s35390a_hr2reg(s35390a, tm->tm_hour);
+	buf[S35390A_BYTE_HOURS] = bin2bcd(tm->tm_hour);
 	buf[S35390A_BYTE_MINS] = bin2bcd(tm->tm_min);
 	buf[S35390A_BYTE_SECS] = bin2bcd(tm->tm_sec);
 
@@ -256,7 +230,7 @@ static int s35390a_rtc_read_time(struct device *dev, struct rtc_time *tm)
 
 	tm->tm_sec = bcd2bin(buf[S35390A_BYTE_SECS]);
 	tm->tm_min = bcd2bin(buf[S35390A_BYTE_MINS]);
-	tm->tm_hour = s35390a_reg2hr(s35390a, buf[S35390A_BYTE_HOURS]);
+	tm->tm_hour = bcd2bin(buf[S35390A_BYTE_HOURS] & 0x3f);
 	tm->tm_wday = bcd2bin(buf[S35390A_BYTE_WDAY]);
 	tm->tm_mday = bcd2bin(buf[S35390A_BYTE_DAY]);
 	tm->tm_mon = bcd2bin(buf[S35390A_BYTE_MONTH]) - 1;
@@ -325,8 +299,7 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
 	else
 		buf[S35390A_ALRM_BYTE_WDAY] = 0;
 
-	buf[S35390A_ALRM_BYTE_HOURS] = s35390a_hr2reg(s35390a,
-			alm->time.tm_hour) | 0x80;
+	buf[S35390A_ALRM_BYTE_HOURS] = bin2bcd(alm->time.tm_hour) | 0x80;
 	buf[S35390A_ALRM_BYTE_MINS] = bin2bcd(alm->time.tm_min) | 0x80;
 
 	if (alm->time.tm_hour >= 12)
@@ -381,8 +354,7 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
 
 	if (buf[S35390A_ALRM_BYTE_HOURS] & 0x80)
 		alm->time.tm_hour =
-			s35390a_reg2hr(s35390a,
-				       buf[S35390A_ALRM_BYTE_HOURS] & ~0x80);
+			bcd2bin(buf[S35390A_ALRM_BYTE_HOURS] & 0x3f);
 
 	if (buf[S35390A_ALRM_BYTE_MINS] & 0x80)
 		alm->time.tm_min = bcd2bin(buf[S35390A_ALRM_BYTE_MINS] & ~0x80);
@@ -459,7 +431,7 @@ static int s35390a_probe(struct i2c_client *client)
 	unsigned int i;
 	struct s35390a *s35390a;
 	struct rtc_device *rtc;
-	u8 buf, status1;
+	u8 buf, status1, time[7];
 	struct device *dev = &client->dev;
 	struct nvmem_config nvmem_cfg = {
 		.name = "s35390a_nvram",
@@ -503,10 +475,31 @@ static int s35390a_probe(struct i2c_client *client)
 		return err_read;
 	}
 
-	if (status1 & S35390A_FLAG_24H)
-		s35390a->twentyfourhour = 1;
-	else
-		s35390a->twentyfourhour = 0;
+	if (!(status1 & S35390A_FLAG_24H) && err_read != 1) {
+		unsigned int hour;
+
+		status1 |= S35390A_FLAG_24H;
+
+		err = s35390a_get_reg(s35390a, S35390A_CMD_TIME1, time, sizeof(time));
+		if (err < 0)
+			return dev_err_probe(dev, err, "reading 12-hour time failed\n");
+
+		/* converting to 24-hour time */
+		time[S35390A_BYTE_HOURS] = bitrev8(time[S35390A_BYTE_HOURS]);
+		hour = bcd2bin(time[S35390A_BYTE_HOURS] & 0x3f);
+		if (time[S35390A_BYTE_HOURS] & 0x40)
+			hour += 12;
+
+		time[S35390A_BYTE_HOURS] = bitrev8(bin2bcd(hour));
+
+		err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, &status1, sizeof(status1));
+		if (err < 0)
+			return dev_err_probe(dev, err, "setting 24-hour mode failed\n");
+
+		err = s35390a_set_reg(s35390a, S35390A_CMD_TIME1, time, sizeof(time));
+		if (err < 0)
+			return dev_err_probe(dev, err, "setting 24-hour time failed\n");
+	}
 
 	if (status1 & S35390A_FLAG_INT2) {
 		/* disable alarm (and maybe test mode) */

-- 
2.54.0

[PATCH v5 5/6] 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       |   2 +
 drivers/rtc/rtc-s35390a.c | 383 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 318 insertions(+), 67 deletions(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 01def8231873..2d79710c4837 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -668,7 +668,9 @@ config RTC_DRV_RC5T619
 
 config RTC_DRV_S35390A
 	tristate "Seiko Instruments S-35390A"
+	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 b34a3f7e8476..bc783be1561e 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -12,11 +12,17 @@
 #include <linux/bcd.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf-generic.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 +42,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,6 +89,10 @@ MODULE_DEVICE_TABLE(of, s35390a_of_match);
 
 struct s35390a {
 	struct i2c_client *client[8];
+	struct rtc_device *rtc;
+
+	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)
@@ -166,20 +195,6 @@ static int s35390a_read_status(struct s35390a *s35390a, char *status1)
 	return 0;
 }
 
-static int s35390a_disable_test_mode(struct s35390a *s35390a)
-{
-	u8 buf[1];
-
-	if (s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, buf, sizeof(buf)) < 0)
-		return -EIO;
-
-	if (!(buf[0] & S35390A_FLAG_TEST))
-		return 0;
-
-	buf[0] &= ~S35390A_FLAG_TEST;
-	return s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, buf, sizeof(buf));
-}
-
 static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -250,10 +265,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)
@@ -266,7 +296,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, "\
@@ -274,26 +304,28 @@ 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
@@ -308,10 +340,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;
 
-	return err;
+		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 0;
 }
 
 static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
@@ -319,24 +373,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;
 
@@ -345,7 +407,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)
@@ -425,13 +487,170 @@ 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 status2, flag, mask;
+	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, &status2, 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 */
+		status2 = (status2 & ~mask) | S35390A_INT_MODE_NOINTR;
+		break;
+	case S35390A_FUNC_WAKEUP:
+		flag = group == 0 ? S35390A_INT1_MODE_ALARM : S35390A_INT2_MODE_ALARM;
+
+		if ((status2 & mask) != flag)
+			status2 = (status2 & ~mask) | S35390A_INT_MODE_NOINTR;
+
+		break;
+	case S35390A_FUNC_PMIN1:
+		flag = group == 0 ? S35390A_INT1_MODE_PMIN1 : S35390A_INT2_MODE_PMIN1;
+		status2 = (status2 & ~mask) | flag;
+		break;
+
+	/* INT1 only modes */
+	case S35390A_FUNC_PMIN2:
+		if (group == 1)
+			return -EINVAL;
+
+		status2 = (status2 & ~mask) | S35390A_INT1_MODE_PMIN2;
+		break;
+	}
+
+	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &status2, 1);
+	if (err < 0) {
+		dev_err(&s35390a->client[0]->dev, "error setting interrupts\n");
+		return err;
+	}
+
+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,
+#if IS_ENABLED(CONFIG_OF)
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
+	.dt_free_map = pinconf_generic_dt_free_map
+#endif
+};
+
+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, time[7];
+	struct pinctrl_dev *pctl;
+	u8 status1, status2, time[7];
+	bool irq = false;
 	struct device *dev = &client->dev;
 	struct nvmem_config nvmem_cfg = {
 		.name = "s35390a_nvram",
@@ -442,6 +661,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;
@@ -450,7 +670,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 */
@@ -465,17 +689,13 @@ static int s35390a_probe(struct i2c_client *client)
 		}
 	}
 
-	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) {
+	err = s35390a_read_status(s35390a, &status1);
+	if (err < 0) {
 		dev_err(dev, "error resetting chip\n");
-		return err_read;
+		return err;
 	}
 
-	if (!(status1 & S35390A_FLAG_24H) && err_read != 1) {
+	if (!(status1 & S35390A_FLAG_24H) && err != 1) {
 		unsigned int hour;
 
 		status1 |= S35390A_FLAG_24H;
@@ -501,23 +721,28 @@ static int s35390a_probe(struct i2c_client *client)
 			return dev_err_probe(dev, err, "setting 24-hour time failed\n");
 	}
 
+	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2));
+	if (err < 0)
+		return dev_err_probe(dev, err, "disabling alarm and test mode failed\n");
+
+	if (status1 & S35390A_FLAG_INT1) {
+		status2 = (status2 & ~S35390A_INT1_MODE_MASK) | S35390A_INT_MODE_NOINTR;
+		irq = true;
+	}
+
 	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;
-		}
+		status2 = (status2 & ~S35390A_INT2_MODE_MASK) | S35390A_INT_MODE_NOINTR;
+		irq = true;
 	}
 
-	device_set_wakeup_capable(dev, 1);
+	status2 &= ~S35390A_FLAG_TEST;
+	err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2));
+	if (err < 0)
+		return dev_err_probe(dev, err, "disabling alarm and test mode failed\n");
+
+	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;
@@ -526,9 +751,33 @@ 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)
+	s35390a->rtc = rtc;
+
+	device_set_wakeup_capable(dev, 1);
+
+	if (irq)
 		rtc_update_irq(rtc, 1, RTC_AF);
 
+	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;
+		}
+	}
+
 	nvmem_cfg.priv = s35390a;
 	err = devm_rtc_nvmem_register(rtc, &nvmem_cfg);
 	if (err)
@@ -539,7 +788,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 v5 6/6] 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 bc783be1561e..72bb3aea1956 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>
@@ -767,8 +768,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 v5 4/6] rtc: s35390a: force 24-hour mode

[Alexandre Belloni]

On 19/08/2026 22:05:56+0000, Markus Probst wrote:
> Instead of trying to remember the current 12/24-hour mode, which could
> result in time corruption if out of sync, we force the use of the 24-hour
> mode. Set to 24-hour mode if in 12-hour mode on startup and assume
> 24-hour mode. The 24-hour mode is already forced on reset.
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
>  drivers/rtc/rtc-s35390a.c | 67 +++++++++++++++++++++--------------------------
>  1 file changed, 30 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index 575bb256eb25..b34a3f7e8476 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c
> @@ -64,7 +64,6 @@ MODULE_DEVICE_TABLE(of, s35390a_of_match);
>  
>  struct s35390a {
>  	struct i2c_client *client[8];
> -	int twentyfourhour;
>  };
>  
>  static int s35390a_set_reg(struct s35390a *s35390a, int reg, u8  *buf, int len)
> @@ -181,31 +180,6 @@ static int s35390a_disable_test_mode(struct s35390a *s35390a)
>  	return s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, buf, sizeof(buf));
>  }
>  
> -static char s35390a_hr2reg(struct s35390a *s35390a, int hour)
> -{
> -	if (s35390a->twentyfourhour)
> -		return bin2bcd(hour);
> -
> -	if (hour < 12)
> -		return bin2bcd(hour);
> -
> -	return 0x40 | bin2bcd(hour - 12);
> -}
> -
> -static int s35390a_reg2hr(struct s35390a *s35390a, char reg)
> -{
> -	unsigned hour;
> -
> -	if (s35390a->twentyfourhour)
> -		return bcd2bin(reg & 0x3f);
> -
> -	hour = bcd2bin(reg & 0x3f);
> -	if (reg & 0x40)
> -		hour += 12;
> -
> -	return hour;
> -}
> -
>  static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> @@ -225,7 +199,7 @@ static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	buf[S35390A_BYTE_MONTH] = bin2bcd(tm->tm_mon + 1);
>  	buf[S35390A_BYTE_DAY] = bin2bcd(tm->tm_mday);
>  	buf[S35390A_BYTE_WDAY] = bin2bcd(tm->tm_wday);
> -	buf[S35390A_BYTE_HOURS] = s35390a_hr2reg(s35390a, tm->tm_hour);
> +	buf[S35390A_BYTE_HOURS] = bin2bcd(tm->tm_hour);
>  	buf[S35390A_BYTE_MINS] = bin2bcd(tm->tm_min);
>  	buf[S35390A_BYTE_SECS] = bin2bcd(tm->tm_sec);
>  
> @@ -256,7 +230,7 @@ static int s35390a_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  
>  	tm->tm_sec = bcd2bin(buf[S35390A_BYTE_SECS]);
>  	tm->tm_min = bcd2bin(buf[S35390A_BYTE_MINS]);
> -	tm->tm_hour = s35390a_reg2hr(s35390a, buf[S35390A_BYTE_HOURS]);
> +	tm->tm_hour = bcd2bin(buf[S35390A_BYTE_HOURS] & 0x3f);
>  	tm->tm_wday = bcd2bin(buf[S35390A_BYTE_WDAY]);
>  	tm->tm_mday = bcd2bin(buf[S35390A_BYTE_DAY]);
>  	tm->tm_mon = bcd2bin(buf[S35390A_BYTE_MONTH]) - 1;
> @@ -325,8 +299,7 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  	else
>  		buf[S35390A_ALRM_BYTE_WDAY] = 0;
>  
> -	buf[S35390A_ALRM_BYTE_HOURS] = s35390a_hr2reg(s35390a,
> -			alm->time.tm_hour) | 0x80;
> +	buf[S35390A_ALRM_BYTE_HOURS] = bin2bcd(alm->time.tm_hour) | 0x80;
>  	buf[S35390A_ALRM_BYTE_MINS] = bin2bcd(alm->time.tm_min) | 0x80;
>  
>  	if (alm->time.tm_hour >= 12)
> @@ -381,8 +354,7 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  
>  	if (buf[S35390A_ALRM_BYTE_HOURS] & 0x80)
>  		alm->time.tm_hour =
> -			s35390a_reg2hr(s35390a,
> -				       buf[S35390A_ALRM_BYTE_HOURS] & ~0x80);
> +			bcd2bin(buf[S35390A_ALRM_BYTE_HOURS] & 0x3f);
>  
>  	if (buf[S35390A_ALRM_BYTE_MINS] & 0x80)
>  		alm->time.tm_min = bcd2bin(buf[S35390A_ALRM_BYTE_MINS] & ~0x80);
> @@ -459,7 +431,7 @@ static int s35390a_probe(struct i2c_client *client)
>  	unsigned int i;
>  	struct s35390a *s35390a;
>  	struct rtc_device *rtc;
> -	u8 buf, status1;
> +	u8 buf, status1, time[7];
>  	struct device *dev = &client->dev;
>  	struct nvmem_config nvmem_cfg = {
>  		.name = "s35390a_nvram",
> @@ -503,10 +475,31 @@ static int s35390a_probe(struct i2c_client *client)
>  		return err_read;
>  	}
>  
> -	if (status1 & S35390A_FLAG_24H)
> -		s35390a->twentyfourhour = 1;
> -	else
> -		s35390a->twentyfourhour = 0;
> +	if (!(status1 & S35390A_FLAG_24H) && err_read != 1) {
> +		unsigned int hour;
> +
> +		status1 |= S35390A_FLAG_24H;
> +
> +		err = s35390a_get_reg(s35390a, S35390A_CMD_TIME1, time, sizeof(time));
> +		if (err < 0)
> +			return dev_err_probe(dev, err, "reading 12-hour time failed\n");
> +
> +		/* converting to 24-hour time */
> +		time[S35390A_BYTE_HOURS] = bitrev8(time[S35390A_BYTE_HOURS]);
> +		hour = bcd2bin(time[S35390A_BYTE_HOURS] & 0x3f);
> +		if (time[S35390A_BYTE_HOURS] & 0x40)
> +			hour += 12;
> +
> +		time[S35390A_BYTE_HOURS] = bitrev8(bin2bcd(hour));
> +
> +		err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, &status1, sizeof(status1));
> +		if (err < 0)
> +			return dev_err_probe(dev, err, "setting 24-hour mode failed\n");
> +
> +		err = s35390a_set_reg(s35390a, S35390A_CMD_TIME1, time, sizeof(time));
> +		if (err < 0)
> +			return dev_err_probe(dev, err, "setting 24-hour time failed\n");

No, don't do this, this will break existing users as your conversion may
happen after a minute rollover. The only function in which you are
allowed to change the time registers is set_time. You definitively need
to be able to read 12H mode time but you are allowed to only support
writing 24h time.

Also, this is super verbose, adding strings to the kernel makes it
bloated.

To avoid any desync, simply test for the S35390A_FLAG_24H bit in status
in s35390a_rtc_read_time as the register is already read anyway.


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

Re: [PATCH v5 3/6] rtc: s35390a: Fix alarm not disabling

[Alexandre Belloni]

On 19/08/2026 22:05:55+0000, Markus Probst wrote:
> 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;
> +}


You can definitively call this from s35390a_rtc_set_alarm instead of
duplicating code.

> +
>  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
> 

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

Re: [PATCH v5 3/6] rtc: s35390a: Fix alarm not disabling

[Markus Probst]

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

On Thu, 2026-08-20 at 00:25 +0200, Alexandre Belloni wrote:
> On 19/08/2026 22:05:55+0000, Markus Probst wrote:
> > 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;
> > +}
> 
> 
> You can definitively call this from s35390a_rtc_set_alarm instead of
> duplicating code.
At this stage yes.

With the pinctrl patch, it needs to be considered that the other pin
might be used as well, so the state needs to be preserved.

This will be done by reading the S35390A_CMD_STATUS2 register first.

This function will not be called in set_alarm, because this would
introduce additional read operations, as the current
S35390A_CMD_STATUS2 reg is already read inside set_alarm.

Thanks
- Markus Probst

> 
> > +
> >  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
> > 

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

Re: [PATCH v5 4/6] rtc: s35390a: force 24-hour mode

[Markus Probst]

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

On Thu, 2026-08-20 at 00:20 +0200, Alexandre Belloni wrote:
> On 19/08/2026 22:05:56+0000, Markus Probst wrote:
> > Instead of trying to remember the current 12/24-hour mode, which could
> > result in time corruption if out of sync, we force the use of the 24-hour
> > mode. Set to 24-hour mode if in 12-hour mode on startup and assume
> > 24-hour mode. The 24-hour mode is already forced on reset.
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> >  drivers/rtc/rtc-s35390a.c | 67 +++++++++++++++++++++--------------------------
> >  1 file changed, 30 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> > index 575bb256eb25..b34a3f7e8476 100644
> > --- a/drivers/rtc/rtc-s35390a.c
> > +++ b/drivers/rtc/rtc-s35390a.c
> > @@ -64,7 +64,6 @@ MODULE_DEVICE_TABLE(of, s35390a_of_match);
> >  
> >  struct s35390a {
> >  	struct i2c_client *client[8];
> > -	int twentyfourhour;
> >  };
> >  
> >  static int s35390a_set_reg(struct s35390a *s35390a, int reg, u8  *buf, int len)
> > @@ -181,31 +180,6 @@ static int s35390a_disable_test_mode(struct s35390a *s35390a)
> >  	return s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, buf, sizeof(buf));
> >  }
> >  
> > -static char s35390a_hr2reg(struct s35390a *s35390a, int hour)
> > -{
> > -	if (s35390a->twentyfourhour)
> > -		return bin2bcd(hour);
> > -
> > -	if (hour < 12)
> > -		return bin2bcd(hour);
> > -
> > -	return 0x40 | bin2bcd(hour - 12);
> > -}
> > -
> > -static int s35390a_reg2hr(struct s35390a *s35390a, char reg)
> > -{
> > -	unsigned hour;
> > -
> > -	if (s35390a->twentyfourhour)
> > -		return bcd2bin(reg & 0x3f);
> > -
> > -	hour = bcd2bin(reg & 0x3f);
> > -	if (reg & 0x40)
> > -		hour += 12;
> > -
> > -	return hour;
> > -}
> > -
> >  static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
> >  {
> >  	struct i2c_client *client = to_i2c_client(dev);
> > @@ -225,7 +199,7 @@ static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
> >  	buf[S35390A_BYTE_MONTH] = bin2bcd(tm->tm_mon + 1);
> >  	buf[S35390A_BYTE_DAY] = bin2bcd(tm->tm_mday);
> >  	buf[S35390A_BYTE_WDAY] = bin2bcd(tm->tm_wday);
> > -	buf[S35390A_BYTE_HOURS] = s35390a_hr2reg(s35390a, tm->tm_hour);
> > +	buf[S35390A_BYTE_HOURS] = bin2bcd(tm->tm_hour);
> >  	buf[S35390A_BYTE_MINS] = bin2bcd(tm->tm_min);
> >  	buf[S35390A_BYTE_SECS] = bin2bcd(tm->tm_sec);
> >  
> > @@ -256,7 +230,7 @@ static int s35390a_rtc_read_time(struct device *dev, struct rtc_time *tm)
> >  
> >  	tm->tm_sec = bcd2bin(buf[S35390A_BYTE_SECS]);
> >  	tm->tm_min = bcd2bin(buf[S35390A_BYTE_MINS]);
> > -	tm->tm_hour = s35390a_reg2hr(s35390a, buf[S35390A_BYTE_HOURS]);
> > +	tm->tm_hour = bcd2bin(buf[S35390A_BYTE_HOURS] & 0x3f);
> >  	tm->tm_wday = bcd2bin(buf[S35390A_BYTE_WDAY]);
> >  	tm->tm_mday = bcd2bin(buf[S35390A_BYTE_DAY]);
> >  	tm->tm_mon = bcd2bin(buf[S35390A_BYTE_MONTH]) - 1;
> > @@ -325,8 +299,7 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> >  	else
> >  		buf[S35390A_ALRM_BYTE_WDAY] = 0;
> >  
> > -	buf[S35390A_ALRM_BYTE_HOURS] = s35390a_hr2reg(s35390a,
> > -			alm->time.tm_hour) | 0x80;
> > +	buf[S35390A_ALRM_BYTE_HOURS] = bin2bcd(alm->time.tm_hour) | 0x80;
> >  	buf[S35390A_ALRM_BYTE_MINS] = bin2bcd(alm->time.tm_min) | 0x80;
> >  
> >  	if (alm->time.tm_hour >= 12)
> > @@ -381,8 +354,7 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> >  
> >  	if (buf[S35390A_ALRM_BYTE_HOURS] & 0x80)
> >  		alm->time.tm_hour =
> > -			s35390a_reg2hr(s35390a,
> > -				       buf[S35390A_ALRM_BYTE_HOURS] & ~0x80);
> > +			bcd2bin(buf[S35390A_ALRM_BYTE_HOURS] & 0x3f);
> >  
> >  	if (buf[S35390A_ALRM_BYTE_MINS] & 0x80)
> >  		alm->time.tm_min = bcd2bin(buf[S35390A_ALRM_BYTE_MINS] & ~0x80);
> > @@ -459,7 +431,7 @@ static int s35390a_probe(struct i2c_client *client)
> >  	unsigned int i;
> >  	struct s35390a *s35390a;
> >  	struct rtc_device *rtc;
> > -	u8 buf, status1;
> > +	u8 buf, status1, time[7];
> >  	struct device *dev = &client->dev;
> >  	struct nvmem_config nvmem_cfg = {
> >  		.name = "s35390a_nvram",
> > @@ -503,10 +475,31 @@ static int s35390a_probe(struct i2c_client *client)
> >  		return err_read;
> >  	}
> >  
> > -	if (status1 & S35390A_FLAG_24H)
> > -		s35390a->twentyfourhour = 1;
> > -	else
> > -		s35390a->twentyfourhour = 0;
> > +	if (!(status1 & S35390A_FLAG_24H) && err_read != 1) {
> > +		unsigned int hour;
> > +
> > +		status1 |= S35390A_FLAG_24H;
> > +
> > +		err = s35390a_get_reg(s35390a, S35390A_CMD_TIME1, time, sizeof(time));
> > +		if (err < 0)
> > +			return dev_err_probe(dev, err, "reading 12-hour time failed\n");
> > +
> > +		/* converting to 24-hour time */
> > +		time[S35390A_BYTE_HOURS] = bitrev8(time[S35390A_BYTE_HOURS]);
> > +		hour = bcd2bin(time[S35390A_BYTE_HOURS] & 0x3f);
> > +		if (time[S35390A_BYTE_HOURS] & 0x40)
> > +			hour += 12;
> > +
> > +		time[S35390A_BYTE_HOURS] = bitrev8(bin2bcd(hour));
> > +
> > +		err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, &status1, sizeof(status1));
> > +		if (err < 0)
> > +			return dev_err_probe(dev, err, "setting 24-hour mode failed\n");
> > +
> > +		err = s35390a_set_reg(s35390a, S35390A_CMD_TIME1, time, sizeof(time));
> > +		if (err < 0)
> > +			return dev_err_probe(dev, err, "setting 24-hour time failed\n");
> 
> No, don't do this, this will break existing users as your conversion may
> happen after a minute rollover. The only function in which you are
> allowed to change the time registers is set_time. You definitively need
> to be able to read 12H mode time but you are allowed to only support
> writing 24h time.
> 
> Also, this is super verbose, adding strings to the kernel makes it
> bloated.
> 
> To avoid any desync, simply test for the S35390A_FLAG_24H bit in status
> in s35390a_rtc_read_time as the register is already read anyway.
> 
I think that would defeat the whole point of the patch, that the rtc is
always in 24-hour mode. If this driver was always used with the rtc, it
already should be in 24-hour mode, as thats the mode this driver does
initialize the rtc.

Your idea would work for the time, but not in combination with alarms.
If the rtc could be in 12-hour mode, then this would need to be
considered in setting and reading alarm. Also setting 24-hour mode
inside set_time would then also require to convert the alarm time.
Effectively making it less maintainable than before.

Currently s35390a_init does initialize the rtc in 24-hour mode, while
not updating s35390a->twentyfourhour. So some change needs to be made.
Just setting s35390a->twentyfourhour inside s35390a_init might now be
the simplest solution here.

Thanks
- Markus Probst


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

Re: [PATCH v5 4/6] rtc: s35390a: force 24-hour mode

[Alexandre Belloni]

On 19/08/2026 22:57:13+0000, Markus Probst wrote:
> On Thu, 2026-08-20 at 00:20 +0200, Alexandre Belloni wrote:
> > On 19/08/2026 22:05:56+0000, Markus Probst wrote:
> > > Instead of trying to remember the current 12/24-hour mode, which could
> > > result in time corruption if out of sync, we force the use of the 24-hour
> > > mode. Set to 24-hour mode if in 12-hour mode on startup and assume
> > > 24-hour mode. The 24-hour mode is already forced on reset.
> > > 
> > > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > > ---
> > >  drivers/rtc/rtc-s35390a.c | 67 +++++++++++++++++++++--------------------------
> > >  1 file changed, 30 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> > > index 575bb256eb25..b34a3f7e8476 100644
> > > --- a/drivers/rtc/rtc-s35390a.c
> > > +++ b/drivers/rtc/rtc-s35390a.c
> > > @@ -64,7 +64,6 @@ MODULE_DEVICE_TABLE(of, s35390a_of_match);
> > >  
> > >  struct s35390a {
> > >  	struct i2c_client *client[8];
> > > -	int twentyfourhour;
> > >  };
> > >  
> > >  static int s35390a_set_reg(struct s35390a *s35390a, int reg, u8  *buf, int len)
> > > @@ -181,31 +180,6 @@ static int s35390a_disable_test_mode(struct s35390a *s35390a)
> > >  	return s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, buf, sizeof(buf));
> > >  }
> > >  
> > > -static char s35390a_hr2reg(struct s35390a *s35390a, int hour)
> > > -{
> > > -	if (s35390a->twentyfourhour)
> > > -		return bin2bcd(hour);
> > > -
> > > -	if (hour < 12)
> > > -		return bin2bcd(hour);
> > > -
> > > -	return 0x40 | bin2bcd(hour - 12);
> > > -}
> > > -
> > > -static int s35390a_reg2hr(struct s35390a *s35390a, char reg)
> > > -{
> > > -	unsigned hour;
> > > -
> > > -	if (s35390a->twentyfourhour)
> > > -		return bcd2bin(reg & 0x3f);
> > > -
> > > -	hour = bcd2bin(reg & 0x3f);
> > > -	if (reg & 0x40)
> > > -		hour += 12;
> > > -
> > > -	return hour;
> > > -}
> > > -
> > >  static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > >  {
> > >  	struct i2c_client *client = to_i2c_client(dev);
> > > @@ -225,7 +199,7 @@ static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > >  	buf[S35390A_BYTE_MONTH] = bin2bcd(tm->tm_mon + 1);
> > >  	buf[S35390A_BYTE_DAY] = bin2bcd(tm->tm_mday);
> > >  	buf[S35390A_BYTE_WDAY] = bin2bcd(tm->tm_wday);
> > > -	buf[S35390A_BYTE_HOURS] = s35390a_hr2reg(s35390a, tm->tm_hour);
> > > +	buf[S35390A_BYTE_HOURS] = bin2bcd(tm->tm_hour);
> > >  	buf[S35390A_BYTE_MINS] = bin2bcd(tm->tm_min);
> > >  	buf[S35390A_BYTE_SECS] = bin2bcd(tm->tm_sec);
> > >  
> > > @@ -256,7 +230,7 @@ static int s35390a_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > >  
> > >  	tm->tm_sec = bcd2bin(buf[S35390A_BYTE_SECS]);
> > >  	tm->tm_min = bcd2bin(buf[S35390A_BYTE_MINS]);
> > > -	tm->tm_hour = s35390a_reg2hr(s35390a, buf[S35390A_BYTE_HOURS]);
> > > +	tm->tm_hour = bcd2bin(buf[S35390A_BYTE_HOURS] & 0x3f);
> > >  	tm->tm_wday = bcd2bin(buf[S35390A_BYTE_WDAY]);
> > >  	tm->tm_mday = bcd2bin(buf[S35390A_BYTE_DAY]);
> > >  	tm->tm_mon = bcd2bin(buf[S35390A_BYTE_MONTH]) - 1;
> > > @@ -325,8 +299,7 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > >  	else
> > >  		buf[S35390A_ALRM_BYTE_WDAY] = 0;
> > >  
> > > -	buf[S35390A_ALRM_BYTE_HOURS] = s35390a_hr2reg(s35390a,
> > > -			alm->time.tm_hour) | 0x80;
> > > +	buf[S35390A_ALRM_BYTE_HOURS] = bin2bcd(alm->time.tm_hour) | 0x80;
> > >  	buf[S35390A_ALRM_BYTE_MINS] = bin2bcd(alm->time.tm_min) | 0x80;
> > >  
> > >  	if (alm->time.tm_hour >= 12)
> > > @@ -381,8 +354,7 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > >  
> > >  	if (buf[S35390A_ALRM_BYTE_HOURS] & 0x80)
> > >  		alm->time.tm_hour =
> > > -			s35390a_reg2hr(s35390a,
> > > -				       buf[S35390A_ALRM_BYTE_HOURS] & ~0x80);
> > > +			bcd2bin(buf[S35390A_ALRM_BYTE_HOURS] & 0x3f);
> > >  
> > >  	if (buf[S35390A_ALRM_BYTE_MINS] & 0x80)
> > >  		alm->time.tm_min = bcd2bin(buf[S35390A_ALRM_BYTE_MINS] & ~0x80);
> > > @@ -459,7 +431,7 @@ static int s35390a_probe(struct i2c_client *client)
> > >  	unsigned int i;
> > >  	struct s35390a *s35390a;
> > >  	struct rtc_device *rtc;
> > > -	u8 buf, status1;
> > > +	u8 buf, status1, time[7];
> > >  	struct device *dev = &client->dev;
> > >  	struct nvmem_config nvmem_cfg = {
> > >  		.name = "s35390a_nvram",
> > > @@ -503,10 +475,31 @@ static int s35390a_probe(struct i2c_client *client)
> > >  		return err_read;
> > >  	}
> > >  
> > > -	if (status1 & S35390A_FLAG_24H)
> > > -		s35390a->twentyfourhour = 1;
> > > -	else
> > > -		s35390a->twentyfourhour = 0;
> > > +	if (!(status1 & S35390A_FLAG_24H) && err_read != 1) {
> > > +		unsigned int hour;
> > > +
> > > +		status1 |= S35390A_FLAG_24H;
> > > +
> > > +		err = s35390a_get_reg(s35390a, S35390A_CMD_TIME1, time, sizeof(time));
> > > +		if (err < 0)
> > > +			return dev_err_probe(dev, err, "reading 12-hour time failed\n");
> > > +
> > > +		/* converting to 24-hour time */
> > > +		time[S35390A_BYTE_HOURS] = bitrev8(time[S35390A_BYTE_HOURS]);
> > > +		hour = bcd2bin(time[S35390A_BYTE_HOURS] & 0x3f);
> > > +		if (time[S35390A_BYTE_HOURS] & 0x40)
> > > +			hour += 12;
> > > +
> > > +		time[S35390A_BYTE_HOURS] = bitrev8(bin2bcd(hour));
> > > +
> > > +		err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, &status1, sizeof(status1));
> > > +		if (err < 0)
> > > +			return dev_err_probe(dev, err, "setting 24-hour mode failed\n");
> > > +
> > > +		err = s35390a_set_reg(s35390a, S35390A_CMD_TIME1, time, sizeof(time));
> > > +		if (err < 0)
> > > +			return dev_err_probe(dev, err, "setting 24-hour time failed\n");
> > 
> > No, don't do this, this will break existing users as your conversion may
> > happen after a minute rollover. The only function in which you are
> > allowed to change the time registers is set_time. You definitively need
> > to be able to read 12H mode time but you are allowed to only support
> > writing 24h time.
> > 
> > Also, this is super verbose, adding strings to the kernel makes it
> > bloated.
> > 
> > To avoid any desync, simply test for the S35390A_FLAG_24H bit in status
> > in s35390a_rtc_read_time as the register is already read anyway.
> > 
> I think that would defeat the whole point of the patch, that the rtc is
> always in 24-hour mode. If this driver was always used with the rtc, it
> already should be in 24-hour mode, as thats the mode this driver does
> initialize the rtc.

This assumption may not be true, the RTC may have been initialized by a
bootloader or a different system in the factory.

> Your idea would work for the time, but not in combination with alarms.
> If the rtc could be in 12-hour mode, then this would need to be
> considered in setting and reading alarm. Also setting 24-hour mode
> inside set_time would then also require to convert the alarm time.
> Effectively making it less maintainable than before.

Note that while setting the time when an alarm has been set is not
forbidden, it is never guaranteed to work anyway. I guess something we
could do in the core is that whenever the time is set we go over the
timerqueue and set the alarm again. This would allow to remove the UIE
disabling/enabling dance.

> 
> Currently s35390a_init does initialize the rtc in 24-hour mode, while
> not updating s35390a->twentyfourhour. So some change needs to be made.
> Just setting s35390a->twentyfourhour inside s35390a_init might now be
> the simplest solution here.

Or maybe simply stop caching the value.

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