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

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

--- Patch [4]: [PATCH v3 4/5] rtc: s35390a: Add pinctrl ---
commit f348aaf86d41faa0c844cc513c01b349d5a5d4e3
Author: Markus Probst <[email protected]>
Subject: rtc: s35390a: Add pinctrl

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

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

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

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

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

[ ... ]
> @@ -453,13 +528,181 @@ static int s35390a_nvmem_write(void *priv, unsigned int offset, void *val,
[ ... ]
> +	case S35390A_FUNC_WAKEUP:
> +		flag = group == 0 ? S35390A_INT1_MODE_ALARM : S35390A_INT2_MODE_ALARM;
> +		if ((buf & mask) != flag) {
> +			buf = (buf & ~mask) | S35390A_INT_MODE_NOINTR;
> +			break;
> +		}
> +
> +		err = s35390a_read_status(s35390a, &status1);

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

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

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

[ ... ]
> @@ -493,39 +741,16 @@ static int s35390a_probe(struct i2c_client *client)
[ ... ]
> +	err = s35390a_disable_test_mode(s35390a);
> +	if (err < 0) {
> +		dev_err(dev, "error disabling test mode\n");
> +		return err;
> +	}
[ ... ]
> +	err = devm_pinctrl_register_and_init(dev, &s35390a_pinctrl_desc, s35390a, &pctl);
> +	if (err)
> +		return dev_err_probe(dev, err, "pinctrl register failed\n");
> +
> +	err = pinctrl_enable(pctl);
> +	if (err)
> +		return dev_err_probe(dev, err, "pinctrl enable failed\n");
[ ... ]
> +	err = s35390a_read_status(s35390a, &status1);
> +	if (err < 0) {
> +		dev_err(dev, "error resetting chip\n");
> +		return err;
> +	}

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4