* [PATCH RESEND v2 1/5] hwmon: (pmbus/adm1266) add firmware_revision debugfs entry
2026-05-11 7:51 [PATCH RESEND v2 0/5] hwmon: (pmbus/adm1266) blackbox, FW rev, rtc_class, GPIO label Abdurrahman Hussain
@ 2026-05-11 7:51 ` Abdurrahman Hussain
2026-05-11 7:51 ` [PATCH RESEND v2 2/5] hwmon: (pmbus/adm1266) add clear_blackbox " Abdurrahman Hussain
` (4 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Abdurrahman Hussain @ 2026-05-11 7:51 UTC (permalink / raw)
To: Guenter Roeck, Alexandru Tachici; +Cc: linux-hwmon, linux-kernel
The ADM1266 reports its firmware revision via the IC_DEVICE_REV
manufacturer-specific block-read command (0xAE, datasheet Rev. D
Table 80). The first three returned bytes are the firmware
major.minor.patch fields. This is useful when correlating field
behaviour against ADI release notes; expose it through debugfs
alongside the existing sequencer_state entry.
The standard PMBus MFR_REVISION (0x9B) register is already exposed
by pmbus_core's debugfs auto-create path and reports the
manufacturer revision, which is a separate thing from the firmware
running on the device.
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/hwmon/pmbus/adm1266.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index d90f8f80be8e..7b8433226176 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -21,6 +21,7 @@
#include <linux/slab.h>
#include <linux/timekeeping.h>
+#define ADM1266_IC_DEVICE_REV 0xAE
#define ADM1266_BLACKBOX_CONFIG 0xD3
#define ADM1266_PDIO_CONFIG 0xD4
#define ADM1266_READ_STATE 0xD9
@@ -331,6 +332,30 @@ static int adm1266_state_read(struct seq_file *s, void *pdata)
return 0;
}
+/*
+ * IC_DEVICE_REV (0xAE) returns an 8-byte block (datasheet Rev. D, Table 80):
+ * [2:0] firmware revision major.minor.patch
+ * [5:3] bootloader revision major.minor.patch
+ * [7:6] silicon revision two ASCII characters
+ */
+static int adm1266_firmware_revision_read(struct seq_file *s, void *pdata)
+{
+ struct device *dev = s->private;
+ struct i2c_client *client = to_i2c_client(dev);
+ u8 buf[I2C_SMBUS_BLOCK_MAX];
+ int ret;
+
+ ret = i2c_smbus_read_block_data(client, ADM1266_IC_DEVICE_REV, buf);
+ if (ret < 0)
+ return ret;
+ if (ret < 3)
+ return -EIO;
+
+ seq_printf(s, "%u.%u.%u\n", buf[0], buf[1], buf[2]);
+
+ return 0;
+}
+
static void adm1266_init_debugfs(struct adm1266_data *data)
{
struct dentry *root;
@@ -343,6 +368,8 @@ static void adm1266_init_debugfs(struct adm1266_data *data)
debugfs_create_devm_seqfile(&data->client->dev, "sequencer_state", data->debugfs_dir,
adm1266_state_read);
+ debugfs_create_devm_seqfile(&data->client->dev, "firmware_revision", data->debugfs_dir,
+ adm1266_firmware_revision_read);
}
static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH RESEND v2 2/5] hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
2026-05-11 7:51 [PATCH RESEND v2 0/5] hwmon: (pmbus/adm1266) blackbox, FW rev, rtc_class, GPIO label Abdurrahman Hussain
2026-05-11 7:51 ` [PATCH RESEND v2 1/5] hwmon: (pmbus/adm1266) add firmware_revision debugfs entry Abdurrahman Hussain
@ 2026-05-11 7:51 ` Abdurrahman Hussain
2026-05-12 0:52 ` sashiko-bot
2026-05-11 7:51 ` [PATCH RESEND v2 3/5] hwmon: (pmbus/adm1266) add powerup_counter " Abdurrahman Hussain
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Abdurrahman Hussain @ 2026-05-11 7:51 UTC (permalink / raw)
To: Guenter Roeck, Alexandru Tachici; +Cc: linux-hwmon, linux-kernel
The ADM1266 blackbox can be configured in two recording modes via
BLACKBOX_CONFIG[0]: cyclic, where the device overwrites the oldest
record once the 32-record buffer fills, and single, where it stops
recording until the buffer is cleared. Deployments that need to
preserve the full record history across multiple fault episodes
typically run in single mode and need a way to clear the buffer
after the records have been collected.
Expose a write-only debugfs file alongside sequencer_state. Writing
any data to it issues the documented clear-blackbox sub-command:
a 2-byte block-write to READ_BLACKBOX (0xDE) with payload
{0xFE, 0x00} (datasheet Rev. D).
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/hwmon/pmbus/adm1266.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 7b8433226176..31aefa8fc75f 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -356,6 +356,36 @@ static int adm1266_firmware_revision_read(struct seq_file *s, void *pdata)
return 0;
}
+/*
+ * Clearing the blackbox is required when the device is configured in
+ * single-recording mode (BLACKBOX_CONFIG[0] = 0): once the 32-record
+ * buffer is full the device stops recording until cleared.
+ *
+ * The clear is issued as a 2-byte block-write to READ_BLACKBOX with
+ * payload {0xFE, 0x00} per the datasheet.
+ */
+static ssize_t adm1266_clear_blackbox_write(struct file *file, const char __user *ubuf,
+ size_t count, loff_t *ppos)
+{
+ struct i2c_client *client = file->private_data;
+ u8 payload[2] = { 0xFE, 0x00 };
+ int ret;
+
+ ret = i2c_smbus_write_block_data(client, ADM1266_READ_BLACKBOX,
+ sizeof(payload), payload);
+ if (ret < 0)
+ return ret;
+
+ return count;
+}
+
+static const struct file_operations adm1266_clear_blackbox_fops = {
+ .owner = THIS_MODULE,
+ .open = simple_open,
+ .write = adm1266_clear_blackbox_write,
+ .llseek = noop_llseek,
+};
+
static void adm1266_init_debugfs(struct adm1266_data *data)
{
struct dentry *root;
@@ -370,6 +400,8 @@ static void adm1266_init_debugfs(struct adm1266_data *data)
adm1266_state_read);
debugfs_create_devm_seqfile(&data->client->dev, "firmware_revision", data->debugfs_dir,
adm1266_firmware_revision_read);
+ debugfs_create_file("clear_blackbox", 0200, data->debugfs_dir, data->client,
+ &adm1266_clear_blackbox_fops);
}
static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH RESEND v2 2/5] hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
2026-05-11 7:51 ` [PATCH RESEND v2 2/5] hwmon: (pmbus/adm1266) add clear_blackbox " Abdurrahman Hussain
@ 2026-05-12 0:52 ` sashiko-bot
2026-05-12 8:27 ` Abdurrahman Hussain
0 siblings, 1 reply; 17+ messages in thread
From: sashiko-bot @ 2026-05-12 0:52 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: linux-hwmon
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The debugfs write handler `adm1266_clear_blackbox_write` performs an I2C transaction without acquiring the required hardware monitoring locks, violating hwmon subsystem guidelines.
--
commit ab9c19373562d58415e28511b037545cfd278c2a
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Subject: hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
This commit adds a debugfs entry to clear the ADM1266 blackbox buffer by
issuing a block-write command to the device. This is needed when the
device operates in single-recording mode to preserve fault history.
> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 7b8433226176f..31aefa8fc75f5 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
[ ... ]
> +static ssize_t adm1266_clear_blackbox_write(struct file *file, const char __user *ubuf,
> + size_t count, loff_t *ppos)
> +{
> + struct i2c_client *client = file->private_data;
> + u8 payload[2] = { 0xFE, 0x00 };
> + int ret;
> +
> + ret = i2c_smbus_write_block_data(client, ADM1266_READ_BLACKBOX,
> + sizeof(payload), payload);
Does this I2C block write need to be protected by hwmon_lock() and
hwmon_unlock()?
The hardware monitoring subsystem guidelines require drivers to implement
locking for attributes registered outside of the core registration functions,
such as debugfs entries.
Without these locks, could this write operation race with other multi-record
transactions? For example, if this clear command is interleaved while
adm1266_nvmem_read_blackbox() is iterating through multiple records, might
it corrupt the read sequence?
> + if (ret < 0)
> + return ret;
> +
> + return count;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-adm1266-v2-0-7a6d34f0de5d@nexthop.ai?part=2
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH RESEND v2 2/5] hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
2026-05-12 0:52 ` sashiko-bot
@ 2026-05-12 8:27 ` Abdurrahman Hussain
2026-05-12 9:19 ` Guenter Roeck
0 siblings, 1 reply; 17+ messages in thread
From: Abdurrahman Hussain @ 2026-05-12 8:27 UTC (permalink / raw)
To: sashiko, Abdurrahman Hussain; +Cc: linux-hwmon
On Mon May 11, 2026 at 5:52 PM PDT, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The debugfs write handler `adm1266_clear_blackbox_write` performs an I2C transaction without acquiring the required hardware monitoring locks, violating hwmon subsystem guidelines.
> --
>
> commit ab9c19373562d58415e28511b037545cfd278c2a
> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
> Subject: hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
>
> This commit adds a debugfs entry to clear the ADM1266 blackbox buffer by
> issuing a block-write command to the device. This is needed when the
> device operates in single-recording mode to preserve fault history.
>
>> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
>> index 7b8433226176f..31aefa8fc75f5 100644
>> --- a/drivers/hwmon/pmbus/adm1266.c
>> +++ b/drivers/hwmon/pmbus/adm1266.c
>
> [ ... ]
>
>> +static ssize_t adm1266_clear_blackbox_write(struct file *file, const char __user *ubuf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct i2c_client *client = file->private_data;
>> + u8 payload[2] = { 0xFE, 0x00 };
>> + int ret;
>> +
>> + ret = i2c_smbus_write_block_data(client, ADM1266_READ_BLACKBOX,
>> + sizeof(payload), payload);
>
> Does this I2C block write need to be protected by hwmon_lock() and
> hwmon_unlock()?
>
> The hardware monitoring subsystem guidelines require drivers to implement
> locking for attributes registered outside of the core registration functions,
> such as debugfs entries.
>
> Without these locks, could this write operation race with other multi-record
> transactions? For example, if this clear command is interleaved while
> adm1266_nvmem_read_blackbox() is iterating through multiple records, might
> it corrupt the read sequence?
>
>> + if (ret < 0)
>> + return ret;
>> +
>> + return count;
>> +}
Good catch!
Will use pmbus_lock_interruptible() in v3.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH RESEND v2 2/5] hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
2026-05-12 8:27 ` Abdurrahman Hussain
@ 2026-05-12 9:19 ` Guenter Roeck
2026-05-12 10:20 ` Abdurrahman Hussain
0 siblings, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2026-05-12 9:19 UTC (permalink / raw)
To: Abdurrahman Hussain, sashiko; +Cc: linux-hwmon
On 5/12/26 01:27, Abdurrahman Hussain wrote:
> On Mon May 11, 2026 at 5:52 PM PDT, sashiko-bot wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [High] The debugfs write handler `adm1266_clear_blackbox_write` performs an I2C transaction without acquiring the required hardware monitoring locks, violating hwmon subsystem guidelines.
>> --
>>
>> commit ab9c19373562d58415e28511b037545cfd278c2a
>> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>> Subject: hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
>>
>> This commit adds a debugfs entry to clear the ADM1266 blackbox buffer by
>> issuing a block-write command to the device. This is needed when the
>> device operates in single-recording mode to preserve fault history.
>>
>>> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
>>> index 7b8433226176f..31aefa8fc75f5 100644
>>> --- a/drivers/hwmon/pmbus/adm1266.c
>>> +++ b/drivers/hwmon/pmbus/adm1266.c
>>
>> [ ... ]
>>
>>> +static ssize_t adm1266_clear_blackbox_write(struct file *file, const char __user *ubuf,
>>> + size_t count, loff_t *ppos)
>>> +{
>>> + struct i2c_client *client = file->private_data;
>>> + u8 payload[2] = { 0xFE, 0x00 };
>>> + int ret;
>>> +
>>> + ret = i2c_smbus_write_block_data(client, ADM1266_READ_BLACKBOX,
>>> + sizeof(payload), payload);
>>
>> Does this I2C block write need to be protected by hwmon_lock() and
>> hwmon_unlock()?
>>
>> The hardware monitoring subsystem guidelines require drivers to implement
>> locking for attributes registered outside of the core registration functions,
>> such as debugfs entries.
>>
>> Without these locks, could this write operation race with other multi-record
>> transactions? For example, if this clear command is interleaved while
>> adm1266_nvmem_read_blackbox() is iterating through multiple records, might
>> it corrupt the read sequence?
>>
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + return count;
>>> +}
>
> Good catch!
>
> Will use pmbus_lock_interruptible() in v3.
>
There is also "guard(pmbus_lock)(client);" or "scoped_guard(pmbus_lock, client) { ... }".
Guenter
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH RESEND v2 2/5] hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
2026-05-12 9:19 ` Guenter Roeck
@ 2026-05-12 10:20 ` Abdurrahman Hussain
2026-05-12 13:55 ` Guenter Roeck
0 siblings, 1 reply; 17+ messages in thread
From: Abdurrahman Hussain @ 2026-05-12 10:20 UTC (permalink / raw)
To: Guenter Roeck, Abdurrahman Hussain, sashiko; +Cc: linux-hwmon, Guenter Roeck
On Tue May 12, 2026 at 2:19 AM PDT, Guenter Roeck wrote:
> On 5/12/26 01:27, Abdurrahman Hussain wrote:
>> On Mon May 11, 2026 at 5:52 PM PDT, sashiko-bot wrote:
>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>> - [High] The debugfs write handler `adm1266_clear_blackbox_write` performs an I2C transaction without acquiring the required hardware monitoring locks, violating hwmon subsystem guidelines.
>>> --
>>>
>>> commit ab9c19373562d58415e28511b037545cfd278c2a
>>> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>>> Subject: hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
>>>
>>> This commit adds a debugfs entry to clear the ADM1266 blackbox buffer by
>>> issuing a block-write command to the device. This is needed when the
>>> device operates in single-recording mode to preserve fault history.
>>>
>>>> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
>>>> index 7b8433226176f..31aefa8fc75f5 100644
>>>> --- a/drivers/hwmon/pmbus/adm1266.c
>>>> +++ b/drivers/hwmon/pmbus/adm1266.c
>>>
>>> [ ... ]
>>>
>>>> +static ssize_t adm1266_clear_blackbox_write(struct file *file, const char __user *ubuf,
>>>> + size_t count, loff_t *ppos)
>>>> +{
>>>> + struct i2c_client *client = file->private_data;
>>>> + u8 payload[2] = { 0xFE, 0x00 };
>>>> + int ret;
>>>> +
>>>> + ret = i2c_smbus_write_block_data(client, ADM1266_READ_BLACKBOX,
>>>> + sizeof(payload), payload);
>>>
>>> Does this I2C block write need to be protected by hwmon_lock() and
>>> hwmon_unlock()?
>>>
>>> The hardware monitoring subsystem guidelines require drivers to implement
>>> locking for attributes registered outside of the core registration functions,
>>> such as debugfs entries.
>>>
>>> Without these locks, could this write operation race with other multi-record
>>> transactions? For example, if this clear command is interleaved while
>>> adm1266_nvmem_read_blackbox() is iterating through multiple records, might
>>> it corrupt the read sequence?
>>>
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + return count;
>>>> +}
>>
>> Good catch!
>>
>> Will use pmbus_lock_interruptible() in v3.
>>
>
> There is also "guard(pmbus_lock)(client);" or "scoped_guard(pmbus_lock, client) { ... }".
> Guenter
Thanks for the tip! Does that mean you're ok with losing the
interruptibility, since there's no DEFINE_GUARD_COND for interruptible
variant?
Abdurrahman
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH RESEND v2 2/5] hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
2026-05-12 10:20 ` Abdurrahman Hussain
@ 2026-05-12 13:55 ` Guenter Roeck
0 siblings, 0 replies; 17+ messages in thread
From: Guenter Roeck @ 2026-05-12 13:55 UTC (permalink / raw)
To: Abdurrahman Hussain, sashiko; +Cc: linux-hwmon, Guenter Roeck
On 5/12/26 03:20, Abdurrahman Hussain wrote:
> On Tue May 12, 2026 at 2:19 AM PDT, Guenter Roeck wrote:
>> On 5/12/26 01:27, Abdurrahman Hussain wrote:
>>> On Mon May 11, 2026 at 5:52 PM PDT, sashiko-bot wrote:
>>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>>> - [High] The debugfs write handler `adm1266_clear_blackbox_write` performs an I2C transaction without acquiring the required hardware monitoring locks, violating hwmon subsystem guidelines.
>>>> --
>>>>
>>>> commit ab9c19373562d58415e28511b037545cfd278c2a
>>>> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>>>> Subject: hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry
>>>>
>>>> This commit adds a debugfs entry to clear the ADM1266 blackbox buffer by
>>>> issuing a block-write command to the device. This is needed when the
>>>> device operates in single-recording mode to preserve fault history.
>>>>
>>>>> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
>>>>> index 7b8433226176f..31aefa8fc75f5 100644
>>>>> --- a/drivers/hwmon/pmbus/adm1266.c
>>>>> +++ b/drivers/hwmon/pmbus/adm1266.c
>>>>
>>>> [ ... ]
>>>>
>>>>> +static ssize_t adm1266_clear_blackbox_write(struct file *file, const char __user *ubuf,
>>>>> + size_t count, loff_t *ppos)
>>>>> +{
>>>>> + struct i2c_client *client = file->private_data;
>>>>> + u8 payload[2] = { 0xFE, 0x00 };
>>>>> + int ret;
>>>>> +
>>>>> + ret = i2c_smbus_write_block_data(client, ADM1266_READ_BLACKBOX,
>>>>> + sizeof(payload), payload);
>>>>
>>>> Does this I2C block write need to be protected by hwmon_lock() and
>>>> hwmon_unlock()?
>>>>
>>>> The hardware monitoring subsystem guidelines require drivers to implement
>>>> locking for attributes registered outside of the core registration functions,
>>>> such as debugfs entries.
>>>>
>>>> Without these locks, could this write operation race with other multi-record
>>>> transactions? For example, if this clear command is interleaved while
>>>> adm1266_nvmem_read_blackbox() is iterating through multiple records, might
>>>> it corrupt the read sequence?
>>>>
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>> +
>>>>> + return count;
>>>>> +}
>>>
>>> Good catch!
>>>
>>> Will use pmbus_lock_interruptible() in v3.
>>>
>>
>> There is also "guard(pmbus_lock)(client);" or "scoped_guard(pmbus_lock, client) { ... }".
>> Guenter
>
> Thanks for the tip! Does that mean you're ok with losing the
> interruptibility, since there's no DEFINE_GUARD_COND for interruptible
> variant?
>
Yes. If anything, I'd like to drop the _interruptible variant entirely.
I only keep it for historic reasons.
Guenter
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH RESEND v2 3/5] hwmon: (pmbus/adm1266) add powerup_counter debugfs entry
2026-05-11 7:51 [PATCH RESEND v2 0/5] hwmon: (pmbus/adm1266) blackbox, FW rev, rtc_class, GPIO label Abdurrahman Hussain
2026-05-11 7:51 ` [PATCH RESEND v2 1/5] hwmon: (pmbus/adm1266) add firmware_revision debugfs entry Abdurrahman Hussain
2026-05-11 7:51 ` [PATCH RESEND v2 2/5] hwmon: (pmbus/adm1266) add clear_blackbox " Abdurrahman Hussain
@ 2026-05-11 7:51 ` Abdurrahman Hussain
2026-05-12 1:29 ` sashiko-bot
2026-05-11 7:51 ` [PATCH RESEND v2 4/5] hwmon: (pmbus/adm1266) replace probe-time RTC seed with rtc_class device Abdurrahman Hussain
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Abdurrahman Hussain @ 2026-05-11 7:51 UTC (permalink / raw)
To: Guenter Roeck, Alexandru Tachici; +Cc: linux-hwmon, linux-kernel
The ADM1266 maintains a 16-bit non-volatile POWERUP_COUNTER register
(0xE4, datasheet Rev. D, Table 93) that increments on every power
cycle and cannot be reset by the host. Each blackbox record already
embeds the counter at record time, so the standalone live value is
primarily useful for matching a captured record back to the boot it
came from when correlating logs.
Expose it as a read-only debugfs file alongside firmware_revision.
The block-read returns two payload bytes in little-endian order.
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/hwmon/pmbus/adm1266.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 31aefa8fc75f..99e92a8365fb 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -28,6 +28,7 @@
#define ADM1266_READ_BLACKBOX 0xDE
#define ADM1266_SET_RTC 0xDF
#define ADM1266_GPIO_CONFIG 0xE1
+#define ADM1266_POWERUP_COUNTER 0xE4
#define ADM1266_BLACKBOX_INFO 0xE6
#define ADM1266_PDIO_STATUS 0xE9
#define ADM1266_GPIO_STATUS 0xEA
@@ -356,6 +357,31 @@ static int adm1266_firmware_revision_read(struct seq_file *s, void *pdata)
return 0;
}
+/*
+ * POWERUP_COUNTER (0xE4) is a 2-byte little-endian non-volatile counter
+ * that increments on every device power cycle (datasheet Rev. D, Table
+ * 93). It saturates at 65535 and cannot be reset by the host. Each
+ * blackbox record embeds the counter value at record time, so this live
+ * read is mainly useful for matching a record back to its boot.
+ */
+static int adm1266_powerup_counter_read(struct seq_file *s, void *pdata)
+{
+ struct device *dev = s->private;
+ struct i2c_client *client = to_i2c_client(dev);
+ u8 buf[I2C_SMBUS_BLOCK_MAX];
+ int ret;
+
+ ret = i2c_smbus_read_block_data(client, ADM1266_POWERUP_COUNTER, buf);
+ if (ret < 0)
+ return ret;
+ if (ret != 2)
+ return -EIO;
+
+ seq_printf(s, "%u\n", buf[0] | (buf[1] << 8));
+
+ return 0;
+}
+
/*
* Clearing the blackbox is required when the device is configured in
* single-recording mode (BLACKBOX_CONFIG[0] = 0): once the 32-record
@@ -400,6 +426,8 @@ static void adm1266_init_debugfs(struct adm1266_data *data)
adm1266_state_read);
debugfs_create_devm_seqfile(&data->client->dev, "firmware_revision", data->debugfs_dir,
adm1266_firmware_revision_read);
+ debugfs_create_devm_seqfile(&data->client->dev, "powerup_counter", data->debugfs_dir,
+ adm1266_powerup_counter_read);
debugfs_create_file("clear_blackbox", 0200, data->debugfs_dir, data->client,
&adm1266_clear_blackbox_fops);
}
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH RESEND v2 3/5] hwmon: (pmbus/adm1266) add powerup_counter debugfs entry
2026-05-11 7:51 ` [PATCH RESEND v2 3/5] hwmon: (pmbus/adm1266) add powerup_counter " Abdurrahman Hussain
@ 2026-05-12 1:29 ` sashiko-bot
2026-05-12 8:34 ` Abdurrahman Hussain
0 siblings, 1 reply; 17+ messages in thread
From: sashiko-bot @ 2026-05-12 1:29 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: linux-hwmon
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing lock in debugfs read function violating hwmon subsystem guidelines.
--
commit 59d29098021812d7f7bd1712ee63ceafd8397c54
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
hwmon: (pmbus/adm1266) add powerup_counter debugfs entry
The ADM1266 maintains a 16-bit non-volatile POWERUP_COUNTER register
(0xE4, datasheet Rev. D, Table 93) that increments on every power
cycle and cannot be reset by the host. Each blackbox record already
embeds the counter at record time, so the standalone live value is
primarily useful for matching a captured record back to the boot it
came from when correlating logs.
> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 31aefa8fc75f5..99e92a8365fb7 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
[ ... ]
> @@ -356,6 +357,31 @@ static int adm1266_firmware_revision_read(struct seq_file *s, void *pdata)
> return 0;
> }
>
> +/*
> + * POWERUP_COUNTER (0xE4) is a 2-byte little-endian non-volatile counter
> + * that increments on every device power cycle (datasheet Rev. D, Table
> + * 93). It saturates at 65535 and cannot be reset by the host. Each
> + * blackbox record embeds the counter value at record time, so this live
> + * read is mainly useful for matching a record back to its boot.
> + */
> +static int adm1266_powerup_counter_read(struct seq_file *s, void *pdata)
> +{
> + struct device *dev = s->private;
> + struct i2c_client *client = to_i2c_client(dev);
> + u8 buf[I2C_SMBUS_BLOCK_MAX];
> + int ret;
> +
> + ret = i2c_smbus_read_block_data(client, ADM1266_POWERUP_COUNTER, buf);
Does this debugfs read need to be protected by hwmon_lock()?
According to the hwmon subsystem guidelines:
"The hardware monitoring subsystem core serializes sysfs operations
for attributes registered with the info parameter of
hwmon_device_register_with_info() and
devm_hwmon_device_register_with_info().
Drivers must implement locking required for interrupt handling and for
attributes registered by any other means. Drivers should use hwmon_lock()
and hwmon_unlock() for this purpose."
Since this attribute is registered via debugfs rather than the info
parameter, concurrent I2C operations here could potentially interleave
with multi-part transactions from the hwmon core (like PMBus page switching),
which could disrupt the device's state machine or cause cross-page data
corruption.
> + if (ret < 0)
> + return ret;
> + if (ret != 2)
> + return -EIO;
> +
> + seq_printf(s, "%u\n", buf[0] | (buf[1] << 8));
> +
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-adm1266-v2-0-7a6d34f0de5d@nexthop.ai?part=3
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH RESEND v2 3/5] hwmon: (pmbus/adm1266) add powerup_counter debugfs entry
2026-05-12 1:29 ` sashiko-bot
@ 2026-05-12 8:34 ` Abdurrahman Hussain
0 siblings, 0 replies; 17+ messages in thread
From: Abdurrahman Hussain @ 2026-05-12 8:34 UTC (permalink / raw)
To: sashiko, Abdurrahman Hussain; +Cc: linux-hwmon
On Mon May 11, 2026 at 6:29 PM PDT, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Missing lock in debugfs read function violating hwmon subsystem guidelines.
> --
>
> commit 59d29098021812d7f7bd1712ee63ceafd8397c54
> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
> hwmon: (pmbus/adm1266) add powerup_counter debugfs entry
>
> The ADM1266 maintains a 16-bit non-volatile POWERUP_COUNTER register
> (0xE4, datasheet Rev. D, Table 93) that increments on every power
> cycle and cannot be reset by the host. Each blackbox record already
> embeds the counter at record time, so the standalone live value is
> primarily useful for matching a captured record back to the boot it
> came from when correlating logs.
>
>> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
>> index 31aefa8fc75f5..99e92a8365fb7 100644
>> --- a/drivers/hwmon/pmbus/adm1266.c
>> +++ b/drivers/hwmon/pmbus/adm1266.c
> [ ... ]
>> @@ -356,6 +357,31 @@ static int adm1266_firmware_revision_read(struct seq_file *s, void *pdata)
>> return 0;
>> }
>>
>> +/*
>> + * POWERUP_COUNTER (0xE4) is a 2-byte little-endian non-volatile counter
>> + * that increments on every device power cycle (datasheet Rev. D, Table
>> + * 93). It saturates at 65535 and cannot be reset by the host. Each
>> + * blackbox record embeds the counter value at record time, so this live
>> + * read is mainly useful for matching a record back to its boot.
>> + */
>> +static int adm1266_powerup_counter_read(struct seq_file *s, void *pdata)
>> +{
>> + struct device *dev = s->private;
>> + struct i2c_client *client = to_i2c_client(dev);
>> + u8 buf[I2C_SMBUS_BLOCK_MAX];
>> + int ret;
>> +
>> + ret = i2c_smbus_read_block_data(client, ADM1266_POWERUP_COUNTER, buf);
>
> Does this debugfs read need to be protected by hwmon_lock()?
>
> According to the hwmon subsystem guidelines:
>
> "The hardware monitoring subsystem core serializes sysfs operations
> for attributes registered with the info parameter of
> hwmon_device_register_with_info() and
> devm_hwmon_device_register_with_info().
> Drivers must implement locking required for interrupt handling and for
> attributes registered by any other means. Drivers should use hwmon_lock()
> and hwmon_unlock() for this purpose."
>
> Since this attribute is registered via debugfs rather than the info
> parameter, concurrent I2C operations here could potentially interleave
> with multi-part transactions from the hwmon core (like PMBus page switching),
> which could disrupt the device's state machine or cause cross-page data
> corruption.
>
>> + if (ret < 0)
>> + return ret;
>> + if (ret != 2)
>> + return -EIO;
>> +
>> + seq_printf(s, "%u\n", buf[0] | (buf[1] << 8));
>> +
>> + return 0;
>> +}
POWERUP_COUNTER (0xE4) is a global, non-paged register. The read here
is a single i2c_smbus_read_block_data() call, which the i2c core
already serialises at the bus level for that transfer. There is no
driver-side multi-step sequence to interleave with, and PMBus PAGE
state has no bearing on the value returned, so an extra lock around
this read would not be protecting anything.
(For what it's worth, hwmon_lock()/hwmon_unlock() aren't exported as
a generic kernel API. The relevant primitive for a PMBus driver is
pmbus_lock() / pmbus_unlock() from drivers/hwmon/pmbus/pmbus.h, which
fences against the PMBus core's own PAGE+register sequences.)
I have added pmbus_lock_interruptible() coverage in v3 for the paths
where it is justified:
- clear_blackbox (patch 2/5) shares command 0xDE with
adm1266_nvmem_read_blackbox(), and the read path walks records
one at a time; a concurrent clear could land between records, so
both paths now hold the lock across the full sequence.
- The RTC read_time/set_time callbacks (patch 4/5) can fire while
the PMBus core has a PAGE write outstanding, so the I2C transfer
is wrapped in pmbus_lock_interruptible() to avoid interleaving.
I would prefer to keep the powerup_counter read lock-free unless
there is a specific scenario I am missing.
Best regards,
Abdurrahman
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH RESEND v2 4/5] hwmon: (pmbus/adm1266) replace probe-time RTC seed with rtc_class device
2026-05-11 7:51 [PATCH RESEND v2 0/5] hwmon: (pmbus/adm1266) blackbox, FW rev, rtc_class, GPIO label Abdurrahman Hussain
` (2 preceding siblings ...)
2026-05-11 7:51 ` [PATCH RESEND v2 3/5] hwmon: (pmbus/adm1266) add powerup_counter " Abdurrahman Hussain
@ 2026-05-11 7:51 ` Abdurrahman Hussain
2026-05-12 3:32 ` sashiko-bot
2026-05-11 7:51 ` [PATCH RESEND v2 5/5] hwmon: (pmbus/adm1266) include adapter number in GPIO line label Abdurrahman Hussain
2026-05-11 14:10 ` [PATCH RESEND v2 0/5] hwmon: (pmbus/adm1266) blackbox, FW rev, rtc_class, GPIO label Guenter Roeck
5 siblings, 1 reply; 17+ messages in thread
From: Abdurrahman Hussain @ 2026-05-11 7:51 UTC (permalink / raw)
To: Guenter Roeck, Alexandru Tachici; +Cc: linux-hwmon, linux-kernel
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.
Fixes: 15609d189302 ("hwmon: (pmbus/adm1266) read blackbox")
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/hwmon/pmbus/adm1266.c | 70 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 60 insertions(+), 10 deletions(-)
diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 99e92a8365fb..0dfb02db8683 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
@@ -513,21 +513,71 @@ 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));
+ 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;
+}
+
+static int adm1266_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+
+ 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 i2c_smbus_write_block_data(data->client, ADM1266_SET_RTC, sizeof(write_buf),
- write_buf);
+ return devm_rtc_register_device(rtc);
}
static int adm1266_probe(struct i2c_client *client)
@@ -553,7 +603,7 @@ static int adm1266_probe(struct i2c_client *client)
if (ret < 0)
return ret;
- ret = adm1266_set_rtc(data);
+ ret = adm1266_register_rtc(data);
if (ret < 0)
return ret;
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH RESEND v2 4/5] hwmon: (pmbus/adm1266) replace probe-time RTC seed with rtc_class device
2026-05-11 7:51 ` [PATCH RESEND v2 4/5] hwmon: (pmbus/adm1266) replace probe-time RTC seed with rtc_class device Abdurrahman Hussain
@ 2026-05-12 3:32 ` sashiko-bot
2026-05-12 8:42 ` Abdurrahman Hussain
0 siblings, 1 reply; 17+ messages in thread
From: sashiko-bot @ 2026-05-12 3:32 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: linux-hwmon
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing locking in RTC callbacks causes race condition with PMBus transactions.
- [Medium] Premature exposure of the RTC device before hardware initialization is complete.
--
commit 20e4a34f4a236495f598a1957552fb4fea44b3d3
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
hwmon: (pmbus/adm1266) replace probe-time RTC seed with rtc_class device
This commit replaces the probe-time RTC seeding with an rtc_class device
backed by SET_RTC. This allows userspace tooling to drive the re-sync against
/dev/rtcN once it trusts the system clock.
> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 99e92a8365fb7..0dfb02db86837 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
[ ... ]
> +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));
> + ret = i2c_smbus_read_block_data(client, ADM1266_SET_RTC, buf);
Since this RTC device is exposed to userspace, can this callback be invoked
concurrently with hwmon sysfs attributes managed by the PMBus core?
The PMBus core uses pmbus_lock to serialize multi-step sequences. If an RTC
I2C transaction interleaves in the middle of a PMBus sequence, can it disrupt
the device's internal state machine or cause unpredictable behavior?
Should this callback acquire pmbus_lock(client) (or its interruptible
variant) around the I2C transfer to prevent a race condition?
> + if (ret < 0)
> + return ret;
[ ... ]
> +static int adm1266_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> +
> + return adm1266_write_rtc(client, rtc_tm_to_time64(tm));
> +}
Does this callback also need to acquire pmbus_lock(client) around the I2C
transfer to prevent concurrent access with PMBus transactions?
[ ... ]
> @@ -553,7 +603,7 @@ static int adm1266_probe(struct i2c_client *client)
> if (ret < 0)
> return ret;
>
> - ret = adm1266_set_rtc(data);
> + ret = adm1266_register_rtc(data);
> if (ret < 0)
> return ret;
By registering the RTC device here, is the /dev/rtcX interface exposed to
userspace before pmbus_do_probe() completes the hardware discovery and
initialization sequence?
If a userspace process immediately opens the RTC device and issues read/write
commands, can the resulting I2C traffic interleave with the driver's
initial setup?
Should adm1266_register_rtc() be called at the end of the probe function,
after pmbus_do_probe() succeeds, to ensure the device is fully initialized
before it is accessed externally?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-adm1266-v2-0-7a6d34f0de5d@nexthop.ai?part=4
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH RESEND v2 4/5] hwmon: (pmbus/adm1266) replace probe-time RTC seed with rtc_class device
2026-05-12 3:32 ` sashiko-bot
@ 2026-05-12 8:42 ` Abdurrahman Hussain
0 siblings, 0 replies; 17+ messages in thread
From: Abdurrahman Hussain @ 2026-05-12 8:42 UTC (permalink / raw)
To: sashiko, Abdurrahman Hussain; +Cc: linux-hwmon
On Mon May 11, 2026 at 8:32 PM PDT, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Missing locking in RTC callbacks causes race condition with PMBus transactions.
> - [Medium] Premature exposure of the RTC device before hardware initialization is complete.
> --
>
> commit 20e4a34f4a236495f598a1957552fb4fea44b3d3
> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
> hwmon: (pmbus/adm1266) replace probe-time RTC seed with rtc_class device
>
> This commit replaces the probe-time RTC seeding with an rtc_class device
> backed by SET_RTC. This allows userspace tooling to drive the re-sync against
> /dev/rtcN once it trusts the system clock.
>
>> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
>> index 99e92a8365fb7..0dfb02db86837 100644
>> --- a/drivers/hwmon/pmbus/adm1266.c
>> +++ b/drivers/hwmon/pmbus/adm1266.c
> [ ... ]
>> +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));
>> + ret = i2c_smbus_read_block_data(client, ADM1266_SET_RTC, buf);
>
> Since this RTC device is exposed to userspace, can this callback be invoked
> concurrently with hwmon sysfs attributes managed by the PMBus core?
>
> The PMBus core uses pmbus_lock to serialize multi-step sequences. If an RTC
> I2C transaction interleaves in the middle of a PMBus sequence, can it disrupt
> the device's internal state machine or cause unpredictable behavior?
>
> Should this callback acquire pmbus_lock(client) (or its interruptible
> variant) around the I2C transfer to prevent a race condition?
>
>> + if (ret < 0)
>> + return ret;
> [ ... ]
>> +static int adm1266_rtc_set_time(struct device *dev, struct rtc_time *tm)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> +
>> + return adm1266_write_rtc(client, rtc_tm_to_time64(tm));
>> +}
>
> Does this callback also need to acquire pmbus_lock(client) around the I2C
> transfer to prevent concurrent access with PMBus transactions?
>
> [ ... ]
Agreed. In v3 both callbacks wrap their I2C transfer in
pmbus_lock_interruptible() / pmbus_unlock(). adm1266_write_rtc() is
no longer reachable from probe (the probe-time SET_RTC seed is what
this patch removes), so the new lock acquisition is purely on the
userspace path and a signal can interrupt a contended wait cleanly.
>> @@ -553,7 +603,7 @@ static int adm1266_probe(struct i2c_client *client)
>> if (ret < 0)
>> return ret;
>>
>> - ret = adm1266_set_rtc(data);
>> + ret = adm1266_register_rtc(data);
>> if (ret < 0)
>> return ret;
>
> By registering the RTC device here, is the /dev/rtcX interface exposed to
> userspace before pmbus_do_probe() completes the hardware discovery and
> initialization sequence?
>
> If a userspace process immediately opens the RTC device and issues read/write
> commands, can the resulting I2C traffic interleave with the driver's
> initial setup?
>
> Should adm1266_register_rtc() be called at the end of the probe function,
> after pmbus_do_probe() succeeds, to ensure the device is fully initialized
> before it is accessed externally?
The ordering follows the existing pattern for the GPIO and nvmem
registrations in this driver. None of the three registration helpers
performs I2C at registration time — devm_{gpiochip,nvmem,rtc}_*()
only install callbacks that fire later when userspace touches the
interface. With the v3 locking in place, any userspace RTC access
that lands during pmbus_do_probe() will serialise on
pmbus_lock_interruptible() against the core's PAGE+register
sequences rather than interleave with them, so the
window-without-protection that the original concern describes is
closed by the locking change.
I would prefer to leave the registration ordering as-is for
consistency with the GPIO and nvmem paths, but I am happy to move
all three after pmbus_do_probe() if you would rather have a stricter
"nothing visible to userspace until probe is done" invariant across
the driver. Happy to do that in a follow-up if so.
Best regards,
Abdurrahman
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH RESEND v2 5/5] hwmon: (pmbus/adm1266) include adapter number in GPIO line label
2026-05-11 7:51 [PATCH RESEND v2 0/5] hwmon: (pmbus/adm1266) blackbox, FW rev, rtc_class, GPIO label Abdurrahman Hussain
` (3 preceding siblings ...)
2026-05-11 7:51 ` [PATCH RESEND v2 4/5] hwmon: (pmbus/adm1266) replace probe-time RTC seed with rtc_class device Abdurrahman Hussain
@ 2026-05-11 7:51 ` Abdurrahman Hussain
2026-05-11 14:10 ` [PATCH RESEND v2 0/5] hwmon: (pmbus/adm1266) blackbox, FW rev, rtc_class, GPIO label Guenter Roeck
5 siblings, 0 replies; 17+ messages in thread
From: Abdurrahman Hussain @ 2026-05-11 7:51 UTC (permalink / raw)
To: Guenter Roeck, Alexandru Tachici; +Cc: linux-hwmon, linux-kernel
Platforms that fit more than one ADM1266 on different I2C buses at
the same 7-bit slave address (a common shelf-management pattern,
e.g. one device per power domain) end up with duplicate GPIO line
labels because the existing format only includes the slave address.
Including the adapter number disambiguates them.
The adapter number is formatted as decimal to match the i2c-N
convention used elsewhere in Linux (sysfs paths, dev nodes); the
slave address keeps its conventional hexadecimal form.
The label is purely informational (visible via gpioinfo and the
gpiochip /sys/class/gpio name); no DT or ABI consumer parses it.
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/hwmon/pmbus/adm1266.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 0dfb02db8683..479e768ff87c 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -292,8 +292,9 @@ static int adm1266_config_gpio(struct adm1266_data *data)
int i;
for (i = 0; i < ARRAY_SIZE(data->gpio_names); i++) {
- gpio_name = devm_kasprintf(&data->client->dev, GFP_KERNEL, "adm1266-%x-%s",
- data->client->addr, adm1266_names[i]);
+ gpio_name = devm_kasprintf(&data->client->dev, GFP_KERNEL, "adm1266-%d-%x-%s",
+ data->client->adapter->nr, data->client->addr,
+ adm1266_names[i]);
if (!gpio_name)
return -ENOMEM;
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH RESEND v2 0/5] hwmon: (pmbus/adm1266) blackbox, FW rev, rtc_class, GPIO label
2026-05-11 7:51 [PATCH RESEND v2 0/5] hwmon: (pmbus/adm1266) blackbox, FW rev, rtc_class, GPIO label Abdurrahman Hussain
` (4 preceding siblings ...)
2026-05-11 7:51 ` [PATCH RESEND v2 5/5] hwmon: (pmbus/adm1266) include adapter number in GPIO line label Abdurrahman Hussain
@ 2026-05-11 14:10 ` Guenter Roeck
2026-05-11 18:34 ` Abdurrahman Hussain
5 siblings, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2026-05-11 14:10 UTC (permalink / raw)
To: Abdurrahman Hussain, Alexandru Tachici; +Cc: linux-hwmon, linux-kernel
On 5/11/26 00:51, Abdurrahman Hussain wrote:
> This series fills in some gaps in the adm1266 driver and removes a
> probe-time RTC seed that was clobbering valid device state.
>
> Patch 1 exposes the firmware revision (IC_DEVICE_REV, 0xAE) via
> debugfs alongside the existing sequencer_state entry.
>
> Patch 2 adds a write-only clear_blackbox debugfs file. Devices
> configured for single-recording mode (BLACKBOX_CONFIG[0] = 0) need
> an explicit clear once the 32-record buffer fills; the documented
> sub-command ({0xFE, 0x00} block-write to 0xDE) wasn't reachable
> from userspace.
>
> Patch 3 exposes the non-volatile POWERUP_COUNTER (0xE4) via debugfs.
> The same value is embedded in every blackbox record, so the live
> value lets userspace match a captured record back to the boot it
> came from when correlating logs.
>
> Patch 4 replaces the probe-time SET_RTC seed with an rtc_class
> device backed by SET_RTC. The existing seed used CLOCK_MONOTONIC
> seconds, which is meaningless as a wall-clock time and clobbers
> whatever the device retained across a warm reboot. The data sheet
> (Rev. D, p. 22) explicitly 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 (no
> external 32.768 kHz crystal). Letting userspace own the policy via
> standard tooling (hwclock, chrony, systemd-timesyncd) against
> /dev/rtcN is both more correct and avoids any driver-specific
> sysfs ABI.
>
> Patch 5 disambiguates GPIO line labels on platforms that fit two
> ADM1266 devices on different I2C buses at the same slave address.
>
> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
> ---
> Changes in v2:
And why the resend ?
Guenter
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH RESEND v2 0/5] hwmon: (pmbus/adm1266) blackbox, FW rev, rtc_class, GPIO label
2026-05-11 14:10 ` [PATCH RESEND v2 0/5] hwmon: (pmbus/adm1266) blackbox, FW rev, rtc_class, GPIO label Guenter Roeck
@ 2026-05-11 18:34 ` Abdurrahman Hussain
0 siblings, 0 replies; 17+ messages in thread
From: Abdurrahman Hussain @ 2026-05-11 18:34 UTC (permalink / raw)
To: Guenter Roeck, Abdurrahman Hussain, Alexandru Tachici
Cc: linux-hwmon, linux-kernel, Guenter Roeck
On Mon May 11, 2026 at 7:10 AM PDT, Guenter Roeck wrote:
> On 5/11/26 00:51, Abdurrahman Hussain wrote:
>> this series fills in some gaps in the adm1266 driver and removes a
>> probe-time rtc seed that was clobbering valid device state.
>>
>
> And why the resend ?
>
> Guenter
Hi Guenter,
I was tinkering with my b4/git/smtp settings trying to avoid sending
over the b4 web endpoint. The `b4 send` went out the same web endpoint
relay again before I realized what happened. Hence the resend.
Apologies for the confusion.
Best regards,
Abdurrahman
^ permalink raw reply [flat|nested] 17+ messages in thread