[PATCH] rtc: pcf8563: add SMBus support

[PATCH] rtc: pcf8563: add SMBus support

[Michael Walle]

Since commit 00f1bb9b8486 ("rtc: pcf8563: Switch to regmap") the driver
uses regmap to access the RTC. Regmap itself will figure out if there is
a suitable I2C or SMBus host controllers. Thus, don't restict the driver
to just I2C controllers and make it work on SMBus controllers supporting
i2c block accesses.

This was tested on a custom x86 board.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
 drivers/rtc/rtc-pcf8563.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index 7eebdb058067..eee9bc752b52 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;
-
 	pcf8563 = devm_kzalloc(&client->dev, sizeof(struct pcf8563),
 				GFP_KERNEL);
 	if (!pcf8563)
-- 
2.47.3

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

[sashiko-bot]

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?

>  	pcf8563 = devm_kzalloc(&client->dev, sizeof(struct pcf8563),
>  				GFP_KERNEL);
>  	if (!pcf8563)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930113126.720555-1-mwalle@kernel.org?part=1

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

[Michael Walle]

On Wed Sep 30, 2026 at 1:40 PM CEST, sashiko-bot wrote:
> [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:

That is actually correct. But I'm not sure how this is handled in
the RTC subsystem (?). I've looked at other drivers (e.g. ds1307,
rv3028), which are using regmap and there seems to be no check
either.

-michael


> 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?
>
>>  	pcf8563 = devm_kzalloc(&client->dev, sizeof(struct pcf8563),
>>  				GFP_KERNEL);
>>  	if (!pcf8563)

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

[Alexandre Belloni]

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

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

[Michael Walle]

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

On Wed Sep 30, 2026 at 5:25 PM CEST, Alexandre Belloni wrote:
>> 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?

Yes, did you read my first reply? I'm not sure how this is handled
in the RTC subsystem. Most regmap converted RTC should fall into
this case. How do you propose to handle that? Instead of removing
the check, we could just test for either I2C or SMBus block read.

Anyway, I have an SMBus host controller which supports the block
read, thus it doesn't fall into the single byte read case.

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

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

[Alexandre Belloni]

On 01/10/2026 15:24:07+0200, Michael Walle wrote:
> On Wed Sep 30, 2026 at 5:25 PM CEST, Alexandre Belloni wrote:
> >> 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?
> 
> Yes, did you read my first reply? I'm not sure how this is handled
> in the RTC subsystem. Most regmap converted RTC should fall into
> this case. How do you propose to handle that? Instead of removing
> the check, we could just test for either I2C or SMBus block read.

For some reason, I didn't get you reply in my inbox. For the RTCs that
are not latching, we do read the sec register twice. I guess testing for
I2C or SMBus block read would make more sense here.


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