Re: [PATCH v3 4/5] hwmon: (pmbus/adm1266) replace probe-time RTC seed with rtc_class device

[Guenter Roeck]

On 5/12/26 11:56, Abdurrahman Hussain wrote:
> The driver currently writes the device's internal RTC at probe with
> ktime_get_seconds(), which returns CLOCK_MONOTONIC seconds since boot
> and is not a wall-clock value. The resulting timestamps embedded in
> blackbox records are therefore meaningless across reboots, defeating
> the cross-reboot record-correlation use case the field exists for.
>
> Switching the seed to ktime_get_real_seconds() does not actually fix
> this: at probe the system wall clock may not yet have been set (no
> external RTC, no userspace NTP), and seeding unconditionally also
> clobbers whatever valid time the ADM1266 retained across a warm
> reboot.
>
> The data sheet (Rev. D, p. 22) recommends "frequently send the time
> stamp to the ADM1266 to synchronize the UNIX time and reduce the time
> from drifting" when running on the internal oscillator. The clean way
> to expose that policy is an rtc_class device backed by SET_RTC, so
> that userspace tooling (hwclock, chrony, systemd-timesyncd) can drive
> the re-sync against /dev/rtcN once it trusts the system clock - with
> no driver-specific sysfs ABI.
>
> Drop the probe-time seed and adm1266_set_rtc() entirely. Add an
> rtc_class device whose ->read_time and ->set_time callbacks read and
> write the SET_RTC frame. The rtc_class API is second-precision, so
> the SET_RTC fractional-seconds bytes are always written as zero.

This doesn't just set the RTC time, it also acts as a real-time-clock.
That seems undesirable, since it isn't really a real-time clock.
It would also be the first (pseudo) rtc residing outside the rtc
subsystem.

It seems to me that there must be a better solution. The rtc subsystem
needs to be involved in this discussion.

Note - as sent separately - that making this driver dependent on
the RTC subsystem (as would be required by this patch) is unacceptable.

Guenter

> Fixes: 15609d189302 ("hwmon: (pmbus/adm1266) read blackbox")
> Signed-off-by: Abdurrahman Hussain <abdurrahman@xxxxxxxxxx>
> ---
>   drivers/hwmon/pmbus/adm1266.c | 78 +++++++++++++++++++++++++++++++++++--------
>   1 file changed, 65 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 080e7dbd0c06..05b31bb08f38 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -18,8 +18,8 @@
>   #include <linux/nvmem-consumer.h>
>   #include <linux/nvmem-provider.h>
>   #include "pmbus.h"
> +#include <linux/rtc.h>
>   #include <linux/slab.h>
> -#include <linux/timekeeping.h>
>   
>   #define ADM1266_IC_DEVICE_REV	0xAE
>   #define ADM1266_BLACKBOX_CONFIG	0xD3
> @@ -517,21 +517,73 @@ static int adm1266_config_nvmem(struct adm1266_data *data)
>   	return 0;
>   }
>   
> -static int adm1266_set_rtc(struct adm1266_data *data)
> +/*
> + * SET_RTC frame layout (datasheet Rev. D, Table 84):
> + *   bytes [1:0] = fractional seconds, LSB = 1/65536 s
> + *   bytes [5:2] = seconds since 1970-01-01 UTC
> + * The rtc_class API is second-precision, so the fractional bytes are
> + * always written as zero.
> + */
> +static int adm1266_write_rtc(struct i2c_client *client, time64_t secs)
>   {
> -	time64_t kt;
> -	char write_buf[6];
> +	u8 buf[6] = { 0 };
>   	int i;
>   
> -	kt = ktime_get_seconds();
> +	for (i = 0; i < 4; i++)
> +		buf[2 + i] = (secs >> (i * 8)) & 0xFF;
> +
> +	return i2c_smbus_write_block_data(client, ADM1266_SET_RTC, sizeof(buf), buf);
> +}
> +
> +static int adm1266_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	u8 buf[I2C_SMBUS_BLOCK_MAX];
> +	u32 secs;
> +	int ret;
> +	int i;
>   
> -	memset(write_buf, 0, sizeof(write_buf));
> +	guard(pmbus_lock)(client);
> +	ret = i2c_smbus_read_block_data(client, ADM1266_SET_RTC, buf);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 6)
> +		return -EIO;
>   
> +	secs = 0;
>   	for (i = 0; i < 4; i++)
> -		write_buf[2 + i] = (kt >> (i * 8)) & 0xFF;
> +		secs |= (u32)buf[2 + i] << (i * 8);
> +
> +	rtc_time64_to_tm(secs, tm);
> +	return 0;
> +}
>   
> -	return i2c_smbus_write_block_data(data->client, ADM1266_SET_RTC, sizeof(write_buf),
> -					  write_buf);
> +static int adm1266_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	guard(pmbus_lock)(client);
> +	return adm1266_write_rtc(client, rtc_tm_to_time64(tm));
> +}
> +
> +static const struct rtc_class_ops adm1266_rtc_ops = {
> +	.read_time = adm1266_rtc_read_time,
> +	.set_time = adm1266_rtc_set_time,
> +};
> +
> +static int adm1266_register_rtc(struct adm1266_data *data)
> +{
> +	struct rtc_device *rtc;
> +
> +	rtc = devm_rtc_allocate_device(&data->client->dev);
> +	if (IS_ERR(rtc))
> +		return PTR_ERR(rtc);
> +
> +	rtc->ops = &adm1266_rtc_ops;
> +	rtc->range_min = 0;
> +	rtc->range_max = U32_MAX;
> +
> +	return devm_rtc_register_device(rtc);
>   }
>   
>   static int adm1266_probe(struct i2c_client *client)
> @@ -557,10 +609,6 @@ static int adm1266_probe(struct i2c_client *client)
>   	if (ret < 0)
>   		return ret;
>   
> -	ret = adm1266_set_rtc(data);
> -	if (ret < 0)
> -		return ret;
> -
>   	ret = pmbus_do_probe(client, &data->info);
>   	if (ret)
>   		return ret;
> @@ -569,6 +617,10 @@ static int adm1266_probe(struct i2c_client *client)
>   	if (ret < 0)
>   		return ret;
>   
> +	ret = adm1266_register_rtc(data);
> +	if (ret < 0)
> +		return ret;
> +
>   	adm1266_init_debugfs(data);
>   
>   	return 0;