Re: [PATCH v9 5/6] rtc: s35390a: Add pinctrl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] pinctrl: 'clock' function is advertised but silently disables the pin
- [Medium] rtc: probe unconditionally clobbers periodic pin configuration if interrupt is pending
--

commit 8933a7b5fe72ca6a0ef2901f1d1c8bf6cbe6372f
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 including keeping previous configuration,
disabling, waking up, and minute periodical interrupts.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index 12ed504db94d8..649d36d37f1f4 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c

[ ... ]

> @@ -464,13 +526,168 @@ static int s35390a_nvmem_write(void *priv, unsigned int offset, void *val,

[ ... ]

> +	if (function != S35390A_FUNC_IGNORE) {
> +		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;

[Severity: Medium]
In s35390a_pinctrl_set_mux(), does this silently fail to implement the
requested clock behavior and disable the pin instead?

If a device tree configuration requests the advertised clock pin function,
should this return an error like -ENOTSUPP or -EINVAL instead of silently
disabling it?

[ ... ]

> @@ -504,33 +726,34 @@ static int s35390a_probe(struct i2c_client *client)

[ ... ]

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

[Severity: Medium]
In s35390a_probe(), can this force the pin to NOINTR mode if an interrupt
was pending at boot, permanently destroying a periodic configuration before
the pinctrl 'keep previous' fallback can save it?

If the device is configured to generate periodic interrupts (like a 1-minute
wakeup), and the system boots while the interrupt flag happens to be set in
STATUS1, overwriting STATUS2 to NOINTR might cause the pinctrl subsystem's
IGNORE fallback to keep this overwritten state, leaving the pin permanently
disabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-rtc_s35390a_int1-v9-0-b2567a3e4959@posteo.de?part=5