* [PATCH v6 1/6] dt-bindings: rtc: Add pinctrl for S35390A
2026-08-23 22:13 [PATCH v6 0/6] rtc: s35390a: Allow use of output pin for interrupt signal 1 for wakealarm Markus Probst
@ 2026-08-23 22:13 ` Markus Probst
2026-08-23 22:20 ` sashiko-bot
2026-08-23 22:13 ` [PATCH v6 2/6] rtc: s35390a: Add missing newline to dev_err Markus Probst
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Markus Probst @ 2026-08-23 22:13 UTC (permalink / raw)
To: Alexandre Belloni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Uwe Kleine-König, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Michael Langer, Andrew Morton,
Linus Walleij, Fabien Lahoudere
Cc: linux-arm-kernel, linux-rtc, devicetree, linux-gpio, linux-kernel,
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v6 1/6] dt-bindings: rtc: Add pinctrl for S35390A
2026-08-23 22:13 ` [PATCH v6 1/6] dt-bindings: rtc: Add pinctrl for S35390A Markus Probst
@ 2026-08-23 22:20 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-08-23 22:20 UTC (permalink / raw)
To: Markus Probst; +Cc: robh, devicetree, conor+dt, linux-rtc, Alexandre Belloni
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The binding adds a clock provider capability (`#clock-cells`) but fails to list it in the `required` properties and include it in the `examples` section.
- [Medium] The schema fails to enforce the documented constraint that the `pmin2` function can only be used with pin `int1`.
--
--- Patch [1]: [PATCH v6 1/6] dt-bindings: rtc: Add pinctrl for S35390A ---
commit ed4fe5fd689ace047dd6fd7d73cbfef4879078ec
Author: Markus Probst <markus.probst@posteo.de>
dt-bindings: rtc: Add pinctrl for S35390A
This commit moves the devicetree bindings for sii,s35390a into its own file.
It also adds properties to configure pins via pinctrl to allow the interrupt
signal 1 to be used for the wakeup alarm.
> diff --git a/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml b/Documentation/devicetree/bindings/rtc/sii,s35390a.yaml
> new file mode 100644
> --- /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.
> + enum:
> + - ignore
> + - disable
> + - wakeup
> + - clock
> + - pmin1
> + - pmin2
[Severity: Medium]
The documentation specifies that pmin2 can only be used with pin int1. Should
the schema include an allOf/if conditional block to logically enforce this
restriction, preventing pmin2 from being used with int2?
[ ... ]
> +required:
> + - compatible
> + - reg
[Severity: Medium]
Does #clock-cells need to be added to this list of required properties?
> +
> +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>;
[Severity: Medium]
Should the examples section include the #clock-cells property to ensure it
passes validation checks?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-rtc_s35390a_int1-v6-0-df96569e3412@posteo.de?part=1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v6 2/6] rtc: s35390a: Add missing newline to dev_err
2026-08-23 22:13 [PATCH v6 0/6] rtc: s35390a: Allow use of output pin for interrupt signal 1 for wakealarm Markus Probst
2026-08-23 22:13 ` [PATCH v6 1/6] dt-bindings: rtc: Add pinctrl for S35390A Markus Probst
@ 2026-08-23 22:13 ` Markus Probst
2026-08-23 22:21 ` sashiko-bot
2026-08-23 22:13 ` [PATCH v6 3/6] rtc: s35390a: Fix alarm not disabling Markus Probst
` (3 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Markus Probst @ 2026-08-23 22:13 UTC (permalink / raw)
To: Alexandre Belloni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Uwe Kleine-König, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Michael Langer, Andrew Morton,
Linus Walleij, Fabien Lahoudere
Cc: linux-arm-kernel, linux-rtc, devicetree, linux-gpio, linux-kernel,
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v6 2/6] rtc: s35390a: Add missing newline to dev_err
2026-08-23 22:13 ` [PATCH v6 2/6] rtc: s35390a: Add missing newline to dev_err Markus Probst
@ 2026-08-23 22:21 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-08-23 22:21 UTC (permalink / raw)
To: Markus Probst; +Cc: robh, linux-rtc, conor+dt, Alexandre Belloni, devicetree
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Probe clears power-on clear (POC) and battery low (BLD) flags without acting on them, causing silent data corruption.
- [High] RTC_VL_CLR ioctl performs a full hardware reset, destructively erasing the RTC time.
--
--- Patch [2]: [PATCH v6 2/6] rtc: s35390a: Add missing newline to dev_err ---
commit f317b10535ad5028ddd5cbe785ad735c87f43819
Author: Markus Probst <markus.probst@posteo.de>
rtc: s35390a: Add missing newline to dev_err
This commit adds a missing newline character to a dev_err() message in the
probe function. It fixes the log output format when disabling the alarm 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 {
[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem
in how the probe function handles power-loss flags:
drivers/rtc/rtc-s35390a.c:s35390a_probe() {
[ ... ]
err_read = s35390a_read_status(s35390a, &status1);
if (err_read < 0) {
dev_err(dev, "error resetting chip\n");
return err_read;
}
[ ... ]
}
Does the driver silently discard the power-loss or battery-low indicators?
When the RTC loses power, reading STATUS1 clears the hardware flags, but
s35390a_read_status() returns 1 to indicate the invalid state. Since this code
doesn't check if err_read == 1, won't it just continue and serve garbage time
data to userspace as if it were valid?
[Severity: High]
This is a pre-existing issue, but there is also a destructive behavior in the
ioctl handler:
drivers/rtc/rtc-s35390a.c:s35390a_rtc_ioctl() {
[ ... ]
case RTC_VL_CLR:
/* update flag and clear register */
err = s35390a_init(s35390a);
if (err < 0)
return err;
break;
[ ... ]
}
Could this ioctl destructively erase the configured RTC time?
Calling s35390a_init() writes S35390A_FLAG_RESET, which resets the chip and
erases the currently programmed time. Since RTC_VL_CLR is only meant to clear
low-voltage warnings, is it safe to let this UAPI call wipe the main time data?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-rtc_s35390a_int1-v6-0-df96569e3412@posteo.de?part=2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v6 3/6] rtc: s35390a: Fix alarm not disabling
2026-08-23 22:13 [PATCH v6 0/6] rtc: s35390a: Allow use of output pin for interrupt signal 1 for wakealarm Markus Probst
2026-08-23 22:13 ` [PATCH v6 1/6] dt-bindings: rtc: Add pinctrl for S35390A Markus Probst
2026-08-23 22:13 ` [PATCH v6 2/6] rtc: s35390a: Add missing newline to dev_err Markus Probst
@ 2026-08-23 22:13 ` Markus Probst
2026-08-23 22:19 ` sashiko-bot
2026-08-23 22:13 ` [PATCH v6 4/6] rtc: s35390a: Read 24-hour mode on access Markus Probst
` (2 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Markus Probst @ 2026-08-23 22:13 UTC (permalink / raw)
To: Alexandre Belloni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Uwe Kleine-König, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Michael Langer, Andrew Morton,
Linus Walleij, Fabien Lahoudere
Cc: linux-arm-kernel, linux-rtc, devicetree, linux-gpio, linux-kernel,
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v6 4/6] rtc: s35390a: Read 24-hour mode on access
2026-08-23 22:13 [PATCH v6 0/6] rtc: s35390a: Allow use of output pin for interrupt signal 1 for wakealarm Markus Probst
` (2 preceding siblings ...)
2026-08-23 22:13 ` [PATCH v6 3/6] rtc: s35390a: Fix alarm not disabling Markus Probst
@ 2026-08-23 22:13 ` Markus Probst
2026-08-23 22:24 ` sashiko-bot
2026-08-23 22:13 ` [PATCH v6 5/6] rtc: s35390a: Add pinctrl Markus Probst
2026-08-23 22:13 ` [PATCH v6 6/6] rtc: s35390a: Add synology quirk Markus Probst
5 siblings, 1 reply; 13+ messages in thread
From: Markus Probst @ 2026-08-23 22:13 UTC (permalink / raw)
To: Alexandre Belloni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Uwe Kleine-König, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Michael Langer, Andrew Morton,
Linus Walleij, Fabien Lahoudere
Cc: linux-arm-kernel, linux-rtc, devicetree, linux-gpio, linux-kernel,
Markus Probst
Instead of reading if the 24-hour mode is used on probe once, read it on
access. This makes it impossible for the mode to be out of sync and fixes
time corruption if resetting the chip while in 12-hour mode, as
`s35390a_init` did not update the cached value.
Fixes: 16486d0c1c65 ("rtc: s35390a: handle invalid RTC time")
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
drivers/rtc/rtc-s35390a.c | 70 +++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 35 deletions(-)
diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 575bb256eb25..8e3616c65d2c 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)
@@ -102,9 +101,8 @@ static int s35390a_get_reg(struct s35390a *s35390a, int reg, u8 *buf, int len)
return 0;
}
-static int s35390a_init(struct s35390a *s35390a)
+static int s35390a_init(struct s35390a *s35390a, u8 *sts)
{
- u8 buf;
int ret;
unsigned initcount = 0;
@@ -117,17 +115,17 @@ static int s35390a_init(struct s35390a *s35390a)
* The 24H bit is kept over reset, so set it already here.
*/
initialize:
- buf = S35390A_FLAG_RESET | S35390A_FLAG_24H;
- ret = s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, &buf, 1);
+ *sts = S35390A_FLAG_RESET | S35390A_FLAG_24H;
+ ret = s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, sts, 1);
if (ret < 0)
return ret;
- ret = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, &buf, 1);
+ ret = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, sts, 1);
if (ret < 0)
return ret;
- if (buf & (S35390A_FLAG_POC | S35390A_FLAG_BLD)) {
+ if (*sts & (S35390A_FLAG_POC | S35390A_FLAG_BLD)) {
/* Try up to five times to reset the chip */
if (initcount < 5) {
++initcount;
@@ -181,9 +179,9 @@ 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)
+static char s35390a_hr2reg(int hour, bool twentyfourhour)
{
- if (s35390a->twentyfourhour)
+ if (twentyfourhour)
return bin2bcd(hour);
if (hour < 12)
@@ -192,11 +190,11 @@ static char s35390a_hr2reg(struct s35390a *s35390a, int hour)
return 0x40 | bin2bcd(hour - 12);
}
-static int s35390a_reg2hr(struct s35390a *s35390a, char reg)
+static int s35390a_reg2hr(char reg, bool twentyfourhour)
{
unsigned hour;
- if (s35390a->twentyfourhour)
+ if (twentyfourhour)
return bcd2bin(reg & 0x3f);
hour = bcd2bin(reg & 0x3f);
@@ -210,7 +208,7 @@ static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
{
struct i2c_client *client = to_i2c_client(dev);
struct s35390a *s35390a = i2c_get_clientdata(client);
- int i;
+ int i, err;
u8 buf[7], status;
dev_dbg(&client->dev, "%s: tm is secs=%d, mins=%d, hours=%d mday=%d, "
@@ -218,14 +216,18 @@ static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
tm->tm_min, tm->tm_hour, tm->tm_mday, tm->tm_mon, tm->tm_year,
tm->tm_wday);
- if (s35390a_read_status(s35390a, &status) == 1)
- s35390a_init(s35390a);
+ err = s35390a_read_status(s35390a, &status);
+ if (err == 1)
+ err = s35390a_init(s35390a, &status);
+
+ if (err < 0)
+ return err;
buf[S35390A_BYTE_YEAR] = bin2bcd(tm->tm_year - 100);
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] = s35390a_hr2reg(tm->tm_hour, status & S35390A_FLAG_24H);
buf[S35390A_BYTE_MINS] = bin2bcd(tm->tm_min);
buf[S35390A_BYTE_SECS] = bin2bcd(tm->tm_sec);
@@ -256,7 +258,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 = s35390a_reg2hr(buf[S35390A_BYTE_HOURS], status & S35390A_FLAG_24H);
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;
@@ -292,7 +294,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], status1, status2 = 0;
int err, i;
dev_dbg(&client->dev, "%s: alm is secs=%d, mins=%d, hours=%d mday=%d, "\
@@ -301,22 +303,22 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
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));
+ err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2));
if (err < 0)
return err;
/* 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_STATUS1, &status1, sizeof(status1));
if (err < 0)
return err;
if (alm->enabled)
- sts = S35390A_INT2_MODE_ALARM;
+ status2 = S35390A_INT2_MODE_ALARM;
else
- sts = S35390A_INT2_MODE_NOINTR;
+ status2 = S35390A_INT2_MODE_NOINTR;
/* set interrupt mode*/
- err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
+ err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2));
if (err < 0)
return err;
@@ -325,8 +327,8 @@ 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] = s35390a_hr2reg(alm->time.tm_hour,
+ status1 & S35390A_FLAG_24H) | 0x80;
buf[S35390A_ALRM_BYTE_MINS] = bin2bcd(alm->time.tm_min) | 0x80;
if (alm->time.tm_hour >= 12)
@@ -345,14 +347,17 @@ 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;
+ u8 buf[3], status1, status2;
int i, err;
- err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
+ if (s35390a_read_status(s35390a, &status1) == 1)
+ return -EINVAL;
+
+ err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2));
if (err < 0)
return err;
- if ((sts & S35390A_INT2_MODE_MASK) != S35390A_INT2_MODE_ALARM) {
+ if ((status2 & S35390A_INT2_MODE_MASK) != S35390A_INT2_MODE_ALARM) {
/*
* When the alarm isn't enabled, the register to configure
* the alarm time isn't accessible.
@@ -381,8 +386,8 @@ 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);
+ s35390a_reg2hr(buf[S35390A_ALRM_BYTE_HOURS] & ~0x80,
+ status1 & S35390A_FLAG_24H);
if (buf[S35390A_ALRM_BYTE_MINS] & 0x80)
alm->time.tm_min = bcd2bin(buf[S35390A_ALRM_BYTE_MINS] & ~0x80);
@@ -416,7 +421,7 @@ static int s35390a_rtc_ioctl(struct device *dev, unsigned int cmd,
break;
case RTC_VL_CLR:
/* update flag and clear register */
- err = s35390a_init(s35390a);
+ err = s35390a_init(s35390a, &sts);
if (err < 0)
return err;
break;
@@ -503,11 +508,6 @@ 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_INT2) {
/* disable alarm (and maybe test mode) */
buf = 0;
--
2.54.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v6 4/6] rtc: s35390a: Read 24-hour mode on access
2026-08-23 22:13 ` [PATCH v6 4/6] rtc: s35390a: Read 24-hour mode on access Markus Probst
@ 2026-08-23 22:24 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-08-23 22:24 UTC (permalink / raw)
To: Markus Probst; +Cc: devicetree, conor+dt, robh, linux-rtc, Alexandre Belloni
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Uninitialized stack variable usage due to missing error check for negative return values from s35390a_read_status.
--
--- Patch [4]: [PATCH v6 4/6] rtc: s35390a: Read 24-hour mode on access ---
commit d45935347291ea27eb3558322c8f56d965fd15e6
Author: Markus Probst <markus.probst@posteo.de>
rtc: s35390a: Read 24-hour mode on access
This commit reads the 24-hour mode on access instead of only on probe. It
fixes time corruption when resetting the chip in 12-hour mode, as
s35390a_init did not update the cached value.
> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index 575bb256eb258..8e3616c65d2c0 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c
[ ... ]
> @@ -256,7 +258,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 = s35390a_reg2hr(buf[S35390A_BYTE_HOURS], status & S35390A_FLAG_24H);
[Severity: High]
Earlier in s35390a_rtc_read_time(), the code calls
s35390a_read_status(s35390a, &status).
If this call fails with a negative error code (e.g. -EIO due to a transient
I2C error), the check if (s35390a_read_status(s35390a, &status) == 1) evaluates
to false, and the function proceeds without returning an error.
Will this cause the uninitialized status stack variable to be evaluated here,
potentially corrupting the time calculation?
[ ... ]
> @@ -345,14 +347,17 @@ 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;
> + u8 buf[3], status1, status2;
> int i, err;
>
> - err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
> + if (s35390a_read_status(s35390a, &status1) == 1)
> + return -EINVAL;
> +
> + err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2));
> if (err < 0)
> return err;
[ ... ]
> @@ -381,8 +386,8 @@ 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);
> + s35390a_reg2hr(buf[S35390A_ALRM_BYTE_HOURS] & ~0x80,
> + status1 & S35390A_FLAG_24H);
[Severity: High]
Similarly to s35390a_rtc_read_time(), if s35390a_read_status() returns a
negative error code, the check above will pass and the status1 stack variable
will remain uninitialized.
Could this allow the uninitialized status1 variable to be used here to determine
the alarm hour calculations?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-rtc_s35390a_int1-v6-0-df96569e3412@posteo.de?part=4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v6 5/6] rtc: s35390a: Add pinctrl
2026-08-23 22:13 [PATCH v6 0/6] rtc: s35390a: Allow use of output pin for interrupt signal 1 for wakealarm Markus Probst
` (3 preceding siblings ...)
2026-08-23 22:13 ` [PATCH v6 4/6] rtc: s35390a: Read 24-hour mode on access Markus Probst
@ 2026-08-23 22:13 ` Markus Probst
2026-08-23 22:26 ` sashiko-bot
2026-08-23 22:13 ` [PATCH v6 6/6] rtc: s35390a: Add synology quirk Markus Probst
5 siblings, 1 reply; 13+ messages in thread
From: Markus Probst @ 2026-08-23 22:13 UTC (permalink / raw)
To: Alexandre Belloni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Uwe Kleine-König, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Michael Langer, Andrew Morton,
Linus Walleij, Fabien Lahoudere
Cc: linux-arm-kernel, linux-rtc, devicetree, linux-gpio, linux-kernel,
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 | 377 ++++++++++++++++++++++++++++++++++++++--------
2 files changed, 315 insertions(+), 64 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 8e3616c65d2c..a2ddce91ef7a 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)
@@ -165,20 +194,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 char s35390a_hr2reg(int hour, bool twentyfourhour)
{
if (twentyfourhour)
@@ -278,10 +293,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)
@@ -302,7 +332,19 @@ 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);
+ guard(mutex)(&s35390a->pinfunction_lock);
+
+ err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2));
+ if (err < 0)
+ return err;
+
/* disable interrupt (which deasserts the irq line) */
+ if (s35390a->pinfunction[0] == S35390A_FUNC_WAKEUP)
+ status2 = (status2 & ~S35390A_INT1_MODE_MASK) | S35390A_INT_MODE_NOINTR;
+
+ if (s35390a->pinfunction[1] == S35390A_FUNC_WAKEUP)
+ status2 = (status2 & ~S35390A_INT2_MODE_MASK) | S35390A_INT_MODE_NOINTR;
+
err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2));
if (err < 0)
return err;
@@ -312,16 +354,6 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
if (err < 0)
return err;
- if (alm->enabled)
- status2 = S35390A_INT2_MODE_ALARM;
- else
- status2 = S35390A_INT2_MODE_NOINTR;
-
- /* set interrupt mode*/
- err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2));
- if (err < 0)
- return err;
-
if (alm->time.tm_wday != -1)
buf[S35390A_ALRM_BYTE_WDAY] = bin2bcd(alm->time.tm_wday) | 0x80;
else
@@ -337,10 +369,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)
+ status2 = (status2 & ~S35390A_INT1_MODE_MASK) | S35390A_INT1_MODE_ALARM;
+
+ if (s35390a->pinfunction[1] == S35390A_FUNC_WAKEUP)
+ status2 = (status2 & ~S35390A_INT2_MODE_MASK) | S35390A_INT2_MODE_ALARM;
+
+ err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2));
+ 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)
@@ -348,7 +402,9 @@ 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], status1, status2;
- int i, err;
+ int i, err, reg;
+
+ guard(mutex)(&s35390a->pinfunction_lock);
if (s35390a_read_status(s35390a, &status1) == 1)
return -EINVAL;
@@ -357,18 +413,24 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
if (err < 0)
return err;
- if ((status2 & S35390A_INT2_MODE_MASK) != S35390A_INT2_MODE_ALARM) {
+ if (s35390a->pinfunction[1] == S35390A_FUNC_WAKEUP &&
+ (status2 & S35390A_INT2_MODE_MASK) == S35390A_INT2_MODE_ALARM) {
+ reg = S35390A_CMD_INT2_REG1;
+ } else if (s35390a->pinfunction[0] == S35390A_FUNC_WAKEUP &&
+ (status2 & 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;
@@ -377,7 +439,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)
@@ -458,13 +520,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;
+ struct pinctrl_dev *pctl;
+ u8 status1, status2;
+ bool irq = false;
struct device *dev = &client->dev;
struct nvmem_config nvmem_cfg = {
.name = "s35390a_nvram",
@@ -475,6 +694,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;
@@ -483,7 +703,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 */
@@ -498,33 +722,34 @@ 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;
+ }
+
+ 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;
@@ -533,9 +758,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)
@@ -546,7 +795,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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v6 5/6] rtc: s35390a: Add pinctrl
2026-08-23 22:13 ` [PATCH v6 5/6] rtc: s35390a: Add pinctrl Markus Probst
@ 2026-08-23 22:26 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-08-23 22:26 UTC (permalink / raw)
To: Markus Probst; +Cc: Alexandre Belloni, devicetree, linux-rtc, conor+dt, robh
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditional dependency on PINCTRL removes the RTC driver on non-pinctrl platforms.
- [Medium] The 'clock' pin function is advertised but silently falls back to disabling the pin rather than returning an error.
- [Medium] TOCTOU race in `s35390a_probe` when reading `pinfunction` without lock after `pinctrl_enable`.
--
--- Patch [5]: [PATCH v6 5/6] rtc: s35390a: Add pinctrl ---
commit 73fe5a984406c6bfdc0258a390d1f790625e1082
Author: Markus Probst <markus.probst@posteo.de>
rtc: s35390a: Add pinctrl
This commit adds pinctrl support to the S35390A RTC driver, allowing
configuration of pin functions like wake up, disabled, and periodic
interrupts. If no function is configured, it falls back to the previous
behavior.
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 01def82318731..2d79710c48375 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
[Severity: High]
Will this unconditional dependency cause the RTC driver to be silently dropped
on platforms without PINCTRL enabled?
This could break backwards compatibility for existing driver users by changing
an optional feature into a hard dependency.
> select BITREVERSE
> + select GENERIC_PINCONF
> 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 8e3616c65d2c0..a2ddce91ef7ac 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c
[ ... ]
> @@ -458,13 +520,170 @@ static int s35390a_nvmem_write(void *priv, unsigned int offset, void *val,
[ ... ]
> +static int s35390a_pinctrl_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
> + unsigned int group)
> +{
[ ... ]
> + switch (function) {
> + case S35390A_FUNC_DISABLE:
> + case S35390A_FUNC_CLOCK: /* not implemented */
> + status2 = (status2 & ~mask) | S35390A_INT_MODE_NOINTR;
> + break;
[Severity: Medium]
Since the clock function is not implemented, should this return an error
instead of silently disabling the pin?
Users specifying the 'clock' function in the device tree might assume the pin
is outputting a clock signal, but it will be disabled instead.
> + case S35390A_FUNC_WAKEUP:
> + flag = group == 0 ? S35390A_INT1_MODE_ALARM : S35390A_INT2_MODE_ALARM;
[ ... ]
> @@ -533,9 +758,33 @@ static int s35390a_probe(struct i2c_client *client)
[ ... ]
> + 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) {
[Severity: Medium]
Is there a data race here since the pinfunction array is accessed locklessly
after pinctrl_enable() has made the pinctrl device available?
If another consumer requests the pins concurrently right after pinctrl_enable()
modifying pinfunction, could their configuration be overwritten by this
fallback logic?
> + 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)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-rtc_s35390a_int1-v6-0-df96569e3412@posteo.de?part=5
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v6 6/6] rtc: s35390a: Add synology quirk
2026-08-23 22:13 [PATCH v6 0/6] rtc: s35390a: Allow use of output pin for interrupt signal 1 for wakealarm Markus Probst
` (4 preceding siblings ...)
2026-08-23 22:13 ` [PATCH v6 5/6] rtc: s35390a: Add pinctrl Markus Probst
@ 2026-08-23 22:13 ` Markus Probst
2026-08-23 22:18 ` sashiko-bot
5 siblings, 1 reply; 13+ messages in thread
From: Markus Probst @ 2026-08-23 22:13 UTC (permalink / raw)
To: Alexandre Belloni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Uwe Kleine-König, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Michael Langer, Andrew Morton,
Linus Walleij, Fabien Lahoudere
Cc: linux-arm-kernel, linux-rtc, devicetree, linux-gpio, linux-kernel,
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 a2ddce91ef7a..6109c4838eb0 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>
@@ -774,8 +775,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
^ permalink raw reply related [flat|nested] 13+ messages in thread