Re: [PATCH v3 3/5] iio: adc: versal-sysmon: add I2C driver
From: Erim, Salih
Date: Thu May 28 2026 - 18:14:58 EST
Hi Jonathan,
On 28/05/2026 13:42, Jonathan Cameron wrote:
On Wed, 27 May 2026 12:42:09 +0100
Salih Erim <salih.erim@xxxxxxx> wrote:
Add an I2C transport driver for the Versal SysMon block. The SysMonA few minor things inline.
provides an I2C slave interface that allows an external master to
read voltage and temperature measurements through the same register
map used by the MMIO path.
The I2C command frame is an 8-byte structure containing a 4-byte data
payload, a 2-byte register offset, and a 1-byte instruction field.
Read operations send the frame with a read instruction, then receive
a 4-byte response containing the register value.
Events are not supported on the I2C path because there is no
interrupt line and the I2C regmap backend cannot be called from
atomic context.
Co-developed-by: Conall O'Griofa <conall.ogriofa@xxxxxxx>
Signed-off-by: Conall O'Griofa <conall.ogriofa@xxxxxxx>
Signed-off-by: Salih Erim <salih.erim@xxxxxxx>
diff --git a/drivers/iio/adc/versal-sysmon-i2c.c b/drivers/iio/adc/versal-sysmon-i2c.c
new file mode 100644
index 00000000000..92d149f517e
--- /dev/null
+++ b/drivers/iio/adc/versal-sysmon-i2c.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Versal SysMon I2C driver
+ *
+ * Copyright (C) 2023 - 2026, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "versal-sysmon.h"
+
+#define SYSMON_I2C_INSTR_READ BIT(2)
+#define SYSMON_I2C_INSTR_WRITE BIT(3)
+
+#define SYSMON_I2C_DATA0_MASK GENMASK(7, 0)
+#define SYSMON_I2C_DATA1_MASK GENMASK(15, 8)
+#define SYSMON_I2C_DATA2_MASK GENMASK(23, 16)
+#define SYSMON_I2C_DATA3_MASK GENMASK(31, 24)
+
+#define SYSMON_I2C_OFS_LOW_MASK GENMASK(9, 2)
+#define SYSMON_I2C_OFS_HIGH_MASK GENMASK(15, 10)
+
+/* Byte positions within the 8-byte I2C command frame (HW-defined) */
+enum sysmon_i2c_payload_idx {
+ SYSMON_I2C_DATA0_IDX = 0,
+ SYSMON_I2C_DATA1_IDX = 1,
+ SYSMON_I2C_DATA2_IDX = 2,
+ SYSMON_I2C_DATA3_IDX = 3,
+ SYSMON_I2C_OFS_LOW_IDX = 4,
+ SYSMON_I2C_OFS_HIGH_IDX = 5,
With changes suggested below I think you only need the two base
offsets and the final one. As such maybe 3 defines makes more sense
than an enum
Accepted. Will switch to defines.
+ SYSMON_I2C_INSTR_IDX = 6,If you do keep an enum, it would be good to add an entry for the final
byte to give some indication of why it is 8 bytes. Even if that is
reserved0
Switching to defines as above.
+};
+
+static int sysmon_i2c_reg_read(void *context, unsigned int reg,
+ unsigned int *val)
+{
+ struct i2c_client *client = context;
+ u8 write_buf[8] = { };
+ u8 read_buf[4];
+ int ret;
+
+ write_buf[SYSMON_I2C_OFS_LOW_IDX] =
+ FIELD_GET(SYSMON_I2C_OFS_LOW_MASK, reg);
+ write_buf[SYSMON_I2C_OFS_HIGH_IDX] =
+ FIELD_GET(SYSMON_I2C_OFS_HIGH_MASK, reg);
I'd guess an unaligned put works here a well? though you'll need
to do a FIELD_GET() to extract the slightly shifted content.
Accepted.
+ write_buf[SYSMON_I2C_INSTR_IDX] = SYSMON_I2C_INSTR_READ;Very complex way to express what I think is
+
+ ret = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (ret < 0)
+ return ret;
+ if (ret != sizeof(write_buf))
+ return -EIO;
+
+ ret = i2c_master_recv(client, read_buf, sizeof(read_buf));
+ if (ret < 0)
+ return ret;
+ if (ret != sizeof(read_buf))
+ return -EIO;
+
+ *val = FIELD_PREP(SYSMON_I2C_DATA0_MASK,
+ read_buf[SYSMON_I2C_DATA0_IDX]) |
+ FIELD_PREP(SYSMON_I2C_DATA1_MASK,
+ read_buf[SYSMON_I2C_DATA1_IDX]) |
+ FIELD_PREP(SYSMON_I2C_DATA2_MASK,
+ read_buf[SYSMON_I2C_DATA2_IDX]) |
+ FIELD_PREP(SYSMON_I2C_DATA3_MASK,
+ read_buf[SYSMON_I2C_DATA3_IDX]);
*val = get_unaligned_le32(&read_buf[0]);
Accepted.
+
+ return 0;
+}
+
+static int sysmon_i2c_reg_write(void *context, unsigned int reg,
+ unsigned int val)
+{
+ struct i2c_client *client = context;
+ u8 write_buf[8] = { };
+ int ret;
+
+ write_buf[SYSMON_I2C_DATA0_IDX] =
+ FIELD_GET(SYSMON_I2C_DATA0_MASK, val);
+ write_buf[SYSMON_I2C_DATA1_IDX] =
+ FIELD_GET(SYSMON_I2C_DATA1_MASK, val);
+ write_buf[SYSMON_I2C_DATA2_IDX] =
+ FIELD_GET(SYSMON_I2C_DATA2_MASK, val);
+ write_buf[SYSMON_I2C_DATA3_IDX] =
+ FIELD_GET(SYSMON_I2C_DATA3_MASK, val);
That's a put_unaligned_le32() I think?
Accepted.
+ write_buf[SYSMON_I2C_OFS_LOW_IDX] =
+ FIELD_GET(SYSMON_I2C_OFS_LOW_MASK, reg);
+ write_buf[SYSMON_I2C_OFS_HIGH_IDX] =
+ FIELD_GET(SYSMON_I2C_OFS_HIGH_MASK, reg);
I'd guess put_unaligned_le16()? Will need a a FIELD_PREP()
for the full thing though.
Accepted.
+ write_buf[SYSMON_I2C_INSTR_IDX] = SYSMON_I2C_INSTR_WRITE;
+
+ ret = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (ret < 0)
+ return ret;
+ if (ret != sizeof(write_buf))
+ return -EIO;
+
+ return 0;
+}
+static const struct of_device_id sysmon_i2c_of_match_table[] = {
+ { .compatible = "xlnx,versal-sysmon" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, sysmon_i2c_of_match_table);
+
+static const struct i2c_device_id sysmon_i2c_id_table[] = {
+ { "versal-sysmon" },
Named initializer for this. Uwe is cleaning these up across IIO;
let us not add another one!
Accepted. Will use { .name = "versal-sysmon" }.
All items will be addressed in v4.
Salih
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, sysmon_i2c_id_table);
+
+static struct i2c_driver sysmon_i2c_driver = {
+ .probe = sysmon_i2c_probe,
+ .driver = {
+ .name = "versal-sysmon-i2c",
+ .of_match_table = sysmon_i2c_of_match_table,
+ },
+ .id_table = sysmon_i2c_id_table,
+};
+module_i2c_driver(sysmon_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("AMD Versal SysMon I2C Driver");
+MODULE_AUTHOR("Conall O'Griofa <conall.ogriofa@xxxxxxx>");
+MODULE_AUTHOR("Salih Erim <salih.erim@xxxxxxx>");