Re: [PATCH v3 3/5] iio: adc: versal-sysmon: add I2C driver

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 27 May 2026 12:42:09 +0100
Salih Erim <salih.erim@amd.com> wrote:

> Add an I2C transport driver for the Versal SysMon block. The SysMon
> 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@amd.com>
> Signed-off-by: Conall O'Griofa <conall.ogriofa@amd.com>
> Signed-off-by: Salih Erim <salih.erim@amd.com>
A few minor things inline.

> 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

> +	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
> +};
> +
> +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.

> +	write_buf[SYSMON_I2C_INSTR_IDX] = SYSMON_I2C_INSTR_READ;
> +
> +	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]);
Very complex way to express what I think is
	*val = get_unaligned_le32(&read_buf[0]);
> +
> +	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?

> +	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.

> +	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!

> +	{ }
> +};
> +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@amd.com>");
> +MODULE_AUTHOR("Salih Erim <salih.erim@amd.com>");