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

[Markus Probst <markus.probst@posteo.de>]

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

On Wed, 2026-08-05 at 20:51 +0000, sashiko-bot@kernel.org wrote:
> 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 <markus.probst@posteo.de>
> 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.
It does not provide dummy stubs.

> 
> > 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?
The only pending interrupt supported by the driver should be the wakeup
alarm, which needs to be cleared in this function in order to set
another alarm.

> 
> [ ... ]
> > @@ -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?
Yes.

> 
> [ ... ]
> > +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.
Possibly.
> 
> [ ... ]
> > @@ -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.
Yes.


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