Re: [PATCH v7 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] rtc: pcf85363: uninitialized ts_last_event state when DT property is missing
- [High] rtc: pcf85363: incorrect TSR3_SHIFT misconfigures hardware
- [High] rtc: pcf85363: GENMASK used as value misconfigures TS pin mode
--

commit f0ef4cbc1465704e262d43ce6c2e731b6a23a590
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
Subject: rtc: pcf85363: add timestamp support with configurable timestamp mode

Add support for the timestamp capture registers on PCF85263/PCF85363,
which latch the time on selected events (TS pin or battery switch-over).
The capture source is selected via the nxp,timestamp-mode device tree
property and latched values are exported through read-only sysfs
attributes.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index 00f37ffcca260..ef846dd231a04 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -102,22 +109,50 @@
>  #define PIN_IO_INTA_OUT	2
>  #define PIN_IO_INTA_HIZ	3
>  
> +#define PIN_IO_TSPM     GENMASK(3, 2)
> +#define PIN_IO_TSIM     BIT(4)
>  
>  #define OSC_CAP_SEL	GENMASK(1, 0)
>  #define OSC_CAP_6000	0x01
>  #define OSC_CAP_12500	0x02
>  
>  #define STOP_EN_STOP	BIT(0)
>  
> +#define RTCM_BIT        BIT(4)
>  
>  #define RESET_CPR	0xa4
>  
>  #define NVRAM_SIZE	0x40
>  
> +#define TSR1_MASK       0x03
> +#define TSR2_MASK       0x07
> +#define TSR3_MASK       0x03
> +#define TSR1_SHIFT      0
> +#define TSR2_SHIFT      2
> +#define TSR3_SHIFT      6

[Severity: High]
Is TSR3_SHIFT correct here? The DT_TS_MODE register uses bits 7:5 for TSR3,
so the shift should be 5. Shifting by 6 maps the value to bits 7:6, corrupting
the TSR3 configuration when written to the hardware.

[ ... ]
> @@ -539,29 +761,72 @@ static int pcf85363_probe(struct i2c_client *client)
>  			.reg_write = pcf85363_nvram_write,
>  		},
>  	};
>  
>  	if (data)
>  		config = data;
>  
> -	pcf85363 = devm_kzalloc(&client->dev, sizeof(struct pcf85363),
> -				GFP_KERNEL);
> +	pcf85363 = devm_kzalloc(&client->dev, sizeof(*pcf85363), GFP_KERNEL);
>  	if (!pcf85363)
>  		return -ENOMEM;
>  
[ ... ]
> +	if (!device_property_read_u32_array(dev, "nxp,timestamp-mode", tsr_mode, 3)) {

[Severity: High]
Can this result in an uninitialized ts_last_event state?

If the "nxp,timestamp-mode" property is missing from the device tree, this
block is skipped and pcf85363_classify_ts_modes() is never called. This leaves
ts_last_event as false (from kzalloc).

If the hardware was pre-configured to a last-event mode by a bootloader, the
driver will treat it as a first-event mode, bypassing the stable-read retry
loop and exposing userspace to torn reads.

> +		bool ts_pin_used;
> +
> +		tsr_mode[0] &= TSR1_MASK;
> +		tsr_mode[1] &= TSR2_MASK;
> +		tsr_mode[2] &= TSR3_MASK;
> +
> +		val = (tsr_mode[2] << TSR3_SHIFT) |
> +		      (tsr_mode[1] << TSR2_SHIFT) |
> +		      (tsr_mode[0] << TSR1_SHIFT);
> +
> +		ret = regmap_write(pcf85363->regmap, DT_TS_MODE, val);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to write timestamp mode register\n");
> +
> +		pcf85363_classify_ts_modes(pcf85363, val);
> +
> +		ts_mode_configured = tsr_mode[0] || tsr_mode[1] || tsr_mode[2];
> +
> +		/*
> +		 * Only the TS-pin capture modes drive the TS pin. Select the
> +		 * timestamp function (TSPM) for those and leave the input mode
> +		 * (TSIM) at its reset default rather than forcing the
> +		 * mechanical-switch detector.
> +		 */
> +		ts_pin_used = tsr_mode[0] == PCF85363_TSR1_FE ||
> +			      tsr_mode[0] == PCF85363_TSR1_LE ||
> +			      tsr_mode[1] == PCF85363_TSR2_FE ||
> +			      tsr_mode[1] == PCF85363_TSR2_LE;
> +
> +		if (ts_pin_used) {
> +			ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
> +						 PIN_IO_TSPM, PIN_IO_TSPM);

[Severity: High]
Does this misconfigure the TS pin mode?

PIN_IO_TSPM evaluates to 0x0C (binary 11 in bits 3:2). Using it as both the
mask and the value to regmap_update_bits() sets bits 3:2 to 11, which
configures the pin as an output according to the datasheet.

To capture timestamps from external events, shouldn't the TS pin be configured
as an input (mode 10)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915034452.4086683-1-lakshay.piplani@nxp.com?part=3