Re: [PATCH] rtc: pcf8563: add SMBus support

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 30/09/2026 11:40:04+0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] rtc: pcf8563: time tearing on SMBus adapters lacking block read
> --
> 
> commit 22b80c04bcdbffd7bde30f876c7c59ca8c532372
> Author: Michael Walle <mwalle@kernel.org>
> 
> rtc: pcf8563: add SMBus support
> 
> This commit removes the restriction that the driver can only be used with
> I2C controllers. It delegates bus compatibility checks to regmap, aiming
> to allow the driver to work on SMBus controllers that support block
> accesses.
> 
> > diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
> > index 7eebdb0580674..eee9bc752b52a 100644
> > --- a/drivers/rtc/rtc-pcf8563.c
> > +++ b/drivers/rtc/rtc-pcf8563.c
> > @@ -479,9 +479,6 @@ static int pcf8563_probe(struct i2c_client *client)
> >  
> >  	dev_dbg(&client->dev, "%s\n", __func__);
> >  
> > -	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > -		return -ENODEV;
> > -
> [Severity: High]
> Could removing this check lead to time tearing on restricted SMBus adapters?
> 
> While the commit message mentions SMBus controllers supporting block
> accesses, removing the check entirely delegates bus validation to regmap. If
> the adapter only supports I2C_FUNC_SMBUS_BYTE_DATA and lacks block transfer
> capabilities, regmap silently falls back to regmap_smbus_byte:
> 
> drivers/base/regmap/regmap-i2c.c:regmap_get_i2c_bus() {
>     ...
>     else if (config->val_bits == 8 && config->reg_bits == 8 &&
>              i2c_check_functionality(i2c->adapter,
>                                      I2C_FUNC_SMBUS_BYTE_DATA))
>         bus = &regmap_smbus_byte;
>     ...
> }
> 
> Because of this fallback, when pcf8563_rtc_read_time() reads the time:
> 
> drivers/rtc/rtc-pcf8563.c:pcf8563_rtc_read_time() {
>     ...
>     err = regmap_bulk_read(pcf8563->regmap, PCF8563_REG_ST1, buf,
>                            sizeof(buf));
>     ...
> }
> 
> The bulk read executes as a loop of separate single-byte SMBus reads instead
> of an atomic transaction. Since each SMBus byte transaction issues its own
> STOP condition, the RTC's internal register latch is released between reads.
> 
> Does this allow the hardware time to roll over mid-read and result in
> corrupt time data?

This is a valid concern, did you test?

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com