* Re: [PATCH v3 4/5] hwmon: (pmbus/adm1266) replace probe-time RTC seed with rtc_class device
[not found] ` <20260512-adm1266-v3-4-a81a479b0bb0@nexthop.ai>
@ 2026-05-14 2:14 ` Guenter Roeck
2026-05-14 4:25 ` Abdurrahman Hussain
0 siblings, 1 reply; 2+ messages in thread
From: Guenter Roeck @ 2026-05-14 2:14 UTC (permalink / raw)
To: Abdurrahman Hussain, Alexandru Tachici
Cc: linux-hwmon, linux-kernel, Alexandre Belloni, linux-rtc
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;
>
^ permalink raw reply [flat|nested] 2+ messages in thread