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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] rtc: s35390a: RTC hardware reset silently destroys pinctrl configurations
- [Medium] rtc: s35390a: silent failure when setting alarm without WAKEUP pin
- [Medium] rtc: s35390a: 'clock' pin function silently behaves as 'disable'
--

commit 45e9a4cae015f35a25fbac259c1165bae0a8f0fd
Author: Markus Probst <markus.probst@posteo.de>
Subject: rtc: s35390a: Add pinctrl

This commit adds pinctrl support to the s35390a RTC driver, allowing
configuration of the INT1 and INT2 pin functions. It supports functions
like wakeup, clock, pmin1, pmin2, disable, and ignore.

> 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
> @@ -142,12 +142,12 @@ static int s35390a_init(struct s35390a *s35390a, u8 *sts)
>  	 * 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);

[Severity: High]
If the RTC has the BLD (Battery Low Detection) flag set, calling
s35390a_rtc_set_time() triggers s35390a_init(), which resets the chip via
S35390A_FLAG_RESET. This clears the STATUS2 register in hardware, stopping
all pin outputs.

Because the driver's software cache (s35390a->pinfunction) is not cleared,
does this permanently disable hardware outputs (like PMIN1) until userspace
bypasses the cache check by setting a different dummy function first?

[ ... ]
> @@ -370,18 +370,30 @@ 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;
>  
> -	return err;
> +		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 0;
>  }

[Severity: Medium]
If neither pin is configured with the WAKEUP function via pinctrl (e.g., both
are set to PMIN1 or DISABLE), does calling set_alarm skip writing the
INT1/INT2 registers and return 0 (success) without actually arming the alarm?

Could this mislead the RTC subsystem and userspace into believing an alarm was
successfully programmed when the hardware is not configured to trigger it?

[ ... ]
> @@ -528,6 +528,162 @@ static int s35390a_nvmem_write(void *priv, unsigned int offset, void *val,
> +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)),

[Severity: Medium]
Does exposing the unsupported 'clock' function to the pinctrl subsystem as a
valid option violate the API contract?

> +	[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_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;

If userspace or devicetree requests the 'clock' function expecting a clock
output, it appears this silently configures the pin to disable the interrupt
output instead. Should it return an error instead of silently behaving as
'disable'?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-rtc_s35390a_int1-v10-0-e7b1cbc50a7b@posteo.de?part=5