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

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@nexthop.ai>
> ---
>   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;
> 

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

[Abdurrahman Hussain]

On Wed May 13, 2026 at 7:14 PM PDT, Guenter Roeck wrote:
> 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.
>

Agreed -- calling it an RTC overstates what the chip does.  The
register is a 32-bit seconds counter seeded via SET_RTC and stamped
into each blackbox record.  No battery, no alarm, nothing the RTC API
normally implies; treating it as an rtc_class device was the wrong
fit.

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

Would you prefer I drop anything RTC-shaped from this driver
altogether and reach for something else (e.g. a read/write debugfs
file under pmbus/<dev>/, consistent with the other debugfs entries
this series adds), or would you rather I post a separate thread to
linux-rtc asking how a device like this should expose a host-driven
seconds counter and wait on their guidance before respinning?

The datasheet's intended use is for the host to frequently send the
current timestamp to the chip, so the value stamped into each blackbox
record reflects wall-clock time at the moment of the event.  Either
direction works for me; my mild preference is the debugfs path since
it lets me close the loop without holding up the rest of the series.

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

Understood -- I'll drop the rtc_class approach in v4 either way.

Thanks for the quick feedback,
Abdurrahman